> On 27 Jul 2018, at 09:50, Carlo Caione <carlo.cai...@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 <ca...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
