mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size
@ 2021-10-28  8:46 Joacim Zetterling
  2021-10-28 12:19 ` Ahmad Fatoum
  0 siblings, 1 reply; 5+ messages in thread
From: Joacim Zetterling @ 2021-10-28  8:46 UTC (permalink / raw)
  To: barebox

The imx8mn has a 16-bit SDRAM bus width access but the calculation
of the memory size treat it as a 32-bit width bus which makes the
memory calculation to be wrong (meminfo wrong and memtest fails).

There is a difference between the imx7 and the imx8 familys.
The imx8 family has a device config field in the master register of
the DDRC controller which the imx7 family doesn't have (the bus width
is 32-bit as default).

The device config field together with the DQ configuration tells us
the actual bus width of the device for a correct mem size calculaton.

>From the imx8mn reference manual:
+----------------------------------------------------+
|    Field      |     Function                       |
|----------------------------------------------------|
|    31-30      | Indicates the configuration of the |
|               | device used in the system.         |
| device_config |     00b - x4 device                |
|               |     01b - x8 device                |
|               |     10b - x16 device               |
|               |     11b - x32 device               |
+----------------------------------------------------+
...
...

Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
with 2GB LPDDR4, checked size and made memory test.

Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
---
 arch/arm/mach-imx/esdctl.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
index e56da3cb76d4..f80d94f2fca0 100644
--- a/arch/arm/mach-imx/esdctl.c
+++ b/arch/arm/mach-imx/esdctl.c
@@ -320,6 +320,7 @@ static int vf610_ddrmc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
 #define DDRC_MSTR_LPDDR4			BIT(5)
 #define DDRC_MSTR_DATA_BUS_WIDTH		GENMASK(13, 12)
 #define DDRC_MSTR_ACTIVE_RANKS			GENMASK(27, 24)
+#define DDRC_MSTR_DEVICE_CONFIG		GENMASK(31, 30)
 
 #define DDRC_ADDRMAP0_CS_BIT1			GENMASK(12,  8)
 
@@ -361,7 +362,7 @@ static resource_size_t
 imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
 		    u8 col_max, const u8 col_b[], unsigned int col_b_num,
 		    u8 row_max, const u8 row_b[], unsigned int row_b_num,
-		    bool reduced_adress_space)
+		    bool reduced_adress_space, bool is_imx8)
 {
 	const u32 mstr = readl(ddrc + DDRC_MSTR);
 	unsigned int banks, ranks, columns, rows, active_ranks, width;
@@ -384,15 +385,20 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
 		BUG();
 	}
 
+	if (is_imx8)
+		width = (1 << FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr)) >> 1;
+	else
+		width = 4;
+
 	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
 	case 0b00:	/* Full DQ bus  */
 		width = 4;
 		break;
 	case 0b01:      /* Half DQ bus  */
-		width = 2;
+		width >>= 1;
 		break;
 	case 0b10:	/* Quarter DQ bus  */
-		width = 1;
+		width >>= 2;
 		break;
 	default:
 		BUG();
@@ -466,7 +472,7 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem *ddrc)
 	return imx_ddrc_sdram_size(ddrc, addrmap,
 				   12, ARRAY_AND_SIZE(col_b),
 				   16, ARRAY_AND_SIZE(row_b),
-				   reduced_adress_space);
+				   reduced_adress_space, true);
 }
 
 static int imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
@@ -508,7 +514,7 @@ static resource_size_t imx7d_ddrc_sdram_size(void __iomem *ddrc)
 	return imx_ddrc_sdram_size(ddrc, addrmap,
 				   11, ARRAY_AND_SIZE(col_b),
 				   15, ARRAY_AND_SIZE(row_b),
-				   reduced_adress_space);
+				   reduced_adress_space, false);
 }
 
 static int imx7d_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size
  2021-10-28  8:46 [PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size Joacim Zetterling
@ 2021-10-28 12:19 ` Ahmad Fatoum
  2021-10-28 13:06   ` Joacim Zetterling
  0 siblings, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2021-10-28 12:19 UTC (permalink / raw)
  To: Joacim Zetterling, barebox

Hello Joacim,

On 28.10.21 10:46, Joacim Zetterling wrote:
> The imx8mn has a 16-bit SDRAM bus width access but the calculation
> of the memory size treat it as a 32-bit width bus which makes the
> memory calculation to be wrong (meminfo wrong and memtest fails).

I missed this because of the /memory node in the device tree filling
up the remainder. I removed /memory and tried your patch on an
i.MX8MN DDR EVK, but it doesn't change iomem output:

  0x0000000040000000 - 0x000000007fffffff (size 0x0000000040000000) ram0

The 8MN DDR4 EVK reports FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr) == 0b10
and FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr) == 0b00.

> There is a difference between the imx7 and the imx8 familys.
> The imx8 family has a device config field in the master register of
> the DDRC controller which the imx7 family doesn't have (the bus width
> is 32-bit as default).
> 
> The device config field together with the DQ configuration tells us
> the actual bus width of the device for a correct mem size calculaton.
> 
> From the imx8mn reference manual:
> +----------------------------------------------------+
> |    Field      |     Function                       |
> |----------------------------------------------------|
> |    31-30      | Indicates the configuration of the |
> |               | device used in the system.         |
> | device_config |     00b - x4 device                |
> |               |     01b - x8 device                |
> |               |     10b - x16 device               |
> |               |     11b - x32 device               |
> +----------------------------------------------------+
> ...
> ...
> 
> Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
> with 2GB LPDDR4, checked size and made memory test.

What's the device_config for each?

> Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
> ---
>  arch/arm/mach-imx/esdctl.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> index e56da3cb76d4..f80d94f2fca0 100644
> --- a/arch/arm/mach-imx/esdctl.c
> +++ b/arch/arm/mach-imx/esdctl.c
> @@ -320,6 +320,7 @@ static int vf610_ddrmc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
>  #define DDRC_MSTR_LPDDR4			BIT(5)
>  #define DDRC_MSTR_DATA_BUS_WIDTH		GENMASK(13, 12)
>  #define DDRC_MSTR_ACTIVE_RANKS			GENMASK(27, 24)
> +#define DDRC_MSTR_DEVICE_CONFIG		GENMASK(31, 30)
>  
>  #define DDRC_ADDRMAP0_CS_BIT1			GENMASK(12,  8)
>  
> @@ -361,7 +362,7 @@ static resource_size_t
>  imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
>  		    u8 col_max, const u8 col_b[], unsigned int col_b_num,
>  		    u8 row_max, const u8 row_b[], unsigned int row_b_num,
> -		    bool reduced_adress_space)
> +		    bool reduced_adress_space, bool is_imx8)
>  {
>  	const u32 mstr = readl(ddrc + DDRC_MSTR);
>  	unsigned int banks, ranks, columns, rows, active_ranks, width;
> @@ -384,15 +385,20 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
>  		BUG();
>  	}
>  
> +	if (is_imx8)
> +		width = (1 << FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr)) >> 1;

for device_config == 0, size should be halved, but instead you would get a
width of zero here.

> +	else
> +		width = 4;
> +
>  	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
>  	case 0b00:	/* Full DQ bus  */
>  		width = 4;

I can't really follow. Shouldn't this be dropped to take the
value of width above?

>  		break;
>  	case 0b01:      /* Half DQ bus  */
> -		width = 2;
> +		width >>= 1;
>  		break;
>  	case 0b10:	/* Quarter DQ bus  */
> -		width = 1;
> +		width >>= 2;
>  		break;
>  	default:
>  		BUG();
> @@ -466,7 +472,7 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem *ddrc)
>  	return imx_ddrc_sdram_size(ddrc, addrmap,
>  				   12, ARRAY_AND_SIZE(col_b),
>  				   16, ARRAY_AND_SIZE(row_b),
> -				   reduced_adress_space);
> +				   reduced_adress_space, true);
>  }
>  
>  static int imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> @@ -508,7 +514,7 @@ static resource_size_t imx7d_ddrc_sdram_size(void __iomem *ddrc)
>  	return imx_ddrc_sdram_size(ddrc, addrmap,
>  				   11, ARRAY_AND_SIZE(col_b),
>  				   15, ARRAY_AND_SIZE(row_b),
> -				   reduced_adress_space);
> +				   reduced_adress_space, false);
>  }
>  
>  static int imx7d_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size
  2021-10-28 12:19 ` Ahmad Fatoum
