On 1/12/22 15:16, Pali Rohár wrote:

On Wednesday 12 January 2022 14:53:23 Stefan Roese wrote:

...

On 1/12/22 12:34, Pali Rohár wrote:

...

On Wednesday 12 January 2022 12:06:22 Stefan Roese wrote:

...

Hi Pali,

On 1/12/22 11:55, Stefan Roese wrote:

...

On 1/12/22 11:41, Pali Rohár wrote:

...

On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote: > Hi Pali, > > while testing with this patchset (amongst others), I get this error > while building for "theadorable_debug": > > $ make theadorable_debug_defconfig > $ make -s -j20 > Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin > with 0 args. > Address must be 4-byte aligned and in range 0x40000028-0x40000424 > .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1 > make: *** Deleting file 'u-boot-spl.kwb' > > Could you please take a look on whats going wrong here? Do I need to > change CONFIG_SPL_TEXT_BASE now? And if yes, why?

Hello!

If everything is working fine without this patch series then address must not be changed.

Yes, everything works just fine without out this series.

...

Now I realized that some boards have text base address 0x40004030 and some have 0x40000030. When I was looking it during writing this patch series, I have not spotted that there is different digit "4" in the middle... And therefore I was in impression that every 32-bit Armada has base address 0x40000000 (increased by magic constant 0x30 which is explained in one of the patches).

I see.

...

So now I need to figure out, why some boards have base address 0x40004000 and some have 0x40000000. It it somewhere documented this magic address? Or do you know source of this address for your board?

This is so loooong ago that I worked on this. I can only guess, that the are up to offset 0x4000 was reserved by/for the BootROM.

This info is still in some of the config headers:

/*

• Memory layout while starting into the bin_hdr via the
• BootROM:
• 0x4000.4000 - 0x4003.4000 headers space (192KiB)
• 0x4000.4030 bin_hdr start address
• 0x4003.4000 - 0x4004.7c00 BootROM memory allocations (15KiB)
• 0x4007.fffc BootROM stack top
• The address space between 0x4007.fffc and 0x400f.fff is not locked in
• L2 cache thus cannot be used.

*/

HTP.

Thanks, Stefan

And now I found this information in old Marvell U-Boot fork from 2013: https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/blob/u-boot-2013...

#if defined(MV_TEST_PLATFORM) #define RAM_TOP 0x81004000 /* Use PEX memory - (16KB for MMU table) */ #elif defined(MV88F6710) || defined(MV88F78X60) #define RAM_TOP 0x40004000 /* L2 cache 512KB - (16KB for MMU table) */ #elif defined(MV88F66XX) || defined(MV88F68XX) || defined(MV88F672X) || defined(MV88F69XX) #define RAM_TOP 0x40000000 /* L2 cache 512KB */ #else #define RAM_TOP 0x40004000 /* L2 cache 512KB */ #endif

IIRC this is chip conversion table:

88F6710 = A370 MV78X60 = AXP 88F66xx = Avanta 88F672x = A375 88F68xx = A38x 88F69xx = A39x

So it looks like that only AXP and A370 use address 0x40004000, other platforms use 0x40000000. AXP and A370 are the last SoCs which use Marvell custom CPU cores Sheeva, all later have ARM A9 cores. Also in functional specifications for AXP and A370 is written that executable code needs to be aligned to 128-bit boundary and in later SoCs specs there is no written restriction, like this. On A385 I tested that this alignment is not really required. So for me it looks like that this 16kB reservation is needed for Marvell custom CPU cores only and is some CPU core specific.

Makes perfect sense, yes.

...

The only suspicious thing is that in configs/db-88f6720_defconfig is defined CONFIG_SPL_TEXT_BASE=0x40004030 and this is A375 (not A370!). But now I found your commit 606576d54b673 ("arm: mvebu: Add basic support for Armada 375 eval board db-88f6720") where you introduced this #define CONFIG_SPL_TEXT_BASE 0x40004030 and wrote "the SPL U-Boot is not fully functional.". So with this base address SPL would never work. I know that Serdes+ddr3 init code is missing, so no SPL for this platform.

AFAIR, this port was done not that thoroughly, which would explain this mismatch you describe above. And could perhaps be fixed by changing this SPL_TEXT_BASE - if someone would like to invest some more time here.

...

So how to solve the problem that kwbimage needs to know if is building image for platform with 16kB reserved for MMU table?

Should I add a new kwbimage command which specifies this information, like building image for Marvell CPU cores (Sheeva)? Or do you have other idea? With this information I can adjust code to enable 128-bit boundary restrictions only for these CPUs...

The first idea is to change this error to a warning, with some comment that this might work on these specific AXP and A370 platforms. Another idea is to add a 2nd valid address area.

Thanks, Stefan

I do not think that changing error to warning would help here. With these changes kwbimage really try to sets load address to specified one. And if kwbimage thinks that base address is on different location then it would generate something unbootable.

Makes sense, okay.

Now I played with it... could you try to test following diff? It propagates information about CPU into kwbimage and then it do different checks and use different base address based on CPU.

Looks good, please see the boot log here:

$ ./tools/kwboot -B 115200 -b u-boot-spl.kwb -t /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0 kwboot version 2022.01-00487-g655eb7d4bc34-dirty Patching image boot signature to UART Sending boot message. Please reboot the target...\ Waiting 2s and flushing tty Sending boot image header (91264 bytes)... 0 % [......................................................................] 9 % [......................................................................] 19 % [......................................................................] 29 % [......................................................................] 39 % [......................................................................] 49 % [......................................................................] 59 % [......................................................................] 68 % [......................................................................] 78 % [......................................................................] 88 % [......................................................................] 98 % [............. ] Done

U-Boot SPL 2022.01-00487-g655eb7d4bc34-dirty (Jan 12 2022 - 16:02:28 +0100) High speed PHY - Version: 2.1.5 (COM-PHY-V20) High speed PHY - Ended Successfully DDR3 Training Sequence - Ver 5.7.4 DDR3 Training Sequence - Ended Successfully Trying to boot from BOOTROM Returning to BootROM (return address 0xffff0aa0)...

Sending boot image data (566180 bytes)... 0 % [......................................................................] 1 % [......................................................................] 3 % [......................................................................] 4 % [......................................................................] 6 % [......................................................................] 7 % [......................................................................] 9 % [......................................................................] 11 % [......................................................................] 12 % [......................................................................] 14 % [......................................................................] 15 % [......................................................................] 17 % [......................................................................] 19 % [......................................................................] 20 % [......................................................................] 22 % [......................................................................] 23 % [......................................................................] 25 % [......................................................................] 26 % [......................................................................] 28 % [......................................................................] 30 % [......................................................................] 31 % [......................................................................] 33 % [......................................................................] 34 % [......................................................................] 36 % [......................................................................] 38 % [......................................................................] 39 % [......................................................................] 41 % [......................................................................] 42 % [......................................................................] 44 % [......................................................................] 45 % [......................................................................] 47 % [......................................................................] 49 % [......................................................................] 50 % [......................................................................] 52 % [......................................................................] 53 % [......................................................................] 55 % [......................................................................] 56 % [......................................................................] 58 % [......................................................................] 60 % [......................................................................] 61 % [......................................................................] 63 % [......................................................................] 64 % [......................................................................] 66 % [......................................................................] 68 % [......................................................................] 69 % [......................................................................] 71 % [......................................................................] 72 % [......................................................................] 74 % [......................................................................] 75 % [......................................................................] 77 % [......................................................................] 79 % [......................................................................] 80 % [......................................................................] 82 % [......................................................................] 83 % [......................................................................] 85 % [......................................................................] 87 % [......................................................................] 88 % [......................................................................] 90 % [......................................................................] 91 % [......................................................................] 93 % [......................................................................] 94 % [......................................................................] 96 % [......................................................................] 98 % [......................................................................] 99 % [.............. ] Done Finishing transfer [Type Ctrl-\ + c to quit]

U-Boot 2022.01-00487-g655eb7d4bc34-dirty (Jan 12 2022 - 16:02:28 +0100)

SoC: MV78260-B0 at 1333 MHz DRAM: 2 GiB (667 MHz, 64-bit, ECC not enabled) Core: 24 devices, 14 uclasses, devicetree: separate Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256 Bytes, erase size 256 KiB, total 16 MiB OK Model: Marvell Armada XP theadorable pcie0.0: Link up pcie2.0: Link up Net: eth0: ethernet@70000 ...

:)

Thanks, Stefan

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile index 4e15101a40cf..74478a3134e3 100644 --- a/arch/arm/mach-mvebu/Makefile +++ b/arch/arm/mach-mvebu/Makefile @@ -30,6 +30,14 @@ obj-$(CONFIG_MVEBU_EFUSE) += efuse.o

extra-y += kwbimage.cfg

+ifneq ($(CONFIG_ARMADA_370)$(CONFIG_ARMADA_XP),)

• KWB_REPLACE += CPU
• KWB_CFG_CPU = SHEEVA

+else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)

• KWB_REPLACE += CPU
• KWB_CFG_CPU = A9

+endif

• KWB_REPLACE += LOAD_ADDRESS KWB_CFG_LOAD_ADDRESS = $(CONFIG_SPL_TEXT_BASE)

diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in index 75f90766dda4..ccb09975817e 100644 --- a/arch/arm/mach-mvebu/kwbimage.cfg.in +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in @@ -5,6 +5,9 @@ # Armada 38x uses version 1 image format VERSION 1

+# Type of the CPU core +#@CPU

• # Boot Media configurations #@BOOT_FROM

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 7ccb582918a4..52005724fbd1 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -99,6 +99,7 @@ enum image_cfg_type { IMAGE_CFG_NAND_BADBLK_LOCATION, IMAGE_CFG_NAND_ECC_MODE, IMAGE_CFG_NAND_PAGESZ,

• IMAGE_CFG_CPU, IMAGE_CFG_BINARY, IMAGE_CFG_DATA, IMAGE_CFG_DATA_DELAY,

@@ -129,6 +130,7 @@ static const char * const id_strs[] = { [IMAGE_CFG_NAND_BADBLK_LOCATION] = "NAND_BADBLK_LOCATION", [IMAGE_CFG_NAND_ECC_MODE] = "NAND_ECC_MODE", [IMAGE_CFG_NAND_PAGESZ] = "NAND_PAGE_SIZE",

• [IMAGE_CFG_CPU] = "CPU", [IMAGE_CFG_BINARY] = "BINARY", [IMAGE_CFG_DATA] = "DATA", [IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",

@@ -152,6 +154,7 @@ struct image_cfg_element { enum image_cfg_type type; union { unsigned int version;

• unsigned int cpu_sheeva;
  

  unsigned int bootfrom; struct { const char *file;

@@ -292,6 +295,17 @@ static int image_get_bootfrom(void) return e->bootfrom; }

+static int image_is_cpu_sheeva(void) +{

• struct image_cfg_element *e;
• e = image_find_option(IMAGE_CFG_CPU);
• if (!e)

• return 0;
  

• return e->cpu_sheeva;

+}

• /*
  • Compute a 8-bit checksum of a memory area. This algorithm follows
  • the requirements of the Marvell SoC BootROM specifications.

@@ -1031,6 +1045,7 @@ static size_t image_headersz_v1(int *hasext) struct image_cfg_element *e; unsigned int count; size_t headersz;

• int cpu_sheeva; struct stat s; int cfgi; int ret;

@@ -1047,6 +1062,8 @@ static size_t image_headersz_v1(int *hasext) *hasext = 1; }

• cpu_sheeva = image_is_cpu_sheeva();
• count = 0; for (cfgi = 0; cfgi < cfgn; cfgi++) { e = &image_cfg[cfgi];

@@ -1089,15 +1106,25 @@ static size_t image_headersz_v1(int *hasext) /* * BootROM loads kwbimage header (in which the * executable code is also stored) to address

• 	 * 0x40000000. Thus there is restriction for the
  

• 	 * load address of the N-th BINARY image.
  

• 	 * 0x40004000 or 0x40000000. Thus there is
  

• 	 * restriction for the load address of the N-th
  

• 	 * BINARY image.
   */
  

• 	unsigned int low_addr, high_addr;
  

• 	unsigned int base_addr, low_addr, high_addr;
  
  

• 	low_addr = 0x40000000 + headersz;
  

• 	base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
  

• 	low_addr = base_addr + headersz;
  high_addr = low_addr +
  	    (BINARY_MAX_ARGS - e->binary.nargs) * sizeof(uint32_t);
  
  

• 	if (cpu_sheeva && e->binary.loadaddr % 16) {
  

• 		fprintf(stderr,
  

• 			"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
  

• 			"Address for CPU SHEEVA must be 16-byte aligned.\n",
  

• 			e->binary.loadaddr, e->binary.file, e->binary.nargs);
  

• 		return 0;
  

• 	}
  

• if (e->binary.loadaddr % 4 || e->binary.loadaddr < low_addr ||
      e->binary.loadaddr > high_addr) {
  	fprintf(stderr,
  

@@ -1107,7 +1134,7 @@ static size_t image_headersz_v1(int *hasext) e->binary.nargs, low_addr, high_addr); return 0; }

• 	headersz = e->binary.loadaddr - 0x40000000;
  

• 	headersz = e->binary.loadaddr - base_addr;
  

  } else { headersz = ALIGN(headersz, 16); }

@@ -1128,10 +1155,12 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext, struct main_hdr_v1 *main_hdr) { struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;

• uint32_t base_addr; uint32_t add_args; uint32_t offset; uint32_t *args; size_t binhdrsz;
• int cpu_sheeva; struct stat s; int argi; FILE *bin;

@@ -1163,18 +1192,22 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext, *cur += (binarye->binary.nargs + 1) * sizeof(uint32_t);

/*

• * ARM executable code inside the BIN header on some mvebu platforms
  

• * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
  

• * ARM executable code inside the BIN header on platforms with Sheeva
  

• * CPU (A370 and AXP) must always be aligned with the 128-bit boundary.
  

  • In the case when this code is not position independent (e.g. ARM
  • SPL), it must be placed at fixed load and execute address.
  • This requirement can be met by inserting dummy arguments into
  • BIN header, if needed.
  */
• cpu_sheeva = image_is_cpu_sheeva();
• base_addr = cpu_sheeva ? 0x40004000 : 0x40000000; offset = *cur - (uint8_t *)main_hdr; if (binarye->binary.loadaddr)

• add_args = (binarye->binary.loadaddr - 0x40000000 - offset) / sizeof(uint32_t);
  

• else

• add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
  

• else if (cpu_sheeva) add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
• else

• add_args = 0;
  

  if (add_args) { *(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args); *cur += add_args * sizeof(uint32_t);

@@ -1553,6 +1586,18 @@ static int image_create_config_parse_oneline(char *line, case IMAGE_CFG_VERSION: el->version = atoi(value1); break;

• case IMAGE_CFG_CPU:

• if (strcmp(value1, "FEROCEON") == 0)
  

• 	el->cpu_sheeva = 0;
  

• else if (strcmp(value1, "SHEEVA") == 0)
  

• 	el->cpu_sheeva = 1;
  

• else if (strcmp(value1, "A9") == 0)
  

• 	el->cpu_sheeva = 0;
  

• else {
  

• 	fprintf(stderr, "Invalid CPU %s\n", value1);
  

• 	return -1;
  

• }
  

• break;
  

  case IMAGE_CFG_BOOT_FROM: ret = image_boot_mode_id(value1);

@@ -1862,7 +1907,7 @@ static void kwbimage_print_header(const void *ptr) printf("BIN Img Size: "); genimg_print_size(opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0]);

• 	printf("BIN Img Addr: %08x\n", 0x40000000 +
  

• 	printf("BIN Img Offs: %08x\n",
  	(unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
  	8 + 4 * ohdr->data[0]);
  

  }

@@ -2031,6 +2076,11 @@ static int kwbimage_generate(struct image_tool_params *params, free(image_cfg); exit(EXIT_FAILURE); }

• if (alloc_len > 192*1024) {
  

• 	fprintf(stderr, "Header is too big (%u bytes), maximal kwbimage header size is %u bytes\n", alloc_len, 192*1024);
  

• 	free(image_cfg);
  

• 	exit(EXIT_FAILURE);
  

• }
  

  break;

  default:

@@ -2079,6 +2129,7 @@ static int kwbimage_generate_config(void *ptr, struct image_tool_params *params) struct ext_hdr_v0_reg *regdata; struct ext_hdr_v0 *ehdr0; struct opt_hdr_v1 *ohdr;

• unsigned offset; int cur_idx; int version; FILE *f;

@@ -2145,9 +2196,9 @@ static int kwbimage_generate_config(void *ptr, struct image_tool_params *params) fprintf(f, "BINARY binary%d.bin", cur_idx); for (i = 0; i < ohdr->data[0]; i++) fprintf(f, " 0x%x", le32_to_cpu(((uint32_t *)ohdr->data)[i + 1]));

• 	fprintf(f, " LOAD_ADDRESS 0x%08x\n",
  

• 		0x40000000 + (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
  

• 		8 + 4 * ohdr->data[0]);
  

• 	offset = (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) + 8 + 4 * ohdr->data[0];
  

• 	fprintf(f, " LOAD_ADDRESS 0x%08x\n", 0x40000000 + offset);
  

• 	fprintf(f, " # for CPU SHEEVA: LOAD_ADDRESS 0x%08x\n", 0x40004000 + offset);
  cur_idx++;
  

  } else if (ohdr->headertype == OPT_HDR_V1_REGISTER_TYPE) { regset_hdr = (struct register_set_hdr_v1 *)ohdr;

On 1/12/22 18:20, Pali Rohár wrote:

Mark all local functions as static.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 224d8156be19..b0598cab4bc7 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -199,7 +199,7 @@ static const char *image_boot_mode_name(unsigned int id) return NULL; }

-int image_boot_mode_id(const char *boot_mode_name) +static int image_boot_mode_id(const char *boot_mode_name) { int i;

@@ -210,7 +210,7 @@ int image_boot_mode_id(const char *boot_mode_name) return -1; }

-int image_nand_ecc_mode_id(const char *nand_ecc_mode_name) +static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name) { int i;

@@ -602,7 +602,8 @@ static int kwb_export_pubkey(RSA *key, struct pubkey_der_v1 *dst, FILE *hashf, return 0; }

-int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig, char *signame) +static int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig,

•     char *signame)
  

  { EVP_PKEY *evp_key; EVP_MD_CTX *ctx;

@@ -662,8 +663,8 @@ err_key: return ret; }

-int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,

•       char *signame)
  

+static int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,

•       char *signame)
  

  { EVP_PKEY *evp_key; EVP_MD_CTX *ctx;

@@ -722,8 +723,8 @@ err_key: return ret; }

-int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,

• 	char *signame)
  

+static int kwb_sign_and_verify(RSA *key, void *data, int datasz,

• 	       struct sig_v1 *sig, char *signame)
  

  { if (kwb_sign(key, data, datasz, sig, signame) < 0) return -1;

@@ -735,7 +736,7 @@ int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig, }

-int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr) +static int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr) { struct hash_v1 kak_pub_hash; struct image_cfg_element *e; @@ -1051,9 +1052,9 @@ static size_t image_headersz_v1(int *hasext) return image_headersz_align(headersz, image_get_bootfrom()); }

-int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,

• 	 struct image_cfg_element *binarye,
  

• 	 struct main_hdr_v1 *main_hdr)
  

+static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,

• 		struct image_cfg_element *binarye,
  

• 		struct main_hdr_v1 *main_hdr)
  

  { struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur; uint32_t add_args;

@@ -1135,7 +1136,7 @@ err_close: return -1; }

-int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr) +static int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr) { FILE *hashf; int res; @@ -1154,8 +1155,8 @@ int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr) return res < 0 ? 1 : 0; }

-int kwb_sign_csk_with_kak(struct image_tool_params *params,

• 	  struct secure_hdr_v1 *secure_hdr, RSA *csk)
  

+static int kwb_sign_csk_with_kak(struct image_tool_params *params,

• 		 struct secure_hdr_v1 *secure_hdr, RSA *csk)
  

  { RSA *kak = NULL; RSA *kak_pub = NULL;

@@ -1196,9 +1197,9 @@ int kwb_sign_csk_with_kak(struct image_tool_params *params, return 0; }

-int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,

• 	 int payloadsz, size_t headersz, uint8_t *image,
  

• 	 struct secure_hdr_v1 *secure_hdr)
  

+static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,

• 		int payloadsz, size_t headersz, uint8_t *image,
  

• 		struct secure_hdr_v1 *secure_hdr)
  

  { struct image_cfg_element *e_jtagdelay; struct image_cfg_element *e_boxid;

@@ -1415,7 +1416,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, return image; }

-int recognize_keyword(char *keyword) +static int recognize_keyword(char *keyword) { int kw_id;

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Deduplicate code that finishes OPT_HDR_V1_REGISTER_TYPE header by extracing it into separate function.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index b0598cab4bc7..552fef9e9aeb 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -1249,6 +1249,22 @@ static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr, return 0; }

+static void finish_register_set_header_v1(uint8_t **cur, uint8_t **next_ext,

• 			  struct register_set_hdr_v1 *register_set_hdr,
  

• 			  int *datai, uint8_t delay)
  

+{

• int size = sizeof(struct register_set_hdr_v1) + 8 * (*datai) + 4;
• register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
• register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
• register_set_hdr->headersz_msb = size >> 16;
• register_set_hdr->data[*datai].last_entry.delay = delay;
• *cur += size;
• **next_ext = 1;
• *next_ext = &register_set_hdr->data[*datai].last_entry.next;
• *datai = 0;

+}

• static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, uint8_t *ptr, int payloadsz) {

@@ -1261,7 +1277,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, uint8_t *image, *cur; int hasext = 0; uint8_t *next_ext = NULL;

• int cfgi, datai, size;

• int cfgi, datai;

  /*

  • Calculate the size of the header and the size of the

@@ -1359,15 +1375,8 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, e->type != IMAGE_CFG_DATA_DELAY) continue; if (e->type == IMAGE_CFG_DATA_DELAY) {

• 	size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
  

• 	register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
  

• 	register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
  

• 	register_set_hdr->headersz_msb = size >> 16;
  

• 	register_set_hdr->data[datai].last_entry.delay = e->regdata_delay;
  

• 	cur += size;
  

• 	*next_ext = 1;
  

• 	next_ext = &register_set_hdr->data[datai].last_entry.next;
  

• 	datai = 0;
  

• 	finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
  

• 				      &datai, e->regdata_delay);
  continue;
  

  } register_set_hdr->data[datai].entry.address =

@@ -1377,15 +1386,9 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, datai++; } if (datai != 0) {

• size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
  

• register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
  

• register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
  

• register_set_hdr->headersz_msb = size >> 16;
  

• /* Set delay to the smallest possible value 1ms. */
  

• register_set_hdr->data[datai].last_entry.delay = 1;
  

• cur += size;
  

• *next_ext = 1;
  

• next_ext = &register_set_hdr->data[datai].last_entry.next;
  

• /* Set delay to the smallest possible value. */
  

• finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
  

• 			      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
  

  }

  for (cfgi = 0; cfgi < cfgn; cfgi++) {

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Register set header consists of sequence of DATA commands followed by exactly one DATA_DELAY command. Thus if we are generating image with multiple DATA_DELAY commands, we need to create more register set headers.

Fix calculation of image size with multiple DATA_DELAY commands and correctly set pointer to struct register_set_hdr_v1 when initializing new register set header.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 552fef9e9aeb..6ee3d0aaa86c 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -993,7 +993,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,

static size_t image_headersz_v1(int *hasext) {

• struct image_cfg_element *binarye;

• struct image_cfg_element *binarye, *e; unsigned int count; size_t headersz; int cfgi;

@@ -1010,7 +1010,18 @@ static size_t image_headersz_v1(int *hasext) *hasext = 1; }

• count = image_count_options(IMAGE_CFG_DATA);

• count = 0;
• for (cfgi = 0; cfgi < cfgn; cfgi++) {

• e = &image_cfg[cfgi];
  

• if (e->type == IMAGE_CFG_DATA)
  

• 	count++;
  

• if (e->type == IMAGE_CFG_DATA_DELAY) {
  

• 	headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
  

• 	count = 0;
  

• }
  

• } if (count > 0) headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;

@@ -1368,12 +1379,13 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, }

datai = 0;

