Hello, I already finished the fix before looking up and only moved the set address function outside as a func after seeing the linux version.
I think drivers are different enough so there isnt much to compare. Regards, Ion On Thu, 17 Oct 2024, 15:13 Mattijs Korpershoek, <mkorpersh...@baylibre.com> wrote: > On jeu., oct. 17, 2024 at 16:02, Svyatoslav Ryhel <clamo...@gmail.com> > wrote: > > > чт, 17 жовт. 2024 р. о 15:21 Mattijs Korpershoek > > <mkorpersh...@baylibre.com> пише: > >> > >> Hi, > >> > >> On mer., oct. 16, 2024 at 21:57, Ion Agorria <ion...@gmail.com> wrote: > >> > >> > 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. > >> > >> You are right, a single variable is enough. I'd still prefer to have > >> what Linux does because it will make it easier to compare with the Linux > >> driver in the future, > >> > > > > Hello! > > > > Proposed patch already does what Linux changes do. Your request to > > make it "same" as Linux is impossible to achieve since the u-boot > > driver itself does not descend from Linux, nor it is similar to Linux > > one. There is nothing to compare with Linux in the future. > > I'm not saying it should be a verbatim copy of the Linux driver. > I'm saying that keeping similar variable names can help > people comparing both. > > Ion just send previously: > > """ > Yes that's the correct commit I found when researching why the issue > didn't happen in Linux > """ > > So clearly, comparison with the linux driver is valuable given that it > was done for **this very patch**. > > > > > Best regards, > > Svyatoslav R. > > > >> Thanks > >> Mattijs > >> > >> > > >> > 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 >
