LinuxLists.cc - [PATCH v4 0/9] hwmon and fsi: Add On-Chip Controller (OCC) driver

2018-07-12 02:54:43

Subject: [PATCH v4 0/9] hwmon and fsi: Add On-Chip Controller (OCC) driver

This series adds a hwmon driver to support the OCC on POWER8 and POWER9
processors. The OCC is an embedded processor that provides realtime power and
thermal monitoring and management.

The series also adds a "bus" driver to handle atomic communication between the
service processor and the OCC on a POWER9 chip. This communication takes place
over FSI bus to the SBE (Self-Boot engine) FIFO, which in turn communicates
with the OCC. The driver for the SBEFIFO is already in linux-next as an FSI
client driver.

For POWER8 OCCs, communication between the service processor and the OCC is
achieved over I2C bus.

Changes since v3:
* Add the FSI OCC driver.
* Pull the sysfs attribute code into it's own file for cleanliness.
* Various fixes for attribute creation and integer overflow.

Changes since v2:
* Add sysfs_notify for the error and throttling attributes when change is
detected.
* Removed occs_present counting of devices bound.
* Improved remove() of P9 driver to avoid bad behavior with relation to OCC
driver when unbound.
* Added default cases (return EINVAL) for all sensor show functions.
* Added temperature fault sensor.
* Added back dt binding documentation for P9 to address checkpatch warning.
* Added occs_present attribute from the poll response.

Changes since v1:
* Remove wait loop in P9 code, as that is now handled by FSI OCC driver.
* Removed dt binding documentation for P9, FSI OCC driver will probe OCC hwmon
driver automatically.
* Moved OCC response code definitions to the OCC include file.
* Fixed includes.
* Changed some structure fields to __beXX as that is what they are.
* Changed some errnos.
* Removed some dev_err().
* Refactored P8 code a bit to use #defined addresses and magic values, and
changed "goto retry" to a loop.
* Refactored error handling a bit.

Eddie James (9):
fsi: Add On-Chip Controller (OCC) driver
Documentation: hwmon: Add OCC documentation
dt-bindings: i2c: Add P8 OCC hwmon device documentation
hwmon: Add On-Chip Controller (OCC) hwmon driver
hwmon (occ): Add command transport method for P8 and P9
hwmon (occ): Parse OCC poll response
hwmon (occ): Add sensor types and versions
hwmon (occ): Add sensor attributes and register hwmon device
hwmon (occ): Add sysfs attributes for additional OCC data

.../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 +
Documentation/hwmon/occ | 73 ++
drivers/fsi/Kconfig | 10 +
drivers/fsi/Makefile | 1 +
drivers/fsi/fsi-occ.c | 609 ++++++++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 28 +
drivers/hwmon/occ/Makefile | 11 +
drivers/hwmon/occ/common.c | 1250 ++++++++++++++++++++
drivers/hwmon/occ/common.h | 136 +++
drivers/hwmon/occ/p8_i2c.c | 264 +++++
drivers/hwmon/occ/p9_sbe.c | 115 ++
drivers/hwmon/occ/sysfs.c | 188 +++
include/linux/fsi-occ.h | 34 +
15 files changed, 2747 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt
create mode 100644 Documentation/hwmon/occ
create mode 100644 drivers/fsi/fsi-occ.c
create mode 100644 drivers/hwmon/occ/Kconfig
create mode 100644 drivers/hwmon/occ/Makefile
create mode 100644 drivers/hwmon/occ/common.c
create mode 100644 drivers/hwmon/occ/common.h
create mode 100644 drivers/hwmon/occ/p8_i2c.c
create mode 100644 drivers/hwmon/occ/p9_sbe.c
create mode 100644 drivers/hwmon/occ/sysfs.c
create mode 100644 include/linux/fsi-occ.h

--
1.8.3.1

2018-07-12 02:54:46

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 1/9] fsi: Add On-Chip Controller (OCC) driver

The OCC is a device embedded on a POWER processor that collects and
aggregates sensor data from the processor and system. The OCC can
provide the raw sensor data as well as perform thermal and power
management on the system.

This driver provides an atomic communications channel between a service
processor (e.g. a BMC) and the OCC. The driver is dependent on the FSI
SBEFIFO driver to get hardware access through the SBE to the OCC SRAM.
Commands are issued to the SBE to send or fetch data to the SRAM.

Signed-off-by: Eddie James <[email protected]>
Signed-off-by: Andrew Jeffery <[email protected]>
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
Signed-off-by: Joel Stanley <[email protected]>
---
drivers/fsi/Kconfig | 10 +
drivers/fsi/Makefile | 1 +
drivers/fsi/fsi-occ.c | 609 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/fsi-occ.h | 34 +++
4 files changed, 654 insertions(+)
create mode 100644 drivers/fsi/fsi-occ.c
create mode 100644 include/linux/fsi-occ.h

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index 24f84a9..322cec3 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -39,4 +39,14 @@ config FSI_SBEFIFO
a pipe-like FSI device for communicating with the self boot engine
(SBE) on POWER processors.

