[PATCH 04/10] timer: cadence-ttc: Add timer_early functions

[Stefan Roese]

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?

> 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:

=> bootstage report
Timer summary in microseconds (9 records):
        Mark    Elapsed  Stage
           0          0  reset
           1          1  board_init_f
       7,908      7,907  board_init_r
       8,306        398  eth_common_init
       8,311          5  main_loop
     561,229    552,918  cli_loop

Accumulated time:
                      9  of_live
                     39  dm_f
                     39  dm_r

> Would be good if you can also add u-boot,dm-pre-reloc before 9/10 patch 
> which enables CONFIG_TIMER_EARLY.

Will do.

Thanks,
Stefan