[PATCH 04/10] timer: cadence-ttc: Add timer_early functions
Simon Glass
sjg at chromium.org
Tue Oct 4 18:29:50 CEST 2022
Hi,
On Tue, 4 Oct 2022 at 05:50, Michal Simek <michal.simek at 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 <sr at 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 <sr at denx.de>
> >>>>>> Cc: Michal Simek <michal.simek at 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 <sjg at 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 at 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 at ff010000
> >> Out: serial at ff010000
> >> Err: serial at 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 at ff110000
> >> serial 0 [ + ] serial_zynq | `-- serial at 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
More information about the U-Boot
mailing list