[U-Boot] [PATCH v4 1/2] common: Add support for Android DT image

[U-Boot] [PATCH v4 1/2] common: Add support for Android DT image

[Sam Protsenko]

Android documentation recommends new image format for storing DTB/DTBO
files: [1]. To support that format, this patch adds helper functions for
Android DTB/DTBO format. In image-android-dt.* files you can find helper
functions to work with Android DT image format, such us routines for:
    - printing the dump of image structure
    - getting the address and size of desired dtb/dtbo file

This patch uses dt_table.h file, that was added in commit 643cefa4d848
("Import Android's dt_table.h for DT image format") by Alex Deymo.

[1] https://source.android.com/devices/architecture/dto/partitions

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v4:
 - fix SPDX tags
 - renamed .reserved[] to .version (just reflect changes in dt_image.h)
Changes in v3:
 - dropped include/dt_image.h (was sent in another patch by Alex Deymo)
 - rebased on top of last master

 common/image-android-dt.c  | 156 +++++++++++++++++++++++++++++++++++++
 include/image-android-dt.h |  20 +++++
 2 files changed, 176 insertions(+)
 create mode 100644 common/image-android-dt.c
 create mode 100644 include/image-android-dt.h

diff --git a/common/image-android-dt.c b/common/image-android-dt.c
new file mode 100644
index 0000000000..c0683ee70f
--- /dev/null
+++ b/common/image-android-dt.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2018 Linaro Ltd.
+ * Sam Protsenko <semen.protsenko@linaro.org>
+ */
+
+#include <image-android-dt.h>
+#include <dt_table.h>
+#include <common.h>
+#include <linux/libfdt.h>
+#include <mapmem.h>
+
+/**
+ * Check if image header is correct.
+ *
+ * @param hdr_addr Start address of DT image
+ * @return true if header is correct or false if header is incorrect
+ */
+bool android_dt_check_header(ulong hdr_addr)
+{
+	const struct dt_table_header *hdr;
+	u32 magic;
+
+	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
+	magic = fdt32_to_cpu(hdr->magic);
+	unmap_sysmem(hdr);
+
+	return magic == DT_TABLE_MAGIC;
+}
+
+/**
+ * Get the address of FDT (dtb or dtbo) in memory by its index in image.
+ *
+ * @param hdr_addr Start address of DT image
+ * @param index Index of desired FDT in image (starting from 0)
+ * @param[out] addr If not NULL, will contain address to specified FDT
+ * @param[out] size If not NULL, will contain size of specified FDT
+ *
+ * @return true on success or false on error
+ */
+bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
+				 u32 *size)
+{
+	const struct dt_table_header *hdr;
+	const struct dt_table_entry *e;
+	u32 entry_count, entries_offset, entry_size;
+	ulong e_addr;
+	u32 dt_offset, dt_size;
+
+	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
+	entry_count = fdt32_to_cpu(hdr->dt_entry_count);
+	entries_offset = fdt32_to_cpu(hdr->dt_entries_offset);
+	entry_size = fdt32_to_cpu(hdr->dt_entry_size);
+	unmap_sysmem(hdr);
+
+	if (index > entry_count) {
+		printf("Error: index > dt_entry_count (%u > %u)\n", index,
+		       entry_count);
+		return false;
+	}
+
+	e_addr = hdr_addr + entries_offset + index * entry_size;
+	e = map_sysmem(e_addr, sizeof(*e));
+	dt_offset = fdt32_to_cpu(e->dt_offset);
+	dt_size = fdt32_to_cpu(e->dt_size);
+	unmap_sysmem(e);
+
+	if (addr)
+		*addr = hdr_addr + dt_offset;
+	if (size)
+		*size = dt_size;
+
+	return true;
+}
+
+#if !defined(CONFIG_SPL_BUILD)
+static void android_dt_print_fdt_info(const struct fdt_header *fdt)
+{
+	u32 fdt_size;
+	int root_node_off;
+	const char *compatible = NULL;
+
+	fdt_size = fdt_totalsize(fdt);
+	root_node_off = fdt_path_offset(fdt, "/");
+	if (root_node_off < 0) {
+		printf("Error: Root node not found\n");
+	} else {
+		compatible = fdt_getprop(fdt, root_node_off, "compatible",
+					 NULL);
+	}
+
+	printf("           (FDT)size = %d\n", fdt_size);
+	printf("     (FDT)compatible = %s\n",
+	       compatible ? compatible : "(unknown)");
+}
+
+/**
+ * Print information about DT image structure.
+ *
+ * @param hdr_addr Start address of DT image
+ */
+void android_dt_print_contents(ulong hdr_addr)
+{
+	const struct dt_table_header *hdr;
+	u32 entry_count, entries_offset, entry_size;
+	u32 i;
+
+	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
+	entry_count = fdt32_to_cpu(hdr->dt_entry_count);
+	entries_offset = fdt32_to_cpu(hdr->dt_entries_offset);
+	entry_size = fdt32_to_cpu(hdr->dt_entry_size);
+
+	/* Print image header info */
+	printf("dt_table_header:\n");
+	printf("               magic = %08x\n", fdt32_to_cpu(hdr->magic));
+	printf("          total_size = %d\n", fdt32_to_cpu(hdr->total_size));
+	printf("         header_size = %d\n", fdt32_to_cpu(hdr->header_size));
+	printf("       dt_entry_size = %d\n", entry_size);
+	printf("      dt_entry_count = %d\n", entry_count);
+	printf("   dt_entries_offset = %d\n", entries_offset);
+	printf("           page_size = %d\n", fdt32_to_cpu(hdr->page_size));
+	printf("             version = %d\n", fdt32_to_cpu(hdr->version));
+
+	unmap_sysmem(hdr);
+
+	/* Print image entries info */
+	for (i = 0; i < entry_count; ++i) {
+		const ulong e_addr = hdr_addr + entries_offset + i * entry_size;
+		const struct dt_table_entry *e;
+		const struct fdt_header *fdt;
+		u32 dt_offset, dt_size;
+		u32 j;
+
+		e = map_sysmem(e_addr, sizeof(*e));
+		dt_offset = fdt32_to_cpu(e->dt_offset);
+		dt_size = fdt32_to_cpu(e->dt_size);
+
+		printf("dt_table_entry[%d]:\n", i);
+		printf("             dt_size = %d\n", dt_size);
+		printf("           dt_offset = %d\n", dt_offset);
+		printf("                  id = %08x\n", fdt32_to_cpu(e->id));
+		printf("                 rev = %08x\n", fdt32_to_cpu(e->rev));
+		for (j = 0; j < 4; ++j) {
+			printf("           custom[%d] = %08x\n", j,
+			       fdt32_to_cpu(e->custom[j]));
+		}
+
+		unmap_sysmem(e);
+
+		/* Print FDT info for this entry */
+		fdt = map_sysmem(hdr_addr + dt_offset, sizeof(*fdt));
+		android_dt_print_fdt_info(fdt);
+		unmap_sysmem(fdt);
+	}
+}
+#endif
diff --git a/include/image-android-dt.h b/include/image-android-dt.h
new file mode 100644
index 0000000000..9a3aa8fa30
--- /dev/null
+++ b/include/image-android-dt.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * (C) Copyright 2018 Linaro Ltd.
+ * Sam Protsenko <semen.protsenko@linaro.org>
+ */
+
+#ifndef IMAGE_ANDROID_DT_H
+#define IMAGE_ANDROID_DT_H
+
+#include <linux/types.h>
+
+bool android_dt_check_header(ulong hdr_addr);
+bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
+				 u32 *size);
+
+#if !defined(CONFIG_SPL_BUILD)
+void android_dt_print_contents(ulong hdr_addr);
+#endif
+
+#endif /* IMAGE_ANDROID_DT_H */
-- 
2.18.0

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Sam Protsenko]

