pinctrl: document the pinctrl PM states

Commit Message

Linus Walleij June 11, 2013, 7:59 p.m. UTC

From: Linus Walleij <[e-mail]>

This document snippet tries to be helpful and define the pin
PM states and helpers, and how they should be used to create
some kind of common ontology around this.

Cc: Ulf Hansson <[e-mail]>
Cc: Kevin Hilman <[e-mail]>
Cc: Tony Lindgren <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
 Documentation/pinctrl.txt | 118 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

Comments

Tony Lindgren June 12, 2013, 6:37 p.m. UTC | #1

* Linus Walleij <[e-mail]> [130611 13:05]:
> From: Linus Walleij <[e-mail]>
> 
> This document snippet tries to be helpful and define the pin
> PM states and helpers, and how they should be used to create
> some kind of common ontology around this.
> 
> Cc: Ulf Hansson <[e-mail]>
> Cc: Kevin Hilman <[e-mail]>
> Cc: Tony Lindgren <[e-mail]>
> Signed-off-by: Linus Walleij <[e-mail]>
> ---
>  Documentation/pinctrl.txt | 118 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index f6e664b..a34ea92 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -1196,6 +1196,124 @@ registered. Thus make sure that the error path in your driver gracefully
>  cleans up and is ready to retry the probing later in the startup process.
>  
>  
> +Default and power management related states
> +===========================================
> +
> +As mentioned earlier the device core will automatically try to obtain a
> +pinctrl handle and activate the "default" state on all drivers.
> +
> +However, for power management and power saving, it is sometimes necessary
> +to switch pin states at runtime. Electrically speaking this involves
> +for example reconfigure some pins to be grounded or pulled-down when the
> +system as a whole goes to sleep, or a pull-up could be turned off when pins
> +are idle, reducing leak current.
> +
> +To help out with this, if CONFIG_PM is selected in the Kconfig, three
> +additional states will also be obtained by the driver core and cached
> +there:
> +
> +"active"   this is indended as an explicit active state, if the "default"
> +           state is not synonymous with the active one.
> +
> +"idle"     this is a state that is relaxing the pins when the system as a
> +           whole is up and running, but these particular pins are unused.
> +
> +"sleep"    this is a state that is used when the whole system goes to
> +           suspend, becomes uninteractive, unresponsive to anything but
> +           specific wake-up events.

In the cases I've seen "idle" and "sleep" are the same. But it sounds like
other people's hardware have different needs and it's optional so:

Acked-by: Tony Lindgren <[e-mail]>

Stephen Warren June 13, 2013, 7:39 p.m. UTC | #2

On 06/11/2013 01:59 PM, Linus Walleij wrote:
> From: Linus Walleij <[e-mail]>
> 
> This document snippet tries to be helpful and define the pin
> PM states and helpers, and how they should be used to create
> some kind of common ontology around this.

Oops. I haven't been keeping up well. I propose we hold off on this
patch for a short while until the other thread on this topic is finalized.

Linus Walleij June 13, 2013, 8:34 p.m. UTC | #3

On Thu, Jun 13, 2013 at 9:39 PM, Stephen Warren <[e-mail]> wrote:
> On 06/11/2013 01:59 PM, Linus Walleij wrote:
>> From: Linus Walleij <[e-mail]>
>>
>> This document snippet tries to be helpful and define the pin
>> PM states and helpers, and how they should be used to create
>> some kind of common ontology around this.
>
> Oops. I haven't been keeping up well. I propose we hold off on this
> patch for a short while until the other thread on this topic is finalized.

Isn't it better if I split it?

Most of this doc is about the default/sleep/idle states and
how that relates to runtime PM, and that seems to be
uncontroversial.

Yours,
Linus Walleij

Stephen Warren June 14, 2013, 3:43 p.m. UTC | #4

On 06/13/2013 02:34 PM, Linus Walleij wrote:
> On Thu, Jun 13, 2013 at 9:39 PM, Stephen Warren <[e-mail]> wrote:
>> On 06/11/2013 01:59 PM, Linus Walleij wrote:
>>> From: Linus Walleij <[e-mail]>
>>>
>>> This document snippet tries to be helpful and define the pin
>>> PM states and helpers, and how they should be used to create
>>> some kind of common ontology around this.
>>
>> Oops. I haven't been keeping up well. I propose we hold off on this
>> patch for a short while until the other thread on this topic is finalized.
> 
> Isn't it better if I split it?
> 
> Most of this doc is about the default/sleep/idle states and
> how that relates to runtime PM, and that seems to be
> uncontroversial.

I would tend to prefer sorting out the issue fully, then documenting it
once. This avoids churn.

I would consider the complete set of standard pinctrl states as an
interface. If we add states, that actually changes the interface even
though it might not affect the definition of any individual states.
Since this also impacts DT which is supposed to be a stable ABI (or at
least evolve in a backwards-compatible fashion), it seems better to get
it right once.

Linus Walleij June 16, 2013, 10:17 a.m. UTC | #5

On Fri, Jun 14, 2013 at 5:43 PM, Stephen Warren <[e-mail]> wrote:
> On 06/13/2013 02:34 PM, Linus Walleij wrote:
>> On Thu, Jun 13, 2013 at 9:39 PM, Stephen Warren <[e-mail]> wrote:
>>> On 06/11/2013 01:59 PM, Linus Walleij wrote:
>>>> From: Linus Walleij <[e-mail]>
>>>>
>>>> This document snippet tries to be helpful and define the pin
>>>> PM states and helpers, and how they should be used to create
>>>> some kind of common ontology around this.
>>>
>>> Oops. I haven't been keeping up well. I propose we hold off on this
>>> patch for a short while until the other thread on this topic is finalized.
>>
>> Isn't it better if I split it?
>>
>> Most of this doc is about the default/sleep/idle states and
>> how that relates to runtime PM, and that seems to be
>> uncontroversial.
>
> I would tend to prefer sorting out the issue fully, then documenting it
> once. This avoids churn.

