| Message ID | 20230531203559.29140-14-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/timer/arm_timer: QOM'ify ARM_TIMER and correct sysbus/irq in ICP_PIT | expand |
On Wed, 31 May 2023 at 21:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > SysBus IRQ are *output* IRQs. As some sort of simplification > to avoid to forward it, IcpPitState misuses it as ARM timer > input IRQ. Fix that by using a simple IRQ forwarder handler. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/timer/arm_timer.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c > index 6f444e1789..874f9b63bc 100644 > --- a/hw/timer/arm_timer.c > +++ b/hw/timer/arm_timer.c > @@ -352,6 +352,7 @@ struct IntegratorPitState { > MemoryRegion iomem; > ArmTimerState *timer[3]; > qemu_irq irq_in[3]; > + qemu_irq irq[3]; > }; > > static uint64_t icp_pit_read(void *opaque, hwaddr offset, > @@ -391,6 +392,13 @@ static const MemoryRegionOps icp_pit_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > +static void icp_pit_fwd_irq(void *opaque, int n, int level) > +{ > + IntegratorPitState *s = opaque; > + > + qemu_set_irq(s->irq[n], level); > +} > + > static void icp_pit_init(Object *obj) > { > static const uint32_t tmr_freq[] = { > @@ -402,9 +410,14 @@ static void icp_pit_init(Object *obj) > IntegratorPitState *s = INTEGRATOR_PIT(obj); > SysBusDevice *dev = SYS_BUS_DEVICE(obj); > > + qdev_init_gpio_in_named(DEVICE(obj), icp_pit_fwd_irq, > + "timer-in", ARRAY_SIZE(s->timer)); > + > for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) { > s->timer[i] = arm_timer_new(tmr_freq[i], s->irq_in[i]); > - sysbus_init_irq(dev, &s->irq_in[i]); > + sysbus_init_irq(dev, &s->irq[i]); > + sysbus_connect_irq(dev, i, > + qdev_get_gpio_in_named(DEVICE(obj), "timer-in", i)); > } This feels a bit clunky but I think it's what we have to do, since the various _pass_ APIs for forwarding GPIOs and IRQs from a container to an inner device only work with an entire set of IRQs. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 8/6/23 17:09, Peter Maydell wrote: > On Wed, 31 May 2023 at 21:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> SysBus IRQ are *output* IRQs. As some sort of simplification >> to avoid to forward it, IcpPitState misuses it as ARM timer >> input IRQ. Fix that by using a simple IRQ forwarder handler. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/timer/arm_timer.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> +static void icp_pit_fwd_irq(void *opaque, int n, int level) >> +{ >> + IntegratorPitState *s = opaque; >> + >> + qemu_set_irq(s->irq[n], level); >> +} >> + >> static void icp_pit_init(Object *obj) >> { >> static const uint32_t tmr_freq[] = { >> @@ -402,9 +410,14 @@ static void icp_pit_init(Object *obj) >> IntegratorPitState *s = INTEGRATOR_PIT(obj); >> SysBusDevice *dev = SYS_BUS_DEVICE(obj); >> >> + qdev_init_gpio_in_named(DEVICE(obj), icp_pit_fwd_irq, >> + "timer-in", ARRAY_SIZE(s->timer)); >> + >> for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) { >> s->timer[i] = arm_timer_new(tmr_freq[i], s->irq_in[i]); >> - sysbus_init_irq(dev, &s->irq_in[i]); >> + sysbus_init_irq(dev, &s->irq[i]); >> + sysbus_connect_irq(dev, i, >> + qdev_get_gpio_in_named(DEVICE(obj), "timer-in", i)); >> } > > This feels a bit clunky but I think it's what we have to do, > since the various _pass_ APIs for forwarding GPIOs and > IRQs from a container to an inner device only work with > an entire set of IRQs. Indeed. sysbus_pass_irq() could resolve the last unused IRQ index and amend starting at that index, but this doesn't seem a lot of need for a such use (so far). We might consider it if we ever merge sysbus IRQ API into qdev. > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Thanks! Phil.
diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c index 6f444e1789..874f9b63bc 100644 --- a/hw/timer/arm_timer.c +++ b/hw/timer/arm_timer.c @@ -352,6 +352,7 @@ struct IntegratorPitState { MemoryRegion iomem; ArmTimerState *timer[3]; qemu_irq irq_in[3]; + qemu_irq irq[3]; }; static uint64_t icp_pit_read(void *opaque, hwaddr offset, @@ -391,6 +392,13 @@ static const MemoryRegionOps icp_pit_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; +static void icp_pit_fwd_irq(void *opaque, int n, int level) +{ + IntegratorPitState *s = opaque; + + qemu_set_irq(s->irq[n], level); +} + static void icp_pit_init(Object *obj) { static const uint32_t tmr_freq[] = { @@ -402,9 +410,14 @@ static void icp_pit_init(Object *obj) IntegratorPitState *s = INTEGRATOR_PIT(obj); SysBusDevice *dev = SYS_BUS_DEVICE(obj); + qdev_init_gpio_in_named(DEVICE(obj), icp_pit_fwd_irq, + "timer-in", ARRAY_SIZE(s->timer)); + for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) { s->timer[i] = arm_timer_new(tmr_freq[i], s->irq_in[i]); - sysbus_init_irq(dev, &s->irq_in[i]); + sysbus_init_irq(dev, &s->irq[i]); + sysbus_connect_irq(dev, i, + qdev_get_gpio_in_named(DEVICE(obj), "timer-in", i)); } memory_region_init_io(&s->iomem, obj, &icp_pit_ops, s,
SysBus IRQ are *output* IRQs. As some sort of simplification to avoid to forward it, IcpPitState misuses it as ARM timer input IRQ. Fix that by using a simple IRQ forwarder handler. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/timer/arm_timer.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)