Message ID | ED3E0BCACD909541BA94A34C4A164D4C427B5E6A@post.tritech.se |
---|---|
State | Changes Requested |
Delegated to: | Tom Rini |
Headers | show |
On 01/27/2013 06:03 PM, Mats Kärrman wrote: > 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, e.g. if the reset function is a > C function this will probably overwrite r0 and cause an endless loop. > > The following patch changes to using r14+r15 instead of r6+r7 (to resemble > what would have been generated by a C compiler) and saves all necessary > registers on the stack. > > The patch has been tested on a custom MPC5125 based machine using the 512x > powerpc architecture. > > Signed-off-by: Mats Karrman <mats.karrman@tritech.se> > Cc: Wolfgang Denk <wd@denx.de> > Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se> Thanks for this fix. I just reproduced this problem on another MPC5200 based board (a4m2k) while implementing watchdog support. And this patch fixes the problem (hangup in wait_ticks). So: Tested-by: Stefan Roese <sr@denx.de> Thanks, Stefan
> > On 01/27/2013 06:03 PM, Mats Kärrman wrote: > > 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, e.g. if the reset function is a > > C function this will probably overwrite r0 and cause an endless loop. > > > > The following patch changes to using r14+r15 instead of r6+r7 (to resemble > > what would have been generated by a C compiler) and saves all necessary > > registers on the stack. > > > > The patch has been tested on a custom MPC5125 based machine using the 512x > > powerpc architecture. > > > > Signed-off-by: Mats Karrman <mats.karrman@tritech.se> > > Cc: Wolfgang Denk <wd@denx.de> > > Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se> > > Thanks for this fix. I just reproduced this problem on another MPC5200 > based board (a4m2k) while implementing watchdog support. And this patch > fixes the problem (hangup in wait_ticks). > > So: > > Tested-by: Stefan Roese <sr@denx.de> If someone wants to play with a C only impl. I THINK it this is a good starting point: #define WATCHDOG_RESET /*static inline */unsigned long long get_ticks(void) { union xx { unsigned long long r3_r4; struct yx { unsigned long r3; unsigned long r4; } xy; } res; register unsigned long tmp; do { asm ("mftbu %0\n\t" "mftb %1\n\t" "mftbu %2" : "=r"(res.xy.r3), "=r"(res.xy.r4), "=r"(tmp)); } while (res.xy.r3 != tmp); return res.r3_r4; } wait_ticks(unsigned long wt) { unsigned long long ticks_end; ticks_end = get_ticks() + wt; WATCHDOG_RESET; do { } while((signed long long)ticks_end - (signed long long)get_ticks() >= 0); }
On Sun, Jan 27, 2013 at 07:03:44AM -0000, =?utf-8?b?TWF0cyBLw6Rycm1hbiA8TWF0cy5LYXJybWFuQHRyaXRlY2guc2U+?= wrote: > 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, e.g. if the reset function is a > C function this will probably overwrite r0 and cause an endless loop. > > The following patch changes to using r14+r15 instead of r6+r7 (to resemble > what would have been generated by a C compiler) and saves all necessary > registers on the stack. > > The patch has been tested on a custom MPC5125 based machine using the 512x > powerpc architecture. > > Signed-off-by: Mats Karrman <mats.karrman@tritech.se> > Cc: Wolfgang Denk <wd@denx.de> > Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se> > Tested-by: Stefan Roese <sr@denx.de> This does not cleanly apply to u-boot/master, can you please resend based on top of tree? Thanks!
diff --git a/arch/powerpc/lib/ticks.S b/arch/powerpc/lib/ticks.S index 1781039..63114bb 100644 --- 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