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

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

[Florian Fainelli]

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.

This is against your pinctrl/for-next branch.

Thanks!

Changes in v2:

- make the property generic and not specific to pinctrl-single

Florian Fainelli (2):
  pinctrl: Allow a device to indicate when to force a state
  pinctrl: Allow indicating loss of pin states during low-power

 .../devicetree/bindings/pinctrl/pinctrl-bindings.txt    |  4 ++++
 drivers/pinctrl/core.c                                  | 17 ++++++++++++++++-
 drivers/pinctrl/core.h                                  |  4 ++++
 3 files changed, 24 insertions(+), 1 deletion(-)

-- 
2.9.3

[PATCH v2 1/2] pinctrl: Allow a device to indicate when to force a state

[Florian Fainelli]

It may happen that a device needs to force applying a state, e.g:
because it only defines one state of pin states (default) but loses
power/register contents when entering low power modes. Add a
pinctrl_dev::flags bitmask to help describe future quirks and define
PINCTRL_FLG_FORCE_STATE as such a settable flag.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pinctrl/core.c | 14 +++++++++++++-
 drivers/pinctrl/core.h |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4c8d5b23e4d0..c91359d48aa1 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1197,9 +1197,21 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 {
 	struct pinctrl_setting *setting, *setting2;
 	struct pinctrl_state *old_state = p->state;
+	bool force = false;
 	int ret;
 
-	if (p->state == state)
+	if (p->state) {
+		list_for_each_entry(setting, &p->state->settings, node) {
+			if (setting->pctldev->flags & PINCTRL_FLG_FORCE_STATE)
+				force = true;
+		}
+	}
+
+	/* Some controllers may want to force this operation when they define
+	 * only one set of functions and lose power state, e.g: pinctrl-single
+	 * with its pinctrl-single,low-power-state-loss property.
+	 */
+	if (p->state == state && !force)
 		return 0;
 
 	if (p->state) {
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 8cf2eba17c8c..8f900e152295 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -39,6 +39,7 @@ struct pinctrl_gpio_range;
  * @hog_sleep: sleep state for pins hogged by this device
  * @mutex: mutex taken on each pin controller specific action
  * @device_root: debugfs root for this device
+ * @flags: feature/quirk flags
  */
 struct pinctrl_dev {
 	struct list_head node;
@@ -63,8 +64,11 @@ struct pinctrl_dev {
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *device_root;
 #endif
+	unsigned long flags;
 };
 
+#define PINCTRL_FLG_FORCE_STATE	(1 << 0)
+
 /**
  * struct pinctrl - per-device pin control state holder
  * @node: global list node
-- 
2.9.3

Re: [PATCH v2 1/2] pinctrl: Allow a device to indicate when to force a state

[Linus Walleij]

On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:

> It may happen that a device needs to force applying a state, e.g:
> because it only defines one state of pin states (default) but loses
> power/register contents when entering low power modes. Add a
> pinctrl_dev::flags bitmask to help describe future quirks and define
> PINCTRL_FLG_FORCE_STATE as such a settable flag.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

So if I understand correctly, the state is lost across
suspend/resume, correct?

Or are we even talking runtime PM runtime_suspend
and runtime_resume here?

> @@ -1197,9 +1197,21 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>  {
>         struct pinctrl_setting *setting, *setting2;
>         struct pinctrl_state *old_state = p->state;
> +       bool force = false;
>         int ret;
>
> -       if (p->state == state)
> +       if (p->state) {
> +               list_for_each_entry(setting, &p->state->settings, node) {
> +                       if (setting->pctldev->flags & PINCTRL_FLG_FORCE_STATE)
> +                               force = true;
> +               }
> +       }
> +
> +       /* Some controllers may want to force this operation when they define
> +        * only one set of functions and lose power state, e.g: pinctrl-single
> +        * with its pinctrl-single,low-power-state-loss property.
> +        */
> +       if (p->state == state && !force)
>                 return 0;

So the idea is we go and change the state even if we are in the right
state already, I understand that much.

But how is pinctrl_select_state() being called in the first place under
these circumstances?

If this comes from the resume() callback in .pm of the device driver,
would not the same thing be achived if you just set some mock
"sleep" state in suspend()? It could even have exactly the same settings
as the "default" state, as long as it is another state, the register
will be reprogrammed.

See further include/linux/pinctrl/pinctrl-state.h

Yours,
Linus Walleij

Re: [PATCH v2 1/2] pinctrl: Allow a device to indicate when to force a state

[Tony Lindgren]

* Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
> It may happen that a device needs to force applying a state, e.g:
> because it only defines one state of pin states (default) but loses
> power/register contents when entering low power modes. Add a
> pinctrl_dev::flags bitmask to help describe future quirks and define
> PINCTRL_FLG_FORCE_STATE as such a settable flag.

It makes sense to tag the existing state with the context loss
information as otherwise we'll be duplicating the state in the
pinctrl driver potentially for hundreds of pins.

Maybe this patch description should clarify that it's the
pinctrl device restoring the pin state, not the pinctrl
consumer devices?

So maybe just "a pinctrl device needs to force apply a state"
instead of just device above?

Regards,

Tony

Re: [PATCH v2 1/2] pinctrl: Allow a device to indicate when to force a state

[Florian Fainelli]

On 11/29/2017 09:01 AM, Tony Lindgren wrote:
> * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
>> It may happen that a device needs to force applying a state, e.g:
>> because it only defines one state of pin states (default) but loses
>> power/register contents when entering low power modes. Add a
>> pinctrl_dev::flags bitmask to help describe future quirks and define
>> PINCTRL_FLG_FORCE_STATE as such a settable flag.
> 
> It makes sense to tag the existing state with the context loss
> information as otherwise we'll be duplicating the state in the
> pinctrl driver potentially for hundreds of pins.
> 
> Maybe this patch description should clarify that it's the
> pinctrl device restoring the pin state, not the pinctrl
> consumer devices?
> 
> So maybe just "a pinctrl device needs to force apply a state"
> instead of just device above?

It's a bit more involved than that, the pinctrl consumer device might
want to restore a particular state by calling pinctrl_select_state(),
however, because of the (p->state == state)check, the pinctrl provider
driver has no chance of making that call do the actual HW programming.
-- 
Florian

Re: [PATCH v2 1/2] pinctrl: Allow a device to indicate when to force a state

[Tony Lindgren]

* Florian Fainelli <f.fainelli@gmail.com> [171129 17:37]:
> On 11/29/2017 09:01 AM, Tony Lindgren wrote:
> > * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
> >> It may happen that a device needs to force applying a state, e.g:
> >> because it only defines one state of pin states (default) but loses
> >> power/register contents when entering low power modes. Add a
> >> pinctrl_dev::flags bitmask to help describe future quirks and define
> >> PINCTRL_FLG_FORCE_STATE as such a settable flag.
> > 
> > It makes sense to tag the existing state with the context loss
> > information as otherwise we'll be duplicating the state in the
> > pinctrl driver potentially for hundreds of pins.
> > 
> > Maybe this patch description should clarify that it's the
> > pinctrl device restoring the pin state, not the pinctrl
> > consumer devices?
> > 
> > So maybe just "a pinctrl device needs to force apply a state"
> > instead of just device above?
> 
> It's a bit more involved than that, the pinctrl consumer device might
> want to restore a particular state by calling pinctrl_select_state(),
> however, because of the (p->state == state)check, the pinctrl provider
> driver has no chance of making that call do the actual HW programming.

Hmm but isn't it the pinctrl provider device losing context here?
I think the restore of the pin state should somehow happen automatically
by the pinctrl provider driver without a need for the pinctrl consumer
drivers to do anything.

Or what's the use case for pinctrl consumer driver wanting to store
a pin?

Regards,

Tony

Re: [PATCH v2 1/2] pinctrl: Allow a device to indicate when to force a state

[Florian Fainelli]

On 11/29/2017 09:45 AM, Tony Lindgren wrote:
> * Florian Fainelli <f.fainelli@gmail.com> [171129 17:37]:
>> On 11/29/2017 09:01 AM, Tony Lindgren wrote:
>>> * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
>>>> It may happen that a device needs to force applying a state, e.g:
>>>> because it only defines one state of pin states (default) but loses
>>>> power/register contents when entering low power modes. Add a
>>>> pinctrl_dev::flags bitmask to help describe future quirks and define
>>>> PINCTRL_FLG_FORCE_STATE as such a settable flag.
>>>
>>> It makes sense to tag the existing state with the context loss
>>> information as otherwise we'll be duplicating the state in the
>>> pinctrl driver potentially for hundreds of pins.
>>>
>>> Maybe this patch description should clarify that it's the
>>> pinctrl device restoring the pin state, not the pinctrl
>>> consumer devices?
>>>
>>> So maybe just "a pinctrl device needs to force apply a state"
>>> instead of just device above?
>>
>> It's a bit more involved than that, the pinctrl consumer device might
>> want to restore a particular state by calling pinctrl_select_state(),
>> however, because of the (p->state == state)check, the pinctrl provider
>> driver has no chance of making that call do the actual HW programming.
> 
> Hmm but isn't it the pinctrl provider device losing context here?

It is the pinctrl provider indeed.

> I think the restore of the pin state should somehow happen automatically
> by the pinctrl provider driver without a need for the pinctrl consumer
> drivers to do anything.

Correct.

> 
> Or what's the use case for pinctrl consumer driver wanting to store
> a pin?

I actually meant that a consumer driver could aalso call
pinctrl_select_state() in one of its resume callback for instance, but
if the pinctrl provider driver does nothing (or rather the core, on
behalf of the provider), this would be an issue. This was not super
clear, so I will stop using that example from now on :)
-- 
Florian

Re: [PATCH v2 1/2] pinctrl: Allow a device to indicate when to force a state

[Tony Lindgren]

* Florian Fainelli <f.fainelli@gmail.com> [171129 18:17]:
> On 11/29/2017 09:45 AM, Tony Lindgren wrote:
> > * Florian Fainelli <f.fainelli@gmail.com> [171129 17:37]:
> >> On 11/29/2017 09:01 AM, Tony Lindgren wrote:
> >>> * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
> >>>> It may happen that a device needs to force applying a state, e.g:
> >>>> because it only defines one state of pin states (default) but loses
> >>>> power/register contents when entering low power modes. Add a
> >>>> pinctrl_dev::flags bitmask to help describe future quirks and define
> >>>> PINCTRL_FLG_FORCE_STATE as such a settable flag.
> >>>
> >>> It makes sense to tag the existing state with the context loss
> >>> information as otherwise we'll be duplicating the state in the
> >>> pinctrl driver potentially for hundreds of pins.
> >>>
> >>> Maybe this patch description should clarify that it's the
> >>> pinctrl device restoring the pin state, not the pinctrl
> >>> consumer devices?
> >>>
> >>> So maybe just "a pinctrl device needs to force apply a state"
> >>> instead of just device above?
> >>
> >> It's a bit more involved than that, the pinctrl consumer device might
> >> want to restore a particular state by calling pinctrl_select_state(),
> >> however, because of the (p->state == state)check, the pinctrl provider
> >> driver has no chance of making that call do the actual HW programming.
> > 
> > Hmm but isn't it the pinctrl provider device losing context here?
> 
> It is the pinctrl provider indeed.
> 
> > I think the restore of the pin state should somehow happen automatically
> > by the pinctrl provider driver without a need for the pinctrl consumer
> > drivers to do anything.
> 
> Correct.

OK thanks for confirming that.

> > Or what's the use case for pinctrl consumer driver wanting to store
> > a pin?
> 
> I actually meant that a consumer driver could aalso call
> pinctrl_select_state() in one of its resume callback for instance, but
> if the pinctrl provider driver does nothing (or rather the core, on
> behalf of the provider), this would be an issue. This was not super
> clear, so I will stop using that example from now on :)

OK yeah that's probably where the confusion comes from :)

Tony

[PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Florian Fainelli]

Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose
their register contents when entering their lower power state. In such a
case, the pinctrl-single driver that is used will not be able to restore
the power states without telling the core about it and having
pinctrl_select_state() check for that.

This patch adds a new optional boolean property that Device Tree can
define in order to obtain exactly that and having the core pinctrl code
take that into account.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 4 ++++
 drivers/pinctrl/core.c                                         | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index ad9bbbba36e9..cc9bae3b7c33 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -103,6 +103,10 @@ Optional properties:
 #pinctrl-cells:	Number of pin control cells in addition to the index within the
 		pin controller device instance
 
+low-power-state-loss: boolean property which indicates that the pins lose their
+state during low power modes and therefore need to be restored upon system
+resumption.
+
 Pin controller devices should contain the pin configuration nodes that client
 devices reference.
 
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c91359d48aa1..3fee457999b5 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1978,6 +1978,9 @@ pinctrl_init_controller(struct pinctrl_desc *pctldesc, struct device *dev,
 	pctldev->dev = dev;
 	mutex_init(&pctldev->mutex);
 
+	if (of_property_read_bool(dev->of_node, "low-power-state-loss"))
+		pctldev->flags |= PINCTRL_FLG_FORCE_STATE;
+
 	/* check core ops for sanity */
 	ret = pinctrl_check_ops(pctldev);
 	if (ret) {
-- 
2.9.3

Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Linus Walleij]

On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:

> Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose
> their register contents when entering their lower power state. In such a
> case, the pinctrl-single driver that is used will not be able to restore
> the power states without telling the core about it and having
> pinctrl_select_state() check for that.
>
> This patch adds a new optional boolean property that Device Tree can
> define in order to obtain exactly that and having the core pinctrl code
> take that into account.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Florian, I'm really sorry for losing track of this patch set, it's
important stuff and I see why systems are dependent on something
like this.

Tony: can you look at this from a pinctrl-single point of view?
This is the intended consumer: pinctrl-single users that lose the
hardware state over suspend/resume.

How do you see this working with other pinctrl-single users?

Yours,
Linus Walleij

Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [171129 13:03]:
> On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> > Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose
> > their register contents when entering their lower power state. In such a
> > case, the pinctrl-single driver that is used will not be able to restore
> > the power states without telling the core about it and having
> > pinctrl_select_state() check for that.
> >
> > This patch adds a new optional boolean property that Device Tree can
> > define in order to obtain exactly that and having the core pinctrl code
> > take that into account.
> >
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Florian, I'm really sorry for losing track of this patch set, it's
> important stuff and I see why systems are dependent on something
> like this.
> 
> Tony: can you look at this from a pinctrl-single point of view?
> This is the intended consumer: pinctrl-single users that lose the
> hardware state over suspend/resume.
> 
> How do you see this working with other pinctrl-single users?

Hmm well typically a device driver that loses it's context just does
save and restore of the registers in runtime PM suspend/resume
as needed. In this case it would mean duplicating the state for
potentially for hundreds of registers.. So using the existing
state in the pinctrl subsystem totally makes sense for the pins.

Florian do you have other reasons why this should be done in the
pinctrl framework instead of the driver? Might be worth describing
the reasoning in the patch descriptions :)

So as long as the pinctrl framework state is used to restore the
state by the pinctrl driver instead of the pinctrl consumer drivers,
I don't have issues with this patchset. So probably just improving
the patch messages a bit should do it.

FYI, on omaps, the PRCM hardware saves and restores the pinctrl
state so this has not been so far an issue.

Regards,

Tony

Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Florian Fainelli]

