On Wed, 16 Aug 2023 10:34:16 -0700 Sam Edwards <cfswo...@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 <cfswo...@gmail.com> Reviewed-by: Andre Przywara <andre.przyw...@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) > {