| Message ID | 20220201163005.989457-1-venture@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i2c: flatten pca954x mux device | expand |
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> >
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> >> >
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
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
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 >
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. >
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)