On 09.04.20 20:51, Daniel Schwierzeck wrote:
Am 08.04.20 um 10:09 schrieb Stefan Roese:
Move the legacy image loading into spl_legacy.c. This makes it easier to extend the legacy image handling with new features that other SPL loaders might use (e.g. spl_spi.c etc).
No functional change indended.
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
Changes in v6:
New patch
common/spl/spl_legacy.c | 17 +++++++++++++++++ common/spl/spl_nor.c | 34 +++++++++++++++++----------------- include/spl.h | 14 ++++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index 772135193e..d0fc4b111e 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -51,3 +51,20 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image,
return 0; }
+int spl_load_legacy_img(struct spl_image_info *spl_image,
const struct image_header *header,uintptr_t dataptr, struct spl_load_info *info)+{
- int ret;
- ret = spl_parse_image_header(spl_image, header);
- if (ret)
return ret;- dataptr = (uintptr_t)header + sizeof(struct image_header);
how should this work? You call this function with
spl_load_legacy_img(spl_image, &hdr, dataptr, &load);
so "header" points to the copy on the stack. "dataptr" already holds the address of the SPL payload. If you overwrite "dataptr" now, it will point to the stack area after the header copy.
You are correct. I fat-fingered while rebasing the patches here. As you will see, this assignment will be removed in the next patch [21/26].
I'll change it in v7.
- info->read(info, dataptr, spl_image->size,
(void *)(unsigned long)spl_image->load_addr);- return 0;
+} diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c index cb8e06fe1f..d55165b772 100644 --- a/common/spl/spl_nor.c +++ b/common/spl/spl_nor.c @@ -24,11 +24,8 @@ unsigned long __weak spl_nor_get_uboot_base(void) static int spl_nor_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) {
int ret; __maybe_unused const struct image_header *header; __maybe_unused struct spl_load_info load;
struct image_header hdr;
uintptr_t dataptr;
/*
- Loading of the payload to SDRAM is done with skipping of
@@ -45,6 +42,8 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, header = (const struct image_header *)CONFIG_SYS_OS_BASE; #ifdef CONFIG_SPL_LOAD_FIT if (image_get_magic(header) == FDT_MAGIC) {
int ret;debug("Found FIT\n"); load.bl_len = 1; load.read = spl_nor_load_read;@@ -63,6 +62,7 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, #endif if (image_get_os(header) == IH_OS_LINUX) { /* happy - was a Linux */
int ret; ret = spl_parse_image_header(spl_image, header); if (ret)@@ -95,11 +95,9 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, debug("Found FIT format U-Boot\n"); load.bl_len = 1; load.read = spl_nor_load_read;
ret = spl_load_simple_fit(spl_image, &load,spl_nor_get_uboot_base(),(void *)header);return ret;
return spl_load_simple_fit(spl_image, &load,spl_nor_get_uboot_base(),(void *)header);@@ -109,17 +107,19 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, spl_nor_get_uboot_base()); }
- ret = spl_parse_image_header(spl_image,
(const struct image_header *)spl_nor_get_uboot_base());- if (ret)
return ret;
- if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_SUPPORT)) {
struct image_header hdr;uintptr_t dataptr;
- /* Payload image may not be aligned, so copy it for safety */
- memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
- dataptr = spl_nor_get_uboot_base() + sizeof(struct image_header);
/* Payload image may not be aligned, so copy it for safety */memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));dataptr = spl_nor_get_uboot_base() + sizeof(hdr);
- memcpy((void *)(unsigned long)spl_image->load_addr,
(void *)dataptr, spl_image->size);
/* Legacy image handling */load.bl_len = 1;load.read = spl_nor_load_read;return spl_load_legacy_img(spl_image, &hdr, dataptr, &load);- }
I don't understand it fully. Is the purpose of the header copy to avoid unaligned access to the header in spl_parse_image_header()?
Yes.
If so then the copy should be made in spl_load_legacy_img() itself.
Thats what I had implemented in an internal version already. Not sure, why I switched to not using it in this v6? I'll re-think about it and will perhaps move it to spl_load_legacy_img() again.
Thanks, Stefan