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

[Marek Vasut]

On 01/02/2017 01:07 AM, Lukasz Majewski wrote:
> 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 :-)

Great, I like this solution.

> Big thanks Marek to be vigilant :-)

You're welcome ;-)

-- 
Best regards,
Marek Vasut