From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: Joacim Zetterling <joaze@westermo.com>, barebox@lists.infradead.org
Subject: Re: [PATCH v2] ARM: imx: Fix problem with imx_ddrc_sdram_size
Date: Sat, 30 Oct 2021 15:45:57 +0200 [thread overview]
Message-ID: <47f1aab9-0594-f35c-f6d3-95a799b65b14@pengutronix.de> (raw)
In-Reply-To: <20211029110600.GA310644@wsevst-c0022>
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
next prev parent reply other threads:[~2021-10-30 13:48 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-29 11:06 Joacim Zetterling
2021-10-30 13:45 ` Ahmad Fatoum [this message]
2021-11-01 10:07 ` Joacim Zetterling
2021-11-01 10:25 ` Ahmad Fatoum
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=47f1aab9-0594-f35c-f6d3-95a799b65b14@pengutronix.de \
--to=a.fatoum@pengutronix.de \
--cc=barebox@lists.infradead.org \
--cc=joaze@westermo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox