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

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

[Hoan Tran]

This patch set adds hardware temperature and power reading support for
APM X-Gene SoC using the mailbox communication interface.
For device tree, it is the standard DT mailbox.
For ACPI, it is the PCC mailbox.

For ACPI, this patch is built on top and depends on patch[1]:
[1] http://www.spinics.net/lists/linux-acpi/msg66041.html
 - [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2

v3
 - Order include files alphabetically
 - Use sign_extend32() to decode temperature
 - Use DEVICE_ATTR_RO() for temperature and power attributes
 - Temperature and power attributes start with index 1
 - Use !!amsg->param2
 - Fix checkpatch WARNING with --strict flag
 - Use hwmon_device_register_with_groups()
 - Check invalid sensor data

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

v1
 - Initial

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

 .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt  |  14 +
 Documentation/hwmon/xgene-hwmon                    |  30 +
 arch/arm64/boot/dts/apm/apm-shadowcat.dtsi         |   5 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   5 +
 drivers/hwmon/Kconfig                              |   7 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/xgene-hwmon.c                        | 755 +++++++++++++++++++++
 7 files changed, 817 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 v3 0/3] hwmon: xgene: Add support for X-Gene hwmon driver

[Hoan Tran]

This patch set adds hardware temperature and power reading support for
APM X-Gene SoC using the mailbox communication interface.
For device tree, it is the standard DT mailbox.
For ACPI, it is the PCC mailbox.

