On 02/26/2015 10:03 AM, Alban Bedel wrote:
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.
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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