+config FSI_OCC
+ tristate "OCC SBEFIFO client device driver"
+ depends on FSI_SBEFIFO
+ ---help---
+ This option enables an SBEFIFO based On-Chip Controller (OCC) device
+ driver. The OCC is a device embedded on a POWER processor that collects
+ and aggregates sensor data from the processor and system. The OCC can
+ provide the raw sensor data as well as perform thermal and power
+ management on the system.
+
endif
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index 851182e..75fdc6d 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
+obj-$(CONFIG_FSI_OCC) += fsi-occ.o
diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
new file mode 100644
index 0000000..bba67a6
--- /dev/null
+++ b/drivers/fsi/fsi-occ.c
@@ -0,0 +1,609 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * On-Chip Controller Driver
+ *
+ * Copyright (C) IBM Corporation 2018
+ *
+ * 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.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/fsi-sbefifo.h>
+#include <linux/gfp.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/fsi-occ.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#define OCC_SRAM_BYTES 4096
+#define OCC_CMD_DATA_BYTES 4090
+#define OCC_RESP_DATA_BYTES 4089
+
+#define OCC_SRAM_CMD_ADDR 0xFFFBE000
+#define OCC_SRAM_RSP_ADDR 0xFFFBF000
+
+/*
+ * Assume we don't have much FFDC, if we do we'll overflow and
+ * fail the command. This needs to be big enough for simple
+ * commands as well.
+ */
+#define OCC_SBE_STATUS_WORDS 32
+
+#define OCC_TIMEOUT_MS 1000
+#define OCC_CMD_IN_PRG_WAIT_MS 50
+
+struct occ {
+ struct device *dev;
+ struct device *sbefifo;
+ char name[32];
+ int idx;
+ struct miscdevice mdev;
+ struct mutex occ_lock;
+};
+
+#define to_occ(x) container_of((x), struct occ, mdev)
+
+struct occ_response {
+ u8 seq_no;
+ u8 cmd_type;
+ u8 return_status;
+ __be16 data_length;
+ u8 data[OCC_RESP_DATA_BYTES + 2]; /* two bytes checksum */
+} __packed;
+
+struct occ_client {
+ struct occ *occ;
+ struct mutex lock;
+ size_t data_size;
+ size_t read_offset;
+ u8 *buffer;
+};
+
+#define to_client(x) container_of((x), struct occ_client, xfr)
+
+static DEFINE_IDA(occ_ida);
+
+static int occ_open(struct inode *inode, struct file *file)
+{
+ struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
+ struct miscdevice *mdev = file->private_data;
+ struct occ *occ = to_occ(mdev);
+
+ if (!client)
+ return -ENOMEM;
+
+ client->buffer = (u8 *)__get_free_page(GFP_KERNEL);
+ if (!client->buffer) {
+ kfree(client);
+ return -ENOMEM;
+ }
+
+ client->occ = occ;
+ mutex_init(&client->lock);
+ file->private_data = client;
+
+ /* We allocate a 1-page buffer, make sure it all fits */
+ BUILD_BUG_ON((OCC_CMD_DATA_BYTES + 3) > PAGE_SIZE);
+ BUILD_BUG_ON((OCC_RESP_DATA_BYTES + 7) > PAGE_SIZE);
+
+ return 0;
+}
+
+static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
+ loff_t *offset)
+{
+ struct occ_client *client = file->private_data;
+ ssize_t rc = 0;
+
+ if (!client)
+ return -ENODEV;
+
+ if (len > OCC_SRAM_BYTES)
+ return -EINVAL;
+
+ mutex_lock(&client->lock);
+
+ /* This should not be possible ... */
+ if (WARN_ON_ONCE(client->read_offset > client->data_size)) {
+ rc = -EIO;
+ goto done;
+ }
+
+ /* Grab how much data we have to read */
+ rc = min(len, client->data_size - client->read_offset);
+ if (copy_to_user(buf, client->buffer + client->read_offset, rc))
+ rc = -EFAULT;
+ else
+ client->read_offset += rc;
+
+ done:
+ mutex_unlock(&client->lock);
+
+ return rc;
+}
+
+static ssize_t occ_write(struct file *file, const char __user *buf,
+ size_t len, loff_t *offset)
+{
+ struct occ_client *client = file->private_data;
+ size_t rlen, data_length;
+ u16 checksum = 0;
+ ssize_t rc, i;
+ u8 *cmd;
+
+ if (!client)
+ return -ENODEV;
+
+ if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
+ return -EINVAL;
+
+ mutex_lock(&client->lock);
+
+ /* Construct the command */
+ cmd = client->buffer;
+
+ /* Sequence number (we could increment and compare with response) */
+ cmd[0] = 1;
+
+ /*
+ * Copy the user command (assume user data follows the occ command
+ * format)
+ * byte 0: command type
+ * bytes 1-2: data length (msb first)
+ * bytes 3-n: data
+ */
+ if (copy_from_user(&cmd[1], buf, len)) {
+ rc = -EFAULT;
+ goto done;
+ }
+
+ /* Extract data length */
+ data_length = (cmd[2] << 8) + cmd[3];
+ if (data_length > OCC_CMD_DATA_BYTES) {
+ rc = -EINVAL;
+ goto done;
+ }
+
+ /* Calculate checksum */
+ for (i = 0; i < data_length + 4; ++i)
+ checksum += cmd[i];
+
+ cmd[data_length + 4] = checksum >> 8;
+ cmd[data_length + 5] = checksum & 0xFF;
+
+ /* Submit command */
+ rlen = PAGE_SIZE;
+ rc = fsi_occ_submit(client->occ->dev, cmd, data_length + 6, cmd,
+ &rlen);
+ if (rc)
+ goto done;
+
+ /* Set read tracking data */
+ client->data_size = rlen;
+ client->read_offset = 0;
+
+ /* Done */
+ rc = len;
+
+ done:
+ mutex_unlock(&client->lock);
+
+ return rc;
+}
+
+static int occ_release(struct inode *inode, struct file *file)
+{
+ struct occ_client *client = file->private_data;
+
+ free_page((unsigned long)client->buffer);
+ kfree(client);
+
+ return 0;
+}
+
+static const struct file_operations occ_fops = {
+ .owner = THIS_MODULE,
+ .open = occ_open,
+ .read = occ_read,
+ .write = occ_write,
+ .release = occ_release,
+};
+
+static int occ_verify_checksum(struct occ_response *resp, u16 data_length)
+{
+ /* Fetch the two bytes after the data for the checksum. */
+ u16 checksum_resp = get_unaligned_be16(&resp->data[data_length]);
+ u16 checksum;
+ u16 i;
+
+ checksum = resp->seq_no;
+ checksum += resp->cmd_type;
+ checksum += resp->return_status;
+ checksum += (data_length >> 8) + (data_length & 0xFF);
+
+ for (i = 0; i < data_length; ++i)
+ checksum += resp->data[i];
+
+ if (checksum != checksum_resp)
+ return -EBADMSG;
+
+ return 0;
+}
+
+static int occ_getsram(struct occ *occ, u32 address, void *data, ssize_t len)
+{
+ u32 data_len = ((len + 7) / 8) * 8; /* must be multiples of 8 B */
+ size_t resp_len, resp_data_len;
+ __be32 *resp, cmd[5];
+ int rc;
+
+ /*
+ * Magic sequence to do SBE getsram command. SBE will fetch data from
+ * specified SRAM address.
+ */
+ cmd[0] = cpu_to_be32(0x5);
+ cmd[1] = cpu_to_be32(SBEFIFO_CMD_GET_OCC_SRAM);
+ cmd[2] = cpu_to_be32(1);
+ cmd[3] = cpu_to_be32(address);
+ cmd[4] = cpu_to_be32(data_len);
+
+ resp_len = (data_len >> 2) + OCC_SBE_STATUS_WORDS;
+ resp = kzalloc(resp_len << 2, GFP_KERNEL);
+ if (!resp)
+ return -ENOMEM;
+
+ rc = sbefifo_submit(occ->sbefifo, cmd, 5, resp, &resp_len);
+ if (rc)
+ goto free;
+
+ rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_GET_OCC_SRAM,
+ resp, resp_len, &resp_len);
+ if (rc)
+ goto free;
+
+ resp_data_len = be32_to_cpu(resp[resp_len - 1]);
+ if (resp_data_len != data_len) {
+ dev_err(occ->dev, "SRAM read expected %d bytes got %zd\n",
+ data_len, resp_data_len);
+ rc = -EBADMSG;
+ } else {
+ memcpy(data, resp, len);
+ }
+
+free:
+ /* Convert positive SBEI status */
+ if (rc > 0) {
+ dev_err(occ->dev, "SRAM read returned failure status: %08x\n",
+ rc);
+ rc = -EBADMSG;
+ }
+
+ kfree(resp);
+ return rc;
+}
+
+static int occ_putsram(struct occ *occ, u32 address, const void *data,
+ ssize_t len)
+{
+ size_t cmd_len, buf_len, resp_len, resp_data_len;
+ u32 data_len = ((len + 7) / 8) * 8; /* must be multiples of 8 B */
+ __be32 *buf;
+ int rc;
+
+ /*
+ * We use the same buffer for command and response, make
+ * sure it's big enough
+ */
+ resp_len = OCC_SBE_STATUS_WORDS;
+ cmd_len = (data_len >> 2) + 5;
+ buf_len = max(cmd_len, resp_len);
+ buf = kzalloc(buf_len << 2, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /*
+ * Magic sequence to do SBE putsram command. SBE will transfer
+ * data to specified SRAM address.
+ */
+ buf[0] = cpu_to_be32(cmd_len);
+ buf[1] = cpu_to_be32(SBEFIFO_CMD_PUT_OCC_SRAM);
+ buf[2] = cpu_to_be32(1);
+ buf[3] = cpu_to_be32(address);
+ buf[4] = cpu_to_be32(data_len);
+
+ memcpy(&buf[5], data, len);
+
+ rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len);
+ if (rc)
+ goto free;
+
+ rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
+ buf, resp_len, &resp_len);
+ if (rc)
+ goto free;
+
+ if (resp_len != 1) {
+ dev_err(occ->dev, "SRAM write response length invalid: %zd\n",
+ resp_len);
+ rc = -EBADMSG;
+ } else {
+ resp_data_len = be32_to_cpu(buf[0]);
+ if (resp_data_len != data_len) {
+ dev_err(occ->dev,
+ "SRAM write expected %d bytes got %zd\n",
+ data_len, resp_data_len);
+ rc = -EBADMSG;
+ }
+ }
+
+free:
+ /* Convert positive SBEI status */
+ if (rc > 0) {
+ dev_err(occ->dev, "SRAM write returned failure status: %08x\n",
+ rc);
+ rc = -EBADMSG;
+ }
+
+ kfree(buf);
+ return rc;
+}
+
+static int occ_trigger_attn(struct occ *occ)
+{
+ __be32 buf[OCC_SBE_STATUS_WORDS];
+ size_t resp_len, resp_data_len;
+ int rc;
+
+ BUILD_BUG_ON(OCC_SBE_STATUS_WORDS < 7);
+ resp_len = OCC_SBE_STATUS_WORDS;
+
+ buf[0] = cpu_to_be32(0x5 + 0x2); /* Chip-op length in words */
+ buf[1] = cpu_to_be32(SBEFIFO_CMD_PUT_OCC_SRAM);
+ buf[2] = cpu_to_be32(0x3); /* Mode: Circular */
+ buf[3] = cpu_to_be32(0x0); /* Address: ignore in mode 3 */
+ buf[4] = cpu_to_be32(0x8); /* Data length in bytes */
+ buf[5] = cpu_to_be32(0x20010000); /* Trigger OCC attention */
+ buf[6] = 0;
+
+ rc = sbefifo_submit(occ->sbefifo, buf, 7, buf, &resp_len);
+ if (rc)
+ goto error;
+
+ rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
+ buf, resp_len, &resp_len);
+ if (rc)
+ goto error;
+
+ if (resp_len != 1) {
+ dev_err(occ->dev, "SRAM attn response length invalid: %zd\n",
+ resp_len);
+ rc = -EBADMSG;
+ } else {
+ resp_data_len = be32_to_cpu(buf[0]);
+ if (resp_data_len != 8) {
+ dev_err(occ->dev,
+ "SRAM attn expected 8 bytes got %zd\n",
+ resp_data_len);
+ rc = -EBADMSG;
+ }
+ }
+
+ error:
+ /* Convert positive SBEI status */
+ if (rc > 0) {
+ dev_err(occ->dev, "SRAM attn returned failure status: %08x\n",
+ rc);
+ rc = -EBADMSG;
+ }
+
+ return rc;
+}
+
+int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
+ void *response, size_t *resp_len)
+{
+ const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
+ const unsigned long wait_time =
+ msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_response *resp = response;
+ u16 resp_data_length;
+ unsigned long start;
+ int rc;
+
+ if (!occ)
+ return -ENODEV;
+
+ if (*resp_len < 7) {
+ dev_dbg(dev, "Bad resplen %zd\n", *resp_len);
+ return -EINVAL;
+ }
+
+ mutex_lock(&occ->occ_lock);
+
+ rc = occ_putsram(occ, OCC_SRAM_CMD_ADDR, request, req_len);
+ if (rc)
+ goto done;
+
+ rc = occ_trigger_attn(occ);
+ if (rc)
+ goto done;
+
+ /* Read occ response header */
+ start = jiffies;
+ do {
+ rc = occ_getsram(occ, OCC_SRAM_RSP_ADDR, resp, 8);
+ if (rc)
+ goto done;
+
+ if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
+ rc = -ETIMEDOUT;
+
+ if (time_after(jiffies, start + timeout))
+ break;
+
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ schedule_timeout(wait_time);
+ }
+ } while (rc);
+
+ /* Extract size of response data */
+ resp_data_length = get_unaligned_be16(&resp->data_length);
+
+ /* Message size is data length + 5 bytes header + 2 bytes checksum */
+ if ((resp_data_length + 7) > *resp_len) {
+ rc = -EMSGSIZE;
+ goto done;
+ }
+
+ dev_dbg(dev, "resp_status=%02x resp_data_len=%d\n",
+ resp->return_status, resp_data_length);
+
+ /* Grab the rest */
+ if (resp_data_length > 1) {
+ /* already got 3 bytes resp, also need 2 bytes checksum */
+ rc = occ_getsram(occ, OCC_SRAM_RSP_ADDR + 8,
+ &resp->data[3], resp_data_length - 1);
+ if (rc)
+ goto done;
+ }
+
+ *resp_len = resp_data_length + 7;
+ rc = occ_verify_checksum(resp, resp_data_length);
+
+ done:
+ mutex_unlock(&occ->occ_lock);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(fsi_occ_submit);
+
+static int occ_unregister_child(struct device *dev, void *data)
+{
+ struct platform_device *hwmon_dev = to_platform_device(dev);
+
+ platform_device_unregister(hwmon_dev);
+
+ return 0;
+}
+
+static int occ_probe(struct platform_device *pdev)
+{
+ int rc;
+ u32 reg;
+ struct occ *occ;
+ struct platform_device *hwmon_dev;
+ struct device *dev = &pdev->dev;
+ struct platform_device_info hwmon_dev_info = {
+ .parent = dev,
+ .name = "occ-hwmon",
+ };
+
+ occ = devm_kzalloc(dev, sizeof(*occ), GFP_KERNEL);
+ if (!occ)
+ return -ENOMEM;
+
+ occ->dev = dev;
+ occ->sbefifo = dev->parent;
+ mutex_init(&occ->occ_lock);
+
+ if (dev->of_node) {
+ rc = of_property_read_u32(dev->of_node, "reg", &reg);
+ if (!rc) {
+ /* make sure we don't have a duplicate from dts */
+ occ->idx = ida_simple_get(&occ_ida, reg, reg + 1,
+ GFP_KERNEL);
+ if (occ->idx < 0)
+ occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
+ GFP_KERNEL);
+ } else {
+ occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
+ GFP_KERNEL);
+ }
+ } else {
+ occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
+ }
+
+ platform_set_drvdata(pdev, occ);
+
+ snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
+ occ->mdev.fops = &occ_fops;
+ occ->mdev.minor = MISC_DYNAMIC_MINOR;
+ occ->mdev.name = occ->name;
+ occ->mdev.parent = dev;
+
+ rc = misc_register(&occ->mdev);
+ if (rc) {
+ dev_err(dev, "failed to register miscdevice: %d\n", rc);
+ ida_simple_remove(&occ_ida, occ->idx);
+ return rc;
+ }
+
+ hwmon_dev_info.id = occ->idx;
+ hwmon_dev = platform_device_register_full(&hwmon_dev_info);
+ if (!hwmon_dev)
+ dev_warn(dev, "failed to create hwmon device\n");
+
+ return 0;
+}
+
+static int occ_remove(struct platform_device *pdev)
+{
+ struct occ *occ = platform_get_drvdata(pdev);
+
+ misc_deregister(&occ->mdev);
+
+ device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
+
+ ida_simple_remove(&occ_ida, occ->idx);
+
+ return 0;
+}
+
+static const struct of_device_id occ_match[] = {
+ { .compatible = "ibm,p9-occ" },
+ { },
+};
+
+static struct platform_driver occ_driver = {
+ .driver = {
+ .name = "occ",
+ .of_match_table = occ_match,
+ },
+ .probe = occ_probe,
+ .remove = occ_remove,
+};
+
+static int occ_init(void)
+{
+ return platform_driver_register(&occ_driver);
+}
+
+static void occ_exit(void)
+{
+ platform_driver_unregister(&occ_driver);
+
+ ida_destroy(&occ_ida);
+}
+
+module_init(occ_init);
+module_exit(occ_exit);
+
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("BMC P9 OCC driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/fsi-occ.h b/include/linux/fsi-occ.h
new file mode 100644
index 0000000..e28cf9a
--- /dev/null
+++ b/include/linux/fsi-occ.h
@@ -0,0 +1,34 @@
+/*
+ * On-Chip Controller Driver definitions and prototypes
+ *
+ * Copyright (C) IBM Corporation 2018
+ *
+ * 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.
+ */
+
+#ifndef LINUX_FSI_OCC_H
+#define LINUX_FSI_OCC_H
+
+struct device;
+
+#define OCC_RESP_CMD_IN_PRG 0xFF
+#define OCC_RESP_SUCCESS 0
+#define OCC_RESP_CMD_INVAL 0x11
+#define OCC_RESP_CMD_LEN_INVAL 0x12
+#define OCC_RESP_DATA_INVAL 0x13
+#define OCC_RESP_CHKSUM_ERR 0x14
+#define OCC_RESP_INT_ERR 0x15
+#define OCC_RESP_BAD_STATE 0x16
+#define OCC_RESP_CRIT_EXCEPT 0xE0
+#define OCC_RESP_CRIT_INIT 0xE1
+#define OCC_RESP_CRIT_WATCHDOG 0xE2
+#define OCC_RESP_CRIT_OCB 0xE3
+#define OCC_RESP_CRIT_HW 0xE4
+
+int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
+ void *response, size_t *resp_len);
+
+#endif /* LINUX_FSI_OCC_H */
--
1.8.3.1

2018-07-12 02:55:29

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 5/9] hwmon (occ): Add command transport method for P8 and P9

For the P8 OCC, add the procedure to send a command to the OCC over I2C
bus. This involves writing the OCC command registers with serial
communication operations (SCOMs) interpreted by the I2C slave. For the
P9 OCC, add a procedure to use the OCC in-kernel API to send a command
to the OCC through the SBE.

Signed-off-by: Eddie James <[email protected]>
---
drivers/hwmon/occ/p8_i2c.c | 185 ++++++++++++++++++++++++++++++++++++++++++++-
drivers/hwmon/occ/p9_sbe.c | 38 +++++++++-
2 files changed, 221 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
index 899294b..9a920db 100644
--- a/drivers/hwmon/occ/p8_i2c.c
+++ b/drivers/hwmon/occ/p8_i2c.c
@@ -12,11 +12,29 @@

#include <linux/device.h>
#include <linux/errno.h>
+#include <linux/fsi-occ.h>
#include <linux/i2c.h>
+#include <linux/jiffies.h>
#include <linux/module.h>
+#include <linux/sched.h>
+#include <asm/unaligned.h>

