[PATCH linux v3 0/6] drivers: hwmon: Add On-Chip Controller driver

[PATCH linux v3 0/6] drivers: hwmon: Add On-Chip Controller driver

[eajames.ibm]

From: "Edward A. James" <eajames@us.ibm.com>

This patchset adds a hwmon driver to support the OCC (On-Chip Controller)
on the IBM POWER8 and POWER9 processors, from a BMC (Baseboard Management
Controller). The OCC is an embedded processor that provides real time
power and thermal monitoring.

The driver provides an interface on a BMC to poll OCC sensor data, set
user power caps, and perform some basic OCC error handling. It interfaces
with userspace through hwmon.

The driver is currently functional only for the OCC on POWER8 chips.
Communicating with the POWER9 OCC requries FSI support.

Edward A. James (6):
  hwmon: Add core On-Chip Controller support for POWER CPUs
  hwmon: occ: Add sysfs interface
  hwmon: occ: Add I2C transport implementation for SCOM operations
  hwmon: occ: Add callbacks for parsing P8 OCC datastructures
  hwmon: occ: Add hwmon implementation for the P8 OCC
  hwmon: occ: Add callbacks for parsing P9 OCC datastructures

 Documentation/devicetree/bindings/hwmon/occ.txt |  13 +
 Documentation/hwmon/occ                         | 114 ++++++
 MAINTAINERS                                     |   7 +
 drivers/hwmon/Kconfig                           |   2 +
 drivers/hwmon/Makefile                          |   1 +
 drivers/hwmon/occ/Kconfig                       |  29 ++
 drivers/hwmon/occ/Makefile                      |   2 +
 drivers/hwmon/occ/occ.c                         | 522 ++++++++++++++++++++++++
 drivers/hwmon/occ/occ.h                         |  81 ++++
 drivers/hwmon/occ/occ_p8.c                      | 247 +++++++++++
 drivers/hwmon/occ/occ_p8.h                      |  30 ++
 drivers/hwmon/occ/occ_p9.c                      | 308 ++++++++++++++
 drivers/hwmon/occ/occ_p9.h                      |  30 ++
 drivers/hwmon/occ/occ_scom_i2c.c                |  72 ++++
 drivers/hwmon/occ/occ_scom_i2c.h                |  26 ++
 drivers/hwmon/occ/occ_sysfs.c                   | 271 ++++++++++++
 drivers/hwmon/occ/occ_sysfs.h                   |  44 ++
 drivers/hwmon/occ/p8_occ_i2c.c                  | 104 +++++
 drivers/hwmon/occ/scom.h                        |  47 +++
 19 files changed, 1950 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
 create mode 100644 Documentation/hwmon/occ
 create mode 100644 drivers/hwmon/occ/Kconfig
 create mode 100644 drivers/hwmon/occ/Makefile
 create mode 100644 drivers/hwmon/occ/occ.c
 create mode 100644 drivers/hwmon/occ/occ.h
 create mode 100644 drivers/hwmon/occ/occ_p8.c
 create mode 100644 drivers/hwmon/occ/occ_p8.h
 create mode 100644 drivers/hwmon/occ/occ_p9.c
 create mode 100644 drivers/hwmon/occ/occ_p9.h
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
 create mode 100644 drivers/hwmon/occ/occ_sysfs.c
 create mode 100644 drivers/hwmon/occ/occ_sysfs.h
 create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
 create mode 100644 drivers/hwmon/occ/scom.h

-- 
1.9.1

[PATCH linux v3 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs

[eajames.ibm]

From: "Edward A. James" <eajames@us.ibm.com>

Add core support for polling the OCC for it's sensor data and parsing that
data into sensor-specific information.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/hwmon/occ    |  40 ++++
 MAINTAINERS                |   7 +
 drivers/hwmon/Kconfig      |   2 +
 drivers/hwmon/Makefile     |   1 +
 drivers/hwmon/occ/Kconfig  |  15 ++
 drivers/hwmon/occ/Makefile |   1 +
 drivers/hwmon/occ/occ.c    | 522 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ.h    |  81 +++++++
 drivers/hwmon/occ/scom.h   |  47 ++++
 9 files changed, 716 insertions(+)
 create mode 100644 Documentation/hwmon/occ
 create mode 100644 drivers/hwmon/occ/Kconfig
 create mode 100644 drivers/hwmon/occ/Makefile
 create mode 100644 drivers/hwmon/occ/occ.c
 create mode 100644 drivers/hwmon/occ/occ.h
 create mode 100644 drivers/hwmon/occ/scom.h

diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
new file mode 100644
index 0000000..79d1642
--- /dev/null
+++ b/Documentation/hwmon/occ
@@ -0,0 +1,40 @@
+Kernel driver occ
+=================
+
+Supported chips:
+ * ASPEED AST2400
+ * ASPEED AST2500
+
+Please note that the chip must be connected to a POWER8 or POWER9 processor
+(see the BMC - Host Communications section).
+
+Author: Eddie James <eajames@us.ibm.com>
+
+Description
+-----------
+
+This driver implements support for the OCC (On-Chip Controller) on the IBM
+POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The
+OCC is an embedded processor that provides real time power and thermal
+monitoring.
+
+This driver provides an interface on a BMC to poll OCC sensor data, set user
+power caps, and perform some basic OCC error handling.
+
+Currently, all versions of the OCC support four types of sensor data: power,
+temperature, frequency, and "caps," which indicate limits and thresholds used
+internally on the OCC.
+
+BMC - Host Communications
+-------------------------
+
+For the POWER8 application, the BMC can communicate with the P8 over I2C bus.
+However, to access the OCC register space, any data transfer must use a SCOM
+operation. SCOM is a procedure to initiate a data transfer, typically of 8
+bytes. SCOMs consist of writing a 32-bit command register and then
+reading/writing two 32-bit data registers. This driver implements these
+SCOM operations over I2C bus in order to communicate with the OCC.
+
+For the POWER9 application, the BMC can communicate with the P9 over FSI bus
+and SBE engine. Once again, SCOM operations are required. This driver will
+implement SCOM ops over FSI/SBE. This will require the FSI driver.
diff --git a/MAINTAINERS b/MAINTAINERS
index 5f0420a..f5d4195 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9112,6 +9112,13 @@ T:	git git://linuxtv.org/media_tree.git
 S:	Maintained
 F:	drivers/media/i2c/ov7670.c
 
+ON-CHIP CONTROLLER HWMON DRIVER
+M:	Eddie James <eajames@us.ibm.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/occ
+F:	drivers/hwmon/occ/
+
 ONENAND FLASH DRIVER
 M:	Kyungmin Park <kyungmin.park@samsung.com>
 L:	linux-mtd@lists.infradead.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 190d270..e80ca81 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1240,6 +1240,8 @@ config SENSORS_NSA320
 	  This driver can also be built as a module. If so, the module
 	  will be called nsa320-hwmon.
 
+source drivers/hwmon/occ/Kconfig
+
 config SENSORS_PCF8591
 	tristate "Philips PCF8591 ADC/DAC"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d2cb7e8..c7ec5d4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
 obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
 obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
 
+obj-$(CONFIG_SENSORS_PPC_OCC)	+= occ/
 obj-$(CONFIG_PMBUS)		+= pmbus/
 
 ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
new file mode 100644
index 0000000..cdb64a7
--- /dev/null
+++ b/drivers/hwmon/occ/Kconfig
@@ -0,0 +1,15 @@
+#
+# On Chip Controller configuration
+#
+
+menuconfig SENSORS_PPC_OCC
+	bool "PPC On-Chip Controller"
+	help
+	  If you say yes here you get support to monitor Power CPU
+	  sensors via the On-Chip Controller (OCC).
+
+	  Generally this is used by management controllers such as a BMC
+	  on an OpenPower system.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called occ.
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
new file mode 100644
index 0000000..93cb52f
--- /dev/null
+++ b/drivers/hwmon/occ/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
diff --git a/drivers/hwmon/occ/occ.c b/drivers/hwmon/occ/occ.c
new file mode 100644
index 0000000..3089762
--- /dev/null
+++ b/drivers/hwmon/occ/occ.c
@@ -0,0 +1,522 @@
+/*
+ * occ.c - OCC hwmon driver
+ *
+ * This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include "occ.h"
+
+#define OCC_DATA_MAX		4096
+#define OCC_BMC_TIMEOUT_MS	20000
+
+/* To generate attn to OCC */
+#define ATTN_DATA		0x0006B035
+
+/* For BMC to read/write SRAM */
+#define OCB_ADDRESS		0x0006B070
+#define OCB_DATA		0x0006B075
+#define OCB_STATUS_CONTROL_AND	0x0006B072
+#define OCB_STATUS_CONTROL_OR	0x0006B073
+
+/* To init OCB */
+#define OCB_AND_INIT0		0xFBFFFFFF
+#define OCB_AND_INIT1		0xFFFFFFFF
+#define OCB_OR_INIT0		0x08000000
+#define OCB_OR_INIT1		0x00000000
+
+/* To generate attention on OCC */
+#define ATTN0			0x01010000
+#define ATTN1			0x00000000
+
+/* OCC return status */
+#define RESP_RETURN_CMD_IN_PRG	0xFF
+#define RESP_RETURN_SUCCESS	0
+#define RESP_RETURN_CMD_INVAL	0x11
+#define RESP_RETURN_CMD_LEN	0x12
+#define RESP_RETURN_DATA_INVAL	0x13
+#define RESP_RETURN_CHKSUM	0x14
+#define RESP_RETURN_OCC_ERR	0x15
+#define RESP_RETURN_STATE	0x16
+
+/* time interval to retry on "command in progress" return status */
+#define CMD_IN_PRG_INT_MS	100
+#define CMD_IN_PRG_RETRIES	(OCC_BMC_TIMEOUT_MS / CMD_IN_PRG_INT_MS)
+
+/* OCC command definitions */
+#define OCC_POLL		0
+#define OCC_SET_USER_POWR_CAP	0x22
+
+/* OCC poll command data */
+#define OCC_POLL_STAT_SENSOR	0x10
+
+/* OCC response data offsets */
+#define RESP_RETURN_STATUS	2
+#define RESP_DATA_LENGTH	3
+#define RESP_HEADER_OFFSET	5
+#define SENSOR_STR_OFFSET	37
+#define SENSOR_BLOCK_NUM_OFFSET	43
+#define SENSOR_BLOCK_OFFSET	45
+
+/* occ_poll_header
+ * structure to match the raw occ poll response data
+ */
+struct occ_poll_header {
+	u8 status;
+	u8 ext_status;
+	u8 occs_present;
+	u8 config;
+	u8 occ_state;
+	u8 mode;
+	u8 ips_status;
+	u8 error_log_id;
+	u32 error_log_addr_start;
+	u16 error_log_length;
+	u8 reserved2;
+	u8 reserved3;
+	u8 occ_code_level[16];
+	u8 sensor_eye_catcher[6];
+	u8 sensor_block_num;
+	u8 sensor_data_version;
+} __attribute__((packed, aligned(4)));
+
+struct occ_response {
+	struct occ_poll_header header;
+	struct occ_blocks data;
+};
+
+struct occ {
+	struct device *dev;
+	void *bus;
+	struct occ_bus_ops bus_ops;
+	struct occ_ops ops;
+	struct occ_config config;
+	unsigned long update_interval;
+	unsigned long last_updated;
+	struct mutex update_lock;
+	struct occ_response response;
+	bool valid;
+};
+
+static void deinit_occ_resp_buf(struct occ_response *resp)
+{
+	int i;
+
+	if (!resp)
+		return;
+
+	if (!resp->data.blocks)
+		return;
+
+	for (i = 0; i < resp->header.sensor_block_num; ++i)
+		kfree(resp->data.blocks[i].sensors);
+
+	kfree(resp->data.blocks);
+
+	memset(resp, 0, sizeof(struct occ_response));
+
+	for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+		resp->data.sensor_block_id[i] = -1;
+}
+
+static void *occ_get_sensor_by_type(struct occ_response *resp,
+				    enum sensor_type t)
+{
+	if (!resp->data.blocks)
+		return NULL;
+
+	if (resp->data.sensor_block_id[t] == -1)
+		return NULL;
+
+	return resp->data.blocks[resp->data.sensor_block_id[t]].sensors;
+}
+
+static int occ_check_sensor(struct occ *driver, u8 sensor_length,
+			    u8 sensor_num, enum sensor_type t, int block)
+{
+	void *sensor;
+	int type_block_id;
+	struct occ_response *resp = &driver->response;
+
+	sensor = occ_get_sensor_by_type(resp, t);
+
+	/* empty sensor block, release older sensor data */
+	if (sensor_num == 0 || sensor_length == 0) {
+		kfree(sensor);
+		dev_err(driver->dev, "no sensor blocks available\n");
+		return -ENODATA;
+	}
+
+	type_block_id = resp->data.sensor_block_id[t];
+	if (!sensor || sensor_num !=
+	    resp->data.blocks[type_block_id].header.sensor_num) {
+		kfree(sensor);
+		resp->data.blocks[block].sensors =
+			driver->ops.alloc_sensor(t, sensor_num);
+		if (!resp->data.blocks[block].sensors)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int parse_occ_response(struct occ *driver, u8 *data,
+			      struct occ_response *resp)
+{
+	int b;
+	int s;
+	int rc;
+	int offset = SENSOR_BLOCK_OFFSET;
+	int sensor_type;
+	u8 sensor_block_num;
+	char sensor_type_string[5] = { 0 };
+	struct sensor_data_block_header *block;
+	struct device *dev = driver->dev;
+
+	/* check if the data is valid */
+	if (strncmp(&data[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {
+		dev_err(dev, "no SENSOR string in response\n");
+		rc = -ENODATA;
+		goto err;
+	}
+
+	sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET];
+	if (sensor_block_num == 0) {
+		dev_err(dev, "no sensor blocks available\n");
+		rc = -ENODATA;
+		goto err;
+	}
+
+	/* if number of sensor block has changed, re-malloc */
+	if (sensor_block_num != resp->header.sensor_block_num) {
+		deinit_occ_resp_buf(resp);
+		resp->data.blocks = kcalloc(sensor_block_num,
+					    sizeof(struct sensor_data_block),
+					    GFP_KERNEL);
+		if (!resp->data.blocks)
+			return -ENOMEM;
+	}
+
+	memcpy(&resp->header, &data[RESP_HEADER_OFFSET],
+	       sizeof(struct occ_poll_header));
+	resp->header.error_log_addr_start =
+		be32_to_cpu(resp->header.error_log_addr_start);
+	resp->header.error_log_length =
+		be16_to_cpu(resp->header.error_log_length);
+
+	dev_dbg(dev, "Reading %d sensor blocks\n",
+		resp->header.sensor_block_num);
+	for (b = 0; b < sensor_block_num; b++) {
+		block = (struct sensor_data_block_header *)&data[offset];
+		/* copy to a null terminated string */
+		strncpy(sensor_type_string, block->sensor_type, 4);
+		offset += 8;
+
+		dev_dbg(dev, "sensor block[%d]: type: %s, sensor_num: %d\n", b,
+			sensor_type_string, block->sensor_num);
+
+		if (strncmp(block->sensor_type, "FREQ", 4) == 0)
+			sensor_type = FREQ;
+		else if (strncmp(block->sensor_type, "TEMP", 4) == 0)
+			sensor_type = TEMP;
+		else if (strncmp(block->sensor_type, "POWR", 4) == 0)
+			sensor_type = POWER;
+		else if (strncmp(block->sensor_type, "CAPS", 4) == 0)
+			sensor_type = CAPS;
+		else {
+			dev_err(dev, "sensor type not supported %s\n",
+				sensor_type_string);
+			continue;
+		}
+
+		rc = occ_check_sensor(driver, block->sensor_length,
+				      block->sensor_num, sensor_type, b);
+		if (rc == -ENOMEM)
+			goto err;
+		else if (rc)
+			continue;
+
+		resp->data.sensor_block_id[sensor_type] = b;
+		for (s = 0; s < block->sensor_num; s++) {
+			driver->ops.parse_sensor(data,
+						 resp->data.blocks[b].sensors,
+						 sensor_type, offset, s);
+			offset += block->sensor_length;
+		}
+
+		/* copy block data over to response pointer */
+		resp->data.blocks[b].header = *block;
+	}
+
+	return 0;
+err:
+	deinit_occ_resp_buf(resp);
+	return rc;
+}
+
+static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,
+		       const u8 *data, u8 *resp)
+{
+	u32 cmd1, cmd2;
+	u16 checksum = 0;
+	u16 length_le = cpu_to_le16(length);
+	bool retry = 0;
+	int i, rc, tries = 0;
+
+	cmd1 = (seq << 24) | (type << 16) | length_le;
+	memcpy(&cmd2, data, length);
+	cmd2 <<= ((4 - length) * 8);
+
+	/* checksum: sum of every bytes of cmd1, cmd2 */
+	for (i = 0; i < 4; i++) {
+		checksum += (cmd1 >> (i * 8)) & 0xFF;
+		checksum += (cmd2 >> (i * 8)) & 0xFF;
+	}
+
+	cmd2 |= checksum << ((2 - length) * 8);
+
+	/* Init OCB */
+	rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
+				     OCB_OR_INIT0, OCB_OR_INIT1);
+	if (rc)
+		goto err;
+
+	rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_AND,
+				     OCB_AND_INIT0, OCB_AND_INIT1);
+	if (rc)
+		goto err;
+
+	/* Send command, 2nd half of the 64-bit addr is unused (write 0) */
+	rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
+				     driver->config.command_addr, 0);
+	if (rc)
+		goto err;
+
+	rc = driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1, cmd2);
+	if (rc)
+		goto err;
+
+	/* Trigger attention */
+	rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA, ATTN0, ATTN1);
+	if (rc)
+		goto err;
+
+	/* Get response data */
+	rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
+				     driver->config.response_addr, 0);
+	if (rc)
+		goto err;
+
+	do {
+		if (retry) {
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
+		}
+
+		rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
+					     (u64 *)resp);
+		if (rc)
+			goto err;
+
+		/* retry if we get "command in progress" return status */
+		retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) &&
+			(tries++ < CMD_IN_PRG_RETRIES);
+	} while (retry);
+
+	switch (resp[RESP_RETURN_STATUS]) {
+	case RESP_RETURN_CMD_IN_PRG:
+		rc = -EALREADY;
+		break;
+	case RESP_RETURN_SUCCESS:
+		rc = 0;
+		break;
+	case RESP_RETURN_CMD_INVAL:
+	case RESP_RETURN_CMD_LEN:
+	case RESP_RETURN_DATA_INVAL:
+	case RESP_RETURN_CHKSUM:
+		rc = -EINVAL;
+		break;
+	case RESP_RETURN_OCC_ERR:
+		rc = -EREMOTE;
+		break;
+	default:
+		rc = -EFAULT;
+	}
+
+	return rc;
+
+err:
+	dev_err(driver->dev, "scom op failed rc:%d\n", rc);
+	return rc;
+}
+
+static int occ_get_all(struct occ *driver)
+{
+	int i = 0, rc;
+	u8 *occ_data;
+	u16 num_bytes;
+	const u8 poll_cmd_data = OCC_POLL_STAT_SENSOR;
+	struct device *dev = driver->dev;
+	struct occ_response *resp = &driver->response;
+
+	occ_data = devm_kzalloc(dev, OCC_DATA_MAX, GFP_KERNEL);
+	if (!occ_data)
+		return -ENOMEM;
+
+	rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data, occ_data);
+	if (rc) {
+		dev_err(dev, "OCC poll failed: %d\n", rc);
+		goto out;
+	}
+
+	num_bytes = get_unaligned((u16 *)&occ_data[RESP_DATA_LENGTH]);
+	num_bytes = be16_to_cpu(num_bytes);
+	dev_dbg(dev, "OCC data length: %d\n", num_bytes);
+
+	if (num_bytes > OCC_DATA_MAX) {
+		dev_err(dev, "OCC data length must be < 4KB\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
+	if (num_bytes <= 0) {
+		dev_err(dev, "OCC data length is zero\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
+	/* read remaining data */
+	for (i = 8; i < num_bytes + 8; i += 8) {
+		rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
+					     (u64 *)&occ_data[i]);
+		if (rc) {
+			dev_err(dev, "scom op failed rc:%d\n", rc);
+			goto out;
+		}
+	}
+
+	/* don't need more sanity checks; buffer is alloc'd for max response
+	 * size so we just check for valid data in parse_occ_response
+	 */
+	rc = parse_occ_response(driver, occ_data, resp);
+
+out:
+	devm_kfree(dev, occ_data);
+	return rc;
+}
+
+int occ_update_device(struct occ *driver)
+{
+	int rc = 0;
+
+	mutex_lock(&driver->update_lock);
+
+	if (time_after(jiffies, driver->last_updated + driver->update_interval)
+	    || !driver->valid) {
+		driver->valid = 1;
+
+		rc = occ_get_all(driver);
+		if (rc)
+			driver->valid = 0;
+
+		driver->last_updated = jiffies;
+	}
+
+	mutex_unlock(&driver->update_lock);
+
+	return rc;
+}
+EXPORT_SYMBOL(occ_update_device);
+
+void *occ_get_sensor(struct occ *driver, int sensor_type)
+{
+	int rc;
+
+	/* occ_update_device locks the update lock */
+	rc = occ_update_device(driver);
+	if (rc) {
+		dev_err(driver->dev, "cannot get occ sensor data: %d\n",
+			rc);
+		return NULL;
+	}
+
+	return occ_get_sensor_by_type(&driver->response, sensor_type);
+}
+EXPORT_SYMBOL(occ_get_sensor);
+
+int occ_get_sensor_value(struct occ *occ, int sensor_type, int sensor_num,
+			 u32 hwmon, long *val)
+{
+	return occ->ops.get_sensor(occ, sensor_type, sensor_num, hwmon, val);
+}
+EXPORT_SYMBOL(occ_get_sensor_value);
+
+void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks)
+{
+	*blocks = &occ->response.data;
+}
+EXPORT_SYMBOL(occ_get_response_blocks);
+
+void occ_set_update_interval(struct occ *occ, unsigned long interval)
+{
+	occ->update_interval = msecs_to_jiffies(interval);
+}
+EXPORT_SYMBOL(occ_set_update_interval);
+
+int occ_set_user_powercap(struct occ *occ, u16 cap)
+{
+	u8 resp[8];
+
+	cap = cpu_to_be16(cap);
+
+	return occ_send_cmd(occ, 0, OCC_SET_USER_POWR_CAP, 2, (const u8 *)&cap,
+			    resp);
+}
+EXPORT_SYMBOL(occ_set_user_powercap);
+
+struct occ *occ_start(struct device *dev, void *bus,
+		      struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
+		      const struct occ_config *config)
+{
+	struct occ *driver = devm_kzalloc(dev, sizeof(struct occ), GFP_KERNEL);
+
+	if (!driver)
+		return ERR_PTR(-ENOMEM);
+
+	driver->dev = dev;
+	driver->bus = bus;
+	driver->bus_ops = *bus_ops;
+	driver->ops = *ops;
+	driver->config = *config;
+
+	driver->update_interval = HZ;
+	mutex_init(&driver->update_lock);
+
+	return driver;
+}
+EXPORT_SYMBOL(occ_start);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("OCC hwmon core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ.h b/drivers/hwmon/occ/occ.h
new file mode 100644
index 0000000..c86b06a
--- /dev/null
+++ b/drivers/hwmon/occ/occ.h
@@ -0,0 +1,81 @@
+/*
+ * occ.h - hwmon OCC driver
+ *
+ * This file contains data structures and function prototypes for common access
+ * between different bus protocols and host systems.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_H__
+#define __OCC_H__
+
+#include "scom.h"
+
+struct device;
+struct occ;
+
+/* sensor_data_block_header
+ * structure to match the raw occ sensor block header
+ */
+struct sensor_data_block_header {
+	u8 sensor_type[4];
+	u8 reserved0;
+	u8 sensor_format;
+	u8 sensor_length;
+	u8 sensor_num;
+} __attribute__((packed, aligned(4)));
+
+struct sensor_data_block {
+	struct sensor_data_block_header header;
+	void *sensors;
+};
+
+enum sensor_type {
+	FREQ = 0,
+	TEMP,
+	POWER,
+	CAPS,
+	MAX_OCC_SENSOR_TYPE
+};
+
+struct occ_ops {
+	void (*parse_sensor)(u8 *data, void *sensor, int sensor_type, int off,
+			     int snum);
+	void *(*alloc_sensor)(int sensor_type, int num_sensors);
+	int (*get_sensor)(struct occ *driver, int sensor_type, int sensor_num,
+			  u32 hwmon, long *val);
+};
+
+struct occ_config {
+	u32 command_addr;
+	u32 response_addr;
+};
+
+struct occ_blocks {
+	int sensor_block_id[MAX_OCC_SENSOR_TYPE];
+	struct sensor_data_block *blocks;
+};
+
+struct occ *occ_start(struct device *dev, void *bus,
+		      struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
+		      const struct occ_config *config);
+void *occ_get_sensor(struct occ *occ, int sensor_type);
+int occ_get_sensor_value(struct occ *occ, int sensor_type, int sensor_num,
+			 u32 hwmon, long *val);
+void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks);
+int occ_update_device(struct occ *driver);
+void occ_set_update_interval(struct occ *occ, unsigned long interval);
+int occ_set_user_powercap(struct occ *occ, u16 cap);
+
+#endif /* __OCC_H__ */
diff --git a/drivers/hwmon/occ/scom.h b/drivers/hwmon/occ/scom.h
new file mode 100644
index 0000000..c1da645
--- /dev/null
+++ b/drivers/hwmon/occ/scom.h
@@ -0,0 +1,47 @@
+/*
+ * scom.h - hwmon OCC driver
+ *
+ * This file contains data structures for scom operations to the OCC
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#ifndef __SCOM_H__
+#define __SCOM_H__
+
+/*
+ * occ_bus_ops - represent the low-level transfer methods to communicate with
+ * the OCC.
+ *
+ * getscom - OCC scom read
+ * @bus: handle to slave device
+ * @address: address
+ * @data: where to store data read from slave; buffer size must be at least
+ * eight bytes.
+ *
+ * Returns 0 on success or a negative errno on error
+ *
+ * putscom - OCC scom write
+ * @bus: handle to slave device
+ * @address: address
+ * @data0: first data byte to write
+ * @data1: second data byte to write
+ *
+ * Returns 0 on success or a negative errno on error
+ */
+struct occ_bus_ops {
+	int (*getscom)(void *bus, u32 address, u64 *data);
+	int (*putscom)(void *bus, u32 address, u32 data0, u32 data1);
+};
+
+#endif /* __SCOM_H__ */
-- 
1.9.1

[PATCH linux v3 2/6] hwmon: occ: Add sysfs interface

[eajames.ibm]

From: "Edward A. James" <eajames@us.ibm.com>

Add a generic mechanism to expose the sensors provided by the OCC in
sysfs.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/hwmon/occ       |  62 ++++++++++
 drivers/hwmon/occ/Makefile    |   2 +-
 drivers/hwmon/occ/occ_sysfs.c | 271 ++++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ_sysfs.h |  44 +++++++
 4 files changed, 378 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hwmon/occ/occ_sysfs.c
 create mode 100644 drivers/hwmon/occ/occ_sysfs.h

diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index 79d1642..d0bdf06 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -25,6 +25,68 @@ Currently, all versions of the OCC support four types of sensor data: power,
 temperature, frequency, and "caps," which indicate limits and thresholds used
 internally on the OCC.
 
+sysfs Entries
+-------------
+
+The OCC driver uses the hwmon sysfs framework to provide data to userspace.
+
+The driver exports a number of sysfs files for each type of sensor. The
+sensor-specific files vary depending on the processor type, though many of the
+attributes are common for both the POWER8 and POWER9.
+
+The hwmon interface cannot define every type of sensor that may be used.
+Therefore, the frequency sensor on the OCC uses the "input" type sensor defined
+by the hwmon interface, rather than defining a new type of custom sensor.
+
+Below are detailed the names and meaning of each sensor file for both types of
+processors. All sensors are read-only unless otherwise specified. <x> indicates
+the hwmon index. sensor id indicates the unique internal OCC identifer. Please
+see the POWER OCC specification for details on all these sensor values.
+
+frequency:
+	all processors:
+		in<x>_input - frequency value
+		in<x>_label - sensor id
+temperature:
+	POWER8:
+		temp<x>_input - temperature value
+		temp<x>_label - sensor id
+	POWER9 (in addition to above):
+		temp<x>_type - FRU type
+
+power:
+	POWER8:
+		power<x>_input - power value
+		power<x>_label - sensor id
+		power<x>_average - accumulator
+		power<x>_average_interval - update tag (number of samples in
+			accumulator)
+	POWER9:
+		power<x>_input - power value
+		power<x>_label - sensor id
+		power<x>_average_min - accumulator[0]
+		power<x>_average_max - accumulator[1] (64 bits total)
+		power<x>_average_interval - update tag
+		power<x>_reset_history - (function_id | (apss_channel << 8)
+
+caps:
+	POWER8:
+		power<x>_cap - current powercap
+		power<x>_cap_max - max powercap
+		power<x>_cap_min - min powercap
+		power<x>_max - normal powercap
+		power<x>_alarm - user powercap, r/w
+	POWER9:
+		power<x>_cap_alarm - user powercap source
+
+The driver also provides two sysfs entries through hwmon to better
+control the driver and monitor the master OCC. Though there may be multiple
+OCCs present on the system, these two files are only present for the "master"
+OCC.
+	name - read the name of the driver
+	update_interval - read or write the minimum interval for polling the
+		OCC.
+
 BMC - Host Communications
 -------------------------
 
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
index 93cb52f..a6881f9 100644
--- a/drivers/hwmon/occ/Makefile
+++ b/drivers/hwmon/occ/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
new file mode 100644
index 0000000..2f20c40
--- /dev/null
+++ b/drivers/hwmon/occ/occ_sysfs.c
@@ -0,0 +1,271 @@
+/*
+ * occ_sysfs.c - OCC sysfs interface
+ *
+ * This file contains the methods and data structures for implementing the OCC
+ * hwmon sysfs entries.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include "occ.h"
+#include "occ_sysfs.h"
+
+#define RESP_RETURN_CMD_INVAL	0x13
+
+static int occ_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			  u32 attr, int channel, long *val)
+{
+	int rc = 0;
+	struct occ_sysfs *driver = dev_get_drvdata(dev);
+	struct occ *occ = driver->occ;
+
+	switch (type) {
+	case hwmon_in:
+		rc = occ_get_sensor_value(occ, FREQ, channel, attr, val);
+		break;
+	case hwmon_temp:
+		rc = occ_get_sensor_value(occ, TEMP, channel, attr, val);
+		break;
+	case hwmon_power:
+		rc = occ_get_sensor_value(occ, POWER, channel, attr, val);
+		break;
+	default:
+		rc = -EOPNOTSUPP;
+	}
+
+	return rc;
+}
+
+static int occ_hwmon_read_string(struct device *dev,
+				 enum hwmon_sensor_types type, u32 attr,
+				 int channel, char **str)
+{
+	int rc;
+	unsigned long val = 0;
+
+	if (!((type == hwmon_in && attr == hwmon_in_label) ||
+	    (type == hwmon_temp && attr == hwmon_temp_label) ||
+	    (type == hwmon_power && attr == hwmon_power_label)))
+		return -EOPNOTSUPP;
+
+	rc = occ_hwmon_read(dev, type, attr, channel, &val);
+	if (rc < 0)
+		return rc;
+
+	rc = snprintf(*str, PAGE_SIZE - 1, "%ld", val);
+	if (rc > 0)
+		rc = 0;
+
+	return rc;
+}
+
+static int occ_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			   u32 attr, int channel, long val)
+{
+	int rc = 0;
+	struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+	if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
+		occ_set_update_interval(driver->occ, val);
+		return 0;
+	} else if (type == hwmon_power && attr == hwmon_power_alarm) {
+		rc = occ_set_user_powercap(driver->occ, val);
+		if (rc) {
+			if (rc == RESP_RETURN_CMD_INVAL) {
+				dev_err(dev,
+					"set invalid powercap value: %ld\n",
+					val);
+				return -EINVAL;
+			}
+
+			dev_err(dev, "set user powercap failed: 0x:%x\n", rc);
+			return rc;
+		}
+
+		driver->user_powercap = val;
+
+		return rc;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static umode_t occ_is_visible(const void *data, enum hwmon_sensor_types type,
+			      u32 attr, int channel)
+{
+	const struct occ_sysfs *driver = data;
+
+	switch (type) {
+	case hwmon_chip:
+		if (attr == hwmon_chip_update_interval)
+			return S_IRUGO | S_IWUSR;
+		break;
+	case hwmon_in:
+		if (BIT(attr) & driver->sensor_hwmon_configs[0])
+			return S_IRUGO;
+		break;
+	case hwmon_temp:
+		if (BIT(attr) & driver->sensor_hwmon_configs[1])
+			return S_IRUGO;
+		break;
+	case hwmon_power:
+		/* user power limit */
+		if (attr == hwmon_power_alarm)
+			return S_IRUGO | S_IWUSR;
+		else if ((BIT(attr) & driver->sensor_hwmon_configs[2]) ||
+			 (BIT(attr) & driver->sensor_hwmon_configs[3]))
+			return S_IRUGO;
+		break;
+	default:
+		return 0;
+	}
+
+	return 0;
+}
+
+static const struct hwmon_ops occ_hwmon_ops = {
+	.is_visible = occ_is_visible,
+	.read = occ_hwmon_read,
+	.read_string = occ_hwmon_read_string,
+	.write = occ_hwmon_write,
+};
+
+static const u32 occ_chip_config[] = {
+	HWMON_C_UPDATE_INTERVAL,
+	0
+};
+
+static const struct hwmon_channel_info occ_chip = {
+	.type = hwmon_chip,
+	.config = occ_chip_config
+};
+
+static const enum hwmon_sensor_types occ_sensor_types[MAX_OCC_SENSOR_TYPE] = {
+	hwmon_in,
+	hwmon_temp,
+	hwmon_power,
+	hwmon_power
+};
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+				  const u32 *sensor_hwmon_configs,
+				  const char *name)
+{
+	bool master_occ = false;
+	int rc, i, j, sensor_num, index = 0, id;
+	char *brk;
+	struct occ_blocks *resp = NULL;
+	u32 *sensor_config;
+	struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
+					       GFP_KERNEL);
+	if (!hwmon)
+		return ERR_PTR(-ENOMEM);
+
+	/* need space for null-termination and occ chip */
+	hwmon->occ_sensors =
+		devm_kzalloc(dev, sizeof(struct hwmon_channel_info *) *
+			     (MAX_OCC_SENSOR_TYPE + 2), GFP_KERNEL);
+	if (!hwmon->occ_sensors)
+		return ERR_PTR(-ENOMEM);
+
+	hwmon->occ = occ;
+	hwmon->sensor_hwmon_configs = (u32 *)sensor_hwmon_configs;
+	hwmon->occ_info.ops = &occ_hwmon_ops;
+	hwmon->occ_info.info =
+		(const struct hwmon_channel_info **)hwmon->occ_sensors;
+
+	occ_get_response_blocks(occ, &resp);
+
+	for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+		resp->sensor_block_id[i] = -1;
+
+	/* read sensor data from occ */
+	rc = occ_update_device(occ);
+	if (rc) {
+		dev_err(dev, "cannot get occ sensor data: %d\n", rc);
+		return ERR_PTR(rc);
+	}
+	if (!resp->blocks)
+		return ERR_PTR(-ENOMEM);
+
+	master_occ = resp->sensor_block_id[CAPS] >= 0;
+
+	for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
+		id = resp->sensor_block_id[i];
+		if (id < 0)
+			continue;
+
+		sensor_num = resp->blocks[id].header.sensor_num;
+		/* need null-termination */
+		sensor_config = devm_kzalloc(dev,
+					     sizeof(u32) * (sensor_num + 1),
+					     GFP_KERNEL);
+		if (!sensor_config)
+			return ERR_PTR(-ENOMEM);
+
+		for (j = 0; j < sensor_num; j++)
+			sensor_config[j] = sensor_hwmon_configs[i];
+
+		hwmon->occ_sensors[index] =
+			devm_kzalloc(dev, sizeof(struct hwmon_channel_info),
+				     GFP_KERNEL);
+		if (!hwmon->occ_sensors[index])
+			return ERR_PTR(-ENOMEM);
+
+		hwmon->occ_sensors[index]->type = occ_sensor_types[i];
+		hwmon->occ_sensors[index]->config = sensor_config;
+		index++;
+	}
+
+	/* only need one of these for any number of occs */
+	if (master_occ)
+		hwmon->occ_sensors[index] =
+			(struct hwmon_channel_info *)&occ_chip;
+
+	/* search for bad chars */
+	strncpy(hwmon->hwmon_name, name, OCC_HWMON_NAME_LENGTH);
+	brk = strpbrk(hwmon->hwmon_name, "-* \t\n");
+	while (brk) {
+		*brk = '_';
+		brk = strpbrk(brk,  "-* \t\n");
+	}
+
+	hwmon->dev = devm_hwmon_device_register_with_info(dev,
+							  hwmon->hwmon_name,
+							  hwmon,
+							  &hwmon->occ_info,
+							  NULL);
+	if (IS_ERR(hwmon->dev)) {
+		dev_err(dev, "cannot register hwmon device %s: %ld\n",
+			hwmon->hwmon_name, PTR_ERR(hwmon->dev));
+		return ERR_CAST(hwmon->dev);
+	}
+
+	return hwmon;
+}
+EXPORT_SYMBOL(occ_sysfs_start);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("OCC sysfs driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_sysfs.h b/drivers/hwmon/occ/occ_sysfs.h
new file mode 100644
index 0000000..7de92e7
--- /dev/null
+++ b/drivers/hwmon/occ/occ_sysfs.h
@@ -0,0 +1,44 @@
+/*
+ * occ_sysfs.h - OCC sysfs interface
+ *
+ * This file contains the data structures and function prototypes for the OCC
+ * hwmon sysfs entries.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_SYSFS_H__
+#define __OCC_SYSFS_H__
+
+#include <linux/hwmon.h>
+
+struct occ;
+struct device;
+
+#define OCC_HWMON_NAME_LENGTH	32
+
+struct occ_sysfs {
+	struct device *dev;
+	struct occ *occ;
+
+	char hwmon_name[OCC_HWMON_NAME_LENGTH + 1];
+	u32 *sensor_hwmon_configs;
+	struct hwmon_channel_info **occ_sensors;
+	struct hwmon_chip_info occ_info;
+	u16 user_powercap;
+};
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+				  const u32 *sensor_hwmon_configs,
+				  const char *name);
+#endif /* __OCC_SYSFS_H__ */
-- 
1.9.1

[PATCH linux v3 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations

[eajames.ibm]

From: "Edward A. James" <eajames@us.ibm.com>

Add functions to send SCOM operations over I2C bus. The BMC can
communicate with the Power8 host processor over I2C, but needs to use SCOM
operations in order to access the OCC register space.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/occ/occ_scom_i2c.c | 72 ++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ_scom_i2c.h | 26 +++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h

diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
new file mode 100644
index 0000000..8b4ca13
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.c
@@ -0,0 +1,72 @@
+/*
+ * occ_scom_i2c.c - hwmon OCC driver
+ *
+ * This file contains the functions for performing SCOM operations over I2C bus
+ * to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "occ_scom_i2c.h"
+#include "scom.h"
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data)
+{
+	ssize_t rc;
+	u64 buf;
+	struct i2c_client *client = bus;
+
+	rc = i2c_master_send(client, (const char *)&address, sizeof(u32));
+	if (rc < 0)
+		return rc;
+	else if (rc != sizeof(u32))
+		return -EIO;
+
+	rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
+	if (rc < 0)
+		return rc;
+	else if (rc != sizeof(u64))
+		return -EIO;
+
+	*data = be64_to_cpu(buf);
+
+	return 0;
+}
+EXPORT_SYMBOL(occ_i2c_getscom);
+
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+	u32 buf[3];
+	ssize_t rc;
+	struct i2c_client *client = bus;
+
+	buf[0] = address;
+	buf[1] = data1;
+	buf[2] = data0;
+
+	rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
+	if (rc < 0)
+		return rc;
+	else if (rc != sizeof(u32) * 3)
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL(occ_i2c_putscom);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("I2C OCC SCOM transport");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_scom_i2c.h b/drivers/hwmon/occ/occ_scom_i2c.h
new file mode 100644
index 0000000..945739c
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.h
@@ -0,0 +1,26 @@
+/*
+ * occ_scom_i2c.h - hwmon OCC driver
+ *
+ * This file contains function protoypes for peforming SCOM operations over I2C
+ * bus to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_SCOM_I2C_H__
+#define __OCC_SCOM_I2C_H__
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data);
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
+
+#endif /* __OCC_SCOM_I2C_H__ */
-- 
1.9.1

[PATCH linux v3 4/6] hwmon: occ: Add callbacks for parsing P8 OCC datastructures

[eajames.ibm]

From: "Edward A. James" <eajames@us.ibm.com>

Add functions to parse the data structures that are specific to the OCC on
the POWER8 processor. These are the sensor data structures, including
temperature, frequency, power, and "caps."

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/hwmon/occ    |   9 ++
 drivers/hwmon/occ/occ_p8.c | 247 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ_p8.h |  30 ++++++
 3 files changed, 286 insertions(+)
 create mode 100644 drivers/hwmon/occ/occ_p8.c
 create mode 100644 drivers/hwmon/occ/occ_p8.h

diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index d0bdf06..143951e 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -25,6 +25,15 @@ Currently, all versions of the OCC support four types of sensor data: power,
 temperature, frequency, and "caps," which indicate limits and thresholds used
 internally on the OCC.
 
+The format for the POWER8 OCC sensor data can be found in the P8 OCC
+specification:
+github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf
+This document provides the details of the OCC sensors: power, frequency,
+temperature, and caps. These sensor formats are specific to the POWER8 OCC. A
+number of data structures, such as command format, response headers, and the
+like, are also defined in this specification, and are common to both POWER8 and
+POWER9 OCCs.
+
 sysfs Entries
 -------------
 
diff --git a/drivers/hwmon/occ/occ_p8.c b/drivers/hwmon/occ/occ_p8.c
new file mode 100644
index 0000000..32215ed
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p8.c
@@ -0,0 +1,247 @@
+/*
+ * occ_p8.c - OCC hwmon driver
+ *
+ * This file contains the Power8-specific methods and data structures for
+ * the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include "occ.h"
+#include "occ_p8.h"
+
+/* P8 OCC sensor data format */
+struct p8_occ_sensor {
+	u16 sensor_id;
+	u16 value;
+};
+
+struct p8_power_sensor {
+	u16 sensor_id;
+	u32 update_tag;
+	u32 accumulator;
+	u16 value;
+};
+
+struct p8_caps_sensor {
+	u16 curr_powercap;
+	u16 curr_powerreading;
+	u16 norm_powercap;
+	u16 max_powercap;
+	u16 min_powercap;
+	u16 user_powerlimit;
+};
+
+static const u32 p8_sensor_hwmon_configs[MAX_OCC_SENSOR_TYPE] = {
+	HWMON_I_INPUT | HWMON_I_LABEL,	/* freq: value | label */
+	HWMON_T_INPUT | HWMON_T_LABEL,	/* temp: value | label */
+	/* power: value | label | accumulator | update_tag */
+	HWMON_P_INPUT | HWMON_P_LABEL | HWMON_P_AVERAGE |
+		HWMON_P_AVERAGE_INTERVAL,
+	/* caps: curr | max | min | norm | user */
+	HWMON_P_CAP | HWMON_P_CAP_MAX | HWMON_P_CAP_MIN | HWMON_P_MAX |
+		HWMON_P_ALARM,
+};
+
+void p8_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
+		     int snum)
+{
+	switch (sensor_type) {
+	case FREQ:
+	case TEMP:
+	{
+		struct p8_occ_sensor *os =
+			&(((struct p8_occ_sensor *)sensor)[snum]);
+
+		os->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
+		os->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+	}
+		break;
+	case POWER:
+	{
+		struct p8_power_sensor *ps =
+			&(((struct p8_power_sensor *)sensor)[snum]);
+
+		ps->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
+		ps->update_tag =
+			be32_to_cpu(get_unaligned((u32 *)&data[off + 2]));
+		ps->accumulator =
+			be32_to_cpu(get_unaligned((u32 *)&data[off + 6]));
+		ps->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+	}
+		break;
+	case CAPS:
+	{
+		struct p8_caps_sensor *cs =
+			&(((struct p8_caps_sensor *)sensor)[snum]);
+
+		cs->curr_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off]));
+		cs->curr_powerreading =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+		cs->norm_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+		cs->max_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 6]));
+		cs->min_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 8]));
+		cs->user_powerlimit =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+	}
+		break;
+	};
+}
+
+void *p8_alloc_sensor(int sensor_type, int num_sensors)
+{
+	switch (sensor_type) {
+	case FREQ:
+	case TEMP:
+		return kcalloc(num_sensors, sizeof(struct p8_occ_sensor),
+			       GFP_KERNEL);
+	case POWER:
+		return kcalloc(num_sensors, sizeof(struct p8_power_sensor),
+			       GFP_KERNEL);
+	case CAPS:
+		return kcalloc(num_sensors, sizeof(struct p8_caps_sensor),
+			       GFP_KERNEL);
+	default:
+		return NULL;
+	}
+}
+
+int p8_get_sensor(struct occ *driver, int sensor_type, int sensor_num,
+		  u32 hwmon, long *val)
+{
+	int rc = 0;
+	void *sensor;
+
+	if (sensor_type == POWER) {
+		if (hwmon == hwmon_power_cap || hwmon == hwmon_power_cap_max ||
+		    hwmon == hwmon_power_cap_min || hwmon == hwmon_power_max ||
+		    hwmon == hwmon_power_alarm)
+			sensor_type = CAPS;
+	}
+
+	sensor = occ_get_sensor(driver, sensor_type);
+	if (!sensor)
+		return -ENODEV;
+
+	switch (sensor_type) {
+	case FREQ:
+	case TEMP:
+	{
+		struct p8_occ_sensor *os =
+			&(((struct p8_occ_sensor *)sensor)[sensor_num]);
+
+		if (hwmon == hwmon_in_input || hwmon == hwmon_temp_input)
+			*val = os->value;
+		else if (hwmon == hwmon_in_label || hwmon == hwmon_temp_label)
+			*val = os->sensor_id;
+		else
+			rc = -EOPNOTSUPP;
+	}
+		break;
+	case POWER:
+	{
+		struct p8_power_sensor *ps =
+			&(((struct p8_power_sensor *)sensor)[sensor_num]);
+
+		switch (hwmon) {
+		case hwmon_power_input:
+			*val = ps->value;
+			break;
+		case hwmon_power_label:
+			*val = ps->sensor_id;
+			break;
+		case hwmon_power_average:
+			*val = ps->accumulator;
+			break;
+		case hwmon_power_average_interval:
+			*val = ps->update_tag;
+			break;
+		default:
+			rc = -EOPNOTSUPP;
+		}
+	}
+		break;
+	case CAPS:
+	{
+		struct p8_caps_sensor *cs =
+			&(((struct p8_caps_sensor *)sensor)[sensor_num]);
+
+		switch (hwmon) {
+		case hwmon_power_cap:
+			*val = cs->curr_powercap;
+			break;
+		case hwmon_power_cap_max:
+			*val = cs->max_powercap;
+			break;
+		case hwmon_power_cap_min:
+			*val = cs->min_powercap;
+			break;
+		case hwmon_power_max:
+			*val = cs->norm_powercap;
+			break;
+		case hwmon_power_alarm:
+			*val = cs->user_powerlimit;
+			break;
+		default:
+			rc = -EOPNOTSUPP;
+		}
+	}
+		break;
+	default:
+		rc = -EINVAL;
+	}
+
+	return rc;
+}
+
+static const struct occ_ops p8_ops = {
+	.parse_sensor = p8_parse_sensor,
+	.alloc_sensor = p8_alloc_sensor,
+	.get_sensor = p8_get_sensor,
+};
+
+static const struct occ_config p8_config = {
+	.command_addr = 0xFFFF6000,
+	.response_addr = 0xFFFF7000,
+};
+
+const u32 *p8_get_sensor_hwmon_configs()
+{
+	return p8_sensor_hwmon_configs;
+}
+EXPORT_SYMBOL(p8_get_sensor_hwmon_configs);
+
+struct occ *p8_occ_start(struct device *dev, void *bus,
+			 struct occ_bus_ops *bus_ops)
+{
+	return occ_start(dev, bus, bus_ops, &p8_ops, &p8_config);
+}
+EXPORT_SYMBOL(p8_occ_start);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("P8 OCC sensors");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_p8.h b/drivers/hwmon/occ/occ_p8.h
new file mode 100644
index 0000000..3fe6417
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p8.h
@@ -0,0 +1,30 @@
+/*
+ * occ_p8.h - OCC hwmon driver
+ *
+ * This file contains Power8-specific function prototypes
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_P8_H__
+#define __OCC_P8_H__
+
+#include "scom.h"
+
+struct device;
+
+const u32 *p8_get_sensor_hwmon_configs(void);
+struct occ *p8_occ_start(struct device *dev, void *bus,
+			 struct occ_bus_ops *bus_ops);
+
+#endif /* __OCC_P8_H__ */
-- 
1.9.1

