[PATCH v3 0/1] RISC-V tracing support

[PATCH v3 0/1] RISC-V tracing support

[Pragnesh Patel]

This series add a support of tracing for RISC-V arch.

This series is also available here [1] for testing.
[1] https://github.com/pragnesh26992/u-boot/tree/trace

How to test this patch:
1) Enable tracing in "configs/sifive_fu540_defconfig"
CONFIG_TRACE=y
CONFIG_TRACE_BUFFER_SIZE=0x01000000
CONFIG_TRACE_CALL_DEPTH_LIMIT=15
CONFIG_CMD_TRACE=y

2) make FTRACE=1 sifive_fu540_defconfig
3) make FTRACE=1

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:
- Remove newly added timer file (arch/riscv/lib/timer.c)
- Added timer_get_us() in riscv_timer.c, sifive_clint_timer.c
  and andes_plmt_timer.c.


Following are the boot messages on FU540 five cores SMP platform:

U-Boot SPL 2021.01-rc1-00244-g88b5af756c-dirty (Nov 11 2020 - 14:57:25 +0530)
Trying to boot from MMC1


U-Boot 2021.01-rc1-00244-g88b5af756c-dirty (Nov 11 2020 - 14:57:25 +0530)

CPU:   rv64imafdc
Model: SiFive HiFive Unleashed A00
DRAM:  8 GiB
trace: enabled
MMC:   spi at 10050000:mmc at 0: 0
*** Warning - bad CRC, using default environment

In:    serial at 10010000
Out:   serial at 10010000
Err:   serial at 10010000
Board serial number should not be 0 !!
Net:
Error: ethernet at 10090000 address not set.
No ethernet found.

Hit any key to stop autoboot:  0
=> trace stats
        178,556 function sites
     15,443,168 function calls
              1 untracked function calls
      1,279,056 traced function calls (14135744 dropped due to overflow)
             19 maximum observed call depth
             15 call depth limit
     15,633,052 calls not traced due to depth
=>


Pragnesh Patel (1):
  riscv: Add timer_get_us() for tracing

 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(-)

-- 
2.17.1

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

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
+	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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Heinrich Schuchardt]

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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

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 at lists.denx.de
>Cc: atish.patra at wdc.com; palmerdabbelt at google.com; bmeng.cn at gmail.com;
>Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; anup.patel at wdc.com;
>Sagar Kadam <sagar.kadam@openfive.com>; rick at 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);  }
>>
>>

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Heinrich Schuchardt]

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 at lists.denx.de
>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com; bmeng.cn at gmail.com;
>> Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>; anup.patel at wdc.com;
>> Sagar Kadam <sagar.kadam@openfive.com>; rick at 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);  }
>>>
>>>
>

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

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 at wdc.com; palmerdabbelt at google.com; u-boot at lists.denx.de;
>bmeng.cn at gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
>anup.patel at wdc.com; Sagar Kadam <sagar.kadam@openfive.com>;
>rick at 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 at lists.denx.de
>>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>>> <paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>> <sagar.kadam@openfive.com>; rick at 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);  }
>>>>
>>>>
>>

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Heinrich Schuchardt]

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 at wdc.com; palmerdabbelt at google.com; u-boot at lists.denx.de;
>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
>> anup.patel at wdc.com; Sagar Kadam <sagar.kadam@openfive.com>;
>> rick at 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 at lists.denx.de
>>>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>>>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>>>> <paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>> <sagar.kadam@openfive.com>; rick at 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);  }
>>>>>
>>>>>
>>>
>

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Heinrich Schuchardt]

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 at wdc.com; palmerdabbelt at google.com; u-boot at lists.denx.de;
>>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
>>> anup.patel at wdc.com; Sagar Kadam <sagar.kadam@openfive.com>;
>>> rick at 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 at lists.denx.de
>>>>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>>>>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>>>>> <paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>>> <sagar.kadam@openfive.com>; rick at 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 at 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);  }
>>>>>>
>>>>>>
>>>>
>>
>

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

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 at wdc.com; palmerdabbelt at google.com; u-boot at lists.denx.de;
>bmeng.cn at gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
>anup.patel at wdc.com; Sagar Kadam <sagar.kadam@openfive.com>;
>rick at 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 at wdc.com; palmerdabbelt at google.com;
>>>> u-boot at lists.denx.de; bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>>>> <paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>> <sagar.kadam@openfive.com>; rick at 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 at lists.denx.de
>>>>>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>>>>>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>>>>>> <paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>>>> <sagar.kadam@openfive.com>; rick at 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 at 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);  }
>>>>>>>
>>>>>>>
>>>>>
>>>
>>

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

