[PATCH v2 0/2] Add support to ignore single SE0 glitches

[PATCH v2 0/2] Add support to ignore single SE0 glitches

[Uttkarsh Aggarwal]

Currently in few of Qualcomm chips USB (Low speed) mouse not
detected showing following errors:

  usb 1-1: Device not responding to setup address.
  usb 1-1: device not accepting address 2, error -71
  usb 1-1: new low-speed USB device number 3 using xhci-hcd
  usb 1-1: Device not responding to setup address.
  usb 1-1: Device not responding to setup address.
  usb 1-1: device not accepting address 3, error -71
  usb usb1-port1: attempt power cycle

Based on the Logic analyzer waveforms, It has been identified that there
is skew of about 8nS b/w DP & DM linestate signals (o/p of PHY & i/p to
controller) at the UTMI interface, Due to this controller is seeing SE0
glitch condition, this is causing controller to pre-maturely assume that
PHY has sent all the data & is initiating next packet much early, though
in reality PHY is still busy sending previous packets.

Enabling the GUCTL1.FILTER_SE0_FSLS_EOP bit29 allows the controller to
ignore single SE0 glitches on the linestate during transmission. Only two
or more SE0 signals are recognized as a valid EOP.

When this feature is activated, SE0 signals on the linestate are validated
over two consecutive UTMI/ULPI clock edges for EOP detection.

