linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
@ 2017-04-18  5:17 William Wu
  2017-04-18 13:18 ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: William Wu @ 2017-04-18  5:17 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: heiko, linux-kernel, linux-usb, linux-rockchip, devicetree,
	robh+dt, frank.wang, huangtao, dianders, briannorris, groeck,
	daniel.meng, wulf, William Wu

This patch adds a quirk to disable USB 2.0 MAC linestate check
during HS transmit. Refer the dwc3 databook, we can use it for
some special platforms if the linestate not reflect the expected
line state(J) during transmission.

When use this quirk, the controller implements a fixed 40-bit
TxEndDelay after the packet is given on UTMI and ignores the
linestate during the transmit of a token (during token-to-token
and token-to-data IPGAP).

On some rockchip platforms (e.g. rk3399), it requires to disable
the u2mac linestate check to decrease the SSPLIT token to SETUP
token inter-packet delay from 566ns to 466ns, and fix the issue
that FS/LS devices not recognized if inserted through USB 3.0 HUB.

Signed-off-by: William Wu <william.wu@rock-chips.com>
---
Changes in v2:
- fix coding style

 Documentation/devicetree/bindings/usb/dwc3.txt |  2 ++
 drivers/usb/dwc3/core.c                        | 14 ++++++++++----
 drivers/usb/dwc3/core.h                        |  4 ++++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index f658f39..6a89f0c 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -45,6 +45,8 @@ Optional properties:
 			a free-running PHY clock.
  - snps,dis-del-phy-power-chg-quirk: when set core will change PHY power
 			from P0 to P1/P2/P3 without delay.
+ - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate check
+			during HS transmit.
  - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
 			utmi_l1_suspend_n, false when asserts utmi_sleep_n
  - snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 455d89a..03429c5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
 	}
 
+	reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+
 	/*
 	 * Enable hardware control of sending remote wakeup in HS when
 	 * the device is in the L1 state.
 	 */
-	if (dwc->revision >= DWC3_REVISION_290A) {
-		reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+	if (dwc->revision >= DWC3_REVISION_290A)
 		reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
-		dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
-	}
+
+	if (dwc->tx_ipgap_linecheck_dis_quirk)
+		reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+
+	dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
 
 	return 0;
 
@@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				"snps,dis-u2-freeclk-exists-quirk");
 	dwc->dis_del_phy_power_chg_quirk = device_property_read_bool(dev,
 				"snps,dis-del-phy-power-chg-quirk");
+	dwc->tx_ipgap_linecheck_dis_quirk = device_property_read_bool(dev,
+				"snps,tx-ipgap-linecheck-dis-quirk");
 
 	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
 				"snps,tx_de_emphasis_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 981c77f..3c2537b 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -204,6 +204,7 @@
 #define DWC3_GCTL_DSBLCLKGTNG		BIT(0)
 
 /* Global User Control 1 Register */
+#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
 #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW	BIT(24)
 
 /* Global USB2 PHY Configuration Register */
