[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Oliver Schinagl]

Hey all,

I just yesterday received my CubieTruck (cubieboard3) with 2 GiB of Ram 
and added support for it to the sunxi-u-boot branch. While I know this 
isn't merged into the main u-boot tree (yet), I ran into the following 
problem.

At the end of the dram init code, it is customary to call get_ram_size() 
and return its value. This is then used to print the DRAM size and also 
is passed to the Linux kernel.

However the return size of get_ram_size() is a long. While I don't 
understand why not unsigned long or even u64 was chosen, this causes 
get_dram_size to overflow when having a ramsize of 2 GiB. While only 
printing of the value isn't hugely important, this does indicate u-boot 
seems to be somewhat artificially limited to 2 GiB of Ram? This only 
seems to affect the SPL as, if I understood correctly, there it stores 
the ram_size into the gd struct which I think is unsigned long.

I've started working on a patch to convert common/memsize.c's 
get_ram_size(), to be completely unsigned long, however there seems to 
be quite a lot of code that calls this. So my question is now before 
going over all drivers and change that and submit a big patch-set, did I 
overlook anything and are my conclusions correct, get_ram_size should 
return unsigned long.

Finally, a long is 32 bits on x86 and armv7, but how will that relate to 
64bits armv8? As I understood, Windows treats long's as 4 bytes no 
matter if it's 32 bit or 64 bit. Linux is better and a long is 4 bytes 
on 32 bits, and 8 bytes on 64 bits versions. So how will u-boot work on 
armv8? Will the long datatype work well, or should I consider changing 
things more future proof? (u32 and u64 come to mind).

Thank you for any input regarding that issue.

Oliver

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Oliver Schinagl]

Hey all,

Having not received any feed back at all, I went ahead and did the 
changes anyway. Everything seems to run and work fine for sunxi and 
prints proper sizes.

For the other boards, I tried to run a MAKEALL but there where so many 
random other warnings I can't say for 100% certainty there where no 
mistakes that crept in. Boards where strange size conversion where done 
or it was assumed that the return value of get_ram_size() was long are 
typecast to have the same (erroneous) behavior. Boards that had simple 
fixes where fixed.

Ideally, every maintainer should check if their board requires deeper 
review and fixing due to this change.

Oliver

On 03-10-13 23:15, Oliver Schinagl wrote:
> Hey all,
>
> I just yesterday received my CubieTruck (cubieboard3) with 2 GiB of Ram
> and added support for it to the sunxi-u-boot branch. While I know this
> isn't merged into the main u-boot tree (yet), I ran into the following
> problem.
>
> At the end of the dram init code, it is customary to call get_ram_size()
> and return its value. This is then used to print the DRAM size and also
> is passed to the Linux kernel.
>
> However the return size of get_ram_size() is a long. While I don't
> understand why not unsigned long or even u64 was chosen, this causes
> get_dram_size to overflow when having a ramsize of 2 GiB. While only
> printing of the value isn't hugely important, this does indicate u-boot
> seems to be somewhat artificially limited to 2 GiB of Ram? This only
> seems to affect the SPL as, if I understood correctly, there it stores
> the ram_size into the gd struct which I think is unsigned long.
>
> I've started working on a patch to convert common/memsize.c's
> get_ram_size(), to be completely unsigned long, however there seems to
> be quite a lot of code that calls this. So my question is now before
> going over all drivers and change that and submit a big patch-set, did I
> overlook anything and are my conclusions correct, get_ram_size should
> return unsigned long.
>
> Finally, a long is 32 bits on x86 and armv7, but how will that relate to
> 64bits armv8? As I understood, Windows treats long's as 4 bytes no
> matter if it's 32 bit or 64 bit. Linux is better and a long is 4 bytes
> on 32 bits, and 8 bytes on 64 bits versions. So how will u-boot work on
> armv8? Will the long datatype work well, or should I consider changing
> things more future proof? (u32 and u64 come to mind).
>
> Thank you for any input regarding that issue.
>
> Oliver
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Albert ARIBAUD]

Hi Oliver,

On Mon, 07 Oct 2013 04:41:31 +0200, Oliver Schinagl
<oliver+list@schinagl.nl> wrote:

