Mike, thanks for your review. Here need your further clarifications, thanks in advance.
On Fri, Apr 9, 2010 at 5:36 AM, Mike Frysinger vapier@gentoo.org wrote:
On Thursday 08 April 2010 05:40:02 Tonny Tzeng wrote:
--- a/common/kgdb.c +++ b/common/kgdb.c @@ -220,6 +220,29 @@ hexToInt(char **ptr, int *intValue) return (numChars); }
+/* Handle the 'z' or 'Z' breakpoint remove or set packets */ +static void gdb_cmd_break(kgdb_data *kdp)
this breakpoint support isnt related to ARM KGDB at all. it should be split into a separate changeset.
Yes, these 2 breakpoint commands are not specific to ARM, but I think they are mandatory required for debugging ARM kernel space, otherwise gdb will use M command to write a SWI instruction code to the trap location.
Anyway, agree with you that it might be more clear to separate these code to other patch, as this is going to modify to common/kgdb.c which is architecture / platform independent.
if (kgdb_setjmp(error_jmp_buf) != 0) panic("kgdb: error or fault in entry init!\n");
- memset(&kd, 0, sizeof(kgdb_data));
there is no logic as to why this is needed. should probably be a different changeset too. personally, i'd also use sizeof(kd).
Since I added a (*hook_break)() to the kgdb_data structure, and in order to make it transparent to other architectures (e.g. ppc, blackfin), I believe we need to clear the entire kd here. But will separate this into other patchset as your suggestion.
--- a/include/kgdb.h +++ b/include/kgdb.h +#ifndef BREAK_INSTR_SIZE +#define BREAK_INSTR_SIZE 4 +#endif
i'm not sure this makes sense. every arch should define it themselves.
Since this include/kgdb.h file has been referenced by other architectures, the existing code may not define BREAK_INSTR_SIZE, so this is to prevent the new added code break the KGDB support for other architectures.
--- a/lib_arm/board.c +++ b/lib_arm/board.c @@ -381,6 +382,11 @@ void start_armboot (void) /* miscellaneous platform dependent initialisations */ misc_init_r (); #endif +#if defined(CONFIG_CMD_KGDB)
- WATCHDOG_RESET();
- puts("KGDB: ");
- kgdb_init();
+#endif
the serial funcs (like puts()) already call the watchdog funcs. this call is spurious. -mike
These code are identical as ppc, m68k, and i386 architectures, and I check the serial driver of my board, it looks like some serial drivers may not call watchdog funcs, so I'd keep these changes. Any comment on this? Thanks.
Best Regards, Tonny