[U-Boot-Users] [RFC/PATCH] fix initdram / use of phys_addr_t
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
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
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
On Mar 12, 2008, at 1:40 PM, Wolfgang Denk wrote:
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.
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
Kumar Gala wrote:
On Mar 12, 2008, at 1:40 PM, Wolfgang Denk wrote:
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.
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
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
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
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.
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
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
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
participants (5)
-
Jean-Christophe PLAGNIOL-VILLARD -
Jerry Van Baren -
Jon Loeliger -
Kumar Gala -
Wolfgang Denk