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

[Caesar Wang <wxt@rock-chips.com>]

在 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