Simon, Looks like we’d like to have a 64bit type for size in ‘struct ram_info’ (see below). Let us know what you think of the proposed change.
Thanks, Philipp. > On 8 May 2018, at 12:21, Dr. Philipp Tomsich > <philipp.toms...@theobroma-systems.com> wrote: > > Marty, > >> On 8 May 2018, at 02:52, Marty E. Plummer <hanet...@startmail.com> wrote: >> >> On Mon, May 07, 2018 at 11:16:28AM +0200, Dr. Philipp Tomsich wrote: >>> >>>> On 7 May 2018, at 04:34, Marty E. Plummer <hanet...@startmail.com> wrote: >>>> >>>> On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote: >>>>> Hi Marty, >>>>> >>>>> >>>>> On 05/06/2018 10:25 PM, Marty E. Plummer wrote: >>>>>> Taken from coreboot's src/soc/rockchip/rk3288/sdram.c >>>>>> >>>>>> Without this change, my u-boot build for the asus c201 chromebook (4GiB) >>>>>> is incorrectly detected as 0 Bytes of ram. >>>>> >>>>> I know the root cause for this issue, and I have a local patch for it. >>>>> The rk3288 is 32bit, and 4GB size is just out of range, so we need to >>>>> before >>>>> the max size before return with '<<20'. Sorry for forgot to send it out. >>>>> >>>>>> >>>>>> Signed-off-by: Marty E. Plummer <hanet...@startmail.com> >>>>>> --- >>>>>> arch/arm/mach-rockchip/sdram_common.c | 62 ++++++++++++++++----------- >>>>>> 1 file changed, 37 insertions(+), 25 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/mach-rockchip/sdram_common.c >>>>>> b/arch/arm/mach-rockchip/sdram_common.c >>>>>> index 76dbdc8715..a9c9f970a4 100644 >>>>>> --- a/arch/arm/mach-rockchip/sdram_common.c >>>>>> +++ b/arch/arm/mach-rockchip/sdram_common.c >>>>>> @@ -10,6 +10,8 @@ >>>>>> #include <asm/io.h> >>>>>> #include <asm/arch/sdram_common.h> >>>>>> #include <dm/uclass-internal.h> >>>>>> +#include <linux/kernel.h> >>>>>> +#include <linux/sizes.h> >>>>>> >>>>>> DECLARE_GLOBAL_DATA_PTR; >>>>>> size_t rockchip_sdram_size(phys_addr_t reg) >>>>>> @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg) >>>>>> size_t size_mb = 0; >>>>>> u32 ch; >>>>>> >>>>>> - u32 sys_reg = readl(reg); >>>>>> - u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT) >>>>>> - & SYS_REG_NUM_CH_MASK); >>>>>> + if (!size_mb) { >>>>> >>>>> I don't understand this and follow up changes, we don't really need it, >>>>> isn't it? >>>>> I think don't need the changes before here. >>>> Yeah, that was just another level of indentation for the if (!size_mb) >>>> guard, but I've reworked the patch to not do that as it was pointed out >>>> that since size_mb is initialized to 0 prior. >>>>>> + /* >>>>>> + * we use the 0x00000000~0xfeffffff space >>>>>> + * since 0xff000000~0xffffffff is soc register space >>>>>> + * so we reserve it >>>>>> + */ >>>>>> + size_mb = min(size_mb, 0xff000000/SZ_1M); >>>>> >>>>> This is what we really need, as Klaus point out, we need to use >>>>> SDRAM_MAX_SIZE >>>>> instead of hard code. >>>> Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed, >>>> build and boot tested on my hardware. >>> >>> In that case you just masked the problem but didn???t solve it: assuming >>> size_mb >>> is size_t (I???ll assume this is 64bit, but did not check), then your 4GB >>> is 0x1_0000_0000 ) >>> which overflows to 0x0 when converted to a u32. >>> >>> In other words: we need to figure out where the truncation occurs (image >>> what >>> happens if a new 32bit processor with LPAE comes out???). >>> >> A very valid point. With the following patch to sdram_common.c and >> sdram_rk3288.c applied I get the debug output that follows it: >> diff --git a/arch/arm/mach-rockchip/sdram_common.c >> b/arch/arm/mach-rockchip/sdram_common.c >> index 232a7fa655..0fe69bf558 100644 >> --- a/arch/arm/mach-rockchip/sdram_common.c >> +++ b/arch/arm/mach-rockchip/sdram_common.c >> @@ -4,6 +4,7 @@ >> * SPDX-License-Identifier: GPL-2.0+ >> */ >> >> +#define DEBUG 1 >> #include <common.h> >> #include <dm.h> >> #include <ram.h> >> @@ -39,16 +40,19 @@ size_t rockchip_sdram_size(phys_addr_t reg) >> SYS_REG_ROW_3_4_MASK; >> >> chipsize_mb = (1 << (cs0_row + col + bk + bw - 20)); >> + debug("%s: %d: chipsize_mb %x\n", __func__, __LINE__, >> chipsize_mb); >> >> if (rank > 1) >> chipsize_mb += chipsize_mb >> (cs0_row - cs1_row); >> if (row_3_4) >> chipsize_mb = chipsize_mb * 3 / 4; >> size_mb += chipsize_mb; >> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb); >> debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n", >> rank, col, bk, cs0_row, bw, row_3_4); >> } >> >> + debug("%s: %d: size_mb %x\n", __func__, __LINE__, size_mb); >> size_mb = min(size_mb, SDRAM_MAX_SIZE/SZ_1M); >> >> return (size_t)size_mb << 20; >> diff --git a/drivers/ram/rockchip/sdram_rk3288.c >> b/drivers/ram/rockchip/sdram_rk3288.c >> index d99bf12476..9738eb088f 100644 >> --- a/drivers/ram/rockchip/sdram_rk3288.c >> +++ b/drivers/ram/rockchip/sdram_rk3288.c >> @@ -7,6 +7,7 @@ >> * Adapted from coreboot. >> */ >> >> +#define DEBUG 1 >> #include <common.h> >> #include <clk.h> >> #include <dm.h> >> >> --- >> U-Boot SPL 2018.05-rc3-02370-g309384e84b-dirty (May 07 2018 - 19:42:15 -0500) >> Trying to boot from SPI >> >> >> U-Boot 2018.05-rc3-02370-g309384e84b-dirty (May 07 2018 - 19:42:15 -0500) >> >> Model: Google Speedy >> DRAM: rockchip_sdram_size ff73009c 3c50dc50 >> rockchip_sdram_size: 42: chipsize_mb 400 >> rockchip_sdram_size: 49: size_mb 800 >> rank 2 col 11 bk 3 cs0_row 14 bw 2 row_3_4 0 >> rockchip_sdram_size: 42: chipsize_mb 400 >> rockchip_sdram_size: 49: size_mb 1000 >> rank 2 col 11 bk 3 cs0_row 14 bw 2 row_3_4 0 >> rockchip_sdram_size: 54: size_mb 1000 >> SDRAM base=0, size=fe000000 >> 4 GiB >> MMC: dwmmc@ff0c0000: 1, dwmmc@ff0d0000: 2, dwmmc@ff0f0000: 0 >> In: cros-ec-keyb >> Out: vidconsole >> Err: vidconsole >> Model: Google Speedy >> rockchip_dnl_key_pressed: adc_channel_single_shot fail! >> Net: Net Initialization Skipped >> No ethernet found. >> Hit any key to stop autoboot: 0 >> I guess we need to change the size_t to something larger; unless I'm >> mistaken, that's a 32 bit value, right? and 0x100000000 is at least 40 > > 4GB is actually the 33rd bit set and the lower 32bits cleared (i.e. the > largest > 32bit value “plus one”). > >> bits, unless I'm missing the issue here somewhere. However, that would >> take a change to include/ram.h, and would impact far more than just >> rk3288/rockchip devices across the board, so I'm unsure how to proceed. >> >> Use the min macro here for now, and begin work migrating the ram_info >> size member to a 64-bit container? > > The min() doesn’t make any sense here, as we implement the hook function > ‘board_get_usable_ram_top’ just a few lines later… > We are at the start of the merge window right now, so I’d rather hold off a > week (or two) and have a permanent solution than merging just a band-aid > now and then having the full fix come in later during the merge window. > > I briefly reviewed the situation yesterday and it looks like the size field in > ram_info is the culprit: it’s defined as ‘size_t’, which again is > __SIZE_TYPE__ > which again is ‘unsigned int’ on a (32bit) arm-*-eabi compiler. > > Expanding this to a phys_size_t won’t be doing us much good, either (as > that one will also be 32bits for the RK3288). > > The root cause of this is really that the RAM size and the ‘usable RAM’ are > two different concepts in U-Boot. On a 32bit physical address space with > memory-mapped peripherals, we can never have the full 4GB of DRAM as > we’ll also have some of the physical address-space set aside for the MMIO; > however, the MMIO range is only removed from the DRAM size when the > usable ram-top is evaluated… so the size can be 4GB after all and overflow > the 32bit size_t. Note that this separation into two different steps makes a > lot of sense, as processors might not use MMIO but specialised instructions > to access peripheral space—in which case there might indeed be a usable > memory of 4GB on a 32bit physical address space. > > From what I can tell, we’ll need to do two things: > (a) fix arch/arm/mach-rockchip/sdram_common.c to not use 32bit types > for the memory size > (b) touch ram.h to change the type of the ‘size’ field in ram_info (it needs > to be larger than 32bits > > I’d like Simon’s input (as he owns ram.h) and can create a patchset for this > change, if he agrees that this is the way forward. > > Regards, > Philipp. > >>>>> >>>>> Thanks, >>>>> - Kever >>>>>> } >>>>>> >>>>>> return (size_t)size_mb << 20; >>>>> >>>>> >>>> _______________________________________________ >>>> U-Boot mailing list >>>> U-Boot@lists.denx.de <mailto:U-Boot@lists.denx.de> >>>> <mailto:U-Boot@lists.denx.de <mailto:U-Boot@lists.denx.de>> >>>> https://lists.denx.de/listinfo/u-boot >>>> <https://lists.denx.de/listinfo/u-boot> >>>> <https://lists.denx.de/listinfo/u-boot >>>> <https://lists.denx.de/listinfo/u-boot>> _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot