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