Adding boottime support

If we attempt to take a 32bit timer value and multiply it by a significant number, the core will not be able to handle it. This gives the illusion that the timer is rolling over, when in fact this is not the case. If we ensure the division in this instance is carried out before the multiplication, the issue vanishes.

Quoted 5 lines

If we attempt to take a 32bit timer value and multiply it by a significant number, the core will not be able to handle it. This gives the illusion that the timer is rolling over, when in fact this is not the case. If we ensure the division in this instance is carried out before the multiplication, the issue vanishes.

Are you sure this is a good idea?

Quoted 4 lines

arch/arm/cpu/armv7/u8500/timer.c +0 -0

@@ -70,7 +70,7 @@ struct u8500_mtu {
  * The MTU is clocked at 133 MHz by default. (V1 and later)

arch/arm/cpu/armv7/u8500/timer.c struct u8500_mtu { · line 73

Quoted 4 lines

arch/arm/cpu/armv7/u8500/timer.c +1 -1

  */
 #define TIMER_CLOCK		(133 * 1000 * 1000 / 16)
-#define COUNT_TO_USEC(x)	((x) * 16 / 133)
+#define COUNT_TO_USEC(x)	((x) / 133 * 16)

Before the change, the result is useful for all values of x in the interval from 0 through UINT_MAX/16 = 268435455 = 268 seconds, i. e. for all values that make sense to be measured in microseconds.

The original implementation is fine until we reach 32.28 seconds, which as you predicted is 0x1000_0000:

0x10000000 * PRESCALER) / (CLOCK_SPEED_133_MHZ)
(268435456 * 16 ) / (133 * 1000 * 1000) == 32.28 seconds

If we spend >30 seconds at the u-boot commandline, which is not unreasonable by any stretch, then the kernel assumes responsibility for the remaining of the time spent at the prompt, which is obviously not acceptable for this usecase.

I doubt x will never be < 133, as that will be 16 micro-seconds post timer-start or role-over. Do we really need that degree of resolution?

After the change, the result is changed to the worse for all low values of X, especially for the ranges from 16 through 132.

You lose accuracy here, and win nothing.

This makes no sense to me.

If you need a bigger number range, then use proper arithmetics. It';s available, just use it.

Quoted 7 lines

arch/arm/cpu/armv7/u8500/timer.c struct u8500_mtu { · line 73

Quoted 3 lines

arch/arm/cpu/armv7/u8500/timer.c +1 -1

-#define COUNT_TO_USEC(x)	((x) * 16 / 133)
+#define COUNT_TO_USEC(x)	((x) / 133 * 16)

Before the change, the result is useful for all values of x in the interval from 0 through UINT_MAX/16 = 268435455 = 268 seconds, i. e. for all values that make sense to be measured in microseconds.

We're actually discussing this elsewhere. I don't have permission to paste the other side of the conversation, but I can show you my calculations:

Quoted 10 lines

The original implementation is fine until we reach 32.28 seconds, which as you predicted is 0x1000_0000:

0x10000000 * PRESCALER) / (CLOCK_SPEED_133_MHZ)
(268435456 * 16 ) / (133 * 1000 * 1000) == 32.28 seconds

If we spend >30 seconds at the u-boot commandline, which is not unreasonable by any stretch, then the kernel assumes responsibility for the remaining of the time spent at the prompt, which is obviously not acceptable for this usecase.

Quoted 2 lines

If you need a bigger number range, then use proper arithmetics. It';s available, just use it.

Would you be able to lend a hand here, as I'm no mathematician?

What are the correct arithmetics?

Quoted 17 lines

Quoted 6 lines

arch/arm/cpu/armv7/u8500/timer.c struct u8500_mtu { · line 73

Quoted 2 lines

arch/arm/cpu/armv7/u8500/timer.c +1 -1

-#define COUNT_TO_USEC(x)	((x) * 16 / 133)
+#define COUNT_TO_USEC(x)	((x) / 133 * 16)

Before the change, the result is useful for all values of x in the interval from 0 through UINT_MAX/16 = 268435455 = 268 seconds, i. e. for all values that make sense to be measured in microseconds.

We're actually discussing this elsewhere. I don't have permission to paste the other side of the conversation, but I can show you my calculations:

Quoted 5 lines

The original implementation is fine until we reach 32.28 seconds, which as you predicted is 0x1000_0000:

0x10000000 * PRESCALER) / (CLOCK_SPEED_133_MHZ)
(268435456 * 16 ) / (133 * 1000 * 1000) == 32.28 seconds

Quoted 4 lines

Quoted 4 lines

If we spend >30 seconds at the u-boot commandline, which is not unreasonable by any stretch, then the kernel assumes responsibility for the remaining of the time spent at the prompt, which is obviously not acceptable for this usecase.

This makes no sense to me. An overflow will not happen before UINT_MAX/16 = 268435455 = 268 seconds.

Anyway. If you have overflof problems, then use proper arithmetics.

Quoted 6 lines

Quoted 2 lines

If you need a bigger number range, then use proper arithmetics. It';s available, just use it.

Would you be able to lend a hand here, as I'm no mathematician?

What are the correct arithmetics?

There are routines like do_div() or lldiv() etc. that can be used when 32 bit arithmetics is really not good enough.

However, I fail to see why that should even be needed here.

Boottime is a tool which can be used for full system booting time measurement. Bootloader boot time is passed to the kernel component though ATAGS. The kernel-side driver then uses this information to provide full system boot time diagnostics though debugfs.

Quoted 4 lines

Boottime is a tool which can be used for full system booting time measurement. Bootloader boot time is passed to the kernel component though ATAGS. The kernel-side driver then uses this information to provide full system boot time diagnostics though debugfs.

Aren't we converting more and more systems to use the device treee to pass information to the kenrel, with the result that ATAGS are kind of becoming extinct?

And forcing something upon a mechanism that was designed for a completely different purpose, where you see right from the first glance that it does not math easily?

This makes no sense to me. Why don't you use standard mechanisms, like a shared log buffer, and simply create time-stamped entries into the kernel boot log?

The advantages should be obvious: we will need no extra kernel modification, we do not depend on ATAGS, and we are automatically architecture-independent.

Yes, I intend to extend this functionality into Device Tree. That way it will be architecture and OS independent.

Quoted 3 lines

And forcing something upon a mechanism that was designed for a completely different purpose, where you see right from the first glance that it does not math easily?

Not entirely sure what you mean here. This mechanism works perfectly with ATAGs.

Quoted 3 lines

The advantages should be obvious: we will need no extra kernel modification, we do not depend on ATAGS, and we are automatically architecture-independent.

Wouldn't this clog up the kernel's log buffer? I'm sure no user wants to see reams of otherwise useless logging scrawled throughout their bootlog. We'd also have a write a text parser to obtain the information for processing. It would be easier to either pass in a struct, as we do with the ATAG mechanism, or though Device Tree as previously discussed.

Quoted 7 lines

Yes, I intend to extend this functionality into Device Tree. That way it will be architecture and OS independent.

Quoted 3 lines

And forcing something upon a mechanism that was designed for a completely different purpose, where you see right from the first glance that it does not math easily?

Quoted 2 lines

Not entirely sure what you mean here. This mechanism works perfectly with ATAGs.

Neither ATAGS not the device tree are intended nor designed for passing logfile information. Yes, you can use them like that, and it will actually work.

You can also drive a nail in using a microscope as hammer.

Quoted 10 lines

Quoted 3 lines

The advantages should be obvious: we will need no extra kernel modification, we do not depend on ATAGS, and we are automatically architecture-independent.

Wouldn't this clog up the kernel's log buffer? I'm sure no user wants to see reams of otherwise useless logging scrawled throughout their bootlog. We'd also have a write a text parser to obtain the information for processing. It would be easier to either pass in a struct, as we do with the ATAG mechanism, or though Device Tree as previously discussed.

I think these are pretty poor arguments. There are standard methods (like log levels) to provide adequate filtering of which messages are passed on to a user. An there exists a plethora of tools for automatic filtering and post-processing of syslog messages. You will need to write _less_ code than with your homebrew implementation.

Quoted 4 lines

Neither ATAGS not the device tree are intended nor designed for passing logfile information. Yes, you can use them like that, and it will actually work.

ATAGs were exactly designed for this type of thing. To pass small data structures though to the kernel. In our case, our trace-points are held in a small data structure. They're not logs.

They're not poor augments if the data stored isn't log messages, which these aren't. If anything I would say that ramming them in as textual kernel messages, then parsing the log text using a userspace tool was an abuse of the system. If we create them as data in the bootloader, then pass them to the kernel as data, then process them as data, _that_ would be the correct mechanism.

Dear Lee Jones,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 4 lines

Neither ATAGS not the device tree are intended nor designed for passing logfile information. Yes, you can use them like that, and it will actually work.

Quoted 3 lines

ATAGs were exactly designed for this type of thing. To pass small data structures though to the kernel. In our case, our trace-points are held in a small data structure. They're not logs.

You appear to have a specific definition of log data in mind. It must be different to mine.

Also, you contradict yourself - here you write "pass small data structures", earlier you wrote about "lots of trace-points", which sounds as if the total amount of data would be not exactly small - actually so big that yu are afraid of annoying users with it.

Anyway. This doesn't take us further.

Quoted 6 lines

They're not poor augments if the data stored isn't log messages, which these aren't. If anything I would say that ramming them in as textual kernel messages, then parsing the log text using a userspace tool was an abuse of the system. If we create them as data in the bootloader, then pass them to the kernel as data, then process them as data, _that_ would be the correct mechanism.

Well, I could pooint out here a number of pretty basic design decisions made earlier in a number of pretty important and successful software projects, like the fact that a large number of internet protocols are based on plain text implementations. Or how useful it is if you can just to a post-mortem dump of the log buffer and actually _read_ the entries, without need to special tools.

I think I should stop here, though. It appears it makes little sense trying to discuss alternative approaches when you have already fixed your mind about the one and only "correct" way to do this.

Quoted 3 lines

You appear to have a specific definition of log data in mind. It must be different to mine.

I would say that a log entry would contain useful descriptive information contained. It is _sometimes_ useful to display a related timestamp too. In our case, the most useful part is the timestamp. I just don't see how riddling the kernel log with timestamps would be useful, or nessersary.

Boottime is a tool which can be used for full system booting time measurement. Bootloader boot time is passed to the kernel component though ATAGS. The kernel-side driver then uses this information to provide full system boot time diagnostics though debugfs.

Based heavily on the original driver by Jonas Aaberg.

Signed-off-by Lee Jones <[email protected]>

Quoted 5 lines

Boottime is a tool which can be used for full system booting time measurement. Bootloader boot time is passed to the kernel component though ATAGS. The kernel-side driver then uses this information to provide full system boot time diagnostics though debugfs.

Quoted 3 lines

Based heavily on the original driver by Jonas Aaberg.

Signed-off-by Lee Jones <[email protected]>

Did you take a look at bootstage, which seems at least on the surface to provide a similar mechanism? This passes the information to the kernel through a device tree, or worse case a 'stash area'.

Hi Simon,

Quoted 10 lines · Lee Jones

Quoted 8 lines

Boottime is a tool which can be used for full system booting time measurement. Bootloader boot time is passed to the kernel component though ATAGS. The kernel-side driver then uses this information to provide full system boot time diagnostics though debugfs.

Based heavily on the original driver by Jonas Aaberg.

Signed-off-by Lee Jones <[email protected]>

Quoted 3 lines

Did you take a look at bootstage, which seems at least on the surface to provide a similar mechanism? This passes the information to the kernel through a device tree, or worse case a 'stash area'.

I didn't see this before.

I don't see the kernel counterpart, did it make it upstream?

Wolfgang, didn't you know about bootstage? This could have saved us a great deal of time.

Kind regards,
Lee

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Quoted 16 lines

Quoted 6 lines · Lee Jones

Quoted 5 lines

Boottime is a tool which can be used for full system booting time measurement. Bootloader boot time is passed to the kernel component though ATAGS. The kernel-side driver then uses this information to provide full system boot time diagnostics though debugfs.

Quoted 7 lines

Quoted 3 lines

Based heavily on the original driver by Jonas Aaberg.

Signed-off-by Lee Jones <[email protected]>

Did you take a look at bootstage, which seems at least on the surface to provide a similar mechanism? This passes the information to the kernel through a device tree, or worse case a 'stash area'.

I didn't see this before.

Quoted 1 line

I don't see the kernel counterpart, did it make it upstream?

It was sent upstream, just for a feeler, but before U-Boot support was mainlined and before we had a way to deal with the non-fdt case. That is now implemented and in mainline, although it has not yet gone out in a release (should be Jan 2013). So I was planning to address that again in the kernel at some point.

By the way, if Wolfgang didn't want these tracepoints in DT, then how was your implementations upstreamed into u-boot?

Dear Lee Jones,

Quoted 3 lines · In message <[email protected]> you wrote:

By the way, if Wolfgang didn't want these tracepoints in DT, then how was your implementations upstreamed into u-boot?

Because I don;t manage a 100% review coverage over all submitted patches, i. e. it escaped my attention (and I'm sorry for that).

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.

Dear Lee Jones,

Quoted 3 lines · In message <[email protected]> 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?

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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?

Quoted 3 lines

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?

Quoted 5 lines

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.

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 <[email protected]>

---
 arch/arm/include/asm/setup.h |   18 +++++++++++++++++
 arch/arm/lib/bootm.c         |   45
++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)

arch/arm/include/asm/setup.h +18 -0

index 78a7fac..6088440 100644
@@ -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;
 };
 