For ACPI, this patch is built on top and depends on patch[1]:
[1] http://www.spinics.net/lists/linux-acpi/msg66041.html
 - [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2

v3
 - Order include files alphabetically
 - Use sign_extend32() to decode temperature
 - Use DEVICE_ATTR_RO() for temperature and power attributes
 - Temperature and power attributes start with index 1
 - Use !!amsg->param2
 - Fix checkpatch WARNING with --strict flag
 - Use hwmon_device_register_with_groups()
 - Check invalid sensor data

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

v1
 - Initial

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

 .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt  |  14 +
 Documentation/hwmon/xgene-hwmon                    |  30 +
 arch/arm64/boot/dts/apm/apm-shadowcat.dtsi         |   5 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   5 +
 drivers/hwmon/Kconfig                              |   7 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/xgene-hwmon.c                        | 755 +++++++++++++++++++++
 7 files changed, 817 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 v3 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>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt

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

[PATCH v3 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>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt

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

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

[Guenter Roeck]

On Thu, Jul 21, 2016 at 01:55:55PM -0700, Hoan Tran wrote:
> This patch adds the APM X-Gene hwmon device tree node documentation.
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>
> Acked-by: Rob Herring <robh@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

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

[Guenter Roeck]

On Thu, Jul 21, 2016 at 01:55:55PM -0700, Hoan Tran wrote:
> This patch adds the APM X-Gene hwmon device tree node documentation.
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>
> Acked-by: Rob Herring <robh@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

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

[Hoan Tran]

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

Signed-off-by: Hoan Tran <hotran@apm.com>
---
 Documentation/hwmon/xgene-hwmon |  30 ++
 drivers/hwmon/Kconfig           |   7 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/xgene-hwmon.c     | 755 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 793 insertions(+)
 create mode 100644 Documentation/hwmon/xgene-hwmon
 create mode 100644 drivers/hwmon/xgene-hwmon.c

diff --git a/Documentation/hwmon/xgene-hwmon b/Documentation/hwmon/xgene-hwmon
new file mode 100644
index 0000000..6ec50ed
--- /dev/null
+++ b/Documentation/hwmon/xgene-hwmon
@@ -0,0 +1,30 @@
+Kernel driver xgene-hwmon
+========================
+
+Supported chips:
+ * APM X-Gene SoC
+
+Description
+-----------
+
+This driver adds hardware temperature and power reading support for
+APM X-Gene SoC using the mailbox communication interface.
+For device tree, it is the standard DT mailbox.
+For ACPI, it is the PCC mailbox.
+
+The following sensors are supported
+
+  * Temperature
+    - SoC on-die temperature in milli-degree C
+    - Alarm when high/over temperature occurs
+  * Power
+    - CPU power in uW
+    - IO power in uW
+
+sysfs-Interface
+---------------
+
+temp0_input - SoC on-die temperature (milli-degree C)
+temp0_critical_alarm - An 1 would indicates on-die temperature exceeded threshold
+power0_input - CPU power in (uW)
+power1_input - IO power in (uW)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ff94007..55218c6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1787,6 +1787,13 @@ config SENSORS_ULTRA45
 	  This driver provides support for the Ultra45 workstation environmental
 	  sensors.
 
+config SENSORS_XGENE
+	tristate "APM X-Gene SoC hardware monitoring driver"
+	depends on XGENE_SLIMPRO_MBOX || PCC
+	help
+	  If you say yes here you get support for the temperature
+	  and power sensors for APM X-Gene SoC.
+
 if ACPI
 
 comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 2ef5b7c..a2460dd 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -162,6 +162,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
 obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
 obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
 obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
+obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
 
 obj-$(CONFIG_PMBUS)		+= pmbus/
 
diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
new file mode 100644
index 0000000..c263e7e
--- /dev/null
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -0,0 +1,755 @@
+/*
+ * APM X-Gene SoC Hardware Monitoring Driver
+ *
+ * Copyright (c) 2016, Applied Micro Circuits Corporation
+ * Author: Loc Ho <lho@apm.com>
+ *         Hoan Tran <hotran@apm.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * This driver provides the following features:
+ *  - Retrieve CPU total power (uW)
+ *  - Retrieve IO total power (uW)
+ *  - Retrieve SoC temperature (milli-degree C) and alarm
+ */
+#include <linux/acpi.h>
+#include <linux/dma-mapping.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/kfifo.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <acpi/acpi_io.h>
+#include <acpi/pcc.h>
+
+/* SLIMpro message defines */
+#define MSG_TYPE_DBG			0
+#define MSG_TYPE_ERR			7
+#define MSG_TYPE_PWRMGMT		9
+
+#define MSG_TYPE(v)			(((v) & 0xF0000000) >> 28)
+#define MSG_TYPE_SET(v)			(((v) << 28) & 0xF0000000)
+#define MSG_SUBTYPE(v)			(((v) & 0x0F000000) >> 24)
+#define MSG_SUBTYPE_SET(v)		(((v) << 24) & 0x0F000000)
+
+#define DBG_SUBTYPE_SENSOR_READ		4
+#define SENSOR_RD_MSG			0x04FFE902
+#define SENSOR_RD_EN_ADDR(a)		((a) & 0x000FFFFF)
+#define PMD_PWR_REG			0x20
+#define PMD_PWR_MW_REG			0x26
+#define SOC_PWR_REG			0x21
+#define SOC_PWR_MW_REG			0x27
+#define SOC_TEMP_REG			0x10
+
+#define TEMP_NEGATIVE_BIT		8
+#define SENSOR_INVALID_DATA		BIT(15)
+
+#define PWRMGMT_SUBTYPE_TPC		1
+#define TPC_ALARM			2
+#define TPC_GET_ALARM			3
+#define TPC_CMD(v)			(((v) & 0x00FF0000) >> 16)
+#define TPC_CMD_SET(v)			(((v) << 16) & 0x00FF0000)
+#define TPC_EN_MSG(hndl, cmd, type) \
+	(MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \
+	MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type)
+
+/* PCC defines */
+#define PCC_SIGNATURE_MASK		0x50424300
+#define PCCC_GENERATE_DB_INT		BIT(15)
+#define PCCS_CMD_COMPLETE		BIT(0)
+#define PCCS_SCI_DOORBEL		BIT(1)
+#define PCCS_PLATFORM_NOTIFICATION	BIT(3)
+/*
+ * Arbitrary retries in case the remote processor is slow to respond
+ * to PCC commands
+ */
+#define PCC_NUM_RETRIES			500
+
+#define ASYNC_MSG_FIFO_SIZE		16
+#define MBOX_OP_TIMEOUTMS		1000
+
+#define WATT_TO_mWATT(x)		((x) * 1000)
+#define mWATT_TO_uWATT(x)		((x) * 1000)
+#define CELSIUS_TO_mCELSIUS(x)		((x) * 1000)
+
+#define to_xgene_hwmon_dev(cl)		\
+	container_of(cl, struct xgene_hwmon_dev, mbox_client)
+
+struct slimpro_resp_msg {
+	u32 msg;
+	u32 param1;
+	u32 param2;
+} __packed;
+
+struct xgene_hwmon_dev {
+	struct device		*dev;
+	struct mbox_chan	*mbox_chan;
+	struct mbox_client	mbox_client;
+	int			mbox_idx;
+
+	spinlock_t		kfifo_lock;
+	struct mutex		rd_mutex;
+	struct completion	rd_complete;
+	int			resp_pending;
+	struct slimpro_resp_msg sync_msg;
+
+	struct work_struct	workq;
+	struct kfifo_rec_ptr_1	async_msg_fifo;
+
+	struct device		*hwmon_dev;
+	bool			temp_critical_alarm;
+
+	phys_addr_t		comm_base_addr;
+	void			*pcc_comm_addr;
+	u64			usecs_lat;
+};
+
+/*
+ * This function tests and clears a bitmask then returns its old value
+ */
+static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
+{
+	u16 ret, val;
+
+	val = readw_relaxed(addr);
+	ret = val & mask;
+	val &= ~mask;
+	writew_relaxed(val, addr);
+
+	return ret;
+}
+
+static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
+{
+	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	void *ptr = generic_comm_base + 1;
+	int rc, i;
+	u16 val;
+
+	mutex_lock(&ctx->rd_mutex);
+	init_completion(&ctx->rd_complete);
+	ctx->resp_pending = true;
+
+	/* Write signature for subspace */
+	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
+		       &generic_comm_base->signature);
+
+	/* Write to the shared command region */
+	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
+		       &generic_comm_base->command);
+
+	/* Flip CMD COMPLETE bit */
+	val = readw_relaxed(&generic_comm_base->status);
+	val &= ~PCCS_CMD_COMPLETE;
+	writew_relaxed(val, &generic_comm_base->status);
+
+	/* Copy the message to the PCC comm space */
+	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
+		writel_relaxed(msg[i], ptr + i * 4);
+
+	/* Ring the doorbell */
+	rc = mbox_send_message(ctx->mbox_chan, msg);
+	if (rc < 0) {
+		dev_err(ctx->dev, "Mailbox send error %d\n", rc);
+		goto err;
+	}
+	if (!wait_for_completion_timeout(&ctx->rd_complete,
+					 usecs_to_jiffies(ctx->usecs_lat))) {
+		dev_err(ctx->dev, "Mailbox operation timed out\n");
+		rc = -ETIMEDOUT;
+		goto err;
+	}
+
+	/* Check for error message */
+	if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) {
+		rc = -EINVAL;
+		goto err;
+	}
+
+	msg[0] = ctx->sync_msg.msg;
+	msg[1] = ctx->sync_msg.param1;
+	msg[2] = ctx->sync_msg.param2;
+
+err:
+	mbox_chan_txdone(ctx->mbox_chan, 0);
+	ctx->resp_pending = false;
+	mutex_unlock(&ctx->rd_mutex);
+	return rc;
+}
+
+static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
+{
+	int rc;
+
+	mutex_lock(&ctx->rd_mutex);
+	init_completion(&ctx->rd_complete);
+	ctx->resp_pending = true;
+
+	rc = mbox_send_message(ctx->mbox_chan, msg);
+	if (rc < 0) {
+		dev_err(ctx->dev, "Mailbox send error %d\n", rc);
+		goto err;
+	}
+
+	if (!wait_for_completion_timeout(&ctx->rd_complete,
+					 msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) {
+		dev_err(ctx->dev, "Mailbox operation timed out\n");
+		rc = -ETIMEDOUT;
+		goto err;
+	}
+
+	/* Check for error message */
+	if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) {
+		rc = -EINVAL;
+		goto err;
+	}
+
+	msg[0] = ctx->sync_msg.msg;
+	msg[1] = ctx->sync_msg.param1;
+	msg[2] = ctx->sync_msg.param2;
+
+err:
+	ctx->resp_pending = false;
+	mutex_unlock(&ctx->rd_mutex);
+	return rc;
+}
+
+static int xgene_hwmon_reg_map_rd(struct xgene_hwmon_dev *ctx, u32 addr,
+				  u32 *data)
+{
+	u32 msg[3];
+	int rc;
+
+	msg[0] = SENSOR_RD_MSG;
+	msg[1] = SENSOR_RD_EN_ADDR(addr);
+	msg[2] = 0;
+
+	if (acpi_disabled)
+		rc = xgene_hwmon_rd(ctx, msg);
+	else
+		rc = xgene_hwmon_pcc_rd(ctx, msg);
+
+	if (rc < 0)
+		return rc;
+
+	/*
+	 * Check if sensor data is valid.
+	 */
+	if (msg[1] & SENSOR_INVALID_DATA)
+		return -ENODATA;
+
+	*data = msg[1];
+
+	return rc;
+}
+
+static int xgene_hwmon_get_notification_msg(struct xgene_hwmon_dev *ctx,
+					    u32 *amsg)
+{
+	u32 msg[3];
+	int rc;
+
+	msg[0] = TPC_EN_MSG(PWRMGMT_SUBTYPE_TPC, TPC_GET_ALARM, 0);
+	msg[1] = 0;
+	msg[2] = 0;
+
+	rc = xgene_hwmon_pcc_rd(ctx, msg);
+	if (rc < 0)
+		return rc;
+
+	amsg[0] = msg[0];
+	amsg[1] = msg[1];
+	amsg[2] = msg[2];
+
+	return rc;
+}
+
+static int xgene_hwmon_get_cpu_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	u32 watt, mwatt;
+	int rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_REG, &watt);
+	if (rc < 0)
+		return rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, &mwatt);
+	if (rc < 0)
+		return rc;
+
+	*val = WATT_TO_mWATT(watt) + mwatt;
+	return 0;
+}
+
+static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	u32 watt, mwatt;
+	int rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, &watt);
+	if (rc < 0)
+		return rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_MW_REG, &mwatt);
+	if (rc < 0)
+		return rc;
+
+	*val = WATT_TO_mWATT(watt) + mwatt;
+	return 0;
+}
+
+static int xgene_hwmon_get_temp(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	return xgene_hwmon_reg_map_rd(ctx, SOC_TEMP_REG, val);
+}
+
+/*
+ * Sensor temperature/power functions
+ */
+static ssize_t temp1_input_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
+	int rc, temp;
+	u32 val;
+
+	rc = xgene_hwmon_get_temp(ctx, &val);
+	if (rc < 0)
+		return rc;
+
+	temp = sign_extend32(val, TEMP_NEGATIVE_BIT);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", CELSIUS_TO_mCELSIUS(temp));
+}
+
+static ssize_t temp1_label_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "SoC Temperature\n");
+}
+
+static ssize_t temp1_critical_alarm_show(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 power1_label_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "CPU power\n");
+}
+
+static ssize_t power2_label_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "IO power\n");
+}
+
+static ssize_t power1_input_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
+	u32 val;
+	int rc;
+
+	rc = xgene_hwmon_get_cpu_pwr(ctx, &val);
+	if (rc < 0)
+		return rc;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val));
+}
+
+static ssize_t power2_input_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
+	u32 val;
+	int rc;
+
+	rc = xgene_hwmon_get_io_pwr(ctx, &val);
+	if (rc < 0)
+		return rc;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val));
+}
+
+static DEVICE_ATTR_RO(temp1_label);
+static DEVICE_ATTR_RO(temp1_input);
+static DEVICE_ATTR_RO(temp1_critical_alarm);
+static DEVICE_ATTR_RO(power1_label);
+static DEVICE_ATTR_RO(power1_input);
+static DEVICE_ATTR_RO(power2_label);
+static DEVICE_ATTR_RO(power2_input);
+
+static struct attribute *xgene_hwmon_attrs[] = {
+	&dev_attr_temp1_label.attr,
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_critical_alarm.attr,
+	&dev_attr_power1_label.attr,
+	&dev_attr_power1_input.attr,
+	&dev_attr_power2_label.attr,
+	&dev_attr_power2_input.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(xgene_hwmon);
+
+static int xgene_hwmon_tpc_alarm(struct xgene_hwmon_dev *ctx,
+				 struct slimpro_resp_msg *amsg)
+{
+	ctx->temp_critical_alarm = !!amsg->param2;
+	sysfs_notify(&ctx->dev->kobj, NULL, "temp1_critical_alarm");
+
+	return 0;
+}
+
+static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx,
+				       struct slimpro_resp_msg *amsg)
+{
+	if ((MSG_SUBTYPE(amsg->msg) == PWRMGMT_SUBTYPE_TPC) &&
+	    (TPC_CMD(amsg->msg) == TPC_ALARM))
+		xgene_hwmon_tpc_alarm(ctx, amsg);
+}
+
+/*
+ * This function is called to process async work queue
+ */
+static void xgene_hwmon_evt_work(struct work_struct *work)
+{
+	struct slimpro_resp_msg amsg;
+	struct xgene_hwmon_dev *ctx;
+	int ret;
+
+	ctx = container_of(work, struct xgene_hwmon_dev, workq);
+	while (kfifo_out_spinlocked(&ctx->async_msg_fifo, &amsg,
+				    sizeof(struct slimpro_resp_msg),
+				    &ctx->kfifo_lock)) {
+		/*
+		 * If PCC, send a consumer command to Platform to get info
+		 * If Slimpro Mailbox, get message from specific FIFO
+		 */
+		if (!acpi_disabled) {
+			ret = xgene_hwmon_get_notification_msg(ctx,
+							       (u32 *)&amsg);
+			if (ret < 0)
+				continue;
+		}
+
+		if (MSG_TYPE(amsg.msg) == MSG_TYPE_PWRMGMT)
+			xgene_hwmon_process_pwrmsg(ctx, &amsg);
+	}
+}
+
+/*
+ * This function is called when the SLIMpro Mailbox received a message
+ */
+static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg)
+{
+	struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
+	struct slimpro_resp_msg amsg;
+
+	/*
+	 * Response message format:
+	 * msg[0] is the return code of the operation
+	 * msg[1] is the first parameter word
+	 * msg[2] is the second parameter word
+	 *
+	 * As message only supports dword size, just assign it.
+	 */
+
+	/* Check for sync query */
+	if (ctx->resp_pending &&
+	    ((MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_ERR) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_DBG &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == DBG_SUBTYPE_SENSOR_READ) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_PWRMGMT &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == PWRMGMT_SUBTYPE_TPC &&
+	      TPC_CMD(((u32 *)msg)[0]) == TPC_ALARM))) {
+		ctx->sync_msg.msg = ((u32 *)msg)[0];
+		ctx->sync_msg.param1 = ((u32 *)msg)[1];
+		ctx->sync_msg.param2 = ((u32 *)msg)[2];
+
+		/* Operation waiting for response */
+		complete(&ctx->rd_complete);
+
+		return;
+	}
+
+	amsg.msg   = ((u32 *)msg)[0];
+	amsg.param1 = ((u32 *)msg)[1];
+	amsg.param2 = ((u32 *)msg)[2];
+
+	/* Enqueue to the FIFO */
+	kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg,
+			    sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock);
+	/* Schedule the bottom handler */
+	schedule_work(&ctx->workq);
+}
+
+/*
+ * This function is called when the PCC Mailbox received a message
+ */
+static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
+{
+	struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
+	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct slimpro_resp_msg amsg;
+
+	msg = generic_comm_base + 1;
+	/* Check if platform sends interrupt */
+	if (!xgene_word_tst_and_clr(&generic_comm_base->status,
+				    PCCS_SCI_DOORBEL))
+		return;
+
+	/*
+	 * Response message format:
+	 * msg[0] is the return code of the operation
+	 * msg[1] is the first parameter word
+	 * msg[2] is the second parameter word
+	 *
+	 * As message only supports dword size, just assign it.
+	 */
+
+	/* Check for sync query */
+	if (ctx->resp_pending &&
+	    ((MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_ERR) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_DBG &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == DBG_SUBTYPE_SENSOR_READ) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_PWRMGMT &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == PWRMGMT_SUBTYPE_TPC &&
+	      TPC_CMD(((u32 *)msg)[0]) == TPC_ALARM))) {
+		/* Check if platform completes command */
+		if (xgene_word_tst_and_clr(&generic_comm_base->status,
+					   PCCS_CMD_COMPLETE)) {
+			ctx->sync_msg.msg = ((u32 *)msg)[0];
+			ctx->sync_msg.param1 = ((u32 *)msg)[1];
+			ctx->sync_msg.param2 = ((u32 *)msg)[2];
+
+			/* Operation waiting for response */
+			complete(&ctx->rd_complete);
+
+			return;
+		}
+	}
+
+	/*
+	 * Platform notifies interrupt to OSPM.
+	 * OPSM schedules a consumer command to get this information
+	 * in a workqueue. Platform must wait until OSPM has issued
+	 * a consumer command that serves this notification.
+	 */
+
+	/* Enqueue to the FIFO */
+	kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg,
+			    sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock);
+	/* Schedule the bottom handler */
+	schedule_work(&ctx->workq);
+}
+
+static void xgene_hwmon_tx_done(struct mbox_client *cl, void *msg, int ret)
+{
+	if (ret) {
+		dev_dbg(cl->dev, "TX did not complete: CMD sent:%x, ret:%d\n",
+			*(u16 *)msg, ret);
+	} else {
+		dev_dbg(cl->dev, "TX completed. CMD sent:%x, ret:%d\n",
+			*(u16 *)msg, ret);
+	}
+}
+
+static int xgene_hwmon_probe(struct platform_device *pdev)
+{
+	struct xgene_hwmon_dev *ctx;
+	struct mbox_client *cl;
+	int rc;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ctx);
+	cl = &ctx->mbox_client;
+
+	/* Request mailbox channel */
+	cl->dev = &pdev->dev;
+	cl->tx_done = xgene_hwmon_tx_done;
+	cl->tx_block = false;
+	cl->tx_tout = MBOX_OP_TIMEOUTMS;
+	cl->knows_txdone = false;
+	if (acpi_disabled) {
+		cl->rx_callback = xgene_hwmon_rx_cb;
+		ctx->mbox_chan = mbox_request_channel(cl, 0);
+		if (IS_ERR(ctx->mbox_chan)) {
+			dev_err(&pdev->dev,
+				"SLIMpro mailbox channel request failed\n");
+			return -ENODEV;
+		}
+	} else {
+		struct acpi_pcct_hw_reduced *cppc_ss;
+
+		if (device_property_read_u32(&pdev->dev, "pcc-channel",
+					     &ctx->mbox_idx)) {
+			dev_err(&pdev->dev, "no pcc-channel property\n");
+			return -ENODEV;
+		}
+
+		cl->rx_callback = xgene_hwmon_pcc_rx_cb;
+		ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx);
+		if (IS_ERR(ctx->mbox_chan)) {
+			dev_err(&pdev->dev,
+				"PPC channel request failed\n");
+			return -ENODEV;
+		}
+
+		/*
+		 * The PCC mailbox controller driver should
+		 * have parsed the PCCT (global table of all
+		 * PCC channels) and stored pointers to the
+		 * subspace communication region in con_priv.
+		 */
+		cppc_ss = ctx->mbox_chan->con_priv;
+		if (!cppc_ss) {
+			dev_err(&pdev->dev, "PPC subspace not found\n");
+			rc = -ENODEV;
+			goto out_mbox_free;
+		}
+
+		if (!ctx->mbox_chan->mbox->txdone_irq) {
+			dev_err(&pdev->dev, "PCC IRQ not supported\n");
+			rc = -ENODEV;
+			goto out_mbox_free;
+		}
+
+		/*
+		 * This is the shared communication region
+		 * for the OS and Platform to communicate over.
+		 */
+		ctx->comm_base_addr = cppc_ss->base_address;
+		if (ctx->comm_base_addr) {
+			ctx->pcc_comm_addr =
+					acpi_os_ioremap(ctx->comm_base_addr,
+							cppc_ss->length);
+		} else {
+			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
+			rc = -ENODEV;
+			goto out_mbox_free;
+		}
+
+		if (!ctx->pcc_comm_addr) {
+			dev_err(&pdev->dev,
+				"Failed to ioremap PCC comm region\n");
+			rc = -ENOMEM;
+			goto out_mbox_free;
+		}
+
+		/*
+		 * cppc_ss->latency is just a Nominal value. In reality
+		 * the remote processor could be much slower to reply.
+		 * So add an arbitrary amount of wait on top of Nominal.
+		 */
+		ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency;
+	}
+
+	spin_lock_init(&ctx->kfifo_lock);
+	mutex_init(&ctx->rd_mutex);
+
+	rc = kfifo_alloc(&ctx->async_msg_fifo,
+			 sizeof(struct slimpro_resp_msg) * ASYNC_MSG_FIFO_SIZE,
+			 GFP_KERNEL);
+	if (rc)
+		goto out_mbox_free;
+
+	INIT_WORK(&ctx->workq, xgene_hwmon_evt_work);
+
+	ctx->hwmon_dev = hwmon_device_register_with_groups(ctx->dev,
+							   "apm_xgene",
+							   ctx,
+							   xgene_hwmon_groups);
+	if (IS_ERR(ctx->hwmon_dev)) {
+		dev_err(&pdev->dev, "Failed to register HW monitor device\n");
+		rc = PTR_ERR(ctx->hwmon_dev);
+		goto out;
+	}
+
+	dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
+
+	return rc;
+
+out:
+	kfifo_free(&ctx->async_msg_fifo);
+out_mbox_free:
+	if (acpi_disabled)
+		mbox_free_channel(ctx->mbox_chan);
+	else
+		pcc_mbox_free_channel(ctx->mbox_chan);
+
+	return rc;
+}
+
+static int xgene_hwmon_remove(struct platform_device *pdev)
+{
+	struct xgene_hwmon_dev *ctx = platform_get_drvdata(pdev);
+
+	hwmon_device_unregister(ctx->hwmon_dev);
+	kfifo_free(&ctx->async_msg_fifo);
+	if (acpi_disabled)
+		mbox_free_channel(ctx->mbox_chan);
+	else
+		pcc_mbox_free_channel(ctx->mbox_chan);
+
+	return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id xgene_hwmon_acpi_match[] = {
+	{"APMC0D29", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match);
+#endif
+
+static const struct of_device_id xgene_hwmon_of_match[] = {
+	{.compatible = "apm,xgene-slimpro-hwmon"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, xgene_hwmon_of_match);
+
+static struct platform_driver xgene_hwmon_driver __refdata = {
+	.probe = xgene_hwmon_probe,
+	.remove = xgene_hwmon_remove,
+	.driver = {
+		.name = "xgene-slimpro-hwmon",
+		.of_match_table = xgene_hwmon_of_match,
+		.acpi_match_table = ACPI_PTR(xgene_hwmon_acpi_match),
+	},
+};
+module_platform_driver(xgene_hwmon_driver);
+
+MODULE_DESCRIPTION("APM X-Gene SoC hardware monitor");
+MODULE_LICENSE("GPL");
-- 
1.9.1

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

[Hoan Tran]

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

Signed-off-by: Hoan Tran <hotran@apm.com>
---
 Documentation/hwmon/xgene-hwmon |  30 ++
 drivers/hwmon/Kconfig           |   7 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/xgene-hwmon.c     | 755 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 793 insertions(+)
 create mode 100644 Documentation/hwmon/xgene-hwmon
 create mode 100644 drivers/hwmon/xgene-hwmon.c

diff --git a/Documentation/hwmon/xgene-hwmon b/Documentation/hwmon/xgene-hwmon
new file mode 100644
index 0000000..6ec50ed
--- /dev/null
+++ b/Documentation/hwmon/xgene-hwmon
@@ -0,0 +1,30 @@
+Kernel driver xgene-hwmon
+========================
+
+Supported chips:
+ * APM X-Gene SoC
+
+Description
+-----------
+
+This driver adds hardware temperature and power reading support for
+APM X-Gene SoC using the mailbox communication interface.
+For device tree, it is the standard DT mailbox.
+For ACPI, it is the PCC mailbox.
+
+The following sensors are supported
+
+  * Temperature
+    - SoC on-die temperature in milli-degree C
+    - Alarm when high/over temperature occurs
+  * Power
+    - CPU power in uW
+    - IO power in uW
+
+sysfs-Interface
+---------------
+
+temp0_input - SoC on-die temperature (milli-degree C)
+temp0_critical_alarm - An 1 would indicates on-die temperature exceeded threshold
+power0_input - CPU power in (uW)
+power1_input - IO power in (uW)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ff94007..55218c6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1787,6 +1787,13 @@ config SENSORS_ULTRA45
 	  This driver provides support for the Ultra45 workstation environmental
 	  sensors.
 
+config SENSORS_XGENE
+	tristate "APM X-Gene SoC hardware monitoring driver"
+	depends on XGENE_SLIMPRO_MBOX || PCC
+	help
+	  If you say yes here you get support for the temperature
+	  and power sensors for APM X-Gene SoC.
+
 if ACPI
 
 comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 2ef5b7c..a2460dd 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -162,6 +162,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
 obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
 obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
 obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
+obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
 
 obj-$(CONFIG_PMBUS)		+= pmbus/
 
diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
new file mode 100644
index 0000000..c263e7e
--- /dev/null
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -0,0 +1,755 @@
+/*
+ * APM X-Gene SoC Hardware Monitoring Driver
+ *
+ * Copyright (c) 2016, Applied Micro Circuits Corporation
+ * Author: Loc Ho <lho@apm.com>
+ *         Hoan Tran <hotran@apm.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * This driver provides the following features:
+ *  - Retrieve CPU total power (uW)
+ *  - Retrieve IO total power (uW)
+ *  - Retrieve SoC temperature (milli-degree C) and alarm
+ */
+#include <linux/acpi.h>
+#include <linux/dma-mapping.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/kfifo.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <acpi/acpi_io.h>
+#include <acpi/pcc.h>
+
+/* SLIMpro message defines */
+#define MSG_TYPE_DBG			0
+#define MSG_TYPE_ERR			7
+#define MSG_TYPE_PWRMGMT		9
+
+#define MSG_TYPE(v)			(((v) & 0xF0000000) >> 28)
+#define MSG_TYPE_SET(v)			(((v) << 28) & 0xF0000000)
+#define MSG_SUBTYPE(v)			(((v) & 0x0F000000) >> 24)
+#define MSG_SUBTYPE_SET(v)		(((v) << 24) & 0x0F000000)
+
+#define DBG_SUBTYPE_SENSOR_READ		4
+#define SENSOR_RD_MSG			0x04FFE902
+#define SENSOR_RD_EN_ADDR(a)		((a) & 0x000FFFFF)
+#define PMD_PWR_REG			0x20
+#define PMD_PWR_MW_REG			0x26
+#define SOC_PWR_REG			0x21
+#define SOC_PWR_MW_REG			0x27
+#define SOC_TEMP_REG			0x10
+
+#define TEMP_NEGATIVE_BIT		8
+#define SENSOR_INVALID_DATA		BIT(15)
+
+#define PWRMGMT_SUBTYPE_TPC		1
+#define TPC_ALARM			2
+#define TPC_GET_ALARM			3
+#define TPC_CMD(v)			(((v) & 0x00FF0000) >> 16)
+#define TPC_CMD_SET(v)			(((v) << 16) & 0x00FF0000)
+#define TPC_EN_MSG(hndl, cmd, type) \
+	(MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \
+	MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type)
+
+/* PCC defines */
+#define PCC_SIGNATURE_MASK		0x50424300
+#define PCCC_GENERATE_DB_INT		BIT(15)
+#define PCCS_CMD_COMPLETE		BIT(0)
+#define PCCS_SCI_DOORBEL		BIT(1)
+#define PCCS_PLATFORM_NOTIFICATION	BIT(3)
+/*
+ * Arbitrary retries in case the remote processor is slow to respond
+ * to PCC commands
+ */
+#define PCC_NUM_RETRIES			500
+
+#define ASYNC_MSG_FIFO_SIZE		16
+#define MBOX_OP_TIMEOUTMS		1000
+
+#define WATT_TO_mWATT(x)		((x) * 1000)
+#define mWATT_TO_uWATT(x)		((x) * 1000)
+#define CELSIUS_TO_mCELSIUS(x)		((x) * 1000)
+
+#define to_xgene_hwmon_dev(cl)		\
+	container_of(cl, struct xgene_hwmon_dev, mbox_client)
+
+struct slimpro_resp_msg {
+	u32 msg;
+	u32 param1;
+	u32 param2;
+} __packed;
+
+struct xgene_hwmon_dev {
+	struct device		*dev;
+	struct mbox_chan	*mbox_chan;
+	struct mbox_client	mbox_client;
+	int			mbox_idx;
+
+	spinlock_t		kfifo_lock;
+	struct mutex		rd_mutex;
+	struct completion	rd_complete;
+	int			resp_pending;
+	struct slimpro_resp_msg sync_msg;
+
+	struct work_struct	workq;
+	struct kfifo_rec_ptr_1	async_msg_fifo;
+
+	struct device		*hwmon_dev;
+	bool			temp_critical_alarm;
+
+	phys_addr_t		comm_base_addr;
+	void			*pcc_comm_addr;
+	u64			usecs_lat;
+};
+
+/*
+ * This function tests and clears a bitmask then returns its old value
+ */
+static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
+{
+	u16 ret, val;
+
+	val = readw_relaxed(addr);
+	ret = val & mask;
+	val &= ~mask;
+	writew_relaxed(val, addr);
+
+	return ret;
+}
+
+static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
+{
+	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	void *ptr = generic_comm_base + 1;
+	int rc, i;
+	u16 val;
+
+	mutex_lock(&ctx->rd_mutex);
+	init_completion(&ctx->rd_complete);
+	ctx->resp_pending = true;
+
+	/* Write signature for subspace */
+	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
+		       &generic_comm_base->signature);
+
+	/* Write to the shared command region */
+	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
+		       &generic_comm_base->command);
+
+	/* Flip CMD COMPLETE bit */
+	val = readw_relaxed(&generic_comm_base->status);
+	val &= ~PCCS_CMD_COMPLETE;
+	writew_relaxed(val, &generic_comm_base->status);
+
+	/* Copy the message to the PCC comm space */
+	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
+		writel_relaxed(msg[i], ptr + i * 4);
+
+	/* Ring the doorbell */
+	rc = mbox_send_message(ctx->mbox_chan, msg);
+	if (rc < 0) {
+		dev_err(ctx->dev, "Mailbox send error %d\n", rc);
+		goto err;
+	}
+	if (!wait_for_completion_timeout(&ctx->rd_complete,
+					 usecs_to_jiffies(ctx->usecs_lat))) {
+		dev_err(ctx->dev, "Mailbox operation timed out\n");
+		rc = -ETIMEDOUT;
+		goto err;
+	}
+
+	/* Check for error message */
+	if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) {
+		rc = -EINVAL;
+		goto err;
+	}
+
+	msg[0] = ctx->sync_msg.msg;
+	msg[1] = ctx->sync_msg.param1;
+	msg[2] = ctx->sync_msg.param2;
+
+err:
+	mbox_chan_txdone(ctx->mbox_chan, 0);
+	ctx->resp_pending = false;
+	mutex_unlock(&ctx->rd_mutex);
+	return rc;
+}
+
+static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
+{
+	int rc;
+
+	mutex_lock(&ctx->rd_mutex);
+	init_completion(&ctx->rd_complete);
+	ctx->resp_pending = true;
+
+	rc = mbox_send_message(ctx->mbox_chan, msg);
+	if (rc < 0) {
+		dev_err(ctx->dev, "Mailbox send error %d\n", rc);
+		goto err;
+	}
+
+	if (!wait_for_completion_timeout(&ctx->rd_complete,
+					 msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) {
+		dev_err(ctx->dev, "Mailbox operation timed out\n");
+		rc = -ETIMEDOUT;
+		goto err;
+	}
+
+	/* Check for error message */
+	if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) {
+		rc = -EINVAL;
+		goto err;
+	}
+
+	msg[0] = ctx->sync_msg.msg;
+	msg[1] = ctx->sync_msg.param1;
+	msg[2] = ctx->sync_msg.param2;
+
+err:
+	ctx->resp_pending = false;
+	mutex_unlock(&ctx->rd_mutex);
+	return rc;
+}
+
+static int xgene_hwmon_reg_map_rd(struct xgene_hwmon_dev *ctx, u32 addr,
+				  u32 *data)
+{
+	u32 msg[3];
+	int rc;
+
+	msg[0] = SENSOR_RD_MSG;
+	msg[1] = SENSOR_RD_EN_ADDR(addr);
+	msg[2] = 0;
+
+	if (acpi_disabled)
+		rc = xgene_hwmon_rd(ctx, msg);
+	else
+		rc = xgene_hwmon_pcc_rd(ctx, msg);
+
+	if (rc < 0)
+		return rc;
+
+	/*
+	 * Check if sensor data is valid.
+	 */
+	if (msg[1] & SENSOR_INVALID_DATA)
+		return -ENODATA;
+
+	*data = msg[1];
+
+	return rc;
+}
+
+static int xgene_hwmon_get_notification_msg(struct xgene_hwmon_dev *ctx,
+					    u32 *amsg)
+{
+	u32 msg[3];
+	int rc;
+
+	msg[0] = TPC_EN_MSG(PWRMGMT_SUBTYPE_TPC, TPC_GET_ALARM, 0);
+	msg[1] = 0;
+	msg[2] = 0;
+
+	rc = xgene_hwmon_pcc_rd(ctx, msg);
+	if (rc < 0)
+		return rc;
+
+	amsg[0] = msg[0];
+	amsg[1] = msg[1];
+	amsg[2] = msg[2];
+
+	return rc;
+}
+
+static int xgene_hwmon_get_cpu_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	u32 watt, mwatt;
+	int rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_REG, &watt);
+	if (rc < 0)
+		return rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, &mwatt);
+	if (rc < 0)
+		return rc;
+
+	*val = WATT_TO_mWATT(watt) + mwatt;
+	return 0;
+}
+
+static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	u32 watt, mwatt;
+	int rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, &watt);
+	if (rc < 0)
+		return rc;
+
+	rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_MW_REG, &mwatt);
+	if (rc < 0)
+		return rc;
+
+	*val = WATT_TO_mWATT(watt) + mwatt;
+	return 0;
+}
+
+static int xgene_hwmon_get_temp(struct xgene_hwmon_dev *ctx, u32 *val)
+{
+	return xgene_hwmon_reg_map_rd(ctx, SOC_TEMP_REG, val);
+}
+
+/*
+ * Sensor temperature/power functions
+ */
+static ssize_t temp1_input_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
+	int rc, temp;
+	u32 val;
+
+	rc = xgene_hwmon_get_temp(ctx, &val);
+	if (rc < 0)
+		return rc;
+
+	temp = sign_extend32(val, TEMP_NEGATIVE_BIT);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", CELSIUS_TO_mCELSIUS(temp));
+}
+
+static ssize_t temp1_label_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "SoC Temperature\n");
+}
+
+static ssize_t temp1_critical_alarm_show(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 power1_label_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "CPU power\n");
+}
+
+static ssize_t power2_label_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "IO power\n");
+}
+
+static ssize_t power1_input_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
+	u32 val;
+	int rc;
+
+	rc = xgene_hwmon_get_cpu_pwr(ctx, &val);
+	if (rc < 0)
+		return rc;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val));
+}
+
+static ssize_t power2_input_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
+	u32 val;
+	int rc;
+
+	rc = xgene_hwmon_get_io_pwr(ctx, &val);
+	if (rc < 0)
+		return rc;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val));
+}
+
+static DEVICE_ATTR_RO(temp1_label);
+static DEVICE_ATTR_RO(temp1_input);
+static DEVICE_ATTR_RO(temp1_critical_alarm);
+static DEVICE_ATTR_RO(power1_label);
+static DEVICE_ATTR_RO(power1_input);
+static DEVICE_ATTR_RO(power2_label);
+static DEVICE_ATTR_RO(power2_input);
+
+static struct attribute *xgene_hwmon_attrs[] = {
+	&dev_attr_temp1_label.attr,
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_critical_alarm.attr,
+	&dev_attr_power1_label.attr,
+	&dev_attr_power1_input.attr,
+	&dev_attr_power2_label.attr,
+	&dev_attr_power2_input.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(xgene_hwmon);
+
+static int xgene_hwmon_tpc_alarm(struct xgene_hwmon_dev *ctx,
+				 struct slimpro_resp_msg *amsg)
+{
+	ctx->temp_critical_alarm = !!amsg->param2;
+	sysfs_notify(&ctx->dev->kobj, NULL, "temp1_critical_alarm");
+
+	return 0;
+}
+
+static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx,
+				       struct slimpro_resp_msg *amsg)
+{
+	if ((MSG_SUBTYPE(amsg->msg) == PWRMGMT_SUBTYPE_TPC) &&
+	    (TPC_CMD(amsg->msg) == TPC_ALARM))
+		xgene_hwmon_tpc_alarm(ctx, amsg);
+}
+
+/*
+ * This function is called to process async work queue
+ */
+static void xgene_hwmon_evt_work(struct work_struct *work)
+{
+	struct slimpro_resp_msg amsg;
+	struct xgene_hwmon_dev *ctx;
+	int ret;
+
+	ctx = container_of(work, struct xgene_hwmon_dev, workq);
+	while (kfifo_out_spinlocked(&ctx->async_msg_fifo, &amsg,
+				    sizeof(struct slimpro_resp_msg),
+				    &ctx->kfifo_lock)) {
+		/*
+		 * If PCC, send a consumer command to Platform to get info
+		 * If Slimpro Mailbox, get message from specific FIFO
+		 */
+		if (!acpi_disabled) {
+			ret = xgene_hwmon_get_notification_msg(ctx,
+							       (u32 *)&amsg);
+			if (ret < 0)
+				continue;
+		}
+
+		if (MSG_TYPE(amsg.msg) == MSG_TYPE_PWRMGMT)
+			xgene_hwmon_process_pwrmsg(ctx, &amsg);
+	}
+}
+
+/*
+ * This function is called when the SLIMpro Mailbox received a message
+ */
+static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg)
+{
+	struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
+	struct slimpro_resp_msg amsg;
+
+	/*
+	 * Response message format:
+	 * msg[0] is the return code of the operation
+	 * msg[1] is the first parameter word
+	 * msg[2] is the second parameter word
+	 *
+	 * As message only supports dword size, just assign it.
+	 */
+
+	/* Check for sync query */
+	if (ctx->resp_pending &&
+	    ((MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_ERR) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_DBG &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == DBG_SUBTYPE_SENSOR_READ) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_PWRMGMT &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == PWRMGMT_SUBTYPE_TPC &&
+	      TPC_CMD(((u32 *)msg)[0]) == TPC_ALARM))) {
+		ctx->sync_msg.msg = ((u32 *)msg)[0];
+		ctx->sync_msg.param1 = ((u32 *)msg)[1];
+		ctx->sync_msg.param2 = ((u32 *)msg)[2];
+
+		/* Operation waiting for response */
+		complete(&ctx->rd_complete);
+
+		return;
+	}
+
+	amsg.msg   = ((u32 *)msg)[0];
+	amsg.param1 = ((u32 *)msg)[1];
+	amsg.param2 = ((u32 *)msg)[2];
+
+	/* Enqueue to the FIFO */
+	kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg,
+			    sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock);
+	/* Schedule the bottom handler */
+	schedule_work(&ctx->workq);
+}
+
+/*
+ * This function is called when the PCC Mailbox received a message
+ */
+static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
+{
+	struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
+	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct slimpro_resp_msg amsg;
+
+	msg = generic_comm_base + 1;
+	/* Check if platform sends interrupt */
+	if (!xgene_word_tst_and_clr(&generic_comm_base->status,
+				    PCCS_SCI_DOORBEL))
+		return;
+
+	/*
+	 * Response message format:
+	 * msg[0] is the return code of the operation
+	 * msg[1] is the first parameter word
+	 * msg[2] is the second parameter word
+	 *
+	 * As message only supports dword size, just assign it.
+	 */
+
+	/* Check for sync query */
+	if (ctx->resp_pending &&
+	    ((MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_ERR) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_DBG &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == DBG_SUBTYPE_SENSOR_READ) ||
+	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_PWRMGMT &&
+	      MSG_SUBTYPE(((u32 *)msg)[0]) == PWRMGMT_SUBTYPE_TPC &&
+	      TPC_CMD(((u32 *)msg)[0]) == TPC_ALARM))) {
+		/* Check if platform completes command */
+		if (xgene_word_tst_and_clr(&generic_comm_base->status,
+					   PCCS_CMD_COMPLETE)) {
+			ctx->sync_msg.msg = ((u32 *)msg)[0];
+			ctx->sync_msg.param1 = ((u32 *)msg)[1];
+			ctx->sync_msg.param2 = ((u32 *)msg)[2];
+
+			/* Operation waiting for response */
+			complete(&ctx->rd_complete);
+
+			return;
+		}
+	}
+
+	/*
+	 * Platform notifies interrupt to OSPM.
+	 * OPSM schedules a consumer command to get this information
+	 * in a workqueue. Platform must wait until OSPM has issued
+	 * a consumer command that serves this notification.
+	 */
+
+	/* Enqueue to the FIFO */
+	kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg,
+			    sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock);
+	/* Schedule the bottom handler */
+	schedule_work(&ctx->workq);
+}
+
+static void xgene_hwmon_tx_done(struct mbox_client *cl, void *msg, int ret)
+{
+	if (ret) {
+		dev_dbg(cl->dev, "TX did not complete: CMD sent:%x, ret:%d\n",
+			*(u16 *)msg, ret);
+	} else {
+		dev_dbg(cl->dev, "TX completed. CMD sent:%x, ret:%d\n",
+			*(u16 *)msg, ret);
+	}
+}
+
+static int xgene_hwmon_probe(struct platform_device *pdev)
+{
+	struct xgene_hwmon_dev *ctx;
+	struct mbox_client *cl;
+	int rc;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ctx);
+	cl = &ctx->mbox_client;
+
+	/* Request mailbox channel */
+	cl->dev = &pdev->dev;
+	cl->tx_done = xgene_hwmon_tx_done;
+	cl->tx_block = false;
+	cl->tx_tout = MBOX_OP_TIMEOUTMS;
+	cl->knows_txdone = false;
+	if (acpi_disabled) {
+		cl->rx_callback = xgene_hwmon_rx_cb;
+		ctx->mbox_chan = mbox_request_channel(cl, 0);
+		if (IS_ERR(ctx->mbox_chan)) {
+			dev_err(&pdev->dev,
+				"SLIMpro mailbox channel request failed\n");
+			return -ENODEV;
+		}
+	} else {
+		struct acpi_pcct_hw_reduced *cppc_ss;
+
+		if (device_property_read_u32(&pdev->dev, "pcc-channel",
+					     &ctx->mbox_idx)) {
+			dev_err(&pdev->dev, "no pcc-channel property\n");
+			return -ENODEV;
+		}
+
+		cl->rx_callback = xgene_hwmon_pcc_rx_cb;
+		ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx);
+		if (IS_ERR(ctx->mbox_chan)) {
+			dev_err(&pdev->dev,
+				"PPC channel request failed\n");
+			return -ENODEV;
+		}
+
+		/*
+		 * The PCC mailbox controller driver should
+		 * have parsed the PCCT (global table of all
+		 * PCC channels) and stored pointers to the
+		 * subspace communication region in con_priv.
+		 */
+		cppc_ss = ctx->mbox_chan->con_priv;
+		if (!cppc_ss) {
+			dev_err(&pdev->dev, "PPC subspace not found\n");
+			rc = -ENODEV;
+			goto out_mbox_free;
+		}
+
+		if (!ctx->mbox_chan->mbox->txdone_irq) {
+			dev_err(&pdev->dev, "PCC IRQ not supported\n");
+			rc = -ENODEV;
+			goto out_mbox_free;
+		}
+
+		/*
+		 * This is the shared communication region
+		 * for the OS and Platform to communicate over.
+		 */
+		ctx->comm_base_addr = cppc_ss->base_address;
+		if (ctx->comm_base_addr) {
+			ctx->pcc_comm_addr =
+					acpi_os_ioremap(ctx->comm_base_addr,
+							cppc_ss->length);
+		} else {
+			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
+			rc = -ENODEV;
+			goto out_mbox_free;
+		}
+
+		if (!ctx->pcc_comm_addr) {
+			dev_err(&pdev->dev,
+				"Failed to ioremap PCC comm region\n");
+			rc = -ENOMEM;
+			goto out_mbox_free;
+		}
+
+		/*
+		 * cppc_ss->latency is just a Nominal value. In reality
+		 * the remote processor could be much slower to reply.
+		 * So add an arbitrary amount of wait on top of Nominal.
+		 */
+		ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency;
+	}
+
+	spin_lock_init(&ctx->kfifo_lock);
+	mutex_init(&ctx->rd_mutex);
+
+	rc = kfifo_alloc(&ctx->async_msg_fifo,
+			 sizeof(struct slimpro_resp_msg) * ASYNC_MSG_FIFO_SIZE,
+			 GFP_KERNEL);
+	if (rc)
+		goto out_mbox_free;
+
+	INIT_WORK(&ctx->workq, xgene_hwmon_evt_work);
+
+	ctx->hwmon_dev = hwmon_device_register_with_groups(ctx->dev,
+							   "apm_xgene",
+							   ctx,
+							   xgene_hwmon_groups);
+	if (IS_ERR(ctx->hwmon_dev)) {
+		dev_err(&pdev->dev, "Failed to register HW monitor device\n");
+		rc = PTR_ERR(ctx->hwmon_dev);
+		goto out;
+	}
+
+	dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
+
+	return rc;
+
+out:
+	kfifo_free(&ctx->async_msg_fifo);
+out_mbox_free:
+	if (acpi_disabled)
+		mbox_free_channel(ctx->mbox_chan);
+	else
+		pcc_mbox_free_channel(ctx->mbox_chan);
+
+	return rc;
+}
+
+static int xgene_hwmon_remove(struct platform_device *pdev)
+{
+	struct xgene_hwmon_dev *ctx = platform_get_drvdata(pdev);
+
+	hwmon_device_unregister(ctx->hwmon_dev);
+	kfifo_free(&ctx->async_msg_fifo);
+	if (acpi_disabled)
+		mbox_free_channel(ctx->mbox_chan);
+	else
+		pcc_mbox_free_channel(ctx->mbox_chan);
+
+	return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id xgene_hwmon_acpi_match[] = {
+	{"APMC0D29", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match);
+#endif
+
+static const struct of_device_id xgene_hwmon_of_match[] = {
+	{.compatible = "apm,xgene-slimpro-hwmon"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, xgene_hwmon_of_match);
+
+static struct platform_driver xgene_hwmon_driver __refdata = {
+	.probe = xgene_hwmon_probe,
+	.remove = xgene_hwmon_remove,
+	.driver = {
+		.name = "xgene-slimpro-hwmon",
+		.of_match_table = xgene_hwmon_of_match,
+		.acpi_match_table = ACPI_PTR(xgene_hwmon_acpi_match),
+	},
+};
+module_platform_driver(xgene_hwmon_driver);
+
+MODULE_DESCRIPTION("APM X-Gene SoC hardware monitor");
+MODULE_LICENSE("GPL");
-- 
1.9.1

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

[Guenter Roeck]

On Thu, Jul 21, 2016 at 01:55:56PM -0700, Hoan Tran wrote:
> This patch adds hardware temperature and power reading support for
> APM X-Gene SoC using the mailbox communication interface.
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>
> ---

[ ... ]

> +
> +	dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
> +
> +	return rc;
> +

Nitpick: rc == 0 here, so return 0; is a better choice.
Otherwise looks good.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Note that I can not apply the patch at this time due to its dependency.

Guenter

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

[Guenter Roeck]

On Thu, Jul 21, 2016 at 01:55:56PM -0700, Hoan Tran wrote:
> This patch adds hardware temperature and power reading support for
> APM X-Gene SoC using the mailbox communication interface.
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>
> ---

[ ... ]

> +
> +	dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
> +
> +	return rc;
> +

Nitpick: rc == 0 here, so return 0; is a better choice.
Otherwise looks good.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Note that I can not apply the patch at this time due to its dependency.

Guenter

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

[Hoan Tran]

Hi Guenter,

On Thu, Jul 21, 2016 at 3:09 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Jul 21, 2016 at 01:55:56PM -0700, Hoan Tran wrote:
>> This patch adds hardware temperature and power reading support for
>> APM X-Gene SoC using the mailbox communication interface.
>>
>> Signed-off-by: Hoan Tran <hotran@apm.com>
>> ---
>
> [ ... ]
>
>> +
>> +     dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
>> +
>> +     return rc;
>> +
>
> Nitpick: rc == 0 here, so return 0; is a better choice.
> Otherwise looks good.

Yes, I'll fix it then send v4 shortly.

>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> Note that I can not apply the patch at this time due to its dependency.

Thanks ! I'll let you know when Rafael applies that dependent patch
into linux-next.

Thanks
Hoan

>
> Guenter

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

[Hoan Tran]

Hi Guenter,

On Thu, Jul 21, 2016 at 3:09 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Jul 21, 2016 at 01:55:56PM -0700, Hoan Tran wrote:
>> This patch adds hardware temperature and power reading support for
>> APM X-Gene SoC using the mailbox communication interface.
>>
>> Signed-off-by: Hoan Tran <hotran@apm.com>
>> ---
>
> [ ... ]
>
>> +
>> +     dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
>> +
>> +     return rc;
>> +
>
> Nitpick: rc == 0 here, so return 0; is a better choice.
> Otherwise looks good.

Yes, I'll fix it then send v4 shortly.

>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> Note that I can not apply the patch at this time due to its dependency.

Thanks ! I'll let you know when Rafael applies that dependent patch
into linux-next.

Thanks
Hoan

>
> Guenter

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

Hi Hoan,

[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.7]
[cannot apply to next-20160801]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or directory
   compilation terminated.

vim +38 drivers/hwmon/xgene-hwmon.c

    32	#include <linux/mailbox_controller.h>
    33	#include <linux/mailbox_client.h>
    34	#include <linux/module.h>
    35	#include <linux/of.h>
    36	#include <linux/platform_device.h>
    37	#include <acpi/acpi_io.h>
  > 38	#include <acpi/pcc.h>
    39	
    40	/* SLIMpro message defines */
    41	#define MSG_TYPE_DBG			0

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 50305 bytes --]

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

[kbuild test robot]

Hi Hoan,

[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.7]
[cannot apply to next-20160801]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or directory
   compilation terminated.

vim +38 drivers/hwmon/xgene-hwmon.c

    32	#include <linux/mailbox_controller.h>
    33	#include <linux/mailbox_client.h>
    34	#include <linux/module.h>
    35	#include <linux/of.h>
    36	#include <linux/platform_device.h>
    37	#include <acpi/acpi_io.h>
  > 38	#include <acpi/pcc.h>
    39	
    40	/* SLIMpro message defines */
    41	#define MSG_TYPE_DBG			0

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 50305 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160801/b7c0a236/attachment-0001.obj>

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

Hi Hoan,

[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.7]
[cannot apply to next-20160801]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or directory
   compilation terminated.

vim +38 drivers/hwmon/xgene-hwmon.c

    32	#include <linux/mailbox_controller.h>
    33	#include <linux/mailbox_client.h>
    34	#include <linux/module.h>
    35	#include <linux/of.h>
    36	#include <linux/platform_device.h>
    37	#include <acpi/acpi_io.h>
  > 38	#include <acpi/pcc.h>
    39	
    40	/* SLIMpro message defines */
    41	#define MSG_TYPE_DBG			0

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 50305 bytes --]

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

[Hoan Tran]

Hi,

On Mon, Aug 1, 2016 at 6:21 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Hoan,
>
> [auto build test ERROR on hwmon/hwmon-next]
> [also build test ERROR on v4.7]
> [cannot apply to next-20160801]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
> config: arm64-allmodconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm64
>
> All errors (new ones prefixed by >>):
>
>>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or directory
>    compilation terminated.

This driver should be built with kernel 4.8 and above where the
"pcc.h" file is available.

Thanks
Hoan

>
> vim +38 drivers/hwmon/xgene-hwmon.c
>
>     32  #include <linux/mailbox_controller.h>
>     33  #include <linux/mailbox_client.h>
>     34  #include <linux/module.h>
>     35  #include <linux/of.h>
>     36  #include <linux/platform_device.h>
>     37  #include <acpi/acpi_io.h>
>   > 38  #include <acpi/pcc.h>
>     39
>     40  /* SLIMpro message defines */
>     41  #define MSG_TYPE_DBG                    0
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[Hoan Tran]

Hi,

On Mon, Aug 1, 2016 at 6:21 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Hoan,
>
> [auto build test ERROR on hwmon/hwmon-next]
> [also build test ERROR on v4.7]
> [cannot apply to next-20160801]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
> config: arm64-allmodconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm64
>
> All errors (new ones prefixed by >>):
>
>>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or directory
>    compilation terminated.

This driver should be built with kernel 4.8 and above where the
"pcc.h" file is available.

Thanks
Hoan

>
> vim +38 drivers/hwmon/xgene-hwmon.c
>
>     32  #include <linux/mailbox_controller.h>
>     33  #include <linux/mailbox_client.h>
>     34  #include <linux/module.h>
>     35  #include <linux/of.h>
>     36  #include <linux/platform_device.h>
>     37  #include <acpi/acpi_io.h>
>   > 38  #include <acpi/pcc.h>
>     39
>     40  /* SLIMpro message defines */
>     41  #define MSG_TYPE_DBG                    0
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[Hoan Tran]

Hi,

On Mon, Aug 1, 2016 at 6:21 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Hoan,
>
> [auto build test ERROR on hwmon/hwmon-next]
> [also build test ERROR on v4.7]
> [cannot apply to next-20160801]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
> config: arm64-allmodconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm64
>
> All errors (new ones prefixed by >>):
>
>>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or directory
>    compilation terminated.

This driver should be built with kernel 4.8 and above where the
"pcc.h" file is available.

Thanks
Hoan

>
> vim +38 drivers/hwmon/xgene-hwmon.c
>
>     32  #include <linux/mailbox_controller.h>
>     33  #include <linux/mailbox_client.h>
>     34  #include <linux/module.h>
>     35  #include <linux/of.h>
>     36  #include <linux/platform_device.h>
>     37  #include <acpi/acpi_io.h>
>   > 38  #include <acpi/pcc.h>
>     39
>     40  /* SLIMpro message defines */
>     41  #define MSG_TYPE_DBG                    0
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[Arnd Bergmann]

On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> +               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);
> 

