Re: [PATCH 18/20] nubus: add support for slot IRQs - Mark Cave-Ayland

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	qemu-devel@nongnu.org, laurent@vivier.eu
Subject: Re: [PATCH 18/20] nubus: add support for slot IRQs
Date: Sun, 12 Sep 2021 18:05:13 +0100	[thread overview]
Message-ID: <3a21dab9-3e92-8093-7199-158ff906dc58@ilande.co.uk> (raw)
In-Reply-To: <1720db0e-5c49-09fa-b749-e7a7f69cbe6c@amsat.org>

On 12/09/2021 17:00, Philippe Mathieu-Daudé wrote:

> On 9/12/21 9:49 AM, 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>
>> ---
>>   hw/nubus/nubus-bridge.c  | 2 ++
>>   hw/nubus/nubus-device.c  | 8 ++++++++
>>   include/hw/nubus/nubus.h | 6 ++++++
>>   3 files changed, 16 insertions(+)
>>
>> 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);
>>   }
> 
> I'm confused, the IRQs belong to the bus, but you create them
> on the bridge device (I know, the bus is not a qdev)...

Following on the same logic from my previous mail: the IRQs already exist as physical 
lines within the bus, but they are published (i.e. connected) via the bridge to the 
CPU. This also allows the use of GPIOs to wire up the Nubus without having to devise 
a whole new set of Nubus infrastructure just for interrupts.

Certainly this feels different when you compare with PCI, but then I'd also argue 
that the existing PCI code predates QOM/qdev and if it were written today it would be 
done differently.

>>   static Property nubus_bridge_properties[] = {
>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
>> index f316eb7789..67ab281943 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 503ebf0c1c..2b9c4c77ac 100644
>> --- a/include/hw/nubus/nubus.h
>> +++ b/include/hw/nubus/nubus.h
>> @@ -24,6 +24,8 @@
>>   #define NUBUS_FIRST_SLOT      0x0
>>   #define NUBUS_LAST_SLOT       0xf
>>   
>> +#define NUBUS_IRQS            16
>> +
>>   #define TYPE_NUBUS_DEVICE "nubus-device"
>>   OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE)
>>   
>> @@ -43,6 +45,8 @@ struct NubusBus {
>>       MemoryRegion slot_io;
>>   
>>       uint32_t slot_available_mask;
>> +
>> +    qemu_irq irqs[NUBUS_IRQS];
>>   };
>>   
>>   #define NUBUS_DECL_ROM_MAX_SIZE    0xffff
>> @@ -58,6 +62,8 @@ struct NubusDevice {
>>       MemoryRegion decl_rom;
>>   };
>>   
>> +void nubus_set_irq(NubusDevice *nd, int level);
> 
> ... then the API only involves a device and a bus, the
> bridge is hidden.

That's correct. All a Nubus device cares about is being able to raise and lower its 
IRQ line: the routing between the Nubus and the CPU is delegated to the Nubus bridge.

ATB,

Mark.