[U-Boot] [RFC] colibri_imx7: Do not hardcode the memory size

[U-Boot] [RFC] colibri_imx7: Do not hardcode the memory size

[Fabio Estevam]

Instead of hardcoding the memory size, use the generic 
imx_ddr_size() function, which extracts the memory size
in runtime by reading the DDR controller registers.

This helps in supporting 512MB and 1GB modules.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
Hi Stefan,

I don't have the hardware to test, but I think we
should be good by using imx_ddr_size() here.

 board/toradex/colibri_imx7/colibri_imx7.c | 2 +-
 include/configs/colibri_imx7.h            | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/board/toradex/colibri_imx7/colibri_imx7.c b/board/toradex/colibri_imx7/colibri_imx7.c
index 2b7591e..9029c0b 100644
--- a/board/toradex/colibri_imx7/colibri_imx7.c
+++ b/board/toradex/colibri_imx7/colibri_imx7.c
@@ -52,7 +52,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 int dram_init(void)
 {
-	gd->ram_size = get_ram_size((void *)PHYS_SDRAM, PHYS_SDRAM_SIZE);
+	gd->ram_size = imx_ddr_size();
 
 	return 0;
 }
diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
index ff6bd67..02849ba 100644
--- a/include/configs/colibri_imx7.h
+++ b/include/configs/colibri_imx7.h
@@ -14,7 +14,6 @@
 #include "mx7_common.h"
 
 /*#define CONFIG_DBG_MONITOR*/
-#define PHYS_SDRAM_SIZE			SZ_1G
 
 /* Size of malloc() pool */
 #define CONFIG_SYS_MALLOC_LEN		(32 * SZ_1M)
-- 
2.7.4

[U-Boot] [RFC] colibri_imx7: Do not hardcode the memory size

[Marcel Ziswiler]

Hi Fabio

Stefan is currently at Linaro Connect and may not have access to
hardware.

On Mon, 2018-09-17 at 14:09 -0300, Fabio Estevam wrote:
> Instead of hardcoding the memory size, use the generic 
> imx_ddr_size() function, which extracts the memory size
> in runtime by reading the DDR controller registers.

In theory that sounds fine. However, in practise that function seems to
return zero on our modules which leads to it freezing during boot.

I am currently looking more closely into it and will keep you posted.

> This helps in supporting 512MB and 1GB modules.
>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Hi Stefan,
> 
> I don't have the hardware to test, but I think we
> should be good by using imx_ddr_size() here.
> 
>  board/toradex/colibri_imx7/colibri_imx7.c | 2 +-
>  include/configs/colibri_imx7.h            | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/board/toradex/colibri_imx7/colibri_imx7.c
> b/board/toradex/colibri_imx7/colibri_imx7.c
> index 2b7591e..9029c0b 100644
> --- a/board/toradex/colibri_imx7/colibri_imx7.c
> +++ b/board/toradex/colibri_imx7/colibri_imx7.c
> @@ -52,7 +52,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  
>  int dram_init(void)
>  {
> -	gd->ram_size = get_ram_size((void *)PHYS_SDRAM,
> PHYS_SDRAM_SIZE);

BTW: Notice the generic get_ram_size() function which actually already
did/does figure out the RAM size across all our modules regardless of
being NXP/imx based or not. The size given there is just some kind of
upper bound maxsize:

https://git.denx.de/?p=u-boot.git;a=blob;f=common/memsize.c#l21

> +	gd->ram_size = imx_ddr_size();
>  
>  	return 0;
>  }
> diff --git a/include/configs/colibri_imx7.h
> b/include/configs/colibri_imx7.h
> index ff6bd67..02849ba 100644
> --- a/include/configs/colibri_imx7.h
> +++ b/include/configs/colibri_imx7.h
> @@ -14,7 +14,6 @@
>  #include "mx7_common.h"
>  
>  /*#define CONFIG_DBG_MONITOR*/
> -#define PHYS_SDRAM_SIZE			SZ_1G
>  
>  /* Size of malloc() pool */
>  #define CONFIG_SYS_MALLOC_LEN		(32 * SZ_1M)

Cheers

Marcel

[U-Boot] [RFC] colibri_imx7: Do not hardcode the memory size

[Marcel Ziswiler]

Hi Fabio

On Tue, 2018-09-18 at 14:28 +0200, Marcel Ziswiler wrote:
> Stefan is currently at Linaro Connect and may not have access to
> hardware.
> 
> On Mon, 2018-09-17 at 14:09 -0300, Fabio Estevam wrote:
> > Instead of hardcoding the memory size, use the generic 
> > imx_ddr_size() function, which extracts the memory size
> > in runtime by reading the DDR controller registers.
> 
> In theory that sounds fine. However, in practise that function seems
> to
> return zero on our modules which leads to it freezing during boot.
> 
> I am currently looking more closely into it and will keep you posted.

I had a closer look at it. It turns out we kind of over provision the
DDR controller in order to be able to actually use various kinds and
sizes of chips without any need to change anything. So while this
really works the imx_ddr_size() function currently seems to overflow in
which case it just returns zero. I suggest fixing this by capping the
function which I will send out a patch for shortly.

> > This helps in supporting 512MB and 1GB modules.

No, due to us over provisioning the DDR controller and using the
generic get_ram_size() function we already do automatically detecting
256MB, 512MB or 1GB modules.

> > Signed-off-by: Fabio Estevam <festevam@gmail.com>
> > ---
> > Hi Stefan,
> > 
> > I don't have the hardware to test, but I think we
> > should be good by using imx_ddr_size() here.
> > 
> >  board/toradex/colibri_imx7/colibri_imx7.c | 2 +-
> >  include/configs/colibri_imx7.h            | 1 -
> >  2 files changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/board/toradex/colibri_imx7/colibri_imx7.c
> > b/board/toradex/colibri_imx7/colibri_imx7.c
> > index 2b7591e..9029c0b 100644
> > --- a/board/toradex/colibri_imx7/colibri_imx7.c
> > +++ b/board/toradex/colibri_imx7/colibri_imx7.c
> > @@ -52,7 +52,7 @@ DECLARE_GLOBAL_DATA_PTR;
> >  
> >  int dram_init(void)
> >  {
> > -	gd->ram_size = get_ram_size((void *)PHYS_SDRAM,
> > PHYS_SDRAM_SIZE);
> 
> BTW: Notice the generic get_ram_size() function which actually
> already
> did/does figure out the RAM size across all our modules regardless of
> being NXP/imx based or not. The size given there is just some kind of
> upper bound maxsize:
> 
> https://git.denx.de/?p=u-boot.git;a=blob;f=common/memsize.c#l21

I suggest we keep using the generic get_ram_size() function which works
very well but rather than passing a hardcoded maxsize we use the i.MX 7
specific imx_ddr_size() function. I will send out an updated patch.

> > +	gd->ram_size = imx_ddr_size();
> >  
> >  	return 0;
> >  }
> > diff --git a/include/configs/colibri_imx7.h
> > b/include/configs/colibri_imx7.h
> > index ff6bd67..02849ba 100644
> > --- a/include/configs/colibri_imx7.h
> > +++ b/include/configs/colibri_imx7.h
> > @@ -14,7 +14,6 @@
> >  #include "mx7_common.h"
> >  
> >  /*#define CONFIG_DBG_MONITOR*/
> > -#define PHYS_SDRAM_SIZE			SZ_1G
> >  
> >  /* Size of malloc() pool */
> >  #define CONFIG_SYS_MALLOC_LEN		(32 * SZ_1M)

Cheers

Marcel