| Message ID | 1421446870-26653-1-git-send-email-sylvain.rochet@finsecur.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 16/01/2015 at 23:21:10 +0100, Sylvain Rochet wrote : > Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce > power consumption if USB PLL is not already necessary for OHCI or EHCI. > If USB host is not connected we can sleep with USB PLL stopped. > > This driver does not support suspend/resume yet, not suspending if we > are still attached to an USB host is fine for what I need, this patch > allow suspending with USB PLL stopped when USB device is not currently > used. > Same as your previous patch, the maintainers are: Felipe Balbi <balbi@ti.com> (maintainer:USB GADGET/PERIPH...) Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c > index ce88237..8ea0a63 100644 > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > + ret = clk_prepare_enable(udc->pclk); > + if (ret) > + goto out; > + ret = clk_prepare_enable(udc->hclk); > + if (ret) { > + clk_disable_unprepare(udc->pclk); > + goto out; You probably got a warning at some point in time, you can't use clk_prepare or clk_unprepare in an irq handler as they may sleep (that is exactly the point of having clk_prepare ans clk_enable) So, use clk_enable and clk_disable here. > reset_all_endpoints(udc); > toggle_bias(0); > usba_writel(udc, CTRL, USBA_DISABLE_MASK); > + > + clk_disable_unprepare(udc->hclk); > + clk_disable_unprepare(udc->pclk); > + Ditto > if (udc->driver->disconnect) { > spin_unlock(&udc->lock); > udc->driver->disconnect(&udc->gadget); > @@ -1772,15 +1786,6 @@ static int atmel_usba_start(struct usb_gadget *gadget, > udc->driver = driver; > spin_unlock_irqrestore(&udc->lock, flags); > > - ret = clk_prepare_enable(udc->pclk); > - if (ret) > - return ret; > - ret = clk_prepare_enable(udc->hclk); > - if (ret) { > - clk_disable_unprepare(udc->pclk); > - return ret; > - } > - Keep clk_prepare and clk_unprepare here. > udc->vbus_prev = 0; > if (gpio_is_valid(udc->vbus_pin)) > enable_irq(gpio_to_irq(udc->vbus_pin)); > @@ -1788,13 +1793,27 @@ static int atmel_usba_start(struct usb_gadget *gadget, > /* If Vbus is present, enable the controller and wait for reset */ > spin_lock_irqsave(&udc->lock, flags); > if (vbus_is_present(udc) && udc->vbus_prev == 0) { > + ret = clk_prepare_enable(udc->pclk); > + if (ret) > + goto out; > + ret = clk_prepare_enable(udc->hclk); > + if (ret) { > + clk_disable_unprepare(udc->pclk); > + goto out; > + } > + clk_enable/clk_disable here. > toggle_bias(1); > usba_writel(udc, CTRL, USBA_ENABLE_MASK); > usba_writel(udc, INT_ENB, USBA_END_OF_RESET); > + > + udc->vbus_prev = 1; > } > spin_unlock_irqrestore(&udc->lock, flags); > > return 0; > +out: > + spin_unlock_irqrestore(&udc->lock, flags); > + return ret; > } >
Hi, On Sat, 17 Jan 2015 02:42:31 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > Hi, > > On 16/01/2015 at 23:21:10 +0100, Sylvain Rochet wrote : > > Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce > > power consumption if USB PLL is not already necessary for OHCI or EHCI. > > If USB host is not connected we can sleep with USB PLL stopped. > > > > This driver does not support suspend/resume yet, not suspending if we > > are still attached to an USB host is fine for what I need, this patch > > allow suspending with USB PLL stopped when USB device is not currently > > used. > > > > Same as your previous patch, the maintainers are: > Felipe Balbi <balbi@ti.com> (maintainer:USB GADGET/PERIPH...) > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) > > > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c > > index ce88237..8ea0a63 100644 > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > > + ret = clk_prepare_enable(udc->pclk); > > + if (ret) > > + goto out; > > + ret = clk_prepare_enable(udc->hclk); > > + if (ret) { > > + clk_disable_unprepare(udc->pclk); > > + goto out; > > > You probably got a warning at some point in time, you can't use > clk_prepare or clk_unprepare in an irq handler as they may sleep (that > is exactly the point of having clk_prepare ans clk_enable) > > So, use clk_enable and clk_disable here. You're right, except that calling enable/disable on the PLL clk in irq context is pretty much useless since the activation/deactivation code of the PLL is in prepare/unprepare, so you won't save much power by doing that (gating the peripheral clk should save a bit though). To solve that issue I thought we could move to a threaded_irq (where we can safely sleep), but you'll also have to take of not calling prepare/unprepare in sections where at least one spinlock is taken (for the same reason => you cannot sleep with while you hold spinlocks). Best Regards, Boris
Hello Boris, On Sat, Jan 17, 2015 at 10:43:58AM +0100, Boris Brezillon wrote: > > You're right, except that calling enable/disable on the PLL clk in irq > context is pretty much useless since the activation/deactivation code > of the PLL is in prepare/unprepare, so you won't save much power by > doing that (gating the peripheral clk should save a bit though). > > To solve that issue I thought we could move to a threaded_irq (where we > can safely sleep), but you'll also have to take of not calling > prepare/unprepare in sections where at least one spinlock is taken (for > the same reason => you cannot sleep with while you hold spinlocks). Hummm, I must admit I waited for such a useful tip, adding the prepare/unprepare in interrupt context looked dirty at first sight, thank you very much :) I will try with a threaded_irq. Sylvain
Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce power consumption if USB PLL is not already necessary for OHCI or EHCI. If USB host is not connected we can sleep with USB PLL stopped. This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used. Change since v1: * Using a threaded irq and mutex instead of spinclock as suggested * Moved a silently fixed bug in a separate patch (1/2) Sylvain Rochet (2): USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change drivers/usb/gadget/udc/atmel_usba_udc.c | 97 ++++++++++++++++++++++++--------- drivers/usb/gadget/udc/atmel_usba_udc.h | 4 ++ 2 files changed, 75 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index ce88237..8ea0a63 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -1723,6 +1723,7 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) { struct usba_udc *udc = devid; int vbus; + int ret; /* debounce */ udelay(10); @@ -1736,6 +1737,15 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) vbus = vbus_is_present(udc); if (vbus != udc->vbus_prev) { if (vbus) { + ret = clk_prepare_enable(udc->pclk); + if (ret) + goto out; + ret = clk_prepare_enable(udc->hclk); + if (ret) { + clk_disable_unprepare(udc->pclk); + goto out; + } + toggle_bias(1); usba_writel(udc, CTRL, USBA_ENABLE_MASK); usba_writel(udc, INT_ENB, USBA_END_OF_RESET); @@ -1744,6 +1754,10 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) reset_all_endpoints(udc); toggle_bias(0); usba_writel(udc, CTRL, USBA_DISABLE_MASK); + + clk_disable_unprepare(udc->hclk); + clk_disable_unprepare(udc->pclk); + if (udc->driver->disconnect) { spin_unlock(&udc->lock); udc->driver->disconnect(&udc->gadget); @@ -1772,15 +1786,6 @@ static int atmel_usba_start(struct usb_gadget *gadget, udc->driver = driver; spin_unlock_irqrestore(&udc->lock, flags); - ret = clk_prepare_enable(udc->pclk); - if (ret) - return ret; - ret = clk_prepare_enable(udc->hclk); - if (ret) { - clk_disable_unprepare(udc->pclk); - return ret; - } - udc->vbus_prev = 0; if (gpio_is_valid(udc->vbus_pin)) enable_irq(gpio_to_irq(udc->vbus_pin)); @@ -1788,13 +1793,27 @@ static int atmel_usba_start(struct usb_gadget *gadget, /* If Vbus is present, enable the controller and wait for reset */ spin_lock_irqsave(&udc->lock, flags); if (vbus_is_present(udc) && udc->vbus_prev == 0) { + ret = clk_prepare_enable(udc->pclk); + if (ret) + goto out; + ret = clk_prepare_enable(udc->hclk); + if (ret) { + clk_disable_unprepare(udc->pclk); + goto out; + } + toggle_bias(1); usba_writel(udc, CTRL, USBA_ENABLE_MASK); usba_writel(udc, INT_ENB, USBA_END_OF_RESET); + + udc->vbus_prev = 1; } spin_unlock_irqrestore(&udc->lock, flags); return 0; +out: + spin_unlock_irqrestore(&udc->lock, flags); + return ret; } static int atmel_usba_stop(struct usb_gadget *gadget)
Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce power consumption if USB PLL is not already necessary for OHCI or EHCI. If USB host is not connected we can sleep with USB PLL stopped. This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used. Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> --- drivers/usb/gadget/udc/atmel_usba_udc.c | 37 +++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 9 deletions(-)