[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