Message ID | 20231031154930.4908-1-dober6023@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2] hwmon:Add MEC172x Micro Chip driver for Lenovo motherboards | expand |
On 10/31/23 08:49, David Ober wrote: > This addition adds in the ability for the system to scan the > MEC172x EC chip in Lenovo ThinkStation systems to get the > current fan RPM speeds and the Maximum speed value for each > fan also provides the current CPU and DIMM thermal status > > Signed-off-by: David Ober <dober6023@gmail.com> > > v2 fixed mixcased naming > v2 add mutex protection > v2 removed references to ACPI as it is not used > v2 added comment to explain why returning a -1 is needed > --- > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/lenovo-ec-sensors.c | 484 ++++++++++++++++++++++++++++++ > 3 files changed, 495 insertions(+) > create mode 100644 drivers/hwmon/lenovo-ec-sensors.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index ec38c8892158..821741ec0d2f 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -862,6 +862,16 @@ config SENSORS_LAN966X > This driver can also be built as a module. If so, the module > will be called lan966x-hwmon. > > +config SENSORS_LENOVO_EC > + tristate "Microchip MEC172X Chip for Lenovo ThinkStation" > + depends on I2C > + help > + If you say yes here you get support for LENOVO > + EC Sensors on newer ThinkStation systems > + > + This driver can also be built as a module. If so, the module > + will be called lenovo_ec_sensors. > + > config SENSORS_LINEAGE > tristate "Lineage Compact Power Line Power Entry Module" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 4ac9452b5430..aa3c2dc390ec 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -104,6 +104,7 @@ obj-$(CONFIG_SENSORS_JC42) += jc42.o > obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o > obj-$(CONFIG_SENSORS_K10TEMP) += k10temp.o > obj-$(CONFIG_SENSORS_LAN966X) += lan966x-hwmon.o > +obj-$(CONFIG_SENSORS_LENOVO_EC) += lenovo-ec-sensors.o > obj-$(CONFIG_SENSORS_LINEAGE) += lineage-pem.o > obj-$(CONFIG_SENSORS_LOCHNAGAR) += lochnagar-hwmon.o > obj-$(CONFIG_SENSORS_LM63) += lm63.o > diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c > new file mode 100644 > index 000000000000..e2b14b3aea08 > --- /dev/null > +++ b/drivers/hwmon/lenovo-ec-sensors.c > @@ -0,0 +1,484 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * HWMON driver for MEC172x chip that publishes some sensor values > + * via the embedded controller registers specific to Lenovo Systems. > + * > + * Copyright (C) 2023 David Ober (Lenovo) <dober@lenovo.com> > + * > + * EC provides: > + * - CPU temperature > + * - DIMM temperature > + * - Chassis zone temperatures > + * - CPU fan RPM > + * - DIMM fan RPM > + * - Chassis fans RPM > + */ > + > +#include <linux/acpi.h> > +#include <linux/delay.h> > +#include <linux/hwmon.h> > +#include <linux/ioport.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/units.h> > + > +#define MCHP_SING_IDX 0x0000 > +#define MCHP_EMI0_APPLICATION_ID 0x090C > +#define MCHP_EMI0_EC_ADDRESS_LSB 0x0902 > +#define MCHP_EMI0_EC_ADDRESS_MSB 0x0903 > +#define MCHP_EMI0_EC_DATA_BYTE0 0x0904 > +#define MCHP_EMI0_EC_DATA_BYTE1 0x0905 > +#define MCHP_EMI0_EC_DATA_BYTE2 0x0906 > +#define MCHP_EMI0_EC_DATA_BYTE3 0x0907 > + > +#define io_write8(a, b) outb_p(b, a) > +#define io_read8(a) inb_p(a) > + > +static inline uint8_t > +get_ec_reg(unsigned char page, unsigned char index) > +{ > + u8 onebyte; > + unsigned short m_index; > + unsigned short phy_index = page * 256 + index; > + > + if (io_read8(MCHP_EMI0_APPLICATION_ID) != 0) /* EMI access locked */ > + return -1; > + Whatever the explanation is, returning -1 from a function declared as returning uint8_t doesn't make sense. Not checking the return code in the calling function makes even less sense. > + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); > + > + m_index = phy_index & 0x7FFC; > + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, m_index); > + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, m_index >> 8); > + > + switch (phy_index & 0x0003) { > + case 0: > + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE0); > + break; > + case 1: > + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE1); > + break; > + case 2: > + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE2); > + break; > + case 3: > + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE3); > + break; > + } > + > + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); /* write same data to clean */ > + return onebyte; > +} > + > +static const char * const systems[] = { > + "Tomcat", > + "Hornet", > + "Falcon", > + "Manta_", > +}; > + > +static const char * const lenovo_px_ec_temp_label[] = { > + "CPU1", > + "CPU2", > + "R_DIMM1", > + "L_DIMM1", > + "R_DIMM2", > + "L_DIMM2", > + "PCH", > + "M2_R", > + "M2_Z1R", > + "M2_Z2R", > + "PCI_Z1", > + "PCI_Z2", > + "PCI_Z3", > + "PCI_Z4", > + "AMB", > +}; > + > +static const char * const lenovo_gen_ec_temp_label[] = { > + "CPU1", > + "", > + "R_DIMM", > + "L_DIMM", > + "", > + "", > + "PCH", > + "M2_R", > + "M2_Z1R", > + "M2_Z2R", > + "PCI_Z1", > + "PCI_Z2", > + "PCI_Z3", > + "PCI_Z4", > + "AMB", > +}; > + > +static const char * const px_ec_fan_label[] = { > + "CPU1_Fan", > + "CPU2_Fan", > + "Front_Fan1-1", > + "Front_Fan1-2", > + "Front_Fan2", > + "Front_Fan3", > + "MEM_Fan1", > + "MEM_Fan2", > + "Rear_Fan1", > + "Rear_Fan2", > + "Flex_Bay_Fan1", > + "Flex_Bay_Fan2", > + "Flex_Bay_Fan2", > + "PSU_HDD_Fan", > + "PSU1_Fan", > + "PSU2_Fan", > +}; > + > +static const char * const p7_ec_fan_label[] = { > + "CPU1_Fan", > + "", > + "HP_CPU_Fan1", > + "HP_CPU_Fan2", > + "PCIE1_4_Fan", > + "PCIE5_7_Fan", > + "MEM_Fan1", > + "MEM_Fan2", > + "Rear_Fan1", > + "", > + "BCB_Fan", > + "Flex_Bay_Fan", > + "", > + "", > + "PSU_Fan", > + "", > +}; > + > +static const char * const p5_ec_fan_label[] = { > + "CPU_Fan", > + "", > + "", > + "", > + "", > + "HDD_Fan", > + "Duct_Fan1", > + "MEM_Fan", > + "Rear_Fan", > + "", > + "Front_Fan", > + "Flex_Bay_Fan", > + "", > + "", > + "PSU_Fan", > + "", > +}; > + > +static const char * const p7_amd_ec_fan_label[] = { > + "CPU1_Fan", > + "CPU2_Fan", > + "HP_CPU_Fan1", > + "HP_CPU_Fan2", > + "PCIE1_4_Fan", > + "PCIE5_7_Fan", > + "DIMM1_Fan1", > + "DIMM1_Fan2", > + "DIMM2_Fan1", > + "DIMM2_Fan2", > + "Rear_Fan", > + "HDD_Bay_Fan", > + "Flex_Bay_Fan", > + "", > + "PSU_Fan", > + "", > +}; > + > +struct ec_sensors_data { > + struct mutex mec_mutex; /* lock for sensors write */ > + u8 platform_id; > + const char *const *fan_labels; > + const char *const *temp_labels; > +}; > + > +static int > +lenovo_ec_do_read_temp(struct ec_sensors_data *data, u32 attr, int channel, long *val) > +{ > + u8 LSB; > + > + switch (attr) { > + case hwmon_temp_input: > + mutex_lock(&data->mec_mutex); > + LSB = get_ec_reg(2, 0x81 + channel); > + mutex_unlock(&data->mec_mutex); > + if (LSB > 0x40) { > + *val = (LSB - 0x40) * 1000; > + } else { > + *val = 0; > + return -1; No. Return valid Linux error codes. Also, error handling should come first, and writing 0 into *val when returning an error is useless. Also, get_ec_reg() returns 0xff if "EMI access locked", which is treated as valid here (and elsewhere). This doesn't make sense to me. > + } > + return 0; > + default: > + break; > + } > + return -EOPNOTSUPP; > +} > + > +static int > +lenovo_ec_do_read_fan(struct ec_sensors_data *data, u32 attr, int channel, long *val) > +{ > + u8 LSB, MSB; > + > + channel *= 2; > + switch (attr) { > + case hwmon_fan_input: > + mutex_lock(&data->mec_mutex); > + LSB = get_ec_reg(4, 0x60 + channel); > + MSB = get_ec_reg(4, 0x61 + channel); > + mutex_unlock(&data->mec_mutex); > + if ((MSB << 8) + LSB != 0) { This is confusing. Why not just "if (MSB || LSB)" ? > + mutex_lock(&data->mec_mutex); > + LSB = get_ec_reg(4, 0x20 + channel); > + MSB = get_ec_reg(4, 0x21 + channel); > + mutex_unlock(&data->mec_mutex); > + *val = (MSB << 8) + LSB; > + return 0; > + } > + return -1; /* Returning -1 here has the sensors tool mark the FAN speed as N/A */ -1 translates to -EPERM. No, this is unacceptable. Please return valid Linux error codes. If you _really_ think that this should return -EPERM, explain in detail (and use -EPERM). FWIW, there is -ENODATA which is displayed in the "sensors" command as N/A. Maybe that is what you want. > + case hwmon_fan_max: > + mutex_lock(&data->mec_mutex); > + LSB = get_ec_reg(4, 0x60 + channel); > + MSB = get_ec_reg(4, 0x61 + channel); > + mutex_unlock(&data->mec_mutex); > + if ((MSB << 8) + LSB != 0) { > + mutex_lock(&data->mec_mutex); > + LSB = get_ec_reg(4, 0x40 + channel); > + MSB = get_ec_reg(4, 0x41 + channel); > + mutex_unlock(&data->mec_mutex); > + *val = (MSB << 8) + LSB; > + } else { > + *val = 0; > + } > + return 0; > + case hwmon_fan_min: > + case hwmon_fan_div: > + case hwmon_fan_alarm: > + break; > + default: > + break; > + } > + return -EOPNOTSUPP; > +} > + > +static int get_platform(struct ec_sensors_data *data) > +{ > + char system_type[6]; > + int ret = -1; > + int idx; > + > + for (idx = 0 ; idx < 6 ; idx++) > + mutex_lock(&data->mec_mutex); > + system_type[idx] = get_ec_reg(0xC, (0x10 + idx)); Please avoid unnecessary ( ). > + mutex_unlock(&data->mec_mutex); > + > + for (idx = 0 ; idx < 4 ; idx++) { > + if (strcmp(systems[idx], system_type) == 0) { Use "!strcmp()". The length of system_type[] is 6, all fields are written, and the data is not initialized. There is no guarantee that the value at string[6] is '\0', and either case strcmp() will read past the end of the array. > + ret = idx; > + break; > + } > + } > + return ret; > +} > + > +static int > +lenovo_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, const char **str) > +{ > + struct ec_sensors_data *state = dev_get_drvdata(dev); > + > + switch (type) { > + case hwmon_temp: > + *str = state->temp_labels[channel]; > + break; > + > + case hwmon_fan: > + *str = state->fan_labels[channel]; Some of those strings are empty. Does that mean the fans don't exist ? If so, don't instantiate them. If it is simply unknown what the fans do, don't instantiate the attribute either. Returning an empty string as "label" is not acceptable. > + break; > + default: > + return -EOPNOTSUPP; /* unreachable */ Kind of inconsistent comment. Please drop. > + } > + return 0; > +} > + > +static int > +lenovo_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct ec_sensors_data *data = dev_get_drvdata(dev); > + > + switch (type) { > + case hwmon_temp: > + return lenovo_ec_do_read_temp(data, attr, channel, val); > + case hwmon_fan: > + return lenovo_ec_do_read_fan(data, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static umode_t > +lenovo_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_temp: > + if (attr == hwmon_temp_input || attr == hwmon_temp_label) > + return 0444; > + break; > + case hwmon_fan: > + if (attr == hwmon_fan_input || attr == hwmon_fan_max || attr == hwmon_fan_label) > + return 0444; > + break; > + default: > + return 0; > + } > + return 0; > +} > + > +static const struct hwmon_channel_info *lenovo_ec_hwmon_info[] = { > + HWMON_CHANNEL_INFO(temp, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL), > + HWMON_CHANNEL_INFO(fan, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX), > + NULL > +}; > + > +static const struct hwmon_ops lenovo_ec_hwmon_ops = { > + .is_visible = lenovo_ec_hwmon_is_visible, > + .read = lenovo_ec_hwmon_read, > + .read_string = lenovo_ec_hwmon_read_string, > +}; > + > +static struct hwmon_chip_info lenovo_ec_chip_info = { > + .ops = &lenovo_ec_hwmon_ops, > + .info = lenovo_ec_hwmon_info, > +}; > + > +static int lenovo_ec_probe(struct platform_device *pdev) > +{ > + struct device *hwdev; > + struct ec_sensors_data *ec_data; > + const struct hwmon_chip_info *chip_info; > + struct device *dev = &pdev->dev; > + > + ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data), GFP_KERNEL); > + if (!ec_data) > + return -ENOMEM; > + > + dev_set_drvdata(dev, ec_data); > + > + chip_info = &lenovo_ec_chip_info; > + > + mutex_lock(&ec_data->mec_mutex); mutex_init() missing. > + if (io_read8(MCHP_EMI0_APPLICATION_ID) != 0) { /* check EMI Application BIT */ > + io_write8(0x90C, io_read8(0x90C)); /* set EMI Application BIT to 0 */ > + } > + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, MCHP_SING_IDX); > + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, MCHP_SING_IDX >> 8); This is really unacceptable. It pretty much immediately writes into i/o registers. I am quite sure this would crash or have undesirable impact on lots of non-Lenovo systems. Also, the driver should reserve the register i/o range it uses, not just use it blindly. > + mutex_unlock(&ec_data->mec_mutex); > + > + if ((io_read8(MCHP_EMI0_EC_DATA_BYTE0) == 'M') && > + (io_read8(MCHP_EMI0_EC_DATA_BYTE1) == 'C') && > + (io_read8(MCHP_EMI0_EC_DATA_BYTE2) == 'H') && > + (io_read8(MCHP_EMI0_EC_DATA_BYTE3) == 'P')) { > + ec_data->platform_id = get_platform(ec_data); > + switch (ec_data->platform_id) { > + case 0: > + ec_data->fan_labels = px_ec_fan_label; > + ec_data->temp_labels = lenovo_px_ec_temp_label; > + break; > + case 1: > + ec_data->fan_labels = p7_ec_fan_label; > + ec_data->temp_labels = lenovo_gen_ec_temp_label; > + break; > + case 2: > + ec_data->fan_labels = p5_ec_fan_label; > + ec_data->temp_labels = lenovo_gen_ec_temp_label; > + break; > + case 3: > + ec_data->fan_labels = p7_amd_ec_fan_label; > + ec_data->temp_labels = lenovo_gen_ec_temp_label; There is only a single set of temperature labels. ec_data->temp_labels is therefore unnecessary and lenovo_gen_ec_temp_label can be used directly. > + break; > + default: > + dev_err(dev, "Unknown ThinkStation Model"); Maybe better "Unsupported" > + return -EINVAL; -ENODEV; > + } > + > + hwdev = devm_hwmon_device_register_with_info(dev, "lenovo_ec", > + ec_data, > + chip_info, NULL); > + > + return PTR_ERR_OR_ZERO(hwdev); > + } > + return -ENODEV; Error handling should come first. > +} > + > +static struct platform_driver lenovo_ec_sensors_platform_driver = { > + .driver = { > + .name = "lenovo-ec-sensors", > + }, > + .probe = lenovo_ec_probe, > +}; > + > +static struct platform_device *lenovo_ec_sensors_platform_device; > + > +static int __init lenovo_ec_init(void) > +{ > + lenovo_ec_sensors_platform_device = > + platform_create_bundle(&lenovo_ec_sensors_platform_driver, > + lenovo_ec_probe, NULL, 0, NULL, 0); > + > + if (IS_ERR(lenovo_ec_sensors_platform_device)) > + return PTR_ERR(lenovo_ec_sensors_platform_device); > + > + return 0; > +} Isn't there a means to determine if this is a supported Lenovo system prior to calling the probe function ? DMI data, maybe ? > + > +static void __exit lenovo_ec_exit(void) > +{ > + platform_device_unregister(lenovo_ec_sensors_platform_device); > + platform_driver_unregister(&lenovo_ec_sensors_platform_driver); > +} > + > +module_init(lenovo_ec_init); > +module_exit(lenovo_ec_exit); > + > +MODULE_AUTHOR("David Ober <dober@lenovo.com>"); > +MODULE_DESCRIPTION("HWMON driver for MEC172x EC sensors on LENOVO motherboards"); > +MODULE_LICENSE("GPL");
Hi David, kernel test robot noticed the following build warnings: [auto build test WARNING on groeck-staging/hwmon-next] [also build test WARNING on linus/master v6.6 next-20231101] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/David-Ober/hwmon-Add-MEC172x-Micro-Chip-driver-for-Lenovo-motherboards/20231031-235345 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20231031154930.4908-1-dober6023%40gmail.com patch subject: [PATCH v2] hwmon:Add MEC172x Micro Chip driver for Lenovo motherboards config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20231101/202311011858.M1Ui9wp1-lkp@intel.com/config) compiler: sh4-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231101/202311011858.M1Ui9wp1-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311011858.M1Ui9wp1-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/hwmon/lenovo-ec-sensors.c: In function 'get_ec_reg': drivers/hwmon/lenovo-ec-sensors.c:36:25: error: implicit declaration of function 'inb_p'; did you mean 'int_pow'? [-Werror=implicit-function-declaration] 36 | #define io_read8(a) inb_p(a) | ^~~~~ drivers/hwmon/lenovo-ec-sensors.c:45:13: note: in expansion of macro 'io_read8' 45 | if (io_read8(MCHP_EMI0_APPLICATION_ID) != 0) /* EMI access locked */ | ^~~~~~~~ drivers/hwmon/lenovo-ec-sensors.c:35:25: error: implicit declaration of function 'outb_p' [-Werror=implicit-function-declaration] 35 | #define io_write8(a, b) outb_p(b, a) | ^~~~~~ drivers/hwmon/lenovo-ec-sensors.c:48:9: note: in expansion of macro 'io_write8' 48 | io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); | ^~~~~~~~~ drivers/hwmon/lenovo-ec-sensors.c: In function 'get_platform': >> drivers/hwmon/lenovo-ec-sensors.c:274:9: warning: this 'for' clause does not guard... [-Wmisleading-indentation] 274 | for (idx = 0 ; idx < 6 ; idx++) | ^~~ drivers/hwmon/lenovo-ec-sensors.c:276:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for' 276 | system_type[idx] = get_ec_reg(0xC, (0x10 + idx)); | ^~~~~~~~~~~ In function 'get_platform', inlined from 'lenovo_ec_probe' at drivers/hwmon/lenovo-ec-sensors.c:420:26: >> drivers/hwmon/lenovo-ec-sensors.c:276:34: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] 276 | system_type[idx] = get_ec_reg(0xC, (0x10 + idx)); | ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/hwmon/lenovo-ec-sensors.c: In function 'lenovo_ec_probe': drivers/hwmon/lenovo-ec-sensors.c:270:14: note: at offset 6 into destination object 'system_type' of size 6 270 | char system_type[6]; | ^~~~~~~~~~~ cc1: some warnings being treated as errors vim +/for +274 drivers/hwmon/lenovo-ec-sensors.c 267 268 static int get_platform(struct ec_sensors_data *data) 269 { 270 char system_type[6]; 271 int ret = -1; 272 int idx; 273 > 274 for (idx = 0 ; idx < 6 ; idx++) 275 mutex_lock(&data->mec_mutex); > 276 system_type[idx] = get_ec_reg(0xC, (0x10 + idx)); 277 mutex_unlock(&data->mec_mutex); 278 279 for (idx = 0 ; idx < 4 ; idx++) { 280 if (strcmp(systems[idx], system_type) == 0) { 281 ret = idx; 282 break; 283 } 284 } 285 return ret; 286 } 287
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index ec38c8892158..821741ec0d2f 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -862,6 +862,16 @@ config SENSORS_LAN966X This driver can also be built as a module. If so, the module will be called lan966x-hwmon. +config SENSORS_LENOVO_EC + tristate "Microchip MEC172X Chip for Lenovo ThinkStation" + depends on I2C + help + If you say yes here you get support for LENOVO + EC Sensors on newer ThinkStation systems + + This driver can also be built as a module. If so, the module + will be called lenovo_ec_sensors. + config SENSORS_LINEAGE tristate "Lineage Compact Power Line Power Entry Module" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 4ac9452b5430..aa3c2dc390ec 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -104,6 +104,7 @@ obj-$(CONFIG_SENSORS_JC42) += jc42.o obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o obj-$(CONFIG_SENSORS_K10TEMP) += k10temp.o obj-$(CONFIG_SENSORS_LAN966X) += lan966x-hwmon.o +obj-$(CONFIG_SENSORS_LENOVO_EC) += lenovo-ec-sensors.o obj-$(CONFIG_SENSORS_LINEAGE) += lineage-pem.o obj-$(CONFIG_SENSORS_LOCHNAGAR) += lochnagar-hwmon.o obj-$(CONFIG_SENSORS_LM63) += lm63.o diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c new file mode 100644 index 000000000000..e2b14b3aea08 --- /dev/null +++ b/drivers/hwmon/lenovo-ec-sensors.c @@ -0,0 +1,484 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * HWMON driver for MEC172x chip that publishes some sensor values + * via the embedded controller registers specific to Lenovo Systems. + * + * Copyright (C) 2023 David Ober (Lenovo) <dober@lenovo.com> + * + * EC provides: + * - CPU temperature + * - DIMM temperature + * - Chassis zone temperatures + * - CPU fan RPM + * - DIMM fan RPM + * - Chassis fans RPM + */ + +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/hwmon.h> +#include <linux/ioport.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/units.h> + +#define MCHP_SING_IDX 0x0000 +#define MCHP_EMI0_APPLICATION_ID 0x090C +#define MCHP_EMI0_EC_ADDRESS_LSB 0x0902 +#define MCHP_EMI0_EC_ADDRESS_MSB 0x0903 +#define MCHP_EMI0_EC_DATA_BYTE0 0x0904 +#define MCHP_EMI0_EC_DATA_BYTE1 0x0905 +#define MCHP_EMI0_EC_DATA_BYTE2 0x0906 +#define MCHP_EMI0_EC_DATA_BYTE3 0x0907 + +#define io_write8(a, b) outb_p(b, a) +#define io_read8(a) inb_p(a) + +static inline uint8_t +get_ec_reg(unsigned char page, unsigned char index) +{ + u8 onebyte; + unsigned short m_index; + unsigned short phy_index = page * 256 + index; + + if (io_read8(MCHP_EMI0_APPLICATION_ID) != 0) /* EMI access locked */ + return -1; + + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); + + m_index = phy_index & 0x7FFC; + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, m_index); + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, m_index >> 8); + + switch (phy_index & 0x0003) { + case 0: + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE0); + break; + case 1: + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE1); + break; + case 2: + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE2); + break; + case 3: + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE3); + break; + } + + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); /* write same data to clean */ + return onebyte; +} + +static const char * const systems[] = { + "Tomcat", + "Hornet", + "Falcon", + "Manta_", +}; + +static const char * const lenovo_px_ec_temp_label[] = { + "CPU1", + "CPU2", + "R_DIMM1", + "L_DIMM1", + "R_DIMM2", + "L_DIMM2", + "PCH", + "M2_R", + "M2_Z1R", + "M2_Z2R", + "PCI_Z1", + "PCI_Z2", + "PCI_Z3", + "PCI_Z4", + "AMB", +}; + +static const char * const lenovo_gen_ec_temp_label[] = { + "CPU1", + "", + "R_DIMM", + "L_DIMM", + "", + "", + "PCH", + "M2_R", + "M2_Z1R", + "M2_Z2R", + "PCI_Z1", + "PCI_Z2", + "PCI_Z3", + "PCI_Z4", + "AMB", +}; + +static const char * const px_ec_fan_label[] = { + "CPU1_Fan", + "CPU2_Fan", + "Front_Fan1-1", + "Front_Fan1-2", + "Front_Fan2", + "Front_Fan3", + "MEM_Fan1", + "MEM_Fan2", + "Rear_Fan1", + "Rear_Fan2", + "Flex_Bay_Fan1", + "Flex_Bay_Fan2", + "Flex_Bay_Fan2", + "PSU_HDD_Fan", + "PSU1_Fan", + "PSU2_Fan", +}; + +static const char * const p7_ec_fan_label[] = { + "CPU1_Fan", + "", + "HP_CPU_Fan1", + "HP_CPU_Fan2", + "PCIE1_4_Fan", + "PCIE5_7_Fan", + "MEM_Fan1", + "MEM_Fan2", + "Rear_Fan1", + "", + "BCB_Fan", + "Flex_Bay_Fan", + "", + "", + "PSU_Fan", + "", +}; + +static const char * const p5_ec_fan_label[] = { + "CPU_Fan", + "", + "", + "", + "", + "HDD_Fan", + "Duct_Fan1", + "MEM_Fan", + "Rear_Fan", + "", + "Front_Fan", + "Flex_Bay_Fan", + "", + "", + "PSU_Fan", + "", +}; + +static const char * const p7_amd_ec_fan_label[] = { + "CPU1_Fan", + "CPU2_Fan", + "HP_CPU_Fan1", + "HP_CPU_Fan2", + "PCIE1_4_Fan", + "PCIE5_7_Fan", + "DIMM1_Fan1", + "DIMM1_Fan2", + "DIMM2_Fan1", + "DIMM2_Fan2", + "Rear_Fan", + "HDD_Bay_Fan", + "Flex_Bay_Fan", + "", + "PSU_Fan", + "", +}; + +struct ec_sensors_data { + struct mutex mec_mutex; /* lock for sensors write */ + u8 platform_id; + const char *const *fan_labels; + const char *const *temp_labels; +}; + +static int +lenovo_ec_do_read_temp(struct ec_sensors_data *data, u32 attr, int channel, long *val) +{ + u8 LSB; + + switch (attr) { + case hwmon_temp_input: + mutex_lock(&data->mec_mutex); + LSB = get_ec_reg(2, 0x81 + channel); + mutex_unlock(&data->mec_mutex); + if (LSB > 0x40) { + *val = (LSB - 0x40) * 1000; + } else { + *val = 0; + return -1; + } + return 0; + default: + break; + } + return -EOPNOTSUPP; +} + +static int +lenovo_ec_do_read_fan(struct ec_sensors_data *data, u32 attr, int channel, long *val) +{ + u8 LSB, MSB; + + channel *= 2; + switch (attr) { + case hwmon_fan_input: + mutex_lock(&data->mec_mutex); + LSB = get_ec_reg(4, 0x60 + channel); + MSB = get_ec_reg(4, 0x61 + channel); + mutex_unlock(&data->mec_mutex); + if ((MSB << 8) + LSB != 0) { + mutex_lock(&data->mec_mutex); + LSB = get_ec_reg(4, 0x20 + channel); + MSB = get_ec_reg(4, 0x21 + channel); + mutex_unlock(&data->mec_mutex); + *val = (MSB << 8) + LSB; + return 0; + } + return -1; /* Returning -1 here has the sensors tool mark the FAN speed as N/A */ + case hwmon_fan_max: + mutex_lock(&data->mec_mutex); + LSB = get_ec_reg(4, 0x60 + channel); + MSB = get_ec_reg(4, 0x61 + channel); + mutex_unlock(&data->mec_mutex); + if ((MSB << 8) + LSB != 0) { + mutex_lock(&data->mec_mutex); + LSB = get_ec_reg(4, 0x40 + channel); + MSB = get_ec_reg(4, 0x41 + channel); + mutex_unlock(&data->mec_mutex); + *val = (MSB << 8) + LSB; + } else { + *val = 0; + } + return 0; + case hwmon_fan_min: + case hwmon_fan_div: + case hwmon_fan_alarm: + break; + default: + break; + } + return -EOPNOTSUPP; +} + +static int get_platform(struct ec_sensors_data *data) +{ + char system_type[6]; + int ret = -1; + int idx; + + for (idx = 0 ; idx < 6 ; idx++) + mutex_lock(&data->mec_mutex); + system_type[idx] = get_ec_reg(0xC, (0x10 + idx)); + mutex_unlock(&data->mec_mutex); + + for (idx = 0 ; idx < 4 ; idx++) { + if (strcmp(systems[idx], system_type) == 0) { + ret = idx; + break; + } + } + return ret; +} + +static int +lenovo_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + struct ec_sensors_data *state = dev_get_drvdata(dev); + + switch (type) { + case hwmon_temp: + *str = state->temp_labels[channel]; + break; + + case hwmon_fan: + *str = state->fan_labels[channel]; + break; + default: + return -EOPNOTSUPP; /* unreachable */ + } + return 0; +} + +static int +lenovo_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct ec_sensors_data *data = dev_get_drvdata(dev); + + switch (type) { + case hwmon_temp: + return lenovo_ec_do_read_temp(data, attr, channel, val); + case hwmon_fan: + return lenovo_ec_do_read_fan(data, attr, channel, val); + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static umode_t +lenovo_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_temp: + if (attr == hwmon_temp_input || attr == hwmon_temp_label) + return 0444; + break; + case hwmon_fan: + if (attr == hwmon_fan_input || attr == hwmon_fan_max || attr == hwmon_fan_label) + return 0444; + break; + default: + return 0; + } + return 0; +} + +static const struct hwmon_channel_info *lenovo_ec_hwmon_info[] = { + HWMON_CHANNEL_INFO(temp, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL), + HWMON_CHANNEL_INFO(fan, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX, + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX), + NULL +}; + +static const struct hwmon_ops lenovo_ec_hwmon_ops = { + .is_visible = lenovo_ec_hwmon_is_visible, + .read = lenovo_ec_hwmon_read, + .read_string = lenovo_ec_hwmon_read_string, +}; + +static struct hwmon_chip_info lenovo_ec_chip_info = { + .ops = &lenovo_ec_hwmon_ops, + .info = lenovo_ec_hwmon_info, +}; + +static int lenovo_ec_probe(struct platform_device *pdev) +{ + struct device *hwdev; + struct ec_sensors_data *ec_data; + const struct hwmon_chip_info *chip_info; + struct device *dev = &pdev->dev; + + ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data), GFP_KERNEL); + if (!ec_data) + return -ENOMEM; + + dev_set_drvdata(dev, ec_data); + + chip_info = &lenovo_ec_chip_info; + + mutex_lock(&ec_data->mec_mutex); + if (io_read8(MCHP_EMI0_APPLICATION_ID) != 0) { /* check EMI Application BIT */ + io_write8(0x90C, io_read8(0x90C)); /* set EMI Application BIT to 0 */ + } + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, MCHP_SING_IDX); + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, MCHP_SING_IDX >> 8); + mutex_unlock(&ec_data->mec_mutex); + + if ((io_read8(MCHP_EMI0_EC_DATA_BYTE0) == 'M') && + (io_read8(MCHP_EMI0_EC_DATA_BYTE1) == 'C') && + (io_read8(MCHP_EMI0_EC_DATA_BYTE2) == 'H') && + (io_read8(MCHP_EMI0_EC_DATA_BYTE3) == 'P')) { + ec_data->platform_id = get_platform(ec_data); + switch (ec_data->platform_id) { + case 0: + ec_data->fan_labels = px_ec_fan_label; + ec_data->temp_labels = lenovo_px_ec_temp_label; + break; + case 1: + ec_data->fan_labels = p7_ec_fan_label; + ec_data->temp_labels = lenovo_gen_ec_temp_label; + break; + case 2: + ec_data->fan_labels = p5_ec_fan_label; + ec_data->temp_labels = lenovo_gen_ec_temp_label; + break; + case 3: + ec_data->fan_labels = p7_amd_ec_fan_label; + ec_data->temp_labels = lenovo_gen_ec_temp_label; + break; + default: + dev_err(dev, "Unknown ThinkStation Model"); + return -EINVAL; + } + + hwdev = devm_hwmon_device_register_with_info(dev, "lenovo_ec", + ec_data, + chip_info, NULL); + + return PTR_ERR_OR_ZERO(hwdev); + } + return -ENODEV; +} + +static struct platform_driver lenovo_ec_sensors_platform_driver = { + .driver = { + .name = "lenovo-ec-sensors", + }, + .probe = lenovo_ec_probe, +}; + +static struct platform_device *lenovo_ec_sensors_platform_device; + +static int __init lenovo_ec_init(void) +{ + lenovo_ec_sensors_platform_device = + platform_create_bundle(&lenovo_ec_sensors_platform_driver, + lenovo_ec_probe, NULL, 0, NULL, 0); + + if (IS_ERR(lenovo_ec_sensors_platform_device)) + return PTR_ERR(lenovo_ec_sensors_platform_device); + + return 0; +} + +static void __exit lenovo_ec_exit(void) +{ + platform_device_unregister(lenovo_ec_sensors_platform_device); + platform_driver_unregister(&lenovo_ec_sensors_platform_driver); +} + +module_init(lenovo_ec_init); +module_exit(lenovo_ec_exit); + +MODULE_AUTHOR("David Ober <dober@lenovo.com>"); +MODULE_DESCRIPTION("HWMON driver for MEC172x EC sensors on LENOVO motherboards"); +MODULE_LICENSE("GPL");
This addition adds in the ability for the system to scan the MEC172x EC chip in Lenovo ThinkStation systems to get the current fan RPM speeds and the Maximum speed value for each fan also provides the current CPU and DIMM thermal status Signed-off-by: David Ober <dober6023@gmail.com> v2 fixed mixcased naming v2 add mutex protection v2 removed references to ACPI as it is not used v2 added comment to explain why returning a -1 is needed --- drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/lenovo-ec-sensors.c | 484 ++++++++++++++++++++++++++++++ 3 files changed, 495 insertions(+) create mode 100644 drivers/hwmon/lenovo-ec-sensors.c