[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Dave Martin]

Hi,

On Tue, Dec 7, 2010 at 11:59 AM, Jean Pihet <jean.pihet at newoldbits.com> wrote:
> Hi Dave,
>
> On Tue, Dec 7, 2010 at 11:16 AM, Dave Martin <dave.martin at linaro.org> wrote:
>> On Tue, Dec 7, 2010 at 6:31 AM, Santosh Shilimkar
>> <santosh.shilimkar at ti.com> wrote:
>>> Dave,
>>>> -----Original Message-----
>>>> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
>>>> bounces at lists.linaro.org] On Behalf Of Dave Martin
>>>> Sent: Monday, December 06, 2010 11:06 PM
>>>> To: linux-arm-kernel at lists.infradead.org
>>>> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
>>>> dev at lists.linaro.org
>>>> Subject: [PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to
>>> work
>>>> withCONFIG_THUMB2_KERNEL
>>>>
>>
>> [...]
>>
>>>
>>> No need to mention but this patch changes lot of things around
>>> power management code. You said " Tested on: omap3 (Beagle xM A2)"
>>>
>>> What did you test ? Is it just boot test ? For sure just boot
>>> test is not enough for this patch and needs to test the PM.
>>
>> That's a fair point--- yes, I've only tested boot / reboot / shutdown so far.
>>
> The ASM idle code is being reworked right now, which means the T2
> support will need to be reworked on top of the patches. Cf. [1] and
> [2].
[...]
On Tue, Dec 7, 2010 at 12:59 PM, Jean Pihet <jean.pihet at newoldbits.com> wrote:
[...]
> For your information, the on-going changes are:
> - run (most of the) code from the DDR instead of the internal SRAM.
> - convert the ASM idle code to C-code,
>
> I am working on getting those changes submitted, it should be done this week.
>
> Unfortunately those changes have implications on your changes for T2.
> As far as I understood converting the code to C-code should solve the
> T2 problem as well. What do you think?
>
> In any case I will let you know. Could you test the changes when they
> are available?
[...]

In general, converting the code to C will solve a lot of potential
problems.  If you don't copy the code, that also simplifies matters.

Note that converting to C doesn't mean that code which attempts to
copy function bodies will work: you still need to handle the fact that
if f() is a C function symbol, then the value of the symbol f is
actually the function's base address + 1.  See my changes in sram.c,
pm34xx.c etc.

If there are still bits of ARM code (such as the resume re-entry
point?) some care will still be needed there.

Anyone maintaining low-level and assembler code may find the following
useful: https://wiki.ubuntu.com/ARM/Thumb2PortingHowto

>> If you have any thoughts about how to exercise the power management
>> functionality more completely, I'd be happy to have a go...
> Cf. [3] for more details on how to exercise the PM on the target. This
> wiki page is slightly outdated but the idea is still the same. Note
> that only cpuidle is currently supported correctly on l-o master.
>
> [1] http://marc.info/?l=linux-omap&m=129139584919242&w=2
> [2] http://marc.info/?l=linux-omap&m=129172034809447&w=2
> [3] http://elinux.org/OMAP_Power_Management

Thanks for this info

Humm, which reference tree should I be using?  I'm not convinced it
works properly/usefully in the linaro trees ... only OMAP_PM_NOOP is
provided, and when I echo mem >/sys/power/state, the platform does not
crash (at least, it doesn't lock up) but doesn't appear to suspend
properly either.  Also, I'm not seeing your debugs contents.


On a separate topic, I have some firmware questions you might be able to answer:
  * Does the secure firmware attempt to read the comment field from
SMC instructions?  And if so, does it assume the SMC is in ARM code?
  * Is the secure firmware able to cope with a Thumb resume re-entry point?

Currently, I've taken the conservative approach and assume that the
answer to both of these questions is "no".  But this means that a few
bits of code need to be built as ARM which could otherwise be Thumb.
It would be cleaner to get the amount of ARM code down to an absolute
minimum when building a Thumb kernel.



Some specific comments on your patches:

After the label "finished:" in sleep34xx.S
+	ldr r1, kernel_flush
+	mov lr, pc
+	bx  r1

If the code might be built as Thumb-2, this won't work correctly,
because the Thumb bit (bit 0) will not get set properly in lr.
Instead:

+	ldr r1, kernel_flush
+	blx r1

Even if you think this code isn't going to be built for Thumb-2, it's
specific to v7+ platforms, so I recommend you use the above change
anyway: it's more compact and will help avoid maintenance problems
further down the line...


+	str	r4, wait_dll_lock_counter
[...]
+	str	r4, kick_counter

PC-relative stores are illegal in Thumb-2.  Worse, current binutils
will assemble invalid code if you do this in the source and build for
Thumb-2.  For details and my fix, search for "PC-relative stores" on
http://lists.arm.linux.org.uk/lurker/message/20101130.133117.a98bf92f.en.html



+ENTRY(get_omap3630_restore_pointer)
+        stmfd   sp!, {lr}     @ save registers on stack
+	adr	r0, restore_3630

If restore_3630 is a Thumb symbol, this will go wrong, because the
"Thumb bit" won't get set in the address.  Instead, you would need to
use "adr r0, BSYM(restore_3630)", but only if restore_3630 is built as
Thumb-2 when the kernel is built with CONFIG_THUMB2_KERNEL.

Cheers
---Dave