* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
@ 2008-01-11 3:23 Larry Johnson
2008-01-11 6:44 ` Stefan Roese
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Larry Johnson @ 2008-01-11 3:23 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 3:23 [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs Larry Johnson
@ 2008-01-11 6:44 ` Stefan Roese
2008-01-11 13:35 ` Jerry Van Baren
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Stefan Roese @ 2008-01-11 6:44 UTC (permalink / raw)
To: u-boot
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 at denx.de
=====================================================================
^ permalink raw reply [flat|nested] 9+ messages in thread* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 3:23 [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs Larry Johnson
2008-01-11 6:44 ` Stefan Roese
@ 2008-01-11 13:35 ` Jerry Van Baren
2008-01-11 17:02 ` Jon Loeliger
2008-01-11 21:25 ` Lawrence R. Johnson
2008-01-12 17:29 ` gvb.uboot
2008-01-12 20:02 ` Wolfgang Denk
3 siblings, 2 replies; 9+ messages in thread
From: Jerry Van Baren @ 2008-01-11 13:35 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 13:35 ` Jerry Van Baren
@ 2008-01-11 17:02 ` Jon Loeliger
2008-01-11 17:20 ` Jerry Van Baren
2008-01-11 21:25 ` Lawrence R. Johnson
1 sibling, 1 reply; 9+ messages in thread
From: Jon Loeliger @ 2008-01-11 17:02 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 17:02 ` Jon Loeliger
@ 2008-01-11 17:20 ` Jerry Van Baren
0 siblings, 0 replies; 9+ messages in thread
From: Jerry Van Baren @ 2008-01-11 17:20 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 13:35 ` Jerry Van Baren
2008-01-11 17:02 ` Jon Loeliger
@ 2008-01-11 21:25 ` Lawrence R. Johnson
2008-01-11 21:46 ` Jerry Van Baren
1 sibling, 1 reply; 9+ messages in thread
From: Lawrence R. Johnson @ 2008-01-11 21:25 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 21:25 ` Lawrence R. Johnson
@ 2008-01-11 21:46 ` Jerry Van Baren
0 siblings, 0 replies; 9+ messages in thread
From: Jerry Van Baren @ 2008-01-11 21:46 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 3:23 [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs Larry Johnson
2008-01-11 6:44 ` Stefan Roese
2008-01-11 13:35 ` Jerry Van Baren
@ 2008-01-12 17:29 ` gvb.uboot
2008-01-12 20:02 ` Wolfgang Denk
3 siblings, 0 replies; 9+ messages in thread
From: gvb.uboot @ 2008-01-12 17:29 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs
2008-01-11 3:23 [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs Larry Johnson
` (2 preceding siblings ...)
2008-01-12 17:29 ` gvb.uboot
@ 2008-01-12 20:02 ` Wolfgang Denk
3 siblings, 0 replies; 9+ messages in thread
From: Wolfgang Denk @ 2008-01-12 20:02 UTC (permalink / raw)
To: u-boot
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Man is the best computer we can put aboard a spacecraft ... and the
only one that can be mass produced with unskilled labor.
- Wernher von Braun
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-01-12 20:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-11 3:23 [U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs Larry Johnson
2008-01-11 6:44 ` Stefan Roese
2008-01-11 13:35 ` Jerry Van Baren
2008-01-11 17:02 ` Jon Loeliger
2008-01-11 17:20 ` Jerry Van Baren
2008-01-11 21:25 ` Lawrence R. Johnson
2008-01-11 21:46 ` Jerry Van Baren
2008-01-12 17:29 ` gvb.uboot
2008-01-12 20:02 ` Wolfgang Denk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox