[PATCH v4 0/5] ARM: omap[34]: Thumb-2 compatibility fixes

[Kevin Hilman]

Dave Martin <dave.martin at linaro.org> writes:

> On Mon, Feb 14, 2011 at 11:15 PM, Kevin Hilman <khilman at ti.com> wrote:
>> Hi Dave,
>>
>> Dave Martin <dave.martin at linaro.org> writes:
>>
>>> On Mon, Feb 14, 2011 at 10:00:23AM -0500, Nicolas Pitre wrote:
>>>> On Mon, 14 Feb 2011, Dave Martin wrote:
>>>>
>>>> > @@ -289,8 +297,20 @@ clean_l2:
>>>> >     *  - should be faster and will change with kernel
>>>> >     *  - 'might' have to copy address, load and jump to it
>>>> >     */
>>>> > +#ifdef CONFIG_THUMB2_KERNEL
>>>> > +  /* kernel is non-interworking : must do this from Thumb */
>>>> > +  adr     r1, . + 1
>>>> > +  bx      r1
>>>> > +  .thumb
>>>> > +#endif
>>>> >    ldr     r1, kernel_flush
>>>>
>>>> Didn't you mean this instead:
>>>>
>>>>      /* kernel is non-interworking : must do this from Thumb */
>>>>      adr     r1, 1f + 1
>>>>      bx      r1
>>>>      .thumb
>>>> 1:   ldr     r1, kernel_flush
>>>>      ...
>>>
>>> Note that this is intended as an experimental hack, not a real patch
>>> (apologies if I didn't make that clear...)
>>>
>>> Well, actually I meant "add r1, pc, #1" ... which means I was too
>>> busy trying to be clever... oops!
>>>
>>> That is of course exactly equivalent to your code...
>>>
>>>>
>>>> ?
>>>>
>>>> >    blx     r1
>>>> > +#ifdef CONFIG_THUMB2_KERNEL
>>>> > +  .align
>>>> > +  bx      pc
>>>> > +  nop
>>>> > +  .arm
>>>>
>>>> Also here, the .align has the potential to introduce a zero halfword in
>>>> the instruction stream before the bx.  What about:
>>>>
>>>>      adr     r3, 1f
>>>>      bx      r3
>>>>      .align
>>>>      .arm
>>>> 1:   ...
>>>
>>> .align inserts a 16-bit nop when misaligned in Thumb in a text section,
>>> and a word-aligned bx pc is a specific architecturally allowed way
>>> to do an inline switch to ARM.  The linker uses this trick for PLT
>>> veneers etc.
>>>
>>> A nicer fix for doing this sort of call from low-level code which
>>> might be ARM is to convert arch/arm/mm/*-v7.S to use "bx lr" to return.
>>>
>>> Generally, we can do this for all arches >= v5, without any
>>> incompatibility.  However, since the need for it will be rare and it
>>> will generate patch noise for not much real benefit,
>>> I haven't proposed this.
>>>
>>> Updated patch below.
>>
>> I tested the updated patch on top of your "dirty" branch I tested with
>> last week, and now see off-mode working just fine.
>
> Thanks for testing-- that's great news.
>
> I will have a think about whether the patch can be tidied up to revert
> most of the code back to Thumb, though that isn't essential.  If I've
> understood what's going on correctly, I think only the restore entry
> points, SMC call sites and the entry to omap3_sram_configure_core_dpll
> could need to be ARM; the rest shouldn't really make any difference...

Yes, that sounds right.

Kevin