Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 19:
(60 comments)
mark remaining comments as done
https://review.coreboot.org/c/coreboot/+/33623/4/src/superio/fintek/common/f... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/c/coreboot/+/33623/4/src/superio/fintek/common/f... PS4, Line 107: fintek_boundaries_size
will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/4/src/superio/fintek/common/f... PS4, Line 115: fintek_sections_size
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/4/src/superio/fintek/common/f... PS4, Line 125: int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections);
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 26: _
Opps.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 23: #define EXTERNAL_SENSOR1 1 : #define EXTERNAL_SENSOR2 2 : #define EXTERNAL_SENSOR3 3 : #define EXTERNAL_SENSOR_4 4
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 27: #define TEMP_SENSOR_THERMISTOR 0 : #define TEMP_SENSOR_BJT 1 : #define TEMP_SENSOR_DEFAULT 2
Will do to all.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 31: #define FAN_TYPE_PWM_PUSH_PULL 0 : #define FAN_TYPE_DAC_POWER 1 : #define FAN_TYPE_PWM_OPEN_DRAIN 2 : #define FAN_TYPE_SET_BY_STRAP 3
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 37: #define FAN_MODE_AUTO_RPM 0 : #define FAN_MODE_AUTO_PWM_DAC 1 : #define FAN_MODE_MANUAL_RPM 2 : #define FAN_MODE_MANUAL_PWM_DAC 3 : #define FAN_MODE_DEFAULT 4
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 42: : #define FAN_PWM_FREQ_23500 0 : #define FAN_PWM_FREQ_11750 1 : #define FAN_PWM_FREQ_5875 2 : #define FAN_PWM_FREQ_220 3
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 48: #define FAN_TEMP_PECI 0 : #define FAN_TEMP_EXTERNAL_1 1 : #define FAN_TEMP_EXTERNAL_2 2 : #define FAN_TEMP_TSI 4 : #define FAN_TEMP_MXM 5
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 54: #define FAN_UP_RATE_2HZ 0 : #define FAN_UP_RATE_5HZ 1 : #define FAN_UP_RATE_10HZ 2 : #define FAN_UP_RATE_20HZ 3 : #define FAN_UP_RATE_DEFAULT 4 : #define FAN_UP_RATE_JUMP 8
same here; last one needs additional explicit value assignment
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 61: #define FAN_DOWN_RATE_2HZ 0 : #define FAN_DOWN_RATE_5HZ 1 : #define FAN_DOWN_RATE_10HZ 2 : #define FAN_DOWN_RATE_20HZ 3 : #define FAN_DOWN_RATE_DEFAULT 4 : #define FAN_DOWN_RATE_SAME_AS_UP 5 : #define FAN_DOWN_RATE_JUMP 8
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 69: #define FAN_FOLLOW_STEP 0 : #define FAN_FOLLOW_INTERPOLATION 1
That's correct, and I have not added code for it. The SIO is capable of it. Working on it now.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 71: : #define HWM_STATUS_SUCCESS 0 : #define HWM_STATUS_INVALID_FAN -1 : #define HWM_STATUS_INVALID_TEMP_SOURCE -2 : #define HWM_STATUS_INVALID_TYPE -3 : #define HWM_STATUS_INVALID_MODE -4 : #define HWM_STATUS_INVALID_RATE -5 : #define HWM_STATUS_INVALID_FREQUENCY -6 : #define HWM_STATUS_INVALID_TEMP_SENSOR -7 : #define HWM_STATUS_INVALID_BOUNDARY_VALUE -8 : #define HWM_STATUS_INVALID_SECTION_VALUE -9 : #define HWM_STATUS_BOUNDARY_WRONG_ORDER -20 : #define HWM_STATUS_SECTIONS_WRONG_ORDER -21 : #define HWM_STATUS_WARNING_SENSOR_DISCONECTED 1 : #define HWM_STATUS_WARNING_FAN_NOT_PWM 2
I'd be ok with that, since i don't see a better, but still easily readable way, right now. […]
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 91: fintek_boundaries_size
Agree, will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 99: fintek_sections_size
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 101: u8 sensor, u8 type
Probably. Same for all other places you marked.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 102: u8 source
enum type?
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 103: u8 type, u8 mode
enum types?
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 104: u8 frequency
enum type?
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 105: u8 *boundaries, u8 *sections
I'd definitely like to have a comment stating this; without a comment this is too easy to use wrongl […]
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/common/f... PS2, Line 106: u8 rate_up, : u8 rate_down
enum types?
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67:
See above, same reason. Will fix.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/f81803a.h:
PS11:
Minor consistency issue: There are some hex values with lowercase letters, and others with uppercase […]
Done
https://review.coreboot.org/c/coreboot/+/33623/8/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/8/src/superio/fintek/f81803a/... PS8, Line 35: #define LDN_REG 0x07 : #define DEVICE_ENABLE_REG 0x30 : #define DEVICE_HIGH_ADDRESS_REG 0x60 : #define DEVICE_LOW_ADDRESS_REG 0x61 : #define DEVICE_IRQ_REG 0x70
Thanks, missed this section.
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 36: #define DEVICE_ENABLE_REG 0x30 : #define DEVICE_HIGH_ADDRESS_REG 0x60 : #define DEVICE_LOW_ADDRESS_REG 0x61 : #define DEVICE_IRQ_REG 0x70
not fintek specific and already defined in pnp_def. […]
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 77: #define FINTEK_ENTRY_KEY 0x87 : #define FINTEK_EXIT_KEY 0xAA
smells like pnp_conf_mode_8787_aa
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 79: #define FINTEK_BASE_ADDRESS 0x4E
Only through power-on strap. By default it's 4e. Was not aware of this capability of devicetree.
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 80: #define FINTEK_INDEX_IO FINTEK_BASE_ADDRESS : #define FINTEK_DATA_IO (FINTEK_BASE_ADDRESS + 1)
generic for most SIO chips, so probably a redefinition
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 87: void fintek_enable_device(u8 ldn, u16 base, u8 irq);
this should be done via devicetree and not by reimplementing functionality
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 35:
alignment
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 37: /* Global Control Registers */
some additional whitespace issues below
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/f81803a_hwm.h:
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 21: #define TP_SENSOR1_TYPE_SHIFT 1 : #define TP_SENSOR2_TYPE_SHIFT 2
TP_SENSOR_TYPE_SHIFT(x)? see my comment below
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 28: #define TP_EXTERNAL_SENSOR2_OPEN 0x04 : #define TP_EXTERNAL_SENSOR1_OPEN 0x02
Will not change this one, those are bit test positions.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 32: #define FAN1_TYPE_SHIFT 0 : #define FAN2_TYPE_SHIFT 2
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 36: #define FAN1_MODE_SHIFT 0 : #define FAN2_MODE_SHIFT 4
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 39: #define FAN1_FREQ_SEL_ADD_SHIFT 3 /* FUNC_PROG_SEL = 1 */ : #define FAN2_FREQ_SEL_ADD_SHIFT 4 /* FUNC_PROG_SEL = 1 */
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 42: #define FAN1_UP_RATE_SHIFT 0 : #define FAN2_UP_RATE_SHIFT 2
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 46: #define FAN1_DOWN_RATE_SHIFT 0 : #define FAN2_DOWN_RATE_SHIFT 2
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 54: #define FAN1_BOUND_TEMP1 0xa6 : #define FAN1_BOUND_TEMP2 0xa7 : #define FAN1_BOUND_TEMP3 0xa8 : #define FAN1_BOUND_TEMP4 0xa9
#define FAN_BOUND_TEMP(fan, temp) (0xa6 + (0x10 * (fan)) + (temp)) […]
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 58: #define FAN1_SECTION_SPEED1 0xaa : #define FAN1_SECTION_SPEED2 0xab : #define FAN1_SECTION_SPEED3 0xac : #define FAN1_SECTION_SPEED4 0xad : #define FAN1_SECTION_SPEED5 0xae
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 63: #define FAN2_BOUND_TEMP1 0xb6 : #define FAN2_BOUND_TEMP2 0xb7 : #define FAN2_BOUND_TEMP3 0xb8 : #define FAN2_BOUND_TEMP4 0xb9
see comment above
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 67: #define FAN2_SECTION_SPEED1 0xba : #define FAN2_SECTION_SPEED2 0xbb : #define FAN2_SECTION_SPEED3 0xbc : #define FAN2_SECTION_SPEED4 0xbd : #define FAN2_SECTION_SPEED5 0xbe
same here
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 72: #define FAN_DETAIL_SKIP (FAN2_BOUND_TEMP1 - FAN1_BOUND_TEMP1)
not needed with my suggestion for this
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 74: #define FAN1_TMP_MAPPING 0xaf : #define FAN2_TMP_MAPPING 0xbf
seem comment above
Done
https://review.coreboot.org/c/coreboot/+/33623/5/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/5/src/superio/fintek/f81803a/... PS5, Line 38: read_and_or_write
hwm_reg_modify maybe? […]
Done
https://review.coreboot.org/c/coreboot/+/33623/5/src/superio/fintek/f81803a/... PS5, Line 44: use_mask = mask << shift; : use_value = (value & mask) << shift;
Ok.
Done
https://review.coreboot.org/c/coreboot/+/33623/5/src/superio/fintek/f81803a/... PS5, Line 51: static inline void set_prog_sel(u16 address, u8 value)
ok. […]
Done
https://review.coreboot.org/c/coreboot/+/33623/5/src/superio/fintek/f81803a/... PS5, Line 57: static int check_value_seq(u8 *values, u8 count)
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/5/src/superio/fintek/f81803a/... PS5, Line 232: 0
ok. […]
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 25: static void f81803a_pme_init(struct device *dev) : { : pnp_enter_conf_mode(dev); : pnp_write_config(dev, LDN_REG, F81803A_PME); : /* enable ERP function*/ : /* also set PSIN to generate PSOUT*/ : pnp_write_config(dev, PME_ERP_ENABLE_REG, ERP_ENABLE | ERP_PSOUT_EN); : pnp_exit_conf_mode(dev); : }
Apparently with any SIO of this family, if you're enabling PME functionality.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 40: or FDC
It does have watch dog timer, so I'll replace FDC with WDT.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 55:
will fix
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 61: /* TODO: Some of the 0x7f8 etc. values may not be correct. */
will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/2/src/superio/fintek/f81803a/... PS2, Line 67: //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 },
This SIO does have it...
Done
https://review.coreboot.org/c/coreboot/+/33623/4/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/c/coreboot/+/33623/4/src/superio/fintek/f81803a/... PS4, Line 55:
Weird, I thought I fixed it. will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 67: //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 },
oh, this one wasn't addressed yet
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 71: read_register
more or less redefinition of pnp_read_config
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 77: write_register
more or less redefinition of pnp_write_config
Done
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 83: fintek_enable_device
is reimplements existing functionality. please use the devicetree instead. […]
Done