| Message ID | 20201111101435.31455-2-pragnesh.patel@sifive.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | RISC-V tracing support | expand |
On 11.11.20 11:14, Pragnesh Patel wrote: > 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.patel@sifive.com> The default implementation of get_timer_us() in lib/time.c calls get_ticks() which calls timer_get_count(). The get_count() operation is implemented in drivers/timer/andes_plmt_timer.c, drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. Why do you need special timer_get_us() implementations? Isn't it enough to supply the get_count() operation in the drivers? Best regards Heinrich > --- > > 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 > + 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); > } > >
Hi Heinrich, >-----Original Message----- >From: Heinrich Schuchardt <xypron.glpk@gmx.de> >Sent: 11 November 2020 16:51 >To: Pragnesh Patel <pragnesh.patel@openfive.com>; u-boot@lists.denx.de >Cc: atish.patra@wdc.com; palmerdabbelt@google.com; bmeng.cn@gmail.com; >Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; anup.patel@wdc.com; >Sagar Kadam <sagar.kadam@openfive.com>; rick@andestech.com; Sean >Anderson <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >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 11.11.20 11:14, Pragnesh Patel wrote: >> 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.patel@sifive.com> > >The default implementation of get_timer_us() in lib/time.c calls >get_ticks() which calls timer_get_count(). The get_count() operation is >implemented in drivers/timer/andes_plmt_timer.c, >drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. > >Why do you need special timer_get_us() implementations? >Isn't it enough to supply the get_count() operation in the drivers? get_ticks() is depend on gd->timer and there are 2 cases 1) if gd->timer== NULL then dm_timer_init() gets called and it will call functions which are not marked with "notrace" so tracing got stuck. 2) if gd->timer is already initialized then still initr_dm() will make gd->timer = NULL; initr_dm() { #ifdef CONFIG_TIMER gd->timer = NULL; #endif } So again dm_timer_init() gets called and tracing got stuck. So I thought better to implement timer_get_us(). > >Best regards > >Heinrich > >> --- >> >> 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 >> + 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); } >> >>
On 11/11/20 12:56 PM, Pragnesh Patel wrote: > Hi Heinrich, > >> -----Original Message----- >> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >> Sent: 11 November 2020 16:51 >> To: Pragnesh Patel <pragnesh.patel@openfive.com>; u-boot@lists.denx.de >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; bmeng.cn@gmail.com; >> Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; anup.patel@wdc.com; >> Sagar Kadam <sagar.kadam@openfive.com>; rick@andestech.com; Sean >> Anderson <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >> 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 11.11.20 11:14, Pragnesh Patel wrote: >>> 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.patel@sifive.com> >> >> The default implementation of get_timer_us() in lib/time.c calls >> get_ticks() which calls timer_get_count(). The get_count() operation is >> implemented in drivers/timer/andes_plmt_timer.c, >> drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. >> >> Why do you need special timer_get_us() implementations? >> Isn't it enough to supply the get_count() operation in the drivers? > > get_ticks() is depend on gd->timer and there are 2 cases > > 1) if gd->timer== NULL then dm_timer_init() gets called and it will call functions > which are not marked with "notrace" so tracing got stuck. Thanks for the background information. Please, identify the existing functions that lack "notrace" and fix them. This will give us a solution for all existing boards and not for a small selection. Furthermore it will avoid code duplication. In lib/trace.c consider replacing "__attribute__((no_instrument_function))" by "notrace". Best regards Heinrich > > 2) if gd->timer is already initialized then still initr_dm() will make gd->timer = NULL; > > initr_dm() > { > #ifdef CONFIG_TIMER > gd->timer = NULL; > #endif > } > > So again dm_timer_init() gets called and tracing got stuck. > > So I thought better to implement timer_get_us(). > >> >> Best regards >> >> Heinrich >> >>> --- >>> >>> 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 >>> + 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); } >>> >>> >
Hi Heinrich, >-----Original Message----- >From: Heinrich Schuchardt <xypron.glpk@gmx.de> >Sent: 11 November 2020 19:18 >To: Pragnesh Patel <pragnesh.patel@openfive.com> >Cc: atish.patra@wdc.com; palmerdabbelt@google.com; u-boot@lists.denx.de; >bmeng.cn@gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; >anup.patel@wdc.com; Sagar Kadam <sagar.kadam@openfive.com>; >rick@andestech.com; Sean Anderson <seanga2@gmail.com>; Simon Glass ><sjg@chromium.org> >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 11/11/20 12:56 PM, Pragnesh Patel wrote: >> Hi Heinrich, >> >>> -----Original Message----- >>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> Sent: 11 November 2020 16:51 >>> To: Pragnesh Patel <pragnesh.patel@openfive.com>; >>> u-boot@lists.denx.de >>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>> bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >>> <paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>> <sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >>> <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >>> 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 11.11.20 11:14, Pragnesh Patel wrote: >>>> 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.patel@sifive.com> >>> >>> The default implementation of get_timer_us() in lib/time.c calls >>> get_ticks() which calls timer_get_count(). The get_count() operation >>> is implemented in drivers/timer/andes_plmt_timer.c, >>> drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. >>> >>> Why do you need special timer_get_us() implementations? >>> Isn't it enough to supply the get_count() operation in the drivers? >> >> get_ticks() is depend on gd->timer and there are 2 cases >> >> 1) if gd->timer== NULL then dm_timer_init() gets called and it will >> call functions which are not marked with "notrace" so tracing got stuck. > >Thanks for the background information. > >Please, identify the existing functions that lack "notrace" and fix them. This will >give us a solution for all existing boards and not for a small selection. >Furthermore it will avoid code duplication. I tried but There are so many functions which need "notrace". Why don’t we consider removing gd->timer=NULL in initr_dm() initr_dm() { #ifdef CONFIG_TIMER gd->timer = NULL; #endif } Or I can use TIMER_EARLY and return ticks and rate by adding timer_early_get_count() and timer_early_get_rate() repectively. Suggestions are welcome > >In lib/trace.c consider replacing >"__attribute__((no_instrument_function))" by "notrace". > >Best regards > >Heinrich > >> >> 2) if gd->timer is already initialized then still initr_dm() will make >> gd->timer = NULL; >> >> initr_dm() >> { >> #ifdef CONFIG_TIMER >> gd->timer = NULL; >> #endif >> } >> >> So again dm_timer_init() gets called and tracing got stuck. >> >> So I thought better to implement timer_get_us(). >> >>> >>> Best regards >>> >>> Heinrich >>> >>>> --- >>>> >>>> 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 >>>> + 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); } >>>> >>>> >>
On 11/12/20 7:34 AM, Pragnesh Patel wrote: > Hi Heinrich, > >> -----Original Message----- >> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >> Sent: 11 November 2020 19:18 >> To: Pragnesh Patel <pragnesh.patel@openfive.com> >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; u-boot@lists.denx.de; >> bmeng.cn@gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; >> anup.patel@wdc.com; Sagar Kadam <sagar.kadam@openfive.com>; >> rick@andestech.com; Sean Anderson <seanga2@gmail.com>; Simon Glass >> <sjg@chromium.org> >> 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 11/11/20 12:56 PM, Pragnesh Patel wrote: >>> Hi Heinrich, >>> >>>> -----Original Message----- >>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>> Sent: 11 November 2020 16:51 >>>> To: Pragnesh Patel <pragnesh.patel@openfive.com>; >>>> u-boot@lists.denx.de >>>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>>> bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >>>> <paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>>> <sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >>>> <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >>>> 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 11.11.20 11:14, Pragnesh Patel wrote: >>>>> 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.patel@sifive.com> >>>> >>>> The default implementation of get_timer_us() in lib/time.c calls >>>> get_ticks() which calls timer_get_count(). The get_count() operation >>>> is implemented in drivers/timer/andes_plmt_timer.c, >>>> drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. >>>> >>>> Why do you need special timer_get_us() implementations? >>>> Isn't it enough to supply the get_count() operation in the drivers? >>> >>> get_ticks() is depend on gd->timer and there are 2 cases >>> >>> 1) if gd->timer== NULL then dm_timer_init() gets called and it will >>> call functions which are not marked with "notrace" so tracing got stuck. >> >> Thanks for the background information. >> >> Please, identify the existing functions that lack "notrace" and fix them. This will >> give us a solution for all existing boards and not for a small selection. >> Furthermore it will avoid code duplication. > > I tried but There are so many functions which need "notrace". Let's start with the problem statement: When tracing functions is enabled this adds calls to __cyg_profile_func_enter() and __cyg_profile_func_exit() to the traced functions. __cyg_profile_func_exit() and __cyg_profile_func_exit() invoke timer_get_us() to record the entry and exit time. If timer_get_us() or any function used to implement does not carry __attribute__((no_instrument_function)) this will lead to an indefinite recursion. We have to change __cyg_profile_func_enter() and __cyg_profile_func_exit() such that during their execution no function is traced. We can do so by temporarily setting trace_enabled to false. Does this match your observation? Best regards Heinrich > > Why don’t we consider removing gd->timer=NULL in initr_dm() > initr_dm() > { > #ifdef CONFIG_TIMER > gd->timer = NULL; > #endif > } > > Or I can use TIMER_EARLY and return ticks and rate by adding timer_early_get_count() > and timer_early_get_rate() repectively. > > Suggestions are welcome > >> >> In lib/trace.c consider replacing >> "__attribute__((no_instrument_function))" by "notrace". >> >> Best regards >> >> Heinrich >> >>> >>> 2) if gd->timer is already initialized then still initr_dm() will make >>> gd->timer = NULL; >>> >>> initr_dm() >>> { >>> #ifdef CONFIG_TIMER >>> gd->timer = NULL; >>> #endif >>> } >>> >>> So again dm_timer_init() gets called and tracing got stuck. >>> >>> So I thought better to implement timer_get_us(). >>> >>>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> --- >>>>> >>>>> 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 >>>>> + 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); } >>>>> >>>>> >>> >
On 11/12/20 8:09 AM, Heinrich Schuchardt wrote: > On 11/12/20 7:34 AM, Pragnesh Patel wrote: >> Hi Heinrich, >> >>> -----Original Message----- >>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> Sent: 11 November 2020 19:18 >>> To: Pragnesh Patel <pragnesh.patel@openfive.com> >>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; u-boot@lists.denx.de; >>> bmeng.cn@gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; >>> anup.patel@wdc.com; Sagar Kadam <sagar.kadam@openfive.com>; >>> rick@andestech.com; Sean Anderson <seanga2@gmail.com>; Simon Glass >>> <sjg@chromium.org> >>> 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 11/11/20 12:56 PM, Pragnesh Patel wrote: >>>> Hi Heinrich, >>>> >>>>> -----Original Message----- >>>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>> Sent: 11 November 2020 16:51 >>>>> To: Pragnesh Patel <pragnesh.patel@openfive.com>; >>>>> u-boot@lists.denx.de >>>>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>>>> bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >>>>> <paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>>>> <sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >>>>> <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >>>>> 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 11.11.20 11:14, Pragnesh Patel wrote: >>>>>> 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.patel@sifive.com> >>>>> >>>>> The default implementation of get_timer_us() in lib/time.c calls >>>>> get_ticks() which calls timer_get_count(). The get_count() operation >>>>> is implemented in drivers/timer/andes_plmt_timer.c, >>>>> drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. >>>>> >>>>> Why do you need special timer_get_us() implementations? >>>>> Isn't it enough to supply the get_count() operation in the drivers? >>>> >>>> get_ticks() is depend on gd->timer and there are 2 cases >>>> >>>> 1) if gd->timer== NULL then dm_timer_init() gets called and it will >>>> call functions which are not marked with "notrace" so tracing got stuck. >>> >>> Thanks for the background information. >>> >>> Please, identify the existing functions that lack "notrace" and fix them. This will >>> give us a solution for all existing boards and not for a small selection. >>> Furthermore it will avoid code duplication. >> >> I tried but There are so many functions which need "notrace". > > Let's start with the problem statement: > > When tracing functions is enabled this adds calls to > __cyg_profile_func_enter() and __cyg_profile_func_exit() to the traced > functions. > > __cyg_profile_func_exit() and __cyg_profile_func_exit() invoke > timer_get_us() to record the entry and exit time. > > If timer_get_us() or any function used to implement does not carry > __attribute__((no_instrument_function)) this will lead to an indefinite > recursion. > > We have to change __cyg_profile_func_enter() and > __cyg_profile_func_exit() such that during their execution no function > is traced. We can do so by temporarily setting trace_enabled to false. > > Does this match your observation? I just tried to put this into a patch [PATCH 1/1] trace: avoid infinite recursion https://lists.denx.de/pipermail/u-boot/2020-November/432589.html https://patchwork.ozlabs.org/project/uboot/patch/20201112071411.4202-1-xypron.glpk@gmx.de/ Does this solve you problem? Best regards Heinrich > >> >> Why don’t we consider removing gd->timer=NULL in initr_dm() >> initr_dm() >> { >> #ifdef CONFIG_TIMER >> gd->timer = NULL; >> #endif >> } >> >> Or I can use TIMER_EARLY and return ticks and rate by adding timer_early_get_count() >> and timer_early_get_rate() repectively. >> >> Suggestions are welcome >> >>> >>> In lib/trace.c consider replacing >>> "__attribute__((no_instrument_function))" by "notrace". >>> >>> Best regards >>> >>> Heinrich >>> >>>> >>>> 2) if gd->timer is already initialized then still initr_dm() will make >>>> gd->timer = NULL; >>>> >>>> initr_dm() >>>> { >>>> #ifdef CONFIG_TIMER >>>> gd->timer = NULL; >>>> #endif >>>> } >>>> >>>> So again dm_timer_init() gets called and tracing got stuck. >>>> >>>> So I thought better to implement timer_get_us(). >>>> >>>>> >>>>> Best regards >>>>> >>>>> Heinrich >>>>> >>>>>> --- >>>>>> >>>>>> 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 >>>>>> + 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); } >>>>>> >>>>>> >>>> >> >
Hi Heinrich, >-----Original Message----- >From: Heinrich Schuchardt <xypron.glpk@gmx.de> >Sent: 12 November 2020 12:49 >To: Pragnesh Patel <pragnesh.patel@openfive.com>; Simon Glass ><sjg@chromium.org> >Cc: atish.patra@wdc.com; palmerdabbelt@google.com; u-boot@lists.denx.de; >bmeng.cn@gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; >anup.patel@wdc.com; Sagar Kadam <sagar.kadam@openfive.com>; >rick@andestech.com; Sean Anderson <seanga2@gmail.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 11/12/20 8:09 AM, Heinrich Schuchardt wrote: >> On 11/12/20 7:34 AM, Pragnesh Patel wrote: >>> Hi Heinrich, >>> >>>> -----Original Message----- >>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>> Sent: 11 November 2020 19:18 >>>> To: Pragnesh Patel <pragnesh.patel@openfive.com> >>>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>>> u-boot@lists.denx.de; bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >>>> <paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>>> <sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >>>> <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >>>> 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 11/11/20 12:56 PM, Pragnesh Patel wrote: >>>>> Hi Heinrich, >>>>> >>>>>> -----Original Message----- >>>>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>>> Sent: 11 November 2020 16:51 >>>>>> To: Pragnesh Patel <pragnesh.patel@openfive.com>; >>>>>> u-boot@lists.denx.de >>>>>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>>>>> bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >>>>>> <paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>>>>> <sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >>>>>> <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >>>>>> 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 11.11.20 11:14, Pragnesh Patel wrote: >>>>>>> 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.patel@sifive.com> >>>>>> >>>>>> The default implementation of get_timer_us() in lib/time.c calls >>>>>> get_ticks() which calls timer_get_count(). The get_count() >>>>>> operation is implemented in drivers/timer/andes_plmt_timer.c, >>>>>> drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. >>>>>> >>>>>> Why do you need special timer_get_us() implementations? >>>>>> Isn't it enough to supply the get_count() operation in the drivers? >>>>> >>>>> get_ticks() is depend on gd->timer and there are 2 cases >>>>> >>>>> 1) if gd->timer== NULL then dm_timer_init() gets called and it will >>>>> call functions which are not marked with "notrace" so tracing got stuck. >>>> >>>> Thanks for the background information. >>>> >>>> Please, identify the existing functions that lack "notrace" and fix >>>> them. This will give us a solution for all existing boards and not for a small >selection. >>>> Furthermore it will avoid code duplication. >>> >>> I tried but There are so many functions which need "notrace". >> >> Let's start with the problem statement: >> >> When tracing functions is enabled this adds calls to >> __cyg_profile_func_enter() and __cyg_profile_func_exit() to the traced >> functions. >> >> __cyg_profile_func_exit() and __cyg_profile_func_exit() invoke >> timer_get_us() to record the entry and exit time. >> >> If timer_get_us() or any function used to implement does not carry >> __attribute__((no_instrument_function)) this will lead to an >> indefinite recursion. >> >> We have to change __cyg_profile_func_enter() and >> __cyg_profile_func_exit() such that during their execution no function >> is traced. We can do so by temporarily setting trace_enabled to false. >> >> Does this match your observation? > >I just tried to put this into a patch > >[PATCH 1/1] trace: avoid infinite recursion https://lists.denx.de/pipermail/u- >boot/2020-November/432589.html >https://patchwork.ozlabs.org/project/uboot/patch/20201112071411.4202-1- >xypron.glpk@gmx.de/ > >Does this solve you problem? Getting error, "Could not initialize timer (err -11)" from get_ticks() function. Below are my boot logs, U-Boot 2021.01-rc1-00244-g794c155581-dirty (Nov 12 2020 - 12:56:27 +0530) CPU: rv64imafdc Model: SiFive HiFive Unleashed A00 DRAM: 8 GiB trace: enabled Could not initialize timer (err -11) Could not initialize timer (err -11) Could not initialize timer (err -11) Could not initialize timer (err -11) > >Best regards > >Heinrich >> >>> >>> Why don’t we consider removing gd->timer=NULL in initr_dm() >>> initr_dm() >>> { >>> #ifdef CONFIG_TIMER >>> gd->timer = NULL; >>> #endif >>> } >>> >>> Or I can use TIMER_EARLY and return ticks and rate by adding >>> timer_early_get_count() and timer_early_get_rate() repectively. >>> >>> Suggestions are welcome >>> >>>> >>>> In lib/trace.c consider replacing >>>> "__attribute__((no_instrument_function))" by "notrace". >>>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> >>>>> 2) if gd->timer is already initialized then still initr_dm() will >>>>> make >>>>> gd->timer = NULL; >>>>> >>>>> initr_dm() >>>>> { >>>>> #ifdef CONFIG_TIMER >>>>> gd->timer = NULL; >>>>> #endif >>>>> } >>>>> >>>>> So again dm_timer_init() gets called and tracing got stuck. >>>>> >>>>> So I thought better to implement timer_get_us(). >>>>> >>>>>> >>>>>> Best regards >>>>>> >>>>>> Heinrich >>>>>> >>>>>>> --- >>>>>>> >>>>>>> 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 >>>>>>> + 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); } >>>>>>> >>>>>>> >>>>> >>> >>
>-----Original Message----- >From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Pragnesh Patel >Sent: 12 November 2020 13:06 >To: Heinrich Schuchardt <xypron.glpk@gmx.de>; Simon Glass ><sjg@chromium.org> >Cc: atish.patra@wdc.com; palmerdabbelt@google.com; u-boot@lists.denx.de; >bmeng.cn@gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; >anup.patel@wdc.com; Sagar Kadam <sagar.kadam@openfive.com>; >rick@andestech.com; Sean Anderson <seanga2@gmail.com> >Subject: RE: [PATCH v3 1/1] riscv: Add timer_get_us() for tracing > >Hi Heinrich, > >>-----Original Message----- >>From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>Sent: 12 November 2020 12:49 >>To: Pragnesh Patel <pragnesh.patel@openfive.com>; Simon Glass >><sjg@chromium.org> >>Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>u-boot@lists.denx.de; bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >><paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >><sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >><seanga2@gmail.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 11/12/20 8:09 AM, Heinrich Schuchardt wrote: >>> On 11/12/20 7:34 AM, Pragnesh Patel wrote: >>>> Hi Heinrich, >>>> >>>>> -----Original Message----- >>>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>> Sent: 11 November 2020 19:18 >>>>> To: Pragnesh Patel <pragnesh.patel@openfive.com> >>>>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>>>> u-boot@lists.denx.de; bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >>>>> <paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>>>> <sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >>>>> <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >>>>> 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 11/11/20 12:56 PM, Pragnesh Patel wrote: >>>>>> Hi Heinrich, >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>>>> Sent: 11 November 2020 16:51 >>>>>>> To: Pragnesh Patel <pragnesh.patel@openfive.com>; >>>>>>> u-boot@lists.denx.de >>>>>>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>>>>>> bmeng.cn@gmail.com; Paul Walmsley ( Sifive) >>>>>>> <paul.walmsley@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>>>>>> <sagar.kadam@openfive.com>; rick@andestech.com; Sean Anderson >>>>>>> <seanga2@gmail.com>; Simon Glass <sjg@chromium.org> >>>>>>> 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 11.11.20 11:14, Pragnesh Patel wrote: >>>>>>>> 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.patel@sifive.com> >>>>>>> >>>>>>> The default implementation of get_timer_us() in lib/time.c calls >>>>>>> get_ticks() which calls timer_get_count(). The get_count() >>>>>>> operation is implemented in drivers/timer/andes_plmt_timer.c, >>>>>>> drivers/timer/sifive_clint_timer.c, drivers/timer/riscv_timer.c. >>>>>>> >>>>>>> Why do you need special timer_get_us() implementations? >>>>>>> Isn't it enough to supply the get_count() operation in the drivers? >>>>>> >>>>>> get_ticks() is depend on gd->timer and there are 2 cases >>>>>> >>>>>> 1) if gd->timer== NULL then dm_timer_init() gets called and it >>>>>> will call functions which are not marked with "notrace" so tracing got >stuck. >>>>> >>>>> Thanks for the background information. >>>>> >>>>> Please, identify the existing functions that lack "notrace" and fix >>>>> them. This will give us a solution for all existing boards and not >>>>> for a small >>selection. >>>>> Furthermore it will avoid code duplication. >>>> >>>> I tried but There are so many functions which need "notrace". >>> >>> Let's start with the problem statement: >>> >>> When tracing functions is enabled this adds calls to >>> __cyg_profile_func_enter() and __cyg_profile_func_exit() to the >>> traced functions. >>> >>> __cyg_profile_func_exit() and __cyg_profile_func_exit() invoke >>> timer_get_us() to record the entry and exit time. >>> >>> If timer_get_us() or any function used to implement does not carry >>> __attribute__((no_instrument_function)) this will lead to an >>> indefinite recursion. >>> >>> We have to change __cyg_profile_func_enter() and >>> __cyg_profile_func_exit() such that during their execution no >>> function is traced. We can do so by temporarily setting trace_enabled to false. >>> >>> Does this match your observation? >> >>I just tried to put this into a patch >> >>[PATCH 1/1] trace: avoid infinite recursion >>https://lists.denx.de/pipermail/u- >>boot/2020-November/432589.html >>https://patchwork.ozlabs.org/project/uboot/patch/20201112071411.4202-1- >>xypron.glpk@gmx.de/ >> >>Does this solve you problem? > >Getting error, "Could not initialize timer (err -11)" from get_ticks() function. > >Below are my boot logs, > >U-Boot 2021.01-rc1-00244-g794c155581-dirty (Nov 12 2020 - 12:56:27 +0530) > >CPU: rv64imafdc >Model: SiFive HiFive Unleashed A00 >DRAM: 8 GiB >trace: enabled >Could not initialize timer (err -11) > >Could not initialize timer (err -11) > >Could not initialize timer (err -11) > >Could not initialize timer (err -11) With Further debugging, this is because gd->dm_root is equal to NULL by initr_dm(). dm_timer_init() will fail if gd->dm_root == NULL. int notrace dm_timer_init(void) { if (gd->dm_root == NULL) return -EAGAIN; } So the solution is to call initr_dm() before initr_trace() so that gd->dm_root will become available. I will send a patch to do the same. > > >> >>Best regards >> >>Heinrich >>> >>>> >>>> Why don’t we consider removing gd->timer=NULL in initr_dm() >>>> initr_dm() >>>> { >>>> #ifdef CONFIG_TIMER >>>> gd->timer = NULL; >>>> #endif >>>> } >>>> >>>> Or I can use TIMER_EARLY and return ticks and rate by adding >>>> timer_early_get_count() and timer_early_get_rate() repectively. >>>> >>>> Suggestions are welcome >>>> >>>>> >>>>> In lib/trace.c consider replacing >>>>> "__attribute__((no_instrument_function))" by "notrace". >>>>> >>>>> Best regards >>>>> >>>>> Heinrich >>>>> >>>>>> >>>>>> 2) if gd->timer is already initialized then still initr_dm() will >>>>>> make >>>>>> gd->timer = NULL; >>>>>> >>>>>> initr_dm() >>>>>> { >>>>>> #ifdef CONFIG_TIMER >>>>>> gd->timer = NULL; >>>>>> #endif >>>>>> } >>>>>> >>>>>> So again dm_timer_init() gets called and tracing got stuck. >>>>>> >>>>>> So I thought better to implement timer_get_us(). >>>>>> >>>>>>> >>>>>>> Best regards >>>>>>> >>>>>>> Heinrich >>>>>>> >>>>>>>> --- >>>>>>>> >>>>>>>> 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 >>>>>>>> + 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); } >>>>>>>> >>>>>>>> >>>>>> >>>> >>>
Hi Pragnesh > From: Pragnesh Patel [mailto:pragnesh.patel@sifive.com] > Sent: Wednesday, November 11, 2020 6:15 PM > To: u-boot@lists.denx.de > Cc: atish.patra@wdc.com; palmerdabbelt@google.com; bmeng.cn@gmail.com; paul.walmsley@sifive.com; anup.patel@wdc.com; sagar.kadam@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.patel@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)); 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 also found that without dm_timer_init() in initf_dm(), andes_plmt_get_count() will be executed ahead of andes_plmt_probe(). 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
Hi Rick, >-----Original Message----- >From: Rick Chen <rickchen36@gmail.com> >Sent: 13 November 2020 13:37 >To: Pragnesh Patel <pragnesh.patel@openfive.com> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra ><atish.patra@wdc.com>; palmerdabbelt@google.com; Bin Meng ><bmeng.cn@gmail.com>; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; >Anup Patel <anup.patel@wdc.com>; Sagar Kadam ><sagar.kadam@openfive.com>; Sean Anderson <seanga2@gmail.com>; rick ><rick@andestech.com>; Alan Kao <alankao@andestech.com>; Leo Liang ><ycliang@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.patel@sifive.com] >> Sent: Wednesday, November 11, 2020 6:15 PM >> To: u-boot@lists.denx.de >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >bmeng.cn@gmail.com; >> paul.walmsley@sifive.com; anup.patel@wdc.com; sagar.kadam@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.patel@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)); No I did not get this. Why are you getting this error because plmt is already defined ? > >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 also found that without dm_timer_init() in initf_dm(), >andes_plmt_get_count() will be executed ahead of andes_plmt_probe(). Thanks Rick but me and Heinrich are working on different approach to use existing dm_timer_init() from lib/time.c https://patchwork.ozlabs.org/project/uboot/patch/20201112071411.4202-1-xypron.glpk@gmx.de/ > >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
Hi Pragnesh > Hi Rick, > > >-----Original Message----- > >From: Rick Chen <rickchen36@gmail.com> > >Sent: 13 November 2020 13:37 > >To: Pragnesh Patel <pragnesh.patel@openfive.com> > >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra > ><atish.patra@wdc.com>; palmerdabbelt@google.com; Bin Meng > ><bmeng.cn@gmail.com>; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; > >Anup Patel <anup.patel@wdc.com>; Sagar Kadam > ><sagar.kadam@openfive.com>; Sean Anderson <seanga2@gmail.com>; rick > ><rick@andestech.com>; Alan Kao <alankao@andestech.com>; Leo Liang > ><ycliang@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.patel@sifive.com] > >> Sent: Wednesday, November 11, 2020 6:15 PM > >> To: u-boot@lists.denx.de > >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; > >bmeng.cn@gmail.com; > >> paul.walmsley@sifive.com; anup.patel@wdc.com; sagar.kadam@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.patel@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: Sorry, it's shall be CONFIG_ANDES_PLMT_TIMER here // because of obj-$(CONFIG_ANDES_PLMT_TIMER) += andes_plmt_timer.o Or it will have a compiling error. > > > >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)); > > No I did not get this. Why are you getting this error because plmt is already defined ? > > > > >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 also found that without dm_timer_init() in initf_dm(), > >andes_plmt_get_count() will be executed ahead of andes_plmt_probe(). > > Thanks Rick but me and Heinrich are working on different approach to use existing dm_timer_init() from lib/time.c > > https://patchwork.ozlabs.org/project/uboot/patch/20201112071411.4202-1-xypron.glpk@gmx.de/ OK Thanks, Rick > > > > >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
Hi Rick, >-----Original Message----- >From: Rick Chen <rickchen36@gmail.com> >Sent: 13 November 2020 13:37 >To: Pragnesh Patel <pragnesh.patel@openfive.com> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra ><atish.patra@wdc.com>; palmerdabbelt@google.com; Bin Meng ><bmeng.cn@gmail.com>; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; >Anup Patel <anup.patel@wdc.com>; Sagar Kadam ><sagar.kadam@openfive.com>; Sean Anderson <seanga2@gmail.com>; rick ><rick@andestech.com>; Alan Kao <alankao@andestech.com>; Leo Liang ><ycliang@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.patel@sifive.com] >> Sent: Wednesday, November 11, 2020 6:15 PM >> To: u-boot@lists.denx.de >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >bmeng.cn@gmail.com; >> paul.walmsley@sifive.com; anup.patel@wdc.com; sagar.kadam@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.patel@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)); > >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
On Tue, Nov 17, 2020 at 08:30:21AM +0000, Pragnesh Patel wrote: Hi Pragnesh, > Hi Rick, > > >-----Original Message----- > >From: Rick Chen <rickchen36@gmail.com> > >Sent: 13 November 2020 13:37 > >To: Pragnesh Patel <pragnesh.patel@openfive.com> > >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra > ><atish.patra@wdc.com>; palmerdabbelt@google.com; Bin Meng > ><bmeng.cn@gmail.com>; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; > >Anup Patel <anup.patel@wdc.com>; Sagar Kadam > ><sagar.kadam@openfive.com>; Sean Anderson <seanga2@gmail.com>; rick > ><rick@andestech.com>; Alan Kao <alankao@andestech.com>; Leo Liang > ><ycliang@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.patel@sifive.com] > >> Sent: Wednesday, November 11, 2020 6:15 PM > >> To: u-boot@lists.denx.de > >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; > >bmeng.cn@gmail.com; > >> paul.walmsley@sifive.com; anup.patel@wdc.com; sagar.kadam@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.patel@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. 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
Hi Leo, >-----Original Message----- >From: Leo Liang <ycliang@andestech.com> >Sent: 23 November 2020 11:28 >To: Pragnesh Patel <pragnesh.patel@openfive.com> >Cc: Rick Chen <rickchen36@gmail.com>; U-Boot Mailing List <u- >boot@lists.denx.de>; Atish Patra <atish.patra@wdc.com>; >palmerdabbelt@google.com; Bin Meng <bmeng.cn@gmail.com>; Paul Walmsley >( Sifive) <paul.walmsley@sifive.com>; Anup Patel <anup.patel@wdc.com>; Sagar >Kadam <sagar.kadam@openfive.com>; Sean Anderson <seanga2@gmail.com>; >rick <rick@andestech.com>; Alan Kao <alankao@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 <rickchen36@gmail.com> >> >Sent: 13 November 2020 13:37 >> >To: Pragnesh Patel <pragnesh.patel@openfive.com> >> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra >> ><atish.patra@wdc.com>; palmerdabbelt@google.com; Bin Meng >> ><bmeng.cn@gmail.com>; Paul Walmsley ( Sifive) >> ><paul.walmsley@sifive.com>; Anup Patel <anup.patel@wdc.com>; Sagar >> >Kadam <sagar.kadam@openfive.com>; Sean Anderson ><seanga2@gmail.com>; >> >rick <rick@andestech.com>; Alan Kao <alankao@andestech.com>; Leo >> >Liang <ycliang@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.patel@sifive.com] >> >> Sent: Wednesday, November 11, 2020 6:15 PM >> >> To: u-boot@lists.denx.de >> >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >> >bmeng.cn@gmail.com; >> >> paul.walmsley@sifive.com; anup.patel@wdc.com; >> >> sagar.kadam@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.patel@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.patel@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
Hi Pragnesh, On Mon, Nov 23, 2020 at 06:19:06AM +0000, Pragnesh Patel wrote: > Hi Leo, > > >-----Original Message----- > >From: Leo Liang <ycliang@andestech.com> > >Sent: 23 November 2020 11:28 > >To: Pragnesh Patel <pragnesh.patel@openfive.com> > >Cc: Rick Chen <rickchen36@gmail.com>; U-Boot Mailing List <u- > >boot@lists.denx.de>; Atish Patra <atish.patra@wdc.com>; > >palmerdabbelt@google.com; Bin Meng <bmeng.cn@gmail.com>; Paul Walmsley > >( Sifive) <paul.walmsley@sifive.com>; Anup Patel <anup.patel@wdc.com>; Sagar > >Kadam <sagar.kadam@openfive.com>; Sean Anderson <seanga2@gmail.com>; > >rick <rick@andestech.com>; Alan Kao <alankao@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 <rickchen36@gmail.com> > >> >Sent: 13 November 2020 13:37 > >> >To: Pragnesh Patel <pragnesh.patel@openfive.com> > >> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra > >> ><atish.patra@wdc.com>; palmerdabbelt@google.com; Bin Meng > >> ><bmeng.cn@gmail.com>; Paul Walmsley ( Sifive) > >> ><paul.walmsley@sifive.com>; Anup Patel <anup.patel@wdc.com>; Sagar > >> >Kadam <sagar.kadam@openfive.com>; Sean Anderson > ><seanga2@gmail.com>; > >> >rick <rick@andestech.com>; Alan Kao <alankao@andestech.com>; Leo > >> >Liang <ycliang@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.patel@sifive.com] > >> >> Sent: Wednesday, November 11, 2020 6:15 PM > >> >> To: u-boot@lists.denx.de > >> >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; > >> >bmeng.cn@gmail.com; > >> >> paul.walmsley@sifive.com; anup.patel@wdc.com; > >> >> sagar.kadam@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.patel@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.patel@sifive.com/ > Got it! Thanks for the explanation, Best regards, Leo > > > >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
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 + 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); }
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.patel@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(-)