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, &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
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, &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? > > > + 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, &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? > + if (ret) > + return ret; > + > + return 0; > +} Regards, Bin