dtimg command allows user to work with Android DTB/DTBO image format.
Such as, getting the address of desired DTB/DTBO file, printing the dump
of the image in U-Boot shell, etc.

This command is needed to provide Android boot with new Android DT image
format further.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v4:
  - rebased
  - fixed SPDX tags
  - use obj-$(CONFIG_CMD_DTIMG) instead #ifdef

 cmd/Kconfig     |   8 +++
 cmd/Makefile    |   1 +
 cmd/dtimg.c     | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
 common/Makefile |   2 +
 4 files changed, 152 insertions(+)
 create mode 100644 cmd/dtimg.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index bd90946667..b9215abae3 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -254,6 +254,14 @@ config CMD_BOOTMENU
 	help
 	  Add an ANSI terminal boot menu command.
 
+config CMD_DTIMG
+	bool "dtimg"
+	help
+	  Android DTB/DTBO image manipulation commands. Read dtb/dtbo files from
+	  image into RAM, dump image structure information, etc. Those dtb/dtbo
+	  files should be merged in one dtb further, which needs to be passed to
+	  the kernel, as part of a boot process.
+
 config CMD_ELF
 	bool "bootelf, bootvx"
 	default y
diff --git a/cmd/Makefile b/cmd/Makefile
index 12d2118f1d..b1206fca85 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -43,6 +43,7 @@ ifdef CONFIG_POST
 obj-$(CONFIG_CMD_DIAG) += diag.o
 endif
 obj-$(CONFIG_CMD_DISPLAY) += display.o
