[PATCH v2] bootstage: Show func name for bootstage_mark/error

[PATCH v2] bootstage: Show func name for bootstage_mark/error

[Michal Simek]

bootstage_mark() and bootstate_error() are not recording any name and in
report it is showing as id=<value>. That's not useful and it is better to
show function name which calls it.
That's why use macros with passing __func__ as recorded name for bootstage.

Origin report looks like this:
ZynqMP> bootstage report
Timer summary in microseconds (10 records):
       Mark    Elapsed  Stage
          0          0  reset
  2,482,383  2,482,383  board_init_f
  4,278,821  1,796,438  board_init_r
  4,825,331    546,510  id=64
  4,858,409     33,078  id=65
  4,862,382      3,973  main_loop
  4,921,713     59,331  usb_start
  9,345,345  4,423,632  id=175

When this patch is applied.
ZynqMP> bootstage report
Timer summary in microseconds (31 records):
       Mark    Elapsed  Stage
          0          0  reset
  2,465,624  2,465,624  board_init_f
  4,278,628  1,813,004  board_init_r
  4,825,139    546,511  eth_common_init
  4,858,228     33,089  eth_initialize
  4,862,201      3,973  main_loop
  4,921,530     59,329  usb_start
  8,885,334  3,963,804  cli_loop

Signed-off-by: Michal Simek <michal.simek@amd.com>
---

Changes in v2:
- Add kernel-doc description

 common/bootstage.c  | 10 ++--------
 include/bootstage.h | 21 ++++++++++++++++++---
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/common/bootstage.c b/common/bootstage.c
index 0fd33be97eae..326c40f1561f 100644
--- a/common/bootstage.c
+++ b/common/bootstage.c
@@ -147,15 +147,9 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
 	return mark;
 }
 
