Platform-specific suspend/resume code in drivers

DISCUSSION Mason 6 participants 13 messages Jun 7, 2016 – Jun 18, 2016

lore

Share & tools

lore

Mason

author

2016-06-07 08:56 UTC #0 lore raw

Hello everyone,

I want to implement system-wide suspend-to-RAM on my platform (an ARM Cortex A9 based SoC).

I'm hoping some of you can clear some of my confusion :-(

AFAIU, what happens to the hardware in the suspended state is very board/platform specific. The SoC may stop some clocks; it may even cut the power to some parts of the chip.

AFAIU, once power is cut, the value of internal and visible registers (called "state" or "context" IIUC) is lost.

Therefore, each driver is supposed to either

1) save context on suspend & restore it on resume, or
2) re-initialize the device on resume (if context is unimportant)

Is that correct?

(Note: Kevin Hilman mentioned using a feature called "regmap" to help in saving/restoring context.)

Documentation/power/devices.txt mentions putting such save/restore code in suspend_late/resume_early callbacks. Is that the preferred solution today?

Should these routines be guarded by #ifdef CONFIG_PM or #ifdef CONFIG_SUSPEND ?

Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?

So this means I need to add in the probe function, for every driver my platform uses:

  if (platform == MY_PLATFORM) {
    ops.suspend = my_suspend;
    ops.resume  = my_resume;
  }

Is that correct?

While I'm listing the points I don't understand... It seems Linux handles suspend/resume "transparently". I mean the CPU calls all the functions leading to the suspend action, then when the system comes back online, it continues at the next instruction, as if nothing had happened. So the stack and registers need to be exactly in the same state.

But on my system, the firmware expects a return address for *where* to return on resume. Should I pass it the next instruction pointer? The address of the resume function?

I see arch/arm/kernel/psci_smp.c seems to have the same notion. => drivers/firmware/psci.c

References:
arch/arm/kernel/suspend.c
kernel/power/suspend.c

Regards.

Alan Stern

2016-06-07 15:06 UTC #1 lore raw

Quoted 14 lines · Mason

Hello everyone,
I want to implement system-wide suspend-to-RAM on my platform (an ARM Cortex A9 based SoC).
I'm hoping some of you can clear some of my confusion :-(
AFAIU, what happens to the hardware in the suspended state is very board/platform specific. The SoC may stop some clocks; it may even cut the power to some parts of the chip.
AFAIU, once power is cut, the value of internal and visible registers (called "state" or "context" IIUC) is lost.

Quoted 6 lines

Therefore, each driver is supposed to either
1) save context on suspend & restore it on resume, or
2) re-initialize the device on resume (if context is unimportant)
Is that correct?

Yes.

Quoted 6 lines

(Note: Kevin Hilman mentioned using a feature called "regmap" to help in saving/restoring context.)
Documentation/power/devices.txt mentions putting such save/restore code in suspend_late/resume_early callbacks. Is that the preferred solution today?

It depends on the device's requirements. If you require interrupts to be enabled while you do the save/restore then you can't use suspend_late/resume_early; you have to use suspend/resume.

Quoted 1 line

Should these routines be guarded by #ifdef CONFIG_PM or #ifdef CONFIG_SUSPEND ?

CONFIG_SUSPEND.

Quoted 3 lines

Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?

Is your device platform-specific? If it is then the driver is also platform-specific, and so no part of the driver will be called at runtime unless your platform is detected.

If the device isn't platform-specific then the driver has to work on a bunch of different platforms. It should be written to be platform-independent as much as possible.

Quoted 9 lines

So this means I need to add in the probe function, for every driver my platform uses:
  if (platform == MY_PLATFORM) {
    ops.suspend = my_suspend;
    ops.resume  = my_resume;
  }
Is that correct?

No. For one thing, you only have to worry about the platform-independent drivers -- you know that the platform-specific ones won't get used unless your platform is present.

For another, the driver should be written in a way that doesn't require this sort of code. The ops pointer (not any of the structure's members -- a pointer to the structure) should be set by the platform-dependent part of the driver that handles initialization.

Quoted 10 lines

