[PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Andy Shevchenko]

On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
(USB Device Controller mode) may be reserved for some special means, such as
tracing. This series extends DT schema and driver to avoid using those.
Without this the USB gadget mode won't work properly (those devices that
"luckily" allocated the reserved EPs).

Ferry already tested the privately sent PoC of this, but I ask him again to
re-test this version which is slightly different.

Andy Shevchenko (3):
  dt-bindings: usb: dwc3: Add a property to reserve endpoints
  usb: dwc3: gadget: Add support for snps,reserved-endpoints property
  usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

 .../devicetree/bindings/usb/snps,dwc3.yaml    | 10 +++++
 drivers/usb/dwc3/dwc3-pci.c                   |  9 +++++
 drivers/usb/dwc3/gadget.c                     | 38 ++++++++++++++++++-
 3 files changed, 56 insertions(+), 1 deletion(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/3] dt-bindings: usb: dwc3: Add a property to reserve endpoints

[Andy Shevchenko]

Some of the endpoints may be reserved by hardware for different purposes,
e.g., tracing control and output. This is the case, for instance, on
Intel Merrifield and Moorefield platforms that reserve a few and USB driver
may not use them for the regular transfers. Add the respective bindings.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 1cd0ca90127d..2ae8e5e85f78 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -109,6 +109,16 @@ properties:
   resets:
     minItems: 1
 
+  snps,reserved-endpoints:
+    description:
+      Reserve endpoints for other needs, e.g, for tracing control and output.
+      When set, the driver will avoid using them for the regular USB transfers.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    maxItems: 30
+    items:
+      minimum: 2
+      maximum: 31
+
   snps,usb2-lpm-disable:
     description: Indicate if we don't want to enable USB2 HW LPM for host
       mode.
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/3] dt-bindings: usb: dwc3: Add a property to reserve endpoints

[Rob Herring]

On Thu, Jan 16, 2025 at 05:40:46PM +0200, Andy Shevchenko wrote:
> Some of the endpoints may be reserved by hardware for different purposes,
> e.g., tracing control and output. This is the case, for instance, on
> Intel Merrifield and Moorefield platforms that reserve a few and USB driver
> may not use them for the regular transfers. Add the respective bindings.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index 1cd0ca90127d..2ae8e5e85f78 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -109,6 +109,16 @@ properties:
>    resets:
>      minItems: 1
>  
> +  snps,reserved-endpoints:
> +    description:
> +      Reserve endpoints for other needs, e.g, for tracing control and output.
> +      When set, the driver will avoid using them for the regular USB transfers.
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    maxItems: 30

Please make minItems explicit.

> +    items:
> +      minimum: 2
> +      maximum: 31
> +
>    snps,usb2-lpm-disable:
>      description: Indicate if we don't want to enable USB2 HW LPM for host
>        mode.
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
> 

Re: [PATCH v1 1/3] dt-bindings: usb: dwc3: Add a property to reserve endpoints

[Andy Shevchenko]

On Thu, Jan 23, 2025 at 04:09:46PM -0600, Rob Herring wrote:
> On Thu, Jan 16, 2025 at 05:40:46PM +0200, Andy Shevchenko wrote:
> > Some of the endpoints may be reserved by hardware for different purposes,
> > e.g., tracing control and output. This is the case, for instance, on
> > Intel Merrifield and Moorefield platforms that reserve a few and USB driver
> > may not use them for the regular transfers. Add the respective bindings.

...

> > +  snps,reserved-endpoints:
> > +    description:
> > +      Reserve endpoints for other needs, e.g, for tracing control and output.
> > +      When set, the driver will avoid using them for the regular USB transfers.
> > +    $ref: /schemas/types.yaml#/definitions/uint8-array
> > +    maxItems: 30
> 
> Please make minItems explicit.

    minItems: 0
    maxItems: 30

Is this what you want to see here?

> > +    items:
> > +      minimum: 2
> > +      maximum: 31

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

The snps,reserved-endpoints property lists the reserved endpoints
that shouldn't be used for normal transfers. Add support for that
to the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 31a654c6f15b..3f806fb8b61c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3349,14 +3349,50 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
 	return 0;
 }
 
