[RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Grygorii Strashko]

Before switching to DT pinctrl states of OMAP IPs have been handled by hwmod
framework. After switching to DT-boot the pinctrl handling was dropped from
hwmod framework and, as it was recommended, OMAP IP's drivers have to be updated
to handle pinctrl states by itself using pinctrl_pm_select_xx() helpers
(see http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173514.html)

But this is not right for OMAP2+ SoC where real IPs state is controlled
by omap_device core which enables/disables modules & clocks actually.

For example, if OMAP I2C driver will handle pinctrl state during system wide
suspend the following issue may occure:
- suspend_noirq - I2C device can be still active because of PM auto-suspend
  |-_od_suspend_noirq
     |- omap_i2c_suspend_noirq
        |- PINs state set to SLEEP
  |- pm_generic_runtime_suspend
     |- omap_i2c_runtime_suspend()
        |- PINs state set to IDLE  <--- *oops* PINs state is IDLE and not SLEEP
  |- omap_device_idle()
     |- omap_hwmod_idle()
        |- _idle()
           |- disbale module (sysc&clocks)

- resume_noirq - I2C was active before suspend
  |-_od_resume_noirq
     |- omap_hwmod_enable()
        |- _enable()
           |- enable module (sysc&clocks)
     |- pm_generic_runtime_resume
        |- omap_i2c_runtime_resume()
           |- PINs state set to DEFAULT  <--- !!!!
     |- omap_i2c_resume_noirq
        |- PINs state set to DEFAULT
        |- PINs state set to IDLE  <--- *big oops* we have active module and its
                                         PINs state is IDLE
(see https://patchwork.kernel.org/patch/2642101/)

Of course, everything can be handled by adding a tons of code in ecah driver to
check PM state of device and override default behavior of omap_device core, but
this not good.

Hence, add pinctrl handling in omap_device core:
1) on PM runtime resume
- switch pinctrl state to "default" (todo: "active")
2) on PM runtime suspend
- switch pinctrl state to "idle"
3) during system wide suspend
- switch pinctrl state to "sleep" or "idle" if omap_device core disables device
- switch pinctrl state to "sleep" if device is disabled already
4) during system wide resume
- switch pinctrl state to "default" (todo: "active") if omap_device core has
  disabled  device during suspend
- switch pinctrl state to "idle" if device was already disabled before suspend

This will enable pinctrl for all OMAP2+ IP's drivers by default -
no changes in code is needed and only DT data will need to be updated
(add "default", "active", "idle", "sleep" states).

This will enable pinctrl handling for all OMAP2+ drivers by default -
no changes in code is needed and only DT data will need to be updated
(add "default", "active", "idle", "sleep" states).

Related discussions:
- [3/3] i2c: nomadik: use pinctrl PM helpers 
 https://patchwork.kernel.org/patch/2670291/
- mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime
 https://patchwork.kernel.org/patch/2690191/
- [PATCH 00/11] drivers: Add Pinctrl PM support
 https://lkml.org/lkml/2013/5/31/210

CC: Hebbar Gururaja <gururaja.hebbar@ti.com>
CC: Linus Walleij  <linus.walleij@linaro.org>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-omap@vger.kernel.org
CC: linux-kernel@vger.kernel.org 

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
Hi Kevin, Tony,

I've verified this patch on OMAP4 SDP board:
- PM runtime for I2C4, UART2, UART3
- suspend/resume with I2C4, UART2, UART3

seems it works and pinctrl states have been updated as expected.

TODO:
- need to be rebased when support for "active" state  will be added.
- need to be rebased when "Omap serial cleanup" patch series will be merged
  https://lkml.org/lkml/2013/4/26/503

based on top of:
 git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
849aa58 Add linux-next specific files for 20130620

Best regards
- grygorii

 arch/arm/mach-omap2/omap_device.c |   45 ++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index e37feb2..d8cf17b 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -39,6 +39,7 @@
 #include "soc.h"
 #include "omap_device.h"
 #include "omap_hwmod.h"
+#include "iomap.h"
 
 /* Private functions */
 
@@ -585,8 +586,10 @@ static int _od_runtime_suspend(struct device *dev)
 
 	ret = pm_generic_runtime_suspend(dev);
 
-	if (!ret)
+	if (!ret) {
 		omap_device_idle(pdev);
+		pinctrl_pm_select_idle_state(dev);
+	}
 
 	return ret;
 }
@@ -595,12 +598,26 @@ static int _od_runtime_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 
+	/* TODO: should be replaced to pinctrl_pm_select_active_state() */
+	pinctrl_pm_select_default_state(dev);
+
 	omap_device_enable(pdev);
 
 	return pm_generic_runtime_resume(dev);
 }
 #endif
 
