[U-Boot] [PATCH 1/7] common: Display >=4GiB memory bank size

[Simon Glass]

Hi Bin,

On 30 July 2015 at 21:44, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jul 31, 2015 at 3:53 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 30 July 2015 at 04:49, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> bd->bi_dram[] has both start address and size defined as 32-bit,
>>> which is not the case on some platforms where >=4GiB memory bank
>>> is used. Change them to support such memory banks.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>> ---
>>>
>>>  common/board_f.c             | 2 +-
>>>  include/asm-generic/u-boot.h | 4 ++--
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/common/board_f.c b/common/board_f.c
>>> index 21be26f..a8ed6c8 100644
>>> --- a/common/board_f.c
>>> +++ b/common/board_f.c
>>> @@ -206,7 +206,7 @@ static int show_dram_config(void)
>>>         debug("\nRAM Configuration:\n");
>>>         for (i = size = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
>>>                 size += gd->bd->bi_dram[i].size;
>>> -               debug("Bank #%d: %08lx ", i, gd->bd->bi_dram[i].start);
>>> +               debug("Bank #%d: %016llx ", i, gd->bd->bi_dram[i].start);
>>
>> This makes all boards use an unreadable 16-character hex string. Can I
>> suggest we only use the larger value when the size demands it? A bit
>> messy but few things have >4GB memory so far.
>
> OK, we can do:
>
> if (gd->bd->bi_dram[i].start >= 0x100000000ULL)
>     debug("Bank #%d: %016llx ", i, (unsigned long
> long)(gd->bd->bi_dram[i].start));
> else
>     debug("Bank #%d: %08lx ", i, (unsigned long)(gd->bd->bi_dram[i].start));
>
> But this is debug output anyway, does that really matter?
>

I know, but I think it is worth it. Most platforms don't want this mess.

>>
>>>  #ifdef DEBUG
>>>                 print_size(gd->bd->bi_dram[i].size, "\n");
>>>  #endif
>>> diff --git a/include/asm-generic/u-boot.h b/include/asm-generic/u-boot.h
>>> index c918049..4e8b481 100644
>>> --- a/include/asm-generic/u-boot.h
>>> +++ b/include/asm-generic/u-boot.h
>>> @@ -130,8 +130,8 @@ typedef struct bd_info {
>>>         ulong           bi_boot_params; /* where this board expects params */
>>>  #ifdef CONFIG_NR_DRAM_BANKS
>>>         struct {                        /* RAM configuration */
>>> -               ulong start;
>>> -               ulong size;
>>> +               unsigned long long start;
>>
>> Probably should add a comment why we can't use phys_addr_t?
>
> I think we can use phys_addr_t.

On a 32-bit platform we might face an address beyond 4GB. But in that
case we would not be able to boot anyway. So yes I think you are
right.

>
>>
>>> +               phys_size_t size;
>>
>> Should this be ULL too?
>
> OK, we can change them to:
>
>     phys_addr_t start;
>     phys_size_t size;
>
> It this OK?

Yes.

>
>>
>>>         } bi_dram[CONFIG_NR_DRAM_BANKS];
>>>  #endif /* CONFIG_NR_DRAM_BANKS */
>>>  } bd_t;
>>> --
>
> Regards,
> Bin

Regards,
Simon