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). > > Regards, > BALATON Zoltan