From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: "Corey Minyard" <cminyard@mvista.com>,
"Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
qemu-devel@nongnu.org,
"Alistair Francis" <alistair@alistair23.me>,
"Markus Armbruster" <armbru@redhat.com>,
"Jiaxun Yang" <jiaxun.yang@flygoat.com>,
"Aleksandar Markovic" <aleksandar.qemu.devel@gmail.com>,
qemu-ppc@nongnu.org, "Cédric Le Goater" <clg@kaod.org>,
"Huacai Chen" <chenhc@lemote.com>,
"Fred Konrad" <konrad@adacore.com>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
Date: Fri, 10 Jul 2020 11:12:33 +0200 [thread overview]
Message-ID: <97e85da0-b297-30da-1ab0-dd725ab8f2f3@amsat.org> (raw)
In-Reply-To: <20200709200523.GK780932@habkost.net>
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.
next prev parent reply other threads:[~2020-07-10 9:13 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-26 10:27 [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description Philippe Mathieu-Daudé
2020-06-26 10:27 ` [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent Philippe Mathieu-Daudé
2020-06-26 10:47 ` BALATON Zoltan
2020-06-26 10:54 ` BALATON Zoltan
2020-06-26 12:40 ` Philippe Mathieu-Daudé
2020-06-26 14:03 ` BALATON Zoltan
2020-06-26 14:15 ` Philippe Mathieu-Daudé
2020-06-26 14:26 ` BALATON Zoltan
2020-06-26 14:37 ` Philippe Mathieu-Daudé
2020-07-09 20:05 ` Eduardo Habkost
2020-07-10 9:12 ` Philippe Mathieu-Daudé [this message]
2020-07-10 20:09 ` Eduardo Habkost
2020-06-26 10:27 ` [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name Philippe Mathieu-Daudé
2020-06-26 11:00 ` BALATON Zoltan
2020-06-26 14:26 ` Philippe Mathieu-Daudé
2020-07-09 19:48 ` Eduardo Habkost
2020-07-10 9:05 ` Philippe Mathieu-Daudé
2020-07-10 17:40 ` Eduardo Habkost
2020-07-13 9:23 ` Markus Armbruster
2020-06-26 10:27 ` [RFC PATCH 3/3] hw/i2c/smbus_eeprom: Trace reset() event Philippe Mathieu-Daudé
2020-06-26 10:37 ` [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description no-reply
2020-06-26 12:43 ` Philippe Mathieu-Daudé
2020-06-26 10:38 ` no-reply
2020-06-26 15:59 ` Aleksandar Markovic
2020-06-26 16:44 ` Philippe Mathieu-Daudé
2020-06-27 6:55 ` Markus Armbruster
2020-06-29 11:38 ` Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=97e85da0-b297-30da-1ab0-dd725ab8f2f3@amsat.org \
--to=f4bug@amsat.org \
--cc=aleksandar.qemu.devel@gmail.com \
--cc=aleksandar.rikalo@syrmia.com \
--cc=alistair@alistair23.me \
--cc=armbru@redhat.com \
--cc=chenhc@lemote.com \
--cc=clg@kaod.org \
--cc=cminyard@mvista.com \
--cc=david@gibson.dropbear.id.au \
--cc=ehabkost@redhat.com \
--cc=jiaxun.yang@flygoat.com \
--cc=konrad@adacore.com \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.