Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent

[Philippe Mathieu-Daudé]

+ Eduardo / Mark / Edgard / Alistair / Fred for QOM design.

On 6/26/20 12:54 PM, BALATON Zoltan wrote:
> On Fri, 26 Jun 2020, BALATON Zoltan wrote:
>> On Fri, 26 Jun 2020, Philippe Mathieu-Daudé wrote:
>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> Aspeed change pending latest ARM pull-request, so meanwhile sending
>>> as RFC.
>>> ---
>>> include/hw/i2c/smbus_eeprom.h |  9 ++++++---
>>> hw/i2c/smbus_eeprom.c         | 13 ++++++++++---
>>> hw/mips/fuloong2e.c           |  2 +-
>>> hw/ppc/sam460ex.c             |  2 +-
>>> 4 files changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/hw/i2c/smbus_eeprom.h
>>> b/include/hw/i2c/smbus_eeprom.h
>>> index 68b0063ab6..037612bbbb 100644
>>> --- a/include/hw/i2c/smbus_eeprom.h
>>> +++ b/include/hw/i2c/smbus_eeprom.h
>>> @@ -26,9 +26,12 @@
>>> #include "exec/cpu-common.h"
>>> #include "hw/i2c/i2c.h"
>>>
>>> -void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t
>>> *eeprom_buf);
>>> -void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
>>> -                       const uint8_t *eeprom_spd, int size);
>>> +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
>>> +                           I2CBus *smbus, uint8_t address,
>>> +                           uint8_t *eeprom_buf);
>>> +void smbus_eeprom_init(Object *parent_obj, const char
>>> *child_name_prefix,
>>> +                       I2CBus *smbus, int nb_eeprom,
>>> +                       const uint8_t *eeprom_spd, int eeprom_spd_size);
>>
>> Keeping I2CBus *smbus and uint8_t address as first parameters before
>> parent_obj and name looks better to me. These functions still operate
>> on an I2Cbus so could be regarded as methods of I2CBus therefore first
>> parameter should be that.
> 
> Also isn't parent_obj is the I2Cbus itself? Why is that need to be
> passed? The i2c_init_bus() also takes parent and name params so both
> I2Cbus and it's parent should be available as parents of the new I2C
> device here without more parameters. What am I missing here?

This is where I'm confused too and what I want to resolve with this
RFC series :)

The SPD EEPROM is soldered on the DIMM module. The DIMM exposes the
memory address/data pins and the i2c pins. We plug DIMMs on a
(mother)board.

I see the DIMM module being the parent. As we don't model it in QOM,
I used the MemoryRegion (which is what the SPD is describing).

We could represent the DIMM as a container of DRAM + SPD EEPROM, but
it makes the modeling slightly more complex. The only benefit is a
clearer modeling.

I'm not sure why the I2C bus is expected to be the parent. Maybe an
old wrong assumption?

>> Regards,
>> BALATON Zoltan
>>
>>> enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
>>> uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);
>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>> index b7def9eeb8..879fd7c416 100644
>>> --- a/hw/i2c/smbus_eeprom.c
>>> +++ b/hw/i2c/smbus_eeprom.c
>>> @@ -165,7 +165,9 @@ static void smbus_eeprom_register_types(void)
>>>
>>> type_init(smbus_eeprom_register_types)
>>>
>>> -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t
>>> *eeprom_buf)
>>> +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
>>> +                           I2CBus *smbus, uint8_t address,
>>> +                           uint8_t *eeprom_buf)
>>> {
>>>     DeviceState *dev;
>>>
>>> @@ -173,10 +175,12 @@ void smbus_eeprom_init_one(I2CBus *smbus,
>>> uint8_t address, uint8_t *eeprom_buf)
>>>     qdev_prop_set_uint8(dev, "address", address);
>>>     /* FIXME: use an array of byte or block backend property? */
>>>     SMBUS_EEPROM(dev)->init_data = eeprom_buf;
>>> +    object_property_add_child(parent_obj, child_name, OBJECT(dev));
>>>     qdev_realize_and_unref(dev, (BusState *)smbus, &error_fatal);
>>> }
>>>
>>> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
>>> +void smbus_eeprom_init(Object *parent_obj, const char
>>> *child_name_prefix,
>>> +                       I2CBus *smbus, int nb_eeprom,
>>>                        const uint8_t *eeprom_spd, int eeprom_spd_size)
>>> {
>>>     int i;
>>> @@ -189,8 +193,11 @@ void smbus_eeprom_init(I2CBus *smbus, int
>>> nb_eeprom,
>>>     }
>>>
>>>     for (i = 0; i < nb_eeprom; i++) {
>>> -        smbus_eeprom_init_one(smbus, 0x50 + i,
>>> +        char *name = g_strdup_printf("%s-%d", child_name_prefix, i);
>>> +
>>> +        smbus_eeprom_init_one(parent_obj, name, smbus, 0x50 + i,
>>>                               eeprom_buf + (i * SMBUS_EEPROM_SIZE));
>>> +        g_free(name);
>>>     }
>>> }
>>>
>>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>>> index 8ca31e5162..304a096c6a 100644
>>> --- a/hw/mips/fuloong2e.c
>>> +++ b/hw/mips/fuloong2e.c
>>> @@ -377,7 +377,7 @@ static void mips_fuloong2e_init(MachineState
>>> *machine)
>>>
>>>     /* Populate SPD eeprom data */
>>>     spd_data = spd_data_generate(DDR, machine->ram_size);
>>> -    smbus_eeprom_init_one(smbus, 0x50, spd_data);
>>> +    smbus_eeprom_init_one(OBJECT(machine->ram), "spd", smbus, 0x50,
>>> spd_data);
>>>
>>>     mc146818_rtc_init(isa_bus, 2000, NULL);
>>>
>>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>>> index 1a106a68de..064d07f4e2 100644
>>> --- a/hw/ppc/sam460ex.c
>>> +++ b/hw/ppc/sam460ex.c
>>> @@ -337,7 +337,7 @@ static void sam460ex_init(MachineState *machine)
>>>     spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
>>>                                  ram_sizes[0]);
>>>     spd_data[20] = 4; /* SO-DIMM module */
>>> -    smbus_eeprom_init_one(i2c, 0x50, spd_data);
>>> +    smbus_eeprom_init_one(OBJECT(machine->ram), "spd", i2c, 0x50,
>>> spd_data);
>>>     /* RTC */
>>>     i2c_create_slave(i2c, "m41t80", 0x68);
>>>
>>