[U-Boot] [PATCH 01/10] nds32: add nds32 arch with cpu support

Fri Jun 11 12:47:50 CEST 2010

Dear Macpaul Lin,

In message <1276248884-21492-2-git-send-email-macpaul at andestech.com> you wrote:
> --===============1276623933==
> 
> 	Add nds32 architecture with generic cpu core support.
> 	Add nds32 architecture with n1213s cpu core support.

Please do not send patches as attachments, send them inline. Try using
"git format-patch" to create the patches and "git send-email" to send
them.

Don't indent the commit messages like that.

> diff --git a/arch/nds32/config.mk b/arch/nds32/config.mk
> new file mode 100644
> index 0000000..e88e516
> --- /dev/null
> +++ b/arch/nds32/config.mk
> @@ -0,0 +1,32 @@
> +#
> +# (C) Copyright 2000-2002
> +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> +#
> +# (C) Copyright 2006
> +# Shawn Lin, Andes Technology Corporation <nobuhiro at andestech.com>
> +# Macpaul Lin, Andes Technology Corporation <macpaul at andestech.com>

So no work has been done on this in the last 4 years?  I guess you
want to update your Copyright messages - all of them.

...
> +START	= start.o
> +COBJS	= interrupts.o cpu.o

Please keep all such lists sorted. 'c' < 'i'.