While I'm listing the points I don't understand... It seems Linux handles suspend/resume "transparently". I mean the CPU calls all the functions leading to the suspend action, then when the system comes back online, it continues at the next instruction, as if nothing had happened. So the stack and registers need to be exactly in the same state.
But on my system, the firmware expects a return address for *where* to return on resume. Should I pass it the next instruction pointer? The address of the resume function?

You should pass it the address of a function you write, one that will clean things up and jump into the normal system resume code.

Alan Stern

Mason

author

2016-06-08 16:26 UTC #2 lore raw

Quoted 9 lines · Alan Stern

Quoted 3 lines · Mason
Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?
Is your device platform-specific? If it is then the driver is also platform-specific, and so no part of the driver will be called at runtime unless your platform is detected.

Specifically, my platform uses drivers/net/ethernet/aurora/nb8800.c => 3 entries in of_match_table. drivers/tty/serial/8250/8250_core.c (CONFIG_SERIAL_8250_RT288X variant)

and also the XHCI USB3 driver, and AHCI SATA driver, wouldn't I need to save the context for these too?

Quoted 12 lines

If the device isn't platform-specific then the driver has to work on a bunch of different platforms. It should be written to be platform-independent as much as possible.
Quoted 8 lines
So this means I need to add in the probe function, for every driver my platform uses:
  if (platform == MY_PLATFORM) {
    ops.suspend = my_suspend;
    ops.resume  = my_resume;
  }

Quoted 5 lines

Quoted 1 line
Is that correct?
No. For one thing, you only have to worry about the platform-independent drivers -- you know that the platform-specific ones won't get used unless your platform is present.

I guess the thermal driver is platform-specific, but most devices are third-party IP blocks, so there is a "common" driver upstream. But I would need a platform-specific suspend/resume sequence, just for my platform.

Quoted 4 lines

For another, the driver should be written in a way that doesn't require this sort of code. The ops pointer (not any of the structure's members -- a pointer to the structure) should be set by the platform-dependent part of the driver that handles initialization.

I don't understand. If my platform loses context on suspend, then I must save/restore it. But this wasteful operation should not be imposed on other platforms.

Regards.

Alan Stern

2016-06-08 17:45 UTC #3 lore raw

Quoted 12 lines · Mason

Quoted 9 lines · Alan Stern
Quoted 3 lines · Mason
Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?
Is your device platform-specific? If it is then the driver is also platform-specific, and so no part of the driver will be called at runtime unless your platform is detected.

Quoted 6 lines

Specifically, my platform uses drivers/net/ethernet/aurora/nb8800.c => 3 entries in of_match_table. drivers/tty/serial/8250/8250_core.c (CONFIG_SERIAL_8250_RT288X variant)
and also the XHCI USB3 driver, and AHCI SATA driver, wouldn't I need to save the context for these too?

Those drivers should already take care of their own contexts. Is there anything platform-specific you need to do in addition? Consider xHCI as an example case -- what more does it need?

Quoted 14 lines

Quoted 14 lines
If the device isn't platform-specific then the driver has to work on a bunch of different platforms. It should be written to be platform-independent as much as possible.
Quoted 9 lines
So this means I need to add in the probe function, for every driver my platform uses:
  if (platform == MY_PLATFORM) {
    ops.suspend = my_suspend;
    ops.resume  = my_resume;
  }
Is that correct?

Quoted 8 lines

Quoted 3 lines
No. For one thing, you only have to worry about the platform-independent drivers -- you know that the platform-specific ones won't get used unless your platform is present.
I guess the thermal driver is platform-specific, but most devices are third-party IP blocks, so there is a "common" driver upstream. But I would need a platform-specific suspend/resume sequence, just for my platform.

Why? What sort of platform-specific things do you need to do?

Quoted 8 lines

Quoted 4 lines
For another, the driver should be written in a way that doesn't require this sort of code. The ops pointer (not any of the structure's members -- a pointer to the structure) should be set by the platform-dependent part of the driver that handles initialization.
I don't understand. If my platform loses context on suspend, then I must save/restore it. But this wasteful operation should not be imposed on other platforms.

