ARM: Add Altera SOCFPGA Cyclone5

From: Dinh Nguyen <[email protected]>

Add minimal support for Altera's SOCFPGA Cyclone 5 hardware.

From: Dinh Nguyen <[email protected]>

Add minimal support for Altera's SOCFPGA Cyclone 5 hardware.

Signed-off-by Dinh Nguyen <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Makefile +1 -1

index 5ce5cc3..12aa372 100644
@@ -369,7 +369,7 @@ BOARD_SIZE_CHECK =
 endif
 
 # Always append ALL so that arch config.mk's can add custom ones
-ALL-y += $(obj)u-boot.srec $(obj)u-boot.bin $(obj)System.map
+ALL-y += $(obj)u-boot.srec $(obj)u-boot.bin $(obj)u-boot.img

$(obj)System.map

ALL-$(CONFIG_NAND_U_BOOT) += $(obj)u-boot-nand.bin ALL-$(CONFIG_ONENAND_U_BOOT) += $(obj)u-boot-onenand.bin

Makefile +10 -0

+/*
+ * Write the reset manager register to cause reset
+ */
+void reset_cpu(ulong addr)
+{
+	/* request a warm reset */
+	writel(RSTMGR_CTRL_SWWARMRSTREQ_LSB, &reset_manager_base->ctrl);
+	/* infinite loop here as watchdog will trigger and reset
+	 * the processor */
+	while (1)

Makefile +1 -0

+}

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/sdram.c new file mode 100644 index 0000000..6714983

arch/arm/cpu/armv7/socfpga/sdram.c new file +16 -0

@@ -0,0 +1,26 @@
+/*
+ *  Copyright (C) 2012 Altera Corporation <www.altera.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/spl.c new file mode 100644 index 0000000..7bd1e2f

arch/arm/cpu/armv7/socfpga/spl.c new file +52 -0

@@ -0,0 +1,182 @@
+ /*
+ *  Copyright (C) 2012 Altera Corporation <www.altera.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/u-boot.h>
+#include <asm/utils.h>
+#include <asm/arch/socfpga_spl.h>
+#include <version.h>
+#include <image.h>
+#include <malloc.h>
+#include <asm/arch/reset_manager.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static u32 *boot_params_ptr;
+static struct spl_image_info spl_image;
+
+extern void __malloc_start, __malloc_end, __stack_start;
+
+/* Define global data structure pointer to it */
+static gd_t gdata __attribute__ ((section(".data")));
+
+/*
+ * Error action
+ */
+inline void hang(void)
+{
+	puts("### ERROR ### Please RESET the board ###\n");
+	for (;;)
+		;
+}
+
+/*
+ * Read the mkimage signature to extract info such as entry point.
+ * The header is generated by tools/mkimage
+ */
+static unsigned long spl_parse_image_header(const struct image_header

*header) +{

arch/arm/cpu/armv7/socfpga/spl.c new file +22 -0

+	/* checking for mkimage signature */
+	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of header */
+		spl_image.size = __be32_to_cpu(header->ih_size);
+		spl_image.entry_point = __be32_to_cpu(header->ih_load);
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+		spl_image.os = header->ih_os;
+		spl_image.name = (const char *)&header->ih_name;
+		spl_image.crc = __be32_to_cpu(header->ih_dcrc);
+		debug("Subsequent boot image info\n");
+		debug("   Image Name:   %.*s\n", IH_NMLEN, spl_image.name);
+		debug("   Data Size:    %d\n", spl_image.size);
+		debug("   Load Address: 0x%x\n", spl_image.load_addr);
+		debug("   Entry Point:  0x%X\n", spl_image.entry_point);
+	} else {
+		/* Not a valid image as mkimage signature not found */
+		printf("Error : mkimage signature not found - ih_magic = %x\n",
+			header->ih_magic);
+		return 1;
+	}
+
+	/* Checking image type. Do any customize stuff per image type here */

Dear dinguyen at altera.com,

Quoted 6 lines

From: Dinh Nguyen <[email protected]>

Add minimal support for Altera's SOCFPGA Cyclone 5 hardware.

Signed-off-by Dinh Nguyen <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

[...]

Please CC albert with new arches.

Quoted 7 lines

Makefile +0 -0

index 5ce5cc3..12aa372 100644
@@ -369,7 +369,7 @@ BOARD_SIZE_CHECK =
 endif
 

Quoted 4 lines

Makefile +1 -1

 # Always append ALL so that arch config.mk's can add custom ones
-ALL-y += $(obj)u-boot.srec $(obj)u-boot.bin $(obj)System.map
+ALL-y += $(obj)u-boot.srec $(obj)u-boot.bin $(obj)u-boot.img

$(obj)System.map

This won't slide

Quoted 85 lines

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/spl.c new file mode 100644 index 0000000..7bd1e2f

arch/arm/cpu/armv7/socfpga/spl.c new file +52 -0

@@ -0,0 +1,182 @@
+ /*
+ *  Copyright (C) 2012 Altera Corporation <www.altera.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/u-boot.h>
+#include <asm/utils.h>
+#include <asm/arch/socfpga_spl.h>
+#include <version.h>
+#include <image.h>
+#include <malloc.h>
+#include <asm/arch/reset_manager.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static u32 *boot_params_ptr;
+static struct spl_image_info spl_image;
+
+extern void __malloc_start, __malloc_end, __stack_start;
+
+/* Define global data structure pointer to it */
+static gd_t gdata __attribute__ ((section(".data")));
+
+/*
+ * Error action
+ */
+inline void hang(void)
+{
+	puts("### ERROR ### Please RESET the board ###\n");
+	for (;;)
+		;
+}
+
+/*
+ * Read the mkimage signature to extract info such as entry point.
+ * The header is generated by tools/mkimage
+ */
+static unsigned long spl_parse_image_header(const struct image_header

*header) +{

arch/arm/cpu/armv7/socfpga/spl.c new file +26 -0

+	/* checking for mkimage signature */
+	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of header */
+		spl_image.size = __be32_to_cpu(header->ih_size);
+		spl_image.entry_point = __be32_to_cpu(header->ih_load);
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+		spl_image.os = header->ih_os;
+		spl_image.name = (const char *)&header->ih_name;
+		spl_image.crc = __be32_to_cpu(header->ih_dcrc);
+		debug("Subsequent boot image info\n");
+		debug("   Image Name:   %.*s\n", IH_NMLEN, spl_image.name);
+		debug("   Data Size:    %d\n", spl_image.size);
+		debug("   Load Address: 0x%x\n", spl_image.load_addr);
+		debug("   Entry Point:  0x%X\n", spl_image.entry_point);
+	} else {
+		/* Not a valid image as mkimage signature not found */
+		printf("Error : mkimage signature not found - ih_magic = %x\n",
+			header->ih_magic);
+		return 1;
+	}
+
+	/* Checking image type. Do any customize stuff per image type here */
+	switch (spl_image.os) {
+	case IH_OS_U_BOOT:
+		debug("   Image Type:   U-Boot\n");
+		break;

arch/arm/cpu/armv7/socfpga/spl.c line 85

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/spl.c new file +6 -0

+	default:
+		printf("   Image Type:   Unknown (%d)\n", spl_image.os);
+		;
+	}
+	return 0;
+}

I think I don't even wanna know what's the purpose here :-)

Quoted 7 lines

Quoted 7 lines

Makefile +0 -0

index 5ce5cc3..12aa372 100644
@@ -369,7 +369,7 @@ BOARD_SIZE_CHECK =
 endif
 

Quoted 6 lines

Quoted 4 lines

Makefile +1 -1

 # Always append ALL so that arch config.mk's can add custom ones
-ALL-y += $(obj)u-boot.srec $(obj)u-boot.bin $(obj)System.map
+ALL-y += $(obj)u-boot.srec $(obj)u-boot.bin $(obj)u-boot.img

$(obj)System.map

This won't slide

You can place this into arch/arm/cpu/armv7/socfpga/config.mk (see am33xx/config.mk or any of the others) for examples.

Quoted 28 lines

Quoted 28 lines

+static unsigned long spl_parse_image_header(const struct image_header *header) +{

Makefile +26 -0

+	/* checking for mkimage signature */
+	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of header */
+		spl_image.size = __be32_to_cpu(header->ih_size);
+		spl_image.entry_point = __be32_to_cpu(header->ih_load);
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+		spl_image.os = header->ih_os;
+		spl_image.name = (const char *)&header->ih_name;
+		spl_image.crc = __be32_to_cpu(header->ih_dcrc);
+		debug("Subsequent boot image info\n");
+		debug("   Image Name:   %.*s\n", IH_NMLEN, spl_image.name);
+		debug("   Data Size:    %d\n", spl_image.size);
+		debug("   Load Address: 0x%x\n", spl_image.load_addr);
+		debug("   Entry Point:  0x%X\n", spl_image.entry_point);
+	} else {
+		/* Not a valid image as mkimage signature not found */
+		printf("Error : mkimage signature not found - ih_magic = %x\n",
+			header->ih_magic);
+		return 1;
+	}
+
+	/* Checking image type. Do any customize stuff per image type here */
+	switch (spl_image.os) {
+	case IH_OS_U_BOOT:
+		debug("   Image Type:   U-Boot\n");
+		break;

Quoted 8 lines

Makefile

Quoted 6 lines

Makefile +6 -0

+	default:
+		printf("   Image Type:   Unknown (%d)\n", spl_image.os);
+		;
+	}
+	return 0;
+}

I think I don't even wanna know what's the purpose here :-)

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

Quoted 37 lines

Quoted 36 lines

Quoted 24 lines

+static unsigned long spl_parse_image_header(const struct image_header *header) +{

arch/arm/cpu/armv7/socfpga/spl.c new file +22 -0

+	/* checking for mkimage signature */
+	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of header */
+		spl_image.size = __be32_to_cpu(header->ih_size);
+		spl_image.entry_point = __be32_to_cpu(header->ih_load);
+		spl_image.load_addr = __be32_to_cpu(header->ih_load);
+		spl_image.os = header->ih_os;
+		spl_image.name = (const char *)&header->ih_name;
+		spl_image.crc = __be32_to_cpu(header->ih_dcrc);
+		debug("Subsequent boot image info\n");
+		debug("   Image Name:   %.*s\n", IH_NMLEN, spl_image.name);
+		debug("   Data Size:    %d\n", spl_image.size);
+		debug("   Load Address: 0x%x\n", spl_image.load_addr);
+		debug("   Entry Point:  0x%X\n", spl_image.entry_point);
+	} else {
+		/* Not a valid image as mkimage signature not found */
+		printf("Error : mkimage signature not found - ih_magic = %x\n",
+			header->ih_magic);
+		return 1;
+	}
+
+	/* Checking image type. Do any customize stuff per image type here */

