decode-dimms: Module Configuration Type not reported for DDR2/3

decode-dimms: Module Configuration Type not reported for DDR2/3

[Andriy Gapon]

It seems that Module Configuration Type ("ECC", "No Parity", etc) is not
reported for DDR2 and DDR3 module types.  Not sure if resolution could be as
easy as adding sdram_module_configuration_type() calls in the corresponding
decode procedures.

-- 
Andriy Gapon

Re: decode-dimms: Module Configuration Type not reported for DDR2/3

[Jean Delvare]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

Hi Andriy,

On Tue, 17 Apr 2012 00:09:08 +0300, Andriy Gapon wrote:
> It seems that Module Configuration Type ("ECC", "No Parity", etc) is not
> reported for DDR2 and DDR3 module types.  Not sure if resolution could be as
> easy as adding sdram_module_configuration_type() calls in the corresponding
> decode procedures.

Won't be so easy. For DDR2 there is one new bit to decode
(Address/Command Parity). For DDR3 byte 11 has a completely different
meaning, and the information seem to be no longer available. As I read
the specification, the only way to tell if a DDR3 module has any form
of parity or ECC is to check the extra bus width in bits 4-3 of byte 8.
But this is a boolean type of information then, telling "there is some
form of data error detection/correction". No way to tell between parity
and ECC AFAICS, and information about Address/Command Parity is gone
too.

I am attaching two patches, one for DDR2, one for DDR3, please give
them a try.

-- 
Jean Delvare

[-- Attachment #2: decode-dimms-ddr2-module-config.patch --]
[-- Type: text/x-patch, Size: 1328 bytes --]

---
 eeprom/decode-dimms |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

--- i2c-tools.orig/eeprom/decode-dimms	2011-02-16 14:58:41.000000000 +0100
+++ i2c-tools/eeprom/decode-dimms	2012-04-17 10:22:06.784083856 +0200
@@ -525,16 +525,21 @@ sub sdram_voltage_interface_level($)
 	return ($_[0] < @levels) ? $levels[$_[0]] : "Undefined!";
 }
 
-# Common to SDR and DDR SDRAM
+# Common to SDR, DDR and DDR2 SDRAM
 sub sdram_module_configuration_type($)
 {
-	my @types = (
-		"No Parity",			# 0
-		"Parity",			# 1
-		"ECC",				# 2
-	);
+	my $byte = $_[0] & 0x07;
+	my @edc;
 
-	return ($_[0] < @types) ? $types[$_[0]] : "Undefined!";
+	return "No Parity" if $byte == 0;
+
+	# Data ECC includes Data Parity so don't print both
+	push @edc, "Data Parity" if ($byte & 0x03) == 0x01;
+	push @edc, "Data ECC" if ($byte & 0x02);
+	# New in DDR2 specification
+	push @edc, "Address/Command Parity" if ($byte & 0x04);
+
+	return join ", ", @edc;
 }
 
 # Parameter: EEPROM bytes 0-127 (using 3-62)
@@ -1019,6 +1024,9 @@ sub decode_ddr2_sdram($)
 	printl("Voltage Interface Level",
 	       sdram_voltage_interface_level($bytes->[8]));
 
+	printl("Module Configuration Type",
+	       sdram_module_configuration_type($bytes->[11]));
+
 	printl("Refresh Rate", ddr2_refresh_rate($bytes->[12]));
 
 	my @burst;

