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

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

[Alban Bedel]

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.

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 drivers/usb/gadget/ci_udc.c | 18 +++++++++++++-----
 drivers/usb/gadget/ci_udc.h |  1 +
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index b0ef35e..694745f 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -523,6 +523,13 @@ static void handle_ep_complete(struct ci_ep *ci_ep)
 
 	DBG("ept%d %s req %p, complete %x\n",
 	    num, in ? "in" : "out", ci_req, len);
+	/* The device address must be applied after the next IN transfer
+	 * completed on ep0. */
+	if (num == 0 && in && controller.set_address) {
+		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
+		writel(controller.set_address << 25, &udc->devaddr);
+		controller.set_address = 0;
+	}
 	if (num != 0 || controller.ep0_data_phase)
 		ci_req->req.complete(&ci_ep->ep, &ci_req->req);
 	if (num == 0 && controller.ep0_data_phase) {
@@ -614,11 +621,9 @@ static void handle_setup(void)
 		return;
 
 	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;
 		req->length = 0;
 		usb_ep_queue(controller.gadget.ep0, req, 0);
 		return;
@@ -670,6 +675,9 @@ static void stop_activity(void)
 			ci_flush_qh(num);
 		}
 	}
+
+	/* clear any pending set address */
+	controller.set_address = 0;
 }
 
 void udc_irq(void)
diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h
index 346164a..44e70b1 100644
--- a/drivers/usb/gadget/ci_udc.h
+++ b/drivers/usb/gadget/ci_udc.h
@@ -99,6 +99,7 @@ struct ci_drv {
 	struct usb_gadget		gadget;
 	struct ci_req			*ep0_req;
 	bool				ep0_data_phase;
+	uint8_t				set_address;
 	struct usb_gadget_driver	*driver;
 	struct ehci_ctrl		*ctrl;
 	struct ept_queue_head		*epts;
-- 
2.3.0

[U-Boot] [PATCH 2/3] ARM: tegra: Fix the USB gadget configuration for T20

[Alban Bedel]

The USB controller on T20 doesn't have the HOSTPC feature.

As there is not define for the SoC type at this level we use the
address of the first USB controller as match. Not very nice but it
should do for now.

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 include/configs/tegra-common-usb-gadget.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/configs/tegra-common-usb-gadget.h b/include/configs/tegra-common-usb-gadget.h
index 287460c..894b531 100644
--- a/include/configs/tegra-common-usb-gadget.h
+++ b/include/configs/tegra-common-usb-gadget.h
@@ -13,7 +13,9 @@
 #define CONFIG_USB_GADGET
 #define CONFIG_USB_GADGET_VBUS_DRAW    2
 #define CONFIG_CI_UDC
+#if TEGRA_USB1_BASE != 0xC5000000
 #define CONFIG_CI_UDC_HAS_HOSTPC
+#endif
 #define CONFIG_USB_GADGET_DUALSPEED
 #define CONFIG_G_DNL_VENDOR_NUM 0x0955
 #define CONFIG_G_DNL_PRODUCT_NUM 0x701A
-- 
2.3.0

[U-Boot] [PATCH 2/3] ARM: tegra: Fix the USB gadget configuration for T20

[Stephen Warren]

On 02/24/2015 09:44 AM, Alban Bedel wrote:
> The USB controller on T20 doesn't have the HOSTPC feature.
>
> As there is not define for the SoC type at this level we use the
> address of the first USB controller as match. Not very nice but it
> should do for now.

CONFIG_TEGRA20 ought to work fine here?

[U-Boot] [PATCH 2/3] ARM: tegra: Fix the USB gadget configuration for T20

[Alban Bedel]

On Tue, 24 Feb 2015 09:54:31 -0700
Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 02/24/2015 09:44 AM, Alban Bedel wrote:
> > The USB controller on T20 doesn't have the HOSTPC feature.
> >
> > As there is not define for the SoC type at this level we use the
> > address of the first USB controller as match. Not very nice but it
> > should do for now.
> 
> CONFIG_TEGRA20 ought to work fine here?

Right, that was a left over of my early debug, I'll fix it in V2.

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/20150224/482ffc58/attachment.sig>

[U-Boot] [PATCH 3/3] ARM: tegra: usb gadgets: Allow accessing the NAND via DFU

[Alban Bedel]

Many T20 boards use NAND instead of MMC, allow accessing it via DFU.

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 include/configs/tegra-common-usb-gadget.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/configs/tegra-common-usb-gadget.h b/include/configs/tegra-common-usb-gadget.h
index 894b531..3fd8ab4 100644
--- a/include/configs/tegra-common-usb-gadget.h
+++ b/include/configs/tegra-common-usb-gadget.h
@@ -34,6 +34,9 @@
 #ifdef CONFIG_SPI_FLASH
 #define CONFIG_DFU_SF
 #endif
+#ifdef CONFIG_CMD_NAND
+#define CONFIG_DFU_NAND
+#endif
 #endif
 
 #endif /* _TEGRA_COMMON_USB_GADGET_H_ */
-- 
2.3.0

[U-Boot] [PATCH 3/3] ARM: tegra: usb gadgets: Allow accessing the NAND via DFU

[Stephen Warren]

On 02/24/2015 09:44 AM, Alban Bedel wrote:
> Many T20 boards use NAND instead of MMC, allow accessing it via DFU.

I have no particular objection to this, but I wonder how extensively 
you've tested this? IIRC I had to make quite a few fixes to the DFU and 
DFU-MMU code when enabling them on Tegra. I'd expect to have to make a 
bunch of changes to the DFU-NAND code to make it work, unless I happened 
to fix it up blindly without issue before:-)

