[PATCH v3 0/3] bootstage support for risc-v
This adds to support bootstage for risc-v. timer_get_boot_us function is required to record each boot stages with microsecond timestamp.
Changes from v2: - Add a Leo's RB tag of 1/3 patch - Add a description of the calculation formula - Correct the divisor value typo of 3/3 patch
Chanho Park (3): riscv: bootstage: correct bootstage_report guard riscv: timer: add timer_get_boot_us for BOOTSTAGE timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE
arch/riscv/lib/bootm.c | 2 +- drivers/timer/riscv_aclint_timer.c | 23 +++++++++++++++++++++++ drivers/timer/riscv_timer.c | 22 ++++++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-)
Below warning can be occurred when CONFIG_BOOTSTAGE and !CONFIG_SPL_BOOTSTAGE. It should be guarded by using CONFIG_IS_ENABLED for SPL build.
arch/riscv/lib/bootm.c:46:9: warning: implicit declaration of function 'bootstage_report' 46 | bootstage_report(); | ^~~~~~~~~~~~~~~~ | bootstage_error
Signed-off-by: Chanho Park chanho61.park@samsung.com Reviewed-by: Leo Yu-Chi Liang ycliang@andestech.com --- arch/riscv/lib/bootm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c index 276677a5e2f9..cc30efc90498 100644 --- a/arch/riscv/lib/bootm.c +++ b/arch/riscv/lib/bootm.c @@ -42,7 +42,7 @@ static void announce_and_cleanup(int fake) #ifdef CONFIG_BOOTSTAGE_FDT bootstage_fdt_add_report(); #endif -#ifdef CONFIG_BOOTSTAGE_REPORT +#if CONFIG_IS_ENABLED(BOOTSTAGE_REPORT) bootstage_report(); #endif
timer_get_boot_us function is required to record the boot stages as us-based timestamp. To get a micro-second time from a timer tick, this converts the formula like below to avoid zero result of (tick / rate) part.
From: time(us) = (tick / rate) * 10000000 To : time(us) = (tick * 1000) / (rate / 1000)
Signed-off-by: Chanho Park chanho61.park@samsung.com --- drivers/timer/riscv_timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c index 28a6a6870b81..169c03dcb5c1 100644 --- a/drivers/timer/riscv_timer.c +++ b/drivers/timer/riscv_timer.c @@ -11,6 +11,7 @@ */
#include <common.h> +#include <div64.h> #include <dm.h> #include <errno.h> #include <fdt_support.h> @@ -51,6 +52,27 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(RISCV_SMODE) && CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{ + int ret; + u64 ticks = 0; + u32 rate; + + ret = dm_timer_init(); + if (!ret) { + rate = timer_get_rate(gd->timer); + timer_get_count(gd->timer, &ticks); + } else { + rate = RISCV_SMODE_TIMER_FREQ; + ticks = riscv_timer_get_count(NULL); + } + + /* Below is converted from time(us) = (tick / rate) * 10000000 */ + return lldiv(ticks * 1000, (rate / 1000)); +} +#endif + static int riscv_timer_probe(struct udevice *dev) { struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
On Wed, Sep 06, 2023 at 02:18:13PM +0900, Chanho Park wrote:
timer_get_boot_us function is required to record the boot stages as us-based timestamp. To get a micro-second time from a timer tick, this converts the formula like below to avoid zero result of (tick / rate) part.
From: time(us) = (tick / rate) * 10000000 To : time(us) = (tick * 1000) / (rate / 1000)
Signed-off-by: Chanho Park chanho61.park@samsung.com
drivers/timer/riscv_timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
Reviewed-by: Leo Yu-Chi Liang ycliang@andestech.com
timer_get_boot_us function is required to record the boot stages as us-based timestamp. To get a micro-second time from a timer tick, this converts the formula like below to avoid zero result of (tick / rate) part.
From: time(us) = (tick / rate) * 10000000 To : time(us) = (tick * 1000) / (rate / 1000)
Signed-off-by: Chanho Park chanho61.park@samsung.com --- drivers/timer/riscv_aclint_timer.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/drivers/timer/riscv_aclint_timer.c b/drivers/timer/riscv_aclint_timer.c index e29d527c8d77..73fb87912851 100644 --- a/drivers/timer/riscv_aclint_timer.c +++ b/drivers/timer/riscv_aclint_timer.c @@ -6,6 +6,7 @@
#include <common.h> #include <clk.h> +#include <div64.h> #include <dm.h> #include <timer.h> #include <asm/io.h> @@ -44,6 +45,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(RISCV_MMODE) && CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{ + int ret; + u64 ticks = 0; + u32 rate; + + ret = dm_timer_init(); + if (!ret) { + rate = timer_get_rate(gd->timer); + timer_get_count(gd->timer, &ticks); + } else { + rate = RISCV_MMODE_TIMER_FREQ; + ticks = readq((void __iomem *)MTIME_REG(RISCV_MMODE_TIMERBASE, + RISCV_MMODE_TIMEROFF)); + } + + /* Below is converted from time(us) = (tick / rate) * 10000000 */ + return lldiv(ticks * 1000, (rate / 1000)); +} +#endif + static const struct timer_ops riscv_aclint_timer_ops = { .get_count = riscv_aclint_timer_get_count, };
On 06.09.23 07:18, Chanho Park wrote:
timer_get_boot_us function is required to record the boot stages as us-based timestamp. To get a micro-second time from a timer tick, this converts the formula like below to avoid zero result of (tick / rate) part.
From: time(us) = (tick / rate) * 10000000
Where is the old implementation that you refer to?
To : time(us) = (tick * 1000) / (rate / 1000)
Signed-off-by: Chanho Park chanho61.park@samsung.com
drivers/timer/riscv_aclint_timer.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/drivers/timer/riscv_aclint_timer.c b/drivers/timer/riscv_aclint_timer.c index e29d527c8d77..73fb87912851 100644 --- a/drivers/timer/riscv_aclint_timer.c +++ b/drivers/timer/riscv_aclint_timer.c @@ -6,6 +6,7 @@
#include <common.h> #include <clk.h> +#include <div64.h> #include <dm.h> #include <timer.h> #include <asm/io.h> @@ -44,6 +45,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(RISCV_MMODE) && CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{
- int ret;
- u64 ticks = 0;
- u32 rate;
- ret = dm_timer_init();
- if (!ret) {
rate = timer_get_rate(gd->timer);timer_get_count(gd->timer, &ticks);- } else {
rate = RISCV_MMODE_TIMER_FREQ;ticks = readq((void __iomem *)MTIME_REG(RISCV_MMODE_TIMERBASE,RISCV_MMODE_TIMEROFF));- }
- /* Below is converted from time(us) = (tick / rate) * 10000000 */
- return lldiv(ticks * 1000, (rate / 1000));
I found similar code in drivers/timer/cadence-ttc.c and drivers/timer/omap-timer.c with
us = (ticks * 1000) / rate; return us.
Either their code or yours must be wrong.
What I am missing in include/timer.h is a documentation that defines if timer_dev_priv.clock_rate and timer_get_rate() yield the frequency in Hz or kHz.
Once we have added the missing information in the include we can start reviewing this patch.
I really dislike that we have code per architecture and don't update and use a implementation in lib/time.c (where we also have an implementation) or drivers/timer/timer-uclass.c. Can't we have a single implementation which is driver model based and eliminate all others?
Best regards
Heinrich
+} +#endif
- static const struct timer_ops riscv_aclint_timer_ops = { .get_count = riscv_aclint_timer_get_count, };
Hi,
-----Original Message----- From: Heinrich Schuchardt heinrich.schuchardt@canonical.com Sent: Thursday, September 7, 2023 2:27 AM To: Chanho Park chanho61.park@samsung.com; Simon Glass sjg@chromium.org Cc: u-boot@lists.denx.de; Rick Chen rick@andestech.com; Leo ycliang@andestech.com Subject: Re: [PATCH v3 3/3] timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE
On 06.09.23 07:18, Chanho Park wrote:
timer_get_boot_us function is required to record the boot stages as us-based timestamp. To get a micro-second time from a timer tick, this converts the formula like below to avoid zero result of (tick / rate) part.
From: time(us) = (tick / rate) * 10000000
Still typo 10000000 -> 1000000
Where is the old implementation that you refer to?
I referred it from timer_get_boot_us function of lib/time.c
lib/time.c 55 else if (timer_rate > 1000000) 56 return lldiv(count, timer_rate / 1000000); 57 else 58 return (unsigned long long)count * 1000000 / timer_rate;
To : time(us) = (tick * 1000) / (rate / 1000)
Signed-off-by: Chanho Park chanho61.park@samsung.com
drivers/timer/riscv_aclint_timer.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/drivers/timer/riscv_aclint_timer.c
b/drivers/timer/riscv_aclint_timer.c
index e29d527c8d77..73fb87912851 100644 --- a/drivers/timer/riscv_aclint_timer.c +++ b/drivers/timer/riscv_aclint_timer.c @@ -6,6 +6,7 @@
#include <common.h> #include <clk.h> +#include <div64.h> #include <dm.h> #include <timer.h> #include <asm/io.h> @@ -44,6 +45,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(RISCV_MMODE) && CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{
- int ret;
- u64 ticks = 0;
- u32 rate;
- ret = dm_timer_init();
- if (!ret) {
rate = timer_get_rate(gd->timer);timer_get_count(gd->timer, &ticks);- } else {
rate = RISCV_MMODE_TIMER_FREQ;ticks = readq((void __iomem*)MTIME_REG(RISCV_MMODE_TIMERBASE,
RISCV_MMODE_TIMEROFF));- }
- /* Below is converted from time(us) = (tick / rate) * 10000000 */
- return lldiv(ticks * 1000, (rate / 1000));
I found similar code in drivers/timer/cadence-ttc.c and drivers/timer/omap-timer.c with
us = (ticks * 1000) / rate; return us.Either their code or yours must be wrong.
What I am missing in include/timer.h is a documentation that defines if timer_dev_priv.clock_rate and timer_get_rate() yield the frequency in Hz or kHz.
'rate' seems to be Hz not kHz. So, I think they need to be corrected.
Once we have added the missing information in the include we can start reviewing this patch.
I really dislike that we have code per architecture and don't update and use a implementation in lib/time.c (where we also have an implementation) or drivers/timer/timer-uclass.c. Can't we have a single implementation which is driver model based and eliminate all others?
Actually, I tried to use lib/time.c's implementation or make a generic function in timer-uclass.c as you mentioned. However, there are different implementations in cadence-ttc.c, rockchip_timer.c, tegra-timer.c, omap-timer.c and tsc_timer.c. The basic codes seems to be almost identical if we can get a DM timer successfully but fallback codes look different. If timer_get_boot_us can be implement in the timer-uclass.c, we still need per-driver callback function for supporting these different fallback codes.
Best Regards, Chanho Park
Hi On Thu, Sep 07, 2023 at 03:39:57PM +0900, Chanho Park wrote:
Hi,
-----Original Message----- From: Heinrich Schuchardt heinrich.schuchardt@canonical.com Sent: Thursday, September 7, 2023 2:27 AM To: Chanho Park chanho61.park@samsung.com; Simon Glass sjg@chromium.org Cc: u-boot@lists.denx.de; Rick Chen rick@andestech.com; Leo ycliang@andestech.com Subject: Re: [PATCH v3 3/3] timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE
On 06.09.23 07:18, Chanho Park wrote:
timer_get_boot_us function is required to record the boot stages as us-based timestamp. To get a micro-second time from a timer tick, this converts the formula like below to avoid zero result of (tick / rate) part.
From: time(us) = (tick / rate) * 10000000
Still typo 10000000 -> 1000000
Where is the old implementation that you refer to?
I referred it from timer_get_boot_us function of lib/time.c
lib/time.c 55 else if (timer_rate > 1000000) 56 return lldiv(count, timer_rate / 1000000); 57 else 58 return (unsigned long long)count * 1000000 / timer_rate;
To : time(us) = (tick * 1000) / (rate / 1000)
Signed-off-by: Chanho Park chanho61.park@samsung.com
drivers/timer/riscv_aclint_timer.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/drivers/timer/riscv_aclint_timer.c
b/drivers/timer/riscv_aclint_timer.c
index e29d527c8d77..73fb87912851 100644 --- a/drivers/timer/riscv_aclint_timer.c +++ b/drivers/timer/riscv_aclint_timer.c @@ -6,6 +6,7 @@
#include <common.h> #include <clk.h> +#include <div64.h> #include <dm.h> #include <timer.h> #include <asm/io.h> @@ -44,6 +45,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(RISCV_MMODE) && CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{
- int ret;
- u64 ticks = 0;
- u32 rate;
- ret = dm_timer_init();
- if (!ret) {
rate = timer_get_rate(gd->timer);timer_get_count(gd->timer, &ticks);- } else {
rate = RISCV_MMODE_TIMER_FREQ;ticks = readq((void __iomem*)MTIME_REG(RISCV_MMODE_TIMERBASE,
RISCV_MMODE_TIMEROFF));- }
- /* Below is converted from time(us) = (tick / rate) * 10000000 */
- return lldiv(ticks * 1000, (rate / 1000));i
Reviewed-by: Leo Yu-Chi Liang ycliang@andestech.com
I found similar code in drivers/timer/cadence-ttc.c and drivers/timer/omap-timer.c with
us = (ticks * 1000) / rate; return us.Either their code or yours must be wrong.
What I am missing in include/timer.h is a documentation that defines if timer_dev_priv.clock_rate and timer_get_rate() yield the frequency in Hz or kHz.
'rate' seems to be Hz not kHz. So, I think they need to be corrected.
Once we have added the missing information in the include we can start reviewing this patch.
I really dislike that we have code per architecture and don't update and use a implementation in lib/time.c (where we also have an implementation) or drivers/timer/timer-uclass.c. Can't we have a single implementation which is driver model based and eliminate all others?
Actually, I tried to use lib/time.c's implementation or make a generic function in timer-uclass.c as you mentioned. However, there are different implementations in cadence-ttc.c, rockchip_timer.c, tegra-timer.c, omap-timer.c and tsc_timer.c. The basic codes seems to be almost identical if we can get a DM timer successfully but fallback codes look different. If timer_get_boot_us can be implement in the timer-uclass.c, we still need per-driver callback function for supporting these different fallback codes.
I think we could merge this patch set first, and then think more thoroghly about how we are going to clean this up.
Best regards, Leo
Best Regards, Chanho Park
participants (3)
-
Chanho Park -
Heinrich Schuchardt -
Leo Liang