[U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems
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'). Signed-off-by: Philipp Tomsich --- arch/arm/include/asm/arch-rockchip/sdram_common.h | 2 +- arch/arm/mach-rockchip/sdram_common.c | 8 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/arm/include/asm/arch-rockchip/sdram_common.h b/arch/arm/include/asm/arch-rockchip/sdram_common.h index 671c318..edf5911 100644 --- a/arch/arm/include/asm/arch-rockchip/sdram_common.h +++ b/arch/arm/include/asm/arch-rockchip/sdram_common.h @@ -50,7 +50,7 @@ #define SYS_REG_DBW_MASK 3 /* Get sdram size decode from reg */ -size_t rockchip_sdram_size(phys_addr_t reg); +u64 rockchip_sdram_size(phys_addr_t reg); /* Called by U-Boot board_init_r for Rockchip SoCs */ int dram_init(void); diff --git a/arch/arm/mach-rockchip/sdram_common.c b/arch/arm/mach-rockchip/sdram_common.c index 650d53e..6bad537 100644 --- a/arch/arm/mach-rockchip/sdram_common.c +++ b/arch/arm/mach-rockchip/sdram_common.c @@ -11,11 +11,11 @@ #include DECLARE_GLOBAL_DATA_PTR; -size_t rockchip_sdram_size(phys_addr_t reg) +u64 rockchip_sdram_size(phys_addr_t reg) { u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4; - size_t chipsize_mb = 0; - size_t size_mb = 0; + u64 chipsize_mb = 0; + u64 size_mb = 0; u32 ch; u32 sys_reg = readl(reg); @@ -48,7 +48,7 @@ size_t rockchip_sdram_size(phys_addr_t reg) rank, col, bk, cs0_row, bw, row_3_4); } - return (size_t)size_mb << 20; + return (u64)size_mb << 20; } int dram_init(void) -- 2.1.4 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems
> On 26 Jul 2018, at 22:05, Carlo Caione 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]. > > Also I was wondering if we should also fix get_effective_memsize() and > gd->bd->bi_dram[].size Yes, we should. I missed that one… I only have RK3368 and RK3399 targets to test here, so your testing is very much appreciated. > > [0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa > > Cheers, > > -- > Carlo Caione signature.asc Description: Message signed with OpenPGP ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems
> On 26 Jul 2018, at 22:05, Carlo Caione 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]. > > Also I was wondering if we should also fix get_effective_memsize() and > gd->bd->bi_dram[].size > > [0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa Thanks for identifying these two… I’ll pick them up and include in the next series. > > Cheers, > > -- > Carlo Caione ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems
> On 26 Jul 2018, at 22:05, Carlo Caione 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. > > Also I was wondering if we should also fix get_effective_memsize() and > gd->bd->bi_dram[].size > > [0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa > > Cheers, > > -- > Carlo Caione ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems
On Fri, 2018-07-27 at 00:54 +0200, Dr. Philipp Tomsich wrote: > > On 26 Jul 2018, at 22:05, Carlo Caione 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 0x1 in my tests. -- Carlo Caione ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems
> On 27 Jul 2018, at 09:50, Carlo Caione wrote: > > On Fri, 2018-07-27 at 00:54 +0200, Dr. Philipp Tomsich wrote: >>> On 26 Jul 2018, at 22:05, Carlo Caione 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 0x1 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
Re: [U-Boot] [PATCH 2/9] rockchip: support 4GB DRAM on 32bit systems
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]. Also I was wondering if we should also fix get_effective_memsize() and gd->bd->bi_dram[].size [0]https://gist.github.com/carlocaione/b93cfd9ee71e07fcf68d5c02256ff0fa Cheers, -- Carlo Caione ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot