[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support

[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support

[Philipp Tomsich]

We support booting both from SD/MMC images and SPI images on the
RK3399-Q7 for different use-cases (e.g. external boot in development
from the SD card, internal boot from MMC or SPI depending on whether
the SPI flash is populated on any given configuration option).

In getting the SPI image support ready for production, we found a
few areas that warranted improvements:
- we had broken SPI bootstrap earlier in the changes introducting
  boot0-style images for the RK3399 (this needed fixing)
- in fixing the broken SPI padding calculation, it became apparent
  that it's best to refactor and document things before we make
  the same mistake again in the future
- with both SD/MMC and SPI images being used for various purposes
  by various people, the wrong image style was inadvertendly used
  in some tests... so we support for 'dumpimage' (i.e. verify_header
  and print_header) had to be added to quickly check the image
  type being handled

Note that with the refactored calculation of the image-size, we
don't pad the image to the maximum SPL size any longer, but pad
SD/MMC to the next 512 byte block (RK_BLK_SIZE) and SPI to the
next 2K boundary.


Philipp Tomsich (8):
  rockchip: mkimage: rkspi: include the header sector in the SPI size
    calculation
  rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI
    images
  rockchip: mkimage: Update comments for header size
  rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize
  rockchip: mkimage: clarify header0 initialisation
  rockchip: mkimage: play nice with dumpimage
  rockchip: mkimage: remove placeholder functions from rkimage
  rockchip: mkimage: add support for verify_header/print_header

 tools/rkcommon.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 tools/rkcommon.h |  29 ++++++++-
 tools/rkimage.c  |  21 +-----
 tools/rksd.c     |  47 +++++---------
 tools/rkspi.c    |  62 +++++++++---------
 5 files changed, 255 insertions(+), 99 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH v1 1/8] rockchip: mkimage: rkspi: include the header sector in the SPI size calculation

[Philipp Tomsich]

Our earlier change broke the generation of SPI images, by excluding the
2K used for header0 from the size-calculation.

This commit makes sure that these are included before calculating the
required total size (including the padding from the 2K-from-every-4K
conversion).

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 tools/rkspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/rkspi.c b/tools/rkspi.c
index d2d3fdd..0a15229 100644
--- a/tools/rkspi.c
+++ b/tools/rkspi.c
@@ -79,12 +79,12 @@ static int rkspi_vrec_header(struct image_tool_params *params,
 
 	rkcommon_vrec_header(params, tparams);
 
-	pad_size = (rkcommon_get_spl_size(params) + 0x7ff) / 0x800 * 0x800;
+	pad_size = ROUND(rkcommon_get_spl_size(params), 0x800);
 	params->orig_file_size = pad_size;
 
 	/* We will double the image size due to the SPI format */
-	pad_size *= 2;
 	pad_size += RK_SPL_HDR_START;
+	pad_size *= 2;
 	debug("pad_size %x\n", pad_size);
 
 	return pad_size - params->file_size - tparams->header_size;
-- 
1.9.1

[U-Boot] [PATCH v1 2/8] rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI images

[Philipp Tomsich]

In (first) breaking and (then) fixing the rkspi tool, I realised that
the calculation of the required padding (for the header-size and the
2K-in-every-4K SPI layout) was not as self-explainatory as it could
have been.  This change rewrites the code (using new, common functions
in rkcommon.c) and adds verbose in-line comments to ensure that we
won't fall into the same pit in the future...

Tested on the RK3399 (with has a boot0-style payload) with SD/MMC and SPI.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 tools/rkcommon.c | 20 ++++++++++++++++++--
 tools/rkcommon.h | 10 ++++++++--
 tools/rksd.c     | 23 ++++++++++++-----------
 tools/rkspi.c    | 41 +++++++++++++++++++++++++++--------------
 4 files changed, 65 insertions(+), 29 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 6cdb749..1311d65 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -199,9 +199,13 @@ void rkcommon_rc4_encode_spl(void *buf, unsigned int offset, unsigned int size)
 	}
 }
 
-void rkcommon_vrec_header(struct image_tool_params *params,
-			  struct image_type_params *tparams)
+int rkcommon_vrec_header(struct image_tool_params *params,
+			 struct image_type_params *tparams,
+			 unsigned int alignment)
 {
+	unsigned int  unpadded_size;
+	unsigned int  padded_size;
+
 	/*
 	 * The SPL image looks as follows:
 	 *
@@ -228,4 +232,16 @@ void rkcommon_vrec_header(struct image_tool_params *params,
 	tparams->hdr = malloc(tparams->header_size);
 	memset(tparams->hdr, 0, tparams->header_size);
 	tparams->header_size = tparams->header_size;
+
+	/*
+	 * If someone passed in 0 for the alignment, we'd better handle
+	 * it correctly...
+	 */
+	if (!alignment)
+		alignment = 1;
+
+	unpadded_size = tparams->header_size + params->file_size;
+	padded_size = ROUND(unpadded_size, alignment);
+
+	return padded_size - unpadded_size;
 }
diff --git a/tools/rkcommon.h b/tools/rkcommon.h
index cc161a6..a21321f 100644
--- a/tools/rkcommon.h
+++ b/tools/rkcommon.h
@@ -83,8 +83,14 @@ void rkcommon_rc4_encode_spl(void *buf, unsigned int offset, unsigned int size);
  * @params:     Pointer to the tool params structure
  * @tparams:    Pointer tot the image type structure (for setting
  *              the header and header_size)
+ * @alignment:  Alignment (a power of two) that the image should be
+ *              padded to (e.g. 512 if we want to align with SD/MMC
+ *              blocksizes or 2048 for the SPI format)
+ *
+ * @return bytes of padding required/added (does not include the header_size)
  */
-void rkcommon_vrec_header(struct image_tool_params *params,
-			  struct image_type_params *tparams);
+int rkcommon_vrec_header(struct image_tool_params *params,
+			 struct image_type_params *tparams,
+			 unsigned int alignment);
 
 #endif
diff --git a/tools/rksd.c b/tools/rksd.c
index ac8a67d..6dafedf 100644
--- a/tools/rksd.c
+++ b/tools/rksd.c
@@ -29,12 +29,20 @@ static void rksd_set_header(void *buf,  struct stat *sbuf,  int ifd,
 	unsigned int size;
 	int ret;
 
+	printf("params->file_size %d\n", params->file_size);
+	printf("params->orig_file_size %d\n", params->orig_file_size);
+
+	/*
+	 * We need to calculate this using 'RK_SPL_HDR_START' and not using
+	 * 'tparams->header_size', as the additional byte inserted when
+	 * 'is_boot0' is true counts towards the payload.
+	 */
 	size = params->file_size - RK_SPL_HDR_START;
 	ret = rkcommon_set_header(buf, size, params);
 	if (ret) {
 		/* TODO(sjg at chromium.org): This method should return an error */
-		printf("Warning: SPL image is too large (size %#x) and will not boot\n",
-		       size);
+		printf("Warning: SPL image is too large (size %#x) and will "
+		       "not boot\n", size);
 	}
 }
 
@@ -51,18 +59,11 @@ static int rksd_check_image_type(uint8_t type)
 		return EXIT_FAILURE;
 }
 
-/* We pad the file out to a fixed size - this method returns that size */
 static int rksd_vrec_header(struct image_tool_params *params,
 			    struct image_type_params *tparams)
 {
-	int pad_size;
-
-	rkcommon_vrec_header(params, tparams);
-
-	pad_size = RK_SPL_HDR_START + rkcommon_get_spl_size(params);
-	debug("pad_size %x\n", pad_size);
-
-	return pad_size - params->file_size - tparams->header_size;
+	/* We don't add any additional padding after the end of the image */
+	return rkcommon_vrec_header(params, tparams, 1);
 }
 
 /*
diff --git a/tools/rkspi.c b/tools/rkspi.c
index 0a15229..87bd1a9 100644
--- a/tools/rkspi.c
+++ b/tools/rkspi.c
@@ -39,8 +39,8 @@ static void rkspi_set_header(void *buf, struct stat *sbuf, int ifd,
 	debug("size %x\n", size);
 	if (ret) {
 		/* TODO(sjg at chromium.org): This method should return an error */