Device mode (FS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then for device LPM
handshake, the controller ignores single SE0 glitch on the linestate during
transmit. Only two or more SE0 is considered as a valid EOP on FS port.

Host mode (FS/LS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then the controller
ignores single SE0 glitch on the linestate during transmit.

DT patch will be sent separately.

Changes in v2:
Included bindings update for the quirk.
Updated commit text for core patch.

Link to v1:
https://lore.kernel.org/all/20240823055642.27638-1-quic_uaggarwa@quicinc.com/

Uttkarsh Aggarwal (2):
  dt-bindings: usb: snps,dwc3: Add snps,filter-se0-fsls-eop quirk
  usb: dwc3: core: Add support to ignore single SE0 glitches

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

-- 
2.17.1

[PATCH v2 1/2] dt-bindings: usb: snps,dwc3: Add snps,filter-se0-fsls-eop quirk

[Uttkarsh Aggarwal]

Adding a new 'snps,filter-se0-fsls-eop quirk' DT quirk to dwc3 core to set
GUCTL1 BIT 29. When set, controller will ignore single SE0 glitch on the
linestate during transmission. Only two or more SE0 is considered as
valid EOP on FS/LS port. This bit is applicable only in FS in device mode
and FS/LS mode of operation in host mode.

Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.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 1cd0ca90127d..d9e813bbcd80 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -180,6 +180,12 @@ properties:
     description: When set core will set Tx de-emphasis value
     type: boolean
 
+  snps,filter-se0-fsls-eop-quirk:
+    description:
+      When set controller will ignore single SE0 glitch on the linestate during transmit
+      Only two or more SE0 is considered as a valid EOP on FS/LS port.
+    type: boolean
+
   snps,tx_de_emphasis:
     description:
       The value driven to the PHY is controlled by the LTSSM during USB3
-- 
2.17.1

[PATCH v2 2/2] usb: dwc3: core: Add support to ignore single SE0 glitches

[Uttkarsh Aggarwal]

Currently in few of Qualcomm chips USB (Low speed) mouse not
detected showing following errors:

  usb 1-1: Device not responding to setup address.
  usb 1-1: device not accepting address 2, error -71
  usb 1-1: new low-speed USB device number 3 using xhci-hcd
  usb 1-1: Device not responding to setup address.
  usb 1-1: Device not responding to setup address.
  usb 1-1: device not accepting address 3, error -71
  usb usb1-port1: attempt power cycle

Based on the Logic analyzer waveforms, It has been identified that there
is skew of about 8nS b/w DP & DM linestate signals (o/p of PHY & i/p to
controller) at the UTMI interface, Due to this controller is seeing SE0
glitch condition, this is causing controller to pre-maturely assume that
PHY has sent all the data & is initiating next packet much early, though
in reality PHY is still busy sending previous packets.

Enabling the GUCTL1.FILTER_SE0_FSLS_EOP bit29 allows the controller to
ignore single SE0 glitches on the linestate during transmission. Only two
or more SE0 signals are recognized as a valid EOP.

When this feature is activated, SE0 signals on the linestate are validated
over two consecutive UTMI/ULPI clock edges for EOP detection.

Device mode (FS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then for device LPM
handshake, the controller ignores single SE0 glitch on the linestate during
transmit. Only two or more SE0 is considered as a valid EOP on FS port.

Host mode (FS/LS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then the controller
ignores single SE0 glitch on the linestate during transmit.

Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.com>
---
 drivers/usb/dwc3/core.c | 13 +++++++++++++
 drivers/usb/dwc3/core.h |  4 ++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 86b37881aab4..4edd32c44e73 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -222,6 +222,17 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	switch (desired_dr_role) {
 	case DWC3_GCTL_PRTCAP_HOST:
+	       /*
+		* Setting GUCTL1 bit 29 so that controller
+		* will ignore single SE0 glitch on the linestate
+		* during transmit.
+		*/
+		if (dwc->filter_se0_fsls_eop_quirk) {
+			reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+			reg |= DWC3_GUCTL1_FILTER_SE0_FSLS_EOP;
+			dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
+		}
+
 		ret = dwc3_host_init(dwc);
 		if (ret) {
 			dev_err(dwc->dev, "failed to initialize host\n");
@@ -1788,6 +1799,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 
 	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
 				"snps,tx_de_emphasis_quirk");
+	dwc->filter_se0_fsls_eop_quirk = device_property_read_bool(dev,
+				"snps,filter-se0-fsls-eop-quirk");
 	device_property_read_u8(dev, "snps,tx_de_emphasis",
 				&tx_de_emphasis);
 	device_property_read_string(dev, "snps,hsphy_interface",
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index cc3f32acfaf5..33d53a436fd7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -276,6 +276,7 @@
 
 /* Global User Control 1 Register */
 #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
+#define DWC3_GUCTL1_FILTER_SE0_FSLS_EOP		BIT(29)
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
 #define DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK	BIT(26)
 #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
@@ -1140,6 +1141,8 @@ struct dwc3_scratchpad_array {
  * @gfladj_refclk_lpm_sel: set if we need to enable SOF/ITP counter
  *                          running based on ref_clk
  * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
+ * @filter_se0_fsls_eop_quirk: set to ignores single
+ *				SE0 glitch on the linestate during transmit.
  * @tx_de_emphasis: Tx de-emphasis value
  *	0	- -6dB de-emphasis
  *	1	- -3.5dB de-emphasis
@@ -1373,6 +1376,7 @@ struct dwc3 {
 	unsigned		gfladj_refclk_lpm_sel:1;
 
 	unsigned		tx_de_emphasis_quirk:1;
+	unsigned		filter_se0_fsls_eop_quirk:1;
 	unsigned		tx_de_emphasis:2;
 
 	unsigned		dis_metastability_quirk:1;
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings: usb: snps,dwc3: Add snps,filter-se0-fsls-eop quirk

[Krzysztof Kozlowski]

On Thu, Oct 17, 2024 at 05:10:54PM +0530, Uttkarsh Aggarwal wrote:
> Adding a new 'snps,filter-se0-fsls-eop quirk' DT quirk to dwc3 core to set
> GUCTL1 BIT 29. When set, controller will ignore single SE0 glitch on the
> linestate during transmission. Only two or more SE0 is considered as
> valid EOP on FS/LS port. This bit is applicable only in FS in device mode
> and FS/LS mode of operation in host mode.

Why this is not device/compatible specific? Just like all other quirks
pushed last one year.

> 
> Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.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 1cd0ca90127d..d9e813bbcd80 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -180,6 +180,12 @@ properties:
>      description: When set core will set Tx de-emphasis value
>      type: boolean
>  
> +  snps,filter-se0-fsls-eop-quirk:
> +    description:
> +      When set controller will ignore single SE0 glitch on the linestate during transmit

Does not look like wrapped according to coding style (checkpatch is not
a coding style document).

Best regards,
Krzysztof

Re: [PATCH v2 2/2] usb: dwc3: core: Add support to ignore single SE0 glitches

[AKASH KUMAR]

Hi Uttkarsh, Thinh,

On 10/17/2024 5:10 PM, Uttkarsh Aggarwal wrote:
> Currently in few of Qualcomm chips USB (Low speed) mouse not
> detected showing following errors:
>
>    usb 1-1: Device not responding to setup address.
>    usb 1-1: device not accepting address 2, error -71
>    usb 1-1: new low-speed USB device number 3 using xhci-hcd
>    usb 1-1: Device not responding to setup address.
>    usb 1-1: Device not responding to setup address.
>    usb 1-1: device not accepting address 3, error -71
>    usb usb1-port1: attempt power cycle
>
> Based on the Logic analyzer waveforms, It has been identified that there
> is skew of about 8nS b/w DP & DM linestate signals (o/p of PHY & i/p to
> controller) at the UTMI interface, Due to this controller is seeing SE0
> glitch condition, this is causing controller to pre-maturely assume that
> PHY has sent all the data & is initiating next packet much early, though
> in reality PHY is still busy sending previous packets.
>
> Enabling the GUCTL1.FILTER_SE0_FSLS_EOP bit29 allows the controller to
> ignore single SE0 glitches on the linestate during transmission. Only two
> or more SE0 signals are recognized as a valid EOP.
>
> When this feature is activated, SE0 signals on the linestate are validated
> over two consecutive UTMI/ULPI clock edges for EOP detection.
>
> Device mode (FS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then for device LPM
> handshake, the controller ignores single SE0 glitch on the linestate during
> transmit. Only two or more SE0 is considered as a valid EOP on FS port.
>
> Host mode (FS/LS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then the controller
> ignores single SE0 glitch on the linestate during transmit.
>
> Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.com>
> ---
>   drivers/usb/dwc3/core.c | 13 +++++++++++++
>   drivers/usb/dwc3/core.h |  4 ++++
>   2 files changed, 17 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 86b37881aab4..4edd32c44e73 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -222,6 +222,17 @@ static void __dwc3_set_mode(struct work_struct *work)
>   
>   	switch (desired_dr_role) {
>   	case DWC3_GCTL_PRTCAP_HOST:
> +	       /*
> +		* Setting GUCTL1 bit 29 so that controller
> +		* will ignore single SE0 glitch on the linestate
> +		* during transmit.
> +		*/
> +		if (dwc->filter_se0_fsls_eop_quirk) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> +			reg |= DWC3_GUCTL1_FILTER_SE0_FSLS_EOP;
> +			dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> +		}
> +
Since this bit is useful for device mode as well along with host mode,we 
should set this in dwc3_core_init
where we are already writing for GUCTL1 bit disable parkmode, instead of 
host mode only. Like below

--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1444,6 +1444,9 @@ static int dwc3_core_init(struct dwc3 *dwc)
                 if (dwc->parkmode_disable_hs_quirk)
                         reg |= DWC3_GUCTL1_PARKMODE_DISABLE_HS;

+               if (dwc->filter_se0_fsls_eop_quirk)
+                       reg |= DWC3_GUCTL1_FILTER_SE0_FSLS_EOP;
+
                 if (DWC3_VER_IS_WITHIN(DWC3, 290A, ANY)) {
                         if (dwc->maximum_speed == USB_SPEED_FULL ||
                             dwc->maximum_speed == USB_SPEED_HIGH)
>   		ret = dwc3_host_init(dwc);
>   		if (ret) {
>   			dev_err(dwc->dev, "failed to initialize host\n");
> @@ -1788,6 +1799,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>   
>   	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
>   				"snps,tx_de_emphasis_quirk");
> +	dwc->filter_se0_fsls_eop_quirk = device_property_read_bool(dev,
> +				"snps,filter-se0-fsls-eop-quirk");
>   	device_property_read_u8(dev, "snps,tx_de_emphasis",
>   				&tx_de_emphasis);
>   	device_property_read_string(dev, "snps,hsphy_interface",
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index cc3f32acfaf5..33d53a436fd7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -276,6 +276,7 @@
>   
>   /* Global User Control 1 Register */
>   #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
> +#define DWC3_GUCTL1_FILTER_SE0_FSLS_EOP		BIT(29)
>   #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
>   #define DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK	BIT(26)
>   #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
> @@ -1140,6 +1141,8 @@ struct dwc3_scratchpad_array {
>    * @gfladj_refclk_lpm_sel: set if we need to enable SOF/ITP counter
>    *                          running based on ref_clk
>    * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
> + * @filter_se0_fsls_eop_quirk: set to ignores single
> + *				SE0 glitch on the linestate during transmit.
>    * @tx_de_emphasis: Tx de-emphasis value
>    *	0	- -6dB de-emphasis
>    *	1	- -3.5dB de-emphasis
> @@ -1373,6 +1376,7 @@ struct dwc3 {
>   	unsigned		gfladj_refclk_lpm_sel:1;
>   
>   	unsigned		tx_de_emphasis_quirk:1;
> +	unsigned		filter_se0_fsls_eop_quirk:1;
>   	unsigned		tx_de_emphasis:2;
>   
>   	unsigned		dis_metastability_quirk:1;
Thanks,
Akash

Re: [PATCH v2 2/2] usb: dwc3: core: Add support to ignore single SE0 glitches

[AKASH KUMAR]

Hi Uttkarsh, Thinh,

On 10/17/2024 5:10 PM, Uttkarsh Aggarwal wrote:
> Currently in few of Qualcomm chips USB (Low speed) mouse not
> detected showing following errors:
>
>    usb 1-1: Device not responding to setup address.
>    usb 1-1: device not accepting address 2, error -71
>    usb 1-1: new low-speed USB device number 3 using xhci-hcd
>    usb 1-1: Device not responding to setup address.
>    usb 1-1: Device not responding to setup address.
>    usb 1-1: device not accepting address 3, error -71
>    usb usb1-port1: attempt power cycle
>
> Based on the Logic analyzer waveforms, It has been identified that there
> is skew of about 8nS b/w DP & DM linestate signals (o/p of PHY & i/p to
> controller) at the UTMI interface, Due to this controller is seeing SE0
> glitch condition, this is causing controller to pre-maturely assume that
> PHY has sent all the data & is initiating next packet much early, though
> in reality PHY is still busy sending previous packets.
>
> Enabling the GUCTL1.FILTER_SE0_FSLS_EOP bit29 allows the controller to
> ignore single SE0 glitches on the linestate during transmission. Only two
> or more SE0 signals are recognized as a valid EOP.
>
> When this feature is activated, SE0 signals on the linestate are validated
> over two consecutive UTMI/ULPI clock edges for EOP detection.
>
> Device mode (FS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then for device LPM
> handshake, the controller ignores single SE0 glitch on the linestate during
> transmit. Only two or more SE0 is considered as a valid EOP on FS port.
>
> Host mode (FS/LS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then the controller
> ignores single SE0 glitch on the linestate during transmit.
>
> Signed-off-by: Uttkarsh Aggarwal<quic_uaggarwa@quicinc.com>
> ---
>   drivers/usb/dwc3/core.c | 13 +++++++++++++
>   drivers/usb/dwc3/core.h |  4 ++++
>   2 files changed, 17 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 86b37881aab4..4edd32c44e73 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -222,6 +222,17 @@ static void __dwc3_set_mode(struct work_struct *work)
>   
>   	switch (desired_dr_role) {
>   	case DWC3_GCTL_PRTCAP_HOST:
> +	       /*
> +		* Setting GUCTL1 bit 29 so that controller
> +		* will ignore single SE0 glitch on the linestate
> +		* during transmit.
> +		*/
> +		if (dwc->filter_se0_fsls_eop_quirk) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> +			reg |= DWC3_GUCTL1_FILTER_SE0_FSLS_EOP;
> +			dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> +		}
> +
Since this bit is useful for device mode as well along with host mode,we 
should set this in dwc3_core_init
where we are already writing for GUCTL1 bit disable parkmode, instead of 
host mode only. Like below

--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1444,6 +1444,9 @@ static int dwc3_core_init(struct dwc3 *dwc)
                 if (dwc->parkmode_disable_hs_quirk)
                         reg |= DWC3_GUCTL1_PARKMODE_DISABLE_HS;

+               if (dwc->filter_se0_fsls_eop_quirk)
+                       reg |= DWC3_GUCTL1_FILTER_SE0_FSLS_EOP;
+
                 if (DWC3_VER_IS_WITHIN(DWC3, 290A, ANY)) {
                         if (dwc->maximum_speed == USB_SPEED_FULL ||
                             dwc->maximum_speed == USB_SPEED_HIGH)
>   		ret = dwc3_host_init(dwc);
>   		if (ret) {
>   			dev_err(dwc->dev, "failed to initialize host\n");
> @@ -1788,6 +1799,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>   
>   	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
>   				"snps,tx_de_emphasis_quirk");
> +	dwc->filter_se0_fsls_eop_quirk = device_property_read_bool(dev,
> +				"snps,filter-se0-fsls-eop-quirk");
>   	device_property_read_u8(dev, "snps,tx_de_emphasis",
>   				&tx_de_emphasis);
>   	device_property_read_string(dev, "snps,hsphy_interface",
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index cc3f32acfaf5..33d53a436fd7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -276,6 +276,7 @@
>   
>   /* Global User Control 1 Register */
>   #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
> +#define DWC3_GUCTL1_FILTER_SE0_FSLS_EOP		BIT(29)
>   #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
>   #define DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK	BIT(26)
>   #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
> @@ -1140,6 +1141,8 @@ struct dwc3_scratchpad_array {
>    * @gfladj_refclk_lpm_sel: set if we need to enable SOF/ITP counter
>    *                          running based on ref_clk
>    * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
> + * @filter_se0_fsls_eop_quirk: set to ignores single
> + *				SE0 glitch on the linestate during transmit.
>    * @tx_de_emphasis: Tx de-emphasis value
>    *	0	- -6dB de-emphasis
>    *	1	- -3.5dB de-emphasis
> @@ -1373,6 +1376,7 @@ struct dwc3 {
>   	unsigned		gfladj_refclk_lpm_sel:1;
>   
>   	unsigned		tx_de_emphasis_quirk:1;
> +	unsigned		filter_se0_fsls_eop_quirk:1;
>   	unsigned		tx_de_emphasis:2;
>   
>   	unsigned		dis_metastability_quirk:1;

Thanks,
Akash

Re: [PATCH v2 1/2] dt-bindings: usb: snps,dwc3: Add snps,filter-se0-fsls-eop quirk

[Krishna Kurapati]

On 10/18/2024 11:57 AM, Krzysztof Kozlowski wrote:
> On Thu, Oct 17, 2024 at 05:10:54PM +0530, Uttkarsh Aggarwal wrote:
>> Adding a new 'snps,filter-se0-fsls-eop quirk' DT quirk to dwc3 core to set
>> GUCTL1 BIT 29. When set, controller will ignore single SE0 glitch on the
>> linestate during transmission. Only two or more SE0 is considered as
>> valid EOP on FS/LS port. This bit is applicable only in FS in device mode
>> and FS/LS mode of operation in host mode.
> 
> Why this is not device/compatible specific? Just like all other quirks
> pushed last one year.

Hi Krzysztof,

  Apologies for a late reply from our end.

  In DWC3 core/dwc3-qcom atleast, there have been no compatible specific 
quirks added. Also since this is a property of the Synopsys controller 
hardware and not QC specific one, can we add it in bindings itself. 
Because this is a property other vendors might also use and adding it 
via compatible might not be appropriate.

  Let us know your thoughts on this.

Regards,
Krishna,

> 
>>
>> Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.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 1cd0ca90127d..d9e813bbcd80 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -180,6 +180,12 @@ properties:
>>       description: When set core will set Tx de-emphasis value
>>       type: boolean
>>   
>> +  snps,filter-se0-fsls-eop-quirk:
>> +    description:
>> +      When set controller will ignore single SE0 glitch on the linestate during transmit
> 
> Does not look like wrapped according to coding style (checkpatch is not
> a coding style document).
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v2 2/2] usb: dwc3: core: Add support to ignore single SE0 glitches

[UTTKARSH AGGARWAL]

On 10/18/2024 4:32 PM, AKASH KUMAR wrote:
> Hi Uttkarsh, Thinh,
>
> On 10/17/2024 5:10 PM, Uttkarsh Aggarwal wrote:
>> Currently in few of Qualcomm chips USB (Low speed) mouse not
>> detected showing following errors:
>>
>>    usb 1-1: Device not responding to setup address.
>>    usb 1-1: device not accepting address 2, error -71
>>    usb 1-1: new low-speed USB device number 3 using xhci-hcd
>>    usb 1-1: Device not responding to setup address.
>>    usb 1-1: Device not responding to setup address.
>>    usb 1-1: device not accepting address 3, error -71
>>    usb usb1-port1: attempt power cycle
>>
>> Based on the Logic analyzer waveforms, It has been identified that there
>> is skew of about 8nS b/w DP & DM linestate signals (o/p of PHY & i/p to
>> controller) at the UTMI interface, Due to this controller is seeing SE0
>> glitch condition, this is causing controller to pre-maturely assume that
>> PHY has sent all the data & is initiating next packet much early, though
>> in reality PHY is still busy sending previous packets.
>>
>> Enabling the GUCTL1.FILTER_SE0_FSLS_EOP bit29 allows the controller to
>> ignore single SE0 glitches on the linestate during transmission. Only 
>> two
>> or more SE0 signals are recognized as a valid EOP.
>>
>> When this feature is activated, SE0 signals on the linestate are 
>> validated
>> over two consecutive UTMI/ULPI clock edges for EOP detection.
>>
>> Device mode (FS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then for 
>> device LPM
>> handshake, the controller ignores single SE0 glitch on the linestate 
>> during
>> transmit. Only two or more SE0 is considered as a valid EOP on FS port.
>>
>> Host mode (FS/LS): If GUCTL1.FILTER_SE0_FSLS_EOP is set, then the 
>> controller
>> ignores single SE0 glitch on the linestate during transmit.
>>
>> Signed-off-by: Uttkarsh Aggarwal<quic_uaggarwa@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c | 13 +++++++++++++
>>   drivers/usb/dwc3/core.h |  4 ++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 86b37881aab4..4edd32c44e73 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -222,6 +222,17 @@ static void __dwc3_set_mode(struct work_struct 
>> *work)
>>         switch (desired_dr_role) {
>>       case DWC3_GCTL_PRTCAP_HOST:
>> +           /*
>> +        * Setting GUCTL1 bit 29 so that controller
>> +        * will ignore single SE0 glitch on the linestate
>> +        * during transmit.
>> +        */
>> +        if (dwc->filter_se0_fsls_eop_quirk) {
>> +            reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>> +            reg |= DWC3_GUCTL1_FILTER_SE0_FSLS_EOP;
>> +            dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>> +        }
>> +
> Since this bit is useful for device mode as well along with host 
> mode,we should set this in dwc3_core_init
> where we are already writing for GUCTL1 bit disable parkmode, instead 
> of host mode only. Like below
>
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1444,6 +1444,9 @@ static int dwc3_core_init(struct dwc3 *dwc)
>                 if (dwc->parkmode_disable_hs_quirk)
>                         reg |= DWC3_GUCTL1_PARKMODE_DISABLE_HS;
>
> +               if (dwc->filter_se0_fsls_eop_quirk)
> +                       reg |= DWC3_GUCTL1_FILTER_SE0_FSLS_EOP;
> +
>                 if (DWC3_VER_IS_WITHIN(DWC3, 290A, ANY)) {
>                         if (dwc->maximum_speed == USB_SPEED_FULL ||
>                             dwc->maximum_speed == USB_SPEED_HIGH)
>>           ret = dwc3_host_init(dwc);
>>           if (ret) {
>>               dev_err(dwc->dev, "failed to initialize host\n");
>> @@ -1788,6 +1799,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>         dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
>>                   "snps,tx_de_emphasis_quirk");
>> +    dwc->filter_se0_fsls_eop_quirk = device_property_read_bool(dev,
>> +                "snps,filter-se0-fsls-eop-quirk");
>>       device_property_read_u8(dev, "snps,tx_de_emphasis",
>>                   &tx_de_emphasis);
>>       device_property_read_string(dev, "snps,hsphy_interface",
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index cc3f32acfaf5..33d53a436fd7 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -276,6 +276,7 @@
>>     /* Global User Control 1 Register */
>>   #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT    BIT(31)
>> +#define DWC3_GUCTL1_FILTER_SE0_FSLS_EOP        BIT(29)
>>   #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS    BIT(28)
>>   #define DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK    BIT(26)
>>   #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW        BIT(24)
>> @@ -1140,6 +1141,8 @@ struct dwc3_scratchpad_array {
>>    * @gfladj_refclk_lpm_sel: set if we need to enable SOF/ITP counter
>>    *                          running based on ref_clk
>>    * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
>> + * @filter_se0_fsls_eop_quirk: set to ignores single
>> + *                SE0 glitch on the linestate during transmit.
>>    * @tx_de_emphasis: Tx de-emphasis value
>>    *    0    - -6dB de-emphasis
>>    *    1    - -3.5dB de-emphasis
>> @@ -1373,6 +1376,7 @@ struct dwc3 {
>>       unsigned        gfladj_refclk_lpm_sel:1;
>>         unsigned        tx_de_emphasis_quirk:1;
>> +    unsigned        filter_se0_fsls_eop_quirk:1;
>>       unsigned        tx_de_emphasis:2;
>>         unsigned        dis_metastability_quirk:1;
>
> Thanks,
> Akash


Hi Akash,

Apologies for a late reply.

About device mode use case of this bit, there is the possibility that a 
device may enter L1 earlier than

the Host LPM Retry Time in the case when a device ACK handshake is 
dropped due to errors.

Thus, this bit is not normally required to be set in device mode.


Thanks,

Uttkarsh

Re: [PATCH v2 1/2] dt-bindings: usb: snps,dwc3: Add snps,filter-se0-fsls-eop quirk

[Krzysztof Kozlowski]

On 07/11/2024 07:17, Krishna Kurapati wrote:
> 
> 
> On 10/18/2024 11:57 AM, Krzysztof Kozlowski wrote:
>> On Thu, Oct 17, 2024 at 05:10:54PM +0530, Uttkarsh Aggarwal wrote:
>>> Adding a new 'snps,filter-se0-fsls-eop quirk' DT quirk to dwc3 core to set
>>> GUCTL1 BIT 29. When set, controller will ignore single SE0 glitch on the
>>> linestate during transmission. Only two or more SE0 is considered as
>>> valid EOP on FS/LS port. This bit is applicable only in FS in device mode
>>> and FS/LS mode of operation in host mode.
>>
>> Why this is not device/compatible specific? Just like all other quirks
>> pushed last one year.
> 
> Hi Krzysztof,
> 
>   Apologies for a late reply from our end.
> 
>   In DWC3 core/dwc3-qcom atleast, there have been no compatible specific 
> quirks added. 

Nothing stops from adding these, I think.

> Also since this is a property of the Synopsys controller
> hardware and not QC specific one, can we add it in bindings itself. 
> Because this is a property other vendors might also use and adding it 
> via compatible might not be appropriate.

This does no answer my question. I don't see how this is not related to
one specific piece of SoC.

If you claim this is board-related, not SoC, give some arguments.
Repeating the same is just no helping.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: usb: snps,dwc3: Add snps,filter-se0-fsls-eop quirk

[Krishna Kurapati]

On 11/7/2024 3:25 PM, Krzysztof Kozlowski wrote:
> On 07/11/2024 07:17, Krishna Kurapati wrote:
>>
>>
>> On 10/18/2024 11:57 AM, Krzysztof Kozlowski wrote:
>>> On Thu, Oct 17, 2024 at 05:10:54PM +0530, Uttkarsh Aggarwal wrote:
>>>> Adding a new 'snps,filter-se0-fsls-eop quirk' DT quirk to dwc3 core to set
>>>> GUCTL1 BIT 29. When set, controller will ignore single SE0 glitch on the
>>>> linestate during transmission. Only two or more SE0 is considered as
>>>> valid EOP on FS/LS port. This bit is applicable only in FS in device mode
>>>> and FS/LS mode of operation in host mode.
>>>
>>> Why this is not device/compatible specific? Just like all other quirks
>>> pushed last one year.
>>
>> Hi Krzysztof,
>>
>>    Apologies for a late reply from our end.
>>
>>    In DWC3 core/dwc3-qcom atleast, there have been no compatible specific
>> quirks added.
> 

Sorry again for late reply.

> Nothing stops from adding these, I think.
> 

Agree, we can take that approach of adding soc specific compatibles to 
dwc3 driver instead of adding through bindings.

>> Also since this is a property of the Synopsys controller
>> hardware and not QC specific one, can we add it in bindings itself.
>> Because this is a property other vendors might also use and adding it
>> via compatible might not be appropriate.
> 
> This does no answer my question. I don't see how this is not related to
> one specific piece of SoC.
> 
> If you claim this is board-related, not SoC, give some arguments.
> Repeating the same is just no helping.
> 

But my point was that although the issue was found only on some QC 
SoC's, the solution still lies in some bits being set in controller 
register space and it is part of Synopsys IP. So wouldn't officially we 
add that support in bindings and then enable/disable the feature via DT 
like we did for other quirks ? If many SoC's need it in future, the 
driver needs to add a long list of compatible specific data which 
otherwise might be quirks in DT.

Regards,
Krishna,