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

[Eduardo Habkost]

On Fri, Jul 10, 2020 at 11:12:33AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/9/20 10:05 PM, Eduardo Habkost wrote:
> > 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.
> 
> I'm trying to understand these monitor commands:
> 
> info qdm  -- show qdev device model list
> info qom-tree [path] -- show QOM composition tree
> info qtree  -- show device tree
> 
> This is the 'QOM composition tree' of the isapc machine:
> 
> (qemu) info qom-tree
> /machine (isapc-machine)
>   /fw_cfg (fw_cfg_io)
>     /fwcfg.dma[0] (qemu:memory-region)
>     /fwcfg[0] (qemu:memory-region)
>   /peripheral (container)
>   /peripheral-anon (container)
>   /unattached (container)
>     /device[0] (486-i386-cpu)
>       /memory[0] (qemu:memory-region)
>       /memory[1] (qemu:memory-region)
>     /device[10] (isa-serial)
>       /serial (serial)
>       /serial[0] (qemu:memory-region)
>     /device[11] (isa-parallel)
>       /parallel[0] (qemu:memory-region)
>     /device[12] (isa-fdc)
>       /fdc[0] (qemu:memory-region)
>       /fdc[1] (qemu:memory-region)
>       /floppy-bus.0 (floppy-bus)
>     /device[13] (floppy)
>     /device[14] (i8042)
>       /i8042-cmd[0] (qemu:memory-region)
>       /i8042-data[0] (qemu:memory-region)
>     /device[15] (vmport)
>       /vmport[0] (qemu:memory-region)
>     /device[16] (vmmouse)
>     /device[17] (port92)
>       /port92[0] (qemu:memory-region)
>     /device[18] (ne2k_isa)
>       /ne2000[0] (qemu:memory-region)
>     /device[19] (isa-ide)
>       /ide.0 (IDE)
>       /ide[0] (qemu:memory-region)
>       /ide[1] (qemu:memory-region)
>     /device[1] (isabus-bridge)
>       /isa.0 (ISA)
>     /device[20] (isa-ide)
>       /ide.1 (IDE)
>       /ide[0] (qemu:memory-region)
>       /ide[1] (qemu:memory-region)
>     /device[21] (ide-cd)
>     /device[2] (isa-i8259)
>       /elcr[0] (qemu:memory-region)
>       /pic[0] (qemu:memory-region)
>       /unnamed-gpio-in[0] (irq)
>       /unnamed-gpio-in[1] (irq)
>       /unnamed-gpio-in[2] (irq)
>       /unnamed-gpio-in[3] (irq)
>       /unnamed-gpio-in[4] (irq)
>       /unnamed-gpio-in[5] (irq)
>       /unnamed-gpio-in[6] (irq)
>       /unnamed-gpio-in[7] (irq)
>     /device[3] (isa-i8259)
>       /elcr[0] (qemu:memory-region)
>       /pic[0] (qemu:memory-region)
>       /unnamed-gpio-in[0] (irq)
>       /unnamed-gpio-in[1] (irq)
>       /unnamed-gpio-in[2] (irq)
>       /unnamed-gpio-in[3] (irq)
>       /unnamed-gpio-in[4] (irq)
>       /unnamed-gpio-in[5] (irq)
>       /unnamed-gpio-in[6] (irq)
>       /unnamed-gpio-in[7] (irq)
>     /device[4] (isa-cirrus-vga)
>       /cirrus-bitblt-mmio[0] (qemu:memory-region)
>       /cirrus-io[0] (qemu:memory-region)
>       /cirrus-linear-io[0] (qemu:memory-region)
>       /cirrus-low-memory[0] (qemu:memory-region)
>       /cirrus-lowmem-container[0] (qemu:memory-region)
>       /cirrus-mmio[0] (qemu:memory-region)
>       /vga.bank0[0] (qemu:memory-region)
>       /vga.bank1[0] (qemu:memory-region)
>       /vga.vram[0] (qemu:memory-region)
>     /device[5] (mc146818rtc)
>       /rtc-index[0] (qemu:memory-region)
>       /rtc[0] (qemu:memory-region)
>     /device[6] (isa-pit)
>       /pit[0] (qemu:memory-region)
>       /unnamed-gpio-in[0] (irq)
>     /device[7] (isa-pcspk)
>       /pcspk[0] (qemu:memory-region)
>     /device[8] (i8257)
>       /dma-chan[0] (qemu:memory-region)
>       /dma-cont[0] (qemu:memory-region)
>       /dma-page[0] (qemu:memory-region)
>       /dma-page[1] (qemu:memory-region)
>     /device[9] (i8257)
>       /dma-chan[0] (qemu:memory-region)
>       /dma-cont[0] (qemu:memory-region)
>       /dma-page[0] (qemu:memory-region)
>       /dma-page[1] (qemu:memory-region)
>     /io[0] (qemu:memory-region)
>     /ioport80[0] (qemu:memory-region)
>     /ioportF0[0] (qemu:memory-region)
>     /isa-bios[0] (qemu:memory-region)
>     /non-qdev-gpio[0] (irq)
>     /non-qdev-gpio[1] (irq)
>     /non-qdev-gpio[2] (irq)
>     /non-qdev-gpio[3] (irq)
>     /non-qdev-gpio[4] (irq)
>     /pc.bios[0] (qemu:memory-region)
>     /pc.rom[0] (qemu:memory-region)
>     /ram-below-4g[0] (qemu:memory-region)
>     /sysbus (System)
>     /system[0] (qemu:memory-region)
> 
> What means for an object to have an '/unattached' parent?