More details, please.

Alan Stern

Mason

author

2016-06-08 21:26 UTC #4 lore raw

Quoted 18 lines · Alan Stern

Quoted 16 lines · Mason
Quoted 9 lines · Alan Stern
Quoted 3 lines · Mason
Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?
Is your device platform-specific? If it is then the driver is also platform-specific, and so no part of the driver will be called at runtime unless your platform is detected.
Specifically, my platform uses drivers/net/ethernet/aurora/nb8800.c => 3 entries in of_match_table. drivers/tty/serial/8250/8250_core.c (CONFIG_SERIAL_8250_RT288X variant)

Quoted 5 lines

Quoted 2 lines
and also the XHCI USB3 driver, and AHCI SATA driver, wouldn't I need to save the context for these too?
Those drivers should already take care of their own contexts. Is there anything platform-specific you need to do in addition?

Take the eth driver, for example:

http://lxr.free-electrons.com/source/drivers/net/ethernet/aurora/nb8800.c

It doesn't have any suspend/resume callbacks.

Would that make the suspend request fail?

Quoted 1 line

Consider xHCI as an example case -- what more does it need?

Lemme see...

http://lxr.free-electrons.com/source/drivers/usb/host/xhci-plat.c

#ifdef CONFIG_PM_SLEEP
static int xhci_plat_suspend(struct device *dev)
{
struct usb_hcd *hcd = dev_get_drvdata(dev);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);

/*
* xhci_suspend() needs do_wakeup to know whether host is allowed
* to do wakeup during suspend. Since xhci_plat_suspend is currently
* only designed for system suspend, device_may_wakeup() is enough
* to dertermine whether host is allowed to do wakeup. Need to
* reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
* also applies to runtime suspend.
*/
return xhci_suspend(xhci, device_may_wakeup(dev));
}

static int xhci_plat_resume(struct device *dev)

{
	struct usb_hcd	*hcd = dev_get_drvdata(dev);
	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);

	return xhci_resume(xhci, 0);
}

static const struct dev_pm_ops xhci_plat_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
};
#define DEV_PM_OPS (&xhci_plat_pm_ops)

(Side note: they guarded the code with CONFIG_PM_SLEEP, rather than CONFIG_SUSPEND. Why?)

Looking at xhci_suspend, it does call xhci_save_registers() so perhaps this specific driver already saves the context required to resume correctly after a full power-down.

Quoted 18 lines

Quoted 18 lines
Quoted 17 lines
If the device isn't platform-specific then the driver has to work on a bunch of different platforms. It should be written to be platform-independent as much as possible.
Quoted 9 lines
So this means I need to add in the probe function, for every driver my platform uses:
  if (platform == MY_PLATFORM) {
    ops.suspend = my_suspend;
    ops.resume  = my_resume;
  }
Is that correct?
No. For one thing, you only have to worry about the platform-independent drivers -- you know that the platform-specific ones won't get used unless your platform is present.

Quoted 6 lines

Quoted 4 lines
I guess the thermal driver is platform-specific, but most devices are third-party IP blocks, so there is a "common" driver upstream. But I would need a platform-specific suspend/resume sequence, just for my platform.
Why? What sort of platform-specific things do you need to do?

Save the value of the MMIO registers which will be lost when the chip is powered down.

Quoted 10 lines

Quoted 8 lines
Quoted 4 lines
For another, the driver should be written in a way that doesn't require this sort of code. The ops pointer (not any of the structure's members -- a pointer to the structure) should be set by the platform-dependent part of the driver that handles initialization.
I don't understand. If my platform loses context on suspend, then I must save/restore it. But this wasteful operation should not be imposed on other platforms.
More details, please.

I'm confused. I must have misunderstood something fundamental, and my explanations come out wrong. Let me try again.

On this SoC (as most SoCs) device registers are mapped into memory. When this system is suspended, it was decided that we would
1) put the RAM in self-refresh mode
2) power everything down, but the RAM

So the contents of RAM are preserved, but the contents of the device registers will be lost. (And this context loss is platform specific; other SoCs may decide to keep some devices powered.) Anyway, when the chip is powered back up, some/most device registers need to be restored to their pre-suspend value.

For example, the reset value for a CTRL register might default to DISABLE_DEVICE, but obviously the probe function would have toggled that to ENABLE_DEVICE.

Am I not making sense?

Regards.

Alan Stern

2016-06-09 15:05 UTC #5 lore raw

Quoted 26 lines · Mason

Quoted 18 lines · Alan Stern
Quoted 16 lines · Mason
Quoted 9 lines · Alan Stern
Quoted 3 lines · Mason
Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?
Is your device platform-specific? If it is then the driver is also platform-specific, and so no part of the driver will be called at runtime unless your platform is detected.
Specifically, my platform uses drivers/net/ethernet/aurora/nb8800.c => 3 entries in of_match_table. drivers/tty/serial/8250/8250_core.c (CONFIG_SERIAL_8250_RT288X variant)
Quoted 5 lines
Quoted 2 lines
and also the XHCI USB3 driver, and AHCI SATA driver, wouldn't I need to save the context for these too?
Those drivers should already take care of their own contexts. Is there anything platform-specific you need to do in addition?

Quoted 7 lines

Take the eth driver, for example:
http://lxr.free-electrons.com/source/drivers/net/ethernet/aurora/nb8800.c
It doesn't have any suspend/resume callbacks.
Would that make the suspend request fail?

There are plenty of drivers that don't have suspend/resume callbacks. (Fewer than there used to be, fortunately.) They don't make suspend fail; rather, the PM core just skips over them when carrying out a suspend.

Of course, if a driver needs suspend support but doesn't include it, then it's quite likely the driver will not work properly after the system has been resumed. But that's not a platform-specific issue.

Quoted 3 lines

Quoted 1 line
Consider xHCI as an example case -- what more does it need?
Lemme see...

...

Quoted 2 lines

(Side note: they guarded the code with CONFIG_PM_SLEEP, rather than CONFIG_SUSPEND. Why?)

CONFIG_PM_SLEEP covers both suspend (AKA suspend-to-ram) and hibernation (AKA suspend-to-disk).

Quoted 27 lines

Looking at xhci_suspend, it does call xhci_save_registers() so perhaps this specific driver already saves the context required to resume correctly after a full power-down.
Quoted 23 lines
Quoted 22 lines
Quoted 12 lines
If the device isn't platform-specific then the driver has to work on a bunch of different platforms. It should be written to be platform-independent as much as possible.
Quoted 8 lines
So this means I need to add in the probe function, for every driver my platform uses:
  if (platform == MY_PLATFORM) {
    ops.suspend = my_suspend;
    ops.resume  = my_resume;
  }
Quoted 5 lines
Quoted 1 line
Is that correct?
No. For one thing, you only have to worry about the platform-independent drivers -- you know that the platform-specific ones won't get used unless your platform is present.
I guess the thermal driver is platform-specific, but most devices are third-party IP blocks, so there is a "common" driver upstream. But I would need a platform-specific suspend/resume sequence, just for my platform.

Quoted 4 lines

Quoted 1 line
Why? What sort of platform-specific things do you need to do?
Save the value of the MMIO registers which will be lost when the chip is powered down.

How did these registers get set in the first place? Wherever the code is that sets them up initially, that's where these registers should be reinitialized following a resume. If they were set up by platform-specific code then they should be reinitialized by platform-specific code.

Quoted 25 lines

Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
For another, the driver should be written in a way that doesn't require this sort of code. The ops pointer (not any of the structure's members -- a pointer to the structure) should be set by the platform-dependent part of the driver that handles initialization.
I don't understand. If my platform loses context on suspend, then I must save/restore it. But this wasteful operation should not be imposed on other platforms.
More details, please.
I'm confused. I must have misunderstood something fundamental, and my explanations come out wrong. Let me try again.
On this SoC (as most SoCs) device registers are mapped into memory. When this system is suspended, it was decided that we would
1) put the RAM in self-refresh mode
2) power everything down, but the RAM
So the contents of RAM are preserved, but the contents of the device registers will be lost. (And this context loss is platform specific; other SoCs may decide to keep some devices powered.) Anyway, when the chip is powered back up, some/most device registers need to be restored to their pre-suspend value.

