From: Linus Walleij <[email protected]>
In addition to the recently introduced pinctrl core
control, the PM runtime pin control for the OMAP platforms
require a fourth state in addtition to the default, idle and
sleep states already handled by the core: an explicit "active"
state. Let's introduce this to the core in addition to the
other states already defined.
Cc: Hebbar Gururaja <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Stephen Warren <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Kevin Hilman <[email protected]>
Suggested-by: Tony Lindgren <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
Greg: need your ACK on this to merge it through the pinctrl
tree.
---
drivers/base/pinctrl.c | 8 +++++++-
drivers/pinctrl/core.c | 15 +++++++++++++++
include/linux/pinctrl/consumer.h | 10 ++++++++++
include/linux/pinctrl/devinfo.h | 4 ++++
include/linux/pinctrl/pinctrl-state.h | 5 +++++
5 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 5fb74b4..833d7cd 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -51,9 +51,15 @@ int pinctrl_bind_pins(struct device *dev)
#ifdef CONFIG_PM
/*
* If power management is enabled, we also look for the optional
- * sleep and idle pin states, with semantics as defined in
+ * active, sleep and idle pin states, with semantics as defined in
* <linux/pinctrl/pinctrl-state.h>
*/
+ dev->pins->active_state = pinctrl_lookup_state(dev->pins->p,
+ PINCTRL_STATE_ACTIVE);
+ if (IS_ERR(dev->pins->active_state))
+ /* Not supplying this state is perfectly legal */
+ dev_dbg(dev, "no active pinctrl state\n");
+
dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
PINCTRL_STATE_SLEEP);
if (IS_ERR(dev->pins->sleep_state))
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index dca9208..ff01b8f 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1217,6 +1217,21 @@ int pinctrl_pm_select_default_state(struct device *dev)
return ret;
}
+int pinctrl_pm_select_active_state(struct device *dev)
+{
+ struct dev_pin_info *pins = dev->pins;
+ int ret;
+
+ if (!pins)
+ return 0;
+ if (IS_ERR(pins->active_state))
+ return 0; /* No active state */
+ ret = pinctrl_select_state(pins->p, pins->active_state);
+ if (ret)
+ dev_err(dev, "failed to activate pinctrl active state\n");
+ return ret;
+}
+
/**
* pinctrl_pm_select_sleep_state() - select sleep pinctrl state for PM
* @dev: device to select sleep state for
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0f32f10..18cdbb1 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -42,6 +42,7 @@ extern void devm_pinctrl_put(struct pinctrl *p);
#ifdef CONFIG_PM
extern int pinctrl_pm_select_default_state(struct device *dev);
+extern int pinctrl_pm_select_active_state(struct device *dev);
extern int pinctrl_pm_select_sleep_state(struct device *dev);
extern int pinctrl_pm_select_idle_state(struct device *dev);
#else
@@ -49,6 +50,10 @@ static inline int pinctrl_pm_select_default_state(struct device *dev)
{
return 0;
}
+static inline int pinctrl_pm_select_active_state(struct device *dev)
+{
+ return 0;
+}
static inline int pinctrl_pm_select_sleep_state(struct device *dev)
{
return 0;
@@ -223,6 +228,11 @@ static inline int pinctrl_pm_select_default_state(struct device *dev)
return 0;
}
+static inline int pinctrl_pm_select_active_state(struct device *dev)
+{
+ return 0;
+}
+
static inline int pinctrl_pm_select_sleep_state(struct device *dev)
{
return 0;
diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
index 281cb91..a61e6a5 100644
--- a/include/linux/pinctrl/devinfo.h
+++ b/include/linux/pinctrl/devinfo.h
@@ -24,11 +24,15 @@
* struct dev_pin_info - pin state container for devices
* @p: pinctrl handle for the containing device
* @default_state: the default state for the handle, if found
+ * @active_state: the active state for the handle, if found
+ * @sleep_state: the sleep state for the handle, if found
+ * @idle_state: the idle state for the handle, if found
*/
struct dev_pin_info {
struct pinctrl *p;
struct pinctrl_state *default_state;
#ifdef CONFIG_PM
+ struct pinctrl_state *active_state;
struct pinctrl_state *sleep_state;
struct pinctrl_state *idle_state;
#endif
diff --git a/include/linux/pinctrl/pinctrl-state.h b/include/linux/pinctrl/pinctrl-state.h
index b5919f8..37d0cd1 100644
--- a/include/linux/pinctrl/pinctrl-state.h
+++ b/include/linux/pinctrl/pinctrl-state.h
@@ -9,6 +9,10 @@
* hogs to configure muxing and pins at boot, and also as a state
* to go into when returning from sleep and idle in
* .pm_runtime_resume() or ordinary .resume() for example.
+ * @PINCTRL_STATE_ACTIVE: the state the pinctrl handle shall be put
+ * into for explicitly active mode, typically in .pm_runtime_resume()
+ * and other occasions where we want to be sure that the pins are
+ * ready to roll.
* @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into
* when the pins are idle. This is a state where the system is relaxed
* but not fully sleeping - some power may be on but clocks gated for
@@ -20,5 +24,6 @@
* ordinary .suspend() function.
*/
#define PINCTRL_STATE_DEFAULT "default"
+#define PINCTRL_STATE_ACTIVE "active"
#define PINCTRL_STATE_IDLE "idle"
#define PINCTRL_STATE_SLEEP "sleep"
--
1.7.11.3
On Tue, Jun 11, 2013 at 10:16:56AM +0200, Linus Walleij wrote:
> From: Linus Walleij <[email protected]>
>
> In addition to the recently introduced pinctrl core
> control, the PM runtime pin control for the OMAP platforms
> require a fourth state in addtition to the default, idle and
> sleep states already handled by the core: an explicit "active"
> state. Let's introduce this to the core in addition to the
> other states already defined.
>
> Cc: Hebbar Gururaja <[email protected]>
> Cc: Dmitry Torokhov <[email protected]>
> Cc: Stephen Warren <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Suggested-by: Tony Lindgren <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> Greg: need your ACK on this to merge it through the pinctrl
> tree.
> ---
> drivers/base/pinctrl.c | 8 +++++++-
> drivers/pinctrl/core.c | 15 +++++++++++++++
For anything that just touches drivers/base/pinctrl.c, you don't need my
ack, that's your code, you can do whatever you want with it :)
But here you go anyway:
Acked-by: Greg Kroah-Hartman <[email protected]>
On 06/11/2013 02:16 AM, Linus Walleij wrote:
> From: Linus Walleij <[email protected]>
>
> In addition to the recently introduced pinctrl core
> control, the PM runtime pin control for the OMAP platforms
> require a fourth state in addtition to the default, idle and
> sleep states already handled by the core: an explicit "active"
> state. Let's introduce this to the core in addition to the
> other states already defined.
Surely "default" /is/ "active"? That's what it's meant so far.
Since the pinctrl states are represented in DT, the DT bindings of all
devices potentially get affected by changes like this. They'd need to be
documented in a DT binding document, and the exact semantics of the
different states clearly explained.
It may be better for struct device, struct platform_driver, or similar,
to contain a list of the states that are required by the driver or its
binding. That way, the list of states (or states beyond the basic
default) is driver-/DT-binding- specific, and custom stuff like this for
OMAP wouldn't require explicit code in drivers/base/pinctrl.c, but
rather simply iterating over a custom list.
Related to that, I'm not sure we should be deriving what we put into DT
based on the runtime PM requirements of drivers; DT is supposed to be
driven by HW definitions, although I suppose you could argue that the
drivers implement what they do because they're implementing the HW
requirements.
On Tue, Jun 11, 2013 at 6:33 PM, Stephen Warren <[email protected]> wrote:
> On 06/11/2013 02:16 AM, Linus Walleij wrote:
>> From: Linus Walleij <[email protected]>
>>
>> In addition to the recently introduced pinctrl core
>> control, the PM runtime pin control for the OMAP platforms
>> require a fourth state in addtition to the default, idle and
>> sleep states already handled by the core: an explicit "active"
>> state. Let's introduce this to the core in addition to the
>> other states already defined.
>
> Surely "default" /is/ "active"? That's what it's meant so far.
I thought so too, until Tony informed us that in the OMAP world
this state is not always the active one.
But I don't know the exact reasons for.
I guess that some "default" states on the OMAP may mean these
are configured as decoupled, inactive, until explicitly activated
or something like that.
> Since the pinctrl states are represented in DT, the DT bindings of all
> devices potentially get affected by changes like this. They'd need to be
> documented in a DT binding document, and the exact semantics of the
> different states clearly explained.
Incidentally pinctrl-bindings.txt refers to the states
"active" and "idle" in the examples, but not "default".
(git blame tells me you wrote this ;-)
We should probably patch that document a bit to reflect
the semantics we agree upon here.
> It may be better for struct device, struct platform_driver, or similar,
> to contain a list of the states that are required by the driver or its
> binding. That way, the list of states (or states beyond the basic
> default) is driver-/DT-binding- specific, and custom stuff like this for
> OMAP wouldn't require explicit code in drivers/base/pinctrl.c, but
> rather simply iterating over a custom list.
Hm, I was more out to nail down some common semantics
here, especially with these helper functions:
extern int pinctrl_pm_select_default_state(struct device *dev);
extern int pinctrl_pm_select_active_state(struct device *dev);
extern int pinctrl_pm_select_sleep_state(struct device *dev);
extern int pinctrl_pm_select_idle_state(struct device *dev);
I am intending this to be *all* for the PM consensual states.
Any other states will be up to the special cases and drivers to
handle still, I have no intention of retrieveing and caching all
possible states in the core.
> Related to that, I'm not sure we should be deriving what we put into DT
> based on the runtime PM requirements of drivers; DT is supposed to be
> driven by HW definitions, although I suppose you could argue that the
> drivers implement what they do because they're implementing the HW
> requirements.
Yes that's a circular proof that we do it this way becuse this is the
way we have to do it :-)
But there is some truth to that.
The idea with these PM states is to correspond to such states
often found in electronic specifications, i.e. derived from a larger
set of hardware, not from runtime PM or Linux.
I conjured the following doc patch as a starter:
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
==========================================
Yours,
Linus Walleij
* Linus Walleij <[email protected]> [130611 12:59]:
> On Tue, Jun 11, 2013 at 6:33 PM, Stephen Warren <[email protected]> wrote:
> > On 06/11/2013 02:16 AM, Linus Walleij wrote:
> >> From: Linus Walleij <[email protected]>
> >>
> >> In addition to the recently introduced pinctrl core
> >> control, the PM runtime pin control for the OMAP platforms
> >> require a fourth state in addtition to the default, idle and
> >> sleep states already handled by the core: an explicit "active"
> >> state. Let's introduce this to the core in addition to the
> >> other states already defined.
> >
> > Surely "default" /is/ "active"? That's what it's meant so far.
>
> I thought so too, until Tony informed us that in the OMAP world
> this state is not always the active one.
>
> But I don't know the exact reasons for.
>
> I guess that some "default" states on the OMAP may mean these
> are configured as decoupled, inactive, until explicitly activated
> or something like that.
The main reason is that remuxing all the pins for a device for
PM runtime suspend and resume is not necessary. Most pins are
static and configured once during the consumer driver probe.
And in most cases, remuxing only one pin for the rx line is
needed. This is performance critical as it may need to be done
constantly while entering and exiting idle, and remuxing all the
pins is just a waste of cycles.
Regards,
Tony
* Greg Kroah-Hartman <[email protected]> [130611 09:08]:
> On Tue, Jun 11, 2013 at 10:16:56AM +0200, Linus Walleij wrote:
> > From: Linus Walleij <[email protected]>
> >
> > In addition to the recently introduced pinctrl core
> > control, the PM runtime pin control for the OMAP platforms
> > require a fourth state in addtition to the default, idle and
> > sleep states already handled by the core: an explicit "active"
> > state. Let's introduce this to the core in addition to the
> > other states already defined.
> >
> > Cc: Hebbar Gururaja <[email protected]>
> > Cc: Dmitry Torokhov <[email protected]>
> > Cc: Stephen Warren <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Mark Brown <[email protected]>
> > Cc: Kevin Hilman <[email protected]>
> > Suggested-by: Tony Lindgren <[email protected]>
> > Signed-off-by: Linus Walleij <[email protected]>
> > ---
> > Greg: need your ACK on this to merge it through the pinctrl
> > tree.
> > ---
> > drivers/base/pinctrl.c | 8 +++++++-
> > drivers/pinctrl/core.c | 15 +++++++++++++++
>
> For anything that just touches drivers/base/pinctrl.c, you don't need my
> ack, that's your code, you can do whatever you want with it :)
>
> But here you go anyway:
> Acked-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Tony Lindgren <[email protected]>
On 06/12/2013 12:33 PM, Tony Lindgren wrote:
> * Linus Walleij <[email protected]> [130611 12:59]:
>> On Tue, Jun 11, 2013 at 6:33 PM, Stephen Warren <[email protected]> wrote:
>>> On 06/11/2013 02:16 AM, Linus Walleij wrote:
>>>> From: Linus Walleij <[email protected]>
>>>>
>>>> In addition to the recently introduced pinctrl core
>>>> control, the PM runtime pin control for the OMAP platforms
>>>> require a fourth state in addtition to the default, idle and
>>>> sleep states already handled by the core: an explicit "active"
>>>> state. Let's introduce this to the core in addition to the
>>>> other states already defined.
>>>
>>> Surely "default" /is/ "active"? That's what it's meant so far.
>>
>> I thought so too, until Tony informed us that in the OMAP world
>> this state is not always the active one.
>>
>> But I don't know the exact reasons for.
>>
>> I guess that some "default" states on the OMAP may mean these
>> are configured as decoupled, inactive, until explicitly activated
>> or something like that.
>
> The main reason is that remuxing all the pins for a device for
> PM runtime suspend and resume is not necessary. Most pins are
> static and configured once during the consumer driver probe.
> And in most cases, remuxing only one pin for the rx line is
> needed. This is performance critical as it may need to be done
> constantly while entering and exiting idle, and remuxing all the
> pins is just a waste of cycles.
Then isn't the correct solution to optimize the code that switches
between states, so that it only reprograms the pins/groups necessary?
Inventing extra states just to work around that issue seems quite wrong.
So, I think you're relying on:
default: Set up everything static.
active: flip just a few pins to active state.
idle: flip just a few pins to idle state.
... and assuming that the default->active or default->idle transitions
will not affect any pins that are used in default, but not used in
active/idle.
However, that very specifically isn't the case; pinctrl drivers are
allowed to force unused pins back to some default unused state when the
pinctrl state that's being switched to doesn't mention them, and hence
this concept won't work in general.
Also, if default uses more pins that active, when you switch to active,
those pins won't be marked as owned any more, I think, so something else
could in theory grab them. At least, debugfs wouldn't be entirely
accurate any more.
On 06/11/2013 01:53 PM, Linus Walleij wrote:
> On Tue, Jun 11, 2013 at 6:33 PM, Stephen Warren <[email protected]> wrote:
>> On 06/11/2013 02:16 AM, Linus Walleij wrote:
>>> From: Linus Walleij <[email protected]>
>>>
>>> In addition to the recently introduced pinctrl core
>>> control, the PM runtime pin control for the OMAP platforms
>>> require a fourth state in addtition to the default, idle and
>>> sleep states already handled by the core: an explicit "active"
>>> state. Let's introduce this to the core in addition to the
>>> other states already defined.
...
> Incidentally pinctrl-bindings.txt refers to the states
> "active" and "idle" in the examples, but not "default".
> (git blame tells me you wrote this ;-)
Oops.
> We should probably patch that document a bit to reflect
> the semantics we agree upon here.
Yes!
>> It may be better for struct device, struct platform_driver, or similar,
>> to contain a list of the states that are required by the driver or its
>> binding. That way, the list of states (or states beyond the basic
>> default) is driver-/DT-binding- specific, and custom stuff like this for
>> OMAP wouldn't require explicit code in drivers/base/pinctrl.c, but
>> rather simply iterating over a custom list.
>
> Hm, I was more out to nail down some common semantics
> here, especially with these helper functions:
>
> extern int pinctrl_pm_select_default_state(struct device *dev);
> extern int pinctrl_pm_select_active_state(struct device *dev);
> extern int pinctrl_pm_select_sleep_state(struct device *dev);
> extern int pinctrl_pm_select_idle_state(struct device *dev);
>
> I am intending this to be *all* for the PM consensual states.
> Any other states will be up to the special cases and drivers to
> handle still, I have no intention of retrieveing and caching all
> possible states in the core.
I have this dream that somehow the driver core can one day manage all
resource acquisition, and handle all of deferred probe, etc.
Unfortunately, this probably isn't going to happen in general, since
e.g. DT bindings aren't general enough that code can parse the binding
and automatically find all resources the device will need without
explicit knowledge of the binding definition. No doubt similar problems
exist for all forms of device representation.
So, perhaps this is a pipe dream.
Related to that, if you replaced pinctrl_pm_select_XXX_state(dev) with
pinctrl_pm_select_state(dev, XXX), that'd make it generic enough that if
the core could pre-parse all states, or auto-parse any new state name it
hadn't seen before, then the API could in fact work for any state at all...
> I conjured the following doc patch as a starter:
I didn't really read this much, since I think we need to sort out
exactly which set of core states there are first. Just a small comment
below though:
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> +A state-chart diagram would look like this:
> +
> + +---------+ +----------+
> + probe() -> | default | -> runtime_suspend() -> | idle |
> + | | <- runtime_resume() <- | |
> + +---------+ +----------+
> + | ^
> + | |
> + suspend() resume()
> + | |
> + V |
> + +---------+
> + | sleep |
> + +---------+
"active" isn't in that diagram.
* Stephen Warren <[email protected]> [130613 12:37]:
> On 06/12/2013 12:33 PM, Tony Lindgren wrote:
> > * Linus Walleij <[email protected]> [130611 12:59]:
> >> On Tue, Jun 11, 2013 at 6:33 PM, Stephen Warren <[email protected]> wrote:
> >>> On 06/11/2013 02:16 AM, Linus Walleij wrote:
> >>>> From: Linus Walleij <[email protected]>
> >>>>
> >>>> In addition to the recently introduced pinctrl core
> >>>> control, the PM runtime pin control for the OMAP platforms
> >>>> require a fourth state in addtition to the default, idle and
> >>>> sleep states already handled by the core: an explicit "active"
> >>>> state. Let's introduce this to the core in addition to the
> >>>> other states already defined.
> >>>
> >>> Surely "default" /is/ "active"? That's what it's meant so far.
> >>
> >> I thought so too, until Tony informed us that in the OMAP world
> >> this state is not always the active one.
> >>
> >> But I don't know the exact reasons for.
> >>
> >> I guess that some "default" states on the OMAP may mean these
> >> are configured as decoupled, inactive, until explicitly activated
> >> or something like that.
> >
> > The main reason is that remuxing all the pins for a device for
> > PM runtime suspend and resume is not necessary. Most pins are
> > static and configured once during the consumer driver probe.
> > And in most cases, remuxing only one pin for the rx line is
> > needed. This is performance critical as it may need to be done
> > constantly while entering and exiting idle, and remuxing all the
> > pins is just a waste of cycles.
>
> Then isn't the correct solution to optimize the code that switches
> between states, so that it only reprograms the pins/groups necessary?
> Inventing extra states just to work around that issue seems quite wrong.
Hmm how would the pinctrl subsystem know which pins to reprogram and
which ones are static?
> So, I think you're relying on:
>
> default: Set up everything static.
> active: flip just a few pins to active state.
> idle: flip just a few pins to idle state.
>
> ... and assuming that the default->active or default->idle transitions
> will not affect any pins that are used in default, but not used in
> active/idle.
Yes except the default pins are not touched after probe at all.
> However, that very specifically isn't the case; pinctrl drivers are
> allowed to force unused pins back to some default unused state when the
> pinctrl state that's being switched to doesn't mention them, and hence
> this concept won't work in general.
We don't have any default state for pins for omaps at least and pinctrl
single does not not set them to anything when disable is called unless
function-off is specified.
But even if the pinctrl driver does something to the pins in disable,
that should work too. It's just an extra step between toggling between
two named pin states.
> Also, if default uses more pins that active, when you switch to active,
> those pins won't be marked as owned any more, I think, so something else
> could in theory grab them. At least, debugfs wouldn't be entirely
> accurate any more.
I think you're misunderstanding. The default pins are held for as long
as the device is loaded. We're not touching the default pins at all
after probe. Active and idle pins are not subsets of default.
Regards,
Tony
On 06/14/2013 02:46 AM, Tony Lindgren wrote:
> * Stephen Warren <[email protected]> [130613 12:37]:
>> On 06/12/2013 12:33 PM, Tony Lindgren wrote:
>>> * Linus Walleij <[email protected]> [130611 12:59]:
>>>> On Tue, Jun 11, 2013 at 6:33 PM, Stephen Warren <[email protected]> wrote:
>>>>> On 06/11/2013 02:16 AM, Linus Walleij wrote:
>>>>>> From: Linus Walleij <[email protected]>
>>>>>>
>>>>>> In addition to the recently introduced pinctrl core
>>>>>> control, the PM runtime pin control for the OMAP platforms
>>>>>> require a fourth state in addtition to the default, idle and
>>>>>> sleep states already handled by the core: an explicit "active"
>>>>>> state. Let's introduce this to the core in addition to the
>>>>>> other states already defined.
>>>>>
>>>>> Surely "default" /is/ "active"? That's what it's meant so far.
>>>>
>>>> I thought so too, until Tony informed us that in the OMAP world
>>>> this state is not always the active one.
>>>>
>>>> But I don't know the exact reasons for.
>>>>
>>>> I guess that some "default" states on the OMAP may mean these
>>>> are configured as decoupled, inactive, until explicitly activated
>>>> or something like that.
>>>
>>> The main reason is that remuxing all the pins for a device for
>>> PM runtime suspend and resume is not necessary. Most pins are
>>> static and configured once during the consumer driver probe.
>>> And in most cases, remuxing only one pin for the rx line is
>>> needed. This is performance critical as it may need to be done
>>> constantly while entering and exiting idle, and remuxing all the
>>> pins is just a waste of cycles.
>>
>> Then isn't the correct solution to optimize the code that switches
>> between states, so that it only reprograms the pins/groups necessary?
>> Inventing extra states just to work around that issue seems quite wrong.
>
> Hmm how would the pinctrl subsystem know which pins to reprogram and
> which ones are static?
Each state should list the desired configuration of all pins used by the
HW module. Any configuration that's identical between the old an new
state when pinctrl_select_state() executes is static, anything else is not.
>> So, I think you're relying on:
>>
>> default: Set up everything static.
>> active: flip just a few pins to active state.
>> idle: flip just a few pins to idle state.
>>
>> ... and assuming that the default->active or default->idle transitions
>> will not affect any pins that are used in default, but not used in
>> active/idle.
>
> Yes except the default pins are not touched after probe at all.
>> However, that very specifically isn't the case; pinctrl drivers are
>> allowed to force unused pins back to some default unused state when the
>> pinctrl state that's being switched to doesn't mention them, and hence
>> this concept won't work in general.
>
> We don't have any default state for pins for omaps at least and pinctrl
> single does not not set them to anything when disable is called unless
> function-off is specified.
But that's OMAP-specific. If the set of pinctrl states that the core PM
code operates on is documented as general policy, it has to work the
same everywhere.
> But even if the pinctrl driver does something to the pins in disable,
> that should work too. It's just an extra step between toggling between
> two named pin states.
If the "default" state says e.g. "set pin X to function A", and the
"idle" state doesn't say anything about pin X, when a switch from
default -> idle will simply program pin X back to its default state (if
the driver does that) and then not program it to anything else, since
the idle state doesn't say what to program it to.
As I said, this might work fine if the pinctrl driver doesn't do
anything in struct pinmux_ops .disable, but given that it's legal for
the driver to do so, relying on that won't work for all drivers, so some
alternative scheme of handling static pinmux configuration is needed.
>> Also, if default uses more pins that active, when you switch to active,
>> those pins won't be marked as owned any more, I think, so something else
>> could in theory grab them. At least, debugfs wouldn't be entirely
>> accurate any more.
>
> I think you're misunderstanding. The default pins are held for as long
> as the device is loaded. We're not touching the default pins at all
> after probe. Active and idle pins are not subsets of default.
OK, then please provide an example of how this is represented.
On Tue, Jun 11, 2013 at 10:16 AM, Linus Walleij
<[email protected]> wrote:
> From: Linus Walleij <[email protected]>
>
> In addition to the recently introduced pinctrl core
> control, the PM runtime pin control for the OMAP platforms
> require a fourth state in addtition to the default, idle and
> sleep states already handled by the core: an explicit "active"
> state. Let's introduce this to the core in addition to the
> other states already defined.
>
> Cc: Hebbar Gururaja <[email protected]>
> Cc: Dmitry Torokhov <[email protected]>
> Cc: Stephen Warren <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Suggested-by: Tony Lindgren <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
It seems this thing needs another round of discussion so I've pulled
this patch out of the branch with stuff for -next and rebased dependencies.
Yours,
Linus Walleij
* Stephen Warren <[email protected]> [130614 08:29]:
> On 06/14/2013 02:46 AM, Tony Lindgren wrote:
> >
> > Hmm how would the pinctrl subsystem know which pins to reprogram and
> > which ones are static?
>
> Each state should list the desired configuration of all pins used by the
> HW module. Any configuration that's identical between the old an new
> state when pinctrl_select_state() executes is static, anything else is not.
Listing all the pins in each named mode won't work too well from hardware
point of view, I think this can be solved by having a bit finer grained
named modes. I've just replied about this in the related thread
"[PATCH] pinctrl: document the pinctrl PM states".
> > We don't have any default state for pins for omaps at least and pinctrl
> > single does not not set them to anything when disable is called unless
> > function-off is specified.
>
> But that's OMAP-specific. If the set of pinctrl states that the core PM
> code operates on is documented as general policy, it has to work the
> same everywhere.
Agreed. Hopefully this issue too is addressed in the other reply I
mention above.
> > But even if the pinctrl driver does something to the pins in disable,
> > that should work too. It's just an extra step between toggling between
> > two named pin states.
>
> If the "default" state says e.g. "set pin X to function A", and the
> "idle" state doesn't say anything about pin X, when a switch from
> default -> idle will simply program pin X back to its default state (if
> the driver does that) and then not program it to anything else, since
> the idle state doesn't say what to program it to.
>
> As I said, this might work fine if the pinctrl driver doesn't do
> anything in struct pinmux_ops .disable, but given that it's legal for
> the driver to do so, relying on that won't work for all drivers, so some
> alternative scheme of handling static pinmux configuration is needed.
And hopefully this issue too is addressed :)
> >> Also, if default uses more pins that active, when you switch to active,
> >> those pins won't be marked as owned any more, I think, so something else
> >> could in theory grab them. At least, debugfs wouldn't be entirely
> >> accurate any more.
> >
> > I think you're misunderstanding. The default pins are held for as long
> > as the device is loaded. We're not touching the default pins at all
> > after probe. Active and idle pins are not subsets of default.
>
> OK, then please provide an example of how this is represented.
I've listed a few examples in the other thread, so maybe take a look
and see if it addresses your concerns.
Regards,
Tony