* [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
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 ` Philippe Mathieu-Daudé
2020-06-26 10:47 ` BALATON Zoltan
2020-06-26 10:27 ` [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name Philippe Mathieu-Daudé
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 10:27 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Corey Minyard, Aleksandar Rikalo, Philippe Mathieu-Daudé,
Jiaxun Yang, Aleksandar Markovic, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, Aurelien Jarno,
David Gibson
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);
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);
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
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
0 siblings, 1 reply; 27+ messages in thread
From: BALATON Zoltan @ 2020-06-26 10:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, Markus Armbruster, Jiaxun Yang,
qemu-devel, Aleksandar Markovic, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, David Gibson
[-- Attachment #1: Type: text/plain, Size: 4535 bytes --]
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);
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-06-26 10:47 ` BALATON Zoltan
@ 2020-06-26 10:54 ` BALATON Zoltan
2020-06-26 12:40 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 27+ messages in thread
From: BALATON Zoltan @ 2020-06-26 10:54 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, Markus Armbruster, Jiaxun Yang,
qemu-devel, Aleksandar Markovic, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, David Gibson
[-- Attachment #1: Type: text/plain, Size: 4992 bytes --]
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);
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-06-26 10:54 ` BALATON Zoltan
@ 2020-06-26 12:40 ` Philippe Mathieu-Daudé
2020-06-26 14:03 ` BALATON Zoltan
0 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 12:40 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Corey Minyard, Aleksandar Rikalo, Eduardo Habkost,
Alistair Francis, Mark Cave-Ayland, Markus Armbruster,
Jiaxun Yang, qemu-devel, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Edgar E. Iglesias, Huacai Chen,
Fred Konrad, Philippe Mathieu-Daudé, David Gibson
+ 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);
>>>
>>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-06-26 12:40 ` Philippe Mathieu-Daudé
@ 2020-06-26 14:03 ` BALATON Zoltan
2020-06-26 14:15 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 27+ messages in thread
From: BALATON Zoltan @ 2020-06-26 14:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, Eduardo Habkost,
Alistair Francis, Markus Armbruster, Jiaxun Yang, qemu-devel,
Aleksandar Markovic, qemu-ppc, Cédric Le Goater, Huacai Chen,
Fred Konrad, Philippe Mathieu-Daudé, David Gibson
[-- Attachment #1: Type: text/plain, Size: 3713 bytes --]
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-06-26 14:03 ` BALATON Zoltan
@ 2020-06-26 14:15 ` Philippe Mathieu-Daudé
2020-06-26 14:26 ` BALATON Zoltan
2020-07-09 20:05 ` Eduardo Habkost
0 siblings, 2 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 14:15 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Corey Minyard, Aleksandar Rikalo, Eduardo Habkost,
Alistair Francis, Markus Armbruster, Jiaxun Yang, qemu-devel,
Aleksandar Markovic, qemu-ppc, Cédric Le Goater, Huacai Chen,
Fred Konrad, Philippe Mathieu-Daudé, David Gibson
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
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
1 sibling, 1 reply; 27+ messages in thread
From: BALATON Zoltan @ 2020-06-26 14:26 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, Eduardo Habkost,
Alistair Francis, Markus Armbruster, Jiaxun Yang, qemu-devel,
Aleksandar Markovic, qemu-ppc, Cédric Le Goater, Huacai Chen,
Fred Konrad, Philippe Mathieu-Daudé, David Gibson
[-- Attachment #1: Type: text/plain, Size: 4765 bytes --]
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-06-26 14:26 ` BALATON Zoltan
@ 2020-06-26 14:37 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 14:37 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Corey Minyard, Aleksandar Rikalo, Eduardo Habkost,
Alistair Francis, Markus Armbruster, Jiaxun Yang, qemu-devel,
Aleksandar Markovic, qemu-ppc, Cédric Le Goater, Huacai Chen,
Fred Konrad, Philippe Mathieu-Daudé, David Gibson
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-06-26 14:15 ` Philippe Mathieu-Daudé
2020-06-26 14:26 ` BALATON Zoltan
@ 2020-07-09 20:05 ` Eduardo Habkost
2020-07-10 9:12 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 27+ messages in thread
From: Eduardo Habkost @ 2020-07-09 20:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Alistair Francis,
Markus Armbruster, Jiaxun Yang, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Huacai Chen, Fred Konrad,
Philippe Mathieu-Daudé, David Gibson
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.
--
Eduardo
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-07-09 20:05 ` Eduardo Habkost
@ 2020-07-10 9:12 ` Philippe Mathieu-Daudé
2020-07-10 20:09 ` Eduardo Habkost
0 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-10 9:12 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Alistair Francis,
Markus Armbruster, Jiaxun Yang, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Huacai Chen, Fred Konrad,
Philippe Mathieu-Daudé, David Gibson
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 1/3] hw/i2c/smbus_eeprom: Set QOM parent
2020-07-10 9:12 ` Philippe Mathieu-Daudé
@ 2020-07-10 20:09 ` Eduardo Habkost
0 siblings, 0 replies; 27+ messages in thread
From: Eduardo Habkost @ 2020-07-10 20:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Alistair Francis,
Markus Armbruster, Jiaxun Yang, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Huacai Chen, Fred Konrad,
Philippe Mathieu-Daudé, David Gibson
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?
--
Eduardo
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
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:27 ` Philippe Mathieu-Daudé
2020-06-26 11:00 ` BALATON Zoltan
2020-06-26 10:27 ` [RFC PATCH 3/3] hw/i2c/smbus_eeprom: Trace reset() event Philippe Mathieu-Daudé
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 10:27 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Corey Minyard, Aleksandar Rikalo, Philippe Mathieu-Daudé,
Jiaxun Yang, Aleksandar Markovic, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, Aurelien Jarno,
David Gibson
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/i2c/smbus_eeprom.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 879fd7c416..22ba7b20d4 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
uint8_t *init_data;
uint8_t offset;
bool accessed;
+ char *description;
} SMBusEEPROMDevice;
static uint8_t eeprom_receive_byte(SMBusDevice *dev)
@@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
smbus_eeprom_reset(dev);
if (eeprom->init_data == NULL) {
error_setg(errp, "init_data cannot be NULL");
+ return;
}
+ eeprom->description = object_get_canonical_path_component(OBJECT(dev));
}
static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
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é
0 siblings, 1 reply; 27+ messages in thread
From: BALATON Zoltan @ 2020-06-26 11:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, Markus Armbruster, Jiaxun Yang,
qemu-devel, Aleksandar Markovic, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, David Gibson
[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]
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>
> ---
> hw/i2c/smbus_eeprom.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 879fd7c416..22ba7b20d4 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
> uint8_t *init_data;
> uint8_t offset;
> bool accessed;
> + char *description;
> } SMBusEEPROMDevice;
>
> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> smbus_eeprom_reset(dev);
> if (eeprom->init_data == NULL) {
> error_setg(errp, "init_data cannot be NULL");
> + return;
> }
> + eeprom->description = object_get_canonical_path_component(OBJECT(dev));
> }
>
> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
What is this for? Shouldn't this description field be in some parent
object and whatever wants to print it could use the
object_get_canonical_path_component() as default value if it's not set
instead of adding more boiler plate to every object?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
2020-06-26 11:00 ` BALATON Zoltan
@ 2020-06-26 14:26 ` Philippe Mathieu-Daudé
2020-07-09 19:48 ` Eduardo Habkost
0 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 14:26 UTC (permalink / raw)
To: BALATON Zoltan, Eduardo Habkost
Cc: Corey Minyard, Aleksandar Rikalo, Markus Armbruster, Jiaxun Yang,
qemu-devel, Aleksandar Markovic, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, David Gibson
On 6/26/20 1:00 PM, 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>
>> ---
>> hw/i2c/smbus_eeprom.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index 879fd7c416..22ba7b20d4 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
>> uint8_t *init_data;
>> uint8_t offset;
>> bool accessed;
>> + char *description;
>> } SMBusEEPROMDevice;
>>
>> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>> @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev,
>> Error **errp)
>> smbus_eeprom_reset(dev);
>> if (eeprom->init_data == NULL) {
>> error_setg(errp, "init_data cannot be NULL");
>> + return;
>> }
>> + eeprom->description =
>> object_get_canonical_path_component(OBJECT(dev));
>> }
>>
>> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>
> What is this for? Shouldn't this description field be in some parent
> object and whatever wants to print it could use the
> object_get_canonical_path_component() as default value if it's not set
> instead of adding more boiler plate to every object?
You are right, if we want to use this field generically, it should be
a static Object field. I'll defer that question to Eduardo/Markus.
>
> Regards,
> BALATON Zoltan
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
2020-06-26 14:26 ` Philippe Mathieu-Daudé
@ 2020-07-09 19:48 ` Eduardo Habkost
2020-07-10 9:05 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 27+ messages in thread
From: Eduardo Habkost @ 2020-07-09 19:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Jiaxun Yang,
Markus Armbruster, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Huacai Chen, Philippe Mathieu-Daudé,
David Gibson
On Fri, Jun 26, 2020 at 04:26:33PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/26/20 1:00 PM, 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>
> >> ---
> >> hw/i2c/smbus_eeprom.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> >> index 879fd7c416..22ba7b20d4 100644
> >> --- a/hw/i2c/smbus_eeprom.c
> >> +++ b/hw/i2c/smbus_eeprom.c
> >> @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
> >> Â Â Â uint8_t *init_data;
> >> Â Â Â uint8_t offset;
> >> Â Â Â bool accessed;
> >> +Â Â Â char *description;
> >> } SMBusEEPROMDevice;
> >>
> >> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> >> @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev,
> >> Error **errp)
> >> Â Â Â smbus_eeprom_reset(dev);
> >> Â Â Â if (eeprom->init_data == NULL) {
> >> Â Â Â Â Â Â Â error_setg(errp, "init_data cannot be NULL");
> >> +Â Â Â Â Â Â Â return;
> >> Â Â Â }
> >> +Â Â Â eeprom->description =
> >> object_get_canonical_path_component(OBJECT(dev));
> >> }
> >>
> >> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> >
> > What is this for? Shouldn't this description field be in some parent
> > object and whatever wants to print it could use the
> > object_get_canonical_path_component() as default value if it's not set
> > instead of adding more boiler plate to every object?
>
> You are right, if we want to use this field generically, it should be
> a static Object field. I'll defer that question to Eduardo/Markus.
I don't understand what's the question here. What would be the
purpose of a static Object field, and why it would be better than
simply calling object_get_canonical_path_component() when you
need it?
--
Eduardo
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
2020-07-09 19:48 ` Eduardo Habkost
@ 2020-07-10 9:05 ` Philippe Mathieu-Daudé
2020-07-10 17:40 ` Eduardo Habkost
0 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-10 9:05 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Jiaxun Yang,
Markus Armbruster, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Stefan Hajnoczi, Huacai Chen,
Philippe Mathieu-Daudé, David Gibson
+Stefan for tracing PoV
On 7/9/20 9:48 PM, Eduardo Habkost wrote:
> On Fri, Jun 26, 2020 at 04:26:33PM +0200, Philippe Mathieu-Daudé wrote:
>> On 6/26/20 1:00 PM, 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>
>>>> ---
>>>> hw/i2c/smbus_eeprom.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>>> index 879fd7c416..22ba7b20d4 100644
>>>> --- a/hw/i2c/smbus_eeprom.c
>>>> +++ b/hw/i2c/smbus_eeprom.c
>>>> @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
>>>> Â Â Â uint8_t *init_data;
>>>> Â Â Â uint8_t offset;
>>>> Â Â Â bool accessed;
>>>> +Â Â Â char *description;
>>>> } SMBusEEPROMDevice;
>>>>
>>>> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>>>> @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev,
>>>> Error **errp)
>>>> Â Â Â smbus_eeprom_reset(dev);
>>>> Â Â Â if (eeprom->init_data == NULL) {
>>>> Â Â Â Â Â Â Â error_setg(errp, "init_data cannot be NULL");
>>>> +Â Â Â Â Â Â Â return;
>>>> Â Â Â }
>>>> +Â Â Â eeprom->description =
>>>> object_get_canonical_path_component(OBJECT(dev));
>>>> }
>>>>
>>>> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>>>
>>> What is this for? Shouldn't this description field be in some parent
>>> object and whatever wants to print it could use the
>>> object_get_canonical_path_component() as default value if it's not set
>>> instead of adding more boiler plate to every object?
>>
>> You are right, if we want to use this field generically, it should be
>> a static Object field. I'll defer that question to Eduardo/Markus.
>
> I don't understand what's the question here. What would be the
> purpose of a static Object field, and why it would be better than
> simply calling object_get_canonical_path_component() when you
> need it?
Because when reading a 8KB EEPROM with tracing enabled we end
up calling:
8192 g_hash_table_iter_init()
8192 g_hash_table_iter_next()
8192 object_property_iter_init()
8192 object_property_iter_next()
8192 g_hash_table_add()
8192 g_strdup()
8192 g_free()
Which is why I added the SMBusEEPROMDeviceState::description
field, to call it once and cache it. But Zoltan realized this
could benefit all QOM objects, not this single one.
So the question is, is it OK to make this a cached field
in object_get_canonical_path_component()?
Something like (incomplete):
-- >8 --
--- a/qom/object.c
+++ b/qom/object.c
@@ -642,6 +642,7 @@ static void object_property_del_child(Object *obj,
Object *child)
break;
}
}
+ g_free(child->canonical_path_component);
}
void object_unparent(Object *obj)
@@ -1666,6 +1667,7 @@ object_property_add_child(Object *obj, const char
*name,
op->resolve = object_resolve_child_property;
object_ref(child);
child->parent = obj;
+ child->canonical_path_component =
object_get_canonical_path_component(child);
return op;
}
@@ -1901,6 +1903,10 @@ char *object_get_canonical_path_component(const
Object *obj)
return NULL;
}
+ if (obj->canonical_path_component) {
+ return obj->canonical_path_component;
+ }
+
g_hash_table_iter_init(&iter, obj->parent->properties);
while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
if (!object_property_is_child(prop)) {
@@ -1908,7 +1914,8 @@ char *object_get_canonical_path_component(const
Object *obj)
}
if (prop->opaque == obj) {
- return g_strdup(prop->name);
+ obj->canonical_path_component_cached = g_strdup(prop->name);
+ return obj->canonical_path_component_cached;
}
}
---
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
2020-07-10 9:05 ` Philippe Mathieu-Daudé
@ 2020-07-10 17:40 ` Eduardo Habkost
2020-07-13 9:23 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Eduardo Habkost @ 2020-07-10 17:40 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Jiaxun Yang,
Markus Armbruster, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Stefan Hajnoczi, Huacai Chen,
Philippe Mathieu-Daudé, David Gibson
On Fri, Jul 10, 2020 at 11:05:29AM +0200, Philippe Mathieu-Daudé wrote:
> +Stefan for tracing PoV
>
> On 7/9/20 9:48 PM, Eduardo Habkost wrote:
> > On Fri, Jun 26, 2020 at 04:26:33PM +0200, Philippe Mathieu-Daudé wrote:
> >> On 6/26/20 1:00 PM, 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>
> >>>> ---
> >>>> hw/i2c/smbus_eeprom.c | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> >>>> index 879fd7c416..22ba7b20d4 100644
> >>>> --- a/hw/i2c/smbus_eeprom.c
> >>>> +++ b/hw/i2c/smbus_eeprom.c
> >>>> @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
> >>>>    uint8_t *init_data;
> >>>>    uint8_t offset;
> >>>>    bool accessed;
> >>>> +   char *description;
> >>>> } SMBusEEPROMDevice;
> >>>>
> >>>> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> >>>> @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev,
> >>>> Error **errp)
> >>>>    smbus_eeprom_reset(dev);
> >>>>    if (eeprom->init_data == NULL) {
> >>>>        error_setg(errp, "init_data cannot be NULL");
> >>>> +       return;
> >>>>    }
> >>>> +   eeprom->description =
> >>>> object_get_canonical_path_component(OBJECT(dev));
> >>>> }
> >>>>
> >>>> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> >>>
> >>> What is this for? Shouldn't this description field be in some parent
> >>> object and whatever wants to print it could use the
> >>> object_get_canonical_path_component() as default value if it's not set
> >>> instead of adding more boiler plate to every object?
> >>
> >> You are right, if we want to use this field generically, it should be
> >> a static Object field. I'll defer that question to Eduardo/Markus.
> >
> > I don't understand what's the question here. What would be the
> > purpose of a static Object field, and why it would be better than
> > simply calling object_get_canonical_path_component() when you
> > need it?
>
> Because when reading a 8KB EEPROM with tracing enabled we end
> up calling:
>
> 8192 g_hash_table_iter_init()
> 8192 g_hash_table_iter_next()
> 8192 object_property_iter_init()
> 8192 object_property_iter_next()
> 8192 g_hash_table_add()
> 8192 g_strdup()
> 8192 g_free()
>
> Which is why I added the SMBusEEPROMDeviceState::description
> field, to call it once and cache it. But Zoltan realized this
> could benefit all QOM objects, not this single one.
>
> So the question is, is it OK to make this a cached field
> in object_get_canonical_path_component()?
That's what I was thinking of, but now I see that
object_get_canonical_path_component() is an inconvenient API
because it requires callers to g_free() the return value.
We could change object_get_canonical_path_component() to not
require callers to call g_free(), or create a new mechanism to
get the object name like you suggested (and then get rid of most
of the existing uses of object_get_canonical_path_component()).
>
> Something like (incomplete):
>
> -- >8 --
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -642,6 +642,7 @@ static void object_property_del_child(Object *obj,
> Object *child)
> break;
> }
> }
> + g_free(child->canonical_path_component);
> }
>
> void object_unparent(Object *obj)
> @@ -1666,6 +1667,7 @@ object_property_add_child(Object *obj, const char
> *name,
> op->resolve = object_resolve_child_property;
> object_ref(child);
> child->parent = obj;
> + child->canonical_path_component =
> object_get_canonical_path_component(child);
> return op;
> }
>
> @@ -1901,6 +1903,10 @@ char *object_get_canonical_path_component(const
> Object *obj)
> return NULL;
> }
>
> + if (obj->canonical_path_component) {
> + return obj->canonical_path_component;
> + }
> +
> g_hash_table_iter_init(&iter, obj->parent->properties);
> while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
> if (!object_property_is_child(prop)) {
> @@ -1908,7 +1914,8 @@ char *object_get_canonical_path_component(const
> Object *obj)
> }
>
> if (prop->opaque == obj) {
> - return g_strdup(prop->name);
> + obj->canonical_path_component_cached = g_strdup(prop->name);
> + return obj->canonical_path_component_cached;
> }
> }
>
> ---
>
--
Eduardo
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
2020-07-10 17:40 ` Eduardo Habkost
@ 2020-07-13 9:23 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2020-07-13 9:23 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Corey Minyard, Aleksandar Rikalo, Philippe Mathieu-Daudé,
Jiaxun Yang, qemu-devel, Aleksandar Markovic, qemu-ppc,
Cédric Le Goater, Stefan Hajnoczi, Huacai Chen,
Philippe Mathieu-Daudé, David Gibson
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Fri, Jul 10, 2020 at 11:05:29AM +0200, Philippe Mathieu-Daudé wrote:
>> +Stefan for tracing PoV
>>
>> On 7/9/20 9:48 PM, Eduardo Habkost wrote:
>> > On Fri, Jun 26, 2020 at 04:26:33PM +0200, Philippe Mathieu-Daudé wrote:
>> >> On 6/26/20 1:00 PM, 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>
>> >>>> ---
>> >>>> hw/i2c/smbus_eeprom.c | 3 +++
>> >>>> 1 file changed, 3 insertions(+)
>> >>>>
>> >>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> >>>> index 879fd7c416..22ba7b20d4 100644
>> >>>> --- a/hw/i2c/smbus_eeprom.c
>> >>>> +++ b/hw/i2c/smbus_eeprom.c
>> >>>> @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
>> >>>>    uint8_t *init_data;
>> >>>>    uint8_t offset;
>> >>>>    bool accessed;
>> >>>> +   char *description;
>> >>>> } SMBusEEPROMDevice;
>> >>>>
>> >>>> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>> >>>> @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev,
>> >>>> Error **errp)
>> >>>>    smbus_eeprom_reset(dev);
>> >>>>    if (eeprom->init_data == NULL) {
>> >>>>        error_setg(errp, "init_data cannot be NULL");
>> >>>> +       return;
>> >>>>    }
>> >>>> +   eeprom->description =
>> >>>> object_get_canonical_path_component(OBJECT(dev));
>> >>>> }
>> >>>>
>> >>>> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>> >>>
>> >>> What is this for? Shouldn't this description field be in some parent
>> >>> object and whatever wants to print it could use the
>> >>> object_get_canonical_path_component() as default value if it's not set
>> >>> instead of adding more boiler plate to every object?
>> >>
>> >> You are right, if we want to use this field generically, it should be
>> >> a static Object field. I'll defer that question to Eduardo/Markus.
>> >
>> > I don't understand what's the question here. What would be the
>> > purpose of a static Object field, and why it would be better than
>> > simply calling object_get_canonical_path_component() when you
>> > need it?
>>
>> Because when reading a 8KB EEPROM with tracing enabled we end
>> up calling:
>>
>> 8192 g_hash_table_iter_init()
>> 8192 g_hash_table_iter_next()
>> 8192 object_property_iter_init()
>> 8192 object_property_iter_next()
>> 8192 g_hash_table_add()
>> 8192 g_strdup()
>> 8192 g_free()
>>
>> Which is why I added the SMBusEEPROMDeviceState::description
>> field, to call it once and cache it. But Zoltan realized this
>> could benefit all QOM objects, not this single one.
>>
>> So the question is, is it OK to make this a cached field
>> in object_get_canonical_path_component()?
>
> That's what I was thinking of, but now I see that
> object_get_canonical_path_component() is an inconvenient API
> because it requires callers to g_free() the return value.
I agree.
> We could change object_get_canonical_path_component() to not
> require callers to call g_free(),
I'll look into that. It would fix a memory leak I created because I
didn't expect object_get_canonical_path_component() to return a malloced
string.
> or create a new mechanism to
> get the object name like you suggested (and then get rid of most
> of the existing uses of object_get_canonical_path_component()).
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH 3/3] hw/i2c/smbus_eeprom: Trace reset() event
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:27 ` [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name Philippe Mathieu-Daudé
@ 2020-06-26 10:27 ` Philippe Mathieu-Daudé
2020-06-26 10:37 ` [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description no-reply
` (3 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 10:27 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Corey Minyard, Aleksandar Rikalo, Philippe Mathieu-Daudé,
Jiaxun Yang, Aleksandar Markovic, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, Aurelien Jarno,
David Gibson
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/i2c/smbus_eeprom.c | 2 ++
hw/i2c/trace-events | 3 +++
2 files changed, 5 insertions(+)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 22ba7b20d4..7a0e1e7455 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -31,6 +31,7 @@
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "hw/i2c/smbus_eeprom.h"
+#include "trace.h"
//#define DEBUG
@@ -124,6 +125,7 @@ static void smbus_eeprom_reset(DeviceState *dev)
{
SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
+ trace_smbus_eeprom_reset(eeprom->description);
memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
eeprom->offset = 0;
}
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 08db8fa689..0539c9e111 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -14,3 +14,6 @@ aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t val
aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
aspeed_i2c_bus_recv(const char *mode, int i, int count, uint8_t byte) "%s recv %d/%d 0x%02x"
+
+# smbus_eeprom.c
+smbus_eeprom_reset(const char *description) "'%s': reset"
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description
2020-06-26 10:27 [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-06-26 10:27 ` [RFC PATCH 3/3] hw/i2c/smbus_eeprom: Trace reset() event Philippe Mathieu-Daudé
@ 2020-06-26 10:37 ` no-reply
2020-06-26 12:43 ` Philippe Mathieu-Daudé
2020-06-26 10:38 ` no-reply
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: no-reply @ 2020-06-26 10:37 UTC (permalink / raw)
To: f4bug
Cc: cminyard, aleksandar.rikalo, qemu-devel, f4bug, jiaxun.yang,
armbru, aleksandar.qemu.devel, qemu-ppc, clg, chenhc, philmd,
aurelien, david
Patchew URL: https://patchew.org/QEMU/20200626102744.15053-1-f4bug@amsat.org/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC aarch64-softmmu/hw/arm/gumstix.o
CC aarch64-softmmu/hw/arm/spitz.o
/tmp/qemu-test/src/hw/i386/pc_q35.c: In function 'pc_q35_init':
/tmp/qemu-test/src/hw/i386/pc_q35.c:310:9: error: passing argument 1 of 'smbus_eeprom_init' from incompatible pointer type [-Werror]
smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
^
In file included from /tmp/qemu-test/src/hw/i386/pc_q35.c:35:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:6: note: expected 'struct Object *' but argument is of type 'struct I2CBus *'
void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
^
/tmp/qemu-test/src/hw/i386/pc_q35.c:310:9: error: passing argument 2 of 'smbus_eeprom_init' makes pointer from integer without a cast [-Werror]
smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
^
In file included from /tmp/qemu-test/src/hw/i386/pc_q35.c:35:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:6: note: expected 'const char *' but argument is of type 'int'
void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
^
/tmp/qemu-test/src/hw/i386/pc_q35.c:310:9: error: too few arguments to function 'smbus_eeprom_init'
smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
^
In file included from /tmp/qemu-test/src/hw/i386/pc_q35.c:35:0:
---
void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
^
cc1: all warnings being treated as errors
make[1]: *** [hw/i386/pc_q35.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/arm/tosa.o
CC aarch64-softmmu/hw/arm/z2.o
---
CC aarch64-softmmu/hw/arm/xilinx_zynq.o
CC aarch64-softmmu/hw/arm/sabrelite.o
/tmp/qemu-test/src/hw/i386/pc_piix.c: In function 'pc_init1':
/tmp/qemu-test/src/hw/i386/pc_piix.c:290:9: error: passing argument 1 of 'smbus_eeprom_init' from incompatible pointer type [-Werror]
smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
^
In file included from /tmp/qemu-test/src/hw/i386/pc_piix.c:48:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:6: note: expected 'struct Object *' but argument is of type 'struct I2CBus *'
void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
^
/tmp/qemu-test/src/hw/i386/pc_piix.c:290:9: error: passing argument 2 of 'smbus_eeprom_init' makes pointer from integer without a cast [-Werror]
smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
^
In file included from /tmp/qemu-test/src/hw/i386/pc_piix.c:48:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:6: note: expected 'const char *' but argument is of type 'int'
void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
^
/tmp/qemu-test/src/hw/i386/pc_piix.c:290:9: error: too few arguments to function 'smbus_eeprom_init'
smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
^
In file included from /tmp/qemu-test/src/hw/i386/pc_piix.c:48:0:
---
void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
^
cc1: all warnings being treated as errors
make[1]: *** [hw/i386/pc_piix.o] Error 1
CC aarch64-softmmu/hw/arm/armv7m.o
make: *** [x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/arm/exynos4210.o
CC aarch64-softmmu/hw/arm/pxa2xx.o
---
GEN aarch64-softmmu/target/arm/decode-t16.inc.c
CC aarch64-softmmu/target/arm/op_helper.o
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'palmetto_bmc_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:388:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct Object *' but argument is of type 'struct I2CBus *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:388:27: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'const char *' but argument is of type 'int'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:388:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct I2CBus *' but argument is of type 'uint8_t *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:388:27: error: too few arguments to function 'smbus_eeprom_init_one'
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
---
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'ast2500_evb_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:405:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct Object *' but argument is of type 'struct I2CBus *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:405:27: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'const char *' but argument is of type 'int'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:405:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct I2CBus *' but argument is of type 'uint8_t *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:405:27: error: too few arguments to function 'smbus_eeprom_init_one'
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
---
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'sonorapass_bmc_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:474:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom4_54);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct Object *' but argument is of type 'struct I2CBus *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:474:27: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror]
eeprom4_54);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'const char *' but argument is of type 'int'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:474:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom4_54);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct I2CBus *' but argument is of type 'uint8_t *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:474:27: error: too few arguments to function 'smbus_eeprom_init_one'
eeprom4_54);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: declared here
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:488:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom8_56);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct Object *' but argument is of type 'struct I2CBus *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:488:27: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror]
eeprom8_56);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'const char *' but argument is of type 'int'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:488:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom8_56);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct I2CBus *' but argument is of type 'uint8_t *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:488:27: error: too few arguments to function 'smbus_eeprom_init_one'
eeprom8_56);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
---
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'witherspoon_bmc_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:527:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct Object *' but argument is of type 'struct I2CBus *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:527:27: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'const char *' but argument is of type 'int'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:527:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror]
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: expected 'struct I2CBus *' but argument is of type 'uint8_t *'
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
/tmp/qemu-test/src/hw/arm/aspeed.c:527:27: error: too few arguments to function 'smbus_eeprom_init_one'
eeprom_buf);
^
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:0:
---
void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
^
cc1: all warnings being treated as errors
make[1]: *** [hw/arm/aspeed.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [aarch64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 669, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=68470e684744434cb3b024efd74e91b6', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-kjoqvac0/src/docker-src.2020-06-26-06.34.50.6138:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=68470e684744434cb3b024efd74e91b6
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-kjoqvac0/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 3m1.597s
user 0m8.290s
The full log is available at
http://patchew.org/logs/20200626102744.15053-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description
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é
0 siblings, 0 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 12:43 UTC (permalink / raw)
To: qemu-devel
Cc: patchew-devel, cminyard, aleksandar.rikalo, armbru, jiaxun.yang,
aleksandar.qemu.devel, qemu-ppc, clg, chenhc, philmd, aurelien,
david
On 6/26/20 12:37 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200626102744.15053-1-f4bug@amsat.org/
>
>
>
> Hi,
>
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
>
> CC aarch64-softmmu/hw/arm/gumstix.o
> CC aarch64-softmmu/hw/arm/spitz.o
> /tmp/qemu-test/src/hw/i386/pc_q35.c: In function 'pc_q35_init':
> /tmp/qemu-test/src/hw/i386/pc_q35.c:310:9: error: passing argument 1 of 'smbus_eeprom_init' from incompatible pointer type [-Werror]
> smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
> ^
This was announced in the patch.
Is there a way to give patchew a hint that it is pointless to build
a patch/series? Other CI services provides a such feature, i.e.
Travis-CI skips if a patch description contains '[ci skip]', we could
simply reuse that for patchew.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description
2020-06-26 10:27 [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-06-26 10:37 ` [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description no-reply
@ 2020-06-26 10:38 ` no-reply
2020-06-26 15:59 ` Aleksandar Markovic
2020-06-27 6:55 ` Markus Armbruster
6 siblings, 0 replies; 27+ messages in thread
From: no-reply @ 2020-06-26 10:38 UTC (permalink / raw)
To: f4bug
Cc: cminyard, aleksandar.rikalo, qemu-devel, f4bug, jiaxun.yang,
armbru, aleksandar.qemu.devel, qemu-ppc, clg, chenhc, philmd,
aurelien, david
Patchew URL: https://patchew.org/QEMU/20200626102744.15053-1-f4bug@amsat.org/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC x86_64-softmmu/target/i386/arch_memory_mapping.o
CC x86_64-softmmu/target/i386/arch_dump.o
/tmp/qemu-test/src/hw/i386/pc_q35.c: In function 'pc_q35_init':
/tmp/qemu-test/src/hw/i386/pc_q35.c:310:31: error: passing argument 1 of 'smbus_eeprom_init' from incompatible pointer type [-Werror=incompatible-pointer-types]
310 | smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
| ~~~~^~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:32: note: expected 'Object *' {aka 'struct Object *'} but argument is of type 'I2CBus *' {aka 'struct I2CBus *'}
32 | void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
| ~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/i386/pc_q35.c:310:40: error: passing argument 2 of 'smbus_eeprom_init' makes pointer from integer without a cast [-Werror=int-conversion]
310 | smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
| ^
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:56: note: expected 'const char *' but argument is of type 'int'
32 | void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/i386/pc_q35.c:310:9: error: too few arguments to function 'smbus_eeprom_init'
310 | smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
| ^~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/i386/pc_q35.c:35:
---
32 | void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
| ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/i386/pc_q35.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/arm/mainstone.o
CC aarch64-softmmu/hw/arm/microbit.o
---
CC aarch64-softmmu/hw/arm/realview.o
CC aarch64-softmmu/hw/arm/sbsa-ref.o
/tmp/qemu-test/src/hw/i386/pc_piix.c: In function 'pc_init1':
/tmp/qemu-test/src/hw/i386/pc_piix.c:290:31: error: passing argument 1 of 'smbus_eeprom_init' from incompatible pointer type [-Werror=incompatible-pointer-types]
290 | smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
| ~~~~^~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:32: note: expected 'Object *' {aka 'struct Object *'} but argument is of type 'I2CBus *' {aka 'struct I2CBus *'}
32 | void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
| ~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/i386/pc_piix.c:290:40: error: passing argument 2 of 'smbus_eeprom_init' makes pointer from integer without a cast [-Werror=int-conversion]
290 | smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
| ^
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:32:56: note: expected 'const char *' but argument is of type 'int'
32 | void smbus_eeprom_init(Object *parent_obj, const char *child_name_prefix,
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/i386/pc_piix.c:290:9: error: too few arguments to function 'smbus_eeprom_init'
290 | smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
| ^~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/i386/pc_piix.c:48:
---
| ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
CC aarch64-softmmu/hw/arm/stellaris.o
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/i386/pc_piix.o] Error 1
CC aarch64-softmmu/hw/arm/collie.o
make: *** [Makefile:527: x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
CC aarch64-softmmu/hw/arm/versatilepb.o
CC aarch64-softmmu/hw/arm/vexpress.o
---
CC aarch64-softmmu/trace/generated-helpers.o
CC aarch64-softmmu/target/arm/translate-sve.o
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'palmetto_bmc_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:387:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
387 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), 0x50,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:36: note: expected 'Object *' {aka 'struct Object *'} but argument is of type 'I2CBus *' {aka 'struct I2CBus *'}
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:387:69: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror=int-conversion]
387 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), 0x50,
| ^~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:60: note: expected 'const char *' but argument is of type 'int'
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:388:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
388 | eeprom_buf);
| ^~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:30:36: note: expected 'I2CBus *' {aka 'struct I2CBus *'} but argument is of type 'uint8_t *' {aka 'unsigned char *'}
30 | I2CBus *smbus, uint8_t address,
| ~~~~~~~~^~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:387:5: error: too few arguments to function 'smbus_eeprom_init_one'
387 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), 0x50,
| ^~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:
---
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ^~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'ast2500_evb_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:404:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
404 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), 0x50,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:36: note: expected 'Object *' {aka 'struct Object *'} but argument is of type 'I2CBus *' {aka 'struct I2CBus *'}
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:404:69: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror=int-conversion]
404 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), 0x50,
| ^~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:60: note: expected 'const char *' but argument is of type 'int'
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:405:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
405 | eeprom_buf);
| ^~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:30:36: note: expected 'I2CBus *' {aka 'struct I2CBus *'} but argument is of type 'uint8_t *' {aka 'unsigned char *'}
30 | I2CBus *smbus, uint8_t address,
| ~~~~~~~~^~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:404:5: error: too few arguments to function 'smbus_eeprom_init_one'
404 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), 0x50,
| ^~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:
---
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ^~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'sonorapass_bmc_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:473:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
473 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), 0x54,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:36: note: expected 'Object *' {aka 'struct Object *'} but argument is of type 'I2CBus *' {aka 'struct I2CBus *'}
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:473:69: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror=int-conversion]
473 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), 0x54,
| ^~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:60: note: expected 'const char *' but argument is of type 'int'
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:474:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
474 | eeprom4_54);
| ^~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:30:36: note: expected 'I2CBus *' {aka 'struct I2CBus *'} but argument is of type 'uint8_t *' {aka 'unsigned char *'}
30 | I2CBus *smbus, uint8_t address,
| ~~~~~~~~^~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:473:5: error: too few arguments to function 'smbus_eeprom_init_one'
473 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), 0x54,
| ^~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:6: note: declared here
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ^~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:487:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
487 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), 0x56,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:36: note: expected 'Object *' {aka 'struct Object *'} but argument is of type 'I2CBus *' {aka 'struct I2CBus *'}
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:487:69: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror=int-conversion]
487 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), 0x56,
| ^~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:60: note: expected 'const char *' but argument is of type 'int'
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:488:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
488 | eeprom8_56);
| ^~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:30:36: note: expected 'I2CBus *' {aka 'struct I2CBus *'} but argument is of type 'uint8_t *' {aka 'unsigned char *'}
30 | I2CBus *smbus, uint8_t address,
| ~~~~~~~~^~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:487:5: error: too few arguments to function 'smbus_eeprom_init_one'
487 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), 0x56,
| ^~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:
---
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ^~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c: In function 'witherspoon_bmc_i2c_init':
/tmp/qemu-test/src/hw/arm/aspeed.c:526:27: error: passing argument 1 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
526 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:36: note: expected 'Object *' {aka 'struct Object *'} but argument is of type 'I2CBus *' {aka 'struct I2CBus *'}
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:526:70: error: passing argument 2 of 'smbus_eeprom_init_one' makes pointer from integer without a cast [-Werror=int-conversion]
526 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
| ^~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:29:60: note: expected 'const char *' but argument is of type 'int'
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ~~~~~~~~~~~~^~~~~~~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:527:27: error: passing argument 3 of 'smbus_eeprom_init_one' from incompatible pointer type [-Werror=incompatible-pointer-types]
527 | eeprom_buf);
| ^~~~~~~~~~
| |
---
/tmp/qemu-test/src/include/hw/i2c/smbus_eeprom.h:30:36: note: expected 'I2CBus *' {aka 'struct I2CBus *'} but argument is of type 'uint8_t *' {aka 'unsigned char *'}
30 | I2CBus *smbus, uint8_t address,
| ~~~~~~~~^~~~~
/tmp/qemu-test/src/hw/arm/aspeed.c:526:5: error: too few arguments to function 'smbus_eeprom_init_one'
526 | smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
| ^~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/hw/arm/aspeed.c:20:
---
29 | void smbus_eeprom_init_one(Object *parent_obj, const char *child_name,
| ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/arm/aspeed.o] Error 1
make: *** [Makefile:527: aarch64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 669, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=aca6f125269d481b8b929d0f14068405', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-v5nrghk7/src/docker-src.2020-06-26-06.34.23.4621:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=aca6f125269d481b8b929d0f14068405
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-v5nrghk7/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 3m47.102s
user 0m8.605s
The full log is available at
http://patchew.org/logs/20200626102744.15053-1-f4bug@amsat.org/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description
2020-06-26 10:27 [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description Philippe Mathieu-Daudé
` (4 preceding siblings ...)
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
6 siblings, 1 reply; 27+ messages in thread
From: Aleksandar Markovic @ 2020-06-26 15:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, QEMU Developers,
Markus Armbruster, Jiaxun Yang, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, Aurelien Jarno,
David Gibson
пет, 26. јун 2020. у 12:27 Philippe Mathieu-Daudé <f4bug@amsat.org> је
написао/ла:
>
> This RFC is simply a proof-of-concept to see if I correctly
> understood Markus' suggestion, see the thread around:
> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08652.html
>
> Philippe Mathieu-Daudé (3):
> hw/i2c/smbus_eeprom: Set QOM parent
> hw/i2c/smbus_eeprom: Add description based on child name
> hw/i2c/smbus_eeprom: Trace reset() event
>
> include/hw/i2c/smbus_eeprom.h | 9 ++++++---
> hw/i2c/smbus_eeprom.c | 18 +++++++++++++++---
> hw/mips/fuloong2e.c | 2 +-
> hw/ppc/sam460ex.c | 2 +-
> hw/i2c/trace-events | 3 +++
> 5 files changed, 26 insertions(+), 8 deletions(-)
>
> --
Is there any documentation related to this interface? If yes, provide
the link, and describe what is not clear to you. If not, then this
series should provide appropriate documentation.
In times we desperately need working "Continuous Integration", it
looks we instead spend our time on "Continuous Interface Guessing" -
for years.
Thanks,
Aleksandar
> 2.21.3
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description
2020-06-26 15:59 ` Aleksandar Markovic
@ 2020-06-26 16:44 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-26 16:44 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Corey Minyard, Aleksandar Rikalo, QEMU Developers,
Markus Armbruster, Jiaxun Yang, qemu-ppc, Cédric Le Goater,
Huacai Chen, Philippe Mathieu-Daudé, Aurelien Jarno,
David Gibson
On 6/26/20 5:59 PM, Aleksandar Markovic wrote:
> пет, 26. јун 2020. у 12:27 Philippe Mathieu-Daudé <f4bug@amsat.org> је
> написао/ла:
>>
>> This RFC is simply a proof-of-concept to see if I correctly
>> understood Markus' suggestion, see the thread around:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08652.html
>>
>> Philippe Mathieu-Daudé (3):
>> hw/i2c/smbus_eeprom: Set QOM parent
>> hw/i2c/smbus_eeprom: Add description based on child name
>> hw/i2c/smbus_eeprom: Trace reset() event
>>
>> include/hw/i2c/smbus_eeprom.h | 9 ++++++---
>> hw/i2c/smbus_eeprom.c | 18 +++++++++++++++---
>> hw/mips/fuloong2e.c | 2 +-
>> hw/ppc/sam460ex.c | 2 +-
>> hw/i2c/trace-events | 3 +++
>> 5 files changed, 26 insertions(+), 8 deletions(-)
>>
>> --
>
> Is there any documentation related to this interface? If yes, provide
> the link, and describe what is not clear to you. If not, then this
> series should provide appropriate documentation.
It is described in "qom/object.h":
/**
* object_get_canonical_path_component:
*
* Returns: The final component in the object's canonical path. The
canonical
* path is the path within the composition tree starting from the root.
* %NULL if the object doesn't have a parent (and thus a canonical path).
*/
char *object_get_canonical_path_component(const Object *obj);
>
> In times we desperately need working "Continuous Integration", it
> looks we instead spend our time on "Continuous Interface Guessing" -
> for years.
>
> Thanks,
> Aleksandar
>
>
>
>> 2.21.3
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description
2020-06-26 10:27 [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2020-06-26 15:59 ` Aleksandar Markovic
@ 2020-06-27 6:55 ` Markus Armbruster
2020-06-29 11:38 ` Philippe Mathieu-Daudé
6 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2020-06-27 6:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Jiaxun Yang,
Aleksandar Markovic, qemu-ppc, Cédric Le Goater, Huacai Chen,
Philippe Mathieu-Daudé, Aurelien Jarno, David Gibson
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> This RFC is simply a proof-of-concept to see if I correctly
> understood Markus' suggestion, see the thread around:
> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08652.html
Please show us output of "info qom-tree" before and after. Feel free to
cut uninteresting parts.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 0/3] Use object_get_canonical_path_component to get child description
2020-06-27 6:55 ` Markus Armbruster
@ 2020-06-29 11:38 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 11:38 UTC (permalink / raw)
To: Markus Armbruster
Cc: Corey Minyard, Aleksandar Rikalo, qemu-devel, Jiaxun Yang,
qemu-ppc, Cédric Le Goater, Huacai Chen,
Philippe Mathieu-Daudé, Aurelien Jarno, David Gibson
On 6/27/20 8:55 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> This RFC is simply a proof-of-concept to see if I correctly
>> understood Markus' suggestion, see the thread around:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08652.html
>
> Please show us output of "info qom-tree" before and after. Feel free to
> cut uninteresting parts.
>
For 'info qom-tree' this is a one-line change, because ram_memdev
is not displayed in it:
/machine (sam460ex-machine)
/peripheral (container)
/peripheral-anon (container)
/unattached (container)
- /device[2] (smbus-eeprom)
^ permalink raw reply [flat|nested] 27+ messages in thread