| Message ID | 20230307234711.55375-15-philmd@linaro.org |
|---|---|
| State | Accepted |
| Commit | fb27a3e9e7c348eb77a3b8d967e3d432d2ef8070 |
| Headers | show |
| Series | [PULL,01/20] docs/system: Remove "mips" board from target-mips.rst | expand |
Hi Zoltan, On 8/3/23 00:47, Philippe Mathieu-Daudé wrote: > From: BALATON Zoltan <balaton@eik.bme.hu> > > According to the PegasosII schematics the PCI interrupt lines are > connected to both the gpp pins of the Mv64361 north bridge and the > PINT pins of the VT8231 south bridge so guests can get interrupts from > either of these. So far we only had the MV64361 connections which > worked for on board devices but for additional PCI devices (such as > network or sound card added with -device) guest OSes expect interrupt > from the ISA IRQ 9 where the firmware routes these PCI interrupts in > VT8231 ISA bridge. After the previous patches we can now model this > and also remove the board specific connection from mv64361. Also > configure routing of these lines when using Virtual Open Firmware to > match board firmware for guests that expect this. IIUC the schematic, only tje INTA and INTB lines (AGP IRQs) are bidirectional and shared between NB/SB. INTC and INTC are SB output to NB input. > This fixes PCI interrupts on pegasos2 under Linux, MorphOS and AmigaOS. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > Tested-by: Rene Engel <ReneEngel80@emailn.de> > Message-Id: <520ff9e6eeef600ee14a4116c0c7b11940cc499c.1678188711.git.balaton@eik.bme.hu> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/pci-host/mv64361.c | 4 ---- > hw/ppc/pegasos2.c | 26 +++++++++++++++++++++++++- > 2 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/hw/pci-host/mv64361.c b/hw/pci-host/mv64361.c > index 298564f1f5..19e8031a3f 100644 > --- a/hw/pci-host/mv64361.c > +++ b/hw/pci-host/mv64361.c > @@ -873,10 +873,6 @@ static void mv64361_realize(DeviceState *dev, Error **errp) > } > sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq); > qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32); > - /* FIXME: PCI IRQ connections may be board specific */ > - for (i = 0; i < PCI_NUM_PINS; i++) { > - s->pci[1].irq[i] = qdev_get_gpio_in_named(dev, "gpp", 12 + i); > - } > } > > static void mv64361_reset(DeviceState *dev) > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c > index 7cc375df05..f1650be5ee 100644 > --- a/hw/ppc/pegasos2.c > +++ b/hw/ppc/pegasos2.c > @@ -73,6 +73,8 @@ struct Pegasos2MachineState { > MachineState parent_obj; > PowerPCCPU *cpu; > DeviceState *mv; > + qemu_irq mv_pirq[PCI_NUM_PINS]; > + qemu_irq via_pirq[PCI_NUM_PINS]; > Vof *vof; > void *fdt_blob; > uint64_t kernel_addr; > @@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque) > } > } > > +static void pegasos2_pci_irq(void *opaque, int n, int level) So this handler is only for A/B. We could rename it pegasos2_agp_irq() and only wire it to the first 2 pins, but I since we can only register one handler per bus, simpler to ... > +{ > + Pegasos2MachineState *pm = opaque; > + > + /* PCI interrupt lines are connected to both MV64361 and VT8231 */ > + qemu_set_irq(pm->mv_pirq[n], level); ... add a 'if (n < 2)' check here. > + qemu_set_irq(pm->via_pirq[n], level); > +} > @@ -156,11 +167,18 @@ static void pegasos2_init(MachineState *machine) > /* Marvell Discovery II system controller */ > pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1, > qdev_get_gpio_in(DEVICE(pm->cpu), PPC6xx_INPUT_INT))); > + for (i = 0; i < PCI_NUM_PINS; i++) { > + pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i); > + } > pci_bus = mv64361_get_pci_bus(pm->mv, 1); > + pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); > > /* VIA VT8231 South Bridge (multifunction PCI device) */ > via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), > true, TYPE_VT8231_ISA)); > + for (i = 0; i < PCI_NUM_PINS; i++) { I'd rather declare as via_pirq[2] and iterate over ARRAY_SIZE() here (and also use ARRAY_SIZE() in the new check in pegasos2_pci_irq). > + pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); > + }
On Wed, 8 Mar 2023, Philippe Mathieu-Daudé wrote: > Hi Zoltan, > > On 8/3/23 00:47, Philippe Mathieu-Daudé wrote: >> From: BALATON Zoltan <balaton@eik.bme.hu> >> >> According to the PegasosII schematics the PCI interrupt lines are >> connected to both the gpp pins of the Mv64361 north bridge and the >> PINT pins of the VT8231 south bridge so guests can get interrupts from >> either of these. So far we only had the MV64361 connections which >> worked for on board devices but for additional PCI devices (such as >> network or sound card added with -device) guest OSes expect interrupt >> from the ISA IRQ 9 where the firmware routes these PCI interrupts in >> VT8231 ISA bridge. After the previous patches we can now model this >> and also remove the board specific connection from mv64361. Also >> configure routing of these lines when using Virtual Open Firmware to >> match board firmware for guests that expect this. > > IIUC the schematic, only tje INTA and INTB lines (AGP IRQs) are > bidirectional and shared between NB/SB. > > INTC and INTC are SB output to NB input. I'll check the schematics again when I have time later but what we know for sure is that guests expect PCI interrupts to raise ISA IRQ9 which is mapped by the VT8231 ISA function 0c55-0x57 registers. The PCI buses are otherwise handled by the north bridge. So how can the VIA PINT pins be outputs? Where do the signals from the PCI cards go into VT8231 otherwise? Also the VT8231 datasheet on page 10 says PINTA-D pins are inputs so I can't understand your reasoning above. Regards, BALATON Zoltan >> This fixes PCI interrupts on pegasos2 under Linux, MorphOS and AmigaOS. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> Tested-by: Rene Engel <ReneEngel80@emailn.de> >> Message-Id: >> <520ff9e6eeef600ee14a4116c0c7b11940cc499c.1678188711.git.balaton@eik.bme.hu> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/pci-host/mv64361.c | 4 ---- >> hw/ppc/pegasos2.c | 26 +++++++++++++++++++++++++- >> 2 files changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/hw/pci-host/mv64361.c b/hw/pci-host/mv64361.c >> index 298564f1f5..19e8031a3f 100644 >> --- a/hw/pci-host/mv64361.c >> +++ b/hw/pci-host/mv64361.c >> @@ -873,10 +873,6 @@ static void mv64361_realize(DeviceState *dev, Error >> **errp) >> } >> sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq); >> qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32); >> - /* FIXME: PCI IRQ connections may be board specific */ >> - for (i = 0; i < PCI_NUM_PINS; i++) { >> - s->pci[1].irq[i] = qdev_get_gpio_in_named(dev, "gpp", 12 + i); >> - } >> } >> static void mv64361_reset(DeviceState *dev) >> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c >> index 7cc375df05..f1650be5ee 100644 >> --- a/hw/ppc/pegasos2.c >> +++ b/hw/ppc/pegasos2.c >> @@ -73,6 +73,8 @@ struct Pegasos2MachineState { >> MachineState parent_obj; >> PowerPCCPU *cpu; >> DeviceState *mv; >> + qemu_irq mv_pirq[PCI_NUM_PINS]; >> + qemu_irq via_pirq[PCI_NUM_PINS]; >> Vof *vof; >> void *fdt_blob; >> uint64_t kernel_addr; >> @@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque) >> } >> } >> +static void pegasos2_pci_irq(void *opaque, int n, int level) > > So this handler is only for A/B. We could rename it pegasos2_agp_irq() > and only wire it to the first 2 pins, but I since we can only register > one handler per bus, simpler to ... > >> +{ >> + Pegasos2MachineState *pm = opaque; >> + >> + /* PCI interrupt lines are connected to both MV64361 and VT8231 */ >> + qemu_set_irq(pm->mv_pirq[n], level); > > ... add a 'if (n < 2)' check here. > >> + qemu_set_irq(pm->via_pirq[n], level); >> +} > >> @@ -156,11 +167,18 @@ static void pegasos2_init(MachineState *machine) >> /* Marvell Discovery II system controller */ >> pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1, >> qdev_get_gpio_in(DEVICE(pm->cpu), >> PPC6xx_INPUT_INT))); >> + for (i = 0; i < PCI_NUM_PINS; i++) { >> + pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i); >> + } >> pci_bus = mv64361_get_pci_bus(pm->mv, 1); >> + pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); >> /* VIA VT8231 South Bridge (multifunction PCI device) */ >> via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, >> 0), >> true, TYPE_VT8231_ISA)); >> + for (i = 0; i < PCI_NUM_PINS; i++) { > > I'd rather declare as via_pirq[2] and iterate over ARRAY_SIZE() here > (and also use ARRAY_SIZE() in the new check in pegasos2_pci_irq). > >> + pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); >> + } > >
On Wed, 8 Mar 2023, BALATON Zoltan wrote: > On Wed, 8 Mar 2023, Philippe Mathieu-Daudé wrote: >> Hi Zoltan, >> >> On 8/3/23 00:47, Philippe Mathieu-Daudé wrote: >>> From: BALATON Zoltan <balaton@eik.bme.hu> >>> >>> According to the PegasosII schematics the PCI interrupt lines are >>> connected to both the gpp pins of the Mv64361 north bridge and the >>> PINT pins of the VT8231 south bridge so guests can get interrupts from >>> either of these. So far we only had the MV64361 connections which >>> worked for on board devices but for additional PCI devices (such as >>> network or sound card added with -device) guest OSes expect interrupt >>> from the ISA IRQ 9 where the firmware routes these PCI interrupts in >>> VT8231 ISA bridge. After the previous patches we can now model this >>> and also remove the board specific connection from mv64361. Also >>> configure routing of these lines when using Virtual Open Firmware to >>> match board firmware for guests that expect this. >> >> IIUC the schematic, only tje INTA and INTB lines (AGP IRQs) are >> bidirectional and shared between NB/SB. >> >> INTC and INTC are SB output to NB input. > > I'll check the schematics again when I have time later but what we know for > sure is that guests expect PCI interrupts to raise ISA IRQ9 which is mapped > by the VT8231 ISA function 0c55-0x57 registers. The PCI buses are otherwise > handled by the north bridge. So how can the VIA PINT pins be outputs? Where > do the signals from the PCI cards go into VT8231 otherwise? Also the VT8231 > datasheet on page 10 says PINTA-D pins are inputs so I can't understand your > reasoning above. I think this may be an error in some schematic pages or nore likely something we don't undestand about the drawing as both Sheet 2 with MV64361 and Sheet 18 with VT8231 show the interrupts as inputs (which also matches the VT8231 datasheet). Other sheets with individual PCI slots show these interrupt lines as outputs as expected. Only sheet 13 shows INTA and B as bidirectional and INTC and D as output but maybe only because that sheet also shows the PCI slots from which the lines are output and the VT8231 in which they are inputs so together for this sheet they are both input and output but for the chips they are input only and for the slots they are output only. The main sheet also has these directions maybe because it shows links to the sheet that has the slots from where the IRQs are coming and also links to the AGP port where only INTA and B are connected so for these they are inputs for sheet 13. So this shows that AGP interrupts are also inputs to Sheet 13 whereas other PCI INTC and D are only outputs from this sheet because they are routed to the VT8231 sheet within sheet 13 so at higher level they don't appear as both the outputs and inputs of INTC and D are on Sheet 13. So I think what we have here is correct and matches the schematics where PCI interrupts come from slots, they are connected to VT8231 on Sheet 13 but INTA and B can also come from AGP which is on sheet 4 so these are also input to sheet 13, finally all of these are output towards sheet 2 with MV64361 that's why the top level sheet has confusing bidirectional arrows but if you look at the chips the lines are input there and output at the slots which is only what makes sense anyway. What is it that bothers you and Mark about this that you both want to change it to something else and what makes you think this can't be right? Regards, BALATON Zoltan >>> This fixes PCI interrupts on pegasos2 under Linux, MorphOS and AmigaOS. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>> Message-Id: >>> <520ff9e6eeef600ee14a4116c0c7b11940cc499c.1678188711.git.balaton@eik.bme.hu> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/pci-host/mv64361.c | 4 ---- >>> hw/ppc/pegasos2.c | 26 +++++++++++++++++++++++++- >>> 2 files changed, 25 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/pci-host/mv64361.c b/hw/pci-host/mv64361.c >>> index 298564f1f5..19e8031a3f 100644 >>> --- a/hw/pci-host/mv64361.c >>> +++ b/hw/pci-host/mv64361.c >>> @@ -873,10 +873,6 @@ static void mv64361_realize(DeviceState *dev, Error >>> **errp) >>> } >>> sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq); >>> qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32); >>> - /* FIXME: PCI IRQ connections may be board specific */ >>> - for (i = 0; i < PCI_NUM_PINS; i++) { >>> - s->pci[1].irq[i] = qdev_get_gpio_in_named(dev, "gpp", 12 + i); >>> - } >>> } >>> static void mv64361_reset(DeviceState *dev) >>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c >>> index 7cc375df05..f1650be5ee 100644 >>> --- a/hw/ppc/pegasos2.c >>> +++ b/hw/ppc/pegasos2.c >>> @@ -73,6 +73,8 @@ struct Pegasos2MachineState { >>> MachineState parent_obj; >>> PowerPCCPU *cpu; >>> DeviceState *mv; >>> + qemu_irq mv_pirq[PCI_NUM_PINS]; >>> + qemu_irq via_pirq[PCI_NUM_PINS]; >>> Vof *vof; >>> void *fdt_blob; >>> uint64_t kernel_addr; >>> @@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque) >>> } >>> } >>> +static void pegasos2_pci_irq(void *opaque, int n, int level) >> >> So this handler is only for A/B. We could rename it pegasos2_agp_irq() >> and only wire it to the first 2 pins, but I since we can only register >> one handler per bus, simpler to ... >> >>> +{ >>> + Pegasos2MachineState *pm = opaque; >>> + >>> + /* PCI interrupt lines are connected to both MV64361 and VT8231 */ >>> + qemu_set_irq(pm->mv_pirq[n], level); >> >> ... add a 'if (n < 2)' check here. >> >>> + qemu_set_irq(pm->via_pirq[n], level); >>> +} >> >>> @@ -156,11 +167,18 @@ static void pegasos2_init(MachineState *machine) >>> /* Marvell Discovery II system controller */ >>> pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1, >>> qdev_get_gpio_in(DEVICE(pm->cpu), >>> PPC6xx_INPUT_INT))); >>> + for (i = 0; i < PCI_NUM_PINS; i++) { >>> + pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i); >>> + } >>> pci_bus = mv64361_get_pci_bus(pm->mv, 1); >>> + pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); >>> /* VIA VT8231 South Bridge (multifunction PCI device) */ >>> via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, >>> 0), >>> true, >>> TYPE_VT8231_ISA)); >>> + for (i = 0; i < PCI_NUM_PINS; i++) { >> >> I'd rather declare as via_pirq[2] and iterate over ARRAY_SIZE() here >> (and also use ARRAY_SIZE() in the new check in pegasos2_pci_irq). >> >>> + pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); >>> + } >> >
diff --git a/hw/pci-host/mv64361.c b/hw/pci-host/mv64361.c index 298564f1f5..19e8031a3f 100644 --- a/hw/pci-host/mv64361.c +++ b/hw/pci-host/mv64361.c @@ -873,10 +873,6 @@ static void mv64361_realize(DeviceState *dev, Error **errp) } sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq); qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32); - /* FIXME: PCI IRQ connections may be board specific */ - for (i = 0; i < PCI_NUM_PINS; i++) { - s->pci[1].irq[i] = qdev_get_gpio_in_named(dev, "gpp", 12 + i); - } } static void mv64361_reset(DeviceState *dev) diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c index 7cc375df05..f1650be5ee 100644 --- a/hw/ppc/pegasos2.c +++ b/hw/ppc/pegasos2.c @@ -73,6 +73,8 @@ struct Pegasos2MachineState { MachineState parent_obj; PowerPCCPU *cpu; DeviceState *mv; + qemu_irq mv_pirq[PCI_NUM_PINS]; + qemu_irq via_pirq[PCI_NUM_PINS]; Vof *vof; void *fdt_blob; uint64_t kernel_addr; @@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque) } } +static void pegasos2_pci_irq(void *opaque, int n, int level) +{ + Pegasos2MachineState *pm = opaque; + + /* PCI interrupt lines are connected to both MV64361 and VT8231 */ + qemu_set_irq(pm->mv_pirq[n], level); + qemu_set_irq(pm->via_pirq[n], level); +} + static void pegasos2_init(MachineState *machine) { Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine); @@ -106,7 +117,7 @@ static void pegasos2_init(MachineState *machine) I2CBus *i2c_bus; const char *fwname = machine->firmware ?: PROM_FILENAME; char *filename; - int sz; + int i, sz; uint8_t *spd_data; /* init CPU */ @@ -156,11 +167,18 @@ static void pegasos2_init(MachineState *machine) /* Marvell Discovery II system controller */ pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1, qdev_get_gpio_in(DEVICE(pm->cpu), PPC6xx_INPUT_INT))); + for (i = 0; i < PCI_NUM_PINS; i++) { + pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i); + } pci_bus = mv64361_get_pci_bus(pm->mv, 1); + pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); /* VIA VT8231 South Bridge (multifunction PCI device) */ via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true, TYPE_VT8231_ISA)); + for (i = 0; i < PCI_NUM_PINS; i++) { + pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); + } object_property_add_alias(OBJECT(machine), "rtc-time", object_resolve_path_component(via, "rtc"), "date"); @@ -267,6 +285,12 @@ static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason) PCI_INTERRUPT_LINE, 2, 0x9); pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) | 0x50, 1, 0x2); + pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) | + 0x55, 1, 0x90); + pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) | + 0x56, 1, 0x99); + pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) | + 0x57, 1, 0x90); pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 1) << 8) | PCI_INTERRUPT_LINE, 2, 0x109);