From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pragnesh Patel Date: Thu, 12 Nov 2020 07:35:58 +0000 Subject: [PATCH v3 1/1] riscv: Add timer_get_us() for tracing In-Reply-To: <079137ab-e3d8-4164-4200-ce49ca505930@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> <079137ab-e3d8-4164-4200-ce49ca505930@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Heinrich, >-----Original Message----- >From: Heinrich Schuchardt >Sent: 12 November 2020 12:49 >To: Pragnesh Patel ; Simon Glass > >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 >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 >>>> 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? 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 >>>>>>> #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); } >>>>>>> >>>>>>> >>>>> >>> >>