| Message ID | 20200408080942.7694-21-sr@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor the architecture parts of mt7628 | expand |
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 at denx.de> > Cc: Weijie Gao <weijie.gao at mediatek.com> > Cc: Simon Goldschmidt <simon.k.r.goldschmidt at 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. > + 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); > } > #endif > if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) { > @@ -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()? If so then the copy should be made in spl_load_legacy_img() itself. > > return 0; > } > diff --git a/include/spl.h b/include/spl.h > index 6087cd793c..5331591db5 100644 > --- a/include/spl.h > +++ b/include/spl.h > @@ -219,6 +219,20 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, > #define SPL_COPY_PAYLOAD_ONLY 1 > #define SPL_FIT_FOUND 2 > > +/** > + * spl_load_legacy_img() - Loads a legacy image from a device. > + * @spl_image: Image description to set up > + * @dataptr: Source location of the image > + * @header: Image header to parse > + * > + * Reads an legacy image from the device. Loads u-boot image to > + * specified load address. > + * Returns 0 on success. > + */ > +int spl_load_legacy_img(struct spl_image_info *spl_image, > + const struct image_header *header, > + uintptr_t dataptr, struct spl_load_info *load); > + > /** > * spl_load_imx_container() - Loads a imx container image from a device. > * @spl_image: Image description to set up >
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 at denx.de> >> Cc: Weijie Gao <weijie.gao at mediatek.com> >> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at 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); >> } >> #endif >> if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) { >> @@ -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
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); + 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); } #endif if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) { @@ -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); + } return 0; } diff --git a/include/spl.h b/include/spl.h index 6087cd793c..5331591db5 100644 --- a/include/spl.h +++ b/include/spl.h @@ -219,6 +219,20 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, #define SPL_COPY_PAYLOAD_ONLY 1 #define SPL_FIT_FOUND 2 +/** + * spl_load_legacy_img() - Loads a legacy image from a device. + * @spl_image: Image description to set up + * @dataptr: Source location of the image + * @header: Image header to parse + * + * Reads an legacy image from the device. Loads u-boot image to + * specified load address. + * Returns 0 on success. + */ +int spl_load_legacy_img(struct spl_image_info *spl_image, + const struct image_header *header, + uintptr_t dataptr, struct spl_load_info *load); + /** * spl_load_imx_container() - Loads a imx container image from a device. * @spl_image: Image description to set up
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 at denx.de> Cc: Weijie Gao <weijie.gao at mediatek.com> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at 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(-)