Patchwork [U-Boot,5/8] arm: Add boottime support for the ARM architecture

login
register
mail settings
Submitter Lee Jones
Date Nov. 20, 2012, 2:33 p.m.
Message ID <1353422034-28107-6-git-send-email-lee.jones@linaro.org>
Download mbox | patch
Permalink /patch/200358/
State Rejected
Delegated to: Wolfgang Denk
Headers show

Comments

Lee Jones - Nov. 20, 2012, 2:33 p.m.
This patch adds support for passing boot time information to
the Linus kernel using ATAGS when booting on ARM based devices.

Based heavily on the original driver by Jonas Aaberg.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/include/asm/setup.h |   18 +++++++++++++++++
 arch/arm/lib/bootm.c         |   45 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)
Otavio Salvador - Nov. 20, 2012, 3:11 p.m.
On Tue, Nov 20, 2012 at 12:33 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This patch adds support for passing boot time information to
> the Linus kernel using ATAGS when booting on ARM based devices.
>

Linus or Linux?


> Based heavily on the original driver by Jonas Aaberg.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/include/asm/setup.h |   18 +++++++++++++++++
>  arch/arm/lib/bootm.c         |   45
> ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/setup.h b/arch/arm/include/asm/setup.h
> index 78a7fac..6088440 100644
> --- a/arch/arm/include/asm/setup.h
> +++ b/arch/arm/include/asm/setup.h
> @@ -205,6 +205,19 @@ struct tag_memclk {
>         u32 fmemclk;
>  };
>
> +/* for automatic boot timing testcases */
> +#define ATAG_BOOTTIME  0x41000403
> +#define BOOTTIME_MAX 10
> +
> +#include <boottime.h>
> +
> +struct tag_boottime {
> +       struct boottime_entry entry[BOOTTIME_MAX];
> +       u32 idle;  /* in us */
> +       u32 total; /* in us */
> +       u8 num;
> +};
> +
>  struct tag {
>         struct tag_header hdr;
>         union {
> @@ -227,6 +240,11 @@ struct tag {
>                  * DC21285 specific
>                  */
>                 struct tag_memclk       memclk;
> +
> +               /*
> +                * Boot time
> +                */
> +               struct tag_boottime     boottime;
>         } u;
>  };
>
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 1bd2730..03774c8 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -28,6 +28,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <image.h>
> +#include <boottime.h>
>  #include <u-boot/zlib.h>
>  #include <asm/byteorder.h>
>  #include <fdt.h>
> @@ -114,7 +115,8 @@ static void announce_and_cleanup(void)
>         defined(CONFIG_CMDLINE_TAG) || \
>         defined(CONFIG_INITRD_TAG) || \
>         defined(CONFIG_SERIAL_TAG) || \
> -       defined(CONFIG_REVISION_TAG)
> +       defined(CONFIG_REVISION_TAG) || \
> +       defined(CONFIG_BOOTTIME)
>  static void setup_start_tag (bd_t *bd)
>  {
>         params = (struct tag *)bd->bi_boot_params;
> @@ -130,6 +132,37 @@ static void setup_start_tag (bd_t *bd)
>  }
>  #endif
>
> +#ifdef CONFIG_BOOTTIME
> +static void setup_boottime_tags(void)
> +{
> +       unsigned int i;
> +       struct boottime_entry *b;
> +
> +       params->hdr.tag = ATAG_BOOTTIME;
> +       params->hdr.size = tag_size(tag_boottime);
> +
> +       params->u.boottime.idle = boottime_idle_get();
> +       params->u.boottime.total = boottime_idle_done();
> +
> +       for (i = 0; i < BOOTTIME_MAX; i++) {
> +               b = boottime_get_entry(i);
> +               if (b == NULL)
> +                       break;
> +
> +               params->u.boottime.entry[i].time = b->time;
> +               strncpy((char *)params->u.boottime.entry[i].name,
> +                       (char *)b->name, BOOTTIME_MAX_NAME_LEN);
> +               params->u.boottime.entry[i].name[BOOTTIME_MAX_NAME_LEN -
> 1] = '\0';
> +
> +       }
> +
> +       params->u.boottime.num = i;
> +
> +       params = tag_next(params);
> +
> +}
> +#endif
> +
>  #ifdef CONFIG_SETUP_MEMORY_TAGS
>  static void setup_memory_tags(bd_t *bd)
>  {
> @@ -233,6 +266,10 @@ static void setup_end_tag(bd_t *bd)
>  }
>  #endif
>
> +#ifdef CONFIG_BOOTTIME
> +static void setup_boottime_tags(void);
> +#endif
> +
>  #ifdef CONFIG_OF_LIBFDT
>  static int create_fdt(bootm_headers_t *images)
>  {
> @@ -293,9 +330,13 @@ static void boot_prep_linux(bootm_headers_t *images)
>         defined(CONFIG_CMDLINE_TAG) || \
>         defined(CONFIG_INITRD_TAG) || \
>         defined(CONFIG_SERIAL_TAG) || \
> -       defined(CONFIG_REVISION_TAG)
> +       defined(CONFIG_REVISION_TAG) || \
> +       defined (CONFIG_BOOTTIME)
>                 debug("using: ATAGS\n");
>                 setup_start_tag(gd->bd);
> +#ifdef CONFIG_BOOTTIME
> +               setup_boottime_tags();
> +#endif
>  #ifdef CONFIG_SERIAL_TAG
>                 setup_serial_tag(&params);
>  #endif
> --
> 1.7.9.5
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Lee Jones - Nov. 20, 2012, 3:52 p.m.
On Tue, 20 Nov 2012, Otavio Salvador wrote:

> On Tue, Nov 20, 2012 at 12:33 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > This patch adds support for passing boot time information to
> > the Linus kernel using ATAGS when booting on ARM based devices.
>
> Linus or Linux?

Linux. 

I'll fix-up when the review process has finished.

Thanks.

> > Based heavily on the original driver by Jonas Aaberg.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/include/asm/setup.h |   18 +++++++++++++++++
> >  arch/arm/lib/bootm.c         |   45
> > ++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 61 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/setup.h b/arch/arm/include/asm/setup.h
> > index 78a7fac..6088440 100644
> > --- a/arch/arm/include/asm/setup.h
> > +++ b/arch/arm/include/asm/setup.h
> > @@ -205,6 +205,19 @@ struct tag_memclk {
> >         u32 fmemclk;
> >  };
> >
> > +/* for automatic boot timing testcases */
> > +#define ATAG_BOOTTIME  0x41000403
> > +#define BOOTTIME_MAX 10
> > +
> > +#include <boottime.h>
> > +
> > +struct tag_boottime {
> > +       struct boottime_entry entry[BOOTTIME_MAX];
> > +       u32 idle;  /* in us */
> > +       u32 total; /* in us */
> > +       u8 num;
> > +};
> > +
> >  struct tag {
> >         struct tag_header hdr;
> >         union {
> > @@ -227,6 +240,11 @@ struct tag {
> >                  * DC21285 specific
> >                  */
> >                 struct tag_memclk       memclk;
> > +
> > +               /*
> > +                * Boot time
> > +                */
> > +               struct tag_boottime     boottime;
> >         } u;
> >  };
> >
> > diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> > index 1bd2730..03774c8 100644
> > --- a/arch/arm/lib/bootm.c
> > +++ b/arch/arm/lib/bootm.c
> > @@ -28,6 +28,7 @@
> >  #include <common.h>
> >  #include <command.h>
> >  #include <image.h>
> > +#include <boottime.h>
> >  #include <u-boot/zlib.h>
> >  #include <asm/byteorder.h>
> >  #include <fdt.h>
> > @@ -114,7 +115,8 @@ static void announce_and_cleanup(void)
> >         defined(CONFIG_CMDLINE_TAG) || \
> >         defined(CONFIG_INITRD_TAG) || \
> >         defined(CONFIG_SERIAL_TAG) || \
> > -       defined(CONFIG_REVISION_TAG)
> > +       defined(CONFIG_REVISION_TAG) || \
> > +       defined(CONFIG_BOOTTIME)
> >  static void setup_start_tag (bd_t *bd)
> >  {
> >         params = (struct tag *)bd->bi_boot_params;
> > @@ -130,6 +132,37 @@ static void setup_start_tag (bd_t *bd)
> >  }
> >  #endif
> >
> > +#ifdef CONFIG_BOOTTIME
> > +static void setup_boottime_tags(void)
> > +{
> > +       unsigned int i;
> > +       struct boottime_entry *b;
> > +
> > +       params->hdr.tag = ATAG_BOOTTIME;
> > +       params->hdr.size = tag_size(tag_boottime);
> > +
> > +       params->u.boottime.idle = boottime_idle_get();
> > +       params->u.boottime.total = boottime_idle_done();
> > +
> > +       for (i = 0; i < BOOTTIME_MAX; i++) {
> > +               b = boottime_get_entry(i);
> > +               if (b == NULL)
> > +                       break;
> > +
> > +               params->u.boottime.entry[i].time = b->time;
> > +               strncpy((char *)params->u.boottime.entry[i].name,
> > +                       (char *)b->name, BOOTTIME_MAX_NAME_LEN);
> > +               params->u.boottime.entry[i].name[BOOTTIME_MAX_NAME_LEN -
> > 1] = '\0';
> > +
> > +       }
> > +
> > +       params->u.boottime.num = i;
> > +
> > +       params = tag_next(params);
> > +
> > +}
> > +#endif
> > +
> >  #ifdef CONFIG_SETUP_MEMORY_TAGS
> >  static void setup_memory_tags(bd_t *bd)
> >  {
> > @@ -233,6 +266,10 @@ static void setup_end_tag(bd_t *bd)
> >  }
> >  #endif
> >
> > +#ifdef CONFIG_BOOTTIME
> > +static void setup_boottime_tags(void);
> > +#endif
> > +
> >  #ifdef CONFIG_OF_LIBFDT
> >  static int create_fdt(bootm_headers_t *images)
> >  {
> > @@ -293,9 +330,13 @@ static void boot_prep_linux(bootm_headers_t *images)
> >         defined(CONFIG_CMDLINE_TAG) || \
> >         defined(CONFIG_INITRD_TAG) || \
> >         defined(CONFIG_SERIAL_TAG) || \
> > -       defined(CONFIG_REVISION_TAG)
> > +       defined(CONFIG_REVISION_TAG) || \
> > +       defined (CONFIG_BOOTTIME)
> >                 debug("using: ATAGS\n");
> >                 setup_start_tag(gd->bd);
> > +#ifdef CONFIG_BOOTTIME
> > +               setup_boottime_tags();
> > +#endif
> >  #ifdef CONFIG_SERIAL_TAG
> >                 setup_serial_tag(&params);
> >  #endif
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
> 
> 
> 
> -- 
> Otavio Salvador                             O.S. Systems
> E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
> Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br
Wolfgang Denk - Nov. 20, 2012, 6:24 p.m.
Dear Lee Jones,

In message <1353422034-28107-6-git-send-email-lee.jones@linaro.org> you wrote:
> This patch adds support for passing boot time information to
> the Linus kernel using ATAGS when booting on ARM based devices.

I implicitly mentioned this before, here it comes clear again:

I dislike the idea of adding such infrastructure in an archittecture
dependent way, knowing from day one that we cannot use this as is for
other architectures, and that the mechanism being used is even going
to go away for this architecutre, too.

Please come up with a solution that works for all architectures
instead.

Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 9:17 a.m.
Hi Wolfgang,

> In message <1353422034-28107-6-git-send-email-lee.jones@linaro.org> you wrote:
> > This patch adds support for passing boot time information to
> > the Linus kernel using ATAGS when booting on ARM based devices.
> 
> I implicitly mentioned this before, here it comes clear again:

Ah, this has been tried before? Sorry, I didn't know that.

> I dislike the idea of adding such infrastructure in an archittecture
> dependent way, knowing from day one that we cannot use this as is for
> other architectures, and that the mechanism being used is even going
> to go away for this architecutre, too.
> 
> Please come up with a solution that works for all architectures
> instead.

So I guess Device Tree it is then.

Kind regards,
Lee
Wolfgang Denk - Nov. 21, 2012, 9:30 a.m.
Dear Lee Jones,

In message <20121121091717.GG28265@gmail.com> you wrote:
> Hi Wolfgang,
> 
> > In message <1353422034-28107-6-git-send-email-lee.jones@linaro.org> you wrote:
> > > This patch adds support for passing boot time information to
> > > the Linus kernel using ATAGS when booting on ARM based devices.
> > 
> > I implicitly mentioned this before, here it comes clear again:
> 
> Ah, this has been tried before? Sorry, I didn't know that.

I expolained it in my reply to your cover letter, i.e. in the message
immediately preceeding the one you replied to here.

> > I dislike the idea of adding such infrastructure in an archittecture
> > dependent way, knowing from day one that we cannot use this as is for
> > other architectures, and that the mechanism being used is even going
> > to go away for this architecutre, too.
> > 
> > Please come up with a solution that works for all architectures
> > instead.
> 
> So I guess Device Tree it is then.

No.  The device tree is for passing hardware information to U-Boot and
the kernel.  It is NOT intended for carrying things like debug or
timing logs.  It is not a good idea to misuse such services for things
they were not made for nor where they fit.

Please use a standard facility, and one designed for such purposes
like the Linux log buffer for this purpose. As explained, this has
the added benefit that you don't need to change any Linux code. And
you can build on the (also existing) show_boot_progress() support in
U-Boot, so the extesions should actually be really small and pretty
clear.

Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 10:13 a.m.
> > > > This patch adds support for passing boot time information to
> > > > the Linus kernel using ATAGS when booting on ARM based devices.
> > > 
> > > I implicitly mentioned this before, here it comes clear again:
> > 
> > Ah, this has been tried before? Sorry, I didn't know that.
> 
> I expolained it in my reply to your cover letter, i.e. in the message
> immediately preceeding the one you replied to here.

So you're telling me off for sending a patch which doesn't agree with 
something you've said, despite you saying it _after_ I sent the patch?

Sounds sensible. :)

> > > I dislike the idea of adding such infrastructure in an archittecture
> > > dependent way, knowing from day one that we cannot use this as is for
> > > other architectures, and that the mechanism being used is even going
> > > to go away for this architecutre, too.
> > > 
> > > Please come up with a solution that works for all architectures
> > > instead.
> > 
> > So I guess Device Tree it is then.
> 
> No.  The device tree is for passing hardware information to U-Boot and
> the kernel.  It is NOT intended for carrying things like debug or
> timing logs.  It is not a good idea to misuse such services for things
> they were not made for nor where they fit.

Okay, got it.

> Please use a standard facility, and one designed for such purposes
> like the Linux log buffer for this purpose. As explained, this has
> the added benefit that you don't need to change any Linux code. And
> you can build on the (also existing) show_boot_progress() support in
> U-Boot, so the extesions should actually be really small and pretty
> clear.

When you say log buffer, do you mean __log_buf? Doesn't this contain
logs used for dmesg; thus won't all this crud end up in a user's
dmesg kernel log? Unless there is another log which is used only
for the kernel.

Also, wouldn't I then have to write a text parser to process this
information? Sounds horrendous. Hopefully, I have missed something
and it's actually easier than what I've mentioned.
Wolfgang Denk - Nov. 21, 2012, 1:58 p.m.
Dear Lee Jones,

In message <20121121101310.GL28265@gmail.com> you wrote:
>
> > I expolained it in my reply to your cover letter, i.e. in the message
> > immediately preceeding the one you replied to here.
> 
> So you're telling me off for sending a patch which doesn't agree with 
> something you've said, despite you saying it _after_ I sent the patch?
> 
> Sounds sensible. :)

Arghh... you don't _want_ to understand, right?

I was referring to my reply to your cover letter (patch 0/8) within
this very patch series.  It makes little sense to repeat what I've
already told you just one or two messages before, or does it?

> > like the Linux log buffer for this purpose. As explained, this has
> > the added benefit that you don't need to change any Linux code. And
> > you can build on the (also existing) show_boot_progress() support in
> > U-Boot, so the extesions should actually be really small and pretty
> > clear.
> 
> When you say log buffer, do you mean __log_buf? Doesn't this contain
> logs used for dmesg; thus won't all this crud end up in a user's
> dmesg kernel log? Unless there is another log which is used only
> for the kernel.

Yes, I do mean __log_buf resp. the syslog services.

Yes, this will end up in the log buffer than can be displayed with
dmesg.

If you consider this information "crud", then consider to disable the
feature.

But then, guess why highres timestamps have been added to the kenrel
logs?  For people not interested in such informtion this is eventually
"crud", but for others it appears important enough that it got added
to Linux mainline.

If you are not interested in such information, then just use
appropriate log levels and filtering.

> Also, wouldn't I then have to write a text parser to process this
> information? Sounds horrendous. Hopefully, I have missed something
> and it's actually easier than what I've mentioned.

Guess how many tools are out there that already deal with filtering
and post-processing of kernel log messages?  A google search for
"syslog filter" returns millions of hits...

Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 2:39 p.m.
> > > I expolained it in my reply to your cover letter, i.e. in the message
> > > immediately preceeding the one you replied to here.
> > 
> > So you're telling me off for sending a patch which doesn't agree with 
> > something you've said, despite you saying it _after_ I sent the patch?
> > 
> > Sounds sensible. :)
> 
> Arghh... you don't _want_ to understand, right?
> 
> I was referring to my reply to your cover letter (patch 0/8) within
> this very patch series.  It makes little sense to repeat what I've
> already told you just one or two messages before, or does it?

I think this is meerly a communication issue. I took "I implicitly 
mentioned this before, here it comes clear again", to mean "I've
told you already, why aren't you listening to me".

> > > like the Linux log buffer for this purpose. As explained, this has
> > > the added benefit that you don't need to change any Linux code. And
> > > you can build on the (also existing) show_boot_progress() support in
> > > U-Boot, so the extesions should actually be really small and pretty
> > > clear.
> > 
> > When you say log buffer, do you mean __log_buf? Doesn't this contain
> > logs used for dmesg; thus won't all this crud end up in a user's
> > dmesg kernel log? Unless there is another log which is used only
> > for the kernel.
> 
> Yes, I do mean __log_buf resp. the syslog services.
> 
> Yes, this will end up in the log buffer than can be displayed with
> dmesg.
> 
> If you consider this information "crud", then consider to disable the
> feature.

It's only "curd" to the user typing `dmesg`. If we're trying to
measure whole system boot-up time, it's useful information.

> But then, guess why highres timestamps have been added to the kenrel
> logs?  For people not interested in such informtion this is eventually
> "crud", but for others it appears important enough that it got added
> to Linux mainline.
> 
> If you are not interested in such information, then just use
> appropriate log levels and filtering.

I think the kernel log is the wrong place for this to go. Although,
the kernel driver will allow you to print the information in a log
format by cat'ing <debugfs>/boottime/bootgraph, it's not really
kernel logging information. It's mearly a collection of trace-points
containing a timestamp and some means of identification.

Filling the kernel log with lots of trace-points is definitely wrong.

> > Also, wouldn't I then have to write a text parser to process this
> > information? Sounds horrendous. Hopefully, I have missed something
> > and it's actually easier than what I've mentioned.
> 
> Guess how many tools are out there that already deal with filtering
> and post-processing of kernel log messages?  A google search for
> "syslog filter" returns millions of hits...

So you're suggesting that we create a userland portion of the driver
too? I don't think this is acceptable. This tool will be used by
kernel engineers, who would be more happy taking the information from
debugfs. At least I know I would.
Wolfgang Denk - Nov. 21, 2012, 4:05 p.m.
Dear Lee Jones,

In message <20121121143928.GA28899@gmail.com> you wrote:
>
> > If you are not interested in such information, then just use
> > appropriate log levels and filtering.
> 
> I think the kernel log is the wrong place for this to go. Although,

OK, this is your opinion, then, and I will respect it.

It is my opinion that mechanisms as ATAGS or device tree are
inappropriate for passing any kind of log data to the kernel.

So far, the established way of passing logging information (like
results of Power-On Selft Tests,e tc.) is through a shared log buffer.

> the kernel driver will allow you to print the information in a log
> format by cat'ing <debugfs>/boottime/bootgraph, it's not really
> kernel logging information. It's mearly a collection of trace-points
> containing a timestamp and some means of identification.

I don't see what a kernel driver may be needed for, but for this part
of the discussion this is not relevant anyway.


> Filling the kernel log with lots of trace-points is definitely wrong.

Again, this is your opinion, and I respect it.  On the other hand,
what is the kernel log being used for on log level INFO, for example?

> So you're suggesting that we create a userland portion of the driver
> too? I don't think this is acceptable. This tool will be used by
> kernel engineers, who would be more happy taking the information from
> debugfs. At least I know I would.

I do not suggest anything like that.  I suggest not to write any
driver at all, nor any other code.  I suggest to use existing tools,
as I recommend to reuse existing (standard) methods and protocols
instead of inventing new ones.


Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 5:48 p.m.
> > > If you are not interested in such information, then just use
> > > appropriate log levels and filtering.
> > 
> > I think the kernel log is the wrong place for this to go. Although,
> 
> OK, this is your opinion, then, and I will respect it.

Thank you, and I yours.

> It is my opinion that mechanisms as ATAGS or device tree are
> inappropriate for passing any kind of log data to the kernel.

I agree with you.

> So far, the established way of passing logging information (like
> results of Power-On Selft Tests,e tc.) is through a shared log buffer.

Also true, but is that data used in this way? Or is it just
printed out at boot time? This tool aims to gather all boot
time statistics in one, easy to access place so that 
(generally kernel) engineers may make improvements based on
the data provided.

> > the kernel driver will allow you to print the information in a log
> > format by cat'ing <debugfs>/boottime/bootgraph, it's not really
> > kernel logging information. It's mearly a collection of trace-points
> > containing a timestamp and some means of identification.
> 
> I don't see what a kernel driver may be needed for, but for this part
> of the discussion this is not relevant anyway.

I've already had lots of interest in this from the kernel
community, but if it can't make use of the bootloader portion
easily, then it just becomes another tracing/bootchart
mechanism.

> > Filling the kernel log with lots of trace-points is definitely wrong.
> 
> Again, this is your opinion, and I respect it.  On the other hand,
> what is the kernel log being used for on log level INFO, for example?

If I print my current bootgraph it's currently 507 lines. This
should not go into the kernel log at any level. Albeit almost
all of them are kernel related entries, I've already mentioned
that I don't want two different mechanisms for storing 
bootloader and kernel entries. It's very rare that you'd even
need to print it out, but when you do it's there in debugfs.

> > So you're suggesting that we create a userland portion of the driver
> > too? I don't think this is acceptable. This tool will be used by
> > kernel engineers, who would be more happy taking the information from
> > debugfs. At least I know I would.
> 
> I do not suggest anything like that.  I suggest not to write any
> driver at all, nor any other code.  I suggest to use existing tools,
> as I recommend to reuse existing (standard) methods and protocols
> instead of inventing new ones.

You're suggesting that we parse the log with a bespoke text
parsing tool in order to get the information we need. However,
this would a) require us to overwhelm the kernel's log and b)
We'd have to maintain a separate script or app that would be
capable of the task. I'm just not happy with the implementation.

