Dear Macpaul Lin,

In message <[email protected]> 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, [email protected].
> +#
> +# (C) Copyright 2006
> +# Shawn Lin, Andes Technology Corporation <[email protected]>
> +# Macpaul Lin, Andes Technology Corporation <[email protected]>

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: [email protected]
The high cost of living hasn't affected its popularity.
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to