Skip to contentSkip to conversation

Fix "i2c sdram" command for DDR2 DIMMs

PATCH New Larry Johnson 6 participants 9 messages Jan 11, 2008 – Jan 12, 2008

lore
Share & tools
lore

Larry Johnson

author
#0 lore raw

PATCH 1/1 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 <[email protected]>

---
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(-)
common/cmd_i2c.c +496 -119
index 10cab46..988f91d 100644
@@ -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
#1 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

Quoted 8 lines · Larry Johnson

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

---
Hi Wolfgang and Stefan and group,

Quoted 7 lines

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.

Quoted 2 lines

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?

Quoted 4 lines

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
=====================================================================

#2 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

In reply to Larry Johnson
Quoted 5 lines · Larry Johnson

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

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

[snip]

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

[snip]

Quoted 41 lines
common/cmd_i2c.c +41 -0
+
+	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)               ");

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 925

Quoted 10 lines
common/cmd_i2c.c +10 -0
+		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]

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 975

Quoted 9 lines
common/cmd_i2c.c +9 -0
+	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]

Quoted 23 lines
common/cmd_i2c.c +23 -0
+	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");

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 1028

Quoted 10 lines
common/cmd_i2c.c +10 -0
+			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]

Quoted 25 lines
common/cmd_i2c.c +25 -0
+	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:

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 1082

Quoted 10 lines
common/cmd_i2c.c +10 -0
+			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]

Quoted 30 lines
common/cmd_i2c.c +30 -0
+	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;
+	}
+

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 1139

Quoted 10 lines
common/cmd_i2c.c +10 -0
+	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

#3 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

Quoted 8 lines · Jerry Van Baren
Quoted 5 lines · Larry Johnson

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

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

#4 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

Quoted 16 lines · Jon Loeliger
Quoted 9 lines · Jerry Van Baren
Quoted 6 lines · Larry Johnson

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

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.

Quoted 7 lines

My one feeling isn't hurt.

Much.

*sniff*

jdl

Yeah, OK, like now I owe you a beer _too_?

gvb

author
#5 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

In reply to Jerry Van Baren
Quoted 11 lines · Jerry Van Baren
Quoted 5 lines · Larry Johnson

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

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.

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

[snip]

Quoted 41 lines
common/cmd_i2c.c +41 -0
+
+    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)               ");

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 925

Quoted 10 lines
common/cmd_i2c.c +10 -0
+        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]);
    }
}
Quoted 7 lines

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

Quoted 19 lines

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",
};
Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 50 lines

[snip]

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 975

Quoted 9 lines
common/cmd_i2c.c +9 -0
+    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]

Quoted 23 lines
common/cmd_i2c.c +23 -0
+    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");

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 1028

Quoted 10 lines
common/cmd_i2c.c +10 -0
+            break;
+        case 0xC:
+            puts("66 ns\n");
+            break;
+        case 0xD:
+            puts("75 ns\n");
+            break;
+        default:
+            puts("?? ns\n");
+            break;
Quoted 2 lines

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

Yes.

Quoted 88 lines

[snip]

Quoted 25 lines
common/cmd_i2c.c +25 -0
+    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:

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 1082

Quoted 10 lines
common/cmd_i2c.c +10 -0
+            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]

Quoted 30 lines
common/cmd_i2c.c +30 -0
+    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;
+    }
+

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 1139

Quoted 10 lines
common/cmd_i2c.c +10 -0
+    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:

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

[snip]

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

Quoted 3 lines

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

#6 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

Quoted 2 lines · Lawrence R. Johnson
Quoted 1 line · Jerry Van Baren

Larry Johnson wrote:

[snip]

Quoted 15 lines
Quoted 15 lines

common/cmd_i2c.c int do_sdram ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 1139

Quoted 10 lines
common/cmd_i2c.c +10 -0
+    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:

Quoted 5 lines
Quoted 3 lines
    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

Quoted 11 lines
Quoted 3 lines

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

#7 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

In reply to Larry Johnson
Quoted 19 lines · Larry Johnson

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

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

Quoted 7 lines

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

Thanks,
gvb

Applied
#8 lore raw

PATCH 1/1 Fix "i2c sdram" command for DDR2 DIMMs

In reply to Larry Johnson
Quoted 6 lines · In message <[email protected]> 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 <[email protected]>

---

Applied. Thanks.

Best regards,

Wolfgang Denk

Signature · 6 lines
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