чт, 21 лист. 2024 р. о 13:14 Mattijs Korpershoek
<mkorpersh...@baylibre.com> пише:
>
> Hello,
>
> On mer., nov. 20, 2024 at 08:45, Svyatoslav Ryhel <clamo...@gmail.com> wrote:
>
> > нд, 27 жовт. 2024 р. о 18:42 Svyatoslav Ryhel <clamo...@gmail.com> пише:
> >>
> >> нд, 27 жовт. 2024 р. о 18:09 Marek Vasut <ma...@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....@baylibre.com/
Both options are acceptable, it is up to you, what to choose.
