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

[Samuel Holland <samuel@sholland.org>]

On 1/27/22 6:59 PM, Andre Przywara wrote:
> 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).

Ah, right, that makes sense. I missed the directory change here.

> 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.

No, I am fine with your method.

Regards,
Samuel

> 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();
>>>   
>>
>