Patchwork [U-Boot,4/8] boottime: Apply some key boottime tags into common code

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

Comments

Lee Jones - Nov. 20, 2012, 2:33 p.m.
Here we add boottime tags to the start of the main loop and just
before the opportunity to break into the u-boot shell. This will
provide a more verbose bootgraph when viewed within debugfs.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 common/main.c |    5 +++++
 1 file changed, 5 insertions(+)
Wolfgang Denk - Nov. 20, 2012, 6:22 p.m.
Dear Lee Jones,

In message <1353422034-28107-5-git-send-email-lee.jones@linaro.org> you wrote:
> Here we add boottime tags to the start of the main loop and just
> before the opportunity to break into the u-boot shell. This will
> provide a more verbose bootgraph when viewed within debugfs.

Assuming we would take this route - then why do we need to add new
boottime_tag() entries - is there anything wrong with the existing
show_boot_progress() code? Did you consider using this instead? If
so, why did you not use it?

Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 9:36 a.m.
On Tue, 20 Nov 2012, Wolfgang Denk wrote:

> Dear Lee Jones,
> 
> In message <1353422034-28107-5-git-send-email-lee.jones@linaro.org> you wrote:
> > Here we add boottime tags to the start of the main loop and just
> > before the opportunity to break into the u-boot shell. This will
> > provide a more verbose bootgraph when viewed within debugfs.
> 
> Assuming we would take this route - then why do we need to add new
> boottime_tag() entries - is there anything wrong with the existing
> show_boot_progress() code? Did you consider using this instead? If
> so, why did you not use it?

No, I didn't know it existed. Basically I've taken responsibility to
upstream someone else's driver. It's more of a kernel thing, but it
requires boottime information from u-boot too, as the intention is
to cover full system booting, rather than the kernel-only tracing
mechanisms which already exist.

I've just taken a look at show_boot_process() though. It doesn't
appear to be suitable for what we require. Each tag needs to be
individually identifiable, and I'm not sure how you would achieve
this if we were to call back into a single function which would do
the tagging.
Wolfgang Denk - Nov. 21, 2012, 1:40 p.m.
Dear Lee Jones,

In message <20121121093647.GH28265@gmail.com> you wrote:
> 
> > show_boot_progress() code? Did you consider using this instead? If
> > so, why did you not use it?
> 
> No, I didn't know it existed. Basically I've taken responsibility to
> upstream someone else's driver. It's more of a kernel thing, but it

This shouldbe considered a design fault.

Why do you need an extra driver when standard mechanisms exist that
provide exactly the needed funtionality?

> requires boottime information from u-boot too, as the intention is
> to cover full system booting, rather than the kernel-only tracing
> mechanisms which already exist.

But we can share a log buffer with Linux, both hence and back, so why
do you not simply use this feature?

> I've just taken a look at show_boot_process() though. It doesn't
> appear to be suitable for what we require. Each tag needs to be
> individually identifiable, and I'm not sure how you would achieve
> this if we were to call back into a single function which would do
> the tagging.

Each call takes an argument which is exactly used for such
identification purposes.  And you implementation can be mapped to
write syslog entries into a shared (with Linux) log buffer, so no
changes in Linux are needed.


Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 3:07 p.m.
> > > show_boot_progress() code? Did you consider using this instead? If
> > > so, why did you not use it?
> > 
> > No, I didn't know it existed. Basically I've taken responsibility to
> > upstream someone else's driver. It's more of a kernel thing, but it
> 
> This shouldbe considered a design fault.
> 
> Why do you need an extra driver when standard mechanisms exist that
> provide exactly the needed funtionality?
> 
> > requires boottime information from u-boot too, as the intention is
> > to cover full system booting, rather than the kernel-only tracing
> > mechanisms which already exist.
> 
> But we can share a log buffer with Linux, both hence and back, so why
> do you not simply use this feature?
> 
> > I've just taken a look at show_boot_process() though. It doesn't
> > appear to be suitable for what we require. Each tag needs to be
> > individually identifiable, and I'm not sure how you would achieve
> > this if we were to call back into a single function which would do
> > the tagging.
> 
> Each call takes an argument which is exactly used for such
> identification purposes.  And you implementation can be mapped to
> write syslog entries into a shared (with Linux) log buffer, so no
> changes in Linux are needed.

It looked to me as though it took an integer identifier, which
isn't going to mean anything to anyone. Unless there is a way to 
change the semantics of the function so that it would take a string,
but then how would it play with the existing show_boot_progress()
calls?

Patch

diff --git a/common/main.c b/common/main.c
index 592ce07..84c88e9 100644
--- a/common/main.c
+++ b/common/main.c
@@ -40,6 +40,7 @@ 
 #include <hush.h>
 #endif
 
+#include <boottime.h>
 #include <post.h>
 #include <linux/ctype.h>
 #include <menu.h>
@@ -219,6 +220,8 @@  int abortboot(int bootdelay)
 {
 	int abort = 0;
 
+	boottime_tag("autoboot_delay");
+
 #ifdef CONFIG_MENUPROMPT
 	printf(CONFIG_MENUPROMPT);
 #else
@@ -299,6 +302,8 @@  void main_loop (void)
 	char bcs_set[16];
 #endif /* CONFIG_BOOTCOUNT_LIMIT */
 
+	boottime_tag("main_loop");
+
 #ifdef CONFIG_BOOTCOUNT_LIMIT
 	bootcount = bootcount_load();
 	bootcount++;