[PATCH 2/2] ARM: i.MX8M: esdctl: split memory banks for devices with >4G

[Lucas Stach]

Am Donnerstag, dem 31.08.2023 um 15:05 +0200 schrieb Marco Felsch:
> At the moment the whole available memory is added to one single memory
> bank "ram0". This can cause barebox chainload issues on devices with a
> huge amount of memory like the i.MX8MP-EVK which has 6G of RAM if the
> barebox pbl binary is to large.
> 
> The reason for this issues is that memory_bank_first_find_space()
> returns the memory area with the largest amount of free space on the
> first memory bank. So in case of Debix SOM-A 8G and i.MX8MP-EVK 6G this
> is the area crossing the 4G boundary. This cause the barebox pbl code to
> trigger a MMU exception once the early MMU gets enabled which is
> configured for sizes <=4G.
> 
> Split the memory space into two memory banks: "ram0" and "ram1" to fix
> this issue.
> 
> Signed-off-by: Marco Felsch <m.felsch at pengutronix.de>
> ---
>  arch/arm/mach-imx/esdctl.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> index 54c62c47338e..de23b6433355 100644
> --- a/arch/arm/mach-imx/esdctl.c
> +++ b/arch/arm/mach-imx/esdctl.c
> @@ -510,16 +510,26 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem *ddrc, unsigned buswid
>  				   reduced_adress_space, mstr);
>  }
>  
> +static int _imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data,
> +			       unsigned int buswidth)
> +{
> +	resource_size_t size = imx8m_ddrc_sdram_size(mmdcbase, buswidth);
> +	resource_size_t size0, size1;
> +
> +	size0 = min_t(resource_size_t, SZ_4G - MX8M_DDR_CSD1_BASE_ADDR, size);
> +	size1 = size - size0;
> +
> +	return add_mem(data->base0, size0, SZ_4G, size1, true);

It's quite bogus to call add_mem from the imx8 code here. add_mem
explicitly deals with different chip selects on the same memory
controller and it's whole purpose is to do the opposite of what you are
trying to achieve here: merging multiple regions into a single memory
bank.

Please just call arm_add_mem_device two times from this little helper
function you are adding here.

However, given that we ignore memory beyond the 4G mark in other parts
of barebox as well, wouldn't it make sense to just clamp the memory to
32bit addresses in memory_bank_first_find_space?

Regards,
Lucas

> +}
> +
>  static int imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
>  {
> -	return arm_add_mem_device("ram0", data->base0,
> -			   imx8m_ddrc_sdram_size(mmdcbase, 32));
> +	return _imx8m_ddrc_add_mem(mmdcbase, data, 32);
>  }
>  
>  static int imx8mn_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
>  {
> -	return arm_add_mem_device("ram0", data->base0,
> -			   imx8m_ddrc_sdram_size(mmdcbase, 16));
> +	return _imx8m_ddrc_add_mem(mmdcbase, data, 16);
>  }
>  
>  static resource_size_t imx7d_ddrc_sdram_size(void __iomem *ddrc)