Quoted 5 lines

For example, the reset value for a CTRL register might default to DISABLE_DEVICE, but obviously the probe function would have toggled that to ENABLE_DEVICE.
Am I not making sense?

Sure. A driver's resume routine is supposed to recognize when the register contents have been lost due to lack of power during suspend or hibernation. It should reinitialize any registers the driver needs.

During hibernation, for instance, it is nearly certain that all the device context will be lost. Drivers have to be prepared to handle this.

Alan Stern

Mason

author

2016-06-10 11:03 UTC #6 lore raw

Quoted 45 lines · Alan Stern

Quoted 26 lines · Mason
Quoted 18 lines · Alan Stern
Quoted 16 lines · Mason
Quoted 9 lines · Alan Stern
Quoted 3 lines · Mason
Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?
Is your device platform-specific? If it is then the driver is also platform-specific, and so no part of the driver will be called at runtime unless your platform is detected.
Specifically, my platform uses drivers/net/ethernet/aurora/nb8800.c => 3 entries in of_match_table. drivers/tty/serial/8250/8250_core.c (CONFIG_SERIAL_8250_RT288X variant)
Quoted 5 lines
Quoted 2 lines
and also the XHCI USB3 driver, and AHCI SATA driver, wouldn't I need to save the context for these too?
Those drivers should already take care of their own contexts. Is there anything platform-specific you need to do in addition?
Quoted 7 lines
Take the eth driver, for example:
http://lxr.free-electrons.com/source/drivers/net/ethernet/aurora/nb8800.c
It doesn't have any suspend/resume callbacks.
Would that make the suspend request fail?
There are plenty of drivers that don't have suspend/resume callbacks. (Fewer than there used to be, fortunately.) They don't make suspend fail; rather, the PM core just skips over them when carrying out a suspend.
Of course, if a driver needs suspend support but doesn't include it, then it's quite likely the driver will not work properly after the system has been resumed. But that's not a platform-specific issue.

Quoted 5 lines

Quoted 2 lines
(Side note: they guarded the code with CONFIG_PM_SLEEP, rather than CONFIG_SUSPEND. Why?)
CONFIG_PM_SLEEP covers both suspend (AKA suspend-to-ram) and hibernation (AKA suspend-to-disk).

Thanks for pointing that out.

Quoted 25 lines

Quoted 25 lines
Quoted 24 lines
Quoted 14 lines
Quoted 14 lines
If the device isn't platform-specific then the driver has to work on a bunch of different platforms. It should be written to be platform-independent as much as possible.
Quoted 9 lines
So this means I need to add in the probe function, for every driver my platform uses:
  if (platform == MY_PLATFORM) {
    ops.suspend = my_suspend;
    ops.resume  = my_resume;
  }
Is that correct?
Quoted 8 lines
Quoted 3 lines
No. For one thing, you only have to worry about the platform-independent drivers -- you know that the platform-specific ones won't get used unless your platform is present.
I guess the thermal driver is platform-specific, but most devices are third-party IP blocks, so there is a "common" driver upstream. But I would need a platform-specific suspend/resume sequence, just for my platform.
Why? What sort of platform-specific things do you need to do?

Quoted 8 lines

Quoted 2 lines
Save the value of the MMIO registers which will be lost when the chip is powered down.
How did these registers get set in the first place? Wherever the code is that sets them up initially, that's where these registers should be reinitialized following a resume. If they were set up by platform-specific code then they should be reinitialized by platform-specific code.

Consider two SoCs, SoCA and SocB.

They both have the same device D, which exposes 16 registers to interact with the hardware.

When SoCA is suspended, it stops D's clock, but keeps D powered on, preserving the 16 registers. Thus when SoCA resumes, it finds the 16 registers in the same state as before suspending.

When SoCB is suspended, it powers D down. When SoCB resumes, the 16 registers contain garbage/random values. Therefore, in the case of SoCB, the suspend routine should save the values of the registers to RAM (which is preserved by "contract") and restore them on resume.

