[PATCH v3 1/1] riscv: Add timer_get_us() for tracing
Pragnesh Patel
pragnesh.patel at openfive.com
Thu Nov 12 08:35:58 CET 2020
Hi Heinrich,
>-----Original Message-----
>From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>Sent: 12 November 2020 12:49
>To: Pragnesh Patel <pragnesh.patel at openfive.com>; Simon Glass
><sjg at chromium.org>
>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 at sifive.com>;
>anup.patel at wdc.com; Sagar Kadam <sagar.kadam at openfive.com>;
>rick at andestech.com; Sean Anderson <seanga2 at 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.glpk at gmx.de>
>>>> Sent: 11 November 2020 19:18
>>>> To: Pragnesh Patel <pragnesh.patel at 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 at sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>> <sagar.kadam at openfive.com>; rick at andestech.com; Sean Anderson
>>>> <seanga2 at gmail.com>; Simon Glass <sjg at 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 at gmx.de>
>>>>>> Sent: 11 November 2020 16:51
>>>>>> To: Pragnesh Patel <pragnesh.patel at 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 at sifive.com>; anup.patel at wdc.com; Sagar Kadam
>>>>>> <sagar.kadam at openfive.com>; rick at andestech.com; Sean Anderson
>>>>>> <seanga2 at gmail.com>; Simon Glass <sjg at 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 at 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.glpk at 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)
>
>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); }
>>>>>>>
>>>>>>>
>>>>>
>>>
>>
More information about the U-Boot
mailing list