Does test/dfu/dfu_gadget_test.sh work without issue on NAND on Tegra?

[U-Boot] [PATCH 3/3] ARM: tegra: usb gadgets: Allow accessing the NAND via DFU

[Alban Bedel]

On Tue, 24 Feb 2015 09:56:50 -0700
Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 02/24/2015 09:44 AM, Alban Bedel wrote:
> > Many T20 boards use NAND instead of MMC, allow accessing it via DFU.
> 
> I have no particular objection to this, but I wonder how extensively 
> you've tested this? IIRC I had to make quite a few fixes to the DFU and 
> DFU-MMU code when enabling them on Tegra. I'd expect to have to make a 
> bunch of changes to the DFU-NAND code to make it work, unless I happened 
> to fix it up blindly without issue before:-)

I only did a few test with DFU, writing u-boot image and that worked
fine.

> Does test/dfu/dfu_gadget_test.sh work without issue on NAND on Tegra?

However this test fails :( So, yes it might a bit early to enable it.

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/20150224/5bc36610/attachment.sig>

[U-Boot] [PATCH 3/3] ARM: tegra: usb gadgets: Allow accessing the NAND via DFU

[Stephen Warren]

On 02/24/2015 10:37 AM, Alban Bedel wrote:
> On Tue, 24 Feb 2015 09:56:50 -0700
> Stephen Warren <swarren@wwwdotorg.org> wrote:
>
>> On 02/24/2015 09:44 AM, Alban Bedel wrote:
>>> Many T20 boards use NAND instead of MMC, allow accessing it via DFU.
>>
>> I have no particular objection to this, but I wonder how extensively
>> you've tested this? IIRC I had to make quite a few fixes to the DFU and
>> DFU-MMU code when enabling them on Tegra. I'd expect to have to make a
>> bunch of changes to the DFU-NAND code to make it work, unless I happened
>> to fix it up blindly without issue before:-)
>
> I only did a few test with DFU, writing u-boot image and that worked
> fine.
>
>> Does test/dfu/dfu_gadget_test.sh work without issue on NAND on Tegra?
>
> However this test fails :( So, yes it might a bit early to enable it.

BTW, I should have mentioned that I did find a couple of failures in the 
DFU test that have crept in recently. If all you're seeing is those two, 
you can probably ignore them. See the following for details:

https://www.marc.info/?l=u-boot&m=142420980925216&w=4
Re: [U-Boot] [PATCH v2 0/8] arm: a few steps to reduce the boot time
Message-ID: <54E3B600.60009@wwwdotorg.org>

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

[Stephen Warren]

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?

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?

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

[Alban Bedel]

On Tue, 24 Feb 2015 10:00:43 -0700
Stephen Warren <swarren@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.

Alban

PS: Sorry for the double mail Stephen, I accidentally used the wrong
    reply button.
-------------- 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/20150224/81797b36/attachment.sig>

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

[Stephen Warren]

On 02/24/2015 10:41 AM, Alban Bedel wrote:
> On Tue, 24 Feb 2015 10:00:43 -0700
> Stephen Warren <swarren@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.

Of course, this is just pure conjecture.

[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@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@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>

[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@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@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.