| Message ID | 20200423090118.11199-5-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a GPIO backend | expand |
On 4/23/20 11:01 AM, Geert Uytterhoeven wrote: > Make the PL061 GPIO controller user-creatable, and allow the user to tie > a newly created instance to a gpiochip on the host. > > To create a new GPIO controller, the QEMU command line must be augmented > with: > > -device pl061,host=<gpiochip> > > with <gpiochip> the name or label of the gpiochip on the host. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2: > - New. > --- > hw/gpio/pl061.c | 35 +++++++++++++++++++++++++++++++++++ > qemu-options.hx | 9 +++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c > index 74ba733a8a5e8ca5..98204f9a586ae8c8 100644 > --- a/hw/gpio/pl061.c > +++ b/hw/gpio/pl061.c > @@ -12,11 +12,14 @@ > #include "hw/arm/fdt.h" > #include "hw/gpio/pl061.h" > #include "hw/irq.h" > +#include "hw/qdev-properties.h" > #include "hw/sysbus.h" > #include "migration/vmstate.h" > +#include "qapi/error.h" > #include "qemu/log.h" > #include "qemu/module.h" > #include "sysemu/device_tree.h" > +#include "sysemu/gpiodev.h" > > //#define DEBUG_PL061 1 > > @@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] = > typedef struct PL061State { > SysBusDevice parent_obj; > > +#ifdef CONFIG_GPIODEV > + char *host; > +#endif > MemoryRegion iomem; > uint32_t locked; > uint32_t data; > @@ -370,10 +376,39 @@ static void pl061_init(Object *obj) > qdev_init_gpio_out(dev, s->out, 8); Not related to this patch, but we should replace this 8 magic value by a proper definition... > } > > +#ifdef CONFIG_GPIODEV > +static Property pl061_properties[] = { > + DEFINE_PROP_STRING("host", PL061State, host), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void pl061_realize(DeviceState *dev, Error **errp) > +{ > + PL061State *s = PL061(dev); > + > + if (!dev->opts) { > + /* Not created by user */ > + return; > + } > + > + if (!s->host) { > + error_setg(errp, "'host' property is required"); > + return; > + } > + > + qemu_gpiodev_add(dev, s->host, 8, errp); > +} > +#endif /* CONFIG_GPIODEV */ > + > static void pl061_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > > +#ifdef CONFIG_GPIODEV > + device_class_set_props(dc, pl061_properties); > + dc->realize = pl061_realize; > + dc->user_creatable = true; > +#endif > dc->vmsd = &vmstate_pl061; > dc->reset = &pl061_reset; > } > diff --git a/qemu-options.hx b/qemu-options.hx > index 292d4e7c0cef6097..182de7fb63923b38 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -875,6 +875,15 @@ SRST > ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]`` > Like the KCS interface, but defines a BT interface. The default port > is 0xe4 and the default interrupt is 5. > + > +#ifdef CONFIG_GPIODEV > +``-device pl061,host=gpiochip`` > + Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO > + controller on the host. > + > + ``host=gpiochip`` > + The name or label of the GPIO controller on the host. > +#endif > ERST > > DEF("name", HAS_ARG, QEMU_OPTION_name, > Instead of restricting this to the pl061, it would be cleaner you add a GPIO_PLUGGABLE_INTERFACE (or GPIO_BINDABLE_INTERFACE or better name), and have TYPE_PL061 implement it.
On 4/23/20 11:33 AM, Philippe Mathieu-Daudé wrote: > On 4/23/20 11:01 AM, Geert Uytterhoeven wrote: >> Make the PL061 GPIO controller user-creatable, and allow the user to tie >> a newly created instance to a gpiochip on the host. >> >> To create a new GPIO controller, the QEMU command line must be augmented >> with: >> >> -device pl061,host=<gpiochip> >> >> with <gpiochip> the name or label of the gpiochip on the host. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> v2: >> - New. >> --- >> hw/gpio/pl061.c | 35 +++++++++++++++++++++++++++++++++++ >> qemu-options.hx | 9 +++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c >> index 74ba733a8a5e8ca5..98204f9a586ae8c8 100644 >> --- a/hw/gpio/pl061.c >> +++ b/hw/gpio/pl061.c >> @@ -12,11 +12,14 @@ >> #include "hw/arm/fdt.h" >> #include "hw/gpio/pl061.h" >> #include "hw/irq.h" >> +#include "hw/qdev-properties.h" >> #include "hw/sysbus.h" >> #include "migration/vmstate.h" >> +#include "qapi/error.h" >> #include "qemu/log.h" >> #include "qemu/module.h" >> #include "sysemu/device_tree.h" >> +#include "sysemu/gpiodev.h" >> >> //#define DEBUG_PL061 1 >> >> @@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] = >> typedef struct PL061State { >> SysBusDevice parent_obj; >> >> +#ifdef CONFIG_GPIODEV >> + char *host; >> +#endif >> MemoryRegion iomem; >> uint32_t locked; >> uint32_t data; >> @@ -370,10 +376,39 @@ static void pl061_init(Object *obj) >> qdev_init_gpio_out(dev, s->out, 8); > > Not related to this patch, but we should replace this 8 magic value by a > proper definition... > >> } >> >> +#ifdef CONFIG_GPIODEV >> +static Property pl061_properties[] = { >> + DEFINE_PROP_STRING("host", PL061State, host), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static void pl061_realize(DeviceState *dev, Error **errp) >> +{ >> + PL061State *s = PL061(dev); >> + >> + if (!dev->opts) { >> + /* Not created by user */ >> + return; >> + } >> + >> + if (!s->host) { >> + error_setg(errp, "'host' property is required"); >> + return; >> + } >> + >> + qemu_gpiodev_add(dev, s->host, 8, errp); >> +} >> +#endif /* CONFIG_GPIODEV */ >> + >> static void pl061_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> >> +#ifdef CONFIG_GPIODEV >> + device_class_set_props(dc, pl061_properties); >> + dc->realize = pl061_realize; >> + dc->user_creatable = true; >> +#endif >> dc->vmsd = &vmstate_pl061; >> dc->reset = &pl061_reset; >> } >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 292d4e7c0cef6097..182de7fb63923b38 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -875,6 +875,15 @@ SRST >> ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]`` >> Like the KCS interface, but defines a BT interface. The default port >> is 0xe4 and the default interrupt is 5. >> + >> +#ifdef CONFIG_GPIODEV >> +``-device pl061,host=gpiochip`` >> + Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO >> + controller on the host. >> + >> + ``host=gpiochip`` >> + The name or label of the GPIO controller on the host. >> +#endif >> ERST >> >> DEF("name", HAS_ARG, QEMU_OPTION_name, >> > > Instead of restricting this to the pl061, it would be cleaner you add a > GPIO_PLUGGABLE_INTERFACE (or GPIO_BINDABLE_INTERFACE or better name), > and have TYPE_PL061 implement it. IOW your backend should consume devices implementing this generic interface.
Hi Philippe, On Thu, Apr 23, 2020 at 12:08 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 4/23/20 11:33 AM, Philippe Mathieu-Daudé wrote: > > On 4/23/20 11:01 AM, Geert Uytterhoeven wrote: > >> Make the PL061 GPIO controller user-creatable, and allow the user to tie > >> a newly created instance to a gpiochip on the host. > >> > >> To create a new GPIO controller, the QEMU command line must be augmented > >> with: > >> > >> -device pl061,host=<gpiochip> > >> > >> with <gpiochip> the name or label of the gpiochip on the host. > >> > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> --- a/hw/gpio/pl061.c > >> +++ b/hw/gpio/pl061.c > >> @@ -12,11 +12,14 @@ > >> #include "hw/arm/fdt.h" > >> #include "hw/gpio/pl061.h" > >> #include "hw/irq.h" > >> +#include "hw/qdev-properties.h" > >> #include "hw/sysbus.h" > >> #include "migration/vmstate.h" > >> +#include "qapi/error.h" > >> #include "qemu/log.h" > >> #include "qemu/module.h" > >> #include "sysemu/device_tree.h" > >> +#include "sysemu/gpiodev.h" > >> > >> //#define DEBUG_PL061 1 > >> > >> @@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] = > >> typedef struct PL061State { > >> SysBusDevice parent_obj; > >> > >> +#ifdef CONFIG_GPIODEV > >> + char *host; > >> +#endif > >> MemoryRegion iomem; > >> uint32_t locked; > >> uint32_t data; > >> @@ -370,10 +376,39 @@ static void pl061_init(Object *obj) > >> qdev_init_gpio_out(dev, s->out, 8); > >> > >> +#ifdef CONFIG_GPIODEV > >> +static Property pl061_properties[] = { > >> + DEFINE_PROP_STRING("host", PL061State, host), > >> + DEFINE_PROP_END_OF_LIST(), > >> +}; > >> + > >> +static void pl061_realize(DeviceState *dev, Error **errp) > >> +{ > >> + PL061State *s = PL061(dev); > >> + > >> + if (!dev->opts) { > >> + /* Not created by user */ > >> + return; > >> + } > >> + > >> + if (!s->host) { > >> + error_setg(errp, "'host' property is required"); > >> + return; > >> + } > >> + > >> + qemu_gpiodev_add(dev, s->host, 8, errp); > >> +} > >> +#endif /* CONFIG_GPIODEV */ > >> + > >> static void pl061_class_init(ObjectClass *klass, void *data) > >> { > >> DeviceClass *dc = DEVICE_CLASS(klass); > >> > >> +#ifdef CONFIG_GPIODEV > >> + device_class_set_props(dc, pl061_properties); > >> + dc->realize = pl061_realize; > >> + dc->user_creatable = true; > >> +#endif > >> dc->vmsd = &vmstate_pl061; > >> dc->reset = &pl061_reset; > >> } > >> diff --git a/qemu-options.hx b/qemu-options.hx > >> index 292d4e7c0cef6097..182de7fb63923b38 100644 > >> --- a/qemu-options.hx > >> +++ b/qemu-options.hx > >> @@ -875,6 +875,15 @@ SRST > >> ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]`` > >> Like the KCS interface, but defines a BT interface. The default port > >> is 0xe4 and the default interrupt is 5. > >> + > >> +#ifdef CONFIG_GPIODEV > >> +``-device pl061,host=gpiochip`` > >> + Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO > >> + controller on the host. > >> + > >> + ``host=gpiochip`` > >> + The name or label of the GPIO controller on the host. > >> +#endif > >> ERST > >> > >> DEF("name", HAS_ARG, QEMU_OPTION_name, > >> > > > > Instead of restricting this to the pl061, it would be cleaner you add a > > GPIO_PLUGGABLE_INTERFACE (or GPIO_BINDABLE_INTERFACE or better name), > > and have TYPE_PL061 implement it. > > IOW your backend should consume devices implementing this generic interface. Thanks for the suggestion! I had a look into implementing this. Please pardon my QEMU illiteracy, but I fail to see how adding an interface, and letting frontends like pl061.c implement it, will help: - The backend never has to call directly into the frontend: all communication is done indirectly, using existing core qemu irq and qdev gpio calls. - The frontend has to call into the backend once, at initialization time, to pass the host= parameter, and the number of GPIOs supported by the frontend (through qemu_gpiodev_add()). - Generalizing host= parsing in the backend would be nice, to avoid duplicating this in each and every frontend, but AFAIU, an interface cannot use device_class_set_props(), as InterfaceClass is not derived from DeviceClass. Note that when adding more features later (input support, and e.g. pull-up/down support), the frontend will have to call into the backend to change GPIO line configuration at runtime, but this is from frontend to backend again, not the other way around. I do agree that if we ever want to support multiple backends, implementing that through an interface (for the backend, not for the frontend) would be needed. What am I missing? Thanks again! Gr{oetje,eeting}s, Geert
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c index 74ba733a8a5e8ca5..98204f9a586ae8c8 100644 --- a/hw/gpio/pl061.c +++ b/hw/gpio/pl061.c @@ -12,11 +12,14 @@ #include "hw/arm/fdt.h" #include "hw/gpio/pl061.h" #include "hw/irq.h" +#include "hw/qdev-properties.h" #include "hw/sysbus.h" #include "migration/vmstate.h" +#include "qapi/error.h" #include "qemu/log.h" #include "qemu/module.h" #include "sysemu/device_tree.h" +#include "sysemu/gpiodev.h" //#define DEBUG_PL061 1 @@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] = typedef struct PL061State { SysBusDevice parent_obj; +#ifdef CONFIG_GPIODEV + char *host; +#endif MemoryRegion iomem; uint32_t locked; uint32_t data; @@ -370,10 +376,39 @@ static void pl061_init(Object *obj) qdev_init_gpio_out(dev, s->out, 8); } +#ifdef CONFIG_GPIODEV +static Property pl061_properties[] = { + DEFINE_PROP_STRING("host", PL061State, host), + DEFINE_PROP_END_OF_LIST(), +}; + +static void pl061_realize(DeviceState *dev, Error **errp) +{ + PL061State *s = PL061(dev); + + if (!dev->opts) { + /* Not created by user */ + return; + } + + if (!s->host) { + error_setg(errp, "'host' property is required"); + return; + } + + qemu_gpiodev_add(dev, s->host, 8, errp); +} +#endif /* CONFIG_GPIODEV */ + static void pl061_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); +#ifdef CONFIG_GPIODEV + device_class_set_props(dc, pl061_properties); + dc->realize = pl061_realize; + dc->user_creatable = true; +#endif dc->vmsd = &vmstate_pl061; dc->reset = &pl061_reset; } diff --git a/qemu-options.hx b/qemu-options.hx index 292d4e7c0cef6097..182de7fb63923b38 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -875,6 +875,15 @@ SRST ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]`` Like the KCS interface, but defines a BT interface. The default port is 0xe4 and the default interrupt is 5. + +#ifdef CONFIG_GPIODEV +``-device pl061,host=gpiochip`` + Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO + controller on the host. + + ``host=gpiochip`` + The name or label of the GPIO controller on the host. +#endif ERST DEF("name", HAS_ARG, QEMU_OPTION_name,
Make the PL061 GPIO controller user-creatable, and allow the user to tie a newly created instance to a gpiochip on the host. To create a new GPIO controller, the QEMU command line must be augmented with: -device pl061,host=<gpiochip> with <gpiochip> the name or label of the gpiochip on the host. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v2: - New. --- hw/gpio/pl061.c | 35 +++++++++++++++++++++++++++++++++++ qemu-options.hx | 9 +++++++++ 2 files changed, 44 insertions(+)