[PATCH linux v3 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC

[eajames.ibm]

From: "Edward A. James" <eajames@us.ibm.com>

Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
well as probe the entire driver from the I2C bus. I2C is the communication
method between the BMC and the P8 OCC.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/devicetree/bindings/hwmon/occ.txt |  13 +++
 drivers/hwmon/occ/Kconfig                       |  14 ++++
 drivers/hwmon/occ/Makefile                      |   1 +
 drivers/hwmon/occ/p8_occ_i2c.c                  | 104 ++++++++++++++++++++++++
 4 files changed, 132 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
 create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c

diff --git a/Documentation/devicetree/bindings/hwmon/occ.txt b/Documentation/devicetree/bindings/hwmon/occ.txt
new file mode 100644
index 0000000..b0d2b36
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/occ.txt
@@ -0,0 +1,13 @@
+HWMON I2C driver for IBM POWER CPU OCC (On Chip Controller)
+
+Required properties:
+ - compatible: must be "ibm,p8-occ-i2c"
+ - reg: physical address
+
+Example:
+i2c3: i2c-bus@100 {
+	occ@50 {
+		compatible = "ibm,p8-occ-i2c";
+		reg = <0x50>;
+	};
+};
diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
index cdb64a7..3a5188f 100644
--- a/drivers/hwmon/occ/Kconfig
+++ b/drivers/hwmon/occ/Kconfig
@@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
 
 	  This driver can also be built as a module. If so, the module
 	  will be called occ.
+
+if SENSORS_PPC_OCC
+
+config SENSORS_PPC_OCC_P8_I2C
+	tristate "POWER8 OCC hwmon support"
+	depends on I2C
+	help
+	 Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
+	 exposing temperature, frequency and power measurements.
+
+	 This driver can also be built as a module. If so, the module will be
+	 called p8-occ-i2c.
+
+endif
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
index a6881f9..9294b58 100644
--- a/drivers/hwmon/occ/Makefile
+++ b/drivers/hwmon/occ/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
+obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o
diff --git a/drivers/hwmon/occ/p8_occ_i2c.c b/drivers/hwmon/occ/p8_occ_i2c.c
new file mode 100644
index 0000000..6273040
--- /dev/null
+++ b/drivers/hwmon/occ/p8_occ_i2c.c
@@ -0,0 +1,104 @@
+/*
+ * p8_occ_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the P8 OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include "occ_p8.h"
+#include "occ_scom_i2c.h"
+#include "occ_sysfs.h"
+#include "scom.h"
+
+#define P8_OCC_I2C_NAME	"p8-occ-i2c"
+
+int p8_i2c_getscom(void *bus, u32 address, u64 *data)
+{
+	/* P8 i2c slave requires address to be shifted by 1 */
+	address = address << 1;
+
+	return occ_i2c_getscom(bus, address, data);
+}
+
+int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+	/* P8 i2c slave requires address to be shifted by 1 */
+	address = address << 1;
+
+	return occ_i2c_putscom(bus, address, data0, data1);
+}
+
+static struct occ_bus_ops p8_bus_ops = {
+	.getscom = p8_i2c_getscom,
+	.putscom = p8_i2c_putscom,
+};
+
+static int p8_occ_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct occ *occ;
+	struct occ_sysfs *hwmon;
+	const u32 *sensor_hwmon_configs = p8_get_sensor_hwmon_configs();
+
+	occ = p8_occ_start(&client->dev, client, &p8_bus_ops);
+	if (IS_ERR(occ))
+		return PTR_ERR(occ);
+
+	hwmon = occ_sysfs_start(&client->dev, occ, sensor_hwmon_configs,
+				P8_OCC_I2C_NAME);
+	if (IS_ERR(hwmon))
+		return PTR_ERR(hwmon);
+
+	i2c_set_clientdata(client, occ);
+
+	return 0;
+}
+
+/* used by old-style board info. */
+static const struct i2c_device_id occ_ids[] = {
+	{ P8_OCC_I2C_NAME, 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, occ_ids);
+
+/* used by device table */
+static const struct of_device_id occ_of_match[] = {
+	{ .compatible = "ibm,p8-occ-i2c" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, occ_of_match);
+
+static struct i2c_driver p8_occ_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name = P8_OCC_I2C_NAME,
+		.of_match_table = occ_of_match,
+	},
+	.probe = p8_occ_probe,
+	.id_table = occ_ids,
+};
+
+module_i2c_driver(p8_occ_driver);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH linux v3 6/6] hwmon: occ: Add callbacks for parsing P9 OCC datastructures

[eajames.ibm]

From: "Edward A. James" <eajames@us.ibm.com>

Add functions to parse the data structures that are specific to the OCC on
the POWER9 processor. These are the sensor data structures, including
temperature, frequency, power, and "caps."

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/hwmon/occ    |   3 +
 drivers/hwmon/occ/occ_p9.c | 308 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ_p9.h |  30 +++++
 3 files changed, 341 insertions(+)
 create mode 100644 drivers/hwmon/occ/occ_p9.c
 create mode 100644 drivers/hwmon/occ/occ_p9.h

diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index 143951e..6cea853 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -34,6 +34,9 @@ number of data structures, such as command format, response headers, and the
 like, are also defined in this specification, and are common to both POWER8 and
 POWER9 OCCs.
 
+There is currently no public P9 OCC specification, and the data structures
+defined in the POWER9 OCC driver are subject to change.
+
 sysfs Entries
 -------------
 
diff --git a/drivers/hwmon/occ/occ_p9.c b/drivers/hwmon/occ/occ_p9.c
new file mode 100644
index 0000000..d99a026
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p9.c
@@ -0,0 +1,308 @@
+/*
+ * occ_p9.c - OCC hwmon driver
+ *
+ * This file contains the Power9-specific methods and data structures for
+ * the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include "occ.h"
+#include "occ_p9.h"
+
+/* P9 OCC sensor data format */
+struct p9_temp_sensor {
+	u32 sensor_id;
+	u8 fru_type;
+	u8 value;
+};
+
+struct p9_freq_sensor {
+	u32 sensor_id;
+	u16 value;
+};
+
+struct p9_power_sensor {
+	u32 sensor_id;
+	u8 function_id;
+	u8 apss_channel;
+	u16 reserved;
+	u32 update_tag;
+	u64 accumulator;
+	u16 value;
+};
+
+struct p9_caps_sensor {
+	u16 curr_powercap;
+	u16 curr_powerreading;
+	u16 norm_powercap;
+	u16 max_powercap;
+	u16 min_powercap;
+	u16 user_powerlimit;
+	u8 user_powerlimit_source;
+};
+
+static const u32 p9_sensor_hwmon_configs[MAX_OCC_SENSOR_TYPE] = {
+	HWMON_I_INPUT | HWMON_I_LABEL,	/* freq: value | label */
+	/* temp: value | label | fru_type */
+	HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_TYPE,
+	/* power: value | label | accum[0] | accum[1] | update_tag |
+	 *	 (function_id | (apss_channel << 8))
+	 */
+	HWMON_P_INPUT | HWMON_P_LABEL | HWMON_P_AVERAGE_MIN |
+		HWMON_P_AVERAGE_MAX | HWMON_P_AVERAGE_INTERVAL |
+		HWMON_P_RESET_HISTORY,
+	/* caps: curr | max | min | norm | user | source */
+	HWMON_P_CAP | HWMON_P_CAP_MAX | HWMON_P_CAP_MIN | HWMON_P_MAX |
+		HWMON_P_ALARM | HWMON_P_CAP_ALARM,
+};
+
+void p9_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
+		     int snum)
+{
+	switch (sensor_type) {
+	case FREQ:
+	{
+		struct p9_freq_sensor *fs =
+			&(((struct p9_freq_sensor *)sensor)[snum]);
+
+		fs->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+		fs->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+	}
+		break;
+	case TEMP:
+	{
+		struct p9_temp_sensor *ts =
+			&(((struct p9_temp_sensor *)sensor)[snum]);
+
+		ts->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+		fs->fru_type = data[off + 4];
+		fs->value = data[off + 5];
+	}
+		break;
+	case POWER:
+	{
+		struct p9_power_sensor *ps =
+			&(((struct p9_power_sensor *)sensor)[snum]);
+
+		ps->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+		ps->function_id = data[off + 4];
+		ps->apss_channel = data[off + 5];
+		ps->update_tag =
+			be32_to_cpu(get_unaligned((u32 *)&data[off + 8]));
+		ps->accumulator =
+			be64_to_cpu(get_unaligned((u64 *)&data[off + 12]));
+		ps->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 20]));
+	}
+		break;
+	case CAPS:
+	{
+		struct p9_caps_sensor *cs =
+			&(((struct p9_caps_sensor *)sensor)[snum]);
+
+		cs->curr_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off]));
+		cs->curr_powerreading =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+		cs->norm_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+		cs->max_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 6]));
+		cs->min_powercap =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 8]));
+		cs->user_powerlimit =
+			be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+		cs->user_powerlimit_source = data[off + 12];
+	}
+		break;
+	};
+}
+
+void *p9_alloc_sensor(int sensor_type, int num_sensors)
+{
+	switch (sensor_type) {
+	case FREQ:
+		return kcalloc(num_sensors, sizeof(struct p9_freq_sensor),
+			       GFP_KERNEL);
+	case TEMP:
+		return kcalloc(num_sensors, sizeof(struct p9_temp_sensor),
+			       GFP_KERNEL);
+	case POWER:
+		return kcalloc(num_sensors, sizeof(struct p9_power_sensor),
+			       GFP_KERNEL);
+	case CAPS:
+		return kcalloc(num_sensors, sizeof(struct p9_caps_sensor),
+			       GFP_KERNEL);
+	default:
+		return NULL;
+	}
+}
+
+int p9_get_sensor(struct occ *driver, int sensor_type, int sensor_num,
+		  u32 hwmon, long *val)
+{
+	int rc = 0;
+	void *sensor;
+
+	if (sensor_type == POWER) {
+		if (hwmon == hwmon_power_cap || hwmon == hwmon_power_cap_max ||
+		    hwmon == hwmon_power_cap_min || hwmon == hwmon_power_max ||
+		    hwmon == hwmon_power_alarm ||
+		    hwmon == hwmon_power_cap_alarm)
+			sensor_type = CAPS;
+	}
+
+	sensor = occ_get_sensor(driver, sensor_type);
+	if (!sensor)
+		return -ENODEV;
+
+	switch (sensor_type) {
+	case FREQ:
+	{
+		struct p9_freq_sensor *fs =
+			&(((struct p9_freq_sensor *)sensor)[snum]);
+
+		switch (hwmon) {
+		case hwmon_in_input:
+			*val = fs->value;
+			break;
+		case hwmon_in_label:
+			*val = fs->sensor_id;
+			break;
+		default:
+			rc = -EOPNOTSUPP;
+		}
+	}
+		break;
+	case TEMP:
+	{
+		struct p9_temp_sensor *ts =
+			&(((struct p9_temp_sensor *)sensor)[snum]);
+
+		switch (hwmon) {
+		case hwmon_temp_input:
+			*val = ts->value;
+			break;
+		case hwmon_temp_type:
+			*val = ts->fru_type;
+			break;
+		case hwmon_temp_label:
+			*val = ts->sensor_id;
+			break;
+		default:
+			rc = -EOPNOTSUPP;
+		}
+	}
+		break;
+	case POWER:
+	{
+		struct p9_power_sensor *ps =
+			&(((struct p9_power_sensor *)sensor)[snum]);
+
+		switch (hwmon) {
+		case hwmon_power_input:
+			*val = ps->value;
+			break;
+		case hwmon_power_label:
+			*val = ps->sensor_id;
+			break;
+		case hwmon_power_average_min:
+			*val = ((u32 *)(&ps->accumulator))[0];
+			break;
+		case hwmon_power_average_max:
+			*val = ((u32 *)(&ps->accumulator))[1];
+			break;
+		case hwmon_power_average_interval:
+			*val = ps->update_tag;
+			break;
+		case hwmon_power_reset_history:
+			*val = ps->function_id | (ps->apss_channel << 8);
+			break;
+		default:
+			rc = -EOPNOTSUPP;
+		}
+	}
+		break;
+	case CAPS:
+	{
+		struct p9_caps_sensor *cs =
+			&(((struct p9_caps_sensor *)sensor)[snum]);
+
+		switch (hwmon) {
+		case hwmon_power_cap:
+			*val = cs->curr_powercap;
+			break;
+		case hwmon_power_cap_max:
+			*val = cs->max_powercap;
+			break;
+		case hwmon_power_cap_min:
+			*val = cs->min_powercap;
+			break;
+		case hwmon_power_max:
+			*val = cs->norm_powercap;
+			break;
+		case hwmon_power_alarm:
+			*val = cs->user_powerlimit;
+			break;
+		case hwmon_power_cap_alarm:
+			*val = cs->user_powerlimit_source;
+			break;
+		default:
+			rc = -EOPNOTSUPP;
+		}
+	}
+		break;
+	default:
+		rc = -EINVAL;
+	}
+
+	return rc;
+}
+
+static const struct occ_ops p9_ops = {
+	.parse_sensor = p9_parse_sensor,
+	.alloc_sensor = p9_alloc_sensor,
+	.get_sensor = p9_get_sensor,
+};
+
+static const struct occ_config p9_config = {
+	.command_addr = 0xFFFBE000,
+	.response_addr = 0xFFFBF000,
+};
+
+const u32 *p9_get_sensor_hwmon_configs()
+{
+	return p9_sensor_hwmon_configs;
+}
+EXPORT_SYMBOL(p9_get_sensor_hwmon_configs);
+
+struct occ *p9_occ_start(struct device *dev, void *bus,
+			 struct occ_bus_ops *bus_ops)
+{
+	return occ_start(dev, bus, bus_ops, &p9_ops, &p9_config);
+}
+EXPORT_SYMBOL(p9_occ_start);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("P9 OCC sensors");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_p9.h b/drivers/hwmon/occ/occ_p9.h
new file mode 100644
index 0000000..18ca16a
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p9.h
@@ -0,0 +1,30 @@
+/*
+ * occ_p9.h - OCC hwmon driver
+ *
+ * This file contains Power9-specific function prototypes
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_P9_H__
+#define __OCC_P9_H__
+
+#include "scom.h"
+
+struct device;
+
+const u32 *p9_get_sensor_hwmon_configs(void);
+struct occ *p9_occ_start(struct device *dev, void *bus,
+			 struct occ_bus_ops *bus_ops);
+
+#endif /* __OCC_P9_H__ */
-- 
1.9.1