This causes the arm64 allmodconfig build to fail now, according to
kernelci:

      1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

Should this perhaps call ioremap() or memremap() instead?

	Arnd

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

[Arnd Bergmann]

On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> +               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);
> 

This causes the arm64 allmodconfig build to fail now, according to
kernelci:

      1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

Should this perhaps call ioremap() or memremap() instead?

	Arnd

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

[Arnd Bergmann]

On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> +               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);
> 

This causes the arm64 allmodconfig build to fail now, according to
kernelci:

      1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

Should this perhaps call ioremap() or memremap() instead?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +               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);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
How do you even get there ? arm64:allmodconfig fails for me in -next with

drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member named ‘chips’

Guenter

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

[Guenter Roeck]

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +               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);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
How do you even get there ? arm64:allmodconfig fails for me in -next with

drivers/pwm/pwm-berlin.c: In function ?berlin_pwm_suspend?:
drivers/pwm/pwm-berlin.c:245:35: error: ?struct berlin_pwm_chip? has no member named ?chips?

Guenter

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

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               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);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> How do you even get there ? arm64:allmodconfig fails for me in -next with
> 
> drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
> drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member named ‘chips’

That was fixed in yesterday's linux-next.

	Arnd

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

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               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);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> How do you even get there ? arm64:allmodconfig fails for me in -next with
> 
> drivers/pwm/pwm-berlin.c: In function ?berlin_pwm_suspend?:
> drivers/pwm/pwm-berlin.c:245:35: error: ?struct berlin_pwm_chip? has no member named ?chips?

That was fixed in yesterday's linux-next.

	Arnd

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

[Guenter Roeck]

On 09/08/2016 01:15 AM, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>> +               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);
>>>>
>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>
>> How do you even get there ? arm64:allmodconfig fails for me in -next with
>>
>> drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
>> drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member named ‘chips’
>
> That was fixed in yesterday's linux-next.
>

Maybe, but now I get

Building arm:allmodconfig ... failed
--------------
Error log:
crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
crypto/crypto_engine.c:234:3: error: implicit declaration of function 'queue_kthread_work'

Building arm64:allmodconfig ... failed
--------------
Error log:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function ‘kvm_timer_hyp_init’:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label ‘out_free’ defined but not used

Guenter

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

[Guenter Roeck]

On 09/08/2016 01:15 AM, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>> +               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);
>>>>
>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>
>> How do you even get there ? arm64:allmodconfig fails for me in -next with
>>
>> drivers/pwm/pwm-berlin.c: In function ?berlin_pwm_suspend?:
>> drivers/pwm/pwm-berlin.c:245:35: error: ?struct berlin_pwm_chip? has no member named ?chips?
>
> That was fixed in yesterday's linux-next.
>

Maybe, but now I get

Building arm:allmodconfig ... failed
--------------
Error log:
crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
crypto/crypto_engine.c:234:3: error: implicit declaration of function 'queue_kthread_work'

