| Message ID | 20220830115317.410812-3-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | Enable CONFIG_TIMER for all Kirwood / MVEBU boards | expand |
Am 2022-08-30 13:53, schrieb Stefan Roese: > Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE > enabled, like pogo_v4. > > Signed-off-by: Stefan Roese <sr@denx.de> > --- > drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c > index 02ed138642b8..7e920eaeaa40 100644 > --- a/drivers/timer/orion-timer.c > +++ b/drivers/timer/orion-timer.c > @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) > } > #endif > > +#if CONFIG_IS_ENABLED(BOOTSTAGE) > +ulong timer_get_boot_us(void) > +{ > + u64 ticks = 0; > + u32 rate = 1; > + u64 us; > + int ret; > + > + ret = dm_timer_init(); > + if (!ret) { > + /* The timer is available */ > + rate = timer_get_rate(gd->timer); > + timer_get_count(gd->timer, &ticks); > + } else { > + return 0; > + } > + > + us = (ticks * 1000) / rate; > + return us; > +} > +#endif This is duplicate code in almost all the timer drivers, shouldn't this be a (weak) default implementation in timer-uclass.c? Also, do you need to guard it with CONFIG_IS_ENABLED(BOOTSTAGE), aren't unused functions discarded anyway? -michael
Adding Simon to Cc... On 30.08.22 14:00, Michael Walle wrote: > Am 2022-08-30 13:53, schrieb Stefan Roese: >> Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE >> enabled, like pogo_v4. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> --- >> drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c >> index 02ed138642b8..7e920eaeaa40 100644 >> --- a/drivers/timer/orion-timer.c >> +++ b/drivers/timer/orion-timer.c >> @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) >> } >> #endif >> >> +#if CONFIG_IS_ENABLED(BOOTSTAGE) >> +ulong timer_get_boot_us(void) >> +{ >> + u64 ticks = 0; >> + u32 rate = 1; >> + u64 us; >> + int ret; >> + >> + ret = dm_timer_init(); >> + if (!ret) { >> + /* The timer is available */ >> + rate = timer_get_rate(gd->timer); >> + timer_get_count(gd->timer, &ticks); >> + } else { >> + return 0; >> + } >> + >> + us = (ticks * 1000) / rate; >> + return us; >> +} >> +#endif > > This is duplicate code in almost all the timer drivers, shouldn't > this be a (weak) default implementation in timer-uclass.c? Yes. I was lazy and just copied this function and did not notice, that even more timer drivers have this function. Of course it makes sense to not duplicate code here, but have a common function for this. Frankly I don't even know why exactly this function is needed, as I did not look into BOOTSTAGE yet. Simon, do we really need this function? Can't bootstage just use the "normal" timer functionality instead? > Also, > do you need to guard it with CONFIG_IS_ENABLED(BOOTSTAGE), aren't > unused functions discarded anyway? Yes, this should be the case. Also just a result of my lazy copy-and- past. Thanks, Stefan
Hi Stefan, On Tue, 30 Aug 2022 at 06:08, Stefan Roese <sr@denx.de> wrote: > > Adding Simon to Cc... > > On 30.08.22 14:00, Michael Walle wrote: > > Am 2022-08-30 13:53, schrieb Stefan Roese: > >> Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE > >> enabled, like pogo_v4. > >> > >> Signed-off-by: Stefan Roese <sr@denx.de> > >> --- > >> drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c > >> index 02ed138642b8..7e920eaeaa40 100644 > >> --- a/drivers/timer/orion-timer.c > >> +++ b/drivers/timer/orion-timer.c > >> @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) > >> } > >> #endif > >> > >> +#if CONFIG_IS_ENABLED(BOOTSTAGE) > >> +ulong timer_get_boot_us(void) > >> +{ > >> + u64 ticks = 0; > >> + u32 rate = 1; > >> + u64 us; > >> + int ret; > >> + > >> + ret = dm_timer_init(); > >> + if (!ret) { > >> + /* The timer is available */ > >> + rate = timer_get_rate(gd->timer); > >> + timer_get_count(gd->timer, &ticks); > >> + } else { > >> + return 0; > >> + } > >> + > >> + us = (ticks * 1000) / rate; > >> + return us; > >> +} > >> +#endif > > > > This is duplicate code in almost all the timer drivers, shouldn't > > this be a (weak) default implementation in timer-uclass.c? > > Yes. I was lazy and just copied this function and did not notice, that > even more timer drivers have this function. Of course it makes sense > to not duplicate code here, but have a common function for this. Frankly > I don't even know why exactly this function is needed, as I did not > look into BOOTSTAGE yet. > > Simon, do we really need this function? Can't bootstage just use the > "normal" timer functionality instead? It is needed because bootstage is called before driver model is ready. In fact it can be used to time driver model things. The function here isn't that useful, since we cannot call dm_timer_init() before driver model is set up. The timer_get_boot_us() function can by in a timer driver, but must exist outside the driver infrastructure. It must init the hard and then return the correct time. One driver model probes the timer driver, it must then continue on in that way. Actually it looks like all the timers do this wrong. See arch/arm/mach-imx/syscounter.c or arch/arm/cpu/armv8/generic_timer.c for some ideas. > > > Also, > > do you need to guard it with CONFIG_IS_ENABLED(BOOTSTAGE), aren't > > unused functions discarded anyway? > > Yes, this should be the case. Also just a result of my lazy copy-and- > past. > Regards, SImon
Hi Simon, On 30.08.22 17:56, Simon Glass wrote: > Hi Stefan, > > On Tue, 30 Aug 2022 at 06:08, Stefan Roese <sr@denx.de> wrote: >> >> Adding Simon to Cc... >> >> On 30.08.22 14:00, Michael Walle wrote: >>> Am 2022-08-30 13:53, schrieb Stefan Roese: >>>> Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE >>>> enabled, like pogo_v4. >>>> >>>> Signed-off-by: Stefan Roese <sr@denx.de> >>>> --- >>>> drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c >>>> index 02ed138642b8..7e920eaeaa40 100644 >>>> --- a/drivers/timer/orion-timer.c >>>> +++ b/drivers/timer/orion-timer.c >>>> @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) >>>> } >>>> #endif >>>> >>>> +#if CONFIG_IS_ENABLED(BOOTSTAGE) >>>> +ulong timer_get_boot_us(void) >>>> +{ >>>> + u64 ticks = 0; >>>> + u32 rate = 1; >>>> + u64 us; >>>> + int ret; >>>> + >>>> + ret = dm_timer_init(); >>>> + if (!ret) { >>>> + /* The timer is available */ >>>> + rate = timer_get_rate(gd->timer); >>>> + timer_get_count(gd->timer, &ticks); >>>> + } else { >>>> + return 0; >>>> + } >>>> + >>>> + us = (ticks * 1000) / rate; >>>> + return us; >>>> +} >>>> +#endif >>> >>> This is duplicate code in almost all the timer drivers, shouldn't >>> this be a (weak) default implementation in timer-uclass.c? >> >> Yes. I was lazy and just copied this function and did not notice, that >> even more timer drivers have this function. Of course it makes sense >> to not duplicate code here, but have a common function for this. Frankly >> I don't even know why exactly this function is needed, as I did not >> look into BOOTSTAGE yet. >> >> Simon, do we really need this function? Can't bootstage just use the >> "normal" timer functionality instead? > > It is needed because bootstage is called before driver model is ready. > In fact it can be used to time driver model things. I see, makes sense. This brings up my next questions though, why isn't CONFIG_TIMER_EARLY enough in this case? AFAICT it's targeted exactly for this early (pre DM) bootstage. From drivers/timer/Kconfig: config TIMER_EARLY bool "Allow timer to be used early in U-Boot" depends on TIMER # initr_bootstage() requires a timer and is called before initr_dm() # so only the early timer is available default y if X86 && BOOTSTAGE help In some cases the timer must be accessible before driver model is active. Examples include when using CONFIG_TRACE to trace U-Boot's execution before driver model is set up. Enable this option to use an early timer. These functions must be supported by your timer driver: timer_early_get_count() and timer_early_get_rate(). So again, do we really need timer_get_boot_us() or isn't it enough to select TIMER_EARLY when BOOTSTAGE is enabled? Thanks, Stefan > The function here isn't that useful, since we cannot call > dm_timer_init() before driver model is set up. > > The timer_get_boot_us() function can by in a timer driver, but must > exist outside the driver infrastructure. It must init the hard and > then return the correct time. One driver model probes the timer > driver, it must then continue on in that way. > > Actually it looks like all the timers do this wrong. See > arch/arm/mach-imx/syscounter.c or arch/arm/cpu/armv8/generic_timer.c > for some ideas. Thanks, Stefan
Hi Stefan, On Tue, 30 Aug 2022 at 23:57, Stefan Roese <sr@denx.de> wrote: > > Hi Simon, > > On 30.08.22 17:56, Simon Glass wrote: > > Hi Stefan, > > > > On Tue, 30 Aug 2022 at 06:08, Stefan Roese <sr@denx.de> wrote: > >> > >> Adding Simon to Cc... > >> > >> On 30.08.22 14:00, Michael Walle wrote: > >>> Am 2022-08-30 13:53, schrieb Stefan Roese: > >>>> Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE > >>>> enabled, like pogo_v4. > >>>> > >>>> Signed-off-by: Stefan Roese <sr@denx.de> > >>>> --- > >>>> drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ > >>>> 1 file changed, 22 insertions(+) > >>>> > >>>> diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c > >>>> index 02ed138642b8..7e920eaeaa40 100644 > >>>> --- a/drivers/timer/orion-timer.c > >>>> +++ b/drivers/timer/orion-timer.c > >>>> @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) > >>>> } > >>>> #endif > >>>> > >>>> +#if CONFIG_IS_ENABLED(BOOTSTAGE) > >>>> +ulong timer_get_boot_us(void) > >>>> +{ > >>>> + u64 ticks = 0; > >>>> + u32 rate = 1; > >>>> + u64 us; > >>>> + int ret; > >>>> + > >>>> + ret = dm_timer_init(); > >>>> + if (!ret) { > >>>> + /* The timer is available */ > >>>> + rate = timer_get_rate(gd->timer); > >>>> + timer_get_count(gd->timer, &ticks); > >>>> + } else { > >>>> + return 0; > >>>> + } > >>>> + > >>>> + us = (ticks * 1000) / rate; > >>>> + return us; > >>>> +} > >>>> +#endif > >>> > >>> This is duplicate code in almost all the timer drivers, shouldn't > >>> this be a (weak) default implementation in timer-uclass.c? > >> > >> Yes. I was lazy and just copied this function and did not notice, that > >> even more timer drivers have this function. Of course it makes sense > >> to not duplicate code here, but have a common function for this. Frankly > >> I don't even know why exactly this function is needed, as I did not > >> look into BOOTSTAGE yet. > >> > >> Simon, do we really need this function? Can't bootstage just use the > >> "normal" timer functionality instead? > > > > It is needed because bootstage is called before driver model is ready. > > In fact it can be used to time driver model things. > > I see, makes sense. This brings up my next questions though, why isn't > CONFIG_TIMER_EARLY enough in this case? AFAICT it's targeted exactly > for this early (pre DM) bootstage. From drivers/timer/Kconfig: > > config TIMER_EARLY > bool "Allow timer to be used early in U-Boot" > depends on TIMER > # initr_bootstage() requires a timer and is called before initr_dm() > # so only the early timer is available > default y if X86 && BOOTSTAGE > help > In some cases the timer must be accessible before driver model is > active. Examples include when using CONFIG_TRACE to trace U-Boot's > execution before driver model is set up. Enable this option to > use an early timer. These functions must be supported by your timer > driver: timer_early_get_count() and timer_early_get_rate(). > > So again, do we really need timer_get_boot_us() or isn't it enough > to select TIMER_EARLY when BOOTSTAGE is enabled? The timer is for milliseconds but for bootstage we need microseconds. Perhaps the ultimate solution here is to support a microsecond timer through the TIMER api and use the TIMER_EARLY thing to provide timer_get_boot_us(), perhaps renaming to timer_early_get_us() ? > > Thanks, > Stefan > > > The function here isn't that useful, since we cannot call > > dm_timer_init() before driver model is set up. > > > > The timer_get_boot_us() function can by in a timer driver, but must > > exist outside the driver infrastructure. It must init the hard and > > then return the correct time. One driver model probes the timer > > driver, it must then continue on in that way. > > > > Actually it looks like all the timers do this wrong. See > > arch/arm/mach-imx/syscounter.c or arch/arm/cpu/armv8/generic_timer.c > > for some ideas. Regards, Simuon
Hi Simon, On 31.08.22 19:44, Simon Glass wrote: <snip> >>> It is needed because bootstage is called before driver model is ready. >>> In fact it can be used to time driver model things. >> >> I see, makes sense. This brings up my next questions though, why isn't >> CONFIG_TIMER_EARLY enough in this case? AFAICT it's targeted exactly >> for this early (pre DM) bootstage. From drivers/timer/Kconfig: >> >> config TIMER_EARLY >> bool "Allow timer to be used early in U-Boot" >> depends on TIMER >> # initr_bootstage() requires a timer and is called before initr_dm() >> # so only the early timer is available >> default y if X86 && BOOTSTAGE >> help >> In some cases the timer must be accessible before driver model is >> active. Examples include when using CONFIG_TRACE to trace U-Boot's >> execution before driver model is set up. Enable this option to >> use an early timer. These functions must be supported by your timer >> driver: timer_early_get_count() and timer_early_get_rate(). >> >> So again, do we really need timer_get_boot_us() or isn't it enough >> to select TIMER_EARLY when BOOTSTAGE is enabled? > > The timer is for milliseconds but for bootstage we need microseconds. > > Perhaps the ultimate solution here is to support a microsecond timer > through the TIMER api and use the TIMER_EARLY thing to provide > timer_get_boot_us(), perhaps renaming to timer_early_get_us() ? Yes, sounds like a plan. We should consolidate these implementations. Let me think about it and perhaps do some basic implementations and tests for a while. Thanks, Stefan
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index 02ed138642b8..7e920eaeaa40 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) } #endif +#if CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{ + u64 ticks = 0; + u32 rate = 1; + u64 us; + int ret; + + ret = dm_timer_init(); + if (!ret) { + /* The timer is available */ + rate = timer_get_rate(gd->timer); + timer_get_count(gd->timer, &ticks); + } else { + return 0; + } + + us = (ticks * 1000) / rate; + return us; +} +#endif + static uint64_t orion_timer_get_count(struct udevice *dev) { struct orion_timer_priv *priv = dev_get_priv(dev);
Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE enabled, like pogo_v4. Signed-off-by: Stefan Roese <sr@denx.de> --- drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)