[U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Tue May 8 10:21:24 UTC 2018
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.
Regards,
Philipp.
>>>>
>>>> Thanks,
>>>> - Kever
>>>>> }
>>>>>
>>>>> return (size_t)size_mb << 20;
>>>>
>>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de <mailto:U-Boot at lists.denx.de>
>>> https://lists.denx.de/listinfo/u-boot <https://lists.denx.de/listinfo/u-boot>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 529 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180508/ca31ec0f/attachment.sig>
More information about the U-Boot
mailing list