Re: [PATCH v2 1/5] malta: Move PCI interrupt handling from gt64xxx to piix4

[BALATON Zoltan]

On Sat, 12 Feb 2022, BALATON Zoltan wrote:
> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> > Handling PCI interrupts in piix4 increases cohesion and reduces differences
> > between piix4 and piix3.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Sorry for being late in commenting, I've missed the first round. Apologies if 
> this causes a delay or another version.
>
> > ---
> > hw/isa/piix4.c         | 58 +++++++++++++++++++++++++++++++++++++++
> > hw/mips/gt64xxx_pci.c  | 62 ++++--------------------------------------
> > hw/mips/malta.c        |  6 +---
> > include/hw/mips/mips.h |  2 +-
> > 4 files changed, 65 insertions(+), 63 deletions(-)
> >
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 0fe7b69bc4..5a86308689 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -45,6 +45,7 @@ struct PIIX4State {
> >     PCIDevice dev;
> >     qemu_irq cpu_intr;
> >     qemu_irq *isa;
> > +    qemu_irq i8259[ISA_NUM_IRQS];
> >
> >     RTCState rtc;
> >     /* Reset Control Register */
> > @@ -54,6 +55,30 @@ struct PIIX4State {
> >
> > OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
> >
> > +static int pci_irq_levels[4];
> > +
> > +static void piix4_set_irq(void *opaque, int irq_num, int level)
> > +{
> > +    int i, pic_irq, pic_level;
> > +    qemu_irq *pic = opaque;
> > +
> > +    pci_irq_levels[irq_num] = level;
> > +
> > +    /* now we change the pic irq level according to the piix irq mappings 
> > */
> > +    /* XXX: optimize */
> > +    pic_irq = piix4_dev->config[PIIX_PIRQCA + irq_num];
> > +    if (pic_irq < 16) {
> > +        /* The pic level is the logical OR of all the PCI irqs mapped to 
> > it. */
> > +        pic_level = 0;
> > +        for (i = 0; i < 4; i++) {
> > +            if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
> > +                pic_level |= pci_irq_levels[i];
> > +            }
> > +        }
> > +        qemu_set_irq(pic[pic_irq], pic_level);
> > +    }
> > +}
> > +
> > static void piix4_isa_reset(DeviceState *dev)
> > {
> >     PIIX4State *d = PIIX4_PCI_DEVICE(dev);
> > @@ -248,8 +273,34 @@ static void piix4_register_types(void)
> >
> > type_init(piix4_register_types)
> >
> > +static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> > +{
> > +    int slot;
> > +
> > +    slot = PCI_SLOT(pci_dev->devfn);
> > +
> > +    switch (slot) {
> > +    /* PIIX4 USB */
> > +    case 10:
> > +        return 3;
> > +    /* AMD 79C973 Ethernet */
> > +    case 11:
> > +        return 1;
> > +    /* Crystal 4281 Sound */
> > +    case 12:
> > +        return 2;
> > +    /* PCI slot 1 to 4 */
> > +    case 18 ... 21:
> > +        return ((slot - 18) + irq_num) & 0x03;
> > +    /* Unknown device, don't do any translation */
> > +    default:
> > +        return irq_num;
> > +    }
> > +}
> > +
> > DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus 
> > **smbus)
> > {
> > +    PIIX4State *s;
> >     PCIDevice *pci;
> >     DeviceState *dev;
> >     int devfn = PCI_DEVFN(10, 0);
> > @@ -257,6 +308,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
> > **isa_bus, I2CBus **smbus)
> >     pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
> >                                           TYPE_PIIX4_PCI_DEVICE);
> >     dev = DEVICE(pci);
> > +    s = PIIX4_PCI_DEVICE(pci);
> >     if (isa_bus) {
> >         *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> >     }
> > @@ -271,5 +323,11 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
> > **isa_bus, I2CBus **smbus)
> >                                NULL, 0, NULL);
> >     }
> >
> > +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->i8259, 4);
> > +
> > +    for (int i = 0; i < ISA_NUM_IRQS; i++) {

Sorry one more nit. This is code movement but are we OK with declaring 
local variable within for? I thinks coding style advises against this, not 
sure if checkpatch accepts it or not. This could be cleaned up the next 
patch together with removing the i8259 field which are simple enough to do 
in one patch then this one stays as code movement and changes in next 
patch.

> > +        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
> > +    }
> > +
> >     return dev;
> > }
> > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
[...]
> > diff --git a/include/hw/mips/mips.h b/include/hw/mips/mips.h
> > index 6c9c8805f3..ff88942e63 100644
> > --- a/include/hw/mips/mips.h
> > +++ b/include/hw/mips/mips.h
> > @@ -10,7 +10,7 @@
> > #include "exec/memory.h"
> >
> > /* gt64xxx.c */
> > -PCIBus *gt64120_register(qemu_irq *pic);
> > +PCIBus *gt64120_register(void);
>
> Now that you don't need to pass anything to it, do you still need this 
> function? Maybe what it does now could be done in the gt64120 device's 
> realize functions (there seems to be at least two: gt64120_realize and 
> gt64120_pci_realize but haven't checked which is more appropriate to put this 
> init in) or in an init function then you can just create the gt64120 device 
> in malta.c with qdev_new as is more usual to do in other boards. This 
> register function looks like the legacy init functions we're trying to get 
> rid of so this seems to be an opportunity to clean this up. This could be 
> done in a separate follow up though so may not need to be part of this series 
> but may be nice to have.

I meant this comment at the end of patch 1. But maybe this is too much to 
do in this series as it needs more understanding of the gt64120 
implementation so unless you already understand it enough to clean this up 
easily now and it would be too much effort for you, then this is just for 
the record for possible future clean up. The series is good enough without 
putting in more stuff but if there's a way to go further then that would 
be nice.

Regards,.
BALATON Zoltan

> > /* bonito.c */
> > PCIBus *bonito_init(qemu_irq *pic);