[PATCH] ARM: imx: Fix problem with imx_ddrc_sdram_size
Ahmad Fatoum
a.fatoum at pengutronix.de
Fri Oct 29 05:15:29 PDT 2021
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 at 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 |
More information about the barebox
mailing list