Hi Leo, >-----Original Message----- >From: Leo Liang <ycli...@andestech.com> >Sent: 23 November 2020 11:28 >To: Pragnesh Patel <pragnesh.pa...@openfive.com> >Cc: Rick Chen <rickche...@gmail.com>; U-Boot Mailing List <u- >b...@lists.denx.de>; Atish Patra <atish.pa...@wdc.com>; >palmerdabb...@google.com; Bin Meng <bmeng...@gmail.com>; Paul Walmsley >( Sifive) <paul.walms...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; Sagar >Kadam <sagar.ka...@openfive.com>; Sean Anderson <sean...@gmail.com>; >rick <r...@andestech.com>; Alan Kao <alan...@andestech.com> >Subject: Re: [PATCH v3 1/1] 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 > >On Tue, Nov 17, 2020 at 08:30:21AM +0000, Pragnesh Patel wrote: > >Hi Pragnesh, > >> Hi Rick, >> >> >-----Original Message----- >> >From: Rick Chen <rickche...@gmail.com> >> >Sent: 13 November 2020 13:37 >> >To: Pragnesh Patel <pragnesh.pa...@openfive.com> >> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra >> ><atish.pa...@wdc.com>; palmerdabb...@google.com; Bin Meng >> ><bmeng...@gmail.com>; Paul Walmsley ( Sifive) >> ><paul.walms...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; Sagar >> >Kadam <sagar.ka...@openfive.com>; Sean Anderson ><sean...@gmail.com>; >> >rick <r...@andestech.com>; Alan Kao <alan...@andestech.com>; Leo >> >Liang <ycli...@andestech.com> >> >Subject: Re: [PATCH v3 1/1] 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: Pragnesh Patel [mailto:pragnesh.pa...@sifive.com] >> >> Sent: Wednesday, November 11, 2020 6:15 PM >> >> To: u-boot@lists.denx.de >> >> Cc: atish.pa...@wdc.com; palmerdabb...@google.com; >> >bmeng...@gmail.com; >> >> paul.walms...@sifive.com; anup.pa...@wdc.com; >> >> sagar.ka...@sifive.com; Rick Jian-Zhi Chen(陳建志); Pragnesh Patel; >> >> Sean Anderson; Heinrich Schuchardt; Simon Glass >> >> Subject: [PATCH v3 1/1] riscv: Add timer_get_us() for tracing >> >> >> >> Add timer_get_us() which is useful for tracing. >> >> For S-mode U-Boot, CSR_TIMEH and CSR_TIME will provide a timer >> >> ticks and For M-mode U-Boot, mtime register will provide the same. >> >> >> >> Signed-off-by: Pragnesh Patel <pragnesh.pa...@sifive.com> >> >> --- >> >> >> >> Changes in v3: >> >> - Added gd->arch.plmt in global data >> >> - For timer_get_us(), use readq() instead of andes_plmt_get_count() >> >> and sifive_clint_get_count() >> >> >> >> Changes in v2: >> >> - Added timer_get_us() in riscv_timer.c, sifive_clint_timer.c >> >> and andes_plmt_timer.c. >> >> >> >> >> >> arch/riscv/include/asm/global_data.h | 3 +++ >> >> drivers/timer/andes_plmt_timer.c | 19 ++++++++++++++++++- >> >> drivers/timer/riscv_timer.c | 14 +++++++++++++- >> >> drivers/timer/sifive_clint_timer.c | 19 ++++++++++++++++++- >> >> 4 files changed, 52 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/arch/riscv/include/asm/global_data.h >> >> b/arch/riscv/include/asm/global_data.h >> >> index d3a0b1d221..4e22ceb83f 100644 >> >> --- a/arch/riscv/include/asm/global_data.h >> >> +++ b/arch/riscv/include/asm/global_data.h >> >> @@ -24,6 +24,9 @@ struct arch_global_data { #ifdef CONFIG_ANDES_PLIC >> >> void __iomem *plic; /* plic base address */ >> >> #endif >> >> +#ifdef CONFIG_ANDES_PLMT >> > >> >It shall be CONFIG_ANDES_PLMT, or it will compile fail as below: >> > >> >drivers/timer/andes_plmt_timer.c: In function 'timer_get_us': >> >drivers/timer/andes_plmt_timer.c:36:15: error: 'struct >> >arch_global_data' has no member named 'plmt'; did you mean 'plic'? >> > if (gd->arch.plmt) { >> > ^~~~ >> > plic >> >drivers/timer/andes_plmt_timer.c:37:52: error: 'struct >> >arch_global_data' has no member named 'plmt'; did you mean 'plic'? >> > ticks = readq((void __iomem *)MTIME_REG(gd->arch.plmt)); >> > > >This patch will cause compile error with ae350 defconfig. > >${uboot}/drivers/timer/Makefile is articulated in this way, >"obj-$(CONFIG_ANDES_PLMT_TIMER) += andes_plmt_timer.o", so the #ifdef >indicator in ${uboot}/arch/riscv/include/asm/global_data.h should use >CONFIG_ANDES_PLMT_TIMER.
Thanks Leo but Rick has already informed about this error. I have discarded this series and added a new patch for Tracing which uses TIMER_EARLY https://patchwork.ozlabs.org/project/uboot/patch/20201117110508.25819-1-pragnesh.pa...@sifive.com/ > >Best regards, >Leo > >> >And it is not proper to have dependency of data structure between >> >/arch/riscv/include/asm/* and /drivers/timer/* Maybe enable >> >TIMER_EARLY and use timer_get_us() of /lib/time.c will be better. >> >> I am planning to use TIMER_EARLY for tracing. >> >> With TIMER_EARLY, I need to implement timer_early_get_rate() and >> timer_early_get_count() >> >> For timer_early_get_count(), I need PLMT or CLINT base address to read mtime >register. >> So I think it's better to save base address in gd->arch.clint or >> gd->arch.plmt. >> >> Let me know if you have any other idea to save PLM or CLINT base address. >> >> For timer_early_get_rate(), I will add new variable in global data (gd) >> Like, gd->arch.clock_rate; /* Clock rate of timer in Hz */ >> This will return Timer frequency in Hz. >> >> Tracing is only useful in U-Boot not in U-Boot SPL so I am doing this >> for M mode U-boot and S mode U-Boot. >> >> >I also found that without dm_timer_init() in initf_dm(), >> >andes_plmt_get_count() will be executed ahead of andes_plmt_probe(). >> >> If andes_plmt_get_count() executed before andes_plmt_probe() then how >> did it will get PLMT base address ? >> >> > >> >Thanks, >> >Rick >> > >> > >> >> + void __iomem *plmt; /* plmt base address */ >> >> +#endif >> >> #if CONFIG_IS_ENABLED(SMP) >> >> struct ipi_data ipi[CONFIG_NR_CPUS]; #endif diff --git >> >> a/drivers/timer/andes_plmt_timer.c >> >> b/drivers/timer/andes_plmt_timer.c >> >> index cec86718c7..7c50c46d9e 100644 >> >> --- a/drivers/timer/andes_plmt_timer.c >> >> +++ b/drivers/timer/andes_plmt_timer.c >> >> @@ -13,11 +13,12 @@ >> >> #include <timer.h> >> >> #include <asm/io.h> >> >> #include <linux/err.h> >> >> +#include <div64.h> >> >> >> >> /* mtime register */ >> >> #define MTIME_REG(base) ((ulong)(base)) >> >> >> >> -static u64 andes_plmt_get_count(struct udevice *dev) >> >> +static u64 notrace andes_plmt_get_count(struct udevice *dev) >> >> { >> >> return readq((void __iomem *)MTIME_REG(dev->priv)); } @@ >> >> -26,12 +27,28 @@ static const struct timer_ops andes_plmt_ops = { >> >> .get_count = andes_plmt_get_count, }; >> >> >> >> +#if CONFIG_IS_ENABLED(RISCV_MMODE) unsigned long notrace >> >> +timer_get_us(void) { >> >> + u64 ticks; >> >> + >> >> + /* FIXME: gd->arch.plmt should contain valid base address */ >> >> + if (gd->arch.plmt) { >> >> + ticks = readq((void __iomem *)MTIME_REG(gd->arch.plmt)); >> >> + do_div(ticks, CONFIG_SYS_HZ); >> >> + } >> >> + >> >> + return ticks; >> >> +} >> >> +#endif >> >> + >> >> static int andes_plmt_probe(struct udevice *dev) { >> >> dev->priv = dev_read_addr_ptr(dev); >> >> if (!dev->priv) >> >> return -EINVAL; >> >> >> >> + gd->arch.plmt = dev->priv; >> >> return timer_timebase_fallback(dev); } >> >> >> >> diff --git a/drivers/timer/riscv_timer.c >> >> b/drivers/timer/riscv_timer.c index 21ae184057..7fa8773da3 100644 >> >> --- a/drivers/timer/riscv_timer.c >> >> +++ b/drivers/timer/riscv_timer.c >> >> @@ -15,8 +15,9 @@ >> >> #include <errno.h> >> >> #include <timer.h> >> >> #include <asm/csr.h> >> >> +#include <div64.h> >> >> >> >> -static u64 riscv_timer_get_count(struct udevice *dev) >> >> +static u64 notrace riscv_timer_get_count(struct udevice *dev) >> >> { >> >> __maybe_unused u32 hi, lo; >> >> >> >> @@ -31,6 +32,17 @@ static u64 riscv_timer_get_count(struct udevice >*dev) >> >> return ((u64)hi << 32) | lo; } >> >> >> >> +#if CONFIG_IS_ENABLED(RISCV_SMODE) unsigned long notrace >> >> +timer_get_us(void) { >> >> + u64 ticks; >> >> + >> >> + ticks = riscv_timer_get_count(NULL); >> >> + do_div(ticks, CONFIG_SYS_HZ); >> >> + return ticks; >> >> +} >> >> +#endif >> >> + >> >> static int riscv_timer_probe(struct udevice *dev) { >> >> struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev); >> >> diff --git a/drivers/timer/sifive_clint_timer.c >> >> b/drivers/timer/sifive_clint_timer.c >> >> index 00ce0f08d6..c341f7789b 100644 >> >> --- a/drivers/timer/sifive_clint_timer.c >> >> +++ b/drivers/timer/sifive_clint_timer.c >> >> @@ -10,11 +10,12 @@ >> >> #include <timer.h> >> >> #include <asm/io.h> >> >> #include <linux/err.h> >> >> +#include <div64.h> >> >> >> >> /* mtime register */ >> >> #define MTIME_REG(base) ((ulong)(base) + 0xbff8) >> >> >> >> -static u64 sifive_clint_get_count(struct udevice *dev) >> >> +static u64 notrace sifive_clint_get_count(struct udevice *dev) >> >> { >> >> return readq((void __iomem *)MTIME_REG(dev->priv)); } @@ >> >> -23,12 +24,28 @@ static const struct timer_ops sifive_clint_ops = { >> >> .get_count = sifive_clint_get_count, }; >> >> >> >> +#if CONFIG_IS_ENABLED(RISCV_MMODE) unsigned long notrace >> >> +timer_get_us(void) { >> >> + u64 ticks; >> >> + >> >> + /* FIXME: gd->arch.clint should contain valid base address */ >> >> + if (gd->arch.clint) { >> >> + ticks = readq((void __iomem *)MTIME_REG(gd->arch.clint)); >> >> + do_div(ticks, CONFIG_SYS_HZ); >> >> + } >> >> + >> >> + return ticks; >> >> +} >> >> +#endif >> >> + >> >> static int sifive_clint_probe(struct udevice *dev) { >> >> dev->priv = dev_read_addr_ptr(dev); >> >> if (!dev->priv) >> >> return -EINVAL; >> >> >> >> + gd->arch.clint = dev->priv; >> >> return timer_timebase_fallback(dev); } >> >> >> >> -- >> >> 2.17.1