[PATCH 1/3] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

Hi Bin,

>-----Original Message-----
>From: Rick Chen <rickchen36 at gmail.com>
>Sent: 21 October 2020 08:58
>To: Pragnesh Patel <pragnesh.patel at openfive.com>
>Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Atish Patra
><atish.patra at wdc.com>; Anup Patel <anup.patel at wdc.com>; Sagar Kadam
><sagar.kadam at openfive.com>; Bin Meng <bmeng.cn at gmail.com>; Lukas Auer
><lukas.auer at aisec.fraunhofer.de>; Sean Anderson <seanga2 at gmail.com>; rick
><rick at andestech.com>; Alan Kao <alankao at andestech.com>
>Subject: Re: [PATCH 1/3] riscv: Add timer_get_us() for tracing
>
>[External Email] Do not click links or attachments unless you recognize the
>sender and know the content is safe
>
>Hi Pragnesh
>
>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
>> Sent: Friday, September 11, 2020 2:48 PM
>> To: Pragnesh Patel
>> Cc: U-Boot Mailing List; Atish Patra; Anup Patel; Sagar Kadam; Rick
>> Jian-Zhi Chen(陳建志); Paul Walmsley; Bin Meng; Lukas Auer; Sean Anderson
>> Subject: Re: [PATCH 1/3] riscv: Add timer_get_us() for tracing
>>
>> Hi Pragnesh,
>>
>> On Mon, Aug 24, 2020 at 10:45 PM Pragnesh Patel
>> <pragnesh.patel at sifive.com> wrote:
>> >
>> > timer_get_us() will use timer_ops->get_count() function for timer counter.
>> > For S-mode U-Boot, CSR_TIMEH and CSR_TIME will provide a timer
>> > counter and For M-mode U-Boot, mtime register will provide the same.
>> >
>> > Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>
>> > ---
>> >  arch/riscv/lib/Makefile |  1 +
>> >  arch/riscv/lib/timer.c  | 50
>> > +++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 51 insertions(+)
>> >  create mode 100644 arch/riscv/lib/timer.c
>> >
>> > diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index
>> > 10ac5b06d3..fbb68e583b 100644
>> > --- a/arch/riscv/lib/Makefile
>> > +++ b/arch/riscv/lib/Makefile
>> > @@ -26,6 +26,7 @@ obj-y   += setjmp.o
>> >  obj-$(CONFIG_$(SPL_)SMP) += smp.o
>> >  obj-$(CONFIG_SPL_BUILD)        += spl.o
>> >  obj-y   += fdt_fixup.o
>> > +obj-$(CONFIG_TIMER) += timer.o
>> >
>> >  # For building EFI apps
>> >  CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI) diff --git
>> > a/arch/riscv/lib/timer.c b/arch/riscv/lib/timer.c new file mode
>> > 100644 index 0000000000..3e423f2805
>> > --- /dev/null
>> > +++ b/arch/riscv/lib/timer.c
>> > @@ -0,0 +1,50 @@
>> > +// SPDX-License-Identifier: GPL-2.0+
>> > +/*
>> > + * Copyright (C) 2020 SiFive, Inc
>> > + *
>> > + * Authors:
>> > + *   Pragnesh Patel <pragnesh.patel at sifive.com>
>> > + */
>> > +
>> > +#include <common.h>
>> > +#include <dm.h>
>> > +#include <timer.h>
>> > +
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +static struct udevice *timer;
>> > +
>> > +ulong notrace timer_get_us(void)
>>
>> Does the weak one in lib/time.c not work on RISC-V?

No because if "gd->timer" is set early then also it will become NULL in initr_dm()

static int initr_dm(void) {
...
#ifdef CONFIG_TIMER
        gd->timer = NULL;
#endif
...
}

So timer_get_us() again try to call dm_timer_init() to initialize "gd->timer" and it got stuck in tracing.

Not all the functins are marked with notrace in dm_timer_init().

>
>Do you have any comments about Bin's reply ?
>
>Thanks,
>Rick
>
>>
>> > +{
>> > +       u64 count;
>> > +       ulong rate;
>> > +       int ret;
>> > +
>> > +       /**
>> > +        * gd->timer will become NULL in initr_dm(), so assign gd->timer
>> > +        * to other static global timer, so that timer_get_us() can use it.
>> > +        */
>> > +       if (!timer && gd->timer)
>> > +               timer = (struct udevice *)gd->timer;
>> > +
>> > +       if (timer) {
>> > +               ret = timer_get_count(timer, &count);
>> > +               if (ret)
>> > +                       return ret;
>> > +
>> > +               rate = timer_get_rate(timer);
>> > +       }
>> > +
>> > +       return (ulong)count / rate;
>> > +}
>> > +
>> > +int timer_init(void)
>>
>> Why is this function necessary?
>>
>> > +{
>> > +       int ret;
>> > +
>> > +       ret = dm_timer_init();
>>
>> Does enabling CONFIG_TIMER_EARLY help?

I need to implement timer_early_get_count() and timer_early_get_rate() for that. Will look into this

>>
>> > +       if (ret)
>> > +               return ret;
>> > +
>> > +       return 0;
>> > +}
>>
>> Regards,
>> Bin