Re: [PATCH linux v3 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC

[Rob Herring]

On Mon, Jan 16, 2017 at 03:13:38PM -0600, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
> well as probe the entire driver from the I2C bus. I2C is the communication
> method between the BMC and the P8 OCC.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/hwmon/occ.txt |  13 +++

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

>  drivers/hwmon/occ/Kconfig                       |  14 ++++
>  drivers/hwmon/occ/Makefile                      |   1 +
>  drivers/hwmon/occ/p8_occ_i2c.c                  | 104 ++++++++++++++++++++++++
>  4 files changed, 132 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
>  create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c

Re: [PATCH linux v3 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs

[Guenter Roeck]

On 01/16/2017 01:13 PM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add core support for polling the OCC for it's sensor data and parsing that
> data into sensor-specific information.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/hwmon/occ    |  40 ++++
>  MAINTAINERS                |   7 +
>  drivers/hwmon/Kconfig      |   2 +
>  drivers/hwmon/Makefile     |   1 +
>  drivers/hwmon/occ/Kconfig  |  15 ++
>  drivers/hwmon/occ/Makefile |   1 +
>  drivers/hwmon/occ/occ.c    | 522 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ.h    |  81 +++++++
>  drivers/hwmon/occ/scom.h   |  47 ++++
>  9 files changed, 716 insertions(+)
>  create mode 100644 Documentation/hwmon/occ
>  create mode 100644 drivers/hwmon/occ/Kconfig
>  create mode 100644 drivers/hwmon/occ/Makefile
>  create mode 100644 drivers/hwmon/occ/occ.c
>  create mode 100644 drivers/hwmon/occ/occ.h
>  create mode 100644 drivers/hwmon/occ/scom.h
>
> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> new file mode 100644
> index 0000000..79d1642
> --- /dev/null
> +++ b/Documentation/hwmon/occ
> @@ -0,0 +1,40 @@
> +Kernel driver occ
> +=================
> +
> +Supported chips:
> + * ASPEED AST2400
> + * ASPEED AST2500
> +
> +Please note that the chip must be connected to a POWER8 or POWER9 processor
> +(see the BMC - Host Communications section).
> +
> +Author: Eddie James <eajames@us.ibm.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for the OCC (On-Chip Controller) on the IBM
> +POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The
> +OCC is an embedded processor that provides real time power and thermal
> +monitoring.
> +
> +This driver provides an interface on a BMC to poll OCC sensor data, set user
> +power caps, and perform some basic OCC error handling.
> +
> +Currently, all versions of the OCC support four types of sensor data: power,
> +temperature, frequency, and "caps," which indicate limits and thresholds used
> +internally on the OCC.
> +
> +BMC - Host Communications
> +-------------------------
> +
> +For the POWER8 application, the BMC can communicate with the P8 over I2C bus.
> +However, to access the OCC register space, any data transfer must use a SCOM
> +operation. SCOM is a procedure to initiate a data transfer, typically of 8
> +bytes. SCOMs consist of writing a 32-bit command register and then
> +reading/writing two 32-bit data registers. This driver implements these
> +SCOM operations over I2C bus in order to communicate with the OCC.
> +
> +For the POWER9 application, the BMC can communicate with the P9 over FSI bus
> +and SBE engine. Once again, SCOM operations are required. This driver will
> +implement SCOM ops over FSI/SBE. This will require the FSI driver.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f0420a..f5d4195 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9112,6 +9112,13 @@ T:	git git://linuxtv.org/media_tree.git
>  S:	Maintained
>  F:	drivers/media/i2c/ov7670.c
>
> +ON-CHIP CONTROLLER HWMON DRIVER
> +M:	Eddie James <eajames@us.ibm.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/occ
> +F:	drivers/hwmon/occ/
> +
>  ONENAND FLASH DRIVER
>  M:	Kyungmin Park <kyungmin.park@samsung.com>
>  L:	linux-mtd@lists.infradead.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 190d270..e80ca81 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1240,6 +1240,8 @@ config SENSORS_NSA320
>  	  This driver can also be built as a module. If so, the module
>  	  will be called nsa320-hwmon.
>
> +source drivers/hwmon/occ/Kconfig
> +
>  config SENSORS_PCF8591
>  	tristate "Philips PCF8591 ADC/DAC"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index d2cb7e8..c7ec5d4 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
>  obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
>  obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
>
> +obj-$(CONFIG_SENSORS_PPC_OCC)	+= occ/
>  obj-$(CONFIG_PMBUS)		+= pmbus/
>
>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
> new file mode 100644
> index 0000000..cdb64a7
> --- /dev/null
> +++ b/drivers/hwmon/occ/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# On Chip Controller configuration
> +#
> +
> +menuconfig SENSORS_PPC_OCC
> +	bool "PPC On-Chip Controller"
> +	help
> +	  If you say yes here you get support to monitor Power CPU
> +	  sensors via the On-Chip Controller (OCC).
> +
> +	  Generally this is used by management controllers such as a BMC
> +	  on an OpenPower system.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called occ.
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> new file mode 100644
> index 0000000..93cb52f
> --- /dev/null
> +++ b/drivers/hwmon/occ/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
> diff --git a/drivers/hwmon/occ/occ.c b/drivers/hwmon/occ/occ.c
> new file mode 100644
> index 0000000..3089762
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ.c
> @@ -0,0 +1,522 @@
> +/*
> + * occ.c - OCC hwmon driver
> + *
> + * This file contains the methods and data structures for the OCC hwmon driver.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include "occ.h"
> +
> +#define OCC_DATA_MAX		4096
> +#define OCC_BMC_TIMEOUT_MS	20000
> +
> +/* To generate attn to OCC */
> +#define ATTN_DATA		0x0006B035
> +
> +/* For BMC to read/write SRAM */
> +#define OCB_ADDRESS		0x0006B070
> +#define OCB_DATA		0x0006B075
> +#define OCB_STATUS_CONTROL_AND	0x0006B072
> +#define OCB_STATUS_CONTROL_OR	0x0006B073
> +
> +/* To init OCB */
> +#define OCB_AND_INIT0		0xFBFFFFFF
> +#define OCB_AND_INIT1		0xFFFFFFFF
> +#define OCB_OR_INIT0		0x08000000
> +#define OCB_OR_INIT1		0x00000000
> +
> +/* To generate attention on OCC */
> +#define ATTN0			0x01010000
> +#define ATTN1			0x00000000
> +
> +/* OCC return status */
> +#define RESP_RETURN_CMD_IN_PRG	0xFF
> +#define RESP_RETURN_SUCCESS	0
> +#define RESP_RETURN_CMD_INVAL	0x11
> +#define RESP_RETURN_CMD_LEN	0x12
> +#define RESP_RETURN_DATA_INVAL	0x13
> +#define RESP_RETURN_CHKSUM	0x14
> +#define RESP_RETURN_OCC_ERR	0x15
> +#define RESP_RETURN_STATE	0x16
> +
> +/* time interval to retry on "command in progress" return status */
> +#define CMD_IN_PRG_INT_MS	100
> +#define CMD_IN_PRG_RETRIES	(OCC_BMC_TIMEOUT_MS / CMD_IN_PRG_INT_MS)
> +
> +/* OCC command definitions */
> +#define OCC_POLL		0
> +#define OCC_SET_USER_POWR_CAP	0x22
> +
> +/* OCC poll command data */
> +#define OCC_POLL_STAT_SENSOR	0x10
> +
> +/* OCC response data offsets */
> +#define RESP_RETURN_STATUS	2
> +#define RESP_DATA_LENGTH	3
> +#define RESP_HEADER_OFFSET	5
> +#define SENSOR_STR_OFFSET	37
> +#define SENSOR_BLOCK_NUM_OFFSET	43
> +#define SENSOR_BLOCK_OFFSET	45
> +
> +/* occ_poll_header
> + * structure to match the raw occ poll response data
> + */
> +struct occ_poll_header {
> +	u8 status;
> +	u8 ext_status;
> +	u8 occs_present;
> +	u8 config;
> +	u8 occ_state;
> +	u8 mode;
> +	u8 ips_status;
> +	u8 error_log_id;
> +	u32 error_log_addr_start;
> +	u16 error_log_length;
> +	u8 reserved2;
> +	u8 reserved3;
> +	u8 occ_code_level[16];
> +	u8 sensor_eye_catcher[6];
> +	u8 sensor_block_num;
> +	u8 sensor_data_version;
> +} __attribute__((packed, aligned(4)));
> +
> +struct occ_response {
> +	struct occ_poll_header header;
> +	struct occ_blocks data;
> +};
> +
> +struct occ {
> +	struct device *dev;
> +	void *bus;
> +	struct occ_bus_ops bus_ops;
> +	struct occ_ops ops;
> +	struct occ_config config;
> +	unsigned long update_interval;
> +	unsigned long last_updated;
> +	struct mutex update_lock;
> +	struct occ_response response;
> +	bool valid;
> +};
> +
> +static void deinit_occ_resp_buf(struct occ_response *resp)
> +{
> +	int i;
> +
> +	if (!resp)
> +		return;
> +

This creates the impression that resp can ever be NULL, which is not the case.

> +	if (!resp->data.blocks)
> +		return;
> +
> +	for (i = 0; i < resp->header.sensor_block_num; ++i)
> +		kfree(resp->data.blocks[i].sensors);
> +
> +	kfree(resp->data.blocks);
> +/sensor_type_s
> +	memset(resp, 0, sizeof(struct occ_response));
> +
> +	for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
> +		resp->data.sensor_block_id[i] = -1;
> +}
> +
> +static void *occ_get_sensor_by_type(struct occ_response *resp,
> +				    enum sensor_type t)
> +{
> +	if (!resp->data.blocks)
> +		return NULL;
> +
> +	if (resp->data.sensor_block_id[t] == -1)
> +		return NULL;
> +
> +	return resp->data.blocks[resp->data.sensor_block_id[t]].sensors;
> +}
> +
> +static int occ_check_sensor(struct occ *driver, u8 sensor_length,
> +			    u8 sensor_num, enum sensor_type t, int block)
> +{
> +	void *sensor;
> +	int type_block_id;
> +	struct occ_response *resp = &driver->response;
> +
> +	sensor = occ_get_sensor_by_type(resp, t);
> +
> +	/* empty sensor block, release older sensor data */
> +	if (sensor_num == 0 || sensor_length == 0) {
> +		kfree(sensor);
> +		dev_err(driver->dev, "no sensor blocks available\n");
> +		return -ENODATA;
> +	}
> +
> +	type_block_id = resp->data.sensor_block_id[t];
> +	if (!sensor || sensor_num !=
> +	    resp->data.blocks[type_block_id].header.sensor_num) {
> +		kfree(sensor);
> +		resp->data.blocks[block].sensors =
> +			driver->ops.alloc_sensor(t, sensor_num);
> +		if (!resp->data.blocks[block].sensors)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int parse_occ_response(struct occ *driver, u8 *data,
> +			      struct occ_response *resp)
> +{
> +	int b;
> +	int s;
> +	int rc;
> +	int offset = SENSOR_BLOCK_OFFSET;
> +	int sensor_type;
> +	u8 sensor_block_num;
> +	char sensor_type_string[5] = { 0 };

The only purpose of this variable seems to be to zero-terminate it to be able
to print it. Why not use length-limited print functions instead ?

> +	struct sensor_data_block_header *block;
> +	struct device *dev = driver->dev;
> +
> +	/* check if the data is valid */
> +	if (strncmp(&data[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {
> +		dev_err(dev, "no SENSOR string in response\n");

Are those messages really necessary ? That can create a lot of logging noise
if the occ responds with bad data.

> +		rc = -ENODATA;
> +		goto err;
> +	}
> +
> +	sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET];

There is a lot of trust in assuming that this data was actually received.
At the same time, there is a lot of error checking. Why check if the
information in this field is valid if you don't check if it was
received in the first place ?

> +	if (sensor_block_num == 0) {
> +		dev_err(dev, "no sensor blocks available\n");
> +		rc = -ENODATA;
> +		goto err;
> +	}
> +
> +	/* if number of sensor block has changed, re-malloc */
> +	if (sensor_block_num != resp->header.sensor_block_num) {
> +		deinit_occ_resp_buf(resp);
> +		resp->data.blocks = kcalloc(sensor_block_num,
> +					    sizeof(struct sensor_data_block),
> +					    GFP_KERNEL);
> +		if (!resp->data.blocks)
> +			return -ENOMEM;

Why not just allocate the maximum number of sensors once ? That might waste
a bit of memory, but it would make the code much less error prone. We know that
there will never be more sensors than the number fitting into 4k of memory,
so the "waste" would not really be that much.

> +	}
> +
> +	memcpy(&resp->header, &data[RESP_HEADER_OFFSET],
> +	       sizeof(struct occ_poll_header));
> +	resp->header.error_log_addr_start =
> +		be32_to_cpu(resp->header.error_log_addr_start);
> +	resp->header.error_log_length =
> +		be16_to_cpu(resp->header.error_log_length);
> +
> +	dev_dbg(dev, "Reading %d sensor blocks\n",
> +		resp->header.sensor_block_num);
> +	for (b = 0; b < sensor_block_num; b++) {
> +		block = (struct sensor_data_block_header *)&data[offset];
> +		/* copy to a null terminated string */
> +		strncpy(sensor_type_string, block->sensor_type, 4);
> +		offset += 8;
> +
> +		dev_dbg(dev, "sensor block[%d]: type: %s, sensor_num: %d\n", b,
> +			sensor_type_string, block->sensor_num);
> +
> +		if (strncmp(block->sensor_type, "FREQ", 4) == 0)
> +			sensor_type = FREQ;
> +		else if (strncmp(block->sensor_type, "TEMP", 4) == 0)
> +			sensor_type = TEMP;
> +		else if (strncmp(block->sensor_type, "POWR", 4) == 0)
> +			sensor_type = POWER;
> +		else if (strncmp(block->sensor_type, "CAPS", 4) == 0)
> +			sensor_type = CAPS;
> +		else {
> +			dev_err(dev, "sensor type not supported %s\n",
> +				sensor_type_string);
> +			continue;
> +		}
> +
> +		rc = occ_check_sensor(driver, block->sensor_length,
> +				      block->sensor_num, sensor_type, b);
> +		if (rc == -ENOMEM)
> +			goto err;
> +		else if (rc)
> +			continue;
> +
> +		resp->data.sensor_block_id[sensor_type] = b;
> +		for (s = 0; s < block->sensor_num; s++) {
> +			driver->ops.parse_sensor(data,
> +						 resp->data.blocks[b].sensors,
> +						 sensor_type, offset, s);
> +			offset += block->sensor_length;
> +		}
> +
> +		/* copy block data over to response pointer */
> +		resp->data.blocks[b].header = *block;
> +	}
> +
> +	return 0;
> +err:
> +	deinit_occ_resp_buf(resp);
> +	return rc;
> +}
> +
> +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,
> +		       const u8 *data, u8 *resp)
> +{
> +	u32 cmd1, cmd2;
> +	u16 checksum = 0;
> +	u16 length_le = cpu_to_le16(length);
> +	bool retry = 0;
> +	int i, rc, tries = 0;
> +
> +	cmd1 = (seq << 24) | (type << 16) | length_le;
> +	memcpy(&cmd2, data, length);
> +	cmd2 <<= ((4 - length) * 8);
> +
> +	/* checksum: sum of every bytes of cmd1, cmd2 */
> +	for (i = 0; i < 4; i++) {
> +		checksum += (cmd1 >> (i * 8)) & 0xFF;
> +		checksum += (cmd2 >> (i * 8)) & 0xFF;
> +	}
> +
> +	cmd2 |= checksum << ((2 - length) * 8);
> +
> +	/* Init OCB */
> +	rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
> +				     OCB_OR_INIT0, OCB_OR_INIT1);
> +	if (rc)
> +		goto err;
> +
> +	rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_AND,
> +				     OCB_AND_INIT0, OCB_AND_INIT1);
> +	if (rc)
> +		goto err;
> +
> +	/* Send command, 2nd half of the 64-bit addr is unused (write 0) */
> +	rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
> +				     driver->config.command_addr, 0);
> +	if (rc)
> +		goto err;
> +
> +	rc = driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1, cmd2);
> +	if (rc)
> +		goto err;
> +
> +	/* Trigger attention */
> +	rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA, ATTN0, ATTN1);
> +	if (rc)
> +		goto err;
> +
> +	/* Get response data */
> +	rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
> +				     driver->config.response_addr, 0);
> +	if (rc)
> +		goto err;
> +
> +	do {
> +		if (retry) {
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
> +		}
> +
Isn't there a better way to do this ? What exactly is interrupted ? The
getscom function ? If so, shouldn't the timeout be handled there ?

> +		rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
> +					     (u64 *)resp);
> +		if (rc)
> +			goto err;
> +
> +		/* retry if we get "command in progress" return status */
> +		retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) &&
> +			(tries++ < CMD_IN_PRG_RETRIES);

There are some unnecessary () in this expression.

> +	} while (retry);
> +
> +	switch (resp[RESP_RETURN_STATUS]) {
> +	case RESP_RETURN_CMD_IN_PRG:
> +		rc = -EALREADY;
> +		break;
> +	case RESP_RETURN_SUCCESS:
> +		rc = 0;
> +		break;
> +	case RESP_RETURN_CMD_INVAL:
> +	case RESP_RETURN_CMD_LEN:
> +	case RESP_RETURN_DATA_INVAL:
> +	case RESP_RETURN_CHKSUM:
> +		rc = -EINVAL;
> +		break;
> +	case RESP_RETURN_OCC_ERR:
> +		rc = -EREMOTE;
> +		break;
> +	default:
> +		rc = -EFAULT;
> +	}
> +
> +	return rc;
> +
> +err:
> +	dev_err(driver->dev, "scom op failed rc:%d\n", rc);

Some of the errors result in an error message, some don't. What is
the guiding principle ? Even if there is an error message, it doesn't report
what function caused it, which makes it as good as no error message.
Can this be dropped ?

> +	return rc;
> +}
> +
> +static int occ_get_all(struct occ *driver)
> +{
> +	int i = 0, rc;
> +	u8 *occ_data;
> +	u16 num_bytes;
> +	const u8 poll_cmd_data = OCC_POLL_STAT_SENSOR;
> +	struct device *dev = driver->dev;
> +	struct occ_response *resp = &driver->response;
> +
> +	occ_data = devm_kzalloc(dev, OCC_DATA_MAX, GFP_KERNEL);

I am a bot lost here. What is the purpose of using a devm function ?

Either case, you might want to allocate the memory once and just keep it allocated.
Allocating a 4k buffer and freeing it each time data is read from the occ seems
to be wasteful and not really worth it.

> +	if (!occ_data)
> +		return -ENOMEM;
> +
> +	rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data, occ_data);
> +	if (rc) {
> +		dev_err(dev, "OCC poll failed: %d\n", rc);
> +		goto out;
> +	}
> +
> +	num_bytes = get_unaligned((u16 *)&occ_data[RESP_DATA_LENGTH]);
> +	num_bytes = be16_to_cpu(num_bytes);

What data is in big endian, and what data is in little endian ?

Looks like everything is big endian except for the length in the send command.
Is that correct ? If so, an explanation would be useful, because the logic
isn't entirely clear.

