several messages

On Thu, 3 Jul 2014, Abhilash Kesavan wrote:

Hi Nicolas,

On Thu, Jul 3, 2014 at 9:15 PM, Nicolas Pitre [off-list ref] wrote:

quoted

On Thu, 3 Jul 2014, Abhilash Kesavan wrote:

quoted

On Thu, Jul 3, 2014 at 6:59 PM, Nicolas Pitre [off-list ref] wrote:

quoted

Please, let's avoid going that route.  There is no such special handling
needed if the API is sufficient.  And the provided API allows you to
suspend a CPU or shut it down.  It shouldn't matter at that level if
this is due to a cluster switch or a hotplug event. Do you really need
something else?

No, just one local flag for suspend should be enough for me. Will remove these.

[...]

quoted

Changes in v5:
      - Removed the MCPM flags and just used a local flag to
      indicate that we are suspending.

[...]

quoted

-static void exynos_power_down(void)
+static void exynos_mcpm_power_down(u64 residency)
 {
      unsigned int mpidr, cpu, cluster;
      bool last_man = false, skip_wfi = false;

@@ -150,7 +153,12 @@ static void exynos_power_down(void)
      BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
      cpu_use_count[cpu][cluster]--;
      if (cpu_use_count[cpu][cluster] == 0) {
-             exynos_cpu_power_down(cpunr);
+             /*
+              * Bypass power down for CPU0 during suspend. This is being
+              * taken care by the SYS_PWR_CFG bit in CORE0_SYS_PWR_REG.
+              */
+             if ((cpunr != 0) || (residency != S5P_CHECK_SLEEP))
+                     exynos_cpu_power_down(cpunr);

              if (exynos_cluster_unused(cluster)) {
                      exynos_cluster_power_down(cluster);

@@ -209,6 +217,11 @@ static void exynos_power_down(void)
      /* Not dead at this point?  Let our caller cope. */
 }

+static void exynos_power_down(void)
+{
+     exynos_mcpm_power_down(0);
+}

[...]

quoted

+static int notrace exynos_mcpm_cpu_suspend(unsigned long arg)
+{
+     /* MCPM works with HW CPU identifiers */
+     unsigned int mpidr = read_cpuid_mpidr();
+     unsigned int cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+     unsigned int cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+
+     __raw_writel(0x0, sysram_base_addr + EXYNOS5420_CPU_STATE);
+
+     mcpm_set_entry_vector(cpu, cluster, exynos_cpu_resume);
+
+     /*
+      * Pass S5P_CHECK_SLEEP flag to the MCPM back-end to indicate that
+      * we are suspending the system and need to skip CPU0 power down.
+      */
+     mcpm_cpu_suspend(S5P_CHECK_SLEEP);

NAK.

When I say "local flag with local meaning", I don't want you to smuggle
that flag through a public interface either.  I find it rather inelegant
to have the notion of special handling for CPU0 being spread around like
that.

If CPU0 is special, then it should be dealth with in one place only,
ideally in the MCPM backend, so the rest of the kernel doesn't have to
care.

Again, here's what I mean:

static void exynos_mcpm_down_handler(int flags)
{
        [...]
        if ((cpunr != 0) || !(flags & SKIP_CPU_POWERDOWN_IF_CPU0))
                exynos_cpu_power_down(cpunr);
        [...]
}

static void exynos_mcpm_power_down()
{
        exynos_mcpm_down_handler(0);
}

static void exynos_mcpm_suspend(u64 residency)
{
        /*
         * Theresidency argument is ignored for now.
         * However, in the CPU suspend case, we bypass power down for
         * CPU0 as this is being taken care by the SYS_PWR_CFG bit in
         * CORE0_SYS_PWR_REG.
         */
        exynos_mcpm_down_handler(SKIP_CPU_POWERDOWN_IF_CPU0);
}

And SKIP_CPU_POWERDOWN_IF_CPU0 is defined in and visible to
mcpm-exynos.c only.

Sorry if I am being dense, but the exynos_mcpm_suspend function would
get called from both the bL cpuidle driver as well the exynos pm code.

What is that exynos pm code actually doing?

We want to skip CPU0 only in case of the S2R case i.e. the pm code and
keep the CPU0 power down code for all other cases including CPUIdle.

OK.  If so I missed that somehow (hint hint).

If I call exynos_mcpm_down_handler with the flag in
exynos_mcpm_suspend(), CPUIdle will also skip CPU0 isn't it ?

As it is, yes.  You could logically use an infinite residency time 
(something like U64_MAX) to distinguish S2RAM from other types of 
suspend.

Yet, why is this SYS_PWR_CFG bit set outside of MCPM?  Couldn't the MCPM 
backend handle it directly instead of expecting some other entity to do 
it?


Nicolas