[PATCH v1 1/1] usb: ci_udc: don't use "advance" feature when setting address
Ion Agorria
ionpl9 at gmail.com
Fri Oct 18 08:52:15 CEST 2024
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, <mkorpershoek at baylibre.com>
wrote:
> On jeu., oct. 17, 2024 at 16:02, Svyatoslav Ryhel <clamor95 at gmail.com>
> wrote:
>
> > чт, 17 жовт. 2024 р. о 15:21 Mattijs Korpershoek
> > <mkorpershoek at baylibre.com> пише:
> >>
> >> Hi,
> >>
> >> On mer., oct. 16, 2024 at 21:57, Ion Agorria <ionpl9 at 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
> >> > (<mkorpershoek at baylibre.com>) escribió:
> >> >>
> >> >> Hi Svyatoslav,
> >> >>
> >> >> Thank you for the patch.
> >> >>
> >> >> On dim., oct. 13, 2024 at 17:58, Svyatoslav Ryhel <
> clamor95 at gmail.com> wrote:
> >> >>
> >> >> > From: Ion Agorria <ion at 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 at agorria.com>
> >> >> > Signed-off-by: Svyatoslav Ryhel <clamor95 at 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
>
More information about the U-Boot
mailing list