PM: knowing the system state in the device callback

PM: knowing the system state in the device callback

[Alexandre Belloni]

Hi,

I'm trying to get rid of at91_suspend_entering_slow_clock() which is
exposing the platform suspend_state_t to the devices. From what I
understand, whenever suspend_state_t is PM_SUSPEND_MEM or
PM_SUSPEND_STANDBY, the pm_message_t passed to the device driver is
always PM_EVENT_SUSPEND.

The requirement is to know whether we are going to cut the master clock
and in that case, avoid calling enable_irq_wake() because we will not be
able to wakeup from the device.

Is there a better way to do that? Or should I implement a similar
function in the pm core (which I guess would already be there if
really needed)?

Regards,

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

PM: knowing the system state in the device callback

[Boris Brezillon]

Hi Alexandre,

On Mon, 16 Mar 2015 20:17:42 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> Hi,
> 
> I'm trying to get rid of at91_suspend_entering_slow_clock() which is
> exposing the platform suspend_state_t to the devices. From what I
> understand, whenever suspend_state_t is PM_SUSPEND_MEM or
> PM_SUSPEND_STANDBY, the pm_message_t passed to the device driver is
> always PM_EVENT_SUSPEND.
> 
> The requirement is to know whether we are going to cut the master clock
> and in that case, avoid calling enable_irq_wake() because we will not be
> able to wakeup from the device.

Actually the master clock is never switched off, we're only changing
its source (from PLLA to slow clk) which in turn changes its rate.

> 
> Is there a better way to do that? Or should I implement a similar
> function in the pm core (which I guess would already be there if
> really needed)?

IMHO we should let devices (RTC/RTT, debug UART, GPIOC, ...) mark their
interrupt line as a wakeup interrupt, and adapt the device
configuration (UART baudrate, ...) according to the new master clock
rate instead of testing the suspend mode to check whether we can mark
irq lines as wakeup sources or not.
The fact that we're disabling PLLA is not really related to
suspend-to-ram mode (we could leave the master clock unchanged and
still put the SDRAM chip in self-refresh mode).

The problem here is that we need some kind of notifier infrastructure
that would be called before the master clock source is switched to slow
clock. This notifier must be written in asm so that it can be called
from the asm code executed from SRAM (the SDRAM chip is placed in self
refresh before switching to slow clock).

Best Regards,

Boris

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

PM: knowing the system state in the device callback

[Sylvain Rochet]

Hi,


On Mon, Mar 16, 2015 at 08:52:45PM +0100, Boris Brezillon wrote:
> Hi Alexandre,
> 
> On Mon, 16 Mar 2015 20:17:42 +0100
> Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
> 
> > Hi,
> > 
> > I'm trying to get rid of at91_suspend_entering_slow_clock() which is
> > exposing the platform suspend_state_t to the devices. From what I
> > understand, whenever suspend_state_t is PM_SUSPEND_MEM or
> > PM_SUSPEND_STANDBY, the pm_message_t passed to the device driver is
> > always PM_EVENT_SUSPEND.
> > 
> > The requirement is to know whether we are going to cut the master clock
> > and in that case, avoid calling enable_irq_wake() because we will not be
> > able to wakeup from the device.
> 
> Actually the master clock is never switched off, we're only changing
> its source (from PLLA to slow clk) which in turn changes its rate.

That's more or less the same, the master clock set to 32k is unusable 
for almost all devices. I guess all except some very simple devices like 
GPIO, AIC and PM controller.


> > Is there a better way to do that? Or should I implement a similar
> > function in the pm core (which I guess would already be there if
> > really needed)?
> 
> IMHO we should let devices (RTC/RTT, debug UART, GPIOC, ...) mark their
> interrupt line as a wakeup interrupt, and adapt the device
> configuration (UART baudrate, ...) according to the new master clock
> rate instead of testing the suspend mode to check whether we can mark
> irq lines as wakeup sources or not.

We are using a 32k clock, 115.2 bits/s which is very common is 3.5x 
faster than tje 32k clock. And, to reduce a bit more the memory 
consumption we can switch of to the 32k RC and not OSC (I do!), which is 
?10% off, that's way too much for UART.

