| Message ID | 20230102180112.1954082-1-daniel.lezcano@kernel.org |
|---|---|
| Headers | show |
| Series | Thermal ACPI APIs for generic trip points | expand |
On Mon, Jan 2, 2023 at 7:01 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > The ACPI specification describes the trip points, the device tree > bindings as well. > > The OF code uses the generic trip point structures. > > The ACPI has their own trip points structure and uses the get_trip_* > ops to retrieve them. > > We can do the same as the OF code and create a set of ACPI functions > to retrieve a trip point description. Having a common code for ACPI > will help to cleanup the remaining Intel drivers and get rid of the > get_trip_* functions. > > These changes add the ACPI thermal calls to retrieve the basic > information we need to be reused in the thermal ACPI and Intel > drivers. > > The different ACPI functions have the generic trip point structure > passed as parameter where it is filled. > > This structure aims to be the one used by all the thermal drivers and > the thermal framework. > > After this series, a couple of Intel drivers and the ACPI thermal > driver will still have their own trip points definition but a new > series on top of this one will finish the conversion to the generic > trip point handling. > > This series depends on the generic trip point added to the thermal > framework and available in the thermal/linux-next branch. > > https://lkml.org/lkml/2022/10/3/456 > > It has been tested on a Intel i7-8650U - x280 with the INT3400, the > PCH, ACPITZ, and x86_pkg_temp. No regression observed so far. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@kernel.org> > --- > drivers/thermal/Kconfig | 13 ++ > drivers/thermal/Makefile | 1 + > drivers/thermal/thermal_acpi.c | 279 +++++++++++++++++++++++++++++++++ > include/linux/thermal.h | 16 ++ > 4 files changed, 309 insertions(+) > create mode 100644 drivers/thermal/thermal_acpi.c > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index e052dae614eb..2c19bccd1223 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -76,6 +76,19 @@ config THERMAL_OF > Say 'Y' here if you need to build thermal infrastructure > based on device tree. > > +config THERMAL_ACPI Not needed. Or if you want it to be built only if there are any users, call it ACPI_THERMAL_LIB and do config ACPI_THERMAL_LIB depends on ACPI_THERMAL bool and let the users select it. > + bool > + prompt "APIs to parse thermal data out of the ACPI tables" > + depends on ACPI_THERMAL > + default y > + help > + This options provides helpers to add the support to > + read and parse thermal data definitions out of the > + ACPI tables blob. > + > + Say 'Y' here if you need to build thermal infrastructure > + based on ACPI. > + > config THERMAL_WRITABLE_TRIPS > bool "Enable writable trip points" > help > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile > index 2506c6c8ca83..60f0dfa9aae2 100644 > --- a/drivers/thermal/Makefile > +++ b/drivers/thermal/Makefile > @@ -13,6 +13,7 @@ thermal_sys-$(CONFIG_THERMAL_NETLINK) += thermal_netlink.o > # interface to/from other layers providing sensors > thermal_sys-$(CONFIG_THERMAL_HWMON) += thermal_hwmon.o > thermal_sys-$(CONFIG_THERMAL_OF) += thermal_of.o > +thermal_sys-$(CONFIG_THERMAL_ACPI) += thermal_acpi.o > > # governors > thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE) += gov_fair_share.o > diff --git a/drivers/thermal/thermal_acpi.c b/drivers/thermal/thermal_acpi.c > new file mode 100644 > index 000000000000..28c629b4d814 > --- /dev/null > +++ b/drivers/thermal/thermal_acpi.c > @@ -0,0 +1,279 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2022 Linaro Limited > + * > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> > + * > + * ACPI thermal configuration > + */ > +#include <linux/acpi.h> > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/units.h> > +#include <uapi/linux/thermal.h> > + > +#include "thermal_core.h" > + > +int thermal_acpi_trip_gtsh(struct acpi_device *adev) > +{ > + unsigned long long hyst; > + acpi_status status; > + > + status = acpi_evaluate_integer(adev->handle, "GTSH", NULL, &hyst); > + if (ACPI_FAILURE(status)) > + return 0; > + > + return (int)(hyst * 100); What if the result is larger than INT_MAX? > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_gtsh); > + > +int thermal_acpi_get_tzd(struct acpi_device *adev, struct acpi_handle_list *devices) > +{ > + acpi_status status; > + > + /* > + * _TZD (Thermal zone device): This optional object evaluates > + * to a package of device names. Each name corresponds to a > + * device in the ACPI namespace that is associated with the > + * thermal zone. The temperature reported by the thermal zone > + * is roughly correspondent to that of each of the devices. > + */ I don't think that the comment is necessary. The spec already contains a definition of this object. > + status = acpi_evaluate_reference(adev->handle, "_TZD", NULL, devices); > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_get_tzd); > + > +int thermal_acpi_get_temp(struct acpi_device *adev, int *temperature) > +{ > + unsigned long long temp; > + acpi_status status; > + > + /* > + * _TMP (Temperature): This control method returns the thermal zone’s > + * current operating temperature. The return value is the current > + * temperature of the thermal zone in tenths of degrees Kelvin > + */ Like above. > + status = acpi_evaluate_integer(adev->handle, "_TMP", NULL, &temp); > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + *temperature = deci_kelvin_to_millicelsius(temp); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_get_temp); > + > +int thermal_acpi_trip_crit(struct acpi_device *adev, struct thermal_trip *trip) > +{ > + unsigned long long temp; > + acpi_status status; > + > + /* > + * _CRT (Critical temperature): This object, when defined under a thermal > + * zone, returns the critical temperature at which OSPM must shutdown > + * the system. If this object it present under a device, the device’s > + * driver evaluates this object to determine the device’s critical cooling > + * temperature trip point. This value may then be used by the device’s > + * driver to program an internal device temperature sensor trip point > + */ Same here. > + status = acpi_evaluate_integer(adev->handle, "_CRT", NULL, &temp); > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + trip->hysteresis = thermal_acpi_trip_gtsh(adev); > + trip->temperature = deci_kelvin_to_millicelsius(temp); > + trip->type = THERMAL_TRIP_CRITICAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_crit); > + > +int thermal_acpi_trip_hot(struct acpi_device *adev, struct thermal_trip *trip) > +{ > + unsigned long long temp; > + acpi_status status; > + > + /* > + * _HOT (Hot Temperature): This optional object, when defined under a > + * thermal zone, returns the critical temperature at which OSPM may > + * choose to transition the system into the S4 sleeping state. The > + * platform vendor should define _HOT to be far enough below _CRT so as > + * to allow OSPM enough time to transition the system into the S4 > + * sleeping state. While dependent on the amount of installed memory, > + * on typical platforms OSPM implementations can transition the system > + * into the S4 sleeping state in tens of seconds. If this object it > + * present under a device, the device’s driver evaluates this object to > + * determine the device’s hot cooling temperature trip point. This value > + * may then be used by the device’s driver to program an internal device > + * temperature sensor trip point. > + */ Same here. > + status = acpi_evaluate_integer(adev->handle, "_HOT", NULL, &temp); > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + trip->hysteresis = thermal_acpi_trip_gtsh(adev); > + trip->temperature = deci_kelvin_to_millicelsius(temp); > + trip->type = THERMAL_TRIP_HOT; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_hot); > + > +int thermal_acpi_trip_psv_psl(struct acpi_device *adev, struct acpi_handle_list *devices) > +{ > + acpi_status status; > + > + /* > + * _PSL (Passive List): This object is defined under a thermal zone and > + * evaluates to a list of processor objects to be used for passive cooling > + */ Same here. > + status = acpi_evaluate_reference(adev->handle, "_PSL", NULL, devices); > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_psv_psl); > + > +int thermal_acpi_trip_psv_tsp(struct acpi_device *adev) > +{ > + acpi_status status; > + unsigned long long tsp; > + > + /* > + * _TSP (Thermal Sampling Period): This object evaluates to a thermal > + * sampling period (in tenths of seconds) used by OSPM to implement the > + * Passive cooling equation. This value, along with _TC1 and _TC2, will > + * enable OSPM to provide the proper hysteresis required by the system > + * to accomplish an effective passive cooling policy. > + */ Same here. > + status = acpi_evaluate_integer(adev->handle, "_TSP", NULL, &tsp); > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + return (int)tsp; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_psv_tsp); > + > +int thermal_acpi_trip_psv_tc1(struct acpi_device *adev) > +{ > + acpi_status status; > + unsigned long long tc1; > + > + /* > + * _TC1 (Thermal Constant 1): This object evaluates to the constant _TC1 > + * for use in the Passive cooling formula > + */ Same here. > + status = acpi_evaluate_integer(adev->handle, "_TC1", NULL, &tc1); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + return (int)tc1; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_psv_tc1); > + > +int thermal_acpi_trip_psv_tc2(struct acpi_device *adev) > +{ > + acpi_status status; > + unsigned long long tc2; > + > + /* > + * _TC2 (Thermal Constant 1): This object evaluates to the constant _TC2 > + * for use in the Passive cooling formula > + */ Same here. > + status = acpi_evaluate_integer(adev->handle, "_TC2", NULL, &tc2); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + return (int)tc2; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_psv_tc2); > + > +int thermal_acpi_trip_psv(struct acpi_device *adev, struct thermal_trip *trip) > +{ > + unsigned long long temp; > + acpi_status status; > + > + /* > + * _PSV (Passive): This optional object, if present under a thermal zone, > + * evaluates to the temperature at which OSPM must activate passive > + * cooling policy > + */ Same here. > + status = acpi_evaluate_integer(adev->handle, "_PSV", NULL, &temp); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + /* > + * The _PSL, _TSP, _TC1 and _TC2 are required if the _PSV object exists. > + * We assume the caller will raise an error if it was able to get the _PSV > + * but then fail to get the other objects. > + */ > + trip->hysteresis = thermal_acpi_trip_gtsh(adev); > + trip->temperature = deci_kelvin_to_millicelsius(temp); > + trip->type = THERMAL_TRIP_PASSIVE; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_psv); > + > +int thermal_acpi_trip_acl(struct acpi_device *adev, > + struct acpi_handle_list *devices, int id) > +{ > + acpi_status status; > + char name[5]; > + > + /* > + * _ALx: This object is defined under a thermal zone and evaluates to a > + * list of Active cooling devices to be turned on when the corresponding > + * _ACx temperature threshold is exceeded. For example, these devices > + * could be fans. > + */ Same here. > + sprintf(name, "_AL%d", id); > + > + status = acpi_evaluate_reference(adev->handle, name, NULL, devices); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_acl); > + > +int thermal_acpi_trip_act(struct acpi_device *adev, > + struct thermal_trip *trip, int id) > +{ > + acpi_status status; > + unsigned long long temp; > + char name[5]; > + > + /* > + * _ACx: This optional object, if present under a thermal zone, returns > + * the temperature trip point at which OSPM must start or stop active > + * cooling, where x is a value between 0 and 9 that designates multiple > + * active cooling levels of the thermal zone. If the Active cooling > + * device has one cooling level (that is, “on”) then that cooling level > + * must be defined as _AC0. If the cooling device has two levels of > + * capability, such as a high fan speed and a low fan speed, then they > + * must be defined as _AC0 and _AC1 respectively. The smaller the value > + * of x, the greater the cooling strength _ACx represents. In the above > + * example, _AC0 represents the greater level of cooling (the faster fan > + * speed) and _AC1 represents the lesser level of cooling (the slower > + * fan speed). For every _ACx method, there must be a matching _ALx > + * object or a corresponding entry in an _ART object’s active cooling > + * relationship list. > + */ Same here. > + sprintf(name, "_AC%d", id); > + > + status = acpi_evaluate_integer(adev->handle, name, NULL, &temp); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + trip->hysteresis = thermal_acpi_trip_gtsh(adev); > + trip->temperature = deci_kelvin_to_millicelsius(temp); > + trip->type = THERMAL_TRIP_ACTIVE; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(thermal_acpi_trip_act); Overall, I'm not sure about simple wrappers around acpi_evaluate_integer/reference() that effectively discard the return value and don't even bother to sanitize the return value before returning it to the caller. The ones that initialize a trip point make more sense IMO. > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index 30353e4b1424..65c1f1aafe02 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -334,6 +334,22 @@ static inline void devm_thermal_of_zone_unregister(struct device *dev, > } > #endif > > +#ifdef CONFIG_THERMAL_ACPI > +struct acpi_handle_list; > +int thermal_acpi_get_temp(struct acpi_device *adev, int *temperature); > +int thermal_acpi_trip_crit(struct acpi_device *adev, struct thermal_trip *trip); > +int thermal_acpi_trip_hot(struct acpi_device *adev, struct thermal_trip *trip); > +int thermal_acpi_trip_psv(struct acpi_device *adev, struct thermal_trip *trip); > +int thermal_acpi_trip_act(struct acpi_device *adev, struct thermal_trip *trip, int id); > +int thermal_acpi_trip_acl(struct acpi_device *adev, struct acpi_handle_list *devices, int id); > +int thermal_acpi_trip_psv_psl(struct acpi_device *adev, struct acpi_handle_list *devices); > +int thermal_acpi_trip_psv_tsp(struct acpi_device *adev); > +int thermal_acpi_trip_psv_tc1(struct acpi_device *adev); > +int thermal_acpi_trip_psv_tc2(struct acpi_device *adev); > +int thermal_acpi_trip_gtsh(struct acpi_device *adev); > +int thermal_acpi_get_tzd(struct acpi_device *adev, struct acpi_handle_list *devices); > +#endif > + > int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id, > struct thermal_trip *trip); > int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id, > -- > 2.34.1 >
On 02/01/2023 19:22, Rafael J. Wysocki wrote: > On Mon, Jan 2, 2023 at 7:01 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> From: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> The ACPI specification describes the trip points, the device tree >> bindings as well. >> >> The OF code uses the generic trip point structures. >> >> The ACPI has their own trip points structure and uses the get_trip_* >> ops to retrieve them. >> >> We can do the same as the OF code and create a set of ACPI functions >> to retrieve a trip point description. Having a common code for ACPI >> will help to cleanup the remaining Intel drivers and get rid of the >> get_trip_* functions. >> >> These changes add the ACPI thermal calls to retrieve the basic >> information we need to be reused in the thermal ACPI and Intel >> drivers. >> >> The different ACPI functions have the generic trip point structure >> passed as parameter where it is filled. >> >> This structure aims to be the one used by all the thermal drivers and >> the thermal framework. >> >> After this series, a couple of Intel drivers and the ACPI thermal >> driver will still have their own trip points definition but a new >> series on top of this one will finish the conversion to the generic >> trip point handling. >> >> This series depends on the generic trip point added to the thermal >> framework and available in the thermal/linux-next branch. >> >> https://lkml.org/lkml/2022/10/3/456 >> >> It has been tested on a Intel i7-8650U - x280 with the INT3400, the >> PCH, ACPITZ, and x86_pkg_temp. No regression observed so far. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@kernel.org> >> --- >> drivers/thermal/Kconfig | 13 ++ >> drivers/thermal/Makefile | 1 + >> drivers/thermal/thermal_acpi.c | 279 +++++++++++++++++++++++++++++++++ >> include/linux/thermal.h | 16 ++ >> 4 files changed, 309 insertions(+) >> create mode 100644 drivers/thermal/thermal_acpi.c >> >> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig >> index e052dae614eb..2c19bccd1223 100644 >> --- a/drivers/thermal/Kconfig >> +++ b/drivers/thermal/Kconfig >> @@ -76,6 +76,19 @@ config THERMAL_OF >> Say 'Y' here if you need to build thermal infrastructure >> based on device tree. >> >> +config THERMAL_ACPI > > Not needed. > > Or if you want it to be built only if there are any users, call it > ACPI_THERMAL_LIB and do > > config ACPI_THERMAL_LIB > depends on ACPI_THERMAL > bool > > and let the users select it. Yes, I think it makes more sense to not provide any option and just compile the wrappers when ACPI_THERMAL is set. >> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile >> index 2506c6c8ca83..60f0dfa9aae2 100644 >> --- a/drivers/thermal/Makefile >> +++ b/drivers/thermal/Makefile >> @@ -13,6 +13,7 @@ thermal_sys-$(CONFIG_THERMAL_NETLINK) += thermal_netlink.o >> # interface to/from other layers providing sensors >> thermal_sys-$(CONFIG_THERMAL_HWMON) += thermal_hwmon.o >> thermal_sys-$(CONFIG_THERMAL_OF) += thermal_of.o >> +thermal_sys-$(CONFIG_THERMAL_ACPI) += thermal_acpi.o >> >> # governors >> thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE) += gov_fair_share.o >> diff --git a/drivers/thermal/thermal_acpi.c b/drivers/thermal/thermal_acpi.c >> new file mode 100644 >> index 000000000000..28c629b4d814 >> --- /dev/null >> +++ b/drivers/thermal/thermal_acpi.c >> @@ -0,0 +1,279 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright 2022 Linaro Limited >> + * >> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org> >> + * >> + * ACPI thermal configuration >> + */ >> +#include <linux/acpi.h> >> +#include <linux/module.h> >> +#include <linux/kernel.h> >> +#include <linux/units.h> >> +#include <uapi/linux/thermal.h> >> + >> +#include "thermal_core.h" >> + >> +int thermal_acpi_trip_gtsh(struct acpi_device *adev) >> +{ >> + unsigned long long hyst; >> + acpi_status status; >> + >> + status = acpi_evaluate_integer(adev->handle, "GTSH", NULL, &hyst); >> + if (ACPI_FAILURE(status)) >> + return 0; >> + >> + return (int)(hyst * 100); > > What if the result is larger than INT_MAX? That would mean ACPI is returning more than 4 billions degree hysteresis value. What strategy should we use in these functions? Trust the values returned by ACPI or double check if they are consistent ? >> +} >> +EXPORT_SYMBOL_GPL(thermal_acpi_trip_gtsh); >> + >> +int thermal_acpi_get_tzd(struct acpi_device *adev, struct acpi_handle_list *devices) >> +{ >> + acpi_status status; >> + >> + /* >> + * _TZD (Thermal zone device): This optional object evaluates >> + * to a package of device names. Each name corresponds to a >> + * device in the ACPI namespace that is associated with the >> + * thermal zone. The temperature reported by the thermal zone >> + * is roughly correspondent to that of each of the devices. >> + */ > > I don't think that the comment is necessary. > > The spec already contains a definition of this object. Yes, this comment is the description from the spec. I put them there to save time for those who are reading the code so they don't have to go back and forth between the documentation and the code. Do you really want me to remove all of them ? [ ... ] > Overall, I'm not sure about simple wrappers around > acpi_evaluate_integer/reference() that effectively discard the return > value and don't even bother to sanitize the return value before > returning it to the caller. Ok, so we don't want to trust the values returned by ACPI. If all the sanity checks are done in the functions, would it make more sense then ? > The ones that initialize a trip point make more sense IMO.
Recently sent as a RFC, the thermal ACPI for generic trip points is a set of functions to fill the generic trip points structure which will become the standard structure for the thermal framework and its users. Different Intel drivers and the ACPI thermal driver are using the ACPI tables to get the thermal zone information. As those are getting the same information, providing this set of ACPI function with the generic trip points will consolidate the code. Also, the Intel PCH and the Intel 34xx drivers are converted to use the generic trip points relying on the ACPI generic trip point parsing functions. These changes have been tested on a Thinkpad Lenovo x280 with the PCH and INT34xx drivers. No regression have been observed, the trip points remain the same for what is described on this system. Changelog: - V2: - Fix the thermal ACPI patch where the thermal_acpi.c was not included in the series - Provide a couple of users of this API which could have been tested on a real system Daniel Lezcano (3): thermal/acpi: Add ACPI trip point routines thermal/drivers/intel: Use generic trip points for intel_pch thermal/drivers/intel: Use generic trip points int340x drivers/thermal/Kconfig | 13 + drivers/thermal/Makefile | 1 + .../int340x_thermal/int340x_thermal_zone.c | 175 +++-------- .../int340x_thermal/int340x_thermal_zone.h | 10 +- drivers/thermal/intel/intel_pch_thermal.c | 88 ++---- drivers/thermal/thermal_acpi.c | 279 ++++++++++++++++++ include/linux/thermal.h | 16 + 7 files changed, 368 insertions(+), 214 deletions(-) create mode 100644 drivers/thermal/thermal_acpi.c