OK I've pulled out this patch for now.

Yours,
Linus Walleij

Tony Lindgren June 17, 2013, 7:20 a.m. UTC | #6

* Stephen Warren <[e-mail]> [130614 08:49]:
> On 06/13/2013 02:34 PM, Linus Walleij wrote:
> > On Thu, Jun 13, 2013 at 9:39 PM, Stephen Warren <[e-mail]> wrote:
> >> On 06/11/2013 01:59 PM, Linus Walleij wrote:
> >>> From: Linus Walleij <[e-mail]>
> >>>
> >>> This document snippet tries to be helpful and define the pin
> >>> PM states and helpers, and how they should be used to create
> >>> some kind of common ontology around this.
> >>
> >> Oops. I haven't been keeping up well. I propose we hold off on this
> >> patch for a short while until the other thread on this topic is finalized.
> > 
> > Isn't it better if I split it?
> > 
> > Most of this doc is about the default/sleep/idle states and
> > how that relates to runtime PM, and that seems to be
> > uncontroversial.
> 
> I would tend to prefer sorting out the issue fully, then documenting it
> once. This avoids churn.
> 
> I would consider the complete set of standard pinctrl states as an
> interface. If we add states, that actually changes the interface even
> though it might not affect the definition of any individual states.
> Since this also impacts DT which is supposed to be a stable ABI (or at
> least evolve in a backwards-compatible fashion), it seems better to get
> it right once.

Agreed, let's sort this out before it's too much of a pain to change.
Below are some reasonings and examples that I'm hoping just might do the
trick.

First, I think the concept of remuxing (or even checking) _all_ the pins
for a consumer device is wrong on most if not all hardware. For past 10
years I have not seen a case where _all_ the pins for a device would need
to be remuxed for any reason.

This means the named states "default", "idle" and "sleep" cannot represent
the needs of hardware. So need to have a bit finer granularity for this.

Maybe let's again try to first list all the known cases where we need to
do remuxing, and the pins we need to remux?

Below are the pin remuxing cases I'm aware of:

1. Remux UART RX pin of a device for a wake-up event

2. Remux whatever device interrupt line to a GPIO input for wake-up

3. Remux audio jack between UART RX and TX to provide a debug console

4. Remux MMC CMD and DAT lines for sleep with pulls to avoid device
   from resetting with lines floating or to save power

Please list any further use cases that I'm not familiar with. I'd like
to hear how messed up this remuxing business can get :)

Then for dealing with the above examples, I think we already have a
pretty good setup in pinctrl framework to deal with this with the
named modes. But I think that to do this properly with the named
modes we should have named modes like the following:

"static" && ("active" || "idle")
"static" && ("rx" || "tx")

Here the "static" pins would be set during driver probe and never
need to change until the driver is unloaded. This is close to what we
currently call "default". But we should probably make it clear that
these will not change to avoid confusion. See below for more info.

The the non-static states like "active"/"idle", or "rx"/"tx", can
be set in addition to "static", but they should not be subsets of
"static" to avoid the issues Stephen described earlier. This way we
allow the named modes to do the work for us while protecting the
claimed pins.

Note also how the remuxing needs are not PM related for the RX/TX case.
Also, the "static" + "active" set would have to be set also in many
cases even if PM is not enabled, while "idle" would be only needed if
we have PM enabled.

Then, based on what I've seen "idle" and "sleep" pins have been
the same. But I guess it's possible that some hardware needs different
states for "idle" and "sleep"? In that case we'd have to have:

"static" && ("active || "idle" || "sleep")

Then iff a device really needs all it's pins remuxed for idle, that
device should have the following sets:

"static"	0 pins
"active"	all pins when running
"idle"		all pins when idle
"sleep"		all pins when sleep if different from idle

Cheers,

Tony

Linus Walleij June 17, 2013, 3:56 p.m. UTC | #7

On Mon, Jun 17, 2013 at 9:20 AM, Tony Lindgren <[e-mail]> wrote:

> Maybe let's again try to first list all the known cases where we need to
> do remuxing, and the pins we need to remux?

In the pinctrl documentation this is known as "runtime pinmuxing".
This is not common, it is much more common to change
pin config, i.e. all other electrical properties of the pins,
at runtime. Especially when going to sleep or idle.

> Below are the pin remuxing cases I'm aware of:
>
> 1. Remux UART RX pin of a device for a wake-up event
>
> 2. Remux whatever device interrupt line to a GPIO input for wake-up
>
> 3. Remux audio jack between UART RX and TX to provide a debug console
>
> 4. Remux MMC CMD and DAT lines for sleep with pulls to avoid device
>    from resetting with lines floating or to save power
>
> Please list any further use cases that I'm not familiar with. I'd like
> to hear how messed up this remuxing business can get :)

We have a debug port that can be muxed out on the keypad(!)
or the SD card, and some other variants...

Stephen added the I2C block switch thing that switch one
and the same IP core between different sets of pins (IIRC).

For runtime pin config I have many more examples, we
change a lot of those to so-called "GPIO mode" (basically
just turned into an input with wakeup,  or pulled to ground)
at sleep.

Yours,
Linus Walleij

Linus Walleij June 17, 2013, 4:05 p.m. UTC | #8

On Mon, Jun 17, 2013 at 9:20 AM, Tony Lindgren <[e-mail]> wrote:

> First, I think the concept of remuxing (or even checking) _all_ the pins
> for a consumer device is wrong on most if not all hardware. For past 10
> years I have not seen a case where _all_ the pins for a device would need
> to be remuxed for any reason.

We may be talking past each other here. On the ux500 we use a lot
of runtime pincontrol, but none of this is *remuxing*.

We are only *reconfiguring*.

Now I know that Haojian only recently added pin config to the
pinctrl-single.c driver so maybe you have mostly seen muxing
in your driver so far, so you view of the world is a bit different.