arch/arm/cpu/armv7/socfpga/spl.c line 85

Quoted 10 lines

arch/arm/cpu/armv7/socfpga/spl.c new file +10 -0

+	switch (spl_image.os) {
+	case IH_OS_U_BOOT:
+		debug("   Image Type:   U-Boot\n");
+		break;
+	default:
+		printf("   Image Type:   Unknown (%d)\n", spl_image.os);
+		;
+	}
+	return 0;
+}

I think I don't even wanna know what's the purpose here :-)

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

Quoted 2 lines

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

Yes, that might happen. But we learned in the past that if we don't try to do such code consolidations right from the beginning, it won't happen at all (most of the time). So I'm also in favor of using the common SPL framework now. Please take a look at Tom Rini's v3 series:

Hi!

Quoted 7 lines

Quoted 2 lines

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

Yes, that might happen. But we learned in the past that if we don't try to do such code consolidations right from the beginning, it won't happen at all (most of the time). So I'm also in favor of using the common SPL framework now. Please take a look at Tom Rini's v3 series:

No problem, it really looks much better on top of Tom's patches.

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

[snip]

Quoted 6 lines

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

My hope is that since I got the SPL patches posted in time we can get them in for v2012.10 so no, this will only help your chances :)

On 08/23/2012 03:43 AM, Pavel Machek wrote:

[snip]

Quoted 4 lines

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

Quoted 2 lines

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

My hope is that since I got the SPL patches posted in time we can get them in for v2012.10 so no, this will only help your chances :)

[snip]

Quoted 6 lines

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

My hope is that since I got the SPL patches posted in time we can get them in for v2012.10 so no, this will only help your chances :)

Hi!

Quoted 10 lines

[snip]

Quoted 6 lines

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

My hope is that since I got the SPL patches posted in time we can get them in for v2012.10 so no, this will only help your chances :)

Hopefully.

Some observations:

Not sure what to do with CONFIG_SPL_STACK: altera was just using symbol from linker script. Is there way to keep it like that? I'd hate to invent magic constant.

Same issue with CONFIG_SYS_SPL_MALLOC_START... I was doing:

        /* setup MALLOC after clocks going faster */
	mem_malloc_init((ulong) &__malloc_start,
					(&__malloc_end -
	&__malloc_start));

I guess defines allow me to keep my definition. Good.

spl_parse_image_header(): We have "modern" system, we'd prefer not to do

                /* Signature not found - assume u-boot.bin */

...but to hang in that case, in order not to jump to random code.

Ok, I should have something publishable, soon ;-).

Quoted 5 lines

Some observations:

Not sure what to do with CONFIG_SPL_STACK: altera was just using symbol from linker script. Is there way to keep it like that? I'd hate to invent magic constant.

Well, lets think about this. Previously, everyone has been saying "use this spot in SRAM" or "use this spot in DRAM (which has been setup previously)". But I think you should be able to do
#define CONFIG_SPL_STACK __linker_symbol and be done with it.

[snip]

Quoted 6 lines

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

My hope is that since I got the SPL patches posted in time we can get them in for v2012.10 so no, this will only help your chances :)

Hi!

Quoted 5 lines

[snip]

Quoted 4 lines

Quoted 3 lines

It's the omap-common/spl.c code with the bits for IH_OS_LINUX taken out :) Can you please re-do your series on top of the SPL series I just posted that provides a common SPL framework? Thanks!

Quoted 5 lines

Quoted 2 lines

I'll take a look. OTOH, ammount of code duplication here is quite low, and redoing it on top of SPL series will mean delaying merge, right?

My hope is that since I got the SPL patches posted in time we can get them in for v2012.10 so no, this will only help your chances :)

Ok, this is promissed patch for comments. socfpga spl.c now got reasonably smaller. It is on top of

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

Now... can I leave spl_ram_load_image() where it is? Probably add some #ifdef?

Dinh, does this look acceptable to you? Could we switch to absolute addresses for stack/heap in the SPL?

arch/arm/cpu/armv7/socfpga/spl.c new file +13 -0

+
+u32 spl_boot_device(void)
+{
+	return BOOT_DEVICE_RAM;
+}
+
+/*
+ * Board initialization after bss clearance
+ */
+void spl_board_init(void)
+{
+	/* init timer for enabling delay function */
+	timer_init();

Quoted 2 lines

Ok, this is promissed patch for comments. socfpga spl.c now got reasonably smaller. It is on top of

Looks good overall. I don't see asm/arch/spl.h but that's where you could put the extern for __stack_start and then define CONFIG_SPL_STACK to &__stack_start.

Quoted 4 lines

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

Now... can I leave spl_ram_load_image() where it is? Probably add some #ifdef?

Yeah, I think that's OK to start with, we can move things more later if needed.

Quoted 1 line

+static void spl_console_init(void)

Looks like preloader_console_init :)

Review

Quoted 3 lines

(quoted diff) +3 -0

+	/* setup MALLOC after clocks going faster */
+	mem_malloc_init((ulong) &__malloc_start,
+			(&__malloc_end - &__malloc_start));

Shouldn't need this, should already be called before you get to spl_board_init.

[snip]

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Quoted 4 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +4 -0

+	. = . + 4;
+	. = ALIGN(8);
+	__malloc_start = .;
+	. = . + CONFIG_SPL_MALLOC_SIZE;

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +6 -0

+	__malloc_end = .;
+
+	. = . + 4;
+	. = . + CONFIG_SPL_STACK_SIZE;
+	. = ALIGN(8);
+	__stack_start = .;

