Page MenuHomeFreeBSD

Add ITE 8613 to superio
Needs ReviewPublic

Authored by jo_bruelltuete.com on Sep 2 2022, 12:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jan 16, 10:44 AM
Unknown Object (File)
Sat, Jan 7, 4:54 AM
Unknown Object (File)
Thu, Jan 5, 9:18 AM
Unknown Object (File)
Tue, Jan 3, 4:00 AM
Unknown Object (File)
Dec 24 2022, 4:39 AM
Unknown Object (File)
Dec 15 2022, 7:46 PM
Unknown Object (File)
Dec 14 2022, 3:37 AM
Unknown Object (File)
Dec 1 2022, 12:19 PM
Subscribers

Details

Reviewers
avg
Summary

Add ITE 8613 so that superio attaches to that.
Add driver for its HWM to read temperature and fan speed.

Test Plan

I have a mainboard here with this chip on it. Gives reasonable numbers.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

jo_bruelltuete.com added inline comments.
sys/dev/superio/superio.c
273

i only know of the hardware monitor for my board.

sys/dev/superio/superio.c
556–558

is this still true? see line 448.

$ devinfo -r
[...]
        isab0
          isa0
            superio0
                ACPI I/O ports:
                    0x2e-0x2f
              it8613hwm0
                  ACPI I/O ports:
                      0xa35-0xa36
[...]

$ tail /var/log/messages
kernel: it8613hwm0: it8613hwm_probe
kernel: it8613hwm0: it8613hwm_probe
kernel: it8613hwm0: <Hardware monitor on ITE SuperIO> at HWM ldn 0x04 on superio0
kernel: it8613hwm0: it8613hwm_attach
kernel: it8613hwm0: it8613hwm iobase: 2608

$ sysctl dev.it8613hwm
dev.it8613hwm.0.temperature2: 27C
dev.it8613hwm.0.temperature1: 41C
dev.it8613hwm.0.temperature0: 51C
dev.it8613hwm.0.%parent: superio0
dev.it8613hwm.0.%pnpinfo: type=HWM
dev.it8613hwm.0.%location: ldn=0x04
dev.it8613hwm.0.%driver: it8613hwm
dev.it8613hwm.0.%desc: Hardware monitor on ITE SuperIO
dev.it8613hwm.%parent:

mostly copy-pasta'd together from itwd and other drivers already in source.

Is this the right thing to do?
I wrote a sbin tool that opens /dev/io and does raw io to talk to the chip. Now turning that into a driver that exposes the system info as sysctl seems reasonable to me... but maybe this should go somewhere else? I'm looking for opinions.

The superio.c change looks mostly good, but I think that you can use ite_devices for your chip as well.
Most likely it does have the watchdog component.
Also, you were correct about the comment and assert on the possible vendor IDs. Those were not updated when fintek support was added.

Regarding the new HWM driver, I haven't thoroughly reviewed it yet, but from a quick look it has many deviations from FreeBSD code.
You can read style(9) man page and look at the existing code.
Things like:

  • too long lines
  • indentation for continuation lines
  • placement of opening braces { in conditional blocks
  • variable definitions as a separate block
  • debugging printf-s should be removed or conditional
This comment was removed by avg.

thanks for taking a look.

yeah lots of style issues. i'll fix those.
but otherwise no objection to this being a kmod and putting the temp/fan/etc into sysctls?

jo_bruelltuete.com edited the summary of this revision. (Show Details)
jo_bruelltuete.com edited the test plan for this revision. (Show Details)

add fan speed

$ sysctl dev.it8613hwm
dev.it8613hwm.0.fan2: 1470
dev.it8613hwm.0.fan1: 648
dev.it8613hwm.0.fan0: 0
dev.it8613hwm.0.temperature2: 35C
dev.it8613hwm.0.temperature1: 40C
dev.it8613hwm.0.temperature0: 49C
dev.it8613hwm.0.%parent: superio0
dev.it8613hwm.0.%pnpinfo: type=HWM
dev.it8613hwm.0.%location: ldn=0x04
dev.it8613hwm.0.%driver: it8613hwm
dev.it8613hwm.0.%desc: Hardware monitor on ITE SuperIO
dev.it8613hwm.%parent:

$ sysctl -a | grep temp
[...]
hw.acpi.thermal.tz0.temperature: 25.1C
dev.it8613hwm.0.temperature2: 35C
dev.it8613hwm.0.temperature1: 40C
dev.it8613hwm.0.temperature0: 49C
dev.jedec_dimm.1.temp: 37.6C
dev.jedec_dimm.0.temp: 37.2C
dev.amdtemp.0.ccd0: 34.6C
dev.amdtemp.0.core0.sensor0: 35.6C
dev.amdtemp.0.sensor_offset: 0
[...]
dev.cpu.0.temperature: 35.6C
[...]

Notice that acpi says the temp is 10 deg lower. Looks like it does not take into account the sensor offset. (Or the BIOS is botched... not unlikely tbh...)

Notice that acpi says the temp is 10 deg lower. Looks like it does not take into account the sensor offset. (Or the BIOS is botched... not unlikely tbh...)

So checking my boards ASL and acpi_thermal, looks like access to those hardware monitor registers are completely unsynchronised. That can be a problem: if we allow an unpriviledged user to initiate a read here that could glitch a concurrently running acpi_thermal update. What exactly will happen depends on the BIOS (in the case of my board a user could cause some gpio functions to be triggered, no idea whats hanging off those, maybe just emergency shutdown).

hi @avg , anything else i should change here? good to commit?

fredgraph.png (613×3 px, 146 KB)

works pretty well, here's a grafana screenshot of the numbers that this driver spits out.

ping @avg or @imp: any thoughts here?
works fine for me. Most likely works with any IT87 chip (because thats the datasheet I found), but i only have it8613 to actually test on.