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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>
>> Tested-by: Caesar Wang <wxt@xxxxxxxxxxxxxx>
>> Tested-by: Ziyuan Xu <xzy.xu@xxxxxxxxxxxxxx>
>> ---
>>
>> 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
>>
>>