Do you really need to do . = . + 4 and then align statements?

Quoted 1 line

+LIBS-$(CONFIG_SPL_LIBGENERIC_SUPPORT) += lib/libgeneric.o

q
Er? There's already a line for lib/libgeneric.o

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Hi!

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Hmm, I tried, but I don't know socfpga memory layout by heart.

Dinh, can you help here?

Quoted 6 lines

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Quoted 3 lines

Hmm, I tried, but I don't know socfpga memory layout by heart.

Dinh, can you help here?

I think once you get the answers you should be able to re-post the series cleanly and depend on my v5 (or v6) branch. Thanks!

Hi!

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Quoted 6 lines

Quoted 3 lines

Hmm, I tried, but I don't know socfpga memory layout by heart.

Dinh, can you help here?

I think once you get the answers you should be able to re-post the series cleanly and depend on my v5 (or v6) branch. Thanks!

Ok, figuring the .lds might be tricky. But... that's mostly additional (safety) feature. Would it be possible to merge it without that and add it later?

Port to v5 of the patch should not be a problem.

For the record and review, current version of patch is here.

arch/arm/cpu/armv7/socfpga/spl.c +0 -0

arch/arm/cpu/armv7/socfpga/spl.c +4 -0

+/*
+ * Board initialization after bss clearance
+ */
+void spl_board_init(void)

arch/arm/include/asm/arch-socfpga/spl.h +0 -0

board/altera/socfpga_cyclone5/socfpga_cyclone5.c +0 -0

include/common.h +0 -0

index 55025c0..469a3af 100644

include/configs/socfpga_cyclone5.h new file +0 -0

new file mode 100644
index 0000000..d001cbb

+#undef CONFIG_USE_IRQ

+#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "

Quoted 7 lines

Quoted 2 lines

I think once you get the answers you should be able to re-post the series cleanly and depend on my v5 (or v6) branch. Thanks!

Ok, figuring the .lds might be tricky. But... that's mostly additional (safety) feature. Would it be possible to merge it without that and add it later?

Quoted 3 lines

Port to v5 of the patch should not be a problem.

For the record and review, current version of patch is here.

A few small comments. No need to re-post intermediate patches, just address in the next full go-round. Thanks!

Quoted 1 line

+void spl_board_init(void)

[snip]

arch/arm/cpu/armv7/socfpga/spl.c line 49

Quoted 1 line

arch/arm/cpu/armv7/socfpga/spl.c new file +1 -0

+	puts("SPL Boot\n");

Sure you want to keep that line? We already have a general "U-Boot SPL ..." line being printed.

board/altera/socfpga_cyclone5/socfpga_cyclone5.c line 81

Quoted 8 lines

+ * miscellaneous platform dependent initialisations
+ */

board/altera/socfpga_cyclone5/socfpga_cyclone5.c new file +6 -0

+int misc_init_r(void)
+{
+	/* Set to "n" for not verifying the uImage */
+	setenv("verify", "n");
+	return 0;
+}

Not good to enforce policy like this. Just don't set it in the config file by default and let users do as they need.

[snip]

Quoted 7 lines

common/spl/spl.c +0 -0

index eaea1c8..5adbf0e 100644
@@ -78,6 +78,7 @@ void spl_parse_image_header(const struct image_header *header)
 	u32 header_size = sizeof(struct image_header);
 

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

Quoted 5 lines

common/spl/spl.c +1 -0

 	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of 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 */

Just an extra comment, drop please. Or split out if you feel it's really helpful. No strong opinion here other than not in the same patch as the rest.

[snip]

Quoted 5 lines

include/common.h +0 -0

index 55025c0..469a3af 100644

include/common.h void external_interrupt (struct pt_regs *); · line 711

Quoted 9 lines

include/common.h +3 -1

@@ -704,7 +704,9 @@ void	external_interrupt (struct pt_regs *);
 void	irq_install_handler(int, interrupt_handler_t *, void *);
 void	irq_free_handler   (int);
 void	reset_timer	   (void);
-ulong	get_timer	   (ulong base);
+ulong	get_timer(ulong base);
+void	reset_timer_count(void);
+ulong	get_timer_count(ulong base);
 void	enable_interrupts  (void);
 int	disable_interrupts (void);

Make the new declarations match the old, spacing wise please.

Quoted 11 lines

board/altera/socfpga_cyclone5/socfpga_cyclone5.c line 81

Quoted 8 lines

+ * miscellaneous platform dependent initialisations
+ */

board/altera/socfpga_cyclone5/socfpga_cyclone5.c new file +6 -0

+int misc_init_r(void)
+{
+	/* Set to "n" for not verifying the uImage */
+	setenv("verify", "n");
+	return 0;
+}

Not good to enforce policy like this. Just don't set it in the config file by default and let users do as they need.

Hmm. I moved it to CONFIG_EXTRA_ENV_SETTINGS as done by for eample u8500_href.h. Is that ok?

Quoted 8 lines

[snip]

Quoted 7 lines

common/spl/spl.c +0 -0

index eaea1c8..5adbf0e 100644
@@ -78,6 +78,7 @@ void spl_parse_image_header(const struct image_header *header)
 	u32 header_size = sizeof(struct image_header);
 

Quoted 9 lines

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

Quoted 5 lines

common/spl/spl.c +1 -0

 	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of 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 */

Just an extra comment, drop please. Or split out if you feel it's really helpful. No strong opinion here other than not in the same patch as the rest.

