Re: [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform

2014-11-04 Thread Peng Fan 

Hi Simon,

在 11/4/2014 2:58 PM, Simon Glass 写道:

HI Peng,

On 31 October 2014 23:12, Peng Fan peng@freescale.com wrote:

The register save/restore:
Use get_bad_stack and bad_save_user_regs to save regs.
Introduce und_restore_regs to restore the previous regs before
trigger a breakpoint.

Signed-off-by: Peng Fan peng@freescale.com


Looks good so far as I understand it. A few nits below and maybe you
can integrate better with the ptrace register numbering.


---
  arch/arm/include/asm/proc-armv/ptrace.h |   2 +-
  arch/arm/include/asm/signal.h   |   1 +
  arch/arm/lib/Makefile   |   3 +
  arch/arm/lib/crt0.S |   4 +
  arch/arm/lib/interrupts.c   |  24 
  arch/arm/lib/kgdb/kgdb.c| 231 
  arch/arm/lib/kgdb/setjmp.S  |  20 +++
  arch/arm/lib/vectors.S  |  28 
  8 files changed, 312 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/proc-armv/ptrace.h 
b/arch/arm/include/asm/proc-armv/ptrace.h
index 71df5a9..33fe587 100644
--- a/arch/arm/include/asm/proc-armv/ptrace.h
+++ b/arch/arm/include/asm/proc-armv/ptrace.h
@@ -58,7 +58,7 @@ struct pt_regs {
 stack during a system call. */

  struct pt_regs {
-   long uregs[18];
+   unsigned long uregs[18];
  };

  #define ARM_cpsr   uregs[16]
diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h
new file mode 100644
index 000..7b1573c
--- /dev/null
+++ b/arch/arm/include/asm/signal.h
@@ -0,0 +1 @@
+#include asm-generic/signal.h
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 1ef2400..c543563 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -52,3 +52,6 @@ endif
  ifneq (,$(findstring -mabi=aapcs-linux,$(PLATFORM_CPPFLAGS)))
  extra-y+= eabi_compat.o
  endif
+
+obj-$(CONFIG_CMD_KGDB) += kgdb/setjmp.o
+obj-$(CONFIG_CMD_KGDB) += kgdb/kgdb.o
diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index 29cdad0..d96e70b 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -99,9 +99,13 @@ clr_gd:
 sub r9, r9, #GD_SIZE/* new GD is below bd */

 adr lr, here
+#ifndef CONFIG_CMD_KGDB
 ldr r0, [r9, #GD_RELOC_OFF] /* r0 = gd-reloc_off */
 add lr, lr, r0
 ldr r0, [r9, #GD_RELOCADDR] /* r0 = gd-relocaddr */
+#else
+  ldr r0, =__image_copy_start
+#endif
 b   relocate_code
  here:

diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c
index 4dacfd9..d16bd58 100644
--- a/arch/arm/lib/interrupts.c
+++ b/arch/arm/lib/interrupts.c
@@ -22,6 +22,7 @@
  #include common.h
  #include asm/proc-armv/ptrace.h
  #include asm/u-boot-arm.h
+#include kgdb.h

  DECLARE_GLOBAL_DATA_PTR;

@@ -160,9 +161,32 @@ void show_regs (struct pt_regs *regs)

  void do_undefined_instruction (struct pt_regs *pt_regs)
  {
+#if defined(CONFIG_CMD_KGDB)
+   unsigned long *tmp_pc = NULL;
+
+   pt_regs-ARM_pc -= 4;
+   tmp_pc = (unsigned long *)pt_regs-ARM_pc;
+
+   if (*tmp_pc == 0xe7ffdeff) {
+   pt_regs-ARM_pc += 4;
+   if (debugger_exception_handler 
+   debugger_exception_handler(pt_regs)) {
+   return;
+   }
+   } else if (*tmp_pc == 0xe7ffdefe) {
+   if (debugger_exception_handler 
+   debugger_exception_handler(pt_regs)) {
+   return;
+   }
+   } else {
+   printf(DCache/ICACHE May need flush\n);
+   return;
+   }
+#else
 printf (undefined instruction\n);
 show_regs (pt_regs);
 bad_mode ();
+#endif
  }

  void do_software_interrupt (struct pt_regs *pt_regs)
diff --git a/arch/arm/lib/kgdb/kgdb.c b/arch/arm/lib/kgdb/kgdb.c
new file mode 100644
index 000..6b2e542
--- /dev/null
+++ b/arch/arm/lib/kgdb/kgdb.c
@@ -0,0 +1,231 @@
+#include common.h
+#include command.h
+#include kgdb.h
+#include asm/signal.h
+#include asm/processor.h
+
+#define KGDB_ARM_GP_REGS   16
+#define KGDB_ARM_FP_REGS   8
+#define KGDB_ARM_EXTRA_REGS2
+#define KGDB_ARM_MAX_REGS  (KGDB_ARM_GP_REGS +\
+(KGDB_ARM_FP_REGS * 3) +\
+ KGDB_ARM_EXTRA_REGS)
+#define KGDB_NUMREGBYTES   (KGDB_ARM_MAX_REGS  2)
+
+enum arm_regs {
+   KGDB_ARM_R0,
+   KGDB_ARM_R1,
+   KGDB_ARM_R2,
+   KGDB_ARM_R3,
+   KGDB_ARM_R4,
+   KGDB_ARM_R5,
+   KGDB_ARM_R6,
+   KGDB_ARM_R7,
+   KGDB_ARM_R8,
+   KGDB_ARM_R9,
+   KGDB_ARM_R10,
+   KGDB_ARM_FP,
+   KGDB_ARM_IP,
+   KGDB_ARM_SP,
+   KGDB_ARM_LR,
+   KGDB_ARM_PC,


So in here there are the FP and EXTRA regs? If you added them to this
enum it would be clearer.
I did not test FP and other extra regs. Actually in uboot, the FP and 
extra regs are used? I am not sure.


Also these mirror the numbers in ptrace.h so I

Re: [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform

2014-11-03 Thread Simon Glass 
HI Peng,

On 31 October 2014 23:12, Peng Fan peng@freescale.com wrote:
 The register save/restore:
 Use get_bad_stack and bad_save_user_regs to save regs.
 Introduce und_restore_regs to restore the previous regs before
 trigger a breakpoint.

 Signed-off-by: Peng Fan peng@freescale.com

Looks good so far as I understand it. A few nits below and maybe you
can integrate better with the ptrace register numbering.

 ---
  arch/arm/include/asm/proc-armv/ptrace.h |   2 +-
  arch/arm/include/asm/signal.h   |   1 +
  arch/arm/lib/Makefile   |   3 +
  arch/arm/lib/crt0.S |   4 +
  arch/arm/lib/interrupts.c   |  24 
  arch/arm/lib/kgdb/kgdb.c| 231 
 
  arch/arm/lib/kgdb/setjmp.S  |  20 +++
  arch/arm/lib/vectors.S  |  28 
  8 files changed, 312 insertions(+), 1 deletion(-)

 diff --git a/arch/arm/include/asm/proc-armv/ptrace.h 
 b/arch/arm/include/asm/proc-armv/ptrace.h
 index 71df5a9..33fe587 100644
 --- a/arch/arm/include/asm/proc-armv/ptrace.h
 +++ b/arch/arm/include/asm/proc-armv/ptrace.h
 @@ -58,7 +58,7 @@ struct pt_regs {
 stack during a system call. */

  struct pt_regs {
 -   long uregs[18];
 +   unsigned long uregs[18];
  };

  #define ARM_cpsr   uregs[16]
 diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h
 new file mode 100644
 index 000..7b1573c
 --- /dev/null
 +++ b/arch/arm/include/asm/signal.h
 @@ -0,0 +1 @@
 +#include asm-generic/signal.h
 diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
 index 1ef2400..c543563 100644
 --- a/arch/arm/lib/Makefile
 +++ b/arch/arm/lib/Makefile
 @@ -52,3 +52,6 @@ endif
  ifneq (,$(findstring -mabi=aapcs-linux,$(PLATFORM_CPPFLAGS)))
  extra-y+= eabi_compat.o
  endif
 +
 +obj-$(CONFIG_CMD_KGDB) += kgdb/setjmp.o
 +obj-$(CONFIG_CMD_KGDB) += kgdb/kgdb.o
 diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
 index 29cdad0..d96e70b 100644
 --- a/arch/arm/lib/crt0.S
 +++ b/arch/arm/lib/crt0.S
 @@ -99,9 +99,13 @@ clr_gd:
 sub r9, r9, #GD_SIZE/* new GD is below bd */

 adr lr, here
 +#ifndef CONFIG_CMD_KGDB
 ldr r0, [r9, #GD_RELOC_OFF] /* r0 = gd-reloc_off */
 add lr, lr, r0
 ldr r0, [r9, #GD_RELOCADDR] /* r0 = gd-relocaddr */
 +#else
 +  ldr r0, =__image_copy_start
 +#endif
 b   relocate_code
  here:

 diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c
 index 4dacfd9..d16bd58 100644
 --- a/arch/arm/lib/interrupts.c
 +++ b/arch/arm/lib/interrupts.c
 @@ -22,6 +22,7 @@
  #include common.h
  #include asm/proc-armv/ptrace.h
  #include asm/u-boot-arm.h
 +#include kgdb.h

  DECLARE_GLOBAL_DATA_PTR;

 @@ -160,9 +161,32 @@ void show_regs (struct pt_regs *regs)

  void do_undefined_instruction (struct pt_regs *pt_regs)
  {
 +#if defined(CONFIG_CMD_KGDB)
 +   unsigned long *tmp_pc = NULL;
 +
 +   pt_regs-ARM_pc -= 4;
 +   tmp_pc = (unsigned long *)pt_regs-ARM_pc;
 +
 +   if (*tmp_pc == 0xe7ffdeff) {
 +   pt_regs-ARM_pc += 4;
 +   if (debugger_exception_handler 
 +   debugger_exception_handler(pt_regs)) {
 +   return;
 +   }
 +   } else if (*tmp_pc == 0xe7ffdefe) {
 +   if (debugger_exception_handler 
 +   debugger_exception_handler(pt_regs)) {
 +   return;
 +   }
 +   } else {
 +   printf(DCache/ICACHE May need flush\n);
 +   return;
 +   }
 +#else
 printf (undefined instruction\n);
 show_regs (pt_regs);
 bad_mode ();
 +#endif
  }

  void do_software_interrupt (struct pt_regs *pt_regs)
 diff --git a/arch/arm/lib/kgdb/kgdb.c b/arch/arm/lib/kgdb/kgdb.c
 new file mode 100644
 index 000..6b2e542
 --- /dev/null
 +++ b/arch/arm/lib/kgdb/kgdb.c
 @@ -0,0 +1,231 @@
 +#include common.h
 +#include command.h
 +#include kgdb.h
 +#include asm/signal.h
 +#include asm/processor.h
 +
 +#define KGDB_ARM_GP_REGS   16
 +#define KGDB_ARM_FP_REGS   8
 +#define KGDB_ARM_EXTRA_REGS2
 +#define KGDB_ARM_MAX_REGS  (KGDB_ARM_GP_REGS +\
 +(KGDB_ARM_FP_REGS * 3) +\
 + KGDB_ARM_EXTRA_REGS)
 +#define KGDB_NUMREGBYTES   (KGDB_ARM_MAX_REGS  2)
 +
 +enum arm_regs {
 +   KGDB_ARM_R0,
 +   KGDB_ARM_R1,
 +   KGDB_ARM_R2,
 +   KGDB_ARM_R3,
 +   KGDB_ARM_R4,
 +   KGDB_ARM_R5,
 +   KGDB_ARM_R6,
 +   KGDB_ARM_R7,
 +   KGDB_ARM_R8,
 +   KGDB_ARM_R9,
 +   KGDB_ARM_R10,
 +   KGDB_ARM_FP,
 +   KGDB_ARM_IP,
 +   KGDB_ARM_SP,
 +   KGDB_ARM_LR,
 +   KGDB_ARM_PC,

So in here there are the FP and EXTRA regs? If you added them to this
enum it would be clearer.

Also these mirror the numbers in ptrace.h so I wonder if somehow they
could be linked?

 +