On 06/15/15 16:33, Markus Armbruster wrote:
> Laszlo Ersek <ler...@redhat.com> writes:
>
>> The sysbus_get_fw_dev_path() function formats OpenFirmware device path
>> nodes ("driver-name@unit-address") for sysbus devices. The first choice
>> for "unit-address" is the base address of the device's first MMIO region.
>> The second choice is its first IO port.
>>
>> However, if two sysbus devices with the same "driver-name" lack both MMIO
>> and PIO resources, then there is no good way to distinguish them based on
>> their OFW nodes, because in this case unit-address is omitted completely
>> for both devices.
>
> Got an example for such a device? Mind adding it to the commit message?
That's the right next patch in the series (on which I didn't Cc you,
apologies). If you'd like I can hint at the next patch / the device in
question (PXB) in the commit message.
>
>> For the sake of such devices, introduce the explicit_ofw_unit_address()
>> "virtual member function". With this function, each sysbus device in the
>> same SysBusDeviceClass can state its own address.
>>
>> Cc: Markus Armbruster <arm...@redhat.com>
>> Cc: Marcel Apfelbaum <mar...@redhat.com>
>> Cc: Michael S. Tsirkin <m...@redhat.com>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> Notes:
>> v4:
>> - Yet another approach. Instead of allowing the creator of the device to
>> set a string property statically, introduce a class level callback.
>>
>> v3:
>> - new in v3
>> - new approach
>>
>> include/hw/sysbus.h | 9 +++++++++
>> hw/core/sysbus.c | 13 +++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index d1f3f00..63b036b 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -41,6 +41,15 @@ typedef struct SysBusDeviceClass {
>> /*< public >*/
>>
>> int (*init)(SysBusDevice *dev);
>> +
>> + /*
>> + * Sometimes a class of SysBusDevices has neither MMIO nor PIO
>> resources,
>> + * yet instances of it would like to distinguish themselves, in
>> + * OpenFirmware device paths, from other instances of the same class on
>> the
>> + * same sysbus. For that end we expose this callback. It returns a
>> + * dynamically allocated string.
>> + */
>> + char *(*explicit_ofw_unit_address)(SysBusDevice *dev);
>
> I prefer function comments to follow a strict pattern:
>
> /*
> * Headline explaining the function's purpose[*]
> * Zero or more paragraphs explaining preconditions, side effects,
> * return values, error conditions.
> */
I follow a very similar requirement in all my edk2 code closely -- but
in edk2 that's actually a *requirement*. :) I wasn't aware of any such
requirement in QEMU, and I thought "any function comment will be seen as
a bonus". :)
I'll rewrite the comment like this, thanks.
> [*] If you can't come up with a headline fitting into a single line,
> chances are the function does too many things.
"Delegate formatting of non-IO, non-MMIO address of sysbus device, due
to bus not knowing."
>
>> } SysBusDeviceClass;
>>
>> struct SysBusDevice {
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index 0ebb4e2..a0ec814 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -281,6 +281,7 @@ static void sysbus_dev_print(Monitor *mon, DeviceState
>> *dev, int indent)
>> static char *sysbus_get_fw_dev_path(DeviceState *dev)
>> {
>> SysBusDevice *s = SYS_BUS_DEVICE(dev);
>> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
>>
>> if (s->num_mmio) {
>> return g_strdup_printf("%s@"TARGET_FMT_plx, qdev_fw_name(dev),
>> @@ -289,6 +290,18 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
>> if (s->num_pio) {
>> return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
>> }
>> + if (sbc->explicit_ofw_unit_address) {
>> + char *addr;
>> +
>> + addr = sbc->explicit_ofw_unit_address(s);
>> + if (addr) {
>> + char *fw_dev_path;
>> +
>> + fw_dev_path = g_strdup_printf("%s@%s", qdev_fw_name(dev), addr);
>> + g_free(addr);
>> + return fw_dev_path;
>> + }
>> + }
>> return g_strdup(qdev_fw_name(dev));
>> }
>
> In short functions like this one, I prefer to have declarations out of
> the way in one place rather than cluttering inner blocks.
Will do.
> Matter of
> taste, so
>
> Reviewed-by: Markus Armbruster <arm...@redhat.com>
Awesome! :) Thank you!
Laszlo
