Marek Vasut marex@denx.de schrieb am Mi., 15. Aug. 2018, 10:57:
On 08/14/2018 08:25 PM, Simon Goldschmidt wrote:
On Tue, Aug 14, 2018 at 8:17 PM Marek Vasut marex@denx.de wrote:
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.
I may have a use case for one of our custom boards, but it's probably not worth doing that now.
Subsequent patch then ?
I'll send a patch if we really need that...
>>> } >>> 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.
Right. All I wanted to say is I saw something that changed but from the commit message, it didn't seem like it was intended. The rest seems fine to me right now, so if it's of any use:
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
-- Best regards, Marek Vasut