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

[BALATON Zoltan]

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)?

Regards,
BALATON Zoltan