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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Svyatoslav,

Thank you for the patch.

On dim., oct. 13, 2024 at 17:58, 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.

Since this is based on a linux commit, can we please mention the
revision in the commit message?

Per my understanding, this would be:
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;

Comparing to
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ef15e5490edc7edf808d3477ab32e0e320792f65,

Can we please keep similar logic ?
Having both:
- bool setaddr
- u8 address

This way, we keep the diff with the linux driver a bit lower.

>  };
>  
>  struct ept_queue_head {
> -- 
> 2.43.0