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

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>
---
 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(-)

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++) {
> +        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.

Regards,.
BALATON Zoltan

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

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);
>

Am 12. Februar 2022 17:44:30 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>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.

I'll move the i8259-removing patch right after the code movement patch where this loop is removed entirely.

>>> +        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.

I'll give it a shot. Merging gt64120_register() into gt64120_realize() seems to preserve relevant control flow.

Regards,
Bernhard

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

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);
>>