linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Confusion about Pinctrl and GPIO
@ 2013-12-26  5:18 曹荣荣
  2013-12-29 10:39 ` Rongrong Cao
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: 曹荣荣 @ 2013-12-26  5:18 UTC (permalink / raw)
  To: linux-kernel

Hi Linus and Stephen,

I'm learning the pinctrl subsystem codes these days, and have a
confusion about it, I'm very appreciated if you can help.

I noticed that Stephen<swarren@nvidia.com> submitted a patch for pinctrl:
http://www.gossamer-threads.com/lists/linux/kernel/1500890?do=post_view_threaded

In this patch, Stephen said, "When an SoC muxes pins in a group, it's
quite possible for the group to contain e.g. 6 pins, but only 4 of
them actually be needed by the HW module that's mux'd to them. In this
case, the other 2 pins could be used as GPIOs. However, pinctrl marks
all the pins within the group as in-use by the selected mux function.
To allow the expected gpiolib interaction, separate the concepts of
pin ownership into two parts: One for the mux function and one for
GPIO usage. Finally, allow those two ownerships to exist in parallel.
"

I agree that gpiolib should be able to use the two idle pins as GPIO,
but after apply this patch, gpiolib can also request the 4 pins used
by HW module succesfully, and this will override the settings of the 4
pins for HW module.

I have read the latest Documentation/pinctrl.txt, and there is two
examples about muxing logic in "GPIO mode pitfalls" section, let me
take example A for instance:

                       pin config
                       logic regs
                       |               +- SPI
     Physical pins --- pad --- pinmux -+- I2C
                               |       +- mmc
                               |       +- GPIO
                               pin
                               multiplex
                               logic regs

Assuming that the pin has been configured as SPI mode, undoubtedly we
can't use it as GPIO any longer. However, if we call gpio_request()
(gpiolib API) to requet this pin for GPIO purpose, gpio_request()
still can return successfully. I think this is unreasonable,
gpio_request() should return an "error code" if the pin is in-use by
PINMUX.

I read the codes of pin_request() in pinmux.c, and guess
pinmux_ops->gpio_request_enable() may be responsible to handle this
use case, that is, to return an "error code" if the pin has been owned
by pinmux. However, no drivers in drivers/pinctrl/ implements such
codes in pinmux_ops->gpio_request_enable().
Or, pinctrl subsystem is just resposible to set the pin in GPIO mode,
and gpio subsystem (gpiolib) is responsible for other things like set
direction, get value if input, or set high/low if output. Is my
understanding right?


********************

Let me talk again about the example described by Stephen. If actually
only 4 pins of the group which contains 6 pins are needed by HW
module, then why does the group be defined to contain 6 pins? I think
the group should be defined only containing 4 pins rather than 6 pins,
then the other 2 idle pins can be used for any other purpose.

Thank you very much in advance!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Confusion about Pinctrl and GPIO
  2013-12-26  5:18 Confusion about Pinctrl and GPIO 曹荣荣
@ 2013-12-29 10:39 ` Rongrong Cao
  2014-01-06  4:11 ` Rongrong Cao
  2014-01-06 21:22 ` Stephen Warren
  2 siblings, 0 replies; 7+ messages in thread
From: Rongrong Cao @ 2013-12-29 10:39 UTC (permalink / raw)
  To: linux-kernel

Anyone can help?

