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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hello,

On mer., nov. 20, 2024 at 08:45, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> нд, 27 жовт. 2024 р. о 18:42 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>>
>> нд, 27 жовт. 2024 р. о 18:09 Marek Vasut <marex@denx.de> пише:
>> >
>> > On 10/13/24 4:58 PM, Svyatoslav Ryhel wrote:
>> >
>> > Sorry for the late reply.
>> >
>> > > +++ 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);
>> >
>> > log_debug() or dev_dbg() please.
>> >
>>
>> DBG macro is used across entire driver, if you wish to replace it,
>> then pls, send a followup with patch for entire driver. This is out of
>> scope of this patch.
>>
>> > > +     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;
>> >
>> > wValue is word , u16 , but next_device_address is u8 below , why ?
>> >
>>
>> wValue is u16 but only 8 bits are relevant since USB address is 8 bit,
>> hence u8. Changing wValue to u8 is out of scope of this patch as well.
>>
>> > >               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;
>> >
>> > Should this be u16 ?
>>
>> No, USB address is only 8bits (u8)
>
> If no other comments were proposed, may this patch be applied?

Ah, sorry, i've been waiting for a v2 patch that includes a link to the
related kernel commit as asked in [1].

Do you want me to fixup the commit message locally or will a v2 be send?

Thanks!
Mattijs

[1] https://lore.kernel.org/all/87sesxzo2o.fsf@baylibre.com/