+obj-$(CONFIG_CMD_DTIMG) += dtimg.o
 obj-$(CONFIG_CMD_ECHO) += echo.o
 obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
 obj-$(CONFIG_CMD_EEPROM) += eeprom.o
diff --git a/cmd/dtimg.c b/cmd/dtimg.c
new file mode 100644
index 0000000000..65c8d101b9
--- /dev/null
+++ b/cmd/dtimg.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2018 Linaro Ltd.
+ * Sam Protsenko <semen.protsenko@linaro.org>
+ */
+
+#include <image-android-dt.h>
+#include <common.h>
+
+enum cmd_dtimg_info {
+	CMD_DTIMG_START = 0,
+	CMD_DTIMG_SIZE,
+};
+
+static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
+			 char * const argv[])
+{
+	char *endp;
+	ulong hdr_addr;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	hdr_addr = simple_strtoul(argv[1], &endp, 16);
+	if (*endp != '\0') {
+		printf("Error: Wrong image address\n");
+		return CMD_RET_FAILURE;
+	}
+
+	if (!android_dt_check_header(hdr_addr)) {
+		printf("Error: DT image header is incorrect\n");
+		return CMD_RET_FAILURE;
+	}
+
+	android_dt_print_contents(hdr_addr);
+
+	return CMD_RET_SUCCESS;
+}
+
+static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
+{
+	ulong hdr_addr;
+	u32 index;
+	char *endp;
+	ulong fdt_addr;
+	u32 fdt_size;
+	char buf[65];
+
+	if (argc != 4)
+		return CMD_RET_USAGE;
+
+	hdr_addr = simple_strtoul(argv[1], &endp, 16);
+	if (*endp != '\0') {
+		printf("Error: Wrong image address\n");
+		return CMD_RET_FAILURE;
+	}
+
+	if (!android_dt_check_header(hdr_addr)) {
+		printf("Error: DT image header is incorrect\n");
+		return CMD_RET_FAILURE;
+	}
+
+	index = simple_strtoul(argv[2], &endp, 0);
+	if (*endp != '\0') {
+		printf("Error: Wrong index\n");
+		return CMD_RET_FAILURE;
+	}
+
+	if (!android_dt_get_fdt_by_index(hdr_addr, index, &fdt_addr, &fdt_size))
+		return CMD_RET_FAILURE;
+
+	switch (cmd) {
+	case CMD_DTIMG_START:
+		snprintf(buf, sizeof(buf), "%lx", fdt_addr);
+		break;
+	case CMD_DTIMG_SIZE:
+		snprintf(buf, sizeof(buf), "%x", fdt_size);
+		break;
+	default:
+		printf("Error: Unknown cmd_dtimg_info value: %d\n", cmd);
+		return CMD_RET_FAILURE;
+	}
+
+	env_set(argv[3], buf);
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_dtimg_start(cmd_tbl_t *cmdtp, int flag, int argc,
+			  char * const argv[])
+{
+	return dtimg_get_fdt(argc, argv, CMD_DTIMG_START);
+}
+
+static int do_dtimg_size(cmd_tbl_t *cmdtp, int flag, int argc,
+			 char * const argv[])
+{
+	return dtimg_get_fdt(argc, argv, CMD_DTIMG_SIZE);
+}
+
+static cmd_tbl_t cmd_dtimg_sub[] = {
+	U_BOOT_CMD_MKENT(dump, 2, 0, do_dtimg_dump, "", ""),
+	U_BOOT_CMD_MKENT(start, 4, 0, do_dtimg_start, "", ""),
+	U_BOOT_CMD_MKENT(size, 4, 0, do_dtimg_size, "", ""),
+};
+
+static int do_dtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	cmd_tbl_t *cp;
+
+	cp = find_cmd_tbl(argv[1], cmd_dtimg_sub, ARRAY_SIZE(cmd_dtimg_sub));
+
+	/* Strip off leading 'dtimg' command argument */
+	argc--;
+	argv++;
+
+	if (!cp || argc > cp->maxargs)
+		return CMD_RET_USAGE;
+	if (flag == CMD_FLAG_REPEAT && !cp->repeatable)
+		return CMD_RET_SUCCESS;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+U_BOOT_CMD(
+	dtimg, CONFIG_SYS_MAXARGS, 0, do_dtimg,
+	"manipulate dtb/dtbo Android image",
+	"dump <addr>\n"
+	"    - parse specified image and print its structure info\n"
+	"      <addr>: image address in RAM, in hex\n"
+	"dtimg start <addr> <index> <varname>\n"
+	"    - get address (hex) of FDT in the image, by index\n"
+	"      <addr>: image address in RAM, in hex\n"
+	"      <index>: index of desired FDT in the image\n"
+	"      <varname>: name of variable where to store address of FDT\n"
+	"dtimg size <addr> <index> <varname>\n"
+	"    - get size (hex, bytes) of FDT in the image, by index\n"
+	"      <addr>: image address in RAM, in hex\n"
+	"      <index>: index of desired FDT in the image\n"
+	"      <varname>: name of variable where to store size of FDT"
+);
diff --git a/common/Makefile b/common/Makefile
index 7100541ece..7473b85011 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -108,6 +108,8 @@ obj-$(CONFIG_IO_TRACE) += iotrace.o
 obj-y += memsize.o
 obj-y += stdio.o
 
+obj-$(CONFIG_CMD_DTIMG) += image-android-dt.o
+
 ifdef CONFIG_CMD_EEPROM_LAYOUT
 obj-y += eeprom/eeprom_field.o eeprom/eeprom_layout.o
 endif
-- 
2.18.0

[U-Boot] [PATCH v4 1/2] common: Add support for Android DT image

[Tom Rini]

On Thu, Aug 16, 2018 at 11:34:12PM +0300, Sam Protsenko wrote:

> Android documentation recommends new image format for storing DTB/DTBO
> files: [1]. To support that format, this patch adds helper functions for
> Android DTB/DTBO format. In image-android-dt.* files you can find helper
> functions to work with Android DT image format, such us routines for:
>     - printing the dump of image structure
>     - getting the address and size of desired dtb/dtbo file
> 
> This patch uses dt_table.h file, that was added in commit 643cefa4d848
> ("Import Android's dt_table.h for DT image format") by Alex Deymo.
> 
> [1] https://source.android.com/devices/architecture/dto/partitions
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20180816/2c5f3993/attachment.sig>

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Tom Rini]