Ok, I'll drop it, and the omap cleanup also. Not worth the merge effort.

spl_ram_load_image... will I need to create some kind of #ifdef? Or would #ifdef BOOT_DEVICE_RAM do the trick?

Quoted 12 lines · Pavel Machek

Quoted 11 lines

board/altera/socfpga_cyclone5/socfpga_cyclone5.c line 81

Quoted 8 lines

+ * miscellaneous platform dependent initialisations
+ */

board/altera/socfpga_cyclone5/socfpga_cyclone5.c new file +6 -0

+int misc_init_r(void)
+{
+	/* Set to "n" for not verifying the uImage */
+	setenv("verify", "n");
+	return 0;
+}

Not good to enforce policy like this. Just don't set it in the config file by default and let users do as they need.

Quoted 2 lines

Hmm. I moved it to CONFIG_EXTRA_ENV_SETTINGS as done by for eample u8500_href.h. Is that ok?

It defaults to no, I believe is the thing (so yes, there's a few bad examples out there including, urk, one that I own).

Quoted 17 lines

Quoted 16 lines

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

Quoted 12 lines

common/spl/spl.c +1 -0

index eaea1c8..5adbf0e 100644
@@ -78,6 +78,7 @@ void spl_parse_image_header(const struct image_header *header)
 	u32 header_size = sizeof(struct image_header);
 
 	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of 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 */

Just an extra comment, drop please. Or split out if you feel it's really helpful. No strong opinion here other than not in the same patch as the rest.

Quoted 5 lines

Ok, I'll drop it, and the omap cleanup also. Not worth the merge effort.

spl_ram_load_image... will I need to create some kind of #ifdef? Or would #ifdef BOOT_DEVICE_RAM do the trick?

Good point, yes, we should add CONFIG_SPL_RAM_DEVICE and document it in docs/README.SPL and the toplevel README.

Hi!

Quoted 20 lines

Quoted 20 lines

Quoted 7 lines

Quoted 7 lines

common/spl/spl.c +0 -0

index eaea1c8..5adbf0e 100644
@@ -78,6 +78,7 @@ void spl_parse_image_header(const struct image_header *header)
 	u32 header_size = sizeof(struct image_header);
 

Quoted 9 lines

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

Quoted 5 lines

common/spl/spl.c +1 -0

 	if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
+		/* Valid image. Extract information out of 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 */

Just an extra comment, drop please. Or split out if you feel it's really helpful. No strong opinion here other than not in the same patch as the rest.

Ok, I'll drop it, and the omap cleanup also. Not worth the merge effort.

Quoted 5 lines

Quoted 2 lines

spl_ram_load_image... will I need to create some kind of #ifdef? Or would #ifdef BOOT_DEVICE_RAM do the trick?

Good point, yes, we should add CONFIG_SPL_RAM_DEVICE and document it in docs/README.SPL and the toplevel README.

Ok, something like this? Posting separately, maybe it makes sense to merge to your PATCH v6...?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

spl_ram_load_image... will I need to create some kind of #ifdef? Or would #ifdef BOOT_DEVICE_RAM do the trick?

Good point, yes, we should add CONFIG_SPL_RAM_DEVICE and document it in docs/README.SPL and the toplevel README.

Ok, something like this? Posting separately, maybe it makes sense to merge to your PATCH v6...?

Sure, just include the actual spl_ram_load_image bits as well and I'll pick it up.

Hi!

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

spl_ram_load_image... will I need to create some kind of #ifdef? Or would #ifdef BOOT_DEVICE_RAM do the trick?

Good point, yes, we should add CONFIG_SPL_RAM_DEVICE and document it in docs/README.SPL and the toplevel README.

Quoted 5 lines

Quoted 2 lines

Ok, something like this? Posting separately, maybe it makes sense to merge to your PATCH v6...?

Sure, just include the actual spl_ram_load_image bits as well and I'll pick it up.

Here you go.

Quoted 6 lines · Pavel Machek

Hi!

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

spl_ram_load_image... will I need to create some kind of #ifdef? Or would #ifdef BOOT_DEVICE_RAM do the trick?

Good point, yes, we should add CONFIG_SPL_RAM_DEVICE and document

it in

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

docs/README.SPL and the toplevel README.

Ok, something like this? Posting separately, maybe it makes sense to merge to your PATCH v6...?

Quoted 4 lines

Quoted 2 lines

Sure, just include the actual spl_ram_load_image bits as well and I'll pick it up.

Here you go.

With the multi-line comment fixed up, queued for my v6.

--
Tom

Hi Tom,

On Thu, Aug 30, 2012 at 4:25 PM, Tom Rini <[email protected] <mailto:[email protected]>> wrote:

On Thu, Aug 30, 2012 at 10:42:11PM +0200, Pavel Machek wrote:
> Hi!
>
> > >>> spl_ram_load_image... will I need to create some kind of #ifdef? Or
> > >>> would #ifdef BOOT_DEVICE_RAM do the trick?
> > >>
> > >> Good point, yes, we should add CONFIG_SPL_RAM_DEVICE and document it in
> > >> docs/README.SPL and the toplevel README.
> > >
> > > Ok, something like this? Posting separately, maybe it makes sense to
> > > merge to your PATCH v6...?
> >
> > Sure, just include the actual spl_ram_load_image bits as well and I'll
> > pick it up.
>

> Here you go.

With the multi-line comment fixed up, queued for my v6.

Do you need me to send a fresh version with all the fixes?

+++ b/common/spl/spl.c

common/spl/spl.c +0 -0

@@ -115,6 +115,22 @@ static void __noreturn jump_to_image_no_args(void)
 	image_entry((u32 *)boot_params_ptr_addr);
 }

Quoted 8 lines · Pavel Machek

Hi!

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Quoted 3 lines

Hmm, I tried, but I don't know socfpga memory layout by heart.

Dinh, can you help here?

I think once you get the answers you should be able to re-post the series cleanly and depend on my v5 (or v6) branch. Thanks!

Quoted 14 lines · Pavel Machek

Quoted 12 lines

Hi!

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 2 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Hmm, I tried, but I don't know socfpga memory layout by heart.

Dinh, can you help here?

Quoted 2 lines

I think once you get the answers you should be able to re-post the series cleanly and depend on my v5 (or v6) branch. Thanks!

Porting it to your v5 was easy :-). Newer patch for review is attached.

I took oportunity to cleanup whitespace in arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. Perhaps someone can merge that...

Thanks,
Pavel

commit a7ecaa40d9a02e84ac81da8f49d48595d7f342ad

Author: Pavel <[email protected]>
Date:   Thu Aug 30 01:28:00 2012 +0200

Add changes for socfpga

MAINTAINERS +5 -0

index c5a6f2f..df48dea 100644
@@ -765,6 +765,11 @@ Nagendra T S  <[email protected]>
 
    am3517_crane    ARM ARMV7 (AM35x SoC)
 
+Dinh Nguyen <[email protected]>
+Chin Liang See <[email protected]>
+
+	socfpga		socfpga_cyclone5
+
 Kyungmin Park <[email protected]>
 
 	apollon		ARM1136EJS

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds index 8867e06..1d8efb2 100644

arch/arm/cpu/armv7/omap-common/u-boot-spl.lds +0 -0

 	. = ALIGN(4);

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/Makefile new file mode 100644 index 0000000..376a4bd

arch/arm/cpu/armv7/socfpga/Makefile new file +51 -0

@@ -0,0 +1,51 @@
+#
+# (C) Copyright 2000-2003
+# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+#
+# Copyright (C) 2012 Altera Corporation <www.altera.com>
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+
+
+include $(TOPDIR)/config.mk
+
+LIB	=  $(obj)lib$(SOC).o
+
+SOBJS	:= lowlevel_init.o
+COBJS-y	:= misc.o timer.o
+COBJS-$(CONFIG_SPL_BUILD) += spl.o
+
+COBJS	:= $(COBJS-y)
+SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
+OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))
+
+all:	 $(obj).depend $(LIB)
+
+$(LIB):	$(OBJS)
+	$(call cmd_link_o_target, $(OBJS))
+
+#########################################################################
+
+# defines $(obj).depend target
+include $(SRCTREE)/rules.mk
+
+sinclude $(obj).depend
+
+#########################################################################

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/config.mk new file mode 100644 index 0000000..b72ed1e

arch/arm/cpu/armv7/socfpga/config.mk new file +16 -0

@@ -0,0 +1,16 @@
+#
+# Copyright (C) 2011, Texas Instruments, Incorporated - http://www.ti.com/
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed "as is" WITHOUT ANY WARRANTY of any
+# kind, whether express or implied; without even the implied warranty
+# of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+ifndef CONFIG_SPL_BUILD
+ALL-y	+= $(obj)u-boot.img
+endif

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/lowlevel_init.S new file mode 100644 index 0000000..815073e

arch/arm/cpu/armv7/socfpga/lowlevel_init.S new file +26 -0

@@ -0,0 +1,79 @@
+/*
+ *  Copyright (C) 2012 Altera Corporation <www.altera.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+#include <version.h>
+
+/* Save the parameter pass in by previous boot loader */
+.global save_boot_params
+save_boot_params:
+	/* save the parameter here */
+
+	/*

arch/arm/cpu/armv7/socfpga/lowlevel_init.S new file +43 -0

+	ldr	r1, =IRQ_STACK_START_IN
+	str	r0, [r1]
+
+	bx	lr
+
+
+/* Set up the platform, once the cpu has been initialized */
+.globl lowlevel_init
+lowlevel_init:
+
+	/* Remap */
+#ifdef CONFIG_SPL_BUILD
+	/*
+	 * SPL : configure the remap (L3 NIC-301 GPV)
+	 * so the on-chip RAM at lower memory instead ROM.
+	 */
+	ldr	r0, =SOCFPGA_L3REGS_ADDRESS
+	mov	r1, #0x19
+	str	r1, [r0]
+#else
+	/*
+	 * U-Boot : configure the remap (L3 NIC-301 GPV)
+	 * so the SDRAM at lower memory instead on-chip RAM.
+	 */
+	ldr	r0, =SOCFPGA_L3REGS_ADDRESS
+	mov	r1, #0x2
+	str	r1, [r0]
+
+	/* Private components security */
+
+	/*
+	 * U-Boot : configure private timer, global timer and cpu
+	 * component access as non secure for kernel stage (as required
+	 * by kernel)
+	 */
+	mrc	p15,4,r0,c15,c0,0
+	add	r1, r0, #0x54
+	ldr	r2, [r1]
+	orr	r2, r2, #0xff
+	orr	r2, r2, #0xf00
+	str	r2, [r1]
+#endif	/* #ifdef CONFIG_SPL_BUILD */
+	mov	pc, lr

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/misc.c new file mode 100644 index 0000000..b906701

