Hi Guenter, On Sat, Jul 16, 2016 at 9:35 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > 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 <hotran@xxxxxxx> >> --- >> 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 <lho@xxxxxxx> >> + * Hoan Tran <hotran@xxxxxxx> >> + * >> + * 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 <http://www.gnu.org/licenses/>. >> + * >> + * 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 <linux/acpi.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/hwmon.h> >> +#include <linux/hwmon-sysfs.h> >> +#include <linux/kfifo.h> >> +#include <linux/interrupt.h> >> +#include <linux/mailbox_controller.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <acpi/acpi_io.h> >> +#include <acpi/pcc.h> >> + > > 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 ? MSG_TYPE_DBG is used by another driver, i2c_xgene_slimpro. Others are only used by 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). That's a good suggestion. I'll use sign_extend32(). > > >> + 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. Yes, I'll use DEVICE_ATTR_RO() and start with index 1. > >> +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. Yes, I'll use hwmon_device_register_with_groups(). Thanks Hoan > > 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"); >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