+void _od_suspend_sel_pinctrl_state(struct device *dev)
+{
+	if (!dev->pins)
+		return;
+	/* try to select *deepest* pinctrl state */
+	if (IS_ERR(dev->pins->sleep_state))
+		pinctrl_pm_select_idle_state(dev);
+	else
+		pinctrl_pm_select_sleep_state(dev);
+}
+
 #ifdef CONFIG_SUSPEND
 static int _od_suspend_noirq(struct device *dev)
 {
@@ -613,12 +630,22 @@ static int _od_suspend_noirq(struct device *dev)
 		return 0;
 
 	ret = pm_generic_suspend_noirq(dev);
+	if (!ret) {
+		if (!pm_runtime_status_suspended(dev)) {
+			if (pm_generic_runtime_suspend(dev) == 0) {
+				if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) {
+					omap_device_idle(pdev);
+					_od_suspend_sel_pinctrl_state(dev);
+				}
+			}
 
-	if (!ret && !pm_runtime_status_suspended(dev)) {
-		if (pm_generic_runtime_suspend(dev) == 0) {
-			if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
-				omap_device_idle(pdev);
 			od->flags |= OMAP_DEVICE_SUSPENDED;
+		} else {
+			/*
+			 * "idle" pinctrl state already applied -
+			 * try to set "sleep" state
+			 */
+			pinctrl_pm_select_sleep_state(dev);
 		}
 	}
 
@@ -633,9 +660,15 @@ static int _od_resume_noirq(struct device *dev)
 	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
 	    !pm_runtime_status_suspended(dev)) {
 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
-		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) {
 			omap_device_enable(pdev);
+			/* TODO: should be replaced to pinctrl_pm_select_active_state() */
+			pinctrl_pm_select_default_state(dev);
+		}
 		pm_generic_runtime_resume(dev);
+	} else if (!pm_runtime_status_suspended(dev)) {
+		/* switch back to "idle" pinctrl state */
+		pinctrl_pm_select_idle_state(dev);
 	}
 
 	return pm_generic_resume_noirq(dev);
-- 
1.7.9.5

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Linus Walleij]

On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Hence, add pinctrl handling in omap_device core:
> 1) on PM runtime resume
> - switch pinctrl state to "default" (todo: "active")
> 2) on PM runtime suspend
> - switch pinctrl state to "idle"
> 3) during system wide suspend
> - switch pinctrl state to "sleep" or "idle" if omap_device core disables device
> - switch pinctrl state to "sleep" if device is disabled already
> 4) during system wide resume
> - switch pinctrl state to "default" (todo: "active") if omap_device core has
>   disabled  device during suspend
> - switch pinctrl state to "idle" if device was already disabled before suspend

I don't understand step 4.

I get the creeps about whether the system is runtime suspended
or runtime resumed when we come to resume proper, so I need
Kevin to have a look at this.

Apart from that it looks good.

Stephen and Tony are trying to figure out the details of whether "active"
is necessary or not in a related thread I think.

Yours,
Linus Walleij

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Kevin Hilman]

Hi Grygorii,

Grygorii Strashko <grygorii.strashko@ti.com> writes:

> Before switching to DT pinctrl states of OMAP IPs have been handled by hwmod
> framework. After switching to DT-boot the pinctrl handling was dropped from
> hwmod framework and, as it was recommended, OMAP IP's drivers have to be updated
> to handle pinctrl states by itself using pinctrl_pm_select_xx() helpers
> (see http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173514.html)
>
> But this is not right for OMAP2+ SoC where real IPs state is controlled
> by omap_device core which enables/disables modules & clocks actually.
>
> For example, if OMAP I2C driver will handle pinctrl state during system wide
> suspend the following issue may occure:
> - suspend_noirq - I2C device can be still active because of PM auto-suspend
>   |-_od_suspend_noirq
>      |- omap_i2c_suspend_noirq
>         |- PINs state set to SLEEP
>   |- pm_generic_runtime_suspend
>      |- omap_i2c_runtime_suspend()
>         |- PINs state set to IDLE  <--- *oops* PINs state is IDLE and not SLEEP
>   |- omap_device_idle()
>      |- omap_hwmod_idle()
>         |- _idle()
>            |- disbale module (sysc&clocks)
>
> - resume_noirq - I2C was active before suspend
>   |-_od_resume_noirq
>      |- omap_hwmod_enable()
>         |- _enable()
>            |- enable module (sysc&clocks)
>      |- pm_generic_runtime_resume
>         |- omap_i2c_runtime_resume()
>            |- PINs state set to DEFAULT  <--- !!!!
>      |- omap_i2c_resume_noirq
>         |- PINs state set to DEFAULT
>         |- PINs state set to IDLE  <--- *big oops* we have active module and its
>                                          PINs state is IDLE
> (see https://patchwork.kernel.org/patch/2642101/)
>
> Of course, everything can be handled by adding a tons of code in ecah driver to
> check PM state of device and override default behavior of omap_device core, but
> this not good.
>
> Hence, add pinctrl handling in omap_device core:
> 1) on PM runtime resume
> - switch pinctrl state to "default" (todo: "active")
> 2) on PM runtime suspend
> - switch pinctrl state to "idle"
> 3) during system wide suspend
> - switch pinctrl state to "sleep" or "idle" if omap_device core disables device
> - switch pinctrl state to "sleep" if device is disabled already
> 4) during system wide resume
> - switch pinctrl state to "default" (todo: "active") if omap_device core has
>   disabled  device during suspend
> - switch pinctrl state to "idle" if device was already disabled before suspend
>
> This will enable pinctrl for all OMAP2+ IP's drivers by default -
> no changes in code is needed and only DT data will need to be updated
> (add "default", "active", "idle", "sleep" states).
>
> This will enable pinctrl handling for all OMAP2+ drivers by default -
> no changes in code is needed and only DT data will need to be updated
> (add "default", "active", "idle", "sleep" states).
>
> Related discussions:
> - [3/3] i2c: nomadik: use pinctrl PM helpers 
>  https://patchwork.kernel.org/patch/2670291/
> - mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime
>  https://patchwork.kernel.org/patch/2690191/
> - [PATCH 00/11] drivers: Add Pinctrl PM support
>  https://lkml.org/lkml/2013/5/31/210
>
> CC: Hebbar Gururaja <gururaja.hebbar@ti.com>
> CC: Linus Walleij  <linus.walleij@linaro.org>
> CC: linux-arm-kernel@lists.infradead.org
> CC: linux-omap@vger.kernel.org
> CC: linux-kernel@vger.kernel.org 
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> Hi Kevin, Tony,
>
> I've verified this patch on OMAP4 SDP board:
> - PM runtime for I2C4, UART2, UART3
> - suspend/resume with I2C4, UART2, UART3
>
> seems it works and pinctrl states have been updated as expected.

