Requesting as a GPIO a pin already used through pinctrl

Requesting as a GPIO a pin already used through pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

Hi,

I just came across an interesting issue on our Allwinner SoCs.

We have, like most of the SoCs these days, a bunch of boards that go
through pinctrl to ensure their exclusive usage for a particular
device. If a second pinctrl user comes in and tries to request one of
those pins, it gets an error. Everything works fine.

However, things are getting weird when you have that requested pin
assigned to one device, and you try to export the GPIO on that pin
(through sysfs for example, but given the implementation, I think that
it would work alike by calling gpiod_request).

In this case, you get no error, and the GPIO is indeed exported,
allowing the user to change the direction and / or value of the pin,
taking away that pin from its device.

It seems from a look at the code that the request and
gpio_request_enable callbacks in the pinmux_ops, called in
pin_request, were meant to prevent such a thing.

But looking at all the drivers around, no one seems to take that case
into consideration, and this has been confirmed on an AT91 (which uses
an entirely different driver).

I have the feeling that the core should prevent that, making sure that
the gpiod_request returns EBUSY in such a case, but I'm not really
sure whether it's the case or not, and if it is, where that check is
happening.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Requesting as a GPIO a pin already used through pinctrl

[Linus Walleij]

On Fri, Sep 16, 2016 at 3:58 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

> However, things are getting weird when you have that requested pin
> assigned to one device, and you try to export the GPIO on that pin
> (through sysfs for example,

DON'T use sysfs. Use the new chardev ABI which is by the way enabled
by default.

(But you will face the same issue there I guess.)

> but given the implementation, I think that
> it would work alike by calling gpiod_request).

Yes

> In this case, you get no error, and the GPIO is indeed exported,
> allowing the user to change the direction and / or value of the pin,
> taking away that pin from its device.

If and only if the pin controller does not specify .strict in
struct pinmux_ops.

> I have the feeling that the core should prevent that, making sure that
> the gpiod_request returns EBUSY in such a case, but I'm not really
> sure whether it's the case or not, and if it is, where that check is
> happening.

- Did you try specifying .strict for the pinmux?

- Did you read Documentation/pinctrl.txt, section titled
  "GPIO mode pitfalls"?

Yours,
Linus Walleij

Re: Requesting as a GPIO a pin already used through pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

Hi Linus,

Thanks for your reply.

On Sun, Sep 18, 2016 at 01:30:24PM +0200, Linus Walleij wrote:
> On Fri, Sep 16, 2016 at 3:58 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> 
> > However, things are getting weird when you have that requested pin
> > assigned to one device, and you try to export the GPIO on that pin
> > (through sysfs for example,
> 
> DON'T use sysfs. Use the new chardev ABI which is by the way enabled
> by default.
> 
> (But you will face the same issue there I guess.)

Yeah, well, we could re-do the discussion on ksummit-discuss :)

> > but given the implementation, I think that
> > it would work alike by calling gpiod_request).
> 
> Yes
> 
> > In this case, you get no error, and the GPIO is indeed exported,
> > allowing the user to change the direction and / or value of the pin,
> > taking away that pin from its device.
> 
> If and only if the pin controller does not specify .strict in
> struct pinmux_ops.
> 
> > I have the feeling that the core should prevent that, making sure that
> > the gpiod_request returns EBUSY in such a case, but I'm not really
> > sure whether it's the case or not, and if it is, where that check is
> > happening.
> 
> - Did you try specifying .strict for the pinmux?
> 
> - Did you read Documentation/pinctrl.txt, section titled
>   "GPIO mode pitfalls"?

Sigh. Sorry for that, I should learn to read the documentation. This
is obviously the right thing to do.

However, it does have an unexpected side-effect. On our DT, for the
GPIOs, we also set up a pinctrl node (which seem to be along the lines
of the doc recommandations, section "Drivers needing both pin control
and GPIOs").

However, when pinctrl_select_default is called by the core, which in
turns ends up calling pinmux_enable_setting, which builds the owner
name using the dev_name. However, when we call gpiod_request, it ends
up in pinmux_request_gpio, which build the owner string using the
pinctrl device name and the pin number.

This results in a mismatch of owners, and the gpiod_request fails,
while the device really is the same.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Requesting as a GPIO a pin already used through pinctrl

[Michael Welling]

On Wed, Sep 21, 2016 at 10:51:28PM +0300, Maxime Ripard wrote:
> Hi Linus,
> 
> Thanks for your reply.
> 
> On Sun, Sep 18, 2016 at 01:30:24PM +0200, Linus Walleij wrote:
> > On Fri, Sep 16, 2016 at 3:58 PM, Maxime Ripard
> > <maxime.ripard@free-electrons.com> wrote:
> > 
> > > However, things are getting weird when you have that requested pin
> > > assigned to one device, and you try to export the GPIO on that pin
> > > (through sysfs for example,
> > 
> > DON'T use sysfs. Use the new chardev ABI which is by the way enabled
> > by default.
> > 
> > (But you will face the same issue there I guess.)
> 
> Yeah, well, we could re-do the discussion on ksummit-discuss :)

Are you guys trolling me?

> 
> > > but given the implementation, I think that
> > > it would work alike by calling gpiod_request).
> > 
> > Yes
> > 
> > > In this case, you get no error, and the GPIO is indeed exported,
> > > allowing the user to change the direction and / or value of the pin,
> > > taking away that pin from its device.
> > 
> > If and only if the pin controller does not specify .strict in
> > struct pinmux_ops.
> > 
> > > I have the feeling that the core should prevent that, making sure that
> > > the gpiod_request returns EBUSY in such a case, but I'm not really
> > > sure whether it's the case or not, and if it is, where that check is
> > > happening.
> > 
> > - Did you try specifying .strict for the pinmux?
> > 
> > - Did you read Documentation/pinctrl.txt, section titled
> >   "GPIO mode pitfalls"?
> 
> Sigh. Sorry for that, I should learn to read the documentation. This
> is obviously the right thing to do.
> 
> However, it does have an unexpected side-effect. On our DT, for the
> GPIOs, we also set up a pinctrl node (which seem to be along the lines
> of the doc recommandations, section "Drivers needing both pin control
> and GPIOs").
> 
> However, when pinctrl_select_default is called by the core, which in
> turns ends up calling pinmux_enable_setting, which builds the owner
> name using the dev_name. However, when we call gpiod_request, it ends
> up in pinmux_request_gpio, which build the owner string using the
> pinctrl device name and the pin number.
> 
> This results in a mismatch of owners, and the gpiod_request fails,
> while the device really is the same.
> 
> Thanks,
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: Requesting as a GPIO a pin already used through pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On Wed, Sep 21, 2016 at 03:34:35PM -0500, Michael Welling wrote:
> On Wed, Sep 21, 2016 at 10:51:28PM +0300, Maxime Ripard wrote:
> > Hi Linus,
> > 
> > Thanks for your reply.
> > 
> > On Sun, Sep 18, 2016 at 01:30:24PM +0200, Linus Walleij wrote:
> > > On Fri, Sep 16, 2016 at 3:58 PM, Maxime Ripard
> > > <maxime.ripard@free-electrons.com> wrote:
> > > 
> > > > However, things are getting weird when you have that requested pin
> > > > assigned to one device, and you try to export the GPIO on that pin
> > > > (through sysfs for example,
> > > 
> > > DON'T use sysfs. Use the new chardev ABI which is by the way enabled
> > > by default.
> > > 
> > > (But you will face the same issue there I guess.)
> > 
> > Yeah, well, we could re-do the discussion on ksummit-discuss :)
> 
> Are you guys trolling me?

No, the point I was trying to make is that not every system and
product kernel out there has been switched to a 4.8+ kernel, for
exactly the reasons that are discussed right now on ksummit-discuss.

I don't really get how that is trolling.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Requesting as a GPIO a pin already used through pinctrl

[Michael Welling]

On Thu, Sep 22, 2016 at 01:48:52PM +0300, Maxime Ripard wrote:
> On Wed, Sep 21, 2016 at 03:34:35PM -0500, Michael Welling wrote:
> > On Wed, Sep 21, 2016 at 10:51:28PM +0300, Maxime Ripard wrote:
> > > Hi Linus,
> > > 
> > > Thanks for your reply.
> > > 
> > > On Sun, Sep 18, 2016 at 01:30:24PM +0200, Linus Walleij wrote:
> > > > On Fri, Sep 16, 2016 at 3:58 PM, Maxime Ripard
> > > > <maxime.ripard@free-electrons.com> wrote:
> > > > 
> > > > > However, things are getting weird when you have that requested pin
> > > > > assigned to one device, and you try to export the GPIO on that pin
> > > > > (through sysfs for example,
> > > > 
> > > > DON'T use sysfs. Use the new chardev ABI which is by the way enabled
> > > > by default.
> > > > 
> > > > (But you will face the same issue there I guess.)
> > > 
> > > Yeah, well, we could re-do the discussion on ksummit-discuss :)
> > 
> > Are you guys trolling me?
> 
> No, the point I was trying to make is that not every system and
> product kernel out there has been switched to a 4.8+ kernel, for
> exactly the reasons that are discussed right now on ksummit-discuss.
> 

Ah thanks for explaining. I read through the latest posts and assume you are
talking about the lengthy backporting discussion.

> I don't really get how that is trolling.

Nevermind that. I thought you were referring to my one and only post to that list.

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2015-July/001879.html

Sorry for the noise.

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: Requesting as a GPIO a pin already used through pinctrl

[Linus Walleij]

On Wed, Sep 21, 2016 at 9:51 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Sun, Sep 18, 2016 at 01:30:24PM +0200, Linus Walleij wrote:

>> > I have the feeling that the core should prevent that, making sure that
>> > the gpiod_request returns EBUSY in such a case, but I'm not really
>> > sure whether it's the case or not, and if it is, where that check is
>> > happening.
>>
>> - Did you try specifying .strict for the pinmux?
>>
>> - Did you read Documentation/pinctrl.txt, section titled
>>   "GPIO mode pitfalls"?
>
> Sigh. Sorry for that, I should learn to read the documentation. This
> is obviously the right thing to do.

Guessed so.

> However, it does have an unexpected side-effect. On our DT, for the
> GPIOs, we also set up a pinctrl node (which seem to be along the lines
> of the doc recommandations, section "Drivers needing both pin control
> and GPIOs").
>
> However, when pinctrl_select_default is called by the core, which in
> turns ends up calling pinmux_enable_setting, which builds the owner
> name using the dev_name. However, when we call gpiod_request, it ends
> up in pinmux_request_gpio, which build the owner string using the
> pinctrl device name and the pin number.
>
> This results in a mismatch of owners, and the gpiod_request fails,
> while the device really is the same.

Yeah, needing both GPIO and pinctrl on the same pin kind of
implies that the subsystems are poking at the same hardware and
that is !=strict.

I have had some half-finished thoughts here, for example to add
more callbacks from GPIO to pinctrl for things like open drain
and biasing, so that GPIO would be a "pure" front-end with a pinctrl
back-end. It ends up duplicating a lot of stuff from pinctrl in GPIO,
but since people will inevitably want to do things like biasing
from the GPIO chardev I might have to bite the bullet and do it
that way.

Other ideas welcome.

Yours,
Linus Walleij

Re: Requesting as a GPIO a pin already used through pinctrl

[Vladimir Zapolskiy]

Hi Linus, Maxime,

On 09/23/2016 04:22 PM, Linus Walleij wrote:
> On Wed, Sep 21, 2016 at 9:51 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> On Sun, Sep 18, 2016 at 01:30:24PM +0200, Linus Walleij wrote:
>
>>>> I have the feeling that the core should prevent that, making sure that
>>>> the gpiod_request returns EBUSY in such a case, but I'm not really
>>>> sure whether it's the case or not, and if it is, where that check is
>>>> happening.
>>>
>>> - Did you try specifying .strict for the pinmux?
>>>
>>> - Did you read Documentation/pinctrl.txt, section titled
>>>   "GPIO mode pitfalls"?
>>
>> Sigh. Sorry for that, I should learn to read the documentation. This
>> is obviously the right thing to do.
>
> Guessed so.
>
>> However, it does have an unexpected side-effect. On our DT, for the
>> GPIOs, we also set up a pinctrl node (which seem to be along the lines
>> of the doc recommandations, section "Drivers needing both pin control
>> and GPIOs").
>>
>> However, when pinctrl_select_default is called by the core, which in
>> turns ends up calling pinmux_enable_setting, which builds the owner
>> name using the dev_name. However, when we call gpiod_request, it ends
>> up in pinmux_request_gpio, which build the owner string using the
>> pinctrl device name and the pin number.
>>
>> This results in a mismatch of owners, and the gpiod_request fails,
>> while the device really is the same.
>
> Yeah, needing both GPIO and pinctrl on the same pin kind of
> implies that the subsystems are poking at the same hardware and
> that is !=strict.

recently I worked on GPIO to pinctrl connection for i.MX SoCs, so
let me share some details until I forget them while time goes by.

IMHO i.MX* IOMUX controllers are type A strict pinctrl controllers
described in Documentation/pinctrl.txt, however practically it won't
be possible to convert them to the strict type due to the countless
runtime issues on boards when DTBs are not updated at the same time,
in other words backward compatibility with the preexisting firmware
is hardly reachable, however for any next added i.MX pinctrl drivers
(and probably many other pinctrl drivers) I would suggest to set
the type to strict, at least it might be nice to

a) more precisely describe what is the strict pinctrl controller
    beast,
b) ask every pinctrl driver submitter for clear confirmation that
    their controller is strict or non-strict.

This should allow to avoid some degree of pain in the future, when
it accidentally happens that users (e.g. fossilized in DTBs) of
a strict pinctrl controller abuse the freedom given exclusively
for non-strict controllers.

Next are some issues I've observed, due to our specifics we have to
control pad muxing in runtime between GPIO and functional modes.
This may imply not only the change of mux setting but as well pin
config changes, hence limiting the discussion to pin descriptions
in OF there should be two or more pinctrl tuples (mux function,
pin config) for the same pin. Usually on practice this case is
resolved by providing pinctrl-[0-9]+ properties, but this works
well only if there is no pad ownership transfer, otherwise one of
two independent users of the same pad resourse will fail to be
registered during boot time due to a conflict for the resourse.
When a device is unbound in runtime, a pad resourse is released
and it can be captured by another device (repeat the procedure to
restore the original state), and when one of the rivals is active
another shall not get a chance to grab the pads. I didn't manage
to find a good description of this possibility in DTS though,
the best one we came across is adding a pinctrl-* property pointing
to GPIO mux/config settings of wanted GPIOs right into the
correspondent GPIO controller device node and set it as non-default
to avoid boot time conflict. When turn for GPIOs begins in runtime
the mux and config values for a pinctrl hardware are taken from
that phandle (by an overly hackish in-kernel pinctrl API extension,
which accounts that non-"default" pinctrl-names values).

And here are some conclusions, as I can see often potentially
dangling pads for GPIOs are described in a hog group (owned by
pinctrl), for strict controllers there is a conflict between
the pinctrl and an end-point GPIO user, probably it might be better
to neutralize this conflict by moving pad groups with GPIO
functioning pads into exclusive usage (including formal runtime
ownership) by GPIO controller drivers and their backends in
pinctrl subsystem. There should be an anchor in DTS, which says
that a pad is functioning as a GPIO, it is given if a pad
belongs to a "gpio-controller" device node. Alternatively hog
groups may be reviewed, all non-GPIO pads placed there are wiped
out or moved to more appropriate groups and hog group ownership
is transferred to the class of GPIO controllers, but this is
a more questionable path.

> I have had some half-finished thoughts here, for example to add
> more callbacks from GPIO to pinctrl for things like open drain
> and biasing, so that GPIO would be a "pure" front-end with a pinctrl
> back-end. It ends up duplicating a lot of stuff from pinctrl in GPIO,
> but since people will inevitably want to do things like biasing
> from the GPIO chardev I might have to bite the bullet and do it
> that way.
>

Open drain, biasing and so on pad config settings can be taken
directly from DTB, if there is no intention to change them in runtime
of course. I don't know if necessity of having OF support is too harsh,
but this may become a good starting point.

> Other ideas welcome.
>

--
With best wishes,
Vladimir

Re: Requesting as a GPIO a pin already used through pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2476 bytes --]

