SPL: Port SPL framework to powerpc

SPL NOR flash booting support is quite simple. Only copying of the images is needed.

On MPC5xxx we need to make sure to only use the standard memcpy() implementation and not the MPC5xxx specific one. As the MPC5xxx version has some complexity which is not needed for this SPL booting.

Quoted 7 lines · Stefan Roese

SPL NOR flash booting support is quite simple. Only copying of the images is needed.

On MPC5xxx we need to make sure to only use the standard memcpy() implementation and not the MPC5xxx specific one. As the MPC5xxx version has some complexity which is not needed for this SPL booting.

I assume there's good reason to use SPL on NOR here? Is a near-future goal to do SPL-boots-Linux and thus the desire for a very small loader?

This patch enables the SPL framework to be used on powerpc platforms and not only ARM.

common/spl/spl.c line 48

Quoted 4 lines

common/spl/spl.c +2 -0

+#ifdef CONFIG_ARM
 /* Define global data structure pointer to it*/
 gd_t gdata __attribute__ ((section(".data")));
+#endif

So you handle cleaning up the BSS differently, interesting. I'm going to see if that would work for ARM too..

[snip]

common/spl/spl.c void spl_parse_image_header(const struct image_header *header) · line 113

Quoted 9 lines

common/spl/spl.c +4 -0

@@ -89,7 +106,11 @@ void spl_parse_image_header(const struct image_header *header)
 		spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
 		spl_image.entry_point = __be32_to_cpu(header->ih_load);
 		/* Load including the header */
+#ifdef CONFIG_ARM
 		spl_image.load_addr = spl_image.entry_point - header_size;
+#else
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+#endif

This isn't an ARM-ism but is instead because spl_nor.c isn't offsetting where the header is like mmc/nand/ymodem do, yes? Would it be possible to make spl_nor.c behave like the others? One of the reasons I ask is I'm looking at a NOR chip on my desk...

Quoted 7 lines · Tom Rini

common/spl/spl.c line 48

Quoted 4 lines

common/spl/spl.c +2 -0

+#ifdef CONFIG_ARM
 /* Define global data structure pointer to it*/
 gd_t gdata __attribute__ ((section(".data")));
+#endif

So you handle cleaning up the BSS differently, interesting. I'm going to see if that would work for ARM too..

Yes. Might be that I missed something though. I'll re-check tomorrow.

Quoted 15 lines

[snip]

common/spl/spl.c void spl_parse_image_header(const struct image_header *header) · line 113

Quoted 9 lines

common/spl/spl.c +4 -0

@@ -89,7 +106,11 @@ void spl_parse_image_header(const struct image_header *header)
 		spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
 		spl_image.entry_point = __be32_to_cpu(header->ih_load);
 		/* Load including the header */
+#ifdef CONFIG_ARM
 		spl_image.load_addr = spl_image.entry_point - header_size;
+#else
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+#endif

This isn't an ARM-ism but is instead because spl_nor.c isn't offsetting where the header is like mmc/nand/ymodem do, yes? Would it be possible to make spl_nor.c behave like the others? One of the reasons I ask is I'm looking at a NOR chip on my desk...

I was wondering about this line as well. Please explain: Why can't ARM just use header->ih_load as load_addr?

Quoted 15 lines

Quoted 14 lines

common/spl/spl.c void spl_parse_image_header(const struct image_header *header) · line 113

Quoted 9 lines

common/spl/spl.c +4 -0

@@ -89,7 +106,11 @@ void spl_parse_image_header(const struct image_header *header)
 		spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
 		spl_image.entry_point = __be32_to_cpu(header->ih_load);
 		/* Load including the header */
+#ifdef CONFIG_ARM
 		spl_image.load_addr = spl_image.entry_point - header_size;
+#else
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+#endif

This isn't an ARM-ism but is instead because spl_nor.c isn't offsetting where the header is like mmc/nand/ymodem do, yes? Would it be possible to make spl_nor.c behave like the others? One of the reasons I ask is I'm looking at a NOR chip on my desk...

Quoted 2 lines

I was wondering about this line as well. Please explain: Why can't ARM just use header->ih_load as load_addr?

Off the top of my head, I believe what goes on is that we read things into SDRAM such that the header is taken into account and we don't need to relocate the payload (U-Boot or Linux).

Hi Tom,

Quoted 15 lines · Tom Rini

Quoted 15 lines

Quoted 14 lines

common/spl/spl.c void spl_parse_image_header(const struct image_header *header) · line 113

Quoted 9 lines

common/spl/spl.c +4 -0

@@ -89,7 +106,11 @@ void spl_parse_image_header(const struct image_header *header)
  		spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
  		spl_image.entry_point = __be32_to_cpu(header->ih_load);
  		/* Load including the header */
+#ifdef CONFIG_ARM
  		spl_image.load_addr = spl_image.entry_point - header_size;
+#else
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+#endif

This isn't an ARM-ism but is instead because spl_nor.c isn't offsetting where the header is like mmc/nand/ymodem do, yes? Would it be possible to make spl_nor.c behave like the others? One of the reasons I ask is I'm looking at a NOR chip on my desk...

Quoted 6 lines

Quoted 2 lines

I was wondering about this line as well. Please explain: Why can't ARM just use header->ih_load as load_addr?

Off the top of my head, I believe what goes on is that we read things into SDRAM such that the header is taken into account and we don't need to relocate the payload (U-Boot or Linux).

Hmmm. So for example, when ih_load is set to 0x100000, then you load the image to (0x100000 - 0x40) = 0xfffc0? Is this correct?

This can't work for powerpc. As here for Linux both load-address and entry-point are set to 0. So when loading the image (e.g. from NOR flash) can't copy the image header in front of the image.

Another thing I'm wondering about: Why is only ih_load from the mkimage header used and not ih_ep (entry-point)?

I suggest that we switch to copying the "real" image (payload) to the load address, skipping the header. Then "ih_load" and "ih_ep" can be used without modification.

BTW: There also seems to be a bug in some of the SPL loaders:

For example in drivers/mtd/nand/nand_spl_load.c:

		...
		if (header->ih_os == IH_OS_LINUX) {
			/* happy - was a linux */
			nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
				spl_image.size, (void *)spl_image.load_addr);

The problem here is that the last 64 bytes of the image are not copied to SDRAM. Since the header is copied which is not included in the spl_image.size variable. With my idea of only copying the payload (skipping the header) this would be:

			nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS +
				sizeof(struct image_header),
				spl_image.size, (void *)spl_image.load_addr);

What do you think? Should we switch to this way of loading images? Seems more logical to me. And we don't run into problems where the load address is 0.

Quoted 10 lines

BTW: There also seems to be a bug in some of the SPL loaders:

For example in drivers/mtd/nand/nand_spl_load.c:

		...
		if (header->ih_os == IH_OS_LINUX) {
			/* happy - was a linux */
			nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
				spl_image.size, (void *)spl_image.load_addr);

Quoted 3 lines

The problem here is that the last 64 bytes of the image are not copied to SDRAM. Since the header is copied which is not included in the spl_image.size variable.

I still would like to move to my suggestion to not copy the header and use the mkimage header values ih_load and ih_ep directly. Right now I don't see any showstopper for doing it this way. I'll send a patch to change this shortly (if everything works out).

Quoted 4 lines

I still would like to move to my suggestion to not copy the header and use the mkimage header values ih_load and ih_ep directly. Right now I don't see any showstopper for doing it this way. I'll send a patch to change this shortly (if everything works out).

Hmmm. As it seems some SPL loading drivers (block like mmc, streaming like ymodem) are not that easily converted to skipping the header. So I'm not so sure if we should go this way after all...

Comments?

Quoted 9 lines

Quoted 4 lines

I still would like to move to my suggestion to not copy the header and use the mkimage header values ih_load and ih_ep directly. Right now I don't see any showstopper for doing it this way. I'll send a patch to change this shortly (if everything works out).

Hmmm. As it seems some SPL loading drivers (block like mmc, streaming like ymodem) are not that easily converted to skipping the header. So I'm not so sure if we should go this way after all...

Quoted 1 line

Comments?

I did similar work for my upcoming Lantiq MIPS SoC port. In my approach I also support compressed u-boot
images as payload (LZO, LZMA).

The u-boot image is merged with SPL
image without any padding or
fixed flash offsets to achieve a maximum reduction of flash footprint.

I ended up with doing something like this:

static int spl_parse_image(const image_header_t *hdr, struct spl_image *spl)

{
       u32 magic;

       magic = image_get_magic(hdr);
       if (magic != IH_MAGIC)
               return -1;

       spl->data_addr += image_get_header_size();
       spl->entry_addr = image_get_load(hdr);
       spl->size = image_get_data_size(hdr);
       spl->comp = image_get_comp(hdr);

       if (spl->comp == IH_COMP_NONE)
               spl->load_addr = spl->entry_addr;
       else
               spl->load_addr = CONFIG_LOADADDR;

       return 0;
}