...
> +void do_reset (cmd_tbl_t *cmdtp, bd_t *bd, int flag, int argc, char *argv[])
> +{
> +	extern void reset_cpu(ulong addr);
> +
> +	disable_interrupts();
> +//	reset_cpu(0);	FIXME: -by Shawn, currently no ROM loader at addr 0

No C++ comments allowd. Please fix globally.

> +void  flush_cache (unsigned long dummy1, unsigned long dummy2)
> +{
> +/*
> +	unsigned long u32IfRun = 0;
> +
> +	if(u32IfRun)  return;
> +	u32IfRun = 1;

We do not allow CamelCase identifiers in U-Boot. Please fix globally.
Also, pay attentiuon to indentation rules, the "return" belongs on a
new line, indented by a TAB.

> +	reset_cpu((unsigned long)flush_cache);
> +	return;
> +*/

Please do not add dead code. Remove it. Please fix globally.

> --- /dev/null
> +++ b/arch/nds32/cpu/interrupts.c
...
> +//#include <board/AndesTech/include/porting.h>
> +#include "../../../board/AndesTech/include/porting.h"

This is fundeamentally flawed. Global code must never include any
board specific header files.

> +/*
> + * TimerBase[0] is a NULL entry
> + */
> +fLib_TimerReg *TimerBase[] ={0, (fLib_TimerReg *) NDS32_COMMON_TIMER1_BASE,
> +	(fLib_TimerReg *) NDS32_COMMON_TIMER2_BASE,(fLib_TimerReg *)NDS32_COMMON_TIMER3_BASE};

Lines too long. Please fix globally.

> +ulong get_timer_masked(void)
> +{
> +	ulong now = Read_Timer_Counter(1);;
> +
> +	if (lastdec >= now)
> +	{
> +		/* normal mode */
> +		timestamp += lastdec - now;
> +	} else {
> +		/* we have an overflow ... */
> +		timestamp += lastdec + TIMER_LOAD_VAL - now;
> +	}

Incorrect brace style. Please fix globally.

Please re-read the CodingStyle document!!!

...
> +uart_init:
> +#! 1. fLib_SetSerialMode(DebugSerialPort, SERIAL_MDR_UART);
> +	li      $r0, #0x99600000		!; CPE_UART4_BASE = 0x99600000
> +	lwi     $r1, [$r0+#0x20]		!; mdr = mdr = inw(CPE_UART4_BASE + SERIAL_MDR)
> +	li      $r2, #0xfffffffc		!; mdr &= ~(0x3) ; SERIAL_MDR_MODE_SEL = 0x3
> +	and     $r1, $r1, $r2
> +	swi     $r1, [$r0+#0x20]		!; outw(CPE_UART4_BASE + SERIAL_MDR, mdr | mode);
> +#! 2. fLib_SerialInit(DebugSerialPort, (int)DEFAULT_HOST_BAUD, PARITY_NONE, 0, 8);
> +#! dgbserialport = 0x99600000; baud = (UART_CLOCK / 614400) = 24; PARITY_NONE = 0; num = 0; len = 8;
> +	li      $r0, #0x99600000		!;lcr = inw(port + SERIAL_LCR) & ~SERIAL_LCR_DLAB; SERIAL_LCR = 0x0c
> +	lwi     $r1, [$r0+#0x0c]
> +	li      $r2, #0xffffff7f		!; SERIAL_LCR_DLAB = 0x80
> +	and     $r1, $r1, $r2
> +	li      $r2, #0x80			!; outw(port + SERIAL_LCR,SERIAL_LCR_DLAB);
> +	swi     $r2, [$r0+#0x0c]
> +	li      $r2, #0				!; outw(port + SERIAL_DLM, ((baudrate & 0xf00) >> 8));
> +	swi     $r2, [$r0+#0x4]
> +#ifdef __NDS32_N1213_43U1H__    /* AG101 */
> +	li      $r2, #60			!; outw(port + SERIAL_DLL, (baudrate & 0xff));
> +#else
> +	li      $r2, #24			!; outw(port + SERIAL_DLL, (baudrate & 0xff));
> +#endif
> +	swi     $r2, [$r0+#0x0]
> +	andi    $r1, $r1, #0xc0			!; lcr &= 0xc0;
> +	li      $r2, #3				!; len-=5;
> +	or      $r1, $r1, $r2			!; lcr|=len;
> +	swi     $r1, [$r0+#0x0c]		!; outw(port+SERIAL_LCR,lcr);
> +#! 3. fLib_SetSerialFifoCtrl(DebugSerialPort, 0, 0, ENABLE(1), ENABLE(1));
> +	li      $r0, #0x99600000
> +	li      $r1, #0x7			!; fcr = 0x7
> +	swi     $r1, [$r0+#0x8]			!; SERIAL_FCR = 0x08; outw(port+SERIAL_FCR,fcr);
> +
> +	ret

Lines too long - why don't you implement uart_init() in C?

> +void cleanup_before_linux(void)
> +{
> +	/*
> +	 * this function is called just before we call linux
> +	 * it prepares the processor for linux
> +	 *
> +	 * we disable interrupt and caches.
> +	 */
> +
> +#ifdef CONFIG_MMU
> +	unsigned long i;
> +#endif

Please move such block comments outside the functions. Functions
should normally start with declarations.  Please apply globally.

> diff --git a/arch/nds32/cpu/n1213s/config.mk b/arch/nds32/cpu/n1213s/config.mk
> new file mode 100644
> index 0000000..316332e
> --- /dev/null
> +++ b/arch/nds32/cpu/n1213s/config.mk
...
> +#
> +
> +PLATFORM_CPPFLAGS +=

This is a noop - why do we need this file?

> diff --git a/arch/nds32/cpu/n1213s/interrupts.c b/arch/nds32/cpu/n1213s/interrupts.c
> new file mode 100644
> index 0000000..a115a72
> --- /dev/null
> +++ b/arch/nds32/cpu/n1213s/interrupts.c
...
> + */
> +
> +#include "../interrupts.c"

Is this all? Then why do we need this file?

> diff --git a/arch/nds32/cpu/n1213s/lowlevel_init.S b/arch/nds32/cpu/n1213s/lowlevel_init.S
> new file mode 100644
> index 0000000..26e0727
> --- /dev/null
> +++ b/arch/nds32/cpu/n1213s/lowlevel_init.S
...
> +#include "../lowlevel_init.S"

Ditto? 

> diff --git a/arch/nds32/cpu/n1213s/start.S b/arch/nds32/cpu/n1213s/start.S
> new file mode 100644
> index 0000000..e93d048
> --- /dev/null
> +++ b/arch/nds32/cpu/n1213s/start.S
...
> +#include "../start.S"

And again.

This makes no sense.

Please find a better way to use common code.

...
> +__start_andesboot:	.word start_andesboot
> +
> +
> +
> +!=========================================================================

Too many blank lines. absolute maximum is 2. Please fix globally.

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
The high cost of living hasn't affected its popularity.