Re: [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs

Larry Johnson wrote:

Many of the SPD bytes for DDR2 SDRAM are not interpreted correctly by the "i2c sdram" command. This patch provides correct alternative interpretations when DDR2 memory is detected.

Signed-off-by: Larry Johnson lrj@acm.org

Thanks for adding the DDR2 decoding, it is very valuable.

[snip]

common/cmd_i2c.c | 615 +++++++++++++++++++++++++++++++++++++++++++----------- 1 files changed, 496 insertions(+), 119 deletions(-)

[snip]

• switch (type) {
• case DDR2:

• puts ("CAS latency(s)              ");
  

• if (data[18] & 0x83) puts (" TBD");
  

• if (data[18] & 0x40) puts (" 6");
  

• if (data[18] & 0x20) puts (" 5");
  

• if (data[18] & 0x10) puts (" 4");
  

• if (data[18] & 0x08) puts (" 3");
  

• if (data[18] & 0x04) puts (" 2");
  

• putc ('\n');
  

• break;
  

• default:

• puts ("CAS latency(s)              ");
  

• if (data[18] & 0x80) puts (" TBD");
  

• if (data[18] & 0x40) puts (" 7");
  

• if (data[18] & 0x20) puts (" 6");
  

• if (data[18] & 0x10) puts (" 5");
  

• if (data[18] & 0x08) puts (" 4");
  

• if (data[18] & 0x04) puts (" 3");
  

• if (data[18] & 0x02) puts (" 2");
  

• if (data[18] & 0x01) puts (" 1");
  

• putc ('\n');
  

• break;
  

• }
• if (DDR2 != type) {

• puts ("CS latency(s)               ");
  

• if (data[19] & 0x80) puts (" TBD");
  

• if (data[19] & 0x40) puts (" 6");
  

• if (data[19] & 0x20) puts (" 5");
  

• if (data[19] & 0x10) puts (" 4");
  

• if (data[19] & 0x08) puts (" 3");
  

• if (data[19] & 0x04) puts (" 2");
  

• if (data[19] & 0x02) puts (" 1");
  

• if (data[19] & 0x01) puts (" 0");
  

• putc ('\n');
  

• }
• if (DDR2 != type) {

• puts ("WE latency(s)               ");
  

• if (data[20] & 0x80) puts (" TBD");
  

• if (data[20] & 0x40) puts (" 6");
  

• if (data[20] & 0x20) puts (" 5");
  

• if (data[20] & 0x10) puts (" 4");
  

• if (data[20] & 0x08) puts (" 3");
  

• if (data[20] & 0x04) puts (" 2");
  

• if (data[20] & 0x02) puts (" 1");
  

• if (data[20] & 0x01) puts (" 0");
  

• putc ('\n');
  

• }

I don't know if it is worth the effort in terms of code size weighed against obscuring the actual code, but there are a lot of bit decoding to strings going on (and, yes, if you look at the history, it is probably my fault). The following concept should work...

static char *decodeDDR2[] = { " TBD", " TBD", " 2", " 3", " 4", " 5", " 6", " TBD", };

static char *decode06[] = { " 0", " 1", " 2", " 3", " 4", " 5", " 6", " TBD", };

static char *decode17[] = { " 1", " 2", " 3", " 4", " 5", " 6", " 7", " TBD", };

void decodebits(int n, char *str[]) { int j, k;

for (k = 0, j = 0x80; j != 0; j >> 1, k++) { if (n & j) puts(str[k]); } }

...and then the example usage would be:

if (DDR2 != type) { puts ("WE latency(s) "); bits2string(data[20], decode06); putc ('\n'); }

If we aren't concerned with TBDs for undefined bits, the above lists could be condensed into one list that is " 0" through " 8" - one decode is 1..8 which could be handled with a decode of 0..7 and a shift.

If you think the decodebits() decoding is worth while, I would strongly advocate dropping the "TBDs" and do the following:

static char *decode08[] = { " 0", " 1", " 2", " 3", " 4", " 5", " 6", " 7", " 8", };

Example for 1..8 decoding: default: puts ("CAS latency(s) "); decodebits((int)data[18] << 1, decode08); putc ('\n'); break;

There are a couple of decodes that '0' means one thing and '1' means another (handled by if/else statements) that the above decode wouldn't handle (could, but the penalty would probably be bigger than the gain).

[snip]

• switch (type) {
• case DDR2:

• if (data[22] & 0x80) puts ("  TBD (bit 7)\n");
  

• if (data[22] & 0x40) puts ("  TBD (bit 6)\n");
  

• if (data[22] & 0x20) puts ("  TBD (bit 5)\n");
  

• if (data[22] & 0x10) puts ("  TBD (bit 4)\n");
  

• if (data[22] & 0x08) puts ("  TBD (bit 3)\n");
  

• if (data[22] & 0x04)
  

• 	puts ("  Supports parital array self refresh\n");
  

Typo: "partial"

[snip]

• switch (type) {
• case DDR2:

• printf("SDRAM cycle time (2nd highest CAS latency)        %d.",
  

•        (data[23] >> 4) & 0x0F);
  

• switch (data[23] & 0x0F) {
  

• case 0x0:
  

• case 0x1:
  

• case 0x2:
  

• case 0x3:
  

• case 0x4:
  

• case 0x5:
  

• case 0x6:
  

• case 0x7:
  

• case 0x8:
  

• case 0x9:
  

• 	printf("%d ns\n", data[23] & 0x0F);
  

• 	break;
  

• case 0xA:
  

• 	puts("25 ns\n");
  

• 	break;
  

• case 0xB:
  

• 	puts("33 ns\n");
  

• 	break;
  

• case 0xC:
  

• 	puts("66 ns\n");
  

• 	break;
  

• case 0xD:
  

• 	puts("75 ns\n");
  

• 	break;
  

• default:
  

• 	puts("?? ns\n");
  

• 	break;
  

We saw this code before, could refactor into a subroutine that does the decoding and printing.

[snip]

• switch (type) {
• case DDR2:

• printf("SDRAM cycle time (3rd highest CAS latency)        %d.",
  

•        (data[25] >> 4) & 0x0F);
  

• switch (data[25] & 0x0F) {
  

• case 0x0:
  

• case 0x1:
  

• case 0x2:
  

• case 0x3:
  

• case 0x4:
  

• case 0x5:
  

• case 0x6:
  

• case 0x7:
  

• case 0x8:
  

• case 0x9:
  

• 	printf("%d ns\n", data[25] & 0x0F);
  

• 	break;
  

• case 0xA:
  

• 	puts("25 ns\n");
  

• 	break;
  

• case 0xB:
  

• 	puts("33 ns\n");
  

• 	break;
  

• case 0xC:
  

• 	puts("66 ns\n");
  

• 	break;
  

• case 0xD:
  

• 	puts("75 ns\n");
  

• 	break;
  

• default:
  

• 	puts("?? ns\n");
  

• 	break;
  

• }
  

• break;
  

Hey, deja vue all over again. :-)

[snip]

• switch (type) {
• case DDR2:

• printf("Minimum row precharge        %d", data[27] >> 2);
  

• switch (data[27] & 0x03) {
  

• 	case 0x0: puts(".00 ns\n"); break;
  

• 	case 0x1: puts(".25 ns\n"); break;
  

• 	case 0x2: puts(".50 ns\n"); break;
  

• 	case 0x3: puts(".75 ns\n"); break;
  

• }
  

• break;
  

• default:

• printf("Minimum row precharge        %d nS\n", data[27]);
  

• break;
  

• }
• switch (type) {
• case DDR2:

• printf("Row active to row active min %d", data[28] >> 2);
  

• switch (data[28] & 0x03) {
  

• 	case 0x0: puts(".00 ns\n"); break;
  

• 	case 0x1: puts(".25 ns\n"); break;
  

• 	case 0x2: puts(".50 ns\n"); break;
  

• 	case 0x3: puts(".75 ns\n"); break;
  

• }
  

• break;
  

• default:

• printf("Row active to row active min %d nS\n", data[28]);
  

• break;
  

• }
• switch (type) {
• case DDR2:

• printf("RAS to CAS delay min         %d", data[29] >> 2);
  

• switch (data[29] & 0x03) {
  

• 	case 0x0: puts(".00 ns\n"); break;
  

• 	case 0x1: puts(".25 ns\n"); break;
  

• 	case 0x2: puts(".50 ns\n"); break;
  

• 	case 0x3: puts(".75 ns\n"); break;
  

• }
  

• break;
  

Hmm, another troika that could be refactored into a helper subroutine. On second thought, this is probably not worth a helper routine (too trivial?): replacing the switch statements with arithmetic is going to be slightly less speed efficient, but quite a bit more code efficient:

printf(".%02d ns\n", (data[29] & 0x03) * 25);

[snip]

Thanks again, gvb