Am 12. Februar 2022 14:18:54 MEZ schrieb BALATON Zoltan <bala...@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 <shen...@gmail.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4...@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);
>>
