Platform-specific suspend/resume code in drivers

Platform-specific suspend/resume code in drivers

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

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.

Re: Platform-specific suspend/resume code in drivers

[Alan Stern]

On Tue, 7 Jun 2016, Mason wrote:

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

Yes.

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

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

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?

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.

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

> 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

Re: Platform-specific suspend/resume code in drivers

[Mason]

On 07/06/2016 17:06, Alan Stern wrote:

> On Tue, 7 Jun 2016, Mason wrote:
>
>> 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?

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

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.

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

Re: Platform-specific suspend/resume code in drivers

[Alan Stern]

On Wed, 8 Jun 2016, Mason wrote:

> On 07/06/2016 17:06, Alan Stern wrote:
> 
> > On Tue, 7 Jun 2016, Mason wrote:
> >
> >> 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?

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?

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

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

Re: Platform-specific suspend/resume code in drivers

[Mason]

On 08/06/2016 19:45, Alan Stern wrote:

> On Wed, 8 Jun 2016, Mason wrote:
> 
>> On 07/06/2016 17:06, Alan Stern wrote:
>>
>>> On Tue, 7 Jun 2016, Mason wrote:
>>>
>>>> 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?
> 
> 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?


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

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


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

Re: Platform-specific suspend/resume code in drivers

[Alan Stern]

On Wed, 8 Jun 2016, Mason wrote:

> On 08/06/2016 19:45, Alan Stern wrote:
> 
> > On Wed, 8 Jun 2016, Mason wrote:
> > 
> >> On 07/06/2016 17:06, Alan Stern wrote:
> >>
> >>> On Tue, 7 Jun 2016, Mason wrote:
> >>>
> >>>> 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?
> > 
> > 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?

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.

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

...

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

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

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.

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

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

Re: Platform-specific suspend/resume code in drivers

[Mason]

On 09/06/2016 17:05, Alan Stern wrote:

> On Wed, 8 Jun 2016, Mason wrote:
> 
>> On 08/06/2016 19:45, Alan Stern wrote:
>>
>>> On Wed, 8 Jun 2016, Mason wrote:
>>>
>>>> On 07/06/2016 17:06, Alan Stern wrote:
>>>>
>>>>> On Tue, 7 Jun 2016, Mason wrote:
>>>>>
>>>>>> 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?
>>>
>>> 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?
> 
> 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.
> 
>> (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.


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

Re: Platform-specific suspend/resume code in drivers

[Alan Stern]

On Fri, 10 Jun 2016, Mason wrote:

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

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.

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

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

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.


On Fri, 10 Jun 2016, Geert Uytterhoeven wrote:

> Hi Alan,

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

Re: Platform-specific suspend/resume code in drivers

[Kevin Hilman]

Mason <slash.tmp@free.fr> writes:

[...]

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

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

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

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

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

Kevin

Re: Platform-specific suspend/resume code in drivers

[Geert Uytterhoeven]

Hi Alan,

On Thu, Jun 9, 2016 at 5:05 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 8 Jun 2016, Mason wrote:
>> On 08/06/2016 19:45, Alan Stern wrote:
>> > On Wed, 8 Jun 2016, Mason wrote:
>> >> On 07/06/2016 17:06, Alan Stern wrote:
>> >>> On Tue, 7 Jun 2016, Mason wrote:
>> >>>> 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?
>> >
>> > 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?
>
> 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@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

Re: Platform-specific suspend/resume code in drivers

[Sebastian Frias]

Hi Alan,

On 06/08/2016 07:45 PM, Alan Stern wrote:
>> 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?
> 
>>> 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.

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

Re: Platform-specific suspend/resume code in drivers

[Alan Stern]

On Thu, 9 Jun 2016, Sebastian Frias wrote:

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

> 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

Re: Platform-specific suspend/resume code in drivers

[Pavel Machek]

Hi!

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

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.

> 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html