[lm-sensors] sensord and RAM SPD

[lm-sensors] sensord and RAM SPD

[Aurelien Jarno]

Hi all,

sensord currently fails decode correctly RAM SPD in most cases
and thus fails to determine the RAM size. It seems the corresponding
code is a bit outdated.

Please find attached a patch to fix that. Most of the code is taken 
from sensors.

Bye,
Aurelien

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net
-------------- next part --------------
--- lm-sensors-2.9.2.orig/prog/sensord/chips.c
+++ lm-sensors-2.9.2/prog/sensord/chips.c
@@ -1008,27 +1008,56 @@
 /** EEPROM **/
 
 static const char *
-fmtType_EEPROM
+fmt_EEPROM
 (const double values[], int alarm, int beep) {
-  if ((int) values[0] = 4)
-    sprintf (buff, "SDRAM DIMM SPD");
-  else if ((int) values[0] = 7)
-    sprintf (buff, "DDR SDRAM DIMM SPD");
-  else
-    sprintf (buff, "Invalid"); /* N.B: sensors just returns, aborting further tests; I don't.. */
-  return fmtExtra (alarm, beep);
-}
-
-static const char *
-fmtRowCol_EEPROM
-(const double values[], int alarm, int beep) {
-  int row = (int) values[0];
-  int col = (int) values[1];
-  int num = (int) values[2];
-  int banks = (int) values[3];
-  int foo = (row & 0xf) + (col & 0xf) + 17;
-  if ((foo > 0) && (foo <= 12) && (num <= 8) && (banks <= 8)) {
-    sprintf (buff, "%d", (1 << foo) * num * banks);
+  int i, k;
+  int type = values[0];
+  int row = (int) values[1];
+  int col = (int) values[2];
+  int num = (int) values[3];
+  int banks = (int) values[4];
+
+  switch (type) {
+    case 1:
+            sprintf (buff, "DRDRAM RIMM SPD");
+	    break;
+    case 2:
+            sprintf (buff, "EDO SPD");
+	    break;
+    case 4:
+            sprintf (buff, "SDR SDRAM DIMM SPD");
+	    break;
+    case 7:
+            sprintf (buff, "DDR SDRAM DIMM SPD");
+	    break;
+    case 8:
+            sprintf (buff, "DDR2 SDRAM DIMM SPD");
+	    break;
+    case 17:
+            sprintf (buff, "RAMBUS RIMM SPD");
+	    break;
+    default:
+	    sprintf (buff, "Invalid");
+	    return buff;
+  }	    
+
+  k = 0; /* multiplier, 0 if invalid */
+  if (type = 17) { /* RAMBUS */
+    i = (((int) row) & 0x0f) + (((int) row) >> 4) + (((int) num) & 0x07) - 13;
+    k = 1;
+  } else if (type = 1) { /* DRDRAM */
+    i = (((int) col) & 0x0f) + (((int) col) >> 4) + (((int) num) & 0x07) - 13;
+    k = 1;
+  } else if (type = 8) { /* DDR2 */
+    i = (((int) row) & 0x0f) + (((int) col) & 0x0f) - 17;
+    k = ((((int) num) & 0x7) + 1) * ((int) banks);
+  } else { /* SDRAM */
+    i = (((int) row) & 0x0f) + (((int) col) & 0x0f) - 17;
+    if (((int) num) <= 8 && ((int) banks) <= 8)
+      k = ((int) num) * ((int) banks);
+  }
+  if(i > 0 && i <= 12 && k > 0) {
+    sprintf (buff, "%d", (1 << i) * k);
   } else {
     sprintf (buff, "Invalid %d %d %d %d", row, col, num, banks);
   }
@@ -1040,10 +1069,8 @@
 };
 
 static const FeatureDescriptor eeprom_features[] = {
-  { fmtType_EEPROM, NULL, DataType_other, 0, 0,
-    { SENSORS_EEPROM_TYPE, -1 } },
-  { fmtRowCol_EEPROM, NULL, DataType_other, 0, 0,
-    { SENSORS_EEPROM_ROWADDR, SENSORS_EEPROM_COLADDR, SENSORS_EEPROM_NUMROWS, SENSORS_EEPROM_BANKS, -1 } },
+  { fmt_EEPROM, NULL, DataType_other, 0, 0,
+    { SENSORS_EEPROM_TYPE, SENSORS_EEPROM_ROWADDR, SENSORS_EEPROM_COLADDR, SENSORS_EEPROM_NUMROWS, SENSORS_EEPROM_BANKS, -1 } },
   { NULL }
 };
 

[lm-sensors] sensord and RAM SPD

[Jean Delvare]

Hi Aurelien,

[Aurelien Jarno]
> sensord currently fails decode correctly RAM SPD in most cases
> and thus fails to determine the RAM size. It seems the corresponding
> code is a bit outdated.
>
> Please find attached a patch to fix that. Most of the code is taken
> from sensors.

Considering that:
* the code was outdated for a long time and nobody complained;
* SPD EEPROMs are not sensors;
* SPD EEPROM values never change;
* eeprom support will be ultimately dropped from libsensors;
I'd prefer that we plain drop eeprom support from sensord.

Thanks,
--
Jean Delvare

[lm-sensors] sensord and RAM SPD

[Aurelien Jarno]

Jean Delvare a ?crit :
> Considering that:
> * the code was outdated for a long time and nobody complained;
> * SPD EEPROMs are not sensors;
> * SPD EEPROM values never change;
> * eeprom support will be ultimately dropped from libsensors;
> I'd prefer that we plain drop eeprom support from sensord.
> 
That's also fine with me. Do you want a patch for that?

Aurelien

-- 
   .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
  : :' :  Debian GNU/Linux developer | Electrical Engineer
  `. `'   aurel32@debian.org         | aurelien@aurel32.net
    `-    people.debian.org/~aurel32 | www.aurel32.net

[lm-sensors] sensord and RAM SPD

[Jean Delvare]

Hi Aurelien,

[Aurelien Jarno]
> > I'd prefer that we plain drop eeprom support from sensord.
>
> That's also fine with me. Do you want a patch for that?

Yes please :)

--
Jean Delvare

[lm-sensors] sensord and RAM SPD

[Aurelien Jarno]

On Fri, Sep 16, 2005 at 12:05:04PM +0200, Jean Delvare wrote:
> 
> Hi Aurelien,
> 
> [Aurelien Jarno]
> > > I'd prefer that we plain drop eeprom support from sensord.
> >
> > That's also fine with me. Do you want a patch for that?
> 
> Yes please :)
> 
Please find it attached.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net
-------------- next part --------------
--- lm-sensors-2.9.2.orig/prog/sensord/chips.c
+++ lm-sensors-2.9.2/prog/sensord/chips.c
@@ -1005,52 +1005,6 @@
   asb100_names, asb100_features, 0, 0
 };
 
-/** EEPROM **/
-
-static const char *
-fmtType_EEPROM
-(const double values[], int alarm, int beep) {
-  if ((int) values[0] = 4)
-    sprintf (buff, "SDRAM DIMM SPD");
-  else if ((int) values[0] = 7)
-    sprintf (buff, "DDR SDRAM DIMM SPD");
-  else
-    sprintf (buff, "Invalid"); /* N.B: sensors just returns, aborting further tests; I don't.. */
-  return fmtExtra (alarm, beep);
-}
-
-static const char *
-fmtRowCol_EEPROM
-(const double values[], int alarm, int beep) {
-  int row = (int) values[0];
-  int col = (int) values[1];
-  int num = (int) values[2];
-  int banks = (int) values[3];
-  int foo = (row & 0xf) + (col & 0xf) + 17;
-  if ((foo > 0) && (foo <= 12) && (num <= 8) && (banks <= 8)) {
-    sprintf (buff, "%d", (1 << foo) * num * banks);
-  } else {
-    sprintf (buff, "Invalid %d %d %d %d", row, col, num, banks);
-  }
-  return buff;
-}
-
-static const char *eeprom_names[] = {
-  SENSORS_EEPROM_PREFIX, NULL
-};
-
-static const FeatureDescriptor eeprom_features[] = {
-  { fmtType_EEPROM, NULL, DataType_other, 0, 0,
-    { SENSORS_EEPROM_TYPE, -1 } },
-  { fmtRowCol_EEPROM, NULL, DataType_other, 0, 0,
-    { SENSORS_EEPROM_ROWADDR, SENSORS_EEPROM_COLADDR, SENSORS_EEPROM_NUMROWS, SENSORS_EEPROM_BANKS, -1 } },
-  { NULL }
-};
-
-static const ChipDescriptor eeprom_chip = {
-  eeprom_names, eeprom_features, 0, 0
-};
-
 /** PC87360 **/
 
 static const char *
@@ -1145,7 +1099,6 @@
   &adm1025_chip,
   &adm9240_chip,
   &ds1621_chip,
-  &eeprom_chip,
   &gl518_chip,
   &lm75_chip,
   &lm78_chip,

[lm-sensors] sensord and RAM SPD

[David Goodenough]

On Friday 16 September 2005 11:05, Jean Delvare wrote:
> Hi Aurelien,
>
> [Aurelien Jarno]
>
> > > I'd prefer that we plain drop eeprom support from sensord.
> >
> > That's also fine with me. Do you want a patch for that?
>
> Yes please :)
>
> --
> Jean Delvare

One little comment, while eeprom support may not really be a
sensor, it is a almost always there and when someone says that
they can not get sensors working if eeprom works then at least
the basics are working properly, its just a matter of finding the
right chip for sensing (and in many economy boards there are
none).  I think it is useful in initial setup and if it causes no 
problems it should be retained for that reason alone.

David

[lm-sensors] sensord and RAM SPD

[Jean Delvare]

Hi David,

[David Goodenough]
> One little comment, while eeprom support may not really be a
> sensor, it is a almost always there and when someone says that
> they can not get sensors working if eeprom works then at least
> the basics are working properly,

What it actually means is that the SMBus driver is functional. This
doesn't really help, because cases of non-working SMBus drivers are
rather rare.

Most hardware monitoring devices these days are embedded in Super-I/O
chips. For these, we do not care about the SMBus and eeprom is of no
help.

Also, missing eeproms don't prove anything, there are motherboards which
do not expose their SPD EEPROMs on the SMBus. So it's a hint at best.

> its just a matter of finding the
> right chip for sensing (and in many economy boards there are
> none).  I think it is useful in initial setup and if it causes no
> problems it should be retained for that reason alone.

I know others (MDS I think) agree with you, but I don't.

From a support point of view, it does not make any difference when a user
reports "no sensors found" with or without eeproms in "sensors". In
any case we have to ask him/her for a full sensors-detect output, or
i2cdetect output, and/or lspci output, before we can conclude or ask
him/her fore more. The fact that the SMBus is working is an information
we can use only in later steps of the investigation, because
non-functional SMBus are the least probable cause of problems. It's so
rare that I don't mind having to ask explicitely.

Since linux 2.6.14-rc1, the hardware monitoring drivers have a dedicated
class, eeprom doesn't have it. Since lm_sensors 2.9.2, eeproms are no
longer shown to Linux 2.6 users, even for older 2.6 kernels. We may keep
support for some times, not to disturb 2.4 kernel users, but that's
about it. Look at the eeprom-specific code in libsensors, you'll see
why I want to get rid of it. It just doesn't fit in the picture.

If you really want to check whether the SMBus is working or not,
i2cdetect and i2cdump will do it. If you think they are too hard to use
for the average user, you can add an automatic mode to i2cdetect, which
will list all available busses and probe them all. You may have it dump
the contents of chips in the 0x50-0x57, these are certainly EEPROMs.
These are disgnostics tools, they are meant for the job, I don't mind
extending them. Sensors and libsensors are no diagnostics tools.

Thanks,
--
Jean Delvare

[lm-sensors] sensord and RAM SPD

[Jean Delvare]

Hi Aurelien,

> Please find it attached.

Applied, thanks.

-- 
Jean Delvare