Re: [PATCH v5 18/20] nubus: add support for slot IRQs

[Mark Cave-Ayland]

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.