[U-Boot] [PATCH 1/3] usb: ci_udc: Fix set address to work with older controllers

[Alban Bedel]

On Tue, 24 Feb 2015 12:25:50 -0700
Stephen Warren <swarren at wwwdotorg.org> wrote:

> On 02/24/2015 10:41 AM, Alban Bedel wrote:
> > On Tue, 24 Feb 2015 10:00:43 -0700
> > Stephen Warren <swarren at wwwdotorg.org> wrote:
> >
> >> On 02/24/2015 09:44 AM, Alban Bedel wrote:
> >>> Older controllers don't implement "Device Address Advance" which allow
> >>> to pass the device address to the controller when it is received.
> >>> To support such controller we need to store the requested address and
> >>> only apply it after the next IN transfer completed on EP0.
> >>
> >>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> >>
> >>>    	case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS):
> >>> -		/*
> >>> -		 * write address delayed (will take effect
> >>> -		 * after the next IN txn)
> >>> -		 */
> >>> -		writel((r.wValue << 25) | (1 << 24), &udc->devaddr);
> >>> +		/* The device address must be updated after the next IN
> >>> +		 * request completed */
> >>> +		controller.set_address = r.wValue;
> >>
> >> Presumably, bit 24 is the "device address advance" feature?
> >
> > Yes, bit 24 is the "device address advance" feature
> >
> >> I'd prefer it if new controllers used the existing code, but we deferred
> >> the write only for older controllers that don't support "device address
> >> advance". That reduces the possibility of regressions on controller HW
> >> that's already working. Presumably, there is some advantage using the
> >> new feature, rather than deferring the address change manually, e.g. it
> >> solves some race condition?
> >
> > I'm no USB expert, but AFAIU it is only a convenience to make the
> > driver code simpler. I though that having less code path and ifdef
> > would make the whole thing easier to maintain. However if that is
> > preferred I can implement it as you suggested.
> 
> Is there not a race condition?
> 
> 1) USB device controller completes the set address's IN transaction 
> (which I assume is the status stage of a control transaction)
> 
> 2) USB device re-programs address register according to the address that 
> was set
> 
> 3) USB host controller sends a USB transaction to the new address.
> 
> (1) must always happen first, but are (2) and (3) always guaranteed to 
> happen in the desired order? I would have assumed the "auto advance" 
> feature was so that the HW could atomically switch to responding to the 
> new address while it completes the set address transaction, to avoid any 
> window where it doesn't respond to the new address.

There is such a small window, however it is handled by the standard
as the host must wait at least 2 ms after set address, so that shouldn't
be a problem. However I saw that it should also be possible to unset the
address, this is not possible any more with my patch but should be easy
to fix.

> Of course, this is just pure conjecture.

The HW solution is a bit better, but it shouldn't make a difference
with compliant hosts. I would leave it to the maintainer to choose if we
should support both mode or spare some ifdef.

Alban
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150226/3a272424/attachment.sig>