[patch 0/2] at91_udc driver: read_fifo timing issue + use spinlocks

[patch 0/2] at91_udc driver: read_fifo timing issue + use spinlocks

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: avoid-disclaimer-footer
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100127/e8798fae/attachment.el>

[patch 1/2] at91_udc.c read_fifo timing issue

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: at91_udc_read_fifo_timing_issue.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100127/0de009f5/attachment.el>

[patch 2/2] at91_udc.c use spinlocks instead of local_irq_xxx

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: at91_udc_use_spinlocks.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100127/e71fbe12/attachment.el>

[patch 2/2] at91_udc.c use spinlocks instead of local_irq_xxx

[H Hartley Sweeten]

On Wednesday, January 27, 2010 9:30 AM, Harro Haan wrote:

Just a couple comments on the locking.

The spinlock is stored in the at91_udc structure.  The structure
is referenced multiple times using different variable names.
Sometimes its:

	struct at91_udc *udc;

Othertimes it's:

	struct at91_udc *dev;

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

> The locking code of this driver is reworked for preempt-rt.
> 
> Signed-off-by: Harro Haan <hrhaan@gmail.com>
> ---
>  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
> ===================================================================
> --- linux-2.6.31.orig/drivers/usb/gadget/at91_udc.h
> +++ linux-2.6.31/drivers/usb/gadget/at91_udc.h
> @@ -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
> ===================================================================
> --- linux-2.6.31.orig/drivers/usb/gadget/at91_udc.c
> +++ linux-2.6.31/drivers/usb/gadget/at91_udc.c
> @@ -102,8 +102,9 @@ static void proc_ep_show(struct seq_file
>  	u32			csr;
>  	struct at91_request	*req;
>  	unsigned long	flags;
> +	struct at91_udc	*udc = ep->udc;
>  
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&dev->lock, flags);

This one should be &udc->lock

>  	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 */
> @@ -552,7 +555,7 @@ bogus_max:
>  	}
>  
>  ok:
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&dev->lock, flags);

This on is correct as &dev->lock

>  
>  	/* initialize endpoint to match this descriptor */
>  	ep->is_in = usb_endpoint_dir_in(desc);
> @@ -574,7 +577,7 @@ ok:
>  	at91_udp_write(dev, AT91_UDP_RST_EP, ep->int_mask);
>  	at91_udp_write(dev, AT91_UDP_RST_EP, 0);
>  
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&dev->lock, flags);

Again, this one is correct as &dev->lock.

>  	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;
>  }
>  
> @@ -666,7 +669,7 @@ static int at91_ep_queue(struct usb_ep *
>  	_req->status = -EINPROGRESS;
>  	_req->actual = 0;
>  
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&dev->lock, flags);

Again, this one is correct as &dev->lock.

>  
>  	/* try to kickstart any empty and idle queue */
>  	if (list_empty(&ep->queue) && !ep->stopped) {
> @@ -729,28 +732,34 @@ ep0_in_status:
>  		at91_udp_write(dev, AT91_UDP_IER, ep->int_mask);
>  	}
>  done:
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&dev->lock, flags);

Correct as &dev->lock.

