[U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

[Marek Vasut]

On 12/18/2017 12:40 PM, Patrick Brünn wrote:
>> From: Marek Vasut [mailto:marex at denx.de]
>> Sent: Montag, 18. Dezember 2017 11:57
>> On 12/18/2017 11:47 AM, Patrick Brünn wrote:
>>>> From: Marek Vasut [mailto:marex at denx.de]
>>>> Sent: Montag, 18. Dezember 2017 10:17
>>>> On 12/18/2017 10:02 AM, linux-kernel-dev at beckhoff.com wrote:
>>>>> From: Patrick Bruenn <p.bruenn at beckhoff.com>
>>>>>
>>>>> Static variables are not available during board_init_f().
>>>>
>>>> They are, since the board runs from RAM at that point already.
>>>>
>>> From reading the README and common/board_f.c I got the impression,
>>> that dram_init() is called before it's save to access mx53_dram_size.
>>> And as that change seemed to cure the strange behavior I described
>>> in [1] and [2], I prepared a patch for my board, which ended up to be
>>> requested for m53evk and mx53loco, too.
>>
>> Technically yes, board_init_f means it runs from FLASH and has no or
>> minimal storage available. On the MX53 with SPL, it's running from RAM
>> so it's safe to use static variables too.
>>
> Thank's for your explanation.

No problem, it's a bit weird.

>>>>> 'static uint32_t mx53_dram_size[2];' was used in board specific
>>>>> dram_init(), dram_init_banksize() and get_effective_memsize() to avoid
>>>>> multiple calls to get_ram_size().
>>>>>
>>>>> Reused dram initialization functions from arch/arm/mach-
>>>> imx/mx5/mx53_dram.c
>>>>>
>>>>> Signed-off-by: Patrick Bruenn <p.bruenn at beckhoff.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2: None
>>>>>
>>>>>
>>>>> Only compile tested with make m53evk_defconfig
>>>>
>>>> Are you sure this patch retains the original functionality ? Note the
>>>> warning ...
>>>
>>> Effectively it changes:
>>>  -      return mx53_dram_size[0];
>>> +       return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);
>>>
>>> So, yes I am convinced that get_effective_memsize() still returns only the
>> size
>>> of the first dram bank.
>>
>> I suspect that will fails on M53 due to it's split-bank configuration.
>> The board has two DRAM banks with a hole between them.
>>
> This is how mx53_dram_size was initialized on m53 before that patch:
> -       mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);
> -       mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);
> 
> So to me that's, absolutely the same. With the only difference, that get_ram_size(bank0)
> is called multiple times, now.

The get_ram_size() above is called for two different bank (addresses),
not for bank0 twice. Maybe I'm missing something.

btw where's the patch adding mx53_dram.c ? I don't see it in mainline yet.

>>> However, I would be fine with just keeping the changes to my board
>> (cx9020).
>>> And if anyone has a better idea of what might be the root cause of [1] and
>> [2],
>>> I would absolute appreciate any input to that.
>>
>> If your board also has two DRAM banks with a hole between them and you
>> can test if this works fine, then I'm OK with this change.
>>
> Yes, cx9020 uses two 512MB banks with 1GB between PHYS_SDRAM_1/2, too.

Then that should be the same situation. Can you share "bdinfo" from that
board of yours?

btw do you use SPL ? If not, you should ...

[...]

-- 
Best regards,
Marek Vasut