LinuxLists.cc - [PATCH fixes v3] pinctrl: Really force states during suspend/resume

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On 03/02/2017 12:54 AM, Andy Shevchenko wrote:
> On Wed, Mar 1, 2017 at 8:32 PM, Florian Fainelli <[email protected]> wrote:
>> In case a platform only defaults a "default" set of pins, but not a
>> "sleep" set of pins, and this particular platform suspends and resumes
>> in a way that the pin states are not preserved by the hardware, when we
>> resume, we would call pinctrl_single_resume() -> pinctrl_force_default()
>> -> pinctrl_select_state() and the first thing we do is check that the
>> pins state is the same as before, and do nothing.
>>
>> In order to fix this, decouple the actual state change from
>> pinctrl_select_state() and move it pinctrl_commit_state(), while keeping
>> the p->state == state check in pinctrl_select_state() not to change the
>> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
>> are updated to bypass the state check by calling pinctrl_commit_state().
>>
>> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device")
>> Signed-off-by: Florian Fainelli <[email protected]>
>
> This change is indeed beautiful and clean.
>
> FWIW:
> Reviewed-by: Andy Shevchenko <[email protected]>

Thanks Andy!

So actually, I have been thinking about this some more, and while this
definitively fixes my original problem with pinctrl-single, I just had
to make another driver (sdhci-brcmstb.c) pinctrl aware. And then again,
same thing, it has got just one "default" state, so when we call
pinctrl_select_state() during driver resume, nothing happens.

So, with that in mind, it seems to me like we may actually just want to
remove the p->state == state statement entirely, even though that's an
optimization....

... or, what we could do is, expose a version of pinctrl_force_default
that takes a struct pinctrl reference instead of a struct pinctrl_dev
reference and named differently of course.

Thoughts?

>
>
>> ---
>> Changes in v3:
>>
>> - move the state check to pinctrl_select_state
>>
>> Changes in v2:
>>
>> - rename __pinctrl_select_state to pinctrl_commit_state
>>
>> drivers/pinctrl/core.c | 24 +++++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index fb38e208f32d..735d8f7f9d71 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -992,19 +992,16 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
>> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>
>> /**
>> - * pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> + * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
>> * @p: the pinctrl handle for the device that requests configuration
>> * @state: the state handle to select/activate/program
>> */
>> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>> +static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>> {
>> struct pinctrl_setting *setting, *setting2;
>> struct pinctrl_state *old_state = p->state;
>> int ret;
>>
>> - if (p->state == state)
>> - return 0;
>> -
>> if (p->state) {
>> /*
>> * For each pinmux setting in the old state, forget SW's record
>> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>>
>> return ret;
>> }
>> +
>> +/**
>> + * pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> + * @p: the pinctrl handle for the device that requests configuration
>> + * @state: the state handle to select/activate/program
>> + */
>> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>> +{
>> + if (p->state == state)
>> + return 0;
>> +
>> + return pinctrl_commit_state(p, state);
>> +}
>> EXPORT_SYMBOL_GPL(pinctrl_select_state);
>>
>> static void devm_pinctrl_release(struct device *dev, void *res)
>> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map const *map)
>> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_sleep);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_default);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_default);
>> --
>> 2.9.3
>>
>
>
>

--
Florian

2017-03-14 10:16:41

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On Thu, Mar 2, 2017 at 11:19 PM, Florian Fainelli <[email protected]> wrote:

> So actually, I have been thinking about this some more, and while this
> definitively fixes my original problem with pinctrl-single, I just had
> to make another driver (sdhci-brcmstb.c) pinctrl aware. And then again,
> same thing, it has got just one "default" state, so when we call
> pinctrl_select_state() during driver resume, nothing happens.
>
> So, with that in mind, it seems to me like we may actually just want to
> remove the p->state == state statement entirely, even though that's an
> optimization....
>
> ... or, what we could do is, expose a version of pinctrl_force_default
> that takes a struct pinctrl reference instead of a struct pinctrl_dev
> reference and named differently of course.
>
> Thoughts?

The root problem that the patch is trying to solve is that the hardware
loose state when going to sleep, without the pin control core
being informed about this, correct?

And that is why the state is then "forced" with this patch, when
setting default state: the core think we are already in "default"
state, and thus the hack force it to be written down to the hardware
again.

But it is IMO just papering over the real bug: that the core does
not know that the state is lost.

What I think is the proper solution is to add a callback to switch
the state in the core.

