On Thu, 2013-05-16 at 06:43 +0800, Stephen Warren wrote:

On 05/15/2013 04:27 AM, Joseph Lo wrote:

quoted

There are some Tegra SoC ID checking code around the low level assembly
code. Adding a marco to replace them. For the single image to support all
the Tegra series, we may also need the marco in other common code. So we
make it become a marco for the usage.

I'm not sure this patch doesn't obfuscate the code too much. The big
issue I see is:

quoted

@@ -115,13 +112,9 @@ ENTRY(__tegra_cpu_reset_handler)
 
 	cpsid	aif, 0x13			@ SVC mode, interrupts disabled
 
-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
-	and	r6, r6, #0xff00
-#ifdef CONFIG_ARCH_TEGRA_2x_SOC
-t20_check:
-	cmp	r6, #(0x20 << 8)
+	tegra_check_soc_id TEGRA20, TEGRA_APB_MISC_BASE, r6, r5

Here, we replace all the code with the new macro, ...

quoted

 #ifdef CONFIG_ARCH_TEGRA_2x_SOC
 	/* Are we on Tegra20? */
-	cmp	r6, #(0x20 << 8)
+	cmp	r6, #(TEGRA20 << 8)

But here we still have to write out the cmp instructions.

The marco did "cmp" once and move the soc_id into r6. But the flags may
be changed after running some other codes, we need to "cmp" again to get
the correct flag.

It may be reasonable to create a macro to read/mask/shift the SoC ID,
similar to the existing cpu_id macro, i.e. roughly:

/* Macro to check Tegra revision */
+#define APB_MISC_GP_HIDREV	0x804
+.macro tegra_soc_id base, tmp1, tmp2
+	mov32	\tmp2, \base
+	ldr	\tmp1, [\tmp2, #APB_MISC_GP_HIDREV]
+	and	\tmp1, \tmp1, #0xff00
+.endm

Also, I wonder:

(a) why the need to pass TEGRA_APB_MISC_BASE into the macro; can't the
macro just hard-code that value since it's always the same?

Because this marco will be used in virtual address space too, I need to
pass different base addr here.

(b) Why need two registers passed to the macro. At least one of the code
segments you've replaced with the macro uses a single register instead:

-	mov32	r6, TEGRA_APB_MISC_BASE
-	ldr	r6, [r6, #APB_MISC_GP_HIDREV]
-	and	r6, r6, #0xff00

Wouldn't that be a better implementation of the macro? I don't think
relying on \tmp2 containing "base" after the macro invocation would be a
good idea, since that's rather like looking inside the black box.

OK. I can remove one tmp register here.