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

From: Stephen Warren <swarren@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@nvidia.com>
---
 arch/arm/mach-tegra/board.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Hi Stephen,

On 7 August 2015 at 16:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@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@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?

Also why do you use CONFIG_ARM64 for the second one and
CONFIG_PHYS_64BIT for the first?

Regards,
Simon

On 08/09/2015 09:07 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 7 August 2015 at 16:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@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@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.

On 10 August 2015 at 10:10, Stephen Warren <swarren@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@wwwdotorg.org> wrote:
>>>
>>> From: Stephen Warren <swarren@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@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@chromium.org>