[PATCH] arm: stm32mp1: activate data cache in SPL and before relocation

[Patrick DELAUNAY]

Hi Marek,

> From: Marek Vasut <marex at denx.de>
> Sent: lundi 30 mars 2020 16:04
> 
> On 3/30/20 3:49 PM, Patrick DELAUNAY wrote:
> > Hi Marek,
> 
> Hi,
> 
> [...]
> 
> >>> -	/* Enable D-cache. I-cache is already enabled in start.S */
> >>> +	/* I-cache is already enabled in start.S */
> >
> > Not needed for arm V7 (I copy this function from other platfrom ... I
> > don't remember which one)
> 
> Maybe this needs to be de-duplicated if it's a copy ?

I don't remember.... 
As I mixed several references

But I found the same content in many arm arch;

arch/arm/mach-imx/mx5/soc.c:67
arch/arm/mach-rockchip/board.c:47
arch/arm/mach-tegra/board.c:271
arch/arm/mach-sunxi/board.c:347
arch/arm/mach-exynos/soc.c:30:
arch/arm/mach-zynq/cpu.c:88:
arch/arm/cpu/armv7/iproc-common/hwinit-common.c:1
arch/arm/mach-u8500/cache.c:14
arch/arm/mach-keystone/init.c:206

And different implementation in 
arch/arm/mach-socfpga/misc.c:55

mach-omap2/omap-cache.c:49
void enable_caches(void)
{

	/* Enable I cache if not enabled */
	if (!icache_status())
		icache_enable();

	dcache_enable();
}

the issue the weak function empty, so it is mandatory to
redefine the real implementation for each platform.

arch/arm/lib/cache.c:35
/*
 * Default implementation of enable_caches()
 * Real implementation should be in platform code
 */
__weak void enable_caches(void)
{
	puts("WARNING: Caches not enabled\n");
}

[...]

> >>>
> >>> +static void set_mmu_dcache(u32 addr, u32 size) {
> >>> +	int	i;
> >>> +	u32 start, end;
> >>> +
> >>> +	start = addr >> MMU_SECTION_SHIFT;
> >>> +	end = ((u64)addr + (u64)size) >> MMU_SECTION_SHIFT;
> >>
> >> Is this a copy of dram_bank_mmu_setup() in arch/arm/lib/cache-cp15.c ?
> >> Why ?
> >
> > It is not just a copy...
> >
> > set__mmu_dache is only a static helper for  function
> > dram_bank_mmu_setup()
> >
> > I override the default implementation of the weak functon
> > dram_bank_mmu_setup()
> 
> Can you instead augment the original implementation to cater for this usecase or
> would that be too difficult ?

Have a generic behavior...

I will propose to protect the access to bd->bi_dram[bank] in dram_bank_mmu_setup

[....]

> >>
> >>> diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h
> >>> index c34a720e0c..5203fc93ad 100644
> >>> --- a/include/configs/stm32mp1.h
> >>> +++ b/include/configs/stm32mp1.h
> >>> @@ -58,8 +58,8 @@
> >>>
> >>>  /* limit SYSRAM usage to first 128 KB */
> >>>  #define CONFIG_SPL_MAX_SIZE		0x00020000
> >>> -#define CONFIG_SPL_STACK		(STM32_SYSRAM_BASE + \
> >>> -					 STM32_SYSRAM_SIZE)
> >>> +/* stack at STM32_SYSRAM_BASE + STM32_SYSRAM_SIZE -
> >> PGTABLE_SIZE (=16kB) */
> >>> +#define CONFIG_SPL_STACK		0x2FFFC000
> >>
> >> Can't you memalign() the pagetable area instead of this hacking around?
> >> Or use something around board_init_f_alloc_reserve().
> >
> > It was my first idea: use malloc
> >
> > But as I try to activate the data cache as soon as possible.
> > So before spl_early_init call (for spl in board_init_f) and malloc is not yet
> accessible.
> >
> > And board_init_f_alloc_reserve  is only called in U-Boot board-f.c.....
> > after the MMU configuration for pre-relocation / not called in SPL.
> >
> > I don't see this address as hack but a memory configuration of SYSRAM:
> >
> > SYRAM content (board_f)
> > - SPL code
> > - SPL data
> > - SPL stack (reversed order)
> > - TTB
> >
> > But I can move it in BSS as global apl variable, I need to think about it....
> > It is probably more clean.
> 
> Please do :)

I willl move it in ".data" section in V2 for SPL and U-Boot.

Even in binary size increase and the SPL load time
by ROM code is increase by 30ms.

Patrick