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

[Eduardo Habkost]

On Fri, Jun 26, 2020 at 04:15:40PM +0200, 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).

I'll be honest: I don't think I understand the main purpose of
QOM parent/child relationships.  My best guess is that they make
object destruction easier to manage (if you destroy a parent, you
will automatically destroy its children).

If we don't write down what QOM parent/child relationships are
supposed to mean (and what they are _not_ supposed to mean), we
will never know when it's appropriate and/or safe to move objects
to a different parent.

-- 
Eduardo