[U-Boot-Users] [PATCH 4/5] Support Analogue Micro's ASP8347DB board.

[Wolfgang Denk]

Dear Pantelis,

in message <20061129172613.24638.18292.stgit at pantathon.hol.gr> you wrote:
> 
>     Add support for Analogue Micro's ASP8347DB board.
>     The board is originally shipped with RedBoot.
>     All appropriate settings are migrated to u-boot & the
>     old kernel booted; a drop in bootloader replacement.

In addition to my  previous  comments  abot  necessary  coding  style
cleanup  ((indentation  by  TABs, trailing white space, C++ comments,
line length, etc.) please cleanup the following items:

> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -130,7 +130,7 @@ LIST_8260="	\
>  #########################################################################
>  
>  LIST_83xx="	\
> -	TQM834x		MPC8349EMDS					\
> +	TQM834x		MPC8349EMDS	ASP8347DB			\

Please keep lists sorted.

> --- a/Makefile
> +++ b/Makefile
> @@ -1597,6 +1597,9 @@ TQM834x_config:	unconfig
>  MPC8349EMDS_config:	unconfig
>  	@$(MKCONFIG) $(@:_config=) ppc mpc83xx mpc8349emds
>  
> +ASP8347DB_config:	unconfig
> +	@$(MKCONFIG) $(@:_config=) ppc mpc83xx asp8347db
> +

Please keep lists sorted.

> --- /dev/null
> +++ b/board/asp8347db/asp8347db.c
...
> +long int initdram (int board_type)
> +{
> +	long size, known_size;
> +
> +	/* 128MB */
> +	known_size = 128 << 20;
...
> +	/* size detection */
> +	debug("\n");
> +	size = get_ram_size(CFG_DDR_BASE, known_size);
> +	if (size != known_size)
> +		debug("Detected other size than what expected!\n");

You probably want to remove this for the production version?

...
> +static void move64(unsigned long long *src, unsigned long long *dest)
> +{
> +#if defined(CONFIG_MPC8260) || defined(CONFIG_MPC824X)

Isn't it unlikely that any of these conditions is true on a 834x board?

> +#if defined (CFG_DRAM_TEST_DATA)
> +
> +unsigned long long pattern[] = {
> +	0xaaaaaaaaaaaaaaaaULL,
> +	0xccccccccccccccccULL,
> +	0xf0f0f0f0f0f0f0f0ULL,
> +	0xff00ff00ff00ff00ULL,
> +	0xffff0000ffff0000ULL,
> +	0xffffffff00000000ULL,
> +	0x00000000ffffffffULL,
> +	0x0000ffff0000ffffULL,
> +	0x00ff00ff00ff00ffULL,
> +	0x0f0f0f0f0f0f0f0fULL,
> +	0x3333333333333333ULL,
> +	0x5555555555555555ULL,
> +};
> +
> +/*********************************************************************/
> +/* NAME:  mem_test_data() -  test data lines for shorts and opens    */
> +/*								     */
> +/* DESCRIPTION:							     */
> +/*   Tests data lines for shorts and opens by forcing adjacent data  */
> +/*   to opposite states. Because the data lines could be routed in   */
> +/*   an arbitrary manner the must ensure test patterns ensure that   */
> +/*   every case is tested. By using the following series of binary   */
> +/*   patterns every combination of adjacent bits is test regardless  */
> +/*   of routing.						     */

Do we really need yet another copy of this code?


> +/*********************************************************************/
> +/* NAME:    testdram() -  calls any enabled memory tests	     */
...

This would be at least the 5th copy of this code.

Please don't do this. Let's clean this up and use one common verion -
if needed at all, as there is other memtest code, too.

> --- /dev/null
> +++ b/board/asp8347db/fpga.c
...
> +static void mdelay(int ms)
> +{
> +	ulong start = get_timer(0);
> +	ulong delay;
> +
> +	delay = (ms * CFG_HZ) / 1000;
> +	while (get_timer(start) < delay) {
> +		udelay (1000);
> +		WATCHDOG_RESET();	/* Trigger watchdog, if needed */

Note: this is redundant, as udelay() will already trigger the
watchdog.


Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Consistency requires you to be as ignorant today as you were a  year
ago."                                              - Bernard Berenson