Preliminary support for DDR3 in decode-dimms (fwd)

Preliminary support for DDR3 in decode-dimms (fwd)

[Paul Goyette]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 878 bytes --]

The attached diffs provide some preliminary decode of the DDR3 SPD... 
This work is based on the recently-published Appendix K to JESD21-C
(see http://www.jedec.org/download/search/4_01_02_11R18.pdf).

Warning:  I'm no perl expert.  The code I've written is probably ugly to
most of the readers of this list.  But it does work!

There's no attempt made here to decode the Intel XMP extension data 
(located beyond byte 176).  I still haven't been able to find a spec.

----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul-3orOWTcw9wBWk0Htik3J/w@public.gmane.org   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette-3r7Miqu9kMnR7s880joybQ@public.gmane.org |
----------------------------------------------------------------------

[-- Attachment #2: Type: TEXT/PLAIN, Size: 8759 bytes --]

872a873,1035
> # Parameter: bytes 0-127
> sub decode_ddr3_sdram($)
> {
> 	my $bytes = shift;
> 	my $l;
> 	my $temp;
> 	my $ctime;
> 
> # SPD revision
> 	if ($bytes->[62] != 0xff) {
> 		printl "SPD Revision", ($bytes->[62] >> 4) . "." .
> 				       ($bytes->[62] & 0xf);
> 	}
> 
> # speed
> 	prints "Memory Characteristics";
> 
> 	$l = "Fine time base";
> 	my $dividend = ($bytes->[9] >> 4) & 15;
> 	my $divisor  = $bytes->[9] & 15;
> 	printl $l, sprintf("%.3f", $dividend / $divisor) . " ps";
> 
> 	$l = "Medium time base";
> 	$dividend = $bytes->[10];
> 	$divisor  = $bytes->[11];
> 	my $mtb = $dividend / $divisor;
> 	printl $l, tns($mtb);
> 
> 	$l = "Maximum module speed";
> 	$ctime = $bytes->[12] * $mtb;
> 	my $ddrclk = 2 * (1000 / $ctime);
> 	my $tbits = 1 << (($bytes->[8] & 7) + 3);
> 	my $pcclk = int ($ddrclk * $tbits / 8);
> 	$ddrclk = int ($ddrclk);
> 	printl $l, "${ddrclk}MHz (PC3-${pcclk})";
> 
> # Size computation
> 
> 	my $cap =  ($bytes->[4]       & 15) + 28;
> 	$cap   +=  ($bytes->[8]       & 7)  + 3;
> 	$cap   -=  ($bytes->[7]       & 7)  + 2;
> 	$cap   -= 20 + 3;
> 	my $k   = (($bytes->[7] >> 3) & 31) + 1;
> 	printl "Size", ((1 << $cap) * $k) . " MB";
> 
> 	printl "Banks x Rows x Columns x Bits",
> 	       join(' x ', 1 << ((($bytes->[4] >> 4) &  7) +  3),
> 			   ((($bytes->[5] >> 3) & 31) + 12),
> 			   ( ($bytes->[5]       &  7) +  9),
> 			   ( 1 << (($bytes->[6] &  7) + 3)) );
> 	printl "Ranks", $k;
> 
> 	printl "SDRAM Device Width", $bytes->[13]." bits";
> 
> 	my $taa;
> 	my $trcd;
> 	my $trp;
> 	my $tras;
> 
> 	$taa  = int($bytes->[16] / $bytes->[12]);
> 	$trcd = int($bytes->[18] / $bytes->[12]);
> 	$trp  = int($bytes->[20] / $bytes->[12]);
> 	$tras = int((($bytes->[21] >> 4) * 256 + $bytes->[22]) / $bytes->[12]);
> 
> 	printl "tCL-tRCD-tRP-tRAS", join("-", $taa, $trcd, $trp, $tras);
> 
> # latencies
> 	my $highestCAS = 0;
> 	my %cas;
> 	my $ii;
> 	my $cas_sup = $bytes->[15] << 8 + $bytes->[14];
> 	for ($ii = 0; $ii < 15; $ii++) {
> 		if ($cas_sup & (1 << $ii)) {
> 			$highestCAS = $ii + 4;
> 			$cas{$highestCAS}++;
> 		}
> 	}
> 	printl "Supported CAS Latencies (tCL)", cas_latencies(keys %cas);
> 
> # more timing information
> 	prints "Timing Parameters" ;
> 
> 	printl "Minimum Write Recovery time (tWRmin)", tns($bytes->[17] * $mtb);
> 	printl "Minimum Row Active-to-Row Active Delay (tRRDmin)",
> 		tns($bytes->[19] * $mtb);
> 	printl "Minimum Active-to-Active Refresh Delay (tRCmin)",
> 		tns((((($bytes->[21] >> 4) & 15) << 8) + $bytes->[23]) * $mtb);
> 	printl "Minimum Refresh Recovery Delay (tRFCmin)", 
> 		tns((($bytes->[25] << 8) + $bytes->[24]) * $mtb);
> 	printl "Minimum Write-to-Read Cmd Delay (tWTRmin)",
> 		tns($bytes->[26] * $mtb);
> 	printl "Minimum Read-to-Precharge Cmd Delay (tRTPmin)",
> 		tns($bytes->[27] * $mtb);
> 	printl "Minimum Four Activate Window Delay (tFAWmin)",
> 		tns((($bytes->[28] & 15) << 8 + $bytes->[29]) * $mtb);
> 
> # miscellaneous stuff
> 	prints "Optional Features";
> 
> 	printl "RZQ/6 supported?", ($bytes->[30] & 1) ? "Yes" : "No";
> 	printl "RZQ/7 supported?", ($bytes->[30] & 2) ? "Yes" : "No";
> 	printl "DLL-Off Mode supported?", ($bytes->[30] & 128) ? "Yes" : "No";
> 	printl "Extended (90-95C) operating range?",
> 		($bytes->[31] & 1) ? "Yes" : "No";
> 	printl "Refresh Rate in extended range",
> 		($bytes->[31] & 2) ? "2X" : "1X";
> 	printl "Auto Self-Refresh?", ($bytes->[31] & 4) ? "Yes" : "No";
> 	printl "On-Die Thermal Sensor readout?",
> 		($bytes->[31] & 8) ? "Yes" : "No";
> 	printl "Partial Array Self-Refresh?",
> 		($bytes->[31] & 128) ? "Yes" : "No";
> 	printl "Thermal Sensor Accuracy",
> 		($bytes->[32] & 128) ? sprintf($bytes->[32] & 127) :
> 					"Not implemented";
> 	printl "SDRAM Device Type",
> 		($bytes->[33] & 128) ? sprintf($bytes->[33] & 127) :
> 					"Standard Monolithic";
> 
> 	prints "Manufacturer Data";
> 
> 	$l = "Module Manufacturer Code";
> 	printl $l, sprintf("0x%.2X%.2X", $bytes->[118], $bytes->[117]);
> 
> 	$l = "DRAM Manufacturer Code";
> 	printl $l, sprintf("0x%.2X%.2X", $bytes->[148], $bytes->[149]);
> 
> 	$l = "Manufacturing Location";
> 	$temp = (chr($bytes->[8]) =~ m/^[\w\d]$/) ? chr($bytes->[8])
> 	      : sprintf("0x%.2X", $bytes->[8]);
> 	printl $l, $temp;
> 
> 	$l = "Part Number";
> 	$temp = "";
> 	for (my $i = 128; $i <= 145; $i++) {
> 		$temp .= chr($bytes->[$i]);
> 	};
> 	printl $l, $temp;
> 
> 	$l = "Revision";
> 	$temp = sprintf("0x%02X%02X\n", $bytes->[146], $bytes->[147]);
> 	printl $l, $temp;
> 
> 	$l = "Manufacturing Date";
> 	# In theory the year and week are in BCD format, but
> 	# this is not always true in practice :(
> 	if (($bytes->[120] & 0xf0) <= 0x90
> 	 && ($bytes->[120] & 0x0f) <= 0x09
> 	 && ($bytes->[121] & 0xf0) <= 0x90
> 	 && ($bytes->[121] & 0x0f) <= 0x09) {
> 		# Note that this heuristic will break in year 2080
> 		$temp = sprintf("%d%02X-W%02X\n",
> 				$bytes->[120] >= 0x80 ? 19 : 20,
> 				$bytes->[120], $bytes->[121]);
> 	} else {
> 		$temp = sprintf("0x%02X%02X\n", $bytes->[120], $bytes->[121]);
> 	}
> 	printl $l, $temp;
> 
> 	$l = "Assembly Serial Number";
> 	$temp = sprintf("0x%02X%02X%02X%02X\n", $bytes->[122], $bytes->[123]);
> 	printl $l, $temp;
> }
> 
1063a1227
> 	"DDR3 SDRAM"	=> \&decode_ddr3_sdram,
1181a1346,1371
> sub readfullspd($$) # reads all bytes from SPD-EEPROM
> {
> 	my ($size, $dimm_i) = @_;
> 	my @bytes;
> 	if ($use_hexdump) {
> 		@bytes = read_hexdump($dimm_i);
> 		return @bytes[0..$size];
> 	} elsif ($use_sysfs) {
> 		# Kernel 2.6 with sysfs
> 		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
> 			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
> 		binmode HANDLE;
> 		sysseek(HANDLE, 0, SEEK_SET);
> 		sysread(HANDLE, my $eeprom, $size);
> 		close HANDLE;
> 		@bytes = unpack(sprintf("C%d", $size), $eeprom);
> 	} else {
> 		# Kernel 2.4 with procfs
> 		for my $i (0 .. $size/16) {
> 			my $hexoff = sprintf('%02x', $i * 16);
> 			push @bytes, split(" ", `cat /proc/sys/dev/sensors/$dimm_i/$hexoff`);
> 		}
> 	}
> 	return @bytes;
> }
> 
1274,1276d1463
< 		next unless $bytes[63] == $dimm_checksum || $opt_igncheck;
< 		$dimm_count++;
< 
1295,1300d1481
< 		my $l = "EEPROM Checksum of bytes 0-62";
< 		printl $l, ($bytes[63] == $dimm_checksum ?
< 			sprintf("OK (0x%.2X)", $bytes[63]):
< 			sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
< 				$bytes[63], $dimm_checksum));
< 
1302c1483,1528
< 		my $is_rambus = $bytes[0] < 4;
---
> 		my $is_rambus = ($bytes[0] < 4 && $bytes[0] >= 0);
> 
> 		if ($is_rambus || $bytes[2] <= 8) {
> 			my $l = "EEPROM Checksum of bytes 0-62";
> 			printl $l, ($bytes[63] == $dimm_checksum ?
> 				sprintf("OK (0x%.2X)", $bytes[63]):
> 				sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
> 					$bytes[63], $dimm_checksum));
> 			next unless $bytes[63] == $dimm_checksum ||
> 				    $opt_igncheck;
> 			$dimm_count++;
> 
> 		} else {
> 			my @sizes = ( 128, 176, 256, 0, 0, 0, 0, 0);
> 			my $spdsize = $sizes[($bytes[0] >> 4) & 7];
> 
> 			@bytes = readfullspd($spdsize, $dimm_list[$i]);
> 			my $dimm_crc = 0;
> 			my $crc_cover = $bytes[0] & 0x80 ? 116 : 125;
> 			my $crc_ptr = 0;
> 			my $crc_bit;
> 			while ($crc_ptr <= $crc_cover) {
> 				$dimm_crc = $dimm_crc ^ ($bytes[$crc_ptr] << 8);
> 				for ($crc_bit = 0; $crc_bit < 8; $crc_bit++) {
> 					if ($dimm_crc & 0x8000) {
> 						$dimm_crc = ($dimm_crc << 1) ^
> 							0x1021;
> 					} else {
> 						$dimm_crc = $dimm_crc << 1
> 					}
> 				}
> 				$crc_ptr++;
> 			}
> 			$dimm_crc = $dimm_crc & 0xffff;
> 
> 			my $l = "EEPROM CRC of bytes 0-" .
> 				sprintf("%d", $crc_cover);
> 			my $crc_calc = $bytes[127] << 8 | $bytes[126];
> 			printl $l, ($dimm_crc == $crc_calc)?
> 				sprintf("OK (0x%.4X)", $dimm_crc):
> 				sprintf("Bad\n(found 0x%.4X, calculated 0x%.4X)\n",
> 					$crc_calc, $dimm_crc);
> 			next unless $crc_calc == $dimm_crc || $opt_igncheck;
> 			$dimm_count++;
> 		}
> 
1315c1541
< 		$l = "Total number of bytes in EEPROM";
---
> 		my $l = "Total number of bytes in EEPROM";
1335a1562,1564
> 			elsif ($bytes[2] == 9) { $type = "FB-DIMM"; }
> 			elsif ($bytes[2] == 10) { $type = "FB-DIMM PROBE"; }
> 			elsif ($bytes[2] == 11) { $type = "DDR3 SDRAM"; }
1342a1572,1576
> 		# DDR3 Manufacturer info is already decoded
> 		# (It's NOT common!)
> 
> 		next if ($type eq "DDR3 SDRAM");
> 
1346c1580,1584
< 		@bytes = readspd64(64, $dimm_list[$i]);
---
> 		if ($#bytes == 63) {
> 			@bytes = readspd64(64, $dimm_list[$i]);
> 		} else {
> 			@bytes = @bytes[64..$#bytes];
> 		}

Re: Preliminary support for DDR3 in decode-dimms (fwd)

[Jean Delvare]

Hi Paul,

On Sun, 23 Nov 2008 07:04:41 -0800 (PST), Paul Goyette wrote:
> The attached diffs provide some preliminary decode of the DDR3 SPD... 
> This work is based on the recently-published Appendix K to JESD21-C
> (see http://www.jedec.org/download/search/4_01_02_11R18.pdf).
> 
> Warning:  I'm no perl expert.  The code I've written is probably ugly to
> most of the readers of this list.  But it does work!

Can you please provide a unified diff (as generated by diff -u)? Other
diff formats are simply too fragile and/or too difficult for humans to
read.

-- 
Jean Delvare

Re: Preliminary support for DDR3 in decode-dimms (fwd)

[Paul Goyette]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1192 bytes --]

On Tue, 25 Nov 2008, Jean Delvare wrote:

> Hi Paul,
>
> On Sun, 23 Nov 2008 07:04:41 -0800 (PST), Paul Goyette wrote:
>> The attached diffs provide some preliminary decode of the DDR3 SPD...
>> This work is based on the recently-published Appendix K to JESD21-C
>> (see http://www.jedec.org/download/search/4_01_02_11R18.pdf).
>>
>> Warning:  I'm no perl expert.  The code I've written is probably ugly to
>> most of the readers of this list.  But it does work!
>
> Can you please provide a unified diff (as generated by diff -u)? Other
> diff formats are simply too fragile and/or too difficult for humans to
> read.

Sure.  See attached.

This version includes some additional decoding of bytes 60-76 for both 
unregistered and registered DIMMs, and module physical dimensions.


----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul-3orOWTcw9wBWk0Htik3J/w@public.gmane.org   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette-3r7Miqu9kMnR7s880joybQ@public.gmane.org |
----------------------------------------------------------------------

[-- Attachment #2: Type: TEXT/PLAIN, Size: 14439 bytes --]

--- decode-dimms.orig	2008-05-23 07:18:37.000000000 -0700
+++ decode-dimms	2008-11-25 04:02:44.000000000 -0800
@@ -408,7 +408,7 @@
 
 sub tns($) # print a time in ns
 {
-	return sprintf("%3.2f ns", $_[0]);
+	return sprintf("%.3f ns", $_[0]);
 }
 
 # Parameter: bytes 0-63
@@ -870,6 +870,251 @@
 	       ($byte & 0x80 ? " - Self Refresh" : "");
 }
 
+sub ddr3_manufacturer(@)
+{
+	my @info = @_;
+	my @mfg_bytes = (0, 0, 0, 0, 0, 0, 0, 0);
+	my $i;
+	my $count = $info[0] & 127;
+	# We'll just ignore the high-order odd-parity bit in the count byte
+	for ($i = 0; $i < $count; $i++) {
+		$mfg_bytes[$i] = 0x7f;
+	}
+	$mfg_bytes[$count] = $info[1];
+	(my $mfg, my $extra) = manufacturer(@mfg_bytes);
+	return $mfg;
+}
+
+# Parameter: bytes 0-127
+sub decode_ddr3_sdram($)
+{
+	my $bytes = shift;
+	my $l;
+	my $temp;
+	my $ctime;
+
+	my @module_types = ("Undefined", "RDIMM", "UDIMM", "SO-DIMM",
+			    "Micro-DIMM", "Mini-RDIMM", "Mini-UDIMM");
+
+	printl "Module Type", ($bytes->[3] <= $#module_types) ?
+					$module_types[$bytes->[3]] :
+					sprint("Reserved (0x%.2X)", $bytes->[3]);
+
+# speed
+	prints "Memory Characteristics";
+
+	$l = "Fine time base";
+	my $dividend = ($bytes->[9] >> 4) & 15;
+	my $divisor  = $bytes->[9] & 15;
+	printl $l, sprintf("%.3f", $dividend / $divisor) . " ps";
+
+	$l = "Medium time base";
+	$dividend = $bytes->[10];
+	$divisor  = $bytes->[11];
+	my $mtb = $dividend / $divisor;
+	printl $l, tns($mtb);
+
+	$l = "Maximum module speed";
+	$ctime = $bytes->[12] * $mtb;
+	my $ddrclk = 2 * (1000 / $ctime);
+	my $tbits = 1 << (($bytes->[8] & 7) + 3);
+	my $pcclk = int ($ddrclk * $tbits / 8);
+	$ddrclk = int ($ddrclk);
+	printl $l, "${ddrclk}MHz (PC3-${pcclk})";
+
+# Size computation
+
+	my $cap =  ($bytes->[4]       & 15) + 28;
+	$cap   +=  ($bytes->[8]       & 7)  + 3;
+	$cap   -=  ($bytes->[7]       & 7)  + 2;
+	$cap   -= 20 + 3;
+	my $k   = (($bytes->[7] >> 3) & 31) + 1;
+	printl "Size", ((1 << $cap) * $k) . " MB";
+
+	printl "Banks x Rows x Columns x Bits",
+	       join(' x ', 1 << ((($bytes->[4] >> 4) &  7) +  3),
+			   ((($bytes->[5] >> 3) & 31) + 12),
+			   ( ($bytes->[5]       &  7) +  9),
+			   ( 1 << (($bytes->[8] &  7) + 3)) );
+	printl "Ranks", $k;
+
+	printl "SDRAM Device Width", (1 << (($bytes->[7] & 7) + 2))." bits";
+
+	my $taa;
+	my $trcd;
+	my $trp;
+	my $tras;
+
+	$taa  = int($bytes->[16] / $bytes->[12]);
+	$trcd = int($bytes->[18] / $bytes->[12]);
+	$trp  = int($bytes->[20] / $bytes->[12]);
+	$tras = int((($bytes->[21] >> 4) * 256 + $bytes->[22]) / $bytes->[12]);
+
+	printl "tCL-tRCD-tRP-tRAS", join("-", $taa, $trcd, $trp, $tras);
+
+# latencies
+	my $highestCAS = 0;
+	my %cas;
+	my $ii;
+	my $cas_sup = ($bytes->[15] << 8) + $bytes->[14];
+	for ($ii = 0; $ii < 15; $ii++) {
+		if ($cas_sup & (1 << $ii)) {
+			$highestCAS = $ii + 4;
+			$cas{$highestCAS}++;
+		}
+	}
+	printl "Supported CAS Latencies (tCL)", cas_latencies(keys %cas);
+
+# more timing information
+	prints "Timing Parameters" ;
+
+	printl "Minimum Write Recovery time (tWR)", tns($bytes->[17] * $mtb);
+	printl "Minimum Row Active to Row Active Delay (tRRD)",
+		tns($bytes->[19] * $mtb);
+	printl "Minimum Active to Auto-Refresh Delay (tRC)",
+		tns((((($bytes->[21] >> 4) & 15) << 8) + $bytes->[23]) * $mtb);
+	printl "Minimum Recovery Delay (tRFC)", 
+		tns((($bytes->[25] << 8) + $bytes->[24]) * $mtb);
+	printl "Minimum Write to Read CMD Delay (tWTR)",
+		tns($bytes->[26] * $mtb);
+	printl "Minimum Read to Pre-charge CMD Delay (tRTP)",
+		tns($bytes->[27] * $mtb);
+	printl "Minimum Four Activate Window Delay (tFAW)",
+		tns(((($bytes->[28] & 15) << 8) + $bytes->[29]) * $mtb);
+
+# miscellaneous stuff
+	prints "Optional Features";
+
+	my $volts = "1.5V";
+	if ($bytes->[6] & 1) {
+		$volts .= " tolerant";
+	}
+	if ($bytes->[6] & 2) {
+		$volts .= ", 1.35V ";
+	}
+	if ($bytes->[6] & 4) {
+		$volts .= ", 1.2X V";
+	}
+	printl "Operable voltages", $volts;
+	printl "RZQ/6 supported?", ($bytes->[30] & 1) ? "Yes" : "No";
+	printl "RZQ/7 supported?", ($bytes->[30] & 2) ? "Yes" : "No";
+	printl "DLL-Off Mode supported?", ($bytes->[30] & 128) ? "Yes" : "No";
+	printl "Operating temperature range", sprintf "0-%dC",
+		($bytes->[31] & 1) ? 95 : 85;
+	printl "Refresh Rate in extended temp range",
+		($bytes->[31] & 2) ? "2X" : "1X";
+	printl "Auto Self-Refresh?", ($bytes->[31] & 4) ? "Yes" : "No";
+	printl "On-Die Thermal Sensor readout?",
+		($bytes->[31] & 8) ? "Yes" : "No";
+	printl "Partial Array Self-Refresh?",
+		($bytes->[31] & 128) ? "Yes" : "No";
+	printl "Thermal Sensor Accuracy",
+		($bytes->[32] & 128) ? sprintf($bytes->[32] & 127) :
+					"Not implemented";
+	printl "SDRAM Device Type",
+		($bytes->[33] & 128) ? sprintf($bytes->[33] & 127) :
+					"Standard Monolithic";
+	if ($bytes->[3] >= 1 && $bytes->[3] <= 6) {
+
+		prints "Physical Characteristics";
+		printl "Module Height (mm)", ($bytes->[60] & 31) + 15;
+		printl "Module Thickness (mm)", sprintf("%d front, %d back",
+						($bytes->[61] & 15) + 1, 
+						(($bytes->[61] >> 4) & 15) +1);
+		printl "Module Width (mm)", ($bytes->[3] <= 2) ? 133.5 :
+					($bytes->[3] == 3) ? 67.6 : "TBD";
+
+		my @alphabet = ("A", "B", "C", "D", "E", "F", "G", "H", "J",
+				"K", "L", "M", "N", "P", "R", "T", "U", "V",
+				"W", "Y" );
+
+		my $ref = $bytes->[62] & 31;
+		my $ref_card;
+		if ($ref == 31) {
+			$ref_card = "ZZ";
+		} else {
+			if ($bytes->[62] & 128) {
+				$ref += 31;
+			}
+			if ($ref < $#alphabet) {
+				$ref_card = $alphabet[$ref];
+			} else {
+				my $ref1 = int($ref / $#alphabet);
+				$ref -= $#alphabet * $ref1;
+				$ref_card = $alphabet[$ref1] . $alphabet[$ref];
+			}
+		}
+		printl "Module Reference Card", $ref_card;
+	}
+	if ($bytes->[3] == 1 || $bytes->[3] == 5) {
+		prints "Registered DIMM";
+
+		my @rows = ("Undefined", 1, 2, 4);
+		printl "# DRAM Rows", $rows[($bytes->[63] >> 2) & 3];
+		printl "# Registers", $rows[$bytes->[63] & 3];
+		my @m_data = ($bytes->[65], $bytes->[66]);
+		printl "Register manufacturer", ddr3_manufacturer(@m_data[0..1]);
+		printl "Register device type",
+				(($bytes->[68] & 7) == 0) ? "SSTE32882" :
+					"Undefined";
+		printl "Register revision", sprintf("0x%.2X", $bytes->[67]);
+		printl "Heat spreader characteristics",
+				($bytes->[64] < 128) ? "Not incorporated" :
+					sprintf("%.2X", ($bytes->[64] & 127));
+		my $regs;
+		for (my $i = 0; $i < 8; $i++) {
+			$regs = sprintf("SSTE32882 RC%d/RC%d",
+					$i * 2, $i * 2 + 1);
+			printl $regs, sprintf("%.2X", $bytes->[$i + 69]);
+		}
+	}
+
+	prints "Manufacturer Data";
+
+	my @m_data = ($bytes->[117], $bytes->[118]);
+	printl "Module Manufacturer", ddr3_manufacturer(@m_data[0..1]);
+
+	@m_data = ($bytes->[148], $bytes->[149]);
+	printl "DRAM Manufacturer Code", ddr3_manufacturer(@m_data[0..1]);
+
+	$l = "Manufacturing Location";
+	$temp = (chr($bytes->[8]) =~ m/^[\w\d]$/) ? chr($bytes->[8])
+	      : sprintf("0x%.2X", $bytes->[8]);
+	printl $l, $temp;
+
+	$l = "Part Number";
+	$temp = "";
+	for (my $i = 128; $i <= 145; $i++) {
+		$temp .= chr($bytes->[$i]);
+	};
+	printl $l, $temp;
+
+	$l = "Revision";
+	$temp = sprintf("0x%02X%02X\n", $bytes->[146], $bytes->[147]);
+	printl $l, $temp;
+
+	$l = "Manufacturing Date";
+	# In theory the year and week are in BCD format, but
+	# this is not always true in practice :(
+	if (($bytes->[120] & 0xf0) <= 0x90
+	 && ($bytes->[120] & 0x0f) <= 0x09
+	 && ($bytes->[121] & 0xf0) <= 0x90
+	 && ($bytes->[121] & 0x0f) <= 0x09) {
+		# Note that this heuristic will break in year 2080
+		$temp = sprintf("%d%02X-W%02X\n",
+				$bytes->[120] >= 0x80 ? 19 : 20,
+				$bytes->[120], $bytes->[121]);
+	} else {
+		$temp = sprintf("0x%02X%02X\n", $bytes->[120], $bytes->[121]);
+	}
+	printl $l, $temp;
+
+	$l = "Assembly Serial Number";
+	$temp = sprintf("0x%02X%02X%02X%02X\n", $bytes->[122], $bytes->[123],
+		$bytes->[124], $bytes->[125]);
+	printl $l, $temp;
+}
+
 # Parameter: bytes 0-63
 sub decode_ddr2_sdram($)
 {
@@ -1061,6 +1306,7 @@
 	"DDR2 SDRAM"	=> \&decode_ddr2_sdram,
 	"Direct Rambus"	=> \&decode_direct_rambus,
 	"Rambus"	=> \&decode_rambus,
+	"DDR3 SDRAM"	=> \&decode_ddr3_sdram,
 );
 
 # Parameter: bytes 64-127
@@ -1179,6 +1425,32 @@
 	return @bytes;
 }
 
+sub readfullspd($$) # reads all bytes from SPD-EEPROM
+{
+	my ($size, $dimm_i) = @_;
+	my @bytes;
+	if ($use_hexdump) {
+		@bytes = read_hexdump($dimm_i);
+		return @bytes[0..$size];
+	} elsif ($use_sysfs) {
+		# Kernel 2.6 with sysfs
+		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
+			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
+		binmode HANDLE;
+		sysseek(HANDLE, 0, SEEK_SET);
+		sysread(HANDLE, my $eeprom, $size);
+		close HANDLE;
+		@bytes = unpack(sprintf("C%d", $size), $eeprom);
+	} else {
+		# Kernel 2.4 with procfs
+		for my $i (0 .. $size/16) {
+			my $hexoff = sprintf('%02x', $i * 16);
+			push @bytes, split(" ", `cat /proc/sys/dev/sensors/$dimm_i/$hexoff`);
+		}
+	}
+	return @bytes;
+}
+
 # Parse command-line
 foreach (@ARGV) {
 	if ($_ eq '-h' || $_ eq '--help') {
@@ -1271,9 +1543,6 @@
 		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
 		$dimm_checksum &= 0xff;
 
-		next unless $bytes[63] == $dimm_checksum || $opt_igncheck;
-		$dimm_count++;
-
 		print "<b><u>" if $opt_html;
 		printl2 "\n\nDecoding EEPROM",
 		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
@@ -1292,32 +1561,87 @@
 # Decode first 3 bytes (0-2)
 		prints "SPD EEPROM Information";
 
-		my $l = "EEPROM Checksum of bytes 0-62";
-		printl $l, ($bytes[63] == $dimm_checksum ?
-			sprintf("OK (0x%.2X)", $bytes[63]):
-			sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
-				$bytes[63], $dimm_checksum));
-
 		# Simple heuristic to detect Rambus
-		my $is_rambus = $bytes[0] < 4;
+		my $spdsize;
+		my $written;
+		my @used = ( 0, 128, 176, 256);
+		my @sizes = ( "Undefined", 256);
+		my $is_rambus = ($bytes[0] < 4 && $bytes[0] >= 0);
+
+		if ($is_rambus || $bytes[2] <= 8) {
+			my $l = "EEPROM Checksum of bytes 0-62";
+			printl $l, ($bytes[63] == $dimm_checksum ?
+				sprintf("OK (0x%.2X)", $bytes[63]):
+				sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
+					$bytes[63], $dimm_checksum));
+			next unless $bytes[63] == $dimm_checksum ||
+				    $opt_igncheck;
+			$dimm_count++;
+			if ($is_rambus) {
+				$spdsize = 0;
+				$written = 0;
+			} else {
+				$written = $bytes[0];
+				if ($bytes[1] <= 14) {
+					$spdsize = 1 << $bytes[1];
+				} else { $spdsize = "ERROR!"; }
+			}
+		} else {
+			if ((($bytes[0] >> 4) & 7) <= $#sizes) {
+				$spdsize = $sizes[($bytes[0] >> 4) & 7];
+			} else { $spdsize = sprintf("Reserved (0x%X)",
+					($bytes[0] >> 4) & 7); }
+
+			if (($bytes[0] & 15) <= $#used) {
+				$written = $used[$bytes[0] & 15];
+			} else { $written = sprintf("Reserved (0x%X)",
+					$bytes[0] & 15); }
+
+			@bytes = readfullspd($written, $dimm_list[$i]);
+			my $dimm_crc = 0;
+			my $crc_cover = $bytes[0] & 0x80 ? 116 : 125;
+			my $crc_ptr = 0;
+			my $crc_bit;
+			while ($crc_ptr <= $crc_cover) {
+				$dimm_crc = $dimm_crc ^ ($bytes[$crc_ptr] << 8);
+				for ($crc_bit = 0; $crc_bit < 8; $crc_bit++) {
+					if ($dimm_crc & 0x8000) {
+						$dimm_crc = ($dimm_crc << 1) ^
+							0x1021;
+					} else {
+						$dimm_crc = $dimm_crc << 1
+					}
+				}
+				$crc_ptr++;
+			}
+			$dimm_crc = $dimm_crc & 0xffff;
+
+			my $l = "EEPROM CRC of bytes 0-" .
+				sprintf("%d", $crc_cover);
+			my $crc_calc = $bytes[127] << 8 | $bytes[126];
+			printl $l, ($dimm_crc == $crc_calc)?
+				sprintf("OK (0x%.4X)", $dimm_crc):
+				sprintf("Bad\n(found 0x%.4X, calculated 0x%.4X)\n",
+					$crc_calc, $dimm_crc);
+			next unless $crc_calc == $dimm_crc || $opt_igncheck;
+			$dimm_count++;
+		}
+
 		my $temp;
 		if ($is_rambus) {
 			if ($bytes[0] == 1) { $temp = "0.7"; }
 			elsif ($bytes[0] == 2) { $temp = "1.0"; }
-			elsif ($bytes[0] == 0 || $bytes[0] == 255) { $temp = "Invalid"; }
-			else { $temp = "Reserved"; }
+			elsif ($bytes[0] == 0 || $bytes[0] == 255) {
+				$temp = "Invalid";
+			} else { $temp = "Reserved"; }
 			printl "SPD Revision", $temp;
-		} else {
-			printl "# of bytes written to SDRAM EEPROM",
-			       $bytes[0];
+		}
+		if ($spdsize != 0) {
+			printl "# of bytes written to SDRAM EEPROM", $written;
 		}
 
-		$l = "Total number of bytes in EEPROM";
-		if ($bytes[1] <= 14) {
-			printl $l, 2**$bytes[1];
-		} elsif ($bytes[1] == 0) {
-			printl $l, "RFU";
-		} else { printl $l, "ERROR!"; }
+		my $l = "Total number of bytes in EEPROM";
+		printl $l, $spdsize;
 
 		$l = "Fundamental Memory type";
 		my $type = "Unknown";
@@ -1325,14 +1649,14 @@
 			if ($bytes[2] == 1) { $type = "Direct Rambus"; }
 			elsif ($bytes[2] == 17) { $type = "Rambus"; }
 		} else {
-			if ($bytes[2] == 1) { $type = "FPM DRAM"; }
-			elsif ($bytes[2] == 2) { $type = "EDO"; }
-			elsif ($bytes[2] == 3) { $type = "Pipelined Nibble"; }
-			elsif ($bytes[2] == 4) { $type = "SDR SDRAM"; }
-			elsif ($bytes[2] == 5) { $type = "Multiplexed ROM"; }
-			elsif ($bytes[2] == 6) { $type = "DDR SGRAM"; }
-			elsif ($bytes[2] == 7) { $type = "DDR SDRAM"; }
-			elsif ($bytes[2] == 8) { $type = "DDR2 SDRAM"; }
+			my @types = ("Reserved", "FPM DRAM", "EDO",
+				"Pipelined Nibble", "SDR DRAM",
+				"Multiplexed ROM", "DDR SGRAM", "DDR SDRAM",
+				"DDR2 SDRAM", "FB_DIMM", "FB-DIMM PROBE",
+				"DDR3 SDRAM");
+			if ($bytes[2] <= $#types) {
+				$type = $types[$bytes[2]];
+			} else { $type = $types[0]; }
 		}
 		printl $l, $type;
 
@@ -1340,10 +1664,19 @@
 		$decode_callback{$type}->(\@bytes)
 			if exists $decode_callback{$type};
 
+		# DDR3 Manufacturer info is already decoded
+		# (It's NOT common!)
+
+		next if ($type eq "DDR3 SDRAM");
+
 # Decode next 35 bytes (64-98, common to all memory types)
 		prints "Manufacturing Information";
 
-		@bytes = readspd64(64, $dimm_list[$i]);
+		if ($#bytes == 63) {
+			@bytes = readspd64(64, $dimm_list[$i]);
+		} else {
+			@bytes = @bytes[64..$#bytes];
+		}
 
 		$l = "Manufacturer";
 		# $extra is a reference to an array containing up to

Re: Preliminary support for DDR3 in decode-dimms (fwd)

[Jean Delvare]

Hi Paul,

Sorry for the delay, I wanted to get i2c-tools 3.0.2 released before
making significant changes to decode-dimms.

On Tue, 25 Nov 2008 04:10:02 -0800 (PST), Paul Goyette wrote:
> The attached diffs provide some preliminary decode of the DDR3 SPD...
> This work is based on the recently-published Appendix K to JESD21-C
> (see http://www.jedec.org/download/search/4_01_02_11R18.pdf).
>
> Warning:  I'm no perl expert.  The code I've written is probably ugly to
> most of the readers of this list.  But it does work!
> (...)
> This version includes some additional decoding of bytes 60-76 for both 
> unregistered and registered DIMMs, and module physical dimensions.

This patch is relatively large and I admit I have a hard time reviewing
it, especially due to the lack of a detailed checklist of what you had
to change and why, and lack of comments in your code. I don't care
about the ddr3-specific functions (ddr3_manufacturer and
decode_ddr3_sdram), this is brand new code so it can't cause a
regression. What worries me are the changes to the common code.

Apparently you had to make some changes because assumptions which were
correct up to DDR2 are no longer correct for DDR3. Some of these
changes look like hacks to me (for example, the readfullspd function
and the conditionals it adds to the common code). I really would prefer
that we first clean up the script to get rid of improper assumptions,
and only after this is done, add support for DDR3. That way I will have
two medium patches to review rather than a single large one, and the
second patch should be pretty straightforward. This will be way easier
for me.

For example, if the manufacturing information is no longer common to
all memory types, then let's move it to a separate function. If
checksum handling is type-specific, then let's move it to a separate
function as well. I really don't want the main loop to grow
indefinitely.

Random comments and questions on your code:

> --- decode-dimms.orig	2008-05-23 07:18:37.000000000 -0700
> +++ decode-dimms	2008-11-25 04:02:44.000000000 -0800
> @@ -408,7 +408,7 @@
>  
>  sub tns($) # print a time in ns
>  {
> -	return sprintf("%3.2f ns", $_[0]);
> +	return sprintf("%.3f ns", $_[0]);
>  }
>  
>  # Parameter: bytes 0-63
> @@ -870,6 +870,251 @@
>  	       ($byte & 0x80 ? " - Self Refresh" : "");
>  }
>  
> +sub ddr3_manufacturer(@)
> +{
> +	my @info = @_;
> +	my @mfg_bytes = (0, 0, 0, 0, 0, 0, 0, 0);
> +	my $i;
> +	my $count = $info[0] & 127;
> +	# We'll just ignore the high-order odd-parity bit in the count byte
> +	for ($i = 0; $i < $count; $i++) {
> +		$mfg_bytes[$i] = 0x7f;
> +	}
> +	$mfg_bytes[$count] = $info[1];
> +	(my $mfg, my $extra) = manufacturer(@mfg_bytes);
> +	return $mfg;
> +}

Here you reformat the manufacturer code from the new (compact) to the
old (stupid) format. A comment explaining this would have been welcome.
But actually, this looks a bit artificial to me. Don't you think it
would be easier and more efficient to decode the new format directly?

> +
> +# Parameter: bytes 0-127
> +sub decode_ddr3_sdram($)
> +{
> +	my $bytes = shift;
> +	my $l;
> +	my $temp;
> +	my $ctime;
> +
> +	my @module_types = ("Undefined", "RDIMM", "UDIMM", "SO-DIMM",
> +			    "Micro-DIMM", "Mini-RDIMM", "Mini-UDIMM");
> +
> +	printl "Module Type", ($bytes->[3] <= $#module_types) ?
> +					$module_types[$bytes->[3]] :
> +					sprint("Reserved (0x%.2X)", $bytes->[3]);
> +
> +# speed
> +	prints "Memory Characteristics";
> +
> +	$l = "Fine time base";
> +	my $dividend = ($bytes->[9] >> 4) & 15;
> +	my $divisor  = $bytes->[9] & 15;
> +	printl $l, sprintf("%.3f", $dividend / $divisor) . " ps";
> +
> +	$l = "Medium time base";
> +	$dividend = $bytes->[10];
> +	$divisor  = $bytes->[11];
> +	my $mtb = $dividend / $divisor;
> +	printl $l, tns($mtb);
> +
> +	$l = "Maximum module speed";
> +	$ctime = $bytes->[12] * $mtb;
> +	my $ddrclk = 2 * (1000 / $ctime);
> +	my $tbits = 1 << (($bytes->[8] & 7) + 3);
> +	my $pcclk = int ($ddrclk * $tbits / 8);
> +	$ddrclk = int ($ddrclk);
> +	printl $l, "${ddrclk}MHz (PC3-${pcclk})";
> +
> +# Size computation
> +
> +	my $cap =  ($bytes->[4]       & 15) + 28;
> +	$cap   +=  ($bytes->[8]       & 7)  + 3;
> +	$cap   -=  ($bytes->[7]       & 7)  + 2;
> +	$cap   -= 20 + 3;
> +	my $k   = (($bytes->[7] >> 3) & 31) + 1;
> +	printl "Size", ((1 << $cap) * $k) . " MB";
> +
> +	printl "Banks x Rows x Columns x Bits",
> +	       join(' x ', 1 << ((($bytes->[4] >> 4) &  7) +  3),
> +			   ((($bytes->[5] >> 3) & 31) + 12),
> +			   ( ($bytes->[5]       &  7) +  9),
> +			   ( 1 << (($bytes->[8] &  7) + 3)) );
> +	printl "Ranks", $k;
> +
> +	printl "SDRAM Device Width", (1 << (($bytes->[7] & 7) + 2))." bits";
> +
> +	my $taa;
> +	my $trcd;
> +	my $trp;
> +	my $tras;
> +
> +	$taa  = int($bytes->[16] / $bytes->[12]);
> +	$trcd = int($bytes->[18] / $bytes->[12]);
> +	$trp  = int($bytes->[20] / $bytes->[12]);
> +	$tras = int((($bytes->[21] >> 4) * 256 + $bytes->[22]) / $bytes->[12]);
> +
> +	printl "tCL-tRCD-tRP-tRAS", join("-", $taa, $trcd, $trp, $tras);
> +
> +# latencies
> +	my $highestCAS = 0;
> +	my %cas;
> +	my $ii;
> +	my $cas_sup = ($bytes->[15] << 8) + $bytes->[14];
> +	for ($ii = 0; $ii < 15; $ii++) {
> +		if ($cas_sup & (1 << $ii)) {
> +			$highestCAS = $ii + 4;
> +			$cas{$highestCAS}++;
> +		}
> +	}
> +	printl "Supported CAS Latencies (tCL)", cas_latencies(keys %cas);
> +
> +# more timing information
> +	prints "Timing Parameters" ;
> +
> +	printl "Minimum Write Recovery time (tWR)", tns($bytes->[17] * $mtb);
> +	printl "Minimum Row Active to Row Active Delay (tRRD)",
> +		tns($bytes->[19] * $mtb);
> +	printl "Minimum Active to Auto-Refresh Delay (tRC)",
> +		tns((((($bytes->[21] >> 4) & 15) << 8) + $bytes->[23]) * $mtb);
> +	printl "Minimum Recovery Delay (tRFC)", 
> +		tns((($bytes->[25] << 8) + $bytes->[24]) * $mtb);
> +	printl "Minimum Write to Read CMD Delay (tWTR)",
> +		tns($bytes->[26] * $mtb);
> +	printl "Minimum Read to Pre-charge CMD Delay (tRTP)",
> +		tns($bytes->[27] * $mtb);
> +	printl "Minimum Four Activate Window Delay (tFAW)",
> +		tns(((($bytes->[28] & 15) << 8) + $bytes->[29]) * $mtb);
> +
> +# miscellaneous stuff
> +	prints "Optional Features";
> +
> +	my $volts = "1.5V";
> +	if ($bytes->[6] & 1) {
> +		$volts .= " tolerant";
> +	}
> +	if ($bytes->[6] & 2) {
> +		$volts .= ", 1.35V ";
> +	}
> +	if ($bytes->[6] & 4) {
> +		$volts .= ", 1.2X V";
> +	}
> +	printl "Operable voltages", $volts;
> +	printl "RZQ/6 supported?", ($bytes->[30] & 1) ? "Yes" : "No";
> +	printl "RZQ/7 supported?", ($bytes->[30] & 2) ? "Yes" : "No";
> +	printl "DLL-Off Mode supported?", ($bytes->[30] & 128) ? "Yes" : "No";
> +	printl "Operating temperature range", sprintf "0-%dC",
> +		($bytes->[31] & 1) ? 95 : 85;
> +	printl "Refresh Rate in extended temp range",
> +		($bytes->[31] & 2) ? "2X" : "1X";
> +	printl "Auto Self-Refresh?", ($bytes->[31] & 4) ? "Yes" : "No";
> +	printl "On-Die Thermal Sensor readout?",
> +		($bytes->[31] & 8) ? "Yes" : "No";
> +	printl "Partial Array Self-Refresh?",
> +		($bytes->[31] & 128) ? "Yes" : "No";
> +	printl "Thermal Sensor Accuracy",
> +		($bytes->[32] & 128) ? sprintf($bytes->[32] & 127) :
> +					"Not implemented";
> +	printl "SDRAM Device Type",
> +		($bytes->[33] & 128) ? sprintf($bytes->[33] & 127) :
> +					"Standard Monolithic";
> +	if ($bytes->[3] >= 1 && $bytes->[3] <= 6) {
> +
> +		prints "Physical Characteristics";
> +		printl "Module Height (mm)", ($bytes->[60] & 31) + 15;
> +		printl "Module Thickness (mm)", sprintf("%d front, %d back",
> +						($bytes->[61] & 15) + 1, 
> +						(($bytes->[61] >> 4) & 15) +1);
> +		printl "Module Width (mm)", ($bytes->[3] <= 2) ? 133.5 :
> +					($bytes->[3] == 3) ? 67.6 : "TBD";
> +
> +		my @alphabet = ("A", "B", "C", "D", "E", "F", "G", "H", "J",
> +				"K", "L", "M", "N", "P", "R", "T", "U", "V",
> +				"W", "Y" );
> +
> +		my $ref = $bytes->[62] & 31;
> +		my $ref_card;
> +		if ($ref == 31) {
> +			$ref_card = "ZZ";
> +		} else {
> +			if ($bytes->[62] & 128) {
> +				$ref += 31;
> +			}
> +			if ($ref < $#alphabet) {
> +				$ref_card = $alphabet[$ref];
> +			} else {
> +				my $ref1 = int($ref / $#alphabet);
> +				$ref -= $#alphabet * $ref1;
> +				$ref_card = $alphabet[$ref1] . $alphabet[$ref];
> +			}
> +		}
> +		printl "Module Reference Card", $ref_card;
> +	}
> +	if ($bytes->[3] == 1 || $bytes->[3] == 5) {
> +		prints "Registered DIMM";
> +
> +		my @rows = ("Undefined", 1, 2, 4);
> +		printl "# DRAM Rows", $rows[($bytes->[63] >> 2) & 3];
> +		printl "# Registers", $rows[$bytes->[63] & 3];
> +		my @m_data = ($bytes->[65], $bytes->[66]);
> +		printl "Register manufacturer", ddr3_manufacturer(@m_data[0..1]);
> +		printl "Register device type",
> +				(($bytes->[68] & 7) == 0) ? "SSTE32882" :
> +					"Undefined";
> +		printl "Register revision", sprintf("0x%.2X", $bytes->[67]);
> +		printl "Heat spreader characteristics",
> +				($bytes->[64] < 128) ? "Not incorporated" :
> +					sprintf("%.2X", ($bytes->[64] & 127));
> +		my $regs;
> +		for (my $i = 0; $i < 8; $i++) {
> +			$regs = sprintf("SSTE32882 RC%d/RC%d",
> +					$i * 2, $i * 2 + 1);
> +			printl $regs, sprintf("%.2X", $bytes->[$i + 69]);
> +		}
> +	}
> +
> +	prints "Manufacturer Data";
> +
> +	my @m_data = ($bytes->[117], $bytes->[118]);
> +	printl "Module Manufacturer", ddr3_manufacturer(@m_data[0..1]);
> +
> +	@m_data = ($bytes->[148], $bytes->[149]);
> +	printl "DRAM Manufacturer Code", ddr3_manufacturer(@m_data[0..1]);
> +
> +	$l = "Manufacturing Location";
> +	$temp = (chr($bytes->[8]) =~ m/^[\w\d]$/) ? chr($bytes->[8])
> +	      : sprintf("0x%.2X", $bytes->[8]);
> +	printl $l, $temp;
> +
> +	$l = "Part Number";
> +	$temp = "";
> +	for (my $i = 128; $i <= 145; $i++) {
> +		$temp .= chr($bytes->[$i]);
> +	};
> +	printl $l, $temp;
> +
> +	$l = "Revision";
> +	$temp = sprintf("0x%02X%02X\n", $bytes->[146], $bytes->[147]);
> +	printl $l, $temp;
> +
> +	$l = "Manufacturing Date";
> +	# In theory the year and week are in BCD format, but
> +	# this is not always true in practice :(
> +	if (($bytes->[120] & 0xf0) <= 0x90
> +	 && ($bytes->[120] & 0x0f) <= 0x09
> +	 && ($bytes->[121] & 0xf0) <= 0x90
> +	 && ($bytes->[121] & 0x0f) <= 0x09) {
> +		# Note that this heuristic will break in year 2080
> +		$temp = sprintf("%d%02X-W%02X\n",
> +				$bytes->[120] >= 0x80 ? 19 : 20,
> +				$bytes->[120], $bytes->[121]);
> +	} else {
> +		$temp = sprintf("0x%02X%02X\n", $bytes->[120], $bytes->[121]);
> +	}
> +	printl $l, $temp;
> +
> +	$l = "Assembly Serial Number";
> +	$temp = sprintf("0x%02X%02X%02X%02X\n", $bytes->[122], $bytes->[123],
> +		$bytes->[124], $bytes->[125]);
> +	printl $l, $temp;
> +}

I won't comment on this function, as it is new it can't break anything,
and it can be improved / fixed over time. Just one thing though:
placing this function in the middle of ddr2-specific functions is quite
confusing, it you have been much clearer to put it afterwards.

> +
>  # Parameter: bytes 0-63
>  sub decode_ddr2_sdram($)
>  {
> @@ -1061,6 +1306,7 @@
>  	"DDR2 SDRAM"	=> \&decode_ddr2_sdram,
>  	"Direct Rambus"	=> \&decode_direct_rambus,
>  	"Rambus"	=> \&decode_rambus,
> +	"DDR3 SDRAM"	=> \&decode_ddr3_sdram,
>  );
>  
>  # Parameter: bytes 64-127
> @@ -1179,6 +1425,32 @@
>  	return @bytes;
>  }
>  
> +sub readfullspd($$) # reads all bytes from SPD-EEPROM
> +{
> +	my ($size, $dimm_i) = @_;
> +	my @bytes;
> +	if ($use_hexdump) {
> +		@bytes = read_hexdump($dimm_i);
> +		return @bytes[0..$size];
> +	} elsif ($use_sysfs) {
> +		# Kernel 2.6 with sysfs
> +		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
> +			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
> +		binmode HANDLE;
> +		sysseek(HANDLE, 0, SEEK_SET);
> +		sysread(HANDLE, my $eeprom, $size);
> +		close HANDLE;
> +		@bytes = unpack(sprintf("C%d", $size), $eeprom);
> +	} else {
> +		# Kernel 2.4 with procfs
> +		for my $i (0 .. $size/16) {
> +			my $hexoff = sprintf('%02x', $i * 16);
> +			push @bytes, split(" ", `cat /proc/sys/dev/sensors/$dimm_i/$hexoff`);
> +		}
> +	}
> +	return @bytes;
> +}

As written above, I don't like this hack. This is fundamentally
duplicating the code of readspd64. If you can handle the DDR3 with
readspd64, please just do that. If not, then let's have a more generic
function which can handle all cases. But please do not duplicate the
code.

> +
>  # Parse command-line
>  foreach (@ARGV) {
>  	if ($_ eq '-h' || $_ eq '--help') {
> @@ -1271,9 +1543,6 @@
>  		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
>  		$dimm_checksum &= 0xff;
>  
> -		next unless $bytes[63] == $dimm_checksum || $opt_igncheck;
> -		$dimm_count++;
> -

That's not OK. You let the script print things when the checksum is
invalid, while it did not before. Your patch should not change the
behavior of the script!

>  		print "<b><u>" if $opt_html;
>  		printl2 "\n\nDecoding EEPROM",
>  		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
> @@ -1292,32 +1561,87 @@
>  # Decode first 3 bytes (0-2)
>  		prints "SPD EEPROM Information";
>  
> -		my $l = "EEPROM Checksum of bytes 0-62";
> -		printl $l, ($bytes[63] == $dimm_checksum ?
> -			sprintf("OK (0x%.2X)", $bytes[63]):
> -			sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
> -				$bytes[63], $dimm_checksum));
> -
>  		# Simple heuristic to detect Rambus

This comment...

> -		my $is_rambus = $bytes[0] < 4;
> +		my $spdsize;
> +		my $written;
> +		my @used = ( 0, 128, 176, 256);
> +		my @sizes = ( "Undefined", 256);
> +		my $is_rambus = ($bytes[0] < 4 && $bytes[0] >= 0);

... if now far away from the code it applied to; confusing. I also do
not understand why you added this second condition... $bytes[0] is a
value between 0 and 255, how could it NOT be >= 0?

> +
> +		if ($is_rambus || $bytes[2] <= 8) {

The second condition needs clarification.

> +			my $l = "EEPROM Checksum of bytes 0-62";
> +			printl $l, ($bytes[63] == $dimm_checksum ?
> +				sprintf("OK (0x%.2X)", $bytes[63]):
> +				sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
> +					$bytes[63], $dimm_checksum));
> +			next unless $bytes[63] == $dimm_checksum ||
> +				    $opt_igncheck;
> +			$dimm_count++;
> +			if ($is_rambus) {
> +				$spdsize = 0;
> +				$written = 0;
> +			} else {
> +				$written = $bytes[0];
> +				if ($bytes[1] <= 14) {
> +					$spdsize = 1 << $bytes[1];
> +				} else { $spdsize = "ERROR!"; }
> +			}
> +		} else {
> +			if ((($bytes[0] >> 4) & 7) <= $#sizes) {
> +				$spdsize = $sizes[($bytes[0] >> 4) & 7];
> +			} else { $spdsize = sprintf("Reserved (0x%X)",
> +					($bytes[0] >> 4) & 7); }
> +
> +			if (($bytes[0] & 15) <= $#used) {
> +				$written = $used[$bytes[0] & 15];
> +			} else { $written = sprintf("Reserved (0x%X)",
> +					$bytes[0] & 15); }
> +
> +			@bytes = readfullspd($written, $dimm_list[$i]);
> +			my $dimm_crc = 0;
> +			my $crc_cover = $bytes[0] & 0x80 ? 116 : 125;
> +			my $crc_ptr = 0;
> +			my $crc_bit;
> +			while ($crc_ptr <= $crc_cover) {
> +				$dimm_crc = $dimm_crc ^ ($bytes[$crc_ptr] << 8);
> +				for ($crc_bit = 0; $crc_bit < 8; $crc_bit++) {
> +					if ($dimm_crc & 0x8000) {
> +						$dimm_crc = ($dimm_crc << 1) ^
> +							0x1021;
> +					} else {
> +						$dimm_crc = $dimm_crc << 1
> +					}
> +				}
> +				$crc_ptr++;
> +			}
> +			$dimm_crc = $dimm_crc & 0xffff;
> +
> +			my $l = "EEPROM CRC of bytes 0-" .
> +				sprintf("%d", $crc_cover);
> +			my $crc_calc = $bytes[127] << 8 | $bytes[126];
> +			printl $l, ($dimm_crc == $crc_calc)?
> +				sprintf("OK (0x%.4X)", $dimm_crc):
> +				sprintf("Bad\n(found 0x%.4X, calculated 0x%.4X)\n",
> +					$crc_calc, $dimm_crc);
> +			next unless $crc_calc == $dimm_crc || $opt_igncheck;
> +			$dimm_count++;
> +		}

As said before, this all should go to a separate function. With
comments explaining what you do.

> +
>  		my $temp;
>  		if ($is_rambus) {
>  			if ($bytes[0] == 1) { $temp = "0.7"; }
>  			elsif ($bytes[0] == 2) { $temp = "1.0"; }
> -			elsif ($bytes[0] == 0 || $bytes[0] == 255) { $temp = "Invalid"; }
> -			else { $temp = "Reserved"; }
> +			elsif ($bytes[0] == 0 || $bytes[0] == 255) {
> +				$temp = "Invalid";
> +			} else { $temp = "Reserved"; }

Hum, gratuitous coding-style change? Please don't do this, this only
slows down reviews.

>  			printl "SPD Revision", $temp;
> -		} else {
> -			printl "# of bytes written to SDRAM EEPROM",
> -			       $bytes[0];
> +		}
> +		if ($spdsize != 0) {
> +			printl "# of bytes written to SDRAM EEPROM", $written;
>  		}
>  
> -		$l = "Total number of bytes in EEPROM";
> -		if ($bytes[1] <= 14) {
> -			printl $l, 2**$bytes[1];
> -		} elsif ($bytes[1] == 0) {
> -			printl $l, "RFU";
> -		} else { printl $l, "ERROR!"; }
> +		my $l = "Total number of bytes in EEPROM";
> +		printl $l, $spdsize;
>  
>  		$l = "Fundamental Memory type";
>  		my $type = "Unknown";
> @@ -1325,14 +1649,14 @@
>  			if ($bytes[2] == 1) { $type = "Direct Rambus"; }
>  			elsif ($bytes[2] == 17) { $type = "Rambus"; }
>  		} else {
> -			if ($bytes[2] == 1) { $type = "FPM DRAM"; }
> -			elsif ($bytes[2] == 2) { $type = "EDO"; }
> -			elsif ($bytes[2] == 3) { $type = "Pipelined Nibble"; }
> -			elsif ($bytes[2] == 4) { $type = "SDR SDRAM"; }
> -			elsif ($bytes[2] == 5) { $type = "Multiplexed ROM"; }
> -			elsif ($bytes[2] == 6) { $type = "DDR SGRAM"; }
> -			elsif ($bytes[2] == 7) { $type = "DDR SDRAM"; }
> -			elsif ($bytes[2] == 8) { $type = "DDR2 SDRAM"; }
> +			my @types = ("Reserved", "FPM DRAM", "EDO",
> +				"Pipelined Nibble", "SDR DRAM",
> +				"Multiplexed ROM", "DDR SGRAM", "DDR SDRAM",
> +				"DDR2 SDRAM", "FB_DIMM", "FB-DIMM PROBE",
> +				"DDR3 SDRAM");
> +			if ($bytes[2] <= $#types) {
> +				$type = $types[$bytes[2]];
> +			} else { $type = $types[0]; }

While I agree this change is a good idea, it should not be part of this
DDR3 patch. Especially when it breaks the SDRAM case! You changed the
"SDR SDRAM" string to SDR DRAM", while we do use these strings as hash
keys later in the script.

>  		}
>  		printl $l, $type;
>  
> @@ -1340,10 +1664,19 @@
>  		$decode_callback{$type}->(\@bytes)
>  			if exists $decode_callback{$type};
>  
> +		# DDR3 Manufacturer info is already decoded
> +		# (It's NOT common!)
> +
> +		next if ($type eq "DDR3 SDRAM");
> +

This hack breaks the html output mode.

>  # Decode next 35 bytes (64-98, common to all memory types)
>  		prints "Manufacturing Information";
>  
> -		@bytes = readspd64(64, $dimm_list[$i]);
> +		if ($#bytes == 63) {
> +			@bytes = readspd64(64, $dimm_list[$i]);
> +		} else {
> +			@bytes = @bytes[64..$#bytes];
> +		}

And this shouldn't be needed: if you reach this part of the code, it
means you're not in the DDR3 case, so you can't have bytes 64-127 in
the @bytes array yet.

>  
>  		$l = "Manufacturer";
>  		# $extra is a reference to an array containing up to

So, as I wrote before, let's clear up the script from its improper
assumptions first. I can help with this if you don't have the time, but
you must tell me what needs to be done and why.

Thanks,
-- 
Jean Delvare

Re: Preliminary support for DDR3 in decode-dimms (fwd)

[Paul Goyette]

On Mon, 8 Dec 2008, Jean Delvare wrote:

> Hi Paul,
>
> Sorry for the delay, I wanted to get i2c-tools 3.0.2 released before
> making significant changes to decode-dimms.
>
> On Tue, 25 Nov 2008 04:10:02 -0800 (PST), Paul Goyette wrote:
>> The attached diffs provide some preliminary decode of the DDR3 SPD...
>> This work is based on the recently-published Appendix K to JESD21-C
>> (see http://www.jedec.org/download/search/4_01_02_11R18.pdf).
>>
>> Warning:  I'm no perl expert.  The code I've written is probably ugly to
>> most of the readers of this list.  But it does work!
>> (...)
>> This version includes some additional decoding of bytes 60-76 for both
>> unregistered and registered DIMMs, and module physical dimensions.
>
> This patch is relatively large and I admit I have a hard time reviewing
> it, especially due to the lack of a detailed checklist of what you had
> to change and why, and lack of comments in your code. I don't care
> about the ddr3-specific functions (ddr3_manufacturer and
> decode_ddr3_sdram), this is brand new code so it can't cause a
> regression. What worries me are the changes to the common code.
>
> Apparently you had to make some changes because assumptions which were
> correct up to DDR2 are no longer correct for DDR3. Some of these
> changes look like hacks to me (for example, the readfullspd function
> and the conditionals it adds to the common code). I really would prefer
> that we first clean up the script to get rid of improper assumptions,
> and only after this is done, add support for DDR3. That way I will have
> two medium patches to review rather than a single large one, and the
> second patch should be pretty straightforward. This will be way easier
> for me.

For FB-DIMMs (which are previously supported) and DDR3, the old checksum 
over bytes 0-62 no longer exists;  instead, there is a CRC in bytes 126 
and 127 that covers a variable section of the SPD data.  readfullspd was 
used to read the entire data, rather than just 0-63.  It would not be 
too difficult to replace the old (multiple) call to read64.

> For example, if the manufacturing information is no longer common to
> all memory types, then let's move it to a separate function. If
> checksum handling is type-specific, then let's move it to a separate
> function as well. I really don't want the main loop to grow
> indefinitely.

There are really only two types of validation:

 	checksum for everything with type < FB-DIMM
 	CRC for everything >= FB-DIMM

I'm not sure it makes sense to add a specific call to each memory type's 
specific processing, especially since it would mean that checksum would 
not get checked for types that don't have a specific processing routine 
(ie, the ROMs).

Manufacturing information is indeed no longer completely common.

> Random comments and questions on your code:
>
>> --- decode-dimms.orig	2008-05-23 07:18:37.000000000 -0700
>> +++ decode-dimms	2008-11-25 04:02:44.000000000 -0800
>> @@ -408,7 +408,7 @@
>>
>>  sub tns($) # print a time in ns
>>  {
>> -	return sprintf("%3.2f ns", $_[0]);
>> +	return sprintf("%.3f ns", $_[0]);
>>  }
>>
>>  # Parameter: bytes 0-63
>> @@ -870,6 +870,251 @@
>>  	       ($byte & 0x80 ? " - Self Refresh" : "");
>>  }
>>
>> +sub ddr3_manufacturer(@)
>> +{
>> +	my @info = @_;
>> +	my @mfg_bytes = (0, 0, 0, 0, 0, 0, 0, 0);
>> +	my $i;
>> +	my $count = $info[0] & 127;
>> +	# We'll just ignore the high-order odd-parity bit in the count byte
>> +	for ($i = 0; $i < $count; $i++) {
>> +		$mfg_bytes[$i] = 0x7f;
>> +	}
>> +	$mfg_bytes[$count] = $info[1];
>> +	(my $mfg, my $extra) = manufacturer(@mfg_bytes);
>> +	return $mfg;
>> +}
>
> Here you reformat the manufacturer code from the new (compact) to the
> old (stupid) format. A comment explaining this would have been welcome.
> But actually, this looks a bit artificial to me. Don't you think it
> would be easier and more efficient to decode the new format directly?

I did not truly understand the formats here, so I simply built code that 
constructed the expected input for the existing routine.

>
>> +
>> +# Parameter: bytes 0-127
>> +sub decode_ddr3_sdram($)
>> +{
>> +	my $bytes = shift;
>> +	my $l;
>> +	my $temp;
>> +	my $ctime;
>> +
>> +	my @module_types = ("Undefined", "RDIMM", "UDIMM", "SO-DIMM",
>> +			    "Micro-DIMM", "Mini-RDIMM", "Mini-UDIMM");
>> +
>> +	printl "Module Type", ($bytes->[3] <= $#module_types) ?
>> +					$module_types[$bytes->[3]] :
>> +					sprint("Reserved (0x%.2X)", $bytes->[3]);
>> +
>> +# speed
>> +	prints "Memory Characteristics";
>> +
>> +	$l = "Fine time base";
>> +	my $dividend = ($bytes->[9] >> 4) & 15;
>> +	my $divisor  = $bytes->[9] & 15;
>> +	printl $l, sprintf("%.3f", $dividend / $divisor) . " ps";
>> +
>> +	$l = "Medium time base";
>> +	$dividend = $bytes->[10];
>> +	$divisor  = $bytes->[11];
>> +	my $mtb = $dividend / $divisor;
>> +	printl $l, tns($mtb);
>> +
>> +	$l = "Maximum module speed";
>> +	$ctime = $bytes->[12] * $mtb;
>> +	my $ddrclk = 2 * (1000 / $ctime);
>> +	my $tbits = 1 << (($bytes->[8] & 7) + 3);
>> +	my $pcclk = int ($ddrclk * $tbits / 8);
>> +	$ddrclk = int ($ddrclk);
>> +	printl $l, "${ddrclk}MHz (PC3-${pcclk})";
>> +
>> +# Size computation
>> +
>> +	my $cap =  ($bytes->[4]       & 15) + 28;
>> +	$cap   +=  ($bytes->[8]       & 7)  + 3;
>> +	$cap   -=  ($bytes->[7]       & 7)  + 2;
>> +	$cap   -= 20 + 3;
>> +	my $k   = (($bytes->[7] >> 3) & 31) + 1;
>> +	printl "Size", ((1 << $cap) * $k) . " MB";
>> +
>> +	printl "Banks x Rows x Columns x Bits",
>> +	       join(' x ', 1 << ((($bytes->[4] >> 4) &  7) +  3),
>> +			   ((($bytes->[5] >> 3) & 31) + 12),
>> +			   ( ($bytes->[5]       &  7) +  9),
>> +			   ( 1 << (($bytes->[8] &  7) + 3)) );
>> +	printl "Ranks", $k;
>> +
>> +	printl "SDRAM Device Width", (1 << (($bytes->[7] & 7) + 2))." bits";
>> +
>> +	my $taa;
>> +	my $trcd;
>> +	my $trp;
>> +	my $tras;
>> +
>> +	$taa  = int($bytes->[16] / $bytes->[12]);
>> +	$trcd = int($bytes->[18] / $bytes->[12]);
>> +	$trp  = int($bytes->[20] / $bytes->[12]);
>> +	$tras = int((($bytes->[21] >> 4) * 256 + $bytes->[22]) / $bytes->[12]);
>> +
>> +	printl "tCL-tRCD-tRP-tRAS", join("-", $taa, $trcd, $trp, $tras);
>> +
>> +# latencies
>> +	my $highestCAS = 0;
>> +	my %cas;
>> +	my $ii;
>> +	my $cas_sup = ($bytes->[15] << 8) + $bytes->[14];
>> +	for ($ii = 0; $ii < 15; $ii++) {
>> +		if ($cas_sup & (1 << $ii)) {
>> +			$highestCAS = $ii + 4;
>> +			$cas{$highestCAS}++;
>> +		}
>> +	}
>> +	printl "Supported CAS Latencies (tCL)", cas_latencies(keys %cas);
>> +
>> +# more timing information
>> +	prints "Timing Parameters" ;
>> +
>> +	printl "Minimum Write Recovery time (tWR)", tns($bytes->[17] * $mtb);
>> +	printl "Minimum Row Active to Row Active Delay (tRRD)",
>> +		tns($bytes->[19] * $mtb);
>> +	printl "Minimum Active to Auto-Refresh Delay (tRC)",
>> +		tns((((($bytes->[21] >> 4) & 15) << 8) + $bytes->[23]) * $mtb);
>> +	printl "Minimum Recovery Delay (tRFC)",
>> +		tns((($bytes->[25] << 8) + $bytes->[24]) * $mtb);
>> +	printl "Minimum Write to Read CMD Delay (tWTR)",
>> +		tns($bytes->[26] * $mtb);
>> +	printl "Minimum Read to Pre-charge CMD Delay (tRTP)",
>> +		tns($bytes->[27] * $mtb);
>> +	printl "Minimum Four Activate Window Delay (tFAW)",
>> +		tns(((($bytes->[28] & 15) << 8) + $bytes->[29]) * $mtb);
>> +
>> +# miscellaneous stuff
>> +	prints "Optional Features";
>> +
>> +	my $volts = "1.5V";
>> +	if ($bytes->[6] & 1) {
>> +		$volts .= " tolerant";
>> +	}
>> +	if ($bytes->[6] & 2) {
>> +		$volts .= ", 1.35V ";
>> +	}
>> +	if ($bytes->[6] & 4) {
>> +		$volts .= ", 1.2X V";
>> +	}
>> +	printl "Operable voltages", $volts;
>> +	printl "RZQ/6 supported?", ($bytes->[30] & 1) ? "Yes" : "No";
>> +	printl "RZQ/7 supported?", ($bytes->[30] & 2) ? "Yes" : "No";
>> +	printl "DLL-Off Mode supported?", ($bytes->[30] & 128) ? "Yes" : "No";
>> +	printl "Operating temperature range", sprintf "0-%dC",
>> +		($bytes->[31] & 1) ? 95 : 85;
>> +	printl "Refresh Rate in extended temp range",
>> +		($bytes->[31] & 2) ? "2X" : "1X";
>> +	printl "Auto Self-Refresh?", ($bytes->[31] & 4) ? "Yes" : "No";
>> +	printl "On-Die Thermal Sensor readout?",
>> +		($bytes->[31] & 8) ? "Yes" : "No";
>> +	printl "Partial Array Self-Refresh?",
>> +		($bytes->[31] & 128) ? "Yes" : "No";
>> +	printl "Thermal Sensor Accuracy",
>> +		($bytes->[32] & 128) ? sprintf($bytes->[32] & 127) :
>> +					"Not implemented";
>> +	printl "SDRAM Device Type",
>> +		($bytes->[33] & 128) ? sprintf($bytes->[33] & 127) :
>> +					"Standard Monolithic";
>> +	if ($bytes->[3] >= 1 && $bytes->[3] <= 6) {
>> +
>> +		prints "Physical Characteristics";
>> +		printl "Module Height (mm)", ($bytes->[60] & 31) + 15;
>> +		printl "Module Thickness (mm)", sprintf("%d front, %d back",
>> +						($bytes->[61] & 15) + 1,
>> +						(($bytes->[61] >> 4) & 15) +1);
>> +		printl "Module Width (mm)", ($bytes->[3] <= 2) ? 133.5 :
>> +					($bytes->[3] == 3) ? 67.6 : "TBD";
>> +
>> +		my @alphabet = ("A", "B", "C", "D", "E", "F", "G", "H", "J",
>> +				"K", "L", "M", "N", "P", "R", "T", "U", "V",
>> +				"W", "Y" );
>> +
>> +		my $ref = $bytes->[62] & 31;
>> +		my $ref_card;
>> +		if ($ref == 31) {
>> +			$ref_card = "ZZ";
>> +		} else {
>> +			if ($bytes->[62] & 128) {
>> +				$ref += 31;
>> +			}
>> +			if ($ref < $#alphabet) {
>> +				$ref_card = $alphabet[$ref];
>> +			} else {
>> +				my $ref1 = int($ref / $#alphabet);
>> +				$ref -= $#alphabet * $ref1;
>> +				$ref_card = $alphabet[$ref1] . $alphabet[$ref];
>> +			}
>> +		}
>> +		printl "Module Reference Card", $ref_card;
>> +	}
>> +	if ($bytes->[3] == 1 || $bytes->[3] == 5) {
>> +		prints "Registered DIMM";
>> +
>> +		my @rows = ("Undefined", 1, 2, 4);
>> +		printl "# DRAM Rows", $rows[($bytes->[63] >> 2) & 3];
>> +		printl "# Registers", $rows[$bytes->[63] & 3];
>> +		my @m_data = ($bytes->[65], $bytes->[66]);
>> +		printl "Register manufacturer", ddr3_manufacturer(@m_data[0..1]);
>> +		printl "Register device type",
>> +				(($bytes->[68] & 7) == 0) ? "SSTE32882" :
>> +					"Undefined";
>> +		printl "Register revision", sprintf("0x%.2X", $bytes->[67]);
>> +		printl "Heat spreader characteristics",
>> +				($bytes->[64] < 128) ? "Not incorporated" :
>> +					sprintf("%.2X", ($bytes->[64] & 127));
>> +		my $regs;
>> +		for (my $i = 0; $i < 8; $i++) {
>> +			$regs = sprintf("SSTE32882 RC%d/RC%d",
>> +					$i * 2, $i * 2 + 1);
>> +			printl $regs, sprintf("%.2X", $bytes->[$i + 69]);
>> +		}
>> +	}
>> +
>> +	prints "Manufacturer Data";
>> +
>> +	my @m_data = ($bytes->[117], $bytes->[118]);
>> +	printl "Module Manufacturer", ddr3_manufacturer(@m_data[0..1]);
>> +
>> +	@m_data = ($bytes->[148], $bytes->[149]);
>> +	printl "DRAM Manufacturer Code", ddr3_manufacturer(@m_data[0..1]);
>> +
>> +	$l = "Manufacturing Location";
>> +	$temp = (chr($bytes->[8]) =~ m/^[\w\d]$/) ? chr($bytes->[8])
>> +	      : sprintf("0x%.2X", $bytes->[8]);
>> +	printl $l, $temp;
>> +
>> +	$l = "Part Number";
>> +	$temp = "";
>> +	for (my $i = 128; $i <= 145; $i++) {
>> +		$temp .= chr($bytes->[$i]);
>> +	};
>> +	printl $l, $temp;
>> +
>> +	$l = "Revision";
>> +	$temp = sprintf("0x%02X%02X\n", $bytes->[146], $bytes->[147]);
>> +	printl $l, $temp;
>> +
>> +	$l = "Manufacturing Date";
>> +	# In theory the year and week are in BCD format, but
>> +	# this is not always true in practice :(
>> +	if (($bytes->[120] & 0xf0) <= 0x90
>> +	 && ($bytes->[120] & 0x0f) <= 0x09
>> +	 && ($bytes->[121] & 0xf0) <= 0x90
>> +	 && ($bytes->[121] & 0x0f) <= 0x09) {
>> +		# Note that this heuristic will break in year 2080
>> +		$temp = sprintf("%d%02X-W%02X\n",
>> +				$bytes->[120] >= 0x80 ? 19 : 20,
>> +				$bytes->[120], $bytes->[121]);
>> +	} else {
>> +		$temp = sprintf("0x%02X%02X\n", $bytes->[120], $bytes->[121]);
>> +	}
>> +	printl $l, $temp;
>> +
>> +	$l = "Assembly Serial Number";
>> +	$temp = sprintf("0x%02X%02X%02X%02X\n", $bytes->[122], $bytes->[123],
>> +		$bytes->[124], $bytes->[125]);
>> +	printl $l, $temp;
>> +}
>
> I won't comment on this function, as it is new it can't break anything,
> and it can be improved / fixed over time. Just one thing though:
> placing this function in the middle of ddr2-specific functions is quite
> confusing, it you have been much clearer to put it afterwards.

Placement is an error on mu part - I can certainly move it.

>
>> +
>>  # Parameter: bytes 0-63
>>  sub decode_ddr2_sdram($)
>>  {
>> @@ -1061,6 +1306,7 @@
>>  	"DDR2 SDRAM"	=> \&decode_ddr2_sdram,
>>  	"Direct Rambus"	=> \&decode_direct_rambus,
>>  	"Rambus"	=> \&decode_rambus,
>> +	"DDR3 SDRAM"	=> \&decode_ddr3_sdram,
>>  );
>>
>>  # Parameter: bytes 64-127
>> @@ -1179,6 +1425,32 @@
>>  	return @bytes;
>>  }
>>
>> +sub readfullspd($$) # reads all bytes from SPD-EEPROM
>> +{
>> +	my ($size, $dimm_i) = @_;
>> +	my @bytes;
>> +	if ($use_hexdump) {
>> +		@bytes = read_hexdump($dimm_i);
>> +		return @bytes[0..$size];
>> +	} elsif ($use_sysfs) {
>> +		# Kernel 2.6 with sysfs
>> +		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
>> +			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
>> +		binmode HANDLE;
>> +		sysseek(HANDLE, 0, SEEK_SET);
>> +		sysread(HANDLE, my $eeprom, $size);
>> +		close HANDLE;
>> +		@bytes = unpack(sprintf("C%d", $size), $eeprom);
>> +	} else {
>> +		# Kernel 2.4 with procfs
>> +		for my $i (0 .. $size/16) {
>> +			my $hexoff = sprintf('%02x', $i * 16);
>> +			push @bytes, split(" ", `cat /proc/sys/dev/sensors/$dimm_i/$hexoff`);
>> +		}
>> +	}
>> +	return @bytes;
>> +}
>
> As written above, I don't like this hack. This is fundamentally
> duplicating the code of readspd64. If you can handle the DDR3 with
> readspd64, please just do that. If not, then let's have a more generic
> function which can handle all cases. But please do not duplicate the
> code.

Sure.  I can do a readfull routine that works for everything.

>> +
>>  # Parse command-line
>>  foreach (@ARGV) {
>>  	if ($_ eq '-h' || $_ eq '--help') {
>> @@ -1271,9 +1543,6 @@
>>  		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
>>  		$dimm_checksum &= 0xff;
>>
>> -		next unless $bytes[63] == $dimm_checksum || $opt_igncheck;
>> -		$dimm_count++;
>> -
>
> That's not OK. You let the script print things when the checksum is
> invalid, while it did not before. Your patch should not change the
> behavior of the script!

I can fix this.  This was a remnant of the readfull vs read64 ...  I 
didn't want to readfull until after determining the memory type, since 
the type itself needs to be knonw before we know how much "full" is!

>
>>  		print "<b><u>" if $opt_html;
>>  		printl2 "\n\nDecoding EEPROM",
>>  		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
>> @@ -1292,32 +1561,87 @@
>>  # Decode first 3 bytes (0-2)
>>  		prints "SPD EEPROM Information";
>>
>> -		my $l = "EEPROM Checksum of bytes 0-62";
>> -		printl $l, ($bytes[63] == $dimm_checksum ?
>> -			sprintf("OK (0x%.2X)", $bytes[63]):
>> -			sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
>> -				$bytes[63], $dimm_checksum));
>> -
>>  		# Simple heuristic to detect Rambus
>
> This comment...
>
>> -		my $is_rambus = $bytes[0] < 4;
>> +		my $spdsize;
>> +		my $written;
>> +		my @used = ( 0, 128, 176, 256);
>> +		my @sizes = ( "Undefined", 256);
>> +		my $is_rambus = ($bytes[0] < 4 && $bytes[0] >= 0);
>
> ... if now far away from the code it applied to; confusing. I also do
> not understand why you added this second condition... $bytes[0] is a
> value between 0 and 255, how could it NOT be >= 0?

Shows my perl-ignorance.  :)  I am not a perl programmer, and couldn't 
tell if perl used signed or unsigned bytes!  :)

>> +
>> +		if ($is_rambus || $bytes[2] <= 8) {
>
> The second condition needs clarification.

Sure...

>> +			my $l = "EEPROM Checksum of bytes 0-62";
>> +			printl $l, ($bytes[63] == $dimm_checksum ?
>> +				sprintf("OK (0x%.2X)", $bytes[63]):
>> +				sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
>> +					$bytes[63], $dimm_checksum));
>> +			next unless $bytes[63] == $dimm_checksum ||
>> +				    $opt_igncheck;
>> +			$dimm_count++;
>> +			if ($is_rambus) {
>> +				$spdsize = 0;
>> +				$written = 0;
>> +			} else {
>> +				$written = $bytes[0];
>> +				if ($bytes[1] <= 14) {
>> +					$spdsize = 1 << $bytes[1];
>> +				} else { $spdsize = "ERROR!"; }
>> +			}
>> +		} else {
>> +			if ((($bytes[0] >> 4) & 7) <= $#sizes) {
>> +				$spdsize = $sizes[($bytes[0] >> 4) & 7];
>> +			} else { $spdsize = sprintf("Reserved (0x%X)",
>> +					($bytes[0] >> 4) & 7); }
>> +
>> +			if (($bytes[0] & 15) <= $#used) {
>> +				$written = $used[$bytes[0] & 15];
>> +			} else { $written = sprintf("Reserved (0x%X)",
>> +					$bytes[0] & 15); }
>> +
>> +			@bytes = readfullspd($written, $dimm_list[$i]);
>> +			my $dimm_crc = 0;
>> +			my $crc_cover = $bytes[0] & 0x80 ? 116 : 125;
>> +			my $crc_ptr = 0;
>> +			my $crc_bit;
>> +			while ($crc_ptr <= $crc_cover) {
>> +				$dimm_crc = $dimm_crc ^ ($bytes[$crc_ptr] << 8);
>> +				for ($crc_bit = 0; $crc_bit < 8; $crc_bit++) {
>> +					if ($dimm_crc & 0x8000) {
>> +						$dimm_crc = ($dimm_crc << 1) ^
>> +							0x1021;
>> +					} else {
>> +						$dimm_crc = $dimm_crc << 1
>> +					}
>> +				}
>> +				$crc_ptr++;
>> +			}
>> +			$dimm_crc = $dimm_crc & 0xffff;
>> +
>> +			my $l = "EEPROM CRC of bytes 0-" .
>> +				sprintf("%d", $crc_cover);
>> +			my $crc_calc = $bytes[127] << 8 | $bytes[126];
>> +			printl $l, ($dimm_crc == $crc_calc)?
>> +				sprintf("OK (0x%.4X)", $dimm_crc):
>> +				sprintf("Bad\n(found 0x%.4X, calculated 0x%.4X)\n",
>> +					$crc_calc, $dimm_crc);
>> +			next unless $crc_calc == $dimm_crc || $opt_igncheck;
>> +			$dimm_count++;
>> +		}
>
> As said before, this all should go to a separate function. With
> comments explaining what you do.

OK

>> +
>>  		my $temp;
>>  		if ($is_rambus) {
>>  			if ($bytes[0] == 1) { $temp = "0.7"; }
>>  			elsif ($bytes[0] == 2) { $temp = "1.0"; }
>> -			elsif ($bytes[0] == 0 || $bytes[0] == 255) { $temp = "Invalid"; }
>> -			else { $temp = "Reserved"; }
>> +			elsif ($bytes[0] == 0 || $bytes[0] == 255) {
>> +				$temp = "Invalid";
>> +			} else { $temp = "Reserved"; }
>
> Hum, gratuitous coding-style change? Please don't do this, this only
> slows down reviews.

Just trying to avoid wrapping of long lines.  Certainly not critical to 
making it work.

>>  			printl "SPD Revision", $temp;
>> -		} else {
>> -			printl "# of bytes written to SDRAM EEPROM",
>> -			       $bytes[0];
>> +		}
>> +		if ($spdsize != 0) {
>> +			printl "# of bytes written to SDRAM EEPROM", $written;
>>  		}
>>
>> -		$l = "Total number of bytes in EEPROM";
>> -		if ($bytes[1] <= 14) {
>> -			printl $l, 2**$bytes[1];
>> -		} elsif ($bytes[1] == 0) {
>> -			printl $l, "RFU";
>> -		} else { printl $l, "ERROR!"; }
>> +		my $l = "Total number of bytes in EEPROM";
>> +		printl $l, $spdsize;
>>
>>  		$l = "Fundamental Memory type";
>>  		my $type = "Unknown";
>> @@ -1325,14 +1649,14 @@
>>  			if ($bytes[2] == 1) { $type = "Direct Rambus"; }
>>  			elsif ($bytes[2] == 17) { $type = "Rambus"; }
>>  		} else {
>> -			if ($bytes[2] == 1) { $type = "FPM DRAM"; }
>> -			elsif ($bytes[2] == 2) { $type = "EDO"; }
>> -			elsif ($bytes[2] == 3) { $type = "Pipelined Nibble"; }
>> -			elsif ($bytes[2] == 4) { $type = "SDR SDRAM"; }
>> -			elsif ($bytes[2] == 5) { $type = "Multiplexed ROM"; }
>> -			elsif ($bytes[2] == 6) { $type = "DDR SGRAM"; }
>> -			elsif ($bytes[2] == 7) { $type = "DDR SDRAM"; }
>> -			elsif ($bytes[2] == 8) { $type = "DDR2 SDRAM"; }
>> +			my @types = ("Reserved", "FPM DRAM", "EDO",
>> +				"Pipelined Nibble", "SDR DRAM",
>> +				"Multiplexed ROM", "DDR SGRAM", "DDR SDRAM",
>> +				"DDR2 SDRAM", "FB_DIMM", "FB-DIMM PROBE",
>> +				"DDR3 SDRAM");
>> +			if ($bytes[2] <= $#types) {
>> +				$type = $types[$bytes[2]];
>> +			} else { $type = $types[0]; }
>
> While I agree this change is a good idea, it should not be part of this
> DDR3 patch. Especially when it breaks the SDRAM case! You changed the
> "SDR SDRAM" string to SDR DRAM", while we do use these strings as hash
> keys later in the script.

Typo on my part.  Will fix.  It just seemed to me a good idea to make 
this type of change now rather than to perptuate the cascading elif's.

>>  		}
>>  		printl $l, $type;
>>
>> @@ -1340,10 +1664,19 @@
>>  		$decode_callback{$type}->(\@bytes)
>>  			if exists $decode_callback{$type};
>>
>> +		# DDR3 Manufacturer info is already decoded
>> +		# (It's NOT common!)
>> +
>> +		next if ($type eq "DDR3 SDRAM");
>> +
>
> This hack breaks the html output mode.

Ooops - did not pay attention to that.

>
>>  # Decode next 35 bytes (64-98, common to all memory types)
>>  		prints "Manufacturing Information";
>>
>> -		@bytes = readspd64(64, $dimm_list[$i]);
>> +		if ($#bytes == 63) {
>> +			@bytes = readspd64(64, $dimm_list[$i]);
>> +		} else {
>> +			@bytes = @bytes[64..$#bytes];
>> +		}
>
> And this shouldn't be needed: if you reach this part of the code, it
> means you're not in the DDR3 case, so you can't have bytes 64-127 in
> the @bytes array yet.

Not true, at least not intended to be true.  Since FB-DIMM also uses CRC 
instad of checksum, it should already have loaded the entire array.  But 
FB-DIMM still uses the common manufacturing info code, so this was 
needed.

>>
>>  		$l = "Manufacturer";
>>  		# $extra is a reference to an array containing up to
>
> So, as I wrote before, let's clear up the script from its improper
> assumptions first. I can help with this if you don't have the time, but
> you must tell me what needs to be done and why.

Hopefully I've addressed your comments here.  I'll be happy to make one 
more pass at this, and submit two separate patches.  But I've already 
spent a lot more time than I expected (OK, most of that time was spent 
trying to understand perl!) so if the next submission isn't adequate, 
I'll have to leave it for someone else to properly integrate.





----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul-3orOWTcw9wBWk0Htik3J/w@public.gmane.org   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette-3r7Miqu9kMnR7s880joybQ@public.gmane.org |
----------------------------------------------------------------------

Re: Preliminary support for DDR3 in decode-dimms (fwd)

[Paul Goyette]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1856 bytes --]

Jean et al,

Attached please find a new set of diffs for decode-dimms.  This is a 
beginning at addressing the earlier comments.

This set of diffs implements the following changes:

1. Instead of reading the spd data 64 bytes at a time, we calculate
    the actual sizes (total and written) of the spd and read the entire
    thing, all at once.

1a.This also involved changing all the offsets at the end of the main
    loop which were pointing to Manufacturing data, etc.

1b.Also, the main loop is modified to use the returned, calulated sizes
    rather than making its own calculations.

2. Remove the checksum calculation from the main loop into a separate
    function which returns the actual and calculated checksums as well
    as a True/False match status.

3. Create a CRC routine with the same arguments and returns as for
    the checksum routine.

4. In the mainline, call the checksum() or CRC() routine as appropriate
    for the specific memory type.

5. The cascading if .. elsif .. for fundamental memory types has been
    replaced with an indexable list.  The list has been updated to
    include FB-DIMM, FB-DIMM Probe, and DDR3 SDRAM types.


The following is not yet done;  I'll leave these for the next round, 
assuming this set of changes comes closer to meeting your approval.

6. Add the routine to actually decode DDR3 data

7. Move the now not-so-common manufacturing data into a separate
    function.


Thanks!


----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul-3orOWTcw9wBWk0Htik3J/w@public.gmane.org   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette-3r7Miqu9kMnR7s880joybQ@public.gmane.org |
----------------------------------------------------------------------

[-- Attachment #2: Type: TEXT/PLAIN, Size: 9500 bytes --]

--- decode-dimms.orig	2008-11-18 09:15:09.000000000 -0800
+++ decode-dimms.cleanup	2008-12-08 09:44:53.000000000 -0800
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/pkg/bin/perl -w
 #
 # EEPROM data decoder for SDRAM DIMM modules
 #
@@ -1156,30 +1156,70 @@ sub read_hexdump($)
 	return @bytes;
 }
 
-sub readspd64($$) # reads 64 bytes from SPD-EEPROM
+sub spd_sizes(@) {
+	my @bytes = @_;
+
+	if ($bytes[2] >= 9) {
+		# For FB-DIMM and newer, decode number of bytes written
+		my $spd_len = ($bytes[0] >> 4) & 7;
+		my $size = 64 << ($bytes[0] & 15);
+		if ($spd_len == 0) {
+			return ($size, 128);
+		} elsif ($spd_len == 1) {
+			return ($size, 176);
+		} elsif ($spd_len == 2) {
+			return ($size, 256);
+		} else {
+			return (64, 64);
+		}
+        } else {
+		my $size;
+		if ($bytes[1] <= 14) {
+			$size = 1 << $bytes[1];
+		} elsif ($bytes[1] == 0) {
+			$size = "RFU";
+		} else { $size = "ERROR!" }
+
+		return ($size, ($bytes[0] < 64) ? 64 : $bytes[0]);
+        }
+}
+
+sub readspd($) # reads entire SPD-EEPROM
 {
-	my ($offset, $dimm_i) = @_;
+	my $dimm_i = $_;
 	my @bytes;
+	my $written;
+	my $size;
 	if ($use_hexdump) {
 		@bytes = read_hexdump($dimm_i);
-		return @bytes[$offset..($offset+63)];
+		($size, $written) = spd_sizes(@bytes);
+		return ($size, $written, @bytes[0..($written - 1)]);
 	} elsif ($use_sysfs) {
 		# Kernel 2.6 with sysfs
 		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
 			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
 		binmode HANDLE;
-		sysseek(HANDLE, $offset, SEEK_SET);
-		sysread(HANDLE, my $eeprom, 64);
+
+		# Read header bytes to determine spd size
+		sysread(HANDLE, my $eeprom, 3);
+		@bytes = unpack("C64", $eeprom);
+		($size, $written) = spd_sizes(@bytes);
+
+		# Now (re)read the entire spd
+		sysseek(HANDLE, 0, SEEK_SET);
+		sysread(HANDLE, $eeprom, $written);
 		close HANDLE;
 		@bytes = unpack("C64", $eeprom);
 	} else {
 		# Kernel 2.4 with procfs
-		for my $i (0 .. 3) {
-			my $hexoff = sprintf('%02x', $offset + $i * 16);
+		@bytes = split(" ", `cat /proc/sys/dev/sensors/$dimm_i/00`);
+		($size, $written) = spd_sizes(@bytes);
+		for my $i (1 .. (($written + 15) / 16)) {
+			my $hexoff = sprintf('%02x', $i * 16);
 			push @bytes, split(" ", `cat /proc/sys/dev/sensors/$dimm_i/$hexoff`);
 		}
 	}
-	return @bytes;
+	return ($size, $written, @bytes);
 }
 
 # Parse command-line
@@ -1243,6 +1283,48 @@ if ($opt_html && !$opt_bodyonly) {
 		  "</head><body>\n";
 }
 
+# Calculate and verify checksum of first 63 bytes
+
+sub checksum(@) {
+	my @bytes = @_;
+	my $dimm_checksum = 0;
+	$dimm_checksum += $bytes[$_] foreach (0 .. 62);
+	$dimm_checksum &= 0xff;
+
+	return ("EEPROM Checksum of bytes 0-62",
+		($bytes[63] == $dimm_checksum) ? 1 : 0,
+		sprintf('0x%02x', $bytes[63]),
+		sprintf('0x%02x', $dimm_checksum));
+}
+
+# Calculate and verify CRC
+
+sub check_crc(@) {
+	my @bytes = @_;
+	my $crc = 0;
+	my $crc_cover = $bytes[0] & 0x80 ? 116 : 125;
+	my $crc_ptr = 0;
+	my $crc_bit;
+	while ($crc_ptr <= $crc_cover) {
+		$crc = $crc ^ ($bytes[$crc_ptr] << 8);
+		for ($crc_bit = 0; $crc_bit < 8; $crc_bit++) {
+			if ($crc & 0x8000) {
+				$crc = ($crc << 1) ^ 0x1021;
+			} else {
+				$crc = $crc << 1
+			}
+		}
+		$crc_ptr++;
+	}
+	$crc = $crc & 0xffff;
+
+	my $dimm_crc = ($bytes[127] << 8) | $bytes[126];
+	return ("EEPROM CRC of bytes 0-" . sprintf("%d", $crc_cover),
+		($dimm_crc == $crc) ? 1 : 0,
+		sprintf("0x%04x", $dimm_crc),
+		sprintf("0x%04x", $crc));
+}
+
 printc "decode-dimms version $revision";
 printh 'Memory Serial Presence Detect Decoder',
 'By Philip Edelbrock, Christian Zuckschwerdt, Burkart Lingner,
@@ -1267,12 +1349,19 @@ for my $i ( 0 .. $#dimm_list ) {
 	if (($use_sysfs && /^\d+-\d+$/)
 	 || (!$use_sysfs && /^eeprom-/)
 	 || $use_hexdump) {
-		my @bytes = readspd64(0, $dimm_list[$i]);
-		my $dimm_checksum = 0;
-		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
-		$dimm_checksum &= 0xff;
+		my @bytes = readspd($dimm_list[$i]);
+		my $spd_size = shift(@bytes);
+		my $spd_used = shift(@bytes);
+		my ($chk_type, $chk_valid, $chk_spd, $chk_calc);
+		if ($bytes[2] < 9) {
+			($chk_type, $chk_valid, $chk_spd, $chk_calc) =
+				checksum(@bytes);
+		} else {
+			($chk_type, $chk_valid, $chk_spd, $chk_calc) =
+				check_crc(@bytes);
+		}
 
-		next unless $bytes[63] == $dimm_checksum || $opt_igncheck;
+		next unless $chk_valid || $opt_igncheck;
 		$dimm_count++;
 
 		print "<b><u>" if $opt_html;
@@ -1293,11 +1382,10 @@ for my $i ( 0 .. $#dimm_list ) {
 # Decode first 3 bytes (0-2)
 		prints "SPD EEPROM Information";
 
-		my $l = "EEPROM Checksum of bytes 0-62";
-		printl $l, ($bytes[63] == $dimm_checksum ?
-			sprintf("OK (0x%.2X)", $bytes[63]):
-			sprintf("Bad\n(found 0x%.2X, calculated 0x%.2X)\n",
-				$bytes[63], $dimm_checksum));
+		printl $chk_type, ($chk_valid ?
+			sprintf("OK (%s)", $chk_calc) :
+			sprintf("Bad\n(found %s, calculated %s)\n",
+				$chk_spd, $chk_calc));
 
 		# Simple heuristic to detect Rambus
 		my $is_rambus = $bytes[0] < 4;
@@ -1309,31 +1397,29 @@ for my $i ( 0 .. $#dimm_list ) {
 			else { $temp = "Reserved"; }
 			printl "SPD Revision", $temp;
 		} else {
-			printl "# of bytes written to SDRAM EEPROM",
-			       $bytes[0];
+			printl "# of bytes written to SDRAM EEPROM", $spd_used;
 		}
 
-		$l = "Total number of bytes in EEPROM";
-		if ($bytes[1] <= 14) {
-			printl $l, 2**$bytes[1];
-		} elsif ($bytes[1] == 0) {
-			printl $l, "RFU";
-		} else { printl $l, "ERROR!"; }
+		my $l = "Total number of bytes in EEPROM";
+		printl $l, $spd_size;
 
 		$l = "Fundamental Memory type";
-		my $type = "Unknown";
+		my $type;
 		if ($is_rambus) {
 			if ($bytes[2] == 1) { $type = "Direct Rambus"; }
 			elsif ($bytes[2] == 17) { $type = "Rambus"; }
+			else { $type = "Unknown"; }
 		} else {
-			if ($bytes[2] == 1) { $type = "FPM DRAM"; }
-			elsif ($bytes[2] == 2) { $type = "EDO"; }
-			elsif ($bytes[2] == 3) { $type = "Pipelined Nibble"; }
-			elsif ($bytes[2] == 4) { $type = "SDR SDRAM"; }
-			elsif ($bytes[2] == 5) { $type = "Multiplexed ROM"; }
-			elsif ($bytes[2] == 6) { $type = "DDR SGRAM"; }
-			elsif ($bytes[2] == 7) { $type = "DDR SDRAM"; }
-			elsif ($bytes[2] == 8) { $type = "DDR2 SDRAM"; }
+			my @type_list = ("Reserved", "FPM DRAM", "EDO",
+				"Pipelined Nibble", "SDR SDRAM",
+				"Multiplexed ROM", "DDR SGRAM", "DDR SDRAM",
+				"DDR2 SDRAM", "FB-DIMM", "FB-DIMM Probe",
+				"DDR3 SDRAM");
+			if ($bytes[2] <= $#type_list) {
+				$type = $type_list[$bytes[2]];
+			} else {
+				$type = sprintf("Unknown (0x%02x)", $bytes[2]);
+			}
 		}
 		printl $l, $type;
 
@@ -1344,61 +1430,59 @@ for my $i ( 0 .. $#dimm_list ) {
 # Decode next 35 bytes (64-98, common to all memory types)
 		prints "Manufacturing Information";
 
-		@bytes = readspd64(64, $dimm_list[$i]);
-
 		$l = "Manufacturer";
 		# $extra is a reference to an array containing up to
 		# 7 extra bytes from the Manufacturer field. Sometimes
 		# these bytes are filled with interesting data.
-		($temp, my $extra) = manufacturer(@bytes[0..7]);
+		($temp, my $extra) = manufacturer(@bytes[64..71]);
 		printl $l, $temp;
 		$l = "Custom Manufacturer Data";
 		$temp = manufacturer_data(@{$extra});
 		printl $l, $temp if defined $temp;
 
-		if (spd_written($bytes[8])) {
+		if (spd_written($bytes[72])) {
 			# Try the location code as ASCII first, as earlier specifications
 			# suggested this. As newer specifications don't mention it anymore,
 			# we still fall back to binary.
 			$l = "Manufacturing Location Code";
-			$temp = (chr($bytes[8]) =~ m/^[\w\d]$/) ? chr($bytes[8])
-			      : sprintf("0x%.2X", $bytes[8]);
+			$temp = (chr($bytes[72]) =~ m/^[\w\d]$/) ? chr($bytes[72])
+			      : sprintf("0x%.2X", $bytes[72]);
 			printl $l, $temp;
 		}
 
 		$l = "Part Number";
-		$temp = part_number(@bytes[9..26]);
+		$temp = part_number(@bytes[73..90]);
 		printl $l, $temp;
 
-		if (spd_written(@bytes[27..28])) {
+		if (spd_written(@bytes[91..92])) {
 			$l = "Revision Code";
-			$temp = sprintf("0x%02X%02X\n", @bytes[27..28]);
+			$temp = sprintf("0x%02X%02X\n", @bytes[91..92]);
 			printl $l, $temp;
 		}
 
-		if (spd_written(@bytes[29..30])) {
+		if (spd_written(@bytes[93..94])) {
 			$l = "Manufacturing Date";
 			# In theory the year and week are in BCD format, but
 			# this is not always true in practice :(
-			if (($bytes[29] & 0xf0) <= 0x90
-			 && ($bytes[29] & 0x0f) <= 0x09
-			 && ($bytes[30] & 0xf0) <= 0x90
-			 && ($bytes[30] & 0x0f) <= 0x09) {
+			if (($bytes[93] & 0xf0) <= 0x90
+			 && ($bytes[93] & 0x0f) <= 0x09
+			 && ($bytes[94] & 0xf0) <= 0x90
+			 && ($bytes[94] & 0x0f) <= 0x09) {
 				# Note that this heuristic will break in year 2080
 				$temp = sprintf("%d%02X-W%02X\n",
-						$bytes[29] >= 0x80 ? 19 : 20,
-						@bytes[29..30]);
+						$bytes[93] >= 0x80 ? 19 : 20,
+						@bytes[93..94]);
 			} else {
 				$temp = sprintf("0x%02X%02X\n",
-						@bytes[29..30]);
+						@bytes[93..94]);
 			}
 			printl $l, $temp;
 		}
 
-		if (spd_written(@bytes[31..34])) {
+		if (spd_written(@bytes[95..98])) {
 			$l = "Assembly Serial Number";
 			$temp = sprintf("0x%02X%02X%02X%02X\n",
-					@bytes[31..34]);
+					@bytes[95..98]);
 			printl $l, $temp;
 		}
 

Re: Preliminary support for DDR3 in decode-dimms (fwd)

[Jean Delvare]

Hi Paul,

On Mon, 8 Dec 2008 06:24:32 -0800 (PST), Paul Goyette wrote:
> On Mon, 8 Dec 2008, Jean Delvare wrote:
> > Apparently you had to make some changes because assumptions which were
> > correct up to DDR2 are no longer correct for DDR3. Some of these
> > changes look like hacks to me (for example, the readfullspd function
> > and the conditionals it adds to the common code). I really would prefer
> > that we first clean up the script to get rid of improper assumptions,
> > and only after this is done, add support for DDR3. That way I will have
> > two medium patches to review rather than a single large one, and the
> > second patch should be pretty straightforward. This will be way easier
> > for me.
> 
> For FB-DIMMs (which are previously supported) and DDR3, the old checksum 
> over bytes 0-62 no longer exists;  instead, there is a CRC in bytes 126 
> and 127 that covers a variable section of the SPD data.  readfullspd was 
> used to read the entire data, rather than just 0-63.  It would not be 
> too difficult to replace the old (multiple) call to read64.

OK. The reason for not reading all 128 bytes initially is that the
first 64 bytes were enough to verify the checksum, and as reading from
EEPROMs can be slow, it was saving some time for invalid or non-SPD
EEPROMs. But optimizing the code for the failure case doesn't really
make sense, I'd rather have more simple code even if it is slower in a
few corner cases.

> > For example, if the manufacturing information is no longer common to
> > all memory types, then let's move it to a separate function. If
> > checksum handling is type-specific, then let's move it to a separate
> > function as well. I really don't want the main loop to grow
> > indefinitely.
> 
> There are really only two types of validation:
> 
>  	checksum for everything with type < FB-DIMM
>  	CRC for everything >= FB-DIMM
> 
> I'm not sure it makes sense to add a specific call to each memory type's 
> specific processing, especially since it would mean that checksum would 
> not get checked for types that don't have a specific processing routine 
> (ie, the ROMs).

We could always add callbacks for these types. But I don't care too
much about the actual implementation as long as it is clean and works.

> Manufacturing information is indeed no longer completely common.

I moved it to a separate function, it will make it easier to implement
DDR3 support cleanly.

> (...)
> Hopefully I've addressed your comments here.  I'll be happy to make one 
> more pass at this, and submit two separate patches.  But I've already 
> spent a lot more time than I expected (OK, most of that time was spent 
> trying to understand perl!) so if the next submission isn't adequate, 
> I'll have to leave it for someone else to properly integrate.

Thank you. I really don't mean to waste your time here, I just want to
make sure that the code quality is maintained at a proper level, that
we do not introduce any regression, and that maintaining the code in
the future won't be too difficult. Do what you can and I'll do the rest.

In fact I had already started committing some clean-ups. I'll now adjust
your latest patch to apply on top of that. I'll let you know what I am
done and we can pursue with DDR3 support.

Thanks,
-- 
Jean Delvare

Re: Preliminary support for DDR3 in decode-dimms (fwd)

[Paul Goyette]

On Tue, 9 Dec 2008, Jean Delvare wrote:

>
> Thank you. I really don't mean to waste your time here, I just want to
> make sure that the code quality is maintained at a proper level, that
> we do not introduce any regression, and that maintaining the code in
> the future won't be too difficult. Do what you can and I'll do the rest.
>
> In fact I had already started committing some clean-ups. I'll now adjust
> your latest patch to apply on top of that. I'll let you know what I am
> done and we can pursue with DDR3 support.

Kewl.  Let me know when you have a new baseline for me to work with.

I actually had done some of the clean-up stuff myself, but I'm more than 
happy to have someone more familiar with perl to do that for me.  :)


----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul-3orOWTcw9wBWk0Htik3J/w@public.gmane.org   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette-3r7Miqu9kMnR7s880joybQ@public.gmane.org |
----------------------------------------------------------------------

Re: Preliminary support for DDR3 in decode-dimms

[Jean Delvare]

On Tue, 9 Dec 2008 04:07:20 -0800 (PST), Paul Goyette wrote:
> On Tue, 9 Dec 2008, Jean Delvare wrote:
> 
> >
> > Thank you. I really don't mean to waste your time here, I just want to
> > make sure that the code quality is maintained at a proper level, that
> > we do not introduce any regression, and that maintaining the code in
> > the future won't be too difficult. Do what you can and I'll do the rest.
> >
> > In fact I had already started committing some clean-ups. I'll now adjust
> > your latest patch to apply on top of that. I'll let you know what I am
> > done and we can pursue with DDR3 support.
> 
> Kewl.  Let me know when you have a new baseline for me to work with.
> 
> I actually had done some of the clean-up stuff myself, but I'm more than 
> happy to have someone more familiar with perl to do that for me.  :)

Here we go. You can fetch an up-to-date version of decode-dimms with:

svn export http://lm-sensors.org/svn/i2c-tools/trunk/eeprom/decode-dimms

Main differences with your original patch:

* readspd() only reads data from the EEPROM, it doesn't attempt to
  compute the size. The reason is that you don't even know if what
  you're reading is an SPD EEPROM yet.

* All functions take a reference to the bytes array as a parameter,
  instead of the array itself.

* Handling of Rambus types is cleaner, in particular spd_sizes() isn't
  called for Rambus, as I don't think it would handle it properly. Not
  that I really care about Rambus though, given how unpopular it is. I'm
  curious if anyone actually ever ran decode-dimms on Rambus.

* Decoding of manufacturing information has moved to a separate
  function.

The rest is essentially the same as the code you had provided. I think
we ar ready to add support for DDR3 SDRAM now.

Thanks,
-- 
Jean Delvare