| Message ID | 20220522181836.864-46-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | PS2 device QOMification - part 1 | expand |
On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > This enables the IRQ to be wired up using qdev_connect_gpio_out() in > lasips2_initfn(). > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/input/lasips2.c | 8 ++++---- > include/hw/input/lasips2.h | 2 ++ > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c > index 6849b71e5c..644cf70955 100644 > --- a/hw/input/lasips2.c > +++ b/hw/input/lasips2.c > @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level) > > LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq) > { > - LASIPS2State *s; > DeviceState *dev; > > dev = qdev_new(TYPE_LASIPS2); > qdev_prop_set_uint64(dev, "base", base); > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > - s = LASIPS2(dev); > + qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq); > > - s->irq = irq; > - return s; > + return LASIPS2(dev); > } > > static void lasips2_realize(DeviceState *dev, Error **errp) > @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj) > > sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg); > sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg); > + > + qdev_init_gpio_out(DEVICE(obj), &s->irq, 1); > } > > static Property lasips2_properties[] = { > diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h > index 7e4437b925..d3e9719d65 100644 > --- a/include/hw/input/lasips2.h > +++ b/include/hw/input/lasips2.h > @@ -22,6 +22,8 @@ typedef struct LASIPS2Port { > bool irq; > } LASIPS2Port; > > +#define LASIPS2_IRQ 0 If you find yourself #defining names for IRQ lines then this is probably a sign you should be using named GPIO lines :-) Alternatively, maybe use sysbus_init_irq()? By convention the only sysbus IRQ on a device is generally "its IRQ line". -- PMM
On 09/06/2022 12:18, Peter Maydell wrote: > On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> This enables the IRQ to be wired up using qdev_connect_gpio_out() in >> lasips2_initfn(). >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/input/lasips2.c | 8 ++++---- >> include/hw/input/lasips2.h | 2 ++ >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c >> index 6849b71e5c..644cf70955 100644 >> --- a/hw/input/lasips2.c >> +++ b/hw/input/lasips2.c >> @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level) >> >> LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq) >> { >> - LASIPS2State *s; >> DeviceState *dev; >> >> dev = qdev_new(TYPE_LASIPS2); >> qdev_prop_set_uint64(dev, "base", base); >> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >> - s = LASIPS2(dev); >> + qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq); >> >> - s->irq = irq; >> - return s; >> + return LASIPS2(dev); >> } >> >> static void lasips2_realize(DeviceState *dev, Error **errp) >> @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj) >> >> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg); >> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg); >> + >> + qdev_init_gpio_out(DEVICE(obj), &s->irq, 1); >> } >> >> static Property lasips2_properties[] = { >> diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h >> index 7e4437b925..d3e9719d65 100644 >> --- a/include/hw/input/lasips2.h >> +++ b/include/hw/input/lasips2.h >> @@ -22,6 +22,8 @@ typedef struct LASIPS2Port { >> bool irq; >> } LASIPS2Port; >> >> +#define LASIPS2_IRQ 0 > > If you find yourself #defining names for IRQ lines then this is > probably a sign you should be using named GPIO lines :-) Yeah that's something I've done a few times here, mainly to have just one "set IRQ" function rather a separate one for both keyboard and mouse. It takes a bit more work, but I can certainly separate them out. > Alternatively, maybe use sysbus_init_irq()? By convention the > only sysbus IRQ on a device is generally "its IRQ line". Thinking longer term about sysbus, I can see that sysbus_init_irq() would be one of the top entries on my list of things to go. For that reason I'd like to stick to using gpios here :) ATB, Mark.
On 10/06/2022 08:17, Mark Cave-Ayland wrote: > On 09/06/2022 12:18, Peter Maydell wrote: > >> On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland >> <mark.cave-ayland@ilande.co.uk> wrote: >>> >>> This enables the IRQ to be wired up using qdev_connect_gpio_out() in >>> lasips2_initfn(). >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> hw/input/lasips2.c | 8 ++++---- >>> include/hw/input/lasips2.h | 2 ++ >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c >>> index 6849b71e5c..644cf70955 100644 >>> --- a/hw/input/lasips2.c >>> +++ b/hw/input/lasips2.c >>> @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level) >>> >>> LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq) >>> { >>> - LASIPS2State *s; >>> DeviceState *dev; >>> >>> dev = qdev_new(TYPE_LASIPS2); >>> qdev_prop_set_uint64(dev, "base", base); >>> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >>> - s = LASIPS2(dev); >>> + qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq); >>> >>> - s->irq = irq; >>> - return s; >>> + return LASIPS2(dev); >>> } >>> >>> static void lasips2_realize(DeviceState *dev, Error **errp) >>> @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj) >>> >>> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg); >>> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg); >>> + >>> + qdev_init_gpio_out(DEVICE(obj), &s->irq, 1); >>> } >>> >>> static Property lasips2_properties[] = { >>> diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h >>> index 7e4437b925..d3e9719d65 100644 >>> --- a/include/hw/input/lasips2.h >>> +++ b/include/hw/input/lasips2.h >>> @@ -22,6 +22,8 @@ typedef struct LASIPS2Port { >>> bool irq; >>> } LASIPS2Port; >>> >>> +#define LASIPS2_IRQ 0 >> >> If you find yourself #defining names for IRQ lines then this is >> probably a sign you should be using named GPIO lines :-) > > Yeah that's something I've done a few times here, mainly to have just one "set IRQ" > function rather a separate one for both keyboard and mouse. It takes a bit more work, > but I can certainly separate them out. Looking at this again, the gpio being defined here actually is the (only) lasips2 output IRQ, and so should be left unnamed. The reason for adding LASIPS2_IRQ was so that the gpio connection process looked like: qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq); instead of: qdev_connect_gpio_out(dev, 0, irq); Would you still prefer for me to simply hardcode 0 here and drop the LASIPS2_IRQ define in this case since there is only one output IRQ? >> Alternatively, maybe use sysbus_init_irq()? By convention the >> only sysbus IRQ on a device is generally "its IRQ line". > > Thinking longer term about sysbus, I can see that sysbus_init_irq() would be one of > the top entries on my list of things to go. For that reason I'd like to stick to > using gpios here :) ATB, Mark.
On Sat, 11 Jun 2022 at 16:44, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 10/06/2022 08:17, Mark Cave-Ayland wrote: > > > On 09/06/2022 12:18, Peter Maydell wrote: > >> If you find yourself #defining names for IRQ lines then this is > >> probably a sign you should be using named GPIO lines :-) > > > > Yeah that's something I've done a few times here, mainly to have just one "set IRQ" > > function rather a separate one for both keyboard and mouse. It takes a bit more work, > > but I can certainly separate them out. > > Looking at this again, the gpio being defined here actually is the (only) lasips2 > output IRQ, and so should be left unnamed. > > The reason for adding LASIPS2_IRQ was so that the gpio connection process looked like: > > qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq); > > instead of: > > qdev_connect_gpio_out(dev, 0, irq); > > Would you still prefer for me to simply hardcode 0 here and drop the LASIPS2_IRQ > define in this case since there is only one output IRQ? Well, I think that "unnamed GPIO out" lines should be for actual GPIO lines, ie on a GPIO controller or similar. If you want an outbound IRQ line and don't want to name it, that's what sysbus IRQ lines do. Otherwise, name the GPIO line. thanks -- PMM
On 20/06/2022 11:17, Peter Maydell wrote: > On Sat, 11 Jun 2022 at 16:44, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 10/06/2022 08:17, Mark Cave-Ayland wrote: >> >>> On 09/06/2022 12:18, Peter Maydell wrote: >>>> If you find yourself #defining names for IRQ lines then this is >>>> probably a sign you should be using named GPIO lines :-) >>> >>> Yeah that's something I've done a few times here, mainly to have just one "set IRQ" >>> function rather a separate one for both keyboard and mouse. It takes a bit more work, >>> but I can certainly separate them out. >> >> Looking at this again, the gpio being defined here actually is the (only) lasips2 >> output IRQ, and so should be left unnamed. >> >> The reason for adding LASIPS2_IRQ was so that the gpio connection process looked like: >> >> qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq); >> >> instead of: >> >> qdev_connect_gpio_out(dev, 0, irq); >> >> Would you still prefer for me to simply hardcode 0 here and drop the LASIPS2_IRQ >> define in this case since there is only one output IRQ? > > Well, I think that "unnamed GPIO out" lines should be for > actual GPIO lines, ie on a GPIO controller or similar. > If you want an outbound IRQ line and don't want to name it, > that's what sysbus IRQ lines do. Otherwise, name the GPIO line. That's interesting - I've always been under the impression that this was the other way around, i.e. for a TYPE_DEVICE then unnamed gpios are equivalent to IRQs, and that gpio lines for any other non-IRQ purpose should be named :/ So... I'm not really sure what to do in the context of this patchset. Would using qdev gpios with suitable "QEMU interface" comments be good enough, or do you really feel strongly that these should be converted to SysBus IRQs? Following on from this we should also consider starting a new thread re: the future of SysBusDevice and how we would like to model SysBus IRQs and mmio regions going forward. ATB, Mark.
On Mon, 20 Jun 2022 at 14:22, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 20/06/2022 11:17, Peter Maydell wrote: > > Well, I think that "unnamed GPIO out" lines should be for > > actual GPIO lines, ie on a GPIO controller or similar. > > If you want an outbound IRQ line and don't want to name it, > > that's what sysbus IRQ lines do. Otherwise, name the GPIO line. > > That's interesting - I've always been under the impression that this was the other > way around, i.e. for a TYPE_DEVICE then unnamed gpios are equivalent to IRQs, and > that gpio lines for any other non-IRQ purpose should be named :/ Well, named GPIO lines are relatively new, so if you look at older devices you'll probably find plenty that use unnamed GPIO lines for various things including IRQ lines. But I think that for clarity if you create something called "gpio_out" the obvious thing is that that's a GPIO output, and if you create something called "sysbus_irq" the obvious thing is that that's an IRQ line, and if you want to do something that's neither of those then the clearest thing is to name the GPIO. -- PMM
On 20/06/2022 15:13, Peter Maydell wrote: > On Mon, 20 Jun 2022 at 14:22, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 20/06/2022 11:17, Peter Maydell wrote: >>> Well, I think that "unnamed GPIO out" lines should be for >>> actual GPIO lines, ie on a GPIO controller or similar. >>> If you want an outbound IRQ line and don't want to name it, >>> that's what sysbus IRQ lines do. Otherwise, name the GPIO line. >> >> That's interesting - I've always been under the impression that this was the other >> way around, i.e. for a TYPE_DEVICE then unnamed gpios are equivalent to IRQs, and >> that gpio lines for any other non-IRQ purpose should be named :/ > > Well, named GPIO lines are relatively new, so if you look at older > devices you'll probably find plenty that use unnamed GPIO lines > for various things including IRQ lines. But I think that for clarity > if you create something called "gpio_out" the obvious thing is that > that's a GPIO output, and if you create something called "sysbus_irq" > the obvious thing is that that's an IRQ line, and if you want to > do something that's neither of those then the clearest thing is > to name the GPIO. Ultimately I'm not too concerned about the choice between sysbus IRQs instead of gpio outputs, since making the change later is quite trivial. I've gone ahead and updated this patch to use a sysbus IRQ instead of an unnamed gpio out for v2. ATB, Mark.
diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c index 6849b71e5c..644cf70955 100644 --- a/hw/input/lasips2.c +++ b/hw/input/lasips2.c @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level) LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq) { - LASIPS2State *s; DeviceState *dev; dev = qdev_new(TYPE_LASIPS2); qdev_prop_set_uint64(dev, "base", base); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); - s = LASIPS2(dev); + qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq); - s->irq = irq; - return s; + return LASIPS2(dev); } static void lasips2_realize(DeviceState *dev, Error **errp) @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj) sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg); sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg); + + qdev_init_gpio_out(DEVICE(obj), &s->irq, 1); } static Property lasips2_properties[] = { diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h index 7e4437b925..d3e9719d65 100644 --- a/include/hw/input/lasips2.h +++ b/include/hw/input/lasips2.h @@ -22,6 +22,8 @@ typedef struct LASIPS2Port { bool irq; } LASIPS2Port; +#define LASIPS2_IRQ 0 + struct LASIPS2State { SysBusDevice parent_obj;
This enables the IRQ to be wired up using qdev_connect_gpio_out() in lasips2_initfn(). Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/input/lasips2.c | 8 ++++---- include/hw/input/lasips2.h | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-)