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

[Simon Goldschmidt]

On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <marex at 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 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?
>
> 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.

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 :-)

> >>>>                 }
> >>>>                 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'.

Simon