> Hey all,
> 
> Having not received any feed back at all, I went ahead and did the 
> changes anyway. Everything seems to run and work fine for sunxi and 
> prints proper sizes.
> 
> For the other boards, I tried to run a MAKEALL but there where so many 
> random other warnings I can't say for 100% certainty there where no 
> mistakes that crept in.

There cannot possibly be a single warning if you're working from an
official U-Boot repo, as warnings are considered failures and thus no
patch reaches u-boot/master if it causes a warning. Thus, any warning
stems from the change you're introducing, and they are probably due to
a few common causes.

Can you provide a rough analysis/classification of the warnings? 

Amicalement,
-- 
Albert.

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Scott Wood]

On Tue, 2013-10-15 at 09:12 +0200, Albert ARIBAUD wrote:
> Hi Oliver,
> 
> On Mon, 07 Oct 2013 04:41:31 +0200, Oliver Schinagl
> <oliver+list@schinagl.nl> wrote:
> 
> > Hey all,
> > 
> > Having not received any feed back at all, I went ahead and did the 
> > changes anyway. Everything seems to run and work fine for sunxi and 
> > prints proper sizes.
> > 
> > For the other boards, I tried to run a MAKEALL but there where so many 
> > random other warnings I can't say for 100% certainty there where no 
> > mistakes that crept in.
> 
> There cannot possibly be a single warning if you're working from an
> official U-Boot repo, as warnings are considered failures and thus no
> patch reaches u-boot/master if it causes a warning.

That might be the theory, but in practice this is simply false.
Different toolchains produce different warnings, and not all patches
always get test-built on every target (especially on obscure
architectures).  And since it's false that no warnings exist, that means
sometimes even when a patch is test-built, some newly introduced
warnings get missed (I got an e-mail pointing out such an occurance just
today).

-Scott

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Albert ARIBAUD]

Hi Scott,

On Tue, 15 Oct 2013 12:57:33 -0500, Scott Wood
<scottwood@freescale.com> wrote:

> On Tue, 2013-10-15 at 09:12 +0200, Albert ARIBAUD wrote:
> > Hi Oliver,
> > 
> > On Mon, 07 Oct 2013 04:41:31 +0200, Oliver Schinagl
> > <oliver+list@schinagl.nl> wrote:
> > 
> > > Hey all,
> > > 
> > > Having not received any feed back at all, I went ahead and did the 
> > > changes anyway. Everything seems to run and work fine for sunxi and 
> > > prints proper sizes.
> > > 
> > > For the other boards, I tried to run a MAKEALL but there where so many 
> > > random other warnings I can't say for 100% certainty there where no 
> > > mistakes that crept in.
> > 
> > There cannot possibly be a single warning if you're working from an
> > official U-Boot repo, as warnings are considered failures and thus no
> > patch reaches u-boot/master if it causes a warning.
> 
> That might be the theory, but in practice this is simply false.
> Different toolchains produce different warnings, and not all patches
> always get test-built on every target (especially on obscure
> architectures).  And since it's false that no warnings exist, that means
> sometimes even when a patch is test-built, some newly introduced
> warnings get missed (I got an e-mail pointing out such an occurance just
> today).

You are correct that the same code may or may not emit warnings
depending on the toolchain, and that U-Boot's build system won't stop
building because of warnings.

However, when a new toolchain version causes such warnings, but they
are not 'random' in any case; they may be numerous though, if in some
source code used in a lot of boards.

In any case, if Oliver gets warnings, chances are we'll get them to
when applying his code, in which case it'll be rejected, or we'll see
them happening later if he's unsing a common toolchain in a new
version, or he's using an unusual toolchain.

> -Scott

Amicalement,
-- 
Albert.

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Oliver Schinagl]