On Fri, Sep 23, 2016 at 03:22:53PM +0200, Linus Walleij wrote:
> > However, it does have an unexpected side-effect. On our DT, for the
> > GPIOs, we also set up a pinctrl node (which seem to be along the lines
> > of the doc recommandations, section "Drivers needing both pin control
> > and GPIOs").
> >
> > However, when pinctrl_select_default is called by the core, which in
> > turns ends up calling pinmux_enable_setting, which builds the owner
> > name using the dev_name. However, when we call gpiod_request, it ends
> > up in pinmux_request_gpio, which build the owner string using the
> > pinctrl device name and the pin number.
> >
> > This results in a mismatch of owners, and the gpiod_request fails,
> > while the device really is the same.
> 
> Yeah, needing both GPIO and pinctrl on the same pin kind of
> implies that the subsystems are poking at the same hardware and
> that is !=strict.

My understanding was that GPIO and pinctrl were pretty much
orthogonal, a pinctrl property would set up the muxing, and mark the
pin as in use, while the GPIO property would just say what we use the
pin for. In a way, that was similar to what any other controller would
behave. You would mark the pins as exclusive, mux them, and leave the
controller deal with its pin.

Anyway, I'll remove those properties from our DTs, and add the .strict
flag.

> I have had some half-finished thoughts here, for example to add
> more callbacks from GPIO to pinctrl for things like open drain
> and biasing, so that GPIO would be a "pure" front-end with a pinctrl
> back-end. It ends up duplicating a lot of stuff from pinctrl in GPIO,
> but since people will inevitably want to do things like biasing
> from the GPIO chardev I might have to bite the bullet and do it
> that way.
> 
> Other ideas welcome.

If we don't have access to the calling device structure, I'm not sure
how we could fix that properly.

On your more mid-term view of things and where the chardev interface
should be going, I definitely second at least having things like
biasing, strength, and all the configuration really, exposed. There's
a lot of people that actually want to enable biasing and we don't
really have a good solution for that.

One way would be to use the DT overlays, but that seems more of a hack
than anything else.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Requesting as a GPIO a pin already used through pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6376 bytes --]

