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

[Tonny Tzeng]

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 at 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