[PATCH v2 0/5] Allwinner R528/T113s PSCI

[PATCH v2 0/5] Allwinner R528/T113s PSCI

[Sam Edwards]

Hi list,

This is the second version of my patchset for supporting PSCI (i.e. second core
bringup) on R528/T113-s#, incorporating most of the feedback from Andre.

I do not expect the final two patches in this series to be winners yet. There's
still some back-and-forth going on to sync them up with an anticipated v2 of
Andre's patchset for R528 support (on which this work depends). My v2 is being
sent now mostly just to smoothen that.

However, the first 3 patches in this series ARE meant to be reviewed seriously:
the refactoring is totally independent of the R528 effort, and I see no reason
to wait for the R528 stuff to be nailed down to get started reviewing them.

Going forward, any more changes will most likely be form over function, so
aggressive testing of the whole 5-patch series -- on sunxis old and new -- is
nonetheless very valuable at this point. :)

Changes v1->v2:
- Power clamp is now adjusted ONLY on sun{6,7}i, H3, R40. The previous version
  was mistakenly doing this EXCEPT on those machines.
- Flattened sunxi_power_switch() into sunxi_cpu_set_power() for simplicity's
  sake.
- Moved the "power clamp is not NULL" conditional into sunxi_cpu_set_power().
- Removed unnecessary H6 special-case, since H6 is actually ARM64.
- Renamed SUNXI_CPUX_BASE to SUNXI_CPUCFG_BASE, to mirror expected changes in
  Andre's v2 of the R528 series (we decided against using a new name for this
  block).
- Removed sunxi_cpucfg_reg struct, and stopped using the PRCM struct in psci.c.

Cheers,
Sam

Sam Edwards (5):
  sunxi: psci: clean away preprocessor macros
  sunxi: psci: refactor register access to separate functions
  sunxi: psci: stop modeling register layout with C structs
  sunxi: psci: implement PSCI on R528
  HACK: sunxi: psci: be compatible with v1 of R528 patchset

 arch/arm/cpu/armv7/sunxi/psci.c          | 195 +++++++++++++++--------
 arch/arm/include/asm/arch-sunxi/cpucfg.h |  67 --------
 arch/arm/mach-sunxi/Kconfig              |   2 +
 include/configs/sunxi-common.h           |   8 +
 4 files changed, 136 insertions(+), 136 deletions(-)
 delete mode 100644 arch/arm/include/asm/arch-sunxi/cpucfg.h

-- 
2.41.0

[PATCH v2 1/5] sunxi: psci: clean away preprocessor macros

[Sam Edwards]

This patch restructures psci.c to get away from the "many different
function definitions switched by #ifdef" paradigm to the preferred style
of having a single function definition with `if (IS_ENABLED(...))` to
make the optimizer include only the appropriate function bodies instead.

There are no functional changes here.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/cpu/armv7/sunxi/psci.c | 102 +++++++++++++-------------------
 1 file changed, 42 insertions(+), 60 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
index e1d3638b5c..7804e0933b 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -76,11 +76,8 @@ static void __secure __mdelay(u32 ms)
 	isb();
 }
 
-static void __secure clamp_release(u32 __maybe_unused *clamp)
+static void __secure clamp_release(u32 *clamp)
 {
-#if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN7I) || \
-	defined(CONFIG_MACH_SUN8I_H3) || \
-	defined(CONFIG_MACH_SUN8I_R40)
 	u32 tmp = 0x1ff;
 	do {
 		tmp >>= 1;
@@ -88,83 +85,68 @@ static void __secure clamp_release(u32 __maybe_unused *clamp)
 	} while (tmp);
 
 	__mdelay(10);
-#endif
 }
 
-static void __secure clamp_set(u32 __maybe_unused *clamp)
+static void __secure clamp_set(u32 *clamp)
 {
-#if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN7I) || \
-	defined(CONFIG_MACH_SUN8I_H3) || \
-	defined(CONFIG_MACH_SUN8I_R40)
 	writel(0xff, clamp);
-#endif
 }
 
-static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
-					int cpu)
+static void __secure sunxi_set_entry_address(void *entry)
 {
-	if (on) {
-		/* Release power clamp */
-		clamp_release(clamp);
-
-		/* Clear power gating */
-		clrbits_le32(pwroff, BIT(cpu));
+	/* secondary core entry address is programmed differently on R40 */
+	if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
+		writel((u32)entry,
+		       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
 	} else {
-		/* Set power gating */
-		setbits_le32(pwroff, BIT(cpu));
+		struct sunxi_cpucfg_reg *cpucfg =
+			(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
-		/* Activate power clamp */
-		clamp_set(clamp);
+		writel((u32)entry, &cpucfg->priv0);
 	}
 }
 
-#ifdef CONFIG_MACH_SUN8I_R40
-/* secondary core entry address is programmed differently on R40 */
-static void __secure sunxi_set_entry_address(void *entry)
-{
-	writel((u32)entry,
-	       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
-}
-#else
-static void __secure sunxi_set_entry_address(void *entry)
+static void __secure sunxi_cpu_set_power(int cpu, bool on)
 {
+	u32 *clamp = NULL;
+	u32 *pwroff;
 	struct sunxi_cpucfg_reg *cpucfg =
 		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
-	writel((u32)entry, &cpucfg->priv0);
-}
-#endif
+	/* sun7i (A20) is different from other single cluster SoCs */
+	if (IS_ENABLED(CONFIG_MACH_SUN7I)) {
+		clamp = &cpucfg->cpu1_pwr_clamp;
+		pwroff = &cpucfg->cpu1_pwroff;
+	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
+		clamp = (void *)cpucfg + SUN8I_R40_PWR_CLAMP(cpu);
+		pwroff = (void *)cpucfg + SUN8I_R40_PWROFF;
+	} else {
+		struct sunxi_prcm_reg *prcm =
+			(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
 
-#ifdef CONFIG_MACH_SUN7I
-/* sun7i (A20) is different from other single cluster SoCs */
-static void __secure sunxi_cpu_set_power(int __always_unused cpu, bool on)
-{
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
+		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
+		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))
+			clamp = &prcm->cpu_pwr_clamp[cpu];
 
-	sunxi_power_switch(&cpucfg->cpu1_pwr_clamp, &cpucfg->cpu1_pwroff,
-			   on, 0);
-}
-#elif defined CONFIG_MACH_SUN8I_R40
-static void __secure sunxi_cpu_set_power(int cpu, bool on)
-{
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
+		pwroff = &prcm->cpu_pwroff;
+	}
 
-	sunxi_power_switch((void *)cpucfg + SUN8I_R40_PWR_CLAMP(cpu),
-			   (void *)cpucfg + SUN8I_R40_PWROFF,
-			   on, cpu);
-}
-#else /* ! CONFIG_MACH_SUN7I && ! CONFIG_MACH_SUN8I_R40 */
-static void __secure sunxi_cpu_set_power(int cpu, bool on)
-{
-	struct sunxi_prcm_reg *prcm =
-		(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
+	if (on) {
+		/* Release power clamp */
+		if (clamp)
+			clamp_release(clamp);
 
-	sunxi_power_switch(&prcm->cpu_pwr_clamp[cpu], &prcm->cpu_pwroff,
-			   on, cpu);
+		/* Clear power gating */
+		clrbits_le32(pwroff, BIT(cpu));
+	} else {
+		/* Set power gating */
+		setbits_le32(pwroff, BIT(cpu));
+
+		/* Activate power clamp */
+		if (clamp)
+			clamp_set(clamp);
+	}
 }
-#endif /* CONFIG_MACH_SUN7I */
 
 void __secure sunxi_cpu_power_off(u32 cpuid)
 {
-- 
2.41.0

Re: [PATCH v2 1/5] sunxi: psci: clean away preprocessor macros

[Andre Przywara]

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

Hi Sam,

thanks for the update.

> This patch restructures psci.c to get away from the "many different
> function definitions switched by #ifdef" paradigm to the preferred style
> of having a single function definition with `if (IS_ENABLED(...))` to
> make the optimizer include only the appropriate function bodies instead.
> 
> There are no functional changes here.

So this diff is hard to read - not your fault, this seems to be common for
those kind of refactorings - but I convinced myself by comparing old and
new side-by-side and test-compiling that this doesn't introduce any
functional change. The resulting object file is different (8 byte larger,
even), so it's hard to prove, and I still have to actually test it on some
boards, but the idea is a good one and it's the right direction, so to
move things forward:

> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arch/arm/cpu/armv7/sunxi/psci.c | 102 +++++++++++++-------------------
>  1 file changed, 42 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index e1d3638b5c..7804e0933b 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -76,11 +76,8 @@ static void __secure __mdelay(u32 ms)
>  	isb();
>  }
>  
> -static void __secure clamp_release(u32 __maybe_unused *clamp)
> +static void __secure clamp_release(u32 *clamp)
>  {
> -#if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN7I) || \
> -	defined(CONFIG_MACH_SUN8I_H3) || \
> -	defined(CONFIG_MACH_SUN8I_R40)
>  	u32 tmp = 0x1ff;
>  	do {
>  		tmp >>= 1;
> @@ -88,83 +85,68 @@ static void __secure clamp_release(u32 __maybe_unused *clamp)
>  	} while (tmp);
>  
>  	__mdelay(10);
> -#endif
>  }
>  
> -static void __secure clamp_set(u32 __maybe_unused *clamp)
> +static void __secure clamp_set(u32 *clamp)
>  {
> -#if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN7I) || \
> -	defined(CONFIG_MACH_SUN8I_H3) || \
> -	defined(CONFIG_MACH_SUN8I_R40)
>  	writel(0xff, clamp);
> -#endif
>  }
>  
> -static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
> -					int cpu)
> +static void __secure sunxi_set_entry_address(void *entry)
>  {
> -	if (on) {
> -		/* Release power clamp */
> -		clamp_release(clamp);
> -
> -		/* Clear power gating */
> -		clrbits_le32(pwroff, BIT(cpu));
> +	/* secondary core entry address is programmed differently on R40 */
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
> +		writel((u32)entry,
> +		       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
>  	} else {
> -		/* Set power gating */
> -		setbits_le32(pwroff, BIT(cpu));
> +		struct sunxi_cpucfg_reg *cpucfg =
> +			(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
>  
> -		/* Activate power clamp */
> -		clamp_set(clamp);
> +		writel((u32)entry, &cpucfg->priv0);
>  	}
>  }
>  
> -#ifdef CONFIG_MACH_SUN8I_R40
> -/* secondary core entry address is programmed differently on R40 */
> -static void __secure sunxi_set_entry_address(void *entry)
> -{
> -	writel((u32)entry,
> -	       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
> -}
> -#else
> -static void __secure sunxi_set_entry_address(void *entry)
> +static void __secure sunxi_cpu_set_power(int cpu, bool on)
>  {
> +	u32 *clamp = NULL;
> +	u32 *pwroff;
>  	struct sunxi_cpucfg_reg *cpucfg =
>  		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
>  
> -	writel((u32)entry, &cpucfg->priv0);
> -}
> -#endif
> +	/* sun7i (A20) is different from other single cluster SoCs */
> +	if (IS_ENABLED(CONFIG_MACH_SUN7I)) {
> +		clamp = &cpucfg->cpu1_pwr_clamp;
> +		pwroff = &cpucfg->cpu1_pwroff;
> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
> +		clamp = (void *)cpucfg + SUN8I_R40_PWR_CLAMP(cpu);
> +		pwroff = (void *)cpucfg + SUN8I_R40_PWROFF;
> +	} else {
> +		struct sunxi_prcm_reg *prcm =
> +			(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
>  
> -#ifdef CONFIG_MACH_SUN7I
> -/* sun7i (A20) is different from other single cluster SoCs */
> -static void __secure sunxi_cpu_set_power(int __always_unused cpu, bool on)
> -{
> -	struct sunxi_cpucfg_reg *cpucfg =
> -		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> +		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
> +		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))
> +			clamp = &prcm->cpu_pwr_clamp[cpu];
>  
> -	sunxi_power_switch(&cpucfg->cpu1_pwr_clamp, &cpucfg->cpu1_pwroff,
> -			   on, 0);
> -}
> -#elif defined CONFIG_MACH_SUN8I_R40
> -static void __secure sunxi_cpu_set_power(int cpu, bool on)
> -{
> -	struct sunxi_cpucfg_reg *cpucfg =
> -		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> +		pwroff = &prcm->cpu_pwroff;
> +	}
>  
> -	sunxi_power_switch((void *)cpucfg + SUN8I_R40_PWR_CLAMP(cpu),
> -			   (void *)cpucfg + SUN8I_R40_PWROFF,
> -			   on, cpu);
> -}
> -#else /* ! CONFIG_MACH_SUN7I && ! CONFIG_MACH_SUN8I_R40 */
> -static void __secure sunxi_cpu_set_power(int cpu, bool on)
> -{
> -	struct sunxi_prcm_reg *prcm =
> -		(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
> +	if (on) {
> +		/* Release power clamp */
> +		if (clamp)
> +			clamp_release(clamp);
>  
> -	sunxi_power_switch(&prcm->cpu_pwr_clamp[cpu], &prcm->cpu_pwroff,
> -			   on, cpu);
> +		/* Clear power gating */
> +		clrbits_le32(pwroff, BIT(cpu));
> +	} else {
> +		/* Set power gating */
> +		setbits_le32(pwroff, BIT(cpu));
> +
> +		/* Activate power clamp */
> +		if (clamp)
> +			clamp_set(clamp);
> +	}
>  }
> -#endif /* CONFIG_MACH_SUN7I */
>  
>  void __secure sunxi_cpu_power_off(u32 cpuid)
>  {

Re: [PATCH v2 1/5] sunxi: psci: clean away preprocessor macros

[Sam Edwards]

On 8/18/23 07:11, Andre Przywara wrote:

Hi Andre,

> The resulting object file is different (8 byte larger,
> even), so it's hard to prove

I'm no stranger to reading object code. Since the output should be 
identical in principle, I'll spend a little bit of time today seeing if 
I can identify what's changing. If it's easy enough, I'd like to adjust 
my patch so that the optimizer does produce the same output. (Keep in 
mind I'm on Clang, though. If Clang already gives the same output for 
both, I'll just report back to use that when comparing.)

>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thank you!

> 
> Cheers,
> Andre

Likewise,
Sam

Re: [PATCH v2 1/5] sunxi: psci: clean away preprocessor macros

[Sam Edwards]

On 8/18/23 10:40, Sam Edwards wrote:
> On 8/18/23 07:11, Andre Przywara wrote:
> 
> Hi Andre,
> 
>> The resulting object file is different (8 byte larger,
>> even), so it's hard to prove
> 
> I'm no stranger to reading object code. Since the output should be 
> identical in principle, I'll spend a little bit of time today seeing if 
> I can identify what's changing. If it's easy enough, I'd like to adjust 
> my patch so that the optimizer does produce the same output. (Keep in 
> mind I'm on Clang, though. If Clang already gives the same output for 
> both, I'll just report back to use that when comparing.)

I built only psci.o from every ARM32 sunxi for which we have a defconfig 
(and for which PSCI is supported), for 81 targets total (though there 
are only 4 variations: R40, sun7i, H3/sun6i, and "everything else"). I 
am working with Clang version 16.0.6.

I compared only the secure text section. The command to extract this 
looks like:
llvm-objcopy -O binary --only-section=._secure.text psci.o text.bin
This is important because there are debug sections that will change when 
the source file line numbers change, so we must ignore those when comparing.

In the majority of cases, there are no changes to the text section 
introduced by this patch. In the R40 case, there's a small change where 
the compiler adds a NULL check onto the result of the `(void *)cpucfg + 
SUN8I_R40_PWR_CLAMP(cpu)` computation, which we can ignore as it won't 
affect anything in practice. In the sun7i case, the only changes are 
because I am NOT hardcoding the CPU to 0, which does look like I broke 
it (since that means it will use cpu=1). So I'm going to need to fix 
that in v3.

For good measure, I also applied the same methodology to patch 2 in this 
series, and that introduces no text section changes whatsoever in any of 
the tested cases. So patch 2 (theoretically, anyway) needs no bugfixes 
or hardware testing.

Patch 3 does cause a text section change for all targets. I will have to 
investigate why, in case I messed up any of the offsets when migrating 
off of structs.

Regards,
Sam

Re: [PATCH v2 1/5] sunxi: psci: clean away preprocessor macros

[Andre Przywara]

On Fri, 18 Aug 2023 14:17:07 -0700
Sam Edwards <cfsworks@gmail.com> wrote:

> On 8/18/23 10:40, Sam Edwards wrote:
> > On 8/18/23 07:11, Andre Przywara wrote:
> > 
> > Hi Andre,
> >   
> >> The resulting object file is different (8 byte larger,
> >> even), so it's hard to prove  
> > 
> > I'm no stranger to reading object code. Since the output should be 
> > identical in principle, I'll spend a little bit of time today seeing if 
> > I can identify what's changing. If it's easy enough, I'd like to adjust 
> > my patch so that the optimizer does produce the same output. (Keep in 
> > mind I'm on Clang, though. If Clang already gives the same output for 
> > both, I'll just report back to use that when comparing.)  
> 
> I built only psci.o from every ARM32 sunxi for which we have a defconfig 
> (and for which PSCI is supported), for 81 targets total (though there 
> are only 4 variations: R40, sun7i, H3/sun6i, and "everything else"). I 
> am working with Clang version 16.0.6.
> 
> I compared only the secure text section. The command to extract this 
> looks like:
> llvm-objcopy -O binary --only-section=._secure.text psci.o text.bin
> This is important because there are debug sections that will change when 
> the source file line numbers change, so we must ignore those when comparing.
> 
> In the majority of cases, there are no changes to the text section 
> introduced by this patch. In the R40 case, there's a small change where 
> the compiler adds a NULL check onto the result of the `(void *)cpucfg + 
> SUN8I_R40_PWR_CLAMP(cpu)` computation, which we can ignore as it won't 
> affect anything in practice. In the sun7i case, the only changes are 
> because I am NOT hardcoding the CPU to 0, which does look like I broke 
> it (since that means it will use cpu=1). So I'm going to need to fix 
> that in v3.

      ^^^^^^^
Do you have an update on this? I will try to test it on an R40 ASAP.

Cheers,
Andre

> For good measure, I also applied the same methodology to patch 2 in this 
> series, and that introduces no text section changes whatsoever in any of 
> the tested cases. So patch 2 (theoretically, anyway) needs no bugfixes 
> or hardware testing.
> 
> Patch 3 does cause a text section change for all targets. I will have to 
> investigate why, in case I messed up any of the offsets when migrating 
> off of structs.
> 
> Regards,
> Sam

Re: [PATCH v2 1/5] sunxi: psci: clean away preprocessor macros

[Sam Edwards]

On 9/27/23 10:34, Andre Przywara wrote:
>> In the majority of cases, there are no changes to the text section
>> introduced by this patch. In the R40 case, there's a small change where
>> the compiler adds a NULL check onto the result of the `(void *)cpucfg +
>> SUN8I_R40_PWR_CLAMP(cpu)` computation, which we can ignore as it won't
>> affect anything in practice. In the sun7i case, the only changes are
>> because I am NOT hardcoding the CPU to 0, which does look like I broke
>> it (since that means it will use cpu=1). So I'm going to need to fix
>> that in v3.
> 
>        ^^^^^^^
> Do you have an update on this? I will try to test it on an R40 ASAP.

In my (not yet submitted) v3, I have the following change done:

  	if (IS_ENABLED(CONFIG_MACH_SUN7I)) {
  		clamp = &cpucfg->cpu1_pwr_clamp;
  		pwroff = &cpucfg->cpu1_pwroff;
+		cpu = 0;
  	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {

That does negate any binary change in the SUN7I case. R40 is the only 
one that needs testing still. If you feel like testing on any SUN7Is 
just for good measure, you'll want to add in that missing line.

> Cheers,
> Andre

Likewise,
Sam

[PATCH v2 2/5] sunxi: psci: refactor register access to separate functions

[Sam Edwards]

This is to prepare for R528, which does not have the typical
"CPUCFG" block; it has a "CPUX" block which provides these
same functions but is organized differently.

Moving the hardware-access bits to their own functions separates the
logic from the hardware so we can reuse the same logic.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/cpu/armv7/sunxi/psci.c | 66 +++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 19 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
index 7804e0933b..e2845f21ab 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -92,7 +92,7 @@ static void __secure clamp_set(u32 *clamp)
 	writel(0xff, clamp);
 }
 
-static void __secure sunxi_set_entry_address(void *entry)
+static void __secure sunxi_cpu_set_entry(int __always_unused cpu, void *entry)
 {
 	/* secondary core entry address is programmed differently on R40 */
 	if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
@@ -148,30 +148,60 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
 	}
 }
 
-void __secure sunxi_cpu_power_off(u32 cpuid)
+static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
+{
+	struct sunxi_cpucfg_reg *cpucfg =
+		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
+
+	writel(reset ? 0b00 : 0b11, &cpucfg->cpu[cpu].rst);
+}
+
+static void __secure sunxi_cpu_set_locking(int cpu, bool lock)
 {
 	struct sunxi_cpucfg_reg *cpucfg =
 		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
+
+	if (lock)
+		clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+	else
+		setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+}
+
+static bool __secure sunxi_cpu_poll_wfi(int cpu)
+{
+	struct sunxi_cpucfg_reg *cpucfg =
+		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
+
+	return !!(readl(&cpucfg->cpu[cpu].status) & BIT(2));
+}
+
+static void __secure sunxi_cpu_invalidate_cache(int cpu)
+{
+	struct sunxi_cpucfg_reg *cpucfg =
+		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
+
+	clrbits_le32(&cpucfg->gen_ctrl, BIT(cpu));
+}
+
+void __secure sunxi_cpu_power_off(u32 cpuid)
+{
 	u32 cpu = cpuid & 0x3;
 
 	/* Wait for the core to enter WFI */
-	while (1) {
-		if (readl(&cpucfg->cpu[cpu].status) & BIT(2))
-			break;
+	while (!sunxi_cpu_poll_wfi(cpu))
 		__mdelay(1);
-	}
 
 	/* Assert reset on target CPU */
-	writel(0, &cpucfg->cpu[cpu].rst);
+	sunxi_cpu_set_reset(cpu, true);
 
 	/* Lock CPU (Disable external debug access) */
-	clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+	sunxi_cpu_set_locking(cpu, true);
 
 	/* Power down CPU */
 	sunxi_cpu_set_power(cpuid, false);
 
-	/* Unlock CPU (Disable external debug access) */
-	setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+	/* Unlock CPU (Reenable external debug access) */
+	sunxi_cpu_set_locking(cpu, false);
 }
 
 static u32 __secure cp15_read_scr(void)
@@ -228,33 +258,31 @@ out:
 int __secure psci_cpu_on(u32 __always_unused unused, u32 mpidr, u32 pc,
 			 u32 context_id)
 {
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 	u32 cpu = (mpidr & 0x3);
 
 	/* store target PC and context id */
 	psci_save(cpu, pc, context_id);
 
 	/* Set secondary core power on PC */
-	sunxi_set_entry_address(&psci_cpu_entry);
+	sunxi_cpu_set_entry(cpu, &psci_cpu_entry);
 
 	/* Assert reset on target CPU */
-	writel(0, &cpucfg->cpu[cpu].rst);
+	sunxi_cpu_set_reset(cpu, true);
 
 	/* Invalidate L1 cache */
-	clrbits_le32(&cpucfg->gen_ctrl, BIT(cpu));
+	sunxi_cpu_invalidate_cache(cpu);
 
 	/* Lock CPU (Disable external debug access) */
-	clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+	sunxi_cpu_set_locking(cpu, true);
 
 	/* Power up target CPU */
 	sunxi_cpu_set_power(cpu, true);
 
 	/* De-assert reset on target CPU */
-	writel(BIT(1) | BIT(0), &cpucfg->cpu[cpu].rst);
+	sunxi_cpu_set_reset(cpu, false);
 
-	/* Unlock CPU (Disable external debug access) */
-	setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+	/* Unlock CPU (Reenable external debug access) */
+	sunxi_cpu_set_locking(cpu, false);
 
 	return ARM_PSCI_RET_SUCCESS;
 }
-- 
2.41.0

Re: [PATCH v2 2/5] sunxi: psci: refactor register access to separate functions

[Andre Przywara]

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

Hi Sam,

> This is to prepare for R528, which does not have the typical
> "CPUCFG" block; it has a "CPUX" block which provides these
> same functions but is organized differently.
> 
> Moving the hardware-access bits to their own functions separates the
> logic from the hardware so we can reuse the same logic.

That's a very nice cleanup, thanks for doing that!

Another one of those hard-to-reason-about diffs, but I placed the register
access back into the place of the new function call, somewhat playing your
patch backwards, manually, and that ended up at a very similar code to
before the patch, so I think it's legit.
Again, needs some testing, but looks good from a diff point of view.

Two smaller things, with them fixed:

> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arch/arm/cpu/armv7/sunxi/psci.c | 66 +++++++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index 7804e0933b..e2845f21ab 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -92,7 +92,7 @@ static void __secure clamp_set(u32 *clamp)
>  	writel(0xff, clamp);
>  }
>  
> -static void __secure sunxi_set_entry_address(void *entry)
> +static void __secure sunxi_cpu_set_entry(int __always_unused cpu, void *entry)

So what is the reasoning behind this change?
If *none* of the Allwinner SoCs have independent secondary entry point
registers, we should not give the impression some do in the prototype.
Should later a SoC emerge that changes this, adjusting this one is the
least of our problems then.

>  {
>  	/* secondary core entry address is programmed differently on R40 */
>  	if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
> @@ -148,30 +148,60 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
>  	}
>  }
>  
> -void __secure sunxi_cpu_power_off(u32 cpuid)
> +static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
> +{
> +	struct sunxi_cpucfg_reg *cpucfg =
> +		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> +
> +	writel(reset ? 0b00 : 0b11, &cpucfg->cpu[cpu].rst);
> +}
> +
> +static void __secure sunxi_cpu_set_locking(int cpu, bool lock)
>  {
>  	struct sunxi_cpucfg_reg *cpucfg =
>  		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> +
> +	if (lock)
> +		clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
> +	else
> +		setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
> +}
> +
> +static bool __secure sunxi_cpu_poll_wfi(int cpu)
> +{
> +	struct sunxi_cpucfg_reg *cpucfg =
> +		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> +
> +	return !!(readl(&cpucfg->cpu[cpu].status) & BIT(2));
> +}
> +
> +static void __secure sunxi_cpu_invalidate_cache(int cpu)
> +{
> +	struct sunxi_cpucfg_reg *cpucfg =
> +		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> +
> +	clrbits_le32(&cpucfg->gen_ctrl, BIT(cpu));
> +}
> +
> +void __secure sunxi_cpu_power_off(u32 cpuid)

Can you please mark this as static on the way? That does not seem to be
used anywhere, and even the name suggests it's local.
Saves 8 bytes of .text ;-)

> +{
>  	u32 cpu = cpuid & 0x3;
>  
>  	/* Wait for the core to enter WFI */
> -	while (1) {
> -		if (readl(&cpucfg->cpu[cpu].status) & BIT(2))
> -			break;
> +	while (!sunxi_cpu_poll_wfi(cpu))
>  		__mdelay(1);
> -	}
>  
>  	/* Assert reset on target CPU */
> -	writel(0, &cpucfg->cpu[cpu].rst);
> +	sunxi_cpu_set_reset(cpu, true);
>  
>  	/* Lock CPU (Disable external debug access) */
> -	clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
> +	sunxi_cpu_set_locking(cpu, true);
>  
>  	/* Power down CPU */
>  	sunxi_cpu_set_power(cpuid, false);
>  
> -	/* Unlock CPU (Disable external debug access) */
> -	setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
> +	/* Unlock CPU (Reenable external debug access) */
> +	sunxi_cpu_set_locking(cpu, false);
>  }
>  
>  static u32 __secure cp15_read_scr(void)
> @@ -228,33 +258,31 @@ out:
>  int __secure psci_cpu_on(u32 __always_unused unused, u32 mpidr, u32 pc,
>  			 u32 context_id)
>  {
> -	struct sunxi_cpucfg_reg *cpucfg =
> -		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
>  	u32 cpu = (mpidr & 0x3);
>  
>  	/* store target PC and context id */
>  	psci_save(cpu, pc, context_id);
>  
>  	/* Set secondary core power on PC */
> -	sunxi_set_entry_address(&psci_cpu_entry);
> +	sunxi_cpu_set_entry(cpu, &psci_cpu_entry);
>  
>  	/* Assert reset on target CPU */
> -	writel(0, &cpucfg->cpu[cpu].rst);
> +	sunxi_cpu_set_reset(cpu, true);
>  
>  	/* Invalidate L1 cache */
> -	clrbits_le32(&cpucfg->gen_ctrl, BIT(cpu));
> +	sunxi_cpu_invalidate_cache(cpu);
>  
>  	/* Lock CPU (Disable external debug access) */
> -	clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
> +	sunxi_cpu_set_locking(cpu, true);
>  
>  	/* Power up target CPU */
>  	sunxi_cpu_set_power(cpu, true);
>  
>  	/* De-assert reset on target CPU */
> -	writel(BIT(1) | BIT(0), &cpucfg->cpu[cpu].rst);
> +	sunxi_cpu_set_reset(cpu, false);
>  
> -	/* Unlock CPU (Disable external debug access) */
> -	setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
> +	/* Unlock CPU (Reenable external debug access) */
> +	sunxi_cpu_set_locking(cpu, false);
>  
>  	return ARM_PSCI_RET_SUCCESS;
>  }

Re: [PATCH v2 2/5] sunxi: psci: refactor register access to separate functions

[Sam Edwards]

On 8/18/23 07:57, Andre Przywara wrote:
> On Wed, 16 Aug 2023 10:34:17 -0700
> Sam Edwards <cfsworks@gmail.com> wrote:
> 
> Hi Sam,

Likewise Andre,

>> -static void __secure sunxi_set_entry_address(void *entry)
>> +static void __secure sunxi_cpu_set_entry(int __always_unused cpu, void *entry)
> 
> So what is the reasoning behind this change?

The primary reason is to be consistent with every other sunxi_cpu_* 
function, while the *tertiary* reason is that it was useful to have that 
argument preserved in a debug build, so that when I was tracing 
execution I could be *sure* that the correct CPU was being chosen (this 
was before I found out that the GIC400 base was being determined 
incorrectly). As for the secondary reason...

> If *none* of the Allwinner SoCs have independent secondary entry point
> registers, we should not give the impression some do in the prototype.
> Should later a SoC emerge that changes this, adjusting this one is the
> least of our problems then.

Ah, but the T113 is already such an SoC. From the user manual:
- The Soft Entry Address Register of CPU0 is 0x070005C4.
- The Soft Entry Address Register of CPU1 is 0x070005C8.

...so my second reason for the function prototype being the way it is 
(and perhaps something which I should be driving home in patch 4/5) is 
indeed to give that impression: that it is *not* the case that none of 
the sunxis have independent secondary entry point registers.

>> +void __secure sunxi_cpu_power_off(u32 cpuid)
> 
> Can you please mark this as static on the way? That does not seem to be
> used anywhere, and even the name suggests it's local.
> Saves 8 bytes of .text ;-)

Easy enough, it shall be done!

Thanks,
Sam

[PATCH v2 3/5] sunxi: psci: stop modeling register layout with C structs

[Sam Edwards]

Since the sunxi support nowadays generally prefers #defined register
offsets instead of modeling register layouts using C structs, now is a
good time to do this for PSCI as well. This patch moves away from using
the structs `sunxi_cpucfg_reg` and `sunxi_prcm_reg` in psci.c.

The former struct and its associated header file existed only to support
PSCI code, so also delete them altogether.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 arch/arm/cpu/armv7/sunxi/psci.c          | 57 ++++++++------------
 arch/arm/include/asm/arch-sunxi/cpucfg.h | 67 ------------------------
 2 files changed, 23 insertions(+), 101 deletions(-)
 delete mode 100644 arch/arm/include/asm/arch-sunxi/cpucfg.h

diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
index e2845f21ab..6ecdd05250 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -11,8 +11,6 @@
 #include <asm/cache.h>
 
 #include <asm/arch/cpu.h>
-#include <asm/arch/cpucfg.h>
-#include <asm/arch/prcm.h>
 #include <asm/armv7.h>
 #include <asm/gic.h>
 #include <asm/io.h>
@@ -27,6 +25,17 @@
 #define	GICD_BASE	(SUNXI_GIC400_BASE + GIC_DIST_OFFSET)
 #define	GICC_BASE	(SUNXI_GIC400_BASE + GIC_CPU_OFFSET_A15)
 
+/*
+ * Offsets into the CPUCFG block applicable to most SUNXIs.
+ */
+#define SUNXI_CPU_RST(cpu)			(0x40 + (cpu) * 0x40 + 0x0)
+#define SUNXI_CPU_STATUS(cpu)			(0x40 + (cpu) * 0x40 + 0x8)
+#define SUNXI_GEN_CTRL				(0x184)
+#define SUNXI_PRIV0				(0x1a4)
+#define SUN7I_CPU1_PWR_CLAMP			(0x1b0)
+#define SUN7I_CPU1_PWROFF			(0x1b4)
+#define SUNXI_DBG_CTRL1				(0x1e4)
+
 /*
  * R40 is different from other single cluster SoCs.
  *
@@ -99,10 +108,7 @@ static void __secure sunxi_cpu_set_entry(int __always_unused cpu, void *entry)
 		writel((u32)entry,
 		       SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0);
 	} else {
-		struct sunxi_cpucfg_reg *cpucfg =
-			(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
-
-		writel((u32)entry, &cpucfg->priv0);
+		writel((u32)entry, SUNXI_CPUCFG_BASE + SUNXI_PRIV0);
 	}
 }
 
@@ -110,25 +116,20 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
 {
 	u32 *clamp = NULL;
 	u32 *pwroff;
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
 	/* sun7i (A20) is different from other single cluster SoCs */
 	if (IS_ENABLED(CONFIG_MACH_SUN7I)) {
-		clamp = &cpucfg->cpu1_pwr_clamp;
-		pwroff = &cpucfg->cpu1_pwroff;
+		clamp = (void *)SUNXI_CPUCFG_BASE + SUN7I_CPU1_PWR_CLAMP;
+		pwroff = (void *)SUNXI_CPUCFG_BASE + SUN7I_CPU1_PWROFF;
 	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
-		clamp = (void *)cpucfg + SUN8I_R40_PWR_CLAMP(cpu);
-		pwroff = (void *)cpucfg + SUN8I_R40_PWROFF;
+		clamp = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWR_CLAMP(cpu);
+		pwroff = (void *)SUNXI_CPUCFG_BASE + SUN8I_R40_PWROFF;
 	} else {
-		struct sunxi_prcm_reg *prcm =
-			(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
-
 		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
 		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))
-			clamp = &prcm->cpu_pwr_clamp[cpu];
+			clamp = (void *)SUNXI_PRCM_BASE + 0x140 + cpu * 0x4;
 
-		pwroff = &prcm->cpu_pwroff;
+		pwroff = (void *)SUNXI_PRCM_BASE + 0x100;
 	}
 
 	if (on) {
@@ -150,37 +151,25 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
 
 static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
 {
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
-
-	writel(reset ? 0b00 : 0b11, &cpucfg->cpu[cpu].rst);
+	writel(reset ? 0b00 : 0b11, SUNXI_CPUCFG_BASE + SUNXI_CPU_RST(cpu));
 }
 
 static void __secure sunxi_cpu_set_locking(int cpu, bool lock)
 {
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
-
 	if (lock)
-		clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+		clrbits_le32(SUNXI_CPUCFG_BASE + SUNXI_DBG_CTRL1, BIT(cpu));
 	else
-		setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
+		setbits_le32(SUNXI_CPUCFG_BASE + SUNXI_DBG_CTRL1, BIT(cpu));
 }
 
 static bool __secure sunxi_cpu_poll_wfi(int cpu)
 {
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
-
-	return !!(readl(&cpucfg->cpu[cpu].status) & BIT(2));
+	return !!(readl(SUNXI_CPUCFG_BASE + SUNXI_CPU_STATUS(cpu)) & BIT(2));
 }
 
 static void __secure sunxi_cpu_invalidate_cache(int cpu)
 {
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
-
-	clrbits_le32(&cpucfg->gen_ctrl, BIT(cpu));
+	clrbits_le32(SUNXI_CPUCFG_BASE + SUNXI_GEN_CTRL, BIT(cpu));
 }
 
 void __secure sunxi_cpu_power_off(u32 cpuid)
diff --git a/arch/arm/include/asm/arch-sunxi/cpucfg.h b/arch/arm/include/asm/arch-sunxi/cpucfg.h
deleted file mode 100644
index 4aaebe0a97..0000000000
--- a/arch/arm/include/asm/arch-sunxi/cpucfg.h
+++ /dev/null
@@ -1,67 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
-/*
- * Sunxi A31 CPUCFG register definition.
- *
- * (C) Copyright 2014 Hans de Goede <hdegoede@redhat.com
- */
-
-#ifndef _SUNXI_CPUCFG_H
-#define _SUNXI_CPUCFG_H
-
-#include <linux/compiler.h>
-#include <linux/types.h>
-
-#ifndef __ASSEMBLY__
-
-struct __packed sunxi_cpucfg_cpu {
-	u32 rst;		/* base + 0x0 */
-	u32 ctrl;		/* base + 0x4 */
-	u32 status;		/* base + 0x8 */
-	u8 res[0x34];		/* base + 0xc */
-};
-
-struct __packed sunxi_cpucfg_reg {
-	u8 res0[0x40];		/* 0x000 */
-	struct sunxi_cpucfg_cpu cpu[4];		/* 0x040 */
-	u8 res1[0x44];		/* 0x140 */
-	u32 gen_ctrl;		/* 0x184 */
-	u32 l2_status;		/* 0x188 */
-	u8 res2[0x4];		/* 0x18c */
-	u32 event_in;		/* 0x190 */
-	u8 res3[0xc];		/* 0x194 */
-	u32 super_standy_flag;	/* 0x1a0 */
-	u32 priv0;		/* 0x1a4 */
-	u32 priv1;		/* 0x1a8 */
-	u8 res4[0x4];		/* 0x1ac */
-	u32 cpu1_pwr_clamp;	/* 0x1b0 sun7i only */
-	u32 cpu1_pwroff;	/* 0x1b4 sun7i only */
-	u8 res5[0x2c];		/* 0x1b8 */
-	u32 dbg_ctrl1;		/* 0x1e4 */
-	u8 res6[0x18];		/* 0x1e8 */
-	u32 idle_cnt0_low;	/* 0x200 */
-	u32 idle_cnt0_high;	/* 0x204 */
-	u32 idle_cnt0_ctrl;	/* 0x208 */
-	u8 res8[0x4];		/* 0x20c */
-	u32 idle_cnt1_low;	/* 0x210 */
-	u32 idle_cnt1_high;	/* 0x214 */
-	u32 idle_cnt1_ctrl;	/* 0x218 */
-	u8 res9[0x4];		/* 0x21c */
-	u32 idle_cnt2_low;	/* 0x220 */
-	u32 idle_cnt2_high;	/* 0x224 */
-	u32 idle_cnt2_ctrl;	/* 0x228 */
-	u8 res10[0x4];		/* 0x22c */
-	u32 idle_cnt3_low;	/* 0x230 */
-	u32 idle_cnt3_high;	/* 0x234 */
-	u32 idle_cnt3_ctrl;	/* 0x238 */
-	u8 res11[0x4];		/* 0x23c */
-	u32 idle_cnt4_low;	/* 0x240 */
-	u32 idle_cnt4_high;	/* 0x244 */
-	u32 idle_cnt4_ctrl;	/* 0x248 */
-	u8 res12[0x34];		/* 0x24c */
-	u32 cnt64_ctrl;		/* 0x280 */
-	u32 cnt64_low;		/* 0x284 */
-	u32 cnt64_high;		/* 0x288 */
-};
-
-#endif /* __ASSEMBLY__ */
-#endif /* _SUNXI_CPUCFG_H */
-- 
2.41.0

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

[Sam Edwards]

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 */
 	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) {
+			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
 	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
+
 /*
  * 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
-- 
2.41.0

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

[Andre Przywara]

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

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

[Sam Edwards]

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

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

[Andre Przywara]

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

[PATCH v2 5/5] HACK: sunxi: psci: be compatible with v1 of R528 patchset

[Sam Edwards]

This is a hack for reviewer QoL. It is not being submitted for mainline
inclusion.
---
 arch/arm/cpu/armv7/sunxi/psci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
index b4ce4f6def..27bac291d5 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -60,6 +60,18 @@
 
 #define SUN8I_R528_C0_STATUS_STANDBYWFI		(16)
 
+/* 3 hacks for compatibility across v1/v2 of Andre's R528 support series */
+#ifndef SUNXI_R_CPUCFG_BASE
+#define SUNXI_R_CPUCFG_BASE			0
+#endif
+#ifndef SUNXI_PRCM_BASE
+#define SUNXI_PRCM_BASE				0
+#endif
+#if defined(SUNXI_CPUX_BASE) && defined(SUNXI_CPUCFG_BASE)
+#undef SUNXI_CPUCFG_BASE
+#define SUNXI_CPUCFG_BASE SUNXI_CPUX_BASE
+#endif
+
 static void __secure cp15_write_cntp_tval(u32 tval)
 {
 	asm volatile ("mcr p15, 0, %0, c14, c2, 0" : : "r" (tval));
-- 
2.41.0

Re: [PATCH v2 5/5] HACK: sunxi: psci: be compatible with v1 of R528 patchset

[Andre Przywara]

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

Hi Sam,

> This is a hack for reviewer QoL. It is not being submitted for mainline
> inclusion.
> ---
>  arch/arm/cpu/armv7/sunxi/psci.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index b4ce4f6def..27bac291d5 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -60,6 +60,18 @@
>  
>  #define SUN8I_R528_C0_STATUS_STANDBYWFI		(16)
>  
> +/* 3 hacks for compatibility across v1/v2 of Andre's R528 support series */
> +#ifndef SUNXI_R_CPUCFG_BASE
> +#define SUNXI_R_CPUCFG_BASE			0
> +#endif

Mmh, I didn't find a better solution than keeping this in.

> +#ifndef SUNXI_PRCM_BASE
> +#define SUNXI_PRCM_BASE				0

So this is now handled. As Samuel pointed out, the R329 (another member of
the "NCAT2" family), actually documents a PRCM block, and arguably the
T113s/D1 have that as well, it's just not very useful (at least for
U-Boot), so we didn't need it so far. I just put the address documented in
the R329 manual into cpu_sunxi_ncat2.h, so the symbol expands properly.

> +#endif
> +#if defined(SUNXI_CPUX_BASE) && defined(SUNXI_CPUCFG_BASE)
> +#undef SUNXI_CPUCFG_BASE
> +#define SUNXI_CPUCFG_BASE SUNXI_CPUX_BASE

So what's the story with this? Do we name this differently
(SUNXI_CPUX_BASE) because the IP block is different from the other SoCs?
Or is there another SUNXI_CPUCFG IP block on the R528/T113s SoCs?

If not, I think we should use the SUNXI_CPUCFG_BASE name directly in
cpu_sunxi_ncat2.h, as we never claimed that same names for some MMIO
address blocks means they are compatible.

Please let me know if I miss something.

Cheers,
Andre



> +#endif
> +
>  static void __secure cp15_write_cntp_tval(u32 tval)
>  {
>  	asm volatile ("mcr p15, 0, %0, c14, c2, 0" : : "r" (tval));

Re: [PATCH v2 5/5] HACK: sunxi: psci: be compatible with v1 of R528 patchset

[Sam Edwards]

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

Hi Andre,

> Mmh, I didn't find a better solution than keeping this in.

I'll keep it if your R528 v2 doesn't find some other way to address it.

>> +#endif
>> +#if defined(SUNXI_CPUX_BASE) && defined(SUNXI_CPUCFG_BASE)
>> +#undef SUNXI_CPUCFG_BASE
>> +#define SUNXI_CPUCFG_BASE SUNXI_CPUX_BASE
> 
> So what's the story with this? Do we name this differently
> (SUNXI_CPUX_BASE) because the IP block is different from the other SoCs?
> Or is there another SUNXI_CPUCFG IP block on the R528/T113s SoCs?
> 
> If not, I think we should use the SUNXI_CPUCFG_BASE name directly in
> cpu_sunxi_ncat2.h, as we never claimed that same names for some MMIO
> address blocks means they are compatible.
> 
> Please let me know if I miss something.

That's just for compatibility with R528 series v1. It's expected that 
you'll rename it to SUNXI_CPUCFG_BASE for v2. The preprocessor trickery 
looks for *both* being defined and applies the update. The rest of the 
code proceeds using SUNXI_CPUCFG_BASE. (Keep in mind this is particular 
patch is a hack patch, it's not considered for inclusion.)

Warm regards,
Sam

Re: [PATCH v2 5/5] HACK: sunxi: psci: be compatible with v1 of R528 patchset

[Andre Przywara]

On Wed, 27 Sep 2023 17:28:51 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

> On 9/27/23 10:32, Andre Przywara wrote:
> > On Wed, 16 Aug 2023 10:34:20 -0700
> > Sam Edwards <cfsworks@gmail.com> wrote:
> > 
> > Hi Sam,  
> 
> Hi Andre,
> 
> > Mmh, I didn't find a better solution than keeping this in.  
> 
> I'll keep it if your R528 v2 doesn't find some other way to address it.
> 
> >> +#endif
> >> +#if defined(SUNXI_CPUX_BASE) && defined(SUNXI_CPUCFG_BASE)
> >> +#undef SUNXI_CPUCFG_BASE
> >> +#define SUNXI_CPUCFG_BASE SUNXI_CPUX_BASE  
> > 
> > So what's the story with this? Do we name this differently
> > (SUNXI_CPUX_BASE) because the IP block is different from the other SoCs?
> > Or is there another SUNXI_CPUCFG IP block on the R528/T113s SoCs?
> > 
> > If not, I think we should use the SUNXI_CPUCFG_BASE name directly in
> > cpu_sunxi_ncat2.h, as we never claimed that same names for some MMIO
> > address blocks means they are compatible.
> > 
> > Please let me know if I miss something.  
> 
> That's just for compatibility with R528 series v1. It's expected that 
> you'll rename it to SUNXI_CPUCFG_BASE for v2. The preprocessor trickery 
> looks for *both* being defined and applies the update. The rest of the 
> code proceeds using SUNXI_CPUCFG_BASE. (Keep in mind this is particular 
> patch is a hack patch, it's not considered for inclusion.)

Yes, I got this, but surely the expectation is that those fixes should
not be needed anymore after a v2 of the R528 support series, right?
Which I am preparing as we speak, so I am supposed to fix them there,
and just wanted to double check whether my solution is in line with what
you had in mind. After all you seem to be deeper into this CPUCFG stuff
than I am.

Cheers,
Andre