[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Dave Martin]

On Wed, Apr 6, 2011 at 12:36 PM, Frank Hofmann <frank.hofmann@tomtom.com> wrote:
>
>> Date: Wed, ?6 Apr 2011 11:29:47 +0100
>> From: Dave Martin <dave.martin@linaro.org>
>> To: linux-arm-kernel at lists.infradead.org
>> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>, ? Russell King - ARM Linux
>> ? ? ? ?<linux@arm.linux.org.uk>, patches at linaro.org
>> Subject: [RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for
>> ? ? ? ?Thumb-2
>> Message-ID: <1302085787-15069-1-git-send-email-dave.martin@linaro.org>
>>
>> * To remove the risk of inconvenient register allocation decisions
>> ?by the compiler, these functions are separated out as pure
>> ?assembler.
>>
>> * The apcs frame manipulation code is not applicable for Thumb-2
>> ?(and also not easily compatible). ?Since it's not essential to
>> ?have a full frame on these leaf assembler functions, the frame
>> ?manipulation is removed, in the interests of simplicity.
>>
>> * Split up ldm/stm instructions to be compatible with Thumb-2,
>> ?as well as avoiding instruction forms deprecated on >= ARMv7.
>
> Hi Dave,
>
> I wonder why the C wrapper set_fiq_regs() is necessary; should be just as
> simple to just have the assembly function called that ?

The main reason for that is to code the ->ARM_r8 in C (as in the
existing code).  I feel safer doing that than hard-coding an offset in
the assembler, though possibly that is overkill since if the layout of
struct pt_regs changes we are probably in trouble for all kinds of
other reasons anyway...

>
> Thing is, the "inline" doesn't force what you're hoping for - it'll still
> create the symbol, because the definition is not known at compile time for
> code using it.

Well, I'm not hoping for the assembly to be inlined, *just* the C
wrapper.  Since the C wrapper is trivial, I prefer to avoid emitting a
function body for this.

>
> Or else leave it inline assembly, how about:
>
> void __naked set_fiq_regs(struct pt_regs *regs)
> {
> asm volatile(
> ? ? ? ?"mov ? ?r2, #(PSR_I_BIT | PSR_F_BIT | FIQ_MODE)\n"
> ? ? ? ?"mrs ? ?r1, cpsr\n"
> ? ? ? ?"msr ? ?cpsr_c, r2\n" ? ? ? ? ? /* select FIQ mode */
> ? ? ? ?"mov ? ?r0, r0\n" ? ? ? ? ? ? ? /* avoid hazard prior to ARMv4 */
> ? ? ? ?"ldmia ?%0!, {r8 - r12}\n"
> ? ? ? ?"ldr ? ?sp, [%0], #4\n"
> ? ? ? ?"ldr ? ?lr, [%0]\n"
> ? ? ? ?"msr ? ?cpsr_c, r1\n" ? ? ? ? ? /* return to SVC mode */
> ? ? ? ?"mov ? ?r0, r0\n" ? ? ? ? ? ? ? /* avoid hazard prior to ARMv4 */
> ? ? ? ?"mov ? ?pc, lr\n"
> ? ? ? ?: : "r" (&regs->ARM_r8) : "r1", "r2");
> }

The main reason for making it pure assembler is there's nothing to
stop the compiler register allocator from using, say, r12 -- that
would break the code because once we switch info FIQ mode the CPU sees
a different r12 from the one the compiler initialised.

I could have used register variables with explicit register
assignments.  Alternatively, I could have added r8-r14 to the clobber
list -- but then the compiler would generate unnecessary save and
restore sequences for all those registers.

However, really all these hacks are attempts to suppress the
compiler's compiler behaviour.  Since we're doing this for complete
functions which we don't need to inline, it seems simplest to achieve
that by taking the compiler out of the situation altogether.

Also, note that PSR_I_BIT | PSR_F_BIT | FIQ_MODE probably has to be an
argument with an "r" constraint -- I don't think the macros will get
expanded if it's embedded in the strings as above.

Does that answer your concerns?

It could be argued that moving these functions is unnecessary churn
for not too much benefit -- if people feel strongly about that, the
functions could still be implemented in inline asm as per your
suggestion.

Cheers
---Dave

>
> FrankH.
>
>>
>> Signed-off-by: Dave Martin <dave.martin@linaro.org>
>> ---
>>
>> -/*
>> - * Taking an interrupt in FIQ mode is death, so both these functions
>> - * disable irqs for the duration. ?Note - these functions are almost
>> - * entirely coded in assembly.
>> - */
>> -void __naked set_fiq_regs(struct pt_regs *regs)
>> -{
>> - ? ? ? register unsigned long tmp;
>> - ? ? ? asm volatile (
>> - ? ? ? "mov ? ?ip, sp\n\
>> - ? ? ? stmfd ? sp!, {fp, ip, lr, pc}\n\
>> - ? ? ? sub ? ? fp, ip, #4\n\
>> - ? ? ? mrs ? ? %0, cpsr\n\
>> - ? ? ? msr ? ? cpsr_c, %2 ? ? ?@ select FIQ mode\n\
>> - ? ? ? mov ? ? r0, r0\n\
>> - ? ? ? ldmia ? %1, {r8 - r14}\n\
>> - ? ? ? msr ? ? cpsr_c, %0 ? ? ?@ return to SVC mode\n\
>> - ? ? ? mov ? ? r0, r0\n\
>> - ? ? ? ldmfd ? sp, {fp, sp, pc}"
>> - ? ? ? : "=&r" (tmp)
>> - ? ? ? : "r" (&regs->ARM_r8), "I" (PSR_I_BIT | PSR_F_BIT | FIQ_MODE));
>> -}
>> -
>> -void __naked get_fiq_regs(struct pt_regs *regs)
>> -{
>> - ? ? ? register unsigned long tmp;
>> - ? ? ? asm volatile (
>> - ? ? ? "mov ? ?ip, sp\n\
>> - ? ? ? stmfd ? sp!, {fp, ip, lr, pc}\n\
>> - ? ? ? sub ? ? fp, ip, #4\n\
>> - ? ? ? mrs ? ? %0, cpsr\n\
>> - ? ? ? msr ? ? cpsr_c, %2 ? ? ?@ select FIQ mode\n\
>> - ? ? ? mov ? ? r0, r0\n\
>> - ? ? ? stmia ? %1, {r8 - r14}\n\
>> - ? ? ? msr ? ? cpsr_c, %0 ? ? ?@ return to SVC mode\n\
>> - ? ? ? mov ? ? r0, r0\n\
>> - ? ? ? ldmfd ? sp, {fp, sp, pc}"
>> - ? ? ? : "=&r" (tmp)
>> - ? ? ? : "r" (&regs->ARM_r8), "I" (PSR_I_BIT | PSR_F_BIT | FIQ_MODE));
>> -}
>> -
>> int claim_fiq(struct fiq_handler *f)
>> {
>> ? ? ? ?int ret = 0;
>> @@ -174,8 +133,8 @@ void disable_fiq(int fiq)
>> }
>>
>> EXPORT_SYMBOL(set_fiq_handler);
>> -EXPORT_SYMBOL(set_fiq_regs);
>> -EXPORT_SYMBOL(get_fiq_regs);
>> +EXPORT_SYMBOL(__set_fiq_regs); /* defined in fiqasm.S */
>> +EXPORT_SYMBOL(__get_fiq_regs); /* defined in fiqasm.S */
>> EXPORT_SYMBOL(claim_fiq);
>> EXPORT_SYMBOL(release_fiq);
>> EXPORT_SYMBOL(enable_fiq);
>> diff --git a/arch/arm/kernel/fiqasm.S b/arch/arm/kernel/fiqasm.S
>> new file mode 100644
>> index 0000000..0504bb8
>> --- /dev/null
>> +++ b/arch/arm/kernel/fiqasm.S
>> @@ -0,0 +1,49 @@
>> +/*
>> + * ?linux/arch/arm/kernel/fiqasm.S
>> + *
>> + * ?Derived from code originally in linux/arch/arm/kernel/fiq.c:
>> + *
>> + * ?Copyright (C) 1998 Russell King
>> + * ?Copyright (C) 1998, 1999 Phil Blundell
>> + * ?Copyright (C) 2011, Linaro Limited
>> + *
>> + * ?FIQ support written by Philip Blundell <philb@gnu.org>, 1998.
>> + *
>> + * ?FIQ support re-written by Russell King to be more generic
>> + *
>> + * ?v7/Thumb-2 compatibility modifications by Linaro Limited, 2011.
>> + */
>> +
>> +#include <arm/assembler.h>
>> +
>> +/*
>> + * Taking an interrupt in FIQ mode is death, so both these functions
>> + * disable irqs for the duration. ?Note - these functions are almost
>> + * entirely coded in assembly.
>> + */
>> +
>> +ENTRY(__set_fiq_regs)
>> + ? ? ? mov ? ? r2, #PSR_I_BIT | PSR_F_BIT | FIQ_MODE
>> + ? ? ? mrs ? ? r1, cpsr
>> + ? ? ? msr ? ? cpsr_c, r2 ? ? ?@ select FIQ mode
>> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
>> + ? ? ? ldmia ? r0!, {r8 - r12}
>> + ? ? ? ldr ? ? sp, [r0], #4
>> + ? ? ? ldr ? ? lr, [r0]
>> + ? ? ? msr ? ? cpsr_c, r1 ? ? ?@ return to SVC mode
>> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
>> + ? ? ? mov ? ? pc, lr
>> +ENDPROC(__set_fiq_regs)
>> +
>> +ENTRY(__get_fiq_regs)
>> + ? ? ? mov ? ? r2, #PSR_I_BIT | PSR_F_BIT | FIQ_MODE
>> + ? ? ? mrs ? ? r1, cpsr
>> + ? ? ? msr ? ? cpsr_c, r2 ? ? ?@ select FIQ mode
>> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
>> + ? ? ? stmia ? r0!, {r8 - r12}
>> + ? ? ? str ? ? sp, [r0], #4
>> + ? ? ? str ? ? lr, [r0]
>> + ? ? ? msr ? ? cpsr_c, r1 ? ? ?@ return to SVC mode
>> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
>> + ? ? ? mov ? ? pc, lr
>> +ENDPROC(__get_fiq_regs)
>> --
>> 1.7.1
>>
>>
>>
>>
>> ------------------------------
>

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Frank Hofmann]

