Hi Heinrich, >-----Original Message----- >From: Heinrich Schuchardt <xypron.g...@gmx.de> >Sent: 11 November 2020 19:18 >To: Pragnesh Patel <pragnesh.pa...@openfive.com> >Cc: atish.pa...@wdc.com; palmerdabb...@google.com; u-boot@lists.denx.de; >bmeng...@gmail.com; Paul Walmsley ( Sifive) <paul.walms...@sifive.com>; >anup.pa...@wdc.com; Sagar Kadam <sagar.ka...@openfive.com>; >r...@andestech.com; Sean Anderson <sean...@gmail.com>; Simon Glass ><s...@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.g...@gmx.de> >>> Sent: 11 November 2020 16:51 >>> To: Pragnesh Patel <pragnesh.pa...@openfive.com>; >>> u-boot@lists.denx.de >>> Cc: atish.pa...@wdc.com; palmerdabb...@google.com; >>> bmeng...@gmail.com; Paul Walmsley ( Sifive) >>> <paul.walms...@sifive.com>; anup.pa...@wdc.com; Sagar Kadam >>> <sagar.ka...@openfive.com>; r...@andestech.com; Sean Anderson >>> <sean...@gmail.com>; Simon Glass <s...@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.pa...@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); } >>>> >>>> >>