[U-Boot] [PATCH v4 0/6] Iotrace improvements

[U-Boot] [PATCH v4 0/6] Iotrace improvements

[Ramon Fried]

These set of patches add few improvements to iotrace.
* Region limiting - allows setting an address and size where only
                                        io operations that falls into that address are
                                        logged.
* Timestamping - Timestamp every iotrace record with current timestamp
* dumping - iotrace dump command for dumping all records from buffer
                        in a readable fashion.

In terms of backwards compatibility, the timestamp is not backward
compatible as it changes the iotrace record. so if one developed an
offline parsing tool it will be broken.
I though of adding #ifdef specific for that, but eventually I didn't.

Changes in v4:
- Resend complete patchset with patman

Changes in v3:
- fixed wrong usage of WARN_ONCE

Changes in v2:
- introduced needed_size to notify the user about the needed buffer size.

Changes in v1:
- Change timestamp function to get_ticks()

Ramon Fried (6):
  cmd: iotrace: add set region command
  iotrace: add IO region limit
  common: iotrace: add timestamp to iotrace records
  iotrace: move record definitons to header file
  cmd: iotrace: add dump trace command
  iotrace: fix behaviour when buffer is full

 cmd/iotrace.c     | 63 ++++++++++++++++++++++++++++++++++++++++++---
 common/iotrace.c  | 65 +++++++++++++++++++++++++++--------------------
 include/iotrace.h | 57 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 153 insertions(+), 32 deletions(-)

-- 
2.17.0

[U-Boot] [PATCH v4 1/6] cmd: iotrace: add set region command

[Ramon Fried]

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

Changes in v4:
- Resend complete patchset with patman

Changes in v3: None
Changes in v2: None
Changes in v1: None

 cmd/iotrace.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/cmd/iotrace.c b/cmd/iotrace.c
index e496787e0d..601b8c8e32 100644
--- a/cmd/iotrace.c
+++ b/cmd/iotrace.c
@@ -15,6 +15,9 @@ static void do_print_stats(void)
 	iotrace_get_buffer(&start, &size, &offset, &count);
 	printf("Start:  %08lx\n", start);
 	printf("Size:   %08lx\n", size);
+	iotrace_get_region(&start, &size);
+	printf("Region: %08lx\n", start);
+	printf("Size:   %08lx\n", size);
 	printf("Offset: %08lx\n", offset);
 	printf("Output: %08lx\n", start + offset);
 	printf("Count:  %08lx\n", count);
@@ -37,6 +40,22 @@ static int do_set_buffer(int argc, char * const argv[])
 	return 0;
 }
 
