* [PATCH] hw/i2c: flatten pca954x mux device
@ 2022-02-01 16:30 Patrick Venture
2022-02-01 19:02 ` Philippe Mathieu-Daudé via
0 siblings, 1 reply; 10+ messages in thread
From: Patrick Venture @ 2022-02-01 16:30 UTC (permalink / raw)
To: cminyard; +Cc: qemu-devel, Patrick Venture, Hao Wu
Previously this device created N subdevices which each owned an i2c bus.
Now this device simply owns the N i2c busses directly.
Tested: Verified devices behind mux are still accessible via qmp and i2c
from within an arm32 SoC.
Reviewed-by: Hao Wu <wuhaotsh@google.com>
Signed-off-by: Patrick Venture <venture@google.com>
---
hw/i2c/i2c_mux_pca954x.c | 75 ++++++----------------------------------
1 file changed, 11 insertions(+), 64 deletions(-)
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index 847c59921c..f9ce633b3a 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -30,24 +30,6 @@
#define PCA9548_CHANNEL_COUNT 8
#define PCA9546_CHANNEL_COUNT 4
-/*
- * struct Pca954xChannel - The i2c mux device will have N of these states
- * that own the i2c channel bus.
- * @bus: The owned channel bus.
- * @enabled: Is this channel active?
- */
-typedef struct Pca954xChannel {
- SysBusDevice parent;
-
- I2CBus *bus;
-
- bool enabled;
-} Pca954xChannel;
-
-#define TYPE_PCA954X_CHANNEL "pca954x-channel"
-#define PCA954X_CHANNEL(obj) \
- OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
-
/*
* struct Pca954xState - The pca954x state object.
* @control: The value written to the mux control.
@@ -59,8 +41,8 @@ typedef struct Pca954xState {
uint8_t control;
- /* The channel i2c buses. */
- Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
+ bool enabled[PCA9548_CHANNEL_COUNT];
+ I2CBus *bus[PCA9548_CHANNEL_COUNT];
} Pca954xState;
/*
@@ -98,11 +80,11 @@ static bool pca954x_match(I2CSlave *candidate, uint8_t address,
}
for (i = 0; i < mc->nchans; i++) {
- if (!mux->channel[i].enabled) {
+ if (!mux->enabled[i]) {
continue;
}
- if (i2c_scan_bus(mux->channel[i].bus, address, broadcast,
+ if (i2c_scan_bus(mux->bus[i], address, broadcast,
current_devs)) {
if (!broadcast) {
return true;
@@ -125,9 +107,9 @@ static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
*/
for (i = 0; i < mc->nchans; i++) {
if (enable_mask & (1 << i)) {
- s->channel[i].enabled = true;
+ s->enabled[i] = true;
} else {
- s->channel[i].enabled = false;
+ s->enabled[i] = false;
}
}
}
@@ -184,23 +166,7 @@ I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
Pca954xState *pca954x = PCA954X(mux);
g_assert(channel < pc->nchans);
- return I2C_BUS(qdev_get_child_bus(DEVICE(&pca954x->channel[channel]),
- "i2c-bus"));
-}
-
-static void pca954x_channel_init(Object *obj)
-{
- Pca954xChannel *s = PCA954X_CHANNEL(obj);
- s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
-
- /* Start all channels as disabled. */
- s->enabled = false;
-}
-
-static void pca954x_channel_class_init(ObjectClass *klass, void *data)
-{
- DeviceClass *dc = DEVICE_CLASS(klass);
- dc->desc = "Pca954x Channel";
+ return pca954x->bus[channel];
}
static void pca9546_class_init(ObjectClass *klass, void *data)
@@ -215,28 +181,17 @@ static void pca9548_class_init(ObjectClass *klass, void *data)
s->nchans = PCA9548_CHANNEL_COUNT;
}
-static void pca954x_realize(DeviceState *dev, Error **errp)
-{
- Pca954xState *s = PCA954X(dev);
- Pca954xClass *c = PCA954X_GET_CLASS(s);
- int i;
-
- /* SMBus modules. Cannot fail. */
- for (i = 0; i < c->nchans; i++) {
- sysbus_realize(SYS_BUS_DEVICE(&s->channel[i]), &error_abort);
- }
-}
-
static void pca954x_init(Object *obj)
{
Pca954xState *s = PCA954X(obj);
Pca954xClass *c = PCA954X_GET_CLASS(obj);
int i;
- /* Only initialize the children we expect. */
+ /* SMBus modules. Cannot fail. */
for (i = 0; i < c->nchans; i++) {
- object_initialize_child(obj, "channel[*]", &s->channel[i],
- TYPE_PCA954X_CHANNEL);
+ /* start all channels as disabled. */
+ s->enabled[i] = false;
+ s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
}
}
@@ -252,7 +207,6 @@ static void pca954x_class_init(ObjectClass *klass, void *data)
rc->phases.enter = pca954x_enter_reset;
dc->desc = "Pca954x i2c-mux";
- dc->realize = pca954x_realize;
k->write_data = pca954x_write_data;
k->receive_byte = pca954x_read_byte;
@@ -278,13 +232,6 @@ static const TypeInfo pca954x_info[] = {
.parent = TYPE_PCA954X,
.class_init = pca9548_class_init,
},
- {
- .name = TYPE_PCA954X_CHANNEL,
- .parent = TYPE_SYS_BUS_DEVICE,
- .class_init = pca954x_channel_class_init,
- .instance_size = sizeof(Pca954xChannel),
- .instance_init = pca954x_channel_init,
- }
};
DEFINE_TYPES(pca954x_info)
--
2.35.0.rc2.247.g8bbb082509-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-01 16:30 [PATCH] hw/i2c: flatten pca954x mux device Patrick Venture
@ 2022-02-01 19:02 ` Philippe Mathieu-Daudé via
2022-02-01 20:54 ` Patrick Venture
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-01 19:02 UTC (permalink / raw)
To: Patrick Venture, cminyard; +Cc: qemu-devel, Hao Wu
On 1/2/22 17:30, Patrick Venture wrote:
> Previously this device created N subdevices which each owned an i2c bus.
> Now this device simply owns the N i2c busses directly.
>
> Tested: Verified devices behind mux are still accessible via qmp and i2c
> from within an arm32 SoC.
>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
> hw/i2c/i2c_mux_pca954x.c | 75 ++++++----------------------------------
> 1 file changed, 11 insertions(+), 64 deletions(-)
> static void pca954x_init(Object *obj)
> {
> Pca954xState *s = PCA954X(obj);
> Pca954xClass *c = PCA954X_GET_CLASS(obj);
> int i;
>
> - /* Only initialize the children we expect. */
> + /* SMBus modules. Cannot fail. */
> for (i = 0; i < c->nchans; i++) {
> - object_initialize_child(obj, "channel[*]", &s->channel[i],
> - TYPE_PCA954X_CHANNEL);
> + /* start all channels as disabled. */
> + s->enabled[i] = false;
> + s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
This is not a QOM property, so you need to initialize manually:
-- >8 --
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index f9ce633b3a..a9517b612a 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
/* SMBus modules. Cannot fail. */
for (i = 0; i < c->nchans; i++) {
+ g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
+
/* start all channels as disabled. */
s->enabled[i] = false;
- s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
+ s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
}
}
---
(look at HMP 'info qtree' output).
> }
> }
With the change:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-01 19:02 ` Philippe Mathieu-Daudé via
@ 2022-02-01 20:54 ` Patrick Venture
2022-02-02 16:20 ` Philippe Mathieu-Daudé via
2022-02-02 16:34 ` Patrick Venture
0 siblings, 2 replies; 10+ messages in thread
From: Patrick Venture @ 2022-02-01 20:54 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, QEMU Developers, Hao Wu
[-- Attachment #1: Type: text/plain, Size: 2903 bytes --]
On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:
> On 1/2/22 17:30, Patrick Venture wrote:
> > Previously this device created N subdevices which each owned an i2c bus.
> > Now this device simply owns the N i2c busses directly.
> >
> > Tested: Verified devices behind mux are still accessible via qmp and i2c
> > from within an arm32 SoC.
> >
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> > hw/i2c/i2c_mux_pca954x.c | 75 ++++++----------------------------------
> > 1 file changed, 11 insertions(+), 64 deletions(-)
>
> > static void pca954x_init(Object *obj)
> > {
> > Pca954xState *s = PCA954X(obj);
> > Pca954xClass *c = PCA954X_GET_CLASS(obj);
> > int i;
> >
> > - /* Only initialize the children we expect. */
> > + /* SMBus modules. Cannot fail. */
> > for (i = 0; i < c->nchans; i++) {
> > - object_initialize_child(obj, "channel[*]", &s->channel[i],
> > - TYPE_PCA954X_CHANNEL);
> > + /* start all channels as disabled. */
> > + s->enabled[i] = false;
> > + s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>
> This is not a QOM property, so you need to initialize manually:
>
that was my suspicion but this is the output I'm seeing:
{'execute': 'qom-list', 'arguments': { 'path':
'/machine/soc/smbus[0]/i2c-bus/child[0]' }}
{"return": [
{"name": "type", "type": "string"},
{"name": "parent_bus", "type": "link<bus>"},
{"name": "realized", "type": "bool"},
{"name": "hotplugged", "type": "bool"},
{"name": "hotpluggable", "type": "bool"},
{"name": "address", "type": "uint8"},
{"name": "channel[3]", "type": "child<i2c-bus>"},
{"name": "channel[0]", "type": "child<i2c-bus>"},
{"name": "channel[1]", "type": "child<i2c-bus>"},
{"name": "channel[2]", "type": "child<i2c-bus>"}
]}
It seems to be naming them via the order they're created.
Is this not behaving how you expect?
>
> -- >8 --
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> index f9ce633b3a..a9517b612a 100644
> --- a/hw/i2c/i2c_mux_pca954x.c
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>
> /* SMBus modules. Cannot fail. */
> for (i = 0; i < c->nchans; i++) {
> + g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
> +
> /* start all channels as disabled. */
> s->enabled[i] = false;
> - s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
> + s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
> }
> }
>
> ---
>
> (look at HMP 'info qtree' output).
>
> > }
> > }
>
> With the change:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
[-- Attachment #2: Type: text/html, Size: 4575 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-01 20:54 ` Patrick Venture
@ 2022-02-02 16:20 ` Philippe Mathieu-Daudé via
2022-02-02 16:34 ` Patrick Venture
1 sibling, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-02 16:20 UTC (permalink / raw)
To: Patrick Venture; +Cc: Corey Minyard, QEMU Developers, Hao Wu
On 1/2/22 21:54, Patrick Venture wrote:
>
>
> On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé <f4bug@amsat.org
> <mailto:f4bug@amsat.org>> wrote:
>
> On 1/2/22 17:30, Patrick Venture wrote:
> > Previously this device created N subdevices which each owned an
> i2c bus.
> > Now this device simply owns the N i2c busses directly.
> >
> > Tested: Verified devices behind mux are still accessible via qmp
> and i2c
> > from within an arm32 SoC.
> >
> > Reviewed-by: Hao Wu <wuhaotsh@google.com
> <mailto:wuhaotsh@google.com>>
> > Signed-off-by: Patrick Venture <venture@google.com
> <mailto:venture@google.com>>
> > ---
> > hw/i2c/i2c_mux_pca954x.c | 75
> ++++++----------------------------------
> > 1 file changed, 11 insertions(+), 64 deletions(-)
>
> > static void pca954x_init(Object *obj)
> > {
> > Pca954xState *s = PCA954X(obj);
> > Pca954xClass *c = PCA954X_GET_CLASS(obj);
> > int i;
> >
> > - /* Only initialize the children we expect. */
> > + /* SMBus modules. Cannot fail. */
> > for (i = 0; i < c->nchans; i++) {
> > - object_initialize_child(obj, "channel[*]", &s->channel[i],
> > - TYPE_PCA954X_CHANNEL);
> > + /* start all channels as disabled. */
> > + s->enabled[i] = false;
> > + s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>
> This is not a QOM property, so you need to initialize manually:
>
>
> that was my suspicion but this is the output I'm seeing:
>
> {'execute': 'qom-list', 'arguments': { 'path':
> '/machine/soc/smbus[0]/i2c-bus/child[0]' }}
>
> {"return": [
> {"name": "type", "type": "string"},
> {"name": "parent_bus", "type": "link<bus>"},
> {"name": "realized", "type": "bool"},
> {"name": "hotplugged", "type": "bool"},
> {"name": "hotpluggable", "type": "bool"},
> {"name": "address", "type": "uint8"},
> {"name": "channel[3]", "type": "child<i2c-bus>"},
> {"name": "channel[0]", "type": "child<i2c-bus>"},
> {"name": "channel[1]", "type": "child<i2c-bus>"},
> {"name": "channel[2]", "type": "child<i2c-bus>"}
> ]}
>
> It seems to be naming them via the order they're created.
>
> Is this not behaving how you expect?
On the monitor:
(qemu) info qtree
bus: main-system-bus
type System
...
dev: npcm7xx-smbus, id ""
gpio-out "sysbus-irq" 1
mmio 00000000f008d000/0000000000001000
bus: i2c-bus
type i2c-bus
dev: pca9548, id ""
address = 119 (0x77)
bus: channel[*]
type i2c-bus
bus: channel[*]
type i2c-bus
bus: channel[*]
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 73 (0x49)
bus: channel[*]
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 72 (0x48)
bus: channel[*]
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 73 (0x49)
bus: channel[*]
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 72 (0x48)
bus: channel[*]
type i2c-bus
bus: channel[*]
type i2c-bus
> -- >8 --
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> index f9ce633b3a..a9517b612a 100644
> --- a/hw/i2c/i2c_mux_pca954x.c
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>
> /* SMBus modules. Cannot fail. */
> for (i = 0; i < c->nchans; i++) {
> + g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
> +
> /* start all channels as disabled. */
> s->enabled[i] = false;
> - s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
> + s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
> }
> }
>
> ---
>
> (look at HMP 'info qtree' output).
With this snippet:
(qemu) info qtree
bus: main-system-bus
type System
...
dev: npcm7xx-smbus, id ""
gpio-out "sysbus-irq" 1
mmio 00000000f008d000/0000000000001000
bus: i2c-bus
type i2c-bus
dev: pca9548, id ""
address = 119 (0x77)
bus: i2c.7
type i2c-bus
bus: i2c.6
type i2c-bus
bus: i2c.5
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 73 (0x49)
bus: i2c.4
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 72 (0x48)
bus: i2c.3
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 73 (0x49)
bus: i2c.2
type i2c-bus
dev: tmp105, id ""
gpio-out "" 1
address = 72 (0x48)
bus: i2c.1
type i2c-bus
bus: i2c.0
type i2c-bus
Regards,
Phil.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-01 20:54 ` Patrick Venture
2022-02-02 16:20 ` Philippe Mathieu-Daudé via
@ 2022-02-02 16:34 ` Patrick Venture
2022-02-02 16:40 ` Patrick Venture
1 sibling, 1 reply; 10+ messages in thread
From: Patrick Venture @ 2022-02-02 16:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, QEMU Developers, Hao Wu
[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]
On Tue, Feb 1, 2022 at 12:54 PM Patrick Venture <venture@google.com> wrote:
>
>
> On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
>
>> On 1/2/22 17:30, Patrick Venture wrote:
>> > Previously this device created N subdevices which each owned an i2c bus.
>> > Now this device simply owns the N i2c busses directly.
>> >
>> > Tested: Verified devices behind mux are still accessible via qmp and i2c
>> > from within an arm32 SoC.
>> >
>> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
>> > Signed-off-by: Patrick Venture <venture@google.com>
>> > ---
>> > hw/i2c/i2c_mux_pca954x.c | 75 ++++++----------------------------------
>> > 1 file changed, 11 insertions(+), 64 deletions(-)
>>
>> > static void pca954x_init(Object *obj)
>> > {
>> > Pca954xState *s = PCA954X(obj);
>> > Pca954xClass *c = PCA954X_GET_CLASS(obj);
>> > int i;
>> >
>> > - /* Only initialize the children we expect. */
>> > + /* SMBus modules. Cannot fail. */
>> > for (i = 0; i < c->nchans; i++) {
>> > - object_initialize_child(obj, "channel[*]", &s->channel[i],
>> > - TYPE_PCA954X_CHANNEL);
>> > + /* start all channels as disabled. */
>> > + s->enabled[i] = false;
>> > + s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>>
>> This is not a QOM property, so you need to initialize manually:
>>
>
> that was my suspicion but this is the output I'm seeing:
>
> {'execute': 'qom-list', 'arguments': { 'path':
> '/machine/soc/smbus[0]/i2c-bus/child[0]' }}
>
> {"return": [
> {"name": "type", "type": "string"},
> {"name": "parent_bus", "type": "link<bus>"},
> {"name": "realized", "type": "bool"},
> {"name": "hotplugged", "type": "bool"},
> {"name": "hotpluggable", "type": "bool"},
> {"name": "address", "type": "uint8"},
> {"name": "channel[3]", "type": "child<i2c-bus>"},
> {"name": "channel[0]", "type": "child<i2c-bus>"},
> {"name": "channel[1]", "type": "child<i2c-bus>"},
> {"name": "channel[2]", "type": "child<i2c-bus>"}
> ]}
>
> It seems to be naming them via the order they're created.
>
> Is this not behaving how you expect?
>
Philippe,
I0202 08:29:45.380384 6641 stream.go:31] qemu: child buses at "pca9546":
"channel[*]", "channel[*]", "channel[*]", "channel[*]"
Ok, so that's interesting. In one system (using qom-list) it's correct,
but then when using it to do path assignment (qdev-monitor), it fails...
I'm not as fond of the name i2c-bus.%d, since they're referred to as
channels in the datasheet. If I do the manual name creation, can I keep
the name channel or should I pivot over?
Thanks
>
>>
>> -- >8 --
>> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
>> index f9ce633b3a..a9517b612a 100644
>> --- a/hw/i2c/i2c_mux_pca954x.c
>> +++ b/hw/i2c/i2c_mux_pca954x.c
>> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>>
>> /* SMBus modules. Cannot fail. */
>> for (i = 0; i < c->nchans; i++) {
>> + g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
>> +
>> /* start all channels as disabled. */
>> s->enabled[i] = false;
>> - s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>> + s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
>> }
>> }
>>
>> ---
>>
>> (look at HMP 'info qtree' output).
>>
>> > }
>> > }
>>
>> With the change:
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>
[-- Attachment #2: Type: text/html, Size: 5811 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-02 16:34 ` Patrick Venture
@ 2022-02-02 16:40 ` Patrick Venture
2022-02-02 17:30 ` Philippe Mathieu-Daudé via
2022-02-02 19:01 ` Peter Maydell
0 siblings, 2 replies; 10+ messages in thread
From: Patrick Venture @ 2022-02-02 16:40 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, QEMU Developers, Hao Wu
[-- Attachment #1: Type: text/plain, Size: 4301 bytes --]
On Wed, Feb 2, 2022 at 8:34 AM Patrick Venture <venture@google.com> wrote:
>
>
> On Tue, Feb 1, 2022 at 12:54 PM Patrick Venture <venture@google.com>
> wrote:
>
>>
>>
>> On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
>> wrote:
>>
>>> On 1/2/22 17:30, Patrick Venture wrote:
>>> > Previously this device created N subdevices which each owned an i2c
>>> bus.
>>> > Now this device simply owns the N i2c busses directly.
>>> >
>>> > Tested: Verified devices behind mux are still accessible via qmp and
>>> i2c
>>> > from within an arm32 SoC.
>>> >
>>> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
>>> > Signed-off-by: Patrick Venture <venture@google.com>
>>> > ---
>>> > hw/i2c/i2c_mux_pca954x.c | 75
>>> ++++++----------------------------------
>>> > 1 file changed, 11 insertions(+), 64 deletions(-)
>>>
>>> > static void pca954x_init(Object *obj)
>>> > {
>>> > Pca954xState *s = PCA954X(obj);
>>> > Pca954xClass *c = PCA954X_GET_CLASS(obj);
>>> > int i;
>>> >
>>> > - /* Only initialize the children we expect. */
>>> > + /* SMBus modules. Cannot fail. */
>>> > for (i = 0; i < c->nchans; i++) {
>>> > - object_initialize_child(obj, "channel[*]", &s->channel[i],
>>> > - TYPE_PCA954X_CHANNEL);
>>> > + /* start all channels as disabled. */
>>> > + s->enabled[i] = false;
>>> > + s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>>>
>>> This is not a QOM property, so you need to initialize manually:
>>>
>>
>> that was my suspicion but this is the output I'm seeing:
>>
>> {'execute': 'qom-list', 'arguments': { 'path':
>> '/machine/soc/smbus[0]/i2c-bus/child[0]' }}
>>
>> {"return": [
>> {"name": "type", "type": "string"},
>> {"name": "parent_bus", "type": "link<bus>"},
>> {"name": "realized", "type": "bool"},
>> {"name": "hotplugged", "type": "bool"},
>> {"name": "hotpluggable", "type": "bool"},
>> {"name": "address", "type": "uint8"},
>> {"name": "channel[3]", "type": "child<i2c-bus>"},
>> {"name": "channel[0]", "type": "child<i2c-bus>"},
>> {"name": "channel[1]", "type": "child<i2c-bus>"},
>> {"name": "channel[2]", "type": "child<i2c-bus>"}
>> ]}
>>
>> It seems to be naming them via the order they're created.
>>
>> Is this not behaving how you expect?
>>
>
> Philippe,
>
> I0202 08:29:45.380384 6641 stream.go:31] qemu: child buses at "pca9546":
> "channel[*]", "channel[*]", "channel[*]", "channel[*]"
>
> Ok, so that's interesting. In one system (using qom-list) it's correct,
> but then when using it to do path assignment (qdev-monitor), it fails...
>
> I'm not as fond of the name i2c-bus.%d, since they're referred to as
> channels in the datasheet. If I do the manual name creation, can I keep
> the name channel or should I pivot over?
>
> Thanks
>
>
>>
>>>
>>> -- >8 --
>>> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
>>> index f9ce633b3a..a9517b612a 100644
>>> --- a/hw/i2c/i2c_mux_pca954x.c
>>> +++ b/hw/i2c/i2c_mux_pca954x.c
>>> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>>>
>>> /* SMBus modules. Cannot fail. */
>>> for (i = 0; i < c->nchans; i++) {
>>> + g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
>>> +
>>> /* start all channels as disabled. */
>>> s->enabled[i] = false;
>>> - s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>>> + s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
>>> }
>>> }
>>>
>>> ---
>>>
>>> (look at HMP 'info qtree' output).
>>>
>>> > }
>>> > }
>>>
>>> With the change:
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>
Just saw your reply, and found a bunch of other non-spam in my spam
folder. I sent the message to the anti-spam team, hopefully that'll
resolve this for myself and presumably others.
I definitely see the same result with the qdev-monitor, but was really
surprised that the qom-list worked. I'll explicitly set the name, and
i2c.%d is fine. The detail that they're channels is not really important
to the end user presumably.
I'll have v2 out shortly.
thanks,
Patrick
[-- Attachment #2: Type: text/html, Size: 6803 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-02 16:40 ` Patrick Venture
@ 2022-02-02 17:30 ` Philippe Mathieu-Daudé via
2022-02-02 21:30 ` Patrick Venture
2022-02-02 19:01 ` Peter Maydell
1 sibling, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-02 17:30 UTC (permalink / raw)
To: Patrick Venture; +Cc: Corey Minyard, QEMU Developers, Hao Wu
On 2/2/22 17:40, Patrick Venture wrote:
> Philippe,
>
> I0202 08:29:45.380384 6641 stream.go:31] qemu: child buses at
> "pca9546": "channel[*]", "channel[*]", "channel[*]", "channel[*]"
>
> Ok, so that's interesting. In one system (using qom-list) it's
> correct, but then when using it to do path assignment
> (qdev-monitor), it fails...
>
> I'm not as fond of the name i2c-bus.%d, since they're referred to as
> channels in the datasheet. If I do the manual name creation, can I
> keep the name channel or should I pivot over?
>
> Thanks
>
>
> -- >8 --
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> index f9ce633b3a..a9517b612a 100644
> --- a/hw/i2c/i2c_mux_pca954x.c
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>
> /* SMBus modules. Cannot fail. */
> for (i = 0; i < c->nchans; i++) {
> + g_autofree gchar *bus_name =
> g_strdup_printf("i2c.%d", i);
> +
> /* start all channels as disabled. */
> s->enabled[i] = false;
> - s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
> + s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
> }
> }
>
> ---
>
> (look at HMP 'info qtree' output).
>
> > }
> > }
>
> With the change:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org
> <mailto:f4bug@amsat.org>>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org
> <mailto:f4bug@amsat.org>>
>
>
> Just saw your reply, and found a bunch of other non-spam in my spam
> folder. I sent the message to the anti-spam team, hopefully that'll
> resolve this for myself and presumably others.
Thanks. I suppose the problem is the amsat.org domain.
> I definitely see the same result with the qdev-monitor, but was really
> surprised that the qom-list worked. I'll explicitly set the name, and
> i2c.%d is fine. The detail that they're channels is not really
> important to the end user presumably.
I agree it is better to follow datasheets, thus I am fine if you
change and use channel. How would it look like? "channel.0"?
FYI qdev busses are described in docs/qdev-device-use.txt.
We should be able to plug a device using some command line
such "-device i2c_test_dev,bus=channel.0,addr=0x55".
I wonder how to select the base PCA9548 ...
Maybe we need to pass the PCA ID to pca954x_init(), so we can
name "channel.2.0" for the 1st channel on the 2nd PCA?
Regards,
Phil.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-02 17:30 ` Philippe Mathieu-Daudé via
@ 2022-02-02 21:30 ` Patrick Venture
0 siblings, 0 replies; 10+ messages in thread
From: Patrick Venture @ 2022-02-02 21:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, QEMU Developers, Hao Wu
[-- Attachment #1: Type: text/plain, Size: 3673 bytes --]
On Wed, Feb 2, 2022 at 9:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:
> On 2/2/22 17:40, Patrick Venture wrote:
>
> > Philippe,
> >
> > I0202 08:29:45.380384 6641 stream.go:31] qemu: child buses at
> > "pca9546": "channel[*]", "channel[*]", "channel[*]", "channel[*]"
> >
> > Ok, so that's interesting. In one system (using qom-list) it's
> > correct, but then when using it to do path assignment
> > (qdev-monitor), it fails...
> >
> > I'm not as fond of the name i2c-bus.%d, since they're referred to as
> > channels in the datasheet. If I do the manual name creation, can I
> > keep the name channel or should I pivot over?
> >
> > Thanks
> >
> >
> > -- >8 --
> > diff --git a/hw/i2c/i2c_mux_pca954x.c
> b/hw/i2c/i2c_mux_pca954x.c
> > index f9ce633b3a..a9517b612a 100644
> > --- a/hw/i2c/i2c_mux_pca954x.c
> > +++ b/hw/i2c/i2c_mux_pca954x.c
> > @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
> >
> > /* SMBus modules. Cannot fail. */
> > for (i = 0; i < c->nchans; i++) {
> > + g_autofree gchar *bus_name =
> > g_strdup_printf("i2c.%d", i);
> > +
> > /* start all channels as disabled. */
> > s->enabled[i] = false;
> > - s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
> > + s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
> > }
> > }
> >
> > ---
> >
> > (look at HMP 'info qtree' output).
> >
> > > }
> > > }
> >
> > With the change:
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org
> > <mailto:f4bug@amsat.org>>
> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org
> > <mailto:f4bug@amsat.org>>
> >
> >
> > Just saw your reply, and found a bunch of other non-spam in my spam
> > folder. I sent the message to the anti-spam team, hopefully that'll
> > resolve this for myself and presumably others.
>
> Thanks. I suppose the problem is the amsat.org domain.
>
Yours aren't the only ones I've missed, but who knows.
>
> > I definitely see the same result with the qdev-monitor, but was really
> > surprised that the qom-list worked. I'll explicitly set the name, and
> > i2c.%d is fine. The detail that they're channels is not really
> > important to the end user presumably.
>
> I agree it is better to follow datasheets, thus I am fine if you
> change and use channel. How would it look like? "channel.0"?
> FYI qdev busses are described in docs/qdev-device-use.txt.
>
Thanks. I went with i2c.%d in v2, since I figured it wasn't super
important.
>
> We should be able to plug a device using some command line
> such "-device i2c_test_dev,bus=channel.0,addr=0x55".
> I wonder how to select the base PCA9548 ...
>
So I have been working on that, and I have been running into a different
issue, but related.
/smbus[1]/i2c-bus/pca9546/i2c.0 works to add a device via command line.
However, if there are two pca9546s on that main bus. So if i do:
/smbus[1]/i2c-bus/child[0]/i2c.0 it'll respond that there is no child[0],
but rather includes "pca9546, pca9546"
>
> Maybe we need to pass the PCA ID to pca954x_init(), so we can
> name "channel.2.0" for the 1st channel on the 2nd PCA?
>
It sounds like you're thinking about the same problem overall.
>
> Regards,
>
> Phil.
>
[-- Attachment #2: Type: text/html, Size: 5548 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-02 16:40 ` Patrick Venture
2022-02-02 17:30 ` Philippe Mathieu-Daudé via
@ 2022-02-02 19:01 ` Peter Maydell
2022-02-02 19:53 ` Patrick Venture
1 sibling, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2022-02-02 19:01 UTC (permalink / raw)
To: Patrick Venture
Cc: Hao Wu, Corey Minyard, Philippe Mathieu-Daudé, QEMU Developers
On Wed, 2 Feb 2022 at 17:36, Patrick Venture <venture@google.com> wrote:
> Just saw your reply, and found a bunch of other non-spam in my
> spam folder. I sent the message to the anti-spam team, hopefully
> that'll resolve this for myself and presumably others.
I dunno if you folk get a specially tuned version or just
the standard gmail spam filter, but IME it's not very good
with mailing list traffic. In particular "this is a patch"
should be a really really easy thing to detect as not-spam
but it doesn't always manage it. I have my filters set to
"Do not send to spam" for mailing list traffic...
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/i2c: flatten pca954x mux device
2022-02-02 19:01 ` Peter Maydell
@ 2022-02-02 19:53 ` Patrick Venture
0 siblings, 0 replies; 10+ messages in thread
From: Patrick Venture @ 2022-02-02 19:53 UTC (permalink / raw)
To: Peter Maydell
Cc: Philippe Mathieu-Daudé, Corey Minyard, QEMU Developers, Hao Wu
[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]
On Wed, Feb 2, 2022 at 11:01 AM Peter Maydell <peter.maydell@linaro.org>
wrote:
> On Wed, 2 Feb 2022 at 17:36, Patrick Venture <venture@google.com> wrote:
> > Just saw your reply, and found a bunch of other non-spam in my
> > spam folder. I sent the message to the anti-spam team, hopefully
> > that'll resolve this for myself and presumably others.
>
> I dunno if you folk get a specially tuned version or just
> the standard gmail spam filter, but IME it's not very good
> with mailing list traffic. In particular "this is a patch"
> should be a really really easy thing to detect as not-spam
> but it doesn't always manage it. I have my filters set to
> "Do not send to spam" for mailing list traffic...
>
I'm sure we have some dogfood version. I have a rule set to all
mailing list from qemu-devel to go into a label and everything... but it
gets the label and then is sometimes sent right to spam, even in messages
where it's an active thread (like this one). And I just saw some other
messages I missed.
>
> -- PMM
>
[-- Attachment #2: Type: text/html, Size: 1663 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-02-02 21:32 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-01 16:30 [PATCH] hw/i2c: flatten pca954x mux device Patrick Venture
2022-02-01 19:02 ` Philippe Mathieu-Daudé via
2022-02-01 20:54 ` Patrick Venture
2022-02-02 16:20 ` Philippe Mathieu-Daudé via
2022-02-02 16:34 ` Patrick Venture
2022-02-02 16:40 ` Patrick Venture
2022-02-02 17:30 ` Philippe Mathieu-Daudé via
2022-02-02 21:30 ` Patrick Venture
2022-02-02 19:01 ` Peter Maydell
2022-02-02 19:53 ` Patrick Venture
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.