Message ID | 1434203538-8075-4-git-send-email-lersek@redhat.com |
---|---|
State | New |
Headers | show |
Laszlo Ersek <lersek@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? > 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 <armbru@redhat.com> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@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. */ [*] If you can't come up with a headline fitting into a single line, chances are the function does too many things. > } 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. Matter of taste, so Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 06/15/15 16:33, Markus Armbruster wrote: > Laszlo Ersek <lersek@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 <armbru@redhat.com> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@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 <armbru@redhat.com> Awesome! :) Thank you! Laszlo
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); } 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)); }
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. 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 <armbru@redhat.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Laszlo Ersek <lersek@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(+)