On Thu, Aug 16, 2018 at 11:34:13PM +0300, Sam Protsenko wrote:

> dtimg command allows user to work with Android DTB/DTBO image format.
> Such as, getting the address of desired DTB/DTBO file, printing the dump
> of the image in U-Boot shell, etc.
> 
> This command is needed to provide Android boot with new Android DT image
> format further.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20180816/3db1bb49/attachment.sig>

[U-Boot] [U-Boot, v4, 1/2] common: Add support for Android DT image

[Tom Rini]

On Thu, Aug 16, 2018 at 11:34:12PM +0300, Sam Protsenko wrote:

> Android documentation recommends new image format for storing DTB/DTBO
> files: [1]. To support that format, this patch adds helper functions for
> Android DTB/DTBO format. In image-android-dt.* files you can find helper
> functions to work with Android DT image format, such us routines for:
>     - printing the dump of image structure
>     - getting the address and size of desired dtb/dtbo file
> 
> This patch uses dt_table.h file, that was added in commit 643cefa4d848
> ("Import Android's dt_table.h for DT image format") by Alex Deymo.
> 
> [1] https://source.android.com/devices/architecture/dto/partitions
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Reviewed-by: Tom Rini <trini@konsulko.com>

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/20180820/6b59af68/attachment.sig>

[U-Boot] [U-Boot,v4,2/2] cmd: Add dtimg command

[Tom Rini]

On Thu, Aug 16, 2018 at 11:34:13PM +0300, Sam Protsenko wrote:

> dtimg command allows user to work with Android DTB/DTBO image format.
> Such as, getting the address of desired DTB/DTBO file, printing the dump
> of the image in U-Boot shell, etc.
> 
> This command is needed to provide Android boot with new Android DT image
> format further.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Reviewed-by: Tom Rini <trini@konsulko.com>

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/20180820/d9804e37/attachment.sig>

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Eugeniu Rosca]

