Drivers taking different actions depending on sleep state

Drivers taking different actions depending on sleep state

[Mason]

Hello,

I read the "Sleep States" documentation:
https://www.kernel.org/doc/Documentation/power/states.txt

It mentions /sys/power/mem_sleep but I don't have that in 4.9
# ll /sys/power/
-rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
-rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
-rw-r--r--    1 root     root          4096 Jan  1 00:31 state
-rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count

# cat /sys/power/state 
freeze mem

Currently my platform's "mem" is a true suspend-to-RAM trigger,
where drivers are supposed to save their state (register values
will be lost), then Linux hands control over to firmware which
enables RAM self-refresh and powers the chip down. When the system
resumes, drivers restore their state from their copy in memory.

One driver is responsible for loading/unloading microcode running
on the DSPs. This operation is required only when powering down
the chip, but it should be avoided for "low-latency" sleeps.

The problem is that, if I understand correctly, drivers have no way
of knowing which sleep state is being entered/exited?

How can I have the microcode driver take different decisions
based on the sleep state?

Regards.

Drivers taking different actions depending on sleep state

[Mason]

Hello,

I read the "Sleep States" documentation:
https://www.kernel.org/doc/Documentation/power/states.txt

It mentions /sys/power/mem_sleep but I don't have that in 4.9
# ll /sys/power/
-rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
-rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
-rw-r--r--    1 root     root          4096 Jan  1 00:31 state
-rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count

# cat /sys/power/state 
freeze mem

Currently my platform's "mem" is a true suspend-to-RAM trigger,
where drivers are supposed to save their state (register values
will be lost), then Linux hands control over to firmware which
enables RAM self-refresh and powers the chip down. When the system
resumes, drivers restore their state from their copy in memory.

One driver is responsible for loading/unloading microcode running
on the DSPs. This operation is required only when powering down
the chip, but it should be avoided for "low-latency" sleeps.

The problem is that, if I understand correctly, drivers have no way
of knowing which sleep state is being entered/exited?

How can I have the microcode driver take different decisions
based on the sleep state?

Regards.

Re: Drivers taking different actions depending on sleep state

[Mason]

On 09/06/2017 17:20, Mason wrote:

> Currently my platform's "mem" is a true suspend-to-RAM trigger,
> where drivers are supposed to save their state (register values
> will be lost), then Linux hands control over to firmware which
> enables RAM self-refresh and powers the chip down. When the system
> resumes, drivers restore their state from their copy in memory.
> 
> One driver is responsible for loading/unloading microcode running
> on the DSPs. This operation is required only when powering down
> the chip, but it should be avoided for "low-latency" sleeps.
> 
> The problem is that, if I understand correctly, drivers have no way
> of knowing which sleep state is being entered/exited?
> 
> How can I have the microcode driver take different decisions
> based on the sleep state?

FWIW, here's a transcript of a parallel discussion on #armlinux

