Re: [PATCH 3/5] sunxi: move early "SRAM setup" into separate file

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

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

Hi Samuel,

> On 1/24/22 7:15 PM, Andre Przywara wrote:
> > Currently we do some magic "SRAM setup" MMIO writes in s_init(), copied
> > from the original BSP U-Boot. The comment speaks of this being required
> > before DRAM access gets enabled, but there is no indication that this
> > would actually be required that early.
> > 
> > Move this out of s_init(), into board_init_f(). Since this actually only
> > affects a very few older SoCs, the actual code goes into the cpu/armv7
> > directory, to move it out of the way for all other SoCs.
> > 
> > This also uses the opportunity to convert some #ifdefs over to the fancy
> > IS_ENABLED() macros used in actual C code.
> > 
> > We keep the s_init() stub around for now, since armv8's lowlevel_init
> > still relies on it.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/cpu/armv7/sunxi/Makefile |  3 +++
> >  arch/arm/cpu/armv7/sunxi/sram.c   | 45 +++++++++++++++++++++++++++++++
> >  arch/arm/mach-sunxi/board.c       | 38 +++++---------------------
> >  3 files changed, 54 insertions(+), 32 deletions(-)
> >  create mode 100644 arch/arm/cpu/armv7/sunxi/sram.c
> > 
> > diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile
> > index 1d40d6a18dc..ad11be78632 100644
> > --- a/arch/arm/cpu/armv7/sunxi/Makefile
> > +++ b/arch/arm/cpu/armv7/sunxi/Makefile
> > @@ -10,6 +10,9 @@ obj-y	+= timer.o
> >  obj-$(CONFIG_MACH_SUN6I)	+= tzpc.o
> >  obj-$(CONFIG_MACH_SUN8I_H3)	+= tzpc.o
> >  
> > +obj-$(CONFIG_MACH_SUN6I)	+= sram.o
> > +obj-$(CONFIG_MACH_SUN8I)	+= sram.o
> > +
> >  ifndef CONFIG_SPL_BUILD
> >  obj-$(CONFIG_ARMV7_PSCI)	+= psci.o
> >  endif
> > diff --git a/arch/arm/cpu/armv7/sunxi/sram.c b/arch/arm/cpu/armv7/sunxi/sram.c
> > new file mode 100644
> > index 00000000000..19395cce17c
> > --- /dev/null
> > +++ b/arch/arm/cpu/armv7/sunxi/sram.c
> > @@ -0,0 +1,45 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * (C) Copyright 2012 Henrik Nordstrom <henrik@henriknordstrom.net>
> > + *
> > + * (C) Copyright 2007-2011
> > + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
> > + * Tom Cubie <tangliang@allwinnertech.com>
> > + *
> > + * SRAM init for older sunxi SoCs.
> > + */
> > +
> > +#include <common.h>
> > +#include <init.h>
> > +#include <asm/io.h>
> > +
> > +void sunxi_sram_init(void)
> > +{
> > +	/*
> > +	 * Undocumented magic taken from boot0, without this DRAM
> > +	 * access gets messed up (seems cache related).
> > +	 * The boot0 sources describe this as: "config ema for cache sram"
> > +	 * Newer SoCs (A83T, H3 and anything beyond) don't need this anymore.
> > +	 */
> > +	if (IS_ENABLED(CONFIG_MACH_SUN6I))
> > +		setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0x1800);
> > +
> > +	if (IS_ENABLED(CONFIG_MACH_SUN8I)) {
> > +		uint version;
> > +
> > +		/* Unlock sram version info reg, read it, relock */
> > +		setbits_le32(SUNXI_SRAMC_BASE + 0x24, (1 << 15));
> > +		version = readl(SUNXI_SRAMC_BASE + 0x24) >> 16;
> > +		clrbits_le32(SUNXI_SRAMC_BASE + 0x24, (1 << 15));  
> 
> This is an open-coded version of sunxi_get_sram_id().

Indeed, thanks. Needs a prototype, though, I just picked one header file
in all of this mess ;-)

> > +
> > +		if (IS_ENABLED(CONFIG_MACH_SUN8I_A23)) {
> > +			if (version == 0x1650)
> > +				setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0x1800);
> > +			else /* 0x1661 ? */
> > +				setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0xc0);
> > +		} else if (IS_ENABLED(CONFIG_MACH_SUN8I_A33)) {
> > +			if (version != 0x1667)
> > +				setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0xc0);
> > +		}
> > +	}
> > +}
> > diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> > index 8667ddf58e3..42ec02d96e3 100644
> > --- a/arch/arm/mach-sunxi/board.c
> > +++ b/arch/arm/mach-sunxi/board.c
> > @@ -186,38 +186,6 @@ SPL_LOAD_IMAGE_METHOD("FEL", 0, BOOT_DEVICE_BOARD, spl_board_load_image);
> >  
> >  void s_init(void)
> >  {
> > -	/*
> > -	 * Undocumented magic taken from boot0, without this DRAM
> > -	 * access gets messed up (seems cache related).
> > -	 * The boot0 sources describe this as: "config ema for cache sram"
> > -	 */
> > -#if defined CONFIG_MACH_SUN6I
> > -	setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0x1800);
> > -#elif defined CONFIG_MACH_SUN8I
> > -	__maybe_unused uint version;
> > -
> > -	/* Unlock sram version info reg, read it, relock */
> > -	setbits_le32(SUNXI_SRAMC_BASE + 0x24, (1 << 15));
> > -	version = readl(SUNXI_SRAMC_BASE + 0x24) >> 16;
> > -	clrbits_le32(SUNXI_SRAMC_BASE + 0x24, (1 << 15));
> > -
> > -	/*
> > -	 * Ideally this would be a switch case, but we do not know exactly
> > -	 * which versions there are and which version needs which settings,
> > -	 * so reproduce the per SoC code from the BSP.
> > -	 */
> > -#if defined CONFIG_MACH_SUN8I_A23
> > -	if (version == 0x1650)
> > -		setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0x1800);
> > -	else /* 0x1661 ? */
> > -		setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0xc0);
> > -#elif defined CONFIG_MACH_SUN8I_A33
> > -	if (version != 0x1667)
> > -		setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0xc0);
> > -#endif
> > -	/* A83T BSP never modifies SUNXI_SRAMC_BASE + 0x44 */
> > -	/* No H3 BSP, boot0 seems to not modify SUNXI_SRAMC_BASE + 0x44 */
> > -#endif
> >  }
> >  
> >  #define SUNXI_INVALID_BOOT_SOURCE	-1
> > @@ -312,8 +280,14 @@ u32 spl_boot_device(void)
> >  	return sunxi_get_boot_device();
> >  }
> >  
> > +__weak void sunxi_sram_init(void)
> > +{
> > +}  
> 
> In configurations other than MACH_SUN6/8I, the strong definition of
> sunxi_sram_init() is also an empty function, so this is no more efficient than
> unconditionally compiling sram.c.

I know, but I put this here to be able to keep this SRAM cruft in
cpu/armv7/sunxi, and avoid compiling this file for everyone else (ARM9,
ARMv8, RISC-V).
Do you have a better idea? I actually tried several ways before, and
this seemed to be the cleanest. Yes, we do a call to a ret, for most
boards, but we probably have bigger problems.
And I consider this just the beginning of a sunxi cleanup journey, so
we can revisit this later.

Cheers,
Andre

> 
> > +
> >  void board_init_f(ulong dummy)
> >  {
> > +	sunxi_sram_init();
> > +
> >  #if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I_H3
> >  	/* Enable non-secure access to some peripherals */
> >  	tzpc_init();
> >   
>