Thanks for working on this.

I agree with the approach, and much prefer this to boiler plate code
throughout the drivers.

I suggest we wait until the dust settles on the active/default thread
before taking this further, but have no objections to the approach.

Kevin

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [130624 05:13]:
> On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
> > Hence, add pinctrl handling in omap_device core:
> > 1) on PM runtime resume
> > - switch pinctrl state to "default" (todo: "active")
> > 2) on PM runtime suspend
> > - switch pinctrl state to "idle"
> > 3) during system wide suspend
> > - switch pinctrl state to "sleep" or "idle" if omap_device core disables device
> > - switch pinctrl state to "sleep" if device is disabled already

Do you need a separate setting for "idle" and "sleep", or are
they the same?

> > 4) during system wide resume
> > - switch pinctrl state to "default" (todo: "active") if omap_device core has
> >   disabled  device during suspend
> > - switch pinctrl state to "idle" if device was already disabled before suspend
> 
> I don't understand step 4.
> 
> I get the creeps about whether the system is runtime suspended
> or runtime resumed when we come to resume proper, so I need
> Kevin to have a look at this.
> 
> Apart from that it looks good.
> 
> Stephen and Tony are trying to figure out the details of whether "active"
> is necessary or not in a related thread I think.

Yes we should have that sorted out over next few weeks, so let's
just wait a little while on all these dynamic remuxing patches
to avoid churn.

Regards,

Tony

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Grygorii Strashko]

Hi All,

On 06/25/2013 09:58 AM, Tony Lindgren wrote:
> * Linus Walleij <linus.walleij@linaro.org> [130624 05:13]:
>> On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>
>>> Hence, add pinctrl handling in omap_device core:
>>> 1) on PM runtime resume
>>> - switch pinctrl state to "default" (todo: "active")
>>> 2) on PM runtime suspend
>>> - switch pinctrl state to "idle"
>>> 3) during system wide suspend
>>> - switch pinctrl state to "sleep" or "idle" if omap_device core disables device
>>> - switch pinctrl state to "sleep" if device is disabled already
> Do you need a separate setting for "idle" and "sleep", or are
> they the same?
>
>>> 4) during system wide resume
>>> - switch pinctrl state to "default" (todo: "active") if omap_device core has
>>>    disabled  device during suspend
>>> - switch pinctrl state to "idle" if device was already disabled before suspend
>> I don't understand step 4.
>>
>> I get the creeps about whether the system is runtime suspended
>> or runtime resumed when we come to resume proper, so I need
>> Kevin to have a look at this.
>>
>> Apart from that it looks good.
>>
>> Stephen and Tony are trying to figure out the details of whether "active"
>> is necessary or not in a related thread I think.
Thanks for your comments.

I've prepared diagram to illustrate how does this work:
                                                  +
                                                  |
                                                  |  .probe()
                                                  |
                                            +-----v--------+
                                            |              |
                                            |  default     |
                                            |              |
                                            +----+--+------+
                                                 |  |
                               pm_runtime_get()  |  | pm_runtime_put()
                              +------------------+ +------------------+
|                                        |
                       +------v--------+ pm_runtime_put() +-------v-------+
                       | +----------------------->               |
           +----------->  Active       |      pm_runtime_get() | 
Idle        <--------------+
           |           | <-----------------------+               
|              |
           |           +-------+-------+ +-------+-------+              |
           | |.suspend_noirq()                       
|.suspend_noirq()      |
           | |                                       
|                      |
           | |                                       
|                      |
           | |                                       
|                      |
           | |                                       
|                      |
           |           +-------v-------+ +-------v--------+             |
           |           |               | |                |             |
           +-----------+  Sleep        ------+                 | 
Sleep         +-------------+
       .resume_noirq() |               |     | |                
|.resume_noirq()
                       +-------|-------+     | +----------------+
                               |   Idle      |
                               +-------------+

The "Sleep" pinctrl state is optional - if "sleep" state isn't defined
then "Idle" pinctrl state will be used during suspend.

As per above pinctrl state transition diagram - I think, that:
- if "idle" or "sleep" states are defined then "active" state have to be 
defined too.
So, pinctrl core should detect such situation and generate a warning.

 From my point of view, we should have "active" state - it would allow 
to avoid additional
runtime overhead and handle properly the case when drivers are used as 
loadable modules.
In this case, after boot, device's pinctrl registers can have HW default 
values (after reset) or
can be configured to some safe (off) state from board file (board's 
"default" configuration).
Driver module will activate its PINs when loaded and need to restore 
safe (off) PINs state when
unloaded.
As result, it seems, we should have "off" state which can be used when 
driver's module is removed.

So, final list of default pnctrl states may be defined as "default", 
"active", "idle", "sleep", "off":
- "active", "idle", "sleep": will be handled by omap_device core
- "default", "off": will be handled by driver itself (or Device core).



> Yes we should have that sorted out over next few weeks, so let's
> just wait a little while on all these dynamic remuxing patches
> to avoid churn.
>
> Regards,
>
> Tony

Regards,
- grygorii

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Grygorii Strashko]

Picture format fixed. Sorry for that.

On 06/26/2013 04:20 PM, Grygorii Strashko wrote:
> Hi All,
>
> On 06/25/2013 09:58 AM, Tony Lindgren wrote:
>> * Linus Walleij <linus.walleij@linaro.org> [130624 05:13]:
>>> On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko
>>> <grygorii.strashko@ti.com> wrote:
>>>
>>>> Hence, add pinctrl handling in omap_device core:
>>>> 1) on PM runtime resume
>>>> - switch pinctrl state to "default" (todo: "active")
>>>> 2) on PM runtime suspend
>>>> - switch pinctrl state to "idle"
>>>> 3) during system wide suspend
>>>> - switch pinctrl state to "sleep" or "idle" if omap_device core
>>>> disables device
>>>> - switch pinctrl state to "sleep" if device is disabled already
>> Do you need a separate setting for "idle" and "sleep", or are
>> they the same?
>>
>>>> 4) during system wide resume
>>>> - switch pinctrl state to "default" (todo: "active") if omap_device
>>>> core has
>>>>    disabled  device during suspend
>>>> - switch pinctrl state to "idle" if device was already disabled
>>>> before suspend
>>> I don't understand step 4.
>>>
>>> I get the creeps about whether the system is runtime suspended
>>> or runtime resumed when we come to resume proper, so I need
>>> Kevin to have a look at this.
>>>
>>> Apart from that it looks good.
>>>
>>> Stephen and Tony are trying to figure out the details of whether
>>> "active"
>>> is necessary or not in a related thread I think.
> Thanks for your comments.
>
> I've prepared diagram to illustrate how does this work:
>                                          +
>                                          |
>                                          |  .probe()
>                                          |
>                                    +-----v--------+
>                                    |              |
>                                    |  default     |
>                                    |              |
>                                    +----+--+------+
>                                         |  |
>                         pm_runtime_get()|  | pm_runtime_put()
>                        +----------------+  +------------+
>                        |                                |
>                 +------v------+ pm_runtime_put()+-------v-----+
>                 |             +----------------->             |
>     +----------->  Active     |pm_runtime_get() |   Idle      <----------+
>     |           |             <-----------------+             |          |
>     |           +-------+-----+                 +-------+-----+          |
>     |                   |.suspend_noirq()               |.suspend_noirq()|
>     |                   |                               |                |
>     |                   |                               |                |
>     |                   |                               |                |
>     |                   |                               |                |
>     |           +-------v-----+                 +-------v-----+          |
>     |           |             |                 |             |          |
>     +-----------+  Sleep      ------+           |  Sleep      +----------+
> .resume_noirq() |             |     |           |             |.resume_noirq()
>                 +-------|-----+     |           +-------------+
>                         |  Idle     |
>                         +-----------+
>
> The "Sleep" pinctrl state is optional - if "sleep" state isn't defined
> then "Idle" pinctrl state will be used during suspend.
>
> As per above pinctrl state transition diagram - I think, that:
> - if "idle" or "sleep" states are defined then "active" state have to be
> defined too.
> So, pinctrl core should detect such situation and generate a warning.
>
>  From my point of view, we should have "active" state - it would allow
> to avoid additional
> runtime overhead and handle properly the case when drivers are used as
> loadable modules.
> In this case, after boot, device's pinctrl registers can have HW default
> values (after reset) or
> can be configured to some safe (off) state from board file (board's
> "default" configuration).
> Driver module will activate its PINs when loaded and need to restore
> safe (off) PINs state when
> unloaded.
> As result, it seems, we should have "off" state which can be used when
> driver's module is removed.
>
> So, final list of default pnctrl states may be defined as "default",
> "active", "idle", "sleep", "off":
> - "active", "idle", "sleep": will be handled by omap_device core
> - "default", "off": will be handled by driver itself (or Device core).
>
>
>
>> Yes we should have that sorted out over next few weeks, so let's
>> just wait a little while on all these dynamic remuxing patches
>> to avoid churn.
>>
>> Regards,
>>
>> Tony
>
> Regards,
> - grygorii
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Linus Walleij]

On Wed, Jun 26, 2013 at 3:20 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> The "Sleep" pinctrl state is optional - if "sleep" state isn't defined
> then "Idle" pinctrl state will be used during suspend.

Why? If we have a clear cut semantic that "idle" is for runtime
suspend, why should it be a fallback for suspend?

You do realize that can just be turned around (as common suspend
is more widely implemented than runtime suspend) so that we
could say that if "idle" does not exist, we go to "sleep" in
runtime suspend.

> So, final list of default pnctrl states may be defined as "default",
> "active", "idle", "sleep", "off":
> - "active", "idle", "sleep": will be handled by omap_device core
> - "default", "off": will be handled by driver itself (or Device core).

Currently the pinctrl system combines what is called "default"
and "active" into one, assuming that all devices shall come up
in the active state.

Also we haven't seen a device that need some "off" state that
is different from "sleep".

If you want to drive this state list home you have to give a
*real world example*.

I want to see a *real* example, for a device and it's pins,
that define totally different things for these states, as a
rationale.

Else we are just defining states to make nice figures or mental
maps and that is not helpful for drivers writers.

Yours,
Linus Walleij

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [130626 12:37]:
> On Wed, Jun 26, 2013 at 3:20 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
> > The "Sleep" pinctrl state is optional - if "sleep" state isn't defined
> > then "Idle" pinctrl state will be used during suspend.
> 
> Why? If we have a clear cut semantic that "idle" is for runtime
> suspend, why should it be a fallback for suspend?
> 
> You do realize that can just be turned around (as common suspend
> is more widely implemented than runtime suspend) so that we
> could say that if "idle" does not exist, we go to "sleep" in
> runtime suspend.
> 
> > So, final list of default pnctrl states may be defined as "default",
> > "active", "idle", "sleep", "off":
> > - "active", "idle", "sleep": will be handled by omap_device core
> > - "default", "off": will be handled by driver itself (or Device core).
> 
> Currently the pinctrl system combines what is called "default"
> and "active" into one, assuming that all devices shall come up
> in the active state.
> 
> Also we haven't seen a device that need some "off" state that
> is different from "sleep".

Right, this is what I've been wondering too.

Do we really need "idle", "sleep", "off"? Or is "idle" enough?
Or "idle" and "sleep"?

Only one of the should be set at a time, but it would be nice
to handle most cases in a generic way in drivers/base/pinctrl.c.
 
> If you want to drive this state list home you have to give a
> *real world example*.
> 
> I want to see a *real* example, for a device and it's pins,
> that define totally different things for these states, as a
> rationale.

Yeah me too :)
 
> Else we are just defining states to make nice figures or mental
> maps and that is not helpful for drivers writers.

Regards,

Tony

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Grygorii Strashko]

Hi Linus,
On 06/26/2013 10:31 PM, Linus Walleij wrote:
> On Wed, Jun 26, 2013 at 3:20 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>
>> The "Sleep" pinctrl state is optional - if "sleep" state isn't defined
>> then "Idle" pinctrl state will be used during suspend.
>
> Why? If we have a clear cut semantic that "idle" is for runtime
> suspend, why should it be a fallback for suspend?

Seems, some misunderstanding is here :)

This is OMAP specific - for OMAP the "Idle" state is typically used, and 
"sleep" state is rather optional. Historically, three states have been 
defined for OMAP:
- "enabled"
- "idle"
- "off"

Most of OMAP drivers use Runtime PM only to manage their states - but, 
any calls to pm_runtime_xxx() helpers are redirected to *OMAP device* 
framework which physically configures power state/clocks/pins of device.

