several messages

[Abhilash Kesavan]

Hi Nicolas,

On Fri, Jul 4, 2014 at 9:43 AM, Nicolas Pitre <nicolas.pitre at linaro.org> wrote:
> On Fri, 4 Jul 2014, Abhilash Kesavan wrote:
>
>> Hi Nicolas,
>>
>> On Fri, Jul 4, 2014 at 12:30 AM, Nicolas Pitre <nicolas.pitre at linaro.org> wrote:
>> > On Thu, 3 Jul 2014, Abhilash Kesavan wrote:
>> >
>> >> Hi Nicolas,
>> >>
>> >> On Thu, Jul 3, 2014 at 9:15 PM, Nicolas Pitre <nicolas.pitre at linaro.org> wrote:
>> >> > On Thu, 3 Jul 2014, Abhilash Kesavan wrote:
>> >> >
>> >> >> On Thu, Jul 3, 2014 at 6:59 PM, Nicolas Pitre <nicolas.pitre at linaro.org> wrote:
>> >> >> > 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.
>> >> >
>> >> > [...]
>> >> >
>> >> >> Changes in v5:
>> >> >>       - Removed the MCPM flags and just used a local flag to
>> >> >>       indicate that we are suspending.
>> >> >
>> >> > [...]
>> >> >
>> >> >> -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);
>> >> >> +}
>> >> >
>> >> > [...]
>> >> >
>> >> >> +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?
>> exynos pm code is shared across Exynos4 and 5 SoCs. It primarily does
>> a series of register configurations which are required to put the
>> system to sleep (some parts of these are SoC specific and others
>> common). It also populates the suspend_ops for exynos. In the current
>> patch, exynos_suspend_enter() is where I have plugged in the
>> mcpm_cpu_suspend call.
>>
>> This patch is based on the S2R series for 5420
>> (http://comments.gmane.org/gmane.linux.kernel.samsung-soc/33898), you
>> may also have a look at that for a clearer picture.
>> >
>> >> 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.
>> OK, I will use this rather than the S5P_CHECK_SLEEP macro.
>
> Another suggestion which might possibly be better: why not looking for
> the SYS_PWR_CFG bit in exynos_cpu_power_down() directly?  After all,
> exynos_cpu_power_down() is semantically supposed to do what its name
> suggest and could simply do nothing if the proper conditions are already
> in place.
I have implemented this and it works fine. Patch coming up.

Regards,
Abhilash
>
>
> Nicolas