| Message ID | 1421609766-11204-4-git-send-email-sylvain.rochet@finsecur.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Sun, Jan 18, 2015 at 08:36:03PM +0100, Sylvain Rochet wrote: > Disable/unprepare clocks without testing the sleep target_state, removed > the at91_suspend_entering_slow_clock() call (which is only a > target_state == PM_SUSPEND_MEM). > > Other kind of suspend now benefit from the power save induced by this > PLL deactivation. The resume penalty is about 500 us, which is not > negligible but acceptable considering the amount of power we are saving. Well, this patch actually lights up a previous issue on wake up support. This device needs to be continuously clocked to provide wake up support, previously, if STANDBY target were chosen the device were enable_irq_wake-prepared and clock still active and if MEM target were chosen the device were also enable_irq_wake-prepared but not clocked anymore, which is wrong, but not so wrong. This patch actually breaks wake up support for STANDBY sleep target. I am going to propose a v5 which reinitiate at91_suspend_entering_slow_clock() and fix wake up support on this driver. Sylvain
USB: host: Atmel OHCI and EHCI drivers improvements
Suspend/resume support for EHCI.
struct dev_pm_ops for OHCI.
Removed global variables from both.
Fixed OHCI wake up support for STANDBY(wake-up enabled) and MEM(wake-up
disabled) sleep targets.
Changes since v4:
* Re-add at91_suspend_entering_slow_clock() to OHCI, we can't naively
remove this one, this device needs to be continuously clocked to
provide wake up support.
The removal of at91_suspend_entering_slow_clock() actually lighted up
an issue on wake up support, which is now fixed.
Changes since v3:
* Using struct dev_pm_ops instead of static struct platform_driver
resume and suspend bindings for both EHCI and OHCI
* Fixed inconsistency in patch subjects, _ intead of - for file names
* Patch cleaning with the help of checkpatch.pl, fixed lines over
80 characters
Changes since v2:
* Added patchs from an other submission, because this series
depended on this one
* EHCI: Move global variables to private struct
* OHCI: Move global variables to private struct
* Using ohci->priv and ehci->priv instead of hcd->hcd_priv,
which were not the right way to do that
Changes since v1:
* Don't use at91_suspend_entering_slow_clock() on EHCI,
we are trying to get read of this of this function
* Removed at91_suspend_entering_slow_clock() from OHCI
Sylvain Rochet (6):
USB: host: ehci-atmel: Add suspend/resume support
USB: host: ohci-at91: Use struct dev_pm_ops instead of struct
platform_driver
USB: host: ehci-atmel: Move global variables to private struct
USB: host: ohci-at91: Fix wake-up support
USB: host: ohci-at91: Move global variables to private struct
USB: host: ohci-at91: usb_hcd_at91_probe(), remove useless stack
initialisation
drivers/usb/host/ehci-atmel.c | 102 ++++++++++++++++++++++++++---------
drivers/usb/host/ohci-at91.c | 120 +++++++++++++++++++++++++-----------------
2 files changed, 150 insertions(+), 72 deletions(-)
Le 18/01/2015 23:25, Sylvain Rochet a écrit : > USB: host: Atmel OHCI and EHCI drivers improvements > > Suspend/resume support for EHCI. > struct dev_pm_ops for OHCI. > Removed global variables from both. > Fixed OHCI wake up support for STANDBY(wake-up enabled) and MEM(wake-up > disabled) sleep targets. I'm okay with the whole series: Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Alan, It seems that Boris and Alexandre also add their tag to the series already. Do you want us to collect them in a new series sent to you? Sylvain, Thanks a lot for this nice series built at a high pace ;-) Even if I'm not sure to keep the separation between "slow clock" PM and "normal" PM on AT91 but more likely to use the "slow clock" for all the PM modes, let's go forward with this step for now. I mean, we will certainly rework this at91_suspend_entering_slow_clock() aspect in the future. Bye, > Changes since v4: > * Re-add at91_suspend_entering_slow_clock() to OHCI, we can't naively > remove this one, this device needs to be continuously clocked to > provide wake up support. > The removal of at91_suspend_entering_slow_clock() actually lighted up > an issue on wake up support, which is now fixed. > > Changes since v3: > * Using struct dev_pm_ops instead of static struct platform_driver > resume and suspend bindings for both EHCI and OHCI > * Fixed inconsistency in patch subjects, _ intead of - for file names > * Patch cleaning with the help of checkpatch.pl, fixed lines over > 80 characters > > Changes since v2: > * Added patchs from an other submission, because this series > depended on this one > * EHCI: Move global variables to private struct > * OHCI: Move global variables to private struct > * Using ohci->priv and ehci->priv instead of hcd->hcd_priv, > which were not the right way to do that > > Changes since v1: > * Don't use at91_suspend_entering_slow_clock() on EHCI, > we are trying to get read of this of this function > * Removed at91_suspend_entering_slow_clock() from OHCI > > Sylvain Rochet (6): > USB: host: ehci-atmel: Add suspend/resume support > USB: host: ohci-at91: Use struct dev_pm_ops instead of struct > platform_driver > USB: host: ehci-atmel: Move global variables to private struct > USB: host: ohci-at91: Fix wake-up support > USB: host: ohci-at91: Move global variables to private struct > USB: host: ohci-at91: usb_hcd_at91_probe(), remove useless stack > initialisation > > drivers/usb/host/ehci-atmel.c | 102 ++++++++++++++++++++++++++--------- > drivers/usb/host/ohci-at91.c | 120 +++++++++++++++++++++++++----------------- > 2 files changed, 150 insertions(+), 72 deletions(-) >
Hello Nicolas, On Mon, Jan 19, 2015 at 02:34:20PM +0100, Nicolas Ferre wrote: > Le 18/01/2015 23:25, Sylvain Rochet a écrit : > > USB: host: Atmel OHCI and EHCI drivers improvements > > > > Suspend/resume support for EHCI. > > struct dev_pm_ops for OHCI. > > Removed global variables from both. > > Fixed OHCI wake up support for STANDBY(wake-up enabled) and MEM(wake-up > > disabled) sleep targets. > > I'm okay with the whole series: > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Oops, you missed that, but there is a v6 I sent this morning, which fixes a small mistake I made in v5. > Alan, It seems that Boris and Alexandre also add their tag to the > series already. Do you want us to collect them in a new series sent to > you? > > Sylvain, > Thanks a lot for this nice series built at a high pace ;-) Yeah, I have the feeling I messed up a "little" :-) Anyway, v6 is fine, I hope. > Even if I'm not sure to keep the separation between "slow clock" PM and > "normal" PM on AT91 but more likely to use the "slow clock" for all the > PM modes, let's go forward with this step for now. I mean, we will > certainly rework this at91_suspend_entering_slow_clock() aspect in the > future. I agree, I discussed privately with Boris about that, this require a larger rework, lets only fix the OHCI wakeup bug for now. Sylvain
On Mon, 19 Jan 2015, Nicolas Ferre wrote: > Alan, > It seems that Boris and Alexandre also add their tag to the series > already. Do you want us to collect them in a new series sent to you? Sylvain has already sent a new series, v6. I suppose it would make life a little easier for Greg KH (who will do the actual merge) if the patches are resent with all the relevant tags, after I have a chance to look through them in detail. Alan Stern
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c index 65e7836..79e343e 100644 --- a/drivers/usb/host/ohci-at91.c +++ b/drivers/usb/host/ohci-at91.c @@ -49,6 +49,8 @@ extern int usb_disabled(void); static void at91_start_clock(void) { + if (clocked) + return; if (IS_ENABLED(CONFIG_COMMON_CLK)) { clk_set_rate(uclk, 48000000); clk_prepare_enable(uclk); @@ -61,6 +63,8 @@ static void at91_start_clock(void) static void at91_stop_clock(void) { + if (!clocked) + return; clk_disable_unprepare(fclk); clk_disable_unprepare(iclk); clk_disable_unprepare(hclk); @@ -615,16 +619,14 @@ ohci_hcd_at91_drv_suspend(struct device *dev) * * REVISIT: some boards will be able to turn VBUS off... */ - if (at91_suspend_entering_slow_clock()) { - ohci->hc_control = ohci_readl(ohci, &ohci->regs->control); - ohci->hc_control &= OHCI_CTRL_RWC; - ohci_writel(ohci, ohci->hc_control, &ohci->regs->control); - ohci->rh_state = OHCI_RH_HALTED; - - /* flush the writes */ - (void) ohci_readl (ohci, &ohci->regs->control); - at91_stop_clock(); - } + ohci->hc_control = ohci_readl(ohci, &ohci->regs->control); + ohci->hc_control &= OHCI_CTRL_RWC; + ohci_writel(ohci, ohci->hc_control, &ohci->regs->control); + ohci->rh_state = OHCI_RH_HALTED; + + /* flush the writes */ + (void) ohci_readl (ohci, &ohci->regs->control); + at91_stop_clock(); return ret; } @@ -636,8 +638,7 @@ static int ohci_hcd_at91_drv_resume(struct device *dev) if (device_may_wakeup(dev)) disable_irq_wake(hcd->irq); - if (!clocked) - at91_start_clock(); + at91_start_clock(); ohci_resume(hcd, false); return 0;
Disable/unprepare clocks without testing the sleep target_state, removed the at91_suspend_entering_slow_clock() call (which is only a target_state == PM_SUSPEND_MEM). Other kind of suspend now benefit from the power save induced by this PLL deactivation. The resume penalty is about 500 us, which is not negligible but acceptable considering the amount of power we are saving. Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/usb/host/ohci-at91.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)