On the Nomadik pin controller we do mostly hogged muxing
at boot time, but a lot of runtime reconfiguration. So our
needs are very different.

Bear in mind that struct pinctl * forks effects in two paths,
one is muxing the other is config, like pull-ups etc.

Yours,
Linus Walleij

Tony Lindgren June 17, 2013, 6:02 p.m. UTC | #9

* Linus Walleij <[e-mail]> [130617 09:11]:
> On Mon, Jun 17, 2013 at 9:20 AM, Tony Lindgren <[e-mail]> wrote:
> 
> > First, I think the concept of remuxing (or even checking) _all_ the pins
> > for a consumer device is wrong on most if not all hardware. For past 10
> > years I have not seen a case where _all_ the pins for a device would need
> > to be remuxed for any reason.
> 
> We may be talking past each other here. On the ux500 we use a lot
> of runtime pincontrol, but none of this is *remuxing*.
> 
> We are only *reconfiguring*.

Hmm routing the signal to a different device is certainly
remuxing but yeah configuring pulls etc does not change the
mux.
 
> Now I know that Haojian only recently added pin config to the
> pinctrl-single.c driver so maybe you have mostly seen muxing
> in your driver so far, so you view of the world is a bit different.
> 
> On the Nomadik pin controller we do mostly hogged muxing
> at boot time, but a lot of runtime reconfiguration. So our
> needs are very different.
> 
> Bear in mind that struct pinctl * forks effects in two paths,
> one is muxing the other is config, like pull-ups etc.

I also thought the plan was to merge pinmux and pinconf and
do things based the named modes?

The last time I tried using the pinconf functions it involved
knowing the name of the pin in the consumer driver. The name
may not be very descriptive in the device tree cases at least
for the pinctrl-single. So I did not pay much attention to
the pinconf functions.

Sorry if I'm confused, but maybe you can try to help me
understand how you would handle the following:

Let's assume you'd want to reconfigure MMC DAT lines with pulls
for idle and not touch the CLK and CMD lines.

How does the MMC driver know the DAT lines to configure with
pinconf?

And then how would you do set up the pins so that we can set
them up automatically for consumer drivers in
drivers/base/pinctrl.c?

Regards,

Tony

Tony Lindgren June 17, 2013, 6:06 p.m. UTC | #10

* Linus Walleij <[e-mail]> [130617 09:02]:
> On Mon, Jun 17, 2013 at 9:20 AM, Tony Lindgren <[e-mail]> wrote:
> 
> > Maybe let's again try to first list all the known cases where we need to
> > do remuxing, and the pins we need to remux?
> 
> In the pinctrl documentation this is known as "runtime pinmuxing".
> This is not common, it is much more common to change
> pin config, i.e. all other electrical properties of the pins,
> at runtime. Especially when going to sleep or idle.
> 
> > Below are the pin remuxing cases I'm aware of:
> >
> > 1. Remux UART RX pin of a device for a wake-up event
> >
> > 2. Remux whatever device interrupt line to a GPIO input for wake-up
> >
> > 3. Remux audio jack between UART RX and TX to provide a debug console
> >
> > 4. Remux MMC CMD and DAT lines for sleep with pulls to avoid device
> >    from resetting with lines floating or to save power
> >
> > Please list any further use cases that I'm not familiar with. I'd like
> > to hear how messed up this remuxing business can get :)
> 
> We have a debug port that can be muxed out on the keypad(!)
> or the SD card, and some other variants...
> 
> Stephen added the I2C block switch thing that switch one
> and the same IP core between different sets of pins (IIRC).

Oh those are pretty messed up.. Probably the worst case I've
seen is the remuxing of two MMC slots for a singel MMC controller
on omap2420 over I2C bus..
 
> For runtime pin config I have many more examples, we
> change a lot of those to so-called "GPIO mode" (basically
> just turned into an input with wakeup,  or pulled to ground)
> at sleep.

Hmm to me "GPIO mode" sounds like routing the signal to a
completely different device, how is that not remuxing?

Regards,

Tony

Rohit Vaswani June 17, 2013, 6:15 p.m. UTC | #11

On 6/17/2013 8:56 AM, Linus Walleij wrote:
> On Mon, Jun 17, 2013 at 9:20 AM, Tony Lindgren <[e-mail]> wrote:
>
>> Maybe let's again try to first list all the known cases where we need to
>> do remuxing, and the pins we need to remux?
> In the pinctrl documentation this is known as "runtime pinmuxing".
> This is not common, it is much more common to change
> pin config, i.e. all other electrical properties of the pins,
> at runtime. Especially when going to sleep or idle.

MSM also does a significant amount of *remuxing* during suspend/resume.
We have cases where a pin might be connected to a different device 
during normal
operation with a different pull and higher drive strength and we will 
put it
gpio mode, input for a low power configuration.
Consolidating them into 1 operation would be preferred.

>> Below are the pin remuxing cases I'm aware of:
>>
>> 1. Remux UART RX pin of a device for a wake-up event
>>
>> 2. Remux whatever device interrupt line to a GPIO input for wake-up
>>
>> 3. Remux audio jack between UART RX and TX to provide a debug console
>>
>> 4. Remux MMC CMD and DAT lines for sleep with pulls to avoid device
>>     from resetting with lines floating or to save power
>>
>> Please list any further use cases that I'm not familiar with. I'd like
>> to hear how messed up this remuxing business can get :)
> We have a debug port that can be muxed out on the keypad(!)
> or the SD card, and some other variants...
>
> Stephen added the I2C block switch thing that switch one
> and the same IP core between different sets of pins (IIRC).
>
> For runtime pin config I have many more examples, we
> change a lot of those to so-called "GPIO mode" (basically
> just turned into an input with wakeup,  or pulled to ground)
> at sleep.
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


Thanks,
Rohit Vaswani

Stephen Warren June 19, 2013, 8:02 p.m. UTC | #12

