On 08/14/2018 02:56 PM, Simon Goldschmidt wrote:
On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut marex@denx.de wrote:
On 08/14/2018 12:22 PM, Simon Goldschmidt wrote:
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?
Do you have a better name that fits all the other usecases ? This function just gets you buffer into which you can load the image.
Not really. I just wonder if you have to override the location for some board, RAM booting might not work any more as it relies on a fixed address, not some generic buffer.
I do, yeah, the board is not upstream completely yet though, so I am just sending this as a cleanup.
Maybe we could add the boot device to your new weak function? If we add some comment to the new weak function, that would have made it much more clear for me how to boot U-Boot from FPGA OnChip RAM when I tried some months ago :-)
This really just gives you a buffer. I don't need to know which boot media is used. If there is a usecase, sure, it can be added later.
} 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.
I suspect it's the SPI that's weird. Look at the surrounding code, IMO this is how it should be.
Reading the code, I guess the exact location of 'header' is not important. So the code should still work after applying your patch, even if it changes the location of 'header'.
The thing is, the payload (ie. uboot) is linked against the TEXT_BASE, so putting it at TEXT_BASE + offset can cause trouble.
Simon