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

[Markus Armbruster]

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.

>> The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
>> EEPROM creation".  Turns out that commit changed memory type and
>> number of banks to
>>
>>    RAM size    #banks  type    bank size
>>     128 MiB         1   DDR2     128 MiB
>>      64 MiB         2   DDR       32 MiB
>>      32 MiB         1   DDR       32 MiB
>>
>> from
>>
>>    RAM size    #banks  type    bank size
>>     128 MiB         2   SDR       64 MiB
>>      64 MiB         2   SDR       32 MiB
>>      32 MiB         2   SDR       16 MiB
>>
>> Reverting that change also gets rid of the warning.
>>
>> I doubt physical Sam460ex boards can take SDR or DDR modules, though.
>
> It can't but it can use both DDR and DDR2 (the board can't but the SoC
> can and the firmware is OK with that too). This is what the commit
> fixed, please don't break it.

When a commit fixes something, it should say so.  This one does not:

    commit 08fd99179a8c5d34c7065e2ad76c7f8b6afe239e
    Author: BALATON Zoltan <address@hidden>
    Date:   Thu Jan 3 17:27:24 2019 +0100

        sam460ex: Clean up SPD EEPROM creation

        Get rid of code from MIPS Malta board used to create SPD EEPROM data
        (parts of which was not even needed for sam460ex) and use the generic
        spd_data_generate() function to simplify this.

        Signed-off-by: BALATON Zoltan <address@hidden>
        Signed-off-by: David Gibson <address@hidden>

This commit message certainly suggests it was a refactoring that did not
change SPD data at all.  Not the case, but you have to look at the patch
closely to see.  Water under the bridge, of course.

It misled me to assume the change was unintentional.

>                               The firmware may be confused if
> presented with different type of SDRAM than DDR or DDR2. Does it still
> boot and finds correct mem size after your change? (I think bdinfo
> U-Boot command tells what it detects.)

You're right: 08fd99179a8^ appears not to boot with -m 128 and lower.
08fd99179a8 was indeed a silent bug fix.

"make check" passes, though :)

I'll replace this patch by one that merely suppresses the warning.

Thanks!