On 06/17/2013 01:20 AM, Tony Lindgren wrote:
...
> First, I think the concept of remuxing (or even checking) _all_ the pins
> for a consumer device is wrong on most if not all hardware. For past 10
> years I have not seen a case where _all_ the pins for a device would need
> to be remuxed for any reason.
> 
> This means the named states "default", "idle" and "sleep" cannot represent
> the needs of hardware. So need to have a bit finer granularity for this.

Well, I don't think that's quite true.

We can certainly list configurations for all pins in each of those 3
states without any issue.

The only issue here is whether the data is normalized or not. By listing
the entire configuration in each state, it's not really normalized.

By separating the static and dynamic parts into separate states as you
propose below, it is more normalized.

However, the final configuration of HW is the same either way, and hence
the overall configuration.

> Below are the pin remuxing cases I'm aware of:
...
> Then for dealing with the above examples, I think we already have a
> pretty good setup in pinctrl framework to deal with this with the
> named modes. But I think that to do this properly with the named
> modes we should have named modes like the following:
> 
> "static" && ("active" || "idle")
> "static" && ("rx" || "tx")
> 
> Here the "static" pins would be set during driver probe and never
> need to change until the driver is unloaded. This is close to what we
> currently call "default". But we should probably make it clear that
> these will not change to avoid confusion. See below for more info.
> 
> The the non-static states like "active"/"idle", or "rx"/"tx", can
> be set in addition to "static", but they should not be subsets of
> "static" to avoid the issues Stephen described earlier. This way we
> allow the named modes to do the work for us while protecting the
> claimed pins.

Yes, I think this can certainly work conceptually. It's equivalent to
pre-computing which parts of the overall state don't change between the
currently-defined "global" active/idle states and then applying the
diffs at runtime - rather like what I suggested before, but without the
pinctrl code having to do the diff at runtime. I'm not sure if I have
(yet) a strong opinion on whether allowing multiple states to be active
at once (i.e. static plus active) is the correct way to go. Maybe once
I've finished reading the thread...

Stephen Warren June 19, 2013, 8:06 p.m. UTC | #13

On 06/17/2013 12:02 PM, Tony Lindgren wrote:
> * Linus Walleij <[e-mail]> [130617 09:11]:
>> On Mon, Jun 17, 2013 at 9:20 AM, Tony Lindgren <[e-mail]> wrote:
>>
>>> First, I think the concept of remuxing (or even checking) _all_ the pins
>>> for a consumer device is wrong on most if not all hardware. For past 10
>>> years I have not seen a case where _all_ the pins for a device would need
>>> to be remuxed for any reason.
>>
>> We may be talking past each other here. On the ux500 we use a lot
>> of runtime pincontrol, but none of this is *remuxing*.
>>
>> We are only *reconfiguring*.
> 
> Hmm routing the signal to a different device is certainly
> remuxing but yeah configuring pulls etc does not change the
> mux.
>  
>> Now I know that Haojian only recently added pin config to the
>> pinctrl-single.c driver so maybe you have mostly seen muxing
>> in your driver so far, so you view of the world is a bit different.
>>
>> On the Nomadik pin controller we do mostly hogged muxing
>> at boot time, but a lot of runtime reconfiguration. So our
>> needs are very different.
>>
>> Bear in mind that struct pinctl * forks effects in two paths,
>> one is muxing the other is config, like pull-ups etc.
> 
> I also thought the plan was to merge pinmux and pinconf and
> do things based the named modes?

Yes, pinctrl_select_state() simply applies a certain named state. A
named state can include values for both mux settings and pin
configuration values. So, everything is unified under this top-level
API. There's certainly no need (nor allowance for really) drivers to be
touching and pin_config API directly.

Tony Lindgren June 20, 2013, 6:38 a.m. UTC | #14

* Stephen Warren <[e-mail]> [130619 13:08]:
> On 06/17/2013 01:20 AM, Tony Lindgren wrote:
> ...
> > First, I think the concept of remuxing (or even checking) _all_ the pins
> > for a consumer device is wrong on most if not all hardware. For past 10
> > years I have not seen a case where _all_ the pins for a device would need
> > to be remuxed for any reason.
> > 
> > This means the named states "default", "idle" and "sleep" cannot represent
> > the needs of hardware. So need to have a bit finer granularity for this.
> 
> Well, I don't think that's quite true.
> 
> We can certainly list configurations for all pins in each of those 3
> states without any issue.
> 
> The only issue here is whether the data is normalized or not. By listing
> the entire configuration in each state, it's not really normalized.
> 
> By separating the static and dynamic parts into separate states as you
> propose below, it is more normalized.
> 
> However, the final configuration of HW is the same either way, and hence
> the overall configuration.

Right, and we maintain compability with existing binding. The "static"
we may need to keep as "default" though to avoid breaking existing
bindings. I think that can be covered by documenting it for the dynamic
cases.
 
> > Below are the pin remuxing cases I'm aware of:
> ...
> > Then for dealing with the above examples, I think we already have a
> > pretty good setup in pinctrl framework to deal with this with the
> > named modes. But I think that to do this properly with the named
> > modes we should have named modes like the following:
> > 
> > "static" && ("active" || "idle")
> > "static" && ("rx" || "tx")
> > 
> > Here the "static" pins would be set during driver probe and never
> > need to change until the driver is unloaded. This is close to what we
> > currently call "default". But we should probably make it clear that
> > these will not change to avoid confusion. See below for more info.
> > 
> > The the non-static states like "active"/"idle", or "rx"/"tx", can
> > be set in addition to "static", but they should not be subsets of
> > "static" to avoid the issues Stephen described earlier. This way we
> > allow the named modes to do the work for us while protecting the
> > claimed pins.
> 
> Yes, I think this can certainly work conceptually. It's equivalent to
> pre-computing which parts of the overall state don't change between the
> currently-defined "global" active/idle states and then applying the
> diffs at runtime - rather like what I suggested before, but without the
> pinctrl code having to do the diff at runtime. I'm not sure if I have
> (yet) a strong opinion on whether allowing multiple states to be active
> at once (i.e. static plus active) is the correct way to go. Maybe once
> I've finished reading the thread...

