Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33623
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 9 files changed, 820 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/1
diff --git a/src/superio/fintek/Makefile.inc b/src/superio/fintek/Makefile.inc index 796e519..db683fd 100644 --- a/src/superio/fintek/Makefile.inc +++ b/src/superio/fintek/Makefile.inc @@ -26,3 +26,4 @@ subdirs-y += f81216h subdirs-y += f81865f subdirs-y += f81866d +subdirs-y += f81803a diff --git a/src/superio/fintek/common/fan_control.h b/src/superio/fintek/common/fan_control.h new file mode 100644 index 0000000..8828c4b --- /dev/null +++ b/src/superio/fintek/common/fan_control.h @@ -0,0 +1,109 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Richard Spiegel richard.spiegel@silverbackltd.com + * Copyright (C) 2019 Silverback ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SUPERIO_FINTEK_FAN_CONTROL_H +#define SUPERIO_FINTEK_FAN_CONTROL_H + +#include <stdint.h> +#include <arch/io.h> + +#define EXTERNAL_SENSOR1 1 +#define EXTERNAL_SENSOR2 2 +#define EXTERNAL_SENSOR3 3 +#define EXTERNAL_SENSOR_4 4 +#define TEMP_SENSOR_THERMISTOR 0 +#define TEMP_SENSOR_BJT 1 +#define TEMP_SENSOR_DEFAULT 2 + +#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 +#define FAN_TYPE_PWM_CHECK 1 /* bit 0 must be 0 for PWM */ + +#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 + +#define FAN_PWM_FREQ_23500 0 +#define FAN_PWM_FREQ_11750 1 +#define FAN_PWM_FREQ_5875 2 +#define FAN_PWM_FREQ_220 3 + +#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 + +#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 + +#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 + +#define FAN_FOLLOW_STEP 0 +#define FAN_FOLLOW_INTERPOLATION 1 + +#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 + +/* + * Boundaries order is from highest temp. to lowest. Values from 0 to 127. + * Boundaries should be defined as u8 boundaries[fintek_boundaries_size]. + */ +#define fintek_boundaries_size 4 +/* + * Section defines the duty_cycle/voltage to be used based on where the + * temperature lies with respect to the boundaries. There are 5 sections + * (4 boundaries) and the order must be from highest to lowest. Values + * from 0% to 100%, will be converted internally to percent of 255. + * Sections should be defined as u8 sections[fintek_sections_size]. + */ +#define fintek_sections_size 5 + +int set_sensor_type(u16 base_address, u8 sensor, u8 type); +int set_fan_temperature_source(u16 base_address, u8 fan, u8 source); +int set_fan_type_mode(u16 base_address, u8 fan, u8 type, u8 mode); +int set_pwm_frequency(u16 base_address, u8 fan, u8 frequency); +int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections); +int set_fan_speed_change_rate(u16 base_address, u8 fan, u8 rate_up, + u8 rate_down); + +#endif /* SUPERIO_FINTEK_FAN_CONTROL_H */ diff --git a/src/superio/fintek/f81803a/Kconfig b/src/superio/fintek/f81803a/Kconfig new file mode 100644 index 0000000..f1f6ef3 --- /dev/null +++ b/src/superio/fintek/f81803a/Kconfig @@ -0,0 +1,24 @@ +## +## This file is part of the coreboot project. +## +## Copyright (C) 2009 Ronald G. Minnich +## Copyright (C) 2014 Edward O'Callaghan eocallaghan@alterapraxis.com +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +config SUPERIO_FINTEK_F81803A + bool + select SUPERIO_FINTEK_COMMON_PRE_RAM + +config SUPERIO_FINTEK_F81803A_FAN_CONTROL + bool + depends on SUPERIO_FINTEK_COMMON_PRE_RAM + default n diff --git a/src/superio/fintek/f81803a/Makefile.inc b/src/superio/fintek/f81803a/Makefile.inc new file mode 100644 index 0000000..a953b4a --- /dev/null +++ b/src/superio/fintek/f81803a/Makefile.inc @@ -0,0 +1,24 @@ +## +## This file is part of the coreboot project. +## +## Copyright (C) 2011 Advanced Micro Devices, Inc. +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; either version 2 of the License, or +## (at your option) any later version. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +ifeq ($(CONFIG_BOOTBLOCK_CONSOLE),y) +bootblock-$(CONFIG_SUPERIO_FINTEK_F81803A) += ../common/early_serial.c +endif + +romstage-$(CONFIG_SUPERIO_FINTEK_F81803A) += ../common/early_serial.c + +ramstage-$(CONFIG_SUPERIO_FINTEK_F81803A) += superio.c +ramstage-$(CONFIG_SUPERIO_FINTEK_F81803A_FAN_CONTROL) += fan_control.c diff --git a/src/superio/fintek/f81803a/acpi/superio.asl b/src/superio/fintek/f81803a/acpi/superio.asl new file mode 100644 index 0000000..7568131 --- /dev/null +++ b/src/superio/fintek/f81803a/acpi/superio.asl @@ -0,0 +1,155 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2011 Christoph Grenz christophg+cb@grenz-bonn.de + * Copyright (C) 2013 secunet Security Networks AG + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * Include this file into a mainboard's DSDT _SB device tree and it will + * expose the F81803A SuperIO and some of its functionality. + * + * It allows the change of IO ports, IRQs and DMA settings on logical + * devices, disabling and reenabling logical devices and controlling power + * saving mode on logical devices or the whole chip. + * + * LDN State + * 0x1 UARTA Implemented, partially tested + * 0x2 UARTB UART only, partially tested + * 0x4 HWM Not implemented + * 0x5 KBC Implemented, untested + * 0x6 GPIO6 Not implemented + * 0x7 WDT0&PLED Not implemented + * 0xa ACPI/PME/ERP Partially implemented + * + * Controllable through preprocessor defines: + * SUPERIO_DEV Device identifier for this SIO (e.g. SIO0) + * SUPERIO_PNP_BASE I/o address of the first PnP configuration register + * F81803A_SHOW_UARTA If defined, UARTA will be exposed. + * F81803A_SHOW_UARTB If defined, UARTB will be exposed. + * F81803A_SHOW_KBC If defined, the KBC will be exposed. + * F81803A_SHOW_PS2M If defined, PS/2 mouse support will be exposed. + * F81803A_SHOW_HWMON If defined, the hardware monitor will be exposed. + * F81803A_SHOW_PME If defined, the PME/EARP/ACPI will be exposed. + */ +#undef SUPERIO_DEV +#define SUPERIO_DEV SIO0 +#undef SUPERIO_CHIP_NAME +#define SUPERIO_CHIP_NAME F81803A +#define SUPERIO_PNP_BASE 0x4E +#define F81803A_SHOW_PME +#include <superio/acpi/pnp.asl> + + + +Device(SUPERIO_DEV) { + Name (_HID, EisaId("PNP0A05")) + Name (_STR, Unicode("Fintek F81803A Super I/O")) + Name (_UID, SUPERIO_UID(SUPERIO_DEV,)) + + /* Mutex for accesses to the configuration ports */ + Mutex(CRMX, 1) + + /* SuperIO configuration ports */ + OperationRegion (CREG, SystemIO, SUPERIO_PNP_BASE, 0x02) + Field (CREG, ByteAcc, NoLock, Preserve) + { + PNP_ADDR_REG, 8, + PNP_DATA_REG, 8 + } + IndexField (ADDR, DATA, ByteAcc, NoLock, Preserve) + { + Offset (0x07), + PNP_LOGICAL_DEVICE, 8, /* Logical device selector */ + Offset (0x30), + PNP_DEVICE_ACTIVE, 1, /* Logical device activation */ + Offset (0x60), + PNP_IO0_HIGH_BYTE, 8, /* First I/O port base - high byte */ + PNP_IO0_LOW_BYTE, 8, /* First I/O port base - low byte */ + Offset (0x62), + PNP_IO1_HIGH_BYTE, 8, /* Second I/O port base - high byte */ + PNP_IO1_LOW_BYTE, 8, /* Second I/O port base - low byte */ + Offset (0x70), + PNP_IRQ0, 8, /* First IRQ */ + offset(0xFB), + APC5, 8, /* PME ACPI Control Register 5 */ + } + + Method(_CRS) + { + /* Announce the used i/o ports to the OS */ + Return (ResourceTemplate () { + IO (Decode16, SUPERIO_PNP_BASE, SUPERIO_PNP_BASE, 0x01, 0x02) + }) + } + + #undef PNP_ENTER_MAGIC_1ST + #undef PNP_ENTER_MAGIC_2ND + #undef PNP_ENTER_MAGIC_3RD + #undef PNP_ENTER_MAGIC_4TH + #undef PNP_EXIT_MAGIC_1ST + #undef PNP_EXIT_SPECIAL_REG + #undef PNP_EXIT_SPECIAL_VAL + #define PNP_ENTER_MAGIC_1ST 0x87 + #define PNP_ENTER_MAGIC_2ND 0x87 + #define PNP_EXIT_MAGIC_1ST 0xaa + #include <superio/acpi/pnp_config.asl> + +#ifdef F81803A_SHOW_UARTA + #undef SUPERIO_UART_LDN + #undef SUPERIO_UART_DDN + #undef SUPERIO_UART_PM_REG + #undef SUPERIO_UART_PM_VAL + #undef SUPERIO_UART_PM_LDN + #define SUPERIO_UART_LDN 1 + #define SUPERIO_UART_PM_REG UAPW + #define SUPERIO_UART_PM_VAL 0 + #define SUPERIO_UART_PM_LDN PNP_NO_LDN_CHANGE + #include <superio/acpi/pnp_uart.asl> +#endif + +#ifdef F81803A_SHOW_UARTB + #undef SUPERIO_UART_LDN + #undef SUPERIO_UART_DDN + #undef SUPERIO_UART_PM_REG + #undef SUPERIO_UART_PM_VAL + #undef SUPERIO_UART_PM_LDN + #define SUPERIO_UART_LDN 2 + #define SUPERIO_UART_PM_REG UBPW + #define SUPERIO_UART_PM_VAL 0 + #define SUPERIO_UART_PM_LDN PNP_NO_LDN_CHANGE + #include <superio/acpi/pnp_uart.asl> +#endif + +#ifdef F81803A_SHOW_PME + #undef SUPERIO_PME_LDN + #define SUPERIO_PME_LDN 0x0A + + OperationRegion(APCx, SystemIO, APC5, 0x01) + Field(APCx, ByteAcc, Nolock, Preserve) /* bits in PME ACPI CONTROL Reg 5*/ + { + Offset(0x00), /*Control Reg 5 */ + , 7, + PSIN, 1 /* PSIN_FLAG */ + } + + /* routine to clear PSIN_FLAG in ACPI_CONTROL_REG_5 of SIO */ + Method(CPSI, 0, Serialized) + { + /* DBG0("SIO CPSI")*/ + ENTER_CONFIG_MODE(SUPERIO_PME_LDN) + Store(1, PSIN) + EXIT_CONFIG_MODE() + } +#endif + +} diff --git a/src/superio/fintek/f81803a/f81803a.h b/src/superio/fintek/f81803a/f81803a.h new file mode 100644 index 0000000..a5e9329 --- /dev/null +++ b/src/superio/fintek/f81803a/f81803a.h @@ -0,0 +1,73 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2011 Advanced Micro Devices, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * Datasheet: + * - Name: F81803A + */ + +#ifndef SUPERIO_FINTEK_F81803_H +#define SUPERIO_FINTEK_F81803_H + +/* Logical Device Numbers (LDN) */ +#define F81803A_SP1 0x01 /* UART1 */ +#define F81803A_SP2 0x02 /* UART2 */ +#define F81803A_HWM 0x04 /* Hardware Monitor */ +#define F81803A_KBC 0x05 /* Keyboard/Mouse */ +#define F81803A_GPIO 0x06 /* General Purpose I/O (GPIO) */ +#define F81803A_WDT 0x07 /* Watch Dog Timer */ +#define F81803A_PME 0x0a /* Power Management Events (PME) */ + +/* registers in Fintek F81803A */ +#define LDN_REG 0x07 + +/* Global Control Registers */ +#define CLOCK_SELECT_REG 0x26 +#define FUNC_PROG_SELECT (1<<3) +#define PORT_SELECT_REG 0x27 + +#define TSI_LEVEL_SELECT_REG 0x28 /* FUNC_PROG_SEL = 0 */ +#define TSI_PIN_SELECT_REG 0x28 /* FUNC_PROG_SEL = 1 */ +#define MULTI_FUNC_SEL_REG1 0x29 +#define MULTI_FUNC_SEL_REG2 0x2A +#define MULTI_FUNC_SEL_REG3 0x2B +#define MULTI_FUNC_SEL_REG 0x2C +#define WAKEUP_CONTROL_REG 0x2d + +/* LUN A - PME, ACPI, ERP */ +#define PME_DEVICE_ENABLE_REG 0x30 +#define PME_ENABLE (1<<0) +#define PME_ERP_ENABLE_REG 0xE0 +#define ERP_ENABLE (1<<7) +#define ERP_PME_EN (1<<1) +#define ERP_PSOUT_EN (1<<0) +#define PME_ERP_CONTROL_1_REG 0xE1 +#define PME_ERP_CONTROL_2_REG 0xE2 +#define PME_ERP_PSIN_DEBOUNCE_REG 0xE3 +#define PME_ERP_WAKEUP_ENABLE_REG 0xE8 +#define PME_ERP_MODE_SELECT_REG 0xEC +#define PME_EVENT_ENABLE_1_REG 0xF0 +#define PME_EVENT_STATUS_1_REG 0xF1 +#define PME_EVENT_ENABLE_2_REG 0xF2 +#define PME_EVENT_STATUS_2_REG 0xF3 +#define PME_ACPI_CONTROL_1_REG 0xF4 +#define PME_ACPI_CONTROL_2_REG 0xF5 +#define PME_ACPI_CONTROL_3_REG 0xF6 +#define PME_ACPI_CONTROL_4_REG 0xF7 +#define PME_ACPI_CONTROL_5_REG 0xFB +#define PME_ACPI_CONTROL_6_REG 0xFC + +#endif /* SUPERIO_FINTEK_F81803_H */ diff --git a/src/superio/fintek/f81803a/f81803a_hwm.h b/src/superio/fintek/f81803a/f81803a_hwm.h new file mode 100644 index 0000000..2fba52b --- /dev/null +++ b/src/superio/fintek/f81803a/f81803a_hwm.h @@ -0,0 +1,92 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Richard Spiegel richard.spiegel@silverbackltd.com + * Copyright (C) 2019 Silverback ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SUPERIO_FINTEK_F81803_HWM_H +#define SUPERIO_FINTEK_F81803_HWM_H + +#define TP_SENSOR_TYPE 0x6b +#define TP_SENSOR1_TYPE_SHIFT 1 +#define TP_SENSOR2_TYPE_SHIFT 2 +#define TP_SENSOR_TYPE_MASK 0x01 +#define TP_DIODE_STATUS 0x6f +#define TP_MMX_OPEN 0x40 +#define TP_PECI_OPEN 0x20 +#define TP_TSI_OPEN 0x10 +#define TP_EXTERNAL_SENSOR2_OPEN 0x04 +#define TP_EXTERNAL_SENSOR1_OPEN 0x02 + +#define FAN_TYPE_REG 0x94 +#define FAN1_TYPE_SHIFT 0 +#define FAN2_TYPE_SHIFT 2 +#define FAN_TYPE_MASK 0x03 +#define FAN_MODE_REG 0x96 +#define FAN1_MODE_SHIFT 0 +#define FAN2_MODE_SHIFT 4 +#define FAN_MODE_MASK 0x07 +#define FAN1_FREQ_SEL_ADD_SHIFT 3 /* FUNC_PROG_SEL = 1 */ +#define FAN2_FREQ_SEL_ADD_SHIFT 4 /* FUNC_PROG_SEL = 1 */ +#define FAN_UP_RATE_REG 0x9a +#define FAN1_UP_RATE_SHIFT 0 +#define FAN2_UP_RATE_SHIFT 2 +#define FAN_UP_RATE_MASK 0x03 +#define FAN_DOWN_RATE_REG 0x9b +#define FAN1_DOWN_RATE_SHIFT 0 +#define FAN2_DOWN_RATE_SHIFT 2 +#define FAN_DOWN_RATE_MASK 0x03 +#define FAN_DOWN_RATE_DIFF_FROM_UP_SHIFT 7 /* FUNC_PROG_SEL = 1 */ +#define FAN_DIRECT_LOAD_EN_SHIFT 6 /* FUNC_PROG_SEL = 1 */ +#define FAN_FAULT_TIME_REG 0x9f +#define FAN_FUNC_PROG_SEL_SHIFT 7 + +#define FAN1_BOUND_TEMP1 0xa6 +#define FAN1_BOUND_TEMP2 0xa7 +#define FAN1_BOUND_TEMP3 0xa8 +#define FAN1_BOUND_TEMP4 0xa9 +#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 +#define FAN2_BOUND_TEMP1 0xb6 +#define FAN2_BOUND_TEMP2 0xb7 +#define FAN2_BOUND_TEMP3 0xb8 +#define FAN2_BOUND_TEMP4 0xb9 +#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 +#define FAN_DETAIL_SKIP (FAN2_BOUND_TEMP1 - FAN1_BOUND_TEMP1) + +#define FAN1_TMP_MAPPING 0xaf +#define FAN2_TMP_MAPPING 0xbf +#define FAN_TEMP_SEL_HIGH_SHIFT 7 +#define FAN_PWM_FREQ_SEL_SHIFT 6 +#define FAN_INTERPOLATION_SHIFT 4 +#define FAN_JUMP_UP_SHIFT 3 +#define FAN_JUMP_DOWN_SHIFT 2 +#define FAN_TEMP_SEL_LOW_SHIFT 0 +#define FAN_TEMP_SEL_LOW_MASK 0x03 +#define FAN_BIT_MASK 0x01 + +#define STATUS_INVALID_VALUE -1 +#define STATUS_INVALID_ORDER -2 + +#define FIRST_FAN 1 +#define LAST_FAN 2 +#define MAX_DUTY 100 + +#endif /* SUPERIO_FINTEK_F81803_HWM_H */ diff --git a/src/superio/fintek/f81803a/fan_control.c b/src/superio/fintek/f81803a/fan_control.c new file mode 100644 index 0000000..47c8086 --- /dev/null +++ b/src/superio/fintek/f81803a/fan_control.c @@ -0,0 +1,263 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Richard Spiegel richard.spiegel@silverbackltd.com + * Copyright (C) 2019 Silverback ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include "../common/fan_control.h" +#include "f81803a_hwm.h" + +static void write_hwm_reg(u16 address, u8 index, u8 value) +{ + u16 index_add, data_add; + index_add = address | 0x0001; /* force odd address */ + data_add = index_add + 1; + outb(index, index_add); + outb(value, data_add); +} + +static u8 read_hwm_reg(u16 address, u8 index) +{ + u16 index_add, data_add; + index_add = address | 0x0001; /* force odd address */ + data_add = index_add + 1; + outb(index, index_add); + return inb(data_add); +} + +static void read_and_or_write(u16 address, u8 index, u8 shift, u8 mask, + u8 value) +{ + u8 use_mask, use_value; + u8 temp = read_hwm_reg(address, index); + + use_mask = mask << shift; + use_value = (value & mask) << shift; + temp &= ~use_mask; + temp |= use_value; + write_hwm_reg(address, index, temp); +} + +static inline void set_prog_sel(u16 address, u8 value) +{ + read_and_or_write(address, FAN_FAULT_TIME_REG, FAN_FUNC_PROG_SEL_SHIFT, + FAN_BIT_MASK, value); +} + +static int check_value_seq(u8 *values, u8 count) +{ + u8 last_value = 128; /* max value is 127 */ + u8 current_value, i; + for (i = 0; i < count; i++) { + current_value = values[i]; + if (current_value > 127) + return STATUS_INVALID_VALUE; + if (current_value >= last_value) + return STATUS_INVALID_ORDER; + last_value = current_value; + } + return HWM_STATUS_SUCCESS; +} + +int set_sensor_type(u16 base_address, u8 sensor, u8 type) +{ + u8 sensor_status = read_hwm_reg(base_address, TP_DIODE_STATUS); + if (sensor == TEMP_SENSOR_DEFAULT) + return HWM_STATUS_SUCCESS; + switch (sensor) { + case EXTERNAL_SENSOR1: + if (sensor_status & TP_EXTERNAL_SENSOR1_OPEN) + return HWM_STATUS_WARNING_SENSOR_DISCONECTED; + read_and_or_write(base_address, TP_SENSOR_TYPE, + TP_SENSOR1_TYPE_SHIFT, TP_SENSOR_TYPE_MASK, type); + break; + case EXTERNAL_SENSOR2: + if (sensor_status & TP_EXTERNAL_SENSOR2_OPEN) + return HWM_STATUS_WARNING_SENSOR_DISCONECTED; + read_and_or_write(base_address, TP_SENSOR_TYPE, + TP_SENSOR2_TYPE_SHIFT, TP_SENSOR_TYPE_MASK, type); + break; + default: + return HWM_STATUS_INVALID_TEMP_SENSOR; + } + return HWM_STATUS_SUCCESS; +} + +int set_fan_temperature_source(u16 base_address, u8 fan, u8 source) +{ + u8 index, high_value, low_value; + + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return HWM_STATUS_INVALID_FAN; + index = FAN1_TMP_MAPPING; + if (fan == LAST_FAN) + index += FAN_DETAIL_SKIP; + high_value = (source >> 2) & FAN_BIT_MASK; + low_value = source & FAN_TEMP_SEL_LOW_MASK; + read_and_or_write(base_address, index, FAN_TEMP_SEL_HIGH_SHIFT, + FAN_BIT_MASK, high_value); + read_and_or_write(base_address, index, FAN_TEMP_SEL_LOW_SHIFT, + FAN_TEMP_SEL_LOW_MASK, low_value); + return HWM_STATUS_SUCCESS; +} + +int set_fan_type_mode(u16 base_address, u8 fan, u8 type, u8 mode) +{ + u8 shift; + + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return HWM_STATUS_INVALID_FAN; + set_prog_sel(base_address, 0); + if (type < FAN_TYPE_SET_BY_STRAP) { + shift = FAN1_TYPE_SHIFT; + if (fan == LAST_FAN) + shift = FAN2_TYPE_SHIFT; + read_and_or_write(base_address, FAN_TYPE_REG, shift, + FAN_TYPE_MASK, type); + } + if (mode < FAN_MODE_DEFAULT) { + shift = FAN1_MODE_SHIFT; + if (fan == LAST_FAN) + shift = FAN2_MODE_SHIFT; + read_and_or_write(base_address, FAN_MODE_REG, shift, + FAN_MODE_MASK, mode); + } + return HWM_STATUS_SUCCESS; +} + +int set_pwm_frequency(u16 base_address, u8 fan, u8 frequency) +{ + u8 shift, index, byte; + + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return HWM_STATUS_INVALID_FAN; + byte = read_hwm_reg(base_address, FAN_TYPE_REG); + shift = FAN1_TYPE_SHIFT; + if (fan == LAST_FAN) + shift = FAN2_TYPE_SHIFT; + if (((byte >> shift) & FAN_TYPE_PWM_CHECK) == FAN_TYPE_PWM_CHECK) + return HWM_STATUS_WARNING_FAN_NOT_PWM; + set_prog_sel(base_address, 1); + shift = FAN1_FREQ_SEL_ADD_SHIFT; + if (fan == LAST_FAN) + shift = FAN2_FREQ_SEL_ADD_SHIFT; + byte = (frequency >> 1) & FAN_BIT_MASK; + read_and_or_write(base_address, FAN_MODE_REG, shift, FAN_BIT_MASK, + byte); + set_prog_sel(base_address, 0); + index = FAN1_TMP_MAPPING; + if (fan == LAST_FAN) + index += FAN_DETAIL_SKIP; + byte = frequency & FAN_BIT_MASK; + read_and_or_write(base_address, index, FAN_PWM_FREQ_SEL_SHIFT, + FAN_BIT_MASK, byte); + return HWM_STATUS_SUCCESS; +} + +int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections) +{ + int status, temp; + u8 i, index, value; + + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return HWM_STATUS_INVALID_FAN; + status = check_value_seq(boundaries, + fintek_boundaries_size); + if (status != HWM_STATUS_SUCCESS) { + if (status == STATUS_INVALID_VALUE) + return HWM_STATUS_INVALID_BOUNDARY_VALUE; + return HWM_STATUS_BOUNDARY_WRONG_ORDER; + } + status = check_value_seq(sections, + fintek_sections_size); + if (status != HWM_STATUS_SUCCESS) { + if (status == STATUS_INVALID_VALUE) + return HWM_STATUS_INVALID_SECTION_VALUE; + return HWM_STATUS_SECTIONS_WRONG_ORDER; + } + index = FAN1_BOUND_TEMP1; + if (fan == LAST_FAN) + index += FAN_DETAIL_SKIP; + for (i = 0; i < fintek_boundaries_size; i++) { + value = boundaries[i]; + write_hwm_reg(base_address, index, value); + index++; + } + index = FAN1_SECTION_SPEED1; + if (fan == LAST_FAN) + index += FAN_DETAIL_SKIP; + for (i = 0; i < fintek_sections_size; i++) { + value = sections[i]; + if (value > 100) + return HWM_STATUS_INVALID_SECTION_VALUE; + temp = (255 * value) / 100; + value = (u8) (temp & 0x00ff); + write_hwm_reg(base_address, index, value); + index++; + } + return HWM_STATUS_SUCCESS; +} + +int set_fan_speed_change_rate(u16 base_address, u8 fan, u8 rate_up, + u8 rate_down) +{ + u8 shift, index; + + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return HWM_STATUS_INVALID_FAN; + + index = FAN1_TMP_MAPPING; + if (fan == LAST_FAN) + index += FAN_DETAIL_SKIP; + shift = FAN1_UP_RATE_SHIFT; + if (fan == LAST_FAN) + shift = FAN2_UP_RATE_SHIFT; + + if (rate_up == FAN_UP_RATE_JUMP) { + read_and_or_write(base_address, index, FAN_JUMP_UP_SHIFT, + FAN_BIT_MASK, 1); + } else { + read_and_or_write(base_address, index, FAN_JUMP_UP_SHIFT, + FAN_BIT_MASK, 0); + if (rate_up < FAN_UP_RATE_DEFAULT) { + read_and_or_write(base_address, FAN_UP_RATE_REG, + shift, FAN_UP_RATE_MASK, rate_up); + } + } + + shift = FAN1_DOWN_RATE_SHIFT; + if (fan == LAST_FAN) + shift = FAN2_DOWN_RATE_SHIFT; + if (rate_down == FAN_DOWN_RATE_JUMP) { + read_and_or_write(base_address, index, FAN_JUMP_DOWN_SHIFT, + FAN_BIT_MASK, 1); + } else { + read_and_or_write(base_address, index, FAN_JUMP_UP_SHIFT, + FAN_BIT_MASK, 0); + set_prog_sel(base_address, 1); + if (rate_down < FAN_DOWN_RATE_DEFAULT) { + read_and_or_write(base_address, FAN_DOWN_RATE_REG, + shift, FAN_DOWN_RATE_MASK, rate_down); + read_and_or_write(base_address, FAN_DOWN_RATE_REG, + FAN_DOWN_RATE_DIFF_FROM_UP_SHIFT, + FAN_BIT_MASK, 0); + } + if (rate_down == FAN_DOWN_RATE_SAME_AS_UP) { + read_and_or_write(base_address, FAN_DOWN_RATE_REG, + FAN_DOWN_RATE_DIFF_FROM_UP_SHIFT, + FAN_BIT_MASK, 1); + } + set_prog_sel(base_address, 0); + } + return HWM_STATUS_SUCCESS; +} diff --git a/src/superio/fintek/f81803a/superio.c b/src/superio/fintek/f81803a/superio.c new file mode 100644 index 0000000..2f78244 --- /dev/null +++ b/src/superio/fintek/f81803a/superio.c @@ -0,0 +1,79 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2011 Advanced Micro Devices, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/device.h> +#include <device/pnp.h> +#include <superio/conf_mode.h> +#include <console/console.h> +#include <stdlib.h> +#include <pc80/keyboard.h> +#include "f81803a.h" + +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); +} + +static void f81803a_init(struct device *dev) +{ + if (!dev->enabled) + return; + switch (dev->path.pnp.device) { + /* TODO: Might potentially need code for HWM or FDC etc. */ + case F81803A_KBC: + pc_keyboard_init(NO_AUX_DEVICE); + break; + case F81803A_PME: + f81803a_pme_init(dev); + break; + } + +} + +static struct device_operations ops = { + .read_resources = pnp_read_resources, + .set_resources = pnp_set_resources, + .enable_resources = pnp_enable_resources, + .enable = pnp_alt_enable, + .init = f81803a_init, + .ops_pnp_mode = &pnp_conf_mode_8787_aa, +}; + +static struct pnp_info pnp_dev_info[] = { + /* TODO: Some of the 0x7f8 etc. values may not be correct. */ + { &ops, F81803A_SP1, PNP_IO0 | PNP_IRQ0, 0x7f8, }, + { &ops, F81803A_SP2, PNP_IO0 | PNP_IRQ0, 0x7f8, }, + { &ops, F81803A_HWM, PNP_IO0 | PNP_IRQ0, 0xff8, }, + { &ops, F81803A_KBC, PNP_IO0 | PNP_IRQ0 | PNP_IRQ1, 0x07ff, }, + { &ops, F81803A_GPIO, PNP_IO0 | PNP_IRQ0, 0x7f8, }, + //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 }, + { &ops, F81803A_PME, }, +}; + +static void enable_dev(struct device *dev) +{ + printk(BIOS_INFO, "F81803A : %s\n", __func__); + pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info); +} + +struct chip_operations superio_fintek_f81803a_ops = { + .enable_dev = enable_dev +};
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 1: Code-Review+1
Looks good, but I can't find a datasheet to double-check.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
Looks good, but I can't find a datasheet to double-check.
It's NDA document, I got it directly from AMD. AMD has it because they build some boards (including padmelon) using it. I can only share it with Silverback because we do have an NDA agreement in place. My real concern was having fan_control.h accepted, because it introduces a new API.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
My real concern was having fan_control.h accepted, because it introduces a new API.
Only had a brief look at the patch, but wanted to get some feedback soonish, so here we go:
For the SIO chips of other vendors (mostly some ITE ones and I think one Nuvoton one) the configuration of the fan control/HWM part is done via device tree settings (look for the chip.h file in the SIO folders). The problem with the configuration via device tree settings is that the code can't tell apart if a setting isn't configured or if it is set to zero. I'm not opposed to do the configuration of the HWM by function calls in ramstage instead of device tree settings, since this would work around that device tree setting issue. For assigning IO ports and IRQ and that sort of stuff, the device tree works rather well, but at least some parts of the configuration of the HWM does indeed feel a bit shoehorned in to the device tree.
The discussion device tree setting vs. doing things in code recently also started in the context of the FSP-S settings, although the problem there is a bit different; the bigger issue there is that some FSP settings simply aren't hooked up to the device tree setting infrastructure and to a lesser extend that this is mostly unnecessary code (well, the FSP contains much more boilerplate compared to the device tree settings stuff).
For the SIO HWM part, I'd like to make sure that we won't end up with multiple non-devicetree API concepts; this getting too inconsistent for different SIOs HWMs would be my only serious concern here. Since the SIO configuration is rather board-specific and a board doesn't have a huge variety of SIOs (sure, some variants of one boards have 2 different SIO chips, but this is no blocker for me), it doesn't need to be one exact API that is used in the first and final form for all SIOs, but the general concept of how things are done should be similar and clear.
Is there a patch that shows how this new API is used?
Might be a good idea to have a discussion about the device tree setting vs. function calls in ramstage on the mailing list, but I don't expect too much opposition against this.
TL;DR since I'm not sure if this sounded a bit too negative: I do like the idea, but want to make sure that the change will improve the architecture and won't be a pain to maintain in the future.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
Oh and if that can be done easily, it might be a good idea to split the patch into the base patch adding the general SIO support and a patch on top of that that adds the HWM support. More an idea and not a requirement from my side though.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1: Code-Review+1
Looks good, but I can't find a datasheet to double-check.
It's NDA document, I got it directly from AMD. AMD has it because they build some boards (including padmelon) using it. I can only share it with Silverback because we do have an NDA agreement in place. My real concern was having fan_control.h accepted, because it introduces a new API.
Ack. It would be nice to have superiotool support for this SuperIO, if it is possible (NDA and corporate stuff might be a blocker).
Regarding the HWM API, I'm not experienced enough to judge it.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
Patch Set 2:
My real concern was having fan_control.h accepted, because it introduces a new API.
Only had a brief look at the patch, but wanted to get some feedback soonish, so here we go:
For the SIO chips of other vendors (mostly some ITE ones and I think one Nuvoton one) the configuration of the fan control/HWM part is done via device tree settings (look for the chip.h file in the SIO folders). The problem with the configuration via device tree settings is that the code can't tell apart if a setting isn't configured or if it is set to zero. I'm not opposed to do the configuration of the HWM by function calls in ramstage instead of device tree settings, since this would work around that device tree setting issue. For assigning IO ports and IRQ and that sort of stuff, the device tree works rather well, but at least some parts of the configuration of the HWM does indeed feel a bit shoehorned in to the device tree.
The discussion device tree setting vs. doing things in code recently also started in the context of the FSP-S settings, although the problem there is a bit different; the bigger issue there is that some FSP settings simply aren't hooked up to the device tree setting infrastructure and to a lesser extend that this is mostly unnecessary code (well, the FSP contains much more boilerplate compared to the device tree settings stuff).
For the SIO HWM part, I'd like to make sure that we won't end up with multiple non-devicetree API concepts; this getting too inconsistent for different SIOs HWMs would be my only serious concern here. Since the SIO configuration is rather board-specific and a board doesn't have a huge variety of SIOs (sure, some variants of one boards have 2 different SIO chips, but this is no blocker for me), it doesn't need to be one exact API that is used in the first and final form for all SIOs, but the general concept of how things are done should be similar and clear.
Is there a patch that shows how this new API is used?
Might be a good idea to have a discussion about the device tree setting vs. function calls in ramstage on the mailing list, but I don't expect too much opposition against this.
TL;DR since I'm not sure if this sounded a bit too negative: I do like the idea, but want to make sure that the change will improve the architecture and won't be a pain to maintain in the future.
I could have implemented it in bootblock, but I considered that code run faster once in ramstage, and fan control is only truly needed after at least 30 seconds after power up. This is specially true with fintek, as at reset it programs a 40% of max speed for all fans until fan control is started.
The patch that shows how this API is used cant't be pushed right now (jenkins would fail) because https://review.coreboot.org/c/blobs/+/33615 is not yet merged. It's the mainboard/amd/padmelon code, which will then make use of all the patches I have implemented so far. If you really want it, I could upload it and mark as work in progress (I'm not sure if jenkins would still examine it if declared work in progress).
I have done fan control on a nuvoton SIO before (UEFI, at Intel), so I do know that it's complicated (was easier this time, because I had done it before). My impression is that within devices of the same vendor, it's very similar (some times identical), but between different vendors it varies a lot, thus I made this API fintek specific. I wanted to do something that was easy to use (or at least copy and adapt) by someone with no experience.
Nuvoton SIO had a paging scheme using 2 IO addresses programmed within the main SIO page (as if a separate SIO) within the HWM while fintek uses a single memory based page (256 bytes range) from an address programmed within the main SIO page.
However, complications apart, both follow a general idea for an SIO only fan control (though nuvoton had a lot more options, including manual control that could be implemented via ACPI). 1) The temperature source has to be defined. 2) The fan type has to be defined (PWM or voltage controlled). 3) 5 regions are defined through 4 inflection points. 4) The fan speed in these 5 regions must be defined (Nuvoton you also needed to define if it was a single speed or linear change between inflections point) 5) Define if fan is voltage controlled or PWM. If PWM, what frequency the PWM is to be set. 6) (Nuvoton only). Define the feedback (tachometer). 6) (Fintek only). Define the rate (RPM per time slice) at which fan speed will change.
Creating an API for each of the 5 common functionality looked the easiest way, with less parameters to be passed on each function. With nuvoton due to time consideration caused by multiple pages in a SIO like IO access, I decided to do a table of triplets: page, register value, and run the table as many times as there were pages and program a single page at a time... but the end result was that it still was complicated and whoever programmed it for a particular board had to have a good knowledge of the SIO itself.
I believe that an API with self explanatory names and easy to understand parameters makes it much easier for anyone to use it, though I'm not saying that the API I defined is the best possible. I'm only saying it's much more simple than what I previously implemented for nuvoton at Intel.
If anyone ever wants to program it for nuvoton, I suggest maintaining the table of triplets scheme (time considerations), but create macros that act in a similar mode as my defined API, thus hiding the details from the regular user. These macros would then actually fill the table of triplets.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
I could have implemented it in bootblock, but I considered that code run faster once in ramstage, and fan control is only truly needed after at least 30 seconds after power up. This is specially true with fintek, as at reset it programs a 40% of max speed for all fans until fan control is started.
I'd also say that setting this up in ramstage is early enough.
The patch that shows how this API is used cant't be pushed right now (jenkins would fail) because https://review.coreboot.org/c/blobs/+/33615 is not yet merged. It's the mainboard/amd/padmelon code, which will then make use of all the patches I have implemented so far. If you really want it, I could upload it and mark as work in progress (I'm not sure if jenkins would still examine it if declared work in progress).
It would make reviewing the patch much easier for me when I can see the part that calls the API.
When are you planning to push the patch? For things that aren't basically adapted copies from some existing code, I usually wait with the review until the code that uses the new functionality is published.
My impression is that within devices of the same vendor, it's very similar (some times identical), but between different vendors it varies a lot, thus I made this API fintek specific.
Sure within one manufacturer the blocks are usually much more similar than between vendors.
I don't expect the API to be exactly the same on all SIOs; the general idea should be applicable for all typical SIOs.
Having some functionality for raw HWM register access that has a slightly different parameter format for different vendors is better than having a super complicated API that can do everything for every device, but requires reading the whole implementation to see what need to be done.
I wanted to do something that was easy to use (or at least copy and adapt) by someone with no experience.
Sounds good.
Nuvoton SIO had a paging scheme using 2 IO addresses programmed within the main SIO page (as if a separate SIO) within the HWM while fintek uses a single memory based page (256 bytes range) from an address programmed within the main SIO page.
Ok. Apart from reviewing some patches I haven't looked too closely at the Fintek SIOs yet.
Creating an API for each of the 5 common functionality looked the easiest way, with less parameters to be passed on each function. With nuvoton due to time consideration caused by multiple pages in a SIO like IO access, I decided to do a table of triplets: page, register value, and run the table as many times as there were pages and program a single page at a time... but the end result was that it still was complicated and whoever programmed it for a particular board had to have a good knowledge of the SIO itself.
Yeah, having an API for the most commonly used functionality that can be extended if needed is a good thing; for some more uncommon stuff the corresponding functionality can be added later or in the worst case things can be configured by some raw register access functions that are specific to a vendor.
I believe that an API with self explanatory names and easy to understand parameters makes it much easier for anyone to use it
I agree.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(39 comments)
haven't had a closer look at fan_control.c yet, since some changes to the header files will simplify the code a bit
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 26: _ stray underscore
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 23: #define EXTERNAL_SENSOR1 1 : #define EXTERNAL_SENSOR2 2 : #define EXTERNAL_SENSOR3 3 : #define EXTERNAL_SENSOR_4 4 enums instead of the defines? since the numbers matter here, add an explicit assignment to the first element of the enum
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 27: #define TEMP_SENSOR_THERMISTOR 0 : #define TEMP_SENSOR_BJT 1 : #define TEMP_SENSOR_DEFAULT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 69: #define FAN_FOLLOW_STEP 0 : #define FAN_FOLLOW_INTERPOLATION 1 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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 enum would also be nice here, but assigning negative values to enum elements is probably something that's not explicitly defined in the C specification. or at least i'd be rather cautious about this
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 91: fintek_boundaries_size defines should be in uppercase
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 99: fintek_sections_size same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 101: u8 sensor, u8 type enum types?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 102: u8 source enum type?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 103: u8 type, u8 mode enum types?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 104: u8 frequency enum type?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 105: u8 *boundaries, u8 *sections hm, this might cause nasty bugs when the caller passes a pointer to a too short array. u8 boundaries[static fintek_boundaries_size] might be worth a try; not sure if this is in the c standard we're currently using though. a struct probably won't work well here, since you can't easily iterate over the struct members
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 106: u8 rate_up, : u8 rate_down enum types?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a.h File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a.h... PS2, Line 35: alignment
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a.h... PS2, Line 37: /* Global Control Registers */ some additional whitespace issues below
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... File src/superio/fintek/f81803a/f81803a_hwm.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 21: #define TP_SENSOR1_TYPE_SHIFT 1 : #define TP_SENSOR2_TYPE_SHIFT 2 TP_SENSOR_TYPE_SHIFT(x)? see my comment below
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 28: #define TP_EXTERNAL_SENSOR2_OPEN 0x04 : #define TP_EXTERNAL_SENSOR1_OPEN 0x02 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 32: #define FAN1_TYPE_SHIFT 0 : #define FAN2_TYPE_SHIFT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 36: #define FAN1_MODE_SHIFT 0 : #define FAN2_MODE_SHIFT 4 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 42: #define FAN1_UP_RATE_SHIFT 0 : #define FAN2_UP_RATE_SHIFT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 46: #define FAN1_DOWN_RATE_SHIFT 0 : #define FAN2_DOWN_RATE_SHIFT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... 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)) would make the code using this a bit more elegant. would be 0-indexed though instead of the current indices
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... 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
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 72: #define FAN_DETAIL_SKIP (FAN2_BOUND_TEMP1 - FAN1_BOUND_TEMP1) not needed with my suggestion for this
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 74: #define FAN1_TMP_MAPPING 0xaf : #define FAN2_TMP_MAPPING 0xbf seem comment above
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... 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); : } is flipping these two bits generally needed with this SIO chip or board-specific?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 40: or FDC i'd guess that this SIO doesn't have a floppy controller, so the part of the comment about the FDC can be removed
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 55: looks like one tab too much; same for next line
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 61: /* TODO: Some of the 0x7f8 etc. values may not be correct. */ please check this and remove the comment after checking and if needed fixing
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 67: //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 }, what about the WDT?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 26: _
stray underscore
Opps.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 23: #define EXTERNAL_SENSOR1 1 : #define EXTERNAL_SENSOR2 2 : #define EXTERNAL_SENSOR3 3 : #define EXTERNAL_SENSOR_4 4
enums instead of the defines? since the numbers matter here, add an explicit assignment to the first […]
Will do.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 27: #define TEMP_SENSOR_THERMISTOR 0 : #define TEMP_SENSOR_BJT 1 : #define TEMP_SENSOR_DEFAULT 2
same here
Will do to all.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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
enum would also be nice here, but assigning negative values to enum elements is probably something t […]
I'll not change these.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 91: fintek_boundaries_size
defines should be in uppercase
Agree, will do.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 101: u8 sensor, u8 type
enum types?
Probably. Same for all other places you marked.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 105: u8 *boundaries, u8 *sections
hm, this might cause nasty bugs when the caller passes a pointer to a too short array. […]
That would be a user error, as there are explicitly 4 boundaries making 5 sections. Maybe a comment to avoid user error?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... 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'll not change these.
I'd be ok with that, since i don't see a better, but still easily readable way, right now. also since this is the return value and not another parameter of the functions, it doesn't make things too inconsistent IMHO, so yeah, just keep this
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 105: u8 *boundaries, u8 *sections
That would be a user error, as there are explicitly 4 boundaries making 5 sections. […]
I'd definitely like to have a comment stating this; without a comment this is too easy to use wrongly. not in this patch, but in the patch using the code: the calling code should probably explicitly define the arrays passed to this function with the boundary and section size as lengths, so no array[], but array[length] instead
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(6 comments)
Some of the code I got ready from previous engineer, and I did not check for correctness. My code is only the fan control. Will fix remaining code.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 69: #define FAN_FOLLOW_STEP 0 : #define FAN_FOLLOW_INTERPOLATION 1
same here
You know, now that you highlighted it, I don't have code for this. I'll have to check why, if I just missed or if I just placed it here and need comment to explain what this was for.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... 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); : }
is flipping these two bits generally needed with this SIO chip or board-specific?
Apparently with any SIO of this family, if you're enabling PME functionality.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 40: or FDC
i'd guess that this SIO doesn't have a floppy controller, so the part of the comment about the FDC c […]
It does have watch dog timer, so I'll replace FDC with WDT.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 55:
looks like one tab too much; same for next line
will fix
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 61: /* TODO: Some of the 0x7f8 etc. values may not be correct. */
please check this and remove the comment after checking and if needed fixing
will do.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 67: //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 },
what about the WDT?
This SIO does have it...
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 69: #define FAN_FOLLOW_STEP 0 : #define FAN_FOLLOW_INTERPOLATION 1
You know, now that you highlighted it, I don't have code for this. […]
my guess would be that this is to select if the fan control should use the lower (or maybe upper) value when being between two boundaries or if it should do a linear interpolation between the values at the boundaries
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 69: #define FAN_FOLLOW_STEP 0 : #define FAN_FOLLOW_INTERPOLATION 1
my guess would be that this is to select if the fan control should use the lower (or maybe upper) va […]
That's correct, and I have not added code for it. The SIO is capable of it. Working on it now.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
Patch Set 2:
My real concern was having fan_control.h accepted, because it introduces a new API.
Only had a brief look at the patch, but wanted to get some feedback soonish, so here we go:
For the SIO chips of other vendors (mostly some ITE ones and I think one Nuvoton one) the configuration of the fan control/HWM part is done via device tree settings (look for the chip.h file in the SIO folders). The problem with the configuration via device tree settings is that the code can't tell apart if a setting isn't configured or if it is set to zero. I'm not opposed to do the configuration of the HWM by function calls in ramstage instead of device tree settings, since this would work around that device tree setting issue. For assigning IO ports and IRQ and that sort of stuff, the device tree works rather well, but at least some parts of the configuration of the HWM does indeed feel a bit shoehorned in to the device tree.
The discussion device tree setting vs. doing things in code recently also started in the context of the FSP-S settings, although the problem there is a bit different; the bigger issue there is that some FSP settings simply aren't hooked up to the device tree setting infrastructure and to a lesser extend that this is mostly unnecessary code (well, the FSP contains much more boilerplate compared to the device tree settings stuff).
For the SIO HWM part, I'd like to make sure that we won't end up with multiple non-devicetree API concepts; this getting too inconsistent for different SIOs HWMs would be my only serious concern here. Since the SIO configuration is rather board-specific and a board doesn't have a huge variety of SIOs (sure, some variants of one boards have 2 different SIO chips, but this is no blocker for me), it doesn't need to be one exact API that is used in the first and final form for all SIOs, but the general concept of how things are done should be similar and clear.
Is there a patch that shows how this new API is used?
Might be a good idea to have a discussion about the device tree setting vs. function calls in ramstage on the mailing list, but I don't expect too much opposition against this.
TL;DR since I'm not sure if this sounded a bit too negative: I do like the idea, but want to make sure that the change will improve the architecture and won't be a pain to maintain in the future.
I could have implemented it in bootblock, but I considered that code run faster once in ramstage, and fan control is only truly needed after at least 30 seconds after power up. This is specially true with fintek, as at reset it programs a 40% of max speed for all fans until fan control is started.
The patch that shows how this API is used cant't be pushed right now (jenkins would fail) because https://review.coreboot.org/c/blobs/+/33615 is not yet merged. It's the mainboard/amd/padmelon code, which will then make use of all the patches I have implemented so far. If you really want it, I could upload it and mark as work in progress (I'm not sure if jenkins would still examine it if declared work in progress).
I have done fan control on a nuvoton SIO before (UEFI, at Intel), so I do know that it's complicated (was easier this time, because I had done it before). My impression is that within devices of the same vendor, it's very similar (some times identical), but between different vendors it varies a lot, thus I made this API fintek specific. I wanted to do something that was easy to use (or at least copy and adapt) by someone with no experience.
Nuvoton SIO had a paging scheme using 2 IO addresses programmed within the main SIO page (as if a separate SIO) within the HWM while fintek uses a single memory based page (256 bytes range) from an address programmed within the main SIO page.
However, complications apart, both follow a general idea for an SIO only fan control (though nuvoton had a lot more options, including manual control that could be implemented via ACPI).
- The temperature source has to be defined.
- The fan type has to be defined (PWM or voltage controlled).
- 5 regions are defined through 4 inflection points.
- The fan speed in these 5 regions must be defined (Nuvoton you also needed to define if it was a single speed or linear change between inflections point)
- Define if fan is voltage controlled or PWM. If PWM, what frequency the PWM is to be set.
- (Nuvoton only). Define the feedback (tachometer).
- (Fintek only). Define the rate (RPM per time slice) at which fan speed will change.
Creating an API for each of the 5 common functionality looked the easiest way, with less parameters to be passed on each function. With nuvoton due to time consideration caused by multiple pages in a SIO like IO access, I decided to do a table of triplets: page, register value, and run the table as many times as there were pages and program a single page at a time... but the end result was that it still was complicated and whoever programmed it for a particular board had to have a good knowledge of the SIO itself.
I believe that an API with self explanatory names and easy to understand parameters makes it much easier for anyone to use it, though I'm not saying that the API I defined is the best possible. I'm only saying it's much more simple than what I previously implemented for nuvoton at Intel.
If anyone ever wants to program it for nuvoton, I suggest maintaining the table of triplets scheme (time considerations), but create macros that act in a similar mode as my defined API, thus hiding the details from the regular user. These macros would then actually fill the table of triplets.
My bad... fintek also uses 2 IO addresses (index, data), but index is odd (bit 0 set) and data is the next address.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... File src/superio/fintek/f81803a/f81803a_hwm.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 28: #define TP_EXTERNAL_SENSOR2_OPEN 0x04 : #define TP_EXTERNAL_SENSOR1_OPEN 0x02
same here
Will not change this one, those are bit test positions.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#3).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 9 files changed, 805 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33623/3/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/3/src/superio/fintek/common/fan_contro... PS3, Line 75: }fan_rate_up; space required after that close brace '}'
https://review.coreboot.org/#/c/33623/3/src/superio/fintek/common/fan_contro... PS3, Line 85: }fan_rate_down; space required after that close brace '}'
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#4).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 9 files changed, 805 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/4
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 4:
(4 comments)
I'd prefer if you assign the 0 explicitly to the first enum element, since the enum values get written to hardware registers. The effect should be the same, but just to make clear that the actual values matter and they aren't only used as symbols. There are also some comments from patch set 2 not addressed. Haven't looked closely at fan_control.c yet, but from what I saw using the macros I suggested improved the readability quite a bit
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 107: fintek_boundaries_size upper case
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 115: fintek_sections_size upper case
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 125: int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections); maybe add a comment right above this function prototype regarding the array sizes. it is written above in detail, but a short pointer to that would be good to have
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/f81803a/superio.c File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/f81803a/superio.c... PS4, Line 55: whitespace issue here and on next line
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 4:
(4 comments)
Patch Set 4:
(4 comments)
I'd prefer if you assign the 0 explicitly to the first enum element, since the enum values get written to hardware registers. The effect should be the same, but just to make clear that the actual values matter and they aren't only used as symbols. There are also some comments from patch set 2 not addressed. Haven't looked closely at fan_control.c yet, but from what I saw using the macros I suggested improved the readability quite a bit
Will do the first element of enum. Not sure which one I missed, there were so many... could you please explicit. I'm also missing code for interpolation, which is set at reset. I need to at least give the option to disable it, even if no one uses it.
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 107: fintek_boundaries_size
upper case
will do.
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 115: fintek_sections_size
upper case
Will do.
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 125: int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections);
maybe add a comment right above this function prototype regarding the array sizes. […]
Will do.
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/f81803a/superio.c File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/f81803a/superio.c... PS4, Line 55:
whitespace issue here and on next line
Weird, I thought I fixed it. will do.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#5).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 9 files changed, 862 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 38: read_and_or_write hwm_reg_modify maybe? i find the the current function name a bit misleading on multiple levels; first i wondered when it'll do reads or writes and then i wondered why it says and_or, but there's no and or or parameter
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 44: use_mask = mask << shift; : use_value = (value & mask) << shift; the assignments can be put in the same line as the variable declaration. that would save one line and make the code a little easier to read (at least for me)
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 51: static inline void set_prog_sel(u16 address, u8 value) what does this function do? a short comment would probably make this clearer. does this select/switch register banks?
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 57: static int check_value_seq(u8 *values, u8 count) a short comment on what this function does would also be good here
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 232: 0 0 or 1? in other places this function was called with last parameter being 1 and afterwards again with last parameter 0
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 44: use_mask = mask << shift; : use_value = (value & mask) << shift;
the assignments can be put in the same line as the variable declaration. […]
Not sure if I understand what you're saying. I do believe that even though 1 line longer, it's clearer this way.
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 51: static inline void set_prog_sel(u16 address, u8 value)
what does this function do? a short comment would probably make this clearer. […]
Yes, registers 96 through 9e have 2 banks. Will add comment.
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 57: static int check_value_seq(u8 *values, u8 count)
a short comment on what this function does would also be good here
Will do.
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 232: 0
0 or 1? in other places this function was called with last parameter being 1 and afterwards again wi […]
I try to exit pointing to bank 0 (default). But sometimes I have to access bank 1.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 44: use_mask = mask << shift; : use_value = (value & mask) << shift;
Not sure if I understand what you're saying. […]
i meant replacing line 41, 44 and 45 with this:
u8 use_mask = mask << shift; u8 use_value = (value & mask) << shift;
shorter and imho a bit easier to read
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 51: static inline void set_prog_sel(u16 address, u8 value)
Yes, registers 96 through 9e have 2 banks. Will add comment.
ok. a function name like "select_hwm_bank" would make the code easier to read imho
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 232: 0
I try to exit pointing to bank 0 (default). But sometimes I have to access bank 1.
ok. i just wondered about this and when I'm not sure I ask if things are correct
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 44: use_mask = mask << shift; : use_value = (value & mask) << shift;
i meant replacing line 41, 44 and 45 with this: […]
Ok.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#6).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 9 files changed, 869 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/6
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 6: Code-Review+1
looks good to me. I'll wait with +2ing and merging until there's a patch using the SIO support added in this patch. Without a board using a SIO, the SIO code doesn't get build tested. Poke me again when the board code is on Gerrit and buildbot is happy with that patch and I'll get this patch merged.
Oh and I have to say that I prefer the current state of the fan control API added in this patch over the devicetree-based one used by other SIO/EC :)
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 6:
Patch Set 6: Code-Review+1
looks good to me. I'll wait with +2ing and merging until there's a patch using the SIO support added in this patch. Without a board using a SIO, the SIO code doesn't get build tested. Poke me again when the board code is on Gerrit and buildbot is happy with that patch and I'll get this patch merged.
Oh and I have to say that I prefer the current state of the fan control API added in this patch over the devicetree-based one used by other SIO/EC :)
I could have created something devicetree based, but the structure would have to be too complicated due to all the parameters that needs to be controlled. I thought that an API breaking the parameters into logically grouped parameters was much easier to understand and program.
As I mentioned before, I can't merge Padmelon board until 33615 is merged. Locally, I have build and tested the SIO code... found 1 mistake (caused by trying to enable HWM too early of all things, lol). Once I moved enabling HWM to early ramstage (just before calling the API), it worked as expected.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 6:
I could have created something devicetree based, but the structure would have to be too complicated due to all the parameters that needs to be controlled. I thought that an API breaking the parameters into logically grouped parameters was much easier to understand and program.
Yep
As I mentioned before, I can't merge Padmelon board until 33615 is merged. Locally, I have build and tested the SIO code... found 1 mistake (caused by trying to enable HWM too early of all things, lol). Once I moved enabling HWM to early ramstage (just before calling the API), it worked as expected.
I know; just wanted to let you know why I haven't +2ed the patch yet, but that I'm happy with its current state
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#7).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 9 files changed, 916 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/7
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 7:
(9 comments)
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.h
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
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 79: #define FINTEK_BASE_ADDRESS 0x4E can probably also be 0x2e; this is usually configured in the devicetree
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
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
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
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
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
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. also this function reimplements a lot of existing building blocks that could be used to configure SIO settings
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 7:
(1 comment)
This whole code has wrong things, and I was not aware (and apparently neither was Marc, who started the SIO code). There's a lot to study before I make any change. Anyway, thanks for pointing it out. Is there any document I can use for reference?
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 79: #define FINTEK_BASE_ADDRESS 0x4E
can probably also be 0x2e; this is usually configured in the devicetree
Only through power-on strap. By default it's 4e. Was not aware of this capability of devicetree.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 7:
This whole code has wrong things, and I was not aware (and apparently neither was Marc, who started the SIO code). There's a lot to study before I make any change. Anyway, thanks for pointing it out. Is there any document I can use for reference?
The documentation is sadly rather thin at the moment. I'd suggest having a look at the SIO part of for example the device tree of asrock/h81m-hds. In line 111 the chip gets selected and in line 148 the 2e.b means the ldn 0xb of pnp device at io address 0x2e gets enabled (the upper 8 bit select the bit number in the enable register (0x30) of the selected ldn. in the next line the first io base address for that ldn gets set, in the next line the second one and in the line after that the irq. th io statement also tells the memory allocator to reserve that memory region; it also uses the masks from the corresponding superio.c. iirc you can only configure things in the devicetree that are mentioned in the pnp_info struct array. some components have a chip.h file and the setting from there can also be configured in devicetree.
apart from the commented line for the WDT the patch set 6 was good and should probably work fine if the resources for the hwm get configured in the devicetree of the board.
during the build process util/sconfig processes the devicetree into the static.c (or .h? not sure on that) file that resides somewhere in the build directory
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#8).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 9 files changed, 872 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/8
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 8:
(1 comment)
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 this can probably be removed, since the code using this isn't here any more and the defines for those registers are also already present in the common pnp code
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 8:
(1 comment)
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
this can probably be removed, since the code using this isn't here any more and the defines for thos […]
Thanks, missed this section.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#9).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 969 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/9
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#10).
Change subject: superio/fintek: Add f81803a ......................................................................
superio/fintek: Add f81803a
In preparation for padmelon board, add f81803a plus the capability to control the fan with the superio.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 968 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/10
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#11).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently onlF81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 968 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/11
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 11: Code-Review+1
(10 comments)
I like the idea. I've got a few comments about the code, though.
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG@10 PS11, Line 10: onlF81803A Looks like a few characters were eaten away
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... File src/superio/fintek/common/fan_api_call.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... PS11, Line 29: void set_fan(uint8_t fan, external_sensor sensor, temp_sensor_type stype, I would suggest moving these printk's to the called functions, so that you can use __func__ instead.
Maybe make this function return the error code as well?
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: Minor: previous line uses tabs here, this one uses spaces
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 letters.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
PS11: Minor: some split lines seem to fit on the current 96 character limit without splitting
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127 I would suggest making this magic number a constant
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 85: return HWM_STATUS_SUCCESS; I would add this to the switch statement below.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) { This looks odd: is the first fan different?
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 161: : Doesn't look like this line needs to be split at all
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 228: if (rate_up < FAN_UP_RATE_DEFAULT) { : hwm_reg_modify(base_address, FAN_UP_RATE_REG, : shift, FAN_RATE_MASK, rate_up); : } Should this only be done on the else path?
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 11:
(1 comment)
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 5: Copyright (C) 2013 secunet Security Networks AG Copyright?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 11:
(2 comments)
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 5: Copyright (C) 2013 secunet Security Networks AG
Copyright?
This was simply copied by the previous engineer from another fintek SIO...I can see there are differences between what is declared in the comments and what is actually physically there in the F81803A. Will have to revisit it, so I'll add copyright at the same time.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67:
Minor: previous line uses tabs here, this one uses spaces
See above, same reason. Will fix.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#12).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently onlF81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 962 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/12
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 12: Code-Review+2
looks good to me and buildbot +1ed the board using the code
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 12:
ah, just saw that there were some comments on minor stuff in patch set 11; you can either address that in a new changes set of this patch or another patch on top of this one. both works for me
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 12:
Patch Set 12:
ah, just saw that there were some comments on minor stuff in patch set 11; you can either address that in a new changes set of this patch or another patch on top of this one. both works for me
Yes, it was late when I submitted, and I missed these requests. Will submit it today, probably afternoon due to personal issues.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#13).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently onlF81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 958 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/13
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 13: Code-Review+1
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, Lance Zhao, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#14).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently onlF81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 1,069 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/14
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 14:
There were bugs in ACPI code if you enabled to show UART. I have fixed them.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 15:
Patch Set 11: Code-Review+1
(10 comments)
I like the idea. I've got a few comments about the code, though.
Hi, any comments on these? Several of them are still present in the latest patchset.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 15:
(9 comments)
Patch Set 15:
Patch Set 11: Code-Review+1
(10 comments)
I like the idea. I've got a few comments about the code, though.
Hi, any comments on these? Several of them are still present in the latest patchset.
Sorry, slid under the radar.
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG@10 PS11, Line 10: onlF81803A
Looks like a few characters were eaten away
Oops... juggling too many things, it passed under the radar... will fix.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... File src/superio/fintek/common/fan_api_call.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... PS11, Line 29: void set_fan(uint8_t fan, external_sensor sensor, temp_sensor_type stype,
I would suggest moving these printk's to the called functions, so that you can use __func__ instead. […]
Will investigate the possibility, not sure. Will try.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 7: This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : 96? I thought it was 76....
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 11: * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : This section is normally copied without change, so I did not try to adjust for 76 characters.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127
I would suggest making this magic number a constant
127 is the maximum celsius value that can be output as a signed byte, The max percentile is actually 100. As I'm treating them as unsigned, I'm able to use 128... though I probably could use 120 and make it a magic number... that's the temperature at which you CPU will be damaged beyond repair.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 85: return HWM_STATUS_SUCCESS;
I would add this to the switch statement below.
Will do.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
This looks odd: is the first fan different?
Yes, fan 1 has a weight mechanism for adjusting for next fan speed. Basically the idea is to react more aggressively (normally CPU fan) based on how high another temperature (system, thermistor near the CPU, anything) is. This would be highly platform dependent, and by setting the weight temperature same as the control temperature I cancel the weight mechanism and make it work with any board. If a board wants to use the weight mechanism, they should implement it after calling the main HWM programming.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 161: :
Doesn't look like this line needs to be split at all
Will fix.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 228: if (rate_up < FAN_UP_RATE_DEFAULT) { : hwm_reg_modify(base_address, FAN_UP_RATE_REG, : shift, FAN_RATE_MASK, rate_up); : }
Should this only be done on the else path?
Yes, this is a trick. Basically testing for FAN_UP_RATE_JUMP, which require a different set of registers. It would be the same as: if (rate_up == FAN_UP_RATE_JUMP) Same trick below for rate down.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 15:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... File src/superio/fintek/common/fan_api_call.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... PS11, Line 30: fan_temp_source temp_source, fan_type ftype, : fan_mode fmode, fan_pwm_freq fan_freq, : fan_rate_up rate_up, fan_rate_down rate_down, : fan_follow follow, u8 *boundaries, u8 *sections Maybe turn all of this into a structure? Doesn't it seem a little silly to pass all of this?
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 5: Copyright (C) 2013 secunet Security Networks AG
This was simply copied by the previous engineer from another fintek SIO... […]
We're going to be moving all of the copyright lines out of the source code and into an authors file. You can add a copyright if you've done anything creative, in updating the file, but I don't know that it matters.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 26: 0x01 Also not a big deal, but this column could be aligned with the column below.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 7: This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. :
96? I thought it was 76....
No, we've changed the maximum line length to 96 characters.
I don't think Angel was talking about the license. That should stay as it is. But if you look at lines 38 & 39, that could be combines.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127
127 is the maximum celsius value that can be output as a signed byte, The max percentile is actually […]
Angel's saying that you should turn it into a #define. We don't want a magic number.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
Yes, fan 1 has a weight mechanism for adjusting for next fan speed. […]
Add that as a comment please?
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 228: if (rate_up < FAN_UP_RATE_DEFAULT) { : hwm_reg_modify(base_address, FAN_UP_RATE_REG, : shift, FAN_RATE_MASK, rate_up); : }
Yes, this is a trick. […]
add that as a comment?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... File src/superio/fintek/common/fan_api_call.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... PS11, Line 30: fan_temp_source temp_source, fan_type ftype, : fan_mode fmode, fan_pwm_freq fan_freq, : fan_rate_up rate_up, fan_rate_down rate_down, : fan_follow follow, u8 *boundaries, u8 *sections
Maybe turn all of this into a structure? Doesn't it seem a little silly to pass all of this?
This was originally part of mainboard, using the API I defined previously. Felix asked to have it moved to SIO code, which I did... but did not thought about converting to a structure. Will do, it's a good idea.
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 5: Copyright (C) 2013 secunet Security Networks AG
We're going to be moving all of the copyright lines out of the source code and into an authors file. […]
Actually nothing creative... Mostly copy/paste with small changes. The HWM API was creative, the ACPI code was not.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 26: 0x01
Also not a big deal, but this column could be aligned with the column below.
Will do.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127
Angel's saying that you should turn it into a #define. We don't want a magic number.
I agree... I was just saying that the define will be 120...
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
Add that as a comment please?
Will do.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 228: if (rate_up < FAN_UP_RATE_DEFAULT) { : hwm_reg_modify(base_address, FAN_UP_RATE_REG, : shift, FAN_RATE_MASK, rate_up); : }
add that as a comment?
I was thinking more o n the line of making it explicit instead of using the trick. This way no comment will be needed.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, Lance Zhao, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#16).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently only F81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 1,170 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/16
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, Lance Zhao, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#17).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently only F81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 1,170 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/17
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 17:
(13 comments)
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 21: static char msg_err_invalid[] = "Error: invalid"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 22: static char msg_err_wrong_order[] = "Error: wrong order,"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 23: static char msg_err_fan[] = "fan"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 24: static char msg_err_temp_source[] = "temperature source"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 25: static char msg_err_type[] = "type"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 26: static char msg_err_mode[] = "mode"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 27: static char msg_err_rate[] = "change rate"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 28: static char msg_err_frequency[] = "frequency"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 29: static char msg_err_temp_sensor[] = "temperature sensor"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 30: static char msg_err_bondary[] = "boundary"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 31: static char msg_err_section[] = "section"; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 32: static char no_msg[] = ""; static char array declaration should probably be static const char
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 53: if (msg_table[i].selection == err) suspect code indent for conditional statements (16, 16)
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 17:
(1 comment)
Will fix all errors found by Jenkins.
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 53: if (msg_table[i].selection == err)
suspect code indent for conditional statements (16, 16)
Will fix.
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, Lance Zhao, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#18).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently only F81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 1,170 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/18
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33623/18/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/18/src/superio/fintek/f81803a... PS18, Line 52: while(msg_table[i].selection) { space required before the open parenthesis '('
Hello Charles Marslett, Felix Held, Angel Pons, Marshall Dawson, Paul Menzel, Lance Zhao, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33623
to look at the new patch set (#19).
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently only F81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 1,170 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/33623/19
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 19:
Ping. Ready, all request uploaded.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 19: Code-Review+2
looks good to me. I think all comments were addressed, but some comments need to be marked as done before I can merge this
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 19:
(42 comments)
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG@10 PS11, Line 10: onlF81803A
Oops... juggling too many things, it passed under the radar... will fix.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... File src/superio/fintek/common/fan_api_call.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... PS11, Line 29: void set_fan(uint8_t fan, external_sensor sensor, temp_sensor_type stype,
Will investigate the possibility, not sure. Will try.
Done.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... PS11, Line 30: fan_temp_source temp_source, fan_type ftype, : fan_mode fmode, fan_pwm_freq fan_freq, : fan_rate_up rate_up, fan_rate_down rate_down, : fan_follow follow, u8 *boundaries, u8 *sections
This was originally part of mainboard, using the API I defined previously. […]
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 5: Copyright (C) 2013 secunet Security Networks AG
Actually nothing creative... Mostly copy/paste with small changes. […]
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 26: 0x01
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
PS11:
Minor: some split lines seem to fit on the current 96 character limit without splitting
Done.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 7: This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. :
No, we've changed the maximum line length to 96 characters. […]
Done.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 11: * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. :
This section is normally copied without change, so I did not try to adjust for 76 characters.
Done.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127
I agree... I was just saying that the define will be 120...
Done.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 85: return HWM_STATUS_SUCCESS;
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 161: :
Will fix.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 228: if (rate_up < FAN_UP_RATE_DEFAULT) { : hwm_reg_modify(base_address, FAN_UP_RATE_REG, : shift, FAN_RATE_MASK, rate_up); : }
I was thinking more o n the line of making it explicit instead of using the trick. […]
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 21: static char msg_err_invalid[] = "Error: invalid";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 22: static char msg_err_wrong_order[] = "Error: wrong order,";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 23: static char msg_err_fan[] = "fan";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 24: static char msg_err_temp_source[] = "temperature source";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 25: static char msg_err_type[] = "type";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 26: static char msg_err_mode[] = "mode";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 27: static char msg_err_rate[] = "change rate";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 28: static char msg_err_frequency[] = "frequency";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 29: static char msg_err_temp_sensor[] = "temperature sensor";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 30: static char msg_err_bondary[] = "boundary";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 31: static char msg_err_section[] = "section";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 32: static char no_msg[] = "";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 49: static char* get_msg(int err)
"foo* bar" should be "foo *bar"
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 52: while(msg_table[i].selection) {
space required before the open parenthesis '('
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 53: if (msg_table[i].selection == err)
suspect code indent for conditional statements (16, 16)
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 21: static char msg_err_invalid[] = "Error: invalid";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 22: static char msg_err_wrong_order[] = "Error: wrong order,";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 23: static char msg_err_fan[] = "fan";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 24: static char msg_err_temp_source[] = "temperature source";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 25: static char msg_err_type[] = "type";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 26: static char msg_err_mode[] = "mode";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 27: static char msg_err_rate[] = "change rate";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 28: static char msg_err_frequency[] = "frequency";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 29: static char msg_err_temp_sensor[] = "temperature sensor";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 30: static char msg_err_bondary[] = "boundary";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 31: static char msg_err_section[] = "section";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 32: static char no_msg[] = "";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 53: if (msg_table[i].selection == err)
Will fix.
Done
https://review.coreboot.org/c/coreboot/+/33623/18/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/18/src/superio/fintek/f81803a... PS18, Line 52: while(msg_table[i].selection) {
space required before the open parenthesis '('
Done
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
Felix Held has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
superio/fintek: Add f81803A
Add f81803A plus the capability to control the fan with any fintek SIO. This will be done through a common API, though currently only F81803A will have it implemented.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I3d336e76bccc38452b1b1aefef5d4a4f7ee129a8 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33623 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/superio/fintek/Makefile.inc A src/superio/fintek/common/fan_api_call.c A src/superio/fintek/common/fan_control.h A src/superio/fintek/f81803a/Kconfig A src/superio/fintek/f81803a/Makefile.inc A src/superio/fintek/f81803a/acpi/superio.asl A src/superio/fintek/f81803a/f81803a.h A src/superio/fintek/f81803a/f81803a_hwm.h A src/superio/fintek/f81803a/fan_control.c A src/superio/fintek/f81803a/superio.c 10 files changed, 1,170 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/superio/fintek/Makefile.inc b/src/superio/fintek/Makefile.inc index 796e519..db683fd 100644 --- a/src/superio/fintek/Makefile.inc +++ b/src/superio/fintek/Makefile.inc @@ -26,3 +26,4 @@ subdirs-y += f81216h subdirs-y += f81865f subdirs-y += f81866d +subdirs-y += f81803a diff --git a/src/superio/fintek/common/fan_api_call.c b/src/superio/fintek/common/fan_api_call.c new file mode 100644 index 0000000..1bd5b2e --- /dev/null +++ b/src/superio/fintek/common/fan_api_call.c @@ -0,0 +1,63 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Richard Spiegel richard.spiegel@silverbackltd.com + * Copyright (C) 2019 Silverback ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include "fan_control.h" + +static int check_status(int status) +{ + if (status < HWM_STATUS_SUCCESS) + return status; + return HWM_STATUS_SUCCESS; /* positive values are warnings only */ +} + +int set_fan(struct fintek_fan *fan_init) +{ + int s; + + s = set_sensor_type(CONFIG_HWM_PORT, fan_init->sensor, fan_init->stype); + if (check_status(s) != HWM_STATUS_SUCCESS) + return s; + + s = set_fan_temperature_source(CONFIG_HWM_PORT, fan_init->fan, fan_init->temp_source); + if (check_status(s) != HWM_STATUS_SUCCESS) + return s; + + s = set_fan_type_mode(CONFIG_HWM_PORT, fan_init->fan, fan_init->ftype, fan_init->fmode); + if (check_status(s) != HWM_STATUS_SUCCESS) + return s; + + s = set_pwm_frequency(CONFIG_HWM_PORT, fan_init->fan, fan_init->fan_freq); + if (check_status(s) != HWM_STATUS_SUCCESS) + return s; + + s = set_fan_speed_change_rate(CONFIG_HWM_PORT, fan_init->fan, fan_init->rate_up, + fan_init->rate_down); + if (check_status(s) != HWM_STATUS_SUCCESS) + return s; + + s = set_fan_follow(CONFIG_HWM_PORT, fan_init->fan, fan_init->follow); + if (check_status(s) != HWM_STATUS_SUCCESS) + return s; + + s = set_sections(CONFIG_HWM_PORT, fan_init->fan, fan_init->boundaries, + fan_init->sections); + if (check_status(s) != HWM_STATUS_SUCCESS) + return s; + + printk(BIOS_DEBUG, "Fan %d completed\n", fan_init->fan); + return HWM_STATUS_SUCCESS; +} diff --git a/src/superio/fintek/common/fan_control.h b/src/superio/fintek/common/fan_control.h new file mode 100644 index 0000000..fbe784b --- /dev/null +++ b/src/superio/fintek/common/fan_control.h @@ -0,0 +1,199 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Richard Spiegel richard.spiegel@silverbackltd.com + * Copyright (C) 2019 Silverback ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SUPERIO_FINTEK_FAN_CONTROL_H +#define SUPERIO_FINTEK_FAN_CONTROL_H + +#include <stdint.h> +#include <arch/io.h> + +typedef enum { + IGNORE_SENSOR = 0, + EXTERNAL_SENSOR1, + EXTERNAL_SENSOR2, + EXTERNAL_SENSOR3, + EXTERNAL_SENSOR4 +} external_sensor; + +typedef enum { + TEMP_SENSOR_THERMISTOR = 0, + TEMP_SENSOR_BJT, + TEMP_SENSOR_DEFAULT +} temp_sensor_type; + +typedef enum { + FAN_TYPE_PWM_PUSH_PULL = 0, + FAN_TYPE_DAC_POWER, + FAN_TYPE_PWM_OPEN_DRAIN, + FAN_TYPE_RESERVED +} fan_type; +#define FAN_TYPE_PWM_CHECK 1 /* bit 0 must be 0 for PWM */ + +typedef enum { + FAN_MODE_AUTO_RPM = 0, + FAN_MODE_AUTO_PWM_DAC, + FAN_MODE_MANUAL_RPM, + FAN_MODE_MANUAL_PWM_DAC, + FAN_MODE_DEFAULT +} fan_mode; + +typedef enum { + FAN_PWM_FREQ_23500 = 0, + FAN_PWM_FREQ_11750, + FAN_PWM_FREQ_5875, + FAN_PWM_FREQ_220 +} fan_pwm_freq; + +typedef enum { + FAN_TEMP_PECI = 0, + FAN_TEMP_EXTERNAL_1, + FAN_TEMP_EXTERNAL_2, + FAN_TEMP_TSI = 4, + FAN_TEMP_MXM, +} fan_temp_source; + +typedef enum { + FAN_UP_RATE_2HZ = 0, + FAN_UP_RATE_5HZ, + FAN_UP_RATE_10HZ, + FAN_UP_RATE_20HZ, + FAN_UP_RATE_DEFAULT, + FAN_UP_RATE_JUMP = 8 +} fan_rate_up; + +typedef enum { + FAN_DOWN_RATE_2HZ = 0, + FAN_DOWN_RATE_5HZ, + FAN_DOWN_RATE_10HZ, + FAN_DOWN_RATE_20HZ, + FAN_DOWN_RATE_DEFAULT, + FAN_DOWN_RATE_SAME_AS_UP, + FAN_DOWN_RATE_JUMP = 8 +} fan_rate_down; + +typedef enum { + FAN_FOLLOW_STEP = 0, + FAN_FOLLOW_INTERPOLATION +} fan_follow; + +struct fintek_fan { + uint8_t fan; + external_sensor sensor; + temp_sensor_type stype; + fan_temp_source temp_source; + fan_type ftype; + fan_mode fmode; + fan_pwm_freq fan_freq; + fan_rate_up rate_up; + fan_rate_down rate_down; + fan_follow follow; + uint8_t *boundaries; + uint8_t *sections; +}; + +#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 -10 +#define HWM_STATUS_SECTIONS_WRONG_ORDER -11 +#define HWM_STATUS_WARNING_SENSOR_DISCONECTED 1 +#define HWM_STATUS_WARNING_FAN_NOT_PWM 2 + +#define CPU_DAMAGE_TEMP 110 + +/* + * Boundaries order is from highest temp. to lowest. Values from 0 to 127. + * Boundaries should be defined as u8 boundaries[FINTEK_BOUNDARIES_SIZE]. + */ +#define FINTEK_BOUNDARIES_SIZE 4 +/* + * Section defines the duty_cycle/voltage to be used based on where the + * temperature lies with respect to the boundaries. There are 5 sections + * (4 boundaries) and the order must be from highest to lowest. Values + * from 0% to 100%, will be converted internally to percent of 255. + * Sections should be defined as u8 sections[FINTEK_SECTIONS_SIZE]. + */ +#define FINTEK_SECTIONS_SIZE 5 + +/* + * When using external sensor, its type must be defined. When using PECI, + * TSI or MXM use IGNORE_SENSOR to indicate so. + */ +int set_sensor_type(u16 base_address, external_sensor sensor, + temp_sensor_type type); + +/* + * Define the temperature source used to control a fan. + */ +int set_fan_temperature_source(u16 base_address, u8 fan, + fan_temp_source source); + +/* + * Define if fan is controlled through PWM or absolute voltage powering it + * (DAC). Then, under mode, define if control is automatic (SIO) or manual + * (CPU, through ACPI). Notice there needs to be a match between type and + * mode (PWM with PWM or DAC with DAC). + */ +int set_fan_type_mode(u16 base_address, u8 fan, fan_type type, fan_mode mode); + +/* + * For fans controlled through pulse width, define the base frequency used. + */ +int set_pwm_frequency(u16 base_address, u8 fan, fan_pwm_freq frequency); + +/* + * For fintek SIO HWM there are 4 (temperature) boundaries points, defining + * 5 sections (1 fan speed per section). Start with the highest temperature/ + * speed. Temperature is in Celsius, speed is in percentile of max speed. The + * highest speed should be 100%, no requirements for minimum speed, could be + * 0 or above 0. + */ +int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections); + +/* + * Define how often temperature is measured to change fan speed. + */ +int set_fan_speed_change_rate(u16 base_address, u8 fan, fan_rate_up rate_up, + fan_rate_down rate_down); + +/* + * There a 2 ways a fan can be controlled: A single speed per section, or + * interpolation. Under interpolation, the section speed is the speed at the + * lowest temperature of the section (0 Celsius for the lowest section), and + * it's the speed of the next section at the boundary to the next section. + * In between these 2 points, it's a linear function. For example, midway + * between temperature points it'll have a speed that is midway between the + * section speed and next section speed. Obviously, there's no variation for + * the highest section, reason why it must be 100% max speed. + */ +int set_fan_follow(u16 base_address, u8 fan, fan_follow follow); + +/* + * This is an upper level API which calls all the above APIs in the + * appropriate order. Any API failure will be displayed. Alerts will + * also be displayed, but will not interrupt the sequence, while errors + * will interrupt the sequence. + */ +int set_fan(struct fintek_fan *fan_init); + +#endif /* SUPERIO_FINTEK_FAN_CONTROL_H */ diff --git a/src/superio/fintek/f81803a/Kconfig b/src/superio/fintek/f81803a/Kconfig new file mode 100644 index 0000000..e1aa537 --- /dev/null +++ b/src/superio/fintek/f81803a/Kconfig @@ -0,0 +1,28 @@ +## +## This file is part of the coreboot project. +## +## Copyright (C) 2009 Ronald G. Minnich +## Copyright (C) 2014 Edward O'Callaghan eocallaghan@alterapraxis.com +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +config SUPERIO_FINTEK_F81803A + bool + select SUPERIO_FINTEK_COMMON_PRE_RAM + +config SUPERIO_FINTEK_FAN_CONTROL + bool + default n + +config SUPERIO_FINTEK_FAN_API_CALL + depends on SUPERIO_FINTEK_FAN_CONTROL + bool + default n diff --git a/src/superio/fintek/f81803a/Makefile.inc b/src/superio/fintek/f81803a/Makefile.inc new file mode 100644 index 0000000..6fe13aa --- /dev/null +++ b/src/superio/fintek/f81803a/Makefile.inc @@ -0,0 +1,30 @@ +## +## This file is part of the coreboot project. +## +## Copyright (C) 2011 Advanced Micro Devices, Inc. +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; either version 2 of the License, or +## (at your option) any later version. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +ifeq ($(CONFIG_BOOTBLOCK_CONSOLE),y) +bootblock-$(CONFIG_SUPERIO_FINTEK_F81803A) += ../common/early_serial.c +endif + +## Notice: For fan control at romstage, HWM must be initialized before +## the API is called. Ramstage can use devicetree to initialize it. + +romstage-$(CONFIG_SUPERIO_FINTEK_F81803A) += ../common/early_serial.c +romstage-$(CONFIG_SUPERIO_FINTEK_FAN_CONTROL) += fan_control.c +romstage-$(CONFIG_SUPERIO_FINTEK_FAN_API_CALL) += ../common/fan_api_call.c + +ramstage-$(CONFIG_SUPERIO_FINTEK_F81803A) += superio.c +ramstage-$(CONFIG_SUPERIO_FINTEK_FAN_CONTROL) += fan_control.c +ramstage-$(CONFIG_SUPERIO_FINTEK_FAN_API_CALL) += ../common/fan_api_call.c diff --git a/src/superio/fintek/f81803a/acpi/superio.asl b/src/superio/fintek/f81803a/acpi/superio.asl new file mode 100644 index 0000000..ae8e6dc --- /dev/null +++ b/src/superio/fintek/f81803a/acpi/superio.asl @@ -0,0 +1,266 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2011 Christoph Grenz christophg+cb@grenz-bonn.de + * Copyright (C) 2013 secunet Security Networks AG + * Copyright (C) 2019, Silverback, ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * Include this file into a mainboard's DSDT _SB device tree and it will + * expose the F81803A SuperIO and some of its functionality. + * + * It allows the change of IO ports, IRQs and DMA settings on logical + * devices, disabling and reenabling logical devices and controlling power + * saving mode on logical devices or the whole chip. + * + * LDN State + * 0x1 UARTA Implemented, partially tested + * 0x2 UARTB Implemented, partially tested + * 0x4 HWM Not implemented + * 0x5 KBC Not implemented + * 0x6 GPIO6 Not implemented + * 0x7 WDT0&PLED Not implemented + * 0xa ACPI/PME/ERP Partially implemented + * + * Controllable through preprocessor defines: + * SUPERIO_DEV Device identifier for this SIO (e.g. SIO0) + * SUPERIO_PNP_BASE I/o address of the first PnP configuration register + * F81803A_SHOW_UARTA If defined, UARTA will be exposed. + * F81803A_SHOW_UARTB If defined, UARTB will be exposed. + * F81803A_SHOW_HWMON If defined, the hardware monitor will be exposed. + * F81803A_SHOW_PME If defined, the PME/EARP/ACPI will be exposed. + * + * Known issue: + * Do not enable UARTA and UARTB simultaneously, Linux boot will crash. + * Select one or the other. + */ +#undef SUPERIO_CHIP_NAME +#define SUPERIO_CHIP_NAME F81803A +#include <superio/acpi/pnp.asl> + +#undef PNP_DEFAULT_PSC +#define PNP_DEFAULT_PSC Return (0) /* no power management */ + +Device(SUPERIO_DEV) { + Name (_HID, EisaId("PNP0A05")) + Name (_STR, Unicode("Fintek F81803A Super I/O")) + Name (_UID, SUPERIO_UID(SUPERIO_DEV,)) + + /* Mutex for accesses to the configuration ports */ + Mutex(CRMX, 1) + + /* SuperIO configuration ports */ + OperationRegion (CREG, SystemIO, SUPERIO_PNP_BASE, 0x02) + Field (CREG, ByteAcc, NoLock, Preserve) + { + PNP_ADDR_REG, 8, + PNP_DATA_REG, 8 + } + IndexField (ADDR, DATA, ByteAcc, NoLock, Preserve) + { + Offset (0x07), + PNP_LOGICAL_DEVICE, 8, /* Logical device selector */ + Offset (0x30), + PNP_DEVICE_ACTIVE, 1, /* Logical device activation */ + Offset (0x60), + PNP_IO0_HIGH_BYTE, 8, /* First I/O port base - high byte */ + PNP_IO0_LOW_BYTE, 8, /* First I/O port base - low byte */ + Offset (0x62), + PNP_IO1_HIGH_BYTE, 8, /* Second I/O port base - high byte */ + PNP_IO1_LOW_BYTE, 8, /* Second I/O port base - low byte */ + Offset (0x70), + PNP_IRQ0, 8, /* First IRQ */ + offset(0xFB), + APC5, 8, /* PME ACPI Control Register 5 */ + } + + Method(_CRS) + { + /* Announce the used i/o ports to the OS */ + Return (ResourceTemplate () { + IO (Decode16, SUPERIO_PNP_BASE, SUPERIO_PNP_BASE, 0x01, 0x02) + }) + } + + #undef PNP_ENTER_MAGIC_1ST + #undef PNP_ENTER_MAGIC_2ND + #undef PNP_ENTER_MAGIC_3RD + #undef PNP_ENTER_MAGIC_4TH + #undef PNP_EXIT_MAGIC_1ST + #undef PNP_EXIT_SPECIAL_REG + #undef PNP_EXIT_SPECIAL_VAL + #define PNP_ENTER_MAGIC_1ST 0x87 + #define PNP_ENTER_MAGIC_2ND 0x87 + #define PNP_EXIT_MAGIC_1ST 0xaa + #include <superio/acpi/pnp_config.asl> + +#ifdef F81803A_SHOW_UARTA + #undef SUPERIO_UART_LDN + #undef SUPERIO_UART_PM_REG + #undef SUPERIO_UART_PM_VAL + #undef SUPERIO_UART_PM_LDN + #define SUPERIO_UART_LDN 1 + + Device (SUPERIO_ID(SER, SUPERIO_UART_LDN)) { + Name (_HID, EisaId ("PNP0501")) + Name (_UID, SUPERIO_UID(SER, SUPERIO_UART_LDN)) + + Method (_STA) + { + PNP_GENERIC_STA(SUPERIO_UART_LDN) + } + + Method (_CRS, 0, Serialized) + { + Name (CRS, ResourceTemplate () { + IO (Decode16, 0x0000, 0x0000, 0x08, 0x08, IO0) + IRQNoFlags (IR0) {} + }) + ENTER_CONFIG_MODE (SUPERIO_UART_LDN) + PNP_READ_IO(PNP_IO0, CRS, IO0) + PNP_READ_IRQ(PNP_IRQ0, CRS, IR0) + EXIT_CONFIG_MODE () + Return (CRS) + } + + Name (_PRS, ResourceTemplate () + { + StartDependentFn (0,0) { + IO (Decode16, 0x03f8, 0x03f8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (0,0) { + IO (Decode16, 0x02f8, 0x02f8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (1,0) { + IO (Decode16, 0x03e8, 0x03e8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (1,0) { + IO (Decode16, 0x02e8, 0x02e8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (2,0) { + IO (Decode16, 0x0100, 0x0ff8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + EndDependentFn() + }) + + Method (_SRS, 1, Serialized) + { + Name (TMPL, ResourceTemplate () { + IO (Decode16, 0x0000, 0x0000, 0x00, 0x00, IO0) + IRQNoFlags (IR0) {} + }) + ENTER_CONFIG_MODE (SUPERIO_UART_LDN) + PNP_WRITE_IO(PNP_IO0, Arg0, IO0) + PNP_WRITE_IRQ(PNP_IRQ0, Arg0, IR0) + Store (One, PNP_DEVICE_ACTIVE) + EXIT_CONFIG_MODE () + } + } +#endif + +#ifdef F81803A_SHOW_UARTB + #undef SUPERIO_UART_LDN + #undef SUPERIO_UART_PM_REG + #undef SUPERIO_UART_PM_VAL + #undef SUPERIO_UART_PM_LDN + #define SUPERIO_UART_LDN 2 + + Device (SUPERIO_ID(SER, SUPERIO_UART_LDN)) { + Name (_HID, EisaId ("PNP0501")) + Name (_UID, SUPERIO_UID(SER, SUPERIO_UART_LDN)) + + Method (_STA) + { + PNP_GENERIC_STA(SUPERIO_UART_LDN) + } + + Method (_CRS, 0, Serialized) + { + Name (CRS, ResourceTemplate () { + IO (Decode16, 0x0000, 0x0000, 0x08, 0x08, IO0) + IRQNoFlags (IR0) {} + }) + ENTER_CONFIG_MODE (SUPERIO_UART_LDN) + PNP_READ_IO(PNP_IO0, CRS, IO0) + PNP_READ_IRQ(PNP_IRQ0, CRS, IR0) + EXIT_CONFIG_MODE () + Return (CRS) + } + + Name (_PRS, ResourceTemplate () + { + StartDependentFn (0,0) { + IO (Decode16, 0x03f8, 0x03f8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (0,0) { + IO (Decode16, 0x02f8, 0x02f8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (1,0) { + IO (Decode16, 0x03e8, 0x03e8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (1,0) { + IO (Decode16, 0x02e8, 0x02e8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + StartDependentFn (2,0) { + IO (Decode16, 0x0100, 0x0ff8, 0x08, 0x08) + IRQNoFlags () {3,4,5,7,9,10,11,12} + } + EndDependentFn() + }) + + Method (_SRS, 1, Serialized) + { + Name (TMPL, ResourceTemplate () { + IO (Decode16, 0x0000, 0x0000, 0x00, 0x00, IO0) + IRQNoFlags (IR0) {} + }) + ENTER_CONFIG_MODE (SUPERIO_UART_LDN) + PNP_WRITE_IO(PNP_IO0, Arg0, IO0) + PNP_WRITE_IRQ(PNP_IRQ0, Arg0, IR0) + Store (One, PNP_DEVICE_ACTIVE) + EXIT_CONFIG_MODE () + } + } +#endif + +#ifdef F81803A_SHOW_PME + #undef SUPERIO_PME_LDN + #define SUPERIO_PME_LDN 0x0A + + OperationRegion(APCx, SystemIO, APC5, 0x01) + Field(APCx, ByteAcc, Nolock, Preserve) /* bits in PME ACPI CONTROL Reg 5*/ + { + Offset(0x00), /*Control Reg 5 */ + , 7, + PSIN, 1 /* PSIN_FLAG */ + } + + /* routine to clear PSIN_FLAG in ACPI_CONTROL_REG_5 of SIO */ + Method(CPSI, 0, Serialized) + { + /* DBG0("SIO CPSI")*/ + ENTER_CONFIG_MODE(SUPERIO_PME_LDN) + Store(1, PSIN) + EXIT_CONFIG_MODE() + } +#endif +} diff --git a/src/superio/fintek/f81803a/f81803a.h b/src/superio/fintek/f81803a/f81803a.h new file mode 100644 index 0000000..c986cb8 --- /dev/null +++ b/src/superio/fintek/f81803a/f81803a.h @@ -0,0 +1,71 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2011 Advanced Micro Devices, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * Datasheet: + * - Name: F81803A + */ + +#ifndef SUPERIO_FINTEK_F81803_H +#define SUPERIO_FINTEK_F81803_H + +#define LDN_REG 0x07 /* Not defined under PNP */ +/* Logical Device Numbers (LDN) */ + #define F81803A_SP1 0x01 /* UART1 */ + #define F81803A_SP2 0x02 /* UART2 */ + #define F81803A_HWM 0x04 /* Hardware Monitor */ + #define F81803A_KBC 0x05 /* Keyboard/Mouse */ + #define F81803A_GPIO 0x06 /* General Purpose I/O (GPIO) */ + #define F81803A_WDT 0x07 /* Watch Dog Timer */ + #define F81803A_PME 0x0a /* Power Management Events (PME) */ + +/* Global Control Registers */ +#define CLOCK_SELECT_REG 0x26 +#define FUNC_PROG_SELECT (1<<3) +#define PORT_SELECT_REG 0x27 + +#define TSI_LEVEL_SELECT_REG 0x28 /* FUNC_PROG_SEL = 0 */ +#define TSI_PIN_SELECT_REG 0x28 /* FUNC_PROG_SEL = 1 */ +#define MULTI_FUNC_SEL_REG1 0x29 +#define MULTI_FUNC_SEL_REG2 0x2A +#define MULTI_FUNC_SEL_REG3 0x2B +#define MULTI_FUNC_SEL_REG 0x2C +#define WAKEUP_CONTROL_REG 0x2d + +/* LUN A - PME, ACPI, ERP */ +#define PME_DEVICE_ENABLE_REG 0x30 +#define PME_ENABLE (1<<0) +#define PME_ERP_ENABLE_REG 0xE0 +#define ERP_ENABLE (1<<7) +#define ERP_PME_EN (1<<1) +#define ERP_PSOUT_EN (1<<0) +#define PME_ERP_CONTROL_1_REG 0xE1 +#define PME_ERP_CONTROL_2_REG 0xE2 +#define PME_ERP_PSIN_DEBOUNCE_REG 0xE3 +#define PME_ERP_WAKEUP_ENABLE_REG 0xE8 +#define PME_ERP_MODE_SELECT_REG 0xEC +#define PME_EVENT_ENABLE_1_REG 0xF0 +#define PME_EVENT_STATUS_1_REG 0xF1 +#define PME_EVENT_ENABLE_2_REG 0xF2 +#define PME_EVENT_STATUS_2_REG 0xF3 +#define PME_ACPI_CONTROL_1_REG 0xF4 +#define PME_ACPI_CONTROL_2_REG 0xF5 +#define PME_ACPI_CONTROL_3_REG 0xF6 +#define PME_ACPI_CONTROL_4_REG 0xF7 +#define PME_ACPI_CONTROL_5_REG 0xFB +#define PME_ACPI_CONTROL_6_REG 0xFC + +#endif /* SUPERIO_FINTEK_F81803_H */ diff --git a/src/superio/fintek/f81803a/f81803a_hwm.h b/src/superio/fintek/f81803a/f81803a_hwm.h new file mode 100644 index 0000000..a764705 --- /dev/null +++ b/src/superio/fintek/f81803a/f81803a_hwm.h @@ -0,0 +1,72 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Richard Spiegel richard.spiegel@silverbackltd.com + * Copyright (C) 2019 Silverback ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SUPERIO_FINTEK_F81803_HWM_H +#define SUPERIO_FINTEK_F81803_HWM_H + +#define TP_SENSOR_TYPE 0x6b +#define TP_SENSOR1_TYPE_SHIFT 1 +#define TP_SENSOR2_TYPE_SHIFT 2 +#define TP_SENSOR_TYPE_MASK 0x01 +#define TP_DIODE_STATUS 0x6f +#define TP_MMX_OPEN 0x40 +#define TP_PECI_OPEN 0x20 +#define TP_TSI_OPEN 0x10 +#define TP_EXTERNAL_SENSOR2_OPEN 0x04 +#define TP_EXTERNAL_SENSOR1_OPEN 0x02 + +#define FAN_TYPE_REG 0x94 +#define FAN_TYPE_SHIFT(fan) ((fan - 1) * 2) +#define FAN_TYPE_MASK 0x03 +#define FAN_MODE_REG 0x96 + /* FUNC_PROG_SEL = 0 */ +#define FAN_MODE_SHIFT(fan) ((fan - 1) * 4) +#define FAN_MODE_MASK 0x07 + /* FUNC_PROG_SEL = 1 */ +#define FAN1_ADJ_SEL_SHIFT 0 +#define FAN1_ADJ_SEL_MASK 0x07 +#define FAN_FREQ_SEL_ADD_SHIFT(fan) (fan + 2) +#define FAN_UP_RATE_REG 0x9a +#define FAN_RATE_SHIFT(fan) ((fan - 1) * 2) +#define FAN_RATE_MASK 0x03 +#define FAN_DOWN_RATE_REG 0x9b +#define FAN_DOWN_RATE_DIFF_FROM_UP_SHIFT 7 /* FUNC_PROG_SEL = 1 */ +#define FAN_DIRECT_LOAD_EN_SHIFT 6 /* FUNC_PROG_SEL = 1 */ +#define FAN_FAULT_TIME_REG 0x9f +#define FAN_FUNC_PROG_SEL_SHIFT 7 + +#define FAN_BOUND_TEMP 0xa6 /* 4 temperatures */ +#define FAN_SECTION_SPEED 0xaa /* 5 sections */ +#define FAN_TMP_MAPPING 0xaf +#define FAN_TEMP_SEL_HIGH_SHIFT 7 +#define FAN_PWM_FREQ_SEL_SHIFT 6 +#define FAN_INTERPOLATION_SHIFT 4 +#define FAN_JUMP_UP_SHIFT 3 +#define FAN_JUMP_DOWN_SHIFT 2 +#define FAN_TEMP_SEL_LOW_SHIFT 0 +#define FAN_TEMP_SEL_LOW_MASK 0x03 +#define FAN_BIT_MASK 0x01 + +#define FAN_ADJUST(fan, start) (((fan - 1) * 0x10) + start) + +#define STATUS_INVALID_VALUE -1 +#define STATUS_INVALID_ORDER -2 + +#define FIRST_FAN 1 +#define LAST_FAN 2 +#define MAX_DUTY 100 + +#endif /* SUPERIO_FINTEK_F81803_HWM_H */ diff --git a/src/superio/fintek/f81803a/fan_control.c b/src/superio/fintek/f81803a/fan_control.c new file mode 100644 index 0000000..17ae9c6 --- /dev/null +++ b/src/superio/fintek/f81803a/fan_control.c @@ -0,0 +1,362 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Richard Spiegel richard.spiegel@silverbackltd.com + * Copyright (C) 2019 Silverback ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include "../common/fan_control.h" +#include "f81803a_hwm.h" + +static const char msg_err_invalid[] = "Error: invalid"; +static const char msg_err_wrong_order[] = "Error: wrong order,"; +static const char msg_err_fan[] = "fan"; +static const char msg_err_temp_source[] = "temperature source"; +static const char msg_err_type[] = "type"; +static const char msg_err_mode[] = "mode"; +static const char msg_err_rate[] = "change rate"; +static const char msg_err_frequency[] = "frequency"; +static const char msg_err_temp_sensor[] = "temperature sensor"; +static const char msg_err_bondary[] = "boundary"; +static const char msg_err_section[] = "section"; +static const char no_msg[] = ""; + +struct cross_ref { + int selection; + const char *message; +}; +static struct cross_ref msg_table[] = { + {HWM_STATUS_INVALID_FAN, msg_err_fan}, + {HWM_STATUS_INVALID_TEMP_SOURCE, msg_err_temp_source}, + {HWM_STATUS_INVALID_TYPE, msg_err_type}, + {HWM_STATUS_INVALID_MODE, msg_err_mode}, + {HWM_STATUS_INVALID_RATE, msg_err_rate}, + {HWM_STATUS_INVALID_FREQUENCY, msg_err_frequency}, + {HWM_STATUS_INVALID_TEMP_SENSOR, msg_err_temp_sensor}, + {0, NULL}, +}; + +static const char *get_msg(int err) +{ + int i = 0; + while (msg_table[i].selection) { + if (msg_table[i].selection == err) + return msg_table[i].message; + i++; + } + return no_msg; +} + +static int message_invalid_1(int err, u8 fan) +{ + if (err == HWM_STATUS_INVALID_FAN) + printk(BIOS_ERR, "%s %s %d!\n", msg_err_invalid, get_msg(err), fan); + else + printk(BIOS_ERR, "%s Fan %d %s!\n", msg_err_invalid, fan, get_msg(err)); + return err; +} + +static int message_invalid_2(int err, u8 fan) +{ + switch (err) { + case HWM_STATUS_INVALID_BOUNDARY_VALUE: + printk(BIOS_ERR, "%s fan %d %s value!\n", msg_err_invalid, fan, + msg_err_bondary); + break; + case HWM_STATUS_INVALID_SECTION_VALUE: + printk(BIOS_ERR, "%s fan %d %s value!\n", msg_err_invalid, fan, + msg_err_section); + break; + case HWM_STATUS_BOUNDARY_WRONG_ORDER: + printk(BIOS_ERR, "%s fan %d %s!\n", msg_err_wrong_order, fan, msg_err_bondary); + break; + case HWM_STATUS_SECTIONS_WRONG_ORDER: + printk(BIOS_ERR, "%s fan %d %s!\n", msg_err_wrong_order, fan, msg_err_section); + break; + default: + break; + } + return err; +} + +static void write_hwm_reg(u16 address, u8 index, u8 value) +{ + u16 index_add, data_add; + index_add = address | 0x0001; /* force odd address */ + data_add = index_add + 1; + outb(index, index_add); + outb(value, data_add); +} + +static u8 read_hwm_reg(u16 address, u8 index) +{ + u16 index_add, data_add; + index_add = address | 0x0001; /* force odd address */ + data_add = index_add + 1; + outb(index, index_add); + return inb(data_add); +} + +static void hwm_reg_modify(u16 address, u8 index, u8 shift, u8 mask, + u8 value) +{ + u8 use_mask = mask << shift; + u8 use_value = (value & mask) << shift; + u8 temp = read_hwm_reg(address, index); + + temp &= ~use_mask; + temp |= use_value; + write_hwm_reg(address, index, temp); +} + +/* + * Registers 0x94,0x95, 0x96 and 0x9b have 2 versions (banks) selected through + * bit 7 of register 0x9f. + */ +static inline void select_hwm_bank(u16 address, u8 value) +{ + hwm_reg_modify(address, FAN_FAULT_TIME_REG, FAN_FUNC_PROG_SEL_SHIFT, + FAN_BIT_MASK, value); +} + +/* + * Boundaries and sections must be presented in the same order as in the HWM + * registers, that is, from highest value to lowest. This procedure checks for + * the correct order. + */ +static int check_value_seq(u8 *values, u8 count) +{ + u8 last_value = CPU_DAMAGE_TEMP; + u8 current_value, i; + for (i = 0; i < count; i++) { + current_value = values[i]; + if (current_value > CPU_DAMAGE_TEMP) + return STATUS_INVALID_VALUE; + if (current_value >= last_value) + return STATUS_INVALID_ORDER; + last_value = current_value; + } + return HWM_STATUS_SUCCESS; +} + +int set_sensor_type(u16 base_address, external_sensor sensor, + temp_sensor_type type) +{ + u8 sensor_status = read_hwm_reg(base_address, TP_DIODE_STATUS); + + printk(BIOS_DEBUG, "%s\n", __func__); + switch (sensor) { + case EXTERNAL_SENSOR1: + if (sensor_status & TP_EXTERNAL_SENSOR1_OPEN) { + printk(BIOS_WARNING, "Sensor 1 disconected!\n"); + return HWM_STATUS_WARNING_SENSOR_DISCONECTED; + } + hwm_reg_modify(base_address, TP_SENSOR_TYPE, + TP_SENSOR1_TYPE_SHIFT, TP_SENSOR_TYPE_MASK, type); + break; + case EXTERNAL_SENSOR2: + if (sensor_status & TP_EXTERNAL_SENSOR2_OPEN) { + printk(BIOS_WARNING, "Sensor 2 disconected!\n"); + return HWM_STATUS_WARNING_SENSOR_DISCONECTED; + } + hwm_reg_modify(base_address, TP_SENSOR_TYPE, + TP_SENSOR2_TYPE_SHIFT, TP_SENSOR_TYPE_MASK, type); + break; + case IGNORE_SENSOR: + break; + default: + return message_invalid_1(HWM_STATUS_INVALID_TEMP_SENSOR, 0); + } + return HWM_STATUS_SUCCESS; +} + +int set_fan_temperature_source(u16 base_address, u8 fan, + fan_temp_source source) +{ + u8 index, high_value, low_value; + + printk(BIOS_DEBUG, "%s\n", __func__); + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return message_invalid_1(HWM_STATUS_INVALID_FAN, fan); + index = FAN_ADJUST(fan, FAN_TMP_MAPPING); + high_value = (source >> 2) & FAN_BIT_MASK; + low_value = source & FAN_TEMP_SEL_LOW_MASK; + hwm_reg_modify(base_address, index, FAN_TEMP_SEL_HIGH_SHIFT, + FAN_BIT_MASK, high_value); + hwm_reg_modify(base_address, index, FAN_TEMP_SEL_LOW_SHIFT, + FAN_TEMP_SEL_LOW_MASK, low_value); + /* + * Fan 1 has a weight mechanism for adjusting for next fan speed. Basically the idea is + * to react more aggressively (normally CPU fan) based on how high another temperature + * (system, thermistor near the CPU, anything) is. This would be highly platform + * dependent, and by setting the weight temperature same as the control temperature. + * This code cancels the weight mechanism and make it work with any board. If a board + * wants to use the weight mechanism, OEM should implement it after calling the main + * HWM programming. + */ + if (fan == FIRST_FAN) { + select_hwm_bank(base_address, 1); + hwm_reg_modify(base_address, FAN_MODE_REG, + FAN1_ADJ_SEL_SHIFT, FAN1_ADJ_SEL_MASK, source); + select_hwm_bank(base_address, 0); + } + return HWM_STATUS_SUCCESS; +} + +int set_fan_type_mode(u16 base_address, u8 fan, fan_type type, fan_mode mode) +{ + u8 shift; + + printk(BIOS_DEBUG, "%s\n", __func__); + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return message_invalid_1(HWM_STATUS_INVALID_FAN, fan); + select_hwm_bank(base_address, 0); + if (type < FAN_TYPE_RESERVED) { + shift = FAN_TYPE_SHIFT(fan); + hwm_reg_modify(base_address, FAN_TYPE_REG, shift, + FAN_TYPE_MASK, type); + } + if (mode < FAN_MODE_DEFAULT) { + shift = FAN_MODE_SHIFT(fan); + hwm_reg_modify(base_address, FAN_MODE_REG, shift, + FAN_MODE_MASK, mode); + } + return HWM_STATUS_SUCCESS; +} + +int set_pwm_frequency(u16 base_address, u8 fan, fan_pwm_freq frequency) +{ + u8 shift, index, byte; + + printk(BIOS_DEBUG, "%s\n", __func__); + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return message_invalid_1(HWM_STATUS_INVALID_FAN, fan); + byte = read_hwm_reg(base_address, FAN_TYPE_REG); + shift = FAN_TYPE_SHIFT(fan); + if (((byte >> shift) & FAN_TYPE_PWM_CHECK) == FAN_TYPE_PWM_CHECK) { + printk(BIOS_WARNING, "Fan %d not programmed as PWM!\n", fan); + return HWM_STATUS_WARNING_FAN_NOT_PWM; + } + select_hwm_bank(base_address, 1); + shift = FAN_FREQ_SEL_ADD_SHIFT(fan); + byte = (frequency >> 1) & FAN_BIT_MASK; + hwm_reg_modify(base_address, FAN_MODE_REG, shift, FAN_BIT_MASK, + byte); + select_hwm_bank(base_address, 0); + index = FAN_ADJUST(fan, FAN_TMP_MAPPING); + byte = frequency & FAN_BIT_MASK; + hwm_reg_modify(base_address, index, FAN_PWM_FREQ_SEL_SHIFT, + FAN_BIT_MASK, byte); + return HWM_STATUS_SUCCESS; +} + +int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections) +{ + int status, temp; + u8 i, index, value; + + printk(BIOS_DEBUG, "%s\n", __func__); + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return message_invalid_1(HWM_STATUS_INVALID_FAN, fan); + status = check_value_seq(boundaries, + FINTEK_BOUNDARIES_SIZE); + if (status != HWM_STATUS_SUCCESS) { + if (status == STATUS_INVALID_VALUE) + return message_invalid_2(HWM_STATUS_INVALID_BOUNDARY_VALUE, fan); + return message_invalid_2(HWM_STATUS_BOUNDARY_WRONG_ORDER, fan); + } + status = check_value_seq(sections, + FINTEK_SECTIONS_SIZE); + if (status != HWM_STATUS_SUCCESS) { + if (status == STATUS_INVALID_VALUE) + return message_invalid_2(HWM_STATUS_INVALID_SECTION_VALUE, fan); + return message_invalid_2(HWM_STATUS_SECTIONS_WRONG_ORDER, fan); + } + index = FAN_ADJUST(fan, FAN_BOUND_TEMP); + for (i = 0; i < FINTEK_BOUNDARIES_SIZE; i++) { + value = boundaries[i]; + write_hwm_reg(base_address, index, value); + index++; + } + index = FAN_ADJUST(fan, FAN_SECTION_SPEED); + for (i = 0; i < FINTEK_SECTIONS_SIZE; i++) { + value = sections[i]; + if (value > 100) + return message_invalid_2(HWM_STATUS_INVALID_SECTION_VALUE, fan); + temp = (255 * value) / 100; + value = (u8) (temp & 0x00ff); + write_hwm_reg(base_address, index, value); + index++; + } + return HWM_STATUS_SUCCESS; +} + +int set_fan_speed_change_rate(u16 base_address, u8 fan, fan_rate_up rate_up, + fan_rate_down rate_down) +{ + u8 shift, index; + + printk(BIOS_DEBUG, "%s\n", __func__); + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return message_invalid_1(HWM_STATUS_INVALID_FAN, fan); + + index = FAN_ADJUST(fan, FAN_TMP_MAPPING); + shift = FAN_RATE_SHIFT(fan); + + if (rate_up == FAN_UP_RATE_JUMP) { + hwm_reg_modify(base_address, index, FAN_JUMP_UP_SHIFT, + FAN_BIT_MASK, 1); + } else { + hwm_reg_modify(base_address, index, FAN_JUMP_UP_SHIFT, + FAN_BIT_MASK, 0); + if (rate_up < FAN_UP_RATE_DEFAULT) { + hwm_reg_modify(base_address, FAN_UP_RATE_REG, + shift, FAN_RATE_MASK, rate_up); + } + } + + if (rate_down == FAN_DOWN_RATE_JUMP) { + hwm_reg_modify(base_address, index, FAN_JUMP_DOWN_SHIFT, + FAN_BIT_MASK, 1); + } else { + hwm_reg_modify(base_address, index, FAN_JUMP_UP_SHIFT, + FAN_BIT_MASK, 0); + select_hwm_bank(base_address, 0); + if (rate_down < FAN_DOWN_RATE_DEFAULT) { + hwm_reg_modify(base_address, FAN_DOWN_RATE_REG, + shift, FAN_RATE_MASK, rate_down); + hwm_reg_modify(base_address, FAN_DOWN_RATE_REG, + FAN_DOWN_RATE_DIFF_FROM_UP_SHIFT, + FAN_BIT_MASK, 0); + } + if (rate_down == FAN_DOWN_RATE_SAME_AS_UP) { + hwm_reg_modify(base_address, FAN_DOWN_RATE_REG, + FAN_DOWN_RATE_DIFF_FROM_UP_SHIFT, + FAN_BIT_MASK, 1); + } + } + return HWM_STATUS_SUCCESS; +} + +int set_fan_follow(u16 base_address, u8 fan, fan_follow follow) +{ + u8 index; + + printk(BIOS_DEBUG, "%s\n", __func__); + if ((fan < FIRST_FAN) || (fan > LAST_FAN)) + return message_invalid_1(HWM_STATUS_INVALID_FAN, fan); + index = FAN_ADJUST(fan, FAN_TMP_MAPPING); + hwm_reg_modify(base_address, index, FAN_INTERPOLATION_SHIFT, + FAN_BIT_MASK, follow); + return HWM_STATUS_SUCCESS; +} diff --git a/src/superio/fintek/f81803a/superio.c b/src/superio/fintek/f81803a/superio.c new file mode 100644 index 0000000..a07b69e --- /dev/null +++ b/src/superio/fintek/f81803a/superio.c @@ -0,0 +1,78 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2011 Advanced Micro Devices, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/device.h> +#include <device/pnp.h> +#include <superio/conf_mode.h> +#include <console/console.h> +#include <stdlib.h> +#include <pc80/keyboard.h> +#include "f81803a.h" + +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); +} + +static void f81803a_init(struct device *dev) +{ + if (!dev->enabled) + return; + switch (dev->path.pnp.device) { + /* TODO: Might potentially need code for GPIO or WDT. */ + case F81803A_KBC: + pc_keyboard_init(NO_AUX_DEVICE); + break; + case F81803A_PME: + f81803a_pme_init(dev); + break; + } + +} + +static struct device_operations ops = { + .read_resources = pnp_read_resources, + .set_resources = pnp_set_resources, + .enable_resources = pnp_enable_resources, + .enable = pnp_alt_enable, + .init = f81803a_init, + .ops_pnp_mode = &pnp_conf_mode_8787_aa, +}; + +static struct pnp_info pnp_dev_info[] = { + { &ops, F81803A_SP1, PNP_IO0 | PNP_IRQ0, 0x7f8, }, + { &ops, F81803A_SP2, PNP_IO0 | PNP_IRQ0, 0x7f8, }, + { &ops, F81803A_HWM, PNP_IO0 | PNP_IRQ0, 0xff8, }, + { &ops, F81803A_KBC, PNP_IO0 | PNP_IRQ0 | PNP_IRQ1, 0x07f8, }, + { &ops, F81803A_GPIO, PNP_IO0 | PNP_IRQ0, 0x7f8, }, + { &ops, F81803A_WDT, PNP_IO0, 0x7f8 }, + { &ops, F81803A_PME, }, +}; + +static void enable_dev(struct device *dev) +{ + pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info); +} + +struct chip_operations superio_fintek_f81803a_ops = { + CHIP_NAME("Fintek F81803A Super I/O") + .enable_dev = enable_dev +};
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Lance Zhao (Code Review)
-
Martin Roth (Code Review)
-
Richard Spiegel (Code Review)