>  	return (status < 0) ? status : 0;
>  }
>  
>  static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>  {
> -	struct at91_ep	*ep;
> +	struct at91_ep		*ep;
>  	struct at91_request	*req;
> +	unsigned long		flags;

Maybe also add:

	struct at91_udc		*udc;

>  
>  	ep = container_of(_ep, struct at91_ep, ep);
>  	if (!_ep || ep->ep.name == ep0name)
>  		return -EINVAL;

	udc = ep->udc;

>  
> +	spin_lock_irqsave(&ep->udc->lock, flags);
> +

Then change this to:

	spin_lock_irqsave(&udc->lock, flags);

>  	/* make sure it's actually queued on this endpoint */
>  	list_for_each_entry (req, &ep->queue, queue) {
>  		if (&req->req == _req)
>  			break;
>  	}
> -	if (&req->req != _req)
> +	if (&req->req != _req) {
> +		spin_unlock_irqrestore(&ep->udc->lock, flags);

	spin_unlock_irqrestore(&udc->lock, flags);

>  		return -EINVAL;
> +	}
>  
>  	done(ep, req, -ECONNRESET);
> +	spin_unlock_irqrestore(&ep->udc->lock, flags);

	spin_unlock_irqrestore(&udc->lock, flags);

>  	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;
>  }
>  
> @@ -882,8 +891,11 @@ static void stop_activity(struct at91_ud
>  		ep->stopped = 1;
>  		nuke(ep, -ESHUTDOWN);
>  	}
> -	if (driver)
> +	if (driver) {
> +		spin_unlock(&udc->lock);
>  		driver->disconnect(&udc->gadget);
> +		spin_lock(&udc->lock);
> +	}

Strange unlock/lock sequence.  Who does the initial locking and who
does the final unlock?

>  
>  	udc_reinit(udc);
>  }
> @@ -966,13 +978,13 @@ static int at91_vbus_session(struct usb_
>  	unsigned long	flags;
>  
>  	// VDBG("vbus %s\n", is_active ? "on" : "off");

// comment?  If it's really not needed just remove it.

> -	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;
>  }
>  
> @@ -1257,8 +1269,11 @@ static void handle_setup(struct at91_udc
>  #undef w_length
>  
>  	/* pass request up to the gadget driver */
> -	if (udc->driver)
> +	if (udc->driver) {
> +		spin_unlock(&udc->lock);
>  		status = udc->driver->setup(&udc->gadget, &pkt.r);
> +		spin_lock(&udc->lock);

Again strange unlock/lock sequence.

> +	}
>  	else
>  		status = -ENODEV;
>  	if (status < 0) {
> @@ -1409,6 +1424,9 @@ static irqreturn_t at91_udc_irq (int irq
>  	struct at91_udc		*udc = _udc;
>  	u32			rescans = 5;
>  	int			disable_clock = 0;
> +	unsigned long		flags;
> +
> +	spin_lock_irqsave(&udc->lock, flags);

You are in an irq path.  Is this locking and the unlock/lock below
really needed?
 
>  	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)
>  		disable_irq_wake(udc->udp_irq);
> -	else
> +	else {
> +		spin_lock_irqsave(&udc->lock, flags);
>  		pullup(udc, 1);
> +		spin_unlock_irqrestore(&udc->lock, flags);
> +	}
>  	return 0;
>  }
>  #else

Regards,
Hartley

[patch 2/2] at91_udc.c use spinlocks instead of local_irq_xxx

[Harro Haan]

Hartley,

Thanks for the feedback.

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

I agree on the naming consistency. I reused the variable when it was already
present, but will make it consistent.

> This one should be &udc->lock

Yes, the spin_lock_irqsave() in proc_ep_show() is incorrect. It compiled because
CONFIG_USB_GADGET_DEBUG_FILES is not defined. Will fix this.

> Strange unlock/lock sequence.

This trick is used to avoid recursive locking. It is also used in:
amd5536udc.c, atmel_usba_udc.c, ci3xxx_udc.c, fsl_qe_udc.c,
fsl_udc_core.c, goku_udc.c, lh7a40x_udc.c, m66592-udc.c and omap_udc.c

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

The unlock/lock sequence is used by the following functions:

handle_setup() _ handle_ep0() _ at91_udc_irq()

stop_activity() _ pullup() (only called with spinlock)
stop_activity() _ at91_udc_irq()

done() _ read_fifo() _ at91_ep_queue() (uses spinlock)
done() _ read_fifo() _ handle_ep() _ handle_setup() _ handle_ep0() _
at91_udc_irq()
done() _ read_fifo() _ handle_ep() _ handle_ep0() _ at91_udc_irq()
done() _ read_fifo() _ handle_ep() _ at91_udc_irq()
done() _ write_fifo() _ at91_ep_queue() (uses spinlock)
done() _ write_fifo() _ handle_ep() _ handle_setup() _ handle_ep0() _
at91_udc_irq()
done() _ write_fifo() _ handle_ep() _ handle_ep0() _ at91_udc_irq()
done() _ write_fifo() _ handle_ep() _ at91_udc_irq()
done() _ nuke() _ stop_activity() _ pullup() (only called with spinlock)
done() _ nuke() _ stop_activity() _ at91_udc_irq()
done() _ nuke() _ handle_ep0() _ at91_udc_irq()
done() _ nuke() _ at91_ep_disable() (uses spinlock)
done() _ at91_ep_dequeue() (uses spinlock)
done() _ handle_ep0() _ at91_udc_irq()

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

Yes, to avoid recursive locking.

udc->driver->suspend() = at91udc_suspend() (uses spinlock)
udc->driver->resume() = at91udc_resume() (uses spinlock)

Regards,

Harro

[patch v2 0/2] at91_udc driver: read_fifo timing issue + use spinlocks

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: avoid-disclaimer-footer
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100129/8247e3dc/attachment.el>

[patch v2 1/2] at91_udc.c read_fifo timing issue

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: at91_udc_read_fifo_timing_issue.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100129/10ba0f8b/attachment.el>

[patch v2 2/2] at91_udc.c use spinlocks instead of local_irq_xxx

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: at91_udc_use_spinlocks.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100129/68686eee/attachment.el>

Re: [patch v2 1/2] at91_udc.c read_fifo timing issue

[Remy Bohmer]

Hi Harro,

2010/1/29 Harro Haan <hrhaan@gmail.com>:
> 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 <hrhaan@gmail.com>
> ---
>  drivers/usb/gadget/at91_udc.c |   46 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)
>
Acked-by: Remy Bohmer <linux@bohmer.net>

