* [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