The most obvious would be to use the API as many already do:
define "sleep" states in the core, and switch to these before
going to sleep. If CONFIG_PM is available simply by calling
pinctrl_pm_select_sleep_state() in the driver suspend() callback.

I think this is the most intuitive and clean solution.

Alternatively we would add a function to set the pinctrl handle to
an "unknown" state, so that when we resume, the pinctrl core at
least knows that we are not in "default" state anymore, so that
"default" is applied.

So then we would add:

/**
* pinctrl_set_unknown_state() - tell the pinctrl core that we lost the state
*/
void pinctrl_set_unknown_state(struct device *dev) {
struct dev_pin_info *pins = dev->pins;

if (!pins)
return 0;

pins->p->state = NULL;
}

I imagine this would give the right results on the suspend path.

Yours,
Linus Walleij

2017-03-15 02:18:27

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On 03/14/2017 03:16 AM, Linus Walleij wrote:
> On Thu, Mar 2, 2017 at 11:19 PM, Florian Fainelli <[email protected]> wrote:
>
>> So actually, I have been thinking about this some more, and while this
>> definitively fixes my original problem with pinctrl-single, I just had
>> to make another driver (sdhci-brcmstb.c) pinctrl aware. And then again,
>> same thing, it has got just one "default" state, so when we call
>> pinctrl_select_state() during driver resume, nothing happens.
>>
>> So, with that in mind, it seems to me like we may actually just want to
>> remove the p->state == state statement entirely, even though that's an
>> optimization....
>>
>> ... or, what we could do is, expose a version of pinctrl_force_default
>> that takes a struct pinctrl reference instead of a struct pinctrl_dev
>> reference and named differently of course.
>>
>> Thoughts?
>
> The root problem that the patch is trying to solve is that the hardware
> loose state when going to sleep, without the pin control core
> being informed about this, correct?

That is exactly what is happening.

>
> And that is why the state is then "forced" with this patch, when
> setting default state: the core think we are already in "default"
> state, and thus the hack force it to be written down to the hardware
> again.

Correct.

>
> But it is IMO just papering over the real bug: that the core does
> not know that the state is lost.

Yes, agree with that statement.

>
> What I think is the proper solution is to add a callback to switch
> the state in the core.
>
> The most obvious would be to use the API as many already do:
> define "sleep" states in the core, and switch to these before
> going to sleep. If CONFIG_PM is available simply by calling
> pinctrl_pm_select_sleep_state() in the driver suspend() callback.

Well, the difficulty for our platforms is that S2 does not make the HW
lose pin states, only S3 does and drivers should be agnostic of S2 vs. S3.

There is not really a "sleep" and "default" state defined for these
platforms just the "default" state. I initially even considered adding a
fake "sleep" state just to satisfy the state transition condition, but
that does not accurately represent the HW.

>
> I think this is the most intuitive and clean solution.
>
> Alternatively we would add a function to set the pinctrl handle to
> an "unknown" state, so that when we resume, the pinctrl core at
> least knows that we are not in "default" state anymore, so that
> "default" is applied.

And such a function would be called during driver suspend? Would not we
still end-up with the drivers having to know about the fact that there
is a) only one pin state defined, and b) these pins potentially lose
their states in some deep sleep mode?

>
> So then we would add:
>
> /**
> * pinctrl_set_unknown_state() - tell the pinctrl core that we lost the state
> */
> void pinctrl_set_unknown_state(struct device *dev) {
> struct dev_pin_info *pins = dev->pins;
>
> if (!pins)
> return 0;
>
> pins->p->state = NULL;
> }
>
> I imagine this would give the right results on the suspend path.

Humm, why not, but I am not clear how I would call that nor under which
conditions from both the pinctrl-single driver and another one that we
use (sdhci-brcmstb.c for instance, but it could be any pinctrl consumer
really)?

Thanks!

>
> Yours,
> Linus Walleij
>

--
Florian

2017-03-16 14:10:04

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On Wed, Mar 15, 2017 at 3:18 AM, Florian Fainelli <[email protected]> wrote:
> On 03/14/2017 03:16 AM, Linus Walleij wrote:

>> The most obvious would be to use the API as many already do:
>> define "sleep" states in the core, and switch to these before
>> going to sleep. If CONFIG_PM is available simply by calling
>> pinctrl_pm_select_sleep_state() in the driver suspend() callback.
>
> Well, the difficulty for our platforms is that S2 does not make the HW
> lose pin states, only S3 does and drivers should be agnostic of S2 vs. S3.
>
> There is not really a "sleep" and "default" state defined for these
> platforms just the "default" state. I initially even considered adding a
> fake "sleep" state just to satisfy the state transition condition, but
> that does not accurately represent the HW.

Do you mean that on the way up, on the resume path, you know
whether the setting was lost or not?

Or you don't know it anywhere?

It is not less elegant to uncessesarily switch to a sleep state
than to unnecessarily program the default state when you only
went into S2 in that case.

I guess then it is better to assume we will loose the state, or
push for more granular handling of S2/3 etc states in the
PM core (I guess these states comes from ACPI or similar).

>> Alternatively we would add a function to set the pinctrl handle to
>> an "unknown" state, so that when we resume, the pinctrl core at
>> least knows that we are not in "default" state anymore, so that
>> "default" is applied.
>
> And such a function would be called during driver suspend? Would not we
> still end-up with the drivers having to know about the fact that there
> is a) only one pin state defined, and b) these pins potentially lose
> their states in some deep sleep mode?

Again, the proposal to switch to default state twice just because
we do not know how deep sleep we went into isn't any more
elegant. Then it is better to just assume we lost the state at
all times.

Alternatively develop the PM core. Is it really impossible for
PM hooks to know which state it went into/came from?

Yours,
Linus Walleij

2017-06-21 21:23:15

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

(sorry for the lag)

On 03/16/2017 07:08 AM, Linus Walleij wrote:
> On Wed, Mar 15, 2017 at 3:18 AM, Florian Fainelli <[email protected]> wrote:
>> On 03/14/2017 03:16 AM, Linus Walleij wrote:
>
>>> The most obvious would be to use the API as many already do:
>>> define "sleep" states in the core, and switch to these before
>>> going to sleep. If CONFIG_PM is available simply by calling
>>> pinctrl_pm_select_sleep_state() in the driver suspend() callback.
>>
>> Well, the difficulty for our platforms is that S2 does not make the HW
>> lose pin states, only S3 does and drivers should be agnostic of S2 vs. S3.
>>
>> There is not really a "sleep" and "default" state defined for these
>> platforms just the "default" state. I initially even considered adding a
>> fake "sleep" state just to satisfy the state transition condition, but
>> that does not accurately represent the HW.
>
> Do you mean that on the way up, on the resume path, you know> whether the setting was lost or not?

In S3 we loose the hardware contents, and in S2 we do not. A platform
device driver has no way (currently) in its suspend/resume callback to
know which state was entered/exited.

>
> Or you don't know it anywhere?
>
> It is not less elegant to uncessesarily switch to a sleep state
> than to unnecessarily program the default state when you only
> went into S2 in that case.

Agreed, but defining a sleep state that does not exist just to force a
transition to the default state upon resumption is not really elegant.

>
> I guess then it is better to assume we will loose the state, or
> push for more granular handling of S2/3 etc states in the
> PM core (I guess these states comes from ACPI or similar).

I expected to see pm_message_t reflect which state we were entering into
(PM_SUSPEND_STANDBY vs. PM_SUSPEND_MEM), but that is not the case.

>
>>> Alternatively we would add a function to set the pinctrl handle to
>>> an "unknown" state, so that when we resume, the pinctrl core at
>>> least knows that we are not in "default" state anymore, so that
>>> "default" is applied.
>>
>> And such a function would be called during driver suspend? Would not we
>> still end-up with the drivers having to know about the fact that there
>> is a) only one pin state defined, and b) these pins potentially lose
>> their states in some deep sleep mode?
>
> Again, the proposal to switch to default state twice just because
> we do not know how deep sleep we went into isn't any more
> elegant. Then it is better to just assume we lost the state at
> all times.
>
> Alternatively develop the PM core. Is it really impossible for
> PM hooks to know which state it went into/came from?

I don't think I liked Rafael's suggestion of putting that kind of detail
into the platform_suspend_ops routine as he seems to suggest here:

https://www.spinics.net/lists/arm-kernel/msg587311.html

and here is my response:

https://www.spinics.net/lists/arm-kernel/msg589844.html
--
Florian

2017-06-29 09:17:59

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

Sorry for slowness...

On Wed, Jun 21, 2017 at 11:23 PM, Florian Fainelli <[email protected]> wrote:
> On 03/16/2017 07:08 AM, Linus Walleij wrote:

>> I guess then it is better to assume we will loose the state, or
>> push for more granular handling of S2/3 etc states in the
>> PM core (I guess these states comes from ACPI or similar).
>
> I expected to see pm_message_t reflect which state we were entering into
> (PM_SUSPEND_STANDBY vs. PM_SUSPEND_MEM), but that is not the case.

Can we fix it?

>> Alternatively develop the PM core. Is it really impossible for
>> PM hooks to know which state it went into/came from?
>
> I don't think I liked Rafael's suggestion of putting that kind of detail
> into the platform_suspend_ops routine as he seems to suggest here:
>
> https://www.spinics.net/lists/arm-kernel/msg587311.html

He is suggesting:

> The cleanest way would be to run that code from one of the platform
> suspend hooks that receive information on what sleep state is to be
> entered.

But what I suggest is more the inverse: that it receive information
on what state it is coming from, rather than which state it is
going to.

But I guess it would be logical that suspend() get to know what state
it is going to and resume() get to know which state it is coming from.

So Rafael seem to be aligned with that idea.

> and here is my response:
>
> https://www.spinics.net/lists/arm-kernel/msg589844.html

So if it is not desireable to have every driver know which exact
state it came from like S3 this or S2 that and on this laptop
we have S2' which is slightly different and such mess (that you
predict IIUC) what we really need to know is pretty simple:
did the hardware loose its state or not?

That is the information we want the PM core to provide to
the resume() callback, somehow. A simple bool is fine.

Any platform specifics or simplifications pertaining to certain
states and whether S5 or S7 looses the context should not
be the concern of a driver, what it wants to know is simply
whether its device has been powered off and lost its hardware
context.

Yours,
Linus Walleij

2017-06-29 19:38:21

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On 06/29/2017 02:17 AM, Linus Walleij wrote:
> Sorry for slowness...
>
> On Wed, Jun 21, 2017 at 11:23 PM, Florian Fainelli <[email protected]> wrote:
>> On 03/16/2017 07:08 AM, Linus Walleij wrote:
>
>>> I guess then it is better to assume we will loose the state, or
>>> push for more granular handling of S2/3 etc states in the
>>> PM core (I guess these states comes from ACPI or similar).
>>
>> I expected to see pm_message_t reflect which state we were entering into
>> (PM_SUSPEND_STANDBY vs. PM_SUSPEND_MEM), but that is not the case.
>
> Can we fix it?

Yes, I proposed this and got no feedback so far:

https://www.spinics.net/lists/arm-kernel/msg590135.html

>
>>> Alternatively develop the PM core. Is it really impossible for
>>> PM hooks to know which state it went into/came from?
>>
>> I don't think I liked Rafael's suggestion of putting that kind of detail
>> into the platform_suspend_ops routine as he seems to suggest here:
>>
>> https://www.spinics.net/lists/arm-kernel/msg587311.html
>
> He is suggesting:
>
>> The cleanest way would be to run that code from one of the platform
>> suspend hooks that receive information on what sleep state is to be
>> entered.
>
> But what I suggest is more the inverse: that it receive information
> on what state it is coming from, rather than which state it is
> going to.

The same information is available and it won't change from one suspend
cycle to resume, since in between these calls you are supposed to be...
suspended.

>
> But I guess it would be logical that suspend() get to know what state
> it is going to and resume() get to know which state it is coming from.>
> So Rafael seem to be aligned with that idea.
>
>> and here is my response:
>>
>> https://www.spinics.net/lists/arm-kernel/msg589844.html
>
> So if it is not desireable to have every driver know which exact
> state it came from like S3 this or S2 that and on this laptop
> we have S2' which is slightly different and such mess (that you
> predict IIUC) what we really need to know is pretty simple:
> did the hardware loose its state or not?

That information is inherently platform specific though, so on platforms
where pinctrl-single is used, you won't necessarily know whether the
state should be restored (conversely saved) so maybe that means we
should have the possibility for a platform to define a wrapper around
pinctrl-single whose purpose is to implement platform specific
suspend/r/resume functions and just that really? Is there such a driver
already that uses pinctrl-single more as a "library" than anything else?

>
> That is the information we want the PM core to provide to
> the resume() callback, somehow. A simple bool is fine.
>
> Any platform specifics or simplifications pertaining to certain
> states and whether S5 or S7 looses the context should not
> be the concern of a driver, what it wants to know is simply
> whether its device has been powered off and lost its hardware
> context.
>
> Yours,
> Linus Walleij
>

--
Florian