On 17-10-13 08:27, Albert ARIBAUD wrote:
> Hi Scott,
>
> On Tue, 15 Oct 2013 12:57:33 -0500, Scott Wood
> <scottwood@freescale.com> wrote:
>
>> On Tue, 2013-10-15 at 09:12 +0200, Albert ARIBAUD wrote:
>>> Hi Oliver,
>>>
>>> On Mon, 07 Oct 2013 04:41:31 +0200, Oliver Schinagl
>>> <oliver+list@schinagl.nl> wrote:
>>>
>>>> Hey all,
>>>>
>>>> Having not received any feed back at all, I went ahead and did the
>>>> changes anyway. Everything seems to run and work fine for sunxi and
>>>> prints proper sizes.
>>>>
>>>> For the other boards, I tried to run a MAKEALL but there where so many
>>>> random other warnings I can't say for 100% certainty there where no
>>>> mistakes that crept in.
>>>
>>> There cannot possibly be a single warning if you're working from an
>>> official U-Boot repo, as warnings are considered failures and thus no
>>> patch reaches u-boot/master if it causes a warning.
>>
>> That might be the theory, but in practice this is simply false.
>> Different toolchains produce different warnings, and not all patches
>> always get test-built on every target (especially on obscure
>> architectures).  And since it's false that no warnings exist, that means
>> sometimes even when a patch is test-built, some newly introduced
>> warnings get missed (I got an e-mail pointing out such an occurance just
>> today).
>
> You are correct that the same code may or may not emit warnings
> depending on the toolchain, and that U-Boot's build system won't stop
> building because of warnings.
>
> However, when a new toolchain version causes such warnings, but they
> are not 'random' in any case; they may be numerous though, if in some
> source code used in a lot of boards.
>
> In any case, if Oliver gets warnings, chances are we'll get them to
> when applying his code, in which case it'll be rejected, or we'll see
> them happening later if he's unsing a common toolchain in a new
> version, or he's using an unusual toolchain.
I wasn't getting warnings or errors even remotely related to my patches 
and while it's a pitty we are discussing peanuts without even looking at 
the patch, cest la vie.

A few of the warnings are from my toolchain (missing bits) a few others 
from random stuff, here a few one liners to give an indication where 
boards break:

mxc_gpio.c:105:9: error: dereferencing pointer to incomplete type

at91rm9200_devices.c:64:20: error: ?AT91_PIO_PORTA? undeclared (first 
use in this function)

mini2440.c:70:24: error: ?GPH8? undeclared (first use in this function

/silo/build/sunxi-bsp/u-boot-sunxi/include/config.h:7:0: warning: 
"CONFIG_SYS_SOC" redefined [enabled by default]
/silo/build/sunxi-bsp/build/u-boot-all/include/config.h:9:0: note: this 
is the location of the previous definition
/silo/build/sunxi-bsp/u-boot-sunxi/include/config.h:8:0: warning: 
"CONFIG_BOARDDIR" redefined [enabled by default]
/
tons of errors on this one for the atmel at91sam configs

at91sam9260_devices.c:34:20: error: ?AT91_PIO_PORTB? undeclared (first 
use in this function)

da8xx_gpio.c:388:1: error: dereferencing pointer to incomplete type

dm355leopard.c:35:2: error: ?DAVINCI_GPIO_BINTEN? undeclared (first use 
in this function)
dm355leopard.c:38:2: error: dereferencing pointer to incomplete type

I must admit however, quite a few boards built cleanly, so I may have 
overstated things?

I'm building using gcc-4.6.3 on gentoo (with gcc build natively via 
cross-dev)
The command I used was:
CROSS_COMPILE=arm-pc-linux-gnueabi- 
BUILD_DIR=/silo/build/sunxi-bsp/build/u-build-all ./MAKEALL -a arm

So while a few of these errors might be long fixed, we merge the u-boot 
patches on a monthly or so basis, I can't imagine all these errors being 
from the wrong toolchain?

So now that that's settled, anything fundamentally wrong with my patch? :)

oliver
>
>> -Scott
>
> Amicalement,
>

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Scott Wood]

On Fri, 2013-10-18 at 02:04 +0200, Oliver Schinagl wrote:
> So now that that's settled, anything fundamentally wrong with my patch? :)

Did you see my other mail in this thread?  This patch is sort of OK for
raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
risk of false positives from I/O), but it can't go beyond that on
32-bit.  A better approach would be to get the RAM size from the memory
controller, which is what we do on many Freescale PPC boards.

-Scott

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Wolfgang Denk]

Dear Scott Wood,

In message <1382114601.7979.843.camel@snotra.buserror.net> you wrote:
>
> Did you see my other mail in this thread?  This patch is sort of OK for
> raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
> risk of false positives from I/O), but it can't go beyond that on
> 32-bit.  A better approach would be to get the RAM size from the memory
> controller, which is what we do on many Freescale PPC boards.

