[U-Boot] [PATCH] ppc4xx: 460SX Eegier board support

[Stefan Roese]

Hi Marri,

On Thursday 09 December 2010 02:12:07 tmarri at apm.com wrote:
> From: Tirumala Marri <tmarri at apm.com>
> 
> Adding Eiger board support for 460SX SoC.

Thanks. Some mostly nitpicking comments below.

First typo in the subject: s/Eeigier/Eiger.

<snip>
 
> b/include/configs/eiger.h
> new file mode 100644
> index 0000000..bc082a6
> --- /dev/null
> +++ b/include/configs/eiger.h
> @@ -0,0 +1,192 @@
> +/*
> + * eiger.h - configuration for Eiger(460SX) Board.
> + *
> + * Copyright (c) 2010, Applied Micro Circuits Corporation
> + * Author: Tirumala R Marri <tmarri at apm.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +/*
> + * High Level Configuration Options
> + */
> +#define CONFIG_4xx			1	/* ... PPC4xx family	*/
> +#define CONFIG_440			1	/* ... PPC460 family	*/
> +#define CONFIG_460SX			1	/* ... PPC460 family	*/
> +#define CONFIG_BOARD_EARLY_INIT_F	1	/* Call board_pre_init	*/
> +
> +#define	CONFIG_SYS_TEXT_BASE	0xfffb0000

Please use space instead of tab after "#define".

> +
> +/*
> + * Include common defines/options for all AMCC boards
> + */
> +#define CONFIG_HOSTNAME		eiger
> +
> +#include "amcc-common.h"
> +
> +#define CONFIG_SYS_CLK_FREQ	33333333	/* external freq to pll	*/
> +
> +/*
> + * Base addresses -- Note these are effective addresses where the
> + * actual resources get mapped (not physical addresses)
> + */
> +#define	CONFIG_SYS_BOOT_BASE_ADDR       0xFF000000
> +#define CONFIG_SYS_FLASH_BASE		0xfff00000	/* start of 
FLASH	*/

Again, space after "#define". And use lower- or upper-case hex values 
consistently in this file. I personally prefer lower-case.

> +#define CONFIG_SYS_ISRAM_BASE		0x90000000	/* internal 
SRAM	*/
> +
> +#define CONFIG_SYS_PCI_BASE		0xd0000000	/* internal PCI regs	
*/
> +
> +#define CONFIG_SYS_PCIE_MEMBASE		0x90000000	/* mapped PCIe 
memory	*/
> +#define CONFIG_SYS_PCIE0_MEMBASE	0x90000000	/* mapped PCIe memory	
*/
> +#define CONFIG_SYS_PCIE1_MEMBASE	0xa0000000	/* mapped PCIe memory	
*/
> +#define CONFIG_SYS_PCIE_MEMSIZE		0x01000000
> +
> +#define CONFIG_SYS_PCIE0_XCFGBASE	0xb0000000
> +#define CONFIG_SYS_PCIE1_XCFGBASE	0xb2000000
> +#define CONFIG_SYS_PCIE2_XCFGBASE	0xb4000000
> +#define CONFIG_SYS_PCIE0_CFGBASE	0xb6000000
> +#define CONFIG_SYS_PCIE1_CFGBASE	0xb8000000
> +#define CONFIG_SYS_PCIE2_CFGBASE	0xba000000
> +
> +/* PCIe mapped UTL registers */
> +#define CONFIG_SYS_PCIE0_REGBASE   0xd0000000
> +#define CONFIG_SYS_PCIE1_REGBASE   0xd0010000
> +#define CONFIG_SYS_PCIE2_REGBASE   0xd0020000
> +
> +/* System RAM mapped to PCI space */
> +#define CONFIG_PCI_SYS_MEM_BUS	CONFIG_SYS_SDRAM_BASE
> +#define CONFIG_PCI_SYS_MEM_PHYS	CONFIG_SYS_SDRAM_BASE
> +#define CONFIG_PCI_SYS_MEM_SIZE	(1024 * 1024 * 1024)
> +
> +#define CONFIG_SYS_OPER_FLASH		0xe7000000	/* SRAM - OPER 
Flash	*/

What is this "OPER Flash"? Is it used at all in this code? If not you should 
better remove it.

> +/*
> + * Serial Port
> + */
> +#define CONFIG_CONS_INDEX	1	/* Use UART0			*/
> +
> +/*
> + * Initial RAM & stack pointer (placed in internal SRAM)
> + */
> +#define CONFIG_SYS_TEMP_STACK_OCM	1
> +#define CONFIG_SYS_OCM_DATA_ADDR	CONFIG_SYS_ISRAM_BASE
> +#define CONFIG_SYS_INIT_RAM_ADDR	CONFIG_SYS_ISRAM_BASE	/* Initial RAM
> address	*/ +#define CONFIG_SYS_INIT_RAM_SIZE	0x2000		/* 
Size of used area
> in RAM */ +
> +#define CONFIG_SYS_GBL_DATA_OFFSET	(CONFIG_SYS_INIT_RAM_SIZE -
> GENERATED_GBL_DATA_SIZE) +#define
> CONFIG_SYS_INIT_SP_OFFSET	(CONFIG_SYS_GBL_DATA_OFFSET - 0x4) +
> +/*
> + * DDR SDRAM
> + */
> +#define CONFIG_SPD_EEPROM	1	/* Use SPD EEPROM for setup	*/
> +#define CONFIG_DDR_ECC		1	/* with ECC support		
*/
> +
> +#define CONFIG_SYS_SPD_MAX_DIMMS	2
> +
> +/* SPD i2c spd addresses */
> +#define SPD_EEPROM_ADDRESS     {IIC0_DIMM0_ADDR, IIC0_DIMM1_ADDR}

Indentation with tabs please. And please add space after "{". Otherwise 
checkpatch will complain:

#define SPD_EEPROM_ADDRESS	{ IIC0_DIMM0_ADDR, IIC0_DIMM1_ADDR }

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de