>-----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 at wdc.com; palmerdabbelt at google.com; u-boot at lists.denx.de;
>bmeng.cn at gmail.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
>anup.patel at wdc.com; Sagar Kadam <sagar.kadam@openfive.com>;
>rick at 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 at wdc.com; palmerdabbelt at google.com;
>>u-boot at lists.denx.de; bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>><paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>><sagar.kadam@openfive.com>; rick at 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 at wdc.com; palmerdabbelt at google.com;
>>>>> u-boot at lists.denx.de; bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>>>>> <paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>>> <sagar.kadam@openfive.com>; rick at 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 at lists.denx.de
>>>>>>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>>>>>>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive)
>>>>>>> <paul.walmsley@sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>>>>> <sagar.kadam@openfive.com>; rick at 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 at 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);  }
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Rick Chen]

Hi Pragnesh

> From: Pragnesh Patel [mailto:pragnesh.patel at sifive.com]
> Sent: Wednesday, November 11, 2020 6:15 PM
> To: u-boot at lists.denx.de
> Cc: atish.patra at wdc.com; palmerdabbelt at google.com; bmeng.cn at gmail.com; paul.walmsley at sifive.com; anup.patel at wdc.com; sagar.kadam at 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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

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 at 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 at sifive.com]
>> Sent: Wednesday, November 11, 2020 6:15 PM
>> To: u-boot at lists.denx.de
>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>bmeng.cn at gmail.com;
>> paul.walmsley at sifive.com; anup.patel at wdc.com; sagar.kadam at 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 at 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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Rick Chen]

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 at 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 at sifive.com]
> >> Sent: Wednesday, November 11, 2020 6:15 PM
> >> To: u-boot at lists.denx.de
> >> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
> >bmeng.cn at gmail.com;
> >> paul.walmsley at sifive.com; anup.patel at wdc.com; sagar.kadam at 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 at 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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

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 at 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 at sifive.com]
>> Sent: Wednesday, November 11, 2020 6:15 PM
>> To: u-boot at lists.denx.de
>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>bmeng.cn at gmail.com;
>> paul.walmsley at sifive.com; anup.patel at wdc.com; sagar.kadam at 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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Leo Liang]

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 at 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 at sifive.com]
> >> Sent: Wednesday, November 11, 2020 6:15 PM
> >> To: u-boot at lists.denx.de
> >> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
> >bmeng.cn at gmail.com;
> >> paul.walmsley at sifive.com; anup.patel at wdc.com; sagar.kadam at 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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Pragnesh Patel]

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 at lists.denx.de>; Atish Patra <atish.patra@wdc.com>;
>palmerdabbelt at 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 at 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 at sifive.com]
>> >> Sent: Wednesday, November 11, 2020 6:15 PM
>> >> To: u-boot at lists.denx.de
>> >> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>> >bmeng.cn at gmail.com;
>> >> paul.walmsley at sifive.com; anup.patel at wdc.com;
>> >> sagar.kadam at 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 at 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

[PATCH v3 1/1] riscv: Add timer_get_us() for tracing

[Leo Liang]

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 at lists.denx.de>; Atish Patra <atish.patra@wdc.com>;
> >palmerdabbelt at 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 at 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 at sifive.com]
> >> >> Sent: Wednesday, November 11, 2020 6:15 PM
> >> >> To: u-boot at lists.denx.de
> >> >> Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
> >> >bmeng.cn at gmail.com;
> >> >> paul.walmsley at sifive.com; anup.patel at wdc.com;
> >> >> sagar.kadam at 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 at 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