-		printf("Warning: SPL image is too large (size %#x) and will not boot\n",
-		       size);
+		printf("Warning: SPL image is too large (size %#x) and will "
+		       "not boot\n", size);
 	}
 
 	/*
@@ -71,23 +71,36 @@ static int rkspi_check_image_type(uint8_t type)
 		return EXIT_FAILURE;
 }
 
-/* We pad the file out to a fixed size - this method returns that size */
+/*
+ * The SPI payload needs to be padded out to make space for odd half-sector
+ * layout used in flash (i.e. only the first 2K of each 4K sector is used).
+ */
 static int rkspi_vrec_header(struct image_tool_params *params,
 			     struct image_type_params *tparams)
 {
-	int pad_size;
-
-	rkcommon_vrec_header(params, tparams);
-
-	pad_size = ROUND(rkcommon_get_spl_size(params), 0x800);
-	params->orig_file_size = pad_size;
+	int padding = rkcommon_vrec_header(params, tparams, 2048);
+	/*
+	 * The file size has not been adjusted at this point (our caller will
+	 * eventually add the header/padding to the file_size), so we need to
+	 * add up the header_size, file_size and padding ourselves.
+	 */
+	int padded_size = tparams->header_size + params->file_size + padding;
 
-	/* We will double the image size due to the SPI format */
-	pad_size += RK_SPL_HDR_START;
-	pad_size *= 2;
-	debug("pad_size %x\n", pad_size);
+	/*
+	 * We need to store the original file-size (i.e. before padding), as
+	 * imagetool does not set this during its adjustment of file_size.
+	 */
+	params->orig_file_size = padded_size;
 
-	return pad_size - params->file_size - tparams->header_size;
+	/*
+	 * Converting to the SPI format (i.e. splitting each 4K page into two
+	 * 2K subpages and then padding these 2K pages up to take a complete
+	 * 4K sector again) will will double the image size.
+	 *
+	 * Thus we return the padded_size as an additional padding requirement
+	 * (be sure to add this to the padding returned from the common code).
+	 */
+	return padded_size + padding;
 }
 
 /*
-- 
1.9.1

[U-Boot] [PATCH v1 3/8] rockchip: mkimage: Update comments for header size

[Philipp Tomsich]

The calculation of the variable header size in rkcommon_vrec_header
had been update twice in the earlier series (introducing boot0-style
images to deal with the alignment of the first instruction in 64bit
binaries). Unfortunately, I didn't update the comment twice (so it
remained out-of-date).

This change brings the comment back in-sync with what the code is
doing.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 tools/rkcommon.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 1311d65..cfd40ac 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -176,7 +176,7 @@ int rkcommon_set_header(void *buf, uint file_size,
 
 	rkcommon_set_header0(buf, file_size, params);
 
-	/* Set up the SPL name and add the AArch64 'nop' padding, if needed */
+	/* Set up the SPL name */
 	memcpy(&hdr->magic, rkcommon_get_spl_hdr(params), RK_SPL_HDR_SIZE);
 
 	if (rkcommon_need_rc4_spl(params))
@@ -211,17 +211,21 @@ int rkcommon_vrec_header(struct image_tool_params *params,
 	 *
 	 * 0x0    header0 (see rkcommon.c)
 	 * 0x800  spl_name ('RK30', ..., 'RK33')
+	 *        (start of the payload for AArch64 payloads: we expect the
+	 *        first 4 bytes to be available for overwriting with our
+	 *        spl_name)
 	 * 0x804  first instruction to be executed
-	 *        (image start for AArch32, 'nop' for AArch64))
-	 * 0x808  second instruction to be executed
-	 *        (image start for AArch64)
+	 *        (start of the image/payload for 32bit payloads)
 	 *
-	 * For AArch64 (ARMv8) payloads, we receive an input file that
-	 * needs to start on an 8-byte boundary (natural alignment), so
-	 * we need to put a NOP at 0x804.
+	 * For AArch64 (ARMv8) payloads, natural alignment (8-bytes) is
+	 * required for its sections (so the image we receive needs to
+	 * have the first 4 bytes reserved for the spl_name).  Reserving
+	 * these 4 bytes is done using the BOOT0_HOOK infrastructure.
 	 *
-	 * Depending on this, the header is either 0x804 or 0x808 bytes
-	 * in length.
+	 * Depending on this, the header is either 0x800 (if this is a
+	 * 'boot0'-style payload, which has reserved 4 bytes at the
+	 * beginning for the 'spl_name' and expects us to overwrite
+	 * its first 4 bytes) or 0x804 bytes in length.
 	 */
 	if (rkcommon_spl_is_boot0(params))
 		tparams->header_size = RK_SPL_HDR_START;
-- 
1.9.1

[U-Boot] [PATCH v1 4/8] rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize

[Philipp Tomsich]

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 tools/rksd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/rksd.c b/tools/rksd.c
index 6dafedf..8627b6d 100644
--- a/tools/rksd.c
+++ b/tools/rksd.c
@@ -62,8 +62,11 @@ static int rksd_check_image_type(uint8_t type)
 static int rksd_vrec_header(struct image_tool_params *params,
 			    struct image_type_params *tparams)
 {
-	/* We don't add any additional padding after the end of the image */
-	return rkcommon_vrec_header(params, tparams, 1);
+	/*
+	 * Pad to the RK_BLK_SIZE (512 bytes) to be consistent with init_size
+	 * being encoded in RK_BLK_SIZE units in header0 (see rkcommon.c).
+	 */
+	return rkcommon_vrec_header(params, tparams, RK_BLK_SIZE);
 }
 
 /*
-- 
1.9.1

[U-Boot] [PATCH v1 5/8] rockchip: mkimage: clarify header0 initialisation

[Philipp Tomsich]

This change set adds documentation to the header0 initialisation and
improves readability for the calculations of various offsets/lengths.

As the U-Boot SPL stage doesn't use any payload beyond what is covered
by init_size, we no longer add RK_MAX_BOOT_SIZE to init_boot_size.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 tools/rkcommon.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index cfd40ac..ed29ef9 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -13,6 +13,8 @@
 #include "mkimage.h"
 #include "rkcommon.h"
 
+#define DIV_ROUND_UP(n, d)	(((n) + (d) - 1) / (d))
+
 enum {
 	RK_SIGNATURE		= 0x0ff0aa55,
 };
@@ -159,9 +161,21 @@ static void rkcommon_set_header0(void *buf, uint file_size,
 	hdr->disable_rc4 = !rkcommon_need_rc4_spl(params);
 	hdr->init_offset = RK_INIT_OFFSET;
 
-	hdr->init_size = (file_size + RK_BLK_SIZE - 1) / RK_BLK_SIZE;
-	hdr->init_size = (hdr->init_size + 3) & ~3;
-	hdr->init_boot_size = hdr->init_size + RK_MAX_BOOT_SIZE / RK_BLK_SIZE;
+	hdr->init_size = DIV_ROUND_UP(file_size, RK_BLK_SIZE);
+	/*
+	 * The init_size has to be a multiple of 4 blocks (i.e. of 2K)
+	 * or the BootROM will not boot the image.
+	 *
+	 * Note: To verify that this is not a legacy constraint, we
+	 *       rechecked this against the RK3399 BootROM.
+	 */
+	hdr->init_size = ROUND(hdr->init_size, 4);
+	/*
+	 * The images we create do not contain the stage following the SPL as
+	 * part of the SPL image, so the init_boot_size (which might have been
+	 * read by Rockchip's miniloder) should be the same as the init_size.
+	 */
+	hdr->init_boot_size = hdr->init_size;
 
 	rc4_encode(buf, RK_BLK_SIZE, rc4_key);
 }
-- 
1.9.1

[U-Boot] [PATCH v1 6/8] rockchip: mkimage: play nice with dumpimage

[Philipp Tomsich]

