From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Wed, 28 Jan 2009 22:05:59 +0100
Subject: [U-Boot] [PATCH] Add support for the digsy MTC board.
In-Reply-To: <12331552753467-git-send-email-gjb@semihalf.com>
References: <12331552753467-git-send-email-gjb@semihalf.com>
Message-ID: <20090128210559.8CF2A832E416@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Grzegorz,

In message <12331552753467-git-send-email-gjb@semihalf.com> you wrote:
> This is the InterControl custom device based on the MPC5200B chip.
...

General comment: there is a lot of code which could use a few comments
so the reader has a chance to understand what exactly you are doing.

> --- /dev/null
> +++ b/board/digsymtc/digsymtc.c
...
> +static void sdram_start(int hi_addr)
> +{
> +	long hi_addr_bit = hi_addr ? 0x01000000 : 0;
> +	long control = SDRAM_CONTROL | hi_addr_bit;
> +
> +	/* unlock mode register */
> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control | 0x80000000;
> +	__asm__ volatile ("sync");
> +
> +	/* precharge all banks */
> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control | 0x80000002;
> +	__asm__ volatile ("sync");
> +
> +	/* auto refresh */
> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control | 0x80000004;
> +	__asm__ volatile ("sync");
> +
> +	/* set mode register */
> +	*(vu_long *)MPC5XXX_SDRAM_MODE = SDRAM_MODE;
> +	__asm__ volatile ("sync");
> +
> +	/* normal operation */
> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control;
> +	__asm__ volatile ("sync");

Please use proper I/O accessor functions instead of volatile pointer
accesses, here and in all other similar places as well...

You can then get rid of the "sync"s as well.

> +phys_size_t initdram(int board_type)
> +{
> +	ulong dramsize = 0;
> +	ulong dramsize2 = 0;
> +	uint svr, pvr;
> +#ifndef CONFIG_SYS_RAMBOOT
> +	ulong test1, test2;
> +
> +	/* setup SDRAM chip selects */
> +	*(vu_long *)MPC5XXX_SDRAM_CS0CFG = 0x0000001C;	/* 512MB at 0x0 */
> +	*(vu_long *)MPC5XXX_SDRAM_CS1CFG = 0x80000000;	/* disabled */

... like here and below.

> +	if (s != NULL) { puts(", "); puts(s); }

Please use proper indentation.

> +void set_ethaddr(void)

Please add "static" here.

> +int last_stage_init (void)
> +{
> +	set_ethaddr();
> +	return 0;

You should do this only as long as "ethaddr" is not already set in the
environment.

> --- /dev/null
> +++ b/board/digsymtc/eeprom.h
...
> +
> +#define	EEPROM_ADDR		CONFIG_SYS_I2C_EEPROM_ADDR
> +#define	EEPROM_LEN		1024
> +#define	EEPROM_IDENT		2408
> +#define	EEPROM_ADDR_IDENT	0x0000
> +#define	EEPROM_ADDR_LEN_SYS	0x0002
> +#define	EEPROM_ADDR_LEN_SYSCFG	0x0004
> +#define	EEPROM_OFF_ETHADDR	23

You want to add some comments what all these magic defines mean...

> diff --git a/cpu/mpc5xxx/ide.c b/cpu/mpc5xxx/ide.c
> index df5b4ac..07d33e3 100644
> --- a/cpu/mpc5xxx/ide.c
> +++ b/cpu/mpc5xxx/ide.c
> @@ -12,7 +12,7 @@
>   *
>   * 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
> + * 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

Please don't arbitrarily reformat the code.

> --- /dev/null
> +++ b/include/configs/digsymtc.h
...
> +#define CONFIG_PSC_CONSOLE	4	/* console is on PSC4  */
...
> +	"console=ttyPSC1\0"				\

This looks fishy to me - is it PSC1 or PSC4 ?


> +#define OF_CPU			"PowerPC,5200 at 0"
> +#define OF_SOC			"soc5200 at f0000000"
> +#define OF_TBCLK		(bd->bi_busfreq / 4)
> +#define OF_STDOUT_PATH		"/soc5200 at f0000000/serial at 2600"

Do we really need this?



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
All he had was nothing, but that was something, and now it  had  been
taken away.                             - Terry Pratchett, _Sourcery_