Re: [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.
>>
>> So, 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 0x1780)
> 
>   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 

Re: [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.
> 
>  So, 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 0x1780)

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 

Re: [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.
> 
>  So, 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 ?

That is why I need to double check it :-)

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

Best regards,
Łukasz Majewski


pgp_2il8_BMcV.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

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.

 So, 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 ?

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

OK

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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/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.
> >>
> >> So, 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.

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

Best regards,
Łukasz Majewski


pgpzNU3WQwbjO.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

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.
>>
>> So, 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 ?

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Tom Rini]

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.
> 
>  So, 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?  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!".

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

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.

 So, 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).

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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/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.
> >>
> >> So, 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.

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

Best regards,
Łukasz Majewski



pgp6qvgEtQg6c.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

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.
>>
>> So, 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 ?
It's a bit strange we had to use u-boot.bin with SPL there.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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 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.
> 
>  So, 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 :-)

Best regards,
Łukasz Majewski





pgpj27Ph3jd2S.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

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.

 So, 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.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Tom Rini]

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.
> >>
> >> So, 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).

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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 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.
> 
> So, 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.

This code is crucial for boards booting from NOR, as a board which
support will be sent soon to u-boot ML :-).

Best regards,
Łukasz Majewski

> I still miss it being
> documented.
> 
> > Signed-off-by: Lukasz Majewski 
> > ---
> > Changes for v2:
> > - Update to code to apply on v2016.11+
> > Changes for v3:
> > - Write better commit mesage to explain the problem
> > 
> > ---
> >  common/spl/Kconfig   | 10 ++
> >  common/spl/spl_nor.c | 12 +---
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > index df9e0ce..d31b26d 100644
> > --- a/common/spl/Kconfig
> > +++ b/common/spl/Kconfig
> > @@ -399,6 +399,16 @@ config SPL_NOR_SUPPORT
> >   a memory-mapped device makes it very easy to access.
> > Loading from NOR is typically achieved with just a memcpy().
> >  
> > +config SPL_NOR_COPY_ENTIRE_IMAGE
> > +   bool
> > +   depends on SPL_NOR_SUPPORT
> > +   prompt "Copy entire image from NOR memory"
> > +   default n
> > +   help
> > + By default the SPL NOR driver supports copying only
> > payload to
> > + destination address. Say Y if you want to copy entire
> > image (including
> > + its image header).
> > +
> >  config SPL_ONENAND_SUPPORT
> > bool "Support OneNAND flash"
> > depends on SPL
> > diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> > index 6bfa399..44f3e99 100644
> > --- a/common/spl/spl_nor.c
> > +++ b/common/spl/spl_nor.c
> > @@ -10,13 +10,15 @@
> >  static int spl_nor_load_image(struct spl_image_info *spl_image,
> >   struct spl_boot_device *bootdev)
> >  {
> > +   void *img_src;
> > int ret;
> > +#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
> > /*
> >  * Loading of the payload to SDRAM is done with skipping of
> >  * the mkimage header in this SPL NOR driver
> >  */
> > spl_image->flags |= SPL_COPY_PAYLOAD_ONLY;
> > -
> > +#endif
> >  #ifdef CONFIG_SPL_OS_BOOT
> > if (!spl_start_uboot()) {
> > const struct image_header *header;
> > @@ -65,9 +67,13 @@ static int spl_nor_load_image(struct
> > spl_image_info *spl_image, if (ret)
> > return ret;
> >  
> > +   img_src = (void *)CONFIG_SYS_UBOOT_BASE;
> > +#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
> > +   img_src += sizeof(struct image_header));
> > +#endif
> > +
> > memcpy((void *)(unsigned long)spl_image->load_addr,
> > -  (void *)(CONFIG_SYS_UBOOT_BASE + sizeof(struct
> > image_header)),
> > -  spl_image->size);
> > +  img_src, spl_image->size);
> >  
> > return 0;
> >  }
> > 
> 
> 



pgpPWtQT8hSff.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

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.
>>
>> So, 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 ?

> This code is crucial for boards booting from NOR, as a board which
> support will be sent soon to u-boot ML :-).
> 
> Best regards,
> Łukasz Majewski
> 
>> I still miss it being
>> documented.

[...]

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Lukasz Majewski]

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.

Signed-off-by: Lukasz Majewski 
---
Changes for v2:
- Update to code to apply on v2016.11+
Changes for v3:
- Write better commit mesage to explain the problem

---
 common/spl/Kconfig   | 10 ++
 common/spl/spl_nor.c | 12 +---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index df9e0ce..d31b26d 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -399,6 +399,16 @@ config SPL_NOR_SUPPORT
  a memory-mapped device makes it very easy to access. Loading from
  NOR is typically achieved with just a memcpy().
 
+config SPL_NOR_COPY_ENTIRE_IMAGE
+   bool
+   depends on SPL_NOR_SUPPORT
+   prompt "Copy entire image from NOR memory"
+   default n
+   help
+ By default the SPL NOR driver supports copying only payload to
+ destination address. Say Y if you want to copy entire image (including
+ its image header).
+
 config SPL_ONENAND_SUPPORT