[patch v2 1/2] at91_udc.c read_fifo timing issue

[Remy Bohmer]

Hi Harro,

2010/1/29 Harro Haan <hrhaan@gmail.com>:
> 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 <hrhaan@gmail.com>
> ---
> ?drivers/usb/gadget/at91_udc.c | ? 46 ++++++++++++++++++++++++++++++++++++++----
> ?1 file changed, 42 insertions(+), 4 deletions(-)
>
Acked-by: Remy Bohmer <linux@bohmer.net>

RE: [patch v2 2/2] at91_udc.c use spinlocks instead of local_irq_xxx

[H Hartley Sweeten]

On Friday, January 29, 2010 9:21 AM, Harro Haan wrote:
>
> The locking code of this driver is reworked for preempt-rt.
>
> Signed-off-by: Harro Haan <hrhaan@gmail.com>
> ---
>  drivers/usb/gadget/at91_udc.c |  139 ++++++++++++++++++++++++++++--------------
>  drivers/usb/gadget/at91_udc.h |    1 
>  2 files changed, 94 insertions(+), 46 deletions(-)

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

[patch v2 2/2] at91_udc.c use spinlocks instead of local_irq_xxx

[H Hartley Sweeten]

On Friday, January 29, 2010 9:21 AM, Harro Haan wrote:
>
> The locking code of this driver is reworked for preempt-rt.
>
> Signed-off-by: Harro Haan <hrhaan@gmail.com>
> ---
>  drivers/usb/gadget/at91_udc.c |  139 ++++++++++++++++++++++++++++--------------
>  drivers/usb/gadget/at91_udc.h |    1 
>  2 files changed, 94 insertions(+), 46 deletions(-)

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Re: [patch v2 1/2] at91_udc.c read_fifo timing issue

[Ryan Mallon]

