Re: [PATCH v7 8/8] mac_oldworld: Add SPD data to cover RAM

[BALATON Zoltan]

On Tue, 13 Oct 2020, Philippe Mathieu-Daudé wrote:
> On 6/29/20 8:55 PM, BALATON Zoltan wrote:
> > OpenBIOS gets RAM size via fw_cfg but rhe original board firmware
>
> Typo "the".
>
> > detects RAM using SPD data so generate and add SDP eeproms to cover as
>
> EEPROMs?
>
> > much RAM as possible to describe with SPD (this may be less than the
> > actual ram_size due to SDRAM size constraints).
> >
> > This patch is more complex as it should be which I intend to fix once
> > agreement can be made on how to get back the necessary functionality
> > removed by previous patches. See in this thread:
> > https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg08710.html
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >   hw/ppc/mac_oldworld.c | 19 ++++++++++++++++++-
> >   1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > index 6276973c95..6a27287c9f 100644
> > --- a/hw/ppc/mac_oldworld.c
> > +++ b/hw/ppc/mac_oldworld.c
> > @@ -34,6 +34,7 @@
> >   #include "hw/input/adb.h"
> >   #include "sysemu/sysemu.h"
> >   #include "net/net.h"
> > +#include "hw/i2c/smbus_eeprom.h"
> >   #include "hw/isa/isa.h"
> >   #include "hw/pci/pci.h"
> >   #include "hw/pci/pci_host.h"
> > @@ -104,6 +105,8 @@ static void ppc_heathrow_init(MachineState *machine)
> >       DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> >       void *fw_cfg;
> >       uint64_t tbfreq;
> > +    uint8_t *spd_data[3] = {};
> > +    I2CBus *i2c_bus;
> >         /* init CPUs */
> >       for (i = 0; i < smp_cpus; i++) {
> > @@ -121,8 +124,16 @@ static void ppc_heathrow_init(MachineState *machine)
> >                        "maximum 2047 MB", ram_size / MiB);
> >           exit(1);
> >       }
> > -
> >       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
> > +    for (i = 0; i < 3; i++) {
>
> 3 -> ARRAY_SIZE(spd_data)
>
> > +        int size_left = ram_size - i * 512 * MiB;
> > +        if (size_left > 0) {
> > +            uint32_t s = size_left / MiB;
> > +            s = (s > 512 ? 512 : s);
>
> MIN()?

Thanks for the review. (You forgot to cc Mark so maybe he missed the other 
R-b tags but hopefully patchwork will pick these up.)

I plan to rewrite this patch eliminating this part and un-breaking 
spd_data_generate() instead to allow it to signal an error again so we 
don't need to duplicate part of it here to avoid aborting. (You may 
remember the thread where this was discussed in, I don't have a link to it 
now.) So just disregard this one patch for now, I'll take your other 
suggestions in account when submitting a rewritten version.

This and the WIP CUDA i2c patches are not yet ready to be merged and you 
had alternative suggestion for [v7,6/7] i2c: Match parameters of 
i2c_start_transfer and i2c_send_recv, but was that ever merged? Do you 
plan to resubmit or send a ping for that? It may have been lost somewhere 
because I have those mails in my mailbox but could not find it in Patchew 
or Patchwork.

Other than these 3 patches in the series others (1-5) should be OK and 
hope most of those could be taken by Mark now in the upcoming Mac fixes 
series to reduce the size of outstanding patches then I can fix up or 
rewrite the remaining ones.

I'm still not sure what to do about the sign extension problem in 
include/hw/elf_ops.h that was holding back patch 1 so if anyone has a 
suggestion please tell me or feel free to propose any fix for it, I'm not 
confident enough to break the elf loader.

Regards,
BALATON Zoltan

> > +            s = 1U << (31 - clz32(s));
> > +            spd_data[i] = spd_data_generate(SDR, s * MiB);
> > +        }
> > +    }
> >         /* allocate and load firmware ROM */
> >       memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE,
> > @@ -302,6 +313,12 @@ static void ppc_heathrow_init(MachineState *machine)
> >       macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
> >         dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
> > +    i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
> > +    for (i = 0; i < 3; i++) {
>
> 3 -> ARRAY_SIZE(spd_data)
>
> > +        if (spd_data[i]) {
> > +            smbus_eeprom_init_one(i2c_bus, 0x50 + i, spd_data[i]);
> > +        }
> > +    }
> >       adb_bus = qdev_get_child_bus(dev, "adb.0");
> >       dev = qdev_new(TYPE_ADB_KEYBOARD);
> >       qdev_realize_and_unref(dev, adb_bus, &error_fatal);