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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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