Some ThinkPad systems ECFW use non-standard addresses for fan control
and reporting. This patch adds support for such ECFW so that it can report
the correct fan values.
Tested on Thinkpads L13 Yoga Gen 2 and X13 Yoga Gen 2.
Co-developed-by: Mark Pearson <[email protected]>
Signed-off-by: Vishnu Sankar <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 88 ++++++++++++++++++++++++----
1 file changed, 76 insertions(+), 12 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index d0b5fd4137bc..51ec20e07b23 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -7950,6 +7950,11 @@ static struct ibm_struct volume_driver_data = {
* but the ACPI tables just mention level 7.
*/
+#define FAN_RPM_CAL_CONST 491520 /* FAN RPM calculation offset for some non-standard ECFW */
+
+#define FAN_NS_CTRL_STATUS BIT(2) /* Bit which determines control is enabled or not */
+#define FAN_NS_CTRL BIT(4) /* Bit which determines control is by host or EC */
+
enum { /* Fan control constants */
fan_status_offset = 0x2f, /* EC register 0x2f */
fan_rpm_offset = 0x84, /* EC register 0x84: LSB, 0x85 MSB (RPM)
@@ -7957,6 +7962,11 @@ enum { /* Fan control constants */
fan_select_offset = 0x31, /* EC register 0x31 (Firmware 7M)
bit 0 selects which fan is active */
+ fan_status_offset_ns = 0x93, /* Special status/control offset for non-standard EC Fan1 */
+ fan2_status_offset_ns = 0x96, /* Special status/control offset for non-standard EC Fan2 */
+ fan_rpm_status_ns = 0x95, /* Special offset for Fan1 RPM status for non-standard EC */
+ fan2_rpm_status_ns = 0x98, /* Special offset for Fan2 RPM status for non-standard EC */
+
TP_EC_FAN_FULLSPEED = 0x40, /* EC fan mode: full speed */
TP_EC_FAN_AUTO = 0x80, /* EC fan mode: auto fan control */
@@ -7967,6 +7977,7 @@ enum fan_status_access_mode {
TPACPI_FAN_NONE = 0, /* No fan status or control */
TPACPI_FAN_RD_ACPI_GFAN, /* Use ACPI GFAN */
TPACPI_FAN_RD_TPEC, /* Use ACPI EC regs 0x2f, 0x84-0x85 */
+ TPACPI_FAN_RD_TPEC_NS, /* Use non-standard ACPI EC regs (eg: L13 Yoga gen2 etc.) */
};
enum fan_control_access_mode {
@@ -7994,6 +8005,8 @@ static u8 fan_control_desired_level;
static u8 fan_control_resume_level;
static int fan_watchdog_maxinterval;
+static bool fan_with_ns_addr;
+
static struct mutex fan_mutex;
static void fan_watchdog_fire(struct work_struct *ignored);
@@ -8123,6 +8136,15 @@ static int fan_get_status(u8 *status)
}
break;
+ case TPACPI_FAN_RD_TPEC_NS:
+ /* Default mode is AUTO which means controlled by EC */
+ if (unlikely(!acpi_ec_read(fan_status_offset_ns, &s)))
+ return -EIO;
+
+ if (likely(status))
+ *status = s;
+
+ break;
default:
return -ENXIO;
@@ -8139,7 +8161,8 @@ static int fan_get_status_safe(u8 *status)
if (mutex_lock_killable(&fan_mutex))
return -ERESTARTSYS;
rc = fan_get_status(&s);
- if (!rc)
+ /* NS EC doesn't have register with level settings */
+ if (!rc && !fan_with_ns_addr)
fan_update_desired_level(s);
mutex_unlock(&fan_mutex);
@@ -8166,7 +8189,13 @@ static int fan_get_speed(unsigned int *speed)
if (likely(speed))
*speed = (hi << 8) | lo;
+ break;
+ case TPACPI_FAN_RD_TPEC_NS:
+ if (unlikely(!acpi_ec_read(fan_rpm_status_ns, &lo)))
+ return -EIO;
+ if (likely(speed))
+ *speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
break;
default:
@@ -8178,7 +8207,7 @@ static int fan_get_speed(unsigned int *speed)
static int fan2_get_speed(unsigned int *speed)
{
- u8 hi, lo;
+ u8 hi, lo, status;
bool rc;
switch (fan_status_access_mode) {
@@ -8194,7 +8223,21 @@ static int fan2_get_speed(unsigned int *speed)
if (likely(speed))
*speed = (hi << 8) | lo;
+ break;
+ case TPACPI_FAN_RD_TPEC_NS:
+ rc = !acpi_ec_read(fan2_status_offset_ns, &status);
+ if (rc)
+ return -EIO;
+ if (!(status & FAN_NS_CTRL_STATUS)) {
+ pr_info("fan fan2 control not supported\n");
+ return -EIO;
+ }
+ rc = !acpi_ec_read(fan2_rpm_status_ns, &lo);
+ if (rc)
+ return -EIO;
+ if (likely(speed))
+ *speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
break;
default:
@@ -8697,6 +8740,7 @@ static const struct attribute_group fan_driver_attr_group = {
#define TPACPI_FAN_2FAN 0x0002 /* EC 0x31 bit 0 selects fan2 */
#define TPACPI_FAN_2CTL 0x0004 /* selects fan2 control */
#define TPACPI_FAN_NOFAN 0x0008 /* no fan available */
+#define TPACPI_FAN_NS 0x0010 /* For EC with non-Standard register addresses */
static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
@@ -8715,6 +8759,8 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL), /* P1 / X1 Extreme (2nd gen) */
TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL), /* P15 (1st gen) / P15v (1st gen) */
TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL), /* T15g (2nd gen) */
+ TPACPI_Q_LNV3('R', '1', 'F', TPACPI_FAN_NS), /* L13 Yoga Gen 2 */
+ TPACPI_Q_LNV3('N', '2', 'U', TPACPI_FAN_NS), /* X13 Yoga Gen 2*/
TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN), /* X1 Tablet (2nd gen) */
};
@@ -8749,6 +8795,13 @@ static int __init fan_init(struct ibm_init_struct *iibm)
return -ENODEV;
}
+ if (quirks & TPACPI_FAN_NS) {
+ pr_info("ECFW with non-standard fan reg control found\n");
+ fan_with_ns_addr = 1;
+ /* Fan ctrl support from host is undefined for now */
+ tp_features.fan_ctrl_status_undef = 1;
+ }
+
if (gfan_handle) {
/* 570, 600e/x, 770e, 770x */
fan_status_access_mode = TPACPI_FAN_RD_ACPI_GFAN;
@@ -8756,11 +8809,13 @@ static int __init fan_init(struct ibm_init_struct *iibm)
/* all other ThinkPads: note that even old-style
* ThinkPad ECs supports the fan control register */
if (likely(acpi_ec_read(fan_status_offset,
- &fan_control_initial_status))) {
+ &fan_control_initial_status)) || fan_with_ns_addr) {
int res;
unsigned int speed;
- fan_status_access_mode = TPACPI_FAN_RD_TPEC;
+ fan_status_access_mode = fan_with_ns_addr ?
+ TPACPI_FAN_RD_TPEC_NS : TPACPI_FAN_RD_TPEC;
+
if (quirks & TPACPI_FAN_Q1)
fan_quirk1_setup();
/* Try and probe the 2nd fan */
@@ -8769,7 +8824,8 @@ static int __init fan_init(struct ibm_init_struct *iibm)
if (res >= 0 && speed != FAN_NOT_PRESENT) {
/* It responded - so let's assume it's there */
tp_features.second_fan = 1;
- tp_features.second_fan_ctl = 1;
+ /* fan control not currently available for ns ECFW */
+ tp_features.second_fan_ctl = fan_with_ns_addr ? 0 : 1;
pr_info("secondary fan control detected & enabled\n");
} else {
/* Fan not auto-detected */
@@ -8944,6 +9000,7 @@ static int fan_read(struct seq_file *m)
str_enabled_disabled(status), status);
break;
+ case TPACPI_FAN_RD_TPEC_NS:
case TPACPI_FAN_RD_TPEC:
/* all except 570, 600e/x, 770e, 770x */
rc = fan_get_status_safe(&status);
@@ -8958,13 +9015,20 @@ static int fan_read(struct seq_file *m)
seq_printf(m, "speed:\t\t%d\n", speed);
- if (status & TP_EC_FAN_FULLSPEED)
- /* Disengaged mode takes precedence */
- seq_printf(m, "level:\t\tdisengaged\n");
- else if (status & TP_EC_FAN_AUTO)
- seq_printf(m, "level:\t\tauto\n");
- else
- seq_printf(m, "level:\t\t%d\n", status);
+ if (fan_status_access_mode == TPACPI_FAN_RD_TPEC_NS) {
+ /* No full speed bit in NS EC*/
+ /* EC Auto mode is set by default. No other levels settings available*/
+ (status & FAN_NS_CTRL) ? seq_puts(m, "level:\t\tunknown\n")
+ : seq_puts(m, "level:\t\tauto\n");
+ } else {
+ if (status & TP_EC_FAN_FULLSPEED)
+ /* Disengaged mode takes precedence */
+ seq_puts(m, "level:\t\tdisengaged\n");
+ else if (status & TP_EC_FAN_AUTO)
+ seq_puts(m, "level:\t\tauto\n");
+ else
+ seq_printf(m, "level:\t\t%d\n", status);
+ }
break;
case TPACPI_FAN_NONE:
--
2.34.1
On Thu, 7 Dec 2023, Vishnu Sankar wrote:
Hi Vishnu,
Thanks for the patch.
> Some ThinkPad systems ECFW use non-standard addresses for fan control
> and reporting. This patch adds support for such ECFW so that it can report
> the correct fan values.
> Tested on Thinkpads L13 Yoga Gen 2 and X13 Yoga Gen 2.
>
> Co-developed-by: Mark Pearson <[email protected]>
> Signed-off-by: Vishnu Sankar <[email protected]>
If Mark wrote any lines, his Signed-off-by is also required before yours,
as per Documentation/process/5.Posting.rst, this is a hard requirement.
If he only helped towards the right direction/solution but provided no
code, I recommend using Suggested-by tag instead.
> ---
> drivers/platform/x86/thinkpad_acpi.c | 88 ++++++++++++++++++++++++----
> 1 file changed, 76 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d0b5fd4137bc..51ec20e07b23 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -7950,6 +7950,11 @@ static struct ibm_struct volume_driver_data = {
> * but the ACPI tables just mention level 7.
> */
>
> +#define FAN_RPM_CAL_CONST 491520 /* FAN RPM calculation offset for some non-standard ECFW */
> +
> +#define FAN_NS_CTRL_STATUS BIT(2) /* Bit which determines control is enabled or not */
> +#define FAN_NS_CTRL BIT(4) /* Bit which determines control is by host or EC */
> +
> enum { /* Fan control constants */
> fan_status_offset = 0x2f, /* EC register 0x2f */
> fan_rpm_offset = 0x84, /* EC register 0x84: LSB, 0x85 MSB (RPM)
> @@ -7957,6 +7962,11 @@ enum { /* Fan control constants */
> fan_select_offset = 0x31, /* EC register 0x31 (Firmware 7M)
> bit 0 selects which fan is active */
>
> + fan_status_offset_ns = 0x93, /* Special status/control offset for non-standard EC Fan1 */
> + fan2_status_offset_ns = 0x96, /* Special status/control offset for non-standard EC Fan2 */
> + fan_rpm_status_ns = 0x95, /* Special offset for Fan1 RPM status for non-standard EC */
> + fan2_rpm_status_ns = 0x98, /* Special offset for Fan2 RPM status for non-standard EC */
> +
> TP_EC_FAN_FULLSPEED = 0x40, /* EC fan mode: full speed */
> TP_EC_FAN_AUTO = 0x80, /* EC fan mode: auto fan control */
>
> @@ -7967,6 +7977,7 @@ enum fan_status_access_mode {
> TPACPI_FAN_NONE = 0, /* No fan status or control */
> TPACPI_FAN_RD_ACPI_GFAN, /* Use ACPI GFAN */
> TPACPI_FAN_RD_TPEC, /* Use ACPI EC regs 0x2f, 0x84-0x85 */
> + TPACPI_FAN_RD_TPEC_NS, /* Use non-standard ACPI EC regs (eg: L13 Yoga gen2 etc.) */
> };
>
> enum fan_control_access_mode {
> @@ -7994,6 +8005,8 @@ static u8 fan_control_desired_level;
> static u8 fan_control_resume_level;
> static int fan_watchdog_maxinterval;
>
> +static bool fan_with_ns_addr;
> +
> static struct mutex fan_mutex;
>
> static void fan_watchdog_fire(struct work_struct *ignored);
> @@ -8123,6 +8136,15 @@ static int fan_get_status(u8 *status)
> }
>
> break;
> + case TPACPI_FAN_RD_TPEC_NS:
There's a big comment about FAN ACCESS MODES and now you seem to be adding
another one. Can you please check if there would be something useful to
add/edit in that big comment because of the addition of
TPACPI_FAN_RD_TPEC_NS.
> + /* Default mode is AUTO which means controlled by EC */
> + if (unlikely(!acpi_ec_read(fan_status_offset_ns, &s)))
I'm skeptical that all these unlikely/likely() are useful. Some might even
be harmful if e.g. is some error condition keeps repeating itself and
the particular if handling that is marked with unlikely().
I know the code in that file is littered with them already but it would
be better to add into that, IMO.
> + return -EIO;
> +
> + if (likely(status))
> + *status = s;
> +
> + break;
>
> default:
> return -ENXIO;
> @@ -8139,7 +8161,8 @@ static int fan_get_status_safe(u8 *status)
> if (mutex_lock_killable(&fan_mutex))
> return -ERESTARTSYS;
> rc = fan_get_status(&s);
> - if (!rc)
> + /* NS EC doesn't have register with level settings */
> + if (!rc && !fan_with_ns_addr)
> fan_update_desired_level(s);
> mutex_unlock(&fan_mutex);
>
> @@ -8166,7 +8189,13 @@ static int fan_get_speed(unsigned int *speed)
>
> if (likely(speed))
> *speed = (hi << 8) | lo;
> + break;
> + case TPACPI_FAN_RD_TPEC_NS:
> + if (unlikely(!acpi_ec_read(fan_rpm_status_ns, &lo)))
> + return -EIO;
>
> + if (likely(speed))
> + *speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
> break;
>
> default:
> @@ -8178,7 +8207,7 @@ static int fan_get_speed(unsigned int *speed)
>
> static int fan2_get_speed(unsigned int *speed)
> {
> - u8 hi, lo;
> + u8 hi, lo, status;
> bool rc;
>
> switch (fan_status_access_mode) {
> @@ -8194,7 +8223,21 @@ static int fan2_get_speed(unsigned int *speed)
>
> if (likely(speed))
> *speed = (hi << 8) | lo;
> + break;
>
> + case TPACPI_FAN_RD_TPEC_NS:
> + rc = !acpi_ec_read(fan2_status_offset_ns, &status);
> + if (rc)
> + return -EIO;
> + if (!(status & FAN_NS_CTRL_STATUS)) {
> + pr_info("fan fan2 control not supported\n");
Perhaps "fan2 control ..." would be enough or perhaps "secondary fan
control ..." (the latter matching fan_init() printouts) ?
> + return -EIO;
> + }
> + rc = !acpi_ec_read(fan2_rpm_status_ns, &lo);
> + if (rc)
> + return -EIO;
> + if (likely(speed))
> + *speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
> break;
>
> default:
> @@ -8697,6 +8740,7 @@ static const struct attribute_group fan_driver_attr_group = {
> #define TPACPI_FAN_2FAN 0x0002 /* EC 0x31 bit 0 selects fan2 */
> #define TPACPI_FAN_2CTL 0x0004 /* selects fan2 control */
> #define TPACPI_FAN_NOFAN 0x0008 /* no fan available */
> +#define TPACPI_FAN_NS 0x0010 /* For EC with non-Standard register addresses */
>
> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> @@ -8715,6 +8759,8 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL), /* P1 / X1 Extreme (2nd gen) */
> TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL), /* P15 (1st gen) / P15v (1st gen) */
> TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL), /* T15g (2nd gen) */
> + TPACPI_Q_LNV3('R', '1', 'F', TPACPI_FAN_NS), /* L13 Yoga Gen 2 */
> + TPACPI_Q_LNV3('N', '2', 'U', TPACPI_FAN_NS), /* X13 Yoga Gen 2*/
> TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN), /* X1 Tablet (2nd gen) */
> };
>
> @@ -8749,6 +8795,13 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> return -ENODEV;
> }
>
> + if (quirks & TPACPI_FAN_NS) {
> + pr_info("ECFW with non-standard fan reg control found\n");
> + fan_with_ns_addr = 1;
> + /* Fan ctrl support from host is undefined for now */
> + tp_features.fan_ctrl_status_undef = 1;
> + }
> +
> if (gfan_handle) {
> /* 570, 600e/x, 770e, 770x */
> fan_status_access_mode = TPACPI_FAN_RD_ACPI_GFAN;
> @@ -8756,11 +8809,13 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> /* all other ThinkPads: note that even old-style
> * ThinkPad ECs supports the fan control register */
> if (likely(acpi_ec_read(fan_status_offset,
> - &fan_control_initial_status))) {
> + &fan_control_initial_status)) || fan_with_ns_addr) {
So if we know the addresses are non-standard, why is the acpi_ec_read
performed at all? That is, why isn't the || logic in reverse order?
I also wonder what will fan_control_initial_status be set to in this case,
is it garbage?
> int res;
> unsigned int speed;
>
> - fan_status_access_mode = TPACPI_FAN_RD_TPEC;
> + fan_status_access_mode = fan_with_ns_addr ?
> + TPACPI_FAN_RD_TPEC_NS : TPACPI_FAN_RD_TPEC;
> +
> if (quirks & TPACPI_FAN_Q1)
> fan_quirk1_setup();
> /* Try and probe the 2nd fan */
> @@ -8769,7 +8824,8 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> if (res >= 0 && speed != FAN_NOT_PRESENT) {
> /* It responded - so let's assume it's there */
> tp_features.second_fan = 1;
> - tp_features.second_fan_ctl = 1;
> + /* fan control not currently available for ns ECFW */
> + tp_features.second_fan_ctl = fan_with_ns_addr ? 0 : 1;
= !fan_with_ns_addr;
> pr_info("secondary fan control detected & enabled\n");
> } else {
> /* Fan not auto-detected */
> @@ -8944,6 +9000,7 @@ static int fan_read(struct seq_file *m)
> str_enabled_disabled(status), status);
> break;
>
> + case TPACPI_FAN_RD_TPEC_NS:
> case TPACPI_FAN_RD_TPEC:
> /* all except 570, 600e/x, 770e, 770x */
> rc = fan_get_status_safe(&status);
> @@ -8958,13 +9015,20 @@ static int fan_read(struct seq_file *m)
>
> seq_printf(m, "speed:\t\t%d\n", speed);
>
> - if (status & TP_EC_FAN_FULLSPEED)
> - /* Disengaged mode takes precedence */
> - seq_printf(m, "level:\t\tdisengaged\n");
> - else if (status & TP_EC_FAN_AUTO)
> - seq_printf(m, "level:\t\tauto\n");
> - else
> - seq_printf(m, "level:\t\t%d\n", status);
> + if (fan_status_access_mode == TPACPI_FAN_RD_TPEC_NS) {
> + /* No full speed bit in NS EC*/
Missing space. But I'd convert these two comments into a multiline one
anyway.
> + /* EC Auto mode is set by default. No other levels settings available*/
> + (status & FAN_NS_CTRL) ? seq_puts(m, "level:\t\tunknown\n")
> + : seq_puts(m, "level:\t\tauto\n");
seq_printf(m, "level:\t\t%s\n", status & FAN_NS_CTRL ? "unknown" : "auto");
> + } else {
> + if (status & TP_EC_FAN_FULLSPEED)
> + /* Disengaged mode takes precedence */
> + seq_puts(m, "level:\t\tdisengaged\n");
> + else if (status & TP_EC_FAN_AUTO)
> + seq_puts(m, "level:\t\tauto\n");
Please don't make an unrelated seq_printf() -> seq_puts() change in this
patch.
> + else
> + seq_printf(m, "level:\t\t%d\n", status);
> + }
> break;
>
> case TPACPI_FAN_NONE:
>
--
i.
On Fri, 8 Dec 2023, Ilpo J?rvinen wrote:
> On Thu, 7 Dec 2023, Vishnu Sankar wrote:
>
> Hi Vishnu,
>
> Thanks for the patch.
>
> > Some ThinkPad systems ECFW use non-standard addresses for fan control
> > and reporting. This patch adds support for such ECFW so that it can report
> > the correct fan values.
> > Tested on Thinkpads L13 Yoga Gen 2 and X13 Yoga Gen 2.
> >
> > Co-developed-by: Mark Pearson <[email protected]>
> > Signed-off-by: Vishnu Sankar <[email protected]>
> > + /* Default mode is AUTO which means controlled by EC */
> > + if (unlikely(!acpi_ec_read(fan_status_offset_ns, &s)))
>
> I'm skeptical that all these unlikely/likely() are useful. Some might even
> be harmful if e.g. is some error condition keeps repeating itself and
> the particular if handling that is marked with unlikely().
>
> I know the code in that file is littered with them already but it would
> be better to add into that, IMO.
So I'm missing the negative here, I meant: ... not add into that ...
--
i.
Thanks Ilpo
On Fri, Dec 8, 2023, at 5:53 AM, Ilpo Järvinen wrote:
> On Thu, 7 Dec 2023, Vishnu Sankar wrote:
>
> Hi Vishnu,
>
> Thanks for the patch.
>
>> Some ThinkPad systems ECFW use non-standard addresses for fan control
>> and reporting. This patch adds support for such ECFW so that it can report
>> the correct fan values.
>> Tested on Thinkpads L13 Yoga Gen 2 and X13 Yoga Gen 2.
>>
>> Co-developed-by: Mark Pearson <[email protected]>
>> Signed-off-by: Vishnu Sankar <[email protected]>
>
> If Mark wrote any lines, his Signed-off-by is also required before yours,
> as per Documentation/process/5.Posting.rst, this is a hard requirement.
>
> If he only helped towards the right direction/solution but provided no
> code, I recommend using Suggested-by tag instead.
>
Ah - I didn't know that. Thanks for the clarification.
I should probably be a Suggested-by here. I've been helping Vishnu with the process but he's been doing all the hard work and writing of code.
Mark
Hi Ilpo,
Thank you for the review
On Fri, Dec 8, 2023 at 7:53 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Thu, 7 Dec 2023, Vishnu Sankar wrote:
>
> Hi Vishnu,
>
> Thanks for the patch.
>
> > Some ThinkPad systems ECFW use non-standard addresses for fan control
> > and reporting. This patch adds support for such ECFW so that it can report
> > the correct fan values.
> > Tested on Thinkpads L13 Yoga Gen 2 and X13 Yoga Gen 2.
> >
> > Co-developed-by: Mark Pearson <[email protected]>
> > Signed-off-by: Vishnu Sankar <[email protected]>
>
> If Mark wrote any lines, his Signed-off-by is also required before yours,
> as per Documentation/process/5.Posting.rst, this is a hard requirement.
>
> If he only helped towards the right direction/solution but provided no
> code, I recommend using Suggested-by tag instead.
>
Ack.
Will change to Suggested-by.
> > ---
> > drivers/platform/x86/thinkpad_acpi.c | 88 ++++++++++++++++++++++++----
> > 1 file changed, 76 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > index d0b5fd4137bc..51ec20e07b23 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -7950,6 +7950,11 @@ static struct ibm_struct volume_driver_data = {
> > * but the ACPI tables just mention level 7.
> > */
> >
> > +#define FAN_RPM_CAL_CONST 491520 /* FAN RPM calculation offset for some non-standard ECFW */
> > +
> > +#define FAN_NS_CTRL_STATUS BIT(2) /* Bit which determines control is enabled or not */
> > +#define FAN_NS_CTRL BIT(4) /* Bit which determines control is by host or EC */
> > +
> > enum { /* Fan control constants */
> > fan_status_offset = 0x2f, /* EC register 0x2f */
> > fan_rpm_offset = 0x84, /* EC register 0x84: LSB, 0x85 MSB (RPM)
> > @@ -7957,6 +7962,11 @@ enum { /* Fan control constants */
> > fan_select_offset = 0x31, /* EC register 0x31 (Firmware 7M)
> > bit 0 selects which fan is active */
> >
> > + fan_status_offset_ns = 0x93, /* Special status/control offset for non-standard EC Fan1 */
> > + fan2_status_offset_ns = 0x96, /* Special status/control offset for non-standard EC Fan2 */
> > + fan_rpm_status_ns = 0x95, /* Special offset for Fan1 RPM status for non-standard EC */
> > + fan2_rpm_status_ns = 0x98, /* Special offset for Fan2 RPM status for non-standard EC */
> > +
> > TP_EC_FAN_FULLSPEED = 0x40, /* EC fan mode: full speed */
> > TP_EC_FAN_AUTO = 0x80, /* EC fan mode: auto fan control */
> >
> > @@ -7967,6 +7977,7 @@ enum fan_status_access_mode {
> > TPACPI_FAN_NONE = 0, /* No fan status or control */
> > TPACPI_FAN_RD_ACPI_GFAN, /* Use ACPI GFAN */
> > TPACPI_FAN_RD_TPEC, /* Use ACPI EC regs 0x2f, 0x84-0x85 */
> > + TPACPI_FAN_RD_TPEC_NS, /* Use non-standard ACPI EC regs (eg: L13 Yoga gen2 etc.) */
> > };
> >
> > enum fan_control_access_mode {
> > @@ -7994,6 +8005,8 @@ static u8 fan_control_desired_level;
> > static u8 fan_control_resume_level;
> > static int fan_watchdog_maxinterval;
> >
> > +static bool fan_with_ns_addr;
> > +
> > static struct mutex fan_mutex;
> >
> > static void fan_watchdog_fire(struct work_struct *ignored);
> > @@ -8123,6 +8136,15 @@ static int fan_get_status(u8 *status)
> > }
> >
> > break;
> > + case TPACPI_FAN_RD_TPEC_NS:
>
> There's a big comment about FAN ACCESS MODES and now you seem to be adding
> another one. Can you please check if there would be something useful to
> add/edit in that big comment because of the addition of
> TPACPI_FAN_RD_TPEC_NS.
I will look into adding this.
>
> > + /* Default mode is AUTO which means controlled by EC */
> > + if (unlikely(!acpi_ec_read(fan_status_offset_ns, &s)))
>
> I'm skeptical that all these unlikely/likely() are useful. Some might even
> be harmful if e.g. is some error condition keeps repeating itself and
> the particular if handling that is marked with unlikely().
>
> I know the code in that file is littered with them already but it would
> be better to add into that, IMO.
I will look into this as well (removing likely/unlikely).
>
> > + return -EIO;
> > +
> > + if (likely(status))
> > + *status = s;
> > +
> > + break;
> >
> > default:
> > return -ENXIO;
> > @@ -8139,7 +8161,8 @@ static int fan_get_status_safe(u8 *status)
> > if (mutex_lock_killable(&fan_mutex))
> > return -ERESTARTSYS;
> > rc = fan_get_status(&s);
> > - if (!rc)
> > + /* NS EC doesn't have register with level settings */
> > + if (!rc && !fan_with_ns_addr)
> > fan_update_desired_level(s);
> > mutex_unlock(&fan_mutex);
> >
> > @@ -8166,7 +8189,13 @@ static int fan_get_speed(unsigned int *speed)
> >
> > if (likely(speed))
> > *speed = (hi << 8) | lo;
> > + break;
> > + case TPACPI_FAN_RD_TPEC_NS:
> > + if (unlikely(!acpi_ec_read(fan_rpm_status_ns, &lo)))
> > + return -EIO;
> >
> > + if (likely(speed))
> > + *speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
> > break;
> >
> > default:
> > @@ -8178,7 +8207,7 @@ static int fan_get_speed(unsigned int *speed)
> >
> > static int fan2_get_speed(unsigned int *speed)
> > {
> > - u8 hi, lo;
> > + u8 hi, lo, status;
> > bool rc;
> >
> > switch (fan_status_access_mode) {
> > @@ -8194,7 +8223,21 @@ static int fan2_get_speed(unsigned int *speed)
> >
> > if (likely(speed))
> > *speed = (hi << 8) | lo;
> > + break;
> >
> > + case TPACPI_FAN_RD_TPEC_NS:
> > + rc = !acpi_ec_read(fan2_status_offset_ns, &status);
> > + if (rc)
> > + return -EIO;
> > + if (!(status & FAN_NS_CTRL_STATUS)) {
> > + pr_info("fan fan2 control not supported\n");
>
> Perhaps "fan2 control ..." would be enough or perhaps "secondary fan
> control ..." (the latter matching fan_init() printouts) ?
Acked
>
> > + return -EIO;
> > + }
> > + rc = !acpi_ec_read(fan2_rpm_status_ns, &lo);
> > + if (rc)
> > + return -EIO;
> > + if (likely(speed))
> > + *speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
> > break;
> >
> > default:
> > @@ -8697,6 +8740,7 @@ static const struct attribute_group fan_driver_attr_group = {
> > #define TPACPI_FAN_2FAN 0x0002 /* EC 0x31 bit 0 selects fan2 */
> > #define TPACPI_FAN_2CTL 0x0004 /* selects fan2 control */
> > #define TPACPI_FAN_NOFAN 0x0008 /* no fan available */
> > +#define TPACPI_FAN_NS 0x0010 /* For EC with non-Standard register addresses */
> >
> > static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> > TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> > @@ -8715,6 +8759,8 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> > TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL), /* P1 / X1 Extreme (2nd gen) */
> > TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL), /* P15 (1st gen) / P15v (1st gen) */
> > TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL), /* T15g (2nd gen) */
> > + TPACPI_Q_LNV3('R', '1', 'F', TPACPI_FAN_NS), /* L13 Yoga Gen 2 */
> > + TPACPI_Q_LNV3('N', '2', 'U', TPACPI_FAN_NS), /* X13 Yoga Gen 2*/
> > TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN), /* X1 Tablet (2nd gen) */
> > };
> >
> > @@ -8749,6 +8795,13 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> > return -ENODEV;
> > }
> >
> > + if (quirks & TPACPI_FAN_NS) {
> > + pr_info("ECFW with non-standard fan reg control found\n");
> > + fan_with_ns_addr = 1;
> > + /* Fan ctrl support from host is undefined for now */
> > + tp_features.fan_ctrl_status_undef = 1;
> > + }
> > +
> > if (gfan_handle) {
> > /* 570, 600e/x, 770e, 770x */
> > fan_status_access_mode = TPACPI_FAN_RD_ACPI_GFAN;
> > @@ -8756,11 +8809,13 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> > /* all other ThinkPads: note that even old-style
> > * ThinkPad ECs supports the fan control register */
> > if (likely(acpi_ec_read(fan_status_offset,
> > - &fan_control_initial_status))) {
> > + &fan_control_initial_status)) || fan_with_ns_addr) {
>
> So if we know the addresses are non-standard, why is the acpi_ec_read
> performed at all? That is, why isn't the || logic in reverse order?
>
Agreed.
I will change the logic to reverse order.
> I also wonder what will fan_control_initial_status be set to in this case,
> is it garbage?
0x84 is still a valid register (but not related to Fan)
>
> > int res;
> > unsigned int speed;
> >
> > - fan_status_access_mode = TPACPI_FAN_RD_TPEC;
> > + fan_status_access_mode = fan_with_ns_addr ?
> > + TPACPI_FAN_RD_TPEC_NS : TPACPI_FAN_RD_TPEC;
> > +
> > if (quirks & TPACPI_FAN_Q1)
> > fan_quirk1_setup();
> > /* Try and probe the 2nd fan */
> > @@ -8769,7 +8824,8 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> > if (res >= 0 && speed != FAN_NOT_PRESENT) {
> > /* It responded - so let's assume it's there */
> > tp_features.second_fan = 1;
> > - tp_features.second_fan_ctl = 1;
> > + /* fan control not currently available for ns ECFW */
> > + tp_features.second_fan_ctl = fan_with_ns_addr ? 0 : 1;
>
> = !fan_with_ns_addr;
Agreed.
>
> > pr_info("secondary fan control detected & enabled\n");
> > } else {
> > /* Fan not auto-detected */
> > @@ -8944,6 +9000,7 @@ static int fan_read(struct seq_file *m)
> > str_enabled_disabled(status), status);
> > break;
> >
> > + case TPACPI_FAN_RD_TPEC_NS:
> > case TPACPI_FAN_RD_TPEC:
> > /* all except 570, 600e/x, 770e, 770x */
> > rc = fan_get_status_safe(&status);
> > @@ -8958,13 +9015,20 @@ static int fan_read(struct seq_file *m)
> >
> > seq_printf(m, "speed:\t\t%d\n", speed);
> >
> > - if (status & TP_EC_FAN_FULLSPEED)
> > - /* Disengaged mode takes precedence */
> > - seq_printf(m, "level:\t\tdisengaged\n");
> > - else if (status & TP_EC_FAN_AUTO)
> > - seq_printf(m, "level:\t\tauto\n");
> > - else
> > - seq_printf(m, "level:\t\t%d\n", status);
> > + if (fan_status_access_mode == TPACPI_FAN_RD_TPEC_NS) {
> > + /* No full speed bit in NS EC*/
>
> Missing space. But I'd convert these two comments into a multiline one
> anyway.
>
Acked
> > + /* EC Auto mode is set by default. No other levels settings available*/
> > + (status & FAN_NS_CTRL) ? seq_puts(m, "level:\t\tunknown\n")
> > + : seq_puts(m, "level:\t\tauto\n");
>
> seq_printf(m, "level:\t\t%s\n", status & FAN_NS_CTRL ? "unknown" : "auto");
>
Acked.
> > + } else {
> > + if (status & TP_EC_FAN_FULLSPEED)
> > + /* Disengaged mode takes precedence */
> > + seq_puts(m, "level:\t\tdisengaged\n");
> > + else if (status & TP_EC_FAN_AUTO)
> > + seq_puts(m, "level:\t\tauto\n");
>
> Please don't make an unrelated seq_printf() -> seq_puts() change in this
> patch.
Ack.
(The checkpatch.pl threw a warning regarding seq_printf() for these
lines after the changes I made.)
>
> > + else
> > + seq_printf(m, "level:\t\t%d\n", status);
> > + }
> > break;
> >
> > case TPACPI_FAN_NONE:
> >
>
> --
> i.
>
Thank you for correcting these and Apologize for any inconvenience caused.
I have noted the comments. Will share the updated patch soon.
--
Regards,
Vishnu Sankar
+817015150407 (Japan)