[PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Teresa Remmet]

As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
now for the row size calculation we need to increase row_max to 18.

For LPDDR4 this only works in combination with ram timings
created with recent configuration spreadsheet versions.
With older versions the register DDRC_ADDRMAP7 may not be set and
calculation will lead to wrong results even with this patch.

Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM row size handle")
Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
---
 arch/arm/mach-imx/esdctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
index 8dd0ddbbc965..b070ebc62a45 100644
--- a/arch/arm/mach-imx/esdctl.c
+++ b/arch/arm/mach-imx/esdctl.c
@@ -488,7 +488,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),
+				   18, ARRAY_AND_SIZE(row_b),
 				   reduced_adress_space, true);
 }
 
-- 
2.25.1


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

Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Sascha Hauer]

On Fri, May 20, 2022 at 04:23:37PM +0200, Teresa Remmet wrote:
> As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
> now for the row size calculation we need to increase row_max to 18.
> 
> For LPDDR4 this only works in combination with ram timings
> created with recent configuration spreadsheet versions.
> With older versions the register DDRC_ADDRMAP7 may not be set and
> calculation will lead to wrong results even with this patch.
> 
> Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM row size handle")
> Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> ---
>  arch/arm/mach-imx/esdctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks

Sascha

> 
> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> index 8dd0ddbbc965..b070ebc62a45 100644
> --- a/arch/arm/mach-imx/esdctl.c
> +++ b/arch/arm/mach-imx/esdctl.c
> @@ -488,7 +488,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),
> +				   18, ARRAY_AND_SIZE(row_b),
>  				   reduced_adress_space, true);
>  }
>  
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

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

Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Ahmad Fatoum]

Hello,

On 20.05.22 16:23, Teresa Remmet wrote:
> As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
> now for the row size calculation we need to increase row_max to 18.
> 
> For LPDDR4 this only works in combination with ram timings
> created with recent configuration spreadsheet versions.
> With older versions the register DDRC_ADDRMAP7 may not be set and
> calculation will lead to wrong results even with this patch.
> 
> Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM row size handle")
> Signed-off-by: Teresa Remmet <t.remmet@phytec.de>

I have an out-of-tree i.MX8MM board with LPDDR4 that reported a correct size of 1G
prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not explicitly initialized,
but it's being read as zero, so the calculation seems to still be broken..

> ---
>  arch/arm/mach-imx/esdctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> index 8dd0ddbbc965..b070ebc62a45 100644
> --- a/arch/arm/mach-imx/esdctl.c
> +++ b/arch/arm/mach-imx/esdctl.c
> @@ -488,7 +488,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),
> +				   18, ARRAY_AND_SIZE(row_b),
>  				   reduced_adress_space, true);
>  }
>  


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

Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Teresa Remmet]

Hello Ahmad,

Am Montag, dem 20.06.2022 um 14:02 +0200 schrieb Ahmad Fatoum:
> Hello,
> 
> On 20.05.22 16:23, Teresa Remmet wrote:
> > As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
> > now for the row size calculation we need to increase row_max to 18.
> > 
> > For LPDDR4 this only works in combination with ram timings
> > created with recent configuration spreadsheet versions.
> > With older versions the register DDRC_ADDRMAP7 may not be set and
> > calculation will lead to wrong results even with this patch.
> > 
> > Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM
> > row size handle")
> > Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> 
> I have an out-of-tree i.MX8MM board with LPDDR4 that reported a
> correct size of 1G
> prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not
> explicitly initialized,
> but it's being read as zero, so the calculation seems to still be
> broken..

yes, this is why I added the note to the commit message. The RAM
timings of the board you are using have been created with a old version
of the spreadsheet ( < version 19 for i.MX8MM).
This issue is worked around when this patch is reverted as
DDRC_ADDRMAP7 is not taken into account. But calculating big ram sizes
will probably not work then.

I have set the DDRC_ADDRMAP7 register manually in the RAM configuration
in such a case. As I don't see a solution that fits for all. But would
be happy for one. :)

Regards,
Teresa

> 
> > ---
> >  arch/arm/mach-imx/esdctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
> > imx/esdctl.c
> > index 8dd0ddbbc965..b070ebc62a45 100644
> > --- a/arch/arm/mach-imx/esdctl.c
> > +++ b/arch/arm/mach-imx/esdctl.c
> > @@ -488,7 +488,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),
> > +				   18, ARRAY_AND_SIZE(row_b),
> >  				   reduced_adress_space, true);
> >  }
> >  
> 
> 
-- 
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany

Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855

Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Ahmad Fatoum]

Hello Teresa,

