at91_udc driver: read_fifo timing issue + use spinlocks

The locking code of this driver is reworked for preempt-rt.

Signed-off-by Harro Haan <[email protected]>

---
 drivers/usb/gadget/at91_udc.c |  102 ++++++++++++++++++++++++++++++------------
 drivers/usb/gadget/at91_udc.h |    1 
 2 files changed, 74 insertions(+), 29 deletions(-)

Index: linux-2.6.31/drivers/usb/gadget/at91_udc.h
===================================================================

drivers/usb/gadget/at91_udc.h +1 -0

@@ -144,6 +144,7 @@ struct at91_udc {
 	struct proc_dir_entry		*pde;
 	void __iomem			*udp_baseaddr;
 	int				udp_irq;
+	spinlock_t			lock;
 };
 
 static inline struct at91_udc *to_udc(struct usb_gadget *g)

Index: linux-2.6.31/drivers/usb/gadget/at91_udc.c
===================================================================

drivers/usb/gadget/at91_udc.c +3 -1

 	csr = __raw_readl(ep->creg);
 

@@ -147,7 +148,7 @@ static void proc_ep_show(struct seq_file
 				&req->req, length,
 				req->req.length, req->req.buf);
 	}
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
 }
 
 static void proc_irq_show(struct seq_file *s, const char *label, u32 mask)

@@ -272,7 +273,9 @@ static void done(struct at91_ep *ep, str
 		VDBG("%s done %p, status %d\n", ep->ep.name, req, status);
 
 	ep->stopped = 1;
+	spin_unlock(&udc->lock);
 	req->req.complete(&ep->ep, &req->req);
+	spin_lock(&udc->lock);
 	ep->stopped = stopped;
 
 	/* ep0 is always ready; other endpoints need a non-empty queue */

drivers/usb/gadget/at91_udc.c +2 -2

 	return 0;
 }
 

@@ -587,7 +590,7 @@ static int at91_ep_disable (struct usb_e
 	if (ep == &ep->udc->ep[0])
 		return -EINVAL;
 
-	local_irq_save(flags);
+	spin_lock_irqsave(&udc->lock, flags);
 
 	nuke(ep, -ESHUTDOWN);
 

@@ -602,7 +605,7 @@ static int at91_ep_disable (struct usb_e
 		__raw_writel(0, ep->creg);
 	}
 
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
 	return 0;
 }
 

drivers/usb/gadget/at91_udc.c +4 -4

 	return 0;
 }
 

@@ -767,7 +776,7 @@ static int at91_ep_set_halt(struct usb_e
 		return -EINVAL;
 
 	creg = ep->creg;
-	local_irq_save(flags);
+	spin_lock_irqsave(&udc->lock, flags);
 
 	csr = __raw_readl(creg);
 

@@ -792,7 +801,7 @@ static int at91_ep_set_halt(struct usb_e
 		__raw_writel(csr, creg);
 	}
 
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
 	return status;
 }
 

@@ -826,7 +835,7 @@ static int at91_wakeup(struct usb_gadget
 	unsigned long	flags;
 
 	DBG("%s\n", __func__ );
-	local_irq_save(flags);
+	spin_lock_irqsave(&udc->lock, flags);
 
 	if (!udc->clocked || !udc->suspended)
 		goto done;

@@ -840,7 +849,7 @@ static int at91_wakeup(struct usb_gadget
 	at91_udp_write(udc, AT91_UDP_GLB_STAT, glbstate);
 
 done:
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
 	return status;
 }
 

drivers/usb/gadget/at91_udc.c +6 -6

-	local_irq_save(flags);
+	spin_lock_irqsave(&udc->lock, flags);
 	udc->vbus = (is_active != 0);
 	if (udc->driver)
 		pullup(udc, is_active);
 	else
 		pullup(udc, 0);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
 	return 0;
 }
 

@@ -981,10 +993,10 @@ static int at91_pullup(struct usb_gadget
 	struct at91_udc	*udc = to_udc(gadget);
 	unsigned long	flags;
 
-	local_irq_save(flags);
+	spin_lock_irqsave(&udc->lock, flags);
 	udc->enabled = is_on = !!is_on;
 	pullup(udc, is_on);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
 	return 0;
 }
 

@@ -993,9 +1005,9 @@ static int at91_set_selfpowered(struct u
 	struct at91_udc	*udc = to_udc(gadget);
 	unsigned long	flags;
 
-	local_irq_save(flags);
+	spin_lock_irqsave(&udc->lock, flags);
 	udc->selfpowered = (is_on != 0);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
 	return 0;
 }
 

drivers/usb/gadget/at91_udc.c +29 -6

 	if (!udc->clocked) {
 		clk_on(udc);

@@ -1464,8 +1482,11 @@ static irqreturn_t at91_udc_irq (int irq
 			 * and then into standby to avoid drawing more than
 			 * 500uA power (2500uA for some high-power configs).
 			 */
-			if (udc->driver && udc->driver->suspend)
+			if (udc->driver && udc->driver->suspend) {
+				spin_unlock(&udc->lock);
 				udc->driver->suspend(&udc->gadget);
+				spin_lock(&udc->lock);
+			}
 
 		/* host initiated resume */
 		} else if (status & AT91_UDP_RXRSM) {

@@ -1482,8 +1503,11 @@ static irqreturn_t at91_udc_irq (int irq
 			 * would normally want to switch out of slow clock
 			 * mode into normal mode.
 			 */
-			if (udc->driver && udc->driver->resume)
+			if (udc->driver && udc->driver->resume) {
+				spin_unlock(&udc->lock);
 				udc->driver->resume(&udc->gadget);
+				spin_lock(&udc->lock);
+			}
 
 		/* endpoint IRQs are cleared by handling them */
 		} else {

@@ -1505,6 +1529,8 @@ static irqreturn_t at91_udc_irq (int irq
 	if (disable_clock)
 		clk_off(udc);
 
+	spin_unlock_irqrestore(&udc->lock, flags);
+
 	return IRQ_HANDLED;
 }
 

@@ -1605,6 +1631,7 @@ int usb_gadget_register_driver (struct u
 {
 	struct at91_udc	*udc = &controller;
 	int		retval;
+	unsigned long	flags;
 
 	if (!driver
 			|| driver->speed < USB_SPEED_FULL

@@ -1636,9 +1663,9 @@ int usb_gadget_register_driver (struct u
 		return retval;
 	}
 
-	local_irq_disable();
+	spin_lock_irqsave(&udc->lock, flags);
 	pullup(udc, 1);
-	local_irq_enable();
+	spin_unlock_irqrestore(&udc->lock, flags);
 
 	DBG("bound to %s\n", driver->driver.name);
 	return 0;

@@ -1648,15 +1675,16 @@ EXPORT_SYMBOL (usb_gadget_register_drive
 int usb_gadget_unregister_driver (struct usb_gadget_driver *driver)
 {
 	struct at91_udc *udc = &controller;
+	unsigned long	flags;
 
 	if (!driver || driver != udc->driver || !driver->unbind)
 		return -EINVAL;
 
-	local_irq_disable();
+	spin_lock_irqsave(&udc->lock, flags);
 	udc->enabled = 0;
 	at91_udp_write(udc, AT91_UDP_IDR, ~0);
 	pullup(udc, 0);
-	local_irq_enable();
+	spin_unlock_irqrestore(&udc->lock, flags);
 
 	driver->unbind(&udc->gadget);
 	udc->gadget.dev.driver = NULL;

@@ -1672,8 +1700,13 @@ EXPORT_SYMBOL (usb_gadget_unregister_dri
 
 static void at91udc_shutdown(struct platform_device *dev)
 {
+	struct at91_udc *udc = platform_get_drvdata(dev);
+	unsigned long	flags;
+
 	/* force disconnect on reboot */
+	spin_lock_irqsave(&udc->lock, flags);
 	pullup(platform_get_drvdata(dev), 0);
+	spin_unlock_irqrestore(&udc->lock, flags);
 }
 
 static int __init at91udc_probe(struct platform_device *pdev)

@@ -1716,6 +1749,7 @@ static int __init at91udc_probe(struct p
 	udc->board = *(struct at91_udc_data *) dev->platform_data;
 	udc->pdev = pdev;
 	udc->enabled = 0;
+	spin_lock_init(&udc->lock);
 
 	/* rm9200 needs manual D+ pullup; off by default */
 	if (cpu_is_at91rm9200()) {

@@ -1838,13 +1872,16 @@ static int __exit at91udc_remove(struct 
 {
 	struct at91_udc *udc = platform_get_drvdata(pdev);
 	struct resource *res;
+	unsigned long	flags;
 
 	DBG("remove\n");
 
 	if (udc->driver)
 		return -EBUSY;
 
+	spin_lock_irqsave(&udc->lock, flags);
 	pullup(udc, 0);
+	spin_unlock_irqrestore(&udc->lock, flags);
 
 	device_init_wakeup(&pdev->dev, 0);
 	remove_debug_file(udc);

@@ -1874,6 +1911,7 @@ static int at91udc_suspend(struct platfo
 {
 	struct at91_udc *udc = platform_get_drvdata(pdev);
 	int		wake = udc->driver && device_may_wakeup(&pdev->dev);
+	unsigned long	flags;
 
 	/* Unless we can act normally to the host (letting it wake us up
 	 * whenever it has work for us) force disconnect.  Wakeup requires

@@ -1883,8 +1921,10 @@ static int at91udc_suspend(struct platfo
 	if ((!udc->suspended && udc->addr)
 			|| !wake
 			|| clk_must_disable(udc->fclk)) {
+		spin_lock_irqsave(&udc->lock, flags);
 		pullup(udc, 0);
 		wake = 0;
+		spin_unlock_irqrestore(&udc->lock, flags);
 	} else
 		enable_irq_wake(udc->udp_irq);
 

@@ -1897,6 +1937,7 @@ static int at91udc_suspend(struct platfo
 static int at91udc_resume(struct platform_device *pdev)
 {
 	struct at91_udc *udc = platform_get_drvdata(pdev);
+	unsigned long	flags;
 
 	if (udc->board.vbus_pin > 0 && udc->active_suspend)
 		disable_irq_wake(udc->board.vbus_pin);

@@ -1904,8 +1945,11 @@ static int at91udc_resume(struct platfor
 	/* maybe reconnect to host; if so, clocks on */
 	if (udc->active_suspend)

For consistency (and better grepping) it would be better to keep the same use throughout the file.

This one should be &udc->lock

Strange unlock/lock sequence.

Who does the initial locking and who does the final unlock?

You are in an irq path. Is this locking and the unlock/lock below really needed?

This solves the read_fifo timing issue for example seen with CDC Ethernet. See the comments below for more info.

Signed-off-by Harro Haan <[email protected]>

---
?drivers/usb/gadget/at91_udc.c | ? 46 ++++++++++++++++++++++++++++++++++++++---- ?1 file changed, 42 insertions(+), 4 deletions(-)

Hi Harro,

2010/1/29 Harro Haan <[email protected]>:

Quoted 3 lines

This solves the read_fifo timing issue for example seen with CDC Ethernet. See the comments below for more info.

Quoted 5 lines

Signed-off-by Harro Haan <[email protected]>

---
 drivers/usb/gadget/at91_udc.c |   46 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

Acked-by Remy Bohmer <[email protected]>

This solves the read_fifo timing issue for example seen with CDC

Add some delay to avoid reading CSR TXCOUNT too early after updating it.

Signed-off-by Anti Sullin <[email protected]>

Signed-off-by Harro Haan <[email protected]>

Acked-by Remy Bohmer <[email protected]>

---
 drivers/usb/gadget/at91_udc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-2.6.31/drivers/usb/gadget/at91_udc.c
===================================================================

drivers/usb/gadget/at91_udc.c +0 -0

@@ -366,6 +366,13 @@ rescan:
       if (is_done)
               done(ep, req, 0);
       else if (ep->is_pingpong) {

hi,

I see in the data-sheet (SAM9261 / SAM9263), the following for the UDP_ CSRx registers:

"WARNING: Due to synchronization between MCK and UDPCK, the software application must wait for the end of the write
operation before executing another write by polling the bits which must be set/cleared."

//! Clear flags of UDP UDP_CSR register and waits for synchronization #define Udp_ep_clr_flag(pInterface, endpoint, flags) { \
? ?while (pInterface->UDP_CSR[endpoint] & (flags)) \
? ? ? ?pInterface->UDP_CSR[endpoint] &= ~(flags); \
? ? }
//! Set flags of UDP UDP_CSR register and waits for synchronization #define Udp_ep_set_flag(pInterface, endpoint, flags) { \
? ?while ( (pInterface->UDP_CSR[endpoint] & (flags)) != (flags) ) \
? ? ? pInterface->UDP_CSR[endpoint] |= (flags); \
? ?}

The at91_udc driver does not seem to do that for its CSR register writes. So I was wondering if we implement what the datasheet says, would we still need the "fix" above.

Regards,
?Andrew Victor

Quoted 3 lines · Andrew Victor

The at91_udc driver does not seem to do that for its CSR register writes. So I was wondering if we implement what the datasheet says, would we still need the "fix" above.

The locking code of this driver is reworked for preempt-rt.

Signed-off-by Harro Haan <[email protected]>

---
 drivers/usb/gadget/at91_udc.c |  139 ++++++++++++++++++++++++++++--------------
 drivers/usb/gadget/at91_udc.h |    1 
 2 files changed, 94 insertions(+), 46 deletions(-)