[PATCH v2 1/2] phy: rockchip-emmc: retry calpad busy trimming

[PATCH v2 1/2] phy: rockchip-emmc: retry calpad busy trimming

[Shawn Lin]

It turns out that 5us isn't enough for all cases, so let's
retry some more times to wait for caldone.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Caesar Wang <wxt@rock-chips.com>
Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com>
---

Changes in v2:
- propagate the error and print it

 drivers/phy/rockchip/phy-rockchip-emmc.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index f1b24f1..547b746 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -76,6 +76,10 @@
 #define PHYCTRL_OTAPDLYSEL_MASK		0xf
 #define PHYCTRL_OTAPDLYSEL_SHIFT	0x7
 
+#define PHYCTRL_IS_CALDONE(x) \
+	((((x) >> PHYCTRL_CALDONE_SHIFT) & \
+	  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+
 struct rockchip_emmc_phy {
 	unsigned int	reg_offset;
 	struct regmap	*reg_base;
@@ -90,6 +94,7 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
 	unsigned int freqsel = PHYCTRL_FREQSEL_200M;
 	unsigned long rate;
 	unsigned long timeout;
+	int ret;
 
 	/*
 	 * Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -160,17 +165,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
 				   PHYCTRL_PDB_SHIFT));
 
 	/*
-	 * According to the user manual, it asks driver to
-	 * wait 5us for calpad busy trimming
+	 * According to the user manual, it asks driver to wait 5us for
+	 * calpad busy trimming. However it is documented that this value is
+	 * PVT(A.K.A process,voltage and temperature) relevant, so some
+	 * failure cases are found which indicates we should be more tolerant
+	 * to calpad busy trimming.
 	 */
-	udelay(5);
-	regmap_read(rk_phy->reg_base,
-		    rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-		    &caldone);
-	caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
-	if (caldone != PHYCTRL_CALDONE_DONE) {
-		pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
-		return -ETIMEDOUT;
+	ret = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+				       caldone, PHYCTRL_IS_CALDONE(caldone),
+				       5, 50);
+	if (ret) {
+		pr_err("%s: caldone failed %d.\n", __func__, ret);
+		return ret;
 	}
 
 	/* Set the frequency of the DLL operation */
-- 
1.9.1

[PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Shawn Lin]

Just use the API instead of open-coding it, no functional change
intended.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Caesar Wang <wxt@rock-chips.com>
Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com>
---

Changes in v2:
- propagate the error and print it
- avoid using busy wait

 drivers/phy/rockchip/phy-rockchip-emmc.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 547b746..e54e78f 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -79,6 +79,9 @@
 #define PHYCTRL_IS_CALDONE(x) \
 	((((x) >> PHYCTRL_CALDONE_SHIFT) & \
 	  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+#define PHYCTRL_IS_DLLRDY(x) \
+	((((x) >> PHYCTRL_DLLRDY_SHIFT) & \
+	  PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
 
 struct rockchip_emmc_phy {
 	unsigned int	reg_offset;
@@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
 	unsigned int dllrdy;
 	unsigned int freqsel = PHYCTRL_FREQSEL_200M;
 	unsigned long rate;
-	unsigned long timeout;
 	int ret;
 
 	/*
@@ -217,28 +219,20 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
 	 * NOTE: There appear to be corner cases where the DLL seems to take
 	 * extra long to lock for reasons that aren't understood.  In some
 	 * extreme cases we've seen it take up to over 10ms (!).  We'll be
-	 * generous and give it 50ms.  We still busy wait here because:
+	 * generous and give it 50ms.  We still wait here because:
 	 * - In most cases it should be super fast.
 	 * - This is not called lots during normal operation so it shouldn't
-	 *   be a power or performance problem to busy wait.  We expect it
+	 *   be a power or performance problem to wait.  We expect it
 	 *   only at boot / resume.  In both cases, eMMC is probably on the
-	 *   critical path so busy waiting a little extra time should be OK.
+	 *   critical path so waiting a little extra time should be OK.
 	 */
-	timeout = jiffies + msecs_to_jiffies(50);
-	do {
-		udelay(1);
-
-		regmap_read(rk_phy->reg_base,
-			rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-			&dllrdy);
-		dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
-		if (dllrdy == PHYCTRL_DLLRDY_DONE)
-			break;
-	} while (!time_after(jiffies, timeout));
-
-	if (dllrdy != PHYCTRL_DLLRDY_DONE) {
-		pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
-		return -ETIMEDOUT;
+	ret = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+				       dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
+				       1, 50 * USEC_PER_MSEC);
+	if (ret) {
+		pr_err("%s: dllrdy failed %d.\n", __func__, ret);
+		return ret;
 	}
 
 	return 0;
-- 
1.9.1

Re: [PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Brian Norris]

+ Caesar

IIUC, you didn't CC him? Also, he already sent a v2 of this patchset,
withi some minor difference.

On Wed, Jan 10, 2018 at 06:49:22PM +0800, Shawn Lin wrote:
> Just use the API instead of open-coding it, no functional change
> intended.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Caesar Wang <wxt@rock-chips.com>
> Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com>
> ---
> 
> Changes in v2:
> - propagate the error and print it
> - avoid using busy wait
> 
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 547b746..e54e78f 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -79,6 +79,9 @@
>  #define PHYCTRL_IS_CALDONE(x) \
>  	((((x) >> PHYCTRL_CALDONE_SHIFT) & \
>  	  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
> +#define PHYCTRL_IS_DLLRDY(x) \
> +	((((x) >> PHYCTRL_DLLRDY_SHIFT) & \
> +	  PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
>  
>  struct rockchip_emmc_phy {
>  	unsigned int	reg_offset;
> @@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>  	unsigned int dllrdy;
>  	unsigned int freqsel = PHYCTRL_FREQSEL_200M;
>  	unsigned long rate;
> -	unsigned long timeout;
>  	int ret;
>  
>  	/*
> @@ -217,28 +219,20 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)

I'd probably like Doug's comment on the comment rewording (and
functional change) since he wrote them in the first place, but this is
also where you and Caesar differed. Caesar just deleted most of the last
paragraph, because it really applied just to the busy wait loop, not
really to the sleep-based loop that you're putting in here.

>  	 * NOTE: There appear to be corner cases where the DLL seems to take
>  	 * extra long to lock for reasons that aren't understood.  In some
>  	 * extreme cases we've seen it take up to over 10ms (!).  We'll be
> -	 * generous and give it 50ms.  We still busy wait here because:
> +	 * generous and give it 50ms.  We still wait here because:
>  	 * - In most cases it should be super fast.
>  	 * - This is not called lots during normal operation so it shouldn't
> -	 *   be a power or performance problem to busy wait.  We expect it
> +	 *   be a power or performance problem to wait.  We expect it

Why would it be a power problem to just "wait"? (Hint: it was only a
potential power problem to *busy* wait, where we're spinning in a tight
loop.)

>  	 *   only at boot / resume.  In both cases, eMMC is probably on the
> -	 *   critical path so busy waiting a little extra time should be OK.
> +	 *   critical path so waiting a little extra time should be OK.

If we all agree that the above *performance* reasoning is not important,
then it should be fine to do the conversion to the sleep/polling macro,
and I think the best comment is just to delete all the above about power
and performance of this wait loop. It was only necessary to justify the
udelay() loop.

So IOW, I think Caesar's version was better :)

Otherwise, my 'Reviewed-by' for both series stands.

Doug, do you have any thoughts? Or at least Caesar and Shawn: please
choose one of your patch series, not both!

Brian

>  	 */
> -	timeout = jiffies + msecs_to_jiffies(50);
> -	do {
> -		udelay(1);
> -
> -		regmap_read(rk_phy->reg_base,
> -			rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> -			&dllrdy);
> -		dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
> -		if (dllrdy == PHYCTRL_DLLRDY_DONE)
> -			break;
> -	} while (!time_after(jiffies, timeout));
> -
> -	if (dllrdy != PHYCTRL_DLLRDY_DONE) {
> -		pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
> -		return -ETIMEDOUT;
> +	ret = regmap_read_poll_timeout(rk_phy->reg_base,
> +				       rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> +				       dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
> +				       1, 50 * USEC_PER_MSEC);
> +	if (ret) {
> +		pr_err("%s: dllrdy failed %d.\n", __func__, ret);
> +		return ret;
>  	}
>  
>  	return 0;
> -- 
> 1.9.1
> 
> 

Re: [PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Doug Anderson]

Hi,

On Wed, Jan 10, 2018 at 9:46 AM, Brian Norris <briannorris@chromium.org> wrote:
> + Caesar
>
> IIUC, you didn't CC him? Also, he already sent a v2 of this patchset,
> withi some minor difference.
>
> On Wed, Jan 10, 2018 at 06:49:22PM +0800, Shawn Lin wrote:
>> Just use the API instead of open-coding it, no functional change
>> intended.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> Reviewed-by: Brian Norris <briannorris@chromium.org>
>> Tested-by: Caesar Wang <wxt@rock-chips.com>
>> Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - propagate the error and print it
>> - avoid using busy wait
>>
>>  drivers/phy/rockchip/phy-rockchip-emmc.c | 32 +++++++++++++-------------------
>>  1 file changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 547b746..e54e78f 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -79,6 +79,9 @@
>>  #define PHYCTRL_IS_CALDONE(x) \
>>       ((((x) >> PHYCTRL_CALDONE_SHIFT) & \
>>         PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
>> +#define PHYCTRL_IS_DLLRDY(x) \
>> +     ((((x) >> PHYCTRL_DLLRDY_SHIFT) & \
>> +       PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
>>
>>  struct rockchip_emmc_phy {
>>       unsigned int    reg_offset;
>> @@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>>       unsigned int dllrdy;
>>       unsigned int freqsel = PHYCTRL_FREQSEL_200M;
>>       unsigned long rate;
>> -     unsigned long timeout;
>>       int ret;
>>
>>       /*
>> @@ -217,28 +219,20 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>
> I'd probably like Doug's comment on the comment rewording (and
> functional change) since he wrote them in the first place, but this is
> also where you and Caesar differed. Caesar just deleted most of the last
> paragraph, because it really applied just to the busy wait loop, not
> really to the sleep-based loop that you're putting in here.
>
>>        * NOTE: There appear to be corner cases where the DLL seems to take
>>        * extra long to lock for reasons that aren't understood.  In some
>>        * extreme cases we've seen it take up to over 10ms (!).  We'll be
>> -      * generous and give it 50ms.  We still busy wait here because:
>> +      * generous and give it 50ms.  We still wait here because:
>>        * - In most cases it should be super fast.
>>        * - This is not called lots during normal operation so it shouldn't
>> -      *   be a power or performance problem to busy wait.  We expect it
>> +      *   be a power or performance problem to wait.  We expect it
>
> Why would it be a power problem to just "wait"? (Hint: it was only a
> potential power problem to *busy* wait, where we're spinning in a tight
> loop.)
>
>>        *   only at boot / resume.  In both cases, eMMC is probably on the
>> -      *   critical path so busy waiting a little extra time should be OK.
>> +      *   critical path so waiting a little extra time should be OK.
>
> If we all agree that the above *performance* reasoning is not important,
> then it should be fine to do the conversion to the sleep/polling macro,
> and I think the best comment is just to delete all the above about power
> and performance of this wait loop. It was only necessary to justify the
> udelay() loop.
>
> So IOW, I think Caesar's version was better :)

Right, I agree that Shawn's changes to this comment block don't make a
ton of sense to me.  Caesar's where he dropped much of it make more
sense to me.

> Otherwise, my 'Reviewed-by' for both series stands.
>
> Doug, do you have any thoughts? Or at least Caesar and Shawn: please
> choose one of your patch series, not both!
>
> Brian
>
>>        */
>> -     timeout = jiffies + msecs_to_jiffies(50);
>> -     do {
>> -             udelay(1);
>> -
>> -             regmap_read(rk_phy->reg_base,
>> -                     rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
>> -                     &dllrdy);
>> -             dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
>> -             if (dllrdy == PHYCTRL_DLLRDY_DONE)
>> -                     break;
>> -     } while (!time_after(jiffies, timeout));
>> -
>> -     if (dllrdy != PHYCTRL_DLLRDY_DONE) {
>> -             pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
>> -             return -ETIMEDOUT;
>> +     ret = regmap_read_poll_timeout(rk_phy->reg_base,
>> +                                    rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
>> +                                    dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
>> +                                    1, 50 * USEC_PER_MSEC);

It seems a bit schizophrenic that one of our delay loops sleeps 1 us
between loops and the other sleeps 5 us between loops.

...and, in fact, both of these numbers seem a little on the silly side
of things.  Assuming that the timer docs are up to date, usleep_range
is intended for sleeping "10us - 20ms".  Both 1 us and 5 us below that
range and "1 us" is an order of magnitude below that range.  ...your 1
and 5 actually translate to usleep_range(1, 1) and usleep_range(3, 5).

It seems like trying to do a sleep (the whole idea that some other
process will get to run for some fraction of the 1 us) is just wasting
cycles.

So I'd say either:

1. Accept that we really expect this to be a long delay and change
your delay to 10 us

2. Change the delay to 0 us and accept that you're busy waiting.

I'd vote for #2 unless you have some evidence that we often need long
delays and we've started calling this code all the time.


>> +     if (ret) {
>> +             pr_err("%s: dllrdy failed %d.\n", __func__, ret);
>> +             return ret;
>>       }
>>
>>       return 0;
>> --
>> 1.9.1
>>
>>

Re: [PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Caesar Wang]

在 2018年01月11日 03:36, Doug Anderson 写道:
> Hi,
>
> On Wed, Jan 10, 2018 at 9:46 AM, Brian Norris <briannorris@chromium.org> wrote:
>>>         */
>>> -     timeout = jiffies + msecs_to_jiffies(50);
>>> -     do {
>>> -             udelay(1);
>>> -
>>> -             regmap_read(rk_phy->reg_base,
>>> -                     rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
>>> -                     &dllrdy);
>>> -             dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
>>> -             if (dllrdy == PHYCTRL_DLLRDY_DONE)
>>> -                     break;
>>> -     } while (!time_after(jiffies, timeout));
>>> -
>>> -     if (dllrdy != PHYCTRL_DLLRDY_DONE) {
>>> -             pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
>>> -             return -ETIMEDOUT;
>>> +     ret = regmap_read_poll_timeout(rk_phy->reg_base,
>>> +                                    rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
>>> +                                    dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
>>> +                                    1, 50 * USEC_PER_MSEC);
> It seems a bit schizophrenic that one of our delay loops sleeps 1 us
> between loops and the other sleeps 5 us between loops.
>
> ...and, in fact, both of these numbers seem a little on the silly side
> of things.  Assuming that the timer docs are up to date, usleep_range
> is intended for sleeping "10us - 20ms".  Both 1 us and 5 us below that
> range and "1 us" is an order of magnitude below that range.  ...your 1
> and 5 actually translate to usleep_range(1, 1) and usleep_range(3, 5).
>
> It seems like trying to do a sleep (the whole idea that some other
> process will get to run for some fraction of the 1 us) is just wasting
> cycles.
>
> So I'd say either:
>
> 1. Accept that we really expect this to be a long delay and change
> your delay to 10 us
>
> 2. Change the delay to 0 us and accept that you're busy waiting.
>
> I'd vote for #2 unless you have some evidence that we often need long
> delays and we've started calling this code all the time.

Agreed with #2


-Caesar
>
>
>>> +     if (ret) {
>>> +             pr_err("%s: dllrdy failed %d.\n", __func__, ret);
>>> +             return ret;
>>>        }
>>>
>>>        return 0;
>>> --
>>> 1.9.1
>>>
>>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Caesar Wang]

As we communicate through QQ, Shawn had been on vacation util next week.

在 2018年01月11日 01:46, Brian Norris 写道:
> + Caesar
>
> IIUC, you didn't CC him? Also, he already sent a v2 of this patchset,
> withi some minor difference.
>
> On Wed, Jan 10, 2018 at 06:49:22PM +0800, Shawn Lin wrote:
>> Just use the API instead of open-coding it, no functional change
>> intended.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> Reviewed-by: Brian Norris <briannorris@chromium.org>
>> Tested-by: Caesar Wang <wxt@rock-chips.com>
>> Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - propagate the error and print it
>> - avoid using busy wait
>>
>>   drivers/phy/rockchip/phy-rockchip-emmc.c | 32 +++++++++++++-------------------
>>   1 file changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 547b746..e54e78f 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -79,6 +79,9 @@
>>   #define PHYCTRL_IS_CALDONE(x) \
>>   	((((x) >> PHYCTRL_CALDONE_SHIFT) & \
>>   	  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
>> +#define PHYCTRL_IS_DLLRDY(x) \
>> +	((((x) >> PHYCTRL_DLLRDY_SHIFT) & \
>> +	  PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
>>   
>>   struct rockchip_emmc_phy {
>>   	unsigned int	reg_offset;
>> @@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>>   	unsigned int dllrdy;
>>   	unsigned int freqsel = PHYCTRL_FREQSEL_200M;
>>   	unsigned long rate;
>> -	unsigned long timeout;
>>   	int ret;
>>   
>>   	/*
>> @@ -217,28 +219,20 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
> I'd probably like Doug's comment on the comment rewording (and
> functional change) since he wrote them in the first place, but this is
> also where you and Caesar differed. Caesar just deleted most of the last
> paragraph, because it really applied just to the busy wait loop, not
> really to the sleep-based loop that you're putting in here.
>
>>   	 * NOTE: There appear to be corner cases where the DLL seems to take
>>   	 * extra long to lock for reasons that aren't understood.  In some
>>   	 * extreme cases we've seen it take up to over 10ms (!).  We'll be
>> -	 * generous and give it 50ms.  We still busy wait here because:
>> +	 * generous and give it 50ms.  We still wait here because:
>>   	 * - In most cases it should be super fast.
>>   	 * - This is not called lots during normal operation so it shouldn't
>> -	 *   be a power or performance problem to busy wait.  We expect it
>> +	 *   be a power or performance problem to wait.  We expect it
> Why would it be a power problem to just "wait"? (Hint: it was only a
> potential power problem to *busy* wait, where we're spinning in a tight
> loop.)
>
>>   	 *   only at boot / resume.  In both cases, eMMC is probably on the
>> -	 *   critical path so busy waiting a little extra time should be OK.
>> +	 *   critical path so waiting a little extra time should be OK.
> If we all agree that the above *performance* reasoning is not important,
> then it should be fine to do the conversion to the sleep/polling macro,
> and I think the best comment is just to delete all the above about power
> and performance of this wait loop. It was only necessary to justify the
> udelay() loop.

Just confirmed with Shawn, we can delete the above isn't important reason.

>
> So IOW, I think Caesar's version was better :)
>
> Otherwise, my 'Reviewed-by' for both series stands.
>
> Doug, do you have any thoughts? Or at least Caesar and Shawn: please
> choose one of your patch series, not both!
>
> Brian
>
>>   	 */
>> -	timeout = jiffies + msecs_to_jiffies(50);
>> -	do {
>> -		udelay(1);
>> -
>> -		regmap_read(rk_phy->reg_base,
>> -			rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
>> -			&dllrdy);
>> -		dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
>> -		if (dllrdy == PHYCTRL_DLLRDY_DONE)
>> -			break;
>> -	} while (!time_after(jiffies, timeout));
>> -
>> -	if (dllrdy != PHYCTRL_DLLRDY_DONE) {
>> -		pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
>> -		return -ETIMEDOUT;
>> +	ret = regmap_read_poll_timeout(rk_phy->reg_base,
>> +				       rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
>> +				       dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
>> +				       1, 50 * USEC_PER_MSEC);
>> +	if (ret) {
>> +		pr_err("%s: dllrdy failed %d.\n", __func__, ret);
>> +		return ret;
>>   	}
>>   
>>   	return 0;
>> -- 
>> 1.9.1
>>
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 1/2] phy: rockchip-emmc: retry calpad busy trimming

[Doug Anderson]

Hi,

This seems like a good idea to me.  The fact that there was no polling
loop here always seemed strange to me, but that's how the original
code was structured and I personally never saw any problems with it.

On Wed, Jan 10, 2018 at 2:49 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> It turns out that 5us isn't enough for all cases, so let's
> retry some more times to wait for caldone.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Caesar Wang <wxt@rock-chips.com>
> Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com>
> ---
>
> Changes in v2:
> - propagate the error and print it
>
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index f1b24f1..547b746 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -76,6 +76,10 @@
>  #define PHYCTRL_OTAPDLYSEL_MASK                0xf
>  #define PHYCTRL_OTAPDLYSEL_SHIFT       0x7
>
> +#define PHYCTRL_IS_CALDONE(x) \
> +       ((((x) >> PHYCTRL_CALDONE_SHIFT) & \
> +         PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
> +
>  struct rockchip_emmc_phy {
>         unsigned int    reg_offset;
>         struct regmap   *reg_base;
> @@ -90,6 +94,7 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>         unsigned int freqsel = PHYCTRL_FREQSEL_200M;
>         unsigned long rate;
>         unsigned long timeout;
> +       int ret;
>
>         /*
>          * Keep phyctrl_pdb and phyctrl_endll low to allow
> @@ -160,17 +165,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>                                    PHYCTRL_PDB_SHIFT));
>
>         /*
> -        * According to the user manual, it asks driver to
> -        * wait 5us for calpad busy trimming
> +        * According to the user manual, it asks driver to wait 5us for
> +        * calpad busy trimming. However it is documented that this value is
> +        * PVT(A.K.A process,voltage and temperature) relevant, so some
> +        * failure cases are found which indicates we should be more tolerant
> +        * to calpad busy trimming.
>          */
> -       udelay(5);
> -       regmap_read(rk_phy->reg_base,
> -                   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> -                   &caldone);
> -       caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
> -       if (caldone != PHYCTRL_CALDONE_DONE) {
> -               pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
> -               return -ETIMEDOUT;
> +       ret = regmap_read_poll_timeout(rk_phy->reg_base,
> +                                      rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> +                                      caldone, PHYCTRL_IS_CALDONE(caldone),
> +                                      5, 50);

See comments in part 2 of this series, AKA
<https://patchwork.kernel.org/patch/10154797/>.  I think this should
be "0, 50", not "5, 50".  ...or, if you insist, "10, 50"


> +       if (ret) {
> +               pr_err("%s: caldone failed %d.\n", __func__, ret);

I like this v2 slightly better than Caesar's v2 because you changed
the word "timeout" to "failed".

[PATCH v2 0/2] phy: rockchip-emmc: fixes emmc-phy power on failed with rk3399 SoCs

[Caesar Wang]

Hi Kishon,

Since the Shawn isn't available, I take over this series patches for now.

As the original bug had tracked on https://issuetracker.google.com/71561742.
In some cases, the mmc phy power on failed during booting up.
The log as below:
...
[   2.375333] rockchip_emmc_phy_power: caldone timeout.
[    2.377815] phy phy-ff770000.syscon:phy@f780.4: phy poweron failed --> -110
...
[    2.489295] mmc0: mmc_select_hs400es failed, error -110
[    2.489302] mmc0: error -110 whilst initialising MMC card
..

The actual emulate, the wait 5us for calpad busy trimming, that's no enough.
We need give the enough margin for it.

Verified on url =
        https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4
This series patches can apply and bring up with kernel-next on rk3399 chromebook.

-Caesar


Changes in v2:
- print the return valut with regmap_read_poll_timeout failing.
- As Brian commented on https://patchwork.kernel.org/patch/10139891/,
  changed the note and added to print error value with
  regmap_read_poll_timeout API.

Shawn Lin (2):
  phy: rockchip-emmc: retry calpad busy trimming
  phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

 drivers/phy/rockchip/phy-rockchip-emmc.c | 60 +++++++++++++++-----------------
 1 file changed, 28 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Caesar Wang]

From: Shawn Lin <shawn.lin@rock-chips.com>

Just use the API instead of open-coding it, no functional change
intended.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>

---

Changes in v2:
- As Brian commented on https://patchwork.kernel.org/patch/10139891/,
  changed the note and added to print error value with
  regmap_read_poll_timeout API.

 drivers/phy/rockchip/phy-rockchip-emmc.c | 33 +++++++++++---------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 574838f..343c623 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -79,6 +79,9 @@
 #define PHYCTRL_IS_CALDONE(x) \
 	((((x) >> PHYCTRL_CALDONE_SHIFT) & \
 	  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+#define PHYCTRL_IS_DLLRDY(x) \
+	((((x) >> PHYCTRL_DLLRDY_SHIFT) & \
+	  PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
 
 struct rockchip_emmc_phy {
 	unsigned int	reg_offset;
@@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
 	unsigned int dllrdy;
 	unsigned int freqsel = PHYCTRL_FREQSEL_200M;
 	unsigned long rate;
-	unsigned long timeout;
 	int ret;
 
 	/*
@@ -217,28 +219,15 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
 	 * NOTE: There appear to be corner cases where the DLL seems to take
 	 * extra long to lock for reasons that aren't understood.  In some
 	 * extreme cases we've seen it take up to over 10ms (!).  We'll be
-	 * generous and give it 50ms.  We still busy wait here because:
-	 * - In most cases it should be super fast.
-	 * - This is not called lots during normal operation so it shouldn't
-	 *   be a power or performance problem to busy wait.  We expect it
-	 *   only at boot / resume.  In both cases, eMMC is probably on the
-	 *   critical path so busy waiting a little extra time should be OK.
+	 * generous and give it 50ms.
 	 */
-	timeout = jiffies + msecs_to_jiffies(50);
-	do {
-		udelay(1);
-
-		regmap_read(rk_phy->reg_base,
-			rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-			&dllrdy);
-		dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
-		if (dllrdy == PHYCTRL_DLLRDY_DONE)
-			break;
-	} while (!time_after(jiffies, timeout));
-
-	if (dllrdy != PHYCTRL_DLLRDY_DONE) {
-		pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
-		return -ETIMEDOUT;
+	ret = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+				       dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
+				       1, 50 * USEC_PER_MSEC);
+	if (ret) {
+		pr_err("%s: dllrdy timeout. ret=%d\n", __func__, ret);
+		return ret;
 	}
 
 	return 0;
-- 
2.7.4

Re: [PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Doug Anderson]

Hi,

On Tue, Jan 9, 2018 at 11:37 PM, Caesar Wang <wxt@rock-chips.com> wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
>
> Just use the API instead of open-coding it, no functional change
> intended.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>
> ---
>
> Changes in v2:
> - As Brian commented on https://patchwork.kernel.org/patch/10139891/,
>   changed the note and added to print error value with
>   regmap_read_poll_timeout API.
>
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 33 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)

See comments in Shawn's v2.  AKA: <https://patchwork.kernel.org/patch/10154797/>