RE: [PATCH 1/3] riscv: Add timer_get_us() for tracing
Hi Bin,
>-Original Message-
>From: Rick Chen
>Sent: 21 October 2020 08:58
>To: Pragnesh Patel
>Cc: U-Boot Mailing List ; Atish Patra
>; Anup Patel ; Sagar Kadam
>; Bin Meng ; Lukas Auer
>; Sean Anderson ; rick
>; Alan Kao
>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...@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
>> 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
>> > ---
>> > 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 00..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
>> > + */
>> > +
>> > +#include
>> > +#include
>> > +#include
>> > +
>> > +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, );
>> > + 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
Re: [PATCH 1/3] riscv: Add timer_get_us() for tracing
Hi Pragnesh
> From: Bin Meng [mailto:bmeng...@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
> 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
> > ---
> > 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 00..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
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +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?
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, );
> > + 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?
>
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
>
> Regards,
> Bin
Re: [PATCH 1/3] riscv: Add timer_get_us() for tracing
Hi Pragnesh,
On Mon, Aug 24, 2020 at 10:45 PM Pragnesh Patel
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
> ---
> 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 00..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
> + */
> +
> +#include
> +#include
> +#include
> +
> +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?
> +{
> + 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, );
> + 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?
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
Regards,
Bin
[PATCH 1/3] riscv: Add timer_get_us() for tracing
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
---
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 00..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
+ */
+
+#include
+#include
+#include
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static struct udevice *timer;
+
+ulong notrace timer_get_us(void)
+{
+ 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, );
+ if (ret)
+ return ret;
+
+ rate = timer_get_rate(timer);
+ }
+
+ return (ulong)count / rate;
+}
+
+int timer_init(void)
+{
+ int ret;
+
+ ret = dm_timer_init();
+ if (ret)
+ return ret;
+
+ return 0;
+}
--
2.17.1