Remy Bohmer wrote:
> Hi Harro,
> 
> 2010/1/29 Harro Haan <hrhaan@gmail.com>:
>> 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 <hrhaan@gmail.com>
>> ---
>>  drivers/usb/gadget/at91_udc.c |   46 ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 42 insertions(+), 4 deletions(-)
>>
> Acked-by: Remy Bohmer <linux@bohmer.net>

Can I get your Acked-by for my patch also Remy?

Harro, I can either submit my patch to the patch system separately, or
you can put it through with your series with my From/Signed-off-by tag
on it.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

[patch v2 1/2] at91_udc.c read_fifo timing issue

[Ryan Mallon]

Remy Bohmer wrote:
> Hi Harro,
> 
> 2010/1/29 Harro Haan <hrhaan@gmail.com>:
>> 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 <hrhaan@gmail.com>
>> ---
>>  drivers/usb/gadget/at91_udc.c |   46 ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 42 insertions(+), 4 deletions(-)
>>
> Acked-by: Remy Bohmer <linux@bohmer.net>

Can I get your Acked-by for my patch also Remy?

Harro, I can either submit my patch to the patch system separately, or
you can put it through with your series with my From/Signed-off-by tag
on it.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [patch v2 1/2] at91_udc.c read_fifo timing issue

[Anti Sullin]

Harro Haan wrote:
 > This solves the read_fifo timing issue for example seen with CDC 
Ethernet.
[...]

Still this bug not fixed?
I sent almost the same fix and a lot of debug info with description of 
the issue and the critical path of the timing to linux-arm-kernel on 
2009-03-25 ( 
http://lists.arm.linux.org.uk/lurker/message/20090325.150843.f515c02f.en.html 
) and we had an off-list discussion with David Brownell about another 
udc patch on 2009-06-19 that later turned out to be the same issue.

This fix has solved the problems on our devices and we've been using it 
for almost a year now.

About Harro's patch:
* why are the marker1 and marker2 comments necessary to be included?
* maybe you should just link to the mailing list and/or move the long 
comment to patch description instead adding a page-long comment to code; 
just leave a small comment to explain why the work-around is needed so 
that nobody would optimize it away?
* Is the error check and warning message necessary after we've got rid
of the problem? Won't it add problems - as I found out, the first csr
read (that comes too early) may return bogus data, you should not use 
its result at all.

-- 
Anti Sullin
Embedded Software Engineer
Artec Design LLC
Teaduspargi 6/2, 12618, Tallinn, Estonia
http://www.artecdesign.ee

[patch v2 1/2] at91_udc.c read_fifo timing issue

[Anti Sullin]

Harro Haan wrote:
 > This solves the read_fifo timing issue for example seen with CDC 
Ethernet.
[...]

Still this bug not fixed?
I sent almost the same fix and a lot of debug info with description of 
the issue and the critical path of the timing to linux-arm-kernel on 
2009-03-25 ( 
http://lists.arm.linux.org.uk/lurker/message/20090325.150843.f515c02f.en.html 
) and we had an off-list discussion with David Brownell about another 
udc patch on 2009-06-19 that later turned out to be the same issue.

This fix has solved the problems on our devices and we've been using it 
for almost a year now.

About Harro's patch:
* why are the marker1 and marker2 comments necessary to be included?
* maybe you should just link to the mailing list and/or move the long 
comment to patch description instead adding a page-long comment to code; 
just leave a small comment to explain why the work-around is needed so 
that nobody would optimize it away?
* Is the error check and warning message necessary after we've got rid
of the problem? Won't it add problems - as I found out, the first csr
read (that comes too early) may return bogus data, you should not use 
its result at all.

-- 
Anti Sullin
Embedded Software Engineer
Artec Design LLC
Teaduspargi 6/2, 12618, Tallinn, Estonia
http://www.artecdesign.ee

[patch v3 0/3] at91_udc: fix soft lockup, HW glitch and use spinlocks

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: avoid-disclaimer-footer
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100203/09cdb3b4/attachment.el>

[patch v3 1/3] Fix soft lockup in at91 udc driver

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: at91-usb-cdc-irq-lockup.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100203/8443a1e4/attachment.el>

[patch v3 2/3] at91_udc HW glitch

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: at91_udc_delay.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100203/08b5b249/attachment.el>

[patch v3 3/3] at91_udc.c use spinlocks instead of local_irq_xxx

[Harro Haan]

An embedded and charset-unspecified text was scrubbed...
Name: at91_udc_use_spinlocks.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100203/3ad44e30/attachment.el>

Re: [patch v3 2/3] at91_udc HW glitch

[Andrew Victor]

hi,

> Add some delay to avoid reading CSR TXCOUNT too early after updating it.
>
> Signed-off-by: Anti Sullin <anti.sullin@artecdesign.ee>
> Signed-off-by: Harro Haan <hrhaan@gmail.com>
> Acked-by: Remy Bohmer <linux@bohmer.net>
> ---
>  drivers/usb/gadget/at91_udc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> Index: linux-2.6.31/drivers/usb/gadget/at91_udc.c
> ===================================================================
> --- linux-2.6.31.orig/drivers/usb/gadget/at91_udc.c
> +++ linux-2.6.31/drivers/usb/gadget/at91_udc.c
> @@ -366,6 +366,13 @@ rescan:
>        if (is_done)
>                done(ep, req, 0);
>        else if (ep->is_pingpong) {
> +               /*
> +                * One dummy read to delay the code because of a HW glitch:
> +                * CSR returns bad RXCOUNT when read too soon after updating
> +                * RX_DATA_BK flags.
> +                */
> +               csr = __raw_readl(creg);
> +
>                bufferspace -= count;
>                buf += count;
>                goto rescan;

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

[patch v3 2/3] at91_udc HW glitch

[Andrew Victor]

hi,

> Add some delay to avoid reading CSR TXCOUNT too early after updating it.
>
> Signed-off-by: Anti Sullin <anti.sullin@artecdesign.ee>
> Signed-off-by: Harro Haan <hrhaan@gmail.com>
> Acked-by: Remy Bohmer <linux@bohmer.net>
> ---
>  drivers/usb/gadget/at91_udc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> Index: linux-2.6.31/drivers/usb/gadget/at91_udc.c
> ===================================================================
> --- linux-2.6.31.orig/drivers/usb/gadget/at91_udc.c
> +++ linux-2.6.31/drivers/usb/gadget/at91_udc.c
> @@ -366,6 +366,13 @@ rescan:
>        if (is_done)
>                done(ep, req, 0);
>        else if (ep->is_pingpong) {
> +               /*
> +                * One dummy read to delay the code because of a HW glitch:
> +                * CSR returns bad RXCOUNT when read too soon after updating
> +                * RX_DATA_BK flags.
> +                */
> +               csr = __raw_readl(creg);
> +
>                bufferspace -= count;
>                buf += count;
>                goto rescan;

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

Re: [patch v3 2/3] at91_udc HW glitch

[Harro Haan]

On 23 March 2010 21:26, Andrew Victor <avictor.za@gmail.com> wrote:
>
> 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

According to the following post it did not solve the problem:

"There are references to syncronization issues between clock domains
in documentation and source code. These references describe required
delays when setting or clearing bits in CSR until the bit actually
changes and the required delays between writing CSR and reading the
data register. Incorporating the delays suggested in datasheet to code
did not change anything, so I dug deeper."
source:
http://lists.arm.linux.org.uk/lurker/message/20090325.150843.f515c02f.en.html

I did not test this myself, maybe Anti Sullin can give more details.

Regards,

Harro Haan

BTW: same message, but now in plain text.

[patch v3 2/3] at91_udc HW glitch

[Harro Haan]

On 23 March 2010 21:26, Andrew Victor <avictor.za@gmail.com> wrote:
>
> 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

According to the following post it did not solve the problem:

"There are references to syncronization issues between clock domains
in documentation and source code. These references describe required
delays when setting or clearing bits in CSR until the bit actually
changes and the required delays between writing CSR and reading the
data register. Incorporating the delays suggested in datasheet to code
did not change anything, so I dug deeper."
source:
http://lists.arm.linux.org.uk/lurker/message/20090325.150843.f515c02f.en.html

I did not test this myself, maybe Anti Sullin can give more details.

Regards,

Harro Haan

BTW: same message, but now in plain text.

Re: [patch v3 2/3] at91_udc HW glitch

[Anti Sullin]

Harro Haan wrote:
> On 23 March 2010 21:26, Andrew Victor <avictor.za@gmail.com> wrote:
>> 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.

Another read is still needed after verifying that the flag is changed.

We are writing a bit that does not have a register behind it. It just
triggers the acknowledge that the data has been read.
We could poll if the corresponding data ready flag is cleared to check
if the write has propagated. But this leads to another problem:
the reads do not seem to be re-syncronized between clock domains.
We just get what is there at the moment the read is performed. This
means that even if the "data ready" bit is cleared, we could not be sure
at that moment that the rest of the changes have been performed
that were triggered by the same write. We even get reads, where
some bits in rx data counter have changed and some bits are old.
So to be fully sure, we should wait until the bit has been cleared
and then perform another read to be sure that the rest of the bits
have been changed as well. See my 2009-03-25 17:08 e-mail for details.

>From practical standpoint: I have not seen a case where the
second read was not ready. If this delay is adequate for the worst
case propagate time it takes for the bits to change, it is not
needed to read the register more than twice. I determined this
experimentally by reading the register 3 times right after write
and comparing the  2nd and 3rd read when doing different transfers
between PC and ARM. As I have tested it only on 9261, somebody should
either run the same kind of tests on other SoC's as well or figure
out the worst case timings on all of them.
The datasheet describes that some changes are performed in
3 USB clock + 3 master clock periods. If so, then one/some extra reads
could create the master-clock dependant small delay needed to
be sure that everything is ready.

Actually, this leads to a another problem. We are able to read the
rx fifo count when the bits are changing there. If some data is being
received at the very moment when we read the register, we're in
trouble. When some bits are old and some new, we can get values that
are larger or smaller than the actual value (ie 0111->1000 change).
This is a rare condition, but it might happen. Should this register
be read always twice to check that nothing was unstable during the
first read?

I would still leave in this extra read because it is known to be
unstable. If it is needed on some SoC's, we could read out the
register value until we get 2 same results to verify that it is
stable. But there is no point of reading the first (known bad) value.

-- 
Anti Sullin
Embedded Software Engineer
Artec Design LLC
Teaduspargi 6/2, 12618, Tallinn, Estonia
http://www.artecdesign.ee 

[patch v3 2/3] at91_udc HW glitch

[Anti Sullin]

Harro Haan wrote:
> On 23 March 2010 21:26, Andrew Victor <avictor.za@gmail.com> wrote:
>> 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.

Another read is still needed after verifying that the flag is changed.

We are writing a bit that does not have a register behind it. It just
triggers the acknowledge that the data has been read.
We could poll if the corresponding data ready flag is cleared to check
if the write has propagated. But this leads to another problem:
the reads do not seem to be re-syncronized between clock domains.
We just get what is there at the moment the read is performed. This
means that even if the "data ready" bit is cleared, we could not be sure
at that moment that the rest of the changes have been performed
that were triggered by the same write. We even get reads, where
some bits in rx data counter have changed and some bits are old.
So to be fully sure, we should wait until the bit has been cleared
and then perform another read to be sure that the rest of the bits
have been changed as well. See my 2009-03-25 17:08 e-mail for details.