| Message ID | 1515729616-8639-1-git-send-email-william.wu@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 12, 2018 at 12:00:16PM +0800, William Wu wrote: > The dwc3_core_init() gets the PHYs and initializes the PHYs with > the usb_phy_init() and phy_init() functions before initializing > core, and power on the PHYs after core initialization is done. > > However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C > USB3 PHY), it needs to do some special operation while power on > the Type-C PHY before initializing DWC3 core. It's because that > the RK3399 Type-C PHY requires to hold the DWC3 controller in > reset state to keep the PIPE power state in P2 while configuring > the Type-C PHY, otherwise, it may cause waiting for the PIPE ready > timeout. In this case, if we power on the PHYs after the DWC3 core > initialization is done, the core will be reset to uninitialized > state after power on the PHYs. > > Fix this by powering on the PHYs before initializing core. And > because the GUID register may also be reset in this case, so we > need to configure the GUID register after powering on the PHYs. > > Signed-off-by: William Wu <william.wu@rock-chips.com> This kinda should be part of your series: [PATCH 0/3] Reset USB3 controller before initializing Type-C PHY on rk3399 or at least mentioned there, because the series there doesn't quite right otherwise, no? Anyway, I think this patch looks OK. I don't immediately see good reasons for delaying the PHY init until later, and I do see reasons why it could be useful earlier: Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > drivers/usb/dwc3/core.c | 46 ++++++++++++++++++++++------------------------ > 1 file changed, 22 insertions(+), 24 deletions(-)
2018-01-17 22:46 GMT+01:00 Brian Norris <briannorris@chromium.org>: > On Fri, Jan 12, 2018 at 12:00:16PM +0800, William Wu wrote: >> The dwc3_core_init() gets the PHYs and initializes the PHYs with >> the usb_phy_init() and phy_init() functions before initializing >> core, and power on the PHYs after core initialization is done. >> >> However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C >> USB3 PHY), it needs to do some special operation while power on >> the Type-C PHY before initializing DWC3 core. It's because that >> the RK3399 Type-C PHY requires to hold the DWC3 controller in >> reset state to keep the PIPE power state in P2 while configuring >> the Type-C PHY, otherwise, it may cause waiting for the PIPE ready >> timeout. In this case, if we power on the PHYs after the DWC3 core >> initialization is done, the core will be reset to uninitialized >> state after power on the PHYs. >> >> Fix this by powering on the PHYs before initializing core. And >> because the GUID register may also be reset in this case, so we >> need to configure the GUID register after powering on the PHYs. >> >> Signed-off-by: William Wu <william.wu@rock-chips.com> > > This kinda should be part of your series: > > [PATCH 0/3] Reset USB3 controller before initializing Type-C PHY on rk3399 > +1 > or at least mentioned there, because the series there doesn't quite > right otherwise, no? > > Anyway, I think this patch looks OK. I don't immediately see good > reasons for delaying the PHY init until later, and I do see reasons why > it could be useful earlier: > > Reviewed-by: Brian Norris <briannorris@chromium.org> Tested on Samsung Chromebook Plus with current mainline Tested-by: Enric Balletbo i Serra <enric.balletbo@chromium.org> > >> --- >> drivers/usb/dwc3/core.c | 46 ++++++++++++++++++++++------------------------ >> 1 file changed, 22 insertions(+), 24 deletions(-)
Hi Roger, William Wu <william.wu@rock-chips.com> writes: > The dwc3_core_init() gets the PHYs and initializes the PHYs with > the usb_phy_init() and phy_init() functions before initializing > core, and power on the PHYs after core initialization is done. > > However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C > USB3 PHY), it needs to do some special operation while power on > the Type-C PHY before initializing DWC3 core. It's because that > the RK3399 Type-C PHY requires to hold the DWC3 controller in > reset state to keep the PIPE power state in P2 while configuring > the Type-C PHY, otherwise, it may cause waiting for the PIPE ready > timeout. In this case, if we power on the PHYs after the DWC3 core > initialization is done, the core will be reset to uninitialized > state after power on the PHYs. > > Fix this by powering on the PHYs before initializing core. And > because the GUID register may also be reset in this case, so we > need to configure the GUID register after powering on the PHYs. > > Signed-off-by: William Wu <william.wu@rock-chips.com> does this cause any regressions for your boards?
Hi, On Thu, Mar 08, 2018 at 12:43:40PM +0200, Felipe Balbi wrote: > William Wu <william.wu@rock-chips.com> writes: > > The dwc3_core_init() gets the PHYs and initializes the PHYs with > > the usb_phy_init() and phy_init() functions before initializing > > core, and power on the PHYs after core initialization is done. > > > > However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C > > USB3 PHY), it needs to do some special operation while power on > > the Type-C PHY before initializing DWC3 core. It's because that > > the RK3399 Type-C PHY requires to hold the DWC3 controller in > > reset state to keep the PIPE power state in P2 while configuring > > the Type-C PHY, otherwise, it may cause waiting for the PIPE ready > > timeout. In this case, if we power on the PHYs after the DWC3 core > > initialization is done, the core will be reset to uninitialized > > state after power on the PHYs. > > > > Fix this by powering on the PHYs before initializing core. And > > because the GUID register may also be reset in this case, so we > > need to configure the GUID register after powering on the PHYs. > > > > Signed-off-by: William Wu <william.wu@rock-chips.com> > > does this cause any regressions for your boards? I'm not Roger, but I believe it was determined we don't need this for the Rockchip systems for which William was originally sending this. At least not right now. I believe our PHY init problems were mostly resolved in other ways. (Although I hear USB is currently pretty broken around suspend/resume for us on -next. Likely unrelated.) I guess we never clearly replied stating the above. I hope this isn't merged anywhere? Or I guess it's no problem to me at the moment, but it might be needless churn. Brian
Hi, On 08/03/18 18:49, Brian Norris wrote: > Hi, > > On Thu, Mar 08, 2018 at 12:43:40PM +0200, Felipe Balbi wrote: >> William Wu <william.wu@rock-chips.com> writes: >>> The dwc3_core_init() gets the PHYs and initializes the PHYs with >>> the usb_phy_init() and phy_init() functions before initializing >>> core, and power on the PHYs after core initialization is done. >>> >>> However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C >>> USB3 PHY), it needs to do some special operation while power on >>> the Type-C PHY before initializing DWC3 core. It's because that >>> the RK3399 Type-C PHY requires to hold the DWC3 controller in >>> reset state to keep the PIPE power state in P2 while configuring >>> the Type-C PHY, otherwise, it may cause waiting for the PIPE ready >>> timeout. In this case, if we power on the PHYs after the DWC3 core >>> initialization is done, the core will be reset to uninitialized >>> state after power on the PHYs. >>> >>> Fix this by powering on the PHYs before initializing core. And >>> because the GUID register may also be reset in this case, so we >>> need to configure the GUID register after powering on the PHYs. >>> >>> Signed-off-by: William Wu <william.wu@rock-chips.com> >> >> does this cause any regressions for your boards? > > I'm not Roger, but I believe it was determined we don't need this for > the Rockchip systems for which William was originally sending this. At > least not right now. I believe our PHY init problems were mostly > resolved in other ways. > > (Although I hear USB is currently pretty broken around suspend/resume > for us on -next. Likely unrelated.) > > I guess we never clearly replied stating the above. I hope this isn't > merged anywhere? Or I guess it's no problem to me at the moment, but it > might be needless churn. > I did some quick tests on TI platforms and didn't see any issues with this patch. Since this patch isn't really fixing your problem and we didn't have any problems to start with I'd suggest to avoid this churn for now.
Hi, Roger Quadros <rogerq@ti.com> writes: > Hi, > > On 08/03/18 18:49, Brian Norris wrote: >> Hi, >> >> On Thu, Mar 08, 2018 at 12:43:40PM +0200, Felipe Balbi wrote: >>> William Wu <william.wu@rock-chips.com> writes: >>>> The dwc3_core_init() gets the PHYs and initializes the PHYs with >>>> the usb_phy_init() and phy_init() functions before initializing >>>> core, and power on the PHYs after core initialization is done. >>>> >>>> However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C >>>> USB3 PHY), it needs to do some special operation while power on >>>> the Type-C PHY before initializing DWC3 core. It's because that >>>> the RK3399 Type-C PHY requires to hold the DWC3 controller in >>>> reset state to keep the PIPE power state in P2 while configuring >>>> the Type-C PHY, otherwise, it may cause waiting for the PIPE ready >>>> timeout. In this case, if we power on the PHYs after the DWC3 core >>>> initialization is done, the core will be reset to uninitialized >>>> state after power on the PHYs. >>>> >>>> Fix this by powering on the PHYs before initializing core. And >>>> because the GUID register may also be reset in this case, so we >>>> need to configure the GUID register after powering on the PHYs. >>>> >>>> Signed-off-by: William Wu <william.wu@rock-chips.com> >>> >>> does this cause any regressions for your boards? >> >> I'm not Roger, but I believe it was determined we don't need this for >> the Rockchip systems for which William was originally sending this. At >> least not right now. I believe our PHY init problems were mostly >> resolved in other ways. >> >> (Although I hear USB is currently pretty broken around suspend/resume >> for us on -next. Likely unrelated.) >> >> I guess we never clearly replied stating the above. I hope this isn't >> merged anywhere? Or I guess it's no problem to me at the moment, but it >> might be needless churn. >> > > I did some quick tests on TI platforms and didn't see any issues with this patch. > Since this patch isn't really fixing your problem and we didn't have any > problems to start with I'd suggest to avoid this churn for now. fair enough, I won't apply it :-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index c32d2b9..4f5573f 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -741,12 +741,6 @@ static int dwc3_core_init(struct dwc3 *dwc) goto err0; } - /* - * Write Linux Version Code to our GUID register so it's easy to figure - * out which kernel version a bug was found. - */ - dwc3_writel(dwc->regs, DWC3_GUID, LINUX_VERSION_CODE); - /* Handle USB2.0-only core configuration */ if (DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3) == DWC3_GHWPARAMS3_SSPHY_IFC_DIS) { @@ -762,34 +756,40 @@ static int dwc3_core_init(struct dwc3 *dwc) if (ret) goto err0; + usb_phy_set_suspend(dwc->usb2_phy, 0); + usb_phy_set_suspend(dwc->usb3_phy, 0); + ret = phy_power_on(dwc->usb2_generic_phy); + if (ret < 0) + goto err1; + + ret = phy_power_on(dwc->usb3_generic_phy); + if (ret < 0) + goto err2; + ret = dwc3_phy_setup(dwc); if (ret) - goto err0; + goto err3; + + /* + * Write Linux Version Code to our GUID register so it's easy to figure + * out which kernel version a bug was found. + */ + dwc3_writel(dwc->regs, DWC3_GUID, LINUX_VERSION_CODE); dwc3_core_setup_global_control(dwc); dwc3_core_num_eps(dwc); ret = dwc3_setup_scratch_buffers(dwc); if (ret) - goto err1; + goto err3; /* Adjust Frame Length */ dwc3_frame_length_adjustment(dwc); - usb_phy_set_suspend(dwc->usb2_phy, 0); - usb_phy_set_suspend(dwc->usb3_phy, 0); - ret = phy_power_on(dwc->usb2_generic_phy); - if (ret < 0) - goto err2; - - ret = phy_power_on(dwc->usb3_generic_phy); - if (ret < 0) - goto err3; - ret = dwc3_event_buffers_setup(dwc); if (ret) { dev_err(dwc->dev, "failed to setup event buffers\n"); - goto err4; + goto err3; } /* @@ -821,17 +821,15 @@ static int dwc3_core_init(struct dwc3 *dwc) return 0; -err4: +err3: phy_power_off(dwc->usb3_generic_phy); -err3: +err2: phy_power_off(dwc->usb2_generic_phy); -err2: +err1: usb_phy_set_suspend(dwc->usb2_phy, 1); usb_phy_set_suspend(dwc->usb3_phy, 1); - -err1: usb_phy_shutdown(dwc->usb2_phy); usb_phy_shutdown(dwc->usb3_phy); phy_exit(dwc->usb2_generic_phy);
The dwc3_core_init() gets the PHYs and initializes the PHYs with the usb_phy_init() and phy_init() functions before initializing core, and power on the PHYs after core initialization is done. However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C USB3 PHY), it needs to do some special operation while power on the Type-C PHY before initializing DWC3 core. It's because that the RK3399 Type-C PHY requires to hold the DWC3 controller in reset state to keep the PIPE power state in P2 while configuring the Type-C PHY, otherwise, it may cause waiting for the PIPE ready timeout. In this case, if we power on the PHYs after the DWC3 core initialization is done, the core will be reset to uninitialized state after power on the PHYs. Fix this by powering on the PHYs before initializing core. And because the GUID register may also be reset in this case, so we need to configure the GUID register after powering on the PHYs. Signed-off-by: William Wu <william.wu@rock-chips.com> --- drivers/usb/dwc3/core.c | 46 ++++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 24 deletions(-)