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

[Lukasz Majewski]

Hi Tom,

> On Sun, Sep 18, 2016 at 09:10:34PM +0200, Lukasz Majewski wrote:
> > Hi Tom,
> > 
> > > On Fri, Sep 16, 2016 at 10:43:51AM +0200, 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.
> > > 
> > > What's the use case?
> > 
> > This code is used on some boards (ppc [1]) in which we copy only
> > payload of the image (i.e. we drop the package metadata).
> > 
> > In my particular case I would like to copy the image with its meta
> > data (in similar way works spl_nand.c and spl_mmc.c).
> > 
> > Since, I wanted to avoid code duplication, I've decided to add this
> > flag.
> 
> So another way of saying this, if I follow correctly, is that today
> SPL NOR doesn't work with u-boot.img but only u-boot.bin ?

Yes, exactly.

> 
> > > > Signed-off-by: Lukasz Majewski 
> > > > ---
> > > >  Kconfig  | 10 ++
> > > 
> > > This should now get moved to common/spl/Kconfig
> > 
> > OK.
> > 
> > As a side note - I do have some other patches which move some
> > SPL_NOR options to Kconfig.
> > 
> > However, I omitted them when I saw great work done by Simon to move
> > a lot of SPL options to Kconfig. I hope them will be merged soon :-)
> 
> It's in now :)

Great :-)

> 
> > > >  README   |  4 
> > > 
> > > This can be dropped.
> > 
> > So, we don't add any new entries to README and now rely solely on
> > Kconfig "help" sections?
> 
> Correct.

OK.

> 
> > > [snip]
> > > > diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> > > > index 8ea874c..1e0f739 100644
> > > > --- a/common/spl/spl_nor.c
> > > > +++ b/common/spl/spl_nor.c
> > > > @@ -9,13 +9,15 @@
> > > >  
> > > >  int spl_nor_load_image(void)
> > > >  {
> > > > +   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;
> > > > @@ -64,9 +66,13 @@ int spl_nor_load_image(void)
> > > > if (ret)
> > > > return ret;
> > > >  
> > > 
> > > We should have a comment here about how we're calculating the
> > > base of where to copy from.
> > 
> > I will be more verbose with v2 :-).
> > 
> > Thanks for review.
> 
> No problem, thank you!
> 

Best regards,
Łukasz Majewski



pgp3I2Gpoi5qu.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] ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

[Tom Rini]

On Sun, Sep 18, 2016 at 09:10:34PM +0200, Lukasz Majewski wrote:
> Hi Tom,
> 
> > On Fri, Sep 16, 2016 at 10:43:51AM +0200, 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.
> > 
> > What's the use case?
> 
> This code is used on some boards (ppc [1]) in which we copy only
> payload of the image (i.e. we drop the package metadata).
> 
> In my particular case I would like to copy the image with its meta
> data (in similar way works spl_nand.c and spl_mmc.c).
> 
> Since, I wanted to avoid code duplication, I've decided to add this
> flag.

So another way of saying this, if I follow correctly, is that today SPL
NOR doesn't work with u-boot.img but only u-boot.bin ?

> > > Signed-off-by: Lukasz Majewski 
> > > ---
> > >  Kconfig  | 10 ++
> > 
> > This should now get moved to common/spl/Kconfig
> 
> OK.
> 
> As a side note - I do have some other patches which move some SPL_NOR
> options to Kconfig.
> 
> However, I omitted them when I saw great work done by Simon to move a
> lot of SPL options to Kconfig. I hope them will be merged soon :-)

It's in now :)

> > >  README   |  4 
> > 
> > This can be dropped.
> 
> So, we don't add any new entries to README and now rely solely on
> Kconfig "help" sections?

Correct.

> > [snip]
> > > diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> > > index 8ea874c..1e0f739 100644
> > > --- a/common/spl/spl_nor.c
> > > +++ b/common/spl/spl_nor.c
> > > @@ -9,13 +9,15 @@
> > >  
> > >  int spl_nor_load_image(void)
> > >  {
> > > + 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;
> > > @@ -64,9 +66,13 @@ int spl_nor_load_image(void)
> > >   if (ret)
> > >   return ret;
> > >  
> > 
> > We should have a comment here about how we're calculating the base of
> > where to copy from.
> 
> I will be more verbose with v2 :-).
> 
> Thanks for review.

