[PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes
Pali Rohár
pali at kernel.org
Wed Jan 12 15:16:20 CET 2022
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.
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.
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;
More information about the U-Boot
mailing list