[PATCH 0/3] Allwinner R528/T113s PSCI

[PATCH 0/3] Allwinner R528/T113s PSCI

[Sam Edwards]

Hi list,

With Andre's R528 support (hopefully) landing soon, I figure now is time to
submit these patches for review. They refactor the ARMv7/sunxi PSCI
implementation to allow for the new NCAT2 CPU complex control registers, and
add support for the R528/T113s.

Depends on series 365063.

Thanks,
Sam

Sam Edwards (3):
  sunxi: psci: clean away preprocessor macros
  sunxi: psci: refactor register access to separate functions
  sunxi: psci: implement PSCI on R528

 arch/arm/cpu/armv7/sunxi/psci.c | 185 +++++++++++++++++++++-----------
 arch/arm/mach-sunxi/Kconfig     |   2 +
 include/configs/sunxi-common.h  |   8 ++
 3 files changed, 133 insertions(+), 62 deletions(-)

-- 
2.41.0

[PATCH 1/3] 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 | 94 ++++++++++++++-------------------
 1 file changed, 41 insertions(+), 53 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
index e1d3638b5c..7809b074f8 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -76,28 +76,24 @@ 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;
-		writel(tmp, clamp);
-	} while (tmp);
-
-	__mdelay(10);
-#endif
+	if (clamp) {
+		u32 tmp = 0x1ff;
+		do {
+			tmp >>= 1;
+			writel(tmp, clamp);
+		} while (tmp);
+
+		__mdelay(10);
+	}
 }
 
-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
+	if (clamp) {
+		writel(0xff, clamp);
+	}
 }
 
 static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
@@ -118,53 +114,45 @@ static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
 	}
 }
 
-#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)
-{
-	struct sunxi_cpucfg_reg *cpucfg =
-		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
+	/* 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 {
+		struct sunxi_cpucfg_reg *cpucfg =
+			(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
-	writel((u32)entry, &cpucfg->priv0);
+		writel((u32)entry, &cpucfg->priv0);
+	}
 }
-#endif
 
-#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;
-
-	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;
 
-	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;
+	/* sun7i (A20) is different from other single cluster SoCs */
+	if (IS_ENABLED(CONFIG_MACH_SUN7I)) {
+		sunxi_power_switch(NULL, &cpucfg->cpu1_pwroff, on, 0);
+	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
+		sunxi_power_switch(NULL, (void *)cpucfg + SUN8I_R40_PWROFF,
+				   on, cpu);
+	} else {
+#if !defined(CONFIG_SUN50I_GEN_H6) && !defined(CONFIG_SUNXI_GEN_NCAT2)
+		struct sunxi_prcm_reg *prcm =
+			(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
 
-	sunxi_power_switch(&prcm->cpu_pwr_clamp[cpu], &prcm->cpu_pwroff,
-			   on, cpu);
+		u32 *clamp = &prcm->cpu_pwr_clamp[cpu];
+		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
+		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))
+			clamp = NULL;
+
+		sunxi_power_switch(clamp, &prcm->cpu_pwroff, on, cpu);
+#endif
+	}
 }
-#endif /* CONFIG_MACH_SUN7I */
 
 void __secure sunxi_cpu_power_off(u32 cpuid)
 {
-- 
2.41.0

[PATCH 2/3] 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 7809b074f8..94120e7526 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -114,7 +114,7 @@ static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
 	}
 }
 
-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)) {
@@ -154,30 +154,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)
@@ -234,33 +264,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

[PATCH 3/3] 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 | 47 ++++++++++++++++++++++++++++++++-
 arch/arm/mach-sunxi/Kconfig     |  2 ++
 include/configs/sunxi-common.h  |  8 ++++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