On 20.06.22 14:27, Teresa Remmet wrote:
> Hello Ahmad,
> 
> Am Montag, dem 20.06.2022 um 14:02 +0200 schrieb Ahmad Fatoum:
>> Hello,
>>
>> On 20.05.22 16:23, Teresa Remmet wrote:
>>> As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
>>> now for the row size calculation we need to increase row_max to 18.
>>>
>>> For LPDDR4 this only works in combination with ram timings
>>> created with recent configuration spreadsheet versions.
>>> With older versions the register DDRC_ADDRMAP7 may not be set and
>>> calculation will lead to wrong results even with this patch.
>>>
>>> Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM
>>> row size handle")
>>> Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
>>
>> I have an out-of-tree i.MX8MM board with LPDDR4 that reported a
>> correct size of 1G
>> prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not
>> explicitly initialized,
>> but it's being read as zero, so the calculation seems to still be
>> broken..
> 
> yes, this is why I added the note to the commit message. The RAM
> timings of the board you are using have been created with a old version
> of the spreadsheet ( < version 19 for i.MX8MM).
> This issue is worked around when this patch is reverted as
> DDRC_ADDRMAP7 is not taken into account. But calculating big ram sizes
> will probably not work then.
> 
> I have set the DDRC_ADDRMAP7 register manually in the RAM configuration
> in such a case. As I don't see a solution that fits for all. But would
> be happy for one. :)

What would the 'neutral' value to write into this register be? zero
seems to not be it.

Thanks,
Ahmad

> 
> Regards,
> Teresa
> 
>>
>>> ---
>>>  arch/arm/mach-imx/esdctl.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
>>> imx/esdctl.c
>>> index 8dd0ddbbc965..b070ebc62a45 100644
>>> --- a/arch/arm/mach-imx/esdctl.c
>>> +++ b/arch/arm/mach-imx/esdctl.c
>>> @@ -488,7 +488,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),
>>> +				   18, ARRAY_AND_SIZE(row_b),
>>>  				   reduced_adress_space, true);
>>>  }
>>>  
>>
>>


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

Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Teresa Remmet]

Am Montag, dem 20.06.2022 um 14:38 +0200 schrieb Ahmad Fatoum:
> Hello Teresa,
> 
> On 20.06.22 14:27, Teresa Remmet wrote:
> > Hello Ahmad,
> > 
> > Am Montag, dem 20.06.2022 um 14:02 +0200 schrieb Ahmad Fatoum:
> > > Hello,
> > > 
> > > On 20.05.22 16:23, Teresa Remmet wrote:
> > > > As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
> > > > now for the row size calculation we need to increase row_max to
> > > > 18.
> > > > 
> > > > For LPDDR4 this only works in combination with ram timings
> > > > created with recent configuration spreadsheet versions.
> > > > With older versions the register DDRC_ADDRMAP7 may not be set
> > > > and
> > > > calculation will lead to wrong results even with this patch.
> > > > 
> > > > Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit
> > > > SDRAM
> > > > row size handle")
> > > > Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> > > 
> > > I have an out-of-tree i.MX8MM board with LPDDR4 that reported a
> > > correct size of 1G
> > > prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not
> > > explicitly initialized,
> > > but it's being read as zero, so the calculation seems to still be
> > > broken..
> > 
> > yes, this is why I added the note to the commit message. The RAM
> > timings of the board you are using have been created with a old
> > version
> > of the spreadsheet ( < version 19 for i.MX8MM).
> > This issue is worked around when this patch is reverted as
> > DDRC_ADDRMAP7 is not taken into account. But calculating big ram
> > sizes
> > will probably not work then.
> > 
> > I have set the DDRC_ADDRMAP7 register manually in the RAM
> > configuration
> > in such a case. As I don't see a solution that fits for all. But
> > would
> > be happy for one. :)
> 
> What would the 'neutral' value to write into this register be? zero
> seems to not be it.

it's 

0xf0f for ADDRMAP7.

Reference Manual says: "If set to 15, row address bit X is set to 0"

Regards,
Teresa


> 
> Thanks,
> Ahmad
> 
> > Regards,
> > Teresa
> > 
> > > > ---
> > > >  arch/arm/mach-imx/esdctl.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
> > > > imx/esdctl.c
> > > > index 8dd0ddbbc965..b070ebc62a45 100644
> > > > --- a/arch/arm/mach-imx/esdctl.c
> > > > +++ b/arch/arm/mach-imx/esdctl.c
> > > > @@ -488,7 +488,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),
> > > > +				   18, ARRAY_AND_SIZE(row_b),
> > > >  				   reduced_adress_space, true);
> > > >  }
> > > >  
> 
> 
-- 
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany

Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855

Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Ahmad Fatoum]

