* [PATCH v2] ARM: imx: Fix problem with imx_ddrc_sdram_size
@ 2021-10-29 11:06 Joacim Zetterling
2021-10-30 13:45 ` Ahmad Fatoum
0 siblings, 1 reply; 4+ messages in thread
From: Joacim Zetterling @ 2021-10-29 11:06 UTC (permalink / raw)
To: barebox; +Cc: a.fatoum
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 |
+----------------------------------------------------+
...
...
The imx8 supports a bus width of 4 bits or x4 (device_config b00).
This is a problem for the calculation of the mem size when it only
handle the bus width in bytes. Therefore we must treat the mem size
calculation for the half bus width (width = 0) in a special way.
Do the calculation with one byte width and then divide the mem size
by 2 later on.
Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
with 2GB LPDDR4, checked size and made memory test. Both platforms
has a mstr_device_config of b10 and the mstr_bus_width of b00.
Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
---
arch/arm/mach-imx/esdctl.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
index e56da3cb76d4..1d76f26dfa39 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();
}
+ /* Bus width in bytes, 0 means half byte or 4-bit mode */
+ 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;
+ case 0b01: /* Half DQ bus */
+ width >>= 1;
break;
case 0b10: /* Quarter DQ bus */
- width = 1;
+ width >>= 2;
break;
default:
BUG();
@@ -412,7 +418,15 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
columns = imx_ddrc_count_bits(col_max, col_b, col_b_num);
rows = imx_ddrc_count_bits(row_max, row_b, row_b_num);
- size = memory_sdram_size(columns, rows, 1 << banks, width) << ranks;
+ /*
+ * Special case when bus width is 0 or x4 mode,
+ * calculate the mem size and then divide the size by 2.
+ */
+ if (width)
+ size = memory_sdram_size(columns, rows, 1 << banks, width);
+ else
+ size = memory_sdram_size(columns, rows, 1 << banks, 1) >> 1;
+ size <<= ranks;
return reduced_adress_space ? size * 3 / 4 : size;
}
@@ -466,7 +480,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 +522,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] 4+ messages in thread
* Re: [PATCH v2] ARM: imx: Fix problem with imx_ddrc_sdram_size
2021-10-29 11:06 [PATCH v2] ARM: imx: Fix problem with imx_ddrc_sdram_size Joacim Zetterling
@ 2021-10-30 13:45 ` Ahmad Fatoum
2021-11-01 10:07 ` Joacim Zetterling
0 siblings, 1 reply; 4+ messages in thread
From: Ahmad Fatoum @ 2021-10-30 13:45 UTC (permalink / raw)
To: Joacim Zetterling, barebox
On 29.10.21 13:06, 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).
>
> 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 |
> +----------------------------------------------------+
> ...
> ...
>
> The imx8 supports a bus width of 4 bits or x4 (device_config b00).
> This is a problem for the calculation of the mem size when it only
> handle the bus width in bytes. Therefore we must treat the mem size
> calculation for the half bus width (width = 0) in a special way.
> Do the calculation with one byte width and then divide the mem size
> by 2 later on.
>
> Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
> with 2GB LPDDR4, checked size and made memory test. Both platforms
> has a mstr_device_config of b10 and the mstr_bus_width of b00.
>
> Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
I agree that the computation for the nano is wrong. I have myself
a board with 1GB LPDDR4 here and with this patch now, it yields
a correct result.
I checked against other boards we have in tree:
arch/arm/boards/zii-imx8mq-dev/ddr_init.c
45: reg32_write(0x3d400000,0xa3080020);
arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c
15: { 0x3d400000, 0x81040010 },
arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c
19: {0x3d400000, 0xa3080020},
arch/arm/boards/protonic-imx8m/lpddr4-timing-prt8mm.c
16: { DDRC_MSTR(0), 0xa1080020 },
arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c
14: { 0x3d400000, 0xa3080020 },
arch/arm/boards/nxp-imx8mq-evk/ddr_init.c
45: reg32_write(0x3d400000,0xa3080020);
arch/arm/boards/phytec-som-imx8mq/ddr_init.c
45: reg32_write(0x3d400000,0xa1080020);
arch/arm/boards/nxp-imx8mm-evk/lpddr4-timing.c
16: { DDRC_MSTR(0), 0xa1080020 },
arch/arm/boards/mnt-reform/lpddr4-timing.c
15: { DDRC_MSTR(0), 0xa0080020 | (LPDDR4_CS << 24) },
And all of them have a width of x16. I find it hard to believe that all
of these boards have so far detected double the actual RAM.
I am wondering if there is another parameter we are missing here.
Cheers,
Ahmad
> ---
> arch/arm/mach-imx/esdctl.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> index e56da3cb76d4..1d76f26dfa39 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();
> }
>
> + /* Bus width in bytes, 0 means half byte or 4-bit mode */
> + 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;
> + case 0b01: /* Half DQ bus */
> + width >>= 1;
> break;
> case 0b10: /* Quarter DQ bus */
> - width = 1;
> + width >>= 2;
> break;
> default:
> BUG();
> @@ -412,7 +418,15 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
> columns = imx_ddrc_count_bits(col_max, col_b, col_b_num);
> rows = imx_ddrc_count_bits(row_max, row_b, row_b_num);
>
> - size = memory_sdram_size(columns, rows, 1 << banks, width) << ranks;
> + /*
> + * Special case when bus width is 0 or x4 mode,
> + * calculate the mem size and then divide the size by 2.
> + */
> + if (width)
> + size = memory_sdram_size(columns, rows, 1 << banks, width);
> + else
> + size = memory_sdram_size(columns, rows, 1 << banks, 1) >> 1;
> + size <<= ranks;
>
> return reduced_adress_space ? size * 3 / 4 : size;
> }
> @@ -466,7 +480,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 +522,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] 4+ messages in thread
* Re: [PATCH v2] ARM: imx: Fix problem with imx_ddrc_sdram_size
2021-10-30 13:45 ` Ahmad Fatoum
@ 2021-11-01 10:07 ` Joacim Zetterling
2021-11-01 10:25 ` Ahmad Fatoum
0 siblings, 1 reply; 4+ messages in thread
From: Joacim Zetterling @ 2021-11-01 10:07 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Sat, Oct 30, 2021 at 03:45:57PM +0200, Ahmad Fatoum wrote:
> On 29.10.21 13:06, 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).
> >
> > 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 |
> > +----------------------------------------------------+
> > ...
> > ...
> >
> > The imx8 supports a bus width of 4 bits or x4 (device_config b00).
> > This is a problem for the calculation of the mem size when it only
> > handle the bus width in bytes. Therefore we must treat the mem size
> > calculation for the half bus width (width = 0) in a special way.
> > Do the calculation with one byte width and then divide the mem size
> > by 2 later on.
> >
> > Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
> > with 2GB LPDDR4, checked size and made memory test. Both platforms
> > has a mstr_device_config of b10 and the mstr_bus_width of b00.
> >
> > Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
>
> I agree that the computation for the nano is wrong. I have myself
> a board with 1GB LPDDR4 here and with this patch now, it yields
> a correct result.
>
> I checked against other boards we have in tree:
>
> arch/arm/boards/zii-imx8mq-dev/ddr_init.c
> 45: reg32_write(0x3d400000,0xa3080020);
>
> arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c
> 15: { 0x3d400000, 0x81040010 },
>
> arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c
> 19: {0x3d400000, 0xa3080020},
>
> arch/arm/boards/protonic-imx8m/lpddr4-timing-prt8mm.c
> 16: { DDRC_MSTR(0), 0xa1080020 },
>
> arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c
> 14: { 0x3d400000, 0xa3080020 },
>
> arch/arm/boards/nxp-imx8mq-evk/ddr_init.c
> 45: reg32_write(0x3d400000,0xa3080020);
>
> arch/arm/boards/phytec-som-imx8mq/ddr_init.c
> 45: reg32_write(0x3d400000,0xa1080020);
>
> arch/arm/boards/nxp-imx8mm-evk/lpddr4-timing.c
> 16: { DDRC_MSTR(0), 0xa1080020 },
>
> arch/arm/boards/mnt-reform/lpddr4-timing.c
> 15: { DDRC_MSTR(0), 0xa0080020 | (LPDDR4_CS << 24) },
>
> And all of them have a width of x16. I find it hard to believe that all
> of these boards have so far detected double the actual RAM.
>
> I am wondering if there is another parameter we are missing here.
>
> Cheers,
> Ahmad
>
You are rigth!
I started to dig into the imx8mn evk DDR4 board and found some things.
The imx8mn DDR4 board has an other mem layout than rest of the boards
compared agains my custom boaard or the LPDDR4 board, I see this.
>From the datasheets:
DDR4 LPDDR4
Banks 4 8
Bank grps 2 1
Rows 17 15
Col 10 10
Therefore we need to add two things:
1. We need the DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 support
to support the DDR4 number of rows.
2. We need two add the DDRC_ADDRMAP8 and do a check for DDRC_ADDRMAP8_BG_B0
DDRC_ADDRMAP8_BG_B1 to get the number of bank groups in the mem chip.
i.e.
...
#define DDRC_ADDRMAP8_BG_B1 GENMASK(13, 8)
#define DDRC_ADDRMAP8_BG_B0 GENMASK( 4, 0)
...
...
if (addrmap[8]) {
if (FIELD_GET(DDRC_ADDRMAP8_BG_B0, addrmap[8]) != 0b11111)
banks++;
if (FIELD_GET(DDRC_ADDRMAP8_BG_B1, addrmap[8]) != 0b111111)
banks++;
}
...
...
I verified this against our custom board (512MB) and the imx8 DDR4 evk board (2GiB)
and seems ok. I also checked the values of the other boards in the repo
and I think this will apply to them as well.
What do You think! shall I make a v3 of this patch with the above changes?
Cheers,
Joacim
> > ---
> > arch/arm/mach-imx/esdctl.c | 30 ++++++++++++++++++++++--------
> > 1 file changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> > index e56da3cb76d4..1d76f26dfa39 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();
> > }
> >
> > + /* Bus width in bytes, 0 means half byte or 4-bit mode */
> > + 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;
> > + case 0b01: /* Half DQ bus */
> > + width >>= 1;
> > break;
> > case 0b10: /* Quarter DQ bus */
> > - width = 1;
> > + width >>= 2;
> > break;
> > default:
> > BUG();
> > @@ -412,7 +418,15 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
> > columns = imx_ddrc_count_bits(col_max, col_b, col_b_num);
> > rows = imx_ddrc_count_bits(row_max, row_b, row_b_num);
> >
> > - size = memory_sdram_size(columns, rows, 1 << banks, width) << ranks;
> > + /*
> > + * Special case when bus width is 0 or x4 mode,
> > + * calculate the mem size and then divide the size by 2.
> > + */
> > + if (width)
> > + size = memory_sdram_size(columns, rows, 1 << banks, width);
> > + else
> > + size = memory_sdram_size(columns, rows, 1 << banks, 1) >> 1;
> > + size <<= ranks;
> >
> > return reduced_adress_space ? size * 3 / 4 : size;
> > }
> > @@ -466,7 +480,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 +522,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 | https://urldefense.com/v3/__http://www.pengutronix.de/__;!!I9LPvj3b!S3iOaFKhB9cEFWi6RANaZPNtXi4DwFoFtNmrEOpNT8BaPsH4hfSSr4ppbB93VGU$ |
> 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] 4+ messages in thread
* Re: [PATCH v2] ARM: imx: Fix problem with imx_ddrc_sdram_size
2021-11-01 10:07 ` Joacim Zetterling
@ 2021-11-01 10:25 ` Ahmad Fatoum
0 siblings, 0 replies; 4+ messages in thread
From: Ahmad Fatoum @ 2021-11-01 10:25 UTC (permalink / raw)
To: Joacim Zetterling; +Cc: barebox
Hello Joacim,
On 01.11.21 11:07, Joacim Zetterling wrote:
> On Sat, Oct 30, 2021 at 03:45:57PM +0200, Ahmad Fatoum wrote:
>> On 29.10.21 13:06, 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).
>>>
>>> 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 |
>>> +----------------------------------------------------+
>>> ...
>>> ...
>>>
>>> The imx8 supports a bus width of 4 bits or x4 (device_config b00).
>>> This is a problem for the calculation of the mem size when it only
>>> handle the bus width in bytes. Therefore we must treat the mem size
>>> calculation for the half bus width (width = 0) in a special way.
>>> Do the calculation with one byte width and then divide the mem size
>>> by 2 later on.
>>>
>>> Tested on the IMX8MN Evk with 2GB DDR4 and on a IMX8MN custom board
>>> with 2GB LPDDR4, checked size and made memory test. Both platforms
>>> has a mstr_device_config of b10 and the mstr_bus_width of b00.
>>>
>>> Signed-off-by: Joacim Zetterling <joacim.zetterling@westermo.com>
>>
>> I agree that the computation for the nano is wrong. I have myself
>> a board with 1GB LPDDR4 here and with this patch now, it yields
>> a correct result.
>>
>> I checked against other boards we have in tree:
>>
>> arch/arm/boards/zii-imx8mq-dev/ddr_init.c
>> 45: reg32_write(0x3d400000,0xa3080020);
>>
>> arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c
>> 15: { 0x3d400000, 0x81040010 },
>>
>> arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c
>> 19: {0x3d400000, 0xa3080020},
>>
>> arch/arm/boards/protonic-imx8m/lpddr4-timing-prt8mm.c
>> 16: { DDRC_MSTR(0), 0xa1080020 },
>>
>> arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c
>> 14: { 0x3d400000, 0xa3080020 },
>>
>> arch/arm/boards/nxp-imx8mq-evk/ddr_init.c
>> 45: reg32_write(0x3d400000,0xa3080020);
>>
>> arch/arm/boards/phytec-som-imx8mq/ddr_init.c
>> 45: reg32_write(0x3d400000,0xa1080020);
>>
>> arch/arm/boards/nxp-imx8mm-evk/lpddr4-timing.c
>> 16: { DDRC_MSTR(0), 0xa1080020 },
>>
>> arch/arm/boards/mnt-reform/lpddr4-timing.c
>> 15: { DDRC_MSTR(0), 0xa0080020 | (LPDDR4_CS << 24) },
>>
>> And all of them have a width of x16. I find it hard to believe that all
>> of these boards have so far detected double the actual RAM.
>>
>> I am wondering if there is another parameter we are missing here.
>>
>> Cheers,
>> Ahmad
>>
>
> You are rigth!
>
> I started to dig into the imx8mn evk DDR4 board and found some things.
>
> The imx8mn DDR4 board has an other mem layout than rest of the boards
> compared agains my custom boaard or the LPDDR4 board, I see this.
>
> From the datasheets:
>
> DDR4 LPDDR4
> Banks 4 8
> Bank grps 2 1
> Rows 17 15
> Col 10 10
>
> Therefore we need to add two things:
>
> 1. We need the DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 support
> to support the DDR4 number of rows.
>
> 2. We need two add the DDRC_ADDRMAP8 and do a check for DDRC_ADDRMAP8_BG_B0
> DDRC_ADDRMAP8_BG_B1 to get the number of bank groups in the mem chip.
> i.e.
> ...
> #define DDRC_ADDRMAP8_BG_B1 GENMASK(13, 8)
> #define DDRC_ADDRMAP8_BG_B0 GENMASK( 4, 0)
> ...
> ...
> if (addrmap[8]) {
> if (FIELD_GET(DDRC_ADDRMAP8_BG_B0, addrmap[8]) != 0b11111)
> banks++;
> if (FIELD_GET(DDRC_ADDRMAP8_BG_B1, addrmap[8]) != 0b111111)
> banks++;
> }
> ...
> ...
>
> I verified this against our custom board (512MB) and the imx8 DDR4 evk board (2GiB)
> and seems ok. I also checked the values of the other boards in the repo
> and I think this will apply to them as well.
Great!
> What do You think! shall I make a v3 of this patch with the above changes?
Yes, please.
Cheers,
Ahmad
>
> Cheers,
> Joacim
>
>>> ---
>>> arch/arm/mach-imx/esdctl.c | 30 ++++++++++++++++++++++--------
>>> 1 file changed, 22 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
>>> index e56da3cb76d4..1d76f26dfa39 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();
>>> }
>>>
>>> + /* Bus width in bytes, 0 means half byte or 4-bit mode */
>>> + 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;
>>> + case 0b01: /* Half DQ bus */
>>> + width >>= 1;
>>> break;
>>> case 0b10: /* Quarter DQ bus */
>>> - width = 1;
>>> + width >>= 2;
>>> break;
>>> default:
>>> BUG();
>>> @@ -412,7 +418,15 @@ imx_ddrc_sdram_size(void __iomem *ddrc, const u32 addrmap[],
>>> columns = imx_ddrc_count_bits(col_max, col_b, col_b_num);
>>> rows = imx_ddrc_count_bits(row_max, row_b, row_b_num);
>>>
>>> - size = memory_sdram_size(columns, rows, 1 << banks, width) << ranks;
>>> + /*
>>> + * Special case when bus width is 0 or x4 mode,
>>> + * calculate the mem size and then divide the size by 2.
>>> + */
>>> + if (width)
>>> + size = memory_sdram_size(columns, rows, 1 << banks, width);
>>> + else
>>> + size = memory_sdram_size(columns, rows, 1 << banks, 1) >> 1;
>>> + size <<= ranks;
>>>
>>> return reduced_adress_space ? size * 3 / 4 : size;
>>> }
>>> @@ -466,7 +480,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 +522,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 | https://urldefense.com/v3/__http://www.pengutronix.de/__;!!I9LPvj3b!S3iOaFKhB9cEFWi6RANaZPNtXi4DwFoFtNmrEOpNT8BaPsH4hfSSr4ppbB93VGU$ |
>> 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] 4+ messages in thread
end of thread, other threads:[~2021-11-01 10:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-29 11:06 [PATCH v2] ARM: imx: Fix problem with imx_ddrc_sdram_size Joacim Zetterling
2021-10-30 13:45 ` Ahmad Fatoum
2021-11-01 10:07 ` Joacim Zetterling
2021-11-01 10:25 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox