Re: [PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support
Hi Sato-san,
On Thu, Oct 19, 2023 at 4:03 AM Yoshinori Sato
wrote:
> On Wed, 18 Oct 2023 21:40:23 +0900,
> Geert Uytterhoeven wrote:
> > The new Linux SH7750 clock driver uses the registers for power-down
> > mode control, causing a crash:
> >
> > byte read to SH7750_STBCR_A7 (0x1fc4) not supported
> > Aborted (core dumped)
> >
> > Fix this by adding support for the Standby Control Registers STBCR and
> > STBCR2.
>
> FRQCR is also not returning the correct value, so it needs to be fixed.
I knew there would be more, hence the RFC ;-)
> Here are my changes.
> https://gitlab.com/yoshinori.sato/qemu.git
>
> It include.
> - Minimal CPG support.
> - DT support
> - Add target LANDISK.
Thank you very much!
It would be a good idea to mention this is the cover letter of your
Linux patch series, so your test audience doesn't have to fix
already-solved problems...
BTW, your commit da64d6541226a516 ("hw/sh4: sh7750.c allow access
STBCR and STBCR2.") just ignores writes, and always returns zero
when reading. This may cause issues with Linux code relying on
clock_ops.is_enabled() to return correct data.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support
Hi Adrian,
On Wed, Oct 18, 2023 at 2:46 PM John Paul Adrian Glaubitz
wrote:
> On Wed, 2023-10-18 at 14:40 +0200, Geert Uytterhoeven wrote:
> > The new Linux SH7750 clock driver uses the registers for power-down
> > mode control, causing a crash:
> >
> > byte read to SH7750_STBCR_A7 (0x1fc4) not supported
> > Aborted (core dumped)
> >
> > Fix this by adding support for the Standby Control Registers STBCR and
> > STBCR2.
> >
> > Signed-off-by: Geert Uytterhoeven
>
> Is this supposed to be applied on top of Yoshinori's DT conversion series?
No, it's a patch for QEMU. Sorry for the confusion.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support
The new Linux SH7750 clock driver uses the registers for power-down
mode control, causing a crash:
byte read to SH7750_STBCR_A7 (0x1fc4) not supported
Aborted (core dumped)
Fix this by adding support for the Standby Control Registers STBCR and
STBCR2.
Signed-off-by: Geert Uytterhoeven
---
[RFC PATCH v3 12/35] drivers/clk/renesas: clk-sh7750.c SH7750/7751 CPG driver.
https://lore.kernel.org/all/a772e1b6de89af22057d3af31cc03dcad7964fc7.1697199949.git.ys...@users.sourceforge.jp
Accesses to CLKSTP00 and CLKSTCLK00 (0xfe0a/0x1e0a and
0xfe0a0008/0x1e0a0008) don't seem to cause any issues, although I can't
see immediately where they are handled?
---
hw/sh4/sh7750.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
index ebe0fd96d94ca17b..deeb83b4540bbf2b 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -59,6 +59,9 @@ typedef struct SH7750State {
uint16_t bcr3;
uint32_t bcr4;
uint16_t rfcr;
+/* Power-Down Modes */
+uint8_t stbcr;
+uint8_t stbcr2;
/* PCMCIA controller */
uint16_t pcr;
/* IO ports */
@@ -219,7 +222,13 @@ static void ignore_access(const char *kind, hwaddr addr)
static uint32_t sh7750_mem_readb(void *opaque, hwaddr addr)
{
+SH7750State *s = opaque;
+
switch (addr) {
+case SH7750_STBCR_A7:
+return s->stbcr;
+case SH7750_STBCR2_A7:
+return s->stbcr2;
default:
error_access("byte read", addr);
abort();
@@ -318,14 +327,24 @@ static uint32_t sh7750_mem_readl(void *opaque, hwaddr
addr)
static void sh7750_mem_writeb(void *opaque, hwaddr addr,
uint32_t mem_value)
{
+SH7750State *s = opaque;
if (is_in_sdrmx(addr, 2) || is_in_sdrmx(addr, 3)) {
ignore_access("byte write", addr);
return;
}
-error_access("byte write", addr);
-abort();
+switch (addr) {
+case SH7750_STBCR_A7:
+s->stbcr = mem_value;
+return;
+case SH7750_STBCR2_A7:
+s->stbcr2 = mem_value;
+return;
+default:
+error_access("byte write", addr);
+abort();
+}
}
static void sh7750_mem_writew(void *opaque, hwaddr addr,
--
2.34.1
Re: [PATCH v2] m68k: virt: pass RNG seed via bootinfo block
On Sun, Jun 26, 2022 at 1:18 PM Jason A. Donenfeld wrote:
> This commit wires up bootinfo's RNG seed attribute so that Linux VMs can
> have their RNG seeded from the earliest possible time in boot, just like
> the "rng-seed" device tree property on those platforms. The link
> contains the corresponding Linux patch.
>
> Link: https://lore.kernel.org/lkml/20220626111509.330159-1-ja...@zx2c4.com/
> Based-on: <20220625152318.120849-1-ja...@zx2c4.com>
> Reviewed-by: Laurent Vivier
> Signed-off-by: Jason A. Donenfeld
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine
CC arnd On Sun, Jun 5, 2022 at 9:32 AM Stafford Horne wrote: > On Sun, Jun 05, 2022 at 10:58:14AM +0900, Stafford Horne wrote: > > On Fri, Jun 03, 2022 at 09:05:09AM +0200, Geert Uytterhoeven wrote: > > > On Thu, Jun 2, 2022 at 9:59 PM Stafford Horne wrote: > > > > On Thu, Jun 02, 2022 at 09:08:52PM +0200, Geert Uytterhoeven wrote: > > > > > On Thu, Jun 2, 2022 at 1:42 PM Joel Stanley wrote: > > > > > > On Fri, 27 May 2022 at 17:27, Stafford Horne > > > > > > wrote: > > > > > > > This patch add the OpenRISC virtual machine 'virt' for OpenRISC. > > > > > > > This > > > > > > > platform allows for a convenient CI platform for toolchain, > > > > > > > software > > > > > > > ports and the OpenRISC linux kernel port. > > > > > > > > > > > > > > Much of this has been sourced from the m68k and riscv virt > > > > > > > platforms. > > > > > > > > > > > I enabled the options: > > > > > > > > > > > > CONFIG_RTC_CLASS=y > > > > > > # CONFIG_RTC_SYSTOHC is not set > > > > > > # CONFIG_RTC_NVMEM is not set > > > > > > CONFIG_RTC_DRV_GOLDFISH=y > > > > > > > > > > > > But it didn't work. It seems the goldfish rtc model doesn't handle a > > > > > > big endian guest running on my little endian host. > > > > > > > > > > > > Doing this fixes it: > > > > > > > > > > > > -.endianness = DEVICE_NATIVE_ENDIAN, > > > > > > +.endianness = DEVICE_HOST_ENDIAN, > > > > > > > > > > > > [0.19] goldfish_rtc 96005000.rtc: registered as rtc0 > > > > > > [0.19] goldfish_rtc 96005000.rtc: setting system clock to > > > > > > 2022-06-02T11:16:04 UTC (1654168564) > > > > > > > > > > > > But literally no other model in the tree does this, so I suspect > > > > > > it's > > > > > > not the right fix. > > > > > > > > > > Goldfish devices are supposed to be little endian. > > > > > Unfortunately m68k got this wrong, cfr. > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e2ac4a3327479f7e2744cdd88a5c823f2057bad > > > > > Please don't duplicate this bad behavior for new architectures > > > > > > > > Thanks for the pointer, I just wired in the goldfish RTC because I > > > > wanted to > > > > play with it. I was not attached to it. I can either remove it our > > > > find another > > > > RTC. > > > > > > Sorry for being too unclear: the mistake was not to use the Goldfish > > > RTC, but to make its register accesses big-endian. > > > Using Goldfish devices as little-endian devices should be fine. > > > > OK, then I would think this patch would be needed on Goldfish. I tested > > this > > out and it seems to work: > > Sorry, it seems maybe I mis-understood this again. In Arnd's mail [1] he, at > the end, mentions. > > It might be a good idea to revisit the qemu implementation and make > sure that the extra byteswap is only inserted on m68k and not on > other targets, but hopefully there are no new targets based on goldfish > anymore and we don't need to care. > > So, it seems that in addition to my patch we would need something in m68k to > switch it back to 'native' (big) endian? > > Looking at the m68k kernel/qemu interface I see: > > Pre 5.19: >(data) <-- kernel(readl / little) <-- m68k qemu (native / big) - RTC/PIC >(data) <-- kernel(__raw_readl / big) <-- m68k qemu (native / big) - TTY > > 5.19: >(data) <-- kernel(gf_ioread32 / big) <-- m68k qemu (native / big) - all > > The new fixes to add gf_ioread32/gf_iowrite32 fix this for goldfish and m68k. > This wouldn't have been an issue for little-endian platforms where > readl/writel > were originally used. > > Why can't m68k switch to little-endian in qemu and the kernel? The m68k virt > platform is not that old, 1 year? Are there a lot of users that this would be > a big > problem? > > [1] > https://lore.kernel.org/lkml/CAK8P3a1oN8NrUjkh2X8jHQbyz42Xo6GSa=5n0gd6vqcxrjm...@mail.gmail.com/ > > -Stafford > > > Patch: > > > > diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c > &
Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine
Hi Stafford,
On Thu, Jun 2, 2022 at 9:59 PM Stafford Horne wrote:
> On Thu, Jun 02, 2022 at 09:08:52PM +0200, Geert Uytterhoeven wrote:
> > On Thu, Jun 2, 2022 at 1:42 PM Joel Stanley wrote:
> > > On Fri, 27 May 2022 at 17:27, Stafford Horne wrote:
> > > > This patch add the OpenRISC virtual machine 'virt' for OpenRISC. This
> > > > platform allows for a convenient CI platform for toolchain, software
> > > > ports and the OpenRISC linux kernel port.
> > > >
> > > > Much of this has been sourced from the m68k and riscv virt platforms.
> >
> > > I enabled the options:
> > >
> > > CONFIG_RTC_CLASS=y
> > > # CONFIG_RTC_SYSTOHC is not set
> > > # CONFIG_RTC_NVMEM is not set
> > > CONFIG_RTC_DRV_GOLDFISH=y
> > >
> > > But it didn't work. It seems the goldfish rtc model doesn't handle a
> > > big endian guest running on my little endian host.
> > >
> > > Doing this fixes it:
> > >
> > > -.endianness = DEVICE_NATIVE_ENDIAN,
> > > +.endianness = DEVICE_HOST_ENDIAN,
> > >
> > > [0.19] goldfish_rtc 96005000.rtc: registered as rtc0
> > > [0.19] goldfish_rtc 96005000.rtc: setting system clock to
> > > 2022-06-02T11:16:04 UTC (1654168564)
> > >
> > > But literally no other model in the tree does this, so I suspect it's
> > > not the right fix.
> >
> > Goldfish devices are supposed to be little endian.
> > Unfortunately m68k got this wrong, cfr.
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e2ac4a3327479f7e2744cdd88a5c823f2057bad
> > Please don't duplicate this bad behavior for new architectures
>
> Thanks for the pointer, I just wired in the goldfish RTC because I wanted to
> play with it. I was not attached to it. I can either remove it our find
> another
> RTC.
Sorry for being too unclear: the mistake was not to use the Goldfish
RTC, but to make its register accesses big-endian.
Using Goldfish devices as little-endian devices should be fine.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine
Hi Joel,
On Thu, Jun 2, 2022 at 1:42 PM Joel Stanley wrote:
> On Fri, 27 May 2022 at 17:27, Stafford Horne wrote:
> > This patch add the OpenRISC virtual machine 'virt' for OpenRISC. This
> > platform allows for a convenient CI platform for toolchain, software
> > ports and the OpenRISC linux kernel port.
> >
> > Much of this has been sourced from the m68k and riscv virt platforms.
> I enabled the options:
>
> CONFIG_RTC_CLASS=y
> # CONFIG_RTC_SYSTOHC is not set
> # CONFIG_RTC_NVMEM is not set
> CONFIG_RTC_DRV_GOLDFISH=y
>
> But it didn't work. It seems the goldfish rtc model doesn't handle a
> big endian guest running on my little endian host.
>
> Doing this fixes it:
>
> -.endianness = DEVICE_NATIVE_ENDIAN,
> +.endianness = DEVICE_HOST_ENDIAN,
>
> [0.19] goldfish_rtc 96005000.rtc: registered as rtc0
> [0.19] goldfish_rtc 96005000.rtc: setting system clock to
> 2022-06-02T11:16:04 UTC (1654168564)
>
> But literally no other model in the tree does this, so I suspect it's
> not the right fix.
Goldfish devices are supposed to be little endian.
Unfortunately m68k got this wrong, cfr.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e2ac4a3327479f7e2744cdd88a5c823f2057bad
Please don't duplicate this bad behavior for new architectures.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH QEMU v2 4/5] ARM: PL061: Add gpiodev support
Hi Philippe,
On Thu, Apr 23, 2020 at 12:08 PM Philippe Mathieu-Daudé 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=
> >>
> >> with the name or label of the gpiochip on the host.
> >>
> >> Signed-off-by: Geert Uytterhoeven
> >> --- 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 = _pl061;
> >> dc->reset = _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
Re: [PATCH] hw/arm/virt: Fix PL061 node name and properties
Hi Peter,
CC Rob, who made these properties mandatory in the pl061 DT bindings.
On Fri, May 22, 2020 at 11:30 AM Peter Maydell wrote:
> On Fri, 22 May 2020 at 09:29, Geert Uytterhoeven wrote:
> > On Thu, May 21, 2020 at 6:59 PM Peter Maydell
> > wrote:
> > > On Tue, 19 May 2020 at 09:49, Geert Uytterhoeven
> > > wrote:
> > > > Make the created node comply with the PL061 Device Tree bindings:
> > > > - Use generic node name "gpio" instead of "pl061",
> > > > - Add missing "#interrupt-cells" and "interrupt-controller"
> > > > properties.
> > >
> > > Where have these properties come from? They must be optional,
> > > because in the version of the binding documentation from Linux
> > > 5.0 they're not described:
> > > https://elixir.bootlin.com/linux/v5.0/source/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
> >
> > Many old DT bindings are incomplete.
>
> Yeah, but production QEMU is out there in the world based on
> the old DT binding documentation. So you can't unilaterally
> make a part of the binding that wasn't documented and that QEMU
> didn't emit mandatory. It might be preferable for new QEMU to
> emit it, of course.
Indeed, else QEMU will differ (more) from real hardware. Most PL061
GPIO controllers in DTS files in upstream Linux are marked as interrupt
controllers (a few haven't been updated yet).
> > When running the validation on a device tree passed to the guest
> > (extracted from /sys/firmware/devicetree/base, converted to dts, and
> > manually fixed up the phandles), the following is reported about the
> > pl061 node:
> >
> > virt.dt.yaml: pl061@903: {'reg': [[0, 151191552, 0, 4096]],
> > 'gpio-controller': True, 'phandle': [[32771]], '#gpio-cells': [[2]],
> > 'clocks': [[32768]], '#interrupt-cells': [[2]], 'compatible':
> > ['arm,pl061', 'arm,primecell'], 'clock-names': ['apb_pclk'],
> > '$nodename': ['pl061@903']} is not valid under any of the given
> > schemas
> > [...]
> > virt.dt.yaml: pl061@903: 'interrupts' is a required property
> >
> > virt.dt.yaml: pl061@903: $nodename:0: 'pl061@903' does not
> > match '^gpio@[0-9a-f]+$'
> > virt.dt.yaml: pl061@903: 'interrupt-controller' is a required
> > property
>
> This is just saying "the yaml says these things are mandatory".
> You could equally get rid of them by marking them optional in
> the yaml, right?
>
> Also, complaining about the nodename seems like a bug in the
> validation: it is not a mandatory part of the spec, just a
> recommendation.
I'll defer that one to Rob, too.
> > > Since the devicetree spec says that the interrupt-controller
> > > property "defines a node as an interrupt controller node"
> > > and a GPIO chip isn't an interrupt controller, this seems
> > > like some kind of error in the dtb binding. Maybe I'm
> > > missing something...
> >
> > PL061 is an interrupt controller, as it can assert its interrupt output
> > based on activity on GPIO input lines.
>
> By that logic the PL011 UART is an interrupt controller, because
> it can assert its interrupt output based on activity on the serial
> port input lines.
Doh, bad wording on my side :-)
> A GPIO controller is not an interrupt controller inherently.
> Maybe you can use it in a system design as an interrupt
> controller if you want to, and in that system's dtb perhaps
> it would make sense to label it as one, but the virt board's
> PL061 is in no way an interrupt controller -- it's just a GPIO
> controller.
Actually it is: the virt board has a gpio-keys node that ties the
Poweroff key to a PL061 GPIO. The Linux gpio-keys driver relies on
interrupts to detect key events (for IRQ incapable GPIOs you have to use
"gpio-keys-polled" instead). This can easily be verified by looking at
/proc/interrupts on the guest:
48: 0 903.gpio 3 Edge GPIO Key Poweroff
(Unfortunately I don't know how to trigger a virtual key press in the
guest)
> > > What actually goes wrong if QEMU doesn't specify these
> > > properties?
> >
> > It means that other devices that have their interrupt output connected
> > to a PL061 GPIO input won't work, as their driver in the guest OS cannot
> > find the interrupt. Note that arm/virt.c currently doesn't instantiate
> > such devices.
Seems I was wrong: arm/virt does have the Poweroff key, and Linux does
find the interrupt.
> OK. But why would we want to run an interrupt line through the GPIO
> controller when we have a perfectly good interrupt cont
Re: [PATCH] hw/arm/virt: Fix PL061 node name and properties
Hi Peter,
On Thu, May 21, 2020 at 6:59 PM Peter Maydell wrote:
> On Tue, 19 May 2020 at 09:49, Geert Uytterhoeven
> wrote:
> > Make the created node comply with the PL061 Device Tree bindings:
> > - Use generic node name "gpio" instead of "pl061",
> > - Add missing "#interrupt-cells" and "interrupt-controller"
> > properties.
>
> Where have these properties come from? They must be optional,
> because in the version of the binding documentation from Linux
> 5.0 they're not described:
> https://elixir.bootlin.com/linux/v5.0/source/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
Many old DT bindings are incomplete.
> They seem to have magically appeared in kernel commit
> 910f38bed9439e765f7e, which purports to only be a change
> of format from plain text to yaml but has added some
> extra properties and claimed them to be mandatory.
The main motivation behind the conversion from plain text to yaml is to
do automatic validation of DTS files, based on the bindings. During the
conversion process, many issues are detected, and fixed; not only in the
DTS files, but also in the bindings (e.g. missing properties, like in
this case).
When running the validation on a device tree passed to the guest
(extracted from /sys/firmware/devicetree/base, converted to dts, and
manually fixed up the phandles), the following is reported about the
pl061 node:
virt.dt.yaml: pl061@903: {'reg': [[0, 151191552, 0, 4096]],
'gpio-controller': True, 'phandle': [[32771]], '#gpio-cells': [[2]],
'clocks': [[32768]], '#interrupt-cells': [[2]], 'compatible':
['arm,pl061', 'arm,primecell'], 'clock-names': ['apb_pclk'],
'$nodename': ['pl061@903']} is not valid under any of the given
schemas
[...]
virt.dt.yaml: pl061@903: 'interrupts' is a required property
virt.dt.yaml: pl061@903: $nodename:0: 'pl061@903' does not
match '^gpio@[0-9a-f]+$'
virt.dt.yaml: pl061@903: 'interrupt-controller' is a required property
> Since the devicetree spec says that the interrupt-controller
> property "defines a node as an interrupt controller node"
> and a GPIO chip isn't an interrupt controller, this seems
> like some kind of error in the dtb binding. Maybe I'm
> missing something...
PL061 is an interrupt controller, as it can assert its interrupt output
based on activity on GPIO input lines.
> What actually goes wrong if QEMU doesn't specify these
> properties?
It means that other devices that have their interrupt output connected
to a PL061 GPIO input won't work, as their driver in the guest OS cannot
find the interrupt. Note that arm/virt.c currently doesn't instantiate
such devices.
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 7dc96abf72cf2b9a..99593d7bce4d85cb 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -818,13 +818,15 @@ static void create_gpio(const VirtMachineState *vms)
> > qdev_get_gpio_in(vms->gic, irq));
> >
> > uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
> > -nodename = g_strdup_printf("/pl061@%" PRIx64, base);
> > +nodename = g_strdup_printf("/gpio@%" PRIx64, base);
>
> Does the devicetree binding really mandate what the node name is?
> I thought that finding the right device was doe via the
> 'compatible' string and the nodename could be whatever the
> device tree creator wanted.
Matching is indeed done based on compatible value.
For node names, please see
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
"2.2.2 Generic Names Recommendation
The name of a node should be somewhat generic, reflecting the
function of the device and not its precise programming model. If
appropriate, the name should be one of the following choices:
[...]
- gpio
[...]"
While many new generic names have been added recently, "gpio" was
already documented in the Power.orgTM Standard for Embedded Power
ArchitectureTM Platform Requirements (ePAPR).
I hope the above explains the rationale behind these change better.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator
Hi Andy,
On Wed, May 20, 2020 at 2:41 PM Andy Shevchenko
wrote:
> On Wed, May 20, 2020 at 3:40 PM Andy Shevchenko
> wrote:
> > On Wed, May 20, 2020 at 3:38 PM Geert Uytterhoeven
> > wrote:
> > > On Wed, May 20, 2020 at 2:14 PM Andy Shevchenko
> > > wrote:
> > > > On Mon, May 11, 2020 at 04:52:51PM +0200, Geert Uytterhoeven wrote:
> >
> > ...
> >
> > > > Sorry for late reply, recently noticed this nice idea.
> > > > The comment I have is, please, can we reuse bitmap parse algorithm and
> > > > syntax?
> > > > We have too many different formats and parsers in the kernel and
> > > > bitmap's one
> > > > seems suitable here.
> > >
> > > Thank you, I wasn't aware of that.
> > >
> > > Which one do you mean? The documentation seems to be confusing,
> > > and incomplete.
> > > My first guess was bitmap_parse(), but that one assumes hex values?
> > > And given it processes the unsigned long bitmap in u32 chunks, I guess
> > > it doesn't work as expected on big-endian 64-bit?
> > >
> > > bitmap_parselist() looks more suitable, and the format seems to be
>
> > > compatible with what's currently used, so it won't change ABI.
>
> What ABI? We didn't have a release with it, right? So, we are quite
ABI = the parameters it accepts currently.
> flexible for few more weeks to amend it.
Indeed, we have time to make changes until the release of v5.8.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator
Hi Andy,
On Wed, May 20, 2020 at 2:14 PM Andy Shevchenko
wrote:
> On Mon, May 11, 2020 at 04:52:51PM +0200, Geert Uytterhoeven wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence this adds a GPIO driver to aggregate existing GPIOs, and expose
> > them as a new gpiochip. This is useful for implementing access control,
> > and assigning a set of GPIOs to a specific user. Furthermore, this
> > simplifies and hardens exporting GPIOs to a virtual machine, as the VM
> > can just grab the full GPIO controller, and no longer needs to care
> > about which GPIOs to grab and which not, reducing the attack surface.
> > This has been implemented for ARM virt in QEMU[1].
> >
> > Recently, other use cases have been discovered[2], like describing
> > simple GPIO-operated devices in DT, and using the GPIO Aggregator as a
> > generic GPIO driver for userspace, which is useful for industrial
> > control.
> >
> > Note that the first patch of this series ("i2c: i801: Use GPIO_LOOKUP()
> > helper macro") has been applied to i2c/for-next.
>
> Sorry for late reply, recently noticed this nice idea.
> The comment I have is, please, can we reuse bitmap parse algorithm and syntax?
> We have too many different formats and parsers in the kernel and bitmap's one
> seems suitable here.
Thank you, I wasn't aware of that.
Which one do you mean? The documentation seems to be confusing,
and incomplete.
My first guess was bitmap_parse(), but that one assumes hex values?
And given it processes the unsigned long bitmap in u32 chunks, I guess
it doesn't work as expected on big-endian 64-bit?
bitmap_parselist() looks more suitable, and the format seems to be
compatible with what's currently used, so it won't change ABI.
Is that the one you propose?
> (Despite other small clean ups, like strstrip() use)
Aka strim()? There are too many of them, to know all of them by heart ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH] ARM: PL061: Introduce N_GPIOS
Add a definition for the number of GPIO lines controlled by a PL061
instance, and use it instead of the hardcoded magic value 8.
Suggested-by: Philippe Mathieu-Daudé
Signed-off-by: Geert Uytterhoeven
---
hw/gpio/pl061.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 2a828260bdb0b946..6d3c36bc16cf9e0d 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -36,6 +36,8 @@ static const uint8_t pl061_id_luminary[12] =
#define TYPE_PL061 "pl061"
#define PL061(obj) OBJECT_CHECK(PL061State, (obj), TYPE_PL061)
+#define N_GPIOS 8
+
typedef struct PL061State {
SysBusDevice parent_obj;
@@ -62,7 +64,7 @@ typedef struct PL061State {
uint32_t cr;
uint32_t amsel;
qemu_irq irq;
-qemu_irq out[8];
+qemu_irq out[N_GPIOS];
const unsigned char *id;
uint32_t rsvd_start; /* reserved area: [rsvd_start, 0xfcc] */
} PL061State;
@@ -112,7 +114,7 @@ static void pl061_update(PL061State *s)
changed = s->old_out_data ^ out;
if (changed) {
s->old_out_data = out;
-for (i = 0; i < 8; i++) {
+for (i = 0; i < N_GPIOS; i++) {
mask = 1 << i;
if (changed & mask) {
DPRINTF("Set output %d = %d\n", i, (out & mask) != 0);
@@ -125,7 +127,7 @@ static void pl061_update(PL061State *s)
changed = (s->old_in_data ^ s->data) & ~s->dir;
if (changed) {
s->old_in_data = s->data;
-for (i = 0; i < 8; i++) {
+for (i = 0; i < N_GPIOS; i++) {
mask = 1 << i;
if (changed & mask) {
DPRINTF("Changed input %d = %d\n", i, (s->data & mask) != 0);
@@ -364,8 +366,8 @@ static void pl061_init(Object *obj)
memory_region_init_io(>iomem, obj, _ops, s, "pl061", 0x1000);
sysbus_init_mmio(sbd, >iomem);
sysbus_init_irq(sbd, >irq);
-qdev_init_gpio_in(dev, pl061_set_irq, 8);
-qdev_init_gpio_out(dev, s->out, 8);
+qdev_init_gpio_in(dev, pl061_set_irq, N_GPIOS);
+qdev_init_gpio_out(dev, s->out, N_GPIOS);
}
static void pl061_class_init(ObjectClass *klass, void *data)
--
2.17.1
[PATCH] hw/arm/virt: Fix PL061 node name and properties
Make the created node comply with the PL061 Device Tree bindings:
- Use generic node name "gpio" instead of "pl061",
- Add missing "#interrupt-cells" and "interrupt-controller"
properties.
Signed-off-by: Geert Uytterhoeven
---
Split off from "[PATCH QEMU v2 2/5] ARM: PL061: Extract pl061_create_fdt()"
(https://lore.kernel.org/r/20200423090118.11199-3-geert+rene...@glider.be).
---
hw/arm/virt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7dc96abf72cf2b9a..99593d7bce4d85cb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -818,13 +818,15 @@ static void create_gpio(const VirtMachineState *vms)
qdev_get_gpio_in(vms->gic, irq));
uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
-nodename = g_strdup_printf("/pl061@%" PRIx64, base);
+nodename = g_strdup_printf("/gpio@%" PRIx64, base);
qemu_fdt_add_subnode(vms->fdt, nodename);
qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
2, base, 2, size);
qemu_fdt_setprop(vms->fdt, nodename, "compatible", compat, sizeof(compat));
qemu_fdt_setprop_cell(vms->fdt, nodename, "#gpio-cells", 2);
qemu_fdt_setprop(vms->fdt, nodename, "gpio-controller", NULL, 0);
+qemu_fdt_setprop_cell(vms->fdt, nodename, "#interrupt-cells", 2);
+qemu_fdt_setprop(vms->fdt, nodename, "interrupt-controller", NULL, 0);
qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
GIC_FDT_IRQ_TYPE_SPI, irq,
GIC_FDT_IRQ_FLAGS_LEVEL_HI);
--
2.17.1
Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator
Hi Linus,
On Mon, May 18, 2020 at 10:17 AM Linus Walleij wrote:
> I have queued this v7 patch set in an immutable branch for testing and also
> merged to my "devel" branch for testing.
>
> If all goes well it also hits linux-next soon.
Thank a lot!
Back to the QEMU side...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v7 0/6] gpio: Add GPIO Aggregator
ggregator.1: gpio 0 => gpio-955
gpio-aggregator gpio-aggregator.1: gpio 1 => gpio-1012
gpio-aggregator gpio-aggregator.1: gpio 2 => gpio-1013
gpio-aggregator gpio-aggregator.1: gpio 3 => gpio-1014
gpiochip_find_base: found new base at 754
gpio gpiochip13: (gpio-aggregator.1): added GPIO chardev (254:13)
gpiochip_setup_dev: registered GPIOs 754 to 757 on device: gpiochip13
(gpio-aggregator.1)
- Adjust permissions on /dev/gpiochip1[23] (optional)
- Control LEDs:
$ gpioset gpiochip12 0=0 1=1 # LED6 OFF, LED7 ON
$ gpioset gpiochip12 0=1 1=0 # LED6 ON, LED7 OFF
$ gpioset gpiochip13 0=1 # LED8 ON
$ gpioset gpiochip13 0=0 # LED8 OFF
- Destroy aggregators:
$ echo gpio-aggregator.0 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
$ echo gpio-aggregator.1 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
To ease testing, I have pushed this series to the
topic/gpio-aggregator-v7 branch of my renesas-drivers repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
Thanks!
References:
[1] "[PATCH QEMU v2 0/5] Add a GPIO backend"
(https://lore.kernel.org/linux-gpio/20200423090118.11199-1-geert+rene...@glider.be/)
[2] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
(https://lore.kernel.org/r/1561699236-18620-3-git-send-email-harish_kand...@mentor.com/)
[3] "[PATCH v6 0/8] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/linux-doc/20200324135328.5796-1-geert+rene...@glider.be/)
[4] "[PATCH v5 0/5] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/r/20200218151812.7816-1-geert+rene...@glider.be/)
[5] "[PATCH v4 0/5] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/r/20200115181523.23556-1-geert+rene...@glider.be)
[6] "[PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater"
(https://lore.kernel.org/r/20191127084253.16356-1-geert+rene...@glider.be/)
[7] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
(https://lore.kernel.org/r/20190911143858.13024-1-geert+rene...@glider.be/)
[8] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
(https://lore.kernel.org/r/20190705160536.12047-1-geert+rene...@glider.be/)
[9] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/r/20181003152521.23144-1-geert+rene...@glider.be/)
[10] "Getting To Blinky: Virt Edition / Making device pass-through
work on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
Geert Uytterhoeven (6):
i2c: i801: Use GPIO_LOOKUP() helper macro
mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
gpiolib: Add support for GPIO lookup by line name
gpio: Add GPIO Aggregator
docs: gpio: Add GPIO Aggregator documentation
MAINTAINERS: Add GPIO Aggregator section
.../admin-guide/gpio/gpio-aggregator.rst | 111
Documentation/admin-guide/gpio/index.rst | 1 +
Documentation/driver-api/gpio/board.rst | 15 +-
MAINTAINERS | 7 +
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-aggregator.c| 568 ++
drivers/gpio/gpiolib.c| 22 +-
drivers/i2c/busses/i2c-i801.c | 6 +-
drivers/mfd/sm501.c | 24 +-
include/linux/gpio/machine.h | 17 +-
11 files changed, 748 insertions(+), 36 deletions(-)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
create mode 100644 drivers/gpio/gpio-aggregator.c
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v7 5/6] docs: gpio: Add GPIO Aggregator documentation
Document the GPIO Aggregator, and the two typical use-cases.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
Reviewed-by: Eugeniu Rosca
Tested-by: Eugeniu Rosca
---
v7:
- No changes,
v6:
- Fix "allows" without object:
-> provides a mechanism to aggregate GPIOs,
-> provides access control for a set of one or more GPIOs,
-> allows the user to communicate,
- Drop "gpiochipN" support,
- Extend example,
v5:
- Add Reviewed-by, Tested-by,
- Fix inconsistent indentation,
v4:
- Add Reviewed-by,
- Drop controversial GPIO repeater,
- Clarify industrial control use case,
- Fix typo s/communicated/communicate/,
- Replace abstract frobnicator example by concrete door example with
gpio-line-names,
v3:
- New.
---
.../admin-guide/gpio/gpio-aggregator.rst | 111 ++
Documentation/admin-guide/gpio/index.rst | 1 +
2 files changed, 112 insertions(+)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst
b/Documentation/admin-guide/gpio/gpio-aggregator.rst
new file mode 100644
index ..5cd1e7221756504c
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -0,0 +1,111 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Aggregator
+===
+
+The GPIO Aggregator provides a mechanism to aggregate GPIOs, and expose them as
+a new gpio_chip. This supports the following use cases.
+
+
+Aggregating GPIOs using Sysfs
+-
+
+GPIO controllers are exported to userspace using /dev/gpiochip* character
+devices. Access control to these devices is provided by standard UNIX file
+system permissions, on an all-or-nothing basis: either a GPIO controller is
+accessible for a user, or it is not.
+
+The GPIO Aggregator provides access control for a set of one or more GPIOs, by
+aggregating them into a new gpio_chip, which can be assigned to a group or user
+using standard UNIX file ownership and permissions. Furthermore, this
+simplifies and hardens exporting GPIOs to a virtual machine, as the VM can just
+grab the full GPIO controller, and no longer needs to care about which GPIOs to
+grab and which not, reducing the attack surface.
+
+Aggregated GPIO controllers are instantiated and destroyed by writing to
+write-only attribute files in sysfs.
+
+/sys/bus/platform/drivers/gpio-aggregator/
+
+ "new_device" ...
+ Userspace may ask the kernel to instantiate an aggregated GPIO
+ controller by writing a string describing the GPIOs to
+ aggregate to the "new_device" file, using the format
+
+ .. code-block:: none
+
+ [] [ ] ...
+
+ Where:
+
+ "" ...
+ is a GPIO line name,
+
+ "" ...
+ is a GPIO chip label, and
+
+ "" ...
+ is a comma-separated list of GPIO offsets and/or
+ GPIO offset ranges denoted by dashes.
+
+ Example: Instantiate a new GPIO aggregator by aggregating GPIO
+ line 19 of "e6052000.gpio" and GPIO lines 20-21 of
+ "e605.gpio" into a new gpio_chip:
+
+ .. code-block:: sh
+
+ $ echo 'e6052000.gpio 19 e605.gpio 20-21' > new_device
+
+ "delete_device" ...
+ Userspace may ask the kernel to destroy an aggregated GPIO
+ controller after use by writing its device name to the
+ "delete_device" file.
+
+ Example: Destroy the previously-created aggregated GPIO
+ controller, assumed to be "gpio-aggregator.0":
+
+ .. code-block:: sh
+
+ $ echo gpio-aggregator.0 > delete_device
+
+
+Generic GPIO Driver
+---
+
+The GPIO Aggregator can also be used as a generic driver for a simple
+GPIO-operated device described in DT, without a dedicated in-kernel driver.
+This is useful in industrial control, and is not unlike e.g. spidev, which
+allows the user to communicate with an SPI device from userspace.
+
+Binding a device to the GPIO Aggregator is performed either by modifying the
+gpio-aggregator driver, or by writing to the "driver_override" file in Sysfs.
+
+Example: If "door" is a GPIO-operated device described in DT, using its own
+compatible value::
+
+ door {
+ compatible = "myvendor,mydoor";
+
+ gpios = < 19 GPIO_ACTIVE_HIGH>,
+ < 20 GPIO_ACTIVE_LOW>;
+ gpio-line-names = "open", "lock";
+ };
+
+it can be bound to the GPIO Aggregator by either:
+
+1. Adding its compatible value
[PATCH v7 3/6] gpiolib: Add support for GPIO lookup by line name
Currently a GPIO lookup table can only refer to a specific GPIO by a
tuple, consisting of a GPIO controller label and a GPIO offset inside
the controller.
However, a GPIO may also carry a line name, defined by DT or ACPI.
If present, the line name is the most use-centric way to refer to a
GPIO. Hence add support for looking up GPIOs by line name.
Note that there is no guarantee that GPIO line names are globally
unique, so this will use the first match found.
Implement this by reusing the existing gpiod_lookup infrastructure.
Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
that this field can have two meanings, and update the kerneldoc and
GPIO_LOOKUP*() macros.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
Reviewed-by: Eugeniu Rosca
Tested-by: Eugeniu Rosca
---
v7:
- Document non-uniqueness of line names,
- Rebase on top of commit a0b66a73785ccc8f ("gpio: Rename variable in
core APIs"),
v6:
- Update Documentation/driver-api/gpio/board.rst,
- Reword rationale,
v5:
- Add Reviewed-by, Tested-by,
v4:
- Add Reviewed-by,
- Rename gpiod_lookup.chip_label.
- Use U16_MAX instead of (u16)-1,
v3:
- New.
---
Documentation/driver-api/gpio/board.rst | 15 ++-
drivers/gpio/gpiolib.c | 22 +-
include/linux/gpio/machine.h| 17 ++---
3 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/Documentation/driver-api/gpio/board.rst
b/Documentation/driver-api/gpio/board.rst
index ce91518bf9f48ded..191fa867826a45c3 100644
--- a/Documentation/driver-api/gpio/board.rst
+++ b/Documentation/driver-api/gpio/board.rst
@@ -113,13 +113,15 @@ files that desire to do so need to include the following
header::
GPIOs are mapped by the means of tables of lookups, containing instances of the
gpiod_lookup structure. Two macros are defined to help declaring such
mappings::
- GPIO_LOOKUP(chip_label, chip_hwnum, con_id, flags)
- GPIO_LOOKUP_IDX(chip_label, chip_hwnum, con_id, idx, flags)
+ GPIO_LOOKUP(key, chip_hwnum, con_id, flags)
+ GPIO_LOOKUP_IDX(key, chip_hwnum, con_id, idx, flags)
where
- - chip_label is the label of the gpiod_chip instance providing the GPIO
- - chip_hwnum is the hardware number of the GPIO within the chip
+ - key is either the label of the gpiod_chip instance providing the GPIO, or
+the GPIO line name
+ - chip_hwnum is the hardware number of the GPIO within the chip, or U16_MAX
+to indicate that key is a GPIO line name
- con_id is the name of the GPIO function from the device point of view. It
can be NULL, in which case it will match any function.
- idx is the index of the GPIO within the function.
@@ -135,7 +137,10 @@ where
In the future, these flags might be extended to support more properties.
-Note that GPIO_LOOKUP() is just a shortcut to GPIO_LOOKUP_IDX() where idx = 0.
+Note that:
+ 1. GPIO line names are not guaranteed to be globally unique, so the first
+ match found will be used.
+ 2. GPIO_LOOKUP() is just a shortcut to GPIO_LOOKUP_IDX() where idx = 0.
A lookup table can then be defined as follows, with an empty entry defining its
end. The 'dev_id' field of the table is the identifier of the device that will
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 98822412f565073a..2e4aacbad8fe586b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4645,7 +4645,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (!table)
return desc;
- for (p = >table[0]; p->chip_label; p++) {
+ for (p = >table[0]; p->key; p++) {
struct gpio_chip *gc;
/* idx must always match exactly */
@@ -4656,18 +4656,30 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
continue;
- gc = find_chip_by_name(p->chip_label);
+ if (p->chip_hwnum == U16_MAX) {
+ desc = gpio_name_to_desc(p->key);
+ if (desc) {
+ *flags = p->flags;
+ return desc;
+ }
+
+ dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+p->key);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ gc = find_chip_by_name(p->key);
if (!gc) {
/*
* As the lookup table indicates a chip with
-* p->chip_label should exist, assume it may
+* p->key should exist, assume it may
* still appear later and let the interested
* consume
[PATCH v7 2/6] mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
i801_add_mux() fills in the GPIO lookup table by manually populating an array of gpiod_lookup structures. Use the existing GPIO_LOOKUP_IDX() helper macro instead, to relax a dependency on the gpiod_lookup structure's member names. Signed-off-by: Geert Uytterhoeven Acked-by: Lee Jones --- v7: - Add Acked-by, v6: - New. --- drivers/mfd/sm501.c | 24 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c index e49787e6bb93e5c8..ccd62b963952814e 100644 --- a/drivers/mfd/sm501.c +++ b/drivers/mfd/sm501.c @@ -1145,22 +1145,14 @@ static int sm501_register_gpio_i2c_instance(struct sm501_devdata *sm, return -ENOMEM; lookup->dev_id = "i2c-gpio"; - if (iic->pin_sda < 32) - lookup->table[0].chip_label = "SM501-LOW"; - else - lookup->table[0].chip_label = "SM501-HIGH"; - lookup->table[0].chip_hwnum = iic->pin_sda % 32; - lookup->table[0].con_id = NULL; - lookup->table[0].idx = 0; - lookup->table[0].flags = GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN; - if (iic->pin_scl < 32) - lookup->table[1].chip_label = "SM501-LOW"; - else - lookup->table[1].chip_label = "SM501-HIGH"; - lookup->table[1].chip_hwnum = iic->pin_scl % 32; - lookup->table[1].con_id = NULL; - lookup->table[1].idx = 1; - lookup->table[1].flags = GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN; + lookup->table[0] = (struct gpiod_lookup) + GPIO_LOOKUP_IDX(iic->pin_sda < 32 ? "SM501-LOW" : "SM501-HIGH", + iic->pin_sda % 32, NULL, 0, + GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN); + lookup->table[1] = (struct gpiod_lookup) + GPIO_LOOKUP_IDX(iic->pin_scl < 32 ? "SM501-LOW" : "SM501-HIGH", + iic->pin_scl % 32, NULL, 1, + GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN); gpiod_add_lookup_table(lookup); icd = dev_get_platdata(>dev); -- 2.17.1
[PATCH v7 6/6] MAINTAINERS: Add GPIO Aggregator section
Add a maintainership section for the GPIO Aggregator, covering documentation and driver source code. Signed-off-by: Geert Uytterhoeven Reviewed-by: Eugeniu Rosca Tested-by: Eugeniu Rosca --- v7: - Maintained => Supported, v6: - No changes, v5: - Add Reviewed-by, Tested-by, v4: - Drop controversial GPIO repeater, v3: - New. --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index fb124a4a29cb539e..4f14c1a88ad45c5b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7232,6 +7232,13 @@ F: Documentation/firmware-guide/acpi/gpio-properties.rst F: drivers/gpio/gpiolib-acpi.c F: drivers/gpio/gpiolib-acpi.h +GPIO AGGREGATOR +M: Geert Uytterhoeven +L: linux-g...@vger.kernel.org +S: Supported +F: Documentation/admin-guide/gpio/gpio-aggregator.rst +F: drivers/gpio/gpio-aggregator.c + GPIO IR Transmitter M: Sean Young L: linux-me...@vger.kernel.org -- 2.17.1
[PATCH v7 4/6] gpio: Add GPIO Aggregator
GPIO controllers are exported to userspace using /dev/gpiochip* character devices. Access control to these devices is provided by standard UNIX file system permissions, on an all-or-nothing basis: either a GPIO controller is accessible for a user, or it is not. Currently no mechanism exists to control access to individual GPIOs. Hence add a GPIO driver to aggregate existing GPIOs, and expose them as a new gpiochip. This supports the following use cases: - Aggregating GPIOs using Sysfs This is useful for implementing access control, and assigning a set of GPIOs to a specific user or virtual machine. - Generic GPIO Driver This is useful for industrial control, where it can provide userspace access to a simple GPIO-operated device described in DT, cfr. e.g. spidev for SPI-operated devices. Signed-off-by: Geert Uytterhoeven Reviewed-by: Eugeniu Rosca Tested-by: Eugeniu Rosca --- v7: - No changes, v6: - Use gpiod_to_chip() instead of open-coding, - Drop debug print of gpio_desc.label, as it usually just points to the GPIO Aggregator itself, - Drop no longer needed #include "gpiolib.h", - Fix missing offset translation in gpio_fwd_set_config(), - Use GPIO_LOOKUP_IDX() to populate struct gpiod_lookup, v5: - Add Reviewed-by, Tested-by, v4: - Remove unused assignment to n in isrange(), - Check correct pointer after aggr->lookups->dev_id allocation, - Preinitialize flags to 0 in gpio_fwd_[gs]et_multiple() to avoid may-be-used-uninitialized warning, - Drop controversial GPIO repeater, - Update for gpiod_lookup.chip_label rename, - Use %pe to format error pointers, - Use U16_MAX instead of (u16)-1, - Correct comment indentation, - Use skip_spaces() helper, - Rename a and b to first_index resp. last_index, - Add comment to tmp[] use, - Improve Kconfig help text, - Include for gpiod_[gs]et_*(), - Drop unneeded valid_mask handling, - Add comment about sleeping and .set_config() support, v3: - Absorb GPIO forwarder, - Integrate GPIO Repeater and Generic GPIO driver functionality, - Use the aggregator parameters to create a GPIO lookup table instead of an array of GPIO descriptors, which allows to simplify the code: 1. This removes the need for calling gpio_name_to_desc(), gpiochip_find(), gpiochip_get_desc(), and gpiod_request(), 2. This allows the platform device to always use devm_gpiod_get_index(), regardless of the origin of the GPIOs, - Move parameter parsing from platform device probe to sysfs attribute store, removing the need for platform data passing, - Use more devm_*() functions to simplify cleanup, - Add pr_fmt(), - General refactoring, v2: - Add missing initialization of i in gpio_virt_agg_probe(), - Update for removed .need_valid_mask field and changed .init_valid_mask() signature, - Drop "virtual", rename to gpio-aggregator, - Drop bogus FIXME related to gpiod_set_transitory() expectations, - Use new GPIO Forwarder Helper, - Lift limit on the maximum number of GPIOs, - Improve parsing: - add support for specifying GPIOs by line name, - add support for specifying GPIO chips by ID, - add support for GPIO offset ranges, - names and offset specifiers must be separated by whitespace, - GPIO offsets must separated by spaces, - Use str_has_prefix() and kstrtouint(). --- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-aggregator.c | 568 + 3 files changed, 581 insertions(+) create mode 100644 drivers/gpio/gpio-aggregator.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 8ef2179fb9991b78..c92e0a16c352f3d1 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1541,6 +1541,18 @@ config GPIO_VIPERBOARD endmenu +config GPIO_AGGREGATOR + tristate "GPIO Aggregator" + help + Say yes here to enable the GPIO Aggregator, which provides a way to + aggregate existing GPIO lines into a new virtual GPIO chip. + This can serve the following purposes: + - Assign permissions for a collection of GPIO lines to a user, + - Export a collection of GPIO lines to a virtual machine, + - Provide a generic driver for a GPIO-operated device in an + industrial control context, to be operated from userspace using + the GPIO chardev interface. + config GPIO_MOCKUP tristate "GPIO Testing Driver" select IRQ_SIM diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index b2cfc21a97f3e52b..65bf3940e33cf734 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GP
[PATCH v7 1/6] i2c: i801: Use GPIO_LOOKUP() helper macro
i801_add_mux() fills in the GPIO lookup table by manually populating an
array of gpiod_lookup structures. Use the existing GPIO_LOOKUP() helper
macro instead, to relax a dependency on the gpiod_lookup structure's
member names.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Jean Delvare
---
This is now commit be1b92c133cc91b2 ("i2c: i801: Use GPIO_LOOKUP()
helper macro") in i2c/for-next.
v7:
- Add Reviewed-by,
v6:
- New.
---
drivers/i2c/busses/i2c-i801.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a9c03f5c34825a95..4f333889489c882a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1439,9 +1439,9 @@ static int i801_add_mux(struct i801_priv *priv)
return -ENOMEM;
lookup->dev_id = "i2c-mux-gpio";
for (i = 0; i < mux_config->n_gpios; i++) {
- lookup->table[i].chip_label = mux_config->gpio_chip;
- lookup->table[i].chip_hwnum = mux_config->gpios[i];
- lookup->table[i].con_id = "mux";
+ lookup->table[i] = (struct gpiod_lookup)
+ GPIO_LOOKUP(mux_config->gpio_chip,
+ mux_config->gpios[i], "mux", 0);
}
gpiod_add_lookup_table(lookup);
priv->lookup = lookup;
--
2.17.1
Re: [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name
Hi Linus,
On Thu, Mar 26, 2020 at 10:18 PM Linus Walleij wrote:
> On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
> wrote:
> > Currently a GPIO lookup table can only refer to a specific GPIO by a
> > tuple, consisting of a GPIO controller label and a GPIO offset inside
> > the controller.
> >
> > However, a GPIO may also carry a line name, defined by DT or ACPI.
> > If present, the line name is the most use-centric way to refer to a
> > GPIO. Hence add support for looking up GPIOs by line name.
> >
> > Implement this by reusing the existing gpiod_lookup infrastructure.
> > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
> > that this field can have two meanings, and update the kerneldoc and
> > GPIO_LOOKUP*() macros.
> >
> > Signed-off-by: Geert Uytterhoeven
> > Reviewed-by: Ulrich Hecht
> > Reviewed-by: Eugeniu Rosca
> > Tested-by: Eugeniu Rosca
>
> I kind of like this approach, however there are things here that
> need to be considered: the line name is in no way globally unique,
> and I think there are already quite a few GPIO chips that
> have the same line names assigned for every instance of that
> chip.
>
> gpiochip_set_desc_names() only warns if there is a line with
> the same name on the same gpio_chip.
on a _different_ gpio chip.
> I suppose we need to document that the line name look-up
> will be on a first-come-first-served basis: whatever line
> we find first with this name is what you will get a reference
> to, no matter what chip it is on, and it is possible albeit
> not recommended that some other chip has a line with the
> same name.
Agreed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH QEMU v2 3/5] Add a GPIO backend using libgpiod
Hi Philippe,
Thanks for your comments!
On Thu, Apr 23, 2020 at 11:28 AM Philippe Mathieu-Daudé wrote:
> On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> > Add a GPIO controller backend, to connect virtual GPIOs on the guest to
> > physical GPIOs on the host. This allows the guest to control any
> > external device connected to the physical GPIOs.
> >
> > Features and limitations:
> > - The backend uses libgpiod on Linux,
> > - For now only GPIO outputs are supported,
> > - The number of GPIO lines mapped is limited to the number of GPIO
> > lines available on the virtual GPIO controller.
> >
> > Future work:
> > - GPIO inputs,
> > - GPIO line configuration,
> > - Optimizations for controlling multiple GPIO lines at once,
> > - ...
> >
> > Signed-off-by: Geert Uytterhoeven
> > --- /dev/null
> > +++ b/backends/gpiodev.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * QEMU GPIO Backend
> > + *
> > + * Copyright (C) 2018-2020 Glider bv
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include
>
> probably not needed.
It is indeed included by one of the other header files.
What is the QEMU policy w.r.t. that?
>
> > +#include
>
> Please move this one...
>
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/config-file.h"
> > +#include "qemu/cutils.h"
I forgot to remove the two above...
> > +#include "qemu/error-report.h"
> > +#include "qemu/module.h"
> > +#include "qemu/option.h"
... and the two above.
> > +#include "qapi/error.h"
> > +
> > +#include "sysemu/gpiodev.h"
> > +
> > +#include "hw/irq.h"
> > +#include "hw/qdev-core.h"
>
> ... here:
>
> #include
OK.
> > --- a/configure
> > +++ b/configure
> > @@ -509,6 +509,7 @@ libpmem=""
> > default_devices="yes"
> > plugins="no"
> > fuzzing="no"
> > +gpio=""
>
> Maybe name this feature 'libgpiod'?
Makes sense.
> >
> > supported_cpu="no"
> > supported_os="no"
> > @@ -1601,6 +1602,10 @@ for opt do
> >;;
> >--gdb=*) gdb_bin="$optarg"
> >;;
> > + --disable-gpio) gpio="no"
> > + ;;
> > + --enable-gpio) gpio="yes"
>
> Ditto: --enable-libgpiod, because else it seems rather confusing.
OK.
> > --- /dev/null
> > +++ b/include/sysemu/gpiodev.h
> > @@ -0,0 +1,12 @@
> > +/*
> > + * QEMU GPIO Backend
> > + *
> > + * Copyright (C) 2018-2020 Glider bv
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "qemu/typedefs.h"
>
> "qemu/typedefs.h" not needed in includes.
While removing that works, it does mean the header file is no longer
self-contained:
include/sysemu/gpiodev.h:10:23: error: unknown type name ‘DeviceState’
> > +
> > +void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int
> > maxgpio,
> > + Error **errp);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH QEMU v2 2/5] ARM: PL061: Extract pl061_create_fdt()
Move the code to create the DT node for the PL061 GPIO controller from
hw/arm/virt.c to the PL061 driver, so it can be reused.
While at it, make the created node comply with the PL061 Device Tree
bindings:
- Use generic node name "gpio" instead of "pl061",
- Add missing "#interrupt-cells" and "interrupt-controller"
properties.
Signed-off-by: Geert Uytterhoeven
---
v2:
- New.
---
hw/arm/virt.c | 20 +++-
hw/gpio/pl061.c | 42 +
include/hw/gpio/pl061.h | 7 +++
3 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7dc96abf72cf2b9a..c88c8850fbe00bdb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -40,6 +40,7 @@
#include "hw/arm/primecell.h"
#include "hw/arm/virt.h"
#include "hw/block/flash.h"
+#include "hw/gpio/pl061.h"
#include "hw/vfio/vfio-calxeda-xgmac.h"
#include "hw/vfio/vfio-amd-xgbe.h"
#include "hw/display/ramfb.h"
@@ -807,30 +808,16 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
static void create_gpio(const VirtMachineState *vms)
{
-char *nodename;
DeviceState *pl061_dev;
hwaddr base = vms->memmap[VIRT_GPIO].base;
hwaddr size = vms->memmap[VIRT_GPIO].size;
int irq = vms->irqmap[VIRT_GPIO];
-const char compat[] = "arm,pl061\0arm,primecell";
pl061_dev = sysbus_create_simple("pl061", base,
qdev_get_gpio_in(vms->gic, irq));
-uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
-nodename = g_strdup_printf("/pl061@%" PRIx64, base);
-qemu_fdt_add_subnode(vms->fdt, nodename);
-qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
- 2, base, 2, size);
-qemu_fdt_setprop(vms->fdt, nodename, "compatible", compat, sizeof(compat));
-qemu_fdt_setprop_cell(vms->fdt, nodename, "#gpio-cells", 2);
-qemu_fdt_setprop(vms->fdt, nodename, "gpio-controller", NULL, 0);
-qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
- GIC_FDT_IRQ_TYPE_SPI, irq,
- GIC_FDT_IRQ_FLAGS_LEVEL_HI);
-qemu_fdt_setprop_cell(vms->fdt, nodename, "clocks", vms->clock_phandle);
-qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
-qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
+uint32_t phandle = pl061_create_fdt(vms->fdt, "", 2, base, size, irq,
+vms->clock_phandle);
gpio_key_dev = sysbus_create_simple("gpio-key", -1,
qdev_get_gpio_in(pl061_dev, 3));
@@ -846,7 +833,6 @@ static void create_gpio(const VirtMachineState *vms)
KEY_POWER);
qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
"gpios", phandle, 3, 0);
-g_free(nodename);
}
static void create_virtio_devices(const VirtMachineState *vms)
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index e776c09e474216ef..74ba733a8a5e8ca5 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -9,12 +9,14 @@
*/
#include "qemu/osdep.h"
+#include "hw/arm/fdt.h"
#include "hw/gpio/pl061.h"
#include "hw/irq.h"
#include "hw/sysbus.h"
#include "migration/vmstate.h"
#include "qemu/log.h"
#include "qemu/module.h"
+#include "sysemu/device_tree.h"
//#define DEBUG_PL061 1
@@ -397,3 +399,43 @@ static void pl061_register_types(void)
}
type_init(pl061_register_types)
+
+/*
+ * pl061_create_fdt: Create a DT node for a PL061 GPIO controller
+ * @fdt: device tree blob
+ * @parent: name of the parent node
+ * @n_cells: value of #address-cells and #size-cells
+ * @base: base address of the controller's register block
+ * @size: size of the controller's register block
+ * @irq: interrupt number
+ * @clock: phandle of the apb-pclk clock
+ *
+ * Return value: a phandle referring to the created DT node.
+ *
+ * See the DT Binding Documentation in the Linux kernel source tree:
+ * Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
+ */
+uint32_t pl061_create_fdt(void *fdt, const char *parent, unsigned int n_cells,
+ hwaddr base, hwaddr size, int irq, uint32_t clock)
+{
+char *nodename = g_strdup_printf("%s/gpio@%" PRIx64, parent, base);
+static const char compat[] = "arm,pl061\0arm,primecell";
+uint32_t phandle = qemu_fdt_alloc_phandle(fdt);
+
+qemu_fdt_add_subnode(fdt, nodename);
+qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", n_cells, base,
[PATCH QEMU v2 5/5] hw/arm/virt: Add dynamic PL061 GPIO support
Add support for dynamically instantiating PL061 GPIO controller
instances in ARM virt. Device tree nodes are created dynamically.
Signed-off-by: Geert Uytterhoeven
---
v2:
- New.
---
hw/arm/sysbus-fdt.c | 18 ++
hw/arm/virt.c | 1 +
2 files changed, 19 insertions(+)
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 6b6906f4cfc36198..493583218d176d0a 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -32,6 +32,7 @@
#include "sysemu/device_tree.h"
#include "sysemu/tpm.h"
#include "hw/platform-bus.h"
+#include "hw/gpio/pl061.h"
#include "hw/vfio/vfio-platform.h"
#include "hw/vfio/vfio-calxeda-xgmac.h"
#include "hw/vfio/vfio-amd-xgbe.h"
@@ -468,6 +469,22 @@ static int add_tpm_tis_fdt_node(SysBusDevice *sbdev, void
*opaque)
return 0;
}
+/*
+ * add_pl061_node: Create a DT node for a PL061 GPIO controller
+ */
+static int add_pl061_node(SysBusDevice *sbdev, void *opaque)
+{
+PlatformBusFDTData *data = opaque;
+PlatformBusDevice *pbus = data->pbus;
+void *fdt = data->fdt;
+
+pl061_create_fdt(fdt, data->pbus_node_name, 1,
+ platform_bus_get_mmio_addr(pbus, sbdev, 0), 0x1000,
+ platform_bus_get_irqn(pbus, sbdev, 0) + data->irq_start,
+ qemu_fdt_get_phandle(fdt, "/apb-pclk"));
+return 0;
+}
+
static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
{
return 0;
@@ -489,6 +506,7 @@ static const BindingEntry bindings[] = {
VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
#endif
TYPE_BINDING(TYPE_TPM_TIS_SYSBUS, add_tpm_tis_fdt_node),
+TYPE_BINDING(TYPE_PL061, add_pl061_node),
TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
TYPE_BINDING("", NULL), /* last element */
};
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c88c8850fbe00bdb..191594db09422d54 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2178,6 +2178,7 @@ static void virt_machine_class_init(ObjectClass *oc, void
*data)
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
+machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PL061);
mc->block_default_type = IF_VIRTIO;
mc->no_cdrom = 1;
mc->pci_allow_0_address = true;
--
2.17.1
[PATCH QEMU v2 1/5] ARM: PL061: Move TYPE_PL061 to hw/gpio/pl061.h
Move the definition of TYPE_PL061 to a new header file, so it can be
used outside the driver.
Signed-off-by: Geert Uytterhoeven
---
v2:
- New.
---
MAINTAINERS | 1 +
hw/gpio/pl061.c | 2 +-
include/hw/gpio/pl061.h | 16
3 files changed, 18 insertions(+), 1 deletion(-)
create mode 100644 include/hw/gpio/pl061.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 8cbc1fac2bfcec86..e760f65270d29d5d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -538,6 +538,7 @@ F: hw/dma/pl080.c
F: include/hw/dma/pl080.h
F: hw/dma/pl330.c
F: hw/gpio/pl061.c
+F: include/hw/gpio/pl061.h
F: hw/input/pl050.c
F: hw/intc/pl190.c
F: hw/sd/pl181.c
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 2a828260bdb0b946..e776c09e474216ef 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -9,6 +9,7 @@
*/
#include "qemu/osdep.h"
+#include "hw/gpio/pl061.h"
#include "hw/irq.h"
#include "hw/sysbus.h"
#include "migration/vmstate.h"
@@ -33,7 +34,6 @@ static const uint8_t pl061_id[12] =
static const uint8_t pl061_id_luminary[12] =
{ 0x00, 0x00, 0x00, 0x00, 0x61, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1 };
-#define TYPE_PL061 "pl061"
#define PL061(obj) OBJECT_CHECK(PL061State, (obj), TYPE_PL061)
typedef struct PL061State {
diff --git a/include/hw/gpio/pl061.h b/include/hw/gpio/pl061.h
new file mode 100644
index ..78cc40c52679dc4e
--- /dev/null
+++ b/include/hw/gpio/pl061.h
@@ -0,0 +1,16 @@
+/*
+ * Arm PrimeCell PL061 General Purpose IO with additional Luminary Micro
+ * Stellaris bits.
+ *
+ * Copyright (c) 2007 CodeSourcery.
+ * Written by Paul Brook
+ *
+ * This code is licensed under the GPL.
+ */
+
+#ifndef PL061_GPIO_H
+#define PL061_GPIO_H
+
+#define TYPE_PL061 "pl061"
+
+#endif /* PL061_GPIO_H */
--
2.17.1
[PATCH QEMU v2 0/5] Add a GPIO backend
Hi all,
This patch series adds a GPIO controller backend, to connect virtual
GPIOs on the guest to physical GPIOs on the host, and enables support
for this using user-creatable PL061 GPIO controller instances. This
allows the guest to control any external device connected to the
physical GPIOs.
While this can be used with an upstream Linux kernel (e.g. using a
dedicated GPIO controller connected to an external bus), proper
isolation and assignment of GPIOs to virtual machines depends on the
GPIO Aggregator[1], which has not been accepted in Linux upstream yet.
Aggregating GPIOs and exposing them as a new gpiochip was suggested in
response to my proof-of-concept for GPIO virtualization with QEMU[2][3].
Features and limitations:
- The backend uses libgpiod on Linux,
- For now only GPIO outputs are supported,
- The number of GPIO lines mapped is limited to the number of GPIO
lines available on the virtual GPIO controller (i.e. 8 on PL061).
Future work:
- GPIO inputs,
- GPIO line configuration,
- Optimizations for controlling multiple GPIO lines at once,
- ...
This series contains 5 patches:
- The first two patches refactor the existing code for reuse,
- The third patch adds a gneric GPIO backend using libgpiod,
- The fourth patch adds gpiodev support to the PL061 driver,
- The fifth patch adds dynamic PL061 support to ARM virt.
Changes compared to v1[2]:
- Drop vgpios and gpios parameters, and map the full gpiochip instead,
- Replace the single hardcoded PL061 instance (created by ARM virt) by
multiple dynamically created instances, one per imported GPIO
controller.
For testing, I have pushed this series to the topic/gpio-backend-v2
branch of my git repository at https://github.com/geertu/qemu.git.
Sample session on the Renesas Salvator-XS development board:
- Unbind keyboard (shared with LEDs) from gpio-keys driver:
host$ echo keys > /sys/bus/platform/drivers/gpio-keys/unbind
- Aggregate GPIO lines connected to LEDs into a new virtual GPIO chip:
host$ echo e6055400.gpio 11-13 \
> /sys/bus/platform/drivers/gpio-aggregator/new_device
gpio-aggregator gpio-aggregator.0: 0 => gpio-371
gpio-aggregator gpio-aggregator.0: 1 => gpio-372
gpio-aggregator gpio-aggregator.0: 2 => gpio-373
gpiochip_find_base: found new base at 343
gpio gpiochip10: (gpio-aggregator.0): added GPIO chardev (254:10)
gpiochip_setup_dev: registered GPIOs 343 to 345 on device: gpiochip10
(gpio-aggregator.0)
- Adjust permissions on /dev/gpiochip10 (optional)
- Launch QEMU:
host$ aarch64-softmmu/qemu-system-aarch64 -enable-kvm -M virt \
-cpu cortex-a57 -m 1024 -nographic -kernel /path/to/Image \
-device pl061,host=gpio-aggregator.0
...
pl061_gpio c00.gpio: PL061 GPIO chip registered
...
- Control LEDs:
guest$ gpioset gpiochip0 0=0 1=1 # LED4 OFF, LED5 ON
guest$ gpioset gpiochip0 0=1 1=0 # LED4 ON, LED5 OFF
Thanks for your comments!
[1] "[PATCH v6 0/8] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/linux-gpio/20200324135328.5796-1-geert+rene...@glider.be/)
[2] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/linux-gpio/20181003152521.23144-1-geert+rene...@glider.be/)
[3] "Getting To Blinky: Virt Edition / Making device pass-through work
on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
Geert Uytterhoeven (5):
ARM: PL061: Move TYPE_PL061 to hw/gpio/pl061.h
ARM: PL061: Extract pl061_create_fdt()
Add a GPIO backend using libgpiod
ARM: PL061: Add gpiodev support
hw/arm/virt: Add dynamic PL061 GPIO support
MAINTAINERS | 7 +++
backends/Makefile.objs | 2 +
backends/gpiodev.c | 94
configure| 28
hw/arm/sysbus-fdt.c | 18
hw/arm/virt.c| 21 ++---
hw/gpio/pl061.c | 79 -
include/hw/gpio/pl061.h | 23 ++
include/sysemu/gpiodev.h | 12 +
qemu-options.hx | 9
10 files changed, 275 insertions(+), 18 deletions(-)
create mode 100644 backends/gpiodev.c
create mode 100644 include/hw/gpio/pl061.h
create mode 100644 include/sysemu/gpiodev.h
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH QEMU v2 3/5] Add a GPIO backend using libgpiod
Add a GPIO controller backend, to connect virtual GPIOs on the guest to
physical GPIOs on the host. This allows the guest to control any
external device connected to the physical GPIOs.
Features and limitations:
- The backend uses libgpiod on Linux,
- For now only GPIO outputs are supported,
- The number of GPIO lines mapped is limited to the number of GPIO
lines available on the virtual GPIO controller.
Future work:
- GPIO inputs,
- GPIO line configuration,
- Optimizations for controlling multiple GPIO lines at once,
- ...
Signed-off-by: Geert Uytterhoeven
---
v2:
- Drop vgpios and gpios parameters, and map the full gpiochip instead,
- Replace hardcoded PL061 instance by multiple dynamic instances,
registered through qemu_gpiodev_add().
---
MAINTAINERS | 6 +++
backends/Makefile.objs | 2 +
backends/gpiodev.c | 94
configure| 28
include/sysemu/gpiodev.h | 12 +
5 files changed, 142 insertions(+)
create mode 100644 backends/gpiodev.c
create mode 100644 include/sysemu/gpiodev.h
diff --git a/MAINTAINERS b/MAINTAINERS
index e760f65270d29d5d..a70af47430083d14 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -607,6 +607,12 @@ F: include/hw/arm/digic.h
F: hw/*/digic*
F: include/hw/*/digic*
+GPIO device backend
+M: Geert Uytterhoeven
+S: Supported
+F: backends/gpiodev.c
+F: include/sysemu/gpiodev.h
+
Goldfish RTC
M: Anup Patel
M: Alistair Francis
diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index 28a847cd571d96ed..ee658e797454119a 100644
--- a/backends/Makefile.objs
+++ b/backends/Makefile.objs
@@ -21,3 +21,5 @@ common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
common-obj-$(CONFIG_GIO) += dbus-vmstate.o
dbus-vmstate.o-cflags = $(GIO_CFLAGS)
dbus-vmstate.o-libs = $(GIO_LIBS)
+
+common-obj-$(CONFIG_GPIODEV) += gpiodev.o
diff --git a/backends/gpiodev.c b/backends/gpiodev.c
new file mode 100644
index ..df1bd0113c7b2985
--- /dev/null
+++ b/backends/gpiodev.c
@@ -0,0 +1,94 @@
+/*
+ * QEMU GPIO Backend
+ *
+ * Copyright (C) 2018-2020 Glider bv
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include
+#include
+
+#include "qemu/osdep.h"
+#include "qemu/config-file.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+#include "qapi/error.h"
+
+#include "sysemu/gpiodev.h"
+
+#include "hw/irq.h"
+#include "hw/qdev-core.h"
+
+static void gpiodev_irq_handler(void *opaque, int n, int level)
+{
+struct gpiod_line *line = opaque;
+int status;
+
+status = gpiod_line_set_value(line, level);
+if (status < 0) {
+struct gpiod_chip *chip = gpiod_line_get_chip(line);
+
+error_report("%s/%s: Cannot set GPIO line %u: %s",
+ gpiod_chip_name(chip), gpiod_chip_label(chip),
+ gpiod_line_offset(line), strerror(errno));
+}
+}
+
+static int gpiodev_map_line(DeviceState *dev, struct gpiod_chip *chip,
+unsigned int gpio, Error **errp)
+{
+struct gpiod_line *line;
+qemu_irq irq;
+int status;
+
+line = gpiod_chip_get_line(chip, gpio);
+if (!line) {
+error_setg(errp, "Cannot obtain GPIO line %u: %s", gpio,
+ strerror(errno));
+return -1;
+}
+
+status = gpiod_line_request_output(line, "qemu", 0);
+if (status < 0) {
+error_setg(errp, "Cannot request GPIO line %u for output: %s", gpio,
+ strerror(errno));
+return status;
+}
+
+irq = qemu_allocate_irq(gpiodev_irq_handler, line, 0);
+qdev_connect_gpio_out(dev, gpio, irq);
+return 0;
+}
+
+void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
+ Error **errp)
+{
+struct gpiod_chip *chip;
+unsigned int i, n;
+int status;
+
+chip = gpiod_chip_open_lookup(name);
+if (!chip) {
+error_setg(errp, "Cannot open GPIO chip %s: %s", name,
+ strerror(errno));
+return;
+}
+
+n = gpiod_chip_num_lines(chip);
+if (n > maxgpio) {
+warn_report("Last %u GPIO line(s) will not be mapped", n - maxgpio);
+n = maxgpio;
+}
+
+for (i = 0; i < n; i++) {
+status = gpiodev_map_line(dev, chip, i, errp);
+if (status < 0) {
+return;
+}
+}
+
+info_report("Mapped %u GPIO lines", n);
+}
diff --git a/configure b/configure
index 23b5e93752b6a259..8b133402ef727c8e 100755
--- a/configure
+++ b/configure
@@ -509,6 +509,7 @@ libpmem=""
default_devices="yes"
plugins="no"
fuzzing="no"
+gpio=""
supported_cpu="no"
supported_os="no"
@@ -1601,6 +1602,10
[PATCH QEMU v2 4/5] ARM: PL061: Add gpiodev support
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=
with the name or label of the gpiochip on the host.
Signed-off-by: Geert Uytterhoeven
---
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);
}
+#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 = _pl061;
dc->reset = _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,
--
2.17.1
Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
Hi Linus,
On Thu, Mar 26, 2020 at 10:26 PM Linus Walleij wrote:
> On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven
> wrote:
> > The GPIO Aggregator will need a method to forward a .set_config() call
> > to its parent gpiochip. This requires obtaining the gpio_chip and
> > offset for a given gpio_desc. While gpiod_to_chip() is public,
> > gpio_chip_hwgpio() is not, so there is currently no method to obtain the
> > needed GPIO offset parameter.
> >
> > Hence introduce a public gpiod_set_config() helper, which invokes the
> > .set_config() callback through a gpio_desc pointer, like is done for
> > most other gpio_chip callbacks.
> >
> > Rewrite the existing gpiod_set_debounce() helper as a wrapper around
> > gpiod_set_config(), to avoid duplication.
> >
> > Signed-off-by: Geert Uytterhoeven
> > ---
> > v6:
> > - New.
>
> This is nice, I tried to actually just apply this (you also sent some
> two cleanups that I tried to apply) byt Yue's cleanup patch
> commit d18fddff061d2796525e6d4a958cb3d30aed8efd
> "gpiolib: Remove duplicated function gpio_do_set_config()"
> makes none of them apply :/
/me confused.
That commit was reverted later, so it shouldn't matter.
I have just verified, and both my full series and just this single
patch, do apply fine to all of current gpio/for-next, linus/master, and
next-20200327. They even apply fine to gpio/for-next before or after
the two cleanups I sent, too.
What am I missing?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v6 6/8] gpio: Add GPIO Aggregator
GPIO controllers are exported to userspace using /dev/gpiochip* character devices. Access control to these devices is provided by standard UNIX file system permissions, on an all-or-nothing basis: either a GPIO controller is accessible for a user, or it is not. Currently no mechanism exists to control access to individual GPIOs. Hence add a GPIO driver to aggregate existing GPIOs, and expose them as a new gpiochip. This supports the following use cases: - Aggregating GPIOs using Sysfs This is useful for implementing access control, and assigning a set of GPIOs to a specific user or virtual machine. - Generic GPIO Driver This is useful for industrial control, where it can provide userspace access to a simple GPIO-operated device described in DT, cfr. e.g. spidev for SPI-operated devices. Signed-off-by: Geert Uytterhoeven Reviewed-by: Eugeniu Rosca Tested-by: Eugeniu Rosca --- v6: - Use gpiod_to_chip() instead of open-coding, - Drop debug print of gpio_desc.label, as it usually just points to the GPIO Aggregator itself, - Drop no longer needed #include "gpiolib.h", - Fix missing offset translation in gpio_fwd_set_config(), - Use GPIO_LOOKUP_IDX() to populate struct gpiod_lookup, v5: - Add Reviewed-by, Tested-by, v4: - Remove unused assignment to n in isrange(), - Check correct pointer after aggr->lookups->dev_id allocation, - Preinitialize flags to 0 in gpio_fwd_[gs]et_multiple() to avoid may-be-used-uninitialized warning, - Drop controversial GPIO repeater, - Update for gpiod_lookup.chip_label rename, - Use %pe to format error pointers, - Use U16_MAX instead of (u16)-1, - Correct comment indentation, - Use skip_spaces() helper, - Rename a and b to first_index resp. last_index, - Add comment to tmp[] use, - Improve Kconfig help text, - Include for gpiod_[gs]et_*(), - Drop unneeded valid_mask handling, - Add comment about sleeping and .set_config() support, v3: - Absorb GPIO forwarder, - Integrate GPIO Repeater and Generic GPIO driver functionality, - Use the aggregator parameters to create a GPIO lookup table instead of an array of GPIO descriptors, which allows to simplify the code: 1. This removes the need for calling gpio_name_to_desc(), gpiochip_find(), gpiochip_get_desc(), and gpiod_request(), 2. This allows the platform device to always use devm_gpiod_get_index(), regardless of the origin of the GPIOs, - Move parameter parsing from platform device probe to sysfs attribute store, removing the need for platform data passing, - Use more devm_*() functions to simplify cleanup, - Add pr_fmt(), - General refactoring, v2: - Add missing initialization of i in gpio_virt_agg_probe(), - Update for removed .need_valid_mask field and changed .init_valid_mask() signature, - Drop "virtual", rename to gpio-aggregator, - Drop bogus FIXME related to gpiod_set_transitory() expectations, - Use new GPIO Forwarder Helper, - Lift limit on the maximum number of GPIOs, - Improve parsing: - add support for specifying GPIOs by line name, - add support for specifying GPIO chips by ID, - add support for GPIO offset ranges, - names and offset specifiers must be separated by whitespace, - GPIO offsets must separated by spaces, - Use str_has_prefix() and kstrtouint(). --- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-aggregator.c | 568 + 3 files changed, 581 insertions(+) create mode 100644 drivers/gpio/gpio-aggregator.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 6234ccc90e7eb4a1..6ddc7353a46afdf6 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1541,6 +1541,18 @@ config GPIO_VIPERBOARD endmenu +config GPIO_AGGREGATOR + tristate "GPIO Aggregator" + help + Say yes here to enable the GPIO Aggregator, which provides a way to + aggregate existing GPIO lines into a new virtual GPIO chip. + This can serve the following purposes: + - Assign permissions for a collection of GPIO lines to a user, + - Export a collection of GPIO lines to a virtual machine, + - Provide a generic driver for a GPIO-operated device in an + industrial control context, to be operated from userspace using + the GPIO chardev interface. + config GPIO_MOCKUP tristate "GPIO Testing Driver" select IRQ_SIM diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index b2cfc21a97f3e52b..65bf3940e33cf734 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) +
[PATCH v6 3/8] mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
i801_add_mux() fills in the GPIO lookup table by manually populating an array of gpiod_lookup structures. Use the existing GPIO_LOOKUP_IDX() helper macro instead, to relax a dependency on the gpiod_lookup structure's member names. Signed-off-by: Geert Uytterhoeven Cc: Lee Jones --- While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name", it can be applied independently. But an Acked-by would be nice, too. Cover letter and full series at https://lore.kernel.org/r/20200324135328.5796-1-geert+rene...@glider.be/ v6: - New. --- drivers/mfd/sm501.c | 24 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c index e49787e6bb93e5c8..ccd62b963952814e 100644 --- a/drivers/mfd/sm501.c +++ b/drivers/mfd/sm501.c @@ -1145,22 +1145,14 @@ static int sm501_register_gpio_i2c_instance(struct sm501_devdata *sm, return -ENOMEM; lookup->dev_id = "i2c-gpio"; - if (iic->pin_sda < 32) - lookup->table[0].chip_label = "SM501-LOW"; - else - lookup->table[0].chip_label = "SM501-HIGH"; - lookup->table[0].chip_hwnum = iic->pin_sda % 32; - lookup->table[0].con_id = NULL; - lookup->table[0].idx = 0; - lookup->table[0].flags = GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN; - if (iic->pin_scl < 32) - lookup->table[1].chip_label = "SM501-LOW"; - else - lookup->table[1].chip_label = "SM501-HIGH"; - lookup->table[1].chip_hwnum = iic->pin_scl % 32; - lookup->table[1].con_id = NULL; - lookup->table[1].idx = 1; - lookup->table[1].flags = GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN; + lookup->table[0] = (struct gpiod_lookup) + GPIO_LOOKUP_IDX(iic->pin_sda < 32 ? "SM501-LOW" : "SM501-HIGH", + iic->pin_sda % 32, NULL, 0, + GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN); + lookup->table[1] = (struct gpiod_lookup) + GPIO_LOOKUP_IDX(iic->pin_scl < 32 ? "SM501-LOW" : "SM501-HIGH", + iic->pin_scl % 32, NULL, 1, + GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN); gpiod_add_lookup_table(lookup); icd = dev_get_platdata(>dev); -- 2.17.1
[PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro
impd1_probe() fills in the GPIO lookup table by manually populating an array of gpiod_lookup structures. Use the existing GPIO_LOOKUP() helper macro instead, to relax a dependency on the gpiod_lookup structure's member names. Signed-off-by: Geert Uytterhoeven Cc: linux-arm-ker...@lists.infradead.org --- While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name", it can be applied independently. But an Acked-by would be nice, too. Cover letter and full series at https://lore.kernel.org/r/20200324135328.5796-1-geert+rene...@glider.be/ v6: - New. --- arch/arm/mach-integrator/impd1.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/arch/arm/mach-integrator/impd1.c b/arch/arm/mach-integrator/impd1.c index 1ecbea5331d6ed8b..6f875ded841924d8 100644 --- a/arch/arm/mach-integrator/impd1.c +++ b/arch/arm/mach-integrator/impd1.c @@ -410,13 +410,10 @@ static int __ref impd1_probe(struct lm_device *dev) * 5 = Key lower right */ /* We need the two MMCI GPIO entries */ - lookup->table[0].chip_label = chipname; - lookup->table[0].chip_hwnum = 3; - lookup->table[0].con_id = "wp"; - lookup->table[1].chip_label = chipname; - lookup->table[1].chip_hwnum = 4; - lookup->table[1].con_id = "cd"; - lookup->table[1].flags = GPIO_ACTIVE_LOW; + lookup->table[0] = (struct gpiod_lookup) + GPIO_LOOKUP(chipname, 3, "wp", 0); + lookup->table[1] = (struct gpiod_lookup) + GPIO_LOOKUP(chipname, 4, "cd", GPIO_ACTIVE_LOW); gpiod_add_lookup_table(lookup); } -- 2.17.1
[PATCH v6 2/8] i2c: i801: Use GPIO_LOOKUP() helper macro
i801_add_mux() fills in the GPIO lookup table by manually populating an
array of gpiod_lookup structures. Use the existing GPIO_LOOKUP() helper
macro instead, to relax a dependency on the gpiod_lookup structure's
member names.
Signed-off-by: Geert Uytterhoeven
Cc: Jean Delvare
Cc: linux-...@vger.kernel.org
---
While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add
support for GPIO lookup by line name", it can be applied independently.
But an Acked-by would be nice, too.
Cover letter and full series at
https://lore.kernel.org/r/20200324135328.5796-1-geert+rene...@glider.be/
v6:
- New.
---
drivers/i2c/busses/i2c-i801.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ca4f096fef749302..8e64a71bea684cc7 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1444,9 +1444,9 @@ static int i801_add_mux(struct i801_priv *priv)
return -ENOMEM;
lookup->dev_id = "i2c-mux-gpio";
for (i = 0; i < mux_config->n_gpios; i++) {
- lookup->table[i].chip_label = mux_config->gpio_chip;
- lookup->table[i].chip_hwnum = mux_config->gpios[i];
- lookup->table[i].con_id = "mux";
+ lookup->table[i] = (struct gpiod_lookup)
+ GPIO_LOOKUP(mux_config->gpio_chip,
+ mux_config->gpios[i], "mux", 0);
}
gpiod_add_lookup_table(lookup);
priv->lookup = lookup;
--
2.17.1
[PATCH v6 7/8] docs: gpio: Add GPIO Aggregator documentation
Document the GPIO Aggregator, and the two typical use-cases.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
Reviewed-by: Eugeniu Rosca
Tested-by: Eugeniu Rosca
---
v6:
- Fix "allows" without object:
-> provides a mechanism to aggregate GPIOs,
-> provides access control for a set of one or more GPIOs,
-> allows the user to communicate,
- Drop "gpiochipN" support,
- Extend example,
v5:
- Add Reviewed-by, Tested-by,
- Fix inconsistent indentation,
v4:
- Add Reviewed-by,
- Drop controversial GPIO repeater,
- Clarify industrial control use case,
- Fix typo s/communicated/communicate/,
- Replace abstract frobnicator example by concrete door example with
gpio-line-names,
v3:
- New.
---
.../admin-guide/gpio/gpio-aggregator.rst | 111 ++
Documentation/admin-guide/gpio/index.rst | 1 +
2 files changed, 112 insertions(+)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst
b/Documentation/admin-guide/gpio/gpio-aggregator.rst
new file mode 100644
index ..5cd1e7221756504c
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -0,0 +1,111 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Aggregator
+===
+
+The GPIO Aggregator provides a mechanism to aggregate GPIOs, and expose them as
+a new gpio_chip. This supports the following use cases.
+
+
+Aggregating GPIOs using Sysfs
+-
+
+GPIO controllers are exported to userspace using /dev/gpiochip* character
+devices. Access control to these devices is provided by standard UNIX file
+system permissions, on an all-or-nothing basis: either a GPIO controller is
+accessible for a user, or it is not.
+
+The GPIO Aggregator provides access control for a set of one or more GPIOs, by
+aggregating them into a new gpio_chip, which can be assigned to a group or user
+using standard UNIX file ownership and permissions. Furthermore, this
+simplifies and hardens exporting GPIOs to a virtual machine, as the VM can just
+grab the full GPIO controller, and no longer needs to care about which GPIOs to
+grab and which not, reducing the attack surface.
+
+Aggregated GPIO controllers are instantiated and destroyed by writing to
+write-only attribute files in sysfs.
+
+/sys/bus/platform/drivers/gpio-aggregator/
+
+ "new_device" ...
+ Userspace may ask the kernel to instantiate an aggregated GPIO
+ controller by writing a string describing the GPIOs to
+ aggregate to the "new_device" file, using the format
+
+ .. code-block:: none
+
+ [] [ ] ...
+
+ Where:
+
+ "" ...
+ is a GPIO line name,
+
+ "" ...
+ is a GPIO chip label, and
+
+ "" ...
+ is a comma-separated list of GPIO offsets and/or
+ GPIO offset ranges denoted by dashes.
+
+ Example: Instantiate a new GPIO aggregator by aggregating GPIO
+ line 19 of "e6052000.gpio" and GPIO lines 20-21 of
+ "e605.gpio" into a new gpio_chip:
+
+ .. code-block:: sh
+
+ $ echo 'e6052000.gpio 19 e605.gpio 20-21' > new_device
+
+ "delete_device" ...
+ Userspace may ask the kernel to destroy an aggregated GPIO
+ controller after use by writing its device name to the
+ "delete_device" file.
+
+ Example: Destroy the previously-created aggregated GPIO
+ controller, assumed to be "gpio-aggregator.0":
+
+ .. code-block:: sh
+
+ $ echo gpio-aggregator.0 > delete_device
+
+
+Generic GPIO Driver
+---
+
+The GPIO Aggregator can also be used as a generic driver for a simple
+GPIO-operated device described in DT, without a dedicated in-kernel driver.
+This is useful in industrial control, and is not unlike e.g. spidev, which
+allows the user to communicate with an SPI device from userspace.
+
+Binding a device to the GPIO Aggregator is performed either by modifying the
+gpio-aggregator driver, or by writing to the "driver_override" file in Sysfs.
+
+Example: If "door" is a GPIO-operated device described in DT, using its own
+compatible value::
+
+ door {
+ compatible = "myvendor,mydoor";
+
+ gpios = < 19 GPIO_ACTIVE_HIGH>,
+ < 20 GPIO_ACTIVE_LOW>;
+ gpio-line-names = "open", "lock";
+ };
+
+it can be bound to the GPIO Aggregator by either:
+
+1. Adding its compatible value to ``gpio_aggregato
[PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name
Currently a GPIO lookup table can only refer to a specific GPIO by a
tuple, consisting of a GPIO controller label and a GPIO offset inside
the controller.
However, a GPIO may also carry a line name, defined by DT or ACPI.
If present, the line name is the most use-centric way to refer to a
GPIO. Hence add support for looking up GPIOs by line name.
Implement this by reusing the existing gpiod_lookup infrastructure.
Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
that this field can have two meanings, and update the kerneldoc and
GPIO_LOOKUP*() macros.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
Reviewed-by: Eugeniu Rosca
Tested-by: Eugeniu Rosca
---
v6:
- Update Documentation/driver-api/gpio/board.rst,
- Reword rationale,
v5:
- Add Reviewed-by, Tested-by,
v4:
- Add Reviewed-by,
- Rename gpiod_lookup.chip_label.
- Use U16_MAX instead of (u16)-1,
v3:
- New.
---
Documentation/driver-api/gpio/board.rst | 10 ++
drivers/gpio/gpiolib.c | 22 +-
include/linux/gpio/machine.h| 15 ---
3 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/Documentation/driver-api/gpio/board.rst
b/Documentation/driver-api/gpio/board.rst
index ce91518bf9f48ded..0ad1f8cacf5e5d26 100644
--- a/Documentation/driver-api/gpio/board.rst
+++ b/Documentation/driver-api/gpio/board.rst
@@ -113,13 +113,15 @@ files that desire to do so need to include the following
header::
GPIOs are mapped by the means of tables of lookups, containing instances of the
gpiod_lookup structure. Two macros are defined to help declaring such
mappings::
- GPIO_LOOKUP(chip_label, chip_hwnum, con_id, flags)
- GPIO_LOOKUP_IDX(chip_label, chip_hwnum, con_id, idx, flags)
+ GPIO_LOOKUP(key, chip_hwnum, con_id, flags)
+ GPIO_LOOKUP_IDX(key, chip_hwnum, con_id, idx, flags)
where
- - chip_label is the label of the gpiod_chip instance providing the GPIO
- - chip_hwnum is the hardware number of the GPIO within the chip
+ - key is either the label of the gpiod_chip instance providing the GPIO, or
+the GPIO line name
+ - chip_hwnum is the hardware number of the GPIO within the chip, or U16_MAX
+to indicate that key is a GPIO line name
- con_id is the name of the GPIO function from the device point of view. It
can be NULL, in which case it will match any function.
- idx is the index of the GPIO within the function.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8d7366f4451fe695..c756602e249c052e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4643,7 +4643,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (!table)
return desc;
- for (p = >table[0]; p->chip_label; p++) {
+ for (p = >table[0]; p->key; p++) {
struct gpio_chip *chip;
/* idx must always match exactly */
@@ -4654,18 +4654,30 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
continue;
- chip = find_chip_by_name(p->chip_label);
+ if (p->chip_hwnum == U16_MAX) {
+ desc = gpio_name_to_desc(p->key);
+ if (desc) {
+ *flags = p->flags;
+ return desc;
+ }
+
+ dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+p->key);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ chip = find_chip_by_name(p->key);
if (!chip) {
/*
* As the lookup table indicates a chip with
-* p->chip_label should exist, assume it may
+* p->key should exist, assume it may
* still appear later and let the interested
* consumer be probed again or let the Deferred
* Probe infrastructure handle the error.
*/
dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
-p->chip_label);
+p->key);
return ERR_PTR(-EPROBE_DEFER);
}
@@ -4696,7 +4708,7 @@ static int platform_gpio_count(struct device *dev, const
char *con_id)
if (!table)
return -ENOENT;
- for (p = >table[0]; p->chip_label; p++) {
+ for (p = >table[0]; p->key; p++) {
if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
(!con_id && !p->con_id))
[PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
The GPIO Aggregator will need a method to forward a .set_config() call
to its parent gpiochip. This requires obtaining the gpio_chip and
offset for a given gpio_desc. While gpiod_to_chip() is public,
gpio_chip_hwgpio() is not, so there is currently no method to obtain the
needed GPIO offset parameter.
Hence introduce a public gpiod_set_config() helper, which invokes the
.set_config() callback through a gpio_desc pointer, like is done for
most other gpio_chip callbacks.
Rewrite the existing gpiod_set_debounce() helper as a wrapper around
gpiod_set_config(), to avoid duplication.
Signed-off-by: Geert Uytterhoeven
---
v6:
- New.
---
drivers/gpio/gpiolib.c| 28 ++--
include/linux/gpio/consumer.h | 8
2 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c756602e249c052e..30ea75e972b5a3b1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3478,6 +3478,26 @@ int gpiod_direction_output(struct gpio_desc *desc, int
value)
}
EXPORT_SYMBOL_GPL(gpiod_direction_output);
+/**
+ * gpiod_set_config - sets @config for a GPIO
+ * @desc: descriptor of the GPIO for which to set the configuration
+ * @config: Same packed config format as generic pinconf
+ *
+ * Returns:
+ * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
+ * configuration.
+ */
+int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
+{
+ struct gpio_chip *chip;
+
+ VALIDATE_DESC(desc);
+ chip = desc->gdev->chip;
+
+ return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_config);
+
/**
* gpiod_set_debounce - sets @debounce time for a GPIO
* @desc: descriptor of the GPIO for which to set debounce time
@@ -3489,14 +3509,10 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output);
*/
int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
{
- struct gpio_chip*chip;
- unsigned long config;
-
- VALIDATE_DESC(desc);
- chip = desc->gdev->chip;
+ unsigned long config;
config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
- return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config);
+ return gpiod_set_config(desc, config);
}
EXPORT_SYMBOL_GPL(gpiod_set_debounce);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 0a72fccf60fff230..901aab89d025f3ff 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -157,6 +157,7 @@ int gpiod_set_raw_array_value_cansleep(unsigned int
array_size,
struct gpio_array *array_info,
unsigned long *value_bitmap);
+int gpiod_set_config(struct gpio_desc *desc, unsigned long config);
int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
void gpiod_toggle_active_low(struct gpio_desc *desc);
@@ -473,6 +474,13 @@ static inline int
gpiod_set_raw_array_value_cansleep(unsigned int array_size,
return 0;
}
+static inline int gpiod_set_config(struct gpio_desc *desc, unsigned long
config)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(desc);
+ return -ENOSYS;
+}
+
static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
{
/* GPIO can never have been requested */
--
2.17.1
[PATCH v6 8/8] MAINTAINERS: Add GPIO Aggregator section
Add a maintainership section for the GPIO Aggregator, covering documentation and driver source code. Signed-off-by: Geert Uytterhoeven Reviewed-by: Eugeniu Rosca Tested-by: Eugeniu Rosca --- v6: - No changes, v5: - Add Reviewed-by, Tested-by, v4: - Drop controversial GPIO repeater, v3: - New. --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index fcd79fc38928fafc..1fad69b956df1162 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7127,6 +7127,13 @@ F: Documentation/firmware-guide/acpi/gpio-properties.rst F: drivers/gpio/gpiolib-acpi.c F: drivers/gpio/gpiolib-acpi.h +GPIO AGGREGATOR +M: Geert Uytterhoeven +L: linux-g...@vger.kernel.org +S: Maintained +F: Documentation/admin-guide/gpio/gpio-aggregator.rst +F: drivers/gpio/gpio-aggregator.c + GPIO IR Transmitter M: Sean Young L: linux-me...@vger.kernel.org -- 2.17.1
[PATCH v6 0/8] gpio: Add GPIO Aggregator
gpioset gpiochip12 0=1 1=0 # LED6 ON, LED7 OFF
$ gpioset gpiochip13 0=1 # LED8 ON
$ gpioset gpiochip13 0=0 # LED8 OFF
- Destroy aggregators:
$ echo gpio-aggregator.0 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
$ echo gpio-aggregator.1 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
To ease testing, I have pushed this series to the
topic/gpio-aggregator-v6 branch of my renesas-drivers repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
Kbuild test robot reported no issues.
Thanks!
References:
[1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
(https://lore.kernel.org/r/1561699236-18620-3-git-send-email-harish_kand...@mentor.com/)
[2] "[PATCH v5 0/5] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/r/20200218151812.7816-1-geert+rene...@glider.be/)
[3] "[PATCH v4 0/5] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/r/20200115181523.23556-1-geert+rene...@glider.be)
[4] "[PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater"
(https://lore.kernel.org/r/20191127084253.16356-1-geert+rene...@glider.be/)
[5] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
(https://lore.kernel.org/r/20190911143858.13024-1-geert+rene...@glider.be/)
[6] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
(https://lore.kernel.org/r/20190705160536.12047-1-geert+rene...@glider.be/)
[7] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/r/20181003152521.23144-1-geert+rene...@glider.be/)
[8] "Getting To Blinky: Virt Edition / Making device pass-through
work on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
Geert Uytterhoeven (8):
ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro
i2c: i801: Use GPIO_LOOKUP() helper macro
mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
gpiolib: Add support for GPIO lookup by line name
gpiolib: Introduce gpiod_set_config()
gpio: Add GPIO Aggregator
docs: gpio: Add GPIO Aggregator documentation
MAINTAINERS: Add GPIO Aggregator section
.../admin-guide/gpio/gpio-aggregator.rst | 111
Documentation/admin-guide/gpio/index.rst | 1 +
Documentation/driver-api/gpio/board.rst | 10 +-
MAINTAINERS | 7 +
arch/arm/mach-integrator/impd1.c | 11 +-
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-aggregator.c| 568 ++
drivers/gpio/gpiolib.c| 50 +-
drivers/i2c/busses/i2c-i801.c | 6 +-
drivers/mfd/sm501.c | 24 +-
include/linux/gpio/consumer.h | 8 +
include/linux/gpio/machine.h | 15 +-
13 files changed, 776 insertions(+), 48 deletions(-)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
create mode 100644 drivers/gpio/gpio-aggregator.c
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v5 3/5] gpio: Add GPIO Aggregator
On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven
wrote:
> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices. Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
>
> Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
> a new gpiochip.
>
> This supports the following use cases:
> - Aggregating GPIOs using Sysfs
> This is useful for implementing access control, and assigning a set
> of GPIOs to a specific user or virtual machine.
> - Generic GPIO Driver
> This is useful for industrial control, where it can provide
> userspace access to a simple GPIO-operated device described in DT,
> cfr. e.g. spidev for SPI-operated devices.
>
> Signed-off-by: Geert Uytterhoeven
> --- /dev/null
> +++ b/drivers/gpio/gpio-aggregator.c
> +static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> + unsigned long config)
> +{
> + struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> + chip = fwd->descs[offset]->gdev->chip;
> + if (chip->set_config)
- chip = fwd->descs[offset]->gdev->chip;
- if (chip->set_config)
+ chip = gpiod_to_chip(fwd->descs[offset]);
+ if (chip && chip->set_config)
> + return chip->set_config(chip, offset, config);
This is not correct: offset should be translated, too, i.e.
offset = gpio_chip_hwgpio(fwd->descs[offset]);
Which adds a new dependency on "gpiolib.h"...
Is there a better alternative, than providing a public gpiod_set_config()
helper?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v5 3/5] gpio: Add GPIO Aggregator
Hi Linus,
On Thu, Mar 12, 2020 at 3:57 PM Linus Walleij wrote:
> thanks for your patience and again sorry for procrastination on my part :(
>
> Overall I start to like this driver a lot. It has come a long way.
>
> Some comments below are nitpicky, bear with me if they seem stupid.
Thanks a lot for your comments!
> On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven
> wrote:
> > +#define DRV_NAME "gpio-aggregator"
> > +#define pr_fmt(fmt)DRV_NAME ": " fmt
>
> I would just use dev_[info|err] for all messages to get rid of this.
See below.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "gpiolib.h"
>
> When this file is includes I prefer if there is a comment next to
> this include saying why we have to touch internals and which
> ones.
I have just discovered gpiod_to_chip(), which removes the need for two
of the three users ;-)
> > +struct gpio_aggregator {
> > + struct gpiod_lookup_table *lookups;
> > + struct platform_device *pdev;
>
> What about just storing struct device *dev?
>
> Then callbacks can just
>
> dev_err(aggregator->dev, "myerror\n");
>pdev.dev or aggr->dev does't make much of a difference.
> > +static char *get_arg(char **args)
> > +{
> > + char *start = *args, *end;
> > +
> > + start = skip_spaces(start);
> > + if (!*start)
> > + return NULL;
> > +
> > + if (*start == '"') {
> > + /* Quoted arg */
> > + end = strchr(++start, '"');
> > + if (!end)
> > + return ERR_PTR(-EINVAL);
> > + } else {
> > + /* Unquoted arg */
> > + for (end = start; *end && !isspace(*end); end++) ;
> > + }
> > +
> > + if (*end)
> > + *end++ = '\0';
> > +
> > + *args = end;
> > + return start;
> > +}
>
> Isn't this function reimplementing strsep()?
> while ((s = strsep(, " \""))) {
> or something.
>
> I'm not the best with strings, just asking so I know you tried it
> already.
strsep(, " \"") would terminate the token if a space or double quote is
seen. I.e. it wouldn't handle spaces in quoted arguments.
There's also argv_split(), but that doesn't handle quoted args, and
duplicates all arguments.
Line names assigned by "gpio-lines-names" may contain spaces, so support
for quoted args is mandatory.
> > +static int aggr_parse(struct gpio_aggregator *aggr)
> > +{
> > + unsigned int first_index, last_index, i, n = 0;
> > + char *name, *offsets, *first, *last, *next;
> > + char *args = aggr->args;
> > + int error;
> > +
> > + for (name = get_arg(), offsets = get_arg(); name;
> > +offsets = get_arg()) {
> > + if (IS_ERR(name)) {
> > + pr_err("Cannot get GPIO specifier: %pe\n", name);
>
> If gpio_aggregrator contained struct device *dev this would be
> dev_err(aggr->dev, "...\n");
aggr_parse() is called before the platform device is created, and before
aggr->pdev is populated. So there is no device to print yet.
> > +static void gpio_aggregator_free(struct gpio_aggregator *aggr)
> > +{
> > + platform_device_unregister(aggr->pdev);
>
> Aha maybe store both the pdev and the dev in the struct then?
>
> Or print using >pdev.dev.
Same for aggr->pdev.dev (or aggr->dev).
> > + /*
> > +* If any of the GPIO lines are sleeping, then the entire forwarder
> > +* will be sleeping.
> > +* If any of the chips support .set_config(), then the forwarder
> > will
> > +* support setting configs.
> > +*/
> > + for (i = 0; i < ngpios; i++) {
> > + dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> > + desc_to_gpio(descs[i]), descs[i]->label ? : "?");
>
> If this desc->label business is why you need to include
> "gpiolib.h" then I'd prefer if you just add a
It was the third reason to include that file...
> const char *gpiod_get_producer_name(struct gpio_desc *desc);
>
> to gpiolib (add in so that gpiolib can
> try to give you something reasonable to print for the label
Re: [PATCH v5 2/5] gpiolib: Add support for GPIO line table lookup
Hi Linus,
On Thu, Mar 12, 2020 at 3:21 PM Linus Walleij wrote:
> On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven
> wrote:
> > Currently GPIOs can only be referred to by GPIO controller and offset in
> > GPIO lookup tables.
> >
> > Add support for looking them up by line name.
> > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
> > that this field can have two meanings, and update the kerneldoc and
> > GPIO_LOOKUP*() macros.
> >
> > Signed-off-by: Geert Uytterhoeven
> > Reviewed-by: Ulrich Hecht
> > Reviewed-by: Eugeniu Rosca
> > Tested-by: Eugeniu Rosca
>
> I will try to understand why this change is necessary to implement
> the gpio aggregator (probablt I will comment that on the other
> patches like "aha now I see it" or so, but it would help a lot if the
> commit message
> would state the technical reason to why we need to do this change,
> like what it is that you want to do and why you cannot do it without
> this change.
It's very simple: how do you want the user to refer to a specific GPIO
line? Currently he can only do so by gpiochip label and index.
However, there exists another stable reference: the (optional) line name,
which can be attached using "gpio-line-names" in DT or ACPI.
As this is the most use-centric way to refer to a GPIO, it makes sense
to support lookup based on that, too.
Will reword to make this clearer.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v5 1/5] gpiolib: Add support for gpiochipN-based table lookup
Hi Linus,
On Thu, Mar 12, 2020 at 3:23 PM Linus Walleij wrote:
> On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven
> wrote:
>
> > Currently GPIO controllers can only be referred to by label in GPIO
> > lookup tables.
> >
> > Add support for looking them up by "gpiochipN" name, with "N" the
> > corresponding GPIO device's ID number.
> >
> > Signed-off-by: Geert Uytterhoeven
> > Reviewed-by: Ulrich Hecht
> > Reviewed-by: Eugeniu Rosca
> > Tested-by: Eugeniu Rosca
>
> Just like with patch 2/5 I have the same problem here that
> the commit message doesn't state the technical reason why
> we need to change this and support the device name in these
> tables and not just labels.
As these "gpiochipN" names are not stable, I will drop this patch, and
the related support.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v5 0/5] gpio: Add GPIO Aggregator
Hi Linus and Bartosz,
On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven
wrote:
> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices. Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
>
> Hence this adds a GPIO driver to aggregate existing GPIOs, and expose
> them as a new gpiochip. This is useful for implementing access control,
> and assigning a set of GPIOs to a specific user. Furthermore, this
> simplifies and hardens exporting GPIOs to a virtual machine, as the VM
> can just grab the full GPIO controller, and no longer needs to care
> about which GPIOs to grab and which not, reducing the attack surface.
Do you have any more comments, before I respin and post v6?
Thanks, and have a niec weekend!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v5 4/5] docs: gpio: Add GPIO Aggregator documentation
Hi Randy,
On Tue, Feb 18, 2020 at 7:30 PM Randy Dunlap wrote:
> On 2/18/20 7:18 AM, Geert Uytterhoeven wrote:
> > Document the GPIO Aggregator, and the two typical use-cases.
> >
> > Signed-off-by: Geert Uytterhoeven
> > --- /dev/null
> > +++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
> > @@ -0,0 +1,102 @@
> > +.. SPDX-License-Identifier: GPL-2.0-only
> > +
> > +GPIO Aggregator
> > +===
> > +
> > +The GPIO Aggregator allows to aggregate GPIOs, and expose them as a new
>
> "allows" really wants an object following the verb [although the kernel
> sources
> and docs have many cases of it not having an object]. Something like
>
>allows {you, one, someone, users, a user} to aggregate
Changing to:
provides a mechanism to aggregate GPIOs
> > +gpio_chip. This supports the following use cases.
> > +
> > +
> > +Aggregating GPIOs using Sysfs
> > +-
> > +
> > +GPIO controllers are exported to userspace using /dev/gpiochip* character
> > +devices. Access control to these devices is provided by standard UNIX file
> > +system permissions, on an all-or-nothing basis: either a GPIO controller is
> > +accessible for a user, or it is not.
> > +
> > +The GPIO Aggregator allows access control for individual GPIOs, by
> > aggregating
Changing to:
provides access control for a set of one or more GPIOs
> > +them into a new gpio_chip, which can be assigned to a group or user using
> > +standard UNIX file ownership and permissions. Furthermore, this
> > simplifies and
> > +hardens exporting GPIOs to a virtual machine, as the VM can just grab the
> > full
> > +GPIO controller, and no longer needs to care about which GPIOs to grab and
> > +which not, reducing the attack surface.
> > +Generic GPIO Driver
> > +---
> > +
> > +The GPIO Aggregator can also be used as a generic driver for a simple
> > +GPIO-operated device described in DT, without a dedicated in-kernel driver.
> > +This is useful in industrial control, and is not unlike e.g. spidev, which
> > +allows to communicate with an SPI device from userspace.
>
>allows {choose an object} to communicate
Changing to:
allows the user to communicate
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v5 2/5] gpiolib: Add support for GPIO line table lookup
On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven
wrote:
> Currently GPIOs can only be referred to by GPIO controller and offset in
> GPIO lookup tables.
>
> Add support for looking them up by line name.
> Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
> that this field can have two meanings, and update the kerneldoc and
> GPIO_LOOKUP*() macros.
>
> Signed-off-by: Geert Uytterhoeven
> Reviewed-by: Ulrich Hecht
> Reviewed-by: Eugeniu Rosca
> Tested-by: Eugeniu Rosca
> --- a/include/linux/gpio/machine.h
> +++ b/include/linux/gpio/machine.h
> @@ -20,8 +20,9 @@ enum gpio_lookup_flags {
>
> /**
> * struct gpiod_lookup - lookup table
> - * @chip_label: name of the chip the GPIO belongs to
> - * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
> + * @key: either the name of the chip the GPIO belongs to, or the GPIO line
> name
> + * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO, or
> + * U16_MAX to indicate that @key is a GPIO line name
> * @con_id: name of the GPIO from the device's point of view
> * @idx: index of the GPIO in case several GPIOs share the same name
> * @flags: bitmask of gpio_lookup_flags GPIO_* values
> @@ -30,7 +31,7 @@ enum gpio_lookup_flags {
> * functions using platform data.
> */
> struct gpiod_lookup {
> - const char *chip_label;
> + const char *key;
> u16 chip_hwnum;
> const char *con_id;
> unsigned int idx;
This needs an update in the documentation:
--- a/Documentation/driver-api/gpio/board.rst
+++ b/Documentation/driver-api/gpio/board.rst
@@ -113,13 +113,15 @@ files that desire to do so need to include the
following header::
GPIOs are mapped by the means of tables of lookups, containing instances of the
gpiod_lookup structure. Two macros are defined to help declaring such
mappings::
- GPIO_LOOKUP(chip_label, chip_hwnum, con_id, flags)
- GPIO_LOOKUP_IDX(chip_label, chip_hwnum, con_id, idx, flags)
+ GPIO_LOOKUP(key, chip_hwnum, con_id, flags)
+ GPIO_LOOKUP_IDX(key, chip_hwnum, con_id, idx, flags)
where
- - chip_label is the label of the gpiod_chip instance providing the GPIO
- - chip_hwnum is the hardware number of the GPIO within the chip
+ - key is either the label of the gpiod_chip instance providing the GPIO, or
+the GPIO line name
+ - chip_hwnum is the hardware number of the GPIO within the chip, or U16_MAX
+to indicate that key is a GPIO line name
- con_id is the name of the GPIO function from the device point of view. It
can be NULL, in which case it will match any function.
- idx is the index of the GPIO within the function.
Furthermore, a few drivers populate the gpiod_lookup members directly,
instead of using the convenience macros:
arch/arm/mach-integrator/impd1.c
drivers/i2c/busses/i2c-i801.c
drivers/mfd/sm501.c
Either they have to be updated s/chip_label/key/, or start using the macros,
e.g.
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1444,9 +1444,9 @@ static int i801_add_mux(struct i801_priv *priv)
return -ENOMEM;
lookup->dev_id = "i2c-mux-gpio";
for (i = 0; i < mux_config->n_gpios; i++) {
- lookup->table[i].chip_label = mux_config->gpio_chip;
- lookup->table[i].chip_hwnum = mux_config->gpios[i];
- lookup->table[i].con_id = "mux";
+ lookup->table[i] = (struct gpiod_lookup)
+ GPIO_LOOKUP(mux_config->gpio_chip,
+ mux_config->gpios[i], "mux", 0);
}
gpiod_add_lookup_table(lookup);
priv->lookup = lookup;
Do you have any preference?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v5 4/5] docs: gpio: Add GPIO Aggregator documentation
Hi Randy,
On Tue, Feb 18, 2020 at 7:30 PM Randy Dunlap wrote:
> On 2/18/20 7:18 AM, Geert Uytterhoeven wrote:
> > Document the GPIO Aggregator, and the two typical use-cases.
> >
> > Signed-off-by: Geert Uytterhoeven
> > --- /dev/null
> > +++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
> > @@ -0,0 +1,102 @@
> > +.. SPDX-License-Identifier: GPL-2.0-only
> > +
> > +GPIO Aggregator
> > +===
> > +
> > +The GPIO Aggregator allows to aggregate GPIOs, and expose them as a new
>
> "allows" really wants an object following the verb [although the kernel
> sources
> and docs have many cases of it not having an object]. Something like
>
>allows {you, one, someone, users, a user} to aggregate
Thanks for the hint!
> > + Example: Instantiate a new GPIO aggregator by aggregating GPIO
> > + 19 of "e6052000.gpio" and GPIOs 20-21 of "gpiochip2" into a
> > new
> > + gpio_chip:
> > +
> > + .. code-block:: bash
> > +
> > + echo 'e6052000.gpio 19 gpiochip2 20-21' > new_device
> > +
>
> Does the above command tell the user that the new device is named
> "gpio-aggregator.0", as used below?
Yes, it will be printed through the kernel log, cfr. the sample session in
the cover letter.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v5 0/5] gpio: Add GPIO Aggregator
6-18620-3-git-send-email-harish_kand...@mentor.com/)
[2] "[PATCH v4 0/5] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/r/20200115181523.23556-1-geert+rene...@glider.be)
[3] "[PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater"
(https://lore.kernel.org/r/20191127084253.16356-1-geert+rene...@glider.be/)
[4] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
(https://lore.kernel.org/r/20190911143858.13024-1-geert+rene...@glider.be/)
[5] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
(https://lore.kernel.org/r/20190705160536.12047-1-geert+rene...@glider.be/)
[6] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/r/20181003152521.23144-1-geert+rene...@glider.be/)
[7] "Getting To Blinky: Virt Edition / Making device pass-through
work on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
Geert Uytterhoeven (5):
gpiolib: Add support for gpiochipN-based table lookup
gpiolib: Add support for GPIO line table lookup
gpio: Add GPIO Aggregator
docs: gpio: Add GPIO Aggregator documentation
MAINTAINERS: Add GPIO Aggregator section
.../admin-guide/gpio/gpio-aggregator.rst | 102
Documentation/admin-guide/gpio/index.rst | 1 +
MAINTAINERS | 7 +
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-aggregator.c| 574 ++
drivers/gpio/gpiolib.c| 33 +-
include/linux/gpio/machine.h | 15 +-
8 files changed, 732 insertions(+), 13 deletions(-)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
create mode 100644 drivers/gpio/gpio-aggregator.c
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v5 4/5] docs: gpio: Add GPIO Aggregator documentation
Document the GPIO Aggregator, and the two typical use-cases.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
Reviewed-by: Eugeniu Rosca
Tested-by: Eugeniu Rosca
---
v5:
- Add Reviewed-by, Tested-by,
- Fix inconsistent indentation.
v4:
- Add Reviewed-by,
- Drop controversial GPIO repeater,
- Clarify industrial control use case,
- Fix typo s/communicated/communicate/,
- Replace abstract frobnicator example by concrete door example with
gpio-line-names,
v3:
- New.
---
.../admin-guide/gpio/gpio-aggregator.rst | 102 ++
Documentation/admin-guide/gpio/index.rst | 1 +
2 files changed, 103 insertions(+)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst
b/Documentation/admin-guide/gpio/gpio-aggregator.rst
new file mode 100644
index ..114f72be33c2571e
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -0,0 +1,102 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Aggregator
+===
+
+The GPIO Aggregator allows to aggregate GPIOs, and expose them as a new
+gpio_chip. This supports the following use cases.
+
+
+Aggregating GPIOs using Sysfs
+-
+
+GPIO controllers are exported to userspace using /dev/gpiochip* character
+devices. Access control to these devices is provided by standard UNIX file
+system permissions, on an all-or-nothing basis: either a GPIO controller is
+accessible for a user, or it is not.
+
+The GPIO Aggregator allows access control for individual GPIOs, by aggregating
+them into a new gpio_chip, which can be assigned to a group or user using
+standard UNIX file ownership and permissions. Furthermore, this simplifies and
+hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
+GPIO controller, and no longer needs to care about which GPIOs to grab and
+which not, reducing the attack surface.
+
+Aggregated GPIO controllers are instantiated and destroyed by writing to
+write-only attribute files in sysfs.
+
+/sys/bus/platform/drivers/gpio-aggregator/
+
+ "new_device" ...
+ Userspace may ask the kernel to instantiate an aggregated GPIO
+ controller by writing a string describing the GPIOs to
+ aggregate to the "new_device" file, using the format
+
+ .. code-block:: none
+
+ [] [ ] ...
+
+ Where:
+
+ "" ...
+ is a GPIO line name,
+
+ "" ...
+ is a GPIO chip label or name, and
+
+ "" ...
+ is a comma-separated list of GPIO offsets and/or
+ GPIO offset ranges denoted by dashes.
+
+ Example: Instantiate a new GPIO aggregator by aggregating GPIO
+ 19 of "e6052000.gpio" and GPIOs 20-21 of "gpiochip2" into a new
+ gpio_chip:
+
+ .. code-block:: bash
+
+ echo 'e6052000.gpio 19 gpiochip2 20-21' > new_device
+
+ "delete_device" ...
+ Userspace may ask the kernel to destroy an aggregated GPIO
+ controller after use by writing its device name to the
+ "delete_device" file.
+
+ Example: Destroy the previously-created aggregated GPIO
+ controller "gpio-aggregator.0":
+
+ .. code-block:: bash
+
+ echo gpio-aggregator.0 > delete_device
+
+
+Generic GPIO Driver
+---
+
+The GPIO Aggregator can also be used as a generic driver for a simple
+GPIO-operated device described in DT, without a dedicated in-kernel driver.
+This is useful in industrial control, and is not unlike e.g. spidev, which
+allows to communicate with an SPI device from userspace.
+
+Binding a device to the GPIO Aggregator is performed either by modifying the
+gpio-aggregator driver, or by writing to the "driver_override" file in Sysfs.
+
+Example: If "door" is a GPIO-operated device described in DT, using its own
+compatible value::
+
+ door {
+ compatible = "myvendor,mydoor";
+
+ gpios = < 19 GPIO_ACTIVE_HIGH>,
+ < 20 GPIO_ACTIVE_LOW>;
+ gpio-line-names = "open", "lock";
+ };
+
+it can be bound to the GPIO Aggregator by either:
+
+1. Adding its compatible value to ``gpio_aggregator_dt_ids[]``,
+2. Binding manually using "driver_override":
+
+.. code-block:: bash
+
+echo gpio-aggregator > /sys/bus/platform/devices/door/driver_override
+echo door > /sys/bus/platform/drivers/gpio-aggregator/bind
diff --git a/Documentation/admin-guide/gpio/index.rst
b/Documentation/admin-guide/gpio/
[PATCH v5 3/5] gpio: Add GPIO Aggregator
GPIO controllers are exported to userspace using /dev/gpiochip* character devices. Access control to these devices is provided by standard UNIX file system permissions, on an all-or-nothing basis: either a GPIO controller is accessible for a user, or it is not. Currently no mechanism exists to control access to individual GPIOs. Hence add a GPIO driver to aggregate existing GPIOs, and expose them as a new gpiochip. This supports the following use cases: - Aggregating GPIOs using Sysfs This is useful for implementing access control, and assigning a set of GPIOs to a specific user or virtual machine. - Generic GPIO Driver This is useful for industrial control, where it can provide userspace access to a simple GPIO-operated device described in DT, cfr. e.g. spidev for SPI-operated devices. Signed-off-by: Geert Uytterhoeven Reviewed-by: Eugeniu Rosca Tested-by: Eugeniu Rosca --- v5: - Add Reviewed-by, Tested-by, v4: - Remove unused assignment to n in isrange(), - Check correct pointer after aggr->lookups->dev_id allocation, - Preinitialize flags to 0 in gpio_fwd_[gs]et_multiple() to avoid may-be-used-uninitialized warning, - Drop controversial GPIO repeater, - Update for gpiod_lookup.chip_label rename, - Use %pe to format error pointers, - Use U16_MAX instead of (u16)-1, - Correct comment indentation, - Use skip_spaces() helper, - Rename a and b to first_index resp. last_index, - Add comment to tmp[] use, - Improve Kconfig help text, - Include for gpiod_[gs]et_*(), - Drop unneeded valid_mask handling, - Add comment about sleeping and .set_config() support, v3: - Absorb GPIO forwarder, - Integrate GPIO Repeater and Generic GPIO driver functionality, - Use the aggregator parameters to create a GPIO lookup table instead of an array of GPIO descriptors, which allows to simplify the code: 1. This removes the need for calling gpio_name_to_desc(), gpiochip_find(), gpiochip_get_desc(), and gpiod_request(), 2. This allows the platform device to always use devm_gpiod_get_index(), regardless of the origin of the GPIOs, - Move parameter parsing from platform device probe to sysfs attribute store, removing the need for platform data passing, - Use more devm_*() functions to simplify cleanup, - Add pr_fmt(), - General refactoring, v2: - Add missing initialization of i in gpio_virt_agg_probe(), - Update for removed .need_valid_mask field and changed .init_valid_mask() signature, - Drop "virtual", rename to gpio-aggregator, - Drop bogus FIXME related to gpiod_set_transitory() expectations, - Use new GPIO Forwarder Helper, - Lift limit on the maximum number of GPIOs, - Improve parsing: - add support for specifying GPIOs by line name, - add support for specifying GPIO chips by ID, - add support for GPIO offset ranges, - names and offset specifiers must be separated by whitespace, - GPIO offsets must separated by spaces, - Use str_has_prefix() and kstrtouint(). --- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-aggregator.c | 574 + 3 files changed, 587 insertions(+) create mode 100644 drivers/gpio/gpio-aggregator.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index b8013cf90064d505..b701984fdc930aa6 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1534,6 +1534,18 @@ config GPIO_VIPERBOARD endmenu +config GPIO_AGGREGATOR + tristate "GPIO Aggregator" + help + Say yes here to enable the GPIO Aggregator, which provides a way to + aggregate existing GPIO lines into a new virtual GPIO chip. + This can serve the following purposes: + - Assign permissions for a collection of GPIO lines to a user, + - Export a collection of GPIO lines to a virtual machine, + - Provide a generic driver for a GPIO-operated device in an + industrial control context, to be operated from userspace using + the GPIO chardev interface. + config GPIO_MOCKUP tristate "GPIO Testing Driver" select IRQ_SIM diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 0b571264ddbcdb49..2a7d85a0004a6f41 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o +obj-$(CONFIG_GPIO_AGGREGATOR) += gpio-aggregator.o obj-$(CONFIG_GPIO_ALTERA_A10SR)+= gpio-altera-a10sr.o obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-agg
[PATCH v5 5/5] MAINTAINERS: Add GPIO Aggregator section
Add a maintainership section for the GPIO Aggregator, covering documentation and driver source code. Signed-off-by: Geert Uytterhoeven Reviewed-by: Eugeniu Rosca Tested-by: Eugeniu Rosca --- v5: - Add Reviewed-by, Tested-by, v4: - Drop controversial GPIO repeater, v3: - New. --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 634376400709d6e8..d39f550ab1555a87 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7128,6 +7128,13 @@ F: Documentation/firmware-guide/acpi/gpio-properties.rst F: drivers/gpio/gpiolib-acpi.c F: drivers/gpio/gpiolib-acpi.h +GPIO AGGREGATOR +M: Geert Uytterhoeven +L: linux-g...@vger.kernel.org +S: Maintained +F: Documentation/admin-guide/gpio/gpio-aggregator.rst +F: drivers/gpio/gpio-aggregator.c + GPIO IR Transmitter M: Sean Young L: linux-me...@vger.kernel.org -- 2.17.1
[PATCH v5 1/5] gpiolib: Add support for gpiochipN-based table lookup
Currently GPIO controllers can only be referred to by label in GPIO
lookup tables.
Add support for looking them up by "gpiochipN" name, with "N" the
corresponding GPIO device's ID number.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
Reviewed-by: Eugeniu Rosca
Tested-by: Eugeniu Rosca
---
v5:
- Add Reviewed-by, Tested-by,
v4:
- Add Reviewed-by,
- Drop support for legacy sysfs interface based name matching,
- Replace complex custom matching by a simple additional check in the
existing gpiochip_match_name() function,
- Add kerneldoc() for find_chip_by_name(), documenting matching order.
v3:
- New.
---
drivers/gpio/gpiolib.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4d0106ceeba7bb24..200c2d2be4b78043 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1738,9 +1738,18 @@ static int gpiochip_match_name(struct gpio_chip *chip,
void *data)
{
const char *name = data;
- return !strcmp(chip->label, name);
+ return !strcmp(chip->label, name) ||
+ !strcmp(dev_name(>gpiodev->dev), name);
}
+/**
+ * find_chip_by_name() - Find a specific gpio_chip by name
+ * @name: Name to match
+ *
+ * Return a reference to a gpio_chip that matches the passed name.
+ * This function first tries matching on the gpio_chip's label, followed by
+ * matching on dev_name() of the corresponding gpio_device.
+ */
static struct gpio_chip *find_chip_by_name(const char *name)
{
return gpiochip_find((void *)name, gpiochip_match_name);
--
2.17.1
[PATCH v5 2/5] gpiolib: Add support for GPIO line table lookup
Currently GPIOs can only be referred to by GPIO controller and offset in
GPIO lookup tables.
Add support for looking them up by line name.
Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
that this field can have two meanings, and update the kerneldoc and
GPIO_LOOKUP*() macros.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
Reviewed-by: Eugeniu Rosca
Tested-by: Eugeniu Rosca
---
v5:
- Add Reviewed-by, Tested-by,
v4:
- Add Reviewed-by,
- Rename gpiod_lookup.chip_label.
- Use U16_MAX instead of (u16)-1,
v3:
- New.
---
drivers/gpio/gpiolib.c | 22 +-
include/linux/gpio/machine.h | 15 ---
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 200c2d2be4b78043..24c02167f9e5472f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4453,7 +4453,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (!table)
return desc;
- for (p = >table[0]; p->chip_label; p++) {
+ for (p = >table[0]; p->key; p++) {
struct gpio_chip *chip;
/* idx must always match exactly */
@@ -4464,18 +4464,30 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
continue;
- chip = find_chip_by_name(p->chip_label);
+ if (p->chip_hwnum == U16_MAX) {
+ desc = gpio_name_to_desc(p->key);
+ if (desc) {
+ *flags = p->flags;
+ return desc;
+ }
+
+ dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+p->key);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ chip = find_chip_by_name(p->key);
if (!chip) {
/*
* As the lookup table indicates a chip with
-* p->chip_label should exist, assume it may
+* p->key should exist, assume it may
* still appear later and let the interested
* consumer be probed again or let the Deferred
* Probe infrastructure handle the error.
*/
dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
-p->chip_label);
+p->key);
return ERR_PTR(-EPROBE_DEFER);
}
@@ -4506,7 +4518,7 @@ static int platform_gpio_count(struct device *dev, const
char *con_id)
if (!table)
return -ENOENT;
- for (p = >table[0]; p->chip_label; p++) {
+ for (p = >table[0]; p->key; p++) {
if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
(!con_id && !p->con_id))
count++;
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 1ebe5be05d5f81fa..84c66fbf54fd5811 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -20,8 +20,9 @@ enum gpio_lookup_flags {
/**
* struct gpiod_lookup - lookup table
- * @chip_label: name of the chip the GPIO belongs to
- * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
+ * @key: either the name of the chip the GPIO belongs to, or the GPIO line name
+ * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO, or
+ * U16_MAX to indicate that @key is a GPIO line name
* @con_id: name of the GPIO from the device's point of view
* @idx: index of the GPIO in case several GPIOs share the same name
* @flags: bitmask of gpio_lookup_flags GPIO_* values
@@ -30,7 +31,7 @@ enum gpio_lookup_flags {
* functions using platform data.
*/
struct gpiod_lookup {
- const char *chip_label;
+ const char *key;
u16 chip_hwnum;
const char *con_id;
unsigned int idx;
@@ -63,17 +64,17 @@ struct gpiod_hog {
/*
* Simple definition of a single GPIO under a con_id
*/
-#define GPIO_LOOKUP(_chip_label, _chip_hwnum, _con_id, _flags) \
- GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, 0, _flags)
+#define GPIO_LOOKUP(_key, _chip_hwnum, _con_id, _flags) \
+ GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, 0, _flags)
/*
* Use this macro if you need to have several GPIOs under the same con_id.
* Each GPIO needs to use a different index and can be accessed using
* gpiod_get_index()
*/
-#define GPIO_LOOKUP_
Re: [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater
Hi Eugeniu,
On Sat, Jan 18, 2020 at 2:46 AM Eugeniu Rosca wrote:
> On Wed, Nov 27, 2019 at 09:42:46AM +0100, Geert Uytterhoeven wrote:
> > - Create aggregators:
> >
> > $ echo e6052000.gpio 19,20 \
> > > /sys/bus/platform/drivers/gpio-aggregator/new_device
> The only unexpected thing is seeing below messages (where gpiochip99 and
> gpiochip22 are inexisting gpiochip names, mistakenly provided on command
> line prior to passing the correct name):
>
> root@rcar-gen3:~# echo gpiochip6 12-13 >
> /sys/bus/platform/drivers/gpio-aggregator/new_device
> [ 915.572905] gpio-aggregator gpio-aggregator.0: cannot find GPIO chip
> gpiochip99, deferring
> [ 915.584224] gpio-aggregator gpio-aggregator.2: cannot find GPIO chip
> gpiochip99, deferring
> [ 915.865281] gpio-aggregator gpio-aggregator.29: cannot find GPIO chip
> gpiochip22, deferring
>
> Obviously, in the above case, due to a typo in the names, the gpio
> chips will never be found, no matter how long gpio-aggregator defers
Indeed, that is expected behavior: you have created platform devices
referring to resources that are not available.
> their probing. Unfortunately, the driver will continuously emit those
> messages, upon each successfully created/aggregated gpiochip. I built
That is expected behavior, too: every time the driver core manages to
bind a device to a driver, it will retry all previously deferred probes,
in the hope they can be satisfied by the just bound device.
Note that you can destroy these bogus devices, using e.g.
# echo gpio-aggregator.0 > \
/sys/bus/platform/drivers/gpio-aggregator/delete_device
> gpio-aggregator as a loadable module, if that's relevant.
Modular or non-modular shouldn't matter w.r.t. this behavior.
Although unloading the module should get rid of the cruft.
> Another comment is that, while the series _does_ allow specifying
> gpio lines in the DTS (this would require a common compatible string
> in gpio_aggregator_dt_ids[] and in the DTS node) and while those lines
> are indeed exposed to userspace, based on my testing, these same gpio
> lines are marked as "used/reserved" by the kernel. This means that
> operating on those gpio pins from userspace will not be possible.
> For instance, gpioget/gpioset return "Device or resource busy":
>
> gpioget: error reading GPIO values: Device or resource busy
> gpioset: error setting the GPIO line values: Device or resource busy
>
> I guess Harish will be unhappy about that, as his expectation was that
> upon merging gpio-aggregator with gpio-inverter, he will be able to
> describe GPIO polarity and names in DTS without "hogging" the pins.
> Perhaps this can be supplemented via an add-on patch later on?
When aggregating GPIO lines, the original GPIO lines are indeed marked
used/reserved, so you cannot use them from userspace.
However, you are expected to use them through the newly created virtual
gpiochip representing the aggregated GPIO lines.
You can try this using the "door" example in
Documentation/admin-guide/gpio/gpio-aggregator.rst, after replacing
gpio2 {19,20} by gpio6 {12,13} to suit your H3ULCB.
> For the whole series (leaving the above findings to your discretion):
>
> Reviewed-by: Eugeniu Rosca
> Tested-by: Eugeniu Rosca
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v4 2/5] gpiolib: Add support for GPIO line table lookup
Currently GPIOs can only be referred to by GPIO controller and offset in
GPIO lookup tables.
Add support for looking them up by line name.
Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
that this field can have two meanings, and update the kerneldoc and
GPIO_LOOKUP*() macros.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
---
v4:
- Add Reviewed-by,
- Rename gpiod_lookup.chip_label.
- Use U16_MAX instead of (u16)-1,
v3:
- New.
---
drivers/gpio/gpiolib.c | 22 +-
include/linux/gpio/machine.h | 15 ---
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 114325b366ae33d5..48fc6cf9a020f4ff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4457,7 +4457,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (!table)
return desc;
- for (p = >table[0]; p->chip_label; p++) {
+ for (p = >table[0]; p->key; p++) {
struct gpio_chip *chip;
/* idx must always match exactly */
@@ -4468,18 +4468,30 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
continue;
- chip = find_chip_by_name(p->chip_label);
+ if (p->chip_hwnum == U16_MAX) {
+ desc = gpio_name_to_desc(p->key);
+ if (desc) {
+ *flags = p->flags;
+ return desc;
+ }
+
+ dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+p->key);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ chip = find_chip_by_name(p->key);
if (!chip) {
/*
* As the lookup table indicates a chip with
-* p->chip_label should exist, assume it may
+* p->key should exist, assume it may
* still appear later and let the interested
* consumer be probed again or let the Deferred
* Probe infrastructure handle the error.
*/
dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
-p->chip_label);
+p->key);
return ERR_PTR(-EPROBE_DEFER);
}
@@ -4510,7 +4522,7 @@ static int platform_gpio_count(struct device *dev, const
char *con_id)
if (!table)
return -ENOENT;
- for (p = >table[0]; p->chip_label; p++) {
+ for (p = >table[0]; p->key; p++) {
if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
(!con_id && !p->con_id))
count++;
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 1ebe5be05d5f81fa..84c66fbf54fd5811 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -20,8 +20,9 @@ enum gpio_lookup_flags {
/**
* struct gpiod_lookup - lookup table
- * @chip_label: name of the chip the GPIO belongs to
- * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
+ * @key: either the name of the chip the GPIO belongs to, or the GPIO line name
+ * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO, or
+ * U16_MAX to indicate that @key is a GPIO line name
* @con_id: name of the GPIO from the device's point of view
* @idx: index of the GPIO in case several GPIOs share the same name
* @flags: bitmask of gpio_lookup_flags GPIO_* values
@@ -30,7 +31,7 @@ enum gpio_lookup_flags {
* functions using platform data.
*/
struct gpiod_lookup {
- const char *chip_label;
+ const char *key;
u16 chip_hwnum;
const char *con_id;
unsigned int idx;
@@ -63,17 +64,17 @@ struct gpiod_hog {
/*
* Simple definition of a single GPIO under a con_id
*/
-#define GPIO_LOOKUP(_chip_label, _chip_hwnum, _con_id, _flags) \
- GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, 0, _flags)
+#define GPIO_LOOKUP(_key, _chip_hwnum, _con_id, _flags) \
+ GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, 0, _flags)
/*
* Use this macro if you need to have several GPIOs under the same con_id.
* Each GPIO needs to use a different index and can be accessed using
* gpiod_get_index()
*/
-#define GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, _idx, _flags) \
+#define GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, _idx, _flags) \
{
[PATCH v4 1/5] gpiolib: Add support for gpiochipN-based table lookup
Currently GPIO controllers can only be referred to by label in GPIO
lookup tables.
Add support for looking them up by "gpiochipN" name, with "N" the
corresponding GPIO device's ID number.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
---
v4:
- Add Reviewed-by,
- Drop support for legacy sysfs interface based name matching,
- Replace complex custom matching by a simple additional check in the
existing gpiochip_match_name() function,
- Add kerneldoc() for find_chip_by_name(), documenting matching order.
v3:
- New.
---
drivers/gpio/gpiolib.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9fd2dfb658546159..114325b366ae33d5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1738,9 +1738,18 @@ static int gpiochip_match_name(struct gpio_chip *chip,
void *data)
{
const char *name = data;
- return !strcmp(chip->label, name);
+ return !strcmp(chip->label, name) ||
+ !strcmp(dev_name(>gpiodev->dev), name);
}
+/**
+ * find_chip_by_name() - Find a specific gpio_chip by name
+ * @name: Name to match
+ *
+ * Return a reference to a gpio_chip that matches the passed name.
+ * This function first tries matching on the gpio_chip's label, followed by
+ * matching on dev_name() of the corresponding gpio_device.
+ */
static struct gpio_chip *find_chip_by_name(const char *name)
{
return gpiochip_find((void *)name, gpiochip_match_name);
--
2.17.1
[PATCH v4 4/5] docs: gpio: Add GPIO Aggregator documentation
Document the GPIO Aggregator, and the two typical use-cases.
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Ulrich Hecht
---
v4:
- Add Reviewed-by,
- Drop controversial GPIO repeater,
- Clarify industrial control use case,
- Fix typo s/communicated/communicate/,
- Replace abstract frobnicator example by concrete door example with
gpio-line-names,
v3:
- New.
---
.../admin-guide/gpio/gpio-aggregator.rst | 102 ++
Documentation/admin-guide/gpio/index.rst | 1 +
2 files changed, 103 insertions(+)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst
b/Documentation/admin-guide/gpio/gpio-aggregator.rst
new file mode 100644
index ..954ed568b0b833b3
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -0,0 +1,102 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Aggregator
+===
+
+The GPIO Aggregator allows to aggregate GPIOs, and expose them as a new
+gpio_chip. This supports the following use cases.
+
+
+Aggregating GPIOs using Sysfs
+-
+
+GPIO controllers are exported to userspace using /dev/gpiochip* character
+devices. Access control to these devices is provided by standard UNIX file
+system permissions, on an all-or-nothing basis: either a GPIO controller is
+accessible for a user, or it is not.
+
+The GPIO Aggregator allows access control for individual GPIOs, by aggregating
+them into a new gpio_chip, which can be assigned to a group or user using
+standard UNIX file ownership and permissions. Furthermore, this simplifies and
+hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
+GPIO controller, and no longer needs to care about which GPIOs to grab and
+which not, reducing the attack surface.
+
+Aggregated GPIO controllers are instantiated and destroyed by writing to
+write-only attribute files in sysfs.
+
+/sys/bus/platform/drivers/gpio-aggregator/
+
+ "new_device" ...
+ Userspace may ask the kernel to instantiate an aggregated GPIO
+ controller by writing a string describing the GPIOs to
+ aggregate to the "new_device" file, using the format
+
+ .. code-block:: none
+
+ [] [ ] ...
+
+ Where:
+
+ "" ...
+ is a GPIO line name,
+
+ "" ...
+ is a GPIO chip label or name, and
+
+ "" ...
+ is a comma-separated list of GPIO offsets and/or
+ GPIO offset ranges denoted by dashes.
+
+ Example: Instantiate a new GPIO aggregator by aggregating GPIO
+ 19 of "e6052000.gpio" and GPIOs 20-21 of "gpiochip2" into a new
+ gpio_chip:
+
+ .. code-block:: bash
+
+ echo 'e6052000.gpio 19 gpiochip2 20-21' > new_device
+
+ "delete_device" ...
+ Userspace may ask the kernel to destroy an aggregated GPIO
+ controller after use by writing its device name to the
+ "delete_device" file.
+
+ Example: Destroy the previously-created aggregated GPIO
+ controller "gpio-aggregator.0":
+
+ .. code-block:: bash
+
+ echo gpio-aggregator.0 > delete_device
+
+
+Generic GPIO Driver
+---
+
+The GPIO Aggregator can also be used as a generic driver for a simple
+GPIO-operated device described in DT, without a dedicated in-kernel driver.
+This is useful in industrial control, and is not unlike e.g. spidev, which
+allows to communicate with an SPI device from userspace.
+
+Binding a device to the GPIO Aggregator is performed either by modifying the
+gpio-aggregator driver, or by writing to the "driver_override" file in Sysfs.
+
+Example: If "door" is a GPIO-operated device described in DT, using its own
+compatible value::
+
+door {
+compatible = "myvendor,mydoor";
+
+gpios = < 19 GPIO_ACTIVE_HIGH>,
+< 20 GPIO_ACTIVE_LOW>;
+ gpio-line-names = "open", "lock";
+};
+
+it can be bound to the GPIO Aggregator by either:
+
+1. Adding its compatible value to ``gpio_aggregator_dt_ids[]``,
+2. Binding manually using "driver_override":
+
+.. code-block:: bash
+
+echo gpio-aggregator > /sys/bus/platform/devices/door/driver_override
+echo door > /sys/bus/platform/drivers/gpio-aggregator/bind
diff --git a/Documentation/admin-guide/gpio/index.rst
b/Documentation/admin-guide/gpio/index.rst
index a244ba4e87d5398a..ef2838638e96 100644
--- a/Documentation/admin-guide/gpio/index.rst
+++ b/Documentation/admin-guide/gpio/index.rst
@@ -7,6 +7,7 @@ gpio
.. toctree::
:maxdepth: 1
+gpio-aggregator
sysfs
.. only:: subproject and html
--
2.17.1
[PATCH v4 0/5] gpio: Add GPIO Aggregator
IO Aggregator/Repeater"
(https://lore.kernel.org/lkml/20191127084253.16356-1-geert+rene...@glider.be/)
[3] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
(https://lore.kernel.org/linux-gpio/20190911143858.13024-1-geert+rene...@glider.be/)
[4] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
(https://lore.kernel.org/lkml/20190705160536.12047-1-geert+rene...@glider.be/)
[5] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+rene...@glider.be/)
[6] "Getting To Blinky: Virt Edition / Making device pass-through
work on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
Geert Uytterhoeven (5):
gpiolib: Add support for gpiochipN-based table lookup
gpiolib: Add support for GPIO line table lookup
gpio: Add GPIO Aggregator
docs: gpio: Add GPIO Aggregator documentation
MAINTAINERS: Add GPIO Aggregator section
.../admin-guide/gpio/gpio-aggregator.rst | 102
Documentation/admin-guide/gpio/index.rst | 1 +
MAINTAINERS | 7 +
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-aggregator.c| 574 ++
drivers/gpio/gpiolib.c| 33 +-
include/linux/gpio/machine.h | 15 +-
8 files changed, 732 insertions(+), 13 deletions(-)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
create mode 100644 drivers/gpio/gpio-aggregator.c
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v4 3/5] gpio: Add GPIO Aggregator
GPIO controllers are exported to userspace using /dev/gpiochip* character devices. Access control to these devices is provided by standard UNIX file system permissions, on an all-or-nothing basis: either a GPIO controller is accessible for a user, or it is not. Currently no mechanism exists to control access to individual GPIOs. Hence add a GPIO driver to aggregate existing GPIOs, and expose them as a new gpiochip. This supports the following use cases: - Aggregating GPIOs using Sysfs This is useful for implementing access control, and assigning a set of GPIOs to a specific user or virtual machine. - Generic GPIO Driver This is useful for industrial control, where it can provide userspace access to a simple GPIO-operated device described in DT, cfr. e.g. spidev for SPI-operated devices. Signed-off-by: Geert Uytterhoeven --- v4: - Remove unused assignment to n in isrange(), - Check correct pointer after aggr->lookups->dev_id allocation, - Preinitialize flags to 0 in gpio_fwd_[gs]et_multiple() to avoid may-be-used-uninitialized warning, - Drop controversial GPIO repeater, - Update for gpiod_lookup.chip_label rename, - Use %pe to format error pointers, - Use U16_MAX instead of (u16)-1, - Correct comment indentation, - Use skip_spaces() helper, - Rename a and b to first_index resp. last_index, - Add comment to tmp[] use, - Improve Kconfig help text, - Include for gpiod_[gs]et_*(), - Drop unneeded valid_mask handling, - Add comment about sleeping and .set_config() support, v3: - Absorb GPIO forwarder, - Integrate GPIO Repeater and Generic GPIO driver functionality, - Use the aggregator parameters to create a GPIO lookup table instead of an array of GPIO descriptors, which allows to simplify the code: 1. This removes the need for calling gpio_name_to_desc(), gpiochip_find(), gpiochip_get_desc(), and gpiod_request(), 2. This allows the platform device to always use devm_gpiod_get_index(), regardless of the origin of the GPIOs, - Move parameter parsing from platform device probe to sysfs attribute store, removing the need for platform data passing, - Use more devm_*() functions to simplify cleanup, - Add pr_fmt(), - General refactoring, v2: - Add missing initialization of i in gpio_virt_agg_probe(), - Update for removed .need_valid_mask field and changed .init_valid_mask() signature, - Drop "virtual", rename to gpio-aggregator, - Drop bogus FIXME related to gpiod_set_transitory() expectations, - Use new GPIO Forwarder Helper, - Lift limit on the maximum number of GPIOs, - Improve parsing: - add support for specifying GPIOs by line name, - add support for specifying GPIO chips by ID, - add support for GPIO offset ranges, - names and offset specifiers must be separated by whitespace, - GPIO offsets must separated by spaces, - Use str_has_prefix() and kstrtouint(). --- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-aggregator.c | 574 + 3 files changed, 587 insertions(+) create mode 100644 drivers/gpio/gpio-aggregator.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 64661b49a4c32874..e4fc7ce98734d918 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1514,6 +1514,18 @@ config GPIO_VIPERBOARD endmenu +config GPIO_AGGREGATOR + tristate "GPIO Aggregator" + help + Say yes here to enable the GPIO Aggregator, which provides a way to + aggregate existing GPIO lines into a new virtual GPIO chip. + This can serve the following purposes: + - Assign permissions for a collection of GPIO lines to a user, + - Export a collection of GPIO lines to a virtual machine, + - Provide a generic driver for a GPIO-operated device in an + industrial control context, to be operated from userspace using + the GPIO chardev interface. + config GPIO_MOCKUP tristate "GPIO Testing Driver" select IRQ_SIM diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index ba53f1fcde3a3591..60071fd1290a375c 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o +obj-$(CONFIG_GPIO_AGGREGATOR) += gpio-aggregator.o obj-$(CONFIG_GPIO_ALTERA_A10SR)+= gpio-altera-a10sr.o obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c new file mode 100644 index ..339335660d1c40c2 --- /dev/null +++ b/drivers/g
[PATCH v4 5/5] MAINTAINERS: Add GPIO Aggregator section
Add a maintainership section for the GPIO Aggregator, covering documentation and driver source code. Signed-off-by: Geert Uytterhoeven --- v4: - Drop controversial GPIO repeater, v3: - New. --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 74533d307d67321a..a28e25fd6a0c169f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7091,6 +7091,13 @@ F: Documentation/firmware-guide/acpi/gpio-properties.rst F: drivers/gpio/gpiolib-acpi.c F: drivers/gpio/gpiolib-acpi.h +GPIO AGGREGATOR +M: Geert Uytterhoeven +L: linux-g...@vger.kernel.org +S: Maintained +F: Documentation/admin-guide/gpio/gpio-aggregator.rst +F: drivers/gpio/gpio-aggregator.c + GPIO IR Transmitter M: Sean Young L: linux-me...@vger.kernel.org -- 2.17.1
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
Hi Eugeniu,
On Tue, Dec 3, 2019 at 11:51 AM Eugeniu Rosca wrote:
> On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> > +static int gpio_aggregator_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = >dev;
> > + struct gpio_desc **descs;
> > + struct gpiochip_fwd *fwd;
> > + int i, n;
>
> FWIW/FTR, doing some blind creation and deletion of gpio aggregator
> chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
> sure this is something 100% reproducible.
>
> [1] while true; do \
>echo e6055400.gpio 12,13 >
> /sys/bus/platform/drivers/gpio-aggregator/new_device; \
>echo gpio-aggregator.0 >
> /sys/bus/platform/drivers/gpio-aggregator/delete_device; \
>done
>
> [2] unreferenced object 0x0006d2c2e000 (size 128):
> comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
> hex dump (first 32 bytes):
> 00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff
> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> backtrace:
> [<a8e18c13>] slab_post_alloc_hook+0x8c/0x94
> [<6f419a4f>] __kmalloc+0x170/0x218
> [<60d185ea>] kobj_map+0x78/0x1c0
> [<c96645f3>] cdev_add+0x68/0x94
> [<a7a5a8ac>] cdev_device_add+0x74/0x90
> [<497871d3>] gpiochip_setup_dev+0x84/0x1f0
> [<b993f95f>] gpiochip_add_data_with_key+0xbcc/0x11f0
> [<fd728c0e>] devm_gpiochip_add_data+0x60/0xa8
> [<442e34c1>] gpio_aggregator_probe+0x210/0x3c8
> [<076e13fb>] platform_drv_probe+0x70/0xe4
> [<de84b58b>] really_probe+0x2d8/0x434
> [<c95c9784>] driver_probe_device+0x15c/0x16c
> [<afb7dd4f>] __device_attach_driver+0xdc/0x120
> [<efa40cae>] bus_for_each_drv+0x12c/0x154
> [<c149acef>] __device_attach+0x148/0x1e0
> [<a74fd158>] device_initial_probe+0x24/0x30
This is the allocation of the GPIO character device, which is allocated
in response to the creation of the GPIO chip, from .probe().
As that is done using devm_gpiochip_add_data(), the chardev should be
deallocated automatically by devm_gpio_chip_release() when
platform_device_unregister() is called.
Weird...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
Hi Linus,
On Sat, Jan 4, 2020 at 1:38 AM Linus Walleij wrote:
> Sorry for slowness... christmas.
Np. Happy New Year!
> On Thu, Dec 12, 2019 at 4:24 PM Geert Uytterhoeven
> wrote:
> > On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij
> > wrote:
> > > > + This can serve the following purposes:
> > > > + 1. Assign a collection of GPIOs to a user, or export them
> > > > to a
> > > > + virtual machine,
> > >
> > > This is ambiguous. What is a "user"? A process calling from
> > > userspace? A device tree node?
> >
> > A user is an entity with a UID, typically listed in /etc/passwd.
> > This is similar to letting some, not all, people on the machine access
> > the CD-ROM drive.
>
> Ah I get it. Maybe we can say "assign permissions for a collection
> of GPIOs to a user".
OK
> > > I would write "assign a collection of GPIO lines from any lines on
> > > existing physical GPIO chips to form a new virtual GPIO chip"
> > >
> > > That should be to the point, right?
> >
> > Yes, that's WHAT it does. The WHY is the granular access control.
>
> So I guess we can write both?
OK.
> > > > + 3. Provide a generic driver for a GPIO-operated device, to
> > > > be
> > > > + controlled from userspace using the GPIO chardev
> > > > interface.
> > >
> > > I don't understand this, it needs to be elaborated. What is meant
> > > by a "GPIO-operated device" in this context? Example?
> >
> > E.g. a motor. Or a door opener.
> >
> > door-opener {
> > compatible = "mydoor,opener";
> >
> > gpios = < 19 GPIO_ACTIVE_HIGH>;
> > };
> >
> > You don't need a full-featured kernel driver for that, so just bind the
> > gpio-aggregator to the door-opener, and control it through libgpiod.
>
> Yep it's a perfect industrial control example, I get it.
>
> Maybe we should blurb something about industrial control?
OK.
> The rest I think we cleared out else I will see it when I review again.
The remaining discussion point is "GPIO Repeater in Device Tree", i.e.
the GPIO inverter usecase, which might be solved better by adding a
GPIO_INVERTED flag.
Shall I rip that out, incorporate review comments, and report?
Thanks!
--
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
Hi Rob,
On Fri, Dec 6, 2019 at 4:04 PM Rob Herring wrote:
> On Fri, Dec 6, 2019 at 3:17 AM Geert Uytterhoeven
> wrote:
> > On Thu, Dec 5, 2019 at 10:06 PM Rob Herring wrote:
> > > On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
> > > > Add Device Tree bindings for a GPIO repeater, with optional translation
> > > > of physical signal properties. This is useful for describing explicitly
> > > > the presence of e.g. an inverter on a GPIO line, and was inspired by the
> > > > non-YAML gpio-inverter bindings by Harish Jenny K N
> > > > [1].
> > > >
> > > > Note that this is different from a GPIO Nexus Node[2], which cannot do
> > > > physical signal property translation.
> > >
> > > It can't? Why not? The point of the passthru mask is to not do
> > > translation of flags, but without it you are always doing translation of
> > > cells.
> >
> > Thanks for pushing me deeper into nexuses!
> > You're right, you can map from one type to another.
> > However, you cannot handle the "double inversion" of an ACTIVE_LOW
> > signal with a physical inverter added:
> >
> > nexus: led-nexus {
> > #gpio-cells = <2>;
> > gpio-map = <0 0 19 GPIO_ACTIVE_LOW>, // inverted
> ><1 0 20 GPIO_ACTIVE_HIGH>,//
> > noninverted
> ><2 0 21 GPIO_ACTIVE_LOW>; // inverted
> > gpio-map-mask = <3 0>;
> > // default gpio-map-pass-thru = <0 0>;
> > };
> >
> > leds {
> > compatible = "gpio-leds";
> > led6-inverted {
> > gpios = < 0 GPIO_ACTIVE_HIGH>;
> > };
> > led7-noninverted {
> > gpios = < 1 GPIO_ACTIVE_HIGH>;
> > };
> > led8-double-inverted { // FAILS: still inverted
> > gpios = < 2 GPIO_ACTIVE_LOW>;
> > };
> > };
> >
> > It "works" if the last entry in gpio-map is changed to GPIO_ACTIVE_HIGH.
> > Still, the consumer would see the final translated polarity, and not the
> > actual one it needs to program the consumer for.
>
> I'm not really following. Why isn't a double inversion just the same
> as no inversion?
Because the nexus can only mask and/or substitute bits.
It cannot do a XOR operation on the GPIO flags.
> > > > While an inverter can be described implicitly by exchanging the
> > > > GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> > > > Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> > > > th provider and consumer sides:
> > > > 1. The GPIO provider (controller) looks at the flags to know the
> > > > polarity, so it can translate between logical (active/not active)
> > > > and physical (high/low) signal levels.
> > > > 2. While the signal polarity is usually fixed on the GPIO consumer
> > > > side (e.g. an LED is tied to either the supply voltage or GND),
> > > > it may be configurable on some devices, and both sides need to
> > > > agree. Hence the GPIO_ACTIVE_* flag as seen by the consumer must
> > > > match the actual polarity.
> > > > There exists a similar issue with interrupt flags, where both the
> > > > interrupt controller and the device generating the interrupt need
> > > > to agree, which breaks in the presence of a physical inverter not
> > > > described in DT (see e.g. [3]).
> > >
> > > Adding an inverted flag as I've suggested would also solve this issue.
> >
> > As per your suggestion in "Re: [PATCH V4 2/2] gpio: inverter: document
> > the inverter bindings"?
> > https://lore.kernel.org/linux-devicetree/cal_jsqlp___2o-nau+2ppqy0qmjx6+an3hbyz-ob9+qfvwg...@mail.gmail.com/
> >
> > Oh, now I understand. I was misguided by Harish' interpretation
> > https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc...@mentor.com/
> > which assumed an "inverted" property, e.g.
> >
> > inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;
> >
> > But you actually meant a new GPIO_INVERTED flag, to be ORed into the 2nd
> > cell of a GPIO specifier? I.e. add to include/dt-bindings/gpio/gpio.h
Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
Hi Linus,
On Sat, Jan 4, 2020 at 1:21 AM Linus Walleij wrote:
> On Thu, Dec 12, 2019 at 3:48 PM Geert Uytterhoeven
> wrote:
> > On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij
> > wrote:
> > > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > > wrote:
> > > > +The GPIO Aggregator allows access control for individual GPIOs, by
> > > > aggregating
> > > > +them into a new gpio_chip, which can be assigned to a group or user
> > > > using
> > > > +standard UNIX file ownership and permissions. Furthermore, this
> > > > simplifies and
> > > > +hardens exporting GPIOs to a virtual machine, as the VM can just grab
> > > > the full
> > > > +GPIO controller, and no longer needs to care about which GPIOs to grab
> > > > and
> > > > +which not, reducing the attack surface.
> > > > +
> > > > +Aggregated GPIO controllers are instantiated and destroyed by writing
> > > > to
> > > > +write-only attribute files in sysfs.
> > >
> > > I suppose virtual machines will have a lengthy config file where
> > > they specify which GPIO lines to pick and use for their GPIO
> > > aggregator, and that will all be fine, the VM starts and the aggregator
> > > is there and we can start executing.
> > >
> > > I would perhaps point out a weakness as with all sysfs and with the
> > > current
> > > gpio sysfs: if a process creates an aggregator device, and then that
> > > process crashes, what happens when you try to restart the process and
> > > run e.g. your VM again?
> > >
> > > Time for a hard reboot? Or should we add some design guidelines for
> > > these machines so that they can cleanly tear down aggregators
> > > previously created by the crashed VM?
> >
> > No, the VM does not create the aggregator.
> >
> > The idea is for the user to create one or more aggregators, set up
> > permissions on /dev/gpiochipX, and launch the VM, passing the aggregated
> > /dev/gpiochipX as parameters.
> > If the VM crashes, just launch it again.
> >
> > Destroying the aggregators is a manual and independent process, after
> > the VM has exited.
>
> I'm thinking about someone making some industrial application for some
> control of a machinery say a robotic arm.
>
> And do make sure this VM is only controlling these GPIOs related to
> this robotic arm, they create a GPIO aggregator. And we care about
> cases like that since we provide this security argument.
>
> Surely that machine will be rebooted.
>
> Surely they don't have a printed paper with all the commands lying
> at the console, and asking whoever powers it back on to manually
> type it all in again. That feels a bit 1981.
>
> So they will have a script for this I suppose. Possibly in some
> initscript so it is set up on boot. And this script echos stuff
> all over the place to set up the aggregator.
>
> Is this the use case you're thinking of?
Exactly.
And they can configure that by echoing the GPIO specifiers to
/sys/bus/platform/drivers/gpio-aggregator/new_device.
If their system has DT, another option is to describe the device in DT,
and add its compatible value to gpio_aggregator_dt_ids[], cfr. the
frobnicator example.
> I just like to have the whole picture here.
Sure. If anything is still unclear, please let me know!
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
Hi Linus,
On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
> > a new gpiochip.
> >
> > This supports the following use cases:
> > 1. Aggregating GPIOs using Sysfs
> > This is useful for implementing access control, and assigning a set
> > of GPIOs to a specific user or virtual machine.
> >
> > 2. GPIO Repeater in Device Tree
> > This supports modelling e.g. GPIO inverters in DT.
> >
> > 3. Generic GPIO Driver
> > This provides userspace access to a simple GPIO-operated device
> > described in DT, cfr. e.g. spidev for SPI-operated devices.
> >
> > Signed-off-by: Geert Uytterhoeven
>
> Overall I like how this is developing!
>
> > +config GPIO_AGGREGATOR
> > + tristate "GPIO Aggregator/Repeater"
> > + help
> > + Say yes here to enable the GPIO Aggregator and repeater, which
> > + provides a way to aggregate and/or repeat existing GPIOs into a
> > new
> > + GPIO device.
>
> Should it say a "new virtual GPIO chip"?
OK.
> > + This can serve the following purposes:
> > + 1. Assign a collection of GPIOs to a user, or export them to a
> > + virtual machine,
>
> This is ambiguous. What is a "user"? A process calling from
> userspace? A device tree node?
A user is an entity with a UID, typically listed in /etc/passwd.
This is similar to letting some, not all, people on the machine access
the CD-ROM drive.
> I would write "assign a collection of GPIO lines from any lines on
> existing physical GPIO chips to form a new virtual GPIO chip"
>
> That should be to the point, right?
Yes, that's WHAT it does. The WHY is the granular access control.
> > + 2. Support GPIOs that are connected to a physical inverter,
>
> s/to/through/g
OK.
> > + 3. Provide a generic driver for a GPIO-operated device, to be
> > + controlled from userspace using the GPIO chardev interface.
>
> I don't understand this, it needs to be elaborated. What is meant
> by a "GPIO-operated device" in this context? Example?
E.g. a motor. Or a door opener.
door-opener {
compatible = "mydoor,opener";
gpios = < 19 GPIO_ACTIVE_HIGH>;
};
You don't need a full-featured kernel driver for that, so just bind the
gpio-aggregator to the door-opener, and control it through libgpiod.
> I consistently use the term "GPIO line" as opposed to "GPIO"
> or "GPIO number" etc that are abigous, so please rephrase using
> "GPIO lines" rather than just "GPIOs" above.
OK.
> > +#include "gpiolib.h"
>
> Whenever this is included in a driver I want it to come with a comment
> explicitly stating exactly why and which internal symbols the driver
> needs to access. Ideally all drivers should just need ...
"gpiolib.h" is needed to access gpio_desc.gdev->chip in
gpio_fwd_set_config(). And for gpio_chip_hwgpio() (see below).
But indeed, I should add #include , for e.g. the
various gpiod_[gs]et_*() functions.
> > +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> > +int hwnum, unsigned int *n)
>
> u16 hwnum for the hardware number but if it is always -1/U16_MAX
> then why pass the parameter at all.
>
> Is "label" the right name of this parameter if that is going to actually
> be line_name then use that.
It's not always -1.
This function can be called either with a gpiochip label/name and an
offset, or a line-name and -1.
> > +{
> > + struct gpiod_lookup_table *lookups;
> > +
> > + lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n +
> > 2),
> > + GFP_KERNEL);
> > + if (!lookups)
> > + return -ENOMEM;
> > +
> > + lookups->table[*n].chip_label = label;
>
> This is pending the discussion on whether to just use "key" for this
> name.
Which would require touching all users (board files and mfd drivers).
> > + lookups-&g
Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
Hi Linus,
On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> wrote:
> > +The GPIO Aggregator allows access control for individual GPIOs, by
> > aggregating
> > +them into a new gpio_chip, which can be assigned to a group or user using
> > +standard UNIX file ownership and permissions. Furthermore, this
> > simplifies and
> > +hardens exporting GPIOs to a virtual machine, as the VM can just grab the
> > full
> > +GPIO controller, and no longer needs to care about which GPIOs to grab and
> > +which not, reducing the attack surface.
> > +
> > +Aggregated GPIO controllers are instantiated and destroyed by writing to
> > +write-only attribute files in sysfs.
>
> I suppose virtual machines will have a lengthy config file where
> they specify which GPIO lines to pick and use for their GPIO
> aggregator, and that will all be fine, the VM starts and the aggregator
> is there and we can start executing.
>
> I would perhaps point out a weakness as with all sysfs and with the current
> gpio sysfs: if a process creates an aggregator device, and then that
> process crashes, what happens when you try to restart the process and
> run e.g. your VM again?
>
> Time for a hard reboot? Or should we add some design guidelines for
> these machines so that they can cleanly tear down aggregators
> previously created by the crashed VM?
No, the VM does not create the aggregator.
The idea is for the user to create one or more aggregators, set up
permissions on /dev/gpiochipX, and launch the VM, passing the aggregated
/dev/gpiochipX as parameters.
If the VM crashes, just launch it again.
Destroying the aggregators is a manual and independent process, after
the VM has exited.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
Hi Linus,
On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> wrote:
> > Currently GPIO controllers can only be referred to by label in GPIO
> > lookup tables.
> >
> > Add support for looking them up by "gpiochipN" name, with "N" either the
> > corresponding GPIO device's ID number, or the GPIO controller's first
> > GPIO number.
> >
> > Signed-off-by: Geert Uytterhoeven
>
> What the commit message is missing is a rationale, why is this needed?
Right. To be added: so they can be looked up in the GPIO lookup table
using either the chip's label, or the "gpiochipN" name.
> > If this is rejected, the GPIO Aggregator documentation must be updated.
> >
> > The second variant is currently used by the legacy sysfs interface only,
> > so perhaps the chip->base check should be dropped?
>
> Anything improving the sysfs is actively discouraged by me.
> If it is just about staying compatible it is another thing.
OK, so N must be the corresponding GPIO device's ID number.
> > +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> > +{
> > + int id = (uintptr_t)data;
> > +
> > + return id == chip->base || id == chip->gpiodev->id;
> > +}
> > static struct gpio_chip *find_chip_by_name(const char *name)
> > {
> > - return gpiochip_find((void *)name, gpiochip_match_name);
> > + struct gpio_chip *chip;
> > + int id;
> > +
> > + chip = gpiochip_find((void *)name, gpiochip_match_name);
> > + if (chip)
> > + return chip;
> > +
> > + if (!str_has_prefix(name, GPIOCHIP_NAME))
> > + return NULL;
> > +
> > + if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, ))
> > + return NULL;
> > +
> > + return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
>
> Isn't it easier to just augment the existing match function to
> check like this:
>
> static int gpiochip_match_name(struct gpio_chip *chip, void *data)
> {
> const char *name = data;
>
> if (!strcmp(chip->label, name))
>return 0;
return true;
> return !strcmp(dev_name(>gpiodev->dev), name);
> }
Oh, didn't think of using dev_name() on the gpiodev.
Yes, with the chip->base check removed, the code can be simplified.
Or just
return !strcmp(chip->label, name) ||
!strcmp(dev_name(>gpiodev->dev), name);
> We should I guess also add some kerneldoc to say we first
> match on the label and second on dev_name().
OK.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
Hi Rob,
On Thu, Dec 5, 2019 at 10:06 PM Rob Herring wrote:
> On Wed, Nov 27, 2019 at 09:42:50AM +0100, Geert Uytterhoeven wrote:
> > Add Device Tree bindings for a GPIO repeater, with optional translation
> > of physical signal properties. This is useful for describing explicitly
> > the presence of e.g. an inverter on a GPIO line, and was inspired by the
> > non-YAML gpio-inverter bindings by Harish Jenny K N
> > [1].
> >
> > Note that this is different from a GPIO Nexus Node[2], which cannot do
> > physical signal property translation.
>
> It can't? Why not? The point of the passthru mask is to not do
> translation of flags, but without it you are always doing translation of
> cells.
Thanks for pushing me deeper into nexuses!
You're right, you can map from one type to another.
However, you cannot handle the "double inversion" of an ACTIVE_LOW
signal with a physical inverter added:
nexus: led-nexus {
#gpio-cells = <2>;
gpio-map = <0 0 19 GPIO_ACTIVE_LOW>, // inverted
<1 0 20 GPIO_ACTIVE_HIGH>,// noninverted
<2 0 21 GPIO_ACTIVE_LOW>; // inverted
gpio-map-mask = <3 0>;
// default gpio-map-pass-thru = <0 0>;
};
leds {
compatible = "gpio-leds";
led6-inverted {
gpios = < 0 GPIO_ACTIVE_HIGH>;
};
led7-noninverted {
gpios = < 1 GPIO_ACTIVE_HIGH>;
};
led8-double-inverted { // FAILS: still inverted
gpios = < 2 GPIO_ACTIVE_LOW>;
};
};
It "works" if the last entry in gpio-map is changed to GPIO_ACTIVE_HIGH.
Still, the consumer would see the final translated polarity, and not the
actual one it needs to program the consumer for.
> > While an inverter can be described implicitly by exchanging the
> > GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> > Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> > th provider and consumer sides:
> > 1. The GPIO provider (controller) looks at the flags to know the
> > polarity, so it can translate between logical (active/not active)
> > and physical (high/low) signal levels.
> > 2. While the signal polarity is usually fixed on the GPIO consumer
> > side (e.g. an LED is tied to either the supply voltage or GND),
> > it may be configurable on some devices, and both sides need to
> > agree. Hence the GPIO_ACTIVE_* flag as seen by the consumer must
> > match the actual polarity.
> > There exists a similar issue with interrupt flags, where both the
> > interrupt controller and the device generating the interrupt need
> > to agree, which breaks in the presence of a physical inverter not
> > described in DT (see e.g. [3]).
>
> Adding an inverted flag as I've suggested would also solve this issue.
As per your suggestion in "Re: [PATCH V4 2/2] gpio: inverter: document
the inverter bindings"?
https://lore.kernel.org/linux-devicetree/cal_jsqlp___2o-nau+2ppqy0qmjx6+an3hbyz-ob9+qfvwg...@mail.gmail.com/
Oh, now I understand. I was misguided by Harish' interpretation
https://lore.kernel.org/linux-devicetree/dde73334-a26d-b53f-6b97-4101c1cdc...@mentor.com/
which assumed an "inverted" property, e.g.
inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;
But you actually meant a new GPIO_INVERTED flag, to be ORed into the 2nd
cell of a GPIO specifier? I.e. add to include/dt-bindings/gpio/gpio.h"
/* Bit 6 expresses the presence of a physical inverter */
#define GPIO_INVERTED 64
We need to be very careful in defining to which side the GPIO_ACTIVE_*
applies to (consumer?), and which side the GPIO_INVERTED flag (provider?).
Still, this doesn't help if e.g. a FET is used instead of a push-pull
inverter, as the former needs translation of other flags (which the
nexus can do, the caveats above still applies, though).
Same for adding IRQ_TYPE_INVERTED.
Related issue: how to handle physical inverters on SPI chip select lines,
if the SPI slave can be configured for both polarities?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
Hi Harish,
On Tue, Dec 3, 2019 at 6:42 AM Harish Jenny K N
wrote:
> > +static int gpio_aggregator_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = >dev;
> > + struct gpio_desc **descs;
> > + struct gpiochip_fwd *fwd;
> > + int i, n;
> > +
> > + n = gpiod_count(dev, NULL);
> > + if (n < 0)
> > + return n;
> > +
> > + descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> > + if (!descs)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < n; i++) {
> > + descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>
> can you please add this check as well as we need to return EPROBE_DEFER.
>
> if (desc[i] == ERR_PTR(-ENOENT))
> < return -EPROBE_DEFER;
So gpiod_get_index() nevers return -EPROBE_DEFER, but returns -ENOENT
instead?
How can a driver distinguish between "GPIO not found" and "gpiochip driver
not yet initialized"?
Worse, so the *_optional() variants will return NULL in both cases, too, so
the caller will always fall back to optional GPIO not present?
Or am I missing something?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
Hi Eugeniu,
On Wed, Nov 27, 2019 at 3:15 PM Eugeniu Rosca wrote:
> On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> > +static bool isrange(const char *s)
> > +{
> > + size_t n = strlen(s);
>
> Cppcheck 1.40-18521-ge6d692d96058:
> drivers/gpio/gpio-aggregator.c:69:11: style: Variable 'n' is assigned a value
> that is never used. [unreadVariable]
>
> Smatch v0.5.0-6150-gc1ed13e4ee7b:
> drivers/gpio/gpio-aggregator.c:69 isrange() warn: unused return: n = strlen()
Correct, this is a remainder of code present temporarily during development.
Will drop.
(where are the days gcc itself warned about that?)
> > + aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> > + if (!aggr->lookups) {
> > + res = -ENOMEM;
> > + goto remove_idr;
> > + }
>
> s/aggr->lookups/aggr->lookups->dev_id/ ?
Thanks, will fix.
> > +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long
> > *mask,
> > + unsigned long *bits)
> > +{
> > + struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > + unsigned long *values, flags;
>
> gcc 9.2.1:
> warning: ‘flags’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>
> [..]
>
> > +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long
> > *mask,
> > + unsigned long *bits)
> > +{
> > + struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > + unsigned long *values, flags;
>
> gcc 9.2.1, same as above:
> warning: ‘flags’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
So newer gcc is (again) no longer smart enough to notice the check is
the same for initializer and user...
> Should these be silenced like in 2bf593f101f3ca ("xilinx_uartps.c:
> suppress "may be used uninitialised" warning") ?
TBH, I'm not a big fan of silencing false positives.
But if people like to see flags preinitialized to zero, that can be done...
> I plan to do some runtime testing soon.
Thanks, looking forward to the results!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
GPIO controllers are exported to userspace using /dev/gpiochip*
character devices. Access control to these devices is provided by
standard UNIX file system permissions, on an all-or-nothing basis:
either a GPIO controller is accessible for a user, or it is not.
Currently no mechanism exists to control access to individual GPIOs.
Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
a new gpiochip.
This supports the following use cases:
1. Aggregating GPIOs using Sysfs
This is useful for implementing access control, and assigning a set
of GPIOs to a specific user or virtual machine.
2. GPIO Repeater in Device Tree
This supports modelling e.g. GPIO inverters in DT.
3. Generic GPIO Driver
This provides userspace access to a simple GPIO-operated device
described in DT, cfr. e.g. spidev for SPI-operated devices.
Signed-off-by: Geert Uytterhoeven
---
v3:
- Absorb GPIO forwarder,
- Integrate GPIO Repeater and Generic GPIO driver functionality,
- Use the aggregator parameters to create a GPIO lookup table instead
of an array of GPIO descriptors, which allows to simplify the code:
1. This removes the need for calling gpio_name_to_desc(),
gpiochip_find(), gpiochip_get_desc(), and gpiod_request(),
2. This allows the platform device to always use
devm_gpiod_get_index(), regardless of the origin of the GPIOs,
- Move parameter parsing from platform device probe to sysfs attribute
store, removing the need for platform data passing,
- Use more devm_*() functions to simplify cleanup,
- Add pr_fmt(),
- General refactoring.
v2:
- Add missing initialization of i in gpio_virt_agg_probe(),
- Update for removed .need_valid_mask field and changed
.init_valid_mask() signature,
- Drop "virtual", rename to gpio-aggregator,
- Drop bogus FIXME related to gpiod_set_transitory() expectations,
- Use new GPIO Forwarder Helper,
- Lift limit on the maximum number of GPIOs,
- Improve parsing:
- add support for specifying GPIOs by line name,
- add support for specifying GPIO chips by ID,
- add support for GPIO offset ranges,
- names and offset specifiers must be separated by whitespace,
- GPIO offsets must separated by spaces,
- Use str_has_prefix() and kstrtouint().
---
drivers/gpio/Kconfig | 13 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-aggregator.c | 587 +
3 files changed, 601 insertions(+)
create mode 100644 drivers/gpio/gpio-aggregator.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8adffd42f8cb0559..36b6b57a6b05e906 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1507,6 +1507,19 @@ config GPIO_VIPERBOARD
endmenu
+config GPIO_AGGREGATOR
+ tristate "GPIO Aggregator/Repeater"
+ help
+ Say yes here to enable the GPIO Aggregator and repeater, which
+ provides a way to aggregate and/or repeat existing GPIOs into a new
+ GPIO device.
+ This can serve the following purposes:
+ 1. Assign a collection of GPIOs to a user, or export them to a
+ virtual machine,
+ 2. Support GPIOs that are connected to a physical inverter,
+ 3. Provide a generic driver for a GPIO-operated device, to be
+ controlled from userspace using the GPIO chardev interface.
+
config GPIO_MOCKUP
tristate "GPIO Testing Driver"
select IRQ_SIM
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 34eb8b2b12dd656c..f9971eeb1f32335f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o
obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_AGGREGATOR) += gpio-aggregator.o
obj-$(CONFIG_GPIO_ALTERA_A10SR)+= gpio-altera-a10sr.o
obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
new file mode 100644
index ..873578c6f9683db8
--- /dev/null
+++ b/drivers/gpio/gpio-aggregator.c
@@ -0,0 +1,587 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// GPIO Aggregator and Repeater
+//
+// Copyright (C) 2019 Glider bvba
+
+#define DRV_NAME "gpio-aggregator"
+#define pr_fmt(fmt)DRV_NAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "gpiolib.h"
+
+
+ /*
+* GPIO Aggregator sysfs interface
+*/
+
+struct gpio_aggregator {
+ struct gpiod_lookup_table *lookups;
+ struct platform_d
[PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
Document the GPIO Aggregator/Repeater, and the three typical use-cases.
Signed-off-by: Geert Uytterhoeven
---
v3:
- New.
---
.../admin-guide/gpio/gpio-aggregator.rst | 111 ++
Documentation/admin-guide/gpio/index.rst | 1 +
2 files changed, 112 insertions(+)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst
b/Documentation/admin-guide/gpio/gpio-aggregator.rst
new file mode 100644
index ..826146e260253299
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -0,0 +1,111 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Aggregator/Repeater
+
+
+The GPIO Aggregator/Repeater allows to aggregate GPIOs, and expose them as a
+new gpio_chip. This supports the following use cases.
+
+
+Aggregating GPIOs using Sysfs
+-
+
+GPIO controllers are exported to userspace using /dev/gpiochip* character
+devices. Access control to these devices is provided by standard UNIX file
+system permissions, on an all-or-nothing basis: either a GPIO controller is
+accessible for a user, or it is not.
+
+The GPIO Aggregator allows access control for individual GPIOs, by aggregating
+them into a new gpio_chip, which can be assigned to a group or user using
+standard UNIX file ownership and permissions. Furthermore, this simplifies and
+hardens exporting GPIOs to a virtual machine, as the VM can just grab the full
+GPIO controller, and no longer needs to care about which GPIOs to grab and
+which not, reducing the attack surface.
+
+Aggregated GPIO controllers are instantiated and destroyed by writing to
+write-only attribute files in sysfs.
+
+/sys/bus/platform/drivers/gpio-aggregator/
+
+ "new_device" ...
+ Userspace may ask the kernel to instantiate an aggregated GPIO
+ controller by writing a string describing the GPIOs to
+ aggregate to the "new_device" file, using the format
+
+ .. code-block:: none
+
+ [] [ ] ...
+
+ Where:
+
+ "" ...
+ is a GPIO line name,
+
+ "" ...
+ is a GPIO chip label or name, and
+
+ "" ...
+ is a comma-separated list of GPIO offsets and/or
+ GPIO offset ranges denoted by dashes.
+
+ Example: Instantiate a new GPIO aggregator by aggregating GPIO
+ 19 of "e6052000.gpio" and GPIOs 20-21 of "gpiochip2" into a new
+ gpio_chip:
+
+ .. code-block:: bash
+
+ echo 'e6052000.gpio 19 gpiochip2 20-21' > new_device
+
+ "delete_device" ...
+ Userspace may ask the kernel to destroy an aggregated GPIO
+ controller after use by writing its device name to the
+ "delete_device" file.
+
+ Example: Destroy the previously-created aggregated GPIO
+ controller "gpio-aggregator.0":
+
+ .. code-block:: bash
+
+ echo gpio-aggregator.0 > delete_device
+
+
+GPIO Repeater in Device Tree
+
+
+A GPIO Repeater is a node in a Device Tree representing a repeater for one or
+more GPIOs, possibly including physical signal property translation (e.g.
+polarity inversion). This allows to model e.g. inverters in DT.
+
+See Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
+
+
+Generic GPIO Driver
+---
+
+The GPIO Aggregator can also be used as a generic driver for a simple
+GPIO-operated device described in DT, without a dedicated in-kernel driver.
+This is not unlike e.g. spidev, which allows to communicated with an SPI device
+from userspace.
+
+Binding a device to the GPIO Aggregator is performed either by modifying the
+gpio-aggregator driver, or by writing to the "driver_override" file in Sysfs.
+
+Example: If "frobnicator" is a GPIO-operated device described in DT, using its
+own compatible value::
+
+frobnicator {
+compatible = "myvendor,frobnicator";
+
+gpios = < 19 GPIO_ACTIVE_HIGH>,
+< 20 GPIO_ACTIVE_LOW>;
+};
+
+it can be bound to the GPIO Aggregator by either:
+
+1. Adding its compatible value to ``gpio_aggregator_dt_ids[]``,
+2. Binding manually using "driver_override":
+
+.. code-block:: bash
+
+echo gpio-aggregator >
/sys/bus/platform/devices/frobnicator/driver_override
+echo frobnicator > /sys/bus/platform/drivers/gpio-aggregator/bind
diff --git a/Documentation/admin-guide/gpio/index.rst
b/Documentation/admin-guide/gpio/index.rst
index a244ba4e87d5398a..ef2838638e96 100644
--- a/Documentat
[PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater
.be/) [4] "[PATCH QEMU POC] Add a GPIO backend" (https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+rene...@glider.be/) [5] "Getting To Blinky: Virt Edition / Making device pass-through work on embedded ARM" (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/) Geert Uytterhoeven (7): gpiolib: Add GPIOCHIP_NAME definition gpiolib: Add support for gpiochipN-based table lookup gpiolib: Add support for GPIO line table lookup dt-bindings: gpio: Add gpio-repeater bindings gpio: Add GPIO Aggregator/Repeater driver docs: gpio: Add GPIO Aggregator/Repeater documentation MAINTAINERS: Add GPIO Aggregator/Repeater section .../admin-guide/gpio/gpio-aggregator.rst | 111 Documentation/admin-guide/gpio/index.rst | 1 + .../bindings/gpio/gpio-repeater.yaml | 53 ++ MAINTAINERS | 8 + drivers/gpio/Kconfig | 13 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-aggregator.c| 587 ++ drivers/gpio/gpiolib-sysfs.c | 7 +- drivers/gpio/gpiolib.c| 38 +- drivers/gpio/gpiolib.h| 2 + include/linux/gpio/machine.h | 2 +- 11 files changed, 815 insertions(+), 8 deletions(-) create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst create mode 100644 Documentation/devicetree/bindings/gpio/gpio-repeater.yaml create mode 100644 drivers/gpio/gpio-aggregator.c -- 2.17.1
[PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings
Add Device Tree bindings for a GPIO repeater, with optional translation
of physical signal properties. This is useful for describing explicitly
the presence of e.g. an inverter on a GPIO line, and was inspired by the
non-YAML gpio-inverter bindings by Harish Jenny K N
[1].
Note that this is different from a GPIO Nexus Node[2], which cannot do
physical signal property translation.
While an inverter can be described implicitly by exchanging the
GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
th provider and consumer sides:
1. The GPIO provider (controller) looks at the flags to know the
polarity, so it can translate between logical (active/not active)
and physical (high/low) signal levels.
2. While the signal polarity is usually fixed on the GPIO consumer
side (e.g. an LED is tied to either the supply voltage or GND),
it may be configurable on some devices, and both sides need to
agree. Hence the GPIO_ACTIVE_* flag as seen by the consumer must
match the actual polarity.
There exists a similar issue with interrupt flags, where both the
interrupt controller and the device generating the interrupt need
to agree, which breaks in the presence of a physical inverter not
described in DT (see e.g. [3]).
[1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
https://lore.kernel.org/linux-gpio/1561699236-18620-3-git-send-email-harish_kand...@mentor.com/
[2] Devicetree Specification v0.3-rc2, Section 2.5
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3-rc2
[3] "[PATCH] wlcore/wl18xx: Add invert-irq OF property for physically
inverted IRQ"
https://lore.kernel.org/linux-renesas-soc/20190607172958.20745-1-ero...@de.adit-jv.com/
Signed-off-by: Geert Uytterhoeven
---
v3:
- New.
---
.../bindings/gpio/gpio-repeater.yaml | 53 +++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
diff --git a/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
new file mode 100644
index ..efdee0c3be43f731
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-repeater.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO Repeater
+
+maintainers:
+ - Harish Jenny K N
+ - Geert Uytterhoeven
+
+description:
+ This represents a repeater for one or more GPIOs, possibly including physical
+ signal property translation (e.g. polarity inversion).
+
+properties:
+ compatible:
+const: gpio-repeater
+
+ "#gpio-cells":
+const: 2
+
+ gpio-controller: true
+
+ gpios:
+description:
+ Phandle and specifier, one for each repeated GPIO.
+
+ gpio-line-names:
+description:
+ Strings defining the names of the GPIO lines going out of the GPIO
+ controller.
+
+required:
+ - compatible
+ - "#gpio-cells"
+ - gpio-controller
+ - gpios
+
+additionalProperties: false
+
+examples:
+ # Device node describing a polarity inverter for a single GPIO
+ - |
+#include
+
+inverter: gpio-repeater {
+compatible = "gpio-repeater";
+#gpio-cells = <2>;
+gpio-controller;
+gpios = < 95 GPIO_ACTIVE_LOW>;
+};
--
2.17.1
[PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition
The string literal "gpiochip" is used in several places.
Add a definition for it, and use it everywhere, to make sure everything
stays in sync.
Signed-off-by: Geert Uytterhoeven
---
v3:
- New.
---
drivers/gpio/gpiolib-sysfs.c | 7 +++
drivers/gpio/gpiolib.c | 4 ++--
drivers/gpio/gpiolib.h | 2 ++
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index fbf6b1a0a4fae6ce..23e3d335cd543d53 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -762,10 +762,9 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
parent = >dev;
/* use chip->base for the ID; it's already known to be unique */
- dev = device_create_with_groups(_class, parent,
- MKDEV(0, 0),
- chip, gpiochip_groups,
- "gpiochip%d", chip->base);
+ dev = device_create_with_groups(_class, parent, MKDEV(0, 0), chip,
+ gpiochip_groups, GPIOCHIP_NAME "%d",
+ chip->base);
if (IS_ERR(dev))
return PTR_ERR(dev);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index dce0b31f4125a6b3..c9e47620d2434983 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1419,7 +1419,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip,
void *data,
ret = gdev->id;
goto err_free_gdev;
}
- dev_set_name(>dev, "gpiochip%d", gdev->id);
+ dev_set_name(>dev, GPIOCHIP_NAME "%d", gdev->id);
device_initialize(>dev);
dev_set_drvdata(>dev, gdev);
if (chip->parent && chip->parent->driver)
@@ -5105,7 +5105,7 @@ static int __init gpiolib_dev_init(void)
return ret;
}
- ret = alloc_chrdev_region(_devt, 0, GPIO_DEV_MAX, "gpiochip");
+ ret = alloc_chrdev_region(_devt, 0, GPIO_DEV_MAX, GPIOCHIP_NAME);
if (ret < 0) {
pr_err("gpiolib: failed to allocate char dev region\n");
bus_unregister(_bus_type);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index ca9bc1e4803c2979..a4a759920faa48ab 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -16,6 +16,8 @@
#include
#include
+#define GPIOCHIP_NAME "gpiochip"
+
/**
* struct gpio_device - internal state container for GPIO devices
* @id: numerical ID number for the GPIO chip
--
2.17.1
[PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
Currently GPIO controllers can only be referred to by label in GPIO
lookup tables.
Add support for looking them up by "gpiochipN" name, with "N" either the
corresponding GPIO device's ID number, or the GPIO controller's first
GPIO number.
Signed-off-by: Geert Uytterhoeven
---
If this is rejected, the GPIO Aggregator documentation must be updated.
The second variant is currently used by the legacy sysfs interface only,
so perhaps the chip->base check should be dropped?
v3:
- New.
---
drivers/gpio/gpiolib.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c9e47620d2434983..d24a3d79dcfe69ad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1746,9 +1746,29 @@ static int gpiochip_match_name(struct gpio_chip *chip,
void *data)
return !strcmp(chip->label, name);
}
+static int gpiochip_match_id(struct gpio_chip *chip, void *data)
+{
+ int id = (uintptr_t)data;
+
+ return id == chip->base || id == chip->gpiodev->id;
+}
+
static struct gpio_chip *find_chip_by_name(const char *name)
{
- return gpiochip_find((void *)name, gpiochip_match_name);
+ struct gpio_chip *chip;
+ int id;
+
+ chip = gpiochip_find((void *)name, gpiochip_match_name);
+ if (chip)
+ return chip;
+
+ if (!str_has_prefix(name, GPIOCHIP_NAME))
+ return NULL;
+
+ if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, ))
+ return NULL;
+
+ return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
}
#ifdef CONFIG_GPIOLIB_IRQCHIP
--
2.17.1
[PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup
Currently GPIOs can only be referred to by GPIO controller and offset in
GPIO lookup tables.
Add support for looking them up by line name.
Signed-off-by: Geert Uytterhoeven
---
If this is rejected, the GPIO Aggregator documentation and code must be
updated.
v3:
- New.
---
drivers/gpio/gpiolib.c | 12
include/linux/gpio/machine.h | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d24a3d79dcfe69ad..cb608512ad6bbded 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4475,6 +4475,18 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
continue;
+ if (p->chip_hwnum == (u16)-1) {
+ desc = gpio_name_to_desc(p->chip_label);
+ if (desc) {
+ *flags = p->flags;
+ return desc;
+ }
+
+ dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+p->chip_label);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
chip = find_chip_by_name(p->chip_label);
if (!chip) {
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -31,7 +31,7 @@ enum gpio_lookup_flags {
*/
struct gpiod_lookup {
const char *chip_label;
- u16 chip_hwnum;
+ u16 chip_hwnum; /* if -1, chip_label is named line */
const char *con_id;
unsigned int idx;
unsigned long flags;
--
2.17.1
[PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section
Add a maintainership section for the GPIO Aggregator/Repeater, covering documentation, Device Tree bindings, and driver source code. Signed-off-by: Geert Uytterhoeven --- Harish: Do you want to be listed as maintainer, too? v3: - New. --- MAINTAINERS | 8 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index e5949b6827b72f2b..0f12ebdaa8faa76b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7043,6 +7043,14 @@ S: Maintained F: Documentation/firmware-guide/acpi/gpio-properties.rst F: drivers/gpio/gpiolib-acpi.c +GPIO AGGREGATOR/REPEATER +M: Geert Uytterhoeven +L: linux-g...@vger.kernel.org +S: Maintained +F: Documentation/admin-guide/gpio/gpio-aggregator.rst +F: Documentation/devicetree/bindings/gpio/gpio-repeater.yaml +F: drivers/gpio/gpio-aggregator.c + GPIO IR Transmitter M: Sean Young L: linux-me...@vger.kernel.org -- 2.17.1
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
Hi Linus,
On Thu, Sep 12, 2019 at 10:56 AM Linus Walleij wrote:
> On Fri, Jul 5, 2019 at 5:05 PM Geert Uytterhoeven
> wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > and expose them as a new gpiochip. This is useful for implementing
> > access control, and assigning a set of GPIOs to a specific user.
> > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > machine, as the VM can just grab the full virtual GPIO controller, and
> > no longer needs to care about which GPIOs to grab and which not,
> > reducing the attack surface.
> >
> > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > attribute file in sysfs:
> >
> > $ echo " [ ...]"
> >"[, [ ...]] ...]"
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > Likewise, virtual GPIO controllers can be destroyed after use:
> >
> > $ echo gpio-virt-agg. \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > Signed-off-by: Geert Uytterhoeven
> > ---
> > Aggregating GPIOs and exposing them as a new gpiochip was suggested in
> > response to my proof-of-concept for GPIO virtualization with QEMU[1][2].
> >
> > Sample session on r8a7791/koelsch:
> >
> > - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts
> >
> > - Create virtual aggregators:
> >
> > $ echo "e6052000.gpio 19 20" \
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19
> > gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20
> > gpiochip_find_base: found new base at 778
> > gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8)
> > gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8
> > (gpio-virt-agg.0)
> >
> > $ echo "e6052000.gpio 21, e605.gpio 20 21 22" \
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21
> > gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e605.gpio/20
> > gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e605.gpio/21
> > gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e605.gpio/22
> > gpiochip_find_base: found new base at 774
> > gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9)
> > gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9
> > (gpio-virt-agg.1)
> >
> > - Adjust permissions on /dev/gpiochip[89] (optional)
> >
> > - Control LEDs:
> >
> > $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
> > $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
> > $ gpioset gpiochip9 0=0 # LED8 OFF
> > $ gpioset gpiochip9 0=1 # LED8 ON
> >
> > - Destroy virtual aggregators:
> >
> > $ echo gpio-virt-agg.0 \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> > $ echo gpio-virt-agg.1 \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > Thanks for your comments!
> >
> > References:
> > - [1] "[PATCH QEMU POC] Add a GPIO backend"
> >
> > (https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+rene...@glider.be/)
> > - [2] "Getting To Blinky: Virt Edition / Making device pass-through
> > work on embedded ARM"
> > (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
>
> I'm looping in my friends at Google for this discussion.
>
> They need a virtualized gpio_chip for their Android emulator,
> and their current approach for other devices has been around
> using virtio in most cases and an emulated AC97 for the
> audio case as far as I remember.
>
> It would be great to have their input on this so we can create a
> virtualization/aggregate that works for all.
>
> Please include ade...@google.com on future postings of this!
I've sent v2 yesterday:
https://lore.kernel.org/lkml/20190911143858.13024-1-geert+rene...@glider.be/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[Qemu-devel] [PATCH/RFC v2 4/5] gpio: Add GPIO Forwarder Helper
Add a helper for creating GPIO chips that merely forward all operations
to other GPIOs.
This will be used by the GPIO Aggregator.
Signed-off-by: Geert Uytterhoeven
---
I expect this can be used by the GPIO inverter, too, after adding an
"invert" flag, or a filter function that checks which offsets need
inversion.
---
drivers/gpio/Kconfig | 3 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpiolib-fwd.c | 272 +
drivers/gpio/gpiolib-fwd.h | 16 +++
4 files changed, 292 insertions(+)
create mode 100644 drivers/gpio/gpiolib-fwd.c
create mode 100644 drivers/gpio/gpiolib-fwd.h
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 38e096e6925fa65d..29d3ce8debcca1f6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -47,6 +47,9 @@ config GPIOLIB_IRQCHIP
select IRQ_DOMAIN
bool
+config GPIOLIB_FWD
+ tristate
+
config DEBUG_GPIO
bool "Debug GPIO calls"
depends on DEBUG_KERNEL
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d2fd19c15bae3fba..8a0e685c92b69855 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib-devprop.o
obj-$(CONFIG_OF_GPIO) += gpiolib-of.o
obj-$(CONFIG_GPIO_SYSFS) += gpiolib-sysfs.o
obj-$(CONFIG_GPIO_ACPI)+= gpiolib-acpi.o
+obj-$(CONFIG_GPIOLIB_FWD) += gpiolib-fwd.o
# Device drivers. Generally keep list sorted alphabetically
obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o
diff --git a/drivers/gpio/gpiolib-fwd.c b/drivers/gpio/gpiolib-fwd.c
new file mode 100644
index ..28dac8c60a981337
--- /dev/null
+++ b/drivers/gpio/gpiolib-fwd.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// GPIO Forwarder Helper
+//
+// Copyright (C) 2019 Glider bvba
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "gpiolib.h"
+#include "gpiolib-fwd.h"
+
+struct gpiochip_fwd {
+ struct gpio_chip chip;
+ struct gpio_desc **descs;
+ union {
+ struct mutex mlock; /* protects tmp[] if can_sleep */
+ spinlock_t slock; /* protects tmp[] if !can_sleep */
+ };
+ unsigned long tmp[];/* values and descs for multiple ops */
+};
+
+static int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+ return gpiod_get_direction(fwd->descs[offset]);
+}
+
+static int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int
offset)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+ return gpiod_direction_input(fwd->descs[offset]);
+}
+
+static int gpio_fwd_direction_output(struct gpio_chip *chip,
+unsigned int offset, int value)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+ return gpiod_direction_output(fwd->descs[offset], value);
+}
+
+static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+ return gpiod_get_value(fwd->descs[offset]);
+}
+
+static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+unsigned long *bits)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+ unsigned long *values, flags;
+ struct gpio_desc **descs;
+ unsigned int i, j = 0;
+ int error;
+
+ if (chip->can_sleep)
+ mutex_lock(>mlock);
+ else
+ spin_lock_irqsave(>slock, flags);
+
+ values = >tmp[0];
+ bitmap_clear(values, 0, fwd->chip.ngpio);
+ descs = (void *)>tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
+
+ for_each_set_bit(i, mask, fwd->chip.ngpio)
+ descs[j++] = fwd->descs[i];
+
+ error = gpiod_get_array_value(j, descs, NULL, values);
+ if (!error) {
+ j = 0;
+ for_each_set_bit(i, mask, fwd->chip.ngpio)
+ __assign_bit(i, bits, test_bit(j++, values));
+ }
+
+ if (chip->can_sleep)
+ mutex_unlock(>mlock);
+ else
+ spin_unlock_irqrestore(>slock, flags);
+
+ return error;
+}
+
+static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int
value)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+ gpiod_set_value(fwd->descs[offset], value);
+}
+
+static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+ unsigned long *values, flags;
+ struct gpio_desc **descs;
+ unsigned int i, j = 0;
+
+ if (chip->can_sleep)
+ mutex_lock(>mlock);
+ else
[Qemu-devel] [PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver
Hi all,
GPIO controllers are exported to userspace using /dev/gpiochip*
character devices. Access control to these devices is provided by
standard UNIX file system permissions, on an all-or-nothing basis:
either a GPIO controller is accessible for a user, or it is not.
Currently no mechanism exists to control access to individual GPIOs.
Hence this second RFC adds a GPIO driver to aggregate existing GPIOs,
and expose them as a new gpiochip. This is useful for implementing
access control, and assigning a set of GPIOs to a specific user.
Furthermore, this simplifies and hardens exporting GPIOs to a virtual
machine, as the VM can just grab the full GPIO controller, and no longer
needs to care about which GPIOs to grab and which not, reducing the
attack surface.
Changes compared to v1[1]:
- Drop "virtual", rename to gpio-aggregator,
- Create and use new GPIO Forwarder Helper, to allow sharing code with
the GPIO inverter,
- Lift limit on the maximum number of GPIOs,
- Improve parsing of GPIO specifiers,
- Fix modular build.
To do:
- Write proper documentation.
Aggregating GPIOs and exposing them as a new gpiochip was suggested in
response to my proof-of-concept for GPIO virtualization with QEMU[2][3].
Aggregated GPIO controllers are instantiated and destroyed by writing to
atribute files in sysfs. Sample session on r8a7791/koelsch:
- Unbind LEDs from leds-gpio driver:
echo leds > /sys/bus/platform/drivers/leds-gpio/unbind
- Create aggregators:
$ echo e6052000.gpio 19,20 \
> /sys/bus/platform/drivers/gpio-aggregator/new_device
gpio-aggregator.0: gpio 0 => gpio-953 (?)
gpio-aggregator.0: gpio 1 => gpio-954 (?)
gpiochip_find_base: found new base at 778
gpio gpiochip8: (gpio-aggregator.0): added GPIO chardev (254:8)
gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8
(gpio-aggregator.0)
$ echo e6052000.gpio 21 e605.gpio 20-22 \
> /sys/bus/platform/drivers/gpio-aggregator/new_device
gpio-aggregator.1: gpio 0 => gpio-955 (?)
gpio-aggregator.1: gpio 1 => gpio-1012 (?)
gpio-aggregator.1: gpio 2 => gpio-1013 (?)
gpio-aggregator.1: gpio 3 => gpio-1014 (?)
gpiochip_find_base: found new base at 774
gpio gpiochip9: (gpio-aggregator.1): added GPIO chardev (254:9)
gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9
(gpio-aggregator.1)
- Adjust permissions on /dev/gpiochip[89] (optional)
- Control LEDs:
$ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
$ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
$ gpioset gpiochip9 0=0 # LED8 OFF
$ gpioset gpiochip9 0=1 # LED8 ON
- Destroy aggregators:
$ echo gpio-aggregator.0 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
$ echo gpio-aggregator.1 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
Thanks for your comments!
References:
- [1] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
(https://lore.kernel.org/lkml/20190705160536.12047-1-geert+rene...@glider.be/)
- [2] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+rene...@glider.be/)
- [3] "Getting To Blinky: Virt Edition / Making device pass-through
work on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
Geert Uytterhoeven (5):
gpio: Export gpiod_{request,free}() to modular GPIO code
gpio: Export gpiochip_get_desc() to modular GPIO code
gpio: Export gpio_name_to_desc() to modular GPIO code
gpio: Add GPIO Forwarder Helper
gpio: Add GPIO Aggregator Driver
drivers/gpio/Kconfig | 12 ++
drivers/gpio/Makefile | 2 +
drivers/gpio/gpio-aggregator.c | 333 +
drivers/gpio/gpiolib-fwd.c | 272 +++
drivers/gpio/gpiolib-fwd.h | 16 ++
drivers/gpio/gpiolib.c | 6 +-
drivers/gpio/gpiolib.h | 1 +
7 files changed, 641 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpio/gpio-aggregator.c
create mode 100644 drivers/gpio/gpiolib-fwd.c
create mode 100644 drivers/gpio/gpiolib-fwd.h
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[Qemu-devel] [PATCH/RFC v2 1/5] gpio: Export gpiod_{request, free}() to modular GPIO code
Export the gpiod_request() and gpiod_free() symbols, so modular GPIO
library code can make use of these functions.
Signed-off-by: Geert Uytterhoeven
---
ERROR: "gpiod_free" [drivers/gpio/gpiolib-fwd.ko] undefined!
ERROR: "gpiod_request" [drivers/gpio/gpiolib-fwd.ko] undefined!
Alternatives:
- Force gpiolib-fwd builtin,
- Call gpio_{,request,free}(desc_to_gpio(...)) instead, as the legacy
functions are exported,
- Call gpiod_put() instead of gpiod_free(), as the former is a simple
exported wrapper around the latter. Unfortunately there's no such
alternative for gpiod_request().
---
drivers/gpio/gpiolib.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f1c1b5d4b00b40a7..dbecf266be5a5a2a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2733,6 +2733,7 @@ int gpiod_request(struct gpio_desc *desc, const char
*label)
return ret;
}
+EXPORT_SYMBOL_GPL(gpiod_request);
static bool gpiod_free_commit(struct gpio_desc *desc)
{
@@ -2777,6 +2778,7 @@ void gpiod_free(struct gpio_desc *desc)
WARN_ON(extra_checks);
}
}
+EXPORT_SYMBOL_GPL(gpiod_free);
/**
* gpiochip_is_requested - return string iff signal was requested
--
2.17.1
[Qemu-devel] [PATCH/RFC v2 3/5] gpio: Export gpio_name_to_desc() to modular GPIO code
Make gpio_name_to_desc() global, and export its symbol, so modular GPIO
driver can make use of this function.
This will be used by the GPIO Aggregator.
Signed-off-by: Geert Uytterhoeven
---
drivers/gpio/gpiolib.c | 3 ++-
drivers/gpio/gpiolib.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c2cf01fb72a9de87..c0caafd111e6ff51 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -288,7 +288,7 @@ static int gpiodev_add_to_list(struct gpio_device *gdev)
/*
* Convert a GPIO name to its descriptor
*/
-static struct gpio_desc *gpio_name_to_desc(const char * const name)
+struct gpio_desc *gpio_name_to_desc(const char * const name)
{
struct gpio_device *gdev;
unsigned long flags;
@@ -315,6 +315,7 @@ static struct gpio_desc *gpio_name_to_desc(const char *
const name)
return NULL;
}
+EXPORT_SYMBOL_GPL(gpio_name_to_desc);
/*
* Takes the names from gc->names and checks if they are all unique. If they
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b8b10a409c7b9c65..240d2868a3024b52 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -117,6 +117,7 @@ struct gpio_desc {
const char *name;
};
+struct gpio_desc *gpio_name_to_desc(const char * const name);
int gpiod_request(struct gpio_desc *desc, const char *label);
void gpiod_free(struct gpio_desc *desc);
int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
--
2.17.1
[Qemu-devel] [PATCH/RFC v2 5/5] gpio: Add GPIO Aggregator Driver
GPIO controllers are exported to userspace using /dev/gpiochip*
character devices. Access control to these devices is provided by
standard UNIX file system permissions, on an all-or-nothing basis:
either a GPIO controller is accessible for a user, or it is not.
Currently no mechanism exists to control access to individual GPIOs.
Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
a new gpiochip. This is useful for implementing access control, and
assigning a set of GPIOs to a specific user.
Furthermore, this simplifies and hardens exporting GPIOs to a virtual
machine, as the VM can just grab the full GPIO controller, and no longer
needs to care about which GPIOs to grab and which not, reducing the
attack surface.
Aggregated GPIO controllers are instantiated by writing to the
"new_device" attribute file in sysfs:
$ echo [] [ ] ...
> /sys/bus/platform/drivers/gpio-aggregator/new_device
Where is a GPIO line name, is a GPIO chip label or
name, and is a comma-separated list of GPIO offsets and/or
GPIO offset ranges.
Likewise, aggregated GPIO controllers can be destroyed after use:
$ echo gpio-aggregator. \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
Signed-off-by: Geert Uytterhoeven
---
v2:
- Add missing initialization of i in gpio_virt_agg_probe(),
- Update for removed .need_valid_mask field and changed
.init_valid_mask() signature,
- Drop "virtual", rename to gpio-aggregator,
- Drop bogus FIXME related to gpiod_set_transitory() expectations,
- Use new GPIO Forwarder Helper,
- Lift limit on the maximum number of GPIOs,
- Improve parsing:
- add support for specifying GPIOs by line name,
- add support for specifying GPIO chips by ID,
- add support for GPIO offset ranges,
- names and offset specifiers must be separated by whitespace,
- GPIO offsets must separated by spaces,
- Use str_has_prefix() and kstrtouint().
I didn't use argv_split(), as it doesn't support quoted strings yet,
and GPIO line names can contain spaces. Perhaps I should enhance
argv_split() first, and use that?
---
drivers/gpio/Kconfig | 9 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-aggregator.c | 333 +
3 files changed, 343 insertions(+)
create mode 100644 drivers/gpio/gpio-aggregator.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 29d3ce8debcca1f6..058aa68fd7015e7c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1483,6 +1483,15 @@ config GPIO_VIPERBOARD
endmenu
+config GPIO_AGGREGATOR
+ tristate "GPIO Aggregator"
+ select GPIOLIB_FWD
+ help
+ This enabled the GPIO Aggregator, which provides a way to aggregate
+ existing GPIOs into a new GPIO device.
+ This is useful for assigning a collection of GPIOs to a user, or
+ exported them to a virtual machine.
+
config GPIO_MOCKUP
tristate "GPIO Testing Driver"
select IRQ_SIM
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 8a0e685c92b69855..2ec9128bcfefa40a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o
obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
+obj-$(CONFIG_GPIO_AGGREGATOR) += gpio-aggregator.o
obj-$(CONFIG_GPIO_ALTERA_A10SR)+= gpio-altera-a10sr.o
obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
new file mode 100644
index ..42485735bd823e02
--- /dev/null
+++ b/drivers/gpio/gpio-aggregator.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// GPIO Aggregator
+//
+// Copyright (C) 2019 Glider bvba
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "gpiolib.h"
+#include "gpiolib-fwd.h"
+
+#define DRV_NAME "gpio-aggregator"
+
+struct gpio_aggregator {
+ struct platform_device *pdev;
+};
+
+static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */
+static DEFINE_IDR(gpio_aggregator_idr);
+
+static int gpiochip_match_label(struct gpio_chip *chip, void *data)
+{
+ return !strcmp(chip->label, data);
+}
+
+static struct gpio_chip *gpiochip_find_by_label(const char *label)
+{
+ return gpiochip_find((void *)label, gpiochip_match_label);
+}
+
+static int gpiochip_match_id(struct gpio_chip *chip, void *data)
+{
+ unsigned int id = (uintptr_t)data;
+
+ return id == chip->base || id == chip->gpiodev->id;
+}
+
+static struct gpio_chip *gpiochip_find_by_id(const char *id)
+{
+ un
[Qemu-devel] [PATCH/RFC v2 2/5] gpio: Export gpiochip_get_desc() to modular GPIO code
Export the gpiochip_get_desc() symbol, so modular GPIO driver can make
use of this function again.
This is a partial revert of commit 1bd6b601fe196b6f ("gpio: make
gpiochip_get_desc() gpiolib-private").
Signed-off-by: Geert Uytterhoeven
---
ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-aggregator.ko] undefined!
---
drivers/gpio/gpiolib.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index dbecf266be5a5a2a..c2cf01fb72a9de87 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -95,6 +95,7 @@ static inline void desc_set_label(struct gpio_desc *d, const
char *label)
{
d->label = label;
}
+EXPORT_SYMBOL_GPL(gpiochip_get_desc);
/**
* gpio_to_desc - Convert a GPIO number to its descriptor
--
2.17.1
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
Hi Bartosz,
On Fri, Jul 12, 2019 at 11:27 AM Bartosz Golaszewski
wrote:
> wt., 9 lip 2019 o 17:59 Geert Uytterhoeven napisał(a):
> > On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
> > wrote:
> > > pon., 8 lip 2019 o 12:24 Geert Uytterhoeven
> > > napisał(a):
> > > > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > > > wrote:
> > > > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven
> > > > > napisał(a):
> > > > > > GPIO controllers are exported to userspace using /dev/gpiochip*
> > > > > > character devices. Access control to these devices is provided by
> > > > > > standard UNIX file system permissions, on an all-or-nothing basis:
> > > > > > either a GPIO controller is accessible for a user, or it is not.
> > > > > > Currently no mechanism exists to control access to individual GPIOs.
> > > > > >
> > > > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to
> > > > > > 32),
> > > > > > and expose them as a new gpiochip. This is useful for implementing
> > > > > > access control, and assigning a set of GPIOs to a specific user.
> > > > > > Furthermore, it would simplify and harden exporting GPIOs to a
> > > > > > virtual
> > > > > > machine, as the VM can just grab the full virtual GPIO controller,
> > > > > > and
> > > > > > no longer needs to care about which GPIOs to grab and which not,
> > > > > > reducing the attack surface.
> > > > > >
> > > > > > Virtual GPIO controllers are instantiated by writing to the
> > > > > > "new_device"
> > > > > > attribute file in sysfs:
> > > > > >
> > > > > > $ echo " [ ...]"
> > > > > >"[, [ ...]] ...]"
> > > > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> > > > > >
> > > > > > Likewise, virtual GPIO controllers can be destroyed after use:
> > > > > >
> > > > > > $ echo gpio-virt-agg. \
> > > > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > > Am I doing it right? I'm trying to create a device and am only getting
> > > this:
> > >
> > > # echo gpiochip2 23 > new_device
> > > [ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip
> > > gpiochip2
> > >
> > > gpiochip2 *does* exist in the system.
> >
> > Please try the name of the platform device instead.
> > I.e. for my koelsch (R-Car M2-W), it needs "e6052000.gpio" instead
> > of "gpiochip2".
> >
> > Probably the driver should match on both.
> >
> > > I see. I'll try to review it more thoroughly once I get to play with
> > > it. So far I'm stuck on creating the virtual chip.
>
> This is not a show-stopper but one thing that's bothering me in this
> is that lines used by the aggregator are considered 'used' in regard
> to the original chip. I'm wondering how much effort would it take to
> have them be 'muxed' into two (real and virtual) chips at once.
Is that really what you want?
If a GPIO is aggregated with othrs, it's intended to be used only through
the aggregator, isn't it?
> Other than that - seems to works pretty nice other than the matching
> by chip name and by line names.
Thanks, working on that...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
wrote:
> pon., 8 lip 2019 o 12:24 Geert Uytterhoeven napisał(a):
> > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > wrote:
> > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven
> > > napisał(a):
> > > > +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> > > > + unsigned int offset, unsigned long
> > > > config)
> > > > +{
> > > > + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> > > > +
> > > > + chip = priv->desc[offset]->gdev->chip;
> > > > + if (chip->set_config)
> > > > + return chip->set_config(chip, offset, config);
> > > > +
> > > > + // FIXME gpiod_set_transitory() expects success if not
> > > > implemented
> >
> > BTW, do you have a comment about this FIXME?
>
> Ha! Interesting. I'll give it a thought and respond elsewhere as it's
> a different subject.
>
> > > > + return -ENOTSUPP;
Upon closer look, this turns out to be a red herring: gpiod_set_transitory()
converts -ENOTSUPP to zero, so there is no issue.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
Hi Phil,
On Thu, Jul 11, 2019 at 11:24 AM Phil Reid wrote:
> On 10/07/2019 18:21, Geert Uytterhoeven wrote:
> > On Wed, Jul 10, 2019 at 4:00 AM Phil Reid wrote:
> >> On 6/07/2019 00:05, Geert Uytterhoeven wrote:
> >>> GPIO controllers are exported to userspace using /dev/gpiochip*
> >>> character devices. Access control to these devices is provided by
> >>> standard UNIX file system permissions, on an all-or-nothing basis:
> >>> either a GPIO controller is accessible for a user, or it is not.
> >>> Currently no mechanism exists to control access to individual GPIOs.
> >>>
> >>> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> >>> and expose them as a new gpiochip. This is useful for implementing
> >>> access control, and assigning a set of GPIOs to a specific user.
> >>> Furthermore, it would simplify and harden exporting GPIOs to a virtual
> >>> machine, as the VM can just grab the full virtual GPIO controller, and
> >>> no longer needs to care about which GPIOs to grab and which not,
> >>> reducing the attack surface.
> >>>
> >>> Virtual GPIO controllers are instantiated by writing to the "new_device"
> >>> attribute file in sysfs:
> >>>
> >>> $ echo " [ ...]"
> >>> "[, [ ...]] ...]"
> >>> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >>>
> >>> Likewise, virtual GPIO controllers can be destroyed after use:
> >>>
> >>> $ echo gpio-virt-agg. \
> >>> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >>>
> >> Would it be possible to do the lookup via line names?
> >
> > Doesn't the fact that a GPIO has a line name means that it is in use, and
> > thus cannot be aggregated and exported to another user?
>
> They can be given line names via the dt property gpio-line-names.
> Which can be used by user space to find a gpio. Not sure if there's an
> equivalent api inkerenl.
> But it looks like we can find the info via struct gpiochip_info /
> gpioline_info linfo and work
> out the chip name and line offsets. So probably not required.
>
> Find the right gpio always seems tricky.
> We have systems with multiple i2c gpio behind muxes that may or may not be
> present.
> So i2c bus numbers are never consistent. And then different board revisions
> move the
> same gpio line to a different pin (or cahnge the gpio chip type completely)
> to make routing easier etc.
OK, so extending lookup to line names makes sense.
This requires making gpio_name_to_desc() public.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
Hi Phil,
On Wed, Jul 10, 2019 at 4:00 AM Phil Reid wrote:
> On 6/07/2019 00:05, Geert Uytterhoeven wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > and expose them as a new gpiochip. This is useful for implementing
> > access control, and assigning a set of GPIOs to a specific user.
> > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > machine, as the VM can just grab the full virtual GPIO controller, and
> > no longer needs to care about which GPIOs to grab and which not,
> > reducing the attack surface.
> >
> > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > attribute file in sysfs:
> >
> > $ echo " [ ...]"
> > "[, [ ...]] ...]"
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > Likewise, virtual GPIO controllers can be destroyed after use:
> >
> > $ echo gpio-virt-agg. \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
>
> Nice.
> This provides similar functionality to the "gpio inverter" driver currently
> on the list.
> Other than being just a buffer.
Indeed, both drivers forward GPIO calls, but the gpio inverter modifies
some parameters passed.
The way the drivers obtain references to GPIOs is different, though: the
inverter driver obtains a fixed description from DT, while the virtual
aggregator receives the description at runtime, from sysfs.
But perhaps both drivers could share some code?
> Would it be possible to do the lookup via line names?
Doesn't the fact that a GPIO has a line name means that it is in use, and
thus cannot be aggregated and exported to another user?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
Hi Bartosz,
On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
wrote:
> pon., 8 lip 2019 o 12:24 Geert Uytterhoeven napisał(a):
> > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > wrote:
> > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven
> > > napisał(a):
> > > > GPIO controllers are exported to userspace using /dev/gpiochip*
> > > > character devices. Access control to these devices is provided by
> > > > standard UNIX file system permissions, on an all-or-nothing basis:
> > > > either a GPIO controller is accessible for a user, or it is not.
> > > > Currently no mechanism exists to control access to individual GPIOs.
> > > >
> > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > > > and expose them as a new gpiochip. This is useful for implementing
> > > > access control, and assigning a set of GPIOs to a specific user.
> > > > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > > > machine, as the VM can just grab the full virtual GPIO controller, and
> > > > no longer needs to care about which GPIOs to grab and which not,
> > > > reducing the attack surface.
> > > >
> > > > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > > > attribute file in sysfs:
> > > >
> > > > $ echo " [ ...]"
> > > >"[, [ ...]] ...]"
> > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> > > >
> > > > Likewise, virtual GPIO controllers can be destroyed after use:
> > > >
> > > > $ echo gpio-virt-agg. \
> > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> Am I doing it right? I'm trying to create a device and am only getting this:
>
> # echo gpiochip2 23 > new_device
> [ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2
>
> gpiochip2 *does* exist in the system.
Please try the name of the platform device instead.
I.e. for my koelsch (R-Car M2-W), it needs "e6052000.gpio" instead
of "gpiochip2".
Probably the driver should match on both.
> I see. I'll try to review it more thoroughly once I get to play with
> it. So far I'm stuck on creating the virtual chip.
Thanks, good luck!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
Hi Bartosz,
On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
wrote:
> pt., 5 lip 2019 o 18:05 Geert Uytterhoeven
> napisał(a):
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > and expose them as a new gpiochip. This is useful for implementing
> > access control, and assigning a set of GPIOs to a specific user.
> > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > machine, as the VM can just grab the full virtual GPIO controller, and
> > no longer needs to care about which GPIOs to grab and which not,
> > reducing the attack surface.
> >
> > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > attribute file in sysfs:
> >
> > $ echo " [ ...]"
> >"[, [ ...]] ...]"
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > Likewise, virtual GPIO controllers can be destroyed after use:
> >
> > $ echo gpio-virt-agg. \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > Signed-off-by: Geert Uytterhoeven
> I like the general idea and the interface looks mostly fine. Since
> this is new ABI I think it needs to be documented as well.
Sure.
> I'm having trouble building this module:
>
> CALLscripts/atomic/check-atomics.sh
> CALLscripts/checksyscalls.sh
> CHK include/generated/compile.h
> Kernel: arch/arm/boot/Image is ready
> Building modules, stage 2.
> MODPOST 235 modules
> ERROR: "gpiod_request" [drivers/gpio/gpio-virt-agg.ko] undefined!
> ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-virt-agg.ko] undefined!
> ERROR: "gpiod_free" [drivers/gpio/gpio-virt-agg.ko] undefined!
> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
> make[1]: *** [__modpost] Error 1
> Makefile:1287: recipe for target 'modules' failed
> make: *** [modules] Error 2
> make: *** Waiting for unfinished jobs
>
> I'm not sure what the problem is.
Oops. As this is an RFC, I didn't bother trying to build this driver as
a module, only builtin. Apparently the 3 symbols above are not yet
exported using EXPORT_SYMBOL_GPL().
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-virt-agg.c
> > @@ -0,0 +1,390 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// GPIO Virtual Aggregator
> > +//
> > +// Copyright (C) 2019 Glider bvba
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "gpiolib.h"
> > +
> > +#define DRV_NAME "gpio-virt-agg"
> > +#define MAX_GPIOS 32
>
> Do we really need this limit? I see it simplifies the code, but maybe
> we can allocate the relevant arrays dynamically and not limit users?
Sure. That limit can be lifted.
> > +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> > + unsigned int offset, unsigned long
> > config)
> > +{
> > + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> > +
> > + chip = priv->desc[offset]->gdev->chip;
> > + if (chip->set_config)
> > + return chip->set_config(chip, offset, config);
> > +
> > + // FIXME gpiod_set_transitory() expects success if not implemented
BTW, do you have a comment about this FIXME?
> > + return -ENOTSUPP;
> > +}
> > +static int gpio_virt_agg_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = >dev;
> > + const char *param = dev_get_platdata(dev);
> > + struct gpio_virt_agg_priv *priv;
> > + const char *label = NULL;
> > + struct gpio_chip *chip;
> > + struct gpio_desc *desc;
> > + unsigned int offset;
> > + int error, i;
>
> This 'i' here is reported as possibly not initialized:
>
> drivers/gpio/gpio-virt-agg.c: In function ‘gpio_virt_agg_probe’:
> drivers/gpio/gpio-virt-agg.c:230:13: warning: ‘i’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> int error, i;
> ^
Oops, should be preinitialized to zero. WIll fix.
> > +
[Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
GPIO controllers are exported to userspace using /dev/gpiochip* character devices. Access control to these devices is provided by standard UNIX file system permissions, on an all-or-nothing basis: either a GPIO controller is accessible for a user, or it is not. Currently no mechanism exists to control access to individual GPIOs. Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32), and expose them as a new gpiochip. This is useful for implementing access control, and assigning a set of GPIOs to a specific user. Furthermore, it would simplify and harden exporting GPIOs to a virtual machine, as the VM can just grab the full virtual GPIO controller, and no longer needs to care about which GPIOs to grab and which not, reducing the attack surface. Virtual GPIO controllers are instantiated by writing to the "new_device" attribute file in sysfs: $ echo " [ ...]" "[, [ ...]] ...]" > /sys/bus/platform/drivers/gpio-virt-agg/new_device Likewise, virtual GPIO controllers can be destroyed after use: $ echo gpio-virt-agg. \ > /sys/bus/platform/drivers/gpio-virt-agg/delete_device Signed-off-by: Geert Uytterhoeven --- Aggregating GPIOs and exposing them as a new gpiochip was suggested in response to my proof-of-concept for GPIO virtualization with QEMU[1][2]. Sample session on r8a7791/koelsch: - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts - Create virtual aggregators: $ echo "e6052000.gpio 19 20" \ > /sys/bus/platform/drivers/gpio-virt-agg/new_device gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19 gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20 gpiochip_find_base: found new base at 778 gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8) gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-virt-agg.0) $ echo "e6052000.gpio 21, e605.gpio 20 21 22" \ > /sys/bus/platform/drivers/gpio-virt-agg/new_device gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21 gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e605.gpio/20 gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e605.gpio/21 gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e605.gpio/22 gpiochip_find_base: found new base at 774 gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9) gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-virt-agg.1) - Adjust permissions on /dev/gpiochip[89] (optional) - Control LEDs: $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF $ gpioset gpiochip9 0=0 # LED8 OFF $ gpioset gpiochip9 0=1 # LED8 ON - Destroy virtual aggregators: $ echo gpio-virt-agg.0 \ > /sys/bus/platform/drivers/gpio-virt-agg/delete_device $ echo gpio-virt-agg.1 \ > /sys/bus/platform/drivers/gpio-virt-agg/delete_device Thanks for your comments! References: - [1] "[PATCH QEMU POC] Add a GPIO backend" (https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+rene...@glider.be/) - [2] "Getting To Blinky: Virt Edition / Making device pass-through work on embedded ARM" (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/) --- drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile| 1 + drivers/gpio/gpio-virt-agg.c | 390 +++ 3 files changed, 400 insertions(+) create mode 100644 drivers/gpio/gpio-virt-agg.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index f1f02dac324e52b6..8aff4d9626dee110 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1475,3 +1475,12 @@ config GPIO_MOCKUP it. endif + +config GPIO_VIRT_AGG + tristate "GPIO Virtual Aggregator" + depends on GPIOLIB + help + This enabled the GPIO Virtual Aggregator, which provides a way to + aggregate existing GPIOs into a new virtual GPIO device. + This is useful for assigning a collection of GPIOs to a user, or + exported them to a virtual machine. diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 0a494052c1e845ee..32e885b7f3aa4eee 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -152,6 +152,7 @@ obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o obj-$(CONFIG_GPIO_UNIPHIER)+= gpio-uniphier.o obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o +obj-$(CONFIG_GPIO_VIRT_AGG)+= gpio-virt-agg.o obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o obj-$(CONFIG_GPIO_WHISKEY_COVE)+= gpio-wcove.o diff --git a/drivers/gpio/gpio-virt-agg.c b/drivers/gpio/gpio-virt-
Re: [Qemu-devel] [PATCH QEMU v5] hw/arm/sysbus-fdt: Add support for instantiating generic devices
Hi Peter,
Thanks for your comments!
On Wed, Jan 9, 2019 at 5:03 PM Peter Maydell wrote:
> On Wed, 9 Jan 2019 at 15:55, Auger Eric wrote:
> > On 1/3/19 10:42 AM, Geert Uytterhoeven wrote:
> > > Add a fallback for instantiating generic devices without a type-specific
> > > or compatible-specific instantiation method. This will be used when no
> > > other match is found.
> > >
> > > Generic device instantiation avoids having to write device-specific
> > > instantiation methods for each and every "simple" device using only a
> > > set of generic properties. Devices that need more specialized handling
> > > can still provide their own instantiation methods.
>
> > > +/* Ignoring the following may or may not work, hence the warning */
> > > +{ "gpio-ranges", PROP_WARN }, /* no support for pinctrl yet */
> > > +{ "dmas",PROP_WARN }, /* no support for external DMACs
> > > yet */
> > I would be tempted to simply reject things that may not work.
>
> More generally, this whole feature seems to be "allow things that
> might not work", isn't it? Otherwise we could just have explicit
I can remove the two PROP_WARN properties above from the list, if you prefer.
Exporting rcar-sata still works fine after that.
> whitelists for the devices we want to allow passthrough of and
> that we've tested to work.
In the end, this will become a loong list (SoC x devices)...
> I have to say I'm not really very enthusiastic about
> enhancing this to allow random device passthrough,
> because it encourages further use of this. If people
> want hardware that can be passed through they should
> put it behind a bus that can be probed and can go
> behind an IOMMU, ie pci or some equivalent. That
> is a supportable hardware mechanism. All this
> machinery feels very heath-robinson...
As no-iommu suppport is not upstream (in Qemu; it is upstream in Linux,
perhaps it should be removed?), all devices using DMA require being
behind an IOMMU.
Reality is that on embedded, on-SoC devices are usually not on a PCI bus.
But IOMMUs are present, and virtualization is wanted.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [Qemu-devel] [PATCH QEMU v5] hw/arm/sysbus-fdt: Add support for instantiating generic devices
Hi Eric,
Thanks for your comments!
On Wed, Jan 9, 2019 at 4:56 PM Auger Eric wrote:
> On 1/3/19 10:42 AM, Geert Uytterhoeven wrote:
> > Add a fallback for instantiating generic devices without a type-specific
> > or compatible-specific instantiation method. This will be used when no
> > other match is found.
> >
> > Generic device instantiation avoids having to write device-specific
> > instantiation methods for each and every "simple" device using only a
> > set of generic properties. Devices that need more specialized handling
> > can still provide their own instantiation methods.
> >
> > The generic instantiation method creates a device node with remapped
> > "reg" and (optional) "interrupts" properties, and copies properties from
> > the host, if deemed appropriate:
> > - In general, properties without phandles are safe to be copied.
> > Unfortunately, the FDT does not carry type information, hence an
> > explicit whitelist is used, which can be extended when needed.
> > - Properties related to power management (power and/or clock domain),
> > isolation, and pin control, are handled by the host, and must not to
> > be copied.
> >
> > Devices nodes with subnodes are rejected, as subnodes cannot easily be
> > handled in a generic way.
> >
> > This has been tested on a Renesas Salvator-XS board (R-Car H3 ES2.0)
> > with SATA, using:
> >
> > -device vfio-platform,host=ee30.sata
> >
> > Signed-off-by: Geert Uytterhoeven
> > ---
> > Note: Also tested with GPIO, which needs "vfio: No-IOMMU mode support":
> >
> > -device vfio-platform,host=e6055400.gpio
> >
> > v5:
> > - Replace copying of a fixed list of properties (and ignoring all
> > others), by scanning all properties on the host, and deciding if
> > they need to be ignored, copied, or rejected,
> > - Reject device nodes with subnodes,
> > - Handle edge interrupts,
> >
> > v3:
> > - New.
> > --- a/hw/arm/sysbus-fdt.c
> > +++ b/hw/arm/sysbus-fdt.c
> > +static struct {
> > +const char *name;
> > +enum GenericPropertyAction action;
> > +} generic_properties[] = {
> > +{ "name",PROP_IGNORE }, /* handled automatically */
> > +{ "phandle", PROP_IGNORE }, /* not needed for the generic case
> > */
> > +
> > +/* The following are copied and remapped by dedicated code */
> > +{ "reg", PROP_IGNORE },
> > +{ "interrupts", PROP_IGNORE },
> Shouldn't interrupt-parent be safely ignored as remapped?
Probably. Typically interrupt-parent is present at a higher level.
And interrupts-extended should be ignored, too.
> > +
> > +/* The following are handled by the host */
> > +{ "power-domains", PROP_IGNORE }, /* power management (+ opt.
> > clocks) */
> > +{ "iommus", PROP_IGNORE }, /* isolation */
> > +{ "resets", PROP_IGNORE }, /* isolation */
> > +{ "pinctrl-*", PROP_IGNORE }, /* pin control */
> > +
> > +/* Ignoring the following may or may not work, hence the warning */
> > +{ "gpio-ranges", PROP_WARN }, /* no support for pinctrl yet */
> > +{ "dmas",PROP_WARN }, /* no support for external DMACs
> > yet */
> I would be tempted to simply reject things that may not work.
I kept gpio-ranges, as it works with my rcar-gpio proof of concept
(depends on with no-iommu support).
Without dmas, drivers are supposed to fall back to PIO. If a driver
doesn't support that, it will complain.
> > +
> > +/* The following are irrelevant, as corresponding specifiers are
> > ignored */
> > +{ "clock-names", PROP_IGNORE },
> > +{ "reset-names", PROP_IGNORE },
> > +{ "dma-names", PROP_IGNORE },
> > +
> > +/* Whitelist of properties not taking phandles, and thus safe to copy
> > */
> > +{ "compatible", PROP_COPY },
> > +{ "status", PROP_COPY },
> > +{ "reg-names", PROP_COPY },
> > +{ "interrupt-names", PROP_COPY },
> > +{ "#*-cells",PROP_COPY },
> > +};
> > +
> > +#ifndef CONFIG_FNMATCH
> > +/* Fall back to exact string matching instead of allowing wildcards */
> > +static inline int fnmatch(const char *pattern, const char *string, int
> > flags)
> > +{
> &
[Qemu-devel] [PATCH QEMU v5] hw/arm/sysbus-fdt: Add support for instantiating generic devices
Add a fallback for instantiating generic devices without a type-specific
or compatible-specific instantiation method. This will be used when no
other match is found.
Generic device instantiation avoids having to write device-specific
instantiation methods for each and every "simple" device using only a
set of generic properties. Devices that need more specialized handling
can still provide their own instantiation methods.
The generic instantiation method creates a device node with remapped
"reg" and (optional) "interrupts" properties, and copies properties from
the host, if deemed appropriate:
- In general, properties without phandles are safe to be copied.
Unfortunately, the FDT does not carry type information, hence an
explicit whitelist is used, which can be extended when needed.
- Properties related to power management (power and/or clock domain),
isolation, and pin control, are handled by the host, and must not to
be copied.
Devices nodes with subnodes are rejected, as subnodes cannot easily be
handled in a generic way.
This has been tested on a Renesas Salvator-XS board (R-Car H3 ES2.0)
with SATA, using:
-device vfio-platform,host=ee300000.sata
Signed-off-by: Geert Uytterhoeven
---
Note: Also tested with GPIO, which needs "vfio: No-IOMMU mode support":
-device vfio-platform,host=e6055400.gpio
v5:
- Replace copying of a fixed list of properties (and ignoring all
others), by scanning all properties on the host, and deciding if
they need to be ignored, copied, or rejected,
- Reject device nodes with subnodes,
- Handle edge interrupts,
v3:
- New.
---
hw/arm/sysbus-fdt.c | 238
1 file changed, 238 insertions(+)
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index ad698d4832c694be..52de8c9a040c282a 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -28,6 +28,9 @@
#ifdef CONFIG_LINUX
#include
#endif
+#ifdef CONFIG_FNMATCH
+#include
+#endif
#include "hw/arm/sysbus-fdt.h"
#include "qemu/error-report.h"
#include "sysemu/device_tree.h"
@@ -415,6 +418,232 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev,
void *opaque)
return 0;
}
+enum GenericPropertyAction {
+PROP_IGNORE,
+PROP_WARN,
+PROP_COPY,
+PROP_REJECT,
+};
+
+static struct {
+const char *name;
+enum GenericPropertyAction action;
+} generic_properties[] = {
+{ "name",PROP_IGNORE }, /* handled automatically */
+{ "phandle", PROP_IGNORE }, /* not needed for the generic case */
+
+/* The following are copied and remapped by dedicated code */
+{ "reg", PROP_IGNORE },
+{ "interrupts", PROP_IGNORE },
+
+/* The following are handled by the host */
+{ "power-domains", PROP_IGNORE }, /* power management (+ opt. clocks) */
+{ "iommus", PROP_IGNORE }, /* isolation */
+{ "resets", PROP_IGNORE }, /* isolation */
+{ "pinctrl-*", PROP_IGNORE }, /* pin control */
+
+/* Ignoring the following may or may not work, hence the warning */
+{ "gpio-ranges", PROP_WARN }, /* no support for pinctrl yet */
+{ "dmas",PROP_WARN }, /* no support for external DMACs yet */
+
+/* The following are irrelevant, as corresponding specifiers are ignored */
+{ "clock-names", PROP_IGNORE },
+{ "reset-names", PROP_IGNORE },
+{ "dma-names", PROP_IGNORE },
+
+/* Whitelist of properties not taking phandles, and thus safe to copy */
+{ "compatible", PROP_COPY },
+{ "status", PROP_COPY },
+{ "reg-names", PROP_COPY },
+{ "interrupt-names", PROP_COPY },
+{ "#*-cells",PROP_COPY },
+};
+
+#ifndef CONFIG_FNMATCH
+/* Fall back to exact string matching instead of allowing wildcards */
+static inline int fnmatch(const char *pattern, const char *string, int flags)
+{
+return strcmp(pattern, string);
+}
+#endif
+
+static enum GenericPropertyAction get_generic_property_action(const char *name)
+{
+unsigned int i;
+
+for (i = 0; i < ARRAY_SIZE(generic_properties); i++) {
+if (!fnmatch(generic_properties[i].name, name, 0)) {
+return generic_properties[i].action;
+}
+}
+
+/*
+ * Unfortunately DT properties do not carry type information,
+ * so we have to assume everything else contains a phandle,
+ * and must be rejected
+ */
+return PROP_REJECT;
+}
+
+/**
+ * copy_generic_node
+ *
+ * Copy the generic part of a DT node from the host
+ */
+static void copy_generic_node(void *host_fdt, void *guest_fdt, char *host_path,
+ char *guest_path)
+{
+int node, prop, len;
+
Re: [Qemu-devel] [PATCH v2 for-3.1] hw/arm/sysbus-fdt: Only call match_fn callback if the type matches
Hi Eric,
On Tue, Nov 6, 2018 at 7:42 PM Eric Auger wrote:
> Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT
> compatible value) introduced a match_fn callback which gets called
> for each registered combo to check whether a sysbus device can be
> dynamically instantiated. However the callback gets called even if
> the device type does not match the binding combo typename field.
> This causes an assert when passing "-device ramfb" to the qemu
> command line as vfio_platform_match() gets called on a non
> vfio-platform device.
>
> To fix this regression, let's change the add_fdt_node() logic so
> that we first check the type and if the match_fn callback is defined,
> then we also call it.
>
> Binding combos only requesting a type check do not define the
> match_fn callback.
>
> Fixes: af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with
> DT compatible value)
>
> Signed-off-by: Eric Auger
> Reported-by: Thomas Huth
>
> ---
>
> v1 -> v2:
> - use "if (!iter->match_fn || iter->match_fn(sbdev, iter)) {"
> as suggested by Peter to avoid code duplication
> - mention the ramfb regression fixed by this patch in the
> commit message.
Thanks for looking into this!
After applying "hw/arm/sysbus-fdt: Add support for instantiating generic
devices", "-device vfio-platform,host=ee30.sata" still works fine.
Tested-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[Qemu-devel] [PATCH QEMU POC] Add a GPIO backend
This is a Proof-of-Concept for a GPIO backend, which allows to connect
virtual GPIOs on the guest to physical GPIOs on the host. This allows
the guest to control any external device connected to the physical
GPIOs.
Features and limitations:
- The backend uses libgpiod on Linux,
- For now only GPIO outputs are supported,
- The frontend is currently hardcoded to be the PL061 GPIO controller
on the arm virtual machine. As the generic qdev_connect_gpio_out()
API call is used, any virtual GPIO controller could do, though.
Future work:
- Adding a user-instantiable GPIO frontend (or is any of the existing
virtualized GPIO controllers already user-instantiable?),
- Proper frontend/backend interface using IDs,
- Adding a QEMU internal API for controlling multiple GPIOs at once,
- Defining an API for GPIO paravirtualization,
- ...
Example:
To connect the first three GPIOs of the virtual PL061 GPIO controller
to the GPIOs controlling the three LEDs on the Renesas Salvator-X(S)
board, add the following to your qemu command invocation:
-gpiodev e6055400.gpio,vgpios=0:1:2,gpios=11:12:13
After that, the guest can cycle through the three LEDs using:
for i in $(seq 504 506); do echo $i > /sys/class/gpio/export; done
while /bin/true; do
for i in $(seq 504 506); do
echo high > /sys/class/gpio/gpio$i/direction
sleep 1
echo low > /sys/class/gpio/gpio$i/direction
done
done
Signed-off-by: Geert Uytterhoeven
---
Thanks for your comments!
---
backends/Makefile.objs | 2 +
backends/gpiodev.c | 183 +++
configure| 29 +++
hw/arm/virt.c| 6 ++
include/sysemu/gpiodev.h | 11 +++
qemu-options.hx | 20 +
vl.c | 27 ++
7 files changed, 278 insertions(+)
create mode 100644 backends/gpiodev.c
create mode 100644 include/sysemu/gpiodev.h
diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index 717fcbdae4715db1..2b5f68fedd40bea0 100644
--- a/backends/Makefile.objs
+++ b/backends/Makefile.objs
@@ -16,3 +16,5 @@ common-obj-$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX))
+= \
endif
common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
+
+common-obj-$(CONFIG_GPIO) += gpiodev.o
diff --git a/backends/gpiodev.c b/backends/gpiodev.c
new file mode 100644
index ..8d90e150f5472463
--- /dev/null
+++ b/backends/gpiodev.c
@@ -0,0 +1,183 @@
+/*
+ * QEMU GPIO Backend
+ *
+ * Copyright (C) 2018 Glider bvba
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include
+#include
+
+#include "qemu/osdep.h"
+#include "qemu/config-file.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+
+#include "sysemu/gpiodev.h"
+
+#include "hw/irq.h"
+#include "hw/qdev-core.h"
+
+DeviceState *the_pl061_dev;
+
+static void gpio_irq_handler(void *opaque, int n, int level)
+{
+struct gpiod_line *line = opaque;
+int status;
+
+status = gpiod_line_set_value(line, level);
+if (status < 0) {
+struct gpiod_chip *chip = gpiod_line_get_chip(line);
+
+error_report("%s/%s: Cannot set GPIO line %u: %s",
+ gpiod_chip_name(chip), gpiod_chip_label(chip),
+ gpiod_line_offset(line), strerror(errno));
+}
+}
+
+static int gpio_connect_line(unsigned int vgpio, struct gpiod_chip *chip,
+ unsigned int gpio)
+{
+const char *name = gpiod_chip_name(chip);
+const char *label = gpiod_chip_label(chip);
+struct gpiod_line *line;
+qemu_irq irq;
+int status;
+
+if (!the_pl061_dev) {
+error_report("PL061 GPIO controller not available");
+return -1;
+}
+
+line = gpiod_chip_get_line(chip, gpio);
+if (!line) {
+error_report("%s/%s: Cannot obtain GPIO line %u: %s", name, label,
+ gpio, strerror(errno));
+return -1;
+}
+
+status = gpiod_line_request_output(line, "qemu", 0);
+if (status < 0) {
+error_report("%s/%s: Cannot request GPIO line %u for output: %s", name,
+ label, gpio, strerror(errno));
+return -1;
+}
+
+irq = qemu_allocate_irq(gpio_irq_handler, line, 0);
+qdev_connect_gpio_out(the_pl061_dev, vgpio, irq);
+
+info_report("%s/%s: Connected PL061 GPIO %u to GPIO line %u", name, label,
+vgpio, gpio);
+return 0;
+}
+
+static int gpio_count_gpios(const char *opt)
+{
+unsigned int len = 0;
+unsigned int n = 0;
+
+do {
+switch (*opt) {
+case '0' ... '9':
+len++;
+break;
+
+case ':':
+case '\0':
+if (!len) {
+return -1;
+}
+
+n++
Re: [Qemu-devel] [PATCH qemu v2] hw/char/sh_serial: Add timeout handling to unbreak serial input
Hi Paolo,
On Tue, Sep 11, 2018 at 3:11 PM Paolo Bonzini wrote:
> On 05/09/2018 15:11, Geert Uytterhoeven wrote:
> > As of commit 18e8cf159177100e ("serial: sh-sci: increase RX FIFO trigger
> > defaults for (H)SCIF") in Linux v4.11-rc1, the serial console on the
> > QEMU SH4 target is broken: it delays serial input until enough data has
> > been received.
> >
> > Since aforementioned commit, the Linux SCIF driver programs the Receive
> > FIFO Data Count Trigger bits in the FIFO Control Register, to postpone
> > generating a receive interrupt until:
> > 1. At least the receive trigger count of bytes of data are available
> > in the receive FIFO, OR
> > 2. No further data has been received for at least 15 etu after the
> > last received data.
> >
> > While QEMU implements the former, it does not implement the latter.
> > Hence the receive interrupt is not generated until the former condition
> > is met.
> >
> > Fix this by adding basic timeout handling. As the QEMU SCIF emulation
> > ignores any serial speed programming, the timeout value used conforms to
> > a default speed of 9600 bps, which is fine for any interactive console.
> >
> > Reported-by: Rob Landley
> > Signed-off-by: Geert Uytterhoeven
> > Tested-by: Ulrich Hecht
> > Tested-by: Rob Landley
> > Tested-by: Rich Felker
> Queued, thanks.
Does that mean it should show up in qemu.git anytime soon?
Thanks, and have a nice weekend!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[Qemu-devel] [PATCH qemu v5 0/3] vfio/sysbus-fdt: Prepare for Generic DT Pass-Through
Hi all,
This patch series prepares for exporting generic devices in DT using
vfio-platform, providing direct access from a QEMU+KVM guest to the
exported devices.
- Patches 1-2 (submitted before by Eric Auger) make the vfio-platform
device non-abstract, incl. matching using a compatible string.
- Patch 3 allows dynamic vfio-platform devices again, without needing to
create device-specific vfio types for each and every new device.
This will avoid having to write device-specific instantation methods for
each and every "simple" device using only a set of generic properties.
Devices that need more specialized handling will still be able provide
their own instantation methods.
Note that this series no longer contains "[PATCH 4/4] hw/arm/sysbus-fdt:
Add support for instantiating generic devices", following advice from Eric
Auger, as that patch needs more safeguards. Hence this series now contains
only preparative work.
Changes compared to v4 ("vfio/sysbus-fdt: Prepare for Generic DT Pass-Through",
http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg01672.html):
- Add Reviewed-by, Tested-by,
- Fix path leak on error,
- s/instantiatable/instantiable/,
- Drop reference to commit 6f2062b9758ebc64 ("hw/arm/virt: Allow only
supported dynamic sysbus devices").
Changes compared to v3 ("hw/arm/sysbus-fdt: Generic DT Pass-Through"),
http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg05006.html):
- Propagate g_file_get_contents() errors through errp,
- Add Tested-by (with amd-xgbe),
- s/From now on/Soon/ in patch description,
- s/sysbus/vfio-platform/ in patch description,
- Postpone "[PATCH 4/4] hw/arm/sysbus-fdt: Add support for instantiating
generic devices".
Changes compared to v2 (not submitted to the mailing list):
- Use the compatible values from sysfs instead of user-supplied
manufacturer and model options,
- Replace "hw/arm/sysbus-fdt: Enable rcar-gen3-gpio dynamic
instatiation" by generic "hw/arm/sysbus-fdt: Add support for
instantiating generic devices",
- Reword patch descriptions,
- Drop RFC state,
- Drop "vfio: No-IOMMU mode support".
Changes compared to v1 ("R-Car Gen3 GPIO Pass-Through Prototype (QEMU)",
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02716.html):
- Restrict dynamic sysbus devices to TYPE_VFIO_PLATFORM, as suggested
by Eric Auger.
This (plus the postponed "hw/arm/sysbus-fdt: Add support for instantiating
generic devices") has been tested on a Renesas Salvator-XS board with R-Car
H3 ES2.0 with SATA:
-device vfio-platform,host=ee30.sata
and GPIO (needs VFIO No-IOMMU support):
-device vfio-platform,host=e6055400.gpio
Thanks for applying!
Auger Eric (2):
vfio/platform: Make the vfio-platform device non-abstract
hw/arm/sysbus-fdt: Allow device matching with DT compatible value
Geert Uytterhoeven (1):
hw/arm/virt: Allow dynamic vfio-platform devices again
hw/arm/sysbus-fdt.c | 61 +
hw/arm/virt.c | 1 +
hw/vfio/amd-xgbe.c | 1 +
hw/vfio/calxeda-xgmac.c | 1 +
hw/vfio/platform.c | 25 +-
include/hw/vfio/vfio-platform.h | 3 +-
6 files changed, 76 insertions(+), 16 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