> +	dev_dbg(dev, "OCC data length: %d\n", num_bytes);
> +
> +	if (num_bytes > OCC_DATA_MAX) {
> +		dev_err(dev, "OCC data length must be < 4KB\n");
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (num_bytes <= 0) {

This can never be < 0.

> +		dev_err(dev, "OCC data length is zero\n");
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* read remaining data */
> +	for (i = 8; i < num_bytes + 8; i += 8) {
> +		rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
> +					     (u64 *)&occ_data[i]);
> +		if (rc) {
> +			dev_err(dev, "scom op failed rc:%d\n", rc);
> +			goto out;
> +		}
> +	}
> +
> +	/* don't need more sanity checks; buffer is alloc'd for max response
> +	 * size so we just check for valid data in parse_occ_response
> +	 */
Yes, but as mentioned above you don't check in that function if the data
was actually received.

> +	rc = parse_occ_response(driver, occ_data, resp);
> +
> +out:
> +	devm_kfree(dev, occ_data);
> +	return rc;
> +}
> +
> +int occ_update_device(struct occ *driver)
> +{
> +	int rc = 0;
> +
> +	mutex_lock(&driver->update_lock);
> +
> +	if (time_after(jiffies, driver->last_updated + driver->update_interval)
> +	    || !driver->valid) {
> +		driver->valid = 1;

valid is bool.

> +
> +		rc = occ_get_all(driver);
> +		if (rc)
> +			driver->valid = 0;

valid is bool.

> +
> +		driver->last_updated = jiffies;
> +	}
> +
> +	mutex_unlock(&driver->update_lock);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(occ_update_device);
> +
> +void *occ_get_sensor(struct occ *driver, int sensor_type)
> +{
> +	int rc;
> +
> +	/* occ_update_device locks the update lock */
> +	rc = occ_update_device(driver);
> +	if (rc) {
> +		dev_err(driver->dev, "cannot get occ sensor data: %d\n",
> +			rc);

Have you counted the number of error messages reported if anything goes wrong.
This driver is quite noisy. I won't force you to drop the messages, but it seems
to me that if anything goes wrong, the kernel log will be clogged with messages
just from this driver.

> +		return NULL;
> +	}
> +
> +	return occ_get_sensor_by_type(&driver->response, sensor_type);
> +}
> +EXPORT_SYMBOL(occ_get_sensor);
> +
> +int occ_get_sensor_value(struct occ *occ, int sensor_type, int sensor_num,
> +			 u32 hwmon, long *val)
> +{
> +	return occ->ops.get_sensor(occ, sensor_type, sensor_num, hwmon, val);
> +}
> +EXPORT_SYMBOL(occ_get_sensor_value);
> +
> +void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks)
> +{
> +	*blocks = &occ->response.data;
> +}
> +EXPORT_SYMBOL(occ_get_response_blocks);
> +
> +void occ_set_update_interval(struct occ *occ, unsigned long interval)
> +{
> +	occ->update_interval = msecs_to_jiffies(interval);
> +}
> +EXPORT_SYMBOL(occ_set_update_interval);
> +
> +int occ_set_user_powercap(struct occ *occ, u16 cap)
> +{
> +	u8 resp[8];
> +
> +	cap = cpu_to_be16(cap);
> +
> +	return occ_send_cmd(occ, 0, OCC_SET_USER_POWR_CAP, 2, (const u8 *)&cap,
> +			    resp);
> +}
> +EXPORT_SYMBOL(occ_set_user_powercap);
> +
> +struct occ *occ_start(struct device *dev, void *bus,
> +		      struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
> +		      const struct occ_config *config)
> +{
> +	struct occ *driver = devm_kzalloc(dev, sizeof(struct occ), GFP_KERNEL);
> +
> +	if (!driver)
> +		return ERR_PTR(-ENOMEM);
> +
> +	driver->dev = dev;
> +	driver->bus = bus;
> +	driver->bus_ops = *bus_ops;
> +	driver->ops = *ops;
> +	driver->config = *config;
> +
> +	driver->update_interval = HZ;

The update interval is only supposed to be configurable if it can be written
into the hardware. Otherwise it is really up to user space to decide how often
it wants to poll the data, not up to the kernel.

I don't see a function to send the update interval to the occ. As such, it should
not be configurable but a constant.

> +	mutex_init(&driver->update_lock);
> +
> +	return driver;
> +}
> +EXPORT_SYMBOL(occ_start);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("OCC hwmon core driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ.h b/drivers/hwmon/occ/occ.h
> new file mode 100644
> index 0000000..c86b06a
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ.h
> @@ -0,0 +1,81 @@
> +/*
> + * occ.h - hwmon OCC driver
> + *
> + * This file contains data structures and function prototypes for common access
> + * between different bus protocols and host systems.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#ifndef __OCC_H__
> +#define __OCC_H__
> +
> +#include "scom.h"
> +
> +struct device;
> +struct occ;
> +
> +/* sensor_data_block_header
> + * structure to match the raw occ sensor block header
> + */
> +struct sensor_data_block_header {
> +	u8 sensor_type[4];
> +	u8 reserved0;
> +	u8 sensor_format;
> +	u8 sensor_length;
> +	u8 sensor_num;
> +} __attribute__((packed, aligned(4)));
> +
> +struct sensor_data_block {
> +	struct sensor_data_block_header header;
> +	void *sensors;
> +};
> +
> +enum sensor_type {
> +	FREQ = 0,
> +	TEMP,
> +	POWER,
> +	CAPS,
> +	MAX_OCC_SENSOR_TYPE
> +};
> +
> +struct occ_ops {
> +	void (*parse_sensor)(u8 *data, void *sensor, int sensor_type, int off,
> +			     int snum);
> +	void *(*alloc_sensor)(int sensor_type, int num_sensors);
> +	int (*get_sensor)(struct occ *driver, int sensor_type, int sensor_num,
> +			  u32 hwmon, long *val);
> +};
> +
> +struct occ_config {
> +	u32 command_addr;
> +	u32 response_addr;
> +};
> +
> +struct occ_blocks {
> +	int sensor_block_id[MAX_OCC_SENSOR_TYPE];
> +	struct sensor_data_block *blocks;
> +};
> +
> +struct occ *occ_start(struct device *dev, void *bus,
> +		      struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
> +		      const struct occ_config *config);
> +void *occ_get_sensor(struct occ *occ, int sensor_type);
> +int occ_get_sensor_value(struct occ *occ, int sensor_type, int sensor_num,
> +			 u32 hwmon, long *val);
> +void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks);
> +int occ_update_device(struct occ *driver);
> +void occ_set_update_interval(struct occ *occ, unsigned long interval);
> +int occ_set_user_powercap(struct occ *occ, u16 cap);
> +
> +#endif /* __OCC_H__ */
> diff --git a/drivers/hwmon/occ/scom.h b/drivers/hwmon/occ/scom.h
> new file mode 100644
> index 0000000..c1da645
> --- /dev/null
> +++ b/drivers/hwmon/occ/scom.h
> @@ -0,0 +1,47 @@
> +/*
> + * scom.h - hwmon OCC driver
> + *
> + * This file contains data structures for scom operations to the OCC
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#ifndef __SCOM_H__
> +#define __SCOM_H__
> +
> +/*
> + * occ_bus_ops - represent the low-level transfer methods to communicate with
> + * the OCC.
> + *
> + * getscom - OCC scom read
> + * @bus: handle to slave device
> + * @address: address
> + * @data: where to store data read from slave; buffer size must be at least
> + * eight bytes.
> + *
> + * Returns 0 on success or a negative errno on error
> + *
> + * putscom - OCC scom write
> + * @bus: handle to slave device
> + * @address: address
> + * @data0: first data byte to write
> + * @data1: second data byte to write

32 bit bytes ? It might also be worthwhile to mention the expected byte order.

> + *
> + * Returns 0 on success or a negative errno on error
> + */
> +struct occ_bus_ops {
> +	int (*getscom)(void *bus, u32 address, u64 *data);
> +	int (*putscom)(void *bus, u32 address, u32 data0, u32 data1);
> +};
> +
> +#endif /* __SCOM_H__ */
>

Re: [PATCH linux v3 2/6] hwmon: occ: Add sysfs interface

[Guenter Roeck]

On 01/16/2017 01:13 PM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add a generic mechanism to expose the sensors provided by the OCC in
> sysfs.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/hwmon/occ       |  62 ++++++++++
>  drivers/hwmon/occ/Makefile    |   2 +-
>  drivers/hwmon/occ/occ_sysfs.c | 271 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ_sysfs.h |  44 +++++++
>  4 files changed, 378 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hwmon/occ/occ_sysfs.c
>  create mode 100644 drivers/hwmon/occ/occ_sysfs.h
>
> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> index 79d1642..d0bdf06 100644
> --- a/Documentation/hwmon/occ
> +++ b/Documentation/hwmon/occ
> @@ -25,6 +25,68 @@ Currently, all versions of the OCC support four types of sensor data: power,
>  temperature, frequency, and "caps," which indicate limits and thresholds used
>  internally on the OCC.
>
> +sysfs Entries
> +-------------
> +
> +The OCC driver uses the hwmon sysfs framework to provide data to userspace.
> +
> +The driver exports a number of sysfs files for each type of sensor. The
> +sensor-specific files vary depending on the processor type, though many of the
> +attributes are common for both the POWER8 and POWER9.
> +
> +The hwmon interface cannot define every type of sensor that may be used.
> +Therefore, the frequency sensor on the OCC uses the "input" type sensor defined
> +by the hwmon interface, rather than defining a new type of custom sensor.
> +
> +Below are detailed the names and meaning of each sensor file for both types of
> +processors. All sensors are read-only unless otherwise specified. <x> indicates
> +the hwmon index. sensor id indicates the unique internal OCC identifer. Please
> +see the POWER OCC specification for details on all these sensor values.
> +
> +frequency:
> +	all processors:
> +		in<x>_input - frequency value
> +		in<x>_label - sensor id
> +temperature:
> +	POWER8:
> +		temp<x>_input - temperature value
> +		temp<x>_label - sensor id
> +	POWER9 (in addition to above):
> +		temp<x>_type - FRU type
> +
> +power:
> +	POWER8:
> +		power<x>_input - power value
> +		power<x>_label - sensor id
> +		power<x>_average - accumulator
> +		power<x>_average_interval - update tag (number of samples in
> +			accumulator)
> +	POWER9:
> +		power<x>_input - power value
> +		power<x>_label - sensor id
> +		power<x>_average_min - accumulator[0]
> +		power<x>_average_max - accumulator[1] (64 bits total)
> +		power<x>_average_interval - update tag
> +		power<x>_reset_history - (function_id | (apss_channel << 8)
> +
> +caps:
> +	POWER8:
> +		power<x>_cap - current powercap
> +		power<x>_cap_max - max powercap
> +		power<x>_cap_min - min powercap
> +		power<x>_max - normal powercap
> +		power<x>_alarm - user powercap, r/w
> +	POWER9:
> +		power<x>_cap_alarm - user powercap source
> +
> +The driver also provides two sysfs entries through hwmon to better
> +control the driver and monitor the master OCC. Though there may be multiple
> +OCCs present on the system, these two files are only present for the "master"
> +OCC.
> +	name - read the name of the driver
> +	update_interval - read or write the minimum interval for polling the
> +		OCC.
> +
>  BMC - Host Communications
>  -------------------------
>
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> index 93cb52f..a6881f9 100644
> --- a/drivers/hwmon/occ/Makefile
> +++ b/drivers/hwmon/occ/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
> new file mode 100644
> index 0000000..2f20c40
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.c
> @@ -0,0 +1,271 @@
> +/*
> + * occ_sysfs.c - OCC sysfs interface
> + *
> + * This file contains the methods and data structures for implementing the OCC
> + * hwmon sysfs entries.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include "occ.h"
> +#include "occ_sysfs.h"
> +
> +#define RESP_RETURN_CMD_INVAL	0x13
> +
> +static int occ_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long *val)
> +{
> +	int rc = 0;

Unnecessary initialization.

> +	struct occ_sysfs *driver = dev_get_drvdata(dev);
> +	struct occ *occ = driver->occ;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		rc = occ_get_sensor_value(occ, FREQ, channel, attr, val);
> +		break;
> +	case hwmon_temp:
> +		rc = occ_get_sensor_value(occ, TEMP, channel, attr, val);
> +		break;
> +	case hwmon_power:
> +		rc = occ_get_sensor_value(occ, POWER, channel, attr, val);
> +		break;
> +	default:
> +		rc = -EOPNOTSUPP;
> +	}
> +
> +	return rc;
> +}
> +
> +static int occ_hwmon_read_string(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, char **str)
> +{
> +	int rc;
> +	unsigned long val = 0;
> +
> +	if (!((type == hwmon_in && attr == hwmon_in_label) ||
> +	    (type == hwmon_temp && attr == hwmon_temp_label) ||
> +	    (type == hwmon_power && attr == hwmon_power_label)))
> +		return -EOPNOTSUPP;
> +
> +	rc = occ_hwmon_read(dev, type, attr, channel, &val);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = snprintf(*str, PAGE_SIZE - 1, "%ld", val);

val is unsigned long.

I am quite confused what this is for, though. The function is used for string attributes,
or in other words for labels. What is the benefit of having a label that returns the same
data as the value attribute ? Is this maybe a misunderstanding ?

> +	if (rc > 0)
> +		rc = 0;
> +
> +	return rc;
> +}
> +
> +static int occ_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long val)
> +{
> +	int rc = 0;
> +	struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> +		occ_set_update_interval(driver->occ, val);

As mentioned in the other patch, please drop. This is not the intended use case
for this attribute.

> +		return 0;
> +	} else if (type == hwmon_power && attr == hwmon_power_alarm) {
> +		rc = occ_set_user_powercap(driver->occ, val);
> +		if (rc) {
> +			if (rc == RESP_RETURN_CMD_INVAL) {
> +				dev_err(dev,
> +					"set invalid powercap value: %ld\n",
> +					val);
> +				return -EINVAL;
> +			}
> +
> +			dev_err(dev, "set user powercap failed: 0x:%x\n", rc);
> +			return rc;
> +		}
> +
> +		driver->user_powercap = val;
> +
> +		return rc;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t occ_is_visible(const void *data, enum hwmon_sensor_types type,
> +			      u32 attr, int channel)
> +{
> +	const struct occ_sysfs *driver = data;
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		if (attr == hwmon_chip_update_interval)
> +			return S_IRUGO | S_IWUSR;
> +		break;
> +	case hwmon_in:
> +		if (BIT(attr) & driver->sensor_hwmon_configs[0])
> +			return S_IRUGO;
> +		break;
> +	case hwmon_temp:
> +		if (BIT(attr) & driver->sensor_hwmon_configs[1])
> +			return S_IRUGO;
> +		break;
> +	case hwmon_power:
> +		/* user power limit */
> +		if (attr == hwmon_power_alarm)
> +			return S_IRUGO | S_IWUSR;
> +		else if ((BIT(attr) & driver->sensor_hwmon_configs[2]) ||
> +			 (BIT(attr) & driver->sensor_hwmon_configs[3]))
> +			return S_IRUGO;
> +		break;
> +	default:
> +		return 0;

Might as well use break; here for consistency.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_ops occ_hwmon_ops = {
> +	.is_visible = occ_is_visible,
> +	.read = occ_hwmon_read,
> +	.read_string = occ_hwmon_read_string,
> +	.write = occ_hwmon_write,
> +};
> +
> +static const u32 occ_chip_config[] = {
> +	HWMON_C_UPDATE_INTERVAL,
> +	0
> +};
> +
> +static const struct hwmon_channel_info occ_chip = {
> +	.type = hwmon_chip,
> +	.config = occ_chip_config
> +};
> +
> +static const enum hwmon_sensor_types occ_sensor_types[MAX_OCC_SENSOR_TYPE] = {
> +	hwmon_in,
> +	hwmon_temp,
> +	hwmon_power,
> +	hwmon_power
> +};
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> +				  const u32 *sensor_hwmon_configs,
> +				  const char *name)
> +{
> +	bool master_occ = false;
> +	int rc, i, j, sensor_num, index = 0, id;
> +	char *brk;
> +	struct occ_blocks *resp = NULL;
> +	u32 *sensor_config;
> +	struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
> +					       GFP_KERNEL);
> +	if (!hwmon)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* need space for null-termination and occ chip */
> +	hwmon->occ_sensors =
> +		devm_kzalloc(dev, sizeof(struct hwmon_channel_info *) *
> +			     (MAX_OCC_SENSOR_TYPE + 2), GFP_KERNEL);
> +	if (!hwmon->occ_sensors)
> +		return ERR_PTR(-ENOMEM);
> +
> +	hwmon->occ = occ;
> +	hwmon->sensor_hwmon_configs = (u32 *)sensor_hwmon_configs;

Either this is a const or it isn't. If it isn't, it should not be passed in as const.
If it is, it can be declared const in struct occ_sysfs.


> +	hwmon->occ_info.ops = &occ_hwmon_ops;
> +	hwmon->occ_info.info =
> +		(const struct hwmon_channel_info **)hwmon->occ_sensors;

Why is this typecast needed ?

> +
> +	occ_get_response_blocks(occ, &resp);
> +
> +	for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
> +		resp->sensor_block_id[i] = -1;
> +
> +	/* read sensor data from occ */
> +	rc = occ_update_device(occ);
> +	if (rc) {
> +		dev_err(dev, "cannot get occ sensor data: %d\n", rc);
> +		return ERR_PTR(rc);
> +	}
> +	if (!resp->blocks)
> +		return ERR_PTR(-ENOMEM);
> +
> +	master_occ = resp->sensor_block_id[CAPS] >= 0;
> +
> +	for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
> +		id = resp->sensor_block_id[i];
> +		if (id < 0)
> +			continue;
> +
> +		sensor_num = resp->blocks[id].header.sensor_num;
> +		/* need null-termination */
> +		sensor_config = devm_kzalloc(dev,
> +					     sizeof(u32) * (sensor_num + 1),
> +					     GFP_KERNEL);
> +		if (!sensor_config)
> +			return ERR_PTR(-ENOMEM);
> +
> +		for (j = 0; j < sensor_num; j++)
> +			sensor_config[j] = sensor_hwmon_configs[i];
> +
> +		hwmon->occ_sensors[index] =
> +			devm_kzalloc(dev, sizeof(struct hwmon_channel_info),
> +				     GFP_KERNEL);
> +		if (!hwmon->occ_sensors[index])
> +			return ERR_PTR(-ENOMEM);
> +
> +		hwmon->occ_sensors[index]->type = occ_sensor_types[i];
> +		hwmon->occ_sensors[index]->config = sensor_config;
> +		index++;
> +	}

I had the impression from patch 1 that the number of sensors can change at runtime.
If so, this doesn't really work well; the sysfs attributes won't be updated in this
case.

Can it really happen that the sensor information can change ?

> +
> +	/* only need one of these for any number of occs */
> +	if (master_occ)
> +		hwmon->occ_sensors[index] =
> +			(struct hwmon_channel_info *)&occ_chip;

Why this typecast ?

