[U-Boot] [PATCH V2] board support patch for phyCORE-MPC5200B-tiny

[Wolfgang Denk]

Dear Jon Smirl,

In message <20090530194126.12535.19728.stgit at terra> you wrote:
> Add support for the Phytec phyCORE-MPC5200B-tiny. Code originally from Pengutronix.de.
> Added MAKEALL and MAINTAINER entry per last posting.
...
> +#ifndef CONFIG_SYS_RAMBOOT
> +static void sdram_start(int hi_addr)
> +{
> +	long hi_addr_bit = hi_addr ? 0x01000000 : 0;
> +
> +	/* unlock mode register */
> +	out_be32 ((unsigned __iomem *)MPC5XXX_SDRAM_CTRL,
> +		(SDRAM_CONTROL | 0x80000000 | hi_addr_bit));
> +
> +	/* precharge all banks */
> +	out_be32 ((unsigned __iomem *)MPC5XXX_SDRAM_CTRL,
> +		(SDRAM_CONTROL | 0x80000002 | hi_addr_bit));

Please use a proper C struct to describe the memory controller's
register layout and get rid of all these casts.

> +#if SDRAM_DDR

Better use "#ifdef" here.

...
> +void init_ide_reset(void)
> +{
> +	debug("init_ide_reset\n");
> +
> +	/* Configure PSC2_4 as GPIO output for ATA reset */
> +	setbits_be32((unsigned __iomem *)MPC5XXX_WU_GPIO_ENABLE, GPIO_PSC2_4);
> +	setbits_be32((unsigned __iomem *)MPC5XXX_WU_GPIO_DIR, GPIO_PSC2_4);
> +	/* Deassert reset */
> +	setbits_be32((unsigned __iomem *)MPC5XXX_WU_GPIO_DATA_O, GPIO_PSC2_4);
> +}

Ditto here, and in thew rest of the patch.


> +void ide_set_reset(int idereset)
> +{
> +	debug("ide_reset(%d)\n", idereset);
> +
> +	if (idereset) {
> +		clrbits_be32((unsigned __iomem *)
> +				MPC5XXX_WU_GPIO_DATA_O, GPIO_PSC2_4);
> +		/* Make a delay. MPC5200 spec says 25 usec min */
> +		udelay(500000);
> +	} else
> +		setbits_be32((unsigned __iomem *)
> +				MPC5XXX_WU_GPIO_DATA_O, GPIO_PSC2_4);

Please use braces for multiline statements.

...
> diff --git a/include/configs/pcm030.h b/include/configs/pcm030.h
> new file mode 100644
> index 0000000..2d10366
> --- /dev/null
> +++ b/include/configs/pcm030.h
...
> +/* To build RAMBOOT, add this to the main Makefile
> +@[ -z "$(findstring RAMBOOT_,$@)" ] || \
> +       { echo "TEXT_BASE = 0x00100000" >board/phycore_mpc5200b_tiny/\
> +		config.tmp ; \
> +         echo "... with RAMBOOT configuration" ; \
> +         echo "... remember to make sure that MBAR is already \
> +			switched to 0xF0000000 !!!" ; \
> +       }
> +*/

This does not belong into the board configuration file. Evantually
you want to provide additional documentation for this board? In any
case, please provide _clear_ instructions. Just "adding" above lines
of code "to the main Makefile" (say, at the end, since you give no
beppter instructions) will probably not work as intended.


> +#define CONFIG_BOARDINFO	 "Phytec Phycore mpc5200b tiny"

Too long. Please be terse.

> +/*-----------------------------------------------------------------------------
> +High Level Configuration Options
> +(easy to change)
> +-----------------------------------------------------------------------------*/
> +#define CONFIG_MPC5xxx		1	/* This is an MPC5xxx CPU */
> +#define CONFIG_MPC5200		1	/* (more precisely an MPC5200 CPU) */
> +#define CONFIG_MPC5200_DDR	1	/* (with DDR-SDRAM) */
> +#define CONFIG_PHYCORE_MPC5200B_TINY 1	/* phyCORE-MPC5200B -> */
> +					/* FEC configuration and IDE */
> +#define CONFIG_SYS_MPC5XXX_CLKIN 33333333 /* ... running at 33.333333MHz */

Please verify: 33.333333 MHz or 33.333000 MHz or 33.0000000 MHz?

> +#define BOOTFLAG_COLD		0x01	/* Normal Power-On: Boot from FLASH  */
> +#define BOOTFLAG_WARM		0x02	/* Software reboot           */
> +
> +/*-----------------------------------------------------------------------------
> +Serial console configuration
> +-----------------------------------------------------------------------------*/
> +#define CONFIG_PSC_CONSOLE	3	/* console is on PSC3 -> */
> +					/*define gps port conf. */
> +					/* register later on to  */
> +					/*enable UART function! */
> +#define CONFIG_BAUDRATE		115200	/* ... at 115200 bps */
> +#define CONFIG_SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400 }
> +
> +/*
> + * Command line configuration.
> + */
> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_DATE
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_EEPROM
> +#define CONFIG_CMD_I2C
> +#define CONFIG_CMD_JFFS2
> +#define CONFIG_CMD_MII
> +#define CONFIG_CMD_NFS
> +#define CONFIG_CMD_PCI
> +
> +#define	CONFIG_TIMESTAMP	1	/* Print image info with timestamp */
> +
> +#if (TEXT_BASE == 0xFF000000)	/* Boot low */
> +#define CONFIG_SYS_LOWBOOT 1
> +#endif
> +/* RAMBOOT will be defined automatically in memory section */
> +
> +#define CONFIG_JFFS2_CMDLINE
> +#define MTDIDS_DEFAULT 		"nor0=physmap-flash.0"
> +#define MTDPARTS_DEFAULT   	"mtdparts=physmap-flash.0:256k(ubootl)," \
> +	"1792k(kernel),13312k(jffs2),256k(uboot)ro,256k(oftree),-(space)"
> +
> +/*-----------------------------------------------------------------------------
> +Autobooting
> +-----------------------------------------------------------------------------*/
> +#define CONFIG_BOOTDELAY	3	/* autoboot after 3 seconds */
> +#define CONFIG_ZERO_BOOTDELAY_CHECK	/* allow stopping of boot process */
> +					/* even with bootdelay=0 */
> +#undef	CONFIG_BOOTARGS
> +
> +
> +#define CONFIG_PREBOOT	"echo;"	\
> +	"echo Type \"run bootcmd_net\" to load Kernel over TFTP and to "\
> +		"mount root filesystem over NFS;" \
> +	"echo"
> +
> +#define	CONFIG_EXTRA_ENV_SETTINGS					\
> +	"netdev=eth0\0"							\
> +	"mtdparts=mtdparts=physmap-flash.0:256k(ubootl),1792k(kernel),"	\
> +		"13312k(jffs2),256k(uboot)ro,256k(oftree),-(space)\0"	\

Do not repeat this. Use MTDPARTS_DEFAULT as defined above instead.

> +	"ipaddr=192.168.23.226\0"					\
> +	"netmask=255.255.255.0\0"					\
> +	"serverip=192.168.23.1\0"					\
> +	"gateway=192.168.23.1\0"					\

NAK. We don't allow this in board config files.

> +/*-------------------------------------------------------------------------
> + * PCI Mapping:
> + * 0x40000000 - 0x4fffffff - PCI Memory
> + * 0x50000000 - 0x50ffffff - PCI IO Space
> + *  -----------------------------------------------------------------------*/

Just a note: don't attempt to use these mappings in Linux; they
overlap with CONFIG_TASKSIZE.

...
> +/*---------------------------------------------------------------------------
> + Environment settings
> +---------------------------------------------------------------------------*/
> +#if 0
> +#define CONFIG_ENV_IS_IN_FLASH	1
> +#define CONFIG_ENV_ADDR		(CONFIG_SYS_FLASH_BASE + 0xfe0000)
> +#define CONFIG_ENV_SIZE		0x20000
> +#define CONFIG_ENV_SECT_SIZE	0x20000
> +#else
> +#define CONFIG_ENV_IS_IN_EEPROM	1
> +#define CONFIG_ENV_OFFSET	0x00	/* environment starts at the */
> +					/*beginning of the EEPROM */
> +#define CONFIG_ENV_SIZE		CONFIG_SYS_EEPROM_SIZE
> +#endif
> +#define CONFIG_ENV_OVERWRITE	1

Are you sure? This is pessimal choice. EEPROM is slow and unreliable.

After you decided for a solution, then please remove the (then) dead
code.

...
> +#define CONFIG_SYS_LOAD_ADDR 0x100000 /* default load address */

This used to be a reasonable choice with Linux 2.4.4; it ain't so any
more.


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
Abstainer: A weak person who yields to the temptation of denying him-
self a pleasure. A total abstainer is one who  abstains  from  every-
thing  but  abstention, and especially from inactivity in the affairs
of others.                                           - Ambrose Bierce