bool "Support OneNAND flash"
depends on SPL
diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
index 6bfa399..44f3e99 100644
--- a/common/spl/spl_nor.c
+++ b/common/spl/spl_nor.c
@@ -10,13 +10,15 @@
 static int spl_nor_load_image(struct spl_image_info *spl_image,
  struct spl_boot_device *bootdev)
 {
+   void *img_src;
int ret;
+#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
/*
 * Loading of the payload to SDRAM is done with skipping of
 * the mkimage header in this SPL NOR driver
 */
spl_image->flags |= SPL_COPY_PAYLOAD_ONLY;
-
+#endif
 #ifdef CONFIG_SPL_OS_BOOT
if (!spl_start_uboot()) {
const struct image_header *header;
@@ -65,9 +67,13 @@ static int spl_nor_load_image(struct spl_image_info 
*spl_image,
if (ret)
return ret;
 
+   img_src = (void *)CONFIG_SYS_UBOOT_BASE;
+#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
+   img_src += sizeof(struct image_header));
+#endif
+
memcpy((void *)(unsigned long)spl_image->load_addr,
-  (void *)(CONFIG_SYS_UBOOT_BASE + sizeof(struct image_header)),
-  spl_image->size);
+  img_src, spl_image->size);
 
return 0;
 }
-- 
2.1.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

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.

So, what's the usecase again ? ;-) I still miss it being documented.

> Signed-off-by: Lukasz Majewski 
> ---
> Changes for v2:
> - Update to code to apply on v2016.11+
> Changes for v3:
> - Write better commit mesage to explain the problem
> 
> ---
>  common/spl/Kconfig   | 10 ++
>  common/spl/spl_nor.c | 12 +---
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index df9e0ce..d31b26d 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -399,6 +399,16 @@ config SPL_NOR_SUPPORT
> a memory-mapped device makes it very easy to access. Loading from
> NOR is typically achieved with just a memcpy().
>  
> +config SPL_NOR_COPY_ENTIRE_IMAGE
> + bool
> + depends on SPL_NOR_SUPPORT
> + prompt "Copy entire image from NOR memory"
> + default n
> + help
> +   By default the SPL NOR driver supports copying only payload to
> +   destination address. Say Y if you want to copy entire image (including
> +   its image header).
> +
>  config SPL_ONENAND_SUPPORT
>   bool "Support OneNAND flash"
>   depends on SPL
> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> index 6bfa399..44f3e99 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -10,13 +10,15 @@
>  static int spl_nor_load_image(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev)
>  {
> + void *img_src;
>   int ret;
> +#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
>   /*
>* Loading of the payload to SDRAM is done with skipping of
>* the mkimage header in this SPL NOR driver
>*/
>   spl_image->flags |= SPL_COPY_PAYLOAD_ONLY;
> -
> +#endif
>  #ifdef CONFIG_SPL_OS_BOOT
>   if (!spl_start_uboot()) {
>   const struct image_header *header;
> @@ -65,9 +67,13 @@ static int spl_nor_load_image(struct spl_image_info 
> *spl_image,
>   if (ret)
>   return ret;
>  
> + img_src = (void *)CONFIG_SYS_UBOOT_BASE;
> +#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
> + img_src += sizeof(struct image_header));
> +#endif
> +
>   memcpy((void *)(unsigned long)spl_image->load_addr,
> -(void *)(CONFIG_SYS_UBOOT_BASE + sizeof(struct image_header)),
> -spl_image->size);
> +img_src, spl_image->size);
>  
>   return 0;
>  }
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

On 11/28/2016 05:50 PM, Tom Rini wrote:
> On Mon, Nov 28, 2016 at 01:43:44PM +0100, Marek Vasut wrote:
>> On 11/28/2016 07:27 AM, Lukasz Majewski wrote:
>>> This define gives the possibility to copy entire image (including header)
>>> from NOR parallel memory to e.g. SDRAM.
>>>
>>> The legacy behavior is preserved, since other board don't enabled this 
>>> option.
>>>
>>> Signed-off-by: Lukasz Majewski 
>>
>> What is the usecase ?
> 
> This is actually a v2, and the answer from v1 is at
> http://lists.denx.de/pipermail/u-boot/2016-September/267019.html and
> should be more prominent in the commit message / help text (so, v3
> please!).  Without this option u-boot.img doesn't work from NOR, only
> u-boot.bin

Ah, I recall this issue, thanks!

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Tom Rini]

On Mon, Nov 28, 2016 at 01:43:44PM +0100, Marek Vasut wrote:
> On 11/28/2016 07:27 AM, Lukasz Majewski wrote:
> > This define gives the possibility to copy entire image (including header)
> > from NOR parallel memory to e.g. SDRAM.
> > 
> > The legacy behavior is preserved, since other board don't enabled this 
> > option.
> > 
> > Signed-off-by: Lukasz Majewski 
> 
> What is the usecase ?