• register_set_hdr = (struct register_set_hdr_v1 *)cur; for (cfgi = 0; cfgi < cfgn; cfgi++) { e = &image_cfg[cfgi]; if (e->type != IMAGE_CFG_DATA && e->type != IMAGE_CFG_DATA_DELAY) continue;

• if (datai == 0)
  

• 	register_set_hdr = (struct register_set_hdr_v1 *)cur;
  

  if (e->type == IMAGE_CFG_DATA_DELAY) { finish_register_set_header_v1(&cur, &next_ext, register_set_hdr, &datai, e->regdata_delay);

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Preserve the order of BINARY, DATA and DATA_DELAY commands as they appear in the input file. They may depend on each other.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 58 +++++++++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 6ee3d0aaa86c..17d3c3cf223c 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -1017,7 +1017,8 @@ static size_t image_headersz_v1(int *hasext) if (e->type == IMAGE_CFG_DATA) count++;

• if (e->type == IMAGE_CFG_DATA_DELAY) {
  

• if (e->type == IMAGE_CFG_DATA_DELAY ||
  

•     (e->type == IMAGE_CFG_BINARY && count > 0)) {
  headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
  count = 0;
  

  }

@@ -1289,6 +1290,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, int hasext = 0; uint8_t *next_ext = NULL; int cfgi, datai;

• uint8_t delay;

  /*

  • Calculate the size of the header and the size of the

@@ -1382,34 +1384,50 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params, for (cfgi = 0; cfgi < cfgn; cfgi++) { e = &image_cfg[cfgi]; if (e->type != IMAGE_CFG_DATA &&

•     e->type != IMAGE_CFG_DATA_DELAY)
  

•     e->type != IMAGE_CFG_DATA_DELAY &&
  

•     e->type != IMAGE_CFG_BINARY)
  continue;
  

• if (datai == 0) register_set_hdr = (struct register_set_hdr_v1 *)cur;

• if (e->type == IMAGE_CFG_DATA_DELAY) {
  

• /* If delay is not specified, use the smallest possible value. */
  

• if (e->type == IMAGE_CFG_DATA_DELAY)
  

• 	delay = e->regdata_delay;
  

• else
  

• 	delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
  

• /*
  

•  * DATA_DELAY command is the last entry in the register set
  

•  * header and BINARY command inserts new binary header.
  

•  * Therefore BINARY command requires to finish register set
  

•  * header if some DATA command was specified. And DATA_DELAY
  

•  * command automatically finish register set header even when
  

•  * there was no DATA command.
  

•  */
  

• if (e->type == IMAGE_CFG_DATA_DELAY ||
  

•     (e->type == IMAGE_CFG_BINARY && datai != 0))
  finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
  

• 				      &datai, e->regdata_delay);
  

• 	continue;
  

• 				      &datai, delay);
  

• if (e->type == IMAGE_CFG_DATA) {
  

• 	register_set_hdr->data[datai].entry.address =
  

• 		cpu_to_le32(e->regdata.raddr);
  

• 	register_set_hdr->data[datai].entry.value =
  

• 		cpu_to_le32(e->regdata.rdata);
  

• 	datai++;
  

• }
  

• if (e->type == IMAGE_CFG_BINARY) {
  

• 	if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
  

• 		return NULL;
  

  }

• register_set_hdr->data[datai].entry.address =
  

• 	cpu_to_le32(e->regdata.raddr);
  

• register_set_hdr->data[datai].entry.value =
  

• 	cpu_to_le32(e->regdata.rdata);
  

• datai++;
  

  } if (datai != 0) { /* Set delay to the smallest possible value. */

• delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
  

  finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,

• 			      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
  

• }
• for (cfgi = 0; cfgi < cfgn; cfgi++) {

• e = &image_cfg[cfgi];
  

• if (e->type != IMAGE_CFG_BINARY)
  

• 	continue;
  

• if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
  

• 	return NULL;
  

• 			      &datai, delay);
  

  }

  if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz + headersz,

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Usage of $(call cmd,...) is standard way to call other commands which generate things.

It also has the advantage of printing build information in the form KWBCFG arch/arm/mach-mvebu/kwbimage.cfg if verbosity is disabled, and printing the build command otherwise.

Note that the '#' character needs to be escaped in Makefile when used as value for make variable assignment.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

arch/arm/mach-mvebu/Makefile | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile index 7e9c206ed6b8..acbaa6449d3d 100644 --- a/arch/arm/mach-mvebu/Makefile +++ b/arch/arm/mach-mvebu/Makefile @@ -58,10 +58,13 @@ KWB_REPLACE += SEC_FUSE_DUMP KWB_CFG_SEC_FUSE_DUMP = a38x endif

+quiet_cmd_kwbcfg = KWBCFG $@ +cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \

• <$< >$(dir $@)$(@F)
• $(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \ include/config/auto.conf

• $(Q)sed -ne '$(foreach V,$(KWB_REPLACE),s/^#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
• <$< >$(dir $@)$(@F)

• $(call cmd,kwbcfg)

  endif # CONFIG_SPL_BUILD obj-y += gpio.o

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

For other changes it is required to know if CPU core is Sheeva or not. Therefore add a new command CPU for specifying CPU.

Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 17d3c3cf223c..44843be2c13a 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -99,6 +99,7 @@ enum image_cfg_type { IMAGE_CFG_NAND_BADBLK_LOCATION, IMAGE_CFG_NAND_ECC_MODE, IMAGE_CFG_NAND_PAGESZ,

• IMAGE_CFG_CPU, IMAGE_CFG_BINARY, IMAGE_CFG_DATA, IMAGE_CFG_DATA_DELAY,

@@ -129,6 +130,7 @@ static const char * const id_strs[] = { [IMAGE_CFG_NAND_BADBLK_LOCATION] = "NAND_BADBLK_LOCATION", [IMAGE_CFG_NAND_ECC_MODE] = "NAND_ECC_MODE", [IMAGE_CFG_NAND_PAGESZ] = "NAND_PAGE_SIZE",

• [IMAGE_CFG_CPU] = "CPU", [IMAGE_CFG_BINARY] = "BINARY", [IMAGE_CFG_DATA] = "DATA", [IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",

@@ -152,6 +154,7 @@ struct image_cfg_element { enum image_cfg_type type; union { unsigned int version;

• unsigned int cpu_sheeva;
  

  unsigned int bootfrom; struct { const char *file;

@@ -280,6 +283,17 @@ static int image_get_bootfrom(void) return e->bootfrom; }

+static int image_is_cpu_sheeva(void) +{

• struct image_cfg_element *e;
• e = image_find_option(IMAGE_CFG_CPU);
• if (!e)

• return 0;
  

• return e->cpu_sheeva;

+}

• /*
  • Compute a 8-bit checksum of a memory area. This algorithm follows
  • the requirements of the Marvell SoC BootROM specifications.

@@ -1489,6 +1503,18 @@ static int image_create_config_parse_oneline(char *line, case IMAGE_CFG_VERSION: el->version = atoi(value1); break;

• case IMAGE_CFG_CPU:

• if (strcmp(value1, "FEROCEON") == 0)
  

• 	el->cpu_sheeva = 0;
  

• else if (strcmp(value1, "SHEEVA") == 0)
  

• 	el->cpu_sheeva = 1;
  

• else if (strcmp(value1, "A9") == 0)
  

• 	el->cpu_sheeva = 0;
  

• else {
  

• 	fprintf(stderr, "Invalid CPU %s\n", value1);
  

• 	return -1;
  

• }
  

• break;
  

  case IMAGE_CFG_BOOT_FROM: ret = image_boot_mode_id(value1);

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

ARM executable code included in kwbimage binary header, which is not position independent, needs to be loaded and executed by BootROM at the correct fixed address.

Armada BootROMs load kwbimage header (in which the executable code is also stored) at fixed address 0x40004000 or 0x40000000 which is mapped to L2-SRAM (L2 Cache as SRAM). Address 0x40004000 is used on Armada platforms with Sheeva CPU core (A370 and AXP) where BootROM uses MMU with 0x4000 bytes for MMU translation table. Address 0x40000000 is used on all other platforms.

Thus the only way to specify load and execute address of this executable code in binary kwbimage header is by filling dummy arguments into the binary header, using the same mechanism we already have for achieving 128-bit boundary alignment on A370 and AXP SoCs.

Extend kwbimage config file parser to allow to specify load address as part of BINARY command with syntax:

 BINARY path_to_binary arg1 arg2 ... argN LOAD_ADDRESS address

If the specified load address is invalid or cannot be used, mkimage will throw fatal error and exit. This will prevent generating kwbimage with invalid load address for non-position independent binary code.

If no load address is specified, kwbimage will not fill any the dummy arguments, thus it will behave the same as before this change.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 109 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 93 insertions(+), 16 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 44843be2c13a..c0f1bdac0210 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -158,6 +158,7 @@ struct image_cfg_element { unsigned int bootfrom; struct { const char *file;

• 	unsigned int loadaddr;
  unsigned int args[BINARY_MAX_ARGS];
  unsigned int nargs;
  

  } binary;

@@ -1007,10 +1008,13 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,

static size_t image_headersz_v1(int *hasext) {

• struct image_cfg_element *binarye, *e;

• struct image_cfg_element *e; unsigned int count; size_t headersz;

• int cpu_sheeva;

• struct stat s; int cfgi;

• int ret;

  /*

  • Calculate the size of the header and the size of the

@@ -1024,6 +1028,8 @@ static size_t image_headersz_v1(int *hasext) *hasext = 1; }

• cpu_sheeva = image_is_cpu_sheeva();
• count = 0; for (cfgi = 0; cfgi < cfgn; cfgi++) { e = &image_cfg[cfgi];

@@ -1036,19 +1042,11 @@ static size_t image_headersz_v1(int *hasext) headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4; count = 0; }

• }

• if (count > 0)

• headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
  
  

• for (cfgi = 0; cfgi < cfgn; cfgi++) {

• int ret;
  

• struct stat s;
  

• binarye = &image_cfg[cfgi];
  

• if (binarye->type != IMAGE_CFG_BINARY)
  

• if (e->type != IMAGE_CFG_BINARY)
  continue;
  
  

• ret = stat(binarye->binary.file, &s);
  

• ret = stat(e->binary.file, &s);
  

  if (ret < 0) { char cwd[PATH_MAX]; char *dir = cwd;

@@ -1063,18 +1061,58 @@ static size_t image_headersz_v1(int *hasext) "Didn't find the file '%s' in '%s' which is mandatory to generate the image\n" "This file generally contains the DDR3 training code, and should be extracted from an existing bootable\n" "image for your board. Use 'dumpimage -T kwbimage -p 0' to extract it from an existing image.\n",

• 		binarye->binary.file, dir);
  

• 		e->binary.file, dir);
  return 0;
  

  }

  headersz += sizeof(struct opt_hdr_v1) + sizeof(uint32_t) +

• 	(binarye->binary.nargs) * sizeof(uint32_t);
  

• headersz = ALIGN(headersz, 16);
  

• 	(e->binary.nargs) * sizeof(uint32_t);
  

• if (e->binary.loadaddr) {
  

• 	/*
  

• 	 * BootROM loads kwbimage header (in which the
  

• 	 * executable code is also stored) to address
  

• 	 * 0x40004000 or 0x40000000. Thus there is
  

• 	 * restriction for the load address of the N-th
  

• 	 * BINARY image.
  

• 	 */
  

• 	unsigned int base_addr, low_addr, high_addr;
  

• 	base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
  

• 	low_addr = base_addr + headersz;
  

• 	high_addr = low_addr +
  

• 		    (BINARY_MAX_ARGS - e->binary.nargs) * sizeof(uint32_t);
  

• 	if (cpu_sheeva && e->binary.loadaddr % 16) {
  

• 		fprintf(stderr,
  

• 			"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
  

• 			"Address for CPU SHEEVA must be 16-byte aligned.\n",
  

• 			e->binary.loadaddr, e->binary.file, e->binary.nargs);
  

• 		return 0;
  

• 	}
  

• 	if (e->binary.loadaddr % 4 || e->binary.loadaddr < low_addr ||
  

• 	    e->binary.loadaddr > high_addr) {
  

• 		fprintf(stderr,
  

• 			"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
  

• 			"Address must be 4-byte aligned and in range 0x%08x-0x%08x.\n",
  

• 			e->binary.loadaddr, e->binary.file,
  

• 			e->binary.nargs, low_addr, high_addr);
  

• 		return 0;
  

• 	}
  

• 	headersz = e->binary.loadaddr - base_addr;
  

• } else {
  

• 	headersz = ALIGN(headersz, 16);
  

• }
  

• headersz += ALIGN(s.st_size, 4) + sizeof(uint32_t); if (hasext) *hasext = 1; }

• if (count > 0)

• headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
  

• return image_headersz_align(headersz, image_get_bootfrom()); }

@@ -1083,10 +1121,12 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext, struct main_hdr_v1 *main_hdr) { struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;

• uint32_t base_addr; uint32_t add_args; uint32_t offset; uint32_t *args; size_t binhdrsz;
• int cpu_sheeva; struct stat s; int argi; FILE *bin;

@@ -1120,11 +1160,18 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext, /* * ARM executable code inside the BIN header on some mvebu platforms * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.

• * In the case when this code is not position independent (e.g. ARM
  

• * SPL), it must be placed at fixed load and execute address.
  

  • This requirement can be met by inserting dummy arguments into
  • BIN header, if needed.
  */
• cpu_sheeva = image_is_cpu_sheeva();
• base_addr = cpu_sheeva ? 0x40004000 : 0x40000000; offset = *cur - (uint8_t *)main_hdr;

• add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);

• if (binarye->binary.loadaddr)

• add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
  

• else

• add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
  

  if (add_args) { *(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args); *cur += add_args * sizeof(uint32_t);

@@ -1548,10 +1595,40 @@ static int image_create_config_parse_oneline(char *line, el->binary.file = strdup(value1); while (1) { char *value = strtok_r(NULL, delimiters, &saveptr);

• 	char *endptr;
  
  if (!value)
  	break;
  

• 	el->binary.args[argi] = strtoul(value, NULL, 16);
  

• 	if (!strcmp(value, "LOAD_ADDRESS")) {
  

• 		value = strtok_r(NULL, delimiters, &saveptr);
  

• 		if (!value) {
  

• 			fprintf(stderr,
  

• 				"Missing address argument for BINARY LOAD_ADDRESS\n");
  

• 			return -1;
  

• 		}
  

• 		el->binary.loadaddr = strtoul(value, &endptr, 16);
  

• 		if (*endptr) {
  

• 			fprintf(stderr,
  

• 				"Invalid argument '%s' for BINARY LOAD_ADDRESS\n",
  

• 				value);
  

• 			return -1;
  

• 		}
  

• 		value = strtok_r(NULL, delimiters, &saveptr);
  

• 		if (value) {
  

• 			fprintf(stderr,
  

• 				"Unexpected argument '%s' after BINARY LOAD_ADDRESS\n",
  

• 				value);
  

• 			return -1;
  

• 		}
  

• 		break;
  

• 	}
  

• 	el->binary.args[argi] = strtoul(value, &endptr, 16);
  

• 	if (*endptr) {
  

• 		fprintf(stderr, "Invalid argument '%s' for BINARY\n", value);
  

• 		return -1;
  

• 	}
  argi++;
  if (argi >= BINARY_MAX_ARGS) {
  	fprintf(stderr,
  
  

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Function image_headersz_v1() may return zero on fatal errors. In this case the function already printed an error message.

Check the return value of image_headersz_v1() in kwbimage_generate(), and exit on zero value with EXIT_FAILURE.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index c0f1bdac0210..a5b518f60bc8 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -2029,6 +2029,10 @@ static int kwbimage_generate(struct image_tool_params *params,

case 1: alloc_len = image_headersz_v1(NULL);

• if (!alloc_len) {
  

• 	free(image_cfg);
  

• 	exit(EXIT_FAILURE);
  

• }
  

  break;

  default:

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

BootROM loads kwbimage header to L2-SRAM and BootROM reserve only 192 kB for it.

Signed-off-by: Pali Rohár pali@kernel.org

tools/kwbimage.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index a5b518f60bc8..ce053a4a5a78 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -2033,6 +2033,11 @@ static int kwbimage_generate(struct image_tool_params *params, free(image_cfg); exit(EXIT_FAILURE); }

• if (alloc_len > 192*1024) {
  

• 	fprintf(stderr, "Header is too big (%u bytes), maximal kwbimage header size is %u bytes\n", alloc_len, 192*1024);
  

• 	free(image_cfg);
  

• 	exit(EXIT_FAILURE);
  

• }
  

Nitpicking: Doesn't checkpatch.pl complain about the missing space around '*'? And wouldn't it be better, to add a macro for this '192 * 1024', as you are using it twice here already?

Other than this:

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

break;

default:

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

kwbimage needs to know CPU type, so set it in kwbimage config file.

Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

arch/arm/mach-mvebu/Makefile | 8 ++++++++ arch/arm/mach-mvebu/kwbimage.cfg.in | 3 +++ 2 files changed, 11 insertions(+)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile index acbaa6449d3d..17006c9df9b0 100644 --- a/arch/arm/mach-mvebu/Makefile +++ b/arch/arm/mach-mvebu/Makefile @@ -30,6 +30,14 @@ obj-$(CONFIG_MVEBU_EFUSE) += efuse.o

extra-y += kwbimage.cfg

+ifneq ($(CONFIG_ARMADA_370)$(CONFIG_ARMADA_XP),)

• KWB_REPLACE += CPU
• KWB_CFG_CPU = SHEEVA

+else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)

• KWB_REPLACE += CPU
• KWB_CFG_CPU = A9

+endif

• KWB_REPLACE += BOOT_FROM ifneq ($(CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI),) KWB_CFG_BOOT_FROM=spi

diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in index 049d23c6ef08..8e720daf4820 100644 --- a/arch/arm/mach-mvebu/kwbimage.cfg.in +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in @@ -5,6 +5,9 @@ # Armada 38x uses version 1 image format VERSION 1

+# Type of the CPU core +#@CPU

• # Boot Media configurations #@BOOT_FROM

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

U-Boot SPL for mvebu platform is not compiled as position independent. Therefore it is required to instruct BootROM to load U-Boot SPL at the correct address. Loading of kwbimage binary code at specific address can be now achieved by the new LOAD_ADDRESS token as part of BINARY command in kwbimage config file.

Update mvebu Makefile to put value of $(CONFIG_SPL_TEXT_BASE) into LOAD_ADDRESS token when generating kwbimage.cfg from kwbimage.cfg.in.

It is required to update regex for sed to find replacement tokens at any position on a line in kwbimage config file and not only at the beginning of the line. This is because LOAD_ADDRESS is specified at the end of line containing the BINARY command.

It looks like all Armada boards set CONFIG_SPL_TEXT_BASE to value 0x40004030 or 0x40000030. Why this value? It is because main kwbimage header is at address 0x40004030 or 0x40000000 and it is 32 bytes long. After the main header there is the binary header, which consist of 1 byte for type, 3 bytes for size, 1 byte for number of arguments, 3 reserved bytes and then 4 bytes for each argument. After these arguments comes the executable code.

So arguments start at address 0x40004028 or 0x40000028. Before commit e6571f38c943 ("arm: mvebu: Remove dummy BIN header arguments for SPL binary") there were two (dummy) arguments, which resulted in load address of 0x40004030 or 0x40000030, always. After that commit (which removed dummy arguments), load address stayed same due to the 128-bit alignment done by mkimage.

This patch now reflects the dependency between $(CONFIG_SPL_TEXT_BASE), load address and dummy kwbimage arguments, and allows the user to adjust $(CONFIG_SPL_TEXT_BASE) config option to some other value.

For unsupported values, when mkimage/kwbimage cannot set chosen load address as specified by $(CONFIG_SPL_TEXT_BASE), the build process now fails, instead of silently generating non-working kwbimage.

Removal of this alignment between $(CONFIG_SPL_TEXT_BASE) and LOAD_ADDRESS can only be done by compiling U-Boot SPL as position independent. But this currently is not possible for 32-bit ARM version of U-Boot SPL.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

arch/arm/mach-mvebu/Makefile | 5 ++++- arch/arm/mach-mvebu/kwbimage.cfg.in | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile index 17006c9df9b0..9ace049c9d7c 100644 --- a/arch/arm/mach-mvebu/Makefile +++ b/arch/arm/mach-mvebu/Makefile @@ -38,6 +38,9 @@ else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),) KWB_CFG_CPU = A9 endif

+KWB_REPLACE += LOAD_ADDRESS +KWB_CFG_LOAD_ADDRESS = $(CONFIG_SPL_TEXT_BASE)

• KWB_REPLACE += BOOT_FROM ifneq ($(CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI),) KWB_CFG_BOOT_FROM=spi

@@ -67,7 +70,7 @@ KWB_CFG_SEC_FUSE_DUMP = a38x endif

quiet_cmd_kwbcfg = KWBCFG $@ -cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \ +cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \ <$< >$(dir $@)$(@F)

$(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \ diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in index 8e720daf4820..603e8863450c 100644 --- a/arch/arm/mach-mvebu/kwbimage.cfg.in +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in @@ -11,5 +11,5 @@ VERSION 1 # Boot Media configurations #@BOOT_FROM

-# Binary Header (bin_hdr) with DDR3 training code -BINARY spl/u-boot-spl.bin +# Include U-Boot SPL with DDR3 training code into Binary Header +BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

This alignment is required only for platforms based on Sheeva CPU core which are A370 and AXP. Now when U-Boot build system correctly propagates LOAD_ADDRESS there is no need to have enabled 128-bit boundary alignment on platforms which do not need it. Previously it was required because load address was implicitly rounded to 128-bit boundary and U-Boot build system expected it and misused it. Now with explicit setting of LOAD_ADDRESS there is no guessing for load address anymore.

Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index ce053a4a5a78..7c2106006ad7 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -1101,8 +1101,10 @@ static size_t image_headersz_v1(int *hasext) return 0; } headersz = e->binary.loadaddr - base_addr;

• } else {
  

• } else if (cpu_sheeva) {
  headersz = ALIGN(headersz, 16);
  

• } else {
  

• 	headersz = ALIGN(headersz, 4);
  

  }

  headersz += ALIGN(s.st_size, 4) + sizeof(uint32_t);

@@ -1158,8 +1160,8 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext, *cur += (binarye->binary.nargs + 1) * sizeof(uint32_t);

/*

• * ARM executable code inside the BIN header on some mvebu platforms
  

• * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
  

• * ARM executable code inside the BIN header on platforms with Sheeva
  

• * CPU (A370 and AXP) must always be aligned with the 128-bit boundary.
  

  • In the case when this code is not position independent (e.g. ARM
  • SPL), it must be placed at fixed load and execute address.
  • This requirement can be met by inserting dummy arguments into

@@ -1170,8 +1172,10 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext, offset = *cur - (uint8_t *)main_hdr; if (binarye->binary.loadaddr) add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);

• else

• else if (cpu_sheeva) add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
• else

• add_args = 0;
  

  if (add_args) { *(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args); *cur += add_args * sizeof(uint32_t);

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

BootROMs on pre-A38x SoCs enabled its output on UART by default, but A38x' BootROM has its output on UART disabled by default.

To enable BootROM output on A38x SoC, it is required to set DEBUG flag (which only enables BootROM output and nothing more) in kwbimage. For UART images this DEBUG flag is ignored by BootROM.

Enable kwbimage DEBUG flag for all A38x boards.

With this change BootROM prints the following (success) information on UART before booting U-Boot kwbimage:

BootROM - 1.73 Booting from SPI flash

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

arch/arm/mach-mvebu/Makefile | 7 +++++++ arch/arm/mach-mvebu/kwbimage.cfg.in | 3 +++ 2 files changed, 10 insertions(+)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile index 9ace049c9d7c..74478a3134e3 100644 --- a/arch/arm/mach-mvebu/Makefile +++ b/arch/arm/mach-mvebu/Makefile @@ -69,6 +69,13 @@ KWB_REPLACE += SEC_FUSE_DUMP KWB_CFG_SEC_FUSE_DUMP = a38x endif

+ifdef CONFIG_ARMADA_38X +# BootROM output is by default enabled on pre-A38x and disabled on A38x +# DEBUG flag on A38x for non-UART boot source only enable BootROM output and nothing more +KWB_REPLACE += DEBUG +KWB_CFG_DEBUG = 1 +endif

• quiet_cmd_kwbcfg = KWBCFG $@ cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \ <$< >$(dir $@)$(@F)

diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in index 603e8863450c..ccb09975817e 100644 --- a/arch/arm/mach-mvebu/kwbimage.cfg.in +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in @@ -11,5 +11,8 @@ VERSION 1 # Boot Media configurations #@BOOT_FROM

+# Enable BootROM output via DEBUG flag on SoCs which require it +#@DEBUG

• # Include U-Boot SPL with DDR3 training code into Binary Header BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Data delay is stored as 8-bit number in kwbimage structure. Ensure the given value is at most 255.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 7c2106006ad7..2de8c371c12a 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -1659,6 +1659,10 @@ static int image_create_config_parse_oneline(char *line, el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_SDRAM_SETUP; else el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_MS(strtoul(value1, NULL, 10));

• if (el->regdata_delay > 255) {
  

• 	fprintf(stderr, "Maximal DATA_DELAY is 255\n");
  

• 	return -1;
  

• }
  

  break; case IMAGE_CFG_BAUDRATE: el->baudrate = strtoul(value1, NULL, 10);

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

For debugging purposes it is good to know where the binary image would be loaded and also it is needed to know if printed size is image size or the size of header together with image.

Make it unambiguous by showing that printed size is not the size of the whole header, but only the size of executable code, and print also the executable offset of this binary image. Load/execute address is the offset relative to the base address (either 0x40004000 or 0x40000000).

Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 2de8c371c12a..d1fb67d3db81 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -1872,9 +1872,12 @@ static void kwbimage_print_header(const void *ptr)

for_each_opt_hdr_v1 (ohdr, mhdr) { if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {

• 	printf("BIN Hdr Size: ");
  

• 	printf("BIN Img Size: ");
  genimg_print_size(opt_hdr_v1_size(ohdr) - 12 -
  		  4 * ohdr->data[0]);
  

• 	printf("BIN Img Offs: %08x\n",
  

• 		(unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
  

• 		8 + 4 * ohdr->data[0]);
  

  } }

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

To regenerate kwbimage from existing image, it is needed to have kwbimage config file. Add a new option to generate kwbimage config file from existing kwbimage when '-p 1' option is given.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index d1fb67d3db81..de7e9acf7fe5 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -214,6 +214,17 @@ static int image_boot_mode_id(const char *boot_mode_name) return -1; }

+static const char *image_nand_ecc_mode_name(unsigned int id) +{

• int i;
• for (i = 0; nand_ecc_modes[i].name; i++)

• if (nand_ecc_modes[i].id == id)
  

• 	return nand_ecc_modes[i].name;
  

• return NULL;

+}

• static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name) { int i;

@@ -359,6 +370,29 @@ static uint32_t image_checksum32(void *start, uint32_t len) return csum; }

+static unsigned int options_to_baudrate(uint8_t options) +{

• switch (options & 0x7) {
• case MAIN_HDR_V1_OPT_BAUD_2400:

• return 2400;
  

• case MAIN_HDR_V1_OPT_BAUD_4800:

• return 4800;
  

• case MAIN_HDR_V1_OPT_BAUD_9600:

• return 9600;
  

• case MAIN_HDR_V1_OPT_BAUD_19200:

• return 19200;
  

• case MAIN_HDR_V1_OPT_BAUD_38400:

• return 38400;
  

• case MAIN_HDR_V1_OPT_BAUD_57600:

• return 57600;
  

• case MAIN_HDR_V1_OPT_BAUD_115200:

• return 115200;
  

• case MAIN_HDR_V1_OPT_BAUD_DEFAULT:
• default:

• return 0;
  

• }

+}

• static uint8_t baudrate_to_option(unsigned int baudrate) { switch (baudrate) {

@@ -2088,6 +2122,144 @@ static int kwbimage_generate(struct image_tool_params *params, return 4 + (4 - s.st_size % 4) % 4; }

+static int kwbimage_generate_config(void *ptr, struct image_tool_params *params) +{

• struct main_hdr_v0 *mhdr0 = (struct main_hdr_v0 *)ptr;
• struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
• size_t header_size = kwbheader_size(ptr);
• struct register_set_hdr_v1 *regset_hdr;
• struct ext_hdr_v0_reg *regdata;
• struct ext_hdr_v0 *ehdr0;
• struct opt_hdr_v1 *ohdr;
• unsigned offset;
• int cur_idx;
• int version;
• FILE *f;
• int i;
• f = fopen(params->outfile, "w");
• if (!f) {

• fprintf(stderr, "Can't open \"%s\": %s\n", params->outfile, strerror(errno));
  

• return -1;
  

• }
• version = kwbimage_version(ptr);
• if (version != 0)

• fprintf(f, "VERSION %d\n", version);
  

• fprintf(f, "BOOT_FROM %s\n", image_boot_mode_name(mhdr->blockid) ?: "<unknown>");
• if (version == 0 && mhdr->blockid == IBR_HDR_NAND_ID)

• fprintf(f, "NAND_ECC_MODE %s\n", image_nand_ecc_mode_name(mhdr0->nandeccmode));
  

• if (mhdr->blockid == IBR_HDR_NAND_ID)

• fprintf(f, "NAND_PAGE_SIZE 0x%x\n", (unsigned)mhdr->nandpagesize);
  

• if (version != 0 && mhdr->blockid == IBR_HDR_NAND_ID) {

• fprintf(f, "NAND_BLKSZ 0x%x\n", (unsigned)mhdr->nandblocksize);
  

• fprintf(f, "NAND_BADBLK_LOCATION 0x%x\n", (unsigned)mhdr->nandbadblklocation);
  

• }
• if (version == 0 && mhdr->blockid == IBR_HDR_SATA_ID)

• fprintf(f, "SATA_PIO_MODE %u\n", (unsigned)mhdr0->satapiomode);
  

• /*

• * Addresses and sizes which are specified by mkimage command line
  

• * arguments and not in kwbimage config file
  

• */
  

• if (version != 0)

• fprintf(f, "#HEADER_SIZE 0x%x\n",
  

• 	((unsigned)mhdr->headersz_msb << 8) | le16_to_cpu(mhdr->headersz_lsb));
  

• fprintf(f, "#SRC_ADDRESS 0x%x\n", le32_to_cpu(mhdr->srcaddr));
• fprintf(f, "#BLOCK_SIZE 0x%x\n", le32_to_cpu(mhdr->blocksize));
• fprintf(f, "#DEST_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->destaddr));
• fprintf(f, "#EXEC_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->execaddr));
• if (version != 0) {

• if (options_to_baudrate(mhdr->options))
  

• 	fprintf(f, "BAUDRATE %u\n", options_to_baudrate(mhdr->options));
  

• if (options_to_baudrate(mhdr->options) ||
  

•     ((mhdr->options >> 3) & 0x3) || ((mhdr->options >> 5) & 0x7)) {
  

• 	fprintf(f, "UART_PORT %u\n", (unsigned)((mhdr->options >> 3) & 0x3));
  

• 	fprintf(f, "UART_MPP 0x%x\n", (unsigned)((mhdr->options >> 5) & 0x7));
  

• }
  

• if (mhdr->flags & 0x1)
  

• 	fprintf(f, "DEBUG 1\n");
  

• }
• cur_idx = 1;
• for_each_opt_hdr_v1(ohdr, ptr) {

• if (ohdr->headertype == OPT_HDR_V1_SECURE_TYPE) {
  

• 	fprintf(f, "#SECURE_HEADER\n");
  

• } else if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {
  

• 	fprintf(f, "BINARY binary%d.bin", cur_idx);
  

• 	for (i = 0; i < ohdr->data[0]; i++)
  

• 		fprintf(f, " 0x%x", le32_to_cpu(((uint32_t *)ohdr->data)[i + 1]));
  

• 	offset = (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) + 8 + 4 * ohdr->data[0];
  

• 	fprintf(f, " LOAD_ADDRESS 0x%08x\n", 0x40000000 + offset);
  

• 	fprintf(f, " # for CPU SHEEVA: LOAD_ADDRESS 0x%08x\n", 0x40004000 + offset);
  

• 	cur_idx++;
  

• } else if (ohdr->headertype == OPT_HDR_V1_REGISTER_TYPE) {
  

• 	regset_hdr = (struct register_set_hdr_v1 *)ohdr;
  

• 	for (i = 0;
  

• 	     i < opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) -
  

• 		 sizeof(regset_hdr->data[0].last_entry);
  

• 	     i++)
  

• 		fprintf(f, "DATA 0x%08x 0x%08x\n",
  

• 			le32_to_cpu(regset_hdr->data[i].entry.address),
  

• 			le32_to_cpu(regset_hdr->data[i].entry.value));
  

• 	if (opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) >=
  

• 	    sizeof(regset_hdr->data[0].last_entry)) {
  

• 		if (regset_hdr->data[0].last_entry.delay)
  

• 			fprintf(f, "DATA_DELAY %u\n",
  

• 				(unsigned)regset_hdr->data[0].last_entry.delay);
  

• 		else
  

• 			fprintf(f, "DATA_DELAY SDRAM_SETUP\n");
  

• 	}
  

• }
  

• }
• if (version == 0 && mhdr0->ext) {

• ehdr0 = (struct ext_hdr_v0 *)(mhdr0 + 1);
  

• if (ehdr0->offset) {
  

• 	for (regdata = (struct ext_hdr_v0_reg *)((uint8_t *)ptr + ehdr0->offset);
  

• 	     (uint8_t *)regdata < (uint8_t *)ptr + header_size && regdata->raddr &&
  

• 	     regdata->rdata;
  

• 	     regdata++)
  

• 		fprintf(f, "DATA 0x%08x 0x%08x\n", le32_to_cpu(regdata->raddr),
  

• 			le32_to_cpu(regdata->rdata));
  

• }
  

• }
• if (version == 0 && le16_to_cpu(mhdr0->ddrinitdelay))

• fprintf(f, "DDR_INIT_DELAY %u\n", (unsigned)le16_to_cpu(mhdr0->ddrinitdelay));
  

• /* Undocumented reserved fields */
• if (version == 0 && (mhdr0->rsvd1[0] || mhdr0->rsvd1[1] || mhdr0->rsvd1[2]))

• fprintf(f, "#RSVD1 0x%x 0x%x 0x%x\n", (unsigned)mhdr0->rsvd1[0],
  

• 	(unsigned)mhdr0->rsvd1[1], (unsigned)mhdr0->rsvd1[2]);
  

• if (version == 0 && mhdr0->rsvd3)

• fprintf(f, "#RSVD3 0x%x\n", (unsigned)mhdr0->rsvd3);
  

• if (version == 0 && le16_to_cpu(mhdr0->rsvd2))

• fprintf(f, "#RSVD2 0x%x\n", (unsigned)le16_to_cpu(mhdr0->rsvd2));
  

• if (version != 0 && mhdr->reserved4)

• fprintf(f, "#RESERVED4 0x%x\n", (unsigned)mhdr->reserved4);
  

• if (version != 0 && mhdr->reserved5)

• fprintf(f, "#RESERVED5 0x%x\n", (unsigned)le16_to_cpu(mhdr->reserved5));
  

• fclose(f);
• return 0;

+}

• static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params) { struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;

@@ -2099,6 +2271,10 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params ulong image; ulong size;

• /* Generate kwbimage config file when '-p -1' is specified */
• if (idx == -1)

• return kwbimage_generate_config(ptr, params);
  

• for_each_opt_hdr_v1 (ohdr, ptr) { if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE) continue;

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Avoid casting const to non-const.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

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

diff --git a/tools/kwbimage.h b/tools/kwbimage.h index 8d37357e5abd..c000cba4b8d1 100644 --- a/tools/kwbimage.h +++ b/tools/kwbimage.h @@ -235,11 +235,11 @@ static inline int opt_hdr_v1_valid_size(const struct opt_hdr_v1 *ohdr, { uint32_t ohdr_size;

• if ((void *)(ohdr + 1) > mhdr_end)

• if ((const void *)(ohdr + 1) > mhdr_end) return 0;

  ohdr_size = opt_hdr_v1_size(ohdr);

• if (ohdr_size < 8 || (void *)((uint8_t *)ohdr + ohdr_size) > mhdr_end)

• if (ohdr_size < 8 || (const void *)((const uint8_t *)ohdr + ohdr_size) > mhdr_end) return 0;

  return 1;

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Despite the official specification, BootROM does not look at the lowest bit of ext field but rather checks if ext field is non-zero.

Moreover original Marvell doimage tool puts into the mhdr->ext field the number of extended headers, so basically it sets ext filed to non-zero value if some extended header is present.

Fix U-Boot dumpimage and kwboot tools to parse correctly also kwbimage files created by Marvell doimage tool, in the same way as the BootROM is doing it when booting these images.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 2 +- tools/kwbimage.h | 6 +++--- tools/kwboot.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index de7e9acf7fe5..92d163b6050e 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -1948,7 +1948,7 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size, if (kwbimage_version(ptr) == 0) { struct main_hdr_v0 *mhdr = (struct main_hdr_v0 *)ptr;

• if (mhdr->ext & 0x1) {
  

• if (mhdr->ext) {
  struct ext_hdr_v0 *ext_hdr = (void *)(mhdr + 1);
  
  csum = image_checksum8(ext_hdr, sizeof(*ext_hdr) - 1);
  

diff --git a/tools/kwbimage.h b/tools/kwbimage.h index c000cba4b8d1..9ebc7d72d363 100644 --- a/tools/kwbimage.h +++ b/tools/kwbimage.h @@ -208,7 +208,7 @@ static inline size_t kwbheader_size(const void *header) const struct main_hdr_v0 *hdr = header;

return sizeof(*hdr) +

•        (hdr->ext & 0x1) ? sizeof(struct ext_hdr_v0) : 0;
  

•        hdr->ext ? sizeof(struct ext_hdr_v0) : 0;
  

  } else { const struct main_hdr_v1 *hdr = header;

@@ -252,7 +252,7 @@ static inline struct opt_hdr_v1 *opt_hdr_v1_first(void *img) { return NULL;

mhdr = img;

• if (mhdr->ext & 0x1)

• if (mhdr->ext) return (struct opt_hdr_v1 *)(mhdr + 1); else return NULL;

@@ -272,7 +272,7 @@ static inline struct opt_hdr_v1 *_opt_hdr_v1_next(struct opt_hdr_v1 *cur)

static inline struct opt_hdr_v1 *opt_hdr_v1_next(struct opt_hdr_v1 *cur) {

• if (*opt_hdr_v1_ext(cur) & 0x1)

• if (*opt_hdr_v1_ext(cur)) return _opt_hdr_v1_next(cur); else return NULL;

diff --git a/tools/kwboot.c b/tools/kwboot.c index d22e6ea96a5c..c3d8ab654417 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1398,7 +1398,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz) uint32_t ohdrsz; uint8_t *prev_ext;

• if (hdr->ext & 0x1) {

• if (hdr->ext) { for_each_opt_hdr_v1 (ohdr, img) if (opt_hdr_v1_next(ohdr) == NULL) break;

@@ -1422,7 +1422,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz) ohdrsz = sizeof(*ohdr) + 4 + 4 * num_args + binsz + 4; kwboot_img_grow_hdr(hdr, size, ohdrsz);

• *prev_ext |= 1;

• *prev_ext = 1;

  ohdr->headertype = OPT_HDR_V1_BINARY_TYPE; ohdr->headersz_msb = ohdrsz >> 16;

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

When there is no -p argument for dumpimage tool specified, extract the main data image from kwbimage file. This makes dumpimage consistent with other image formats.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 67 +++++++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 92d163b6050e..d159087d9dd6 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -2266,7 +2266,7 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params size_t header_size = kwbheader_size(ptr); struct opt_hdr_v1 *ohdr; int idx = params->pflag;

• int cur_idx = 0;

• int cur_idx; uint32_t offset; ulong image; ulong size;

@@ -2275,41 +2275,54 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params if (idx == -1) return kwbimage_generate_config(ptr, params);

• for_each_opt_hdr_v1 (ohdr, ptr) {

• if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
  

• 	continue;
  

• image = 0;
• size = 0;
• if (idx == 0) {

• /* Extract data image when -p is not specified or when '-p 0' is specified */
  

• offset = le32_to_cpu(mhdr->srcaddr);
  
  

• if (idx == cur_idx) {
  

• 	image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
  

• 	size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
  

• 	goto extract;
  

• if (mhdr->blockid == IBR_HDR_SATA_ID) {
  

• 	offset -= 1;
  

• 	offset *= 512;
  

  }

• ++cur_idx;
  

• }

• if (mhdr->blockid == IBR_HDR_SDIO_ID)
  

• 	offset *= 512;
  
  

• if (idx != cur_idx) {

• printf("Image %d is not present\n", idx);
  

• return -1;
  

• }
• offset = le32_to_cpu(mhdr->srcaddr);

• if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
  

• 	offset = header_size;
  
  

• if (mhdr->blockid == IBR_HDR_SATA_ID) {

• offset -= 1;
  

• offset *= 512;
  

• }

• image = (ulong)((uint8_t *)ptr + offset);
  

• size = le32_to_cpu(mhdr->blocksize) - 4;
  

• } else {

• /* Extract N-th binary header executabe image when other '-p N' is specified */
  

• cur_idx = 1;
  

• for_each_opt_hdr_v1(ohdr, ptr) {
  

• 	if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
  

• 		continue;
  
  

• if (mhdr->blockid == IBR_HDR_SDIO_ID)

• offset *= 512;
  

• 	if (idx == cur_idx) {
  

• 		image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
  

• 		size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
  

• 		break;
  

• 	}
  
  

• if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)

• offset = header_size;
  

• 	++cur_idx;
  

• }
  
  

• image = (ulong)((uint8_t *)ptr + offset);
• size = le32_to_cpu(mhdr->blocksize) - 4;

• if (!image) {
  

• 	fprintf(stderr, "Argument -p %d is invalid\n", idx);
  

• 	fprintf(stderr, "Available subimages:\n");
  

• 	fprintf(stderr, " -p -1  - kwbimage config file\n");
  

• 	fprintf(stderr, " -p 0   - data image\n");
  

• 	if (cur_idx - 1 > 0)
  

• 		fprintf(stderr, " -p N   - Nth binary header image (totally: %d)\n",
  

• 			cur_idx - 1);
  

• 	return -1;
  

• }
  

• }

-extract: return imagetool_save_subimage(params->outfile, image, size); }

Viele Grüße, Stefan Roese

On 1/12/22 18:20, Pali Rohár wrote:

Do not check for kwbimage configuration file when just showing information about existing kwbimage file.

The check for kwbimage configuration file is required only when creating kwbimage, not when showing information about image or when extracting data from image.

With this change, it is possible to call mkimage -l and dumpimage -l also for existing kwbimage file.

Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index d159087d9dd6..9b63ce80ff4e 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -2331,7 +2331,8 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params */ static int kwbimage_check_params(struct image_tool_params *params) {

• if (!params->iflag && (!params->imagename || !strlen(params->imagename))) {

• if (!params->lflag && !params->iflag &&

•    (!params->imagename || !strlen(params->imagename))) {
  

  char *msg = "Configuration file for kwbimage creation omitted";

  fprintf(stderr, "Error:%s - %s\n", params->cmdname, msg);

Viele Grüße, Stefan Roese