Re: [PATCH 2/5] sunxi: move Cortex SMPEN setting into start.S

[Andre Przywara <andre.przywara@arm.com>]

On Mon, 24 Jan 2022 20:47:30 -0600
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

many thanks for having a look!

> On 1/24/22 7:15 PM, Andre Przywara wrote:
> > According to their TRMs, Cortex ARMv7 CPUs with SMP support require the
> > ACTLR.SMPEN bit to be set as early as possible, before any cache or TLB
> > maintenance operations are done. As we do those things still in start.S,
> > we need to move the SMPEN bit setting there, too.
> > 
> > This introduces a new ARMv7 wide symbol and code to set bit 6 in ACTLR
> > very early in start.S, and moves sunxi boards over to use that instead
> > of the custom code we had in our board.c file (where it was called
> > technically too late).
> > 
> > In practice we got away with this so far, because at this point all the
> > other cores were still in reset, so any broadcasting would have been
> > ignored anyway. But it is architecturally cleaner to do it early, and
> > we move a core specific piece of code out of board.c.
> > 
> > This also gets rid of the ARM_CORTEX_CPU_IS_UP kludge I introduced a few
> > years back, and moves the respective logic into the new Kconfig entry.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/Kconfig            |  3 ---
> >  arch/arm/cpu/armv7/Kconfig  |  7 +++++++
> >  arch/arm/cpu/armv7/start.S  | 11 +++++++++++
> >  arch/arm/mach-sunxi/Kconfig |  2 --
> >  arch/arm/mach-sunxi/board.c |  9 ---------
> >  5 files changed, 18 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 6b11c3a50d9..7893d74fab2 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -452,9 +452,6 @@ config ENABLE_ARM_SOC_BOOT0_HOOK
> >  	  values, then choose this option, and create a file included as
> >  	  <asm/arch/boot0.h> which contains the required assembler code.
> >  
> > -config ARM_CORTEX_CPU_IS_UP
> > -	bool
> > -
> >  config USE_ARCH_MEMCPY
> >  	bool "Use an assembly optimized implementation of memcpy"
> >  	default y if !ARM64
> > diff --git a/arch/arm/cpu/armv7/Kconfig b/arch/arm/cpu/armv7/Kconfig
> > index 60bb0a9e1ec..85e5229a060 100644
> > --- a/arch/arm/cpu/armv7/Kconfig
> > +++ b/arch/arm/cpu/armv7/Kconfig
> > @@ -76,4 +76,11 @@ config ARMV7_LPAE
> >  	Say Y here to use the long descriptor page table format. This is
> >  	required if U-Boot runs in HYP mode.
> >  
> > +config ARMV7_SET_CORTEX_SMPEN
> > +	bool "Enable ACTLR.SMP bit"
> > +	default n if MACH_SUN4I || MACH_SUN5I
> > +	default y if ARCH_SUNXI && !ARM64  
> 
> If this is required by the CPU, why is it shown to the user? Shouldn't it be
> selected by MACH_SUN6/7/8/9I? That is what I see platforms doing with
> ARMV8_SET_SMPEN.

You are right, it shouldn't be user visible, instead selected by platform
Kconfigs. Technically we could even work that out at runtime (read MIDR &
MPIDR), but I think there are situations were we want to avoid this, so we
would probably need an opt-in from the platform anyways.
This patch was just trying to mimic the existing logic as close as
possible, since we know that works for years. Changing this is scary
enough. And the negative logic is shorter than enumerating all ARMv7 SMP
cores, the list of which could even grow.

But indeed there is more cleanup potential here. I see some platforms
setting this bit in their code (similar to what sunxi did), but I didn't
dare to touch those. Maybe I add a follow up patch to move those platforms
over, and possibly merge this with ARMV8_SET_SMPEN on the way.

Cheers,
Andre

> > +	help
> > +	  Enable the ARM Cortex ACTLR.SMP enable bit on startup.
> > +
> >  endif
> > diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> > index 698e15b8e18..af87a5432ae 100644
> > --- a/arch/arm/cpu/armv7/start.S
> > +++ b/arch/arm/cpu/armv7/start.S
> > @@ -173,6 +173,17 @@ ENDPROC(switch_to_hypervisor)
> >   *
> >   *************************************************************************/
> >  ENTRY(cpu_init_cp15)
> > +
> > +#if CONFIG_IS_ENABLED(ARMV7_SET_CORTEX_SMPEN)
> > +	/*
> > +	 * The Arm Cortex-A7 TRM says this bit must be enabled before
> > +	 * "any cache or TLB maintenance operations are performed".
> > +	 */
> > +	mrc	p15, 0, r0, c1, c0, 1	@ read auxilary control register
> > +	orr	r0, r0, #1 << 6		@ set SMP bit to enable coherency
> > +	mcr	p15, 0, r0, c1, c0, 1	@ write auxilary control register
> > +#endif
> > +
> >  	/*
> >  	 * Invalidate L1 I/D
> >  	 */
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index 2c18cf02d1a..4e49ad4f0c0 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -186,7 +186,6 @@ choice
> >  config MACH_SUN4I
> >  	bool "sun4i (Allwinner A10)"
> >  	select CPU_V7A
> > -	select ARM_CORTEX_CPU_IS_UP
> >  	select PHY_SUN4I_USB
> >  	select DRAM_SUN4I
> >  	select SUNXI_GEN_SUN4I
> > @@ -197,7 +196,6 @@ config MACH_SUN4I
> >  config MACH_SUN5I
> >  	bool "sun5i (Allwinner A13)"
> >  	select CPU_V7A
> > -	select ARM_CORTEX_CPU_IS_UP
> >  	select DRAM_SUN4I
> >  	select PHY_SUN4I_USB
> >  	select SUNXI_GEN_SUN4I
> > diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> > index 390c9f8850d..8667ddf58e3 100644
> > --- a/arch/arm/mach-sunxi/board.c
> > +++ b/arch/arm/mach-sunxi/board.c
> > @@ -218,15 +218,6 @@ void s_init(void)
> >  	/* A83T BSP never modifies SUNXI_SRAMC_BASE + 0x44 */
> >  	/* No H3 BSP, boot0 seems to not modify SUNXI_SRAMC_BASE + 0x44 */
> >  #endif
> > -
> > -#if !defined(CONFIG_ARM_CORTEX_CPU_IS_UP) && !defined(CONFIG_ARM64)
> > -	/* Enable SMP mode for CPU0, by setting bit 6 of Auxiliary Ctl reg */
> > -	asm volatile(
> > -		"mrc p15, 0, r0, c1, c0, 1\n"
> > -		"orr r0, r0, #1 << 6\n"
> > -		"mcr p15, 0, r0, c1, c0, 1\n"
> > -		::: "r0");
> > -#endif
> >  }
> >  
> >  #define SUNXI_INVALID_BOOT_SOURCE	-1
> >   
>