On 08/14/2018 02:56 PM, Simon Goldschmidt wrote: > On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <ma...@denx.de> wrote: >> >> On 08/14/2018 12:22 PM, Simon Goldschmidt wrote: >>> On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <ma...@denx.de> wrote: >>>> >>>> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote: >>>>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <ma...@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 <ma...@denx.de> >>>>>> Cc: Tom Rini <tr...@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 > -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot