From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:41601 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751566AbcGPQgA (ORCPT ); Sat, 16 Jul 2016 12:36:00 -0400 Subject: Re: [PATCH v2 2/3] hwmon: xgene: Add hwmon driver To: Hoan Tran , Jean Delvare , Jonathan Corbet , Rob Herring References: <1468283416-12780-1-git-send-email-hotran@apm.com> <1468283416-12780-3-git-send-email-hotran@apm.com> Cc: Jassi Brar , Ashwin Chaugule , linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, Duc Dang , lho@apm.com From: Guenter Roeck Message-ID: <578A6257.1050204@roeck-us.net> Date: Sat, 16 Jul 2016 09:35:35 -0700 MIME-Version: 1.0 In-Reply-To: <1468283416-12780-3-git-send-email-hotran@apm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-hwmon-owner@vger.kernel.org List-Id: linux-hwmon@vger.kernel.org On 07/11/2016 05:30 PM, Hoan Tran wrote: > This patch adds hardware temperature and power reading support for > APM X-Gene SoC using the mailbox communication interface. > > Signed-off-by: Hoan Tran > --- > Documentation/hwmon/xgene-hwmon | 30 ++ > drivers/hwmon/Kconfig | 7 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/xgene-hwmon.c | 772 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 810 insertions(+) > create mode 100644 Documentation/hwmon/xgene-hwmon > create mode 100644 drivers/hwmon/xgene-hwmon.c > > diff --git a/Documentation/hwmon/xgene-hwmon b/Documentation/hwmon/xgene-hwmon > new file mode 100644 > index 0000000..6ec50ed > --- /dev/null > +++ b/Documentation/hwmon/xgene-hwmon > @@ -0,0 +1,30 @@ > +Kernel driver xgene-hwmon > +======================== > + > +Supported chips: > + * APM X-Gene SoC > + > +Description > +----------- > + > +This driver adds hardware temperature and power reading support for > +APM X-Gene SoC using the mailbox communication interface. > +For device tree, it is the standard DT mailbox. > +For ACPI, it is the PCC mailbox. > + > +The following sensors are supported > + > + * Temperature > + - SoC on-die temperature in milli-degree C > + - Alarm when high/over temperature occurs > + * Power > + - CPU power in uW > + - IO power in uW > + > +sysfs-Interface > +--------------- > + > +temp0_input - SoC on-die temperature (milli-degree C) > +temp0_critical_alarm - An 1 would indicates on-die temperature exceeded threshold > +power0_input - CPU power in (uW) > +power1_input - IO power in (uW) > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index ff94007..55218c6 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1787,6 +1787,13 @@ config SENSORS_ULTRA45 > This driver provides support for the Ultra45 workstation environmental > sensors. > > +config SENSORS_XGENE > + tristate "APM X-Gene SoC hardware monitoring driver" > + depends on XGENE_SLIMPRO_MBOX || PCC > + help > + If you say yes here you get support for the temperature > + and power sensors for APM X-Gene SoC. > + > if ACPI > > comment "ACPI drivers" > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 2ef5b7c..a2460dd 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -162,6 +162,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o > obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o > obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o > obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o > +obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o > > obj-$(CONFIG_PMBUS) += pmbus/ > > diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c > new file mode 100644 > index 0000000..03917e3 > --- /dev/null > +++ b/drivers/hwmon/xgene-hwmon.c > @@ -0,0 +1,772 @@ > +/* > + * APM X-Gene SoC Hardware Monitoring Driver > + * > + * Copyright (c) 2016, Applied Micro Circuits Corporation > + * Author: Loc Ho > + * Hoan Tran > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see . > + * > + * This driver provides the following features: > + * - Retrieve CPU total power (uW) > + * - Retrieve IO total power (uW) > + * - Retrieve SoC temperature (milli-degree C) and alarm > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + Please order include files alphabetically. > +/* SLIMpro message defines */ > +#define MSG_TYPE_DBG 0 > +#define MSG_TYPE_ERR 7 > +#define MSG_TYPE_PWRMGMT 9 Are those only used in this driver ? > +#define MSG_TYPE(v) (((v) & 0xF0000000) >> 28) > +#define MSG_TYPE_SET(v) (((v) << 28) & 0xF0000000) > +#define MSG_SUBTYPE(v) (((v) & 0x0F000000) >> 24) > +#define MSG_SUBTYPE_SET(v) (((v) << 24) & 0x0F000000) > + > +#define DBG_SUBTYPE_SENSOR_READ 4 > +#define SENSOR_RD_MSG 0x04FFE902 > +#define SENSOR_RD_EN_ADDR(a) ((a) & 0x000FFFFF) > +#define PMD_PWR_REG 0x20 > +#define PMD_PWR_MW_REG 0x26 > +#define SOC_PWR_REG 0x21 > +#define SOC_PWR_MW_REG 0x27 > +#define SOC_TEMP_REG 0x10 > + > +#define TEMP_NEGATIVE_BIT 8 > + > +#define PWRMGMT_SUBTYPE_TPC 1 > +#define TPC_ALARM 2 > +#define TPC_GET_ALARM 3 > +#define TPC_CMD(v) (((v) & 0x00FF0000) >> 16) > +#define TPC_CMD_SET(v) (((v) << 16) & 0x00FF0000) > +#define TPC_EN_MSG(hndl, cmd, type) \ > + (MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \ > + MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type) > + > +/* PCC defines */ > +#define PCC_SIGNATURE_MASK 0x50424300 > +#define PCCC_GENERATE_DB_INT BIT(15) > +#define PCCS_CMD_COMPLETE BIT(0) > +#define PCCS_SCI_DOORBEL BIT(1) > +#define PCCS_PLATFORM_NOTIFICATION BIT(3) > +/* > + * Arbitrary retries in case the remote processor is slow to respond > + * to PCC commands > + */ > +#define PCC_NUM_RETRIES 500 > + > +#define ASYNC_MSG_FIFO_SIZE 16 > +#define MBOX_OP_TIMEOUTMS 1000 > + > +#define WATT_TO_mWATT(x) ((x) * 1000) > +#define mWATT_TO_uWATT(x) ((x) * 1000) > +#define CELSIUS_TO_mCELSIUS(x) ((x) * 1000) > + > +#define to_xgene_hwmon_dev(cl) \ > + container_of(cl, struct xgene_hwmon_dev, mbox_client) > + > +struct slimpro_resp_msg { > + u32 msg; > + u32 param1; > + u32 param2; > +} __packed; > + > +struct xgene_hwmon_dev { > + struct device *dev; > + struct mbox_chan *mbox_chan; > + struct mbox_client mbox_client; > + int mbox_idx; > + > + spinlock_t kfifo_lock; > + struct mutex rd_mutex; > + struct completion rd_complete; > + int resp_pending; > + struct slimpro_resp_msg sync_msg; > + > + struct work_struct workq; > + struct kfifo_rec_ptr_1 async_msg_fifo; > + > + struct device *hwmon_dev; > + bool temp_critical_alarm; > + > + phys_addr_t comm_base_addr; > + void *pcc_comm_addr; > + u64 usecs_lat; > +}; > + > +/* > + * This function tests and clears a bitmask then returns its old value > + */ > +static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask) > +{ > + u16 ret, val; > + > + val = readw_relaxed(addr); > + ret = val & mask; > + val &= ~mask; > + writew_relaxed(val, addr); > + > + return ret; > +} > + > +static int xgene_hwmon_decode_temp(u32 val) The "hwmon" in the function names does not really add any value. I would suggest to drop it. > +{ > + unsigned long temp = val; > + > + /* Convert 9 bit signed temperature to integer */ > + if (test_and_clear_bit(TEMP_NEGATIVE_BIT, &temp)) > + return (temp - 256); > + else else after return is unnecessary. I would suggest to use sign_extend32(val, TEMP_NEGATIVE_BIT). > + return temp; > +} > + > +static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg) > +{ > + struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; > + void *ptr = generic_comm_base + 1; > + int rc, i; > + u16 val; > + > + mutex_lock(&ctx->rd_mutex); > + init_completion(&ctx->rd_complete); > + ctx->resp_pending = true; > + > + /* Write signature for subspace */ > + writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx, > + &generic_comm_base->signature); > + > + /* Write to the shared command region */ > + writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT, > + &generic_comm_base->command); > + > + /* Flip CMD COMPLETE bit */ > + val = readw_relaxed(&generic_comm_base->status); > + val &= ~PCCS_CMD_COMPLETE; > + writew_relaxed(val, &generic_comm_base->status); > + > + /* Copy the message to the PCC comm space */ > + for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++) > + writel_relaxed(msg[i], ptr + i * 4); > + > + /* Ring the doorbell */ > + rc = mbox_send_message(ctx->mbox_chan, msg); > + if (rc < 0) { > + dev_err(ctx->dev, "Mailbox send error %d\n", rc); > + goto err; > + } > + if (!wait_for_completion_timeout(&ctx->rd_complete, > + usecs_to_jiffies(ctx->usecs_lat))) { > + dev_err(ctx->dev, "Mailbox operation timed out\n"); > + rc = -ETIMEDOUT; > + goto err; > + } > + > + /* Check for invalid data */ > + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { > + rc = -EINVAL; > + goto err; > + } > + > + msg[0] = ctx->sync_msg.msg; > + msg[1] = ctx->sync_msg.param1; > + msg[2] = ctx->sync_msg.param2; > + > +err: > + mbox_chan_txdone(ctx->mbox_chan, 0); > + ctx->resp_pending = false; > + mutex_unlock(&ctx->rd_mutex); > + return rc; > +} > + > +static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg) > +{ > + int rc; > + > + mutex_lock(&ctx->rd_mutex); > + init_completion(&ctx->rd_complete); > + ctx->resp_pending = true; > + > + rc = mbox_send_message(ctx->mbox_chan, msg); > + if (rc < 0) { > + dev_err(ctx->dev, "Mailbox send error %d\n", rc); > + goto err; > + } > + > + if (!wait_for_completion_timeout(&ctx->rd_complete, > + msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) { > + dev_err(ctx->dev, "Mailbox operation timed out\n"); > + rc = -ETIMEDOUT; > + goto err; > + } > + > + /* Check for invalid data */ > + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { > + rc = -EINVAL; > + goto err; > + } > + > + msg[0] = ctx->sync_msg.msg; > + msg[1] = ctx->sync_msg.param1; > + msg[2] = ctx->sync_msg.param2; > + > +err: > + ctx->resp_pending = false; > + mutex_unlock(&ctx->rd_mutex); > + return rc; > +} > + > +static int xgene_hwmon_reg_map_rd(struct xgene_hwmon_dev *ctx, u32 addr, > + u32 *data) > +{ > + u32 msg[3]; > + int rc; > + > + msg[0] = SENSOR_RD_MSG; > + msg[1] = SENSOR_RD_EN_ADDR(addr); > + msg[2] = 0; > + > + if (acpi_disabled) > + rc = xgene_hwmon_rd(ctx, msg); > + else > + rc = xgene_hwmon_pcc_rd(ctx, msg); > + > + if (rc < 0) > + return rc; > + > + *data = msg[1]; > + > + return rc; > +} > + > +static int xgene_hwmon_get_notification_msg(struct xgene_hwmon_dev *ctx, > + u32 *amsg) > +{ > + u32 msg[3]; > + int rc; > + > + msg[0] = TPC_EN_MSG(PWRMGMT_SUBTYPE_TPC, TPC_GET_ALARM, 0); > + msg[1] = 0; > + msg[2] = 0; > + > + rc = xgene_hwmon_pcc_rd(ctx, msg); > + if (rc < 0) > + return rc; > + > + amsg[0] = msg[0]; > + amsg[1] = msg[1]; > + amsg[2] = msg[2]; > + > + return rc; > +} > + > +static int xgene_hwmon_get_cpu_pwr(struct xgene_hwmon_dev *ctx, u32 *val) > +{ > + u32 watt, mwatt; > + int rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_REG, &watt); > + if (rc < 0) > + return rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, &mwatt); > + if (rc < 0) > + return rc; > + > + *val = WATT_TO_mWATT(watt) + mwatt; > + return 0; > +} > + > +static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val) > +{ > + u32 watt, mwatt; > + int rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, &watt); > + if (rc < 0) > + return rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_MW_REG, &mwatt); > + if (rc < 0) > + return rc; > + > + *val = WATT_TO_mWATT(watt) + mwatt; > + return 0; > +} > + > +static int xgene_hwmon_get_temp(struct xgene_hwmon_dev *ctx, u32 *val) > +{ > + return xgene_hwmon_reg_map_rd(ctx, SOC_TEMP_REG, val); > +} > + > +/* > + * Sensor temperature/power functions > + */ > +static ssize_t xgene_hwmon_show_temp(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + int rc, temp; > + u32 val; > + > + rc = xgene_hwmon_get_temp(ctx, &val); > + if (rc < 0) > + return rc; > + > + temp = xgene_hwmon_decode_temp(val); > + > + return snprintf(buf, PAGE_SIZE, "%d\n", CELSIUS_TO_mCELSIUS(temp)); > +} > + > +static ssize_t xgene_hwmon_show_temp_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "SoC Temperature\n"); > +} > + > +static ssize_t > +xgene_hwmon_show_temp_critical_alarm(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + > + return snprintf(buf, PAGE_SIZE, "%d\n", ctx->temp_critical_alarm); > +} > + > + > +static ssize_t xgene_hwmon_show_cpu_pwr_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "CPU power\n"); > +} > + > +static ssize_t xgene_hwmon_show_io_pwr_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "IO power\n"); > +} > + > +static ssize_t xgene_hwmon_show_cpu_pwr(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + u32 val; > + int rc; > + > + rc = xgene_hwmon_get_cpu_pwr(ctx, &val); > + if (rc < 0) > + return rc; > + > + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); > +} > + > +static ssize_t xgene_hwmon_show_io_pwr(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + u32 val; > + int rc; > + > + rc = xgene_hwmon_get_io_pwr(ctx, &val); > + if (rc < 0) > + return rc; > + > + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); > +} > + > +static SENSOR_DEVICE_ATTR(temp0_label, S_IRUGO, xgene_hwmon_show_temp_label, > + NULL, 0); > +static SENSOR_DEVICE_ATTR(temp0_input, S_IRUGO, xgene_hwmon_show_temp, > + NULL, 0); > +static SENSOR_DEVICE_ATTR(temp0_critical_alarm, S_IRUGO, > + xgene_hwmon_show_temp_critical_alarm, NULL, 0); > + > +static SENSOR_DEVICE_ATTR(power0_label, S_IRUGO, > + xgene_hwmon_show_cpu_pwr_label, NULL, 0); > +static SENSOR_DEVICE_ATTR(power0_input, S_IRUGO, xgene_hwmon_show_cpu_pwr, > + NULL, 0); > + > +static SENSOR_DEVICE_ATTR(power1_label, S_IRUGO, > + xgene_hwmon_show_io_pwr_label, NULL, 1); > +static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, xgene_hwmon_show_io_pwr, > + NULL, 1); > + Temperature and power attributes start with index 1. You don't use the index value in SENSOR_DEVICE_ATTR(), so DEVCIE_ATTR_RO() might be a better choice. > +static struct attribute *xgene_hwmon_attrs[] = { > + &sensor_dev_attr_temp0_label.dev_attr.attr, > + &sensor_dev_attr_temp0_input.dev_attr.attr, > + &sensor_dev_attr_temp0_critical_alarm.dev_attr.attr, > + &sensor_dev_attr_power0_label.dev_attr.attr, > + &sensor_dev_attr_power0_input.dev_attr.attr, > + &sensor_dev_attr_power1_label.dev_attr.attr, > + &sensor_dev_attr_power1_input.dev_attr.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(xgene_hwmon); > + > +static int xgene_hwmon_tpc_alarm(struct xgene_hwmon_dev *ctx, > + struct slimpro_resp_msg *amsg) > +{ > + char name[40]; > + > + ctx->temp_critical_alarm = amsg->param2 ? true : false; You could use !!amsg->param2, or simply rely on the compiler and C standard to auto-convert the integer to boolean. > + snprintf(name, sizeof(name), "temp0_critical_alarm"); > + sysfs_notify(&ctx->dev->kobj, NULL, name); Why not just use "temp0_critical_alarm" (or rather "temp1_critical_alarm") as parameter to sysfs_notify() ? > + > + return 0; > +} > + > +static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx, > + struct slimpro_resp_msg *amsg) > +{ > + if ((MSG_SUBTYPE(amsg->msg) == PWRMGMT_SUBTYPE_TPC) && > + (TPC_CMD(amsg->msg) == TPC_ALARM)) > + xgene_hwmon_tpc_alarm(ctx, amsg); > +} > + > +/* > + * This function is called to process async work queue > + */ > +static void xgene_hwmon_evt_work(struct work_struct *work) > +{ > + struct slimpro_resp_msg amsg; > + struct xgene_hwmon_dev *ctx; > + int ret; > + > + ctx = container_of(work, struct xgene_hwmon_dev, workq); > + while (kfifo_out_spinlocked(&ctx->async_msg_fifo, &amsg, > + sizeof(struct slimpro_resp_msg), > + &ctx->kfifo_lock)) { > + /* > + * If PCC, send a consumer command to Platform to get info > + * If Slimpro Mailbox, get message from specific FIFO > + */ > + if (!acpi_disabled) { > + ret = xgene_hwmon_get_notification_msg(ctx, > + (u32 *)&amsg); > + if (ret < 0) > + continue; > + } > + > + if (MSG_TYPE(amsg.msg) == MSG_TYPE_PWRMGMT) > + xgene_hwmon_process_pwrmsg(ctx, &amsg); > + } > +} > + > +/* > + * This function is called when the SLIMpro Mailbox received a message > + */ > +static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg) > +{ > + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); > + struct slimpro_resp_msg amsg; > + > + /* > + * Response message format: > + * msg[0] is the return code of the operation > + * msg[1] is the first parameter word > + * msg[2] is the second parameter word > + * > + * As message only supports dword size, just assign it. > + */ > + > + /* Check for sync query */ > + if (ctx->resp_pending && > + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && > + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && > + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && > + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { > + ctx->sync_msg.msg = ((u32 *) msg)[0]; > + ctx->sync_msg.param1 = ((u32 *) msg)[1]; > + ctx->sync_msg.param2 = ((u32 *) msg)[2]; > + > + /* Operation waiting for response */ > + complete(&ctx->rd_complete); > + > + return; > + } > + > + amsg.msg = ((u32 *) msg)[0]; > + amsg.param1 = ((u32 *) msg)[1]; > + amsg.param2 = ((u32 *) msg)[2]; > + > + /* Enqueue to the FIFO */ > + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, > + sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock); > + /* Schedule the bottom handler */ > + schedule_work(&ctx->workq); > +} > + > +/* > + * This function is called when the PCC Mailbox received a message > + */ > +static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg) > +{ > + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); > + struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; > + struct slimpro_resp_msg amsg; > + > + msg = generic_comm_base + 1; > + /* Check if platform sends interrupt */ > + if (!xgene_word_tst_and_clr(&generic_comm_base->status, > + PCCS_SCI_DOORBEL)) > + return; > + > + /* > + * Response message format: > + * msg[0] is the return code of the operation > + * msg[1] is the first parameter word > + * msg[2] is the second parameter word > + * > + * As message only supports dword size, just assign it. > + */ > + > + /* Check for sync query */ > + if (ctx->resp_pending && > + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && > + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && > + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && > + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { > + > + /* Check if platform completes command */ > + if (xgene_word_tst_and_clr(&generic_comm_base->status, > + PCCS_CMD_COMPLETE)) { > + ctx->sync_msg.msg = ((u32 *) msg)[0]; > + ctx->sync_msg.param1 = ((u32 *) msg)[1]; > + ctx->sync_msg.param2 = ((u32 *) msg)[2]; > + > + /* Operation waiting for response */ > + complete(&ctx->rd_complete); > + > + return; > + } > + } > + > + /* > + * Platform notifies interrupt to OSPM. > + * OPSM schedules a consumer command to get this information > + * in a workqueue. Platform must wait until OSPM has issued > + * a consumer command that serves this notification. > + */ > + > + /* Enqueue to the FIFO */ > + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, > + sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock); > + /* Schedule the bottom handler */ > + schedule_work(&ctx->workq); > +} > + > +static void xgene_hwmon_tx_done(struct mbox_client *cl, void *msg, int ret) > +{ > + if (ret) { > + dev_dbg(cl->dev, "TX did not complete: CMD sent:%x, ret:%d\n", > + *(u16 *) msg, ret); > + } else { > + dev_dbg(cl->dev, "TX completed. CMD sent:%x, ret:%d\n", > + *(u16 *) msg, ret); Please run your patch through checkoatch --strict. I would like to see at least the following messages resolved. "No space is necessary after a cast" "Blank lines aren't necessary after an open brace '{'" "Please don't use multiple blank lines" "Alignment should match open parenthesis" "line over 80 characters" (with precedence over continuation line alignment) > + } > +} > + > +static int xgene_hwmon_probe(struct platform_device *pdev) > +{ > + struct xgene_hwmon_dev *ctx; > + struct mbox_client *cl; > + int rc; > + > + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + > + ctx->dev = &pdev->dev; > + platform_set_drvdata(pdev, ctx); > + cl = &ctx->mbox_client; > + > + /* Request mailbox channel */ > + cl->dev = &pdev->dev; > + cl->tx_done = xgene_hwmon_tx_done; > + cl->tx_block = false; > + cl->tx_tout = MBOX_OP_TIMEOUTMS; > + cl->knows_txdone = false; > + if (acpi_disabled) { > + cl->rx_callback = xgene_hwmon_rx_cb; > + ctx->mbox_chan = mbox_request_channel(cl, 0); > + if (IS_ERR(ctx->mbox_chan)) { > + dev_err(&pdev->dev, > + "SLIMpro mailbox channel request failed\n"); > + return -ENODEV; > + } > + } else { > + struct acpi_pcct_hw_reduced *cppc_ss; > + > + if (device_property_read_u32(&pdev->dev, "pcc-channel", > + &ctx->mbox_idx)) { > + dev_err(&pdev->dev, "no pcc-channel property\n"); > + return -ENODEV; > + } > + > + cl->rx_callback = xgene_hwmon_pcc_rx_cb; > + ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx); > + if (IS_ERR(ctx->mbox_chan)) { > + dev_err(&pdev->dev, > + "PPC channel request failed\n"); > + return -ENODEV; > + } > + > + /* > + * The PCC mailbox controller driver should > + * have parsed the PCCT (global table of all > + * PCC channels) and stored pointers to the > + * subspace communication region in con_priv. > + */ > + cppc_ss = ctx->mbox_chan->con_priv; > + if (!cppc_ss) { > + dev_err(&pdev->dev, "PPC subspace not found\n"); > + rc = -ENODEV; > + goto out_mbox_free; > + } > + > + if (!ctx->mbox_chan->mbox->txdone_irq) { > + dev_err(&pdev->dev, "PCC IRQ not supported\n"); > + rc = -ENODEV; > + goto out_mbox_free; > + } > + > + /* > + * This is the shared communication region > + * for the OS and Platform to communicate over. > + */ > + ctx->comm_base_addr = cppc_ss->base_address; > + if (ctx->comm_base_addr) { > + ctx->pcc_comm_addr = > + acpi_os_ioremap(ctx->comm_base_addr, > + cppc_ss->length); > + } else { > + dev_err(&pdev->dev, "Failed to get PCC comm region\n"); > + rc = -ENODEV; > + goto out_mbox_free; > + } > + > + if (!ctx->pcc_comm_addr) { > + dev_err(&pdev->dev, > + "Failed to ioremap PCC comm region\n"); > + rc = -ENOMEM; > + goto out_mbox_free; > + } > + > + /* > + * cppc_ss->latency is just a Nominal value. In reality > + * the remote processor could be much slower to reply. > + * So add an arbitrary amount of wait on top of Nominal. > + */ > + ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency; > + } > + > + spin_lock_init(&ctx->kfifo_lock); > + mutex_init(&ctx->rd_mutex); > + > + rc = kfifo_alloc(&ctx->async_msg_fifo, > + sizeof(struct slimpro_resp_msg) * ASYNC_MSG_FIFO_SIZE, > + GFP_KERNEL); > + if (rc) > + goto out_mbox_free; > + > + INIT_WORK(&ctx->workq, xgene_hwmon_evt_work); > + > + ctx->hwmon_dev = devm_hwmon_device_register_with_groups(ctx->dev, > + "apm_xgene", > + ctx, > + xgene_hwmon_groups); Using the devm_ function here would, on remove, result in the mailbox and kfifo being removed before the hwmon driver is removed. This might result in race conditions. You have two options: Use devm_add_action() to remove those in sequence, or use hwmon_device_register_with_groups() and remove the hwmon device explicitly (and first) in the remove function. Note that you don't have to store hwmon_dev in ctx if you use devm_add_action() and devm_hwmon_device_register_with_groups(). > + if (IS_ERR(ctx->hwmon_dev)) { > + dev_err(&pdev->dev, "Failed to register HW monitor device\n"); > + rc = PTR_ERR(ctx->hwmon_dev); > + goto out; > + } > + > + dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n"); > + > + return rc; > + > +out: > + kfifo_free(&ctx->async_msg_fifo); > +out_mbox_free: > + if (acpi_disabled) > + mbox_free_channel(ctx->mbox_chan); > + else > + pcc_mbox_free_channel(ctx->mbox_chan); > + > + return rc; > +} > + > +static int xgene_hwmon_remove(struct platform_device *pdev) > +{ > + struct xgene_hwmon_dev *ctx = platform_get_drvdata(pdev); > + > + kfifo_free(&ctx->async_msg_fifo); > + if (acpi_disabled) > + mbox_free_channel(ctx->mbox_chan); > + else > + pcc_mbox_free_channel(ctx->mbox_chan); > + > + return 0; > +} > + > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id xgene_hwmon_acpi_match[] = { > + {"APMC0D29", 0}, > + {}, > +}; > +MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match); > +#endif > + > +static const struct of_device_id xgene_hwmon_of_match[] = { > + {.compatible = "apm,xgene-slimpro-hwmon"}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, xgene_hwmon_of_match); > + > +static struct platform_driver xgene_hwmon_driver __refdata = { > + .probe = xgene_hwmon_probe, > + .remove = xgene_hwmon_remove, > + .driver = { > + .name = "xgene-slimpro-hwmon", > + .of_match_table = xgene_hwmon_of_match, > + .acpi_match_table = ACPI_PTR(xgene_hwmon_acpi_match), > + }, > +}; > +module_platform_driver(xgene_hwmon_driver); > + > +MODULE_DESCRIPTION("APM X-Gene SoC hardware monitor"); > +MODULE_LICENSE("GPL"); >