[PATCH 1/6] usb: gadget: g_dnl: Drop usb_gadget_controller_number()

[PATCH 1/6] usb: gadget: g_dnl: Drop usb_gadget_controller_number()

[Marek Vasut]

The bcdDevice field is defined as
|Device release number in binary-coded decimal
in the USB 2.0 specification. We use this field to distinguish the UDCs
from each other. In theory this could be used on the host side to apply
certain quirks if the "special" UDC in combination with this gadget is
used. This hasn't been done as far as I am aware. In practice it would
be better to fix the UDC driver before shipping since a later release
might not need this quirk anymore.

This patch converts this gadget to use the U-Boot version instead of a
random 2 or 3 plus the UDC number. Linux stopped using this functionality
in 2012, remove it from U-Boot as well.

Matching Linux kernel commit:
ed9cbda63d45 ("usb: gadget: remove usb_gadget_controller_number()")

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/usb/gadget/g_dnl.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
index b5b5f5d8c11..631969b3405 100644
--- a/drivers/usb/gadget/g_dnl.c
+++ b/drivers/usb/gadget/g_dnl.c
@@ -17,10 +17,10 @@
 #include <usb_mass_storage.h>
 #include <dfu.h>
 #include <thor.h>
+#include <version.h>
 
 #include <env_callback.h>
 
-#include "gadget_chips.h"
 #include "composite.c"
 
 /*
@@ -199,18 +199,6 @@ void g_dnl_clear_detach(void)
 	g_dnl_detach_request = false;
 }
 
-static int g_dnl_get_bcd_device_number(struct usb_composite_dev *cdev)
-{
-	struct usb_gadget *gadget = cdev->gadget;
-	int gcnum;
-
-	gcnum = usb_gadget_controller_number(gadget);
-	if (gcnum > 0)
-		gcnum += 0x200;
-
-	return g_dnl_get_board_bcd_device_number(gcnum);
-}
-
 /**
  * Update internal serial number variable when the "serial#" env var changes.
  *
@@ -261,7 +249,8 @@ static int g_dnl_bind(struct usb_composite_dev *cdev)
 	if (ret)
 		goto error;
 
-	gcnum = g_dnl_get_bcd_device_number(cdev);
+	gcnum = g_dnl_get_board_bcd_device_number((U_BOOT_VERSION_NUM << 4) |
+						  U_BOOT_VERSION_NUM_PATCH);
 	if (gcnum >= 0)
 		device_desc.bcdDevice = cpu_to_le16(gcnum);
 	else {
-- 
2.43.0

[PATCH 2/6] usb: gadget: ether: Drop usb_gadget_controller_number()

[Marek Vasut]

The bcdDevice field is defined as
|Device release number in binary-coded decimal
in the USB 2.0 specification. We use this field to distinguish the UDCs
from each other. In theory this could be used on the host side to apply
certain quirks if the "special" UDC in combination with this gadget is
used. This hasn't been done as far as I am aware. In practice it would
be better to fix the UDC driver before shipping since a later release
might not need this quirk anymore.

This patch converts this gadget to use the U-Boot version instead of a
random 2 or 3 plus the UDC number. Linux stopped using this functionality
in 2012, remove it from U-Boot as well.

Matching Linux kernel commit:
ed9cbda63d45 ("usb: gadget: remove usb_gadget_controller_number()")

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/usb/gadget/ether.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index b8b29d399b1..e76464e121b 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -22,8 +22,8 @@
 #include <malloc.h>
 #include <memalign.h>
 #include <linux/ctype.h>
+#include <version.h>
 
-#include "gadget_chips.h"
 #include "rndis.h"
 
 #include <dm.h>
@@ -1998,19 +1998,8 @@ static int eth_bind(struct usb_gadget *gadget)
 		rndis = 0;
 	}
 
-	gcnum = usb_gadget_controller_number(gadget);
-	if (gcnum >= 0)
-		device_desc.bcdDevice = cpu_to_le16(0x0300 + gcnum);
-	else {
-		/*
-		 * can't assume CDC works.  don't want to default to
-		 * anything less functional on CDC-capable hardware,
-		 * so we fail in this case.
-		 */
-		pr_err("controller '%s' not recognized",
-			gadget->name);
-		return -ENODEV;
-	}
+	gcnum = (U_BOOT_VERSION_NUM << 4) | U_BOOT_VERSION_NUM_PATCH;
+	device_desc.bcdDevice = cpu_to_le16(gcnum);
 
 	/*
 	 * If there's an RNDIS configuration, that's what Windows wants to
-- 
2.43.0

Re: [PATCH 2/6] usb: gadget: ether: Drop usb_gadget_controller_number()

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On dim., juin 09, 2024 at 23:32, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> The bcdDevice field is defined as
> |Device release number in binary-coded decimal
> in the USB 2.0 specification. We use this field to distinguish the UDCs
> from each other. In theory this could be used on the host side to apply
> certain quirks if the "special" UDC in combination with this gadget is
> used. This hasn't been done as far as I am aware. In practice it would
> be better to fix the UDC driver before shipping since a later release
> might not need this quirk anymore.
>
> This patch converts this gadget to use the U-Boot version instead of a
> random 2 or 3 plus the UDC number. Linux stopped using this functionality
> in 2012, remove it from U-Boot as well.
>
> Matching Linux kernel commit:
> ed9cbda63d45 ("usb: gadget: remove usb_gadget_controller_number()")
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/gadget/ether.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> index b8b29d399b1..e76464e121b 100644
> --- a/drivers/usb/gadget/ether.c
> +++ b/drivers/usb/gadget/ether.c
> @@ -22,8 +22,8 @@
>  #include <malloc.h>
>  #include <memalign.h>
>  #include <linux/ctype.h>
> +#include <version.h>
>  
> -#include "gadget_chips.h"
>  #include "rndis.h"
>  
>  #include <dm.h>
> @@ -1998,19 +1998,8 @@ static int eth_bind(struct usb_gadget *gadget)
>  		rndis = 0;
>  	}
>  
> -	gcnum = usb_gadget_controller_number(gadget);
> -	if (gcnum >= 0)
> -		device_desc.bcdDevice = cpu_to_le16(0x0300 + gcnum);
> -	else {
> -		/*
> -		 * can't assume CDC works.  don't want to default to
> -		 * anything less functional on CDC-capable hardware,
> -		 * so we fail in this case.
> -		 */
> -		pr_err("controller '%s' not recognized",
> -			gadget->name);
> -		return -ENODEV;
> -	}
> +	gcnum = (U_BOOT_VERSION_NUM << 4) | U_BOOT_VERSION_NUM_PATCH;
> +	device_desc.bcdDevice = cpu_to_le16(gcnum);
>  
>  	/*
>  	 * If there's an RNDIS configuration, that's what Windows wants to
> -- 
> 2.43.0

[PATCH 3/6] usb: gadget: Drop usb_gadget_controller_number()

[Marek Vasut]

The bcdDevice field is defined as
|Device release number in binary-coded decimal
in the USB 2.0 specification. We use this field to distinguish the UDCs
from each other. In theory this could be used on the host side to apply
certain quirks if the "special" UDC in combination with this gadget is
used. This hasn't been done as far as I am aware. In practice it would
be better to fix the UDC driver before shipping since a later release
might not need this quirk anymore.

This patch removes the newly unused function. Linux stopped using this
functionality in 2012, remove it from U-Boot as well.

Matching Linux kernel commit:
ed9cbda63d45 ("usb: gadget: remove usb_gadget_controller_number()")

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/usb/gadget/gadget_chips.h | 62 -------------------------------
 1 file changed, 62 deletions(-)

diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
index 98156c312d2..316051686c4 100644
--- a/drivers/usb/gadget/gadget_chips.h
+++ b/drivers/usb/gadget/gadget_chips.h
@@ -146,65 +146,3 @@
 #else
 #define gadget_is_dwc2(g)        0
 #endif
-
-/**
- * usb_gadget_controller_number - support bcdDevice id convention
- * @gadget: the controller being driven
- *
- * Return a 2-digit BCD value associated with the peripheral controller,
- * suitable for use as part of a bcdDevice value, or a negative error code.
- *
- * NOTE:  this convention is purely optional, and has no meaning in terms of
- * any USB specification.  If you want to use a different convention in your
- * gadget driver firmware -- maybe a more formal revision ID -- feel free.
- *
- * Hosts see these bcdDevice numbers, and are allowed (but not encouraged!)
- * to change their behavior accordingly.  For example it might help avoiding
- * some chip bug.
- */
-static inline int usb_gadget_controller_number(struct usb_gadget *gadget)
-{
-	if (gadget_is_net2280(gadget))
-		return 0x01;
-	else if (gadget_is_dummy(gadget))
-		return 0x02;
-	else if (gadget_is_sh(gadget))
-		return 0x04;
-	else if (gadget_is_goku(gadget))
-		return 0x06;
-	else if (gadget_is_mq11xx(gadget))
-		return 0x07;
-	else if (gadget_is_omap(gadget))
-		return 0x08;
-	else if (gadget_is_n9604(gadget))
-		return 0x09;
-	else if (gadget_is_at91(gadget))
-		return 0x12;
-	else if (gadget_is_imx(gadget))
-		return 0x13;
-	else if (gadget_is_musbhsfc(gadget))
-		return 0x14;
-	else if (gadget_is_musbhdrc(gadget))
-		return 0x15;
-	else if (gadget_is_atmel_usba(gadget))
-		return 0x17;
-	else if (gadget_is_fsl_usb2(gadget))
-		return 0x18;
-	else if (gadget_is_amd5536udc(gadget))
-		return 0x19;
-	else if (gadget_is_m66592(gadget))
-		return 0x20;
-	else if (gadget_is_ci(gadget))
-		return 0x21;
-	else if (gadget_is_dwc3(gadget))
-		return 0x23;
-	else if (gadget_is_cdns3(gadget))
-		return 0x24;
-	else if (gadget_is_max3420(gadget))
-		return 0x25;
-	else if (gadget_is_mtu3(gadget))
-		return 0x26;
-	else if (gadget_is_dwc2(gadget))
-		return 0x27;
-	return -ENOENT;
-}
-- 
2.43.0

Re: [PATCH 3/6] usb: gadget: Drop usb_gadget_controller_number()

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On dim., juin 09, 2024 at 23:32, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> The bcdDevice field is defined as
> |Device release number in binary-coded decimal
> in the USB 2.0 specification. We use this field to distinguish the UDCs
> from each other. In theory this could be used on the host side to apply
> certain quirks if the "special" UDC in combination with this gadget is
> used. This hasn't been done as far as I am aware. In practice it would
> be better to fix the UDC driver before shipping since a later release
> might not need this quirk anymore.
>
> This patch removes the newly unused function. Linux stopped using this
> functionality in 2012, remove it from U-Boot as well.
>
> Matching Linux kernel commit:
> ed9cbda63d45 ("usb: gadget: remove usb_gadget_controller_number()")
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/gadget/gadget_chips.h | 62 -------------------------------
>  1 file changed, 62 deletions(-)
>
> diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
> index 98156c312d2..316051686c4 100644
> --- a/drivers/usb/gadget/gadget_chips.h
> +++ b/drivers/usb/gadget/gadget_chips.h
> @@ -146,65 +146,3 @@
>  #else
>  #define gadget_is_dwc2(g)        0
>  #endif
> -
> -/**
> - * usb_gadget_controller_number - support bcdDevice id convention
> - * @gadget: the controller being driven
> - *
> - * Return a 2-digit BCD value associated with the peripheral controller,
> - * suitable for use as part of a bcdDevice value, or a negative error code.
> - *
> - * NOTE:  this convention is purely optional, and has no meaning in terms of
> - * any USB specification.  If you want to use a different convention in your
> - * gadget driver firmware -- maybe a more formal revision ID -- feel free.
> - *
> - * Hosts see these bcdDevice numbers, and are allowed (but not encouraged!)
> - * to change their behavior accordingly.  For example it might help avoiding
> - * some chip bug.
> - */
> -static inline int usb_gadget_controller_number(struct usb_gadget *gadget)
> -{
> -	if (gadget_is_net2280(gadget))
> -		return 0x01;
> -	else if (gadget_is_dummy(gadget))
> -		return 0x02;
> -	else if (gadget_is_sh(gadget))
> -		return 0x04;
> -	else if (gadget_is_goku(gadget))
> -		return 0x06;
> -	else if (gadget_is_mq11xx(gadget))
> -		return 0x07;
> -	else if (gadget_is_omap(gadget))
> -		return 0x08;
> -	else if (gadget_is_n9604(gadget))
> -		return 0x09;
> -	else if (gadget_is_at91(gadget))
> -		return 0x12;
> -	else if (gadget_is_imx(gadget))
> -		return 0x13;
> -	else if (gadget_is_musbhsfc(gadget))
> -		return 0x14;
> -	else if (gadget_is_musbhdrc(gadget))
> -		return 0x15;
> -	else if (gadget_is_atmel_usba(gadget))
> -		return 0x17;
> -	else if (gadget_is_fsl_usb2(gadget))
> -		return 0x18;
> -	else if (gadget_is_amd5536udc(gadget))
> -		return 0x19;
> -	else if (gadget_is_m66592(gadget))
> -		return 0x20;
> -	else if (gadget_is_ci(gadget))
> -		return 0x21;
> -	else if (gadget_is_dwc3(gadget))
> -		return 0x23;
> -	else if (gadget_is_cdns3(gadget))
> -		return 0x24;
> -	else if (gadget_is_max3420(gadget))
> -		return 0x25;
> -	else if (gadget_is_mtu3(gadget))
> -		return 0x26;
> -	else if (gadget_is_dwc2(gadget))
> -		return 0x27;
> -	return -ENOENT;
> -}
> -- 
> 2.43.0

Re: [PATCH 3/6] usb: gadget: Drop usb_gadget_controller_number()

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4696 bytes --]

On Tue, 11 Jun 2024 09:20:33 +0200
Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

