Re: [PATCH 6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback
Hi Marek,
Thank you for the patch.
On dim., juin 09, 2024 at 23:32, 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.
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
To me, this looks good.
Reviewed-by: Mattijs Korpershoek
Tested-by: Mattijs Korpershoek # on vim3
> ---
> Cc: Alexander Sverdlin
> Cc: Felipe Balbi
> Cc: Lukasz Majewski
> Cc: Mattijs Korpershoek
> Cc: Nishanth Menon
> Cc: Simon Glass
> Cc: Thinh Nguyen
> Cc: Tom Rini
> 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, >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, >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)
> -
Re: [PATCH 6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback
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
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
> ---
> Cc: Alexander Sverdlin
> Cc: Felipe Balbi
> Cc: Lukasz Majewski
> Cc: Mattijs Korpershoek
> Cc: Nishanth Menon
> Cc: Simon Glass
> Cc: Thinh Nguyen
> Cc: Tom Rini
> 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, >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, >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);
> -
[PATCH 6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback
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
---
Cc: Alexander Sverdlin
Cc: Felipe Balbi
Cc: Lukasz Majewski
Cc: Mattijs Korpershoek
Cc: Nishanth Menon
Cc: Simon Glass
Cc: Thinh Nguyen
Cc: Tom Rini
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, >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, >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
