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
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 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 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 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]. > > 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
[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