It's the usual situation, when the device is in "Active/ON" state when 
system is going to suspend, because PM runtime *do not allow* to disable 
device during system wide suspend. To workaround this, the *OMAP device* 
framework disables all active devices forcibly at .suspend_noirq() stage 
- and, at this moment, it should apply "Idle" or "Sleep" configuration 
on PINs.

I need to note here that "Sleep" state is/will be *not* used during PM 
runtime operation - it's very specific.

"off" state is used very rare now - and for the cases when device driver 
is removed or device need to completely switched off (HSI omap_hsi.c and 
RPROC remoteproc.c).

I think, In the future the OMAP pinctrl configurations would be manged 
in more flexible way then now (thanks to "pinctrl PM helpers" and you;))
- "Idle" state will be splitted to "Idle"/"sleep"
- "default" state will be splitted to "default"/"active"

>
> You do realize that can just be turned around (as common suspend
> is more widely implemented than runtime suspend) so that we
> could say that if "idle" does not exist, we go to "sleep" in
> runtime suspend.
>
>> So, final list of default pnctrl states may be defined as "default",
>> "active", "idle", "sleep", "off":
>> - "active", "idle", "sleep": will be handled by omap_device core
>> - "default", "off": will be handled by driver itself (or Device core).
>
> Currently the pinctrl system combines what is called "default"
> and "active" into one, assuming that all devices shall come up
> in the active state.
>
> Also we haven't seen a device that need some "off" state that
> is different from "sleep".
>
> If you want to drive this state list home you have to give a
> *real world example*.
>
> I want to see a *real* example, for a device and it's pins,
> that define totally different things for these states, as a
> rationale.
>
> Else we are just defining states to make nice figures or mental
> maps and that is not helpful for drivers writers.
OK. Please pay attention to the following example (taken from TI Android 
product kernel 3.4 - need to have the same in mainline kernel):
static const struct omap_device_pad port1_phy_pads[] __initconst = {
	{
		.name = "usbb1_ulpitll_stp.usbb1_ulpiphy_stp",
		.flags  = OMAP_DEVICE_PAD_REMUX,
		.enable = OMAP_PIN_OUTPUT | OMAP_MUX_MODE4,
		.idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE7,
		.off = OMAP_PIN_OFF_OUTPUT_LOW | OMAP_MUX_MODE7,
	},
	{
		.name = "usbb1_ulpitll_clk.usbb1_ulpiphy_clk",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dir.usbb1_ulpiphy_dir",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_nxt.usbb1_ulpiphy_nxt",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat0.usbb1_ulpiphy_dat0",
		.flags  = OMAP_DEVICE_PAD_REMUX |OMAP_DEVICE_PAD_WAKEUP,
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
		.idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat1.usbb1_ulpiphy_dat1",
		.flags  = OMAP_DEVICE_PAD_REMUX |OMAP_DEVICE_PAD_WAKEUP,
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
		.idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat2.usbb1_ulpiphy_dat2",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat3.usbb1_ulpiphy_dat3",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat4.usbb1_ulpiphy_dat4",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat5.usbb1_ulpiphy_dat5",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat6.usbb1_ulpiphy_dat6",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
	{
		.name = "usbb1_ulpitll_dat7.usbb1_ulpiphy_dat7",
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
	},
};

As you can see, from 12 pins only 3 pins need to be reconfigured while 
switching from "active"->"idle" states and back (and as I mentioned 
above for OMAP "idle" == "sleep" now).

Regarding "OFF" state:
OMAP mux HW defines special state for unused pins which is selected by 
default after reset and need to be selected when device isn't used, for 
example:
_MUXMODE - Functional multiplexing selection for pad
0x0: Select usbb1_hsic_data
0x3: Select gpio_96
0x7: Select safe_mode <<--- pin unused

In opposite, in "sleep" state some pins need to be reconfigured in 
special way to avoid glitches/support wake up/follow HW requirements:
	{
		.name = "usbb1_ulpitll_dat0.usbb1_ulpiphy_dat0",
		.flags  = OMAP_DEVICE_PAD_REMUX |OMAP_DEVICE_PAD_WAKEUP,
		.enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
<<- "active" PIN is input+pulldown
		.idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4,
<<- "idle/sleep" PIN is input+pulldown+wakeup_en
	},
- or -
	{
		.name	= "uart2_rts.uart2_rts",
		.flags  = OMAP_DEVICE_PAD_REMUX,
		.enable	= OMAP_PIN_OUTPUT | OMAP_MUX_MODE0,
<<- "active" PIN is output
		.idle   = OMAP_PIN_OFF_INPUT_PULLUP | OMAP_MUX_MODE7,
<<- "idle/sleep" PIN is input+pullup+safe
	},


More over, OAMP mux HW allow to define special state for PIN which
will be applied in suspend/OFF mode (OFF-mode deepest possible PM state 
for OMAP) automatically.
X_OFFMODEPULLTYPESELECT OffMode mode pullup/down selection for pad
X_OFFMODEPULLUDENABLE OffMode mode pullup/down enable for pad
X_OFFMODEOUTVALUE OffMode mode output value for pad
X_OFFMODEOUTENABLE OffMode mode output enable value for pad
X_OFFMODEENABLE OffMode mode override control for pad

>
> Yours,
> Linus Walleij
>

Regards,
-grygorii

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [130627 07:12]:
> 
> As you can see, from 12 pins only 3 pins need to be reconfigured
> while switching from "active"->"idle" states and back (and as I
> mentioned above for OMAP "idle" == "sleep" now).
> 
> Regarding "OFF" state:
> OMAP mux HW defines special state for unused pins which is selected
> by default after reset and need to be selected when device isn't
> used, for example:
> _MUXMODE - Functional multiplexing selection for pad
> 0x0: Select usbb1_hsic_data
> 0x3: Select gpio_96
> 0x7: Select safe_mode <<--- pin unused

The off mode bits can be enabled continuously, the mux hardware
automatically sets them. So sounds like you don't need any
separate "idle" "sleep" and "off" states, the following should
do:

"default" (or "static")	static pins that don't need to be touched
			after consumer driver probe

"active"		dynamic pins that are not a subset of
			"default" needed for runtime; these pins
			are the same as "idle" below, but with
			different muxing or pinconf device
			runtime

"idle"			dynamic pins that are not a subset of
			"default" needed for various idle modes;
			these pins are the same as "active" above,
			but with different muxing or pinconf for
			various idle states

Can you please confirm that these named modes are enough for
your needs?

If your hardware does not have specific off mode bits, then
I can understand that you may need one mor state "off".

Regards,

Tony

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Grygorii Strashko]

