* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
@ 2010-05-27 18:11 Wolfgang Denk
2010-05-27 18:16 ` [U-Boot] [PATCH v2] " Wolfgang Denk
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 18:11 UTC (permalink / raw)
To: u-boot
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! */
--
1.6.6.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v2] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 18:11 [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit Wolfgang Denk
@ 2010-05-27 18:16 ` Wolfgang Denk
2010-05-27 19:46 ` Scott Wood
2010-05-27 18:23 ` [U-Boot] [PATCH] " Timur Tabi
2010-05-27 18:59 ` Wolfgang Wegner
2 siblings, 1 reply; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 18:16 UTC (permalink / raw)
To: u-boot
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);
--
1.6.6.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 18:11 [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit Wolfgang Denk
2010-05-27 18:16 ` [U-Boot] [PATCH v2] " Wolfgang Denk
@ 2010-05-27 18:23 ` Timur Tabi
2010-05-27 19:44 ` Wolfgang Denk
2010-05-27 18:59 ` Wolfgang Wegner
2 siblings, 1 reply; 18+ messages in thread
From: Timur Tabi @ 2010-05-27 18:23 UTC (permalink / raw)
To: u-boot
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".
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 18:11 [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit Wolfgang Denk
2010-05-27 18:16 ` [U-Boot] [PATCH v2] " Wolfgang Denk
2010-05-27 18:23 ` [U-Boot] [PATCH] " Timur Tabi
@ 2010-05-27 18:59 ` Wolfgang Wegner
2010-05-27 19:49 ` Wolfgang Denk
2 siblings, 1 reply; 18+ messages in thread
From: Wolfgang Wegner @ 2010-05-27 18:59 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 18:23 ` [U-Boot] [PATCH] " Timur Tabi
@ 2010-05-27 19:44 ` Wolfgang Denk
2010-05-27 20:01 ` Timur Tabi
2010-05-27 20:06 ` Scott Wood
0 siblings, 2 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 19:44 UTC (permalink / raw)
To: u-boot
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
--
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
"... freedom ... is a worship word..."
"It is our worship word too."
-- Cloud William and Kirk, "The Omega Glory", stardate unknown
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v2] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 18:16 ` [U-Boot] [PATCH v2] " Wolfgang Denk
@ 2010-05-27 19:46 ` Scott Wood
2010-05-27 19:57 ` Wolfgang Denk
0 siblings, 1 reply; 18+ messages in thread
From: Scott Wood @ 2010-05-27 19:46 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 18:59 ` Wolfgang Wegner
@ 2010-05-27 19:49 ` Wolfgang Denk
0 siblings, 0 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 19:49 UTC (permalink / raw)
To: u-boot
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
--
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
Never underestimate the bandwidth of a station wagon full of tapes.
-- Dr. Warren Jackson, Director, UTCS
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v2] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 19:46 ` Scott Wood
@ 2010-05-27 19:57 ` Wolfgang Denk
2010-05-27 20:00 ` Scott Wood
0 siblings, 1 reply; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 19:57 UTC (permalink / raw)
To: u-boot
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
--
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
"And it should be the law: If you use the word `paradigm' without
knowing what the dictionary says it means, you go to jail. No
exceptions." - David Jones @ Megatest Corporation
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v2] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 19:57 ` Wolfgang Denk
@ 2010-05-27 20:00 ` Scott Wood
2010-05-27 20:53 ` Wolfgang Denk
0 siblings, 1 reply; 18+ messages in thread
From: Scott Wood @ 2010-05-27 20:00 UTC (permalink / raw)
To: u-boot
On 05/27/2010 02:57 PM, Wolfgang Denk wrote:
> 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?
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 19:44 ` Wolfgang Denk
@ 2010-05-27 20:01 ` Timur Tabi
2010-05-27 20:57 ` Wolfgang Denk
2010-05-27 20:06 ` Scott Wood
1 sibling, 1 reply; 18+ messages in thread
From: Timur Tabi @ 2010-05-27 20:01 UTC (permalink / raw)
To: u-boot
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.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 19:44 ` Wolfgang Denk
2010-05-27 20:01 ` Timur Tabi
@ 2010-05-27 20:06 ` Scott Wood
2010-05-27 21:06 ` Wolfgang Denk
1 sibling, 1 reply; 18+ messages in thread
From: Scott Wood @ 2010-05-27 20:06 UTC (permalink / raw)
To: u-boot
On 05/27/2010 02:44 PM, Wolfgang Denk wrote:
> 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.
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v2] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 20:00 ` Scott Wood
@ 2010-05-27 20:53 ` Wolfgang Denk
0 siblings, 0 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 20:53 UTC (permalink / raw)
To: u-boot
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
--
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
The most difficult thing in the world is to know how to do a thing
and to watch someone else doing it wrong, without commenting.
-- T.H. White
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 20:01 ` Timur Tabi
@ 2010-05-27 20:57 ` Wolfgang Denk
2010-05-27 21:05 ` Timur Tabi
2010-05-27 21:10 ` Kumar Gala
0 siblings, 2 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 20:57 UTC (permalink / raw)
To: u-boot
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
--
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
"The two most common things in the universe are hydrogen and stupi-
dity."
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 20:57 ` Wolfgang Denk
@ 2010-05-27 21:05 ` Timur Tabi
2010-05-27 21:13 ` Wolfgang Denk
2010-05-27 21:10 ` Kumar Gala
1 sibling, 1 reply; 18+ messages in thread
From: Timur Tabi @ 2010-05-27 21:05 UTC (permalink / raw)
To: u-boot
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.
--
Timur Tabi
Linux kernel developer@Freescale
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 20:06 ` Scott Wood
@ 2010-05-27 21:06 ` Wolfgang Denk
0 siblings, 0 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 21:06 UTC (permalink / raw)
To: u-boot
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
--
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
I object to intellect without discipline; I object to power without
constructive purpose.
-- Spock, "The Squire of Gothos", stardate 2124.5
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 20:57 ` Wolfgang Denk
2010-05-27 21:05 ` Timur Tabi
@ 2010-05-27 21:10 ` Kumar Gala
2010-05-27 21:16 ` Wolfgang Denk
1 sibling, 1 reply; 18+ messages in thread
From: Kumar Gala @ 2010-05-27 21:10 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 21:05 ` Timur Tabi
@ 2010-05-27 21:13 ` Wolfgang Denk
0 siblings, 0 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 21:13 UTC (permalink / raw)
To: u-boot
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
--
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
"This is a test of the Emergency Broadcast System. If this had been an
actual emergency, do you really think we'd stick around to tell you?"
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit
2010-05-27 21:10 ` Kumar Gala
@ 2010-05-27 21:16 ` Wolfgang Denk
0 siblings, 0 replies; 18+ messages in thread
From: Wolfgang Denk @ 2010-05-27 21:16 UTC (permalink / raw)
To: u-boot
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
--
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
miracle: an extremely outstanding or unusual event, thing, or
accomplishment. - Webster's Dictionary
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2010-05-27 21:16 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-27 18:11 [U-Boot] [PATCH] [RFC] memsize.c: adapt get_ram_size() for address spaces >32 bit Wolfgang Denk
2010-05-27 18:16 ` [U-Boot] [PATCH v2] " Wolfgang Denk
2010-05-27 19:46 ` Scott Wood
2010-05-27 19:57 ` Wolfgang Denk
2010-05-27 20:00 ` Scott Wood
2010-05-27 20:53 ` Wolfgang Denk
2010-05-27 18:23 ` [U-Boot] [PATCH] " Timur Tabi
2010-05-27 19:44 ` Wolfgang Denk
2010-05-27 20:01 ` Timur Tabi
2010-05-27 20:57 ` Wolfgang Denk
2010-05-27 21:05 ` Timur Tabi
2010-05-27 21:13 ` Wolfgang Denk
2010-05-27 21:10 ` Kumar Gala
2010-05-27 21:16 ` Wolfgang Denk
2010-05-27 20:06 ` Scott Wood
2010-05-27 21:06 ` Wolfgang Denk
2010-05-27 18:59 ` Wolfgang Wegner
2010-05-27 19:49 ` Wolfgang Denk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox