[v2,4/5] sunxi: psci: implement PSCI on R528

This patch adds the necessary code to make nonsec booting and PSCI
secondary core management functional on the R528/T113.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
Tested-by: Maksim Kiselev <bigunclemax@gmail.com>
---
 arch/arm/cpu/armv7/sunxi/psci.c | 48 ++++++++++++++++++++++++++++++++-
 arch/arm/mach-sunxi/Kconfig     |  2 ++
 include/configs/sunxi-common.h  |  8 ++++++
 3 files changed, 57 insertions(+), 1 deletion(-)

On Wed, 16 Aug 2023 10:34:19 -0700
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

> This patch adds the necessary code to make nonsec booting and PSCI
> secondary core management functional on the R528/T113.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> Tested-by: Maksim Kiselev <bigunclemax@gmail.com>
> ---
>  arch/arm/cpu/armv7/sunxi/psci.c | 48 ++++++++++++++++++++++++++++++++-
>  arch/arm/mach-sunxi/Kconfig     |  2 ++
>  include/configs/sunxi-common.h  |  8 ++++++
>  3 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index 6ecdd05250..b4ce4f6def 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -47,6 +47,19 @@
>  #define SUN8I_R40_PWR_CLAMP(cpu)		(0x120 + (cpu) * 0x4)
>  #define SUN8I_R40_SRAMC_SOFT_ENTRY_REG0		(0xbc)
>  
> +/*
> + * R528 is also different, as it has both cores powered up (but held in reset
> + * state) after the SoC is reset. Like the R40, it uses a "soft" entry point
> + * address register, but unlike the R40, it uses a newer "CPUX" block to manage
> + * CPU state, rather than the older CPUCFG system.
> + */
> +#define SUN8I_R528_SOFT_ENTRY			(0x1c8)
> +#define SUN8I_R528_C0_RST_CTRL			(0x0000)
> +#define SUN8I_R528_C0_CTRL_REG0			(0x0010)
> +#define SUN8I_R528_C0_CPU_STATUS		(0x0080)
> +
> +#define SUN8I_R528_C0_STATUS_STANDBYWFI		(16)
> +
>  static void __secure cp15_write_cntp_tval(u32 tval)
>  {
>  	asm volatile ("mcr p15, 0, %0, c14, c2, 0" : : "r" (tval));
> @@ -103,10 +116,13 @@ static void __secure clamp_set(u32 *clamp)
>  
>  static void __secure sunxi_cpu_set_entry(int __always_unused cpu, void *entry)
>  {
> -	/* secondary core entry address is programmed differently on R40 */
> +	/* secondary core entry address is programmed differently on R40/528 */

I think that's somewhat obvious now from the code, so you can remove this
comment.

>  	if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
>  		writel((u32)entry,
>  		       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		writel((u32)entry,
> +		       SUNXI_R_CPUCFG_BASE + SUN8I_R528_SOFT_ENTRY);
>  	} else {
>  		writel((u32)entry, SUNXI_CPUCFG_BASE + SUNXI_PRIV0);
>  	}
> @@ -124,6 +140,9 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
>  	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
>  		clamp = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWR_CLAMP(cpu);
>  		pwroff = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWROFF;
> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		/* R528 leaves both cores powered up, manages them via reset */
> +		return;
>  	} else {
>  		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
>  		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))
> @@ -151,11 +170,27 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
>  
>  static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
>  {
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		if (reset) {

I think you can lose the brackets here, since it's a single statement
branch, even if it spans multiple lines. The indentation should make this
clear.

> +			clrbits_le32(SUNXI_CPUCFG_BASE + SUN8I_R528_C0_RST_CTRL,
> +				     BIT(cpu));
> +		} else {
> +			setbits_le32(SUNXI_CPUCFG_BASE + SUN8I_R528_C0_RST_CTRL,
> +				     BIT(cpu));
> +		}
> +		return;
> +	}
> +
>  	writel(reset ? 0b00 : 0b11, SUNXI_CPUCFG_BASE + SUNXI_CPU_RST(cpu));
>  }
>  
>  static void __secure sunxi_cpu_set_locking(int cpu, bool lock)
>  {
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		/* Not required on R528 */
> +		return;
> +	}
> +
>  	if (lock)
>  		clrbits_le32(SUNXI_CPUCFG_BASE + SUNXI_DBG_CTRL1, BIT(cpu));
>  	else
> @@ -164,11 +199,22 @@ static void __secure sunxi_cpu_set_locking(int cpu, bool lock)
>  
>  static bool __secure sunxi_cpu_poll_wfi(int cpu)
>  {
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		return !!(readl(SUNXI_CPUCFG_BASE + SUN8I_R528_C0_CPU_STATUS) &
> +			  BIT(SUN8I_R528_C0_STATUS_STANDBYWFI + cpu));
> +	}
> +
>  	return !!(readl(SUNXI_CPUCFG_BASE + SUNXI_CPU_STATUS(cpu)) & BIT(2));
>  }
>  
>  static void __secure sunxi_cpu_invalidate_cache(int cpu)
>  {
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		clrbits_le32(SUNXI_CPUCFG_BASE + SUN8I_R528_C0_CTRL_REG0,
> +			     BIT(cpu));
> +		return;
> +	}
> +
>  	clrbits_le32(SUNXI_CPUCFG_BASE + SUNXI_GEN_CTRL, BIT(cpu));
>  }
>  
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 0a3454a51a..d46fd8c0bc 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -355,6 +355,8 @@ config MACH_SUN8I_R40
>  config MACH_SUN8I_R528
>  	bool "sun8i (Allwinner R528)"
>  	select CPU_V7A
> +	select CPU_V7_HAS_NONSEC
> +	select ARCH_SUPPORT_PSCI

