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 <arm...@redhat.com> >>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@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