[U-Boot,RFC] powerpc/lib: unsafe register handling in wait_ticks

Hi,

If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks() function
calls the function specified by the WATCHDOG_RESET macro.
The wait_ticks function depends on the registers r0, r6 and r7 being
preserved however that is not guaranteed if the reset function is a
C function.

The following patch changes to using r14+r15 instead of r6+r7 and
saves all necessary registers on the stack.

As I'm quite fresh to PowerPC assembly language I would appreciate
any feedback on the implementation.

On a side note, one could wonder why this function is not written in
C language to start with.

Best regards,
Mats Kärrman

> 
> Hi,

Hi Mats

I would appreciate if you CC me directly on stuff I have been involved in.
I don't read every mail on u-boot list(to many of them). It was just
plain luck I saw this one. 

> 
> If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks() 
function
> calls the function specified by the WATCHDOG_RESET macro.
> The wait_ticks function depends on the registers r0, r6 and r7 being
> preserved however that is not guaranteed if the reset function is a
> C function.
> 
> The following patch changes to using r14+r15 instead of r6+r7 and
> saves all necessary registers on the stack.

This adds some extra churn to the code that my patch didn't do.
On the other hand your patch makes the function look more
like how gcc would have done so I am fine with that.
However, I am not sure r14 is a good fit, I cannot remember if there is 
some
special purpose for r14. Hopefully somebody on the list knows. 

> 
> As I'm quite fresh to PowerPC assembly language I would appreciate
> any feedback on the implementation.
> 
> On a side note, one could wonder why this function is not written in
> C language to start with.

History I guess, once upon a time this function didn't need(or could not 
use)
the stack. Now it would be quite possible to change it into C.

> 
> Best regards,
> Mats Kärrman
> 
> --- a/arch/powerpc/lib/ticks.S
> +++ b/arch/powerpc/lib/ticks.S
> @@ -50,19 +50,24 @@ wait_ticks:
>     stwu   r1, -16(r1)
>     mflr   r0      /* save link register */
>     stw   r0, 20(r1)   /* Use r0 or GDB will be unhappy */
> -   mr   r7, r3      /* save tick count */
> +   stw   r14, 12(r1)   /* save used registers */
> +   stw   r15, 8(r1)
> +   mr   r14, r3      /* save tick count */
>     bl   get_ticks   /* Get start time */
> 
>     /* Calculate end time */
> -   addc    r7, r4, r7   /* Compute end time lower */
> -   addze   r6, r3      /*     and end time upper */
> +   addc    r14, r4, r14   /* Compute end time lower */
> +   addze   r15, r3      /*     and end time upper */
> 
>     WATCHDOG_RESET      /* Trigger watchdog, if needed */
>  1:   bl   get_ticks   /* Get current time */
> -   subfc   r4, r4, r7   /* Subtract current time from end time */
> -   subfe.   r3, r3, r6
> +   subfc   r4, r4, r14   /* Subtract current time from end time */
> +   subfe.   r3, r3, r15
>     bge   1b      /* Loop until time expired */
> 
> -   mtlr   r0      /* restore link register */
> +   lwz     r15, 8(r1)   /* restore saved registers */
> +   lwz     r14, 12(r1)
> +   lwz     r0, 20(r1)
>     addi   r1,r1,16
> +   mtlr   r0
>     blr
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Dear Joakim Tjernlund,

In message <OF52C94A3D.C3BD2E6F-ONC1257AFD.005BAFE0-C1257AFD.006736C5@transmode.se> you wrote:
> 
> This adds some extra churn to the code that my patch didn't do.
> On the other hand your patch makes the function look more
> like how gcc would have done so I am fine with that.
> However, I am not sure r14 is a good fit, I cannot remember if there is
> some
> special purpose for r14. Hopefully somebody on the list knows.

This is documented in the README:

| For PowerPC, the following registers have specific use:
| 	R1:	stack pointer
| 	R2:	reserved for system use
| 	R3-R4:	parameter passing and return values
| 	R5-R10: parameter passing
| 	R13:	small data area pointer
| 	R30:	GOT pointer
| 	R31:	frame pointer
| 
| 	(U-Boot also uses R12 as internal GOT pointer. r12
| 	is a volatile register so r12 needs to be reset when
| 	going back and forth between asm and C)
| 
|     ==> U-Boot will use R2 to hold a pointer to the global data
| 

Best regards,

Wolfgang Denk

Wolfgang Denk <wd@denx.de> wrote on 2013/01/24 20:27:26:
> 
> Dear Joakim Tjernlund,
> 
> In message <OF52C94A3D.C3BD2E6F-ONC1257AFD.005BAFE0-C1257AFD.
> 006736C5@transmode.se> you wrote:
> > 
> > This adds some extra churn to the code that my patch didn't do.
> > On the other hand your patch makes the function look more
> > like how gcc would have done so I am fine with that.
> > However, I am not sure r14 is a good fit, I cannot remember if there 
is
> > some
> > special purpose for r14. Hopefully somebody on the list knows.
> 
> This is documented in the README:
> 
> | For PowerPC, the following registers have specific use:
> |    R1:   stack pointer
> |    R2:   reserved for system use
> |    R3-R4:   parameter passing and return values
> |    R5-R10: parameter passing
> |    R13:   small data area pointer
> |    R30:   GOT pointer
> |    R31:   frame pointer
> | 
> |    (U-Boot also uses R12 as internal GOT pointer. r12
> |    is a volatile register so r12 needs to be reset when
> |    going back and forth between asm and C)
> | 
> |     ==> U-Boot will use R2 to hold a pointer to the global data
> | 

Right, then I think the patch is good:

Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se>