From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Wed, 11 Nov 2020 14:48:08 +0100 Subject: [PATCH v3 1/1] riscv: Add timer_get_us() for tracing In-Reply-To: References: <20201111101435.31455-1-pragnesh.patel@sifive.com> <20201111101435.31455-2-pragnesh.patel@sifive.com> <5cec2764-ee9b-c5eb-b54d-35f0d061f097@gmx.de> Message-ID: <7af38364-6585-4bbc-211d-4205a7f6bb2e@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 11/11/20 12:56 PM, Pragnesh Patel wrote: > Hi Heinrich, > >> -----Original Message----- >> From: Heinrich Schuchardt >> Sent: 11 November 2020 16:51 >> To: Pragnesh Patel ; u-boot at lists.denx.de >> Cc: atish.patra at wdc.com; palmerdabbelt at google.com; bmeng.cn at gmail.com; >> Paul Walmsley ( Sifive) ; anup.patel at wdc.com; >> Sagar Kadam ; rick at andestech.com; Sean >> Anderson ; Simon Glass >> 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 >> >> 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 >>> #include >>> #include >>> +#include >>> >>> /* 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 >>> #include >>> #include >>> +#include >>> >>> -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 >>> #include >>> #include >>> +#include >>> >>> /* 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); } >>> >>> >