On Fri, Sep 23, 2016 at 06:24:04PM +0300, Vladimir Zapolskiy wrote:
> On 09/23/2016 04:22 PM, Linus Walleij wrote:
> >On Wed, Sep 21, 2016 at 9:51 PM, Maxime Ripard
> ><maxime.ripard@free-electrons.com> wrote:
> >>On Sun, Sep 18, 2016 at 01:30:24PM +0200, Linus Walleij wrote:
> >
> >>>>I have the feeling that the core should prevent that, making sure that
> >>>>the gpiod_request returns EBUSY in such a case, but I'm not really
> >>>>sure whether it's the case or not, and if it is, where that check is
> >>>>happening.
> >>>
> >>>- Did you try specifying .strict for the pinmux?
> >>>
> >>>- Did you read Documentation/pinctrl.txt, section titled
> >>>  "GPIO mode pitfalls"?
> >>
> >>Sigh. Sorry for that, I should learn to read the documentation. This
> >>is obviously the right thing to do.
> >
> >Guessed so.
> >
> >>However, it does have an unexpected side-effect. On our DT, for the
> >>GPIOs, we also set up a pinctrl node (which seem to be along the lines
> >>of the doc recommandations, section "Drivers needing both pin control
> >>and GPIOs").
> >>
> >>However, when pinctrl_select_default is called by the core, which in
> >>turns ends up calling pinmux_enable_setting, which builds the owner
> >>name using the dev_name. However, when we call gpiod_request, it ends
> >>up in pinmux_request_gpio, which build the owner string using the
> >>pinctrl device name and the pin number.
> >>
> >>This results in a mismatch of owners, and the gpiod_request fails,
> >>while the device really is the same.
> >
> >Yeah, needing both GPIO and pinctrl on the same pin kind of
> >implies that the subsystems are poking at the same hardware and
> >that is !=strict.
> 
> recently I worked on GPIO to pinctrl connection for i.MX SoCs, so
> let me share some details until I forget them while time goes by.
> 
> IMHO i.MX* IOMUX controllers are type A strict pinctrl controllers
> described in Documentation/pinctrl.txt, however practically it won't
> be possible to convert them to the strict type due to the countless
> runtime issues on boards when DTBs are not updated at the same time,
> in other words backward compatibility with the preexisting firmware
> is hardly reachable,

I'm not sure marking an optional, generic, property (pinctrl-0) as
exclusive with an often mandatory, if not optional, usually
binding-specific property (*-gpio) would be considered an ABI
breakage, even more so if it was wrong the whole time, and it prevents
us from fixing an actual bug.

Rob? Any opinion on that one?

> however for any next added i.MX pinctrl drivers (and probably many
> other pinctrl drivers) I would suggest to set the type to strict, at
> least it might be nice to
> 
> a) more precisely describe what is the strict pinctrl controller
>    beast,
> b) ask every pinctrl driver submitter for clear confirmation that
>    their controller is strict or non-strict.
> 
> This should allow to avoid some degree of pain in the future, when
> it accidentally happens that users (e.g. fossilized in DTBs) of
> a strict pinctrl controller abuse the freedom given exclusively
> for non-strict controllers.
> 
> Next are some issues I've observed, due to our specifics we have to
> control pad muxing in runtime between GPIO and functional modes.
> This may imply not only the change of mux setting but as well pin
> config changes, hence limiting the discussion to pin descriptions
> in OF there should be two or more pinctrl tuples (mux function,
> pin config) for the same pin. Usually on practice this case is
> resolved by providing pinctrl-[0-9]+ properties, but this works
> well only if there is no pad ownership transfer, otherwise one of
> two independent users of the same pad resourse will fail to be
> registered during boot time due to a conflict for the resourse.
> When a device is unbound in runtime, a pad resourse is released
> and it can be captured by another device (repeat the procedure to
> restore the original state), and when one of the rivals is active
> another shall not get a chance to grab the pads. I didn't manage
> to find a good description of this possibility in DTS though,
> the best one we came across is adding a pinctrl-* property pointing
> to GPIO mux/config settings of wanted GPIOs right into the
> correspondent GPIO controller device node and set it as non-default
> to avoid boot time conflict. When turn for GPIOs begins in runtime
> the mux and config values for a pinctrl hardware are taken from
> that phandle (by an overly hackish in-kernel pinctrl API extension,
> which accounts that non-"default" pinctrl-names values).
> 
> And here are some conclusions, as I can see often potentially
> dangling pads for GPIOs are described in a hog group (owned by
> pinctrl), for strict controllers there is a conflict between
> the pinctrl and an end-point GPIO user, probably it might be better
> to neutralize this conflict by moving pad groups with GPIO
> functioning pads into exclusive usage (including formal runtime
> ownership) by GPIO controller drivers and their backends in
> pinctrl subsystem. There should be an anchor in DTS, which says
> that a pad is functioning as a GPIO, it is given if a pad
> belongs to a "gpio-controller" device node. Alternatively hog
> groups may be reviewed, all non-GPIO pads placed there are wiped
> out or moved to more appropriate groups and hog group ownership
> is transferred to the class of GPIO controllers, but this is
> a more questionable path.