Please add
	select CPU_V7_HAS_VIRT
here, as the cores are perfectly capable of virtualisation. Granted,
support for KVM is long gone from Linux, but at least Xen still supports it.
And I believe you also need:
	select SPL_ARMV7_SET_CORTEX_SMPEN
At least this is what the other cores do. The PSCI code sets this bit for
the secondaries, but for the primary core we need to set it as early as
possible. Probably not a biggie on an A7, in reality, but good to have,
and be it for correctness and consistency's sake.

>  	select SUNXI_GEN_NCAT2
>  	select SUNXI_NEW_PINCTRL
>  	select MMC_SUNXI_HAS_NEW_MODE
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index b8ca77d031..67eb0d25db 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -33,6 +33,14 @@
>  
>  /* CPU */
>  
> +/*
> + * Newer ARM SoCs have moved the GIC, but have not updated their ARM cores to
> + * reflect the correct address in CBAR/PERIPHBASE.
> + */
> +#if defined(CONFIG_SUN50I_GEN_H6) || defined(CONFIG_SUNXI_GEN_NCAT2)
> +#define CFG_ARM_GIC_BASE_ADDRESS	0x03020000
> +#endif

I feel this should go into Kconfig. I can make a patch, unless you want to
beat me to it.

Cheers,
Andre

> +
>  /*
>   * The DRAM Base differs between some models. We cannot use macros for the
>   * CONFIG_FOO defines which contain the DRAM base address since they end

On 9/27/23 10:31, Andre Przywara wrote:
> On Wed, 16 Aug 2023 10:34:19 -0700
> Sam Edwards <cfsworks@gmail.com> wrote:
> 
> Hi Sam,

Hi Andre,

>> @@ -103,10 +116,13 @@ static void __secure clamp_set(u32 *clamp)
>>   
>>   static void __secure sunxi_cpu_set_entry(int __always_unused cpu, void *entry)
>>   {
>> -	/* secondary core entry address is programmed differently on R40 */
>> +	/* secondary core entry address is programmed differently on R40/528 */
> 
> I think that's somewhat obvious now from the code, so you can remove this
> comment.

Done, change will be included in v3.

