[U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage

[Simon Goldschmidt]

On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex at 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 at 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 at denx.de>
> >> Cc: Tom Rini <trini at 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