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

[Philippe Mathieu-Daudé]

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?


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?

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

Thanks,

Phil.