Am 12. Februar 2022 17:44:30 MEZ schrieb BALATON Zoltan <bala...@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 <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. >> >>> --- >>> 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); >>