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.