| Message ID | 20210407212015.298222-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: Correct thermal sensor allocation | expand |
Hi, On 4/7/21 11:20 PM, Mark Pearson wrote: > On recent Thinkpad platforms it was reported that temp sensor 11 was > always incorrectly displaying 66C. It turns out the reason for this is > that this location in EC RAM is not a temperature sensor but is the > power supply ID (offset 0xC2). > > Based on feedback from the Lenovo firmware team the EC RAM version can > be determined and for the current version (3) only the 0x78 to 0x7F > range is used for temp sensors. I don't have any details for earlier > versions so I have left the implementation unaltered there. > > Note - in this block only 0x78 and 0x79 are officially designated (CPU & > GPU sensors). The use of the other locations in the block will vary from > platform to platform; but the existing logic to detect a sensor presence > holds. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> I've merged this, note I've added one small fixup to initialize ver to 0 when it is declared, also see a remark inline below. Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. > --- > drivers/platform/x86/thinkpad_acpi.c | 31 ++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 07de21941..8fbe4d3d9 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -6260,6 +6260,7 @@ enum thermal_access_mode { > enum { /* TPACPI_THERMAL_TPEC_* */ > TP_EC_THERMAL_TMP0 = 0x78, /* ACPI EC regs TMP 0..7 */ > TP_EC_THERMAL_TMP8 = 0xC0, /* ACPI EC regs TMP 8..15 */ > + TP_EC_FUNCREV = 0xEF, /* ACPI EC Functional revision */ > TP_EC_THERMAL_TMP_NA = -128, /* ACPI EC sensor not available */ > > TPACPI_THERMAL_SENSOR_NA = -128000, /* Sensor not available */ > @@ -6458,7 +6459,7 @@ static const struct attribute_group thermal_temp_input8_group = { > > static int __init thermal_init(struct ibm_init_struct *iibm) > { > - u8 t, ta1, ta2; > + u8 t, ta1, ta2, ver; > int i; > int acpi_tmp7; > int res; > @@ -6473,7 +6474,14 @@ static int __init thermal_init(struct ibm_init_struct *iibm) > * 0x78-0x7F, 0xC0-0xC7. Registers return 0x00 for > * non-implemented, thermal sensors return 0x80 when > * not available > + * The above rule is unfortunately flawed. This has been seen with > + * 0xC2 (power supply ID) causing thermal control problems. > + * The EC version can be determined by offset 0xEF and at least for > + * version 3 the Lenovo firmware team confirmed that registers 0xC0-0xC7 > + * are not thermal registers. > */ > + if (!acpi_ec_read(TP_EC_FUNCREV, &ver)) > + pr_warn("Thinkpad ACPI EC unable to access EC version\n"); So what happens with ver in case the pr_warn here triggers? As said above I've added a small change to initialize ver to 0 when declared, so that we keep the old behavior when this pr_warn triggers. > > ta1 = ta2 = 0; > for (i = 0; i < 8; i++) { > @@ -6483,11 +6491,13 @@ static int __init thermal_init(struct ibm_init_struct *iibm) > ta1 = 0; > break; > } > - if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) { > - ta2 |= t; > - } else { > - ta1 = 0; > - break; > + if (ver < 3) { > + if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) { > + ta2 |= t; > + } else { > + ta1 = 0; > + break; > + } > } > } > if (ta1 == 0) { > @@ -6500,9 +6510,12 @@ static int __init thermal_init(struct ibm_init_struct *iibm) > thermal_read_mode = TPACPI_THERMAL_NONE; > } > } else { > - thermal_read_mode = > - (ta2 != 0) ? > - TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8; > + if (ver >= 3) > + thermal_read_mode = TPACPI_THERMAL_TPEC_8; > + else > + thermal_read_mode = > + (ta2 != 0) ? > + TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8; > } > } else if (acpi_tmp7) { > if (tpacpi_is_ibm() && > Regards, Hans
Thanks Hans On 13/04/2021 02:28, Hans de Goede wrote: > Hi, > > On 4/7/21 11:20 PM, Mark Pearson wrote: >> On recent Thinkpad platforms it was reported that temp sensor 11 was >> always incorrectly displaying 66C. It turns out the reason for this is >> that this location in EC RAM is not a temperature sensor but is the >> power supply ID (offset 0xC2). >> >> Based on feedback from the Lenovo firmware team the EC RAM version can >> be determined and for the current version (3) only the 0x78 to 0x7F >> range is used for temp sensors. I don't have any details for earlier >> versions so I have left the implementation unaltered there. >> >> Note - in this block only 0x78 and 0x79 are officially designated (CPU & >> GPU sensors). The use of the other locations in the block will vary from >> platform to platform; but the existing logic to detect a sensor presence >> holds. >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> > > I've merged this, note I've added one small fixup to initialize ver to > 0 when it is declared, also see a remark inline below. > Agreed on setting ver to zero - thanks for adding that Mark
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 07de21941..8fbe4d3d9 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -6260,6 +6260,7 @@ enum thermal_access_mode { enum { /* TPACPI_THERMAL_TPEC_* */ TP_EC_THERMAL_TMP0 = 0x78, /* ACPI EC regs TMP 0..7 */ TP_EC_THERMAL_TMP8 = 0xC0, /* ACPI EC regs TMP 8..15 */ + TP_EC_FUNCREV = 0xEF, /* ACPI EC Functional revision */ TP_EC_THERMAL_TMP_NA = -128, /* ACPI EC sensor not available */ TPACPI_THERMAL_SENSOR_NA = -128000, /* Sensor not available */ @@ -6458,7 +6459,7 @@ static const struct attribute_group thermal_temp_input8_group = { static int __init thermal_init(struct ibm_init_struct *iibm) { - u8 t, ta1, ta2; + u8 t, ta1, ta2, ver; int i; int acpi_tmp7; int res; @@ -6473,7 +6474,14 @@ static int __init thermal_init(struct ibm_init_struct *iibm) * 0x78-0x7F, 0xC0-0xC7. Registers return 0x00 for * non-implemented, thermal sensors return 0x80 when * not available + * The above rule is unfortunately flawed. This has been seen with + * 0xC2 (power supply ID) causing thermal control problems. + * The EC version can be determined by offset 0xEF and at least for + * version 3 the Lenovo firmware team confirmed that registers 0xC0-0xC7 + * are not thermal registers. */ + if (!acpi_ec_read(TP_EC_FUNCREV, &ver)) + pr_warn("Thinkpad ACPI EC unable to access EC version\n"); ta1 = ta2 = 0; for (i = 0; i < 8; i++) { @@ -6483,11 +6491,13 @@ static int __init thermal_init(struct ibm_init_struct *iibm) ta1 = 0; break; } - if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) { - ta2 |= t; - } else { - ta1 = 0; - break; + if (ver < 3) { + if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) { + ta2 |= t; + } else { + ta1 = 0; + break; + } } } if (ta1 == 0) { @@ -6500,9 +6510,12 @@ static int __init thermal_init(struct ibm_init_struct *iibm) thermal_read_mode = TPACPI_THERMAL_NONE; } } else { - thermal_read_mode = - (ta2 != 0) ? - TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8; + if (ver >= 3) + thermal_read_mode = TPACPI_THERMAL_TPEC_8; + else + thermal_read_mode = + (ta2 != 0) ? + TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8; } } else if (acpi_tmp7) { if (tpacpi_is_ibm() &&
On recent Thinkpad platforms it was reported that temp sensor 11 was always incorrectly displaying 66C. It turns out the reason for this is that this location in EC RAM is not a temperature sensor but is the power supply ID (offset 0xC2). Based on feedback from the Lenovo firmware team the EC RAM version can be determined and for the current version (3) only the 0x78 to 0x7F range is used for temp sensors. I don't have any details for earlier versions so I have left the implementation unaltered there. Note - in this block only 0x78 and 0x79 are officially designated (CPU & GPU sensors). The use of the other locations in the block will vary from platform to platform; but the existing logic to detect a sensor presence holds. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- drivers/platform/x86/thinkpad_acpi.c | 31 ++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 9 deletions(-)