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

[Philippe Mathieu-Daudé]

On 6/26/20 4:26 PM, BALATON Zoltan wrote:
> On Fri, 26 Jun 2020, Philippe Mathieu-Daudé wrote:
>> On 6/26/20 4:03 PM, BALATON Zoltan wrote:
>>> On Fri, 26 Jun 2020, Philippe Mathieu-Daudé wrote:
>>>> + 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?
>>>
>>> I guess it's a question of what the parent should mean? Is it parent of
>>> the object in which case it's the I2CBus (which is kind of logical view
>>> of the object tree modelling the machine) or the parent of the thing
>>> modelled in the machine (which is physical view of the machine
>>> components) then it should be the RAM module. The confusion probably
>>> comes from this question not answered. Also the DIMM module is not
>>> modelled so when you assign SPD eeproms to memory region it could be
>>> multiple SPD eeproms will be parented by a single RAM memory region
>>> (possibly not covering it fully as in the mac_oldworld or sam460ex case
>>> discussed in another thread). This does not seem too intuitive.
>>
>> From the bus perspective, requests are sent hoping for a device to
>> answer to the requested address ("Hello, do I have children? Hello?
>> Anybody here?"), if nobody is here, the request timeouts.
>> So there is not really a strong family relationship here.
>>
>> If you unplug a DIMM, you remove both the MemoryRegion and the EEPROM.
>> This is how I understand the QOM parent relationship so far (if you
>> remove a parent, you also remove its children).
> 
> OK but what if we don't have a DIMM object as that is not modelled?
> Should you set parent to the board in that case? Or is it still modelled
> as an I2C device that is plugged in an I2C bus. Does it need to have a
> parent in this case at all or is it a case for an unattached device
> (becuase its physical parent is not modelled)?

So you reduced the dependency as SPD -> RAM, which is clever because
we won't have DIMM without RAM =)

Suggestion for new prototype (eeprom content populated in callee):

  SMBusEEPROMDevice spd_eeprom_new(MemoryDeviceState *memdev,
                                   const char *eeprom_name,
                                   I2CBus *smbus,
                                   uint8_t i2c_address);

The SPD describes the @memdev (which is its parent).
We can still modify the eeprom content after its creation.

> 
> Regards,
> BALATON Zoltan