On Tue, 24 Feb 2015 12:25:50 -0700 Stephen Warren <swar...@wwwdotorg.org> wrote:
> On 02/24/2015 10:41 AM, Alban Bedel wrote: > > On Tue, 24 Feb 2015 10:00:43 -0700 > > Stephen Warren <swar...@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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