Also, if standby target is chosen, we are able to wake up on USB Host 
wake-up events, which needs the USB PLL to be running. If mem target is 
chosen we are going to switch off the USB PLL because we are going to 
switch off the PLL source, the master clock, in this case we most ensure 
all USB drivers switches off their controller (this is what my USBA and 
EHCI/OHCI PM series did).


> The fact that we're disabling PLLA is not really related to
> suspend-to-ram mode (we could leave the master clock unchanged and
> still put the SDRAM chip in self-refresh mode).

This is what we do in standby target.


> The problem here is that we need some kind of notifier infrastructure
> that would be called before the master clock source is switched to slow
> clock. This notifier must be written in asm so that it can be called
> from the asm code executed from SRAM (the SDRAM chip is placed in self
> refresh before switching to slow clock).

I don't think that's possible, some drivers needs to know the master 
clock is going to be switched off (USB).


Sylvain

PM: knowing the system state in the device callback

[Boris Brezillon]

Hi Sylvain,

On Mon, 16 Mar 2015 21:32:52 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:

> Hi,
> 
> 
> On Mon, Mar 16, 2015 at 08:52:45PM +0100, Boris Brezillon wrote:
> > Hi Alexandre,
> > 
> > On Mon, 16 Mar 2015 20:17:42 +0100
> > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
> > 
> > > Hi,
> > > 
> > > I'm trying to get rid of at91_suspend_entering_slow_clock() which is
> > > exposing the platform suspend_state_t to the devices. From what I
> > > understand, whenever suspend_state_t is PM_SUSPEND_MEM or
> > > PM_SUSPEND_STANDBY, the pm_message_t passed to the device driver is
> > > always PM_EVENT_SUSPEND.
> > > 
> > > The requirement is to know whether we are going to cut the master clock
> > > and in that case, avoid calling enable_irq_wake() because we will not be
> > > able to wakeup from the device.
> > 
> > Actually the master clock is never switched off, we're only changing
> > its source (from PLLA to slow clk) which in turn changes its rate.
> 
> That's more or less the same, the master clock set to 32k is unusable 
> for almost all devices. I guess all except some very simple devices like 
> GPIO, AIC and PM controller.

Okay, I thought suspend-to-ram was only implying putting the SDRAM chip
in self-refresh, and stopping everything that can be stopped.
In the existing code we're actually forcing the switch to the slow
clock even if some devices marked as wakeup sources need a fast master
clock.

> 
> 
> > > Is there a better way to do that? Or should I implement a similar
> > > function in the pm core (which I guess would already be there if
> > > really needed)?
> > 
> > IMHO we should let devices (RTC/RTT, debug UART, GPIOC, ...) mark their
> > interrupt line as a wakeup interrupt, and adapt the device
> > configuration (UART baudrate, ...) according to the new master clock
> > rate instead of testing the suspend mode to check whether we can mark
> > irq lines as wakeup sources or not.
> 
> We are using a 32k clock, 115.2 bits/s which is very common is 3.5x 
> faster than tje 32k clock.

Right.

> And, to reduce a bit more the memory 
> consumption we can switch of to the 32k RC and not OSC (I do!), which is 
> ?10% off, that's way too much for UART.

Hm, I think we should stay focus on the mainline code for now, but I
understand your concern.

> 
> Also, if standby target is chosen, we are able to wake up on USB Host 
> wake-up events, which needs the USB PLL to be running. If mem target is 
> chosen we are going to switch off the USB PLL because we are going to 
> switch off the PLL source, the master clock, in this case we most ensure 
> all USB drivers switches off their controller (this is what my USBA and 
> EHCI/OHCI PM series did).

