[PATCH v2 0/1] Fix peripheral USB mode detection

[PATCH v2 0/1] Fix peripheral USB mode detection

[Svyatoslav Ryhel]

In the older USB controllers like for example in ChipIdea controller
used by the Tegra 2 the "USBADRA: Device Address Advance" bitflag
does not exist, so the new device address set during SET_ADDRESS
can't be deferred by hardware, which causes the host to not recognize
the device and give an error.

Instead store it until ep completes to apply the change into the hw
register as Linux kernel does. This should fix regression on old and
and be compatible with newer controllers.

Fixes usb peripheral on Tegra 2 devices.

---
Changes from v1:
- added reference link to linux patch which inspired this change
---

Ion Agorria (1):
  usb: ci_udc: don't use "advance" feature when setting address

 drivers/usb/gadget/ci_udc.c | 24 +++++++++++++++++++++++-
 drivers/usb/gadget/ci_udc.h |  1 +
 2 files changed, 24 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/1] usb: ci_udc: don't use "advance" feature when setting address

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

In the older USB controllers like for example in ChipIdea controller
used by the Tegra 2 the "USBADRA: Device Address Advance" bitflag
does not exist, so the new device address set during SET_ADDRESS
can't be deferred by hardware, which causes the host to not recognize
the device and give an error.

Instead store it until ep completes to apply the change into the hw
register as Linux kernel does. This should fix regression on old and
and be compatible with newer controllers.

Inspired by: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ef15e5490edc7edf808d3477ab32e0e320792f65

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/usb/gadget/ci_udc.c | 24 +++++++++++++++++++++++-
 drivers/usb/gadget/ci_udc.h |  1 +
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index bbe03cfff1f..4bff75da759 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -649,12 +649,30 @@ static void flip_ep0_direction(void)
 	}
 }
 
