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

[BALATON Zoltan]

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 <address@hidden>
> ---
> 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);
>     }