| Message ID | 20230606104609.3692557-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | allwinner-a10: Fix interrupt controller regression | expand |
Hi Peter, On 6/6/23 12:46, Peter Maydell wrote: > QEMU allows qemu_irq lines to transfer arbitrary integers. However > the convention is that for a simple IRQ line the values transferred > are always 0 and 1. The A10 SD controller device instead assumes a > 0-vs-non-0 convention, which happens to work with the interrupt > controller it is wired up to. > > Coerce the value to boolean to follow our usual convention. I remember once wanting to convert qemu_set_irq() to take a boolean argument but someone said using integer was more useful because ...? I searched a bit but can't find that in my mail archives, maybe this was on IRC. Any clue? (I find simpler to use a boolean rather than having a convention of using integers restricted to [0, 1] range). Regards, Phil.
On Tue, 6 Jun 2023 at 13:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Hi Peter, > > On 6/6/23 12:46, Peter Maydell wrote: > > QEMU allows qemu_irq lines to transfer arbitrary integers. However > > the convention is that for a simple IRQ line the values transferred > > are always 0 and 1. The A10 SD controller device instead assumes a > > 0-vs-non-0 convention, which happens to work with the interrupt > > controller it is wired up to. > > > > Coerce the value to boolean to follow our usual convention. > > I remember once wanting to convert qemu_set_irq() to take a boolean > argument but someone said using integer was more useful because ...? > I searched a bit but can't find that in my mail archives, maybe this > was on IRC. Any clue? (I find simpler to use a boolean rather than > having a convention of using integers restricted to [0, 1] range). We have a lot of use cases where we just want to transfer a boolean value between two devices. We have a few use cases where we want to transfer an arbitrary integer across the channel between two devices. (For instance hw/intc/etraxfs_pic.c:pic_update() sends a vector number to the CPU via a qemu_irq -- see commit f4f643882d9dc467.) At the moment we use qemu_irq() for both. In theory we could construct a parallel set of machinery for wiring up and setting values for the "want an integer" case and restrict qemu_irq() to bool only, but the lazy path is to use the same function for both. (If we had machinery that made it easy to construct arbitrary strongly-typed signal connections, that might perhaps be ideal. But it's probably not very easy especially in C.) thanks -- PMM
On 6/6/23 14:55, Peter Maydell wrote: > On Tue, 6 Jun 2023 at 13:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Hi Peter, >> >> On 6/6/23 12:46, Peter Maydell wrote: >>> QEMU allows qemu_irq lines to transfer arbitrary integers. However >>> the convention is that for a simple IRQ line the values transferred >>> are always 0 and 1. The A10 SD controller device instead assumes a >>> 0-vs-non-0 convention, which happens to work with the interrupt >>> controller it is wired up to. >>> >>> Coerce the value to boolean to follow our usual convention. >> >> I remember once wanting to convert qemu_set_irq() to take a boolean >> argument but someone said using integer was more useful because ...? >> I searched a bit but can't find that in my mail archives, maybe this >> was on IRC. Any clue? (I find simpler to use a boolean rather than >> having a convention of using integers restricted to [0, 1] range). > > We have a lot of use cases where we just want to transfer a boolean > value between two devices. We have a few use cases where we want to > transfer an arbitrary integer across the channel between two devices. > (For instance hw/intc/etraxfs_pic.c:pic_update() sends a vector > number to the CPU via a qemu_irq -- see commit f4f643882d9dc467.) Thanks for this reference! > At the moment we use qemu_irq() for both. In theory we could > construct a parallel set of machinery for wiring up and setting > values for the "want an integer" case and restrict qemu_irq() to > bool only, but the lazy path is to use the same function for both. > (If we had machinery that made it easy to construct arbitrary > strongly-typed signal connections, that might perhaps be ideal. > But it's probably not very easy especially in C.) I see, thanks.
On Tue, Jun 06, 2023 at 11:46:09AM +0100, Peter Maydell wrote: > QEMU allows qemu_irq lines to transfer arbitrary integers. However > the convention is that for a simple IRQ line the values transferred > are always 0 and 1. The A10 SD controller device instead assumes a > 0-vs-non-0 convention, which happens to work with the interrupt > controller it is wired up to. > > Coerce the value to boolean to follow our usual convention. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Tested-by: Guenter Roeck <linux@roeck-us.net> > --- > hw/sd/allwinner-sdhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c > index 92a0f42708d..62df8f3d396 100644 > --- a/hw/sd/allwinner-sdhost.c > +++ b/hw/sd/allwinner-sdhost.c > @@ -191,7 +191,7 @@ static void allwinner_sdhost_update_irq(AwSdHostState *s) > } > > trace_allwinner_sdhost_update_irq(irq); > - qemu_set_irq(s->irq, irq); > + qemu_set_irq(s->irq, !!irq); > } > > static void allwinner_sdhost_update_transfer_cnt(AwSdHostState *s, > -- > 2.34.1 >
diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c index 92a0f42708d..62df8f3d396 100644 --- a/hw/sd/allwinner-sdhost.c +++ b/hw/sd/allwinner-sdhost.c @@ -191,7 +191,7 @@ static void allwinner_sdhost_update_irq(AwSdHostState *s) } trace_allwinner_sdhost_update_irq(irq); - qemu_set_irq(s->irq, irq); + qemu_set_irq(s->irq, !!irq); } static void allwinner_sdhost_update_transfer_cnt(AwSdHostState *s,
QEMU allows qemu_irq lines to transfer arbitrary integers. However the convention is that for a simple IRQ line the values transferred are always 0 and 1. The A10 SD controller device instead assumes a 0-vs-non-0 convention, which happens to work with the interrupt controller it is wired up to. Coerce the value to boolean to follow our usual convention. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/sd/allwinner-sdhost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)