Hi Rick,
>-----Original Message-----
>From: Rick Chen <rickche...@gmail.com>
>Sent: 10 November 2020 13:17
>To: Pragnesh Patel <pragnesh.pa...@openfive.com>
>Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra
><atish.pa...@wdc.com>; Bin Meng <bmeng...@gmail.com>; Paul Walmsley (
>Sifive) <paul.walms...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; Sagar
>Kadam <sagar.ka...@openfive.com>; Simon Glass <s...@chromium.org>; Sean
>Anderson <sean...@gmail.com>; palmerdabb...@google.com; rick
><r...@andestech.com>; Alan Kao <alan...@andestech.com>
>Subject: Re: [RESEND,PATCH v2 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
>
>> Hi Rick,
>>
>> >-----Original Message-----
>> >From: Rick Chen <rickche...@gmail.com>
>> >Sent: 09 November 2020 13:44
>> >To: Pragnesh Patel <pragnesh.pa...@openfive.com>
>> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra
>> ><atish.pa...@wdc.com>; Bin Meng <bmeng...@gmail.com>; Paul Walmsley (
>> >Sifive) <paul.walms...@sifive.com>; Anup Patel <anup.pa...@wdc.com>;
>> >Sagar Kadam <sagar.ka...@openfive.com>; Simon Glass
>> ><s...@chromium.org>; Sean Anderson <sean...@gmail.com>;
>> >palmerdabb...@google.com; rick <r...@andestech.com>; Alan Kao
>> ><alan...@andestech.com>
>> >Subject: Re: [RESEND,PATCH v2 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
>> >
>> >> From: Pragnesh Patel [mailto:pragnesh.pa...@sifive.com]
>> >> Sent: Thursday, November 05, 2020 7:31 PM
>> >> To: u-boot@lists.denx.de
>> >> Cc: atish.pa...@wdc.com; palmerdabb...@google.com;
>> >bmeng...@gmail.com;
>> >> paul.walms...@sifive.com; anup.pa...@wdc.com;
>> >> sagar.ka...@sifive.com; Rick Jian-Zhi Chen(陳建志); Pragnesh Patel;
>> >> Sean Anderson; Claudiu Beznea; Simon Glass
>> >> Subject: [RESEND,PATCH v2 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.pa...@sifive.com>
>> >> ---
>> >> drivers/timer/andes_plmt_timer.c | 16 +++++++++++++++-
>> >> drivers/timer/riscv_timer.c | 14 +++++++++++++-
>> >> drivers/timer/sifive_clint_timer.c | 16 +++++++++++++++-
>> >> 3 files changed, 43 insertions(+), 3 deletions(-)
>> >>
>> >
>> >I verify it fail as below:
>> >
>> >U-Boot 2020.10-20532-g0910882 (Nov 09 2020 - 15:51:31 +0800)
>> >
>> >DRAM: 1 GiB
>> >trace: enabled
>> >
>> >Do you have any suggestion ?
>>
>> On which platform ?
>>
>> Do you follow this
>https://patchwork.ozlabs.org/project/uboot/cover/20201105113032.26234-1-
>pragnesh.pa...@sifive.com/ ?
>>
>
>I verify on AE350 platform with the following configurations:
>
>CONFIG_TRACE=y
>CONFIG_TRACE_BUFFER_SIZE=0x01000000
>CONFIG_TRACE_CALL_DEPTH_LIMIT=15
>CONFIG_CMD_TRACE=y
>
>make FTRACE=1 ae350_rv64_defconfig
>make FTRACE=1
>
>After dig in and I found the root cause.
>You can't use gd->arch.plic to record plmt base.
>It conflicts with andes plic.
Ohhhh nice catch, will update in v3.
>
>Thanks,
>Rick
>
>> >
>> >Thanks,
>> >Rick
>> >
>> >
>> >> diff --git a/drivers/timer/andes_plmt_timer.c
>> >> b/drivers/timer/andes_plmt_timer.c
>> >> index cec86718c7..9d663e036e 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,25 @@ 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.plic should contain valid base address */
Any comment here for FIXME ?
>> >> + ticks = andes_plmt_get_count(gd->arch.plic);
>>
>> Here andes_plmt_get_count() should be replaced with MTIME_REG() macro.
>Will update in v3.
>>
>> >> + 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.plic = 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..166655e99d 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,25 @@ 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 */
>> >> + ticks = sifive_clint_get_count(gd->arch.clint);
>>
>> Here sifive_clint_get_count () should be replaced with MTIME_REG() macro.
>Will update in v3.
>>
>> >> + 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
>> >>
