* Requesting as a GPIO a pin already used through pinctrl @ 2016-09-16 13:58 Maxime Ripard 2016-09-18 11:30 ` Linus Walleij 0 siblings, 1 reply; 15+ messages in thread From: Maxime Ripard @ 2016-09-16 13:58 UTC (permalink / raw) To: Linus Walleij, Alexandre Courbot Cc: linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-16 13:58 Requesting as a GPIO a pin already used through pinctrl Maxime Ripard @ 2016-09-18 11:30 ` Linus Walleij 2016-09-21 19:51 ` Maxime Ripard 0 siblings, 1 reply; 15+ messages in thread From: Linus Walleij @ 2016-09-18 11:30 UTC (permalink / raw) To: Maxime Ripard Cc: Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-18 11:30 ` Linus Walleij @ 2016-09-21 19:51 ` Maxime Ripard 2016-09-21 20:34 ` Michael Welling 2016-09-23 13:22 ` Linus Walleij 0 siblings, 2 replies; 15+ messages in thread From: Maxime Ripard @ 2016-09-21 19:51 UTC (permalink / raw) To: Linus Walleij Cc: Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-21 19:51 ` Maxime Ripard @ 2016-09-21 20:34 ` Michael Welling 2016-09-22 10:48 ` Maxime Ripard 2016-09-23 13:22 ` Linus Walleij 1 sibling, 1 reply; 15+ messages in thread From: Michael Welling @ 2016-09-21 20:34 UTC (permalink / raw) To: Maxime Ripard Cc: Linus Walleij, Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-21 20:34 ` Michael Welling @ 2016-09-22 10:48 ` Maxime Ripard 2016-09-22 15:46 ` Michael Welling 0 siblings, 1 reply; 15+ messages in thread From: Maxime Ripard @ 2016-09-22 10:48 UTC (permalink / raw) To: Michael Welling Cc: Linus Walleij, Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-22 10:48 ` Maxime Ripard @ 2016-09-22 15:46 ` Michael Welling 0 siblings, 0 replies; 15+ messages in thread From: Michael Welling @ 2016-09-22 15:46 UTC (permalink / raw) To: Maxime Ripard Cc: Linus Walleij, Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-21 19:51 ` Maxime Ripard 2016-09-21 20:34 ` Michael Welling @ 2016-09-23 13:22 ` Linus Walleij 2016-09-23 15:24 ` Vladimir Zapolskiy 2016-09-23 21:05 ` Maxime Ripard 1 sibling, 2 replies; 15+ messages in thread From: Linus Walleij @ 2016-09-23 13:22 UTC (permalink / raw) To: Maxime Ripard Cc: Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-23 13:22 ` Linus Walleij @ 2016-09-23 15:24 ` Vladimir Zapolskiy 2016-09-23 21:34 ` Maxime Ripard 2016-09-30 16:26 ` Linus Walleij 2016-09-23 21:05 ` Maxime Ripard 1 sibling, 2 replies; 15+ messages in thread From: Vladimir Zapolskiy @ 2016-09-23 15:24 UTC (permalink / raw) To: Linus Walleij, Maxime Ripard Cc: Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-23 15:24 ` Vladimir Zapolskiy @ 2016-09-23 21:34 ` Maxime Ripard 2016-09-30 16:26 ` Linus Walleij 1 sibling, 0 replies; 15+ messages in thread From: Maxime Ripard @ 2016-09-23 21:34 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Linus Walleij, Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon, Rob Herring [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-23 15:24 ` Vladimir Zapolskiy 2016-09-23 21:34 ` Maxime Ripard @ 2016-09-30 16:26 ` Linus Walleij 1 sibling, 0 replies; 15+ messages in thread From: Linus Walleij @ 2016-09-30 16:26 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Maxime Ripard, Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-23 13:22 ` Linus Walleij 2016-09-23 15:24 ` Vladimir Zapolskiy @ 2016-09-23 21:05 ` Maxime Ripard 2016-10-26 15:49 ` Maxime Ripard 1 sibling, 1 reply; 15+ messages in thread From: Maxime Ripard @ 2016-09-23 21:05 UTC (permalink / raw) To: Linus Walleij Cc: Alexandre Courbot, linux-gpio, Alexandre Belloni, Nicolas Ferre, Boris Brezillon [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-09-23 21:05 ` Maxime Ripard @ 2016-10-26 15:49 ` Maxime Ripard 2016-10-27 12:12 ` Linus Walleij 0 siblings, 1 reply; 15+ messages in thread From: Maxime Ripard @ 2016-10-26 15:49 UTC (permalink / raw) To: mark.rutland, Rob Herring, Linus Walleij Cc: Alexandre Courbot, linux-gpio, Thomas Petazzoni, Alexandre Belloni, Nicolas Ferre, Boris Brezillon, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-10-26 15:49 ` Maxime Ripard @ 2016-10-27 12:12 ` Linus Walleij 2016-11-02 21:31 ` Maxime Ripard 0 siblings, 1 reply; 15+ messages in thread From: Linus Walleij @ 2016-10-27 12:12 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, Rob Herring, Alexandre Courbot, linux-gpio, Thomas Petazzoni, Alexandre Belloni, Nicolas Ferre, Boris Brezillon, Chen-Yu Tsai 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-10-27 12:12 ` Linus Walleij @ 2016-11-02 21:31 ` Maxime Ripard 2016-11-06 10:11 ` Linus Walleij 0 siblings, 1 reply; 15+ messages in thread From: Maxime Ripard @ 2016-11-02 21:31 UTC (permalink / raw) To: Linus Walleij Cc: Mark Rutland, Rob Herring, Alexandre Courbot, linux-gpio, Thomas Petazzoni, Alexandre Belloni, Nicolas Ferre, Boris Brezillon, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Requesting as a GPIO a pin already used through pinctrl 2016-11-02 21:31 ` Maxime Ripard @ 2016-11-06 10:11 ` Linus Walleij 0 siblings, 0 replies; 15+ messages in thread From: Linus Walleij @ 2016-11-06 10:11 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, Rob Herring, Alexandre Courbot, linux-gpio, Thomas Petazzoni, Alexandre Belloni, Nicolas Ferre, Boris Brezillon, Chen-Yu Tsai 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-11-06 10:11 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-16 13:58 Requesting as a GPIO a pin already used through pinctrl Maxime Ripard 2016-09-18 11:30 ` Linus Walleij 2016-09-21 19:51 ` Maxime Ripard 2016-09-21 20:34 ` Michael Welling 2016-09-22 10:48 ` Maxime Ripard 2016-09-22 15:46 ` Michael Welling 2016-09-23 13:22 ` Linus Walleij 2016-09-23 15:24 ` Vladimir Zapolskiy 2016-09-23 21:34 ` Maxime Ripard 2016-09-30 16:26 ` Linus Walleij 2016-09-23 21:05 ` Maxime Ripard 2016-10-26 15:49 ` Maxime Ripard 2016-10-27 12:12 ` Linus Walleij 2016-11-02 21:31 ` Maxime Ripard 2016-11-06 10:11 ` Linus Walleij
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.