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

[Simon Glass]

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?

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

Regards,
Simon