[PATCHv6 0/5] USB: gadget: atmel_usba_udc: Driver improvements

[PATCHv6 0/5] USB: gadget: atmel_usba_udc: Driver improvements

[Sylvain Rochet]

Start clocks on rising edge of the Vbus signal, stop clocks on falling
edge of the Vbus signal.

Add suspend/resume with wakeup support.

This series is based on linux-next and depends on this complete series:
  https://lkml.org/lkml/2015/1/12/209
which is currently only partially pushed to linux-next, the following 
patch should be pushed along with this series:
  https://lkml.org/lkml/2015/1/12/212

Changes since v5:
  * Some boards does not support configuring a GPIO interrupt on a specific
    edge (rising only or falling only). As suggested added a config boolean
    to distinguish between boards with and without support
  * Added a missing mutex_lock()/unlock() in usba_udc_suspend(), we need to
    protect usba_stop() because usba_start() and usba_stop() can still 
    be called by Vbus IRQ handler.
  * Rebased the full series on linux-next
  * Patch cleaning with the help of checkpatch.pl

Changes since v4:
  * Now using IRQ_NOAUTOEN flag to remove the unused check for
    udc->driver is not NULL in the Vbus IRQ.
  * Reworked the start/stop of clocks on Vbus edges to prepare for
    suspend/resume support, factorised start and stop procedures
  * New patch, suspend/resume for USBA with wakeup support

Changes since v3:
  * Added stable tag for the first patch
  * As suggested, removed the unused check for udc->driver is NULL in
    Vbus IRQ by requesting IRQ after udc->driver is set and by releasing
    IRQ before udc->driver is cleared
  * Rebased the core patch of this series against the just explained changes

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 (5):
  USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
  USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ
    instead of an auto enabled IRQ request followed by IRQ disable
  USB: gadget: atmel_usba_udc: Start clocks on rising edge of the Vbus
    signal, stop clocks on falling edge of the Vbus signal
  USB: gadget: atmel_usba_udc: Prepare for IRQ single edge support
  USB: gadget: atmel_usba_udc: Add suspend/resume with wakeup support

 drivers/usb/gadget/udc/atmel_usba_udc.c | 244 +++++++++++++++++++++++---------
 drivers/usb/gadget/udc/atmel_usba_udc.h |   9 +-
 2 files changed, 188 insertions(+), 65 deletions(-)

-- 
2.1.4

[PATCHv6 1/5] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state

[Sylvain Rochet]

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>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.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 4b5b7fa..40e5fc7 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1807,6 +1807,8 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 		toggle_bias(udc, 1);
 		usba_writel(udc, CTRL, USBA_ENABLE_MASK);
 		usba_int_enb_set(udc, USBA_END_OF_RESET);
+
+		udc->vbus_prev = 1;
 	}
 	spin_unlock_irqrestore(&udc->lock, flags);
 
-- 
2.1.4

[PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable

[Sylvain Rochet]

Vbus IRQ handler needs a started UDC driver to work because it uses
udc->driver, which is set by the UDC start handler. The previous way
chosen was to return from interrupt if udc->driver is NULL using a
spinlock around the check.

We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
of an auto enabled IRQ followed by disable_irq(). This way we remove the
very small timeslot of enabled IRQ which existed previously between
request() and disable(). We don't need anymore to check if udc->driver
is NULL in IRQ handler.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 40e5fc7..f9f305f 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
 
 	spin_lock(&udc->lock);
 
-	/* May happen if Vbus pin toggles during probe() */
-	if (!udc->driver)
-		goto out;
-
 	vbus = vbus_is_present(udc);
 	if (vbus != udc->vbus_prev) {
 		if (vbus) {
@@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
 		udc->vbus_prev = vbus;
 	}
 
-out:
 	spin_unlock(&udc->lock);
 
 	return IRQ_HANDLED;
@@ -2109,6 +2104,8 @@ 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")) {
+			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
+					IRQ_NOAUTOEN);
 			ret = devm_request_irq(&pdev->dev,
 					gpio_to_irq(udc->vbus_pin),
 					usba_vbus_irq, 0,
@@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev)
 				dev_warn(&udc->pdev->dev,
 					 "failed to request vbus irq; "
 					 "assuming always on\n");
-			} else {
-				disable_irq(gpio_to_irq(udc->vbus_pin));
 			}
 		} else {
 			/* gpio_request fail so use -EINVAL for gpio_is_valid */
-- 
2.1.4

[PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable

[Jean-Christophe PLAGNIOL-VILLARD]

> On Jan 23, 2015, at 12:56 AM, Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> 
> Vbus IRQ handler needs a started UDC driver to work because it uses
> udc->driver, which is set by the UDC start handler. The previous way
> chosen was to return from interrupt if udc->driver is NULL using a
> spinlock around the check.
> 
> We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
> of an auto enabled IRQ followed by disable_irq(). This way we remove the
> very small timeslot of enabled IRQ which existed previously between
> request() and disable(). We don't need anymore to check if udc->driver
> is NULL in IRQ handler.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 40e5fc7..f9f305f 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
> 
> 	spin_lock(&udc->lock);
> 
> -	/* May happen if Vbus pin toggles during probe() */
> -	if (!udc->driver)
> -		goto out;
> -
> 	vbus = vbus_is_present(udc);
> 	if (vbus != udc->vbus_prev) {
> 		if (vbus) {
> @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
> 		udc->vbus_prev = vbus;
> 	}
> 
> -out:
> 	spin_unlock(&udc->lock);
> 
> 	return IRQ_HANDLED;
> @@ -2109,6 +2104,8 @@ 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")) {
> +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
> +					IRQ_NOAUTOEN);

not happy about still using the broken gpio_to_irq API

Linus can pass the IRQ via board file?

and via DT we can use IRQ directly

Best Regards,
J.
> 			ret = devm_request_irq(&pdev->dev,
> 					gpio_to_irq(udc->vbus_pin),
> 					usba_vbus_irq, 0,
> @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev)
> 				dev_warn(&udc->pdev->dev,
> 					 "failed to request vbus irq; "
> 					 "assuming always on\n");
> -			} else {
> -				disable_irq(gpio_to_irq(udc->vbus_pin));
> 			}
> 		} else {
> 			/* gpio_request fail so use -EINVAL for gpio_is_valid */
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable

[Nicolas Ferre]

Le 23/01/2015 08:43, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> 
>> On Jan 23, 2015, at 12:56 AM, Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
>>
>> Vbus IRQ handler needs a started UDC driver to work because it uses
>> udc->driver, which is set by the UDC start handler. The previous way
>> chosen was to return from interrupt if udc->driver is NULL using a
>> spinlock around the check.
>>
>> We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
>> of an auto enabled IRQ followed by disable_irq(). This way we remove the
>> very small timeslot of enabled IRQ which existed previously between
>> request() and disable(). We don't need anymore to check if udc->driver
>> is NULL in IRQ handler.
>>
>> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
>> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> ---
>> drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index 40e5fc7..f9f305f 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>>
>> 	spin_lock(&udc->lock);
>>
>> -	/* May happen if Vbus pin toggles during probe() */
>> -	if (!udc->driver)
>> -		goto out;
>> -
>> 	vbus = vbus_is_present(udc);
>> 	if (vbus != udc->vbus_prev) {
>> 		if (vbus) {
>> @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>> 		udc->vbus_prev = vbus;
>> 	}
>>
>> -out:
>> 	spin_unlock(&udc->lock);
>>
>> 	return IRQ_HANDLED;
>> @@ -2109,6 +2104,8 @@ 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")) {
>> +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
>> +					IRQ_NOAUTOEN);
> 
> not happy about still using the broken gpio_to_irq API
> 
> Linus can pass the IRQ via board file?
> 
> and via DT we can use IRQ directly

Absolutely not the topic of this patch series.
Maybe the subject of another patch later?

Best regards,


>> 			ret = devm_request_irq(&pdev->dev,
>> 					gpio_to_irq(udc->vbus_pin),
>> 					usba_vbus_irq, 0,
>> @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev)
>> 				dev_warn(&udc->pdev->dev,
>> 					 "failed to request vbus irq; "
>> 					 "assuming always on\n");
>> -			} else {
>> -				disable_irq(gpio_to_irq(udc->vbus_pin));
>> 			}
>> 		} else {
>> 			/* gpio_request fail so use -EINVAL for gpio_is_valid */
>> -- 
>> 2.1.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 


-- 
Nicolas Ferre

[PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable

[Sylvain Rochet]

Hello,

On Fri, Jan 23, 2015 at 09:59:58AM +0100, Nicolas Ferre wrote:
> Le 23/01/2015 08:43, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> > > On Jan 23, 2015, at 12:56 AM, Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> > > +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
> > > +					IRQ_NOAUTOEN);
> > 
> > not happy about still using the broken gpio_to_irq API
> > 
> > Linus can pass the IRQ via board file?
> > 
> > and via DT we can use IRQ directly

Although I agree on the general principle, this driver is also used by 
non-DT boards.

I have concerns at least for at32ap700x-based boards which use this 
driver and are non-DT. The suggested change requires changing platform 
data of at32ap700x in such a way we may break it in my opinion.


> Absolutely not the topic of this patch series.
> Maybe the subject of another patch later?

Indeed. This series is only about PM support, harmless rework, or 
necessary rework for PM. Changing the way we acquire the IRQ is not 
harmless, this should be checked carefully for DT and particularly 
non-DT boards.


Sylvain

[PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable

[Nicolas Ferre]

Le 22/01/2015 17:56, Sylvain Rochet a ?crit :
> Vbus IRQ handler needs a started UDC driver to work because it uses
> udc->driver, which is set by the UDC start handler. The previous way
> chosen was to return from interrupt if udc->driver is NULL using a
> spinlock around the check.
> 
> We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
> of an auto enabled IRQ followed by disable_irq(). This way we remove the
> very small timeslot of enabled IRQ which existed previously between
> request() and disable(). We don't need anymore to check if udc->driver
> is NULL in IRQ handler.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 40e5fc7..f9f305f 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>  
>  	spin_lock(&udc->lock);
>  
> -	/* May happen if Vbus pin toggles during probe() */
> -	if (!udc->driver)
> -		goto out;
> -
>  	vbus = vbus_is_present(udc);
>  	if (vbus != udc->vbus_prev) {
>  		if (vbus) {
> @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>  		udc->vbus_prev = vbus;
>  	}
>  
> -out:
>  	spin_unlock(&udc->lock);
>  
>  	return IRQ_HANDLED;
> @@ -2109,6 +2104,8 @@ 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")) {
> +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
> +					IRQ_NOAUTOEN);
>  			ret = devm_request_irq(&pdev->dev,
>  					gpio_to_irq(udc->vbus_pin),
>  					usba_vbus_irq, 0,
> @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev)
>  				dev_warn(&udc->pdev->dev,
>  					 "failed to request vbus irq; "
>  					 "assuming always on\n");
> -			} else {
> -				disable_irq(gpio_to_irq(udc->vbus_pin));
>  			}
>  		} else {
>  			/* gpio_request fail so use -EINVAL for gpio_is_valid */
> 


-- 
Nicolas Ferre

[PATCHv6 3/5] USB: gadget: atmel_usba_udc: Start clocks on rising edge of the Vbus signal, stop clocks on falling edge of the Vbus signal

[Sylvain Rochet]

If USB PLL is not necessary for other USB drivers (e.g. OHCI and EHCI)
we 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 stays continuously clocked.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 144 +++++++++++++++++++++-----------
 drivers/usb/gadget/udc/atmel_usba_udc.h |   4 +
 2 files changed, 100 insertions(+), 48 deletions(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index f9f305f..d554106 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1735,7 +1735,72 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t usba_vbus_irq(int irq, void *devid)
+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 0;
+}
+
+static void stop_clock(struct usba_udc *udc)
+{
+	if (!udc->clocked)
+		return;
+
+	clk_disable_unprepare(udc->hclk);
+	clk_disable_unprepare(udc->pclk);
+
+	udc->clocked = false;
+}
+
+static int usba_start(struct usba_udc *udc)
+{
+	unsigned long flags;
+	int ret;
+
+	ret = start_clock(udc);
+	if (ret)
+		return ret;
+
+	spin_lock_irqsave(&udc->lock, flags);
+	toggle_bias(udc, 1);
+	usba_writel(udc, CTRL, USBA_ENABLE_MASK);
+	usba_int_enb_set(udc, USBA_END_OF_RESET);
+	spin_unlock_irqrestore(&udc->lock, flags);
+
+	return 0;
+}
+
+static void usba_stop(struct usba_udc *udc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&udc->lock, flags);
+	udc->gadget.speed = USB_SPEED_UNKNOWN;
+	reset_all_endpoints(udc);
+
+	/* This will also disable the DP pullup */
+	toggle_bias(udc, 0);
+	usba_writel(udc, CTRL, USBA_DISABLE_MASK);
+	spin_unlock_irqrestore(&udc->lock, flags);
+
+	stop_clock(udc);
+}
+
+static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
 {
 	struct usba_udc *udc = devid;
 	int vbus;
@@ -1743,30 +1808,22 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
 	/* debounce */
 	udelay(10);
 
-	spin_lock(&udc->lock);
+	mutex_lock(&udc->vbus_mutex);
 
 	vbus = vbus_is_present(udc);
 	if (vbus != udc->vbus_prev) {
 		if (vbus) {
-			toggle_bias(udc, 1);
-			usba_writel(udc, CTRL, USBA_ENABLE_MASK);
-			usba_int_enb_set(udc, USBA_END_OF_RESET);
+			usba_start(udc);
 		} else {
-			udc->gadget.speed = USB_SPEED_UNKNOWN;
-			reset_all_endpoints(udc);
-			toggle_bias(udc, 0);
-			usba_writel(udc, CTRL, USBA_DISABLE_MASK);
-			if (udc->driver->disconnect) {
-				spin_unlock(&udc->lock);
+			usba_stop(udc);
+
+			if (udc->driver->disconnect)
 				udc->driver->disconnect(&udc->gadget);
-				spin_lock(&udc->lock);
-			}
 		}
 		udc->vbus_prev = vbus;
 	}
 
-	spin_unlock(&udc->lock);
-
+	mutex_unlock(&udc->vbus_mutex);
 	return IRQ_HANDLED;
 }
 
