[PATCH] usb: dwc3: core: power on PHYs before initializing core

[PATCH] usb: dwc3: core: power on PHYs before initializing core

[William Wu]

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(-)

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);
-- 
2.0.0

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Brian Norris]

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(-)

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Enric Balletbo Serra]

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(-)

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]


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?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Brian Norris]

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

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Roger Quadros]

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.

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]


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 :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]