No problem, thank you!

-- 
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] ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

[Lukasz Majewski]

Hi Tom,

> On Fri, Sep 16, 2016 at 10:43:51AM +0200, 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.
> 
> What's the use case?

This code is used on some boards (ppc [1]) in which we copy only
payload of the image (i.e. we drop the package metadata).

In my particular case I would like to copy the image with its meta
data (in similar way works spl_nand.c and spl_mmc.c).

Since, I wanted to avoid code duplication, I've decided to add this
flag.

> 
> > Signed-off-by: Lukasz Majewski 
> > ---
> >  Kconfig  | 10 ++
> 
> This should now get moved to common/spl/Kconfig

OK.

As a side note - I do have some other patches which move some SPL_NOR
options to Kconfig.

However, I omitted them when I saw great work done by Simon to move a
lot of SPL options to Kconfig. I hope them will be merged soon :-)

> 
> >  README   |  4 
> 
> This can be dropped.

So, we don't add any new entries to README and now rely solely on
Kconfig "help" sections?

> 
> [snip]
> > diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> > index 8ea874c..1e0f739 100644
> > --- a/common/spl/spl_nor.c
> > +++ b/common/spl/spl_nor.c
> > @@ -9,13 +9,15 @@
> >  
> >  int spl_nor_load_image(void)
> >  {
> > +   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;
> > @@ -64,9 +66,13 @@ int spl_nor_load_image(void)
> > if (ret)
> > return ret;
> >  
> 
> We should have a comment here about how we're calculating the base of
> where to copy from.

I will be more verbose with v2 :-).

Thanks for review.

> 
> > +   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;
> >  }
> 
> Thanks!
> 

Best regards,
Łukasz Majewski


Notes:
[1] - Extract from source code (spl_nor.c):

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


pgpIRinR5zffw.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] ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

[Tom Rini]

On Fri, Sep 16, 2016 at 10:43:51AM +0200, 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.

What's the use case?

> Signed-off-by: Lukasz Majewski 
> ---
>  Kconfig  | 10 ++

This should now get moved to common/spl/Kconfig

>  README   |  4 

This can be dropped.

[snip]
> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> index 8ea874c..1e0f739 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -9,13 +9,15 @@
>  
>  int spl_nor_load_image(void)
>  {
> + 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;
> @@ -64,9 +66,13 @@ int spl_nor_load_image(void)
>   if (ret)
>   return ret;
>  

We should have a comment here about how we're calculating the base of
where to copy from.

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

Thanks!

-- 
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] ML: 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 
---
 Kconfig  | 10 ++
 README   |  4 
 common/spl/spl_nor.c | 12 +---
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Kconfig b/Kconfig
index e443490..473a099 100644
--- a/Kconfig
+++ b/Kconfig
@@ -159,6 +159,16 @@ config SPL_NOR_SUPPORT
help
  This option enables SPL NOR driver.
 
+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_SYS_MALLOC_SIMPLE
bool
depends on SPL
diff --git a/README b/README
index f41a6af..e9a3c5b 100644
--- a/README
+++ b/README
@@ -3661,6 +3661,10 @@ FIT uImage format:
It conflicts with SPL env from storage medium specified by
CONFIG_ENV_IS_xxx but CONFIG_ENV_IS_NOWHERE
 
+   CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE
+   Allow NOR stored u-boot image to be copied by SPL with header to
+   SDRAM memory and executed from it
+
CONFIG_SPL_PAD_TO
Image offset to which the SPL should be padded before appending
the SPL payload. By default, this is defined as
diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
index 8ea874c..1e0f739 100644
--- a/common/spl/spl_nor.c
+++ b/common/spl/spl_nor.c
@@ -9,13 +9,15 @@
 
 int spl_nor_load_image(void)
 {
+   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;
@@ -64,9 +66,13 @@ int spl_nor_load_image(void)
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