@@ -1778,57 +1835,47 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 	unsigned long flags;
 
 	spin_lock_irqsave(&udc->lock, flags);
-
 	udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
 	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) {
-		toggle_bias(udc, 1);
-		usba_writel(udc, CTRL, USBA_ENABLE_MASK);
-		usba_int_enb_set(udc, USBA_END_OF_RESET);
-
-		udc->vbus_prev = 1;
+	udc->vbus_prev = vbus_is_present(udc);
+	if (udc->vbus_prev) {
+		ret = usba_start(udc);
+		if (ret)
+			goto err;
 	}
-	spin_unlock_irqrestore(&udc->lock, flags);
 
+	mutex_unlock(&udc->vbus_mutex);
 	return 0;
+
+err:
+	if (gpio_is_valid(udc->vbus_pin))
+		disable_irq(gpio_to_irq(udc->vbus_pin));
+
+	mutex_unlock(&udc->vbus_mutex);
+
+	spin_lock_irqsave(&udc->lock, flags);
+	udc->devstatus &= ~(1 << USB_DEVICE_SELF_POWERED);
+	udc->driver = NULL;
+	spin_unlock_irqrestore(&udc->lock, flags);
+	return ret;
 }
 
 static int atmel_usba_stop(struct usb_gadget *gadget)
 {
 	struct usba_udc *udc = container_of(gadget, struct usba_udc, gadget);
-	unsigned long flags;
 
 	if (gpio_is_valid(udc->vbus_pin))
 		disable_irq(gpio_to_irq(udc->vbus_pin));
 
-	spin_lock_irqsave(&udc->lock, flags);
-	udc->gadget.speed = USB_SPEED_UNKNOWN;
-	reset_all_endpoints(udc);
-	spin_unlock_irqrestore(&udc->lock, flags);
-
-	/* This will also disable the DP pullup */
-	toggle_bias(udc, 0);
-	usba_writel(udc, CTRL, USBA_DISABLE_MASK);
-
-	clk_disable_unprepare(udc->hclk);
-	clk_disable_unprepare(udc->pclk);
+	usba_stop(udc);
 
 	udc->driver = NULL;
 
@@ -2050,6 +2097,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;
@@ -2106,9 +2154,9 @@ static int usba_udc_probe(struct platform_device *pdev)
 		if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
 			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
 					IRQ_NOAUTOEN);
