| Message ID | 20240603130126.25758-1-joswang1221@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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
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
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
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
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
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;