Krystian Hebel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31616
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
superio/ite/common: add option for enabling 5 FANs
Some ITEs have more than 3 independent FAN controller outputs. As the initial implementation assumed only 3 outputs some registers are not consequently numbered. This change adds macros for accessing those registers.
Additionally some chips have SmartGuardian always enabled, without the option for turning it off. For these chips bits that were responsible for ON/OFF control are either reserved or have different meaning. Another Kconfig option was added to disable ON/OFF functionality on platforms that do not support it.
Change-Id: Icd60a16b6b5583a3b981bdc220aac472c2a8f40f Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com --- M src/superio/ite/common/Kconfig M src/superio/ite/common/env_ctrl.c M src/superio/ite/common/env_ctrl.h M src/superio/ite/common/env_ctrl_chip.h 4 files changed, 132 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31616/1
diff --git a/src/superio/ite/common/Kconfig b/src/superio/ite/common/Kconfig index 8e52cf4..a4b7ece 100644 --- a/src/superio/ite/common/Kconfig +++ b/src/superio/ite/common/Kconfig @@ -4,6 +4,7 @@ ## Copyright (C) 2009 Ronald G. Minnich ## Copyright (C) 2014 Edward O'Callaghan eocallaghan@alterapraxis.com ## Copyright (C) 2016 secunet Security Networks AG +## Copyright (C) 2019 Protectli ## ## 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 @@ -41,4 +42,13 @@ help The second FAN controller has a separate frequency setting.
+config SUPERIO_ITE_ENV_CTRL_NO_ONOFF + bool + help + FAN controller always works in SmartGuardian mode + +config SUPERIO_ITE_ENV_CTRL_5FANS + bool + help + ITE FAN controller has 5 independent outputs endif diff --git a/src/superio/ite/common/env_ctrl.c b/src/superio/ite/common/env_ctrl.c index 1dc5bb6..29a9b94 100644 --- a/src/superio/ite/common/env_ctrl.c +++ b/src/superio/ite/common/env_ctrl.c @@ -3,6 +3,7 @@ * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. * Copyright (C) 2016 secunet Security Networks AG + * Copyright (C) 2019 Protectli * * 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 @@ -191,7 +192,9 @@ { u8 reg;
- if (conf->mode == FAN_IGNORE) + if (conf->mode == FAN_IGNORE || + (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_NO_ONOFF) && + conf->mode <= FAN_MODE_OFF)) return;
/* FAN_CTL2 might have its own frequency setting */ @@ -213,23 +216,36 @@ ite_ec_write(base, ITE_EC_FAN_CTL_MODE, reg); }
- if (IS_ENABLED(SUPERIO_ITE_ENV_CTRL_FAN16_CONFIG) + if (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_FAN16_CONFIG) && conf->mode >= FAN_MODE_ON) { reg = ite_ec_read(base, ITE_EC_FAN_TAC_COUNTER_ENABLE); reg |= ITE_EC_FAN_TAC_16BIT_ENABLE(fan); ite_ec_write(base, ITE_EC_FAN_TAC_COUNTER_ENABLE, reg); }
- reg = ite_ec_read(base, ITE_EC_FAN_MAIN_CTL); - if (conf->mode >= FAN_MODE_ON) - reg |= ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); - else - reg &= ~ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); - if (conf->mode >= FAN_SMART_SOFTWARE) - reg |= ITE_EC_FAN_MAIN_CTL_SMART(fan); - else - reg &= ~ITE_EC_FAN_MAIN_CTL_SMART(fan); - ite_ec_write(base, ITE_EC_FAN_MAIN_CTL, reg); + if (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) && fan > 3) { + reg = ite_ec_read(base, ITE_EC_FAN_SEC_CTL); + if (conf->mode >= FAN_MODE_ON) + reg |= ITE_EC_FAN_SEC_CTL_TAC_EN(fan); + else + reg &= ~ITE_EC_FAN_SEC_CTL_TAC_EN(fan); + ite_ec_write(base, ITE_EC_FAN_SEC_CTL, reg); + } else { + reg = ite_ec_read(base, ITE_EC_FAN_MAIN_CTL); + if (conf->mode >= FAN_MODE_ON) + reg |= ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); + else + reg &= ~ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); + + /* Some ITEs have SmartGuardian always enabled */ + if (!IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_NO_ONOFF)) { + if (conf->mode >= FAN_SMART_SOFTWARE) + reg |= ITE_EC_FAN_MAIN_CTL_SMART(fan); + else + reg &= ~ITE_EC_FAN_MAIN_CTL_SMART(fan); + } + ite_ec_write(base, ITE_EC_FAN_MAIN_CTL, reg); + } }
static void enable_beeps(const u16 base, const struct ite_ec_config *const conf) diff --git a/src/superio/ite/common/env_ctrl.h b/src/superio/ite/common/env_ctrl.h index 11316db..a265458 100644 --- a/src/superio/ite/common/env_ctrl.h +++ b/src/superio/ite/common/env_ctrl.h @@ -3,6 +3,7 @@ * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. * Copyright (C) 2016 secunet Security Networks AG + * Copyright (C) 2019 Protectli * * 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 @@ -52,8 +53,26 @@
#define ITE_EC_FAN_TAC_COUNTER_ENABLE 0x0c #define ITE_EC_FAN_TAC_16BIT_ENABLE(x) (1 << ((x)-1)) + +#if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) +#define ITE_EC_FAN_SEC_CTL 0x0c +#define ITE_EC_FAN_SEC_CTL_TAC_EN(x) (1 << (x)) +#define ITE_EC_FAN_TAC_LIMIT(x) \ + ({ \ + (x > 3) \ + ? (0x84 + ((x)-4) * 2) \ + : (0x10 + ((x)-1)); \ + }) +#define ITE_EC_FAN_TAC_EXT_LIMIT(x) \ + ({ \ + (x > 3) \ + ? (0x85 + ((x)-4) * 2) \ + : (0x1b + ((x)-1)); \ + }) +#else #define ITE_EC_FAN_TAC_LIMIT(x) (0x10 + ((x)-1)) #define ITE_EC_FAN_TAC_EXT_LIMIT(x) (0x1b + ((x)-1)) +#endif
#define ITE_EC_FAN_MAIN_CTL 0x13 #define ITE_EC_FAN_MAIN_CTL_TAC_EN(x) (1 << ((x)+3)) @@ -72,7 +91,22 @@ #define ITE_EC_FAN_PWM_CLOCK_51KHZ (7 << 4) #define ITE_EC_FAN_PWM_MIN_DUTY_20 (1 << 3) #define ITE_EC_FAN_CTL_ON(x) (1 << ((x)-1)) + +#if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) +#define ITE_EC_FAN_CTL_PWM_CONTROL(x) \ + ({ \ + (x > 3) \ + ? (0x1e + ((x)-4)) \ + : (0x15 + ((x)-1)); \ + }) +#define ITE_EC_FAN_CTL_TEMPIN_MASK (7 << 3) +#define ITE_EC_FAN_CTL_TEMPIN(x) ((((x)-1) & 7) << 3) +#else #define ITE_EC_FAN_CTL_PWM_CONTROL(x) (0x15 + ((x)-1)) +#define ITE_EC_FAN_CTL_TEMPIN_MASK (3 << 0) +#define ITE_EC_FAN_CTL_TEMPIN(x) (((x)-1) & 3) +#endif + #define ITE_EC_FAN_CTL_PWM_MODE_SOFTWARE (0 << 7) #define ITE_EC_FAN_CTL_PWM_MODE_AUTOMATIC (1 << 7) #define ITE_EC_FAN_CTL_PWM_DUTY_MASK (ITE_EC_FAN_MAX_PWM << 0) @@ -83,8 +117,6 @@ ? ITE_EC_FAN_MAX_PWM \ : (_p * ITE_EC_FAN_MAX_PWM) / 100; \ }) -#define ITE_EC_FAN_CTL_TEMPIN_MASK (3 << 0) -#define ITE_EC_FAN_CTL_TEMPIN(x) (((x)-1) & 3)
#define ITE_EC_HIGH_TEMP_LIMIT(x) (0x40 + ((x-1) * 2)) #define ITE_EC_LOW_TEMP_LIMIT(x) (0x41 + ((x-1) * 2)) @@ -119,16 +151,64 @@ #define ITE_EC_BEEP_TONE_DIVISOR(x) (((x) & 0x0f) << 4) #define ITE_EC_BEEP_FREQ_DIVISOR(x) (((x) & 0x0f) << 0)
+#if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) +#define ITE_EC_FAN_CTL_TEMP_LIMIT_OFF(x) \ + ({ \ + (x == 5) \ + ? (0xa0) \ + : (0x60 + ((x)-1) * 8); \ + }) +#define ITE_EC_FAN_CTL_TEMP_LIMIT_START(x) \ + ({ \ + (x == 5) \ + ? (0xa1) \ + : (0x61 + ((x)-1) * 8); \ + }) +#define ITE_EC_FAN_CTL_TEMP_LIMIT_FULL(x) \ + ({ \ + (x == 5) \ + ? (0xa2) \ + : (0x62 + ((x)-1) * 8); \ + }) +#define ITE_EC_FAN_CTL_PWM_START(x) \ + ({ \ + (x == 5) \ + ? (0xa3) \ + : (0x63 + ((x)-1) * 8); \ + }) +#define ITE_EC_FAN_CTL_PWM_AUTO(x) \ + ({ \ + (x == 5) \ + ? (0xa4) \ + : (0x64 + ((x)-1) * 8); \ + }) +#define ITE_EC_FAN_CTL_DELTA_TEMP(x) \ + ({ \ + (x == 5) \ + ? (0xa5) \ + : (0x65 + ((x)-1) * 8); \ + }) +#define ITE_EC_FAN_CTL_DELTA_TEMP_INTRVL(c) ((c) & 0x1f) + +#else + #define ITE_EC_FAN_CTL_TEMP_LIMIT_OFF(x) (0x60 + ((x)-1) * 8) #define ITE_EC_FAN_CTL_TEMP_LIMIT_START(x) (0x61 + ((x)-1) * 8) #define ITE_EC_FAN_CTL_TEMP_LIMIT_FULL(x) (0x62 + ((x)-1) * 8) #define ITE_EC_FAN_CTL_PWM_START(x) (0x63 + ((x)-1) * 8) +#define ITE_EC_FAN_CTL_PWM_AUTO(x) (0x64 + ((x)-1) * 8) +#define ITE_EC_FAN_CTL_DELTA_TEMP(x) (0x65 + ((x)-1) * 8) +#endif + +/* Common for ITE_EC_FAN_CTL_PWM_START */ #define ITE_EC_FAN_CTL_PWM_SLOPE_BIT6(s) (((s) & 0x40) << 1) #define ITE_EC_FAN_CTL_PWM_START_DUTY(p) ITE_EC_FAN_CTL_PWM_DUTY(p) -#define ITE_EC_FAN_CTL_PWM_AUTO(x) (0x64 + ((x)-1) * 8) + +/* Common for ITE_EC_FAN_CTL_PWM_AUTO */ #define ITE_EC_FAN_CTL_AUTO_SMOOTHING_EN (1 << 7) #define ITE_EC_FAN_CTL_PWM_SLOPE_LOWER(s) ((s) & 0x3f) -#define ITE_EC_FAN_CTL_DELTA_TEMP(x) (0x65 + ((x)-1) * 8) + +/* Common for ITE_EC_FAN_CTL_DELTA_TEMP */ #define ITE_EC_FAN_CTL_DELTA_TEMP_INTRVL(c) ((c) & 0x1f)
#define ITE_EC_EXTEMP_STATUS 0x88 diff --git a/src/superio/ite/common/env_ctrl_chip.h b/src/superio/ite/common/env_ctrl_chip.h index f8f2e1e..5069b45 100644 --- a/src/superio/ite/common/env_ctrl_chip.h +++ b/src/superio/ite/common/env_ctrl_chip.h @@ -3,6 +3,7 @@ * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. * Copyright (C) 2016 secunet Security Networks AG + * Copyright (C) 2019 Protectli * * 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 @@ -19,7 +20,12 @@ #define SUPERIO_ITE_ENV_CTRL_CHIP_H
#define ITE_EC_TMPIN_CNT 3 + +#if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) +#define ITE_EC_FAN_CNT 5 +#else #define ITE_EC_FAN_CNT 3 +#endif
/* Supported thermal mode on TMPINx */ enum ite_ec_thermal_mode { @@ -105,5 +111,9 @@ #define FAN1 ec.fan[0] #define FAN2 ec.fan[1] #define FAN3 ec.fan[2] +#if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) +#define FAN4 ec.fan[3] +#define FAN5 ec.fan[4] +#endif
#endif /* SUPERIO_ITE_ENV_CTRL_CHIP_H */
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Very nice.
https://review.coreboot.org/#/c/31616/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31616/1//COMMIT_MSG@17 PS1, Line 17: was is
https://review.coreboot.org/#/c/31616/1/src/superio/ite/common/Kconfig File src/superio/ite/common/Kconfig:
https://review.coreboot.org/#/c/31616/1/src/superio/ite/common/Kconfig@48 PS1, Line 48: FAN controller always works in SmartGuardian mode Please add a dot/period at the end of the sentence.
https://review.coreboot.org/#/c/31616/1/src/superio/ite/common/Kconfig@53 PS1, Line 53: ITE FAN controller has 5 independent outputs Please add a dot/period at the end of the sentence.
Hello Felix Held, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31616
to look at the new patch set (#2).
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
superio/ite/common: add option for enabling 5 FANs
Some ITEs have more than 3 independent FAN controller outputs. As the initial implementation assumed only 3 outputs some registers are not consequently numbered. This change adds macros for accessing those registers.
Additionally some chips have SmartGuardian always enabled, without the option for turning it off. For these chips bits that were responsible for ON/OFF control are either reserved or have different meaning. Another Kconfig option is added to disable ON/OFF functionality on platforms that do not support it.
Change-Id: Icd60a16b6b5583a3b981bdc220aac472c2a8f40f Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com --- M src/superio/ite/common/Kconfig M src/superio/ite/common/env_ctrl.c M src/superio/ite/common/env_ctrl.h M src/superio/ite/common/env_ctrl_chip.h 4 files changed, 137 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31616/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 2:
(8 comments)
Thanks for your patch.
I'm not sure about some of the added #if in `env_ctrl.h`. Do you want to keep it from compiling unnecessary code?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@60 PS2, Line 60: */ Lot's of things here might be unused in one case or another, no need to comment.
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@64 PS2, Line 64: #if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) As the definition doesn't change for x <= 3, we don't need the #if, I suppose?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@66 PS2, Line 66: ({ \ Why putting it into a code-block `{}`?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@67 PS2, Line 67: x Please put parameters always in parentheses `(x)`, they may use operators with lower precedence than what follows here.
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@19... PS2, Line 196: #define ITE_EC_FAN_CTL_DELTA_TEMP_INTRVL(c) ((c) & 0x1f) same definition as below?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.c File src/superio/ite/common/env_ctrl.c:
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.c@19... PS2, Line 196: (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_NO_ONOFF) && Please align this more clearly. e.g. tab + 4 spaces in this line and tab + 5 spaces in the next
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.c@21... PS2, Line 219: if (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_FAN16_CONFIG) I thought we fixed this by now. There might be more patches lurking on Gerrit...
Can you please split this into a separate patch so we can merge the fix fast?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl_chip... File src/superio/ite/common/env_ctrl_chip.h:
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl_chip... PS2, Line 114: #if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) No need to guard this, imho.
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 2:
Do you want to keep it from compiling unnecessary code?
Yes. I've recently played a bit with Secure Boot and noticed that every byte in IBB counts, maybe I'm too paranoid about that though.
Hello Felix Held, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31616
to look at the new patch set (#3).
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
superio/ite/common: add option for enabling 5 FANs
Some ITEs have more than 3 independent FAN controller outputs. As the initial implementation assumed only 3 outputs some registers are not consequently numbered. This change adds macros for accessing those registers.
Additionally some chips have SmartGuardian always enabled, without the option for turning it off. For these chips bits that were responsible for ON/OFF control are either reserved or have different meaning. Another Kconfig option is added to disable ON/OFF functionality on platforms that do not support it.
Change-Id: Icd60a16b6b5583a3b981bdc220aac472c2a8f40f Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com --- M src/superio/ite/common/Kconfig M src/superio/ite/common/env_ctrl.c M src/superio/ite/common/env_ctrl.h M src/superio/ite/common/env_ctrl_chip.h 4 files changed, 129 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31616/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 3:
(1 comment)
Do you want to keep it from compiling unnecessary code?
Yes. I've recently played a bit with Secure Boot and noticed that every byte in IBB counts, maybe I'm too paranoid about that though.
So is it arguable? I don't think this code will ever be put into a constrained environment (it should only be used in ramstage, I guess). And removing the #if's would make the whole thing easier to read, IMHO.
https://review.coreboot.org/#/c/31616/3/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/3/src/superio/ite/common/env_ctrl.h@66 PS3, Line 66: ) There is still a forest of parentheses that only distracts they eye, IMHO. If this works, a simple expression should work, too, and we could even align the register offsets somewhat with those of the simpler definitions, e.g. #define ITE_EC_FAN_TAC_LIMIT(x) ((x) ? 0x84 + ((x)-4) * 2 : 0x10 + ((x)-1))
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31616/3/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/3/src/superio/ite/common/env_ctrl.h@66 PS3, Line 66: )
There is still a forest of parentheses that only distracts […]
Another idea that might keep it more readable and could be easier optimized by the compiler:
#define ITE_EC_FAN_TAC_LIMIT(x) \ ((x) > 3 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) \ ? 0x84 + ((x)-4) * 2 \ : 0x10 + ((x)-1))
Hello Felix Held, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31616
to look at the new patch set (#4).
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
superio/ite/common: add option for enabling 5 FANs
Some ITEs have more than 3 independent FAN controller outputs. As the initial implementation assumed only 3 outputs some registers are not consequently numbered. This change adds macros for accessing those registers.
Additionally some chips have SmartGuardian always enabled, without the option for turning it off. For these chips bits that were responsible for ON/OFF control are either reserved or have different meaning. Another Kconfig option is added to disable ON/OFF functionality on platforms that do not support it.
Change-Id: Icd60a16b6b5583a3b981bdc220aac472c2a8f40f Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com --- M src/superio/ite/common/Kconfig M src/superio/ite/common/env_ctrl.c M src/superio/ite/common/env_ctrl.h M src/superio/ite/common/env_ctrl_chip.h 4 files changed, 102 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31616/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 4:
(9 comments)
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@60 PS4, Line 60: #define ITE_EC_FAN_TAC_LIMIT(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@64 PS4, Line 64: #define ITE_EC_FAN_TAC_EXT_LIMIT(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@87 PS4, Line 87: #define ITE_EC_FAN_CTL_PWM_CONTROL(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@14... PS4, Line 144: #define ITE_EC_FAN_CTL_TEMP_LIMIT_OFF(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@14... PS4, Line 148: #define ITE_EC_FAN_CTL_TEMP_LIMIT_START(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@15... PS4, Line 152: #define ITE_EC_FAN_CTL_TEMP_LIMIT_FULL(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@15... PS4, Line 156: #define ITE_EC_FAN_CTL_PWM_START(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@16... PS4, Line 160: #define ITE_EC_FAN_CTL_PWM_AUTO(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@16... PS4, Line 164: #define ITE_EC_FAN_CTL_DELTA_TEMP(x) \ Macros with complex values should be enclosed in parentheses
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31616/3/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/3/src/superio/ite/common/env_ctrl.h@66 PS3, Line 66: )
Another idea that might keep it more readable and could […]
That's good suggestion, thank you
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Thank you, this looks much better already.
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/4/src/superio/ite/common/env_ctrl.h@62 PS4, Line 62: ? (0x84 + ((x)-4) * 2) \ : : (0x10 + ((x)-1)) I would still prefer this to be aligned up with the other register offsets :)
Hello Felix Held, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31616
to look at the new patch set (#5).
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
superio/ite/common: add option for enabling 5 FANs
Some ITEs have more than 3 independent FAN controller outputs. As the initial implementation assumed only 3 outputs some registers are not consequently numbered. This change adds macros for accessing those registers.
Additionally some chips have SmartGuardian always enabled, without the option for turning it off. For these chips bits that were responsible for ON/OFF control are either reserved or have different meaning. Another Kconfig option is added to disable ON/OFF functionality on platforms that do not support it.
Change-Id: Icd60a16b6b5583a3b981bdc220aac472c2a8f40f Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com --- M src/superio/ite/common/Kconfig M src/superio/ite/common/env_ctrl.c M src/superio/ite/common/env_ctrl.h M src/superio/ite/common/env_ctrl_chip.h 4 files changed, 102 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31616/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 5:
(9 comments)
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@60 PS5, Line 60: #define ITE_EC_FAN_TAC_LIMIT(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@64 PS5, Line 64: #define ITE_EC_FAN_TAC_EXT_LIMIT(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@87 PS5, Line 87: #define ITE_EC_FAN_CTL_PWM_CONTROL(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@14... PS5, Line 144: #define ITE_EC_FAN_CTL_TEMP_LIMIT_OFF(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@14... PS5, Line 148: #define ITE_EC_FAN_CTL_TEMP_LIMIT_START(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@15... PS5, Line 152: #define ITE_EC_FAN_CTL_TEMP_LIMIT_FULL(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@15... PS5, Line 156: #define ITE_EC_FAN_CTL_PWM_START(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@16... PS5, Line 160: #define ITE_EC_FAN_CTL_PWM_AUTO(x) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@16... PS5, Line 164: #define ITE_EC_FAN_CTL_DELTA_TEMP(x) \ Macros with complex values should be enclosed in parentheses
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/5/src/superio/ite/common/env_ctrl.h@60 PS5, Line 60: #define ITE_EC_FAN_TAC_LIMIT(x) \
Macros with complex values should be enclosed in parentheses
ugh, I missed it earlier that checkpatch is right here
Hello Felix Held, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31616
to look at the new patch set (#6).
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
superio/ite/common: add option for enabling 5 FANs
Some ITEs have more than 3 independent FAN controller outputs. As the initial implementation assumed only 3 outputs some registers are not consequently numbered. This change adds macros for accessing those registers.
Additionally some chips have SmartGuardian always enabled, without the option for turning it off. For these chips bits that were responsible for ON/OFF control are either reserved or have different meaning. Another Kconfig option is added to disable ON/OFF functionality on platforms that do not support it.
Change-Id: Icd60a16b6b5583a3b981bdc220aac472c2a8f40f Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com --- M src/superio/ite/common/Kconfig M src/superio/ite/common/env_ctrl.c M src/superio/ite/common/env_ctrl.h M src/superio/ite/common/env_ctrl_chip.h 4 files changed, 111 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31616/6
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 6: Code-Review+2
couldn't fully check the new parts due to not having the datasheet; didn't find regressions though and the new functionality didn't look obviously wrong to me
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 6: Code-Review+2
Felix Held has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
superio/ite/common: add option for enabling 5 FANs
Some ITEs have more than 3 independent FAN controller outputs. As the initial implementation assumed only 3 outputs some registers are not consequently numbered. This change adds macros for accessing those registers.
Additionally some chips have SmartGuardian always enabled, without the option for turning it off. For these chips bits that were responsible for ON/OFF control are either reserved or have different meaning. Another Kconfig option is added to disable ON/OFF functionality on platforms that do not support it.
Change-Id: Icd60a16b6b5583a3b981bdc220aac472c2a8f40f Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com Reviewed-on: https://review.coreboot.org/c/31616 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: Nico Huber nico.h@gmx.de --- M src/superio/ite/common/Kconfig M src/superio/ite/common/env_ctrl.c M src/superio/ite/common/env_ctrl.h M src/superio/ite/common/env_ctrl_chip.h 4 files changed, 111 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Felix Held: Looks good to me, approved
diff --git a/src/superio/ite/common/Kconfig b/src/superio/ite/common/Kconfig index 8e52cf4..6c78741 100644 --- a/src/superio/ite/common/Kconfig +++ b/src/superio/ite/common/Kconfig @@ -4,6 +4,7 @@ ## Copyright (C) 2009 Ronald G. Minnich ## Copyright (C) 2014 Edward O'Callaghan eocallaghan@alterapraxis.com ## Copyright (C) 2016 secunet Security Networks AG +## Copyright (C) 2019 Protectli ## ## 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 @@ -41,4 +42,13 @@ help The second FAN controller has a separate frequency setting.
+config SUPERIO_ITE_ENV_CTRL_NO_ONOFF + bool + help + FAN controller always works in SmartGuardian mode. + +config SUPERIO_ITE_ENV_CTRL_5FANS + bool + help + ITE FAN controller has 5 independent outputs. endif diff --git a/src/superio/ite/common/env_ctrl.c b/src/superio/ite/common/env_ctrl.c index 92c9bca..f69273e 100644 --- a/src/superio/ite/common/env_ctrl.c +++ b/src/superio/ite/common/env_ctrl.c @@ -3,6 +3,7 @@ * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. * Copyright (C) 2016 secunet Security Networks AG + * Copyright (C) 2019 Protectli * * 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 @@ -191,7 +192,9 @@ { u8 reg;
- if (conf->mode == FAN_IGNORE) + if (conf->mode == FAN_IGNORE || + (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_NO_ONOFF) && + conf->mode <= FAN_MODE_OFF)) return;
/* FAN_CTL2 might have its own frequency setting */ @@ -220,16 +223,29 @@ ite_ec_write(base, ITE_EC_FAN_TAC_COUNTER_ENABLE, reg); }
- reg = ite_ec_read(base, ITE_EC_FAN_MAIN_CTL); - if (conf->mode >= FAN_MODE_ON) - reg |= ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); - else - reg &= ~ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); - if (conf->mode >= FAN_SMART_SOFTWARE) - reg |= ITE_EC_FAN_MAIN_CTL_SMART(fan); - else - reg &= ~ITE_EC_FAN_MAIN_CTL_SMART(fan); - ite_ec_write(base, ITE_EC_FAN_MAIN_CTL, reg); + if (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) && fan > 3) { + reg = ite_ec_read(base, ITE_EC_FAN_SEC_CTL); + if (conf->mode >= FAN_MODE_ON) + reg |= ITE_EC_FAN_SEC_CTL_TAC_EN(fan); + else + reg &= ~ITE_EC_FAN_SEC_CTL_TAC_EN(fan); + ite_ec_write(base, ITE_EC_FAN_SEC_CTL, reg); + } else { + reg = ite_ec_read(base, ITE_EC_FAN_MAIN_CTL); + if (conf->mode >= FAN_MODE_ON) + reg |= ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); + else + reg &= ~ITE_EC_FAN_MAIN_CTL_TAC_EN(fan); + + /* Some ITEs have SmartGuardian always enabled */ + if (!IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_NO_ONOFF)) { + if (conf->mode >= FAN_SMART_SOFTWARE) + reg |= ITE_EC_FAN_MAIN_CTL_SMART(fan); + else + reg &= ~ITE_EC_FAN_MAIN_CTL_SMART(fan); + } + ite_ec_write(base, ITE_EC_FAN_MAIN_CTL, reg); + } }
static void enable_beeps(const u16 base, const struct ite_ec_config *const conf) diff --git a/src/superio/ite/common/env_ctrl.h b/src/superio/ite/common/env_ctrl.h index 11316db..e67af34 100644 --- a/src/superio/ite/common/env_ctrl.h +++ b/src/superio/ite/common/env_ctrl.h @@ -3,6 +3,7 @@ * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. * Copyright (C) 2016 secunet Security Networks AG + * Copyright (C) 2019 Protectli * * 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 @@ -52,8 +53,20 @@
#define ITE_EC_FAN_TAC_COUNTER_ENABLE 0x0c #define ITE_EC_FAN_TAC_16BIT_ENABLE(x) (1 << ((x)-1)) -#define ITE_EC_FAN_TAC_LIMIT(x) (0x10 + ((x)-1)) -#define ITE_EC_FAN_TAC_EXT_LIMIT(x) (0x1b + ((x)-1)) + +#define ITE_EC_FAN_SEC_CTL 0x0c +#define ITE_EC_FAN_SEC_CTL_TAC_EN(x) (1 << (x)) + +#define ITE_EC_FAN_TAC_LIMIT(x) \ + (((x) > 3 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0x84 + ((x)-4) * 2) \ + : (0x10 + ((x)-1)) \ + ) +#define ITE_EC_FAN_TAC_EXT_LIMIT(x) \ + (((x) > 3 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0x85 + ((x)-4) * 2) \ + : (0x1b + ((x)-1)) \ + )
#define ITE_EC_FAN_MAIN_CTL 0x13 #define ITE_EC_FAN_MAIN_CTL_TAC_EN(x) (1 << ((x)+3)) @@ -72,7 +85,21 @@ #define ITE_EC_FAN_PWM_CLOCK_51KHZ (7 << 4) #define ITE_EC_FAN_PWM_MIN_DUTY_20 (1 << 3) #define ITE_EC_FAN_CTL_ON(x) (1 << ((x)-1)) -#define ITE_EC_FAN_CTL_PWM_CONTROL(x) (0x15 + ((x)-1)) + +#define ITE_EC_FAN_CTL_PWM_CONTROL(x) \ + (((x) > 3 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0x1e + ((x)-4)) \ + : (0x15 + ((x)-1)) \ + ) + +#if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) +#define ITE_EC_FAN_CTL_TEMPIN_MASK (7 << 3) +#define ITE_EC_FAN_CTL_TEMPIN(x) ((((x)-1) & 7) << 3) +#else +#define ITE_EC_FAN_CTL_TEMPIN_MASK (3 << 0) +#define ITE_EC_FAN_CTL_TEMPIN(x) (((x)-1) & 3) +#endif + #define ITE_EC_FAN_CTL_PWM_MODE_SOFTWARE (0 << 7) #define ITE_EC_FAN_CTL_PWM_MODE_AUTOMATIC (1 << 7) #define ITE_EC_FAN_CTL_PWM_DUTY_MASK (ITE_EC_FAN_MAX_PWM << 0) @@ -83,8 +110,6 @@ ? ITE_EC_FAN_MAX_PWM \ : (_p * ITE_EC_FAN_MAX_PWM) / 100; \ }) -#define ITE_EC_FAN_CTL_TEMPIN_MASK (3 << 0) -#define ITE_EC_FAN_CTL_TEMPIN(x) (((x)-1) & 3)
#define ITE_EC_HIGH_TEMP_LIMIT(x) (0x40 + ((x-1) * 2)) #define ITE_EC_LOW_TEMP_LIMIT(x) (0x41 + ((x-1) * 2)) @@ -119,16 +144,46 @@ #define ITE_EC_BEEP_TONE_DIVISOR(x) (((x) & 0x0f) << 4) #define ITE_EC_BEEP_FREQ_DIVISOR(x) (((x) & 0x0f) << 0)
-#define ITE_EC_FAN_CTL_TEMP_LIMIT_OFF(x) (0x60 + ((x)-1) * 8) -#define ITE_EC_FAN_CTL_TEMP_LIMIT_START(x) (0x61 + ((x)-1) * 8) -#define ITE_EC_FAN_CTL_TEMP_LIMIT_FULL(x) (0x62 + ((x)-1) * 8) -#define ITE_EC_FAN_CTL_PWM_START(x) (0x63 + ((x)-1) * 8) +#define ITE_EC_FAN_CTL_TEMP_LIMIT_OFF(x) \ + (((x) == 5 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0xa0) \ + : (0x60 + ((x)-1) * 8) \ + ) +#define ITE_EC_FAN_CTL_TEMP_LIMIT_START(x) \ + (((x) == 5 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0xa1) \ + : (0x61 + ((x)-1) * 8) \ + ) +#define ITE_EC_FAN_CTL_TEMP_LIMIT_FULL(x) \ + (((x) == 5 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0xa2) \ + : (0x62 + ((x)-1) * 8) \ + ) +#define ITE_EC_FAN_CTL_PWM_START(x) \ + (((x) == 5 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0xa3) \ + : (0x63 + ((x)-1) * 8) \ + ) +#define ITE_EC_FAN_CTL_PWM_AUTO(x) \ + (((x) == 5 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0xa4) \ + : (0x64 + ((x)-1) * 8) \ + ) +#define ITE_EC_FAN_CTL_DELTA_TEMP(x) \ + (((x) == 5 && IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS)) \ + ? (0xa5) \ + : (0x65 + ((x)-1) * 8) \ + ) + +/* Common for ITE_EC_FAN_CTL_PWM_START */ #define ITE_EC_FAN_CTL_PWM_SLOPE_BIT6(s) (((s) & 0x40) << 1) #define ITE_EC_FAN_CTL_PWM_START_DUTY(p) ITE_EC_FAN_CTL_PWM_DUTY(p) -#define ITE_EC_FAN_CTL_PWM_AUTO(x) (0x64 + ((x)-1) * 8) + +/* Common for ITE_EC_FAN_CTL_PWM_AUTO */ #define ITE_EC_FAN_CTL_AUTO_SMOOTHING_EN (1 << 7) #define ITE_EC_FAN_CTL_PWM_SLOPE_LOWER(s) ((s) & 0x3f) -#define ITE_EC_FAN_CTL_DELTA_TEMP(x) (0x65 + ((x)-1) * 8) + +/* Common for ITE_EC_FAN_CTL_DELTA_TEMP */ #define ITE_EC_FAN_CTL_DELTA_TEMP_INTRVL(c) ((c) & 0x1f)
#define ITE_EC_EXTEMP_STATUS 0x88 diff --git a/src/superio/ite/common/env_ctrl_chip.h b/src/superio/ite/common/env_ctrl_chip.h index f8f2e1e..68bf5a0 100644 --- a/src/superio/ite/common/env_ctrl_chip.h +++ b/src/superio/ite/common/env_ctrl_chip.h @@ -3,6 +3,7 @@ * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. * Copyright (C) 2016 secunet Security Networks AG + * Copyright (C) 2019 Protectli * * 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 @@ -19,7 +20,12 @@ #define SUPERIO_ITE_ENV_CTRL_CHIP_H
#define ITE_EC_TMPIN_CNT 3 + +#if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) +#define ITE_EC_FAN_CNT 5 +#else #define ITE_EC_FAN_CNT 3 +#endif
/* Supported thermal mode on TMPINx */ enum ite_ec_thermal_mode { @@ -105,5 +111,7 @@ #define FAN1 ec.fan[0] #define FAN2 ec.fan[1] #define FAN3 ec.fan[2] +#define FAN4 ec.fan[3] +#define FAN5 ec.fan[4]
#endif /* SUPERIO_ITE_ENV_CTRL_CHIP_H */
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
Krystian Hebel (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)