This is actually a v2, and the answer from v1 is at
http://lists.denx.de/pipermail/u-boot/2016-September/267019.html and
should be more prominent in the commit message / help text (so, v3
please!).  Without this option u-boot.img doesn't work from NOR, only
u-boot.bin

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Lukasz Majewski]

This define gives the possibility to copy entire image (including header)
from NOR parallel memory to e.g. SDRAM.

The legacy behavior is preserved, since other board don't enabled this option.

Signed-off-by: Lukasz Majewski 
---
 common/spl/Kconfig   | 10 ++
 common/spl/spl_nor.c | 12 +---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index df9e0ce..d31b26d 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -399,6 +399,16 @@ config SPL_NOR_SUPPORT
  a memory-mapped device makes it very easy to access. Loading from
  NOR is typically achieved with just a memcpy().
 
+config SPL_NOR_COPY_ENTIRE_IMAGE
+   bool
+   depends on SPL_NOR_SUPPORT
+   prompt "Copy entire image from NOR memory"
+   default n
+   help
+ By default the SPL NOR driver supports copying only payload to
+ destination address. Say Y if you want to copy entire image (including
+ its image header).
+
 config SPL_ONENAND_SUPPORT
bool "Support OneNAND flash"
depends on SPL
diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
index 6bfa399..44f3e99 100644
--- a/common/spl/spl_nor.c
+++ b/common/spl/spl_nor.c
@@ -10,13 +10,15 @@
 static int spl_nor_load_image(struct spl_image_info *spl_image,
  struct spl_boot_device *bootdev)
 {
+   void *img_src;
int ret;
+#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
/*
 * Loading of the payload to SDRAM is done with skipping of
 * the mkimage header in this SPL NOR driver
 */
spl_image->flags |= SPL_COPY_PAYLOAD_ONLY;
-
+#endif
 #ifdef CONFIG_SPL_OS_BOOT
if (!spl_start_uboot()) {
const struct image_header *header;
@@ -65,9 +67,13 @@ static int spl_nor_load_image(struct spl_image_info 
*spl_image,
if (ret)
return ret;
 
+   img_src = (void *)CONFIG_SYS_UBOOT_BASE;
+#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
+   img_src += sizeof(struct image_header));
+#endif
+
memcpy((void *)(unsigned long)spl_image->load_addr,
-  (void *)(CONFIG_SYS_UBOOT_BASE + sizeof(struct image_header)),
-  spl_image->size);
+  img_src, spl_image->size);
 
return 0;
 }
-- 
2.1.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

On 11/28/2016 07:27 AM, Lukasz Majewski wrote:
> This define gives the possibility to copy entire image (including header)
> from NOR parallel memory to e.g. SDRAM.
> 
> The legacy behavior is preserved, since other board don't enabled this option.
> 
> Signed-off-by: Lukasz Majewski 

What is the usecase ?

> ---
>  common/spl/Kconfig   | 10 ++
>  common/spl/spl_nor.c | 12 +---
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index df9e0ce..d31b26d 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -399,6 +399,16 @@ config SPL_NOR_SUPPORT
> a memory-mapped device makes it very easy to access. Loading from
> NOR is typically achieved with just a memcpy().
>  
> +config SPL_NOR_COPY_ENTIRE_IMAGE
> + bool
> + depends on SPL_NOR_SUPPORT
> + prompt "Copy entire image from NOR memory"
> + default n
> + help
> +   By default the SPL NOR driver supports copying only payload to
> +   destination address. Say Y if you want to copy entire image (including
> +   its image header).
> +
>  config SPL_ONENAND_SUPPORT
>   bool "Support OneNAND flash"
>   depends on SPL
> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> index 6bfa399..44f3e99 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -10,13 +10,15 @@
>  static int spl_nor_load_image(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev)
>  {
> + void *img_src;
>   int ret;
> +#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
>   /*
>* Loading of the payload to SDRAM is done with skipping of
>* the mkimage header in this SPL NOR driver
>*/
>   spl_image->flags |= SPL_COPY_PAYLOAD_ONLY;
> -
> +#endif
>  #ifdef CONFIG_SPL_OS_BOOT
>   if (!spl_start_uboot()) {
>   const struct image_header *header;
> @@ -65,9 +67,13 @@ static int spl_nor_load_image(struct spl_image_info 
> *spl_image,
>   if (ret)
>   return ret;
>  
> + img_src = (void *)CONFIG_SYS_UBOOT_BASE;
> +#ifndef CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
> + img_src += sizeof(struct image_header));
> +#endif
> +
>   memcpy((void *)(unsigned long)spl_image->load_addr,
> -(void *)(CONFIG_SYS_UBOOT_BASE + sizeof(struct image_header)),
> -spl_image->size);
> +img_src, spl_image->size);
>  
>   return 0;
>  }
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot