Re: [PATCH v2] sunxi: psci: Fix sunxi_power_switch on sun8i-r40 platform

[Andre Przywara <andre.przywara@arm.com>]

On Sat, 14 May 2022 11:52:01 +0800
Chen-Yu Tsai <wens@kernel.org> wrote:

Hi Qianfan,

> On Sat, May 14, 2022 at 11:19 AM <qianfanguijin@163.com> wrote:
> >
> > From: qianfan Zhao <qianfanguijin@163.com>
> >
> > linux system will die if we offline one of the cpu on R40 based board:
> > eg: echo 0 > /sys/devices/system/cpu/cpu3/online

Thanks for bringing this up: indeed CPU offlining does not work on the
R40 at the moment.
More below ...

> >
> > Fixed sunxi_power_switch based on allwinner lichee 3.10 kernel driver.
> >
> > Signed-off-by: qianfan Zhao <qianfanguijin@163.com>  
> 
> Please add a Fixes tag.
> 
> > ---
> > v2 changes: Fix the commit message, the source code doesn't change.
> >
> >  arch/arm/cpu/armv7/sunxi/psci.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> > index 1ac50f558a..63186a9388 100644
> > --- a/arch/arm/cpu/armv7/sunxi/psci.c
> > +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> > @@ -79,8 +79,7 @@ static void __secure __mdelay(u32 ms)
> >  static void __secure clamp_release(u32 __maybe_unused *clamp)
> >  {
> >  #if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN7I) || \
> > -       defined(CONFIG_MACH_SUN8I_H3) || \
> > -       defined(CONFIG_MACH_SUN8I_R40)
> > +       defined(CONFIG_MACH_SUN8I_H3)
> >         u32 tmp = 0x1ff;
> >         do {
> >                 tmp >>= 1;
> > @@ -88,15 +87,30 @@ static void __secure clamp_release(u32 __maybe_unused *clamp)
> >         } while (tmp);
> >
> >         __mdelay(10);
> > +#elif defined(CONFIG_MACH_SUN8I_R40)
> > +       u8 i, tmp = 0xfe;
> > +
> > +       for (i = 0; i < 5; i++) { /* 0xfe, 0xf8, 0xe0, 0x80, 0x00 */
> > +               writel(tmp, clamp);
> > +               tmp <<= 2;
> > +       }
> > +
> > +       while (0x00 != readl(clamp)) {
> > +               ;
> > +       }
> >  #endif
> >  }
> >
> >  static void __secure clamp_set(u32 __maybe_unused *clamp)
> >  {
> >  #if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN7I) || \
> > -       defined(CONFIG_MACH_SUN8I_H3) || \
> > -       defined(CONFIG_MACH_SUN8I_R40)
> > +       defined(CONFIG_MACH_SUN8I_H3)
> >         writel(0xff, clamp);
> > +#elif defined(CONFIG_MACH_SUN8I_R40)
> > +       writel(0xff, clamp);
> > +       while (0xff != readl(clamp)) {
> > +               ;
> > +       }
> >  #endif
> >  }
> >
> > @@ -153,7 +167,7 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
> >
> >         sunxi_power_switch((void *)cpucfg + SUN8I_R40_PWR_CLAMP(cpu),
> >                            (void *)cpucfg + SUN8I_R40_PWROFF,
> > -                          on, 0);
> > +                          on, cpu);  
> 
> I think this is the only change that is needed. Looking again at the
> R40 user manual, the original code turned off core 0 regardless of
> which core was being brought down.

Yes, I agree here, that just always turns off core 0 :-(
I wrote to the PWROFF registers from U-Boot, all four registers +0x120,
+0x124, +0x128, +0x12c exist and store only the lowest 8 bits, the next
four words are not implemented on the R40. So this makes it very likely
that those are the indeed the PWROFF registers for the four cores, even
though the manual only mentions two.

> Could you give that a try? The power clamp stuff shouldn't change
> much, as the end result is the same. The readback might make a
> difference, but if it does, it should be applied to all SoCs.

So indeed just the change above *seems* to work, although it still
didn't survive my CPU on/offline test:
# for i in $(seq 1 100); do echo $((RANDOM%2)) >
/sys/devices/system/cpu/cpu$((RANDOM%4))/online; done
This hangs halfway through.

Applying the other changes didn't make a difference: I tried just
the read-back, read-back + delay, the "0xfe, 0xf8, 0xe0, 0x80, 0x00"
sequence, and combinations. Also mdelay(100) didn't help.
Not sure if this is power sequence related, or a separate bug, maybe in
U-Boot's PSCI implementation? I will try to debug this further, also on
other SoCs.

But for now I am tempted to take this one-line change, as this looks
like an obvious bug and the fix definitely improves the situation.

Thanks!
Andre

> >  }
> >  #else /* ! CONFIG_MACH_SUN7I && ! CONFIG_MACH_SUN8I_R40 */
> >  static void __secure sunxi_cpu_set_power(int cpu, bool on)
> > --
> > 2.25.1
> >