Hello Sam,

On Thu, Aug 16, 2018 at 11:34:13PM +0300, Sam Protsenko wrote:
> dtimg command allows user to work with Android DTB/DTBO image format.
> Such as, getting the address of desired DTB/DTBO file, printing the dump
> of the image in U-Boot shell, etc.
> 
> This command is needed to provide Android boot with new Android DT image
> format further.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Reviewed-by: Tom Rini <trini@konsulko.com>

[..]

> +U_BOOT_CMD(
> +	dtimg, CONFIG_SYS_MAXARGS, 0, do_dtimg,
> +	"manipulate dtb/dtbo Android image",
> +	"dump <addr>\n"
> +	"    - parse specified image and print its structure info\n"
> +	"      <addr>: image address in RAM, in hex\n"
> +	"dtimg start <addr> <index> <varname>\n"
> +	"    - get address (hex) of FDT in the image, by index\n"
> +	"      <addr>: image address in RAM, in hex\n"
> +	"      <index>: index of desired FDT in the image\n"
> +	"      <varname>: name of variable where to store address of FDT\n"
> +	"dtimg size <addr> <index> <varname>\n"
> +	"    - get size (hex, bytes) of FDT in the image, by index\n"
> +	"      <addr>: image address in RAM, in hex\n"
> +	"      <index>: index of desired FDT in the image\n"
> +	"      <varname>: name of variable where to store size of FDT"
> +);

Since you are the author and the main stakeholder of "dtimg", could you
kindly feedback the command usage you envision for getting the start and
size of dtb/dtbo blob given a certain "id" and "rev" fields used by
mkdtboimg.py [1] and visible in the output of U-Boot's "dtimg dump" [2]?

One option would be to extend the existing "dtimg {start|size}" to
accept an argument like "id:<val>" and "rev:<val>".

Another possibility is to create brand new dtimg sub-command.
What would be your preference? TIA.

[1] https://android.googlesource.com/platform/system/libufdt/+/master/utils/src/mkdtboimg.py
[2] https://gitlab.denx.de/u-boot/u-boot/commit/e63bf1b13b3a7a

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Roman Stratiienko]

On Tue, Nov 12, 2019 at 8:18 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hello Sam,
>
> On Thu, Aug 16, 2018 at 11:34:13PM +0300, Sam Protsenko wrote:
> > dtimg command allows user to work with Android DTB/DTBO image format.
> > Such as, getting the address of desired DTB/DTBO file, printing the dump
> > of the image in U-Boot shell, etc.
> >
> > This command is needed to provide Android boot with new Android DT image
> > format further.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > Reviewed-by: Tom Rini <trini@konsulko.com>
>
> [..]
>
> > +U_BOOT_CMD(
> > +     dtimg, CONFIG_SYS_MAXARGS, 0, do_dtimg,
> > +     "manipulate dtb/dtbo Android image",
> > +     "dump <addr>\n"
> > +     "    - parse specified image and print its structure info\n"
> > +     "      <addr>: image address in RAM, in hex\n"
> > +     "dtimg start <addr> <index> <varname>\n"
> > +     "    - get address (hex) of FDT in the image, by index\n"
> > +     "      <addr>: image address in RAM, in hex\n"
> > +     "      <index>: index of desired FDT in the image\n"
> > +     "      <varname>: name of variable where to store address of FDT\n"
> > +     "dtimg size <addr> <index> <varname>\n"
> > +     "    - get size (hex, bytes) of FDT in the image, by index\n"
> > +     "      <addr>: image address in RAM, in hex\n"
> > +     "      <index>: index of desired FDT in the image\n"
> > +     "      <varname>: name of variable where to store size of FDT"
> > +);
>
> Since you are the author and the main stakeholder of "dtimg", could you
> kindly feedback the command usage you envision for getting the start and
> size of dtb/dtbo blob given a certain "id" and "rev" fields used by
> mkdtboimg.py [1] and visible in the output of U-Boot's "dtimg dump" [2]?
>
> One option would be to extend the existing "dtimg {start|size}" to
> accept an argument like "id:<val>" and "rev:<val>".
>
> Another possibility is to create brand new dtimg sub-command.
> What would be your preference? TIA.
>
> [1] https://android.googlesource.com/platform/system/libufdt/+/master/utils/src/mkdtboimg.py
> [2] https://gitlab.denx.de/u-boot/u-boot/commit/e63bf1b13b3a7a

