Re: [PATCH QEMU v2 4/5] ARM: PL061: Add gpiodev support

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Philippe,

On Thu, Apr 23, 2020 at 12:08 PM Philippe Mathieu-Daudé <f4bug@xxxxxxxxx> 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@xxxxxxxxx>

> >> --- 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds