From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Thu, 26 Feb 2015 10:24:09 -0700 Subject: [U-Boot] [PATCH 1/3] usb: ci_udc: Fix set address to work with older controllers In-Reply-To: <20150226180338.69028bce@avionic-0020> References: <1424796293-22746-1-git-send-email-alban.bedel@avionic-design.de> <54ECAE3B.8080208@wwwdotorg.org> <20150224184122.5f8341c3@avionic-0020> <54ECD03E.5010003@wwwdotorg.org> <20150226180338.69028bce@avionic-0020> Message-ID: <54EF56B9.8020805@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 02/26/2015 10:03 AM, Alban Bedel wrote: > On Tue, 24 Feb 2015 12:25:50 -0700 > Stephen Warren wrote: > >> On 02/24/2015 10:41 AM, Alban Bedel wrote: >>> On Tue, 24 Feb 2015 10:00:43 -0700 >>> Stephen Warren 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. Ah. That should certainly be enough time for your modified code to work then. > 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. One data point that would be very useful - if maintainers of some non-Tegra boards that contain SoCs that contain ci_udc could test your changes.