[U-Boot] [PATCH v1 0/8] rockchip: mkimage: refactor rksd/rkspi padding calculation and add dumpimage support
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(-)
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;
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
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!
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@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@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; }
/*
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
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!
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;
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
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!
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); }
/*
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
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!
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 withinit_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@lists.denx.de/msg245573.html
* ./*
1.9.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
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); }
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
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!
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 SPLas
* part of the SPL image, so the init_boot_size (which might havebeen
* read by Rockchip's miniloder) should be the same as theinit_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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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)
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
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
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
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!
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
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
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(-)
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(-)
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(-)
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(-)
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=8c38deeabfda64ed...
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(-)
participants (6)
-
Andy Yan -
Dr. Philipp Tomsich -
Heiko Stuebner -
Kever Yang -
Philipp Tomsich
-
Simon Glass