-
-ulong bootstage_mark(enum bootstage_id id)
-{
-	return bootstage_add_record(id, NULL, 0, timer_get_boot_us());
-}
-
-ulong bootstage_error(enum bootstage_id id)
+ulong bootstage_error_name(enum bootstage_id id, const char *name)
 {
-	return bootstage_add_record(id, NULL, BOOTSTAGEF_ERROR,
+	return bootstage_add_record(id, name, BOOTSTAGEF_ERROR,
 				    timer_get_boot_us());
 }
 
diff --git a/include/bootstage.h b/include/bootstage.h
index bca9438418f5..7088d0b875e4 100644
--- a/include/bootstage.h
+++ b/include/bootstage.h
@@ -268,12 +268,27 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
 /**
  * Mark a time stamp for the current boot stage.
  */
-ulong bootstage_mark(enum bootstage_id id);
-
-ulong bootstage_error(enum bootstage_id id);
+#define bootstage_mark(id)	bootstage_mark_name(id, __func__)
+#define bootstage_error(id)	bootstage_error_name(id, __func__)
 
+/**
+ * bootstage_mark_name - record bootstage with passing id and name
+ * @id: Bootstage id to record this timestamp against
+ * @name: Textual name to display for this id in the report
+ *
+ * Return: recorded time stamp
+ */
 ulong bootstage_mark_name(enum bootstage_id id, const char *name);
 
+/**
+ * bootstage_error_name - record bootstage error with passing id and name
+ * @id: Bootstage id to record this timestamp against
+ * @name: Textual name to display for this id in the report
+ *
+ * Return: recorded time stamp
+ */
+ulong bootstage_error_name(enum bootstage_id id, const char *name);
+
 /**
  * Mark a time stamp in the given function and line number
  *
-- 
2.36.1

Re: [PATCH v2] bootstage: Show func name for bootstage_mark/error

[Simon Glass]

Hi Michal,

On Fri, 12 Aug 2022 at 02:55, Michal Simek <michal.simek@amd.com> wrote:
>
> bootstage_mark() and bootstate_error() are not recording any name and in
> report it is showing as id=<value>. That's not useful and it is better to
> show function name which calls it.
> That's why use macros with passing __func__ as recorded name for bootstage.
>
> Origin report looks like this:
> ZynqMP> bootstage report
> Timer summary in microseconds (10 records):
>        Mark    Elapsed  Stage
>           0          0  reset
>   2,482,383  2,482,383  board_init_f
>   4,278,821  1,796,438  board_init_r
>   4,825,331    546,510  id=64
>   4,858,409     33,078  id=65
>   4,862,382      3,973  main_loop
>   4,921,713     59,331  usb_start
>   9,345,345  4,423,632  id=175
>
> When this patch is applied.
> ZynqMP> bootstage report
> Timer summary in microseconds (31 records):
>        Mark    Elapsed  Stage
>           0          0  reset
>   2,465,624  2,465,624  board_init_f
>   4,278,628  1,813,004  board_init_r
>   4,825,139    546,511  eth_common_init
>   4,858,228     33,089  eth_initialize
>   4,862,201      3,973  main_loop
>   4,921,530     59,329  usb_start
>   8,885,334  3,963,804  cli_loop
>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
>
> Changes in v2:
> - Add kernel-doc description
>
>  common/bootstage.c  | 10 ++--------
>  include/bootstage.h | 21 ++++++++++++++++++---
>  2 files changed, 20 insertions(+), 11 deletions(-)

I can see what is going on here after applying it.

Reviewed-by: Simon Glass <sjg@chromium.org>

But I think it would be better as inline functions rather than #define

>
> diff --git a/common/bootstage.c b/common/bootstage.c
> index 0fd33be97eae..326c40f1561f 100644
> --- a/common/bootstage.c
> +++ b/common/bootstage.c
> @@ -147,15 +147,9 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
>         return mark;
>  }
>
> -
> -ulong bootstage_mark(enum bootstage_id id)
> -{
> -       return bootstage_add_record(id, NULL, 0, timer_get_boot_us());
> -}
> -
> -ulong bootstage_error(enum bootstage_id id)
> +ulong bootstage_error_name(enum bootstage_id id, const char *name)
>  {
> -       return bootstage_add_record(id, NULL, BOOTSTAGEF_ERROR,
> +       return bootstage_add_record(id, name, BOOTSTAGEF_ERROR,
>                                     timer_get_boot_us());
>  }
>
> diff --git a/include/bootstage.h b/include/bootstage.h
> index bca9438418f5..7088d0b875e4 100644
> --- a/include/bootstage.h
> +++ b/include/bootstage.h
> @@ -268,12 +268,27 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
>  /**
>   * Mark a time stamp for the current boot stage.
>   */
> -ulong bootstage_mark(enum bootstage_id id);
> -
> -ulong bootstage_error(enum bootstage_id id);
> +#define bootstage_mark(id)     bootstage_mark_name(id, __func__)
> +#define bootstage_error(id)    bootstage_error_name(id, __func__)
>
> +/**
> + * bootstage_mark_name - record bootstage with passing id and name
> + * @id: Bootstage id to record this timestamp against
> + * @name: Textual name to display for this id in the report
> + *
> + * Return: recorded time stamp
> + */
>  ulong bootstage_mark_name(enum bootstage_id id, const char *name);
>
> +/**
> + * bootstage_error_name - record bootstage error with passing id and name
> + * @id: Bootstage id to record this timestamp against
> + * @name: Textual name to display for this id in the report
> + *
> + * Return: recorded time stamp
> + */
> +ulong bootstage_error_name(enum bootstage_id id, const char *name);
> +
>  /**
>   * Mark a time stamp in the given function and line number
>   *
> --
> 2.36.1
>

Regards,
Simon

Re: [PATCH v2] bootstage: Show func name for bootstage_mark/error

[Michal Simek]

so 13. 8. 2022 v 4:21 odesílatel Simon Glass <sjg@chromium.org> napsal:
>
> Hi Michal,
>
> On Fri, 12 Aug 2022 at 02:55, Michal Simek <michal.simek@amd.com> wrote:
> >
> > bootstage_mark() and bootstate_error() are not recording any name and in
> > report it is showing as id=<value>. That's not useful and it is better to
> > show function name which calls it.
> > That's why use macros with passing __func__ as recorded name for bootstage.
> >
> > Origin report looks like this:
> > ZynqMP> bootstage report
> > Timer summary in microseconds (10 records):
> >        Mark    Elapsed  Stage
> >           0          0  reset
> >   2,482,383  2,482,383  board_init_f
> >   4,278,821  1,796,438  board_init_r
> >   4,825,331    546,510  id=64
> >   4,858,409     33,078  id=65
> >   4,862,382      3,973  main_loop
> >   4,921,713     59,331  usb_start
> >   9,345,345  4,423,632  id=175
> >
> > When this patch is applied.
> > ZynqMP> bootstage report
> > Timer summary in microseconds (31 records):
> >        Mark    Elapsed  Stage
> >           0          0  reset
> >   2,465,624  2,465,624  board_init_f
> >   4,278,628  1,813,004  board_init_r
> >   4,825,139    546,511  eth_common_init
> >   4,858,228     33,089  eth_initialize
> >   4,862,201      3,973  main_loop
> >   4,921,530     59,329  usb_start
> >   8,885,334  3,963,804  cli_loop
> >
> > Signed-off-by: Michal Simek <michal.simek@amd.com>
> > ---
> >
> > Changes in v2:
> > - Add kernel-doc description
> >
> >  common/bootstage.c  | 10 ++--------
> >  include/bootstage.h | 21 ++++++++++++++++++---
> >  2 files changed, 20 insertions(+), 11 deletions(-)
>
> I can see what is going on here after applying it.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But I think it would be better as inline functions rather than #define

I was trying static inline functions but __func__ is using bootstage
names instead of the function
which is calling them. Maybe there is another function specifier which
can be used to do the same job
as macros.

Thanks,
Michal

Re: [PATCH v2] bootstage: Show func name for bootstage_mark/error

[Simon Glass]

Hi Michal,

On Fri, 12 Aug 2022 at 23:39, Michal Simek <monstr@monstr.eu> wrote:
>
> so 13. 8. 2022 v 4:21 odesílatel Simon Glass <sjg@chromium.org> napsal:
> >
> > Hi Michal,
> >
> > On Fri, 12 Aug 2022 at 02:55, Michal Simek <michal.simek@amd.com> wrote:
> > >
> > > bootstage_mark() and bootstate_error() are not recording any name and in
> > > report it is showing as id=<value>. That's not useful and it is better to
> > > show function name which calls it.
> > > That's why use macros with passing __func__ as recorded name for bootstage.
> > >
> > > Origin report looks like this:
> > > ZynqMP> bootstage report
> > > Timer summary in microseconds (10 records):
> > >        Mark    Elapsed  Stage
> > >           0          0  reset
> > >   2,482,383  2,482,383  board_init_f
> > >   4,278,821  1,796,438  board_init_r
> > >   4,825,331    546,510  id=64
> > >   4,858,409     33,078  id=65
> > >   4,862,382      3,973  main_loop
> > >   4,921,713     59,331  usb_start
> > >   9,345,345  4,423,632  id=175
> > >
> > > When this patch is applied.
> > > ZynqMP> bootstage report
> > > Timer summary in microseconds (31 records):
> > >        Mark    Elapsed  Stage
> > >           0          0  reset
> > >   2,465,624  2,465,624  board_init_f
> > >   4,278,628  1,813,004  board_init_r
> > >   4,825,139    546,511  eth_common_init
> > >   4,858,228     33,089  eth_initialize
> > >   4,862,201      3,973  main_loop
> > >   4,921,530     59,329  usb_start
> > >   8,885,334  3,963,804  cli_loop
> > >
> > > Signed-off-by: Michal Simek <michal.simek@amd.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Add kernel-doc description
> > >
> > >  common/bootstage.c  | 10 ++--------
> > >  include/bootstage.h | 21 ++++++++++++++++++---
> > >  2 files changed, 20 insertions(+), 11 deletions(-)
> >
> > I can see what is going on here after applying it.
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > But I think it would be better as inline functions rather than #define
>
> I was trying static inline functions but __func__ is using bootstage
> names instead of the function
> which is calling them. Maybe there is another function specifier which
> can be used to do the same job
> as macros.

Not that I know of...I was thinking of using a #define to call a
function with a similar name (e.,g. with a trailing underscore). But I
think I'm just being too precious. Let's leave it as it is and see how
things go.

Regards,
Simon

Re: [PATCH v2] bootstage: Show func name for bootstage_mark/error

[Simon Glass]

Hi Michal,

On Fri, 12 Aug 2022 at 23:39, Michal Simek <monstr@monstr.eu> wrote:
>
> so 13. 8. 2022 v 4:21 odesílatel Simon Glass <sjg@chromium.org> napsal:
> >
> > Hi Michal,
> >
> > On Fri, 12 Aug 2022 at 02:55, Michal Simek <michal.simek@amd.com> wrote:
> > >
> > > bootstage_mark() and bootstate_error() are not recording any name and in
> > > report it is showing as id=<value>. That's not useful and it is better to
> > > show function name which calls it.
> > > That's why use macros with passing __func__ as recorded name for bootstage.
> > >
> > > Origin report looks like this:
> > > ZynqMP> bootstage report
> > > Timer summary in microseconds (10 records):
> > >        Mark    Elapsed  Stage
> > >           0          0  reset
> > >   2,482,383  2,482,383  board_init_f
> > >   4,278,821  1,796,438  board_init_r
> > >   4,825,331    546,510  id=64
> > >   4,858,409     33,078  id=65
> > >   4,862,382      3,973  main_loop
> > >   4,921,713     59,331  usb_start
> > >   9,345,345  4,423,632  id=175
> > >
> > > When this patch is applied.
> > > ZynqMP> bootstage report
> > > Timer summary in microseconds (31 records):
> > >        Mark    Elapsed  Stage
> > >           0          0  reset
> > >   2,465,624  2,465,624  board_init_f
> > >   4,278,628  1,813,004  board_init_r
> > >   4,825,139    546,511  eth_common_init
> > >   4,858,228     33,089  eth_initialize
> > >   4,862,201      3,973  main_loop
> > >   4,921,530     59,329  usb_start
> > >   8,885,334  3,963,804  cli_loop
> > >
> > > Signed-off-by: Michal Simek <michal.simek@amd.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Add kernel-doc description
> > >
> > >  common/bootstage.c  | 10 ++--------
> > >  include/bootstage.h | 21 ++++++++++++++++++---
> > >  2 files changed, 20 insertions(+), 11 deletions(-)
> >
> > I can see what is going on here after applying it.
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > But I think it would be better as inline functions rather than #define
>
> I was trying static inline functions but __func__ is using bootstage
> names instead of the function
> which is calling them. Maybe there is another function specifier which
> can be used to do the same job
> as macros.

Not that I know of...I was thinking of using a #define to call a
function with a similar name (e.,g. with a trailing underscore). But I
think I'm just being too precious. Let's leave it as it is and see how
things go.

Regards,
Simon

Applied to u-boot-dm, thanks!