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

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

[Hoan Tran]

This patch set adds hardware temperature and power reading support ​for
APM X-Gene SoC's 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] https://lkml.org/lkml/2016/5/6/482
 - [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2

Hoan Tran (3):
  Documentation: dtb: xgene: Add hwmon dts binding documentation
  hwmon: xgene: Adds 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                    |  32 +
 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                        | 801 +++++++++++++++++++++
 7 files changed, 865 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 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..49a482e
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
@@ -0,0 +1,14 @@
+APM X-Gene hwmon driver
+
+Hwmon driver accesses sensors 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 2/3] hwmon: xgene: Adds hwmon driver

[Hoan Tran]

This patch adds hardware temperature and power reading support for
APM X-Gene SoC's using the mailbox communication interface.

Signed-off-by: Hoan Tran <hotran@apm.com>
---
 Documentation/hwmon/xgene-hwmon |  32 ++
 drivers/hwmon/Kconfig           |   7 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/xgene-hwmon.c     | 801 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 841 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..040a7f2
--- /dev/null
+++ b/Documentation/hwmon/xgene-hwmon
@@ -0,0 +1,32 @@
+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's 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
+    - Alarm when high/over temperature occurs
+  * Power
+    - CPU power in uW
+    - IO power in uW
+    - CPU and IO power in uW
+
+sysfs-Interface
+---------------
+
+temp1_input - SoC on-die temperature (milli-degree C)
+temp1_critical_alarm - An 1 would indicates on-die temperature exceeded threshold
+power1_input - CPU power in (uW)
+power2_input - IO power in (uW)
+power3_input - CPU and IO power in (uW)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5c2d13a..91c3056 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1776,6 +1776,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 58cc3ac..668d0f1 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -161,6 +161,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..bc4ad29
--- /dev/null
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -0,0 +1,801 @@
+/*
+ * 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's total power (uW)
+ *  - Retrieve IO's total power (uW)
+ *  - Retrieve SoC 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_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <acpi/cppc_acpi.h>
+
+/* SLIMpro message defines */
+#define SLIMPRO_MSG_TYPE_DBG_ID		0
+#define SLIMPRO_MSG_TYPE_ERR_ID		7
+#define SLIMPRO_MSG_TYPE_PWRMGMT_ID	9
+
+#define SLIMPRO_MSG_TYPE(v)		(((v) & 0xF0000000) >> 28)
+#define SLIMPRO_MSG_TYPE_SET(v)		(((v) << 28) & 0xF0000000)
+#define SLIMPRO_MSG_SUBTYPE(v)		(((v) & 0x0F000000) >> 24)
+#define SLIMPRO_MSG_SUBTYPE_SET(v)	(((v) << 24) & 0x0F000000)
+
+#define SLIMPRO_DBG_SUBTYPE_SENSOR_READ	4
+#define SLIMPRO_SENSOR_READ_MSG		0x04FFE902
+#define SLIMPRO_SENSOR_READ_ENCODE_ADDR(a) \
+	((a) & 0x000FFFFF)
+#define PMD_PWR_MW_REG			0x26
+#define SOC_PWR_REG			0x21
+#define SOC_TEMP_REG			0x10
+
+#define SLIMPRO_PWRMGMT_SUBTYPE_TPC	1
+#define SLIMPRO_TPC_ALARM		2
+#define SLIMPRO_TPC_GET_ALARM		3
+#define SLIMPRO_TPC_CMD(v)		(((v) & 0x00FF0000) >> 16)
+#define SLIMPRO_TPC_CMD_SET(v)		(((v) << 16) & 0x00FF0000)
+#define SLIMPRO_TPC_ENCODE_MSG(hndl, cmd, type) \
+	(SLIMPRO_MSG_TYPE_SET(SLIMPRO_MSG_TYPE_PWRMGMT_ID) | \
+	SLIMPRO_MSG_SUBTYPE_SET(hndl) | \
+	SLIMPRO_TPC_CMD_SET(cmd) | \
+	type)
+
+/* PCC defines */
+#define SLIMPRO_MSG_PCC_SUBSPACE	7
+#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_HWMON_INDEX		0
+#define MBOX_OP_TIMEOUTMS		1000
+
+#define SOC_TEMP			0
+#define CPU_POWER			0
+#define IO_POWER			1
+#define SOC_POWER			2
+
+#define WATT_TO_mWATT(x)		((x) * 1000)
+#define mWATT_TO_uWATT(x)		((x) * 1000)
+#define WATT_TO_uWATT(x)		((x) * 1000000)
+#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;
+
+	spinlock_t		lock;
+	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_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;
+	unsigned long flags;
+	u16 val;
+	int rc;
+
+	spin_lock_irqsave(&ctx->lock, flags);
+	if (ctx->resp_pending) {
+		spin_unlock_irqrestore(&ctx->lock, flags);
+		return -EAGAIN;
+	}
+
+	init_completion(&ctx->rd_complete);
+	ctx->resp_pending = true;
+
+	/* Write signature for subspace */
+	writel_relaxed(PCC_SIGNATURE_MASK | SLIMPRO_MSG_PCC_SUBSPACE,
+		       &generic_comm_base->signature);
+
+	/* Write to the shared command region */
+	writew_relaxed(SLIMPRO_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 */
+	memcpy(ptr, msg, sizeof(struct slimpro_resp_msg));
+
+	/* 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;
+	}
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	if (!wait_for_completion_timeout(&ctx->rd_complete,
+					 usecs_to_jiffies(ctx->usecs_lat))) {
+		spin_lock_irqsave(&ctx->lock, flags);
+		dev_err(ctx->dev, "Mailbox operation timed out\n");
+		rc = -EIO;
+		goto err;
+	}
+	spin_lock_irqsave(&ctx->lock, flags);
+
+	/* Check for invalid data or no device */
+	if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
+	    ctx->sync_msg.msg == 0xffffffff) {
+		rc = -ENODEV;
+		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;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	return rc;
+}
+
+static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
+{
+	unsigned long flags;
+	int rc;
+
+	spin_lock_irqsave(&ctx->lock, flags);
+	if (ctx->resp_pending) {
+		spin_unlock_irqrestore(&ctx->lock, flags);
+		return -EAGAIN;
+	}
+
+	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;
+	}
+	spin_unlock_irqrestore(&ctx->lock, flags);
+
+	if (!wait_for_completion_timeout(&ctx->rd_complete,
+					 msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) {
+		spin_lock_irqsave(&ctx->lock, flags);
+		dev_err(ctx->dev, "Mailbox operation timed out\n");
+		rc = -EIO;
+		goto err;
+	}
+	spin_lock_irqsave(&ctx->lock, flags);
+
+	/* Check for invalid data or no device */
+	if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
+	    ctx->sync_msg.msg == 0xffffffff) {
+		rc = -ENODEV;
+		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;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	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] = SLIMPRO_SENSOR_READ_MSG;
+	msg[1] = SLIMPRO_SENSOR_READ_ENCODE_ADDR(addr);
+	msg[2] = 0;
+
+	if (ACPI_COMPANION(ctx->dev))
+		rc = xgene_hwmon_pcc_rd(ctx, msg);
+	else
+		rc = xgene_hwmon_rd(ctx, msg);
+	if (rc < 0) {
+		/* To support compatibility with firmware, return 0 */
+		dev_err(ctx->dev, "SLIMpro register 0x%02X read error %d\n",
+			addr, rc);
+		*data = 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] = SLIMPRO_TPC_ENCODE_MSG(SLIMPRO_PWRMGMT_SUBTYPE_TPC,
+					SLIMPRO_TPC_GET_ALARM, 0);
+	msg[1] = 0;
+	msg[2] = 0;
+
+	rc = xgene_hwmon_pcc_rd(ctx, msg);
+	if (rc < 0) {
+		dev_err(ctx->dev, "PCC Alarm read error %d\n", rc);
+		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)
+{
+	return xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, val);
+}
+
+static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	return xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, val);
+}
+
+static int xgene_hwmon_get_soc_power(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	u32 pmd_vrm_power;
+	u32 io_vrm_power;
+	int rc;
+
+	rc = xgene_hwmon_get_cpu_pwr(ctx, &pmd_vrm_power);
+	if (rc < 0)
+		return rc;
+
+	rc = xgene_hwmon_get_io_pwr(ctx, &io_vrm_power);
+	if (rc < 0)
+		return rc;
+
+	*val = pmd_vrm_power + WATT_TO_mWATT(io_vrm_power);
+
+	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 const char * const sensor_temp_input_names[] = {
+	[SOC_TEMP] = "SoC Temperature",
+};
+
+static const char * const sensor_pwr_input_names[] = {
+	[CPU_POWER] = "CPU's power",
+	[IO_POWER] = "IO's power",
+	[SOC_POWER] = "SoC power"
+};
+
+static ssize_t xgene_hwmon_show_name(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "APM X-Gene\n");
+}
+
+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);
+	u32 val;
+	int rc;
+
+	rc = xgene_hwmon_get_temp(ctx, &val);
+	if (rc < 0)
+		return rc;
+
+	val = CELSIUS_TO_mCELSIUS(val);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t xgene_hwmon_show_temp_label(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	int channel = to_sensor_dev_attr(attr)->index - 1;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			sensor_temp_input_names[channel]);
+}
+
+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_pwr_label(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	int channel = to_sensor_dev_attr(attr)->index - 1;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			sensor_pwr_input_names[channel]);
+}
+
+static ssize_t xgene_hwmon_show_pwr(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
+	int channel = to_sensor_dev_attr(attr)->index - 1;
+	u32 val;
+	int rc;
+
+	switch (channel) {
+	case CPU_POWER:
+		rc = xgene_hwmon_get_cpu_pwr(ctx, &val);
+		break;
+	case IO_POWER:
+		rc = xgene_hwmon_get_io_pwr(ctx, &val);
+		break;
+	case SOC_POWER:
+		rc = xgene_hwmon_get_soc_power(ctx, &val);
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+	if (rc < 0)
+		return rc;
+
+	if (channel == IO_POWER)
+		val = WATT_TO_uWATT(val);
+	else
+		val = mWATT_TO_uWATT(val);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+/* Chip name, required by hwmon */
+static DEVICE_ATTR(name, S_IRUGO, xgene_hwmon_show_name, NULL);
+
+#define SENSOR_ATTR_TEMP(index) \
+	static SENSOR_DEVICE_ATTR(temp##index##_label, S_IRUGO, \
+				  xgene_hwmon_show_temp_label, NULL, index); \
+	static SENSOR_DEVICE_ATTR(temp##index##_input, S_IRUGO, \
+				  xgene_hwmon_show_temp, NULL, index); \
+	static SENSOR_DEVICE_ATTR(temp##index##_critical_alarm, S_IRUGO, \
+				  xgene_hwmon_show_temp_critical_alarm, NULL, \
+				  index);
+#define SENSOR_ATTR_PWR(index) \
+	static SENSOR_DEVICE_ATTR(power##index##_label, S_IRUGO, \
+				  xgene_hwmon_show_pwr_label, NULL, index); \
+	static SENSOR_DEVICE_ATTR(power##index##_input, S_IRUGO, \
+				  xgene_hwmon_show_pwr, NULL, index);
+
+#define APM_TEMP_SENSOR_ATTR(index) \
+	&sensor_dev_attr_temp##index##_input.dev_attr.attr, \
+	&sensor_dev_attr_temp##index##_label.dev_attr.attr, \
+	&sensor_dev_attr_temp##index##_critical_alarm.dev_attr.attr
+#define APM_PWR_SENSOR_ATTR(index) \
+	&sensor_dev_attr_power##index##_input.dev_attr.attr, \
+	&sensor_dev_attr_power##index##_label.dev_attr.attr
+
+SENSOR_ATTR_TEMP(1); /* SoC temperature */
+SENSOR_ATTR_PWR(1);  /* CPU power */
+SENSOR_ATTR_PWR(2);  /* IO power */
+SENSOR_ATTR_PWR(3);  /* SoC power */
+
+static struct attribute *xgene_hwmon_attributes[] = {
+	&dev_attr_name.attr,
+	APM_TEMP_SENSOR_ATTR(1),
+	APM_PWR_SENSOR_ATTR(1),
+	APM_PWR_SENSOR_ATTR(2),
+	APM_PWR_SENSOR_ATTR(3),
+	NULL,
+};
+
+static const struct attribute_group xgene_hwmon_attr_group = {
+	.attrs	= xgene_hwmon_attributes,
+};
+
+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), "temp%d_critical_alarm", SOC_TEMP + 1);
+	sysfs_notify(&ctx->dev->kobj, NULL, name);
+	dev_alert(ctx->dev, "SoC temperature alarm at %d degree\n",
+		  amsg->param1);
+	return 0;
+}
+
+static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx,
+				       struct slimpro_resp_msg *amsg)
+{
+	switch (SLIMPRO_MSG_SUBTYPE(amsg->msg)) {
+	case SLIMPRO_PWRMGMT_SUBTYPE_TPC:
+		switch (SLIMPRO_TPC_CMD(amsg->msg)) {
+		case SLIMPRO_TPC_ALARM:
+			xgene_hwmon_tpc_alarm(ctx, amsg);
+			break;
+		default:
+			dev_warn(ctx->dev,
+				 "Un-supported TPC message received 0x%08X\n",
+				 amsg->msg);
+			break;
+		}
+		break;
+	default:
+		dev_warn(ctx->dev, "Un-supported message received 0x%08X\n",
+			 amsg->msg);
+		break;
+	}
+}
+
+/*
+ * 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->lock)) {
+		/*
+		 * If PCC, send a consumer command to Platform to get info
+		 * If Slimpro Mailbox, get message from specific FIFO
+		 */
+		if (ACPI_COMPANION(ctx->dev)) {
+			ret = xgene_hwmon_get_notification_msg(ctx,
+							       (u32 *)&amsg);
+			if (ret < 0)
+				continue;
+		}
+		switch (SLIMPRO_MSG_TYPE(amsg.msg)) {
+		case SLIMPRO_MSG_TYPE_PWRMGMT_ID:
+			xgene_hwmon_process_pwrmsg(ctx, &amsg);
+			break;
+		default:
+			dev_warn(ctx->dev,
+				 "Invalid mailbox msg received 0x%08X 0x%08X 0x%08X\n",
+				 amsg.msg, amsg.param1, amsg.param2);
+			break;
+		}
+	}
+}
+
+/*
+ * This function is called when the SLIMpro/PCC 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 acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct slimpro_resp_msg amsg;
+
+	/* If PCC mailbox controller, get msg from shared memory */
+	if (ACPI_COMPANION(ctx->dev)) {
+		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 &&
+	    ((SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_ERR_ID) ||
+	     (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_DBG_ID &&
+	      SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_DBG_SUBTYPE_SENSOR_READ) ||
+	     (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_PWRMGMT_ID &&
+	      SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_PWRMGMT_SUBTYPE_TPC &&
+	      SLIMPRO_TPC_CMD(((u32 *) msg)[0]) == SLIMPRO_TPC_ALARM))) {
+		if (ACPI_COMPANION(ctx->dev)) {
+			/* Check if platform completes command */
+			if (!xgene_word_tst_and_clr(&generic_comm_base->status,
+						    PCCS_CMD_COMPLETE))
+				goto notify;
+		}
+		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;
+	}
+
+	/*
+	 * If PCC, 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.
+	 */
+notify:
+	if (ACPI_COMPANION(ctx->dev)) {
+		/* Check and clear Platform Notification bit */
+		if (!xgene_word_tst_and_clr(&generic_comm_base->status,
+					    PCCS_PLATFORM_NOTIFICATION))
+			return;
+	} else {
+		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->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 __init 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->rx_callback = xgene_hwmon_rx_cb;
+	cl->tx_done = xgene_hwmon_tx_done;
+	cl->tx_block = false;
+	cl->tx_tout = MBOX_OP_TIMEOUTMS;
+	cl->knows_txdone = false;
+	if (!ACPI_COMPANION(cl->dev)) {
+		ctx->mbox_chan = mbox_request_channel(cl, MBOX_HWMON_INDEX);
+		if (IS_ERR(ctx->mbox_chan)) {
+			dev_err(&pdev->dev,
+				"SLIMpro mailbox channel request failed\n");
+			return PTR_ERR(ctx->mbox_chan);
+		}
+	} else {
+		struct acpi_pcct_hw_reduced *cppc_ss;
+
+		ctx->mbox_chan =
+			pcc_mbox_request_channel(cl, SLIMPRO_MSG_PCC_SUBSPACE);
+		if (IS_ERR(ctx->mbox_chan)) {
+			dev_err(&pdev->dev,
+				"PPC channel request failed\n");
+			return PTR_ERR(ctx->mbox_chan);
+		}
+
+		/*
+		 * 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->lock);
+
+	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);
+
+	/* Hook up sysfs for sensor monitor */
+	rc = sysfs_create_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
+	if (rc) {
+		dev_err(&pdev->dev, "Fail to create sysfs\n");
+		goto out_kfifo_free;
+	}
+
+	ctx->hwmon_dev = hwmon_device_register(ctx->dev);
+	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:
+	sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
+out_kfifo_free:
+	kfifo_free(&ctx->async_msg_fifo);
+out_mbox_free:
+	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);
+
+	hwmon_device_unregister(ctx->hwmon_dev);
+	sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
+	kfifo_free(&ctx->async_msg_fifo);
+	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 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 a055a5d..672c0ce 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 ae4a173..7a59159 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 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation

[Rob Herring]

On Mon, May 16, 2016 at 09:17:25AM -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
> 
> 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..49a482e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
> @@ -0,0 +1,14 @@
> +APM X-Gene hwmon driver
> +
> +Hwmon driver accesses sensors over the "SLIMpro" mailbox.

DT bindings describe h/w, not driver data. I'm not sure this belongs in 
DT and perhaps the devices for the mailbox should be created by the 
mailbox driver.

> +
> +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.

When do you expect this to be different mailbox numbers? 

> +
> +Example :
> +	hwmonslimpro {
> +		compatible = "apm,xgene-slimpro-hwmon";
> +		mboxes = <&mailbox 7>;
> +	};
> -- 
> 1.9.1
> 

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

[Hoan Tran]

Hi Rob,

Thanks for your review !

On Mon, May 23, 2016 at 1:30 PM, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, May 16, 2016 at 09:17:25AM -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
> >
> > 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..49a482e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
> > @@ -0,0 +1,14 @@
> > +APM X-Gene hwmon driver
> > +
> > +Hwmon driver accesses sensors over the "SLIMpro" mailbox.
>
> DT bindings describe h/w, not driver data.
How about this description: "APM X-Gene SOC sensors are accessed over
the "SLIMpro" mailbox" ?
> I'm not sure this belongs in
> DT and perhaps the devices for the mailbox should be created by the
> mailbox driver.
I don't think the current mailbox supports it.
>
> > +
> > +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.
>
> When do you expect this to be different mailbox numbers?
No, this number is not changed. This "mboxes" property is used and
required by mailbox.c when hwmon driver requests a mailbox channel

Thanks and Regards
Hoan
>
>
> > +
> > +Example :
> > +     hwmonslimpro {
> > +             compatible = "apm,xgene-slimpro-hwmon";
> > +             mboxes = <&mailbox 7>;
> > +     };
> > --
> > 1.9.1
> >

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

[Rob Herring]

On Mon, May 23, 2016 at 06:01:14PM -0700, Hoan Tran wrote:
> Hi Rob,
> 
> Thanks for your review !
> 
> On Mon, May 23, 2016 at 1:30 PM, Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, May 16, 2016 at 09:17:25AM -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
> > >
> > > 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..49a482e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
> > > @@ -0,0 +1,14 @@
> > > +APM X-Gene hwmon driver
> > > +
> > > +Hwmon driver accesses sensors over the "SLIMpro" mailbox.
> >
> > DT bindings describe h/w, not driver data.
> How about this description: "APM X-Gene SOC sensors are accessed over
> the "SLIMpro" mailbox" ?
> > I'm not sure this belongs in
> > DT and perhaps the devices for the mailbox should be created by the
> > mailbox driver.
> I don't think the current mailbox supports it.

Then add it. The mailbox binding is really only needed when other h/w 
blocks use a mailbox. As this is purely a firmware interface then the 
mailbox driver can create any devices it needs. The devices don't have 
to be in DT to be created.

Rob

Re: [2/3] hwmon: xgene: Adds hwmon driver

[Guenter Roeck]

On Mon, May 16, 2016 at 09:17:26AM -0700, hotran wrote:
> This patch adds hardware temperature and power reading support for
> APM X-Gene SoC's using the mailbox communication interface.
> 
Please drop the "'".

> 
> Signed-off-by: Hoan Tran <hotran@apm.com>
> ---
>  Documentation/hwmon/xgene-hwmon |  32 ++
>  drivers/hwmon/Kconfig           |   7 +
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/xgene-hwmon.c     | 801 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 841 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..040a7f2
> --- /dev/null
> +++ b/Documentation/hwmon/xgene-hwmon
> @@ -0,0 +1,32 @@
> +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's using the mailbox communication interface.
> 
No "'".
> 
> +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
> +    - Alarm when high/over temperature occurs
> +  * Power
> +    - CPU power in uW
> +    - IO power in uW
> +    - CPU and IO power in uW
> +
> +sysfs-Interface
> +---------------
> +
> +temp1_input - SoC on-die temperature (milli-degree C)
> +temp1_critical_alarm - An 1 would indicates on-die temperature exceeded threshold
> +power1_input - CPU power in (uW)
> +power2_input - IO power in (uW)
> +power3_input - CPU and IO power in (uW)
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5c2d13a..91c3056 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1776,6 +1776,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 58cc3ac..668d0f1 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -161,6 +161,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..bc4ad29
> --- /dev/null
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -0,0 +1,801 @@
> +/*
> + * 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's total power (uW)
> + *  - Retrieve IO's total power (uW)
 
Please drop the "'s".

> + *  - Retrieve SoC 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_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <acpi/cppc_acpi.h>
> +
> +/* SLIMpro message defines */
> +#define SLIMPRO_MSG_TYPE_DBG_ID		0
> +#define SLIMPRO_MSG_TYPE_ERR_ID		7
> +#define SLIMPRO_MSG_TYPE_PWRMGMT_ID	9
> +
> +#define SLIMPRO_MSG_TYPE(v)		(((v) & 0xF0000000) >> 28)
> +#define SLIMPRO_MSG_TYPE_SET(v)		(((v) << 28) & 0xF0000000)
> +#define SLIMPRO_MSG_SUBTYPE(v)		(((v) & 0x0F000000) >> 24)
> +#define SLIMPRO_MSG_SUBTYPE_SET(v)	(((v) << 24) & 0x0F000000)
> +
> +#define SLIMPRO_DBG_SUBTYPE_SENSOR_READ	4
> +#define SLIMPRO_SENSOR_READ_MSG		0x04FFE902
> +#define SLIMPRO_SENSOR_READ_ENCODE_ADDR(a) \
> +	((a) & 0x000FFFFF)
> +#define PMD_PWR_MW_REG			0x26
> +#define SOC_PWR_REG			0x21
> +#define SOC_TEMP_REG			0x10
> +
> +#define SLIMPRO_PWRMGMT_SUBTYPE_TPC	1
> +#define SLIMPRO_TPC_ALARM		2
> +#define SLIMPRO_TPC_GET_ALARM		3
> +#define SLIMPRO_TPC_CMD(v)		(((v) & 0x00FF0000) >> 16)
> +#define SLIMPRO_TPC_CMD_SET(v)		(((v) << 16) & 0x00FF0000)
> +#define SLIMPRO_TPC_ENCODE_MSG(hndl, cmd, type) \
> +	(SLIMPRO_MSG_TYPE_SET(SLIMPRO_MSG_TYPE_PWRMGMT_ID) | \
> +	SLIMPRO_MSG_SUBTYPE_SET(hndl) | \
> +	SLIMPRO_TPC_CMD_SET(cmd) | \
> +	type)
> +

Do you _have_ to use such long defines ? This causes lots of "line too long"
checkpatch warnings, which would really be easy to avoid with shorter defines.

> +/* PCC defines */
> +#define SLIMPRO_MSG_PCC_SUBSPACE	7
> +#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_HWMON_INDEX		0
> +#define MBOX_OP_TIMEOUTMS		1000
> +
> +#define SOC_TEMP			0
> +#define CPU_POWER			0
> +#define IO_POWER			1
> +#define SOC_POWER			2
> +

Are those random defines, to be used for index values ?
If so, how about enums ?

> +#define WATT_TO_mWATT(x)		((x) * 1000)
> +#define mWATT_TO_uWATT(x)		((x) * 1000)
> +#define WATT_TO_uWATT(x)		((x) * 1000000)
> +#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;
> +
> +	spinlock_t		lock;
> +	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_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;
> +	unsigned long flags;
> +	u16 val;
> +	int rc;
> +
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	if (ctx->resp_pending) {
> +		spin_unlock_irqrestore(&ctx->lock, flags);
> +		return -EAGAIN;
> +	}
> +
> +	init_completion(&ctx->rd_complete);
> +	ctx->resp_pending = true;
> +
> +	/* Write signature for subspace */
> +	writel_relaxed(PCC_SIGNATURE_MASK | SLIMPRO_MSG_PCC_SUBSPACE,
> +		       &generic_comm_base->signature);
> +
> +	/* Write to the shared command region */
> +	writew_relaxed(SLIMPRO_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 */
> +	memcpy(ptr, msg, sizeof(struct slimpro_resp_msg));
> +
> +	/* 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;
> +	}
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +	if (!wait_for_completion_timeout(&ctx->rd_complete,
> +					 usecs_to_jiffies(ctx->usecs_lat))) {
> +		spin_lock_irqsave(&ctx->lock, flags);
> +		dev_err(ctx->dev, "Mailbox operation timed out\n");
> +		rc = -EIO;

	-ETIMEDOUT ?

> +		goto err;
> +	}
> +	spin_lock_irqsave(&ctx->lock, flags);
> +
> +	/* Check for invalid data or no device */
> +	if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
> +	    ctx->sync_msg.msg == 0xffffffff) {
> +		rc = -ENODEV;

Two different errors ?

> +		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;
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +	return rc;
> +}
> +
> +static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
> +{
> +	unsigned long flags;
> +	int rc;
> +
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	if (ctx->resp_pending) {
> +		spin_unlock_irqrestore(&ctx->lock, flags);
> +		return -EAGAIN;
> +	}
> +
> +	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;
> +	}
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +
> +	if (!wait_for_completion_timeout(&ctx->rd_complete,
> +					 msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) {
> +		spin_lock_irqsave(&ctx->lock, flags);
> +		dev_err(ctx->dev, "Mailbox operation timed out\n");
> +		rc = -EIO;

	-ETIMEDOUT ?

> +		goto err;
> +	}
> +	spin_lock_irqsave(&ctx->lock, flags);
> +
> +	/* Check for invalid data or no device */
> +	if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
> +	    ctx->sync_msg.msg == 0xffffffff) {
> +		rc = -ENODEV;

Isn't that two different errors ?

> +		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;
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +	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] = SLIMPRO_SENSOR_READ_MSG;
> +	msg[1] = SLIMPRO_SENSOR_READ_ENCODE_ADDR(addr);
> +	msg[2] = 0;
> +
Does the msg endianness always match the CPU endianness ?
Probably yes, just want to make sure.

> +	if (ACPI_COMPANION(ctx->dev))
> +		rc = xgene_hwmon_pcc_rd(ctx, msg);
> +	else
> +		rc = xgene_hwmon_rd(ctx, msg);
> +	if (rc < 0) {
> +		/* To support compatibility with firmware, return 0 */
> +		dev_err(ctx->dev, "SLIMpro register 0x%02X read error %d\n",
> +			addr, rc);

Is this error mesage necessary ?

> +		*data = 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] = SLIMPRO_TPC_ENCODE_MSG(SLIMPRO_PWRMGMT_SUBTYPE_TPC,
> +					SLIMPRO_TPC_GET_ALARM, 0);
> +	msg[1] = 0;
> +	msg[2] = 0;
> +
> +	rc = xgene_hwmon_pcc_rd(ctx, msg);
> +	if (rc < 0) {
> +		dev_err(ctx->dev, "PCC Alarm read error %d\n", rc);

Is this error message necessary ?

> +		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)
> +{
> +	return xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, val);
> +}
> +
> +static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
> +{
> +	return xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, val);
> +}
> +
> +static int xgene_hwmon_get_soc_power(struct xgene_hwmon_dev *ctx, u32 *val)
> +{
> +	u32 pmd_vrm_power;
> +	u32 io_vrm_power;
> +	int rc;
> +
> +	rc = xgene_hwmon_get_cpu_pwr(ctx, &pmd_vrm_power);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = xgene_hwmon_get_io_pwr(ctx, &io_vrm_power);
> +	if (rc < 0)
> +		return rc;
> +
> +	*val = pmd_vrm_power + WATT_TO_mWATT(io_vrm_power);
> +

So the SoC power is just adding CPU and IO power, ie it reports the sum
of both ?  Please drop. Such calculations should be done in user space
if wanted.

> +	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 const char * const sensor_temp_input_names[] = {
> +	[SOC_TEMP] = "SoC Temperature",
> +};
> +
> +static const char * const sensor_pwr_input_names[] = {
> +	[CPU_POWER] = "CPU's power",
> +	[IO_POWER] = "IO's power",

Please drop the "'s".

> +	[SOC_POWER] = "SoC power"
> +};
> +
> +static ssize_t xgene_hwmon_show_name(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "APM X-Gene\n");
> +}
> +
> +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);
> +	u32 val;
> +	int rc;
> +
> +	rc = xgene_hwmon_get_temp(ctx, &val);
> +	if (rc < 0)
> +		return rc;
> +
> +	val = CELSIUS_TO_mCELSIUS(val);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);

