[PATCH v3 1/1] riscv: Add timer_get_us() for tracing
Pragnesh Patel
pragnesh.patel at openfive.com
Mon Nov 23 07:19:06 CET 2020
Hi Leo,
>-----Original Message-----
>From: Leo Liang <ycliang at andestech.com>
>Sent: 23 November 2020 11:28
>To: Pragnesh Patel <pragnesh.patel at openfive.com>
>Cc: Rick Chen <rickchen36 at gmail.com>; U-Boot Mailing List <u-
>boot at lists.denx.de>; Atish Patra <atish.patra at wdc.com>;
>palmerdabbelt at google.com; Bin Meng <bmeng.cn at gmail.com>; Paul Walmsley
>( Sifive) <paul.walmsley at sifive.com>; Anup Patel <anup.patel at wdc.com>; Sagar
>Kadam <sagar.kadam at openfive.com>; Sean Anderson <seanga2 at gmail.com>;
>rick <rick at andestech.com>; Alan Kao <alankao at 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 at gmail.com>
>> >Sent: 13 November 2020 13:37
>> >To: Pragnesh Patel <pragnesh.patel at openfive.com>
>> >Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Atish Patra
>> ><atish.patra at wdc.com>; palmerdabbelt at google.com; Bin Meng
>> ><bmeng.cn at gmail.com>; Paul Walmsley ( Sifive)
>> ><paul.walmsley at sifive.com>; Anup Patel <anup.patel at wdc.com>; Sagar
>> >Kadam <sagar.kadam at openfive.com>; Sean Anderson
><seanga2 at gmail.com>;
>> >rick <rick at andestech.com>; Alan Kao <alankao at andestech.com>; Leo
>> >Liang <ycliang at 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 at 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@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
More information about the U-Boot
mailing list