[PATCH v1 0/1] Fix peripheral USB mode detection

[PATCH v1 0/1] Fix peripheral USB mode detection

[Svyatoslav Ryhel]

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.

Fixes usb peripheral on Tegra 2 devices.

Ion Agorria (1):
  usb: ci_udc: don't use "advance" feature when setting address

 drivers/usb/gadget/ci_udc.c | 24 +++++++++++++++++++++++-
 drivers/usb/gadget/ci_udc.h |  1 +
 2 files changed, 24 insertions(+), 1 deletion(-)

-- 
2.43.0

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

[Svyatoslav Ryhel]

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.

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;
 };
 
 struct ept_queue_head {
-- 
2.43.0

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

[Mattijs Korpershoek]

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

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

[Ion Agorria]

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.

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

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

[Mattijs Korpershoek]

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

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

[Svyatoslav Ryhel]

чт, 17 жовт. 2024 р. о 15:21 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,
>

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.

Best regards,
Svyatoslav R.

> 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

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

[Mattijs Korpershoek]

On jeu., oct. 17, 2024 at 16:02, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> чт, 17 жовт. 2024 р. о 15:21 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,
>>
>
> 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@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

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

[Ion Agorria]

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@baylibre.com>
wrote:

> On jeu., oct. 17, 2024 at 16:02, Svyatoslav Ryhel <clamor95@gmail.com>
> wrote:
>
> > чт, 17 жовт. 2024 р. о 15:21 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,
> >>
> >
> > 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@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
>

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

[Marek Vasut]

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.

> +	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 ?

>   		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 ?

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

[Svyatoslav Ryhel]

нд, 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)

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

[Svyatoslav Ryhel]

нд, 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?

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

[Mattijs Korpershoek]

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/

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

[Svyatoslav Ryhel]

чт, 21 лист. 2024 р. о 13:14 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/

Both options are acceptable, it is up to you, what to choose.

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

[Mattijs Korpershoek]

Hi,

On jeu., nov. 21, 2024 at 13:15, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

[...]

>> > 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/
>
> Both options are acceptable, it is up to you, what to choose.

Please send a v2 then. Thanks