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