arch/arm/cpu/armv7/socfpga/misc.c new file +19 -0

@@ -0,0 +1,54 @@
+/*
+ *  Copyright (C) 2012 Altera Corporation <www.altera.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <common.h>
+#include <asm/io.h>

arch/arm/cpu/armv7/socfpga/misc.c new file +11 -0

+void reset_cpu(ulong addr)
+{
+	/* request a warm reset */
+	writel(RSTMGR_CTRL_SWWARMRSTREQ_LSB, &reset_manager_base->ctrl);
+	/*
+	 * infinite loop here as watchdog will trigger and reset
+	 * the processor
+	 */
+	while (1)
+		;
+}

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/socfpga/timer.c new file mode 100644 index 0000000..28da4d0

arch/arm/cpu/armv7/socfpga/timer.c new file +24 -0

@@ -0,0 +1,149 @@
+/*
+ *  Copyright (C) 2012 Altera Corporation <www.altera.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/timer.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static const struct socfpga_timer *timer_base = (void

*)CONFIG_SYS_TIMERBASE; +

arch/arm/cpu/armv7/socfpga/timer.c new file +3 -0

+		(&timer_base->ctrl));
+	return 0;
+}

arch/arm/cpu/armv7/socfpga/timer.c new file +9 -0

+#include <common.h>
+#include <asm/arch/reset_manager.h>
+#include <asm/io.h>
+
+#include <netdev.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+void show_boot_progress(int progress)

Quoted 3 lines

I took oportunity to cleanup whitespace in arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. Perhaps someone can merge that...

Argh ... what about using git send-email for the patch submission please?

NOTE: I really have a great deal of respect for Pavel, so i do have trouble stepping on him properly during the patch review ;-)

arch/arm/cpu/armv7/socfpga/lowlevel_init.S line 36

Quoted 3 lines

arch/arm/cpu/armv7/socfpga/lowlevel_init.S new file +3 -0

+	ldr	r0, =CONFIG_SYS_INIT_RAM_ADDR
+	ldr	r1, =CONFIG_SYS_INIT_RAM_SIZE
+	add	r0, r0, r1

Won't the preprocessor compute (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) for you? Then you'd only have to do one LDR and no ADD instruction, saving two ticks of CPU :-)

arch/arm/cpu/armv7/socfpga/misc.c line 29

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/misc.c new file +6 -0

+static const struct socfpga_reset_manager *reset_manager_base =
+		(void *)SOCFPGA_RSTMGR_ADDRESS;
+
+/*
+ * Write the reset manager register to cause reset
+ */

You might want to add __attribute__((noreturn)) to this function :-)

arch/arm/cpu/armv7/socfpga/timer.c line 33

Quoted 3 lines

arch/arm/cpu/armv7/socfpga/timer.c new file +3 -0

+	writel(TIMER_LOAD_VAL, &timer_base->load_val);
+	writel(TIMER_LOAD_VAL, &timer_base->curr_val);
+	writel((readl((&timer_base->ctrl)) | 0x3),

I think you should stick to programming in C here, not ((((LISP)))), so try cutting down on the ((braces)) :-)

btw. what's this 0x3 magic constant ?

board/altera/socfpga_cyclone5/socfpga_cyclone5.c line 36

Quoted 6 lines

board/altera/socfpga_cyclone5/socfpga_cyclone5.c new file +6 -0

+static inline void delay(unsigned long loops)
+{
+	__asm__ volatile ("1:\n"
+		"subs %0, %1, #1\n"
+		"bne 1b" : "=r" (loops) : "0" (loops));
+}

Am I flat blind or is this not used here?

Hi!

Quoted 5 lines

Quoted 3 lines

I took oportunity to cleanup whitespace in arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. Perhaps someone can merge that...

Argh ... what about using git send-email for the patch submission please?

I'm not supposed to submit patches here, these are just requests for review ;-).

Quoted 2 lines

NOTE: I really have a great deal of respect for Pavel, so i do have trouble stepping on him properly during the patch review ;-)

;-).

Quoted 7 lines

arch/arm/cpu/armv7/socfpga/lowlevel_init.S line 36

Quoted 3 lines

arch/arm/cpu/armv7/socfpga/lowlevel_init.S new file +3 -0

+	ldr	r0, =CONFIG_SYS_INIT_RAM_ADDR
+	ldr	r1, =CONFIG_SYS_INIT_RAM_SIZE
+	add	r0, r0, r1

Won't the preprocessor compute (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) for you? Then you'd only have to do one LDR and no ADD instruction, saving two ticks of CPU :-)

:
:-). And opportunity for me to try my luck at arm assembly. Does this

look correct?

        ldr     r0, =(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE)
        ldr     r1, =IRQ_STACK_START_IN
        str     r0, [r1]

bx lr

Quoted 8 lines

arch/arm/cpu/armv7/socfpga/misc.c line 29

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/misc.c new file +6 -0

+static const struct socfpga_reset_manager *reset_manager_base =
+		(void *)SOCFPGA_RSTMGR_ADDRESS;
+
+/*
+ * Write the reset manager register to cause reset
+ */

You might want to add __attribute__((noreturn)) to this function :-)

Hmm... I guess that should go to the central header file? No need to sprinkle it all through the C files. (Done).

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/timer.c line 33

Quoted 3 lines

arch/arm/cpu/armv7/socfpga/timer.c new file +3 -0