+static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, u8 *eps, size_t count)
+{
+	const char *propname = "snps,reserved-endpoints";
+	int ret;
+
+	ret = device_property_count_u8(dwc->dev, propname);
+	if (ret < 0)
+		return 0;
+	if (ret == 0)
+		return 0;
+	if (ret > count) {
+		dev_err(dwc->dev, "too many entries in %s\n", propname);
+		return -EINVAL;
+	}
+
+	count = ret;
+	ret = device_property_read_u8_array(dwc->dev, propname, eps, count);
+	if (ret)
+		dev_err(dwc->dev, "failed to read %s\n", propname);
+
+	return ret;
+}
+
 static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
 {
 	u8				epnum;
+	u8 eps[DWC3_ENDPOINTS_NUM];
+	u8 count, num;
+	int ret;
 
 	INIT_LIST_HEAD(&dwc->gadget->ep_list);
 
+	ret = dwc3_gadget_parse_reserved_endpoints(dwc, eps, ARRAY_SIZE(eps));
+	if (ret < 0)
+		return ret;
+	count = ret;
+
 	for (epnum = 0; epnum < total; epnum++) {
-		int			ret;
+		for (num = 0; num < count; num++) {
+			if (epnum == eps[num])
+				break;
+		}
+		if (num < count)
+			continue;
 
 		ret = dwc3_gadget_init_endpoint(dwc, epnum);
 		if (ret)
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> The snps,reserved-endpoints property lists the reserved endpoints
> that shouldn't be used for normal transfers. Add support for that
> to the driver.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/dwc3/gadget.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 31a654c6f15b..3f806fb8b61c 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3349,14 +3349,50 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
>  	return 0;
>  }
>  
> +static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, u8 *eps, size_t count)
> +{
> +	const char *propname = "snps,reserved-endpoints";
> +	int ret;
> +
> +	ret = device_property_count_u8(dwc->dev, propname);
> +	if (ret < 0)
> +		return 0;
> +	if (ret == 0)
> +		return 0;

Just use if (ret <= 0) return 0.

> +	if (ret > count) {
> +		dev_err(dwc->dev, "too many entries in %s\n", propname);
> +		return -EINVAL;
> +	}
> +
> +	count = ret;
> +	ret = device_property_read_u8_array(dwc->dev, propname, eps, count);
> +	if (ret)
> +		dev_err(dwc->dev, "failed to read %s\n", propname);
> +
> +	return ret;
> +}
> +
>  static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
>  {
>  	u8				epnum;
> +	u8 eps[DWC3_ENDPOINTS_NUM];

Please keep consistent alignment.

> +	u8 count, num;

Please keep declaration in separate lines here.

> +	int ret;
>  
>  	INIT_LIST_HEAD(&dwc->gadget->ep_list);
>  
> +	ret = dwc3_gadget_parse_reserved_endpoints(dwc, eps, ARRAY_SIZE(eps));
> +	if (ret < 0)
> +		return ret;
> +	count = ret;
> +
>  	for (epnum = 0; epnum < total; epnum++) {
> -		int			ret;
> +		for (num = 0; num < count; num++) {
> +			if (epnum == eps[num])
> +				break;
> +		}
> +		if (num < count)
> +			continue;

You can probably rewrite this logic better.

>  
>  		ret = dwc3_gadget_init_endpoint(dwc, epnum);
>  		if (ret)
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
> 

BR,
Thinh

Re: [PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> >  	for (epnum = 0; epnum < total; epnum++) {
> > -		int			ret;
> > +		for (num = 0; num < count; num++) {
> > +			if (epnum == eps[num])
> > +				break;
> > +		}
> > +		if (num < count)
> > +			continue;
> 
> You can probably rewrite this logic better.

Any suggestions?

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > >  	for (epnum = 0; epnum < total; epnum++) {
> > > -		int			ret;
> > > +		for (num = 0; num < count; num++) {
> > > +			if (epnum == eps[num])
> > > +				break;
> > > +		}
> > > +		if (num < count)
> > > +			continue;
> > 
> > You can probably rewrite this logic better.
> 
> Any suggestions?
> 
> Thanks for the review!
> 

From the first look, is the list sorted? If so, you don't need another
for-loop.

Also, we loop over the number of endpoints throughout the driver, but
you only skip it here during init. Please double check for invalid
accessing of endpoints in other places.

Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
when you parse the reserved endpoints so you can skip them in your loop.

BR,
Thinh

Re: [PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

On Wed, Jan 22, 2025 at 01:44:02AM +0000, Thinh Nguyen wrote:
> On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > >  	for (epnum = 0; epnum < total; epnum++) {
> > > > -		int			ret;
> > > > +		for (num = 0; num < count; num++) {
> > > > +			if (epnum == eps[num])
> > > > +				break;
> > > > +		}
> > > > +		if (num < count)
> > > > +			continue;
> > > 
> > > You can probably rewrite this logic better.
> > 
> > Any suggestions?
> > 
> > Thanks for the review!
> 
> From the first look, is the list sorted? If so, you don't need another
> for-loop.

Even if it's sorted it's not 1:1 mapped by indices. I do not understand how we
can avoid the second loop. The only possibility is indeed to sort the list and
sparse it in accordance to the endpoint numbers, but if we are going this way,
it's much easier to switch to bitmap and the respective bitops.

> Also, we loop over the number of endpoints throughout the driver, but
> you only skip it here during init. Please double check for invalid
> accessing of endpoints in other places.
> 
> Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
> when you parse the reserved endpoints so you can skip them in your loop.

Note, this is only for UDC, in USB host these are okay to be used.
Does your suggestion imply that?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> On Wed, Jan 22, 2025 at 01:44:02AM +0000, Thinh Nguyen wrote:
> > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > >  	for (epnum = 0; epnum < total; epnum++) {
> > > > > -		int			ret;
> > > > > +		for (num = 0; num < count; num++) {
> > > > > +			if (epnum == eps[num])
> > > > > +				break;
> > > > > +		}
> > > > > +		if (num < count)
> > > > > +			continue;
> > > > 
> > > > You can probably rewrite this logic better.
> > > 
> > > Any suggestions?
> > > 
> > > Thanks for the review!
> > 
> > From the first look, is the list sorted? If so, you don't need another
> > for-loop.
> 
> Even if it's sorted it's not 1:1 mapped by indices. I do not understand how we
> can avoid the second loop. The only possibility is indeed to sort the list and
> sparse it in accordance to the endpoint numbers, but if we are going this way,
> it's much easier to switch to bitmap and the respective bitops.

If it's sorted, it can be something like this. Just a quick logic and not tested:

num = 0
for (epnum = 0; epnum < total; epnum++) {
	if (num < count && epnum == eps[num]) {
		num++;
		continue;
	}

	...
}

> 
> > Also, we loop over the number of endpoints throughout the driver, but
> > you only skip it here during init. Please double check for invalid
> > accessing of endpoints in other places.
> > 
> > Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
> > when you parse the reserved endpoints so you can skip them in your loop.
> 
> Note, this is only for UDC, in USB host these are okay to be used.
> Does your suggestion imply that?
> 

No. We track the total num_eps in dwc->num_eps. Then we do for-loop to
dwc->eps[i] and access the endpoint. Often we check if the endpoint is
NULL before accessing dwc->eps[i]. However, we don't do it everywhere.
So I ask for you to review to make sure that this change doesn't break
elsewhere where we may try to access dwc->eps[i] to an uninit endpoint
(Note I see at least 1 place e.g. dwc3_gadget_clear_tx_fifos that may
break)

BR,
Thinh

Re: [PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

On Tue, Jan 28, 2025 at 02:39:50AM +0000, Thinh Nguyen wrote:
> On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> > On Wed, Jan 22, 2025 at 01:44:02AM +0000, Thinh Nguyen wrote:
> > > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > > On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > > >  	for (epnum = 0; epnum < total; epnum++) {
> > > > > > -		int			ret;
> > > > > > +		for (num = 0; num < count; num++) {
> > > > > > +			if (epnum == eps[num])
> > > > > > +				break;
> > > > > > +		}
> > > > > > +		if (num < count)
> > > > > > +			continue;
> > > > > 
> > > > > You can probably rewrite this logic better.
> > > > 
> > > > Any suggestions?
> > > > 
> > > > Thanks for the review!
> > > 
> > > From the first look, is the list sorted? If so, you don't need another
> > > for-loop.
> > 
> > Even if it's sorted it's not 1:1 mapped by indices. I do not understand how we
> > can avoid the second loop. The only possibility is indeed to sort the list and
> > sparse it in accordance to the endpoint numbers, but if we are going this way,
> > it's much easier to switch to bitmap and the respective bitops.
> 
> If it's sorted, it can be something like this. Just a quick logic and not tested:
> 
> num = 0
> for (epnum = 0; epnum < total; epnum++) {
> 	if (num < count && epnum == eps[num]) {
> 		num++;
> 		continue;
> 	}
> 
> 	...
> }

Ah, okay, I have got the idea. Let me try to mock up something working
out of it.

> > > Also, we loop over the number of endpoints throughout the driver, but
> > > you only skip it here during init. Please double check for invalid
> > > accessing of endpoints in other places.
> > > 
> > > Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
> > > when you parse the reserved endpoints so you can skip them in your loop.
> > 
> > Note, this is only for UDC, in USB host these are okay to be used.
> > Does your suggestion imply that?
> 
> No. We track the total num_eps in dwc->num_eps. Then we do for-loop to
> dwc->eps[i] and access the endpoint. Often we check if the endpoint is
> NULL before accessing dwc->eps[i]. However, we don't do it everywhere.
> So I ask for you to review to make sure that this change doesn't break
> elsewhere where we may try to access dwc->eps[i] to an uninit endpoint
> (Note I see at least 1 place e.g. dwc3_gadget_clear_tx_fifos that may
> break)

I see, so having my code as is also requiring to check all users of
the eps array in the _gadget part_ of the driver to see if they won't
crash due to NULL pointer dereference. Is it what you want?
If so, definitely I will revisit that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Tue, Jan 28, 2025, Andy Shevchenko wrote:
> On Tue, Jan 28, 2025 at 02:39:50AM +0000, Thinh Nguyen wrote:
> > On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> > > On Wed, Jan 22, 2025 at 01:44:02AM +0000, Thinh Nguyen wrote:
> > 
> > No. We track the total num_eps in dwc->num_eps. Then we do for-loop to
> > dwc->eps[i] and access the endpoint. Often we check if the endpoint is
> > NULL before accessing dwc->eps[i]. However, we don't do it everywhere.
> > So I ask for you to review to make sure that this change doesn't break
> > elsewhere where we may try to access dwc->eps[i] to an uninit endpoint
> > (Note I see at least 1 place e.g. dwc3_gadget_clear_tx_fifos that may
> > break)
> 
> I see, so having my code as is also requiring to check all users of
> the eps array in the _gadget part_ of the driver to see if they won't
> crash due to NULL pointer dereference. Is it what you want?
> If so, definitely I will revisit that.
> 

Yes, help double check that.

Thanks,
Thinh

[PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Andy Shevchenko]

Intel Merrifield SoC uses these endpoints for tracing and they shouldn't
be used for normal transfers, we need to skip them.

• 1 High BW Bulk IN (IN#1) (RTIT)
• 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/dwc3/dwc3-pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 052852f80146..9b73dfd34823 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -148,11 +148,20 @@ static const struct property_entry dwc3_pci_intel_byt_properties[] = {
 	{}
 };
 
+/*
+ * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
+ * for normal transfers, we need to skip them.
+ * • 1 High BW Bulk IN (IN#1) (RTIT)
+ * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
+ */
+static const u8 dwc3_pci_mrfld_reserved_endpoints[] = { 3, 16, 17 };
+
 static const struct property_entry dwc3_pci_mrfld_properties[] = {
 	PROPERTY_ENTRY_STRING("dr_mode", "otg"),
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "mrfld_bcove_pwrsrc"),
 	PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
 	PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
+	PROPERTY_ENTRY_U8_ARRAY("snps,reserved-endpoints", dwc3_pci_mrfld_reserved_endpoints),
 	PROPERTY_ENTRY_BOOL("snps,usb2-gadget-lpm-disable"),
 	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
 	{}
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Thinh Nguyen]

On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> Intel Merrifield SoC uses these endpoints for tracing and they shouldn't
> be used for normal transfers, we need to skip them.
> 
> • 1 High BW Bulk IN (IN#1) (RTIT)
> • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/dwc3/dwc3-pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index 052852f80146..9b73dfd34823 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -148,11 +148,20 @@ static const struct property_entry dwc3_pci_intel_byt_properties[] = {
>  	{}
>  };
>  
> +/*
> + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> + * for normal transfers, we need to skip them.
> + * • 1 High BW Bulk IN (IN#1) (RTIT)
> + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)

Please use regular bullet character and list the endpoint per line.

BR,
Thinh

> + */
> +static const u8 dwc3_pci_mrfld_reserved_endpoints[] = { 3, 16, 17 };
> +
>  static const struct property_entry dwc3_pci_mrfld_properties[] = {
>  	PROPERTY_ENTRY_STRING("dr_mode", "otg"),
>  	PROPERTY_ENTRY_STRING("linux,extcon-name", "mrfld_bcove_pwrsrc"),
>  	PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
>  	PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
> +	PROPERTY_ENTRY_U8_ARRAY("snps,reserved-endpoints", dwc3_pci_mrfld_reserved_endpoints),
>  	PROPERTY_ENTRY_BOOL("snps,usb2-gadget-lpm-disable"),
>  	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
>  	{}
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
> 

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Andy Shevchenko]

On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > + * for normal transfers, we need to skip them.
> > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> 
> Please use regular bullet character and list the endpoint per line.

Which is...?

To my curiosity, what's wrong with the above?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Thinh Nguyen]

On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > + * for normal transfers, we need to skip them.
> > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > 
> > Please use regular bullet character and list the endpoint per line.
> 
> Which is...?
> 
> To my curiosity, what's wrong with the above?
> 

Please use a character that we can find on the keyboard (- or * for
example).

And why would you want to list them like this:

	* Endpoint A
	* Endpoint B + Endpoint C

As oppose to:

	* Endpoint A
	* Endpoint B
	* Endpoint C

BR,
Thinh

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Thinh Nguyen]

On Tue, Jan 21, 2025, Thinh Nguyen wrote:
> On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > > + * for normal transfers, we need to skip them.
> > > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > 
> > > Please use regular bullet character and list the endpoint per line.
> > 
> > Which is...?
> > 
> > To my curiosity, what's wrong with the above?
> > 
> 
> Please use a character that we can find on the keyboard (- or * for
> example).
> 
> And why would you want to list them like this:
> 
> 	* Endpoint A
> 	* Endpoint B + Endpoint C
> 
> As oppose to:
> 
> 	* Endpoint A
> 	* Endpoint B
> 	* Endpoint C
> 

Also, please fix the $subject and replace "endpoints ep[18]{in,out}" to
just "reserved endpoints".

BR,
Thinh

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Andy Shevchenko]

On Tue, Jan 21, 2025 at 11:51:51PM +0000, Thinh Nguyen wrote:
> On Tue, Jan 21, 2025, Thinh Nguyen wrote:

...

> Also, please fix the $subject and replace "endpoints ep[18]{in,out}" to
> just "reserved endpoints".

Sure.

Thank you for the review.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Andy Shevchenko]

On Tue, Jan 21, 2025 at 11:46:17PM +0000, Thinh Nguyen wrote:
> On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > > + * for normal transfers, we need to skip them.
> > > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > 
> > > Please use regular bullet character and list the endpoint per line.
> > 
> > Which is...?
> > 
> > To my curiosity, what's wrong with the above?
> 
> Please use a character that we can find on the keyboard (- or * for
> example).

Hmm... We can find all characters on keyboard by using standard approach of
typing Unicode ones. I'm not sure why this is a problem. Linux kernel is UTF-8
ready project (from source tree point of view), at least I haven't found any
limitations in the documentation.

Note, this is _not_ a kernel-doc style to which you may refer when pointing out
to the how lists should be represented.

But it's not big deal for me to change the • character.

> And why would you want to list them like this:
> 
> 	* Endpoint A
> 	* Endpoint B + Endpoint C

Because:
1) they are logically connected;
2) the above is the exact citation from the specification and I would like to
keep it that way.

> As oppose to:
> 
> 	* Endpoint A
> 	* Endpoint B
> 	* Endpoint C

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Thinh Nguyen]

On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> On Tue, Jan 21, 2025 at 11:46:17PM +0000, Thinh Nguyen wrote:
> > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > > > + * for normal transfers, we need to skip them.
> > > > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > > 
> > > > Please use regular bullet character and list the endpoint per line.
> > > 
> > > Which is...?
> > > 
> > > To my curiosity, what's wrong with the above?
> > 
> > Please use a character that we can find on the keyboard (- or * for
> > example).
> 
> Hmm... We can find all characters on keyboard by using standard approach of
> typing Unicode ones. I'm not sure why this is a problem. Linux kernel is UTF-8
> ready project (from source tree point of view), at least I haven't found any
> limitations in the documentation.
> 
> Note, this is _not_ a kernel-doc style to which you may refer when pointing out

I'm not requesting this out of any kernel-doc style. It's just a
personal preference and consistency in dwc3. If it's not too difficult,
please use "-". But if you must insist, future lists would need to be
consistent to this new unicode style. Then I would need to ask others to
use the new Unicode one. Typically typing * doesn't automatically
convert to • unless you edit using Word, and so I prefer something I and
others can easily find on the keyboard.

> to the how lists should be represented.
> 
> But it's not big deal for me to change the • character.
> 
> > And why would you want to list them like this:
> > 
> > 	* Endpoint A
> > 	* Endpoint B + Endpoint C
> 
> Because:
> 1) they are logically connected;
> 2) the above is the exact citation from the specification and I would like to
> keep it that way.
> 
> > As oppose to:
> > 
> > 	* Endpoint A
> > 	* Endpoint B
> > 	* Endpoint C
> 

If you prefer to keep the snippet of your vendor specification intact,
we can instead document this fully in the commit message and note the
EBC feature. Remove these comments here.

Thanks,
Thinh

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Andy Shevchenko]

On Tue, Jan 28, 2025 at 02:21:40AM +0000, Thinh Nguyen wrote:
> On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> > On Tue, Jan 21, 2025 at 11:46:17PM +0000, Thinh Nguyen wrote:
> > > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > > On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > > > > + * for normal transfers, we need to skip them.
> > > > > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > > > 
> > > > > Please use regular bullet character and list the endpoint per line.
> > > > 
> > > > Which is...?
> > > > 
> > > > To my curiosity, what's wrong with the above?
> > > 
> > > Please use a character that we can find on the keyboard (- or * for
> > > example).
> > 
> > Hmm... We can find all characters on keyboard by using standard approach of
> > typing Unicode ones. I'm not sure why this is a problem. Linux kernel is UTF-8
> > ready project (from source tree point of view), at least I haven't found any
> > limitations in the documentation.
> > 
> > Note, this is _not_ a kernel-doc style to which you may refer when pointing out
> 
> I'm not requesting this out of any kernel-doc style. It's just a
> personal preference and consistency in dwc3. If it's not too difficult,
> please use "-".

As I said...

> But if you must insist, future lists would need to be
> consistent to this new unicode style. Then I would need to ask others to
> use the new Unicode one. Typically typing * doesn't automatically
> convert to • unless you edit using Word, and so I prefer something I and
> others can easily find on the keyboard.
> 
> > to the how lists should be represented.
> > 
> > But it's not big deal for me to change the • character.

...not a big deal to me, I will change as requested.

> > > And why would you want to list them like this:
> > > 
> > > 	* Endpoint A
> > > 	* Endpoint B + Endpoint C
> > 
> > Because:
> > 1) they are logically connected;
> > 2) the above is the exact citation from the specification and I would like to
> > keep it that way.
> > 
> > > As oppose to:
> > > 
> > > 	* Endpoint A
> > > 	* Endpoint B
> > > 	* Endpoint C
> 
> If you prefer to keep the snippet of your vendor specification intact,
> we can instead document this fully in the commit message and note the
> EBC feature. Remove these comments here.

