Re: [PATCH v5 18/20] nubus: add support for slot IRQs - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	qemu-devel@nongnu.org, laurent@vivier.eu
Subject: Re: [PATCH v5 18/20] nubus: add support for slot IRQs
Date: Fri, 24 Sep 2021 11:01:27 +0200	[thread overview]
Message-ID: <8e879ded-3a4e-b12f-1479-62401af06927@amsat.org> (raw)
In-Reply-To: <43591fdf-045b-6c33-d54b-a3f18f0c5d58@ilande.co.uk>

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.