| Message ID | 20171102231551.16220-1-f.fainelli@gmail.com |
|---|---|
| Headers | show |
| Series | pinctrl: Allow indicating loss of state across suspend/resume | expand |
On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: > Hello Linus, > > It's me again, so I have been thinking about the problem originally > reported in: [PATCH fixes v3] pinctrl: Really force states during suspend/resume > > and other similar patches a while ago, and this new version allows a platform > using pinctrl-single to specify whether its pins are going to lose their state > during a system deep sleep. > > Note that this is still checked at the pinctrl_select_state() because consumers > of the pinctrl API might be calling this from their suspend/resume functions > and should not have to know whether the provider does lose its pin states. > Still feels to me like it should be the providers job to the restore the state rather than expecting the consumer to re-request any state it had. But lets wait and see what Linus thinks. Also not sure if you have seen this chain, but probably worth a look: https://www.spinics.net/lists/devicetree/msg200649.html It is adding support to the GPIO code for controllers that can have options to retain state across reset, not the same but probably at least slightly related to this series. Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Charles Keepax <ckeepax@opensource.cirrus.com> [171103 10:38]: > On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: > > Hello Linus, > > > > It's me again, so I have been thinking about the problem originally > > reported in: [PATCH fixes v3] pinctrl: Really force states during suspend/resume > > > > and other similar patches a while ago, and this new version allows a platform > > using pinctrl-single to specify whether its pins are going to lose their state > > during a system deep sleep. > > > > Note that this is still checked at the pinctrl_select_state() because consumers > > of the pinctrl API might be calling this from their suspend/resume functions > > and should not have to know whether the provider does lose its pin states. > > > > Still feels to me like it should be the providers job to the > restore the state rather than expecting the consumer to > re-request any state it had. But lets wait and see what Linus > thinks. But isn't it the consumer device losing it's state here? Or the pinctrl provider losing it's state? Anyways, the context lost flag should be managed in the PM core for the device, so adding linux-pm and Rafael to Cc. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03/2017 09:11 AM, Tony Lindgren wrote: > * Charles Keepax <ckeepax@opensource.cirrus.com> [171103 10:38]: >> On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: >>> Hello Linus, >>> >>> It's me again, so I have been thinking about the problem originally >>> reported in: [PATCH fixes v3] pinctrl: Really force states during suspend/resume >>> >>> and other similar patches a while ago, and this new version allows a platform >>> using pinctrl-single to specify whether its pins are going to lose their state >>> during a system deep sleep. >>> >>> Note that this is still checked at the pinctrl_select_state() because consumers >>> of the pinctrl API might be calling this from their suspend/resume functions >>> and should not have to know whether the provider does lose its pin states. >>> >> >> Still feels to me like it should be the providers job to the >> restore the state rather than expecting the consumer to >> re-request any state it had. But lets wait and see what Linus >> thinks. > > But isn't it the consumer device losing it's state here? Or the > pinctrl provider losing it's state? The pinctrl provider is losing its state, hence these two patches. > > Anyways, the context lost flag should be managed in the PM core for > the device, so adding linux-pm and Rafael to Cc. I don't think it's that simple but sure, why not.
On 11/03/2017 03:37 AM, Charles Keepax wrote: > On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: >> Hello Linus, >> >> It's me again, so I have been thinking about the problem originally >> reported in: [PATCH fixes v3] pinctrl: Really force states during suspend/resume >> >> and other similar patches a while ago, and this new version allows a platform >> using pinctrl-single to specify whether its pins are going to lose their state >> during a system deep sleep. >> >> Note that this is still checked at the pinctrl_select_state() because consumers >> of the pinctrl API might be calling this from their suspend/resume functions >> and should not have to know whether the provider does lose its pin states. >> > > Still feels to me like it should be the providers job to the > restore the state rather than expecting the consumer to > re-request any state it had. But lets wait and see what Linus > thinks. The mechanism is generic, but the property needs to be placed at the pinctrl provider level anyways. > > Also not sure if you have seen this chain, but probably worth a > look: > > https://www.spinics.net/lists/devicetree/msg200649.html > > It is adding support to the GPIO code for controllers that can > have options to retain state across reset, not the same but > probably at least slightly related to this series. Let me take a closer look and see how much appears applicable. > > Thanks, > Charles >
* Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > The pinctrl provider is losing its state, hence these two patches. OK > > Anyways, the context lost flag should be managed in the PM core for > > the device, so adding linux-pm and Rafael to Cc. > > I don't think it's that simple but sure, why not. Just having bool context_lost in struct dev_pm_info would probably be enough to allow drivers to deal with it. This flag could then be set for a device by power domain related code that knows if context got lost. Anybody got better ideas? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 03, 2017 at 10:33:53AM -0700, Tony Lindgren wrote: > * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > The pinctrl provider is losing its state, hence these two patches. > > OK > > > > Anyways, the context lost flag should be managed in the PM core for > > > the device, so adding linux-pm and Rafael to Cc. > > > > I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. > > Anybody got better ideas? > Should the provider driver not know its state will be lost since will have had its PM ops called, and it should be aware of the state it was in. So doesn't it just need to restore that state on resume? Feels a bit like we are over complicating this here. Apologies if I am missing some here. Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > The pinctrl provider is losing its state, hence these two patches. > > OK > > > > Anyways, the context lost flag should be managed in the PM core for > > > the device, so adding linux-pm and Rafael to Cc. > > > > I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. Something like: if the driver sees "context_lost" set, it should restore the context to the device from memory? But the it would also need to save the context beforehand, so why not to restore it unconditionally on resume? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: >> * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: >>> The pinctrl provider is losing its state, hence these two patches. >> >> OK >> >>>> Anyways, the context lost flag should be managed in the PM core for >>>> the device, so adding linux-pm and Rafael to Cc. >>> >>> I don't think it's that simple but sure, why not. >> >> Just having bool context_lost in struct dev_pm_info would probably >> be enough to allow drivers to deal with it. This flag could then >> be set for a device by power domain related code that knows if >> context got lost. > > Something like: if the driver sees "context_lost" set, it should restore > the context to the device from memory? That is what is being proposed here, except that the actual mechanism where this matters needs to be in the core pinctrl code, otherwise the state (context) is not restored due to a check that attempts not to (re)apply a previous state. > > But the it would also need to save the context beforehand, so why not to > restore it unconditionally on resume? That's what my original attempts did here: https://patchwork.kernel.org/patch/9598969/ but Linus rightfully requested this to be done differently, hence this attempt now to solve it in a slightly more flexible way based on DT properties.
* Florian Fainelli <f.fainelli@gmail.com> [171104 17:21]: > > > On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > >> * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > >>> The pinctrl provider is losing its state, hence these two patches. > >> > >> OK > >> > >>>> Anyways, the context lost flag should be managed in the PM core for > >>>> the device, so adding linux-pm and Rafael to Cc. > >>> > >>> I don't think it's that simple but sure, why not. > >> > >> Just having bool context_lost in struct dev_pm_info would probably > >> be enough to allow drivers to deal with it. This flag could then > >> be set for a device by power domain related code that knows if > >> context got lost. > > > > Something like: if the driver sees "context_lost" set, it should restore > > the context to the device from memory? > > That is what is being proposed here, except that the actual mechanism > where this matters needs to be in the core pinctrl code, otherwise the > state (context) is not restored due to a check that attempts not to > (re)apply a previous state. > > > > > But the it would also need to save the context beforehand, so why not to > > restore it unconditionally on resume? > > That's what my original attempts did here: > > https://patchwork.kernel.org/patch/9598969/ > > but Linus rightfully requested this to be done differently, hence this > attempt now to solve it in a slightly more flexible way based on DT > properties. For runtime PM, restoring the state constantly is unnecessary and not good for battery life. The logic can be just: 1. Device driver runtime PM suspend saves the state when needed 2. Device driver runtime PM resume checks if context_lost was set by the bus or power domain code 3. If context was lost, device driver restores the state, or in some cases may need re-run the driver register init related parts to bring the driver back up, then clears the context_lost flag How about something like the following patch? So far only compile tested with CONFIG_PM enabled. If that looks like the way to go, I'll test it properly and add some comments for the functions and post a proper patch :) Regards, Tony 8< ---------------------- From tony Mon Sep 17 00:00:00 2001 From: Tony Lindgren <tony@atomide.com> Date: Mon, 6 Nov 2017 07:02:54 -0800 Subject: [PATCH] RFC: PM / context: Add support for context lost handling On some hardware device drivers can lose their context in deeper idle states when the power is cut off for some power domain. As the drivers themselves may not be aware if the context was lost without reinitializing the device, we want to provide few helpers for tracking if device context was lost in a generic way. Typically the interconnect or power domain hardware knows when context was lost and we can set up a function for the consumer drivers to query the context lost state. The helpers for context lost that can be used in the following way: 1. Power domain or interconnect code registers a callback function with for a device error = dev_pm_register_context_lost_handler(dev, cb, data); if (error) ... 2. A consumer device driver saves it's state as needed in suspend or runtime PM suspend 3. A power domain is powered off during idle, and context is lost for all the devices in that power domain 4. On resume or runtime PM resume, a device driver queries the context lost state and restores the context if needed error = dev_pm_update_context_lost(dev); if (error) ... if (dev_pm_was_context_lost(dev)) restore_context(ddata); REVISIT: Compile tested only, needs testing and comments Not-Yet-Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/base/power/Makefile | 1 + drivers/base/power/context.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ drivers/base/power/power.h | 6 ++++ include/linux/pm.h | 1 + include/linux/pm_context.h | 51 +++++++++++++++++++++++++++ 5 files changed, 143 insertions(+) create mode 100644 drivers/base/power/context.c create mode 100644 include/linux/pm_context.h diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o +obj-$(CONFIG_PM) += context.o obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o obj-$(CONFIG_PM_TRACE_RTC) += trace.o obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o diff --git a/drivers/base/power/context.c b/drivers/base/power/context.c new file mode 100644 --- /dev/null +++ b/drivers/base/power/context.c @@ -0,0 +1,84 @@ +/* + * context.c - Device context lost helper functions + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/device.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/slab.h> +#include <linux/pm_runtime.h> +#include <linux/pm_context.h> + +#include "power.h" + +bool dev_pm_was_context_lost(struct device *dev) +{ + struct context_lost *ctx = dev->power.context_lost; + unsigned long flags; + + if (!dev || !ctx) + return false; + + spin_lock_irqsave(&dev->power.lock, flags); + if (ctx->cb) + ctx->lost = ctx->cb(dev, ctx->data); + spin_unlock_irqrestore(&dev->power.lock, flags); + + return ctx->lost; +} +EXPORT_SYMBOL_GPL(dev_pm_was_context_lost); + +int dev_pm_register_context_lost_handler(struct device *dev, + bool (*cb)(struct device *dev, + void *data), + void *data) +{ + struct context_lost *ctx = dev->power.context_lost; + unsigned long flags; + + if (!dev || !cb) + return -EINVAL; + + if (ctx) + return -EEXIST; + + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + ctx->data = data; + ctx->cb = cb; + + spin_lock_irqsave(&dev->power.lock, flags); + dev->power.context_lost = ctx; + spin_unlock_irqrestore(&dev->power.lock, flags); + + return 0; +} +EXPORT_SYMBOL_GPL(dev_pm_register_context_lost_handler); + +int dev_pm_unregister_context_lost_handler(struct device *dev) +{ + struct context_lost *ctx = dev->power.context_lost; + unsigned long flags; + + if (!ctx) + return -ENODEV; + + spin_lock_irqsave(&dev->power.lock, flags); + dev->power.context_lost = NULL; + spin_unlock_irqrestore(&dev->power.lock, flags); + kfree(ctx); + + return 0; +} +EXPORT_SYMBOL_GPL(dev_pm_unregister_context_lost_handler); diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h --- a/drivers/base/power/power.h +++ b/drivers/base/power/power.h @@ -39,6 +39,12 @@ extern void dev_pm_enable_wake_irq_check(struct device *dev, bool can_change_status); extern void dev_pm_disable_wake_irq_check(struct device *dev); +struct context_lost { + bool lost; + bool (*cb)(struct device *dev, void *data); + void *data; +}; + #ifdef CONFIG_PM_SLEEP extern int device_wakeup_attach_irq(struct device *dev, diff --git a/include/linux/pm.h b/include/linux/pm.h --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -606,6 +606,7 @@ struct dev_pm_info { struct work_struct work; wait_queue_head_t wait_queue; struct wake_irq *wakeirq; + struct context_lost *context_lost; atomic_t usage_count; atomic_t child_count; unsigned int disable_depth:3; diff --git a/include/linux/pm_context.h b/include/linux/pm_context.h new file mode 100644 --- /dev/null +++ b/include/linux/pm_context.h @@ -0,0 +1,51 @@ +/* + * pm_context.h - Device context lost helper functions + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _LINUX_PM_CONTEXT_H +#define _LINUX_PM_CONTEXT_H + +#ifdef CONFIG_PM + +extern bool dev_pm_was_context_lost(struct device *dev); + +extern int +dev_pm_register_context_lost_handler(struct device *dev, + bool (*cb)(struct device *dev, + void *data), + void *data); + +extern int dev_pm_unregister_context_lost_handler(struct device *dev); + +#else /* !CONFIG_PM */ + +static inline bool dev_pm_was_context_lost(struct device *dev) +{ + return false; +} + +static inline int +dev_pm_register_context_lost_handler(struct device *dev, + bool (*cb)(struct device *dev, + void *data), + void *data); +{ + return -ENODEV; +} + +static inline int dev_pm_unregister_context_lost_handler(struct device *dev) +{ + return -ENODEV; +} + +#endif /* CONFIG_PM */ +#endif /* _LINUX_PM_CONTEXT_H */
* Charles Keepax <ckeepax@opensource.cirrus.com> [171104 08:38]: > On Fri, Nov 03, 2017 at 10:33:53AM -0700, Tony Lindgren wrote: > > * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > > The pinctrl provider is losing its state, hence these two patches. > > > > OK > > > > > > Anyways, the context lost flag should be managed in the PM core for > > > > the device, so adding linux-pm and Rafael to Cc. > > > > > > I don't think it's that simple but sure, why not. > > > > Just having bool context_lost in struct dev_pm_info would probably > > be enough to allow drivers to deal with it. This flag could then > > be set for a device by power domain related code that knows if > > context got lost. > > > > Anybody got better ideas? > > > > Should the provider driver not know its state will be lost since > will have had its PM ops called, and it should be aware of the > state it was in. So doesn't it just need to restore that state on > resume? Feels a bit like we are over complicating this here. > Apologies if I am missing some here. Well any driver can lose context depending on the hardare, this could be both the pinctrl provider and the pinctrl consumer drivers :) If your pinctrl provider driver knows when it lost context and knows when to restore it, then no generic code is needed necessarily for your use case. But as the drivers themselves are not aware themselves when they lost context without checking that all registers are initialized properly, I think we need something generic for tracking the context lost. Typically it's the power domain hardware that knows when context was lost. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > * Florian Fainelli <f.fainelli@gmail.com> [171104 17:21]: > > > > > > On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > >> * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > > >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > >>> The pinctrl provider is losing its state, hence these two patches. > > >> > > >> OK > > >> > > >>>> Anyways, the context lost flag should be managed in the PM core for > > >>>> the device, so adding linux-pm and Rafael to Cc. > > >>> > > >>> I don't think it's that simple but sure, why not. > > >> > > >> Just having bool context_lost in struct dev_pm_info would probably > > >> be enough to allow drivers to deal with it. This flag could then > > >> be set for a device by power domain related code that knows if > > >> context got lost. > > > > > > Something like: if the driver sees "context_lost" set, it should restore > > > the context to the device from memory? > > > > That is what is being proposed here, except that the actual mechanism > > where this matters needs to be in the core pinctrl code, otherwise the > > state (context) is not restored due to a check that attempts not to > > (re)apply a previous state. > > > > > > > > But the it would also need to save the context beforehand, so why not to > > > restore it unconditionally on resume? > > > > That's what my original attempts did here: > > > > https://patchwork.kernel.org/patch/9598969/ > > > > but Linus rightfully requested this to be done differently, hence this > > attempt now to solve it in a slightly more flexible way based on DT > > properties. > > For runtime PM, restoring the state constantly is unnecessary and not > good for battery life. The logic can be just: > > 1. Device driver runtime PM suspend saves the state when needed > > 2. Device driver runtime PM resume checks if context_lost was set by > the bus or power domain code > > 3. If context was lost, device driver restores the state, or in some > cases may need re-run the driver register init related parts > to bring the driver back up, then clears the context_lost flag > > How about something like the following patch? So far only compile > tested with CONFIG_PM enabled. If that looks like the way to go, > I'll test it properly and add some comments for the functions and > post a proper patch :) Honestly, I'm not sure. I'd rather have a context_lost flag to start with and see how/if drivers will use that before adding any common infra for handling this. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: >> * Florian Fainelli <f.fainelli@gmail.com> [171104 17:21]: >>> >>> >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: >>>> On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: >>>>> * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: >>>>>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: >>>>>> The pinctrl provider is losing its state, hence these two patches. >>>>> >>>>> OK >>>>> >>>>>>> Anyways, the context lost flag should be managed in the PM core for >>>>>>> the device, so adding linux-pm and Rafael to Cc. >>>>>> >>>>>> I don't think it's that simple but sure, why not. >>>>> >>>>> Just having bool context_lost in struct dev_pm_info would probably >>>>> be enough to allow drivers to deal with it. This flag could then >>>>> be set for a device by power domain related code that knows if >>>>> context got lost. >>>> >>>> Something like: if the driver sees "context_lost" set, it should restore >>>> the context to the device from memory? >>> >>> That is what is being proposed here, except that the actual mechanism >>> where this matters needs to be in the core pinctrl code, otherwise the >>> state (context) is not restored due to a check that attempts not to >>> (re)apply a previous state. >>> >>>> >>>> But the it would also need to save the context beforehand, so why not to >>>> restore it unconditionally on resume? >>> >>> That's what my original attempts did here: >>> >>> https://patchwork.kernel.org/patch/9598969/ >>> >>> but Linus rightfully requested this to be done differently, hence this >>> attempt now to solve it in a slightly more flexible way based on DT >>> properties. >> >> For runtime PM, restoring the state constantly is unnecessary and not >> good for battery life. The logic can be just: >> >> 1. Device driver runtime PM suspend saves the state when needed >> >> 2. Device driver runtime PM resume checks if context_lost was set by >> the bus or power domain code >> >> 3. If context was lost, device driver restores the state, or in some >> cases may need re-run the driver register init related parts >> to bring the driver back up, then clears the context_lost flag >> >> How about something like the following patch? So far only compile >> tested with CONFIG_PM enabled. If that looks like the way to go, >> I'll test it properly and add some comments for the functions and >> post a proper patch :) > > Honestly, I'm not sure. > > I'd rather have a context_lost flag to start with and see how/if > drivers will use that before adding any common infra for handling > this. I am afraid we are being slightly side tracked here on this context_loss flag, because the crux of the problem is not whether a driver knows or not when it loses state, it's more than the pinctrl core refuses to re-apply the same state upon resumption even when the consumer driver tells it to, because it has not seen a transition (consider it as a stale software cache of the state), and there is only one state defined. I don't particularly care how its gets solved, at the generic device driver model or at the pinctrl level, but I think the pinctrl code needs to change in that regard no matter what we do, because right now, if you call pinctrl_select_state() in your driver's resume function, and there is only one state defined, nothing happens, that's a problem.
On Wednesday, November 8, 2017 1:28:24 AM CET Florian Fainelli wrote: > On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > >> * Florian Fainelli <f.fainelli@gmail.com> [171104 17:21]: > >>> > >>> > >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > >>>> On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > >>>>> * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > >>>>>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > >>>>>> The pinctrl provider is losing its state, hence these two patches. > >>>>> > >>>>> OK > >>>>> > >>>>>>> Anyways, the context lost flag should be managed in the PM core for > >>>>>>> the device, so adding linux-pm and Rafael to Cc. > >>>>>> > >>>>>> I don't think it's that simple but sure, why not. > >>>>> > >>>>> Just having bool context_lost in struct dev_pm_info would probably > >>>>> be enough to allow drivers to deal with it. This flag could then > >>>>> be set for a device by power domain related code that knows if > >>>>> context got lost. > >>>> > >>>> Something like: if the driver sees "context_lost" set, it should restore > >>>> the context to the device from memory? > >>> > >>> That is what is being proposed here, except that the actual mechanism > >>> where this matters needs to be in the core pinctrl code, otherwise the > >>> state (context) is not restored due to a check that attempts not to > >>> (re)apply a previous state. > >>> > >>>> > >>>> But the it would also need to save the context beforehand, so why not to > >>>> restore it unconditionally on resume? > >>> > >>> That's what my original attempts did here: > >>> > >>> https://patchwork.kernel.org/patch/9598969/ > >>> > >>> but Linus rightfully requested this to be done differently, hence this > >>> attempt now to solve it in a slightly more flexible way based on DT > >>> properties. > >> > >> For runtime PM, restoring the state constantly is unnecessary and not > >> good for battery life. The logic can be just: > >> > >> 1. Device driver runtime PM suspend saves the state when needed > >> > >> 2. Device driver runtime PM resume checks if context_lost was set by > >> the bus or power domain code > >> > >> 3. If context was lost, device driver restores the state, or in some > >> cases may need re-run the driver register init related parts > >> to bring the driver back up, then clears the context_lost flag > >> > >> How about something like the following patch? So far only compile > >> tested with CONFIG_PM enabled. If that looks like the way to go, > >> I'll test it properly and add some comments for the functions and > >> post a proper patch :) > > > > Honestly, I'm not sure. > > > > I'd rather have a context_lost flag to start with and see how/if > > drivers will use that before adding any common infra for handling > > this. > > I am afraid we are being slightly side tracked here on this context_loss > flag, because the crux of the problem is not whether a driver knows or > not when it loses state, it's more than the pinctrl core refuses to > re-apply the same state upon resumption even when the consumer driver > tells it to, because it has not seen a transition (consider it as a > stale software cache of the state), and there is only one state defined. > > I don't particularly care how its gets solved, at the generic device > driver model or at the pinctrl level, but I think the pinctrl code needs > to change in that regard no matter what we do, because right now, if you > call pinctrl_select_state() in your driver's resume function, and there > is only one state defined, nothing happens, that's a problem. I see, thanks for explaining that clearly. Then it looks like that there needs to be a way for the "cached" state to be invalidated and the question is what that way should be and how it is going to be triggered. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Rafael J. Wysocki <rjw@rjwysocki.net> [171108 00:25]: > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > > For runtime PM, restoring the state constantly is unnecessary and not > > good for battery life. The logic can be just: > > > > 1. Device driver runtime PM suspend saves the state when needed > > > > 2. Device driver runtime PM resume checks if context_lost was set by > > the bus or power domain code > > > > 3. If context was lost, device driver restores the state, or in some > > cases may need re-run the driver register init related parts > > to bring the driver back up, then clears the context_lost flag > > > > How about something like the following patch? So far only compile > > tested with CONFIG_PM enabled. If that looks like the way to go, > > I'll test it properly and add some comments for the functions and > > post a proper patch :) > > Honestly, I'm not sure. > > I'd rather have a context_lost flag to start with and see how/if > drivers will use that before adding any common infra for handling > this. Right, I'll provide some use cases but it will be a little while. Currently it's done in non-generic way at the interconnect code for my use cases: $ git grep "\.context_offs = " arch/arm/mach-omap2/*data.c | wc -l 276 It seems that we could have genpd take care of this in a generic way with the patch I posted. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Rafael J. Wysocki <rjw@rjwysocki.net> [171108 00:47]: > On Wednesday, November 8, 2017 1:28:24 AM CET Florian Fainelli wrote: > > On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > > > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > > >> * Florian Fainelli <f.fainelli@gmail.com> [171104 17:21]: > > >>> > > >>> > > >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > >>>> On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > >>>>> * Florian Fainelli <f.fainelli@gmail.com> [171103 17:04]: > > >>>>>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > >>>>>> The pinctrl provider is losing its state, hence these two patches. > > >>>>> > > >>>>> OK > > >>>>> > > >>>>>>> Anyways, the context lost flag should be managed in the PM core for > > >>>>>>> the device, so adding linux-pm and Rafael to Cc. > > >>>>>> > > >>>>>> I don't think it's that simple but sure, why not. > > >>>>> > > >>>>> Just having bool context_lost in struct dev_pm_info would probably > > >>>>> be enough to allow drivers to deal with it. This flag could then > > >>>>> be set for a device by power domain related code that knows if > > >>>>> context got lost. > > >>>> > > >>>> Something like: if the driver sees "context_lost" set, it should restore > > >>>> the context to the device from memory? > > >>> > > >>> That is what is being proposed here, except that the actual mechanism > > >>> where this matters needs to be in the core pinctrl code, otherwise the > > >>> state (context) is not restored due to a check that attempts not to > > >>> (re)apply a previous state. > > >>> > > >>>> > > >>>> But the it would also need to save the context beforehand, so why not to > > >>>> restore it unconditionally on resume? > > >>> > > >>> That's what my original attempts did here: > > >>> > > >>> https://patchwork.kernel.org/patch/9598969/ > > >>> > > >>> but Linus rightfully requested this to be done differently, hence this > > >>> attempt now to solve it in a slightly more flexible way based on DT > > >>> properties. > > >> > > >> For runtime PM, restoring the state constantly is unnecessary and not > > >> good for battery life. The logic can be just: > > >> > > >> 1. Device driver runtime PM suspend saves the state when needed > > >> > > >> 2. Device driver runtime PM resume checks if context_lost was set by > > >> the bus or power domain code > > >> > > >> 3. If context was lost, device driver restores the state, or in some > > >> cases may need re-run the driver register init related parts > > >> to bring the driver back up, then clears the context_lost flag > > >> > > >> How about something like the following patch? So far only compile > > >> tested with CONFIG_PM enabled. If that looks like the way to go, > > >> I'll test it properly and add some comments for the functions and > > >> post a proper patch :) > > > > > > Honestly, I'm not sure. > > > > > > I'd rather have a context_lost flag to start with and see how/if > > > drivers will use that before adding any common infra for handling > > > this. > > > > I am afraid we are being slightly side tracked here on this context_loss > > flag, because the crux of the problem is not whether a driver knows or > > not when it loses state, it's more than the pinctrl core refuses to > > re-apply the same state upon resumption even when the consumer driver > > tells it to, because it has not seen a transition (consider it as a > > stale software cache of the state), and there is only one state defined. > > > > I don't particularly care how its gets solved, at the generic device > > driver model or at the pinctrl level, but I think the pinctrl code needs > > to change in that regard no matter what we do, because right now, if you > > call pinctrl_select_state() in your driver's resume function, and there > > is only one state defined, nothing happens, that's a problem. > > I see, thanks for explaining that clearly. > > Then it looks like that there needs to be a way for the "cached" state to be > invalidated and the question is what that way should be and how it is going to > be triggered. Yeah seems like that issue needs to be solved at the pinctrl level even if the drivers were using a generic state for the context lost for figuring out when to do it. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html