I prefer to have a comment to explain magic numbers. I just want it to be
as closer as possible to the specification wording.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

[Thinh Nguyen]

On Tue, Jan 28, 2025, Andy Shevchenko wrote:
> On Tue, Jan 28, 2025 at 02:21:40AM +0000, Thinh Nguyen wrote:
> > 
> > If you prefer to keep the snippet of your vendor specification intact,
> > we can instead document this fully in the commit message and note the
> > EBC feature. Remove these comments here.
> 
> I prefer to have a comment to explain magic numbers. I just want it to be
> as closer as possible to the specification wording.
> 

Ok. That's fine.

BR,
Thinh

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Thinh Nguyen]

On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> (USB Device Controller mode) may be reserved for some special means, such as
> tracing. This series extends DT schema and driver to avoid using those.
> Without this the USB gadget mode won't work properly (those devices that
> "luckily" allocated the reserved EPs).
> 
> Ferry already tested the privately sent PoC of this, but I ask him again to
> re-test this version which is slightly different.
> 
> Andy Shevchenko (3):
>   dt-bindings: usb: dwc3: Add a property to reserve endpoints
>   usb: dwc3: gadget: Add support for snps,reserved-endpoints property
>   usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield
> 
>  .../devicetree/bindings/usb/snps,dwc3.yaml    | 10 +++++
>  drivers/usb/dwc3/dwc3-pci.c                   |  9 +++++
>  drivers/usb/dwc3/gadget.c                     | 38 ++++++++++++++++++-
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
> 