Basing that on whether the pinctrl controller has gpio-controller is a
pretty good idea.

I think the only we would need then is some kind of callback in the
pintrl driver to query for whether a particular configuration is a
gpio, and we can have some neat handover without reducing the
strictness of the whole driver, and without basing it on the owner
string.

Something like "if the configuration is a gpio, and my pinctrl driver
is also a gpio controller, and that gpiod_request is being called on
that pin, hand over the reference"

We'd probably need some additional check on the refcount too (for
example only the first gpiod_request gets the handover, and the next
one are rejected.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Requesting as a GPIO a pin already used through pinctrl

[Linus Walleij]

On Fri, Sep 23, 2016 at 8:24 AM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:

> I would suggest to set
> the type to strict, at least it might be nice to
>
> a) more precisely describe what is the strict pinctrl controller
>    beast,
> b) ask every pinctrl driver submitter for clear confirmation that
>    their controller is strict or non-strict.

Sure, for (a) I tried to write that in Documentation/pinctrl.txt but if
it's unclear, patches are accepted! :)

For (b) I might have slipped, I need more help with review and
sometimes some guerilla patching to random drivers is
welcome.

Not that strictness is the most important thing in the world, there
are many aspects to pin control, but it is one of the things
you might want.

> This should allow to avoid some degree of pain in the future, when
> it accidentally happens that users (e.g. fossilized in DTBs) of
> a strict pinctrl controller abuse the freedom given exclusively
> for non-strict controllers.

