[U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
get_ram_size() used to use "long" data types for addresses and data, which in limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Timur Tabi timur@freescale.com --- Note: this is only minimally tested - I just compiled a dozen of ppc boards with it without appearent problems. Please review and test carefully.
common/memsize.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/common/memsize.c b/common/memsize.c index 6c275c9..b99e51b 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -37,14 +37,14 @@ * the actually available RAM size between addresses `base' and * `base + maxsize'. */ -long get_ram_size(volatile long *base, long maxsize) +phys_size_t get_ram_size(volatile phys_addr_t *base, phys_size_t maxsize) { - volatile long *addr; - long save[32]; - long cnt; - long val; - long size; - int i = 0; + volatile phys_addr_t *addr; + phys_size_t save[32]; + phys_size_t cnt; + phys_size_t val; + phys_size_t size; + int i = 0;
for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */
get_ram_size() used to use "long" data types for addresses and data, which limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Timur Tabi timur@freescale.com --- Note: this is only minimally tested - I just compiled a dozen of ppc boards with it without appearent problems. Please review and test carefully.
v2: - fix commit message - change function prototype, too.
common/memsize.c | 14 +++++++------- include/common.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/common/memsize.c b/common/memsize.c index 6c275c9..b99e51b 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -37,14 +37,14 @@ * the actually available RAM size between addresses `base' and * `base + maxsize'. */ -long get_ram_size(volatile long *base, long maxsize) +phys_size_t get_ram_size(volatile phys_addr_t *base, phys_size_t maxsize) { - volatile long *addr; - long save[32]; - long cnt; - long val; - long size; - int i = 0; + volatile phys_addr_t *addr; + phys_size_t save[32]; + phys_size_t cnt; + phys_size_t val; + phys_size_t size; + int i = 0;
for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ diff --git a/include/common.h b/include/common.h index 8bca04f..648e00f 100644 --- a/include/common.h +++ b/include/common.h @@ -316,7 +316,7 @@ const char *symbol_lookup(unsigned long addr, unsigned long *caddr); void api_init (void);
/* common/memsize.c */ -long get_ram_size (volatile long *, long); +phys_size_t get_ram_size(volatile phys_addr_t *, phys_size_t);
/* $(BOARD)/$(BOARD).c */ void reset_phy (void);
On Thu, May 27, 2010 at 08:16:28PM +0200, Wolfgang Denk wrote:
get_ram_size() used to use "long" data types for addresses and data, which limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
This cannot work as is. The only systems where this makes a difference are where physical addresses are larger than virtual pointers -- but you try to shove the 64-bit physical offset into a 32-bit pointer.
You need to create temporary mappings, if you really want to do this.
-Scott
Dear Scott Wood,
In message 20100527194618.GC5915@schlenkerla.am.freescale.net you wrote:
On Thu, May 27, 2010 at 08:16:28PM +0200, Wolfgang Denk wrote:
get_ram_size() used to use "long" data types for addresses and data, which limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
This cannot work as is. The only systems where this makes a difference are where physical addresses are larger than virtual pointers -- but you try to shove the 64-bit physical offset into a 32-bit pointer.
You need to create temporary mappings, if you really want to do this.
?
Isn't phys_addr_t assumed to be the right data type to hold a physical address?
Best regards,
Wolfgang Denk
On 05/27/2010 02:57 PM, Wolfgang Denk wrote:
Dear Scott Wood,
In message20100527194618.GC5915@schlenkerla.am.freescale.net you wrote:
On Thu, May 27, 2010 at 08:16:28PM +0200, Wolfgang Denk wrote:
get_ram_size() used to use "long" data types for addresses and data, which limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
This cannot work as is. The only systems where this makes a difference are where physical addresses are larger than virtual pointers -- but you try to shove the 64-bit physical offset into a 32-bit pointer.
You need to create temporary mappings, if you really want to do this.
?
Isn't phys_addr_t assumed to be the right data type to hold a physical address?
Yes. But you can't dereference a physical address directly.
When you do "addr = base + cnt", you're throwing away the upper 32 bits.
"phys_addr_t *" is not a 64-bit pointer, it is a 32-bit pointer to a 64-bit quantity.
-Scott
Dear Scott Wood,
In message 4BFECF43.40600@freescale.com you wrote:
Isn't phys_addr_t assumed to be the right data type to hold a physical address?
Yes. But you can't dereference a physical address directly.
Ah, right. OK, so this needs more work...
Best regards,
Wolfgang Denk
On Thu, May 27, 2010 at 1:11 PM, Wolfgang Denk wd@denx.de wrote:
get_ram_size() used to use "long" data types for addresses and data, which in limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Timur Tabi timur@freescale.com
The problem is that on all of our PowerPC boards, the TLBs only map the lower 2GB of memory, regardless as to how much is present. So we still can't use get_ram_size() to determine how much memory is in the system, because any attempt to access memory higher than 2GB will fail.
And even if we did have TLBs for all of memory, an attempt to access RAM that doesn't exist will cause a machine check, which will hang U-Boot. So we still couldn't use get_ram_size() to determine how much RAM actually exists.
-long get_ram_size(volatile long *base, long maxsize) +phys_size_t get_ram_size(volatile phys_addr_t *base, phys_size_t maxsize)
I don't think you want 'base' to be a pointer to phys_addr_t, because the pointer type determines how much is read/written in a single operation. I don't think you want to be doing 64-bit reads and writes.
Plus, you still have this:
for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ sync (); save[i++] = *addr; sync (); *addr = ~cnt; }
I'm not quite sure what this loop is doing, but I have a suspicion the "sizeof(long)" does not match with "phys_addr_t *base".
Dear Timur Tabi,
In message AANLkTilrFhTwnQgQX9NVmplaBl8HpISZY97HKK73-ncz@mail.gmail.com you wrote:
On Thu, May 27, 2010 at 1:11 PM, Wolfgang Denk wd@denx.de wrote:
get_ram_size() used to use "long" data types for addresses and data, which in limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Timur Tabi timur@freescale.com
The problem is that on all of our PowerPC boards, the TLBs only map the lower 2GB of memory, regardless as to how much is present. So we still can't use get_ram_size() to determine how much memory is in the system, because any attempt to access memory higher than 2GB will fail.
Now this is your problem, then, and you should kno how to fix it.
And even if we did have TLBs for all of memory, an attempt to access RAM that doesn't exist will cause a machine check, which will hang U-Boot. So we still couldn't use get_ram_size() to determine how much RAM actually exists.
Please see how it's done on all other PowerPC systems, and do similar.
-long get_ram_size(volatile long *base, long maxsize) +phys_size_t get_ram_size(volatile phys_addr_t *base, phys_size_t maxsize)
I don't think you want 'base' to be a pointer to phys_addr_t, because the pointer type determines how much is read/written in a single operation. I don't think you want to be doing 64-bit reads and writes.
I don't know your mnemory bus. This is an RFC patch.
Plus, you still have this:
for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ sync (); save[i++] = *addr; sync (); *addr = ~cnt; }
I'm not quite sure what this loop is doing, but I have a suspicion the "sizeof(long)" does not match with "phys_addr_t *base".
Right. This needs to be fixed.
Thanks for pointing out.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
The problem is that on all of our PowerPC boards, the TLBs only map the lower 2GB of memory, regardless as to how much is present. So we still can't use get_ram_size() to determine how much memory is in the system, because any attempt to access memory higher than 2GB will fail.
Now this is your problem, then, and you should kno how to fix it.
Scott pointed out that writing/reading memory to determine how much memory actually exists is dangerous. I'm not convinced that I should be using get_ram_size(). I still believe that I shouldn't.
And even if we did have TLBs for all of memory, an attempt to access RAM that doesn't exist will cause a machine check, which will hang U-Boot. So we still couldn't use get_ram_size() to determine how much RAM actually exists.
Please see how it's done on all other PowerPC systems, and do similar.
I have not been able to find any other PowerPC system in U-boot that supports more memory than is mapped. If you know of one, please tell me. Otherwise, I would say that there are no other comparable PowerPC systems that I can use as an example.
-long get_ram_size(volatile long *base, long maxsize) +phys_size_t get_ram_size(volatile phys_addr_t *base, phys_size_t maxsize)
I don't think you want 'base' to be a pointer to phys_addr_t, because the pointer type determines how much is read/written in a single operation. I don't think you want to be doing 64-bit reads and writes.
I don't know your mnemory bus. This is an RFC patch.
My point is that sizeof(phys_addr_t) has got nothing to do with the size of the read/write operation, so I think it's wrong on all platforms.
Dear Timur Tabi,
In message 4BFECF82.60901@freescale.com you wrote:
Scott pointed out that writing/reading memory to determine how much memory actually exists is dangerous. I'm not convinced that I should be using get_ram_size(). I still believe that I shouldn't.
And I point out that we have been doing this for a decade on tens of different board configurations with millions of devices in the field.
I have not been able to find any other PowerPC system in U-boot that supports more memory than is mapped. If you know of one, please tell me.
The systems I know are the opposite - initially they map more memory than they support, then they determine the real size, then they adjust the mapping to the real size.
My point is that sizeof(phys_addr_t) has got nothing to do with the size of the read/write operation, so I think it's wrong on all platforms.
Actually it will only be wrong on systems where phys_addr_t != ulong, but you are right.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
The systems I know are the opposite - initially they map more memory than they support, then they determine the real size, then they adjust the mapping to the real size.
But we don't ever do that, at least not on systems that use SPD. We query the DIMMs directly via SPD and calculate how much memory is in the system.
Most of our boards support both SPD and "fixed" DDR programming. In fixed mode, the actual values to be programmed in the controller are hard-coded in the board header file, like this:
#define CONFIG_SYS_DDR_CS0_BNDS 0x0000003F #define CONFIG_SYS_DDR_CS1_BNDS 0x00000000 #define CONFIG_SYS_DDR_CS0_CONFIG 0x80014202 #define CONFIG_SYS_DDR_CS1_CONFIG 0x00000000 ...
So in this case, as long as there's <= 2GB of DDR expected, then get_ram_size() can work.
But on the P1022DS, I don't support fixed DDR mode. Only SPD is supported, so I have no idea at compile-time how much memory is in the system. That's why I don't think calling get_ram_size() is appropriate for the P1022DS board.
Dear Timur Tabi,
In message 4BFEDE90.6070802@freescale.com you wrote:
Wolfgang Denk wrote:
The systems I know are the opposite - initially they map more memory than they support, then they determine the real size, then they adjust the mapping to the real size.
But we don't ever do that, at least not on systems that use SPD. We query the DIMMs directly via SPD and calculate how much memory is in the system.
Most of our boards support both SPD and "fixed" DDR programming. In fixed mode, the actual values to be programmed in the controller are hard-coded in the board header file, like this:
When you have SPD information you can of course use this for size information. In the "fixed" case you get a max size from the #defines.
But on the P1022DS, I don't support fixed DDR mode. Only SPD is supported, so I have no idea at compile-time how much memory is in the system. That's why I don't think calling get_ram_size() is appropriate for the P1022DS board.
You do not need to know this at compile time - it is sufficient to know it when calling get_ram_size().
Best regards,
Wolfgang Denk
On May 27, 2010, at 3:57 PM, Wolfgang Denk wrote:
Dear Timur Tabi,
In message 4BFECF82.60901@freescale.com you wrote:
Scott pointed out that writing/reading memory to determine how much memory actually exists is dangerous. I'm not convinced that I should be using get_ram_size(). I still believe that I shouldn't.
And I point out that we have been doing this for a decade on tens of different board configurations with millions of devices in the field.
I have not been able to find any other PowerPC system in U-boot that supports more memory than is mapped. If you know of one, please tell me.
The systems I know are the opposite - initially they map more memory than they support, then they determine the real size, then they adjust the mapping to the real size.
Is the point of get_ram_size() to deal with having the same firmware image (binary) be able to be agnostic of memory size?
I'm not sure I understand what utility one gets out of it if we have SPD eeprom information? I can see some purpose in the soldered memory case if you dont want to tweak binaries between different memory size configs.
- k
Dear Kumar Gala,
In message 48A9B08D-9237-4837-91CA-0D23CE1E56D8@kernel.crashing.org you wrote:
The systems I know are the opposite - initially they map more memory than they support, then they determine the real size, then they adjust the mapping to the real size.
Is the point of get_ram_size() to deal with having the same firmware image (binary) be able to be agnostic of memory size?
Yes, this is the key point of it.
I'm not sure I understand what utility one gets out of it if we have SPD eeprom information? I can see some purpose in the soldered memory case if you dont want to tweak binaries between different memory size configs.
With SPD information the benefits go down to the (limited) testing it does, so you might still detect hardware issues - if you try and insert a 2 GiB DIMM and U-Boot sees only 512 MiB then you can be damn sure that something is not working as expected.
Especially on systems where the end user can plug in any types of memory modules that seems to be a pretty useful feature to me.
Best regards,
Wolfgang Denk
On 05/27/2010 02:44 PM, Wolfgang Denk wrote:
Dear Timur Tabi,
In messageAANLkTilrFhTwnQgQX9NVmplaBl8HpISZY97HKK73-ncz@mail.gmail.com you wrote:
On Thu, May 27, 2010 at 1:11 PM, Wolfgang Denkwd@denx.de wrote:
get_ram_size() used to use "long" data types for addresses and data, which in limited it to systems with less than 4 GiB memory. As more and more systems are coming up with bigger memory resources, we adapt the code to use phys_addr_t / phys_size_t data types instead.
Signed-off-by: Wolfgang Denkwd@denx.de Cc: Timur Tabitimur@freescale.com
The problem is that on all of our PowerPC boards, the TLBs only map the lower 2GB of memory, regardless as to how much is present. So we still can't use get_ram_size() to determine how much memory is in the system, because any attempt to access memory higher than 2GB will fail.
Now this is your problem, then, and you should kno how to fix it.
Not using get_ram_size(), which is of minimal utility when you know the actual size (and is an unsafe approach to the problem when you don't, unless you know a lot about where I/O is mapped and how the system responds to accessing memory that doesn't exist -- not exactly the sort of thing you want to make a mandate for any board to be accepted), seems like a good way to "fix it".
And even if we did have TLBs for all of memory, an attempt to access RAM that doesn't exist will cause a machine check, which will hang U-Boot. So we still couldn't use get_ram_size() to determine how much RAM actually exists.
Please see how it's done on all other PowerPC systems, and do similar.
"All other PowerPC systems" is a ridiculous overstatement. I see exactly *one* Freescale board that uses this thing, and it's an ARM board.
-Scott
Dear Scott Wood,
In message 4BFED0CD.5010301@freescale.com you wrote:
Not using get_ram_size(), which is of minimal utility when you know the actual size (and is an unsafe approach to the problem when you don't, unless you know a lot about where I/O is mapped and how the system responds to accessing memory that doesn't exist -- not exactly the sort of thing you want to make a mandate for any board to be accepted), seems like a good way to "fix it".
Come on. Anybody who does NOT know the exact memory map of a system (and is able to define it as needed) is obviously not in a position to port U-Boot to this hardware.
This is basic knowledge.
"All other PowerPC systems" is a ridiculous overstatement. I see exactly *one* Freescale board that uses this thing, and it's an ARM board.
And this uses it incorrectly, like all ARM boards, if I'm not wrong.
Well, there is a ton of other boards besides the ones manufactured by Freescale, including many that use Freescale chips.
I am not to blame if Freescale did not pick up a number of design concepts.
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
as the systems I was working on always had far less memory, I can not comment much on the extension you propose here, but as Timur Tabi's comments seem to go in a direction which could lead to a bigger overhaul/discussion, I would like to add 2C from my point of view on coldfire...
- MCF53xx/MCF5445x both simply lock up if non-existent memory is accessed. So if the SDRAM controller is set up for a too big size of memory, get_ram_size() will fail. I assume this applies to most coldfire devices. - How about systems/configurations using CONFIG_MONITOR_IS_IN_RAM? I could not see special precautions for this, but in case an address to be tested by accident is occupied by a part of get_ram_size() itself, the result would be ... interesting. ;-) Of course, this is a rare thing (both using CONFIG_MONITOR_IS_IN_RAM and the chance to have get_ram_size() at such a crucial location), but still I fear it might have an impact if get_ram_size() gets "mandatory". - at least in the coldfire world, CONFIG_SYS_SDRAM_SIZE is quite often used for cache setup in the assembly code. This contradicts changing/setting the SDRAM size at runtime...
(Please don't see this as a vote against using and promoting get_ram_size() - I just see some problems that I am not aware of an easy solution for.)
Best regards, Wolfgang Wegner
Dear Wolfgang Wegner,
In message 20100527185909.GG31271@leila.ping.de you wrote:
as the systems I was working on always had far less memory, I can not comment much on the extension you propose here, but as Timur Tabi's comments seem to go in a direction which could lead to a bigger overhaul/discussion, I would like to add 2C from my point of view on coldfire...
- MCF53xx/MCF5445x both simply lock up if non-existent memory is accessed. So if the SDRAM controller is set up for a too big size of memory, get_ram_size() will fail. I assume this applies to most coldfire devices.
The design of get_ram_size() is based on the assumption that you start with a "big enough" mapping. Usually we map twice the maximum possible size that actually can be fit.
- How about systems/configurations using CONFIG_MONITOR_IS_IN_RAM?
Here you obviously cannot use this. What some ARM systems are doing right now is plain wrong. get_ram_size() must always and only be called _before_ the code is running from RAM.
- at least in the coldfire world, CONFIG_SYS_SDRAM_SIZE is quite often used for cache setup in the assembly code. This contradicts changing/setting the SDRAM size at runtime...
You may want to change this :-)
Note that we have plans underway to rework the ARM architecture to do proper relocation including auto-sizing of the RAM. Other architectures are invited to follow.
Best regards,
Wolfgang Denk
participants (5)
-
Kumar Gala -
Scott Wood -
Timur Tabi -
Wolfgang Denk -
wolfgang＠leila.ping.de