I'm not entirely clear on the reason for this change yet.

How would this even work without dwc3 managing these endpoints (all the
init/teardown/fifo allocation/start/stop flow). Can you provide more
info on this hardware?

Thanks,
Thinh

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Andy Shevchenko]

On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> > On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> > (USB Device Controller mode) may be reserved for some special means, such as
> > tracing. This series extends DT schema and driver to avoid using those.
> > Without this the USB gadget mode won't work properly (those devices that
> > "luckily" allocated the reserved EPs).
> > 
> > Ferry already tested the privately sent PoC of this, but I ask him again to
> > re-test this version which is slightly different.

...

> I'm not entirely clear on the reason for this change yet.
> 
> How would this even work without dwc3 managing these endpoints (all the
> init/teardown/fifo allocation/start/stop flow).

You perhaps know much better how it can be done, I have access to a limited
documentation and in practice if those endpoints are not skipped any gadget
that allocates them simply won't work, and IIRC the entire USB transfers are
stuck.

> Can you provide more info on this hardware?

I am afraid I can't provide more, sorry. I can look for some specifics,
but I'm not that guy who know anything about in-SoC tracing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> > > On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> > > (USB Device Controller mode) may be reserved for some special means, such as
> > > tracing. This series extends DT schema and driver to avoid using those.
> > > Without this the USB gadget mode won't work properly (those devices that
> > > "luckily" allocated the reserved EPs).
> > > 
> > > Ferry already tested the privately sent PoC of this, but I ask him again to
> > > re-test this version which is slightly different.