2017-06-29 22:25:30

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On Thu, Jun 29, 2017 at 9:38 PM, Florian Fainelli <[email protected]> wrote:
> On 06/29/2017 02:17 AM, Linus Walleij wrote:

>> So if it is not desireable to have every driver know which exact
>> state it came from like S3 this or S2 that and on this laptop
>> we have S2' which is slightly different and such mess (that you
>> predict IIUC) what we really need to know is pretty simple:
>> did the hardware loose its state or not?
>
> That information is inherently platform specific though, so on platforms
> where pinctrl-single is used, you won't necessarily know whether the
> state should be restored (conversely saved) so maybe that means we
> should have the possibility for a platform to define a wrapper around
> pinctrl-single whose purpose is to implement platform specific
> suspend/r/resume functions and just that really? Is there such a driver
> already that uses pinctrl-single more as a "library" than anything else?

pinctrl-single maye not be the easiest in cases like this, but Tony
et al have bolted in a few OMAP specifics to pinctrl-single in the
past so I don't see why this would be a problem.

commit 02e483f66deb6bd8df6af450726574614eb53be3
"pinctrl: single: Prepare for supporting SoC specific features"
etc.

Yours,
Linus Walleij

2018-02-19 17:26:36

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

Hi all,

On 2017-03-01 18:32, Florian Fainelli wrote:
> In case a platform only defaults a "default" set of pins, but not a
> "sleep" set of pins, and this particular platform suspends and
> resumes
> in a way that the pin states are not preserved by the hardware, when
> we
> resume, we would call pinctrl_single_resume() ->
> pinctrl_force_default()
> -> pinctrl_select_state() and the first thing we do is check that the
> pins state is the same as before, and do nothing.
>
> In order to fix this, decouple the actual state change from
> pinctrl_select_state() and move it pinctrl_commit_state(), while
> keeping
> the p->state == state check in pinctrl_select_state() not to change
> the
> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
> are updated to bypass the state check by calling
> pinctrl_commit_state().
>
> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states
> per device")
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> Changes in v3:
>
> - move the state check to pinctrl_select_state
>
> Changes in v2:
>
> - rename __pinctrl_select_state to pinctrl_commit_state
>
> drivers/pinctrl/core.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index fb38e208f32d..735d8f7f9d71 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -992,19 +992,16 @@ struct pinctrl_state
> *pinctrl_lookup_state(struct pinctrl *p,
> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>
> /**
> - * pinctrl_select_state() - select/activate/program a pinctrl state
> to HW
> + * pinctrl_commit_state() - select/activate/program a pinctrl state
> to HW
> * @p: the pinctrl handle for the device that requests configuration
> * @state: the state handle to select/activate/program
> */
> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state
> *state)
> +static int pinctrl_commit_state(struct pinctrl *p, struct
> pinctrl_state *state)
> {
> struct pinctrl_setting *setting, *setting2;
> struct pinctrl_state *old_state = p->state;
> int ret;
>
> - if (p->state == state)
> - return 0;
> -
> if (p->state) {
> /*
> * For each pinmux setting in the old state, forget SW's record
> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p,
> struct pinctrl_state *state)
>
> return ret;
> }
> +
> +/**
> + * pinctrl_select_state() - select/activate/program a pinctrl state
> to HW
> + * @p: the pinctrl handle for the device that requests configuration
> + * @state: the state handle to select/activate/program
> + */
> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state
> *state)
> +{
> + if (p->state == state)
> + return 0;
> +
> + return pinctrl_commit_state(p, state);
> +}
> EXPORT_SYMBOL_GPL(pinctrl_select_state);
>
> static void devm_pinctrl_release(struct device *dev, void *res)
> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map
> const *map)
> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_sleep);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_default);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_default);

I don't often go back over a year worth of LKML, but since this patch
recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an
anchor to report the following:

It turns out that this patch completely breaks resume on my
rk3399-based Chromebook. Most things are timing out, the box is
unusable. And since this is my everyday tool, I'm mildly grumpy. Please
don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes
me productive again...

More seriously, I have no idea what's wrong here. It could be a
SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you
could have.

Thanks,

M.
--
Who you jivin' with that Cosmik Debris?

