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

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 {

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

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

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; -}

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

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

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

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

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) &&
const char *name = NULL; /*!strcmp("dwc3-gadget", gadget->name)) {
- 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) &&
/* reduce tx dma overhead by avoiding special cases */ zlp = 0;!strcmp("musb-hdrc", gadget->name)) {
} 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

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) {

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

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);

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);

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

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) |
if (gcnum >= 0) device_desc.bcdDevice = cpu_to_le16(gcnum); else {U_BOOT_VERSION_NUM_PATCH);
-- 2.43.0

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/0fca00114a805ec... [2/6] usb: gadget: ether: Drop usb_gadget_controller_number() https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/7f1b062ca4f23ea... [3/6] usb: gadget: Drop usb_gadget_controller_number() https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/2cee3bc6abc592c... [4/6] usb: gadget: Drop all gadget_is_*() functions https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/db62b6a0a016b69... [5/6] usb: gadget: Add full ep_matches() check past .match_ep() callback https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/bd7ec7b04f877b8... [6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/1918b8010c321c9...
-- Mattijs
participants (4)
-
Lukasz Majewski
-
Marek Vasut
-
Mattijs Korpershoek
-
Sverdlin, Alexander