...

> > I'm not entirely clear on the reason for this change yet.
> > 
> > How would this even work without dwc3 managing these endpoints (all the
> > init/teardown/fifo allocation/start/stop flow).
> 
> You perhaps know much better how it can be done, I have access to a limited
> documentation and in practice if those endpoints are not skipped any gadget
> that allocates them simply won't work, and IIRC the entire USB transfers are
> stuck.
> 
> > Can you provide more info on this hardware?
> 
> I am afraid I can't provide more, sorry. I can look for some specifics,
> but I'm not that guy who know anything about in-SoC tracing.

So, here is what I found:

---8<---

However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
if being used because the sideband flow control signals are hard wired to certain
endpoints:
• 1 High BW Bulk IN (IN#1) (RTIT)
• 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)

In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
Trace Module) unit and the EXI target driver will as well configure the TRB location for
EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
these endpoints and interface to OTG3 controller through EBC interface, there is no
need to enable any events (such as XferComplete etc) for these end points.

Does it help you to understand the required quirk better?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Thinh Nguyen]

On Mon, Jan 20, 2025, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> > > > On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> > > > (USB Device Controller mode) may be reserved for some special means, such as
> > > > tracing. This series extends DT schema and driver to avoid using those.
> > > > Without this the USB gadget mode won't work properly (those devices that
> > > > "luckily" allocated the reserved EPs).
> > > > 
> > > > Ferry already tested the privately sent PoC of this, but I ask him again to
> > > > re-test this version which is slightly different.
> 
> ...
> 
> > > I'm not entirely clear on the reason for this change yet.
> > > 
> > > How would this even work without dwc3 managing these endpoints (all the
> > > init/teardown/fifo allocation/start/stop flow).
> > 
> > You perhaps know much better how it can be done, I have access to a limited
> > documentation and in practice if those endpoints are not skipped any gadget
> > that allocates them simply won't work, and IIRC the entire USB transfers are
> > stuck.
> > 
> > > Can you provide more info on this hardware?
> > 
> > I am afraid I can't provide more, sorry. I can look for some specifics,
> > but I'm not that guy who know anything about in-SoC tracing.
> 
> So, here is what I found:
> 
> ---8<---
> 
> However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
> if being used because the sideband flow control signals are hard wired to certain
> endpoints:
> • 1 High BW Bulk IN (IN#1) (RTIT)
> • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> 
> In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
> Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
> EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
> Trace Module) unit and the EXI target driver will as well configure the TRB location for
> EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
> these endpoints and interface to OTG3 controller through EBC interface, there is no
> need to enable any events (such as XferComplete etc) for these end points.
> 
> Does it help you to understand the required quirk better?
> 

