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

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

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