Hi,

On 20.06.22 14:47, Teresa Remmet wrote:
> Am Montag, dem 20.06.2022 um 14:38 +0200 schrieb Ahmad Fatoum:
>> On 20.06.22 14:27, Teresa Remmet wrote:
>>> I have set the DDRC_ADDRMAP7 register manually in the RAM
>>> configuration
>>> in such a case. As I don't see a solution that fits for all. But
>>> would
>>> be happy for one. :)
>>
>> What would the 'neutral' value to write into this register be? zero
>> seems to not be it.
> 
> it's 
> 
> 0xf0f for ADDRMAP7.
> 
> Reference Manual says: "If set to 15, row address bit X is set to 0"

Do newer spreadsheets always generate ADDRMAP7 writes even if the
value is zero? If so, we could perhaps initialize it to 0xf0f before
running ddr_cfg_umctl2(). The DDRC seems to be in reset while the
registers are being written, so this might just work.

As 0 is a valid value, I am wondering if this snippet introduced with
42d45ef380c5 ("ARM: imx: Add imx8 support for SDRAM with two or more bank groups")
is correct:

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++;
}

Thanks,
Ahmad

> 
> Regards,
> Teresa
> 
> 
>>
>> Thanks,
>> Ahmad
>>
>>> Regards,
>>> Teresa
>>>
>>>>> ---
>>>>>  arch/arm/mach-imx/esdctl.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
>>>>> imx/esdctl.c
>>>>> index 8dd0ddbbc965..b070ebc62a45 100644
>>>>> --- a/arch/arm/mach-imx/esdctl.c
>>>>> +++ b/arch/arm/mach-imx/esdctl.c
>>>>> @@ -488,7 +488,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),
>>>>> +				   18, ARRAY_AND_SIZE(row_b),
>>>>>  				   reduced_adress_space, true);
>>>>>  }
>>>>>  
>>
>>


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

Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m

[Teresa Remmet]

Hi!

Am Montag, dem 20.06.2022 um 15:19 +0200 schrieb Ahmad Fatoum:
> Hi,
> 
> On 20.06.22 14:47, Teresa Remmet wrote:
> > Am Montag, dem 20.06.2022 um 14:38 +0200 schrieb Ahmad Fatoum:
> > > On 20.06.22 14:27, Teresa Remmet wrote:
> > > > I have set the DDRC_ADDRMAP7 register manually in the RAM
> > > > configuration
> > > > in such a case. As I don't see a solution that fits for all.
> > > > But
> > > > would
> > > > be happy for one. :)
> > > 
> > > What would the 'neutral' value to write into this register be?
> > > zero
> > > seems to not be it.
> > 
> > it's 
> > 
> > 0xf0f for ADDRMAP7.
> > 
> > Reference Manual says: "If set to 15, row address bit X is set to
> > 0"
> 
> Do newer spreadsheets always generate ADDRMAP7 writes even if the
> value is zero? If so, we could perhaps initialize it to 0xf0f before
> running ddr_cfg_umctl2(). The DDRC seems to be in reset while the
> registers are being written, so this might just work.

this is not so easy to figure out because the spread sheet does 
not generate the c headers directly. 
But it is true for other ADDRMAP registers where the reset value is also zero.
So I would give it a try.

> 
> As 0 is a valid value, I am wondering if this snippet introduced with
> 42d45ef380c5 ("ARM: imx: Add imx8 support for SDRAM with two or more
> bank groups")
> is correct:
> 
> 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++;
> }

yes, this is wrong. LPDDR4 is no problem at the moment as the
spreadsheet does not set the value and the reset value here is zero.
But DDR4 uses it and there could be a "real" zero set which is then
ignored.

Regards,
Teresa

> 
> Thanks,
> Ahmad
> 
> > Regards,
> > Teresa
> > 
> > 
> > > Thanks,
> > > Ahmad
> > > 
> > > > Regards,
> > > > Teresa
> > > > 
> > > > > > ---
> > > > > >  arch/arm/mach-imx/esdctl.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
> > > > > > imx/esdctl.c
> > > > > > index 8dd0ddbbc965..b070ebc62a45 100644
> > > > > > --- a/arch/arm/mach-imx/esdctl.c
> > > > > > +++ b/arch/arm/mach-imx/esdctl.c
> > > > > > @@ -488,7 +488,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),
> > > > > > +				   18, ARRAY_AND_SIZE(row_b),
> > > > > >  				   reduced_adress_space, true);
> > > > > >  }
> > > > > >  
> 
> 
-- 
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany

Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855