| Message ID | 20231110102054.1393570-1-joychakr@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] PM: runtime: Apply pinctrl settings if defined | expand |
On Fri, Nov 10, 2023 at 11:21 AM Joy Chakraborty <joychakr@google.com> wrote: > Apply pinctrl state from runtime framework device state transtion. > > Pinctrl states if defined in DT are bookmarked in device structures > but they need to be explicitly applied from device driver callbacks > which is boiler plate code and also not present in many drivers. > > If there is a specific order of setting pinctrl state with other driver > actions then the device driver can choose to do it from its pm callbacks, > in such a case this call will be a no-op from the pinctrl core framework > since the desired pinctrl state would already be set. > > We could also add a Kconfig knob to enable/disable this, but I do not > see a need to. > > Signed-off-by: Joy Chakraborty <joychakr@google.com> It has a certain beauty to it does it not! The reason it wasn't done like this from the start was, if I recall correctly, that in some cases a device needs to do the pin control state switching in a special sequence with other operations, that can not be reordered, i.e.: 1. The pin control state change is not context-free. 2. The order of events, i.e. context, does not necessarily match the order that Linux subsystems happen to do things. When looking through the kernel tree I don't see that people use the sleep state and idle state much, so we could very well go with this, and then expect people that need special-casing to name their states differently. What do people thing about that? Yours, Linus Walleij
Tue, Nov 14, 2023 at 02:01:48PM +0100, Linus Walleij kirjoitti: > On Fri, Nov 10, 2023 at 11:21 AM Joy Chakraborty <joychakr@google.com> wrote: > > > Apply pinctrl state from runtime framework device state transtion. > > > > Pinctrl states if defined in DT are bookmarked in device structures > > but they need to be explicitly applied from device driver callbacks > > which is boiler plate code and also not present in many drivers. > > > > If there is a specific order of setting pinctrl state with other driver > > actions then the device driver can choose to do it from its pm callbacks, > > in such a case this call will be a no-op from the pinctrl core framework > > since the desired pinctrl state would already be set. > > > > We could also add a Kconfig knob to enable/disable this, but I do not > > see a need to. Besides questionable code style (inline functions in the C file)... > It has a certain beauty to it does it not! > > The reason it wasn't done like this from the start was, if I recall correctly, > that in some cases a device needs to do the pin control state switching > in a special sequence with other operations, that can not be reordered, > i.e.: > > 1. The pin control state change is not context-free. > > 2. The order of events, i.e. context, does not necessarily match the > order that Linux subsystems happen to do things. > > When looking through the kernel tree I don't see that people use > the sleep state and idle state much, so we could very well go > with this, and then expect people that need special-casing to name > their states differently. > > What do people thing about that? ...I think the patch is incomplete(?) due to misterious ways of PM runtime calls. For example, in some cases we force runtime PM during system suspend which may have an undesired effect of the switching pin control states (hence glitches or some real issues with the hardware, up to hanging the system). Some pins may be critical to work with and shuffling their states in an unappropriate time can lead to a disaster. So, I would consider this change okay if and only if it will have a detailed research for all existing users to prove there will be no changes in the whole set of possible scenarious (of system sleep / resume, runtime, runtime with a custom ->prepare callback and so on).
On Wed, Nov 15, 2023 at 7:21 AM <andy.shevchenko@gmail.com> wrote: > > Tue, Nov 14, 2023 at 02:01:48PM +0100, Linus Walleij kirjoitti: > > On Fri, Nov 10, 2023 at 11:21 AM Joy Chakraborty <joychakr@google.com> wrote: > > > > > Apply pinctrl state from runtime framework device state transtion. > > > > > > Pinctrl states if defined in DT are bookmarked in device structures > > > but they need to be explicitly applied from device driver callbacks > > > which is boiler plate code and also not present in many drivers. > > > > > > If there is a specific order of setting pinctrl state with other driver > > > actions then the device driver can choose to do it from its pm callbacks, > > > in such a case this call will be a no-op from the pinctrl core framework > > > since the desired pinctrl state would already be set. > > > > > > We could also add a Kconfig knob to enable/disable this, but I do not > > > see a need to. > > Besides questionable code style (inline functions in the C file)... Sure, I can change that. > > > It has a certain beauty to it does it not! > > > > The reason it wasn't done like this from the start was, if I recall correctly, > > that in some cases a device needs to do the pin control state switching > > in a special sequence with other operations, that can not be reordered, > > i.e.: > > > > 1. The pin control state change is not context-free. > > > > 2. The order of events, i.e. context, does not necessarily match the > > order that Linux subsystems happen to do things. > > > > When looking through the kernel tree I don't see that people use > > the sleep state and idle state much, so we could very well go > > with this, and then expect people that need special-casing to name > > their states differently. > > > > What do people thing about that? > > ...I think the patch is incomplete(?) due to misterious ways of PM runtime > calls. For example, in some cases we force runtime PM during system suspend > which may have an undesired effect of the switching pin control states > (hence glitches or some real issues with the hardware, up to hanging the > system). Some pins may be critical to work with and shuffling their states > in an unappropriate time can lead to a disaster. > > So, I would consider this change okay if and only if it will have a detailed > research for all existing users to prove there will be no changes in the whole > set of possible scenarious (of system sleep / resume, runtime, runtime with a > custom ->prepare callback and so on). > I tried to place the calls to set the pinctrl states after driver/user callback based on my understanding of runtime code so that existing users do get a chance to set the state with any special sequence that needs to be performed post which doing another call to set the state would be ignored in the pinctrl framework. But this only would be possible with the assumption that even in any special sequences executed by users they set nothing but "default" state in runtime_resume, "idle" state in runtime_idle and "'sleep" state in their runtime suspend callbacks. And like Andy mentions about "->prepare callback", if there are drivers that are setting pinctrl state "default", "sleep" or "idle" from any callback but ... int (*runtime_suspend)(struct device *dev); int (*runtime_resume)(struct device *dev); int (*runtime_idle)(struct device *dev); ... it could indeed be a problem. I'll dig into users of pinctrl_select_sleep/default/idle and see if there are such cases or if it could be done in some other way. Thanks Joy > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Nov 16, 2023 at 4:34 PM Joy Chakraborty <joychakr@google.com> wrote: > I tried to place the calls to set the pinctrl states after driver/user > callback based on my understanding of runtime code so that existing > users do get a chance to set the state with any special sequence that > needs to be performed post which doing another call to set the state > would be ignored in the pinctrl framework. This makes sense. (And also is in the original commit.) I think you should actually over-document this by also mentioning this in the kerneldoc above each of the *_try_* callbacks so users simply can't miss this point. > But this only would be possible with the assumption that even in any > special sequences executed by users they set nothing but "default" > state in runtime_resume, "idle" state in runtime_idle and "'sleep" > state in their runtime suspend callbacks. > And like Andy mentions about "->prepare callback", if there are > drivers that are setting pinctrl state "default", "sleep" or "idle" > from any callback but > ... > int (*runtime_suspend)(struct device *dev); > int (*runtime_resume)(struct device *dev); > int (*runtime_idle)(struct device *dev); > ... > it could indeed be a problem. > I'll dig into users of pinctrl_select_sleep/default/idle and see if > there are such cases or if it could be done in some other way. It's worth a check but I doubt much will turn up. The "idle" and "sleep" states are simply not used much in the kernel. Your users will likely be the first. So which hardware target will use this? It's immensely useful to have a good example to point at: that device use "defaul", "sleep", "idle" the idiomatic way. Yours, Linus Walleij
On Fri, Nov 17, 2023 at 1:30 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Nov 16, 2023 at 4:34 PM Joy Chakraborty <joychakr@google.com> wrote: > > > I tried to place the calls to set the pinctrl states after driver/user > > callback based on my understanding of runtime code so that existing > > users do get a chance to set the state with any special sequence that > > needs to be performed post which doing another call to set the state > > would be ignored in the pinctrl framework. > > This makes sense. (And also is in the original commit.) > > I think you should actually over-document this by also mentioning > this in the kerneldoc above each of the *_try_* callbacks so > users simply can't miss this point. > > > But this only would be possible with the assumption that even in any > > special sequences executed by users they set nothing but "default" > > state in runtime_resume, "idle" state in runtime_idle and "'sleep" > > state in their runtime suspend callbacks. > > And like Andy mentions about "->prepare callback", if there are > > drivers that are setting pinctrl state "default", "sleep" or "idle" > > from any callback but > > ... > > int (*runtime_suspend)(struct device *dev); > > int (*runtime_resume)(struct device *dev); > > int (*runtime_idle)(struct device *dev); > > ... > > it could indeed be a problem. > > I'll dig into users of pinctrl_select_sleep/default/idle and see if > > there are such cases or if it could be done in some other way. > > It's worth a check but I doubt much will turn up. The "idle" and > "sleep" states are simply not used much in the kernel. > Right, I will look into this. > Your users will likely be the first. > > So which hardware target will use this? > It's immensely useful to have a good example to point at: > that device use "defaul", "sleep", "idle" the idiomatic way. I see some upstreaming activity on gs101 SOC. I think gs101 and follow on SOCs could use this, I will find that out and get back. > > Yours, > Linus Walleij Thanks a lot for your feedback Joy Joy
diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c index c22864458511..24bb6dba8cb7 100644 --- a/drivers/base/pinctrl.c +++ b/drivers/base/pinctrl.c @@ -103,3 +103,36 @@ int pinctrl_bind_pins(struct device *dev) return 0; } + +#ifdef CONFIG_PM +/** + * pinctrl_state_try_default() - Try setting default state if available + * @dev The device to set pinctrl state + */ +inline void pinctrl_state_try_default(struct device *dev) +{ + if (dev->pins && !IS_ERR(dev->pins->default_state)) + pinctrl_select_state(dev->pins->p, dev->pins->default_state); +} + +/** + * pinctrl_state_try_idle() - Try setting idle state if available + * @dev The device to set pinctrl state + */ +inline void pinctrl_state_try_idle(struct device *dev) +{ + if (dev->pins && !IS_ERR(dev->pins->idle_state)) + pinctrl_select_state(dev->pins->p, dev->pins->idle_state); +} + +/** + * pinctrl_state_try_sleep() - Try setting sleep state if available + * @dev The device to set pinctrl state + */ +inline void pinctrl_state_try_sleep(struct device *dev) +{ + if (dev->pins && !IS_ERR(dev->pins->sleep_state)) + pinctrl_select_state(dev->pins->p, dev->pins->sleep_state); +} +#endif + diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 4545669cb973..a07efa446330 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -9,6 +9,7 @@ #include <linux/ktime.h> #include <linux/hrtimer.h> #include <linux/export.h> +#include <linux/pinctrl/devinfo.h> #include <linux/pm_runtime.h> #include <linux/pm_wakeirq.h> #include <trace/events/rpm.h> @@ -499,7 +500,7 @@ static int rpm_idle(struct device *dev, int rpmflags) /* If no callback assume success. */ if (!callback || dev->power.no_callbacks) - goto out; + goto no_callback; /* Carry out an asynchronous or a synchronous idle notification. */ if (rpmflags & RPM_ASYNC) { @@ -529,6 +530,8 @@ static int rpm_idle(struct device *dev, int rpmflags) dev->power.idle_notification = false; wake_up_all(&dev->power.wait_queue); + no_callback: + pinctrl_state_try_idle(dev); out: trace_rpm_return_int(dev, _THIS_IP_, retval); return retval ? retval : rpm_suspend(dev, rpmflags | RPM_AUTO); @@ -676,6 +679,7 @@ static int rpm_suspend(struct device *dev, int rpmflags) no_callback: __update_runtime_status(dev, RPM_SUSPENDED); pm_runtime_deactivate_timer(dev); + pinctrl_state_try_sleep(dev); if (dev->parent) { parent = dev->parent; @@ -918,6 +922,7 @@ static int rpm_resume(struct device *dev, int rpmflags) no_callback: __update_runtime_status(dev, RPM_ACTIVE); pm_runtime_mark_last_busy(dev); + pinctrl_state_try_default(dev); if (parent) atomic_inc(&parent->power.child_count); } @@ -1899,6 +1904,7 @@ int pm_runtime_force_suspend(struct device *dev) __update_runtime_status(dev, RPM_SUSPENDED); dev->power.needs_force_resume = 1; } + pinctrl_state_try_sleep(dev); return 0; @@ -1946,6 +1952,7 @@ int pm_runtime_force_resume(struct device *dev) } pm_runtime_mark_last_busy(dev); + pinctrl_state_try_default(dev); out: dev->power.needs_force_resume = 0; pm_runtime_enable(dev); diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h index bb6653af4f92..c3a63e96dd37 100644 --- a/include/linux/pinctrl/devinfo.h +++ b/include/linux/pinctrl/devinfo.h @@ -54,6 +54,12 @@ static inline struct pinctrl *dev_pinctrl(struct device *dev) return dev->pins->p; } +#ifdef CONFIG_PM +extern inline void pinctrl_state_try_default(struct device *dev); +extern inline void pinctrl_state_try_idle(struct device *dev); +extern inline void pinctrl_state_try_sleep(struct device *dev); +#endif /* CONFIG_PM */ + #else /* Stubs if we're not using pinctrl */ @@ -73,5 +79,10 @@ static inline struct pinctrl *dev_pinctrl(struct device *dev) return NULL; } +#ifdef CONFIG_PM +static inline void pinctrl_state_try_default(struct device *dev) {} +static inline void pinctrl_state_try_idle(struct device *dev) {} +static inline void pinctrl_state_try_sleep(struct device *dev) {} +#endif /* CONFIG_PM */ #endif /* CONFIG_PINCTRL */ #endif /* PINCTRL_DEVINFO_H */
Apply pinctrl state from runtime framework device state transtion. Pinctrl states if defined in DT are bookmarked in device structures but they need to be explicitly applied from device driver callbacks which is boiler plate code and also not present in many drivers. If there is a specific order of setting pinctrl state with other driver actions then the device driver can choose to do it from its pm callbacks, in such a case this call will be a no-op from the pinctrl core framework since the desired pinctrl state would already be set. We could also add a Kconfig knob to enable/disable this, but I do not see a need to. Signed-off-by: Joy Chakraborty <joychakr@google.com> --- drivers/base/pinctrl.c | 33 +++++++++++++++++++++++++++++++++ drivers/base/power/runtime.c | 9 ++++++++- include/linux/pinctrl/devinfo.h | 11 +++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-)