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