| Message ID | 1353422034-28107-6-git-send-email-lee.jones@linaro.org |
|---|---|
| State | Rejected |
| Delegated to: | Wolfgang Denk |
| Headers | show |
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(¶ms); > #endif > -- > 1.7.9.5 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
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(¶ms); > > #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
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
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
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
> > > > 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.
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
> > > 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.
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
> > > 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.
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
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.
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
> > 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?
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
> > 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. ;)
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(¶ms); #endif
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(-)