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

[Mark Cave-Ayland]

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.