diff mbox

[soc] ARM: use ARM_SINGLE_ARMV7M for ARMv7-M platforms

Message ID CALszF6D-3pXHtyVMwhO0JkT-dCSSB98UNDHNN7-LD04Wdgd2cA@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Coquelin May 22, 2015, 3:29 p.m. UTC 
2015-05-22 16:50 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> [one small request as I have four armv7-m folks on Cc already:
>  could one of you try to fix the warning that I get with every
>  single build: "/git/arm-soc/arch/arm/kernel/head-nommu.S: Assembler
> messages: /git/arm-soc/arch/arm/kernel/head-nommu.S:167: Warning:
> Use of r13 as a source register is deprecated when r15 is the
> destination register."]

Moving r13 to r12 and returning r12 seems to do the job (see below).
But I don't know if there is a more elegant way, and if it is also
valid for other architectures than armv7-m.
I can propose a patch if someone can confirm it is valid.

Regards,
Maxime

-------------------------------------------------------------------------------------------------------------

Comments

Stefan Agner May 22, 2015, 3:36 p.m. UTC | #1 
On 2015-05-22 17:29, Maxime Coquelin wrote:
> 2015-05-22 16:50 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> [one small request as I have four armv7-m folks on Cc already:
>>  could one of you try to fix the warning that I get with every
>>  single build: "/git/arm-soc/arch/arm/kernel/head-nommu.S: Assembler
>> messages: /git/arm-soc/arch/arm/kernel/head-nommu.S:167: Warning:
>> Use of r13 as a source register is deprecated when r15 is the
>> destination register."]
> 
> Moving r13 to r12 and returning r12 seems to do the job (see below).
> But I don't know if there is a more elegant way, and if it is also
> valid for other architectures than armv7-m.
> I can propose a patch if someone can confirm it is valid.

> 
> -------------------------------------------------------------------------------------------------------------
> 
> diff --git a/arch/arm/kernel/head-nommu.S b/arch/arm/kernel/head-nommu.S
> index aebfbf7..e84bdad 100644
> --- a/arch/arm/kernel/head-nommu.S
> +++ b/arch/arm/kernel/head-nommu.S
> @@ -164,7 +164,8 @@ __after_proc_init:
>  #endif
>         mcr     p15, 0, r0, c1, c0, 0           @ write control reg
>  #endif /* CONFIG_CPU_CP15 */
> -       ret     r13
> +       mov     r12, r13
> +       ret     r12
>  ENDPROC(__after_proc_init)
>         .ltorg

That is actually a patch I have here too, altough I used r11 back
then... :-)

However, I don't think this is a nice solution. We should avoid using
r13 for that address in the first place, e.g. using a different register
to get the value when calling __mmap_switched. However, for that one
need to know what registers are guaranteed to be not used within
PROCINFO_INITFUNC...

--
Stefan
Daniel Thompson May 22, 2015, 3:56 p.m. UTC | #2 
On 22/05/15 16:29, Maxime Coquelin wrote:
> 2015-05-22 16:50 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> [one small request as I have four armv7-m folks on Cc already:
>>   could one of you try to fix the warning that I get with every
>>   single build: "/git/arm-soc/arch/arm/kernel/head-nommu.S: Assembler
>> messages: /git/arm-soc/arch/arm/kernel/head-nommu.S:167: Warning:
>> Use of r13 as a source register is deprecated when r15 is the
>> destination register."]
>
> Moving r13 to r12 and returning r12 seems to do the job (see below).
> But I don't know if there is a more elegant way, and if it is also
> valid for other architectures than armv7-m.

Why not just s/r13/r11/?

(works for me but I'm only working on single core system)
Stefan Agner May 22, 2015, 4:28 p.m. UTC | #3 
On 2015-05-22 17:56, Daniel Thompson wrote:
> On 22/05/15 16:29, Maxime Coquelin wrote:
>> 2015-05-22 16:50 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>> [one small request as I have four armv7-m folks on Cc already:
>>>   could one of you try to fix the warning that I get with every
>>>   single build: "/git/arm-soc/arch/arm/kernel/head-nommu.S: Assembler
>>> messages: /git/arm-soc/arch/arm/kernel/head-nommu.S:167: Warning:
>>> Use of r13 as a source register is deprecated when r15 is the
>>> destination register."]
>>
>> Moving r13 to r12 and returning r12 seems to do the job (see below).
>> But I don't know if there is a more elegant way, and if it is also
>> valid for other architectures than armv7-m.
> 
> Why not just s/r13/r11/?
> 
> (works for me but I'm only working on single core system)

For ARMv7-M this works, since r11 is not used in the processors
PROCINFO_INITFUNC function (__cpu_flush in struct proc_info_list, which
is __v7m_setup in proc-v7m.S).

However, afaik, head-nommu.S can be used by different processors too,
hence that register needs to be free to use for all possible __cpu_flush
implementations.

That said, proc-v7.S stores r11 on the stack, so it really seems that
r11 is ok to use?

--
Stefan
Russell King - ARM Linux May 22, 2015, 5:56 p.m. UTC | #4 
On Fri, May 22, 2015 at 05:29:20PM +0200, Maxime Coquelin wrote:
> 2015-05-22 16:50 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> > [one small request as I have four armv7-m folks on Cc already:
> >  could one of you try to fix the warning that I get with every
> >  single build: "/git/arm-soc/arch/arm/kernel/head-nommu.S: Assembler
> > messages: /git/arm-soc/arch/arm/kernel/head-nommu.S:167: Warning:
> > Use of r13 as a source register is deprecated when r15 is the
> > destination register."]
> 
> Moving r13 to r12 and returning r12 seems to do the job (see below).
> But I don't know if there is a more elegant way, and if it is also
> valid for other architectures than armv7-m.
> I can propose a patch if someone can confirm it is valid.

Please follow the ARM code example, rather than inventing alternative
solutions to the same problem that was solved there.  We use r3 for
this there:

        mov     r3, r13
        ret     r3

Consistency _is_ important.

Thanks.
Russell King - ARM Linux May 22, 2015, 6:06 p.m. UTC | #5 
On Fri, May 22, 2015 at 06:28:16PM +0200, Stefan Agner wrote:
> On 2015-05-22 17:56, Daniel Thompson wrote:
> > On 22/05/15 16:29, Maxime Coquelin wrote:
> >> 2015-05-22 16:50 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> >>> [one small request as I have four armv7-m folks on Cc already:
> >>>   could one of you try to fix the warning that I get with every
> >>>   single build: "/git/arm-soc/arch/arm/kernel/head-nommu.S: Assembler
> >>> messages: /git/arm-soc/arch/arm/kernel/head-nommu.S:167: Warning:
> >>> Use of r13 as a source register is deprecated when r15 is the
> >>> destination register."]
> >>
> >> Moving r13 to r12 and returning r12 seems to do the job (see below).
> >> But I don't know if there is a more elegant way, and if it is also
> >> valid for other architectures than armv7-m.
> > 
> > Why not just s/r13/r11/?
> > 
> > (works for me but I'm only working on single core system)
> 
> For ARMv7-M this works, since r11 is not used in the processors
> PROCINFO_INITFUNC function (__cpu_flush in struct proc_info_list, which
> is __v7m_setup in proc-v7m.S).
> 
> However, afaik, head-nommu.S can be used by different processors too,
> hence that register needs to be free to use for all possible __cpu_flush
> implementations.
> 
> That said, proc-v7.S stores r11 on the stack, so it really seems that
> r11 is ok to use?

Please use r3 (as I just said).  We don't need random deviations between
MMU and noMMU stuff - that just makes maintanence of other code more
difficult.

You can also avoid the issues of having it passed through the processor
specific init function (which isn't guaranteed to preserve r13) by
doing this:

-	ldr	r13, =__mmap_switched		@ address to jump to after
-						@ initialising sctlr
	badr	lr, 1f				@ return (PIC) address
	ldr	r12, [r10, #PROCINFO_INITFUNC]
	add	r12, r12, r10
	ret	r12
- 1:	b	__after_proc_init
+1:	ldr	r13, =__mmap_switched		@ address to jump to after
+						@ initialising sctlr
	b	__after_proc_init

However, because you have no MMU to turn on, and no address switch,
you actually don't need any of this.  __after_proc_init can become
a "function" which returns via the link register.

You can then do:

1:	bl	__after_proc_init
	b	__mmap_switched

You'll need to fix secondary_startup in there as well:

-	adr	r4, __secondary_data
-	ldmia	r4, {r7, r12}
	ldr	r7, __secondary_data

#ifdef CONFIG_ARM_MPU
	/* Use MPU region info supplied by __cpu_up */
	ldr	r6, [r7]			@ get secondary_data.mpu_szr
	bl	__setup_mpu			@ Initialize the MPU
#endif

-	badr	lr, __after_proc_init		@ return address
-	mov	r13, r12			@ __secondary_switched address
+	badr	lr, 1f				@ return (PIC) address
	ldr	r12, [r10, #PROCINFO_INITFUNC]
	add	r12, r12, r10
	ret	r12
-ENDPROC(secondary_startup)
-
-ENTRY(__secondary_switched)
+1:	bl	__after_proc_init
	ldr	sp, [r7, #12]			@ set up the stack pointer
	mov	fp, #0
	b	secondary_start_kernel
-ENDPROC(__secondary_switched)
+ENDPROC(secondary_startup)
Stefan Agner May 22, 2015, 7:34 p.m. UTC | #6 
On 2015-05-22 20:06, Russell King - ARM Linux wrote:
> On Fri, May 22, 2015 at 06:28:16PM +0200, Stefan Agner wrote:
>> On 2015-05-22 17:56, Daniel Thompson wrote:
>> > On 22/05/15 16:29, Maxime Coquelin wrote:
>> >> 2015-05-22 16:50 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> >>> [one small request as I have four armv7-m folks on Cc already:
>> >>>   could one of you try to fix the warning that I get with every
>> >>>   single build: "/git/arm-soc/arch/arm/kernel/head-nommu.S: Assembler
>> >>> messages: /git/arm-soc/arch/arm/kernel/head-nommu.S:167: Warning:
>> >>> Use of r13 as a source register is deprecated when r15 is the
>> >>> destination register."]
>> >>
>> >> Moving r13 to r12 and returning r12 seems to do the job (see below).
>> >> But I don't know if there is a more elegant way, and if it is also
>> >> valid for other architectures than armv7-m.
>> >
>> > Why not just s/r13/r11/?
>> >
>> > (works for me but I'm only working on single core system)
>>
>> For ARMv7-M this works, since r11 is not used in the processors
>> PROCINFO_INITFUNC function (__cpu_flush in struct proc_info_list, which
>> is __v7m_setup in proc-v7m.S).
>>
>> However, afaik, head-nommu.S can be used by different processors too,
>> hence that register needs to be free to use for all possible __cpu_flush
>> implementations.
>>
>> That said, proc-v7.S stores r11 on the stack, so it really seems that
>> r11 is ok to use?
> 
> Please use r3 (as I just said).  We don't need random deviations between
> MMU and noMMU stuff - that just makes maintanence of other code more
> difficult.
> 
> You can also avoid the issues of having it passed through the processor
> specific init function (which isn't guaranteed to preserve r13) by
> doing this:
> 
> -	ldr	r13, =__mmap_switched		@ address to jump to after
> -						@ initialising sctlr
> 	badr	lr, 1f				@ return (PIC) address
> 	ldr	r12, [r10, #PROCINFO_INITFUNC]
> 	add	r12, r12, r10
> 	ret	r12
> - 1:	b	__after_proc_init
> +1:	ldr	r13, =__mmap_switched		@ address to jump to after
> +						@ initialising sctlr
> 	b	__after_proc_init

Hm, this is looks sensible, could also be used for head.S I guess...
secondary_startup would need a similar approach then.

> 
> However, because you have no MMU to turn on, and no address switch,
> you actually don't need any of this.  __after_proc_init can become
> a "function" which returns via the link register.
> 
> You can then do:
> 
> 1:	bl	__after_proc_init
> 	b	__mmap_switched
> 
> You'll need to fix secondary_startup in there as well:
> 
> -	adr	r4, __secondary_data
> -	ldmia	r4, {r7, r12}
> 	ldr	r7, __secondary_data
> 
> #ifdef CONFIG_ARM_MPU
> 	/* Use MPU region info supplied by __cpu_up */
> 	ldr	r6, [r7]			@ get secondary_data.mpu_szr
> 	bl	__setup_mpu			@ Initialize the MPU
> #endif
> 
> -	badr	lr, __after_proc_init		@ return address
> -	mov	r13, r12			@ __secondary_switched address
> +	badr	lr, 1f				@ return (PIC) address
> 	ldr	r12, [r10, #PROCINFO_INITFUNC]
> 	add	r12, r12, r10
> 	ret	r12
> -ENDPROC(secondary_startup)
> -
> -ENTRY(__secondary_switched)
> +1:	bl	__after_proc_init
> 	ldr	sp, [r7, #12]			@ set up the stack pointer
> 	mov	fp, #0
> 	b	secondary_start_kernel
> -ENDPROC(__secondary_switched)
> +ENDPROC(secondary_startup)

Sounds like a much simpler approach. Will test that and send out a patch
in case it works here.

--
Stefan
diff mbox

Patch

diff --git a/arch/arm/kernel/head-nommu.S b/arch/arm/kernel/head-nommu.S
index aebfbf7..e84bdad 100644
--- a/arch/arm/kernel/head-nommu.S
+++ b/arch/arm/kernel/head-nommu.S
@@ -164,7 +164,8 @@  __after_proc_init:
 #endif
        mcr     p15, 0, r0, c1, c0, 0           @ write control reg
 #endif /* CONFIG_CPU_CP15 */
-       ret     r13
+       mov     r12, r13
+       ret     r12
 ENDPROC(__after_proc_init)
        .ltorg