On 11/29/2017 09:02 AM, Tony Lindgren wrote:
> * Linus Walleij <linus.walleij@linaro.org> [171129 13:03]:
>> On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>> Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose
>>> their register contents when entering their lower power state. In such a
>>> case, the pinctrl-single driver that is used will not be able to restore
>>> the power states without telling the core about it and having
>>> pinctrl_select_state() check for that.
>>>
>>> This patch adds a new optional boolean property that Device Tree can
>>> define in order to obtain exactly that and having the core pinctrl code
>>> take that into account.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>
>> Florian, I'm really sorry for losing track of this patch set, it's
>> important stuff and I see why systems are dependent on something
>> like this.
>>
>> Tony: can you look at this from a pinctrl-single point of view?
>> This is the intended consumer: pinctrl-single users that lose the
>> hardware state over suspend/resume.
>>
>> How do you see this working with other pinctrl-single users?
> 
> Hmm well typically a device driver that loses it's context just does
> save and restore of the registers in runtime PM suspend/resume
> as needed. In this case it would mean duplicating the state for
> potentially for hundreds of registers.. So using the existing
> state in the pinctrl subsystem totally makes sense for the pins.
> 
> Florian do you have other reasons why this should be done in the
> pinctrl framework instead of the driver? Might be worth describing
> the reasoning in the patch descriptions :)

