Re: [PATCH 2/6] TI816X: Update common omap platform files

[Kevin Hilman <khilman@deeprootsystems.com>]

"Pedanekar, Hemant" <hemantp@ti.com> writes:

> Hi Kevin,
>
> Kevin Hilman wrote:
>> Hemant Pedanekar <hemantp@ti.com> writes:
>> 
>>> This patch updates the common platform files with TI816X specific
>>> additions. 
>>> 
>>> Also adds new files for TI816X modules base addresseses and irq
>>> definitions. 
>>> 
>>> Signed-off-by: Hemant Pedanekar <hemantp@ti.com>
>>> ---
> [...]
>>> 
>>> diff --git a/arch/arm/mach-omap2/include/mach/entry-macro.S
>> b/arch/arm/mach-omap2/include/mach/entry-macro.S
>>> index 50fd749..6516cbd 100644
>>> --- a/arch/arm/mach-omap2/include/mach/entry-macro.S
>>> +++ b/arch/arm/mach-omap2/include/mach/entry-macro.S @@ -34,7 +34,7 @@
>>>  		.endm
>>> 
>>>  /*
>>> - * Unoptimized irq functions for multi-omap2, 3 and 4
>>> + * Unoptimized irq functions for multi-omap2, 3, 4 and ti816x   */
>>> 
>>>  #ifdef MULTI_OMAP2
>>> @@ -57,7 +57,8 @@ omap_irq_base:	.word	0
> [...]
>>> +		bne	9998f
>>> +
>>> +		/*
>>> +		 * ti816x has additional IRQ pending register. Checking this
>>> +		 * register on omap2 & omap3 has no effect (read as 0). +		 */
>>> +		ldr	\irqnr, [\base, #0xf8] /* IRQ pending reg 4 */
>>> +		cmp	\irqnr, #0x0
>> 
>> This part makes me a slightly nervous.  At least according to
>> the TRMs,
>> this address is undefined on OMAP2 & OMAP3 (yet still in the
>> INTC block.)
>> Was this tested on OMAP2/3 hardware and verified to return 0?
>> 
>> You might also consider wrapping this section in
>> #ifdef CONFIG_ARCH_TI816X so a multi-OMAP kernel without 816x support would
>> avoid this extra read. 
>> 
>
> Won't the usage of #ifdef inside MULTI_OMAP2 make things look strange?
> E.g.,
>
> #ifdef MULTI_OMAP2
> ...
> #ifdef CONFIG_ARCH_TI816X
> ...
> #endif
> #endif
>
> (Specifically, since there is already a custom block present in #else part?)
>

Yeah, I thought about that too.  That's why I said "you might
consider..." above.

But thinking more, you're right.  When we want an optimized version for
a specific SoC, we just compile for that SoC and get the optimized
version.

Go ahead and leave out the #ifdef, but I'd still like to see some tests
on OMAP2 that show that reading this undefined part of the INTC block
does indeed return zero.

Kevin