2013/12/26 曹荣荣 <caorr1980@gmail.com>:
> Hi Linus and Stephen,
>
> I'm learning the pinctrl subsystem codes these days, and have a
> confusion about it, I'm very appreciated if you can help.
>
> I noticed that Stephen<swarren@nvidia.com> submitted a patch for pinctrl:
> http://www.gossamer-threads.com/lists/linux/kernel/1500890?do=post_view_threaded
>
> In this patch, Stephen said, "When an SoC muxes pins in a group, it's
> quite possible for the group to contain e.g. 6 pins, but only 4 of
> them actually be needed by the HW module that's mux'd to them. In this
> case, the other 2 pins could be used as GPIOs. However, pinctrl marks
> all the pins within the group as in-use by the selected mux function.
> To allow the expected gpiolib interaction, separate the concepts of
> pin ownership into two parts: One for the mux function and one for
> GPIO usage. Finally, allow those two ownerships to exist in parallel.
> "
>
> I agree that gpiolib should be able to use the two idle pins as GPIO,
> but after apply this patch, gpiolib can also request the 4 pins used
> by HW module succesfully, and this will override the settings of the 4
> pins for HW module.
>
> I have read the latest Documentation/pinctrl.txt, and there is two
> examples about muxing logic in "GPIO mode pitfalls" section, let me
> take example A for instance:
>
>                        pin config
>                        logic regs
>                        |               +- SPI
>      Physical pins --- pad --- pinmux -+- I2C
>                                |       +- mmc
>                                |       +- GPIO
>                                pin
>                                multiplex
>                                logic regs
>
> Assuming that the pin has been configured as SPI mode, undoubtedly we
> can't use it as GPIO any longer. However, if we call gpio_request()
> (gpiolib API) to requet this pin for GPIO purpose, gpio_request()
> still can return successfully. I think this is unreasonable,
> gpio_request() should return an "error code" if the pin is in-use by
> PINMUX.
>
> I read the codes of pin_request() in pinmux.c, and guess
> pinmux_ops->gpio_request_enable() may be responsible to handle this
> use case, that is, to return an "error code" if the pin has been owned
> by pinmux. However, no drivers in drivers/pinctrl/ implements such
> codes in pinmux_ops->gpio_request_enable().
> Or, pinctrl subsystem is just resposible to set the pin in GPIO mode,
> and gpio subsystem (gpiolib) is responsible for other things like set
> direction, get value if input, or set high/low if output. Is my
> understanding right?
>
>
> ********************
>
> Let me talk again about the example described by Stephen. If actually
> only 4 pins of the group which contains 6 pins are needed by HW
> module, then why does the group be defined to contain 6 pins? I think
> the group should be defined only containing 4 pins rather than 6 pins,
> then the other 2 idle pins can be used for any other purpose.
>
> Thank you very much in advance!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Confusion about Pinctrl and GPIO
  2013-12-26  5:18 Confusion about Pinctrl and GPIO 曹荣荣
  2013-12-29 10:39 ` Rongrong Cao
@ 2014-01-06  4:11 ` Rongrong Cao
  2014-01-14  9:26   ` Linus Walleij
  2014-01-06 21:22 ` Stephen Warren
  2 siblings, 1 reply; 7+ messages in thread
From: Rongrong Cao @ 2014-01-06  4:11 UTC (permalink / raw)
  To: linux-kernel, Linus Walleij, Stephen Warren

Hi Linus and Stephen,
Can you help on my question?
Thanks a lot in advance!

2013/12/26 曹荣荣 <caorr1980@gmail.com>:
> Hi Linus and Stephen,
>
> I'm learning the pinctrl subsystem codes these days, and have a
> confusion about it, I'm very appreciated if you can help.
>
> I noticed that Stephen<swarren@nvidia.com> submitted a patch for pinctrl:
> http://www.gossamer-threads.com/lists/linux/kernel/1500890?do=post_view_threaded
>
> In this patch, Stephen said, "When an SoC muxes pins in a group, it's
> quite possible for the group to contain e.g. 6 pins, but only 4 of
> them actually be needed by the HW module that's mux'd to them. In this
> case, the other 2 pins could be used as GPIOs. However, pinctrl marks
> all the pins within the group as in-use by the selected mux function.
> To allow the expected gpiolib interaction, separate the concepts of
> pin ownership into two parts: One for the mux function and one for
> GPIO usage. Finally, allow those two ownerships to exist in parallel.
> "
>
> I agree that gpiolib should be able to use the two idle pins as GPIO,
> but after apply this patch, gpiolib can also request the 4 pins used
> by HW module succesfully, and this will override the settings of the 4
> pins for HW module.
>
> I have read the latest Documentation/pinctrl.txt, and there is two
> examples about muxing logic in "GPIO mode pitfalls" section, let me
> take example A for instance:
>
>                        pin config
>                        logic regs
>                        |               +- SPI
>      Physical pins --- pad --- pinmux -+- I2C
>                                |       +- mmc
>                                |       +- GPIO
>                                pin
>                                multiplex
>                                logic regs
>
> Assuming that the pin has been configured as SPI mode, undoubtedly we
> can't use it as GPIO any longer. However, if we call gpio_request()
> (gpiolib API) to requet this pin for GPIO purpose, gpio_request()
> still can return successfully. I think this is unreasonable,
> gpio_request() should return an "error code" if the pin is in-use by
> PINMUX.
>
> I read the codes of pin_request() in pinmux.c, and guess
> pinmux_ops->gpio_request_enable() may be responsible to handle this
> use case, that is, to return an "error code" if the pin has been owned
> by pinmux. However, no drivers in drivers/pinctrl/ implements such
> codes in pinmux_ops->gpio_request_enable().
> Or, pinctrl subsystem is just resposible to set the pin in GPIO mode,
> and gpio subsystem (gpiolib) is responsible for other things like set
> direction, get value if input, or set high/low if output. Is my
> understanding right?
>
>
> ********************
>
> Let me talk again about the example described by Stephen. If actually
> only 4 pins of the group which contains 6 pins are needed by HW
> module, then why does the group be defined to contain 6 pins? I think
> the group should be defined only containing 4 pins rather than 6 pins,
> then the other 2 idle pins can be used for any other purpose.
>
> Thank you very much in advance!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Confusion about Pinctrl and GPIO
  2013-12-26  5:18 Confusion about Pinctrl and GPIO 曹荣荣
  2013-12-29 10:39 ` Rongrong Cao
  2014-01-06  4:11 ` Rongrong Cao
@ 2014-01-06 21:22 ` Stephen Warren
  2014-01-08  7:29   ` Rongrong Cao
  2 siblings, 1 reply; 7+ messages in thread
From: Stephen Warren @ 2014-01-06 21:22 UTC (permalink / raw)
  To: 曹荣荣, linux-kernel

On 12/25/2013 10:18 PM, 曹荣荣 wrote:

> I noticed that Stephen<swarren@nvidia.com> submitted a patch for pinctrl:
> http://www.gossamer-threads.com/lists/linux/kernel/1500890?do=post_view_threaded
> 
> In this patch, Stephen said, "When an SoC muxes pins in a group, it's
> quite possible for the group to contain e.g. 6 pins, but only 4 of
> them actually be needed by the HW module that's mux'd to them. In this
> case, the other 2 pins could be used as GPIOs. However, pinctrl marks
> all the pins within the group as in-use by the selected mux function.
> To allow the expected gpiolib interaction, separate the concepts of
> pin ownership into two parts: One for the mux function and one for
> GPIO usage. Finally, allow those two ownerships to exist in parallel.
> "
> 
> I agree that gpiolib should be able to use the two idle pins as GPIO,
> but after apply this patch, gpiolib can also request the 4 pins used
> by HW module succesfully, and this will override the settings of the 4
> pins for HW module.

Yes, that's true.

The solution is: don't do that.

> Let me talk again about the example described by Stephen. If actually
> only 4 pins of the group which contains 6 pins are needed by HW
> module, then why does the group be defined to contain 6 pins? I think
> the group should be defined only containing 4 pins rather than 6 pins,
> then the other 2 idle pins can be used for any other purpose.

It all depends on what you mean by group...

A lot of HW has a mux setting per pin. In this case, it would make sense
for the pinctrl driver to expose a separate group for each pin, and for
the pinctrl mapping table (or DT content) to contain an entry for each
individual pin setting that pin to whatever mux function was relevant.

So in this case, yes, it'd make sense in most cases to disallow pins
with a defined/selected mux setting from being used as a GPIO. However,
even in this case, we can't ban dual mux/GPIO usage completely, since
e.g. an I2C driver might want the I2C HW module to drive the pins most
of the time, but still need to acquire the pins as GPIO to implement
some kind of manual bit-banging e.g. to implement a "bus unstick" algorithm.

Some other HW has mux settings that affect multiple pins at once. Tegra
is an example. In this case, there's a single register bit that defines
the mux functions for e.g. 6 pins. In this case, there *must* be a
single pinctrl group definition that encompasses all those 6 pins, since
that's how the HW works. However, the GPIO-vs-non-GPIO setting on Tegra
at least is still per pin, hence the need for the patch of mine that you
mentioned above.

Finally, some people want to use pinctrl groups to represent something
higher level than HW that has a mux bit for a group of pins rather than
per-pin. In that case, you also may need GPIO/mux sharing of a pin, for
similar reasons to the case where the HW muxing really does operate in
groups.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Confusion about Pinctrl and GPIO
       [not found] <CAGHPBZrPx-ouPkVfvT6mXh1QH5gSPbUwuyAnmSU60AbqKybvsA@mail.gmail.com>
@ 2014-01-07 19:02 ` Linus Walleij
  0 siblings, 0 replies; 7+ messages in thread
From: Linus Walleij @ 2014-01-07 19:02 UTC (permalink / raw)
  To: 曹荣荣; +Cc: linux-kernel@vger.kernel.org, Stephen Warren

On Thu, Dec 26, 2013 at 5:56 AM, 曹荣荣 <caorr1980@gmail.com> wrote:

> I have read the latest Documentation/pinctrl.txt, and there is two examples
> about muxing logic in "GPIO mode pitfalls" section, let me take example A
> for instance:
>
>                        pin config
>                        logic regs
>                        |               +- SPI
>      Physical pins --- pad --- pinmux -+- I2C
>                                |       +- mmc
>                                |       +- GPIO
>                                pin
>                                multiplex
>                                logic regs
>
> Assuming that the pin has been configured as SPI mode, undoubtedly we can't
> use it as GPIO any longer. However, if we call gpio_request() (gpiolib API)
> to requet this pin for GPIO purpose, gpio_request() still can return
> successfully. I think this is unreasonable, gpio_request() should return an
> "error code" if the pin is in-use by PINMUX.

Yes but there is also example (B) where it is perfectly possible
to use the same pin as GPIO and for a device at the same time.
So that case needs to be supported as well.

> I read the codes of pin_request() in pinmux.c, and guess
> pinmux_ops->gpio_request_enable() may be responsible to handle this use
> case, that is, to return an "error code" if the pin has been owned by
> pinmux.

Yes this is one way to fix this: let the driver impose such
restrictions.

> However, no drivers in drivers/pinctrl/ implements such codes in
> pinmux_ops->gpio_request_enable().

OK so what about you fix it for a driver you have a problem
with? :-)

> Or, pinctrl subsystem is just resposible to set the pin in GPIO mode, and
> gpio subsystem (gpiolib) is responsible for other things like set direction,
> get value if input, or set high/low if output. Is my understanding right?

The pinctrl subsystem is responsible for all muxing, and the GPIO
subsystem drives and reads GPIO lines yes.

> Let me talk about the example described by Stephen. If actually only 4 pins
> of the group which contains 6 pins are needed by HW module, then why does
> the group be defined to contain 6 pins? I think the group should be defined
> only containing 4 pins rather than 6 pins, then the other 2 idle pins can be
> used for any other purpose.

Group definition should be done at the granularity needed to work
with the system mux configuration. Nominally the group definition
should reflect the electrical use case(s) indicated in the documentation
of the pin controller. Some choose to create one group per pin when
all pins can be muxed around individually.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Confusion about Pinctrl and GPIO
  2014-01-06 21:22 ` Stephen Warren
@ 2014-01-08  7:29   ` Rongrong Cao
  0 siblings, 0 replies; 7+ messages in thread
From: Rongrong Cao @ 2014-01-08  7:29 UTC (permalink / raw)
  To: Stephen Warren; +Cc: linux-kernel

Hi Stephen,
Thank you very much!
I understood now!

2014/1/7 Stephen Warren <swarren@wwwdotorg.org>:
> On 12/25/2013 10:18 PM, 曹荣荣 wrote:
>
>> I noticed that Stephen<swarren@nvidia.com> submitted a patch for pinctrl:
>> http://www.gossamer-threads.com/lists/linux/kernel/1500890?do=post_view_threaded
>>
>> In this patch, Stephen said, "When an SoC muxes pins in a group, it's
>> quite possible for the group to contain e.g. 6 pins, but only 4 of
>> them actually be needed by the HW module that's mux'd to them. In this
>> case, the other 2 pins could be used as GPIOs. However, pinctrl marks
>> all the pins within the group as in-use by the selected mux function.
>> To allow the expected gpiolib interaction, separate the concepts of
>> pin ownership into two parts: One for the mux function and one for
>> GPIO usage. Finally, allow those two ownerships to exist in parallel.
>> "
>>
>> I agree that gpiolib should be able to use the two idle pins as GPIO,
>> but after apply this patch, gpiolib can also request the 4 pins used
>> by HW module succesfully, and this will override the settings of the 4
>> pins for HW module.
>
> Yes, that's true.
>
> The solution is: don't do that.
>
>> Let me talk again about the example described by Stephen. If actually
>> only 4 pins of the group which contains 6 pins are needed by HW
>> module, then why does the group be defined to contain 6 pins? I think
>> the group should be defined only containing 4 pins rather than 6 pins,
>> then the other 2 idle pins can be used for any other purpose.
>
> It all depends on what you mean by group...
>
> A lot of HW has a mux setting per pin. In this case, it would make sense
> for the pinctrl driver to expose a separate group for each pin, and for
> the pinctrl mapping table (or DT content) to contain an entry for each
> individual pin setting that pin to whatever mux function was relevant.
>
> So in this case, yes, it'd make sense in most cases to disallow pins
> with a defined/selected mux setting from being used as a GPIO. However,
> even in this case, we can't ban dual mux/GPIO usage completely, since
> e.g. an I2C driver might want the I2C HW module to drive the pins most
> of the time, but still need to acquire the pins as GPIO to implement
> some kind of manual bit-banging e.g. to implement a "bus unstick" algorithm.
>
> Some other HW has mux settings that affect multiple pins at once. Tegra
> is an example. In this case, there's a single register bit that defines
> the mux functions for e.g. 6 pins. In this case, there *must* be a
> single pinctrl group definition that encompasses all those 6 pins, since
> that's how the HW works. However, the GPIO-vs-non-GPIO setting on Tegra
> at least is still per pin, hence the need for the patch of mine that you
> mentioned above.
>
> Finally, some people want to use pinctrl groups to represent something
> higher level than HW that has a mux bit for a group of pins rather than
> per-pin. In that case, you also may need GPIO/mux sharing of a pin, for
> similar reasons to the case where the HW muxing really does operate in
> groups.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Confusion about Pinctrl and GPIO
  2014-01-06  4:11 ` Rongrong Cao
