Re: [Patch v4 5/5] mcpm: exynos: populate suspend and powered_up callbacks

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Tue, May 13, 2014 at 12:43:31PM +0100, Chander Kashyap wrote:

[...]

> >> +static void exynos_suspend(u64 residency)
> >> +{
> >> +     unsigned int mpidr, cpunr;
> >> +
> >> +     mpidr = read_cpuid_mpidr();
> >> +     cpunr = exynos_pmu_cpunr(mpidr);
> >
> > If I were to be picky, I would compute these values only if they
> > are needed, ie move the computation after exynos_power_down().
> 
> Yes thats make sense. I will realign it.
> 
> >
> > There is another quite horrible issue here. We know this code works
> > because the processors A15/A7 hit the caches with C bit in SCTLR cleared.
> >
> > On processors where this is not true, this sequence would explode
> > if power down fails (in case core is gated but L2 is still powered on,
> > the stack is stuck in L2) since it is going to read stack data that is
> > in L2 but can't be read.
> >
> > It is not related to this sequence only, but it is an issue in general
> > and wanted to mention that on the lists for public awareness.
> >
> 
> Can you please elaborate. I didn't understand.

It is not related to this patch only. This function carries out writes to the
stack (which might end up in eg L2) and then disables the C bit in SCTLR
through MCPM.

A15 and A7 processors hit the cache with the C bit clear in the SCTLR
so the processor still "hits" the stack values if the power down fails.
On processors where caches are not hit with the C bit clear (eg A9) this code
would fail since the stack values that sit in the caches cannot be read with
the C bit clear in SCTLR until the SCTLR is restored, so it will have to
be implemented in assembly with no stack usage (or better, no cacheable data
usage).

So, all I am saying is, to avoid copy'n'paste havoc and to avoid running
this code on Exynos platforms where it must not be run as-is, please add
a comment along the line:

"This function requires the stack data to be visible through power down
and can only be executed on processors like A15 and A7 that hit the cache
with the C bit clear in the SCTLR register."

Please let me know if that's clear.

Lorenzo

> 
> > The gist of what I am saying is, please add a comment to that extent,
> > here and it should be added in exynos_power_down() too.
> >
> >> +     __raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 0x1c);
> >
> > No magic numbers please (0x1c). You can add a macro/wrapper, as TC2 does.
> 
> Yes i will remove it.
> 
> >
> >> +     exynos_power_down();
> >> +
> >> +     /*
> >> +      * Execution reaches here only if cpu did not power down.
> >> +      * Hence roll back the changes done in exynos_power_down function.
> >> +     */
> >> +     exynos_cpu_powerup(cpunr);
> >
> > Please be aware that if this function returns MCPM will soft reboot, and
> > the CPUidle driver will have no way to detect a state entry failure.
> >
> > I am just flagging this up, since fixing this behaviour is not easy, and
> > honestly, since power down failure should be the exception not the rule,
> > the idle stats should not be affected much.
> >
> > I think this is the proper way of implementing the sequence but please
> > all keep in mind what I wrote above.
> >
> > Lorenzo
> >
> >> +}
> >> +
> >>  static const struct mcpm_platform_ops exynos_power_ops = {
> >>       .power_up               = exynos_power_up,
> >>       .power_down             = exynos_power_down,
> >>       .power_down_finish      = exynos_power_down_finish,
> >> +     .suspend                = exynos_suspend,
> >> +     .powered_up             = exynos_powered_up,
> >>  };
> >>
> >>  static void __init exynos_mcpm_usage_count_init(void)
> >> --
> >> 1.7.9.5
> >>
> >>
> >
> 
> 
> 
> -- 
> with warm regards,
> Chander Kashyap
>