[PATCH v2, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[PATCH v2, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[joswang]

From: joswang <joswang@lenovo.com>

There is an issue with the DWC31 2.00a and earlier versions
where the controller link power state transition from
P3/P3CPM/P4 to P2 may take longer than expected, ultimately
resulting in the hibernation D3 entering time exceeding the
expected 10ms.

Add a new 'snps,p2p3tranok-quirk' DT quirk to dwc3 core
for enable the controller transitions directly from phy
power state P2 to P3 or from state P3 to P2.

Note that this can only be set if the USB3 PHY supports
direct p3 to p2 or p2 to p3 conversion.

Signed-off-by: joswang <joswang@lenovo.com>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 1cd0ca90127d..721927495887 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -242,6 +242,13 @@ properties:
       When set, all HighSpeed bus instances in park mode are disabled.
     type: boolean
 
+  snps,p2p3tranok-quirk:
+    description:
+      When set, the controller transitions directly from phy power state
+      P2 to P3 or from state P3 to P2. Note that this can only be set
+      if the USB3 PHY supports direct p3 to p2 or p2 to p3 conversion.
+    type: boolean
+
   snps,dis_metastability_quirk:
     description:
       When set, disable metastability workaround. CAUTION! Use only if you are
-- 
2.17.1

Re: [PATCH v2, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[Krzysztof Kozlowski]

On 03/06/2024 15:00, joswang wrote:
> From: joswang <joswang@lenovo.com>

Is this your full name or known identity you want to use for all kernel
contributions? Looks like login...

> 
> There is an issue with the DWC31 2.00a and earlier versions
> where the controller link power state transition from
> P3/P3CPM/P4 to P2 may take longer than expected, ultimately
> resulting in the hibernation D3 entering time exceeding the
> expected 10ms.
> 
> Add a new 'snps,p2p3tranok-quirk' DT quirk to dwc3 core
> for enable the controller transitions directly from phy
> power state P2 to P3 or from state P3 to P2.
> 
> Note that this can only be set if the USB3 PHY supports
> direct p3 to p2 or p2 to p3 conversion.
> 
> Signed-off-by: joswang <joswang@lenovo.com>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index 1cd0ca90127d..721927495887 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -242,6 +242,13 @@ properties:
>        When set, all HighSpeed bus instances in park mode are disabled.
>      type: boolean
>  
> +  snps,p2p3tranok-quirk:

Why this cannot be deduced from compatible? Which upstream SoCs are
affected?



Best regards,
Krzysztof

Re: [PATCH v2, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[joswang]

On Tue, Jun 4, 2024 at 2:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 03/06/2024 15:00, joswang wrote:
> > From: joswang <joswang@lenovo.com>
>
> Is this your full name or known identity you want to use for all kernel
> contributions? Looks like login...
>
> >
> > There is an issue with the DWC31 2.00a and earlier versions
> > where the controller link power state transition from
> > P3/P3CPM/P4 to P2 may take longer than expected, ultimately
> > resulting in the hibernation D3 entering time exceeding the
> > expected 10ms.
> >
> > Add a new 'snps,p2p3tranok-quirk' DT quirk to dwc3 core
> > for enable the controller transitions directly from phy
> > power state P2 to P3 or from state P3 to P2.
> >
> > Note that this can only be set if the USB3 PHY supports
> > direct p3 to p2 or p2 to p3 conversion.
> >
> > Signed-off-by: joswang <joswang@lenovo.com>
> > ---
> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > index 1cd0ca90127d..721927495887 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > @@ -242,6 +242,13 @@ properties:
> >        When set, all HighSpeed bus instances in park mode are disabled.
> >      type: boolean
> >
> > +  snps,p2p3tranok-quirk:
>
> Why this cannot be deduced from compatible? Which upstream SoCs are
> affected?
>
>
>
> Best regards,
> Krzysztof
>

Thanks for your help in reviewing the code
DWC31_USB 2.00a and earlier versions IP bug, regardless of platform.

Re: [PATCH v2, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[Krzysztof Kozlowski]

On 12/06/2024 16:28, joswang wrote:
> On Tue, Jun 4, 2024 at 2:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 03/06/2024 15:00, joswang wrote:
>>> From: joswang <joswang@lenovo.com>
>>
>> Is this your full name or known identity you want to use for all kernel
>> contributions? Looks like login...
>>
>>>
>>> There is an issue with the DWC31 2.00a and earlier versions
>>> where the controller link power state transition from
>>> P3/P3CPM/P4 to P2 may take longer than expected, ultimately
>>> resulting in the hibernation D3 entering time exceeding the
>>> expected 10ms.
>>>
>>> Add a new 'snps,p2p3tranok-quirk' DT quirk to dwc3 core
>>> for enable the controller transitions directly from phy
>>> power state P2 to P3 or from state P3 to P2.
>>>
>>> Note that this can only be set if the USB3 PHY supports
>>> direct p3 to p2 or p2 to p3 conversion.
>>>
>>> Signed-off-by: joswang <joswang@lenovo.com>
>>> ---
>>>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> index 1cd0ca90127d..721927495887 100644
>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> @@ -242,6 +242,13 @@ properties:
>>>        When set, all HighSpeed bus instances in park mode are disabled.
>>>      type: boolean
>>>
>>> +  snps,p2p3tranok-quirk:
>>
>> Why this cannot be deduced from compatible? Which upstream SoCs are
>> affected?
>>
>>
>>
>> Best regards,
>> Krzysztof
>>
> 
> Thanks for your help in reviewing the code
> DWC31_USB 2.00a and earlier versions IP bug, regardless of platform.

So this can be deduced from compatible, then use quirks in the driver
based on compatible and drop the property.

Best regards,
Krzysztof

[PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[joswang]

From: joswang <joswang@lenovo.com>

In the case of enable hibernation, there is an issue with
the DWC31 2.00a and earlier versions where the controller
link power state transition from P3/P3CPM/P4 to P2 may take
longer than expected, ultimately resulting in the hibernation
D3 entering time exceeding the expected 10ms.

Synopsys workaround:
If the PHY supports direct P3 to P2 transition, program
GUSB3PIPECTL.P3P2Tran0K=1.

Therefore, adding p3p2tranok quirk for workaround hibernation
D3 exceeded the expected entry time.

Signed-off-by: joswang <joswang@lenovo.com>
---
 drivers/usb/dwc3/core.c | 5 +++++
 drivers/usb/dwc3/core.h | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 7ee61a89520b..3a8fbc2d6b99 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -666,6 +666,9 @@ static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
 	if (dwc->dis_del_phy_power_chg_quirk)
 		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
 
+	if (dwc->p2p3tranok_quirk)
+		reg |= DWC3_GUSB3PIPECTL_P3P2TRANOK;
+
 	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);
 
 	return 0;
@@ -1715,6 +1718,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 
 	dwc->dis_split_quirk = device_property_read_bool(dev,
 				"snps,dis-split-quirk");
+	dwc->p2p3tranok_quirk = device_property_read_bool(dev,
+				"snps,p2p3tranok-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 3781c736c1a1..2810dce8b42e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -327,6 +327,7 @@
 #define DWC3_GUSB3PIPECTL_DEP1P2P3_EN	DWC3_GUSB3PIPECTL_DEP1P2P3(1)
 #define DWC3_GUSB3PIPECTL_DEPOCHANGE	BIT(18)
 #define DWC3_GUSB3PIPECTL_SUSPHY	BIT(17)
+#define DWC3_GUSB3PIPECTL_P3P2TRANOK	BIT(11)
 #define DWC3_GUSB3PIPECTL_LFPSFILT	BIT(9)
 #define DWC3_GUSB3PIPECTL_RX_DETOPOLL	BIT(8)
 #define DWC3_GUSB3PIPECTL_TX_DEEPH_MASK	DWC3_GUSB3PIPECTL_TX_DEEPH(3)
@@ -1132,6 +1133,8 @@ struct dwc3_scratchpad_array {
  *			instances in park mode.
  * @parkmode_disable_hs_quirk: set if we need to disable all HishSpeed
  *			instances in park mode.
+ * @p2p3tranok_quirk: set if Controller transitions directly from phy
+ *			power state P2 to P3 or from state P3 to P2.
  * @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
@@ -1361,6 +1364,7 @@ struct dwc3 {
 	unsigned		ulpi_ext_vbus_drv:1;
 	unsigned		parkmode_disable_ss_quirk:1;
 	unsigned		parkmode_disable_hs_quirk:1;
+	unsigned		p2p3tranok_quirk:1;
 	unsigned		gfladj_refclk_lpm_sel:1;
 
 	unsigned		tx_de_emphasis_quirk:1;
-- 
2.17.1

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[Thinh Nguyen]

On Mon, Jun 03, 2024, joswang wrote:
> From: joswang <joswang@lenovo.com>
> 
> In the case of enable hibernation, there is an issue with

I assume this is for host mode since we currently don't handle
hibernation in device mode (please confirm).

> the DWC31 2.00a and earlier versions where the controller
> link power state transition from P3/P3CPM/P4 to P2 may take
> longer than expected, ultimately resulting in the hibernation
> D3 entering time exceeding the expected 10ms.

Can you provide more context where the 10ms requirement is from?

> 
> Synopsys workaround:
> If the PHY supports direct P3 to P2 transition, program
> GUSB3PIPECTL.P3P2Tran0K=1.
> 

Which STAR issue is this?

> Therefore, adding p3p2tranok quirk for workaround hibernation
> D3 exceeded the expected entry time.
> 
> Signed-off-by: joswang <joswang@lenovo.com>
> ---

Please provide change note for v1->v2 here (and the rest of the other
patches).

>  drivers/usb/dwc3/core.c | 5 +++++
>  drivers/usb/dwc3/core.h | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 7ee61a89520b..3a8fbc2d6b99 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -666,6 +666,9 @@ static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
>  	if (dwc->dis_del_phy_power_chg_quirk)
>  		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
>  
> +	if (dwc->p2p3tranok_quirk)
> +		reg |= DWC3_GUSB3PIPECTL_P3P2TRANOK;
> +
>  	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);
>  
>  	return 0;
> @@ -1715,6 +1718,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
> +	dwc->p2p3tranok_quirk = device_property_read_bool(dev,
> +				"snps,p2p3tranok-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 3781c736c1a1..2810dce8b42e 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -327,6 +327,7 @@
>  #define DWC3_GUSB3PIPECTL_DEP1P2P3_EN	DWC3_GUSB3PIPECTL_DEP1P2P3(1)
>  #define DWC3_GUSB3PIPECTL_DEPOCHANGE	BIT(18)
>  #define DWC3_GUSB3PIPECTL_SUSPHY	BIT(17)
> +#define DWC3_GUSB3PIPECTL_P3P2TRANOK	BIT(11)
>  #define DWC3_GUSB3PIPECTL_LFPSFILT	BIT(9)
>  #define DWC3_GUSB3PIPECTL_RX_DETOPOLL	BIT(8)
>  #define DWC3_GUSB3PIPECTL_TX_DEEPH_MASK	DWC3_GUSB3PIPECTL_TX_DEEPH(3)
> @@ -1132,6 +1133,8 @@ struct dwc3_scratchpad_array {
>   *			instances in park mode.
>   * @parkmode_disable_hs_quirk: set if we need to disable all HishSpeed
>   *			instances in park mode.
> + * @p2p3tranok_quirk: set if Controller transitions directly from phy
> + *			power state P2 to P3 or from state P3 to P2.
>   * @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
> @@ -1361,6 +1364,7 @@ struct dwc3 {
>  	unsigned		ulpi_ext_vbus_drv:1;
>  	unsigned		parkmode_disable_ss_quirk:1;
>  	unsigned		parkmode_disable_hs_quirk:1;
> +	unsigned		p2p3tranok_quirk:1;
>  	unsigned		gfladj_refclk_lpm_sel:1;
>  
>  	unsigned		tx_de_emphasis_quirk:1;
> -- 
> 2.17.1
> 

Thanks,
Thinh

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[joswang]

On Tue, Jun 4, 2024 at 8:02 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Mon, Jun 03, 2024, joswang wrote:
> > From: joswang <joswang@lenovo.com>
> >
> > In the case of enable hibernation, there is an issue with
>
> I assume this is for host mode since we currently don't handle
> hibernation in device mode (please confirm).
Yes, your consideration is correct, hibernation is only handled in host mode
>
> > the DWC31 2.00a and earlier versions where the controller
> > link power state transition from P3/P3CPM/P4 to P2 may take
> > longer than expected, ultimately resulting in the hibernation
> > D3 entering time exceeding the expected 10ms.
>
> Can you provide more context where the 10ms requirement is from?
>
The P3/P3CPM/P4 to P2 power state change might take longer (maximum 10 ms).
If there is an impending D3 entry request, the controller does not
respond as long as the power state change is completed causing
unnecessary delays in D3 entry.
The above information is provided by your company.
STAR number 4236358
> >
> > Synopsys workaround:
> > If the PHY supports direct P3 to P2 transition, program
> > GUSB3PIPECTL.P3P2Tran0K=1.
> >
>
> Which STAR issue is this?
This is the solution provided by your company
STAR issue:  the DWC31 2.00a and earlier versions where the controller
link power state transition from P3/P3CPM/P4 to P2 may take longer
than expected.
>
> > Therefore, adding p3p2tranok quirk for workaround hibernation
> > D3 exceeded the expected entry time.
> >
> > Signed-off-by: joswang <joswang@lenovo.com>
> > ---
>
> Please provide change note for v1->v2 here (and the rest of the other
> patches).
>
> >  drivers/usb/dwc3/core.c | 5 +++++
> >  drivers/usb/dwc3/core.h | 4 ++++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 7ee61a89520b..3a8fbc2d6b99 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -666,6 +666,9 @@ static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
> >       if (dwc->dis_del_phy_power_chg_quirk)
> >               reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
> >
> > +     if (dwc->p2p3tranok_quirk)
> > +             reg |= DWC3_GUSB3PIPECTL_P3P2TRANOK;
> > +
> >       dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);
> >
> >       return 0;
> > @@ -1715,6 +1718,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> >
> >       dwc->dis_split_quirk = device_property_read_bool(dev,
> >                               "snps,dis-split-quirk");
> > +     dwc->p2p3tranok_quirk = device_property_read_bool(dev,
> > +                             "snps,p2p3tranok-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 3781c736c1a1..2810dce8b42e 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -327,6 +327,7 @@
> >  #define DWC3_GUSB3PIPECTL_DEP1P2P3_EN        DWC3_GUSB3PIPECTL_DEP1P2P3(1)
> >  #define DWC3_GUSB3PIPECTL_DEPOCHANGE BIT(18)
> >  #define DWC3_GUSB3PIPECTL_SUSPHY     BIT(17)
> > +#define DWC3_GUSB3PIPECTL_P3P2TRANOK BIT(11)
> >  #define DWC3_GUSB3PIPECTL_LFPSFILT   BIT(9)
> >  #define DWC3_GUSB3PIPECTL_RX_DETOPOLL        BIT(8)
> >  #define DWC3_GUSB3PIPECTL_TX_DEEPH_MASK      DWC3_GUSB3PIPECTL_TX_DEEPH(3)
> > @@ -1132,6 +1133,8 @@ struct dwc3_scratchpad_array {
> >   *                   instances in park mode.
> >   * @parkmode_disable_hs_quirk: set if we need to disable all HishSpeed
> >   *                   instances in park mode.
> > + * @p2p3tranok_quirk: set if Controller transitions directly from phy
> > + *                   power state P2 to P3 or from state P3 to P2.
> >   * @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
> > @@ -1361,6 +1364,7 @@ struct dwc3 {
> >       unsigned                ulpi_ext_vbus_drv:1;
> >       unsigned                parkmode_disable_ss_quirk:1;
> >       unsigned                parkmode_disable_hs_quirk:1;
> > +     unsigned                p2p3tranok_quirk:1;
> >       unsigned                gfladj_refclk_lpm_sel:1;
> >
> >       unsigned                tx_de_emphasis_quirk:1;
> > --
> > 2.17.1
> >
>
> Thanks,
> Thinh

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[joswang]

On Tue, Jun 4, 2024 at 8:02 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Mon, Jun 03, 2024, joswang wrote:
> > From: joswang <joswang@lenovo.com>
> >
> > In the case of enable hibernation, there is an issue with
>
> I assume this is for host mode since we currently don't handle
> hibernation in device mode (please confirm).
>
> > the DWC31 2.00a and earlier versions where the controller
> > link power state transition from P3/P3CPM/P4 to P2 may take
> > longer than expected, ultimately resulting in the hibernation
> > D3 entering time exceeding the expected 10ms.
>
> Can you provide more context where the 10ms requirement is from?
>
> >
> > Synopsys workaround:
> > If the PHY supports direct P3 to P2 transition, program
> > GUSB3PIPECTL.P3P2Tran0K=1.
> >
>
> Which STAR issue is this?
>
> > Therefore, adding p3p2tranok quirk for workaround hibernation
> > D3 exceeded the expected entry time.
> >
> > Signed-off-by: joswang <joswang@lenovo.com>
> > ---
>
> Please provide change note for v1->v2 here (and the rest of the other
> patches).
>
> >  drivers/usb/dwc3/core.c | 5 +++++
> >  drivers/usb/dwc3/core.h | 4 ++++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 7ee61a89520b..3a8fbc2d6b99 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -666,6 +666,9 @@ static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
> >       if (dwc->dis_del_phy_power_chg_quirk)
> >               reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
> >
> > +     if (dwc->p2p3tranok_quirk)
> > +             reg |= DWC3_GUSB3PIPECTL_P3P2TRANOK;
> > +
> >       dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);
> >
> >       return 0;
> > @@ -1715,6 +1718,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> >
> >       dwc->dis_split_quirk = device_property_read_bool(dev,
> >                               "snps,dis-split-quirk");
> > +     dwc->p2p3tranok_quirk = device_property_read_bool(dev,
> > +                             "snps,p2p3tranok-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 3781c736c1a1..2810dce8b42e 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -327,6 +327,7 @@
> >  #define DWC3_GUSB3PIPECTL_DEP1P2P3_EN        DWC3_GUSB3PIPECTL_DEP1P2P3(1)
> >  #define DWC3_GUSB3PIPECTL_DEPOCHANGE BIT(18)
> >  #define DWC3_GUSB3PIPECTL_SUSPHY     BIT(17)
> > +#define DWC3_GUSB3PIPECTL_P3P2TRANOK BIT(11)
> >  #define DWC3_GUSB3PIPECTL_LFPSFILT   BIT(9)
> >  #define DWC3_GUSB3PIPECTL_RX_DETOPOLL        BIT(8)
> >  #define DWC3_GUSB3PIPECTL_TX_DEEPH_MASK      DWC3_GUSB3PIPECTL_TX_DEEPH(3)
> > @@ -1132,6 +1133,8 @@ struct dwc3_scratchpad_array {
> >   *                   instances in park mode.
> >   * @parkmode_disable_hs_quirk: set if we need to disable all HishSpeed
> >   *                   instances in park mode.
> > + * @p2p3tranok_quirk: set if Controller transitions directly from phy
> > + *                   power state P2 to P3 or from state P3 to P2.
> >   * @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
> > @@ -1361,6 +1364,7 @@ struct dwc3 {
> >       unsigned                ulpi_ext_vbus_drv:1;
> >       unsigned                parkmode_disable_ss_quirk:1;
> >       unsigned                parkmode_disable_hs_quirk:1;
> > +     unsigned                p2p3tranok_quirk:1;
> >       unsigned                gfladj_refclk_lpm_sel:1;
> >
> >       unsigned                tx_de_emphasis_quirk:1;
> > --
> > 2.17.1
> >
>
> Thanks,
> Thinh

Difference between V2 and V1: This patch has no changes, only the
"dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk" patch is added.

Thanks
Jos Wang

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[joswang]

Hi Thinh

The workaround solution provided by your company for this issue is as follows:
  Workaround：if the phy support direct P3 to P2 transition，program
GUSB3PIPECTL.P3P2Tranok=1

As the databook mentions:
This bit is used only for some non-Synopsys PHYs that cannot do LFPS in P3.
This bit is used by third-party SS PHY. It must be set to '0' for Synopsys PHY.

For Synopsys PHY, if this bit is set to "1", will it cause unknown problems?
Please help confirm this, thank you!

Thanks,
Jos Wang

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[Thinh Nguyen]

Sorry for the delay response regarding this.

On Wed, Jun 19, 2024, joswang wrote:
> Hi Thinh
> 
> The workaround solution provided by your company for this issue is as follows:
>   Workaround：if the phy support direct P3 to P2 transition，program
> GUSB3PIPECTL.P3P2Tranok=1
> 
> As the databook mentions:
> This bit is used only for some non-Synopsys PHYs that cannot do LFPS in P3.
> This bit is used by third-party SS PHY. It must be set to '0' for Synopsys PHY.
> 
> For Synopsys PHY, if this bit is set to "1", will it cause unknown problems?
> Please help confirm this, thank you!
> 

That depends on what your use case and requirements are.

I've reviewed this case. The impact to this issue is that power state
change may take longer than expected. It may violate the PIPE spec, but
functionally, at least for how linux drivers are handled, I'm not clear
on how this will impact the typical user.

Can you help clarify your use case and what does this resolve beside the
fact that it workaround the increase latency/response time.

Thanks,
Thinh

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[joswang]

On Sat, Jun 22, 2024 at 8:05 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Sorry for the delay response regarding this.
>
> On Wed, Jun 19, 2024, joswang wrote:
> > Hi Thinh
> >
> > The workaround solution provided by your company for this issue is as follows:
> >   Workaround：if the phy support direct P3 to P2 transition，program
> > GUSB3PIPECTL.P3P2Tranok=1
> >
> > As the databook mentions:
> > This bit is used only for some non-Synopsys PHYs that cannot do LFPS in P3.
> > This bit is used by third-party SS PHY. It must be set to '0' for Synopsys PHY.
> >
> > For Synopsys PHY, if this bit is set to "1", will it cause unknown problems?
> > Please help confirm this, thank you!
> >
>
> That depends on what your use case and requirements are.
>
> I've reviewed this case. The impact to this issue is that power state
> change may take longer than expected. It may violate the PIPE spec, but
> functionally, at least for how linux drivers are handled, I'm not clear
> on how this will impact the typical user.
>
> Can you help clarify your use case and what does this resolve beside the
> fact that it workaround the increase latency/response time.
>
> Thanks,
> Thinh

Your company provides usage scenarios:
System software places the controller in low-power when there is no
traffic on the USB.
Subsequently, system software programs the controller to exit
low-power to resume traffic.

The method to reproduce the problem provided by your company:
1. Program the DWC_usb31 controller to operate in device mode of
operation. Program GUSB3PIPECTL.P3P2TranOK=0. To increase the
probability of hitting the problem run with a slower frequency for
suspend_clk (for example, 32 KHz and 160 KHz).
2. Place the link in U3 while ensuring that pipe_powerdown is driven to P3.
3. Program DWC_usb31 controller to exit U3. Ensure that for P0 ->P2
transition pipe_PhyStatus is returned immediately.
4. Program U3 exit from the remote link.
5. Program a D3 entry (pm_power_state_request=D3) at the same time
(from the device application) and observe if the D3 entry
acknowledgement (current_power_state_u3pmu=D3) takes longer than
expected (> 10 ms).

Currently, we do not have a real environment to verify this case, but
considering the Android GKI regulations, we need to submit patches to
Linux in advance. Based on the following workaround solution provided
by your company，since the hardware cannot be changed, we can only use
workaround 1 at present.
Workaround 1: If the PHY supports direct P3 to P2 transition, program
GUSB3PIPECTL.P3P2TranOK=1. However, note that as per PIPE4
Specification, direct transition from P3 to P2 is illegal.
Workaround 2: Delay the pipe_PhyStatus assertion by an amount greater
than two suspend_clk durations at the input of the controller's PIPE
interface.

We have the following questions and hope you can help us confirm them.
Thank you!
1. This case seems to describe that the P3 to P2 power state change
takes a long time, that is, the DWC3_usb31 controller takes a long
time to exit the D3 state. Please help evaluate whether this problem
is perceived from the software perspective, such as whether there is a
problem in the xhci_suspend or xhci_resume process. If from the
software perspective, this case will not cause the xhci driver to
fail, then we may not deal with this problem.
2. If this case causes the above problem, for Synopsys PHY,
configuring GUSB3PIPECTL.P3P2TranOK=1 will cause other unknown
problems?

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[Thinh Nguyen]

Hi Joswang,

On Tue, Jun 25, 2024, joswang wrote:
> On Sat, Jun 22, 2024 at 8:05 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > Sorry for the delay response regarding this.
> >
> > On Wed, Jun 19, 2024, joswang wrote:
> > > Hi Thinh
> > >
> > > The workaround solution provided by your company for this issue is as follows:
> > >   Workaround：if the phy support direct P3 to P2 transition，program
> > > GUSB3PIPECTL.P3P2Tranok=1
> > >
> > > As the databook mentions:
> > > This bit is used only for some non-Synopsys PHYs that cannot do LFPS in P3.
> > > This bit is used by third-party SS PHY. It must be set to '0' for Synopsys PHY.
> > >
> > > For Synopsys PHY, if this bit is set to "1", will it cause unknown problems?
> > > Please help confirm this, thank you!
> > >
> >
> > That depends on what your use case and requirements are.
> >
> > I've reviewed this case. The impact to this issue is that power state
> > change may take longer than expected. It may violate the PIPE spec, but
> > functionally, at least for how linux drivers are handled, I'm not clear
> > on how this will impact the typical user.
> >
> > Can you help clarify your use case and what does this resolve beside the
> > fact that it workaround the increase latency/response time.
> >
> > Thanks,
> > Thinh
> 
> Your company provides usage scenarios:
> System software places the controller in low-power when there is no
> traffic on the USB.
> Subsequently, system software programs the controller to exit
> low-power to resume traffic.
> 
> The method to reproduce the problem provided by your company:
> 1. Program the DWC_usb31 controller to operate in device mode of
> operation. Program GUSB3PIPECTL.P3P2TranOK=0. To increase the
> probability of hitting the problem run with a slower frequency for
> suspend_clk (for example, 32 KHz and 160 KHz).
> 2. Place the link in U3 while ensuring that pipe_powerdown is driven to P3.
> 3. Program DWC_usb31 controller to exit U3. Ensure that for P0 ->P2
> transition pipe_PhyStatus is returned immediately.
> 4. Program U3 exit from the remote link.
> 5. Program a D3 entry (pm_power_state_request=D3) at the same time
> (from the device application) and observe if the D3 entry
> acknowledgement (current_power_state_u3pmu=D3) takes longer than
> expected (> 10 ms).
> 
> Currently, we do not have a real environment to verify this case, but
> considering the Android GKI regulations, we need to submit patches to
> Linux in advance. Based on the following workaround solution provided
> by your company，since the hardware cannot be changed, we can only use
> workaround 1 at present.
> Workaround 1: If the PHY supports direct P3 to P2 transition, program
> GUSB3PIPECTL.P3P2TranOK=1. However, note that as per PIPE4
> Specification, direct transition from P3 to P2 is illegal.
> Workaround 2: Delay the pipe_PhyStatus assertion by an amount greater
> than two suspend_clk durations at the input of the controller's PIPE
> interface.
> 
> We have the following questions and hope you can help us confirm them.
> Thank you!
> 1. This case seems to describe that the P3 to P2 power state change
> takes a long time, that is, the DWC3_usb31 controller takes a long
> time to exit the D3 state. Please help evaluate whether this problem
> is perceived from the software perspective, such as whether there is a
> problem in the xhci_suspend or xhci_resume process. If from the
> software perspective, this case will not cause the xhci driver to
> fail, then we may not deal with this problem.
> 2. If this case causes the above problem, for Synopsys PHY,
> configuring GUSB3PIPECTL.P3P2TranOK=1 will cause other unknown
> problems?

For this to occur, the host must try to transition from P3 to P2, and
somehow goes into suspend and request for D3 immediately, which causes
D3 request to take longer than expected.

This is not something we would expect for xhci, because:
1) On xhci_resume(), we would expect the pci device to be powered on
   (D0). So it would not be in a condition for this issue to occur.
2) xhci_resume() takes some time restore the host controller states
   and reinitialize the registers and start the controller. Then
   xhci_suspend() also takes some time to save the states and halt the
   controller. So there's some time before the pci driver can send a D3
   request. I don't know how long your setup may take, but it's unlikely
   to hit this condition.

Even if we do somehow manage to run into this scenario, we can set a pci
quirk to increase pci_pm_d3hot_delay to increase the suspend/resume
timeout, avoid hitting this.

Unfortunately we don't have the real environment to verify this. But
IMHO, for a typical use case, I don't see the need to introduce this
"snps,p2p3tranok-quirk".

BR,
Thinh

Re: [PATCH v2, 2/3] usb: dwc3: core: add p3p2tranok quirk

[joswang]

Thank you for your feedback. We will not deal with this issue after
internal discussion.
Thank you again for taking the time to review the code.

Thanks,
Jos Wang

On Wed, Jun 26, 2024 at 9:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Hi Joswang,
>
> On Tue, Jun 25, 2024, joswang wrote:
> > On Sat, Jun 22, 2024 at 8:05 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > >
> > > Sorry for the delay response regarding this.
> > >
> > > On Wed, Jun 19, 2024, joswang wrote:
> > > > Hi Thinh
> > > >
> > > > The workaround solution provided by your company for this issue is as follows:
> > > >   Workaround：if the phy support direct P3 to P2 transition，program
> > > > GUSB3PIPECTL.P3P2Tranok=1
> > > >
> > > > As the databook mentions:
> > > > This bit is used only for some non-Synopsys PHYs that cannot do LFPS in P3.
> > > > This bit is used by third-party SS PHY. It must be set to '0' for Synopsys PHY.
> > > >
> > > > For Synopsys PHY, if this bit is set to "1", will it cause unknown problems?
> > > > Please help confirm this, thank you!
> > > >
> > >
> > > That depends on what your use case and requirements are.
> > >
> > > I've reviewed this case. The impact to this issue is that power state
> > > change may take longer than expected. It may violate the PIPE spec, but
> > > functionally, at least for how linux drivers are handled, I'm not clear
> > > on how this will impact the typical user.
> > >
> > > Can you help clarify your use case and what does this resolve beside the
> > > fact that it workaround the increase latency/response time.
> > >
> > > Thanks,
> > > Thinh
> >
> > Your company provides usage scenarios:
> > System software places the controller in low-power when there is no
> > traffic on the USB.
> > Subsequently, system software programs the controller to exit
> > low-power to resume traffic.
> >
> > The method to reproduce the problem provided by your company:
> > 1. Program the DWC_usb31 controller to operate in device mode of
> > operation. Program GUSB3PIPECTL.P3P2TranOK=0. To increase the
> > probability of hitting the problem run with a slower frequency for
> > suspend_clk (for example, 32 KHz and 160 KHz).
> > 2. Place the link in U3 while ensuring that pipe_powerdown is driven to P3.
> > 3. Program DWC_usb31 controller to exit U3. Ensure that for P0 ->P2
> > transition pipe_PhyStatus is returned immediately.
> > 4. Program U3 exit from the remote link.
> > 5. Program a D3 entry (pm_power_state_request=D3) at the same time
> > (from the device application) and observe if the D3 entry
> > acknowledgement (current_power_state_u3pmu=D3) takes longer than
> > expected (> 10 ms).
> >
> > Currently, we do not have a real environment to verify this case, but
> > considering the Android GKI regulations, we need to submit patches to
> > Linux in advance. Based on the following workaround solution provided
> > by your company，since the hardware cannot be changed, we can only use
> > workaround 1 at present.
> > Workaround 1: If the PHY supports direct P3 to P2 transition, program
> > GUSB3PIPECTL.P3P2TranOK=1. However, note that as per PIPE4
> > Specification, direct transition from P3 to P2 is illegal.
> > Workaround 2: Delay the pipe_PhyStatus assertion by an amount greater
> > than two suspend_clk durations at the input of the controller's PIPE
> > interface.
> >
> > We have the following questions and hope you can help us confirm them.
> > Thank you!
> > 1. This case seems to describe that the P3 to P2 power state change
> > takes a long time, that is, the DWC3_usb31 controller takes a long
> > time to exit the D3 state. Please help evaluate whether this problem
> > is perceived from the software perspective, such as whether there is a
> > problem in the xhci_suspend or xhci_resume process. If from the
> > software perspective, this case will not cause the xhci driver to
> > fail, then we may not deal with this problem.
> > 2. If this case causes the above problem, for Synopsys PHY,
> > configuring GUSB3PIPECTL.P3P2TranOK=1 will cause other unknown
> > problems?
>
> For this to occur, the host must try to transition from P3 to P2, and
> somehow goes into suspend and request for D3 immediately, which causes
> D3 request to take longer than expected.
>
> This is not something we would expect for xhci, because:
> 1) On xhci_resume(), we would expect the pci device to be powered on
>    (D0). So it would not be in a condition for this issue to occur.
> 2) xhci_resume() takes some time restore the host controller states
>    and reinitialize the registers and start the controller. Then
>    xhci_suspend() also takes some time to save the states and halt the
>    controller. So there's some time before the pci driver can send a D3
>    request. I don't know how long your setup may take, but it's unlikely
>    to hit this condition.
>
> Even if we do somehow manage to run into this scenario, we can set a pci
> quirk to increase pci_pm_d3hot_delay to increase the suspend/resume
> timeout, avoid hitting this.
>
> Unfortunately we don't have the real environment to verify this. But
> IMHO, for a typical use case, I don't see the need to introduce this
> "snps,p2p3tranok-quirk".
>
> BR,
> Thinh

[PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[joswang]

From: joswang <joswang@lenovo.com>

DWC31 version 2.00a have an issue that would cause
a CSR read timeout When CSR read coincides with RAM
Clock Gating Entry.

This workaround solution disable Clock Gating, sacrificing
power consumption for normal operation.

Signed-off-by: joswang <joswang@lenovo.com>
---
 drivers/usb/dwc3/core.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 3a8fbc2d6b99..1df85c505c9e 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -978,11 +978,22 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
 		 *
 		 * STAR#9000588375: Clock Gating, SOF Issues when ref_clk-Based
 		 * SOF/ITP Mode Used
+		 *
+		 * WORKAROUND: DWC31 version 2.00a have an issue that would
+		 * cause a CSR read timeout When CSR read coincides with RAM
+		 * Clock Gating Entry.
+		 *
+		 * This workaround solution disable Clock Gating, sacrificing
+		 * power consumption for normal operation.
 		 */
 		if ((dwc->dr_mode == USB_DR_MODE_HOST ||
 				dwc->dr_mode == USB_DR_MODE_OTG) &&
 				DWC3_VER_IS_WITHIN(DWC3, 210A, 250A))
 			reg |= DWC3_GCTL_DSBLCLKGTNG | DWC3_GCTL_SOFITPSYNC;
+		else if ((dwc->dr_mode == USB_DR_MODE_HOST ||
+				dwc->dr_mode == USB_DR_MODE_OTG) &&
+				DWC3_VER_IS(DWC31, 200A))
+			reg |= DWC3_GCTL_DSBLCLKGTNG;
 		else
 			reg &= ~DWC3_GCTL_DSBLCLKGTNG;
 		break;
@@ -992,6 +1003,18 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
 		 * will work. Device-mode hibernation is not yet implemented.
 		 */
 		reg |= DWC3_GCTL_GBLHIBERNATIONEN;
+
+		/*
+		 * WORKAROUND: DWC31 version 2.00a have an issue that would
+		 * cause a CSR read timeout When CSR read coincides with RAM
+		 * Clock Gating Entry.
+		 *
+		 * This workaround solution disable Clock Gating, sacrificing
+		 * power consumption for normal operation.
+		 */
+		if ((dwc->dr_mode == USB_DR_MODE_HOST ||
+		     dwc->dr_mode == USB_DR_MODE_OTG) && DWC3_VER_IS(DWC31, 200A))
+			reg |= DWC3_GCTL_DSBLCLKGTNG;
 		break;
 	default:
 		/* nothing */
-- 
2.17.1

Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[Thinh Nguyen]

On Mon, Jun 03, 2024, joswang wrote:
> From: joswang <joswang@lenovo.com>
> 
> DWC31 version 2.00a have an issue that would cause
> a CSR read timeout When CSR read coincides with RAM
> Clock Gating Entry.

Do you have the STAR issue number?

> 
> This workaround solution disable Clock Gating, sacrificing
> power consumption for normal operation.
> 
> Signed-off-by: joswang <joswang@lenovo.com>
> ---
>  drivers/usb/dwc3/core.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 3a8fbc2d6b99..1df85c505c9e 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -978,11 +978,22 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
>  		 *
>  		 * STAR#9000588375: Clock Gating, SOF Issues when ref_clk-Based
>  		 * SOF/ITP Mode Used
> +		 *
> +		 * WORKAROUND: DWC31 version 2.00a have an issue that would
> +		 * cause a CSR read timeout When CSR read coincides with RAM
> +		 * Clock Gating Entry.
> +		 *
> +		 * This workaround solution disable Clock Gating, sacrificing
> +		 * power consumption for normal operation.
>  		 */
>  		if ((dwc->dr_mode == USB_DR_MODE_HOST ||
>  				dwc->dr_mode == USB_DR_MODE_OTG) &&
>  				DWC3_VER_IS_WITHIN(DWC3, 210A, 250A))
>  			reg |= DWC3_GCTL_DSBLCLKGTNG | DWC3_GCTL_SOFITPSYNC;
> +		else if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> +				dwc->dr_mode == USB_DR_MODE_OTG) &&
> +				DWC3_VER_IS(DWC31, 200A))
> +			reg |= DWC3_GCTL_DSBLCLKGTNG;
>  		else
>  			reg &= ~DWC3_GCTL_DSBLCLKGTNG;
>  		break;
> @@ -992,6 +1003,18 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
>  		 * will work. Device-mode hibernation is not yet implemented.
>  		 */
>  		reg |= DWC3_GCTL_GBLHIBERNATIONEN;
> +
> +		/*
> +		 * WORKAROUND: DWC31 version 2.00a have an issue that would
> +		 * cause a CSR read timeout When CSR read coincides with RAM
> +		 * Clock Gating Entry.
> +		 *
> +		 * This workaround solution disable Clock Gating, sacrificing
> +		 * power consumption for normal operation.
> +		 */
> +		if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> +		     dwc->dr_mode == USB_DR_MODE_OTG) && DWC3_VER_IS(DWC31, 200A))
> +			reg |= DWC3_GCTL_DSBLCLKGTNG;
>  		break;
>  	default:
>  		/* nothing */
> -- 
> 2.17.1
> 

This doesn't seem like it should be applied globally. Please provide the
STAR number if you can so I can review further. If possible I'd prefer
to only target your platform.

Thanks,
Thinh

Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[joswang]

On Tue, Jun 4, 2024 at 8:07 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Mon, Jun 03, 2024, joswang wrote:
> > From: joswang <joswang@lenovo.com>
> >
> > DWC31 version 2.00a have an issue that would cause
> > a CSR read timeout When CSR read coincides with RAM
> > Clock Gating Entry.
>
> Do you have the STAR issue number?
>
Thanks for reviewing the code.
The STAR number provided by Synopsys is 4846132.
Please help review further.

> >
> > This workaround solution disable Clock Gating, sacrificing
> > power consumption for normal operation.
> >
> > Signed-off-by: joswang <joswang@lenovo.com>
> > ---
> >  drivers/usb/dwc3/core.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 3a8fbc2d6b99..1df85c505c9e 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -978,11 +978,22 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> >                *
> >                * STAR#9000588375: Clock Gating, SOF Issues when ref_clk-Based
> >                * SOF/ITP Mode Used
> > +              *
> > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> > +              * cause a CSR read timeout When CSR read coincides with RAM
> > +              * Clock Gating Entry.
> > +              *
> > +              * This workaround solution disable Clock Gating, sacrificing
> > +              * power consumption for normal operation.
> >                */
> >               if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> >                               dwc->dr_mode == USB_DR_MODE_OTG) &&
> >                               DWC3_VER_IS_WITHIN(DWC3, 210A, 250A))
> >                       reg |= DWC3_GCTL_DSBLCLKGTNG | DWC3_GCTL_SOFITPSYNC;
> > +             else if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > +                             dwc->dr_mode == USB_DR_MODE_OTG) &&
> > +                             DWC3_VER_IS(DWC31, 200A))
> > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> >               else
> >                       reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> >               break;
> > @@ -992,6 +1003,18 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> >                * will work. Device-mode hibernation is not yet implemented.
> >                */
> >               reg |= DWC3_GCTL_GBLHIBERNATIONEN;
> > +
> > +             /*
> > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> > +              * cause a CSR read timeout When CSR read coincides with RAM
> > +              * Clock Gating Entry.
> > +              *
> > +              * This workaround solution disable Clock Gating, sacrificing
> > +              * power consumption for normal operation.
> > +              */
> > +             if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > +                  dwc->dr_mode == USB_DR_MODE_OTG) && DWC3_VER_IS(DWC31, 200A))
> > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> >               break;
> >       default:
> >               /* nothing */
> > --
> > 2.17.1
> >
>
> This doesn't seem like it should be applied globally. Please provide the
> STAR number if you can so I can review further. If possible I'd prefer
> to only target your platform.
>
Best Regards,
Jos Wang
> Thanks,
> Thinh

Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[Thinh Nguyen]

On Tue, Jun 04, 2024, joswang wrote:
> On Tue, Jun 4, 2024 at 8:07 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Mon, Jun 03, 2024, joswang wrote:
> > > From: joswang <joswang@lenovo.com>
> > >
> > > DWC31 version 2.00a have an issue that would cause
> > > a CSR read timeout When CSR read coincides with RAM
> > > Clock Gating Entry.
> >
> > Do you have the STAR issue number?
> >
> Thanks for reviewing the code.
> The STAR number provided by Synopsys is 4846132.
> Please help review further.
> 

I'll get back on this. Please also provide the STAR for the other case.

Thanks,
Thinh

Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[Thinh Nguyen]

On Tue, Jun 04, 2024, joswang wrote:
> On Tue, Jun 4, 2024 at 8:07 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Mon, Jun 03, 2024, joswang wrote:
> > > From: joswang <joswang@lenovo.com>
> > >
> > > DWC31 version 2.00a have an issue that would cause
> > > a CSR read timeout When CSR read coincides with RAM
> > > Clock Gating Entry.
> >
> > Do you have the STAR issue number?
> >
> Thanks for reviewing the code.
> The STAR number provided by Synopsys is 4846132.
> Please help review further.

I've confirmed internally. As you have noted, this applies to DWC_usb31
v2.00a for host mode only and DRD mode operating as host.

> 
> > >
> > > This workaround solution disable Clock Gating, sacrificing
> > > power consumption for normal operation.
> > >
> > > Signed-off-by: joswang <joswang@lenovo.com>
> > > ---
> > >  drivers/usb/dwc3/core.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 3a8fbc2d6b99..1df85c505c9e 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -978,11 +978,22 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > >                *
> > >                * STAR#9000588375: Clock Gating, SOF Issues when ref_clk-Based
> > >                * SOF/ITP Mode Used

Since there's another STAR, let's split the if-else case separately and
provide the comments separately.

> > > +              *
> > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would

Can we use the full name DWC_usb31 instead of DWC31.

> > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > +              * Clock Gating Entry.
> > > +              *
> > > +              * This workaround solution disable Clock Gating, sacrificing
> > > +              * power consumption for normal operation.
> > >                */
> > >               if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > >                               dwc->dr_mode == USB_DR_MODE_OTG) &&
> > >                               DWC3_VER_IS_WITHIN(DWC3, 210A, 250A))
> > >                       reg |= DWC3_GCTL_DSBLCLKGTNG | DWC3_GCTL_SOFITPSYNC;
> > > +             else if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > +                             dwc->dr_mode == USB_DR_MODE_OTG) &&

