[U-Boot] [PATCH 05/10] nds32: add nds32 board with common support

[Ben Warren]

Hello Macpaul,

On Fri, Jun 11, 2010 at 2:34 AM, Macpaul Lin <macpaul at andestech.com> wrote:

>        Add nds32 based common board related support.
>
> Signed-off-by: Macpaul Lin <macpaul at andestech.com>
> ---
>  board/AndesTech/common/env.c         |  138 ++++++
>  board/AndesTech/common/flash.c       |  621 +++++++++++++++++++++++++++
>  board/AndesTech/common/flib_flash.c  |  721
> ++++++++++++++++++++++++++++++++
>  board/AndesTech/common/flib_serial.c |  373 +++++++++++++++++
>  board/AndesTech/common/fotg2xx.c     |   60 +++
>  board/AndesTech/common/ftmac100.c    |  766
> ++++++++++++++++++++++++++++++++++
>  board/AndesTech/common/ftpci100.c    |  712
> +++++++++++++++++++++++++++++++
>  board/AndesTech/common/serial.c      |  141 +++++++
>
Please respect the file hierarchy.  Drivers go in the proper place (e.g.
flib_serial.c should go in drivers/serial, ftmac100.c should go in
drivers/net etc.)
<snip>

> diff --git a/board/AndesTech/common/ftmac100.c
> b/board/AndesTech/common/ftmac100.c
> new file mode 100644
> index 0000000..825031d
> --- /dev/null
> +++ b/board/AndesTech/common/ftmac100.c
> @@ -0,0 +1,766 @@
> +/*
> + * Copyright (C) 2009 Andes Technology Corporation
> + * Shawn Lin, Andes Technology Corporation <nobuhiro at andestech.com>
> + * Macpaul Lin, Andes Technology Corporation <macpaul at andestech.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +// --------------------------------------------------------------------
> +//     lmc83:  modified from smc91111.c (2002-11-29)
> +// --------------------------------------------------------------------
> +
> +
> +#include <common.h>
> +#include <asm/andesboot.h>
> +#include <malloc.h>
> +#include <command.h>
> +#include "../include/porting.h"
> +#include "../include/ftmac100.h"
> +#include <net.h>
> +
> +
> +#ifdef CONFIG_DRIVER_FTMAC100
> +
> +// Use power-down feature of the chip
> +#define POWER_DOWN     0
> +
> +static unsigned char ftmac100_mac_addr[] = {0x00, 0x41, 0x71, 0x99, 0x00,
> 0x00};
>
Please don't make assumptions like this.

> +
> +static const char version[] =
> +       "Faraday FTMAC100 Driver, (Linux Kernel 2.4) 10/18/02 - by
> Faraday\n";
>
I don't think so.

> +
> +#define inl(addr)                      (*((volatile u32 *)(addr)))
> +#define inw(addr)                      (*((volatile u16 *)(addr)))
> +#define outl(value, addr)      (*((volatile u32 *)(addr)) = value)
> +#define outb(value, addr)      (*((volatile u8 *)(addr)) = value)
>
This isn't the place to define these.

> +
> +struct net_device dev_eth0;
> +int tx_rx_cnt = 0;
> +/*
> + *
> + * Configuration options, for the experienced user to change.
> + *
> + */
> +/*
> + * DEBUGGING LEVELS
> + *
> + * 0 for normal operation
> + * 1 for slightly more details
> + * >2 for various levels of increasingly useless information
> + *     2 for interrupt tracking, status flags
> + *     3 for packet info
> + *     4 for complete packet dumps
> + */
> +
> +#define DO_PRINT(args...) printk(args)
> +
> +//#define FTMAC100_DEBUG 5 // Must be defined in makefile
> +
> +#if (FTMAC100_DEBUG > 2 )
> +#define PRINTK3(args...) DO_PRINT(args)
> +#else
> +#define PRINTK3(args...)
> +#endif
> +
> +#if FTMAC100_DEBUG > 1
> +#define PRINTK2(args...) DO_PRINT(args)
> +#else
> +#define PRINTK2(args...)
> +#endif
> +
> +#ifdef FTMAC100_DEBUG
> +#define PRINTK(args...) DO_PRINT(args)
> +#else
> +#define PRINTK(args...)
> +#endif
>
Please don't do this stuff.  The standard debug() will suffice.

> +
> +
> +///#define FTMAC100_TIMER
> +
> +/*
> + *
> + * The internal workings of the driver.  If you are changing anything
> + * here with the SMC stuff, you should have the datasheet and know
> + * what you are doing.
> + *
> + */
> +#define CARDNAME "FTMAC100"
> +
> +#ifdef FTMAC100_TIMER
> +       static struct timer_list ftmac100_timer;
> +#endif
> +
> +#define ETH_ZLEN 60
> +
> +#ifdef  CONFIG_SMC_USE_32_BIT
>
Please use your own CONFIG

> +#define USE_32_BIT
> +#else
> +#undef USE_32_BIT
>
This sort of thing needs more namespace

> +#endif
> +/*
> + *
> + * The driver can be entered at any of the following entry points.
> + *
> + */
> +
> +extern int eth_init(bd_t *bd);
> +extern void eth_halt(void);
> +extern int eth_rx(void);
> +extern int eth_send(volatile void *packet, int length);
>
You're using an API that's tagged as deprecated.  Please read the
documentation, update the driver to the appropriate API and then we'll
proceed with the review.

regards,
Ben