The pinctrl provider driver that I am using is pinctrl-single, which has
proper suspend/resume callbacks but those are not causing any HW
programming to happen because of the (p->state == state) check, hence
this patch series.

> 
> So as long as the pinctrl framework state is used to restore the
> state by the pinctrl driver instead of the pinctrl consumer drivers,
> I don't have issues with this patchset. So probably just improving
> the patch messages a bit should do it.
> 
> FYI, on omaps, the PRCM hardware saves and restores the pinctrl
> state so this has not been so far an issue.
> 
> Regards,
> 
> Tony
> 
> 


-- 
Florian

Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Linus Walleij]

On Wed, Nov 29, 2017 at 6:37 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/29/2017 09:02 AM, Tony Lindgren wrote:

>> Hmm well typically a device driver that loses it's context just does
>> save and restore of the registers in runtime PM suspend/resume
>> as needed. In this case it would mean duplicating the state for
>> potentially for hundreds of registers.. So using the existing
>> state in the pinctrl subsystem totally makes sense for the pins.
>>
>> Florian do you have other reasons why this should be done in the
>> pinctrl framework instead of the driver? Might be worth describing
>> the reasoning in the patch descriptions :)
>
> The pinctrl provider driver that I am using is pinctrl-single, which has
> proper suspend/resume callbacks but those are not causing any HW
> programming to happen because of the (p->state == state) check, hence
> this patch series.

