| Message ID | 20171114225520.6793-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wolfram, On Tue, Nov 14, 2017 at 11:55 PM, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Documentation/timers/timers-howto.txt recommends to use usleep_range for > delays 1-20ms. Let's adhere to it. No need for messing with HZ and still > do busy looping these days. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Here is a more detailed test page for this describing my tests: > > https://elinux.org/Tests:mmc-delay-refactor > > I did mainly test the insert/eject cycle because powering up the cards > seemed to trigger most delays. Transferring data did not cause any calls > to mmc_delay() for me. Please let me know if someone knows a test pattern > which should be included before applying this change. JFTR, with CONFIG_DEBUG_ATOMIC_SLEEP=y, I assume ? (also for the other patch) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > I did mainly test the insert/eject cycle because powering up the cards > > seemed to trigger most delays. Transferring data did not cause any calls > > to mmc_delay() for me. Please let me know if someone knows a test pattern > > which should be included before applying this change. > > JFTR, with CONFIG_DEBUG_ATOMIC_SLEEP=y, I assume ? > (also for the other patch) Actually, no :( Did this now and retested, nothing showed up luckily (would have been really a surprise, too). Also, added those config options to the configs in the test branch. Thanks for the pointer, Geert!
On 14 November 2017 at 23:55, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Documentation/timers/timers-howto.txt recommends to use usleep_range for > delays 1-20ms. Let's adhere to it. No need for messing with HZ and still > do busy looping these days. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Here is a more detailed test page for this describing my tests: > > https://elinux.org/Tests:mmc-delay-refactor > > I did mainly test the insert/eject cycle because powering up the cards > seemed to trigger most delays. Transferring data did not cause any calls > to mmc_delay() for me. Please let me know if someone knows a test pattern > which should be included before applying this change. > > Works fine for me on Lager (R-Car H2) and Salvator-X (R-Car M3-W). Testing on > other platforms very welcome. This should be independent of the IP core. > > drivers/mmc/core/core.h | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h > index 71e6c6d7ceb70d..b2877e2d740fa5 100644 > --- a/drivers/mmc/core/core.h > +++ b/drivers/mmc/core/core.h > @@ -62,12 +62,10 @@ void mmc_set_initial_state(struct mmc_host *host); > > static inline void mmc_delay(unsigned int ms) > { > - if (ms < 1000 / HZ) { > - cond_resched(); > - mdelay(ms); > - } else { > + if (ms <= 20) > + usleep_range(ms * 1000, ms * 1250); This means we get usleep_range(20000, 25000) for the worst case. Perhaps we want "ms <= 16" instead, thus getting usleep usleep_range(16000, 20000) for the worst case? No? > + else > msleep(ms); > - } > } Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 2017/11/16 15:47, Ulf Hansson wrote: > On 14 November 2017 at 23:55, Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: >> Documentation/timers/timers-howto.txt recommends to use usleep_range for >> delays 1-20ms. Let's adhere to it. No need for messing with HZ and still >> do busy looping these days. >> Sorry for chime in this topic but I think for some cases, for instance, mmc_host_set_uhs_voltage calls mmc_delay and it should guarantee the delay is accurate, especially for the comment"Keep clock gated for at least 10 ms, though spec only says 5 ms" But the usleep_range won't guarantee that, see https://patchwork.kernel.org/patch/9369963/ It wasn't fixed at the end, so you should carefully modify this fundamentally. >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> --- >> >> Here is a more detailed test page for this describing my tests: >> >> https://elinux.org/Tests:mmc-delay-refactor >> >> I did mainly test the insert/eject cycle because powering up the cards >> seemed to trigger most delays. Transferring data did not cause any calls >> to mmc_delay() for me. Please let me know if someone knows a test pattern >> which should be included before applying this change. >> >> Works fine for me on Lager (R-Car H2) and Salvator-X (R-Car M3-W). Testing on >> other platforms very welcome. This should be independent of the IP core. >> >> drivers/mmc/core/core.h | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h >> index 71e6c6d7ceb70d..b2877e2d740fa5 100644 >> --- a/drivers/mmc/core/core.h >> +++ b/drivers/mmc/core/core.h >> @@ -62,12 +62,10 @@ void mmc_set_initial_state(struct mmc_host *host); >> >> static inline void mmc_delay(unsigned int ms) >> { >> - if (ms < 1000 / HZ) { >> - cond_resched(); >> - mdelay(ms); >> - } else { >> + if (ms <= 20) >> + usleep_range(ms * 1000, ms * 1250); > > This means we get usleep_range(20000, 25000) for the worst case. > > Perhaps we want "ms <= 16" instead, thus getting usleep > usleep_range(16000, 20000) for the worst case? No? > >> + else >> msleep(ms); >> - } >> } > > Kind regards > Uffe > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 November 2017 at 08:59, Shawn Lin <shawn.lin@rock-chips.com> wrote: > Hi, > > On 2017/11/16 15:47, Ulf Hansson wrote: >> >> On 14 November 2017 at 23:55, Wolfram Sang >> <wsa+renesas@sang-engineering.com> wrote: >>> >>> Documentation/timers/timers-howto.txt recommends to use usleep_range for >>> delays 1-20ms. Let's adhere to it. No need for messing with HZ and still >>> do busy looping these days. >>> > > Sorry for chime in this topic but I think for some cases, for instance, > mmc_host_set_uhs_voltage calls mmc_delay and it should guarantee the > delay is accurate, especially for the comment"Keep clock gated for at > least 10 ms, though spec only says 5 ms" > > But the usleep_range won't guarantee that, see > > https://patchwork.kernel.org/patch/9369963/ > > It wasn't fixed at the end, so you should carefully modify > this fundamentally. git tag --contains 6c5e9059692567740a4ee51530dffe51a4b9584d It should be fixed, however, thanks for pointing to an interesting patch. :-) [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 2017/11/16 18:42, Ulf Hansson wrote: > On 16 November 2017 at 08:59, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> Hi, >> >> On 2017/11/16 15:47, Ulf Hansson wrote: >>> >>> On 14 November 2017 at 23:55, Wolfram Sang >>> <wsa+renesas@sang-engineering.com> wrote: >>>> >>>> Documentation/timers/timers-howto.txt recommends to use usleep_range for >>>> delays 1-20ms. Let's adhere to it. No need for messing with HZ and still >>>> do busy looping these days. >>>> >> >> Sorry for chime in this topic but I think for some cases, for instance, >> mmc_host_set_uhs_voltage calls mmc_delay and it should guarantee the >> delay is accurate, especially for the comment"Keep clock gated for at >> least 10 ms, though spec only says 5 ms" >> >> But the usleep_range won't guarantee that, see >> >> https://patchwork.kernel.org/patch/9369963/ >> >> It wasn't fixed at the end, so you should carefully modify >> this fundamentally. > > git tag --contains 6c5e9059692567740a4ee51530dffe51a4b9584d > > It should be fixed, however, thanks for pointing to an interesting patch. :-) Ah, I wasn't continuing tracing that topic and from the argue of v2, I thought it wasn't fixed... So it seems fine now. > > [...] > > Kind regards > Uffe > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> This means we get usleep_range(20000, 25000) for the worst case. > > Perhaps we want "ms <= 16" instead, thus getting usleep > usleep_range(16000, 20000) for the worst case? No? I read the timers-howto that ms <= 20 is for the min value. If you think 25ms is too much for max in that case, maybe we can find a better formula for max? My honest opinion is, it won't matter much in practice, though. Especially, since we are not busy-looping anymore...
On 14 November 2017 at 23:55, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Documentation/timers/timers-howto.txt recommends to use usleep_range for > delays 1-20ms. Let's adhere to it. No need for messing with HZ and still > do busy looping these days. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks, applied for next! Kind regards Uffe > --- > > Here is a more detailed test page for this describing my tests: > > https://elinux.org/Tests:mmc-delay-refactor > > I did mainly test the insert/eject cycle because powering up the cards > seemed to trigger most delays. Transferring data did not cause any calls > to mmc_delay() for me. Please let me know if someone knows a test pattern > which should be included before applying this change. > > Works fine for me on Lager (R-Car H2) and Salvator-X (R-Car M3-W). Testing on > other platforms very welcome. This should be independent of the IP core. > > drivers/mmc/core/core.h | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h > index 71e6c6d7ceb70d..b2877e2d740fa5 100644 > --- a/drivers/mmc/core/core.h > +++ b/drivers/mmc/core/core.h > @@ -62,12 +62,10 @@ void mmc_set_initial_state(struct mmc_host *host); > > static inline void mmc_delay(unsigned int ms) > { > - if (ms < 1000 / HZ) { > - cond_resched(); > - mdelay(ms); > - } else { > + if (ms <= 20) > + usleep_range(ms * 1000, ms * 1250); > + else > msleep(ms); > - } > } > > void mmc_rescan(struct work_struct *work); > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index 71e6c6d7ceb70d..b2877e2d740fa5 100644 --- a/drivers/mmc/core/core.h +++ b/drivers/mmc/core/core.h @@ -62,12 +62,10 @@ void mmc_set_initial_state(struct mmc_host *host); static inline void mmc_delay(unsigned int ms) { - if (ms < 1000 / HZ) { - cond_resched(); - mdelay(ms); - } else { + if (ms <= 20) + usleep_range(ms * 1000, ms * 1250); + else msleep(ms); - } } void mmc_rescan(struct work_struct *work);
Documentation/timers/timers-howto.txt recommends to use usleep_range for delays 1-20ms. Let's adhere to it. No need for messing with HZ and still do busy looping these days. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Here is a more detailed test page for this describing my tests: https://elinux.org/Tests:mmc-delay-refactor I did mainly test the insert/eject cycle because powering up the cards seemed to trigger most delays. Transferring data did not cause any calls to mmc_delay() for me. Please let me know if someone knows a test pattern which should be included before applying this change. Works fine for me on Lager (R-Car H2) and Salvator-X (R-Car M3-W). Testing on other platforms very welcome. This should be independent of the IP core. drivers/mmc/core/core.h | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)