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

[Pantelis Antoniou]

On 29 Νοε 2006, at 11:32 ΜΜ, Wolfgang Denk wrote:

> 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.
>

OK

>> --- 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.
>

OK

>> --- /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?
>

Well, here's the thing, if the board's DRAM is not soldered properly  
this will
probably trigger - think of it as a cheap memory test.

But it's not biggie to go...

> ...
>> +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?
>

OK

>> +#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.
>

Well, there is not a common memory test code. I could move this to a  
common
area. But I'll only modify this board to use it; the rest is the job  
of their
respective maintainers.

>> --- /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.
>

Nice catch - forgot it.

>
> 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

Regards

Pantelis