ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

PATCH In review Lukasz Majewski 4 participants 27 messages Sep 16, 2016 – Jan 2, 2017

lore

Share & tools

lore

Lukasz Majewski

author

2016-09-16 08:43 UTC #0 lore raw

PATCH 1/1 ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

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 <[email protected]>

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

Kconfig +10 -0

index e443490..473a099 100644
@@ -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

README +4 -0

index f41a6af..e9a3c5b 100644
@@ -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

common/spl/spl_nor.c +9 -3

index 8ea874c..1e0f739 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Tom Rini

2016-09-18 18:03 UTC #1 lore raw

PATCH 1/1 ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 4 lines · 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.

What's the use case?

Quoted 3 lines

Signed-off-by Lukasz Majewski <[email protected]>
---
 Kconfig              | 10 ++++++++++

This should now get moved to common/spl/Kconfig

Quoted 1 line

 README               |  4 ++++

This can be dropped.

[snip]

Quoted 22 lines

common/spl/spl_nor.c +3 -1

index 8ea874c..1e0f739 100644
@@ -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;

common/spl/spl_nor.c int spl_nor_load_image(void) · line 68

Quoted 3 lines

common/spl/spl_nor.c +0 -0

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

common/spl/spl_nor.c int spl_nor_load_image(void) · line 78

Quoted 12 lines

common/spl/spl_nor.c +6 -2

+	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!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160918/d1be3db9/attachment.sig>

Lukasz Majewski

author

2016-09-18 19:10 UTC #2 lore raw

PATCH 1/1 ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Hi Tom,

Quoted 9 lines · Lukasz Majewski

Quoted 6 lines
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.

Quoted 6 lines

Quoted 4 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
 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 :-)

Quoted 4 lines

Quoted 2 lines
 README               |  4 ++++
This can be dropped.

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

Quoted 22 lines

[snip]

Quoted 20 lines

common/spl/spl_nor.c +3 -1

index 8ea874c..1e0f739 100644
@@ -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()) {

Quoted 8 lines

common/spl/spl_nor.c int spl_nor_load_image(void) · line 68
Quoted 5 lines
common/spl/spl_nor.c +0 -0
 		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.

Quoted 12 lines

common/spl/spl_nor.c +6 -2

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

Quoted 5 lines

Quoted 2 lines
common/spl/spl_nor.c +0 -0
 	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.
*/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160918/377aa428/attachment.sig>

Tom Rini

2016-09-18 19:24 UTC #3 lore raw

PATCH 1/1 ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 12 lines · Lukasz Majewski

Hi Tom,
Quoted 9 lines · Lukasz Majewski
Quoted 6 lines
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?

Quoted 8 lines

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 ?

Quoted 6 lines

Quoted 5 lines
Quoted 3 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
 Kconfig              | 10 ++++++++++
This should now get moved to common/spl/Kconfig

Quoted 7 lines

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 :)

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
 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.

Quoted 30 lines

Quoted 21 lines

[snip]

Quoted 20 lines

common/spl/spl_nor.c +3 -1

index 8ea874c..1e0f739 100644
@@ -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()) {

Quoted 8 lines

common/spl/spl_nor.c int spl_nor_load_image(void) · line 68
Quoted 5 lines
common/spl/spl_nor.c +0 -0
 		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.

Quoted 3 lines

I will be more verbose with v2 :-).
Thanks for review.

No problem, thank you!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160918/3d12616e/attachment.sig>

Lukasz Majewski

author

2016-09-18 23:27 UTC #4 lore raw

PATCH 1/1 ML: SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Hi Tom,

Quoted 16 lines · Lukasz Majewski

Quoted 15 lines
Hi Tom,
Quoted 9 lines · Lukasz Majewski
Quoted 6 lines
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).

Quoted 8 lines

Quoted 5 lines
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.

Quoted 9 lines

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
 Kconfig              | 10 ++++++++++
This should now get moved to common/spl/Kconfig
OK.

Quoted 7 lines

Quoted 5 lines
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 :-)

Quoted 9 lines

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
 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.

Quoted 32 lines

Quoted 32 lines
Quoted 31 lines
[snip]
Quoted 23 lines
common/spl/spl_nor.c +2 -0
index 8ea874c..1e0f739 100644
@@ -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
common/spl/spl_nor.c +1 -1
 	 * 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;
common/spl/spl_nor.c int spl_nor_load_image(void) · line 68
Quoted 3 lines
common/spl/spl_nor.c +0 -0
@@ -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.

Quoted 6 lines

Quoted 3 lines
I will be more verbose with v2 :-).
Thanks for review.
No problem, thank you!

Best regards,
?ukasz Majewski

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160919/9ab05c8b/attachment.sig>

Tom Rini

2016-10-07 01:00 UTC #5 lore raw

In reply to Lukasz Majewski

