Hi Stefan, On Tue, 30 Aug 2022 at 23:57, Stefan Roese <[email protected]> wrote: > > Hi Simon, > > On 30.08.22 17:56, Simon Glass wrote: > > Hi Stefan, > > > > On Tue, 30 Aug 2022 at 06:08, Stefan Roese <[email protected]> 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 <[email protected]> > >>>> --- > >>>> 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