> Hi Marek,
> 
> Thank you for the patch.
> 
> On dim., juin 09, 2024 at 23:32, Marek Vasut
> <marek.vasut+renesas@mailbox.org> wrote:
> 
> > The bcdDevice field is defined as
> > |Device release number in binary-coded decimal
> > in the USB 2.0 specification. We use this field to distinguish the
> > UDCs from each other. In theory this could be used on the host side
> > to apply certain quirks if the "special" UDC in combination with
> > this gadget is used. This hasn't been done as far as I am aware. In
> > practice it would be better to fix the UDC driver before shipping
> > since a later release might not need this quirk anymore.
> >
> > This patch removes the newly unused function. Linux stopped using
> > this functionality in 2012, remove it from U-Boot as well.
> >
> > Matching Linux kernel commit:
> > ed9cbda63d45 ("usb: gadget: remove usb_gadget_controller_number()")
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>  
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3
> 
> > ---
> > Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> > Cc: Lukasz Majewski <lukma@denx.de>
> > Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> > Cc: Nishanth Menon <nm@ti.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: u-boot@lists.denx.de
> > ---
> >  drivers/usb/gadget/gadget_chips.h | 62
> > ------------------------------- 1 file changed, 62 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/gadget_chips.h
> > b/drivers/usb/gadget/gadget_chips.h index 98156c312d2..316051686c4
> > 100644 --- a/drivers/usb/gadget/gadget_chips.h
> > +++ b/drivers/usb/gadget/gadget_chips.h
> > @@ -146,65 +146,3 @@
> >  #else
> >  #define gadget_is_dwc2(g)        0
> >  #endif
> > -
> > -/**
> > - * usb_gadget_controller_number - support bcdDevice id convention
> > - * @gadget: the controller being driven
> > - *
> > - * Return a 2-digit BCD value associated with the peripheral
> > controller,
> > - * suitable for use as part of a bcdDevice value, or a negative
> > error code.
> > - *
> > - * NOTE:  this convention is purely optional, and has no meaning
> > in terms of
> > - * any USB specification.  If you want to use a different
> > convention in your
> > - * gadget driver firmware -- maybe a more formal revision ID --
> > feel free.
> > - *
> > - * Hosts see these bcdDevice numbers, and are allowed (but not
> > encouraged!)
> > - * to change their behavior accordingly.  For example it might
> > help avoiding
> > - * some chip bug.
> > - */
> > -static inline int usb_gadget_controller_number(struct usb_gadget
> > *gadget) -{
> > -	if (gadget_is_net2280(gadget))
> > -		return 0x01;
> > -	else if (gadget_is_dummy(gadget))
> > -		return 0x02;
> > -	else if (gadget_is_sh(gadget))
> > -		return 0x04;
> > -	else if (gadget_is_goku(gadget))
> > -		return 0x06;
> > -	else if (gadget_is_mq11xx(gadget))
> > -		return 0x07;
> > -	else if (gadget_is_omap(gadget))
> > -		return 0x08;
> > -	else if (gadget_is_n9604(gadget))
> > -		return 0x09;
> > -	else if (gadget_is_at91(gadget))
> > -		return 0x12;
> > -	else if (gadget_is_imx(gadget))
> > -		return 0x13;
> > -	else if (gadget_is_musbhsfc(gadget))
> > -		return 0x14;
> > -	else if (gadget_is_musbhdrc(gadget))
> > -		return 0x15;
> > -	else if (gadget_is_atmel_usba(gadget))
> > -		return 0x17;
> > -	else if (gadget_is_fsl_usb2(gadget))
> > -		return 0x18;
> > -	else if (gadget_is_amd5536udc(gadget))
> > -		return 0x19;
> > -	else if (gadget_is_m66592(gadget))
> > -		return 0x20;
> > -	else if (gadget_is_ci(gadget))
> > -		return 0x21;
> > -	else if (gadget_is_dwc3(gadget))
> > -		return 0x23;
> > -	else if (gadget_is_cdns3(gadget))
> > -		return 0x24;
> > -	else if (gadget_is_max3420(gadget))
> > -		return 0x25;
> > -	else if (gadget_is_mtu3(gadget))
> > -		return 0x26;
> > -	else if (gadget_is_dwc2(gadget))
> > -		return 0x27;
> > -	return -ENOENT;
> > -}
> > -- 
> > 2.43.0  

FInally..... :-)

Thanks Mattijs for this cleanup.

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/6] usb: gadget: Drop usb_gadget_controller_number()

[Mattijs Korpershoek]

Hi Lukasz,

On mar., juin 11, 2024 at 10:51, Lukasz Majewski <lukma@denx.de> wrote:

> On Tue, 11 Jun 2024 09:20:33 +0200
> Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

[...]

>
>> > -- 
>> > 2.43.0  
>
> FInally..... :-)
>
> Thanks Mattijs for this cleanup.

You should thank Marek, i've just reviewed his work :)

>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[PATCH 4/6] usb: gadget: Drop all gadget_is_*() functions

[Marek Vasut]

The only actually used gadget_is_*() functions are the one for DWC3
used in epautoconf.c usb_ep_autoconfig() and one for MUSB in ether.c.
The DWC3 one should be fixed in some separate patch.

Inline the gadget_is_dwc3() and stop using ifdefs in favor of
IS_ENABLED() macro.

The rest of gadget_is_*() calls in usb_ep_autoconfig() can never
be anything but 0, since those gadgets are not supported in U-Boot,
so remove all that unused code. Remove gadget_chips.h as well.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/usb/gadget/epautoconf.c   |  40 +-------
 drivers/usb/gadget/ether.c        |   8 +-
 drivers/usb/gadget/gadget_chips.h | 148 ------------------------------
 3 files changed, 6 insertions(+), 190 deletions(-)
 delete mode 100644 drivers/usb/gadget/gadget_chips.h

diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 0a70035ce04..09950ceeaed 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -12,7 +12,6 @@
 #include <linux/errno.h>
 #include <linux/usb/gadget.h>
 #include <asm/unaligned.h>
-#include "gadget_chips.h"
 
 #define isdigit(c)      ('0' <= (c) && (c) <= '9')
 