Building arm64:allmodconfig ... failed
--------------
Error log:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function ?kvm_timer_hyp_init?:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label ?out_free? defined but not used

Guenter

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

[Arnd Bergmann]

On Thursday, September 8, 2016 7:55:44 AM CEST Guenter Roeck wrote:
> Maybe, but now I get
> 
> Building arm:allmodconfig ... failed
> --------------
> Error log:
> crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
> crypto/crypto_engine.c:234:3: error: implicit declaration of function 'queue_kthread_work'
> 
> Building arm64:allmodconfig ... failed
> --------------
> Error log:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function ‘kvm_timer_hyp_init’:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label ‘out_free’ defined but not used

Right, I sent fixes for both. I think we had a clean build yesterday,
probably tomorrow those will both be fixed.

The time between -rc5 and -rc7 seems to be the worst for build testing ;-)

	Arnd

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

[Arnd Bergmann]

On Thursday, September 8, 2016 7:55:44 AM CEST Guenter Roeck wrote:
> Maybe, but now I get
> 
> Building arm:allmodconfig ... failed
> --------------
> Error log:
> crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
> crypto/crypto_engine.c:234:3: error: implicit declaration of function 'queue_kthread_work'
> 
> Building arm64:allmodconfig ... failed
> --------------
> Error log:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function ?kvm_timer_hyp_init?:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label ?out_free? defined but not used

Right, I sent fixes for both. I think we had a clean build yesterday,
probably tomorrow those will both be fixed.

The time between -rc5 and -rc7 seems to be the worst for build testing ;-)

	Arnd

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

[Guenter Roeck]

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +               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);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> Should this perhaps call ioremap() or memremap() instead?
> 
Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
mark reserved memblock regions explicitly in iomem") starts using a function
in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
is declared as __init_memblock, which makes me really uncomfortable.
If acpi_os_ioremap() must not be used by modules, and possibly only during
early (?) initialization, maybe its declaration should state those limitations ?

Thanks,
Guenter

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

[Guenter Roeck]

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +               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);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> Should this perhaps call ioremap() or memremap() instead?
> 
Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
mark reserved memblock regions explicitly in iomem") starts using a function
in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
is declared as __init_memblock, which makes me really uncomfortable.
If acpi_os_ioremap() must not be used by modules, and possibly only during
early (?) initialization, maybe its declaration should state those limitations ?

Thanks,
Guenter

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

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               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);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those limitations ?

Ah, I didn't notice that. I guess both patches were correct individually and
got added to linux-next around the same time but caused allmodconfig to blow up
when used together.

Adding everyone who was involved in the memblock patch to Cc here, maybe one
of them has an idea what the correct fix is. There are only two other drivers
using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
that drivers are not actually supposed to use this symbol. Making
acpi_os_ioremap() an exported function in arm64 would also work.

	Arnd

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

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               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);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those limitations ?

Ah, I didn't notice that. I guess both patches were correct individually and
got added to linux-next around the same time but caused allmodconfig to blow up
when used together.

Adding everyone who was involved in the memblock patch to Cc here, maybe one
of them has an idea what the correct fix is. There are only two other drivers
using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
that drivers are not actually supposed to use this symbol. Making
acpi_os_ioremap() an exported function in arm64 would also work.

	Arnd

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

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               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);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those limitations ?

Ah, I didn't notice that. I guess both patches were correct individually and
got added to linux-next around the same time but caused allmodconfig to blow up
when used together.

Adding everyone who was involved in the memblock patch to Cc here, maybe one
of them has an idea what the correct fix is. There are only two other drivers
using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
that drivers are not actually supposed to use this symbol. Making
acpi_os_ioremap() an exported function in arm64 would also work.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[James Morse]

Hi,

On 08/09/16 09:14, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>> +               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);
>>>>
>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>
>>> Should this perhaps call ioremap() or memremap() instead?
>>>
>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
>> mark reserved memblock regions explicitly in iomem") starts using a function
>> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
>> is declared as __init_memblock, which makes me really uncomfortable.
>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>> early (?) initialization, maybe its declaration should state those limitations ?
> 
> Ah, I didn't notice that. I guess both patches were correct individually and
> got added to linux-next around the same time but caused allmodconfig to blow up
> when used together.
> 
> Adding everyone who was involved in the memblock patch to Cc here, maybe one
> of them has an idea what the correct fix is. There are only two other drivers
> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> that drivers are not actually supposed to use this symbol. Making
> acpi_os_ioremap() an exported function in arm64 would also work.

You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
already EXPORT_SYMBOL_GPL().

(I'm still waiting for allmodconfig on linux-next to finish building)


Thanks,

James

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

[James Morse]

Hi,

On 08/09/16 09:14, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>> +               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);
>>>>
>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>
>>> Should this perhaps call ioremap() or memremap() instead?
>>>
>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
>> mark reserved memblock regions explicitly in iomem") starts using a function
>> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
>> is declared as __init_memblock, which makes me really uncomfortable.
>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>> early (?) initialization, maybe its declaration should state those limitations ?
> 
> Ah, I didn't notice that. I guess both patches were correct individually and
> got added to linux-next around the same time but caused allmodconfig to blow up
> when used together.
> 
> Adding everyone who was involved in the memblock patch to Cc here, maybe one
> of them has an idea what the correct fix is. There are only two other drivers
> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> that drivers are not actually supposed to use this symbol. Making
> acpi_os_ioremap() an exported function in arm64 would also work.

You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
already EXPORT_SYMBOL_GPL().

(I'm still waiting for allmodconfig on linux-next to finish building)


Thanks,

James

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

[James Morse]

Hi,

On 08/09/16 09:14, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>> +               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);
>>>>
>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>
>>> Should this perhaps call ioremap() or memremap() instead?
>>>
>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
>> mark reserved memblock regions explicitly in iomem") starts using a function
>> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
>> is declared as __init_memblock, which makes me really uncomfortable.
>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>> early (?) initialization, maybe its declaration should state those limitations ?
> 
> Ah, I didn't notice that. I guess both patches were correct individually and
> got added to linux-next around the same time but caused allmodconfig to blow up
> when used together.
> 
> Adding everyone who was involved in the memblock patch to Cc here, maybe one
> of them has an idea what the correct fix is. There are only two other drivers
> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> that drivers are not actually supposed to use this symbol. Making
> acpi_os_ioremap() an exported function in arm64 would also work.

You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
already EXPORT_SYMBOL_GPL().

(I'm still waiting for allmodconfig on linux-next to finish building)


Thanks,

James


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[AKASHI Takahiro]

On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
> Hi,
> 
> On 08/09/16 09:14, Arnd Bergmann wrote:
> > On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> >> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> >>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> >>>> +               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);
> >>>>
> >>>
> >>> This causes the arm64 allmodconfig build to fail now, according to
> >>> kernelci:
> >>>
> >>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> >>>
> >>> Should this perhaps call ioremap() or memremap() instead?
> >>>
> >> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> >> mark reserved memblock regions explicitly in iomem") starts using a function
> >> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> >> is declared as __init_memblock, which makes me really uncomfortable.
> >> If acpi_os_ioremap() must not be used by modules, and possibly only during
> >> early (?) initialization, maybe its declaration should state those limitations ?
> > 
> > Ah, I didn't notice that. I guess both patches were correct individually and
> > got added to linux-next around the same time but caused allmodconfig to blow up
> > when used together.
> > 
> > Adding everyone who was involved in the memblock patch to Cc here, maybe one
> > of them has an idea what the correct fix is. There are only two other drivers
> > using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> > that drivers are not actually supposed to use this symbol. Making
> > acpi_os_ioremap() an exported function in arm64 would also work.
> 
> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
> already EXPORT_SYMBOL_GPL().

acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.

The problem is that, as memblock_is_memory() is declared as __init,
we cannot build any drivers which call acpi_os_ioremap() as modules.

As far as this specific issue is concerned, if we make a change like:

===8<===
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -207,7 +207,7 @@ static void __init request_standard_resources(void)
                res = alloc_bootmem_low(sizeof(*res));
                if (memblock_is_nomap(region)) {
                        res->name  = "reserved";
-                       res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+                       res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
                } else {
                        res->name  = "System RAM";
                        res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
===>8===

and revert the following hunk from the commit:

===8<===
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,7 +12,7 @@
 #ifndef _ASM_ACPI_H
 #define _ASM_ACPI_H
 
-#include <linux/mm.h>
+#include <linux/memblock.h>
 #include <linux/psci.h>
 
 #include <asm/cputype.h>
@@ -32,7 +32,11 @@
 static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
                                            acpi_size size)
 {
-       if (!page_is_ram(phys >> PAGE_SHIFT))
+       /*
+        * EFI's reserve_regions() call adds memory with the WB attribute
+        * to memblock via early_init_dt_add_memory_arch().
+        */
+       if (!memblock_is_memory(phys))
                return ioremap(phys, size);
 
        return ioremap_cache(phys, size);
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
===>8===

The build error will be gone. (and still kdump should work.)

But I don't know how we should distinguish IORESOURCE_MEM and
IORESOURCE_SYSTEM_RAM.

Thanks,
-Takahiro AKASHI

> (I'm still waiting for allmodconfig on linux-next to finish building)
> 
> 
> Thanks,
> 
> James
> 
> 

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

[AKASHI Takahiro]

On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
> Hi,
> 
> On 08/09/16 09:14, Arnd Bergmann wrote:
> > On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> >> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> >>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> >>>> +               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);
> >>>>
> >>>
> >>> This causes the arm64 allmodconfig build to fail now, according to
> >>> kernelci:
> >>>
> >>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> >>>
> >>> Should this perhaps call ioremap() or memremap() instead?
> >>>
> >> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> >> mark reserved memblock regions explicitly in iomem") starts using a function
> >> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> >> is declared as __init_memblock, which makes me really uncomfortable.
> >> If acpi_os_ioremap() must not be used by modules, and possibly only during
> >> early (?) initialization, maybe its declaration should state those limitations ?
> > 
> > Ah, I didn't notice that. I guess both patches were correct individually and
> > got added to linux-next around the same time but caused allmodconfig to blow up
> > when used together.
> > 
> > Adding everyone who was involved in the memblock patch to Cc here, maybe one
> > of them has an idea what the correct fix is. There are only two other drivers
> > using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> > that drivers are not actually supposed to use this symbol. Making
> > acpi_os_ioremap() an exported function in arm64 would also work.
> 
> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
> already EXPORT_SYMBOL_GPL().

acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.

The problem is that, as memblock_is_memory() is declared as __init,
we cannot build any drivers which call acpi_os_ioremap() as modules.

As far as this specific issue is concerned, if we make a change like:

===8<===
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -207,7 +207,7 @@ static void __init request_standard_resources(void)
                res = alloc_bootmem_low(sizeof(*res));
                if (memblock_is_nomap(region)) {
                        res->name  = "reserved";
-                       res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+                       res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
                } else {
                        res->name  = "System RAM";
                        res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
===>8===

and revert the following hunk from the commit:

===8<===
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,7 +12,7 @@
 #ifndef _ASM_ACPI_H
 #define _ASM_ACPI_H
 
-#include <linux/mm.h>
+#include <linux/memblock.h>
 #include <linux/psci.h>
 
 #include <asm/cputype.h>
@@ -32,7 +32,11 @@
 static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
                                            acpi_size size)
 {
-       if (!page_is_ram(phys >> PAGE_SHIFT))
+       /*
+        * EFI's reserve_regions() call adds memory with the WB attribute
+        * to memblock via early_init_dt_add_memory_arch().
+        */
+       if (!memblock_is_memory(phys))
                return ioremap(phys, size);
 
        return ioremap_cache(phys, size);
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
===>8===

The build error will be gone. (and still kdump should work.)

But I don't know how we should distinguish IORESOURCE_MEM and
IORESOURCE_SYSTEM_RAM.

Thanks,
-Takahiro AKASHI

> (I'm still waiting for allmodconfig on linux-next to finish building)
> 
> 
> Thanks,
> 
> James
> 
> 

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

[James Morse]

Hi,

On 09/09/16 04:18, AKASHI Takahiro wrote:
> On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
>> On 08/09/16 09:14, Arnd Bergmann wrote:
>>> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>>>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>>>> +               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);
>>>>>>
>>>>>
>>>>> This causes the arm64 allmodconfig build to fail now, according to
>>>>> kernelci:
>>>>>
>>>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>>>
>>>>> Should this perhaps call ioremap() or memremap() instead?
>>>>>
>>>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
>>>> mark reserved memblock regions explicitly in iomem") starts using a function
>>>> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
>>>> is declared as __init_memblock, which makes me really uncomfortable.
>>>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>>>> early (?) initialization, maybe its declaration should state those limitations ?
>>>
>>> Ah, I didn't notice that. I guess both patches were correct individually and
>>> got added to linux-next around the same time but caused allmodconfig to blow up
>>> when used together.
>>>
>>> Adding everyone who was involved in the memblock patch to Cc here, maybe one
>>> of them has an idea what the correct fix is. There are only two other drivers
>>> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
>>> that drivers are not actually supposed to use this symbol. Making
>>> acpi_os_ioremap() an exported function in arm64 would also work.
>>
>> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
>> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
>> already EXPORT_SYMBOL_GPL().
> 
> acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.
> 
> The problem is that, as memblock_is_memory() is declared as __init,

__init_memblock ...

... as is memblock_is_map_memory(), which we call from pfn_valid() which is
EXPORT_SYMBOL()'d
and used from modules, (e.g. mac80211.ko). So something fishy is going on...

>From include/linux/memblock.h:
> #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> #define __init_memblock __meminit
> #define __initdata_memblock __meminitdata
> #else
> #define __init_memblock
> #define __initdata_memblock
> #endif

arm64 doesn't define ARCH_DISCARD_MEMBLOCK, so we always keep these symbols.
If we didn't, pfn_valid() would break too.


Thanks,

James

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

[James Morse]

Hi,

On 09/09/16 04:18, AKASHI Takahiro wrote:
> On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
>> On 08/09/16 09:14, Arnd Bergmann wrote:
>>> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>>>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>>>> +               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);
>>>>>>
>>>>>
>>>>> This causes the arm64 allmodconfig build to fail now, according to
>>>>> kernelci:
>>>>>
>>>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>>>
>>>>> Should this perhaps call ioremap() or memremap() instead?
>>>>>
>>>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
>>>> mark reserved memblock regions explicitly in iomem") starts using a function
>>>> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
>>>> is declared as __init_memblock, which makes me really uncomfortable.
>>>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>>>> early (?) initialization, maybe its declaration should state those limitations ?
>>>
>>> Ah, I didn't notice that. I guess both patches were correct individually and
>>> got added to linux-next around the same time but caused allmodconfig to blow up
>>> when used together.
>>>
>>> Adding everyone who was involved in the memblock patch to Cc here, maybe one
>>> of them has an idea what the correct fix is. There are only two other drivers
>>> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
>>> that drivers are not actually supposed to use this symbol. Making
>>> acpi_os_ioremap() an exported function in arm64 would also work.
>>
>> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
>> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
>> already EXPORT_SYMBOL_GPL().
> 
> acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.
> 
> The problem is that, as memblock_is_memory() is declared as __init,

__init_memblock ...

... as is memblock_is_map_memory(), which we call from pfn_valid() which is
EXPORT_SYMBOL()'d
and used from modules, (e.g. mac80211.ko). So something fishy is going on...

>From include/linux/memblock.h:
> #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> #define __init_memblock __meminit
> #define __initdata_memblock __meminitdata
> #else
> #define __init_memblock
> #define __initdata_memblock
> #endif

arm64 doesn't define ARCH_DISCARD_MEMBLOCK, so we always keep these symbols.
If we didn't, pfn_valid() would break too.


Thanks,

James

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

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               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);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those limitations ?