This is NOT a better approach.  Reading the memory controller just
tells you what is supposed to be there, i. e. what you programmed into
the controller.  get_ram_size() shows you what is _actually_ there,
which may be a totally different thing, for example when different RAM
chips can be fit on the board, or when the working area of the RAM is
not the same as the actual chip size, for example due to hardware
errors (shorts or interruptions on the address lines, etc.).

get_ram_size() is a very efficient memory test that detects 95% or
more of all RAM related hardware issues.

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 know engineers. They love to change things.             - Dr. McCoy

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Scott Wood]

On Fri, 2013-10-18 at 22:26 +0200, Wolfgang Denk wrote:
> Dear Scott Wood,
> 
> In message <1382114601.7979.843.camel@snotra.buserror.net> you wrote:
> >
> > Did you see my other mail in this thread?  This patch is sort of OK for
> > raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
> > risk of false positives from I/O), but it can't go beyond that on
> > 32-bit.  A better approach would be to get the RAM size from the memory
> > controller, which is what we do on many Freescale PPC boards.
> 
> This is NOT a better approach.  Reading the memory controller just
> tells you what is supposed to be there, i. e. what you programmed into
> the controller.  get_ram_size() shows you what is _actually_ there,

That may be useful with simpler memory controllers where there isn't
much to get wrong other than size, but on modern DDR controllers you're
pretty screwed anyway if you don't know what's actually there.  If you
don't get the size right, what are the odds you got the timing right?
We hard code a lot of other things in U-Boot such as the address of
various I/O...

And if RAM is socketed (or the board designer was nice enough to include
it with non-socketed RAM), the information should be coming from SPD
EEPROMs, not hardcoded in the U-Boot image.

> which may be a totally different thing, for example when different RAM
> chips can be fit on the board, or when the working area of the RAM is
> not the same as the actual chip size, for example due to hardware
> errors (shorts or interruptions on the address lines, etc.).
> 
> get_ram_size() is a very efficient memory test that detects 95% or
> more of all RAM related hardware issues.

So call it something like test_ram_simple() and bound it by the maximum
amount of RAM that you expect to be there, so you don't accidentally
touch I/O.  For RAM beyond 2G, either leave it untested or use the MMU
to test it, but don't tell the rest of the system that RAM beyond that
doesn't exist.  For RAM that ends on a non-power-of-2 (below 2G), do one
final test at the end of the supplied size.

And if the test finds missing/bad RAM below the supplied limit, the
caller should generally make noise that there's a problem rather than
just use less RAM.

-Scott

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Wolfgang Denk]

Dear Scott,

In message <1382130258.7979.896.camel@snotra.buserror.net> you wrote:
>
> > This is NOT a better approach.  Reading the memory controller just
> > tells you what is supposed to be there, i. e. what you programmed into
> > the controller.  get_ram_size() shows you what is _actually_ there,
> 
> That may be useful with simpler memory controllers where there isn't
> much to get wrong other than size, but on modern DDR controllers you're
> pretty screwed anyway if you don't know what's actually there.  If you

The standard approach here is to go through all expected memory
configurations...

> don't get the size right, what are the odds you got the timing right?
> We hard code a lot of other things in U-Boot such as the address of
> various I/O...

You sound as if you were proud of that?  I don't think this is any
special kind of "acchievement".

> And if RAM is socketed (or the board designer was nice enough to include
> it with non-socketed RAM), the information should be coming from SPD
> EEPROMs, not hardcoded in the U-Boot image.

Agreed - but then, still, get_ram_size() is a pretty efficient test if
your memory is actually working as expected.  Even DIMMs with SPD
EEPROMs tend to fail every now and then.  And when you know you have
fit a 1 GB module, and U-Boot recognizes only 128 MB, you might start
to think...

> > get_ram_size() is a very efficient memory test that detects 95% or
> > more of all RAM related hardware issues.
> 
> So call it something like test_ram_simple() and bound it by the maximum

Actually the _primary_ function is the memory sizing.  The testimg is
just a pleasant side effect.

> amount of RAM that you expect to be there, so you don't accidentally
> touch I/O.  For RAM beyond 2G, either leave it untested or use the MMU
> to test it, but don't tell the rest of the system that RAM beyond that
> doesn't exist.  For RAM that ends on a non-power-of-2 (below 2G), do one
> final test at the end of the supplied size.

Patches welcoem :-)

> And if the test finds missing/bad RAM below the supplied limit, the
> caller should generally make noise that there's a problem rather than
> just use less RAM.