To have something in the kernel, which does all this automagically
would be much simpler for the user. This tool was created to solve
a real problem. Kernel engineers do not currently have an easy way
to trace booting time from bootloader. This is the solution to that
problem.

1. Enable boottime config
2. Enable debugfs config      (if it's not already)
3. Mount debugfs              (if it's not already)
4. cat /sys/kernel/debug/[summary|bootgraph]

Simples.
Wolfgang Denk - Nov. 21, 2012, 7:18 p.m.
Dear Lee Jones,

In message <20121121174856.GC417@gmail.com> you wrote:
>
> > So far, the established way of passing logging information (like
> > results of Power-On Selft Tests,e tc.) is through a shared log buffer.
> 
> Also true, but is that data used in this way? Or is it just
> printed out at boot time? This tool aims to gather all boot

It is definitely used that way.  For example, there are systems that
parse the syslog output for specific POST results, and prevent certain
functionality to be enabled when the POST detected problems with the
hardware, or other conditions that require specific actions.

> time statistics in one, easy to access place so that 
> (generally kernel) engineers may make improvements based on
> the data provided.

And it has to be implemented using a completely new method, because
all existing ones are crap or not the "correct way" to do it.  We've
been there before.

> If I print my current bootgraph it's currently 507 lines. This
> should not go into the kernel log at any level. Albeit almost

"This should not".  OK, this is your decision, whatever the reasoning
for it may be, and whatever others may think about it.

I accept this, but please also accept that I ask you not to add 
code that duplicates existing functionality to U-Boot.

> parsing tool in order to get the information we need. However,
> this would a) require us to overwhelm the kernel's log and b)
> We'd have to maintain a separate script or app that would be
> capable of the task. I'm just not happy with the implementation.
> 
> To have something in the kernel, which does all this automagically
> would be much simpler for the user. This tool was created to solve
> a real problem. Kernel engineers do not currently have an easy way
> to trace booting time from bootloader. This is the solution to that
> problem.

Again, this is your opinion.  My strong believe is that it is almost
always much more advisable to implement such functionality not in the
kernel, but in user space.  There are certain things that belong to
the kernel, but everything else should not be done there.  Gathering
and processing statistical information, generating charts and the
like are nothing I consider typical kernel tasks.

But we're off topic here - this is the U-Boot mailing list, and I'm
not in a position to citizise the design of your kernel code.

But as much as you "don't want two different mechanisms for storing 
bootloader and kernel entries" I don't want that either.  And that is
the reason to reject these patches, asking for a change of the
implementation to use the already existing methods.

Best regards,

Wolfgang Denk
Lee Jones - Nov. 22, 2012, 10:14 a.m.
Let's try to move this forward.

> And it has to be implemented using a completely new method, because
> all existing ones are crap or not the "correct way" to do it.  We've
> been there before.

> I accept this, but please also accept that I ask you not to add 
> code that duplicates existing functionality to U-Boot.

> But as much as you "don't want two different mechanisms for storing 
> bootloader and kernel entries" I don't want that either.  And that is
> the reason to reject these patches, asking for a change of the
> implementation to use the already existing methods.

Okay, to summarise so far:

1. Bootloader and kernel mechanisms should be the same

So putting bootloader tracepoints in the bootlog and the kernel's
in an internal data structure is not acceptable, as it would add
too much extra overhead to link them together and parse.

2. The kernel bootlog is not the correct place for tracepoints

If we were to adhere to point No1 and bootloader & kernel
entries would be placed into the bootlog; no self respecting
kernel engineer/maintainer will allow 100's of spurious
tracepoint entries in the kernel log, regardless of log level.
No other tracing mechanism does this and there's a good reason
for that.

3. Existing mechanisms to be used if they exist and are suitable

I am more than happy to use anything that already exists, but I
haven't seen anything yet. I can see how we could use the
show_boot_progress() call-back, but we'd have to use #defines
to store anything useful with regards to unique and readable 
identifiers. Also, we'd have to be careful to use it in such a
what that it wouldn't trash any architecture's implementation,
which would probably mean calling boottime_tag from within them.

As for passing the information to the kernel; munging it as
text and sticking it in __log_buf is out of the question, ATAGs
are architecture specific and are leaving us and DT is designed
to carry hardware information. So where does that leave us?

Actually, putting it in DT has lots of advantages. 1) DT works
cross-architecture and cross-platform, so your architecture
independent box is inherently ticked 2) DT already carries
non-hardware specific information such as the kernel cmdline.
I don't see how adding <10 (but would more likely to be 2-3)
tracepoint entries would completely break convention. We can
either get the kernel driver to scrub the entries if you'd be
that offended by keeping them in.