Dumpimage (it invoked with "-T rkspi" or "-T rksd") would not work due
to check_params failing. These changes ensure that we can both be called
with an empty imagename.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 tools/rkcommon.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index ed29ef9..773e4f6 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -85,6 +85,9 @@ static struct spl_info *rkcommon_get_spl_info(char *imagename)
 {
 	int i;
 
+	if (!imagename)
+		return NULL;
+
 	for (i = 0; i < ARRAY_SIZE(spl_infos); i++)
 		if (!strncmp(imagename, spl_infos[i].imagename, 6))
 			return spl_infos + i;
@@ -97,17 +100,24 @@ int rkcommon_check_params(struct image_tool_params *params)
 	int i;
 
 	if (rkcommon_get_spl_info(params->imagename) != NULL)
-		return 0;
+		return EXIT_SUCCESS;
+
+	/*
+	 * If this is a operation (list or extract), the don't require
+	 * imagename to be set.
+	 */
+	if (params->lflag || params->iflag)
+		return EXIT_SUCCESS;
 
 	fprintf(stderr, "ERROR: imagename (%s) is not supported!\n",
-		strlen(params->imagename) > 0 ? params->imagename : "NULL");
+		params->imagename ? params->imagename : "NULL");
 
 	fprintf(stderr, "Available imagename:");
 	for (i = 0; i < ARRAY_SIZE(spl_infos); i++)
 		fprintf(stderr, "\t%s", spl_infos[i].imagename);
 	fprintf(stderr, "\n");
 
-	return -1;
+	return EXIT_FAILURE;
 }
 
 const char *rkcommon_get_spl_hdr(struct image_tool_params *params)
-- 
1.9.1

[U-Boot] [PATCH v1 7/8] rockchip: mkimage: remove placeholder functions from rkimage

[Philipp Tomsich]

The imagetool framework checks whether function pointer for the verify,
print and extract actions are available and will will handle their
absence appropriately.

This change removes the unnecessary functions and uses the driver
structure to convey available functionality to imagetool.  This is in
fact better than having verify just return 0 (which previously broke
dumpimage, as dumpimage assumed that we had handled the image and did
not continue to probe further).

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 tools/rkimage.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/tools/rkimage.c b/tools/rkimage.c
index 44d098c..9880b15 100644
--- a/tools/rkimage.c
+++ b/tools/rkimage.c
@@ -13,16 +13,6 @@
 
 static uint32_t header;
 
-static int rkimage_verify_header(unsigned char *buf, int size,
-				 struct image_tool_params *params)
-{
-	return 0;
-}
-
-static void rkimage_print_header(const void *buf)
-{
-}
-
 static void rkimage_set_header(void *buf, struct stat *sbuf, int ifd,
 			       struct image_tool_params *params)
 {
@@ -33,11 +23,6 @@ static void rkimage_set_header(void *buf, struct stat *sbuf, int ifd,
 		rkcommon_rc4_encode_spl(buf, 4, params->file_size);
 }
 
-static int rkimage_extract_subimage(void *buf, struct image_tool_params *params)
-{
-	return 0;
-}
-
 static int rkimage_check_image_type(uint8_t type)
 {
 	if (type == IH_TYPE_RKIMAGE)
@@ -55,10 +40,10 @@ U_BOOT_IMAGE_TYPE(
 	4,
 	&header,
 	rkcommon_check_params,
-	rkimage_verify_header,
-	rkimage_print_header,
+	NULL,
+	NULL,
 	rkimage_set_header,
-	rkimage_extract_subimage,
+	NULL,
 	rkimage_check_image_type,
 	NULL,
 	NULL
-- 
1.9.1

[U-Boot] [PATCH v1 8/8] rockchip: mkimage: add support for verify_header/print_header

[Philipp Tomsich]

The rockchip image generation was previously missing the ability to
verify the generated header (and dump the image-type) without having
to resort to hexdump or od. Experience in our testing has showed it
to be very easy to get the rkspi and rksd images mixed up and the
lab... so we add the necessary support to have dumpimage tell us
what image type we're dealing with.

This change set adds the verify_header and print_header capability
to the rksd/rkspi image drivers (through shared code in rkcommon).

As of now, we only support images fully that are not RC4-encoded for
the SPL payload (i.e. header1 and payload). For RC4-encoded payloads,
the outer header (header0) is checked, but no detection of whether
this is a SD/MMC or SPI formatted payload takes place.

The output of dumpsys now prints the image type (spl_hdr), whether it
is a SD/MMC or SPI image, and the (padded) size of the image:
  $ ./tools/dumpimage -l ./spl.img
  Image Type:   Rockchip RK33 (SD/MMC) boot image
                               ^^^^^^ SD/MMC vs. SPI indication
                         ^^^^ spl_hdr indicated by the image
  Data Size:    79872 bytes

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

 tools/rkcommon.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 tools/rkcommon.h |  19 +++++++++
 tools/rksd.c     |  29 +++-----------
 tools/rkspi.c    |  21 ++--------
 4 files changed, 146 insertions(+), 42 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 773e4f6..ae414b3 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -2,6 +2,8 @@
  * (C) Copyright 2015 Google,  Inc
  * Written by Simon Glass <sjg@chromium.org>
  *
+ * (C) 2017 Theobroma Systems Design und Consulting GmbH
+ *
  * SPDX-License-Identifier:	GPL-2.0+
  *
  * Helper functions for Rockchip images
@@ -200,7 +202,7 @@ int rkcommon_set_header(void *buf, uint file_size,
 
 	rkcommon_set_header0(buf, file_size, params);
 
-	/* Set up the SPL name */
+	/* Set up the SPL name (i.e. copy spl_hdr over) */
 	memcpy(&hdr->magic, rkcommon_get_spl_hdr(params), RK_SPL_HDR_SIZE);
 
 	if (rkcommon_need_rc4_spl(params))
@@ -210,6 +212,121 @@ int rkcommon_set_header(void *buf, uint file_size,
 	return 0;
 }
 
+static inline unsigned rkcommon_offset_to_spi(unsigned offset)
+{
+	/*
+	 * While SD/MMC images use a flat addressing, SPI images are padded
+	 * to use the first 2K of every 4K sector only.
+	 */
+	return ((offset & ~0x7ff) << 1) + (offset & 0x7ff);
+}
+
+static inline unsigned rkcommon_spi_to_offset(unsigned offset)
+{
+	return ((offset & ~0x7ff) >> 1) + (offset & 0x7ff);
+}
+
+static int rkcommon_parse_header(const void *buf, struct header0_info *header0,
+				 struct spl_info **spl_info)
+{
+	unsigned hdr1_offset;
+	struct header1_info *hdr1_sdmmc, *hdr1_spi;
+	int i;
+
+	if (spl_info)
+		*spl_info = NULL;
+
+	/*
+	 * The first header (hdr0) is always RC4 encoded, so try to decrypt
+	 * with the well-known key.
+	 */
+	memcpy((void *)header0, buf, sizeof(struct header0_info));
+	rc4_encode((void *)header0, sizeof(struct header0_info), rc4_key);
+
+	if (header0->signature != RK_SIGNATURE)
+		return -FDT_ERR_BADSTRUCTURE;
+
+	/* We don't support RC4 encoded image payloads here, yet... */
+	if (header0->disable_rc4 == 0)
+		return -ENOSYS;
+
+	hdr1_offset = header0->init_offset * RK_BLK_SIZE;
+	hdr1_sdmmc = (struct header1_info *)(buf + hdr1_offset);
+	hdr1_spi = (struct header1_info *)(buf +
+					   rkcommon_offset_to_spi(hdr1_offset));
+
+	for (i = 0; i < ARRAY_SIZE(spl_infos); i++) {
+		if (!memcmp(&hdr1_sdmmc->magic, spl_infos[i].spl_hdr, 4)) {
+			if (spl_info)
+				*spl_info = &spl_infos[i];
+			return IH_TYPE_RKSD;
+		} else if (!memcmp(&hdr1_spi->magic, spl_infos[i].spl_hdr, 4)) {
+			if (spl_info)
+				*spl_info = &spl_infos[i];
+			return IH_TYPE_RKSPI;
+		}
+	}
+
+	return -1;
+}
+
+int rkcommon_verify_header(unsigned char *buf, int size,
+			   struct image_tool_params *params)
+{
+	struct header0_info header0;
+	struct spl_info *img_spl_info, *spl_info;
+	int ret;
+
+	ret = rkcommon_parse_header(buf, &header0, &img_spl_info);
+
+	/* If this is the (unimplemented) RC4 case, then rewrite the result */
+	if (ret == -ENOSYS)
+		return 0;
+
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * If no 'imagename' is specified via the commandline (e.g. if this is
+	 * 'dumpimage -l' w/o any further constraints), we accept any spl_info.
+	 */
+	if (params->imagename == NULL)
+		return 0;
+
+	/* Match the 'imagename' against the 'spl_hdr' found */
+	spl_info = rkcommon_get_spl_info(params->imagename);
+	if (spl_info && img_spl_info)
+		return strcmp(spl_info->spl_hdr, img_spl_info->spl_hdr);
+
+	return -ENOENT;
+}
+
+void rkcommon_print_header(const void *buf)
+{
+	struct header0_info header0;
+	struct spl_info *spl_info;
+	uint8_t image_type;
+	int ret;
+
+	ret = rkcommon_parse_header(buf, &header0, &spl_info);
+
+	/* If this is the (unimplemented) RC4 case, then fail silently */
+	if (ret == -ENOSYS)
+		return;
+
+	if (ret < 0) {
+		fprintf(stderr, "Error: image verification failed\n");
+		return;
+	}
+
+	image_type = ret;
+
+	printf("Image Type:   Rockchip %s (%s) boot image\n",
+	       spl_info->spl_hdr,
+	       (image_type == IH_TYPE_RKSD) ? "SD/MMC" : "SPI");
+	printf("Data Size:    %d bytes\n", header0.init_size * RK_BLK_SIZE);
+}
+
 void rkcommon_rc4_encode_spl(void *buf, unsigned int offset, unsigned int size)
 {
 	unsigned int remaining = size;
diff --git a/tools/rkcommon.h b/tools/rkcommon.h
index a21321f..75a4ed6 100644
--- a/tools/rkcommon.h
+++ b/tools/rkcommon.h
@@ -56,6 +56,25 @@ int rkcommon_set_header(void *buf, uint file_size,
 			struct image_tool_params *params);
 
 /**
+ * rkcommon_verify_header() - verify the header for a Rockchip boot image
+ *
+ * @buf:	Pointer to the image file
+ * @file_size:	Size of entire bootable image file (incl. all padding)
+ * @return 0 if OK
+ */
+int rkcommon_verify_header(unsigned char *buf, int size,
+			   struct image_tool_params *params);
+
+/**
+ * rkcommon_print_header() - print the header for a Rockchip boot image
+ *
+ * This prints the header, spl_name and whether this is a SD/MMC or SPI image.
+ *
+ * @buf:	Pointer to the image (can be a read-only file-mapping)
+ */
+void rkcommon_print_header(const void *buf);
+
+/**
  * rkcommon_need_rc4_spl() - check if rc4 encoded spl is required
  *
  * Some socs cannot disable the rc4-encryption of the spl binary.
diff --git a/tools/rksd.c b/tools/rksd.c
index 8627b6d..a880a26 100644
--- a/tools/rksd.c
+++ b/tools/rksd.c
@@ -13,29 +13,17 @@
 #include "mkimage.h"
 #include "rkcommon.h"
 
-static int rksd_verify_header(unsigned char *buf,  int size,
-				 struct image_tool_params *params)
-{
-	return 0;
-}
-
-static void rksd_print_header(const void *buf)
-{
-}
-
 static void rksd_set_header(void *buf,  struct stat *sbuf,  int ifd,
-			       struct image_tool_params *params)
+			    struct image_tool_params *params)
 {
 	unsigned int size;
 	int ret;
 
-	printf("params->file_size %d\n", params->file_size);
-	printf("params->orig_file_size %d\n", params->orig_file_size);
-
 	/*
 	 * We need to calculate this using 'RK_SPL_HDR_START' and not using
 	 * 'tparams->header_size', as the additional byte inserted when
-	 * 'is_boot0' is true counts towards the payload.
+	 * 'is_boot0' is true counts towards the payload (and not towards the
+	 * header).
 	 */
 	size = params->file_size - RK_SPL_HDR_START;
 	ret = rkcommon_set_header(buf, size, params);
@@ -46,11 +34,6 @@ static void rksd_set_header(void *buf,  struct stat *sbuf,  int ifd,
 	}
 }
 
-static int rksd_extract_subimage(void *buf,  struct image_tool_params *params)
-{
-	return 0;
-}
-
 static int rksd_check_image_type(uint8_t type)
 {
 	if (type == IH_TYPE_RKSD)
@@ -78,10 +61,10 @@ U_BOOT_IMAGE_TYPE(
 	0,
 	NULL,
 	rkcommon_check_params,
-	rksd_verify_header,
-	rksd_print_header,
+	rkcommon_verify_header,
+	rkcommon_print_header,
 	rksd_set_header,
-	rksd_extract_subimage,
+	NULL,
 	rksd_check_image_type,
 	NULL,
 	rksd_vrec_header
diff --git a/tools/rkspi.c b/tools/rkspi.c
index 87bd1a9..f8a565d 100644
--- a/tools/rkspi.c
+++ b/tools/rkspi.c
@@ -17,16 +17,6 @@ enum {
 	RKSPI_SECT_LEN		= RK_BLK_SIZE * 4,
 };
 
-static int rkspi_verify_header(unsigned char *buf, int size,
-			       struct image_tool_params *params)
-{
-	return 0;
-}
-
-static void rkspi_print_header(const void *buf)
-{
-}
-
 static void rkspi_set_header(void *buf, struct stat *sbuf, int ifd,
 			     struct image_tool_params *params)
 {
@@ -58,11 +48,6 @@ static void rkspi_set_header(void *buf, struct stat *sbuf, int ifd,
 	}
 }
 
-static int rkspi_extract_subimage(void *buf, struct image_tool_params *params)
-{
-	return 0;
-}
-
 static int rkspi_check_image_type(uint8_t type)
 {
 	if (type == IH_TYPE_RKSPI)
@@ -112,10 +97,10 @@ U_BOOT_IMAGE_TYPE(
 	0,
 	NULL,
 	rkcommon_check_params,
-	rkspi_verify_header,
-	rkspi_print_header,
+	rkcommon_verify_header,
+	rkcommon_print_header,
 	rkspi_set_header,
-	rkspi_extract_subimage,
+	NULL,
 	rkspi_check_image_type,
 	NULL,
 	rkspi_vrec_header
-- 
1.9.1

[U-Boot] [PATCH v1 1/8] rockchip: mkimage: rkspi: include the header sector in the SPI size calculation

[Simon Glass]

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:
>
> Our earlier change broke the generation of SPI images, by excluding the
> 2K used for header0 from the size-calculation.
>
> This commit makes sure that these are included before calculating the
> required total size (including the padding from the 2K-from-every-4K
> conversion).
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> tools/rkspi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

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

[U-Boot] [PATCH v1 2/8] rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI images

[Simon Glass]

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:
> In (first) breaking and (then) fixing the rkspi tool, I realised that
> the calculation of the required padding (for the header-size and the
> 2K-in-every-4K SPI layout) was not as self-explainatory as it could
> have been. This change rewrites the code (using new, common functions
> in rkcommon.c) and adds verbose in-line comments to ensure that we
> won't fall into the same pit in the future...
>
> Tested on the RK3399 (with has a boot0-style payload) with SD/MMC and SPI.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> tools/rkcommon.c | 20 ++++++++++++++++++--
> tools/rkcommon.h | 10 ++++++++--
> tools/rksd.c | 23 ++++++++++++-----------
> tools/rkspi.c | 41 +++++++++++++++++++++++++++--------------
> 4 files changed, 65 insertions(+), 29 deletions(-)
>

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

[U-Boot] [PATCH v1 3/8] rockchip: mkimage: Update comments for header size

[Simon Glass]

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:
> The calculation of the variable header size in rkcommon_vrec_header
> had been update twice in the earlier series (introducing boot0-style
> images to deal with the alignment of the first instruction in 64bit
> binaries). Unfortunately, I didn't update the comment twice (so it
> remained out-of-date).
>
> This change brings the comment back in-sync with what the code is
> doing.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> tools/rkcommon.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>

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

[U-Boot] [PATCH v1 4/8] rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize

[Simon Glass]

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:

Commit message?

> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> tools/rksd.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>

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

[U-Boot] [PATCH v1 5/8] rockchip: mkimage: clarify header0 initialisation

[Simon Glass]

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:
> This change set adds documentation to the header0 initialisation and
> improves readability for the calculations of various offsets/lengths.
>
> As the U-Boot SPL stage doesn't use any payload beyond what is covered
> by init_size, we no longer add RK_MAX_BOOT_SIZE to init_boot_size.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> tools/rkcommon.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)

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

[U-Boot] [PATCH v1 7/8] rockchip: mkimage: remove placeholder functions from rkimage

[Simon Glass]

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:
> The imagetool framework checks whether function pointer for the verify,
> print and extract actions are available and will will handle their
> absence appropriately.
>
> This change removes the unnecessary functions and uses the driver
> structure to convey available functionality to imagetool. This is in
> fact better than having verify just return 0 (which previously broke
> dumpimage, as dumpimage assumed that we had handled the image and did
> not continue to probe further).
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> tools/rkimage.c | 21 +++------------------
> 1 file changed, 3 insertions(+), 18 deletions(-)

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

[U-Boot] [PATCH v1 8/8] rockchip: mkimage: add support for verify_header/print_header

[Simon Glass]

Hi Philipp,

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:
> The rockchip image generation was previously missing the ability to
> verify the generated header (and dump the image-type) without having
> to resort to hexdump or od. Experience in our testing has showed it
> to be very easy to get the rkspi and rksd images mixed up and the
> lab... so we add the necessary support to have dumpimage tell us
> what image type we're dealing with.
>
> This change set adds the verify_header and print_header capability
> to the rksd/rkspi image drivers (through shared code in rkcommon).
>
> As of now, we only support images fully that are not RC4-encoded for
> the SPL payload (i.e. header1 and payload). For RC4-encoded payloads,
> the outer header (header0) is checked, but no detection of whether
> this is a SD/MMC or SPI formatted payload takes place.
>
> The output of dumpsys now prints the image type (spl_hdr), whether it
> is a SD/MMC or SPI image, and the (padded) size of the image:
> $ ./tools/dumpimage -l ./spl.img
> Image Type: Rockchip RK33 (SD/MMC) boot image
> ^^^^^^ SD/MMC vs. SPI indication
> ^^^^ spl_hdr indicated by the image
> Data Size: 79872 bytes
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> tools/rkcommon.c | 119
++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> tools/rkcommon.h | 19 +++++++++
> tools/rksd.c | 29 +++-----------
> tools/rkspi.c | 21 ++--------
> 4 files changed, 146 insertions(+), 42 deletions(-)

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

But please see below.

>
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index 773e4f6..ae414b3 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -2,6 +2,8 @@
> * (C) Copyright 2015 Google, Inc
> * Written by Simon Glass <sjg@chromium.org>
> *
> + * (C) 2017 Theobroma Systems Design und Consulting GmbH
> + *
> * SPDX-License-Identifier: GPL-2.0+
> *
> * Helper functions for Rockchip images
> @@ -200,7 +202,7 @@ int rkcommon_set_header(void *buf, uint file_size,
>
> rkcommon_set_header0(buf, file_size, params);
>
> - /* Set up the SPL name */
> + /* Set up the SPL name (i.e. copy spl_hdr over) */
> memcpy(&hdr->magic, rkcommon_get_spl_hdr(params), RK_SPL_HDR_SIZE);
>
> if (rkcommon_need_rc4_spl(params))
> @@ -210,6 +212,121 @@ int rkcommon_set_header(void *buf, uint file_size,
> return 0;
> }
>
> +static inline unsigned rkcommon_offset_to_spi(unsigned offset)
> +{
> + /*
> + * While SD/MMC images use a flat addressing, SPI images are padded
> + * to use the first 2K of every 4K sector only.
> + */
> + return ((offset & ~0x7ff) << 1) + (offset & 0x7ff);
> +}
> +
> +static inline unsigned rkcommon_spi_to_offset(unsigned offset)
> +{
> + return ((offset & ~0x7ff) >> 1) + (offset & 0x7ff);
> +}
> +
> +static int rkcommon_parse_header(const void *buf, struct header0_info
*header0,
> + struct spl_info **spl_info)
> +{
> + unsigned hdr1_offset;
> + struct header1_info *hdr1_sdmmc, *hdr1_spi;
> + int i;
> +
> + if (spl_info)
> + *spl_info = NULL;
> +
> + /*
> + * The first header (hdr0) is always RC4 encoded, so try to decrypt
> + * with the well-known key.
> + */
> + memcpy((void *)header0, buf, sizeof(struct header0_info));
> + rc4_encode((void *)header0, sizeof(struct header0_info), rc4_key);
> +
> + if (header0->signature != RK_SIGNATURE)
> + return -FDT_ERR_BADSTRUCTURE;

Can you choose a standard error? We cannot mix and match erno and libfdt.h

> +
> + /* We don't support RC4 encoded image payloads here, yet... */
> + if (header0->disable_rc4 == 0)
> + return -ENOSYS;
> +

Regards,
Simon

[U-Boot] [PATCH v1 6/8] rockchip: mkimage: play nice with dumpimage

[Simon Glass]

On 17 April 2017 at 09:48, Philipp Tomsich <
philipp.tomsich@theobroma-systems.com> wrote:
> Dumpimage (it invoked with "-T rkspi" or "-T rksd") would not work due
> to check_params failing. These changes ensure that we can both be called
> with an empty imagename.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> tools/rkcommon.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)

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

[U-Boot] [PATCH v1 1/8] rockchip: mkimage: rkspi: include the header sector in the SPI size calculation

[Simon Glass]

On 17 April 2017 at 22:00, Simon Glass <sjg@chromium.org> wrote:
> On 17 April 2017 at 09:48, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>>
>> Our earlier change broke the generation of SPI images, by excluding the
>> 2K used for header0 from the size-calculation.
>>
>> This commit makes sure that these are included before calculating the
>> required total size (including the padding from the 2K-from-every-4K
>> conversion).
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>> tools/rkspi.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-rockchip/next, thanks!

[U-Boot] [PATCH v1 2/8] rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI images

[Simon Glass]

On 17 April 2017 at 22:00, Simon Glass <sjg@chromium.org> wrote:
> On 17 April 2017 at 09:48, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> In (first) breaking and (then) fixing the rkspi tool, I realised that
>> the calculation of the required padding (for the header-size and the
>> 2K-in-every-4K SPI layout) was not as self-explainatory as it could
>> have been. This change rewrites the code (using new, common functions
>> in rkcommon.c) and adds verbose in-line comments to ensure that we
>> won't fall into the same pit in the future...
>>
>> Tested on the RK3399 (with has a boot0-style payload) with SD/MMC and SPI.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>> tools/rkcommon.c | 20 ++++++++++++++++++--
>> tools/rkcommon.h | 10 ++++++++--
>> tools/rksd.c | 23 ++++++++++++-----------
>> tools/rkspi.c | 41 +++++++++++++++++++++++++++--------------
>> 4 files changed, 65 insertions(+), 29 deletions(-)
>>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-rockchip/next, thanks!

[U-Boot] [PATCH v1 3/8] rockchip: mkimage: Update comments for header size

[Simon Glass]

On 17 April 2017 at 22:00, Simon Glass <sjg@chromium.org> wrote:
> On 17 April 2017 at 09:48, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> The calculation of the variable header size in rkcommon_vrec_header
>> had been update twice in the earlier series (introducing boot0-style
>> images to deal with the alignment of the first instruction in 64bit
>> binaries). Unfortunately, I didn't update the comment twice (so it
>> remained out-of-date).
>>
>> This change brings the comment back in-sync with what the code is
>> doing.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>> tools/rkcommon.c | 22 +++++++++++++---------
>> 1 file changed, 13 insertions(+), 9 deletions(-)
>>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-rockchip/next, thanks!

[U-Boot] [PATCH v1 4/8] rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize

[Simon Glass]

On 17 April 2017 at 22:00, Simon Glass <sjg@chromium.org> wrote:
> On 17 April 2017 at 09:48, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>
> Commit message?
>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>> tools/rksd.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-rockchip/next, thanks!

[U-Boot] [PATCH v1 5/8] rockchip: mkimage: clarify header0 initialisation

[Simon Glass]

On 17 April 2017 at 22:00, Simon Glass <sjg@chromium.org> wrote:
> On 17 April 2017 at 09:48, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> This change set adds documentation to the header0 initialisation and
>> improves readability for the calculations of various offsets/lengths.
>>
>> As the U-Boot SPL stage doesn't use any payload beyond what is covered
>> by init_size, we no longer add RK_MAX_BOOT_SIZE to init_boot_size.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>> tools/rkcommon.c | 20 +++++++++++++++++---
>> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-rockchip/next, thanks!

[U-Boot] [PATCH v1 7/8] rockchip: mkimage: remove placeholder functions from rkimage

[Simon Glass]

On 17 April 2017 at 22:00, Simon Glass <sjg@chromium.org> wrote:
> On 17 April 2017 at 09:48, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> The imagetool framework checks whether function pointer for the verify,
>> print and extract actions are available and will will handle their
>> absence appropriately.
>>
>> This change removes the unnecessary functions and uses the driver
>> structure to convey available functionality to imagetool. This is in
>> fact better than having verify just return 0 (which previously broke
>> dumpimage, as dumpimage assumed that we had handled the image and did
>> not continue to probe further).
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>> tools/rkimage.c | 21 +++------------------
>> 1 file changed, 3 insertions(+), 18 deletions(-)
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-rockchip/next, thanks!

[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support

[Kever Yang]

Hi Philipp,

This patch makes all the Rockchip SoCs with BACK_TO_BROM enabled can not 
work,

does the size correct for the SPL correct?

Thanks,
- Kever
On 04/17/2017 11:47 PM, Philipp Tomsich wrote:
> We support booting both from SD/MMC images and SPI images on the
> RK3399-Q7 for different use-cases (e.g. external boot in development
> from the SD card, internal boot from MMC or SPI depending on whether
> the SPI flash is populated on any given configuration option).
>
> In getting the SPI image support ready for production, we found a
> few areas that warranted improvements:
> - we had broken SPI bootstrap earlier in the changes introducting
>    boot0-style images for the RK3399 (this needed fixing)
> - in fixing the broken SPI padding calculation, it became apparent
>    that it's best to refactor and document things before we make
>    the same mistake again in the future
> - with both SD/MMC and SPI images being used for various purposes
>    by various people, the wrong image style was inadvertendly used
>    in some tests... so we support for 'dumpimage' (i.e. verify_header
>    and print_header) had to be added to quickly check the image
>    type being handled
>
> Note that with the refactored calculation of the image-size, we
> don't pad the image to the maximum SPL size any longer, but pad
> SD/MMC to the next 512 byte block (RK_BLK_SIZE) and SPI to the
> next 2K boundary.
>
>
> Philipp Tomsich (8):
>    rockchip: mkimage: rkspi: include the header sector in the SPI size
>      calculation
>    rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI
>      images
>    rockchip: mkimage: Update comments for header size
>    rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize
>    rockchip: mkimage: clarify header0 initialisation
>    rockchip: mkimage: play nice with dumpimage
>    rockchip: mkimage: remove placeholder functions from rkimage
>    rockchip: mkimage: add support for verify_header/print_header
>
>   tools/rkcommon.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>   tools/rkcommon.h |  29 ++++++++-
>   tools/rkimage.c  |  21 +-----
>   tools/rksd.c     |  47 +++++---------
>   tools/rkspi.c    |  62 +++++++++---------
>   5 files changed, 255 insertions(+), 99 deletions(-)
>

[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support

[Dr. Philipp Tomsich]

Kever,

What are the requirements for BACK_TO_BROM?
All I can see about how BACK_TO_BROM works is that it needs to save the register
context on the stack for returning to the ROM, but that seems to be only half the story.

Assuming that the header0 structure plays into this, the only significant change there
is that I don’t set the 'hdr->init_boot_size’ to the maximum SPL size any longer...

Regards,
Philipp.

> On 17 May 2017, at 11:50, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> Hi Philipp,
> 
> This patch makes all the Rockchip SoCs with BACK_TO_BROM enabled can not work,
> 
> does the size correct for the SPL correct?
> 
> Thanks,
> - Kever
> On 04/17/2017 11:47 PM, Philipp Tomsich wrote:
>> We support booting both from SD/MMC images and SPI images on the
>> RK3399-Q7 for different use-cases (e.g. external boot in development
>> from the SD card, internal boot from MMC or SPI depending on whether
>> the SPI flash is populated on any given configuration option).
>> 
>> In getting the SPI image support ready for production, we found a
>> few areas that warranted improvements:
>> - we had broken SPI bootstrap earlier in the changes introducting
>>   boot0-style images for the RK3399 (this needed fixing)
>> - in fixing the broken SPI padding calculation, it became apparent
>>   that it's best to refactor and document things before we make
>>   the same mistake again in the future
>> - with both SD/MMC and SPI images being used for various purposes
>>   by various people, the wrong image style was inadvertendly used
>>   in some tests... so we support for 'dumpimage' (i.e. verify_header
>>   and print_header) had to be added to quickly check the image
>>   type being handled
>> 
>> Note that with the refactored calculation of the image-size, we
>> don't pad the image to the maximum SPL size any longer, but pad
>> SD/MMC to the next 512 byte block (RK_BLK_SIZE) and SPI to the
>> next 2K boundary.
>> 
>> 
>> Philipp Tomsich (8):
>>   rockchip: mkimage: rkspi: include the header sector in the SPI size
>>     calculation
>>   rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI
>>     images
>>   rockchip: mkimage: Update comments for header size
>>   rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize
>>   rockchip: mkimage: clarify header0 initialisation
>>   rockchip: mkimage: play nice with dumpimage
>>   rockchip: mkimage: remove placeholder functions from rkimage
>>   rockchip: mkimage: add support for verify_header/print_header
>> 
>>  tools/rkcommon.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  tools/rkcommon.h |  29 ++++++++-
>>  tools/rkimage.c  |  21 +-----
>>  tools/rksd.c     |  47 +++++---------
>>  tools/rkspi.c    |  62 +++++++++---------
>>  5 files changed, 255 insertions(+), 99 deletions(-)
>> 
> 
> 

[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support

[Heiko Stuebner]

Hi Philipp,

Am Mittwoch, 17. Mai 2017, 12:12:51 CEST schrieb Dr. Philipp Tomsich:
> What are the requirements for BACK_TO_BROM?
> All I can see about how BACK_TO_BROM works is that it needs to save the register
> context on the stack for returning to the ROM, but that seems to be only half the story.
> 
> Assuming that the header0 structure plays into this, the only significant change there
> is that I don’t set the 'hdr->init_boot_size’ to the maximum SPL size any longer...

Which is most likely the problem. back_to_bootrom-images are concatenated
with the spl in front (init_size) and when returned to the bootrom it
reads the rest up to init_boot_size into the sdram.

So ideally we would return that line back to RK_MAX_BOOT_SIZE (512KB).
Somewhat safe value and boards not using back_to_bootrom, as this value
really only affects that second stage and not the actual spl loading.

I'm sadly away from my boardfarm this and next week, so testing bootloader
on my rk3188 board can only happend after that, but I'm somewhat
confident that this would solve the problem. Maybe Kever can test that
meanwhile.


Heiko


> 
> Regards,
> Philipp.
> 
> > On 17 May 2017, at 11:50, Kever Yang <kever.yang@rock-chips.com> wrote:
> > 
> > Hi Philipp,
> > 
> > This patch makes all the Rockchip SoCs with BACK_TO_BROM enabled can not work,
> > 
> > does the size correct for the SPL correct?
> > 
> > Thanks,
> > - Kever
> > On 04/17/2017 11:47 PM, Philipp Tomsich wrote:
> >> We support booting both from SD/MMC images and SPI images on the
> >> RK3399-Q7 for different use-cases (e.g. external boot in development
> >> from the SD card, internal boot from MMC or SPI depending on whether
> >> the SPI flash is populated on any given configuration option).
> >> 
> >> In getting the SPI image support ready for production, we found a
> >> few areas that warranted improvements:
> >> - we had broken SPI bootstrap earlier in the changes introducting
> >>   boot0-style images for the RK3399 (this needed fixing)
> >> - in fixing the broken SPI padding calculation, it became apparent
> >>   that it's best to refactor and document things before we make
> >>   the same mistake again in the future
> >> - with both SD/MMC and SPI images being used for various purposes
> >>   by various people, the wrong image style was inadvertendly used
> >>   in some tests... so we support for 'dumpimage' (i.e. verify_header
> >>   and print_header) had to be added to quickly check the image
> >>   type being handled
> >> 
> >> Note that with the refactored calculation of the image-size, we
> >> don't pad the image to the maximum SPL size any longer, but pad
> >> SD/MMC to the next 512 byte block (RK_BLK_SIZE) and SPI to the
> >> next 2K boundary.
> >> 
> >> 
> >> Philipp Tomsich (8):
> >>   rockchip: mkimage: rkspi: include the header sector in the SPI size
> >>     calculation
> >>   rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI
> >>     images
> >>   rockchip: mkimage: Update comments for header size
> >>   rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize
> >>   rockchip: mkimage: clarify header0 initialisation
> >>   rockchip: mkimage: play nice with dumpimage
> >>   rockchip: mkimage: remove placeholder functions from rkimage
> >>   rockchip: mkimage: add support for verify_header/print_header
> >> 
> >>  tools/rkcommon.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >>  tools/rkcommon.h |  29 ++++++++-
> >>  tools/rkimage.c  |  21 +-----
> >>  tools/rksd.c     |  47 +++++---------
> >>  tools/rkspi.c    |  62 +++++++++---------
> >>  5 files changed, 255 insertions(+), 99 deletions(-)
> >> 
> > 
> > 
> 
> 
> 

[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support

[Dr. Philipp Tomsich]

Heiko,

thanks for the insight into the BROM.
I’ll respin this with part of the change reverted and have Kever test.

Regards,
Philipp.

> On 19 May 2017, at 20:39, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> Hi Philipp,
> 
> Am Mittwoch, 17. Mai 2017, 12:12:51 CEST schrieb Dr. Philipp Tomsich:
>> What are the requirements for BACK_TO_BROM?
>> All I can see about how BACK_TO_BROM works is that it needs to save the register
>> context on the stack for returning to the ROM, but that seems to be only half the story.
>> 
>> Assuming that the header0 structure plays into this, the only significant change there
>> is that I don’t set the 'hdr->init_boot_size’ to the maximum SPL size any longer...
> 
> Which is most likely the problem. back_to_bootrom-images are concatenated
> with the spl in front (init_size) and when returned to the bootrom it
> reads the rest up to init_boot_size into the sdram.
> 
> So ideally we would return that line back to RK_MAX_BOOT_SIZE (512KB).
> Somewhat safe value and boards not using back_to_bootrom, as this value
> really only affects that second stage and not the actual spl loading.
> 
> I'm sadly away from my boardfarm this and next week, so testing bootloader
> on my rk3188 board can only happend after that, but I'm somewhat
> confident that this would solve the problem. Maybe Kever can test that
> meanwhile.
> 
> 
> Heiko
> 
> 
>> 
>> Regards,
>> Philipp.
>> 
>>> On 17 May 2017, at 11:50, Kever Yang <kever.yang@rock-chips.com> wrote:
>>> 
>>> Hi Philipp,
>>> 
>>> This patch makes all the Rockchip SoCs with BACK_TO_BROM enabled can not work,
>>> 
>>> does the size correct for the SPL correct?
>>> 
>>> Thanks,
>>> - Kever
>>> On 04/17/2017 11:47 PM, Philipp Tomsich wrote:
>>>> We support booting both from SD/MMC images and SPI images on the
>>>> RK3399-Q7 for different use-cases (e.g. external boot in development
>>>> from the SD card, internal boot from MMC or SPI depending on whether
>>>> the SPI flash is populated on any given configuration option).
>>>> 
>>>> In getting the SPI image support ready for production, we found a
>>>> few areas that warranted improvements:
>>>> - we had broken SPI bootstrap earlier in the changes introducting
>>>>  boot0-style images for the RK3399 (this needed fixing)
>>>> - in fixing the broken SPI padding calculation, it became apparent
>>>>  that it's best to refactor and document things before we make
>>>>  the same mistake again in the future
>>>> - with both SD/MMC and SPI images being used for various purposes
>>>>  by various people, the wrong image style was inadvertendly used
>>>>  in some tests... so we support for 'dumpimage' (i.e. verify_header
>>>>  and print_header) had to be added to quickly check the image
>>>>  type being handled
>>>> 
>>>> Note that with the refactored calculation of the image-size, we
>>>> don't pad the image to the maximum SPL size any longer, but pad
>>>> SD/MMC to the next 512 byte block (RK_BLK_SIZE) and SPI to the
>>>> next 2K boundary.
>>>> 
>>>> 
>>>> Philipp Tomsich (8):
>>>>  rockchip: mkimage: rkspi: include the header sector in the SPI size
>>>>    calculation
>>>>  rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI
>>>>    images
>>>>  rockchip: mkimage: Update comments for header size
>>>>  rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize
>>>>  rockchip: mkimage: clarify header0 initialisation
>>>>  rockchip: mkimage: play nice with dumpimage
>>>>  rockchip: mkimage: remove placeholder functions from rkimage
>>>>  rockchip: mkimage: add support for verify_header/print_header
>>>> 
>>>> tools/rkcommon.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>>> tools/rkcommon.h |  29 ++++++++-
>>>> tools/rkimage.c  |  21 +-----
>>>> tools/rksd.c     |  47 +++++---------
>>>> tools/rkspi.c    |  62 +++++++++---------
>>>> 5 files changed, 255 insertions(+), 99 deletions(-)
>>>> 
>>> 
>>> 
>> 
>> 
>> 
> 
> 

[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support

[Heiko Stuebner]

Am Freitag, 19. Mai 2017, 20:44:07 CEST schrieb Dr. Philipp Tomsich:
> Heiko,
> 
> thanks for the insight into the BROM.
> I’ll respin this with part of the change reverted and have Kever test.

The patch is already in Simon's next branch [0],
so a fixup might be better :-)


Heiko

[0] http://git.denx.de/?p=u-boot/u-boot-rockchip.git;a=commit;h=8c38deeabfda64ed24c867c4657cb7406375d27e

> 
> Regards,
> Philipp.
> 
> > On 19 May 2017, at 20:39, Heiko Stuebner <heiko@sntech.de> wrote:
> > 
> > Hi Philipp,
> > 
> > Am Mittwoch, 17. Mai 2017, 12:12:51 CEST schrieb Dr. Philipp Tomsich:
> >> What are the requirements for BACK_TO_BROM?
> >> All I can see about how BACK_TO_BROM works is that it needs to save the register
> >> context on the stack for returning to the ROM, but that seems to be only half the story.
> >> 
> >> Assuming that the header0 structure plays into this, the only significant change there
> >> is that I don’t set the 'hdr->init_boot_size’ to the maximum SPL size any longer...
> > 
> > Which is most likely the problem. back_to_bootrom-images are concatenated
> > with the spl in front (init_size) and when returned to the bootrom it
> > reads the rest up to init_boot_size into the sdram.
> > 
> > So ideally we would return that line back to RK_MAX_BOOT_SIZE (512KB).
> > Somewhat safe value and boards not using back_to_bootrom, as this value
> > really only affects that second stage and not the actual spl loading.
> > 
> > I'm sadly away from my boardfarm this and next week, so testing bootloader
> > on my rk3188 board can only happend after that, but I'm somewhat
> > confident that this would solve the problem. Maybe Kever can test that
> > meanwhile.
> > 
> > 
> > Heiko
> > 
> > 
> >> 
> >> Regards,
> >> Philipp.
> >> 
> >>> On 17 May 2017, at 11:50, Kever Yang <kever.yang@rock-chips.com> wrote:
> >>> 
> >>> Hi Philipp,
> >>> 
> >>> This patch makes all the Rockchip SoCs with BACK_TO_BROM enabled can not work,
> >>> 
> >>> does the size correct for the SPL correct?
> >>> 
> >>> Thanks,
> >>> - Kever
> >>> On 04/17/2017 11:47 PM, Philipp Tomsich wrote:
> >>>> We support booting both from SD/MMC images and SPI images on the
> >>>> RK3399-Q7 for different use-cases (e.g. external boot in development
> >>>> from the SD card, internal boot from MMC or SPI depending on whether
> >>>> the SPI flash is populated on any given configuration option).
> >>>> 
> >>>> In getting the SPI image support ready for production, we found a
> >>>> few areas that warranted improvements:
> >>>> - we had broken SPI bootstrap earlier in the changes introducting
> >>>>  boot0-style images for the RK3399 (this needed fixing)
> >>>> - in fixing the broken SPI padding calculation, it became apparent
> >>>>  that it's best to refactor and document things before we make
> >>>>  the same mistake again in the future
> >>>> - with both SD/MMC and SPI images being used for various purposes
> >>>>  by various people, the wrong image style was inadvertendly used
> >>>>  in some tests... so we support for 'dumpimage' (i.e. verify_header
> >>>>  and print_header) had to be added to quickly check the image
> >>>>  type being handled
> >>>> 
> >>>> Note that with the refactored calculation of the image-size, we
> >>>> don't pad the image to the maximum SPL size any longer, but pad
> >>>> SD/MMC to the next 512 byte block (RK_BLK_SIZE) and SPI to the
> >>>> next 2K boundary.
> >>>> 
> >>>> 
> >>>> Philipp Tomsich (8):
> >>>>  rockchip: mkimage: rkspi: include the header sector in the SPI size
> >>>>    calculation
> >>>>  rockchip: mkimage: rewrite padding calculation for SD/MMC and SPI
> >>>>    images
> >>>>  rockchip: mkimage: Update comments for header size
> >>>>  rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize
> >>>>  rockchip: mkimage: clarify header0 initialisation
> >>>>  rockchip: mkimage: play nice with dumpimage
> >>>>  rockchip: mkimage: remove placeholder functions from rkimage
> >>>>  rockchip: mkimage: add support for verify_header/print_header
> >>>> 
> >>>> tools/rkcommon.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >>>> tools/rkcommon.h |  29 ++++++++-
> >>>> tools/rkimage.c  |  21 +-----
> >>>> tools/rksd.c     |  47 +++++---------
> >>>> tools/rkspi.c    |  62 +++++++++---------
> >>>> 5 files changed, 255 insertions(+), 99 deletions(-)
> >>>> 
> >>> 
> >>> 
> >> 
> >> 
> >> 
> > 
> > 
> 
> 
> 

[U-Boot] [PATCH v1 5/8] rockchip: mkimage: clarify header0 initialisation

[Andy Yan]

Philipp, Simon:

2017-04-17 23:48 GMT+08:00 Philipp Tomsich <
philipp.tomsich@theobroma-systems.com>:

> This change set adds documentation to the header0 initialisation and
> improves readability for the calculations of various offsets/lengths.
>
> As the U-Boot SPL stage doesn't use any payload beyond what is covered
> by init_size, we no longer add RK_MAX_BOOT_SIZE to init_boot_size.
>


    I thinks this is one case that break BACK_TO_BROM  function. The
bootrom code will reads init_boot_size to get the size of the second level
loader(such as uboot) to load from storage to sdram when the BACK_TO_BROM
function enabled. This patch set init_boot_size to init_size, so the
bootrom will think that there is no second level bootloader, so it won't
load it and jump to it.

>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  tools/rkcommon.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index cfd40ac..ed29ef9 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -13,6 +13,8 @@
>  #include "mkimage.h"
>  #include "rkcommon.h"
>
> +#define DIV_ROUND_UP(n, d)     (((n) + (d) - 1) / (d))
> +
>  enum {
>         RK_SIGNATURE            = 0x0ff0aa55,
>  };
> @@ -159,9 +161,21 @@ static void rkcommon_set_header0(void *buf, uint
> file_size,
>         hdr->disable_rc4 = !rkcommon_need_rc4_spl(params);
>         hdr->init_offset = RK_INIT_OFFSET;
>
> -       hdr->init_size = (file_size + RK_BLK_SIZE - 1) / RK_BLK_SIZE;
> -       hdr->init_size = (hdr->init_size + 3) & ~3;
> -       hdr->init_boot_size = hdr->init_size + RK_MAX_BOOT_SIZE /
> RK_BLK_SIZE;
> +       hdr->init_size = DIV_ROUND_UP(file_size, RK_BLK_SIZE);
> +       /*
> +        * The init_size has to be a multiple of 4 blocks (i.e. of 2K)
> +        * or the BootROM will not boot the image.
> +        *
> +        * Note: To verify that this is not a legacy constraint, we
> +        *       rechecked this against the RK3399 BootROM.
> +        */
> +       hdr->init_size = ROUND(hdr->init_size, 4);
> +       /*
> +        * The images we create do not contain the stage following the SPL
> as
> +        * part of the SPL image, so the init_boot_size (which might have
> been
> +        * read by Rockchip's miniloder) should be the same as the
> init_size.
> +        */
> +       hdr->init_boot_size = hdr->init_size;
>
>         rc4_encode(buf, RK_BLK_SIZE, rc4_key);
>  }
> --
> 1.9.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

[U-Boot] [PATCH v1 4/8] rockchip: mkimage: rksd: pad SD/MMC images to a full blocksize

[Andy Yan]

Hi Philipp, Simon:

2017-04-17 23:48 GMT+08:00 Philipp Tomsich <
philipp.tomsich@theobroma-systems.com>:

> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
>  tools/rksd.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/rksd.c b/tools/rksd.c
> index 6dafedf..8627b6d 100644
> --- a/tools/rksd.c
> +++ b/tools/rksd.c
> @@ -62,8 +62,11 @@ static int rksd_check_image_type(uint8_t type)
>  static int rksd_vrec_header(struct image_tool_params *params,
>                             struct image_type_params *tparams)
>  {
> -       /* We don't add any additional padding after the end of the image
> */
> -       return rkcommon_vrec_header(params, tparams, 1);
> +       /*
> +        * Pad to the RK_BLK_SIZE (512 bytes) to be consistent with
> init_size
> +        * being encoded in RK_BLK_SIZE units in header0 (see rkcommon.c).
> +        */
> +       return rkcommon_vrec_header(params, tparams, RK_BLK_SIZE);
>

     This is another case that breaks BACK_TO_BROM function, as you
documented in [1]:
     The init_size has to be a multiple of 4 blocks (i.e. of 2K)
or the BootROM will not boot the image. So you need to pad the spl to 2kb
aligned.

[1]https://www.mail-archive.com/u-boot at lists.denx.de/msg245573.html






> +        * .
>
>
>  /*
> --
> 1.9.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>