| Message ID | 20200620225854.31160-5-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/misc/pca9552: Trace GPIO change events | expand |
On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: > Add a description field to distinguish between multiple devices. Reviewed-by: Cédric Le Goater <clg@kaod.org> Could it be a QOM attribute ? C. > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/misc/pca9552.h | 1 + > hw/misc/pca9552.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h > index ef6da4988f..c5be7f1c5e 100644 > --- a/include/hw/misc/pca9552.h > +++ b/include/hw/misc/pca9552.h > @@ -27,6 +27,7 @@ typedef struct PCA9552State { > > uint8_t regs[PCA9552_NR_REGS]; > uint8_t max_reg; > + char *description; /* For debugging purpose only */ > uint8_t nr_leds; > } PCA9552State; > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > index b97fc2893c..54ccdcf6d4 100644 > --- a/hw/misc/pca9552.c > +++ b/hw/misc/pca9552.c > @@ -12,6 +12,7 @@ > #include "qemu/osdep.h" > #include "qemu/log.h" > #include "qemu/module.h" > +#include "hw/qdev-properties.h" > #include "hw/misc/pca9552.h" > #include "hw/misc/pca9552_regs.h" > #include "migration/vmstate.h" > @@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp) > __func__, s->nr_leds, PCA9552_PIN_COUNT); > return; > } > + if (!s->description) { > + s->description = g_strdup("pca-unspecified"); > + } > } > > +static Property pca9552_properties[] = { > + DEFINE_PROP_STRING("description", PCA9552State, description), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void pca9552_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data) > dc->realize = pca9552_realize; > dc->reset = pca9552_reset; > dc->vmsd = &pca9552_vmstate; > + device_class_set_props(dc, pca9552_properties); > } > > static const TypeInfo pca9552_info = { >
On 6/22/20 8:27 AM, Cédric Le Goater wrote: > On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >> Add a description field to distinguish between multiple devices. > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > Could it be a QOM attribute ? What do you call a 'QOM attribute'? Is it what qdev properties implement? (in this case via DEFINE_PROP_STRING). > > C. > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/hw/misc/pca9552.h | 1 + >> hw/misc/pca9552.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h >> index ef6da4988f..c5be7f1c5e 100644 >> --- a/include/hw/misc/pca9552.h >> +++ b/include/hw/misc/pca9552.h >> @@ -27,6 +27,7 @@ typedef struct PCA9552State { >> >> uint8_t regs[PCA9552_NR_REGS]; >> uint8_t max_reg; >> + char *description; /* For debugging purpose only */ >> uint8_t nr_leds; >> } PCA9552State; >> >> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c >> index b97fc2893c..54ccdcf6d4 100644 >> --- a/hw/misc/pca9552.c >> +++ b/hw/misc/pca9552.c >> @@ -12,6 +12,7 @@ >> #include "qemu/osdep.h" >> #include "qemu/log.h" >> #include "qemu/module.h" >> +#include "hw/qdev-properties.h" >> #include "hw/misc/pca9552.h" >> #include "hw/misc/pca9552_regs.h" >> #include "migration/vmstate.h" >> @@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp) >> __func__, s->nr_leds, PCA9552_PIN_COUNT); >> return; >> } >> + if (!s->description) { >> + s->description = g_strdup("pca-unspecified"); >> + } >> } >> >> +static Property pca9552_properties[] = { >> + DEFINE_PROP_STRING("description", PCA9552State, description), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> static void pca9552_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> @@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data) >> dc->realize = pca9552_realize; >> dc->reset = pca9552_reset; >> dc->vmsd = &pca9552_vmstate; >> + device_class_set_props(dc, pca9552_properties); >> } >> >> static const TypeInfo pca9552_info = { >> > >
On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote: > On 6/22/20 8:27 AM, Cédric Le Goater wrote: >> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >>> Add a description field to distinguish between multiple devices. >> >> Reviewed-by: Cédric Le Goater <clg@kaod.org> >> >> Could it be a QOM attribute ? > > What do you call a 'QOM attribute'? > Is it what qdev properties implement? > (in this case via DEFINE_PROP_STRING). I meant a default Object property, which would apply to all Objects. What you did is fine, so : Reviewed-by: Cédric Le Goater <clg@kaod.org> but, may be, a well defined child name is enough for the purpose. C. > >> >> C. >> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> include/hw/misc/pca9552.h | 1 + >>> hw/misc/pca9552.c | 10 ++++++++++ >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h >>> index ef6da4988f..c5be7f1c5e 100644 >>> --- a/include/hw/misc/pca9552.h >>> +++ b/include/hw/misc/pca9552.h >>> @@ -27,6 +27,7 @@ typedef struct PCA9552State { >>> >>> uint8_t regs[PCA9552_NR_REGS]; >>> uint8_t max_reg; >>> + char *description; /* For debugging purpose only */ >>> uint8_t nr_leds; >>> } PCA9552State; >>> >>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c >>> index b97fc2893c..54ccdcf6d4 100644 >>> --- a/hw/misc/pca9552.c >>> +++ b/hw/misc/pca9552.c >>> @@ -12,6 +12,7 @@ >>> #include "qemu/osdep.h" >>> #include "qemu/log.h" >>> #include "qemu/module.h" >>> +#include "hw/qdev-properties.h" >>> #include "hw/misc/pca9552.h" >>> #include "hw/misc/pca9552_regs.h" >>> #include "migration/vmstate.h" >>> @@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp) >>> __func__, s->nr_leds, PCA9552_PIN_COUNT); >>> return; >>> } >>> + if (!s->description) { >>> + s->description = g_strdup("pca-unspecified"); >>> + } >>> } >>> >>> +static Property pca9552_properties[] = { >>> + DEFINE_PROP_STRING("description", PCA9552State, description), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> static void pca9552_class_init(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> @@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data) >>> dc->realize = pca9552_realize; >>> dc->reset = pca9552_reset; >>> dc->vmsd = &pca9552_vmstate; >>> + device_class_set_props(dc, pca9552_properties); >>> } >>> >>> static const TypeInfo pca9552_info = { >>> >> >>
Cédric Le Goater <clg@kaod.org> writes: > On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote: >> On 6/22/20 8:27 AM, Cédric Le Goater wrote: >>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >>>> Add a description field to distinguish between multiple devices. Pardon my lack of imagination: how does this help you with debugging? >>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>> >>> Could it be a QOM attribute ? >> >> What do you call a 'QOM attribute'? >> Is it what qdev properties implement? >> (in this case via DEFINE_PROP_STRING). > > I meant a default Object property, which would apply to all Objects. Good point. Many devices have multiple component objects of the same type. > What you did is fine, so : > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > but, may be, a well defined child name is enough for the purpose. object_get_canonical_path() returns a distinct path for each (component) object. The path components are the child property names. Properties can have descriptions: object_property_set_description(). Sufficient?
On 6/25/20 8:37 AM, Markus Armbruster wrote: > Cédric Le Goater <clg@kaod.org> writes: > >> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote: >>> On 6/22/20 8:27 AM, Cédric Le Goater wrote: >>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >>>>> Add a description field to distinguish between multiple devices. > > Pardon my lack of imagination: how does this help you with debugging? Ah, the patch subject is indeed incorrect, this should be: "... for *tracing* purpose" (I use tracing when debugging). In the next patch, we use the 'description' property: +# pca9552.c +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]" So when the machine has multiple PCA9552 and guest accesses both, we can distinct which one is used. For me having "pca1" / "pca0" is easier to follow than the address of the QOM object. > >>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>> >>>> Could it be a QOM attribute ? >>> >>> What do you call a 'QOM attribute'? >>> Is it what qdev properties implement? >>> (in this case via DEFINE_PROP_STRING). >> >> I meant a default Object property, which would apply to all Objects. > > Good point. Many devices have multiple component objects of the same > type. > >> What you did is fine, so : >> >> Reviewed-by: Cédric Le Goater <clg@kaod.org> >> >> but, may be, a well defined child name is enough for the purpose. > > object_get_canonical_path() returns a distinct path for each (component) > object. The path components are the child property names. > > Properties can have descriptions: object_property_set_description(). TIL object_property_set_description :> Ah, there is no equivalent object_property_get_description(), we have to use object_get_canonical_path(). Hmm, not obvious. > > Sufficient? I don't know... This seems a complex way to do something simple... This is already a QDEV. Having to use QOM API seems going backward, since we have the DEFINE_PROP_STRING() macros available in "hw/qdev-properties.h". Maybe I'm not seeing the advantages clearly. I'll try later. Thanks for your review, Phil.
On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote: > On 6/25/20 8:37 AM, Markus Armbruster wrote: >> Cédric Le Goater <clg@kaod.org> writes: >> >>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote: >>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote: >>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >>>>>> Add a description field to distinguish between multiple devices. >> >> Pardon my lack of imagination: how does this help you with debugging? > > Ah, the patch subject is indeed incorrect, this should be: > "... for *tracing* purpose" (I use tracing when debugging). > > In the next patch, we use the 'description' property: > > +# pca9552.c > +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs > 0-15 [%s]" > > So when the machine has multiple PCA9552 and guest accesses both, > we can distinct which one is used. For me having "pca1" / "pca0" > is easier to follow than the address of the QOM object. > >> >>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>> >>>>> Could it be a QOM attribute ? >>>> >>>> What do you call a 'QOM attribute'? >>>> Is it what qdev properties implement? >>>> (in this case via DEFINE_PROP_STRING). >>> >>> I meant a default Object property, which would apply to all Objects. >> >> Good point. Many devices have multiple component objects of the same >> type. >> >>> What you did is fine, so : >>> >>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>> >>> but, may be, a well defined child name is enough for the purpose. >> >> object_get_canonical_path() returns a distinct path for each (component) >> object. The path components are the child property names. >> >> Properties can have descriptions: object_property_set_description(). > > TIL object_property_set_description :> > > Ah, there is no equivalent object_property_get_description(), > we have to use object_get_canonical_path(). Hmm, not obvious. > >> >> Sufficient? > > I don't know... This seems a complex way to do something simple... > This is already a QDEV. Having to use QOM API seems going > backward, since we have the DEFINE_PROP_STRING() macros available > in "hw/qdev-properties.h". > > Maybe I'm not seeing the advantages clearly. I'll try later. The canonical path is not very helpful in trace log... The description I set matches the hardware definitions and is easier to follow, see patch #6 (where it is set) where the description comes from: https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html Description name taken from: https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml So in this particular case I don't find the canonical pathname practical (from an hardware debugging perspective). > > Thanks for your review, > > Phil. >
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote: >> On 6/25/20 8:37 AM, Markus Armbruster wrote: >>> Cédric Le Goater <clg@kaod.org> writes: >>> >>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote: >>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote: >>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >>>>>>> Add a description field to distinguish between multiple devices. >>> >>> Pardon my lack of imagination: how does this help you with debugging? >> >> Ah, the patch subject is indeed incorrect, this should be: >> "... for *tracing* purpose" (I use tracing when debugging). >> >> In the next patch, we use the 'description' property: >> >> +# pca9552.c >> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs >> 0-15 [%s]" >> >> So when the machine has multiple PCA9552 and guest accesses both, >> we can distinct which one is used. For me having "pca1" / "pca0" >> is easier to follow than the address of the QOM object. >> >>> >>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>>> >>>>>> Could it be a QOM attribute ? >>>>> >>>>> What do you call a 'QOM attribute'? >>>>> Is it what qdev properties implement? >>>>> (in this case via DEFINE_PROP_STRING). >>>> >>>> I meant a default Object property, which would apply to all Objects. >>> >>> Good point. Many devices have multiple component objects of the same >>> type. >>> >>>> What you did is fine, so : >>>> >>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>> >>>> but, may be, a well defined child name is enough for the purpose. >>> >>> object_get_canonical_path() returns a distinct path for each (component) >>> object. The path components are the child property names. >>> >>> Properties can have descriptions: object_property_set_description(). >> >> TIL object_property_set_description :> >> >> Ah, there is no equivalent object_property_get_description(), >> we have to use object_get_canonical_path(). Hmm, not obvious. >> >>> >>> Sufficient? >> >> I don't know... This seems a complex way to do something simple... >> This is already a QDEV. Having to use QOM API seems going >> backward, since we have the DEFINE_PROP_STRING() macros available >> in "hw/qdev-properties.h". >> >> Maybe I'm not seeing the advantages clearly. I'll try later. > > The canonical path is not very helpful in trace log... Why? Okay, I checked the code. Since the devices in question don't get a composition tree parent assigned, realize puts them in the /machine/unattached orphanage. The canonical path is something like "/machine/unattached/device[6]", which is less than clear. The components of the canonical path are the names of the QOM child properties. object_get_canonical_path_component() returns the last one, in this case "device[6]". If we made the devices QOM children of some other device, we could name the child properties "pca0" and "pca1". object_get_canonical_path_component() would then return the strings you want to see. We make a device a QOM child of some QOM parent device only if the child is a component device of the parent (hence the name "composition tree"). Are these devices integral components of something else, or are they separate chips? > The description I set matches the hardware definitions > and is easier to follow, see patch #6 (where it is set) > where the description comes from: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html > > Description name taken from: > https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml > > So in this particular case I don't find the canonical pathname > practical (from an hardware debugging perspective). Personally, I'd be content with i2c bus and address for debugging purposes. The i2c buses *are* components: canonical paths look like "/machine/soc/i2c/aspeed.i2c.3". The combination of object_get_canonical_path_component(dev) and object_property_get_uint(dev, "address", &error_abort) identifies any i2c device on this machine, not just the two you decorate with a description string.
On 6/26/20 7:49 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > >> On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote: >>> On 6/25/20 8:37 AM, Markus Armbruster wrote: >>>> Cédric Le Goater <clg@kaod.org> writes: >>>> >>>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote: >>>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote: >>>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >>>>>>>> Add a description field to distinguish between multiple devices. >>>> >>>> Pardon my lack of imagination: how does this help you with debugging? >>> >>> Ah, the patch subject is indeed incorrect, this should be: >>> "... for *tracing* purpose" (I use tracing when debugging). >>> >>> In the next patch, we use the 'description' property: >>> >>> +# pca9552.c >>> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs >>> 0-15 [%s]" >>> >>> So when the machine has multiple PCA9552 and guest accesses both, >>> we can distinct which one is used. For me having "pca1" / "pca0" >>> is easier to follow than the address of the QOM object. >>> >>>> >>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>>>> >>>>>>> Could it be a QOM attribute ? >>>>>> >>>>>> What do you call a 'QOM attribute'? >>>>>> Is it what qdev properties implement? >>>>>> (in this case via DEFINE_PROP_STRING). >>>>> >>>>> I meant a default Object property, which would apply to all Objects. >>>> >>>> Good point. Many devices have multiple component objects of the same >>>> type. >>>> >>>>> What you did is fine, so : >>>>> >>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>> >>>>> but, may be, a well defined child name is enough for the purpose. >>>> >>>> object_get_canonical_path() returns a distinct path for each (component) >>>> object. The path components are the child property names. >>>> >>>> Properties can have descriptions: object_property_set_description(). >>> >>> TIL object_property_set_description :> >>> >>> Ah, there is no equivalent object_property_get_description(), >>> we have to use object_get_canonical_path(). Hmm, not obvious. >>> >>>> >>>> Sufficient? >>> >>> I don't know... This seems a complex way to do something simple... >>> This is already a QDEV. Having to use QOM API seems going >>> backward, since we have the DEFINE_PROP_STRING() macros available >>> in "hw/qdev-properties.h". >>> >>> Maybe I'm not seeing the advantages clearly. I'll try later. >> >> The canonical path is not very helpful in trace log... > > Why? > > Okay, I checked the code. Since the devices in question don't get a > composition tree parent assigned, realize puts them in the > /machine/unattached orphanage. The canonical path is something like > "/machine/unattached/device[6]", which is less than clear. > > The components of the canonical path are the names of the QOM child > properties. object_get_canonical_path_component() returns the last one, > in this case "device[6]". > > If we made the devices QOM children of some other device, we could name > the child properties "pca0" and "pca1". > object_get_canonical_path_component() would then return the strings you > want to see. > > We make a device a QOM child of some QOM parent device only if the child > is a component device of the parent (hence the name "composition > tree"). > > Are these devices integral components of something else, or are they > separate chips? Separate chips in the machine (actually not even on the machine mother board where is the CPU, but on a daughter board card). So in the composition tree I expect to see them as /machine/pca0 /machine/pca1 >> The description I set matches the hardware definitions >> and is easier to follow, see patch #6 (where it is set) >> where the description comes from: >> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html >> >> Description name taken from: >> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml >> >> So in this particular case I don't find the canonical pathname >> practical (from an hardware debugging perspective). > > Personally, I'd be content with i2c bus and address for debugging > purposes. > > The i2c buses *are* components: canonical paths look like > "/machine/soc/i2c/aspeed.i2c.3". The combination of > object_get_canonical_path_component(dev) and > object_property_get_uint(dev, "address", &error_abort) identifies any > i2c device on this machine, not just the two you decorate with a > description string. The I2C busses is provided by Aspeed peripherals. I counted 19 different I2C busses on this machine. "pca0" is connected to i2c bus #11, "pca1" to bus #3. I still don't think this will be practical, but I'll try your suggestion. Regards, Phil.
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 6/26/20 7:49 AM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> >>> On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote: >>>> On 6/25/20 8:37 AM, Markus Armbruster wrote: >>>>> Cédric Le Goater <clg@kaod.org> writes: >>>>> >>>>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote: >>>>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote: >>>>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote: >>>>>>>>> Add a description field to distinguish between multiple devices. >>>>> >>>>> Pardon my lack of imagination: how does this help you with debugging? >>>> >>>> Ah, the patch subject is indeed incorrect, this should be: >>>> "... for *tracing* purpose" (I use tracing when debugging). >>>> >>>> In the next patch, we use the 'description' property: >>>> >>>> +# pca9552.c >>>> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs >>>> 0-15 [%s]" >>>> >>>> So when the machine has multiple PCA9552 and guest accesses both, >>>> we can distinct which one is used. For me having "pca1" / "pca0" >>>> is easier to follow than the address of the QOM object. >>>> >>>>> >>>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>>>>> >>>>>>>> Could it be a QOM attribute ? >>>>>>> >>>>>>> What do you call a 'QOM attribute'? >>>>>>> Is it what qdev properties implement? >>>>>>> (in this case via DEFINE_PROP_STRING). >>>>>> >>>>>> I meant a default Object property, which would apply to all Objects. >>>>> >>>>> Good point. Many devices have multiple component objects of the same >>>>> type. >>>>> >>>>>> What you did is fine, so : >>>>>> >>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>>> >>>>>> but, may be, a well defined child name is enough for the purpose. >>>>> >>>>> object_get_canonical_path() returns a distinct path for each (component) >>>>> object. The path components are the child property names. >>>>> >>>>> Properties can have descriptions: object_property_set_description(). >>>> >>>> TIL object_property_set_description :> >>>> >>>> Ah, there is no equivalent object_property_get_description(), >>>> we have to use object_get_canonical_path(). Hmm, not obvious. >>>> >>>>> >>>>> Sufficient? >>>> >>>> I don't know... This seems a complex way to do something simple... >>>> This is already a QDEV. Having to use QOM API seems going >>>> backward, since we have the DEFINE_PROP_STRING() macros available >>>> in "hw/qdev-properties.h". >>>> >>>> Maybe I'm not seeing the advantages clearly. I'll try later. >>> >>> The canonical path is not very helpful in trace log... >> >> Why? >> >> Okay, I checked the code. Since the devices in question don't get a >> composition tree parent assigned, realize puts them in the >> /machine/unattached orphanage. The canonical path is something like >> "/machine/unattached/device[6]", which is less than clear. This is common for onboard devices. I hate it. Some boards do better than others. For instance, ast2600-evb has 33 sensibly named entries under /machine/soc/, and only 9 entries in the /machine/unattached/ orphanage. i440fx has two sensible named ones under /machine/, and 26 in the orphanage. >> The components of the canonical path are the names of the QOM child >> properties. object_get_canonical_path_component() returns the last one, >> in this case "device[6]". >> >> If we made the devices QOM children of some other device, we could name >> the child properties "pca0" and "pca1". >> object_get_canonical_path_component() would then return the strings you >> want to see. >> >> We make a device a QOM child of some QOM parent device only if the child >> is a component device of the parent (hence the name "composition >> tree"). >> >> Are these devices integral components of something else, or are they >> separate chips? > > Separate chips in the machine (actually not even on the machine mother > board where is the CPU, but on a daughter board card). > > So in the composition tree I expect to see them as > > /machine/pca0 > /machine/pca1 As long as the final canonical path component is something readable rather than auto-generated crap like "device[6]", object_get_canonical_path_component() is usable for tracing. >>> The description I set matches the hardware definitions >>> and is easier to follow, see patch #6 (where it is set) >>> where the description comes from: >>> >>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html >>> >>> Description name taken from: >>> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml >>> >>> So in this particular case I don't find the canonical pathname >>> practical (from an hardware debugging perspective). >> >> Personally, I'd be content with i2c bus and address for debugging >> purposes. >> >> The i2c buses *are* components: canonical paths look like >> "/machine/soc/i2c/aspeed.i2c.3". The combination of >> object_get_canonical_path_component(dev) and >> object_property_get_uint(dev, "address", &error_abort) identifies any >> i2c device on this machine, not just the two you decorate with a >> description string. > > The I2C busses is provided by Aspeed peripherals. I counted 19 different > I2C busses on this machine. > > "pca0" is connected to i2c bus #11, "pca1" to bus #3. > > I still don't think this will be practical, but I'll try your > suggestion. "bus=11 addr=0x60" isn't exactly wonderful, but it seems practical enough for me. Even "device[6]" seems workable, just bothersome, because it's needlessly hard to remember, and prone to change. Mind, I'm not recommending any particular solution for "want a more useful device ID in traces", I'm just throwing out ideas on how to solve the problem in a more general way. Working towards a a neater QOM composition tree might help with more than tracing.
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h index ef6da4988f..c5be7f1c5e 100644 --- a/include/hw/misc/pca9552.h +++ b/include/hw/misc/pca9552.h @@ -27,6 +27,7 @@ typedef struct PCA9552State { uint8_t regs[PCA9552_NR_REGS]; uint8_t max_reg; + char *description; /* For debugging purpose only */ uint8_t nr_leds; } PCA9552State; diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c index b97fc2893c..54ccdcf6d4 100644 --- a/hw/misc/pca9552.c +++ b/hw/misc/pca9552.c @@ -12,6 +12,7 @@ #include "qemu/osdep.h" #include "qemu/log.h" #include "qemu/module.h" +#include "hw/qdev-properties.h" #include "hw/misc/pca9552.h" #include "hw/misc/pca9552_regs.h" #include "migration/vmstate.h" @@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp) __func__, s->nr_leds, PCA9552_PIN_COUNT); return; } + if (!s->description) { + s->description = g_strdup("pca-unspecified"); + } } +static Property pca9552_properties[] = { + DEFINE_PROP_STRING("description", PCA9552State, description), + DEFINE_PROP_END_OF_LIST(), +}; + static void pca9552_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data) dc->realize = pca9552_realize; dc->reset = pca9552_reset; dc->vmsd = &pca9552_vmstate; + device_class_set_props(dc, pca9552_properties); } static const TypeInfo pca9552_info = {
Add a description field to distinguish between multiple devices. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/misc/pca9552.h | 1 + hw/misc/pca9552.c | 10 ++++++++++ 2 files changed, 11 insertions(+)