[U-Boot] [PATCH 2/3] ARM: tegra: query_sdram_size() cleanup

[Simon Glass]

On 10 August 2015 at 10:10, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 08/09/2015 09:07 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 7 August 2015 at 16:12, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> From: Stephen Warren <swarren at nvidia.com>
>>>
>>> The return value of query_sdram_size() is assigned directly to
>>> gd->ram_size in dram_init(). Adjust the return type to match the field
>>> it's assigned to. This has the beneficial effect that on 64-bit systems,
>>> the return value can correctly represent large RAM sizes over 4GB.
>>>
>>> For similar reasons, change the type of variable size_bytes in the same
>>> way.
>>>
>>> query_sdram_size() would previously clip the detected RAM size to at most
>>> just under 4GB in all cases, since on 32-bit systems, larger values could
>>> not be represented. Disable this feature on 64-bit systems since the
>>> representation restriction does not exist.
>>>
>>> On 64-bit systems, never call get_ram_size() to validate the detected/
>>> calculated RAM size. On any system with a secure OS/... carve-out, RAM
>>> may not have a single contiguous usable area, and this can confuse
>>> get_ram_size(). Ideally, we'd make this call conditional upon some other
>>> flag that indicates specifically that a carve-out is actually in use. At
>>> present, building for a 64-bit system is the best indication we have of
>>> this fact. In fact, the call to get_ram_size() is not useful by the time
>>> U-Boot runs on any system, since U-Boot (and potentially much other early
>>> boot software) always runs from RAM on Tegra, so any mistakes in memory
>>> controller register programming will already have manifested themselves
>>> and prevented U-Boot from running to this point. In the future, we may
>>> simply delete the call to get_ram_size() in all cases.
>>>
>>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>>> ---
>>>   arch/arm/mach-tegra/board.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-tegra/board.c b/arch/arm/mach-tegra/board.c
>>> index 40de72dc575f..b00e4b5c1e25 100644
>>> --- a/arch/arm/mach-tegra/board.c
>>> +++ b/arch/arm/mach-tegra/board.c
>>> @@ -66,10 +66,11 @@ bool tegra_cpu_is_non_secure(void)
>>>   #endif
>>>
>>>   /* Read the RAM size directly from the memory controller */
>>> -unsigned int query_sdram_size(void)
>>> +static phys_size_t query_sdram_size(void)
>>>   {
>>>          struct mc_ctlr *const mc = (struct mc_ctlr *)NV_PA_MC_BASE;
>>> -       u32 emem_cfg, size_bytes;
>>> +       u32 emem_cfg;
>>> +       phys_size_t size_bytes;
>>>
>>>          emem_cfg = readl(&mc->mc_emem_cfg);
>>>   #if defined(CONFIG_TEGRA20)
>>> @@ -77,6 +78,7 @@ unsigned int query_sdram_size(void)
>>>          size_bytes = get_ram_size((void *)PHYS_SDRAM_1, emem_cfg *
>>> 1024);
>>>   #else
>>>          debug("mc->mc_emem_cfg (MEM_SIZE_MB) = 0x%08x\n", emem_cfg);
>>> +#ifndef CONFIG_PHYS_64BIT
>>>          /*
>>>           * If >=4GB RAM is present, the byte RAM size won't fit into
>>> 32-bits
>>>           * and will wrap. Clip the reported size to the maximum that a
>>> 32-bit
>>> @@ -84,9 +86,12 @@ unsigned int query_sdram_size(void)
>>>           */
>>>          if (emem_cfg >= 4096) {
>>>                  size_bytes = U32_MAX & ~(0x1000 - 1);
>>> -       } else {
>>> +       } else
>>> +#endif
>>> +       {
>>>                  /* RAM size EMC is programmed to. */
>>> -               size_bytes = emem_cfg * 1024 * 1024;
>>> +               size_bytes = (phys_size_t)emem_cfg * 1024 * 1024;
>>> +#ifndef CONFIG_ARM64
>>>                  /*
>>>                   * If all RAM fits within 32-bits, it can be accessed
>>> without
>>>                   * LPAE, so go test the RAM size. Otherwise, we can't
>>> access
>>> @@ -97,6 +102,7 @@ unsigned int query_sdram_size(void)
>>>                  if (emem_cfg <= (0 - PHYS_SDRAM_1) / (1024 * 1024))
>>>                          size_bytes = get_ram_size((void *)PHYS_SDRAM_1,
>>>                                                    size_bytes);
>>> +#endif
>>>          }
>>>   #endif
>>>
>>> --
>>> 1.9.1
>>>
>>
>> You might consider using 'if IS_ENABLED()' instead of #ifdef. Or
>> perhaps you should create a board_64.c if the code going to be so
>> different?
>
>
> There's plenty of other code in the file that isn't ifdef'd, so I'd rather
> not split up the file. Also, putting duplicate copies into different files
> means duplicating the common parts. IS_ENABLED is useful for code coverage,
> but I think we have plenty of that for now, given that all paths are tested
> by building all Tegra boards, or even just a well picked pair:-).
>
>> Also why do you use CONFIG_ARM64 for the second one and
>> CONFIG_PHYS_64BIT for the first?
>
>
> The first chunk of code is purely based on the size used to represent a
> physical address, since it deals with overflow of the type used to store
> sizes. Hence, CONFIG_PHYS_64BIT. It should be quite legal (albeit silly) to
> use a 64-bit type to hold addresses/sizes irrespective of ARM architecture.
>
> The second chunk of code is more to do with whether we're running under a
> secure monitor or not. For now, the closest config option we have for that
> is CONFIG_ARM64, although I dare say we might need to introduce a new option
> to cover some 32-bit systems too, if we add support on e.g. Jetson TK1 for
> running under a secure monitor.

Reviewed-by: Simon Glass <sjg at chromium.org>