[U-Boot] [PATCH 08/29] MPC512x: add support for ARIA board

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4A0691FE.6050707 at denx.de> you wrote:
> 
> > +	/*
> > +	 * Configure Flash Speed
> > +	 */
> > +	out_be32(&im->lpc.cs_cfg[0], CONFIG_SYS_CS0_CFG);
> > +
> > +	spridr = in_be32(&im->sysconf.spridr);
> > +
> 
> no blank line necessary.

Not necessary, but I find it easier to read.

> > +	u32 msize = 0;
> > +
> > +	msize = fixed_sdram ();
> > +
> 
> no blank line necessary.
> 
> > +	return msize;

Same here.

...
> > +	out_be32(&im->mddrc.lut_table1_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT1_AL);
> > +	out_be32(&im->mddrc.lut_table2_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT2_AU);
> > +	out_be32(&im->mddrc.lut_table2_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT2_AL);
> > +	out_be32(&im->mddrc.lut_table3_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT3_AU);
> > +	out_be32(&im->mddrc.lut_table3_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT3_AL);
> > +	out_be32(&im->mddrc.lut_table4_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT4_AU);
> > +	out_be32(&im->mddrc.lut_table4_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT4_AL);
> 
> too long lines ... but 2 lines would also look ugly ...

I tried splitting it in too lines, but it looks *really* ugly. As it's
just 85 columns, I decided to leave it that way.


> > +#ifdef CONFIG_FSL_DIU_FB
> > +#if	!(defined(CONFIG_VIDEO) || defined(CONFIG_CFB_CONSOLE))
> > +	mpc5121_diu_init();
> > +#endif
> > +#endif
> > +
> 
> no blank line necessary.

Same as above.

> > +	iopin_initialize(ioregs_init, sizeof(ioregs_init) / sizeof(ioregs_init[0]));
> 
> line too long

Changed.

> > +
> > +#define CALC_TIMING(t) (t + period - 1) / period
> > +
> > +int ide_preinit (void)
> > +{
> > +	volatile immap_t *im = (immap_t *) CONFIG_SYS_IMMR;
> > +	long t;
> > +	const struct {
> > +		short t0;
> > +		short t1;
> > +		short t2_8;
> > +		short t2_16;
> > +		short t2i;
> > +		short t4;
> > +		short t9;
> > +		short tA;
> > +	} pio_specs = {
> 
> Is this a processor specific register? If so, shouldn;t this go in
> include/asm-ppc/immap_512x.h?

Well spotted, thanks - especially, since the whole code is duplicated
in board/freescale/mpc5121ads/mpc5121ads.c; split into new file,
cpu/mpc512x/ide.c

> > +#define CONFIG_ARIA 1
> > +/*
> > + * Memory map for the ARIA board:
> > + *
> > + * 0x0000_0000 - 0x0FFF_FFFF	DDR RAM (256 MB)
> > + * 0x3000_0000 - 0x3001_FFFF	On Chip SRAM (128 KB)
> > + * 0x3010_0000 - 0x3003_FFFF	On Board SRAM (128 KB - max 512KB - 1MB reserved) - CS6
> 
> line too long.

Indeed. All long lines fixed. Thanks for pointing out.

> > +#define CONFIG_ENV_IS_IN_FLASH	1
> > +/* This has to be a multiple of the Flash sector size */
> > +#define CONFIG_ENV_ADDR		(CONFIG_SYS_MONITOR_BASE + CONFIG_SYS_MONITOR_LEN)
> 
> line too long ... Hmm.. shouldn;t we accept such long lines? It seems
> better to me to have such long lines, because it would look uglier if
> we split this in two or more lines?

No, in this case it's easy to write n two lines, I think.


Thanks a lot!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How many QA engineers does it take to screw in a lightbulb? 3:  1  to
screw it in and 2 to say "I told you so" when it doesn't work.