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

[Laurent Vivier]

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 <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?

No, I'm going to queue the v6 to my branch and send the PR.

Thanks,
Laurent