[1]

(comment on this below)


> 
> 
> And now the raspi2:
> 
> (qemu) info qom-tree
> /machine (raspi2-machine)
>   /peripheral (container)
>   /peripheral-anon (container)
>   /soc (bcm2836)
>     /control (bcm2836-control)
>       /bcm2836-control[0] (qemu:memory-region)
>       /cnthpirq[0] (irq)
>       /cnthpirq[1] (irq)
>       [...]
>       /gpu-fiq[0] (irq)
>       /gpu-irq[0] (irq)
>     /cpu[0] (cortex-a7-arm-cpu)
>       /unnamed-gpio-in[0] (irq)
>       /unnamed-gpio-in[1] (irq)
>       /unnamed-gpio-in[2] (irq)
>       /unnamed-gpio-in[3] (irq)
>     /cpu[1] (cortex-a7-arm-cpu)
>       /unnamed-gpio-in[0] (irq)
>       /unnamed-gpio-in[1] (irq)
>       /unnamed-gpio-in[2] (irq)
>       /unnamed-gpio-in[3] (irq)
>     /cpu[2] (cortex-a7-arm-cpu)
>       /unnamed-gpio-in[0] (irq)
>       /unnamed-gpio-in[1] (irq)
>       /unnamed-gpio-in[2] (irq)
>       /unnamed-gpio-in[3] (irq)
>     /cpu[3] (cortex-a7-arm-cpu)
>       /unnamed-gpio-in[0] (irq)
>       /unnamed-gpio-in[1] (irq)
>       /unnamed-gpio-in[2] (irq)
>       /unnamed-gpio-in[3] (irq)
>     /peripherals (bcm2835-peripherals)
>       /aux (bcm2835-aux)
>         /bcm2835-aux[0] (qemu:memory-region)
>       /bcm2835-a2w (unimplemented-device)
>         /bcm2835-a2w[0] (qemu:memory-region)
>       /bcm2835-ave0 (unimplemented-device)
>         /bcm2835-ave0[0] (qemu:memory-region)
>       /bcm2835-cprman (unimplemented-device)
>         /bcm2835-cprman[0] (qemu:memory-region)
>       /bcm2835-dbus (unimplemented-device)
>         /bcm2835-dbus[0] (qemu:memory-region)
>       /bcm2835-gpu-ram-alias\x5b*\x5d[0] (qemu:memory-region)
>       /bcm2835-gpu-ram-alias\x5b*\x5d[1] (qemu:memory-region)
>       /bcm2835-gpu-ram-alias\x5b*\x5d[2] (qemu:memory-region)
>       /bcm2835-gpu-ram-alias\x5b*\x5d[3] (qemu:memory-region)
>       /bcm2835-gpu[0] (qemu:memory-region)
>       /bcm2835-i2c0 (unimplemented-device)
>         /bcm2835-i2c0[0] (qemu:memory-region)
>       /bcm2835-i2c1 (unimplemented-device)
>         /bcm2835-i2c1[0] (qemu:memory-region)
>       /bcm2835-i2c2 (unimplemented-device)
>         /bcm2835-i2c2[0] (qemu:memory-region)
>       /bcm2835-i2s (unimplemented-device)
>         /bcm2835-i2s[0] (qemu:memory-region)
>       /bcm2835-mbox[0] (qemu:memory-region)
>       /bcm2835-otp (unimplemented-device)
>         /bcm2835-otp[0] (qemu:memory-region)
>       /bcm2835-peripherals[0] (qemu:memory-region)
>       /bcm2835-peripherals[1] (qemu:memory-region)
>       /bcm2835-sdramc (unimplemented-device)
>         /bcm2835-sdramc[0] (qemu:memory-region)
>       /bcm2835-smi (unimplemented-device)
>         /bcm2835-smi[0] (qemu:memory-region)
>       /bcm2835-sp804 (unimplemented-device)
>         /bcm2835-sp804[0] (qemu:memory-region)
>       /bcm2835-spi0 (unimplemented-device)
>         /bcm2835-spi0[0] (qemu:memory-region)
>       /bcm2835-spis (unimplemented-device)
>         /bcm2835-spis[0] (qemu:memory-region)
>       /dma (bcm2835-dma)
>         /bcm2835-dma-chan15[0] (qemu:memory-region)
>         /bcm2835-dma[0] (qemu:memory-region)
>       /dwc2 (dwc2-usb)
>         /dwc2-fifo[0] (qemu:memory-region)
>         /dwc2-io[0] (qemu:memory-region)
>         /dwc2[0] (qemu:memory-region)
>         /usb-bus.0 (usb-bus)
>       /fb (bcm2835-fb)
>         /bcm2835-fb[0] (qemu:memory-region)
>       /gpio (bcm2835_gpio)
>         /bcm2835_gpio[0] (qemu:memory-region)
>         /sd-bus (sd-bus)
>       /ic (bcm2835-ic)
>         /arm-irq[0] (irq)
>         /arm-irq[1] (irq)
>         /arm-irq[2] (irq)
>         /arm-irq[3] (irq)
>         /arm-irq[4] (irq)
>         /arm-irq[5] (irq)
>         /arm-irq[6] (irq)
>         /arm-irq[7] (irq)
>         /bcm2835-ic[0] (qemu:memory-region)
>         /gpu-irq[0] (irq)
>         /gpu-irq[10] (irq)
>         [...]
>       /mbox (bcm2835-mbox)
>         /bcm2835-mbox[0] (qemu:memory-region)
>         /unnamed-gpio-in[0] (irq)
>         /unnamed-gpio-in[1] (irq)
>         /unnamed-gpio-in[2] (irq)
>         /unnamed-gpio-in[3] (irq)
>         /unnamed-gpio-in[4] (irq)
>         /unnamed-gpio-in[5] (irq)
>         /unnamed-gpio-in[6] (irq)
>         /unnamed-gpio-in[7] (irq)
>         /unnamed-gpio-in[8] (irq)
>       /mphi (bcm2835-mphi)
>         /mphi[0] (qemu:memory-region)
>       /property (bcm2835-property)
>         /bcm2835-property[0] (qemu:memory-region)
>       /rng (bcm2835-rng)
>         /bcm2835-rng[0] (qemu:memory-region)
>       /sdhci (generic-sdhci)
>         /sd-bus (sdhci-bus)
>         /sdhci[0] (qemu:memory-region)
>       /sdhost (bcm2835-sdhost)
>         /bcm2835-sdhost[0] (qemu:memory-region)
>         /sd-bus (bcm2835-sdhost-bus)
>       /systimer (bcm2835-sys-timer)
>         /bcm2835-sys-timer[0] (qemu:memory-region)
>       /thermal (bcm2835-thermal)
>         /bcm2835-thermal[0] (qemu:memory-region)
>       /uart0 (pl011)
>         /pl011[0] (qemu:memory-region)
>   /unattached (container)
>     /device[0] (sd-card)
>     /io[0] (qemu:memory-region)
>     /sysbus (System)
>     /system[0] (qemu:memory-region)
> 
> Who is the 'parent' of the sd-card? The sd-bus? The sdhci controller?
> The machine?

This one is easy to answer: the parent of the sd-card is the
container object called "/machine/unattached".

Which leads to your question above[1].  What does it mean to be a
child of /machine/unattached?  Does it matter?  When and why?
Who *should* be the QOM parent of sd-card, ideally?  Why aren't
unattached devices added as children of "/machine" directly by
default?  What would be the consequences if we did it?

> 
> The sd-card can be 'reparented' between the sd-busses of
> '/sdhci (generic-sdhci)' and '/sdhost (bcm2835-sdhost)'.

What "can be reparented" means here?

-- 
Eduardo