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 <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
Tested-by: Caesar Wang <[email protected]>
Tested-by: Ziyuan Xu <[email protected]>
---
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
Just use the API instead of open-coding it, no functional change
intended.
Signed-off-by: Shawn Lin <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
Tested-by: Caesar Wang <[email protected]>
Tested-by: Ziyuan Xu <[email protected]>
---
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
+ 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 <[email protected]>
> Reviewed-by: Brian Norris <[email protected]>
> Tested-by: Caesar Wang <[email protected]>
> Tested-by: Ziyuan Xu <[email protected]>
> ---
>
> 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
>
>
Hi,
On Wed, Jan 10, 2018 at 9:46 AM, Brian Norris <[email protected]> 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 <[email protected]>
>> Reviewed-by: Brian Norris <[email protected]>
>> Tested-by: Caesar Wang <[email protected]>
>> Tested-by: Ziyuan Xu <[email protected]>
>> ---
>>
>> 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
>>
>>
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 <[email protected]> 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 <[email protected]>
> Reviewed-by: Brian Norris <[email protected]>
> Tested-by: Caesar Wang <[email protected]>
> Tested-by: Ziyuan Xu <[email protected]>
> ---
>
> 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".
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 <[email protected]>
>> Reviewed-by: Brian Norris <[email protected]>
>> Tested-by: Caesar Wang <[email protected]>
>> Tested-by: Ziyuan Xu <[email protected]>
>> ---
>>
>> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
?? 2018??01??11?? 03:36, Doug Anderson д??:
> Hi,
>
> On Wed, Jan 10, 2018 at 9:46 AM, Brian Norris <[email protected]> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip