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

[Marek Vasut]

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.

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