Let's be pragmatic about this. I'm happy to make changes to
the current implementation and code-up any sensible solutions
to this issue. It's a real problem that has gained a great
deal of interest amongst my kernel engineer peers. So much
so that people have stopped to ask me about it at kernel
conferences. Let's find a nice, long lasting way to solve it.
Wolfgang Denk - Nov. 22, 2012, 1:04 p.m.
Dear Lee Jones,

In message <20121122101433.GA4328@gmail.com> you wrote:
> Let's try to move this forward.

Actually you don't.  You insist on not changing anything on your side,
and ask others to adapt to it.

> Okay, to summarise so far:
> 
> 1. Bootloader and kernel mechanisms should be the same
> 
> So putting bootloader tracepoints in the bootlog and the kernel's
> in an internal data structure is not acceptable, as it would add
> too much extra overhead to link them together and parse.

OK.  But this appears a new requirement - your original implementation
did not bother about this, using ATAGs here and somethign else
there...

> 2. The kernel bootlog is not the correct place for tracepoints
> 
> If we were to adhere to point No1 and bootloader & kernel
> entries would be placed into the bootlog; no self respecting
> kernel engineer/maintainer will allow 100's of spurious
> tracepoint entries in the kernel log, regardless of log level.

I wonder about the self-assuredness you speak for all of them.  Has
this been dicussed in full context before?

