[PATCH v2 0/3] bootstage support for risc-v

[PATCH v2 0/3] bootstage support for risc-v

[Chanho Park]

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 v1:
- Correct #ifdef guard for riscv_aclint_timer and riscv_timer

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 | 22 ++++++++++++++++++++++
 drivers/timer/riscv_timer.c        | 21 +++++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH v2 1/3] riscv: bootstage: correct bootstage_report guard

[Chanho Park]

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>
---
 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
 
-- 
2.39.2

[PATCH v2 2/3] riscv: timer: add timer_get_boot_us for BOOTSTAGE

[Chanho Park]

timer_get_boot_us function is required to record the boot stages as
us-based timestamp.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/timer/riscv_timer.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c
index 3627ed79b819..6cb589fcdc45 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 <timer.h>
@@ -50,6 +51,26 @@ 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);
+	}
+
+	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);
-- 
2.39.2

[PATCH v2 3/3] timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE

[Chanho Park]

timer_get_boot_us function is required to record the boot stages as
us-based timestamp.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/timer/riscv_aclint_timer.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/timer/riscv_aclint_timer.c b/drivers/timer/riscv_aclint_timer.c
index e29d527c8d77..8b67745bb4a2 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,27 @@ 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));
+	}
+
+	return lldiv(ticks * 1001, (rate / 1000));
+}
+#endif
+
 static const struct timer_ops riscv_aclint_timer_ops = {
 	.get_count = riscv_aclint_timer_get_count,
 };
-- 
2.39.2

Re: [PATCH v2 1/3] riscv: bootstage: correct bootstage_report guard

[Leo Liang]

On Mon, Aug 28, 2023 at 06:49:36PM +0900, Chanho Park wrote:
> 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>
> ---
>  arch/riscv/lib/bootm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

Re: [PATCH v2 2/3] riscv: timer: add timer_get_boot_us for BOOTSTAGE

[Leo Liang]

Hi Chanho,

On Mon, Aug 28, 2023 at 06:49:37PM +0900, Chanho Park wrote:
> timer_get_boot_us function is required to record the boot stages as
> us-based timestamp.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/timer/riscv_timer.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c
> index 3627ed79b819..6cb589fcdc45 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 <timer.h>
> @@ -50,6 +51,26 @@ 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);
> +	}
> +
> +	return lldiv(ticks * 1000, (rate / 1000));

Could you elaborate a little how this formula is derived?

Best regards,
Leo

> +}
> +#endif
> +
>  static int riscv_timer_probe(struct udevice *dev)
>  {
>  	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> -- 
> 2.39.2
> 

Re: [PATCH v2 3/3] timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE

[Leo Liang]

Hi Chanho,

On Mon, Aug 28, 2023 at 06:49:38PM +0900, Chanho Park wrote:
> timer_get_boot_us function is required to record the boot stages as
> us-based timestamp.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/timer/riscv_aclint_timer.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/timer/riscv_aclint_timer.c b/drivers/timer/riscv_aclint_timer.c
> index e29d527c8d77..8b67745bb4a2 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,27 @@ 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));
> +	}
> +
> +	return lldiv(ticks * 1001, (rate / 1000));

Why is this dividend 1001 ?

Best regards,
Leo

> +}
> +#endif
> +
>  static const struct timer_ops riscv_aclint_timer_ops = {
>  	.get_count = riscv_aclint_timer_get_count,
>  };
> -- 
> 2.39.2
> 

RE: [PATCH v2 2/3] riscv: timer: add timer_get_boot_us for BOOTSTAGE

[Chanho Park]

Hi,

> -----Original Message-----
> From: Leo Liang <ycliang@andestech.com>
> Sent: Monday, September 4, 2023 4:01 PM
> To: Chanho Park <chanho61.park@samsung.com>
> Cc: Rick Chen <rick@andestech.com>; Simon Glass <sjg@chromium.org>; u-
> boot@lists.denx.de
> Subject: Re: [PATCH v2 2/3] riscv: timer: add timer_get_boot_us for
> BOOTSTAGE
> 
> Hi Chanho,
> 
> On Mon, Aug 28, 2023 at 06:49:37PM +0900, Chanho Park wrote:
> > timer_get_boot_us function is required to record the boot stages as
> > us-based timestamp.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > ---
> >  drivers/timer/riscv_timer.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c
> > index 3627ed79b819..6cb589fcdc45 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 <timer.h>
> > @@ -50,6 +51,26 @@ 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);
> > +	}
> > +
> > +	return lldiv(ticks * 1000, (rate / 1000));
> 
> Could you elaborate a little how this formula is derived?

Sure. Below is the original formula. To avoid zero of (tick/rate), I split
the calculation to ticks and rate, respectively.