Quoted 6 lines · 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 <[email protected]>

Please re-base this on top of master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161006/56aa2d15/attachment.sig>

Lukasz Majewski

author

2016-11-28 06:27 UTC #6 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

In reply to 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 <[email protected]>

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

common/spl/Kconfig +10 -0

index df9e0ce..d31b26d 100644
@@ -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

common/spl/spl_nor.c +9 -3

index 6bfa399..44f3e99 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Marek Vasut

2016-11-28 12:43 UTC #7 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 6 lines · 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 <[email protected]>

What is the usecase ?

Quoted 64 lines

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

common/spl/Kconfig +10 -0

index df9e0ce..d31b26d 100644
@@ -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

common/spl/spl_nor.c +9 -3

index 6bfa399..44f3e99 100644
@@ -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;
 }

Signature · 2 lines

Best regards,
Marek Vasut

Tom Rini

2016-11-28 16:50 UTC #8 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 9 lines · Marek Vasut

Quoted 6 lines · 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 <[email protected]>
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

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161128/7742be95/attachment.sig>

Marek Vasut

2016-11-28 17:17 UTC #9 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 9 lines · Tom Rini

Quoted 8 lines · Marek Vasut
Quoted 6 lines · 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 <[email protected]>

Quoted 7 lines

Quoted 1 line
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!

Signature · 2 lines

Best regards,
Marek Vasut

Lukasz Majewski

author

2016-11-28 21:09 UTC #10 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

In reply to 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 <[email protected]>

---
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(-)

common/spl/Kconfig +10 -0

index df9e0ce..d31b26d 100644
@@ -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

common/spl/spl_nor.c +9 -3

index 6bfa399..44f3e99 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Marek Vasut

2016-11-28 21:15 UTC #11 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 5 lines · 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.

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

Quoted 71 lines

Signed-off-by Lukasz Majewski <[email protected]>

---
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(-)

common/spl/Kconfig +10 -0

index df9e0ce..d31b26d 100644
@@ -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

common/spl/spl_nor.c +9 -3

index 6bfa399..44f3e99 100644
@@ -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;
 }

Signature · 2 lines

Best regards,
Marek Vasut

Lukasz Majewski

author

2016-11-29 09:11 UTC #12 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Hi Marek,

Quoted 10 lines · Lukasz Majewski

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

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

Quoted 74 lines

I still miss it being
documented.

Quoted 71 lines

Signed-off-by Lukasz Majewski <[email protected]>

---
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(-)

common/spl/Kconfig +0 -0

index df9e0ce..d31b26d 100644
@@ -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().

common/spl/Kconfig +7 -0

+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

common/spl/Kconfig +1 -0

+	  destination address. Say Y if you want to copy entire

image (including

common/spl/Kconfig +2 -0

+	  its image header).
+
 config SPL_ONENAND_SUPPORT
 	bool "Support OneNAND flash"
 	depends on SPL

common/spl/spl_nor.c +9 -3

index 6bfa399..44f3e99 100644
@@ -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);

Quoted 5 lines

Quoted 3 lines
common/spl/spl_nor.c +0 -0
 	return 0;
}

Marek Vasut

2016-11-29 10:50 UTC #13 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 11 lines · Lukasz Majewski

Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.

Quoted 6 lines

Quoted 1 line
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 ?

Quoted 8 lines

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
Quoted 2 lines
I still miss it being
documented.

[...]

Signature · 2 lines

Best regards,
Marek Vasut

Tom Rini

2016-11-29 18:18 UTC #14 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Lukasz Majewski
Hi Marek,
Quoted 10 lines · Lukasz Majewski
Quoted 7 lines
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 ? ;-)

Quoted 7 lines

Quoted 4 lines
:-)
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).

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161129/106dd542/attachment.sig>

Marek Vasut

2016-11-29 18:34 UTC #15 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 20 lines · Tom Rini

Quoted 19 lines · Marek Vasut
Quoted 17 lines · Lukasz Majewski
Hi Marek,
Quoted 10 lines · Lukasz Majewski
Quoted 7 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Signature · 2 lines

Best regards,
Marek Vasut

Lukasz Majewski

author

2016-12-26 16:36 UTC #16 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Hi Marek,

Quoted 24 lines · Tom Rini

Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?

Quoted 7 lines

Quoted 4 lines
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

Marek Vasut

2016-12-27 23:29 UTC #17 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 34 lines · Lukasz Majewski

Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 7 lines
Quoted 4 lines
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.

Quoted 7 lines

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.

Signature · 2 lines

Best regards,
Marek Vasut

Lukasz Majewski

author

2016-12-28 08:52 UTC #18 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Hi Marek,

Quoted 38 lines · Lukasz Majewski

Quoted 37 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 7 lines
Quoted 4 lines
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 2 lines

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

Marek Vasut