On Thu, 7 Apr 2011, Dave Martin wrote:

[ ... ]
> Well, I'm not hoping for the assembly to be inlined, *just* the C
> wrapper.  Since the C wrapper is trivial, I prefer to avoid emitting a
> function body for this.

_That_ specific bit is what isn't going to happen; it's a separate 
compilation unit, it needs to be callable from places not known at compile 
nor link time, and the function isn't "static". The "inline" is 
meaningless here.

Making set_fiq_regs() a wrapper in C will emit the symbol and the code for 
it, just objdump -d vmlinux.o and see what you find - set_fiq_regs _will_ 
exist with your change (caveat: it's not even compiled, normally, see 
below):

0000560c <get_fiq_regs>:
     560c:       e2800020        add     r0, r0, #32     ; 0x20
     5610:       eafffffe        b       56f8 <__get_fiq_regs>

00005614 <set_fiq_regs>:
     5614:       e2800020        add     r0, r0, #32     ; 0x20
     5618:       eafffffe        b       56d0 <__set_fiq_regs>


There's no other way of forcing the thing to be actually inlined into the 
code of the callers but putting it into fiq.h instead:

#define	set_fiq_regs(arg)	__set_fiq_regs(&((arg)->ARM_r8))

But then if you actually want to go that route, you can make it inline 
assembly right at that place, like e.g. the 64bit division stuff.




A bit more on "not even compiled normally":

Many architectures don't seem to compile FIQ in by default; fiq.c 
references "FIQ_START" and doesn't compile if the define doesn't exist. 
It's only defined for a few:

arch/arm/mach-at91/include/mach/irqs.h		46 #define FIQ_START AT91_ID_FIQ
arch/arm/mach-rpc/include/mach/irqs.h		43 #define FIQ_START               64
arch/arm/mach-s3c2410/include/mach/irqs.h	200 #define FIQ_START               IRQ_EINT0
arch/arm/mach-stmp378x/include/mach/irqs.h	92 #define FIQ_START               IRQ_DEBUG_UART
arch/arm/mach-stmp37xx/include/mach/irqs.h	94 #define FIQ_START                       IRQ_TIMER0
arch/arm/plat-mxc/include/mach/irqs.h		72 #define FIQ_START       0
arch/arm/plat-omap/include/plat/irqs.h		450 #define FIQ_START               1024
arch/arm/plat-s3c24xx/irq.c			505                 offs = irq - FIQ_START;

