Skip to contentSkip to conversation

USB: gadget: atmel_usba_udc: Start clocks on rising edge of the Vbus signal, stop clocks on falling edge of the Vbus signal

PATCH Superseded Sylvain Rochet 2 participants 6 messages Jan 20, 2015 – Jan 21, 2015

Subsystems (3)

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v4 cover USB: gadget: atmel_usba_udc: 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) reduce power consumption by switching off the USB PLL if no USB Host is currently connected to this USB Device.

Changes since v3:
* Added stable tag for the first patch
* As suggested, removed the unused check for udc->driver is not 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 (3):
USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state
USB: gadget: atmel_usba_udc: Enable Vbus signal IRQ in UDC start
instead of UDC probe
USB: gadget: atmel_usba_udc: Start clocks on rising edge of the Vbus
signal, stop clocks on falling edge of the Vbus signal

 drivers/usb/gadget/udc/atmel_usba_udc.c | 130 +++++++++++++++++++++-----------
 drivers/usb/gadget/udc/atmel_usba_udc.h |   4 +
 2 files changed, 89 insertions(+), 45 deletions(-)
Signature · 1 line
2.1.4
#1 lore raw

PATCH v4 1/3 USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state

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.

Fixes 914a3f3b3754 ("USB: add atmel_usba_udc driver")
Cc <[email protected]> #2.6.24+
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/usb/gadget/udc/atmel_usba_udc.c +2 -0
index ce88237..e207d75 100644
@@ -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);
 
Signature · 1 line
2.1.4
#2 lore raw

PATCH v4 2/3 USB: gadget: atmel_usba_udc: Enable Vbus signal IRQ in UDC start instead of UDC probe

In reply to 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.

This patch now request the Vbus signal IRQ in UDC start instead of UDC probe and release the IRQ in UDC stop before udc->driver is set back to NULL. This way we don't need the check about udc->driver in interruption handler, therefore we don't need the spinlock as well anymore.

This was chosen against using set_irq_flags() to request a not auto enabled IRQ (IRQF_NOAUTOEN flag) because set_irq_flags() can't change just one flag, therefore it must be called with all flags, without respecting what the AIC previously did. Naively copying IRQ flags currently set by the AIC looked like error-prone if defaults flags change at some point in the future.

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 64 ++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 32 deletions(-)
drivers/usb/gadget/udc/atmel_usba_udc.c +32 -32
index e207d75..546da63 100644
@@ -1729,10 +1729,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) {
@@ -1753,7 +1749,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
 		udc->vbus_prev = vbus;
 	}
 
-out:
 	spin_unlock(&udc->lock);
 
 	return IRQ_HANDLED;
@@ -1767,23 +1762,27 @@ 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;
+		goto err_pclk;
 	ret = clk_prepare_enable(udc->hclk);
-	if (ret) {
-		clk_disable_unprepare(udc->pclk);
-		return ret;
-	}
+	if (ret)
+		goto err_hclk;
 
 	udc->vbus_prev = 0;
-	if (gpio_is_valid(udc->vbus_pin))
-		enable_irq(gpio_to_irq(udc->vbus_pin));
+	if (gpio_is_valid(udc->vbus_pin)) {
+		ret = request_irq(gpio_to_irq(udc->vbus_pin),
+				usba_vbus_irq, 0,
+				"atmel_usba_udc", udc);
+		if (ret) {
+			udc->vbus_pin = -ENODEV;
+			goto err;
+		}
+	}
 
 	/* If Vbus is present, enable the controller and wait for reset */
 	spin_lock_irqsave(&udc->lock, flags);
@@ -1797,6 +1796,17 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 	spin_unlock_irqrestore(&udc->lock, flags);
 
 	return 0;
+
+err:
+	clk_disable_unprepare(udc->hclk);
+err_hclk:
+	clk_disable_unprepare(udc->pclk);
+err_pclk:
+	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)
@@ -1805,7 +1815,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
 	unsigned long flags;
 
 	if (gpio_is_valid(udc->vbus_pin))
-		disable_irq(gpio_to_irq(udc->vbus_pin));
+		free_irq(gpio_to_irq(udc->vbus_pin), udc);
 
 	spin_lock_irqsave(&udc->lock, flags);
 	udc->gadget.speed = USB_SPEED_UNKNOWN;
@@ -2047,24 +2057,14 @@ static int usba_udc_probe(struct platform_device *pdev)
 	}
 	udc->irq = irq;
 
-	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,
-					"atmel_usba_udc", udc);
-			if (ret) {
-				udc->vbus_pin = -ENODEV;
-				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 */
-			udc->vbus_pin = -EINVAL;
-		}
+	if (gpio_is_valid(udc->vbus_pin)
+			&& devm_gpio_request(&pdev->dev, udc->vbus_pin,
+					"atmel_usba_udc")) {
+		/* gpio_request fail so use -EINVAL for gpio_is_valid */
+		udc->vbus_pin = -EINVAL;
+		dev_warn(&udc->pdev->dev,
+			"failed to request vbus gpio; "
+			"assuming always on\n");
 	}
 
 	ret = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
Signature · 1 line
2.1.4
#3 lore raw

PATCH v4 2/3 USB: gadget: atmel_usba_udc: Enable Vbus signal IRQ in UDC start instead of UDC probe

Hi Sylvain,

Quoted 66 lines · On Tue, 20 Jan 2015 22:23:29 +0100 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.

This patch now request the Vbus signal IRQ in UDC start instead of UDC probe and release the IRQ in UDC stop before udc->driver is set back to NULL. This way we don't need the check about udc->driver in interruption handler, therefore we don't need the spinlock as well anymore.

This was chosen against using set_irq_flags() to request a not auto enabled IRQ (IRQF_NOAUTOEN flag) because set_irq_flags() can't change just one flag, therefore it must be called with all flags, without respecting what the AIC previously did. Naively copying IRQ flags currently set by the AIC looked like error-prone if defaults flags change at some point in the future.

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 64 ++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 32 deletions(-)
drivers/usb/gadget/udc/atmel_usba_udc.c +3 -11
index e207d75..546da63 100644
@@ -1729,10 +1729,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) {
@@ -1753,7 +1749,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
 		udc->vbus_prev = vbus;
 	}
 
-out:
 	spin_unlock(&udc->lock);
 
 	return IRQ_HANDLED;
@@ -1767,23 +1762,27 @@ 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;
+		goto err_pclk;
 	ret = clk_prepare_enable(udc->hclk);
-	if (ret) {
-		clk_disable_unprepare(udc->pclk);
-		return ret;
-	}
+	if (ret)
+		goto err_hclk;

drivers/usb/gadget/udc/atmel_usba_udc.c static int atmel_usba_start(struct usb_gadget *gadget, · line 1782

Quoted 10 lines
drivers/usb/gadget/udc/atmel_usba_udc.c +6 -2
 
 	udc->vbus_prev = 0;
-	if (gpio_is_valid(udc->vbus_pin))
-		enable_irq(gpio_to_irq(udc->vbus_pin));
+	if (gpio_is_valid(udc->vbus_pin)) {
+		ret = request_irq(gpio_to_irq(udc->vbus_pin),
+				usba_vbus_irq, 0,
+				"atmel_usba_udc", udc);
+		if (ret) {
+			udc->vbus_pin = -ENODEV;

I guess you're trying to protect against free_irq by changing the vbus_pin value (making it an invalid gpio id), but I think you should leave it unchanged for two reasons:
1) If the request_irq call temporary fails (an ENOMEM for example) then you should be able to retry later, and modifying the vbus_pin value will prevent that.
2) atmel_usba_stop will never be called if the atmel_usba_start failed, so there's no need to protect against this free_irq.

Best Regards,

Boris

Signature · 3 lines
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
#4 lore raw

PATCH v4 2/3 USB: gadget: atmel_usba_udc: Enable Vbus signal IRQ in UDC start instead of UDC probe

Hi Boris,

Quoted 11 lines · Boris Brezillon

drivers/usb/gadget/udc/atmel_usba_udc.c static int atmel_usba_start(struct usb_gadget *gadget, · line 1782

Quoted 10 lines
drivers/usb/gadget/udc/atmel_usba_udc.c +6 -2
 	udc->vbus_prev = 0;
-	if (gpio_is_valid(udc->vbus_pin))
-		enable_irq(gpio_to_irq(udc->vbus_pin));
+	if (gpio_is_valid(udc->vbus_pin)) {
+		ret = request_irq(gpio_to_irq(udc->vbus_pin),
+				usba_vbus_irq, 0,
+				"atmel_usba_udc", udc);
+		if (ret) {
+			udc->vbus_pin = -ENODEV;
Quoted 8 lines

I guess you're trying to protect against free_irq by changing the vbus_pin value (making it an invalid gpio id), but I think you should leave it unchanged for two reasons:
1) If the request_irq call temporary fails (an ENOMEM for example) then you should be able to retry later, and modifying the vbus_pin value will prevent that.
2) atmel_usba_stop will never be called if the atmel_usba_start failed, so there's no need to protect against this free_irq.

Indeed.

By the way, I just discovered irq_set_status_flags(irq, IRQ_NOAUTOEN); ? so I am going for a v5 and the previous part is not relevant anymore.

Sylvain

#5 lore raw

PATCH v4 3/3 USB: gadget: atmel_usba_udc: Start clocks on rising edge of the Vbus signal, stop clocks on falling edge of the Vbus signal

In reply to Sylvain Rochet

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 stays 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.

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 86 ++++++++++++++++++++++++---------
 drivers/usb/gadget/udc/atmel_usba_udc.h |  4 ++
 2 files changed, 66 insertions(+), 24 deletions(-)
drivers/usb/gadget/udc/atmel_usba_udc.c +62 -24
index 546da63..e05b16c 100644
@@ -315,6 +315,37 @@ 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 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 vbus_is_present(struct usba_udc *udc)
 {
 	if (gpio_is_valid(udc->vbus_pin))
@@ -1719,37 +1750,48 @@ 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);
 
 	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;
 	}
 
-	spin_unlock(&udc->lock);
+out:
+	mutex_unlock(&udc->vbus_mutex);
 
 	return IRQ_HANDLED;
 }
@@ -1766,17 +1808,12 @@ 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)
-		goto err_pclk;
-	ret = clk_prepare_enable(udc->hclk);
-	if (ret)
-		goto err_hclk;
+	mutex_lock(&udc->vbus_mutex);
 
 	udc->vbus_prev = 0;
 	if (gpio_is_valid(udc->vbus_pin)) {
-		ret = request_irq(gpio_to_irq(udc->vbus_pin),
-				usba_vbus_irq, 0,
+		ret = request_threaded_irq(gpio_to_irq(udc->vbus_pin), NULL,
+				usba_vbus_irq_thread, IRQF_ONESHOT,
 				"atmel_usba_udc", udc);
 		if (ret) {
 			udc->vbus_pin = -ENODEV;
@@ -1785,23 +1822,24 @@ 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 = start_clock(udc);
+		if (ret)
+			goto err;
+
+		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);
 
+	mutex_unlock(&udc->vbus_mutex);
 	return 0;
-
 err:
-	clk_disable_unprepare(udc->hclk);
-err_hclk:
-	clk_disable_unprepare(udc->pclk);
-err_pclk:
+	mutex_unlock(&udc->vbus_mutex);
 	spin_lock_irqsave(&udc->lock, flags);
 	udc->devstatus &= ~(1 << USB_DEVICE_SELF_POWERED);
 	udc->driver = NULL;
@@ -1826,8 +1864,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;
 
@@ -2007,6 +2044,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;
drivers/usb/gadget/udc/atmel_usba_udc.h +4 -0
index a70706e..3ceed76 100644
@@ -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;
 
Signature · 1 line
2.1.4