[U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Tue Jul 10 18:55:18 UTC 2018
> On 10 Jul 2018, at 16:41, Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
> On 8 May 2018 at 04:21, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich at theobroma-systems.com>> wrote:
>> Marty,
>>
>>> On 8 May 2018, at 02:52, Marty E. Plummer <hanetzer at 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 <hanetzer at 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 <hanetzer at 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 at ff0c0000: 1, dwmmc at ff0d0000: 2, dwmmc at 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.
>
> Sorry I missed this. Yes changing ram.h is fine with me. In fact I
> thought that size_t would be OK. Clearly not.
Ok, I’ll try to get this done over the next few days.
> Regards,
> Simon
More information about the U-Boot
mailing list