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

[Marco Felsch <m.felsch@pengutronix.de>]

On 23-08-31, Lucas Stach wrote:
> 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@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.

okay.

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

This is something I discussed with Ahmad as well, but I'm not sure how
this will interfere with platforms having memory space beyond 4G only.

As you said there are issues when it comes to platforms which do use
memory above 4G for other purposes than memory. But I would keep the
current behaviour of memory_bank_first_find_space() to not make it even
harder for them.
Also at least the imx8mp-evk lists two memory banks within the dts, so
splitting it into two banks here is more aligned with the dts.

Regards,
  Marco

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