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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi,

On mer., oct. 16, 2024 at 21:57, Ion Agorria <ionpl9@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,

Thanks
Mattijs

>
> Regards,
> Ion Agorria
>
> El mar, 15 oct 2024 a las 11:56, Mattijs Korpershoek
> (<mkorpershoek@baylibre.com>) escribió:
>>
>> 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