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.

[Mason]

Post by Mason
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

[Pavel Machek]

Post by Mason

Post by Mason
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.

Post by Mason
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

[Mason]

Post by Pavel Machek

Post by Mason

Post by Mason
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.

Post by Pavel Machek
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?

Post by Pavel Machek

Post by Mason
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.

[Florian Fainelli]

Post by Mason
Hello,
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.

Post by Mason
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.)

[Rafael J. Wysocki]

Hi,

Post by Mason
Hello,
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

[Florian Fainelli]

Post by Rafael J. Wysocki
Hi,

Post by Mason
Hello,
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.

Post by Rafael J. Wysocki
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!

[Rafael J. Wysocki]

Post by Florian Fainelli

Post by Rafael J. Wysocki
Hi,

Post by Mason
Hello,
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.

Post by Rafael J. Wysocki
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.
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

[Florian Fainelli]

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki
Hi,

Post by Mason
Hello,
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.

Post by Rafael J. Wysocki
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.
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 */
}

[Rafael J. Wysocki]

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki
Hi,

Post by Mason
Hello,
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.

Post by Rafael J. Wysocki
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.
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.

Post by Florian Fainelli
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
static int drv_suspend(struct device d)
{
suspend_state_t state = suspend_get_state();
switch (state) {
return 0;
/* 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

[Florian Fainelli]

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki
Hi,

Post by Mason
Hello,
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.

Post by Rafael J. Wysocki
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.
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.

Post by Florian Fainelli
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
static int drv_suspend(struct device d)
{
suspend_state_t state = suspend_get_state();
switch (state) {
return 0;
/* 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!

[Rafael J. Wysocki]

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki
Hi,

Post by Mason
Hello,
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.

Post by Rafael J. Wysocki
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.
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.

Post by Florian Fainelli
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
static int drv_suspend(struct device d)
{
suspend_state_t state = suspend_get_state();
switch (state) {
return 0;
/* 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.

Post by Florian Fainelli
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

[Florian Fainelli]

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki
Hi,

Post by Mason
Hello,
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.

Post by Rafael J. Wysocki
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.
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.

Post by Florian Fainelli
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
static int drv_suspend(struct device d)
{
suspend_state_t state = suspend_get_state();
switch (state) {
return 0;
/* 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.

Post by Rafael J. Wysocki
ACPI has acpi_target_system_state() for this very purpose, for example.

Post by Florian Fainelli
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.

[Rafael J. Wysocki]

[cut]

Post by Florian Fainelli
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
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.

Post by Rafael J. Wysocki
ACPI has acpi_target_system_state() for this very purpose, for example.

Post by Florian Fainelli
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

[Alexandre Belloni]

Post by Florian Fainelli

Post by Rafael J. Wysocki
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.

[Rafael J. Wysocki]

Post by Alexandre Belloni

Post by Florian Fainelli

Post by Rafael J. Wysocki
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.

- whether main clock is switched from the master clock to the slow
clock
- whether VDDcore will be cut
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

[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(+)

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

[Rafael J. Wysocki]

Post by Florian Fainelli
Add an optional platform_suspend_ops callback: target_state, and a
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.
---
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.
+ *
+ * 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
+ * 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).

Post by Florian Fainelli
};

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

[Florian Fainelli]

Post by Rafael J. Wysocki

Post by Florian Fainelli
Add an optional platform_suspend_ops callback: target_state, and a
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.
---
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.
+ *
+ * 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
+ * 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).

Post by Florian Fainelli
};

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!

[Rafael J. Wysocki]

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli
Add an optional platform_suspend_ops callback: target_state, and a
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.
---
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.
+ *
+ * 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
+ * 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).

Post by Florian Fainelli
};

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

[Pavel Machek]

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli
Add an optional platform_suspend_ops callback: target_state, and a
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.
---
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.
+ *
+ * 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
+ * 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).

Post by Florian Fainelli
};

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

[Rafael J. Wysocki]

Post by Pavel Machek

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Florian Fainelli
Add an optional platform_suspend_ops callback: target_state, and a
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.
---
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.
+ *
+ * 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
+ * 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).

Post by Florian Fainelli
};

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

Post by Pavel Machek
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.

Post by Pavel Machek
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

[Pavel Machek]

Hi!

Post by Rafael J. Wysocki

Post by Pavel Machek

Post by Rafael J. Wysocki
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.
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.

Post by Rafael J. Wysocki

Post by Pavel Machek
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

[Florian Fainelli]

Post by Pavel Machek
Hi!

Post by Rafael J. Wysocki

Post by Pavel Machek

Post by Rafael J. Wysocki
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.
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.

Post by Rafael J. Wysocki

Post by Pavel Machek
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?

Post by Pavel Machek
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.
* mode.
* mode.
* 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

Post by Pavel Machek
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.

[Alexandre Belloni]

Post by Florian Fainelli

Post by Pavel Machek
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.
* mode.
* mode.
* 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.

[Pavel Machek]

Post by Alexandre Belloni

Post by Florian Fainelli

Post by Pavel Machek
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.
* mode.
* mode.
* 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

[Alexandre Belloni]

Post by Pavel Machek

Post by Alexandre Belloni

Post by Florian Fainelli

Post by Pavel Machek
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.
* mode.
* mode.
* 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.

Post by Pavel Machek
Same with memory retention, pin/pad controls.

Same here.

[Florian Fainelli]

Post by Alexandre Belloni

Post by Pavel Machek

Post by Alexandre Belloni

Post by Florian Fainelli

Post by Pavel Machek
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.
* mode.
* mode.
* 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.

Post by Pavel Machek
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)

[Rafael J. Wysocki]

Post by Florian Fainelli

Post by Pavel Machek
Hi!

Post by Rafael J. Wysocki

Post by Pavel Machek

Post by Rafael J. Wysocki
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.
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.

Post by Rafael J. Wysocki

Post by Pavel Machek
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

[Mason]

Post by Rafael J. Wysocki

Post by Florian Fainelli
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?
In any case, just agree and I will be happy to follow-up with patches.

Regards.

[Rafael J. Wysocki]

Post by Rafael J. Wysocki

Post by Florian Fainelli
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?
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

[Florian Fainelli]

Post by Rafael J. Wysocki

Post by Rafael J. Wysocki

Post by Florian Fainelli
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?
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

[Rafael J. Wysocki]

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Rafael J. Wysocki

Post by Florian Fainelli
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?
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

[Alexandre Belloni]

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Rafael J. Wysocki

Post by Florian Fainelli
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?
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.

[Pavel Machek]

Hi!

Post by Alexandre Belloni

Post by Rafael J. Wysocki
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

[Florian Fainelli]

Post by Rafael J. Wysocki

Post by Florian Fainelli

Post by Rafael J. Wysocki

Post by Rafael J. Wysocki

Post by Florian Fainelli
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?
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.

Post by Rafael J. Wysocki
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!

Post by Rafael J. Wysocki
That should address the Pavel's objections too I guess.
Thanks,
Rafael

[Rafael J. Wysocki]

Post by Pavel Machek
Hi!

Post by Rafael J. Wysocki

Post by Pavel Machek

Post by Rafael J. Wysocki
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.
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

[Pavel Machek]

Post by Rafael J. Wysocki

Post by Pavel Machek
Hi!

Post by Rafael J. Wysocki

Post by Pavel Machek

Post by Rafael J. Wysocki
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.
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

[Rafael J. Wysocki]

Post by Pavel Machek

Post by Rafael J. Wysocki

Post by Pavel Machek
Hi!

Post by Rafael J. Wysocki

Post by Pavel Machek

Post by Rafael J. Wysocki
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.
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.

Post by Pavel Machek
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

[Pavel Machek]

Hi!

Post by Rafael J. Wysocki

Post by Pavel Machek

Post by Rafael J. Wysocki
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...?

Post by Rafael J. Wysocki

Post by Pavel Machek
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

[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 <***@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[] = {

[Rafael J. Wysocki]

Post by 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).
---
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

[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(+)

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

[Pavel Machek]

Hi!

Post by Florian Fainelli
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.
---
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

[Florian Fainelli]

Post by Pavel Machek
Hi!

Post by Florian Fainelli
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.
---
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.

[Rafael J. Wysocki]

Post by Florian Fainelli
Add an optional platform_suspend_ops callback: target_state, and a
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

[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 <***@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 {

[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 <***@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;

[Pavel Machek]

Hi!

Post by 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().

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

Post by Florian Fainelli
+static inline suspend_state_t suspend_target_state(void) { return -ENOSYS; }

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

Post by Florian Fainelli
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

[Rafael J. Wysocki]

Post by Pavel Machek
Hi!

Post by 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().

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

[Florian Fainelli]

Post by Rafael J. Wysocki

Post by Pavel Machek
Hi!

Post by 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().

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

[Rafael J. Wysocki]

Post by 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.
---
- just export pm_suspend_target_state without a helper function
- 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

[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 <***@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:

[Rafael J. Wysocki]

Post by 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.
---
- clear pm_suspend_target_state in Close label
- just export pm_suspend_target_state without a helper function
- 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!

[Pavel Machek]

Post by Alexandre Belloni

Post by Florian Fainelli

Post by Rafael J. Wysocki
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.

- whether main clock is switched from the master clock to the slow
clock
- whether VDDcore will be cut
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

[Pavel Machek]

Post by 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.

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