val is u32 (is it reported by the chip as unsigned ?)

> +}
> +
> +static ssize_t xgene_hwmon_show_temp_label(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	int channel = to_sensor_dev_attr(attr)->index - 1;
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			sensor_temp_input_names[channel]);
> +}
> +
> +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_pwr_label(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	int channel = to_sensor_dev_attr(attr)->index - 1;
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			sensor_pwr_input_names[channel]);
> +}
> +
> +static ssize_t xgene_hwmon_show_pwr(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
> +	int channel = to_sensor_dev_attr(attr)->index - 1;
> +	u32 val;
> +	int rc;
> +
> +	switch (channel) {
> +	case CPU_POWER:
> +		rc = xgene_hwmon_get_cpu_pwr(ctx, &val);
> +		break;
> +	case IO_POWER:
> +		rc = xgene_hwmon_get_io_pwr(ctx, &val);
> +		break;
> +	case SOC_POWER:
> +		rc = xgene_hwmon_get_soc_power(ctx, &val);
> +		break;

Those are not the index values specified. Please use those constants
in the attribute declaration.

Overall, it would probably be easier to just have two separate
functions for CPU an IO power.

> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +	if (rc < 0)
> +		return rc;
> +
> +	if (channel == IO_POWER)
> +		val = WATT_TO_uWATT(val);

What is the maximum possible reported IO power ? I understand that
the maximum supported value with the current code is 4,294 kW, but
if the hardware can report more than that it would be prudent to
use an unsigned long for the value.

> +	else
> +		val = mWATT_TO_uWATT(val);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);

val is u32.

> +}
> +
> +/* Chip name, required by hwmon */
> +static DEVICE_ATTR(name, S_IRUGO, xgene_hwmon_show_name, NULL);
> +

