Hello, Yes that's the correct commit I found when researching why the issue didn't happen in Linux, I already found that Tegra 2 reference docs had a different information about the register compared to Tegra 3.
I consider that a single variable is enough since the value is only non-zero when we want to set a new address. But if is necessary i can copy like Linux does. Regards, Ion Agorria El mar, 15 oct 2024 a las 11:56, Mattijs Korpershoek (<mkorpersh...@baylibre.com>) escribió: > > Hi Svyatoslav, > > Thank you for the patch. > > On dim., oct. 13, 2024 at 17:58, Svyatoslav Ryhel <clamo...@gmail.com> wrote: > > > From: Ion Agorria <i...@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 <i...@agorria.com> > > Signed-off-by: Svyatoslav Ryhel <clamo...@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
