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

[qianfan <qianfanguijin@163.com>]

在 2022/6/25 8:51, Andre Przywara 写道:
> 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.
Next is the scripts I used:

unfortunately it also failed.

#!/bin/sh
i=1

while true ; do
     # cpu0 can't offline, skip it
     cpu=$((RANDOM%3))
     let cpu++

     # togger online status
     online=$(cat /sys/devices/system/cpu/cpu${cpu}/online)
     if [ ${online} -eq 0 ] ; then
         online=1
     else
         online=0
     fi

     echo -n -e "${i}\tcpu${cpu} ${online}\t"
     echo ${online} > /sys/devices/system/cpu/cpu${cpu}/online
     echo "cpu onlines: $(cat /sys/devices/system/cpu/online)"

     let i++
done

>
> 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
>>>