Maybe write some do's and don'ts into the documentation then?
But mainly it comes up during review of course.

> Next are some issues I've observed, due to our specifics we have to
> control pad muxing in runtime between GPIO and functional modes.

Interesting. That is not very common I think, do you mean

> This may imply not only the change of mux setting but as well pin
> config changes, hence limiting the discussion to pin descriptions
> in OF there should be two or more pinctrl tuples (mux function,
> pin config) for the same pin. Usually on practice this case is
> resolved by providing pinctrl-[0-9]+ properties, but this works
> well only if there is no pad ownership transfer, otherwise one of
> two independent users of the same pad resourse will fail to be
> registered during boot time due to a conflict for the resourse.

Yeah for complex use cases you likely need a few non-"default"
pin states and have code explicitly switch between them.

> When a device is unbound in runtime, a pad resourse is released
> and it can be captured by another device (repeat the procedure to
> restore the original state), and when one of the rivals is active
> another shall not get a chance to grab the pads. I didn't manage
> to find a good description of this possibility in DTS though,
> the best one we came across is adding a pinctrl-* property pointing
> to GPIO mux/config settings of wanted GPIOs right into the
> correspondent GPIO controller device node and set it as non-default
> to avoid boot time conflict. When turn for GPIOs begins in runtime
> the mux and config values for a pinctrl hardware are taken from
> that phandle (by an overly hackish in-kernel pinctrl API extension,
> which accounts that non-"default" pinctrl-names values).

