Re: [PATCH V5 5/5] arm: mvebu: Added SMP support for Armada XP

On Wed, Nov 21, 2012 at 11:57:12AM +0000, Gregory CLEMENT wrote:

On 11/21/2012 11:41 AM, Will Deacon wrote:

quoted

quoted

+/*
+ * Armada XP specific entry point for secondary CPUs.
+ * We add the CPU to the coherency fabric and then jump to secondary
+ * startup
+ */
+ENTRY(armada_xp_secondary_startup)
+
+	/* Read CPU id */
+ 	mrc     p15, 0, r1, c0, c0, 5
+	and     r1, r1, #0xF
+
+	/* Add CPU to coherency fabric */
+	ldr     r0, = ARMADA_XP_CFB_BASE
+	mov	lr, pc
+
+	b	ll_set_cpu_coherent
+	b	secondary_startup

Why didn't you use my adr suggestion here?

Because when I used BSYM(secondary_startup), I get the following
message during compilation:

Error: undefined symbol secondary_startup used as an immediate value

Sorry, my mistake (I was thinking on my feet) since secondary_startup is
declared in a C file, right? How about:

	ldr	lr, =secondary_startup
	b	ll_set_cpu_coherent

And I didn't want to miss the merge window for something which could be
fixed during the v3.8-rc1 if needed.

Sure, but it also seems daft to merge something that's nine tenths of the
way there when it's straight-forward to finish it off properly.

Will

On Wed, Nov 21, 2012 at 12:22:51PM +0000, Will Deacon wrote:

Sorry, my mistake (I was thinking on my feet) since secondary_startup is
declared in a C file, right? How about:

	ldr	lr, =secondary_startup
	b	ll_set_cpu_coherent

Why?  Do we really want LR to be the _virtual_ address?  No we don't,
because secondary_startup expects to be called with the MMU off.

So I go back to my original comment.  What's wrong with:

	bl	ll_set_cpu_coherent
	b	secondary_startup

?