I think there is more wrong with it, the driver also accesses a shared
memory area with kernel pointers using readl_relaxed/writel_relaxed,
which are only valid on MMIO registers.

I've prepared a patch, please have a look at the follow-up email.

	Arnd

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

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               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);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those limitations ?

I think there is more wrong with it, the driver also accesses a shared
memory area with kernel pointers using readl_relaxed/writel_relaxed,
which are only valid on MMIO registers.

I've prepared a patch, please have a look at the follow-up email.

	Arnd

[PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

The newly added hwmon driver fails to build in an allmodconfig
kernel:

      1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

According to comments in the code, the mailbox is a shared memory region,
not a set of MMIO registers, so we should use memremap() for mapping it
instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual
to work with functions that operate on __iomem pointers, and this
fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
regarding the ordering of the accesses from the CPU, but note that
there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether
the message data was supposed to be in LE32 format in the first
place, it's possible this was meant to be interpreted as a byte
stream instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index bc78a5d10182..e834dfb3acca 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -34,7 +34,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+#include <linux/io.h>
+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */
@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
 	u16 ret, val;
 
-	val = readw_relaxed(addr);
+	val = le16_to_cpu(READ_ONCE(*addr));
 	ret = val & mask;
 	val &= ~mask;
-	writew_relaxed(val, addr);
+	WRITE_ONCE(*addr, cpu_to_le16(val));
 
 	return ret;
 }
@@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 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;
+	u32 *ptr = (void*)(generic_comm_base + 1);
 	int rc, i;
 	u16 val;
 
@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 	ctx->resp_pending = true;
 
 	/* Write signature for subspace */
-	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-		       &generic_comm_base->signature);
+	WRITE_ONCE(generic_comm_base->signature,
+		   cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
 	/* Write to the shared command region */
-	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-		       &generic_comm_base->command);
+	WRITE_ONCE(generic_comm_base->command,
+		   cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
 	/* Flip CMD COMPLETE bit */
-	val = readw_relaxed(&generic_comm_base->status);
+	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
 	val &= ~PCCS_CMD_COMPLETE;
-	writew_relaxed(val, &generic_comm_base->status);
+	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
 	/* Copy the message to the PCC comm space */
 	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-		writel_relaxed(msg[i], ptr + i * 4);
+		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
 	/* Ring the doorbell */
 	rc = mbox_send_message(ctx->mbox_chan, msg);
@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		 */
 		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);
+			ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+							cppc_ss->length,
+							MEMREMAP_WT);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
 			rc = -ENODEV;

[PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

The newly added hwmon driver fails to build in an allmodconfig
kernel:

      1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

According to comments in the code, the mailbox is a shared memory region,
not a set of MMIO registers, so we should use memremap() for mapping it
instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual
to work with functions that operate on __iomem pointers, and this
fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
regarding the ordering of the accesses from the CPU, but note that
there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether
the message data was supposed to be in LE32 format in the first
place, it's possible this was meant to be interpreted as a byte
stream instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index bc78a5d10182..e834dfb3acca 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -34,7 +34,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+#include <linux/io.h>
+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */
@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
 	u16 ret, val;
 
-	val = readw_relaxed(addr);
+	val = le16_to_cpu(READ_ONCE(*addr));
 	ret = val & mask;
 	val &= ~mask;
-	writew_relaxed(val, addr);
+	WRITE_ONCE(*addr, cpu_to_le16(val));
 
 	return ret;
 }
@@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 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;
+	u32 *ptr = (void*)(generic_comm_base + 1);
 	int rc, i;
 	u16 val;
 
@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 	ctx->resp_pending = true;
 
 	/* Write signature for subspace */
-	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-		       &generic_comm_base->signature);
+	WRITE_ONCE(generic_comm_base->signature,
+		   cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
 	/* Write to the shared command region */
-	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-		       &generic_comm_base->command);
+	WRITE_ONCE(generic_comm_base->command,
+		   cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
 	/* Flip CMD COMPLETE bit */
-	val = readw_relaxed(&generic_comm_base->status);
+	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
 	val &= ~PCCS_CMD_COMPLETE;
-	writew_relaxed(val, &generic_comm_base->status);
+	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
 	/* Copy the message to the PCC comm space */
 	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-		writel_relaxed(msg[i], ptr + i * 4);
+		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
 	/* Ring the doorbell */
 	rc = mbox_send_message(ctx->mbox_chan, msg);
@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		 */
 		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);
+			ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+							cppc_ss->length,
+							MEMREMAP_WT);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
 			rc = -ENODEV;

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Guenter Roeck]

Hi Arnd,

On Fri, Sep 09, 2016 at 05:38:58PM +0200, Arnd Bergmann wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
> 
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
> 
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
> 
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 

Thanks a lot for looking into this.

I'll apply this patch to address the build problem. Much better than
my rude "depends on BROKEN". It would be great to get a Tested-by:
from someone with access to the hardware.

Guenter

> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e834dfb3acca 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -34,7 +34,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <acpi/acpi_io.h>
> +#include <linux/io.h>
> +
>  #include <acpi/pcc.h>
>  
>  /* SLIMpro message defines */
> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  {
>  	u16 ret, val;
>  
> -	val = readw_relaxed(addr);
> +	val = le16_to_cpu(READ_ONCE(*addr));
>  	ret = val & mask;
>  	val &= ~mask;
> -	writew_relaxed(val, addr);
> +	WRITE_ONCE(*addr, cpu_to_le16(val));
>  
>  	return ret;
>  }
> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  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;
> +	u32 *ptr = (void*)(generic_comm_base + 1);
>  	int rc, i;
>  	u16 val;
>  
> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>  	ctx->resp_pending = true;
>  
>  	/* Write signature for subspace */
> -	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
> -		       &generic_comm_base->signature);
> +	WRITE_ONCE(generic_comm_base->signature,
> +		   cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>  
>  	/* Write to the shared command region */
> -	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
> -		       &generic_comm_base->command);
> +	WRITE_ONCE(generic_comm_base->command,
> +		   cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>  
>  	/* Flip CMD COMPLETE bit */
> -	val = readw_relaxed(&generic_comm_base->status);
> +	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>  	val &= ~PCCS_CMD_COMPLETE;
> -	writew_relaxed(val, &generic_comm_base->status);
> +	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>  
>  	/* Copy the message to the PCC comm space */
>  	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -		writel_relaxed(msg[i], ptr + i * 4);
> +		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>  
>  	/* Ring the doorbell */
>  	rc = mbox_send_message(ctx->mbox_chan, msg);
> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>  		 */
>  		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);
> +			ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> +							cppc_ss->length,
> +							MEMREMAP_WT);
>  		} else {
>  			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>  			rc = -ENODEV;
> 

[PATCH] hwmon: xgene: access mailbox as RAM

[Guenter Roeck]

Hi Arnd,

On Fri, Sep 09, 2016 at 05:38:58PM +0200, Arnd Bergmann wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
> 
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
> 
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
> 
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 

Thanks a lot for looking into this.

I'll apply this patch to address the build problem. Much better than
my rude "depends on BROKEN". It would be great to get a Tested-by:
from someone with access to the hardware.

Guenter

> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e834dfb3acca 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -34,7 +34,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <acpi/acpi_io.h>
> +#include <linux/io.h>
> +
>  #include <acpi/pcc.h>
>  
>  /* SLIMpro message defines */
> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  {
>  	u16 ret, val;
>  
> -	val = readw_relaxed(addr);
> +	val = le16_to_cpu(READ_ONCE(*addr));
>  	ret = val & mask;
>  	val &= ~mask;
> -	writew_relaxed(val, addr);
> +	WRITE_ONCE(*addr, cpu_to_le16(val));
>  
>  	return ret;
>  }
> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  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;
> +	u32 *ptr = (void*)(generic_comm_base + 1);
>  	int rc, i;
>  	u16 val;
>  
> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>  	ctx->resp_pending = true;
>  
>  	/* Write signature for subspace */
> -	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
> -		       &generic_comm_base->signature);
> +	WRITE_ONCE(generic_comm_base->signature,
> +		   cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>  
>  	/* Write to the shared command region */
> -	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
> -		       &generic_comm_base->command);
> +	WRITE_ONCE(generic_comm_base->command,
> +		   cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>  
>  	/* Flip CMD COMPLETE bit */
> -	val = readw_relaxed(&generic_comm_base->status);
> +	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>  	val &= ~PCCS_CMD_COMPLETE;
> -	writew_relaxed(val, &generic_comm_base->status);
> +	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>  
>  	/* Copy the message to the PCC comm space */
>  	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -		writel_relaxed(msg[i], ptr + i * 4);
> +		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>  
>  	/* Ring the doorbell */
>  	rc = mbox_send_message(ctx->mbox_chan, msg);
> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>  		 */
>  		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);
> +			ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> +							cppc_ss->length,
> +							MEMREMAP_WT);
>  		} else {
>  			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>  			rc = -ENODEV;
> 

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 9:58 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi Arnd,
>
> On Fri, Sep 09, 2016 at 05:38:58PM +0200, Arnd Bergmann wrote:
>> The newly added hwmon driver fails to build in an allmodconfig
>> kernel:
>>
>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>
>> According to comments in the code, the mailbox is a shared memory region,
>> not a set of MMIO registers, so we should use memremap() for mapping it
>> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
>> of readl/writel.
>>
>> The driver already uses plain kernel pointers, so it's a bit unusual
>> to work with functions that operate on __iomem pointers, and this
>> fixes that part too.
>>
>> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
>> regarding the ordering of the accesses from the CPU, but note that
>> there are no barriers (also unchanged from before).
>>
>> I'm also keeping the endianess behavior, though I'm unsure whether
>> the message data was supposed to be in LE32 format in the first
>> place, it's possible this was meant to be interpreted as a byte
>> stream instead.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>
> Thanks a lot for looking into this.
>
> I'll apply this patch to address the build problem. Much better than
> my rude "depends on BROKEN". It would be great to get a Tested-by:
> from someone with access to the hardware.
>

Hi Arnd and Guenter,

Thanks for the patch. I'm testing it out.

Hoan

> Guenter
>
>> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
>> index bc78a5d10182..e834dfb3acca 100644
>> --- a/drivers/hwmon/xgene-hwmon.c
>> +++ b/drivers/hwmon/xgene-hwmon.c
>> @@ -34,7 +34,8 @@
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/platform_device.h>
>> -#include <acpi/acpi_io.h>
>> +#include <linux/io.h>
>> +
>>  #include <acpi/pcc.h>
>>
>>  /* SLIMpro message defines */
>> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>>  {
>>       u16 ret, val;
>>
>> -     val = readw_relaxed(addr);
>> +     val = le16_to_cpu(READ_ONCE(*addr));
>>       ret = val & mask;
>>       val &= ~mask;
>> -     writew_relaxed(val, addr);
>> +     WRITE_ONCE(*addr, cpu_to_le16(val));
>>
>>       return ret;
>>  }
>> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>>  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;
>> +     u32 *ptr = (void*)(generic_comm_base + 1);
>>       int rc, i;
>>       u16 val;
>>
>> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>>       ctx->resp_pending = true;
>>
>>       /* Write signature for subspace */
>> -     writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
>> -                    &generic_comm_base->signature);
>> +     WRITE_ONCE(generic_comm_base->signature,
>> +                cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>>
>>       /* Write to the shared command region */
>> -     writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
>> -                    &generic_comm_base->command);
>> +     WRITE_ONCE(generic_comm_base->command,
>> +                cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>>
>>       /* Flip CMD COMPLETE bit */
>> -     val = readw_relaxed(&generic_comm_base->status);
>> +     val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>>       val &= ~PCCS_CMD_COMPLETE;
>> -     writew_relaxed(val, &generic_comm_base->status);
>> +     WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>>
>>       /* Copy the message to the PCC comm space */
>>       for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
>> -             writel_relaxed(msg[i], ptr + i * 4);
>> +             WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>>
>>       /* Ring the doorbell */
>>       rc = mbox_send_message(ctx->mbox_chan, msg);
>> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>>                */
>>               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);
>> +                     ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
>> +                                                     cppc_ss->length,
>> +                                                     MEMREMAP_WT);
>>               } else {
>>                       dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>>                       rc = -ENODEV;
>>

[PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 9:58 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi Arnd,
>
> On Fri, Sep 09, 2016 at 05:38:58PM +0200, Arnd Bergmann wrote:
>> The newly added hwmon driver fails to build in an allmodconfig
>> kernel:
>>
>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>
>> According to comments in the code, the mailbox is a shared memory region,
>> not a set of MMIO registers, so we should use memremap() for mapping it
>> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
>> of readl/writel.
>>
>> The driver already uses plain kernel pointers, so it's a bit unusual
>> to work with functions that operate on __iomem pointers, and this
>> fixes that part too.
>>
>> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
>> regarding the ordering of the accesses from the CPU, but note that
>> there are no barriers (also unchanged from before).
>>
>> I'm also keeping the endianess behavior, though I'm unsure whether
>> the message data was supposed to be in LE32 format in the first
>> place, it's possible this was meant to be interpreted as a byte
>> stream instead.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>
> Thanks a lot for looking into this.
>
> I'll apply this patch to address the build problem. Much better than
> my rude "depends on BROKEN". It would be great to get a Tested-by:
> from someone with access to the hardware.
>

Hi Arnd and Guenter,

Thanks for the patch. I'm testing it out.

Hoan

> Guenter
>
>> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
>> index bc78a5d10182..e834dfb3acca 100644
>> --- a/drivers/hwmon/xgene-hwmon.c
>> +++ b/drivers/hwmon/xgene-hwmon.c
>> @@ -34,7 +34,8 @@
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/platform_device.h>
>> -#include <acpi/acpi_io.h>
>> +#include <linux/io.h>
>> +
>>  #include <acpi/pcc.h>
>>
>>  /* SLIMpro message defines */
>> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>>  {
>>       u16 ret, val;
>>
>> -     val = readw_relaxed(addr);
>> +     val = le16_to_cpu(READ_ONCE(*addr));
>>       ret = val & mask;
>>       val &= ~mask;
>> -     writew_relaxed(val, addr);
>> +     WRITE_ONCE(*addr, cpu_to_le16(val));
>>
>>       return ret;
>>  }
>> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>>  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;
>> +     u32 *ptr = (void*)(generic_comm_base + 1);
>>       int rc, i;
>>       u16 val;
>>
>> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>>       ctx->resp_pending = true;
>>
>>       /* Write signature for subspace */
>> -     writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
>> -                    &generic_comm_base->signature);
>> +     WRITE_ONCE(generic_comm_base->signature,
>> +                cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>>
>>       /* Write to the shared command region */
>> -     writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
>> -                    &generic_comm_base->command);
>> +     WRITE_ONCE(generic_comm_base->command,
>> +                cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>>
>>       /* Flip CMD COMPLETE bit */
>> -     val = readw_relaxed(&generic_comm_base->status);
>> +     val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>>       val &= ~PCCS_CMD_COMPLETE;
>> -     writew_relaxed(val, &generic_comm_base->status);
>> +     WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>>
>>       /* Copy the message to the PCC comm space */
>>       for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
>> -             writel_relaxed(msg[i], ptr + i * 4);
>> +             WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>>
>>       /* Ring the doorbell */
>>       rc = mbox_send_message(ctx->mbox_chan, msg);
>> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>>                */
>>               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);
>> +                     ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
>> +                                                     cppc_ss->length,
>> +                                                     MEMREMAP_WT);
>>               } else {
>>                       dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>>                       rc = -ENODEV;
>>

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 9:58 AM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> Hi Arnd,
>
> On Fri, Sep 09, 2016 at 05:38:58PM +0200, Arnd Bergmann wrote:
>> The newly added hwmon driver fails to build in an allmodconfig
>> kernel:
>>
>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>
>> According to comments in the code, the mailbox is a shared memory region,
>> not a set of MMIO registers, so we should use memremap() for mapping it
>> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
>> of readl/writel.
>>
>> The driver already uses plain kernel pointers, so it's a bit unusual
>> to work with functions that operate on __iomem pointers, and this
>> fixes that part too.
>>
>> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
>> regarding the ordering of the accesses from the CPU, but note that
>> there are no barriers (also unchanged from before).
>>
>> I'm also keeping the endianess behavior, though I'm unsure whether
>> the message data was supposed to be in LE32 format in the first
>> place, it's possible this was meant to be interpreted as a byte
>> stream instead.
>>
>> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>>
>
> Thanks a lot for looking into this.
>
> I'll apply this patch to address the build problem. Much better than
> my rude "depends on BROKEN". It would be great to get a Tested-by:
> from someone with access to the hardware.
>

Hi Arnd and Guenter,

Thanks for the patch. I'm testing it out.

Hoan

> Guenter
>
>> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
>> index bc78a5d10182..e834dfb3acca 100644
>> --- a/drivers/hwmon/xgene-hwmon.c
>> +++ b/drivers/hwmon/xgene-hwmon.c
>> @@ -34,7 +34,8 @@
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/platform_device.h>
>> -#include <acpi/acpi_io.h>
>> +#include <linux/io.h>
>> +
>>  #include <acpi/pcc.h>
>>
>>  /* SLIMpro message defines */
>> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>>  {
>>       u16 ret, val;
>>
>> -     val = readw_relaxed(addr);
>> +     val = le16_to_cpu(READ_ONCE(*addr));
>>       ret = val & mask;
>>       val &= ~mask;
>> -     writew_relaxed(val, addr);
>> +     WRITE_ONCE(*addr, cpu_to_le16(val));
>>
>>       return ret;
>>  }
>> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>>  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;
>> +     u32 *ptr = (void*)(generic_comm_base + 1);
>>       int rc, i;
>>       u16 val;
>>
>> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>>       ctx->resp_pending = true;
>>
>>       /* Write signature for subspace */
>> -     writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
>> -                    &generic_comm_base->signature);
>> +     WRITE_ONCE(generic_comm_base->signature,
>> +                cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>>
>>       /* Write to the shared command region */
>> -     writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
>> -                    &generic_comm_base->command);
>> +     WRITE_ONCE(generic_comm_base->command,
>> +                cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>>
>>       /* Flip CMD COMPLETE bit */
>> -     val = readw_relaxed(&generic_comm_base->status);
>> +     val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>>       val &= ~PCCS_CMD_COMPLETE;
>> -     writew_relaxed(val, &generic_comm_base->status);
>> +     WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>>
>>       /* Copy the message to the PCC comm space */
>>       for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
>> -             writel_relaxed(msg[i], ptr + i * 4);
>> +             WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>>
>>       /* Ring the doorbell */
>>       rc = mbox_send_message(ctx->mbox_chan, msg);
>> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>>                */
>>               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);
>> +                     ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
>> +                                                     cppc_ss->length,
>> +                                                     MEMREMAP_WT);
>>               } else {
>>                       dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>>                       rc = -ENODEV;
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

Hi Arnd,

On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
>
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
>
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
>
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
>
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e834dfb3acca 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -34,7 +34,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <acpi/acpi_io.h>
> +#include <linux/io.h>

Alphabetical order.

> +
>  #include <acpi/pcc.h>
>
>  /* SLIMpro message defines */
> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  {
>         u16 ret, val;
>
> -       val = readw_relaxed(addr);
> +       val = le16_to_cpu(READ_ONCE(*addr));
>         ret = val & mask;
>         val &= ~mask;
> -       writew_relaxed(val, addr);
> +       WRITE_ONCE(*addr, cpu_to_le16(val));
>
>         return ret;
>  }
> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  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;
> +       u32 *ptr = (void*)(generic_comm_base + 1);

Space before "*".

>         int rc, i;
>         u16 val;
>
> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>         ctx->resp_pending = true;
>
>         /* Write signature for subspace */
> -       writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
> -                      &generic_comm_base->signature);
> +       WRITE_ONCE(generic_comm_base->signature,
> +                  cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>
>         /* Write to the shared command region */
> -       writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
> -                      &generic_comm_base->command);
> +       WRITE_ONCE(generic_comm_base->command,
> +                  cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>
>         /* Flip CMD COMPLETE bit */
> -       val = readw_relaxed(&generic_comm_base->status);
> +       val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>         val &= ~PCCS_CMD_COMPLETE;
> -       writew_relaxed(val, &generic_comm_base->status);
> +       WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>
>         /* Copy the message to the PCC comm space */
>         for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -               writel_relaxed(msg[i], ptr + i * 4);
> +               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>
>         /* Ring the doorbell */
>         rc = mbox_send_message(ctx->mbox_chan, msg);
> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>                  */
>                 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);
> +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> +                                                       cppc_ss->length,
> +                                                       MEMREMAP_WT);