Also inconsistent here is that some of those do a "select FIQ" from 
arch/arm/Kconfig while others (like OMAP) seem to expect a "CONFIG_FIQ=y" 
from defconfig.


In all likelihood, it's not being compiled ...



Which also explains that your fiqasm.S code, as posted, doesn't compile - 
it lacks #include <linux/linkage.h> for the ENTRY/ENDPROC, and 
<arm/assembler.h> doesn't exist what you seem to mean is <asm/assembler.h> 
...

Except that all you use from there is the #include <asm/ptrace.h> (for 
the cpsr bit definitions).


[ ... ]
> Does that answer your concerns?

I agree with you that this should be assembly, no arguing it's better to 
nip gcc's optimizer escapades in the bud ...

The urgency I don't get, though; this code still looks a bit "hot".



FrankH.

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Dave Martin]

On Thu, Apr 7, 2011 at 12:35 PM, Frank Hofmann <frank.hofmann@tomtom.com> wrote:
> On Thu, 7 Apr 2011, Dave Martin wrote:
>
> [ ... ]
>>
>> Well, I'm not hoping for the assembly to be inlined, *just* the C
>> wrapper. ?Since the C wrapper is trivial, I prefer to avoid emitting a
>> function body for this.
>
> _That_ specific bit is what isn't going to happen; it's a separate
> compilation unit, it needs to be callable from places not known at compile
> nor link time, and the function isn't "static". The "inline" is meaningless
> here.

The wrapper functions _are_ static:

+static inline void set_fiq_regs(struct pt_regs const *regs)
+{
+       __set_fiq_regs(&regs->ARM_r8);
+}
+
+static inline void get_fiq_regs(struct pt_regs *regs)
+{
+       __get_fiq_regs(&regs->ARM_r8);
+}

>
> Making set_fiq_regs() a wrapper in C will emit the symbol and the code for
> it, just objdump -d vmlinux.o and see what you find - set_fiq_regs _will_
> exist with your change (caveat: it's not even compiled, normally, see
> below):
>
> 0000560c <get_fiq_regs>:
> ? ?560c: ? ? ? e2800020 ? ? ? ?add ? ? r0, r0, #32 ? ? ; 0x20
> ? ?5610: ? ? ? eafffffe ? ? ? ?b ? ? ? 56f8 <__get_fiq_regs>
>
> 00005614 <set_fiq_regs>:
> ? ?5614: ? ? ? e2800020 ? ? ? ?add ? ? r0, r0, #32 ? ? ; 0x20
> ? ?5618: ? ? ? eafffffe ? ? ? ?b ? ? ? 56d0 <__set_fiq_regs>
>

I'm not sure why you're observing this.  To give a random example,
here's the kind of code I observe at a call site:

...
   e:   6858            ldr     r0, [r3, #4]
  10:   f500 50fe       add.w   r0, r0, #8128   ; 0x1fc0
  14:   3010            adds    r0, #16
  16:   f7ff fffe       bl      0 <__set_fiq_regs>
  1a:   4819            ldr     r0, [pc, #100]  ; (68 <bdi_init+0x68>)
  1c:   f7ff fffe       bl      0 <bdi_init>
  20:   4601            mov     r1, r0
...

It's been fully inlined -- including merging the pointer arithmetic
with the previous calculation.

>
> There's no other way of forcing the thing to be actually inlined into the
> code of the callers but putting it into fiq.h instead:
>
> #define set_fiq_regs(arg) ? ? ? __set_fiq_regs(&((arg)->ARM_r8))

The wrapper functions _are_ in asm/fiq.h, and (disregarding
type-checking) they're equivalent to your macro.

Documentation/CodingStyle:610:Generally, inline functions are
preferable to macros resembling functions.

>
> But then if you actually want to go that route, you can make it inline
> assembly right at that place, like e.g. the 64bit division stuff.

Probably not necessary -- the original code wasn't inline either, and
I wouldn't expect this to be on any performance critical path.

> A bit more on "not even compiled normally":
>
> Many architectures don't seem to compile FIQ in by default; fiq.c references
> "FIQ_START" and doesn't compile if the define doesn't exist. It's only
> defined for a few:
>
> arch/arm/mach-at91/include/mach/irqs.h ? ? ? ? ?46 #define FIQ_START
> AT91_ID_FIQ
> arch/arm/mach-rpc/include/mach/irqs.h ? ? ? ? ? 43 #define FIQ_START
> ? ? ? 64
> arch/arm/mach-s3c2410/include/mach/irqs.h ? ? ? 200 #define FIQ_START
> ? ? ? ? IRQ_EINT0
> arch/arm/mach-stmp378x/include/mach/irqs.h ? ? ?92 #define FIQ_START
> ? ? ? IRQ_DEBUG_UART
> arch/arm/mach-stmp37xx/include/mach/irqs.h ? ? ?94 #define FIQ_START
> ? ? ? ? ? ? ? IRQ_TIMER0
> arch/arm/plat-mxc/include/mach/irqs.h ? ? ? ? ? 72 #define FIQ_START ? ? ? 0
> arch/arm/plat-omap/include/plat/irqs.h ? ? ? ? ?450 #define FIQ_START
> ? ? ? ? 1024
> arch/arm/plat-s3c24xx/irq.c ? ? ? ? ? ? ? ? ? ? 505 ? ? ? ? ? ? ? ? offs =
> irq - FIQ_START;
>
> Also inconsistent here is that some of those do a "select FIQ" from
> arch/arm/Kconfig while others (like OMAP) seem to expect a "CONFIG_FIQ=y"
> from defconfig.
>
> In all likelihood, it's not being compiled ...

Indeed -- most people aren't compiling at and so this is not an issue for them.

The tree and config I was working with is definitely using this stuff,
and giving me build errors as a result -- hence the desire for the
fix.

> Which also explains that your fiqasm.S code, as posted, doesn't compile - it
> lacks #include <linux/linkage.h> for the ENTRY/ENDPROC, and
> <arm/assembler.h> doesn't exist what you seem to mean is <asm/assembler.h>
> ...

That is true-- there are numerous minor errors in that first post; see
my repost for a fixed version.

>
> Except that all you use from there is the #include <asm/ptrace.h> (for the
> cpsr bit definitions).

IIUC, <arm/assembler.h> establishes the standard environment for .S
files for arm, which includes <asm/ptrace.h>.  kernel/entry-header.S
gets those bit definitions by the same route, for example.

>
>
> [ ... ]
>>
>> Does that answer your concerns?
>
> I agree with you that this should be assembly, no arguing it's better to nip
> gcc's optimizer escapades in the bud ...
>
> The urgency I don't get, though; this code still looks a bit "hot".

Well, I didn't say it was urgent...  What do you mean by "hot"?

Cheers
---Dave

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Frank Hofmann]