index 94120e7526..f3c1c459c2 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -38,6 +38,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));
@@ -116,10 +129,13 @@ static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
 
 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 {
 		struct sunxi_cpucfg_reg *cpucfg =
 			(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
@@ -139,6 +155,8 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
 	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
 		sunxi_power_switch(NULL, (void *)cpucfg + SUN8I_R40_PWROFF,
 				   on, cpu);
+	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
+		/* R528 leaves both cores powered up, manages them via reset */
 	} else {
 #if !defined(CONFIG_SUN50I_GEN_H6) && !defined(CONFIG_SUNXI_GEN_NCAT2)
 		struct sunxi_prcm_reg *prcm =
@@ -159,6 +177,17 @@ static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
 	struct sunxi_cpucfg_reg *cpucfg =
 		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
+	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
+		if (reset) {
+			clrbits_le32(SUNXI_CPUX_BASE + SUN8I_R528_C0_RST_CTRL,
+				     BIT(cpu));
+		} else {
+			setbits_le32(SUNXI_CPUX_BASE + SUN8I_R528_C0_RST_CTRL,
+				     BIT(cpu));
+		}
+		return;
+	}
+
 	writel(reset ? 0b00 : 0b11, &cpucfg->cpu[cpu].rst);
 }
 
@@ -167,6 +196,11 @@ static void __secure sunxi_cpu_set_locking(int cpu, bool lock)
 	struct sunxi_cpucfg_reg *cpucfg =
 		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
+	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
+		/* Not required on R528 */
+		return;
+	}
+
 	if (lock)
 		clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
 	else
@@ -178,6 +212,11 @@ static bool __secure sunxi_cpu_poll_wfi(int cpu)
 	struct sunxi_cpucfg_reg *cpucfg =
 		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
+	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
+		return !!(readl(SUNXI_CPUX_BASE + SUN8I_R528_C0_CPU_STATUS) &
+			  BIT(SUN8I_R528_C0_STATUS_STANDBYWFI + cpu));
+	}
+
 	return !!(readl(&cpucfg->cpu[cpu].status) & BIT(2));
 }
 
@@ -186,6 +225,12 @@ static void __secure sunxi_cpu_invalidate_cache(int cpu)
 	struct sunxi_cpucfg_reg *cpucfg =
 		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
 
+	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
+		clrbits_le32(SUNXI_CPUX_BASE + SUN8I_R528_C0_CTRL_REG0,
+			     BIT(cpu));
+		return;
+	}
+
 	clrbits_le32(&cpucfg->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 0/3] Allwinner R528/T113s PSCI

[Andre Przywara]

On Fri, 11 Aug 2023 18:30:52 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

many many thanks for sending this, I especially like your clean up around
the #ifdef's!

The patches looks good on the first glance (apart from some regression in
patch 3/3), but I will reply to them individually.

Cheers,
Andre

> Hi list,
> 
> With Andre's R528 support (hopefully) landing soon, I figure now is time to
> submit these patches for review. They refactor the ARMv7/sunxi PSCI
> implementation to allow for the new NCAT2 CPU complex control registers, and
> add support for the R528/T113s.
> 
> Depends on series 365063.
> 
> Thanks,
> Sam
> 
> Sam Edwards (3):
>   sunxi: psci: clean away preprocessor macros
>   sunxi: psci: refactor register access to separate functions
>   sunxi: psci: implement PSCI on R528
> 
>  arch/arm/cpu/armv7/sunxi/psci.c | 185 +++++++++++++++++++++-----------
>  arch/arm/mach-sunxi/Kconfig     |   2 +
>  include/configs/sunxi-common.h  |   8 ++
>  3 files changed, 133 insertions(+), 62 deletions(-)
> 

Re: [PATCH 3/3] sunxi: psci: implement PSCI on R528

[Andre Przywara]

On Fri, 11 Aug 2023 18:30:55 -0600
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.

Unfortunately this patch breaks the build on older 32-bit SoCs, as
SUNXI_CPUX_BASE is not defined there. That's a typical problem of the
"C-if" approach, but typically there is a clean, albeit not trivial,
solution:

It seems like SUNXI_CPUX_BASE and SUNXI_CPUCFG_BASE are mutually
exclusive, and I actually carry a "#define SUNXI_CPUCFG_BASE 0" hack in my
patches already.
So I wonder if we should unify those two into SUNXI_CPUCFG_BASE, with the
following rework:
 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> Tested-by: Maksim Kiselev <bigunclemax@gmail.com>
> ---
>  arch/arm/cpu/armv7/sunxi/psci.c | 47 ++++++++++++++++++++++++++++++++-
>  arch/arm/mach-sunxi/Kconfig     |  2 ++
>  include/configs/sunxi-common.h  |  8 ++++++
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index 94120e7526..f3c1c459c2 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -38,6 +38,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));
> @@ -116,10 +129,13 @@ static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
>  
>  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 {
>  		struct sunxi_cpucfg_reg *cpucfg =
>  			(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> @@ -139,6 +155,8 @@ static void __secure sunxi_cpu_set_power(int cpu, bool on)
>  	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
>  		sunxi_power_switch(NULL, (void *)cpucfg + SUN8I_R40_PWROFF,
>  				   on, cpu);
> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		/* R528 leaves both cores powered up, manages them via reset */
>  	} else {
>  #if !defined(CONFIG_SUN50I_GEN_H6) && !defined(CONFIG_SUNXI_GEN_NCAT2)
>  		struct sunxi_prcm_reg *prcm =
> @@ -159,6 +177,17 @@ static void __secure sunxi_cpu_set_reset(int cpu, bool reset)
>  	struct sunxi_cpucfg_reg *cpucfg =
>  		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;

Can you check what it would take to get rid of this struct altogether, and
replace accesses with "SUNXI_CPUCFG_BASE + REG_OFFSET", similiar to what
you do for the R528?
I was never a friend of ab-using C structs to model hardware register
layouts, so it seems like a good excuse to get rid of that here ;-)

Then we can define SUNXI_CPUCFG_BASE everywhere, and don't need SUNXI_CPUX_BASE.

Or do I miss something here?

Cheers,
Andre

P.S.: You can test-build for all SoCs with buildman:
$ tools/buildman/buildman -j 4 sunxi
(you might need some setup in ~/.buildman to announce your cross-compilers)
Or you just build one board per SoC, for instance "Bananapi_defconfig" for
the A20.

>  
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		if (reset) {
> +			clrbits_le32(SUNXI_CPUX_BASE + SUN8I_R528_C0_RST_CTRL,
> +				     BIT(cpu));
> +		} else {
> +			setbits_le32(SUNXI_CPUX_BASE + SUN8I_R528_C0_RST_CTRL,
> +				     BIT(cpu));
> +		}
> +		return;
> +	}
> +
>  	writel(reset ? 0b00 : 0b11, &cpucfg->cpu[cpu].rst);
>  }
>  
> @@ -167,6 +196,11 @@ static void __secure sunxi_cpu_set_locking(int cpu, bool lock)
>  	struct sunxi_cpucfg_reg *cpucfg =
>  		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
>  
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		/* Not required on R528 */
> +		return;
> +	}
> +
>  	if (lock)
>  		clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu));
>  	else
> @@ -178,6 +212,11 @@ static bool __secure sunxi_cpu_poll_wfi(int cpu)
>  	struct sunxi_cpucfg_reg *cpucfg =
>  		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
>  
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		return !!(readl(SUNXI_CPUX_BASE + SUN8I_R528_C0_CPU_STATUS) &
> +			  BIT(SUN8I_R528_C0_STATUS_STANDBYWFI + cpu));
> +	}
> +
>  	return !!(readl(&cpucfg->cpu[cpu].status) & BIT(2));
>  }
>  
> @@ -186,6 +225,12 @@ static void __secure sunxi_cpu_invalidate_cache(int cpu)
>  	struct sunxi_cpucfg_reg *cpucfg =
>  		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
>  
> +	if (IS_ENABLED(CONFIG_MACH_SUN8I_R528)) {
> +		clrbits_le32(SUNXI_CPUX_BASE + SUN8I_R528_C0_CTRL_REG0,
> +			     BIT(cpu));
> +		return;
> +	}
> +
>  	clrbits_le32(&cpucfg->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

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

[Andre Przywara]

On Fri, 11 Aug 2023 18:30:53 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

many thanks for that cleanup, that's very much welcome!

I am still comparing the outcome for the different SoC families, and
testing this on the boards I have, but two things I stumbled upon already:

> 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 | 94 ++++++++++++++-------------------
>  1 file changed, 41 insertions(+), 53 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index e1d3638b5c..7809b074f8 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -76,28 +76,24 @@ 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;
> -		writel(tmp, clamp);
> -	} while (tmp);
> -
> -	__mdelay(10);
> -#endif
> +	if (clamp) {
> +		u32 tmp = 0x1ff;
> +		do {
> +			tmp >>= 1;
> +			writel(tmp, clamp);
> +		} while (tmp);
> +
> +		__mdelay(10);
> +	}
>  }
>  
> -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
> +	if (clamp) {
> +		writel(0xff, clamp);
> +	}
>  }
>  
>  static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
> @@ -118,53 +114,45 @@ static void __secure sunxi_power_switch(u32 *clamp, u32 *pwroff, bool on,
>  	}
>  }
>  
> -#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)
> -{
> -	struct sunxi_cpucfg_reg *cpucfg =
> -		(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
> +	/* 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 {
> +		struct sunxi_cpucfg_reg *cpucfg =
> +			(struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE;
>  
> -	writel((u32)entry, &cpucfg->priv0);
> +		writel((u32)entry, &cpucfg->priv0);
> +	}
>  }
> -#endif
>  
> -#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;
> -
> -	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;
>  
> -	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;
> +	/* sun7i (A20) is different from other single cluster SoCs */
> +	if (IS_ENABLED(CONFIG_MACH_SUN7I)) {
> +		sunxi_power_switch(NULL, &cpucfg->cpu1_pwroff, on, 0);
> +	} else if (IS_ENABLED(CONFIG_MACH_SUN8I_R40)) {
> +		sunxi_power_switch(NULL, (void *)cpucfg + SUN8I_R40_PWROFF,
> +				   on, cpu);
> +	} else {
> +#if !defined(CONFIG_SUN50I_GEN_H6) && !defined(CONFIG_SUNXI_GEN_NCAT2)

So I think we can get rid of this:
- GEN_H6 never compiles this code here, as both H6 and H616 are arm64.
- We can define SUNXI_PRCM_BASE for NCAT2, I believe Samuel once
mentioned that the D1/T113 does have such a block, actually.
- The non-existing cpu_pwr_clamp member should go away when you switch to
a BASE_ADDR + REG_OFFSET approach, I think.

> +		struct sunxi_prcm_reg *prcm =
> +			(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
>  
> -	sunxi_power_switch(&prcm->cpu_pwr_clamp[cpu], &prcm->cpu_pwroff,
> -			   on, cpu);
> +		u32 *clamp = &prcm->cpu_pwr_clamp[cpu];
> +		if (IS_ENABLED(CONFIG_MACH_SUN6I) ||
> +		    IS_ENABLED(CONFIG_MACH_SUN8I_H3))

Shouldn't that be the opposite? In the existing code, sun6i and H3 DO
program the clamp (see the "-" section above).

Cheers,
Andre

> +			clamp = NULL;
> +
> +		sunxi_power_switch(clamp, &prcm->cpu_pwroff, on, cpu);
> +#endif
> +	}
>  }
> -#endif /* CONFIG_MACH_SUN7I */
>  
>  void __secure sunxi_cpu_power_off(u32 cpuid)
>  {

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

[Sam Edwards]

Hi Andre,

On 8/14/23 10:37, Andre Przywara wrote:
> So I think we can get rid of this:
> - GEN_H6 never compiles this code here, as both H6 and H616 are arm64.

Easy!

> - We can define SUNXI_PRCM_BASE for NCAT2, I believe Samuel once
> mentioned that the D1/T113 does have such a block, actually.

Will you be taking care of this in v2 of your T113s series, or should I 
be adding it (in which case I'll need to know the location of the block)?

> - The non-existing cpu_pwr_clamp member should go away when you switch to
> a BASE_ADDR + REG_OFFSET approach, I think.

Less easy, but still can do.

> Shouldn't that be the opposite? In the existing code, sun6i and H3 DO
> program the clamp (see the "-" section above).

And sun7i and R40, as well. It appears I simply read the #if 
defined(...) mess backwards. I'll fix that for v2. As a bonus, this 
lends itself to a rather nice refactoring of sunxi_cpu_set_power() where 
I can have the if block only determine the pwroff/clamp addresses, and 
have a single tail-call to sunxi_power_switch() at the bottom. Since the 
latter function is so simple, I may as well just inline it into 
sunxi_cpu_set_power() (which I suspect might be more readable).

> Cheers,
> Andre

Likewise,
Sam

Re: [PATCH 0/3] Allwinner R528/T113s PSCI

[Sam Edwards]

On 8/14/23 08:06, Andre Przywara wrote:
> Hi Sam,
> 
> many many thanks for sending this, I especially like your clean up around
> the #ifdef's!
> 
> The patches looks good on the first glance (apart from some regression in
> patch 3/3), but I will reply to them individually.
> 
> Cheers,
> Andre

Thanks for your attention to these! While you're in a PSCI headspace, 
could you also look at my patch titled:
sunxi: psci: remove redundant initialization from psci_arch_init

...dated May 31?

This one should be pretty uncontroversial and straightforward to review. :)

Many thanks,
Sam

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

[Andre Przywara]

On Mon, 14 Aug 2023 12:10:25 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

> Hi Andre,
> 
> On 8/14/23 10:37, Andre Przywara wrote:
> > So I think we can get rid of this:
> > - GEN_H6 never compiles this code here, as both H6 and H616 are arm64.  
> 
> Easy!
> 
> > - We can define SUNXI_PRCM_BASE for NCAT2, I believe Samuel once
> > mentioned that the D1/T113 does have such a block, actually.  
> 
> Will you be taking care of this in v2 of your T113s series, or should I 
> be adding it (in which case I'll need to know the location of the block)?

Yes, I will add this to the header file, either defined as 0, or to its
actual address.
 
> > - The non-existing cpu_pwr_clamp member should go away when you switch to
> > a BASE_ADDR + REG_OFFSET approach, I think.  
> 
> Less easy, but still can do.
> 
> > Shouldn't that be the opposite? In the existing code, sun6i and H3 DO
> > program the clamp (see the "-" section above).  
> 
> And sun7i and R40, as well.

Yes, but you handle both above explicitly.

> It appears I simply read the #if 
> defined(...) mess backwards. I'll fix that for v2. As a bonus, this 
> lends itself to a rather nice refactoring of sunxi_cpu_set_power() where 
> I can have the if block only determine the pwroff/clamp addresses, and 
> have a single tail-call to sunxi_power_switch() at the bottom. Since the 
> latter function is so simple, I may as well just inline it into 
> sunxi_cpu_set_power() (which I suspect might be more readable).

Yes, any further simplification is welcome, and probably somewhat
rewarding in this case ;-)

Cheers,
Andre

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

[Sam Edwards]

On 8/14/23 15:05, Andre Przywara wrote:
> Yes, I will add this to the header file, either defined as 0, or to its
> actual address.

Gotcha; my v2 will also assume you've taken care of merging these guys:
+#define SUNXI_CPUX_BASE			0x09010000
+#define SUNXI_CPUCFG_BASE		0

(As I presume from your comments on 3/3 that it's better to consider 
"CPUX" simply an updated CPUCFG than to distinguish between them as 
fundamentally different concepts.)

> Yes, but you handle both above explicitly.

No, I was passing a NULL clamp address in those cases too. So I'm noting 
that this must also be fixed in v2.

Thanks,
Sam

Re: [PATCH 3/3] sunxi: psci: implement PSCI on R528

[Sam Edwards]

On 8/14/23 08:16, Andre Przywara wrote:
> Hi Sam,
> 
>> This patch adds the necessary code to make nonsec booting and PSCI
>> secondary core management functional on the R528/T113.
> 
> Unfortunately this patch breaks the build on older 32-bit SoCs, as
> SUNXI_CPUX_BASE is not defined there. That's a typical problem of the
> "C-if" approach, but typically there is a clean, albeit not trivial,
> solution:
> 
> It seems like SUNXI_CPUX_BASE and SUNXI_CPUCFG_BASE are mutually
> exclusive, and I actually carry a "#define SUNXI_CPUCFG_BASE 0" hack in my
> patches already.
> So I wonder if we should unify those two into SUNXI_CPUCFG_BASE, with the
> following rework:

The SUNXI_CPUX_BASE -> SUNXI_CPUCFG_BASE rename worked excellently. 
We're having the same problem with SUNXI_R_CPUCFG_BASE as well, though. 
How do you want to handle that?

Thanks,
Sam

Re: [PATCH 3/3] sunxi: psci: implement PSCI on R528

[Andre Przywara]

On Tue, 15 Aug 2023 13:17:33 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

> On 8/14/23 08:16, Andre Przywara wrote:
> > Hi Sam,
> >   
> >> This patch adds the necessary code to make nonsec booting and PSCI
> >> secondary core management functional on the R528/T113.  
> > 
> > Unfortunately this patch breaks the build on older 32-bit SoCs, as
> > SUNXI_CPUX_BASE is not defined there. That's a typical problem of the
> > "C-if" approach, but typically there is a clean, albeit not trivial,
> > solution:
> > 
> > It seems like SUNXI_CPUX_BASE and SUNXI_CPUCFG_BASE are mutually
> > exclusive, and I actually carry a "#define SUNXI_CPUCFG_BASE 0" hack in my
> > patches already.
> > So I wonder if we should unify those two into SUNXI_CPUCFG_BASE, with the
> > following rework:  
> 
> The SUNXI_CPUX_BASE -> SUNXI_CPUCFG_BASE rename worked excellently. 
> We're having the same problem with SUNXI_R_CPUCFG_BASE as well, though. 
> How do you want to handle that?

So that's a bit more nasty indeed. I don't even know if R_CPUCFG really
makes sense here, as the _R_ term typically refers to the management
processor, which the D1/R528 don't have. Or at least the always-on power
domain, but then again this hardly relates to the secondary entry
point. I think the name was just used because the address matches the
one used in the H6. Anyway, I got the impression that Allwinner just
uses registers wherever they find them, and that they don't care too
much about logical grouping or compatibility.

So taking a step back, I wonder if we should actually just define a
CONFIG_SUNXI_CPU_SOFT_ENTRY (or so) *Kconfig* symbol, which holds that
address, and let the per-SoC definition be solved in Kconfig instead.
Because SUNXI_R_CPUCFG_BASE and also SUNXI_RTC_BASE seem to be just
used as the base address for that purpose, with some magic offset
added, across all of U-Boot (ARMv8 FEL and v7 PSCI).

So can you try to work on that base? I will take care of
armv8/fel_utils.S, which uses some post-increment assembly trick to
keep the code small, which wouldn't work anymore. But I have an idea
how to solve this.

Cheers,
Andre

Re: [PATCH 3/3] sunxi: psci: implement PSCI on R528

[Sam Edwards]

On 8/15/23 15:59, Andre Przywara wrote:
> Hi Sam,

Hi Andre,

> So that's a bit more nasty indeed. I don't even know if R_CPUCFG really
> makes sense here, as the _R_ term typically refers to the management
> processor, which the D1/R528 don't have. Or at least the always-on power
> domain, but then again this hardly relates to the secondary entry
> point. I think the name was just used because the address matches the
> one used in the H6.

Oh, no. That was my doing (and my reasoning) by suggesting that for 
inclusion in your series. Yours is good reasoning to be rid of it.

> So taking a step back, I wonder if we should actually just define a
> CONFIG_SUNXI_CPU_SOFT_ENTRY (or so) *Kconfig* symbol, which holds that
> address, and let the per-SoC definition be solved in Kconfig instead.
> Because SUNXI_R_CPUCFG_BASE and also SUNXI_RTC_BASE seem to be just
> used as the base address for that purpose, with some magic offset
> added, across all of U-Boot (ARMv8 FEL and v7 PSCI).

Mmh, since this is a block of soft registers for managing several 
functions of both cores, I think I'd rather point to the base of the 
block and still use an offset to get to the specific soft register. 
Allwinner may keep this layout for a 4-core chip in the near future or 
U-Boot may want to add code that sets the CPU0 hotplug flag, for example.

I'm not unwilling to do the Kconfig route, but just out of curiosity, 
what would your fallback plan be?

> So can you try to work on that base? I will take care of
> armv8/fel_utils.S, which uses some post-increment assembly trick to
> keep the code small, which wouldn't work anymore. But I have an idea
> how to solve this.

Before that, I think now might be a good time for me to send in the v2 
that I have so far; I doubt the final patch of my v2 series will pass 
review, but I'd like to keep us synced up (and clear away any patches in 
that series that do pass review off from my mental desktop).

> Cheers,
> Andre

Likewise,
Sam

Re: [PATCH 3/3] sunxi: psci: implement PSCI on R528

[Andre Przywara]

On Tue, 15 Aug 2023 18:48:49 -0700
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

> On 8/15/23 15:59, Andre Przywara wrote:
> > Hi Sam,  
> 
> Hi Andre,
> 
> > So that's a bit more nasty indeed. I don't even know if R_CPUCFG really
> > makes sense here, as the _R_ term typically refers to the management
> > processor, which the D1/R528 don't have. Or at least the always-on power
> > domain, but then again this hardly relates to the secondary entry
> > point. I think the name was just used because the address matches the
> > one used in the H6.  
> 
> Oh, no. That was my doing (and my reasoning) by suggesting that for 
> inclusion in your series. Yours is good reasoning to be rid of it.
> 
> > So taking a step back, I wonder if we should actually just define a
> > CONFIG_SUNXI_CPU_SOFT_ENTRY (or so) *Kconfig* symbol, which holds that
> > address, and let the per-SoC definition be solved in Kconfig instead.
> > Because SUNXI_R_CPUCFG_BASE and also SUNXI_RTC_BASE seem to be just
> > used as the base address for that purpose, with some magic offset
> > added, across all of U-Boot (ARMv8 FEL and v7 PSCI).  
> 
> Mmh, since this is a block of soft registers for managing several 
> functions of both cores, I think I'd rather point to the base of the 
> block and still use an offset to get to the specific soft register. 
> Allwinner may keep this layout for a 4-core chip in the near future or 
> U-Boot may want to add code that sets the CPU0 hotplug flag, for example.

That's a neat idea, but I gave up hoping that there is some real pattern,
especially about the memory map. In the past (up to A64/H5) that was
pretty stable, but changed wildly multiple times already afterwards.
So I don't know about the future, and we can always fix that should the
need arise.
What I do see though now is the following:
R40: SUNXI_SRAMC_BASE + SUN8I_R40_SRAMC_SOFT_ENTRY_REG0
R528: SUNXI_R_CPUCFG_BASE + SUN8I_R528_SOFT_ENTRY
rest: SUNXI_CPUCFG_BASE + SUNXI_PRIV0
so three totally different base addresses plus different offsets. That
comes close to random for me.
And that continues if you look into the ARMv8 fel_utils.S:
==================
#ifdef CONFIG_MACH_SUN50I_H616
        ldr     w2, =(SUNXI_R_CPUCFG_BASE + 0x1c0)
        str     w0, [x2], #0x4
#elif CONFIG_MACH_SUN50I_H6
        ldr     w2, =(SUNXI_RTC_BASE + 0x1b8)   // BOOT_CPU_HP_FLAG_REG
        str     w0, [x2], #0x4
#else
        ldr     w2, =(SUNXI_CPUCFG_BASE + 0x1a4) // offset for CPU hotplug base
	str     w0, [x2, #0x8]
==================
Again different entry points for each new SoC.
So instead of trying to derive some pattern from where there is none, I'd
rather do: config SUNXI_CPU_HOTPLUG_ADDRESS
	hex
	default 0x01c000bc if MACH_SUN8I_R40
	....
	default 0x01c25da4
and then use that symbol in both places (v7 PSCI + v8 FEL).
As mentioned, I can fix the pieces that break when naively doing so in the
FEL code. That has also the rewarding property of allowing to remove both
SUNXI_R_CPUCFG_BASE and SUNXI_RTC_BASE from the code base completely.

What do you think?

Cheers,
Andre


> I'm not unwilling to do the Kconfig route, but just out of curiosity, 
> what would your fallback plan be?
> 
> > So can you try to work on that base? I will take care of
> > armv8/fel_utils.S, which uses some post-increment assembly trick to
> > keep the code small, which wouldn't work anymore. But I have an idea
> > how to solve this.  
> 
> Before that, I think now might be a good time for me to send in the v2 
> that I have so far; I doubt the final patch of my v2 series will pass 
> review, but I'd like to keep us synced up (and clear away any patches in 
> that series that do pass review off from my mental desktop).
> 
> > Cheers,
> > Andre  
> 
> Likewise,
> Sam

Re: [PATCH 3/3] sunxi: psci: implement PSCI on R528

[Sam Edwards]

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

Hi Andre,

> So instead of trying to derive some pattern from where there is none, I'd
> rather do: config SUNXI_CPU_HOTPLUG_ADDRESS
> 	hex
> 	default 0x01c000bc if MACH_SUN8I_R40

But the hotplug flag register is at 0x01c000b8 for R40?

> 	....
> 	default 0x01c25da4
> and then use that symbol in both places (v7 PSCI + v8 FEL).
> As mentioned, I can fix the pieces that break when naively doing so in the
> FEL code. That has also the rewarding property of allowing to remove both
> SUNXI_R_CPUCFG_BASE and SUNXI_RTC_BASE from the code base completely.

I might hold on to SUNXI_RTC_BASE for a bit, since the SPL may need it 
for the various reset-to-FEL mechanisms. I still want a nice way of 
getting mainline U-Boot to do this in the near term.

> What do you think?

I don't think this really solves your underlying grievance, since 
various sunxis need distinct offsets to jump from the hotplug address 
register to the secondary CPU soft entry point:

CONFIG_SUNXI_CPU_HOTPLUG_ADDRESS + 4 // for R40
CONFIG_SUNXI_CPU_HOTPLUG_ADDRESS + 8 // for R528

...at which point we're back to the "Kconfig symbol is really pointing 
to a sort of control block" idea.

We could take advantage of the fact that ARMv7 psci.c and ARMv8 
fel_utils.S are never both built and call it something horrible like 
"SUNXI_CPU_HOTPLUG_OR_SOFT_ENTRY_ADDR" but then it wouldn't be clear 
from a glance *which* it is. We could go with *two* (three, if we need 
R528's CPU0 entry for a reset-to-FEL mechanism) Kconfig symbols, but 
that's a lot for configuration symbols that are really only needed in 
pretty much one place each.

> 
> Cheers,
> Andre

Likewise,
Sam