Hi Todd,
Thanks for taking the time to review and offer improvements. I'm attaching
a new diff that incorporates your suggestion for simplifying the matching
and eliminating the unneeded struct and function. It is definitely a
cleaner, simpler approach. I also corrected the whitespace issue you
pointed out.
I made a few tweaks to the voltage calculations. I actually spent quite a
bit of time going down the rabbit hole here, and while I made some
improvements, it is definitely not perfect yet. I removed the division by
2 from Vcore; I'm pretty confident this is correct, because I'm now seeing
the voltage I expect, and the datasheet says:
"The CPUVCORE pin feeds directly into the ADC with no voltage divider
since the nominal voltage on this pin is only 1.2V."
I actually installed Windows on this machine so I could run HWiNFO64 and
see how the sensors looked there; I'm including a screenshot for your
reference. Under Windows, it looks like the sensors are enumerated and
displayed in the same order, though VBAT and VTT are skipped. Windows
appears to have VIN2 and VIN3 labeled as +12V and +5V, though the values I
see in HWiNFO64 are both slightly low and disagree with the values I see
in the BIOS. With the current version of my patch, VIN3 looks correct and
matches the BIOS value for +5V but VIN2 is low by a factor of about 3.5. I
adjusted VIN4, VIN5 and VIN6 by dividing by 2; this brings their readings
in line with what I see in HWiNFO64.
>From what I've read from the Linux lm_sensors folks, it's a
trial-and-error process to adjust these values correctly, and sometimes
the readings are just garbage. As it stands, the patch is definitely
better than not having sensors, and these values could always be tweaked
with subsequent patches. If you have any suggestions for improvements
here, please let me know.
Thanks again for your help; I hope this patch is able to go in.
Joe
> Hello,
>
> On Fri, Dec 13, 2019 at 06:32:36PM -0500, Joe Gidi wrote:
>> Hi all,
>>
>> I recently built a new system with an ASRock B450M Pro4 motherboard.
>
> I literally fired up a new system with the exact same board today and
> saw the temp sensors didn't appear to be supported, so I'm very happy
> that you posted this!
>
>> This board has a Nuvoton NCT6779D chip to monitor temperatures, fans and
>> voltages. OpenBSD's lm(4) currently only supports the Nuvoton NCT6776F
>> chip, added by Marco Pfatschbacher in 2011:
>>
>> https://marc.info/?l=openbsd-cvs&m=132318770131497&w=2
>>
>> NetBSD's lm(4) gained support for several other Nuvoton chips in this
>> commit by SAITOH Masanobu in 2017:
>>
>> http://mail-index.netbsd.org/source-changes/2017/07/11/msg086253.html
>>
>> I have adapted the NetBSD code to OpenBSD and confirmed that it appears
>> to
>> work correctly with my NCT6779D chip. With the attached patch, I get
>> this
>> in dmesg:
>>
>> wbsio0 at isa0 port 0x2e/2: NCT6779D rev 0x62
>> lm1 at wbsio0 port 0x290/8: NCT6779D
>>
>> And here's the sensor data:
>>
>> $ sysctl hw.sensors.lm1
>> hw.sensors.lm1.temp0=29.00 degC (MB Temperature)
>> hw.sensors.lm1.temp1=31.00 degC (CPU Temperature)
>> hw.sensors.lm1.temp2=78.00 degC (Aux Temp0)
>> hw.sensors.lm1.temp3=98.00 degC (Aux Temp1)
>> hw.sensors.lm1.temp4=22.50 degC (Aux Temp2)
>> hw.sensors.lm1.temp5=-23.00 degC (Aux Temp3)
>> hw.sensors.lm1.fan0=1095 RPM (System Fan)
>> hw.sensors.lm1.fan1=739 RPM (CPU Fan)
>> hw.sensors.lm1.fan2=400 RPM (Aux Fan0)
>> hw.sensors.lm1.fan3=0 RPM (Aux Fan1)
>> hw.sensors.lm1.fan4=387 RPM (Aux Fan2)
>> hw.sensors.lm1.volt0=0.74 VDC (VCore)
>> hw.sensors.lm1.volt1=2.16 VDC (VIN1)
>> hw.sensors.lm1.volt2=3.33 VDC (AVCC)
>> hw.sensors.lm1.volt3=3.33 VDC (+3.3V)
>> hw.sensors.lm1.volt4=21.56 VDC (VIN0)
>> hw.sensors.lm1.volt5=0.87 VDC (VIN8)
>> hw.sensors.lm1.volt6=0.59 VDC (VIN4)
>> hw.sensors.lm1.volt7=3.46 VDC (+3.3VSB)
>> hw.sensors.lm1.volt8=0.00 VDC (VBAT)
>> hw.sensors.lm1.volt9=0.00 VDC (VTT)
>> hw.sensors.lm1.volt10=0.45 VDC (VIN5)
>> hw.sensors.lm1.volt11=2.13 VDC (VIN6)
>> hw.sensors.lm1.volt12=3.38 VDC (VIN2)
>> hw.sensors.lm1.volt13=5.12 VDC (VIN3)
>> hw.sensors.lm1.volt14=1.81 VDC (VIN7)
>>
>> The motherboard and CPU temperature values look very reasonable; the Aux
>> Temp values look like garbage, possibly because there aren't any other
>> sensors on this board? Fan values look reasonable. I am unsure about the
>> voltage values, but the ones that claim to be 3.3 volts look sane.
>
> I see similar numbers with your patch applied.
>
> The BIOS doesn't show any other temperature values, so it is possble the
> Aux Temp sensors aren't hooked up to anything. In any event, the read
> procedure looks the same as the others (according to the datasheet), so
> I don't think it is wrong.
>
>> This is the first non-trivial patch I'm submitting and my C is pretty
>> rusty, so I would appreciate review and corrections. I don't have any
>> other systems with different Nuvoton chips to test, so I can't confirm
>> that this works for the other chips.
>>
>> Could anyone please review this and help me get it into commit-ready
>> shape?
>
> The diff looks pretty good to me - I have a couple of comments inline.
> Overall it looks good, and comparing to the NetBSD source it looks like
> you got everything.
>
>> Index: sys/dev/ic/lm78.c
>> ===================================================================
>> RCS file: /cvs/src/sys/dev/ic/lm78.c,v
>> retrieving revision 1.24
>> diff -u -p -u -r1.24 lm78.c
>> --- sys/dev/ic/lm78.c 14 Mar 2015 03:38:47 -0000 1.24
>> +++ sys/dev/ic/lm78.c 13 Dec 2019 22:49:03 -0000
>> @@ -67,6 +67,7 @@ void wb_refresh_temp(struct lm_softc *,
>> void wb_refresh_fanrpm(struct lm_softc *, int);
>> void wb_nct6776f_refresh_fanrpm(struct lm_softc *, int);
>> void wb_w83792d_refresh_fanrpm(struct lm_softc *, int);
>> +const char * wb_nct67xx_id2str(uint8_t);
>>
>> void as_refresh_temp(struct lm_softc *, int);
>>
>> @@ -80,6 +81,20 @@ struct lm_chip lm_chips[] = {
>> { def_match } /* Must be last */
>> };
>>
>> +struct {
>> + uint8_t id;
>> + const char *str;
>> +} nct_chips[] = {
>> + {WBSIO_ID_NCT6775F, "NCT6775F"},
>> + {WBSIO_ID_NCT6776F, "NCT6776F"},
>> + {WBSIO_ID_NCT5104D, "NCT5104D or 610[246]D"},
>
> The existing code prints this as just NCT5104D, so I would leave it
> at that, but I don't think we actually need this struct (see below).
>
>> + {WBSIO_ID_NCT6779D, "NCT6779D"},
>> + {WBSIO_ID_NCT6791D, "NCT6791D"},
>> + {WBSIO_ID_NCT6792D, "NCT6792D"},
>> + {WBSIO_ID_NCT6793D, "NCT6793D"},
>> + {WBSIO_ID_NCT6795D, "NCT6795D"},
>> +};
>> +
>> @@ -489,6 +541,7 @@ def_match(struct lm_softc *sc)
>> int
>> wb_match(struct lm_softc *sc)
>> {
>> + const char *model = NULL;
>> int banksel, vendid, devid;
>>
>> /* Read vendor ID */
>> @@ -526,12 +579,46 @@ wb_match(struct lm_softc *sc)
>> lm_setup_sensors(sc, w83627ehf_sensors);
>> break;
>> case WB_CHIPID_W83627DHG:
>> - if (sc->sioid == WBSIO_ID_NCT6776F) {
>> - printf(": NCT6776F\n");
>> - lm_setup_sensors(sc, nct6776f_sensors);
>> + model = wb_nct67xx_id2str(sc->sioid);
>
> Here we're just using 'model' to decide if it is a chip we know about or
> not - the value of model is never printed because we're just printf()ing
> the chip name inside the switch cases.
>
> The way this 'case WB_CHIPID_W83627DHG' flows is straightforward: If it
> is a NCTXXXX that we know about, then we call the appropriate
> lm_setup_sensors() function; otherwise we fall into the else {} case and
> treat it as a W83627DHG. So we can just organize this as a switch on
> sc->sioid where we handle each of the WBSIO_ID_NCTXXXXY cases, and the
> default case is just the else clause where we treat it as a W83627DHG.
> If we do this, then we can actually remove wb_nct67xx_id2str() and the
> nct_chips struct, which we weren't really using other than deciding if
> it is a chip we know about.
>
>> + if (model != NULL) {
>> + switch (sc->sioid) {
>> + case WBSIO_ID_NCT6775F:
>> + printf(": NCT6775F\n");
>> + lm_setup_sensors(sc, nct6776f_sensors);
>> + break;
>> + case WBSIO_ID_NCT6776F:
>
> Indentation on this case and the one below is off.
>
>> + printf(": NCT6776F\n");
>> + lm_setup_sensors(sc, nct6776f_sensors);
>> + break;
>> + case WBSIO_ID_NCT5104D:
>> + printf(": NCT5104D\n");
>> + lm_setup_sensors(sc, nct6776f_sensors);
>> + break;
>> + case WBSIO_ID_NCT6779D:
>> + printf(": NCT6779D\n");
>> + lm_setup_sensors(sc, nct6779d_sensors);
>> + break;
>> + case WBSIO_ID_NCT6791D:
>> + printf(": NCT6791D\n");
>> + lm_setup_sensors(sc, nct6779d_sensors);
>> + break;
>> + case WBSIO_ID_NCT6792D:
>> + printf(": NCT6792D\n");
>> + lm_setup_sensors(sc, nct6779d_sensors);
>> + break;
>> + case WBSIO_ID_NCT6793D:
>> + printf(": NCT6793D\n");
>> + lm_setup_sensors(sc, nct6779d_sensors);
>> + break;
>> + case WBSIO_ID_NCT6795D:
>> + printf(": NCT6795D\n");
>> + lm_setup_sensors(sc, nct6779d_sensors);
>> + break;
>> + }
>> } else {
>> printf(": W83627DHG\n");
>> lm_setup_sensors(sc, w83627dhg_sensors);
>> + break;
>> }
>> break;
>> case WB_CHIPID_W83637HF:
>
> Thanks for taking the time to send this in. If you want to tweak the
> stuff above then I can commit it for you, unless someone else has other
> commentary or beats me to it.
>
> Cheers,
> Todd
>
>
--
Joe Gidi
j...@entropicblur.com
"You cannot buy skill." -- Ross SeyfriedIndex: share/man/man4/lm.4
===================================================================
RCS file: /cvs/src/share/man/man4/lm.4,v
retrieving revision 1.25
diff -u -p -r1.25 lm.4
--- share/man/man4/lm.4 16 Jul 2013 16:05:49 -0000 1.25
+++ share/man/man4/lm.4 16 Dec 2019 16:05:53 -0000
@@ -82,7 +82,7 @@ National Semiconductor LM79
.It
National Semiconductor LM81
.It
-Nuvoton NCT6776F
+Nuvoton NCT6775F, NCT6776F, NCT6779D, NCT6791D, NCT6792D, NCT6793D, NCT6795D
.It
Winbond W83627HF, W83627THF, W83637HF and W83697HF
.It
Index: sys/dev/ic/lm78.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/lm78.c,v
retrieving revision 1.24
diff -u -p -r1.24 lm78.c
--- sys/dev/ic/lm78.c 14 Mar 2015 03:38:47 -0000 1.24
+++ sys/dev/ic/lm78.c 16 Dec 2019 16:05:53 -0000
@@ -215,6 +215,43 @@ struct lm_sensor nct6776f_sensors[] = {
{ NULL }
};
+/* NCT6779D */
+struct lm_sensor nct6779d_sensors[] = {
+ /* Voltage */
+ { "VCore", SENSOR_VOLTS_DC, 4, 0x80, lm_refresh_volt, RFACT_NONE },
+ { "VIN1", SENSOR_VOLTS_DC, 4, 0x81, lm_refresh_volt, RFACT(56, 10) / 2 },
+ { "AVCC", SENSOR_VOLTS_DC, 4, 0x82, lm_refresh_volt, RFACT(34, 34) / 2 },
+ { "+3.3V", SENSOR_VOLTS_DC, 4, 0x83, lm_refresh_volt, RFACT(34, 34) / 2 },
+ { "VIN0", SENSOR_VOLTS_DC, 4, 0x84, lm_refresh_volt, RFACT(48600, 10000) },
+ { "VIN8", SENSOR_VOLTS_DC, 4, 0x85, lm_refresh_volt, RFACT_NONE / 2 },
+ { "VIN4", SENSOR_VOLTS_DC, 4, 0x86, lm_refresh_volt, RFACT_NONE / 2 },
+ { "+3.3VSB", SENSOR_VOLTS_DC, 4, 0x87, lm_refresh_volt, RFACT(34, 34) / 2 },
+ { "VBAT", SENSOR_VOLTS_DC, 4, 0x88, lm_refresh_volt, RFACT_NONE },
+ { "VTT", SENSOR_VOLTS_DC, 4, 0x89, lm_refresh_volt, RFACT_NONE },
+ { "VIN5", SENSOR_VOLTS_DC, 4, 0x8a, lm_refresh_volt, RFACT_NONE / 2 },
+ { "VIN6", SENSOR_VOLTS_DC, 4, 0x8b, lm_refresh_volt, RFACT_NONE / 2 },
+ { "VIN2", SENSOR_VOLTS_DC, 4, 0x8c, lm_refresh_volt, RFACT_NONE },
+ { "VIN3", SENSOR_VOLTS_DC, 4, 0x8d, lm_refresh_volt, RFACT(14414, 10000) },
+ { "VIN7", SENSOR_VOLTS_DC, 4, 0x8e, lm_refresh_volt, RFACT_NONE / 2 },
+
+ /* Temperature */
+ { "MB Temperature", SENSOR_TEMP, 4, 0x90, lm_refresh_temp, 0 },
+ { "CPU Temperature", SENSOR_TEMP, 4, 0x91, wb_refresh_temp, 0 },
+ { "Aux Temp0", SENSOR_TEMP, 4, 0x92, wb_refresh_temp, 0 },
+ { "Aux Temp1", SENSOR_TEMP, 4, 0x93, wb_refresh_temp, 0 },
+ { "Aux Temp2", SENSOR_TEMP, 4, 0x94, wb_refresh_temp, 0 },
+ { "Aux Temp3", SENSOR_TEMP, 4, 0x95, wb_refresh_temp, 0 },
+
+ /* Fans */
+ { "System Fan", SENSOR_FANRPM, 4, 0xc0, wb_nct6776f_refresh_fanrpm, 0 },
+ { "CPU Fan", SENSOR_FANRPM, 4, 0xc2, wb_nct6776f_refresh_fanrpm, 0 },
+ { "Aux Fan0", SENSOR_FANRPM, 4, 0xc4, wb_nct6776f_refresh_fanrpm, 0 },
+ { "Aux Fan1", SENSOR_FANRPM, 4, 0xc6, wb_nct6776f_refresh_fanrpm, 0 },
+ { "Aux Fan2", SENSOR_FANRPM, 4, 0xc8, wb_nct6776f_refresh_fanrpm, 0 },
+
+ { NULL }
+};
+
struct lm_sensor w83637hf_sensors[] = {
/* Voltage */
{ "VCore", SENSOR_VOLTS_DC, 0, 0x20, wb_w83637hf_refresh_vcore },
@@ -526,10 +563,40 @@ wb_match(struct lm_softc *sc)
lm_setup_sensors(sc, w83627ehf_sensors);
break;
case WB_CHIPID_W83627DHG:
- if (sc->sioid == WBSIO_ID_NCT6776F) {
+ switch (sc->sioid) {
+ case WBSIO_ID_NCT6775F:
+ printf(": NCT6775F\n");
+ lm_setup_sensors(sc, nct6776f_sensors);
+ break;
+ case WBSIO_ID_NCT6776F:
printf(": NCT6776F\n");
lm_setup_sensors(sc, nct6776f_sensors);
- } else {
+ break;
+ case WBSIO_ID_NCT5104D:
+ printf(": NCT5104D\n");
+ lm_setup_sensors(sc, nct6776f_sensors);
+ break;
+ case WBSIO_ID_NCT6779D:
+ printf(": NCT6779D\n");
+ lm_setup_sensors(sc, nct6779d_sensors);
+ break;
+ case WBSIO_ID_NCT6791D:
+ printf(": NCT6791D\n");
+ lm_setup_sensors(sc, nct6779d_sensors);
+ break;
+ case WBSIO_ID_NCT6792D:
+ printf(": NCT6792D\n");
+ lm_setup_sensors(sc, nct6779d_sensors);
+ break;
+ case WBSIO_ID_NCT6793D:
+ printf(": NCT6793D\n");
+ lm_setup_sensors(sc, nct6779d_sensors);
+ break;
+ case WBSIO_ID_NCT6795D:
+ printf(": NCT6795D\n");
+ lm_setup_sensors(sc, nct6779d_sensors);
+ break;
+ default:
printf(": W83627DHG\n");
lm_setup_sensors(sc, w83627dhg_sensors);
}
Index: sys/dev/ic/lm78var.h
===================================================================
RCS file: /cvs/src/sys/dev/ic/lm78var.h,v
retrieving revision 1.18
diff -u -p -r1.18 lm78var.h
--- sys/dev/ic/lm78var.h 15 Mar 2016 20:50:22 -0000 1.18
+++ sys/dev/ic/lm78var.h 16 Dec 2019 16:05:53 -0000
@@ -123,7 +123,7 @@
#define WB_VREF 3600
#define WB_W83627EHF_VREF 2048
-#define WB_MAX_SENSORS 19
+#define WB_MAX_SENSORS 36
struct lm_softc;
Index: sys/dev/isa/wbsio.c
===================================================================
RCS file: /cvs/src/sys/dev/isa/wbsio.c,v
retrieving revision 1.10
diff -u -p -r1.10 wbsio.c
--- sys/dev/isa/wbsio.c 14 Mar 2015 03:38:47 -0000 1.10
+++ sys/dev/isa/wbsio.c 16 Dec 2019 16:05:53 -0000
@@ -110,8 +110,12 @@ wbsio_probe(struct device *parent, void
case WBSIO_ID_W83627UHG:
case WBSIO_ID_W83637HF:
case WBSIO_ID_W83697HF:
+ case WBSIO_ID_NCT6775F:
case WBSIO_ID_NCT6776F:
case WBSIO_ID_NCT5104D:
+ case WBSIO_ID_NCT6779D:
+ case WBSIO_ID_NCT6791D:
+ case WBSIO_ID_NCT6792D:
ia->ipa_nio = 1;
ia->ipa_io[0].length = WBSIO_IOSIZE;
ia->ipa_nmem = 0;
@@ -170,8 +174,26 @@ wbsio_attach(struct device *parent, stru
case WBSIO_ID_W83697HF:
printf(": W83697HF");
break;
+ case WBSIO_ID_NCT6775F:
+ printf(": NCT6775F");
+ break;
case WBSIO_ID_NCT6776F:
printf(": NCT6776F");
+ break;
+ case WBSIO_ID_NCT6779D:
+ printf(": NCT6779D");
+ break;
+ case WBSIO_ID_NCT6791D:
+ printf(": NCT6791D");
+ break;
+ case WBSIO_ID_NCT6792D:
+ printf(": NCT6792D");
+ break;
+ case WBSIO_ID_NCT6793D:
+ printf(": NCT6793D");
+ break;
+ case WBSIO_ID_NCT6795D:
+ printf(": NCT6795D");
break;
case WBSIO_ID_NCT5104D:
printf(": NCT5104D");
Index: sys/dev/isa/wbsioreg.h
===================================================================
RCS file: /cvs/src/sys/dev/isa/wbsioreg.h,v
retrieving revision 1.4
diff -u -p -r1.4 wbsioreg.h
--- sys/dev/isa/wbsioreg.h 2 Jan 2015 23:02:54 -0000 1.4
+++ sys/dev/isa/wbsioreg.h 16 Dec 2019 16:05:53 -0000
@@ -42,8 +42,14 @@
#define WBSIO_ID_W83627SF 0x59
#define WBSIO_ID_W83637HF 0x70
#define WBSIO_ID_W83697HF 0x60
+#define WBSIO_ID_NCT6775F 0xb4
#define WBSIO_ID_NCT6776F 0xc3
#define WBSIO_ID_NCT5104D 0xc4
+#define WBSIO_ID_NCT6779D 0xc5
+#define WBSIO_ID_NCT6791D 0xc8
+#define WBSIO_ID_NCT6792D 0xc9
+#define WBSIO_ID_NCT6793D 0xd1
+#define WBSIO_ID_NCT6795D 0xd3
/* Logical Device Number (LDN) Assignments */
#define WBSIO_LDN_HM 0x0b
