[it8718f] PWM fan control does not work

Tested on both the ga-g41m-es2l and the ga-945gcm-s2l: The fan runs at a fixed level, no matter the system load.

On the ga-g41m-es2l, the hardware monitor chip cannot be accessed safely. Running sudo modprobe it87 will hard lock up the system after ~10 seconds. During these 10 seconds, all sensor output is shown and seems to be correct.

On the ga-945gcm-s2l, on the other hand, loading the it87 module does nothing. No sensor output is shown, but the board also doesn't crash.

[ArthurHeymans]

Hooked up the common ite Environment-Controller driver to it8718f: https://review.coreboot.org/#/c/17581/
What should now be done is to find a nice configuration for it and put that in devicetree.

[zamaudio]

Nice, does it work on both boards? Let's get this upstreamed!

Tested on ga-945gcm-s2l. Working for the most part.

lm-sensors now outputs it8718-isa-0290 values that look sane to me.

I do get this: it87 it87.656: Detected broken BIOS defaults, disabling PWM interface

Fan speed still seems static around 900RPM, regardless of system load.

[ArthurHeymans]

What left to be done is configuring the fan<->sensors and setting it to smart-magic-auto-orsomethinglikethat (modes have funky names on ite EC) and treshholds. It is also possible to hook up a CPU sensors to SIO using PECI, which is probably desired.

[zamaudio]

Here's a decompiled g41m-es2l superiotool dump for vendor bios EC region, will provide the blobtool spec file on wiki:

