Re: [PATCH 1/4] sam460ex: Revert change to SPD memory type for <= 128 MiB

[Markus Armbruster]

BALATON Zoltan <address@hidden> writes:

> On Tue, 21 Apr 2020, Markus Armbruster wrote:
>> BALATON Zoltan <address@hidden> writes:
>>> On Mon, 20 Apr 2020, Markus Armbruster wrote:
>>>> Requesting 32 or 64 MiB of RAM with the sam460ex machine type produces
>>>> a useless warning:
>>>>
>>>>    qemu-system-ppc: warning: Memory size is too small for SDRAM type, 
>>>> adjusting type
>>>
>>> Why is it useless? It lets user know there was a change so it could
>>> help debugging for example.
>>
>> The memory type is chosen by QEMU, not the user.  Why should QEMU warn
>> the user when it chooses DDR, but not when it chooses DDR2?
>>
>>>> This is because sam460ex_init() asks spd_data_generate() for DDR2,
>>>> which is impossible, so spd_data_generate() corrects it to DDR.
>>>
>>> This is correct and intended. The idea is that the board code should
>>> not need to know about SPD data, all knowledge about that should be in
>>> spd_data_genereate().
>>
>> I challenge this idea.
>>
>> The kind of RAM module a board accepts is a property of the board.
>> Modelling that in board code is sensible and easy.  Attempting to model
>> it in a one size fits all helper function is unlikely to work for all
>> boards.
>>
>> Apparently some boards (including malta) need two banks, so your helper
>> increases the number of banks from one to two, but only when that's
>> possible without changing the type.
>>
>> What if another board needs one bank?  Four?  Two even if that requires
>> changing the type?  You'll end up with a bunch of flags to drive the
>> helper's magic.  Not yet because the helper has a grand total of *two*
>> users, and much of its magic is used by neither, as demonstrated by
>> PATCH 2.
>>
>> If you want magic, have a non-magic function that does exactly what it's
>> told, and a magic one to tell it what to do.  The non-magic one will be
>> truly reusable.  You can have any number of magic ones.  Boards with
>> sufficiently similar requirements can share a magic one.
>
> So far we have only sufficiently similar boards that can share the
> only magic function. Not many boards use SPD data (these are mostly
> needed for real board firmware so anything purely virtual don't model
> it usually). The refactoring you propose could be needed if we had
> more dissimilar boards but I think we could do that at that
> time. Until then I've tried to make it simple for board code and put

Keeping things simple and just serve the needs you actually have is
good.  We're in a much better position to figure out how to best serve
more complicated needs once we actually have them :)

> all magic in one place instead of having separate implementation of
> this in several boards. Maybe someone should try to convert the
> remaining boards (MIPS Malta and ARM integratorcp) to see if any
> refactoring is needed before doing those refactoring without checking
> first what's needed. I did not try to convert those boards because I
> cannot test them.

That's fair.

[...]