[U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
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 --- Hi Wolfgang and Stefan and group,
In this patch, I have fixed the DDR2 output for the SPD bytes currently displayed by the "i2c sdram". I have not added any display for the additional bytes defined in DDR2; I may do that later when I have time.
I don't have any way of testing the patched code for non-DDR2 memory, but if I force the code to treat the DDR2 memory type as "unknown", I get the same values as before the patch.
Stefan, the added size of this causes a section overlap on the Luan board, and makes the existing overlap on the Ocotea board worse.
If memory size is critical, I could redo this patch using conditional compilation for different memory types, though I believe this would add complexity and reduce flexibility. If you would prefer this, please let me know.
Best regards, Larry
common/cmd_i2c.c | 615 +++++++++++++++++++++++++++++++++++++++++++----------- 1 files changed, 496 insertions(+), 119 deletions(-)
diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c index 10cab46..988f91d 100644 --- a/common/cmd_i2c.c +++ b/common/cmd_i2c.c @@ -662,6 +662,7 @@ int do_i2c_loop(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) */ int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { + enum {unknown, EDO, SDRAM, DDR2} type; u_char chip; u_char data[128]; u_char cksum; @@ -696,10 +697,22 @@ int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) printf("Serial memory size 0x%02X\n", 1 << data[1]); puts ("Memory type "); switch(data[2]) { - case 2: puts ("EDO\n"); break; - case 4: puts ("SDRAM\n"); break; - case 8: puts ("DDR2\n"); break; - default: puts ("unknown\n"); break; + case 2: + type = EDO; + puts ("EDO\n"); + break; + case 4: + type = SDRAM; + puts ("SDRAM\n"); + break; + case 8: + type = DDR2; + puts ("DDR2\n"); + break; + default: + type = unknown; + puts ("unknown\n"); + break; } puts ("Row address bits "); if ((data[3] & 0x00F0) == 0) @@ -711,22 +724,89 @@ int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) printf("%d\n", data[4] & 0x0F); else printf("%d/%d\n", data[4] & 0x0F, (data[4] >> 4) & 0x0F); - printf("Module rows %d\n", data[5]); - printf("Module data width %d bits\n", (data[7] << 8) | data[6]); + + switch (type) { + case DDR2: + printf("Number of ranks %d\n", + (data[5] & 0x07) + 1); + break; + default: + printf("Module rows %d\n", data[5]); + break; + } + + switch (type) { + case DDR2: + printf("Module data width %d bits\n", data[6]); + break; + default: + printf("Module data width %d bits\n", + (data[7] << 8) | data[6]); + break; + } + puts ("Interface signal levels "); switch(data[8]) { - case 0: puts ("5.0v/TTL\n"); break; + case 0: puts ("TTL 5.0 V\n"); break; case 1: puts ("LVTTL\n"); break; - case 2: puts ("HSTL 1.5\n"); break; - case 3: puts ("SSTL 3.3\n"); break; - case 4: puts ("SSTL 2.5\n"); break; - case 5: puts ("SSTL 1.8\n"); break; + case 2: puts ("HSTL 1.5 V\n"); break; + case 3: puts ("SSTL 3.3 V\n"); break; + case 4: puts ("SSTL 2.5 V\n"); break; + case 5: puts ("SSTL 1.8 V\n"); break; default: puts ("unknown\n"); break; } - printf("SDRAM cycle time %d.%d nS\n", - (data[9] >> 4) & 0x0F, data[9] & 0x0F); - printf("SDRAM access time %d.%d nS\n", - (data[10] >> 4) & 0x0F, data[10] & 0x0F); + + switch (type) { + case DDR2: + printf("SDRAM cycle time %d.", + (data[9] >> 4) & 0x0F); + switch (data[9] & 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[9] & 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; + default: + printf("SDRAM cycle time %d.%d nS\n", + (data[9] >> 4) & 0x0F, data[9] & 0x0F); + break; + } + + switch (type) { + case DDR2: + printf("SDRAM access time 0.%d%d ns\n", + (data[10] >> 4) & 0x0F, data[10] & 0x0F); + break; + default: + printf("SDRAM access time %d.%d nS\n", + (data[10] >> 4) & 0x0F, data[10] & 0x0F); + break; + } + puts ("EDC configuration "); switch(data[11]) { case 0: puts ("None\n"); break; @@ -739,28 +819,51 @@ int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) else puts ("Self refresh, rate "); switch(data[12] & 0x7F) { - case 0: puts ("15.625uS\n"); break; - case 1: puts ("3.9uS\n"); break; - case 2: puts ("7.8uS\n"); break; - case 3: puts ("31.3uS\n"); break; - case 4: puts ("62.5uS\n"); break; - case 5: puts ("125uS\n"); break; + case 0: puts ("15.625 uS\n"); break; + case 1: puts ("3.9 uS\n"); break; + case 2: puts ("7.8 uS \n"); break; + case 3: puts ("31.3 uS\n"); break; + case 4: puts ("62.5 uS\n"); break; + case 5: puts ("125 uS\n"); break; default: puts ("unknown\n"); break; } - printf("SDRAM width (primary) %d\n", data[13] & 0x7F); - if ((data[13] & 0x80) != 0) { - printf(" (second bank) %d\n", - 2 * (data[13] & 0x7F)); - } - if (data[14] != 0) { - printf("EDC width %d\n", - data[14] & 0x7F); - if ((data[14] & 0x80) != 0) + + switch (type) { + case DDR2: + printf("SDRAM width (primary) %d\n", data[13]); + break; + default: + printf("SDRAM width (primary) %d\n", data[13] & 0x7F); + if ((data[13] & 0x80) != 0) { printf(" (second bank) %d\n", - 2 * (data[14] & 0x7F)); + 2 * (data[13] & 0x7F)); + } + break; + } + + switch (type) { + case DDR2: + if (data[14] != 0) + printf("EDC width %d\n", data[14]); + break; + default: + if (data[14] != 0) { + printf("EDC width %d\n", + data[14] & 0x7F); + + if ((data[14] & 0x80) != 0) { + printf(" (second bank) %d\n", + 2 * (data[14] & 0x7F)); + } + } + break; } - printf("Min clock delay, back-to-back random column addresses %d\n", - data[15]); + + if (DDR2 != type ) { + printf("Min clock delay, back-to-back random column addresses " + "%d\n", data[15]); + } + puts ("Burst length(s) "); if (data[16] & 0x80) puts (" Page"); if (data[16] & 0x08) puts (" 8"); @@ -769,91 +872,363 @@ int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) if (data[16] & 0x01) puts (" 1"); putc ('\n'); printf("Number of banks %d\n", data[17]); - 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'); - 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'); - 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'); - puts ("Module attributes:\n"); - if (!data[21]) puts (" (none)\n"); - if (data[21] & 0x80) puts (" TBD (bit 7)\n"); - if (data[21] & 0x40) puts (" Redundant row address\n"); - if (data[21] & 0x20) puts (" Differential clock input\n"); - if (data[21] & 0x10) puts (" Registerd DQMB inputs\n"); - if (data[21] & 0x08) puts (" Buffered DQMB inputs\n"); - if (data[21] & 0x04) puts (" On-card PLL\n"); - if (data[21] & 0x02) puts (" Registered address/control lines\n"); - if (data[21] & 0x01) puts (" Buffered address/control lines\n"); - puts ("Device attributes:\n"); - if (data[22] & 0x80) puts (" TBD (bit 7)\n"); - if (data[22] & 0x40) puts (" TBD (bit 6)\n"); - if (data[22] & 0x20) puts (" Upper Vcc tolerance 5%\n"); - else puts (" Upper Vcc tolerance 10%\n"); - if (data[22] & 0x10) puts (" Lower Vcc tolerance 5%\n"); - else puts (" Lower Vcc tolerance 10%\n"); - if (data[22] & 0x08) puts (" Supports write1/read burst\n"); - if (data[22] & 0x04) puts (" Supports precharge all\n"); - if (data[22] & 0x02) puts (" Supports auto precharge\n"); - if (data[22] & 0x01) puts (" Supports early RAS# precharge\n"); - printf("SDRAM cycle time (2nd highest CAS latency) %d.%d nS\n", - (data[23] >> 4) & 0x0F, data[23] & 0x0F); - printf("SDRAM access from clock (2nd highest CAS latency) %d.%d nS\n", - (data[24] >> 4) & 0x0F, data[24] & 0x0F); - printf("SDRAM cycle time (3rd highest CAS latency) %d.%d nS\n", - (data[25] >> 4) & 0x0F, data[25] & 0x0F); - printf("SDRAM access from clock (3rd highest CAS latency) %d.%d nS\n", - (data[26] >> 4) & 0x0F, data[26] & 0x0F); - printf("Minimum row precharge %d nS\n", data[27]); - printf("Row active to row active min %d nS\n", data[28]); - printf("RAS to CAS delay min %d nS\n", data[29]); + + 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'); + } + + switch (type) { + case DDR2: + puts ("Module attributes:\n"); + if (data[21] & 0x80) + puts (" TBD (bit 7)\n"); + if (data[21] & 0x40) + puts (" Analysis probe installed\n"); + if (data[21] & 0x20) + puts (" TBD (bit 5)\n"); + if (data[21] & 0x10) + puts (" FET switch external enable\n"); + printf(" %d PLLs on DIMM\n", (data[21] >> 2) & 0x03); + if (data[20] & 0x11) { + printf(" %d active registers on DIMM\n", + (data[21] & 0x03) + 1); + } + break; + default: + puts ("Module attributes:\n"); + if (!data[21]) + puts (" (none)\n"); + if (data[21] & 0x80) + puts (" TBD (bit 7)\n"); + if (data[21] & 0x40) + puts (" Redundant row address\n"); + if (data[21] & 0x20) + puts (" Differential clock input\n"); + if (data[21] & 0x10) + puts (" Registerd DQMB inputs\n"); + if (data[21] & 0x08) + puts (" Buffered DQMB inputs\n"); + if (data[21] & 0x04) + puts (" On-card PLL\n"); + if (data[21] & 0x02) + puts (" Registered address/control lines\n"); + if (data[21] & 0x01) + puts (" Buffered address/control lines\n"); + break; + } + + 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"); + if (data[22] & 0x02) + puts (" Supports 50 ohm ODT\n"); + if (data[22] & 0x01) + puts (" Supports weak driver\n"); + break; + default: + puts ("Device attributes:\n"); + if (data[22] & 0x80) puts (" TBD (bit 7)\n"); + if (data[22] & 0x40) puts (" TBD (bit 6)\n"); + if (data[22] & 0x20) puts (" Upper Vcc tolerance 5%\n"); + else puts (" Upper Vcc tolerance 10%\n"); + if (data[22] & 0x10) puts (" Lower Vcc tolerance 5%\n"); + else puts (" Lower Vcc tolerance 10%\n"); + if (data[22] & 0x08) puts (" Supports write1/read burst\n"); + if (data[22] & 0x04) puts (" Supports precharge all\n"); + if (data[22] & 0x02) puts (" Supports auto precharge\n"); + if (data[22] & 0x01) puts (" Supports early RAS# precharge\n"); + break; + } + + 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; + } + break; + default: + printf("SDRAM cycle time (2nd highest CAS latency) %d." + "%d nS\n", (data[23] >> 4) & 0x0F, data[23] & 0x0F); + break; + } + + switch (type) { + case DDR2: + printf("SDRAM access from clock (2nd highest CAS latency) 0." + "%d%d ns\n", (data[24] >> 4) & 0x0F, data[24] & 0x0F); + break; + default: + printf("SDRAM access from clock (2nd highest CAS latency) %d." + "%d nS\n", (data[24] >> 4) & 0x0F, data[24] & 0x0F); + break; + } + + 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; + default: + printf("SDRAM cycle time (3rd highest CAS latency) %d." + "%d nS\n", (data[25] >> 4) & 0x0F, data[25] & 0x0F); + break; + } + + switch (type) { + case DDR2: + printf("SDRAM access from clock (3rd highest CAS latency) 0." + "%d%d ns\n", (data[26] >> 4) & 0x0F, data[26] & 0x0F); + break; + default: + printf("SDRAM access from clock (3rd highest CAS latency) %d." + "%d nS\n", (data[26] >> 4) & 0x0F, data[26] & 0x0F); + break; + } + + 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; + default: + printf("RAS to CAS delay min %d nS\n", data[29]); + break; + } + printf("Minimum RAS pulse width %d nS\n", data[30]); - puts ("Density of each row "); - if (data[31] & 0x80) puts (" 512"); - if (data[31] & 0x40) puts (" 256"); - if (data[31] & 0x20) puts (" 128"); - if (data[31] & 0x10) puts (" 64"); - if (data[31] & 0x08) puts (" 32"); - if (data[31] & 0x04) puts (" 16"); - if (data[31] & 0x02) puts (" 8"); - if (data[31] & 0x01) puts (" 4"); - puts ("MByte\n"); - printf("Command and Address setup %c%d.%d nS\n", - (data[32] & 0x80) ? '-' : '+', - (data[32] >> 4) & 0x07, data[32] & 0x0F); - printf("Command and Address hold %c%d.%d nS\n", - (data[33] & 0x80) ? '-' : '+', - (data[33] >> 4) & 0x07, data[33] & 0x0F); - printf("Data signal input setup %c%d.%d nS\n", - (data[34] & 0x80) ? '-' : '+', - (data[34] >> 4) & 0x07, data[34] & 0x0F); - printf("Data signal input hold %c%d.%d nS\n", - (data[35] & 0x80) ? '-' : '+', - (data[35] >> 4) & 0x07, data[35] & 0x0F); + + switch (type) { + case DDR2: + puts ("Density of each row "); + if (data[31] & 0x80) puts (" 512 MiB\n"); + if (data[31] & 0x40) puts (" 256 MiB\n"); + if (data[31] & 0x20) puts (" 128 MiB\n"); + if (data[31] & 0x10) puts (" 16 GiB\n"); + if (data[31] & 0x08) puts (" 8 GiB\n"); + if (data[31] & 0x04) puts (" 4 GiB\n"); + if (data[31] & 0x02) puts (" 2 GiB\n"); + if (data[31] & 0x01) puts (" 1 GiB\n"); + break; + default: + puts ("Density of each row "); + if (data[31] & 0x80) puts (" 512 MiB\n"); + if (data[31] & 0x40) puts (" 256 MiB\n"); + if (data[31] & 0x20) puts (" 128 MiB\n"); + if (data[31] & 0x10) puts (" 64 MiB\n"); + if (data[31] & 0x08) puts (" 32 MiB\n"); + if (data[31] & 0x04) puts (" 16 MiB\n"); + if (data[31] & 0x02) puts (" 8 MiB\n"); + if (data[31] & 0x01) puts (" 4 MiB\n"); + break; + } + + switch (type) { + case DDR2: + puts("Command and Address setup "); + if (data[32] >= 0xA0) { + printf("1.%d%d ns\n", + ((data[32] >> 4) & 0x0F) - 10, data[32] & 0x0F); + } else { + printf("0.%d%d ns\n", + ((data[32] >> 4) & 0x0F), data[32] & 0x0F); + } + break; + default: + printf("Command and Address setup %c%d.%d nS\n", + (data[32] & 0x80) ? '-' : '+', + (data[32] >> 4) & 0x07, data[32] & 0x0F); + break; + } + + switch (type) { + case DDR2: + puts("Command and Address hold "); + if (data[33] >= 0xA0) { + printf("1.%d%d ns\n", + ((data[33] >> 4) & 0x0F) - 10, data[33] & 0x0F); + } else { + printf("0.%d%d ns\n", + ((data[33] >> 4) & 0x0F), data[33] & 0x0F); + } + break; + default: + printf("Command and Address hold %c%d.%d nS\n", + (data[33] & 0x80) ? '-' : '+', + (data[33] >> 4) & 0x07, data[33] & 0x0F); + break; + } + + switch (type) { + case DDR2: + printf("Data signal input setup 0.%d%d ns\n", + (data[34] >> 4) & 0x0F, data[34] & 0x0F); + break; + default: + printf("Data signal input setup %c%d.%d nS\n", + (data[34] & 0x80) ? '-' : '+', + (data[34] >> 4) & 0x07, data[34] & 0x0F); + break; + } + + switch (type) { + case DDR2: + printf("Data signal input hold 0.%d%d ns\n", + (data[35] >> 4) & 0x0F, data[35] & 0x0F); + break; + default: + printf("Data signal input hold %c%d.%d nS\n", + (data[35] & 0x80) ? '-' : '+', + (data[35] >> 4) & 0x07, data[35] & 0x0F); + break; + } + puts ("Manufacturer's JEDEC ID "); for (j = 64; j <= 71; j++) printf("%02X ", data[j]); @@ -869,9 +1244,11 @@ int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) for (j = 95; j <= 98; j++) printf("%02X ", data[j]); putc ('\n'); - printf("Speed rating PC%d\n", - data[126] == 0x66 ? 66 : data[126]);
+ if (DDR2 != type) { + printf("Speed rating PC%d\n", + data[126] == 0x66 ? 66 : data[126]); + } return 0; } #endif
On Friday 11 January 2008, 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
Hi Wolfgang and Stefan and group,
In this patch, I have fixed the DDR2 output for the SPD bytes currently displayed by the "i2c sdram". I have not added any display for the additional bytes defined in DDR2; I may do that later when I have time.
I don't have any way of testing the patched code for non-DDR2 memory, but if I force the code to treat the DDR2 memory type as "unknown", I get the same values as before the patch.
I'll give it a try on a DDR system in a short while.
Stefan, the added size of this causes a section overlap on the Luan board, and makes the existing overlap on the Ocotea board worse.
This should not be a real problem with the bss NOLOAD patch applied:
[PATCH] Fix linker scripts: add NOLOAD atribute to .bss/.sbss sections
Wolfgang: What's the status with this patch?
If memory size is critical, I could redo this patch using conditional compilation for different memory types, though I believe this would add complexity and reduce flexibility. If you would prefer this, please let me know.
Let's wait on the results with the NOLOAD patch first.
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@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 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
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.
And I'm chopped liver, huh? 'Cuz, I only added DDR2 decode to the 8548, like, *years* ago now.
I see how it is.
My one feeling isn't hurt.
Much.
*sniff*
jdl
Jon Loeliger wrote:
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.
And I'm chopped liver, huh? 'Cuz, I only added DDR2 decode to the 8548, like, *years* ago now.
I see how it is.
My one feeling isn't hurt.
Much.
*sniff*
jdl
Yeah, OK, like now I owe you a beer _too_?
gvb
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
Lawrence R. Johnson wrote:
Jerry Van Baren wrote:
Larry Johnson wrote:
[snip]
- 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.
You were set up on that one! :-D
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
Always a good plan, make it work before optimizing. DAMHIKT :-/
gvb
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
Hi Wolfgang and Stefan and group,
In this patch, I have fixed the DDR2 output for the SPD bytes currently displayed by the "i2c sdram". I have not added any display for the additional bytes defined in DDR2; I may do that later when I have time.
I don't have any way of testing the patched code for non-DDR2 memory, but if I force the code to treat the DDR2 memory type as "unknown", I get the same values as before the patch.
Stefan, the added size of this causes a section overlap on the Luan board, and makes the existing overlap on the Ocotea board worse.
If memory size is critical, I could redo this patch using conditional compilation for different memory types, though I believe this would add complexity and reduce flexibility. If you would prefer this, please let me know.
Best regards, Larry
Verified that the code prints the same thing for SDRAM (sacsng board) before and after Larry's patches (including "Refactor code for "i2c sdram" command") other than some gratuitous ;-) s/nS/ns/ s/uS/us/.
Acked-by: Gerald Van Baren vanbaren@cideas.com
Thanks, gvb
In message 4786E13B.3030105@acm.org you 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
Applied. Thanks.
Best regards,
Wolfgang Denk
participants (7)
-
gvb.uboot -
Jerry Van Baren -
Jon Loeliger -
Larry Johnson -
Lawrence R. Johnson
-
Stefan Roese -
Wolfgang Denk