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

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

[qianfanguijin]

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

Fixed sunxi_power_switch based on allwinner lichee 3.10 kernel driver.

Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
---
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);
 }
 #else /* ! CONFIG_MACH_SUN7I && ! CONFIG_MACH_SUN8I_R40 */
 static void __secure sunxi_cpu_set_power(int cpu, bool on)
-- 
2.25.1

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

[Chen-Yu Tsai]

Hi,

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

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.


Regards
ChenYu

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

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

[qianfan]

在 2022/5/14 11:52, Chen-Yu Tsai 写道:
> Hi,
>
> 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
>>
>> 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.
>
> 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.
Just change the last params of sunxi_power_switch can also work fine.

diff --git a/arch/arm/cpu/armv7/sunxi/psci.c 
b/arch/arm/cpu/armv7/sunxi/psci.c
index 1ac50f558a..ae21879e69 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -153,7 +153,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);
  }
  #else /* ! CONFIG_MACH_SUN7I && ! CONFIG_MACH_SUN8I_R40 */
  static void __secure sunxi_cpu_set_power(int cpu, bool on)

The readback might as same as __mdelay.

But I can't make sure the 0xfe, 0x80 ... 0x00 sequence number when 
clame_release.
>
>
> Regards
> ChenYu
>
>>   }
>>   #else /* ! CONFIG_MACH_SUN7I && ! CONFIG_MACH_SUN8I_R40 */
>>   static void __secure sunxi_cpu_set_power(int cpu, bool on)
>> --
>> 2.25.1
>>

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

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

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

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

[Andre Przywara]

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

Hi,

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

Reduced the patch to this one change, adjusted and extended the commit
message, and applied to sunxi/master, for v2022.07.

Thanks,
Andre

> 
> 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.
> 
> 
> Regards
> ChenYu
> 
> >  }
> >  #else /* ! CONFIG_MACH_SUN7I && ! CONFIG_MACH_SUN8I_R40 */
> >  static void __secure sunxi_cpu_set_power(int cpu, bool on)
> > --
> > 2.25.1
> >  

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

[qianfan]

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