> Actually, putting it in DT has lots of advantages. 1) DT works
> cross-architecture and cross-platform, so your architecture
> independent box is inherently ticked 2) DT already carries
> non-hardware specific information such as the kernel cmdline.
> I don't see how adding <10 (but would more likely to be 2-3)
> tracepoint entries would completely break convention. We can
> either get the kernel driver to scrub the entries if you'd be
> that offended by keeping them in.

Hm... in accordance to No. 1 above your kernel code will add new
entries to the device tree while the kernel is booting?  So instead of
"adding <10" it now will be adding "100's of spurious tracepoint
entries" to the DT?

Are you sure this is a good idea?  [Not considering if it's actually
possible.]

But then, if you drop item 1, then what's wrong with "<10 (but would
more likely to be 2-3)" additional log messages?


Best regards,

Wolfgang Denk
Lee Jones - Nov. 22, 2012, 4:08 p.m.
> > Let's try to move this forward.
> 
> Actually you don't.  You insist on not changing anything on your side,
> and ask others to adapt to it.

Not at all.

This current implementation is unacceptable to you and your counter-
suggestion is unacceptable to me (and all other kernel engineers).

I'm seeking a new avenue. 

> > Okay, to summarise so far:
> > 
> > 1. Bootloader and kernel mechanisms should be the same
> > 
> > So putting bootloader tracepoints in the bootlog and the kernel's
> > in an internal data structure is not acceptable, as it would add
> > too much extra overhead to link them together and parse.
> 
> OK.  But this appears a new requirement - your original implementation
> did not bother about this, using ATAGs here and somethign else
> there...