Well, IMO we should never guess what the user want to do (and that's
what we're currently doing in the existing code).
If someone marked the USB controller as a wakeup source, then we should
keep the USB device active even when entering suspend-to-ram mode.
If this mean consuming more power when USB is a wakeup source, then
it should be fine, because in the end it's the user who chooses which
device should be a wakeup source (through sysfs), and if he really
wants to consume less, he can decide to disable wakeup on USB events.
In the other, letting the user think the system can wakeup on USB while
it actually can't is a bit broken.

> 
> 
> > The fact that we're disabling PLLA is not really related to
> > suspend-to-ram mode (we could leave the master clock unchanged and
> > still put the SDRAM chip in self-refresh mode).
> 
> This is what we do in standby target.

Ok, thanks for the pointer. I though standy mode was just some kind of
cpuidle with a few devices disables (thanks to the PM ops implemented
in each drivers), but apparently I was wrong.

> 
> 
> > The problem here is that we need some kind of notifier infrastructure
> > that would be called before the master clock source is switched to slow
> > clock. This notifier must be written in asm so that it can be called
> > from the asm code executed from SRAM (the SDRAM chip is placed in self
> > refresh before switching to slow clock).
> 
> I don't think that's possible, some drivers needs to know the master 
> clock is going to be switched off (USB).

Yep, you're right again.

Here is another approach thought about:
1/ use clock constraints in device drivers to forbid any change on the
   main clock
2/ remove this constraint when suspend is called if the device is not
   tagged as a wakeup source, or keep it if it is.
3/ call a 'clk_test_rate' function in PM code to check if we can
   switch to a 32kHz clk (clk_test_rate(mck, 32768)).
   This clk_test_rate does not exist, but it would validate all mck
   users constraint, returning an error code if the constraints does
   not allow such a rate or 0 on success.

This is just an idea.
Maybe exposing the current suspend state and testing its value in each
driver is more appropriate, but from a user point of view, I find it
weird to silently ignore the wakeup configuration on some devices when
entering suspend-to-ram.

Best Regards,

Boris

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

PM: knowing the system state in the device callback

[Sylvain Rochet]

Hi,

On Tue, Mar 17, 2015 at 09:38:15AM +0100, Boris Brezillon wrote:
> On Mon, 16 Mar 2015 21:32:52 +0100
> Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> > On Mon, Mar 16, 2015 at 08:52:45PM +0100, Boris Brezillon wrote:
> > > On Mon, 16 Mar 2015 20:17:42 +0100
> > > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
> > > > 
> > > > I'm trying to get rid of at91_suspend_entering_slow_clock() which is
> > > > exposing the platform suspend_state_t to the devices. From what I
> > > > understand, whenever suspend_state_t is PM_SUSPEND_MEM or
> > > > PM_SUSPEND_STANDBY, the pm_message_t passed to the device driver is
> > > > always PM_EVENT_SUSPEND.
> > > > 
> > > > The requirement is to know whether we are going to cut the master clock
> > > > and in that case, avoid calling enable_irq_wake() because we will not be
> > > > able to wakeup from the device.
> > > 
> > > Actually the master clock is never switched off, we're only changing
> > > its source (from PLLA to slow clk) which in turn changes its rate.
> > 
> > That's more or less the same, the master clock set to 32k is unusable 
> > for almost all devices. I guess all except some very simple devices like 
> > GPIO, AIC and PM controller.
> 
> Okay, I thought suspend-to-ram was only implying putting the SDRAM chip
> in self-refresh, and stopping everything that can be stopped.

That's the standby target. What we have currently in linux-next for AT91 
is:

mem target: all PLL stopped, master clock switched slow clock(32k),
            self-refresh, "all" drivers forced suspend even if wake-up
            flag is set, wait-for-interrupt

standby target: fast clocks active(PLL A, USB PLL), self-refresh,
                "all" drivers suspended but wake-up flag is respected
                (e.g. USB controller is NOT suspended if wake-up flag is set),
                wait-for-interrupt


> In the existing code we're actually forcing the switch to the slow
> clock even if some devices marked as wakeup sources need a fast master
> clock.

Yes, in this case we currently discard the wakeup flag, this is bad.


> > And, to reduce a bit more the memory 
> > consumption we can switch of to the 32k RC and not OSC (I do!), which is 
> > ?10% off, that's way too much for UART.
> 
> Hm, I think we should stay focus on the mainline code for now, but I
> understand your concern.

This is what mainline is doing, at least on SAMA5D3 ;-)


