[U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Mon May 14 15:56:27 UTC 2018
I had a bit more time to look into this and it looks as if we have two problem-spots...
First, there's a type-mismatch between ram_info.size (a size_t) and gd.ram_size (phys_size_t).
While we can increase the size of a phys_size_t to 64bit (by defining CONFIG_PHYS_64BIT),
the size_t will always be an unsigned int on a 32bit arm architecture. So here’s one possible
pitfall that should be resolved.
Once this is adjusted, we might just increase the width of ram_info.size to 64bit by enabling
CONFIG_PHYS_64BIT … however, this comes with a caveat: the default cell sizes for the
FDT (via fdtdec) also increases. I.e. if any come in our arch or the drivers still goes through
the fdtdec-functions, we’ll end up with a problem.
As my test coverage is limited to 64bit targets, I can’t tell whether defining the PHYS_64BIT
configuration would be possible for the RK3288 — if it is, we’d have a rather easy way forward
and could reuse the phys_size_t for ram_info.size.
I’d appreciate if you could take a look at whether CONFIG_PHYS_64BIT gets us into a lot
of trouble on the RK3288 or whether this will trigger just a few minor adjustments…
Thanks,
Philipp.
> On 9 May 2018, at 09:24, Dr. Philipp Tomsich <philipp.tomsich at theobroma-systems.com> wrote:
>
>>
>> On 9 May 2018, at 07:29, Marty E. Plummer <hanetzer at startmail.com> wrote:
>>
>> On Tue, May 08, 2018 at 11:08:14PM +0200, Dr. Philipp Tomsich wrote:
>>>
>>>> On 8 May 2018, at 21:21, Marty E. Plummer <hanetzer at startmail.com> wrote:
>>>>
>>>> On Tue, May 08, 2018 at 12:21:24PM +0200, Dr. Philipp Tomsich 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.
>>>> Yeah, I was talking about this with Marek on irc yesterday, I had the
>>>> same conclusion. However, being very new and inexperienced with u-boot
>>>> development in general, I'm a bit averse to to messing with what appears
>>>> to be a global header file, as it can easily break any other board which
>>>> uses it.
>>>
>>> No problem ??? let???s wait for Simon???s input and I???ll create the patchset.
>>> I???ll need your help testing, as I don???t have any 32bit platforms w/ 4GB??? and I???ll
>>> let you know once something is ready for you to test.
>>>
>> Even if we do change ram_info.size into a larger, 64-bit variable, we
>> should still probably use that min call since with a 32-bit address
>> space, even with 4GiB ram physically, the rk3288 can only address
>> 0xff00000 of ram, which is just a tad shy of the full 4GiB.
>
> This is not the meaning of ram_info.size, though… for the addressability there’s
> the ‘board_get_usable_ram_top’ hook used today.
>
> Unfortunately, there’s no formal definition (i.e. clear documentation of this in a header
> file) of board_get_usable_ram_top; however, the default (weak) implementation in
> board_f hints at the intent to use it to clip the end if the ’size’ field exceeds the
> addressable space:
> /*
> * Detect whether we have so much RAM that it goes past the end of our
> * 32-bit address space. If so, clip the usable RAM so it doesn't.
> */
>
> This is also consistent with usage on Tegra and a few others.
> Until someone decides that ram_info should not hold the DRAM controller’s view,
> but rather consider addressability by the CPU, I strongly disagree with applying
> the carveout for the MMIO this early.
>
> Also note that just because this part of memory is not addressable from the CPUs
> does not necessarily mean that it is not addressable from one of the DMA controllers
> (I don’t know enough about the internal implementation of the address decode logic
> in this SoC to determine whether that part of memory is really unaccessible or if it
> is merely “special”).
>
>>>>>
>>>>> 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>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list