+/*
+ * This function explicitly sets the address, without the "USBADRA" (advance)
+ * feature, which is not supported by older versions of the controller.
+ */
+static void ci_set_address(struct ci_udc *udc, u8 address)
+{
+	DBG("%s %x\n", __func__, address);
+	writel(address << 25, &udc->devaddr);
+}
+
 static void handle_ep_complete(struct ci_ep *ci_ep)
 {
 	struct ept_queue_item *item, *next_td;
 	int num, in, len, j;
 	struct ci_req *ci_req;
 
+	/* Set the device address that was previously sent by SET_ADDRESS */
+	if (controller.next_device_address != 0) {
+		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
+
+		ci_set_address(udc, controller.next_device_address);
+		controller.next_device_address = 0;
+	}
+
 	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
 	item = ci_get_qtd(num, in);
@@ -783,7 +801,7 @@ static void handle_setup(void)
 		 * write address delayed (will take effect
 		 * after the next IN txn)
 		 */
-		writel((r.wValue << 25) | (1 << 24), &udc->devaddr);
+		controller.next_device_address = r.wValue;
 		req->length = 0;
 		usb_ep_queue(controller.gadget.ep0, req, 0);
 		return;
@@ -814,6 +832,9 @@ static void stop_activity(void)
 	int i, num, in;
 	struct ept_queue_head *head;
 	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
+
+	ci_set_address(udc, 0);
+
 	writel(readl(&udc->epcomp), &udc->epcomp);
 #ifdef CONFIG_CI_UDC_HAS_HOSTPC
 	writel(readl(&udc->epsetupstat), &udc->epsetupstat);
@@ -934,6 +955,7 @@ static int ci_pullup(struct usb_gadget *gadget, int is_on)
 	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
 	if (is_on) {
 		/* RESET */
+		controller.next_device_address = 0;
 		writel(USBCMD_ITC(MICRO_8FRAME) | USBCMD_RST, &udc->usbcmd);
 		udelay(200);
 
diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h
index bea2f9f3fe3..807f2084c1e 100644
--- a/drivers/usb/gadget/ci_udc.h
+++ b/drivers/usb/gadget/ci_udc.h
@@ -105,6 +105,7 @@ struct ci_drv {
 	struct ept_queue_head		*epts;
 	uint8_t				*items_mem;
 	struct ci_ep			ep[NUM_ENDPOINTS];
+	u8				next_device_address;
 };
 
 struct ept_queue_head {
-- 
2.43.0

Re: [PATCH v2 1/1] usb: ci_udc: don't use "advance" feature when setting address

[Mattijs Korpershoek]

Hi Svyatoslav,

Thank you for the patch.

On mar., nov. 26, 2024 at 09:29, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> From: Ion Agorria <ion@agorria.com>
>
> In the older USB controllers like for example in ChipIdea controller
> used by the Tegra 2 the "USBADRA: Device Address Advance" bitflag
> does not exist, so the new device address set during SET_ADDRESS
> can't be deferred by hardware, which causes the host to not recognize
> the device and give an error.
>
> Instead store it until ep completes to apply the change into the hw
> register as Linux kernel does. This should fix regression on old and
> and be compatible with newer controllers.
>
> Inspired by: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ef15e5490edc7edf808d3477ab32e0e320792f65
>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/ci_udc.c | 24 +++++++++++++++++++++++-
>  drivers/usb/gadget/ci_udc.h |  1 +
>  2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index bbe03cfff1f..4bff75da759 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -649,12 +649,30 @@ static void flip_ep0_direction(void)
>  	}
>  }
>  
> +/*
> + * This function explicitly sets the address, without the "USBADRA" (advance)
> + * feature, which is not supported by older versions of the controller.
> + */
> +static void ci_set_address(struct ci_udc *udc, u8 address)
> +{
> +	DBG("%s %x\n", __func__, address);
> +	writel(address << 25, &udc->devaddr);
> +}
> +
>  static void handle_ep_complete(struct ci_ep *ci_ep)
>  {
>  	struct ept_queue_item *item, *next_td;
>  	int num, in, len, j;
>  	struct ci_req *ci_req;
>  
> +	/* Set the device address that was previously sent by SET_ADDRESS */
> +	if (controller.next_device_address != 0) {
> +		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
> +
> +		ci_set_address(udc, controller.next_device_address);
> +		controller.next_device_address = 0;
> +	}
> +
>  	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
>  	in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
>  	item = ci_get_qtd(num, in);
> @@ -783,7 +801,7 @@ static void handle_setup(void)
>  		 * write address delayed (will take effect
>  		 * after the next IN txn)
>  		 */
> -		writel((r.wValue << 25) | (1 << 24), &udc->devaddr);
> +		controller.next_device_address = r.wValue;
>  		req->length = 0;
>  		usb_ep_queue(controller.gadget.ep0, req, 0);
>  		return;
> @@ -814,6 +832,9 @@ static void stop_activity(void)
>  	int i, num, in;
>  	struct ept_queue_head *head;
>  	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
> +
> +	ci_set_address(udc, 0);
> +
>  	writel(readl(&udc->epcomp), &udc->epcomp);
>  #ifdef CONFIG_CI_UDC_HAS_HOSTPC
>  	writel(readl(&udc->epsetupstat), &udc->epsetupstat);
> @@ -934,6 +955,7 @@ static int ci_pullup(struct usb_gadget *gadget, int is_on)
>  	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
>  	if (is_on) {
>  		/* RESET */
> +		controller.next_device_address = 0;
>  		writel(USBCMD_ITC(MICRO_8FRAME) | USBCMD_RST, &udc->usbcmd);
>  		udelay(200);
>  
> diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h
> index bea2f9f3fe3..807f2084c1e 100644
> --- a/drivers/usb/gadget/ci_udc.h
> +++ b/drivers/usb/gadget/ci_udc.h
> @@ -105,6 +105,7 @@ struct ci_drv {
>  	struct ept_queue_head		*epts;
>  	uint8_t				*items_mem;
>  	struct ci_ep			ep[NUM_ENDPOINTS];
> +	u8				next_device_address;
>  };
>  
>  struct ept_queue_head {
> -- 
> 2.43.0

Re: [PATCH v2 0/1] Fix peripheral USB mode detection

[Mattijs Korpershoek]

Hi,

On Tue, 26 Nov 2024 09:29:55 +0200, Svyatoslav Ryhel wrote:
> In the older USB controllers like for example in ChipIdea controller
> used by the Tegra 2 the "USBADRA: Device Address Advance" bitflag
> does not exist, so the new device address set during SET_ADDRESS
> can't be deferred by hardware, which causes the host to not recognize
> the device and give an error.
> 
> Instead store it until ep completes to apply the change into the hw
> register as Linux kernel does. This should fix regression on old and
> and be compatible with newer controllers.
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/1] usb: ci_udc: don't use "advance" feature when setting address
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/35d967f5a8219adc47628247a98c302b1870313e

--
Mattijs