| Message ID | 20210923091308.13832-19-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | nubus: bus, device, bridge, IRQ and address space improvements | expand |
On 9/23/21 11:13, Mark Cave-Ayland wrote: > Each Nubus slot has an IRQ line that can be used to request service from the > CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using qdev > gpios accordingly, and introduce a new nubus_set_irq() function that can be used > by Nubus devices to control the slot IRQ. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > --- > hw/nubus/nubus-bridge.c | 2 ++ > hw/nubus/nubus-device.c | 8 ++++++++ > include/hw/nubus/nubus.h | 6 ++++++ > 3 files changed, 16 insertions(+) > static Property nubus_bridge_properties[] = { > diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c > index 280f40e88a..0f1852f671 100644 > --- a/hw/nubus/nubus-device.c > +++ b/hw/nubus/nubus-device.c > @@ -10,12 +10,20 @@ > > #include "qemu/osdep.h" > #include "qemu/datadir.h" > +#include "hw/irq.h" > #include "hw/loader.h" > #include "hw/nubus/nubus.h" > #include "qapi/error.h" > #include "qemu/error-report.h" > > > +void nubus_set_irq(NubusDevice *nd, int level) > +{ > + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); > + A trace-event could be helpful here, otherwise: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > + qemu_set_irq(nubus->irqs[nd->slot], level); > +}
On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote: > On 9/23/21 11:13, Mark Cave-Ayland wrote: >> Each Nubus slot has an IRQ line that can be used to request service from the >> CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using qdev >> gpios accordingly, and introduce a new nubus_set_irq() function that can be used >> by Nubus devices to control the slot IRQ. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> Reviewed-by: Laurent Vivier <laurent@vivier.eu> >> --- >> hw/nubus/nubus-bridge.c | 2 ++ >> hw/nubus/nubus-device.c | 8 ++++++++ >> include/hw/nubus/nubus.h | 6 ++++++ >> 3 files changed, 16 insertions(+) > >> static Property nubus_bridge_properties[] = { >> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >> index 280f40e88a..0f1852f671 100644 >> --- a/hw/nubus/nubus-device.c >> +++ b/hw/nubus/nubus-device.c >> @@ -10,12 +10,20 @@ >> #include "qemu/osdep.h" >> #include "qemu/datadir.h" >> +#include "hw/irq.h" >> #include "hw/loader.h" >> #include "hw/nubus/nubus.h" >> #include "qapi/error.h" >> #include "qemu/error-report.h" >> +void nubus_set_irq(NubusDevice *nd, int level) >> +{ >> + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); >> + > > A trace-event could be helpful here, otherwise: > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> + qemu_set_irq(nubus->irqs[nd->slot], level); >> +} I think adding a trace event here would just be too verbose (particularly if you have more than one nubus device) and not particularly helpful: normally the part you want to debug is the in the device itself looking at which constituent flags combine to raise/lower the interrupt line. And once you've done that then you've already got a suitable trace-event in place... ATB, Mark.
On 9/24/21 09:06, Mark Cave-Ayland wrote: > On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote: > >> On 9/23/21 11:13, Mark Cave-Ayland wrote: >>> Each Nubus slot has an IRQ line that can be used to request service >>> from the >>> CPU. Connect the IRQs to the Nubus bridge so that they can be wired >>> up using qdev >>> gpios accordingly, and introduce a new nubus_set_irq() function that >>> can be used >>> by Nubus devices to control the slot IRQ. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> Reviewed-by: Laurent Vivier <laurent@vivier.eu> >>> --- >>> hw/nubus/nubus-bridge.c | 2 ++ >>> hw/nubus/nubus-device.c | 8 ++++++++ >>> include/hw/nubus/nubus.h | 6 ++++++ >>> 3 files changed, 16 insertions(+) >> >>> static Property nubus_bridge_properties[] = { >>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >>> index 280f40e88a..0f1852f671 100644 >>> --- a/hw/nubus/nubus-device.c >>> +++ b/hw/nubus/nubus-device.c >>> @@ -10,12 +10,20 @@ >>> #include "qemu/osdep.h" >>> #include "qemu/datadir.h" >>> +#include "hw/irq.h" >>> #include "hw/loader.h" >>> #include "hw/nubus/nubus.h" >>> #include "qapi/error.h" >>> #include "qemu/error-report.h" >>> +void nubus_set_irq(NubusDevice *nd, int level) >>> +{ >>> + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); >>> + >> >> A trace-event could be helpful here, otherwise: >> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >>> + qemu_set_irq(nubus->irqs[nd->slot], level); >>> +} > > I think adding a trace event here would just be too verbose > (particularly if you have more than one nubus device) and not > particularly helpful: normally the part you want to debug is the in the > device itself looking at which constituent flags combine to raise/lower > the interrupt line. And once you've done that then you've already got a > suitable trace-event in place... But devices accessing the bus are not aware of which devices are plugged onto it. Wait, what is suppose to call nubus_set_irq()? Devices on the bus, to propagate the interrupt up to the CPU? OK so then the trace event is irrelevant indeed. I understood this API as any external device could trigger an IRQ to device on the bus. I wonder if renaming as nubus_device_set_irq() could make it clearer. Or even simpler, add a comment in "hw/nubus/nubus.h" explaining what nubus_set_irq() is for to avoid any confusion, and we are good.
On 9/24/21 11:01, Philippe Mathieu-Daudé wrote: > On 9/24/21 09:06, Mark Cave-Ayland wrote: >> On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote: >> >>> On 9/23/21 11:13, Mark Cave-Ayland wrote: >>>> Each Nubus slot has an IRQ line that can be used to request service >>>> from the >>>> CPU. Connect the IRQs to the Nubus bridge so that they can be wired >>>> up using qdev >>>> gpios accordingly, and introduce a new nubus_set_irq() function that >>>> can be used >>>> by Nubus devices to control the slot IRQ. >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu> >>>> --- >>>> hw/nubus/nubus-bridge.c | 2 ++ >>>> hw/nubus/nubus-device.c | 8 ++++++++ >>>> include/hw/nubus/nubus.h | 6 ++++++ >>>> 3 files changed, 16 insertions(+) >>> >>>> static Property nubus_bridge_properties[] = { >>>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >>>> index 280f40e88a..0f1852f671 100644 >>>> --- a/hw/nubus/nubus-device.c >>>> +++ b/hw/nubus/nubus-device.c >>>> @@ -10,12 +10,20 @@ >>>> #include "qemu/osdep.h" >>>> #include "qemu/datadir.h" >>>> +#include "hw/irq.h" >>>> #include "hw/loader.h" >>>> #include "hw/nubus/nubus.h" >>>> #include "qapi/error.h" >>>> #include "qemu/error-report.h" >>>> +void nubus_set_irq(NubusDevice *nd, int level) >>>> +{ >>>> + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); >>>> + >>> >>> A trace-event could be helpful here, otherwise: >>> >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> >>>> + qemu_set_irq(nubus->irqs[nd->slot], level); >>>> +} >> >> I think adding a trace event here would just be too verbose >> (particularly if you have more than one nubus device) and not >> particularly helpful: normally the part you want to debug is the in >> the device itself looking at which constituent flags combine to >> raise/lower the interrupt line. And once you've done that then you've >> already got a suitable trace-event in place... > > But devices accessing the bus are not aware of which devices are plugged > onto it. Wait, what is suppose to call nubus_set_irq()? Devices on the > bus, to propagate the interrupt up to the CPU? OK so then the trace > event is irrelevant indeed. I understood this API as any external device > could trigger an IRQ to device on the bus. I wonder if renaming as > nubus_device_set_irq() could make it clearer. Or even simpler, add > a comment in "hw/nubus/nubus.h" explaining what nubus_set_irq() is for > to avoid any confusion, and we are good. Just noticed v6 was sent, so the function description could either - sent as reply to v6 patch and squashed by Laurent when applying - sent later as a new cleanup patch on top - never added Up to you, I don't mind mind much the outcome.
On 24/09/2021 10:01, Philippe Mathieu-Daudé wrote: > On 9/24/21 09:06, Mark Cave-Ayland wrote: >> On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote: >> >>> On 9/23/21 11:13, Mark Cave-Ayland wrote: >>>> Each Nubus slot has an IRQ line that can be used to request service from the >>>> CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using qdev >>>> gpios accordingly, and introduce a new nubus_set_irq() function that can be used >>>> by Nubus devices to control the slot IRQ. >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu> >>>> --- >>>> hw/nubus/nubus-bridge.c | 2 ++ >>>> hw/nubus/nubus-device.c | 8 ++++++++ >>>> include/hw/nubus/nubus.h | 6 ++++++ >>>> 3 files changed, 16 insertions(+) >>> >>>> static Property nubus_bridge_properties[] = { >>>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >>>> index 280f40e88a..0f1852f671 100644 >>>> --- a/hw/nubus/nubus-device.c >>>> +++ b/hw/nubus/nubus-device.c >>>> @@ -10,12 +10,20 @@ >>>> #include "qemu/osdep.h" >>>> #include "qemu/datadir.h" >>>> +#include "hw/irq.h" >>>> #include "hw/loader.h" >>>> #include "hw/nubus/nubus.h" >>>> #include "qapi/error.h" >>>> #include "qemu/error-report.h" >>>> +void nubus_set_irq(NubusDevice *nd, int level) >>>> +{ >>>> + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); >>>> + >>> >>> A trace-event could be helpful here, otherwise: >>> >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> >>>> + qemu_set_irq(nubus->irqs[nd->slot], level); >>>> +} >> >> I think adding a trace event here would just be too verbose (particularly if you >> have more than one nubus device) and not particularly helpful: normally the part >> you want to debug is the in the device itself looking at which constituent flags >> combine to raise/lower the interrupt line. And once you've done that then you've >> already got a suitable trace-event in place... > > But devices accessing the bus are not aware of which devices are plugged > onto it. Wait, what is suppose to call nubus_set_irq()? Devices on the > bus, to propagate the interrupt up to the CPU? Yes indeed, that is correct. > OK so then the trace > event is irrelevant indeed. I understood this API as any external device > could trigger an IRQ to device on the bus. I wonder if renaming as > nubus_device_set_irq() could make it clearer. Or even simpler, add > a comment in "hw/nubus/nubus.h" explaining what nubus_set_irq() is for > to avoid any confusion, and we are good. The function name and signature nubus_set_irq(NubusDevice *nd, int level) was chosen because it was intended to be the Nubus equivalent of PCI's pci_set_irq(PCIDevice *d, int level) where the first parameter of both functions nicely indicates that this is to be called by the device. I don't think we've had any reports of confusion with the existing pci_set_irq() function usage? Another thing that makes me less worried is that the next series after this contains a number of changes for the macfb device, including the addition of VBL interrupts implemented using nubus_set_irq() so at that point there will be a grep-able usage example in the codebase. ATB, Mark.
On 24/09/2021 10:05, Philippe Mathieu-Daudé wrote: > On 9/24/21 11:01, Philippe Mathieu-Daudé wrote: >> On 9/24/21 09:06, Mark Cave-Ayland wrote: >>> On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote: >>> >>>> On 9/23/21 11:13, Mark Cave-Ayland wrote: >>>>> Each Nubus slot has an IRQ line that can be used to request service from the >>>>> CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using qdev >>>>> gpios accordingly, and introduce a new nubus_set_irq() function that can be used >>>>> by Nubus devices to control the slot IRQ. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu> >>>>> --- >>>>> hw/nubus/nubus-bridge.c | 2 ++ >>>>> hw/nubus/nubus-device.c | 8 ++++++++ >>>>> include/hw/nubus/nubus.h | 6 ++++++ >>>>> 3 files changed, 16 insertions(+) >>>> >>>>> static Property nubus_bridge_properties[] = { >>>>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >>>>> index 280f40e88a..0f1852f671 100644 >>>>> --- a/hw/nubus/nubus-device.c >>>>> +++ b/hw/nubus/nubus-device.c >>>>> @@ -10,12 +10,20 @@ >>>>> #include "qemu/osdep.h" >>>>> #include "qemu/datadir.h" >>>>> +#include "hw/irq.h" >>>>> #include "hw/loader.h" >>>>> #include "hw/nubus/nubus.h" >>>>> #include "qapi/error.h" >>>>> #include "qemu/error-report.h" >>>>> +void nubus_set_irq(NubusDevice *nd, int level) >>>>> +{ >>>>> + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); >>>>> + >>>> >>>> A trace-event could be helpful here, otherwise: >>>> >>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>> >>>>> + qemu_set_irq(nubus->irqs[nd->slot], level); >>>>> +} >>> >>> I think adding a trace event here would just be too verbose (particularly if you >>> have more than one nubus device) and not particularly helpful: normally the part >>> you want to debug is the in the device itself looking at which constituent flags >>> combine to raise/lower the interrupt line. And once you've done that then you've >>> already got a suitable trace-event in place... >> >> But devices accessing the bus are not aware of which devices are plugged >> onto it. Wait, what is suppose to call nubus_set_irq()? Devices on the >> bus, to propagate the interrupt up to the CPU? OK so then the trace >> event is irrelevant indeed. I understood this API as any external device >> could trigger an IRQ to device on the bus. I wonder if renaming as >> nubus_device_set_irq() could make it clearer. Or even simpler, add >> a comment in "hw/nubus/nubus.h" explaining what nubus_set_irq() is for >> to avoid any confusion, and we are good. > > Just noticed v6 was sent, so the function description could either > - sent as reply to v6 patch and squashed by Laurent when applying > - sent later as a new cleanup patch on top > - never added > > Up to you, I don't mind mind much the outcome. I'm happy enough with the current version given the existing precedent of pci_set_irq() and that the next set of q800 patches will make use of nubus_set_irq() to provide a working reference in-tree. Laurent, do you have any further comments on the series? ATB, Mark.
Le 29/09/2021 à 08:42, Mark Cave-Ayland a écrit : > On 24/09/2021 10:05, Philippe Mathieu-Daudé wrote: > >> On 9/24/21 11:01, Philippe Mathieu-Daudé wrote: >>> On 9/24/21 09:06, Mark Cave-Ayland wrote: >>>> On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote: >>>> >>>>> On 9/23/21 11:13, Mark Cave-Ayland wrote: >>>>>> Each Nubus slot has an IRQ line that can be used to request service from the >>>>>> CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using qdev >>>>>> gpios accordingly, and introduce a new nubus_set_irq() function that can be used >>>>>> by Nubus devices to control the slot IRQ. >>>>>> >>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu> >>>>>> --- >>>>>> hw/nubus/nubus-bridge.c | 2 ++ >>>>>> hw/nubus/nubus-device.c | 8 ++++++++ >>>>>> include/hw/nubus/nubus.h | 6 ++++++ >>>>>> 3 files changed, 16 insertions(+) >>>>> >>>>>> static Property nubus_bridge_properties[] = { >>>>>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >>>>>> index 280f40e88a..0f1852f671 100644 >>>>>> --- a/hw/nubus/nubus-device.c >>>>>> +++ b/hw/nubus/nubus-device.c >>>>>> @@ -10,12 +10,20 @@ >>>>>> #include "qemu/osdep.h" >>>>>> #include "qemu/datadir.h" >>>>>> +#include "hw/irq.h" >>>>>> #include "hw/loader.h" >>>>>> #include "hw/nubus/nubus.h" >>>>>> #include "qapi/error.h" >>>>>> #include "qemu/error-report.h" >>>>>> +void nubus_set_irq(NubusDevice *nd, int level) >>>>>> +{ >>>>>> + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); >>>>>> + >>>>> >>>>> A trace-event could be helpful here, otherwise: >>>>> >>>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> >>>>>> + qemu_set_irq(nubus->irqs[nd->slot], level); >>>>>> +} >>>> >>>> I think adding a trace event here would just be too verbose (particularly if you have more than >>>> one nubus device) and not particularly helpful: normally the part you want to debug is the in >>>> the device itself looking at which constituent flags combine to raise/lower the interrupt line. >>>> And once you've done that then you've already got a suitable trace-event in place... >>> >>> But devices accessing the bus are not aware of which devices are plugged >>> onto it. Wait, what is suppose to call nubus_set_irq()? Devices on the >>> bus, to propagate the interrupt up to the CPU? OK so then the trace >>> event is irrelevant indeed. I understood this API as any external device >>> could trigger an IRQ to device on the bus. I wonder if renaming as >>> nubus_device_set_irq() could make it clearer. Or even simpler, add >>> a comment in "hw/nubus/nubus.h" explaining what nubus_set_irq() is for >>> to avoid any confusion, and we are good. >> >> Just noticed v6 was sent, so the function description could either >> - sent as reply to v6 patch and squashed by Laurent when applying >> - sent later as a new cleanup patch on top >> - never added >> >> Up to you, I don't mind mind much the outcome. > > I'm happy enough with the current version given the existing precedent of pci_set_irq() and that the > next set of q800 patches will make use of nubus_set_irq() to provide a working reference in-tree. > > Laurent, do you have any further comments on the series? No, I'm going to queue the v6 to my branch and send the PR. Thanks, Laurent
diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c index 2c7c4ee121..0366d925a9 100644 --- a/hw/nubus/nubus-bridge.c +++ b/hw/nubus/nubus-bridge.c @@ -19,6 +19,8 @@ static void nubus_bridge_init(Object *obj) NubusBus *bus = &s->bus; qbus_create_inplace(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL); + + qdev_init_gpio_out(DEVICE(s), bus->irqs, NUBUS_IRQS); } static Property nubus_bridge_properties[] = { diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c index 280f40e88a..0f1852f671 100644 --- a/hw/nubus/nubus-device.c +++ b/hw/nubus/nubus-device.c @@ -10,12 +10,20 @@ #include "qemu/osdep.h" #include "qemu/datadir.h" +#include "hw/irq.h" #include "hw/loader.h" #include "hw/nubus/nubus.h" #include "qapi/error.h" #include "qemu/error-report.h" +void nubus_set_irq(NubusDevice *nd, int level) +{ + NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd))); + + qemu_set_irq(nubus->irqs[nd->slot], level); +} + static void nubus_device_realize(DeviceState *dev, Error **errp) { NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev)); diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h index 3620247be2..65d7b078b8 100644 --- a/include/hw/nubus/nubus.h +++ b/include/hw/nubus/nubus.h @@ -26,6 +26,8 @@ #define NUBUS_LAST_SLOT 0xf #define NUBUS_SLOT_NB (NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1) +#define NUBUS_IRQS 16 + #define TYPE_NUBUS_DEVICE "nubus-device" OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE) @@ -45,6 +47,8 @@ struct NubusBus { MemoryRegion slot_io; uint32_t slot_available_mask; + + qemu_irq irqs[NUBUS_IRQS]; }; #define NUBUS_DECL_ROM_MAX_SIZE (128 * KiB) @@ -60,6 +64,8 @@ struct NubusDevice { MemoryRegion decl_rom; }; +void nubus_set_irq(NubusDevice *nd, int level); + struct NubusBridge { SysBusDevice parent_obj;