However, this save/restore operation is unnecessary on SoCA.

Should the save/restore operation be added unconditionally to the driver, even if some SoCs do not need it?

In other words, do we consider the performance penalty from saving and restoring device registers small enough that it should be done systematically, even if some SoCs do not require it?

Some device tree nodes have a property called "always-on" which is documented as "If present, the block is powered through an always-on power domain, therefore it never loses context."

In that case, I suppose the driver's suspend/resume routine should:

suspend:

if (always-on is not present) {
    save device registers to RAM;
}

resume:

if (always-on is not present) {
    restore device registers from RAM;
}

Do you disagree?

Regards.

Alan Stern

2016-06-10 14:19 UTC #7 lore raw

Quoted 14 lines · Mason

Consider two SoCs, SoCA and SocB.
They both have the same device D, which exposes 16 registers to interact with the hardware.
When SoCA is suspended, it stops D's clock, but keeps D powered on, preserving the 16 registers. Thus when SoCA resumes, it finds the 16 registers in the same state as before suspending.
When SoCB is suspended, it powers D down. When SoCB resumes, the 16 registers contain garbage/random values. Therefore, in the case of SoCB, the suspend routine should save the values of the registers to RAM (which is preserved by "contract") and restore them on resume.

Quoted 4 lines

However, this save/restore operation is unnecessary on SoCA.
Should the save/restore operation be added unconditionally to the driver, even if some SoCs do not need it?

It certainly should be added, because otherwise SoCB won't work. Whether it is unconditional is up to the programmer. If the driver has some way to know that power won't be lost, it can skip saving the registers.

Quoted 3 lines

In other words, do we consider the performance penalty from saving and restoring device registers small enough that it should be done systematically, even if some SoCs do not require it?

Time is not the most important consideration -- proper operation is.

Quoted 11 lines

Some device tree nodes have a property called "always-on" which is documented as "If present, the block is powered through an always-on power domain, therefore it never loses context."
In that case, I suppose the driver's suspend/resume routine should:
suspend:
if (always-on is not present) {
    save device registers to RAM;
}

Quoted 6 lines

resume:

if (always-on is not present) {
    restore device registers from RAM;
}

Do you disagree?

Apart from nit-picking, no. In the resume routine, it might be better to check a flag that the suspend routine would set to indicate whether or not the registers were saved, instead of retrieving the always-on attribute again.

Quoted 1 line · Geert Uytterhoeven

Hi Alan,

Hello.

Quoted 9 lines

Quoted 3 lines
Of course, if a driver needs suspend support but doesn't include it, then it's quite likely the driver will not work properly after the system has been resumed. But that's not a platform-specific issue.
It may be. The same device may be present on different SoCs. On some SoCs, the device may be part of a power area, on others not. When running on the former SoC, the driver needs to save/restore registers, while this is not needed (but harmless, except for a small time impact) on the latter SoC.

Splitting hairs. As a general principle, you have to agree that:

If a driver needs suspend support but doesn't include it, then it's quite likely the driver will not work properly after the system has been resumed.

is true on all platforms. On the other hand, the question of whether or not the driver needs suspend support may well be platform-specific.

Alan Stern

Kevin Hilman

2016-06-10 22:32 UTC #8 lore raw

In reply to Mason

Mason <[email protected]> writes:

[...]

Quoted 14 lines

Consider two SoCs, SoCA and SocB.
They both have the same device D, which exposes 16 registers to interact with the hardware.
When SoCA is suspended, it stops D's clock, but keeps D powered on, preserving the 16 registers. Thus when SoCA resumes, it finds the 16 registers in the same state as before suspending.
When SoCB is suspended, it powers D down. When SoCB resumes, the 16 registers contain garbage/random values. Therefore, in the case of SoCB, the suspend routine should save the values of the registers to RAM (which is preserved by "contract") and restore them on resume.

Quoted 4 lines

However, this save/restore operation is unnecessary on SoCA.
Should the save/restore operation be added unconditionally to the driver, even if some SoCs do not need it?