> +
> +	/* search for bad chars */
> +	strncpy(hwmon->hwmon_name, name, OCC_HWMON_NAME_LENGTH);
> +	brk = strpbrk(hwmon->hwmon_name, "-* \t\n");
> +	while (brk) {
> +		*brk = '_';
> +		brk = strpbrk(brk,  "-* \t\n");
> +	}
> +
> +	hwmon->dev = devm_hwmon_device_register_with_info(dev,
> +							  hwmon->hwmon_name,
> +							  hwmon,
> +							  &hwmon->occ_info,
> +							  NULL);
> +	if (IS_ERR(hwmon->dev)) {
> +		dev_err(dev, "cannot register hwmon device %s: %ld\n",
> +			hwmon->hwmon_name, PTR_ERR(hwmon->dev));
> +		return ERR_CAST(hwmon->dev);
> +	}
> +
> +	return hwmon;
> +}
> +EXPORT_SYMBOL(occ_sysfs_start);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("OCC sysfs driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_sysfs.h b/drivers/hwmon/occ/occ_sysfs.h
> new file mode 100644
> index 0000000..7de92e7
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.h
> @@ -0,0 +1,44 @@
> +/*
> + * occ_sysfs.h - OCC sysfs interface
> + *
> + * This file contains the data structures and function prototypes for the OCC
> + * hwmon sysfs entries.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#ifndef __OCC_SYSFS_H__
> +#define __OCC_SYSFS_H__
> +
> +#include <linux/hwmon.h>
> +
> +struct occ;
> +struct device;
> +
> +#define OCC_HWMON_NAME_LENGTH	32
> +
> +struct occ_sysfs {
> +	struct device *dev;
> +	struct occ *occ;
> +
> +	char hwmon_name[OCC_HWMON_NAME_LENGTH + 1];
> +	u32 *sensor_hwmon_configs;
> +	struct hwmon_channel_info **occ_sensors;
> +	struct hwmon_chip_info occ_info;
> +	u16 user_powercap;
> +};

Is this information used outside the sysfs code ? If not, it should be defined
in the source file and not be available outside of it.

> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> +				  const u32 *sensor_hwmon_configs,
> +				  const char *name);
> +#endif /* __OCC_SYSFS_H__ */
>

Re: [PATCH linux v3 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations

[Guenter Roeck]

On 01/16/2017 01:13 PM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add functions to send SCOM operations over I2C bus. The BMC can
> communicate with the Power8 host processor over I2C, but needs to use SCOM
> operations in order to access the OCC register space.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/hwmon/occ/occ_scom_i2c.c | 72 ++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ_scom_i2c.h | 26 +++++++++++++++
>  2 files changed, 98 insertions(+)
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
>
> diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
> new file mode 100644
> index 0000000..8b4ca13
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_scom_i2c.c
> @@ -0,0 +1,72 @@
> +/*
> + * occ_scom_i2c.c - hwmon OCC driver
> + *
> + * This file contains the functions for performing SCOM operations over I2C bus
> + * to access the OCC.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "occ_scom_i2c.h"
> +#include "scom.h"
> +
> +int occ_i2c_getscom(void *bus, u32 address, u64 *data)
> +{
> +	ssize_t rc;
> +	u64 buf;
> +	struct i2c_client *client = bus;
> +
> +	rc = i2c_master_send(client, (const char *)&address, sizeof(u32));
> +	if (rc < 0)
> +		return rc;
> +	else if (rc != sizeof(u32))
> +		return -EIO;
> +
> +	rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
> +	if (rc < 0)
> +		return rc;
> +	else if (rc != sizeof(u64))
> +		return -EIO;
> +
> +	*data = be64_to_cpu(buf);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(occ_i2c_getscom);
> +
> +int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
> +{
> +	u32 buf[3];
> +	ssize_t rc;
> +	struct i2c_client *client = bus;
> +
> +	buf[0] = address;
> +	buf[1] = data1;
> +	buf[2] = data0;
> +

Receive data is converted from be64 to host byte order, transmit data is sent as-is.
Yet, if I recall correctly, some of the calling code passes in constants. Are you sure
this works on both little and big endian systems ?

> +	rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
> +	if (rc < 0)
> +		return rc;
> +	else if (rc != sizeof(u32) * 3)
> +		return -EIO;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(occ_i2c_putscom);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("I2C OCC SCOM transport");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_scom_i2c.h b/drivers/hwmon/occ/occ_scom_i2c.h
> new file mode 100644
> index 0000000..945739c
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_scom_i2c.h
> @@ -0,0 +1,26 @@
> +/*
> + * occ_scom_i2c.h - hwmon OCC driver
> + *
> + * This file contains function protoypes for peforming SCOM operations over I2C
> + * bus to access the OCC.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#ifndef __OCC_SCOM_I2C_H__
> +#define __OCC_SCOM_I2C_H__
> +
> +int occ_i2c_getscom(void *bus, u32 address, u64 *data);
> +int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
> +
> +#endif /* __OCC_SCOM_I2C_H__ */
>

Re: [PATCH linux v3 4/6] hwmon: occ: Add callbacks for parsing P8 OCC datastructures

[Guenter Roeck]

On 01/16/2017 01:13 PM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add functions to parse the data structures that are specific to the OCC on
> the POWER8 processor. These are the sensor data structures, including
> temperature, frequency, power, and "caps."
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/hwmon/occ    |   9 ++
>  drivers/hwmon/occ/occ_p8.c | 247 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ_p8.h |  30 ++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 drivers/hwmon/occ/occ_p8.c
>  create mode 100644 drivers/hwmon/occ/occ_p8.h
>
> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> index d0bdf06..143951e 100644
> --- a/Documentation/hwmon/occ
> +++ b/Documentation/hwmon/occ
> @@ -25,6 +25,15 @@ Currently, all versions of the OCC support four types of sensor data: power,
>  temperature, frequency, and "caps," which indicate limits and thresholds used
>  internally on the OCC.
>
> +The format for the POWER8 OCC sensor data can be found in the P8 OCC
> +specification:
> +github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf
> +This document provides the details of the OCC sensors: power, frequency,
> +temperature, and caps. These sensor formats are specific to the POWER8 OCC. A
> +number of data structures, such as command format, response headers, and the
> +like, are also defined in this specification, and are common to both POWER8 and
> +POWER9 OCCs.
> +
>  sysfs Entries
>  -------------
>
> diff --git a/drivers/hwmon/occ/occ_p8.c b/drivers/hwmon/occ/occ_p8.c
> new file mode 100644
> index 0000000..32215ed
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_p8.c
> @@ -0,0 +1,247 @@
> +/*
> + * occ_p8.c - OCC hwmon driver
> + *
> + * This file contains the Power8-specific methods and data structures for
> + * the OCC hwmon driver.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include "occ.h"
> +#include "occ_p8.h"
> +
> +/* P8 OCC sensor data format */
> +struct p8_occ_sensor {
> +	u16 sensor_id;
> +	u16 value;
> +};
> +
> +struct p8_power_sensor {
> +	u16 sensor_id;
> +	u32 update_tag;
> +	u32 accumulator;
> +	u16 value;
> +};
> +
> +struct p8_caps_sensor {
> +	u16 curr_powercap;
> +	u16 curr_powerreading;
> +	u16 norm_powercap;
> +	u16 max_powercap;
> +	u16 min_powercap;
> +	u16 user_powerlimit;
> +};
> +
> +static const u32 p8_sensor_hwmon_configs[MAX_OCC_SENSOR_TYPE] = {
> +	HWMON_I_INPUT | HWMON_I_LABEL,	/* freq: value | label */
> +	HWMON_T_INPUT | HWMON_T_LABEL,	/* temp: value | label */
> +	/* power: value | label | accumulator | update_tag */
> +	HWMON_P_INPUT | HWMON_P_LABEL | HWMON_P_AVERAGE |
> +		HWMON_P_AVERAGE_INTERVAL,
> +	/* caps: curr | max | min | norm | user */
> +	HWMON_P_CAP | HWMON_P_CAP_MAX | HWMON_P_CAP_MIN | HWMON_P_MAX |
> +		HWMON_P_ALARM,
> +};
> +
> +void p8_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
> +		     int snum)
> +{
> +	switch (sensor_type) {
> +	case FREQ:
> +	case TEMP:
> +	{
> +		struct p8_occ_sensor *os =
> +			&(((struct p8_occ_sensor *)sensor)[snum]);
> +
> +		os->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
> +		os->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
> +	}
> +		break;
> +	case POWER:
> +	{
> +		struct p8_power_sensor *ps =
> +			&(((struct p8_power_sensor *)sensor)[snum]);
> +
> +		ps->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
> +		ps->update_tag =
> +			be32_to_cpu(get_unaligned((u32 *)&data[off + 2]));
> +		ps->accumulator =
> +			be32_to_cpu(get_unaligned((u32 *)&data[off + 6]));
> +		ps->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
> +	}
> +		break;
> +	case CAPS:
> +	{
> +		struct p8_caps_sensor *cs =
> +			&(((struct p8_caps_sensor *)sensor)[snum]);
> +
> +		cs->curr_powercap =
> +			be16_to_cpu(get_unaligned((u16 *)&data[off]));
> +		cs->curr_powerreading =
> +			be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
> +		cs->norm_powercap =
> +			be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
> +		cs->max_powercap =
> +			be16_to_cpu(get_unaligned((u16 *)&data[off + 6]));
> +		cs->min_powercap =
> +			be16_to_cpu(get_unaligned((u16 *)&data[off + 8]));
> +		cs->user_powerlimit =
> +			be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
> +	}
> +		break;
> +	};
> +}
> +
> +void *p8_alloc_sensor(int sensor_type, int num_sensors)
> +{
> +	switch (sensor_type) {
> +	case FREQ:
> +	case TEMP:
> +		return kcalloc(num_sensors, sizeof(struct p8_occ_sensor),
> +			       GFP_KERNEL);
> +	case POWER:
> +		return kcalloc(num_sensors, sizeof(struct p8_power_sensor),
> +			       GFP_KERNEL);
> +	case CAPS:
> +		return kcalloc(num_sensors, sizeof(struct p8_caps_sensor),
> +			       GFP_KERNEL);
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +int p8_get_sensor(struct occ *driver, int sensor_type, int sensor_num,
> +		  u32 hwmon, long *val)
> +{
> +	int rc = 0;
> +	void *sensor;
> +
> +	if (sensor_type == POWER) {
> +		if (hwmon == hwmon_power_cap || hwmon == hwmon_power_cap_max ||
> +		    hwmon == hwmon_power_cap_min || hwmon == hwmon_power_max ||
> +		    hwmon == hwmon_power_alarm)
> +			sensor_type = CAPS;
> +	}
> +
> +	sensor = occ_get_sensor(driver, sensor_type);
> +	if (!sensor)
> +		return -ENODEV;
> +
> +	switch (sensor_type) {
> +	case FREQ:
> +	case TEMP:
> +	{
> +		struct p8_occ_sensor *os =
> +			&(((struct p8_occ_sensor *)sensor)[sensor_num]);
> +
> +		if (hwmon == hwmon_in_input || hwmon == hwmon_temp_input)
> +			*val = os->value;
> +		else if (hwmon == hwmon_in_label || hwmon == hwmon_temp_label)
> +			*val = os->sensor_id;

Guess that explains the numeric labels I wondered about earlier. Are you really sure
you want to have such numbered labels, ie temp1_label = "1" and so on ? Your call,
really, just wondering.

> +		else
> +			rc = -EOPNOTSUPP;
> +	}
> +		break;
> +	case POWER:
> +	{
> +		struct p8_power_sensor *ps =
> +			&(((struct p8_power_sensor *)sensor)[sensor_num]);
> +
> +		switch (hwmon) {
> +		case hwmon_power_input:
> +			*val = ps->value;
> +			break;
> +		case hwmon_power_label:
> +			*val = ps->sensor_id;
> +			break;
> +		case hwmon_power_average:
> +			*val = ps->accumulator;
> +			break;
> +		case hwmon_power_average_interval:
> +			*val = ps->update_tag;
> +			break;
> +		default:
> +			rc = -EOPNOTSUPP;
> +		}
> +	}
> +		break;
> +	case CAPS:
> +	{
> +		struct p8_caps_sensor *cs =
> +			&(((struct p8_caps_sensor *)sensor)[sensor_num]);
> +
> +		switch (hwmon) {
> +		case hwmon_power_cap:
> +			*val = cs->curr_powercap;
> +			break;
> +		case hwmon_power_cap_max:
> +			*val = cs->max_powercap;
> +			break;
> +		case hwmon_power_cap_min:
> +			*val = cs->min_powercap;
> +			break;
> +		case hwmon_power_max:
> +			*val = cs->norm_powercap;
> +			break;
> +		case hwmon_power_alarm:
> +			*val = cs->user_powerlimit;
> +			break;
> +		default:
> +			rc = -EOPNOTSUPP;
> +		}
> +	}
> +		break;
> +	default:
> +		rc = -EINVAL;
> +	}
> +
> +	return rc;
> +}
> +
> +static const struct occ_ops p8_ops = {
> +	.parse_sensor = p8_parse_sensor,
> +	.alloc_sensor = p8_alloc_sensor,
> +	.get_sensor = p8_get_sensor,
> +};
> +
> +static const struct occ_config p8_config = {
> +	.command_addr = 0xFFFF6000,
> +	.response_addr = 0xFFFF7000,
> +};
> +
> +const u32 *p8_get_sensor_hwmon_configs()
> +{
> +	return p8_sensor_hwmon_configs;
> +}
> +EXPORT_SYMBOL(p8_get_sensor_hwmon_configs);
> +
> +struct occ *p8_occ_start(struct device *dev, void *bus,
> +			 struct occ_bus_ops *bus_ops)
> +{
> +	return occ_start(dev, bus, bus_ops, &p8_ops, &p8_config);
> +}
> +EXPORT_SYMBOL(p8_occ_start);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("P8 OCC sensors");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_p8.h b/drivers/hwmon/occ/occ_p8.h
> new file mode 100644
> index 0000000..3fe6417
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_p8.h
> @@ -0,0 +1,30 @@
> +/*
> + * occ_p8.h - OCC hwmon driver
> + *
> + * This file contains Power8-specific function prototypes
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#ifndef __OCC_P8_H__
> +#define __OCC_P8_H__
> +
> +#include "scom.h"
> +
> +struct device;
> +
> +const u32 *p8_get_sensor_hwmon_configs(void);
> +struct occ *p8_occ_start(struct device *dev, void *bus,
> +			 struct occ_bus_ops *bus_ops);
> +
> +#endif /* __OCC_P8_H__ */
>

Re: [PATCH linux v3 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations

[Benjamin Herrenschmidt]

On Sat, 2017-01-21 at 10:11 -0800, Guenter Roeck wrote:
> > +int occ_i2c_getscom(void *bus, u32 address, u64 *data)
> > +{
> > +     ssize_t rc;
> > +     u64 buf;
> > +     struct i2c_client *client = bus;
> > +
> > +     rc = i2c_master_send(client, (const char *)&address,
> > sizeof(u32));
> > +     if (rc < 0)
> > +             return rc;
> > +     else if (rc != sizeof(u32))
> > +             return -EIO;
> > +
> > +     rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
> > +     if (rc < 0)
> > +             return rc;
> > +     else if (rc != sizeof(u64))
> > +             return -EIO;
> > +
> > +     *data = be64_to_cpu(buf);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(occ_i2c_getscom);

Additionally, this assumes that is is the only other user of that i2c
path to P8. Something interleaving will break it. pdbg for example.

Talk to Alistair, the right thing here would probably be to have
a separate driver that provides the XSCOM interface via i2c to both
in-kernel and userspace, with proper arbitration.

An expedient might be to instead have the address and data cycles
of the above be 2 i2c messages in one i2c request, thus being
serialized somewhat atomically with other transactions to that i2c
bus.

You may need to make sure the underlying i2c controller driver supports
dual messages, and if it doesn't fix it. This is rather classic 4-bytes 
subaddress style i2c, it shouldn't be too hard.

Cheers,
Ben.

Re: [PATCH linux v3 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs

[Guenter Roeck]

On Mon, Jan 23, 2017 at 01:28:52PM -0600, Edward James wrote:
> I still just can't get gmail to reply to this. Thanks for the review.
> 
> On Sat, Jan 21, 2017 at 11:49 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 01/16/2017 01:13 PM, eajames.ibm@gmail.com wrote:
> >>
> >> From: "Edward A. James" <eajames@us.ibm.com>
> >>
> >> Add core support for polling the OCC for it's sensor data and parsing
> that
> >> data into sensor-specific information.
> >>
> >> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> >> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> >> ---
> >>  Documentation/hwmon/occ    |  40 ++++
> >>  MAINTAINERS                |   7 +
> >>  drivers/hwmon/Kconfig      |   2 +
> >>  drivers/hwmon/Makefile     |   1 +
> >>  drivers/hwmon/occ/Kconfig  |  15 ++
> >>  drivers/hwmon/occ/Makefile |   1 +
> >>  drivers/hwmon/occ/occ.c    | 522
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >>  drivers/hwmon/occ/occ.h    |  81 +++++++
> >>  drivers/hwmon/occ/scom.h   |  47 ++++
> >>  9 files changed, 716 insertions(+)
> >>  create mode 100644 Documentation/hwmon/occ
> >>  create mode 100644 drivers/hwmon/occ/Kconfig
> >>  create mode 100644 drivers/hwmon/occ/Makefile
> >>  create mode 100644 drivers/hwmon/occ/occ.c
> >>  create mode 100644 drivers/hwmon/occ/occ.h
> >>  create mode 100644 drivers/hwmon/occ/scom.h
> >>
> >> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> >> new file mode 100644
> >> index 0000000..79d1642
> >> --- /dev/null
> >> +++ b/Documentation/hwmon/occ
> >> @@ -0,0 +1,40 @@
> >> +Kernel driver occ
> >> +=================
> >> +
> >> +Supported chips:
> >> + * ASPEED AST2400
> >> + * ASPEED AST2500
> >> +
> >> +Please note that the chip must be connected to a POWER8 or POWER9
> >> processor
> >> +(see the BMC - Host Communications section).
> >> +
> >> +Author: Eddie James <eajames@us.ibm.com>
> >> +
> >> +Description
> >> +-----------
> >> +
> >> +This driver implements support for the OCC (On-Chip Controller) on the
> >> IBM
> >> +POWER8 and POWER9 processors, from a BMC (Baseboard Management
> >> Controller). The
> >> +OCC is an embedded processor that provides real time power and thermal
> >> +monitoring.
> >> +
> >> +This driver provides an interface on a BMC to poll OCC sensor data, set
> >> user
> >> +power caps, and perform some basic OCC error handling.
> >> +
> >> +Currently, all versions of the OCC support four types of sensor data:
> >> power,
> >> +temperature, frequency, and "caps," which indicate limits and
> thresholds
> >> used
> >> +internally on the OCC.
> >> +
> >> +BMC - Host Communications
> >> +-------------------------
> >> +
> >> +For the POWER8 application, the BMC can communicate with the P8 over
> I2C
> >> bus.
> >> +However, to access the OCC register space, any data transfer must use a
> >> SCOM
> >> +operation. SCOM is a procedure to initiate a data transfer, typically
> of
> >> 8
> >> +bytes. SCOMs consist of writing a 32-bit command register and then
> >> +reading/writing two 32-bit data registers. This driver implements these
> >> +SCOM operations over I2C bus in order to communicate with the OCC.
> >> +
> >> +For the POWER9 application, the BMC can communicate with the P9 over
> FSI
> >> bus
> >> +and SBE engine. Once again, SCOM operations are required. This driver
> >> will
> >> +implement SCOM ops over FSI/SBE. This will require the FSI driver.
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 5f0420a..f5d4195 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -9112,6 +9112,13 @@ T:       git git://linuxtv.org/media_tree.git
> >>  S:     Maintained
> >>  F:     drivers/media/i2c/ov7670.c
> >>
> >> +ON-CHIP CONTROLLER HWMON DRIVER
> >> +M:     Eddie James <eajames@us.ibm.com>
> >> +L:     linux-hwmon@vger.kernel.org
> >> +S:     Maintained
> >> +F:     Documentation/hwmon/occ
> >> +F:     drivers/hwmon/occ/
> >> +
> >>  ONENAND FLASH DRIVER
> >>  M:     Kyungmin Park <kyungmin.park@samsung.com>
> >>  L:     linux-mtd@lists.infradead.org
> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >> index 190d270..e80ca81 100644
> >> --- a/drivers/hwmon/Kconfig
> >> +++ b/drivers/hwmon/Kconfig
> >> @@ -1240,6 +1240,8 @@ config SENSORS_NSA320
> >>           This driver can also be built as a module. If so, the module
> >>           will be called nsa320-hwmon.
> >>
> >> +source drivers/hwmon/occ/Kconfig
> >> +
> >>  config SENSORS_PCF8591
> >>         tristate "Philips PCF8591 ADC/DAC"
> >>         depends on I2C
> >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >> index d2cb7e8..c7ec5d4 100644
> >> --- a/drivers/hwmon/Makefile
> >> +++ b/drivers/hwmon/Makefile
> >> @@ -169,6 +169,7 @@ obj-$(CONFIG_SENSORS_WM831X)        +=
> wm831x-hwmon.o
> >>  obj-$(CONFIG_SENSORS_WM8350)   += wm8350-hwmon.o
> >>  obj-$(CONFIG_SENSORS_XGENE)    += xgene-hwmon.o
> >>
> >> +obj-$(CONFIG_SENSORS_PPC_OCC)  += occ/
> >>  obj-$(CONFIG_PMBUS)            += pmbus/
> >>
> >>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> >> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
> >> new file mode 100644
> >> index 0000000..cdb64a7
> >> --- /dev/null
> >> +++ b/drivers/hwmon/occ/Kconfig
> >> @@ -0,0 +1,15 @@
> >> +#
> >> +# On Chip Controller configuration
> >> +#
> >> +
> >> +menuconfig SENSORS_PPC_OCC
> >> +       bool "PPC On-Chip Controller"
> >> +       help
> >> +         If you say yes here you get support to monitor Power CPU
> >> +         sensors via the On-Chip Controller (OCC).
> >> +
> >> +         Generally this is used by management controllers such as a BMC
> >> +         on an OpenPower system.
> >> +
> >> +         This driver can also be built as a module. If so, the module
> >> +         will be called occ.
> >> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> >> new file mode 100644
> >> index 0000000..93cb52f
> >> --- /dev/null
> >> +++ b/drivers/hwmon/occ/Makefile
> >> @@ -0,0 +1 @@
> >> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
> >> diff --git a/drivers/hwmon/occ/occ.c b/drivers/hwmon/occ/occ.c
> >> new file mode 100644
> >> index 0000000..3089762
> >> --- /dev/null
> >> +++ b/drivers/hwmon/occ/occ.c
> >> @@ -0,0 +1,522 @@
> >> +/*
> >> + * occ.c - OCC hwmon driver
> >> + *
> >> + * This file contains the methods and data structures for the OCC hwmon
> >> driver.
> >> + *
> >> + * Copyright 2016 IBM Corp.
> >> + *
> >> + * 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.
> >> + */
> >> +
> >> +#include <asm/unaligned.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >> +#include <linux/init.h>
> >> +#include <linux/jiffies.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/slab.h>
> >> +#include "occ.h"
> >> +
> >> +#define OCC_DATA_MAX           4096
> >> +#define OCC_BMC_TIMEOUT_MS     20000
> >> +
> >> +/* To generate attn to OCC */
> >> +#define ATTN_DATA              0x0006B035
> >> +
> >> +/* For BMC to read/write SRAM */
> >> +#define OCB_ADDRESS            0x0006B070
> >> +#define OCB_DATA               0x0006B075
> >> +#define OCB_STATUS_CONTROL_AND 0x0006B072
> >> +#define OCB_STATUS_CONTROL_OR  0x0006B073
> >> +
> >> +/* To init OCB */
> >> +#define OCB_AND_INIT0          0xFBFFFFFF
> >> +#define OCB_AND_INIT1          0xFFFFFFFF
> >> +#define OCB_OR_INIT0           0x08000000
> >> +#define OCB_OR_INIT1           0x00000000
> >> +
> >> +/* To generate attention on OCC */
> >> +#define ATTN0                  0x01010000
> >> +#define ATTN1                  0x00000000
> >> +
> >> +/* OCC return status */
> >> +#define RESP_RETURN_CMD_IN_PRG 0xFF
> >> +#define RESP_RETURN_SUCCESS    0
> >> +#define RESP_RETURN_CMD_INVAL  0x11
> >> +#define RESP_RETURN_CMD_LEN    0x12
> >> +#define RESP_RETURN_DATA_INVAL 0x13
> >> +#define RESP_RETURN_CHKSUM     0x14
> >> +#define RESP_RETURN_OCC_ERR    0x15
> >> +#define RESP_RETURN_STATE      0x16
> >> +
> >> +/* time interval to retry on "command in progress" return status */
> >> +#define CMD_IN_PRG_INT_MS      100
> >> +#define CMD_IN_PRG_RETRIES     (OCC_BMC_TIMEOUT_MS / CMD_IN_PRG_INT_MS)
> >> +
> >> +/* OCC command definitions */
> >> +#define OCC_POLL               0
> >> +#define OCC_SET_USER_POWR_CAP  0x22
> >> +
> >> +/* OCC poll command data */
> >> +#define OCC_POLL_STAT_SENSOR   0x10
> >> +
> >> +/* OCC response data offsets */
> >> +#define RESP_RETURN_STATUS     2
> >> +#define RESP_DATA_LENGTH       3
> >> +#define RESP_HEADER_OFFSET     5
> >> +#define SENSOR_STR_OFFSET      37
> >> +#define SENSOR_BLOCK_NUM_OFFSET        43
> >> +#define SENSOR_BLOCK_OFFSET    45
> >> +
> >> +/* occ_poll_header
> >> + * structure to match the raw occ poll response data
> >> + */
> >> +struct occ_poll_header {
> >> +       u8 status;
> >> +       u8 ext_status;
> >> +       u8 occs_present;
> >> +       u8 config;
> >> +       u8 occ_state;
> >> +       u8 mode;
> >> +       u8 ips_status;
> >> +       u8 error_log_id;
> >> +       u32 error_log_addr_start;
> >> +       u16 error_log_length;
> >> +       u8 reserved2;
> >> +       u8 reserved3;
> >> +       u8 occ_code_level[16];
> >> +       u8 sensor_eye_catcher[6];
> >> +       u8 sensor_block_num;
> >> +       u8 sensor_data_version;
> >> +} __attribute__((packed, aligned(4)));
> >> +
> >> +struct occ_response {
> >> +       struct occ_poll_header header;
> >> +       struct occ_blocks data;
> >> +};
> >> +
> >> +struct occ {
> >> +       struct device *dev;
> >> +       void *bus;
> >> +       struct occ_bus_ops bus_ops;
> >> +       struct occ_ops ops;
> >> +       struct occ_config config;
> >> +       unsigned long update_interval;
> >> +       unsigned long last_updated;
> >> +       struct mutex update_lock;
> >> +       struct occ_response response;
> >> +       bool valid;
> >> +};
> >> +
> >> +static void deinit_occ_resp_buf(struct occ_response *resp)
> >> +{
> >> +       int i;
> >> +
> >> +       if (!resp)
> >> +               return;
> >> +
> >
> >
> > This creates the impression that resp can ever be NULL, which is not the
> > case.
> 
> Agreed. This might go away anyway if I follow your suggestions below
> about memory management.
> 
> >
> >> +       if (!resp->data.blocks)
> >> +               return;
> >> +
> >> +       for (i = 0; i < resp->header.sensor_block_num; ++i)
> >> +               kfree(resp->data.blocks[i].sensors);
> >> +
> >> +       kfree(resp->data.blocks);
> >> +/sensor_type_s
> >>
> >> +       memset(resp, 0, sizeof(struct occ_response));
> >> +
> >> +       for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
> >> +               resp->data.sensor_block_id[i] = -1;
> >> +}
> >> +
> >> +static void *occ_get_sensor_by_type(struct occ_response *resp,
> >> +                                   enum sensor_type t)
> >> +{
> >> +       if (!resp->data.blocks)
> >> +               return NULL;
> >> +
> >> +       if (resp->data.sensor_block_id[t] == -1)
> >> +               return NULL;
> >> +
> >> +       return resp->data.blocks[resp->data.sensor_block_id[t]].sensors;
> >> +}
> >> +
> >> +static int occ_check_sensor(struct occ *driver, u8 sensor_length,
> >> +                           u8 sensor_num, enum sensor_type t, int
> block)
> >> +{
> >> +       void *sensor;
> >> +       int type_block_id;
> >> +       struct occ_response *resp = &driver->response;
> >> +
> >> +       sensor = occ_get_sensor_by_type(resp, t);
> >> +
> >> +       /* empty sensor block, release older sensor data */
> >> +       if (sensor_num == 0 || sensor_length == 0) {
> >> +               kfree(sensor);
> >> +               dev_err(driver->dev, "no sensor blocks available\n");
> >> +               return -ENODATA;
> >> +       }
> >> +
> >> +       type_block_id = resp->data.sensor_block_id[t];
> >> +       if (!sensor || sensor_num !=
> >> +           resp->data.blocks[type_block_id].header.sensor_num) {
> >> +               kfree(sensor);
> >> +               resp->data.blocks[block].sensors =
> >> +                       driver->ops.alloc_sensor(t, sensor_num);
> >> +               if (!resp->data.blocks[block].sensors)
> >> +                       return -ENOMEM;
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int parse_occ_response(struct occ *driver, u8 *data,
> >> +                             struct occ_response *resp)
> >> +{
> >> +       int b;
> >> +       int s;
> >> +       int rc;
> >> +       int offset = SENSOR_BLOCK_OFFSET;
> >> +       int sensor_type;
> >> +       u8 sensor_block_num;
> >> +       char sensor_type_string[5] = { 0 };
> >
> >
> > The only purpose of this variable seems to be to zero-terminate it to be
> > able
> > to print it. Why not use length-limited print functions instead ?
> 
> Is there such a function to length limit to dev_err or dev_dbg?
> Otherwise I have to snprintf or strncpy into a string to pass anyway.
> I can drop the dev_dbg but I would like to print out the sensor string
> if it doesn't match any of the expected ones.
> 
%*s or %-*s ?

> >
> >> +       struct sensor_data_block_header *block;
> >> +       struct device *dev = driver->dev;
> >> +
> >> +       /* check if the data is valid */
> >> +       if (strncmp(&data[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {
> >> +               dev_err(dev, "no SENSOR string in response\n");
> >
> >
> > Are those messages really necessary ? That can create a lot of logging
> noise
> > if the occ responds with bad data.
> 
> I will audit these, but I think it's limited to one error message
> logged in any one function in case of error. Is that acceptable?
> Otherwise it may be hard to tell where the error occurred.
> 
> >
> >> +               rc = -ENODATA;
> >> +               goto err;
> >> +       }
> >> +
> >> +       sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET];
> >
> >
> > There is a lot of trust in assuming that this data was actually received.
> > At the same time, there is a lot of error checking. Why check if the
> > information in this field is valid if you don't check if it was
> > received in the first place ?
> 
> If we get to parse_occ_response, we know we got some data back from
> the OCC (see occ_get_all). Here we are really checking the response is
> a valid one, which I think makes sense in the parsing function.
> 
Yes, but you don't check the length of the data vs. the length indicated
in the header.

> >
> >> +       if (sensor_block_num == 0) {
> >> +               dev_err(dev, "no sensor blocks available\n");
> >> +               rc = -ENODATA;
> >> +               goto err;
> >> +       }
> >> +
> >> +       /* if number of sensor block has changed, re-malloc */
> >> +       if (sensor_block_num != resp->header.sensor_block_num) {
> >> +               deinit_occ_resp_buf(resp);
> >> +               resp->data.blocks = kcalloc(sensor_block_num,
> >> +                                           sizeof(struct
> >> sensor_data_block),
> >> +                                           GFP_KERNEL);
> >> +               if (!resp->data.blocks)
> >> +                       return -ENOMEM;
> >
> >
> > Why not just allocate the maximum number of sensors once ? That might
> waste
> > a bit of memory, but it would make the code much less error prone. We
> know
> > that
> > there will never be more sensors than the number fitting into 4k of
> memory,
> > so the "waste" would not really be that much.
> 
> Fair point. I'll do that.
> 
> >
> >
> >> +       }
> >> +
> >> +       memcpy(&resp->header, &data[RESP_HEADER_OFFSET],
> >> +              sizeof(struct occ_poll_header));
> >> +       resp->header.error_log_addr_start =
> >> +               be32_to_cpu(resp->header.error_log_addr_start);
> >> +       resp->header.error_log_length =
> >> +               be16_to_cpu(resp->header.error_log_length);
> >> +
> >> +       dev_dbg(dev, "Reading %d sensor blocks\n",
> >> +               resp->header.sensor_block_num);
> >> +       for (b = 0; b < sensor_block_num; b++) {
> >> +               block = (struct sensor_data_block_header *)&data
> [offset];
> >> +               /* copy to a null terminated string */
> >> +               strncpy(sensor_type_string, block->sensor_type, 4);
> >> +               offset += 8;
> >> +
> >> +               dev_dbg(dev, "sensor block[%d]: type: %s, sensor_num:
> >> %d\n", b,
> >> +                       sensor_type_string, block->sensor_num);
> >> +
> >> +               if (strncmp(block->sensor_type, "FREQ", 4) == 0)
> >> +                       sensor_type = FREQ;
> >> +               else if (strncmp(block->sensor_type, "TEMP", 4) == 0)
> >> +                       sensor_type = TEMP;
> >> +               else if (strncmp(block->sensor_type, "POWR", 4) == 0)
> >> +                       sensor_type = POWER;
> >> +               else if (strncmp(block->sensor_type, "CAPS", 4) == 0)
> >> +                       sensor_type = CAPS;
> >> +               else {
> >> +                       dev_err(dev, "sensor type not supported %s\n",
> >> +                               sensor_type_string);
> >> +                       continue;
> >> +               }
> >> +
> >> +               rc = occ_check_sensor(driver, block->sensor_length,
> >> +                                     block->sensor_num, sensor_type,
> b);
> >> +               if (rc == -ENOMEM)
> >> +                       goto err;
> >> +               else if (rc)
> >> +                       continue;
> >> +
> >> +               resp->data.sensor_block_id[sensor_type] = b;
> >> +               for (s = 0; s < block->sensor_num; s++) {
> >> +                       driver->ops.parse_sensor(data,
> >> +
> >> resp->data.blocks[b].sensors,
> >> +                                                sensor_type, offset,
> s);
> >> +                       offset += block->sensor_length;
> >> +               }
> >> +
> >> +               /* copy block data over to response pointer */
> >> +               resp->data.blocks[b].header = *block;
> >> +       }
> >> +
> >> +       return 0;
> >> +err:
> >> +       deinit_occ_resp_buf(resp);
> >> +       return rc;
> >> +}
> >> +
> >> +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,
> >> +                      const u8 *data, u8 *resp)
> >> +{
> >> +       u32 cmd1, cmd2;
> >> +       u16 checksum = 0;
> >> +       u16 length_le = cpu_to_le16(length);
> >> +       bool retry = 0;
> >> +       int i, rc, tries = 0;
> >> +
> >> +       cmd1 = (seq << 24) | (type << 16) | length_le;
> >> +       memcpy(&cmd2, data, length);
> >> +       cmd2 <<= ((4 - length) * 8);
> >> +
> >> +       /* checksum: sum of every bytes of cmd1, cmd2 */
> >> +       for (i = 0; i < 4; i++) {
> >> +               checksum += (cmd1 >> (i * 8)) & 0xFF;
> >> +               checksum += (cmd2 >> (i * 8)) & 0xFF;
> >> +       }
> >> +
> >> +       cmd2 |= checksum << ((2 - length) * 8);
> >> +
> >> +       /* Init OCB */
> >> +       rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
> >> +                                    OCB_OR_INIT0, OCB_OR_INIT1);
> >> +       if (rc)
> >> +               goto err;
> >> +
> >> +       rc = driver->bus_ops.putscom(driver->bus,
> OCB_STATUS_CONTROL_AND,
> >> +                                    OCB_AND_INIT0, OCB_AND_INIT1);
> >> +       if (rc)
> >> +               goto err;
> >> +
> >> +       /* Send command, 2nd half of the 64-bit addr is unused (write 0)
> >> */
> >> +       rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
> >> +                                    driver->config.command_addr, 0);
> >> +       if (rc)
> >> +               goto err;
> >> +
> >> +       rc = driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1, cmd2);
> >> +       if (rc)
> >> +               goto err;
> >> +
> >> +       /* Trigger attention */
> >> +       rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA, ATTN0,
> >> ATTN1);
> >> +       if (rc)
> >> +               goto err;
> >> +
> >> +       /* Get response data */
> >> +       rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
> >> +                                    driver->config.response_addr, 0);
> >> +       if (rc)
> >> +               goto err;
> >> +
> >> +       do {
> >> +               if (retry) {
> >> +                       set_current_state(TASK_INTERRUPTIBLE);
> >> +
> >> schedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
> >> +               }
> >> +
> >
> > Isn't there a better way to do this ? What exactly is interrupted ? The
> > getscom function ? If so, shouldn't the timeout be handled there ?
> 
> There shouldn't be any interrupt. I set TASK_INTERRUPTIBLE because it
> doesn't really matter if it's interrupted or not. The purpose of this
> loop is to retry if we get the "command in progress" status from the
> OCC. It's higher level than the getscom function, so I think the loop
> is appropriate here.
> 
This is quite unusual. Why is this needed here but not elsewhere
in the kernel ? Normally the called function would time out and return
an appropriate error.

> >
> >> +               rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
> >> +                                            (u64 *)resp);
> >> +               if (rc)
> >> +                       goto err;
> >> +
> >> +               /* retry if we get "command in progress" return status
> */
> >> +               retry = (resp[RESP_RETURN_STATUS] ==
> >> RESP_RETURN_CMD_IN_PRG) &&
> >> +                       (tries++ < CMD_IN_PRG_RETRIES);
> >
> >
> > There are some unnecessary () in this expression.
> >
> >
> >> +       } while (retry);
> >> +
> >> +       switch (resp[RESP_RETURN_STATUS]) {
> >> +       case RESP_RETURN_CMD_IN_PRG:
> >> +               rc = -EALREADY;
> >> +               break;
> >> +       case RESP_RETURN_SUCCESS:
> >> +               rc = 0;
> >> +               break;
> >> +       case RESP_RETURN_CMD_INVAL:
> >> +       case RESP_RETURN_CMD_LEN:
> >> +       case RESP_RETURN_DATA_INVAL:
> >> +       case RESP_RETURN_CHKSUM:
> >> +               rc = -EINVAL;
> >> +               break;
> >> +       case RESP_RETURN_OCC_ERR:
> >> +               rc = -EREMOTE;
> >> +               break;
> >> +       default:
> >> +               rc = -EFAULT;
> >> +       }
> >> +
> >> +       return rc;
> >> +
> >> +err:
> >> +       dev_err(driver->dev, "scom op failed rc:%d\n", rc);
> >
> >
> > Some of the errors result in an error message, some don't. What is
> > the guiding principle ? Even if there is an error message, it doesn't
> report
> > what function caused it, which makes it as good as no error message.
> > Can this be dropped ?
> 
> resp[RESP_RETURN_STATUS] is the status reported on the OCC, these
> aren't necessarily errors, though most are. This is translating an OCC
> status code to an error for occ_get_all to pick up. It cannot be
> dropped, we need to know if the occ poll was successful. I'm not sure
> what you mean by reporting the function? occ_get_all will fail out if

There are several function returns which all end up here. You don't know
which one actually failed.

> we report the error here. If it's the scom op that failed we will have
> the dev_err message.
> 
But which scom operation ? You mean it is important to report that some
operation failed, but not which one ? Ok, I'll accept that, it just seems
odd.

> >
> >> +       return rc;
> >> +}
> >> +
> >> +static int occ_get_all(struct occ *driver)
> >> +{
> >> +       int i = 0, rc;
> >> +       u8 *occ_data;
> >> +       u16 num_bytes;
> >> +       const u8 poll_cmd_data = OCC_POLL_STAT_SENSOR;
> >> +       struct device *dev = driver->dev;
> >> +       struct occ_response *resp = &driver->response;
> >> +
> >> +       occ_data = devm_kzalloc(dev, OCC_DATA_MAX, GFP_KERNEL);
> >
> >
> > I am a bot lost here. What is the purpose of using a devm function ?
> >
> > Either case, you might want to allocate the memory once and just keep it
> > allocated.
> > Allocating a 4k buffer and freeing it each time data is read from the occ
> > seems
> > to be wasteful and not really worth it.
> 
> Agreed.
> 
> >
> >> +       if (!occ_data)
> >> +               return -ENOMEM;
> >> +
> >> +       rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data,
> >> occ_data);
> >> +       if (rc) {
> >> +               dev_err(dev, "OCC poll failed: %d\n", rc);
> >> +               goto out;
> >> +       }
> >> +
> >> +       num_bytes = get_unaligned((u16 *)&occ_data[RESP_DATA_LENGTH]);
> >> +       num_bytes = be16_to_cpu(num_bytes);
> >
> >
> > What data is in big endian, and what data is in little endian ?
> >
> > Looks like everything is big endian except for the length in the send
> > command.
> > Is that correct ? If so, an explanation would be useful, because the
> logic
> > isn't entirely clear.
> 
> I'll add some comments.
> 
> >
> >> +       dev_dbg(dev, "OCC data length: %d\n", num_bytes);
> >> +
> >> +       if (num_bytes > OCC_DATA_MAX) {
> >> +               dev_err(dev, "OCC data length must be < 4KB\n");
> >> +               rc = -EINVAL;
> >> +               goto out;
> >> +       }
> >> +
> >> +       if (num_bytes <= 0) {
> >
> >
> > This can never be < 0.
> 
> Good point.
> 
> >
> >> +               dev_err(dev, "OCC data length is zero\n");
> >> +               rc = -EINVAL;
> >> +               goto out;
> >> +       }
> >> +
> >> +       /* read remaining data */
> >> +       for (i = 8; i < num_bytes + 8; i += 8) {
> >> +               rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
> >> +                                            (u64 *)&occ_data[i]);
> >> +               if (rc) {
> >> +                       dev_err(dev, "scom op failed rc:%d\n", rc);
> >> +                       goto out;
> >> +               }
> >> +       }
> >> +
> >> +       /* don't need more sanity checks; buffer is alloc'd for max
> >> response
> >> +        * size so we just check for valid data in parse_occ_response
> >> +        */
> >
> > Yes, but as mentioned above you don't check in that function if the data
> > was actually received.
> 
> If we don't receive data, the data length will be 0 or the low level
> getscom will fail. So we know we have data here.
> 
Ok. However, since you don't pass num_bytes to the parse function,
there is no verification that num_bytes actually matches what is in the packet.
The rest of the header could indicate that there is 4k of data, but num_bytes
reports, say, the minimum size. In that case, parse_occ_response() would end
up parsing a stale buffer.

> >
> >> +       rc = parse_occ_response(driver, occ_data, resp);
> >> +
> >> +out:
> >> +       devm_kfree(dev, occ_data);
> >> +       return rc;
> >> +}
> >> +
> >> +int occ_update_device(struct occ *driver)
> >> +{
> >> +       int rc = 0;
> >> +
> >> +       mutex_lock(&driver->update_lock);
> >> +
> >> +       if (time_after(jiffies, driver->last_updated +
> >> driver->update_interval)
> >> +           || !driver->valid) {
> >> +               driver->valid = 1;
> >
> >
> > valid is bool.
> >
> >> +
> >> +               rc = occ_get_all(driver);
> >> +               if (rc)
> >> +                       driver->valid = 0;
> >
> >
> > valid is bool.
> 
> Good point.
> 
> >
> >> +
> >> +               driver->last_updated = jiffies;
> >> +       }
> >> +
> >> +       mutex_unlock(&driver->update_lock);
> >> +
> >> +       return rc;
> >> +}
> >> +EXPORT_SYMBOL(occ_update_device);
> >> +
> >> +void *occ_get_sensor(struct occ *driver, int sensor_type)
> >> +{
> >> +       int rc;
> >> +
> >> +       /* occ_update_device locks the update lock */
> >> +       rc = occ_update_device(driver);
> >> +       if (rc) {
> >> +               dev_err(driver->dev, "cannot get occ sensor data: %d\n",
> >> +                       rc);
> >
> >
> > Have you counted the number of error messages reported if anything goes
> > wrong.
> > This driver is quite noisy. I won't force you to drop the messages, but
> it
> > seems
> > to me that if anything goes wrong, the kernel log will be clogged with
> > messages
> > just from this driver.
> 
> I'll audit these.
> 
> >
> >
> >> +               return NULL;
> >> +       }
> >> +
> >> +       return occ_get_sensor_by_type(&driver->response, sensor_type);
> >> +}
> >> +EXPORT_SYMBOL(occ_get_sensor);
> >> +
> >> +int occ_get_sensor_value(struct occ *occ, int sensor_type, int
> >> sensor_num,
> >> +                        u32 hwmon, long *val)
> >> +{
> >> +       return occ->ops.get_sensor(occ, sensor_type, sensor_num, hwmon,
> >> val);
> >> +}
> >> +EXPORT_SYMBOL(occ_get_sensor_value);
> >> +
> >> +void occ_get_response_blocks(struct occ *occ, struct occ_blocks
> **blocks)
> >> +{
> >> +       *blocks = &occ->response.data;
> >> +}
> >> +EXPORT_SYMBOL(occ_get_response_blocks);
> >> +
> >> +void occ_set_update_interval(struct occ *occ, unsigned long interval)
> >> +{
> >> +       occ->update_interval = msecs_to_jiffies(interval);
> >> +}
> >> +EXPORT_SYMBOL(occ_set_update_interval);
> >> +
> >> +int occ_set_user_powercap(struct occ *occ, u16 cap)
> >> +{
> >> +       u8 resp[8];
> >> +
> >> +       cap = cpu_to_be16(cap);
> >> +
> >> +       return occ_send_cmd(occ, 0, OCC_SET_USER_POWR_CAP, 2, (const u8
> >> *)&cap,
> >> +                           resp);
> >> +}
> >> +EXPORT_SYMBOL(occ_set_user_powercap);
> >> +
> >> +struct occ *occ_start(struct device *dev, void *bus,
> >> +                     struct occ_bus_ops *bus_ops, const struct occ_ops
> >> *ops,
> >> +                     const struct occ_config *config)
> >> +{
> >> +       struct occ *driver = devm_kzalloc(dev, sizeof(struct occ),
> >> GFP_KERNEL);
> >> +
> >> +       if (!driver)
> >> +               return ERR_PTR(-ENOMEM);
> >> +
> >> +       driver->dev = dev;
> >> +       driver->bus = bus;
> >> +       driver->bus_ops = *bus_ops;
> >> +       driver->ops = *ops;
> >> +       driver->config = *config;
> >> +
> >> +       driver->update_interval = HZ;
> >
> >
> > The update interval is only supposed to be configurable if it can be
> written
> > into the hardware. Otherwise it is really up to user space to decide how
> > often
> > it wants to poll the data, not up to the kernel.
> >
> > I don't see a function to send the update interval to the occ. As such,
> it
> > should
> > not be configurable but a constant.
> 
> I don't quite follow. Right now, the driver maximum polling rate is
> configurable by userspace (via hwmon sysfs attribute). If you mean
> that user space should control that rate just by how often users read
> the attributes, I'm not sure that will work as there are a large
> number of attributes that will all try and trigger an occ poll when
> read. Besides, different users don't want to communicate about when
> they last polled. If the rate is constant, if may be too slow or too
> fast depending on the system and needs of the users. Why can't the
> polling rate be configurable? To user space, the update interval works
> the same if it's driver or hardware.
> 

The update_interval attribute is only supposed to exist if writing it
results in writing a value into a chip register. It is supposed to set
the _chip_ update interval, not the kernel update interval. If you want
to limit the rate of chip accesses, you can do it by using a constant
in the driver, just like other drivers do it.

This is similar to limit registers. If the chip does not support limit
registers, we don't make up "soft" limits either, but don't support
the respective attributes.

Thanks,
Guenter

Re: [PATCH linux v3 2/6] hwmon: occ: Add sysfs interface

[Guenter Roeck]

On Mon, Jan 23, 2017 at 04:01:25PM -0600, Edward James wrote:
> 
> 
> On Sat, Jan 21, 2017 at 12:08 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 01/16/2017 01:13 PM, eajames.ibm@gmail.com wrote:
> >>
> >> From: "Edward A. James" <eajames@us.ibm.com>
> >>
> >> Add a generic mechanism to expose the sensors provided by the OCC in
> >> sysfs.
> >>
> >> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> >> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> >> ---
> >>  Documentation/hwmon/occ       |  62 ++++++++++
> >>  drivers/hwmon/occ/Makefile    |   2 +-
> >>  drivers/hwmon/occ/occ_sysfs.c | 271
> >> ++++++++++++++++++++++++++++++++++++++++++
> >>  drivers/hwmon/occ/occ_sysfs.h |  44 +++++++
> >>  4 files changed, 378 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/hwmon/occ/occ_sysfs.c
> >>  create mode 100644 drivers/hwmon/occ/occ_sysfs.h
> >>
> >> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> >> index 79d1642..d0bdf06 100644
> >> --- a/Documentation/hwmon/occ
> >> +++ b/Documentation/hwmon/occ
> >> @@ -25,6 +25,68 @@ Currently, all versions of the OCC support four types
> >> of sensor data: power,
> >>  temperature, frequency, and "caps," which indicate limits and
> thresholds
> >> used
> >>  internally on the OCC.
> >>
> >> +sysfs Entries
> >> +-------------
> >> +
> >> +The OCC driver uses the hwmon sysfs framework to provide data to
> >> userspace.
> >> +
> >> +The driver exports a number of sysfs files for each type of sensor. The
> >> +sensor-specific files vary depending on the processor type, though many
> >> of the
> >> +attributes are common for both the POWER8 and POWER9.
> >> +
> >> +The hwmon interface cannot define every type of sensor that may be
> used.
> >> +Therefore, the frequency sensor on the OCC uses the "input" type sensor
> >> defined
> >> +by the hwmon interface, rather than defining a new type of custom
> sensor.
> >> +
> >> +Below are detailed the names and meaning of each sensor file for both
> >> types of
> >> +processors. All sensors are read-only unless otherwise specified. <x>
> >> indicates
> >> +the hwmon index. sensor id indicates the unique internal OCC identifer.
> >> Please
> >> +see the POWER OCC specification for details on all these sensor values.
> >> +
> >> +frequency:
> >> +       all processors:
> >> +               in<x>_input - frequency value
> >> +               in<x>_label - sensor id
> >> +temperature:
> >> +       POWER8:
> >> +               temp<x>_input - temperature value
> >> +               temp<x>_label - sensor id
> >> +       POWER9 (in addition to above):
> >> +               temp<x>_type - FRU type
> >> +
> >> +power:
> >> +       POWER8:
> >> +               power<x>_input - power value
> >> +               power<x>_label - sensor id
> >> +               power<x>_average - accumulator
> >> +               power<x>_average_interval - update tag (number of
> samples
> >> in
> >> +                       accumulator)
> >> +       POWER9:
> >> +               power<x>_input - power value
> >> +               power<x>_label - sensor id
> >> +               power<x>_average_min - accumulator[0]
> >> +               power<x>_average_max - accumulator[1] (64 bits total)
> >> +               power<x>_average_interval - update tag
> >> +               power<x>_reset_history - (function_id | (apss_channel <<
> >> 8)
> >> +
> >> +caps:
> >> +       POWER8:
> >> +               power<x>_cap - current powercap
> >> +               power<x>_cap_max - max powercap
> >> +               power<x>_cap_min - min powercap
> >> +               power<x>_max - normal powercap
> >> +               power<x>_alarm - user powercap, r/w
> >> +       POWER9:
> >> +               power<x>_cap_alarm - user powercap source
> >> +
> >> +The driver also provides two sysfs entries through hwmon to better
> >> +control the driver and monitor the master OCC. Though there may be
> >> multiple
> >> +OCCs present on the system, these two files are only present for the
> >> "master"
> >> +OCC.
> >> +       name - read the name of the driver
> >> +       update_interval - read or write the minimum interval for polling
> >> the
> >> +               OCC.
> >> +
> >>  BMC - Host Communications
> >>  -------------------------
> >>
> >> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> >> index 93cb52f..a6881f9 100644
> >> --- a/drivers/hwmon/occ/Makefile
> >> +++ b/drivers/hwmon/occ/Makefile
> >> @@ -1 +1 @@
> >> -obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
> >> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> >> diff --git a/drivers/hwmon/occ/occ_sysfs.c
> b/drivers/hwmon/occ/occ_sysfs.c
> >> new file mode 100644
> >> index 0000000..2f20c40
> >> --- /dev/null
> >> +++ b/drivers/hwmon/occ/occ_sysfs.c
> >> @@ -0,0 +1,271 @@
> >> +/*
> >> + * occ_sysfs.c - OCC sysfs interface
> >> + *
> >> + * This file contains the methods and data structures for implementing
> >> the OCC
> >> + * hwmon sysfs entries.
> >> + *
> >> + * Copyright 2016 IBM Corp.
> >> + *
> >> + * 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.
> >> + */
> >> +
> >> +#include <linux/delay.h>
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >> +#include <linux/hwmon.h>
> >> +#include <linux/hwmon-sysfs.h>
> >> +#include <linux/init.h>
> >> +#include <linux/jiffies.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/slab.h>
> >> +#include "occ.h"
> >> +#include "occ_sysfs.h"
> >> +
> >> +#define RESP_RETURN_CMD_INVAL  0x13
> >> +
> >> +static int occ_hwmon_read(struct device *dev, enum hwmon_sensor_types
> >> type,
> >> +                         u32 attr, int channel, long *val)
> >> +{
> >> +       int rc = 0;
> >
> >
> > Unnecessary initialization.
> 
> Got it.
> 
> >
> >
> >> +       struct occ_sysfs *driver = dev_get_drvdata(dev);
> >> +       struct occ *occ = driver->occ;
> >> +
> >> +       switch (type) {
> >> +       case hwmon_in:
> >> +               rc = occ_get_sensor_value(occ, FREQ, channel, attr,
> val);
> >> +               break;
> >> +       case hwmon_temp:
> >> +               rc = occ_get_sensor_value(occ, TEMP, channel, attr,
> val);
> >> +               break;
> >> +       case hwmon_power:
> >> +               rc = occ_get_sensor_value(occ, POWER, channel, attr,
> val);
> >> +               break;
> >> +       default:
> >> +               rc = -EOPNOTSUPP;
> >> +       }
> >> +
> >> +       return rc;
> >> +}
> >> +
> >> +static int occ_hwmon_read_string(struct device *dev,
> >> +                                enum hwmon_sensor_types type, u32 attr,
> >> +                                int channel, char **str)
> >> +{
> >> +       int rc;
> >> +       unsigned long val = 0;
> >> +
> >> +       if (!((type == hwmon_in && attr == hwmon_in_label) ||
> >> +           (type == hwmon_temp && attr == hwmon_temp_label) ||
> >> +           (type == hwmon_power && attr == hwmon_power_label)))
> >> +               return -EOPNOTSUPP;
> >> +
> >> +       rc = occ_hwmon_read(dev, type, attr, channel, &val);
> >> +       if (rc < 0)
> >> +               return rc;
> >> +
> >> +       rc = snprintf(*str, PAGE_SIZE - 1, "%ld", val);
> >
> >
> > val is unsigned long.
> >
> > I am quite confused what this is for, though. The function is used for
> > string attributes,
> > or in other words for labels. What is the benefit of having a label that
> > returns the same
> > data as the value attribute ? Is this maybe a misunderstanding ?
> 
> So this does work, though I admit it's a bit confusing.
> occ_get_sensor_value returns either the value or the occ sensor id
> depending on the "type" passed in. I'll rename occ_get_sensor_value
> and add a comment.
> 
> >
> >> +       if (rc > 0)
> >> +               rc = 0;
> >> +
> >> +       return rc;
> >> +}
> >> +
> >> +static int occ_hwmon_write(struct device *dev, enum hwmon_sensor_types
> >> type,
> >> +                          u32 attr, int channel, long val)
> >> +{
> >> +       int rc = 0;
> >> +       struct occ_sysfs *driver = dev_get_drvdata(dev);
> >> +
> >> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> >> +               occ_set_update_interval(driver->occ, val);
> >
> >
> > As mentioned in the other patch, please drop. This is not the intended
> use
> > case
> > for this attribute.
> 
> OK.
> 
> >
> >
> >> +               return 0;
> >> +       } else if (type == hwmon_power && attr == hwmon_power_alarm) {
> >> +               rc = occ_set_user_powercap(driver->occ, val);
> >> +               if (rc) {
> >> +                       if (rc == RESP_RETURN_CMD_INVAL) {
> >> +                               dev_err(dev,
> >> +                                       "set invalid powercap value:
> >> %ld\n",
> >> +                                       val);
> >> +                               return -EINVAL;
> >> +                       }
> >> +
> >> +                       dev_err(dev, "set user powercap failed: 0x:%x
> \n",
> >> rc);
> >> +                       return rc;
> >> +               }
> >> +
> >> +               driver->user_powercap = val;
> >> +
> >> +               return rc;
> >> +       }
> >> +
> >> +       return -EOPNOTSUPP;
> >> +}
> >> +
> >> +static umode_t occ_is_visible(const void *data, enum hwmon_sensor_types
> >> type,
> >> +                             u32 attr, int channel)
> >> +{
> >> +       const struct occ_sysfs *driver = data;
> >> +
> >> +       switch (type) {
> >> +       case hwmon_chip:
> >> +               if (attr == hwmon_chip_update_interval)
> >> +                       return S_IRUGO | S_IWUSR;
> >> +               break;
> >> +       case hwmon_in:
> >> +               if (BIT(attr) & driver->sensor_hwmon_configs[0])
> >> +                       return S_IRUGO;
> >> +               break;
> >> +       case hwmon_temp:
> >> +               if (BIT(attr) & driver->sensor_hwmon_configs[1])
> >> +                       return S_IRUGO;
> >> +               break;
> >> +       case hwmon_power:
> >> +               /* user power limit */
> >> +               if (attr == hwmon_power_alarm)
> >> +                       return S_IRUGO | S_IWUSR;
> >> +               else if ((BIT(attr) & driver->sensor_hwmon_configs[2])
> ||
> >> +                        (BIT(attr) & driver->sensor_hwmon_configs[3]))
> >> +                       return S_IRUGO;
> >> +               break;
> >> +       default:
> >> +               return 0;
> >
> >
> > Might as well use break; here for consistency.
> >
> >
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static const struct hwmon_ops occ_hwmon_ops = {
> >> +       .is_visible = occ_is_visible,
> >> +       .read = occ_hwmon_read,
> >> +       .read_string = occ_hwmon_read_string,
> >> +       .write = occ_hwmon_write,
> >> +};
> >> +
> >> +static const u32 occ_chip_config[] = {
> >> +       HWMON_C_UPDATE_INTERVAL,
> >> +       0
> >> +};
> >> +
> >> +static const struct hwmon_channel_info occ_chip = {
> >> +       .type = hwmon_chip,
> >> +       .config = occ_chip_config
> >> +};
> >> +
> >> +static const enum hwmon_sensor_types
> >> occ_sensor_types[MAX_OCC_SENSOR_TYPE] = {
> >> +       hwmon_in,
> >> +       hwmon_temp,
> >> +       hwmon_power,
> >> +       hwmon_power
> >> +};
> >> +
> >> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> >> +                                 const u32 *sensor_hwmon_configs,
> >> +                                 const char *name)
> >> +{
> >> +       bool master_occ = false;
> >> +       int rc, i, j, sensor_num, index = 0, id;
> >> +       char *brk;
> >> +       struct occ_blocks *resp = NULL;
> >> +       u32 *sensor_config;
> >> +       struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct
> >> occ_sysfs),
> >> +                                              GFP_KERNEL);
> >> +       if (!hwmon)
> >> +               return ERR_PTR(-ENOMEM);
> >> +
> >> +       /* need space for null-termination and occ chip */
> >> +       hwmon->occ_sensors =
> >> +               devm_kzalloc(dev, sizeof(struct hwmon_channel_info *) *
> >> +                            (MAX_OCC_SENSOR_TYPE + 2), GFP_KERNEL);
> >> +       if (!hwmon->occ_sensors)
> >> +               return ERR_PTR(-ENOMEM);
> >> +
> >> +       hwmon->occ = occ;
> >> +       hwmon->sensor_hwmon_configs = (u32 *)sensor_hwmon_configs;
> >
> >
> > Either this is a const or it isn't. If it isn't, it should not be passed
> in
> > as const.
> > If it is, it can be declared const in struct occ_sysfs.
> 
> It is const, i'll fix.
> 
> >
> >
> >> +       hwmon->occ_info.ops = &occ_hwmon_ops;
> >> +       hwmon->occ_info.info =
> >> +               (const struct hwmon_channel_info **)hwmon->occ_sensors;
> >
> >
> > Why is this typecast needed ?
> 
> struct hwmon_chip_info {
>         const struct hwmon_ops *ops;
>         const struct hwmon_channel_info **info;
> };
> 
> My compiler throws an error without a cast here.
> 
> occ_sysfs.c:195:23: error: assignment from incompatible pointer type
> [-Werror=incompatible-pointer-types]
> |   hwmon->occ_info.info = hwmon->occ_sensors;
> 
> occ_sensors is not const, as it's dynamically zero-allocated and then
> if we poll and get a sensor, we allocate a new hwmon_channel_info. Not
> sure what the best solution is here except to cast.
> 

Your statement makes me quite concerned. "if we poll and get a sensor, we
allocate a new hwmon_channel_info". Are you doing that before or after
registering with the hwmon core ? Hopefully before.

> >
> >
> >> +
> >> +       occ_get_response_blocks(occ, &resp);
> >> +
> >> +       for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
> >> +               resp->sensor_block_id[i] = -1;
> >> +
> >> +       /* read sensor data from occ */
> >> +       rc = occ_update_device(occ);
> >> +       if (rc) {
> >> +               dev_err(dev, "cannot get occ sensor data: %d\n", rc);
> >> +               return ERR_PTR(rc);
> >> +       }
> >> +       if (!resp->blocks)
> >> +               return ERR_PTR(-ENOMEM);
> >> +
> >> +       master_occ = resp->sensor_block_id[CAPS] >= 0;
> >> +
> >> +       for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
> >> +               id = resp->sensor_block_id[i];
> >> +               if (id < 0)
> >> +                       continue;
> >> +
> >> +               sensor_num = resp->blocks[id].header.sensor_num;
> >> +               /* need null-termination */
> >> +               sensor_config = devm_kzalloc(dev,
> >> +                                            sizeof(u32) * (sensor_num +
> >> 1),
> >> +                                            GFP_KERNEL);
> >> +               if (!sensor_config)
> >> +                       return ERR_PTR(-ENOMEM);
> >> +
> >> +               for (j = 0; j < sensor_num; j++)
> >> +                       sensor_config[j] = sensor_hwmon_configs[i];
> >> +
> >> +               hwmon->occ_sensors[index] =
> >> +                       devm_kzalloc(dev, sizeof(struct
> >> hwmon_channel_info),
> >> +                                    GFP_KERNEL);
> >> +               if (!hwmon->occ_sensors[index])
> >> +                       return ERR_PTR(-ENOMEM);
> >> +
> >> +               hwmon->occ_sensors[index]->type = occ_sensor_types[i];
> >> +               hwmon->occ_sensors[index]->config = sensor_config;
> >> +               index++;
> >> +       }
> >
> >
> > I had the impression from patch 1 that the number of sensors can change
> at
> > runtime.
> > If so, this doesn't really work well; the sysfs attributes won't be
> updated
> > in this
> > case.
> >
> > Can it really happen that the sensor information can change ?
> 
> Well, the number of sensors won't change while we are running. But we
> don't know how many sensors we have when we start up. So this first
> poll will tell us how many we need and allow the driver to initialize
> the correct number of hwmon attributes. But yes, it shouldn't change while
> we
> are running.
> 
I'll have to look into the other patch again. I seem to recall that it does
handle changing number of sensors, but I may be wrong.

> >
> >> +
> >> +       /* only need one of these for any number of occs */
> >> +       if (master_occ)
> >> +               hwmon->occ_sensors[index] =
> >> +                       (struct hwmon_channel_info *)&occ_chip;
> >
> >
> > Why this typecast ?
> 
> occ_chip is const but my occ_sensors field isn't.
> 
Hmm .. I'll have to have a closer look. Maybe I need to make the core fields
non-const. Having to use typecasts isn't really desirable and defeats the
purpose.

Guenter

> >
> >
> >> +
> >> +       /* search for bad chars */
> >> +       strncpy(hwmon->hwmon_name, name, OCC_HWMON_NAME_LENGTH);
> >> +       brk = strpbrk(hwmon->hwmon_name, "-* \t\n");
> >> +       while (brk) {
> >> +               *brk = '_';
> >> +               brk = strpbrk(brk,  "-* \t\n");
> >> +       }
> >> +
> >> +       hwmon->dev = devm_hwmon_device_register_with_info(dev,
> >> +
> >> hwmon->hwmon_name,
> >> +                                                         hwmon,
> >> +
> >> &hwmon->occ_info,
> >> +                                                         NULL);
> >> +       if (IS_ERR(hwmon->dev)) {
> >> +               dev_err(dev, "cannot register hwmon device %s: %ld\n",
> >> +                       hwmon->hwmon_name, PTR_ERR(hwmon->dev));
> >> +               return ERR_CAST(hwmon->dev);
> >> +       }
> >> +
> >> +       return hwmon;
> >> +}
> >> +EXPORT_SYMBOL(occ_sysfs_start);
> >> +
> >> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> >> +MODULE_DESCRIPTION("OCC sysfs driver");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/drivers/hwmon/occ/occ_sysfs.h
> b/drivers/hwmon/occ/occ_sysfs.h
> >> new file mode 100644
> >> index 0000000..7de92e7
> >> --- /dev/null
> >> +++ b/drivers/hwmon/occ/occ_sysfs.h
> >> @@ -0,0 +1,44 @@
> >> +/*
> >> + * occ_sysfs.h - OCC sysfs interface
> >> + *
> >> + * This file contains the data structures and function prototypes for
> the
> >> OCC
> >> + * hwmon sysfs entries.
> >> + *
> >> + * Copyright 2016 IBM Corp.
> >> + *
> >> + * 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.
> >> + */
> >> +
> >> +#ifndef __OCC_SYSFS_H__
> >> +#define __OCC_SYSFS_H__
> >> +
> >> +#include <linux/hwmon.h>
> >> +
> >> +struct occ;
> >> +struct device;
> >> +
> >> +#define OCC_HWMON_NAME_LENGTH  32
> >> +
> >> +struct occ_sysfs {
> >> +       struct device *dev;
> >> +       struct occ *occ;
> >> +
> >> +       char hwmon_name[OCC_HWMON_NAME_LENGTH + 1];
> >> +       u32 *sensor_hwmon_configs;
> >> +       struct hwmon_channel_info **occ_sensors;
> >> +       struct hwmon_chip_info occ_info;
> >> +       u16 user_powercap;
> >> +};
> >
> >
> > Is this information used outside the sysfs code ? If not, it should be
> > defined
> > in the source file and not be available outside of it.
> 
> OK. It is used in p8_occ_i2c.c but none of the members are accessed so
> we shouldn't need it defined there, just declared, you're right.
> 
> 
> >
> >
> >> +
> >> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> >> +                                 const u32 *sensor_hwmon_configs,
> >> +                                 const char *name);
> >> +#endif /* __OCC_SYSFS_H__ */
> >>
> >

Re: [PATCH linux v3 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC

[kbuild test robot]

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

Hi Edward,

[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.10-rc5 next-20170125]
[cannot apply to linux/master]
[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/eajames-ibm-gmail-com/drivers-hwmon-Add-On-Chip-Controller-driver/20170117-062329
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-x078-01291145 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "occ_sysfs_start" [drivers/hwmon/occ/p8_occ_i2c.ko] undefined!
>> ERROR: "occ_start" [drivers/hwmon/occ/occ_p8.ko] undefined!
>> ERROR: "occ_get_sensor" [drivers/hwmon/occ/occ_p8.ko] undefined!

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25665 bytes --]