[U-Boot] [PATCH 3/3] XPedite5370 board support

[Peter Tyser]

On Tue, 2008-11-18 at 22:44 +0100, Wolfgang Denk wrote:
> Dear Peter Tyser,
> 
> In message <1224800639-31350-4-git-send-email-ptyser at 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