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

[Simon Goldschmidt]

Marek Vasut <marex at 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 at 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 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.
> >>
> >> 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 at gmail.com>
> >
>
>
> --
> Best regards,
> Marek Vasut
>