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

[Marek Vasut]

On 12/18/2017 01:16 PM, Patrick Brünn wrote:
>> From: Marek Vasut [mailto:marex at denx.de]
>> Sent: Montag, 18. Dezember 2017 12:52
>> 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 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.
>>
> It's #2 of this series:
> https://lists.denx.de/pipermail/u-boot/2017-December/314742.html

Ah, sorry, I missed that.

>>>>> 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?
>>
> => bdinfo
> arch_number = 0x00000000
> boot_params = 0x70000100
> DRAM bank   = 0x00000000
> -> start    = 0x70000000
> -> size     = 0x20000000
> DRAM bank   = 0x00000001
> -> start    = 0xB0000000
> -> size     = 0x20000000
> eth0name    = FEC
> ethaddr     = 00:01:05:23:87:85
> current eth = FEC
> ip_addr     = <NULL>
> baudrate    = 115200 bps
> TLB addr    = 0x8FFF0000
> relocaddr   = 0x8FF8B000
> reloc off   = 0x1878B000
> irq_sp      = 0x8F586960
> sp start    = 0x8F586950
> FB base     = 0x8F58C7C0
> Early malloc usage: 134 / 400
> fdt_blob = 8f586978

Looks fine then.

>> btw do you use SPL ? If not, you should ...
> I will read more about SPL. Until now, I thought I will only benefit,
> if my initial boot media is limited in size. As we boot from sdcard,
> that wasn't a problem to store 360k u-boot.

The other is that you can ie. skip the whole u-boot altogether and boot
linux directly.

I wonder if it would be better to keep the static variable and avoid
calling the get_ram_size() twice, it can save some CPU cycles. Besides
that, thanks for the explanation/discussion !1

-- 
Best regards,
Marek Vasut