>>   	if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
>>   		writel((u32)entry,
>>   		       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
>> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
>> +		writel((u32)entry,
>> +		       SUNXI_R_CPUCFG_BASE + SUN8I_R528_SOFT_ENTRY);
>>   	} else {
>>   		writel((u32)entry, SUNXI_CPUCFG_BASE + SUNXI_PRIV0);
>>   	}
>> @@ -124,6 +140,9 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
>>   	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
>>   		clamp = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWR_CLAMP(cpu);
>>   		pwroff = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWROFF;
>> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
>> +		/* R528 leaves both cores powered up, manages them via reset */
>> +		return;
>>   	} else {
>>   		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
>>   		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))
>> @@ -151,11 +170,27 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
>>   
>>   static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
>>   {
>> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
>> +		if (reset) {
> 
> I think you can lose the brackets here, since it's a single statement
> branch, even if it spans multiple lines. The indentation should make this
> clear.

FWIW a lot of reviewers insist on braces surrounding *any* multiline 
blocks, even if said block is only a single statement. This is to 
prevent mishaps where another developer comes along later to add another 
statement to the same block (at the same indentation level), but doesn't 
think to look for missing brackets because the block is already bigger 
than one line.

I could go either way on it, but would like to be sure that your 
feedback stands in light of that counterpoint.

>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
>> index 0a3454a51a..d46fd8c0bc 100644
>> --- a/arch/arm/mach-sunxi/Kconfig
>> +++ b/arch/arm/mach-sunxi/Kconfig
>> @@ -355,6 +355,8 @@ config MACH_SUN8I_R40
>>   config MACH_SUN8I_R528
>>   	bool "sun8i (Allwinner R528)"
>>   	select CPU_V7A
>> +	select CPU_V7_HAS_NONSEC
>> +	select ARCH_SUPPORT_PSCI
> 
> Please add
> 	select CPU_V7_HAS_VIRT
> here, as the cores are perfectly capable of virtualisation. Granted,
> support for KVM is long gone from Linux, but at least Xen still supports it.

Good catch; will be done in v3.

> And I believe you also need:
> 	select SPL_ARMV7_SET_CORTEX_SMPEN
> At least this is what the other cores do. The PSCI code sets this bit for
> the secondaries, but for the primary core we need to set it as early as
> possible. Probably not a biggie on an A7, in reality, but good to have,
> and be it for correctness and consistency's sake.

That's already enabled down below:
# The sun8i SoCs share a lot, this helps to avoid a lot of "if A23 || A33"
config MACH_SUN8I
         bool
         select SPL_ARMV7_SET_CORTEX_SMPEN if !ARM64

>> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
>> index b8ca77d031..67eb0d25db 100644
>> --- a/include/configs/sunxi-common.h
>> +++ b/include/configs/sunxi-common.h
>> @@ -33,6 +33,14 @@
>>   
>>   /* CPU */
>>   
>> +/*
>> + * Newer ARM SoCs have moved the GIC, but have not updated their ARM cores to
>> + * reflect the correct address in CBAR/PERIPHBASE.
>> + */
>> +#if defined(CONFIG_SUN50I_GEN_H6) || defined(CONFIG_SUNXI_GEN_NCAT2)
>> +#define CFG_ARM_GIC_BASE_ADDRESS	0x03020000
>> +#endif
> 
> I feel this should go into Kconfig. I can make a patch, unless you want to
> beat me to it.

Note that you had previously [1] suggested placing this here, though 
even then speculated that it belonged in Kconfig. I'm probably holding 
off on sending a PSCI v3 until you send your R528 v2, so that might be a 
good place to patch it. I'll remove this hunk if it's unnecessary by then.

[1]: 
https://lore.kernel.org/u-boot/20230531161937.20d37f54@donnerap.cambridge.arm.com/

> Cheers,
> Andre

Likewise,
Sam

On Wed, 27 Sep 2023 18:01:40 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

> On 9/27/23 10:31, Andre Przywara wrote:
> > On Wed, 16 Aug 2023 10:34:19 -0700
> > Sam Edwards <cfsworks@gmail.com> wrote:
> > 
> > Hi Sam,  
> 
> Hi Andre,
> 
> >> @@ -103,10 +116,13 @@ static void __secure clamp_set(u32 *clamp)
> >>   
> >>   static void __secure sunxi_cpu_set_entry(int __always_unused cpu, void *entry)
> >>   {
> >> -	/* secondary core entry address is programmed differently on R40 */
> >> +	/* secondary core entry address is programmed differently on R40/528 */  
> > 
> > I think that's somewhat obvious now from the code, so you can remove this
> > comment.  
> 
> Done, change will be included in v3.

Thanks!
 
> >>   	if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
> >>   		writel((u32)entry,
> >>   		       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
> >> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> >> +		writel((u32)entry,
> >> +		       SUNXI_R_CPUCFG_BASE + SUN8I_R528_SOFT_ENTRY);
> >>   	} else {
> >>   		writel((u32)entry, SUNXI_CPUCFG_BASE + SUNXI_PRIV0);
> >>   	}
> >> @@ -124,6 +140,9 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
> >>   	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
> >>   		clamp = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWR_CLAMP(cpu);
> >>   		pwroff = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWROFF;
> >> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> >> +		/* R528 leaves both cores powered up, manages them via reset */
> >> +		return;
> >>   	} else {
> >>   		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
> >>   		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))
> >> @@ -151,11 +170,27 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
> >>   
> >>   static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
> >>   {
> >> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> >> +		if (reset) {  
> > 
> > I think you can lose the brackets here, since it's a single statement
> > branch, even if it spans multiple lines. The indentation should make this
> > clear.  
> 
> FWIW a lot of reviewers insist on braces surrounding *any* multiline 
> blocks, even if said block is only a single statement. This is to 
> prevent mishaps where another developer comes along later to add another 
> statement to the same block (at the same indentation level), but doesn't 
> think to look for missing brackets because the block is already bigger 
> than one line.
> 
> I could go either way on it, but would like to be sure that your 
> feedback stands in light of that counterpoint.

Yeah, I hear you, but my reflex is to look for that other statement if
I see curly braces. Seeing something without braces matches a pattern
of "just a single statement being different" for me.

And modern compilers actually warn about those indentation issues in
connection with if-statements or for-loops without braces.

But I leave this up to you, checkpatch doesn't seem to care here, so I
am fine either way.

> 
> >> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> >> index 0a3454a51a..d46fd8c0bc 100644
> >> --- a/arch/arm/mach-sunxi/Kconfig
> >> +++ b/arch/arm/mach-sunxi/Kconfig
> >> @@ -355,6 +355,8 @@ config MACH_SUN8I_R40
> >>   config MACH_SUN8I_R528
> >>   	bool "sun8i (Allwinner R528)"
> >>   	select CPU_V7A
> >> +	select CPU_V7_HAS_NONSEC
> >> +	select ARCH_SUPPORT_PSCI  
> > 
> > Please add
> > 	select CPU_V7_HAS_VIRT
> > here, as the cores are perfectly capable of virtualisation. Granted,
> > support for KVM is long gone from Linux, but at least Xen still supports it.  
> 
> Good catch; will be done in v3.
> 
> > And I believe you also need:
> > 	select SPL_ARMV7_SET_CORTEX_SMPEN
> > At least this is what the other cores do. The PSCI code sets this bit for
> > the secondaries, but for the primary core we need to set it as early as
> > possible. Probably not a biggie on an A7, in reality, but good to have,
> > and be it for correctness and consistency's sake.  
> 
> That's already enabled down below:
> # The sun8i SoCs share a lot, this helps to avoid a lot of "if A23 || A33"
> config MACH_SUN8I
>          bool
>          select SPL_ARMV7_SET_CORTEX_SMPEN if !ARM64

Ah, that's the big confusion about that Allwinner naming change:
https://linux-sunxi.org/Allwinner_SoC_Family#2013_naming_scheme_change

So if you look closely, this MACH_SUN8I is more related to that old SoC
generation, not to "anything with an Cortex-A7 in it". And consequently
the R528 support series does NOT enable this symbol, but uses the new
NCAT2 family symbol.
I was checking the generated .config, and didn't find it in there,
hence it needs to be set separately.

> >> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> >> index b8ca77d031..67eb0d25db 100644
> >> --- a/include/configs/sunxi-common.h
> >> +++ b/include/configs/sunxi-common.h
> >> @@ -33,6 +33,14 @@
> >>   
> >>   /* CPU */
> >>   
> >> +/*
> >> + * Newer ARM SoCs have moved the GIC, but have not updated their ARM cores to
> >> + * reflect the correct address in CBAR/PERIPHBASE.
> >> + */
> >> +#if defined(CONFIG_SUN50I_GEN_H6) || defined(CONFIG_SUNXI_GEN_NCAT2)
> >> +#define CFG_ARM_GIC_BASE_ADDRESS	0x03020000
> >> +#endif  
> > 
> > I feel this should go into Kconfig. I can make a patch, unless you want to
> > beat me to it.  
> 
> Note that you had previously [1] suggested placing this here, though 
> even then speculated that it belonged in Kconfig. I'm probably holding 
> off on sending a PSCI v3 until you send your R528 v2, so that might be a 
> good place to patch it. I'll remove this hunk if it's unnecessary by then.

Yeah, I remember saying that, just wanted to reiterate that because it
still is (bad!) "old school" U-Boot style, and we shouldn't add to the
mess.

I am doing the final checks on v2 tomorrow, if nothing pops up, that
should go out then. Just as a heads up ...

Cheers,
Andre

> [1]: 
> https://lore.kernel.org/u-boot/20230531161937.20d37f54@donnerap.cambridge.arm.com/
> 
> > Cheers,
> > Andre  
> 
> Likewise,
> Sam