On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut marex@denx.de wrote:
On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut marex@denx.de wrote:
The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location is available and can be corrupted by loading ie. uImage or fitImage headers there. Sometimes it could be beneficial to load the headers elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we still want to parse the image headers in some local onchip memory to ie. extract firmware from that image.
Add the possibility to override the location where the headers get loaded by introducing new function, spl_get_load_buffer() which takes two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the data that are to be loaded there -- and returns a valid buffer address or hangs the system. The default behavior is the same as before, add the offset to CONFIG_SYS_TEXT_BASE and return that address. User can override the weak spl_get_load_buffer() function though.
Signed-off-by: Marek Vasut marex@denx.de Cc: Tom Rini trini@konsulko.com
V2: Fix build issues on multiple boards due to incorrect pointer casting
[...]
diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c index e594beaeaa..619b39a537 100644 --- a/common/spl/spl_ram.c +++ b/common/spl/spl_ram.c @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image, * No binman support or no information. For now, fix it * to the address pointed to by U-Boot. */
u_boot_pos = CONFIG_SYS_TEXT_BASE -sizeof(struct image_header);
header = spl_get_load_buffer(-sizeof(*header),sizeof(*header));Using "spl_get_load_buffer()" here seems to be a bit misleading, as the address is used for "execute-in-place", not for loading.
Do you have a better solution ? Instead of hard-coding the load buffer address with some macro, this uses function which could be overridden. Whether it's XIP or not has nothing to do with it.
I meant the name is a bit misleading as it implies loading. But since the preferred way to do this is using binman, it's probably not worth adding yet another weak function for RAM boot?
} header = (struct image_header *)map_sysmem(u_boot_pos, 0);diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c index ba60a3a3c5..e10cf0124f 100644 --- a/common/spl/spl_spi.c +++ b/common/spl/spl_spi.c @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image, return -ENODEV; }
/* use CONFIG_SYS_TEXT_BASE as temporary storage area */header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
header = spl_get_load_buffer(-sizeof(*header), 0x40);Shouldn't the first argument be 0 here instead of -sizeof(*header)?
No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
Sorry, I haven't studied the code around the patch, only the patch. And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to "CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might be a change required to get it work, I don't know that. But as this isn' mentioned in the commit message, to me it seemed like a copy and paste error or something.
Simon