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

Jerry Van Baren wrote:

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]

Jerry, thank you for your suggestions! I tried to keep my patch mostly functional for clarity: I've learned (through experience ;-)) that mixing functional changes with implementation and cosmetic changes in a submission is not a good idea. I did have the notion that this command could benefit from some refactoring, but I hadn't thought it through in detail as you have.

...

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');
}

I think this is worth the worth the effort both for code size and for maintainability.

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;

I'm inclined to keep the TBDs, at least for now. I think they convey useful information about the state of the standards at the time the code was written. Also, if we do want to change this functionality, that should probably happen outside the context of a refactoring.

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

I agree that the simple case is the only one that (currently) occurs frequently enough to have a helper function.

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

Yes.

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

Doh! I must have been stuck in switch-statement mode.

Thanks again, gvb

You have inspired me to go ahead with the refactoring. I'm running MAKEALLs on my changes, and will post them when they come out OK. I plan to create the patch against my original submission, which should make the changes easier to follow.

Best regards, Larry