@ 2014-01-14  9:26   ` Linus Walleij
  0 siblings, 0 replies; 7+ messages in thread
From: Linus Walleij @ 2014-01-14  9:26 UTC (permalink / raw)
  To: Rongrong Cao
  Cc: linux-kernel@vger.kernel.org, Stephen Warren,
	linux-gpio@vger.kernel.org

Hi Rongrong,

> I noticed that Stephen<swarren@nvidia.com> submitted a patch for pinctrl:
> http://www.gossamer-threads.com/lists/linux/kernel/1500890?do=post_view_threaded
>
> In this patch, Stephen said, "When an SoC muxes pins in a group, it's
> quite possible for the group to contain e.g. 6 pins, but only 4 of
> them actually be needed by the HW module that's mux'd to them. In this
> case, the other 2 pins could be used as GPIOs. However, pinctrl marks
> all the pins within the group as in-use by the selected mux function.
> To allow the expected gpiolib interaction, separate the concepts of
> pin ownership into two parts: One for the mux function and one for
> GPIO usage. Finally, allow those two ownerships to exist in parallel.

Yes that is how it works.

> I agree that gpiolib should be able to use the two idle pins as GPIO,
> but after apply this patch, gpiolib can also request the 4 pins used
> by HW module succesfully, and this will override the settings of the 4
> pins for HW module.

If your hardware cannot use the same pins for function X and
GPIO at the same time, it is the responsibility of the individual
driver to disallow this by returning an error code from e.g.
.gpio_request_enable() and .enable() in struct pinmux_ops.

> Assuming that the pin has been configured as SPI mode, undoubtedly we
> can't use it as GPIO any longer. However, if we call gpio_request()
> (gpiolib API) to requet this pin for GPIO purpose, gpio_request()
> still can return successfully. I think this is unreasonable,

It is only unreasonable that a certain driver allows this when
it should be disallowed. Just like a display driver shall disallow
setting update frequencies that can harm a display.

> However, no drivers in drivers/pinctrl/ implements such
> codes in pinmux_ops->gpio_request_enable().

So, if you know a driver is doing something wrong, why not
send a (tested) patch to fix the issue?

> Or, pinctrl subsystem is just resposible to set the pin in GPIO mode,
> and gpio subsystem (gpiolib) is responsible for other things like set
> direction, get value if input, or set high/low if output.

This is true but unrelated to the issue you're bringing up.

A separate GPIO driver should call pinctrl_request_gpio() etc from
the consumer interface to ensure proper muxing of the
GPIO line from the pinctrl backend.

> Let me talk again about the example described by Stephen. If actually
> only 4 pins of the group which contains 6 pins are needed by HW
> module, then why does the group be defined to contain 6 pins? I think
> the group should be defined only containing 4 pins rather than 6 pins,
> then the other 2 idle pins can be used for any other purpose.

It is possible to define two groups of 4+2 pins and only activate
the group containing the first four pins in a certain state in the
pin table. Then there will be no conflict about the two remaining
pins.

Another option is to implement .gpio_request_enable() as
outlined above and not deny the simultaneous usage of the
last two pins, but deny any attempt to use the first 4 pins
for GPIO.

I think many drivers certainly have logical holes here, but
they survive anyway because people do not subject them to
silly configurations and impossible usecases, but indeed if
you want to hammer down any possible mistake from a user
you can proceed to implement proper checking logic into
the driver(s).

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-01-14  9:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-26  5:18 Confusion about Pinctrl and GPIO 曹荣荣
2013-12-29 10:39 ` Rongrong Cao
2014-01-06  4:11 ` Rongrong Cao
2014-01-14  9:26   ` Linus Walleij
2014-01-06 21:22 ` Stephen Warren
2014-01-08  7:29   ` Rongrong Cao
     [not found] <CAGHPBZrPx-ouPkVfvT6mXh1QH5gSPbUwuyAnmSU60AbqKybvsA@mail.gmail.com>
2014-01-07 19:02 ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).