Thanks for looking up the info. This makes more sense now. So these
endpoints use EBC. Can you also provide this info to the commit?

BR,
Thinh

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Andy Shevchenko]

On Tue, Jan 21, 2025 at 11:43:21PM +0000, Thinh Nguyen wrote:
> On Mon, Jan 20, 2025, Andy Shevchenko wrote:
> > On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > I'm not entirely clear on the reason for this change yet.
> > > > 
> > > > How would this even work without dwc3 managing these endpoints (all the
> > > > init/teardown/fifo allocation/start/stop flow).
> > > 
> > > You perhaps know much better how it can be done, I have access to a limited
> > > documentation and in practice if those endpoints are not skipped any gadget
> > > that allocates them simply won't work, and IIRC the entire USB transfers are
> > > stuck.
> > > 
> > > > Can you provide more info on this hardware?
> > > 
> > > I am afraid I can't provide more, sorry. I can look for some specifics,
> > > but I'm not that guy who know anything about in-SoC tracing.
> > 
> > So, here is what I found:
> > 
> > ---8<---
> > 
> > However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
> > if being used because the sideband flow control signals are hard wired to certain
> > endpoints:
> > • 1 High BW Bulk IN (IN#1) (RTIT)
> > • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > 
> > In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
> > Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
> > EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
> > Trace Module) unit and the EXI target driver will as well configure the TRB location for
> > EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
> > these endpoints and interface to OTG3 controller through EBC interface, there is no
> > need to enable any events (such as XferComplete etc) for these end points.
> > 
> > ---8<---
> > 
> > Does it help you to understand the required quirk better?
> 
> Thanks for looking up the info. This makes more sense now. So these
> endpoints use EBC. Can you also provide this info to the commit?