@ 2021-10-28 13:06   ` Joacim Zetterling
  2021-10-29 12:15     ` Ahmad Fatoum
  0 siblings, 1 reply; 5+ messages in thread
From: Joacim Zetterling @ 2021-10-28 13:06 UTC (permalink / raw)
  To: barebox; +Cc: a.fatoum

On Thu, Oct 28, 2021 at 02:19:18PM +0200, Ahmad Fatoum wrote:
> Hello Joacim,
> 
> On 28.10.21 10:46, Joacim Zetterling wrote:
> > The imx8mn has a 16-bit SDRAM bus width access but the calculation
> > of the memory size treat it as a 32-bit width bus which makes the
> > memory calculation to be wrong (meminfo wrong and memtest fails).
> 
> I missed this because of the /memory node in the device tree filling
> up the remainder. I removed /memory and tried your patch on an
> i.MX8MN DDR EVK, but it doesn't change iomem output:
> 
>   0x0000000040000000 - 0x000000007fffffff (size 0x0000000040000000) ram0
> 
> The 8MN DDR4 EVK reports FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr) == 0b10
> and FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr) == 0b00.
> 
> > There is a difference between the imx7 and the imx8 familys.
> > The imx8 family has a device config field in the master register of
> > the DDRC controller which the imx7 family doesn't have (the bus width
> > is 32-bit as default).
> > 
> > The device config field together with the DQ configuration tells us
> > the actual bus width of the device for a correct mem size calculaton.
> > 
> > From the imx8mn reference manual:
> > +----------------------------------------------------+
> > |    Field      |     Function                       |
> > |----------------------------------------------------|
> > |    31-30      | Indicates the configuration of the |
> > |               | device used in the system.         |
> > | device_config |     00b - x4 device                |
> > |               |     01b - x8 device                |
> > |               |     10b - x16 device               |
> > |               |     11b - x32 device               |
> > +----------------------------------------------------+
> > ...
> > ...
> > 
> > Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
> > with 2GB LPDDR4, checked size and made memory test.
> 
> What's the device_config for each?
The device_config for both platforms are b10

> 
> > Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
> > ---
> >  arch/arm/mach-imx/esdctl.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> > index e56da3cb76d4..f80d94f2fca0 100644
> > --- a/arch/arm/mach-imx/esdctl.c
> > +++ b/arch/arm/mach-imx/esdctl.c
> > @@ -320,6 +320,7 @@ static int vf610_ddrmc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> >  #define DDRC_MSTR_LPDDR4			BIT(5)
> >  #define DDRC_MSTR_DATA_BUS_WIDTH		GENMASK(13, 12)
> >  #define DDRC_MSTR_ACTIVE_RANKS			GENMASK(27, 24)
> > +#define DDRC_MSTR_DEVICE_CONFIG		GENMASK(31, 30)
> >  
> >  #define DDRC_ADDRMAP0_CS_BIT1			GENMASK(12,  8)
> >  
> > @@ -361,7 +362,7 @@ static resource_size_t
> >  imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
> >  		    u8 col_max, const u8 col_b[], unsigned int col_b_num,
> >  		    u8 row_max, const u8 row_b[], unsigned int row_b_num,
> > -		    bool reduced_adress_space)
> > +		    bool reduced_adress_space, bool is_imx8)
> >  {
> >  	const u32 mstr = readl(ddrc + DDRC_MSTR);
> >  	unsigned int banks, ranks, columns, rows, active_ranks, width;
> > @@ -384,15 +385,20 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
> >  		BUG();
> >  	}
> >  
> > +	if (is_imx8)
> > +		width = (1 << FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr)) >> 1;
> 
> for device_config == 0, size should be halved, but instead you would get a
> width of zero here.
Good catch! Need to treat bus width 4 in a special way when calculating
the memory size. i.e.

	if (width)
		size = memory_sdram_size(columns, rows, 1 << banks, width);
	else
		size = memory_sdram_size(columns, rows, 1 << banks, 1) >> 1;
	size <<= ranks;

> 
> > +	else
> > +		width = 4;
> > +
> >  	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
> >  	case 0b00:	/* Full DQ bus  */
> >  		width = 4;
> 
> I can't really follow. Shouldn't this be dropped to take the
> value of width above?
My bad! Should have been removed earlier.

	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
	case 0b00:	/* Full DQ bus  */
		break;
	case 0b01:	/* Half DQ bus  */
        ...
	...
> 
> >  		break;
> >  	case 0b01:      /* Half DQ bus  */
> > -		width = 2;
> > +		width >>= 1;
> >  		break;
> >  	case 0b10:	/* Quarter DQ bus  */
> > -		width = 1;
> > +		width >>= 2;
> >  		break;
> >  	default:
> >  		BUG();
> > @@ -466,7 +472,7 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem *ddrc)
> >  	return imx_ddrc_sdram_size(ddrc, addrmap,
> >  				   12, ARRAY_AND_SIZE(col_b),
> >  				   16, ARRAY_AND_SIZE(row_b),
> > -				   reduced_adress_space);
> > +				   reduced_adress_space, true);
> >  }
> >  
> >  static int imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> > @@ -508,7 +514,7 @@ static resource_size_t imx7d_ddrc_sdram_size(void __iomem *ddrc)
> >  	return imx_ddrc_sdram_size(ddrc, addrmap,
> >  				   11, ARRAY_AND_SIZE(col_b),
> >  				   15, ARRAY_AND_SIZE(row_b),
> > -				   reduced_adress_space);
> > +				   reduced_adress_space, false);
> >  }
> >  
> >  static int imx7d_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> > 
> 
> 
I'll send a new patch version.
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | https://urldefense.com/v3/__http://www.pengutronix.de/__;!!I9LPvj3b!UEEry3YzdrJRIMGgQgDd_crPHhM_0abIlHJq7-FIV6hTs9m7uhtHdAcHsA99yIw$   |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size
  2021-10-28 13:06   ` Joacim Zetterling
@ 2021-10-29 12:15     ` Ahmad Fatoum
  2021-10-29 13:41       ` Joacim Zetterling
  0 siblings, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2021-10-29 12:15 UTC (permalink / raw)
  To: Joacim Zetterling, barebox

Hi,

On 28.10.21 15:06, Joacim Zetterling wrote:
> On Thu, Oct 28, 2021 at 02:19:18PM +0200, Ahmad Fatoum wrote:
>> Hello Joacim,
>>
>> On 28.10.21 10:46, Joacim Zetterling wrote:
>>> The imx8mn has a 16-bit SDRAM bus width access but the calculation
>>> of the memory size treat it as a 32-bit width bus which makes the
>>> memory calculation to be wrong (meminfo wrong and memtest fails).
>>
>> I missed this because of the /memory node in the device tree filling
>> up the remainder. I removed /memory and tried your patch on an
>> i.MX8MN DDR EVK, but it doesn't change iomem output:
>>
>>   0x0000000040000000 - 0x000000007fffffff (size 0x0000000040000000) ram0
>>
>> The 8MN DDR4 EVK reports FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr) == 0b10
>> and FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr) == 0b00.
>>
>>> There is a difference between the imx7 and the imx8 familys.
>>> The imx8 family has a device config field in the master register of
>>> the DDRC controller which the imx7 family doesn't have (the bus width
>>> is 32-bit as default).
>>>
>>> The device config field together with the DQ configuration tells us
>>> the actual bus width of the device for a correct mem size calculaton.
>>>
>>> From the imx8mn reference manual:
>>> +----------------------------------------------------+
>>> |    Field      |     Function                       |
>>> |----------------------------------------------------|
>>> |    31-30      | Indicates the configuration of the |
>>> |               | device used in the system.         |
>>> | device_config |     00b - x4 device                |
>>> |               |     01b - x8 device                |
>>> |               |     10b - x16 device               |
>>> |               |     11b - x32 device               |
>>> +----------------------------------------------------+
>>> ...
>>> ...
>>>
>>> Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
>>> with 2GB LPDDR4, checked size and made memory test.
>>
>> What's the device_config for each?
> The device_config for both platforms are b10
> 
>>
>>> Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
>>> ---
>>>  arch/arm/mach-imx/esdctl.c | 16 +++++++++++-----
>>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
>>> index e56da3cb76d4..f80d94f2fca0 100644
>>> --- a/arch/arm/mach-imx/esdctl.c
>>> +++ b/arch/arm/mach-imx/esdctl.c
>>> @@ -320,6 +320,7 @@ static int vf610_ddrmc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
>>>  #define DDRC_MSTR_LPDDR4			BIT(5)
>>>  #define DDRC_MSTR_DATA_BUS_WIDTH		GENMASK(13, 12)
>>>  #define DDRC_MSTR_ACTIVE_RANKS			GENMASK(27, 24)
>>> +#define DDRC_MSTR_DEVICE_CONFIG		GENMASK(31, 30)
>>>  
>>>  #define DDRC_ADDRMAP0_CS_BIT1			GENMASK(12,  8)
>>>  
>>> @@ -361,7 +362,7 @@ static resource_size_t
>>>  imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
>>>  		    u8 col_max, const u8 col_b[], unsigned int col_b_num,
>>>  		    u8 row_max, const u8 row_b[], unsigned int row_b_num,
>>> -		    bool reduced_adress_space)
>>> +		    bool reduced_adress_space, bool is_imx8)
>>>  {
>>>  	const u32 mstr = readl(ddrc + DDRC_MSTR);
>>>  	unsigned int banks, ranks, columns, rows, active_ranks, width;
>>> @@ -384,15 +385,20 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
>>>  		BUG();
>>>  	}
>>>  
>>> +	if (is_imx8)
>>> +		width = (1 << FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr)) >> 1;
>>
>> for device_config == 0, size should be halved, but instead you would get a
>> width of zero here.
> Good catch! Need to treat bus width 4 in a special way when calculating
> the memory size. i.e.
> 
> 	if (width)
> 		size = memory_sdram_size(columns, rows, 1 << banks, width);
> 	else
> 		size = memory_sdram_size(columns, rows, 1 << banks, 1) >> 1;
> 	size <<= ranks;
> 
>>
>>> +	else
>>> +		width = 4;
>>> +
>>>  	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
>>>  	case 0b00:	/* Full DQ bus  */
>>>  		width = 4;
>>
>> I can't really follow. Shouldn't this be dropped to take the
>> value of width above?
> My bad! Should have been removed earlier.

Now I wonder, how could v1 of this patch have worked for you?
You should've had double the really existing RAM or am I missing something?

> 
> 	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
> 	case 0b00:	/* Full DQ bus  */
> 		break;
> 	case 0b01:	/* Half DQ bus  */
>         ...
> 	...
>>
>>>  		break;
>>>  	case 0b01:      /* Half DQ bus  */
>>> -		width = 2;
>>> +		width >>= 1;
>>>  		break;
>>>  	case 0b10:	/* Quarter DQ bus  */
>>> -		width = 1;
>>> +		width >>= 2;
>>>  		break;
>>>  	default:
>>>  		BUG();
>>> @@ -466,7 +472,7 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem *ddrc)
>>>  	return imx_ddrc_sdram_size(ddrc, addrmap,
>>>  				   12, ARRAY_AND_SIZE(col_b),
>>>  				   16, ARRAY_AND_SIZE(row_b),
>>> -				   reduced_adress_space);
>>> +				   reduced_adress_space, true);
>>>  }
>>>  
>>>  static int imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
>>> @@ -508,7 +514,7 @@ static resource_size_t imx7d_ddrc_sdram_size(void __iomem *ddrc)
>>>  	return imx_ddrc_sdram_size(ddrc, addrmap,
>>>  				   11, ARRAY_AND_SIZE(col_b),
>>>  				   15, ARRAY_AND_SIZE(row_b),
>>> -				   reduced_adress_space);
>>> +				   reduced_adress_space, false);
>>>  }
>>>  
>>>  static int imx7d_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
>>>
>>
>>
> I'll send a new patch version.
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | https://urldefense.com/v3/__http://www.pengutronix.de/__;!!I9LPvj3b!UEEry3YzdrJRIMGgQgDd_crPHhM_0abIlHJq7-FIV6hTs9m7uhtHdAcHsA99yIw$   |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size
  2021-10-29 12:15     ` Ahmad Fatoum