Let me add some background information to clarify why new command was suggested.
We came up with this during brainstorming on what is the best way to
implement lookup fdt by id and rev fields.

First suggestion was to implement separate lookup command, e.g.:

--> dtimg lookup id:<board_id> <dt_image_index_variable>

Second one was to integrate it into existing start/size command to
make command look more natural:

--> dtimg start|size <addr> [<index>|id:<id>] <varname>

Then after some time I suggested to combine 'start/size' subcommands
into single 'range' subcommand:

--> dtimg range <addr> id:<id> [rev:<rev>] [<start_varname> [<size_varname>]]
--> dtimg range <addr> index:<index> [<start_varname> [<size_varname>]]

Benefits of such combining:
- Reduce chance of human mistake in between of this commands (for
example different values for start and size).
- Increase readability and slightly reduce parsing time.

Downsides:
This solution is a little revolutionary since it requires to start
long-term deprecation process of start/size subcommands.

So the main question to the community is next:
Are we ready to make long term deprecation in the name of benefits
mentioned above?

In case not - we have no other choice but to extend existing
start/size subcommands.

-- 
Best regards,
Roman Stratiienko
Global Logic Inc.

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Eugeniu Rosca]

Hi Roman,
(CC-ing Igor for Android topics)

On Wed, Nov 13, 2019 at 12:19:59PM +0200, Roman Stratiienko wrote:
> On Tue, Nov 12, 2019 at 8:18 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Hello Sam,
> >
> > On Thu, Aug 16, 2018 at 11:34:13PM +0300, Sam Protsenko wrote:
> > > dtimg command allows user to work with Android DTB/DTBO image format.
> > > Such as, getting the address of desired DTB/DTBO file, printing the dump
> > > of the image in U-Boot shell, etc.

[..]

> > Since you are the author and the main stakeholder of "dtimg", could you
> > kindly feedback the command usage you envision for getting the start and
> > size of dtb/dtbo blob given a certain "id" and "rev" fields used by
> > mkdtboimg.py [1] and visible in the output of U-Boot's "dtimg dump" [2]?
> >
> > One option would be to extend the existing "dtimg {start|size}" to
> > accept an argument like "id:<val>" and "rev:<val>".
> >
> > Another possibility is to create brand new dtimg sub-command.
> > What would be your preference? TIA.
> >
> > [1] https://android.googlesource.com/platform/system/libufdt/+/master/utils/src/mkdtboimg.py
> > [2] https://gitlab.denx.de/u-boot/u-boot/commit/e63bf1b13b3a7a
> 
> Let me add some background information to clarify why new command was suggested.
> We came up with this during brainstorming on what is the best way to
> implement lookup fdt by id and rev fields.
> 
> First suggestion was to implement separate lookup command, e.g.:
> 
> --> dtimg lookup id:<board_id> <dt_image_index_variable>
> 
> Second one was to integrate it into existing start/size command to
> make command look more natural:
> 
> --> dtimg start|size <addr> [<index>|id:<id>] <varname>
> 
> Then after some time I suggested to combine 'start/size' subcommands
> into single 'range' subcommand:
> 
> --> dtimg range <addr> id:<id> [rev:<rev>] [<start_varname> [<size_varname>]]
> --> dtimg range <addr> index:<index> [<start_varname> [<size_varname>]]
> 
> Benefits of such combining:
> - Reduce chance of human mistake in between of this commands (for
> example different values for start and size).
> - Increase readability and slightly reduce parsing time.
> 
> Downsides:
> This solution is a little revolutionary since it requires to start
> long-term deprecation process of start/size subcommands.

So, what you are proposing is to migrate away from the "start" and
"size" sub-commands in the long run. While I understand the good
intentions, let me share my opinion what this means for platform
maintainers like us.

As a platform maintainer, it is not uncommon to encounter a scenario
like below:
 - platform X reached "feature freeze" one year ago with U-Boot v2018.11
 - platform Y reaches "feature freeze" soon with U-Boot v2020.01

If "dtimg" is used in both platforms and the command usage is not
backward compatible, this generates all kind of overhead like the need
to maintain two different versions of boot scripts (in case you keep
track of them externally as text files, which is what we do).

This is hugely simplified involving one single command. Imagine
several U-Boot commands fundamentally changing their usage pattern
over time. Things would become pretty messy.

