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