Well, it reports the working RAM size.  You can check that, if you
like.

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
A wise man asks himself the reason for his  mistakes,  while  a  fool
will ask others.

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Oliver Schinagl]

On 10/18/13 22:26, Wolfgang Denk wrote:
> Dear Scott Wood,
>
> In message <1382114601.7979.843.camel@snotra.buserror.net> you wrote:
>>
>> Did you see my other mail in this thread?  This patch is sort of OK for
>> raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
>> risk of false positives from I/O), but it can't go beyond that on
>> 32-bit.  A better approach would be to get the RAM size from the memory
>> controller, which is what we do on many Freescale PPC boards.
>
> This is NOT a better approach.  Reading the memory controller just
> tells you what is supposed to be there, i. e. what you programmed into
> the controller.  get_ram_size() shows you what is _actually_ there,
> which may be a totally different thing, for example when different RAM
> chips can be fit on the board, or when the working area of the RAM is
> not the same as the actual chip size, for example due to hardware
> errors (shorts or interruptions on the address lines, etc.).
>
> get_ram_size() is a very efficient memory test that detects 95% or
> more of all RAM related hardware issues.
But is my patch acceptable and does it fix the phys_size_t = long vs 
phys_size_t = unsigned long 'issue'. Does this patch fix that or make it 
worse? And if so, how would that needed to be fixed.

Oliver
>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Oliver Schinagl]

On 10/18/13 18:43, Scott Wood wrote:
> On Fri, 2013-10-18 at 02:04 +0200, Oliver Schinagl wrote:
>> So now that that's settled, anything fundamentally wrong with my patch? :)
>
> Did you see my other mail in this thread?  This patch is sort of OK for
Sorry I did and I got distracted from it.

> raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
> risk of false positives from I/O), but it can't go beyond that on
I'd ask 'how so' but I'm not sure I'd understand anyway ;)

> 32-bit.  A better approach would be to get the RAM size from the memory
> controller, which is what we do on many Freescale PPC boards.
Not possible for us at this moment. The memory controller is programed 
with hard-coded values on a per board basis. I think we could 
technically obtain values via/from the memory controller, but have no 
knowledge at this moment. Allwinner has a tool, livesuit, which is used 
to flash full disk images to a device. We currently guesstimate that 
livesuit can somehow detect the memory parameters and injects it into 
the stock bootloader. But we really have no clue if that really happens 
or how it's done. So we rely on extracting the information from a 
running stock android/linux and hardcode it into u-boot.

Oliver

>
> -Scott
>
>
>

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Scott Wood]

On Sat, 2013-10-19 at 01:07 +0200, Oliver Schinagl wrote:
> On 10/18/13 18:43, Scott Wood wrote:
> > On Fri, 2013-10-18 at 02:04 +0200, Oliver Schinagl wrote:
> >> So now that that's settled, anything fundamentally wrong with my patch? :)
> >
> > Did you see my other mail in this thread?  This patch is sort of OK for
> Sorry I did and I got distracted from it.
> 
> > raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
> > risk of false positives from I/O), but it can't go beyond that on
> I'd ask 'how so' but I'm not sure I'd understand anyway ;)

Do you mean why it can't go beyond 2 GiB?  The next address to probe
after 0x8000_0000 would be 0x1_0000_0000 which is beyond what can be
addressed in a 32-bit environment.  I suppose you could return 4 GiB if
0x8000_0000 tests OK, but nothing beyond that.  You'd need a larger
datatype than "unsigned long" if you want to return 4 GiB, though.

And the one 64-bit environment that we're about to have in U-Boot
(armv8) has discontiguous memory, which is another case where
get_ram_size() won't work.

> > 32-bit.  A better approach would be to get the RAM size from the memory
> > controller, which is what we do on many Freescale PPC boards.
> Not possible for us at this moment. The memory controller is programed 
> with hard-coded values on a per board basis. I think we could 
> technically obtain values via/from the memory controller, but have no 
> knowledge at this moment. Allwinner has a tool, livesuit, which is used 
> to flash full disk images to a device. We currently guesstimate that 
> livesuit can somehow detect the memory parameters and injects it into 
> the stock bootloader. But we really have no clue if that really happens 
> or how it's done. So we rely on extracting the information from a 
> running stock android/linux and hardcode it into u-boot.

