* [PATCH] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
@ 2015-01-16 22:21 Sylvain Rochet
2015-01-17 1:42 ` Alexandre Belloni
0 siblings, 1 reply; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-16 22:21 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
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)
--
2.1.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-16 22:21 [PATCH] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
@ 2015-01-17 1:42 ` Alexandre Belloni
2015-01-17 9:43 ` Boris Brezillon
0 siblings, 1 reply; 20+ messages in thread
From: Alexandre Belloni @ 2015-01-17 1:42 UTC (permalink / raw)
To: linux-arm-kernel
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;
> }
>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-17 1:42 ` Alexandre Belloni
@ 2015-01-17 9:43 ` Boris Brezillon
2015-01-17 11:07 ` Sylvain Rochet
0 siblings, 1 reply; 20+ messages in thread
From: Boris Brezillon @ 2015-01-17 9:43 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-17 9:43 ` Boris Brezillon
@ 2015-01-17 11:07 ` Sylvain Rochet
2015-01-18 14:51 ` [PATCHv2 0/2] " Sylvain Rochet
0 siblings, 1 reply; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-17 11:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 0/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-17 11:07 ` Sylvain Rochet
@ 2015-01-18 14:51 ` Sylvain Rochet
2015-01-18 14:51 ` [PATCHv2 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
2015-01-18 14:51 ` [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
0 siblings, 2 replies; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-18 14:51 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
--
2.1.4
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
2015-01-18 14:51 ` [PATCHv2 0/2] " Sylvain Rochet
@ 2015-01-18 14:51 ` Sylvain Rochet
2015-01-18 14:51 ` [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
1 sibling, 0 replies; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-18 14:51 UTC (permalink / raw)
To: linux-arm-kernel
If vbus gpio is high at init, we should set vbus_prev to true
accordingly to the current vbus state. Without that, we skip the first
vbus interrupt because the saved vbus state is not consistent.
Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
drivers/usb/gadget/udc/atmel_usba_udc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index ce88237..e207d75 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1791,6 +1791,8 @@ static int atmel_usba_start(struct usb_gadget *gadget,
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);
--
2.1.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-18 14:51 ` [PATCHv2 0/2] " Sylvain Rochet
2015-01-18 14:51 ` [PATCHv2 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
@ 2015-01-18 14:51 ` Sylvain Rochet
2015-01-18 16:56 ` Boris Brezillon
2015-01-18 17:34 ` [PATCH " Boris Brezillon
1 sibling, 2 replies; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-18 14:51 UTC (permalink / raw)
To: linux-arm-kernel
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 | 95 ++++++++++++++++++++++++---------
drivers/usb/gadget/udc/atmel_usba_udc.h | 4 ++
2 files changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index e207d75..986677b 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
}
#endif
+static int start_clock(struct usba_udc *udc)
+{
+ int ret;
+
+ if (udc->clocked)
+ return 0;
+
+ 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->clocked = true;
+ return ret;
+}
+
+static int stop_clock(struct usba_udc *udc)
+{
+ if (!udc->clocked)
+ return 0;
+
+ clk_disable_unprepare(udc->hclk);
+ clk_disable_unprepare(udc->pclk);
+
+ udc->clocked = false;
+ return 0;
+}
+
static int vbus_is_present(struct usba_udc *udc)
{
if (gpio_is_valid(udc->vbus_pin))
@@ -1719,42 +1751,55 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
return IRQ_HANDLED;
}
-static irqreturn_t usba_vbus_irq(int irq, void *devid)
+static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
{
struct usba_udc *udc = devid;
int vbus;
+ int ret;
/* debounce */
udelay(10);
- spin_lock(&udc->lock);
+ mutex_lock(&udc->vbus_mutex);
/* May happen if Vbus pin toggles during probe() */
- if (!udc->driver)
+ spin_lock(&udc->lock);
+ if (!udc->driver) {
+ spin_unlock(&udc->lock);
goto out;
+ }
+ spin_unlock(&udc->lock);
vbus = vbus_is_present(udc);
if (vbus != udc->vbus_prev) {
if (vbus) {
+ ret = start_clock(udc);
+ if (ret)
+ goto out;
+
+ spin_lock(&udc->lock);
toggle_bias(1);
usba_writel(udc, CTRL, USBA_ENABLE_MASK);
usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+ spin_unlock(&udc->lock);
} else {
+ spin_lock(&udc->lock);
udc->gadget.speed = USB_SPEED_UNKNOWN;
reset_all_endpoints(udc);
toggle_bias(0);
usba_writel(udc, CTRL, USBA_DISABLE_MASK);
- if (udc->driver->disconnect) {
- spin_unlock(&udc->lock);
+ spin_unlock(&udc->lock);
+
+ stop_clock(udc);
+
+ if (udc->driver->disconnect)
udc->driver->disconnect(&udc->gadget);
- spin_lock(&udc->lock);
- }
}
udc->vbus_prev = vbus;
}
out:
- spin_unlock(&udc->lock);
+ mutex_unlock(&udc->vbus_mutex);
return IRQ_HANDLED;
}
@@ -1762,7 +1807,7 @@ out:
static int atmel_usba_start(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)
{
- int ret;
+ int ret = 0;
struct usba_udc *udc = container_of(gadget, struct usba_udc, gadget);
unsigned long flags;
@@ -1772,31 +1817,29 @@ 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;
- }
-
+ mutex_lock(&udc->vbus_mutex);
udc->vbus_prev = 0;
if (gpio_is_valid(udc->vbus_pin))
enable_irq(gpio_to_irq(udc->vbus_pin));
/* 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 = start_clock(udc);
+ if (ret)
+ goto out;
+
+ spin_lock_irqsave(&udc->lock, flags);
toggle_bias(1);
usba_writel(udc, CTRL, USBA_ENABLE_MASK);
usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+ spin_unlock_irqrestore(&udc->lock, flags);
udc->vbus_prev = 1;
}
- spin_unlock_irqrestore(&udc->lock, flags);
- return 0;
+out:
+ mutex_unlock(&udc->vbus_mutex);
+ return ret;
}
static int atmel_usba_stop(struct usb_gadget *gadget)
@@ -1816,8 +1859,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
toggle_bias(0);
usba_writel(udc, CTRL, USBA_DISABLE_MASK);
- clk_disable_unprepare(udc->hclk);
- clk_disable_unprepare(udc->pclk);
+ stop_clock(udc);
udc->driver = NULL;
@@ -1997,6 +2039,7 @@ static int usba_udc_probe(struct platform_device *pdev)
return PTR_ERR(hclk);
spin_lock_init(&udc->lock);
+ mutex_init(&udc->vbus_mutex);
udc->pdev = pdev;
udc->pclk = pclk;
udc->hclk = hclk;
@@ -2049,9 +2092,9 @@ static int usba_udc_probe(struct platform_device *pdev)
if (gpio_is_valid(udc->vbus_pin)) {
if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
- ret = devm_request_irq(&pdev->dev,
- gpio_to_irq(udc->vbus_pin),
- usba_vbus_irq, 0,
+ ret = devm_request_threaded_irq(&pdev->dev,
+ gpio_to_irq(udc->vbus_pin), NULL,
+ usba_vbus_irq_thread, IRQF_NO_SUSPEND | IRQF_ONESHOT,
"atmel_usba_udc", udc);
if (ret) {
udc->vbus_pin = -ENODEV;
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index a70706e..3ceed76 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -308,6 +308,9 @@ struct usba_udc {
/* Protect hw registers from concurrent modifications */
spinlock_t lock;
+ /* Mutex to prevent concurrent start or stop */
+ struct mutex vbus_mutex;
+
void __iomem *regs;
void __iomem *fifo;
@@ -321,6 +324,7 @@ struct usba_udc {
struct clk *pclk;
struct clk *hclk;
struct usba_ep *usba_ep;
+ bool clocked;
u16 devstatus;
--
2.1.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-18 14:51 ` [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
@ 2015-01-18 16:56 ` Boris Brezillon
2015-01-18 17:24 ` [PATCHv3 0/2] " Sylvain Rochet
2015-01-18 17:34 ` [PATCH " Boris Brezillon
1 sibling, 1 reply; 20+ messages in thread
From: Boris Brezillon @ 2015-01-18 16:56 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, 18 Jan 2015 15:51:21 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> 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.
>
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 95 ++++++++++++++++++++++++---------
> drivers/usb/gadget/udc/atmel_usba_udc.h | 4 ++
> 2 files changed, 73 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index e207d75..986677b 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
> }
> #endif
>
> +static int start_clock(struct usba_udc *udc)
> +{
> + int ret;
> +
> + if (udc->clocked)
> + return 0;
> +
> + 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->clocked = true;
> + return ret;
> +}
> +
> +static int stop_clock(struct usba_udc *udc)
> +{
> + if (!udc->clocked)
> + return 0;
> +
> + clk_disable_unprepare(udc->hclk);
> + clk_disable_unprepare(udc->pclk);
> +
> + udc->clocked = false;
> + return 0;
> +}
> +
> static int vbus_is_present(struct usba_udc *udc)
> {
> if (gpio_is_valid(udc->vbus_pin))
> @@ -1719,42 +1751,55 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> return IRQ_HANDLED;
> }
>
> -static irqreturn_t usba_vbus_irq(int irq, void *devid)
> +static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
> {
> struct usba_udc *udc = devid;
> int vbus;
> + int ret;
>
> /* debounce */
> udelay(10);
>
> - spin_lock(&udc->lock);
> + mutex_lock(&udc->vbus_mutex);
>
> /* May happen if Vbus pin toggles during probe() */
> - if (!udc->driver)
> + spin_lock(&udc->lock);
Since this lock is taken in irq context (usba_udc_irq) and this
handler is not called in irq context anymore you should use
spin_lock_irqsave/unlock_irqrestore here.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 0/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-18 16:56 ` Boris Brezillon
@ 2015-01-18 17:24 ` Sylvain Rochet
2015-01-18 17:24 ` [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
2015-01-18 17:24 ` [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
0 siblings, 2 replies; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-18 17:24 UTC (permalink / raw)
To: linux-arm-kernel
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.
Changes since v2:
* Use spin_lock_irqsave/unlock_irqrestore instead of spin_lock/unlock in
threaded interrupt because we are not in irq context anymore
* Removed useless and probably harmful IRQF_NO_SUSPEND from
devm_request_threaded_irq() flags
Changes 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 | 98 ++++++++++++++++++++++++---------
drivers/usb/gadget/udc/atmel_usba_udc.h | 4 ++
2 files changed, 76 insertions(+), 26 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
2015-01-18 17:24 ` [PATCHv3 0/2] " Sylvain Rochet
@ 2015-01-18 17:24 ` Sylvain Rochet
2015-01-19 14:09 ` Nicolas Ferre
2015-01-18 17:24 ` [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
1 sibling, 1 reply; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-18 17:24 UTC (permalink / raw)
To: linux-arm-kernel
If vbus gpio is high at init, we should set vbus_prev to true
accordingly to the current vbus state. Without that, we skip the first
vbus interrupt because the saved vbus state is not consistent.
Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
drivers/usb/gadget/udc/atmel_usba_udc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index ce88237..e207d75 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1791,6 +1791,8 @@ static int atmel_usba_start(struct usb_gadget *gadget,
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);
--
2.1.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-18 17:24 ` [PATCHv3 0/2] " Sylvain Rochet
2015-01-18 17:24 ` [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
@ 2015-01-18 17:24 ` Sylvain Rochet
2015-01-19 16:55 ` Nicolas Ferre
1 sibling, 1 reply; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-18 17:24 UTC (permalink / raw)
To: linux-arm-kernel
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 | 96 ++++++++++++++++++++++++---------
drivers/usb/gadget/udc/atmel_usba_udc.h | 4 ++
2 files changed, 74 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index e207d75..9cce50a 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
}
#endif
+static int start_clock(struct usba_udc *udc)
+{
+ int ret;
+
+ if (udc->clocked)
+ return 0;
+
+ 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->clocked = true;
+ return ret;
+}
+
+static int stop_clock(struct usba_udc *udc)
+{
+ if (!udc->clocked)
+ return 0;
+
+ clk_disable_unprepare(udc->hclk);
+ clk_disable_unprepare(udc->pclk);
+
+ udc->clocked = false;
+ return 0;
+}
+
static int vbus_is_present(struct usba_udc *udc)
{
if (gpio_is_valid(udc->vbus_pin))
@@ -1719,42 +1751,56 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
return IRQ_HANDLED;
}
-static irqreturn_t usba_vbus_irq(int irq, void *devid)
+static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
{
struct usba_udc *udc = devid;
int vbus;
+ int ret;
+ unsigned long flags;
/* debounce */
udelay(10);
- spin_lock(&udc->lock);
+ mutex_lock(&udc->vbus_mutex);
/* May happen if Vbus pin toggles during probe() */
- if (!udc->driver)
+ spin_lock_irqsave(&udc->lock, flags);
+ if (!udc->driver) {
+ spin_unlock_irqrestore(&udc->lock, flags);
goto out;
+ }
+ spin_unlock_irqrestore(&udc->lock, flags);
vbus = vbus_is_present(udc);
if (vbus != udc->vbus_prev) {
if (vbus) {
+ ret = start_clock(udc);
+ if (ret)
+ goto out;
+
+ spin_lock_irqsave(&udc->lock, flags);
toggle_bias(1);
usba_writel(udc, CTRL, USBA_ENABLE_MASK);
usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+ spin_unlock_irqrestore(&udc->lock, flags);
} else {
+ spin_lock_irqsave(&udc->lock, flags);
udc->gadget.speed = USB_SPEED_UNKNOWN;
reset_all_endpoints(udc);
toggle_bias(0);
usba_writel(udc, CTRL, USBA_DISABLE_MASK);
- if (udc->driver->disconnect) {
- spin_unlock(&udc->lock);
+ spin_unlock_irqrestore(&udc->lock, flags);
+
+ stop_clock(udc);
+
+ if (udc->driver->disconnect)
udc->driver->disconnect(&udc->gadget);
- spin_lock(&udc->lock);
- }
}
udc->vbus_prev = vbus;
}
out:
- spin_unlock(&udc->lock);
+ mutex_unlock(&udc->vbus_mutex);
return IRQ_HANDLED;
}
@@ -1762,7 +1808,7 @@ out:
static int atmel_usba_start(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)
{
- int ret;
+ int ret = 0;
struct usba_udc *udc = container_of(gadget, struct usba_udc, gadget);
unsigned long flags;
@@ -1772,31 +1818,29 @@ 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;
- }
-
+ mutex_lock(&udc->vbus_mutex);
udc->vbus_prev = 0;
if (gpio_is_valid(udc->vbus_pin))
enable_irq(gpio_to_irq(udc->vbus_pin));
/* 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 = start_clock(udc);
+ if (ret)
+ goto out;
+
+ spin_lock_irqsave(&udc->lock, flags);
toggle_bias(1);
usba_writel(udc, CTRL, USBA_ENABLE_MASK);
usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+ spin_unlock_irqrestore(&udc->lock, flags);
udc->vbus_prev = 1;
}
- spin_unlock_irqrestore(&udc->lock, flags);
- return 0;
+out:
+ mutex_unlock(&udc->vbus_mutex);
+ return ret;
}
static int atmel_usba_stop(struct usb_gadget *gadget)
@@ -1816,8 +1860,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
toggle_bias(0);
usba_writel(udc, CTRL, USBA_DISABLE_MASK);
- clk_disable_unprepare(udc->hclk);
- clk_disable_unprepare(udc->pclk);
+ stop_clock(udc);
udc->driver = NULL;
@@ -1997,6 +2040,7 @@ static int usba_udc_probe(struct platform_device *pdev)
return PTR_ERR(hclk);
spin_lock_init(&udc->lock);
+ mutex_init(&udc->vbus_mutex);
udc->pdev = pdev;
udc->pclk = pclk;
udc->hclk = hclk;
@@ -2049,9 +2093,9 @@ static int usba_udc_probe(struct platform_device *pdev)
if (gpio_is_valid(udc->vbus_pin)) {
if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
- ret = devm_request_irq(&pdev->dev,
- gpio_to_irq(udc->vbus_pin),
- usba_vbus_irq, 0,
+ ret = devm_request_threaded_irq(&pdev->dev,
+ gpio_to_irq(udc->vbus_pin), NULL,
+ usba_vbus_irq_thread, IRQF_ONESHOT,
"atmel_usba_udc", udc);
if (ret) {
udc->vbus_pin = -ENODEV;
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index a70706e..3ceed76 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -308,6 +308,9 @@ struct usba_udc {
/* Protect hw registers from concurrent modifications */
spinlock_t lock;
+ /* Mutex to prevent concurrent start or stop */
+ struct mutex vbus_mutex;
+
void __iomem *regs;
void __iomem *fifo;
@@ -321,6 +324,7 @@ struct usba_udc {
struct clk *pclk;
struct clk *hclk;
struct usba_ep *usba_ep;
+ bool clocked;
u16 devstatus;
--
2.1.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-18 14:51 ` [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
2015-01-18 16:56 ` Boris Brezillon
@ 2015-01-18 17:34 ` Boris Brezillon
1 sibling, 0 replies; 20+ messages in thread
From: Boris Brezillon @ 2015-01-18 17:34 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, 18 Jan 2015 15:51:21 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> 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.
>
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 95 ++++++++++++++++++++++++---------
> drivers/usb/gadget/udc/atmel_usba_udc.h | 4 ++
> 2 files changed, 73 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index e207d75..986677b 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
> }
> #endif
>
[...]
>
> if (gpio_is_valid(udc->vbus_pin)) {
> if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
> - ret = devm_request_irq(&pdev->dev,
> - gpio_to_irq(udc->vbus_pin),
> - usba_vbus_irq, 0,
> + ret = devm_request_threaded_irq(&pdev->dev,
> + gpio_to_irq(udc->vbus_pin), NULL,
> + usba_vbus_irq_thread, IRQF_NO_SUSPEND | IRQF_ONESHOT,
You should drop the IRQF_NO_SUSPEND flag. If you want to wakeup on VBUS
change you should use enable_irq_wake instead.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
2015-01-18 17:24 ` [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
@ 2015-01-19 14:09 ` Nicolas Ferre
2015-01-19 18:55 ` Felipe Balbi
0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Ferre @ 2015-01-19 14:09 UTC (permalink / raw)
To: linux-arm-kernel
Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
> If vbus gpio is high at init, we should set vbus_prev to true
> accordingly to the current vbus state. Without that, we skip the first
> vbus interrupt because the saved vbus state is not consistent.
>
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
Indeed:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
We can also add the following tags:
Fixes: 914a3f3b3754 ("USB: add atmel_usba_udc driver")
Cc: <stable@vger.kernel.org> # 2.6.x-ish
Bye,
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index ce88237..e207d75 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1791,6 +1791,8 @@ static int atmel_usba_start(struct usb_gadget *gadget,
> 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);
>
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-18 17:24 ` [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
@ 2015-01-19 16:55 ` Nicolas Ferre
2015-01-19 17:46 ` Sylvain Rochet
0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Ferre @ 2015-01-19 16:55 UTC (permalink / raw)
To: linux-arm-kernel
Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
> Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce
Please re-write which "edge" we are talking about: "... falling edge of
the Vbus signal" for example.
> power consumption if USB PLL is not already necessary for OHCI or EHCI.
Is a verb missing in the previous sentence?
> 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.
Can you please make it more clear. Several separated sentences seem
necessary here.
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 96 ++++++++++++++++++++++++---------
> drivers/usb/gadget/udc/atmel_usba_udc.h | 4 ++
> 2 files changed, 74 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index e207d75..9cce50a 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
> }
> #endif
>
> +static int start_clock(struct usba_udc *udc)
> +{
> + int ret;
> +
> + if (udc->clocked)
> + return 0;
> +
> + 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->clocked = true;
> + return ret;
> +}
> +
> +static int stop_clock(struct usba_udc *udc)
> +{
> + if (!udc->clocked)
> + return 0;
> +
> + clk_disable_unprepare(udc->hclk);
> + clk_disable_unprepare(udc->pclk);
> +
> + udc->clocked = false;
> + return 0;
> +}
> +
> static int vbus_is_present(struct usba_udc *udc)
> {
> if (gpio_is_valid(udc->vbus_pin))
> @@ -1719,42 +1751,56 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> return IRQ_HANDLED;
> }
>
> -static irqreturn_t usba_vbus_irq(int irq, void *devid)
> +static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
> {
> struct usba_udc *udc = devid;
> int vbus;
> + int ret;
> + unsigned long flags;
>
> /* debounce */
> udelay(10);
>
> - spin_lock(&udc->lock);
> + mutex_lock(&udc->vbus_mutex);
>
> /* May happen if Vbus pin toggles during probe() */
> - if (!udc->driver)
> + spin_lock_irqsave(&udc->lock, flags);
> + if (!udc->driver) {
> + spin_unlock_irqrestore(&udc->lock, flags);
> goto out;
> + }
> + spin_unlock_irqrestore(&udc->lock, flags);
I'm not sure that the protection by spin_lock is needed above.
>
> vbus = vbus_is_present(udc);
> if (vbus != udc->vbus_prev) {
> if (vbus) {
> + ret = start_clock(udc);
> + if (ret)
> + goto out;
> +
> + spin_lock_irqsave(&udc->lock, flags);
> toggle_bias(1);
> usba_writel(udc, CTRL, USBA_ENABLE_MASK);
> usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
> + spin_unlock_irqrestore(&udc->lock, flags);
> } else {
> + spin_lock_irqsave(&udc->lock, flags);
> udc->gadget.speed = USB_SPEED_UNKNOWN;
> reset_all_endpoints(udc);
> toggle_bias(0);
> usba_writel(udc, CTRL, USBA_DISABLE_MASK);
> - if (udc->driver->disconnect) {
> - spin_unlock(&udc->lock);
> + spin_unlock_irqrestore(&udc->lock, flags);
> +
> + stop_clock(udc);
> +
> + if (udc->driver->disconnect)
> udc->driver->disconnect(&udc->gadget);
> - spin_lock(&udc->lock);
> - }
> }
> udc->vbus_prev = vbus;
> }
>
> out:
> - spin_unlock(&udc->lock);
> + mutex_unlock(&udc->vbus_mutex);
>
> return IRQ_HANDLED;
> }
> @@ -1762,7 +1808,7 @@ out:
> static int atmel_usba_start(struct usb_gadget *gadget,
> struct usb_gadget_driver *driver)
> {
> - int ret;
> + int ret = 0;
> struct usba_udc *udc = container_of(gadget, struct usba_udc, gadget);
> unsigned long flags;
>
> @@ -1772,31 +1818,29 @@ 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;
> - }
> -
> + mutex_lock(&udc->vbus_mutex);
> udc->vbus_prev = 0;
> if (gpio_is_valid(udc->vbus_pin))
> enable_irq(gpio_to_irq(udc->vbus_pin));
>
> /* 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 = start_clock(udc);
> + if (ret)
> + goto out;
> +
> + spin_lock_irqsave(&udc->lock, flags);
> toggle_bias(1);
> usba_writel(udc, CTRL, USBA_ENABLE_MASK);
> usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
> + spin_unlock_irqrestore(&udc->lock, flags);
>
> udc->vbus_prev = 1;
> }
> - spin_unlock_irqrestore(&udc->lock, flags);
>
> - return 0;
> +out:
> + mutex_unlock(&udc->vbus_mutex);
> + return ret;
> }
>
> static int atmel_usba_stop(struct usb_gadget *gadget)
> @@ -1816,8 +1860,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
> toggle_bias(0);
> usba_writel(udc, CTRL, USBA_DISABLE_MASK);
>
> - clk_disable_unprepare(udc->hclk);
> - clk_disable_unprepare(udc->pclk);
> + stop_clock(udc);
>
> udc->driver = NULL;
>
> @@ -1997,6 +2040,7 @@ static int usba_udc_probe(struct platform_device *pdev)
> return PTR_ERR(hclk);
>
> spin_lock_init(&udc->lock);
> + mutex_init(&udc->vbus_mutex);
> udc->pdev = pdev;
> udc->pclk = pclk;
> udc->hclk = hclk;
> @@ -2049,9 +2093,9 @@ static int usba_udc_probe(struct platform_device *pdev)
>
> if (gpio_is_valid(udc->vbus_pin)) {
> if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
> - ret = devm_request_irq(&pdev->dev,
> - gpio_to_irq(udc->vbus_pin),
> - usba_vbus_irq, 0,
> + ret = devm_request_threaded_irq(&pdev->dev,
> + gpio_to_irq(udc->vbus_pin), NULL,
> + usba_vbus_irq_thread, IRQF_ONESHOT,
> "atmel_usba_udc", udc);
> if (ret) {
> udc->vbus_pin = -ENODEV;
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
> index a70706e..3ceed76 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
> @@ -308,6 +308,9 @@ struct usba_udc {
> /* Protect hw registers from concurrent modifications */
> spinlock_t lock;
>
> + /* Mutex to prevent concurrent start or stop */
> + struct mutex vbus_mutex;
> +
> void __iomem *regs;
> void __iomem *fifo;
>
> @@ -321,6 +324,7 @@ struct usba_udc {
> struct clk *pclk;
> struct clk *hclk;
> struct usba_ep *usba_ep;
> + bool clocked;
>
> u16 devstatus;
Otherwise, it looks okay, so once little corrections done, you can add my:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Thanks, bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-19 16:55 ` Nicolas Ferre
@ 2015-01-19 17:46 ` Sylvain Rochet
2015-01-19 20:15 ` Boris Brezillon
0 siblings, 1 reply; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-19 17:46 UTC (permalink / raw)
To: linux-arm-kernel
Hello Nicolas,
On Mon, Jan 19, 2015 at 05:55:18PM +0100, Nicolas Ferre wrote:
> Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
> > Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce
>
> Please re-write which "edge" we are talking about: "... falling edge of
> the Vbus signal" for example.
>
> > power consumption if USB PLL is not already necessary for OHCI or EHCI.
>
> Is a verb missing in the previous sentence?
>
> > 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.
>
> Can you please make it more clear. Several separated sentences seem
> necessary here.
Maybe :)
Proposal:
Start clocks on rising edge of the Vbus signal, stop clocks on
falling edge of the Vbus signal.
If USB PLL is not necessary for other USB drivers (e.g. OHCI and EHCI)
it will reduce power consumption by switching off the USB PLL if no USB
Host is currently connected to this USB Device.
We are using Vbus GPIO signal to detect Host presence. If Vbus signal is
not available then the device stay continuously clocked.
Note this driver does not support suspend/resume yet, it may stay
clocked if USB Host is still connected when suspending. For what I need,
forbidding suspend from userland if we are still attached to an USB host
is fine, but we might as well add suspend/resume to this driver in the
future.
> > /* May happen if Vbus pin toggles during probe() */
> > - if (!udc->driver)
> > + spin_lock_irqsave(&udc->lock, flags);
> > + if (!udc->driver) {
> > + spin_unlock_irqrestore(&udc->lock, flags);
> > goto out;
> > + }
> > + spin_unlock_irqrestore(&udc->lock, flags);
>
> I'm not sure that the protection by spin_lock is needed above.
I'm not sure too, it was already in a spinlock area, I obviously kept it
because it was not the purpose of this patch.
This seem to be in mirror of atmel_usba_start() which does:
spin_lock_irqsave(&udc->lock, flags);
udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
udc->driver = driver;
spin_unlock_irqrestore(&udc->lock, flags);
? but vbus_pin IRQ is not yet enabled.
Same for atmel_usba_stop() which disable vbus_pin IRQ before setting
udc->driver to NULL, but without spinlock this time (why?, this should
be consistent IMHO).
I don't know if it is guaranteed that IRQ does not fire nor continue to
run after disable_irq() returns, especially for threaded IRQ.
If the following sequence might happen:
atmel_usba_stop()
disable_irq(vbus)
usba_vbus_irq_thread() called lately
check for (!udc->driver) and continue
udc->driver = NULL;
if (udc->driver->disconnect)
*CRASH*
Then the patch should be modified to protect udc->driver with the vbus
mutex.
In this case the previous implementation wasn't perfect too, the
atmel_usba_stop() does not lock around the NULLification of driver,
Moreover the spinlock is (and was) unlocked in VBUS interrupt just
before calling udc->driver->disconnect, which makes udc->driver actually
not locked anywere.
If the previous sequence possible ? If no, udc->driver does not need
locking, if yes, it is currently not locked enough.
Sylvain
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
2015-01-19 14:09 ` Nicolas Ferre
@ 2015-01-19 18:55 ` Felipe Balbi
2015-01-20 11:02 ` Sylvain Rochet
0 siblings, 1 reply; 20+ messages in thread
From: Felipe Balbi @ 2015-01-19 18:55 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 19, 2015 at 03:09:44PM +0100, Nicolas Ferre wrote:
> Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
> > If vbus gpio is high at init, we should set vbus_prev to true
> > accordingly to the current vbus state. Without that, we skip the first
> > vbus interrupt because the saved vbus state is not consistent.
> >
> > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
>
> Indeed:
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> We can also add the following tags:
> Fixes: 914a3f3b3754 ("USB: add atmel_usba_udc driver")
> Cc: <stable@vger.kernel.org> # 2.6.x-ish
Please resend with the proper stable format. Also, this 2.6.x-ish should
be 2.6.24+, git describe helps a lot figuring these out.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150119/d7afe913/attachment-0001.sig>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-19 17:46 ` Sylvain Rochet
@ 2015-01-19 20:15 ` Boris Brezillon
2015-01-20 9:02 ` Nicolas Ferre
0 siblings, 1 reply; 20+ messages in thread
From: Boris Brezillon @ 2015-01-19 20:15 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 19 Jan 2015 18:46:31 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> Hello Nicolas,
>
>
> On Mon, Jan 19, 2015 at 05:55:18PM +0100, Nicolas Ferre wrote:
> > Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
> > > Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce
> >
> > Please re-write which "edge" we are talking about: "... falling edge of
> > the Vbus signal" for example.
> >
> > > power consumption if USB PLL is not already necessary for OHCI or EHCI.
> >
> > Is a verb missing in the previous sentence?
> >
> > > 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.
> >
> > Can you please make it more clear. Several separated sentences seem
> > necessary here.
>
> Maybe :)
>
>
> Proposal:
>
> Start clocks on rising edge of the Vbus signal, stop clocks on
> falling edge of the Vbus signal.
>
> If USB PLL is not necessary for other USB drivers (e.g. OHCI and EHCI)
> it will reduce power consumption by switching off the USB PLL if no USB
> Host is currently connected to this USB Device.
>
> We are using Vbus GPIO signal to detect Host presence. If Vbus signal is
> not available then the device stay continuously clocked.
>
> Note this driver does not support suspend/resume yet, it may stay
> clocked if USB Host is still connected when suspending. For what I need,
> forbidding suspend from userland if we are still attached to an USB host
> is fine, but we might as well add suspend/resume to this driver in the
> future.
>
>
>
> > > /* May happen if Vbus pin toggles during probe() */
> > > - if (!udc->driver)
> > > + spin_lock_irqsave(&udc->lock, flags);
> > > + if (!udc->driver) {
> > > + spin_unlock_irqrestore(&udc->lock, flags);
> > > goto out;
> > > + }
> > > + spin_unlock_irqrestore(&udc->lock, flags);
> >
> > I'm not sure that the protection by spin_lock is needed above.
>
> I'm not sure too, it was already in a spinlock area, I obviously kept it
> because it was not the purpose of this patch.
According to the comment placed above this test it's here to prevent
any action triggered by the vbus pin irq while the driver is not
properly probed yet.
You could use:
set_irq_flags(vbus_irq, IRQF_NOAUTOEN);
before calling devm_request_threaded_irq.
This will keep the irq disabled instead of enabling it at request time.
Moreover, this simplify the vbus_irq request code (which disables the
irq just after requesting it).
>
> This seem to be in mirror of atmel_usba_start() which does:
>
> spin_lock_irqsave(&udc->lock, flags);
> udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
> udc->driver = driver;
> spin_unlock_irqrestore(&udc->lock, flags);
>
> ? but vbus_pin IRQ is not yet enabled.
>
>
> Same for atmel_usba_stop() which disable vbus_pin IRQ before setting
> udc->driver to NULL, but without spinlock this time (why?, this should
> be consistent IMHO).
>
>
> I don't know if it is guaranteed that IRQ does not fire nor continue to
> run after disable_irq() returns, especially for threaded IRQ.
And yes, disable_irq waits for all irq handler termination, including
threaded irqs: see [1], [2].
>
>
> If the following sequence might happen:
> atmel_usba_stop()
> disable_irq(vbus)
> usba_vbus_irq_thread() called lately
> check for (!udc->driver) and continue
> udc->driver = NULL;
> if (udc->driver->disconnect)
> *CRASH*
>
> Then the patch should be modified to protect udc->driver with the vbus
> mutex.
>
> In this case the previous implementation wasn't perfect too, the
> atmel_usba_stop() does not lock around the NULLification of driver,
>
> Moreover the spinlock is (and was) unlocked in VBUS interrupt just
> before calling udc->driver->disconnect, which makes udc->driver actually
> not locked anywere.
>
> If the previous sequence possible ? If no, udc->driver does not need
> locking, if yes, it is currently not locked enough.
If you rework the vbus_irq request as I suggested I think you can get
rid of this !udc->driver test, and thus drop the locking around it ;-).
Best Regards,
Boris
[1]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L432
[2]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L92
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
2015-01-19 20:15 ` Boris Brezillon
@ 2015-01-20 9:02 ` Nicolas Ferre
0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2015-01-20 9:02 UTC (permalink / raw)
To: linux-arm-kernel
Le 19/01/2015 21:15, Boris Brezillon a ?crit :
> On Mon, 19 Jan 2015 18:46:31 +0100
> Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
>
>> Hello Nicolas,
>>
>>
>> On Mon, Jan 19, 2015 at 05:55:18PM +0100, Nicolas Ferre wrote:
>>> Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
>>>> Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce
>>>
>>> Please re-write which "edge" we are talking about: "... falling edge of
>>> the Vbus signal" for example.
>>>
>>>> power consumption if USB PLL is not already necessary for OHCI or EHCI.
>>>
>>> Is a verb missing in the previous sentence?
>>>
>>>> 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.
>>>
>>> Can you please make it more clear. Several separated sentences seem
>>> necessary here.
>>
>> Maybe :)
>>
>>
>> Proposal:
>>
>> Start clocks on rising edge of the Vbus signal, stop clocks on
>> falling edge of the Vbus signal.
>>
>> If USB PLL is not necessary for other USB drivers (e.g. OHCI and EHCI)
>> it will reduce power consumption by switching off the USB PLL if no USB
>> Host is currently connected to this USB Device.
>>
>> We are using Vbus GPIO signal to detect Host presence. If Vbus signal is
>> not available then the device stay continuously clocked.
Typo: s/stay/stays/
>> Note this driver does not support suspend/resume yet, it may stay
>> clocked if USB Host is still connected when suspending. For what I need,
>> forbidding suspend from userland if we are still attached to an USB host
>> is fine, but we might as well add suspend/resume to this driver in the
>> future.
Sylvain, thanks for having rephrased this part! It looks clear.
>>>> /* May happen if Vbus pin toggles during probe() */
>>>> - if (!udc->driver)
>>>> + spin_lock_irqsave(&udc->lock, flags);
>>>> + if (!udc->driver) {
>>>> + spin_unlock_irqrestore(&udc->lock, flags);
>>>> goto out;
>>>> + }
>>>> + spin_unlock_irqrestore(&udc->lock, flags);
>>>
>>> I'm not sure that the protection by spin_lock is needed above.
>>
>> I'm not sure too, it was already in a spinlock area, I obviously kept it
>> because it was not the purpose of this patch.
>
> According to the comment placed above this test it's here to prevent
> any action triggered by the vbus pin irq while the driver is not
> properly probed yet.
> You could use:
>
> set_irq_flags(vbus_irq, IRQF_NOAUTOEN);
>
> before calling devm_request_threaded_irq.
> This will keep the irq disabled instead of enabling it at request time.
> Moreover, this simplify the vbus_irq request code (which disables the
> irq just after requesting it).
>
>>
>> This seem to be in mirror of atmel_usba_start() which does:
>>
>> spin_lock_irqsave(&udc->lock, flags);
>> udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
>> udc->driver = driver;
>> spin_unlock_irqrestore(&udc->lock, flags);
>>
>> ? but vbus_pin IRQ is not yet enabled.
>>
>>
>> Same for atmel_usba_stop() which disable vbus_pin IRQ before setting
>> udc->driver to NULL, but without spinlock this time (why?, this should
>> be consistent IMHO).
>>
>>
>> I don't know if it is guaranteed that IRQ does not fire nor continue to
>> run after disable_irq() returns, especially for threaded IRQ.
>
> And yes, disable_irq waits for all irq handler termination, including
> threaded irqs: see [1], [2].
>
>>
>>
>> If the following sequence might happen:
>> atmel_usba_stop()
>> disable_irq(vbus)
>> usba_vbus_irq_thread() called lately
>> check for (!udc->driver) and continue
>> udc->driver = NULL;
>> if (udc->driver->disconnect)
>> *CRASH*
>>
>> Then the patch should be modified to protect udc->driver with the vbus
>> mutex.
>>
>> In this case the previous implementation wasn't perfect too, the
>> atmel_usba_stop() does not lock around the NULLification of driver,
>>
>> Moreover the spinlock is (and was) unlocked in VBUS interrupt just
>> before calling udc->driver->disconnect, which makes udc->driver actually
>> not locked anywere.
>>
>> If the previous sequence possible ? If no, udc->driver does not need
>> locking, if yes, it is currently not locked enough.
>
> If you rework the vbus_irq request as I suggested I think you can get
> rid of this !udc->driver test, and thus drop the locking around it ;-).
>
> Best Regards,
>
> Boris
>
> [1]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L432
> [2]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L92
Thanks Sylvain for having described the problem lengthly and Boris for
your detailed explanation and suggestion concerning this sequence.
So, if you can re-send a new version and also add the stable tags as
suggested by Felipe, it would be really cool.
Bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
2015-01-19 18:55 ` Felipe Balbi
@ 2015-01-20 11:02 ` Sylvain Rochet
2015-01-20 11:11 ` Nicolas Ferre
0 siblings, 1 reply; 20+ messages in thread
From: Sylvain Rochet @ 2015-01-20 11:02 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Mon, Jan 19, 2015 at 12:55:47PM -0600, Felipe Balbi wrote:
> On Mon, Jan 19, 2015 at 03:09:44PM +0100, Nicolas Ferre wrote:
> > Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
> > > If vbus gpio is high at init, we should set vbus_prev to true
> > > accordingly to the current vbus state. Without that, we skip the first
> > > vbus interrupt because the saved vbus state is not consistent.
> > >
> > > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> >
> > Indeed:
> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> >
> > We can also add the following tags:
> > Fixes: 914a3f3b3754 ("USB: add atmel_usba_udc driver")
> > Cc: <stable@vger.kernel.org> # 2.6.x-ish
>
> Please resend with the proper stable format. Also, this 2.6.x-ish should
> be 2.6.24+, git describe helps a lot figuring these out.
Unfortunately I failed to understand what I should do. The "Also"
keyword means for me that there are two differents things to fix:
1. proper stable format
2. 2.6.x-ish ? 2.6.24+
2. does not imply fixing 1., isn'it ?
I read SubmittingPatches thoroughly and I can't find a reference to
stable format.
Sylvain
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
2015-01-20 11:02 ` Sylvain Rochet
@ 2015-01-20 11:11 ` Nicolas Ferre
0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2015-01-20 11:11 UTC (permalink / raw)
To: linux-arm-kernel
Le 20/01/2015 12:02, Sylvain Rochet a ?crit :
> Hello,
>
> On Mon, Jan 19, 2015 at 12:55:47PM -0600, Felipe Balbi wrote:
>> On Mon, Jan 19, 2015 at 03:09:44PM +0100, Nicolas Ferre wrote:
>>> Le 18/01/2015 18:24, Sylvain Rochet a ?crit :
>>>> If vbus gpio is high at init, we should set vbus_prev to true
>>>> accordingly to the current vbus state. Without that, we skip the first
>>>> vbus interrupt because the saved vbus state is not consistent.
>>>>
>>>> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
>>>
>>> Indeed:
>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>
>>> We can also add the following tags:
>>> Fixes: 914a3f3b3754 ("USB: add atmel_usba_udc driver")
>>> Cc: <stable@vger.kernel.org> # 2.6.x-ish
>>
>> Please resend with the proper stable format. Also, this 2.6.x-ish should
>> be 2.6.24+, git describe helps a lot figuring these out.
>
> Unfortunately I failed to understand what I should do. The "Also"
> keyword means for me that there are two differents things to fix:
>
> 1. proper stable format
> 2. 2.6.x-ish ? 2.6.24+
>
> 2. does not imply fixing 1., isn'it ?
Well, I think it does. My understanding is that fixing '2' is the solution.
> I read SubmittingPatches thoroughly and I can't find a reference to
> stable format.
>
> Sylvain
Bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2015-01-20 11:11 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-16 22:21 [PATCH] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
2015-01-17 1:42 ` Alexandre Belloni
2015-01-17 9:43 ` Boris Brezillon
2015-01-17 11:07 ` Sylvain Rochet
2015-01-18 14:51 ` [PATCHv2 0/2] " Sylvain Rochet
2015-01-18 14:51 ` [PATCHv2 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
2015-01-18 14:51 ` [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
2015-01-18 16:56 ` Boris Brezillon
2015-01-18 17:24 ` [PATCHv3 0/2] " Sylvain Rochet
2015-01-18 17:24 ` [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
2015-01-19 14:09 ` Nicolas Ferre
2015-01-19 18:55 ` Felipe Balbi
2015-01-20 11:02 ` Sylvain Rochet
2015-01-20 11:11 ` Nicolas Ferre
2015-01-18 17:24 ` [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
2015-01-19 16:55 ` Nicolas Ferre
2015-01-19 17:46 ` Sylvain Rochet
2015-01-19 20:15 ` Boris Brezillon
2015-01-20 9:02 ` Nicolas Ferre
2015-01-18 17:34 ` [PATCH " Boris Brezillon
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.