[U-Boot] [PATCH] SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

[Lukasz Majewski]

Hi Marek,

> On 12/30/2016 10:28 PM, Lukasz Majewski wrote:
> > Hi Marek,
> > 
> >> On 12/29/2016 04:26 PM, Tom Rini wrote:
> >>> On Thu, Dec 29, 2016 at 12:41:06AM +0100, Marek Vasut wrote:
> >>>> On 12/28/2016 09:52 AM, Lukasz Majewski wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>>> On 12/26/2016 05:36 PM, Lukasz Majewski wrote:
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>>> On 11/29/2016 07:18 PM, Tom Rini wrote:
> >>>>>>>>> On Tue, Nov 29, 2016 at 11:50:34AM +0100, Marek Vasut wrote:
> >>>>>>>>>> On 11/29/2016 10:11 AM, Lukasz Majewski wrote:
> >>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>
> >>>>>>>>>>>> On 11/28/2016 10:09 PM, Lukasz Majewski wrote:
> >>>>>>>>>>>>> This define gives the possibility to copy entire image
> >>>>>>>>>>>>> (including header - e.g. u-boot.img) from NOR parallel
> >>>>>>>>>>>>> memory to e.g. SDRAM. The current code only supports
> >>>>>>>>>>>>> loading the raw binary image (the u-boot.bin).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> The legacy behavior is preserved, since other board
> >>>>>>>>>>>>> don't enabled this option.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Sooooo, what's the usecase again ? ;-) 
> >>>>>>>>>>>
> >>>>>>>>>>> :-)
> >>>>>>>>>>>
> >>>>>>>>>>> The use case is to allow u-boot.img being loaded from
> >>>>>>>>>>> Parallel NOR. The current code only supports u-boot.bin.
> >>>>>>>>>>
> >>>>>>>>>> Why is u-boot.bin (or the payload) not sufficient ? Why do
> >>>>>>>>>> you need the header ?
> >>>>>>>>>
> >>>>>>>>> Well, the general use-case and code flow is that we load
> >>>>>>>>> u-boot.img (or a FIT image) and if all else fails, fall back
> >>>>>>>>> to assuming a .bin and a known address).
> >>>>>>>>>
> >>>>>>>> And exactly how is that whole image useful in RAM ? Sorry, I
> >>>>>>>> still do not see it, usually you just need the executable
> >>>>>>>> payload, although even that can be left in flash most of the
> >>>>>>>> time.
> >>>>>>>
> >>>>>>> The use case is that I do want to boot from SD card/eMMC and
> >>>>>>> NOR with using u-boot.img.
> >>>>>>>
> >>>>>>> I would like to avoid situation when for NOR I must use
> >>>>>>> u-boot.bin and for eMMC u-boot.img.
> >>>>>>>
> >>>>>>> Such approach keeps things as simple as possible :-)
> >>>>>>
> >>>>>> Oh, so it allows you to detect bitrot for the content in SPI
> >>>>>> NOR ?
> >>>>>
> >>>>> I do not use SPI NOR, it is parallel NOR.
> >>>>
> >>>> Sorry, I meant parallel NOR of course.
> >>>>
> >>>>>> It's a bit strange we had to use u-boot.bin with SPL there.
> >>>>>>
> >>>>>
> >>>>> This is how the legacy system behaves. It uses (by default)
> >>>>> Parallel NOR for booting (with advised/provided NOR memory
> >>>>> timings). After doing some measurements, it turned out that for
> >>>>> "tunned" u-boot/SPL there would be the best way to copy it to
> >>>>> ram and execute it from there (just like eMMC).
> >>>>>
> >>>>> Hence, I would like to use u-boot.img in both booting scenarios.
> >>>>
> >>>> I think I was mistaken yesterday, I don't think I understand why
> >>>> copying the image including the header into RAM has any benefit
> >>>> compared to copying just the image payload to RAM (and yes, we're
> >>>> getting back to my original question).
> >>>
> >>> Code complexity and forward compatibility?
> >>
> >> This is adding code complexity, but this is not my point.
> >>
> >>> The general case in the SPL
> >>> framework is that we have either a "legacy" image or a FIT image
> >>> and we fall back to "well, just run it!".
> >>
> >> Well, this doesn't answer my question, because if I understand this
> >> patch correctly, it copies the entire legacy image (incl. header)
> >> into RAM instead of copying just the image payload (which we
> >> already do). I don't really understand why we want to do this. Or
> >> do I misunderstand something ?
> > 
> > No, you understood everything correctly. After some thoughts, I
> > think that only payload should be copied.
> 
> But that's what we already do, no ? So what is the point of this
> patch ?

So now I do know a bit more ...

Let's start with ./common/spl/spl.c - spl_parse_image_header()

With SPL_COPY_PAYLOAD_ONLY flag set (@ common/spl/spl_nor.c) we go to:

if (spl_image->flags & SPL_COPY_PAYLOAD_ONLY) {
	/*
	 * On some system (e.g. powerpc), the load-address and
	 * entry-point is located at address 0. We can't load
	 * to 0-0x40. So skip header in this case.
	 */
	spl_image->load_addr = image_get_load(header);
	spl_image->entry_point = image_get_ep(header);
	^^^^^^^^^^^^^^^^^^^^^^^- here we set it to 0x0 by default (which
	is not true for our setup - we expect to boot from 0x17800000)

	spl_image->size = image_get_data_size(header);
}

My patch:

1. Do not set SPL_COPY_PAYLOAD_ONLY flag, so we go to else clause,
which according to comment:
"/* Load including the header */"
and performs some address manipulation.

2. In my patch I undo those address calculations to load only payload.


Conclusion :-)
--------------

And most of all ........ after thinking for a few minutes it is just
enough to add:

#define CONFIG_SYS_UBOOT_START CONFIG_SYS_TEXT_BASE


to my config file and this patch can be dropped :-)


Big thanks Marek to be vigilant :-)

> 
> > I'll prepare test setup and double check this patch usability by
> > Monday.
> 
> OK
> 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de