Unnecesary - see below.

> +#define SENSOR_ATTR_TEMP(index) \
> +	static SENSOR_DEVICE_ATTR(temp##index##_label, S_IRUGO, \
> +				  xgene_hwmon_show_temp_label, NULL, index); \
> +	static SENSOR_DEVICE_ATTR(temp##index##_input, S_IRUGO, \
> +				  xgene_hwmon_show_temp, NULL, index); \
> +	static SENSOR_DEVICE_ATTR(temp##index##_critical_alarm, S_IRUGO, \
> +				  xgene_hwmon_show_temp_critical_alarm, NULL, \
> +				  index);
> +#define SENSOR_ATTR_PWR(index) \
> +	static SENSOR_DEVICE_ATTR(power##index##_label, S_IRUGO, \
> +				  xgene_hwmon_show_pwr_label, NULL, index); \
> +	static SENSOR_DEVICE_ATTR(power##index##_input, S_IRUGO, \
> +				  xgene_hwmon_show_pwr, NULL, index);
> +
> +#define APM_TEMP_SENSOR_ATTR(index) \
> +	&sensor_dev_attr_temp##index##_input.dev_attr.attr, \
> +	&sensor_dev_attr_temp##index##_label.dev_attr.attr, \
> +	&sensor_dev_attr_temp##index##_critical_alarm.dev_attr.attr
> +#define APM_PWR_SENSOR_ATTR(index) \
> +	&sensor_dev_attr_power##index##_input.dev_attr.attr, \
> +	&sensor_dev_attr_power##index##_label.dev_attr.attr
> +
> +SENSOR_ATTR_TEMP(1); /* SoC temperature */
> +SENSOR_ATTR_PWR(1);  /* CPU power */
> +SENSOR_ATTR_PWR(2);  /* IO power */
> +SENSOR_ATTR_PWR(3);  /* SoC power */
> +
> +static struct attribute *xgene_hwmon_attributes[] = {
> +	&dev_attr_name.attr,
> +	APM_TEMP_SENSOR_ATTR(1),
> +	APM_PWR_SENSOR_ATTR(1),
> +	APM_PWR_SENSOR_ATTR(2),
> +	APM_PWR_SENSOR_ATTR(3),

Please drop the macros and write explicit code.

'index' values are always accessed with '- 1'. Please start the index with 0
and drop the '- 1'.

> +	NULL,
> +};
> +
> +static const struct attribute_group xgene_hwmon_attr_group = {
> +	.attrs	= xgene_hwmon_attributes,
> +};
> +
> +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), "temp%d_critical_alarm", SOC_TEMP + 1);
> +	sysfs_notify(&ctx->dev->kobj, NULL, name);
> +	dev_alert(ctx->dev, "SoC temperature alarm at %d degree\n",
> +		  amsg->param1);

Please drop this message. That is what the notification is for.
User space can then decide if it wants to log or not.

> +	return 0;
> +}
> +
> +static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx,
> +				       struct slimpro_resp_msg *amsg)
> +{
> +	switch (SLIMPRO_MSG_SUBTYPE(amsg->msg)) {
> +	case SLIMPRO_PWRMGMT_SUBTYPE_TPC:
> +		switch (SLIMPRO_TPC_CMD(amsg->msg)) {
> +		case SLIMPRO_TPC_ALARM:
> +			xgene_hwmon_tpc_alarm(ctx, amsg);
> +			break;
> +		default:
> +			dev_warn(ctx->dev,
> +				 "Un-supported TPC message received 0x%08X\n",
> +				 amsg->msg);
> +			break;
> +		}
> +		break;
> +	default:
> +		dev_warn(ctx->dev, "Un-supported message received 0x%08X\n",
> +			 amsg->msg);

Are those messages necessary ? Can they clog the log ?

> +		break;
> +	}
> +}
> +
> +/*
> + * 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->lock)) {
> +		/*
> +		 * If PCC, send a consumer command to Platform to get info
> +		 * If Slimpro Mailbox, get message from specific FIFO
> +		 */
> +		if (ACPI_COMPANION(ctx->dev)) {
> +			ret = xgene_hwmon_get_notification_msg(ctx,
> +							       (u32 *)&amsg);
> +			if (ret < 0)
> +				continue;
> +		}
> +		switch (SLIMPRO_MSG_TYPE(amsg.msg)) {
> +		case SLIMPRO_MSG_TYPE_PWRMGMT_ID:
> +			xgene_hwmon_process_pwrmsg(ctx, &amsg);
> +			break;
> +		default:
> +			dev_warn(ctx->dev,
> +				 "Invalid mailbox msg received 0x%08X 0x%08X 0x%08X\n",
> +				 amsg.msg, amsg.param1, amsg.param2);

Is this message necessary ? I am concerned about the logging noise it might
create.

> +			break;
> +		}
> +	}
> +}
> +
> +/*
> + * This function is called when the SLIMpro/PCC 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 acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
> +	struct slimpro_resp_msg amsg;
> +
> +	/* If PCC mailbox controller, get msg from shared memory */
> +	if (ACPI_COMPANION(ctx->dev)) {
> +		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 &&
> +	    ((SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_ERR_ID) ||
> +	     (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_DBG_ID &&
> +	      SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_DBG_SUBTYPE_SENSOR_READ) ||
> +	     (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_PWRMGMT_ID &&
> +	      SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_PWRMGMT_SUBTYPE_TPC &&
> +	      SLIMPRO_TPC_CMD(((u32 *) msg)[0]) == SLIMPRO_TPC_ALARM))) {
> +		if (ACPI_COMPANION(ctx->dev)) {
> +			/* Check if platform completes command */
> +			if (!xgene_word_tst_and_clr(&generic_comm_base->status,
> +						    PCCS_CMD_COMPLETE))
> +				goto notify;

How about reversing the logic here and dropping the goto ?

Overall, I have to say that the code is quite complex due to the repeated
checks for ACPI. I am close to suggest having two separate drivers,
one for ACPI and one for non-ACPI. Any chance to separate ACPI vs. non-ACPI
code a bit better ?

> +		}
> +		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;
> +	}
> +
> +	/*
> +	 * If PCC, 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.
> +	 */
> +notify:
> +	if (ACPI_COMPANION(ctx->dev)) {
> +		/* Check and clear Platform Notification bit */
> +		if (!xgene_word_tst_and_clr(&generic_comm_base->status,
> +					    PCCS_PLATFORM_NOTIFICATION))
> +			return;
> +	} else {
> +		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->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 __init 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->rx_callback = xgene_hwmon_rx_cb;
> +	cl->tx_done = xgene_hwmon_tx_done;
> +	cl->tx_block = false;
> +	cl->tx_tout = MBOX_OP_TIMEOUTMS;
> +	cl->knows_txdone = false;
> +	if (!ACPI_COMPANION(cl->dev)) {
> +		ctx->mbox_chan = mbox_request_channel(cl, MBOX_HWMON_INDEX);
> +		if (IS_ERR(ctx->mbox_chan)) {
> +			dev_err(&pdev->dev,
> +				"SLIMpro mailbox channel request failed\n");
> +			return PTR_ERR(ctx->mbox_chan);
> +		}
> +	} else {
> +		struct acpi_pcct_hw_reduced *cppc_ss;
> +
> +		ctx->mbox_chan =
> +			pcc_mbox_request_channel(cl, SLIMPRO_MSG_PCC_SUBSPACE);
> +		if (IS_ERR(ctx->mbox_chan)) {
> +			dev_err(&pdev->dev,
> +				"PPC channel request failed\n");
> +			return PTR_ERR(ctx->mbox_chan);
> +		}
> +
> +		/*
> +		 * 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->lock);
> +
> +	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);
> +
> +	/* Hook up sysfs for sensor monitor */
> +	rc = sysfs_create_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Fail to create sysfs\n");
> +		goto out_kfifo_free;
> +	}
> +
> +	ctx->hwmon_dev = hwmon_device_register(ctx->dev);
> +	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;
> +	}