@@ -850,6 +851,8 @@ struct dwc3_scratchpad_array {
  *			provide a free-running PHY clock.
  * @dis_del_phy_power_chg_quirk: set if we disable delay phy power
  *			change quirk.
+ * @tx_ipgap_linecheck_dis_quirk: set if we disable u2mac linestate
+ *			check during HS transmit.
  * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
  * @tx_de_emphasis: Tx de-emphasis value
  * 	0	- -6dB de-emphasis
@@ -1004,6 +1007,7 @@ struct dwc3 {
 	unsigned		dis_rxdet_inp3_quirk:1;
 	unsigned		dis_u2_freeclk_exists_quirk:1;
 	unsigned		dis_del_phy_power_chg_quirk:1;
+	unsigned		tx_ipgap_linecheck_dis_quirk:1;
 
 	unsigned		tx_de_emphasis_quirk:1;
 	unsigned		tx_de_emphasis:2;
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
  2017-04-18  5:17 [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk William Wu
@ 2017-04-18 13:18 ` Guenter Roeck
  2017-04-19  3:59   ` wlf
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2017-04-18 13:18 UTC (permalink / raw)
  To: William Wu
  Cc: Felipe Balbi, gregkh, Heiko Stübner, linux-kernel,
	linux-usb, open list:ARM/Rockchip SoC...,
	devicetree, Rob Herring, Frank Wang, Tao Huang, Doug Anderson,
	Brian Norris, daniel.meng, wulf

On Mon, Apr 17, 2017 at 10:17 PM, William Wu <william.wu@rock-chips.com> wrote:
> This patch adds a quirk to disable USB 2.0 MAC linestate check
> during HS transmit. Refer the dwc3 databook, we can use it for
> some special platforms if the linestate not reflect the expected
> line state(J) during transmission.
>
> When use this quirk, the controller implements a fixed 40-bit
> TxEndDelay after the packet is given on UTMI and ignores the
> linestate during the transmit of a token (during token-to-token
> and token-to-data IPGAP).
>
> On some rockchip platforms (e.g. rk3399), it requires to disable
> the u2mac linestate check to decrease the SSPLIT token to SETUP
> token inter-packet delay from 566ns to 466ns, and fix the issue
> that FS/LS devices not recognized if inserted through USB 3.0 HUB.
>
> Signed-off-by: William Wu <william.wu@rock-chips.com>
> ---
> Changes in v2:
> - fix coding style
>
>  Documentation/devicetree/bindings/usb/dwc3.txt |  2 ++
>  drivers/usb/dwc3/core.c                        | 14 ++++++++++----
>  drivers/usb/dwc3/core.h                        |  4 ++++
>  3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index f658f39..6a89f0c 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -45,6 +45,8 @@ Optional properties:
>                         a free-running PHY clock.
>   - snps,dis-del-phy-power-chg-quirk: when set core will change PHY power
>                         from P0 to P1/P2/P3 without delay.
> + - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate check
> +                       during HS transmit.

All other disable-something quirks are named
"snps,dis-something-quirk". Maybe use the same naming convention ?

>   - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>                         utmi_l1_suspend_n, false when asserts utmi_sleep_n
>   - snps,hird-threshold: HIRD threshold
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 455d89a..03429c5 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>                 dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
>         }
>
> +       reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> +

My understanding is that the register was only introduced with dwc3
revision 2.50a. Is it ok to read and write it unconditionally ?

>         /*
>          * Enable hardware control of sending remote wakeup in HS when
>          * the device is in the L1 state.
>          */
> -       if (dwc->revision >= DWC3_REVISION_290A) {
> -               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> +       if (dwc->revision >= DWC3_REVISION_290A)
>                 reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
> -               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> -       }
> +
> +       if (dwc->tx_ipgap_linecheck_dis_quirk)
> +               reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
> +
> +       dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>
>         return 0;
>
> @@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>                                 "snps,dis-u2-freeclk-exists-quirk");
>         dwc->dis_del_phy_power_chg_quirk = device_property_read_bool(dev,
>                                 "snps,dis-del-phy-power-chg-quirk");
> +       dwc->tx_ipgap_linecheck_dis_quirk = device_property_read_bool(dev,
> +                               "snps,tx-ipgap-linecheck-dis-quirk");
>
>         dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
>                                 "snps,tx_de_emphasis_quirk");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 981c77f..3c2537b 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -204,6 +204,7 @@
>  #define DWC3_GCTL_DSBLCLKGTNG          BIT(0)
>
>  /* Global User Control 1 Register */
> +#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS     BIT(28)
>  #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW  BIT(24)
>
>  /* Global USB2 PHY Configuration Register */
> @@ -850,6 +851,8 @@ struct dwc3_scratchpad_array {
>   *                     provide a free-running PHY clock.
>   * @dis_del_phy_power_chg_quirk: set if we disable delay phy power
>   *                     change quirk.
> + * @tx_ipgap_linecheck_dis_quirk: set if we disable u2mac linestate
> + *                     check during HS transmit.
>   * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
>   * @tx_de_emphasis: Tx de-emphasis value
>   *     0       - -6dB de-emphasis
> @@ -1004,6 +1007,7 @@ struct dwc3 {
>         unsigned                dis_rxdet_inp3_quirk:1;
>         unsigned                dis_u2_freeclk_exists_quirk:1;
>         unsigned                dis_del_phy_power_chg_quirk:1;
> +       unsigned                tx_ipgap_linecheck_dis_quirk:1;
>
>         unsigned                tx_de_emphasis_quirk:1;
>         unsigned                tx_de_emphasis:2;
> --
> 2.0.0
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
  2017-04-18 13:18 ` Guenter Roeck
@ 2017-04-19  3:59   ` wlf
  2017-04-19  5:15     ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: wlf @ 2017-04-19  3:59 UTC (permalink / raw)
  To: Guenter Roeck, William Wu, Felipe Balbi, johnyoun
  Cc: gregkh, Heiko Stübner, linux-kernel, linux-usb,
	open list:ARM/Rockchip SoC...,
	devicetree, Rob Herring, Frank Wang, Tao Huang, Doug Anderson,
	Brian Norris, daniel.meng

Dear Guenter,


在 2017年04月18日 21:18, Guenter Roeck 写道:
> On Mon, Apr 17, 2017 at 10:17 PM, William Wu <william.wu@rock-chips.com> wrote:
>> This patch adds a quirk to disable USB 2.0 MAC linestate check
>> during HS transmit. Refer the dwc3 databook, we can use it for
>> some special platforms if the linestate not reflect the expected
>> line state(J) during transmission.
>>
>> When use this quirk, the controller implements a fixed 40-bit
>> TxEndDelay after the packet is given on UTMI and ignores the
>> linestate during the transmit of a token (during token-to-token
>> and token-to-data IPGAP).
>>
>> On some rockchip platforms (e.g. rk3399), it requires to disable
>> the u2mac linestate check to decrease the SSPLIT token to SETUP
>> token inter-packet delay from 566ns to 466ns, and fix the issue
>> that FS/LS devices not recognized if inserted through USB 3.0 HUB.
>>
>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>> ---
>> Changes in v2:
>> - fix coding style
>>
>>   Documentation/devicetree/bindings/usb/dwc3.txt |  2 ++
>>   drivers/usb/dwc3/core.c                        | 14 ++++++++++----
>>   drivers/usb/dwc3/core.h                        |  4 ++++
>>   3 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index f658f39..6a89f0c 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -45,6 +45,8 @@ Optional properties:
>>                          a free-running PHY clock.
>>    - snps,dis-del-phy-power-chg-quirk: when set core will change PHY power
>>                          from P0 to P1/P2/P3 without delay.
>> + - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate check
>> +                       during HS transmit.
> All other disable-something quirks are named
> "snps,dis-something-quirk". Maybe use the same naming convention ?
Yes, good idea! I will fix it with "snps,dis-tx-ipgap-linecheck-quirk"  
in next patch verison.
Thanks:-)
>
>>    - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>>                          utmi_l1_suspend_n, false when asserts utmi_sleep_n
>>    - snps,hird-threshold: HIRD threshold
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 455d89a..03429c5 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>                  dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
>>          }
>>
>> +       reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>> +
> My understanding is that the register was only introduced with dwc3
> revision 2.50a. Is it ok to read and write it unconditionally ?
Yes, refer to dwc3 databook, the DWC3_GUCTL1 was introduced since 2.50a. 
Maybe it's better
to read and write it only when we know our controller version.

