[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems

[Dr. Philipp Tomsich]

> On 27 Jul 2018, at 09:50, Carlo Caione <carlo.caione at gmail.com> wrote:
> 
> On Fri, 2018-07-27 at 00:54 +0200, Dr. Philipp Tomsich wrote:
>>> On 26 Jul 2018, at 22:05, Carlo Caione <carlo at endlessm.com> wrote:
>>> 
>>> On Thu, 2018-07-26 at 15:59 +0200, Philipp Tomsich wrote:
>>>> The calculation in `rockchip_sdram_size` would overflow for 4GB
>>>> on
>>>> 32bit systems (i.e. when PHYS_64BIT is not defined).
>>>> 
>>>> This makes the internal variables and the signature prototype use
>>>> a
>>>> u64 to ensure that we can represent the 33rd bit (as in '4GB').
>>>> 
>>> 
>>> Hi Philipp,
>>> just to let you know that this is still not working on the veyron
>>> jerry
>>> chromebook with 4GB I have here (RK3288). The boot stops at:
>>> 
>>> U-Boot SPL 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
>>> Trying to boot from SPI
>>> 
>>> U-Boot 2018.07-00403-gdbe6ef8276 (Jul 26 2018 - 17:21:11 +0100)
>>> 
>>> Model: Google Jerry
>>> DRAM:  0 Bytes
>>> 
>>> I'm still investigating why but for sure there are several points
>>> to
>>> fix to have a proper debug, see [0].
>> 
>> I reread ‘rockchip_sdram_size’ and it looks as if I forgot to mark
>> the value
>> shifted to create chipsize_mb as ULL.  When looking at this code I
>> had an
>> uneasy feeling that this might run into the C type rules (i.e. 1 will
>> be a 32bit
>> integer and shifting it by 32 would result in 0), but I didn’t think
>> that this
>> would ever be the case and that any 4GB DRAM setup would be made
>> up of multiple channels or of multiple ranks.
> 
> It doesn't hurt but rockchip_sdram_size() is returning the correct
> value of 0x100000000 in my tests.

The 'gd->bd->bi_dram[i].size’ path will also be involved and truncate this
later on, but I am not convinced that it’s worth fixing the dram banks info
for the general case.

Generally, typing for these bi_dram structures seems a mess: they are
often cast to 32bit and sometimes to 64bit in common code (i.e. fdtdec.c
uses unsigned long long).
I hope I can avoid touching this code.

Btw, here’s a gem from board_f.c (these two lines are after each other):
>         gd->ram_top = gd->ram_base + get_effective_memsize();
>         gd->ram_top = board_get_usable_ram_top(gd->mon_len);

As the first line is clearly deal (except the fact that the compiler can’t tell
that get_effective_memsize() should be side-effect free), we can drop it.
I’ll send a separate patch for this to be picked up by Tom…

—Phil.