| Message ID | 1515581362-181200-2-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+ 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@rock-chips.com> > Reviewed-by: Brian Norris <briannorris@chromium.org> > Tested-by: Caesar Wang <wxt@rock-chips.com> > Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com> > --- > > 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 <briannorris@chromium.org> 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@rock-chips.com> >> Reviewed-by: Brian Norris <briannorris@chromium.org> >> Tested-by: Caesar Wang <wxt@rock-chips.com> >> Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com> >> --- >> >> 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 >> >>
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@rock-chips.com> >> Reviewed-by: Brian Norris <briannorris@chromium.org> >> Tested-by: Caesar Wang <wxt@rock-chips.com> >> Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com> >> --- >> >> 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@lists.infradead.org > 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 <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
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;