On 11/12/20 7:34 AM, Pragnesh Patel wrote:
> Hi Heinrich,
>
>> -----Original Message-----
>> From: Heinrich Schuchardt <xypron.g...@gmx.de>
>> Sent: 11 November 2020 19:18
>> To: Pragnesh Patel <pragnesh.pa...@openfive.com>
>> Cc: atish.pa...@wdc.com; palmerdabb...@google.com; u-boot@lists.denx.de;
>> bmeng...@gmail.com; Paul Walmsley ( Sifive) <paul.walms...@sifive.com>;
>> anup.pa...@wdc.com; Sagar Kadam <sagar.ka...@openfive.com>;
>> r...@andestech.com; Sean Anderson <sean...@gmail.com>; Simon Glass
>> <s...@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.g...@gmx.de>
>>>> Sent: 11 November 2020 16:51
>>>> To: Pragnesh Patel <pragnesh.pa...@openfive.com>;
>>>> u-boot@lists.denx.de
>>>> Cc: atish.pa...@wdc.com; palmerdabb...@google.com;
>>>> bmeng...@gmail.com; Paul Walmsley ( Sifive)
>>>> <paul.walms...@sifive.com>; anup.pa...@wdc.com; Sagar Kadam
>>>> <sagar.ka...@openfive.com>; r...@andestech.com; Sean Anderson
>>>> <sean...@gmail.com>; Simon Glass <s...@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.pa...@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); }
>>>>>
>>>>>
>>>
>