#include "common.h"

+#define OCC_TIMEOUT_MS 1000
+#define OCC_CMD_IN_PRG_WAIT_MS 50
+
+/* OCB (on-chip control bridge - interface to OCC) registers */
+#define OCB_DATA1 0x6B035
+#define OCB_ADDR 0x6B070
+#define OCB_DATA3 0x6B075
+
+/* OCC SRAM address space */
+#define OCC_SRAM_ADDR_CMD 0xFFFF6000
+#define OCC_SRAM_ADDR_RESP 0xFFFF7000
+
+#define OCC_DATA_ATTN 0x20010000
+
struct p8_i2c_occ {
struct occ occ;
struct i2c_client *client;
@@ -24,9 +42,174 @@ struct p8_i2c_occ {

#define to_p8_i2c_occ(x) container_of((x), struct p8_i2c_occ, occ)

+static int p8_i2c_occ_getscom(struct i2c_client *client, u32 address, u8 *data)
+{
+ ssize_t rc;
+ __be64 buf;
+ struct i2c_msg msgs[2];
+
+ /* p8 i2c slave requires shift */
+ address <<= 1;
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags & I2C_M_TEN;
+ msgs[0].len = sizeof(u32);
+ /* address is a scom address; bus-endian */
+ msgs[0].buf = (char *)&address;
+
+ /* data from OCC is big-endian */
+ msgs[1].addr = client->addr;
+ msgs[1].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
+ msgs[1].len = sizeof(u64);
+ msgs[1].buf = (char *)&buf;
+
+ rc = i2c_transfer(client->adapter, msgs, 2);
+ if (rc < 0)
+ return rc;
+
+ *(u64 *)data = be64_to_cpu(buf);
+
+ return 0;
+}
+
+static int p8_i2c_occ_putscom(struct i2c_client *client, u32 address, u8 *data)
+{
+ u32 buf[3];
+ ssize_t rc;
+
+ /* p8 i2c slave requires shift */
+ address <<= 1;
+
+ /* address is bus-endian; data passed through from user as-is */
+ buf[0] = address;
+ memcpy(&buf[1], &data[4], sizeof(u32));
+ memcpy(&buf[2], data, sizeof(u32));
+
+ rc = i2c_master_send(client, (const char *)buf, sizeof(buf));
+ if (rc < 0)
+ return rc;
+ else if (rc != sizeof(buf))
+ return -EIO;
+
+ return 0;
+}
+
+static int p8_i2c_occ_putscom_u32(struct i2c_client *client, u32 address,
+ u32 data0, u32 data1)
+{
+ u8 buf[8];
+
+ memcpy(buf, &data0, 4);
+ memcpy(buf + 4, &data1, 4);
+
+ return p8_i2c_occ_putscom(client, address, buf);
+}
+
+static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32 address,
+ u8 *data)
+{
+ __be32 data0, data1;
+
+ memcpy(&data0, data, 4);
+ memcpy(&data1, data + 4, 4);
+
+ return p8_i2c_occ_putscom_u32(client, address, be32_to_cpu(data0),
+ be32_to_cpu(data1));
+}
+
static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
{
- return -EOPNOTSUPP;
+ int i, rc;
+ unsigned long start;
+ u16 data_length;
+ const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
+ const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
+ struct p8_i2c_occ *ctx = to_p8_i2c_occ(occ);
+ struct i2c_client *client = ctx->client;
+ struct occ_response *resp = &occ->resp;
+
+ start = jiffies;
+
+ /* set sram address for command */
+ rc = p8_i2c_occ_putscom_u32(client, OCB_ADDR, OCC_SRAM_ADDR_CMD, 0);
+ if (rc)
+ return rc;
+
+ /* write command (expected to already be BE), we need bus-endian... */
+ rc = p8_i2c_occ_putscom_be(client, OCB_DATA3, cmd);
+ if (rc)
+ return rc;
+
+ /* trigger OCC attention */
+ rc = p8_i2c_occ_putscom_u32(client, OCB_DATA1, OCC_DATA_ATTN, 0);
+ if (rc)
+ return rc;
+
+ do {
+ /* set sram address for response */
+ rc = p8_i2c_occ_putscom_u32(client, OCB_ADDR,
+ OCC_SRAM_ADDR_RESP, 0);
+ if (rc)
+ return rc;
+
+ rc = p8_i2c_occ_getscom(client, OCB_DATA3, (u8 *)resp);
+ if (rc)
+ return rc;
+
+ /* wait for OCC */
+ if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
+ rc = -EALREADY;
+
+ if (time_after(jiffies, start + timeout))
+ break;
+
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(wait_time);
+ }
+ } while (rc);
+
+ /* check the OCC response */
+ switch (resp->return_status) {
+ case OCC_RESP_CMD_IN_PRG:
+ rc = -ETIMEDOUT;
+ break;
+ case OCC_RESP_SUCCESS:
+ rc = 0;
+ break;
+ case OCC_RESP_CMD_INVAL:
+ case OCC_RESP_CMD_LEN_INVAL:
+ case OCC_RESP_DATA_INVAL:
+ case OCC_RESP_CHKSUM_ERR:
+ rc = -EINVAL;
+ break;
+ case OCC_RESP_INT_ERR:
+ case OCC_RESP_BAD_STATE:
+ case OCC_RESP_CRIT_EXCEPT:
+ case OCC_RESP_CRIT_INIT:
+ case OCC_RESP_CRIT_WATCHDOG:
+ case OCC_RESP_CRIT_OCB:
+ case OCC_RESP_CRIT_HW:
+ rc = -EREMOTEIO;
+ break;
+ default:
+ rc = -EPROTO;
+ }
+
+ if (rc < 0)
+ return rc;
+
+ data_length = get_unaligned_be16(&resp->data_length);
+ if (data_length > OCC_RESP_DATA_BYTES)
+ return -EMSGSIZE;
+
+ /* fetch the rest of the response data */
+ for (i = 8; i < data_length + 7; i += 8) {
+ rc = p8_i2c_occ_getscom(client, OCB_DATA3, ((u8 *)resp) + i);
+ if (rc)
+ return rc;
+ }
+
+ return 0;
}

static int p8_i2c_occ_probe(struct i2c_client *client,
diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 7c026a5..fb32788 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -12,6 +12,7 @@

#include <linux/device.h>
#include <linux/errno.h>
+#include <linux/fsi-occ.h>
#include <linux/module.h>
#include <linux/platform_device.h>

@@ -26,7 +27,42 @@ struct p9_sbe_occ {

static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
{
- return -EOPNOTSUPP;
+ struct occ_response *resp = &occ->resp;
+ struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
+ size_t resp_len = sizeof(*resp);
+ int rc;
+
+ rc = fsi_occ_submit(ctx->sbe, cmd, 8, resp, &resp_len);
+ if (rc < 0)
+ return rc;
+
+ switch (resp->return_status) {
+ case OCC_RESP_CMD_IN_PRG:
+ rc = -ETIMEDOUT;
+ break;
+ case OCC_RESP_SUCCESS:
+ rc = 0;
+ break;
+ case OCC_RESP_CMD_INVAL:
+ case OCC_RESP_CMD_LEN_INVAL:
+ case OCC_RESP_DATA_INVAL:
+ case OCC_RESP_CHKSUM_ERR:
+ rc = -EINVAL;
+ break;
+ case OCC_RESP_INT_ERR:
+ case OCC_RESP_BAD_STATE:
+ case OCC_RESP_CRIT_EXCEPT:
+ case OCC_RESP_CRIT_INIT:
+ case OCC_RESP_CRIT_WATCHDOG:
+ case OCC_RESP_CRIT_OCB:
+ case OCC_RESP_CRIT_HW:
+ rc = -EREMOTEIO;
+ break;
+ default:
+ rc = -EPROTO;
+ }
+
+ return rc;
}

static int p9_sbe_occ_probe(struct platform_device *pdev)
--
1.8.3.1

2018-07-12 02:55:40

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 6/9] hwmon (occ): Parse OCC poll response

Add method to parse the response from the OCC poll command. This only
needs to be done during probe(), since the OCC shouldn't change the
number or format of sensors while it's running. The parsed response
allows quick access to sensor data, as well as information on the
number and version of sensors, which we need to instantiate hwmon
attributes.

Signed-off-by: Eddie James <[email protected]>
---
drivers/hwmon/occ/common.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/common.h | 55 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 116 insertions(+)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 1fd3453..73f62aa 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -11,6 +11,7 @@
*/

#include <linux/device.h>
+#include <linux/kernel.h>

#include "common.h"

@@ -32,6 +33,64 @@ static int occ_poll(struct occ *occ)
return occ->send_cmd(occ, cmd);
}

+/* only need to do this once at startup, as OCC won't change sensors on us */
+static void occ_parse_poll_response(struct occ *occ)
+{
+ unsigned int i, old_offset, offset = 0, size = 0;
+ struct occ_sensor *sensor;
+ struct occ_sensors *sensors = &occ->sensors;
+ struct occ_response *resp = &occ->resp;
+ struct occ_poll_response *poll =
+ (struct occ_poll_response *)&resp->data[0];
+ struct occ_poll_response_header *header = &poll->header;
+ struct occ_sensor_data_block *block = &poll->block;
+
+ dev_info(occ->bus_dev, "OCC found, code level: %.16s\n",
+ header->occ_code_level);
+
+ for (i = 0; i < header->num_sensor_data_blocks; ++i) {
+ block = (struct occ_sensor_data_block *)((u8 *)block + offset);
+ old_offset = offset;
+ offset = (block->header.num_sensors *
+ block->header.sensor_length) + sizeof(block->header);
+ size += offset;
+
+ /* validate all the length/size fields */
+ if ((size + sizeof(*header)) >= OCC_RESP_DATA_BYTES) {
+ dev_warn(occ->bus_dev, "exceeded response buffer\n");
+ return;
+ }
+
+ dev_dbg(occ->bus_dev, " %04x..%04x: %.4s (%d sensors)\n",
+ old_offset, offset - 1, block->header.eye_catcher,
+ block->header.num_sensors);
+
+ /* match sensor block type */
+ if (strncmp(block->header.eye_catcher, "TEMP", 4) == 0)
+ sensor = &sensors->temp;
+ else if (strncmp(block->header.eye_catcher, "FREQ", 4) == 0)
+ sensor = &sensors->freq;
+ else if (strncmp(block->header.eye_catcher, "POWR", 4) == 0)
+ sensor = &sensors->power;
+ else if (strncmp(block->header.eye_catcher, "CAPS", 4) == 0)
+ sensor = &sensors->caps;
+ else if (strncmp(block->header.eye_catcher, "EXTN", 4) == 0)
+ sensor = &sensors->extended;
+ else {
+ dev_warn(occ->bus_dev, "sensor not supported %.4s\n",
+ block->header.eye_catcher);
+ continue;
+ }
+
+ sensor->num_sensors = block->header.num_sensors;
+ sensor->version = block->header.sensor_format;
+ sensor->data = &block->data;
+ }
+
+ dev_dbg(occ->bus_dev, "Max resp size: %u+%zd=%zd\n", size,
+ sizeof(*header), size + sizeof(*header));
+}
+
int occ_setup(struct occ *occ, const char *name)
{
int rc;
@@ -46,5 +105,7 @@ int occ_setup(struct occ *occ, const char *name)
return rc;
}

+ occ_parse_poll_response(occ);
+
return 0;
}
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index 775b421..cfcd353 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -29,10 +29,65 @@ struct occ_response {
__be16 checksum;
} __packed;

+struct occ_sensor_data_block_header {
+ u8 eye_catcher[4];
+ u8 reserved;
+ u8 sensor_format;
+ u8 sensor_length;
+ u8 num_sensors;
+} __packed;
+
+struct occ_sensor_data_block {
+ struct occ_sensor_data_block_header header;
+ u32 data;
+} __packed;
+
+struct occ_poll_response_header {
+ u8 status;
+ u8 ext_status;
+ u8 occs_present;
+ u8 config_data;
+ u8 occ_state;
+ u8 mode;
+ u8 ips_status;
+ u8 error_log_id;
+ __be32 error_log_start_address;
+ __be16 error_log_length;
+ u16 reserved;
+ u8 occ_code_level[16];
+ u8 eye_catcher[6];
+ u8 num_sensor_data_blocks;
+ u8 sensor_data_block_header_version;
+} __packed;
+
+struct occ_poll_response {
+ struct occ_poll_response_header header;
+ struct occ_sensor_data_block block;
+} __packed;
+
+struct occ_sensor {
+ u8 num_sensors;
+ u8 version;
+ void *data; /* pointer to sensor data start within response */
+};
+
+/*
+ * OCC only provides one sensor data block of each type, but any number of
+ * sensors within that block.
+ */
+struct occ_sensors {
+ struct occ_sensor temp;
+ struct occ_sensor freq;
+ struct occ_sensor power;
+ struct occ_sensor caps;
+ struct occ_sensor extended;
+};
+
struct occ {
struct device *bus_dev;

struct occ_response resp;
+ struct occ_sensors sensors;

u8 poll_cmd_data; /* to perform OCC poll command */
int (*send_cmd)(struct occ *occ, u8 *cmd);
--
1.8.3.1

2018-07-12 02:55:51

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 3/9] dt-bindings: i2c: Add P8 OCC hwmon device documentation

