| Message ID | 20230601094625.39569-8-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sysbus: Add sysbus_init_irqs and reduce SYSBUS_DEVICE_GPIO_IRQ scope | expand |
On Thu, 1 Jun 2023 at 10:48, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Audit the sysbus_init_irq() calls and manually convert > to sysbus_init_irqs() when a loop is involved. > > In omap2_intc_init(), the parent_intr[] array contains > 2 elements: use ARRAY_SIZE() to iterate over. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c > index c15f654738..dd2929d6e7 100644 > --- a/hw/timer/renesas_tmr.c > +++ b/hw/timer/renesas_tmr.c > @@ -428,17 +428,14 @@ static void rtmr_init(Object *obj) > { > SysBusDevice *d = SYS_BUS_DEVICE(obj); > RTMRState *tmr = RTMR(obj); > - int i; > > memory_region_init_io(&tmr->memory, OBJECT(tmr), &tmr_ops, > tmr, "renesas-tmr", 0x10); > sysbus_init_mmio(d, &tmr->memory); > > - for (i = 0; i < ARRAY_SIZE(tmr->ovi); i++) { > - sysbus_init_irq(d, &tmr->cmia[i]); > - sysbus_init_irq(d, &tmr->cmib[i]); > - sysbus_init_irq(d, &tmr->ovi[i]); > - } > + sysbus_init_irqs(d, tmr->cmia, ARRAY_SIZE(tmr->cmia)); > + sysbus_init_irqs(d, tmr->cmib, ARRAY_SIZE(tmr->cmib)); > + sysbus_init_irqs(d, tmr->ovi, ARRAY_SIZE(tmr->ovi)); Doesn't this change the order of the IRQs? Previously we had channel 0 CMIA, channel 0 CMIA, channel 0 OVI, channel 1 CMIA, channel 1 CMIB, channel 1 OVI. Now we have channel 0 CMIA, channel 1 CMIA, channel 0 CMIB, channel 1 CMIB, channel 0 OVI, channel 1 OVI. So they'll get miswired in the board code now... thanks -- PMM
On 1/6/23 12:19, Peter Maydell wrote: > On Thu, 1 Jun 2023 at 10:48, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Audit the sysbus_init_irq() calls and manually convert >> to sysbus_init_irqs() when a loop is involved. >> >> In omap2_intc_init(), the parent_intr[] array contains >> 2 elements: use ARRAY_SIZE() to iterate over. >> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c >> index c15f654738..dd2929d6e7 100644 >> --- a/hw/timer/renesas_tmr.c >> +++ b/hw/timer/renesas_tmr.c >> @@ -428,17 +428,14 @@ static void rtmr_init(Object *obj) >> { >> SysBusDevice *d = SYS_BUS_DEVICE(obj); >> RTMRState *tmr = RTMR(obj); >> - int i; >> >> memory_region_init_io(&tmr->memory, OBJECT(tmr), &tmr_ops, >> tmr, "renesas-tmr", 0x10); >> sysbus_init_mmio(d, &tmr->memory); >> >> - for (i = 0; i < ARRAY_SIZE(tmr->ovi); i++) { >> - sysbus_init_irq(d, &tmr->cmia[i]); >> - sysbus_init_irq(d, &tmr->cmib[i]); >> - sysbus_init_irq(d, &tmr->ovi[i]); >> - } >> + sysbus_init_irqs(d, tmr->cmia, ARRAY_SIZE(tmr->cmia)); >> + sysbus_init_irqs(d, tmr->cmib, ARRAY_SIZE(tmr->cmib)); >> + sysbus_init_irqs(d, tmr->ovi, ARRAY_SIZE(tmr->ovi)); > > Doesn't this change the order of the IRQs? Previously > we had channel 0 CMIA, channel 0 CMIA, channel 0 OVI, > channel 1 CMIA, channel 1 CMIB, channel 1 OVI. Now > we have channel 0 CMIA, channel 1 CMIA, channel 0 CMIB, > channel 1 CMIB, channel 0 OVI, channel 1 OVI. So they'll > get miswired in the board code now... Oops, good catch. Good case to show the sysbus_init_irq() is fragile. At least named_gpio are clearer. I'm surprised because v1 series passed CI, but looking closer the RX tests isn't run. And now I see commit e5d402b28f ("tests/acceptance: disable machine_rx_gdbsim on GitLab"). "Hopefully we can re-enable both once the serial timing patches have been added." Alex, are these patches in the tree now?
diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c index 647bf324a8..f324b640e3 100644 --- a/hw/intc/omap_intc.c +++ b/hw/intc/omap_intc.c @@ -627,8 +627,7 @@ static void omap2_intc_init(Object *obj) s->level_only = 1; s->nbanks = 3; - sysbus_init_irq(sbd, &s->parent_intr[0]); - sysbus_init_irq(sbd, &s->parent_intr[1]); + sysbus_init_irqs(sbd, s->parent_intr, ARRAY_SIZE(s->parent_intr)); qdev_init_gpio_in(dev, omap_set_intr_noedge, s->nbanks * 32); memory_region_init_io(&s->mmio, obj, &omap2_inth_mem_ops, s, "omap2-intc", 0x1000); diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c index a6752fac5e..7b46e3e36e 100644 --- a/hw/pci-host/gpex.c +++ b/hw/pci-host/gpex.c @@ -128,8 +128,8 @@ static void gpex_host_realize(DeviceState *dev, Error **errp) sysbus_init_mmio(sbd, &s->io_ioport); } + sysbus_init_irqs(sbd, s->irq, GPEX_NUM_IRQS); for (i = 0; i < GPEX_NUM_IRQS; i++) { - sysbus_init_irq(sbd, &s->irq[i]); s->irq_num[i] = -1; } diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c index c15f654738..dd2929d6e7 100644 --- a/hw/timer/renesas_tmr.c +++ b/hw/timer/renesas_tmr.c @@ -428,17 +428,14 @@ static void rtmr_init(Object *obj) { SysBusDevice *d = SYS_BUS_DEVICE(obj); RTMRState *tmr = RTMR(obj); - int i; memory_region_init_io(&tmr->memory, OBJECT(tmr), &tmr_ops, tmr, "renesas-tmr", 0x10); sysbus_init_mmio(d, &tmr->memory); - for (i = 0; i < ARRAY_SIZE(tmr->ovi); i++) { - sysbus_init_irq(d, &tmr->cmia[i]); - sysbus_init_irq(d, &tmr->cmib[i]); - sysbus_init_irq(d, &tmr->ovi[i]); - } + sysbus_init_irqs(d, tmr->cmia, ARRAY_SIZE(tmr->cmia)); + sysbus_init_irqs(d, tmr->cmib, ARRAY_SIZE(tmr->cmib)); + sysbus_init_irqs(d, tmr->ovi, ARRAY_SIZE(tmr->ovi)); timer_init_ns(&tmr->timer[0], QEMU_CLOCK_VIRTUAL, timer_event0, tmr); timer_init_ns(&tmr->timer[1], QEMU_CLOCK_VIRTUAL, timer_event1, tmr); }