So the issue is that you don't have documentation on what the values you
program into the memory controller mean?  Can you extract the memory
size as well from a running stock image?

BTW, shouldn't get_ram_size restore the original data in the final
"return (maxsize)" case?  I know, patches welcome. :-)

-Scott

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Scott Wood]

On Fri, 2013-10-18 at 18:25 -0500, Scott Wood wrote:
> On Sat, 2013-10-19 at 01:07 +0200, Oliver Schinagl wrote:
> > On 10/18/13 18:43, Scott Wood wrote:
> > > On Fri, 2013-10-18 at 02:04 +0200, Oliver Schinagl wrote:
> > >> So now that that's settled, anything fundamentally wrong with my patch? :)
> > >
> > > Did you see my other mail in this thread?  This patch is sort of OK for
> > Sorry I did and I got distracted from it.
> > 
> > > raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
> > > risk of false positives from I/O), but it can't go beyond that on
> > I'd ask 'how so' but I'm not sure I'd understand anyway ;)
> 
> Do you mean why it can't go beyond 2 GiB?  The next address to probe
> after 0x8000_0000 would be 0x1_0000_0000 which is beyond what can be
> addressed in a 32-bit environment.  I suppose you could return 4 GiB if
> 0x8000_0000 tests OK, but nothing beyond that.  You'd need a larger
> datatype than "unsigned long" if you want to return 4 GiB, though.

Oh, and if you actually had 4 GiB of RAM mapped in a 32-bit environment,
where would I/O go?

-Scott

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Oliver Schinagl]

On 10/19/13 01:25, Scott Wood wrote:
> On Fri, 2013-10-18 at 18:25 -0500, Scott Wood wrote:
>> On Sat, 2013-10-19 at 01:07 +0200, Oliver Schinagl wrote:
>>> On 10/18/13 18:43, Scott Wood wrote:
>>>> On Fri, 2013-10-18 at 02:04 +0200, Oliver Schinagl wrote:
>>>>> So now that that's settled, anything fundamentally wrong with my patch? :)
>>>>
>>>> Did you see my other mail in this thread?  This patch is sort of OK for
>>> Sorry I did and I got distracted from it.
>>>
>>>> raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
>>>> risk of false positives from I/O), but it can't go beyond that on
>>> I'd ask 'how so' but I'm not sure I'd understand anyway ;)
>>
>> Do you mean why it can't go beyond 2 GiB?  The next address to probe
>> after 0x8000_0000 would be 0x1_0000_0000 which is beyond what can be
>> addressed in a 32-bit environment.  I suppose you could return 4 GiB if
>> 0x8000_0000 tests OK, but nothing beyond that.  You'd need a larger
>> datatype than "unsigned long" if you want to return 4 GiB, though.
>
> Oh, and if you actually had 4 GiB of RAM mapped in a 32-bit environment,
> where would I/O go?
Well you need 4 GiB of address-space don't you? If you have 2 GiB of 
ram, the other 2 GiB is used as register-space isn't it. So to support 2 
GiB of ram you need 4 GiB of address sapce. Granted having more then 2 
GiB of ram is highly unlikly as you probably can't get ramsizes that 
would fit. But the bug exists with 2 GiB, get_ram_size in its current 
form, which triggered me into fixing it. get_ram_size reports negative 
ram on our 2 GiB board. Obviously incidentally it doesn't hugely matter 
because ramsize = get_ram_size(); you put a long inside an unsigned long 
and it gets automagically fixed.

My point was simply get_ram_size is bugged, it cramps signed long into 
an unsigned long, and I tried to fix that. A side effect is that upto 4 
GiB of address space is fixed. :)

Oliver
>
> -Scott
>
>
>

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Oliver Schinagl]

On 10/19/13 01:25, Scott Wood wrote:
> On Sat, 2013-10-19 at 01:07 +0200, Oliver Schinagl wrote:
>> On 10/18/13 18:43, Scott Wood wrote:
>>> On Fri, 2013-10-18 at 02:04 +0200, Oliver Schinagl wrote:
>>>> So now that that's settled, anything fundamentally wrong with my patch? :)
>>>
>>> Did you see my other mail in this thread?  This patch is sort of OK for
>> Sorry I did and I got distracted from it.
>>
>>> raising the get_ram_size() limit from 1 GiB to 2 GiB (with an increased
>>> risk of false positives from I/O), but it can't go beyond that on
>> I'd ask 'how so' but I'm not sure I'd understand anyway ;)
>
> Do you mean why it can't go beyond 2 GiB?  The next address to probe
> after 0x8000_0000 would be 0x1_0000_0000 which is beyond what can be
Yeah why can't it go beyond 2 GiB. It should be an unsigned long, so it 
should be able to go beyond 2 GiB, as you state, upto 4 GiB on a 32-bit 
environment.

