[PATCH 1/2] ARM: i.MX: esdctl: add force_split option to add_mem

[PATCH 1/2] ARM: i.MX: esdctl: add force_split option to add_mem

[Marco Felsch]

Currently add_mem() tries to merge the memory banks if they are
continuous. Add a option to avoid this merge which is not always useful
e.g. for devices with memory > 4G.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 arch/arm/mach-imx/esdctl.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
index 1798ad48e50a..54c62c47338e 100644
--- a/arch/arm/mach-imx/esdctl.c
+++ b/arch/arm/mach-imx/esdctl.c
@@ -183,14 +183,14 @@ static inline u64 __imx6_mmdc_sdram_size(void __iomem *mmdcbase, int cs)
 }
 
 static int add_mem(unsigned long base0, unsigned long size0,
-		unsigned long base1, unsigned long size1)
+		   unsigned long base1, unsigned long size1, bool force_split)
 {
 	int ret0 = 0, ret1 = 0;
 
 	debug("%s: cs0 base: 0x%08lx cs0 size: 0x%08lx\n", __func__, base0, size0);
 	debug("%s: cs1 base: 0x%08lx cs1 size: 0x%08lx\n", __func__, base1, size1);
 
-	if (base0 + size0 == base1 && size1 > 0) {
+	if (!force_split && base0 + size0 == base1 && size1 > 0) {
 		/*
 		 * concatenate both chip selects to a single bank
 		 */
@@ -229,13 +229,13 @@ static inline void imx_esdctl_v2_disable_default(void __iomem *esdctlbase)
 static int imx_esdctl_v1_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
 	return add_mem(data->base0, imx_v1_sdram_size(esdctlbase, 0),
-			data->base1, imx_v1_sdram_size(esdctlbase, 1));
+		       data->base1, imx_v1_sdram_size(esdctlbase, 1), false);
 }
 
 static int imx_esdctl_v2_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
 	return add_mem(data->base0, imx_v2_sdram_size(esdctlbase, 0),
-			data->base1, imx_v2_sdram_size(esdctlbase, 1));
+		       data->base1, imx_v2_sdram_size(esdctlbase, 1), false);
 }
 
 static int imx_esdctl_v2_bug_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
@@ -243,19 +243,19 @@ static int imx_esdctl_v2_bug_add_mem(void *esdctlbase, struct imx_esdctl_data *d
 	imx_esdctl_v2_disable_default(esdctlbase);
 
 	return add_mem(data->base0, imx_v2_sdram_size(esdctlbase, 0),
-			data->base1, imx_v2_sdram_size(esdctlbase, 1));
+		       data->base1, imx_v2_sdram_size(esdctlbase, 1), false);
 }
 
 static int imx_esdctl_v3_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
 	return add_mem(data->base0, imx_v3_sdram_size(esdctlbase, 0),
-			data->base1, imx_v3_sdram_size(esdctlbase, 1));
+		       data->base1, imx_v3_sdram_size(esdctlbase, 1), false);
 }
 
 static int imx_esdctl_v4_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
 	return add_mem(data->base0, imx_v4_sdram_size(esdctlbase, 0),
-			data->base1, imx_v4_sdram_size(esdctlbase, 1));
+		       data->base1, imx_v4_sdram_size(esdctlbase, 1), false);
 }
 
 /*
-- 
2.39.2

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

[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);
+}
+
 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)
-- 
2.39.2

Re: [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@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)

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

[Marco Felsch]

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