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

[Abhilash Kesavan]

Hi Dave,

On Thu, Apr 17, 2014 at 3:33 PM, Dave Martin <Dave.Martin at arm.com> wrote:
> On Thu, Apr 17, 2014 at 12:41:31AM +0530, Abhilash Kesavan wrote:
>> Hi Dave,
>>
>> On Tue, Apr 15, 2014 at 2:06 PM, Dave Martin <Dave.Martin at arm.com> wrote:
>> > On Mon, Apr 14, 2014 at 10:01:27AM -0400, Nicolas Pitre wrote:
>> >> On Mon, 14 Apr 2014, Dave Martin wrote:
>> >>
>> >> > On Fri, Apr 11, 2014 at 04:23:04PM -0400, Nicolas Pitre wrote:
>> >> > > On Fri, 11 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>
>> >> > >
>> >> > > See comments inline.
>> >> >
>> >> > I won't duplicate Nico's review, but I have a couple of extra comments/
>> >> > questions, below.
>> >> >
>> >> > [...]
>> >> >
>> >> > > > diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
>> >> > > > new file mode 100644
>> >> > > > index 0000000..46d4968
>> >> > > > --- /dev/null
>> >> > > > +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>> >> > > > @@ -0,0 +1,444 @@
>> >> >
>> >> > [...]
>> >> >
>> >> > > > +static void exynos_cluster_power_control(unsigned int cluster, int enable)
>> >> > > > +{
>> >> > > > +       unsigned long timeout = jiffies + msecs_to_jiffies(20);
>> >> > > > +       unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
>> >> > > > +
>> >> > > > +       if (enable)
>> >> > > > +               val = EXYNOS_CORE_LOCAL_PWR_EN;
>> >> > > > +
>> >> > > > +       status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
>> >> > > > +       if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
>> >> > > > +               return;
>> >> > > > +
>> >> > > > +       __raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
>> >> > > > +       /* Wait until cluster power control is applied */
>> >> > > > +       while (time_before(jiffies, timeout)) {
>> >> > > > +               status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
>> >> > > > +
>> >> > > > +               if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
>> >> > > > +                       return;
>> >> > > > +
>> >> > > > +               cpu_relax();
>> >> > > > +       }
>> >> > > > +       pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
>> >> > > > +               enable ? "on" : "off");
>> >> > >
>> >> > > You should not have to wait for the power status to change here.
>> >> > > Simply signaling the desired state and returning is all that is
>> >> > > expected.  And because IRQs are turned off, it is likely that
>> >> > > time_before(jiffies, timeout) will always be true anyway because jiffies
>> >> > > are not updated if there is no other CPU to service the timer interrupt.
>> >> > >
>> >> > > The actual power status should be polled for in the mcpm_finish()
>> >> > > method only.
>> >> >
>>
>> >> > Depending on the power controller, it might be possible for writes to
>> >> > the controller to be lost or not acted upon, if a previous change is
>> >> > still pending.
>> >> >
>> >> > Does this issue apply to the exynos power controller?
>> >>
>> >> Given the way the code is structured now, I suppose that has not been
>> >> the case so far.
>> >
>> > It depends on the purpose of the polling loop.  If it was added to resolve
>> > a race between a power-up and a power-down that is complete in MCPM but
>> > still pending in the hardware, than that would suggest the power controller
>> > does have this behaviour.
>> >
>> > But I'm just guessing.
>> >
>> > Abhilash, can you comment?
>> The polling loop in the above function was added to ensure that the
>> cluster is switched on before we power on the individual cores in
>> exynos_power_up. This is only required when the first man comes up.
>
> Is this only required on power-up, not power-down?  I don't see a call
> to exynos_cluster_power_control(..., 0), but maybe I'm looking in the
> wrong place.
No, you have not missed it. As of now, we are turning on both the L2s
for 8 core bring-up and then not turning it off (if all 4 cores in the
clusters are down). Turning the L2 off (A15 L2 mainly) brings in
significant power savings and I will be looking at adding support for
this next.
>
> I wonder whether this is needed, though.  It imposes a requirement on
> the OS to waste time polling for the cluster to come online.  Can the
> hardware not cope with a CPU poweron request being pending already before
> the cluster is powered on?  If that were possible, there would be no need
> for an extra poll.
No, we need to ensure that the cluster L2 is turned On before we can
turn the core on, the hardware does not handle it.
>
> How long do you expect to need to poll for?
In internal testing when we are turning the cluster on from an off
state, I have seen the loop being executed 3-4 times occasionally
during ageing tests. Generally, the status returns as being powered ON
immediately.

Regards,
Abhilash
>
> Cheers
> ---Dave