There's no OTG mode for DWC_usb31. Let's enable this workaround if the
HW mode is not DWC_GHWPARAMS0_MODE_GADGET.

> > > +                             DWC3_VER_IS(DWC31, 200A))
> > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > >               else
> > >                       reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> > >               break;
> > > @@ -992,6 +1003,18 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > >                * will work. Device-mode hibernation is not yet implemented.
> > >                */
> > >               reg |= DWC3_GCTL_GBLHIBERNATIONEN;
> > > +
> > > +             /*
> > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > +              * Clock Gating Entry.
> > > +              *
> > > +              * This workaround solution disable Clock Gating, sacrificing
> > > +              * power consumption for normal operation.
> > > +              */
> > > +             if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > +                  dwc->dr_mode == USB_DR_MODE_OTG) && DWC3_VER_IS(DWC31, 200A))
> > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > >               break;
> > >       default:
> > >               /* nothing */
> > > --
> > > 2.17.1
> > >
> >

We have the same checks and comments here. Can we refactor?
Perhaps something this?

power_opt = DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1);
switch (power_opt) {
    ...
}

/*
 * <comment>
 */
if (power_opt != DWC3_GHWPARAMS1_EN_PWROPT_NO) {
}


Thanks,
Thinh

Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[joswang]

On Thu, Jun 6, 2024 at 9:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Tue, Jun 04, 2024, joswang wrote:
> > On Tue, Jun 4, 2024 at 8:07 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > >
> > > On Mon, Jun 03, 2024, joswang wrote:
> > > > From: joswang <joswang@lenovo.com>
> > > >
> > > > DWC31 version 2.00a have an issue that would cause
> > > > a CSR read timeout When CSR read coincides with RAM
> > > > Clock Gating Entry.
> > >
> > > Do you have the STAR issue number?
> > >
> > Thanks for reviewing the code.
> > The STAR number provided by Synopsys is 4846132.
> > Please help review further.
>
> I've confirmed internally. As you have noted, this applies to DWC_usb31
> v2.00a for host mode only and DRD mode operating as host.
>
> >
> > > >
> > > > This workaround solution disable Clock Gating, sacrificing
> > > > power consumption for normal operation.
> > > >
> > > > Signed-off-by: joswang <joswang@lenovo.com>
> > > > ---
> > > >  drivers/usb/dwc3/core.c | 23 +++++++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 3a8fbc2d6b99..1df85c505c9e 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -978,11 +978,22 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > > >                *
> > > >                * STAR#9000588375: Clock Gating, SOF Issues when ref_clk-Based
> > > >                * SOF/ITP Mode Used
>
> Since there's another STAR, let's split the if-else case separately and
> provide the comments separately.
>
OK
> > > > +              *
> > > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
>
> Can we use the full name DWC_usb31 instead of DWC31.
>
Subsequent V3 versions use DWC_usb31
> > > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > > +              * Clock Gating Entry.
> > > > +              *
> > > > +              * This workaround solution disable Clock Gating, sacrificing
> > > > +              * power consumption for normal operation.
> > > >                */
> > > >               if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > >                               dwc->dr_mode == USB_DR_MODE_OTG) &&
> > > >                               DWC3_VER_IS_WITHIN(DWC3, 210A, 250A))
> > > >                       reg |= DWC3_GCTL_DSBLCLKGTNG | DWC3_GCTL_SOFITPSYNC;
> > > > +             else if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > +                             dwc->dr_mode == USB_DR_MODE_OTG) &&
>
> There's no OTG mode for DWC_usb31. Let's enable this workaround if the
> HW mode is not DWC_GHWPARAMS0_MODE_GADGET.
>
> > > > +                             DWC3_VER_IS(DWC31, 200A))
> > > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > > >               else
> > > >                       reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> > > >               break;
> > > > @@ -992,6 +1003,18 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > > >                * will work. Device-mode hibernation is not yet implemented.
> > > >                */
> > > >               reg |= DWC3_GCTL_GBLHIBERNATIONEN;
> > > > +
> > > > +             /*
> > > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> > > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > > +              * Clock Gating Entry.
> > > > +              *
> > > > +              * This workaround solution disable Clock Gating, sacrificing
> > > > +              * power consumption for normal operation.
> > > > +              */
> > > > +             if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > +                  dwc->dr_mode == USB_DR_MODE_OTG) && DWC3_VER_IS(DWC31, 200A))
> > > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > > >               break;
> > > >       default:
> > > >               /* nothing */
> > > > --
> > > > 2.17.1
> > > >
> > >
>
> We have the same checks and comments here. Can we refactor?
> Perhaps something this?
>
> power_opt = DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1);
> switch (power_opt) {
>     ...
> }
>
> /*
>  * <comment>
>  */
> if (power_opt != DWC3_GHWPARAMS1_EN_PWROPT_NO) {
> }
>
>
> Thanks,
> Thinh

