* PM: knowing the system state in the device callback @ 2015-03-16 19:17 ` Alexandre Belloni 0 siblings, 0 replies; 12+ messages in thread From: Alexandre Belloni @ 2015-03-16 19:17 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Pavel Machek, Boris Brezillon, Nicolas Ferre, linux-pm, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* PM: knowing the system state in the device callback @ 2015-03-16 19:17 ` Alexandre Belloni 0 siblings, 0 replies; 12+ messages in thread From: Alexandre Belloni @ 2015-03-16 19:17 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PM: knowing the system state in the device callback 2015-03-16 19:17 ` Alexandre Belloni @ 2015-03-16 19:52 ` Boris Brezillon -1 siblings, 0 replies; 12+ messages in thread From: Boris Brezillon @ 2015-03-16 19:52 UTC (permalink / raw) To: Alexandre Belloni Cc: Rafael J. Wysocki, Pavel Machek, Nicolas Ferre, linux-pm, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* PM: knowing the system state in the device callback @ 2015-03-16 19:52 ` Boris Brezillon 0 siblings, 0 replies; 12+ messages in thread From: Boris Brezillon @ 2015-03-16 19:52 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PM: knowing the system state in the device callback 2015-03-16 19:52 ` Boris Brezillon @ 2015-03-16 20:32 ` Sylvain Rochet -1 siblings, 0 replies; 12+ messages in thread From: Sylvain Rochet @ 2015-03-16 20:32 UTC (permalink / raw) To: Boris Brezillon Cc: Alexandre Belloni, linux-pm, Rafael J. Wysocki, Nicolas Ferre, linux-kernel, Pavel Machek, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* PM: knowing the system state in the device callback @ 2015-03-16 20:32 ` Sylvain Rochet 0 siblings, 0 replies; 12+ messages in thread From: Sylvain Rochet @ 2015-03-16 20:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PM: knowing the system state in the device callback 2015-03-16 20:32 ` Sylvain Rochet @ 2015-03-17 8:38 ` Boris Brezillon -1 siblings, 0 replies; 12+ messages in thread From: Boris Brezillon @ 2015-03-17 8:38 UTC (permalink / raw) To: Sylvain Rochet Cc: Alexandre Belloni, linux-pm, Rafael J. Wysocki, Nicolas Ferre, linux-kernel, Pavel Machek, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* PM: knowing the system state in the device callback @ 2015-03-17 8:38 ` Boris Brezillon 0 siblings, 0 replies; 12+ messages in thread From: Boris Brezillon @ 2015-03-17 8:38 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PM: knowing the system state in the device callback 2015-03-17 8:38 ` Boris Brezillon @ 2015-03-17 10:46 ` Sylvain Rochet -1 siblings, 0 replies; 12+ messages in thread From: Sylvain Rochet @ 2015-03-17 10:46 UTC (permalink / raw) To: Boris Brezillon Cc: Alexandre Belloni, linux-pm, Rafael J. Wysocki, Nicolas Ferre, linux-kernel, Pavel Machek, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* PM: knowing the system state in the device callback @ 2015-03-17 10:46 ` Sylvain Rochet 0 siblings, 0 replies; 12+ messages in thread From: Sylvain Rochet @ 2015-03-17 10:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PM: knowing the system state in the device callback 2015-03-16 19:17 ` Alexandre Belloni @ 2015-03-17 12:27 ` Gregory CLEMENT -1 siblings, 0 replies; 12+ messages in thread From: Gregory CLEMENT @ 2015-03-17 12:27 UTC (permalink / raw) To: Alexandre Belloni, Rafael J. Wysocki Cc: Boris Brezillon, linux-pm, Nicolas Ferre, linux-kernel, Pavel Machek, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* PM: knowing the system state in the device callback @ 2015-03-17 12:27 ` Gregory CLEMENT 0 siblings, 0 replies; 12+ messages in thread From: Gregory CLEMENT @ 2015-03-17 12:27 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-03-17 12:27 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-16 19:17 PM: knowing the system state in the device callback Alexandre Belloni 2015-03-16 19:17 ` Alexandre Belloni 2015-03-16 19:52 ` Boris Brezillon 2015-03-16 19:52 ` Boris Brezillon 2015-03-16 20:32 ` Sylvain Rochet 2015-03-16 20:32 ` Sylvain Rochet 2015-03-17 8:38 ` Boris Brezillon 2015-03-17 8:38 ` Boris Brezillon 2015-03-17 10:46 ` Sylvain Rochet 2015-03-17 10:46 ` Sylvain Rochet 2015-03-17 12:27 ` Gregory CLEMENT 2015-03-17 12:27 ` Gregory CLEMENT
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.