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

[Stephen Warren]

On 02/26/2015 10:03 AM, Alban Bedel wrote:
> 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.

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.