[PATCH v1] usb: phy: tegra: Increase PHY clock stabilization timeout

[PATCH v1] usb: phy: tegra: Increase PHY clock stabilization timeout

[Dmitry Osipenko]

This fixes "utmi_phy_clk_enable: timeout waiting for phy to stabilize"
error message.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/phy/phy-tegra-usb.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index f668bfb708d3..7d5db625f800 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -16,7 +16,7 @@
 #include <linux/export.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/gpio.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -305,14 +305,9 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy)
 
 static int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
 {
-	unsigned long timeout = 2000;
-	do {
-		if ((readl(reg) & mask) == result)
-			return 0;
-		udelay(1);
-		timeout--;
-	} while (timeout);
-	return -1;
+	u32 tmp;
+
+	return readl_poll_timeout(reg, tmp, (tmp & mask) == result, 1, 5000);
 }
 
 static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
-- 
2.15.1

--
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 v1] usb: phy: tegra: Increase PHY clock stabilization timeout

[Thierry Reding]

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

On Mon, Dec 11, 2017 at 01:55:35AM +0300, Dmitry Osipenko wrote:
> This fixes "utmi_phy_clk_enable: timeout waiting for phy to stabilize"
> error message.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/phy/phy-tegra-usb.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index f668bfb708d3..7d5db625f800 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c
> @@ -16,7 +16,7 @@
>  #include <linux/export.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/gpio.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -305,14 +305,9 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy)
>  
>  static int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
>  {
> -	unsigned long timeout = 2000;
> -	do {
> -		if ((readl(reg) & mask) == result)
> -			return 0;
> -		udelay(1);
> -		timeout--;
> -	} while (timeout);
> -	return -1;
> +	u32 tmp;
> +
> +	return readl_poll_timeout(reg, tmp, (tmp & mask) == result, 1, 5000);

Technically I think this should be readl_poll_timeout_atomic() because
the above used to use udelay() instead of usleep_range(). But since the
function is never used inside atomic context, this looks fine.

You may want to bump the sleep time between reads to something like 10
or 20. usleep_range() doesn't always work well with very short values,
and given that you already bump the timeout from 2 ms to 5 ms indicates
to me that we're actually spending a lot of time in this loop, and
iterating somewhere between 2000 and 5000 times isn't any good. We only
use this function to wait for the USB_PHY_CLK_VALID bit which happens
during clock enable and disable, which isn't going to be very often, so
even in the best case (where the clock is immediately valid) there's no
need to return within a microsecond.

Thierry

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

Re: [PATCH v1] usb: phy: tegra: Increase PHY clock stabilization timeout

[Dmitry Osipenko]

On 11.12.2017 12:53, Thierry Reding wrote:
> On Mon, Dec 11, 2017 at 01:55:35AM +0300, Dmitry Osipenko wrote:
>> This fixes "utmi_phy_clk_enable: timeout waiting for phy to stabilize"
>> error message.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/usb/phy/phy-tegra-usb.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>> index f668bfb708d3..7d5db625f800 100644
>> --- a/drivers/usb/phy/phy-tegra-usb.c
>> +++ b/drivers/usb/phy/phy-tegra-usb.c
>> @@ -16,7 +16,7 @@
>>  #include <linux/export.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> -#include <linux/io.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/gpio.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> @@ -305,14 +305,9 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy)
>>  
>>  static int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
>>  {
>> -	unsigned long timeout = 2000;
>> -	do {
>> -		if ((readl(reg) & mask) == result)
>> -			return 0;
>> -		udelay(1);
>> -		timeout--;
>> -	} while (timeout);
>> -	return -1;
>> +	u32 tmp;
>> +
>> +	return readl_poll_timeout(reg, tmp, (tmp & mask) == result, 1, 5000);
> 
> Technically I think this should be readl_poll_timeout_atomic() because
> the above used to use udelay() instead of usleep_range(). But since the
> function is never used inside atomic context, this looks fine.
> 
> You may want to bump the sleep time between reads to something like 10
> or 20. usleep_range() doesn't always work well with very short values,
> and given that you already bump the timeout from 2 ms to 5 ms indicates
> to me that we're actually spending a lot of time in this loop, and
> iterating somewhere between 2000 and 5000 times isn't any good. We only
> use this function to wait for the USB_PHY_CLK_VALID bit which happens
> during clock enable and disable, which isn't going to be very often, so
> even in the best case (where the clock is immediately valid) there's no
> need to return within a microsecond.

Thank you very much for the suggestion. Given that 2ms isn't enough, it should
be fine increase sleep time even to 1-2ms.