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

[Bernhard Beschow <shentey@gmail.com>]

Am 12. Februar 2022 14:18:54 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>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.

Don't worry. Your comments are appreciated!

>> ---
>> 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++) {
>> +        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
>> index c7480bd019..9e23e32eff 100644
>> --- a/hw/mips/gt64xxx_pci.c
>> +++ b/hw/mips/gt64xxx_pci.c
>> @@ -981,56 +981,6 @@ static const MemoryRegionOps isd_mem_ops = {
>>     },
>> };
>>
>> -static int gt64120_pci_map_irq(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;
>> -    }
>> -}
>> -
>> -static int pci_irq_levels[4];
>> -
>> -static void gt64120_pci_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 gt64120_reset(DeviceState *dev)
>> {
>>     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
>> @@ -1207,7 +1157,7 @@ static void gt64120_realize(DeviceState *dev, Error **errp)
>>                           "gt64120-isd", 0x1000);
>> }
>>
>> -PCIBus *gt64120_register(qemu_irq *pic)
>> +PCIBus *gt64120_register(void)
>> {
>>     GT64120State *d;
>>     PCIHostState *phb;
>> @@ -1218,12 +1168,10 @@ PCIBus *gt64120_register(qemu_irq *pic)
>>     phb = PCI_HOST_BRIDGE(dev);
>>     memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB);
>>     address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem");
>> -    phb->bus = pci_register_root_bus(dev, "pci",
>> -                                     gt64120_pci_set_irq, gt64120_pci_map_irq,
>> -                                     pic,
>> -                                     &d->pci0_mem,
>> -                                     get_system_io(),
>> -                                     PCI_DEVFN(18, 0), 4, TYPE_PCI_BUS);
>> +    phb->bus = pci_root_bus_new(dev, "pci",
>> +                                &d->pci0_mem,
>> +                                get_system_io(),
>> +                                PCI_DEVFN(18, 0), TYPE_PCI_BUS);
>>     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>
>>     pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>> index b770b8d367..13254dbc89 100644
>> --- a/hw/mips/malta.c
>> +++ b/hw/mips/malta.c
>> @@ -97,7 +97,6 @@ struct MaltaState {
>>
>>     Clock *cpuclk;
>>     MIPSCPSState cps;
>> -    qemu_irq i8259[ISA_NUM_IRQS];
>> };
>>
>> static struct _loaderparams {
>> @@ -1391,7 +1390,7 @@ void mips_malta_init(MachineState *machine)
>>     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
>>
>>     /* Northbridge */
>> -    pci_bus = gt64120_register(s->i8259);
>> +    pci_bus = gt64120_register();
>>     /*
>>      * The whole address space decoded by the GT-64120A doesn't generate
>>      * exception when accessing invalid memory. Create an empty slot to
>> @@ -1404,9 +1403,6 @@ void mips_malta_init(MachineState *machine)
>>
>>     /* Interrupt controller */
>>     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
>> -    for (int i = 0; i < ISA_NUM_IRQS; i++) {
>> -        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
>> -    }
>>
>>     /* generate SPD EEPROM data */
>>     generate_eeprom_spd(&smbus_eeprom_buf[0 * 256], ram_size);
>> 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'll give it a shot (see my other mail).

Regards,
Bernhard

>Regards,.
>BALATON Zoltan
>
>>
>> /* bonito.c */
>> PCIBus *bonito_init(qemu_irq *pic);
>>