| Message ID | 20200626102744.15053-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use object_get_canonical_path_component to get child description | expand |
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. Regards, BALATON Zoltan > enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; > uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size); > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > index b7def9eeb8..879fd7c416 100644 > --- a/hw/i2c/smbus_eeprom.c > +++ b/hw/i2c/smbus_eeprom.c > @@ -165,7 +165,9 @@ static void smbus_eeprom_register_types(void) > > type_init(smbus_eeprom_register_types) > > -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name, > + I2CBus *smbus, uint8_t address, > + uint8_t *eeprom_buf) > { > DeviceState *dev; > > @@ -173,10 +175,12 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > qdev_prop_set_uint8(dev, "address", address); > /* FIXME: use an array of byte or block backend property? */ > SMBUS_EEPROM(dev)->init_data = eeprom_buf; > + object_property_add_child(parent_obj, child_name, OBJECT(dev)); > qdev_realize_and_unref(dev, (BusState *)smbus, &error_fatal); > } > > -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, > +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) > { > int i; > @@ -189,8 +193,11 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, > } > > for (i = 0; i < nb_eeprom; i++) { > - smbus_eeprom_init_one(smbus, 0x50 + i, > + char *name = g_strdup_printf("%s-%d", child_name_prefix, i); > + > + smbus_eeprom_init_one(parent_obj, name, smbus, 0x50 + i, > eeprom_buf + (i * SMBUS_EEPROM_SIZE)); > + g_free(name); > } > } > > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c > index 8ca31e5162..304a096c6a 100644 > --- a/hw/mips/fuloong2e.c > +++ b/hw/mips/fuloong2e.c > @@ -377,7 +377,7 @@ static void mips_fuloong2e_init(MachineState *machine) > > /* Populate SPD eeprom data */ > spd_data = spd_data_generate(DDR, machine->ram_size); > - smbus_eeprom_init_one(smbus, 0x50, spd_data); > + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", smbus, 0x50, spd_data); > > mc146818_rtc_init(isa_bus, 2000, NULL); > > diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c > index 1a106a68de..064d07f4e2 100644 > --- a/hw/ppc/sam460ex.c > +++ b/hw/ppc/sam460ex.c > @@ -337,7 +337,7 @@ static void sam460ex_init(MachineState *machine) > spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2, > ram_sizes[0]); > spd_data[20] = 4; /* SO-DIMM module */ > - smbus_eeprom_init_one(i2c, 0x50, spd_data); > + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", i2c, 0x50, spd_data); > /* RTC */ > i2c_create_slave(i2c, "m41t80", 0x68); > >
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? > Regards, > BALATON Zoltan > >> enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; >> uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size); >> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >> index b7def9eeb8..879fd7c416 100644 >> --- a/hw/i2c/smbus_eeprom.c >> +++ b/hw/i2c/smbus_eeprom.c >> @@ -165,7 +165,9 @@ static void smbus_eeprom_register_types(void) >> >> type_init(smbus_eeprom_register_types) >> >> -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t >> *eeprom_buf) >> +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name, >> + I2CBus *smbus, uint8_t address, >> + uint8_t *eeprom_buf) >> { >> DeviceState *dev; >> >> @@ -173,10 +175,12 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t >> address, uint8_t *eeprom_buf) >> qdev_prop_set_uint8(dev, "address", address); >> /* FIXME: use an array of byte or block backend property? */ >> SMBUS_EEPROM(dev)->init_data = eeprom_buf; >> + object_property_add_child(parent_obj, child_name, OBJECT(dev)); >> qdev_realize_and_unref(dev, (BusState *)smbus, &error_fatal); >> } >> >> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >> +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) >> { >> int i; >> @@ -189,8 +193,11 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >> } >> >> for (i = 0; i < nb_eeprom; i++) { >> - smbus_eeprom_init_one(smbus, 0x50 + i, >> + char *name = g_strdup_printf("%s-%d", child_name_prefix, i); >> + >> + smbus_eeprom_init_one(parent_obj, name, smbus, 0x50 + i, >> eeprom_buf + (i * SMBUS_EEPROM_SIZE)); >> + g_free(name); >> } >> } >> >> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c >> index 8ca31e5162..304a096c6a 100644 >> --- a/hw/mips/fuloong2e.c >> +++ b/hw/mips/fuloong2e.c >> @@ -377,7 +377,7 @@ static void mips_fuloong2e_init(MachineState *machine) >> >> /* Populate SPD eeprom data */ >> spd_data = spd_data_generate(DDR, machine->ram_size); >> - smbus_eeprom_init_one(smbus, 0x50, spd_data); >> + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", smbus, 0x50, >> spd_data); >> >> mc146818_rtc_init(isa_bus, 2000, NULL); >> >> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c >> index 1a106a68de..064d07f4e2 100644 >> --- a/hw/ppc/sam460ex.c >> +++ b/hw/ppc/sam460ex.c >> @@ -337,7 +337,7 @@ static void sam460ex_init(MachineState *machine) >> spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2, >> ram_sizes[0]); >> spd_data[20] = 4; /* SO-DIMM module */ >> - smbus_eeprom_init_one(i2c, 0x50, spd_data); >> + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", i2c, 0x50, >> spd_data); >> /* RTC */ >> i2c_create_slave(i2c, "m41t80", 0x68); >> >
+ 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? >> Regards, >> BALATON Zoltan >> >>> enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; >>> uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size); >>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >>> index b7def9eeb8..879fd7c416 100644 >>> --- a/hw/i2c/smbus_eeprom.c >>> +++ b/hw/i2c/smbus_eeprom.c >>> @@ -165,7 +165,9 @@ static void smbus_eeprom_register_types(void) >>> >>> type_init(smbus_eeprom_register_types) >>> >>> -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t >>> *eeprom_buf) >>> +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name, >>> + I2CBus *smbus, uint8_t address, >>> + uint8_t *eeprom_buf) >>> { >>> DeviceState *dev; >>> >>> @@ -173,10 +175,12 @@ void smbus_eeprom_init_one(I2CBus *smbus, >>> uint8_t address, uint8_t *eeprom_buf) >>> qdev_prop_set_uint8(dev, "address", address); >>> /* FIXME: use an array of byte or block backend property? */ >>> SMBUS_EEPROM(dev)->init_data = eeprom_buf; >>> + object_property_add_child(parent_obj, child_name, OBJECT(dev)); >>> qdev_realize_and_unref(dev, (BusState *)smbus, &error_fatal); >>> } >>> >>> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >>> +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) >>> { >>> int i; >>> @@ -189,8 +193,11 @@ void smbus_eeprom_init(I2CBus *smbus, int >>> nb_eeprom, >>> } >>> >>> for (i = 0; i < nb_eeprom; i++) { >>> - smbus_eeprom_init_one(smbus, 0x50 + i, >>> + char *name = g_strdup_printf("%s-%d", child_name_prefix, i); >>> + >>> + smbus_eeprom_init_one(parent_obj, name, smbus, 0x50 + i, >>> eeprom_buf + (i * SMBUS_EEPROM_SIZE)); >>> + g_free(name); >>> } >>> } >>> >>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c >>> index 8ca31e5162..304a096c6a 100644 >>> --- a/hw/mips/fuloong2e.c >>> +++ b/hw/mips/fuloong2e.c >>> @@ -377,7 +377,7 @@ static void mips_fuloong2e_init(MachineState >>> *machine) >>> >>> /* Populate SPD eeprom data */ >>> spd_data = spd_data_generate(DDR, machine->ram_size); >>> - smbus_eeprom_init_one(smbus, 0x50, spd_data); >>> + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", smbus, 0x50, >>> spd_data); >>> >>> mc146818_rtc_init(isa_bus, 2000, NULL); >>> >>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c >>> index 1a106a68de..064d07f4e2 100644 >>> --- a/hw/ppc/sam460ex.c >>> +++ b/hw/ppc/sam460ex.c >>> @@ -337,7 +337,7 @@ static void sam460ex_init(MachineState *machine) >>> spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2, >>> ram_sizes[0]); >>> spd_data[20] = 4; /* SO-DIMM module */ >>> - smbus_eeprom_init_one(i2c, 0x50, spd_data); >>> + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", i2c, 0x50, >>> spd_data); >>> /* RTC */ >>> i2c_create_slave(i2c, "m41t80", 0x68); >>> >>
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. Regards, BALATON Zoltan
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). > > Regards, > BALATON Zoltan
On Fri, 26 Jun 2020, 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). OK but what if we don't have a DIMM object as that is not modelled? Should you set parent to the board in that case? Or is it still modelled as an I2C device that is plugged in an I2C bus. Does it need to have a parent in this case at all or is it a case for an unattached device (becuase its physical parent is not modelled)? Regards, BALATON Zoltan
On 6/26/20 4:26 PM, BALATON Zoltan wrote: > On Fri, 26 Jun 2020, 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). > > OK but what if we don't have a DIMM object as that is not modelled? > Should you set parent to the board in that case? Or is it still modelled > as an I2C device that is plugged in an I2C bus. Does it need to have a > parent in this case at all or is it a case for an unattached device > (becuase its physical parent is not modelled)? So you reduced the dependency as SPD -> RAM, which is clever because we won't have DIMM without RAM =) Suggestion for new prototype (eeprom content populated in callee): SMBusEEPROMDevice spd_eeprom_new(MemoryDeviceState *memdev, const char *eeprom_name, I2CBus *smbus, uint8_t i2c_address); The SPD describes the @memdev (which is its parent). We can still modify the eeprom content after its creation. > > Regards, > BALATON Zoltan
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.
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.
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?
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); enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size); diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index b7def9eeb8..879fd7c416 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -165,7 +165,9 @@ static void smbus_eeprom_register_types(void) type_init(smbus_eeprom_register_types) -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) +void smbus_eeprom_init_one(Object *parent_obj, const char *child_name, + I2CBus *smbus, uint8_t address, + uint8_t *eeprom_buf) { DeviceState *dev; @@ -173,10 +175,12 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) qdev_prop_set_uint8(dev, "address", address); /* FIXME: use an array of byte or block backend property? */ SMBUS_EEPROM(dev)->init_data = eeprom_buf; + object_property_add_child(parent_obj, child_name, OBJECT(dev)); qdev_realize_and_unref(dev, (BusState *)smbus, &error_fatal); } -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, +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) { int i; @@ -189,8 +193,11 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, } for (i = 0; i < nb_eeprom; i++) { - smbus_eeprom_init_one(smbus, 0x50 + i, + char *name = g_strdup_printf("%s-%d", child_name_prefix, i); + + smbus_eeprom_init_one(parent_obj, name, smbus, 0x50 + i, eeprom_buf + (i * SMBUS_EEPROM_SIZE)); + g_free(name); } } diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c index 8ca31e5162..304a096c6a 100644 --- a/hw/mips/fuloong2e.c +++ b/hw/mips/fuloong2e.c @@ -377,7 +377,7 @@ static void mips_fuloong2e_init(MachineState *machine) /* Populate SPD eeprom data */ spd_data = spd_data_generate(DDR, machine->ram_size); - smbus_eeprom_init_one(smbus, 0x50, spd_data); + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", smbus, 0x50, spd_data); mc146818_rtc_init(isa_bus, 2000, NULL); diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c index 1a106a68de..064d07f4e2 100644 --- a/hw/ppc/sam460ex.c +++ b/hw/ppc/sam460ex.c @@ -337,7 +337,7 @@ static void sam460ex_init(MachineState *machine) spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2, ram_sizes[0]); spd_data[20] = 4; /* SO-DIMM module */ - smbus_eeprom_init_one(i2c, 0x50, spd_data); + smbus_eeprom_init_one(OBJECT(machine->ram), "spd", i2c, 0x50, spd_data); /* RTC */ i2c_create_slave(i2c, "m41t80", 0x68);
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(-)