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

[Peter Tyser]

On Tue, 2008-11-18 at 23:20 +0100, Wolfgang Denk wrote:
> Dear Peter Tyser,
> 
> In message <1227046406.3065.49.camel at localhost.localdomain> you wrote:
> >
> > 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.
> 
> You really need this level of detail only for board bring up and
> debugging. After that, this is a constand and never changes, right? Si
> it is normally enough to print the RAM size (and eventually some speed
> characteristics).

User's can change the DDR configuration with the "memctl_intlv_ctl" and
"ba_intlv_ctl".  I don't imagine they'll fuss with those variable much,
but they should certainly be aware if they do, or change one by
accident, etc.  Different board configs do/don't have ECC (same product,
different build options), so I think the ECC info is also useful to a
customer as they may have multiple boards with different configs.

> > We have the standard Freescale DDR printf's turned into debug as that is
> > much, much more verbose than the output above.
> 
> Indeed :-(
> 
> > > > +	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.
> 
> Is it, really? well...
> 
> > > > +#define	CONFIG_EXTRA_ENV_SETTINGS				\
> > > > + "autoload=yes\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"?
> 
> You indent the lines by a single space, but they should be indented by
> a TAB.

Many boards I look at don't use TABS.  In particular Freescale reference
platforms.  Many boards even have different amounts of spaces:)  I don't
see the value of indentation in this case as it doesn't increase
readability and just makes already long lines even longer.

> > 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.
> 
> At least the ';' is a good place to split. And you might check if  it
> really  makes  sense  to  use  variable names with 25 characters like
> "CONFIG_OS2_FLASH_ADDR_STR" (no, it doesn't).

That define is no longer than the standard CONFIG_EXTRA_ENV_SETTINGS
define that its used in, or any number of other standard defines.  And
they aren't used anywhere other than this 40 line section of code in 1
file.  I thought the increase in clarity of long defines made up for
their ugliness.  I'll go ahead and split the lines on the ';' as you
suggested.

You have the final say, so if any of the above are sticking points to
getting the code accepted let me know and I'll change them as requested.

Thanks,
Peter