arm: Tegra2: add support for A9 CPU init

arch/arm/cpu/armv7/tegra2/ap20.c new file +25 -0

+/*
+ * TBD: Move cold_boot() to assembly file.
+ * Values/offsets of the table vars make this difficult.
+ */
+
+void cold_boot(void)
+{
+	asm volatile(
+		"msr	cpsr_c, #0xD3	\n"
+		/*
+		* Check current processor: CPU or AVP?
+		* If CPU, go to CPU boot code, else continue on AVP path.
+		*/
+		"mov	r0, %0		\n"
+		"ldrb	r2, [r0, %1]	\n"
+		/* are we the CPU? */
+		"cmp	r2, %2		\n"
+		"mov	sp, %3		\n"
+		/*  yep, we are the CPU */
+		"bxeq	%4		\n"
+
+		/* AVP initialization follows this path */
+		"mov	sp, %5		\n"
+		/* Init and start CPU */
+		"b	startup_cpu	\n"

+.globl startup_cpu

arch/arm/cpu/armv7/tegra2/lowlevel_init.S +8 -0

+startup_cpu:
+	@ Initialize the AVP, clocks, and memory controller
+	@ SDRAM is guaranteed to be on at this point
+
+	ldr     r0, =cold_boot			@ R0 = reset vector for CPU
+	bl      start_cpu			@ start the CPU
+
+	@ Transfer control to the AVP code */

Hi Tom,
Things look pretty good. ?Minor comments/questions below.

<snip>

Quoted 32 lines

(quoted diff) +32 -0

+/*
+ * TBD: Move cold_boot() to assembly file.
+ * Values/offsets of the table vars make this difficult.
+ */
+
+void cold_boot(void)
+{
+ ? ? asm volatile(
+ ? ? ? ? ? ? "msr ? ?cpsr_c, #0xD3 ? \n"
+ ? ? ? ? ? ? /*
+ ? ? ? ? ? ? * Check current processor: CPU or AVP?
+ ? ? ? ? ? ? * If CPU, go to CPU boot code, else continue on AVP path.
+ ? ? ? ? ? ? */
+ ? ? ? ? ? ? "mov ? ?r0, %0 ? ? ? ? ?\n"
+ ? ? ? ? ? ? "ldrb ? r2, [r0, %1] ? ?\n"
+ ? ? ? ? ? ? /* are we the CPU? */
+ ? ? ? ? ? ? "cmp ? ?r2, %2 ? ? ? ? ?\n"
+ ? ? ? ? ? ? "mov ? ?sp, %3 ? ? ? ? ?\n"
+ ? ? ? ? ? ? /* ?yep, we are the CPU */
+ ? ? ? ? ? ? "bxeq ? %4 ? ? ? ? ? ? ?\n"
+
+ ? ? ? ? ? ? /* AVP initialization follows this path */
+ ? ? ? ? ? ? "mov ? ?sp, %5 ? ? ? ? ?\n"
+ ? ? ? ? ? ? /* Init and start CPU */
+ ? ? ? ? ? ? "b ? ? ?startup_cpu ? ? \n"
+ ? ? ? ? ? ? :
+ ? ? ? ? ? ? : "i"(NV_PA_PG_UP_BASE),
+ ? ? ? ? ? ? "i"(PG_UP_TAG_0),
+ ? ? ? ? ? ? "r"(proc_tag),
+ ? ? ? ? ? ? "r"(cpu_boot_stack),
+ ? ? ? ? ? ? "r"(_armboot_start),
+ ? ? ? ? ? ? "r"(avp_boot_stack)

Quoted 3 lines

+ ? ? ? ? ? ? : "r0", "r2", "cc", "lr"
+ ? ? );
+}

What errors did you encounter when this was in the assembly file? ?It'd be nice to put it there now. ?Likely it will never get fixed if it doesn't implemented correctly off the bat. ?If you post the errors perhaps someone on the list can provide insight.

<snip>

Quoted 14 lines

+.globl startup_cpu

(quoted diff) +13 -0

+startup_cpu:
+ ? ? @ Initialize the AVP, clocks, and memory controller
+ ? ? @ SDRAM is guaranteed to be on at this point
+
+ ? ? ldr ? ? r0, =cold_boot ? ? ? ? ? ? ? ? ?@ R0 = reset vector for CPU
+ ? ? bl ? ? ?start_cpu ? ? ? ? ? ? ? ? ? ? ? @ start the CPU
+
+ ? ? @ Transfer control to the AVP code */
+ ? ? bl ? ? ?halt_avp
+
+ ? ? @ Should never get here
+_loop_forever2:
+ ? ? b ? ? ? _loop_forever2

arch/arm/cpu/armv7/tegra2/lowlevel_init.S lowlevel_init:

Quoted 5 lines

arch/arm/cpu/armv7/tegra2/lowlevel_init.S +5 -0

+
+.globl cache_configure
+cache_configure:
+ ? ? stmdb r13!,{r14}
+@ ? ?/* invalidate instruction cache */

It looks like there's a combination of comment forms @, @ */, and @ /* */. ?Is there a reason not to use the normal /* */ universally?

Best,
Peter

Peter,

Quoted 37 lines · Peter Tyser

Hi Tom,
Things look pretty good. Minor comments/questions below.

<snip>

Quoted 32 lines

arch/arm/cpu/armv7/tegra2/ap20.c new file +32 -0

+/*
+ * TBD: Move cold_boot() to assembly file.
+ * Values/offsets of the table vars make this difficult.
+ */
+
+void cold_boot(void)
+{
+     asm volatile(
+             "msr    cpsr_c, #0xD3   \n"
+             /*
+             * Check current processor: CPU or AVP?
+             * If CPU, go to CPU boot code, else continue on AVP path.
+             */
+             "mov    r0, %0          \n"
+             "ldrb   r2, [r0, %1]    \n"
+             /* are we the CPU? */
+             "cmp    r2, %2          \n"
+             "mov    sp, %3          \n"
+             /*  yep, we are the CPU */
+             "bxeq   %4              \n"
+
+             /* AVP initialization follows this path */
+             "mov    sp, %5          \n"
+             /* Init and start CPU */
+             "b      startup_cpu     \n"
+             :
+             : "i"(NV_PA_PG_UP_BASE),
+             "i"(PG_UP_TAG_0),
+             "r"(proc_tag),
+             "r"(cpu_boot_stack),
+             "r"(_armboot_start),
+             "r"(avp_boot_stack)

Quoted 8 lines

arch/arm/cpu/armv7/tegra2/ap20.c line 366

Quoted 3 lines

arch/arm/cpu/armv7/tegra2/ap20.c new file +3 -0

+             : "r0", "r2", "cc", "lr"
+     );
+}

What errors did you encounter when this was in the assembly file? It'd be nice to put it there now. Likely it will never get fixed if it doesn't implemented correctly off the bat. If you post the errors perhaps someone on the list can provide insight.

I didn't capture a log of the errors when I was trying to put the cold_boot code into lowlevel_init.S. But I saw fixup errors and undefined constant errors, all related to the #defines (NV_PG_UP_BASE, avp/cpu_boot_stack, etc.) and how the compiler/assembler references indirect and relative constants.

Note that this code works perfectly as-is, so there's no pressing need to move it to assembly now, except for a cosmetic/procedural one. I'd rather get this accepted into mainline, so I can move on to the eMMC/SPI/USB drivers so people can use the code to boot an OS on our (many) Tegra2 boards coming to market RSN.

If some ARM / gcc assembly wizard wants to attempt moving this code to a .S file, I welcome the help - I may even attack it at a later date, when I've got more bandwidth. But it isn't a priority for me right now, unless someone on the list adamantly opposes the code as-s. But I'd expect anyone with that strong an opinion about to be able to fix it, or at least attempt it and see why I decided to defer moving it to assembly for now.

Quoted 60 lines · Tom Warren

Peter,

Quoted 37 lines · Peter Tyser

Hi Tom,
Things look pretty good. ?Minor comments/questions below.

<snip>

Quoted 32 lines

(quoted diff) +32 -0

+/*
+ * TBD: Move cold_boot() to assembly file.
+ * Values/offsets of the table vars make this difficult.
+ */
+
+void cold_boot(void)
+{
+ ? ? asm volatile(
+ ? ? ? ? ? ? "msr ? ?cpsr_c, #0xD3 ? \n"
+ ? ? ? ? ? ? /*
+ ? ? ? ? ? ? * Check current processor: CPU or AVP?
+ ? ? ? ? ? ? * If CPU, go to CPU boot code, else continue on AVP path.
+ ? ? ? ? ? ? */
+ ? ? ? ? ? ? "mov ? ?r0, %0 ? ? ? ? ?\n"
+ ? ? ? ? ? ? "ldrb ? r2, [r0, %1] ? ?\n"
+ ? ? ? ? ? ? /* are we the CPU? */
+ ? ? ? ? ? ? "cmp ? ?r2, %2 ? ? ? ? ?\n"
+ ? ? ? ? ? ? "mov ? ?sp, %3 ? ? ? ? ?\n"
+ ? ? ? ? ? ? /* ?yep, we are the CPU */
+ ? ? ? ? ? ? "bxeq ? %4 ? ? ? ? ? ? ?\n"
+
+ ? ? ? ? ? ? /* AVP initialization follows this path */
+ ? ? ? ? ? ? "mov ? ?sp, %5 ? ? ? ? ?\n"
+ ? ? ? ? ? ? /* Init and start CPU */
+ ? ? ? ? ? ? "b ? ? ?startup_cpu ? ? \n"
+ ? ? ? ? ? ? :
+ ? ? ? ? ? ? : "i"(NV_PA_PG_UP_BASE),
+ ? ? ? ? ? ? "i"(PG_UP_TAG_0),
+ ? ? ? ? ? ? "r"(proc_tag),
+ ? ? ? ? ? ? "r"(cpu_boot_stack),
+ ? ? ? ? ? ? "r"(_armboot_start),
+ ? ? ? ? ? ? "r"(avp_boot_stack)

Quoted 8 lines

Quoted 3 lines

+ ? ? ? ? ? ? : "r0", "r2", "cc", "lr"
+ ? ? );
+}

What errors did you encounter when this was in the assembly file? ?It'd be nice to put it there now. ?Likely it will never get fixed if it doesn't implemented correctly off the bat. ?If you post the errors perhaps someone on the list can provide insight.

I didn't capture a log of the errors when I was trying to put the cold_boot code into lowlevel_init.S. But I saw fixup errors and undefined constant errors, all related to the #defines (NV_PG_UP_BASE, avp/cpu_boot_stack, etc.) and how the compiler/assembler references indirect and relative constants.

Note that this code works perfectly as-is, so there's no pressing need to move it to assembly now, except for a cosmetic/procedural one. I'd rather get this accepted into mainline, so I can move on to the eMMC/SPI/USB drivers so people can use the code to boot an OS on our (many) Tegra2 boards coming to market RSN.

Quoted 7 lines

If some ARM / gcc assembly wizard wants to attempt moving this code to a .S file, I welcome the help - I may even attack it at a later date, when I've got more bandwidth. But it isn't a priority for me right now, unless someone on the list adamantly opposes the code as-s. But I'd expect anyone with that strong an opinion about to be able to fix it, or at least attempt it and see why I decided to defer moving it to assembly for now.

I understand your perspective, but why not spend the extra 30 minutes and do it the right way? ?Passing the buck to someone else who cares about maintaining high quality code isn't the right thing to do in my opinion. ?This patch isn't going to make it into the upcoming release, so it won't gate the other eMMC/SPI/USB drivers you want to add. ?The bar to get code into open source project generally is higher than "it works" - it has to adhere to the project's design principles and guidelines. ?U-Boot already needs cleanup as is without adding more cruft. ?Solving this small issue now results in cleaner code, less headache down the road, and shouldn't take long. ?As usual, I'm not the maintainer, so its just my $0.02.

Best,
Peter

Peter,

Quoted 69 lines · Peter Tyser

Quoted 60 lines · Tom Warren

Peter,

Quoted 37 lines · Peter Tyser

Hi Tom,
Things look pretty good. Minor comments/questions below.

<snip>

Quoted 32 lines

arch/arm/cpu/armv7/tegra2/ap20.c new file +32 -0

+/*
+ * TBD: Move cold_boot() to assembly file.
+ * Values/offsets of the table vars make this difficult.
+ */
+
+void cold_boot(void)
+{
+     asm volatile(
+             "msr    cpsr_c, #0xD3   \n"
+             /*
+             * Check current processor: CPU or AVP?
+             * If CPU, go to CPU boot code, else continue on AVP path.
+             */
+             "mov    r0, %0          \n"
+             "ldrb   r2, [r0, %1]    \n"
+             /* are we the CPU? */
+             "cmp    r2, %2          \n"
+             "mov    sp, %3          \n"
+             /*  yep, we are the CPU */
+             "bxeq   %4              \n"
+
+             /* AVP initialization follows this path */
+             "mov    sp, %5          \n"
+             /* Init and start CPU */
+             "b      startup_cpu     \n"
+             :
+             : "i"(NV_PA_PG_UP_BASE),
+             "i"(PG_UP_TAG_0),
+             "r"(proc_tag),
+             "r"(cpu_boot_stack),
+             "r"(_armboot_start),
+             "r"(avp_boot_stack)

Quoted 8 lines

arch/arm/cpu/armv7/tegra2/ap20.c line 366

Quoted 3 lines

arch/arm/cpu/armv7/tegra2/ap20.c new file +3 -0

+             : "r0", "r2", "cc", "lr"
+     );
+}

What errors did you encounter when this was in the assembly file? It'd be nice to put it there now. Likely it will never get fixed if it doesn't implemented correctly off the bat. If you post the errors perhaps someone on the list can provide insight.

I didn't capture a log of the errors when I was trying to put the cold_boot code into lowlevel_init.S. But I saw fixup errors and undefined constant errors, all related to the #defines (NV_PG_UP_BASE, avp/cpu_boot_stack, etc.) and how the compiler/assembler references indirect and relative constants.

Note that this code works perfectly as-is, so there's no pressing need to move it to assembly now, except for a cosmetic/procedural one. I'd rather get this accepted into mainline, so I can move on to the eMMC/SPI/USB drivers so people can use the code to boot an OS on our (many) Tegra2 boards coming to market RSN.

Quoted 7 lines

If some ARM / gcc assembly wizard wants to attempt moving this code to a .S file, I welcome the help - I may even attack it at a later date, when I've got more bandwidth. But it isn't a priority for me right now, unless someone on the list adamantly opposes the code as-s. But I'd expect anyone with that strong an opinion about to be able to fix it, or at least attempt it and see why I decided to defer moving it to assembly for now.

Quoted 11 lines

I understand your perspective, but why not spend the extra 30 minutes and do it the right way? Passing the buck to someone else who cares about maintaining high quality code isn't the right thing to do in my opinion. This patch isn't going to make it into the upcoming release, so it won't gate the other eMMC/SPI/USB drivers you want to add. The bar to get code into open source project generally is higher than "it works" - it has to adhere to the project's design principles and guidelines. U-Boot already needs cleanup as is without adding more cruft. Solving this small issue now results in cleaner code, less headache down the road, and shouldn't take long. As usual, I'm not the maintainer, so its just my $0.02.

FWIW, I spent _far_ more than 30 minutes on this .. close to a full day of frustration/banging my head against the wall. I have other priorities besides upstreaming Tegra2 U-Boot support, and I can't justify spending days on this. As I originally stated, I'm no expert in the intricacies of ARM asm programming - my expertise is in x86 CPUs, and hit a wall with this port to assembly.

I'm not passing the buck, and while I agree that this code is not the cleanest I've seen, I don't think I'm pushing low-quality code here.

It's 8 lines of embedded assembly, plus a table. Looking at what the C compiler produces in assembly, it's twice as long, pushes some of the tabled values on the stack and then pulls them back into registers, and is, IMHO, harder to understand. I could just cut-and-paste the compiler output into lowlevel_init.S, and add some comments, but is that really any better? Is the goal to get clean, understandable code, or messier, harder to parse code in the right files?

As to adhering to U-Boot's design principles and guidelines, could you point me to the section on embedded assembly in C files? I don't remember seeing a specific section on that topic, don't see it under U-Boot Coding Style, nor in the Linux coding guidelines, and I'd like to reference it for future use.

For now, I'm going to let this percolate, as I have other fish to fry.

FWIW, I spent _far_ more than 30 minutes on this .. close to a full day of frustration/banging my head against the wall. [...]