2018-02-19 18:19:57

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On February 19, 2018 9:25:26 AM PST, Marc Zyngier <[email protected]> wrote:
>Hi all,
>
>On 2017-03-01 18:32, Florian Fainelli wrote:
>> In case a platform only defaults a "default" set of pins, but not a
>> "sleep" set of pins, and this particular platform suspends and
>> resumes
>> in a way that the pin states are not preserved by the hardware, when
>> we
>> resume, we would call pinctrl_single_resume() ->
>> pinctrl_force_default()
>> -> pinctrl_select_state() and the first thing we do is check that the
>> pins state is the same as before, and do nothing.
>>
>> In order to fix this, decouple the actual state change from
>> pinctrl_select_state() and move it pinctrl_commit_state(), while
>> keeping
>> the p->state == state check in pinctrl_select_state() not to change
>> the
>> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
>> are updated to bypass the state check by calling
>> pinctrl_commit_state().
>>
>> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states
>> per device")
>> Signed-off-by: Florian Fainelli <[email protected]>
>> ---
>> Changes in v3:
>>
>> - move the state check to pinctrl_select_state
>>
>> Changes in v2:
>>
>> - rename __pinctrl_select_state to pinctrl_commit_state
>>
>> drivers/pinctrl/core.c | 24 +++++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index fb38e208f32d..735d8f7f9d71 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -992,19 +992,16 @@ struct pinctrl_state
>> *pinctrl_lookup_state(struct pinctrl *p,
>> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>
>> /**
>> - * pinctrl_select_state() - select/activate/program a pinctrl state
>> to HW
>> + * pinctrl_commit_state() - select/activate/program a pinctrl state
>> to HW
>> * @p: the pinctrl handle for the device that requests configuration
>> * @state: the state handle to select/activate/program
>> */
>> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state
>> *state)
>> +static int pinctrl_commit_state(struct pinctrl *p, struct
>> pinctrl_state *state)
>> {
>> struct pinctrl_setting *setting, *setting2;
>> struct pinctrl_state *old_state = p->state;
>> int ret;
>>
>> - if (p->state == state)
>> - return 0;
>> -
>> if (p->state) {
>> /*
>> * For each pinmux setting in the old state, forget SW's record
>> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p,
>> struct pinctrl_state *state)
>>
>> return ret;
>> }
>> +
>> +/**
>> + * pinctrl_select_state() - select/activate/program a pinctrl state
>> to HW
>> + * @p: the pinctrl handle for the device that requests configuration
>> + * @state: the state handle to select/activate/program
>> + */
>> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state
>> *state)
>> +{
>> + if (p->state == state)
>> + return 0;
>> +
>> + return pinctrl_commit_state(p, state);
>> +}
>> EXPORT_SYMBOL_GPL(pinctrl_select_state);
>>
>> static void devm_pinctrl_release(struct device *dev, void *res)
>> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map
>> const *map)
>> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_sleep);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_default);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_default);

Hey Marc,

>
>I don't often go back over a year worth of LKML, but since this patch
>recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an
>anchor to report the following:
>
>It turns out that this patch completely breaks resume on my
>rk3399-based Chromebook. Most things are timing out, the box is
>unusable. And since this is my everyday tool, I'm mildly grumpy. Please
>
>don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes
>me productive again...
>
>More seriously, I have no idea what's wrong here. It could be a
>SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you
>could have.

Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage.

--
Florian

2018-02-19 18:58:26