+static int do_set_region(int argc, char * const argv[])
+{
+	ulong addr = 0, size = 0;
+
+	if (argc == 2) {
+		addr = simple_strtoul(*argv++, NULL, 16);
+		size = simple_strtoul(*argv++, NULL, 16);
+	} else if (argc != 0) {
+		return CMD_RET_USAGE;
+	}
+
+	iotrace_set_region(addr, size);
+
+	return 0;
+}
+
 int do_iotrace(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	const char *cmd = argc < 2 ? NULL : argv[1];
@@ -46,6 +65,8 @@ int do_iotrace(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	switch (*cmd) {
 	case 'b':
 		return do_set_buffer(argc - 2, argv + 2);
+	case 'l':
+		return do_set_region(argc - 2, argv + 2);
 	case 'p':
 		iotrace_set_enabled(0);
 		break;
@@ -67,6 +88,7 @@ U_BOOT_CMD(
 	"iotrace utility commands",
 	"stats                        - display iotrace stats\n"
 	"iotrace buffer <address> <size>      - set iotrace buffer\n"
+	"iotrace limit <address> <size>       - set iotrace region limit\n"
 	"iotrace pause                        - pause tracing\n"
 	"iotrace resume                       - resume tracing"
 );
-- 
2.17.0

[U-Boot] [PATCH v4 2/6] iotrace: add IO region limit

[Ramon Fried]

When dealing with a lot of IO regions, sometimes
it makes sense only to trace a specific one.
This patch adds support for region limits.
If region is not set, the iotrace works the same as it was.
If region is set, the iotrace only logs io operation that falls
in the defined region.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 common/iotrace.c  | 27 +++++++++++++++++++++++++++
 include/iotrace.h | 24 ++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/common/iotrace.c b/common/iotrace.c
index b16b0d612d..f39885663a 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -42,6 +42,8 @@ struct iotrace_record {
  * @start:	Start address of iotrace buffer
  * @size:	Size of iotrace buffer in bytes
  * @offset:	Current write offset into iotrace buffer
+ * @region_start: Address of IO region to trace
+ * @region_size: Size of region to trace. if 0 will trace all address space
  * @crc32:	Current value of CRC chceksum of trace records
  * @enabled:	true if enabled, false if disabled
  */
@@ -49,6 +51,8 @@ static struct iotrace {
 	ulong start;
 	ulong size;
 	ulong offset;
+	ulong region_start;
+	ulong region_size;
 	u32 crc32;
 	bool enabled;
 } iotrace;
@@ -66,6 +70,11 @@ static void add_record(int flags, const void *ptr, ulong value)
 	if (!(gd->flags & GD_FLG_RELOC) || !iotrace.enabled)
 		return;
 
+	if (iotrace.region_size)
+		if ((ulong)ptr < iotrace.region_start ||
+		    (ulong)ptr > iotrace.region_start + iotrace.region_size)
+			return;
+
 	/* Store it if there is room */
 	if (iotrace.offset + sizeof(*rec) < iotrace.size) {
 		rec = (struct iotrace_record *)map_sysmem(
@@ -142,6 +151,24 @@ u32 iotrace_get_checksum(void)
 	return iotrace.crc32;
 }
 
+void iotrace_set_region(ulong start, ulong size)
+{
+	iotrace.region_start = start;
+	iotrace.region_size = size;
+}
+
+void iotrace_reset_region(void)
+{
+	iotrace.region_start = 0;
+	iotrace.region_size = 0;
+}
+
+void iotrace_get_region(ulong *start, ulong *size)
+{
+	*start = iotrace.region_start;
+	*size = iotrace.region_size;
+}
+
 void iotrace_set_enabled(int enable)
 {
 	iotrace.enabled = enable;
diff --git a/include/iotrace.h b/include/iotrace.h
index 9fe5733f87..1efb117343 100644
--- a/include/iotrace.h
+++ b/include/iotrace.h
@@ -58,6 +58,30 @@ void iotrace_reset_checksum(void);
  */
 u32 iotrace_get_checksum(void);
 
+/**
+ * iotrace_set_region() - Set whether iotrace is limited to a specific
+ * io region.
+ *
+ * Defines the address and size of the limited region.
+ *
+ * @start: address of the beginning of the region
+ * @size: size of the region in bytes.
+ */
+void iotrace_set_region(ulong start, ulong size);
+
+/**
+ * iotrace_reset_region() - Reset the region limit
+ */
+void iotrace_reset_region(void);
+
+/**
+ * iotrace_get_region() - Get region information
+ *
+ * @start: Returns start address of region
+ * @size: Returns size of region in bytes
+ */
+void iotrace_get_region(ulong *start, ulong *size);
+
 /**
  * iotrace_set_enabled() - Set whether iotracing is enabled or not
  *
-- 
2.17.0

[U-Boot] [PATCH v4 3/6] common: iotrace: add timestamp to iotrace records

[Ramon Fried]

Add timestamp to each iotrace record to aid in debugging
of IO timing access bugs.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1:
- Change timestamp function to get_ticks()

 common/iotrace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/iotrace.c b/common/iotrace.c
index f39885663a..2f03a6082e 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -27,11 +27,13 @@ enum iotrace_flags {
  * struct iotrace_record - Holds a single I/O trace record
  *
  * @flags: I/O access type
+ * @timestamp: Timestamp of access
  * @addr: Address of access
  * @value: Value written or read
  */
 struct iotrace_record {
 	enum iotrace_flags flags;
+	u64 timestamp;
 	phys_addr_t addr;
 	iovalue_t value;
 };
@@ -81,7 +83,7 @@ static void add_record(int flags, const void *ptr, ulong value)
 					iotrace.start + iotrace.offset,
 					sizeof(value));
 	}
-
+	rec->timestamp = timer_get_us();
 	rec->flags = flags;
 	rec->addr = map_to_sysmem(ptr);
 	rec->value = value;
-- 
2.17.0

[U-Boot] [PATCH v4 4/6] iotrace: move record definitons to header file

[Ramon Fried]

The header definitions are needed for reading
record information in cmd/iotrace.c

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 common/iotrace.c  | 27 ---------------------------
 include/iotrace.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/common/iotrace.c b/common/iotrace.c
index 2f03a6082e..83691b1dba 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -11,33 +11,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/* Support up to the machine word length for now */
-typedef ulong iovalue_t;
-
-enum iotrace_flags {
-	IOT_8 = 0,
-	IOT_16,
-	IOT_32,
-
-	IOT_READ = 0 << 3,
-	IOT_WRITE = 1 << 3,
-};
-
-/**
- * struct iotrace_record - Holds a single I/O trace record
- *
- * @flags: I/O access type
- * @timestamp: Timestamp of access
- * @addr: Address of access
- * @value: Value written or read
- */
-struct iotrace_record {
-	enum iotrace_flags flags;
-	u64 timestamp;
-	phys_addr_t addr;
-	iovalue_t value;
-};
-
 /**
  * struct iotrace - current trace status and checksum
  *
diff --git a/include/iotrace.h b/include/iotrace.h
index 1efb117343..b6e006ced0 100644
--- a/include/iotrace.h
+++ b/include/iotrace.h
@@ -7,6 +7,34 @@
 #define __IOTRACE_H
 
 #include <linux/types.h>
+#include <common.h>
+
+/* Support up to the machine word length for now */
+typedef ulong iovalue_t;
+
+enum iotrace_flags {
+	IOT_8 = 0,
+	IOT_16,
+	IOT_32,
+
+	IOT_READ = 0 << 3,
+	IOT_WRITE = 1 << 3,
+};
+
+/**
+ * struct iotrace_record - Holds a single I/O trace record
+ *
+ * @flags: I/O access type
+ * @timestamp: Timestamp of access
+ * @addr: Address of access
+ * @value: Value written or read
+ */
+struct iotrace_record {
+	enum iotrace_flags flags;
+	u64 timestamp;
+	phys_addr_t addr;
+	iovalue_t value;
+};
 
 /*
  * This file is designed to be included in arch/<arch>/include/asm/io.h.
-- 
2.17.0

[U-Boot] [PATCH v4 5/6] cmd: iotrace: add dump trace command

[Ramon Fried]

Add dump trace command which dump all trace
buffer content in a much more readable fashion
than md.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 cmd/iotrace.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/cmd/iotrace.c b/cmd/iotrace.c
index 601b8c8e32..f50a3c3a1a 100644
--- a/cmd/iotrace.c
+++ b/cmd/iotrace.c
@@ -24,6 +24,36 @@ static void do_print_stats(void)
 	printf("CRC32:  %08lx\n", (ulong)iotrace_get_checksum());
 }
 
+static void do_print_trace(void)
+{
+	ulong start, size, offset, count;
+
+	struct iotrace_record *cur_record;
+
+	iotrace_get_buffer(&start, &size, &offset, &count);
+
+	if (!start || !size || !count)
+		return;
+
+	printf("Timestamp  Value          Address\n");
+
+	cur_record = (struct iotrace_record *)start;
+	for (int i = 0; i < count; i++) {
+		if (cur_record->flags & IOT_WRITE)
+			printf("%08llu: 0x%08lx --> 0x%08lx\n",
+			       cur_record->timestamp,
+					cur_record->value,
+					cur_record->addr);
+		else
+			printf("%08llu: 0x%08lx <-- 0x%08lx\n",
+			       cur_record->timestamp,
+					cur_record->value,
+					cur_record->addr);
+
+		cur_record++;
+	}
+}
+
 static int do_set_buffer(int argc, char * const argv[])
 {
 	ulong addr = 0, size = 0;
@@ -76,6 +106,9 @@ int do_iotrace(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	case 's':
 		do_print_stats();
 		break;
+	case 'd':
+		do_print_trace();
+		break;
 	default:
 		return CMD_RET_USAGE;
 	}
@@ -90,5 +123,6 @@ U_BOOT_CMD(
 	"iotrace buffer <address> <size>      - set iotrace buffer\n"
 	"iotrace limit <address> <size>       - set iotrace region limit\n"
 	"iotrace pause                        - pause tracing\n"
-	"iotrace resume                       - resume tracing"
+	"iotrace resume                       - resume tracing\n"
+	"iotrace dump                         - dump iotrace buffer"
 );
-- 
2.17.0

[U-Boot] [PATCH v4 6/6] iotrace: fix behaviour when buffer is full

[Ramon Fried]

Don't continue updating the offset when buffer is full.
When the buffer size exhausts and there's no space left to write
warn the user and update only the needed size and not both the
offset and needed size.

Add needed buffer size information in the iotrace command.

Signed-off-by: Ramon Fried <ramon.fried@gmail.com>

---

Changes in v4: None
Changes in v3:
- fixed wrong usage of WARN_ONCE

Changes in v2:
- introduced needed_size to notify the user about the needed buffer size.

Changes in v1: None

 cmd/iotrace.c     | 11 ++++++-----
 common/iotrace.c  | 13 +++++++++++--
 include/iotrace.h |  5 +++--
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/cmd/iotrace.c b/cmd/iotrace.c
index f50a3c3a1a..fc77f63de6 100644
--- a/cmd/iotrace.c
+++ b/cmd/iotrace.c
@@ -9,12 +9,13 @@
 
 static void do_print_stats(void)
 {
-	ulong start, size, offset, count;
+	ulong start, size, needed_size, offset, count;
 
 	printf("iotrace is %sabled\n", iotrace_get_enabled() ? "en" : "dis");
-	iotrace_get_buffer(&start, &size, &offset, &count);
+	iotrace_get_buffer(&start, &size, &needed_size, &offset, &count);
 	printf("Start:  %08lx\n", start);
-	printf("Size:   %08lx\n", size);
+	printf("Actual Size:   %08lx\n", size);
+	printf("Needed Size:   %08lx\n", needed_size);
 	iotrace_get_region(&start, &size);
 	printf("Region: %08lx\n", start);
 	printf("Size:   %08lx\n", size);
@@ -26,11 +27,11 @@ static void do_print_stats(void)
 
 static void do_print_trace(void)
 {
-	ulong start, size, offset, count;
+	ulong start, size, needed_size, offset, count;
 
 	struct iotrace_record *cur_record;
 
-	iotrace_get_buffer(&start, &size, &offset, &count);
+	iotrace_get_buffer(&start, &size, &needed_size, &offset, &count);
 
 	if (!start || !size || !count)
 		return;
diff --git a/common/iotrace.c b/common/iotrace.c
index 83691b1dba..49bee3c92a 100644
--- a/common/iotrace.c
+++ b/common/iotrace.c
@@ -15,7 +15,8 @@ DECLARE_GLOBAL_DATA_PTR;
  * struct iotrace - current trace status and checksum
  *
  * @start:	Start address of iotrace buffer
- * @size:	Size of iotrace buffer in bytes
+ * @size:	Actual size of iotrace buffer in bytes
+ * @needed_size: Needed of iotrace buffer in bytes
  * @offset:	Current write offset into iotrace buffer
  * @region_start: Address of IO region to trace
  * @region_size: Size of region to trace. if 0 will trace all address space
@@ -25,6 +26,7 @@ DECLARE_GLOBAL_DATA_PTR;
 static struct iotrace {
 	ulong start;
 	ulong size;
+	ulong needed_size;
 	ulong offset;
 	ulong region_start;
 	ulong region_size;
@@ -55,7 +57,12 @@ static void add_record(int flags, const void *ptr, ulong value)
 		rec = (struct iotrace_record *)map_sysmem(
 					iotrace.start + iotrace.offset,
 					sizeof(value));
+	} else {
+		WARN_ONCE(1, "WARNING: iotrace buffer exhausted, please check needed length using \"iotrace stats\"\n");
+		iotrace.needed_size += sizeof(struct iotrace_record);
+		return;
 	}
+
 	rec->timestamp = timer_get_us();
 	rec->flags = flags;
 	rec->addr = map_to_sysmem(ptr);
@@ -65,6 +72,7 @@ static void add_record(int flags, const void *ptr, ulong value)
 	iotrace.crc32 = crc32(iotrace.crc32, (unsigned char *)rec,
 			      sizeof(*rec));
 
+	iotrace.needed_size += sizeof(struct iotrace_record);
 	iotrace.offset += sizeof(struct iotrace_record);
 }
 
@@ -162,10 +170,11 @@ void iotrace_set_buffer(ulong start, ulong size)
 	iotrace.crc32 = 0;
 }
 
-void iotrace_get_buffer(ulong *start, ulong *size, ulong *offset, ulong *count)
+void iotrace_get_buffer(ulong *start, ulong *size, ulong *needed_size, ulong *offset, ulong *count)
 {
 	*start = iotrace.start;
 	*size = iotrace.size;
+	*needed_size = iotrace.needed_size;
 	*offset = iotrace.offset;
 	*count = iotrace.offset / sizeof(struct iotrace_record);
 }
diff --git a/include/iotrace.h b/include/iotrace.h
index b6e006ced0..ad7092e97d 100644
--- a/include/iotrace.h
+++ b/include/iotrace.h
@@ -146,11 +146,12 @@ void iotrace_set_buffer(ulong start, ulong size);
  * iotrace_get_buffer() - Get buffer information
  *
  * @start: Returns start address of buffer
- * @size: Returns size of buffer in bytes
+ * @size: Returns actual size of buffer in bytes
+ * @needed_size: Returns needed size of buffer in bytes
  * @offset: Returns the byte offset where the next output trace record will
  * @count: Returns the number of trace records recorded
  * be written (or would be if the buffer was large enough)
  */
-void iotrace_get_buffer(ulong *start, ulong *size, ulong *offset, ulong *count);
+void iotrace_get_buffer(ulong *start, ulong *size, ulong *needed_size, ulong *offset, ulong *count);
 
 #endif /* __IOTRACE_H */
-- 
2.17.0

[U-Boot] [PATCH v4 6/6] iotrace: fix behaviour when buffer is full

[Simon Glass]

On 30 May 2018 at 14:10, Ramon Fried <ramon.fried@gmail.com> wrote:
> Don't continue updating the offset when buffer is full.
> When the buffer size exhausts and there's no space left to write
> warn the user and update only the needed size and not both the
> offset and needed size.
>
> Add needed buffer size information in the iotrace command.
>
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
>
> ---
>
> Changes in v4: None
> Changes in v3:
> - fixed wrong usage of WARN_ONCE
>
> Changes in v2:
> - introduced needed_size to notify the user about the needed buffer size.
>
> Changes in v1: None
>
>  cmd/iotrace.c     | 11 ++++++-----
>  common/iotrace.c  | 13 +++++++++++--
>  include/iotrace.h |  5 +++--
>  3 files changed, 20 insertions(+), 9 deletions(-)
>

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

[U-Boot] [PATCH v4 6/6] iotrace: fix behaviour when buffer is full

[Ramon Fried]

Hi Tom.
The below patch is depened on this one:
https://patchwork.ozlabs.org/patch/922071/
Please wait with meging until the patch above is reviewed and mereged.

Thanks,
Ramon.

On Thu, May 31, 2018 at 4:01 PM, Simon Glass <sjg@chromium.org> wrote:
> On 30 May 2018 at 14:10, Ramon Fried <ramon.fried@gmail.com> wrote:
>> Don't continue updating the offset when buffer is full.
>> When the buffer size exhausts and there's no space left to write
>> warn the user and update only the needed size and not both the
>> offset and needed size.
>>
>> Add needed buffer size information in the iotrace command.
>>
>> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
>>
>> ---
>>
>> Changes in v4: None
>> Changes in v3:
>> - fixed wrong usage of WARN_ONCE
>>
>> Changes in v2:
>> - introduced needed_size to notify the user about the needed buffer size.
>>
>> Changes in v1: None
>>
>>  cmd/iotrace.c     | 11 ++++++-----
>>  common/iotrace.c  | 13 +++++++++++--
>>  include/iotrace.h |  5 +++--
>>  3 files changed, 20 insertions(+), 9 deletions(-)
>>
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [U-Boot,v4,5/6] cmd: iotrace: add dump trace command

[Tom Rini]

On Wed, May 30, 2018 at 11:10:01PM +0300, Ramon Fried wrote:

> Add dump trace command which dump all trace
> buffer content in a much more readable fashion
> than md.
> 
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> Changes in v1: None
> 
>  cmd/iotrace.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/iotrace.c b/cmd/iotrace.c
> index 601b8c8e32..f50a3c3a1a 100644
> --- a/cmd/iotrace.c
> +++ b/cmd/iotrace.c
> @@ -24,6 +24,36 @@ static void do_print_stats(void)
>  	printf("CRC32:  %08lx\n", (ulong)iotrace_get_checksum());
>  }
>  
> +static void do_print_trace(void)
> +{
> +	ulong start, size, offset, count;
> +
> +	struct iotrace_record *cur_record;
> +
> +	iotrace_get_buffer(&start, &size, &offset, &count);
> +
> +	if (!start || !size || !count)
> +		return;
> +
> +	printf("Timestamp  Value          Address\n");
> +
> +	cur_record = (struct iotrace_record *)start;
> +	for (int i = 0; i < count; i++) {
> +		if (cur_record->flags & IOT_WRITE)
> +			printf("%08llu: 0x%08lx --> 0x%08lx\n",
> +			       cur_record->timestamp,
> +					cur_record->value,
> +					cur_record->addr);
> +		else
> +			printf("%08llu: 0x%08lx <-- 0x%08lx\n",
> +			       cur_record->timestamp,
> +					cur_record->value,
> +					cur_record->addr);
> +
> +		cur_record++;
> +	}
> +}

This isn't portable.  If you build for sandbox64:
cmd/iotrace.c: In function ‘do_print_trace’:
cmd/iotrace.c:44:11: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘phys_addr_t {aka unsigned int}’ [-Wformat=]
    printf("%08llu: 0x%08lx --> 0x%08lx\n",
           ^
cmd/iotrace.c:49:11: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘phys_addr_t {aka unsigned int}’ [-Wformat=]
    printf("%08llu: 0x%08lx <-- 0x%08lx\n",
           ^
t

Also, as-is, with clang-7:
u-boot/cmd/iotrace.c:47:6: warning: format specifies type 'unsigned long' but the argument has type 'phys_addr_t' (aka 'unsigned int') [-Wformat]
                                        cur_record->addr);
                                        ^~~~~~~~~~~~~~~~
cmd/iotrace.c:52:6: warning: format specifies type 'unsigned long' but the argument has type 'phys_addr_t' (aka 'unsigned int') [-Wformat]
                                        cur_record->addr);
                                        ^~~~~~~~~~~~~~~~

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180608/12888794/attachment.sig>

[U-Boot] [U-Boot, v4, 4/6] iotrace: move record definitons to header file

[Tom Rini]

On Wed, May 30, 2018 at 11:10:00PM +0300, Ramon Fried wrote:

> The header definitions are needed for reading
> record information in cmd/iotrace.c
> 
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

This change ends up breaking a number of platforms including k2l_hs_evm
in a very odd way.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180608/ff64556e/attachment.sig>

[U-Boot] [U-Boot,v4,1/6] cmd: iotrace: add set region command

[Tom Rini]

On Wed, May 30, 2018 at 11:09:57PM +0300, Ramon Fried wrote:

> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180608/144a95ed/attachment.sig>

[U-Boot] [U-Boot,v4,2/6] iotrace: add IO region limit

[Tom Rini]

On Wed, May 30, 2018 at 11:09:58PM +0300, Ramon Fried wrote:

> When dealing with a lot of IO regions, sometimes
> it makes sense only to trace a specific one.
> This patch adds support for region limits.
> If region is not set, the iotrace works the same as it was.
> If region is set, the iotrace only logs io operation that falls
> in the defined region.
> 
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180608/a510de80/attachment.sig>

[U-Boot] [U-Boot, v4, 3/6] common: iotrace: add timestamp to iotrace records

[Tom Rini]

On Wed, May 30, 2018 at 11:09:59PM +0300, Ramon Fried wrote:

> Add timestamp to each iotrace record to aid in debugging
> of IO timing access bugs.
> 
> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180608/1572941d/attachment.sig>

[U-Boot] [U-Boot, v4, 4/6] iotrace: move record definitons to header file

[Ramon Fried]

On Fri, Jun 8, 2018 at 2:19 PM, Tom Rini <trini@konsulko.com> wrote:
> On Wed, May 30, 2018 at 11:10:00PM +0300, Ramon Fried wrote:
>
>> The header definitions are needed for reading
>> record information in cmd/iotrace.c
>>
>> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> This change ends up breaking a number of platforms including k2l_hs_evm
> in a very odd way.
It's very odd indeed. basically, just adding #include <common.h>
breaks the build.
I'm investigating.
Thanks,
Ramon.

>
> --
> Tom

[U-Boot] [U-Boot, v4, 4/6] iotrace: move record definitons to header file

[Ramon Fried]

On Fri, Jun 8, 2018 at 3:49 PM, Ramon Fried <ramon.fried@gmail.com> wrote:
> On Fri, Jun 8, 2018 at 2:19 PM, Tom Rini <trini@konsulko.com> wrote:
>> On Wed, May 30, 2018 at 11:10:00PM +0300, Ramon Fried wrote:
>>
>>> The header definitions are needed for reading
>>> record information in cmd/iotrace.c
>>>
>>> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> This change ends up breaking a number of platforms including k2l_hs_evm
>> in a very odd way.
> It's very odd indeed. basically, just adding #include <common.h>
> breaks the build.
> I'm investigating.
Didn't find how it breaks, but I actually don't need common.h so I
removed it and now it works.
I will resend the patch shortly.

> Thanks,
> Ramon.
>
>>
>> --
>> Tom

[U-Boot] [U-Boot,v4,5/6] cmd: iotrace: add dump trace command

[Ramon Fried]

On Fri, Jun 8, 2018 at 2:18 PM, Tom Rini <trini@konsulko.com> wrote:
> On Wed, May 30, 2018 at 11:10:01PM +0300, Ramon Fried wrote:
>
>> Add dump trace command which dump all trace
>> buffer content in a much more readable fashion
>> than md.
>>
>> Signed-off-by: Ramon Fried <ramon.fried@gmail.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>> Changes in v1: None
>>
>>  cmd/iotrace.c | 36 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmd/iotrace.c b/cmd/iotrace.c
>> index 601b8c8e32..f50a3c3a1a 100644
>> --- a/cmd/iotrace.c
>> +++ b/cmd/iotrace.c
>> @@ -24,6 +24,36 @@ static void do_print_stats(void)
>>       printf("CRC32:  %08lx\n", (ulong)iotrace_get_checksum());
>>  }
>>
>> +static void do_print_trace(void)
>> +{
>> +     ulong start, size, offset, count;
>> +
>> +     struct iotrace_record *cur_record;
>> +
>> +     iotrace_get_buffer(&start, &size, &offset, &count);
>> +
>> +     if (!start || !size || !count)
>> +             return;
>> +
>> +     printf("Timestamp  Value          Address\n");
>> +
>> +     cur_record = (struct iotrace_record *)start;
>> +     for (int i = 0; i < count; i++) {
>> +             if (cur_record->flags & IOT_WRITE)
>> +                     printf("%08llu: 0x%08lx --> 0x%08lx\n",
>> +                            cur_record->timestamp,
>> +                                     cur_record->value,
>> +                                     cur_record->addr);
>> +             else
>> +                     printf("%08llu: 0x%08lx <-- 0x%08lx\n",
>> +                            cur_record->timestamp,
>> +                                     cur_record->value,
>> +                                     cur_record->addr);
>> +
>> +             cur_record++;
>> +     }
>> +}
>
> This isn't portable.  If you build for sandbox64:
> cmd/iotrace.c: In function ‘do_print_trace’:
> cmd/iotrace.c:44:11: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘phys_addr_t {aka unsigned int}’ [-Wformat=]
>     printf("%08llu: 0x%08lx --> 0x%08lx\n",
>            ^
> cmd/iotrace.c:49:11: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘phys_addr_t {aka unsigned int}’ [-Wformat=]
>     printf("%08llu: 0x%08lx <-- 0x%08lx\n",
>            ^
> t
>
> Also, as-is, with clang-7:
> u-boot/cmd/iotrace.c:47:6: warning: format specifies type 'unsigned long' but the argument has type 'phys_addr_t' (aka 'unsigned int') [-Wformat]
>                                         cur_record->addr);
>                                         ^~~~~~~~~~~~~~~~
> cmd/iotrace.c:52:6: warning: format specifies type 'unsigned long' but the argument has type 'phys_addr_t' (aka 'unsigned int') [-Wformat]
>                                         cur_record->addr);
>                                         ^~~~~~~~~~~~~~~~
>
> --
> Tom
Fixed it and resent the patches. Thanks.