[PATCH] usb: phy: tegra: Fix phy suspend for UDC

[PATCH] usb: phy: tegra: Fix phy suspend for UDC

[Jon Hunter]

Commit dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
added UDC support for Tegra but with UDC support enabled, is was found
that Tegra30, Tegra114 and Tegra124 would hang on entry to suspend.

The hang occurred during the suspend of the USB PHY when the Tegra PHY
driver attempted to disable the PHY clock. The problem is that before
the Tegra PHY driver is suspended, the chipidea driver already disabled
the PHY clock and when the Tegra PHY driver suspended, it could not read
DEVLC register and caused the device to hang.

The Tegra USB PHY driver is used by both the Tegra EHCI driver and now
the chipidea UDC driver and so simply removing the disabling of the PHY
clock from the USB PHY driver would not work for the Tegra EHCI driver.
Fortunately, the status of the USB PHY clock can be read from the
USB_SUSP_CTRL register and therefore, to workaround this issue, simply
poll the register prior to disabling the clock in USB PHY driver to see
if clock gating has already been initiated. Please note that it can take
a few uS for the clock to disable and so simply reading this status
register once on entry is not sufficient.

Please note that no issues are seen with Tegra20 because it has a slightly
different PHY to Tegra30/114/124.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/phy/phy-tegra-usb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 5fe4a5704bde..c8fff99bd16e 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -329,6 +329,14 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 	unsigned long val;
 	void __iomem *base = phy->regs;
 
+	/*
+	 * The USB driver may have already initiated the phy clock
+	 * disable so wait to see if the clock turns off and if not
+	 * then proceed with gating the clock.
+	 */
+	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) == 0)
+		return;
+
 	if (phy->is_legacy_phy) {
 		val = readl(base + USB_SUSP_CTRL);
 		val |= USB_SUSP_SET;
-- 
2.7.4

Re: [PATCH] usb: phy: tegra: Fix phy suspend for UDC

[Jon Hunter]

Adding Dmitry ...

Felipe, Thierry, this is needed for v4.14-rc because suspend is currently
broken for some Tegra devices.

Jon

On 29/09/17 14:06, Jon Hunter wrote:
> Commit dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
> added UDC support for Tegra but with UDC support enabled, is was found
> that Tegra30, Tegra114 and Tegra124 would hang on entry to suspend.
> 
> The hang occurred during the suspend of the USB PHY when the Tegra PHY
> driver attempted to disable the PHY clock. The problem is that before
> the Tegra PHY driver is suspended, the chipidea driver already disabled
> the PHY clock and when the Tegra PHY driver suspended, it could not read
> DEVLC register and caused the device to hang.
> 
> The Tegra USB PHY driver is used by both the Tegra EHCI driver and now
> the chipidea UDC driver and so simply removing the disabling of the PHY
> clock from the USB PHY driver would not work for the Tegra EHCI driver.
> Fortunately, the status of the USB PHY clock can be read from the
> USB_SUSP_CTRL register and therefore, to workaround this issue, simply
> poll the register prior to disabling the clock in USB PHY driver to see
> if clock gating has already been initiated. Please note that it can take
> a few uS for the clock to disable and so simply reading this status
> register once on entry is not sufficient.
> 
> Please note that no issues are seen with Tegra20 because it has a slightly
> different PHY to Tegra30/114/124.
>

I forgot the fixes tag ...

Fixes: dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/usb/phy/phy-tegra-usb.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index 5fe4a5704bde..c8fff99bd16e 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c
> @@ -329,6 +329,14 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>  	unsigned long val;
>  	void __iomem *base = phy->regs;
>  
> +	/*
> +	 * The USB driver may have already initiated the phy clock
> +	 * disable so wait to see if the clock turns off and if not
> +	 * then proceed with gating the clock.
> +	 */
> +	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) == 0)
> +		return;
> +
>  	if (phy->is_legacy_phy) {
>  		val = readl(base + USB_SUSP_CTRL);
>  		val |= USB_SUSP_SET;
> 

-- 
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: phy: tegra: Fix phy suspend for UDC

[Dmitry Osipenko]

On 29.09.2017 16:10, Jon Hunter wrote:
> Adding Dmitry ...
> 
> Felipe, Thierry, this is needed for v4.14-rc because suspend is currently
> broken for some Tegra devices.
> 
> Jon
> 
> On 29/09/17 14:06, Jon Hunter wrote:
>> Commit dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
>> added UDC support for Tegra but with UDC support enabled, is was found
>> that Tegra30, Tegra114 and Tegra124 would hang on entry to suspend.
>>
>> The hang occurred during the suspend of the USB PHY when the Tegra PHY
>> driver attempted to disable the PHY clock. The problem is that before
>> the Tegra PHY driver is suspended, the chipidea driver already disabled
>> the PHY clock and when the Tegra PHY driver suspended, it could not read
>> DEVLC register and caused the device to hang.
>>
>> The Tegra USB PHY driver is used by both the Tegra EHCI driver and now
>> the chipidea UDC driver and so simply removing the disabling of the PHY
>> clock from the USB PHY driver would not work for the Tegra EHCI driver.
>> Fortunately, the status of the USB PHY clock can be read from the
>> USB_SUSP_CTRL register and therefore, to workaround this issue, simply
>> poll the register prior to disabling the clock in USB PHY driver to see
>> if clock gating has already been initiated. Please note that it can take
>> a few uS for the clock to disable and so simply reading this status
>> register once on entry is not sufficient.
>>
>> Please note that no issues are seen with Tegra20 because it has a slightly
>> different PHY to Tegra30/114/124.
>>
> 
> I forgot the fixes tag ...
> 
> Fixes: dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
>  
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/usb/phy/phy-tegra-usb.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>> index 5fe4a5704bde..c8fff99bd16e 100644
>> --- a/drivers/usb/phy/phy-tegra-usb.c
>> +++ b/drivers/usb/phy/phy-tegra-usb.c
>> @@ -329,6 +329,14 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>>  	unsigned long val;
>>  	void __iomem *base = phy->regs;
>>  
>> +	/*
>> +	 * The USB driver may have already initiated the phy clock
>> +	 * disable so wait to see if the clock turns off and if not
>> +	 * then proceed with gating the clock.
>> +	 */
>> +	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) == 0)
>> +		return;
>> +
>>  	if (phy->is_legacy_phy) {
>>  		val = readl(base + USB_SUSP_CTRL);
>>  		val |= USB_SUSP_SET;
>>
> 

Adding analogical check to clk_enable fixes "utmi_phy_clk_enable: timeout
waiting for phy to stabilize" error message during of kernel boot-up on Tegra20.

What about to incorporate it to this patch?

@@ -354,16 +354,25 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 		pr_err("%s: timeout waiting for phy to stabilize\n", __func__);
 }

 static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 {
 	unsigned long val;
 	void __iomem *base = phy->regs;

+	/*
+	 * The USB driver may have already initiated the phy clock
+	 * enable so wait to see if the clock turns on and if not
+	 * then proceed with ungating the clock.
+	 */
+	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID,
+						     USB_PHY_CLK_VALID) == 0)
+		return;
+
 	if (phy->is_legacy_phy) {
 		val = readl(base + USB_SUSP_CTRL);
 		val |= USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);

 		udelay(10);

 		val = readl(base + USB_SUSP_CTRL);

