Hi, On Tue, 4 Oct 2022 at 05:50, Michal Simek <michal.si...@amd.com> wrote: > > Hi Stefan, > > On 9/30/22 15:45, Stefan Roese wrote: > > Hi Michal, > > > > On 30.09.22 14:02, Michal Simek wrote: > >> Hi Simon and Stefan, > >> > >> On 9/28/22 03:54, Simon Glass wrote: > >>>> Hi Simon, > >>>> Hi Michal, > >>>> > >>>> On 25.09.22 16:15, Simon Glass wrote: > >>>>> Hi Stefan, > >>>>> > >>>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <s...@denx.de> wrote: > >>>>>> > >>>>>> Currently this timer driver provides timer_get_boot_us() to support the > >>>>>> BOOTSTAGE functionality. This patch adds the timer_early functions so > >>>>>> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY > >>>>>> is enabled. > >>>>>> > >>>>>> timer_get_boot_us() will get removed in a follow-up patch, once the > >>>>>> BOOTSTAGE interface is migrated to timer_get_us(). > >>>>>> > >>>>>> Signed-off-by: Stefan Roese <s...@denx.de> > >>>>>> Cc: Michal Simek <michal.si...@xilinx.com> > >>>>>> --- > >>>>>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++ > >>>>>> 1 file changed, 25 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c > >>>>>> index 2eff45060ad6..e26c7923a140 100644 > >>>>>> --- a/drivers/timer/cadence-ttc.c > >>>>>> +++ b/drivers/timer/cadence-ttc.c > >>>>>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void) > >>>>>> } > >>>>>> #endif > >>>>>> > >>>>>> +unsigned long notrace timer_early_get_rate(void) > >>>>>> +{ > >>>>>> + return 1; > >>>>>> +} > >>>>>> + > >>>>>> +u64 notrace timer_early_get_count(void) > >>>>>> +{ > >>>>>> + u64 ticks = 0; > >>>>>> + u32 rate = 1; > >>>>>> + u64 us; > >>>>>> + int ret; > >>>>>> + > >>>>>> + ret = dm_timer_init(); > >>>>> > >>>>> I don't think you can call this if you want to support bootstage, > >>>>> since driver model may not be inited. > >>>> > >>>> Yes, thanks for noticing. Still, this code is copied from the original > >>>> timer_get_boot_us() function in this driver. Which also has problems > >>>> with early timer access AFAICT. > >>> > >>> Yes, good point. > >>> > >>> Reviewed-by: Simon Glass <s...@chromium.org> > >> > >> I looked at it and kind of interesting code. For getting this working with > >> this whole series if ttc gets u-boot,dm-pre-reloc everthing is working > >> fine. > > > > Yes, makes sense that this is needed. > > > >> diff --git a/arch/arm/dts/zynqmp-r5.dts b/arch/arm/dts/zynqmp-r5.dts > >> index a72172ef2ea4..8059931f2162 100644 > >> --- a/arch/arm/dts/zynqmp-r5.dts > >> +++ b/arch/arm/dts/zynqmp-r5.dts > >> @@ -56,6 +56,7 @@ > >> > >> ttc0: timer@ff110000 { > >> compatible = "cdns,ttc"; > >> + u-boot,dm-pre-reloc; > >> status = "okay"; > >> reg = <0xff110000 0x1000>; > >> timer-width = <32>; > >> > >> What I see based on behavior. Every bootstage call is asking for > >> timer_early_get_count() and because dm_timer_init() is failing 0 is > >> returned. > >> > >> Inside initf_dm() dm_init_and_scan() is called and initialized timer uclass > >> also with timer instance. With u-boot,dm-pre-reloc also cadence timer > >> driver. > >> On the next dm_timer_init() gd->timer is initialized and value is returned > >> and > >> initf_dm() passes. > > > > This "early" implementation of the counter is broken, as you have > > noticed above. Resulting in timer functions only returning valid > > values after dm_timer_init(). This timer driver needs re-written > > early_timer functions, that will return proper timer values even in > > the early boot phase. Please take a look at e.g. rockchip_timer.c, > > perhaps something similar works for the cadence timer driver as well? > > I looked at it and there is a code when OF_REAL is enabled. If not then 0 is > returned. That's why I can't see any difference between rockchip and cadence > when OF_REAL is not enabled. > > And not sure how this is working in rockchip case. Who is starting timer in > their case? I see start when probe happens but after it dm_timer_init() should > return 0 and value can be obtained. > > > > >> Here is the log and output from bootstage. > >> > >> U-Boot 2022.10-rc5-00130-g1be5bed207c9 (Sep 30 2022 - 14:00:08 +0200) > >> > >> Model: Xilinx ZynqMP R5 > >> DRAM: 512 MiB > >> Core: 7 devices, 7 uclasses, devicetree: embed > >> MMC: > >> Loading Environment from nowhere... OK > >> In: serial@ff010000 > >> Out: serial@ff010000 > >> Err: serial@ff010000 > >> Net: No ethernet found. > >> ZynqMP r5> dm tree > >> Class Index Probed Driver Name > >> ----------------------------------------------------------- > >> root 0 [ + ] root_driver root_driver > >> clk 0 [ + ] fixed_clock |-- clk100 > >> simple_bus 0 [ + ] simple_bus |-- amba > >> timer 0 [ + ] cadence_ttc | |-- timer@ff110000 > >> serial 0 [ + ] serial_zynq | `-- serial@ff010000 > >> bootstd 0 [ ] bootstd_drv `-- bootstd > >> bootmeth 0 [ ] bootmeth_distro `-- distro > >> ZynqMP r5> bootstage report > >> Timer summary in microseconds (8 records): > >> Mark Elapsed Stage > >> 0 0 reset > >> 0 0 board_init_f > >> 0 0 dm_f > >> 0 0 dm_r > >> 16,319 16,319 eth_common_init > >> 18,061 1,742 main_loop > >> 18,281 220 cli_loop > >> 29,249 10,968 board_init_r > >> > >> Accumulated time: > > > > Take a look at sandbox - here you will get proper early timing values > > as well: > > I expect early functions are called before DM is ready that's why to get it > work > that early we would have to start timer on the first call of early function. > It is definitely doable but is it worth to do it? > > Is there any other use case where early timer counts are needed other then > bootstage?
Here is how it should work, I think: static void notrace ensure_timer_setup(void) { // set up the timer if not already done // avoid using any DM functions } static u64 xx_get_count(void) { // read and return the timer counter } u64 notrace timer_early_get_count(void) { ensure_timer_setup(); return xxx_get_count(); } static u64 xx_timer_get_count(struct udevice *dev) { return xxx_get_count(); } int xxx_timer_probe(struct udevice *dev) { ensure_timer_setup()\; return 0; } Regards, Simon