Time(us) = (ticks / rate) * 1000000
          Or (ticks * 1000000) / rate
          Or tick / (rate / 10000000)

Best Regards,
Chanho Park

RE: [PATCH v2 3/3] timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE

[Chanho Park]

Hi,

> -----Original Message-----
> From: Leo Liang <ycliang@andestech.com>
> Sent: Monday, September 4, 2023 4:02 PM
> To: Chanho Park <chanho61.park@samsung.com>
> Cc: Rick Chen <rick@andestech.com>; Simon Glass <sjg@chromium.org>; u-
> boot@lists.denx.de
> Subject: Re: [PATCH v2 3/3] timer: riscv_aclint_timer: add
> timer_get_boot_us for BOOTSTAGE
> 
> Hi Chanho,
> 
> On Mon, Aug 28, 2023 at 06:49:38PM +0900, Chanho Park wrote:
> > timer_get_boot_us function is required to record the boot stages as
> > us-based timestamp.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > ---
> >  drivers/timer/riscv_aclint_timer.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/timer/riscv_aclint_timer.c
> > b/drivers/timer/riscv_aclint_timer.c
> > index e29d527c8d77..8b67745bb4a2 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,27 @@ 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));
> > +	}
> > +
> > +	return lldiv(ticks * 1001, (rate / 1000));
> 
> Why is this dividend 1001 ?

It's a typo. I'll correct when I send v2.

Best Regards,
Chanho Park

Re: [PATCH v2 3/3] timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE

[Simon Glass]

Hi Chanho,

On Mon, 28 Aug 2023 at 03:50, Chanho Park <chanho61.park@samsung.com> wrote:
>
> timer_get_boot_us function is required to record the boot stages as
> us-based timestamp.
>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/timer/riscv_aclint_timer.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/timer/riscv_aclint_timer.c b/drivers/timer/riscv_aclint_timer.c
> index e29d527c8d77..8b67745bb4a2 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,27 @@ u64 notrace timer_early_get_count(void)
>  }
>  #endif
>
> +#if CONFIG_IS_ENABLED(RISCV_MMODE) && CONFIG_IS_ENABLED(BOOTSTAGE)

Just a nit...you should not need this #if, since if the function is
not used it will be stripped from the image by the linker.

> +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));
> +       }
> +
> +       return lldiv(ticks * 1001, (rate / 1000));
> +}
> +#endif
> +
>  static const struct timer_ops riscv_aclint_timer_ops = {
>         .get_count = riscv_aclint_timer_get_count,
>  };
> --
> 2.39.2
>

Regards,
Simon

RE: [PATCH v2 3/3] timer: riscv_aclint_timer: add timer_get_boot_us for BOOTSTAGE

[Chanho Park]

Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Tuesday, September 5, 2023 1:49 AM
> To: Chanho Park <chanho61.park@samsung.com>
> Cc: Rick Chen <rick@andestech.com>; Leo <ycliang@andestech.com>; u-
> boot@lists.denx.de
> Subject: Re: [PATCH v2 3/3] timer: riscv_aclint_timer: add
> timer_get_boot_us for BOOTSTAGE
> 
> Hi Chanho,
> 
> On Mon, 28 Aug 2023 at 03:50, Chanho Park <chanho61.park@samsung.com>
> wrote:
> >
> > timer_get_boot_us function is required to record the boot stages as
> > us-based timestamp.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > ---
> >  drivers/timer/riscv_aclint_timer.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/timer/riscv_aclint_timer.c
> b/drivers/timer/riscv_aclint_timer.c
> > index e29d527c8d77..8b67745bb4a2 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,27 @@ u64 notrace timer_early_get_count(void)
> >  }
> >  #endif
> >
> > +#if CONFIG_IS_ENABLED(RISCV_MMODE) && CONFIG_IS_ENABLED(BOOTSTAGE)
> 
> Just a nit...you should not need this #if, since if the function is
> not used it will be stripped from the image by the linker.

Thank you for the suggestion.
Without these guards, compile error can be occurred due to multiple timer_get_boot_us definitions.
For risc-v, there are two timers for S-mode and M-mode, respectively.
So, I put them to avoid below build errors.

riscv64-unknown-linux-gnu-ld.bfd: drivers/timer/riscv_aclint_timer.o: in function `timer_get_boot_us':

/data/risc-v/qemu/u-boot/drivers/timer/riscv_aclint_timer.c:49: multiple definition of `timer_get_boot_us'; drivers/timer/riscv_timer.o:/data/risc-v/qemu/u-boot/drivers/timer/riscv_timer.c:55: first defined here

make[1]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1

Best Regards,
Chanho Park