[U-Boot] [PATCH] powerpc/85xx:Add PSC9131 RDB Support

[Wolfgang Denk]

Dear Prabhakar Kushwaha,

In message <1329200040-23039-1-git-send-email-prabhakar at freescale.com> you wrote:
>  PSC9131RDB is a Freescale reference design board for PSC9131 SoC. PSC9131 SOC
>  is an integrated device that targets Femto base station market. It combines
>  Power Architecture e500v2 and DSP StarCore SC3850 core technologies with
>  MAPLE-B2F baseband acceleration processing elements
> 
>   PSC9131RDB Overview
>    -----------------
>      -1Gbyte DDR3 (on board DDR)
>      -128Mbyte 2K page size NAND Flash
>      -256 Kbit M24256 I2C EEPROM
>      -128 Mbit SPI Flash memory
>      -USB-ULPI
>      -eTSEC1: Connected to RGMII PHY
>      -eTSEC2: Connected to RGMII PHY
>      -DUART interface: supports one UARTs up to 115200 bps for console display
> Apart from the above it also consists various peripherals to support DSP
> functionalities.
> 
> This patch adds support for mainly Power side functionalities and peripherals
> 
> Signed-off-by: Ramneek Mehresh <ramneek.mehresh at freescale.com>
> Signed-off-by: Priyanka Jain <Priyanka.Jain at freescale.com>
> Signed-off-by: Akhil Goyal <Akhil.Goyal at freescale.com>
> Signed-off-by: Rajan Srivastava <rajan.srivastava at freescale.com>
> Signed-off-by: Poonam Aggrwal <poonam.aggrwal at freescale.com>
> Signed-off-by: Prabhakar Kushwaha <prabhakar at freescale.com>
> ---
>  Applied on git://git.denx.de/u-boot.git (branch master)
> 
>  board/freescale/psc9131rdb/Makefile             |   54 +++
>  board/freescale/psc9131rdb/ddr.c                |  186 +++++++++
>  board/freescale/psc9131rdb/law.c                |   31 ++
>  board/freescale/psc9131rdb/psc9131rdb.c         |  116 ++++++
>  board/freescale/psc9131rdb/tlb.c                |   67 ++++
>  boards.cfg                                      |    2 +
>  doc/README.psc9131rdb                           |  137 +++++++
>  include/configs/PSC9131RDB.h                    |  484 +++++++++++++++++++++++
>  nand_spl/board/freescale/psc9131rdb/Makefile    |  145 +++++++
>  nand_spl/board/freescale/psc9131rdb/nand_boot.c |  122 ++++++
>  10 files changed, 1344 insertions(+), 0 deletions(-)
>  create mode 100644 board/freescale/psc9131rdb/Makefile
>  create mode 100644 board/freescale/psc9131rdb/ddr.c
>  create mode 100644 board/freescale/psc9131rdb/law.c
>  create mode 100644 board/freescale/psc9131rdb/psc9131rdb.c
>  create mode 100644 board/freescale/psc9131rdb/tlb.c
>  create mode 100644 doc/README.psc9131rdb
>  create mode 100644 include/configs/PSC9131RDB.h
>  create mode 100644 nand_spl/board/freescale/psc9131rdb/Makefile
>  create mode 100644 nand_spl/board/freescale/psc9131rdb/nand_boot.c

Entry to MAINTAINERS missing.

> new file mode 100644
> index 0000000..0d01b8d
> --- /dev/null
> +++ b/board/freescale/psc9131rdb/Makefile
...
> +#########################################################################
> +

Please don't add trailing empty lines.


...
> +unsigned long get_sdram_size(void)
> +{
> +	return CONFIG_SYS_DRAM_SIZE;
> +}

Please use get_ram_size() instead of a hardcoded setting.


> +	if (fixed_ddr_parm_0[i].max_freq == 0)
> +		panic("Unsupported DDR data rate %s MT/s data rate\n",
> +					strmhz(buf, ddr_freq));

Braces needed for multi-line statement.

> +int checkboard(void)
> +{
> +	struct cpu_type *cpu;
> +
> +	cpu = gd->cpu;
> +	printf("Board: %sRDB\n", cpu->name);

What exactly gets printed here?

> +int board_eth_init(bd_t *bis)
> +{
...
> +	fsl_pq_mdio_init(bis, &mdio_info);
> +	tsec_eth_init(bis, tsec_info, num);
> +
> +	return 0;

return num; ??

> +/*
> + * Memory map
> + *
> + * 0x0000_0000	0x3FFF_FFFF	DDR			1G cacheable
> + * 0x8800_0000	0x8810_0000	NAND memory		1M

Can you please explain to me what "NAND memory" is?  I must admit that
I never heard of that - I always thought NAND was only available as
storage devices with a command based controller interface?

...
> +/* NAND boot: 8K NAND loader config */
> +#define CONFIG_SYS_NAND_SPL_SIZE	0x2000
> +#define CONFIG_SYS_NAND_U_BOOT_SIZE	(512 << 10)
> +#define CONFIG_SYS_NAND_U_BOOT_DST	(0x11000000 - CONFIG_SYS_NAND_SPL_SIZE)
> +#define CONFIG_SYS_NAND_U_BOOT_START	0x11000000
> +#define CONFIG_SYS_NAND_U_BOOT_OFFS	(0)

Please don't use parens around simple values.

> +/* Seconed UART port is connected to GPS */

Typo.

> +/* I2C EEPROM */
> +#undef CONFIG_ID_EEPROM

Please do not undef what is not defined anyway.

> +#ifndef CONFIG_NET_MULTI
> +#define CONFIG_NET_MULTI
> +#endif

Obsolete - please remove.

> +/*
> + * Command line configuration.
> + */
> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_ERRATA
> +#define CONFIG_CMD_ELF
> +#define CONFIG_CMD_IRQ
> +#define CONFIG_CMD_MII
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_SETEXPR
> +#define CONFIG_CMD_REGINFO
> +#define CONFIG_CMD_DHCP
> +#undef CONFIG_WATCHDOG			/* watchdog disabled */
> +#define CONFIG_CMD_EXT2
> +#define CONFIG_CMD_FAT
> +#define CONFIG_DOS_PARTITION

How about sorting these?  And again: do not undef what is not defined.

> + * Boot Flags
> + */
> +#define BOOTFLAG_COLD	0x01		/* Normal Power-On: Boot from FLASH */
> +#define BOOTFLAG_WARM	0x02		/* Software reboot */

Bogus, please remove.

> +#if defined(CONFIG_TSEC_ENET)
> +#define CONFIG_HAS_ETH0
> +#endif
> +
> +

Only one blank line in places like this.

> +#define CONFIG_HOSTNAME		9131rdb
> +#define CONFIG_ROOTPATH		"/opt/nfsroot"
> +#define CONFIG_BOOTFILE		"uImage"
> +#define CONFIG_UBOOTPATH	u-boot.bin /* U-Boot image on TFTP server */

CONFIG_ options are kind of special; if you define new ones (which
should only be done when it really makes sense - this appears not to
be the case here) you must also add documentation for these to the
README.

Also, please stick with a common format - these are all strings, so
they all should be quoted (this saves you a few calls to MK_STR()
further down below).

Finally, while strictly legal according to rfc1123, it is pretty
unusal to have a host name start with digits - please follow the
common style used by other Freescale boards.

> +/* default location for tftp and bootm */
> +#define CONFIG_LOADADDR		1000000
> +
> +#define CONFIG_BOOTDELAY	10	/* -1 disables auto-boot */
> +#define  CONFIG_BOOTARGS		/* the boot command will set bootargs */

Drop this bogus #define.


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 find this a nice feature but it is not according to  the  documen-
tation. Or is it a BUG?"   "Let's call it an accidental feature. :-)"
                       - Larry Wall in <6909 at jpl-devvax.JPL.NASA.GOV>