[U-Boot] [PATCH v6 5/5] mpc85xx: Add gdsys ControlCenter Digital board

[Wolfgang Denk]

Dear dirk.eibach at gdsys.cc,

In message <1371024486-15629-6-git-send-email-dirk.eibach at gdsys.cc> you wrote:
> 
> The gdsys ControlCenter Digital board is based on a Freescale P1022 QorIQ SOC.
> It boots from SPI-Flash but can be configured to boot from SD-card for
> factory programming and testing.
> On board peripherals include:

> diff --git a/.checkpatch.conf b/.checkpatch.conf
> index d88af57..ef9b595 100644
> --- a/.checkpatch.conf
> +++ b/.checkpatch.conf
> @@ -2,10 +2,10 @@
>  --no-tree
>  
>  # Temporary for false positive in checkpatch
> ---ignore COMPLEX_MACRO
> +#--ignore COMPLEX_MACRO
>  
>  # For CONFIG_SYS_I2C_NOPROBES
> ---ignore MULTISTATEMENT_MACRO_USE_DO_WHILE
> +#--ignore MULTISTATEMENT_MACRO_USE_DO_WHILE

NAK.  Please do not mess with public settings.


> +static void *compute_and(void *_dst, const void *_src, size_t n)
> +{
> +	uint8_t *dst = _dst;
> +	const uint8_t *src = _src;
> +	size_t i;
> +	for (i = n; i-- > 0; )
> +		*dst++ &= *src++;
> +	return _dst;
> +}

Here and everywhere else: please separate declarations and code by
inserting a blank line.

> +			for (i = 0; i < 20; ++i)
> +				if (src_reg->digest[i])
> +					return NULL;

This muti-line statement requires braces.

> +enum {
> +	REG_REFLECTION_LOW = 0x0000,
> +	REG_VERSIONS = 0x0004,
> +	REG_FPGA_VERSION = 0x0008,
> +	REG_FEATURES = 0x000C,
> +	REG_TOP_INTERRUPT = 0x0010,
> +	REG_TOP_INTERRUPT_SET = 0x0014,
> +	REG_TOP_INTERRUPT_CLEAR = 0x0018,
> +	REG_STATUS = 0x001C,
> +	REG_CONTROL = 0x0020,
> +	REG_TESTMEM1 = 0x100,
> +	REG_DMA_WRITE_CONTROL = 0x0200,
> +	REG_DMA_WRITE_BASE_ADDRESS_LOW = 0x0204,
> +	REG_DMA_WRITE_BASE_ADDRESS_HIGH = 0x0208,
> +	REG_DMA_WRITE_LENGTH = 0x020C,
> +	REG_DMA_WRITE_HEAD = 0x0210,
> +	REG_DMA_WRITE_TAIL = 0x0214,
> +	REG_DMA_WRITE_MIN_INT_INTERVAL = 0x0218,
> +	REG_DMA_WRITE_MAX_PACKETS_INT_INTERVAL = 0x021C,
> +	REG_DMA_READ_CONTROL = 0x0300,
> +	REG_DMA_READ_BASE_ADDRESS_LOW = 0x0304,
> +	REG_DMA_READ_BASE_ADDRESS_HIGH = 0x0308,
> +	REG_DMA_READ_LENGTH = 0x030C,
> +	REG_DMA_READ_HEAD = 0x0310,
> +	REG_DMA_READ_TAIL = 0x0314,
> +	REG_DMA_READ_MIN_INT_INTERVAL = 0x0318,
> +	REG_DMA_READ_MAX_PACKETS_INT_INTERVAL = 0x031C,
> +};

This looks very much like register offsets to me, that should rather
be represented by a C struct ?

> --- a/boards.cfg
> +++ b/boards.cfg
> @@ -904,6 +904,13 @@ BSC9132QDS_SDCARD_DDRCLK100  powerpc     mpc85xx     bsc9132qds          freesca
>  BSC9132QDS_SDCARD_DDRCLK133  powerpc     mpc85xx     bsc9132qds          freescale      -           BSC9132QDS:BSC9132QDS,SDCARD,SYS_CLK_100_DDR_133
>  BSC9132QDS_SPIFLASH_DDRCLK100 powerpc    mpc85xx     bsc9132qds          freescale      -           BSC9132QDS:BSC9132QDS,SPIFLASH,SYS_CLK_100_DDR_100
>  BSC9132QDS_SPIFLASH_DDRCLK133 powerpc    mpc85xx     bsc9132qds          freescale      -           BSC9132QDS:BSC9132QDS,SPIFLASH,SYS_CLK_100_DDR_133
> +controlcenterd_36BIT_SDCARD         powerpc     mpc85xx     p1022             gdsys      -           controlcenterd:36BIT,SDCARD
> +controlcenterd_36BIT_SDCARD_DEVELOP powerpc     mpc85xx     p1022             gdsys      -           controlcenterd:36BIT,SDCARD,DEVELOP
> +controlcenterd_36BIT_SPIFLASH       powerpc     mpc85xx     p1022             gdsys      -           controlcenterd:36BIT,SPIFLASH
> +controlcenterd_SDCARD               powerpc     mpc85xx     p1022             gdsys      -           controlcenterd:SDCARD
> +controlcenterd_SPIFLASH             powerpc     mpc85xx     p1022             gdsys      -           controlcenterd:SPIFLASH
> +controlcenterd_TRAILBLAZER          powerpc     mpc85xx     p1022             gdsys      -           controlcenterd:TRAILBLAZER,SPIFLASH
> +controlcenterd_TRAILBLAZER_DEVELOP  powerpc     mpc85xx     p1022             gdsys      -           controlcenterd:TRAILBLAZER,SPIFLASH,DEVELOP
>  stxgp3                       powerpc     mpc85xx     stxgp3              stx
>  stxssa                       powerpc     mpc85xx     stxssa              stx            -           stxssa
>  stxssa_4M                    powerpc     mpc85xx     stxssa              stx            -           stxssa:STXSSA_4M

Is it really, _really_ necessary to add 7 entries just for a single
board?   Please decide which configurations you really need, and omit
the rest.

...
> +#define CONFIG_HDBOOT					\
> +	"setenv bootargs root=/dev/$bdev rw "		\
> +	"console=$consoledev,$baudrate $othbootargs $videobootargs;"	\
> +	"tftp $loadaddr $bootfile;"			\
> +	"tftp $fdtaddr $fdtfile;"			\
> +	"bootm $loadaddr - $fdtaddr"

Should this definition (and the follwing ones) not rather be NUL
terminated?

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 think there's a world market for about five computers.
         -- attr. Thomas J. Watson (Chairman of the Board, IBM), 1943