[PATCH v3 5/6] arm: exynos: Add MCPM call-back functions

[Nicolas Pitre]

On Sat, 26 Apr 2014, Abhilash Kesavan wrote:

> Add machine-dependent MCPM call-backs for Exynos5420. These are used
> to power up/down the secondary CPUs during boot, shutdown, s2r and
> switching.
> 
> Signed-off-by: Thomas Abraham <thomas.ab at samsung.com>
> Signed-off-by: Inderpal Singh <inderpal.s at samsung.com>
> Signed-off-by: Andrew Bresticker <abrestic at chromium.org>
> Signed-off-by: Abhilash Kesavan <a.kesavan at samsung.com>

Small comments below.

> +static int exynos_cluster_power_control(unsigned int cluster, int enable)
> +{
> +	unsigned int tries = 100;
> +	unsigned int val = 0;
> +
> +	if (enable) {
> +		exynos_cluster_powerup(cluster);
> +		val = S5P_CORE_LOCAL_PWR_EN;
> +	} else {
> +		exynos_cluster_powerdown(cluster);

It would be clearer if you have "val = 0" here instead so it looks 
symetric.

> +	}
> +
> +	/* Wait until cluster power control is applied */
> +	while (tries--) {
> +		if (exynos_cluster_power_state(cluster) == val)
> +			return 0;
> +
> +		cpu_relax();
> +	}
> +	pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
> +		enable ? "on" : "off");
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int exynos_power_up(unsigned int cpu, unsigned int cluster)
> +{
> +	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
> +	unsigned int err;
> +
> +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
> +		cluster >= EXYNOS5420_NR_CLUSTERS)
> +		return -EINVAL;
> +
> +	/*
> +	 * Since this is called with IRQs enabled, and no arch_spin_lock_irq
> +	 * variant exists, we need to disable IRQs manually here.
> +	 */
> +	local_irq_disable();
> +	arch_spin_lock(&exynos_mcpm_lock);
> +
> +	cpu_use_count[cpu][cluster]++;
> +	if (cpu_use_count[cpu][cluster] == 1) {
> +		bool was_cluster_down =
> +			__mcpm_cluster_state(cluster) == CLUSTER_DOWN;
> +
> +		/*
> +		 * Turn on the cluster (L2/COMMON) and then power on the cores.
> +		 * TODO: Turn off the clusters when all cores in the cluster
> +		 * are down to achieve significant power savings.
> +		 */

This TODO comment should be moved in exynos_power_down() were last_man 
is determined to be true.

> +		if (was_cluster_down) {
> +			err = exynos_cluster_power_control(cluster, 1);
> +			if (err) {
> +				exynos_cluster_power_control(cluster, 0);
> +				return err;

The lock is still held.

To make the code clearer, you should do:

			if (!err)
				exynos_cpu_powerup(cpunr);

and return err at the end.

> +			}
> +		}
> +
> +		exynos_cpu_powerup(cpunr);
> +
> +		/* CPU should be powered up there */
> +		/* Also setup Cluster Power Sequence here */

Both comments are wrong.  The CPU is not necessarily powered up yet at 
this point, and "Cluster Power Sequence" (whatever that is) should not 
happen here but in the callback provided to mcpm_sync_init().

> +	} else if (cpu_use_count[cpu][cluster] != 2) {
> +		/*
> +		 * The only possible values are:
> +		 * 0 = CPU down
> +		 * 1 = CPU (still) up
> +		 * 2 = CPU requested to be up before it had a chance
> +		 *     to actually make itself down.
> +		 * Any other value is a bug.
> +		 */
> +		BUG();
> +	}
> +
> +	arch_spin_unlock(&exynos_mcpm_lock);
> +	local_irq_enable();
> +
> +	return 0;
> +}
> +
> +static void exynos_power_down(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +	bool last_man = false, skip_wfi = false;
> +	unsigned int cpunr;
> +
> +	mpidr = read_cpuid_mpidr();
> +	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +	cpunr =  cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
> +
> +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +	BUG_ON(cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
> +			cluster >= EXYNOS5420_NR_CLUSTERS);
> +
> +	__mcpm_cpu_going_down(cpu, cluster);
> +
> +	arch_spin_lock(&exynos_mcpm_lock);
> +	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> +	cpu_use_count[cpu][cluster]--;
> +	if (cpu_use_count[cpu][cluster] == 0) {
> +		int cnt = 0, i;
> +
> +		exynos_cpu_powerdown(cpunr);
> +		for (i = 0; i < EXYNOS5420_CPUS_PER_CLUSTER; i++)
> +			cnt += cpu_use_count[i][cluster];
> +		if (cnt == 0)
> +			last_man = true;

I think Lorenzo commented on this code block already.

Otherwise it is almost there.


Nicolas