No, they're the same in the current implementation:

  struct tag_boottime {
         struct boottime_entry entry[BOOTTIME_MAX];
         u32 idle;  /* in us */
         u32 total; /* in us */
         u8 num;
  };

ATAGs is mearly a way to pass the data structure through.

Once the data is passed to though, the kernel then just populates
the structure with bootloader and kernel tracepoints alike. No
differentiation is made between the two.

> > 2. The kernel bootlog is not the correct place for tracepoints
> > 
> > If we were to adhere to point No1 and bootloader & kernel
> > entries would be placed into the bootlog; no self respecting
> > kernel engineer/maintainer will allow 100's of spurious
> > tracepoint entries in the kernel log, regardless of log level.
> 
> I wonder about the self-assuredness you speak for all of them.  Has
> this been dicussed in full context before?

It's just not logical. 

Putting 100's of tracepoints in the kernel log is insane. 

> > Actually, putting it in DT has lots of advantages. 1) DT works
> > cross-architecture and cross-platform, so your architecture
> > independent box is inherently ticked 2) DT already carries
> > non-hardware specific information such as the kernel cmdline.
> > I don't see how adding <10 (but would more likely to be 2-3)
> > tracepoint entries would completely break convention. We can
> > either get the kernel driver to scrub the entries if you'd be
> > that offended by keeping them in.
> 
> Hm... in accordance to No. 1 above your kernel code will add new
> entries to the device tree while the kernel is booting?  So instead of
> "adding <10" it now will be adding "100's of spurious tracepoint
> entries" to the DT?
>
> Are you sure this is a good idea?  [Not considering if it's actually
> possible.]
> 
> But then, if you drop item 1, then what's wrong with "<10 (but would
> more likely to be 2-3)" additional log messages?