So we are talking about these callbacks, correct?

#ifdef CONFIG_PM
static int pinctrl_single_suspend(struct platform_device *pdev,
                                        pm_message_t state)
{
        struct pcs_device *pcs;

        pcs = platform_get_drvdata(pdev);
        if (!pcs)
                return -EINVAL;

        return pinctrl_force_sleep(pcs->pctl);
}

static int pinctrl_single_resume(struct platform_device *pdev)
{
        struct pcs_device *pcs;

        pcs = platform_get_drvdata(pdev);
        if (!pcs)
                return -EINVAL;

        return pinctrl_force_default(pcs->pctl);
}
#endif

Which falls through to this:

/**
 * pinctrl_force_sleep() - turn a given controller device into sleep state
 * @pctldev: pin controller device
 */
int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
{
        if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
                return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
        return 0;
}
EXPORT_SYMBOL_GPL(pinctrl_force_sleep);

/**
 * pinctrl_force_default() - turn a given controller device into default state
 * @pctldev: pin controller device
 */
int pinctrl_force_default(struct pinctrl_dev *pctldev)
{
        if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
                return pinctrl_select_state(pctldev->p, pctldev->hog_default);
        return 0;
}
EXPORT_SYMBOL_GPL(pinctrl_force_default);

So am I right in assuming it is actually the hogs that is your biggest
problem, and those are the states that get lost over suspend/resume
that are especially problematic?