Is it good to fix it like the following patch?
But this patch has a problem that we need to read and write the register
twice if our controller verison > = 2.90a, and need this quirk.

--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -806,6 +806,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
                 dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
         }

+       if (dwc->dis_tx_ipgap_linecheck_quirk) {
+               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+               reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
+       }
+

Hi John & Felipe,
        Could you provide me some suggestion?
        Thank you!
>>          /*
>>           * Enable hardware control of sending remote wakeup in HS when
>>           * the device is in the L1 state.
>>           */
>> -       if (dwc->revision >= DWC3_REVISION_290A) {
>> -               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>> +       if (dwc->revision >= DWC3_REVISION_290A)
>>                  reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
>> -               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>> -       }
>> +
>> +       if (dwc->tx_ipgap_linecheck_dis_quirk)
>> +               reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
>> +
>> +       dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>>
>>          return 0;
>>
>> @@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>                                  "snps,dis-u2-freeclk-exists-quirk");
>>          dwc->dis_del_phy_power_chg_quirk = device_property_read_bool(dev,
>>                                  "snps,dis-del-phy-power-chg-quirk");
>> +       dwc->tx_ipgap_linecheck_dis_quirk = device_property_read_bool(dev,
>> +                               "snps,tx-ipgap-linecheck-dis-quirk");
>>
>>          dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
>>                                  "snps,tx_de_emphasis_quirk");
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 981c77f..3c2537b 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -204,6 +204,7 @@
>>   #define DWC3_GCTL_DSBLCLKGTNG          BIT(0)
>>
>>   /* Global User Control 1 Register */
>> +#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS     BIT(28)
>>   #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW  BIT(24)
>>
>>   /* Global USB2 PHY Configuration Register */
>> @@ -850,6 +851,8 @@ struct dwc3_scratchpad_array {
>>    *                     provide a free-running PHY clock.
>>    * @dis_del_phy_power_chg_quirk: set if we disable delay phy power
>>    *                     change quirk.
>> + * @tx_ipgap_linecheck_dis_quirk: set if we disable u2mac linestate
>> + *                     check during HS transmit.
>>    * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
>>    * @tx_de_emphasis: Tx de-emphasis value
>>    *     0       - -6dB de-emphasis
>> @@ -1004,6 +1007,7 @@ struct dwc3 {
>>          unsigned                dis_rxdet_inp3_quirk:1;
>>          unsigned                dis_u2_freeclk_exists_quirk:1;
>>          unsigned                dis_del_phy_power_chg_quirk:1;
>> +       unsigned                tx_ipgap_linecheck_dis_quirk:1;
>>
>>          unsigned                tx_de_emphasis_quirk:1;
>>          unsigned                tx_de_emphasis:2;
>> --
>> 2.0.0
>>
>>
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
  2017-04-19  3:59   ` wlf
@ 2017-04-19  5:15     ` Guenter Roeck
  2017-04-19 11:32       ` wlf
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2017-04-19  5:15 UTC (permalink / raw)
  To: wlf
  Cc: William Wu, Felipe Balbi, johnyoun, gregkh, Heiko Stübner,
	linux-kernel, linux-usb, open list:ARM/Rockchip SoC...,
	devicetree, Rob Herring, Frank Wang, Tao Huang, Doug Anderson,
	Brian Norris, daniel.meng

On Tue, Apr 18, 2017 at 8:59 PM, wlf <wulf@rock-chips.com> wrote:
> Dear Guenter,
>
>
>
> 在 2017年04月18日 21:18, Guenter Roeck 写道:
>>
>> On Mon, Apr 17, 2017 at 10:17 PM, William Wu <william.wu@rock-chips.com>
>> wrote:
>>>
>>> This patch adds a quirk to disable USB 2.0 MAC linestate check
>>> during HS transmit. Refer the dwc3 databook, we can use it for
>>> some special platforms if the linestate not reflect the expected
>>> line state(J) during transmission.
>>>
>>> When use this quirk, the controller implements a fixed 40-bit
>>> TxEndDelay after the packet is given on UTMI and ignores the
>>> linestate during the transmit of a token (during token-to-token
>>> and token-to-data IPGAP).
>>>
>>> On some rockchip platforms (e.g. rk3399), it requires to disable
>>> the u2mac linestate check to decrease the SSPLIT token to SETUP
>>> token inter-packet delay from 566ns to 466ns, and fix the issue
>>> that FS/LS devices not recognized if inserted through USB 3.0 HUB.
>>>
>>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>>> ---
>>> Changes in v2:
>>> - fix coding style
>>>
>>>   Documentation/devicetree/bindings/usb/dwc3.txt |  2 ++
>>>   drivers/usb/dwc3/core.c                        | 14 ++++++++++----
>>>   drivers/usb/dwc3/core.h                        |  4 ++++
>>>   3 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index f658f39..6a89f0c 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -45,6 +45,8 @@ Optional properties:
>>>                          a free-running PHY clock.
>>>    - snps,dis-del-phy-power-chg-quirk: when set core will change PHY
>>> power
>>>                          from P0 to P1/P2/P3 without delay.
>>> + - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate
>>> check
>>> +                       during HS transmit.
>>
>> All other disable-something quirks are named
>> "snps,dis-something-quirk". Maybe use the same naming convention ?
>
> Yes, good idea! I will fix it with "snps,dis-tx-ipgap-linecheck-quirk"  in
> next patch verison.
> Thanks:-)
>>
>>
>>>    - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>>>                          utmi_l1_suspend_n, false when asserts
>>> utmi_sleep_n
>>>    - snps,hird-threshold: HIRD threshold
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 455d89a..03429c5 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>>                  dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
>>>          }
>>>
>>> +       reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>>> +
>>
>> My understanding is that the register was only introduced with dwc3
>> revision 2.50a. Is it ok to read and write it unconditionally ?
>
> Yes, refer to dwc3 databook, the DWC3_GUCTL1 was introduced since 2.50a.
> Maybe it's better
> to read and write it only when we know our controller version.
>
> Is it good to fix it like the following patch?
> But this patch has a problem that we need to read and write the register
> twice if our controller verison > = 2.90a, and need this quirk.
>
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -806,6 +806,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>                 dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>         }
>
> +       if (dwc->dis_tx_ipgap_linecheck_quirk) {
> +               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> +               reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
> +               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> +       }
> +
>

How about this ?

    if (dwc->revision >= DWC3_REVISION_250A) {
        reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
        if (dwc->revision >= DWC3_REVISION_290A)
            reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
        if (dwc->dis_tx_ipgap_linecheck_quirk)
           reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
        dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
    }

Thanks,
Guenter

> Hi John & Felipe,
>        Could you provide me some suggestion?
>        Thank you!
>
>>>          /*
>>>           * Enable hardware control of sending remote wakeup in HS when
>>>           * the device is in the L1 state.
>>>           */
>>> -       if (dwc->revision >= DWC3_REVISION_290A) {
>>> -               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>>> +       if (dwc->revision >= DWC3_REVISION_290A)
>>>                  reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
>>> -               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>>> -       }
>>> +
>>> +       if (dwc->tx_ipgap_linecheck_dis_quirk)
>>> +               reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
>>> +
>>> +       dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>>>
>>>          return 0;
>>>
>>> @@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>                                  "snps,dis-u2-freeclk-exists-quirk");
>>>          dwc->dis_del_phy_power_chg_quirk =
>>> device_property_read_bool(dev,
>>>                                  "snps,dis-del-phy-power-chg-quirk");
>>> +       dwc->tx_ipgap_linecheck_dis_quirk =
>>> device_property_read_bool(dev,
>>> +                               "snps,tx-ipgap-linecheck-dis-quirk");
>>>
>>>          dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
>>>                                  "snps,tx_de_emphasis_quirk");
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 981c77f..3c2537b 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -204,6 +204,7 @@
>>>   #define DWC3_GCTL_DSBLCLKGTNG          BIT(0)
>>>
>>>   /* Global User Control 1 Register */
>>> +#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS     BIT(28)
>>>   #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW  BIT(24)
>>>
>>>   /* Global USB2 PHY Configuration Register */
>>> @@ -850,6 +851,8 @@ struct dwc3_scratchpad_array {
>>>    *                     provide a free-running PHY clock.
>>>    * @dis_del_phy_power_chg_quirk: set if we disable delay phy power
>>>    *                     change quirk.
>>> + * @tx_ipgap_linecheck_dis_quirk: set if we disable u2mac linestate
>>> + *                     check during HS transmit.
>>>    * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
>>>    * @tx_de_emphasis: Tx de-emphasis value
>>>    *     0       - -6dB de-emphasis
>>> @@ -1004,6 +1007,7 @@ struct dwc3 {
>>>          unsigned                dis_rxdet_inp3_quirk:1;
>>>          unsigned                dis_u2_freeclk_exists_quirk:1;
>>>          unsigned                dis_del_phy_power_chg_quirk:1;
>>> +       unsigned                tx_ipgap_linecheck_dis_quirk:1;
>>>
>>>          unsigned                tx_de_emphasis_quirk:1;
>>>          unsigned                tx_de_emphasis:2;
>>> --
>>> 2.0.0
>>>
>>>
>>
>>
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
  2017-04-19  5:15     ` Guenter Roeck
@ 2017-04-19 11:32       ` wlf
  0 siblings, 0 replies; 5+ messages in thread
From: wlf @ 2017-04-19 11:32 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: William Wu, Felipe Balbi, johnyoun, gregkh, Heiko Stübner,
	linux-kernel, linux-usb, open list:ARM/Rockchip SoC...,
	devicetree, Rob Herring, Frank Wang, Tao Huang, Doug Anderson,
	Brian Norris, daniel.meng

Dear Guenter,

在 2017年04月19日 13:15, Guenter Roeck 写道:
> On Tue, Apr 18, 2017 at 8:59 PM, wlf <wulf@rock-chips.com> wrote:
>> Dear Guenter,
>>
>>
>>
>> 在 2017年04月18日 21:18, Guenter Roeck 写道:
>>> On Mon, Apr 17, 2017 at 10:17 PM, William Wu <william.wu@rock-chips.com>
>>> wrote:
>>>> This patch adds a quirk to disable USB 2.0 MAC linestate check
>>>> during HS transmit. Refer the dwc3 databook, we can use it for
>>>> some special platforms if the linestate not reflect the expected
>>>> line state(J) during transmission.
>>>>
>>>> When use this quirk, the controller implements a fixed 40-bit
>>>> TxEndDelay after the packet is given on UTMI and ignores the
>>>> linestate during the transmit of a token (during token-to-token
>>>> and token-to-data IPGAP).
>>>>
>>>> On some rockchip platforms (e.g. rk3399), it requires to disable
>>>> the u2mac linestate check to decrease the SSPLIT token to SETUP
>>>> token inter-packet delay from 566ns to 466ns, and fix the issue
>>>> that FS/LS devices not recognized if inserted through USB 3.0 HUB.
>>>>
>>>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>>>> ---
>>>> Changes in v2:
>>>> - fix coding style
>>>>
>>>>    Documentation/devicetree/bindings/usb/dwc3.txt |  2 ++
>>>>    drivers/usb/dwc3/core.c                        | 14 ++++++++++----
>>>>    drivers/usb/dwc3/core.h                        |  4 ++++
>>>>    3 files changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index f658f39..6a89f0c 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -45,6 +45,8 @@ Optional properties:
>>>>                           a free-running PHY clock.
>>>>     - snps,dis-del-phy-power-chg-quirk: when set core will change PHY
>>>> power
>>>>                           from P0 to P1/P2/P3 without delay.
>>>> + - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate
>>>> check
>>>> +                       during HS transmit.
>>> All other disable-something quirks are named
>>> "snps,dis-something-quirk". Maybe use the same naming convention ?
>> Yes, good idea! I will fix it with "snps,dis-tx-ipgap-linecheck-quirk"  in
>> next patch verison.
>> Thanks:-)
>>>
>>>>     - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>>>>                           utmi_l1_suspend_n, false when asserts
>>>> utmi_sleep_n
>>>>     - snps,hird-threshold: HIRD threshold
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 455d89a..03429c5 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>>>                   dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
>>>>           }
>>>>
>>>> +       reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>>>> +
>>> My understanding is that the register was only introduced with dwc3
>>> revision 2.50a. Is it ok to read and write it unconditionally ?
>> Yes, refer to dwc3 databook, the DWC3_GUCTL1 was introduced since 2.50a.
>> Maybe it's better
>> to read and write it only when we know our controller version.
>>
>> Is it good to fix it like the following patch?
>> But this patch has a problem that we need to read and write the register
>> twice if our controller verison > = 2.90a, and need this quirk.
>>
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -806,6 +806,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>                  dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>>          }
>>
>> +       if (dwc->dis_tx_ipgap_linecheck_quirk) {
>> +               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>> +               reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
>> +               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>> +       }
>> +
>>
> How about this ?
>
>      if (dwc->revision >= DWC3_REVISION_250A) {
>          reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>          if (dwc->revision >= DWC3_REVISION_290A)
>              reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
>          if (dwc->dis_tx_ipgap_linecheck_quirk)
>             reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
>          dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>      }
>
> Thanks,
> Guenter
Thanks, looks good to me. I will fix it in patch v2.
>
>> Hi John & Felipe,
>>         Could you provide me some suggestion?
>>         Thank you!
>>
>>>>           /*
>>>>            * Enable hardware control of sending remote wakeup in HS when
>>>>            * the device is in the L1 state.
>>>>            */
>>>> -       if (dwc->revision >= DWC3_REVISION_290A) {
>>>> -               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>>>> +       if (dwc->revision >= DWC3_REVISION_290A)
>>>>                   reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
>>>> -               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>>>> -       }
>>>> +
>>>> +       if (dwc->tx_ipgap_linecheck_dis_quirk)
>>>> +               reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
>>>> +
>>>> +       dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>>>>
>>>>           return 0;
>>>>
>>>> @@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>                                   "snps,dis-u2-freeclk-exists-quirk");
>>>>           dwc->dis_del_phy_power_chg_quirk =
>>>> device_property_read_bool(dev,
>>>>                                   "snps,dis-del-phy-power-chg-quirk");
>>>> +       dwc->tx_ipgap_linecheck_dis_quirk =
>>>> device_property_read_bool(dev,
>>>> +                               "snps,tx-ipgap-linecheck-dis-quirk");
>>>>
>>>>           dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
>>>>                                   "snps,tx_de_emphasis_quirk");
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 981c77f..3c2537b 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -204,6 +204,7 @@
>>>>    #define DWC3_GCTL_DSBLCLKGTNG          BIT(0)
>>>>
>>>>    /* Global User Control 1 Register */
>>>> +#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS     BIT(28)
>>>>    #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW  BIT(24)
>>>>
>>>>    /* Global USB2 PHY Configuration Register */
>>>> @@ -850,6 +851,8 @@ struct dwc3_scratchpad_array {
>>>>     *                     provide a free-running PHY clock.
>>>>     * @dis_del_phy_power_chg_quirk: set if we disable delay phy power
>>>>     *                     change quirk.
>>>> + * @tx_ipgap_linecheck_dis_quirk: set if we disable u2mac linestate
>>>> + *                     check during HS transmit.
>>>>     * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
>>>>     * @tx_de_emphasis: Tx de-emphasis value
>>>>     *     0       - -6dB de-emphasis
>>>> @@ -1004,6 +1007,7 @@ struct dwc3 {
>>>>           unsigned                dis_rxdet_inp3_quirk:1;
>>>>           unsigned                dis_u2_freeclk_exists_quirk:1;
>>>>           unsigned                dis_del_phy_power_chg_quirk:1;
>>>> +       unsigned                tx_ipgap_linecheck_dis_quirk:1;
>>>>
>>>>           unsigned                tx_de_emphasis_quirk:1;
>>>>           unsigned                tx_de_emphasis:2;
>>>> --
>>>> 2.0.0
>>>>
>>>>
>>>
>>
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-04-19 11:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18  5:17 [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk William Wu
2017-04-18 13:18 ` Guenter Roeck
2017-04-19  3:59   ` wlf
2017-04-19  5:15     ` Guenter Roeck
2017-04-19 11:32       ` wlf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).