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

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.