Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1672056 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH fixes v3] pinctrl: Really force states during  suspend/resume Florian Fainelli <f.fainelli@gmail.com> - 2017-06-21 23:30 +0200
    Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume Linus Walleij <linus.walleij@linaro.org> - 2017-06-29 11:20 +0200
      Re: [PATCH fixes v3] pinctrl: Really force states during  suspend/resume Florian Fainelli <f.fainelli@gmail.com> - 2017-06-29 21:40 +0200
        Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume Linus Walleij <linus.walleij@linaro.org> - 2017-06-30 00:30 +0200

#1672056 — 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 <f.fainelli@gmail.com> 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

[toc] | [next] | [standalone]

#1677548 — 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 <f.fainelli@gmail.com> 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

[toc] | [prev] | [next] | [standalone]

#1678130

On 06/29/2017 02:17 AM, Linus Walleij wrote:
> Sorry for slowness...
> 
> On Wed, Jun 21, 2017 at 11:23 PM, Florian Fainelli <f.fainelli@gmail.com> 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

[toc] | [prev] | [next] | [standalone]

#1678246 — Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

On Thu, Jun 29, 2017 at 9:38 PM, Florian Fainelli <f.fainelli@gmail.com> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web