I don't think there's any issue for having multiple sets active the same
is an issue, we're already doing it quite a bit although for different
device drivers so we have the framework ready for that already.

For the dynamic muxing and reconfiguring of the pins we need to keep
the code down to minimum as that can happen every time we enter and exit
idle. So the checking of pins and functions to set should be only done
once during the driver probe.

Regards,

Tony

Stephen Warren June 20, 2013, 7:26 p.m. UTC | #15

On 06/20/2013 12:38 AM, Tony Lindgren wrote:
> * Stephen Warren <[e-mail]> [130619 13:08]:
>> On 06/17/2013 01:20 AM, Tony Lindgren wrote:
...
>>> Below are the pin remuxing cases I'm aware of:
>> ...
>>> Then for dealing with the above examples, I think we already have a
>>> pretty good setup in pinctrl framework to deal with this with the
>>> named modes. But I think that to do this properly with the named
>>> modes we should have named modes like the following:
>>>
>>> "static" && ("active" || "idle")
>>> "static" && ("rx" || "tx")
>>>
>>> Here the "static" pins would be set during driver probe and never
>>> need to change until the driver is unloaded. This is close to what we
>>> currently call "default". But we should probably make it clear that
>>> these will not change to avoid confusion. See below for more info.
>>>
>>> The the non-static states like "active"/"idle", or "rx"/"tx", can
>>> be set in addition to "static", but they should not be subsets of
>>> "static" to avoid the issues Stephen described earlier. This way we
>>> allow the named modes to do the work for us while protecting the
>>> claimed pins.
>>
>> Yes, I think this can certainly work conceptually. It's equivalent to
>> pre-computing which parts of the overall state don't change between the
>> currently-defined "global" active/idle states and then applying the
>> diffs at runtime - rather like what I suggested before, but without the
>> pinctrl code having to do the diff at runtime. I'm not sure if I have
>> (yet) a strong opinion on whether allowing multiple states to be active
>> at once (i.e. static plus active) is the correct way to go. Maybe once
>> I've finished reading the thread...
> 
> I don't think there's any issue for having multiple sets active the same
> is an issue, we're already doing it quite a bit although for different
> device drivers so we have the framework ready for that already.

I assume you mean there shouldn't be any issue *modifying* the pinctrl
API to allow multiple states to be active at once? And where you're
talking about having multiple sets active at once already, you're
talking about some other API?

Right now, pinctrl_select_state() de-activates the old state while
activating the new state, so it's not possible to have more than one
active at once.

Tony Lindgren June 21, 2013, 6:25 a.m. UTC | #16

* Stephen Warren <[e-mail]> [130620 12:32]:
> On 06/20/2013 12:38 AM, Tony Lindgren wrote:
> > * Stephen Warren <[e-mail]> [130619 13:08]:
> >> On 06/17/2013 01:20 AM, Tony Lindgren wrote:
> ...
> >>> Below are the pin remuxing cases I'm aware of:
> >> ...
> >>> Then for dealing with the above examples, I think we already have a
> >>> pretty good setup in pinctrl framework to deal with this with the
> >>> named modes. But I think that to do this properly with the named
> >>> modes we should have named modes like the following:
> >>>
> >>> "static" && ("active" || "idle")
> >>> "static" && ("rx" || "tx")
> >>>
> >>> Here the "static" pins would be set during driver probe and never
> >>> need to change until the driver is unloaded. This is close to what we
> >>> currently call "default". But we should probably make it clear that
> >>> these will not change to avoid confusion. See below for more info.
> >>>
> >>> The the non-static states like "active"/"idle", or "rx"/"tx", can
> >>> be set in addition to "static", but they should not be subsets of
> >>> "static" to avoid the issues Stephen described earlier. This way we
> >>> allow the named modes to do the work for us while protecting the
> >>> claimed pins.
> >>
> >> Yes, I think this can certainly work conceptually. It's equivalent to
> >> pre-computing which parts of the overall state don't change between the
> >> currently-defined "global" active/idle states and then applying the
> >> diffs at runtime - rather like what I suggested before, but without the
> >> pinctrl code having to do the diff at runtime. I'm not sure if I have
> >> (yet) a strong opinion on whether allowing multiple states to be active
> >> at once (i.e. static plus active) is the correct way to go. Maybe once
> >> I've finished reading the thread...
> > 
> > I don't think there's any issue for having multiple sets active the same
> > is an issue, we're already doing it quite a bit although for different
> > device drivers so we have the framework ready for that already.
> 
> I assume you mean there shouldn't be any issue *modifying* the pinctrl
> API to allow multiple states to be active at once? And where you're
> talking about having multiple sets active at once already, you're
> talking about some other API?

Nope, the standard pinctrl API. At least I have not seen issues with
having multiple states active the same time in a single driver.
 
> Right now, pinctrl_select_state() de-activates the old state while
> activating the new state, so it's not possible to have more than one
> active at once.

That should be fine if the hardware needs it. The "static" (or "default")
state can stay active continuously, and "active" and "idle" states are
not subsets of "static".

Then flipping between "active" and "idle" states is fine as they cover
the same pins, and only either "active" or "idle" state is selected at
a time.

If the hardware needs to de-activate the old state inbetween the change
from "active" to "idle", that should be just fine.

If there are other issues with de-activating pins, then let me know.
I'm not be seeing the issue with de-activating states as pinctrl-single
does not de-activate anything for omap. The disable call gets called
between the state changes though, so hardware needing de-activating
can specify it.

Regards,

Tony

Stephen Warren June 21, 2013, 7:12 p.m. UTC | #17

On 06/21/2013 12:25 AM, Tony Lindgren wrote:
> * Stephen Warren <[e-mail]> [130620 12:32]:
>> On 06/20/2013 12:38 AM, Tony Lindgren wrote:
>>> * Stephen Warren <[e-mail]> [130619 13:08]:
>>>> On 06/17/2013 01:20 AM, Tony Lindgren wrote:
>> ...
>>>>> Below are the pin remuxing cases I'm aware of:
>>>> ...
>>>>> Then for dealing with the above examples, I think we already have a
>>>>> pretty good setup in pinctrl framework to deal with this with the
>>>>> named modes. But I think that to do this properly with the named
>>>>> modes we should have named modes like the following:
>>>>>
>>>>> "static" && ("active" || "idle")
>>>>> "static" && ("rx" || "tx")
>>>>>
>>>>> Here the "static" pins would be set during driver probe and never
>>>>> need to change until the driver is unloaded. This is close to what we
>>>>> currently call "default". But we should probably make it clear that
>>>>> these will not change to avoid confusion. See below for more info.
>>>>>
>>>>> The the non-static states like "active"/"idle", or "rx"/"tx", can
>>>>> be set in addition to "static", but they should not be subsets of
>>>>> "static" to avoid the issues Stephen described earlier. This way we
>>>>> allow the named modes to do the work for us while protecting the
>>>>> claimed pins.
>>>>
>>>> Yes, I think this can certainly work conceptually. It's equivalent to
>>>> pre-computing which parts of the overall state don't change between the
>>>> currently-defined "global" active/idle states and then applying the
>>>> diffs at runtime - rather like what I suggested before, but without the
>>>> pinctrl code having to do the diff at runtime. I'm not sure if I have
>>>> (yet) a strong opinion on whether allowing multiple states to be active
>>>> at once (i.e. static plus active) is the correct way to go. Maybe once
>>>> I've finished reading the thread...
>>>
>>> I don't think there's any issue for having multiple sets active the same
>>> is an issue, we're already doing it quite a bit although for different
>>> device drivers so we have the framework ready for that already.
>>
>> I assume you mean there shouldn't be any issue *modifying* the pinctrl
>> API to allow multiple states to be active at once? And where you're
>> talking about having multiple sets active at once already, you're
>> talking about some other API?
> 
> Nope, the standard pinctrl API. At least I have not seen issues with
> having multiple states active the same time in a single driver.

Please take a look at the implementation of pinctrl_select_state(). It
very explicitly performs the following steps:

1) Find all pins(groups) that are used in the current state but not the
new state, and execute pinctrl_disable_setting() on them. (For mux
settings only, not pin config, since the core doesn't have any idea how
to reverse config settings).

2) For all settings in the new state, apply those settings.

So, it very explicitly only allows a single state to be set at a time.
Equally, p->state (the field which stores the currently selected state)
is a single item, not a set/list/array.

So, this code needs rework if you want the core to support the concept
of having multiple states active at once, since it needs separate
pinctrl_activate_state() and pinctrl_deactivate_state() APIs, in order
to avoid step (1) above. And of course, p->state would need to be a
set/list/array.

>> Right now, pinctrl_select_state() de-activates the old state while
>> activating the new state, so it's not possible to have more than one
>> active at once.
> 
> That should be fine if the hardware needs it. The "static" (or "default")
> state can stay active continuously, and "active" and "idle" states are
> not subsets of "static".
> 
> Then flipping between "active" and "idle" states is fine as they cover
> the same pins, and only either "active" or "idle" state is selected at
> a time.
> 
> If the hardware needs to de-activate the old state inbetween the change
> from "active" to "idle", that should be just fine.

Well, I'm not saying the code couldn't be written to do that. It may
even be the right thing to do. However, I'm just pointing out that it
would need conceptual/semantic changes to the pinctrl API and
implementation.

> If there are other issues with de-activating pins, then let me know.
> I'm not be seeing the issue with de-activating states as pinctrl-single
> does not de-activate anything for omap. The disable call gets called
> between the state changes though, so hardware needing de-activating
> can specify it.

Tony Lindgren June 24, 2013, 10:10 a.m. UTC | #18

* Stephen Warren <[e-mail]> [130621 12:18]:
> On 06/21/2013 12:25 AM, Tony Lindgren wrote:
> > * Stephen Warren <[e-mail]> [130620 12:32]:
> >>
> >> I assume you mean there shouldn't be any issue *modifying* the pinctrl
> >> API to allow multiple states to be active at once? And where you're
> >> talking about having multiple sets active at once already, you're
> >> talking about some other API?
> > 
> > Nope, the standard pinctrl API. At least I have not seen issues with
> > having multiple states active the same time in a single driver.
> 
> Please take a look at the implementation of pinctrl_select_state(). It
> very explicitly performs the following steps:
> 
> 1) Find all pins(groups) that are used in the current state but not the
> new state, and execute pinctrl_disable_setting() on them. (For mux
> settings only, not pin config, since the core doesn't have any idea how
> to reverse config settings).
>
> 2) For all settings in the new state, apply those settings.
> 
> So, it very explicitly only allows a single state to be set at a time.
> Equally, p->state (the field which stores the currently selected state)
> is a single item, not a set/list/array.

OK thanks I get now what you're saying. I did not see the p->state
issue as the disable function won't do anything for the SoCs that I
mostly deal with.

> So, this code needs rework if you want the core to support the concept
> of having multiple states active at once, since it needs separate
> pinctrl_activate_state() and pinctrl_deactivate_state() APIs, in order
> to avoid step (1) above. And of course, p->state would need to be a
> set/list/array.

I'll think about it a bit and do a patch to fix this. It seems that
that we need just two entries in the p->state array: static (default),
and dynamic. Then the dynamic would be typically one of: active, idle,
rx, tx.

Regards,

Tony

Linus Walleij June 24, 2013, 12:37 p.m. UTC | #19

On Mon, Jun 17, 2013 at 8:02 PM, Tony Lindgren <[e-mail]> wrote:
> * Linus Walleij <[e-mail]> [130617 09:11]:

