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
- 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-xypro...
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
- 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); }