by Heiko Stuebner

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli:
> On February 19, 2018 9:25:26 AM PST, Marc Zyngier <[email protected]> wrote:
> >Hi all,
> >
> >On 2017-03-01 18:32, Florian Fainelli wrote:
> >> In case a platform only defaults a "default" set of pins, but not a
> >> "sleep" set of pins, and this particular platform suspends and
> >> resumes
> >> in a way that the pin states are not preserved by the hardware, when
> >> we
> >> resume, we would call pinctrl_single_resume() ->
> >> pinctrl_force_default()
> >> -> pinctrl_select_state() and the first thing we do is check that the
> >> pins state is the same as before, and do nothing.
> >>
> >> In order to fix this, decouple the actual state change from
> >> pinctrl_select_state() and move it pinctrl_commit_state(), while
> >> keeping
> >> the p->state == state check in pinctrl_select_state() not to change
> >> the
> >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
> >> are updated to bypass the state check by calling
> >> pinctrl_commit_state().
> >>
> >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states
> >> per device")
> >> Signed-off-by: Florian Fainelli <[email protected]>
> >> ---
> >> Changes in v3:
> >>
> >> - move the state check to pinctrl_select_state
> >>
> >> Changes in v2:
> >>
> >> - rename __pinctrl_select_state to pinctrl_commit_state
> >>
> >> drivers/pinctrl/core.c | 24 +++++++++++++++++-------
> >> 1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> >> index fb38e208f32d..735d8f7f9d71 100644
> >> --- a/drivers/pinctrl/core.c
> >> +++ b/drivers/pinctrl/core.c
> >> @@ -992,19 +992,16 @@ struct pinctrl_state
> >> *pinctrl_lookup_state(struct pinctrl *p,
> >> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
> >>
> >> /**
> >> - * pinctrl_select_state() - select/activate/program a pinctrl state
> >> to HW
> >> + * pinctrl_commit_state() - select/activate/program a pinctrl state
> >> to HW
> >> * @p: the pinctrl handle for the device that requests configuration
> >> * @state: the state handle to select/activate/program
> >> */
> >> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state
> >> *state)
> >> +static int pinctrl_commit_state(struct pinctrl *p, struct
> >> pinctrl_state *state)
> >> {
> >> struct pinctrl_setting *setting, *setting2;
> >> struct pinctrl_state *old_state = p->state;
> >> int ret;
> >>
> >> - if (p->state == state)
> >> - return 0;
> >> -
> >> if (p->state) {
> >> /*
> >> * For each pinmux setting in the old state, forget SW's record
> >> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p,
> >> struct pinctrl_state *state)
> >>
> >> return ret;
> >> }
> >> +
> >> +/**
> >> + * pinctrl_select_state() - select/activate/program a pinctrl state
> >> to HW
> >> + * @p: the pinctrl handle for the device that requests configuration
> >> + * @state: the state handle to select/activate/program
> >> + */
> >> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state
> >> *state)
> >> +{
> >> + if (p->state == state)
> >> + return 0;
> >> +
> >> + return pinctrl_commit_state(p, state);
> >> +}
> >> EXPORT_SYMBOL_GPL(pinctrl_select_state);
> >>
> >> static void devm_pinctrl_release(struct device *dev, void *res)
> >> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map
> >> const *map)
> >> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_sleep);
> >> return 0;
> >> }
> >> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> >> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> >> 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 pinctrl_commit_state(pctldev->p, pctldev->hog_default);
> >> return 0;
> >> }
> >> EXPORT_SYMBOL_GPL(pinctrl_force_default);
>
> Hey Marc,
>
> >
> >I don't often go back over a year worth of LKML, but since this patch
> >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an
> >anchor to report the following:
> >
> >It turns out that this patch completely breaks resume on my
> >rk3399-based Chromebook. Most things are timing out, the box is
> >unusable. And since this is my everyday tool, I'm mildly grumpy. Please
> >
> >don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes
> >me productive again...
> >
> >More seriously, I have no idea what's wrong here. It could be a
> >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you
> >could have.
>
> Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage.

that should be
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts

I'm vacationing right now, so don't think I'll find time to dive into
Rockchip pinctrl this week. But I'd guess it could be somehow
related to the ATF touching pins during suspend/resume?

2018-02-19 19:14:04

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On Mon, 19 Feb 2018 18:03:27 +0000,
Florian Fainelli wrote:
>
> On February 19, 2018 9:25:26 AM PST, Marc Zyngier <[email protected]> wrote:
> >Hi all,
> >
> >On 2017-03-01 18:32, Florian Fainelli wrote:
> >> In case a platform only defaults a "default" set of pins, but not a
> >> "sleep" set of pins, and this particular platform suspends and
> >> resumes
> >> in a way that the pin states are not preserved by the hardware, when
> >> we
> >> resume, we would call pinctrl_single_resume() ->
> >> pinctrl_force_default()
> >> -> pinctrl_select_state() and the first thing we do is check that the
> >> pins state is the same as before, and do nothing.
> >>
> >> In order to fix this, decouple the actual state change from
> >> pinctrl_select_state() and move it pinctrl_commit_state(), while
> >> keeping
> >> the p->state == state check in pinctrl_select_state() not to change
> >> the
> >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
> >> are updated to bypass the state check by calling
> >> pinctrl_commit_state().
> >>
> >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states
> >> per device")
> >> Signed-off-by: Florian Fainelli <[email protected]>

[back to using my ARM address]

Hey Florian,

> Hey Marc,
>
> >
> >I don't often go back over a year worth of LKML, but since this patch
> >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an
> >anchor to report the following:
> >
> >It turns out that this patch completely breaks resume on my
> >rk3399-based Chromebook. Most things are timing out, the box is
> >unusable. And since this is my everyday tool, I'm mildly
> >grumpy. Please don't break my toys! ;-) Reverting this patch on top
> >of 4.16-rc2 makes me productive again...
> >
> >More seriously, I have no idea what's wrong here. It could be a
> >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you
> >could have.
>
> Can you indicate which DTS file is used for your Chromebook model?

Sure. That's arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts, with a
couple of fixes on top (some clocks and big-little idiosyncrasies).

> Sorry about the breakage.

No worries.

M.

--
Jazz is not dead, it just smell funny.

2018-02-19 19:24:44

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On Mon, 19 Feb 2018 18:57:23 +0000,
Heiko Stuebner wrote:
>
> Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli:
> > On February 19, 2018 9:25:26 AM PST, Marc Zyngier <[email protected]> wrote:
> > >Hi all,
> > >
> > >On 2017-03-01 18:32, Florian Fainelli wrote:
> > >> In case a platform only defaults a "default" set of pins, but not a
> > >> "sleep" set of pins, and this particular platform suspends and
> > >> resumes
> > >> in a way that the pin states are not preserved by the hardware, when
> > >> we
> > >> resume, we would call pinctrl_single_resume() ->
> > >> pinctrl_force_default()
> > >> -> pinctrl_select_state() and the first thing we do is check that the
> > >> pins state is the same as before, and do nothing.
> > >>
> > >> In order to fix this, decouple the actual state change from
> > >> pinctrl_select_state() and move it pinctrl_commit_state(), while
> > >> keeping
> > >> the p->state == state check in pinctrl_select_state() not to change
> > >> the
> > >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
> > >> are updated to bypass the state check by calling
> > >> pinctrl_commit_state().
> > >>
> > >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states
> > >> per device")
> > >> Signed-off-by: Florian Fainelli <[email protected]>
> > >> ---
> > >> Changes in v3:
> > >>
> > >> - move the state check to pinctrl_select_state
> > >>
> > >> Changes in v2:
> > >>
> > >> - rename __pinctrl_select_state to pinctrl_commit_state
> > >>
> > >> drivers/pinctrl/core.c | 24 +++++++++++++++++-------
> > >> 1 file changed, 17 insertions(+), 7 deletions(-)
> > >>

Hey Heiko,

> > Hey Marc,
> >
> > >
> > >I don't often go back over a year worth of LKML, but since this patch
> > >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an
> > >anchor to report the following:
> > >
> > >It turns out that this patch completely breaks resume on my
> > >rk3399-based Chromebook. Most things are timing out, the box is
> > >unusable. And since this is my everyday tool, I'm mildly grumpy. Please
> > >
> > >don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes
> > >me productive again...
> > >
> > >More seriously, I have no idea what's wrong here. It could be a
> > >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you
> > >could have.
> >
> > Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage.
>
> that should be
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>
> I'm vacationing right now, so don't think I'll find time to dive into
> Rockchip pinctrl this week. But I'd guess it could be somehow
> related to the ATF touching pins during suspend/resume?

That'd be really unfortunate. I would have assumed that ATF would
leave things as they were instead of re-configuring them to whatever
default.

The most annoying thing is that if that's indeed the case, we need to
find a solution that will cope with the current state of the
firmware. I guess that'd mean eagerly saving/restoring the pin state
across suspend/resume, irrespective of what firmware could do?

Enjoy your holiday!

M.

--
Jazz is not dead, it just smell funny.

2018-02-22 15:31:44

[permalink] [raw]

Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On Mon, Feb 19, 2018 at 8:23 PM, Marc Zyngier <[email protected]> wrote:
>> Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli:

>> > Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage.
>>
>> that should be
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>
>> I'm vacationing right now, so don't think I'll find time to dive into
>> Rockchip pinctrl this week. But I'd guess it could be somehow
>> related to the ATF touching pins during suspend/resume?
>
> That'd be really unfortunate. I would have assumed that ATF would
> leave things as they were instead of re-configuring them to whatever
> default.
>
> The most annoying thing is that if that's indeed the case, we need to
> find a solution that will cope with the current state of the
> firmware. I guess that'd mean eagerly saving/restoring the pin state
> across suspend/resume, irrespective of what firmware could do?

What is ATF? Asus Touch Firmware?

Does it in effect mean that when the Rockchip pinctrl driver
says pinctrl_force_sleep() and pinctrl_force_default()
it expects those to be a noop?

Then the real patch to apply is something deleting the
pinctrl_force* calls from the pinctrl-rockchip driver,
is it not?

Yours,
Linus Walleij

2018-02-22 18:12:28