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

[Michal Simek <michal.simek@amd.com>]

Hi,

On 10/4/22 18:29, Simon Glass wrote:
> Hi,
> 
> On Tue, 4 Oct 2022 at 05:50, Michal Simek <michal.simek@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@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@denx.de>
>>>>>>>> Cc: Michal Simek <michal.simek@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@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;
> }

ok this is clear and understandable.
As far as I see the only information we need is to have address where timer is.

Rockchip and uclass drivers are using tick-timer when multiple timers are 
described in DT.
That's why for systems with only one timer this property is not needed.
Also this option is enabled only when OF_REAL is enabled.

When it is clear which link to use for getting address from DT or via DM we can 
change drivers pretty easily.

Thanks,
Michal