On 27 Jul 2018, at 09:50, Carlo Caione carlo.caione@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@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.