Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754054AbeAJTgJ (ORCPT + 1 other); Wed, 10 Jan 2018 14:36:09 -0500 Received: from mail-ua0-f194.google.com ([209.85.217.194]:39534 "EHLO mail-ua0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753731AbeAJTgE (ORCPT ); Wed, 10 Jan 2018 14:36:04 -0500 X-Google-Smtp-Source: ACJfBovuEgzwNKTkm8KZHdlY7ZuGB9IlVWgyNCjeTAs30lnq0XP1Acx4MKGOs5Dy0MtgbeLRXdSVvzx48n0kXiJOV/4= MIME-Version: 1.0 In-Reply-To: <20180110174615.GA30753@google.com> References: <1515581362-181200-1-git-send-email-shawn.lin@rock-chips.com> <1515581362-181200-2-git-send-email-shawn.lin@rock-chips.com> <20180110174615.GA30753@google.com> From: Doug Anderson Date: Wed, 10 Jan 2018 11:36:01 -0800 X-Google-Sender-Auth: 24CArZ8TMBUrd4o6x29XRPiaJ9g Message-ID: Subject: Re: [PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy To: Brian Norris Cc: Shawn Lin , Kishon Vijay Abraham I , "open list:ARM/Rockchip SoC..." , Heiko Stuebner , Ziyuan Xu , LKML , Caesar Wang Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi, On Wed, Jan 10, 2018 at 9:46 AM, Brian Norris 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 >> Reviewed-by: Brian Norris >> Tested-by: Caesar Wang >> Tested-by: Ziyuan Xu >> --- >> >> 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 >> >>