What my patch fixes is, that u-boot passes ram parameters (ramsize) via 
the global data -> ramsize. Ramsize is a phys_size_t which is defined as 
unsigned long (yay for typedef :p). Nearly all platforms pass the memory 
size to the linux kernel as gd->ramsize = get_ram_size();

And here is the bug, (unsigned long)ramsize = (signed long)get_ram_size();

 > addressed in a 32-bit environment.  I suppose you could return 4 GiB > if
 > 0x8000_0000 tests OK, but nothing beyond that.  You'd need a larger
 > datatype than "unsigned long" if you want to return 4 GiB, though.
As for returning exactly 4 GiB, assuming that register space is 0 bytes 
(impossible but lets just say for arguments sake) we now have 
0x8000_0000 addresses available, so exactly 4 GiB. If for whatever 
reason it ends up being only 4 GiB -1 byte, I don't think anybody will 
care/notice (but it would have to be taken into account I suppose?

>
> And the one 64-bit environment that we're about to have in U-Boot
> (armv8) has discontiguous memory, which is another case where
> get_ram_size() won't work.
So get_ram_size() needs a brother, get_discont_ram_size? :)
>
>>> 32-bit.  A better approach would be to get the RAM size from the memory
>>> controller, which is what we do on many Freescale PPC boards.
>> Not possible for us at this moment. The memory controller is programed
>> with hard-coded values on a per board basis. I think we could
>> technically obtain values via/from the memory controller, but have no
>> knowledge at this moment. Allwinner has a tool, livesuit, which is used
>> to flash full disk images to a device. We currently guesstimate that
>> livesuit can somehow detect the memory parameters and injects it into
>> the stock bootloader. But we really have no clue if that really happens
>> or how it's done. So we rely on extracting the information from a
>> running stock android/linux and hardcode it into u-boot.
>
> So the issue is that you don't have documentation on what the values you
> program into the memory controller mean?  Can you extract the memory
> size as well from a running stock image?
Sort of, we bus-width, io-size and chip density, from those values we 
determine the chip-size/ram-size we can't exctract the actual number.

That said, io-size I think (or was it bus-width?) while programmed into 
the ram controller, isn't even highly important, it is expected that it 
is used for drive strength, 2 chips vs 4 chips, but physical examination 
of the tablet/board helps here.

Oliver
>
> BTW, shouldn't get_ram_size restore the original data in the final
> "return (maxsize)" case?  I know, patches welcome. :-)
>
> -Scott
>
>
>

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Tom Rini]

On Sat, Oct 19, 2013 at 11:07:54AM +0200, Oliver Schinagl wrote:

[snip]
> What my patch fixes is, that u-boot passes ram parameters (ramsize)
> via the global data -> ramsize. Ramsize is a phys_size_t which is
> defined as unsigned long (yay for typedef :p). Nearly all platforms
> pass the memory size to the linux kernel as gd->ramsize =
> get_ram_size();
> 
> And here is the bug, (unsigned long)ramsize = (signed long)get_ram_size();

Well, here's where we have a few related problems.  We need to:
a) Know how much memory U-Boot has available to it, to work with (for
relocation, image loading, etc, etc)
b) Tell an OS how much memory exists via ATAGs or Device Tree.

(b) is what gets complicated because the rule of thumb U-Boot has had
forever is to update the device tree to have "correct" memory
information.  A number of PowerPC boards have dtb files with 0x0 for
size, for auto-detect in U-Boot.  A number of ARM boards just have
128MiB size and then work well in Linux as we fixup that incorrect
amount to the larger real value.

We need a way to add in support for case (c) of "Device Tree has correct
memory information, DO NOT TOUCH".  This has been talked about before
but what I said in (b) complicates things.  We may way to see if for
example the DT indicates a size beyond what we can determine and if so,
just go with it (and perhaps a debug() print about not touching).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131019/857b70b5/attachment.pgp>

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Wolfgang Denk]

Dear Scott Wood,

In message <1382138723.7979.928.camel@snotra.buserror.net> you wrote:
>
> And the one 64-bit environment that we're about to have in U-Boot
> (armv8) has discontiguous memory, which is another case where
> get_ram_size() won't work.

get_ram_size() is supposed to be run per memory bank.  If you have
discontiguous memory, then you probably have several memory banks that
can be sized separately?

> BTW, shouldn't get_ram_size restore the original data in the final
> "return (maxsize)" case?  I know, patches welcome. :-)

Yes, get_ram_size() is non-destructive (at least in the no-error
case; otherwise things like PRAM would not 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
I think there's a world market for about five computers.
         -- attr. Thomas J. Watson (Chairman of the Board, IBM), 1943

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Olliver Schinagl]

Hey all,

*ping*

On 10/21/2013 09:44 PM, Wolfgang Denk wrote:
> Dear Scott Wood,
>
> In message <1382138723.7979.928.camel@snotra.buserror.net> you wrote:
>>
>> And the one 64-bit environment that we're about to have in U-Boot
>> (armv8) has discontiguous memory, which is another case where
>> get_ram_size() won't work.
>
> get_ram_size() is supposed to be run per memory bank.  If you have
> discontiguous memory, then you probably have several memory banks that
> can be sized separately?
>
>> BTW, shouldn't get_ram_size restore the original data in the final
>> "return (maxsize)" case?  I know, patches welcome. :-)
>
> Yes, get_ram_size() is non-destructive (at least in the no-error
> case; otherwise things like PRAM would not work).
Is there anything else that needs to be cleaned up in the patch I 
submitted back then (other then re-basing it I suppose).

With all the sunxi stuff slowly being cleaned up, this patch came to 
mind again and I was just wondering if anything needs to be done to get 
it merged. There was quite a big discussion about get_ram_size() in 
general, but nobody ever said if the signed long -> unsigned long was a 
good fix.

Olliver

>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [RFC] ARM: U-boot and 2 GiB of ram with get_ram_size only being long

[Scott Wood]

On Thu, 2013-10-03 at 23:15 +0200, Oliver Schinagl wrote:
> Hey all,
> 
> I just yesterday received my CubieTruck (cubieboard3) with 2 GiB of Ram 
> and added support for it to the sunxi-u-boot branch. While I know this 
> isn't merged into the main u-boot tree (yet), I ran into the following 
> problem.
> 
> At the end of the dram init code, it is customary to call get_ram_size() 
> and return its value. This is then used to print the DRAM size and also 
> is passed to the Linux kernel.
> 
> However the return size of get_ram_size() is a long. While I don't 
> understand why not unsigned long or even u64 was chosen, this causes 
> get_dram_size to overflow when having a ramsize of 2 GiB. While only 
> printing of the value isn't hugely important, this does indicate u-boot 
> seems to be somewhat artificially limited to 2 GiB of Ram? This only 
> seems to affect the SPL as, if I understood correctly, there it stores 
> the ram_size into the gd struct which I think is unsigned long.
> 
> I've started working on a patch to convert common/memsize.c's 
> get_ram_size(), to be completely unsigned long, however there seems to 
> be quite a lot of code that calls this. So my question is now before 
> going over all drivers and change that and submit a big patch-set, did I 
> overlook anything and are my conclusions correct, get_ram_size should 
> return unsigned long.

get_ram_size() is inherently incapable of dealing with memory that is
too large to be mapped all at once.  It also would have a hard time with
2GiB RAM, because it sizes by powers of two, so the next address to be
checked would be at 4GiB which cannot be mapped on 32-bit (plus, at such
large RAM sizes you run a significant risk of a false positive by
hitting I/O).

Can't you get the RAM size from the memory controller config instead?

> Finally, a long is 32 bits on x86 and armv7, but how will that relate to 
> 64bits armv8? As I understood, Windows treats long's as 4 bytes no 
> matter if it's 32 bit or 64 bit. Linux is better and a long is 4 bytes 
> on 32 bits, and 8 bytes on 64 bits versions. So how will u-boot work on 
> armv8? Will the long datatype work well, or should I consider changing 
> things more future proof? (u32 and u64 come to mind).

U-Boot on armv8 follows an LP64 model just like Linux.

-Scott