[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for deb
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose |
Date: |
Fri, 26 Jun 2020 11:43:51 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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.
- Re: [PATCH v4 3/8] hw/misc/pca9552: Use the PCA9552_PIN_COUNT definition, (continued)
[PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Philippe Mathieu-Daudé, 2020/06/20
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Cédric Le Goater, 2020/06/22
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Philippe Mathieu-Daudé, 2020/06/22
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Cédric Le Goater, 2020/06/22
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Markus Armbruster, 2020/06/25
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Philippe Mathieu-Daudé, 2020/06/25
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Philippe Mathieu-Daudé, 2020/06/25
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Markus Armbruster, 2020/06/26
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose,
Philippe Mathieu-Daudé <=
- Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose, Markus Armbruster, 2020/06/27
[PATCH v4 5/8] hw/misc/pca9552: Trace GPIO High/Low events, Philippe Mathieu-Daudé, 2020/06/20
[PATCH v4 6/8] hw/arm/aspeed: Describe each PCA9552 device, Philippe Mathieu-Daudé, 2020/06/20
[PATCH v4 7/8] hw/misc/pca9552: Trace GPIO change events, Philippe Mathieu-Daudé, 2020/06/20