Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Tony Lindgren]

* 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

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Tony Lindgren]

* 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

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Charles Keepax]

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

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Tony Lindgren]

* Charles Keepax <ckeepax-yzvPICuk2AA4QjBA90+/kJqQE7yCjDx5@public.gmane.org> [171104 08:38]:
> On Fri, Nov 03, 2017 at 10:33:53AM -0700, Tony Lindgren wrote:
> > * Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Rafael J. Wysocki]

On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote:
> * Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Florian Fainelli]

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

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Tony Lindgren]

* 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 */
-- 
2.15.0

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Rafael J. Wysocki]

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

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Rafael J. Wysocki]

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

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Tony Lindgren]

* 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

Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Tony Lindgren]

* 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