[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Kumar Gala]

Wolfgang,

Before I went and looked at every board that uses initdram I wanted to get
some feedback of such a patch (for a wide majority of boards) would be
acceptable.

The idea is that initdram() should really have returned a 'unsigned long'.
However if we are going to change everyone that has initdram I figure we
should make it return a phys_addr_t.

I believe you've had some discussions with Jon on the subject and I wanted
to know if using 'phys_addr_t' here would be acceptable (before I looked
at trying to fix up ~200 boards).

The patch gives an example of what I'm looking at changing.

- k

diff --git a/board/freescale/mpc8544ds/mpc8544ds.c b/board/freescale/mpc8544ds/mpc8544ds.c
index 8107016..1577f13 100644
--- a/board/freescale/mpc8544ds/mpc8544ds.c
+++ b/board/freescale/mpc8544ds/mpc8544ds.c
@@ -64,10 +64,9 @@ int checkboard (void)
 	return 0;
 }

-long int
-initdram(int board_type)
+phys_addr_t initdram(int board_type)
 {
-	long dram_size = 0;
+	phys_addr_t dram_size = 0;

 	puts("Initializing\n");

diff --git a/cpu/mpc85xx/spd_sdram.c b/cpu/mpc85xx/spd_sdram.c
index abc63c4..55c45b5 100644
--- a/cpu/mpc85xx/spd_sdram.c
+++ b/cpu/mpc85xx/spd_sdram.c
@@ -169,8 +169,7 @@ unsigned int determine_refresh_rate(unsigned int spd_refresh)
 }


-long int
-spd_sdram(void)
+phys_addr_t spd_sdram(void)
 {
 	volatile ccsr_ddr_t *ddr = (void *)(CFG_MPC85xx_DDR_ADDR);
 	spd_eeprom_t spd;
diff --git a/include/common.h b/include/common.h
index cd8aad0..229d15a 100644
--- a/include/common.h
+++ b/include/common.h
@@ -107,6 +107,8 @@ typedef volatile unsigned char	vu_char;
 #include <asm/blackfin.h>
 #endif

+#include <asm/io.h>
+
 #include <part.h>
 #include <flash.h>
 #include <image.h>
@@ -191,7 +193,7 @@ int	serial_buffered_tstc (void);
 void	hang		(void) __attribute__ ((noreturn));

 /* */
-long int initdram (int);
+phys_addr_t initdram (int);
 int	display_options (void);
 void	print_size (ulong, const char *);
 int	print_buffer (ulong addr, void* data, uint width, uint count, uint linelen);
diff --git a/include/spd_sdram.h b/include/spd_sdram.h
index a2be96c..8f994b2 100644
--- a/include/spd_sdram.h
+++ b/include/spd_sdram.h
@@ -1,6 +1,6 @@
 #ifndef _SPD_SDRAM_H_
 #define _SPD_SDRAM_H_

-long int spd_sdram(void);
+phys_addr_t spd_sdram(void);

 #endif

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Jon Loeliger]

Kumar Gala wrote:
> Wolfgang,
> 
> Before I went and looked at every board that uses initdram I wanted to get
> some feedback of such a patch (for a wide majority of boards) would be
> acceptable.
> 
> The idea is that initdram() should really have returned a 'unsigned long'.
> However if we are going to change everyone that has initdram I figure we
> should make it return a phys_addr_t.
> 
> I believe you've had some discussions with Jon on the subject and I wanted
> to know if using 'phys_addr_t' here would be acceptable (before I looked
> at trying to fix up ~200 boards).
> 
> The patch gives an example of what I'm looking at changing.
> 
> - k

Just FYI, I am in full support of this proposed change to
use phys_addr_t.


> index cd8aad0..229d15a 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -107,6 +107,8 @@ typedef volatile unsigned char	vu_char;
>  #include <asm/blackfin.h>
>  #endif
> 
> +#include <asm/io.h>
> +

I don't think asm/io.h is the right place for phys_addr_t.
However, adding it to asm/types.h might me.  That is where
Becky and I have been headed...

jdl

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Wolfgang Denk]

Dear Kumar,

in message <Pine.LNX.4.64.0803121037440.13065@blarg.am.freescale.net> you wrote:
> 
> The idea is that initdram() should really have returned a 'unsigned long'.
> However if we are going to change everyone that has initdram I figure we
> should make it return a phys_addr_t.

Um, no, I don't think so.

> +phys_addr_t initdram(int board_type)
>  {
> -	long dram_size = 0;
> +	phys_addr_t dram_size = 0;

No - initdram() does not return an address, it returns a size.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"There is nothing new under the sun, but there are lots of old things
we don't know yet."                                  - Ambrose Bierce

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Kumar Gala]

On Mar 12, 2008, at 1:40 PM, Wolfgang Denk wrote:

> Dear Kumar,
>
> in message <Pine.LNX. 
> 4.64.0803121037440.13065 at blarg.am.freescale.net> you wrote:
>>
>> The idea is that initdram() should really have returned a 'unsigned  
>> long'.
>> However if we are going to change everyone that has initdram I  
>> figure we
>> should make it return a phys_addr_t.
>
> Um, no, I don't think so.

would phys_size_t be better?

>> +phys_addr_t initdram(int board_type)
>> {
>> -	long dram_size = 0;
>> +	phys_addr_t dram_size = 0;
>
> No - initdram() does not return an address, it returns a size.

Sure, I understand it returns a size.  I was just using phys_addr_t to  
represent the type for both addresses and sizes.

- k

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Jerry Van Baren]

Kumar Gala wrote:
> On Mar 12, 2008, at 1:40 PM, Wolfgang Denk wrote:
> 
>> Dear Kumar,
>>
>> in message <Pine.LNX. 
>> 4.64.0803121037440.13065 at blarg.am.freescale.net> you wrote:
>>> The idea is that initdram() should really have returned a 'unsigned  
>>> long'.
>>> However if we are going to change everyone that has initdram I  
>>> figure we
>>> should make it return a phys_addr_t.
>> Um, no, I don't think so.
> 
> would phys_size_t be better?
> 
>>> +phys_addr_t initdram(int board_type)
>>> {
>>> -	long dram_size = 0;
>>> +	phys_addr_t dram_size = 0;
>> No - initdram() does not return an address, it returns a size.
> 
> Sure, I understand it returns a size.  I was just using phys_addr_t to  
> represent the type for both addresses and sizes.
> 
> - k

Shouldn't we just use size_t to return the size of what is effectively 
an array of /n/ bytes of RAM?  (Does size_t's baggage WRT different C 
standards and different C compilers cause more grief than it solves?)

gvb

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Wolfgang Denk]

Dear Kumar,

in message <96DCB2C0-3028-4A5B-A6F4-5CC3767A5040@kernel.crashing.org> you wrote:
> 
> would phys_size_t be better?

Yes, definitely much better :-)

> >> +phys_addr_t initdram(int board_type)
> >> {
> >> -	long dram_size = 0;
> >> +	phys_addr_t dram_size = 0;
> >
> > No - initdram() does not return an address, it returns a size.
> 
> Sure, I understand it returns a size.  I was just using phys_addr_t to  
> represent the type for both addresses and sizes.

I think that would be misleading. To me, phys_addr_t looks like a
pointer type, while *_size_t is an (unsigned?) integer type.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
... bacteriological warfare ... hard to believe we were once foolish
enough to play around with that.
	-- McCoy, "The Omega Glory", stardate unknown

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Wolfgang Denk]

In message <47D82D99.3050703@ge.com> you wrote:
>
> Shouldn't we just use size_t to return the size of what is effectively 
> an array of /n/ bytes of RAM?  (Does size_t's baggage WRT different C 
> standards and different C compilers cause more grief than it solves?)

My understanding is that size_t is a 32 bit type even on systems with
32+ bit physical addresses.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Nature is very un-American.  Nature never hurries."
- William George Jordan

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:38 Wed 12 Mar     , Kumar Gala wrote:
> Wolfgang,
> 
> Before I went and looked at every board that uses initdram I wanted to get
> some feedback of such a patch (for a wide majority of boards) would be
> acceptable.
> 
> The idea is that initdram() should really have returned a 'unsigned long'.
> However if we are going to change everyone that has initdram I figure we
> should make it return a phys_addr_t.
> 
> I believe you've had some discussions with Jon on the subject and I wanted
> to know if using 'phys_addr_t' here would be acceptable (before I looked
> at trying to fix up ~200 boards).
> 
> The patch gives an example of what I'm looking at changing.

I'm currently fixing the get_ram_size too that is define as
long   get_ram_size  (volatile long *, long);

Best Regards,
J.

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Kumar Gala]

So is this acceptable?

- k

diff --git a/board/freescale/mpc8544ds/mpc8544ds.c b/board/freescale/mpc8544ds/mpc8544ds.c
index 8107016..24eea6f 100644
--- a/board/freescale/mpc8544ds/mpc8544ds.c
+++ b/board/freescale/mpc8544ds/mpc8544ds.c
@@ -64,10 +64,9 @@ int checkboard (void)
 	return 0;
 }

-long int
-initdram(int board_type)
+phys_size_t initdram(int board_type)
 {
-	long dram_size = 0;
+	phys_size_t dram_size = 0;

 	puts("Initializing\n");

diff --git a/cpu/mpc85xx/spd_sdram.c b/cpu/mpc85xx/spd_sdram.c
index abc63c4..b2b1911 100644
--- a/cpu/mpc85xx/spd_sdram.c
+++ b/cpu/mpc85xx/spd_sdram.c
@@ -169,8 +169,7 @@ unsigned int determine_refresh_rate(unsigned int spd_refresh)
 }


