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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 46 ++++++++++++++++++++++------------------------
> 1 file changed, 22 insertions(+), 24 deletions(-)
2018-01-17 22:46 GMT+01:00 Brian Norris <[email protected]>:
> 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 <[email protected]>
>
> 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 <[email protected]>
Tested on Samsung Chromebook Plus with current mainline
Tested-by: Enric Balletbo i Serra <[email protected]>
>
>> ---
>> drivers/usb/dwc3/core.c | 46 ++++++++++++++++++++++------------------------
>> 1 file changed, 22 insertions(+), 24 deletions(-)
Hi Roger,
William Wu <[email protected]> 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 <[email protected]>
does this cause any regressions for your boards?
--
balbi
Hi,
On Thu, Mar 08, 2018 at 12:43:40PM +0200, Felipe Balbi wrote:
> William Wu <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>>
>> 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
Hi,
Roger Quadros <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>
>>> 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