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@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.

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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-rockchip