[U-Boot] [PATCH 04/10] nds32: add nds32 board with ag101 support

[Wolfgang Denk]

Dear Macpaul Lin,

In message <1276248884-21492-5-git-send-email-macpaul at andestech.com> you wrote:
> 	Add nds32 based board adp-ag101 support.
> 
> Signed-off-by: Macpaul Lin <macpaul at andestech.com>
> ---
>  board/AndesTech/adp-ag101/Makefile    |   60 ++++++++++++++
>  board/AndesTech/adp-ag101/adp-ag101.c |  144 +++++++++++++++++++++++++++++++++
>  board/AndesTech/adp-ag101/config.mk   |   46 +++++++++++
>  3 files changed, 250 insertions(+), 0 deletions(-)
>  create mode 100644 board/AndesTech/adp-ag101/Makefile
>  create mode 100644 board/AndesTech/adp-ag101/adp-ag101.c
>  create mode 100644 board/AndesTech/adp-ag101/config.mk

This makes no sense.  You need to refactor your patches.  Everything
that logically belongs together should be within one patch only.

So please add ALL board support with a single commit.


Ditto for the CPU support.


Also,please note that entries to MAKEALL and MAINTAINERS files are
missing.

> --- /dev/null
> +++ b/board/AndesTech/adp-ag101/adp-ag101.c
...
> +#include <asm/andesboot.h>
> +//#include <porting.h>
> +#include "../include/porting.h"

Try do do without such "../include/porting.h" includes.

> +/*
> + *	bank_num ==> setting the config of which bank (0, 1st, 2nd, ..)
> + *	data_width ==> the width of the SDRAM module on the bank
> + *	sdram_size ==> the size of each module
> + *	bus_width ==> the width of data bus
> + *	bank_size ==> the total memory size of the bank (should be equal as "sdram_size * how many modules")
> + */
> +#define SDRAM_MODULE_WIDTH_4			0
> +#define SDRAM_MODULE_WIDTH_8			(1<<12)
> +#define SDRAM_MODULE_WIDTH_16			(2<<12)
> +#define SDRAM_MODULE_WIDTH_32			(3<<12)
> +
> +
> +// 16M bit, 64M bit,...
> +#define SDRAM_MODULE_SIZE_16M			0
> +#define SDRAM_MODULE_SIZE_64M			(1<<8)
> +#define SDRAM_MODULE_SIZE_128M			(2<<8)
> +#define SDRAM_MODULE_SIZE_256M			(3<<8)
> +
> +#define SDRAM_BUS_WIDTH_8			0
> +#define SDRAM_BUS_WIDTH_16			(1<<4)
> +#define SDRAM_BUS_WIDTH_32			(2<<4)
> +
> +// 1M byte, 2M byte,...
> +#define SDRAM_BANK_SIZE_1M			0
> +#define SDRAM_BANK_SIZE_2M			1
> +#define SDRAM_BANK_SIZE_4M			2
> +#define SDRAM_BANK_SIZE_8M			3
> +#define SDRAM_BANK_SIZE_16M			4
> +#define SDRAM_BANK_SIZE_32M			5
> +#define SDRAM_BANK_SIZE_64M			6
> +#define SDRAM_BANK_SIZE_128M			7
> +#define SDRAM_BANK_SIZE_256M			8

I don't like this; the resulting code will become unreadable because
of too many too long macro names.



I stop reviewing your patches here.  Most of my previous comments
apply globally, i. e. please make sure to rework all your code. Then
refactor the patches into logical, bisectable units, and resubmit.

Thanks.

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
I object to intellect without discipline;  I object to power without
constructive purpose.
	-- Spock, "The Squire of Gothos", stardate 2124.5