Please use hwmon_device_register_with_groups().

> +
> +	dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
> +
> +	return rc;
> +
> +out:
> +	sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
> +out_kfifo_free:
> +	kfifo_free(&ctx->async_msg_fifo);
> +out_mbox_free:
> +	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);
> +
> +	hwmon_device_unregister(ctx->hwmon_dev);
> +	sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
> +	kfifo_free(&ctx->async_msg_fifo);
> +	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: [2/3] hwmon: xgene: Adds hwmon driver

[Hoan Tran]

Hi Guenter,

Thanks for your review !

On Sun, May 29, 2016 at 10:25 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, May 16, 2016 at 09:17:26AM -0700, hotran wrote:
>> This patch adds hardware temperature and power reading support for
>> APM X-Gene SoC's using the mailbox communication interface.
>>
> Please drop the "'".

Yes, will remove it.

>
>>
>> Signed-off-by: Hoan Tran <hotran@apm.com>
>> ---
>>  Documentation/hwmon/xgene-hwmon |  32 ++
>>  drivers/hwmon/Kconfig           |   7 +
>>  drivers/hwmon/Makefile          |   1 +
>>  drivers/hwmon/xgene-hwmon.c     | 801 ++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 841 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..040a7f2
>> --- /dev/null
>> +++ b/Documentation/hwmon/xgene-hwmon
>> @@ -0,0 +1,32 @@
>> +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's using the mailbox communication interface.
>>
> No "'".

Yes,

>>
>> +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
>> +    - Alarm when high/over temperature occurs
>> +  * Power
>> +    - CPU power in uW
>> +    - IO power in uW
>> +    - CPU and IO power in uW
>> +
>> +sysfs-Interface
>> +---------------
>> +
>> +temp1_input - SoC on-die temperature (milli-degree C)
>> +temp1_critical_alarm - An 1 would indicates on-die temperature exceeded threshold
>> +power1_input - CPU power in (uW)
>> +power2_input - IO power in (uW)
>> +power3_input - CPU and IO power in (uW)
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 5c2d13a..91c3056 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1776,6 +1776,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 58cc3ac..668d0f1 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -161,6 +161,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..bc4ad29
>> --- /dev/null
>> +++ b/drivers/hwmon/xgene-hwmon.c
>> @@ -0,0 +1,801 @@
>> +/*
>> + * 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's total power (uW)
>> + *  - Retrieve IO's total power (uW)
>
> Please drop the "'s".

Yes,

>
>> + *  - Retrieve SoC 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_client.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <acpi/cppc_acpi.h>
>> +
>> +/* SLIMpro message defines */
>> +#define SLIMPRO_MSG_TYPE_DBG_ID              0
>> +#define SLIMPRO_MSG_TYPE_ERR_ID              7
>> +#define SLIMPRO_MSG_TYPE_PWRMGMT_ID  9
>> +
>> +#define SLIMPRO_MSG_TYPE(v)          (((v) & 0xF0000000) >> 28)
>> +#define SLIMPRO_MSG_TYPE_SET(v)              (((v) << 28) & 0xF0000000)
>> +#define SLIMPRO_MSG_SUBTYPE(v)               (((v) & 0x0F000000) >> 24)
>> +#define SLIMPRO_MSG_SUBTYPE_SET(v)   (((v) << 24) & 0x0F000000)
>> +
>> +#define SLIMPRO_DBG_SUBTYPE_SENSOR_READ      4
>> +#define SLIMPRO_SENSOR_READ_MSG              0x04FFE902
>> +#define SLIMPRO_SENSOR_READ_ENCODE_ADDR(a) \
>> +     ((a) & 0x000FFFFF)
>> +#define PMD_PWR_MW_REG                       0x26
>> +#define SOC_PWR_REG                  0x21
>> +#define SOC_TEMP_REG                 0x10
>> +
>> +#define SLIMPRO_PWRMGMT_SUBTYPE_TPC  1
>> +#define SLIMPRO_TPC_ALARM            2
>> +#define SLIMPRO_TPC_GET_ALARM                3
>> +#define SLIMPRO_TPC_CMD(v)           (((v) & 0x00FF0000) >> 16)
>> +#define SLIMPRO_TPC_CMD_SET(v)               (((v) << 16) & 0x00FF0000)
>> +#define SLIMPRO_TPC_ENCODE_MSG(hndl, cmd, type) \
>> +     (SLIMPRO_MSG_TYPE_SET(SLIMPRO_MSG_TYPE_PWRMGMT_ID) | \
>> +     SLIMPRO_MSG_SUBTYPE_SET(hndl) | \
>> +     SLIMPRO_TPC_CMD_SET(cmd) | \
>> +     type)
>> +
>
> Do you _have_ to use such long defines ? This causes lots of "line too long"
> checkpatch warnings, which would really be easy to avoid with shorter defines.

Ok, I'll shorten these defines.

>
>> +/* PCC defines */
>> +#define SLIMPRO_MSG_PCC_SUBSPACE     7
>> +#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_HWMON_INDEX             0
>> +#define MBOX_OP_TIMEOUTMS            1000
>> +
>> +#define SOC_TEMP                     0
>> +#define CPU_POWER                    0
>> +#define IO_POWER                     1
>> +#define SOC_POWER                    2
>> +
>
> Are those random defines, to be used for index values ?
> If so, how about enums ?

Yes, will move them into an enum.

>
>> +#define WATT_TO_mWATT(x)             ((x) * 1000)
>> +#define mWATT_TO_uWATT(x)            ((x) * 1000)
>> +#define WATT_TO_uWATT(x)             ((x) * 1000000)
>> +#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;
>> +
>> +     spinlock_t              lock;
>> +     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_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;
>> +     unsigned long flags;
>> +     u16 val;
>> +     int rc;
>> +
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +     if (ctx->resp_pending) {
>> +             spin_unlock_irqrestore(&ctx->lock, flags);
>> +             return -EAGAIN;
>> +     }
>> +
>> +     init_completion(&ctx->rd_complete);
>> +     ctx->resp_pending = true;
>> +
>> +     /* Write signature for subspace */
>> +     writel_relaxed(PCC_SIGNATURE_MASK | SLIMPRO_MSG_PCC_SUBSPACE,
>> +                    &generic_comm_base->signature);
>> +
>> +     /* Write to the shared command region */
>> +     writew_relaxed(SLIMPRO_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 */
>> +     memcpy(ptr, msg, sizeof(struct slimpro_resp_msg));
>> +
>> +     /* 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;
>> +     }
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +     if (!wait_for_completion_timeout(&ctx->rd_complete,
>> +                                      usecs_to_jiffies(ctx->usecs_lat))) {
>> +             spin_lock_irqsave(&ctx->lock, flags);
>> +             dev_err(ctx->dev, "Mailbox operation timed out\n");
>> +             rc = -EIO;
>
>         -ETIMEDOUT ?

OK

>
>> +             goto err;
>> +     }
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +
>> +     /* Check for invalid data or no device */
>> +     if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
>> +         ctx->sync_msg.msg == 0xffffffff) {
>> +             rc = -ENODEV;
>
> Two different errors ?

Yes, will fix it

>
>> +             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;
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +     return rc;
>> +}
>> +
>> +static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>> +{
>> +     unsigned long flags;
>> +     int rc;
>> +
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +     if (ctx->resp_pending) {
>> +             spin_unlock_irqrestore(&ctx->lock, flags);
>> +             return -EAGAIN;
>> +     }
>> +
>> +     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;
>> +     }
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +
>> +     if (!wait_for_completion_timeout(&ctx->rd_complete,
>> +                                      msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) {
>> +             spin_lock_irqsave(&ctx->lock, flags);
>> +             dev_err(ctx->dev, "Mailbox operation timed out\n");
>> +             rc = -EIO;
>
>         -ETIMEDOUT ?

OK

>
>> +             goto err;
>> +     }
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +
>> +     /* Check for invalid data or no device */
>> +     if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
>> +         ctx->sync_msg.msg == 0xffffffff) {
>> +             rc = -ENODEV;
>
> Isn't that two different errors ?

Will fix it.

>
>> +             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;
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +     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] = SLIMPRO_SENSOR_READ_MSG;
>> +     msg[1] = SLIMPRO_SENSOR_READ_ENCODE_ADDR(addr);
>> +     msg[2] = 0;
>> +
> Does the msg endianness always match the CPU endianness ?
> Probably yes, just want to make sure.

For DT, SLIMpro mailbox controller will take care of endianness.
For ACPI, maybe I missed it, it should be corrected when PCC writes
message into shared memory. Thanks

>
>> +     if (ACPI_COMPANION(ctx->dev))
>> +             rc = xgene_hwmon_pcc_rd(ctx, msg);
>> +     else
>> +             rc = xgene_hwmon_rd(ctx, msg);
>> +     if (rc < 0) {
>> +             /* To support compatibility with firmware, return 0 */
>> +             dev_err(ctx->dev, "SLIMpro register 0x%02X read error %d\n",
>> +                     addr, rc);
>
> Is this error mesage necessary ?

Will remove it.

>
>> +             *data = 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] = SLIMPRO_TPC_ENCODE_MSG(SLIMPRO_PWRMGMT_SUBTYPE_TPC,
>> +                                     SLIMPRO_TPC_GET_ALARM, 0);
>> +     msg[1] = 0;
>> +     msg[2] = 0;
>> +
>> +     rc = xgene_hwmon_pcc_rd(ctx, msg);
>> +     if (rc < 0) {
>> +             dev_err(ctx->dev, "PCC Alarm read error %d\n", rc);
>
> Is this error message necessary ?

Will remove it.

>
>> +             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)
>> +{
>> +     return xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, val);
>> +}
>> +
>> +static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
>> +{
>> +     return xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, val);
>> +}
>> +
>> +static int xgene_hwmon_get_soc_power(struct xgene_hwmon_dev *ctx, u32 *val)
>> +{
>> +     u32 pmd_vrm_power;
>> +     u32 io_vrm_power;
>> +     int rc;
>> +
>> +     rc = xgene_hwmon_get_cpu_pwr(ctx, &pmd_vrm_power);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     rc = xgene_hwmon_get_io_pwr(ctx, &io_vrm_power);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     *val = pmd_vrm_power + WATT_TO_mWATT(io_vrm_power);
>> +
>
> So the SoC power is just adding CPU and IO power, ie it reports the sum
> of both ?  Please drop. Such calculations should be done in user space
> if wanted.

Ok, will drop this SoC power.

>
>> +     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 const char * const sensor_temp_input_names[] = {
>> +     [SOC_TEMP] = "SoC Temperature",
>> +};
>> +
>> +static const char * const sensor_pwr_input_names[] = {
>> +     [CPU_POWER] = "CPU's power",
>> +     [IO_POWER] = "IO's power",
>
> Please drop the "'s".

Ok,

>
>> +     [SOC_POWER] = "SoC power"
>> +};
>> +
>> +static ssize_t xgene_hwmon_show_name(struct device *dev,
>> +                                  struct device_attribute *attr, char *buf)
>> +{
>> +     return snprintf(buf, PAGE_SIZE, "APM X-Gene\n");
>> +}
>> +
>> +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);
>> +     u32 val;
>> +     int rc;
>> +
>> +     rc = xgene_hwmon_get_temp(ctx, &val);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     val = CELSIUS_TO_mCELSIUS(val);
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", val);
>
> val is u32 (is it reported by the chip as unsigned ?)

It also give a negative temp. I'll fix it

>
>> +}
>> +
>> +static ssize_t xgene_hwmon_show_temp_label(struct device *dev,
>> +                                        struct device_attribute *attr,
>> +                                        char *buf)
>> +{
>> +     int channel = to_sensor_dev_attr(attr)->index - 1;
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%s\n",
>> +                     sensor_temp_input_names[channel]);
>> +}
>> +
>> +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_pwr_label(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       char *buf)
>> +{
>> +     int channel = to_sensor_dev_attr(attr)->index - 1;
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%s\n",
>> +                     sensor_pwr_input_names[channel]);
>> +}
>> +
>> +static ssize_t xgene_hwmon_show_pwr(struct device *dev,
>> +                                 struct device_attribute *attr,
>> +                                 char *buf)
>> +{
>> +     struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
>> +     int channel = to_sensor_dev_attr(attr)->index - 1;
>> +     u32 val;
>> +     int rc;
>> +
>> +     switch (channel) {
>> +     case CPU_POWER:
>> +             rc = xgene_hwmon_get_cpu_pwr(ctx, &val);
>> +             break;
>> +     case IO_POWER:
>> +             rc = xgene_hwmon_get_io_pwr(ctx, &val);
>> +             break;
>> +     case SOC_POWER:
>> +             rc = xgene_hwmon_get_soc_power(ctx, &val);
>> +             break;
>
> Those are not the index values specified. Please use those constants
> in the attribute declaration.
>
> Overall, it would probably be easier to just have two separate
> functions for CPU an IO power.

Yes, I'll use separate functions.

>
>> +     default:
>> +             rc = -EINVAL;
>> +             break;
>> +     }
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     if (channel == IO_POWER)
>> +             val = WATT_TO_uWATT(val);
>
> What is the maximum possible reported IO power ? I understand that
> the maximum supported value with the current code is 4,294 kW, but
> if the hardware can report more than that it would be prudent to
> use an unsigned long for the value.

This is the power of internal SoC and it could not exceed this number

>
>> +     else
>> +             val = mWATT_TO_uWATT(val);
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", val);
>
> val is u32.

Yes, change to %u.

>
>> +}
>> +
>> +/* Chip name, required by hwmon */
>> +static DEVICE_ATTR(name, S_IRUGO, xgene_hwmon_show_name, NULL);
>> +
>
> Unnecesary - see below.
>
>> +#define SENSOR_ATTR_TEMP(index) \
>> +     static SENSOR_DEVICE_ATTR(temp##index##_label, S_IRUGO, \
>> +                               xgene_hwmon_show_temp_label, NULL, index); \
>> +     static SENSOR_DEVICE_ATTR(temp##index##_input, S_IRUGO, \
>> +                               xgene_hwmon_show_temp, NULL, index); \
>> +     static SENSOR_DEVICE_ATTR(temp##index##_critical_alarm, S_IRUGO, \
>> +                               xgene_hwmon_show_temp_critical_alarm, NULL, \
>> +                               index);
>> +#define SENSOR_ATTR_PWR(index) \
>> +     static SENSOR_DEVICE_ATTR(power##index##_label, S_IRUGO, \
>> +                               xgene_hwmon_show_pwr_label, NULL, index); \
>> +     static SENSOR_DEVICE_ATTR(power##index##_input, S_IRUGO, \
>> +                               xgene_hwmon_show_pwr, NULL, index);
>> +
>> +#define APM_TEMP_SENSOR_ATTR(index) \
>> +     &sensor_dev_attr_temp##index##_input.dev_attr.attr, \
>> +     &sensor_dev_attr_temp##index##_label.dev_attr.attr, \
>> +     &sensor_dev_attr_temp##index##_critical_alarm.dev_attr.attr
>> +#define APM_PWR_SENSOR_ATTR(index) \
>> +     &sensor_dev_attr_power##index##_input.dev_attr.attr, \
>> +     &sensor_dev_attr_power##index##_label.dev_attr.attr
>> +
>> +SENSOR_ATTR_TEMP(1); /* SoC temperature */
>> +SENSOR_ATTR_PWR(1);  /* CPU power */
>> +SENSOR_ATTR_PWR(2);  /* IO power */
>> +SENSOR_ATTR_PWR(3);  /* SoC power */
>> +
>> +static struct attribute *xgene_hwmon_attributes[] = {
>> +     &dev_attr_name.attr,
>> +     APM_TEMP_SENSOR_ATTR(1),
>> +     APM_PWR_SENSOR_ATTR(1),
>> +     APM_PWR_SENSOR_ATTR(2),
>> +     APM_PWR_SENSOR_ATTR(3),
>
> Please drop the macros and write explicit code.
>
> 'index' values are always accessed with '- 1'. Please start the index with 0
> and drop the '- 1'.

Ok, change to an explicit code.

>
>> +     NULL,
>> +};
>> +
>> +static const struct attribute_group xgene_hwmon_attr_group = {
>> +     .attrs  = xgene_hwmon_attributes,
>> +};
>> +
>> +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), "temp%d_critical_alarm", SOC_TEMP + 1);
>> +     sysfs_notify(&ctx->dev->kobj, NULL, name);
>> +     dev_alert(ctx->dev, "SoC temperature alarm at %d degree\n",
>> +               amsg->param1);
>
> Please drop this message. That is what the notification is for.
> User space can then decide if it wants to log or not.
>

Ok

>> +     return 0;
>> +}
>> +
>> +static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx,
>> +                                    struct slimpro_resp_msg *amsg)
>> +{
>> +     switch (SLIMPRO_MSG_SUBTYPE(amsg->msg)) {
>> +     case SLIMPRO_PWRMGMT_SUBTYPE_TPC:
>> +             switch (SLIMPRO_TPC_CMD(amsg->msg)) {
>> +             case SLIMPRO_TPC_ALARM:
>> +                     xgene_hwmon_tpc_alarm(ctx, amsg);
>> +                     break;
>> +             default:
>> +                     dev_warn(ctx->dev,
>> +                              "Un-supported TPC message received 0x%08X\n",
>> +                              amsg->msg);
>> +                     break;
>> +             }
>> +             break;
>> +     default:
>> +             dev_warn(ctx->dev, "Un-supported message received 0x%08X\n",
>> +                      amsg->msg);
>
> Are those messages necessary ? Can they clog the log ?

Will remove it.

>
>> +             break;
>> +     }
>> +}
>> +
>> +/*
>> + * 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->lock)) {
>> +             /*
>> +              * If PCC, send a consumer command to Platform to get info
>> +              * If Slimpro Mailbox, get message from specific FIFO
>> +              */
>> +             if (ACPI_COMPANION(ctx->dev)) {
>> +                     ret = xgene_hwmon_get_notification_msg(ctx,
>> +                                                            (u32 *)&amsg);
>> +                     if (ret < 0)
>> +                             continue;
>> +             }
>> +             switch (SLIMPRO_MSG_TYPE(amsg.msg)) {
>> +             case SLIMPRO_MSG_TYPE_PWRMGMT_ID:
>> +                     xgene_hwmon_process_pwrmsg(ctx, &amsg);
>> +                     break;
>> +             default:
>> +                     dev_warn(ctx->dev,
>> +                              "Invalid mailbox msg received 0x%08X 0x%08X 0x%08X\n",
>> +                              amsg.msg, amsg.param1, amsg.param2);
>
> Is this message necessary ? I am concerned about the logging noise it might
> create.

Will remove it.

>
>> +                     break;
>> +             }
>> +     }
>> +}
>> +
>> +/*
>> + * This function is called when the SLIMpro/PCC 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 acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
>> +     struct slimpro_resp_msg amsg;
>> +
>> +     /* If PCC mailbox controller, get msg from shared memory */
>> +     if (ACPI_COMPANION(ctx->dev)) {
>> +             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 &&
>> +         ((SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_ERR_ID) ||
>> +          (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_DBG_ID &&
>> +           SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_DBG_SUBTYPE_SENSOR_READ) ||
>> +          (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_PWRMGMT_ID &&
>> +           SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_PWRMGMT_SUBTYPE_TPC &&
>> +           SLIMPRO_TPC_CMD(((u32 *) msg)[0]) == SLIMPRO_TPC_ALARM))) {
>> +             if (ACPI_COMPANION(ctx->dev)) {
>> +                     /* Check if platform completes command */
>> +                     if (!xgene_word_tst_and_clr(&generic_comm_base->status,
>> +                                                 PCCS_CMD_COMPLETE))
>> +                             goto notify;
>
> How about reversing the logic here and dropping the goto ?

OK,

>
> Overall, I have to say that the code is quite complex due to the repeated
> checks for ACPI. I am close to suggest having two separate drivers,
> one for ACPI and one for non-ACPI. Any chance to separate ACPI vs. non-ACPI
> code a bit better ?

How about create separate rx_cb functions for ACPI and non-ACPI ?
As almost functions are the same between ACPI and non-ACPI, I thought
keep the same driver is still OK, isn't it ?

>
>> +             }
>> +             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;
>> +     }
>> +
>> +     /*
>> +      * If PCC, 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.
>> +      */
>> +notify:
>> +     if (ACPI_COMPANION(ctx->dev)) {
>> +             /* Check and clear Platform Notification bit */
>> +             if (!xgene_word_tst_and_clr(&generic_comm_base->status,
>> +                                         PCCS_PLATFORM_NOTIFICATION))
>> +                     return;
>> +     } else {
>> +             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->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 __init 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->rx_callback = xgene_hwmon_rx_cb;
>> +     cl->tx_done = xgene_hwmon_tx_done;
>> +     cl->tx_block = false;
>> +     cl->tx_tout = MBOX_OP_TIMEOUTMS;
>> +     cl->knows_txdone = false;
>> +     if (!ACPI_COMPANION(cl->dev)) {
>> +             ctx->mbox_chan = mbox_request_channel(cl, MBOX_HWMON_INDEX);
>> +             if (IS_ERR(ctx->mbox_chan)) {
>> +                     dev_err(&pdev->dev,
>> +                             "SLIMpro mailbox channel request failed\n");
>> +                     return PTR_ERR(ctx->mbox_chan);
>> +             }
>> +     } else {
>> +             struct acpi_pcct_hw_reduced *cppc_ss;
>> +
>> +             ctx->mbox_chan =
>> +                     pcc_mbox_request_channel(cl, SLIMPRO_MSG_PCC_SUBSPACE);
>> +             if (IS_ERR(ctx->mbox_chan)) {
>> +                     dev_err(&pdev->dev,
>> +                             "PPC channel request failed\n");
>> +                     return PTR_ERR(ctx->mbox_chan);
>> +             }
>> +
>> +             /*
>> +              * 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->lock);
>> +
>> +     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);
>> +
>> +     /* Hook up sysfs for sensor monitor */
>> +     rc = sysfs_create_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
>> +     if (rc) {
>> +             dev_err(&pdev->dev, "Fail to create sysfs\n");
>> +             goto out_kfifo_free;
>> +     }
>> +
>> +     ctx->hwmon_dev = hwmon_device_register(ctx->dev);
>> +     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;
>> +     }
>
> Please use hwmon_device_register_with_groups().

OK,

Thanks and Regards
Hoan
>
>> +
>> +     dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
>> +
>> +     return rc;
>> +
>> +out:
>> +     sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
>> +out_kfifo_free:
>> +     kfifo_free(&ctx->async_msg_fifo);
>> +out_mbox_free:
>> +     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);
>> +
>> +     hwmon_device_unregister(ctx->hwmon_dev);
>> +     sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
>> +     kfifo_free(&ctx->async_msg_fifo);
>> +     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: [2/3] hwmon: xgene: Adds hwmon driver