-long int
-spd_sdram(void)
+phys_size_t spd_sdram(void)
 {
 	volatile ccsr_ddr_t *ddr = (void *)(CFG_MPC85xx_DDR_ADDR);
 	spd_eeprom_t spd;
diff --git a/include/asm-ppc/types.h b/include/asm-ppc/types.h
index 7adf145..a797a00 100644
--- a/include/asm-ppc/types.h
+++ b/include/asm-ppc/types.h
@@ -44,6 +44,8 @@ typedef unsigned long long u64;
 /* DMA addresses are 32-bits wide */
 typedef u32 dma_addr_t;

+typedef unsigned long phys_size_t;
+
 #endif /* __KERNEL__ */
 #endif /* __ASSEMBLY__ */

diff --git a/include/common.h b/include/common.h
index cd8aad0..97e8e5a 100644
--- a/include/common.h
+++ b/include/common.h
@@ -107,6 +107,8 @@ typedef volatile unsigned char	vu_char;
 #include <asm/blackfin.h>
 #endif

+#include <asm/types.h>
+
 #include <part.h>
 #include <flash.h>
 #include <image.h>
@@ -191,7 +193,7 @@ int	serial_buffered_tstc (void);
 void	hang		(void) __attribute__ ((noreturn));

 /* */
-long int initdram (int);
+phys_size_t initdram (int);
 int	display_options (void);
 void	print_size (ulong, const char *);
 int	print_buffer (ulong addr, void* data, uint width, uint count, uint linelen);
diff --git a/include/spd_sdram.h b/include/spd_sdram.h
index a2be96c..4909e41 100644
--- a/include/spd_sdram.h
+++ b/include/spd_sdram.h
@@ -1,6 +1,6 @@
 #ifndef _SPD_SDRAM_H_
 #define _SPD_SDRAM_H_

-long int spd_sdram(void);
+phys_size_t spd_sdram(void);

 #endif

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Jon Loeliger]

Kumar Gala wrote:
> So is this acceptable?
> 
> - k
> 
> diff --git a/board/freescale/mpc8544ds/mpc8544ds.c b/board/freescale/mpc8544ds/mpc8544ds.c
> index 8107016..24eea6f 100644
> --- a/board/freescale/mpc8544ds/mpc8544ds.c
> +++ b/board/freescale/mpc8544ds/mpc8544ds.c
> @@ -64,10 +64,9 @@ int checkboard (void)
>  	return 0;
>  }
> 
> -long int
> -initdram(int board_type)
> +phys_size_t initdram(int board_type)
>  {
> -	long dram_size = 0;
> +	phys_size_t dram_size = 0;
> 
>  	puts("Initializing\n");
> 
> diff --git a/cpu/mpc85xx/spd_sdram.c b/cpu/mpc85xx/spd_sdram.c
> index abc63c4..b2b1911 100644
> --- a/cpu/mpc85xx/spd_sdram.c
> +++ b/cpu/mpc85xx/spd_sdram.c
> @@ -169,8 +169,7 @@ unsigned int determine_refresh_rate(unsigned int spd_refresh)
>  }
> 
> 
> -long int
> -spd_sdram(void)
> +phys_size_t spd_sdram(void)
>  {
>  	volatile ccsr_ddr_t *ddr = (void *)(CFG_MPC85xx_DDR_ADDR);
>  	spd_eeprom_t spd;
> diff --git a/include/asm-ppc/types.h b/include/asm-ppc/types.h
> index 7adf145..a797a00 100644
> --- a/include/asm-ppc/types.h
> +++ b/include/asm-ppc/types.h
> @@ -44,6 +44,8 @@ typedef unsigned long long u64;
>  /* DMA addresses are 32-bits wide */
>  typedef u32 dma_addr_t;
> 
> +typedef unsigned long phys_size_t;
> +
>  #endif /* __KERNEL__ */
>  #endif /* __ASSEMBLY__ */
> 
> diff --git a/include/common.h b/include/common.h
> index cd8aad0..97e8e5a 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -107,6 +107,8 @@ typedef volatile unsigned char	vu_char;
>  #include <asm/blackfin.h>
>  #endif
> 
> +#include <asm/types.h>
> +
>  #include <part.h>
>  #include <flash.h>
>  #include <image.h>
> @@ -191,7 +193,7 @@ int	serial_buffered_tstc (void);
>  void	hang		(void) __attribute__ ((noreturn));
> 
>  /* */
> -long int initdram (int);
> +phys_size_t initdram (int);
>  int	display_options (void);
>  void	print_size (ulong, const char *);
>  int	print_buffer (ulong addr, void* data, uint width, uint count, uint linelen);
> diff --git a/include/spd_sdram.h b/include/spd_sdram.h
> index a2be96c..4909e41 100644
> --- a/include/spd_sdram.h
> +++ b/include/spd_sdram.h
> @@ -1,6 +1,6 @@
>  #ifndef _SPD_SDRAM_H_
>  #define _SPD_SDRAM_H_
> 
> -long int spd_sdram(void);
> +phys_size_t spd_sdram(void);
> 
>  #endif


Looks good to me.

Is this the right patch to change the type of
the parameter to print_size() too?

jdl

[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t

[Wolfgang Denk]

In message <Pine.LNX.4.64.0803121811580.17362@blarg.am.freescale.net> you wrote:
> So is this acceptable?

Looks good to me.

Please don't forget common/memsize.c and callers; also, printing the
new type may be tricky.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"You shouldn't make my toaster angry." - Household security explained
in "Johnny Quest"