> 
> So the main question to the community is next:
> Are we ready to make long term deprecation in the name of benefits
> mentioned above?

I hope more people will feedback on that, but in my opinion, we have to
honor the existing "dtimg" usage as a blueprint and carefully add
backward-compatible changes to it. This roughly translates to, IMHO:
 - in case of adding a brand new sub-command, it must not overlap in
   its function with any of pre-existing sub-commands
 - in case of enriching/extending a pre-existing sub-command, it only
   sounds appropriate to me adding one or more _optional_ arguments to
   maintain backward compatibility

Both above points can be met if "dtimg {start|size}" is modified as
follows, to account for the "id" and "rev" fields:

 -> dtimg start|size <addr> <index> [id:<id>] [rev:<rev>] <varname>

Some usage examples derived from the above proposal:

 -> dtimg start|size <addr> <index> <varname> 
    current behavior
 -> dtimg start|size <addr> <index> id:<id> <varname> 
    get the "start|size" of the blob carrying "id" value <id>. Assuming
    there are several such blobs in the image, get the <index>th one
 -> dtimg start|size <addr> <index> rev:<rev> <varname> 
    get the "start|size" of the blob carrying "rev" value <rev>.
    Assuming there are several such blobs, get the <index>th one
 -> dtimg start|size <addr> <index> id:<id> rev:<rev> <varname> 
    get the "start|size" of the blob carrying "id" value <id> AND "rev"
    value <rev>. Assuming several such blobs, get the <index>th one

> 
> In case not - we have no other choice but to extend existing
> start/size subcommands.

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Eugeniu Rosca]

Hello Igor, hello Sam,

We still have high hopes getting your response to
https://patchwork.ozlabs.org/patch/958594/#2302310

If not given, we will proceed with implementing the proposal from
https://patchwork.ozlabs.org/patch/958594/#2303657

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Eugeniu Rosca]

Dear Igor and Sam,
Cc: Tom, Simon

On Mon, Nov 18, 2019 at 10:27:32PM +0100, Eugeniu Rosca wrote:
> Hello Igor, hello Sam,
> 
> We still have high hopes getting your response to
> https://patchwork.ozlabs.org/patch/958594/#2302310
> 
> If not given, we will proceed with implementing the proposal from
> https://patchwork.ozlabs.org/patch/958594/#2303657

As the ones responsible for Android-specific patches, would you kindly
review the functionality submitted to
https://patchwork.ozlabs.org/cover/1202575/ ?

In case you are not available in the next weeks, I hope Tom and Simon
will suggest a way to go forward with the new patches?

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Sam Protsenko]

Hi Eugeniu,