Thank you for your valuable suggestions.I can refactor according to
your suggestion.
Do I need to submit a V3 version patch separately, or should I submit
a V3 version patch together with other cases?

Thanks,
Jos Wang

Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[Thinh Nguyen]

On Fri, Jun 07, 2024, joswang wrote:
> On Thu, Jun 6, 2024 at 9:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Tue, Jun 04, 2024, joswang wrote:
> > > On Tue, Jun 4, 2024 at 8:07 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > > >
> > > > On Mon, Jun 03, 2024, joswang wrote:
> > > > > From: joswang <joswang@lenovo.com>
> > > > >
> > > > > DWC31 version 2.00a have an issue that would cause
> > > > > a CSR read timeout When CSR read coincides with RAM
> > > > > Clock Gating Entry.
> > > >
> > > > Do you have the STAR issue number?
> > > >
> > > Thanks for reviewing the code.
> > > The STAR number provided by Synopsys is 4846132.
> > > Please help review further.
> >
> > I've confirmed internally. As you have noted, this applies to DWC_usb31
> > v2.00a for host mode only and DRD mode operating as host.
> >
> > >
> > > > >
> > > > > This workaround solution disable Clock Gating, sacrificing
> > > > > power consumption for normal operation.
> > > > >
> > > > > Signed-off-by: joswang <joswang@lenovo.com>
> > > > > ---
> > > > >  drivers/usb/dwc3/core.c | 23 +++++++++++++++++++++++
> > > > >  1 file changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > index 3a8fbc2d6b99..1df85c505c9e 100644
> > > > > --- a/drivers/usb/dwc3/core.c
> > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > @@ -978,11 +978,22 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > > > >                *
> > > > >                * STAR#9000588375: Clock Gating, SOF Issues when ref_clk-Based
> > > > >                * SOF/ITP Mode Used
> >
> > Since there's another STAR, let's split the if-else case separately and
> > provide the comments separately.
> >
> OK
> > > > > +              *
> > > > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> >
> > Can we use the full name DWC_usb31 instead of DWC31.
> >
> Subsequent V3 versions use DWC_usb31
> > > > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > > > +              * Clock Gating Entry.
> > > > > +              *
> > > > > +              * This workaround solution disable Clock Gating, sacrificing
> > > > > +              * power consumption for normal operation.
> > > > >                */
> > > > >               if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > >                               dwc->dr_mode == USB_DR_MODE_OTG) &&
> > > > >                               DWC3_VER_IS_WITHIN(DWC3, 210A, 250A))
> > > > >                       reg |= DWC3_GCTL_DSBLCLKGTNG | DWC3_GCTL_SOFITPSYNC;
> > > > > +             else if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > > +                             dwc->dr_mode == USB_DR_MODE_OTG) &&
> >
> > There's no OTG mode for DWC_usb31. Let's enable this workaround if the
> > HW mode is not DWC_GHWPARAMS0_MODE_GADGET.
> >
> > > > > +                             DWC3_VER_IS(DWC31, 200A))
> > > > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > > > >               else
> > > > >                       reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> > > > >               break;
> > > > > @@ -992,6 +1003,18 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > > > >                * will work. Device-mode hibernation is not yet implemented.
> > > > >                */
> > > > >               reg |= DWC3_GCTL_GBLHIBERNATIONEN;
> > > > > +
> > > > > +             /*
> > > > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> > > > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > > > +              * Clock Gating Entry.
> > > > > +              *
> > > > > +              * This workaround solution disable Clock Gating, sacrificing
> > > > > +              * power consumption for normal operation.
> > > > > +              */
> > > > > +             if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > > +                  dwc->dr_mode == USB_DR_MODE_OTG) && DWC3_VER_IS(DWC31, 200A))
> > > > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > > > >               break;
> > > > >       default:
> > > > >               /* nothing */
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> >
> > We have the same checks and comments here. Can we refactor?
> > Perhaps something this?
> >
> > power_opt = DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1);
> > switch (power_opt) {
> >     ...
> > }
> >
> > /*
> >  * <comment>
> >  */
> > if (power_opt != DWC3_GHWPARAMS1_EN_PWROPT_NO) {
> > }
> >
> >
> > Thanks,
> > Thinh
> 
> Thank you for your valuable suggestions.I can refactor according to
> your suggestion.
> Do I need to submit a V3 version patch separately, or should I submit
> a V3 version patch together with other cases?