+	writel(TIMER_LOAD_VAL, &timer_base->load_val);
+	writel(TIMER_LOAD_VAL, &timer_base->curr_val);
+	writel((readl((&timer_base->ctrl)) | 0x3),

I think you should stick to programming in C here, not ((((LISP)))), so try cutting down on the ((braces)) :-)

Some braces have been harmed by creation of this patch.

Quoted 1 line

btw. what's this 0x3 magic constant ?

Dinh has to help here :-(.

Quoted 8 lines

board/altera/socfpga_cyclone5/socfpga_cyclone5.c line 36

Quoted 6 lines

board/altera/socfpga_cyclone5/socfpga_cyclone5.c new file +6 -0

+static inline void delay(unsigned long loops)
+{
+	__asm__ volatile ("1:\n"
+		"subs %0, %1, #1\n"
+		"bne 1b" : "=r" (loops) : "0" (loops));
+}

Am I flat blind or is this not used here?

Killed, thanks. Lets see what the compiler thinks.

Pavel

Quoted 8 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	writel(TIMER_LOAD_VAL, &timer_base->load_val);
+	writel(TIMER_LOAD_VAL, &timer_base->curr_val);
+	writel((readl((&timer_base->ctrl)) | 0x3),

I think you should stick to programming in C here, not ((((LISP)))), so try cutting down on the ((braces)) :-)

Some braces have been harmed by creation of this patch.

At least one more pair must be killed on the above statement :)

Quoted 3 lines

Quoted 1 line

btw. what's this 0x3 magic constant ?

Dinh has to help here :-(.

Also, use setbits_le32() there instead anyway.

Hi!

Quoted 10 lines

Quoted 8 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	writel(TIMER_LOAD_VAL, &timer_base->load_val);
+	writel(TIMER_LOAD_VAL, &timer_base->curr_val);
+	writel((readl((&timer_base->ctrl)) | 0x3),

I think you should stick to programming in C here, not ((((LISP)))), so try cutting down on the ((braces)) :-)

Some braces have been harmed by creation of this patch.

At least one more pair must be killed on the above statement :)

I removed two pairs but failed to post the patch ;-). Be sure you'll never see them again.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

btw. what's this 0x3 magic constant ?

Dinh has to help here :-(.

Also, use setbits_le32() there instead anyway.

Uff, setbits_le32 looks seriously strange. Should I do this?

Thanks for review,

Quoted 3 lines

I took oportunity to cleanup whitespace in arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. Perhaps someone can merge that...

Argh ... what about using git send-email for the patch submission please?

NOTE: I really have a great deal of respect for Pavel, so i do have trouble stepping on him properly during the patch review ;-)

arch/arm/cpu/armv7/socfpga/lowlevel_init.S line 36

Quoted 3 lines

arch/arm/cpu/armv7/socfpga/lowlevel_init.S new file +3 -0

+	ldr	r0, =CONFIG_SYS_INIT_RAM_ADDR
+	ldr	r1, =CONFIG_SYS_INIT_RAM_SIZE
+	add	r0, r0, r1

Won't the preprocessor compute (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) for you? Then you'd only have to do one LDR and no ADD instruction, saving two ticks of CPU :-)

arch/arm/cpu/armv7/socfpga/misc.c

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/misc.c new file +5 -0

+static const struct socfpga_reset_manager *reset_manager_base =
+		(void *)SOCFPGA_RSTMGR_ADDRESS;
+
+/*
+ * Write the reset manager register to cause reset  */

You might want to add __attribute__((noreturn)) to this function :-)

arch/arm/cpu/armv7/socfpga/timer.c line 33

Quoted 3 lines

arch/arm/cpu/armv7/socfpga/timer.c new file +3 -0

+	writel(TIMER_LOAD_VAL, &timer_base->load_val);
+	writel(TIMER_LOAD_VAL, &timer_base->curr_val);
+	writel((readl((&timer_base->ctrl)) | 0x3),

I think you should stick to programming in C here, not ((((LISP)))), so try cutting down on the ((braces)) :-)

btw. what's this 0x3 magic constant ?

board/altera/socfpga_cyclone5/socfpga_cyclone5.c

Quoted 4 lines

board/altera/socfpga_cyclone5/socfpga_cyclone5.c new file +4 -0

+static inline void delay(unsigned long loops) {
+	__asm__ volatile ("1:\n"
+		"subs %0, %1, #1\n"
+		"bne 1b" : "=r" (loops) : "0" (loops)); }

Am I flat blind or is this not used here?

Quoted 13 lines · Tom Rini

Quoted 11 lines · Pavel Machek

Hi!

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Hmm, I tried, but I don't know socfpga memory layout by heart.

Dinh, can you help here?

Quoted 2 lines

I think once you get the answers you should be able to re-post the series cleanly and depend on my v5 (or v6) branch. Thanks!

Porting it to your v5 was easy :-). Newer patch for review is attached.

I took oportunity to cleanup whitespace in arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. Perhaps someone can merge that...

Quoted 17 lines · Pavel Machek

Quoted 15 lines · Tom Rini

Quoted 11 lines · Pavel Machek

Hi!

Quoted 5 lines

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds

Quoted 1 line

arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +0 -0

You should setup MEMORY declarations like the other u-boot-spl linker scripts do so we get build-time confirmation that we haven't exceeded our size limitations.

Hmm, I tried, but I don't know socfpga memory layout by heart.

Dinh, can you help here?

I think once you get the answers you should be able to re-post the series cleanly and depend on my v5 (or v6) branch. Thanks!