I.e. you don't have any problem with any non-hogged pinctrl
handles, those are handled just fine in the suspend/resume
paths of the client drivers?

If this is the case, it changes the problem scope slightly.

It is fair that functions named *force* should actually enforce
programming a state.

So then I would suggest somethin else: break pinctrl_select_state()
into two:

pinctrl_select_state() that works just like before, checking if
(p->state == state) but which calls a static function
pinctrl_select_state_commit() that commits the change unconditonally.
Then alter pinctrl_force_sleep() and pinctrl_force_sleep() to call
that function.

This should solve your problem without having to alter the semantics
of pinctrl_select_state() for everyone.

If you want I can cook a patch to illustrate what I mean so you can
try it.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Florian Fainelli]

On 12/02/2017 04:48 AM, Linus Walleij wrote:
> On Wed, Nov 29, 2017 at 6:37 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 11/29/2017 09:02 AM, Tony Lindgren wrote:
> 
>>> Hmm well typically a device driver that loses it's context just does
>>> save and restore of the registers in runtime PM suspend/resume
>>> as needed. In this case it would mean duplicating the state for
>>> potentially for hundreds of registers.. So using the existing
>>> state in the pinctrl subsystem totally makes sense for the pins.
>>>
>>> Florian do you have other reasons why this should be done in the
>>> pinctrl framework instead of the driver? Might be worth describing
>>> the reasoning in the patch descriptions :)
>>
>> The pinctrl provider driver that I am using is pinctrl-single, which has
>> proper suspend/resume callbacks but those are not causing any HW
>> programming to happen because of the (p->state == state) check, hence
>> this patch series.
> 
> So we are talking about these callbacks, correct?
> 
> #ifdef CONFIG_PM
> static int pinctrl_single_suspend(struct platform_device *pdev,
>                                         pm_message_t state)
> {
>         struct pcs_device *pcs;
> 
>         pcs = platform_get_drvdata(pdev);
>         if (!pcs)
>                 return -EINVAL;
> 
>         return pinctrl_force_sleep(pcs->pctl);
> }
> 
> static int pinctrl_single_resume(struct platform_device *pdev)
> {
>         struct pcs_device *pcs;
> 
>         pcs = platform_get_drvdata(pdev);
>         if (!pcs)
>                 return -EINVAL;
> 
>         return pinctrl_force_default(pcs->pctl);
> }
> #endif
> 
> Which falls through to this:
> 
> /**
>  * pinctrl_force_sleep() - turn a given controller device into sleep state
>  * @pctldev: pin controller device
>  */
> int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
> {
>         if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
>                 return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
>         return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> 
> /**
>  * pinctrl_force_default() - turn a given controller device into default state
>  * @pctldev: pin controller device
>  */
> int pinctrl_force_default(struct pinctrl_dev *pctldev)
> {
>         if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
>                 return pinctrl_select_state(pctldev->p, pctldev->hog_default);
>         return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_default);
> 
> So am I right in assuming it is actually the hogs that is your biggest
> problem, and those are the states that get lost over suspend/resume
> that are especially problematic?
> 
> I.e. you don't have any problem with any non-hogged pinctrl
> handles, those are handled just fine in the suspend/resume
> paths of the client drivers?
> 
> If this is the case, it changes the problem scope slightly.
> 
> It is fair that functions named *force* should actually enforce
> programming a state.
> 
> So then I would suggest somethin else: break pinctrl_select_state()
> into two:
> 
> pinctrl_select_state() that works just like before, checking if
> (p->state == state) but which calls a static function
> pinctrl_select_state_commit() that commits the change unconditonally.
> Then alter pinctrl_force_sleep() and pinctrl_force_sleep() to call
> that function.
> 
> This should solve your problem without having to alter the semantics
> of pinctrl_select_state() for everyone.

This was exactly what I proposed initially here:

http://patchwork.ozlabs.org/patch/734326/

I really want to get this fixed, but I can't do that if we keep losing
the context of the discussion (pun intended) :).
-- 
Florian

Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Linus Walleij]

