On Friday 09 April 2010 11:32:19 Tonny Tzeng wrote:
On Fri, Apr 9, 2010 at 5:36 AM, Mike Frysinger wrote:
On Thursday 08 April 2010 05:40:02 Tonny Tzeng wrote:
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.
hmm, kgdb_enter() is more for initializing state, not new hooks. my understanding is that either an arch always implements hook_break or it doesnt, it doesnt just do it some of the time.
that means the better way to add breakpoint support is via kgdb_stubs.c. add a new weak func to that where it always returns 0. then the remaining kgdb code can always assume the func exists and merely checks the return value.
although, now that i look at the breakpoint logic added to lib_arm/kgdb.c, it looks like most of that should be common code. the interface between common bp code and arch bp code should be the breakinst value (perhaps an unsigned char array encoding the opcode) and the size of the breakpoint array.
also, unless i missed something, your code that sets the bp is missing an icache flush.
--- 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.
i think an undefined size should be used as a tip that the arch doesnt yet support break points
--- 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.
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?
historical warts should be fixed, not extended. in other words, any serial driver not calling WATCHDOG_RESET() in putc()/tstc() is broken. i know this isnt documented properly, but it's still true. look at the cpu/blackfin/serial.c driver for a simple example. -mike