[U-Boot] [PATCH v2] mpc83xx: Add esd VME8349 board support

[Wolfgang Denk]

Dear Stefan Roese,

In message <1248440296-17129-1-git-send-email-sr at denx.de> you wrote:
> From: Reinhard Arlt <reinhard.arlt at esd-electronics.com>
> 
> This patch adds support for the esd VME8349 board equipped with the
> MPC8349. It's a VME PMC carrier board equipped with the Tundra
> TSI148 VME-bridge.
> 
> Signed-off-by: Reinhard Arlt <reinhard.arlt at esd-electronics.com>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Cc: Kim Phillips <kim.phillips at freescale.com>
...
> +int do_caddy(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
...
> +			switch (caddy_cmd->cmd) {
> +			case CADDY_CMD_IO_READ_8:
> +				result[0] = in_8((void *)CONFIG_SYS_PCI1_IO_PHYS +
> +						 (caddy_cmd->addr & 0x001fffff));
> +				generate_answer(caddy_cmd, status, &result[0]);
> +				break;
> +			case CADDY_CMD_IO_READ_16:
> +				result[0] = in_be16((void *)CONFIG_SYS_PCI1_IO_PHYS +
> +						    (caddy_cmd->addr & 0x001fffff));
> +				generate_answer(caddy_cmd, status, &result[0]);

Can we get rid of these base address plus offset constructs? I  would
like  to get rid of the casts which nullify one of the major benefits
of using I/O acessors, i. e. having type checking done  by  the  com-
piler.

> +			case CADDY_CMD_IO_READ_32:
> +				result[0] = in_be32((void *)CONFIG_SYS_PCI1_IO_PHYS +
> +						    (caddy_cmd->addr & 0x001fffff));
> +				generate_answer(caddy_cmd, status, &result[0]);
> +				break;

Hm... and the code for the CADDY_CMD_IO_READ_*, CADDY_CMD_IO_WRITE_*
etc. cases looks always the same, execpt for the access size.
Maybe you want to use some macro? Thsi would make the code much easier
to read and to maintain.

...
> diff --git a/board/esd/vme8349/vme8349.c b/board/esd/vme8349/vme8349.c
> new file mode 100644
> index 0000000..db24a7c
> --- /dev/null
> +++ b/board/esd/vme8349/vme8349.c
> @@ -0,0 +1,139 @@
...
> +	im->ddr.sdram_cfg   = 0x63000000;
> +	im->ddr.sdram_cfg2  = 0x04061000;
> +	im->ddr.sdram_mode  = 0x07940242;
> +	im->ddr.sdram_mode2 = 0x00000000;

Maybe you want to use some nice readable constants here instead of
hard-coded magic numbers?

> diff --git a/include/configs/vme8349.h b/include/configs/vme8349.h
> new file mode 100644
> index 0000000..cde98ae
> --- /dev/null
> +++ b/include/configs/vme8349.h
...
> +#define CONFIG_SYS_DDR_TIMING_0	        0x00220802
> +#define CONFIG_SYS_DDR_TIMING_1	        0x39377322
> +#define CONFIG_SYS_DDR_TIMING_2	        0x2f9848ca	/* P9-45,may need tuning */
> +#define CONFIG_SYS_DDR_TIMING_3	        0x00000000
> +#define CONFIG_SYS_DDR_CONTROL		0xc2000000	/* unbuffered,no DYN_PWR */
> +#define CONFIG_SYS_DDR_INTERVAL	        0x04060100	/* autocharge,no open page */
> +
> +/* the default burst length is 4 - for 64-bit data path */
> +#define CONFIG_SYS_DDR_MODE		0x00000022	/* DLL,normal,seq,4/2.5, 4 burst len */

Line too long. Please check globally.

> +#if 1 /*528/264*/
> +#define CONFIG_SYS_HRCW_LOW (\
> +	HRCWL_LCL_BUS_TO_SCB_CLK_1X1 |\
> +	HRCWL_DDR_TO_SCB_CLK_1X1 |\
> +	HRCWL_CSB_TO_CLKIN |\
> +	HRCWL_VCO_1X2 |\
> +	HRCWL_CORE_TO_CSB_2X1)
> +#elif 0 /*396/132*/
> +#define CONFIG_SYS_HRCW_LOW (\
> +	HRCWL_LCL_BUS_TO_SCB_CLK_1X1 |\
> +	HRCWL_DDR_TO_SCB_CLK_1X1 |\
> +	HRCWL_CSB_TO_CLKIN |\
> +	HRCWL_VCO_1X4 |\
> +	HRCWL_CORE_TO_CSB_3X1)
> +#elif 0 /*264/132*/
> +#define CONFIG_SYS_HRCW_LOW (\
> +	HRCWL_LCL_BUS_TO_SCB_CLK_1X1 |\
> +	HRCWL_DDR_TO_SCB_CLK_1X1 |\
> +	HRCWL_CSB_TO_CLKIN |\
> +	HRCWL_VCO_1X4 |\
> +	HRCWL_CORE_TO_CSB_2X1)
> +#elif 0 /*132/132*/
> +#define CONFIG_SYS_HRCW_LOW (\
> +	HRCWL_LCL_BUS_TO_SCB_CLK_1X1 |\
> +	HRCWL_DDR_TO_SCB_CLK_1X1 |\
> +	HRCWL_CSB_TO_CLKIN |\
> +	HRCWL_VCO_1X4 |\
> +	HRCWL_CORE_TO_CSB_1X1)
> +#elif 0 /*264/264 */
> +#define CONFIG_SYS_HRCW_LOW (\
> +	HRCWL_LCL_BUS_TO_SCB_CLK_1X1 |\
> +	HRCWL_DDR_TO_SCB_CLK_1X1 |\
> +	HRCWL_CSB_TO_CLKIN |\
> +	HRCWL_VCO_1X4 |\
> +	HRCWL_CORE_TO_CSB_1X1)
> +#endif

There's a lot of dead code here. Please remove (or move to
documentation).

> +#define CONFIG_SERVERIP		192.168.1.1
> +#define CONFIG_GATEWAYIP	192.168.1.1
> +#define CONFIG_NETMASK		255.255.255.0

Please do not add network configuration data into the  default  board
config file.


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
What is tolerance? -- it is the consequence of humanity. We  are  all
formed  of frailty and error; let us pardon reciprocally each other's
folly -- that is the first law of nature.                  - Voltaire