It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
co-processor is also in the coherency domain.

Thanks
Hoan

>                 } else {
>                         dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>                         rc = -ENODEV;
>

[PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

Hi Arnd,

On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
>
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
>
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
>
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
>
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e834dfb3acca 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -34,7 +34,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <acpi/acpi_io.h>
> +#include <linux/io.h>

Alphabetical order.

> +
>  #include <acpi/pcc.h>
>
>  /* SLIMpro message defines */
> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  {
>         u16 ret, val;
>
> -       val = readw_relaxed(addr);
> +       val = le16_to_cpu(READ_ONCE(*addr));
>         ret = val & mask;
>         val &= ~mask;
> -       writew_relaxed(val, addr);
> +       WRITE_ONCE(*addr, cpu_to_le16(val));
>
>         return ret;
>  }
> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  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;
> +       u32 *ptr = (void*)(generic_comm_base + 1);

Space before "*".

>         int rc, i;
>         u16 val;
>
> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>         ctx->resp_pending = true;
>
>         /* Write signature for subspace */
> -       writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
> -                      &generic_comm_base->signature);
> +       WRITE_ONCE(generic_comm_base->signature,
> +                  cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>
>         /* Write to the shared command region */
> -       writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
> -                      &generic_comm_base->command);
> +       WRITE_ONCE(generic_comm_base->command,
> +                  cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>
>         /* Flip CMD COMPLETE bit */
> -       val = readw_relaxed(&generic_comm_base->status);
> +       val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>         val &= ~PCCS_CMD_COMPLETE;
> -       writew_relaxed(val, &generic_comm_base->status);
> +       WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>
>         /* Copy the message to the PCC comm space */
>         for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -               writel_relaxed(msg[i], ptr + i * 4);
> +               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>
>         /* Ring the doorbell */
>         rc = mbox_send_message(ctx->mbox_chan, msg);
> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>                  */
>                 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);
> +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> +                                                       cppc_ss->length,
> +                                                       MEMREMAP_WT);

It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
co-processor is also in the coherency domain.

Thanks
Hoan

>                 } else {
>                         dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>                         rc = -ENODEV;
>

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > The newly added hwmon driver fails to build in an allmodconfig
> > index bc78a5d10182..e834dfb3acca 100644
> > --- a/drivers/hwmon/xgene-hwmon.c
> > +++ b/drivers/hwmon/xgene-hwmon.c
> > @@ -34,7 +34,8 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > -#include <acpi/acpi_io.h>
> > +#include <linux/io.h>
> 
> Alphabetical order.
> 
> >         struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
> > -       void *ptr = generic_comm_base + 1;
> > +       u32 *ptr = (void*)(generic_comm_base + 1);
> 
> Space before "*".

Ok.

> > @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
> >                  */
> >                 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);
> > +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> > +                                                       cppc_ss->length,
> > +                                                       MEMREMAP_WT);
> 
> It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
> co-processor is also in the coherency domain.

Right, I was wondering about this, since I could not figure out what
the other side is (hardware, service processor or firmware).
So MEMREMAP_WB makes sense here.

Two more questions:

* Any comment on the byte ordering of the data in this line:

        /* Copy the message to the PCC comm space */
        for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-               writel_relaxed(msg[i], ptr + i * 4);
+               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));

This assumes that the old code was correct even when running on
big-endian kernels and the message data consists of 32-bit data words.
If the message has some other format instead, we would need to treat
this as a byte stream and not do swapping here but instead do it
(if any) in the code that reads or writes the actual data here.

* Are you sure you don't need any smp_rmb()/smp_wmb() barriers
between the accesses?

	Arnd

[PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > The newly added hwmon driver fails to build in an allmodconfig
> > index bc78a5d10182..e834dfb3acca 100644
> > --- a/drivers/hwmon/xgene-hwmon.c
> > +++ b/drivers/hwmon/xgene-hwmon.c
> > @@ -34,7 +34,8 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > -#include <acpi/acpi_io.h>
> > +#include <linux/io.h>
> 
> Alphabetical order.
> 
> >         struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
> > -       void *ptr = generic_comm_base + 1;
> > +       u32 *ptr = (void*)(generic_comm_base + 1);
> 
> Space before "*".

Ok.

> > @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
> >                  */
> >                 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);
> > +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> > +                                                       cppc_ss->length,
> > +                                                       MEMREMAP_WT);
> 
> It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
> co-processor is also in the coherency domain.

Right, I was wondering about this, since I could not figure out what
the other side is (hardware, service processor or firmware).
So MEMREMAP_WB makes sense here.

Two more questions:

* Any comment on the byte ordering of the data in this line:

        /* Copy the message to the PCC comm space */
        for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-               writel_relaxed(msg[i], ptr + i * 4);
+               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));

This assumes that the old code was correct even when running on
big-endian kernels and the message data consists of 32-bit data words.
If the message has some other format instead, we would need to treat
this as a byte stream and not do swapping here but instead do it
(if any) in the code that reads or writes the actual data here.

* Are you sure you don't need any smp_rmb()/smp_wmb() barriers
between the accesses?

	Arnd

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > The newly added hwmon driver fails to build in an allmodconfig
> > index bc78a5d10182..e834dfb3acca 100644
> > --- a/drivers/hwmon/xgene-hwmon.c
> > +++ b/drivers/hwmon/xgene-hwmon.c
> > @@ -34,7 +34,8 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > -#include <acpi/acpi_io.h>
> > +#include <linux/io.h>
> 
> Alphabetical order.
> 
> >         struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
> > -       void *ptr = generic_comm_base + 1;
> > +       u32 *ptr = (void*)(generic_comm_base + 1);
> 
> Space before "*".

Ok.

> > @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
> >                  */
> >                 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);
> > +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> > +                                                       cppc_ss->length,
> > +                                                       MEMREMAP_WT);
> 
> It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
> co-processor is also in the coherency domain.

Right, I was wondering about this, since I could not figure out what
the other side is (hardware, service processor or firmware).
So MEMREMAP_WB makes sense here.

Two more questions:

* Any comment on the byte ordering of the data in this line:

        /* Copy the message to the PCC comm space */
        for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-               writel_relaxed(msg[i], ptr + i * 4);
+               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));

This assumes that the old code was correct even when running on
big-endian kernels and the message data consists of 32-bit data words.
If the message has some other format instead, we would need to treat
this as a byte stream and not do swapping here but instead do it
(if any) in the code that reads or writes the actual data here.

* Are you sure you don't need any smp_rmb()/smp_wmb() barriers
between the accesses?

	Arnd

[PATCH v2] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

The newly added hwmon driver fails to build in an allmodconfig
kernel:

      ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

According to comments in the code, the mailbox is a shared memory region,
not a set of MMIO registers, so we should use memremap() for mapping it
instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual
to work with functions that operate on __iomem pointers, and this
fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
regarding the ordering of the accesses from the CPU, but note that
there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether
the message data was supposed to be in LE32 format in the first
place, it's possible this was meant to be interpreted as a byte
stream instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: use write-back mapping instead of write-thru,
    minor coding style changes

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index bc78a5d10182..e5470bd49067 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -27,6 +27,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/kfifo.h>
 #include <linux/mailbox_controller.h>
@@ -34,7 +35,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */
@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
 	u16 ret, val;
 
-	val = readw_relaxed(addr);
+	val = le16_to_cpu(READ_ONCE(*addr));
 	ret = val & mask;
 	val &= ~mask;
-	writew_relaxed(val, addr);
+	WRITE_ONCE(*addr, cpu_to_le16(val));
 
 	return ret;
 }
@@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 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;
+	u32 *ptr = (void *)(generic_comm_base + 1);
 	int rc, i;
 	u16 val;
 
@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 	ctx->resp_pending = true;
 
 	/* Write signature for subspace */
-	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-		       &generic_comm_base->signature);
+	WRITE_ONCE(generic_comm_base->signature,
+		   cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
 	/* Write to the shared command region */
-	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-		       &generic_comm_base->command);
+	WRITE_ONCE(generic_comm_base->command,
+		   cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
 	/* Flip CMD COMPLETE bit */
-	val = readw_relaxed(&generic_comm_base->status);
+	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
 	val &= ~PCCS_CMD_COMPLETE;
-	writew_relaxed(val, &generic_comm_base->status);
+	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
 	/* Copy the message to the PCC comm space */
 	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-		writel_relaxed(msg[i], ptr + i * 4);
+		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
 	/* Ring the doorbell */
 	rc = mbox_send_message(ctx->mbox_chan, msg);
@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		 */
 		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);
+			ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+							cppc_ss->length,
+							MEMREMAP_WB);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
 			rc = -ENODEV;

[PATCH v2] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

The newly added hwmon driver fails to build in an allmodconfig
kernel:

      ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

According to comments in the code, the mailbox is a shared memory region,
not a set of MMIO registers, so we should use memremap() for mapping it
instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual
to work with functions that operate on __iomem pointers, and this
fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
regarding the ordering of the accesses from the CPU, but note that
there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether
the message data was supposed to be in LE32 format in the first
place, it's possible this was meant to be interpreted as a byte
stream instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: use write-back mapping instead of write-thru,
    minor coding style changes

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index bc78a5d10182..e5470bd49067 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -27,6 +27,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/kfifo.h>
 #include <linux/mailbox_controller.h>
@@ -34,7 +35,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */
@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
 	u16 ret, val;
 
-	val = readw_relaxed(addr);
+	val = le16_to_cpu(READ_ONCE(*addr));
 	ret = val & mask;
 	val &= ~mask;
-	writew_relaxed(val, addr);
+	WRITE_ONCE(*addr, cpu_to_le16(val));
 
 	return ret;
 }
@@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 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;
+	u32 *ptr = (void *)(generic_comm_base + 1);
 	int rc, i;
 	u16 val;
 
@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 	ctx->resp_pending = true;
 
 	/* Write signature for subspace */
