Re: [PATCH 1/6] ARM: tegra: add an assembly marco to check Tegra SoC ID

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 05/15/2013 04:27 AM, Joseph Lo wrote:
> 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:

> @@ -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, ...

>  #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.

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?

(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.