[PATCH v2 0/3] hwmon: xgene: Add support for X-Gene hwmon driver

[PATCH v2 0/3] hwmon: xgene: Add support for X-Gene hwmon driver

[Hoan Tran]

This patch set 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.

For ACPI, tested with this patch[1] which supports PCC subspace 2
[1] http://www.spinics.net/lists/linux-acpi/msg66041.html
 - [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2

v2
 - Increase power reading accurateness by using 2 registers
(a register for Watt, another for milli-Watt)
 - Remove power reading for SoC
 - Fix review comments from Guenter

v1
 - Initial

Hoan Tran (3):
  Documentation: dtb: xgene: Add hwmon dts binding documentation
  hwmon: xgene: Add hwmon driver
  arm64: dts: apm: Add X-Gene SoC hwmon to device tree

 .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt  |  14 +
 Documentation/hwmon/xgene-hwmon                    |  30 +
 arch/arm64/boot/dts/apm/apm-shadowcat.dtsi         |   5 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   5 +
 drivers/hwmon/Kconfig                              |   7 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/xgene-hwmon.c                        | 772 +++++++++++++++++++++
 7 files changed, 834 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
 create mode 100644 Documentation/hwmon/xgene-hwmon
 create mode 100644 drivers/hwmon/xgene-hwmon.c

-- 
1.9.1

[PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation

[Hoan Tran]

This patch adds the APM X-Gene hwmon device tree node documentation.

Signed-off-by: Hoan Tran <hotran@apm.com>
---
 .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt

diff --git a/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
new file mode 100644
index 0000000..59b3855
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
@@ -0,0 +1,14 @@
+APM X-Gene hwmon driver
+
+APM X-Gene SOC sensors are accessed over the "SLIMpro" mailbox.
+
+Required properties :
+ - compatible : should be "apm,xgene-slimpro-hwmon"
+ - mboxes : use the label reference for the mailbox as the first parameter.
+	    The second parameter is the channel number.
+
+Example :
+	hwmonslimpro {
+		compatible = "apm,xgene-slimpro-hwmon";
+		mboxes = <&mailbox 7>;
+	};
-- 
1.9.1

[PATCH v2 2/3] hwmon: xgene: Add hwmon driver

[Hoan Tran]

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@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@apm.com>
+ *         Hoan Tran <hotran@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>
+
+/* SLIMpro message defines */
+#define MSG_TYPE_DBG			0
+#define MSG_TYPE_ERR			7
+#define MSG_TYPE_PWRMGMT		9
+
+#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)
+{
+	unsigned long temp = val;
+
+	/* Convert 9 bit signed temperature to integer */
+	if (test_and_clear_bit(TEMP_NEGATIVE_BIT, &temp))
+		return (temp - 256);
+	else
+		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);
+
+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;
+	snprintf(name, sizeof(name), "temp0_critical_alarm");
+	sysfs_notify(&ctx->dev->kobj, NULL, name);
+
+	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);
+	}
+}
+
+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);
+	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");
-- 
1.9.1

[PATCH v2 3/3] arm64: dts: apm: Add X-Gene SoC hwmon to device tree

[Hoan Tran]

This patch adds DT node to enable hwmon driver for APM X-Gene SoC.

Signed-off-by: Hoan Tran <hotran@apm.com>
---
 arch/arm64/boot/dts/apm/apm-shadowcat.dtsi | 5 +++++
 arch/arm64/boot/dts/apm/apm-storm.dtsi     | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi b/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi
index c569f76..a5935f5 100644
--- a/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi
@@ -472,6 +472,11 @@
 			mboxes = <&mailbox 0>;
 		};
 
+		hwmonslimpro {
+			compatible = "apm,xgene-slimpro-hwmon";
+			mboxes = <&mailbox 7>;
+		};
+
 		serial0: serial@10600000 {
 			device_type = "serial";
 			compatible = "ns16550";
diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index 5147d76..f8ea5b5 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -716,6 +716,11 @@
 			mboxes = <&mailbox 0>;
 		};
 
+		hwmonslimpro {
+			compatible = "apm,xgene-slimpro-hwmon";
+			mboxes = <&mailbox 7>;
+		};
+
 		serial0: serial@1c020000 {
 			status = "disabled";
 			device_type = "serial";
-- 
1.9.1

Re: [PATCH v2 2/3] hwmon: xgene: Add hwmon driver

[Guenter Roeck]

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@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@apm.com>
> + *         Hoan Tran <hotran@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");
>

Re: [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation

[Rob Herring]

On Mon, Jul 11, 2016 at 05:30:14PM -0700, Hoan Tran wrote:
> This patch adds the APM X-Gene hwmon device tree node documentation.
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>
> ---
>  .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt          | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/3] hwmon: xgene: Add hwmon driver

[Hoan Tran]

Hi Guenter,

On Sat, Jul 16, 2016 at 9:35 AM, Guenter Roeck <linux@roeck-us.net> 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@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@apm.com>
>> + *         Hoan Tran <hotran@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 ?

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");
>>
>