-	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-		       &generic_comm_base->signature);
+	WRITE_ONCE(generic_comm_base->signature,
+		   cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
 	/* Write to the shared command region */
-	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-		       &generic_comm_base->command);
+	WRITE_ONCE(generic_comm_base->command,
+		   cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
 	/* Flip CMD COMPLETE bit */
-	val = readw_relaxed(&generic_comm_base->status);
+	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
 	val &= ~PCCS_CMD_COMPLETE;
-	writew_relaxed(val, &generic_comm_base->status);
+	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
 	/* Copy the message to the PCC comm space */
 	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-		writel_relaxed(msg[i], ptr + i * 4);
+		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
 	/* Ring the doorbell */
 	rc = mbox_send_message(ctx->mbox_chan, msg);
@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		 */
 		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);
+			ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+							cppc_ss->length,
+							MEMREMAP_WB);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
 			rc = -ENODEV;

Re: [PATCH v2] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

Hi Arnd,

On Fri, Sep 9, 2016 at 1:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
>
>       ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
>
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
>
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
>
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: use write-back mapping instead of write-thru,
>     minor coding style changes
>
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e5470bd49067 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -27,6 +27,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> +#include <linux/io.h>
>  #include <linux/interrupt.h>
>  #include <linux/kfifo.h>
>  #include <linux/mailbox_controller.h>
> @@ -34,7 +35,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <acpi/acpi_io.h>
> +
>  #include <acpi/pcc.h>
>
>  /* SLIMpro message defines */
> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  {
>         u16 ret, val;
>
> -       val = readw_relaxed(addr);
> +       val = le16_to_cpu(READ_ONCE(*addr));
>         ret = val & mask;
>         val &= ~mask;
> -       writew_relaxed(val, addr);
> +       WRITE_ONCE(*addr, cpu_to_le16(val));
>
>         return ret;
>  }
> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  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;
> +       u32 *ptr = (void *)(generic_comm_base + 1);
>         int rc, i;
>         u16 val;
>
> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>         ctx->resp_pending = true;
>
>         /* Write signature for subspace */
> -       writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
> -                      &generic_comm_base->signature);
> +       WRITE_ONCE(generic_comm_base->signature,
> +                  cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>
>         /* Write to the shared command region */
> -       writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
> -                      &generic_comm_base->command);
> +       WRITE_ONCE(generic_comm_base->command,
> +                  cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>
>         /* Flip CMD COMPLETE bit */
> -       val = readw_relaxed(&generic_comm_base->status);
> +       val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>         val &= ~PCCS_CMD_COMPLETE;
> -       writew_relaxed(val, &generic_comm_base->status);
> +       WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>
>         /* Copy the message to the PCC comm space */
>         for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -               writel_relaxed(msg[i], ptr + i * 4);
> +               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>
>         /* Ring the doorbell */
>         rc = mbox_send_message(ctx->mbox_chan, msg);
> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>                  */
>                 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);
> +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> +                                                       cppc_ss->length,
> +                                                       MEMREMAP_WB);
>                 } else {
>                         dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>                         rc = -ENODEV;
>

Acked-by: Hoan Tran <hotran@apm.com>
Tested-by: Hoan Tran <hotran@apm.com>

Thanks
Hoan

[PATCH v2] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

Hi Arnd,

On Fri, Sep 9, 2016 at 1:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
>
>       ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
>
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
>
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
>
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: use write-back mapping instead of write-thru,
>     minor coding style changes
>
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e5470bd49067 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -27,6 +27,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> +#include <linux/io.h>
>  #include <linux/interrupt.h>
>  #include <linux/kfifo.h>
>  #include <linux/mailbox_controller.h>
> @@ -34,7 +35,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <acpi/acpi_io.h>
> +
>  #include <acpi/pcc.h>
>
>  /* SLIMpro message defines */
> @@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  {
>         u16 ret, val;
>
> -       val = readw_relaxed(addr);
> +       val = le16_to_cpu(READ_ONCE(*addr));
>         ret = val & mask;
>         val &= ~mask;
> -       writew_relaxed(val, addr);
> +       WRITE_ONCE(*addr, cpu_to_le16(val));
>
>         return ret;
>  }
> @@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
>  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;
> +       u32 *ptr = (void *)(generic_comm_base + 1);
>         int rc, i;
>         u16 val;
>
> @@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>         ctx->resp_pending = true;
>
>         /* Write signature for subspace */
> -       writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
> -                      &generic_comm_base->signature);
> +       WRITE_ONCE(generic_comm_base->signature,
> +                  cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
>
>         /* Write to the shared command region */
> -       writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
> -                      &generic_comm_base->command);
> +       WRITE_ONCE(generic_comm_base->command,
> +                  cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
>
>         /* Flip CMD COMPLETE bit */
> -       val = readw_relaxed(&generic_comm_base->status);
> +       val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
>         val &= ~PCCS_CMD_COMPLETE;
> -       writew_relaxed(val, &generic_comm_base->status);
> +       WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>
>         /* Copy the message to the PCC comm space */
>         for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -               writel_relaxed(msg[i], ptr + i * 4);
> +               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>
>         /* Ring the doorbell */
>         rc = mbox_send_message(ctx->mbox_chan, msg);
> @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>                  */
>                 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);
> +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> +                                                       cppc_ss->length,
> +                                                       MEMREMAP_WB);
>                 } else {
>                         dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>                         rc = -ENODEV;
>

Acked-by: Hoan Tran <hotran@apm.com>
Tested-by: Hoan Tran <hotran@apm.com>

Thanks
Hoan

Re: [PATCH v2] hwmon: xgene: access mailbox as RAM

[Guenter Roeck]

On Fri, Sep 09, 2016 at 10:10:45PM +0200, Arnd Bergmann wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
> 
>       ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
> 
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
> 
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
> 
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied after 's/endianess/endianness/' to make checkpatch happy.

Thanks,
Guenter

[PATCH v2] hwmon: xgene: access mailbox as RAM

[Guenter Roeck]

On Fri, Sep 09, 2016 at 10:10:45PM +0200, Arnd Bergmann wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
> 
>       ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
> 
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
> 
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
> 
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied after 's/endianess/endianness/' to make checkpatch happy.

Thanks,
Guenter

Re: [PATCH v2] hwmon: xgene: access mailbox as RAM

[Guenter Roeck]

On Fri, Sep 09, 2016 at 10:10:45PM +0200, Arnd Bergmann wrote:
> The newly added hwmon driver fails to build in an allmodconfig
> kernel:
> 
>       ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> According to comments in the code, the mailbox is a shared memory region,
> not a set of MMIO registers, so we should use memremap() for mapping it
> instead of ioremap or acpi_os_ioremap, and pointer dereferences instead
> of readl/writel.
> 
> The driver already uses plain kernel pointers, so it's a bit unusual
> to work with functions that operate on __iomem pointers, and this
> fixes that part too.
> 
> I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior
> regarding the ordering of the accesses from the CPU, but note that
> there are no barriers (also unchanged from before).
> 
> I'm also keeping the endianess behavior, though I'm unsure whether
> the message data was supposed to be in LE32 format in the first
> place, it's possible this was meant to be interpreted as a byte
> stream instead.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Applied after 's/endianess/endianness/' to make checkpatch happy.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 12:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
>> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > The newly added hwmon driver fails to build in an allmodconfig
>> > index bc78a5d10182..e834dfb3acca 100644
>> > --- a/drivers/hwmon/xgene-hwmon.c
>> > +++ b/drivers/hwmon/xgene-hwmon.c
>> > @@ -34,7 +34,8 @@
>> >  #include <linux/module.h>
>> >  #include <linux/of.h>
>> >  #include <linux/platform_device.h>
>> > -#include <acpi/acpi_io.h>
>> > +#include <linux/io.h>
>>
>> Alphabetical order.
>>
>> >         struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
>> > -       void *ptr = generic_comm_base + 1;
>> > +       u32 *ptr = (void*)(generic_comm_base + 1);
>>
>> Space before "*".
>
> Ok.
>
>> > @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>> >                  */
>> >                 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);
>> > +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
>> > +                                                       cppc_ss->length,
>> > +                                                       MEMREMAP_WT);
>>
>> It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
>> co-processor is also in the coherency domain.
>
> Right, I was wondering about this, since I could not figure out what
> the other side is (hardware, service processor or firmware).
> So MEMREMAP_WB makes sense here.
>
> Two more questions:
>
> * Any comment on the byte ordering of the data in this line:
>
>         /* Copy the message to the PCC comm space */
>         for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -               writel_relaxed(msg[i], ptr + i * 4);
> +               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>
> This assumes that the old code was correct even when running on
> big-endian kernels and the message data consists of 32-bit data words.
> If the message has some other format instead, we would need to treat
> this as a byte stream and not do swapping here but instead do it
> (if any) in the code that reads or writes the actual data here.

This is 32-bit data words.

>
> * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
> between the accesses?

No, we don't need a strict read/write during access PCC subspace. Just
make sure all access is committed before PCC send message to the
platform which done by PCC mailbox driver.

Thanks
Hoan

>
>         Arnd

[PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 12:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
>> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > The newly added hwmon driver fails to build in an allmodconfig
>> > index bc78a5d10182..e834dfb3acca 100644
>> > --- a/drivers/hwmon/xgene-hwmon.c
>> > +++ b/drivers/hwmon/xgene-hwmon.c
>> > @@ -34,7 +34,8 @@
>> >  #include <linux/module.h>
>> >  #include <linux/of.h>
>> >  #include <linux/platform_device.h>
>> > -#include <acpi/acpi_io.h>
>> > +#include <linux/io.h>
>>
>> Alphabetical order.
>>
>> >         struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
>> > -       void *ptr = generic_comm_base + 1;
>> > +       u32 *ptr = (void*)(generic_comm_base + 1);
>>
>> Space before "*".
>
> Ok.
>
>> > @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>> >                  */
>> >                 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);
>> > +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
>> > +                                                       cppc_ss->length,
>> > +                                                       MEMREMAP_WT);
>>
>> It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
>> co-processor is also in the coherency domain.
>
> Right, I was wondering about this, since I could not figure out what
> the other side is (hardware, service processor or firmware).
> So MEMREMAP_WB makes sense here.
>
> Two more questions:
>
> * Any comment on the byte ordering of the data in this line:
>
>         /* Copy the message to the PCC comm space */
>         for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -               writel_relaxed(msg[i], ptr + i * 4);
> +               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>
> This assumes that the old code was correct even when running on
> big-endian kernels and the message data consists of 32-bit data words.
> If the message has some other format instead, we would need to treat
> this as a byte stream and not do swapping here but instead do it
> (if any) in the code that reads or writes the actual data here.

This is 32-bit data words.

>
> * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
> between the accesses?

No, we don't need a strict read/write during access PCC subspace. Just
make sure all access is committed before PCC send message to the
platform which done by PCC mailbox driver.

Thanks
Hoan

>
>         Arnd

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 12:58 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
>> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> > The newly added hwmon driver fails to build in an allmodconfig
>> > index bc78a5d10182..e834dfb3acca 100644
>> > --- a/drivers/hwmon/xgene-hwmon.c
>> > +++ b/drivers/hwmon/xgene-hwmon.c
>> > @@ -34,7 +34,8 @@
>> >  #include <linux/module.h>
>> >  #include <linux/of.h>
>> >  #include <linux/platform_device.h>
>> > -#include <acpi/acpi_io.h>
>> > +#include <linux/io.h>
>>
>> Alphabetical order.
>>
>> >         struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
>> > -       void *ptr = generic_comm_base + 1;
>> > +       u32 *ptr = (void*)(generic_comm_base + 1);
>>
>> Space before "*".
>
> Ok.
>
>> > @@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
>> >                  */
>> >                 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);
>> > +                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
>> > +                                                       cppc_ss->length,
>> > +                                                       MEMREMAP_WT);
>>
>> It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our
>> co-processor is also in the coherency domain.
>
> Right, I was wondering about this, since I could not figure out what
> the other side is (hardware, service processor or firmware).
> So MEMREMAP_WB makes sense here.
>
> Two more questions:
>
> * Any comment on the byte ordering of the data in this line:
>
>         /* Copy the message to the PCC comm space */
>         for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
> -               writel_relaxed(msg[i], ptr + i * 4);
> +               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
>
> This assumes that the old code was correct even when running on
> big-endian kernels and the message data consists of 32-bit data words.
> If the message has some other format instead, we would need to treat
> this as a byte stream and not do swapping here but instead do it
> (if any) in the code that reads or writes the actual data here.

This is 32-bit data words.

>
> * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
> between the accesses?

No, we don't need a strict read/write during access PCC subspace. Just
make sure all access is committed before PCC send message to the
platform which done by PCC mailbox driver.

Thanks
Hoan

>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

On Friday, September 9, 2016 1:43:17 PM CEST Hoan Tran wrote:
> 
> > * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
> > between the accesses?
> 
> No, we don't need a strict read/write during access PCC subspace. Just
> make sure all access is committed before PCC send message to the
> platform which done by PCC mailbox driver.
> 

Ok, got it. The PCC mailbox driver presumably uses writel() to
send the message, and that implies the necessary barrier
(unlike writel_relaxed), right?

	Arnd

[PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

On Friday, September 9, 2016 1:43:17 PM CEST Hoan Tran wrote:
> 
> > * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
> > between the accesses?
> 
> No, we don't need a strict read/write during access PCC subspace. Just
> make sure all access is committed before PCC send message to the
> platform which done by PCC mailbox driver.
> 

Ok, got it. The PCC mailbox driver presumably uses writel() to
send the message, and that implies the necessary barrier
(unlike writel_relaxed), right?

	Arnd

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Arnd Bergmann]

On Friday, September 9, 2016 1:43:17 PM CEST Hoan Tran wrote:
> 
> > * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
> > between the accesses?
> 
> No, we don't need a strict read/write during access PCC subspace. Just
> make sure all access is committed before PCC send message to the
> platform which done by PCC mailbox driver.
> 

Ok, got it. The PCC mailbox driver presumably uses writel() to
send the message, and that implies the necessary barrier
(unlike writel_relaxed), right?

	Arnd

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 1:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, September 9, 2016 1:43:17 PM CEST Hoan Tran wrote:
>>
>> > * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
>> > between the accesses?
>>
>> No, we don't need a strict read/write during access PCC subspace. Just
>> make sure all access is committed before PCC send message to the
>> platform which done by PCC mailbox driver.
>>
>
> Ok, got it. The PCC mailbox driver presumably uses writel() to
> send the message, and that implies the necessary barrier
> (unlike writel_relaxed), right?

Yes,

Hoan
>
>         Arnd
>

[PATCH] hwmon: xgene: access mailbox as RAM

[Hoan Tran]

On Fri, Sep 9, 2016 at 1:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, September 9, 2016 1:43:17 PM CEST Hoan Tran wrote:
>>
>> > * Are you sure you don't need any smp_rmb()/smp_wmb() barriers
>> > between the accesses?
>>
>> No, we don't need a strict read/write during access PCC subspace. Just
>> make sure all access is committed before PCC send message to the
>> platform which done by PCC mailbox driver.
>>
>
> Ok, got it. The PCC mailbox driver presumably uses writel() to
> send the message, and that implies the necessary barrier
> (unlike writel_relaxed), right?

Yes,

Hoan
>
>         Arnd
>

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

[Hoan Tran]

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

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

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

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

[Hoan Tran]

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

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

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

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

[Guenter Roeck]

On Thu, Jul 21, 2016 at 01:55:57PM -0700, Hoan Tran wrote:
> This patch adds DT node to enable hwmon driver for APM X-Gene SoC.
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

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

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

[Guenter Roeck]

On Thu, Jul 21, 2016 at 01:55:57PM -0700, Hoan Tran wrote:
> This patch adds DT node to enable hwmon driver for APM X-Gene SoC.
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

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