Sure, since I published it already it makes no difference if it appears in the
Git log (from the publicity point of view).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Thinh Nguyen]

On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> On Tue, Jan 21, 2025 at 11:43:21PM +0000, Thinh Nguyen wrote:
> > On Mon, Jan 20, 2025, Andy Shevchenko wrote:
> > > On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > I'm not entirely clear on the reason for this change yet.
> > > > > 
> > > > > How would this even work without dwc3 managing these endpoints (all the
> > > > > init/teardown/fifo allocation/start/stop flow).
> > > > 
> > > > You perhaps know much better how it can be done, I have access to a limited
> > > > documentation and in practice if those endpoints are not skipped any gadget
> > > > that allocates them simply won't work, and IIRC the entire USB transfers are
> > > > stuck.
> > > > 
> > > > > Can you provide more info on this hardware?
> > > > 
> > > > I am afraid I can't provide more, sorry. I can look for some specifics,
> > > > but I'm not that guy who know anything about in-SoC tracing.
> > > 
> > > So, here is what I found:
> > > 
> > > ---8<---
> > > 
> > > However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
> > > if being used because the sideband flow control signals are hard wired to certain
> > > endpoints:
> > > • 1 High BW Bulk IN (IN#1) (RTIT)
> > > • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > 
> > > In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
> > > Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
> > > EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
> > > Trace Module) unit and the EXI target driver will as well configure the TRB location for
> > > EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
> > > these endpoints and interface to OTG3 controller through EBC interface, there is no
> > > need to enable any events (such as XferComplete etc) for these end points.
> > > 
> > > ---8<---
> > > 
> > > Does it help you to understand the required quirk better?
> > 
> > Thanks for looking up the info. This makes more sense now. So these
> > endpoints use EBC. Can you also provide this info to the commit?
> 
> Sure, since I published it already it makes no difference if it appears in the
> Git log (from the publicity point of view).
> 