On Thu, Nov 14, 2019 at 12:58 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Roman,
> (CC-ing Igor for Android topics)
>
> On Wed, Nov 13, 2019 at 12:19:59PM +0200, Roman Stratiienko wrote:
> > On Tue, Nov 12, 2019 at 8:18 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > >
> > > Hello Sam,
> > >
> > > On Thu, Aug 16, 2018 at 11:34:13PM +0300, Sam Protsenko wrote:
> > > > dtimg command allows user to work with Android DTB/DTBO image format.
> > > > Such as, getting the address of desired DTB/DTBO file, printing the dump
> > > > of the image in U-Boot shell, etc.
>
> [..]
>
> > > Since you are the author and the main stakeholder of "dtimg", could you
> > > kindly feedback the command usage you envision for getting the start and
> > > size of dtb/dtbo blob given a certain "id" and "rev" fields used by
> > > mkdtboimg.py [1] and visible in the output of U-Boot's "dtimg dump" [2]?
> > >
> > > One option would be to extend the existing "dtimg {start|size}" to
> > > accept an argument like "id:<val>" and "rev:<val>".
> > >
> > > Another possibility is to create brand new dtimg sub-command.
> > > What would be your preference? TIA.
> > >
> > > [1] https://android.googlesource.com/platform/system/libufdt/+/master/utils/src/mkdtboimg.py
> > > [2] https://gitlab.denx.de/u-boot/u-boot/commit/e63bf1b13b3a7a
> >
> > Let me add some background information to clarify why new command was suggested.
> > We came up with this during brainstorming on what is the best way to
> > implement lookup fdt by id and rev fields.
> >
> > First suggestion was to implement separate lookup command, e.g.:
> >
> > --> dtimg lookup id:<board_id> <dt_image_index_variable>
> >
> > Second one was to integrate it into existing start/size command to
> > make command look more natural:
> >
> > --> dtimg start|size <addr> [<index>|id:<id>] <varname>
> >
> > Then after some time I suggested to combine 'start/size' subcommands
> > into single 'range' subcommand:
> >
> > --> dtimg range <addr> id:<id> [rev:<rev>] [<start_varname> [<size_varname>]]
> > --> dtimg range <addr> index:<index> [<start_varname> [<size_varname>]]
> >
> > Benefits of such combining:
> > - Reduce chance of human mistake in between of this commands (for
> > example different values for start and size).
> > - Increase readability and slightly reduce parsing time.
> >
> > Downsides:
> > This solution is a little revolutionary since it requires to start
> > long-term deprecation process of start/size subcommands.
>
> So, what you are proposing is to migrate away from the "start" and
> "size" sub-commands in the long run. While I understand the good
> intentions, let me share my opinion what this means for platform
> maintainers like us.
>
> As a platform maintainer, it is not uncommon to encounter a scenario
> like below:
>  - platform X reached "feature freeze" one year ago with U-Boot v2018.11
>  - platform Y reaches "feature freeze" soon with U-Boot v2020.01
>
> If "dtimg" is used in both platforms and the command usage is not
> backward compatible, this generates all kind of overhead like the need
> to maintain two different versions of boot scripts (in case you keep
> track of them externally as text files, which is what we do).
>
> This is hugely simplified involving one single command. Imagine
> several U-Boot commands fundamentally changing their usage pattern
> over time. Things would become pretty messy.
>
> >
> > So the main question to the community is next:
> > Are we ready to make long term deprecation in the name of benefits
> > mentioned above?
>
> I hope more people will feedback on that, but in my opinion, we have to
> honor the existing "dtimg" usage as a blueprint and carefully add
> backward-compatible changes to it. This roughly translates to, IMHO:
>  - in case of adding a brand new sub-command, it must not overlap in
>    its function with any of pre-existing sub-commands
>  - in case of enriching/extending a pre-existing sub-command, it only
>    sounds appropriate to me adding one or more _optional_ arguments to
>    maintain backward compatibility
>

We should never change any commands (user interface), unless they are
deprecated / completely broken / etc. So although it might be seen as
more logical to modify existing 'dtimg' commands, we shouldn't do that
if it's breaking existing user interfaces. It's similar to golden
kernel rule ("we don't break user space"). Hence new sub-commands
(probably) should be added. Sorry, I didn't have much time to read the
whole discussion yet. But please stick to next two rules:

   1. Don't break existing interface; change it only in
backward-compatible manner
   2. Don't introduce non-standard extensions or non-standard
usage/namings to 'dtimg'. If you add some functionality, it must be
present in official Android DTBO format documentation, and we should
stick to namings suggested there.

Not asking that as 'dtimg' original author (it doesn't matter much),
but I think it's just a common sense in projects as big as U-Boot or
Linux kernel. And you correctly pointed out all possible implications,
so I have nothing to add on that matter.

I'm gonna review the whole discussion and associated patches in the
next few days, so maybe you'll hear more valuable feedback from me
soon.

Also, we'll surely need to sync up on our patch series, as my Android
Boot patches also make use of some 'dtimg' functionality.
Unfortunately, I don't have much time left on my current project, so
I'd really like to get my pending patches merged until then.

Thanks!

> Both above points can be met if "dtimg {start|size}" is modified as
> follows, to account for the "id" and "rev" fields:
>
>  -> dtimg start|size <addr> <index> [id:<id>] [rev:<rev>] <varname>
>
> Some usage examples derived from the above proposal:
>
>  -> dtimg start|size <addr> <index> <varname>
>     current behavior
>  -> dtimg start|size <addr> <index> id:<id> <varname>
>     get the "start|size" of the blob carrying "id" value <id>. Assuming
>     there are several such blobs in the image, get the <index>th one
>  -> dtimg start|size <addr> <index> rev:<rev> <varname>
>     get the "start|size" of the blob carrying "rev" value <rev>.
>     Assuming there are several such blobs, get the <index>th one
>  -> dtimg start|size <addr> <index> id:<id> rev:<rev> <varname>
>     get the "start|size" of the blob carrying "id" value <id> AND "rev"
>     value <rev>. Assuming several such blobs, get the <index>th one
>
> >
> > In case not - we have no other choice but to extend existing
> > start/size subcommands.
>
> --
> Best Regards,
> Eugeniu