On Thu, 7 Apr 2011, Dave Martin wrote:

> On Thu, Apr 7, 2011 at 12:35 PM, Frank Hofmann <frank.hofmann@tomtom.com> wrote:
>> On Thu, 7 Apr 2011, Dave Martin wrote:
>>
>> [ ... ]
>>>
>>> Well, I'm not hoping for the assembly to be inlined, *just* the C
>>> wrapper. ?Since the C wrapper is trivial, I prefer to avoid emitting a
>>> function body for this.
>>
>> _That_ specific bit is what isn't going to happen; it's a separate
>> compilation unit, it needs to be callable from places not known at compile
>> nor link time, and the function isn't "static". The "inline" is meaningless
>> here.
>
> The wrapper functions _are_ static:
>
> +static inline void set_fiq_regs(struct pt_regs const *regs)
> +{
> +       __set_fiq_regs(&regs->ARM_r8);
> +}
> +
> +static inline void get_fiq_regs(struct pt_regs *regs)
> +{
> +       __get_fiq_regs(&regs->ARM_r8);
> +}

Ah ok ... now I get that bit, sorry <facepalm> - I had a patch mixup ...
You're right. When in the header these are fine.

Thx for the coding style pointer :)


> IIUC, <arm/assembler.h> establishes the standard environment for .S
> files for arm, which includes <asm/ptrace.h>.  kernel/entry-header.S
> gets those bit definitions by the same route, for example.

You're sure ? Sorry for being nitpicky there. "arm" vs. "asm", single 
character typo.

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/kernel/entry-header.S;h=051166c2a932cfed1620bb3a5612383ffff12149;hb=HEAD#l4

says:

4 #include <asm/assembler.h>

"grep -r arm/assembler.h" on my tree comes out empty.

Besides, without <linux/linkage.h> the ENTRY/ENDPROC aren't defined; where 
does your environment pick them up from ? <asm/assembler.h> doesn't 
include it.

>
>>
>>
>> [ ... ]
>>>
>>> Does that answer your concerns?
>>
>> I agree with you that this should be assembly, no arguing it's better to nip
>> gcc's optimizer escapades in the bud ...
>>
>> The urgency I don't get, though; this code still looks a bit "hot".
>
> Well, I didn't say it was urgent...  What do you mean by "hot"?

Not perfect ;-)

FrankH.

>
> Cheers
> ---Dave
>

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Dave Martin]

On Thu, Apr 7, 2011 at 3:24 PM, Frank Hofmann <frank.hofmann@tomtom.com> wrote:
> On Thu, 7 Apr 2011, Dave Martin wrote:
>
>> On Thu, Apr 7, 2011 at 12:35 PM, Frank Hofmann <frank.hofmann@tomtom.com>
>> wrote:
>>>
>>> On Thu, 7 Apr 2011, Dave Martin wrote:
>>>
>>> [ ... ]
>>>>
>>>> Well, I'm not hoping for the assembly to be inlined, *just* the C
>>>> wrapper. ?Since the C wrapper is trivial, I prefer to avoid emitting a
>>>> function body for this.
>>>
>>> _That_ specific bit is what isn't going to happen; it's a separate
>>> compilation unit, it needs to be callable from places not known at
>>> compile
>>> nor link time, and the function isn't "static". The "inline" is
>>> meaningless
>>> here.
>>
>> The wrapper functions _are_ static:
>>
>> +static inline void set_fiq_regs(struct pt_regs const *regs)
>> +{
>> + ? ? ? __set_fiq_regs(&regs->ARM_r8);
>> +}
>> +
>> +static inline void get_fiq_regs(struct pt_regs *regs)
>> +{
>> + ? ? ? __get_fiq_regs(&regs->ARM_r8);
>> +}
>
> Ah ok ... now I get that bit, sorry <facepalm> - I had a patch mixup ...
> You're right. When in the header these are fine.
>
> Thx for the coding style pointer :)
>
>
>> IIUC, <arm/assembler.h> establishes the standard environment for .S
>> files for arm, which includes <asm/ptrace.h>. ?kernel/entry-header.S
>> gets those bit definitions by the same route, for example.

Argh, that's me repeating the same typo I had in the patch...  yes, it
should be <asm/assembler.h>

>
> You're sure ? Sorry for being nitpicky there. "arm" vs. "asm", single
> character typo.
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/kernel/entry-header.S;h=051166c2a932cfed1620bb3a5612383ffff12149;hb=HEAD#l4
>
> says:
>
> 4 #include <asm/assembler.h>
>
> "grep -r arm/assembler.h" on my tree comes out empty.
>
> Besides, without <linux/linkage.h> the ENTRY/ENDPROC aren't defined; where
> does your environment pick them up from ? <asm/assembler.h> doesn't include
> it.

Yup. <linux/linkage.h> is definitley needed too.

>
>>
>>>
>>>
>>> [ ... ]
>>>>
>>>> Does that answer your concerns?
>>>
>>> I agree with you that this should be assembly, no arguing it's better to
>>> nip
>>> gcc's optimizer escapades in the bud ...
>>>
>>> The urgency I don't get, though; this code still looks a bit "hot".
>>
>> Well, I didn't say it was urgent... ?What do you mean by "hot"?
>
> Not perfect ;-)

Ah, well that was perfectly true ;)

Take a look at the v2 I posted to the list this morning ... I believe
it fixes all those issues.

Cheers
---Dave

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Russell King - ARM Linux]

On Thu, Apr 07, 2011 at 11:02:18AM +0100, Dave Martin wrote:
> The main reason for that is to code the ->ARM_r8 in C (as in the
> existing code).  I feel safer doing that than hard-coding an offset in
> the assembler, though possibly that is overkill since if the layout of
> struct pt_regs changes we are probably in trouble for all kinds of
> other reasons anyway...

It may be worth looking up exactly what's allowed with naked functions.
A naked function tells the compiler to omit the function prologue and
epologue, which effectively means you can't do very much other than
inline asm in them.  So I think we're pretty safe from register
allocation issues.

If the compiler was to do register allocation for &regs->ARM_r8, and it
landed up in r8, then that would not only break the code, but also break
the ABI as the compiler would be unable to save the value of r8.

> I could have used register variables with explicit register
> assignments.  Alternatively, I could have added r8-r14 to the clobber
> list -- but then the compiler would generate unnecessary save and
> restore sequences for all those registers.

I doubt it would for a naked function.  You're expected to handle
that stuff yourself with such things.

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Dave Martin]

On Thu, Apr 7, 2011 at 11:28 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Apr 07, 2011 at 11:02:18AM +0100, Dave Martin wrote:
>> The main reason for that is to code the ->ARM_r8 in C (as in the
>> existing code). ?I feel safer doing that than hard-coding an offset in
>> the assembler, though possibly that is overkill since if the layout of
>> struct pt_regs changes we are probably in trouble for all kinds of
>> other reasons anyway...
>
> It may be worth looking up exactly what's allowed with naked functions.
> A naked function tells the compiler to omit the function prologue and
> epologue, which effectively means you can't do very much other than
> inline asm in them. ?So I think we're pretty safe from register
> allocation issues.
>
> If the compiler was to do register allocation for &regs->ARM_r8, and it
> landed up in r8, then that would not only break the code, but also break
> the ABI as the compiler would be unable to save the value of r8.
>
>> I could have used register variables with explicit register
>> assignments.  Alternatively, I could have added r8-r14 to the clobber
>> list -- but then the compiler would generate unnecessary save and
>> restore sequences for all those registers.
>
> I doubt it would for a naked function.  You're expected to handle
> that stuff yourself with such things.
>

It looks like your instinct is correct -- if I add those extra
registers to the clobber list, I get no save/resrote sequences, as you
suggest... but...

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Frank Hofmann]

On Fri, 8 Apr 2011, Dave Martin wrote:

> On Thu, Apr 7, 2011 at 11:28 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Apr 07, 2011 at 11:02:18AM +0100, Dave Martin wrote:
>>> The main reason for that is to code the ->ARM_r8 in C (as in the
>>> existing code). ?I feel safer doing that than hard-coding an offset in
>>> the assembler, though possibly that is overkill since if the layout of
>>> struct pt_regs changes we are probably in trouble for all kinds of
>>> other reasons anyway...
>>
>> It may be worth looking up exactly what's allowed with naked functions.
>> A naked function tells the compiler to omit the function prologue and
>> epologue, which effectively means you can't do very much other than
>> inline asm in them. ?So I think we're pretty safe from register
>> allocation issues.
>>
>> If the compiler was to do register allocation for &regs->ARM_r8, and it
>> landed up in r8, then that would not only break the code, but also break
>> the ABI as the compiler would be unable to save the value of r8.
>>
>>> I could have used register variables with explicit register
>>> assignments.  Alternatively, I could have added r8-r14 to the clobber
>>> list -- but then the compiler would generate unnecessary save and
>>> restore sequences for all those registers.
>>
>> I doubt it would for a naked function.  You're expected to handle
>> that stuff yourself with such things.
>>
>
> It looks like your instinct is correct -- if I add those extra
> registers to the clobber list, I get no save/resrote sequences, as you
> suggest... but...

Just some observation on inline assembly behaviour ...

If gcc in its wisdom decides it wants a register, it'll take that 
register, in the sense that a clobber list r0-r12 will make compiles fail 
even if gcc theoretically were able to restore every item from the stack 
and/or first principles. But if you leave it a way out, then imagine 
the compiler deciding to want your R8 reg, and doing code like:

 	mov	r12, r8
 	... your inline assembly goes here
 	mov	r8, r12

because you left it a nonclobbered register to store things in ...

I've tried that once, with inline assembly that had a "ldm r0,{r0-r14}" 
and the whole function operated on nothing else at all but global 
variables. gcc wanted to keep the global pointer in a reg, and refused to 
re-load that reg after that ldm, and whinced about the compile when giving 
it a "full" clobber list".


Anyway, re __naked: See <linux/compiler-gcc.h> for the goodie:

/*
  * it doesn't make sense on ARM (currently the only user of __naked) to trace
  * naked functions because then mcount is called without stack and frame pointer
  * being set up and there is no chance to restore the lr register to the value
  * before mcount was called.
  */
#define __naked                         __attribute__((naked)) notrace

who knows what other nice autoextensions beyond -finstrument-functions gcc 
ultimately will come up with are going to have effects that end up 
"clothing" naked functions nicely.


In the end, fully agree that Dave is right, an assembler sourcefile is the 
best place.

FrankH.

>
> From the GCC info docs:
> `naked'
>     Use this attribute on the ARM, AVR, IP2K and SPU ports to indicate
>     that the specified function does not need prologue/epilogue
>     sequences generated by the compiler.  It is up to the programmer
>     to provide these sequences. The only statements that can be safely
>     included in naked functions are `asm' statements that do not have
>     operands.  All other statements, including declarations of local
>     variables, `if' statements, and so forth, should be avoided.
>     Naked functions should be used to implement the body of an
>     assembly function, while allowing the compiler to construct the
>     requisite function declaration for the assembler.
>
> ... but we don't have an asm statement with no operands and no
> accompanying declarations in this case.  Logically the code ought to
> work if the compiler doesn't spill anything to the stack, and explicit
> register variables ought to avoid this provided we don't have too
> many.
>
> But I doubt whether this behaviour is well tested by the compiler guys.
>
> Cheers
> ---Dave
>

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Dave Martin]

 * To remove the risk of inconvenient register allocation decisions
   by the compiler, these functions are separated out as pure
   assembler.

 * The apcs frame manipulation code is not applicable for Thumb-2
   (and also not easily compatible).  Since it's not essential to
   have a full frame on these leaf assembler functions, the frame
   manipulation is removed, in the interests of simplicity.

 * Split up ldm/stm instructions to be compatible with Thumb-2,
   as well as avoiding instruction forms deprecated on >= ARMv7.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---

 arch/arm/include/asm/fiq.h |   16 ++++++++++++-
 arch/arm/kernel/Makefile   |    2 +-
 arch/arm/kernel/fiq.c      |   45 +--------------------------------------
 arch/arm/kernel/fiqasm.S   |   49 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 46 deletions(-)
 create mode 100644 arch/arm/kernel/fiqasm.S

diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h
index 2242ce2..45bb3ee 100644
--- a/arch/arm/include/asm/fiq.h
+++ b/arch/arm/include/asm/fiq.h
@@ -29,9 +29,21 @@ struct fiq_handler {
 extern int claim_fiq(struct fiq_handler *f);
 extern void release_fiq(struct fiq_handler *f);
 extern void set_fiq_handler(void *start, unsigned int length);
-extern void set_fiq_regs(struct pt_regs *regs);
-extern void get_fiq_regs(struct pt_regs *regs);
 extern void enable_fiq(int fiq);
 extern void disable_fiq(int fiq);
 
+/* helpers defined in fiqasm.S: */
+extern void __set_fiq_regs(unsigned long const *regs);
+extern void __get_fiq_regs(unsigned long *regs);
+
+static inline void set_fiq_regs(struct pt_regs const *regs)
+{
+	__set_fiq_regs(&regs->ARM_r8);
+}
+
+static inline void get_fiq_regs(struct pt_regs *regs)
+{
+	__get_fiq_regs(&regs->ARM_r8);
+}
+
 #endif
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 74554f1..689f4d9 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -24,7 +24,7 @@ obj-$(CONFIG_OC_ETM)		+= etm.o
 
 obj-$(CONFIG_ISA_DMA_API)	+= dma.o
 obj-$(CONFIG_ARCH_ACORN)	+= ecard.o 
-obj-$(CONFIG_FIQ)		+= fiq.o
+obj-$(CONFIG_FIQ)		+= fiq.o fiqasm.o
 obj-$(CONFIG_MODULES)		+= armksyms.o module.o
 obj-$(CONFIG_ARTHUR)		+= arthur.o
 obj-$(CONFIG_ISA_DMA)		+= dma-isa.o
diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
index e72dc34..4c164ec 100644
--- a/arch/arm/kernel/fiq.c
+++ b/arch/arm/kernel/fiq.c
@@ -89,47 +89,6 @@ void set_fiq_handler(void *start, unsigned int length)
 		flush_icache_range(0x1c, 0x1c + length);
 }
 
-/*
- * Taking an interrupt in FIQ mode is death, so both these functions
- * disable irqs for the duration.  Note - these functions are almost
- * entirely coded in assembly.
- */
-void __naked set_fiq_regs(struct pt_regs *regs)
-{
-	register unsigned long tmp;
-	asm volatile (
-	"mov	ip, sp\n\
-	stmfd	sp!, {fp, ip, lr, pc}\n\
-	sub	fp, ip, #4\n\
-	mrs	%0, cpsr\n\
-	msr	cpsr_c, %2	@ select FIQ mode\n\
-	mov	r0, r0\n\
-	ldmia	%1, {r8 - r14}\n\
-	msr	cpsr_c, %0	@ return to SVC mode\n\
-	mov	r0, r0\n\
-	ldmfd	sp, {fp, sp, pc}"
-	: "=&r" (tmp)
-	: "r" (&regs->ARM_r8), "I" (PSR_I_BIT | PSR_F_BIT | FIQ_MODE));
-}
-
-void __naked get_fiq_regs(struct pt_regs *regs)
-{
-	register unsigned long tmp;
-	asm volatile (
-	"mov	ip, sp\n\
-	stmfd	sp!, {fp, ip, lr, pc}\n\
-	sub	fp, ip, #4\n\
-	mrs	%0, cpsr\n\
-	msr	cpsr_c, %2	@ select FIQ mode\n\
-	mov	r0, r0\n\
-	stmia	%1, {r8 - r14}\n\
-	msr	cpsr_c, %0	@ return to SVC mode\n\
-	mov	r0, r0\n\
-	ldmfd	sp, {fp, sp, pc}"
-	: "=&r" (tmp)
-	: "r" (&regs->ARM_r8), "I" (PSR_I_BIT | PSR_F_BIT | FIQ_MODE));
-}
-
 int claim_fiq(struct fiq_handler *f)
 {
 	int ret = 0;
@@ -174,8 +133,8 @@ void disable_fiq(int fiq)
 }
 
 EXPORT_SYMBOL(set_fiq_handler);
-EXPORT_SYMBOL(set_fiq_regs);
-EXPORT_SYMBOL(get_fiq_regs);
+EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
+EXPORT_SYMBOL(__get_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(claim_fiq);
 EXPORT_SYMBOL(release_fiq);
 EXPORT_SYMBOL(enable_fiq);
diff --git a/arch/arm/kernel/fiqasm.S b/arch/arm/kernel/fiqasm.S
new file mode 100644
index 0000000..0504bb8
--- /dev/null
+++ b/arch/arm/kernel/fiqasm.S
@@ -0,0 +1,49 @@
+/*
+ *  linux/arch/arm/kernel/fiqasm.S
+ *
+ *  Derived from code originally in linux/arch/arm/kernel/fiq.c:
+ *
+ *  Copyright (C) 1998 Russell King
+ *  Copyright (C) 1998, 1999 Phil Blundell
+ *  Copyright (C) 2011, Linaro Limited
+ *
+ *  FIQ support written by Philip Blundell <philb@gnu.org>, 1998.
+ *
+ *  FIQ support re-written by Russell King to be more generic
+ *
+ *  v7/Thumb-2 compatibility modifications by Linaro Limited, 2011.
+ */
+	
+#include <arm/assembler.h>
+
+/*
+ * Taking an interrupt in FIQ mode is death, so both these functions
+ * disable irqs for the duration.  Note - these functions are almost
+ * entirely coded in assembly.
+ */
+
+ENTRY(__set_fiq_regs)
+	mov	r2, #PSR_I_BIT | PSR_F_BIT | FIQ_MODE
+	mrs	r1, cpsr
+	msr	cpsr_c, r2	@ select FIQ mode
+	mov	r0, r0		@ avoid hazard prior to ARMv4
+	ldmia	r0!, {r8 - r12}
+	ldr	sp, [r0], #4
+	ldr	lr, [r0]
+	msr	cpsr_c, r1	@ return to SVC mode
+	mov	r0, r0		@ avoid hazard prior to ARMv4
+	mov	pc, lr
+ENDPROC(__set_fiq_regs)
+
+ENTRY(__get_fiq_regs)
+	mov	r2, #PSR_I_BIT | PSR_F_BIT | FIQ_MODE
+	mrs	r1, cpsr
+	msr	cpsr_c, r2	@ select FIQ mode
+	mov	r0, r0		@ avoid hazard prior to ARMv4
+	stmia	r0!, {r8 - r12}
+	str	sp, [r0], #4
+	str	lr, [r0]
+	msr	cpsr_c, r1	@ return to SVC mode
+	mov	r0, r0		@ avoid hazard prior to ARMv4
+	mov	pc, lr
+ENDPROC(__get_fiq_regs)
-- 
1.7.1

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Dave Martin]

On Wed, Apr 6, 2011 at 11:29 AM, Dave Martin <dave.martin@linaro.org> wrote:
> ?* To remove the risk of inconvenient register allocation decisions
> ? by the compiler, these functions are separated out as pure
> ? assembler.
>
> ?* The apcs frame manipulation code is not applicable for Thumb-2
> ? (and also not easily compatible). ?Since it's not essential to
> ? have a full frame on these leaf assembler functions, the frame
> ? manipulation is removed, in the interests of simplicity.
>
> ?* Split up ldm/stm instructions to be compatible with Thumb-2,
> ? as well as avoiding instruction forms deprecated on >= ARMv7.
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
>
> ?arch/arm/include/asm/fiq.h | ? 16 ++++++++++++-
> ?arch/arm/kernel/Makefile ? | ? ?2 +-
> ?arch/arm/kernel/fiq.c ? ? ?| ? 45 +--------------------------------------
> ?arch/arm/kernel/fiqasm.S ? | ? 49 ++++++++++++++++++++++++++++++++++++++++++++
> ?4 files changed, 66 insertions(+), 46 deletions(-)
> ?create mode 100644 arch/arm/kernel/fiqasm.S
>
> diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h
> index 2242ce2..45bb3ee 100644
> --- a/arch/arm/include/asm/fiq.h
> +++ b/arch/arm/include/asm/fiq.h
> @@ -29,9 +29,21 @@ struct fiq_handler {
> ?extern int claim_fiq(struct fiq_handler *f);
> ?extern void release_fiq(struct fiq_handler *f);
> ?extern void set_fiq_handler(void *start, unsigned int length);
> -extern void set_fiq_regs(struct pt_regs *regs);
> -extern void get_fiq_regs(struct pt_regs *regs);
> ?extern void enable_fiq(int fiq);
> ?extern void disable_fiq(int fiq);
>
> +/* helpers defined in fiqasm.S: */
> +extern void __set_fiq_regs(unsigned long const *regs);
> +extern void __get_fiq_regs(unsigned long *regs);
> +
> +static inline void set_fiq_regs(struct pt_regs const *regs)
> +{
> + ? ? ? __set_fiq_regs(&regs->ARM_r8);
> +}
> +
> +static inline void get_fiq_regs(struct pt_regs *regs)
> +{
> + ? ? ? __get_fiq_regs(&regs->ARM_r8);
> +}
> +
> ?#endif
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 74554f1..689f4d9 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -24,7 +24,7 @@ obj-$(CONFIG_OC_ETM) ? ? ? ? ?+= etm.o
>
> ?obj-$(CONFIG_ISA_DMA_API) ? ? ?+= dma.o
> ?obj-$(CONFIG_ARCH_ACORN) ? ? ? += ecard.o
> -obj-$(CONFIG_FIQ) ? ? ? ? ? ? ?+= fiq.o
> +obj-$(CONFIG_FIQ) ? ? ? ? ? ? ?+= fiq.o fiqasm.o
> ?obj-$(CONFIG_MODULES) ? ? ? ? ?+= armksyms.o module.o
> ?obj-$(CONFIG_ARTHUR) ? ? ? ? ? += arthur.o
> ?obj-$(CONFIG_ISA_DMA) ? ? ? ? ?+= dma-isa.o
> diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
> index e72dc34..4c164ec 100644
> --- a/arch/arm/kernel/fiq.c
> +++ b/arch/arm/kernel/fiq.c
> @@ -89,47 +89,6 @@ void set_fiq_handler(void *start, unsigned int length)
> ? ? ? ? ? ? ? ?flush_icache_range(0x1c, 0x1c + length);
> ?}
>
> -/*
> - * Taking an interrupt in FIQ mode is death, so both these functions
> - * disable irqs for the duration. ?Note - these functions are almost
> - * entirely coded in assembly.
> - */
> -void __naked set_fiq_regs(struct pt_regs *regs)
> -{
> - ? ? ? register unsigned long tmp;
> - ? ? ? asm volatile (
> - ? ? ? "mov ? ?ip, sp\n\
> - ? ? ? stmfd ? sp!, {fp, ip, lr, pc}\n\
> - ? ? ? sub ? ? fp, ip, #4\n\
> - ? ? ? mrs ? ? %0, cpsr\n\
> - ? ? ? msr ? ? cpsr_c, %2 ? ? ?@ select FIQ mode\n\
> - ? ? ? mov ? ? r0, r0\n\
> - ? ? ? ldmia ? %1, {r8 - r14}\n\
> - ? ? ? msr ? ? cpsr_c, %0 ? ? ?@ return to SVC mode\n\
> - ? ? ? mov ? ? r0, r0\n\
> - ? ? ? ldmfd ? sp, {fp, sp, pc}"
> - ? ? ? : "=&r" (tmp)
> - ? ? ? : "r" (&regs->ARM_r8), "I" (PSR_I_BIT | PSR_F_BIT | FIQ_MODE));
> -}
> -
> -void __naked get_fiq_regs(struct pt_regs *regs)
> -{
> - ? ? ? register unsigned long tmp;
> - ? ? ? asm volatile (
> - ? ? ? "mov ? ?ip, sp\n\
> - ? ? ? stmfd ? sp!, {fp, ip, lr, pc}\n\
> - ? ? ? sub ? ? fp, ip, #4\n\
> - ? ? ? mrs ? ? %0, cpsr\n\
> - ? ? ? msr ? ? cpsr_c, %2 ? ? ?@ select FIQ mode\n\
> - ? ? ? mov ? ? r0, r0\n\
> - ? ? ? stmia ? %1, {r8 - r14}\n\
> - ? ? ? msr ? ? cpsr_c, %0 ? ? ?@ return to SVC mode\n\
> - ? ? ? mov ? ? r0, r0\n\
> - ? ? ? ldmfd ? sp, {fp, sp, pc}"
> - ? ? ? : "=&r" (tmp)
> - ? ? ? : "r" (&regs->ARM_r8), "I" (PSR_I_BIT | PSR_F_BIT | FIQ_MODE));
> -}
> -
> ?int claim_fiq(struct fiq_handler *f)
> ?{
> ? ? ? ?int ret = 0;
> @@ -174,8 +133,8 @@ void disable_fiq(int fiq)
> ?}
>
> ?EXPORT_SYMBOL(set_fiq_handler);
> -EXPORT_SYMBOL(set_fiq_regs);
> -EXPORT_SYMBOL(get_fiq_regs);
> +EXPORT_SYMBOL(__set_fiq_regs); /* defined in fiqasm.S */
> +EXPORT_SYMBOL(__get_fiq_regs); /* defined in fiqasm.S */
> ?EXPORT_SYMBOL(claim_fiq);
> ?EXPORT_SYMBOL(release_fiq);
> ?EXPORT_SYMBOL(enable_fiq);
> diff --git a/arch/arm/kernel/fiqasm.S b/arch/arm/kernel/fiqasm.S
> new file mode 100644
> index 0000000..0504bb8
> --- /dev/null
> +++ b/arch/arm/kernel/fiqasm.S
> @@ -0,0 +1,49 @@
> +/*
> + * ?linux/arch/arm/kernel/fiqasm.S
> + *
> + * ?Derived from code originally in linux/arch/arm/kernel/fiq.c:
> + *
> + * ?Copyright (C) 1998 Russell King
> + * ?Copyright (C) 1998, 1999 Phil Blundell
> + * ?Copyright (C) 2011, Linaro Limited
> + *
> + * ?FIQ support written by Philip Blundell <philb@gnu.org>, 1998.
> + *
> + * ?FIQ support re-written by Russell King to be more generic
> + *
> + * ?v7/Thumb-2 compatibility modifications by Linaro Limited, 2011.
> + */
> +
> +#include <arm/assembler.h>
> +
> +/*
> + * Taking an interrupt in FIQ mode is death, so both these functions
> + * disable irqs for the duration. ?Note - these functions are almost
> + * entirely coded in assembly.
> + */
> +
> +ENTRY(__set_fiq_regs)
> + ? ? ? mov ? ? r2, #PSR_I_BIT | PSR_F_BIT | FIQ_MODE
> + ? ? ? mrs ? ? r1, cpsr
> + ? ? ? msr ? ? cpsr_c, r2 ? ? ?@ select FIQ mode
> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
> + ? ? ? ldmia ? r0!, {r8 - r12}
> + ? ? ? ldr ? ? sp, [r0], #4
> + ? ? ? ldr ? ? lr, [r0]
> + ? ? ? msr ? ? cpsr_c, r1 ? ? ?@ return to SVC mode
> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
> + ? ? ? mov ? ? pc, lr
> +ENDPROC(__set_fiq_regs)
> +
> +ENTRY(__get_fiq_regs)
> + ? ? ? mov ? ? r2, #PSR_I_BIT | PSR_F_BIT | FIQ_MODE
> + ? ? ? mrs ? ? r1, cpsr
> + ? ? ? msr ? ? cpsr_c, r2 ? ? ?@ select FIQ mode
> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
> + ? ? ? stmia ? r0!, {r8 - r12}
> + ? ? ? str ? ? sp, [r0], #4
> + ? ? ? str ? ? lr, [r0]
> + ? ? ? msr ? ? cpsr_c, r1 ? ? ?@ return to SVC mode
> + ? ? ? mov ? ? r0, r0 ? ? ? ? ?@ avoid hazard prior to ARMv4
> + ? ? ? mov ? ? pc, lr
> +ENDPROC(__get_fiq_regs)
> --
> 1.7.1
>
>

Note -- this patch contains some minor errors, but I'm more interested
in feedback on the general idea for now.

Cheers
---Dave

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Nicolas Pitre]

On Wed, 6 Apr 2011, Dave Martin wrote:

>  * To remove the risk of inconvenient register allocation decisions
>    by the compiler, these functions are separated out as pure
>    assembler.
> 
>  * The apcs frame manipulation code is not applicable for Thumb-2
>    (and also not easily compatible).  Since it's not essential to
>    have a full frame on these leaf assembler functions, the frame
>    manipulation is removed, in the interests of simplicity.
> 
>  * Split up ldm/stm instructions to be compatible with Thumb-2,
>    as well as avoiding instruction forms deprecated on >= ARMv7.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>

I like.  Minor nit:

> +/*
> + * Taking an interrupt in FIQ mode is death, so both these functions
> + * disable irqs for the duration.  Note - these functions are almost
> + * entirely coded in assembly.
> + */

I think it is obvious the last sentence may go, especially since 
"almost" is not exactly true anymore.

Other than that:

Reviewed-by: Nicolas Pitre <nicolas.pitre@linaro.org>


Nicolas

[RFC PATCH] ARM: fiq: Refactor {get,set}_fiq_regs() for Thumb-2

[Dave Martin]

On Wed, Apr 6, 2011 at 6:37 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Wed, 6 Apr 2011, Dave Martin wrote:
>
>> ?* To remove the risk of inconvenient register allocation decisions
>> ? ?by the compiler, these functions are separated out as pure
>> ? ?assembler.
>>
>> ?* The apcs frame manipulation code is not applicable for Thumb-2
>> ? ?(and also not easily compatible). ?Since it's not essential to
>> ? ?have a full frame on these leaf assembler functions, the frame
>> ? ?manipulation is removed, in the interests of simplicity.
>>
>> ?* Split up ldm/stm instructions to be compatible with Thumb-2,
>> ? ?as well as avoiding instruction forms deprecated on >= ARMv7.
>>
>> Signed-off-by: Dave Martin <dave.martin@linaro.org>
>
> I like. ?Minor nit:
>
>> +/*
>> + * Taking an interrupt in FIQ mode is death, so both these functions
>> + * disable irqs for the duration. ?Note - these functions are almost
>> + * entirely coded in assembly.
>> + */
>
> I think it is obvious the last sentence may go, especially since
> "almost" is not exactly true anymore.

Actually, my local fixes tidied that away too... I'll repost, since
there are other errors too...

>
> Other than that:
>
> Reviewed-by: Nicolas Pitre <nicolas.pitre@linaro.org>

Thanks

---Dave