From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Thu, 12 Nov 2020 08:19:23 +0100 Subject: [PATCH v3 1/1] riscv: Add timer_get_us() for tracing In-Reply-To: <76f1859c-5657-423c-e4ba-d2ef1ef36207@gmx.de> References: <20201111101435.31455-1-pragnesh.patel@sifive.com> <20201111101435.31455-2-pragnesh.patel@sifive.com> <5cec2764-ee9b-c5eb-b54d-35f0d061f097@gmx.de> <7af38364-6585-4bbc-211d-4205a7f6bb2e@gmx.de> <76f1859c-5657-423c-e4ba-d2ef1ef36207@gmx.de> Message-ID: <079137ab-e3d8-4164-4200-ce49ca505930@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> Sent: 11 November 2020 19:18 >>> To: Pragnesh Patel >>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com; u-boot at lists.denx.de; >>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive) ; >>> anup.patel at wdc.com; Sagar Kadam ; >>> rick at andestech.com; Sean Anderson ; Simon Glass >>> >>> 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 >>>>> Sent: 11 November 2020 16:51 >>>>> To: Pragnesh Patel ; >>>>> u-boot at lists.denx.de >>>>> Cc: atish.patra at wdc.com; palmerdabbelt at google.com; >>>>> bmeng.cn at gmail.com; Paul Walmsley ( Sifive) >>>>> ; anup.patel at wdc.com; Sagar Kadam >>>>> ; rick at andestech.com; Sean Anderson >>>>> ; Simon Glass >>>>> 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 >>>>> >>>>> 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? 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> >>>>>> /* 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> >>>>>> -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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> >>>>>> /* 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); } >>>>>> >>>>>> >>>> >> >