[U-Boot] [PATCH] powerpc/85xx:Add PSC9131 RDB Support
Prabhakar Kushwaha
prabhakar at freescale.com
Wed Feb 15 11:40:55 CET 2012
Thanks for reviewing this patch.
Please find my response in-lined
On Tuesday 14 February 2012 07:48 PM, Wolfgang Denk wrote:
> 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.
I will add in next patch version
>> new file mode 100644
>> index 0000000..0d01b8d
>> --- /dev/null
>> +++ b/board/freescale/psc9131rdb/Makefile
> ...
>> +#########################################################################
>> +
> Please don't add trailing empty lines.
Sure i will take care of it
> ...
>> +unsigned long get_sdram_size(void)
>> +{
>> + return CONFIG_SYS_DRAM_SIZE;
>> +}
> Please use get_ram_size() instead of a hardcoded setting.
I will take care in next patch version
>
>> + 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.
I will take care of the braces.
>> +int checkboard(void)
>> +{
>> + struct cpu_type *cpu;
>> +
>> + cpu = gd->cpu;
>> + printf("Board: %sRDB\n", cpu->name);
> What exactly gets printed here?
It prints<SoC>RDB i.e. "PSC9131RDB"
>> +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; ??
I will take care
>> +/*
>> + * 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?
>
> ...
This of course not the NAND memory.
Actually in our NAND controller we have internal SRAM. Data read from NAND is placed here(SRAM).
Memory region defined here is for this internal SRAM of controller.
My mistake, I should have wrote controller's internal SRAM memory instead of NAND memory.
>> +/* 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.
I will take care of it.
>> +/* Seconed UART port is connected to GPS */
> Typo.
My Mistake.
>> +/* I2C EEPROM */
>> +#undef CONFIG_ID_EEPROM
> Please do not undef what is not defined anyway.
I will remove
>> +#ifndef CONFIG_NET_MULTI
>> +#define CONFIG_NET_MULTI
>> +#endif
> Obsolete - please remove.
Sure . i will do
>> +/*
>> + * 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.
I will do the same.
>
>> + * Boot Flags
>> + */
>> +#define BOOTFLAG_COLD 0x01 /* Normal Power-On: Boot from FLASH */
>> +#define BOOTFLAG_WARM 0x02 /* Software reboot */
> Bogus, please remove.
I will remove
>> +#if defined(CONFIG_TSEC_ENET)
>> +#define CONFIG_HAS_ETH0
>> +#endif
>> +
>> +
> Only one blank line in places like this.
I will take care of it
>> +#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.
I am not clear about this. These are already defined and used by other platforms.
> 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.
I will take care of this for CONFIG_HOSTNAME
>> +/* 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.
I will remove it
Regards,
Prabhakar
More information about the U-Boot
mailing list