Document the bindings for I2C-based OCC hwmon device.

Signed-off-by: Eddie James <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 ++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt

diff --git a/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt
new file mode 100644
index 0000000..5dc5d2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt
@@ -0,0 +1,25 @@
+Device-tree bindings for I2C-based On-Chip Controller hwmon device
+------------------------------------------------------------------
+
+Required properties:
+ - compatible = "ibm,p8-occ-hwmon";
+ - reg = <I2C address>; : I2C bus address
+
+Examples:
+
+ i2c-bus@100 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clock-frequency = <100000>;
+ < more properties >
+
+ occ-hwmon@1 {
+ compatible = "ibm,p8-occ-hwmon";
+ reg = <0x50>;
+ };
+
+ occ-hwmon@2 {
+ compatible = "ibm,p8-occ-hwmon";
+ reg = <0x51>;
+ };
+ };
--
1.8.3.1

2018-07-12 02:55:55

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 2/9] Documentation: hwmon: Add OCC documentation

Document the hwmon interface for the OCC.

Signed-off-by: Eddie James <[email protected]>
---
Documentation/hwmon/occ | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 73 insertions(+)
create mode 100644 Documentation/hwmon/occ

diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
new file mode 100644
index 0000000..465fa1a
--- /dev/null
+++ b/Documentation/hwmon/occ
@@ -0,0 +1,73 @@
+Kernel driver occ-hwmon
+=======================
+
+Supported chips:
+ * POWER8
+ * POWER9
+
+Author: Eddie James <[email protected]>
+
+Description
+-----------
+
+This driver supports hardware monitoring for the On-Chip Controller (OCC)
+embedded on POWER processors. The OCC is a device that collects and aggregates
+sensor data from the processor and the system. The OCC can provide the raw
+sensor data as well as perform thermal and power management on the system.
+
+The P8 version of this driver is a client driver of I2C. It may be probed
+manually if an "ibm,p8-occ-hwmon" compatible device is found under the
+appropriate I2C bus node in the device-tree.
+
+The P9 version of this driver is a client driver of the FSI-based OCC driver.
+It will be probed automatically by the FSI-based OCC driver.
+
+Sysfs entries
+-------------
+
+The following attributes are supported. All attributes are read-only unless
+specified.
+
+temp[1-n]_label OCC sensor id.
+temp[1-n]_input Measured temperature in millidegrees C.
+[with temperature sensor version 2+]
+ temp[1-n]_fru_type Given FRU (Field Replaceable Unit) type.
+ temp[1-n]_fault Temperature sensor fault.
+
+freq[1-n]_label OCC sensor id.
+freq[1-n]_input Measured frequency.
+
+power[1-n]_label OCC sensor id.
+power[1-n]_input Measured power in microwatts.
+power[1-n]_update_tag Number of 250us samples represented in accumulator.
+power[1-n]_accumulator Accumulation of 250us power readings.
+[with power sensor version 2+]
+ power[1-n]_function_id Identifies what the power reading is for.
+ power[1-n]_apss_channel Indicates APSS channel.
+
+[power version 0xa0 only]
+power1_id OCC sensor id.
+power[1-n]_label Sensor type, "system", "proc", "vdd", or "vdn".
+power[1-n]_input Most recent power reading in microwatts.
+power[1-n]_update_tag Number of samples in the accumulator.
+power[1-n]_accumulator Accumulation of power readings.
+[with sensor type "system" and "proc" only]
+ power[1-n]_update_time Time in us that the power value is read.
+
+caps1_current Current OCC power cap in watts.
+caps1_reading Current system output power in watts.
+caps1_norm Power cap without redundant power.
+caps1_max Maximum power cap.
+[caps version 1 and 2 only]
+ caps1_min Minimum power cap.
+[caps version 3+]
+ caps1_min_hard Hard minimum cap that can be set and held.
+ caps1_min_soft Soft minimum cap below hard, not guaranteed.
+caps1_user The powercap specified by the user. Will be 0 if no
+ user powercap exists. This attribute is read-write.
+[caps version 1+]
+ caps1_user_source Indicates how the user power limit was set.
+
+extn[1-n]_label ASCII id or sensor id.
+extn[1-n]_flags Indicates type of label attribute.
+extn[1-n]_input Data.
--
1.8.3.1

2018-07-12 02:56:03

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 4/9] hwmon: Add On-Chip Controller (OCC) hwmon driver

2018-07-12 02:56:26

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 8/9] hwmon (occ): Add sensor attributes and register hwmon device

Setup the sensor attributes for every OCC sensor found by the first poll
response. Register the attributes with hwmon.

Signed-off-by: Eddie James <[email protected]>
---
drivers/hwmon/occ/common.c | 452 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/common.h | 16 ++
2 files changed, 468 insertions(+)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 1719536..da55919 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -11,10 +11,12 @@
*/

#include <linux/device.h>
+#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/mutex.h>
+#include <linux/sysfs.h>
#include <asm/unaligned.h>

#include "common.h"
@@ -678,6 +680,439 @@ static ssize_t occ_show_extended(struct device *dev,
return rc;
}

+/*
+ * Some helper macros to make it easier to define an occ_attribute. Since these
+ * are dynamically allocated, we shouldn't use the existing kernel macros which
+ * stringify the name argument.
+ */
+#define ATTR_OCC(_name, _mode, _show, _store) { \
+ .attr = { \
+ .name = _name, \
+ .mode = VERIFY_OCTAL_PERMISSIONS(_mode), \
+ }, \
+ .show = _show, \
+ .store = _store, \
+}
+
+#define SENSOR_ATTR_OCC(_name, _mode, _show, _store, _nr, _index) { \
+ .dev_attr = ATTR_OCC(_name, _mode, _show, _store), \
+ .index = _index, \
+ .nr = _nr, \
+}
+
+#define OCC_INIT_ATTR(_name, _mode, _show, _store, _nr, _index) \
+ ((struct sensor_device_attribute_2) \
+ SENSOR_ATTR_OCC(_name, _mode, _show, _store, _nr, _index))
+
+/*
+ * Allocate and instatiate sensor_device_attribute_2s. It's most efficient to
+ * use our own instead of the built-in hwmon attribute types.
+ */
+static int occ_setup_sensor_attrs(struct occ *occ)
+{
+ unsigned int i, s, num_attrs = 0;
+ struct device *dev = occ->bus_dev;
+ struct occ_sensors *sensors = &occ->sensors;
+ struct occ_attribute *attr;
+ struct temp_sensor_2 *temp;
+ ssize_t (*show_temp)(struct device *, struct device_attribute *,
+ char *) = occ_show_temp_1;
+ ssize_t (*show_freq)(struct device *, struct device_attribute *,
+ char *) = occ_show_freq_1;
+ ssize_t (*show_power)(struct device *, struct device_attribute *,
+ char *) = occ_show_power_1;
+ ssize_t (*show_caps)(struct device *, struct device_attribute *,
+ char *) = occ_show_caps_1;
+
+ switch (sensors->temp.version) {
+ case 1:
+ num_attrs += (sensors->temp.num_sensors * 2);
+ break;
+ case 2:
+ num_attrs += (sensors->temp.num_sensors * 4);
+ show_temp = occ_show_temp_2;
+ break;
+ default:
+ sensors->temp.num_sensors = 0;
+ }
+
+ switch (sensors->freq.version) {
+ case 2:
+ show_freq = occ_show_freq_2;
+ /* fall through */
+ case 1:
+ num_attrs += (sensors->freq.num_sensors * 2);
+ break;
+ default:
+ sensors->freq.num_sensors = 0;
+ }
+
+ switch (sensors->power.version) {
+ case 1:
+ num_attrs += (sensors->power.num_sensors * 4);
+ break;
+ case 2:
+ num_attrs += (sensors->power.num_sensors * 6);
+ show_power = occ_show_power_2;
+ break;
+ case 0xA0:
+ num_attrs += (sensors->power.num_sensors * 19);
+ show_power = occ_show_power_a0;
+ break;
+ default:
+ sensors->power.num_sensors = 0;
+ }
+
+ switch (sensors->caps.version) {
+ case 1:
+ num_attrs += (sensors->caps.num_sensors * 6);
+ break;
+ case 2:
+ num_attrs += (sensors->caps.num_sensors * 7);
+ show_caps = occ_show_caps_2;
+ break;
+ case 3:
+ num_attrs += (sensors->caps.num_sensors * 8);
+ show_caps = occ_show_caps_3;
+ break;
+ default:
+ sensors->caps.num_sensors = 0;
+ }
+
+ switch (sensors->extended.version) {
+ case 1:
+ num_attrs += (sensors->extended.num_sensors * 3);
+ break;
+ default:
+ sensors->extended.num_sensors = 0;
+ }
+
+ occ->attrs = devm_kzalloc(dev, sizeof(*occ->attrs) * num_attrs,
+ GFP_KERNEL);
+ if (!occ->attrs)
+ return -ENOMEM;
+
+ /* null-terminated list */
+ occ->group.attrs = devm_kzalloc(dev, sizeof(*occ->group.attrs) *
+ num_attrs + 1, GFP_KERNEL);
+ if (!occ->group.attrs)
+ return -ENOMEM;
+
+ attr = occ->attrs;
+
+ for (i = 0; i < sensors->temp.num_sensors; ++i) {
+ s = i + 1;
+ temp = ((struct temp_sensor_2 *)sensors->temp.data) + i;
+
+ snprintf(attr->name, sizeof(attr->name), "temp%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_temp, NULL,
+ 0, i);
+ attr++;
+
+ if (sensors->temp.version > 1 &&
+ temp->fru_type == OCC_FRU_TYPE_VRM) {
+ snprintf(attr->name, sizeof(attr->name),
+ "temp%d_alarm", s);
+ } else {
+ snprintf(attr->name, sizeof(attr->name),
+ "temp%d_input", s);
+ }
+
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_temp, NULL,
+ 1, i);
+ attr++;
+
+ if (sensors->temp.version > 1) {
+ snprintf(attr->name, sizeof(attr->name),
+ "temp%d_fru_type", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_temp, NULL, 2, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "temp%d_fault", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_temp, NULL, 3, i);
+ attr++;
+ }
+ }
+
+ for (i = 0; i < sensors->freq.num_sensors; ++i) {
+ s = i + 1;
+
+ snprintf(attr->name, sizeof(attr->name), "freq%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_freq, NULL,
+ 0, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name), "freq%d_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_freq, NULL,
+ 1, i);
+ attr++;
+ }
+
+ if (sensors->power.version == 0xA0) {
+ /* Special case for many-attribute power sensor. Split it into
+ * a sensor number per power type, emulating several sensors.
+ */
+ for (i = 0; i < sensors->power.num_sensors; ++i) {
+ s = (i * 4) + 1;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_id", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 0, i);
+ attr++;
+
+ /* system power attributes */
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 1, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_update_time", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 2, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 3, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 4, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 5, i);
+ attr++;
+
+ s++;
+
+ /* processor power attributes */
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 6, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_update_time", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 7, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 8, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 9, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 10, i);
+ attr++;
+
+ s++;
+
+ /* vdd power attributes */
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 11, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 12, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 13, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 14, i);
+ attr++;
+
+ s++;
+
+ /* vdn power attributes */
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 15, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 16, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 17, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 18, i);
+ attr++;
+ }
+ } else {
+ for (i = 0; i < sensors->power.num_sensors; ++i) {
+ s = i + 1;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 0, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 1, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 2, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 3, i);
+ attr++;
+
+ if (sensors->power.version > 1) {
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_function_id", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL,
+ 4, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_apss_channel", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL,
+ 5, i);
+ attr++;
+ }
+ }
+ }
+
+ for (i = 0; i < sensors->caps.num_sensors; ++i) {
+ s = i + 1;
+
+ snprintf(attr->name, sizeof(attr->name), "caps%d_current", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_caps, NULL,
+ 0, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name), "caps%d_reading", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_caps, NULL,
+ 1, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name), "caps%d_norm", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_caps, NULL,
+ 2, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name), "caps%d_max", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_caps, NULL,
+ 3, i);
+ attr++;
+
+ if (sensors->caps.version > 2) {
+ snprintf(attr->name, sizeof(attr->name),
+ "caps%d_min_hard", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_caps, NULL, 4, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "caps%d_min_soft", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_caps, NULL, 7, i);
+ attr++;
+ } else {
+ snprintf(attr->name, sizeof(attr->name), "caps%d_min",
+ s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_caps, NULL, 4, i);
+ attr++;
+ }
+
+ snprintf(attr->name, sizeof(attr->name), "caps%d_user", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0644, show_caps,
+ occ_store_caps_user, 5, i);
+ attr++;
+
+ if (sensors->caps.version > 1) {
+ snprintf(attr->name, sizeof(attr->name),
+ "caps%d_user_source", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_caps, NULL, 6, i);
+ attr++;
+ }
+ }
+
+ for (i = 0; i < sensors->extended.num_sensors; ++i) {
+ s = i + 1;
+
+ snprintf(attr->name, sizeof(attr->name), "extn%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ occ_show_extended, NULL, 0, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name), "extn%d_flags", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ occ_show_extended, NULL, 1, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name), "extn%d_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ occ_show_extended, NULL, 2, i);
+ attr++;
+ }
+
+ /* put the sensors in the group */
+ for (i = 0; i < num_attrs; ++i) {
+ sysfs_attr_init(&occ->attrs[i].sensor.dev_attr.attr);
+ occ->group.attrs[i] = &occ->attrs[i].sensor.dev_attr.attr;
+ }
+
+ return 0;
+}
+
/* only need to do this once at startup, as OCC won't change sensors on us */
static void occ_parse_poll_response(struct occ *occ)
{
@@ -741,6 +1176,7 @@ int occ_setup(struct occ *occ, const char *name)
int rc;

mutex_init(&occ->lock);
+ occ->groups[0] = &occ->group;

/* no need to lock */
rc = occ_poll(occ);
@@ -755,5 +1191,21 @@ int occ_setup(struct occ *occ, const char *name)

occ_parse_poll_response(occ);

+ rc = occ_setup_sensor_attrs(occ);
+ if (rc) {
+ dev_err(occ->bus_dev, "failed to setup sensor attrs: %d\n",
+ rc);
+ return rc;
+ }
+
+ occ->hwmon = devm_hwmon_device_register_with_groups(occ->bus_dev, name,
+ occ, occ->groups);
+ if (IS_ERR(occ->hwmon)) {
+ rc = PTR_ERR(occ->hwmon);
+ dev_err(occ->bus_dev, "failed to register hwmon device: %d\n",
+ rc);
+ return rc;
+ }
+
return 0;
}
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index f697adb..2c1c06e 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -12,7 +12,9 @@
#ifndef OCC_COMMON_H
#define OCC_COMMON_H