Granted, I can understand the points above.

If it's feasible to put them in the buffer, parse it, then scrub
them so they don't appear in dmesg output, I guess that could be
doable.

Ideally I'd like to keep it all as data, as it will save lots of
text parsing code in the kernel. Surely there must be a call for
passing data structures from the bootloader to the kernel. After
all, that's why ATAGs were brought about wasn't it?
Wolfgang Denk - Nov. 22, 2012, 5:40 p.m.
Dear Lee Jones,

In message <20121122160847.GD10986@gmail.com> you wrote:
>
> Ideally I'd like to keep it all as data, as it will save lots of
> text parsing code in the kernel. Surely there must be a call for
> passing data structures from the bootloader to the kernel. After
> all, that's why ATAGs were brought about wasn't it?

Look at which ATAGS exist to see what they have been invented for.
Read the previous discussions why for example pretty useful
extensions like a tag to pass a MAC address from a boot loader to the
kernel have never been accepted for mainline.  But you claim that
trace data are different, and fit better?

I consider it a design flaw to do such statictics stuff in the kernel.
It does not belong there.  Such functions belong to user space.

But as mentioned before, this is actually off topic here.

Best regards,

Wolfgang Denk
Lee Jones - Nov. 23, 2012, 10:08 a.m.
> > Ideally I'd like to keep it all as data, as it will save lots of
> > text parsing code in the kernel. Surely there must be a call for
> > passing data structures from the bootloader to the kernel. After
> > all, that's why ATAGs were brought about wasn't it?
> 
> Look at which ATAGS exist to see what they have been invented for.
> Read the previous discussions why for example pretty useful
> extensions like a tag to pass a MAC address from a boot loader to the
> kernel have never been accepted for mainline.  But you claim that
> trace data are different, and fit better?

I don't know the history.

> I consider it a design flaw to do such statictics stuff in the kernel.
> It does not belong there.  Such functions belong to user space.

I don't agree.

> But as mentioned before, this is actually off topic here.

Then stop mentioning it. ;)

Patch

diff --git a/arch/arm/include/asm/setup.h b/arch/arm/include/asm/setup.h
index 78a7fac..6088440 100644
--- a/arch/arm/include/asm/setup.h
+++ b/arch/arm/include/asm/setup.h
@@ -205,6 +205,19 @@  struct tag_memclk {
 	u32 fmemclk;
 };
 
+/* for automatic boot timing testcases */
+#define ATAG_BOOTTIME  0x41000403
+#define BOOTTIME_MAX 10
+
+#include <boottime.h>
+
+struct tag_boottime {
+	struct boottime_entry entry[BOOTTIME_MAX];
+	u32 idle;  /* in us */
+	u32 total; /* in us */
+	u8 num;
+};
+
 struct tag {
 	struct tag_header hdr;
 	union {
@@ -227,6 +240,11 @@  struct tag {
 		 * DC21285 specific
 		 */
 		struct tag_memclk	memclk;
+
+		/*
+		 * Boot time
+		 */
+		struct tag_boottime     boottime;
 	} u;
 };
 
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 1bd2730..03774c8 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -28,6 +28,7 @@ 
 #include <common.h>
 #include <command.h>
 #include <image.h>
+#include <boottime.h>
 #include <u-boot/zlib.h>
 #include <asm/byteorder.h>
 #include <fdt.h>
@@ -114,7 +115,8 @@  static void announce_and_cleanup(void)
 	defined(CONFIG_CMDLINE_TAG) || \
 	defined(CONFIG_INITRD_TAG) || \
 	defined(CONFIG_SERIAL_TAG) || \
-	defined(CONFIG_REVISION_TAG)
+	defined(CONFIG_REVISION_TAG) || \
+	defined(CONFIG_BOOTTIME)
 static void setup_start_tag (bd_t *bd)
 {
 	params = (struct tag *)bd->bi_boot_params;
@@ -130,6 +132,37 @@  static void setup_start_tag (bd_t *bd)
 }
 #endif
 
+#ifdef CONFIG_BOOTTIME
+static void setup_boottime_tags(void)
+{
+	unsigned int i;
+	struct boottime_entry *b;
+
+	params->hdr.tag = ATAG_BOOTTIME;
+	params->hdr.size = tag_size(tag_boottime);
+
+	params->u.boottime.idle = boottime_idle_get();
+	params->u.boottime.total = boottime_idle_done();
+
+	for (i = 0; i < BOOTTIME_MAX; i++) {
+		b = boottime_get_entry(i);
+		if (b == NULL)
+			break;
+
+		params->u.boottime.entry[i].time = b->time;
+		strncpy((char *)params->u.boottime.entry[i].name,
+			(char *)b->name, BOOTTIME_MAX_NAME_LEN);
+		params->u.boottime.entry[i].name[BOOTTIME_MAX_NAME_LEN - 1] = '\0';
+
+	}
+
+	params->u.boottime.num = i;
+
+	params = tag_next(params);
+
+}
+#endif
+
 #ifdef CONFIG_SETUP_MEMORY_TAGS
 static void setup_memory_tags(bd_t *bd)
 {
@@ -233,6 +266,10 @@  static void setup_end_tag(bd_t *bd)
 }
 #endif
 
+#ifdef CONFIG_BOOTTIME
+static void setup_boottime_tags(void);
+#endif
+
 #ifdef CONFIG_OF_LIBFDT
 static int create_fdt(bootm_headers_t *images)
 {
@@ -293,9 +330,13 @@  static void boot_prep_linux(bootm_headers_t *images)
 	defined(CONFIG_CMDLINE_TAG) || \
 	defined(CONFIG_INITRD_TAG) || \
 	defined(CONFIG_SERIAL_TAG) || \
-	defined(CONFIG_REVISION_TAG)
+	defined(CONFIG_REVISION_TAG) || \
+	defined (CONFIG_BOOTTIME)
 		debug("using: ATAGS\n");
 		setup_start_tag(gd->bd);
+#ifdef CONFIG_BOOTTIME
+		setup_boottime_tags();
+#endif
 #ifdef CONFIG_SERIAL_TAG
 		setup_serial_tag(&params);
 #endif