| Message ID | 1421437274-31615-1-git-send-email-sylvain.rochet@finsecur.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, You should probably put the susbsytem maintainers in copy too. As reported by get_maintainer.pl: Alan Stern <stern@rowland.harvard.edu> (maintainer:USB EHCI DRIVER) Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) They will be the one taking the patch. And when dealing with PM on AT91, please copy Wenyou Yang <wenyou.yang@atmel.com> On 16/01/2015 at 20:41:14 +0100, Sylvain Rochet wrote : > This patch add suspend/resume support for Atmel EHCI, mostly > about disabling and unpreparing clocks so USB PLL is stopped > before entering sleep state. > > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > + > + if (at91_suspend_entering_slow_clock()) > + atmel_stop_clock(); > + We should definitely find a way to get rid of at91_suspend_entering_slow_clock() at some point in time.
Hi Sylvain, On Sat, 17 Jan 2015 02:34:42 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > Hi, > > You should probably put the susbsytem maintainers in copy too. As > reported by get_maintainer.pl: > Alan Stern <stern@rowland.harvard.edu> (maintainer:USB EHCI DRIVER) > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) > > They will be the one taking the patch. > > And when dealing with PM on AT91, please copy > Wenyou Yang <wenyou.yang@atmel.com> > > > On 16/01/2015 at 20:41:14 +0100, Sylvain Rochet wrote : > > This patch add suspend/resume support for Atmel EHCI, mostly > > about disabling and unpreparing clocks so USB PLL is stopped > > before entering sleep state. > > > > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> > > Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > > + > > + if (at91_suspend_entering_slow_clock()) > > + atmel_stop_clock(); > > + > > We should definitely find a way to get rid of > at91_suspend_entering_slow_clock() at some point in time. > > Can't we just disable clocks without testing for target_state == PM_SUSPEND_MEM (which is exactly what at91_suspend_entering_slow_clock does [1]) when entering suspend ? I mean, IMHO other kind of suspend should still benefit from the power save induced by this PLL deactivation. Is there such a big penalty when resuming the device if the PLL and peripheral clocks are disabled ? [1]http://lxr.free-electrons.com/source/arch/arm/mach-at91/pm.c#L116
Hi Boris, On Sat, Jan 17, 2015 at 10:36:09AM +0100, Boris Brezillon wrote: > On Sat, 17 Jan 2015 02:34:42 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > We should definitely find a way to get rid of > > at91_suspend_entering_slow_clock() at some point in time. > > Can't we just disable clocks without testing for target_state == > PM_SUSPEND_MEM (which is exactly what at91_suspend_entering_slow_clock > does [1]) when entering suspend ? > I mean, IMHO other kind of suspend should still benefit from the power > save induced by this PLL deactivation. I agree, but it depends on what we mean with standby vs mem, there should be a difference between the two sleep mode. This behavior follows what the Atmel OHCI driver is currently doing. > Is there such a big penalty when resuming the device if the PLL and > peripheral clocks are disabled ? There is a penalty, starting up a PLL takes about 500 µs, however I can't decide if this is a small or a big penalty. Sylvain
On Sat, 17 Jan 2015 11:43:00 +0100 Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > Hi Boris, > > > On Sat, Jan 17, 2015 at 10:36:09AM +0100, Boris Brezillon wrote: > > On Sat, 17 Jan 2015 02:34:42 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > > We should definitely find a way to get rid of > > > at91_suspend_entering_slow_clock() at some point in time. > > > > Can't we just disable clocks without testing for target_state == > > PM_SUSPEND_MEM (which is exactly what at91_suspend_entering_slow_clock > > does [1]) when entering suspend ? > > I mean, IMHO other kind of suspend should still benefit from the power > > save induced by this PLL deactivation. > > I agree, but it depends on what we mean with standby vs mem, there > should be a difference between the two sleep mode. AFAIU PM_SUSPEND_MEM mode only implies putting the SDRAM in self-refresh mode to avoid waking the processor up for periodic SDRAM bank refresh. IMO this should not impact other peripherals behavior (BTW I haven't found any USB driver testing for this suspend state before disabling their clks). And what if we decide to implement runtime PM for this peripheral ? Shouldn't we disable these clks (which are one of the main source of power consumption of this IP) ? > > This behavior follows what the Atmel OHCI driver is currently doing. Yes, but it doesn't mean we should do the same ;-), especially since we're trying to remove this at91_suspend_entering_slow_clock function. > > > > Is there such a big penalty when resuming the device if the PLL and > > peripheral clocks are disabled ? > > There is a penalty, starting up a PLL takes about 500 µs, however I > can't decide if this is a small or a big penalty. That's indeed not negligible, but when one enters suspend, I guess it does expect some latency to resume from the suspended state.
Hello Boris, On Sat, Jan 17, 2015 at 01:58:57PM +0100, Boris Brezillon wrote: > On Sat, 17 Jan 2015 11:43:00 +0100 Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > > > > I agree, but it depends on what we mean with standby vs mem, there > > should be a difference between the two sleep mode. > > AFAIU PM_SUSPEND_MEM mode only implies putting the SDRAM in > self-refresh mode to avoid waking the processor up for periodic SDRAM > bank refresh. > IMO this should not impact other peripherals behavior (BTW I haven't > found any USB driver testing for this suspend state before disabling > their clks). This is my understanding as well. > And what if we decide to implement runtime PM for this peripheral ? > Shouldn't we disable these clks (which are one of the main source of > power consumption of this IP) ? Of course we should, disengaging clocks and stopping unused PLL and crystals is almost the only way to save power. Should I have added the "Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>" on 1/2 ? > > This behavior follows what the Atmel OHCI driver is currently doing. > > Yes, but it doesn't mean we should do the same ;-), especially since > we're trying to remove this at91_suspend_entering_slow_clock function. Arrg, this is not what I meant ;-), anyway, proposing as well a change which remove the at91_suspend_entering_slow_clock() from OHCI. I am not so happy with the global clocked boolean, but I guess this is necessary if a suspended device gets removed. > > > Is there such a big penalty when resuming the device if the PLL and > > > peripheral clocks are disabled ? > > > > There is a penalty, starting up a PLL takes about 500 µs, however I > > can't decide if this is a small or a big penalty. > > That's indeed not negligible, but when one enters suspend, I guess it > does expect some latency to resume from the suspended state. Overall resume of all drivers takes about 300 ms, whichever chosen suspend mode. Suspend to ram with slow clock mode enabled adds about 15 ms of resume time. As a side note, unfortunately AT91 slow clock mode only work on "recent" boards with Wenyou changes on linux4sam/linux-at91/linux-3.10-at91 branch and is why I am still stuck to this branch (and the HLCDC FB driver, but this is going to be merged soon from what I saw, YAY !). Sylvain
On 17/01/2015 at 17:03:04 +0100, Sylvain Rochet wrote : > Should I have added the "Acked-by: Alexandre Belloni > <alexandre.belloni@free-electrons.com>" on 1/2 ? > Yes, you could. Just a small remark, you send both patches as separate in v1, you should have kept them separate so it is easier to understand what v2 is relating to. Also, I don't think you need to use --in-reply-to when sending v2. Finally, please including a changelog after the '---' marker in each patch or in the cover letter. Those are simply small things and your work is great. > As a side note, unfortunately AT91 slow clock mode only work on "recent" > boards with Wenyou changes on linux4sam/linux-at91/linux-3.10-at91 > branch and is why I am still stuck to this branch (and the HLCDC FB > driver, but this is going to be merged soon from what I saw, YAY !). > Work is in progress to get that in 3.20 or 3.21.
diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c index 56a8850..790de92 100644 --- a/drivers/usb/host/ehci-atmel.c +++ b/drivers/usb/host/ehci-atmel.c @@ -19,6 +19,7 @@ #include <linux/of.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/platform_data/atmel.h> #include <linux/usb.h> #include <linux/usb/hcd.h> @@ -174,6 +175,36 @@ static int ehci_atmel_drv_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM +static int ehci_atmel_drv_suspend(struct platform_device *pdev, pm_message_t mesg) +{ + struct usb_hcd *hcd = platform_get_drvdata(pdev); + int ret; + + ret = ehci_suspend(hcd, false); + if (ret) + return ret; + + if (at91_suspend_entering_slow_clock()) + atmel_stop_clock(); + + return 0; +} + +static int ehci_atmel_drv_resume(struct platform_device *pdev) +{ + struct usb_hcd *hcd = platform_get_drvdata(pdev); + + if (!clocked) + atmel_start_clock(); + + return ehci_resume(hcd, false); +} +#else +#define ehci_atmel_drv_suspend NULL +#define ehci_atmel_drv_resume NULL +#endif + #ifdef CONFIG_OF static const struct of_device_id atmel_ehci_dt_ids[] = { { .compatible = "atmel,at91sam9g45-ehci" }, @@ -187,6 +218,8 @@ static struct platform_driver ehci_atmel_driver = { .probe = ehci_atmel_drv_probe, .remove = ehci_atmel_drv_remove, .shutdown = usb_hcd_platform_shutdown, + .suspend = ehci_atmel_drv_suspend, + .resume = ehci_atmel_drv_resume, .driver = { .name = "atmel-ehci", .of_match_table = of_match_ptr(atmel_ehci_dt_ids),
This patch add suspend/resume support for Atmel EHCI, mostly about disabling and unpreparing clocks so USB PLL is stopped before entering sleep state. Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> --- drivers/usb/host/ehci-atmel.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)