+#include <linux/hwmon-sysfs.h>
#include <linux/mutex.h>
+#include <linux/sysfs.h>

struct device;

@@ -85,6 +87,15 @@ struct occ_sensors {
struct occ_sensor extended;
};

+/*
+ * Use our own attribute struct so we can dynamically allocate space for the
+ * name.
+ */
+struct occ_attribute {
+ char name[32];
+ struct sensor_device_attribute_2 sensor;
+};
+
struct occ {
struct device *bus_dev;

@@ -96,6 +107,11 @@ struct occ {

unsigned long last_update;
struct mutex lock; /* lock OCC access */
+
+ struct device *hwmon;
+ struct occ_attribute *attrs;
+ struct attribute_group group;
+ const struct attribute_group *groups[2];
};

int occ_setup(struct occ *occ, const char *name);
--
1.8.3.1

2018-07-12 02:56:40

by Eddie James

[permalink] [raw]

Subject: [PATCH v4 7/9] hwmon (occ): Add sensor types and versions

Add structures to define all sensor types and versions. Add sysfs show
and store functions for each sensor type. Add a method to construct the
"set user power cap" command and send it to the OCC. Add rate limit to
polling the OCC (in case user-space reads our hwmon entries rapidly).

Signed-off-by: Eddie James <[email protected]>
---
drivers/hwmon/occ/common.c | 648 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/common.h | 5 +
2 files changed, 653 insertions(+)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 73f62aa..1719536 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -11,10 +11,119 @@
*/

#include <linux/device.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <asm/unaligned.h>

#include "common.h"

+#define OCC_UPDATE_FREQUENCY msecs_to_jiffies(1000)
+
+#define OCC_TEMP_SENSOR_FAULT 0xFF
+
+#define OCC_FRU_TYPE_VRM 0x3
+
+/* OCC sensor type and version definitions */
+
+struct temp_sensor_1 {
+ u16 sensor_id;
+ u16 value;
+} __packed;
+
+struct temp_sensor_2 {
+ u32 sensor_id;
+ u8 fru_type;
+ u8 value;
+} __packed;
+
+struct freq_sensor_1 {
+ u16 sensor_id;
+ u16 value;
+} __packed;
+
+struct freq_sensor_2 {
+ u32 sensor_id;
+ u16 value;
+} __packed;
+
+struct power_sensor_1 {
+ u16 sensor_id;
+ u32 update_tag;
+ u32 accumulator;
+ u16 value;
+} __packed;
+
+struct power_sensor_2 {
+ u32 sensor_id;
+ u8 function_id;
+ u8 apss_channel;
+ u16 reserved;
+ u32 update_tag;
+ u64 accumulator;
+ u16 value;
+} __packed;
+
+struct power_sensor_data {
+ u16 value;
+ u32 update_tag;
+ u64 accumulator;
+} __packed;
+
+struct power_sensor_data_and_time {
+ u16 update_time;
+ u16 value;
+ u32 update_tag;
+ u64 accumulator;
+} __packed;
+
+struct power_sensor_a0 {
+ u32 sensor_id;
+ struct power_sensor_data_and_time system;
+ u32 reserved;
+ struct power_sensor_data_and_time proc;
+ struct power_sensor_data vdd;
+ struct power_sensor_data vdn;
+} __packed;
+
+struct caps_sensor_1 {
+ u16 curr_powercap;
+ u16 curr_powerreading;
+ u16 norm_powercap;
+ u16 max_powercap;
+ u16 min_powercap;
+ u16 user_powerlimit;
+} __packed;
+
+struct caps_sensor_2 {
+ u16 curr_powercap;
+ u16 curr_powerreading;
+ u16 norm_powercap;
+ u16 max_powercap;
+ u16 min_powercap;
+ u16 user_powerlimit;
+ u8 user_powerlimit_source;
+} __packed;
+
+struct caps_sensor_3 {
+ u16 curr_powercap;
+ u16 curr_powerreading;
+ u16 norm_powercap;
+ u16 max_powercap;
+ u16 hard_min_powercap;
+ u16 soft_min_powercap;
+ u16 user_powerlimit;
+ u8 user_powerlimit_source;
+} __packed;
+
+struct extended_sensor {
+ u8 name[4];
+ u8 flags;
+ u8 reserved;
+ u8 data[6];
+} __packed;
+
static int occ_poll(struct occ *occ)
{
u16 checksum = occ->poll_cmd_data + 1;
@@ -30,9 +139,545 @@ static int occ_poll(struct occ *occ)
cmd[6] = checksum & 0xFF; /* checksum lsb */
cmd[7] = 0;

+ /* mutex should already be locked if necessary */
return occ->send_cmd(occ, cmd);
}

+static int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
+{
+ int rc;
+ u8 cmd[8];
+ u16 checksum = 0x24;
+ __be16 user_power_cap_be = cpu_to_be16(user_power_cap);
+
+ cmd[0] = 0;
+ cmd[1] = 0x22;
+ cmd[2] = 0;
+ cmd[3] = 2;
+
+ memcpy(&cmd[4], &user_power_cap_be, 2);
+
+ checksum += cmd[4] + cmd[5];
+ cmd[6] = checksum >> 8;
+ cmd[7] = checksum & 0xFF;
+
+ rc = mutex_lock_interruptible(&occ->lock);
+ if (rc)
+ return rc;
+
+ rc = occ->send_cmd(occ, cmd);
+
+ mutex_unlock(&occ->lock);
+
+ return rc;
+}
+
+static int occ_update_response(struct occ *occ)
+{
+ int rc = mutex_lock_interruptible(&occ->lock);
+
+ if (rc)
+ return rc;
+
+ /* limit the maximum rate of polling the OCC */
+ if (time_after(jiffies, occ->last_update + OCC_UPDATE_FREQUENCY)) {
+ rc = occ_poll(occ);
+ occ->last_update = jiffies;
+ }
+
+ mutex_unlock(&occ->lock);
+ return rc;
+}
+
+static ssize_t occ_show_temp_1(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u32 val = 0;
+ struct temp_sensor_1 *temp;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ temp = ((struct temp_sensor_1 *)sensors->temp.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be16(&temp->sensor_id);
+ break;
+ case 1:
+ /* millidegrees */
+ val = get_unaligned_be16(&temp->value) * 1000;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+}
+
+static ssize_t occ_show_temp_2(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u32 val = 0;
+ struct temp_sensor_2 *temp;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ temp = ((struct temp_sensor_2 *)sensors->temp.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be32(&temp->sensor_id);
+ break;
+ case 1:
+ val = temp->value;
+ if (val == OCC_TEMP_SENSOR_FAULT)
+ return -EREMOTEIO;
+
+ if (temp->fru_type != OCC_FRU_TYPE_VRM) {
+ /* sensor not ready */
+ if (val == 0)
+ return -EAGAIN;
+
+ val *= 1000; /* millidegrees */
+ }
+ break;
+ case 2:
+ val = temp->fru_type;
+ break;
+ case 3:
+ val = temp->value == OCC_TEMP_SENSOR_FAULT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+}
+
+static ssize_t occ_show_freq_1(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u16 val = 0;
+ struct freq_sensor_1 *freq;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ freq = ((struct freq_sensor_1 *)sensors->freq.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be16(&freq->sensor_id);
+ break;
+ case 1:
+ val = get_unaligned_be16(&freq->value);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+}
+
+static ssize_t occ_show_freq_2(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u32 val = 0;
+ struct freq_sensor_2 *freq;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ freq = ((struct freq_sensor_2 *)sensors->freq.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be32(&freq->sensor_id);
+ break;
+ case 1:
+ val = get_unaligned_be16(&freq->value);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+}
+
+static ssize_t occ_show_power_1(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u64 val = 0;
+ struct power_sensor_1 *power;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ power = ((struct power_sensor_1 *)sensors->power.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be16(&power->sensor_id);
+ break;
+ case 1:
+ val = get_unaligned_be32(&power->update_tag);
+ break;
+ case 2:
+ val = get_unaligned_be32(&power->accumulator);
+ break;
+ case 3:
+ /* microwatts */
+ val = get_unaligned_be16(&power->value) * 1000000ULL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%llu\n", val);
+}
+
+static ssize_t occ_show_power_2(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u64 val = 0;
+ struct power_sensor_2 *power;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ power = ((struct power_sensor_2 *)sensors->power.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be32(&power->sensor_id);
+ break;
+ case 1:
+ val = get_unaligned_be32(&power->update_tag);
+ break;
+ case 2:
+ val = get_unaligned_be64(&power->accumulator);
+ break;
+ case 3:
+ /* microwatts */
+ val = get_unaligned_be16(&power->value) * 1000000ULL;
+ break;
+ case 4:
+ val = power->function_id;
+ break;
+ case 5:
+ val = power->apss_channel;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%llu\n", val);
+}
+
+static ssize_t occ_show_power_a0(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u64 val = 0;
+ struct power_sensor_a0 *power;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ power = ((struct power_sensor_a0 *)sensors->power.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be32(&power->sensor_id);
+ break;
+ case 1:
+ return snprintf(buf, PAGE_SIZE - 1, "system\n");
+ case 2:
+ val = get_unaligned_be16(&power->system.update_time);
+ break;
+ case 3:
+ /* microwatts */
+ val = get_unaligned_be16(&power->system.value) * 1000000ULL;
+ break;
+ case 4:
+ val = get_unaligned_be32(&power->system.update_tag);
+ break;
+ case 5:
+ val = get_unaligned_be64(&power->system.accumulator);
+ break;
+ case 6:
+ return snprintf(buf, PAGE_SIZE - 1, "proc\n");
+ case 7:
+ val = get_unaligned_be16(&power->proc.update_time);
+ break;
+ case 8:
+ /* microwatts */
+ val = get_unaligned_be16(&power->proc.value) * 1000000ULL;
+ break;
+ case 9:
+ val = get_unaligned_be32(&power->proc.update_tag);
+ break;
+ case 10:
+ val = get_unaligned_be64(&power->proc.accumulator);
+ break;
+ case 11:
+ return snprintf(buf, PAGE_SIZE - 1, "vdd\n");
+ case 12:
+ /* microwatts */
+ val = get_unaligned_be16(&power->vdd.value) * 1000000ULL;
+ break;
+ case 13:
+ val = get_unaligned_be32(&power->vdd.update_tag);
+ break;
+ case 14:
+ val = get_unaligned_be64(&power->vdd.accumulator);
+ break;
+ case 15:
+ return snprintf(buf, PAGE_SIZE - 1, "vdn\n");
+ case 16:
+ /* microwatts */
+ val = get_unaligned_be16(&power->vdn.value) * 1000000ULL;
+ break;
+ case 17:
+ val = get_unaligned_be32(&power->vdn.update_tag);
+ break;
+ case 18:
+ val = get_unaligned_be64(&power->vdn.accumulator);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%llu\n", val);
+}
+
+static ssize_t occ_show_caps_1(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u16 val = 0;
+ struct caps_sensor_1 *caps;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ caps = ((struct caps_sensor_1 *)sensors->caps.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be16(&caps->curr_powercap);
+ break;
+ case 1:
+ val = get_unaligned_be16(&caps->curr_powerreading);
+ break;
+ case 2:
+ val = get_unaligned_be16(&caps->norm_powercap);
+ break;
+ case 3:
+ val = get_unaligned_be16(&caps->max_powercap);
+ break;
+ case 4:
+ val = get_unaligned_be16(&caps->min_powercap);
+ break;
+ case 5:
+ val = get_unaligned_be16(&caps->user_powerlimit);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+}
+
+static ssize_t occ_show_caps_2(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u16 val = 0;
+ struct caps_sensor_2 *caps;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ caps = ((struct caps_sensor_2 *)sensors->caps.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be16(&caps->curr_powercap);
+ break;
+ case 1:
+ val = get_unaligned_be16(&caps->curr_powerreading);
+ break;
+ case 2:
+ val = get_unaligned_be16(&caps->norm_powercap);
+ break;
+ case 3:
+ val = get_unaligned_be16(&caps->max_powercap);
+ break;
+ case 4:
+ val = get_unaligned_be16(&caps->min_powercap);
+ break;
+ case 5:
+ val = get_unaligned_be16(&caps->user_powerlimit);
+ break;
+ case 6:
+ val = caps->user_powerlimit_source;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+}
+
+static ssize_t occ_show_caps_3(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ u16 val = 0;
+ struct caps_sensor_3 *caps;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ caps = ((struct caps_sensor_3 *)sensors->caps.data) + sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ val = get_unaligned_be16(&caps->curr_powercap);
+ break;
+ case 1:
+ val = get_unaligned_be16(&caps->curr_powerreading);
+ break;
+ case 2:
+ val = get_unaligned_be16(&caps->norm_powercap);
+ break;
+ case 3:
+ val = get_unaligned_be16(&caps->max_powercap);
+ break;
+ case 4:
+ val = get_unaligned_be16(&caps->hard_min_powercap);
+ break;
+ case 5:
+ val = get_unaligned_be16(&caps->user_powerlimit);
+ break;
+ case 6:
+ val = caps->user_powerlimit_source;
+ break;
+ case 7:
+ val = get_unaligned_be16(&caps->soft_min_powercap);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+}
+
+static ssize_t occ_store_caps_user(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int rc;
+ u16 user_power_cap;
+ struct occ *occ = dev_get_drvdata(dev);
+
+ rc = kstrtou16(buf, 0, &user_power_cap);
+ if (rc)
+ return rc;
+
+ rc = occ_set_user_power_cap(occ, user_power_cap);
+ if (rc)
+ return rc;
+
+ return count;
+}
+
+static ssize_t occ_show_extended(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ struct extended_sensor *extn;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_sensors *sensors = &occ->sensors;
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ extn = ((struct extended_sensor *)sensors->extended.data) +
+ sattr->index;
+
+ switch (sattr->nr) {
+ case 0:
+ rc = snprintf(buf, PAGE_SIZE - 1, "%02x%02x%02x%02x\n",
+ extn->name[0], extn->name[1], extn->name[2],
+ extn->name[3]);
+ break;
+ case 1:
+ rc = snprintf(buf, PAGE_SIZE - 1, "%02x\n", extn->flags);
+ break;
+ case 2:
+ rc = snprintf(buf, PAGE_SIZE - 1, "%02x%02x%02x%02x%02x%02x\n",
+ extn->data[0], extn->data[1], extn->data[2],
+ extn->data[3], extn->data[4], extn->data[5]);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return rc;
+}
+
/* only need to do this once at startup, as OCC won't change sensors on us */
static void occ_parse_poll_response(struct occ *occ)
{
@@ -95,6 +740,9 @@ int occ_setup(struct occ *occ, const char *name)
{
int rc;

+ mutex_init(&occ->lock);
+
+ /* no need to lock */
rc = occ_poll(occ);
if (rc == -ESHUTDOWN) {
dev_info(occ->bus_dev, "host is not ready\n");
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index cfcd353..f697adb 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -12,6 +12,8 @@
#ifndef OCC_COMMON_H
#define OCC_COMMON_H

+#include <linux/mutex.h>
+
struct device;

#define OCC_RESP_DATA_BYTES 4089
@@ -91,6 +93,9 @@ struct occ {

u8 poll_cmd_data; /* to perform OCC poll command */
int (*send_cmd)(struct occ *occ, u8 *cmd);
+
+ unsigned long last_update;
+ struct mutex lock; /* lock OCC access */
};

int occ_setup(struct occ *occ, const char *name);
--
1.8.3.1

2018-07-25 16:20:44

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH v4 1/9] fsi: Add On-Chip Controller (OCC) driver

On Wed, Jul 11, 2018 at 04:01:30PM -0500, Eddie James wrote:
> The OCC is a device embedded on a POWER processor that collects and
> aggregates sensor data from the processor and system. The OCC can
> provide the raw sensor data as well as perform thermal and power
> management on the system.
>
> This driver provides an atomic communications channel between a service
> processor (e.g. a BMC) and the OCC. The driver is dependent on the FSI
> SBEFIFO driver to get hardware access through the SBE to the OCC SRAM.
> Commands are issued to the SBE to send or fetch data to the SRAM.
>
> Signed-off-by: Eddie James <[email protected]>
> Signed-off-by: Andrew Jeffery <[email protected]>
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
> Signed-off-by: Joel Stanley <[email protected]>
> ---
> drivers/fsi/Kconfig | 10 +
> drivers/fsi/Makefile | 1 +
> drivers/fsi/fsi-occ.c | 609 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/fsi-occ.h | 34 +++
> 4 files changed, 654 insertions(+)
> create mode 100644 drivers/fsi/fsi-occ.c
> create mode 100644 include/linux/fsi-occ.h
>
> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> index 24f84a9..322cec3 100644
> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -39,4 +39,14 @@ config FSI_SBEFIFO
> a pipe-like FSI device for communicating with the self boot engine
> (SBE) on POWER processors.
>
> +config FSI_OCC
> + tristate "OCC SBEFIFO client device driver"
> + depends on FSI_SBEFIFO
> + ---help---
> + This option enables an SBEFIFO based On-Chip Controller (OCC) device
> + driver. The OCC is a device embedded on a POWER processor that collects
> + and aggregates sensor data from the processor and system. The OCC can
> + provide the raw sensor data as well as perform thermal and power
> + management on the system.
> +
> endif
> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
> index 851182e..75fdc6d 100644
> --- a/drivers/fsi/Makefile
> +++ b/drivers/fsi/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
> obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
> obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
> obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
> +obj-$(CONFIG_FSI_OCC) += fsi-occ.o
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> new file mode 100644
> index 0000000..bba67a6
> --- /dev/null
> +++ b/drivers/fsi/fsi-occ.c
> @@ -0,0 +1,609 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * On-Chip Controller Driver
> + *
> + * Copyright (C) IBM Corporation 2018
> + *
> + * 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.

I thought the whole idea of using SPDX license identifiers was to get rid
of this boilerplate.

> + */
> +
> +#include <asm/unaligned.h>

asm includes should be after generic includes.

> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/fsi-sbefifo.h>
> +#include <linux/gfp.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/fsi-occ.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#define OCC_SRAM_BYTES 4096
> +#define OCC_CMD_DATA_BYTES 4090
> +#define OCC_RESP_DATA_BYTES 4089
> +
> +#define OCC_SRAM_CMD_ADDR 0xFFFBE000
> +#define OCC_SRAM_RSP_ADDR 0xFFFBF000
> +
> +/*
> + * Assume we don't have much FFDC, if we do we'll overflow and
> + * fail the command. This needs to be big enough for simple
> + * commands as well.
> + */
> +#define OCC_SBE_STATUS_WORDS 32
> +
> +#define OCC_TIMEOUT_MS 1000
> +#define OCC_CMD_IN_PRG_WAIT_MS 50
> +
> +struct occ {
> + struct device *dev;
> + struct device *sbefifo;
> + char name[32];
> + int idx;
> + struct miscdevice mdev;
> + struct mutex occ_lock;
> +};
> +
> +#define to_occ(x) container_of((x), struct occ, mdev)
> +
> +struct occ_response {
> + u8 seq_no;
> + u8 cmd_type;
> + u8 return_status;
> + __be16 data_length;
> + u8 data[OCC_RESP_DATA_BYTES + 2]; /* two bytes checksum */
> +} __packed;
> +
> +struct occ_client {
> + struct occ *occ;
> + struct mutex lock;
> + size_t data_size;
> + size_t read_offset;
> + u8 *buffer;
> +};
> +
> +#define to_client(x) container_of((x), struct occ_client, xfr)
> +
> +static DEFINE_IDA(occ_ida);
> +
> +static int occ_open(struct inode *inode, struct file *file)
> +{
> + struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
> + struct miscdevice *mdev = file->private_data;
> + struct occ *occ = to_occ(mdev);
> +
> + if (!client)
> + return -ENOMEM;
> +
> + client->buffer = (u8 *)__get_free_page(GFP_KERNEL);
> + if (!client->buffer) {
> + kfree(client);
> + return -ENOMEM;
> + }
> +
> + client->occ = occ;
> + mutex_init(&client->lock);
> + file->private_data = client;
> +
> + /* We allocate a 1-page buffer, make sure it all fits */
> + BUILD_BUG_ON((OCC_CMD_DATA_BYTES + 3) > PAGE_SIZE);
> + BUILD_BUG_ON((OCC_RESP_DATA_BYTES + 7) > PAGE_SIZE);
> +
> + return 0;
> +}
> +
> +static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
> + loff_t *offset)
> +{
> + struct occ_client *client = file->private_data;
> + ssize_t rc = 0;
> +
> + if (!client)
> + return -ENODEV;
> +
> + if (len > OCC_SRAM_BYTES)
> + return -EINVAL;
> +
> + mutex_lock(&client->lock);
> +
> + /* This should not be possible ... */
> + if (WARN_ON_ONCE(client->read_offset > client->data_size)) {
> + rc = -EIO;
> + goto done;
> + }
> +
> + /* Grab how much data we have to read */
> + rc = min(len, client->data_size - client->read_offset);
> + if (copy_to_user(buf, client->buffer + client->read_offset, rc))
> + rc = -EFAULT;
> + else
> + client->read_offset += rc;
> +
> + done:
> + mutex_unlock(&client->lock);
> +
> + return rc;
> +}
> +
> +static ssize_t occ_write(struct file *file, const char __user *buf,
> + size_t len, loff_t *offset)
> +{
> + struct occ_client *client = file->private_data;
> + size_t rlen, data_length;
> + u16 checksum = 0;
> + ssize_t rc, i;
> + u8 *cmd;
> +
> + if (!client)
> + return -ENODEV;
> +
> + if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
> + return -EINVAL;
> +
> + mutex_lock(&client->lock);
> +
> + /* Construct the command */
> + cmd = client->buffer;
> +
> + /* Sequence number (we could increment and compare with response) */
> + cmd[0] = 1;
> +
> + /*
> + * Copy the user command (assume user data follows the occ command
> + * format)
> + * byte 0: command type
> + * bytes 1-2: data length (msb first)
> + * bytes 3-n: data
> + */
> + if (copy_from_user(&cmd[1], buf, len)) {
> + rc = -EFAULT;
> + goto done;
> + }
> +
> + /* Extract data length */
> + data_length = (cmd[2] << 8) + cmd[3];
> + if (data_length > OCC_CMD_DATA_BYTES) {
> + rc = -EINVAL;
> + goto done;
> + }
> +
> + /* Calculate checksum */
> + for (i = 0; i < data_length + 4; ++i)
> + checksum += cmd[i];
> +
> + cmd[data_length + 4] = checksum >> 8;
> + cmd[data_length + 5] = checksum & 0xFF;
> +
> + /* Submit command */
> + rlen = PAGE_SIZE;
> + rc = fsi_occ_submit(client->occ->dev, cmd, data_length + 6, cmd,
> + &rlen);
> + if (rc)
> + goto done;
> +
> + /* Set read tracking data */
> + client->data_size = rlen;
> + client->read_offset = 0;
> +
> + /* Done */
> + rc = len;
> +
> + done:
> + mutex_unlock(&client->lock);
> +
> + return rc;
> +}
> +
> +static int occ_release(struct inode *inode, struct file *file)
> +{
> + struct occ_client *client = file->private_data;
> +
> + free_page((unsigned long)client->buffer);
> + kfree(client);
> +
> + return 0;
> +}
> +
> +static const struct file_operations occ_fops = {
> + .owner = THIS_MODULE,
> + .open = occ_open,
> + .read = occ_read,
> + .write = occ_write,
> + .release = occ_release,
> +};
> +
> +static int occ_verify_checksum(struct occ_response *resp, u16 data_length)
> +{
> + /* Fetch the two bytes after the data for the checksum. */
> + u16 checksum_resp = get_unaligned_be16(&resp->data[data_length]);
> + u16 checksum;
> + u16 i;
> +
> + checksum = resp->seq_no;
> + checksum += resp->cmd_type;
> + checksum += resp->return_status;
> + checksum += (data_length >> 8) + (data_length & 0xFF);
> +
> + for (i = 0; i < data_length; ++i)
> + checksum += resp->data[i];
> +
> + if (checksum != checksum_resp)
> + return -EBADMSG;
> +
> + return 0;
> +}
> +
> +static int occ_getsram(struct occ *occ, u32 address, void *data, ssize_t len)
> +{
> + u32 data_len = ((len + 7) / 8) * 8; /* must be multiples of 8 B */
> + size_t resp_len, resp_data_len;
> + __be32 *resp, cmd[5];
> + int rc;
> +
> + /*
> + * Magic sequence to do SBE getsram command. SBE will fetch data from
> + * specified SRAM address.
> + */
> + cmd[0] = cpu_to_be32(0x5);
> + cmd[1] = cpu_to_be32(SBEFIFO_CMD_GET_OCC_SRAM);
> + cmd[2] = cpu_to_be32(1);
> + cmd[3] = cpu_to_be32(address);
> + cmd[4] = cpu_to_be32(data_len);
> +
> + resp_len = (data_len >> 2) + OCC_SBE_STATUS_WORDS;
> + resp = kzalloc(resp_len << 2, GFP_KERNEL);
> + if (!resp)
> + return -ENOMEM;
> +
> + rc = sbefifo_submit(occ->sbefifo, cmd, 5, resp, &resp_len);
> + if (rc)
> + goto free;
> +
> + rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_GET_OCC_SRAM,
> + resp, resp_len, &resp_len);
> + if (rc)
> + goto free;
> +
> + resp_data_len = be32_to_cpu(resp[resp_len - 1]);
> + if (resp_data_len != data_len) {
> + dev_err(occ->dev, "SRAM read expected %d bytes got %zd\n",
> + data_len, resp_data_len);
> + rc = -EBADMSG;
> + } else {
> + memcpy(data, resp, len);
> + }
> +
> +free:
> + /* Convert positive SBEI status */
> + if (rc > 0) {
> + dev_err(occ->dev, "SRAM read returned failure status: %08x\n",
> + rc);
> + rc = -EBADMSG;
> + }
> +
> + kfree(resp);
> + return rc;
> +}
> +
> +static int occ_putsram(struct occ *occ, u32 address, const void *data,
> + ssize_t len)
> +{
> + size_t cmd_len, buf_len, resp_len, resp_data_len;
> + u32 data_len = ((len + 7) / 8) * 8; /* must be multiples of 8 B */
> + __be32 *buf;
> + int rc;
> +
> + /*
> + * We use the same buffer for command and response, make
> + * sure it's big enough
> + */
> + resp_len = OCC_SBE_STATUS_WORDS;
> + cmd_len = (data_len >> 2) + 5;
> + buf_len = max(cmd_len, resp_len);
> + buf = kzalloc(buf_len << 2, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /*
> + * Magic sequence to do SBE putsram command. SBE will transfer
> + * data to specified SRAM address.
> + */
> + buf[0] = cpu_to_be32(cmd_len);
> + buf[1] = cpu_to_be32(SBEFIFO_CMD_PUT_OCC_SRAM);
> + buf[2] = cpu_to_be32(1);
> + buf[3] = cpu_to_be32(address);
> + buf[4] = cpu_to_be32(data_len);
> +
> + memcpy(&buf[5], data, len);
> +
> + rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len);
> + if (rc)
> + goto free;
> +
> + rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
> + buf, resp_len, &resp_len);
> + if (rc)
> + goto free;
> +
> + if (resp_len != 1) {
> + dev_err(occ->dev, "SRAM write response length invalid: %zd\n",
> + resp_len);
> + rc = -EBADMSG;
> + } else {
> + resp_data_len = be32_to_cpu(buf[0]);
> + if (resp_data_len != data_len) {
> + dev_err(occ->dev,
> + "SRAM write expected %d bytes got %zd\n",
> + data_len, resp_data_len);
> + rc = -EBADMSG;
> + }
> + }
> +
> +free:
> + /* Convert positive SBEI status */
> + if (rc > 0) {
> + dev_err(occ->dev, "SRAM write returned failure status: %08x\n",
> + rc);
> + rc = -EBADMSG;
> + }
> +
> + kfree(buf);
> + return rc;
> +}
> +
> +static int occ_trigger_attn(struct occ *occ)
> +{
> + __be32 buf[OCC_SBE_STATUS_WORDS];
> + size_t resp_len, resp_data_len;
> + int rc;
> +
> + BUILD_BUG_ON(OCC_SBE_STATUS_WORDS < 7);
> + resp_len = OCC_SBE_STATUS_WORDS;
> +
> + buf[0] = cpu_to_be32(0x5 + 0x2); /* Chip-op length in words */
> + buf[1] = cpu_to_be32(SBEFIFO_CMD_PUT_OCC_SRAM);
> + buf[2] = cpu_to_be32(0x3); /* Mode: Circular */
> + buf[3] = cpu_to_be32(0x0); /* Address: ignore in mode 3 */
> + buf[4] = cpu_to_be32(0x8); /* Data length in bytes */
> + buf[5] = cpu_to_be32(0x20010000); /* Trigger OCC attention */
> + buf[6] = 0;
> +
> + rc = sbefifo_submit(occ->sbefifo, buf, 7, buf, &resp_len);
> + if (rc)
> + goto error;
> +
> + rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
> + buf, resp_len, &resp_len);
> + if (rc)
> + goto error;
> +
> + if (resp_len != 1) {
> + dev_err(occ->dev, "SRAM attn response length invalid: %zd\n",
> + resp_len);
> + rc = -EBADMSG;
> + } else {
> + resp_data_len = be32_to_cpu(buf[0]);
> + if (resp_data_len != 8) {
> + dev_err(occ->dev,
> + "SRAM attn expected 8 bytes got %zd\n",
> + resp_data_len);
> + rc = -EBADMSG;
> + }
> + }
> +
> + error:
> + /* Convert positive SBEI status */
> + if (rc > 0) {
> + dev_err(occ->dev, "SRAM attn returned failure status: %08x\n",
> + rc);
> + rc = -EBADMSG;
> + }
> +
> + return rc;
> +}
> +
> +int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
> + void *response, size_t *resp_len)
> +{
> + const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
> + const unsigned long wait_time =
> + msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
> + struct occ *occ = dev_get_drvdata(dev);
> + struct occ_response *resp = response;
> + u16 resp_data_length;
> + unsigned long start;
> + int rc;
> +
> + if (!occ)
> + return -ENODEV;
> +
> + if (*resp_len < 7) {
> + dev_dbg(dev, "Bad resplen %zd\n", *resp_len);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&occ->occ_lock);
> +
> + rc = occ_putsram(occ, OCC_SRAM_CMD_ADDR, request, req_len);
> + if (rc)
> + goto done;
> +
> + rc = occ_trigger_attn(occ);
> + if (rc)
> + goto done;
> +
> + /* Read occ response header */
> + start = jiffies;
> + do {
> + rc = occ_getsram(occ, OCC_SRAM_RSP_ADDR, resp, 8);
> + if (rc)
> + goto done;
> +
> + if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
> + rc = -ETIMEDOUT;
> +
> + if (time_after(jiffies, start + timeout))
> + break;
> +
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + schedule_timeout(wait_time);
> + }
> + } while (rc);
> +
> + /* Extract size of response data */
> + resp_data_length = get_unaligned_be16(&resp->data_length);
> +
> + /* Message size is data length + 5 bytes header + 2 bytes checksum */
> + if ((resp_data_length + 7) > *resp_len) {
> + rc = -EMSGSIZE;
> + goto done;
> + }
> +
> + dev_dbg(dev, "resp_status=%02x resp_data_len=%d\n",
> + resp->return_status, resp_data_length);
> +
> + /* Grab the rest */
> + if (resp_data_length > 1) {
> + /* already got 3 bytes resp, also need 2 bytes checksum */
> + rc = occ_getsram(occ, OCC_SRAM_RSP_ADDR + 8,
> + &resp->data[3], resp_data_length - 1);
> + if (rc)
> + goto done;
> + }
> +
> + *resp_len = resp_data_length + 7;
> + rc = occ_verify_checksum(resp, resp_data_length);
> +
> + done:
> + mutex_unlock(&occ->occ_lock);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(fsi_occ_submit);
> +
> +static int occ_unregister_child(struct device *dev, void *data)
> +{
> + struct platform_device *hwmon_dev = to_platform_device(dev);
> +
> + platform_device_unregister(hwmon_dev);
> +
> + return 0;
> +}
> +
> +static int occ_probe(struct platform_device *pdev)
> +{
> + int rc;
> + u32 reg;
> + struct occ *occ;
> + struct platform_device *hwmon_dev;
> + struct device *dev = &pdev->dev;
> + struct platform_device_info hwmon_dev_info = {
> + .parent = dev,
> + .name = "occ-hwmon",
> + };
> +
> + occ = devm_kzalloc(dev, sizeof(*occ), GFP_KERNEL);
> + if (!occ)
> + return -ENOMEM;
> +
> + occ->dev = dev;
> + occ->sbefifo = dev->parent;
> + mutex_init(&occ->occ_lock);
> +
> + if (dev->of_node) {
> + rc = of_property_read_u32(dev->of_node, "reg", &reg);
> + if (!rc) {
> + /* make sure we don't have a duplicate from dts */
> + occ->idx = ida_simple_get(&occ_ida, reg, reg + 1,
> + GFP_KERNEL);
> + if (occ->idx < 0)
> + occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
> + GFP_KERNEL);
> + } else {
> + occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
> + GFP_KERNEL);
> + }
> + } else {
> + occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
> + }
> +
> + platform_set_drvdata(pdev, occ);
> +
> + snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
> + occ->mdev.fops = &occ_fops;
> + occ->mdev.minor = MISC_DYNAMIC_MINOR;
> + occ->mdev.name = occ->name;
> + occ->mdev.parent = dev;
> +
> + rc = misc_register(&occ->mdev);
> + if (rc) {
> + dev_err(dev, "failed to register miscdevice: %d\n", rc);
> + ida_simple_remove(&occ_ida, occ->idx);
> + return rc;
> + }
> +
> + hwmon_dev_info.id = occ->idx;
> + hwmon_dev = platform_device_register_full(&hwmon_dev_info);
> + if (!hwmon_dev)
> + dev_warn(dev, "failed to create hwmon device\n");
> +

Not my call to make, but since you are using devicetree I would have
thought it would be possible to use something like of_platform_populate()
to register child drivers.

> + return 0;
> +}
> +
> +static int occ_remove(struct platform_device *pdev)
> +{
> + struct occ *occ = platform_get_drvdata(pdev);
> +
> + misc_deregister(&occ->mdev);
> +
> + device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
> +
> + ida_simple_remove(&occ_ida, occ->idx);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id occ_match[] = {
> + { .compatible = "ibm,p9-occ" },
> + { },
> +};
> +
> +static struct platform_driver occ_driver = {
> + .driver = {
> + .name = "occ",
> + .of_match_table = occ_match,
> + },
> + .probe = occ_probe,
> + .remove = occ_remove,
> +};
> +
> +static int occ_init(void)
> +{
> + return platform_driver_register(&occ_driver);
> +}
> +
> +static void occ_exit(void)
> +{
> + platform_driver_unregister(&occ_driver);
> +
> + ida_destroy(&occ_ida);
> +}
> +
> +module_init(occ_init);
> +module_exit(occ_exit);
> +
> +MODULE_AUTHOR("Eddie James <[email protected]>");
> +MODULE_DESCRIPTION("BMC P9 OCC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/fsi-occ.h b/include/linux/fsi-occ.h
> new file mode 100644
> index 0000000..e28cf9a
> --- /dev/null
> +++ b/include/linux/fsi-occ.h
> @@ -0,0 +1,34 @@
> +/*
> + * On-Chip Controller Driver definitions and prototypes
> + *
> + * Copyright (C) IBM Corporation 2018
> + *
> + * 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.
> + */
> +
> +#ifndef LINUX_FSI_OCC_H
> +#define LINUX_FSI_OCC_H
> +
> +struct device;
> +
> +#define OCC_RESP_CMD_IN_PRG 0xFF
> +#define OCC_RESP_SUCCESS 0
> +#define OCC_RESP_CMD_INVAL 0x11
> +#define OCC_RESP_CMD_LEN_INVAL 0x12
> +#define OCC_RESP_DATA_INVAL 0x13
> +#define OCC_RESP_CHKSUM_ERR 0x14
> +#define OCC_RESP_INT_ERR 0x15
> +#define OCC_RESP_BAD_STATE 0x16
> +#define OCC_RESP_CRIT_EXCEPT 0xE0
> +#define OCC_RESP_CRIT_INIT 0xE1
> +#define OCC_RESP_CRIT_WATCHDOG 0xE2
> +#define OCC_RESP_CRIT_OCB 0xE3
> +#define OCC_RESP_CRIT_HW 0xE4
> +
> +int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
> + void *response, size_t *resp_len);
> +
> +#endif /* LINUX_FSI_OCC_H */

2018-07-25 16:37:13

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH v4 2/9] Documentation: hwmon: Add OCC documentation

On Wed, Jul 11, 2018 at 04:01:31PM -0500, Eddie James wrote:
> Document the hwmon interface for the OCC.
>
> Signed-off-by: Eddie James <[email protected]>
> ---
> Documentation/hwmon/occ | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
> create mode 100644 Documentation/hwmon/occ
>
> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> new file mode 100644
> index 0000000..465fa1a
> --- /dev/null
> +++ b/Documentation/hwmon/occ
> @@ -0,0 +1,73 @@
> +Kernel driver occ-hwmon
> +=======================
> +
> +Supported chips:
> + * POWER8
> + * POWER9
> +
> +Author: Eddie James <[email protected]>
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for the On-Chip Controller (OCC)
> +embedded on POWER processors. The OCC is a device that collects and aggregates
> +sensor data from the processor and the system. The OCC can provide the raw
> +sensor data as well as perform thermal and power management on the system.
> +
> +The P8 version of this driver is a client driver of I2C. It may be probed
> +manually if an "ibm,p8-occ-hwmon" compatible device is found under the
> +appropriate I2C bus node in the device-tree.
> +
> +The P9 version of this driver is a client driver of the FSI-based OCC driver.
> +It will be probed automatically by the FSI-based OCC driver.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. All attributes are read-only unless
> +specified.
> +
> +temp[1-n]_label OCC sensor id.
> +temp[1-n]_input Measured temperature in millidegrees C.
> +[with temperature sensor version 2+]
> + temp[1-n]_fru_type Given FRU (Field Replaceable Unit) type.

What is this ? An integer ? A string ?

> + temp[1-n]_fault Temperature sensor fault.
> +
> +freq[1-n]_label OCC sensor id.
> +freq[1-n]_input Measured frequency.

What does that have to do with hardware monitoring, and what exactly does it
measure ? AC voltage frequency ? Frequency of rainstorms in the surrounding
area ?

> +
> +power[1-n]_label OCC sensor id.
> +power[1-n]_input Measured power in microwatts.
> +power[1-n]_update_tag Number of 250us samples represented in accumulator.

update_tag to represent number of samples ? Odd choice for
an attribute name. Why not "_samples" ? Also, if each sample
represents a specific amount of time, why not report a time ?

> +power[1-n]_accumulator Accumulation of 250us power readings.

There is no explanation of "accumulation". Is this the energy ?
If so, why not use energy attributes ? And what is the unit of
this measurement ?

> +[with power sensor version 2+]
> + power[1-n]_function_id Identifies what the power reading is for.

String ? Number ? Slot index ? Bitmap ? And why isn't that reported
in the label ? After all, that is what the label is supposed to be
used for.

> + power[1-n]_apss_channel Indicates APSS channel.
> +

Does that provide any value to the user ?

> +[power version 0xa0 only]
> +power1_id OCC sensor id.

This is inconsistent with the other attributes and even with itself.

> +power[1-n]_label Sensor type, "system", "proc", "vdd", or "vdn".
> +power[1-n]_input Most recent power reading in microwatts.

Overall I am left with no idea what
_id
_label
_function_id
_apps_channel
are and how they relate to each other, except that it all looks quite
inconsistent. You might want to consider merging all those attributes into
the label in some consistent way.

> +power[1-n]_update_tag Number of samples in the accumulator.
> +power[1-n]_accumulator Accumulation of power readings.

Same as above.

> +[with sensor type "system" and "proc" only]
> + power[1-n]_update_time Time in us that the power value is read.
> +
> +caps1_current Current OCC power cap in watts.
> +caps1_reading Current system output power in watts.
> +caps1_norm Power cap without redundant power.
> +caps1_max Maximum power cap.

Why do those have to be non-standard attributes ? Please explain why you can not
use power[1-n]_cap attributes.

> +[caps version 1 and 2 only]
> + caps1_min Minimum power cap.
> +[caps version 3+]
> + caps1_min_hard Hard minimum cap that can be set and held.
> + caps1_min_soft Soft minimum cap below hard, not guaranteed.
> +caps1_user The powercap specified by the user. Will be 0 if no
> + user powercap exists. This attribute is read-write.
> +[caps version 1+]
> + caps1_user_source Indicates how the user power limit was set.
> +
> +extn[1-n]_label ASCII id or sensor id.
> +extn[1-n]_flags Indicates type of label attribute.
> +extn[1-n]_input Data.

Great non-explanation.

Not reviewing the series further. I am sure I asked that each non-standard
attribute is explained. There is neither an explanation why the attributes
are needed nor, in many cases, why non-standard attributes were chosen
instead of standard ones. On top of that, the non-standard attributes are
not even documented properly, leaving the reader wondering not only why
they are needed, but what they are used for in the first place.

Guenter

2018-08-30 21:30:57

by Eddie James

[permalink] [raw]

Subject: Re: [PATCH v4 2/9] Documentation: hwmon: Add OCC documentation

On 07/25/2018 11:36 AM, Guenter Roeck wrote:
> On Wed, Jul 11, 2018 at 04:01:31PM -0500, Eddie James wrote:
>> Document the hwmon interface for the OCC.
>>
>> Signed-off-by: Eddie James <[email protected]>
>> ---
>> Documentation/hwmon/occ | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 73 insertions(+)
>> create mode 100644 Documentation/hwmon/occ
>>
>> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
>> new file mode 100644
>> index 0000000..465fa1a
>> --- /dev/null
>> +++ b/Documentation/hwmon/occ
>> @@ -0,0 +1,73 @@
>> +Kernel driver occ-hwmon
>> +=======================
>> +
>> +Supported chips:
>> + * POWER8
>> + * POWER9
>> +
>> +Author: Eddie James <[email protected]>
>> +
>> +Description
>> +-----------
>> +
>> +This driver supports hardware monitoring for the On-Chip Controller (OCC)
>> +embedded on POWER processors. The OCC is a device that collects and aggregates
>> +sensor data from the processor and the system. The OCC can provide the raw
>> +sensor data as well as perform thermal and power management on the system.
>> +
>> +The P8 version of this driver is a client driver of I2C. It may be probed
>> +manually if an "ibm,p8-occ-hwmon" compatible device is found under the
>> +appropriate I2C bus node in the device-tree.
>> +
>> +The P9 version of this driver is a client driver of the FSI-based OCC driver.
>> +It will be probed automatically by the FSI-based OCC driver.
>> +
>> +Sysfs entries
>> +-------------
>> +
>> +The following attributes are supported. All attributes are read-only unless
>> +specified.
>> +
>> +temp[1-n]_label OCC sensor id.
>> +temp[1-n]_input Measured temperature in millidegrees C.
>> +[with temperature sensor version 2+]
>> + temp[1-n]_fru_type Given FRU (Field Replaceable Unit) type.
> What is this ? An integer ? A string ?
>
>> + temp[1-n]_fault Temperature sensor fault.
>> +
>> +freq[1-n]_label OCC sensor id.
>> +freq[1-n]_input Measured frequency.
> What does that have to do with hardware monitoring, and what exactly does it
> measure ? AC voltage frequency ? Frequency of rainstorms in the surrounding
> area ?
>
>> +
>> +power[1-n]_label OCC sensor id.
>> +power[1-n]_input Measured power in microwatts.
>> +power[1-n]_update_tag Number of 250us samples represented in accumulator.
> update_tag to represent number of samples ? Odd choice for
> an attribute name. Why not "_samples" ? Also, if each sample
> represents a specific amount of time, why not report a time ?
>
>> +power[1-n]_accumulator Accumulation of 250us power readings.
> There is no explanation of "accumulation". Is this the energy ?
> If so, why not use energy attributes ? And what is the unit of
> this measurement ?
>
>> +[with power sensor version 2+]
>> + power[1-n]_function_id Identifies what the power reading is for.
> String ? Number ? Slot index ? Bitmap ? And why isn't that reported
> in the label ? After all, that is what the label is supposed to be
> used for.
>
>> + power[1-n]_apss_channel Indicates APSS channel.
>> +
> Does that provide any value to the user ?
>
>> +[power version 0xa0 only]
>> +power1_id OCC sensor id.
> This is inconsistent with the other attributes and even with itself.
>
>> +power[1-n]_label Sensor type, "system", "proc", "vdd", or "vdn".
>> +power[1-n]_input Most recent power reading in microwatts.
> Overall I am left with no idea what
> _id
> _label
> _function_id
> _apps_channel
> are and how they relate to each other, except that it all looks quite
> inconsistent. You might want to consider merging all those attributes into
> the label in some consistent way.
>
>> +power[1-n]_update_tag Number of samples in the accumulator.
>> +power[1-n]_accumulator Accumulation of power readings.
> Same as above.
>
>> +[with sensor type "system" and "proc" only]
>> + power[1-n]_update_time Time in us that the power value is read.
>> +
>> +caps1_current Current OCC power cap in watts.
>> +caps1_reading Current system output power in watts.
>> +caps1_norm Power cap without redundant power.
>> +caps1_max Maximum power cap.
> Why do those have to be non-standard attributes ? Please explain why you can not
> use power[1-n]_cap attributes.
>
>> +[caps version 1 and 2 only]
>> + caps1_min Minimum power cap.
>> +[caps version 3+]
>> + caps1_min_hard Hard minimum cap that can be set and held.
>> + caps1_min_soft Soft minimum cap below hard, not guaranteed.
>> +caps1_user The powercap specified by the user. Will be 0 if no
>> + user powercap exists. This attribute is read-write.
>> +[caps version 1+]
>> + caps1_user_source Indicates how the user power limit was set.
>> +
>> +extn[1-n]_label ASCII id or sensor id.
>> +extn[1-n]_flags Indicates type of label attribute.
>> +extn[1-n]_input Data.
> Great non-explanation.
>
> Not reviewing the series further. I am sure I asked that each non-standard
> attribute is explained. There is neither an explanation why the attributes
> are needed nor, in many cases, why non-standard attributes were chosen
> instead of standard ones. On top of that, the non-standard attributes are
> not even documented properly, leaving the reader wondering not only why
> they are needed, but what they are used for in the first place.

Hi,

Thanks for the feedback Guenter. I am about to put up a new patch set
with fixes for many of the issues you indicated and better descriptions.
Now all the attributes except two should conform to standard hwmon
attributes. The exceptions are for the user power cap and user power cap
source. These are needed in order to make decisions about power
management while controlling the system. Please look at their
documentation to see if they're acceptable.

Let me know what you think!
Thanks,
Eddie

>
> Guenter
>