spl->data_addr points to the image header of the payload and is initialized by the caller
dependent on the used load mechanism.

If the payload is uncompressed
it can be directly copied to
its final RAM location. A compressed payload needs an intermediate copy step if it is stored in SPI or NAND flash.
I chose CONFIG_LOADADDR. After this the decompression function extracts the image to its final RAM location.

I think we should keep an architecture-neutral SPL image context inside the generic SPL framework.
The content of this context should be filled by architecture/SoC/board specific code.

Hi Daniel,

Quoted 9 lines · Daniel Schwierzeck

Quoted 9 lines

Quoted 4 lines

I still would like to move to my suggestion to not copy the header and use the mkimage header values ih_load and ih_ep directly. Right now I don't see any showstopper for doing it this way. I'll send a patch to change this shortly (if everything works out).

Hmmm. As it seems some SPL loading drivers (block like mmc, streaming like ymodem) are not that easily converted to skipping the header. So I'm not so sure if we should go this way after all...

Quoted 5 lines

Quoted 1 line

Comments?

I did similar work for my upcoming Lantiq MIPS SoC port. In my approach I also support compressed u-boot
images as payload (LZO, LZMA).

Yes, I thought about adding this as well. Nice.

Quoted 3 lines

The u-boot image is merged with SPL
image without any padding or
fixed flash offsets to achieve a maximum reduction of flash footprint.

Interesting. I'm still padding to the fixed offset. Let me look into squeezing those two images together as well...

Quoted 20 lines

I ended up with doing something like this:

static int spl_parse_image(const image_header_t *hdr, struct spl_image *spl)

{
       u32 magic;

       magic = image_get_magic(hdr);
       if (magic != IH_MAGIC)
               return -1;

       spl->data_addr += image_get_header_size();
       spl->entry_addr = image_get_load(hdr);
       spl->size = image_get_data_size(hdr);
       spl->comp = image_get_comp(hdr);

       if (spl->comp == IH_COMP_NONE)
               spl->load_addr = spl->entry_addr;
       else
               spl->load_addr = CONFIG_LOADADDR;

Quoted 2 lines

       return 0;
}

So you introduced a new "struct spl_image" and did not use "struct spl_image_info" from the SPL framework? Why is that? We should definitely consolidate to one structure here.

Quoted 4 lines

spl->data_addr points to the image header of the payload and is initialized by the caller
dependent on the used load mechanism.

Good idea. Looks like your spl_parse_image might be a potential replacement for spl_parse_image_header() from this common SPL framework.

Quoted 6 lines

If the payload is uncompressed
it can be directly copied to
its final RAM location. A compressed payload needs an intermediate copy step if it is stored in SPI or NAND flash.
I chose CONFIG_LOADADDR. After this the decompression function extracts the image to its final RAM location.

Sounds good. Do you have some patches that you could send for review? Did you try to keep as much platform/arch independent as possible?

Quoted 5 lines

I think we should keep an architecture-neutral SPL image context inside the generic SPL framework.
The content of this context should be filled by architecture/SoC/board specific code.

Ack. It would be great to review your code here. Please keep us informed.

I still would like to move to my suggestion to not copy the header and use the mkimage header values ih_load and ih_ep directly. Right now I don't see any showstopper for doing it this way. I'll send a patch to change this shortly (if everything works out).

Quoted 3 lines

The u-boot image is merged with SPL
image without any padding or
fixed flash offsets to achieve a maximum reduction of flash footprint.

Interesting. I'm still padding to the fixed offset. Let me look into squeezing those two images together as well...

Hi Daniel,

Quoted 4 lines · Stefan Roese

Quoted 3 lines

The u-boot image is merged with SPL
image without any padding or
fixed flash offsets to achieve a maximum reduction of flash footprint.

Quoted 2 lines

Interesting. I'm still padding to the fixed offset. Let me look into squeezing those two images together as well...

Looks good. One question though: How do you make sure, that your SPL image length is 4-byte aligned?

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

The u-boot image is merged with SPL
image without any padding or
fixed flash offsets to achieve a maximum reduction of flash footprint.

Interesting. I'm still padding to the fixed offset. Let me look into squeezing those two images together as well...

Quoted 2 lines

Looks good. One question though: How do you make sure, that your SPL image length is 4-byte aligned?

I have 4-byte alignments for each section and for each address symbol in my linker script.
Actually all MIPS linker scripts do this.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

The u-boot image is merged with SPL
image without any padding or
fixed flash offsets to achieve a maximum reduction of flash footprint.

Interesting. I'm still padding to the fixed offset. Let me look into squeezing those two images together as well...

Quoted 6 lines

Quoted 2 lines

Looks good. One question though: How do you make sure, that your SPL image length is 4-byte aligned?

I have 4-byte alignments for each section and for each address symbol in my linker script.
Actually all MIPS linker scripts do this.

Yes. I have those section alignments in the linker script as well. The problem is the end of the last (physical end in flash) section. If I have here a string with length 7 for example, the SPL binary has a non-4-byte aligned length.

Any ideas on this?

Quoted 4 lines · Stefan Roese

I still would like to move to my suggestion to not copy the header and use the mkimage header values ih_load and ih_ep directly. Right now I don't see any showstopper for doing it this way. I'll send a patch to change this shortly (if everything works out).

Hmmm. As it seems some SPL loading drivers (block like mmc, streaming like ymodem) are not that easily converted to skipping the header. So I'm not so sure if we should go this way after all...

Quoted 5 lines

Quoted 4 lines

I still would like to move to my suggestion to not copy the header and use the mkimage header values ih_load and ih_ep directly. Right now I don't see any showstopper for doing it this way. I'll send a patch to change this shortly (if everything works out).

Quoted 3 lines

Hmmm. As it seems some SPL loading drivers (block like mmc, streaming like ymodem) are not that easily converted to skipping the header. So I'm not so sure if we should go this way after all...

Maybe I'm missing something, but maybe we just need to mimic the behavior full U-Boot does and if we haven't been loaded where we need to execute, shift bits around?

This patch enables the SPL framework to be used on powerpc platforms and not only ARM.

Quoted 2 lines · Stefan Roese

This patch enables the SPL framework to be used on powerpc platforms and not only ARM.

[snip]

common/spl/spl.c void board_init_r(gd_t *id, ulong dummy) · line 255

Quoted 3 lines

common/spl/spl.c +2 -0

+#ifdef CONFIG_ARM
 	gd = &gdata;
+#endif

So, here's what I don't understand. On ARM, in general, we can't rely on the global data pointer register (r8) to be set to a useful value, so we do the above to ensure it points to something useful. Are you always able to rely on r2 it looks like pointing to something useful? Or do you take care of this much earlier on in powerpc? Thanks!

Hi Tom,

Quoted 5 lines · Tom Rini

Quoted 2 lines · Stefan Roese

This patch enables the SPL framework to be used on powerpc platforms and not only ARM.

[snip]

Quoted 9 lines

common/spl/spl.c void board_init_r(gd_t *id, ulong dummy) · line 255

Quoted 3 lines

common/spl/spl.c +2 -0

+#ifdef CONFIG_ARM
 	gd = &gdata;
+#endif

So, here's what I don't understand. On ARM, in general, we can't rely on the global data pointer register (r8) to be set to a useful value, so we do the above to ensure it points to something useful. Are you always able to rely on r2 it looks like pointing to something useful? Or do you take care of this much earlier on in powerpc? Thanks!

You are correct, I missed something here. r2 was still configured to the value written to it from the "real" U-Boot (pointing to internal SRAM).

I can't use the code in preloader_console_init() though to setup the gd pointer. As I need to write some values in gd *before* calling preloader_console_init() (mainly clocks for serial driver). And since this "gd stuff" is quite platform specific, we should probably move this into an platform/arch spl file instead. As you also mentioned in another reply to create an arch/${ARCH}/lib/spl.c file.

What do you think? Can you move this gd init stuff into such a common ARM spl file in the next patchset version?

Quoted 16 lines

Quoted 5 lines

So, here's what I don't understand. On ARM, in general, we can't rely on the global data pointer register (r8) to be set to a useful value, so we do the above to ensure it points to something useful. Are you always able to rely on r2 it looks like pointing to something useful? Or do you take care of this much earlier on in powerpc? Thanks!

You are correct, I missed something here. r2 was still configured to the value written to it from the "real" U-Boot (pointing to internal SRAM).

I can't use the code in preloader_console_init() though to setup the gd pointer. As I need to write some values in gd *before* calling preloader_console_init() (mainly clocks for serial driver). And since this "gd stuff" is quite platform specific, we should probably move this into an platform/arch spl file instead. As you also mentioned in another reply to create an arch/${ARCH}/lib/spl.c file.