-			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 497cd18..085749a 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -313,6 +313,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;
 
@@ -328,6 +331,7 @@ struct usba_udc {
 	struct clk *hclk;
 	struct usba_ep *usba_ep;
 	bool bias_pulse_needed;
+	bool clocked;
 
 	u16 devstatus;
 
-- 
2.1.4

[PATCHv6 4/5] USB: gadget: atmel_usba_udc: Prepare for IRQ single edge support

[Sylvain Rochet]

Renamed struct usba_udc_errata to struct usba_udc_caps, we are adding a
new property which is not about errata, this way the struct is not
misnamed.

New struct usba_udc_caps property: irq_single_edge_support, boolean,
set to true if the board supports IRQ_TYPE_EDGE_FALLING and
IRQ_TYPE_EDGE_RISING, otherwise set to false.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 25 +++++++++++++++----------
 drivers/usb/gadget/udc/atmel_usba_udc.h |  5 +++--
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index d554106..361f740 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -338,8 +338,8 @@ static int vbus_is_present(struct usba_udc *udc)
 
 static void toggle_bias(struct usba_udc *udc, int is_on)
 {
-	if (udc->errata && udc->errata->toggle_bias)
-		udc->errata->toggle_bias(udc, is_on);
+	if (udc->caps && udc->caps->toggle_bias)
+		udc->caps->toggle_bias(udc, is_on);
 }
 
 static void generate_bias_pulse(struct usba_udc *udc)
@@ -347,8 +347,8 @@ static void generate_bias_pulse(struct usba_udc *udc)
 	if (!udc->bias_pulse_needed)
 		return;
 
-	if (udc->errata && udc->errata->pulse_bias)
-		udc->errata->pulse_bias(udc);
+	if (udc->caps && udc->caps->pulse_bias)
+		udc->caps->pulse_bias(udc);
 
 	udc->bias_pulse_needed = false;
 }
@@ -1901,18 +1901,23 @@ static void at91sam9g45_pulse_bias(struct usba_udc *udc)
 	at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN);
 }
 
-static const struct usba_udc_errata at91sam9rl_errata = {
+static const struct usba_udc_caps at91sam9rl_caps = {
 	.toggle_bias = at91sam9rl_toggle_bias,
 };
 
-static const struct usba_udc_errata at91sam9g45_errata = {
+static const struct usba_udc_caps at91sam9g45_caps = {
 	.pulse_bias = at91sam9g45_pulse_bias,
+	.irq_single_edge_support = true,
+};
+
+static const struct usba_udc_caps sama5d3_caps = {
+	.irq_single_edge_support = true,
 };
 
 static const struct of_device_id atmel_udc_dt_ids[] = {
-	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata },
-	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata },
-	{ .compatible = "atmel,sama5d3-udc" },
+	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_caps },
+	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_caps },
+	{ .compatible = "atmel,sama5d3-udc", .data = &sama5d3_caps },
 	{ /* sentinel */ }
 };
 
@@ -1934,7 +1939,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 	if (!match)
 		return ERR_PTR(-EINVAL);
 
-	udc->errata = match->data;
+	udc->caps = match->data;
 
 	udc->num_ep = 0;
 
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index 085749a..4fe4c87 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -304,9 +304,10 @@ struct usba_request {
 	unsigned int				mapped:1;
 };
 
-struct usba_udc_errata {
+struct usba_udc_caps {
 	void (*toggle_bias)(struct usba_udc *udc, int is_on);
 	void (*pulse_bias)(struct usba_udc *udc);
+	bool irq_single_edge_support;
 };
 
 struct usba_udc {
@@ -322,7 +323,7 @@ struct usba_udc {
 	struct usb_gadget gadget;
 	struct usb_gadget_driver *driver;
 	struct platform_device *pdev;
-	const struct usba_udc_errata *errata;
+	const struct usba_udc_caps *caps;
 	int irq;
 	int vbus_pin;
 	int vbus_pin_inverted;
-- 
2.1.4

[PATCHv6 4/5] USB: gadget: atmel_usba_udc: Prepare for IRQ single edge support

[Boris Brezillon]

On Thu, 22 Jan 2015 17:56:44 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:

> Renamed struct usba_udc_errata to struct usba_udc_caps, we are adding a
> new property which is not about errata, this way the struct is not
> misnamed.
> 
> New struct usba_udc_caps property: irq_single_edge_support, boolean,
> set to true if the board supports IRQ_TYPE_EDGE_FALLING and
> IRQ_TYPE_EDGE_RISING, otherwise set to false.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 25 +++++++++++++++----------
>  drivers/usb/gadget/udc/atmel_usba_udc.h |  5 +++--
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index d554106..361f740 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -338,8 +338,8 @@ static int vbus_is_present(struct usba_udc *udc)
>  
>  static void toggle_bias(struct usba_udc *udc, int is_on)
>  {
> -	if (udc->errata && udc->errata->toggle_bias)
> -		udc->errata->toggle_bias(udc, is_on);
> +	if (udc->caps && udc->caps->toggle_bias)
> +		udc->caps->toggle_bias(udc, is_on);
>  }
>  
>  static void generate_bias_pulse(struct usba_udc *udc)
> @@ -347,8 +347,8 @@ static void generate_bias_pulse(struct usba_udc *udc)
>  	if (!udc->bias_pulse_needed)
>  		return;
>  
> -	if (udc->errata && udc->errata->pulse_bias)
> -		udc->errata->pulse_bias(udc);
> +	if (udc->caps && udc->caps->pulse_bias)
> +		udc->caps->pulse_bias(udc);
>  
>  	udc->bias_pulse_needed = false;
>  }
> @@ -1901,18 +1901,23 @@ static void at91sam9g45_pulse_bias(struct usba_udc *udc)
>  	at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN);
>  }
>  
> -static const struct usba_udc_errata at91sam9rl_errata = {
> +static const struct usba_udc_caps at91sam9rl_caps = {
>  	.toggle_bias = at91sam9rl_toggle_bias,
>  };
>  
> -static const struct usba_udc_errata at91sam9g45_errata = {
> +static const struct usba_udc_caps at91sam9g45_caps = {
>  	.pulse_bias = at91sam9g45_pulse_bias,
> +	.irq_single_edge_support = true,
> +};
> +
> +static const struct usba_udc_caps sama5d3_caps = {
> +	.irq_single_edge_support = true,
>  };
>  
>  static const struct of_device_id atmel_udc_dt_ids[] = {
> -	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata },
> -	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata },
> -	{ .compatible = "atmel,sama5d3-udc" },
> +	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_caps },
> +	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_caps },
> +	{ .compatible = "atmel,sama5d3-udc", .data = &sama5d3_caps },
>  	{ /* sentinel */ }
>  };
>  
> @@ -1934,7 +1939,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  	if (!match)
>  		return ERR_PTR(-EINVAL);
>  
> -	udc->errata = match->data;
> +	udc->caps = match->data;
>  
>  	udc->num_ep = 0;
>  
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
> index 085749a..4fe4c87 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
> @@ -304,9 +304,10 @@ struct usba_request {
>  	unsigned int				mapped:1;
>  };
>  
> -struct usba_udc_errata {
> +struct usba_udc_caps {
>  	void (*toggle_bias)(struct usba_udc *udc, int is_on);
>  	void (*pulse_bias)(struct usba_udc *udc);
> +	bool irq_single_edge_support;
>  };
>  
>  struct usba_udc {
> @@ -322,7 +323,7 @@ struct usba_udc {
>  	struct usb_gadget gadget;
>  	struct usb_gadget_driver *driver;
>  	struct platform_device *pdev;
> -	const struct usba_udc_errata *errata;
> +	const struct usba_udc_caps *caps;
>  	int irq;
>  	int vbus_pin;
>  	int vbus_pin_inverted;



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCHv6 4/5] USB: gadget: atmel_usba_udc: Prepare for IRQ single edge support

[Nicolas Ferre]

Le 22/01/2015 18:14, Boris Brezillon a ?crit :
> On Thu, 22 Jan 2015 17:56:44 +0100
> Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> 
>> Renamed struct usba_udc_errata to struct usba_udc_caps, we are adding a
>> new property which is not about errata, this way the struct is not
>> misnamed.
>>
>> New struct usba_udc_caps property: irq_single_edge_support, boolean,
>> set to true if the board supports IRQ_TYPE_EDGE_FALLING and
>> IRQ_TYPE_EDGE_RISING, otherwise set to false.
>>
>> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> 
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Some comments:

>> ---
>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 25 +++++++++++++++----------
>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  5 +++--
>>  2 files changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index d554106..361f740 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -338,8 +338,8 @@ static int vbus_is_present(struct usba_udc *udc)
>>  
>>  static void toggle_bias(struct usba_udc *udc, int is_on)
>>  {
>> -	if (udc->errata && udc->errata->toggle_bias)
>> -		udc->errata->toggle_bias(udc, is_on);
>> +	if (udc->caps && udc->caps->toggle_bias)
>> +		udc->caps->toggle_bias(udc, is_on);
>>  }
>>  
>>  static void generate_bias_pulse(struct usba_udc *udc)
>> @@ -347,8 +347,8 @@ static void generate_bias_pulse(struct usba_udc *udc)
>>  	if (!udc->bias_pulse_needed)
>>  		return;
>>  
>> -	if (udc->errata && udc->errata->pulse_bias)
>> -		udc->errata->pulse_bias(udc);
>> +	if (udc->caps && udc->caps->pulse_bias)
>> +		udc->caps->pulse_bias(udc);
>>  
>>  	udc->bias_pulse_needed = false;
>>  }
>> @@ -1901,18 +1901,23 @@ static void at91sam9g45_pulse_bias(struct usba_udc *udc)
>>  	at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN);
>>  }
>>  
>> -static const struct usba_udc_errata at91sam9rl_errata = {
>> +static const struct usba_udc_caps at91sam9rl_caps = {
>>  	.toggle_bias = at91sam9rl_toggle_bias,
>>  };
>>  
>> -static const struct usba_udc_errata at91sam9g45_errata = {
>> +static const struct usba_udc_caps at91sam9g45_caps = {
>>  	.pulse_bias = at91sam9g45_pulse_bias,
>> +	.irq_single_edge_support = true,

Nope! at91sam9g45 doesn't have this property. You'll have to create
another compatible string and capabilities structure
("atmel,at91sam9x5-udc")

But still, I don't know if it's the proper approach. The possibility tro
trigger an IRQ on both edges or a single edge is a capacity of the gpio
controller, not the USBA IP. So, it's a little bit strange to have this
capability here.

I don't know if it's actually feasible but trying to configure the IRQ
on a single edge, testing if it's accepted by the GPIO irq controller
and if not, falling back to a "both edge" pattern. Doesn't it look like
a way to workaround this issue nicely? Can you give it a try?

Bye,

>> +};
>> +
>> +static const struct usba_udc_caps sama5d3_caps = {
>> +	.irq_single_edge_support = true,
>>  };
>>  
>>  static const struct of_device_id atmel_udc_dt_ids[] = {
>> -	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata },
>> -	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata },
>> -	{ .compatible = "atmel,sama5d3-udc" },
>> +	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_caps },
>> +	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_caps },
>> +	{ .compatible = "atmel,sama5d3-udc", .data = &sama5d3_caps },
>>  	{ /* sentinel */ }
>>  };
>>  
>> @@ -1934,7 +1939,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  	if (!match)
>>  		return ERR_PTR(-EINVAL);
>>  
>> -	udc->errata = match->data;
>> +	udc->caps = match->data;
>>  
>>  	udc->num_ep = 0;
>>  
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
>> index 085749a..4fe4c87 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
>> @@ -304,9 +304,10 @@ struct usba_request {
>>  	unsigned int				mapped:1;
>>  };
>>  
>> -struct usba_udc_errata {
>> +struct usba_udc_caps {
>>  	void (*toggle_bias)(struct usba_udc *udc, int is_on);
>>  	void (*pulse_bias)(struct usba_udc *udc);
>> +	bool irq_single_edge_support;
>>  };
>>  
>>  struct usba_udc {
>> @@ -322,7 +323,7 @@ struct usba_udc {
>>  	struct usb_gadget gadget;
>>  	struct usb_gadget_driver *driver;
>>  	struct platform_device *pdev;
>> -	const struct usba_udc_errata *errata;
>> +	const struct usba_udc_caps *caps;
>>  	int irq;
>>  	int vbus_pin;
>>  	int vbus_pin_inverted;
> 
> 
> 


-- 
Nicolas Ferre

[PATCHv6 4/5] USB: gadget: atmel_usba_udc: Prepare for IRQ single edge support

[Sylvain Rochet]

Hello Nicolas,


On Thu, Feb 05, 2015 at 06:19:55PM +0100, Nicolas Ferre wrote:
> Le 22/01/2015 18:14, Boris Brezillon a ?crit :
> > On Thu, 22 Jan 2015 17:56:44 +0100
> > Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> > 
> > > -static const struct usba_udc_errata at91sam9g45_errata = {
> > > +static const struct usba_udc_caps at91sam9g45_caps = {
> > >  	.pulse_bias = at91sam9g45_pulse_bias,
> > > +	.irq_single_edge_support = true,
> 
> Nope! at91sam9g45 doesn't have this property. You'll have to create
> another compatible string and capabilities structure
> ("atmel,at91sam9x5-udc")

Oops.


> But still, I don't know if it's the proper approach. The possibility to
> trigger an IRQ on both edges or a single edge is a capacity of the gpio
> controller, not the USBA IP. So, it's a little bit strange to have this
> capability here.

I agree.


> I don't know if it's actually feasible but trying to configure the IRQ
> on a single edge, testing if it's accepted by the GPIO irq controller
> and if not, falling back to a "both edge" pattern. Doesn't it look like
> a way to workaround this issue nicely? Can you give it a try?

Tried, it works, but it displays the following message from 
__irq_set_trigger() [1] during devm_request_threaded_irq(?, 
IRQF_TRIGGER_RISING, ?) on boards which does not support single-edge 
IRQ:

genirq: Setting trigger mode 1 for irq 176 failed (gpio_irq_type+0x0/0x34)

Is it acceptable ?
If not, is udc->caps->irq_single_edge_support boolean acceptable ?
If not, I am ok to drop the feature, this is only a bonus.


Sylvain


[1] http://lxr.free-electrons.com/source/kernel/irq/manage.c#L619

[PATCHv6 4/5] USB: gadget: atmel_usba_udc: Prepare for IRQ single edge support

[Boris Brezillon]

On Sat, 7 Feb 2015 20:37:23 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:

> Hello Nicolas,
> 
> 
> On Thu, Feb 05, 2015 at 06:19:55PM +0100, Nicolas Ferre wrote:
> > Le 22/01/2015 18:14, Boris Brezillon a ?crit :
> > > On Thu, 22 Jan 2015 17:56:44 +0100
> > > Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> > > 
> > > > -static const struct usba_udc_errata at91sam9g45_errata = {
> > > > +static const struct usba_udc_caps at91sam9g45_caps = {
> > > >  	.pulse_bias = at91sam9g45_pulse_bias,
> > > > +	.irq_single_edge_support = true,
> > 
> > Nope! at91sam9g45 doesn't have this property. You'll have to create
> > another compatible string and capabilities structure
> > ("atmel,at91sam9x5-udc")
> 
> Oops.
> 
> 
> > But still, I don't know if it's the proper approach. The possibility to
> > trigger an IRQ on both edges or a single edge is a capacity of the gpio
> > controller, not the USBA IP. So, it's a little bit strange to have this
> > capability here.
> 
> I agree.

Me too (even if I'm the one who proposed this approach in the first
place ;-)).

> 
> 
> > I don't know if it's actually feasible but trying to configure the IRQ
> > on a single edge, testing if it's accepted by the GPIO irq controller
> > and if not, falling back to a "both edge" pattern. Doesn't it look like
> > a way to workaround this issue nicely? Can you give it a try?
> 
> Tried, it works, but it displays the following message from 
> __irq_set_trigger() [1] during devm_request_threaded_irq(?, 
> IRQF_TRIGGER_RISING, ?) on boards which does not support single-edge 
> IRQ:
> 
> genirq: Setting trigger mode 1 for irq 176 failed (gpio_irq_type+0x0/0x34)
> 
> Is it acceptable ?

IMHO it's not.

> If not, is udc->caps->irq_single_edge_support boolean acceptable ?

Do you mean keeping the current approach ?
If you do, then maybe you can rework a bit the way you detect the GPIO
controller you depends on: instead of linking this information to the
usba compatible string you could link it to the gpio controller
compatible string.

You can find the gpio controller node thanks to your "vbus-gpio"
property: use the phandle defined in this property to find the gpio
controller node, and once you have the device_node referencing the gpio
controller you can match it with your internal device_id table
(containing 2 entries: one for the at91rm9200 IP and the other for the
at91sam9x5 IP).

Another solution would be to add an irq_try_set_irq_type function that
would not complain when it fails to set the requested trigger.

Thomas, I know you did not follow the whole thread, but would you mind
adding this irq_try_set_irq_type function (here is a reference
implementation [1]), to prevent this error trace from happening when
we're just trying a configuration ?

> If not, I am ok to drop the feature, this is only a bonus.

That could be a short term solution, to get this series accepted. We
could then find a proper way to support that optimization.


Best Regards,

Boris

[1]http://code.bulix.org/z4bu9k-87846

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCHv6 4/5] USB: gadget: atmel_usba_udc: Prepare for IRQ single edge support

[Sylvain Rochet]

Hello Boris,

On Sun, Feb 08, 2015 at 10:24:39AM +0100, Boris Brezillon wrote:
> On Sat, 7 Feb 2015 20:37:23 +0100
> Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> 
> > If not, is udc->caps->irq_single_edge_support boolean acceptable ?
> 
> Do you mean keeping the current approach ?

Yes!


> If you do, then maybe you can rework a bit the way you detect the GPIO
> controller you depends on: instead of linking this information to the
> usba compatible string you could link it to the gpio controller
> compatible string.
> 
> You can find the gpio controller node thanks to your "vbus-gpio"
> property: use the phandle defined in this property to find the gpio
> controller node, and once you have the device_node referencing the gpio
> controller you can match it with your internal device_id table
> (containing 2 entries: one for the at91rm9200 IP and the other for the
> at91sam9x5 IP).

I have a working PoC for that if this is the chosen solution.


> Another solution would be to add an irq_try_set_irq_type function that
> would not complain when it fails to set the requested trigger.
> 
> Thomas, I know you did not follow the whole thread, but would you mind
> adding this irq_try_set_irq_type function (here is a reference
> implementation [1]), to prevent this error trace from happening when
> we're just trying a configuration ?

This would be great :-)


> > If not, I am ok to drop the feature, this is only a bonus.
> 
> That could be a short term solution, to get this series accepted. We
> could then find a proper way to support that optimization.

I agree, I have the feeling your proposed core change may takes a long 
time, I just sent a v7 without IRQ single edge support.


Sylvain

[PATCHv6 5/5] USB: gadget: atmel_usba_udc: Add suspend/resume with wakeup support

[Sylvain Rochet]

This patch add suspend/resume with wakeup support for Atmel USBA.

On suspend: We stay continuously clocked if Vbus signal is not
available. If Vbus signal is available we set the Vbus signal as a wake
up source then we stop the USBA itself and all clocks used by USBA.

On resume: We recover clocks and USBA if we stopped them. If a device is
currently connected at resume time we enable the controller.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 68 +++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 361f740..7942c2f 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -2178,6 +2178,7 @@ static int usba_udc_probe(struct platform_device *pdev)
 	ret = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
 	if (ret)
 		return ret;
+	device_init_wakeup(&pdev->dev, 1);
 
 	usba_init_debugfs(udc);
 	for (i = 1; i < udc->num_ep; i++)
@@ -2193,6 +2194,7 @@ static int __exit usba_udc_remove(struct platform_device *pdev)
 
 	udc = platform_get_drvdata(pdev);
 
+	device_init_wakeup(&pdev->dev, 0);
 	usb_del_gadget_udc(&udc->gadget);
 
 	for (i = 1; i < udc->num_ep; i++)
@@ -2202,10 +2204,76 @@ static int __exit usba_udc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int usba_udc_suspend(struct device *dev)
+{
+	struct usba_udc *udc = dev_get_drvdata(dev);
+
+	/* Not started */
+	if (!udc->driver)
+		return 0;
+
+	mutex_lock(&udc->vbus_mutex);
+
+	if (!device_may_wakeup(dev)) {
+		usba_stop(udc);
+		goto out;
+	}
+
+	/*
+	 * Device may wake up. We stay clocked if we failed
+	 * to request vbus irq, assuming always on.
+	 */
+	if (gpio_is_valid(udc->vbus_pin)) {
+		usba_stop(udc);
+		/* Only wake up on device connection */
+		if (udc->caps && udc->caps->irq_single_edge_support)
+			irq_set_irq_type(gpio_to_irq(udc->vbus_pin),
+				udc->vbus_pin_inverted ?
+				IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING);
+
+		enable_irq_wake(gpio_to_irq(udc->vbus_pin));
+	}
+
+out:
+	mutex_unlock(&udc->vbus_mutex);
+	return 0;
+}
+
+static int usba_udc_resume(struct device *dev)
+{
+	struct usba_udc *udc = dev_get_drvdata(dev);
+
+	/* Not started */
+	if (!udc->driver)
+		return 0;
+
+	if (device_may_wakeup(dev) && gpio_is_valid(udc->vbus_pin)) {
+		disable_irq_wake(gpio_to_irq(udc->vbus_pin));
+
+		if (udc->caps && udc->caps->irq_single_edge_support)
+			irq_set_irq_type(gpio_to_irq(udc->vbus_pin),
+					IRQ_TYPE_EDGE_BOTH);
+	}
+
+	/* If Vbus is present, enable the controller and wait for reset */
+	mutex_lock(&udc->vbus_mutex);
+	udc->vbus_prev = vbus_is_present(udc);
+	if (udc->vbus_prev)
+		usba_start(udc);
+	mutex_unlock(&udc->vbus_mutex);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(usba_udc_pm_ops, usba_udc_suspend, usba_udc_resume);
+
 static struct platform_driver udc_driver = {
 	.remove		= __exit_p(usba_udc_remove),
 	.driver		= {
 		.name		= "atmel_usba_udc",
+		.pm		= &usba_udc_pm_ops,
 		.of_match_table	= of_match_ptr(atmel_udc_dt_ids),
 	},
 };
-- 
2.1.4

[PATCHv6 5/5] USB: gadget: atmel_usba_udc: Add suspend/resume with wakeup support

[Nicolas Ferre]

Le 22/01/2015 17:56, Sylvain Rochet a ?crit :
> This patch add suspend/resume with wakeup support for Atmel USBA.
> 
> On suspend: We stay continuously clocked if Vbus signal is not
> available. If Vbus signal is available we set the Vbus signal as a wake
> up source then we stop the USBA itself and all clocks used by USBA.
> 
> On resume: We recover clocks and USBA if we stopped them. If a device is
> currently connected at resume time we enable the controller.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 68 +++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 361f740..7942c2f 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -2178,6 +2178,7 @@ static int usba_udc_probe(struct platform_device *pdev)
>  	ret = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
>  	if (ret)
>  		return ret;
> +	device_init_wakeup(&pdev->dev, 1);
>  
>  	usba_init_debugfs(udc);
>  	for (i = 1; i < udc->num_ep; i++)
> @@ -2193,6 +2194,7 @@ static int __exit usba_udc_remove(struct platform_device *pdev)
>  
>  	udc = platform_get_drvdata(pdev);
>  
> +	device_init_wakeup(&pdev->dev, 0);
>  	usb_del_gadget_udc(&udc->gadget);
>  
>  	for (i = 1; i < udc->num_ep; i++)
> @@ -2202,10 +2204,76 @@ static int __exit usba_udc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int usba_udc_suspend(struct device *dev)
> +{
> +	struct usba_udc *udc = dev_get_drvdata(dev);
> +
> +	/* Not started */
> +	if (!udc->driver)
> +		return 0;
> +
> +	mutex_lock(&udc->vbus_mutex);
> +
> +	if (!device_may_wakeup(dev)) {
> +		usba_stop(udc);
> +		goto out;
> +	}
> +
> +	/*
> +	 * Device may wake up. We stay clocked if we failed
> +	 * to request vbus irq, assuming always on.
> +	 */
> +	if (gpio_is_valid(udc->vbus_pin)) {
> +		usba_stop(udc);
> +		/* Only wake up on device connection */
> +		if (udc->caps && udc->caps->irq_single_edge_support)
> +			irq_set_irq_type(gpio_to_irq(udc->vbus_pin),
> +				udc->vbus_pin_inverted ?
> +				IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING);

As said earlier, let's try another approach for this.

Bye,

> +
> +		enable_irq_wake(gpio_to_irq(udc->vbus_pin));
> +	}
> +
> +out:
> +	mutex_unlock(&udc->vbus_mutex);
> +	return 0;
> +}
> +
> +static int usba_udc_resume(struct device *dev)
> +{
> +	struct usba_udc *udc = dev_get_drvdata(dev);
> +
> +	/* Not started */
> +	if (!udc->driver)
> +		return 0;
> +
> +	if (device_may_wakeup(dev) && gpio_is_valid(udc->vbus_pin)) {
> +		disable_irq_wake(gpio_to_irq(udc->vbus_pin));
> +
> +		if (udc->caps && udc->caps->irq_single_edge_support)
> +			irq_set_irq_type(gpio_to_irq(udc->vbus_pin),
> +					IRQ_TYPE_EDGE_BOTH);
> +	}
> +
> +	/* If Vbus is present, enable the controller and wait for reset */
> +	mutex_lock(&udc->vbus_mutex);
> +	udc->vbus_prev = vbus_is_present(udc);
> +	if (udc->vbus_prev)
> +		usba_start(udc);
> +	mutex_unlock(&udc->vbus_mutex);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(usba_udc_pm_ops, usba_udc_suspend, usba_udc_resume);
> +
>  static struct platform_driver udc_driver = {
>  	.remove		= __exit_p(usba_udc_remove),
>  	.driver		= {
>  		.name		= "atmel_usba_udc",
> +		.pm		= &usba_udc_pm_ops,
>  		.of_match_table	= of_match_ptr(atmel_udc_dt_ids),
>  	},
>  };
> 


-- 
Nicolas Ferre