Dear Antonio, in message <[email protected]> you wrote: > > I've just written and tested this kgdb patch for ARM920T cpus. > The development was aimed at providing a low-cost cross-debugging > environment for a Samsung ARM board produced from SERP (www.serp.it) > used in an Computer Architecture course taught at the university of > Padova (IT) (www.dei.unipd.it). > > It works with gdb software breakpoint (on the host computer). Also > with an arm-linux- toolchain.
Thanks a lot for your contribution. Unfortunately I have a few formal change requests. Please see http://www.denx.de/wiki/U-Boot/CodingStyle and http://www.denx.de/wiki/U-Boot/Patches for more detailed instructions. First: please submit your patch with an appropriate commit message, and make sure to add your Signed-off-by: line. > --- u-boot/common/kgdb.c 2007-11-06 18:07:07.000000000 +0100 > +++ u-boot-UniPd/common/kgdb.c 2009-02-23 16:47:31.000000000 +0100 > @@ -322,7 +322,7 @@ > > kgdb_interruptible(0); > > - printf("kgdb: handle_exception; trap [0x%x]\n", kgdb_trap(regs)); > +// printf("kgdb: handle_exception; trap [0x%x]\n", kgdb_trap(regs)); Are you sure it is a good idea to comment out this code? Anyway - if you want to remove this line, then really remove it; don;t just leve it commented out. Also, please do not use C++ comments. > if (kgdb_setjmp((long*)error_jmp_buf) != 0) > panic("kgdb: error or fault in entry init!\n"); > @@ -457,10 +457,11 @@ > } > > goto doexit; > - > - case 'S': /* SSS single step with signal SS */ > - *ptr = '\0'; /* ignore the signal number for now */ > - /* fall through */ > +/* > +#ifdef _PPC > + case 'S': // SSS single step with signal SS > + *ptr = '\0'; // ignore the signal number for now > + // fall through > > case 's': > kd.extype = KGDBEXIT_SINGLE; > @@ -469,7 +470,8 @@ > kd.exaddr = addr; > kd.extype |= KGDBEXIT_WITHADDR; > } > - > +#endif > +*/ It seems here you are removing PowerPC support? Why that? > doexit: > /* Need to flush the instruction cache here, as we may have deposited a > * breakpoint, and the icache probably has no way of knowing that a data re> > f to > @@ -496,8 +498,12 @@ > } else { > kgdb_error(KGDBERR_BADPARAMS); > } > - break; > - } /* switch */ > + case 'q': > + if (!(strcmp(ptr, "Supported"))) { > + sprintf(remcomOutBuffer, "PacketSize=%d", > BUFMAX); > + } > + break; > + } /* switch */ What exactly is that? > if (errnum != 0) > sprintf(remcomOutBuffer, "E%02d", errnum); > diff -r -u u-boot/cpu/arm920t/Makefile u-boot-UniPd/cpu/arm920t/Makefile > --- u-boot/cpu/arm920t/Makefile 2007-11-07 17:18:30.000000000 +0100 > +++ u-boot-UniPd/cpu/arm920t/Makefile 2009-01-15 17:01:58.000000000 +0100 > @@ -26,7 +26,8 @@ > LIB = $(obj)lib$(CPU).a > > START = start.o > -COBJS = cpu.o interrupts.o > +COBJS = cpu.o interrupts.o > +SOBJS = kgdb.o > > SRCS := $(START:.o=.S) $(SOBJS:.o=.S) $(COBJS:.o=.c) > OBJS := $(addprefix $(obj),$(COBJS) $(SOBJS)) > diff -r -u u-boot/cpu/arm920t/interrupts.c u-boot-UniPd/cpu/arm920t/interrup> > ts.c > --- u-boot/cpu/arm920t/interrupts.c 2007-11-06 18:11:29.000000000 +0100 > +++ u-boot-UniPd/cpu/arm920t/interrupts.c 2009-01-19 22:55:29.000000000 > +0> 100 ... > void do_software_interrupt (struct pt_regs *pt_regs) > { > +/* > +#if defined(CONFIG_CMD_KGDB) > + if (debugger_exception_handler && > (*debugger_exception_handler)(pt_regs)> ) > + return; > +#endif > +*/ > printf ("software interrupt\n"); > show_regs (pt_regs); > bad_mode (); > @@ -133,13 +150,24 @@ > > void do_prefetch_abort (struct pt_regs *pt_regs) > { > +/* > +#if defined(CONFIG_CMD_KGDB) > + if (debugger_exception_handler && > (*debugger_exception_handler)(pt_regs)> ) > + return; > +#endif > +*/ > printf ("prefetch abort\n"); > show_regs (pt_regs); > bad_mode (); > } It seems the added code is commented out? Please do not add dead code. > void do_data_abort (struct pt_regs *pt_regs) > -{ > +{/* > +#if defined(CONFIG_CMD_KGDB) > + if (debugger_exception_handler && > (*debugger_exception_handler)(pt_regs)> ) > + return; > +#endif > +*/ Ditto. > printf ("data abort\n"); > show_regs (pt_regs); > bad_mode (); > @@ -154,6 +182,12 @@ > > void do_fiq (struct pt_regs *pt_regs) > { > +/* > +#if defined(CONFIG_CMD_KGDB) > + if (debugger_exception_handler && > (*debugger_exception_handler)(pt_regs)> ) > + return; > +#endif > +*/ And again. > printf ("fast interrupt request\n"); > show_regs (pt_regs); > bad_mode (); > @@ -161,19 +195,27 @@ > > void do_irq (struct pt_regs *pt_regs) > { > -#if defined (CONFIG_USE_IRQ) > -#if defined (ARM920_IRQ_CALLBACK) > - ARM920_IRQ_CALLBACK(); > - return; > -#elif defined (CONFIG_ARCH_INTEGRATOR) > +//#if defined (CONFIG_USE_IRQ) > +//#if defined (ARM920_IRQ_CALLBACK) > +// ARM920_IRQ_CALLBACK(); > +// return; > +//#elif defined (CONFIG_ARCH_INTEGRATOR) > /* ASSUMED to be a timer interrupt */ > /* Just clear it - count handled in */ > /* integratorap.c */ > - *(volatile ulong *)(CFG_TIMERBASE + 0x0C) = 0; > -#endif /* ARCH_INTEGRATOR */ > -#else > +// *(volatile ulong *)(CFG_TIMERBASE + 0x0C) = 0; > +//#endif /* ARCH_INTEGRATOR */ > +//#else That doesn't look clean to me. > +/* > +#if defined(CONFIG_CMD_KGDB) > + if (debugger_exception_handler && > (*debugger_exception_handler)(pt_regs)> ) > + return; > +#endif > +*/ Please do not add dead code. > diff -r -u u-boot/cpu/arm920t/s3c24x0/serial.c u-boot-UniPd/cpu/arm920t/s3c2> > 4x0/serial.c > --- u-boot/cpu/arm920t/s3c24x0/serial.c 2007-11-06 18:13:13.000000000 > +010> 0 > +++ u-boot-UniPd/cpu/arm920t/s3c24x0/serial.c 2009-01-15 17:29:24.00000000> 0 > +0100 > @@ -306,6 +306,114 @@ > > #endif /* CONFIG_SERIAL_MULTI */ > > +#if defined(CONFIG_CMD_KGDB) > +/* > + AS HARNOIS : according to CONFIG_KGDB_SER_INDEX kgdb uses serial port Was this comment really written by As. Harnois? > + number 0 or number 1 > + - if CONFIG_KGDB_SER_INDEX = 1 => serial port number 0 : > + configuration has been already done > + - if CONFIG_KGDB_SER_INDEX = 2 => serial port number 1 : > + configure port 1 for serial I/O with rate = CONFIG_KGDB_BAUDRATE > +*/ > +#if (CONFIG_KGDB_SER_INDEX & 2) > +void kgdb_serial_init (void) > +{ > + volatile char val; > + unsigned short br_reg; > + > + get_clocks (); > + br_reg = (((((gd->cpu_clk / 16) / 18) * 10) / CONFIG_KGDB_BAUDRATE) + > + 5) / 10; > + /* > + * Init onboard 16550 UART > + */ > + out8 (ACTING_UART1_BASE + UART_LCR, 0x80); /* set DLAB bit */ > + out8 (ACTING_UART1_BASE + UART_DLL, (br_reg & 0x00ff)); /* set divisor> > for 9600 baud */ > + out8 (ACTING_UART1_BASE + UART_DLM, ((br_reg & 0xff00) >> 8)); /* set > > divisor for 9600 baud */ Lines too long. > diff -r -u u-boot/cpu/arm920t/start.S u-boot-UniPd/cpu/arm920t/start.S > --- u-boot/cpu/arm920t/start.S 2008-01-21 11:27:32.000000000 +0100 > +++ u-boot-UniPd/cpu/arm920t/start.S 2009-01-21 16:21:13.000000000 +0100 > @@ -160,6 +160,8 @@ > */ > > start_code: > +#define START_SVC32 1 > +#if defined (START_SVC32) > /* > * set the cpu to SVC32 mode > */ > @@ -167,6 +169,8 @@ > bic r0,r0,#0x1f > orr r0,r0,#0xd3 > msr cpsr,r0 > +#else > +#endif Please do not do such things in common code. Either add new code, or not. But always without such bogus #define's. Also, never add empty #else clauses. > @@ -757,8 +761,8 @@ > #define I_BIT 0x80 > > /* > - * use bad_save_user_regs for abort/prefetch/undef/swi ... > - * use irq_save_user_regs / irq_restore_user_regs for IRQ/FIQ handling > + * use bad_save_user_regs for abort/prefetch/undef/swi > + * use irq_save_user_regs / irq_restore_user_regs for IRQ/FIQ handling Please do not add trailing white space. ... > + mov r1, sp @ r1 and > + mov r2, lr @ r2 then > + msr cpsr_c, r0 @ return in the correct mode and Use TABs for indentation only. > diff -r -u u-boot/examples/Makefile u-boot-UniPd/examples/Makefile > --- u-boot/examples/Makefile 2007-11-06 18:07:07.000000000 +0100 > +++ u-boot-UniPd/examples/Makefile 2009-01-15 19:54:59.000000000 +0100 > @@ -30,7 +30,7 @@ > endif > > ifeq ($(ARCH),arm) > -LOAD_ADDR = 0xc100000 > +LOAD_ADDR = 0x30000000 > endif Are you aware that this is a global file, used by all ARM systems? You must never change such files just as you like. Also, this changes is completely untelated to the rest of your patch. > ifeq ($(ARCH),mips) > diff -r -u u-boot/examples/hello_world.c u-boot-UniPd/examples/hello_world.c > --- u-boot/examples/hello_world.c 2007-11-06 18:07:07.000000000 +0100 > +++ u-boot-UniPd/examples/hello_world.c 2009-01-19 15:14:08.000000000 > +010> 0 > @@ -49,6 +49,14 @@ > /* consume input */ > (void) getc(); > > - printf ("\n\n"); > +// asm volatile ("\t.long 0xe7f001f0\n"); > + kgdb_breakpoint(argc, argv); > + > + printf ("break0 test\n\n"); > + printf ("break1 test\n\n"); > + printf ("break2 test\n\n"); > + > +// asm volatile ("\t.long 0xef9f0001\n"); > + > return (0); Ditto. > } > diff -r -u u-boot/include/_exports.h u-boot-UniPd/include/_exports.h > --- u-boot/include/_exports.h 2007-11-06 18:07:07.000000000 +0100 > +++ u-boot-UniPd/include/_exports.h 2009-01-19 15:16:07.000000000 +0100 > @@ -24,3 +24,6 @@ > EXPORT_FUNC(i2c_write) > EXPORT_FUNC(i2c_read) > #endif > +#ifdef CONFIG_CMD_KGDB > +EXPORT_FUNC(kgdb_breakpoint) > +#endif > \ No newline at end of file ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Fix that error. And make sure that the file still compiles for other architectures. > diff -r -u u-boot/include/exports.h u-boot-UniPd/include/exports.h > --- u-boot/include/exports.h 2007-11-06 18:07:07.000000000 +0100 > +++ u-boot-UniPd/include/exports.h 2009-01-19 15:14:12.000000000 +0100 > @@ -25,6 +25,9 @@ > void setenv (char *varname, char *varvalue); > long simple_strtol(const char *cp,char **endp,unsigned int base); > int strcmp(const char * cs,const char * ct); > +#ifdef CONFIG_CMD_KGDB > +void kgdb_breakpoint(int argc, char *argv[]); > +#endif > #ifdef CONFIG_HAS_UID > void forceenv (char *varname, char *varvalue); > #endif After changing the exports list, you should also increment the version of the exported interface. > diff -r -u u-boot/lib_arm/board.c u-boot-UniPd/lib_arm/board.c > --- u-boot/lib_arm/board.c 2007-11-06 18:07:08.000000000 +0100 > +++ u-boot-UniPd/lib_arm/board.c 2009-01-15 20:48:37.000000000 +0100 > @@ -284,6 +284,8 @@ > } > } > > + > + Too many empty linmes. Please clean up and resubmit. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [email protected] Der Dativ ist dem Genitiv sein Tod. _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
