[U-Boot] [PATCH] Add KGDB support for ARM platforms

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100409/cac18593/attachment.pgp