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