Quoted 2 lines

What do you think? Can you move this gd init stuff into such a common ARM spl file in the next patchset version?

Yes. We should probably say it's the job of board_init_f. I know full U-Boot does it at the start of board_init_r but I think for SPL it makes sense to document that board_init_f does any early init it needs to, clears BSS, sets up gd and calls board_init_r.

Quoted 19 lines · Tom Rini

Quoted 18 lines

Quoted 5 lines

So, here's what I don't understand. On ARM, in general, we can't rely on the global data pointer register (r8) to be set to a useful value, so we do the above to ensure it points to something useful. Are you always able to rely on r2 it looks like pointing to something useful? Or do you take care of this much earlier on in powerpc? Thanks!

You are correct, I missed something here. r2 was still configured to the value written to it from the "real" U-Boot (pointing to internal SRAM).

I can't use the code in preloader_console_init() though to setup the gd pointer. As I need to write some values in gd *before* calling preloader_console_init() (mainly clocks for serial driver). And since this "gd stuff" is quite platform specific, we should probably move this into an platform/arch spl file instead. As you also mentioned in another reply to create an arch/${ARCH}/lib/spl.c file.

What do you think? Can you move this gd init stuff into such a common ARM spl file in the next patchset version?

Quoted 4 lines

Yes. We should probably say it's the job of board_init_f. I know full U-Boot does it at the start of board_init_r but I think for SPL it makes sense to document that board_init_f does any early init it needs to, clears BSS, sets up gd and calls board_init_r.

Full ack. In my next patchset version, I already extracted this gd setup into board_init_f().

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

What do you think? Can you move this gd init stuff into such a common ARM spl file in the next patchset version?

Yes. We should probably say it's the job of board_init_f. I know full U-Boot does it at the start of board_init_r but I think for SPL it makes sense to document that board_init_f does any early init it needs to, clears BSS, sets up gd and calls board_init_r.

Quoted 2 lines

Full ack. In my next patchset version, I already extracted this gd setup into board_init_f().

OK good, I just booted and reset my arm9 platform with this proof-of-concepted over. I should post v4 today.

This patch enables the SPL framework to be used on powerpc platforms and not only ARM.

common/spl/spl.c +4 -0

+#ifdef CONFIG_PPC
+static void __noreturn jump_to_image_linux(void *arg)
+{
+	debug("Entering kernel arg pointer: 0x%p\n", arg);

Quoted 2 lines · Stefan Roese

This patch enables the SPL framework to be used on powerpc platforms and not only ARM.

[snip]

Quoted 10 lines

common/spl/spl.c +10 -0

+#ifdef CONFIG_PPC
+static void __noreturn jump_to_image_linux(void *arg)
+{
+	debug("Entering kernel arg pointer: 0x%p\n", arg);
+	typedef void (*image_entry_arg_t)(void *, ulong r4, ulong r5, ulong r6,
+					  ulong r7, ulong r8, ulong r9)
+		__attribute__ ((noreturn));
+	image_entry_arg_t image_entry =
+		(image_entry_arg_t)spl_image.entry_point;
+

common/spl/spl.c static void __noreturn jump_to_image_linux(void *arg) · line 164

Quoted 4 lines

common/spl/spl.c +4 -0

+	image_entry(arg, 0, 0, EPAPR_MAGIC, CONFIG_SYS_BOOTMAPSZ, 0, 0);
+}
+#endif /* CONFIG_PPC */
+#endif /* CONFIG_SPL_OS_BOOT */

This, along with board_init_f make me wonder if we shouldn't have an arch/${ARCH}/lib/spl.c that contains them and make them non-static. Perhaps sharing with the non-SPL code portion as well?

This patchset ports the SPL framework to powerpc. Its based on the SPL generalization lately done by Tom Rini. The patches can be applied on top of his 3rd version located here:

http://github.com/trini/u-boot WIP/spl-improvements

Additionally, a new MPC5200 board port is included, the a3m071 board port. This board port uses this SPL framework mainly to speed up booting into the OS (Linux of course). Detection of Linux vs. U-Boot booting is done here by checking the environment variable "boot_os". If "boot_os" is set to "yes", then the OS (Linux) is booted. Otherwise the "real" U-Boot is booted. For this env checking in the SPL, a small restructuring of the env code has been done.

Comments welcome!