>> Bear in mind that struct pinctl * forks effects in two paths,
>> one is muxing the other is config, like pull-ups etc.
>
> I also thought the plan was to merge pinmux and pinconf and
> do things based the named modes?

That is done from a consumer point of view.
Consumers only care about pinctrl * handles
and pinctrl_state * switches.

> The last time I tried using the pinconf functions it involved
> knowing the name of the pin in the consumer driver. The name
> may not be very descriptive in the device tree cases at least
> for the pinctrl-single. So I did not pay much attention to
> the pinconf functions.

Consumers should not use that interface, i.e.:

int pin_config_get(const char *dev_name, const char *name,
                                unsigned long *config);
int pin_config_set(const char *dev_name, const char *name,
                                 unsigned long config)

This needs to be deleted from <linux/pinctrl/consumer.h>
I'll see if I can get rid of it pronto to avoid any more confusion
and sorry for leaving that in place for too long.

The proper way to use it is to use the states.

Yours,
Linus Walleij

Stephen Warren June 24, 2013, 6:09 p.m. UTC | #20

On 06/24/2013 04:10 AM, Tony Lindgren wrote:
> * Stephen Warren <[e-mail]> [130621 12:18]:
>> On 06/21/2013 12:25 AM, Tony Lindgren wrote:
>>> * Stephen Warren <[e-mail]> [130620 12:32]:
>>>>
>>>> I assume you mean there shouldn't be any issue *modifying* the pinctrl
>>>> API to allow multiple states to be active at once? And where you're
>>>> talking about having multiple sets active at once already, you're
>>>> talking about some other API?
>>>
>>> Nope, the standard pinctrl API. At least I have not seen issues with
>>> having multiple states active the same time in a single driver.
>>
>> Please take a look at the implementation of pinctrl_select_state(). It
>> very explicitly performs the following steps:
>>
>> 1) Find all pins(groups) that are used in the current state but not the
>> new state, and execute pinctrl_disable_setting() on them. (For mux
>> settings only, not pin config, since the core doesn't have any idea how
>> to reverse config settings).
>>
>> 2) For all settings in the new state, apply those settings.
>>
>> So, it very explicitly only allows a single state to be set at a time.
>> Equally, p->state (the field which stores the currently selected state)
>> is a single item, not a set/list/array.
> 
> OK thanks I get now what you're saying. I did not see the p->state
> issue as the disable function won't do anything for the SoCs that I
> mostly deal with.
> 
>> So, this code needs rework if you want the core to support the concept
>> of having multiple states active at once, since it needs separate
>> pinctrl_activate_state() and pinctrl_deactivate_state() APIs, in order
>> to avoid step (1) above. And of course, p->state would need to be a
>> set/list/array.
> 
> I'll think about it a bit and do a patch to fix this. It seems that
> that we need just two entries in the p->state array: static (default),
> and dynamic. Then the dynamic would be typically one of: active, idle,
> rx, tx.

I'm not entirely convinced that "2" is the right number. If we start
allowing drivers to "piece together" multiple different state names, why
wouldn't you allow 3 (or n) different state names to be active at once?
Off-hand, I don't have specific use-cases in mind for more than 2 state
(or even 1 in my case I suspect) - it just seems like expecting to
arbitrarily restrict the number of co-active states is unlikely to last
for long, and it'll end up being a slippery slope.

Tony Lindgren June 25, 2013, 7:31 a.m. UTC | #21

* Linus Walleij <[e-mail]> [130624 05:43]:
> On Mon, Jun 17, 2013 at 8:02 PM, Tony Lindgren <[e-mail]> wrote:
> > * Linus Walleij <[e-mail]> [130617 09:11]:
> 
> >> Bear in mind that struct pinctl * forks effects in two paths,
> >> one is muxing the other is config, like pull-ups etc.
> >
> > I also thought the plan was to merge pinmux and pinconf and
> > do things based the named modes?
> 
> That is done from a consumer point of view.
> Consumers only care about pinctrl * handles
> and pinctrl_state * switches.
> 
> > The last time I tried using the pinconf functions it involved
> > knowing the name of the pin in the consumer driver. The name
> > may not be very descriptive in the device tree cases at least
> > for the pinctrl-single. So I did not pay much attention to
> > the pinconf functions.
> 
> Consumers should not use that interface, i.e.:
> 
> int pin_config_get(const char *dev_name, const char *name,
>                                 unsigned long *config);
> int pin_config_set(const char *dev_name, const char *name,
>                                  unsigned long config)
> 
> This needs to be deleted from <linux/pinctrl/consumer.h>
> I'll see if I can get rid of it pronto to avoid any more confusion
> and sorry for leaving that in place for too long.
> 
> The proper way to use it is to use the states.

OK thanks for clarifying that. Yes I think the named states
is a good way to handle the pins in a generic way.

Regards,

Tony

Tony Lindgren June 25, 2013, 7:38 a.m. UTC | #22

