Change in ...coreboot[master]: superio/fintek/f71808a: Add more optional ramstage registers
Max Blau has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32376
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
superio/fintek/f71808a: Add more optional ramstage registers
More registers added and made optional so they keep untouched/default if ommitted.
Change-Id: I5d8008176d2972976b387c558658b8e70b50af8e Signed-off-by: Max Blau tripleshiftone@gmail.com --- M src/superio/fintek/f71808a/Makefile.inc M src/superio/fintek/f71808a/chip.h M src/superio/fintek/f71808a/f71808a_hwm.c M src/superio/fintek/f71808a/f71808a_multifunc.c A src/superio/fintek/f71808a/f71808a_pme.c M src/superio/fintek/f71808a/fintek_internal.h 6 files changed, 329 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/32376/1
diff --git a/src/superio/fintek/f71808a/Makefile.inc b/src/superio/fintek/f71808a/Makefile.inc index f43e602..2bc27b5 100644 --- a/src/superio/fintek/f71808a/Makefile.inc +++ b/src/superio/fintek/f71808a/Makefile.inc @@ -16,4 +16,5 @@
ramstage-$(CONFIG_SUPERIO_FINTEK_F71808A) += f71808a_multifunc.c ramstage-$(CONFIG_SUPERIO_FINTEK_F71808A) += f71808a_hwm.c +ramstage-$(CONFIG_SUPERIO_FINTEK_F71808A) += f71808a_pme.c ramstage-$(CONFIG_SUPERIO_FINTEK_F71808A) += superio.c diff --git a/src/superio/fintek/f71808a/chip.h b/src/superio/fintek/f71808a/chip.h index f4e80ff..6c4a8db 100644 --- a/src/superio/fintek/f71808a/chip.h +++ b/src/superio/fintek/f71808a/chip.h @@ -22,6 +22,10 @@
struct superio_fintek_f71808a_config {
+ /* Global */ + uint8_t configuration_port_select_0x27; + uint8_t wakeup_control_0x2d; + /* Multi function registers */ uint8_t multi_function_register_0; uint8_t multi_function_register_1; @@ -31,9 +35,16 @@
/* Intel Ibex Peak/PECI/AMD TSI */ uint8_t hwm_peci_tsi_ctrl; + uint8_t hwm_domain1_en_0x0b; uint8_t hwm_tcc_temp;
/* Fan 1 control */ + uint8_t hwm_fan1_boundary_hysteresis_0x98; + uint8_t hwm_vt1_boundary_1_temperature_0xa6; + uint8_t hwm_vt1_boundary_2_temperature_0xa7; + uint8_t hwm_vt1_boundary_3_temperature_0xa8; + uint8_t hwm_vt1_boundary_4_temperature_0xa9; + uint8_t hwm_fan1_seg1_speed; uint8_t hwm_fan1_seg2_speed; uint8_t hwm_fan1_seg3_speed; @@ -48,6 +59,34 @@ uint8_t hwm_fan2_seg4_speed; uint8_t hwm_fan2_seg5_speed; uint8_t hwm_fan2_temp_src; + + /* PME */ + uint8_t eup_enable_0xe0; + uint8_t eup_control1_0xe1; + uint8_t eup_control2_0xe2; + uint8_t eup_psin_deb_0xe3; + uint8_t eup_rsmrst_deb_0xe4; + uint8_t eup_psout_deb_0xe5; + uint8_t eup_pson_deb_0xe6; + uint8_t eup_s5_deb_0xe7; + uint8_t eup_wakeup_event_enable1_0xe8; + uint8_t eup_s3_delay_0xe9; + uint8_t eup_wakeup_event_enable2_0xec; + uint8_t eup_watchdog_control_0xed; + uint8_t eup_watchdog_time_0xee; + uint8_t pme_event_enable1_0xf0; + uint8_t pme_event_enable2_0xf1; + uint8_t pme_event_status1_0xf2; + uint8_t pme_event_status2_0xf3; + uint8_t keep_last_state_select_0xf4; + uint8_t vddok_delay_0xf5; + uint8_t pcirst_control_0xf6; + uint8_t power_sequence_control_0xf7; + uint8_t led_vcc_mode_select_0xf8; + uint8_t led_vsb_mode_select_0xf9; + uint8_t led_mode_select_add_0xfa; + uint8_t intel_dsw_delay_select_0xfc; + uint8_t ri_debounce_select_0xfe; };
#endif /* SUPERIO_FINTEK_F71808A_CHIP_H */ diff --git a/src/superio/fintek/f71808a/f71808a_hwm.c b/src/superio/fintek/f71808a/f71808a_hwm.c index a370219..aec6af8 100644 --- a/src/superio/fintek/f71808a/f71808a_hwm.c +++ b/src/superio/fintek/f71808a/f71808a_hwm.c @@ -22,24 +22,30 @@ #include "chip.h"
/* Intel Ibex Peak/PECI/AMD TSI */ -#define HWM_PECI_TSI_CTRL_REG 0x0a -#define HWM_TCC_TEMPERATURE_REG 0x0c +#define HWM_PECI_TSI_CTRL_REG 0x0a +#define HWM_DOMAIN1_EN 0x0b +#define HWM_TCC_TEMPERATURE_REG 0x0c
/* Fan 1 control */ -#define HWM_FAN1_SEG1_SPEED_REG 0xaa -#define HWM_FAN1_SEG2_SPEED_REG 0xab -#define HWM_FAN1_SEG3_SPEED_REG 0xac -#define HWM_FAN1_SEG4_SPEED_REG 0xad -#define HWM_FAN1_SEG5_SPEED_REG 0xae -#define HWM_FAN1_TEMP_SRC_REG 0xaf +#define HWM_FAN1_BOUNDARY_HYSTERESIS 0x98 +#define HWM_VT1_BOUNDARY_1_TEMPERATURE 0xa6 +#define HWM_VT1_BOUNDARY_2_TEMPERATURE 0xa7 +#define HWM_VT1_BOUNDARY_3_TEMPERATURE 0xa8 +#define HWM_VT1_BOUNDARY_4_TEMPERATURE 0xa9 +#define HWM_FAN1_SEG1_SPEED_REG 0xaa +#define HWM_FAN1_SEG2_SPEED_REG 0xab +#define HWM_FAN1_SEG3_SPEED_REG 0xac +#define HWM_FAN1_SEG4_SPEED_REG 0xad +#define HWM_FAN1_SEG5_SPEED_REG 0xae +#define HWM_FAN1_TEMP_SRC_REG 0xaf
/* Fan 2 control */ -#define HWM_FAN2_SEG1_SPEED_REG 0xba -#define HWM_FAN2_SEG2_SPEED_REG 0xbb -#define HWM_FAN2_SEG3_SPEED_REG 0xbc -#define HWM_FAN2_SEG4_SPEED_REG 0xbd -#define HWM_FAN2_SEG5_SPEED_REG 0xbe -#define HWM_FAN2_TEMP_SRC_REG 0xbf +#define HWM_FAN2_SEG1_SPEED_REG 0xba +#define HWM_FAN2_SEG2_SPEED_REG 0xbb +#define HWM_FAN2_SEG3_SPEED_REG 0xbc +#define HWM_FAN2_SEG4_SPEED_REG 0xbd +#define HWM_FAN2_SEG5_SPEED_REG 0xbe +#define HWM_FAN2_TEMP_SRC_REG 0xbf
void f71808a_hwm_init(struct device *dev) { @@ -55,32 +61,92 @@
pnp_enter_conf_mode(dev);
- pnp_write_index(port, HWM_PECI_TSI_CTRL_REG, reg->hwm_peci_tsi_ctrl); - pnp_write_index(port, HWM_TCC_TEMPERATURE_REG, reg->hwm_tcc_temp); + if (reg->hwm_peci_tsi_ctrl) { + pnp_write_index(port, HWM_PECI_TSI_CTRL_REG, + reg->hwm_peci_tsi_ctrl); + } + if (reg->hwm_tcc_temp) { + pnp_write_index(port, HWM_TCC_TEMPERATURE_REG, + reg->hwm_tcc_temp); + } + if (reg->hwm_domain1_en_0x0b) { + pnp_write_index(port, HWM_DOMAIN1_EN, + reg->hwm_domain1_en_0x0b); + }
- pnp_write_index(port, HWM_FAN1_SEG1_SPEED_REG, + /* fan1_speed */ + if (reg->hwm_fan1_seg1_speed) { + pnp_write_index(port, HWM_FAN1_SEG1_SPEED_REG, reg->hwm_fan1_seg1_speed); - pnp_write_index(port, HWM_FAN1_SEG2_SPEED_REG, + } + if (reg->hwm_fan1_seg2_speed) { + pnp_write_index(port, HWM_FAN1_SEG2_SPEED_REG, reg->hwm_fan1_seg2_speed); - pnp_write_index(port, HWM_FAN1_SEG3_SPEED_REG, + } + if (reg->hwm_fan1_seg3_speed) { + pnp_write_index(port, HWM_FAN1_SEG3_SPEED_REG, reg->hwm_fan1_seg3_speed); - pnp_write_index(port, HWM_FAN1_SEG4_SPEED_REG, + } + if (reg->hwm_fan1_seg4_speed) { + pnp_write_index(port, HWM_FAN1_SEG4_SPEED_REG, reg->hwm_fan1_seg4_speed); - pnp_write_index(port, HWM_FAN1_SEG5_SPEED_REG, + } + if (reg->hwm_fan1_seg5_speed) { + pnp_write_index(port, HWM_FAN1_SEG5_SPEED_REG, reg->hwm_fan1_seg5_speed); - pnp_write_index(port, HWM_FAN1_TEMP_SRC_REG, reg->hwm_fan1_temp_src); + } + if (reg->hwm_fan1_temp_src) { + pnp_write_index(port, HWM_FAN1_TEMP_SRC_REG, + reg->hwm_fan1_temp_src); + }
- pnp_write_index(port, HWM_FAN2_SEG1_SPEED_REG, + /* fan2_speed */ + if (reg->hwm_fan2_seg1_speed) { + pnp_write_index(port, HWM_FAN2_SEG1_SPEED_REG, reg->hwm_fan2_seg1_speed); - pnp_write_index(port, HWM_FAN2_SEG2_SPEED_REG, + } + if (reg->hwm_fan2_seg2_speed) { + pnp_write_index(port, HWM_FAN2_SEG2_SPEED_REG, reg->hwm_fan2_seg2_speed); - pnp_write_index(port, HWM_FAN2_SEG3_SPEED_REG, + } + if (reg->hwm_fan2_seg3_speed) { + pnp_write_index(port, HWM_FAN2_SEG3_SPEED_REG, reg->hwm_fan2_seg3_speed); - pnp_write_index(port, HWM_FAN2_SEG4_SPEED_REG, + } + if (reg->hwm_fan2_seg4_speed) { + pnp_write_index(port, HWM_FAN2_SEG4_SPEED_REG, reg->hwm_fan2_seg4_speed); - pnp_write_index(port, HWM_FAN2_SEG5_SPEED_REG, + } + if (reg->hwm_fan2_seg5_speed) { + pnp_write_index(port, HWM_FAN2_SEG5_SPEED_REG, reg->hwm_fan2_seg5_speed); - pnp_write_index(port, HWM_FAN2_TEMP_SRC_REG, reg->hwm_fan2_temp_src); + } + if (reg->hwm_fan2_temp_src) { + pnp_write_index(port, HWM_FAN2_TEMP_SRC_REG, + reg->hwm_fan2_temp_src); + } + + /* fan1_boundary_temperatures */ + if (reg->hwm_fan1_boundary_hysteresis_0x98) { + pnp_write_index(port, HWM_FAN1_BOUNDARY_HYSTERESIS, + reg->hwm_fan1_boundary_hysteresis_0x98); + } + if (reg->hwm_vt1_boundary_1_temperature_0xa6) { + pnp_write_index(port, HWM_VT1_BOUNDARY_1_TEMPERATURE, + reg->hwm_vt1_boundary_1_temperature_0xa6); + } + if (reg->hwm_vt1_boundary_2_temperature_0xa7) { + pnp_write_index(port, HWM_VT1_BOUNDARY_2_TEMPERATURE, + reg->hwm_vt1_boundary_2_temperature_0xa7); + } + if (reg->hwm_vt1_boundary_3_temperature_0xa8) { + pnp_write_index(port, HWM_VT1_BOUNDARY_3_TEMPERATURE, + reg->hwm_vt1_boundary_3_temperature_0xa8); + } + if (reg->hwm_vt1_boundary_4_temperature_0xa9) { + pnp_write_index(port, HWM_VT1_BOUNDARY_4_TEMPERATURE, + reg->hwm_vt1_boundary_4_temperature_0xa9); + }
pnp_exit_conf_mode(dev); } diff --git a/src/superio/fintek/f71808a/f71808a_multifunc.c b/src/superio/fintek/f71808a/f71808a_multifunc.c index 32119e2..0abb81d 100644 --- a/src/superio/fintek/f71808a/f71808a_multifunc.c +++ b/src/superio/fintek/f71808a/f71808a_multifunc.c @@ -20,11 +20,13 @@ #include "chip.h" #include "fintek_internal.h"
+#define CONFIGURATION_PORT_SELECT 0x27 #define MULTI_FUNC_SEL_REG0 0x28 #define MULTI_FUNC_SEL_REG1 0x29 #define MULTI_FUNC_SEL_REG2 0x2A #define MULTI_FUNC_SEL_REG3 0x2B #define MULTI_FUNC_SEL_REG4 0x2C +#define WAKEUP_CONTROL 0x2D
void f71808a_multifunc_init(struct device *dev) { @@ -32,25 +34,40 @@
pnp_enter_conf_mode(dev);
- /* multi-func select reg0 */ - pnp_write_config(dev, MULTI_FUNC_SEL_REG0, - conf->multi_function_register_0); + if (conf->configuration_port_select_0x27) { + pnp_write_config(dev, CONFIGURATION_PORT_SELECT, + conf->configuration_port_select_0x27); + }
- /* multi-func select reg1 */ - pnp_write_config(dev, MULTI_FUNC_SEL_REG1, - conf->multi_function_register_1); + if (conf->wakeup_control_0x2d) { + pnp_write_config(dev, WAKEUP_CONTROL, + conf->wakeup_control_0x2d); + }
- /* multi-func select reg2 */ - pnp_write_config(dev, MULTI_FUNC_SEL_REG2, - conf->multi_function_register_2); + if (conf->multi_function_register_0) { + pnp_write_config(dev, MULTI_FUNC_SEL_REG0, + conf->multi_function_register_0); + }
- /* multi-func select reg3 */ - pnp_write_config(dev, MULTI_FUNC_SEL_REG3, - conf->multi_function_register_3); + if (conf->multi_function_register_1) { + pnp_write_config(dev, MULTI_FUNC_SEL_REG1, + conf->multi_function_register_1); + }
- /* multi-func select reg4 */ - pnp_write_config(dev, MULTI_FUNC_SEL_REG4, - conf->multi_function_register_4); + if (conf->multi_function_register_2) { + pnp_write_config(dev, MULTI_FUNC_SEL_REG2, + conf->multi_function_register_2); + } + + if (conf->multi_function_register_3) { + pnp_write_config(dev, MULTI_FUNC_SEL_REG3, + conf->multi_function_register_3); + } + + if (conf->multi_function_register_4) { + pnp_write_config(dev, MULTI_FUNC_SEL_REG4, + conf->multi_function_register_4); + }
pnp_exit_conf_mode(dev); } diff --git a/src/superio/fintek/f71808a/f71808a_pme.c b/src/superio/fintek/f71808a/f71808a_pme.c new file mode 100644 index 0000000..5ce1b67 --- /dev/null +++ b/src/superio/fintek/f71808a/f71808a_pme.c @@ -0,0 +1,162 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2014 Edward O'Callaghan eocallaghan@alterapraxis.com + * Copyright (C) 2017 Nicola Corna nicola@corna.info + * + * 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 "chip.h" +#include "fintek_internal.h" + +#define EUP_ENABLE 0xE0 +#define EUP_CONTROL1 0xE1 +#define EUP_CONTROL2 0xE2 +#define EUP_PSIN_DEB 0xE3 +#define EUP_RSMRST_DEB 0xE4 +#define EUP_PSOUT_DEB 0xE5 +#define EUP_PSON_DEB 0xE6 +#define EUP_S5_DEB 0xE7 +#define EUP_WAKEUP_EVENT_ENABLE1 0xE8 +#define EUP_S3_DELAY 0xE9 +#define EUP_WAKEUP_EVENT_ENABLE2 0xEC +#define EUP_WATCHDOG_CONTROL 0xED +#define EUP_WATCHDOG_TIME 0xEE +#define PME_EVENT_ENABLE1 0xF0 +#define PME_EVENT_ENABLE2 0xF1 +#define PME_EVENT_STATUS1 0xF2 +#define PME_EVENT_STATUS2 0xF3 +#define KEEP_LAST_STATE_SELECT 0xF4 +#define VDDOK_DELAY 0xF5 +#define PCIRST_CONTROL 0xF6 +#define POWER_SEQUENCE_CONTROL 0xF7 +#define LED_VCC_MODE_SELECT 0xF8 +#define LED_VSB_MODE_SELECT 0xF9 +#define LED_MODE_SELECT_ADD 0xFA +#define INTEL_DSW_DELAY_SELECT 0xFC +#define RI_DEBOUNCE_SELECT 0xFE + +void f71808a_pme_init(struct device *dev) +{ + const struct superio_fintek_f71808a_config *conf = dev->chip_info; + + pnp_enter_conf_mode(dev); + + if (conf->eup_enable_0xe0) { + pnp_write_config(dev, EUP_ENABLE, + conf->eup_enable_0xe0); + } + if (conf->eup_control1_0xe1) { + pnp_write_config(dev, EUP_CONTROL1, + conf->eup_control1_0xe1); + } + if (conf->eup_control2_0xe2) { + pnp_write_config(dev, EUP_CONTROL2, + conf->eup_control2_0xe2); + } + if (conf->eup_psin_deb_0xe3) { + pnp_write_config(dev, EUP_PSIN_DEB, + conf->eup_psin_deb_0xe3); + } + if (conf->eup_rsmrst_deb_0xe4) { + pnp_write_config(dev, EUP_RSMRST_DEB, + conf->eup_rsmrst_deb_0xe4); + } + if (conf->eup_psout_deb_0xe5) { + pnp_write_config(dev, EUP_PSOUT_DEB, + conf->eup_psout_deb_0xe5); + } + if (conf->eup_pson_deb_0xe6) { + pnp_write_config(dev, EUP_PSON_DEB, + conf->eup_pson_deb_0xe6); + } + if (conf->eup_s5_deb_0xe7) { + pnp_write_config(dev, EUP_S5_DEB, + conf->eup_s5_deb_0xe7); + } + if (conf->eup_wakeup_event_enable1_0xe8) { + pnp_write_config(dev, EUP_WAKEUP_EVENT_ENABLE1, + conf->eup_wakeup_event_enable1_0xe8); + } + if (conf->eup_s3_delay_0xe9) { + pnp_write_config(dev, EUP_S3_DELAY, + conf->eup_s3_delay_0xe9); + } + if (conf->eup_wakeup_event_enable2_0xec) { + pnp_write_config(dev, EUP_WAKEUP_EVENT_ENABLE2, + conf->eup_wakeup_event_enable2_0xec); + } + if (conf->eup_watchdog_control_0xed) { + pnp_write_config(dev, EUP_WATCHDOG_CONTROL, + conf->eup_watchdog_control_0xed); + } + if (conf->eup_watchdog_time_0xee) { + pnp_write_config(dev, EUP_WATCHDOG_TIME, + conf->eup_watchdog_time_0xee); + } + if (conf->pme_event_enable1_0xf0) { + pnp_write_config(dev, PME_EVENT_ENABLE1, + conf->pme_event_enable1_0xf0); + } + if (conf->pme_event_enable2_0xf1) { + pnp_write_config(dev, PME_EVENT_ENABLE2, + conf->pme_event_enable2_0xf1); + } + if (conf->pme_event_status1_0xf2) { + pnp_write_config(dev, PME_EVENT_STATUS1, + conf->pme_event_status1_0xf2); + } + if (conf->pme_event_status2_0xf3) { + pnp_write_config(dev, PME_EVENT_STATUS2, + conf->pme_event_status2_0xf3); + } + if (conf->keep_last_state_select_0xf4) { + pnp_write_config(dev, KEEP_LAST_STATE_SELECT, + conf->keep_last_state_select_0xf4); + } + if (conf->vddok_delay_0xf5) { + pnp_write_config(dev, VDDOK_DELAY, + conf->vddok_delay_0xf5); + } + if (conf->pcirst_control_0xf6) { + pnp_write_config(dev, PCIRST_CONTROL, + conf->pcirst_control_0xf6); + } + if (conf->power_sequence_control_0xf7) { + pnp_write_config(dev, POWER_SEQUENCE_CONTROL, + conf->power_sequence_control_0xf7); + } + if (conf->led_vcc_mode_select_0xf8) { + pnp_write_config(dev, LED_VCC_MODE_SELECT, + conf->led_vcc_mode_select_0xf8); + } + if (conf->led_vsb_mode_select_0xf9) { + pnp_write_config(dev, LED_VSB_MODE_SELECT, + conf->led_vsb_mode_select_0xf9); + } + if (conf->led_mode_select_add_0xfa) { + pnp_write_config(dev, LED_MODE_SELECT_ADD, + conf->led_mode_select_add_0xfa); + } + if (conf->intel_dsw_delay_select_0xfc) { + pnp_write_config(dev, INTEL_DSW_DELAY_SELECT, + conf->intel_dsw_delay_select_0xfc); + } + if (conf->ri_debounce_select_0xfe) { + pnp_write_config(dev, RI_DEBOUNCE_SELECT, + conf->ri_debounce_select_0xfe); + } + + pnp_exit_conf_mode(dev); +} diff --git a/src/superio/fintek/f71808a/fintek_internal.h b/src/superio/fintek/f71808a/fintek_internal.h index 7bdb94d..28a77a0 100644 --- a/src/superio/fintek/f71808a/fintek_internal.h +++ b/src/superio/fintek/f71808a/fintek_internal.h @@ -22,5 +22,6 @@
void f71808a_multifunc_init(struct device *dev); void f71808a_hwm_init(struct device *dev); +void f71808a_pme_init(struct device *dev);
#endif /* SUPERIO_FINTEK_F71808A_INTERNAL_H */
Hello Felix Held, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32376
to look at the new patch set (#2).
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
superio/fintek/f71808a: Add more optional ramstage registers
More registers added and made optional so they keep untouched/default if omitted.
Change-Id: I5d8008176d2972976b387c558658b8e70b50af8e Signed-off-by: Max Blau tripleshiftone@gmail.com --- M src/superio/fintek/f71808a/Makefile.inc M src/superio/fintek/f71808a/chip.h M src/superio/fintek/f71808a/f71808a_hwm.c M src/superio/fintek/f71808a/f71808a_multifunc.c A src/superio/fintek/f71808a/f71808a_pme.c M src/superio/fintek/f71808a/fintek_internal.h 6 files changed, 329 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/32376/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 2:
(1 comment)
only had a very brief look at the patch
https://review.coreboot.org/#/c/32376/2/src/superio/fintek/f71808a/f71808a_m... File src/superio/fintek/f71808a/f71808a_multifunc.c:
https://review.coreboot.org/#/c/32376/2/src/superio/fintek/f71808a/f71808a_m... PS2, Line 36: : if (conf->configuration_port_select_0x27) { : pnp_write_config(dev, CONFIGURATION_PORT_SELECT, : conf->configuration_port_select_0x27); : } This smells like a recipe for disaster to me. Changing the SIO config interface base address (I haven't seen a board where this was needed) will likely break SIO access and the other case would be that the change would never be applied to the SIO. So I don't see a reason to write the PORT_4E_EN bit. If your intention is to just make PWOK_MODE configurable, then only change that bit instead of the full register.
Hello Felix Held, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32376
to look at the new patch set (#3).
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
superio/fintek/f71808a: Add more optional ramstage registers
More registers added and made optional so they keep untouched/default if omitted.
Change-Id: I5d8008176d2972976b387c558658b8e70b50af8e Signed-off-by: Max Blau tripleshiftone@gmail.com --- M src/superio/fintek/f71808a/Makefile.inc M src/superio/fintek/f71808a/chip.h M src/superio/fintek/f71808a/f71808a_hwm.c M src/superio/fintek/f71808a/f71808a_multifunc.c A src/superio/fintek/f71808a/f71808a_pme.c M src/superio/fintek/f71808a/fintek_internal.h M src/superio/fintek/f71808a/superio.c 7 files changed, 378 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/32376/3
Max Blau has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 3: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/32376/2/src/superio/fintek/f71808a/f71808a_m... File src/superio/fintek/f71808a/f71808a_multifunc.c:
https://review.coreboot.org/#/c/32376/2/src/superio/fintek/f71808a/f71808a_m... PS2, Line 36: : if (conf->configuration_port_select_0x27) { : pnp_write_config(dev, CONFIGURATION_PORT_SELECT, : conf->configuration_port_select_0x27); : }
This smells like a recipe for disaster to me. […]
Done
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c File src/superio/fintek/f71808a/superio.c:
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c... PS3, Line 68: 0x07f8, Many Finteks have 0x0ff8 here. I don't understand this mask so if anything's suspicious let me know. Btw, the code layout change here has been done by the clang optimizer.
Max Blau has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
If not wanted i can revert PME and global registers. They are just cosmetics and can be set by 'irq' as well. But the 'io 0x295' HWM registers are only controllable in ramstage due to device re-initialization (reset to default settings).
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... File src/superio/fintek/f71808a/f71808a_hwm.c:
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... PS4, Line 96: reg->hwm_fan1_seg5_speed); what happens if you want to write zero into one of the registers?
Max Blau has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... File src/superio/fintek/f71808a/f71808a_hwm.c:
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... PS4, Line 96: reg->hwm_fan1_seg5_speed);
what happens if you want to write zero into one of the registers?
then it will be ignored (kept default).
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... File src/superio/fintek/f71808a/f71808a_hwm.c:
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... PS4, Line 96: reg->hwm_fan1_seg5_speed);
then it will be ignored (kept default).
but if you don't want to default, but write zero to it?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
(2 comments)
Thank you for your patch.
You should not score your own changes.
https://review.coreboot.org/#/c/32376/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32376/4//COMMIT_MSG@9 PS4, Line 9: More registers added and made optional so they keep untouched/default if omitted. Please wrap the line after 72/75 characters.
https://review.coreboot.org/#/c/32376/4//COMMIT_MSG@9 PS4, Line 9: added and made Summaries can be written in present tense.
Add more registers and make them optional, so they keep untouched/ their default if omitted.
Max Blau has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4: -Code-Review
(1 comment)
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... File src/superio/fintek/f71808a/f71808a_hwm.c:
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/f71808a_h... PS4, Line 96: reg->hwm_fan1_seg5_speed);
but if you don't want to default, but write zero to it?
E.g. by removing the encapsulating 'if' locally. If that's a problem provide a reference implementation how this section should look like.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
(2 comments)
If C supported algebraic data types, the issue not specified vs. zero would be easy to solve; that would just be some sort of sum type. But yeah, this definitely is a problem in the current code base, but I haven't gotten around to think about a proper generic solution. Often the default option is chosen to be zero, so that only things that are non-default need to be specified; this might not really work here though, since quite a few options are not booleans. An idea would be to introduce another setting (maybe do_pme_init) and only call f71808a_pme_init if this setting is true/1. Sure, this isn't the nicest solution, but it would reduce the the chance of breaking things for existing boards.
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c File src/superio/fintek/f71808a/superio.c:
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c... PS3, Line 48: .read_resources = pnp_read_resources, : .set_resources = pnp_set_resources, : .enable_resources = pnp_enable_resources, : .enable = pnp_alt_enable, : .init = f71808a_init, : .ops_pnp_mode = &pnp_conf_mode_8787_aa, i find it more readable with the right-hand sides being aligned
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c... PS3, Line 68: 0x07f8,
Many Finteks have 0x0ff8 here. I don't understand this mask so if anything's suspicious let me know. […]
to see what the masks do have a loot at the function pnp_get_ioresource in pnp_device.c. basically the number of zeros at the end encodes the size of the IO resource and the number of zeros at the beginning encodes the maximal address where the IO resource may be placed at. the reformatting makes the coding style inconsistent with the existing code.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/chip.h File src/superio/fintek/f71808a/chip.h:
https://review.coreboot.org/#/c/32376/4/src/superio/fintek/f71808a/chip.h@26 PS4, Line 26: _0x27 while these postfixes make the SIO code easier to read, they might make the devicetree settings a bit uglier, since it doesn't really matter there to which register the settings are written to.
Hello Felix Held, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32376
to look at the new patch set (#5).
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
superio/fintek/f71808a: Add more optional ramstage registers
Add more registers and make them optional, so they keep untouched/ their default if omitted.
Change-Id: I5d8008176d2972976b387c558658b8e70b50af8e Signed-off-by: Max Blau tripleshiftone@gmail.com --- M src/superio/fintek/f71808a/chip.h M src/superio/fintek/f71808a/f71808a_hwm.c 2 files changed, 29 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/32376/5
Max Blau has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 5:
I've reduced the patch to a minimum. 'hwm_vt1_boundary_1_temperature' is used as a trigger to enable the new values ('if' block) so other boards using this Fintek are not affected.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 5: Code-Review+1
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 5: Code-Review+2
Felix Held has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
superio/fintek/f71808a: Add more optional ramstage registers
Add more registers and make them optional, so they keep untouched/ their default if omitted.
Change-Id: I5d8008176d2972976b387c558658b8e70b50af8e Signed-off-by: Max Blau tripleshiftone@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32376 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/superio/fintek/f71808a/chip.h M src/superio/fintek/f71808a/f71808a_hwm.c 2 files changed, 29 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Patrick Rudolph: Looks good to me, but someone else must approve
diff --git a/src/superio/fintek/f71808a/chip.h b/src/superio/fintek/f71808a/chip.h index f4e80ff..d838f3a 100644 --- a/src/superio/fintek/f71808a/chip.h +++ b/src/superio/fintek/f71808a/chip.h @@ -22,6 +22,13 @@
struct superio_fintek_f71808a_config {
+ uint8_t hwm_vt1_boundary_1_temperature; + uint8_t hwm_vt1_boundary_2_temperature; + uint8_t hwm_vt1_boundary_3_temperature; + uint8_t hwm_vt1_boundary_4_temperature; + uint8_t hwm_fan1_boundary_hysteresis; + uint8_t hwm_domain1_en; + /* Multi function registers */ uint8_t multi_function_register_0; uint8_t multi_function_register_1; diff --git a/src/superio/fintek/f71808a/f71808a_hwm.c b/src/superio/fintek/f71808a/f71808a_hwm.c index a370219..f5e62f9 100644 --- a/src/superio/fintek/f71808a/f71808a_hwm.c +++ b/src/superio/fintek/f71808a/f71808a_hwm.c @@ -23,6 +23,7 @@
/* Intel Ibex Peak/PECI/AMD TSI */ #define HWM_PECI_TSI_CTRL_REG 0x0a +#define HWM_DOMAIN1_EN 0x0b #define HWM_TCC_TEMPERATURE_REG 0x0c
/* Fan 1 control */ @@ -33,6 +34,12 @@ #define HWM_FAN1_SEG5_SPEED_REG 0xae #define HWM_FAN1_TEMP_SRC_REG 0xaf
+#define HWM_FAN1_BOUNDARY_HYSTERESIS 0x98 +#define HWM_VT1_BOUNDARY_1_TEMPERATURE 0xa6 +#define HWM_VT1_BOUNDARY_2_TEMPERATURE 0xa7 +#define HWM_VT1_BOUNDARY_3_TEMPERATURE 0xa8 +#define HWM_VT1_BOUNDARY_4_TEMPERATURE 0xa9 + /* Fan 2 control */ #define HWM_FAN2_SEG1_SPEED_REG 0xba #define HWM_FAN2_SEG2_SPEED_REG 0xbb @@ -55,6 +62,21 @@
pnp_enter_conf_mode(dev);
+ if (reg->hwm_vt1_boundary_1_temperature) { + pnp_write_index(port, HWM_VT1_BOUNDARY_4_TEMPERATURE, + reg->hwm_vt1_boundary_4_temperature); + pnp_write_index(port, HWM_VT1_BOUNDARY_3_TEMPERATURE, + reg->hwm_vt1_boundary_3_temperature); + pnp_write_index(port, HWM_VT1_BOUNDARY_2_TEMPERATURE, + reg->hwm_vt1_boundary_2_temperature); + pnp_write_index(port, HWM_VT1_BOUNDARY_1_TEMPERATURE, + reg->hwm_vt1_boundary_1_temperature); + pnp_write_index(port, HWM_FAN1_BOUNDARY_HYSTERESIS, + reg->hwm_fan1_boundary_hysteresis); + pnp_write_index(port, HWM_DOMAIN1_EN, + reg->hwm_domain1_en); + } + pnp_write_index(port, HWM_PECI_TSI_CTRL_REG, reg->hwm_peci_tsi_ctrl); pnp_write_index(port, HWM_TCC_TEMPERATURE_REG, reg->hwm_tcc_temp);
-
Felix Held (Code Review) -
Max Blau (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)