Marek Vasut <ma...@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 <ma...@denx.de> wrote: > >> > >> On 08/14/2018 02:56 PM, Simon Goldschmidt wrote: > >>> On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <ma...@denx.de> wrote: > >>>> > >>>> On 08/14/2018 12:22 PM, Simon Goldschmidt wrote: > >>>>> On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <ma...@denx.de> wrote: > >>>>>> > >>>>>> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote: > >>>>>>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <ma...@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 <ma...@denx.de> > >>>>>>>> Cc: Tom Rini <tr...@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.goldschm...@gmail.com> > > > > > -- > Best regards, > Marek Vasut > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
