Re: [PATCH v5 18/20] nubus: add support for slot IRQs
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
>> Reviewed-by: Laurent Vivier
>> ---
>> 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é
>
>> + 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
Re: [PATCH v5 18/20] nubus: add support for slot IRQs
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
Reviewed-by: Laurent Vivier
---
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é
+ 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.
Re: [PATCH v5 18/20] nubus: add support for slot IRQs
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
Reviewed-by: Laurent Vivier
---
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é
+ 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.
Re: [PATCH v5 18/20] nubus: add support for slot IRQs
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
Reviewed-by: Laurent Vivier
---
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é
+ 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.
Re: [PATCH v5 18/20] nubus: add support for slot IRQs
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
Reviewed-by: Laurent Vivier
---
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é
+ 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.
Re: [PATCH v5 18/20] nubus: add support for slot IRQs
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
Reviewed-by: Laurent Vivier
---
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é
+ 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.
Re: [PATCH v5 18/20] nubus: add support for slot IRQs
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
Reviewed-by: Laurent Vivier
---
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é
+qemu_set_irq(nubus->irqs[nd->slot], level);
+}
[PATCH v5 18/20] nubus: add support for slot IRQs
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
Reviewed-by: Laurent Vivier
---
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);
}
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_IRQS16
+
#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;
--
2.20.1