[-- Attachment #3: decode-dimms-ddr3-module-config.patch --]
[-- Type: text/x-patch, Size: 434 bytes --]

---
 eeprom/decode-dimms |    2 ++
 1 file changed, 2 insertions(+)

--- i2c-tools.orig/eeprom/decode-dimms	2012-04-17 10:22:06.000000000 +0200
+++ i2c-tools/eeprom/decode-dimms	2012-04-17 10:42:10.205990901 +0200
@@ -1169,6 +1169,8 @@ sub decode_ddr3_sdram($)
 
 	printl("SDRAM Device Width", (1 << (($bytes->[7] & 7) + 2))." bits");
 
+	printl("Bus Width Extension", ($bytes->[8] & 24)." bits");
+
 	my $taa;
 	my $trcd;
 	my $trp;

Re: decode-dimms: Module Configuration Type not reported for DDR2/3

[Andriy Gapon]

on 17/04/2012 14:18 Jean Delvare said the following:
> Hi Andriy,
> 
> On Tue, 17 Apr 2012 00:09:08 +0300, Andriy Gapon wrote:
>> It seems that Module Configuration Type ("ECC", "No Parity", etc) is not
>> reported for DDR2 and DDR3 module types.  Not sure if resolution could be as
>> easy as adding sdram_module_configuration_type() calls in the corresponding
>> decode procedures.
> 
> Won't be so easy. For DDR2 there is one new bit to decode
> (Address/Command Parity). For DDR3 byte 11 has a completely different
> meaning, and the information seem to be no longer available. As I read
> the specification, the only way to tell if a DDR3 module has any form
> of parity or ECC is to check the extra bus width in bits 4-3 of byte 8.
> But this is a boolean type of information then, telling "there is some
> form of data error detection/correction". No way to tell between parity
> and ECC AFAICS, and information about Address/Command Parity is gone
> too.
> 
> I am attaching two patches, one for DDR2, one for DDR3, please give
> them a try.
> 

Thanks a lot!
I am able to test only the DDR2 part at the moment.
With the patch I am getting the following
Module Configuration Type                       Data ECC
for these modules: http://www.valueram.com/datasheets/kvr800d2e5_2g.pdf

For some definitely non-ECC modules I get the expected
Module Configuration Type                       No Parity

P.S.
I've noticed that the "Decoding EEPROM" message is printed with
$dimm[$current]->{eeprom} when $opt_side_by_side is true but it is printed with
$dimm[$current]->{file} in the other case.  Not sure if this is on purpose.

-- 
Andriy Gapon

Re: decode-dimms: Module Configuration Type not reported for DDR2/3

[Jean Delvare]

Hi Andriy,

On Tue, 17 Apr 2012 23:03:16 +0300, Andriy Gapon wrote:
> on 17/04/2012 14:18 Jean Delvare said the following:
> > I am attaching two patches, one for DDR2, one for DDR3, please give
> > them a try.
> 
> Thanks a lot!
> I am able to test only the DDR2 part at the moment.
> With the patch I am getting the following
> Module Configuration Type                       Data ECC
> for these modules: http://www.valueram.com/datasheets/kvr800d2e5_2g.pdf
> 
> For some definitely non-ECC modules I get the expected
> Module Configuration Type                       No Parity

Thanks for the report, I've applied both patches.

> P.S.
> I've noticed that the "Decoding EEPROM" message is printed with
> $dimm[$current]->{eeprom} when $opt_side_by_side is true but it is printed with
> $dimm[$current]->{file} in the other case.  Not sure if this is on purpose.

I think it is on purpose. ->{file} contains the full path so it is
rather long, ->{eeprom} only contains the file name so it is much
shorter. The full path would make side-by-side output very wide, that
wouldn't be convenient, that's why the short name is used in this
output mode. In legacy output mode, the long name fits.

We _could_ use the short name in legacy output mode, as the long name
doesn't necessarily add a lot of value. I think I tried to leave the
legacy output unchanged when implementing the side-by-side output, to
avoid any regression in case someone was parsing the output of
decode-dimms. If you believe it would be better to always use the short
name, we could do that, this may even allow for a small code clean-up.

-- 
Jean Delvare

Re: decode-dimms: Module Configuration Type not reported for DDR2/3

[Andriy Gapon]

on 18/04/2012 11:30 Jean Delvare said the following:
> Hi Andriy,
> 
> On Tue, 17 Apr 2012 23:03:16 +0300, Andriy Gapon wrote:
>> on 17/04/2012 14:18 Jean Delvare said the following:
>>> I am attaching two patches, one for DDR2, one for DDR3, please give
>>> them a try.
>>
>> Thanks a lot!
>> I am able to test only the DDR2 part at the moment.
>> With the patch I am getting the following
>> Module Configuration Type                       Data ECC
>> for these modules: http://www.valueram.com/datasheets/kvr800d2e5_2g.pdf
>>
>> For some definitely non-ECC modules I get the expected
>> Module Configuration Type                       No Parity
> 
> Thanks for the report, I've applied both patches.

Thank you!

>> P.S.
>> I've noticed that the "Decoding EEPROM" message is printed with
>> $dimm[$current]->{eeprom} when $opt_side_by_side is true but it is printed with
>> $dimm[$current]->{file} in the other case.  Not sure if this is on purpose.
> 
> I think it is on purpose. ->{file} contains the full path so it is
> rather long, ->{eeprom} only contains the file name so it is much
> shorter. The full path would make side-by-side output very wide, that
> wouldn't be convenient, that's why the short name is used in this
> output mode. In legacy output mode, the long name fits.
> 
> We _could_ use the short name in legacy output mode, as the long name
> doesn't necessarily add a lot of value. I think I tried to leave the
> legacy output unchanged when implementing the side-by-side output, to
> avoid any regression in case someone was parsing the output of
> decode-dimms. If you believe it would be better to always use the short
> name, we could do that, this may even allow for a small code clean-up.

I don't have any strong opinion about this.  I see pros and contras for both
choices and I can not make up my mind :)

-- 
Andriy Gapon