Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
From: Eddie James
Date: Mon Jun 26 2017 - 11:07:27 EST
On 06/24/2017 09:15 AM, Guenter Roeck wrote:
On 06/22/2017 03:48 PM, Eddie James wrote:
From: "Edward A. James" <eajames@xxxxxxxxxx>
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 a hwmon interface to the OCC from a service
processor (e.g. a BMC). The driver supports both POWER8 and POWER9 OCCs.
Communications with the POWER8 OCC are established over standard I2C
bus. The driver communicates with the POWER9 OCC through the FSI-based
OCC driver, which handles the lower-level communication details.
This patch lays out the structure of the OCC hwmon driver. There are two
platform drivers, one each for P8 and P9 OCCs. These are probed through
the I2C tree and the FSI-based OCC driver, respectively. The patch also
Where is that driver ?
My apologies Guenter, here is the series:
https://patchwork.kernel.org/patch/9802807/
https://patchwork.kernel.org/patch/9802801/
https://patchwork.kernel.org/patch/9802805/
https://patchwork.kernel.org/patch/9802803/
Do these FSI drivers need to be into linux-next before you want to look
at this hwmon driver?
Just a couple questions below on your comments.
Thanks,
Eddie
defines the first common structures and methods between the two OCC
versions.
Signed-off-by: Edward A. James <eajames@xxxxxxxxxx>
---
.../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 ++++++
.../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 ++++++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 28 +++++++++
drivers/hwmon/occ/Makefile | 11 ++++
drivers/hwmon/occ/common.c | 43 +++++++++++++
drivers/hwmon/occ/common.h | 41 +++++++++++++
drivers/hwmon/occ/p8_i2c.c | 70
++++++++++++++++++++++
drivers/hwmon/occ/p9_sbe.c | 65
++++++++++++++++++++
10 files changed, 304 insertions(+)
create mode 100644
Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt
create mode 100644
Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt
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
diff --git
a/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt
b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt
new file mode 100644
index 0000000..0ecebb7
--- /dev/null
+++ b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt
This needs to be approved by a DT maintainer, if for nothing else for
the new directory and file naming. For my part I have no idea how this
relates to the "fsi" directory introduced in -next.
@@ -0,0 +1,18 @@
+Device-tree bindings for FSI-based On-Chip Controller hwmon driver
+------------------------------------------------------------------
+
+This node MUST be a child node of an OCC driver node.
+
+Required properties:
+ - compatible = "ibm,p9-occ-hwmon";
+
+Examples:
+
+ occ@1 {
+ compatible = "ibm,p9-occ";
I don't see "ibm,p9-occ" documented anywhere (including linux-next).
+ reg = <1>;
+
+ occ-hwmon@1 {
+ compatible = "ibm,p9-occ-hwmon";
+ };
+ };
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..8c765d0
--- /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 driver
+------------------------------------------------------------------
+
+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>;
+ };
+ };
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5ef2814..08add7b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1250,6 +1250,8 @@ config SENSORS_NSA320
This driver can also be built as a module. If so, the module
will be called nsa320-hwmon.
+source drivers/hwmon/occ/Kconfig
+
config SENSORS_PCF8591
tristate "Philips PCF8591 ADC/DAC"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d4641a9..0b342d0 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
+obj-$(CONFIG_SENSORS_OCC) += occ/
obj-$(CONFIG_PMBUS) += pmbus/
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
new file mode 100644
index 0000000..0501280
--- /dev/null
+++ b/drivers/hwmon/occ/Kconfig
@@ -0,0 +1,28 @@
+#
+# On-Chip Controller configuration
+#
+
+config SENSORS_OCC
+ tristate "POWER On-Chip Controller"
+ ---help---
+ This option enables support for monitoring a variety of system
sensors
+ provided by the On-Chip Controller (OCC) on a POWER processor.
+
+ This driver can also be built as a module. If so, the module
will be
+ called occ-hwmon.
+
+config SENSORS_OCC_P8_I2C
+ bool "POWER8 OCC through I2C"
+ depends on I2C && SENSORS_OCC
+ ---help---
+ This option enables support for monitoring sensors provided by
the OCC
+ on a POWER8 processor. Communications with the OCC are established
+ through I2C bus.
+
+config SENSORS_OCC_P9_SBE
+ bool "POWER9 OCC through SBE"
+ depends on OCCFIFO && SENSORS_OCC
OCCFIFO is not declared anywhere in the kernel, including -next. This
leads me to
believe that I am missing some context. As a result, I can not even
compile this driver.
Please provide the missing context.
+ ---help---
+ This option enables support for monitoring sensors provided by
the OCC
+ on a POWER9 processor. Communications with the OCC are established
+ through SBE engine on an FSI bus.
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
new file mode 100644
index 0000000..ab5c3e9
--- /dev/null
+++ b/drivers/hwmon/occ/Makefile
@@ -0,0 +1,11 @@
+occ-hwmon-objs := common.o
+
+ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y)
+occ-hwmon-objs += p9_sbe.o
+endif
+
+ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y)
+occ-hwmon-objs += p8_i2c.o
+endif
+
+obj-$(CONFIG_SENSORS_OCC) += occ-hwmon.o
diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
new file mode 100644
index 0000000..60c808c
--- /dev/null
+++ b/drivers/hwmon/occ/common.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "common.h"
Please include local include files last, and include files needed here
from here, not indirectly.
+#include <linux/hwmon.h>
+
+static int occ_poll(struct occ *occ)
+{
+ u16 checksum = occ->poll_cmd_data + 1;
+ u8 cmd[8];
+
+ /* big endian */
+ cmd[0] = 0; /* sequence number */
+ cmd[1] = 0; /* cmd type */
+ cmd[2] = 0; /* data length msb */
+ cmd[3] = 1; /* data length lsb */
+ cmd[4] = occ->poll_cmd_data; /* data */
+ cmd[5] = checksum >> 8; /* checksum msb */
+ cmd[6] = checksum & 0xFF; /* checksum lsb */
+ cmd[7] = 0;
+
+ return occ->send_cmd(occ, cmd);
+}
+
+int occ_setup(struct occ *occ, const char *name)
+{
+ int rc;
+
+ rc = occ_poll(occ);
+ if (rc < 0) {
+ dev_err(occ->bus_dev, "failed to get OCC poll response: %d\n",
+ rc);
+ return rc;
+ }
+
+ return 0;
+}
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
new file mode 100644
index 0000000..dca642a
--- /dev/null
+++ b/drivers/hwmon/occ/common.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef OCC_COMMON_H
+#define OCC_COMMON_H
+
+#include <linux/hwmon-sysfs.h>
+#include <linux/sysfs.h>
Those include files are not needed here. Other include files,
which are needed, are missing. You can not expect the above
to include everything you need for you.
+
+#define OCC_RESP_DATA_BYTES 4089
+
+/* Same response format for all OCC versions.
+ * Allocate the largest possible response.
+ */
+struct occ_response {
+ u8 seq_no;
+ u8 cmd_type;
+ u8 return_status;
+ u16 data_length_be;
Why not "__be16 data_length" if that is what it is ?
+ u8 data[OCC_RESP_DATA_BYTES];
+ u16 checksum_be;
Same here.
+} __packed;
+
+struct occ {
+ struct device *bus_dev;
+
+ struct occ_response resp;
+
+ u8 poll_cmd_data; /* to perform OCC poll command */
+ int (*send_cmd)(struct occ *occ, u8 *cmd);
+};
+
+int occ_setup(struct occ *occ, const char *name);
+
+#endif /* OCC_COMMON_H */
diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
new file mode 100644
index 0000000..5075146
--- /dev/null
+++ b/drivers/hwmon/occ/p8_i2c.c
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "common.h"
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
Please include files in alphabetic order, and local include file(s) last.
+
+struct p8_i2c_occ {
+ struct occ occ;
+ struct i2c_client *client;
+};
+
+#define to_p8_i2c_occ(x) container_of((x), struct p8_i2c_occ, occ)
+
+static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
+{
+ return -EOPNOTSUPP;
+}
+
+static int p8_i2c_occ_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct occ *occ;
+ struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev,
+ sizeof(*p8_i2c_occ),
+ GFP_KERNEL);
I am quite sure this results in a checkpatch warning. It is also
clumsy, with
all those continuation lines. I would sugegst to split the variable
declaration
and the assignment.
+ if (!p8_i2c_occ)
+ return -ENOMEM;
+
+ p8_i2c_occ->client = client;
+ occ = &p8_i2c_occ->occ;
+ occ->bus_dev = &client->dev;
+ dev_set_drvdata(&client->dev, occ);
+
+ occ->poll_cmd_data = 0x10; /* P8 OCC poll data */
It would be beneficial to define those commands in the include file.
I can add a #define for them, but I'm not sure it makes sense to have
the P8/P9 specific command in the common include file? They don't need
to be used anywhere else.
+ occ->send_cmd = p8_i2c_occ_send_cmd;
+
+ return occ_setup(occ, "p8_occ");
+}
+
+static const struct of_device_id p8_i2c_occ_of_match[] = {
+ { .compatible = "ibm,p8-occ-hwmon" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
+
+static const unsigned short p8_i2c_occ_addr[] = { 0x50, 0x51,
I2C_CLIENT_END };
Those addresses should never be listed for auto-detection.
Why not? I see lots of drivers in the kernel with a list of I2C
addresses to scan.
+
+static struct i2c_driver p8_i2c_occ_driver = {
+ .class = I2C_CLASS_HWMON,
FWIW, this only adds value if the driver has a detect function.
+ .driver = {
+ .name = "occ-hwmon",
+ .of_match_table = p8_i2c_occ_of_match,
+ },
+ .probe = p8_i2c_occ_probe,
+ .address_list = p8_i2c_occ_addr,
Same here.
+};
+
+module_i2c_driver(p8_i2c_occ_driver);
+
+MODULE_AUTHOR("Eddie James <eajames@xxxxxxxxxx>");
+MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
new file mode 100644
index 0000000..0cef428
--- /dev/null
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -0,0 +1,65 @@
+/*
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "common.h"
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/occ.h>
+
Alphabetic order, and local include file(s) last.
+struct p9_sbe_occ {
+ struct occ occ;
+ struct device *sbe;
+};
+
+#define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ)
+
+static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
+{
+ return -EOPNOTSUPP;
+}
+
+static int p9_sbe_occ_probe(struct platform_device *pdev)
+{
+ struct occ *occ;
+ struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev,
+ sizeof(*p9_sbe_occ),
+ GFP_KERNEL);
Same as above.
+ if (!p9_sbe_occ)
+ return -ENOMEM;
+
+ p9_sbe_occ->sbe = pdev->dev.parent;
+ occ = &p9_sbe_occ->occ;
+ occ->bus_dev = &pdev->dev;
+ platform_set_drvdata(pdev, occ);
+
+ occ->poll_cmd_data = 0x20; /* P9 OCC poll data */
+ occ->send_cmd = p9_sbe_occ_send_cmd;
+
+ return occ_setup(occ, "p9_occ");
+}
+
+static const struct of_device_id p9_sbe_occ_of_match[] = {
+ { .compatible = "ibm,p9-occ-hwmon" },
+ { },
+};
+
+static struct platform_driver p9_sbe_occ_driver = {
+ .driver = {
+ .name = "occ-hwmon",
+ .of_match_table = p9_sbe_occ_of_match,
+ },
+ .probe = p9_sbe_occ_probe,
+};
+
+module_platform_driver(p9_sbe_occ_driver);
+
+MODULE_AUTHOR("Eddie James <eajames@xxxxxxxxxx>");
+MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
+MODULE_LICENSE("GPL");