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

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

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