setting gpio-line-names in dts for sama5d2 SoC

setting gpio-line-names in dts for sama5d2 SoC

[Alexander Dahl]

Hei hei,

for a Microchip SAMA5D2 SoC based device I try to set gpio-line-names in .dts file like this:

257 &pioA {                                                                 
258         gpio-line-names = "",                   /* PA0  */              
259                           "",                   /* PA1  */              
260                           "",                   /* PA2  */              
261                           "FOO",                /* PA3  */              
262                           "BAR",                /* PA4  */              
263                           "",                   /* PA5  */              

… and so on for all 4 * 32 GPIO Pins. However when calling `gpioinfo` in userspace, I always get this:

$ gpioinfo 
gpiochip0 - 128 lines:
        line   0:        "PA0"       unused   input  active-high 
        line   1:        "PA1"       unused   input  active-high 
        line   2:        "PA2"       unused   input  active-high 
        line   3:        "PA3"       unused   input  active-high 
        line   4:        "PA4"       unused   input  active-high 
        line   5:        "PA5"       unused   input  active-high 

… and so on. Those "PA0" line names are set by the pinctrl driver in drivers/pinctrl/pinctrl-at91-pio4.c before calling gpiochip_add_data() and from reading the code in drivers/gpio/gpiolib.c I suspect devprop_gpiochip_set_names() is never called then, so those names in .dts are simply ignored.

Those default names from the at91 pio4 pinctrl driver are certainly correct speaking of pin names, but from a userspace point of view it would be better if I could override those from dts, so an application using libgpiod could look for a name like e.g. "VALVE7_EN" without caring to which pin that's actually connected. Can I override those with a currently present kernel? Or is it a problem in either the at91 pio4 driver or the gpiolib core? Or is that no real usecase and should I do it differently?

Greets
Alex

Re: setting gpio-line-names in dts for sama5d2 SoC

[Andy Shevchenko]

+Cc: Bart, Linus

On Wed, Mar 17, 2021 at 1:22 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
>
> On Tuesday, March 16, 2021, Alexander Dahl <ada@thorsis.com> wrote:
>>
>> Hei hei,
>>
>> for a Microchip SAMA5D2 SoC based device I try to set gpio-line-names in .dts file like this:
>>
>> 257 &pioA {
>> 258         gpio-line-names = "",                   /* PA0  */
>> 259                           "",                   /* PA1  */
>> 260                           "",                   /* PA2  */
>> 261                           "FOO",                /* PA3  */
>> 262                           "BAR",                /* PA4  */
>> 263                           "",                   /* PA5  */
>>
>> … and so on for all 4 * 32 GPIO Pins. However when calling `gpioinfo` in userspace, I always get this:
>>
>> $ gpioinfo
>> gpiochip0 - 128 lines:
>>         line   0:        "PA0"       unused   input  active-high
>>         line   1:        "PA1"       unused   input  active-high
>>         line   2:        "PA2"       unused   input  active-high
>>         line   3:        "PA3"       unused   input  active-high
>>         line   4:        "PA4"       unused   input  active-high
>>         line   5:        "PA5"       unused   input  active-high
>>
>> … and so on. Those "PA0" line names are set by the pinctrl driver in drivers/pinctrl/pinctrl-at91-pio4.c before calling gpiochip_add_data() and from reading the code in drivers/gpio/gpiolib.c I suspect devprop_gpiochip_set_names() is never called then, so those names in .dts are simply ignored.
>>
>> Those default names from the at91 pio4 pinctrl driver are certainly correct speaking of pin names, but from a userspace point of view it would be better if I could override those from dts, so an application using libgpiod could look for a name like e.g. "VALVE7_EN" without caring to which pin that's actually connected. Can I override those with a currently present kernel? Or is it a problem in either the at91 pio4 driver or the gpiolib core? Or is that no real usecase and should I do it differently?
>
>
> I would like to know the consequences if allow this, but to me it sounds like a bug in the gpiolib.c.
>
>>
>> Greets
>> Alex
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
With Best Regards,
Andy Shevchenko

Re: setting gpio-line-names in dts for sama5d2 SoC

[Alexander Dahl]

Hei hei,

meanwhile I tried to proof my hypothesis. See below.

> Andy Shevchenko <andy.shevchenko@gmail.com> hat am 17.03.2021 13:33 geschrieben:
> 
>  
> +Cc: Bart, Linus
> 
> On Wed, Mar 17, 2021 at 1:22 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> >
> >
> > On Tuesday, March 16, 2021, Alexander Dahl <ada@thorsis.com> wrote:
> >>
> >> Hei hei,
> >>
> >> for a Microchip SAMA5D2 SoC based device I try to set gpio-line-names in .dts file like this:
> >>
> >> 257 &pioA {
> >> 258         gpio-line-names = "",                   /* PA0  */
> >> 259                           "",                   /* PA1  */
> >> 260                           "",                   /* PA2  */
> >> 261                           "FOO",                /* PA3  */
> >> 262                           "BAR",                /* PA4  */
> >> 263                           "",                   /* PA5  */
> >>
> >> … and so on for all 4 * 32 GPIO Pins. However when calling `gpioinfo` in userspace, I always get this:
> >>
> >> $ gpioinfo
> >> gpiochip0 - 128 lines:
> >>         line   0:        "PA0"       unused   input  active-high
> >>         line   1:        "PA1"       unused   input  active-high
> >>         line   2:        "PA2"       unused   input  active-high
> >>         line   3:        "PA3"       unused   input  active-high
> >>         line   4:        "PA4"       unused   input  active-high
> >>         line   5:        "PA5"       unused   input  active-high
> >>
> >> … and so on. Those "PA0" line names are set by the pinctrl driver in drivers/pinctrl/pinctrl-at91-pio4.c before calling gpiochip_add_data() and from reading the code in drivers/gpio/gpiolib.c I suspect devprop_gpiochip_set_names() is never called then, so those names in .dts are simply ignored.
> >>
> >> Those default names from the at91 pio4 pinctrl driver are certainly correct speaking of pin names, but from a userspace point of view it would be better if I could override those from dts, so an application using libgpiod could look for a name like e.g. "VALVE7_EN" without caring to which pin that's actually connected. Can I override those with a currently present kernel? 

I don't think I can override with a recent kernel.

>>> Or is it a problem in either the at91 pio4 driver or the gpiolib core? Or is that no real usecase and should I do it differently?

If I disable the name assignment in the pinctrl-at91-pio4 driver, I get the line names from the dts. I used the following diff:


diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index 578b387100d9..f994a2468cc3 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -1095,7 +1095,9 @@ static int atmel_pinctrl_probe(struct platform_device *pdev)
        atmel_pioctrl->gpio_chip->ngpio = atmel_pioctrl->npins;
        atmel_pioctrl->gpio_chip->label = dev_name(dev);
        atmel_pioctrl->gpio_chip->parent = dev;
+#if 0
        atmel_pioctrl->gpio_chip->names = atmel_pioctrl->group_names;
+#endif
 
        atmel_pioctrl->pm_wakeup_sources = devm_kcalloc(dev,
                        atmel_pioctrl->nbanks,


Now gpioinfo shows what I set in dts, more or less similar to this:

gpiochip0 - 128 lines:
        line   0:      unnamed       unused   input  active-high 
        line   1:      unnamed       unused   input  active-high 
        line   2:      unnamed       unused   input  active-high 
        line   3:        "FOO"       unused   input  active-high 
        line   4:        "BAR"       unused   input  active-high 
        line   5:      unnamed       unused   input  active-high 

> >
> >
> > I would like to know the consequences if allow this, but to me it sounds like a bug in the gpiolib.c.

At least the documentation recommends to not use pin names: https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio.txt

If anyone has an idea how to fix this, let me know. I can try to make a patch then. Currently however, I'm not familiar enough with the gpio subsystem to just start that.

Greets
Alex

Re: setting gpio-line-names in dts for sama5d2 SoC

[Andy Shevchenko]

+Cc: Kent (sorry, forgot to include you earlier)

On Thu, Mar 18, 2021 at 11:50 AM Alexander Dahl <ada@thorsis.com> wrote:
> > Andy Shevchenko <andy.shevchenko@gmail.com> hat am 17.03.2021 13:33 geschrieben:
> > On Wed, Mar 17, 2021 at 1:22 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tuesday, March 16, 2021, Alexander Dahl <ada@thorsis.com> wrote:

...

> > >> for a Microchip SAMA5D2 SoC based device I try to set gpio-line-names in .dts file like this:
> > >>
> > >> 257 &pioA {
> > >> 258         gpio-line-names = "",                   /* PA0  */
> > >> 259                           "",                   /* PA1  */
> > >> 260                           "",                   /* PA2  */
> > >> 261                           "FOO",                /* PA3  */
> > >> 262                           "BAR",                /* PA4  */
> > >> 263                           "",                   /* PA5  */
> > >>
> > >> … and so on for all 4 * 32 GPIO Pins. However when calling `gpioinfo` in userspace, I always get this:
> > >>
> > >> $ gpioinfo
> > >> gpiochip0 - 128 lines:
> > >>         line   0:        "PA0"       unused   input  active-high
> > >>         line   1:        "PA1"       unused   input  active-high
> > >>         line   2:        "PA2"       unused   input  active-high
> > >>         line   3:        "PA3"       unused   input  active-high
> > >>         line   4:        "PA4"       unused   input  active-high
> > >>         line   5:        "PA5"       unused   input  active-high
> > >>
> > >> … and so on. Those "PA0" line names are set by the pinctrl driver in drivers/pinctrl/pinctrl-at91-pio4.c before calling gpiochip_add_data() and from reading the code in drivers/gpio/gpiolib.c I suspect devprop_gpiochip_set_names() is never called then, so those names in .dts are simply ignored.
> > >>
> > >> Those default names from the at91 pio4 pinctrl driver are certainly correct speaking of pin names, but from a userspace point of view it would be better if I could override those from dts, so an application using libgpiod could look for a name like e.g. "VALVE7_EN" without caring to which pin that's actually connected. Can I override those with a currently present kernel?
>
> I don't think I can override with a recent kernel.
>
> >>> Or is it a problem in either the at91 pio4 driver or the gpiolib core? Or is that no real usecase and should I do it differently?
>
> If I disable the name assignment in the pinctrl-at91-pio4 driver, I get the line names from the dts. I used the following diff:

...

> Now gpioinfo shows what I set in dts, more or less similar to this:
>
> gpiochip0 - 128 lines:
>         line   0:      unnamed       unused   input  active-high
>         line   1:      unnamed       unused   input  active-high
>         line   2:      unnamed       unused   input  active-high
>         line   3:        "FOO"       unused   input  active-high
>         line   4:        "BAR"       unused   input  active-high
>         line   5:      unnamed       unused   input  active-high
>
> > > I would like to know the consequences if allow this, but to me it sounds like a bug in the gpiolib.c.
>
> At least the documentation recommends to not use pin names: https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio.txt
>
> If anyone has an idea how to fix this, let me know. I can try to make a patch then. Currently however, I'm not familiar enough with the gpio subsystem to just start that.

You may look into the commit
7cba1a4d5e16 ("gpiolib: generalize devprop_gpiochip_set_names() for
device properties")
which unifies the logic, but at the same time removed one comment:

-       /* If the chip defines names itself, these take precedence */

So, logic was that for a long time. And unfortunately I don't see how
we may change this without breakage. This backs us to the discussion
if the name of the line is ABI or not.

-- 
With Best Regards,
Andy Shevchenko

Re: setting gpio-line-names in dts for sama5d2 SoC

[Alexander Dahl]

Hello Andy,

> Andy Shevchenko <andy.shevchenko@gmail.com> hat am 18.03.2021 13:40 geschrieben:
> You may look into the commit
> 7cba1a4d5e16 ("gpiolib: generalize devprop_gpiochip_set_names() for
> device properties")
> which unifies the logic, but at the same time removed one comment:
> 
> -       /* If the chip defines names itself, these take precedence */

Well, that could be added again. And if that precedence stays as is, it should be documented in the gpio devicetree bindings to not cause further confusion?

> So, logic was that for a long time. And unfortunately I don't see how
> we may change this without breakage. This backs us to the discussion
> if the name of the line is ABI or not.

I want to point out one point for consideration. There's usually a difference between the SoC and the boards using that SoC. So those pins are all the same from the chips point of view and the default names are set in a driver related to the chip (family).  However the line names set in device tree are probably related to a board.  Obviously different boards may have different line names, even when using the same SoC.

In other words: if I design a new board, there's no interface I would break, at least not from the userspace point of view just looking at the line names, or is there?

Thanks for looking into this.

Greets
Alex

Re: setting gpio-line-names in dts for sama5d2 SoC

[Andy Shevchenko]

On Thu, Mar 18, 2021 at 3:56 PM Alexander Dahl <ada@thorsis.com> wrote:
> > Andy Shevchenko <andy.shevchenko@gmail.com> hat am 18.03.2021 13:40 geschrieben:
> > You may look into the commit
> > 7cba1a4d5e16 ("gpiolib: generalize devprop_gpiochip_set_names() for
> > device properties")
> > which unifies the logic, but at the same time removed one comment:
> >
> > -       /* If the chip defines names itself, these take precedence */
>
> Well, that could be added again. And if that precedence stays as is, it should be documented in the gpio devicetree bindings to not cause further confusion?
>
> > So, logic was that for a long time. And unfortunately I don't see how
> > we may change this without breakage. This backs us to the discussion
> > if the name of the line is ABI or not.
>
> I want to point out one point for consideration. There's usually a difference between the SoC and the boards using that SoC. So those pins are all the same from the chips point of view and the default names are set in a driver related to the chip (family).  However the line names set in device tree are probably related to a board.  Obviously different boards may have different line names, even when using the same SoC.
>
> In other words: if I design a new board, there's no interface I would break, at least not from the userspace point of view just looking at the line names, or is there?

I understand that  names are platform specific and I'm on your side as
an IoT specialist, but on the Linux side we may not blindly do changes
like this due to ABI concerns. You see the problem here: you can't fix
all (legacy) DTSs in the world which do not have GPIO line names
listed, and for those users we effectively break their tools and
scripts which relies on the hard coded naming scheme.

So, we may change if and only if the names of the lines are not an ABI!

What we can do as a workaround is to reverse the partsing order, and
if somebody complains, it will be their DTS issues, whoever not the
best solution either.

+Cc: Rob to hear his opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: setting gpio-line-names in dts for sama5d2 SoC

[Linus Walleij]

On Thu, Mar 18, 2021 at 4:00 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 18, 2021 at 3:56 PM Alexander Dahl <ada@thorsis.com> wrote:

> > In other words: if I design a new board, there's no interface I would break, at least not from the userspace point of view just looking at the line names, or is there?
>
> I understand that  names are platform specific and I'm on your side as
> an IoT specialist, but on the Linux side we may not blindly do changes
> like this due to ABI concerns. You see the problem here: you can't fix
> all (legacy) DTSs in the world which do not have GPIO line names
> listed, and for those users we effectively break their tools and
> scripts which relies on the hard coded naming scheme.
>
> So, we may change if and only if the names of the lines are not an ABI!
>
> What we can do as a workaround is to reverse the partsing order, and
> if somebody complains, it will be their DTS issues, whoever not the
> best solution either.

The line names are not really supposed to be used as ABI since
they are not necessarily unique. This problem was pointed out
by Johan a few times. The real ABI is the HW line number (local
offset) when used with the GPIO character device.

This is logical since the gpio-line-names attribute is optional.

It must be possible to get a handle to a GPIO line without a
name. And using the offset number should be the preferred
way for userspace: use a number on a gpiochipN with the
character device.

(The topology of the gpiochip per se can be determined from sysfs.)

However this has not stopped userspace from using it as ABI,
and these names are used by the GPIO aggregator.

It's hard to do it right without resetting history and requireing that
all names in the device tree be unique as well, at the root of the
problem, but when introduced, these names were there for
the file /sys/kernel/debug/gpio, not for exposing to userspace.

However the character device does give the name away, as
a piece of information, it is not supposed to be used to look
up lines but merely as information, but well people use it
for looking up lines by name anyway so what can we do.

I don't think it's a big deal to change these names.

Yours,
Linus Walleij