[PATCH v3 0/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit

[PATCH v3 0/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit

[Yaqin Pan]

Hi,

Sorry for the late reply.

Changes in v3:
modify Documentation/devicetree/bindings/usb/snps,dwc3.yaml
because the errors in dt-binding check.

Changes in v2:
modify Documentation/devicetree/bindings/usb/snps,dwc3.yaml

Yaqin Pan (2):
  usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.
  dt-bindings: usb: document snps,sprs-ctrl-trans-quirk property in dwc3

 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
 drivers/usb/dwc3/core.c                              | 4 ++++
 drivers/usb/dwc3/core.h                              | 3 +++
 3 files changed, 13 insertions(+)

-- 
2.17.1

[PATCH v3 1/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.

[Yaqin Pan]

This quirk is only for dwc3 host mode.
the dwc3 controller can't emurate some devices successfully.
For example, TF card reader (aaaa:8816):
failed log
usb 1-1: new high-speed USB device number 2 using xhci-hcd
usb 1-1: device descriptor read/all, error -110
From the usb analyzer, always return NAK in the data phase.
if enable the GUCTL.SPRSCTRLTRANSEN bit. then the log is:
usb 2-1: new high-speed USB device number 3 using xhci-hcd
usb 2-1: New USB device found, idVendor=aaaa,
idProduct=8816, bcdDevice=13.08
usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 2-1: Product: MXT USB Device
usb 2-1: Manufacturer: MXTronics
usb 2-1: SerialNumber: 150101v01
usb 2-1: New USB device found, VID=aaaa, PID=8816

Some devices are slow in responding to Control transfers.
Scheduling mulitiple transactions in one microframe/frame
can cause the devices to misbehave. if this qurik is enabled,
the host controller schedules transations for a Control transfer
in defferent microframes/frame.

Signed-off-by: Yaqin Pan <akingchen@vivo.com>
---
 drivers/usb/dwc3/core.c | 4 ++++
 drivers/usb/dwc3/core.h | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index ba74ad7f6995..93ac2c79a2c0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1071,6 +1071,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		 * packet with Retry=1 & Nump != 0)
 		 */
 		reg |= DWC3_GUCTL_HSTINAUTORETRY;
+		if (dwc->sprs_ctrl_trans_quirk)
+			reg |= DWC3_GUCTL_SPRSCTRLTRANSEN;
 
 		dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
 	}
@@ -1377,6 +1379,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 
 	dwc->dis_split_quirk = device_property_read_bool(dev,
 				"snps,dis-split-quirk");
+	dwc->sprs_ctrl_trans_quirk = device_property_read_bool(dev,
+				"snps,sprs-ctrl-trans-quirk");
 
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5991766239ba..6048087df1d1 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -254,6 +254,7 @@
 
 /* Global User Control Register */
 #define DWC3_GUCTL_HSTINAUTORETRY	BIT(14)
+#define DWC3_GUCTL_SPRSCTRLTRANSEN	BIT(17)
 
 /* Global User Control 1 Register */
 #define DWC3_GUCTL1_PARKMODE_DISABLE_SS	BIT(17)
@@ -1077,6 +1078,7 @@ struct dwc3_scratchpad_array {
  *	3	- Reserved
  * @dis_metastability_quirk: set to disable metastability quirk.
  * @dis_split_quirk: set to disable split boundary.
+ * @sprs_ctrl_trans_quirk: set to enable sparse control transaction quirk.
  * @imod_interval: set the interrupt moderation interval in 250ns
  *			increments or 0 to disable.
  */
@@ -1279,6 +1281,7 @@ struct dwc3 {
 	unsigned		dis_metastability_quirk:1;
 
 	unsigned		dis_split_quirk:1;
+	unsigned		sprs_ctrl_trans_quirk:1;
 	unsigned		async_callbacks:1;
 
 	u16			imod_interval;
-- 
2.17.1

Re: [PATCH v3 1/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.

[Greg Kroah-Hartman]

On Thu, Dec 30, 2021 at 09:58:30PM +0800, Yaqin Pan wrote:
> This quirk is only for dwc3 host mode.
> the dwc3 controller can't emurate some devices successfully.
> For example, TF card reader (aaaa:8816):
> failed log
> usb 1-1: new high-speed USB device number 2 using xhci-hcd
> usb 1-1: device descriptor read/all, error -110
> >From the usb analyzer, always return NAK in the data phase.
> if enable the GUCTL.SPRSCTRLTRANSEN bit. then the log is:
> usb 2-1: new high-speed USB device number 3 using xhci-hcd
> usb 2-1: New USB device found, idVendor=aaaa,
> idProduct=8816, bcdDevice=13.08
> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> usb 2-1: Product: MXT USB Device
> usb 2-1: Manufacturer: MXTronics
> usb 2-1: SerialNumber: 150101v01
> usb 2-1: New USB device found, VID=aaaa, PID=8816
> 
> Some devices are slow in responding to Control transfers.
> Scheduling mulitiple transactions in one microframe/frame
> can cause the devices to misbehave. if this qurik is enabled,
> the host controller schedules transations for a Control transfer
> in defferent microframes/frame.

If this is needed for all devices (i.e. you do not know what device is
going to be plugged in), why not just enable it for all controllers?
Why whould you NOT want this enabled?

Or is this a broken hardware device and only specific host controllers
need this?  If so, how do we know which ones need this set and which do
not?

thanks,

greg k-h

Re: [PATCH v3 1/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.

[Yaqin Pan]

On Thu, 30 Dec 2021 15:12:27 +0100 Greg Kroah-Hartman wrote:
>> This quirk is only for dwc3 host mode.
>> the dwc3 controller can't emurate some devices successfully.
>> For example, TF card reader (aaaa:8816):
>> failed log
>> usb 1-1: new high-speed USB device number 2 using xhci-hcd
>> usb 1-1: device descriptor read/all, error -110
>> >From the usb analyzer, always return NAK in the data phase.
>> if enable the GUCTL.SPRSCTRLTRANSEN bit. then the log is:
>> usb 2-1: new high-speed USB device number 3 using xhci-hcd
>> usb 2-1: New USB device found, idVendor=aaaa,
>> idProduct=8816, bcdDevice=13.08
>> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>> usb 2-1: Product: MXT USB Device
>> usb 2-1: Manufacturer: MXTronics
>> usb 2-1: SerialNumber: 150101v01
>> usb 2-1: New USB device found, VID=aaaa, PID=8816
>> 
>> Some devices are slow in responding to Control transfers.
>> Scheduling mulitiple transactions in one microframe/frame
>> can cause the devices to misbehave. if this qurik is enabled,
>> the host controller schedules transations for a Control transfer
>> in defferent microframes/frame.
>
>If this is needed for all devices (i.e. you do not know what device is
>going to be plugged in), why not just enable it for all controllers?
>Why whould you NOT want this enabled?
>
>Or is this a broken hardware device and only specific host controllers
>need this?  If so, how do we know which ones need this set and which do
>not?

I think not all dwc3 controllers need this. For cell phone,customers may
use various usb devices, we can enable this quirk to fix some compatibility
issues. For some chip platform of qcom, i encounter this issue, not every
platform i encounter this problem.

If enabled for all controllers, it will reduce the speed of Control transfers. 
So i think it would be better for user to enable it by their own purposes.

Re: [PATCH v3 1/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.

[Greg KH]

On Thu, Dec 30, 2021 at 11:36:12PM +0800, Yaqin Pan wrote:
> On Thu, 30 Dec 2021 15:12:27 +0100 Greg Kroah-Hartman wrote:
> >> This quirk is only for dwc3 host mode.
> >> the dwc3 controller can't emurate some devices successfully.
> >> For example, TF card reader (aaaa:8816):
> >> failed log
> >> usb 1-1: new high-speed USB device number 2 using xhci-hcd
> >> usb 1-1: device descriptor read/all, error -110
> >> >From the usb analyzer, always return NAK in the data phase.
> >> if enable the GUCTL.SPRSCTRLTRANSEN bit. then the log is:
> >> usb 2-1: new high-speed USB device number 3 using xhci-hcd
> >> usb 2-1: New USB device found, idVendor=aaaa,
> >> idProduct=8816, bcdDevice=13.08
> >> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> >> usb 2-1: Product: MXT USB Device
> >> usb 2-1: Manufacturer: MXTronics
> >> usb 2-1: SerialNumber: 150101v01
> >> usb 2-1: New USB device found, VID=aaaa, PID=8816
> >> 
> >> Some devices are slow in responding to Control transfers.
> >> Scheduling mulitiple transactions in one microframe/frame
> >> can cause the devices to misbehave. if this qurik is enabled,
> >> the host controller schedules transations for a Control transfer
> >> in defferent microframes/frame.
> >
> >If this is needed for all devices (i.e. you do not know what device is
> >going to be plugged in), why not just enable it for all controllers?
> >Why whould you NOT want this enabled?
> >
> >Or is this a broken hardware device and only specific host controllers
> >need this?  If so, how do we know which ones need this set and which do
> >not?
> 
> I think not all dwc3 controllers need this. For cell phone,customers may
> use various usb devices, we can enable this quirk to fix some compatibility
> issues. For some chip platform of qcom, i encounter this issue, not every
> platform i encounter this problem.
> 
> If enabled for all controllers, it will reduce the speed of Control transfers. 
> So i think it would be better for user to enable it by their own purposes.

But how do hardware vendors know to enable this?  Can we trigger off of
PCI ids?  Do we need a list of quirks to show which host controllers are
broken this way?

Burying something as basic as "reliable device connection" in a DT quirk
seems very sloppy to me.  We want reliable systems, right?

thanks,

greg k-h

Re: [PATCH v3 1/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.

[Yaqin Pan]

On Thu, 30 Dec 2021 16:48:09 +0100 Greg Kroah-Hartman wrote:
>On Thu, Dec 30, 2021 at 11:36:12PM +0800, Yaqin Pan wrote:
>> On Thu, 30 Dec 2021 15:12:27 +0100 Greg Kroah-Hartman wrote:
>> >> This quirk is only for dwc3 host mode.
>> >> the dwc3 controller can't emurate some devices successfully.
>> >> For example, TF card reader (aaaa:8816):
>> >> failed log
>> >> usb 1-1: new high-speed USB device number 2 using xhci-hcd
>> >> usb 1-1: device descriptor read/all, error -110
>> >> >From the usb analyzer, always return NAK in the data phase.
>> >> if enable the GUCTL.SPRSCTRLTRANSEN bit. then the log is:
>> >> usb 2-1: new high-speed USB device number 3 using xhci-hcd
>> >> usb 2-1: New USB device found, idVendor=aaaa,
>> >> idProduct=8816, bcdDevice=13.08
>> >> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>> >> usb 2-1: Product: MXT USB Device
>> >> usb 2-1: Manufacturer: MXTronics
>> >> usb 2-1: SerialNumber: 150101v01
>> >> usb 2-1: New USB device found, VID=aaaa, PID=8816
>> >> 
>> >> Some devices are slow in responding to Control transfers.
>> >> Scheduling mulitiple transactions in one microframe/frame
>> >> can cause the devices to misbehave. if this qurik is enabled,
>> >> the host controller schedules transations for a Control transfer
>> >> in defferent microframes/frame.
>> >
>> >If this is needed for all devices (i.e. you do not know what device is
>> >going to be plugged in), why not just enable it for all controllers?
>> >Why whould you NOT want this enabled?
>> >
>> >Or is this a broken hardware device and only specific host controllers
>> >need this?  If so, how do we know which ones need this set and which do
>> >not?
>> 
>> I think not all dwc3 controllers need this. For cell phone,customers may
>> use various usb devices, we can enable this quirk to fix some compatibility
>> issues. For some chip platform of qcom, i encounter this issue, not every
>> platform i encounter this problem.
>> 
>> If enabled for all controllers, it will reduce the speed of Control transfers. 
>> So i think it would be better for user to enable it by their own purposes.
>
>But how do hardware vendors know to enable this?  Can we trigger off of
>PCI ids?  Do we need a list of quirks to show which host controllers are
>broken this way?
>
>Burying something as basic as "reliable device connection" in a DT quirk
>seems very sloppy to me.  We want reliable systems, right?

Yes, we want reliable systems. But i don't have a good ideal about this issue.
when we meet this problem, and from the dwc-usb3 controller datasheet,we know
enable one bit in dwc-usb3 controller's register can fixed this issue.

Of course, i can list the host controllers that i used broken this way if needed.

thanks,

Yaqin pan

Re: [PATCH v3 1/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.

[Greg KH]

On Fri, Dec 31, 2021 at 07:59:31PM +0800, Yaqin Pan wrote:
> On Thu, 30 Dec 2021 16:48:09 +0100 Greg Kroah-Hartman wrote:
> >On Thu, Dec 30, 2021 at 11:36:12PM +0800, Yaqin Pan wrote:
> >> On Thu, 30 Dec 2021 15:12:27 +0100 Greg Kroah-Hartman wrote:
> >> >> This quirk is only for dwc3 host mode.
> >> >> the dwc3 controller can't emurate some devices successfully.
> >> >> For example, TF card reader (aaaa:8816):
> >> >> failed log
> >> >> usb 1-1: new high-speed USB device number 2 using xhci-hcd
> >> >> usb 1-1: device descriptor read/all, error -110
> >> >> >From the usb analyzer, always return NAK in the data phase.
> >> >> if enable the GUCTL.SPRSCTRLTRANSEN bit. then the log is:
> >> >> usb 2-1: new high-speed USB device number 3 using xhci-hcd
> >> >> usb 2-1: New USB device found, idVendor=aaaa,
> >> >> idProduct=8816, bcdDevice=13.08
> >> >> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> >> >> usb 2-1: Product: MXT USB Device
> >> >> usb 2-1: Manufacturer: MXTronics
> >> >> usb 2-1: SerialNumber: 150101v01
> >> >> usb 2-1: New USB device found, VID=aaaa, PID=8816
> >> >> 
> >> >> Some devices are slow in responding to Control transfers.
> >> >> Scheduling mulitiple transactions in one microframe/frame
> >> >> can cause the devices to misbehave. if this qurik is enabled,
> >> >> the host controller schedules transations for a Control transfer
> >> >> in defferent microframes/frame.
> >> >
> >> >If this is needed for all devices (i.e. you do not know what device is
> >> >going to be plugged in), why not just enable it for all controllers?
> >> >Why whould you NOT want this enabled?
> >> >
> >> >Or is this a broken hardware device and only specific host controllers
> >> >need this?  If so, how do we know which ones need this set and which do
> >> >not?
> >> 
> >> I think not all dwc3 controllers need this. For cell phone,customers may
> >> use various usb devices, we can enable this quirk to fix some compatibility
> >> issues. For some chip platform of qcom, i encounter this issue, not every
> >> platform i encounter this problem.
> >> 
> >> If enabled for all controllers, it will reduce the speed of Control transfers. 
> >> So i think it would be better for user to enable it by their own purposes.
> >
> >But how do hardware vendors know to enable this?  Can we trigger off of
> >PCI ids?  Do we need a list of quirks to show which host controllers are
> >broken this way?
> >
> >Burying something as basic as "reliable device connection" in a DT quirk
> >seems very sloppy to me.  We want reliable systems, right?
> 
> Yes, we want reliable systems. But i don't have a good ideal about this issue.
> when we meet this problem, and from the dwc-usb3 controller datasheet,we know
> enable one bit in dwc-usb3 controller's register can fixed this issue.
> 
> Of course, i can list the host controllers that i used broken this way if needed.

Please have a list of controller that this is needed for, and add the
quirk for them only.  Don't require this to be in a DT file as that will
never be noticed.

thanks,

greg k-h

Re: [PATCH v3 1/2] usb: dwc3: Add a quirk to set GUCTL.SPRSCTRLTRANSEN bit.

[Yaqin Pan]

On Thu, 2022-01-03 13:35 UTC Greg Kroah-Hartman wrote:
>On Fri, Dec 31, 2021 at 07:59:31PM +0800, Yaqin Pan wrote:
>> On Thu, 30 Dec 2021 16:48:09 +0100 Greg Kroah-Hartman wrote:
>> >On Thu, Dec 30, 2021 at 11:36:12PM +0800, Yaqin Pan wrote:
>> >> On Thu, 30 Dec 2021 15:12:27 +0100 Greg Kroah-Hartman wrote:
>> >> >> This quirk is only for dwc3 host mode.
>> >> >> the dwc3 controller can't emurate some devices successfully.
>> >> >> For example, TF card reader (aaaa:8816):
>> >> >> failed log
>> >> >> usb 1-1: new high-speed USB device number 2 using xhci-hcd
>> >> >> usb 1-1: device descriptor read/all, error -110
>> >> >> >From the usb analyzer, always return NAK in the data phase.
>> >> >> if enable the GUCTL.SPRSCTRLTRANSEN bit. then the log is:
>> >> >> usb 2-1: new high-speed USB device number 3 using xhci-hcd
>> >> >> usb 2-1: New USB device found, idVendor=aaaa,
>> >> >> idProduct=8816, bcdDevice=13.08
>> >> >> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>> >> >> usb 2-1: Product: MXT USB Device
>> >> >> usb 2-1: Manufacturer: MXTronics
>> >> >> usb 2-1: SerialNumber: 150101v01
>> >> >> usb 2-1: New USB device found, VID=aaaa, PID=8816
>> >> >> 
>> >> >> Some devices are slow in responding to Control transfers.
>> >> >> Scheduling mulitiple transactions in one microframe/frame
>> >> >> can cause the devices to misbehave. if this qurik is enabled,
>> >> >> the host controller schedules transations for a Control transfer
>> >> >> in defferent microframes/frame.
>> >> >
>> >> >If this is needed for all devices (i.e. you do not know what device is
>> >> >going to be plugged in), why not just enable it for all controllers?
>> >> >Why whould you NOT want this enabled?
>> >> >
>> >> >Or is this a broken hardware device and only specific host controllers
>> >> >need this?  If so, how do we know which ones need this set and which do
>> >> >not?
>> >> 
>> >> I think not all dwc3 controllers need this. For cell phone,customers may
>> >> use various usb devices, we can enable this quirk to fix some compatibility
>> >> issues. For some chip platform of qcom, i encounter this issue, not every
>> >> platform i encounter this problem.
>> >> 
>> >> If enabled for all controllers, it will reduce the speed of Control transfers. 
>> >> So i think it would be better for user to enable it by their own purposes.
>> >
>> >But how do hardware vendors know to enable this?  Can we trigger off of
>> >PCI ids?  Do we need a list of quirks to show which host controllers are
>> >broken this way?
>> >
>> >Burying something as basic as "reliable device connection" in a DT quirk
>> >seems very sloppy to me.  We want reliable systems, right?
>> 
>> Yes, we want reliable systems. But i don't have a good ideal about this issue.
>> when we meet this problem, and from the dwc-usb3 controller datasheet,we know
>> enable one bit in dwc-usb3 controller's register can fixed this issue.
>> 
>> Of course, i can list the host controllers that i used broken this way if needed.
>
>Please have a list of controller that this is needed for, and add the
>quirk for them only.  Don't require this to be in a DT file as that will
>never be noticed.

The dwc3-core i list below:
qcom,sm8350-dwc3;
qcom,sm7325-dwc3;
qcom,sm6225-dwc3;
....
And i will try to contact with qcom for further help.

thanks,

Yaqin pan

[PATCH v3 2/2] dt-bindings: usb: document snps,sprs-ctrl-trans-quirk property in dwc3

[Yaqin Pan]

Add snps,sprs-ctrl-trans-quirk property for dwc3 controller

Signed-off-by: Yaqin Pan <akingchen@vivo.com>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 41416fbd92aa..7a127f0cb530 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -226,6 +226,12 @@ properties:
       avoid -EPROTO errors with usbhid on some devices (Hikey 970).
     type: boolean
 
+  snps,sprs-ctrl-trans-quirk:
+    description:
+      When set, change the way host controller schedules transations for a Control transfer.
+      Avoid failing to enumerate some devices due to usb compatibility issues.
+    type: boolean
+
   snps,is-utmi-l1-suspend:
     description:
       True when DWC3 asserts output signal utmi_l1_suspend_n, false when
-- 
2.17.1