I haven't reviewed the other case in detail yet. I'll get back on that.

It may be better if you can submit this separatedly so that the other
case won't hold this back (and it maybe easier for tracking too).

Thanks,
Thinh

Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[Thinh Nguyen]

On Fri, Jun 07, 2024, joswang wrote:

> My initial idea was similar to yours,Please help review the following changes.


> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 3a8fbc2d6b99..8c6a09718737 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -961,11 +961,15 @@ static bool dwc3_core_is_valid(struct dwc3 *dwc)
>  static void dwc3_core_setup_global_control(struct dwc3 *dwc)
>  {
>         u32 reg;
> +       unsigned int power_opt;
> +       unsigned int hw_mode;

Use reverse christmas tree declaration style:

	type1 abcdefg
	type2 abcde
	type3 abc

>  
>         reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>         reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> +       hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +       power_opt = DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1);
>  
> -       switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> +       switch (power_opt) {
>         case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
>                 /**
>                  * WORKAROUND: DWC3 revisions between 2.10a and 2.50a have an
> @@ -998,6 +1002,18 @@ static void dwc3_core_setup_global_control(struct dwc3
> *dwc)
>                 break;
>         }
>  
> +       /*
> +        * WORKAROUND: DWC_usb31 version 2.00a have an issue that would
> +        * cause a CSR read timeout When CSR read coincides with RAM
> +        * Clock Gating Entry.

Note in the comment and commit message that this applies while operating
as host mode. Add the STAR number reference in the commit message.

> +        *
> +        * This workaround solution disable Clock Gating, sacrificing
> +        * power consumption for normal operation.
> +        */
> +       if (power_opt != DWC3_GHWPARAMS1_EN_PWROPT_NO &&
> +           hw_mode != DWC3_GHWPARAMS0_MODE_GADGET && DWC3_VER_IS(DWC31, 200A))
> +               reg |= DWC3_GCTL_DSBLCLKGTNG;
> +

Thanks,
Thinh

[PATCH v3, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[joswang]

From: Jos Wang <joswang@lenovo.com>

There is an issue with the DWC31 2.00a and earlier versions
where the controller link power state transition from
P3/P3CPM/P4 to P2 may take longer than expected, ultimately
resulting in the hibernation D3 entering time exceeding the
expected 10ms.

Add a new 'snps,p2p3tranok-quirk' DT quirk to dwc3 core
for enable the controller transitions directly from phy
power state P2 to P3 or from state P3 to P2.

Note that this can only be set if the USB3 PHY supports
direct p3 to p2 or p2 to p3 conversion.

Signed-off-by: Jos Wang <joswang@lenovo.com>
---
v1 -> v2:
- v1 did not add this PATCH
v2 -> v3:
- modify Author Jos Wang
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 1cd0ca90127d..721927495887 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -242,6 +242,13 @@ properties:
       When set, all HighSpeed bus instances in park mode are disabled.
     type: boolean
 
+  snps,p2p3tranok-quirk:
+    description:
+      When set, the controller transitions directly from phy power state
+      P2 to P3 or from state P3 to P2. Note that this can only be set
+      if the USB3 PHY supports direct p3 to p2 or p2 to p3 conversion.
+    type: boolean
+
   snps,dis_metastability_quirk:
     description:
       When set, disable metastability workaround. CAUTION! Use only if you are
-- 
2.17.1

[PATCH v4, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[joswang]

From: Jos Wang <joswang@lenovo.com>

There is an issue with the DWC31 2.00a and earlier versions
where the controller link power state transition from
P3/P3CPM/P4 to P2 may take longer than expected, ultimately
resulting in the hibernation D3 entering time exceeding the
expected 10ms.

Add a new 'snps,p2p3tranok-quirk' DT quirk to dwc3 core
for enable the controller transitions directly from phy
power state P2 to P3 or from state P3 to P2.

Note that this can only be set if the USB3 PHY supports
direct p3 to p2 or p2 to p3 conversion.

Signed-off-by: Jos Wang <joswang@lenovo.com>
---
v1 -> v2:
- v1 did not add this PATCH
v2 -> v3:
- modify Author Jos Wang
v3 -> v4:
- no change
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 1cd0ca90127d..721927495887 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -242,6 +242,13 @@ properties:
       When set, all HighSpeed bus instances in park mode are disabled.
     type: boolean
 
+  snps,p2p3tranok-quirk:
+    description:
+      When set, the controller transitions directly from phy power state
+      P2 to P3 or from state P3 to P2. Note that this can only be set
+      if the USB3 PHY supports direct p3 to p2 or p2 to p3 conversion.
+    type: boolean
+
   snps,dis_metastability_quirk:
     description:
       When set, disable metastability workaround. CAUTION! Use only if you are
-- 
2.17.1

Re: [PATCH v4, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[Krzysztof Kozlowski]

On 12/06/2024 17:23, joswang wrote:
>  
> +  snps,p2p3tranok-quirk:
> +    description:
> +      When set, the controller transitions directly from phy power state
> +      P2 to P3 or from state P3 to P2. Note that this can only be set
> +      if the USB3 PHY supports direct p3 to p2 or p2 to p3 conversion.
> +    type: boolean

Hm? You respond to feedback and, without waiting for my answer,
immediately send new version?

No. Read feedback on your previous version. Drop the quirk.

Best regards,
Krzysztof

Re: [PATCH v4, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[joswang]

On Thu, Jun 13, 2024 at 2:17 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 12/06/2024 17:23, joswang wrote:
> >
> > +  snps,p2p3tranok-quirk:
> > +    description:
> > +      When set, the controller transitions directly from phy power state
> > +      P2 to P3 or from state P3 to P2. Note that this can only be set
> > +      if the USB3 PHY supports direct p3 to p2 or p2 to p3 conversion.
> > +    type: boolean
>
> Hm? You respond to feedback and, without waiting for my answer,
> immediately send new version?
>
> No. Read feedback on your previous version. Drop the quirk.
>
> Best regards,
> Krzysztof
>

Thank you for your help in reviewing the code.
Sorry, I submitted three patches in total. Patch1 (the current patch)
and patch2 solve one case, and patch3 solves another case. Because
patch3 needs to submit a new version, I resubmitted v3 and v4
versions.
Patch2 is under review, and there is no clear conclusion. For now,
patch1 does not need to be paid attention to. I will notify you when
patch2 has a clear conclusion.

Thanks,

Jos Wang

Re: [PATCH v4, 1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk

[Krzysztof Kozlowski]

On 13/06/2024 15:19, joswang wrote:
> On Thu, Jun 13, 2024 at 2:17 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 12/06/2024 17:23, joswang wrote:
>>>
>>> +  snps,p2p3tranok-quirk:
>>> +    description:
>>> +      When set, the controller transitions directly from phy power state
>>> +      P2 to P3 or from state P3 to P2. Note that this can only be set
>>> +      if the USB3 PHY supports direct p3 to p2 or p2 to p3 conversion.
>>> +    type: boolean
>>
>> Hm? You respond to feedback and, without waiting for my answer,
>> immediately send new version?
>>
>> No. Read feedback on your previous version. Drop the quirk.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Thank you for your help in reviewing the code.
> Sorry, I submitted three patches in total. Patch1 (the current patch)
> and patch2 solve one case, and patch3 solves another case. Because
> patch3 needs to submit a new version, I resubmitted v3 and v4
> versions.
> Patch2 is under review, and there is no clear conclusion. For now,
> patch1 does not need to be paid attention to. I will notify you when
> patch2 has a clear conclusion.

This does no work like this.

Implement feedback and send new version of entire patchset *ONCE* there
is conclusion. Sending new version of some parts ignoring feedback or
skipping conclusion is not the way.

Sorry.

Still drop.

Or in case we still have here misunderstanding - so far it looks like: NAK

Best regards,
Krzysztof