[PATCH v3 1/1] riscv: Add timer_get_us() for tracing
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Nov 12 08:19:23 CET 2020
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@gmx.de/
Does this solve you problem?
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