Uncertain what you mean here, but yeah, we have certainly not
designed the subsystem for all usecases people may have so
contributions are welcome.

> And here are some conclusions, as I can see often potentially
> dangling pads for GPIOs are described in a hog group (owned by
> pinctrl),

OK

> for strict controllers there is a conflict between
> the pinctrl and an end-point GPIO user,

Do you mean when you need to add a few pin configs (no muxing)
to a pin/pad used for GPIO? So you have a hog or something
setting up the pin config and then grab the GPIO from the gpiolib
side calling down to the pin control subsystem and thus needing the
pin controller to be non-strict?

> probably it might be better
> to neutralize this conflict by moving pad groups with GPIO
> functioning pads into exclusive usage (including formal runtime
> ownership) by GPIO controller drivers and their backends in
> pinctrl subsystem. There should be an anchor in DTS, which says
> that a pad is functioning as a GPIO, it is given if a pad
> belongs to a "gpio-controller" device node.

That is interesting. However the usual pattern is to let the
device consuming both the pin control state and the gpio line
own them both.

So what you're suggesting is that they should only own/fetch
the GPIO line and then the gpio controller handles the
corresponding line config settings when the pin is in GPIO
mode?

That sounds elegant. While old DTS must be supported, a
scheme moving to haing the gpiochip own the pincontrol
settings for each of its pins when and only when used as
GPIO, would be very nice.

I imagine a piece of code added to gpiolib for this in that
case, and some nice DT bindings for it.

I think it could be a bit of work, but it can be done.

> Alternatively hog
> groups may be reviewed, all non-GPIO pads placed there are wiped
> out or moved to more appropriate groups and hog group ownership
> is transferred to the class of GPIO controllers, but this is
> a more questionable path.

Nah not as elegant.

>> I have had some half-finished thoughts here, for example to add
>> more callbacks from GPIO to pinctrl for things like open drain
>> and biasing, so that GPIO would be a "pure" front-end with a pinctrl
>> back-end. It ends up duplicating a lot of stuff from pinctrl in GPIO,
>> but since people will inevitably want to do things like biasing
>> from the GPIO chardev I might have to bite the bullet and do it
>> that way.
>
> Open drain, biasing and so on pad config settings can be taken
> directly from DTB, if there is no intention to change them in runtime
> of course. I don't know if necessity of having OF support is too harsh,
> but this may become a good starting point.

Your idea to have the gpiochip just select the static config for a line
when it is used as GPIO is more elegant I think.

Yours,
Linus Walleij

Re: Requesting as a GPIO a pin already used through pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2940 bytes --]