-- 
Dmitry

Re: [PATCH] usb: phy: tegra: Fix phy suspend for UDC

[Dmitry Osipenko]

On 29.09.2017 17:57, Dmitry Osipenko wrote:
> On 29.09.2017 16:10, Jon Hunter wrote:
>> Adding Dmitry ...
>>
>> Felipe, Thierry, this is needed for v4.14-rc because suspend is currently
>> broken for some Tegra devices.
>>
>> Jon
>>
>> On 29/09/17 14:06, Jon Hunter wrote:
>>> Commit dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
>>> added UDC support for Tegra but with UDC support enabled, is was found
>>> that Tegra30, Tegra114 and Tegra124 would hang on entry to suspend.
>>>
>>> The hang occurred during the suspend of the USB PHY when the Tegra PHY
>>> driver attempted to disable the PHY clock. The problem is that before
>>> the Tegra PHY driver is suspended, the chipidea driver already disabled
>>> the PHY clock and when the Tegra PHY driver suspended, it could not read
>>> DEVLC register and caused the device to hang.
>>>
>>> The Tegra USB PHY driver is used by both the Tegra EHCI driver and now
>>> the chipidea UDC driver and so simply removing the disabling of the PHY
>>> clock from the USB PHY driver would not work for the Tegra EHCI driver.
>>> Fortunately, the status of the USB PHY clock can be read from the
>>> USB_SUSP_CTRL register and therefore, to workaround this issue, simply
>>> poll the register prior to disabling the clock in USB PHY driver to see
>>> if clock gating has already been initiated. Please note that it can take
>>> a few uS for the clock to disable and so simply reading this status
>>> register once on entry is not sufficient.
>>>
>>> Please note that no issues are seen with Tegra20 because it has a slightly
>>> different PHY to Tegra30/114/124.
>>>
>>
>> I forgot the fixes tag ...
>>
>> Fixes: dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
>>  
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/usb/phy/phy-tegra-usb.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>>> index 5fe4a5704bde..c8fff99bd16e 100644
>>> --- a/drivers/usb/phy/phy-tegra-usb.c
>>> +++ b/drivers/usb/phy/phy-tegra-usb.c
>>> @@ -329,6 +329,14 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>>>  	unsigned long val;
>>>  	void __iomem *base = phy->regs;
>>>  
>>> +	/*
>>> +	 * The USB driver may have already initiated the phy clock
>>> +	 * disable so wait to see if the clock turns off and if not
>>> +	 * then proceed with gating the clock.
>>> +	 */
>>> +	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) == 0)
>>> +		return;
>>> +
>>>  	if (phy->is_legacy_phy) {
>>>  		val = readl(base + USB_SUSP_CTRL);
>>>  		val |= USB_SUSP_SET;
>>>
>>
> 
> Adding analogical check to clk_enable fixes "utmi_phy_clk_enable: timeout
> waiting for phy to stabilize" error message during of kernel boot-up on Tegra20.
> 

'During of UDC probe' would me more correct to say and UDC driver still working
excellent.


-- 
Dmitry

Re: [PATCH] usb: phy: tegra: Fix phy suspend for UDC

[Jon Hunter]

On 29/09/17 15:57, Dmitry Osipenko wrote:
> On 29.09.2017 16:10, Jon Hunter wrote:
>> Adding Dmitry ...
>>
>> Felipe, Thierry, this is needed for v4.14-rc because suspend is currently
>> broken for some Tegra devices.
>>
>> Jon
>>
>> On 29/09/17 14:06, Jon Hunter wrote:
>>> Commit dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
>>> added UDC support for Tegra but with UDC support enabled, is was found
>>> that Tegra30, Tegra114 and Tegra124 would hang on entry to suspend.
>>>
>>> The hang occurred during the suspend of the USB PHY when the Tegra PHY
>>> driver attempted to disable the PHY clock. The problem is that before
>>> the Tegra PHY driver is suspended, the chipidea driver already disabled
>>> the PHY clock and when the Tegra PHY driver suspended, it could not read
>>> DEVLC register and caused the device to hang.
>>>
>>> The Tegra USB PHY driver is used by both the Tegra EHCI driver and now
>>> the chipidea UDC driver and so simply removing the disabling of the PHY
>>> clock from the USB PHY driver would not work for the Tegra EHCI driver.
>>> Fortunately, the status of the USB PHY clock can be read from the
>>> USB_SUSP_CTRL register and therefore, to workaround this issue, simply
>>> poll the register prior to disabling the clock in USB PHY driver to see
>>> if clock gating has already been initiated. Please note that it can take
>>> a few uS for the clock to disable and so simply reading this status
>>> register once on entry is not sufficient.
>>>
>>> Please note that no issues are seen with Tegra20 because it has a slightly
>>> different PHY to Tegra30/114/124.
>>>
>>
>> I forgot the fixes tag ...
>>
>> Fixes: dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
>>  
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/usb/phy/phy-tegra-usb.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>>> index 5fe4a5704bde..c8fff99bd16e 100644
>>> --- a/drivers/usb/phy/phy-tegra-usb.c
>>> +++ b/drivers/usb/phy/phy-tegra-usb.c
>>> @@ -329,6 +329,14 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>>>  	unsigned long val;
>>>  	void __iomem *base = phy->regs;
>>>  
>>> +	/*
>>> +	 * The USB driver may have already initiated the phy clock
>>> +	 * disable so wait to see if the clock turns off and if not
>>> +	 * then proceed with gating the clock.
>>> +	 */
>>> +	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) == 0)
>>> +		return;
>>> +
>>>  	if (phy->is_legacy_phy) {
>>>  		val = readl(base + USB_SUSP_CTRL);
>>>  		val |= USB_SUSP_SET;
>>>
>>
> 
> Adding analogical check to clk_enable fixes "utmi_phy_clk_enable: timeout
> waiting for phy to stabilize" error message during of kernel boot-up on Tegra20.
> 
> What about to incorporate it to this patch?
> 
> @@ -354,16 +354,25 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>  		pr_err("%s: timeout waiting for phy to stabilize\n", __func__);
>  }
> 
>  static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
>  {
>  	unsigned long val;
>  	void __iomem *base = phy->regs;
> 
> +	/*
> +	 * The USB driver may have already initiated the phy clock
> +	 * enable so wait to see if the clock turns on and if not
> +	 * then proceed with ungating the clock.
> +	 */
> +	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID,
> +						     USB_PHY_CLK_VALID) == 0)
> +		return;
> +
>  	if (phy->is_legacy_phy) {
>  		val = readl(base + USB_SUSP_CTRL);
>  		val |= USB_SUSP_CLR;
>  		writel(val, base + USB_SUSP_CTRL);
> 
>  		udelay(10);
> 
>  		val = readl(base + USB_SUSP_CTRL);
> 

Yes, I see that message as well. However, looks like that has been
around for a long time (for all v4.x kernels)! I also see the following
on Tegra20-trimslice (for all v4.x kernels as well) ...

[    1.852474] ulpi_phy_power_on: ulpi write failed
[    1.857086] tegra-ehci c5004000.usb: Failed to power on the phy
[    1.863039] tegra-ehci: probe of c5004000.usb failed with error -110

The above is not related to this issue though. Let me take a look at bit
closer at these messages.

Cheers
Jon

-- 
nvpublic