# AUTOGENERATED SETTER BY BLOBTOOL
{
	"conf00_start" = 0x1,
	"conf00_smien" = 0x1,
	"conf00_irqen" = 0x0,
	"conf00_irqclr" = 0x0,
	"conf00_ro_one" = 0x1,
	"conf00_copen" = 0x0,
	"conf00_vbat" = 0x0,
	"conf00_initreset" = 0x0,
	"irq1_maxfantac1" = 0x0,
	"irq1_maxfantac2" = 0x0,
	"irq1_maxfantac3" = 0x0,
	"irq1_maxfantac4" = 0x0,
	"irq1_copen" = 0x1,
	"irq1_reserved0" = 0x0,
	"irq1_maxfantac5" = 0x0,
	"irq1_reserved1" = 0x0,
	"irq2_limit_vin0" = 0x0,
	"irq2_limit_vin1" = 0x0,
	"irq2_limit_vin2" = 0x0,
	"irq2_limit_vin3" = 0x0,
	"irq2_limit_vin4" = 0x0,
	"irq2_limit_vin5" = 0x0,
	"irq2_limit_vin6" = 0x0,
	"irq2_limit_vin7" = 0x0,
	"irq3_limit_temp1" = 0x0,
	"irq3_limit_temp2" = 0x0,
	"irq3_limit_temp3" = 0x0,
	"irq3_reserved" = 0x0,
	"smi1_dis_fantac1" = 0x1,
	"smi1_dis_fantac2" = 0x1,
	"smi1_dis_fantac3" = 0x1,
	"smi1_dis_fantac4" = 0x1,
	"smi1_dis_copen" = 0x1,
	"smi1_reserved0" = 0x1,
	"smi1_dis_fantac5" = 0x1,
	"smi1_reserved1" = 0x1,
	"smi2_dis_vin0" = 0x1,
	"smi2_dis_vin1" = 0x1,
	"smi2_dis_vin2" = 0x1,
	"smi2_dis_vin3" = 0x1,
	"smi2_dis_vin4" = 0x0,
	"smi2_dis_vin5" = 0x1,
	"smi2_dis_vin6" = 0x1,
	"smi2_dis_vin7" = 0x1,
	"smi3_dis_temp1" = 0x0,
	"smi3_dis_temp2" = 0x0,
	"smi3_dis_temp3" = 0x0,
	"smi3_reserved" = 0x0,
	"irqmask1_fantac1" = 0x1,
	"irqmask1_fantac2" = 0x1,
	"irqmask1_fantac3" = 0x1,
	"irqmask1_fantac4" = 0x0,
	"irqmask1_copen" = 0x1,
	"irqmask1_reserved0" = 0x1,
	"irqmask1_fantac5" = 0x0,
	"irqmask1_reserved1" = 0x0,
	"irqmask2_vin0" = 0x1,
	"irqmask2_vin1" = 0x1,
	"irqmask2_vin2" = 0x1,
	"irqmask2_vin3" = 0x1,
	"irqmask2_vin4" = 0x1,
	"irqmask2_vin5" = 0x1,
	"irqmask2_vin6" = 0x1,
	"irqmask2_vin7" = 0x1,
	"irqmask3_temp1" = 0x1,
	"irqmask3_temp2" = 0x1,
	"irqmask3_temp3" = 0x1,
	"irqmask3_reserved" = 0x0,
	"irqmask3_extsensor" = 0x1,
	"iface_reserved" = 0x4,
	"iface_extsensor_select" = 0x5,
	"iface_pseudo_eoc" = 0x0,
	"fanpwm_reserved" = 0x9,
	"fanpwm_smoothing_step" = 0x0,
	"fantach16_en_tac1" = 0x1,
	"fantach16_en_tac2" = 0x1,
	"fantach16_en_tac3" = 0x1,
	"fantach16_tmpin1_enh" = 0x0,
	"fantach16_en_tac4" = 0x0,
	"fantach16_en_tac5" = 0x0,
	"fantach16_tmpin2_enh" = 0x0,
	"fantach16_tmpin3_enh" = 0x0,
	"fantach_lo_counts1" = 0x3e,
	"fantach_lo_counts2" = 0xff,
	"fantach_lo_counts3" = 0x0,
	"fantach_lo_limit1" = 0xff,
	"fantach_lo_limit2" = 0xff,
	"fantach_lo_limit3" = 0xff,
	"fanctlmain_mode1" = 0x0,
	"fanctlmain_mode2" = 0x1,
	"fanctlmain_mode3" = 0x1,
	"fanctlmain_reserved0" = 0x0,
	"fanctlmain_en_tac1" = 0x1,
	"fanctlmain_en_tac2" = 0x1,
	"fanctlmain_en_tac3" = 0x0,
	"fanctlmain_reserved1" = 0x0,
	"fanctl_enable1" = 0x1,
	"fanctl_enable2" = 0x1,
	"fanctl_enable3" = 0x1,
	"fanctl_minduty_sel" = 0x0,
	"fanctl_pwm_base_clock" = 0x5,
	"fanctl_allpolarity" = 0x1,
	"fanctl1_tmpin_sel" = 0x2,
	"fanctl1_steps" = 0x1f,
	"fanctl1_pwm_mode" = 0x1,
	"fanctl2_tmpin_sel" = 0x3,
	"fanctl2_steps" = 0x1f,
	"fanctl2_pwm_mode" = 0x0,
	"fanctl3_tmpin_sel" = 0x2,
	"fanctl3_steps" = 0x10,
	"fanctl3_pwm_mode" = 0x1,
	"fantach_hi_counts1" = 0x2,
	"fantach_hi_counts2" = 0xff,
	"fantach_hi_counts3" = 0x0,
	"fantach_hi_limit1" = 0xff,
	"fantach_hi_limit2" = 0xff,
	"fantach_hi_limit3" = 0xff,
	"reserved1e" = 0x0,
	"reserved1f" = 0x0,
	"vin0" = 0x43,
	"vin1" = 0x76,
	"vin2" = 0xcb,
	"vin3" = 0xb8,
	"vin4" = 0x11,
	"vin5" = 0x80,
	"vin6" = 0x8b,
	"vin7" = 0xc0,
	"vbat" = 0xbb,
	"tmpin1" = 0xc9,
	"tmpin2" = 0xfe,
	"tmpin3" = 0x12,
	"reserved2c" = 0x0,
	"reserved2d" = 0x0,
	"reserved2e" = 0x0,
	"reserved2f" = 0x0,
	"limit_hi_vin0" = 0xff,
	"limit_lo_vin0" = 0x0,
	"limit_hi_vin1" = 0xff,
	"limit_lo_vin1" = 0x0,
	"limit_hi_vin2" = 0xff,
	"limit_lo_vin2" = 0x0,
	"limit_hi_vin3" = 0xff,
	"limit_lo_vin3" = 0x0,
	"limit_hi_vin4" = 0x83,
	"limit_lo_vin4" = 0x0,
	"limit_hi_vin5" = 0xff,
	"limit_lo_vin5" = 0x0,
	"limit_hi_vin6" = 0xff,
	"limit_lo_vin6" = 0x0,
	"limit_hi_vin7" = 0xff,
	"limit_lo_vin7" = 0x0,
	"limit_hi_tmpin1" = 0x7f,
	"limit_lo_tmpin1" = 0x7f,
	"limit_hi_tmpin2" = 0x7f,
	"limit_lo_tmpin2" = 0x7f,
	"limit_hi_tmpin3" = 0x7f,
	"limit_lo_tmpin3" = 0x7f,
	"reserved46" = 0x0,
	"reserved47" = 0x0,
	"reserved48" = 0x0,
	"reserved49" = 0x0,
	"reserved4a" = 0x0,
	"reserved4b" = 0x0,
	"reserved4c" = 0x0,
	"reserved4d" = 0x0,
	"reserved4e" = 0x0,
	"reserved4f" = 0x0,
	"adc_scan_enable_vin0" = 0x1,
	"adc_scan_enable_vin1" = 0x1,
	"adc_scan_enable_vin2" = 0x1,
	"adc_scan_enable_vin3" = 0x1,
	"adc_scan_enable_vin4" = 0x1,
	"adc_scan_enable_vin5" = 0x0,
	"adc_scan_enable_vin6" = 0x0,
	"adc_scan_enable_vin7" = 0x1,
	"therm_diode_tmpin1" = 0x0,
	"therm_diode_tmpin2" = 0x0,
	"therm_diode_tmpin3" = 0x1,
	"therm_resistor_tmpin1" = 0x1,
	"therm_resistor_tmpin2" = 0x1,
	"therm_resistor_tmpin3" = 0x0,
	"therm_reserved" = 0x0,
	"therm_limit_tmpin1" = 0x7f,
	"therm_limit_tmpin2" = 0x7f,
	"therm_limit_tmpin3" = 0x7f,
	"therm_resistor_vin4" = 0x0,
	"therm_resistor_vin5" = 0x0,
	"therm_resistor_vin6" = 0x0,
	"adc_fanctl2_pwm_duty" = 0x0,
	"adc_fanctl2_pwm_bclk" = 0x0,
	"adc_tmpin3_ext_select" = 0x0,
	"thermal_zero_diode1" = 0xf6,
	"thermal_zero_diode2" = 0xf6,
	"ite_vendor_id" = 0x90,
	"thermal_zero_diode3" = 0xf6,
	"reserved5a" = 0x0,
	"ite_code_id" = 0x12,
	"beep_fantac" = 0x0,
	"beep_vin" = 0x0,
	"beep_tmpin" = 0x0,
	"beep_reserved" = 0x0,
	"adc_clock_select" = 0x6,
	"thermal_zero_adj_en" = 0x0,
	"beep_fan_freq_div" = 0x0,
	"beep_fan_tone_div" = 0x0,
	"beep_volt_freq_div" = 0x0,
	"beep_volt_tone_div" = 0x0,
	"beep_temp_freq_div" = 0x0,
	"beep_temp_tone_div" = 0x0,
	"sguard1_temp_lim_off" = 0x0,
	"sguard1_temp_lim_fan" = 0x14,
	"reserved62" = 0x41,
	"sguard1_pwm_start" = 0x23,
	"sguard1_pwm_slope6" = 0x0,
	"sguard1_pwm_slope05" = 0x10,
	"sguard1_pwm_reserved" = 0x0,
	"sguard1_fan_smooth_en" = 0x1,
	"sguard1_temp_interval" = 0x3,
	"sguard1_temp_reserved" = 0x0,
	"sguard1_temp_pwm_lin" = 0x0,
	"reserved66" = 0x0,
	"reserved67" = 0x0,
	"sguard2_temp_lim_off" = 0x7f,
	"sguard2_temp_lim_fan" = 0x7f,
	"reserved6a" = 0x7f,
	"sguard2_pwm_start" = 0x0,
	"sguard2_pwm_slope6" = 0x0,
	"sguard2_pwm_slope05" = 0x0,
	"sguard2_pwm_reserved" = 0x0,
	"sguard2_fan_smooth_en" = 0x0,
	"sguard2_temp_interval" = 0x1f,
	"sguard2_temp_reserved" = 0x3,
	"sguard2_temp_pwm_lin" = 0x0,
	"reserved6e" = 0x0,
	"reserved6f" = 0x0,
	"sguard3_temp_lim_off" = 0x0,
	"sguard3_temp_lim_fan" = 0x14,
	"reserved72" = 0x41,
	"sguard3_pwm_start" = 0x23,
	"sguard3_pwm_slope6" = 0x0,
	"sguard3_pwm_slope05" = 0x10,
	"sguard3_pwm_reserved" = 0x0,
	"sguard3_fan_smooth_en" = 0x1,
	"sguard3_temp_interval" = 0x3,
	"sguard3_temp_reserved" = 0x0,
	"sguard3_temp_pwm_lin" = 0x0,
	"reserved76" = 0x0,
	"reserved77" = 0x0,
	"reserved78" = 0x0,
	"reserved79" = 0x0,
	"reserved7a" = 0x0,
	"reserved7b" = 0x0,
	"reserved7c" = 0x0,
	"reserved7d" = 0x0,
	"reserved7e" = 0x0,
	"reserved7f" = 0x0,
	"fantach_lo_counts4" = 0x0,
	"fantach_hi_counts4" = 0x0,
	"fantach_lo_counts5" = 0x0,
	"fantach_hi_counts5" = 0x0,
	"fantach_lo_limit4" = 0x0,
	"fantach_hi_limit4" = 0x0,
	"fantach_lo_limit5" = 0x0,
	"fantach_hi_limit5" = 0x0,
	"ext_host_busy" = 0x0,
	"ext_host_fnsh" = 0x0,
	"ext_host_r_fcs_error" = 0x0,
	"ext_host_w_fcs_error" = 0x0,
	"ext_host_peci_highz" = 0x0,
	"ext_host_sst_slave" = 0x0,
	"ext_host_sst_bus" = 0x0,
	"ext_host_data_fifo_clr" = 0x0,
	"ext_host_target_addr" = 0x0,
	"ext_host_write_length" = 0x0,
	"ext_host_read_length" = 0x0,
	"ext_host_cmd" = 0x0,
	"ext_host_writedata" = 0x0,
	"ext_hostctl_start" = 0x0,
	"ext_hostctl_sst_amdsi" = 0x1,
	"ext_hostctl_sst_ctl" = 0x0,
	"ext_hostctl_resetfifo" = 0x0,
	"ext_hostctl_fcs_abort" = 0x0,
	"ext_hostctl_start_en" = 0x0,
	"ext_hostctl_start_ctl" = 0x0,
	"ext_host_readdata" = 0xc8,
	"fan1_temp_limit_start" = 0xff,
	"fan1_slope_pwm" = 0x0,
	"fan1_temp_input_sel0" = 0x0,
	"fan1_ctlmode_temp_ivl" = 0x0,
	"fan1_ctlmode_target" = 0x0,
	"fan1_temp_input_sel1" = 0x0,
	"reserved93" = 0x0,
	"fan2_temp_limit_start" = 0xff,
	"fan2_slope_pwm" = 0x0,
	"fan2_temp_input_sel0" = 0x0,
	"fan2_ctlmode_temp_ivl" = 0x0,
	"fan2_ctlmode_target" = 0x0,
	"fan2_temp_input_sel1" = 0x0
}

[ArthurHeymans]

So I think the sensors should be configured as vendor bios. The rest like fan settings can be set to automatic where vendor bios sets them to manual.
Example of a devicetree using common EC driver:
https://review.coreboot.org/#/c/17446/1/src/mainboard/roda/rv11/variants/rw11/devicetree.cb

Tested on GA-G41M-ES2L. All issues are fixed.

• Running sudo modprobe it87 no longer locks the system up, and shows sensor output.
• The fan is running at a much lower level when idle than before.
• Increasing system temperature increases the fan speed.
• Decreasing system temperature decreases the fan speed.

[ArthurHeymans]

Should be trivial to port to ga-945gcm-s2l.