arch/arm/lib/bootm.c +34 -1

index 1bd2730..03774c8 100644
@@ -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)

{

arch/arm/lib/bootm.c +9 -2

@@ -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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 2 lines · Lee Jones

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

Linus or Linux?

Quoted 140 lines

Based heavily on the original driver by Jonas Aaberg.

Signed-off-by Lee Jones <[email protected]>

---
 arch/arm/include/asm/setup.h |   18 +++++++++++++++++
 arch/arm/lib/bootm.c         |   45
++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)

arch/arm/include/asm/setup.h +18 -0

index 78a7fac..6088440 100644
@@ -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;
 };
 

arch/arm/lib/bootm.c +34 -1

index 1bd2730..03774c8 100644
@@ -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)

{

arch/arm/lib/bootm.c +9 -2

@@ -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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Signature · 3 lines

Otavio Salvador                             O.S. Systems
E-mail: otavio at ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br

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

Quoted 2 lines · In message <[email protected]> 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.

Hi Wolfgang,

Quoted 5 lines

Quoted 2 lines · In message <[email protected]> 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.

Quoted 7 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 3 lines

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. :)

Quoted 5 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 1 line

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?

Quoted 10 lines

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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.

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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?

Quoted 4 lines

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.

Quoted 3 lines

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.

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

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.

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.

Let's try to move this forward.

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.

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.

Quoted 1 line

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.

Quoted 7 lines

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

Quoted 6 lines

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?

Quoted 8 lines

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?

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?

Quoted 4 lines

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.

In this patch-set we're attempting to add boottime measurement support to u-boot. A patch-set has already hit the kernel MLs which intends to solve the other half of the puzzle.

This new tool allows an engineer to apply tags into key areas around the bootloader, which are then passed to the Linux kernel for processing and statistics analysis of full (bootloader
+ kernel) device booting.

Quoted 8 lines

In this patch-set we're attempting to add boottime measurement support to u-boot. A patch-set has already hit the kernel MLs which intends to solve the other half of the puzzle.

This new tool allows an engineer to apply tags into key areas around the bootloader, which are then passed to the Linux kernel for processing and statistics analysis of full (bootloader
+ kernel) device booting.

tags into key areas around the boot loader?

Why do you reinvent the wheel, squarely this time? Why don't you simply use a shared log buffer?