2016-12-28 23:41 UTC #19 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 40 lines · Lukasz Majewski

Hi Marek,
Quoted 38 lines · Lukasz Majewski
Quoted 37 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 7 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
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.

Quoted 10 lines

Quoted 2 lines
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).

Signature · 2 lines

Best regards,
Marek Vasut

Tom Rini

2016-12-29 15:26 UTC #20 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 61 lines · Marek Vasut

Quoted 40 lines · Lukasz Majewski
Hi Marek,
Quoted 38 lines · Lukasz Majewski
Quoted 37 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 7 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
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.
Quoted 9 lines
Quoted 2 lines
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).

Quoted 6 lines

Quoted 1 line
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!".

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161229/1621467d/attachment.sig>

Marek Vasut

2016-12-30 19:08 UTC #21 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 62 lines · Tom Rini

Quoted 61 lines · Marek Vasut
Quoted 40 lines · Lukasz Majewski
Hi Marek,
Quoted 38 lines · Lukasz Majewski
Quoted 37 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 7 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
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.
Quoted 9 lines
Quoted 2 lines
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).

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
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.

Quoted 3 lines

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 ?

Signature · 2 lines

Best regards,
Marek Vasut

Lukasz Majewski

author

2016-12-30 21:28 UTC #22 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Hi Marek,

Quoted 80 lines · Tom Rini

Quoted 64 lines · Marek Vasut
Quoted 63 lines · Lukasz Majewski
Quoted 44 lines
Hi Marek,
Quoted 42 lines · Lukasz Majewski
Quoted 41 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
Quoted 2 lines
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).
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
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!".

Quoted 5 lines

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161230/8d811192/attachment.sig>

Marek Vasut

2016-12-30 21:48 UTC #23 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 82 lines · Lukasz Majewski

Hi Marek,
Quoted 80 lines · Tom Rini
Quoted 64 lines · Marek Vasut
Quoted 63 lines · Lukasz Majewski
Quoted 44 lines
Hi Marek,
Quoted 42 lines · Lukasz Majewski
Quoted 41 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · Lukasz Majewski
Quoted 7 lines
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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
Quoted 2 lines
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).
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
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!".

Quoted 8 lines

Quoted 5 lines
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 ?

Quoted 1 line

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

Signature · 2 lines

Best regards,
Marek Vasut

Lukasz Majewski

author

2016-12-30 21:50 UTC #24 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Hi Marek,

Quoted 89 lines · Lukasz Majewski

Quoted 88 lines
Hi Marek,
Quoted 85 lines · Tom Rini
Quoted 64 lines · Marek Vasut
Quoted 63 lines · Lukasz Majewski
Quoted 44 lines
Hi Marek,
Quoted 42 lines · Lukasz Majewski
Quoted 41 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · 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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
Quoted 2 lines
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).
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
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 ?

Quoted 5 lines

Quoted 2 lines
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 :-)

Quoted 6 lines

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

Lukasz Majewski

2017-01-02 00:07 UTC #25 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

In reply to Marek Vasut

Hi Marek,

Quoted 89 lines · Lukasz Majewski

Quoted 88 lines
Hi Marek,
Quoted 85 lines · Tom Rini
Quoted 64 lines · Marek Vasut
Quoted 63 lines · Lukasz Majewski
Quoted 44 lines
Hi Marek,
Quoted 42 lines · Lukasz Majewski
Quoted 41 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 11 lines
Hi Marek,
Quoted 9 lines · 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.
Quoted 6 lines
Quoted 1 line
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 ?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
Quoted 2 lines
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).
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
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 ?

Quoted 5 lines

Quoted 2 lines
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 :-)

Quoted 6 lines

Quoted 3 lines
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

Marek Vasut

2017-01-02 18:57 UTC #26 lore raw

PATCH 1/1 SPL: NOR: Add CONFIG_SPL_NOR_COPY_ENTIRE_IMAGE define to enable whole image copy from NOR

Quoted 130 lines · Lukasz Majewski

Hi Marek,
Quoted 89 lines · Lukasz Majewski
Quoted 88 lines
Hi Marek,
Quoted 85 lines · Tom Rini
Quoted 64 lines · Marek Vasut
Quoted 63 lines · Lukasz Majewski
Quoted 44 lines
Hi Marek,
Quoted 42 lines · Lukasz Majewski
Quoted 41 lines
Hi Marek,
Quoted 24 lines · Tom Rini
Quoted 23 lines · Marek Vasut
Quoted 21 lines · Lukasz Majewski
Quoted 17 lines
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 ?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
Quoted 2 lines
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).
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
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 ?
Quoted 5 lines
Quoted 2 lines
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 :-)
--------------

Quoted 7 lines

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.

Quoted 1 line

Big thanks Marek to be vigilant :-)

You're welcome ;-)

Signature · 2 lines

Best regards,
Marek Vasut