Yes. For starters, this is the way to go. If the added save/restore is too expensive, a few things can be done...

Quoted 3 lines

In other words, do we consider the performance penalty from saving and restoring device registers small enough that it should be done systematically, even if some SoCs do not require it?

The "save" part can be made to be almost no overhead by always keeping a "shadow" copy of the registers in memory whenever they change (not just at suspend time.) This could be done "manually" by the driver for the needed registers, or with the aid of something like the regmap cache feature. Doing that, there's no additional time for saving context during suspend.

On resume time, on most hardware there are at least some registers that will have some pre-determined power-on reset value. You could optimize the restore context by comparing one (or a few) of the registers from your saved context against the power-on reset values to determine if context was lost, and thus avoid doing a full restore if needed.

Honestly however, the save/restore context time for most devices is going to be insignificant compared with the total system-wide suspend/resume time, so the optimizations will not likely be noticed for system-wide suspend/resume.

However, if you would like to also implement runtime PM (and you should), then the save/restore context for individual devices will start to matter since for latency reasons, you will likely care about runtime suspend/resume times for individual devices.

In that case however, the solution is use PM QoS constraints when there are specific latency requirements so that the power-off of the device is avoided in cases where latency constraints could not be met.

Quoted 11 lines

Some device tree nodes have a property called "always-on" which is documented as "If present, the block is powered through an always-on power domain, therefore it never loses context."
In that case, I suppose the driver's suspend/resume routine should:
suspend:
if (always-on is not present) {
    save device registers to RAM;
}

Quoted 6 lines

resume:

if (always-on is not present) {
    restore device registers from RAM;
}

Do you disagree?

Yes. I'm not a fan of using always-on for this.

Kevin

Geert Uytterhoeven

2016-06-10 13:39 UTC #9 lore raw

In reply to Alan Stern

Hi Alan,

Quoted 31 lines · Alan Stern

Quoted 22 lines · Mason
Quoted 15 lines · Alan Stern
Quoted 14 lines · Mason
Quoted 8 lines · Alan Stern
Quoted 3 lines · Mason
Another point of confusion for me is this: drivers are supposed to be shared among platforms, right? So my platform-specific suspend code should be enabled only if my platform is detected at run-time?
Is your device platform-specific? If it is then the driver is also platform-specific, and so no part of the driver will be called at runtime unless your platform is detected.
Specifically, my platform uses drivers/net/ethernet/aurora/nb8800.c => 3 entries in of_match_table. drivers/tty/serial/8250/8250_core.c (CONFIG_SERIAL_8250_RT288X variant)
Quoted 5 lines
Quoted 2 lines
and also the XHCI USB3 driver, and AHCI SATA driver, wouldn't I need to save the context for these too?
Those drivers should already take care of their own contexts. Is there anything platform-specific you need to do in addition?
Quoted 7 lines
Take the eth driver, for example:
http://lxr.free-electrons.com/source/drivers/net/ethernet/aurora/nb8800.c
It doesn't have any suspend/resume callbacks.
Would that make the suspend request fail?

Quoted 8 lines

There are plenty of drivers that don't have suspend/resume callbacks. (Fewer than there used to be, fortunately.) They don't make suspend fail; rather, the PM core just skips over them when carrying out a suspend.
Of course, if a driver needs suspend support but doesn't include it, then it's quite likely the driver will not work properly after the system has been resumed. But that's not a platform-specific issue.

It may be. The same device may be present on different SoCs. On some SoCs, the device may be part of a power area, on others not. When running on the former SoC, the driver needs to save/restore registers, while this is not needed (but harmless, except for a small time impact) on the latter SoC.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Sebastian Frias

2016-06-09 08:52 UTC #10 lore raw

In reply to Alan Stern

Hi Alan,

Quoted 12 lines · Alan Stern

Quoted 4 lines
I guess the thermal driver is platform-specific, but most devices are third-party IP blocks, so there is a "common" driver upstream. But I would need a platform-specific suspend/resume sequence, just for my platform.
Why? What sort of platform-specific things do you need to do?
Quoted 5 lines
Quoted 4 lines
For another, the driver should be written in a way that doesn't require this sort of code. The ops pointer (not any of the structure's members -- a pointer to the structure) should be set by the platform-dependent part of the driver that handles initialization.