@@ -222,41 +221,9 @@ struct usb_ep *usb_ep_autoconfig(
 	/* First, apply chip-specific "best usage" knowledge.
 	 * This might make a good usb_gadget_ops hook ...
 	 */
-	if (gadget_is_net2280(gadget) && type == USB_ENDPOINT_XFER_INT) {
-		/* ep-e, ep-f are PIO with only 64 byte fifos */
-		ep = find_ep(gadget, "ep-e");
-		if (ep && ep_matches(gadget, ep, desc))
-			return ep;
-		ep = find_ep(gadget, "ep-f");
-		if (ep && ep_matches(gadget, ep, desc))
-			return ep;
-
-	} else if (gadget_is_goku(gadget)) {
-		if (USB_ENDPOINT_XFER_INT == type) {
-			/* single buffering is enough */
-			ep = find_ep(gadget, "ep3-bulk");
-			if (ep && ep_matches(gadget, ep, desc))
-				return ep;
-		} else if (USB_ENDPOINT_XFER_BULK == type
-				&& (USB_DIR_IN & desc->bEndpointAddress)) {
-			/* DMA may be available */
-			ep = find_ep(gadget, "ep2-bulk");
-			if (ep && ep_matches(gadget, ep, desc))
-				return ep;
-		}
-
-	} else if (gadget_is_sh(gadget) && USB_ENDPOINT_XFER_INT == type) {
-		/* single buffering is enough; maybe 8 byte fifo is too */
-		ep = find_ep(gadget, "ep3in-bulk");
-		if (ep && ep_matches(gadget, ep, desc))
-			return ep;
-
-	} else if (gadget_is_mq11xx(gadget) && USB_ENDPOINT_XFER_INT == type) {
-		ep = find_ep(gadget, "ep1-bulk");
-		if (ep && ep_matches(gadget, ep, desc))
-			return ep;
-#ifndef CONFIG_SPL_BUILD
-	} else if (gadget_is_dwc3(gadget)) {
+	if (!IS_ENABLED(CONFIG_SPL_BUILD) &&
+	    IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
+	    !strcmp("dwc3-gadget", gadget->name)) {
 		const char *name = NULL;
 		/*
 		 * First try standard, common configuration: ep1in-bulk,
@@ -278,7 +245,6 @@ struct usb_ep *usb_ep_autoconfig(
 			ep = find_ep(gadget, name);
 		if (ep && ep_matches(gadget, ep, desc))
 			return ep;
-#endif
 	}
 
 	if (gadget->ops->match_ep)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index e76464e121b..b7b7bacb00d 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -1989,13 +1989,11 @@ static int eth_bind(struct usb_gadget *gadget)
 	 * standard protocol is _strongly_ preferred for interop purposes.
 	 * (By everyone except Microsoft.)
 	 */
-	if (gadget_is_musbhdrc(gadget)) {
+
+	if (IS_ENABLED(CONFIG_USB_MUSB_GADGET) &&
+	    !strcmp("musb-hdrc", gadget->name)) {
 		/* reduce tx dma overhead by avoiding special cases */
 		zlp = 0;
-	} else if (gadget_is_sh(gadget)) {
-		/* sh doesn't support multiple interfaces or configs */
-		cdc = 0;
-		rndis = 0;
 	}
 
 	gcnum = (U_BOOT_VERSION_NUM << 4) | U_BOOT_VERSION_NUM_PATCH;
diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
deleted file mode 100644
index 316051686c4..00000000000
--- a/drivers/usb/gadget/gadget_chips.h
+++ /dev/null
@@ -1,148 +0,0 @@
-/*
- * USB device controllers have lots of quirks.  Use these macros in
- * gadget drivers or other code that needs to deal with them, and which
- * autoconfigures instead of using early binding to the hardware.
- *
- * This SHOULD eventually work like the ARM mach_is_*() stuff, driven by
- * some config file that gets updated as new hardware is supported.
- * (And avoiding all runtime comparisons in typical one-choice configs!)
- *
- * NOTE:  some of these controller drivers may not be available yet.
- * Some are available on 2.4 kernels; several are available, but not
- * yet pushed in the 2.6 mainline tree.
- *
- * Ported to U-Boot by: Thomas Smits <ts.smits@gmail.com> and
- *                      Remy Bohmer <linux@bohmer.net>
- */
-#ifdef CONFIG_USB_GADGET_NET2280
-#define	gadget_is_net2280(g)	(!strcmp("net2280", (g)->name))
-#else
-#define	gadget_is_net2280(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_AMD5536UDC
-#define	gadget_is_amd5536udc(g)	(!strcmp("amd5536udc", (g)->name))
-#else
-#define	gadget_is_amd5536udc(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_DUMMY_HCD
-#define	gadget_is_dummy(g)	(!strcmp("dummy_udc", (g)->name))
-#else
-#define	gadget_is_dummy(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_GOKU
-#define	gadget_is_goku(g)	(!strcmp("goku_udc", (g)->name))
-#else
-#define	gadget_is_goku(g)	0
-#endif
-
-/* SH3 UDC -- not yet ported 2.4 --> 2.6 */
-#ifdef CONFIG_USB_GADGET_SUPERH
-#define	gadget_is_sh(g)		(!strcmp("sh_udc", (g)->name))
-#else
-#define	gadget_is_sh(g)		0
-#endif
-
-/* handhelds.org tree (?) */
-#ifdef CONFIG_USB_GADGET_MQ11XX
-#define	gadget_is_mq11xx(g)	(!strcmp("mq11xx_udc", (g)->name))
-#else
-#define	gadget_is_mq11xx(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_OMAP
-#define	gadget_is_omap(g)	(!strcmp("omap_udc", (g)->name))
-#else
-#define	gadget_is_omap(g)	0
-#endif
-
-/* not yet ported 2.4 --> 2.6 */
-#ifdef CONFIG_USB_GADGET_N9604
-#define	gadget_is_n9604(g)	(!strcmp("n9604_udc", (g)->name))
-#else
-#define	gadget_is_n9604(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_ATMEL_USBA
-#define gadget_is_atmel_usba(g)	(!strcmp("atmel_usba_udc", (g)->name))
-#else
-#define gadget_is_atmel_usba(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_AT91
-#define gadget_is_at91(g)	(!strcmp("at91_udc", (g)->name))
-#else
-#define gadget_is_at91(g)	0
-#endif
-
-/* status unclear */
-#ifdef CONFIG_USB_GADGET_IMX
-#define gadget_is_imx(g)	(!strcmp("imx_udc", (g)->name))
-#else
-#define gadget_is_imx(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_FSL_USB2
-#define gadget_is_fsl_usb2(g)	(!strcmp("fsl-usb2-udc", (g)->name))
-#else
-#define gadget_is_fsl_usb2(g)	0
-#endif
-
-/* Mentor high speed function controller */
-/* from Montavista kernel (?) */
-#ifdef CONFIG_USB_GADGET_MUSBHSFC
-#define gadget_is_musbhsfc(g)	(!strcmp("musbhsfc_udc", (g)->name))
-#else
-#define gadget_is_musbhsfc(g)	0
-#endif
-
-/* Mentor high speed "dual role" controller, in peripheral role */
-#ifdef CONFIG_USB_MUSB_GADGET
-#define gadget_is_musbhdrc(g)	(!strcmp("musb-hdrc", (g)->name))
-#else
-#define gadget_is_musbhdrc(g)	0
-#endif
-
-#ifdef CONFIG_USB_GADGET_M66592
-#define	gadget_is_m66592(g)	(!strcmp("m66592_udc", (g)->name))
-#else
-#define	gadget_is_m66592(g)	0
-#endif
-
-#ifdef CONFIG_CI_UDC
-#define gadget_is_ci(g)        (!strcmp("ci_udc", (g)->name))
-#else
-#define gadget_is_ci(g)        0
-#endif
-
-#ifdef CONFIG_USB_DWC3_GADGET
-#define gadget_is_dwc3(g)        (!strcmp("dwc3-gadget", (g)->name))
-#else
-#define gadget_is_dwc3(g)        0
-#endif
-
-#ifdef CONFIG_USB_CDNS3_GADGET
-#define gadget_is_cdns3(g)        (!strcmp("cdns3-gadget", (g)->name))
-#else
-#define gadget_is_cdns3(g)        0
-#endif
-
-#ifdef CONFIG_USB_GADGET_MAX3420
-#define gadget_is_max3420(g)        (!strcmp("max3420-udc", (g)->name))
-#else
-#define gadget_is_max3420(g)        0
-#endif
-
-#ifdef CONFIG_USB_MTU3_GADGET
-#define gadget_is_mtu3(g)        (!strcmp("mtu3-gadget", (g)->name))
-#else
-#define gadget_is_mtu3(g)        0
-#endif
-
-#ifdef CONFIG_USB_GADGET_DWC2_OTG
-#define gadget_is_dwc2(g)        (!strcmp("dwc2-udc", (g)->name))
-#else
-#define gadget_is_dwc2(g)        0
-#endif
-- 
2.43.0

Re: [PATCH 4/6] usb: gadget: Drop all gadget_is_*() functions

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On dim., juin 09, 2024 at 23:32, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> The only actually used gadget_is_*() functions are the one for DWC3
> used in epautoconf.c usb_ep_autoconfig() and one for MUSB in ether.c.
> The DWC3 one should be fixed in some separate patch.
>
> Inline the gadget_is_dwc3() and stop using ifdefs in favor of
> IS_ENABLED() macro.
>
> The rest of gadget_is_*() calls in usb_ep_autoconfig() can never
> be anything but 0, since those gadgets are not supported in U-Boot,
> so remove all that unused code. Remove gadget_chips.h as well.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/gadget/epautoconf.c   |  40 +-------
>  drivers/usb/gadget/ether.c        |   8 +-
>  drivers/usb/gadget/gadget_chips.h | 148 ------------------------------
>  3 files changed, 6 insertions(+), 190 deletions(-)
>  delete mode 100644 drivers/usb/gadget/gadget_chips.h
>
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 0a70035ce04..09950ceeaed 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -12,7 +12,6 @@
>  #include <linux/errno.h>
>  #include <linux/usb/gadget.h>
>  #include <asm/unaligned.h>
> -#include "gadget_chips.h"
>  
>  #define isdigit(c)      ('0' <= (c) && (c) <= '9')
>  
> @@ -222,41 +221,9 @@ struct usb_ep *usb_ep_autoconfig(
>  	/* First, apply chip-specific "best usage" knowledge.
>  	 * This might make a good usb_gadget_ops hook ...
>  	 */
> -	if (gadget_is_net2280(gadget) && type == USB_ENDPOINT_XFER_INT) {
> -		/* ep-e, ep-f are PIO with only 64 byte fifos */
> -		ep = find_ep(gadget, "ep-e");
> -		if (ep && ep_matches(gadget, ep, desc))
> -			return ep;
> -		ep = find_ep(gadget, "ep-f");
> -		if (ep && ep_matches(gadget, ep, desc))
> -			return ep;
> -
> -	} else if (gadget_is_goku(gadget)) {
> -		if (USB_ENDPOINT_XFER_INT == type) {
> -			/* single buffering is enough */
> -			ep = find_ep(gadget, "ep3-bulk");
> -			if (ep && ep_matches(gadget, ep, desc))
> -				return ep;
> -		} else if (USB_ENDPOINT_XFER_BULK == type
> -				&& (USB_DIR_IN & desc->bEndpointAddress)) {
> -			/* DMA may be available */
> -			ep = find_ep(gadget, "ep2-bulk");
> -			if (ep && ep_matches(gadget, ep, desc))
> -				return ep;
> -		}
> -
> -	} else if (gadget_is_sh(gadget) && USB_ENDPOINT_XFER_INT == type) {
> -		/* single buffering is enough; maybe 8 byte fifo is too */
> -		ep = find_ep(gadget, "ep3in-bulk");
> -		if (ep && ep_matches(gadget, ep, desc))
> -			return ep;
> -
> -	} else if (gadget_is_mq11xx(gadget) && USB_ENDPOINT_XFER_INT == type) {
> -		ep = find_ep(gadget, "ep1-bulk");
> -		if (ep && ep_matches(gadget, ep, desc))
> -			return ep;
> -#ifndef CONFIG_SPL_BUILD
> -	} else if (gadget_is_dwc3(gadget)) {
> +	if (!IS_ENABLED(CONFIG_SPL_BUILD) &&
> +	    IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
> +	    !strcmp("dwc3-gadget", gadget->name)) {
>  		const char *name = NULL;
>  		/*
>  		 * First try standard, common configuration: ep1in-bulk,
> @@ -278,7 +245,6 @@ struct usb_ep *usb_ep_autoconfig(
>  			ep = find_ep(gadget, name);
>  		if (ep && ep_matches(gadget, ep, desc))
>  			return ep;
> -#endif
>  	}
>  
>  	if (gadget->ops->match_ep)
> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> index e76464e121b..b7b7bacb00d 100644
> --- a/drivers/usb/gadget/ether.c
> +++ b/drivers/usb/gadget/ether.c
> @@ -1989,13 +1989,11 @@ static int eth_bind(struct usb_gadget *gadget)
>  	 * standard protocol is _strongly_ preferred for interop purposes.
>  	 * (By everyone except Microsoft.)
>  	 */
> -	if (gadget_is_musbhdrc(gadget)) {
> +
> +	if (IS_ENABLED(CONFIG_USB_MUSB_GADGET) &&
> +	    !strcmp("musb-hdrc", gadget->name)) {
>  		/* reduce tx dma overhead by avoiding special cases */
>  		zlp = 0;
> -	} else if (gadget_is_sh(gadget)) {
> -		/* sh doesn't support multiple interfaces or configs */
> -		cdc = 0;
> -		rndis = 0;
>  	}
>  
>  	gcnum = (U_BOOT_VERSION_NUM << 4) | U_BOOT_VERSION_NUM_PATCH;
> diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
> deleted file mode 100644
> index 316051686c4..00000000000
> --- a/drivers/usb/gadget/gadget_chips.h
> +++ /dev/null
> @@ -1,148 +0,0 @@
> -/*
> - * USB device controllers have lots of quirks.  Use these macros in
> - * gadget drivers or other code that needs to deal with them, and which
> - * autoconfigures instead of using early binding to the hardware.
> - *
> - * This SHOULD eventually work like the ARM mach_is_*() stuff, driven by
> - * some config file that gets updated as new hardware is supported.
> - * (And avoiding all runtime comparisons in typical one-choice configs!)
> - *
> - * NOTE:  some of these controller drivers may not be available yet.
> - * Some are available on 2.4 kernels; several are available, but not
> - * yet pushed in the 2.6 mainline tree.
> - *
> - * Ported to U-Boot by: Thomas Smits <ts.smits@gmail.com> and
> - *                      Remy Bohmer <linux@bohmer.net>
> - */
> -#ifdef CONFIG_USB_GADGET_NET2280
> -#define	gadget_is_net2280(g)	(!strcmp("net2280", (g)->name))
> -#else
> -#define	gadget_is_net2280(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_AMD5536UDC
> -#define	gadget_is_amd5536udc(g)	(!strcmp("amd5536udc", (g)->name))
> -#else
> -#define	gadget_is_amd5536udc(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_DUMMY_HCD
> -#define	gadget_is_dummy(g)	(!strcmp("dummy_udc", (g)->name))
> -#else
> -#define	gadget_is_dummy(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_GOKU
> -#define	gadget_is_goku(g)	(!strcmp("goku_udc", (g)->name))
> -#else
> -#define	gadget_is_goku(g)	0
> -#endif
> -
> -/* SH3 UDC -- not yet ported 2.4 --> 2.6 */
> -#ifdef CONFIG_USB_GADGET_SUPERH
> -#define	gadget_is_sh(g)		(!strcmp("sh_udc", (g)->name))
> -#else
> -#define	gadget_is_sh(g)		0
> -#endif
> -
> -/* handhelds.org tree (?) */
> -#ifdef CONFIG_USB_GADGET_MQ11XX
> -#define	gadget_is_mq11xx(g)	(!strcmp("mq11xx_udc", (g)->name))
> -#else
> -#define	gadget_is_mq11xx(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_OMAP
> -#define	gadget_is_omap(g)	(!strcmp("omap_udc", (g)->name))
> -#else
> -#define	gadget_is_omap(g)	0
> -#endif
> -
> -/* not yet ported 2.4 --> 2.6 */
> -#ifdef CONFIG_USB_GADGET_N9604
> -#define	gadget_is_n9604(g)	(!strcmp("n9604_udc", (g)->name))
> -#else
> -#define	gadget_is_n9604(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_ATMEL_USBA
> -#define gadget_is_atmel_usba(g)	(!strcmp("atmel_usba_udc", (g)->name))
> -#else
> -#define gadget_is_atmel_usba(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_AT91
> -#define gadget_is_at91(g)	(!strcmp("at91_udc", (g)->name))
> -#else
> -#define gadget_is_at91(g)	0
> -#endif
> -
> -/* status unclear */
> -#ifdef CONFIG_USB_GADGET_IMX
> -#define gadget_is_imx(g)	(!strcmp("imx_udc", (g)->name))
> -#else
> -#define gadget_is_imx(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_FSL_USB2
> -#define gadget_is_fsl_usb2(g)	(!strcmp("fsl-usb2-udc", (g)->name))
> -#else
> -#define gadget_is_fsl_usb2(g)	0
> -#endif
> -
> -/* Mentor high speed function controller */
> -/* from Montavista kernel (?) */
> -#ifdef CONFIG_USB_GADGET_MUSBHSFC
> -#define gadget_is_musbhsfc(g)	(!strcmp("musbhsfc_udc", (g)->name))
> -#else
> -#define gadget_is_musbhsfc(g)	0
> -#endif
> -
> -/* Mentor high speed "dual role" controller, in peripheral role */
> -#ifdef CONFIG_USB_MUSB_GADGET
> -#define gadget_is_musbhdrc(g)	(!strcmp("musb-hdrc", (g)->name))
> -#else
> -#define gadget_is_musbhdrc(g)	0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_M66592
> -#define	gadget_is_m66592(g)	(!strcmp("m66592_udc", (g)->name))
> -#else
> -#define	gadget_is_m66592(g)	0
> -#endif
> -
> -#ifdef CONFIG_CI_UDC
> -#define gadget_is_ci(g)        (!strcmp("ci_udc", (g)->name))
> -#else
> -#define gadget_is_ci(g)        0
> -#endif
> -
> -#ifdef CONFIG_USB_DWC3_GADGET
> -#define gadget_is_dwc3(g)        (!strcmp("dwc3-gadget", (g)->name))
> -#else
> -#define gadget_is_dwc3(g)        0
> -#endif
> -
> -#ifdef CONFIG_USB_CDNS3_GADGET
> -#define gadget_is_cdns3(g)        (!strcmp("cdns3-gadget", (g)->name))
> -#else
> -#define gadget_is_cdns3(g)        0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_MAX3420
> -#define gadget_is_max3420(g)        (!strcmp("max3420-udc", (g)->name))
> -#else
> -#define gadget_is_max3420(g)        0
> -#endif
> -
> -#ifdef CONFIG_USB_MTU3_GADGET
> -#define gadget_is_mtu3(g)        (!strcmp("mtu3-gadget", (g)->name))
> -#else
> -#define gadget_is_mtu3(g)        0
> -#endif
> -
> -#ifdef CONFIG_USB_GADGET_DWC2_OTG
> -#define gadget_is_dwc2(g)        (!strcmp("dwc2-udc", (g)->name))
> -#else
> -#define gadget_is_dwc2(g)        0
> -#endif
> -- 
> 2.43.0

[PATCH 5/6] usb: gadget: Add full ep_matches() check past .match_ep() callback

[Marek Vasut]

If .match_ep() callback returns non-NULL endpoint, immediately check
its usability and if the returned endpoint is usable, stop search and
return the endpoint. Otherwise, continue with best effort search for
usable endpoint.

Currently the code would attempt the best effort search in any case,
which may find another unexpected endpoint. It is likely that the
intention of the original code was to stop the search early.

Fixes: 77dcbdf3c1ce ("usb: gadget: Add match_ep() op to usb_gadget_ops")
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/usb/gadget/epautoconf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 09950ceeaed..66599ce8efa 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -247,8 +247,11 @@ struct usb_ep *usb_ep_autoconfig(
 			return ep;
 	}
 
-	if (gadget->ops->match_ep)
+	if (gadget->ops->match_ep) {
 		ep = gadget->ops->match_ep(gadget, desc, NULL);
+		if (ep && ep_matches(gadget, ep, desc))
+			return ep;
+	}
 
 	/* Second, look at endpoints until an unclaimed one looks usable */
 	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
-- 
2.43.0

Re: [PATCH 5/6] usb: gadget: Add full ep_matches() check past .match_ep() callback

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On dim., juin 09, 2024 at 23:32, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> If .match_ep() callback returns non-NULL endpoint, immediately check
> its usability and if the returned endpoint is usable, stop search and
> return the endpoint. Otherwise, continue with best effort search for
> usable endpoint.
>
> Currently the code would attempt the best effort search in any case,
> which may find another unexpected endpoint. It is likely that the
> intention of the original code was to stop the search early.
>
> Fixes: 77dcbdf3c1ce ("usb: gadget: Add match_ep() op to usb_gadget_ops")
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

I've added Vignesh to the CC list since he is the author of
77dcbdf3c1ce. He might be able to comment if this was indeed a mistake.

It looks like a good fix to me as well. With this change we match more closely
the linux implementation (usb_ep_autoconfig_ss()).

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/gadget/epautoconf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 09950ceeaed..66599ce8efa 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -247,8 +247,11 @@ struct usb_ep *usb_ep_autoconfig(
>  			return ep;
>  	}
>  
> -	if (gadget->ops->match_ep)
> +	if (gadget->ops->match_ep) {
>  		ep = gadget->ops->match_ep(gadget, desc, NULL);
> +		if (ep && ep_matches(gadget, ep, desc))
> +			return ep;
> +	}
>  
>  	/* Second, look at endpoints until an unclaimed one looks usable */
>  	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> -- 
> 2.43.0

[PATCH 6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback

[Marek Vasut]

Use the .match_ep() callback instead of workaround in core code.
Replace descriptor parsing with ch9 macros with the same effect.
Drop the SPL specific behavior, it is unclear why SPL should even
be special.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/usb/dwc3/gadget.c       | 33 +++++++++++++++++++++++
 drivers/usb/gadget/epautoconf.c | 46 +--------------------------------
 2 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fab32575647..3ef2f016a60 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1606,6 +1606,38 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 	return 0;
 }
 
+static struct usb_ep *dwc3_find_ep(struct usb_gadget *gadget, const char *name)
+{
+	struct usb_ep *ep;
+
+	list_for_each_entry(ep, &gadget->ep_list, ep_list)
+		if (!strcmp(ep->name, name))
+			return ep;
+
+	return NULL;
+}
+
+static struct
+usb_ep *dwc3_gadget_match_ep(struct usb_gadget *gadget,
+			     struct usb_endpoint_descriptor *desc,
+			     struct usb_ss_ep_comp_descriptor *comp_desc)
+{
+	/*
+	 * First try standard, common configuration: ep1in-bulk,
+	 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
+	 * confusion in already deployed software (endpoint numbers
+	 * hardcoded in userspace software/drivers)
+	 */
+	if (usb_endpoint_is_bulk_in(desc))
+		return dwc3_find_ep(gadget, "ep1in");
+	if (usb_endpoint_is_bulk_out(desc))
+		return dwc3_find_ep(gadget, "ep2out");
+	if (usb_endpoint_is_int_in(desc))
+		return dwc3_find_ep(gadget, "ep3in");
+
+	return NULL;
+}
+
 static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.get_frame		= dwc3_gadget_get_frame,
 	.wakeup			= dwc3_gadget_wakeup,
@@ -1613,6 +1645,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.pullup			= dwc3_gadget_pullup,
 	.udc_start		= dwc3_gadget_start,
 	.udc_stop		= dwc3_gadget_stop,
+	.match_ep		= dwc3_gadget_match_ep,
 };
 
 /* -------------------------------------------------------------------------- */
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 66599ce8efa..a4da4f72de9 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -166,18 +166,6 @@ static int ep_matches(
 	return 1;
 }
 
-static struct usb_ep *
-find_ep(struct usb_gadget *gadget, const char *name)
-{
-	struct usb_ep	*ep;
-
-	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
-		if (0 == strcmp(ep->name, name))
-			return ep;
-	}
-	return NULL;
-}
-
 /**
  * usb_ep_autoconfig - choose an endpoint matching the descriptor
  * @gadget: The device to which the endpoint must belong.
@@ -213,39 +201,7 @@ struct usb_ep *usb_ep_autoconfig(
 	struct usb_endpoint_descriptor	*desc
 )
 {
-	struct usb_ep	*ep = NULL;
-	u8		type;
-
-	type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
-
-	/* First, apply chip-specific "best usage" knowledge.
-	 * This might make a good usb_gadget_ops hook ...
-	 */
-	if (!IS_ENABLED(CONFIG_SPL_BUILD) &&
-	    IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
-	    !strcmp("dwc3-gadget", gadget->name)) {
-		const char *name = NULL;
-		/*
-		 * First try standard, common configuration: ep1in-bulk,
-		 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
-		 * confusion in already deployed software (endpoint numbers
-		 * hardcoded in userspace software/drivers)
-		 */
-		if ((desc->bEndpointAddress & USB_DIR_IN) &&
-		    type == USB_ENDPOINT_XFER_BULK)
-			name = "ep1in";
-		else if ((desc->bEndpointAddress & USB_DIR_IN) == 0 &&
-			 type == USB_ENDPOINT_XFER_BULK)
-			name = "ep2out";
-		else if ((desc->bEndpointAddress & USB_DIR_IN) &&
-			 type == USB_ENDPOINT_XFER_INT)
-			name = "ep3in";
-
-		if (name)
-			ep = find_ep(gadget, name);
-		if (ep && ep_matches(gadget, ep, desc))
-			return ep;
-	}
+	struct usb_ep *ep;
 
 	if (gadget->ops->match_ep) {
 		ep = gadget->ops->match_ep(gadget, desc, NULL);
-- 
2.43.0

Re: [PATCH 6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback

[Sverdlin, Alexander]

Hello Marek!

On Sun, 2024-06-09 at 23:32 +0200, Marek Vasut wrote:
> Use the .match_ep() callback instead of workaround in core code.
> Replace descriptor parsing with ch9 macros with the same effect.
> Drop the SPL specific behavior, it is unclear why SPL should even
> be special.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

I've tested the whole series on TI AM62, booting over USB DFU
(both SPL and U-Boot proper), looks good:

Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/dwc3/gadget.c       | 33 +++++++++++++++++++++++
>  drivers/usb/gadget/epautoconf.c | 46 +--------------------------------
>  2 files changed, 34 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index fab32575647..3ef2f016a60 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1606,6 +1606,38 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>  	return 0;
>  }
>  
> +static struct usb_ep *dwc3_find_ep(struct usb_gadget *gadget, const char *name)
> +{
> +	struct usb_ep *ep;
> +
> +	list_for_each_entry(ep, &gadget->ep_list, ep_list)
> +		if (!strcmp(ep->name, name))
> +			return ep;
> +
> +	return NULL;
> +}
> +
> +static struct
> +usb_ep *dwc3_gadget_match_ep(struct usb_gadget *gadget,
> +			     struct usb_endpoint_descriptor *desc,
> +			     struct usb_ss_ep_comp_descriptor *comp_desc)
> +{
> +	/*
> +	 * First try standard, common configuration: ep1in-bulk,
> +	 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
> +	 * confusion in already deployed software (endpoint numbers
> +	 * hardcoded in userspace software/drivers)
> +	 */
> +	if (usb_endpoint_is_bulk_in(desc))
> +		return dwc3_find_ep(gadget, "ep1in");
> +	if (usb_endpoint_is_bulk_out(desc))
> +		return dwc3_find_ep(gadget, "ep2out");
> +	if (usb_endpoint_is_int_in(desc))
> +		return dwc3_find_ep(gadget, "ep3in");
> +
> +	return NULL;
> +}
> +
>  static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.get_frame		= dwc3_gadget_get_frame,
>  	.wakeup			= dwc3_gadget_wakeup,
> @@ -1613,6 +1645,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.pullup			= dwc3_gadget_pullup,
>  	.udc_start		= dwc3_gadget_start,
>  	.udc_stop		= dwc3_gadget_stop,
> +	.match_ep		= dwc3_gadget_match_ep,
>  };
>  
>  /* -------------------------------------------------------------------------- */
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 66599ce8efa..a4da4f72de9 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -166,18 +166,6 @@ static int ep_matches(
>  	return 1;
>  }
>  
> -static struct usb_ep *
> -find_ep(struct usb_gadget *gadget, const char *name)
> -{
> -	struct usb_ep	*ep;
> -
> -	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> -		if (0 == strcmp(ep->name, name))
> -			return ep;
> -	}
> -	return NULL;
> -}
> -
>  /**
>   * usb_ep_autoconfig - choose an endpoint matching the descriptor
>   * @gadget: The device to which the endpoint must belong.
> @@ -213,39 +201,7 @@ struct usb_ep *usb_ep_autoconfig(
>  	struct usb_endpoint_descriptor	*desc
>  )
>  {
> -	struct usb_ep	*ep = NULL;
> -	u8		type;
> -
> -	type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> -
> -	/* First, apply chip-specific "best usage" knowledge.
> -	 * This might make a good usb_gadget_ops hook ...
> -	 */
> -	if (!IS_ENABLED(CONFIG_SPL_BUILD) &&
> -	    IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
> -	    !strcmp("dwc3-gadget", gadget->name)) {
> -		const char *name = NULL;
> -		/*
> -		 * First try standard, common configuration: ep1in-bulk,
> -		 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
> -		 * confusion in already deployed software (endpoint numbers
> -		 * hardcoded in userspace software/drivers)
> -		 */
> -		if ((desc->bEndpointAddress & USB_DIR_IN) &&
> -		    type == USB_ENDPOINT_XFER_BULK)
> -			name = "ep1in";
> -		else if ((desc->bEndpointAddress & USB_DIR_IN) == 0 &&
> -			 type == USB_ENDPOINT_XFER_BULK)
> -			name = "ep2out";
> -		else if ((desc->bEndpointAddress & USB_DIR_IN) &&
> -			 type == USB_ENDPOINT_XFER_INT)
> -			name = "ep3in";
> -
> -		if (name)
> -			ep = find_ep(gadget, name);
> -		if (ep && ep_matches(gadget, ep, desc))
> -			return ep;
> -	}
> +	struct usb_ep *ep;
>  
>  	if (gadget->ops->match_ep) {
>  		ep = gadget->ops->match_ep(gadget, desc, NULL);

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH 6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On dim., juin 09, 2024 at 23:32, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> Use the .match_ep() callback instead of workaround in core code.
> Replace descriptor parsing with ch9 macros with the same effect.
> Drop the SPL specific behavior, it is unclear why SPL should even
> be special.

Li, Peng,

Is this good for you as well?

You seem to be the author/committer of:
c93edbf5385e ("usb: gadget: don't change ep name for dwc3 while ep autoconfig")

I'd like to make sure this patch does not break your use-case.
Please let me know within 2 weeks or so, otherwise I'll apply the changes.

>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

To me, this looks good.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/dwc3/gadget.c       | 33 +++++++++++++++++++++++
>  drivers/usb/gadget/epautoconf.c | 46 +--------------------------------
>  2 files changed, 34 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index fab32575647..3ef2f016a60 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1606,6 +1606,38 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>  	return 0;
>  }
>  
> +static struct usb_ep *dwc3_find_ep(struct usb_gadget *gadget, const char *name)
> +{
> +	struct usb_ep *ep;
> +
> +	list_for_each_entry(ep, &gadget->ep_list, ep_list)
> +		if (!strcmp(ep->name, name))
> +			return ep;
> +
> +	return NULL;
> +}
> +
> +static struct
> +usb_ep *dwc3_gadget_match_ep(struct usb_gadget *gadget,
> +			     struct usb_endpoint_descriptor *desc,
> +			     struct usb_ss_ep_comp_descriptor *comp_desc)
> +{
> +	/*
> +	 * First try standard, common configuration: ep1in-bulk,
> +	 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
> +	 * confusion in already deployed software (endpoint numbers
> +	 * hardcoded in userspace software/drivers)
> +	 */
> +	if (usb_endpoint_is_bulk_in(desc))
> +		return dwc3_find_ep(gadget, "ep1in");
> +	if (usb_endpoint_is_bulk_out(desc))
> +		return dwc3_find_ep(gadget, "ep2out");
> +	if (usb_endpoint_is_int_in(desc))
> +		return dwc3_find_ep(gadget, "ep3in");
> +
> +	return NULL;
> +}
> +
>  static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.get_frame		= dwc3_gadget_get_frame,
>  	.wakeup			= dwc3_gadget_wakeup,
> @@ -1613,6 +1645,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.pullup			= dwc3_gadget_pullup,
>  	.udc_start		= dwc3_gadget_start,
>  	.udc_stop		= dwc3_gadget_stop,
> +	.match_ep		= dwc3_gadget_match_ep,
>  };
>  
>  /* -------------------------------------------------------------------------- */
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 66599ce8efa..a4da4f72de9 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -166,18 +166,6 @@ static int ep_matches(
>  	return 1;
>  }
>  
> -static struct usb_ep *
> -find_ep(struct usb_gadget *gadget, const char *name)
> -{
> -	struct usb_ep	*ep;
> -
> -	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> -		if (0 == strcmp(ep->name, name))
> -			return ep;
> -	}
> -	return NULL;
> -}
> -
>  /**
>   * usb_ep_autoconfig - choose an endpoint matching the descriptor
>   * @gadget: The device to which the endpoint must belong.
> @@ -213,39 +201,7 @@ struct usb_ep *usb_ep_autoconfig(
>  	struct usb_endpoint_descriptor	*desc
>  )
>  {
> -	struct usb_ep	*ep = NULL;
> -	u8		type;
> -
> -	type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> -
> -	/* First, apply chip-specific "best usage" knowledge.
> -	 * This might make a good usb_gadget_ops hook ...
> -	 */
> -	if (!IS_ENABLED(CONFIG_SPL_BUILD) &&
> -	    IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
> -	    !strcmp("dwc3-gadget", gadget->name)) {
> -		const char *name = NULL;
> -		/*
> -		 * First try standard, common configuration: ep1in-bulk,
> -		 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
> -		 * confusion in already deployed software (endpoint numbers
> -		 * hardcoded in userspace software/drivers)
> -		 */
> -		if ((desc->bEndpointAddress & USB_DIR_IN) &&
> -		    type == USB_ENDPOINT_XFER_BULK)
> -			name = "ep1in";
> -		else if ((desc->bEndpointAddress & USB_DIR_IN) == 0 &&
> -			 type == USB_ENDPOINT_XFER_BULK)
> -			name = "ep2out";
> -		else if ((desc->bEndpointAddress & USB_DIR_IN) &&
> -			 type == USB_ENDPOINT_XFER_INT)
> -			name = "ep3in";
> -
> -		if (name)
> -			ep = find_ep(gadget, name);
> -		if (ep && ep_matches(gadget, ep, desc))
> -			return ep;
> -	}
> +	struct usb_ep *ep;
>  
>  	if (gadget->ops->match_ep) {
>  		ep = gadget->ops->match_ep(gadget, desc, NULL);
> -- 
> 2.43.0

Re: [PATCH 1/6] usb: gadget: g_dnl: Drop usb_gadget_controller_number()

[Mattijs Korpershoek]

Hi Marek,

Thank you for the patch.

On dim., juin 09, 2024 at 23:32, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> The bcdDevice field is defined as
> |Device release number in binary-coded decimal
> in the USB 2.0 specification. We use this field to distinguish the UDCs
> from each other. In theory this could be used on the host side to apply
> certain quirks if the "special" UDC in combination with this gadget is
> used. This hasn't been done as far as I am aware. In practice it would
> be better to fix the UDC driver before shipping since a later release
> might not need this quirk anymore.
>
> This patch converts this gadget to use the U-Boot version instead of a
> random 2 or 3 plus the UDC number. Linux stopped using this functionality
> in 2012, remove it from U-Boot as well.
>
> Matching Linux kernel commit:
> ed9cbda63d45 ("usb: gadget: remove usb_gadget_controller_number()")
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Compared with linux commit, and looks good to me.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Tested that I could use fastboot, ums and scan for storage devices on
khadas vim3

Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # vim3

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/gadget/g_dnl.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index b5b5f5d8c11..631969b3405 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -17,10 +17,10 @@
>  #include <usb_mass_storage.h>
>  #include <dfu.h>
>  #include <thor.h>
> +#include <version.h>
>  
>  #include <env_callback.h>
>  
> -#include "gadget_chips.h"
>  #include "composite.c"
>  
>  /*
> @@ -199,18 +199,6 @@ void g_dnl_clear_detach(void)
>  	g_dnl_detach_request = false;
>  }
>  
> -static int g_dnl_get_bcd_device_number(struct usb_composite_dev *cdev)
> -{
> -	struct usb_gadget *gadget = cdev->gadget;
> -	int gcnum;
> -
> -	gcnum = usb_gadget_controller_number(gadget);
> -	if (gcnum > 0)
> -		gcnum += 0x200;
> -
> -	return g_dnl_get_board_bcd_device_number(gcnum);
> -}
> -
>  /**
>   * Update internal serial number variable when the "serial#" env var changes.
>   *
> @@ -261,7 +249,8 @@ static int g_dnl_bind(struct usb_composite_dev *cdev)
>  	if (ret)
>  		goto error;
>  
> -	gcnum = g_dnl_get_bcd_device_number(cdev);
> +	gcnum = g_dnl_get_board_bcd_device_number((U_BOOT_VERSION_NUM << 4) |
> +						  U_BOOT_VERSION_NUM_PATCH);
>  	if (gcnum >= 0)
>  		device_desc.bcdDevice = cpu_to_le16(gcnum);
>  	else {
> -- 
> 2.43.0

Re: [PATCH 1/6] usb: gadget: g_dnl: Drop usb_gadget_controller_number()

[Mattijs Korpershoek]

Hi,

On Sun, 09 Jun 2024 23:32:14 +0200, Marek Vasut wrote:
> The bcdDevice field is defined as
> |Device release number in binary-coded decimal
> in the USB 2.0 specification. We use this field to distinguish the UDCs
> from each other. In theory this could be used on the host side to apply
> certain quirks if the "special" UDC in combination with this gadget is
> used. This hasn't been done as far as I am aware. In practice it would
> be better to fix the UDC driver before shipping since a later release
> might not need this quirk anymore.
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/6] usb: gadget: g_dnl: Drop usb_gadget_controller_number()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/0fca00114a805ec3c68c6156bc4ae6c4214a6e8a
[2/6] usb: gadget: ether: Drop usb_gadget_controller_number()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/7f1b062ca4f23ea77541006759c1ec1108bdda74
[3/6] usb: gadget: Drop usb_gadget_controller_number()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/2cee3bc6abc592c403140b8a7191879ce95be992
[4/6] usb: gadget: Drop all gadget_is_*() functions
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/db62b6a0a016b69cf0e26eea4004c3edbebd4394
[5/6] usb: gadget: Add full ep_matches() check past .match_ep() callback
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/bd7ec7b04f877b8b4a88d4367f100dc3f0af27a3
[6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/1918b8010c321c939fdedd6e461ccac87e0d3415

--
Mattijs