From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerry Van Baren Date: Fri, 11 Jan 2008 08:35:23 -0500 Subject: [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs In-Reply-To: <4786E13B.3030105@acm.org> References: <4786E13B.3030105@acm.org> Message-ID: <4787709B.8000907@ge.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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