| Message ID | 20191015204341.23613-1-marek.vasut+renesas@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] lib: time: Add microsecond timer | expand |
Hi Marek, On Tue, 15 Oct 2019 at 14:43, Marek Vasut <marek.vasut@gmail.com> wrote: > > Add get_timer_us(), which is useful e.g. when we need higher > precision timestamps. Can we use timer_get_us()? It seems confusing to have two. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > --- > include/time.h | 1 + > lib/time.c | 14 ++++++++++++++ > 2 files changed, 15 insertions(+) Regards, Simon
On 10/16/19 3:30 AM, Simon Glass wrote: > Hi Marek, Hi, > On Tue, 15 Oct 2019 at 14:43, Marek Vasut wrote: >> >> Add get_timer_us(), which is useful e.g. when we need higher >> precision timestamps. > > Can we use timer_get_us()? It seems confusing to have two. Nope, that one doesn't have the range (unsigned long vs. u64) and also doesn't behave the same way as old get_timer(). I wanted something which is similar, just for uS instead of mS.
Hi Marek, On Wed, 16 Oct 2019 at 02:55, Marek Vasut <marek.vasut@gmail.com> wrote: > > On 10/16/19 3:30 AM, Simon Glass wrote: > > Hi Marek, > > Hi, > > > On Tue, 15 Oct 2019 at 14:43, Marek Vasut wrote: > >> > >> Add get_timer_us(), which is useful e.g. when we need higher > >> precision timestamps. > > > > Can we use timer_get_us()? It seems confusing to have two. > > Nope, that one doesn't have the range (unsigned long vs. u64) and also > doesn't behave the same way as old get_timer(). I wanted something which > is similar, just for uS instead of mS. Can you add comments to your patch to indicate what is going on any why to use this? Bootstage uses ulong which is enough for about an hour. How long is U-Boot running? If you are using differential times, presumably for timeouts, then there seems to be little reason to need u64. Regards, Simon
On 10/16/19 6:40 PM, Simon Glass wrote: > Hi Marek, Hi, > On Wed, 16 Oct 2019 at 02:55, Marek Vasut wrote: >> >> On 10/16/19 3:30 AM, Simon Glass wrote: >>> Hi Marek, >> >> Hi, >> >>> On Tue, 15 Oct 2019 at 14:43, Marek Vasut wrote: >>>> >>>> Add get_timer_us(), which is useful e.g. when we need higher >>>> precision timestamps. >>> >>> Can we use timer_get_us()? It seems confusing to have two. >> >> Nope, that one doesn't have the range (unsigned long vs. u64) and also >> doesn't behave the same way as old get_timer(). I wanted something which >> is similar, just for uS instead of mS. > > Can you add comments to your patch to indicate what is going on any > why to use this? > > Bootstage uses ulong which is enough for about an hour. How long is > U-Boot running? It can run as long as anyone needs. > If you are using differential times, presumably for timeouts, then > there seems to be little reason to need u64. I use it for logging timestamps during profiling, e.g. of the EHCI driver.
Hi Marek, On Wed, 16 Oct 2019 at 10:44, Marek Vasut <marek.vasut@gmail.com> wrote: > > On 10/16/19 6:40 PM, Simon Glass wrote: > > Hi Marek, > > Hi, > > > On Wed, 16 Oct 2019 at 02:55, Marek Vasut wrote: > >> > >> On 10/16/19 3:30 AM, Simon Glass wrote: > >>> Hi Marek, > >> > >> Hi, > >> > >>> On Tue, 15 Oct 2019 at 14:43, Marek Vasut wrote: > >>>> > >>>> Add get_timer_us(), which is useful e.g. when we need higher > >>>> precision timestamps. > >>> > >>> Can we use timer_get_us()? It seems confusing to have two. > >> > >> Nope, that one doesn't have the range (unsigned long vs. u64) and also > >> doesn't behave the same way as old get_timer(). I wanted something which > >> is similar, just for uS instead of mS. > > > > Can you add comments to your patch to indicate what is going on any > > why to use this? > > > > Bootstage uses ulong which is enough for about an hour. How long is > > U-Boot running? > > It can run as long as anyone needs. > > > If you are using differential times, presumably for timeouts, then > > there seems to be little reason to need u64. > > I use it for logging timestamps during profiling, e.g. of the EHCI driver. Have you tried bootstage? Regards, Simon
On 10/16/19 6:54 PM, Simon Glass wrote: > Hi Marek, Hello Simon, > On Wed, 16 Oct 2019 at 10:44, Marek Vasut wrote: >> >> On 10/16/19 6:40 PM, Simon Glass wrote: >>> Hi Marek, >> >> Hi, >> >>> On Wed, 16 Oct 2019 at 02:55, Marek Vasut wrote: >>>> >>>> On 10/16/19 3:30 AM, Simon Glass wrote: >>>>> Hi Marek, >>>> >>>> Hi, >>>> >>>>> On Tue, 15 Oct 2019 at 14:43, Marek Vasut wrote: >>>>>> >>>>>> Add get_timer_us(), which is useful e.g. when we need higher >>>>>> precision timestamps. >>>>> >>>>> Can we use timer_get_us()? It seems confusing to have two. >>>> >>>> Nope, that one doesn't have the range (unsigned long vs. u64) and also >>>> doesn't behave the same way as old get_timer(). I wanted something which >>>> is similar, just for uS instead of mS. >>> >>> Can you add comments to your patch to indicate what is going on any >>> why to use this? >>> >>> Bootstage uses ulong which is enough for about an hour. How long is >>> U-Boot running? >> >> It can run as long as anyone needs. >> >>> If you are using differential times, presumably for timeouts, then >>> there seems to be little reason to need u64. >> >> I use it for logging timestamps during profiling, e.g. of the EHCI driver. > > Have you tried bootstage? What for ? I need to debug the EHCI driver performance across the EHCI driver and USB storage implementation. Bootstage is not helpful here.
Hi Marek, On Wed, 16 Oct 2019 at 10:55, Marek Vasut <marek.vasut@gmail.com> wrote: > > On 10/16/19 6:54 PM, Simon Glass wrote: > > Hi Marek, > > Hello Simon, > > > On Wed, 16 Oct 2019 at 10:44, Marek Vasut wrote: > >> > >> On 10/16/19 6:40 PM, Simon Glass wrote: > >>> Hi Marek, > >> > >> Hi, > >> > >>> On Wed, 16 Oct 2019 at 02:55, Marek Vasut wrote: > >>>> > >>>> On 10/16/19 3:30 AM, Simon Glass wrote: > >>>>> Hi Marek, > >>>> > >>>> Hi, > >>>> > >>>>> On Tue, 15 Oct 2019 at 14:43, Marek Vasut wrote: > >>>>>> > >>>>>> Add get_timer_us(), which is useful e.g. when we need higher > >>>>>> precision timestamps. > >>>>> > >>>>> Can we use timer_get_us()? It seems confusing to have two. > >>>> > >>>> Nope, that one doesn't have the range (unsigned long vs. u64) and also > >>>> doesn't behave the same way as old get_timer(). I wanted something which > >>>> is similar, just for uS instead of mS. > >>> > >>> Can you add comments to your patch to indicate what is going on any > >>> why to use this? > >>> > >>> Bootstage uses ulong which is enough for about an hour. How long is > >>> U-Boot running? > >> > >> It can run as long as anyone needs. > >> > >>> If you are using differential times, presumably for timeouts, then > >>> there seems to be little reason to need u64. > >> > >> I use it for logging timestamps during profiling, e.g. of the EHCI driver. > > > > Have you tried bootstage? > > What for ? I need to debug the EHCI driver performance across the EHCI > driver and USB storage implementation. Bootstage is not helpful here. It can track the time spent in particular parts of the code, accumulating it over multiple calls, etc. E.g. how much is FS, how much is driver, how much is waiting for replies... Regards, Simon
On 10/16/19 6:58 PM, Simon Glass wrote: [...] >>> Have you tried bootstage? >> >> What for ? I need to debug the EHCI driver performance across the EHCI >> driver and USB storage implementation. Bootstage is not helpful here. > > It can track the time spent in particular parts of the code, > accumulating it over multiple calls, etc. E.g. how much is FS, how > much is driver, how much is waiting for replies... And how much overhead does it add compared to calling get_timer{,_us}() directly ? It was about uSecs in this case, so this was the best approach.
On Tue, Oct 15, 2019 at 10:43:41PM +0200, Marek Vasut wrote: > Add get_timer_us(), which is useful e.g. when we need higher > precision timestamps. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> FWIW, I agree with Simon that bootstage [1] can be an awesome tool for profiling and boot time measurements. With a bit of instrumentation and host-side scripting, it allows to produce accurate bootcharts like [2]. [1] https://patchwork.ozlabs.org/patch/1177393/#2281091 [2] https://i.ibb.co/mG6Xc1p/2019-10-16-190251.png
On 10/16/19 7:11 PM, Eugeniu Rosca wrote: > On Tue, Oct 15, 2019 at 10:43:41PM +0200, Marek Vasut wrote: >> Add get_timer_us(), which is useful e.g. when we need higher >> precision timestamps. > > FWIW, I agree with Simon that bootstage [1] can be an awesome tool for > profiling and boot time measurements. With a bit of instrumentation and > host-side scripting, it allows to produce accurate bootcharts like [2]. > > [1] https://patchwork.ozlabs.org/patch/1177393/#2281091 > [2] https://i.ibb.co/mG6Xc1p/2019-10-16-190251.png I don't need that though, I really only need to know how long the code spent between two points in code.
On Wed, Oct 16, 2019 at 07:26:44PM +0200, Marek Vasut wrote: > On 10/16/19 7:11 PM, Eugeniu Rosca wrote: > > On Tue, Oct 15, 2019 at 10:43:41PM +0200, Marek Vasut wrote: > >> Add get_timer_us(), which is useful e.g. when we need higher > >> precision timestamps. > > > > FWIW, I agree with Simon that bootstage [1] can be an awesome tool for > > profiling and boot time measurements. With a bit of instrumentation and > > host-side scripting, it allows to produce accurate bootcharts like [2]. > > > > [1] https://patchwork.ozlabs.org/patch/1177393/#2281091 > > [2] https://i.ibb.co/mG6Xc1p/2019-10-16-190251.png > > I don't need that though, I really only need to know how long the code > spent between two points in code. It can be accomplished in N ways. A quick and dirty way to use "bootstage" would be to add below instrumentation: ----------8<---------- bootstage_mark_name(BOOTSTAGE_ID_ALLOC, "count/time from here"); /* * my-precious-code */ bootstage_mark_name(BOOTSTAGE_ID_ALLOC, "duration of my-precious-code"); ----------8<---------- It's likely orthogonal to what's being proposed in your patch.
On Wed, Oct 16, 2019 at 07:43:09PM +0200, Eugeniu Rosca wrote: > On Wed, Oct 16, 2019 at 07:26:44PM +0200, Marek Vasut wrote: > > On 10/16/19 7:11 PM, Eugeniu Rosca wrote: > > > On Tue, Oct 15, 2019 at 10:43:41PM +0200, Marek Vasut wrote: > > >> Add get_timer_us(), which is useful e.g. when we need higher > > >> precision timestamps. > > > > > > FWIW, I agree with Simon that bootstage [1] can be an awesome tool for > > > profiling and boot time measurements. With a bit of instrumentation and > > > host-side scripting, it allows to produce accurate bootcharts like [2]. > > > > > > [1] https://patchwork.ozlabs.org/patch/1177393/#2281091 > > > [2] https://i.ibb.co/mG6Xc1p/2019-10-16-190251.png > > > > I don't need that though, I really only need to know how long the code > > spent between two points in code. > > It can be accomplished in N ways. A quick and dirty way to use > "bootstage" would be to add below instrumentation: > > ----------8<---------- > bootstage_mark_name(BOOTSTAGE_ID_ALLOC, "count/time from here"); > /* > * my-precious-code > */ > bootstage_mark_name(BOOTSTAGE_ID_ALLOC, "duration of my-precious-code"); > ----------8<---------- > > It's likely orthogonal to what's being proposed in your patch. Bottom line is "bootstage" already makes use of timer_get_boot_us(), so it's not entirely clear to me why another us-resolution timer API would be needed.
On 10/16/19 7:51 PM, Eugeniu Rosca wrote: > On Wed, Oct 16, 2019 at 07:43:09PM +0200, Eugeniu Rosca wrote: >> On Wed, Oct 16, 2019 at 07:26:44PM +0200, Marek Vasut wrote: >>> On 10/16/19 7:11 PM, Eugeniu Rosca wrote: >>>> On Tue, Oct 15, 2019 at 10:43:41PM +0200, Marek Vasut wrote: >>>>> Add get_timer_us(), which is useful e.g. when we need higher >>>>> precision timestamps. >>>> >>>> FWIW, I agree with Simon that bootstage [1] can be an awesome tool for >>>> profiling and boot time measurements. With a bit of instrumentation and >>>> host-side scripting, it allows to produce accurate bootcharts like [2]. >>>> >>>> [1] https://patchwork.ozlabs.org/patch/1177393/#2281091 >>>> [2] https://i.ibb.co/mG6Xc1p/2019-10-16-190251.png >>> >>> I don't need that though, I really only need to know how long the code >>> spent between two points in code. >> >> It can be accomplished in N ways. A quick and dirty way to use >> "bootstage" would be to add below instrumentation: >> >> ----------8<---------- >> bootstage_mark_name(BOOTSTAGE_ID_ALLOC, "count/time from here"); >> /* >> * my-precious-code >> */ >> bootstage_mark_name(BOOTSTAGE_ID_ALLOC, "duration of my-precious-code"); >> ----------8<---------- >> >> It's likely orthogonal to what's being proposed in your patch. > > Bottom line is "bootstage" already makes use of timer_get_boot_us(), so > it's not entirely clear to me why another us-resolution timer API would > be needed. Because the new API is actually aligned with the old one. I don't need the machinery behind the bootstage either.
On Tue, Oct 15, 2019 at 10:43:41PM +0200, Marek Vasut wrote: > Add get_timer_us(), which is useful e.g. when we need higher > precision timestamps. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
diff --git a/include/time.h b/include/time.h index 1e9b369be7..a1149522ed 100644 --- a/include/time.h +++ b/include/time.h @@ -13,6 +13,7 @@ unsigned long get_timer(unsigned long base); * Granularity may be larger than 1us if hardware does not support this. */ unsigned long timer_get_us(void); +uint64_t get_timer_us(uint64_t base); /* * timer_test_add_offset() diff --git a/lib/time.c b/lib/time.c index f5751ab162..f30fc05804 100644 --- a/lib/time.c +++ b/lib/time.c @@ -134,6 +134,20 @@ ulong __weak get_timer(ulong base) return tick_to_time(get_ticks()) - base; } +static uint64_t notrace tick_to_time_us(uint64_t tick) +{ + ulong div = get_tbclk() / 1000; + + tick *= CONFIG_SYS_HZ; + do_div(tick, div); + return tick; +} + +uint64_t __weak get_timer_us(uint64_t base) +{ + return tick_to_time_us(get_ticks()) - base; +} + unsigned long __weak notrace timer_get_us(void) { return tick_to_time(get_ticks() * 1000);
Add get_timer_us(), which is useful e.g. when we need higher precision timestamps. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Tom Rini <trini@konsulko.com> Cc: Simon Glass <sjg@chromium.org> --- include/time.h | 1 + lib/time.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)