Message ID | 20230816173420.83232-3-CFSworks@gmail.com |
---|---|
State | Superseded |
Delegated to: | Andre Przywara |
Headers | show |
Series | Allwinner R528/T113s PSCI | expand |
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; > }
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
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; }
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(-)