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
This is because sam460ex_init() asks spd_data_generate() for DDR2,
which is impossible, so spd_data_generate() corrects it to DDR.
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.
The commit changed SPD contents in other places, too. So does commit
fb1b0fcc03 "target/mips: fulong2e: Dynamically generate SPD EEPROM
data" for machine type fulong2e. I'm not reverting these changes.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ppc/sam460ex.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 898453cf30..856bc0b5a3 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -335,7 +335,8 @@ static void sam460ex_init(MachineState *machine)
dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
i2c = PPC4xx_I2C(dev)->bus;
/* SPD EEPROM on RAM module */
- spd_data = spd_data_generate(DDR2, ram_sizes[0], &err);
+ spd_data = spd_data_generate(ram_sizes[0] < 256 * MiB ? SDR : DDR2,
+ ram_sizes[0], &err);
if (err) {
warn_report_err(err);
}
--
2.21.1
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.
> 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().
> 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. 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.)
Regards,
BALATON Zoltan
> The commit changed SPD contents in other places, too. So does commit
> fb1b0fcc03 "target/mips: fulong2e: Dynamically generate SPD EEPROM
> data" for machine type fulong2e. I'm not reverting these changes.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/ppc/sam460ex.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 898453cf30..856bc0b5a3 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -335,7 +335,8 @@ static void sam460ex_init(MachineState *machine)
> dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
> i2c = PPC4xx_I2C(dev)->bus;
> /* SPD EEPROM on RAM module */
> - spd_data = spd_data_generate(DDR2, ram_sizes[0], &err);
> + spd_data = spd_data_generate(ram_sizes[0] < 256 * MiB ? SDR : DDR2,
> + ram_sizes[0], &err);
> if (err) {
> warn_report_err(err);
> }
>
BALATON Zoltan <balaton@eik.bme.hu> 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 <balaton@eik.bme.hu>
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 <balaton@eik.bme.hu>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
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!
On Tue, 21 Apr 2020, Markus Armbruster wrote: > BALATON Zoltan <balaton@eik.bme.hu> 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 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 >>> >>> 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 <balaton@eik.bme.hu> > 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 <balaton@eik.bme.hu> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > 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. Regards, BALATON Zoltan
BALATON Zoltan <balaton@eik.bme.hu> writes: > On Tue, 21 Apr 2020, Markus Armbruster wrote: >> BALATON Zoltan <balaton@eik.bme.hu> 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. [...]
© 2016 - 2026 Red Hat, Inc.