On 06/27/2013 05:45 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [130627 07:12]:
>>
>> As you can see, from 12 pins only 3 pins need to be reconfigured
>> while switching from "active"->"idle" states and back (and as I
>> mentioned above for OMAP "idle" == "sleep" now).
>>
>> Regarding "OFF" state:
>> OMAP mux HW defines special state for unused pins which is selected
>> by default after reset and need to be selected when device isn't
>> used, for example:
>> _MUXMODE - Functional multiplexing selection for pad
>> 0x0: Select usbb1_hsic_data
>> 0x3: Select gpio_96
>> 0x7: Select safe_mode <<--- pin unused
>
> The off mode bits can be enabled continuously, the mux hardware
> automatically sets them. So sounds like you don't need any
> separate "idle" "sleep" and "off" states, the following should
> do:
>
> "default" (or "static")	static pins that don't need to be touched
> 			after consumer driver probe
>
> "active"		dynamic pins that are not a subset of
> 			"default" needed for runtime; these pins
> 			are the same as "idle" below, but with
> 			different muxing or pinconf device
> 			runtime
>
> "idle"			dynamic pins that are not a subset of
> 			"default" needed for various idle modes;
> 			these pins are the same as "active" above,
> 			but with different muxing or pinconf for
> 			various idle states
>
> Can you please confirm that these named modes are enough for
> your needs?
  Confirm )
>
> If your hardware does not have specific off mode bits, then
> I can understand that you may need one mor state "off".
>
> Regards,
>
> Tony
>

Regards,
-grygorii

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Linus Walleij]

On Thu, Jun 27, 2013 at 4:01 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> I think, In the future the OMAP pinctrl configurations would be manged in
> more flexible way then now (thanks to "pinctrl PM helpers" and you;))
> - "Idle" state will be splitted to "Idle"/"sleep"
> - "default" state will be splitted to "default"/"active"

OK so the first ones we already have so the discussion is now down
to adding the "active" state (and pinctrl_pm* helper function).

I guess we need a patch set prepared which adds the "active" state
and helper function as the first patch, i.e. this:
http://marc.info/?l=linux-kernel&m=137094012703340&w=2
Can I have your ACK on this patch?

I do not want to add the state unless there is a clear consumer,
so it needs to go in with the first patch to a device driver that uses
pinctrl_pm_select_active_state() which will be a good demonstration
on its use and utility. (And a point to object and suggest other ways
to do the same thing...)

Yours,
Linus Walleij

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Linus Walleij]

On Thu, Jun 27, 2013 at 4:45 PM, Tony Lindgren <tony@atomide.com> wrote:

> The off mode bits can be enabled continuously, the mux hardware
> automatically sets them. So sounds like you don't need any
> separate "idle" "sleep" and "off" states, the following should
> do:
>
> "default" (or "static") static pins that don't need to be touched
>                         after consumer driver probe

Please use "default" for this. We cannot shoehorn OMAP lingo
into the core kernel and expect everyone to accept that...

