Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

Stefan Roese
2022-01-12T15:07:11Z

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.01-armada-18.06/tools/marvell/bin_hdr/inc/common/soc_spec.h#L84-L92
>>>
>>> #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;
>