It's more difficult to find this outside of git log, especially to a
link version of a git change that's not applied.

BR,
Thinh

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Andy Shevchenko]

On Tue, Jan 28, 2025 at 02:25:21AM +0000, Thinh Nguyen wrote:
> On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> > On Tue, Jan 21, 2025 at 11:43:21PM +0000, Thinh Nguyen wrote:
> > > On Mon, Jan 20, 2025, Andy Shevchenko wrote:
> > > > On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > > > I'm not entirely clear on the reason for this change yet.
> > > > > > 
> > > > > > How would this even work without dwc3 managing these endpoints (all the
> > > > > > init/teardown/fifo allocation/start/stop flow).
> > > > > 
> > > > > You perhaps know much better how it can be done, I have access to a limited
> > > > > documentation and in practice if those endpoints are not skipped any gadget
> > > > > that allocates them simply won't work, and IIRC the entire USB transfers are
> > > > > stuck.
> > > > > 
> > > > > > Can you provide more info on this hardware?
> > > > > 
> > > > > I am afraid I can't provide more, sorry. I can look for some specifics,
> > > > > but I'm not that guy who know anything about in-SoC tracing.
> > > > 
> > > > So, here is what I found:
> > > > 
> > > > ---8<---
> > > > 
> > > > However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
> > > > if being used because the sideband flow control signals are hard wired to certain
> > > > endpoints:
> > > > • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > > 
> > > > In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
> > > > Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
> > > > EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
> > > > Trace Module) unit and the EXI target driver will as well configure the TRB location for
> > > > EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
> > > > these endpoints and interface to OTG3 controller through EBC interface, there is no
> > > > need to enable any events (such as XferComplete etc) for these end points.
> > > > 
> > > > ---8<---
> > > > 
> > > > Does it help you to understand the required quirk better?
> > > 
> > > Thanks for looking up the info. This makes more sense now. So these
> > > endpoints use EBC. Can you also provide this info to the commit?
> > 
> > Sure, since I published it already it makes no difference if it appears in the
> > Git log (from the publicity point of view).
> 
> It's more difficult to find this outside of git log, especially to a
> link version of a git change that's not applied.

I'm not objecting this, what I am telling is that information went public in
this thread, so expanding a commit message in the next version of the series
to include additional information is fine with me. I'll do that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] usb: dwc3: Avoid using reserved EPs

[Ferry Toth]

Op 16-01-2025 om 16:40 schreef Andy Shevchenko:
> On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> (USB Device Controller mode) may be reserved for some special means, such as
> tracing. This series extends DT schema and driver to avoid using those.
> Without this the USB gadget mode won't work properly (those devices that
> "luckily" allocated the reserved EPs).
> 
> Ferry already tested the privately sent PoC of this, but I ask him again to
> re-test this version which is slightly different.
> 
> Andy Shevchenko (3):
>    dt-bindings: usb: dwc3: Add a property to reserve endpoints
>    usb: dwc3: gadget: Add support for snps,reserved-endpoints property
>    usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield
> 
>   .../devicetree/bindings/usb/snps,dwc3.yaml    | 10 +++++
>   drivers/usb/dwc3/dwc3-pci.c                   |  9 +++++
>   drivers/usb/dwc3/gadget.c                     | 38 ++++++++++++++++++-
>   3 files changed, 56 insertions(+), 1 deletion(-)
> 
Yes I retested this now on v6.13.0-rc7 Intel Merrifield and found no 
problems. Skipping the tracing end point has definitely always been 
needed on this platform. Thanks!

Tested-by: Ferry Toth <fntoth@gmail.com>