> > Also, if standby target is chosen, we are able to wake up on USB Host 
> > wake-up events, which needs the USB PLL to be running. If mem target is 
> > chosen we are going to switch off the USB PLL because we are going to 
> > switch off the PLL source, the master clock, in this case we most ensure 
> > all USB drivers switches off their controller (this is what my USBA and 
> > EHCI/OHCI PM series did).
> 
> Well, IMO we should never guess what the user want to do (and that's
> what we're currently doing in the existing code).
> If someone marked the USB controller as a wakeup source, then we should
> keep the USB device active even when entering suspend-to-ram mode.
> If this mean consuming more power when USB is a wakeup source, then
> it should be fine, because in the end it's the user who chooses which
> device should be a wakeup source (through sysfs), and if he really
> wants to consume less, he can decide to disable wakeup on USB events.
> In the other, letting the user think the system can wakeup on USB while
> it actually can't is a bit broken.

I mostly agree. In my opinion we should just abort the mem target 
suspend if some peripherals have wake-up bit set and need the master 
clock (or USB PLL, or PLL B, or any clock divider/multiplier from master 
clock) to do wake up. We are actually more or less doing that using the 
at91_pm_verify_clocks() function.


> > > The fact that we're disabling PLLA is not really related to
> > > suspend-to-ram mode (we could leave the master clock unchanged and
> > > still put the SDRAM chip in self-refresh mode).
> > 
> > This is what we do in standby target.
> 
> Ok, thanks for the pointer. I though standy mode was just some kind of
> cpuidle with a few devices disables (thanks to the PM ops implemented
> in each drivers), but apparently I was wrong.

Yes, the only difference between standby and mem target is the switch to 
slow clock mode.


> > > The problem here is that we need some kind of notifier infrastructure
> > > that would be called before the master clock source is switched to slow
> > > clock. This notifier must be written in asm so that it can be called
> > > from the asm code executed from SRAM (the SDRAM chip is placed in self
> > > refresh before switching to slow clock).
> > 
> > I don't think that's possible, some drivers needs to know the master 
> > clock is going to be switched off (USB).
> 
> Yep, you're right again.
> 
> Here is another approach thought about:
> 1/ use clock constraints in device drivers to forbid any change on the
>    main clock
> 2/ remove this constraint when suspend is called if the device is not
>    tagged as a wakeup source, or keep it if it is.
> 3/ call a 'clk_test_rate' function in PM code to check if we can
>    switch to a 32kHz clk (clk_test_rate(mck, 32768)).
>    This clk_test_rate does not exist, but it would validate all mck
>    users constraint, returning an error code if the constraints does
>    not allow such a rate or 0 on success.
> 
> This is just an idea.
> Maybe exposing the current suspend state and testing its value in each
> driver is more appropriate,

Well, I don't know if it's more appropriate or not, but testing the 
suspend state is much much simpler than adding a clk_test_rate() :-)


> but from a user point of view, I find it weird to silently ignore the 
> wakeup configuration on some devices when entering suspend-to-ram.

I agree that's weird.


Sylvain	

PM: knowing the system state in the device callback

[Gregory CLEMENT]

Hi,


On 16/03/2015 20:17, Alexandre Belloni wrote:
> Hi,
> 
> I'm trying to get rid of at91_suspend_entering_slow_clock() which is
> exposing the platform suspend_state_t to the devices. From what I
> understand, whenever suspend_state_t is PM_SUSPEND_MEM or
> PM_SUSPEND_STANDBY, the pm_message_t passed to the device driver is
> always PM_EVENT_SUSPEND.
> 
> The requirement is to know whether we are going to cut the master clock
> and in that case, avoid calling enable_irq_wake() because we will not be
> able to wakeup from the device.
> 
> Is there a better way to do that? Or should I implement a similar
> function in the pm core (which I guess would already be there if
> really needed)?

I am interesting by this topic because I am working on adding standby
support for the mvebu SoCs (or at least some of them). In suspend to
ram, the SoC is completely shutdown, whereas in standby it will remain
powered. So for standby we need to do extra steps such as switching off
the SerDes. Initially I though that I could use the PM_SUSPEND_STANDBY
but as pointed by Alexandre it is not this event which is sent to the
drivers.

With the current code, it seems that the specific work for standby is
done at machine level, how could you also doing it at device level?


Thanks,

Gregory



> 
> Regards,
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com