From mboxrd@z Thu Jan 1 00:00:00 1970 From: Prabhakar Kushwaha Date: Wed, 7 Mar 2012 11:43:01 +0530 Subject: [U-Boot] [PATCH 2/2][v2] powerpc/85xx:Add PSC9131 RDB Support In-Reply-To: <20120306151120.3EAB4202D7D@gemini.denx.de> References: <1329304781-27758-1-git-send-email-prabhakar@freescale.com> <20120306151120.3EAB4202D7D@gemini.denx.de> Message-ID: <4F56FC6D.8080401@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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