* Stephen Warren <[e-mail]> [130624 11:15]:
> On 06/24/2013 04:10 AM, Tony Lindgren wrote:
> > * Stephen Warren <[e-mail]> [130621 12:18]:
> >> On 06/21/2013 12:25 AM, Tony Lindgren wrote:
> >>> * Stephen Warren <[e-mail]> [130620 12:32]:
> >>>>
> >>>> I assume you mean there shouldn't be any issue *modifying* the pinctrl
> >>>> API to allow multiple states to be active at once? And where you're
> >>>> talking about having multiple sets active at once already, you're
> >>>> talking about some other API?
> >>>
> >>> Nope, the standard pinctrl API. At least I have not seen issues with
> >>> having multiple states active the same time in a single driver.
> >>
> >> Please take a look at the implementation of pinctrl_select_state(). It
> >> very explicitly performs the following steps:
> >>
> >> 1) Find all pins(groups) that are used in the current state but not the
> >> new state, and execute pinctrl_disable_setting() on them. (For mux
> >> settings only, not pin config, since the core doesn't have any idea how
> >> to reverse config settings).
> >>
> >> 2) For all settings in the new state, apply those settings.
> >>
> >> So, it very explicitly only allows a single state to be set at a time.
> >> Equally, p->state (the field which stores the currently selected state)
> >> is a single item, not a set/list/array.
> > 
> > OK thanks I get now what you're saying. I did not see the p->state
> > issue as the disable function won't do anything for the SoCs that I
> > mostly deal with.
> > 
> >> So, this code needs rework if you want the core to support the concept
> >> of having multiple states active at once, since it needs separate
> >> pinctrl_activate_state() and pinctrl_deactivate_state() APIs, in order
> >> to avoid step (1) above. And of course, p->state would need to be a
> >> set/list/array.
> > 
> > I'll think about it a bit and do a patch to fix this. It seems that
> > that we need just two entries in the p->state array: static (default),
> > and dynamic. Then the dynamic would be typically one of: active, idle,
> > rx, tx.
> 
> I'm not entirely convinced that "2" is the right number. If we start
> allowing drivers to "piece together" multiple different state names, why
> wouldn't you allow 3 (or n) different state names to be active at once?
> Off-hand, I don't have specific use-cases in mind for more than 2 state
> (or even 1 in my case I suspect) - it just seems like expecting to
> arbitrarily restrict the number of co-active states is unlikely to last
> for long, and it'll end up being a slippery slope.

Yes let's set it up so we can expand it if needed.

We probably don't want the consumer drivers to piece together various
named states directly though as that will lead to custom code in every
driver..

I think we can make it happen automaticall for the cases we've discussed.

Regards,

Tony

Patch

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index f6e664b..a34ea92 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -1196,6 +1196,124 @@  registered. Thus make sure that the error path in your driver gracefully
 cleans up and is ready to retry the probing later in the startup process.
 
 
+Default and power management related states
+===========================================
+
+As mentioned earlier the device core will automatically try to obtain a
+pinctrl handle and activate the "default" state on all drivers.
+
+However, for power management and power saving, it is sometimes necessary
+to switch pin states at runtime. Electrically speaking this involves
+for example reconfigure some pins to be grounded or pulled-down when the
+system as a whole goes to sleep, or a pull-up could be turned off when pins
+are idle, reducing leak current.
+
+To help out with this, if CONFIG_PM is selected in the Kconfig, three
+additional states will also be obtained by the driver core and cached
+there:
+
+"active"   this is indended as an explicit active state, if the "default"
+           state is not synonymous with the active one.
+
+"idle"     this is a state that is relaxing the pins when the system as a
+           whole is up and running, but these particular pins are unused.
+
+"sleep"    this is a state that is used when the whole system goes to
+           suspend, becomes uninteractive, unresponsive to anything but
+           specific wake-up events.
+
+These correspond to the definitions found in <linux/pinctrl/pinctrl-state.h>
+where the same strings are encoded.
+
+When CONFIG_PM is set, the following functions will simultaneously be made
+available to switch between these power-related states:
+
+#include <linux/pinctrl/consumer.h>
+
+int pinctrl_pm_select_default_state(struct device *dev);
+int pinctrl_pm_select_active_state(struct device *dev);
+int pinctrl_pm_select_sleep_state(struct device *dev);
+int pinctrl_pm_select_idle_state(struct device *dev);
+
+As the corresponding pinctrl handle and states are cached in the driver
+core, nothing apart from these calls is needed to move the pins between
+these states.
+
+For a typical runtime PM enabled (see Documentation/power/runtime_pm.txt)
+driver the following outline could be followed:
+
+probe():
+    pinctrl_pm_select_default_state()
+
+runtime_suspend():
+    pinctrl_pm_select_idle_state()
+
+runtime_resume():
+    pinctrl_pm_select_default_state()
+
+suspend:
+    pinctrl_pm_select_sleep_state()
+
+resume:
+    pinctrl_pm_select_idle_state()
+
+Some of the state selectors could be skipped if the driver is for a
+certain system where e.g. the "active" state is not defined, instead
+relying on the "default" state to make the pins active at all times.
+For a driver only supporting suspend and resume, the "idle" state also
+loose its meaning.
+
+A state-chart diagram would look like this:
+
+                +---------+                          +----------+
+    probe() ->  | default |  -> runtime_suspend() -> | idle     |
+                |         |  <- runtime_resume()  <- |          |
+                +---------+                          +----------+
+                                                        |     ^
+                                                        |     |
+                                                  suspend()  resume()
+                                                        |     |
+                                                        V     |
+                                                      +---------+
+                                                      |  sleep  |
+                                                      +---------+
+
+This reflects the runtime PM concept that we are always runtime
+suspended when we go to suspend.
+
+A more complex example is a driver written for many different
+hardware configurations, some which have only the default state,
+some which have the default state and the sleep state, some that
+have no idle state but indeed a default state and so on. Since
+all states are basically optional (the core will not complain if
+they are not found) we can write our state transitions like
+this:
+
+probe():
+    pinctrl_pm_select_default_state()
+
+runtime_suspend():
+    pinctrl_pm_select_idle_state()
+
+runtime_resume():
+    pinctrl_pm_select_default_state()
+    pinctrl_pm_select_active_state()
+
+suspend:
+    pinctrl_pm_select_sleep_state()
+
+resume:
+    pinctrl_pm_select_default_state()
+    pinctrl_pm_select_idle_state()
+
+Here runtime_resume() and resume() passes through the "default"
+state to the "active" and "idle" states respectively since everything
+but "default" is optional. For example it is OK to only supply the
+"default" and "sleep" state pair with this code, and it will
+transition the pins from "default" to "sleep" and leaving the rest
+as no-ops.
+
+
 Drivers needing both pin control and GPIOs
 ==========================================