On Tue, 2008-11-18 at 22:44 +0100, Wolfgang Denk wrote:
Dear Peter Tyser,
In message 1224800639-31350-4-git-send-email-ptyser@xes-inc.com you wrote:
Initial support for Extreme Engineering Solutions XPedite5370 - a MPC8572-based 3U VPX single board computer with a PMC/XMC site.
...
diff --git a/board/xes/common/fsl_85xx_ddr.c b/board/xes/common/fsl_85xx_ddr.c new file mode 100644 index 0000000..30b4767 --- /dev/null +++ b/board/xes/common/fsl_85xx_ddr.c
...
- puts(" (");
+#if (CONFIG_NUM_DDR_CONTROLLERS > 1)
- /* Print interleaving information */
- if (ddr1->cs0_config & 0x20000000) {
switch ((ddr1->cs0_config >> 24) & 0xf) {case 0:puts("cache line");break;case 1:puts("page");break;case 2:puts("bank");break;case 3:puts("super-bank");break;default:puts("invalid");break;}- } else {
puts("no");- }
- puts(" interleaving");
+#endif
+#if (CONFIG_NUM_DDR_CONTROLLERS > 1) && defined(CONFIG_DDR_ECC)
- puts(", ");
+#endif
+#if defined(CONFIG_DDR_ECC)
- puts("ECC enabled");
+#endif
- puts(")");
Please don't be too noisy. Turn this into debug() calls.
This information is very useful to a customer and doesn't add much as far as output. No newlines at least. The output also prints information which is configurable. I think giving the user feedback about how they have the board configured is useful.
We have the standard Freescale DDR printf's turned into debug as that is much, much more verbose than the output above.
diff --git a/board/xes/common/fsl_85xx_pci.c b/board/xes/common/fsl_85xx_pci.c new file mode 100644 index 0000000..12437be --- /dev/null +++ b/board/xes/common/fsl_85xx_pci.c
...
- if (width && !(devdisr & MPC85xx_DEVDISR_PCIE)) {
printf("\n PCIE1 connected as %s (x%d)",host ? "Root Complex" : "End Point", width);Alignment not by TAB.
Ok.
if (pci->pme_msg_det) {pci->pme_msg_det = 0xffffffff;debug(" with errors. Clearing. Now 0x%08x",pci->pme_msg_det);Alignment not by TAB.
}printf("\n");/* inbound */pci_set_region(hose->regions + 0,CONFIG_SYS_PCI_MEMORY_BUS,CONFIG_SYS_PCI_MEMORY_PHYS,CONFIG_SYS_PCI_MEMORY_SIZE,PCI_REGION_MEM | PCI_REGION_MEMORY);Alignment not by TAB.
And so on.
diff --git a/board/xes/xpedite5370/xpedite5370.c b/board/xes/xpedite5370/xpedite5370.c new file mode 100644 index 0000000..3526172 --- /dev/null +++ b/board/xes/xpedite5370/xpedite5370.c
...
- /* Print boot dev and swap flash flash chip selects if booted from 2nd
* flash. Swapping chip selects presents user with a common memory* map regardless of which flash was booted from. */Multiline comment style.
Will fix.
- flash_sel = !((pca953x_get_val(CONFIG_SYS_I2C_PCA953X_ADDR0) &
CONFIG_SYS_PCA953X_C0_FLASH_PASS_CS));- printf("FLASH: Executed from FLASH%d\n", flash_sel ? 2 : 1);
Please be less noisy! s/printf/debug/
This information is also very useful to a customer.
+#define CONFIG_EXTRA_ENV_SETTINGS \
- "autoload=yes\0" \
- "download_cmd=tftp\0" \
- "console_args=console=ttyS0,115200\0" \
- "root_args=root=/dev/nfs rw\0" \
- "misc_args=ip=on\0" \
- "set_bootargs=setenv bootargs ${console_args} ${root_args} ${misc_args}\0" \
- "bootfile=/home/user/file\0" \
- "osfile=/home/user/uImage-XPedite5370\0" \
- "fdtfile=/home/user/xpedite5370.dtb\0" \
- "ubootfile=/home/user/u-boot.bin\0" \
- "fdtaddr=c00000\0" \
- "osaddr=0x1000000\0" \
- "loadaddr=0x1000000\0" \
- "prog_uboot1="CONFIG_PROG_UBOOT1"\0" \
- "prog_uboot2="CONFIG_PROG_UBOOT2"\0" \
- "prog_os1="CONFIG_PROG_OS1"\0" \
- "prog_os2="CONFIG_PROG_OS2"\0" \
- "prog_fdt1="CONFIG_PROG_FDT1"\0" \
- "prog_fdt2="CONFIG_PROG_FDT2"\0" \
- "bootcmd_net=run set_bootargs; "CONFIG_BOOT_OS_NET"\0" \
- "bootcmd_flash1=run set_bootargs; bootm "CONFIG_OS1_FLASH_ADDR_STR" - "CONFIG_FDT1_FLASH_ADDR_STR"\0" \
- "bootcmd_flash2=run set_bootargs; bootm "CONFIG_OS2_FLASH_ADDR_STR" - "CONFIG_FDT2_FLASH_ADDR_STR"\0" \
- "bootcmd=run bootcmd_flash1\0"
+#endif /* __CONFIG_H */
Alignment not by TAB, lines way too long.
What are you referring to as far as "Alignment not by TAB"?
Everyone's config file breaks the 80 column rule, is this really necessary to change? I think splitting it up makes an already confusing define even more confusing.
Thanks, Peter