[U-Boot] U-BOOT kgdb support for ARM920T (s3c2440)
Wolfgang Denk
wd at denx.de
Tue May 5 15:44:29 CEST 2009
Dear Antonio,
in message <20090505134728.464224dhyq8huesk at webmail.igi.cnr.it> 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: wd at denx.de
Der Dativ ist dem Genitiv sein Tod.
More information about the U-Boot
mailing list