[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH 1/4] sam460ex: Revert change to SPD memory type for <= 128 MiB
Date: Wed, 22 Apr 2020 15:56:46 +0200 (CEST)
User-agent: Alpine 2.22 (BSF 395 2020-01-19)

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 

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

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

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

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 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.

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


   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.

Sorry, I may have forgotten it by the time I was refactorig commits for submission.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]