[Guenter Roeck]

On Tue, May 31, 2016 at 11:00:05PM -0700, Hoan Tran wrote:
> Hi Guenter,
> 
> >
> > Overall, I have to say that the code is quite complex due to the repeated
> > checks for ACPI. I am close to suggest having two separate drivers,
> > one for ACPI and one for non-ACPI. Any chance to separate ACPI vs. non-ACPI
> > code a bit better ?
> 
> How about create separate rx_cb functions for ACPI and non-ACPI ?

Yes, that would be great if it is possible.

> As almost functions are the same between ACPI and non-ACPI, I thought
> keep the same driver is still OK, isn't it ?
> 
If you can separate the ACPI code from the non-ACPI code a bit better than
right now, yes.

Thanks,
Guenter

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

[Hoan Tran]

Hi Rob,

On Wed, May 25, 2016 at 10:09 AM, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, May 23, 2016 at 06:01:14PM -0700, Hoan Tran wrote:
> > Hi Rob,
> >
> > Thanks for your review !
> >
> > On Mon, May 23, 2016 at 1:30 PM, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, May 16, 2016 at 09:17:25AM -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
> > > >
> > > > 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..49a482e
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
> > > > @@ -0,0 +1,14 @@
> > > > +APM X-Gene hwmon driver
> > > > +
> > > > +Hwmon driver accesses sensors over the "SLIMpro" mailbox.
> > >
> > > DT bindings describe h/w, not driver data.
> > How about this description: "APM X-Gene SOC sensors are accessed over
> > the "SLIMpro" mailbox" ?
> > > I'm not sure this belongs in
> > > DT and perhaps the devices for the mailbox should be created by the
> > > mailbox driver.
> > I don't think the current mailbox supports it.
>
> Then add it. The mailbox binding is really only needed when other h/w
> blocks use a mailbox. As this is purely a firmware interface then the
> mailbox driver can create any devices it needs. The devices don't have
> to be in DT to be created.

Yes, I'll create a function inside mailbox. It allows client request a
channel by mailbox name and index without DT node.

Thanks
Hoan


>
>
> Rob

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

[Jassi Brar]

On Tue, May 24, 2016 at 6:31 AM, Hoan Tran <hotran@apm.com> wrote:
> Hi Rob,
>
> Thanks for your review !
>
> On Mon, May 23, 2016 at 1:30 PM, Rob Herring <robh@kernel.org> wrote:
>>
>> On Mon, May 16, 2016 at 09:17:25AM -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
>> >
>> > 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..49a482e
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
>> > @@ -0,0 +1,14 @@
>> > +APM X-Gene hwmon driver
>> > +
>> > +Hwmon driver accesses sensors over the "SLIMpro" mailbox.
>>
>> DT bindings describe h/w, not driver data.
> How about this description: "APM X-Gene SOC sensors are accessed over
> the "SLIMpro" mailbox" ?
>> I'm not sure this belongs in
>> DT and perhaps the devices for the mailbox should be created by the
>> mailbox driver.
> I don't think the current mailbox supports it.
>>
>> > +
>> > +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.
>>
>> When do you expect this to be different mailbox numbers?
> No, this number is not changed. This "mboxes" property is used and
> required by mailbox.c when hwmon driver requests a mailbox channel
>
I think that's inaccurate.

The h/w and the firmware combined is the "platform" from Linux POV.
Channels are physical resources provided by a mailbox controller.
Currently the firmware listens on Channel-7 but some future revision
might switch to, say, Channel-9.  Or say the same firmware on next
revision of h/w may have to switch to Channel-3 because it has only 4
channels. So I see the mailbox channel number as a hardware property
just like an IRQ (which very often change with SoC iterations).

Cheers.

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

[Hoan Tran]

Hi Jassi,

Thanks for your reply !

On Tue, Jun 7, 2016 at 10:20 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Tue, May 24, 2016 at 6:31 AM, Hoan Tran <hotran@apm.com> wrote:
>> Hi Rob,
>>
>> Thanks for your review !
>>
>> On Mon, May 23, 2016 at 1:30 PM, Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Mon, May 16, 2016 at 09:17:25AM -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
>>> >
>>> > 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..49a482e
>>> > --- /dev/null
>>> > +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
>>> > @@ -0,0 +1,14 @@
>>> > +APM X-Gene hwmon driver
>>> > +
>>> > +Hwmon driver accesses sensors over the "SLIMpro" mailbox.
>>>
>>> DT bindings describe h/w, not driver data.
>> How about this description: "APM X-Gene SOC sensors are accessed over
>> the "SLIMpro" mailbox" ?
>>> I'm not sure this belongs in
>>> DT and perhaps the devices for the mailbox should be created by the
>>> mailbox driver.
>> I don't think the current mailbox supports it.
>>>
>>> > +
>>> > +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.
>>>
>>> When do you expect this to be different mailbox numbers?
>> No, this number is not changed. This "mboxes" property is used and
>> required by mailbox.c when hwmon driver requests a mailbox channel
>>
> I think that's inaccurate.
>
> The h/w and the firmware combined is the "platform" from Linux POV.
> Channels are physical resources provided by a mailbox controller.
> Currently the firmware listens on Channel-7 but some future revision
> might switch to, say, Channel-9.  Or say the same firmware on next
> revision of h/w may have to switch to Channel-3 because it has only 4
> channels. So I see the mailbox channel number as a hardware property
> just like an IRQ (which very often change with SoC iterations).

Agree about that. I suppose this number is not changed. But as you
said, the mailbox channel number can be changed based on SoC or
Firmware. It would be better if this channel number is specified
inside a DT node.

Hi Rob, do you have any comments ?

Thanks
Hoan

>
> Cheers.

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

[Hoan Tran]

On Tue, Jun 7, 2016 at 11:05 AM, Hoan Tran <hotran@apm.com> wrote:
> Hi Jassi,
>
> Thanks for your reply !
>
> On Tue, Jun 7, 2016 at 10:20 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Tue, May 24, 2016 at 6:31 AM, Hoan Tran <hotran@apm.com> wrote:
>>> Hi Rob,
>>>
>>> Thanks for your review !
>>>
>>> On Mon, May 23, 2016 at 1:30 PM, Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Mon, May 16, 2016 at 09:17:25AM -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
>>>> >
>>>> > 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..49a482e
>>>> > --- /dev/null
>>>> > +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
>>>> > @@ -0,0 +1,14 @@
>>>> > +APM X-Gene hwmon driver
>>>> > +
>>>> > +Hwmon driver accesses sensors over the "SLIMpro" mailbox.
>>>>
>>>> DT bindings describe h/w, not driver data.
>>> How about this description: "APM X-Gene SOC sensors are accessed over
>>> the "SLIMpro" mailbox" ?
>>>> I'm not sure this belongs in
>>>> DT and perhaps the devices for the mailbox should be created by the
>>>> mailbox driver.
>>> I don't think the current mailbox supports it.
>>>>
>>>> > +
>>>> > +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.
>>>>
>>>> When do you expect this to be different mailbox numbers?
>>> No, this number is not changed. This "mboxes" property is used and
>>> required by mailbox.c when hwmon driver requests a mailbox channel
>>>
>> I think that's inaccurate.
>>
>> The h/w and the firmware combined is the "platform" from Linux POV.
>> Channels are physical resources provided by a mailbox controller.
>> Currently the firmware listens on Channel-7 but some future revision
>> might switch to, say, Channel-9.  Or say the same firmware on next
>> revision of h/w may have to switch to Channel-3 because it has only 4
>> channels. So I see the mailbox channel number as a hardware property
>> just like an IRQ (which very often change with SoC iterations).
>
> Agree about that. I suppose this number is not changed. But as you
> said, the mailbox channel number can be changed based on SoC or
> Firmware. It would be better if this channel number is specified
> inside a DT node.
>
> Hi Rob, do you have any comments ?
>
> Thanks
> Hoan
>
>>
>> Cheers.

Hi Rob,

Do you have any comments on Jassi's reply ?
If not, I'll send another version which included the binding document
and DT node.

Thanks
Hoan