Mason385	The kernel supports several "levels" of sleeps (S0, S1, S3, S4) ... is it possible for drivers to differentiate which level is being entered/exited ?
kos_tom		Mason385: ask abelloni, he knows this topic very well
Mason385	abelloni: I'd be happy to tap that knowledge of yours
Mason385	I think khilman also knows a thing or three
abelloni	it is not currently possible
Mason385	abelloni: one of the drivers is responsible for loading/unloading microcode to the DSP, and this is a very long operation (on the order of 1-2 seconds) and the author wants to be able to avoid this unload/reload for "freeze"
Mason385	the current "solution" has been to export the requested state in a global variable exported to all kernel modules (bleh)
Mason385	IIUC, there is no better way then?
abelloni	no, that is what we do on at91
Mason385	abelloni: I'm confused because there's already an argument passed to drivers to indicate the sleep state, only it's the same arg for every state. Did someone determine it is unnecessary for drivers to have that information? But then why have the parameter in the first place?
abelloni	I think up until recently, the stance was that it was unnecessary for the drivers to have that info
abelloni	I would like to change that too but currently, you can't have it
khilman		Mason385: abelloni is right (unfortunately)
Mason385	khilman: so you would also recommend exporting a global variable then?
khilman		I cannot confirm or deny such a recommendation.  
khilman		(but there is no other way)
Mason385	Don't worry, this code has negative chance to ever hit upstream
khilman		curious though: is the reason to avoid the unload/reload just for optimization of suspend/resume time?  or is it because the DSP actually loses context?
Mason385	on suspend to RAM, the chip is powered down and loses all context
khilman		so the compromise solution I've used in this case is to make the driver a bit smarter.
khilman		iow, the driver always saves context, but then on resume, checks to see if restore is actually needed before doing the full restore (which is usually the time consuming part)
khilman		often that can be done by checking some register that has a known power-on reset value (that's different from the driver programmed state)
abelloni	khilman: I think it would still be worth exporting the target state to the drivers
Mason385	khilman: I *think* even the suspend side of the problem is time-consuming
javier__	Mason385: silly question, but does it necessarily lose state because the machine is suspended to RAM and the DSP needs to be reset on resume or is because the DSP loses power?
abelloni	It may be difficult for drivers to know whether the IP has lost state
abelloni	Also, we only have one platform loosing state and now, all the previous SoC are taking the hit
khilman		abelloni: sure, but the goal is still that the drivers try to be smart.  Having the global variable is an optimization.
Mason385	javier__: there's some authentication required when S2R is involved (from the firmware)
javier__	Mason385: ah, Ok. I just asked because if it was the latter, the regulator subsystem has infrastructure to keep the regulators on during S2R
Mason385	javier__: OK so there's two issues. We are required to re-authenticate microcode when resuming from S2R (because someone "may" have tampered with the contents) and on suspend, power is cut to the DSPs and they lose context
Mason385	javier__: were you suggesting to keep power to the DSPs during S2R (I don't think that is possible in the chip)
javier__	Mason385: I wasn't suggesting since I don't know your HW and use case
Mason385	I meant if it was possible, of course 
javier__	Mason385: yes, I was just mentioning that the regulator subsys had a regulator-on-in-suspend property since we had a similar issue with a chip and solved that way

Drivers taking different actions depending on sleep state

[Mason]

On 09/06/2017 17:20, Mason wrote:

> Currently my platform's "mem" is a true suspend-to-RAM trigger,
> where drivers are supposed to save their state (register values
> will be lost), then Linux hands control over to firmware which
> enables RAM self-refresh and powers the chip down. When the system
> resumes, drivers restore their state from their copy in memory.
> 
> One driver is responsible for loading/unloading microcode running
> on the DSPs. This operation is required only when powering down
> the chip, but it should be avoided for "low-latency" sleeps.
> 
> The problem is that, if I understand correctly, drivers have no way
> of knowing which sleep state is being entered/exited?
> 
> How can I have the microcode driver take different decisions
> based on the sleep state?

FWIW, here's a transcript of a parallel discussion on #armlinux

Mason385	The kernel supports several "levels" of sleeps (S0, S1, S3, S4) ... is it possible for drivers to differentiate which level is being entered/exited ?
kos_tom		Mason385: ask abelloni, he knows this topic very well
Mason385	abelloni: I'd be happy to tap that knowledge of yours
Mason385	I think khilman also knows a thing or three
abelloni	it is not currently possible
Mason385	abelloni: one of the drivers is responsible for loading/unloading microcode to the DSP, and this is a very long operation (on the order of 1-2 seconds) and the author wants to be able to avoid this unload/reload for "freeze"
Mason385	the current "solution" has been to export the requested state in a global variable exported to all kernel modules (bleh)
Mason385	IIUC, there is no better way then?
abelloni	no, that is what we do on at91
Mason385	abelloni: I'm confused because there's already an argument passed to drivers to indicate the sleep state, only it's the same arg for every state. Did someone determine it is unnecessary for drivers to have that information? But then why have the parameter in the first place?
abelloni	I think up until recently, the stance was that it was unnecessary for the drivers to have that info
abelloni	I would like to change that too but currently, you can't have it
khilman		Mason385: abelloni is right (unfortunately)
Mason385	khilman: so you would also recommend exporting a global variable then?
khilman		I cannot confirm or deny such a recommendation.  
khilman		(but there is no other way)
Mason385	Don't worry, this code has negative chance to ever hit upstream
khilman		curious though: is the reason to avoid the unload/reload just for optimization of suspend/resume time?  or is it because the DSP actually loses context?
Mason385	on suspend to RAM, the chip is powered down and loses all context
khilman		so the compromise solution I've used in this case is to make the driver a bit smarter.
khilman		iow, the driver always saves context, but then on resume, checks to see if restore is actually needed before doing the full restore (which is usually the time consuming part)
khilman		often that can be done by checking some register that has a known power-on reset value (that's different from the driver programmed state)
abelloni	khilman: I think it would still be worth exporting the target state to the drivers
Mason385	khilman: I *think* even the suspend side of the problem is time-consuming
javier__	Mason385: silly question, but does it necessarily lose state because the machine is suspended to RAM and the DSP needs to be reset on resume or is because the DSP loses power?
abelloni	It may be difficult for drivers to know whether the IP has lost state
abelloni	Also, we only have one platform loosing state and now, all the previous SoC are taking the hit
khilman		abelloni: sure, but the goal is still that the drivers try to be smart.  Having the global variable is an optimization.
Mason385	javier__: there's some authentication required when S2R is involved (from the firmware)
javier__	Mason385: ah, Ok. I just asked because if it was the latter, the regulator subsystem has infrastructure to keep the regulators on during S2R
Mason385	javier__: OK so there's two issues. We are required to re-authenticate microcode when resuming from S2R (because someone "may" have tampered with the contents) and on suspend, power is cut to the DSPs and they lose context
Mason385	javier__: were you suggesting to keep power to the DSPs during S2R (I don't think that is possible in the chip)
javier__	Mason385: I wasn't suggesting since I don't know your HW and use case
Mason385	I meant if it was possible, of course 
javier__	Mason385: yes, I was just mentioning that the regulator subsys had a regulator-on-in-suspend property since we had a similar issue with a chip and solved that way

Re: Drivers taking different actions depending on sleep state

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]

On Fri 2017-06-09 18:27:45, Mason wrote:
> On 09/06/2017 17:20, Mason wrote:
> 
> > Currently my platform's "mem" is a true suspend-to-RAM trigger,
> > where drivers are supposed to save their state (register values
> > will be lost), then Linux hands control over to firmware which
> > enables RAM self-refresh and powers the chip down. When the system
> > resumes, drivers restore their state from their copy in memory.
> > 
> > One driver is responsible for loading/unloading microcode running
> > on the DSPs. This operation is required only when powering down
> > the chip, but it should be avoided for "low-latency" sleeps.
> > 
> > The problem is that, if I understand correctly, drivers have no way
> > of knowing which sleep state is being entered/exited?
> > 
> > How can I have the microcode driver take different decisions
> > based on the sleep state?
> 
> FWIW, here's a transcript of a parallel discussion on #armlinux


Well... question "does my chip lose state during standby/mem on _this_
machine" is more complex then "is it standby or mem", right?

You should really ask the regulator framework, not core code.

> Mason385	javier__: there's some authentication required when S2R is involved (from the firmware)
> javier__	Mason385: ah, Ok. I just asked because if it was the latter, the regulator subsystem has infrastructure to keep the regulators on during S2R
> Mason385	javier__: OK so there's two issues. We are required to
> re-authenticate microcode when resuming from S2R (because someone
> "may" have tampered with the contents) and on suspend, power is cut
> to the DSPs and they lose context

I'm not sure what you are developing. Someone also "may" have modified
the microcode while you were running. Someone also "may" have modified
the kernel in RAM. Not sure what you are developing, but protecting
against attacker with direct hardware access is impossible and not
welcome.

Best regards,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Drivers taking different actions depending on sleep state

[Pavel Machek]

On Fri 2017-06-09 18:27:45, Mason wrote:
> On 09/06/2017 17:20, Mason wrote:
> 
> > Currently my platform's "mem" is a true suspend-to-RAM trigger,
> > where drivers are supposed to save their state (register values
> > will be lost), then Linux hands control over to firmware which
> > enables RAM self-refresh and powers the chip down. When the system
> > resumes, drivers restore their state from their copy in memory.
> > 
> > One driver is responsible for loading/unloading microcode running
> > on the DSPs. This operation is required only when powering down
> > the chip, but it should be avoided for "low-latency" sleeps.
> > 
> > The problem is that, if I understand correctly, drivers have no way
> > of knowing which sleep state is being entered/exited?
> > 
> > How can I have the microcode driver take different decisions
> > based on the sleep state?
> 
> FWIW, here's a transcript of a parallel discussion on #armlinux


Well... question "does my chip lose state during standby/mem on _this_
machine" is more complex then "is it standby or mem", right?

You should really ask the regulator framework, not core code.

> Mason385	javier__: there's some authentication required when S2R is involved (from the firmware)
> javier__	Mason385: ah, Ok. I just asked because if it was the latter, the regulator subsystem has infrastructure to keep the regulators on during S2R
> Mason385	javier__: OK so there's two issues. We are required to
> re-authenticate microcode when resuming from S2R (because someone
> "may" have tampered with the contents) and on suspend, power is cut
> to the DSPs and they lose context

I'm not sure what you are developing. Someone also "may" have modified
the microcode while you were running. Someone also "may" have modified
the kernel in RAM. Not sure what you are developing, but protecting
against attacker with direct hardware access is impossible and not
welcome.

Best regards,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170609/642ebb76/attachment-0001.sig>

Re: Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/09/2017 08:20 AM, Mason wrote:
> Hello,
> 
> I read the "Sleep States" documentation:
> https://www.kernel.org/doc/Documentation/power/states.txt
> 
> It mentions /sys/power/mem_sleep but I don't have that in 4.9
> # ll /sys/power/
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
> 
> # cat /sys/power/state 
> freeze mem
> 
> Currently my platform's "mem" is a true suspend-to-RAM trigger,
> where drivers are supposed to save their state (register values
> will be lost), then Linux hands control over to firmware which
> enables RAM self-refresh and powers the chip down. When the system
> resumes, drivers restore their state from their copy in memory.
> 
> One driver is responsible for loading/unloading microcode running
> on the DSPs. This operation is required only when powering down
> the chip, but it should be avoided for "low-latency" sleeps.

Yes, makes sense, the STB SoCs I work with have similar requirements,
and we could benefit from being able to skip/bypass re-initialization
while entering/leaving S2 for instance.

> 
> The problem is that, if I understand correctly, drivers have no way
> of knowing which sleep state is being entered/exited?
> 
> How can I have the microcode driver take different decisions
> based on the sleep state?

Ideally, pm_message_t could have reflected the system suspend/resume
state you are entering/leaving, but that does not appear to be the case
(unlike say, PCI).

I am not sure why that's not the case, but since most platform drivers
use the "SIMPLE_DEV_PM_OPS" if we were to actually make pm_message_t
reflect the system sleep state (PM_SUSPEND_STANDBY, PM_SUSPEND_MEM etc.)
-- 
Florian

Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/09/2017 08:20 AM, Mason wrote:
> Hello,
> 
> I read the "Sleep States" documentation:
> https://www.kernel.org/doc/Documentation/power/states.txt
> 
> It mentions /sys/power/mem_sleep but I don't have that in 4.9
> # ll /sys/power/
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
> 
> # cat /sys/power/state 
> freeze mem
> 
> Currently my platform's "mem" is a true suspend-to-RAM trigger,
> where drivers are supposed to save their state (register values
> will be lost), then Linux hands control over to firmware which
> enables RAM self-refresh and powers the chip down. When the system
> resumes, drivers restore their state from their copy in memory.
> 
> One driver is responsible for loading/unloading microcode running
> on the DSPs. This operation is required only when powering down
> the chip, but it should be avoided for "low-latency" sleeps.

Yes, makes sense, the STB SoCs I work with have similar requirements,
and we could benefit from being able to skip/bypass re-initialization
while entering/leaving S2 for instance.

> 
> The problem is that, if I understand correctly, drivers have no way
> of knowing which sleep state is being entered/exited?
> 
> How can I have the microcode driver take different decisions
> based on the sleep state?

Ideally, pm_message_t could have reflected the system suspend/resume
state you are entering/leaving, but that does not appear to be the case
(unlike say, PCI).

I am not sure why that's not the case, but since most platform drivers
use the "SIMPLE_DEV_PM_OPS" if we were to actually make pm_message_t
reflect the system sleep state (PM_SUSPEND_STANDBY, PM_SUSPEND_MEM etc.)
-- 
Florian

Re: Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

Hi,

On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
> Hello,
>
> I read the "Sleep States" documentation:
> https://www.kernel.org/doc/Documentation/power/states.txt
>
> It mentions /sys/power/mem_sleep but I don't have that in 4.9
> # ll /sys/power/
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>
> # cat /sys/power/state
> freeze mem
>
> Currently my platform's "mem" is a true suspend-to-RAM trigger,
> where drivers are supposed to save their state (register values
> will be lost), then Linux hands control over to firmware which
> enables RAM self-refresh and powers the chip down. When the system
> resumes, drivers restore their state from their copy in memory.
>
> One driver is responsible for loading/unloading microcode running
> on the DSPs. This operation is required only when powering down
> the chip, but it should be avoided for "low-latency" sleeps.
>
> The problem is that, if I understand correctly, drivers have no way
> of knowing which sleep state is being entered/exited?
>
> How can I have the microcode driver take different decisions
> based on the sleep state?

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.

Alternatively, those hooks can set/clear flags that can be accessed by
drivers, but that of course may your drivers depend on the platform
(still, in the microcode case the driver seems to be
platform-dependent anyway).

Thanks,
Rafael

Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

Hi,

On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
> Hello,
>
> I read the "Sleep States" documentation:
> https://www.kernel.org/doc/Documentation/power/states.txt
>
> It mentions /sys/power/mem_sleep but I don't have that in 4.9
> # ll /sys/power/
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>
> # cat /sys/power/state
> freeze mem
>
> Currently my platform's "mem" is a true suspend-to-RAM trigger,
> where drivers are supposed to save their state (register values
> will be lost), then Linux hands control over to firmware which
> enables RAM self-refresh and powers the chip down. When the system
> resumes, drivers restore their state from their copy in memory.
>
> One driver is responsible for loading/unloading microcode running
> on the DSPs. This operation is required only when powering down
> the chip, but it should be avoided for "low-latency" sleeps.
>
> The problem is that, if I understand correctly, drivers have no way
> of knowing which sleep state is being entered/exited?
>
> How can I have the microcode driver take different decisions
> based on the sleep state?

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.

Alternatively, those hooks can set/clear flags that can be accessed by
drivers, but that of course may your drivers depend on the platform
(still, in the microcode case the driver seems to be
platform-dependent anyway).

Thanks,
Rafael

Re: Drivers taking different actions depending on sleep state

[Mason]

On 09/06/2017 23:30, Pavel Machek wrote:
> On Fri 2017-06-09 18:27:45, Mason wrote:
>> On 09/06/2017 17:20, Mason wrote:
>>
>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>> where drivers are supposed to save their state (register values
>>> will be lost), then Linux hands control over to firmware which
>>> enables RAM self-refresh and powers the chip down. When the system
>>> resumes, drivers restore their state from their copy in memory.
>>>
>>> One driver is responsible for loading/unloading microcode running
>>> on the DSPs. This operation is required only when powering down
>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>
>>> The problem is that, if I understand correctly, drivers have no way
>>> of knowing which sleep state is being entered/exited?
>>>
>>> How can I have the microcode driver take different decisions
>>> based on the sleep state?
> 
> Well... question "does my chip lose state during standby/mem on _this_
> machine" is more complex then "is it standby or mem", right?

I think it's binary...
If power to the DSPs is cut, then they lose state.
If the DSPs remain powered, then they maintain state.

"mem" powers the entire chip down, including the DSPs
(by implementation's choice) but we are investigating
a lower-latency sleep state that wouldn't cut power.

> You should really ask the regulator framework, not core code.

The issue is that power cutting is not handled in Linux,
it is done by firmware. So I'm not sure what there is
to ask to the regulator framework?

>> Mason385	javier__: there's some authentication required when S2R is involved (from the firmware)
>> javier__	Mason385: ah, Ok. I just asked because if it was the latter, the regulator subsystem has infrastructure to keep the regulators on during S2R
>> Mason385	javier__: OK so there's two issues. We are required to
>> re-authenticate microcode when resuming from S2R (because someone
>> "may" have tampered with the contents) and on suspend, power is cut
>> to the DSPs and they lose context
> 
> I'm not sure what you are developing. Someone also "may" have modified
> the microcode while you were running. Someone also "may" have modified
> the kernel in RAM. Not sure what you are developing, but protecting
> against attacker with direct hardware access is impossible and not
> welcome.

There is no point in discussing the technical relevance
of these requirements, because they are *mandatory* for
certification. No certification, no customer.

So the feature must be implemented, whether it increases
"security" or not. FTR, what is being bitterly defended
is Hollywood's pixels.

Regards.

Drivers taking different actions depending on sleep state

[Mason]

On 09/06/2017 23:30, Pavel Machek wrote:
> On Fri 2017-06-09 18:27:45, Mason wrote:
>> On 09/06/2017 17:20, Mason wrote:
>>
>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>> where drivers are supposed to save their state (register values
>>> will be lost), then Linux hands control over to firmware which
>>> enables RAM self-refresh and powers the chip down. When the system
>>> resumes, drivers restore their state from their copy in memory.
>>>
>>> One driver is responsible for loading/unloading microcode running
>>> on the DSPs. This operation is required only when powering down
>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>
>>> The problem is that, if I understand correctly, drivers have no way
>>> of knowing which sleep state is being entered/exited?
>>>
>>> How can I have the microcode driver take different decisions
>>> based on the sleep state?
> 
> Well... question "does my chip lose state during standby/mem on _this_
> machine" is more complex then "is it standby or mem", right?

I think it's binary...
If power to the DSPs is cut, then they lose state.
If the DSPs remain powered, then they maintain state.

"mem" powers the entire chip down, including the DSPs
(by implementation's choice) but we are investigating
a lower-latency sleep state that wouldn't cut power.

> You should really ask the regulator framework, not core code.

The issue is that power cutting is not handled in Linux,
it is done by firmware. So I'm not sure what there is
to ask to the regulator framework?

>> Mason385	javier__: there's some authentication required when S2R is involved (from the firmware)
>> javier__	Mason385: ah, Ok. I just asked because if it was the latter, the regulator subsystem has infrastructure to keep the regulators on during S2R
>> Mason385	javier__: OK so there's two issues. We are required to
>> re-authenticate microcode when resuming from S2R (because someone
>> "may" have tampered with the contents) and on suspend, power is cut
>> to the DSPs and they lose context
> 
> I'm not sure what you are developing. Someone also "may" have modified
> the microcode while you were running. Someone also "may" have modified
> the kernel in RAM. Not sure what you are developing, but protecting
> against attacker with direct hardware access is impossible and not
> welcome.

There is no point in discussing the technical relevance
of these requirements, because they are *mandatory* for
certification. No certification, no customer.

So the feature must be implemented, whether it increases
"security" or not. FTR, what is being bitterly defended
is Hollywood's pixels.

Regards.

Re: Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
> Hi,
> 
> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>> Hello,
>>
>> I read the "Sleep States" documentation:
>> https://www.kernel.org/doc/Documentation/power/states.txt
>>
>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>> # ll /sys/power/
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>
>> # cat /sys/power/state
>> freeze mem
>>
>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>> where drivers are supposed to save their state (register values
>> will be lost), then Linux hands control over to firmware which
>> enables RAM self-refresh and powers the chip down. When the system
>> resumes, drivers restore their state from their copy in memory.
>>
>> One driver is responsible for loading/unloading microcode running
>> on the DSPs. This operation is required only when powering down
>> the chip, but it should be avoided for "low-latency" sleeps.
>>
>> The problem is that, if I understand correctly, drivers have no way
>> of knowing which sleep state is being entered/exited?
>>
>> How can I have the microcode driver take different decisions
>> based on the sleep state?
> 
> 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.

I am not sure this would be cleaner, because that would create a tighter
dependency between different drivers, each of them having their
suspend/resume routings and the driver that implements the
platform_suspend_ops, that could also create some nice layering
violations and some difficult to solve dependencies.

> 
> Alternatively, those hooks can set/clear flags that can be accessed by
> drivers, but that of course may your drivers depend on the platform
> (still, in the microcode case the driver seems to be
> platform-dependent anyway).

It may be platform dependent, but the actual system-wide suspend/resume
implementations can vary a lot. For example you may have started with
some particular CPU architecture on your platforms, with one driver
implementing an instance of platform_suspend_ops, and then as you moved
to another CPU architecture, some of that could be managed by a generic
driver (e.g: ARM SCPI, ACPI etc. etc.).

The same HW blocks are likely to be present on these different SoCs, and
have the same requirements where they need to see a slightly different
path taken on suspend/resume. If we have to patch both the "legacy"
platform_suspend_ops, and the "new" platform_suspend_ops that does not
really scale.

Would it be that much of a stretch if we reflected e.g:
PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
communicated to platform_driver::suspend and platform_driver::resume?

Thanks!
-- 
Florian

Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
> Hi,
> 
> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>> Hello,
>>
>> I read the "Sleep States" documentation:
>> https://www.kernel.org/doc/Documentation/power/states.txt
>>
>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>> # ll /sys/power/
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>
>> # cat /sys/power/state
>> freeze mem
>>
>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>> where drivers are supposed to save their state (register values
>> will be lost), then Linux hands control over to firmware which
>> enables RAM self-refresh and powers the chip down. When the system
>> resumes, drivers restore their state from their copy in memory.
>>
>> One driver is responsible for loading/unloading microcode running
>> on the DSPs. This operation is required only when powering down
>> the chip, but it should be avoided for "low-latency" sleeps.
>>
>> The problem is that, if I understand correctly, drivers have no way
>> of knowing which sleep state is being entered/exited?
>>
>> How can I have the microcode driver take different decisions
>> based on the sleep state?
> 
> 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.

I am not sure this would be cleaner, because that would create a tighter
dependency between different drivers, each of them having their
suspend/resume routings and the driver that implements the
platform_suspend_ops, that could also create some nice layering
violations and some difficult to solve dependencies.

> 
> Alternatively, those hooks can set/clear flags that can be accessed by
> drivers, but that of course may your drivers depend on the platform
> (still, in the microcode case the driver seems to be
> platform-dependent anyway).

It may be platform dependent, but the actual system-wide suspend/resume
implementations can vary a lot. For example you may have started with
some particular CPU architecture on your platforms, with one driver
implementing an instance of platform_suspend_ops, and then as you moved
to another CPU architecture, some of that could be managed by a generic
driver (e.g: ARM SCPI, ACPI etc. etc.).

The same HW blocks are likely to be present on these different SoCs, and
have the same requirements where they need to see a slightly different
path taken on suspend/resume. If we have to patch both the "legacy"
platform_suspend_ops, and the "new" platform_suspend_ops that does not
really scale.

Would it be that much of a stretch if we reflected e.g:
PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
communicated to platform_driver::suspend and platform_driver::resume?

Thanks!
-- 
Florian

Re: Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>> Hi,
>>
>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>> Hello,
>>>
>>> I read the "Sleep States" documentation:
>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>
>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>> # ll /sys/power/
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>
>>> # cat /sys/power/state
>>> freeze mem
>>>
>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>> where drivers are supposed to save their state (register values
>>> will be lost), then Linux hands control over to firmware which
>>> enables RAM self-refresh and powers the chip down. When the system
>>> resumes, drivers restore their state from their copy in memory.
>>>
>>> One driver is responsible for loading/unloading microcode running
>>> on the DSPs. This operation is required only when powering down
>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>
>>> The problem is that, if I understand correctly, drivers have no way
>>> of knowing which sleep state is being entered/exited?
>>>
>>> How can I have the microcode driver take different decisions
>>> based on the sleep state?
>>
>> 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.
>
> I am not sure this would be cleaner, because that would create a tighter
> dependency between different drivers, each of them having their
> suspend/resume routings and the driver that implements the
> platform_suspend_ops, that could also create some nice layering
> violations and some difficult to solve dependencies.
>
>>
>> Alternatively, those hooks can set/clear flags that can be accessed by
>> drivers, but that of course may your drivers depend on the platform
>> (still, in the microcode case the driver seems to be
>> platform-dependent anyway).
>
> It may be platform dependent, but the actual system-wide suspend/resume
> implementations can vary a lot. For example you may have started with
> some particular CPU architecture on your platforms, with one driver
> implementing an instance of platform_suspend_ops, and then as you moved
> to another CPU architecture, some of that could be managed by a generic
> driver (e.g: ARM SCPI, ACPI etc. etc.).
>
> The same HW blocks are likely to be present on these different SoCs, and
> have the same requirements where they need to see a slightly different
> path taken on suspend/resume. If we have to patch both the "legacy"
> platform_suspend_ops, and the "new" platform_suspend_ops that does not
> really scale.
>
> Would it be that much of a stretch if we reflected e.g:
> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
> communicated to platform_driver::suspend and platform_driver::resume?

I'm not sure what you mean, really.

The ->suspend callback in struct platform_driver has been long deprecated.

Thanks,
Rafael

Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>> Hi,
>>
>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>> Hello,
>>>
>>> I read the "Sleep States" documentation:
>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>
>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>> # ll /sys/power/
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>
>>> # cat /sys/power/state
>>> freeze mem
>>>
>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>> where drivers are supposed to save their state (register values
>>> will be lost), then Linux hands control over to firmware which
>>> enables RAM self-refresh and powers the chip down. When the system
>>> resumes, drivers restore their state from their copy in memory.
>>>
>>> One driver is responsible for loading/unloading microcode running
>>> on the DSPs. This operation is required only when powering down
>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>
>>> The problem is that, if I understand correctly, drivers have no way
>>> of knowing which sleep state is being entered/exited?
>>>
>>> How can I have the microcode driver take different decisions
>>> based on the sleep state?
>>
>> 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.
>
> I am not sure this would be cleaner, because that would create a tighter
> dependency between different drivers, each of them having their
> suspend/resume routings and the driver that implements the
> platform_suspend_ops, that could also create some nice layering
> violations and some difficult to solve dependencies.
>
>>
>> Alternatively, those hooks can set/clear flags that can be accessed by
>> drivers, but that of course may your drivers depend on the platform
>> (still, in the microcode case the driver seems to be
>> platform-dependent anyway).
>
> It may be platform dependent, but the actual system-wide suspend/resume
> implementations can vary a lot. For example you may have started with
> some particular CPU architecture on your platforms, with one driver
> implementing an instance of platform_suspend_ops, and then as you moved
> to another CPU architecture, some of that could be managed by a generic
> driver (e.g: ARM SCPI, ACPI etc. etc.).
>
> The same HW blocks are likely to be present on these different SoCs, and
> have the same requirements where they need to see a slightly different
> path taken on suspend/resume. If we have to patch both the "legacy"
> platform_suspend_ops, and the "new" platform_suspend_ops that does not
> really scale.
>
> Would it be that much of a stretch if we reflected e.g:
> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
> communicated to platform_driver::suspend and platform_driver::resume?

I'm not sure what you mean, really.

The ->suspend callback in struct platform_driver has been long deprecated.

Thanks,
Rafael

Re: Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>> Hi,
>>>
>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>> Hello,
>>>>
>>>> I read the "Sleep States" documentation:
>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>
>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>> # ll /sys/power/
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>
>>>> # cat /sys/power/state
>>>> freeze mem
>>>>
>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>> where drivers are supposed to save their state (register values
>>>> will be lost), then Linux hands control over to firmware which
>>>> enables RAM self-refresh and powers the chip down. When the system
>>>> resumes, drivers restore their state from their copy in memory.
>>>>
>>>> One driver is responsible for loading/unloading microcode running
>>>> on the DSPs. This operation is required only when powering down
>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>
>>>> The problem is that, if I understand correctly, drivers have no way
>>>> of knowing which sleep state is being entered/exited?
>>>>
>>>> How can I have the microcode driver take different decisions
>>>> based on the sleep state?
>>>
>>> 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.
>>
>> I am not sure this would be cleaner, because that would create a tighter
>> dependency between different drivers, each of them having their
>> suspend/resume routings and the driver that implements the
>> platform_suspend_ops, that could also create some nice layering
>> violations and some difficult to solve dependencies.
>>
>>>
>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>> drivers, but that of course may your drivers depend on the platform
>>> (still, in the microcode case the driver seems to be
>>> platform-dependent anyway).
>>
>> It may be platform dependent, but the actual system-wide suspend/resume
>> implementations can vary a lot. For example you may have started with
>> some particular CPU architecture on your platforms, with one driver
>> implementing an instance of platform_suspend_ops, and then as you moved
>> to another CPU architecture, some of that could be managed by a generic
>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>
>> The same HW blocks are likely to be present on these different SoCs, and
>> have the same requirements where they need to see a slightly different
>> path taken on suspend/resume. If we have to patch both the "legacy"
>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>> really scale.
>>
>> Would it be that much of a stretch if we reflected e.g:
>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>> communicated to platform_driver::suspend and platform_driver::resume?
> 
> I'm not sure what you mean, really.
> 
> The ->suspend callback in struct platform_driver has been long deprecated.

What I mean is that we could take advantage of the pm_message_t argument
passed to platform_driver::resume and platform_driver::resume to
communicate the system sleep state we are about to enter (and conversely
exit).

This would allow drivers to take a different path whether e.g:
pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.

If these are deprecated, then should we introduce a global getter
function that reflects which suspend state we are about to enter?  This
would allow drivers do to something like (pseudo code):

static int drv_suspend(struct device d)
{
	suspend_state_t state = suspend_get_state();

	switch (state) {
	case PM_SUSPEND_STANDBY:
		return 0;
	case PM_SUSPEND_MEM:
		/* save HW context */
}
-- 
Florian

Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>> Hi,
>>>
>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>> Hello,
>>>>
>>>> I read the "Sleep States" documentation:
>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>
>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>> # ll /sys/power/
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>
>>>> # cat /sys/power/state
>>>> freeze mem
>>>>
>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>> where drivers are supposed to save their state (register values
>>>> will be lost), then Linux hands control over to firmware which
>>>> enables RAM self-refresh and powers the chip down. When the system
>>>> resumes, drivers restore their state from their copy in memory.
>>>>
>>>> One driver is responsible for loading/unloading microcode running
>>>> on the DSPs. This operation is required only when powering down
>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>
>>>> The problem is that, if I understand correctly, drivers have no way
>>>> of knowing which sleep state is being entered/exited?
>>>>
>>>> How can I have the microcode driver take different decisions
>>>> based on the sleep state?
>>>
>>> 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.
>>
>> I am not sure this would be cleaner, because that would create a tighter
>> dependency between different drivers, each of them having their
>> suspend/resume routings and the driver that implements the
>> platform_suspend_ops, that could also create some nice layering
>> violations and some difficult to solve dependencies.
>>
>>>
>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>> drivers, but that of course may your drivers depend on the platform
>>> (still, in the microcode case the driver seems to be
>>> platform-dependent anyway).
>>
>> It may be platform dependent, but the actual system-wide suspend/resume
>> implementations can vary a lot. For example you may have started with
>> some particular CPU architecture on your platforms, with one driver
>> implementing an instance of platform_suspend_ops, and then as you moved
>> to another CPU architecture, some of that could be managed by a generic
>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>
>> The same HW blocks are likely to be present on these different SoCs, and
>> have the same requirements where they need to see a slightly different
>> path taken on suspend/resume. If we have to patch both the "legacy"
>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>> really scale.
>>
>> Would it be that much of a stretch if we reflected e.g:
>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>> communicated to platform_driver::suspend and platform_driver::resume?
> 
> I'm not sure what you mean, really.
> 
> The ->suspend callback in struct platform_driver has been long deprecated.

What I mean is that we could take advantage of the pm_message_t argument
passed to platform_driver::resume and platform_driver::resume to
communicate the system sleep state we are about to enter (and conversely
exit).

This would allow drivers to take a different path whether e.g:
pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.

If these are deprecated, then should we introduce a global getter
function that reflects which suspend state we are about to enter?  This
would allow drivers do to something like (pseudo code):

static int drv_suspend(struct device d)
{
	suspend_state_t state = suspend_get_state();

	switch (state) {
	case PM_SUSPEND_STANDBY:
		return 0;
	case PM_SUSPEND_MEM:
		/* save HW context */
}
-- 
Florian

Re: Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>> Hi,
>>>>
>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>> Hello,
>>>>>
>>>>> I read the "Sleep States" documentation:
>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>
>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>> # ll /sys/power/
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>
>>>>> # cat /sys/power/state
>>>>> freeze mem
>>>>>
>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>> where drivers are supposed to save their state (register values
>>>>> will be lost), then Linux hands control over to firmware which
>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>
>>>>> One driver is responsible for loading/unloading microcode running
>>>>> on the DSPs. This operation is required only when powering down
>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>
>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>> of knowing which sleep state is being entered/exited?
>>>>>
>>>>> How can I have the microcode driver take different decisions
>>>>> based on the sleep state?
>>>>
>>>> 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.
>>>
>>> I am not sure this would be cleaner, because that would create a tighter
>>> dependency between different drivers, each of them having their
>>> suspend/resume routings and the driver that implements the
>>> platform_suspend_ops, that could also create some nice layering
>>> violations and some difficult to solve dependencies.
>>>
>>>>
>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>> drivers, but that of course may your drivers depend on the platform
>>>> (still, in the microcode case the driver seems to be
>>>> platform-dependent anyway).
>>>
>>> It may be platform dependent, but the actual system-wide suspend/resume
>>> implementations can vary a lot. For example you may have started with
>>> some particular CPU architecture on your platforms, with one driver
>>> implementing an instance of platform_suspend_ops, and then as you moved
>>> to another CPU architecture, some of that could be managed by a generic
>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>
>>> The same HW blocks are likely to be present on these different SoCs, and
>>> have the same requirements where they need to see a slightly different
>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>> really scale.
>>>
>>> Would it be that much of a stretch if we reflected e.g:
>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>> communicated to platform_driver::suspend and platform_driver::resume?
>>
>> I'm not sure what you mean, really.
>>
>> The ->suspend callback in struct platform_driver has been long deprecated.
>
> What I mean is that we could take advantage of the pm_message_t argument
> passed to platform_driver::resume and platform_driver::resume to
> communicate the system sleep state we are about to enter (and conversely
> exit).

So the ->suspend and ->resume callbacks in struct platform_driver
(which I think is what you are referring to) are legacy.

The majority of drivers I know about use struct dev_pm_ops and the
callbacks in there do not take the pm_message_t argument.

Moreover, even if they did take it, the exact meaning of
PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
would need to find out what they actually mean for the given platform
somehow anyway.

> This would allow drivers to take a different path whether e.g:
> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>
> If these are deprecated, then should we introduce a global getter
> function that reflects which suspend state we are about to enter?  This
> would allow drivers do to something like (pseudo code):
>
> static int drv_suspend(struct device d)
> {
>         suspend_state_t state = suspend_get_state();
>
>         switch (state) {
>         case PM_SUSPEND_STANDBY:
>                 return 0;
>         case PM_SUSPEND_MEM:
>                 /* save HW context */
> }

That is conceivable, but again, the meaning of STANDBY and MEM is
platform-specific.  Actions to be taken for, say, STANDBY, may differ
from one platform to another.

Thanks,
Rafael

Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>> Hi,
>>>>
>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>> Hello,
>>>>>
>>>>> I read the "Sleep States" documentation:
>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>
>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>> # ll /sys/power/
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>
>>>>> # cat /sys/power/state
>>>>> freeze mem
>>>>>
>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>> where drivers are supposed to save their state (register values
>>>>> will be lost), then Linux hands control over to firmware which
>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>
>>>>> One driver is responsible for loading/unloading microcode running
>>>>> on the DSPs. This operation is required only when powering down
>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>
>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>> of knowing which sleep state is being entered/exited?
>>>>>
>>>>> How can I have the microcode driver take different decisions
>>>>> based on the sleep state?
>>>>
>>>> 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.
>>>
>>> I am not sure this would be cleaner, because that would create a tighter
>>> dependency between different drivers, each of them having their
>>> suspend/resume routings and the driver that implements the
>>> platform_suspend_ops, that could also create some nice layering
>>> violations and some difficult to solve dependencies.
>>>
>>>>
>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>> drivers, but that of course may your drivers depend on the platform
>>>> (still, in the microcode case the driver seems to be
>>>> platform-dependent anyway).
>>>
>>> It may be platform dependent, but the actual system-wide suspend/resume
>>> implementations can vary a lot. For example you may have started with
>>> some particular CPU architecture on your platforms, with one driver
>>> implementing an instance of platform_suspend_ops, and then as you moved
>>> to another CPU architecture, some of that could be managed by a generic
>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>
>>> The same HW blocks are likely to be present on these different SoCs, and
>>> have the same requirements where they need to see a slightly different
>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>> really scale.
>>>
>>> Would it be that much of a stretch if we reflected e.g:
>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>> communicated to platform_driver::suspend and platform_driver::resume?
>>
>> I'm not sure what you mean, really.
>>
>> The ->suspend callback in struct platform_driver has been long deprecated.
>
> What I mean is that we could take advantage of the pm_message_t argument
> passed to platform_driver::resume and platform_driver::resume to
> communicate the system sleep state we are about to enter (and conversely
> exit).

So the ->suspend and ->resume callbacks in struct platform_driver
(which I think is what you are referring to) are legacy.

The majority of drivers I know about use struct dev_pm_ops and the
callbacks in there do not take the pm_message_t argument.

Moreover, even if they did take it, the exact meaning of
PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
would need to find out what they actually mean for the given platform
somehow anyway.

> This would allow drivers to take a different path whether e.g:
> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>
> If these are deprecated, then should we introduce a global getter
> function that reflects which suspend state we are about to enter?  This
> would allow drivers do to something like (pseudo code):
>
> static int drv_suspend(struct device d)
> {
>         suspend_state_t state = suspend_get_state();
>
>         switch (state) {
>         case PM_SUSPEND_STANDBY:
>                 return 0;
>         case PM_SUSPEND_MEM:
>                 /* save HW context */
> }

That is conceivable, but again, the meaning of STANDBY and MEM is
platform-specific.  Actions to be taken for, say, STANDBY, may differ
from one platform to another.

Thanks,
Rafael

Re: Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I read the "Sleep States" documentation:
>>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>>
>>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>>> # ll /sys/power/
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>>
>>>>>> # cat /sys/power/state
>>>>>> freeze mem
>>>>>>
>>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>>> where drivers are supposed to save their state (register values
>>>>>> will be lost), then Linux hands control over to firmware which
>>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>>
>>>>>> One driver is responsible for loading/unloading microcode running
>>>>>> on the DSPs. This operation is required only when powering down
>>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>>
>>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>>> of knowing which sleep state is being entered/exited?
>>>>>>
>>>>>> How can I have the microcode driver take different decisions
>>>>>> based on the sleep state?
>>>>>
>>>>> 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.
>>>>
>>>> I am not sure this would be cleaner, because that would create a tighter
>>>> dependency between different drivers, each of them having their
>>>> suspend/resume routings and the driver that implements the
>>>> platform_suspend_ops, that could also create some nice layering
>>>> violations and some difficult to solve dependencies.
>>>>
>>>>>
>>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>>> drivers, but that of course may your drivers depend on the platform
>>>>> (still, in the microcode case the driver seems to be
>>>>> platform-dependent anyway).
>>>>
>>>> It may be platform dependent, but the actual system-wide suspend/resume
>>>> implementations can vary a lot. For example you may have started with
>>>> some particular CPU architecture on your platforms, with one driver
>>>> implementing an instance of platform_suspend_ops, and then as you moved
>>>> to another CPU architecture, some of that could be managed by a generic
>>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>>
>>>> The same HW blocks are likely to be present on these different SoCs, and
>>>> have the same requirements where they need to see a slightly different
>>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>>> really scale.
>>>>
>>>> Would it be that much of a stretch if we reflected e.g:
>>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>>> communicated to platform_driver::suspend and platform_driver::resume?
>>>
>>> I'm not sure what you mean, really.
>>>
>>> The ->suspend callback in struct platform_driver has been long deprecated.
>>
>> What I mean is that we could take advantage of the pm_message_t argument
>> passed to platform_driver::resume and platform_driver::resume to
>> communicate the system sleep state we are about to enter (and conversely
>> exit).
> 
> So the ->suspend and ->resume callbacks in struct platform_driver
> (which I think is what you are referring to) are legacy.
> 
> The majority of drivers I know about use struct dev_pm_ops and the
> callbacks in there do not take the pm_message_t argument.
> 
> Moreover, even if they did take it, the exact meaning of
> PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
> would need to find out what they actually mean for the given platform
> somehow anyway.
> 
>> This would allow drivers to take a different path whether e.g:
>> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>>
>> If these are deprecated, then should we introduce a global getter
>> function that reflects which suspend state we are about to enter?  This
>> would allow drivers do to something like (pseudo code):
>>
>> static int drv_suspend(struct device d)
>> {
>>         suspend_state_t state = suspend_get_state();
>>
>>         switch (state) {
>>         case PM_SUSPEND_STANDBY:
>>                 return 0;
>>         case PM_SUSPEND_MEM:
>>                 /* save HW context */
>> }
> 
> That is conceivable, but again, the meaning of STANDBY and MEM is
> platform-specific.  Actions to be taken for, say, STANDBY, may differ
> from one platform to another.

True, though I would only expect drivers for that particular platform to
care about that information and these drivers would only make sense on
that particular platform so the meaning of STANDBY and MEM would be
clear for those drivers. The intent is really to keep the "distributed"
model where individual drivers manage their particular piece of HW,
while utilizing a global piece of information that is platform specific.

If this seems acceptable to you along with proper documentation to
illustrate the platform specific meaning of these states, I will got
ahead and cook a patch.

Thanks!
-- 
Florian

Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I read the "Sleep States" documentation:
>>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>>
>>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>>> # ll /sys/power/
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>>
>>>>>> # cat /sys/power/state
>>>>>> freeze mem
>>>>>>
>>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>>> where drivers are supposed to save their state (register values
>>>>>> will be lost), then Linux hands control over to firmware which
>>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>>
>>>>>> One driver is responsible for loading/unloading microcode running
>>>>>> on the DSPs. This operation is required only when powering down
>>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>>
>>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>>> of knowing which sleep state is being entered/exited?
>>>>>>
>>>>>> How can I have the microcode driver take different decisions
>>>>>> based on the sleep state?
>>>>>
>>>>> 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.
>>>>
>>>> I am not sure this would be cleaner, because that would create a tighter
>>>> dependency between different drivers, each of them having their
>>>> suspend/resume routings and the driver that implements the
>>>> platform_suspend_ops, that could also create some nice layering
>>>> violations and some difficult to solve dependencies.
>>>>
>>>>>
>>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>>> drivers, but that of course may your drivers depend on the platform
>>>>> (still, in the microcode case the driver seems to be
>>>>> platform-dependent anyway).
>>>>
>>>> It may be platform dependent, but the actual system-wide suspend/resume
>>>> implementations can vary a lot. For example you may have started with
>>>> some particular CPU architecture on your platforms, with one driver
>>>> implementing an instance of platform_suspend_ops, and then as you moved
>>>> to another CPU architecture, some of that could be managed by a generic
>>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>>
>>>> The same HW blocks are likely to be present on these different SoCs, and
>>>> have the same requirements where they need to see a slightly different
>>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>>> really scale.
>>>>
>>>> Would it be that much of a stretch if we reflected e.g:
>>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>>> communicated to platform_driver::suspend and platform_driver::resume?
>>>
>>> I'm not sure what you mean, really.
>>>
>>> The ->suspend callback in struct platform_driver has been long deprecated.
>>
>> What I mean is that we could take advantage of the pm_message_t argument
>> passed to platform_driver::resume and platform_driver::resume to
>> communicate the system sleep state we are about to enter (and conversely
>> exit).
> 
> So the ->suspend and ->resume callbacks in struct platform_driver
> (which I think is what you are referring to) are legacy.
> 
> The majority of drivers I know about use struct dev_pm_ops and the
> callbacks in there do not take the pm_message_t argument.
> 
> Moreover, even if they did take it, the exact meaning of
> PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
> would need to find out what they actually mean for the given platform
> somehow anyway.
> 
>> This would allow drivers to take a different path whether e.g:
>> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>>
>> If these are deprecated, then should we introduce a global getter
>> function that reflects which suspend state we are about to enter?  This
>> would allow drivers do to something like (pseudo code):
>>
>> static int drv_suspend(struct device d)
>> {
>>         suspend_state_t state = suspend_get_state();
>>
>>         switch (state) {
>>         case PM_SUSPEND_STANDBY:
>>                 return 0;
>>         case PM_SUSPEND_MEM:
>>                 /* save HW context */
>> }
> 
> That is conceivable, but again, the meaning of STANDBY and MEM is
> platform-specific.  Actions to be taken for, say, STANDBY, may differ
> from one platform to another.

True, though I would only expect drivers for that particular platform to
care about that information and these drivers would only make sense on
that particular platform so the meaning of STANDBY and MEM would be
clear for those drivers. The intent is really to keep the "distributed"
model where individual drivers manage their particular piece of HW,
while utilizing a global piece of information that is platform specific.

If this seems acceptable to you along with proper documentation to
illustrate the platform specific meaning of these states, I will got
ahead and cook a patch.

Thanks!
-- 
Florian

Re: Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 1:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
>> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> I read the "Sleep States" documentation:
>>>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>>>
>>>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>>>> # ll /sys/power/
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>>>
>>>>>>> # cat /sys/power/state
>>>>>>> freeze mem
>>>>>>>
>>>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>>>> where drivers are supposed to save their state (register values
>>>>>>> will be lost), then Linux hands control over to firmware which
>>>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>>>
>>>>>>> One driver is responsible for loading/unloading microcode running
>>>>>>> on the DSPs. This operation is required only when powering down
>>>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>>>
>>>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>>>> of knowing which sleep state is being entered/exited?
>>>>>>>
>>>>>>> How can I have the microcode driver take different decisions
>>>>>>> based on the sleep state?
>>>>>>
>>>>>> 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.
>>>>>
>>>>> I am not sure this would be cleaner, because that would create a tighter
>>>>> dependency between different drivers, each of them having their
>>>>> suspend/resume routings and the driver that implements the
>>>>> platform_suspend_ops, that could also create some nice layering
>>>>> violations and some difficult to solve dependencies.
>>>>>
>>>>>>
>>>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>>>> drivers, but that of course may your drivers depend on the platform
>>>>>> (still, in the microcode case the driver seems to be
>>>>>> platform-dependent anyway).
>>>>>
>>>>> It may be platform dependent, but the actual system-wide suspend/resume
>>>>> implementations can vary a lot. For example you may have started with
>>>>> some particular CPU architecture on your platforms, with one driver
>>>>> implementing an instance of platform_suspend_ops, and then as you moved
>>>>> to another CPU architecture, some of that could be managed by a generic
>>>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>>>
>>>>> The same HW blocks are likely to be present on these different SoCs, and
>>>>> have the same requirements where they need to see a slightly different
>>>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>>>> really scale.
>>>>>
>>>>> Would it be that much of a stretch if we reflected e.g:
>>>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>>>> communicated to platform_driver::suspend and platform_driver::resume?
>>>>
>>>> I'm not sure what you mean, really.
>>>>
>>>> The ->suspend callback in struct platform_driver has been long deprecated.
>>>
>>> What I mean is that we could take advantage of the pm_message_t argument
>>> passed to platform_driver::resume and platform_driver::resume to
>>> communicate the system sleep state we are about to enter (and conversely
>>> exit).
>>
>> So the ->suspend and ->resume callbacks in struct platform_driver
>> (which I think is what you are referring to) are legacy.
>>
>> The majority of drivers I know about use struct dev_pm_ops and the
>> callbacks in there do not take the pm_message_t argument.
>>
>> Moreover, even if they did take it, the exact meaning of
>> PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
>> would need to find out what they actually mean for the given platform
>> somehow anyway.
>>
>>> This would allow drivers to take a different path whether e.g:
>>> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>>>
>>> If these are deprecated, then should we introduce a global getter
>>> function that reflects which suspend state we are about to enter?  This
>>> would allow drivers do to something like (pseudo code):
>>>
>>> static int drv_suspend(struct device d)
>>> {
>>>         suspend_state_t state = suspend_get_state();
>>>
>>>         switch (state) {
>>>         case PM_SUSPEND_STANDBY:
>>>                 return 0;
>>>         case PM_SUSPEND_MEM:
>>>                 /* save HW context */
>>> }
>>
>> That is conceivable, but again, the meaning of STANDBY and MEM is
>> platform-specific.  Actions to be taken for, say, STANDBY, may differ
>> from one platform to another.
>
> True, though I would only expect drivers for that particular platform to
> care about that information and these drivers would only make sense on
> that particular platform so the meaning of STANDBY and MEM would be
> clear for those drivers. The intent is really to keep the "distributed"
> model where individual drivers manage their particular piece of HW,
> while utilizing a global piece of information that is platform specific.

Well, but in that case, why don't you have a "target_state" variable
somewhere in the platform code and make your drivers look at it?

ACPI has acpi_target_system_state() for this very purpose, for example.

> If this seems acceptable to you along with proper documentation to
> illustrate the platform specific meaning of these states, I will got
> ahead and cook a patch.

I wouldn't like platform-specific things to pretend that they are generic.

Thanks,
Rafael

Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 1:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
>> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> I read the "Sleep States" documentation:
>>>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>>>
>>>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>>>> # ll /sys/power/
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>>>
>>>>>>> # cat /sys/power/state
>>>>>>> freeze mem
>>>>>>>
>>>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>>>> where drivers are supposed to save their state (register values
>>>>>>> will be lost), then Linux hands control over to firmware which
>>>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>>>
>>>>>>> One driver is responsible for loading/unloading microcode running
>>>>>>> on the DSPs. This operation is required only when powering down
>>>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>>>
>>>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>>>> of knowing which sleep state is being entered/exited?
>>>>>>>
>>>>>>> How can I have the microcode driver take different decisions
>>>>>>> based on the sleep state?
>>>>>>
>>>>>> 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.
>>>>>
>>>>> I am not sure this would be cleaner, because that would create a tighter
>>>>> dependency between different drivers, each of them having their
>>>>> suspend/resume routings and the driver that implements the
>>>>> platform_suspend_ops, that could also create some nice layering
>>>>> violations and some difficult to solve dependencies.
>>>>>
>>>>>>
>>>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>>>> drivers, but that of course may your drivers depend on the platform
>>>>>> (still, in the microcode case the driver seems to be
>>>>>> platform-dependent anyway).
>>>>>
>>>>> It may be platform dependent, but the actual system-wide suspend/resume
>>>>> implementations can vary a lot. For example you may have started with
>>>>> some particular CPU architecture on your platforms, with one driver
>>>>> implementing an instance of platform_suspend_ops, and then as you moved
>>>>> to another CPU architecture, some of that could be managed by a generic
>>>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>>>
>>>>> The same HW blocks are likely to be present on these different SoCs, and
>>>>> have the same requirements where they need to see a slightly different
>>>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>>>> really scale.
>>>>>
>>>>> Would it be that much of a stretch if we reflected e.g:
>>>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>>>> communicated to platform_driver::suspend and platform_driver::resume?
>>>>
>>>> I'm not sure what you mean, really.
>>>>
>>>> The ->suspend callback in struct platform_driver has been long deprecated.
>>>
>>> What I mean is that we could take advantage of the pm_message_t argument
>>> passed to platform_driver::resume and platform_driver::resume to
>>> communicate the system sleep state we are about to enter (and conversely
>>> exit).
>>
>> So the ->suspend and ->resume callbacks in struct platform_driver
>> (which I think is what you are referring to) are legacy.
>>
>> The majority of drivers I know about use struct dev_pm_ops and the
>> callbacks in there do not take the pm_message_t argument.
>>
>> Moreover, even if they did take it, the exact meaning of
>> PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
>> would need to find out what they actually mean for the given platform
>> somehow anyway.
>>
>>> This would allow drivers to take a different path whether e.g:
>>> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>>>
>>> If these are deprecated, then should we introduce a global getter
>>> function that reflects which suspend state we are about to enter?  This
>>> would allow drivers do to something like (pseudo code):
>>>
>>> static int drv_suspend(struct device d)
>>> {
>>>         suspend_state_t state = suspend_get_state();
>>>
>>>         switch (state) {
>>>         case PM_SUSPEND_STANDBY:
>>>                 return 0;
>>>         case PM_SUSPEND_MEM:
>>>                 /* save HW context */
>>> }
>>
>> That is conceivable, but again, the meaning of STANDBY and MEM is
>> platform-specific.  Actions to be taken for, say, STANDBY, may differ
>> from one platform to another.
>
> True, though I would only expect drivers for that particular platform to
> care about that information and these drivers would only make sense on
> that particular platform so the meaning of STANDBY and MEM would be
> clear for those drivers. The intent is really to keep the "distributed"
> model where individual drivers manage their particular piece of HW,
> while utilizing a global piece of information that is platform specific.

Well, but in that case, why don't you have a "target_state" variable
somewhere in the platform code and make your drivers look at it?

ACPI has acpi_target_system_state() for this very purpose, for example.

> If this seems acceptable to you along with proper documentation to
> illustrate the platform specific meaning of these states, I will got
> ahead and cook a patch.

I wouldn't like platform-specific things to pretend that they are generic.

Thanks,
Rafael

Re: Drivers taking different actions depending on sleep state

[Alexandre Belloni]

On 21/06/2017 at 16:55:04 -0700, Florian Fainelli wrote:
> > That is conceivable, but again, the meaning of STANDBY and MEM is
> > platform-specific.  Actions to be taken for, say, STANDBY, may differ
> > from one platform to another.
> 
> True, though I would only expect drivers for that particular platform to
> care about that information and these drivers would only make sense on
> that particular platform so the meaning of STANDBY and MEM would be
> clear for those drivers. The intent is really to keep the "distributed"
> model where individual drivers manage their particular piece of HW,
> while utilizing a global piece of information that is platform specific.
> 
> If this seems acceptable to you along with proper documentation to
> illustrate the platform specific meaning of these states, I will got
> ahead and cook a patch.

Well, that won't work for us. We need need two kind of information:
 - whether main clock is switched from the master clock to the slow
   clock
 - whether VDDcore will be cut

for the first one, we already have an hackish callback:
at91_suspend_entering_slow_clock() that will call from the platform
specific drivers.

The main issue now is for the second one. We also need it for IPs that
are shared with other SoCs. For example, the macb ethernet controller
that is shared with the zynq and the m_can controller. In those cases,
I don't think it is a good idea to add platform specific code in the
drivers but at the same time, I don't think it is reasonable to
unconditionally reinit the IP on resume as there is a significant
latency hit. I don't think that the xilinx guys will like when suddenly
their platform takes 500ms to resume.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Drivers taking different actions depending on sleep state

[Alexandre Belloni]

On 21/06/2017 at 16:55:04 -0700, Florian Fainelli wrote:
> > That is conceivable, but again, the meaning of STANDBY and MEM is
> > platform-specific.  Actions to be taken for, say, STANDBY, may differ
> > from one platform to another.
> 
> True, though I would only expect drivers for that particular platform to
> care about that information and these drivers would only make sense on
> that particular platform so the meaning of STANDBY and MEM would be
> clear for those drivers. The intent is really to keep the "distributed"
> model where individual drivers manage their particular piece of HW,
> while utilizing a global piece of information that is platform specific.
> 
> If this seems acceptable to you along with proper documentation to
> illustrate the platform specific meaning of these states, I will got
> ahead and cook a patch.

Well, that won't work for us. We need need two kind of information:
 - whether main clock is switched from the master clock to the slow
   clock
 - whether VDDcore will be cut

for the first one, we already have an hackish callback:
at91_suspend_entering_slow_clock() that will call from the platform
specific drivers.

The main issue now is for the second one. We also need it for IPs that
are shared with other SoCs. For example, the macb ethernet controller
that is shared with the zynq and the m_can controller. In those cases,
I don't think it is a good idea to add platform specific code in the
drivers but at the same time, I don't think it is reasonable to
unconditionally reinit the IP on resume as there is a significant
latency hit. I don't think that the xilinx guys will like when suddenly
their platform takes 500ms to resume.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/21/2017 05:03 PM, Rafael J. Wysocki wrote:
> On Thu, Jun 22, 2017 at 1:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
>>> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> I read the "Sleep States" documentation:
>>>>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>>>>
>>>>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>>>>> # ll /sys/power/
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>>>>
>>>>>>>> # cat /sys/power/state
>>>>>>>> freeze mem
>>>>>>>>
>>>>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>>>>> where drivers are supposed to save their state (register values
>>>>>>>> will be lost), then Linux hands control over to firmware which
>>>>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>>>>
>>>>>>>> One driver is responsible for loading/unloading microcode running
>>>>>>>> on the DSPs. This operation is required only when powering down
>>>>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>>>>
>>>>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>>>>> of knowing which sleep state is being entered/exited?
>>>>>>>>
>>>>>>>> How can I have the microcode driver take different decisions
>>>>>>>> based on the sleep state?
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> I am not sure this would be cleaner, because that would create a tighter
>>>>>> dependency between different drivers, each of them having their
>>>>>> suspend/resume routings and the driver that implements the
>>>>>> platform_suspend_ops, that could also create some nice layering
>>>>>> violations and some difficult to solve dependencies.
>>>>>>
>>>>>>>
>>>>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>>>>> drivers, but that of course may your drivers depend on the platform
>>>>>>> (still, in the microcode case the driver seems to be
>>>>>>> platform-dependent anyway).
>>>>>>
>>>>>> It may be platform dependent, but the actual system-wide suspend/resume
>>>>>> implementations can vary a lot. For example you may have started with
>>>>>> some particular CPU architecture on your platforms, with one driver
>>>>>> implementing an instance of platform_suspend_ops, and then as you moved
>>>>>> to another CPU architecture, some of that could be managed by a generic
>>>>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>>>>
>>>>>> The same HW blocks are likely to be present on these different SoCs, and
>>>>>> have the same requirements where they need to see a slightly different
>>>>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>>>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>>>>> really scale.
>>>>>>
>>>>>> Would it be that much of a stretch if we reflected e.g:
>>>>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>>>>> communicated to platform_driver::suspend and platform_driver::resume?
>>>>>
>>>>> I'm not sure what you mean, really.
>>>>>
>>>>> The ->suspend callback in struct platform_driver has been long deprecated.
>>>>
>>>> What I mean is that we could take advantage of the pm_message_t argument
>>>> passed to platform_driver::resume and platform_driver::resume to
>>>> communicate the system sleep state we are about to enter (and conversely
>>>> exit).
>>>
>>> So the ->suspend and ->resume callbacks in struct platform_driver
>>> (which I think is what you are referring to) are legacy.
>>>
>>> The majority of drivers I know about use struct dev_pm_ops and the
>>> callbacks in there do not take the pm_message_t argument.
>>>
>>> Moreover, even if they did take it, the exact meaning of
>>> PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
>>> would need to find out what they actually mean for the given platform
>>> somehow anyway.
>>>
>>>> This would allow drivers to take a different path whether e.g:
>>>> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>>>>
>>>> If these are deprecated, then should we introduce a global getter
>>>> function that reflects which suspend state we are about to enter?  This
>>>> would allow drivers do to something like (pseudo code):
>>>>
>>>> static int drv_suspend(struct device d)
>>>> {
>>>>         suspend_state_t state = suspend_get_state();
>>>>
>>>>         switch (state) {
>>>>         case PM_SUSPEND_STANDBY:
>>>>                 return 0;
>>>>         case PM_SUSPEND_MEM:
>>>>                 /* save HW context */
>>>> }
>>>
>>> That is conceivable, but again, the meaning of STANDBY and MEM is
>>> platform-specific.  Actions to be taken for, say, STANDBY, may differ
>>> from one platform to another.
>>
>> True, though I would only expect drivers for that particular platform to
>> care about that information and these drivers would only make sense on
>> that particular platform so the meaning of STANDBY and MEM would be
>> clear for those drivers. The intent is really to keep the "distributed"
>> model where individual drivers manage their particular piece of HW,
>> while utilizing a global piece of information that is platform specific.
> 
> Well, but in that case, why don't you have a "target_state" variable
> somewhere in the platform code and make your drivers look at it?

For the reasons explained before, if the same set of drivers need to
deal with one or more platform_suspend_ops driver, say a classic
homegrown one, and one that is ACPI/ARM SCPI based for instance, we
would have to sprinkle checks like these in the driver:

static int drv_suspend(struct device *d)
{
	if (platform_suspend_get_state() == PM_SUSPEND_STANDBY ||
	    acpi_target_system_state() == XXXX |


and so on and so forth, that does not seem to scale horizontally.

> 
> ACPI has acpi_target_system_state() for this very purpose, for example.
> 
>> If this seems acceptable to you along with proper documentation to
>> illustrate the platform specific meaning of these states, I will got
>> ahead and cook a patch.
> 
> I wouldn't like platform-specific things to pretend that they are generic.

Would a notifier model be more appropriate perhaps? The mechanism by
which the notifications get registered to and signaled can be made
generic, the exact information however would be inherently
platform_suspend_ops specific, and only the relevant drivers that need
to subscribe to that kind of information would do that.
-- 
Florian

Drivers taking different actions depending on sleep state

[Florian Fainelli]

On 06/21/2017 05:03 PM, Rafael J. Wysocki wrote:
> On Thu, Jun 22, 2017 at 1:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
>>> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Fri, Jun 9, 2017 at 5:20 PM, Mason <slash.tmp@free.fr> wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> I read the "Sleep States" documentation:
>>>>>>>> https://www.kernel.org/doc/Documentation/power/states.txt
>>>>>>>>
>>>>>>>> It mentions /sys/power/mem_sleep but I don't have that in 4.9
>>>>>>>> # ll /sys/power/
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_async
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 pm_freeze_timeout
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 state
>>>>>>>> -rw-r--r--    1 root     root          4096 Jan  1 00:31 wakeup_count
>>>>>>>>
>>>>>>>> # cat /sys/power/state
>>>>>>>> freeze mem
>>>>>>>>
>>>>>>>> Currently my platform's "mem" is a true suspend-to-RAM trigger,
>>>>>>>> where drivers are supposed to save their state (register values
>>>>>>>> will be lost), then Linux hands control over to firmware which
>>>>>>>> enables RAM self-refresh and powers the chip down. When the system
>>>>>>>> resumes, drivers restore their state from their copy in memory.
>>>>>>>>
>>>>>>>> One driver is responsible for loading/unloading microcode running
>>>>>>>> on the DSPs. This operation is required only when powering down
>>>>>>>> the chip, but it should be avoided for "low-latency" sleeps.
>>>>>>>>
>>>>>>>> The problem is that, if I understand correctly, drivers have no way
>>>>>>>> of knowing which sleep state is being entered/exited?
>>>>>>>>
>>>>>>>> How can I have the microcode driver take different decisions
>>>>>>>> based on the sleep state?
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> I am not sure this would be cleaner, because that would create a tighter
>>>>>> dependency between different drivers, each of them having their
>>>>>> suspend/resume routings and the driver that implements the
>>>>>> platform_suspend_ops, that could also create some nice layering
>>>>>> violations and some difficult to solve dependencies.
>>>>>>
>>>>>>>
>>>>>>> Alternatively, those hooks can set/clear flags that can be accessed by
>>>>>>> drivers, but that of course may your drivers depend on the platform
>>>>>>> (still, in the microcode case the driver seems to be
>>>>>>> platform-dependent anyway).
>>>>>>
>>>>>> It may be platform dependent, but the actual system-wide suspend/resume
>>>>>> implementations can vary a lot. For example you may have started with
>>>>>> some particular CPU architecture on your platforms, with one driver
>>>>>> implementing an instance of platform_suspend_ops, and then as you moved
>>>>>> to another CPU architecture, some of that could be managed by a generic
>>>>>> driver (e.g: ARM SCPI, ACPI etc. etc.).
>>>>>>
>>>>>> The same HW blocks are likely to be present on these different SoCs, and
>>>>>> have the same requirements where they need to see a slightly different
>>>>>> path taken on suspend/resume. If we have to patch both the "legacy"
>>>>>> platform_suspend_ops, and the "new" platform_suspend_ops that does not
>>>>>> really scale.
>>>>>>
>>>>>> Would it be that much of a stretch if we reflected e.g:
>>>>>> PM_SUSPEND_STANDBY, PM_SUSPEND_MEM into the pm_message_t that is
>>>>>> communicated to platform_driver::suspend and platform_driver::resume?
>>>>>
>>>>> I'm not sure what you mean, really.
>>>>>
>>>>> The ->suspend callback in struct platform_driver has been long deprecated.
>>>>
>>>> What I mean is that we could take advantage of the pm_message_t argument
>>>> passed to platform_driver::resume and platform_driver::resume to
>>>> communicate the system sleep state we are about to enter (and conversely
>>>> exit).
>>>
>>> So the ->suspend and ->resume callbacks in struct platform_driver
>>> (which I think is what you are referring to) are legacy.
>>>
>>> The majority of drivers I know about use struct dev_pm_ops and the
>>> callbacks in there do not take the pm_message_t argument.
>>>
>>> Moreover, even if they did take it, the exact meaning of
>>> PM_SUSPEND_STANDBY and PM_SUSPEND_MEM is platform-specific and drivers
>>> would need to find out what they actually mean for the given platform
>>> somehow anyway.
>>>
>>>> This would allow drivers to take a different path whether e.g:
>>>> pm_message_t == PM_SUSPEND_STANDBY or PM_SUSPEND_MEM.
>>>>
>>>> If these are deprecated, then should we introduce a global getter
>>>> function that reflects which suspend state we are about to enter?  This
>>>> would allow drivers do to something like (pseudo code):
>>>>
>>>> static int drv_suspend(struct device d)
>>>> {
>>>>         suspend_state_t state = suspend_get_state();
>>>>
>>>>         switch (state) {
>>>>         case PM_SUSPEND_STANDBY:
>>>>                 return 0;
>>>>         case PM_SUSPEND_MEM:
>>>>                 /* save HW context */
>>>> }
>>>
>>> That is conceivable, but again, the meaning of STANDBY and MEM is
>>> platform-specific.  Actions to be taken for, say, STANDBY, may differ
>>> from one platform to another.
>>
>> True, though I would only expect drivers for that particular platform to
>> care about that information and these drivers would only make sense on
>> that particular platform so the meaning of STANDBY and MEM would be
>> clear for those drivers. The intent is really to keep the "distributed"
>> model where individual drivers manage their particular piece of HW,
>> while utilizing a global piece of information that is platform specific.
> 
> Well, but in that case, why don't you have a "target_state" variable
> somewhere in the platform code and make your drivers look at it?

For the reasons explained before, if the same set of drivers need to
deal with one or more platform_suspend_ops driver, say a classic
homegrown one, and one that is ACPI/ARM SCPI based for instance, we
would have to sprinkle checks like these in the driver:

static int drv_suspend(struct device *d)
{
	if (platform_suspend_get_state() == PM_SUSPEND_STANDBY ||
	    acpi_target_system_state() == XXXX |


and so on and so forth, that does not seem to scale horizontally.

> 
> ACPI has acpi_target_system_state() for this very purpose, for example.
> 
>> If this seems acceptable to you along with proper documentation to
>> illustrate the platform specific meaning of these states, I will got
>> ahead and cook a patch.
> 
> I wouldn't like platform-specific things to pretend that they are generic.

Would a notifier model be more appropriate perhaps? The mechanism by
which the notifications get registered to and signaled can be made
generic, the exact information however would be inherently
platform_suspend_ops specific, and only the relevant drivers that need
to subscribe to that kind of information would do that.
-- 
Florian

Re: Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thursday, June 22, 2017 10:51:02 AM Alexandre Belloni wrote:
> On 21/06/2017 at 16:55:04 -0700, Florian Fainelli wrote:
> > > That is conceivable, but again, the meaning of STANDBY and MEM is
> > > platform-specific.  Actions to be taken for, say, STANDBY, may differ
> > > from one platform to another.
> > 
> > True, though I would only expect drivers for that particular platform to
> > care about that information and these drivers would only make sense on
> > that particular platform so the meaning of STANDBY and MEM would be
> > clear for those drivers. The intent is really to keep the "distributed"
> > model where individual drivers manage their particular piece of HW,
> > while utilizing a global piece of information that is platform specific.
> > 
> > If this seems acceptable to you along with proper documentation to
> > illustrate the platform specific meaning of these states, I will got
> > ahead and cook a patch.
> 
> Well, that won't work for us. We need need two kind of information:
>  - whether main clock is switched from the master clock to the slow
>    clock
>  - whether VDDcore will be cut
> 
> for the first one, we already have an hackish callback:
> at91_suspend_entering_slow_clock() that will call from the platform
> specific drivers.
> 
> The main issue now is for the second one. We also need it for IPs that
> are shared with other SoCs. For example, the macb ethernet controller
> that is shared with the zynq and the m_can controller. In those cases,
> I don't think it is a good idea to add platform specific code in the
> drivers but at the same time, I don't think it is reasonable to
> unconditionally reinit the IP on resume as there is a significant
> latency hit. I don't think that the xilinx guys will like when suddenly
> their platform takes 500ms to resume.

We can add a ->target_state callback in struct platform_suspend_ops that
will return a value representing the the particular target state the platform
has chosen to go to.  Along with it, there can be a list of values for all
of the low-level sleep states defined by all of the different platforms
(in which case I'd like to reserve values 0-5 for the ACPI system states).

There can be a wrapper, say platform_suspend_target_state() around that,
so the driver will call it, compare the return value with what is known to it
about the different platforms it can run on and act accordingly.

Thanks,
Rafael

Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thursday, June 22, 2017 10:51:02 AM Alexandre Belloni wrote:
> On 21/06/2017 at 16:55:04 -0700, Florian Fainelli wrote:
> > > That is conceivable, but again, the meaning of STANDBY and MEM is
> > > platform-specific.  Actions to be taken for, say, STANDBY, may differ
> > > from one platform to another.
> > 
> > True, though I would only expect drivers for that particular platform to
> > care about that information and these drivers would only make sense on
> > that particular platform so the meaning of STANDBY and MEM would be
> > clear for those drivers. The intent is really to keep the "distributed"
> > model where individual drivers manage their particular piece of HW,
> > while utilizing a global piece of information that is platform specific.
> > 
> > If this seems acceptable to you along with proper documentation to
> > illustrate the platform specific meaning of these states, I will got
> > ahead and cook a patch.
> 
> Well, that won't work for us. We need need two kind of information:
>  - whether main clock is switched from the master clock to the slow
>    clock
>  - whether VDDcore will be cut
> 
> for the first one, we already have an hackish callback:
> at91_suspend_entering_slow_clock() that will call from the platform
> specific drivers.
> 
> The main issue now is for the second one. We also need it for IPs that
> are shared with other SoCs. For example, the macb ethernet controller
> that is shared with the zynq and the m_can controller. In those cases,
> I don't think it is a good idea to add platform specific code in the
> drivers but at the same time, I don't think it is reasonable to
> unconditionally reinit the IP on resume as there is a significant
> latency hit. I don't think that the xilinx guys will like when suddenly
> their platform takes 500ms to resume.

We can add a ->target_state callback in struct platform_suspend_ops that
will return a value representing the the particular target state the platform
has chosen to go to.  Along with it, there can be a list of values for all
of the low-level sleep states defined by all of the different platforms
(in which case I'd like to reserve values 0-5 for the ACPI system states).

There can be a wrapper, say platform_suspend_target_state() around that,
so the driver will call it, compare the return value with what is known to it
about the different platforms it can run on and act accordingly.

Thanks,
Rafael

Re: Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 5:18 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
> On 06/21/2017 05:03 PM, Rafael J. Wysocki wrote:
>> On Thu, Jun 22, 2017 at 1:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
>>>> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>>>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:

[cut]

>
> For the reasons explained before, if the same set of drivers need to
> deal with one or more platform_suspend_ops driver, say a classic
> homegrown one, and one that is ACPI/ARM SCPI based for instance, we
> would have to sprinkle checks like these in the driver:
>
> static int drv_suspend(struct device *d)
> {
>         if (platform_suspend_get_state() == PM_SUSPEND_STANDBY ||
>             acpi_target_system_state() == XXXX |
>
>
> and so on and so forth, that does not seem to scale horizontally.
>
>>
>> ACPI has acpi_target_system_state() for this very purpose, for example.
>>
>>> If this seems acceptable to you along with proper documentation to
>>> illustrate the platform specific meaning of these states, I will got
>>> ahead and cook a patch.
>>
>> I wouldn't like platform-specific things to pretend that they are generic.
>
> Would a notifier model be more appropriate perhaps? The mechanism by
> which the notifications get registered to and signaled can be made
> generic, the exact information however would be inherently
> platform_suspend_ops specific, and only the relevant drivers that need
> to subscribe to that kind of information would do that.

Please see the message I've just sent in this thread. :-)

Thanks,
Rafael

Drivers taking different actions depending on sleep state

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 5:18 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
> On 06/21/2017 05:03 PM, Rafael J. Wysocki wrote:
>> On Thu, Jun 22, 2017 at 1:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 06/21/2017 03:57 PM, Rafael J. Wysocki wrote:
>>>> On Thu, Jun 22, 2017 at 12:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> On 06/21/2017 02:59 PM, Rafael J. Wysocki wrote:
>>>>>> On Wed, Jun 21, 2017 at 11:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>> On 06/09/2017 03:53 PM, Rafael J. Wysocki wrote:

[cut]

>
> For the reasons explained before, if the same set of drivers need to
> deal with one or more platform_suspend_ops driver, say a classic
> homegrown one, and one that is ACPI/ARM SCPI based for instance, we
> would have to sprinkle checks like these in the driver:
>
> static int drv_suspend(struct device *d)
> {
>         if (platform_suspend_get_state() == PM_SUSPEND_STANDBY ||
>             acpi_target_system_state() == XXXX |
>
>
> and so on and so forth, that does not seem to scale horizontally.
>
>>
>> ACPI has acpi_target_system_state() for this very purpose, for example.
>>
>>> If this seems acceptable to you along with proper documentation to
>>> illustrate the platform specific meaning of these states, I will got
>>> ahead and cook a patch.
>>
>> I wouldn't like platform-specific things to pretend that they are generic.
>
> Would a notifier model be more appropriate perhaps? The mechanism by
> which the notifications get registered to and signaled can be made
> generic, the exact information however would be inherently
> platform_suspend_ops specific, and only the relevant drivers that need
> to subscribe to that kind of information would do that.

Please see the message I've just sent in this thread. :-)

Thanks,
Rafael

[RFC 0/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

This patch series implements the idea discussed in this thread:

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

The last patch is relative to the pending submission of the Broadcom STB
S2/S3/S5 suspend/resume code that can be found below, and is provided
as an example of how this can be useful.

https://lkml.org/lkml/2017/6/16/737

Florian Fainelli (2):
  PM / suspend: Add platform_suspend_target_state()
  soc: bcm: brcmstb: PM: Implement target_state callback

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
 include/linux/suspend.h             | 12 ++++++++++++
 kernel/power/suspend.c              | 15 +++++++++++++++
 3 files changed, 42 insertions(+)

-- 
2.9.3

[RFC 0/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

This patch series implements the idea discussed in this thread:

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

The last patch is relative to the pending submission of the Broadcom STB
S2/S3/S5 suspend/resume code that can be found below, and is provided
as an example of how this can be useful.

https://lkml.org/lkml/2017/6/16/737

Florian Fainelli (2):
  PM / suspend: Add platform_suspend_target_state()
  soc: bcm: brcmstb: PM: Implement target_state callback

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
 include/linux/suspend.h             | 12 ++++++++++++
 kernel/power/suspend.c              | 15 +++++++++++++++
 3 files changed, 42 insertions(+)

-- 
2.9.3

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

Add an optional platform_suspend_ops callback: target_state, and a
helper function globally visible to get this called:
platform_suspend_target_state().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Although this callback is optional and documented as such, it requires
a platform_suspend_ops::begin callback to be implemented in order to
provide an accurate suspend/resume state within the driver that
implements this platform_suspend_ops.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/suspend.h | 12 ++++++++++++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index d9718378a8be..d998a04a90a2 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
  *	Called by the PM core if the suspending of devices fails.
  *	This callback is optional and should only be implemented by platforms
  *	which require special recovery actions in that situation.
+ *
+ * @target_state: Returns the suspend state the suspend_ops will be entering.
+ * 	Called by device drivers that need to know the platform specific suspend
+ * 	state the system is about to enter.
+ * 	This callback is optional and should only be implemented by platforms
+ * 	which require special handling of power management states within
+ * 	drivers. It does require @begin to be implemented to provide the suspend
+ * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
+ * 	mapping to suspend_state_t when relevant.
  */
 struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
@@ -184,6 +193,7 @@ struct platform_suspend_ops {
 	bool (*suspend_again)(void);
 	void (*end)(void);
 	void (*recover)(void);
+	int (*target_state)(void);
 };
 
 struct platform_freeze_ops {
@@ -200,6 +210,7 @@ struct platform_freeze_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern int platform_suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -279,6 +290,7 @@ static inline bool pm_resume_via_firmware(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline int platform_suspend_target_state(void) { return -ENOSYS; }
 static inline bool idle_should_freeze(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 15e6baef5c73..d31fe7a08f4a 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -177,6 +177,21 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 /**
+ * platform_suspend_target_state - Return the platform specific suspend state.
+ * a begin() callback is necessary in order to fill this information correctly
+ * for callers.
+ */
+int platform_suspend_target_state(void)
+{
+	if (!suspend_ops || !suspend_ops->target_state ||
+	    (suspend_ops->target_state && !suspend_ops->begin))
+		return -ENOTSUPP;
+
+	return suspend_ops->target_state();
+}
+EXPORT_SYMBOL_GPL(platform_suspend_target_state);
+
+/**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
  * Platform drivers that implement mem suspend only and only need to check for
-- 
2.9.3

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

Add an optional platform_suspend_ops callback: target_state, and a
helper function globally visible to get this called:
platform_suspend_target_state().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Although this callback is optional and documented as such, it requires
a platform_suspend_ops::begin callback to be implemented in order to
provide an accurate suspend/resume state within the driver that
implements this platform_suspend_ops.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/suspend.h | 12 ++++++++++++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index d9718378a8be..d998a04a90a2 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
  *	Called by the PM core if the suspending of devices fails.
  *	This callback is optional and should only be implemented by platforms
  *	which require special recovery actions in that situation.
+ *
+ * @target_state: Returns the suspend state the suspend_ops will be entering.
+ * 	Called by device drivers that need to know the platform specific suspend
+ * 	state the system is about to enter.
+ * 	This callback is optional and should only be implemented by platforms
+ * 	which require special handling of power management states within
+ * 	drivers. It does require @begin to be implemented to provide the suspend
+ * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
+ * 	mapping to suspend_state_t when relevant.
  */
 struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
@@ -184,6 +193,7 @@ struct platform_suspend_ops {
 	bool (*suspend_again)(void);
 	void (*end)(void);
 	void (*recover)(void);
+	int (*target_state)(void);
 };
 
 struct platform_freeze_ops {
@@ -200,6 +210,7 @@ struct platform_freeze_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern int platform_suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -279,6 +290,7 @@ static inline bool pm_resume_via_firmware(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline int platform_suspend_target_state(void) { return -ENOSYS; }
 static inline bool idle_should_freeze(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 15e6baef5c73..d31fe7a08f4a 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -177,6 +177,21 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 /**
+ * platform_suspend_target_state - Return the platform specific suspend state.
+ * a begin() callback is necessary in order to fill this information correctly
+ * for callers.
+ */
+int platform_suspend_target_state(void)
+{
+	if (!suspend_ops || !suspend_ops->target_state ||
+	    (suspend_ops->target_state && !suspend_ops->begin))
+		return -ENOTSUPP;
+
+	return suspend_ops->target_state();
+}
+EXPORT_SYMBOL_GPL(platform_suspend_target_state);
+
+/**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
  * Platform drivers that implement mem suspend only and only need to check for
-- 
2.9.3

[RFC 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Florian Fainelli]

Provide a target_state callback implementation which just returns the
suspend_state_t the system is about to enter. Broadcom STB drivers can
utilize platform_suspend_target_state() to retrieve that and take
appropriate actions (e.g: full vs. partial re-initialization).

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index 4b7e6c297b23..7d4695734093 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -104,6 +104,7 @@ struct brcmstb_pm_control {
 	u32 phy_b_standby_ctrl_offs;
 	bool needs_ddr_pad;
 	struct platform_device *pdev;
+	suspend_state_t pm_state;
 };
 
 enum bsp_initiate_command {
@@ -547,9 +548,23 @@ static int brcmstb_pm_valid(suspend_state_t state)
 	}
 }
 
+static int brcmstb_pm_begin(suspend_state_t state)
+{
+	ctrl.pm_state = state;
+
+	return 0;
+}
+
+static int brcmstb_target_state(void)
+{
+	return ctrl.pm_state;
+}
+
 static const struct platform_suspend_ops brcmstb_pm_ops = {
+	.begin		= brcmstb_pm_begin,
 	.enter		= brcmstb_pm_enter,
 	.valid		= brcmstb_pm_valid,
+	.target_state	= brcmstb_target_state,
 };
 
 static const struct of_device_id aon_ctrl_dt_ids[] = {
-- 
2.9.3

[RFC 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Florian Fainelli]

Provide a target_state callback implementation which just returns the
suspend_state_t the system is about to enter. Broadcom STB drivers can
utilize platform_suspend_target_state() to retrieve that and take
appropriate actions (e.g: full vs. partial re-initialization).

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index 4b7e6c297b23..7d4695734093 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -104,6 +104,7 @@ struct brcmstb_pm_control {
 	u32 phy_b_standby_ctrl_offs;
 	bool needs_ddr_pad;
 	struct platform_device *pdev;
+	suspend_state_t pm_state;
 };
 
 enum bsp_initiate_command {
@@ -547,9 +548,23 @@ static int brcmstb_pm_valid(suspend_state_t state)
 	}
 }
 
+static int brcmstb_pm_begin(suspend_state_t state)
+{
+	ctrl.pm_state = state;
+
+	return 0;
+}
+
+static int brcmstb_target_state(void)
+{
+	return ctrl.pm_state;
+}
+
 static const struct platform_suspend_ops brcmstb_pm_ops = {
+	.begin		= brcmstb_pm_begin,
 	.enter		= brcmstb_pm_enter,
 	.valid		= brcmstb_pm_valid,
+	.target_state	= brcmstb_target_state,
 };
 
 static const struct of_device_id aon_ctrl_dt_ids[] = {
-- 
2.9.3

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> Add an optional platform_suspend_ops callback: target_state, and a
> helper function globally visible to get this called:
> platform_suspend_target_state().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Although this callback is optional and documented as such, it requires
> a platform_suspend_ops::begin callback to be implemented in order to
> provide an accurate suspend/resume state within the driver that
> implements this platform_suspend_ops.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/linux/suspend.h | 12 ++++++++++++
>  kernel/power/suspend.c  | 15 +++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index d9718378a8be..d998a04a90a2 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
>   *	Called by the PM core if the suspending of devices fails.
>   *	This callback is optional and should only be implemented by platforms
>   *	which require special recovery actions in that situation.
> + *
> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> + * 	Called by device drivers that need to know the platform specific suspend
> + * 	state the system is about to enter.
> + * 	This callback is optional and should only be implemented by platforms
> + * 	which require special handling of power management states within
> + * 	drivers. It does require @begin to be implemented to provide the suspend
> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> + * 	mapping to suspend_state_t when relevant.
>   */
>  struct platform_suspend_ops {
>  	int (*valid)(suspend_state_t state);
> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
>  	bool (*suspend_again)(void);
>  	void (*end)(void);
>  	void (*recover)(void);
> +	int (*target_state)(void);

I would use unsigned int (the sign should not matter).

>  };

That's almost what I was thinking about except that the values returned by
->target_state should be unique, so it would be good to do something to
ensure that.

The concern is as follows.

Say you have a driver develped for platform X where ->target_state returns
A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
returning B for "mem" and C for "standby" and now the driver cannot
distinguish between them.

Moreover, even if they both returned A for "mem" there might be differences
in how "mem" was defined by each of them and therefore in what the driver was
expected to do to handle "mem" on X and Y.

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> Add an optional platform_suspend_ops callback: target_state, and a
> helper function globally visible to get this called:
> platform_suspend_target_state().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Although this callback is optional and documented as such, it requires
> a platform_suspend_ops::begin callback to be implemented in order to
> provide an accurate suspend/resume state within the driver that
> implements this platform_suspend_ops.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/linux/suspend.h | 12 ++++++++++++
>  kernel/power/suspend.c  | 15 +++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index d9718378a8be..d998a04a90a2 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
>   *	Called by the PM core if the suspending of devices fails.
>   *	This callback is optional and should only be implemented by platforms
>   *	which require special recovery actions in that situation.
> + *
> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> + * 	Called by device drivers that need to know the platform specific suspend
> + * 	state the system is about to enter.
> + * 	This callback is optional and should only be implemented by platforms
> + * 	which require special handling of power management states within
> + * 	drivers. It does require @begin to be implemented to provide the suspend
> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> + * 	mapping to suspend_state_t when relevant.
>   */
>  struct platform_suspend_ops {
>  	int (*valid)(suspend_state_t state);
> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
>  	bool (*suspend_again)(void);
>  	void (*end)(void);
>  	void (*recover)(void);
> +	int (*target_state)(void);

I would use unsigned int (the sign should not matter).

>  };

That's almost what I was thinking about except that the values returned by
->target_state should be unique, so it would be good to do something to
ensure that.

The concern is as follows.

Say you have a driver develped for platform X where ->target_state returns
A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
returning B for "mem" and C for "standby" and now the driver cannot
distinguish between them.

Moreover, even if they both returned A for "mem" there might be differences
in how "mem" was defined by each of them and therefore in what the driver was
expected to do to handle "mem" on X and Y.

Thanks,
Rafael

Re: [RFC 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Rafael J. Wysocki]

On Thursday, June 22, 2017 06:08:37 PM Florian Fainelli wrote:
> Provide a target_state callback implementation which just returns the
> suspend_state_t the system is about to enter. Broadcom STB drivers can
> utilize platform_suspend_target_state() to retrieve that and take
> appropriate actions (e.g: full vs. partial re-initialization).
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> index 4b7e6c297b23..7d4695734093 100644
> --- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> +++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> @@ -104,6 +104,7 @@ struct brcmstb_pm_control {
>  	u32 phy_b_standby_ctrl_offs;
>  	bool needs_ddr_pad;
>  	struct platform_device *pdev;
> +	suspend_state_t pm_state;

I wouldn't use suspend_state_t here, because the mapping between those
things and real platform power states is somewhat arbitrary and totally
platform-specific.

It's better to define symbols representing platform power states for your
platform (or use an enum) and then use those symbols in the drivers IMO.

Thanks,
Rafael

[RFC 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Rafael J. Wysocki]

On Thursday, June 22, 2017 06:08:37 PM Florian Fainelli wrote:
> Provide a target_state callback implementation which just returns the
> suspend_state_t the system is about to enter. Broadcom STB drivers can
> utilize platform_suspend_target_state() to retrieve that and take
> appropriate actions (e.g: full vs. partial re-initialization).
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> index 4b7e6c297b23..7d4695734093 100644
> --- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> +++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> @@ -104,6 +104,7 @@ struct brcmstb_pm_control {
>  	u32 phy_b_standby_ctrl_offs;
>  	bool needs_ddr_pad;
>  	struct platform_device *pdev;
> +	suspend_state_t pm_state;

I wouldn't use suspend_state_t here, because the mapping between those
things and real platform power states is somewhat arbitrary and totally
platform-specific.

It's better to define symbols representing platform power states for your
platform (or use an enum) and then use those symbols in the drivers IMO.

Thanks,
Rafael

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

On Sun 2017-07-16 01:29:57, Rafael J. Wysocki wrote:
> On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote:
> > Hi!
> > 
> > > > > I had an idea of using an enum type encompassing all of the power states
> > > > > defined for various platforms and serving both as a registry (to ensure the
> > > > > uniqueness of the values assigned to the states) and a common ground
> > > > > between platforms and drivers.
> > > > > 
> > > > > Something like:
> > > > > 
> > > > > enum platform_target_state {
> > > > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > > > 	PLATFORM_STATE_WORKING = 0,
> > > > > 	PLATFORM_STATE_ACPI_S1,
> > > > > 	PLATFORM_STATE_ACPI_S2,
> > > > > 	PLATFORM_STATE_ACPI_S3,
> > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > > > 	...
> > > > > };
> > > > > 
> > > > > and define ->target_state to return a value of this type.
> > > > > 
> > > > > Then, if a driver sees one of these and recognizes that value, it should
> > > > > know exactly what to do.
> > > > 
> > > > Remind me why this is good idea?
> > > 
> > > Because there are drivers that need to do specific things during
> > > suspend on a specific board when it goes into a specific state as a
> > > whole.
> > 
> > We have seen driver that cares about voltage to his device being
> > lost. That's reasonable.
> > 
> > Inquiring what the platform target state is... is not.
> 
> So why exactly isn't it reasonable?
> 
> Please use technical arguments.  Saying that something is wrong without
> explaining the problem you see with it isn't particulatly useful in technical
> discussions.

Deep in your heart, you should know that having enum listing all the platforms linux
runs on is a very bad idea.

Anyway, there are better solutions, regulator framework already knows if given rail
will be powered off or not, and their driver already knows if they are going
suspend/standby. They just need to use existing interfaces.

										Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

On Sun 2017-07-16 01:29:57, Rafael J. Wysocki wrote:
> On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote:
> > Hi!
> > 
> > > > > I had an idea of using an enum type encompassing all of the power states
> > > > > defined for various platforms and serving both as a registry (to ensure the
> > > > > uniqueness of the values assigned to the states) and a common ground
> > > > > between platforms and drivers.
> > > > > 
> > > > > Something like:
> > > > > 
> > > > > enum platform_target_state {
> > > > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > > > 	PLATFORM_STATE_WORKING = 0,
> > > > > 	PLATFORM_STATE_ACPI_S1,
> > > > > 	PLATFORM_STATE_ACPI_S2,
> > > > > 	PLATFORM_STATE_ACPI_S3,
> > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > > > 	...
> > > > > };
> > > > > 
> > > > > and define ->target_state to return a value of this type.
> > > > > 
> > > > > Then, if a driver sees one of these and recognizes that value, it should
> > > > > know exactly what to do.
> > > > 
> > > > Remind me why this is good idea?
> > > 
> > > Because there are drivers that need to do specific things during
> > > suspend on a specific board when it goes into a specific state as a
> > > whole.
> > 
> > We have seen driver that cares about voltage to his device being
> > lost. That's reasonable.
> > 
> > Inquiring what the platform target state is... is not.
> 
> So why exactly isn't it reasonable?
> 
> Please use technical arguments.  Saying that something is wrong without
> explaining the problem you see with it isn't particulatly useful in technical
> discussions.

Deep in your heart, you should know that having enum listing all the platforms linux
runs on is a very bad idea.

Anyway, there are better solutions, regulator framework already knows if given rail
will be powered off or not, and their driver already knows if they are going
suspend/standby. They just need to use existing interfaces.

										Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote:
> On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
> > > We already have
> > > 
> > > struct regulator_state {
> > >        int uV; /* suspend voltage */
> > >        unsigned int mode; /* suspend regulator operating mode */
> > >        int enabled; /* is regulator enabled in this suspend state */
> > >        int disabled; /* is the regulator disabled in this suspend state */
> > > };
> > > 
> > >  * struct regulation_constraints - regulator operating constraints.
> > >   * @state_disk: State for regulator when system is suspended in disk
> > >   * mode.
> > >   * @state_mem: State for regulator when system is suspended in mem
> > >   * mode.
> > >   * @state_standby: State for regulator when system is suspended in
> > >   * standby
> > >   *                 mode.
> > >    
> > > . So it seems that maybe we should tell the drivers if we are entering
> > > "state_mem" or "state_standby" (something I may have opposed, sorry),
> > > then the driver can get neccessary information from regulator
> > > framework.
> > 
> > OK, so what would be the mechanism to tell these drivers about the
> > system wide suspend state they are entering if it is not via
> > platform_suspend_target_state()?
> > 
> > Keep in mind that regulators might be one aspect of what could be
> > causing the platform to behave specifically in one suspend state vs.
> > another, but there could be pieces of HW within the SoC that can't be
> > described with power domains, voltage islands etc. that would still have
> > inherent suspend states properties (like memory retention, pin/pad
> > controls etc. etc). We still need some mechanism, possibly centralized
> > 
> 
> I concur, the regulator stuff is one aspect of one of our suspend state
> (cutting VDDcore). But we have another state where the main clock (going
> to the IPs) is going from a few hundred MHz to 32kHz. This is currently
> handled by calling at91_suspend_entering_slow_clock(). I think it is
> important to take that into account so we can remove this hack from the
> kernel.

Cure should not be worse then the disease... and it is in this case.

For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended"
as regulator framework had. If not, implement it.

Same with memory retention, pin/pad controls.

									Pavel

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote:
> On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
> > > We already have
> > > 
> > > struct regulator_state {
> > >        int uV; /* suspend voltage */
> > >        unsigned int mode; /* suspend regulator operating mode */
> > >        int enabled; /* is regulator enabled in this suspend state */
> > >        int disabled; /* is the regulator disabled in this suspend state */
> > > };
> > > 
> > >  * struct regulation_constraints - regulator operating constraints.
> > >   * @state_disk: State for regulator when system is suspended in disk
> > >   * mode.
> > >   * @state_mem: State for regulator when system is suspended in mem
> > >   * mode.
> > >   * @state_standby: State for regulator when system is suspended in
> > >   * standby
> > >   *                 mode.
> > >    
> > > . So it seems that maybe we should tell the drivers if we are entering
> > > "state_mem" or "state_standby" (something I may have opposed, sorry),
> > > then the driver can get neccessary information from regulator
> > > framework.
> > 
> > OK, so what would be the mechanism to tell these drivers about the
> > system wide suspend state they are entering if it is not via
> > platform_suspend_target_state()?
> > 
> > Keep in mind that regulators might be one aspect of what could be
> > causing the platform to behave specifically in one suspend state vs.
> > another, but there could be pieces of HW within the SoC that can't be
> > described with power domains, voltage islands etc. that would still have
> > inherent suspend states properties (like memory retention, pin/pad
> > controls etc. etc). We still need some mechanism, possibly centralized
> > 
> 
> I concur, the regulator stuff is one aspect of one of our suspend state
> (cutting VDDcore). But we have another state where the main clock (going
> to the IPs) is going from a few hundred MHz to 32kHz. This is currently
> handled by calling at91_suspend_entering_slow_clock(). I think it is
> important to take that into account so we can remove this hack from the
> kernel.

Cure should not be worse then the disease... and it is in this case.

For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended"
as regulator framework had. If not, implement it.

Same with memory retention, pin/pad controls.

									Pavel

Re: [PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

Hi!

> Although this callback is optional and documented as such, it requires
> a platform_suspend_ops::begin callback to be implemented in order to
> provide an accurate suspend/resume state within the driver that
> implements this platform_suspend_ops.
> 
> An enumeration: platform_target_state is defined which currently defines
> the standard ACPI_S[1-4] states and can be extended with platform
> specific suspend states.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/linux/suspend.h | 25 +++++++++++++++++++++++++
>  kernel/power/suspend.c  | 15 +++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 0b1cf32edfd7..6e6cc0778816 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -50,6 +50,16 @@ enum suspend_stat_step {
>  	SUSPEND_RESUME
>  };
>  
> +enum platform_target_state {
> +	PLATFORM_STATE_UNKNOWN = -1,
> +	PLATFORM_STATE_WORKING = 0,
> +	PLATFORM_STATE_ACPI_S1,
> +	PLATFORM_STATE_ACPI_S2,
> +	PLATFORM_STATE_ACPI_S3,
> +	PLATFORM_STATE_ACPI_S4,
> +	/* Add platform specific states here */
> +};
> +

As I tried to explain in the email thread, having list with all the possible platform
states is no-go. We have about 1000 platforms supported...

NAK.
									Pavel

[PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

Hi!

> Although this callback is optional and documented as such, it requires
> a platform_suspend_ops::begin callback to be implemented in order to
> provide an accurate suspend/resume state within the driver that
> implements this platform_suspend_ops.
> 
> An enumeration: platform_target_state is defined which currently defines
> the standard ACPI_S[1-4] states and can be extended with platform
> specific suspend states.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/linux/suspend.h | 25 +++++++++++++++++++++++++
>  kernel/power/suspend.c  | 15 +++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 0b1cf32edfd7..6e6cc0778816 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -50,6 +50,16 @@ enum suspend_stat_step {
>  	SUSPEND_RESUME
>  };
>  
> +enum platform_target_state {
> +	PLATFORM_STATE_UNKNOWN = -1,
> +	PLATFORM_STATE_WORKING = 0,
> +	PLATFORM_STATE_ACPI_S1,
> +	PLATFORM_STATE_ACPI_S2,
> +	PLATFORM_STATE_ACPI_S3,
> +	PLATFORM_STATE_ACPI_S4,
> +	/* Add platform specific states here */
> +};
> +

As I tried to explain in the email thread, having list with all the possible platform
states is no-go. We have about 1000 platforms supported...

NAK.
									Pavel

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
>> Add an optional platform_suspend_ops callback: target_state, and a
>> helper function globally visible to get this called:
>> platform_suspend_target_state().
>>
>> This is useful for platform specific drivers that may need to take a
>> slightly different suspend/resume path based on the system's
>> suspend/resume state being entered.
>>
>> Although this callback is optional and documented as such, it requires
>> a platform_suspend_ops::begin callback to be implemented in order to
>> provide an accurate suspend/resume state within the driver that
>> implements this platform_suspend_ops.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/linux/suspend.h | 12 ++++++++++++
>>  kernel/power/suspend.c  | 15 +++++++++++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index d9718378a8be..d998a04a90a2 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
>>   *	Called by the PM core if the suspending of devices fails.
>>   *	This callback is optional and should only be implemented by platforms
>>   *	which require special recovery actions in that situation.
>> + *
>> + * @target_state: Returns the suspend state the suspend_ops will be entering.
>> + * 	Called by device drivers that need to know the platform specific suspend
>> + * 	state the system is about to enter.
>> + * 	This callback is optional and should only be implemented by platforms
>> + * 	which require special handling of power management states within
>> + * 	drivers. It does require @begin to be implemented to provide the suspend
>> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
>> + * 	mapping to suspend_state_t when relevant.
>>   */
>>  struct platform_suspend_ops {
>>  	int (*valid)(suspend_state_t state);
>> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
>>  	bool (*suspend_again)(void);
>>  	void (*end)(void);
>>  	void (*recover)(void);
>> +	int (*target_state)(void);
> 
> I would use unsigned int (the sign should not matter).
> 
>>  };
> 
> That's almost what I was thinking about except that the values returned by
> ->target_state should be unique, so it would be good to do something to
> ensure that.
> 
> The concern is as follows.
> 
> Say you have a driver develped for platform X where ->target_state returns
> A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> returning B for "mem" and C for "standby" and now the driver cannot
> distinguish between them.
> 
> Moreover, even if they both returned A for "mem" there might be differences
> in how "mem" was defined by each of them and therefore in what the driver was
> expected to do to handle "mem" on X and Y.

That makes sense, would you need the core implementation in
platform_suspend_target_state() to range check what
suspend_ops->target_state() returns against a set of reserved value say,
checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
would like to see being used?

Thanks!
-- 
Florian

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
>> Add an optional platform_suspend_ops callback: target_state, and a
>> helper function globally visible to get this called:
>> platform_suspend_target_state().
>>
>> This is useful for platform specific drivers that may need to take a
>> slightly different suspend/resume path based on the system's
>> suspend/resume state being entered.
>>
>> Although this callback is optional and documented as such, it requires
>> a platform_suspend_ops::begin callback to be implemented in order to
>> provide an accurate suspend/resume state within the driver that
>> implements this platform_suspend_ops.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/linux/suspend.h | 12 ++++++++++++
>>  kernel/power/suspend.c  | 15 +++++++++++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index d9718378a8be..d998a04a90a2 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
>>   *	Called by the PM core if the suspending of devices fails.
>>   *	This callback is optional and should only be implemented by platforms
>>   *	which require special recovery actions in that situation.
>> + *
>> + * @target_state: Returns the suspend state the suspend_ops will be entering.
>> + * 	Called by device drivers that need to know the platform specific suspend
>> + * 	state the system is about to enter.
>> + * 	This callback is optional and should only be implemented by platforms
>> + * 	which require special handling of power management states within
>> + * 	drivers. It does require @begin to be implemented to provide the suspend
>> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
>> + * 	mapping to suspend_state_t when relevant.
>>   */
>>  struct platform_suspend_ops {
>>  	int (*valid)(suspend_state_t state);
>> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
>>  	bool (*suspend_again)(void);
>>  	void (*end)(void);
>>  	void (*recover)(void);
>> +	int (*target_state)(void);
> 
> I would use unsigned int (the sign should not matter).
> 
>>  };
> 
> That's almost what I was thinking about except that the values returned by
> ->target_state should be unique, so it would be good to do something to
> ensure that.
> 
> The concern is as follows.
> 
> Say you have a driver develped for platform X where ->target_state returns
> A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> returning B for "mem" and C for "standby" and now the driver cannot
> distinguish between them.
> 
> Moreover, even if they both returned A for "mem" there might be differences
> in how "mem" was defined by each of them and therefore in what the driver was
> expected to do to handle "mem" on X and Y.

That makes sense, would you need the core implementation in
platform_suspend_target_state() to range check what
suspend_ops->target_state() returns against a set of reserved value say,
checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
would like to see being used?

Thanks!
-- 
Florian

Re: Drivers taking different actions depending on sleep state

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

On Thu 2017-06-22 10:51:02, Alexandre Belloni wrote:
> On 21/06/2017 at 16:55:04 -0700, Florian Fainelli wrote:
> > > That is conceivable, but again, the meaning of STANDBY and MEM is
> > > platform-specific.  Actions to be taken for, say, STANDBY, may differ
> > > from one platform to another.
> > 
> > True, though I would only expect drivers for that particular platform to
> > care about that information and these drivers would only make sense on
> > that particular platform so the meaning of STANDBY and MEM would be
> > clear for those drivers. The intent is really to keep the "distributed"
> > model where individual drivers manage their particular piece of HW,
> > while utilizing a global piece of information that is platform specific.
> > 
> > If this seems acceptable to you along with proper documentation to
> > illustrate the platform specific meaning of these states, I will got
> > ahead and cook a patch.
> 
> Well, that won't work for us. We need need two kind of information:
>  - whether main clock is switched from the master clock to the slow
>    clock
>  - whether VDDcore will be cut
> 
> for the first one, we already have an hackish callback:
> at91_suspend_entering_slow_clock() that will call from the platform
> specific drivers.

Sounds like you need another "will_vddcore_be_cut?()" callback, not
"int platform_suspend_target_state(void)".

And actually I'd hope you have some kind of regulator, so that "will
this regulator be available over suspend" question can be asked?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Drivers taking different actions depending on sleep state

[Pavel Machek]

On Thu 2017-06-22 10:51:02, Alexandre Belloni wrote:
> On 21/06/2017 at 16:55:04 -0700, Florian Fainelli wrote:
> > > That is conceivable, but again, the meaning of STANDBY and MEM is
> > > platform-specific.  Actions to be taken for, say, STANDBY, may differ
> > > from one platform to another.
> > 
> > True, though I would only expect drivers for that particular platform to
> > care about that information and these drivers would only make sense on
> > that particular platform so the meaning of STANDBY and MEM would be
> > clear for those drivers. The intent is really to keep the "distributed"
> > model where individual drivers manage their particular piece of HW,
> > while utilizing a global piece of information that is platform specific.
> > 
> > If this seems acceptable to you along with proper documentation to
> > illustrate the platform specific meaning of these states, I will got
> > ahead and cook a patch.
> 
> Well, that won't work for us. We need need two kind of information:
>  - whether main clock is switched from the master clock to the slow
>    clock
>  - whether VDDcore will be cut
> 
> for the first one, we already have an hackish callback:
> at91_suspend_entering_slow_clock() that will call from the platform
> specific drivers.

Sounds like you need another "will_vddcore_be_cut?()" callback, not
"int platform_suspend_target_state(void)".

And actually I'd hope you have some kind of regulator, so that "will
this regulator be available over suspend" question can be asked?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170713/a0aff987/attachment.sig>

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote:
> On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> >> Add an optional platform_suspend_ops callback: target_state, and a
> >> helper function globally visible to get this called:
> >> platform_suspend_target_state().
> >>
> >> This is useful for platform specific drivers that may need to take a
> >> slightly different suspend/resume path based on the system's
> >> suspend/resume state being entered.
> >>
> >> Although this callback is optional and documented as such, it requires
> >> a platform_suspend_ops::begin callback to be implemented in order to
> >> provide an accurate suspend/resume state within the driver that
> >> implements this platform_suspend_ops.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  include/linux/suspend.h | 12 ++++++++++++
> >>  kernel/power/suspend.c  | 15 +++++++++++++++
> >>  2 files changed, 27 insertions(+)
> >>
> >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> >> index d9718378a8be..d998a04a90a2 100644
> >> --- a/include/linux/suspend.h
> >> +++ b/include/linux/suspend.h
> >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
> >>   *	Called by the PM core if the suspending of devices fails.
> >>   *	This callback is optional and should only be implemented by platforms
> >>   *	which require special recovery actions in that situation.
> >> + *
> >> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> >> + * 	Called by device drivers that need to know the platform specific suspend
> >> + * 	state the system is about to enter.
> >> + * 	This callback is optional and should only be implemented by platforms
> >> + * 	which require special handling of power management states within
> >> + * 	drivers. It does require @begin to be implemented to provide the suspend
> >> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> >> + * 	mapping to suspend_state_t when relevant.
> >>   */
> >>  struct platform_suspend_ops {
> >>  	int (*valid)(suspend_state_t state);
> >> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
> >>  	bool (*suspend_again)(void);
> >>  	void (*end)(void);
> >>  	void (*recover)(void);
> >> +	int (*target_state)(void);
> > 
> > I would use unsigned int (the sign should not matter).
> > 
> >>  };
> > 
> > That's almost what I was thinking about except that the values returned by
> > ->target_state should be unique, so it would be good to do something to
> > ensure that.
> > 
> > The concern is as follows.
> > 
> > Say you have a driver develped for platform X where ->target_state returns
> > A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> > returning B for "mem" and C for "standby" and now the driver cannot
> > distinguish between them.
> > 
> > Moreover, even if they both returned A for "mem" there might be differences
> > in how "mem" was defined by each of them and therefore in what the driver was
> > expected to do to handle "mem" on X and Y.
> 
> That makes sense, would you need the core implementation in
> platform_suspend_target_state() to range check what
> suspend_ops->target_state() returns against a set of reserved value say,
> checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
> would like to see being used?

I had an idea of using an enum type encompassing all of the power states
defined for various platforms and serving both as a registry (to ensure the
uniqueness of the values assigned to the states) and a common ground
between platforms and drivers.

Something like:

enum platform_target_state {
	PLATFORM_STATE_UNKNOWN = -1,
	PLATFORM_STATE_WORKING = 0,
	PLATFORM_STATE_ACPI_S1,
	PLATFORM_STATE_ACPI_S2,
	PLATFORM_STATE_ACPI_S3,
	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
	...
};

and define ->target_state to return a value of this type.

Then, if a driver sees one of these and recognizes that value, it should
know exactly what to do.

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote:
> On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> >> Add an optional platform_suspend_ops callback: target_state, and a
> >> helper function globally visible to get this called:
> >> platform_suspend_target_state().
> >>
> >> This is useful for platform specific drivers that may need to take a
> >> slightly different suspend/resume path based on the system's
> >> suspend/resume state being entered.
> >>
> >> Although this callback is optional and documented as such, it requires
> >> a platform_suspend_ops::begin callback to be implemented in order to
> >> provide an accurate suspend/resume state within the driver that
> >> implements this platform_suspend_ops.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  include/linux/suspend.h | 12 ++++++++++++
> >>  kernel/power/suspend.c  | 15 +++++++++++++++
> >>  2 files changed, 27 insertions(+)
> >>
> >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> >> index d9718378a8be..d998a04a90a2 100644
> >> --- a/include/linux/suspend.h
> >> +++ b/include/linux/suspend.h
> >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
> >>   *	Called by the PM core if the suspending of devices fails.
> >>   *	This callback is optional and should only be implemented by platforms
> >>   *	which require special recovery actions in that situation.
> >> + *
> >> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> >> + * 	Called by device drivers that need to know the platform specific suspend
> >> + * 	state the system is about to enter.
> >> + * 	This callback is optional and should only be implemented by platforms
> >> + * 	which require special handling of power management states within
> >> + * 	drivers. It does require @begin to be implemented to provide the suspend
> >> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> >> + * 	mapping to suspend_state_t when relevant.
> >>   */
> >>  struct platform_suspend_ops {
> >>  	int (*valid)(suspend_state_t state);
> >> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
> >>  	bool (*suspend_again)(void);
> >>  	void (*end)(void);
> >>  	void (*recover)(void);
> >> +	int (*target_state)(void);
> > 
> > I would use unsigned int (the sign should not matter).
> > 
> >>  };
> > 
> > That's almost what I was thinking about except that the values returned by
> > ->target_state should be unique, so it would be good to do something to
> > ensure that.
> > 
> > The concern is as follows.
> > 
> > Say you have a driver develped for platform X where ->target_state returns
> > A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> > returning B for "mem" and C for "standby" and now the driver cannot
> > distinguish between them.
> > 
> > Moreover, even if they both returned A for "mem" there might be differences
> > in how "mem" was defined by each of them and therefore in what the driver was
> > expected to do to handle "mem" on X and Y.
> 
> That makes sense, would you need the core implementation in
> platform_suspend_target_state() to range check what
> suspend_ops->target_state() returns against a set of reserved value say,
> checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
> would like to see being used?

I had an idea of using an enum type encompassing all of the power states
defined for various platforms and serving both as a registry (to ensure the
uniqueness of the values assigned to the states) and a common ground
between platforms and drivers.

Something like:

enum platform_target_state {
	PLATFORM_STATE_UNKNOWN = -1,
	PLATFORM_STATE_WORKING = 0,
	PLATFORM_STATE_ACPI_S1,
	PLATFORM_STATE_ACPI_S2,
	PLATFORM_STATE_ACPI_S3,
	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
	...
};

and define ->target_state to return a value of this type.

Then, if a driver sees one of these and recognizes that value, it should
know exactly what to do.

Thanks,
Rafael

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4893 bytes --]

On Sat 2017-07-15 00:16:16, Rafael J. Wysocki wrote:
> On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote:
> > On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> > > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> > >> Add an optional platform_suspend_ops callback: target_state, and a
> > >> helper function globally visible to get this called:
> > >> platform_suspend_target_state().
> > >>
> > >> This is useful for platform specific drivers that may need to take a
> > >> slightly different suspend/resume path based on the system's
> > >> suspend/resume state being entered.
> > >>
> > >> Although this callback is optional and documented as such, it requires
> > >> a platform_suspend_ops::begin callback to be implemented in order to
> > >> provide an accurate suspend/resume state within the driver that
> > >> implements this platform_suspend_ops.
> > >>
> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > >> ---
> > >>  include/linux/suspend.h | 12 ++++++++++++
> > >>  kernel/power/suspend.c  | 15 +++++++++++++++
> > >>  2 files changed, 27 insertions(+)
> > >>
> > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > >> index d9718378a8be..d998a04a90a2 100644
> > >> --- a/include/linux/suspend.h
> > >> +++ b/include/linux/suspend.h
> > >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
> > >>   *	Called by the PM core if the suspending of devices fails.
> > >>   *	This callback is optional and should only be implemented by platforms
> > >>   *	which require special recovery actions in that situation.
> > >> + *
> > >> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> > >> + * 	Called by device drivers that need to know the platform specific suspend
> > >> + * 	state the system is about to enter.
> > >> + * 	This callback is optional and should only be implemented by platforms
> > >> + * 	which require special handling of power management states within
> > >> + * 	drivers. It does require @begin to be implemented to provide the suspend
> > >> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> > >> + * 	mapping to suspend_state_t when relevant.
> > >>   */
> > >>  struct platform_suspend_ops {
> > >>  	int (*valid)(suspend_state_t state);
> > >> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
> > >>  	bool (*suspend_again)(void);
> > >>  	void (*end)(void);
> > >>  	void (*recover)(void);
> > >> +	int (*target_state)(void);
> > > 
> > > I would use unsigned int (the sign should not matter).
> > > 
> > >>  };
> > > 
> > > That's almost what I was thinking about except that the values returned by
> > > ->target_state should be unique, so it would be good to do something to
> > > ensure that.
> > > 
> > > The concern is as follows.
> > > 
> > > Say you have a driver develped for platform X where ->target_state returns
> > > A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> > > returning B for "mem" and C for "standby" and now the driver cannot
> > > distinguish between them.
> > > 
> > > Moreover, even if they both returned A for "mem" there might be differences
> > > in how "mem" was defined by each of them and therefore in what the driver was
> > > expected to do to handle "mem" on X and Y.
> > 
> > That makes sense, would you need the core implementation in
> > platform_suspend_target_state() to range check what
> > suspend_ops->target_state() returns against a set of reserved value say,
> > checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
> > would like to see being used?
> 
> I had an idea of using an enum type encompassing all of the power states
> defined for various platforms and serving both as a registry (to ensure the
> uniqueness of the values assigned to the states) and a common ground
> between platforms and drivers.
> 
> Something like:
> 
> enum platform_target_state {
> 	PLATFORM_STATE_UNKNOWN = -1,
> 	PLATFORM_STATE_WORKING = 0,
> 	PLATFORM_STATE_ACPI_S1,
> 	PLATFORM_STATE_ACPI_S2,
> 	PLATFORM_STATE_ACPI_S3,
> 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> 	...
> };
> 
> and define ->target_state to return a value of this type.
> 
> Then, if a driver sees one of these and recognizes that value, it should
> know exactly what to do.

Remind me why this is good idea?

We currently have 1364+ boards in tree. That will be rather large
enum.

If board wants to know if certain regulator stays online during
suspend, it should invent an API for _that_.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

On Sat 2017-07-15 00:16:16, Rafael J. Wysocki wrote:
> On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote:
> > On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> > > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> > >> Add an optional platform_suspend_ops callback: target_state, and a
> > >> helper function globally visible to get this called:
> > >> platform_suspend_target_state().
> > >>
> > >> This is useful for platform specific drivers that may need to take a
> > >> slightly different suspend/resume path based on the system's
> > >> suspend/resume state being entered.
> > >>
> > >> Although this callback is optional and documented as such, it requires
> > >> a platform_suspend_ops::begin callback to be implemented in order to
> > >> provide an accurate suspend/resume state within the driver that
> > >> implements this platform_suspend_ops.
> > >>
> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > >> ---
> > >>  include/linux/suspend.h | 12 ++++++++++++
> > >>  kernel/power/suspend.c  | 15 +++++++++++++++
> > >>  2 files changed, 27 insertions(+)
> > >>
> > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > >> index d9718378a8be..d998a04a90a2 100644
> > >> --- a/include/linux/suspend.h
> > >> +++ b/include/linux/suspend.h
> > >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
> > >>   *	Called by the PM core if the suspending of devices fails.
> > >>   *	This callback is optional and should only be implemented by platforms
> > >>   *	which require special recovery actions in that situation.
> > >> + *
> > >> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> > >> + * 	Called by device drivers that need to know the platform specific suspend
> > >> + * 	state the system is about to enter.
> > >> + * 	This callback is optional and should only be implemented by platforms
> > >> + * 	which require special handling of power management states within
> > >> + * 	drivers. It does require @begin to be implemented to provide the suspend
> > >> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> > >> + * 	mapping to suspend_state_t when relevant.
> > >>   */
> > >>  struct platform_suspend_ops {
> > >>  	int (*valid)(suspend_state_t state);
> > >> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
> > >>  	bool (*suspend_again)(void);
> > >>  	void (*end)(void);
> > >>  	void (*recover)(void);
> > >> +	int (*target_state)(void);
> > > 
> > > I would use unsigned int (the sign should not matter).
> > > 
> > >>  };
> > > 
> > > That's almost what I was thinking about except that the values returned by
> > > ->target_state should be unique, so it would be good to do something to
> > > ensure that.
> > > 
> > > The concern is as follows.
> > > 
> > > Say you have a driver develped for platform X where ->target_state returns
> > > A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> > > returning B for "mem" and C for "standby" and now the driver cannot
> > > distinguish between them.
> > > 
> > > Moreover, even if they both returned A for "mem" there might be differences
> > > in how "mem" was defined by each of them and therefore in what the driver was
> > > expected to do to handle "mem" on X and Y.
> > 
> > That makes sense, would you need the core implementation in
> > platform_suspend_target_state() to range check what
> > suspend_ops->target_state() returns against a set of reserved value say,
> > checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
> > would like to see being used?
> 
> I had an idea of using an enum type encompassing all of the power states
> defined for various platforms and serving both as a registry (to ensure the
> uniqueness of the values assigned to the states) and a common ground
> between platforms and drivers.
> 
> Something like:
> 
> enum platform_target_state {
> 	PLATFORM_STATE_UNKNOWN = -1,
> 	PLATFORM_STATE_WORKING = 0,
> 	PLATFORM_STATE_ACPI_S1,
> 	PLATFORM_STATE_ACPI_S2,
> 	PLATFORM_STATE_ACPI_S3,
> 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> 	...
> };
> 
> and define ->target_state to return a value of this type.
> 
> Then, if a driver sees one of these and recognizes that value, it should
> know exactly what to do.

Remind me why this is good idea?

We currently have 1364+ boards in tree. That will be rather large
enum.

If board wants to know if certain regulator stays online during
suspend, it should invent an API for _that_.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170715/4bc94312/attachment.sig>

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 08:28:38 AM Pavel Machek wrote:
> On Sat 2017-07-15 00:16:16, Rafael J. Wysocki wrote:
> > On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote:
> > > On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> > > > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> > > >> Add an optional platform_suspend_ops callback: target_state, and a
> > > >> helper function globally visible to get this called:
> > > >> platform_suspend_target_state().
> > > >>
> > > >> This is useful for platform specific drivers that may need to take a
> > > >> slightly different suspend/resume path based on the system's
> > > >> suspend/resume state being entered.
> > > >>
> > > >> Although this callback is optional and documented as such, it requires
> > > >> a platform_suspend_ops::begin callback to be implemented in order to
> > > >> provide an accurate suspend/resume state within the driver that
> > > >> implements this platform_suspend_ops.
> > > >>
> > > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > >> ---
> > > >>  include/linux/suspend.h | 12 ++++++++++++
> > > >>  kernel/power/suspend.c  | 15 +++++++++++++++
> > > >>  2 files changed, 27 insertions(+)
> > > >>
> > > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > >> index d9718378a8be..d998a04a90a2 100644
> > > >> --- a/include/linux/suspend.h
> > > >> +++ b/include/linux/suspend.h
> > > >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
> > > >>   *	Called by the PM core if the suspending of devices fails.
> > > >>   *	This callback is optional and should only be implemented by platforms
> > > >>   *	which require special recovery actions in that situation.
> > > >> + *
> > > >> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> > > >> + * 	Called by device drivers that need to know the platform specific suspend
> > > >> + * 	state the system is about to enter.
> > > >> + * 	This callback is optional and should only be implemented by platforms
> > > >> + * 	which require special handling of power management states within
> > > >> + * 	drivers. It does require @begin to be implemented to provide the suspend
> > > >> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> > > >> + * 	mapping to suspend_state_t when relevant.
> > > >>   */
> > > >>  struct platform_suspend_ops {
> > > >>  	int (*valid)(suspend_state_t state);
> > > >> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
> > > >>  	bool (*suspend_again)(void);
> > > >>  	void (*end)(void);
> > > >>  	void (*recover)(void);
> > > >> +	int (*target_state)(void);
> > > > 
> > > > I would use unsigned int (the sign should not matter).
> > > > 
> > > >>  };
> > > > 
> > > > That's almost what I was thinking about except that the values returned by
> > > > ->target_state should be unique, so it would be good to do something to
> > > > ensure that.
> > > > 
> > > > The concern is as follows.
> > > > 
> > > > Say you have a driver develped for platform X where ->target_state returns
> > > > A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> > > > returning B for "mem" and C for "standby" and now the driver cannot
> > > > distinguish between them.
> > > > 
> > > > Moreover, even if they both returned A for "mem" there might be differences
> > > > in how "mem" was defined by each of them and therefore in what the driver was
> > > > expected to do to handle "mem" on X and Y.
> > > 
> > > That makes sense, would you need the core implementation in
> > > platform_suspend_target_state() to range check what
> > > suspend_ops->target_state() returns against a set of reserved value say,
> > > checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
> > > would like to see being used?
> > 
> > I had an idea of using an enum type encompassing all of the power states
> > defined for various platforms and serving both as a registry (to ensure the
> > uniqueness of the values assigned to the states) and a common ground
> > between platforms and drivers.
> > 
> > Something like:
> > 
> > enum platform_target_state {
> > 	PLATFORM_STATE_UNKNOWN = -1,
> > 	PLATFORM_STATE_WORKING = 0,
> > 	PLATFORM_STATE_ACPI_S1,
> > 	PLATFORM_STATE_ACPI_S2,
> > 	PLATFORM_STATE_ACPI_S3,
> > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > 	...
> > };
> > 
> > and define ->target_state to return a value of this type.
> > 
> > Then, if a driver sees one of these and recognizes that value, it should
> > know exactly what to do.
> 
> Remind me why this is good idea?

Because there are drivers that need to do specific things during
suspend on a specific board when it goes into a specific state as a whole.

> We currently have 1364+ boards in tree. That will be rather large
> enum.

Fortunately enough, only some of the platform states need to be listed here,
the ones that drivers need to find out about.

The vast majority of drivers do the same thing regardless of the target state
of the platform.

> If board wants to know if certain regulator stays online during
> suspend, it should invent an API for _that_.

Ideally, yes.  However, that may be problematic for multiplatform kernels,
because they would need to have all of those APIs built in and the driver
code to figure out which API to use would be rather nasty.

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 08:28:38 AM Pavel Machek wrote:
> On Sat 2017-07-15 00:16:16, Rafael J. Wysocki wrote:
> > On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote:
> > > On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote:
> > > > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote:
> > > >> Add an optional platform_suspend_ops callback: target_state, and a
> > > >> helper function globally visible to get this called:
> > > >> platform_suspend_target_state().
> > > >>
> > > >> This is useful for platform specific drivers that may need to take a
> > > >> slightly different suspend/resume path based on the system's
> > > >> suspend/resume state being entered.
> > > >>
> > > >> Although this callback is optional and documented as such, it requires
> > > >> a platform_suspend_ops::begin callback to be implemented in order to
> > > >> provide an accurate suspend/resume state within the driver that
> > > >> implements this platform_suspend_ops.
> > > >>
> > > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > >> ---
> > > >>  include/linux/suspend.h | 12 ++++++++++++
> > > >>  kernel/power/suspend.c  | 15 +++++++++++++++
> > > >>  2 files changed, 27 insertions(+)
> > > >>
> > > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > >> index d9718378a8be..d998a04a90a2 100644
> > > >> --- a/include/linux/suspend.h
> > > >> +++ b/include/linux/suspend.h
> > > >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
> > > >>   *	Called by the PM core if the suspending of devices fails.
> > > >>   *	This callback is optional and should only be implemented by platforms
> > > >>   *	which require special recovery actions in that situation.
> > > >> + *
> > > >> + * @target_state: Returns the suspend state the suspend_ops will be entering.
> > > >> + * 	Called by device drivers that need to know the platform specific suspend
> > > >> + * 	state the system is about to enter.
> > > >> + * 	This callback is optional and should only be implemented by platforms
> > > >> + * 	which require special handling of power management states within
> > > >> + * 	drivers. It does require @begin to be implemented to provide the suspend
> > > >> + * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
> > > >> + * 	mapping to suspend_state_t when relevant.
> > > >>   */
> > > >>  struct platform_suspend_ops {
> > > >>  	int (*valid)(suspend_state_t state);
> > > >> @@ -184,6 +193,7 @@ struct platform_suspend_ops {
> > > >>  	bool (*suspend_again)(void);
> > > >>  	void (*end)(void);
> > > >>  	void (*recover)(void);
> > > >> +	int (*target_state)(void);
> > > > 
> > > > I would use unsigned int (the sign should not matter).
> > > > 
> > > >>  };
> > > > 
> > > > That's almost what I was thinking about except that the values returned by
> > > > ->target_state should be unique, so it would be good to do something to
> > > > ensure that.
> > > > 
> > > > The concern is as follows.
> > > > 
> > > > Say you have a driver develped for platform X where ->target_state returns
> > > > A for "mem" and B for "standby".  Then, the same IP is re-used on platform Y
> > > > returning B for "mem" and C for "standby" and now the driver cannot
> > > > distinguish between them.
> > > > 
> > > > Moreover, even if they both returned A for "mem" there might be differences
> > > > in how "mem" was defined by each of them and therefore in what the driver was
> > > > expected to do to handle "mem" on X and Y.
> > > 
> > > That makes sense, would you need the core implementation in
> > > platform_suspend_target_state() to range check what
> > > suspend_ops->target_state() returns against a set of reserved value say,
> > > checking from 0 up to ACPI_S_STATE_COUNT or is there another range you
> > > would like to see being used?
> > 
> > I had an idea of using an enum type encompassing all of the power states
> > defined for various platforms and serving both as a registry (to ensure the
> > uniqueness of the values assigned to the states) and a common ground
> > between platforms and drivers.
> > 
> > Something like:
> > 
> > enum platform_target_state {
> > 	PLATFORM_STATE_UNKNOWN = -1,
> > 	PLATFORM_STATE_WORKING = 0,
> > 	PLATFORM_STATE_ACPI_S1,
> > 	PLATFORM_STATE_ACPI_S2,
> > 	PLATFORM_STATE_ACPI_S3,
> > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > 	...
> > };
> > 
> > and define ->target_state to return a value of this type.
> > 
> > Then, if a driver sees one of these and recognizes that value, it should
> > know exactly what to do.
> 
> Remind me why this is good idea?

Because there are drivers that need to do specific things during
suspend on a specific board when it goes into a specific state as a whole.

> We currently have 1364+ boards in tree. That will be rather large
> enum.

Fortunately enough, only some of the platform states need to be listed here,
the ones that drivers need to find out about.

The vast majority of drivers do the same thing regardless of the target state
of the platform.

> If board wants to know if certain regulator stays online during
> suspend, it should invent an API for _that_.

Ideally, yes.  However, that may be problematic for multiplatform kernels,
because they would need to have all of those APIs built in and the driver
code to figure out which API to use would be rather nasty.

Thanks,
Rafael

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]

Hi!

> > > I had an idea of using an enum type encompassing all of the power states
> > > defined for various platforms and serving both as a registry (to ensure the
> > > uniqueness of the values assigned to the states) and a common ground
> > > between platforms and drivers.
> > > 
> > > Something like:
> > > 
> > > enum platform_target_state {
> > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > 	PLATFORM_STATE_WORKING = 0,
> > > 	PLATFORM_STATE_ACPI_S1,
> > > 	PLATFORM_STATE_ACPI_S2,
> > > 	PLATFORM_STATE_ACPI_S3,
> > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > 	...
> > > };
> > > 
> > > and define ->target_state to return a value of this type.
> > > 
> > > Then, if a driver sees one of these and recognizes that value, it should
> > > know exactly what to do.
> > 
> > Remind me why this is good idea?
> 
> Because there are drivers that need to do specific things during
> suspend on a specific board when it goes into a specific state as a
> whole.

We have seen driver that cares about voltage to his device being
lost. That's reasonable.

Inquiring what the platform target state is... is not.

> > If board wants to know if certain regulator stays online during
> > suspend, it should invent an API for _that_.
> 
> Ideally, yes.  However, that may be problematic for multiplatform kernels,
> because they would need to have all of those APIs built in and the driver
> code to figure out which API to use would be rather nasty.

Lets do it the right way. Big enum is wrong.

We already have

struct regulator_state {
       int uV; /* suspend voltage */
       unsigned int mode; /* suspend regulator operating mode */
       int enabled; /* is regulator enabled in this suspend state */
       int disabled; /* is the regulator disabled in this suspend state */
};

 * struct regulation_constraints - regulator operating constraints.
  * @state_disk: State for regulator when system is suspended in disk
  * mode.
  * @state_mem: State for regulator when system is suspended in mem
  * mode.
  * @state_standby: State for regulator when system is suspended in
  * standby
  *                 mode.
   
. So it seems that maybe we should tell the drivers if we are entering
"state_mem" or "state_standby" (something I may have opposed, sorry),
then the driver can get neccessary information from regulator
framework.

I don't think it should cause problems with multiplatform kernels.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

Hi!

> > > I had an idea of using an enum type encompassing all of the power states
> > > defined for various platforms and serving both as a registry (to ensure the
> > > uniqueness of the values assigned to the states) and a common ground
> > > between platforms and drivers.
> > > 
> > > Something like:
> > > 
> > > enum platform_target_state {
> > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > 	PLATFORM_STATE_WORKING = 0,
> > > 	PLATFORM_STATE_ACPI_S1,
> > > 	PLATFORM_STATE_ACPI_S2,
> > > 	PLATFORM_STATE_ACPI_S3,
> > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > 	...
> > > };
> > > 
> > > and define ->target_state to return a value of this type.
> > > 
> > > Then, if a driver sees one of these and recognizes that value, it should
> > > know exactly what to do.
> > 
> > Remind me why this is good idea?
> 
> Because there are drivers that need to do specific things during
> suspend on a specific board when it goes into a specific state as a
> whole.

We have seen driver that cares about voltage to his device being
lost. That's reasonable.

Inquiring what the platform target state is... is not.

> > If board wants to know if certain regulator stays online during
> > suspend, it should invent an API for _that_.
> 
> Ideally, yes.  However, that may be problematic for multiplatform kernels,
> because they would need to have all of those APIs built in and the driver
> code to figure out which API to use would be rather nasty.

Lets do it the right way. Big enum is wrong.

We already have

struct regulator_state {
       int uV; /* suspend voltage */
       unsigned int mode; /* suspend regulator operating mode */
       int enabled; /* is regulator enabled in this suspend state */
       int disabled; /* is the regulator disabled in this suspend state */
};

 * struct regulation_constraints - regulator operating constraints.
  * @state_disk: State for regulator when system is suspended in disk
  * mode.
  * @state_mem: State for regulator when system is suspended in mem
  * mode.
  * @state_standby: State for regulator when system is suspended in
  * standby
  *                 mode.
   
. So it seems that maybe we should tell the drivers if we are entering
"state_mem" or "state_standby" (something I may have opposed, sorry),
then the driver can get neccessary information from regulator
framework.

I don't think it should cause problems with multiplatform kernels.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170715/aad6fa22/attachment.sig>

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/15/2017 09:46 AM, Pavel Machek wrote:
> Hi!
> 
>>>> I had an idea of using an enum type encompassing all of the power states
>>>> defined for various platforms and serving both as a registry (to ensure the
>>>> uniqueness of the values assigned to the states) and a common ground
>>>> between platforms and drivers.
>>>>
>>>> Something like:
>>>>
>>>> enum platform_target_state {
>>>> 	PLATFORM_STATE_UNKNOWN = -1,
>>>> 	PLATFORM_STATE_WORKING = 0,
>>>> 	PLATFORM_STATE_ACPI_S1,
>>>> 	PLATFORM_STATE_ACPI_S2,
>>>> 	PLATFORM_STATE_ACPI_S3,
>>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
>>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
>>>> 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
>>>> 	...
>>>> };
>>>>
>>>> and define ->target_state to return a value of this type.
>>>>
>>>> Then, if a driver sees one of these and recognizes that value, it should
>>>> know exactly what to do.
>>>
>>> Remind me why this is good idea?
>>
>> Because there are drivers that need to do specific things during
>> suspend on a specific board when it goes into a specific state as a
>> whole.
> 
> We have seen driver that cares about voltage to his device being
> lost. That's reasonable.
> 
> Inquiring what the platform target state is... is not.
> 
>>> If board wants to know if certain regulator stays online during
>>> suspend, it should invent an API for _that_.
>>
>> Ideally, yes.  However, that may be problematic for multiplatform kernels,
>> because they would need to have all of those APIs built in and the driver
>> code to figure out which API to use would be rather nasty.
> 
> Lets do it the right way. Big enum is wrong.

The enum offers the advantage of centralizing how many different states
exist for all the platforms we know about in the kernel, it's easy to
define common values for platforms that have the same semantics, just
like it's simple to add new values for platform specific details.

Is the concern that we could overflow the enum size, or that there is
not a common set of values to describe states?

> 
> We already have
> 
> struct regulator_state {
>        int uV; /* suspend voltage */
>        unsigned int mode; /* suspend regulator operating mode */
>        int enabled; /* is regulator enabled in this suspend state */
>        int disabled; /* is the regulator disabled in this suspend state */
> };
> 
>  * struct regulation_constraints - regulator operating constraints.
>   * @state_disk: State for regulator when system is suspended in disk
>   * mode.
>   * @state_mem: State for regulator when system is suspended in mem
>   * mode.
>   * @state_standby: State for regulator when system is suspended in
>   * standby
>   *                 mode.
>    
> . So it seems that maybe we should tell the drivers if we are entering
> "state_mem" or "state_standby" (something I may have opposed, sorry),
> then the driver can get neccessary information from regulator
> framework.

OK, so what would be the mechanism to tell these drivers about the
system wide suspend state they are entering if it is not via
platform_suspend_target_state()?

Keep in mind that regulators might be one aspect of what could be
causing the platform to behave specifically in one suspend state vs.
another, but there could be pieces of HW within the SoC that can't be
described with power domains, voltage islands etc. that would still have
inherent suspend states properties (like memory retention, pin/pad
controls etc. etc). We still need some mechanism, possibly centralized

> 
> I don't think it should cause problems with multiplatform kernels.

Just like platform_suspend_target_state() with an enum is not creating
problems either. suspend_ops is already a singleton for a given kernel
image so a given kernel running on a given platform will get to see a
subset of the enum values defined.

In any case, just agree and I will be happy to follow-up with patches.
-- 
Florian

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/15/2017 09:46 AM, Pavel Machek wrote:
> Hi!
> 
>>>> I had an idea of using an enum type encompassing all of the power states
>>>> defined for various platforms and serving both as a registry (to ensure the
>>>> uniqueness of the values assigned to the states) and a common ground
>>>> between platforms and drivers.
>>>>
>>>> Something like:
>>>>
>>>> enum platform_target_state {
>>>> 	PLATFORM_STATE_UNKNOWN = -1,
>>>> 	PLATFORM_STATE_WORKING = 0,
>>>> 	PLATFORM_STATE_ACPI_S1,
>>>> 	PLATFORM_STATE_ACPI_S2,
>>>> 	PLATFORM_STATE_ACPI_S3,
>>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
>>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
>>>> 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
>>>> 	...
>>>> };
>>>>
>>>> and define ->target_state to return a value of this type.
>>>>
>>>> Then, if a driver sees one of these and recognizes that value, it should
>>>> know exactly what to do.
>>>
>>> Remind me why this is good idea?
>>
>> Because there are drivers that need to do specific things during
>> suspend on a specific board when it goes into a specific state as a
>> whole.
> 
> We have seen driver that cares about voltage to his device being
> lost. That's reasonable.
> 
> Inquiring what the platform target state is... is not.
> 
>>> If board wants to know if certain regulator stays online during
>>> suspend, it should invent an API for _that_.
>>
>> Ideally, yes.  However, that may be problematic for multiplatform kernels,
>> because they would need to have all of those APIs built in and the driver
>> code to figure out which API to use would be rather nasty.
> 
> Lets do it the right way. Big enum is wrong.

The enum offers the advantage of centralizing how many different states
exist for all the platforms we know about in the kernel, it's easy to
define common values for platforms that have the same semantics, just
like it's simple to add new values for platform specific details.

Is the concern that we could overflow the enum size, or that there is
not a common set of values to describe states?

> 
> We already have
> 
> struct regulator_state {
>        int uV; /* suspend voltage */
>        unsigned int mode; /* suspend regulator operating mode */
>        int enabled; /* is regulator enabled in this suspend state */
>        int disabled; /* is the regulator disabled in this suspend state */
> };
> 
>  * struct regulation_constraints - regulator operating constraints.
>   * @state_disk: State for regulator when system is suspended in disk
>   * mode.
>   * @state_mem: State for regulator when system is suspended in mem
>   * mode.
>   * @state_standby: State for regulator when system is suspended in
>   * standby
>   *                 mode.
>    
> . So it seems that maybe we should tell the drivers if we are entering
> "state_mem" or "state_standby" (something I may have opposed, sorry),
> then the driver can get neccessary information from regulator
> framework.

OK, so what would be the mechanism to tell these drivers about the
system wide suspend state they are entering if it is not via
platform_suspend_target_state()?

Keep in mind that regulators might be one aspect of what could be
causing the platform to behave specifically in one suspend state vs.
another, but there could be pieces of HW within the SoC that can't be
described with power domains, voltage islands etc. that would still have
inherent suspend states properties (like memory retention, pin/pad
controls etc. etc). We still need some mechanism, possibly centralized

> 
> I don't think it should cause problems with multiplatform kernels.

Just like platform_suspend_target_state() with an enum is not creating
problems either. suspend_ops is already a singleton for a given kernel
image so a given kernel running on a given platform will get to see a
subset of the enum values defined.

In any case, just agree and I will be happy to follow-up with patches.
-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
> > We already have
> > 
> > struct regulator_state {
> >        int uV; /* suspend voltage */
> >        unsigned int mode; /* suspend regulator operating mode */
> >        int enabled; /* is regulator enabled in this suspend state */
> >        int disabled; /* is the regulator disabled in this suspend state */
> > };
> > 
> >  * struct regulation_constraints - regulator operating constraints.
> >   * @state_disk: State for regulator when system is suspended in disk
> >   * mode.
> >   * @state_mem: State for regulator when system is suspended in mem
> >   * mode.
> >   * @state_standby: State for regulator when system is suspended in
> >   * standby
> >   *                 mode.
> >    
> > . So it seems that maybe we should tell the drivers if we are entering
> > "state_mem" or "state_standby" (something I may have opposed, sorry),
> > then the driver can get neccessary information from regulator
> > framework.
> 
> OK, so what would be the mechanism to tell these drivers about the
> system wide suspend state they are entering if it is not via
> platform_suspend_target_state()?
> 
> Keep in mind that regulators might be one aspect of what could be
> causing the platform to behave specifically in one suspend state vs.
> another, but there could be pieces of HW within the SoC that can't be
> described with power domains, voltage islands etc. that would still have
> inherent suspend states properties (like memory retention, pin/pad
> controls etc. etc). We still need some mechanism, possibly centralized
> 

I concur, the regulator stuff is one aspect of one of our suspend state
(cutting VDDcore). But we have another state where the main clock (going
to the IPs) is going from a few hundred MHz to 32kHz. This is currently
handled by calling at91_suspend_entering_slow_clock(). I think it is
important to take that into account so we can remove this hack from the
kernel.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
> > We already have
> > 
> > struct regulator_state {
> >        int uV; /* suspend voltage */
> >        unsigned int mode; /* suspend regulator operating mode */
> >        int enabled; /* is regulator enabled in this suspend state */
> >        int disabled; /* is the regulator disabled in this suspend state */
> > };
> > 
> >  * struct regulation_constraints - regulator operating constraints.
> >   * @state_disk: State for regulator when system is suspended in disk
> >   * mode.
> >   * @state_mem: State for regulator when system is suspended in mem
> >   * mode.
> >   * @state_standby: State for regulator when system is suspended in
> >   * standby
> >   *                 mode.
> >    
> > . So it seems that maybe we should tell the drivers if we are entering
> > "state_mem" or "state_standby" (something I may have opposed, sorry),
> > then the driver can get neccessary information from regulator
> > framework.
> 
> OK, so what would be the mechanism to tell these drivers about the
> system wide suspend state they are entering if it is not via
> platform_suspend_target_state()?
> 
> Keep in mind that regulators might be one aspect of what could be
> causing the platform to behave specifically in one suspend state vs.
> another, but there could be pieces of HW within the SoC that can't be
> described with power domains, voltage islands etc. that would still have
> inherent suspend states properties (like memory retention, pin/pad
> controls etc. etc). We still need some mechanism, possibly centralized
> 

I concur, the regulator stuff is one aspect of one of our suspend state
(cutting VDDcore). But we have another state where the main clock (going
to the IPs) is going from a few hundred MHz to 32kHz. This is currently
handled by calling at91_suspend_entering_slow_clock(). I think it is
important to take that into account so we can remove this hack from the
kernel.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> 
> On 07/15/2017 09:46 AM, Pavel Machek wrote:
> > Hi!
> > 
> >>>> I had an idea of using an enum type encompassing all of the power states
> >>>> defined for various platforms and serving both as a registry (to ensure the
> >>>> uniqueness of the values assigned to the states) and a common ground
> >>>> between platforms and drivers.
> >>>>
> >>>> Something like:
> >>>>
> >>>> enum platform_target_state {
> >>>> 	PLATFORM_STATE_UNKNOWN = -1,
> >>>> 	PLATFORM_STATE_WORKING = 0,
> >>>> 	PLATFORM_STATE_ACPI_S1,
> >>>> 	PLATFORM_STATE_ACPI_S2,
> >>>> 	PLATFORM_STATE_ACPI_S3,
> >>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> >>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> >>>> 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> >>>> 	...
> >>>> };
> >>>>
> >>>> and define ->target_state to return a value of this type.
> >>>>
> >>>> Then, if a driver sees one of these and recognizes that value, it should
> >>>> know exactly what to do.
> >>>
> >>> Remind me why this is good idea?
> >>
> >> Because there are drivers that need to do specific things during
> >> suspend on a specific board when it goes into a specific state as a
> >> whole.
> > 
> > We have seen driver that cares about voltage to his device being
> > lost. That's reasonable.
> > 
> > Inquiring what the platform target state is... is not.
> > 
> >>> If board wants to know if certain regulator stays online during
> >>> suspend, it should invent an API for _that_.
> >>
> >> Ideally, yes.  However, that may be problematic for multiplatform kernels,
> >> because they would need to have all of those APIs built in and the driver
> >> code to figure out which API to use would be rather nasty.
> > 
> > Lets do it the right way. Big enum is wrong.
> 
> The enum offers the advantage of centralizing how many different states
> exist for all the platforms we know about in the kernel, it's easy to
> define common values for platforms that have the same semantics, just
> like it's simple to add new values for platform specific details.

Well, you seem to be liking this, so why don't you just implement it?

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> 
> On 07/15/2017 09:46 AM, Pavel Machek wrote:
> > Hi!
> > 
> >>>> I had an idea of using an enum type encompassing all of the power states
> >>>> defined for various platforms and serving both as a registry (to ensure the
> >>>> uniqueness of the values assigned to the states) and a common ground
> >>>> between platforms and drivers.
> >>>>
> >>>> Something like:
> >>>>
> >>>> enum platform_target_state {
> >>>> 	PLATFORM_STATE_UNKNOWN = -1,
> >>>> 	PLATFORM_STATE_WORKING = 0,
> >>>> 	PLATFORM_STATE_ACPI_S1,
> >>>> 	PLATFORM_STATE_ACPI_S2,
> >>>> 	PLATFORM_STATE_ACPI_S3,
> >>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> >>>> 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> >>>> 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> >>>> 	...
> >>>> };
> >>>>
> >>>> and define ->target_state to return a value of this type.
> >>>>
> >>>> Then, if a driver sees one of these and recognizes that value, it should
> >>>> know exactly what to do.
> >>>
> >>> Remind me why this is good idea?
> >>
> >> Because there are drivers that need to do specific things during
> >> suspend on a specific board when it goes into a specific state as a
> >> whole.
> > 
> > We have seen driver that cares about voltage to his device being
> > lost. That's reasonable.
> > 
> > Inquiring what the platform target state is... is not.
> > 
> >>> If board wants to know if certain regulator stays online during
> >>> suspend, it should invent an API for _that_.
> >>
> >> Ideally, yes.  However, that may be problematic for multiplatform kernels,
> >> because they would need to have all of those APIs built in and the driver
> >> code to figure out which API to use would be rather nasty.
> > 
> > Lets do it the right way. Big enum is wrong.
> 
> The enum offers the advantage of centralizing how many different states
> exist for all the platforms we know about in the kernel, it's easy to
> define common values for platforms that have the same semantics, just
> like it's simple to add new values for platform specific details.

Well, you seem to be liking this, so why don't you just implement it?

Thanks,
Rafael

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote:
> Hi!
> 
> > > > I had an idea of using an enum type encompassing all of the power states
> > > > defined for various platforms and serving both as a registry (to ensure the
> > > > uniqueness of the values assigned to the states) and a common ground
> > > > between platforms and drivers.
> > > > 
> > > > Something like:
> > > > 
> > > > enum platform_target_state {
> > > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > > 	PLATFORM_STATE_WORKING = 0,
> > > > 	PLATFORM_STATE_ACPI_S1,
> > > > 	PLATFORM_STATE_ACPI_S2,
> > > > 	PLATFORM_STATE_ACPI_S3,
> > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > > 	...
> > > > };
> > > > 
> > > > and define ->target_state to return a value of this type.
> > > > 
> > > > Then, if a driver sees one of these and recognizes that value, it should
> > > > know exactly what to do.
> > > 
> > > Remind me why this is good idea?
> > 
> > Because there are drivers that need to do specific things during
> > suspend on a specific board when it goes into a specific state as a
> > whole.
> 
> We have seen driver that cares about voltage to his device being
> lost. That's reasonable.
> 
> Inquiring what the platform target state is... is not.

So why exactly isn't it reasonable?

Please use technical arguments.  Saying that something is wrong without
explaining the problem you see with it isn't particulatly useful in technical
discussions.

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote:
> Hi!
> 
> > > > I had an idea of using an enum type encompassing all of the power states
> > > > defined for various platforms and serving both as a registry (to ensure the
> > > > uniqueness of the values assigned to the states) and a common ground
> > > > between platforms and drivers.
> > > > 
> > > > Something like:
> > > > 
> > > > enum platform_target_state {
> > > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > > 	PLATFORM_STATE_WORKING = 0,
> > > > 	PLATFORM_STATE_ACPI_S1,
> > > > 	PLATFORM_STATE_ACPI_S2,
> > > > 	PLATFORM_STATE_ACPI_S3,
> > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > > 	...
> > > > };
> > > > 
> > > > and define ->target_state to return a value of this type.
> > > > 
> > > > Then, if a driver sees one of these and recognizes that value, it should
> > > > know exactly what to do.
> > > 
> > > Remind me why this is good idea?
> > 
> > Because there are drivers that need to do specific things during
> > suspend on a specific board when it goes into a specific state as a
> > whole.
> 
> We have seen driver that cares about voltage to his device being
> lost. That's reasonable.
> 
> Inquiring what the platform target state is... is not.

So why exactly isn't it reasonable?

Please use technical arguments.  Saying that something is wrong without
explaining the problem you see with it isn't particulatly useful in technical
discussions.

Thanks,
Rafael

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Mason]

On 16/07/2017 01:24, Rafael J. Wysocki wrote:

> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
>
>> The enum offers the advantage of centralizing how many different states
>> exist for all the platforms we know about in the kernel, it's easy to
>> define common values for platforms that have the same semantics, just
>> like it's simple to add new values for platform specific details.
> 
> Well, you seem to be liking this, so why don't you just implement it?

At the end of his message, Florian wrote:

> In any case, just agree and I will be happy to follow-up with patches.

Regards.

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Mason]

On 16/07/2017 01:24, Rafael J. Wysocki wrote:

> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
>
>> The enum offers the advantage of centralizing how many different states
>> exist for all the platforms we know about in the kernel, it's easy to
>> define common values for platforms that have the same semantics, just
>> like it's simple to add new values for platform specific details.
> 
> Well, you seem to be liking this, so why don't you just implement it?

At the end of his message, Florian wrote:

> In any case, just agree and I will be happy to follow-up with patches.

Regards.

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
> 
> > On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> >
> >> The enum offers the advantage of centralizing how many different states
> >> exist for all the platforms we know about in the kernel, it's easy to
> >> define common values for platforms that have the same semantics, just
> >> like it's simple to add new values for platform specific details.
> > 
> > Well, you seem to be liking this, so why don't you just implement it?
> 
> At the end of his message, Florian wrote:
> 
> > In any case, just agree and I will be happy to follow-up with patches.

But it may be hard to convince everybody without posting code changes
and often enough showing a patch makes a good argument.

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
> 
> > On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> >
> >> The enum offers the advantage of centralizing how many different states
> >> exist for all the platforms we know about in the kernel, it's easy to
> >> define common values for platforms that have the same semantics, just
> >> like it's simple to add new values for platform specific details.
> > 
> > Well, you seem to be liking this, so why don't you just implement it?
> 
> At the end of his message, Florian wrote:
> 
> > In any case, just agree and I will be happy to follow-up with patches.

But it may be hard to convince everybody without posting code changes
and often enough showing a patch makes a good argument.

Thanks,
Rafael

[PATCH 0/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

This patch series implements the idea discussed in this thread:

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

The last patch is relative to the pending submission of the Broadcom STB
S2/S3/S5 suspend/resume code that can be found below, and is provided
as an example of how this can be useful.

https://lkml.org/lkml/2017/6/16/737

Changes from RFC:

- make platform_target_state an enum that platforms can modify to
  include their own states
- updated brcmstb PM code to translate internal states to externally
  visible platform_target_state

Florian Fainelli (2):
  PM / suspend: Add platform_suspend_target_state()
  soc: bcm: brcmstb: PM: Implement target_state callback

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 22 ++++++++++++++++++++++
 include/linux/suspend.h             | 26 ++++++++++++++++++++++++++
 kernel/power/suspend.c              | 15 +++++++++++++++
 3 files changed, 63 insertions(+)

-- 
2.9.3

[PATCH 0/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

This patch series implements the idea discussed in this thread:

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

The last patch is relative to the pending submission of the Broadcom STB
S2/S3/S5 suspend/resume code that can be found below, and is provided
as an example of how this can be useful.

https://lkml.org/lkml/2017/6/16/737

Changes from RFC:

- make platform_target_state an enum that platforms can modify to
  include their own states
- updated brcmstb PM code to translate internal states to externally
  visible platform_target_state

Florian Fainelli (2):
  PM / suspend: Add platform_suspend_target_state()
  soc: bcm: brcmstb: PM: Implement target_state callback

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 22 ++++++++++++++++++++++
 include/linux/suspend.h             | 26 ++++++++++++++++++++++++++
 kernel/power/suspend.c              | 15 +++++++++++++++
 3 files changed, 63 insertions(+)

-- 
2.9.3

[PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

Add an optional platform_suspend_ops callback: target_state, and a
helper function globally visible to get this called:
platform_suspend_target_state().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Although this callback is optional and documented as such, it requires
a platform_suspend_ops::begin callback to be implemented in order to
provide an accurate suspend/resume state within the driver that
implements this platform_suspend_ops.

An enumeration: platform_target_state is defined which currently defines
the standard ACPI_S[1-4] states and can be extended with platform
specific suspend states.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/suspend.h | 25 +++++++++++++++++++++++++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..6e6cc0778816 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -50,6 +50,16 @@ enum suspend_stat_step {
 	SUSPEND_RESUME
 };
 
+enum platform_target_state {
+	PLATFORM_STATE_UNKNOWN = -1,
+	PLATFORM_STATE_WORKING = 0,
+	PLATFORM_STATE_ACPI_S1,
+	PLATFORM_STATE_ACPI_S2,
+	PLATFORM_STATE_ACPI_S3,
+	PLATFORM_STATE_ACPI_S4,
+	/* Add platform specific states here */
+};
+
 struct suspend_stats {
 	int	success;
 	int	fail;
@@ -172,6 +182,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
  *	Called by the PM core if the suspending of devices fails.
  *	This callback is optional and should only be implemented by platforms
  *	which require special recovery actions in that situation.
+ *
+ * @target_state: Returns the suspend state the suspend_ops will be entering.
+ * 	Called by device drivers that need to know the platform specific suspend
+ * 	state the system is about to enter.
+ * 	This callback is optional and should only be implemented by platforms
+ * 	which require special handling of power management states within
+ * 	drivers. It does require @begin to be implemented to provide the suspend
+ * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
+ * 	mapping to suspend_state_t when relevant.
  */
 struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
@@ -184,6 +203,7 @@ struct platform_suspend_ops {
 	bool (*suspend_again)(void);
 	void (*end)(void);
 	void (*recover)(void);
+	enum platform_target_state (*target_state)(void);
 };
 
 struct platform_freeze_ops {
@@ -202,6 +222,7 @@ struct platform_freeze_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern enum platform_target_state platform_suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -281,6 +302,10 @@ static inline bool pm_resume_via_firmware(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline int platform_suspend_target_state(void)
+{
+	return PLATFORM_STATE_UNKNOWN;
+}
 static inline bool idle_should_freeze(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..cd1b62f23b0e 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -202,6 +202,21 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 /**
+ * platform_suspend_target_state - Return the platform specific suspend state.
+ * a begin() callback is necessary in order to fill this information correctly
+ * for callers.
+ */
+enum platform_target_state platform_suspend_target_state(void)
+{
+	if (!suspend_ops || !suspend_ops->target_state ||
+	    (suspend_ops->target_state && !suspend_ops->begin))
+		return -ENOTSUPP;
+
+	return suspend_ops->target_state();
+}
+EXPORT_SYMBOL_GPL(platform_suspend_target_state);
+
+/**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
  * Platform drivers that implement mem suspend only and only need to check for
-- 
2.9.3

[PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

Add an optional platform_suspend_ops callback: target_state, and a
helper function globally visible to get this called:
platform_suspend_target_state().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Although this callback is optional and documented as such, it requires
a platform_suspend_ops::begin callback to be implemented in order to
provide an accurate suspend/resume state within the driver that
implements this platform_suspend_ops.

An enumeration: platform_target_state is defined which currently defines
the standard ACPI_S[1-4] states and can be extended with platform
specific suspend states.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/suspend.h | 25 +++++++++++++++++++++++++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..6e6cc0778816 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -50,6 +50,16 @@ enum suspend_stat_step {
 	SUSPEND_RESUME
 };
 
+enum platform_target_state {
+	PLATFORM_STATE_UNKNOWN = -1,
+	PLATFORM_STATE_WORKING = 0,
+	PLATFORM_STATE_ACPI_S1,
+	PLATFORM_STATE_ACPI_S2,
+	PLATFORM_STATE_ACPI_S3,
+	PLATFORM_STATE_ACPI_S4,
+	/* Add platform specific states here */
+};
+
 struct suspend_stats {
 	int	success;
 	int	fail;
@@ -172,6 +182,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step)
  *	Called by the PM core if the suspending of devices fails.
  *	This callback is optional and should only be implemented by platforms
  *	which require special recovery actions in that situation.
+ *
+ * @target_state: Returns the suspend state the suspend_ops will be entering.
+ * 	Called by device drivers that need to know the platform specific suspend
+ * 	state the system is about to enter.
+ * 	This callback is optional and should only be implemented by platforms
+ * 	which require special handling of power management states within
+ * 	drivers. It does require @begin to be implemented to provide the suspend
+ * 	state. Return value is platform_suspend_ops specific, and may be a 1:1
+ * 	mapping to suspend_state_t when relevant.
  */
 struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
@@ -184,6 +203,7 @@ struct platform_suspend_ops {
 	bool (*suspend_again)(void);
 	void (*end)(void);
 	void (*recover)(void);
+	enum platform_target_state (*target_state)(void);
 };
 
 struct platform_freeze_ops {
@@ -202,6 +222,7 @@ struct platform_freeze_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern enum platform_target_state platform_suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -281,6 +302,10 @@ static inline bool pm_resume_via_firmware(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline int platform_suspend_target_state(void)
+{
+	return PLATFORM_STATE_UNKNOWN;
+}
 static inline bool idle_should_freeze(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..cd1b62f23b0e 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -202,6 +202,21 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 /**
+ * platform_suspend_target_state - Return the platform specific suspend state.
+ * a begin() callback is necessary in order to fill this information correctly
+ * for callers.
+ */
+enum platform_target_state platform_suspend_target_state(void)
+{
+	if (!suspend_ops || !suspend_ops->target_state ||
+	    (suspend_ops->target_state && !suspend_ops->begin))
+		return -ENOTSUPP;
+
+	return suspend_ops->target_state();
+}
+EXPORT_SYMBOL_GPL(platform_suspend_target_state);
+
+/**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
  * Platform drivers that implement mem suspend only and only need to check for
-- 
2.9.3

[PATCH 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Florian Fainelli]

Provide a target_state callback implementation which just returns the
suspend_state_t the system is about to enter. Broadcom STB drivers can
utilize platform_suspend_target_state() to retrieve that and take
appropriate actions (e.g: full vs. partial re-initialization). Two
states are implemented: PLATFORM_STATE_WORKING and
PLATFORM_STATE_BRCMSTB_S3MEM to that end.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 22 ++++++++++++++++++++++
 include/linux/suspend.h             |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index dcf8c8065508..750a74b3fc9d 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -104,6 +104,7 @@ struct brcmstb_pm_control {
 	u32 phy_b_standby_ctrl_offs;
 	bool needs_ddr_pad;
 	struct platform_device *pdev;
+	suspend_state_t pm_state;
 };
 
 enum bsp_initiate_command {
@@ -533,9 +534,30 @@ static int brcmstb_pm_valid(suspend_state_t state)
 	}
 }
 
+static int brcmstb_pm_begin(suspend_state_t state)
+{
+	ctrl.pm_state = state;
+
+	return 0;
+}
+
+static enum platform_target_state brcmstb_target_state(void)
+{
+	switch (ctrl.pm_state) {
+	case PM_SUSPEND_STANDBY:
+		return PLATFORM_STATE_WORKING;
+	case PM_SUSPEND_MEM:
+		return PLATFORM_STATE_BRCMSTB_S3MEM;
+	default:
+		return PLATFORM_STATE_UNKNOWN;
+	}
+}
+
 static const struct platform_suspend_ops brcmstb_pm_ops = {
+	.begin		= brcmstb_pm_begin,
 	.enter		= brcmstb_pm_enter,
 	.valid		= brcmstb_pm_valid,
+	.target_state	= brcmstb_target_state,
 };
 
 static const struct of_device_id aon_ctrl_dt_ids[] = {
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6e6cc0778816..15d21d76e0bf 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -58,6 +58,7 @@ enum platform_target_state {
 	PLATFORM_STATE_ACPI_S3,
 	PLATFORM_STATE_ACPI_S4,
 	/* Add platform specific states here */
+	PLATFORM_STATE_BRCMSTB_S3MEM,
 };
 
 struct suspend_stats {
-- 
2.9.3

[PATCH 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Florian Fainelli]

Provide a target_state callback implementation which just returns the
suspend_state_t the system is about to enter. Broadcom STB drivers can
utilize platform_suspend_target_state() to retrieve that and take
appropriate actions (e.g: full vs. partial re-initialization). Two
states are implemented: PLATFORM_STATE_WORKING and
PLATFORM_STATE_BRCMSTB_S3MEM to that end.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 22 ++++++++++++++++++++++
 include/linux/suspend.h             |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index dcf8c8065508..750a74b3fc9d 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -104,6 +104,7 @@ struct brcmstb_pm_control {
 	u32 phy_b_standby_ctrl_offs;
 	bool needs_ddr_pad;
 	struct platform_device *pdev;
+	suspend_state_t pm_state;
 };
 
 enum bsp_initiate_command {
@@ -533,9 +534,30 @@ static int brcmstb_pm_valid(suspend_state_t state)
 	}
 }
 
+static int brcmstb_pm_begin(suspend_state_t state)
+{
+	ctrl.pm_state = state;
+
+	return 0;
+}
+
+static enum platform_target_state brcmstb_target_state(void)
+{
+	switch (ctrl.pm_state) {
+	case PM_SUSPEND_STANDBY:
+		return PLATFORM_STATE_WORKING;
+	case PM_SUSPEND_MEM:
+		return PLATFORM_STATE_BRCMSTB_S3MEM;
+	default:
+		return PLATFORM_STATE_UNKNOWN;
+	}
+}
+
 static const struct platform_suspend_ops brcmstb_pm_ops = {
+	.begin		= brcmstb_pm_begin,
 	.enter		= brcmstb_pm_enter,
 	.valid		= brcmstb_pm_valid,
+	.target_state	= brcmstb_target_state,
 };
 
 static const struct of_device_id aon_ctrl_dt_ids[] = {
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6e6cc0778816..15d21d76e0bf 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -58,6 +58,7 @@ enum platform_target_state {
 	PLATFORM_STATE_ACPI_S3,
 	PLATFORM_STATE_ACPI_S4,
 	/* Add platform specific states here */
+	PLATFORM_STATE_BRCMSTB_S3MEM,
 };
 
 struct suspend_stats {
-- 
2.9.3

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
> On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
>> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
>>
>>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
>>>
>>>> The enum offers the advantage of centralizing how many different states
>>>> exist for all the platforms we know about in the kernel, it's easy to
>>>> define common values for platforms that have the same semantics, just
>>>> like it's simple to add new values for platform specific details.
>>>
>>> Well, you seem to be liking this, so why don't you just implement it?
>>
>> At the end of his message, Florian wrote:
>>
>>> In any case, just agree and I will be happy to follow-up with patches.
> 
> But it may be hard to convince everybody without posting code changes
> and often enough showing a patch makes a good argument.

I had the patches ready last night, saw the emails this morning and
decided to go mountain bike for a bit to think about it some more. You
will find my follow-up patches that hopefully implement your recommendation.

Thanks
-- 
Florian

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
> On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
>> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
>>
>>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
>>>
>>>> The enum offers the advantage of centralizing how many different states
>>>> exist for all the platforms we know about in the kernel, it's easy to
>>>> define common values for platforms that have the same semantics, just
>>>> like it's simple to add new values for platform specific details.
>>>
>>> Well, you seem to be liking this, so why don't you just implement it?
>>
>> At the end of his message, Florian wrote:
>>
>>> In any case, just agree and I will be happy to follow-up with patches.
> 
> But it may be hard to convince everybody without posting code changes
> and often enough showing a patch makes a good argument.

I had the patches ready last night, saw the emails this morning and
decided to go mountain bike for a bit to think about it some more. You
will find my follow-up patches that hopefully implement your recommendation.

Thanks
-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote:
> 
> On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
> > On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
> >> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
> >>
> >>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> >>>
> >>>> The enum offers the advantage of centralizing how many different states
> >>>> exist for all the platforms we know about in the kernel, it's easy to
> >>>> define common values for platforms that have the same semantics, just
> >>>> like it's simple to add new values for platform specific details.
> >>>
> >>> Well, you seem to be liking this, so why don't you just implement it?
> >>
> >> At the end of his message, Florian wrote:
> >>
> >>> In any case, just agree and I will be happy to follow-up with patches.
> > 
> > But it may be hard to convince everybody without posting code changes
> > and often enough showing a patch makes a good argument.
> 
> I had the patches ready last night, saw the emails this morning and
> decided to go mountain bike for a bit to think about it some more. You
> will find my follow-up patches that hopefully implement your recommendation.

OK, thanks!

There is one problem with this I missed before, though, sorry about that.

Drivers need to be able to distinguish between suspend-to-idle and the platform
states too, so we need to store the argument passed to suspend_devices_and_enter()
somewhere too, either in the core or in the platform code.

And if we need to store it anyway, let's just store it in the core in a global var
(say pm_suspend_target_state), export that and be done.

There still will be a concern regarding drivers that care about differences between
PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
platform-dependent, but let's defer addressing this until we have a driver
that needs to run on different platforms with different definitions for those
things.

That should address the Pavel's objections too I guess.

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote:
> 
> On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
> > On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
> >> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
> >>
> >>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> >>>
> >>>> The enum offers the advantage of centralizing how many different states
> >>>> exist for all the platforms we know about in the kernel, it's easy to
> >>>> define common values for platforms that have the same semantics, just
> >>>> like it's simple to add new values for platform specific details.
> >>>
> >>> Well, you seem to be liking this, so why don't you just implement it?
> >>
> >> At the end of his message, Florian wrote:
> >>
> >>> In any case, just agree and I will be happy to follow-up with patches.
> > 
> > But it may be hard to convince everybody without posting code changes
> > and often enough showing a patch makes a good argument.
> 
> I had the patches ready last night, saw the emails this morning and
> decided to go mountain bike for a bit to think about it some more. You
> will find my follow-up patches that hopefully implement your recommendation.

OK, thanks!

There is one problem with this I missed before, though, sorry about that.

Drivers need to be able to distinguish between suspend-to-idle and the platform
states too, so we need to store the argument passed to suspend_devices_and_enter()
somewhere too, either in the core or in the platform code.

And if we need to store it anyway, let's just store it in the core in a global var
(say pm_suspend_target_state), export that and be done.

There still will be a concern regarding drivers that care about differences between
PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
platform-dependent, but let's defer addressing this until we have a driver
that needs to run on different platforms with different definitions for those
things.

That should address the Pavel's objections too I guess.

Thanks,
Rafael

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Thursday, July 06, 2017 05:17:50 AM Pavel Machek wrote:
> On Sun 2017-07-16 01:29:57, Rafael J. Wysocki wrote:
> > On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > I had an idea of using an enum type encompassing all of the power states
> > > > > > defined for various platforms and serving both as a registry (to ensure the
> > > > > > uniqueness of the values assigned to the states) and a common ground
> > > > > > between platforms and drivers.
> > > > > > 
> > > > > > Something like:
> > > > > > 
> > > > > > enum platform_target_state {
> > > > > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > > > > 	PLATFORM_STATE_WORKING = 0,
> > > > > > 	PLATFORM_STATE_ACPI_S1,
> > > > > > 	PLATFORM_STATE_ACPI_S2,
> > > > > > 	PLATFORM_STATE_ACPI_S3,
> > > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > > > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > > > > 	...
> > > > > > };
> > > > > > 
> > > > > > and define ->target_state to return a value of this type.
> > > > > > 
> > > > > > Then, if a driver sees one of these and recognizes that value, it should
> > > > > > know exactly what to do.
> > > > > 
> > > > > Remind me why this is good idea?
> > > > 
> > > > Because there are drivers that need to do specific things during
> > > > suspend on a specific board when it goes into a specific state as a
> > > > whole.
> > > 
> > > We have seen driver that cares about voltage to his device being
> > > lost. That's reasonable.
> > > 
> > > Inquiring what the platform target state is... is not.
> > 
> > So why exactly isn't it reasonable?
> > 
> > Please use technical arguments.  Saying that something is wrong without
> > explaining the problem you see with it isn't particulatly useful in technical
> > discussions.
> 
> Deep in your heart, you should know that having enum listing all the platforms linux
> runs on is a very bad idea.

Even so, if I'm unable to explain to people why this is a bad idea in technical
terms, that doesn't mean too much.

I actually noticed an issue with the approach that I missed before, see my last
reply to Florian.

> Anyway, there are better solutions, regulator framework already knows if given rail
> will be powered off or not, and their driver already knows if they are going
> suspend/standby. They just need to use existing interfaces.

So they need to know what has been passed to suspend_devices_and_enter()
anyway and currently there's no interface for that.  That actually is the source
of the whole issue.

Thanks,
Rafael

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Thursday, July 06, 2017 05:17:50 AM Pavel Machek wrote:
> On Sun 2017-07-16 01:29:57, Rafael J. Wysocki wrote:
> > On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > I had an idea of using an enum type encompassing all of the power states
> > > > > > defined for various platforms and serving both as a registry (to ensure the
> > > > > > uniqueness of the values assigned to the states) and a common ground
> > > > > > between platforms and drivers.
> > > > > > 
> > > > > > Something like:
> > > > > > 
> > > > > > enum platform_target_state {
> > > > > > 	PLATFORM_STATE_UNKNOWN = -1,
> > > > > > 	PLATFORM_STATE_WORKING = 0,
> > > > > > 	PLATFORM_STATE_ACPI_S1,
> > > > > > 	PLATFORM_STATE_ACPI_S2,
> > > > > > 	PLATFORM_STATE_ACPI_S3,
> > > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS,
> > > > > > 	PLATFORM_STATE_MY_BOARD_1_GATE_POWER,
> > > > > > 	PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF,
> > > > > > 	...
> > > > > > };
> > > > > > 
> > > > > > and define ->target_state to return a value of this type.
> > > > > > 
> > > > > > Then, if a driver sees one of these and recognizes that value, it should
> > > > > > know exactly what to do.
> > > > > 
> > > > > Remind me why this is good idea?
> > > > 
> > > > Because there are drivers that need to do specific things during
> > > > suspend on a specific board when it goes into a specific state as a
> > > > whole.
> > > 
> > > We have seen driver that cares about voltage to his device being
> > > lost. That's reasonable.
> > > 
> > > Inquiring what the platform target state is... is not.
> > 
> > So why exactly isn't it reasonable?
> > 
> > Please use technical arguments.  Saying that something is wrong without
> > explaining the problem you see with it isn't particulatly useful in technical
> > discussions.
> 
> Deep in your heart, you should know that having enum listing all the platforms linux
> runs on is a very bad idea.

Even so, if I'm unable to explain to people why this is a bad idea in technical
terms, that doesn't mean too much.

I actually noticed an issue with the approach that I missed before, see my last
reply to Florian.

> Anyway, there are better solutions, regulator framework already knows if given rail
> will be powered off or not, and their driver already knows if they are going
> suspend/standby. They just need to use existing interfaces.

So they need to know what has been passed to suspend_devices_and_enter()
anyway and currently there's no interface for that.  That actually is the source
of the whole issue.

Thanks,
Rafael

Re: [PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 07:36:09 PM Florian Fainelli wrote:
> Add an optional platform_suspend_ops callback: target_state, and a
> helper function globally visible to get this called:
> platform_suspend_target_state().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Although this callback is optional and documented as such, it requires
> a platform_suspend_ops::begin callback to be implemented in order to
> provide an accurate suspend/resume state within the driver that
> implements this platform_suspend_ops.
> 
> An enumeration: platform_target_state is defined which currently defines
> the standard ACPI_S[1-4] states and can be extended with platform
> specific suspend states.

This has a couple of problems, but I'm not sure if it is worth to go too much
into details here.

Let's just take a different approach as I said in the other thread.

Thanks,
Rafael

[PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 07:36:09 PM Florian Fainelli wrote:
> Add an optional platform_suspend_ops callback: target_state, and a
> helper function globally visible to get this called:
> platform_suspend_target_state().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Although this callback is optional and documented as such, it requires
> a platform_suspend_ops::begin callback to be implemented in order to
> provide an accurate suspend/resume state within the driver that
> implements this platform_suspend_ops.
> 
> An enumeration: platform_target_state is defined which currently defines
> the standard ACPI_S[1-4] states and can be extended with platform
> specific suspend states.

This has a couple of problems, but I'm not sure if it is worth to go too much
into details here.

Let's just take a different approach as I said in the other thread.

Thanks,
Rafael

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

On 16/07/2017 at 12:22:08 +0200, Rafael J. Wysocki wrote:
> On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote:
> > 
> > On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
> > > On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
> > >> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
> > >>
> > >>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> > >>>
> > >>>> The enum offers the advantage of centralizing how many different states
> > >>>> exist for all the platforms we know about in the kernel, it's easy to
> > >>>> define common values for platforms that have the same semantics, just
> > >>>> like it's simple to add new values for platform specific details.
> > >>>
> > >>> Well, you seem to be liking this, so why don't you just implement it?
> > >>
> > >> At the end of his message, Florian wrote:
> > >>
> > >>> In any case, just agree and I will be happy to follow-up with patches.
> > > 
> > > But it may be hard to convince everybody without posting code changes
> > > and often enough showing a patch makes a good argument.
> > 
> > I had the patches ready last night, saw the emails this morning and
> > decided to go mountain bike for a bit to think about it some more. You
> > will find my follow-up patches that hopefully implement your recommendation.
> 
> OK, thanks!
> 
> There is one problem with this I missed before, though, sorry about that.
> 
> Drivers need to be able to distinguish between suspend-to-idle and the platform
> states too, so we need to store the argument passed to suspend_devices_and_enter()
> somewhere too, either in the core or in the platform code.
> 
> And if we need to store it anyway, let's just store it in the core in a global var
> (say pm_suspend_target_state), export that and be done.
> 
> There still will be a concern regarding drivers that care about differences between
> PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
> platform-dependent, but let's defer addressing this until we have a driver
> that needs to run on different platforms with different definitions for those
> things.
> 

We already have the case for drivers/net/ethernet/cadence/ and
drivers/net/can/m_can/ and many of the at91 drivers. Depending on the
specific SoC they run on, PM_SUSPEND_MEM may or may not cut VDDcore or
may or may not change the peripheral clock.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

On 16/07/2017 at 12:22:08 +0200, Rafael J. Wysocki wrote:
> On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote:
> > 
> > On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
> > > On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
> > >> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
> > >>
> > >>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
> > >>>
> > >>>> The enum offers the advantage of centralizing how many different states
> > >>>> exist for all the platforms we know about in the kernel, it's easy to
> > >>>> define common values for platforms that have the same semantics, just
> > >>>> like it's simple to add new values for platform specific details.
> > >>>
> > >>> Well, you seem to be liking this, so why don't you just implement it?
> > >>
> > >> At the end of his message, Florian wrote:
> > >>
> > >>> In any case, just agree and I will be happy to follow-up with patches.
> > > 
> > > But it may be hard to convince everybody without posting code changes
> > > and often enough showing a patch makes a good argument.
> > 
> > I had the patches ready last night, saw the emails this morning and
> > decided to go mountain bike for a bit to think about it some more. You
> > will find my follow-up patches that hopefully implement your recommendation.
> 
> OK, thanks!
> 
> There is one problem with this I missed before, though, sorry about that.
> 
> Drivers need to be able to distinguish between suspend-to-idle and the platform
> states too, so we need to store the argument passed to suspend_devices_and_enter()
> somewhere too, either in the core or in the platform code.
> 
> And if we need to store it anyway, let's just store it in the core in a global var
> (say pm_suspend_target_state), export that and be done.
> 
> There still will be a concern regarding drivers that care about differences between
> PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
> platform-dependent, but let's defer addressing this until we have a driver
> that needs to run on different platforms with different definitions for those
> things.
> 

We already have the case for drivers/net/ethernet/cadence/ and
drivers/net/can/m_can/ and many of the at91 drivers. Depending on the
specific SoC they run on, PM_SUSPEND_MEM may or may not cut VDDcore or
may or may not change the peripheral clock.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

On 06/07/2017 at 05:18:19 +0200, Pavel Machek wrote:
> On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote:
> > On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
> > > > We already have
> > > > 
> > > > struct regulator_state {
> > > >        int uV; /* suspend voltage */
> > > >        unsigned int mode; /* suspend regulator operating mode */
> > > >        int enabled; /* is regulator enabled in this suspend state */
> > > >        int disabled; /* is the regulator disabled in this suspend state */
> > > > };
> > > > 
> > > >  * struct regulation_constraints - regulator operating constraints.
> > > >   * @state_disk: State for regulator when system is suspended in disk
> > > >   * mode.
> > > >   * @state_mem: State for regulator when system is suspended in mem
> > > >   * mode.
> > > >   * @state_standby: State for regulator when system is suspended in
> > > >   * standby
> > > >   *                 mode.
> > > >    
> > > > . So it seems that maybe we should tell the drivers if we are entering
> > > > "state_mem" or "state_standby" (something I may have opposed, sorry),
> > > > then the driver can get neccessary information from regulator
> > > > framework.
> > > 
> > > OK, so what would be the mechanism to tell these drivers about the
> > > system wide suspend state they are entering if it is not via
> > > platform_suspend_target_state()?
> > > 
> > > Keep in mind that regulators might be one aspect of what could be
> > > causing the platform to behave specifically in one suspend state vs.
> > > another, but there could be pieces of HW within the SoC that can't be
> > > described with power domains, voltage islands etc. that would still have
> > > inherent suspend states properties (like memory retention, pin/pad
> > > controls etc. etc). We still need some mechanism, possibly centralized
> > > 
> > 
> > I concur, the regulator stuff is one aspect of one of our suspend state
> > (cutting VDDcore). But we have another state where the main clock (going
> > to the IPs) is going from a few hundred MHz to 32kHz. This is currently
> > handled by calling at91_suspend_entering_slow_clock(). I think it is
> > important to take that into account so we can remove this hack from the
> > kernel.
> 
> Cure should not be worse then the disease... and it is in this case.
> 
> For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended"
> as regulator framework had. If not, implement it.
> 

See Rafael's comment, currently, the clock framework can't say whether
the clock will change because it doesn't know anything about the suspend
target.

> Same with memory retention, pin/pad controls.
> 

Same here.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

On 06/07/2017 at 05:18:19 +0200, Pavel Machek wrote:
> On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote:
> > On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
> > > > We already have
> > > > 
> > > > struct regulator_state {
> > > >        int uV; /* suspend voltage */
> > > >        unsigned int mode; /* suspend regulator operating mode */
> > > >        int enabled; /* is regulator enabled in this suspend state */
> > > >        int disabled; /* is the regulator disabled in this suspend state */
> > > > };
> > > > 
> > > >  * struct regulation_constraints - regulator operating constraints.
> > > >   * @state_disk: State for regulator when system is suspended in disk
> > > >   * mode.
> > > >   * @state_mem: State for regulator when system is suspended in mem
> > > >   * mode.
> > > >   * @state_standby: State for regulator when system is suspended in
> > > >   * standby
> > > >   *                 mode.
> > > >    
> > > > . So it seems that maybe we should tell the drivers if we are entering
> > > > "state_mem" or "state_standby" (something I may have opposed, sorry),
> > > > then the driver can get neccessary information from regulator
> > > > framework.
> > > 
> > > OK, so what would be the mechanism to tell these drivers about the
> > > system wide suspend state they are entering if it is not via
> > > platform_suspend_target_state()?
> > > 
> > > Keep in mind that regulators might be one aspect of what could be
> > > causing the platform to behave specifically in one suspend state vs.
> > > another, but there could be pieces of HW within the SoC that can't be
> > > described with power domains, voltage islands etc. that would still have
> > > inherent suspend states properties (like memory retention, pin/pad
> > > controls etc. etc). We still need some mechanism, possibly centralized
> > > 
> > 
> > I concur, the regulator stuff is one aspect of one of our suspend state
> > (cutting VDDcore). But we have another state where the main clock (going
> > to the IPs) is going from a few hundred MHz to 32kHz. This is currently
> > handled by calling at91_suspend_entering_slow_clock(). I think it is
> > important to take that into account so we can remove this hack from the
> > kernel.
> 
> Cure should not be worse then the disease... and it is in this case.
> 
> For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended"
> as regulator framework had. If not, implement it.
> 

See Rafael's comment, currently, the clock framework can't say whether
the clock will change because it doesn't know anything about the suspend
target.

> Same with memory retention, pin/pad controls.
> 

Same here.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/16/2017 06:41 AM, Alexandre Belloni wrote:
> On 06/07/2017 at 05:18:19 +0200, Pavel Machek wrote:
>> On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote:
>>> On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
>>>>> We already have
>>>>>
>>>>> struct regulator_state {
>>>>>        int uV; /* suspend voltage */
>>>>>        unsigned int mode; /* suspend regulator operating mode */
>>>>>        int enabled; /* is regulator enabled in this suspend state */
>>>>>        int disabled; /* is the regulator disabled in this suspend state */
>>>>> };
>>>>>
>>>>>  * struct regulation_constraints - regulator operating constraints.
>>>>>   * @state_disk: State for regulator when system is suspended in disk
>>>>>   * mode.
>>>>>   * @state_mem: State for regulator when system is suspended in mem
>>>>>   * mode.
>>>>>   * @state_standby: State for regulator when system is suspended in
>>>>>   * standby
>>>>>   *                 mode.
>>>>>    
>>>>> . So it seems that maybe we should tell the drivers if we are entering
>>>>> "state_mem" or "state_standby" (something I may have opposed, sorry),
>>>>> then the driver can get neccessary information from regulator
>>>>> framework.
>>>>
>>>> OK, so what would be the mechanism to tell these drivers about the
>>>> system wide suspend state they are entering if it is not via
>>>> platform_suspend_target_state()?
>>>>
>>>> Keep in mind that regulators might be one aspect of what could be
>>>> causing the platform to behave specifically in one suspend state vs.
>>>> another, but there could be pieces of HW within the SoC that can't be
>>>> described with power domains, voltage islands etc. that would still have
>>>> inherent suspend states properties (like memory retention, pin/pad
>>>> controls etc. etc). We still need some mechanism, possibly centralized
>>>>
>>>
>>> I concur, the regulator stuff is one aspect of one of our suspend state
>>> (cutting VDDcore). But we have another state where the main clock (going
>>> to the IPs) is going from a few hundred MHz to 32kHz. This is currently
>>> handled by calling at91_suspend_entering_slow_clock(). I think it is
>>> important to take that into account so we can remove this hack from the
>>> kernel.
>>
>> Cure should not be worse then the disease... and it is in this case.
>>
>> For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended"
>> as regulator framework had. If not, implement it.
>>
> 
> See Rafael's comment, currently, the clock framework can't say whether
> the clock will change because it doesn't know anything about the suspend
> target.
> 
>> Same with memory retention, pin/pad controls.
>>
> 
> Same here.

Exactly, here is another side effect of not knowing the platform
suspend/state that I came across on our platforms:

https://patchwork.kernel.org/patch/9561575/

(we later discussed this in details with Linus and this is why this very
patch set is being introduced now)
-- 
Florian

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/16/2017 06:41 AM, Alexandre Belloni wrote:
> On 06/07/2017 at 05:18:19 +0200, Pavel Machek wrote:
>> On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote:
>>> On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:
>>>>> We already have
>>>>>
>>>>> struct regulator_state {
>>>>>        int uV; /* suspend voltage */
>>>>>        unsigned int mode; /* suspend regulator operating mode */
>>>>>        int enabled; /* is regulator enabled in this suspend state */
>>>>>        int disabled; /* is the regulator disabled in this suspend state */
>>>>> };
>>>>>
>>>>>  * struct regulation_constraints - regulator operating constraints.
>>>>>   * @state_disk: State for regulator when system is suspended in disk
>>>>>   * mode.
>>>>>   * @state_mem: State for regulator when system is suspended in mem
>>>>>   * mode.
>>>>>   * @state_standby: State for regulator when system is suspended in
>>>>>   * standby
>>>>>   *                 mode.
>>>>>    
>>>>> . So it seems that maybe we should tell the drivers if we are entering
>>>>> "state_mem" or "state_standby" (something I may have opposed, sorry),
>>>>> then the driver can get neccessary information from regulator
>>>>> framework.
>>>>
>>>> OK, so what would be the mechanism to tell these drivers about the
>>>> system wide suspend state they are entering if it is not via
>>>> platform_suspend_target_state()?
>>>>
>>>> Keep in mind that regulators might be one aspect of what could be
>>>> causing the platform to behave specifically in one suspend state vs.
>>>> another, but there could be pieces of HW within the SoC that can't be
>>>> described with power domains, voltage islands etc. that would still have
>>>> inherent suspend states properties (like memory retention, pin/pad
>>>> controls etc. etc). We still need some mechanism, possibly centralized
>>>>
>>>
>>> I concur, the regulator stuff is one aspect of one of our suspend state
>>> (cutting VDDcore). But we have another state where the main clock (going
>>> to the IPs) is going from a few hundred MHz to 32kHz. This is currently
>>> handled by calling at91_suspend_entering_slow_clock(). I think it is
>>> important to take that into account so we can remove this hack from the
>>> kernel.
>>
>> Cure should not be worse then the disease... and it is in this case.
>>
>> For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended"
>> as regulator framework had. If not, implement it.
>>
> 
> See Rafael's comment, currently, the clock framework can't say whether
> the clock will change because it doesn't know anything about the suspend
> target.
> 
>> Same with memory retention, pin/pad controls.
>>
> 
> Same here.

Exactly, here is another side effect of not knowing the platform
suspend/state that I came across on our platforms:

https://patchwork.kernel.org/patch/9561575/

(we later discussed this in details with Linus and this is why this very
patch set is being introduced now)
-- 
Florian

Re: [PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/05/2017 08:18 PM, Pavel Machek wrote:
> Hi!
> 
>> Although this callback is optional and documented as such, it requires
>> a platform_suspend_ops::begin callback to be implemented in order to
>> provide an accurate suspend/resume state within the driver that
>> implements this platform_suspend_ops.
>>
>> An enumeration: platform_target_state is defined which currently defines
>> the standard ACPI_S[1-4] states and can be extended with platform
>> specific suspend states.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/linux/suspend.h | 25 +++++++++++++++++++++++++
>>  kernel/power/suspend.c  | 15 +++++++++++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 0b1cf32edfd7..6e6cc0778816 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -50,6 +50,16 @@ enum suspend_stat_step {
>>  	SUSPEND_RESUME
>>  };
>>  
>> +enum platform_target_state {
>> +	PLATFORM_STATE_UNKNOWN = -1,
>> +	PLATFORM_STATE_WORKING = 0,
>> +	PLATFORM_STATE_ACPI_S1,
>> +	PLATFORM_STATE_ACPI_S2,
>> +	PLATFORM_STATE_ACPI_S3,
>> +	PLATFORM_STATE_ACPI_S4,
>> +	/* Add platform specific states here */
>> +};
>> +
> 
> As I tried to explain in the email thread, having list with all the possible platform
> states is no-go. We have about 1000 platforms supported...

FYI, the recent (relatively recent) CPU hotplug conversion from
notifiers to a state machine has a similar pattern whereby pieces of
code needing to hook into the CPU hotplug state machine add their own
enum values as they need. So far it's been working for them, and there
were tons of CPU hotplug notifiers in the kernel.

Anyhow, let me implement Rafael's suggestions and we can see how we move
from there.
-- 
Florian

[PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/05/2017 08:18 PM, Pavel Machek wrote:
> Hi!
> 
>> Although this callback is optional and documented as such, it requires
>> a platform_suspend_ops::begin callback to be implemented in order to
>> provide an accurate suspend/resume state within the driver that
>> implements this platform_suspend_ops.
>>
>> An enumeration: platform_target_state is defined which currently defines
>> the standard ACPI_S[1-4] states and can be extended with platform
>> specific suspend states.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/linux/suspend.h | 25 +++++++++++++++++++++++++
>>  kernel/power/suspend.c  | 15 +++++++++++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 0b1cf32edfd7..6e6cc0778816 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -50,6 +50,16 @@ enum suspend_stat_step {
>>  	SUSPEND_RESUME
>>  };
>>  
>> +enum platform_target_state {
>> +	PLATFORM_STATE_UNKNOWN = -1,
>> +	PLATFORM_STATE_WORKING = 0,
>> +	PLATFORM_STATE_ACPI_S1,
>> +	PLATFORM_STATE_ACPI_S2,
>> +	PLATFORM_STATE_ACPI_S3,
>> +	PLATFORM_STATE_ACPI_S4,
>> +	/* Add platform specific states here */
>> +};
>> +
> 
> As I tried to explain in the email thread, having list with all the possible platform
> states is no-go. We have about 1000 platforms supported...

FYI, the recent (relatively recent) CPU hotplug conversion from
notifiers to a state machine has a similar pattern whereby pieces of
code needing to hook into the CPU hotplug state machine add their own
enum values as they need. So far it's been working for them, and there
were tons of CPU hotplug notifiers in the kernel.

Anyhow, let me implement Rafael's suggestions and we can see how we move
from there.
-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/16/2017 03:22 AM, Rafael J. Wysocki wrote:
> On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote:
>>
>> On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
>>> On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
>>>> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
>>>>
>>>>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
>>>>>
>>>>>> The enum offers the advantage of centralizing how many different states
>>>>>> exist for all the platforms we know about in the kernel, it's easy to
>>>>>> define common values for platforms that have the same semantics, just
>>>>>> like it's simple to add new values for platform specific details.
>>>>>
>>>>> Well, you seem to be liking this, so why don't you just implement it?
>>>>
>>>> At the end of his message, Florian wrote:
>>>>
>>>>> In any case, just agree and I will be happy to follow-up with patches.
>>>
>>> But it may be hard to convince everybody without posting code changes
>>> and often enough showing a patch makes a good argument.
>>
>> I had the patches ready last night, saw the emails this morning and
>> decided to go mountain bike for a bit to think about it some more. You
>> will find my follow-up patches that hopefully implement your recommendation.
> 
> OK, thanks!
> 
> There is one problem with this I missed before, though, sorry about that.
> 
> Drivers need to be able to distinguish between suspend-to-idle and the platform
> states too, so we need to store the argument passed to suspend_devices_and_enter()
> somewhere too, either in the core or in the platform code.
> 
> And if we need to store it anyway, let's just store it in the core in a global var
> (say pm_suspend_target_state), export that and be done.

I was not sure this would be acceptable which was why I opted for making
suspend_ops::begin store the state passed from suspend_ops::enter, I
will change that.

> 
> There still will be a concern regarding drivers that care about differences between
> PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
> platform-dependent, but let's defer addressing this until we have a driver
> that needs to run on different platforms with different definitions for those
> things.

Makes sense, thanks!

> 
> That should address the Pavel's objections too I guess.
> 
> Thanks,
> Rafael
> 

-- 
Florian

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/16/2017 03:22 AM, Rafael J. Wysocki wrote:
> On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote:
>>
>> On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote:
>>> On Sunday, July 16, 2017 01:34:53 AM Mason wrote:
>>>> On 16/07/2017 01:24, Rafael J. Wysocki wrote:
>>>>
>>>>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote:
>>>>>
>>>>>> The enum offers the advantage of centralizing how many different states
>>>>>> exist for all the platforms we know about in the kernel, it's easy to
>>>>>> define common values for platforms that have the same semantics, just
>>>>>> like it's simple to add new values for platform specific details.
>>>>>
>>>>> Well, you seem to be liking this, so why don't you just implement it?
>>>>
>>>> At the end of his message, Florian wrote:
>>>>
>>>>> In any case, just agree and I will be happy to follow-up with patches.
>>>
>>> But it may be hard to convince everybody without posting code changes
>>> and often enough showing a patch makes a good argument.
>>
>> I had the patches ready last night, saw the emails this morning and
>> decided to go mountain bike for a bit to think about it some more. You
>> will find my follow-up patches that hopefully implement your recommendation.
> 
> OK, thanks!
> 
> There is one problem with this I missed before, though, sorry about that.
> 
> Drivers need to be able to distinguish between suspend-to-idle and the platform
> states too, so we need to store the argument passed to suspend_devices_and_enter()
> somewhere too, either in the core or in the platform code.
> 
> And if we need to store it anyway, let's just store it in the core in a global var
> (say pm_suspend_target_state), export that and be done.

I was not sure this would be acceptable which was why I opted for making
suspend_ops::begin store the state passed from suspend_ops::enter, I
will change that.

> 
> There still will be a concern regarding drivers that care about differences between
> PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
> platform-dependent, but let's defer addressing this until we have a driver
> that needs to run on different platforms with different definitions for those
> things.

Makes sense, thanks!

> 
> That should address the Pavel's objections too I guess.
> 
> Thanks,
> Rafael
> 

-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

Hi!

> > > So why exactly isn't it reasonable?
> > > 
> > > Please use technical arguments.  Saying that something is wrong without
> > > explaining the problem you see with it isn't particulatly useful in technical
> > > discussions.
> > 
> > Deep in your heart, you should know that having enum listing all the platforms linux
> > runs on is a very bad idea.
> 
> Even so, if I'm unable to explain to people why this is a bad idea in technical
> terms, that doesn't mean too much.

I could say something O(#drivers * #platforms) vs. O(#drivers +
#platforms) lines of code -- but I thought it was obvious...?

> > Anyway, there are better solutions, regulator framework already knows if given rail
> > will be powered off or not, and their driver already knows if they are going
> > suspend/standby. They just need to use existing interfaces.
> 
> So they need to know what has been passed to suspend_devices_and_enter()
> anyway and currently there's no interface for that.  That actually is the source
> of the whole issue.

Yep, I don't like that, but I guess we should give drivers enough
information to ask regulator framework.

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

Hi!

> > > So why exactly isn't it reasonable?
> > > 
> > > Please use technical arguments.  Saying that something is wrong without
> > > explaining the problem you see with it isn't particulatly useful in technical
> > > discussions.
> > 
> > Deep in your heart, you should know that having enum listing all the platforms linux
> > runs on is a very bad idea.
> 
> Even so, if I'm unable to explain to people why this is a bad idea in technical
> terms, that doesn't mean too much.

I could say something O(#drivers * #platforms) vs. O(#drivers +
#platforms) lines of code -- but I thought it was obvious...?

> > Anyway, there are better solutions, regulator framework already knows if given rail
> > will be powered off or not, and their driver already knows if they are going
> > suspend/standby. They just need to use existing interfaces.
> 
> So they need to know what has been passed to suspend_devices_and_enter()
> anyway and currently there's no interface for that.  That actually is the source
> of the whole issue.

Yep, I don't like that, but I guess we should give drivers enough
information to ask regulator framework.

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170716/5b0418fd/attachment.sig>

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

Hi!

> > There still will be a concern regarding drivers that care about differences between
> > PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
> > platform-dependent, but let's defer addressing this until we have a driver
> > that needs to run on different platforms with different definitions for those
> > things.
> > 
> 
> We already have the case for drivers/net/ethernet/cadence/ and
> drivers/net/can/m_can/ and many of the at91 drivers. Depending on the
> specific SoC they run on, PM_SUSPEND_MEM may or may not cut VDDcore or
> may or may not change the peripheral clock.

Please please introduce will_vddcore_be_cut_down() or similar helper,
so that we have one place to fix..

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

Hi!

> > There still will be a concern regarding drivers that care about differences between
> > PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are
> > platform-dependent, but let's defer addressing this until we have a driver
> > that needs to run on different platforms with different definitions for those
> > things.
> > 
> 
> We already have the case for drivers/net/ethernet/cadence/ and
> drivers/net/can/m_can/ and many of the at91 drivers. Depending on the
> specific SoC they run on, PM_SUSPEND_MEM may or may not cut VDDcore or
> may or may not change the peripheral clock.

Please please introduce will_vddcore_be_cut_down() or similar helper,
so that we have one place to fix..

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170716/382ea0f6/attachment.sig>

[PATCH v2] PM / suspend: Add suspend_target_state()

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via suspend_target_state() in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()

 include/linux/suspend.h |  2 ++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..7b70e7d6a006 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -202,6 +202,7 @@ struct platform_freeze_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern suspend_state_t suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -281,6 +282,7 @@ static inline bool pm_resume_via_firmware(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline suspend_state_t suspend_target_state(void) { return -ENOSYS; }
 static inline bool idle_should_freeze(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..a296d6e25d52 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,7 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+static suspend_state_t pm_suspend_target_state;
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -202,6 +203,18 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 /**
+ * suspend_target_state - Return the system wide suspend state.
+ *
+ * The pm_suspend_target_state becomes valid during
+ * suspend_devices_and_enter().
+ */
+suspend_state_t suspend_target_state(void)
+{
+	return pm_suspend_target_state;
+}
+EXPORT_SYMBOL_GPL(suspend_target_state);
+
+/**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
  * Platform drivers that implement mem suspend only and only need to check for
@@ -456,6 +469,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
-- 
2.9.3

[PATCH v2] PM / suspend: Add suspend_target_state()

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via suspend_target_state() in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()

 include/linux/suspend.h |  2 ++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..7b70e7d6a006 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -202,6 +202,7 @@ struct platform_freeze_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern suspend_state_t suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -281,6 +282,7 @@ static inline bool pm_resume_via_firmware(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline suspend_state_t suspend_target_state(void) { return -ENOSYS; }
 static inline bool idle_should_freeze(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..a296d6e25d52 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,7 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+static suspend_state_t pm_suspend_target_state;
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -202,6 +203,18 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 /**
+ * suspend_target_state - Return the system wide suspend state.
+ *
+ * The pm_suspend_target_state becomes valid during
+ * suspend_devices_and_enter().
+ */
+suspend_state_t suspend_target_state(void)
+{
+	return pm_suspend_target_state;
+}
+EXPORT_SYMBOL_GPL(suspend_target_state);
+
+/**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
  * Platform drivers that implement mem suspend only and only need to check for
@@ -456,6 +469,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
-- 
2.9.3

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

Hi!

> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via suspend_target_state() in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().

Do we really want to have variable + inline functions that just read
that variable?

> +static inline suspend_state_t suspend_target_state(void) { return  -ENOSYS; }

I'm pretty sure -ENOSYS is not compatible with suspend_state_t ...

> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3ecf275d7e44..a296d6e25d52 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -47,6 +47,7 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
>  
>  suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
>  static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
> +static suspend_state_t pm_suspend_target_state;

Is there disadvantage of just having this variable non-static?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH v2] PM / suspend: Add suspend_target_state()

[Pavel Machek]

Hi!

> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via suspend_target_state() in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().

Do we really want to have variable + inline functions that just read
that variable?

> +static inline suspend_state_t suspend_target_state(void) { return  -ENOSYS; }

I'm pretty sure -ENOSYS is not compatible with suspend_state_t ...

> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3ecf275d7e44..a296d6e25d52 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -47,6 +47,7 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
>  
>  suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
>  static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
> +static suspend_state_t pm_suspend_target_state;

Is there disadvantage of just having this variable non-static?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170717/40b9fdf6/attachment.sig>

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Rafael J. Wysocki]

On Mon, Jul 17, 2017 at 10:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> Have the core suspend/resume framework store the system-wide suspend
>> state (suspend_state_t) we are about to enter, and expose it to drivers
>> via suspend_target_state() in order to retrieve that. The state is
>> assigned in suspend_devices_and_enter().
>
> Do we really want to have variable + inline functions that just read
> that variable?

Florian, Pavel is right, you can simply export the variable.

Anything accessing it should go under CONFIG_PM_SLEEP anyway.

Thanks,
Rafael

[PATCH v2] PM / suspend: Add suspend_target_state()

[Rafael J. Wysocki]

On Mon, Jul 17, 2017 at 10:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> Have the core suspend/resume framework store the system-wide suspend
>> state (suspend_state_t) we are about to enter, and expose it to drivers
>> via suspend_target_state() in order to retrieve that. The state is
>> assigned in suspend_devices_and_enter().
>
> Do we really want to have variable + inline functions that just read
> that variable?

Florian, Pavel is right, you can simply export the variable.

Anything accessing it should go under CONFIG_PM_SLEEP anyway.

Thanks,
Rafael

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Florian Fainelli]

On 07/17/2017 02:03 PM, Rafael J. Wysocki wrote:
> On Mon, Jul 17, 2017 at 10:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>>> Have the core suspend/resume framework store the system-wide suspend
>>> state (suspend_state_t) we are about to enter, and expose it to drivers
>>> via suspend_target_state() in order to retrieve that. The state is
>>> assigned in suspend_devices_and_enter().
>>
>> Do we really want to have variable + inline functions that just read
>> that variable?
> 
> Florian, Pavel is right, you can simply export the variable.
> 
> Anything accessing it should go under CONFIG_PM_SLEEP anyway.

Alright then, I will just export it. Stay tuned.
-- 
Florian

[PATCH v2] PM / suspend: Add suspend_target_state()

[Florian Fainelli]

On 07/17/2017 02:03 PM, Rafael J. Wysocki wrote:
> On Mon, Jul 17, 2017 at 10:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>>> Have the core suspend/resume framework store the system-wide suspend
>>> state (suspend_state_t) we are about to enter, and expose it to drivers
>>> via suspend_target_state() in order to retrieve that. The state is
>>> assigned in suspend_devices_and_enter().
>>
>> Do we really want to have variable + inline functions that just read
>> that variable?
> 
> Florian, Pavel is right, you can simply export the variable.
> 
> Anything accessing it should go under CONFIG_PM_SLEEP anyway.

Alright then, I will just export it. Stay tuned.
-- 
Florian

[PATCH v3] PM / suspend: Export pm_suspend_target_state

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via pm_suspend_target_state in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v3:

- just export pm_suspend_target_state without a helper function

Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()

 include/linux/suspend.h | 1 +
 kernel/power/suspend.c  | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..2159f6841768 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 /* drivers/base/power/wakeup.c */
 extern bool events_check_enabled;
 extern unsigned int pm_wakeup_irq;
+extern suspend_state_t pm_suspend_target_state;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..1aecdaf22ab5 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+suspend_state_t pm_suspend_target_state;
+EXPORT_SYMBOL_GPL(pm_suspend_target_state);
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
-- 
2.9.3

[PATCH v3] PM / suspend: Export pm_suspend_target_state

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via pm_suspend_target_state in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v3:

- just export pm_suspend_target_state without a helper function

Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()

 include/linux/suspend.h | 1 +
 kernel/power/suspend.c  | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..2159f6841768 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 /* drivers/base/power/wakeup.c */
 extern bool events_check_enabled;
 extern unsigned int pm_wakeup_irq;
+extern suspend_state_t pm_suspend_target_state;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..1aecdaf22ab5 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+suspend_state_t pm_suspend_target_state;
+EXPORT_SYMBOL_GPL(pm_suspend_target_state);
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
-- 
2.9.3

Re: [PATCH v3] PM / suspend: Export pm_suspend_target_state

[Rafael J. Wysocki]

On Monday, July 17, 2017 03:10:59 PM Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via pm_suspend_target_state in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
> 
> - just export pm_suspend_target_state without a helper function
> 
> Changes in v2:
> 
> - rename platform_suspend_target_state() -> suspend_target_state()
> - directly export the suspend_state_t value and assign it in
>   suspend_devices_and_enter()
> 
>  include/linux/suspend.h | 1 +
>  kernel/power/suspend.c  | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 0b1cf32edfd7..2159f6841768 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
>  /* drivers/base/power/wakeup.c */
>  extern bool events_check_enabled;
>  extern unsigned int pm_wakeup_irq;
> +extern suspend_state_t pm_suspend_target_state;
>  
>  extern bool pm_wakeup_pending(void);
>  extern void pm_system_wakeup(void);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3ecf275d7e44..1aecdaf22ab5 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
>  
>  suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
>  static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
> +suspend_state_t pm_suspend_target_state;
> +EXPORT_SYMBOL_GPL(pm_suspend_target_state);
>  
>  unsigned int pm_suspend_global_flags;
>  EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
> @@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
>  	if (!sleep_state_supported(state))
>  		return -ENOSYS;
>  
> +	pm_suspend_target_state = state;
> +
>  	error = platform_suspend_begin(state);
>  	if (error)
>  		goto Close;
> 

And please clear pm_suspend_target_state before returning from
suspend_devices_and_enter().

Thanks,
Rafael

[PATCH v3] PM / suspend: Export pm_suspend_target_state

[Rafael J. Wysocki]

On Monday, July 17, 2017 03:10:59 PM Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via pm_suspend_target_state in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
> 
> - just export pm_suspend_target_state without a helper function
> 
> Changes in v2:
> 
> - rename platform_suspend_target_state() -> suspend_target_state()
> - directly export the suspend_state_t value and assign it in
>   suspend_devices_and_enter()
> 
>  include/linux/suspend.h | 1 +
>  kernel/power/suspend.c  | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 0b1cf32edfd7..2159f6841768 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
>  /* drivers/base/power/wakeup.c */
>  extern bool events_check_enabled;
>  extern unsigned int pm_wakeup_irq;
> +extern suspend_state_t pm_suspend_target_state;
>  
>  extern bool pm_wakeup_pending(void);
>  extern void pm_system_wakeup(void);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3ecf275d7e44..1aecdaf22ab5 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
>  
>  suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
>  static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
> +suspend_state_t pm_suspend_target_state;
> +EXPORT_SYMBOL_GPL(pm_suspend_target_state);
>  
>  unsigned int pm_suspend_global_flags;
>  EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
> @@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
>  	if (!sleep_state_supported(state))
>  		return -ENOSYS;
>  
> +	pm_suspend_target_state = state;
> +
>  	error = platform_suspend_begin(state);
>  	if (error)
>  		goto Close;
> 

And please clear pm_suspend_target_state before returning from
suspend_devices_and_enter().

Thanks,
Rafael

[PATCH v4] PM / suspend: Export pm_suspend_target_state

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via pm_suspend_target_state in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v4:
- clear pm_suspend_target_state in Close label

Changes in v3:

- just export pm_suspend_target_state without a helper function

Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()


 include/linux/suspend.h | 1 +
 kernel/power/suspend.c  | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..2159f6841768 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 /* drivers/base/power/wakeup.c */
 extern bool events_check_enabled;
 extern unsigned int pm_wakeup_irq;
+extern suspend_state_t pm_suspend_target_state;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..d0c0b96c2383 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+suspend_state_t pm_suspend_target_state;
+EXPORT_SYMBOL_GPL(pm_suspend_target_state);
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
@@ -485,6 +489,7 @@ int suspend_devices_and_enter(suspend_state_t state)
 
  Close:
 	platform_resume_end(state);
+	pm_suspend_target_state = PM_SUSPEND_ON;
 	return error;
 
  Recover_platform:
-- 
2.9.3

[PATCH v4] PM / suspend: Export pm_suspend_target_state

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via pm_suspend_target_state in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

This is useful for platform specific drivers that may need to take a
slightly different suspend/resume path based on the system's
suspend/resume state being entered.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v4:
- clear pm_suspend_target_state in Close label

Changes in v3:

- just export pm_suspend_target_state without a helper function

Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()


 include/linux/suspend.h | 1 +
 kernel/power/suspend.c  | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..2159f6841768 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 /* drivers/base/power/wakeup.c */
 extern bool events_check_enabled;
 extern unsigned int pm_wakeup_irq;
+extern suspend_state_t pm_suspend_target_state;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..d0c0b96c2383 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+suspend_state_t pm_suspend_target_state;
+EXPORT_SYMBOL_GPL(pm_suspend_target_state);
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
@@ -485,6 +489,7 @@ int suspend_devices_and_enter(suspend_state_t state)
 
  Close:
 	platform_resume_end(state);
+	pm_suspend_target_state = PM_SUSPEND_ON;
 	return error;
 
  Recover_platform:
-- 
2.9.3

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Mon 2017-07-17 13:06:28, Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via suspend_target_state() in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH v2] PM / suspend: Add suspend_target_state()

[Pavel Machek]

On Mon 2017-07-17 13:06:28, Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via suspend_target_state() in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170720/676f552a/attachment.sig>

Re: [PATCH v4] PM / suspend: Export pm_suspend_target_state

[Rafael J. Wysocki]

On Monday, July 17, 2017 05:19:25 PM Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via pm_suspend_target_state in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v4:
> - clear pm_suspend_target_state in Close label
> 
> Changes in v3:
> 
> - just export pm_suspend_target_state without a helper function
> 
> Changes in v2:
> 
> - rename platform_suspend_target_state() -> suspend_target_state()
> - directly export the suspend_state_t value and assign it in
>   suspend_devices_and_enter()

Applied, thanks!

[PATCH v4] PM / suspend: Export pm_suspend_target_state

[Rafael J. Wysocki]

On Monday, July 17, 2017 05:19:25 PM Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via pm_suspend_target_state in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> This is useful for platform specific drivers that may need to take a
> slightly different suspend/resume path based on the system's
> suspend/resume state being entered.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v4:
> - clear pm_suspend_target_state in Close label
> 
> Changes in v3:
> 
> - just export pm_suspend_target_state without a helper function
> 
> Changes in v2:
> 
> - rename platform_suspend_target_state() -> suspend_target_state()
> - directly export the suspend_state_t value and assign it in
>   suspend_devices_and_enter()

Applied, thanks!