On Sat, Sep 24, 2016 at 12:05:49AM +0300, Maxime Ripard wrote:
> On Fri, Sep 23, 2016 at 03:22:53PM +0200, Linus Walleij wrote:
> > > However, it does have an unexpected side-effect. On our DT, for the
> > > GPIOs, we also set up a pinctrl node (which seem to be along the lines
> > > of the doc recommandations, section "Drivers needing both pin control
> > > and GPIOs").
> > >
> > > However, when pinctrl_select_default is called by the core, which in
> > > turns ends up calling pinmux_enable_setting, which builds the owner
> > > name using the dev_name. However, when we call gpiod_request, it ends
> > > up in pinmux_request_gpio, which build the owner string using the
> > > pinctrl device name and the pin number.
> > >
> > > This results in a mismatch of owners, and the gpiod_request fails,
> > > while the device really is the same.
> > 
> > Yeah, needing both GPIO and pinctrl on the same pin kind of
> > implies that the subsystems are poking at the same hardware and
> > that is !=strict.
> 
> My understanding was that GPIO and pinctrl were pretty much
> orthogonal, a pinctrl property would set up the muxing, and mark the
> pin as in use, while the GPIO property would just say what we use the
> pin for. In a way, that was similar to what any other controller would
> behave. You would mark the pins as exclusive, mux them, and leave the
> controller deal with its pin.
> 
> Anyway, I'll remove those properties from our DTs, and add the .strict
> flag.

So I discussed that with Mark at ELCE.

In order not to break the DT, we looked at the code of pin_request
(which is the one using the strict flag), and went to the conclusion
that it needs to be amended to check the owner based on the device
structure pointer.

Which would need just to add an extra parameter to the pin_request
function, right?

It should be quite easy, because there's basically two users of that
function: pinmux_request_gpio, and pinmux_enable_setting, which in
turn are called by pinmux_request_gpio and pinctrl_select_state,
respectively. Which are exported, and used in a significant number of
callers.

... None of them having access to the struct device
directly. pinctrl_select_state is used in 13 different
drivers. pinctrl_request_gpio by 16 of them, but in a gpio hook
(gpio_request) that do not have the calling device structure.

Which means that in order to avoid removing one property from a DT to
fix an actual bug that can cause real stability issues to Linux, we
end up reworking the gpiolib API and fixing all the users.

Mark, Rob, do you really think this is a reasonable request?
Especially when the feature in question was added more than 2 years
after our driver, leaving us no chance to actually benefit from it
without breaking that ABI?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Requesting as a GPIO a pin already used through pinctrl

[Linus Walleij]

On Wed, Oct 26, 2016 at 5:49 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

> In order not to break the DT, we looked at the code of pin_request
> (which is the one using the strict flag), and went to the conclusion
> that it needs to be amended to check the owner based on the device
> structure pointer.
(...)
> Which means that in order to avoid removing one property from a DT to
> fix an actual bug that can cause real stability issues to Linux, we
> end up reworking the gpiolib API and fixing all the users.

DT backward compatibility is for one case: when devices are shipped
with a DT burnt into ROM/flash. Is that happening?

If Allwinner or their subvendors are not shipping devcie trees - i.e. if
this a community-only effort and you're just building and booting the
DT+kernel on all these devices, by all means break the ABI if it
makes sense. Noone is going to complain.

Then we can discuss backward compatibility the day Allwinner start
shipping device trees, not now.

Yours,
Linus Walleij

Re: Requesting as a GPIO a pin already used through pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

Hi Linus,

On Thu, Oct 27, 2016 at 02:12:45PM +0200, Linus Walleij wrote:
> On Wed, Oct 26, 2016 at 5:49 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> 
> > In order not to break the DT, we looked at the code of pin_request
> > (which is the one using the strict flag), and went to the conclusion
> > that it needs to be amended to check the owner based on the device
> > structure pointer.
> (...)
> > Which means that in order to avoid removing one property from a DT to
> > fix an actual bug that can cause real stability issues to Linux, we
> > end up reworking the gpiolib API and fixing all the users.
> 
> DT backward compatibility is for one case: when devices are shipped
> with a DT burnt into ROM/flash. Is that happening?
>
> If Allwinner or their subvendors are not shipping devcie trees - i.e. if
> this a community-only effort and you're just building and booting the
> DT+kernel on all these devices, by all means break the ABI if it
> makes sense. Noone is going to complain.
> 
> Then we can discuss backward compatibility the day Allwinner start
> shipping device trees, not now.

As far as I know, no one is shipping DTs in a read-only
storage. Allwinner ships a different binary format (FEX), or a widely
incompatible DT in their newer SoCs in their BSPs, so it doesn't
really matter anyway.

The only product that I know of that is running only a mainline-ish
kernel is the CHIP, which I happen to work on, and we just have an
upgradable debian package holding the DT.

And we'll *have* to update it anyway, since between the time where we
ship a feature, and the time it gets upstreamed, the DT binding is
very likely to have changed anyway because of the reviews.

Some other products have a mainline-ish options too, but are in a
similar situation, since their DT have bindings of an earlier version
of patches.

So no, I really don't think the DT ABI matters here, or at least
compared to the bug we face and the changes that we would need to make
in order to fix it. But Mark will probably disagree.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Requesting as a GPIO a pin already used through pinctrl

[Linus Walleij]

On Wed, Nov 2, 2016 at 10:31 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

> So no, I really don't think the DT ABI matters here, or at least
> compared to the bug we face and the changes that we would need to make
> in order to fix it. But Mark will probably disagree.

In this case we have to think about that it is a community effort driving
DT standardization for the community, the company Allwinner is not
actively involved.

I think etching DT bindings in stone is for companies shipping products.
Those decisions will affect their internal company culture and therefore
we exercise pressure on them as a standards body. It is a clear message
that they need to work together to standardize and take responsibility for
what they ship.

For community efforts just wanting to have a nice and upstream-compliant
structure on things, the requirements for ABI should be lower. Getting the
community code to even run on the device is a hacker undertaking already
in the first place, and the community is using their precious free time for
this.

Yours,
Linus Walleij