[PATCH v2 2/3] hwmon: xgene: Add hwmon driver
Guenter Roeck
linux at roeck-us.net
Sat Jul 16 09:35:35 PDT 2016
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 at apm.com>
> ---
> 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 at apm.com>
> + * Hoan Tran <hotran at apm.com>
> + *
> + * 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 ?
> +#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");
>
More information about the linux-arm-kernel
mailing list