Quoted 5 lines

Quoted 3 lines
I don't understand. If my platform loses context on suspend, then I must save/restore it. But this wasteful operation should not be imposed on other platforms.
More details, please.

In addition to what Marc already replied, allow me to put a small (and hopefully self-contained) example:

- There's a SoC with a HW IP (UART, ethernet, usb or whatever else) for which a generic driver exists.
- The generic driver only needs to know the base address of the block, and it will just work.
- It'll just work, provided that when Linux boots the HW IP is enabled.
- Indeed, the SoC may have other registers controlling if the HW IP is enabled/powered up.
- Those registers are SoC specific, even if the HW IP is generic.

So the question is:
=> Does the use of a generic driver indirectly implies that any SoC specific registers are to be setup outside Linux? In other words, does Linux expects the HW IP to be powered up/enabled and ready to use? (ie: setup by the bootloader)

If that is not the case, what is the recommended way to handle those registers?
In other words, do the generic drivers provide with an API to handle SoC specific enable/clockgating/powerup registers?

Thanks in advance,

Sebastian

Alan Stern

2016-06-09 15:14 UTC #11 lore raw

Quoted 11 lines · Sebastian Frias

In addition to what Marc already replied, allow me to put a small (and hopefully self-contained) example:
- There's a SoC with a HW IP (UART, ethernet, usb or whatever else) for which a generic driver exists.
- The generic driver only needs to know the base address of the block, and it will just work.
- It'll just work, provided that when Linux boots the HW IP is enabled.
- Indeed, the SoC may have other registers controlling if the HW IP is enabled/powered up.
- Those registers are SoC specific, even if the HW IP is generic.
So the question is:
=> Does the use of a generic driver indirectly implies that any SoC specific registers are to be setup outside Linux? In other words, does Linux expects the HW IP to be powered up/enabled and ready to use? (ie: setup by the bootloader)

It varies. Some implementations do expect the platform-specific parts to be initialized before boot, whereas others include platform-specific code to take care of this.

Quoted 2 lines

If that is not the case, what is the recommended way to handle those registers?
In other words, do the generic drivers provide with an API to handle SoC specific enable/clockgating/powerup registers?

Frequently drivers include platform-specific parts for this purpose. These parts are called through various platform-specific mechanisms at runtime, such as MODULE_DEVICE_TABLE entries for OF (i.e., Device Tree) or ACPI or just plain old platform devices.

For an example, look near the end of drivers/usb/host/ehci-platform.c.

Alan Stern

Pavel Machek

2016-06-18 14:35 UTC #12 lore raw

In reply to Sebastian Frias

Hi!

Quoted 10 lines

Quoted 1 line
More details, please.
In addition to what Marc already replied, allow me to put a small (and hopefully self-contained) example:
- There's a SoC with a HW IP (UART, ethernet, usb or whatever else) for which a generic driver exists.
- The generic driver only needs to know the base address of the block, and it will just work.
- It'll just work, provided that when Linux boots the HW IP is enabled.
- Indeed, the SoC may have other registers controlling if the HW IP is enabled/powered up.
- Those registers are SoC specific, even if the HW IP is generic.

Quoted 4 lines

So the question is:
=> Does the use of a generic driver indirectly implies that any SoC specific registers are to be setup outside Linux? In other words, does Linux expects the HW IP to be powered up/enabled and ready to use? (ie: setup by the bootloader)

On PC, Linux has some expectations about bootloader. On other platforms, it should not have to... and with suspend / hibernation, it is harder and harder to rely on it.

Quoted 3 lines

If that is not the case, what is the recommended way to handle those pregisters?
In other words, do the generic drivers provide with an API to handle SoC specific enable/clockgating/powerup registers?

You should create a special driver for your hardware when this happens. clock gating is usually handled by specifying what clocks it needs in the device tree, and you specify power control GPIO, too.

Best regards,
Pavel

Signature · 2 lines

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