@ 2021-10-29 13:41       ` Joacim Zetterling
  0 siblings, 0 replies; 5+ messages in thread
From: Joacim Zetterling @ 2021-10-29 13:41 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Fri, Oct 29, 2021 at 02:15:29PM +0200, Ahmad Fatoum wrote:
> Hi,
> 
> On 28.10.21 15:06, Joacim Zetterling wrote:
> > On Thu, Oct 28, 2021 at 02:19:18PM +0200, Ahmad Fatoum wrote:
> >> Hello Joacim,
> >>
> >> On 28.10.21 10:46, Joacim Zetterling wrote:
> >>> The imx8mn has a 16-bit SDRAM bus width access but the calculation
> >>> of the memory size treat it as a 32-bit width bus which makes the
> >>> memory calculation to be wrong (meminfo wrong and memtest fails).
> >>
> >> I missed this because of the /memory node in the device tree filling
> >> up the remainder. I removed /memory and tried your patch on an
> >> i.MX8MN DDR EVK, but it doesn't change iomem output:
> >>
> >>   0x0000000040000000 - 0x000000007fffffff (size 0x0000000040000000) ram0
> >>
> >> The 8MN DDR4 EVK reports FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr) == 0b10
> >> and FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr) == 0b00.
> >>
> >>> There is a difference between the imx7 and the imx8 familys.
> >>> The imx8 family has a device config field in the master register of
> >>> the DDRC controller which the imx7 family doesn't have (the bus width
> >>> is 32-bit as default).
> >>>
> >>> The device config field together with the DQ configuration tells us
> >>> the actual bus width of the device for a correct mem size calculaton.
> >>>
> >>> From the imx8mn reference manual:
> >>> +----------------------------------------------------+
> >>> |    Field      |     Function                       |
> >>> |----------------------------------------------------|
> >>> |    31-30      | Indicates the configuration of the |
> >>> |               | device used in the system.         |
> >>> | device_config |     00b - x4 device                |
> >>> |               |     01b - x8 device                |
> >>> |               |     10b - x16 device               |
> >>> |               |     11b - x32 device               |
> >>> +----------------------------------------------------+
> >>> ...
> >>> ...
> >>>
> >>> Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
> >>> with 2GB LPDDR4, checked size and made memory test.
> >>
> >> What's the device_config for each?
> > The device_config for both platforms are b10
> > 
> >>
> >>> Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
> >>> ---
> >>>  arch/arm/mach-imx/esdctl.c | 16 +++++++++++-----
> >>>  1 file changed, 11 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> >>> index e56da3cb76d4..f80d94f2fca0 100644
> >>> --- a/arch/arm/mach-imx/esdctl.c
> >>> +++ b/arch/arm/mach-imx/esdctl.c
> >>> @@ -320,6 +320,7 @@ static int vf610_ddrmc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> >>>  #define DDRC_MSTR_LPDDR4			BIT(5)
> >>>  #define DDRC_MSTR_DATA_BUS_WIDTH		GENMASK(13, 12)
> >>>  #define DDRC_MSTR_ACTIVE_RANKS			GENMASK(27, 24)
> >>> +#define DDRC_MSTR_DEVICE_CONFIG		GENMASK(31, 30)
> >>>  
> >>>  #define DDRC_ADDRMAP0_CS_BIT1			GENMASK(12,  8)
> >>>  
> >>> @@ -361,7 +362,7 @@ static resource_size_t
> >>>  imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
> >>>  		    u8 col_max, const u8 col_b[], unsigned int col_b_num,
> >>>  		    u8 row_max, const u8 row_b[], unsigned int row_b_num,
> >>> -		    bool reduced_adress_space)
> >>> +		    bool reduced_adress_space, bool is_imx8)
> >>>  {
> >>>  	const u32 mstr = readl(ddrc + DDRC_MSTR);
> >>>  	unsigned int banks, ranks, columns, rows, active_ranks, width;
> >>> @@ -384,15 +385,20 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
> >>>  		BUG();
> >>>  	}
> >>>  
> >>> +	if (is_imx8)
> >>> +		width = (1 << FIELD_GET(DDRC_MSTR_DEVICE_CONFIG, mstr)) >> 1;
> >>
> >> for device_config == 0, size should be halved, but instead you would get a
> >> width of zero here.
> > Good catch! Need to treat bus width 4 in a special way when calculating
> > the memory size. i.e.
> > 
> > 	if (width)
> > 		size = memory_sdram_size(columns, rows, 1 << banks, width);
> > 	else
> > 		size = memory_sdram_size(columns, rows, 1 << banks, 1) >> 1;
> > 	size <<= ranks;
> > 
> >>
> >>> +	else
> >>> +		width = 4;
> >>> +
> >>>  	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
> >>>  	case 0b00:	/* Full DQ bus  */
> >>>  		width = 4;
> >>
> >> I can't really follow. Shouldn't this be dropped to take the
> >> value of width above?
> > My bad! Should have been removed earlier.
> 
> Now I wonder, how could v1 of this patch have worked for you?
> You should've had double the really existing RAM or am I missing something?
There was a mistake from my side when I prepared the patch where the
width was mistakely set back to 4. Before the width was set to 2, I got
as You say the double mem size and the memtest command failed.

All this is corrected in v2.
> 
> > 
> > 	switch (FIELD_GET(DDRC_MSTR_DATA_BUS_WIDTH, mstr)) {
> > 	case 0b00:	/* Full DQ bus  */
> > 		break;
> > 	case 0b01:	/* Half DQ bus  */
> >         ...
> > 	...
> >>
> >>>  		break;
> >>>  	case 0b01:      /* Half DQ bus  */
> >>> -		width = 2;
> >>> +		width >>= 1;
> >>>  		break;
> >>>  	case 0b10:	/* Quarter DQ bus  */
> >>> -		width = 1;
> >>> +		width >>= 2;
> >>>  		break;
> >>>  	default:
> >>>  		BUG();
> >>> @@ -466,7 +472,7 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem *ddrc)
> >>>  	return imx_ddrc_sdram_size(ddrc, addrmap,
> >>>  				   12, ARRAY_AND_SIZE(col_b),
> >>>  				   16, ARRAY_AND_SIZE(row_b),
> >>> -				   reduced_adress_space);
> >>> +				   reduced_adress_space, true);
> >>>  }
> >>>  
> >>>  static int imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> >>> @@ -508,7 +514,7 @@ static resource_size_t imx7d_ddrc_sdram_size(void __iomem *ddrc)
> >>>  	return imx_ddrc_sdram_size(ddrc, addrmap,
> >>>  				   11, ARRAY_AND_SIZE(col_b),
> >>>  				   15, ARRAY_AND_SIZE(row_b),
> >>> -				   reduced_adress_space);
> >>> +				   reduced_adress_space, false);
> >>>  }
> >>>  
> >>>  static int imx7d_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
> >>>
> >>
> >>
> > I'll send a new patch version.
> >> -- 
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | https://urldefense.com/v3/__http://www.pengutronix.de/__;!!I9LPvj3b!UEEry3YzdrJRIMGgQgDd_crPHhM_0abIlHJq7-FIV6hTs9m7uhtHdAcHsA99yIw$   |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> > 
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | https://urldefense.com/v3/__http://www.pengutronix.de/__;!!I9LPvj3b!TjpjVc01M7G9ikkRLRYrBUaGfra3FdKn89fwomU_lqiRKADVGxObbSWmFlqXHdE$   |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-10-29 13:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-28  8:46 [PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size Joacim Zetterling
2021-10-28 12:19 ` Ahmad Fatoum
2021-10-28 13:06   ` Joacim Zetterling
2021-10-29 12:15     ` Ahmad Fatoum
2021-10-29 13:41       ` Joacim Zetterling

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox