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

[Prabhakar Kushwaha <prabhakar@freescale.com>]

Hi Wolfgang,

Thanks for reviewing it.
Please find my response in-lined.


On Tuesday 06 March 2012 08:41 PM, Wolfgang Denk wrote:
> Dear Prabhakar Kushwaha,
>
> In message<1329304781-27758-1-git-send-email-prabhakar@freescale.com>  you wrote:
> ...
>> +#define CONFIG_L2_CACHE			/* toggle L2 cache */
>> +#define CONFIG_BTB			/* toggle branch predition */
> Toggle?  Do you really toggle, or enable?

We enable the branch prediction. Comments is wrong.
I will change it.

>> +#define CONFIG_ADDR_STREAMING		/* toggle addr streaming */
> What is this good for?
>
> It is undocumented, and there is no code that uses it.

This #define is no more in used. I just copy pasted from P1010RDB.h and 
P1_P2_RDB.h.
i will remove it.

>> +#define CONFIG_SYS_MEMTEST_START	0x00000000	/* memtest works on */
>> +#define CONFIG_SYS_MEMTEST_END		0x1fffffff
> Have these been tested?  A memtest starting at 0x0000 is likely to
> overwrite the exception vectors - I would expect the system will crash
> or hang immediately?

You are right. exception vector table is located here.
I will add correct value.



>> +#define CONFIG_PANIC_HANG		/* do not reset board on panic */
> Why is this needed?
>
> ...

It is defined in README at line 2093.
But as part of powerpc no code is written for this.

i will remove it.

>> +#define CONFIG_HOSTNAME		PSC9131rdb
>> +#define CONFIG_ROOTPATH		"/opt/nfsroot"
>> +#define CONFIG_BOOTFILE		"uImage"
>> +#define CONFIG_UBOOTPATH	u-boot.bin /* U-Boot image on TFTP server */
>> +
>> +#define CONFIG_BAUDRATE		115200
>> +
>> +#define	CONFIG_EXTRA_ENV_SETTINGS				\
>> +	"netdev=eth0\0"						\
>> +	"uboot=" MK_STR(CONFIG_UBOOTPATH) "\0"				\
> Why didn't you follow my change request here?  Define as "u-boot.bin"
> so it is at least consistent with the other strings, and drop the
> MK_STR here.
>
> ...

The requirement is to use u-boot.bin instead of CONFIG_UBOOTPATH?
is my understanding correct?



>> --- /dev/null
>> +++ b/nand_spl/board/freescale/psc9131rdb/Makefile
> ...
>> --- /dev/null
>> +++ b/nand_spl/board/freescale/psc9131rdb/nand_boot.c
>
> Sorry, but this is the old, legacy SPL code.  Please do not use this
> any more for new board.
>
> Use the new code (in /spl/ directory) instead.

I will check this point and re-spin the patch
>> +#define udelay(x) \
>> +	{int i, j; for (i = 0; i<  x; i++) for (j = 0; j<  10000; j++); }
> Have you ever tested this?  Or looked at what the compiler generates
> for it?

I will check this point,,

>
>> +unsigned long ddr_freq_mhz;
> Do you have BSS before relocation?
>
>
I will check this point

--Prabhakar