> "active"                dynamic pins that are not a subset of
>                         "default" needed for runtime; these pins
>                         are the same as "idle" below, but with
>                         different muxing or pinconf device
>                         runtime
>
> "idle"                  dynamic pins that are not a subset of
>                         "default" needed for various idle modes;
>                         these pins are the same as "active" above,
>                         but with different muxing or pinconf for
>                         various idle states

The misunderstanding earlier was that it was assumed that
"active" == "default", just with another name.

If the above is true with an actual driver patch calling
*both* pinctrl_pm_select_default_state() *and*
pinctrl_pm_select_active_state() in different runpaths I
see no reason to reject it.

But if it turns out that the drivers always use either "default"
or "active" and never both I consider it a pure naming
convention and will not accept the "active" state.

Now it's time for patches :-)

Yours,
Linus Walleij

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [130710 13:42]:
> On Thu, Jun 27, 2013 at 4:01 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
> > I think, In the future the OMAP pinctrl configurations would be manged in
> > more flexible way then now (thanks to "pinctrl PM helpers" and you;))
> > - "Idle" state will be splitted to "Idle"/"sleep"
> > - "default" state will be splitted to "default"/"active"
> 
> OK so the first ones we already have so the discussion is now down
> to adding the "active" state (and pinctrl_pm* helper function).

I think I have a patchset ready for pinctrl to allow multiple simulaneous
states as discussed, need to test it first though. Should be able to
post it on Friday hopefully.
 
> I guess we need a patch set prepared which adds the "active" state
> and helper function as the first patch, i.e. this:
> http://marc.info/?l=linux-kernel&m=137094012703340&w=2
> Can I have your ACK on this patch?

Hmm I have gone a bit further with the drivers/base/pinctrl.c in my
set where if active state is defined then sleep and idle states must
match active state for the pingroups to avoid constantly checking
those sets during runtime.

Then the pinctrl_pm_select_active_state() does not actually have
anything to do with PM in the rx/tx case, so we should rename that.

It's pretty close, but before we can apply that we need the changes
I have to allow multiple simultaneous states. I suggest wait just
few days on that patch.
 
> I do not want to add the state unless there is a clear consumer,
> so it needs to go in with the first patch to a device driver that uses
> pinctrl_pm_select_active_state() which will be a good demonstration
> on its use and utility. (And a point to object and suggest other ways
> to do the same thing...)

Right, we have quite a few consumers with omaps for that as am33xx
requires remuxing wake-up events for all drivers AFAIK. The MMC SDIO
pin remuxing is probably closest one ready.

Regards,

Tony

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Grygorii Strashko]

Hi Linus,

On 07/10/2013 11:36 PM, Linus Walleij wrote:
> On Thu, Jun 27, 2013 at 4:01 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>
>> I think, In the future the OMAP pinctrl configurations would be manged in
>> more flexible way then now (thanks to "pinctrl PM helpers" and you;))
>> - "Idle" state will be splitted to "Idle"/"sleep"
>> - "default" state will be splitted to "default"/"active"
>
> OK so the first ones we already have so the discussion is now down
> to adding the "active" state (and pinctrl_pm* helper function).
>
> I guess we need a patch set prepared which adds the "active" state
> and helper function as the first patch, i.e. this:
> http://marc.info/?l=linux-kernel&m=137094012703340&w=2
> Can I have your ACK on this patch?
I've verified combination of
[PATCH] drivers: pinctrl: add active state to core
[RFC] ARM: OMAP2+: omap_device: add pinctrl handling
on OMAP4 SDP board+UARTs and going to send patches on Monday.

So, Acked/Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
(I can't reply directly to thread - not signed to those lists :))

What is the best branch to base my patches on?

>
> I do not want to add the state unless there is a clear consumer,
> so it needs to go in with the first patch to a device driver that uses
> pinctrl_pm_select_active_state() which will be a good demonstration
> on its use and utility. (And a point to object and suggest other ways
> to do the same thing...)
>
> Yours,
> Linus Walleij
>

Regards,
-grygorii

Re: [RFC] ARM: OMAP2+: omap_device: add pinctrl handling

[Linus Walleij]

On Fri, Jul 12, 2013 at 5:36 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 07/10/2013 11:36 PM, Linus Walleij wrote:
>>
>> I guess we need a patch set prepared which adds the "active" state
>> and helper function as the first patch, i.e. this:
>> http://marc.info/?l=linux-kernel&m=137094012703340&w=2
>> Can I have your ACK on this patch?
>
> I've verified combination of
> [PATCH] drivers: pinctrl: add active state to core
> [RFC] ARM: OMAP2+: omap_device: add pinctrl handling
> on OMAP4 SDP board+UARTs and going to send patches on Monday.
>
> So, Acked/Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
> (I can't reply directly to thread - not signed to those lists :))
>
> What is the best branch to base my patches on?

I suggest you coordinate that with Tony. He can take my patch
with your ACK if it helps, but it seems he's already up to
optimizing the pinctrl core :-)

Yours,
Linus Walleij