>-----Original Message-----
>From: U-Boot <u-boot-boun...@lists.denx.de> On Behalf Of Pragnesh Patel
>Sent: 12 November 2020 13:06
>To: Heinrich Schuchardt <xypron.g...@gmx.de>; Simon Glass
><s...@chromium.org>
>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>
>Subject: RE: [PATCH v3 1/1] riscv: Add timer_get_us() for tracing
>
>Hi Heinrich,
>
>>-----Original Message-----
>>From: Heinrich Schuchardt <xypron.g...@gmx.de>
>>Sent: 12 November 2020 12:49
>>To: Pragnesh Patel <pragnesh.pa...@openfive.com>; Simon Glass
>><s...@chromium.org>
>>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>
>>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/12/20 8:09 AM, Heinrich Schuchardt wrote:
>>> 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?
>>
>>I just tried to put this into a patch
>>
>>[PATCH 1/1] trace: avoid infinite recursion
>>https://lists.denx.de/pipermail/u-
>>boot/2020-November/432589.html
>>https://patchwork.ozlabs.org/project/uboot/patch/20201112071411.4202-1-
>>xypron.g...@gmx.de/
>>
>>Does this solve you problem?
>
>Getting error, "Could not initialize timer (err -11)" from get_ticks()
>function.
>
>Below are my boot logs,
>
>U-Boot 2021.01-rc1-00244-g794c155581-dirty (Nov 12 2020 - 12:56:27 +0530)
>
>CPU: rv64imafdc
>Model: SiFive HiFive Unleashed A00
>DRAM: 8 GiB
>trace: enabled
>Could not initialize timer (err -11)
>
>Could not initialize timer (err -11)
>
>Could not initialize timer (err -11)
>
>Could not initialize timer (err -11)
With Further debugging, this is because gd->dm_root is equal to NULL by
initr_dm().
dm_timer_init() will fail if gd->dm_root == NULL.
int notrace dm_timer_init(void)
{
if (gd->dm_root == NULL)
return -EAGAIN;
}
So the solution is to call initr_dm() before initr_trace() so that gd->dm_root
will become available. I will send a patch
to do the same.
>
>
>>
>>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); }
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