On Mon, Dec 11, 2017 at 12:38 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 12/02/2017 04:48 AM, Linus Walleij wrote:

>> This should solve your problem without having to alter the semantics
>> of pinctrl_select_state() for everyone.
>
> This was exactly what I proposed initially here:
>
> http://patchwork.ozlabs.org/patch/734326/
>
> I really want to get this fixed, but I can't do that if we keep losing
> the context of the discussion (pun intended) :).

Oh sorry man. I am clearly too stupid for this job...

In accordance with things needing to be intuitive, something named
*force_* should of course force the setting into the hardware.

The original patch didn't mention the fact that it was hogs
and hogs only that was causing the trouble and that is why I
got lost. (I guess.) I have been going about this as if it was
something generic that affect all states in all devices, and to
me hogs is just an abscure corner of pin controlling...

I applied the patchwork patch from above, and elaborated
a bit on that it pertains to hogs, let's see what
happens.

For the case where a driver (not hog) needs to handle
suspend/resume transitions, proper states can hopefully
be used.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power

[Florian Fainelli]

Le 12/19/17 à 23:24, Linus Walleij a écrit :
> On Mon, Dec 11, 2017 at 12:38 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 12/02/2017 04:48 AM, Linus Walleij wrote:
> 
>>> This should solve your problem without having to alter the semantics
>>> of pinctrl_select_state() for everyone.
>>
>> This was exactly what I proposed initially here:
>>
>> http://patchwork.ozlabs.org/patch/734326/
>>
>> I really want to get this fixed, but I can't do that if we keep losing
>> the context of the discussion (pun intended) :).
> 
> Oh sorry man. I am clearly too stupid for this job...

No need to slap yourself!

> 
> In accordance with things needing to be intuitive, something named
> *force_* should of course force the setting into the hardware.
> 
> The original patch didn't mention the fact that it was hogs
> and hogs only that was causing the trouble and that is why I
> got lost. (I guess.) I have been going about this as if it was
> something generic that affect all states in all devices, and to
> me hogs is just an abscure corner of pin controlling...
> 
> I applied the patchwork patch from above, and elaborated
> a bit on that it pertains to hogs, let's see what
> happens.
> 
> For the case where a driver (not hog) needs to handle
> suspend/resume transitions, proper states can hopefully
> be used.

Your commit message makes that clear now, thanks for applying the patch
and gott nytt år!
-- 
Florian

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

[Charles Keepax]

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

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@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

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@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

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

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

[Florian Fainelli]

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