Hi Nicolas,
On Wed, Apr 30, 2014 at 8:13 PM, Nicolas Pitre <nicolas.pitre@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxxx>
>> Signed-off-by: Inderpal Singh <inderpal.s@xxxxxxxxxxx>
>> Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
>> Signed-off-by: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
>
> 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.
OK.
>
>> + }
>> +
>> + /* 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.
Will move the comment.
>
>> + 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.
OK.
>
>> + }
>> + }
>> +
>> + 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().
Have removed these.
>
>> + } 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.
>
Thanks,
Abhilash
>
> Nicolas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
