[U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores

[Stelian Pop]

Hi Rémy,

I do have a few comments, see below:

On Mon, Jun 01, 2009 at 08:21:42PM +0200, Remy Bohmer wrote:

>  V2 reworked comments from Jean-Christophe PLAGNIOL-VILLARD
[...]

> +COBJS-$(CONFIG_DISPLAY_CPUINFO) +=cpuinfo.o

missing space ?

> +	switch ((cidr & AT91_CIDR_EPROC) >> 5) {
> +	 /* 0x1 = ARM946ES, 0x2 = ARM7TDMI, 0x4 = ARM920T */
> +	case 0x5: txt = "ARM926EJS";	break;
> +	default:  txt = "undefined";	break;

since there are only four choices (until now at least...), why not
put the 'txt' for all of them ?

and the 'undefined' string is not a good choice: it represents a
configuration not known to U-Boot, but its numeric value is well
defined. So I suggest you change this to 'unknown', or even better,
'unknown: 0xfoo'.

> +	}
> +	printf("Embedded Processor: %s\n", txt);
> +
> +	switch ((cidr & AT91_CIDR_ARCH) >> 20) {
> +	case 0x19: txt = "AT91SAM9xx";		break;
> +	case 0x29: txt = "AT91SAM9XExx";	break;
> +	case 0x39: txt = "CAP9";		break;
> +	default:   txt = "undefined";		break;

same comments as above for 'undefined'.

> +	switch ((cidr & AT91_CIDR_NVPTYP) >> 28) {
> +	case 0x0: txt = "ROM";				 break;
> +	case 0x1: txt = "ROMless or onchip flash";	 break;
> +	case 0x2: txt = "Embedded flash memory";	 break;
> +	case 0x3: txt = "ROM and Embedded flash memory"; break;
> +	case 0x4: txt = "SRAM emulating ROM";		 break;
> +	default:  txt = "undefined";			 break;

ditto.

> +	switch (cidr & AT91_CIDR_SRAMSIZ) {
> +	case AT91_CIDR_SRAMSIZ_1K: txt = "1K";		break;
> +	case AT91_CIDR_SRAMSIZ_2K: txt = "2K";		break;
> +	case AT91_CIDR_SRAMSIZ_112K: txt = "112K";	break;
> +	case AT91_CIDR_SRAMSIZ_4K: txt = "4K";		break;
> +	case AT91_CIDR_SRAMSIZ_80K: txt = "80K";	break;
> +	case AT91_CIDR_SRAMSIZ_160K: txt = "160K";	break;
> +	case AT91_CIDR_SRAMSIZ_8K: txt = "8K";		break;
> +	case AT91_CIDR_SRAMSIZ_16K: txt = "16K";	break;
> +	case AT91_CIDR_SRAMSIZ_32K: txt = "32K";	break;
> +	case AT91_CIDR_SRAMSIZ_64K: txt = "64K";	break;
> +	case AT91_CIDR_SRAMSIZ_128K: txt = "128K";	break;
> +	case AT91_CIDR_SRAMSIZ_256K: txt = "256K";	break;
> +	case AT91_CIDR_SRAMSIZ_96K: txt = "96K";	break;
> +	case AT91_CIDR_SRAMSIZ_512K: txt = "512K";	break;
> +	default:  txt = "undefined";			break;

bad alignment here (you did align the 'txt =' properly in all other
switch/case statements, but not here).

ditto for 'undefined'.

Stelian.

-- 
Stelian Pop <stelian at popies.net>