* [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
@ 2017-06-22 22:48 ` Eddie James
2017-06-24 14:15 ` Guenter Roeck
2017-06-26 19:01 ` Rob Herring
2017-06-22 22:48 ` [PATCH 2/7] drivers/hwmon/occ: Add command transport method for P8 and P9 Eddie James
` (6 subsequent siblings)
7 siblings, 2 replies; 17+ messages in thread
From: Eddie James @ 2017-06-22 22:48 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hwmon, devicetree, linux, jdelvare, mark.rutland, robh+dt,
gregkh, cbostic, jk, joel, andrew, eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
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
defines the first common structures and methods between the two OCC
versions.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
.../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
@@ -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";
+ 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
+ ---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"
+#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>
+
+#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;
+ u8 data[OCC_RESP_DATA_BYTES];
+ u16 checksum_be;
+} __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>
+
+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);
+ 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 */
+ 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 };
+
+static struct i2c_driver p8_i2c_occ_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = "occ-hwmon",
+ .of_match_table = p8_i2c_occ_of_match,
+ },
+ .probe = p8_i2c_occ_probe,
+ .address_list = p8_i2c_occ_addr,
+};
+
+module_i2c_driver(p8_i2c_occ_driver);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+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>
+
+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);
+ 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@us.ibm.com>");
+MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
+MODULE_LICENSE("GPL");
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
2017-06-22 22:48 ` [PATCH 1/7] " Eddie James
@ 2017-06-24 14:15 ` Guenter Roeck
2017-06-26 15:06 ` Eddie James
2017-06-26 19:01 ` Rob Herring
1 sibling, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2017-06-24 14:15 UTC (permalink / raw)
To: Eddie James, linux-kernel
Cc: linux-hwmon, devicetree, jdelvare, mark.rutland, robh+dt, gregkh,
cbostic, jk, joel, andrew, Edward A. James
On 06/22/2017 03:48 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> 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 ?
> defines the first common structures and methods between the two OCC
> versions.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
> .../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.
> + 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.
> +
> +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@us.ibm.com>");
> +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@us.ibm.com>");
> +MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
2017-06-24 14:15 ` Guenter Roeck
@ 2017-06-26 15:06 ` Eddie James
2017-06-26 16:38 ` Guenter Roeck
0 siblings, 1 reply; 17+ messages in thread
From: Eddie James @ 2017-06-26 15:06 UTC (permalink / raw)
To: Guenter Roeck, linux-kernel
Cc: linux-hwmon, devicetree, jdelvare, mark.rutland, robh+dt, gregkh,
cbostic, jk, joel, andrew, Edward A. James
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@us.ibm.com>
>>
>> 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@us.ibm.com>
>> ---
>> .../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@us.ibm.com>");
>> +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@us.ibm.com>");
>> +MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
>> +MODULE_LICENSE("GPL");
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
2017-06-26 15:06 ` Eddie James
@ 2017-06-26 16:38 ` Guenter Roeck
0 siblings, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2017-06-26 16:38 UTC (permalink / raw)
To: Eddie James
Cc: linux-kernel, linux-hwmon, devicetree, jdelvare, mark.rutland,
robh+dt, gregkh, cbostic, jk, joel, andrew, Edward A. James
On Mon, Jun 26, 2017 at 10:06:50AM -0500, Eddie James wrote:
>
>
> 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@us.ibm.com>
> >>
> >>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?
>
Not necessarily in -next, but I'll need to have at least an immutable branch
to be able to integrate the series.
> 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@us.ibm.com>
> >>---
> >> .../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.
>
In the source file is ok as well.
> >
> >>+ 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.
>
Sure, but not in the EEPROM address range (0x50..0x57), and not without
detect function. A detect function on an EEPROM is pretty much worthless;
one would only have to write the right (or wrong) sequence of values
into the EEPROM to cause lots of false positive detections.
> >
> >>+
> >>+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@us.ibm.com>");
> >>+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@us.ibm.com>");
> >>+MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
> >>+MODULE_LICENSE("GPL");
> >>
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
2017-06-22 22:48 ` [PATCH 1/7] " Eddie James
2017-06-24 14:15 ` Guenter Roeck
@ 2017-06-26 19:01 ` Rob Herring
1 sibling, 0 replies; 17+ messages in thread
From: Rob Herring @ 2017-06-26 19:01 UTC (permalink / raw)
To: Eddie James
Cc: linux-kernel, linux-hwmon, devicetree, linux, jdelvare,
mark.rutland, gregkh, cbostic, jk, joel, andrew, Edward A. James
On Thu, Jun 22, 2017 at 05:48:30PM -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> 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
> defines the first common structures and methods between the two OCC
> versions.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
> .../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
> @@ -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.
Bindings describe h/w, not drivers. And hwmon is a Linuxism.
> +
> +Required properties:
> + - compatible = "ibm,p9-occ-hwmon";
> +
> +Examples:
> +
> + occ@1 {
> + compatible = "ibm,p9-occ";
> + reg = <1>;
> +
> + occ-hwmon@1 {
> + compatible = "ibm,p9-occ-hwmon";
See my comment in the other version I just reviewed...
> + };
> + };
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/7] drivers/hwmon/occ: Add command transport method for P8 and P9
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
2017-06-22 22:48 ` [PATCH 1/7] " Eddie James
@ 2017-06-22 22:48 ` Eddie James
2017-06-24 14:49 ` Guenter Roeck
2017-06-22 22:48 ` [PATCH 3/7] drivers/hwmon/occ: Parse OCC poll response Eddie James
` (5 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Eddie James @ 2017-06-22 22:48 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hwmon, devicetree, linux, jdelvare, mark.rutland, robh+dt,
gregkh, cbostic, jk, joel, andrew, eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
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 engine.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/hwmon/occ/common.h | 13 ++++
drivers/hwmon/occ/p8_i2c.c | 166 ++++++++++++++++++++++++++++++++++++++++++++-
drivers/hwmon/occ/p9_sbe.c | 66 +++++++++++++++++-
3 files changed, 243 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index dca642a..0c3f26f 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -15,6 +15,19 @@
#define OCC_RESP_DATA_BYTES 4089
+#define OCC_TIMEOUT_MS 5000
+#define OCC_CMD_IN_PRG_MS 100
+
+/* OCC return codes */
+#define RESP_RETURN_CMD_IN_PRG 0xFF
+#define RESP_RETURN_SUCCESS 0
+#define RESP_RETURN_CMD_INVAL 0x11
+#define RESP_RETURN_CMD_LEN 0x12
+#define RESP_RETURN_DATA_INVAL 0x13
+#define RESP_RETURN_CHKSUM 0x14
+#define RESP_RETURN_OCC_ERR 0x15
+#define RESP_RETURN_STATE 0x16
+
/* Same response format for all OCC versions.
* Allocate the largest possible response.
*/
diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
index 5075146..d6d70ce 100644
--- a/drivers/hwmon/occ/p8_i2c.c
+++ b/drivers/hwmon/occ/p8_i2c.c
@@ -7,6 +7,7 @@
* (at your option) any later version.
*/
+#include <asm/unaligned.h>
#include "common.h"
#include <linux/i2c.h>
#include <linux/init.h>
@@ -19,9 +20,172 @@ 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_be;
+ u64 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);
+ msgs[0].buf = (char *)&address;
+
+
+ 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_be;
+
+ rc = i2c_transfer(client->adapter, msgs, 2);
+ if (rc < 0)
+ return rc;
+
+ buf = be64_to_cpu(buf_be);
+ memcpy(data, &buf, sizeof(u64));
+
+ 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;
+
+ 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;
+ struct p8_i2c_occ *p8_i2c_occ = to_p8_i2c_occ(occ);
+ struct i2c_client *client = p8_i2c_occ->client;
+ struct occ_response *resp = &occ->resp;
+
+ start = jiffies;
+
+ /* set sram address for command */
+ rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF6000, 0);
+ if (rc)
+ goto err;
+
+ /* write command (must already be BE), i2c expects cpu-endian */
+ rc = p8_i2c_occ_putscom_be(client, 0x6B075, cmd);
+ if (rc)
+ goto err;
+
+ /* trigger OCC attention */
+ rc = p8_i2c_occ_putscom_u32(client, 0x6B035, 0x20010000, 0);
+ if (rc)
+ goto err;
+
+retry:
+ /* set sram address for response */
+ rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF7000, 0);
+ if (rc)
+ goto err;
+
+ /* read response */
+ rc = p8_i2c_occ_getscom(client, 0x6B075, (u8 *)resp);
+ if (rc)
+ goto err;
+
+ /* check the OCC response */
+ switch (resp->return_status) {
+ case RESP_RETURN_CMD_IN_PRG:
+ if (time_after(jiffies,
+ start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
+ rc = -EALREADY;
+ else {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
+
+ goto retry;
+ }
+ break;
+ case RESP_RETURN_SUCCESS:
+ rc = 0;
+ break;
+ case RESP_RETURN_CMD_INVAL:
+ case RESP_RETURN_CMD_LEN:
+ case RESP_RETURN_DATA_INVAL:
+ case RESP_RETURN_CHKSUM:
+ rc = -EINVAL;
+ break;
+ case RESP_RETURN_OCC_ERR:
+ rc = -EREMOTE;
+ break;
+ default:
+ rc = -EFAULT;
+ }
+
+ if (rc < 0) {
+ dev_warn(&client->dev, "occ bad response: %d\n",
+ resp->return_status);
+ return rc;
+ }
+
+ data_length = get_unaligned_be16(&resp->data_length_be);
+ if (data_length > OCC_RESP_DATA_BYTES) {
+ dev_warn(&client->dev, "occ bad data length: %d\n",
+ data_length);
+ return -EDOM;
+ }
+
+ /* read remaining response */
+ for (i = 8; i < data_length + 7; i += 8) {
+ rc = p8_i2c_occ_getscom(client, 0x6B075, ((u8 *)resp) + i);
+ if (rc)
+ goto err;
+ }
+
+ return data_length + 7;
+
+err:
+ dev_err(&client->dev, "i2c scom op failed rc: %d\n", rc);
+ return rc;
}
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 0cef428..981c53f 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -22,7 +22,71 @@ struct p9_sbe_occ {
static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
{
- return -EOPNOTSUPP;
+ int rc;
+ unsigned long start;
+ struct occ_client *client;
+ struct occ_response *resp = &occ->resp;
+ struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
+
+ start = jiffies;
+
+retry:
+ client = occ_drv_open(p9_sbe_occ->sbe, 0);
+ if (!client)
+ return -ENODEV;
+
+ /* skip first byte (sequence number), OCC driver handles it */
+ rc = occ_drv_write(client, (const char *)&cmd[1], 7);
+ if (rc < 0)
+ goto err;
+
+ rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+ if (rc < 0)
+ goto err;
+
+ occ_drv_release(client);
+
+ /* check the OCC response */
+ switch (resp->return_status) {
+ case RESP_RETURN_CMD_IN_PRG:
+ if (time_after(jiffies,
+ start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
+ rc = -EALREADY;
+ else {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
+
+ goto retry;
+ }
+ break;
+ case RESP_RETURN_SUCCESS:
+ rc = 0;
+ break;
+ case RESP_RETURN_CMD_INVAL:
+ case RESP_RETURN_CMD_LEN:
+ case RESP_RETURN_DATA_INVAL:
+ case RESP_RETURN_CHKSUM:
+ rc = -EINVAL;
+ break;
+ case RESP_RETURN_OCC_ERR:
+ rc = -EREMOTE;
+ break;
+ default:
+ rc = -EFAULT;
+ }
+
+ if (rc < 0) {
+ dev_warn(occ->bus_dev, "occ bad response: %d\n",
+ resp->return_status);
+ return rc;
+ }
+
+ return 0;
+
+err:
+ occ_drv_release(client);
+ dev_err(occ->bus_dev, "occ bus op failed rc: %d\n", rc);
+ return rc;
}
static int p9_sbe_occ_probe(struct platform_device *pdev)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/7] drivers/hwmon/occ: Add command transport method for P8 and P9
2017-06-22 22:48 ` [PATCH 2/7] drivers/hwmon/occ: Add command transport method for P8 and P9 Eddie James
@ 2017-06-24 14:49 ` Guenter Roeck
2017-06-26 15:30 ` Eddie James
0 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2017-06-24 14:49 UTC (permalink / raw)
To: Eddie James, linux-kernel
Cc: linux-hwmon, devicetree, jdelvare, mark.rutland, robh+dt, gregkh,
cbostic, jk, joel, andrew, Edward A. James
On 06/22/2017 03:48 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> 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 engine.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
> drivers/hwmon/occ/common.h | 13 ++++
> drivers/hwmon/occ/p8_i2c.c | 166 ++++++++++++++++++++++++++++++++++++++++++++-
> drivers/hwmon/occ/p9_sbe.c | 66 +++++++++++++++++-
> 3 files changed, 243 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> index dca642a..0c3f26f 100644
> --- a/drivers/hwmon/occ/common.h
> +++ b/drivers/hwmon/occ/common.h
> @@ -15,6 +15,19 @@
>
> #define OCC_RESP_DATA_BYTES 4089
>
> +#define OCC_TIMEOUT_MS 5000
Five seconds ? Isn't that a bit excessive ?
> +#define OCC_CMD_IN_PRG_MS 100
> +
> +/* OCC return codes */
> +#define RESP_RETURN_CMD_IN_PRG 0xFF
> +#define RESP_RETURN_SUCCESS 0
> +#define RESP_RETURN_CMD_INVAL 0x11
> +#define RESP_RETURN_CMD_LEN 0x12
> +#define RESP_RETURN_DATA_INVAL 0x13
> +#define RESP_RETURN_CHKSUM 0x14
> +#define RESP_RETURN_OCC_ERR 0x15
> +#define RESP_RETURN_STATE 0x16
> +
Why are those return codes defined here ? They must be set somewhere
outside this driver, and I would expect them to be defined there.
Also see below - the missing context makes it all but impossible
th review this patch series.
> /* Same response format for all OCC versions.
> * Allocate the largest possible response.
> */
> diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
> index 5075146..d6d70ce 100644
> --- a/drivers/hwmon/occ/p8_i2c.c
> +++ b/drivers/hwmon/occ/p8_i2c.c
> @@ -7,6 +7,7 @@
> * (at your option) any later version.
> */
>
> +#include <asm/unaligned.h>
Alphabetic order, though it is ok to list linux.. includes first followed by
asm... includes followed by local includes.
> #include "common.h"
> #include <linux/i2c.h>
> #include <linux/init.h>
> @@ -19,9 +20,172 @@ 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_be;
_be is redundant. Yes, you have buf as well, but that is really not needed.
> + u64 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);
> + msgs[0].buf = (char *)&address;
No endianness concerns here ?
> +
> +
checkpatch --strict, please
> + 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_be;
> +
> + rc = i2c_transfer(client->adapter, msgs, 2);
> + if (rc < 0)
> + return rc;
> +
> + buf = be64_to_cpu(buf_be);
> + memcpy(data, &buf, sizeof(u64));
*(u64 *)data = be64_to_cpu(buf_be);
> +
> + 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;
> +
> + buf[0] = address;
> + memcpy(&buf[1], &data[4], sizeof(u32));
> + memcpy(&buf[2], data, sizeof(u32));
No endianness concerns ? Presumably not, but that might warrant a comment
above, with the function declaration.
> +
> + 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;
> + struct p8_i2c_occ *p8_i2c_occ = to_p8_i2c_occ(occ);
> + struct i2c_client *client = p8_i2c_occ->client;
> + struct occ_response *resp = &occ->resp;
> +
> + start = jiffies;
> +
> + /* set sram address for command */
> + rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF6000, 0);
Please declare those magics as defines to give readers an idea
what this is about.
> + if (rc)
> + goto err;
> +
> + /* write command (must already be BE), i2c expects cpu-endian */
> + rc = p8_i2c_occ_putscom_be(client, 0x6B075, cmd);
> + if (rc)
> + goto err;
> +
> + /* trigger OCC attention */
> + rc = p8_i2c_occ_putscom_u32(client, 0x6B035, 0x20010000, 0);
> + if (rc)
> + goto err;
> +
> +retry:
> + /* set sram address for response */
> + rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF7000, 0);
> + if (rc)
> + goto err;
> +
> + /* read response */
> + rc = p8_i2c_occ_getscom(client, 0x6B075, (u8 *)resp);
> + if (rc)
> + goto err;
> +
> + /* check the OCC response */
> + switch (resp->return_status) {
> + case RESP_RETURN_CMD_IN_PRG:
> + if (time_after(jiffies,
> + start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
> + rc = -EALREADY;
> + else {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
> +
> + goto retry;
Please refactor as loop.
> + }
> + break;
> + case RESP_RETURN_SUCCESS:
> + rc = 0;
> + break;
> + case RESP_RETURN_CMD_INVAL:
> + case RESP_RETURN_CMD_LEN:
> + case RESP_RETURN_DATA_INVAL:
> + case RESP_RETURN_CHKSUM:
> + rc = -EINVAL;
> + break;
> + case RESP_RETURN_OCC_ERR:
> + rc = -EREMOTE;
"Object is remote"
> + break;
> + default:
> + rc = -EFAULT;
"Bad address" ?
> + }
> +
> + if (rc < 0) {
> + dev_warn(&client->dev, "occ bad response: %d\n",
> + resp->return_status);
> + return rc;
> + }
> +
> + data_length = get_unaligned_be16(&resp->data_length_be);
> + if (data_length > OCC_RESP_DATA_BYTES) {
> + dev_warn(&client->dev, "occ bad data length: %d\n",
> + data_length);
> + return -EDOM;
"Math argument out of domain" ?
Your error codes seem to be kind of random.
> + }
> +
> + /* read remaining response */
> + for (i = 8; i < data_length + 7; i += 8) {
> + rc = p8_i2c_occ_getscom(client, 0x6B075, ((u8 *)resp) + i);
> + if (rc)
> + goto err;
> + }
> +
> + return data_length + 7;
> +
> +err:
> + dev_err(&client->dev, "i2c scom op failed rc: %d\n", rc);
> + return rc;
> }
>
> 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 0cef428..981c53f 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -22,7 +22,71 @@ struct p9_sbe_occ {
>
> static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
> {
> - return -EOPNOTSUPP;
> + int rc;
> + unsigned long start;
> + struct occ_client *client;
> + struct occ_response *resp = &occ->resp;
> + struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
> +
> + start = jiffies;
> +
> +retry:
> + client = occ_drv_open(p9_sbe_occ->sbe, 0);
Where does this come from ? Ah, I see, there is an "include <linux/occ.h>",
but no mention where it comes from.
I'll stop reviewing the series here. I can not review it without complete context.
> + if (!client)
> + return -ENODEV;
> +
> + /* skip first byte (sequence number), OCC driver handles it */
> + rc = occ_drv_write(client, (const char *)&cmd[1], 7);
> + if (rc < 0)
> + goto err;
> +
> + rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
> + if (rc < 0)
> + goto err;
> +
> + occ_drv_release(client);
Might as well cann release() first to save the double call below.
> +
> + /* check the OCC response */
> + switch (resp->return_status) {
> + case RESP_RETURN_CMD_IN_PRG:
> + if (time_after(jiffies,
> + start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
> + rc = -EALREADY;
This is another odd return value. Normally one would expect something like
-ETIMEDOUT.
> + else {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
> +
Does the called code generate an interrupt if the command is complete ?
Makes me wonder why the write/read sequence isn't just synchronous.
Again, missing context.
> + goto retry;
Please refactor as loop.
> + }
> + break;
> + case RESP_RETURN_SUCCESS:
> + rc = 0;
> + break;
> + case RESP_RETURN_CMD_INVAL:
> + case RESP_RETURN_CMD_LEN:
> + case RESP_RETURN_DATA_INVAL:
> + case RESP_RETURN_CHKSUM:
> + rc = -EINVAL;
> + break;
> + case RESP_RETURN_OCC_ERR:
> + rc = -EREMOTE;
> + break;
> + default:
> + rc = -EFAULT;
> + }
> +
> + if (rc < 0) {
> + dev_warn(occ->bus_dev, "occ bad response: %d\n",
> + resp->return_status);
> + return rc;
> + }
> +
> + return 0;
> +
> +err:
> + occ_drv_release(client);
> + dev_err(occ->bus_dev, "occ bus op failed rc: %d\n", rc);
Is all this noise really needed ? I can understand it for debugging,
but for released code ?
> + return rc;
> }
>
> static int p9_sbe_occ_probe(struct platform_device *pdev)
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/7] drivers/hwmon/occ: Add command transport method for P8 and P9
2017-06-24 14:49 ` Guenter Roeck
@ 2017-06-26 15:30 ` Eddie James
2017-06-26 16:55 ` Guenter Roeck
0 siblings, 1 reply; 17+ messages in thread
From: Eddie James @ 2017-06-26 15:30 UTC (permalink / raw)
To: Guenter Roeck, linux-kernel
Cc: linux-hwmon, devicetree, jdelvare, mark.rutland, robh+dt, gregkh,
cbostic, jk, joel, andrew, Edward A. James
On 06/24/2017 09:49 AM, Guenter Roeck wrote:
> On 06/22/2017 03:48 PM, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> 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 engine.
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>> drivers/hwmon/occ/common.h | 13 ++++
>> drivers/hwmon/occ/p8_i2c.c | 166
>> ++++++++++++++++++++++++++++++++++++++++++++-
>> drivers/hwmon/occ/p9_sbe.c | 66 +++++++++++++++++-
>> 3 files changed, 243 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
>> index dca642a..0c3f26f 100644
>> --- a/drivers/hwmon/occ/common.h
>> +++ b/drivers/hwmon/occ/common.h
>> @@ -15,6 +15,19 @@
>> #define OCC_RESP_DATA_BYTES 4089
>> +#define OCC_TIMEOUT_MS 5000
>
> Five seconds ? Isn't that a bit excessive ?
>
>> +#define OCC_CMD_IN_PRG_MS 100
>> +
>> +/* OCC return codes */
>> +#define RESP_RETURN_CMD_IN_PRG 0xFF
>> +#define RESP_RETURN_SUCCESS 0
>> +#define RESP_RETURN_CMD_INVAL 0x11
>> +#define RESP_RETURN_CMD_LEN 0x12
>> +#define RESP_RETURN_DATA_INVAL 0x13
>> +#define RESP_RETURN_CHKSUM 0x14
>> +#define RESP_RETURN_OCC_ERR 0x15
>> +#define RESP_RETURN_STATE 0x16
>> +
>
> Why are those return codes defined here ? They must be set somewhere
> outside this driver, and I would expect them to be defined there.
They are defined in code that runs on the OCC itself, and perhaps in
applications that talk to the OCC, but it's nowhere in the kernel. I'm
not aware of any plans to either define or use these codes anywhere else
in the kernel at the moment.
>
> Also see below - the missing context makes it all but impossible
> th review this patch series.
Agreed, I have sent you links to the FSI client driver series that this
driver uses. Thanks for the review, will include your suggestions in a
v2 soon. Couple of comments below.
Thanks,
Eddie
>
>> /* Same response format for all OCC versions.
>> * Allocate the largest possible response.
>> */
>> diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
>> index 5075146..d6d70ce 100644
>> --- a/drivers/hwmon/occ/p8_i2c.c
>> +++ b/drivers/hwmon/occ/p8_i2c.c
>> @@ -7,6 +7,7 @@
>> * (at your option) any later version.
>> */
>> +#include <asm/unaligned.h>
>
> Alphabetic order, though it is ok to list linux.. includes first
> followed by
> asm... includes followed by local includes.
>
>> #include "common.h"
>> #include <linux/i2c.h>
>> #include <linux/init.h>
>> @@ -19,9 +20,172 @@ 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_be;
>
> _be is redundant. Yes, you have buf as well, but that is really not
> needed.
>
>> + u64 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);
>> + msgs[0].buf = (char *)&address;
>
> No endianness concerns here ?
>
>> +
>> +
> checkpatch --strict, please
>
>> + 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_be;
>> +
>> + rc = i2c_transfer(client->adapter, msgs, 2);
>> + if (rc < 0)
>> + return rc;
>> +
>> + buf = be64_to_cpu(buf_be);
>> + memcpy(data, &buf, sizeof(u64));
>
> *(u64 *)data = be64_to_cpu(buf_be);
>
>> +
>> + 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;
>> +
>> + buf[0] = address;
>> + memcpy(&buf[1], &data[4], sizeof(u32));
>> + memcpy(&buf[2], data, sizeof(u32));
>
> No endianness concerns ? Presumably not, but that might warrant a comment
> above, with the function declaration.
>
>> +
>> + 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;
>> + struct p8_i2c_occ *p8_i2c_occ = to_p8_i2c_occ(occ);
>> + struct i2c_client *client = p8_i2c_occ->client;
>> + struct occ_response *resp = &occ->resp;
>> +
>> + start = jiffies;
>> +
>> + /* set sram address for command */
>> + rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF6000, 0);
>
> Please declare those magics as defines to give readers an idea
> what this is about.
>
>> + if (rc)
>> + goto err;
>> +
>> + /* write command (must already be BE), i2c expects cpu-endian */
>> + rc = p8_i2c_occ_putscom_be(client, 0x6B075, cmd);
>> + if (rc)
>> + goto err;
>> +
>> + /* trigger OCC attention */
>> + rc = p8_i2c_occ_putscom_u32(client, 0x6B035, 0x20010000, 0);
>> + if (rc)
>> + goto err;
>> +
>> +retry:
>> + /* set sram address for response */
>> + rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF7000, 0);
>> + if (rc)
>> + goto err;
>> +
>> + /* read response */
>> + rc = p8_i2c_occ_getscom(client, 0x6B075, (u8 *)resp);
>> + if (rc)
>> + goto err;
>> +
>> + /* check the OCC response */
>> + switch (resp->return_status) {
>> + case RESP_RETURN_CMD_IN_PRG:
>> + if (time_after(jiffies,
>> + start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
>> + rc = -EALREADY;
>> + else {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
>> +
>> + goto retry;
>
> Please refactor as loop.
>
>> + }
>> + break;
>> + case RESP_RETURN_SUCCESS:
>> + rc = 0;
>> + break;
>> + case RESP_RETURN_CMD_INVAL:
>> + case RESP_RETURN_CMD_LEN:
>> + case RESP_RETURN_DATA_INVAL:
>> + case RESP_RETURN_CHKSUM:
>> + rc = -EINVAL;
>> + break;
>> + case RESP_RETURN_OCC_ERR:
>> + rc = -EREMOTE;
>
> "Object is remote"
Closest error code i could find to say "the error is not in the driver,
but on the OCC". I'll add a comment.
>
>> + break;
>> + default:
>> + rc = -EFAULT;
>
> "Bad address" ?
>
>> + }
>> +
>> + if (rc < 0) {
>> + dev_warn(&client->dev, "occ bad response: %d\n",
>> + resp->return_status);
>> + return rc;
>> + }
>> +
>> + data_length = get_unaligned_be16(&resp->data_length_be);
>> + if (data_length > OCC_RESP_DATA_BYTES) {
>> + dev_warn(&client->dev, "occ bad data length: %d\n",
>> + data_length);
>> + return -EDOM;
>
> "Math argument out of domain" ?
>
>
> Your error codes seem to be kind of random.
Tricky to match up the OCC errors with linux errnos.
>
>> + }
>> +
>> + /* read remaining response */
>> + for (i = 8; i < data_length + 7; i += 8) {
>> + rc = p8_i2c_occ_getscom(client, 0x6B075, ((u8 *)resp) + i);
>> + if (rc)
>> + goto err;
>> + }
>> +
>> + return data_length + 7;
>> +
>> +err:
>> + dev_err(&client->dev, "i2c scom op failed rc: %d\n", rc);
>> + return rc;
>> }
>> 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 0cef428..981c53f 100644
>> --- a/drivers/hwmon/occ/p9_sbe.c
>> +++ b/drivers/hwmon/occ/p9_sbe.c
>> @@ -22,7 +22,71 @@ struct p9_sbe_occ {
>> static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>> {
>> - return -EOPNOTSUPP;
>> + int rc;
>> + unsigned long start;
>> + struct occ_client *client;
>> + struct occ_response *resp = &occ->resp;
>> + struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
>> +
>> + start = jiffies;
>> +
>> +retry:
>> + client = occ_drv_open(p9_sbe_occ->sbe, 0);
>
> Where does this come from ? Ah, I see, there is an "include
> <linux/occ.h>",
> but no mention where it comes from.
>
> I'll stop reviewing the series here. I can not review it without
> complete context.
>
>> + if (!client)
>> + return -ENODEV;
>> +
>> + /* skip first byte (sequence number), OCC driver handles it */
>> + rc = occ_drv_write(client, (const char *)&cmd[1], 7);
>> + if (rc < 0)
>> + goto err;
>> +
>> + rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
>> + if (rc < 0)
>> + goto err;
>> +
>> + occ_drv_release(client);
>
> Might as well cann release() first to save the double call below.
>
>> +
>> + /* check the OCC response */
>> + switch (resp->return_status) {
>> + case RESP_RETURN_CMD_IN_PRG:
>> + if (time_after(jiffies,
>> + start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
>> + rc = -EALREADY;
>
> This is another odd return value. Normally one would expect something
> like
> -ETIMEDOUT.
Could do, but EALREADY more closely matches the "command in progress"
return code from the OCC.
>
>> + else {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
>> +
>
> Does the called code generate an interrupt if the command is complete ?
>
> Makes me wonder why the write/read sequence isn't just synchronous.
> Again, missing context.
>
>> + goto retry;
>
> Please refactor as loop.
>
>> + }
>> + break;
>> + case RESP_RETURN_SUCCESS:
>> + rc = 0;
>> + break;
>> + case RESP_RETURN_CMD_INVAL:
>> + case RESP_RETURN_CMD_LEN:
>> + case RESP_RETURN_DATA_INVAL:
>> + case RESP_RETURN_CHKSUM:
>> + rc = -EINVAL;
>> + break;
>> + case RESP_RETURN_OCC_ERR:
>> + rc = -EREMOTE;
>> + break;
>> + default:
>> + rc = -EFAULT;
>> + }
>> +
>> + if (rc < 0) {
>> + dev_warn(occ->bus_dev, "occ bad response: %d\n",
>> + resp->return_status);
>> + return rc;
>> + }
>> +
>> + return 0;
>> +
>> +err:
>> + occ_drv_release(client);
>> + dev_err(occ->bus_dev, "occ bus op failed rc: %d\n", rc);
>
> Is all this noise really needed ? I can understand it for debugging,
> but for released code ?
Errors can come from a lot of different places in this driver, so I
thought it helpful. We'd only get one dev_x print per failure, so
doesn't seem too excessive to me.
>
>> + return rc;
>> }
>> static int p9_sbe_occ_probe(struct platform_device *pdev)
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/7] drivers/hwmon/occ: Add command transport method for P8 and P9
2017-06-26 15:30 ` Eddie James
@ 2017-06-26 16:55 ` Guenter Roeck
0 siblings, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2017-06-26 16:55 UTC (permalink / raw)
To: Eddie James
Cc: linux-kernel, linux-hwmon, devicetree, jdelvare, mark.rutland,
robh+dt, gregkh, cbostic, jk, joel, andrew, Edward A. James
On Mon, Jun 26, 2017 at 10:30:28AM -0500, Eddie James wrote:
>
>
> On 06/24/2017 09:49 AM, Guenter Roeck wrote:
> >On 06/22/2017 03:48 PM, Eddie James wrote:
> >>From: "Edward A. James" <eajames@us.ibm.com>
> >>
> >>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 engine.
> >>
> >>Signed-off-by: Edward A. James <eajames@us.ibm.com>
> >>---
> >> drivers/hwmon/occ/common.h | 13 ++++
> >> drivers/hwmon/occ/p8_i2c.c | 166
> >>++++++++++++++++++++++++++++++++++++++++++++-
> >> drivers/hwmon/occ/p9_sbe.c | 66 +++++++++++++++++-
> >> 3 files changed, 243 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> >>index dca642a..0c3f26f 100644
> >>--- a/drivers/hwmon/occ/common.h
> >>+++ b/drivers/hwmon/occ/common.h
> >>@@ -15,6 +15,19 @@
> >> #define OCC_RESP_DATA_BYTES 4089
> >> +#define OCC_TIMEOUT_MS 5000
> >
> >Five seconds ? Isn't that a bit excessive ?
> >
> >>+#define OCC_CMD_IN_PRG_MS 100
> >>+
> >>+/* OCC return codes */
> >>+#define RESP_RETURN_CMD_IN_PRG 0xFF
> >>+#define RESP_RETURN_SUCCESS 0
> >>+#define RESP_RETURN_CMD_INVAL 0x11
> >>+#define RESP_RETURN_CMD_LEN 0x12
> >>+#define RESP_RETURN_DATA_INVAL 0x13
> >>+#define RESP_RETURN_CHKSUM 0x14
> >>+#define RESP_RETURN_OCC_ERR 0x15
> >>+#define RESP_RETURN_STATE 0x16
> >>+
> >
> >Why are those return codes defined here ? They must be set somewhere
> >outside this driver, and I would expect them to be defined there.
>
> They are defined in code that runs on the OCC itself, and perhaps in
> applications that talk to the OCC, but it's nowhere in the kernel. I'm not
> aware of any plans to either define or use these codes anywhere else in the
> kernel at the moment.
>
Ok.
> >
> >Also see below - the missing context makes it all but impossible
> >th review this patch series.
>
> Agreed, I have sent you links to the FSI client driver series that this
> driver uses. Thanks for the review, will include your suggestions in a v2
> soon. Couple of comments below.
>
> Thanks,
> Eddie
>
> >
> >> /* Same response format for all OCC versions.
> >> * Allocate the largest possible response.
> >> */
> >>diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
> >>index 5075146..d6d70ce 100644
> >>--- a/drivers/hwmon/occ/p8_i2c.c
> >>+++ b/drivers/hwmon/occ/p8_i2c.c
> >>@@ -7,6 +7,7 @@
> >> * (at your option) any later version.
> >> */
> >> +#include <asm/unaligned.h>
> >
> >Alphabetic order, though it is ok to list linux.. includes first followed
> >by
> >asm... includes followed by local includes.
> >
> >> #include "common.h"
> >> #include <linux/i2c.h>
> >> #include <linux/init.h>
> >>@@ -19,9 +20,172 @@ 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_be;
> >
> >_be is redundant. Yes, you have buf as well, but that is really not
> >needed.
> >
> >>+ u64 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);
> >>+ msgs[0].buf = (char *)&address;
> >
> >No endianness concerns here ?
> >
> >>+
> >>+
> >checkpatch --strict, please
> >
> >>+ 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_be;
> >>+
> >>+ rc = i2c_transfer(client->adapter, msgs, 2);
> >>+ if (rc < 0)
> >>+ return rc;
> >>+
> >>+ buf = be64_to_cpu(buf_be);
> >>+ memcpy(data, &buf, sizeof(u64));
> >
> > *(u64 *)data = be64_to_cpu(buf_be);
> >
> >>+
> >>+ 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;
> >>+
> >>+ buf[0] = address;
> >>+ memcpy(&buf[1], &data[4], sizeof(u32));
> >>+ memcpy(&buf[2], data, sizeof(u32));
> >
> >No endianness concerns ? Presumably not, but that might warrant a comment
> >above, with the function declaration.
> >
> >>+
> >>+ 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;
> >>+ struct p8_i2c_occ *p8_i2c_occ = to_p8_i2c_occ(occ);
> >>+ struct i2c_client *client = p8_i2c_occ->client;
> >>+ struct occ_response *resp = &occ->resp;
> >>+
> >>+ start = jiffies;
> >>+
> >>+ /* set sram address for command */
> >>+ rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF6000, 0);
> >
> >Please declare those magics as defines to give readers an idea
> >what this is about.
> >
> >>+ if (rc)
> >>+ goto err;
> >>+
> >>+ /* write command (must already be BE), i2c expects cpu-endian */
> >>+ rc = p8_i2c_occ_putscom_be(client, 0x6B075, cmd);
> >>+ if (rc)
> >>+ goto err;
> >>+
> >>+ /* trigger OCC attention */
> >>+ rc = p8_i2c_occ_putscom_u32(client, 0x6B035, 0x20010000, 0);
> >>+ if (rc)
> >>+ goto err;
> >>+
> >>+retry:
> >>+ /* set sram address for response */
> >>+ rc = p8_i2c_occ_putscom_u32(client, 0x6B070, 0xFFFF7000, 0);
> >>+ if (rc)
> >>+ goto err;
> >>+
> >>+ /* read response */
> >>+ rc = p8_i2c_occ_getscom(client, 0x6B075, (u8 *)resp);
> >>+ if (rc)
> >>+ goto err;
> >>+
> >>+ /* check the OCC response */
> >>+ switch (resp->return_status) {
> >>+ case RESP_RETURN_CMD_IN_PRG:
> >>+ if (time_after(jiffies,
> >>+ start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
> >>+ rc = -EALREADY;
> >>+ else {
> >>+ set_current_state(TASK_INTERRUPTIBLE);
> >>+ schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
> >>+
> >>+ goto retry;
> >
> >Please refactor as loop.
> >
> >>+ }
> >>+ break;
> >>+ case RESP_RETURN_SUCCESS:
> >>+ rc = 0;
> >>+ break;
> >>+ case RESP_RETURN_CMD_INVAL:
> >>+ case RESP_RETURN_CMD_LEN:
> >>+ case RESP_RETURN_DATA_INVAL:
> >>+ case RESP_RETURN_CHKSUM:
> >>+ rc = -EINVAL;
> >>+ break;
> >>+ case RESP_RETURN_OCC_ERR:
> >>+ rc = -EREMOTE;
> >
> >"Object is remote"
>
> Closest error code i could find to say "the error is not in the driver, but
> on the OCC". I'll add a comment.
>
This is not the meaning of this error code. It means that an object is remote,
not that the remote end had an error. You can use EIO or EREMOTEIO, but you
can not redefine existing error codes.
> >
> >>+ break;
> >>+ default:
> >>+ rc = -EFAULT;
> >
> >"Bad address" ?
> >
-EFAULT reflects a bad _memory_ address, while the actual error is that
there is an unknown return code. -EPROTO, maybe, or -EIO if you don't know
any better.
> >>+ }
> >>+
> >>+ if (rc < 0) {
> >>+ dev_warn(&client->dev, "occ bad response: %d\n",
> >>+ resp->return_status);
> >>+ return rc;
> >>+ }
> >>+
> >>+ data_length = get_unaligned_be16(&resp->data_length_be);
> >>+ if (data_length > OCC_RESP_DATA_BYTES) {
> >>+ dev_warn(&client->dev, "occ bad data length: %d\n",
> >>+ data_length);
> >>+ return -EDOM;
> >
> >"Math argument out of domain" ?
> >
> >
> >Your error codes seem to be kind of random.
>
> Tricky to match up the OCC errors with linux errnos.
>
Maybe, but that doesn't mean you can just randomly pick error codes.
The above might be -EPROTO, for example.
> >
> >>+ }
> >>+
> >>+ /* read remaining response */
> >>+ for (i = 8; i < data_length + 7; i += 8) {
> >>+ rc = p8_i2c_occ_getscom(client, 0x6B075, ((u8 *)resp) + i);
> >>+ if (rc)
> >>+ goto err;
> >>+ }
> >>+
> >>+ return data_length + 7;
> >>+
> >>+err:
> >>+ dev_err(&client->dev, "i2c scom op failed rc: %d\n", rc);
> >>+ return rc;
> >> }
> >> 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 0cef428..981c53f 100644
> >>--- a/drivers/hwmon/occ/p9_sbe.c
> >>+++ b/drivers/hwmon/occ/p9_sbe.c
> >>@@ -22,7 +22,71 @@ struct p9_sbe_occ {
> >> static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
> >> {
> >>- return -EOPNOTSUPP;
> >>+ int rc;
> >>+ unsigned long start;
> >>+ struct occ_client *client;
> >>+ struct occ_response *resp = &occ->resp;
> >>+ struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
> >>+
> >>+ start = jiffies;
> >>+
> >>+retry:
> >>+ client = occ_drv_open(p9_sbe_occ->sbe, 0);
> >
> >Where does this come from ? Ah, I see, there is an "include
> ><linux/occ.h>",
> >but no mention where it comes from.
> >
> >I'll stop reviewing the series here. I can not review it without complete
> >context.
> >
> >>+ if (!client)
> >>+ return -ENODEV;
> >>+
> >>+ /* skip first byte (sequence number), OCC driver handles it */
> >>+ rc = occ_drv_write(client, (const char *)&cmd[1], 7);
> >>+ if (rc < 0)
> >>+ goto err;
> >>+
> >>+ rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
> >>+ if (rc < 0)
> >>+ goto err;
> >>+
> >>+ occ_drv_release(client);
> >
> >Might as well cann release() first to save the double call below.
> >
> >>+
> >>+ /* check the OCC response */
> >>+ switch (resp->return_status) {
> >>+ case RESP_RETURN_CMD_IN_PRG:
> >>+ if (time_after(jiffies,
> >>+ start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
> >>+ rc = -EALREADY;
> >
> >This is another odd return value. Normally one would expect something like
> >-ETIMEDOUT.
>
> Could do, but EALREADY more closely matches the "command in progress" return
> code from the OCC.
>
Yes, but it has a different meaning in Linux, and it is only the case
in the first place because you keep resending the request. Logically,
this is a timeout.
> >
> >>+ else {
> >>+ set_current_state(TASK_INTERRUPTIBLE);
> >>+ schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
> >>+
> >
> >Does the called code generate an interrupt if the command is complete ?
> >
> >Makes me wonder why the write/read sequence isn't just synchronous.
> >Again, missing context.
> >
> >>+ goto retry;
> >
> >Please refactor as loop.
> >
> >>+ }
> >>+ break;
> >>+ case RESP_RETURN_SUCCESS:
> >>+ rc = 0;
> >>+ break;
> >>+ case RESP_RETURN_CMD_INVAL:
> >>+ case RESP_RETURN_CMD_LEN:
> >>+ case RESP_RETURN_DATA_INVAL:
> >>+ case RESP_RETURN_CHKSUM:
> >>+ rc = -EINVAL;
> >>+ break;
> >>+ case RESP_RETURN_OCC_ERR:
> >>+ rc = -EREMOTE;
> >>+ break;
> >>+ default:
> >>+ rc = -EFAULT;
> >>+ }
> >>+
> >>+ if (rc < 0) {
> >>+ dev_warn(occ->bus_dev, "occ bad response: %d\n",
> >>+ resp->return_status);
> >>+ return rc;
> >>+ }
> >>+
> >>+ return 0;
> >>+
> >>+err:
> >>+ occ_drv_release(client);
> >>+ dev_err(occ->bus_dev, "occ bus op failed rc: %d\n", rc);
> >
> >Is all this noise really needed ? I can understand it for debugging,
> >but for released code ?
>
> Errors can come from a lot of different places in this driver, so I thought
> it helpful. We'd only get one dev_x print per failure, so doesn't seem too
> excessive to me.
>
Unless the error is permanent or likely to be repeated if seen once,
in which case it fills up the log.
> >
> >>+ return rc;
> >> }
> >> static int p9_sbe_occ_probe(struct platform_device *pdev)
> >>
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/7] drivers/hwmon/occ: Parse OCC poll response
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
2017-06-22 22:48 ` [PATCH 1/7] " Eddie James
2017-06-22 22:48 ` [PATCH 2/7] drivers/hwmon/occ: Add command transport method for P8 and P9 Eddie James
@ 2017-06-22 22:48 ` Eddie James
2017-06-22 22:48 ` [PATCH 4/7] drivers/hwmon/occ: Add sensor types and versions Eddie James
` (4 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2017-06-22 22:48 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hwmon, devicetree, linux, jdelvare, mark.rutland, robh+dt,
gregkh, cbostic, jk, joel, andrew, eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
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: Edward A. James <eajames@us.ibm.com>
---
drivers/hwmon/occ/common.c | 49 +++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/common.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 103 insertions(+)
diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 60c808c..d032ef0 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -28,6 +28,53 @@ 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, 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;
+
+ for (i = 0; i < header->num_sensor_data_blocks; ++i) {
+ block = (struct occ_sensor_data_block *)((u8 *)block + 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;
+ }
+
+ /* 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;
+ }
+}
+
int occ_setup(struct occ *occ, const char *name)
{
int rc;
@@ -39,5 +86,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 0c3f26f..b5b7f02 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -40,10 +40,64 @@ struct occ_response {
u16 checksum_be;
} __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;
+ u32 error_log_start_address_be;
+ u16 error_log_length_be;
+ 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 4/7] drivers/hwmon/occ: Add sensor types and versions
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
` (2 preceding siblings ...)
2017-06-22 22:48 ` [PATCH 3/7] drivers/hwmon/occ: Parse OCC poll response Eddie James
@ 2017-06-22 22:48 ` Eddie James
2017-06-22 22:48 ` [PATCH 5/7] drivers/hwmon/occ: Add sensor attributes and register hwmon device Eddie James
` (3 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2017-06-22 22:48 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hwmon, devicetree, linux, jdelvare, mark.rutland, robh+dt,
gregkh, cbostic, jk, joel, andrew, eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
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: Edward A. James <eajames@us.ibm.com>
---
drivers/hwmon/occ/common.c | 590 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/common.h | 4 +
2 files changed, 594 insertions(+)
diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index d032ef0..b55813a3 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -7,9 +7,109 @@
* (at your option) any later version.
*/
+#include <asm/unaligned.h>
#include "common.h"
#include <linux/hwmon.h>
+/* 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;
@@ -25,9 +125,496 @@ 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;
+ u16 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;
+ }
+
+ 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 * 1000; /* millidegrees */
+ break;
+ case 2:
+ val = temp->fru_type;
+ break;
+ }
+
+ 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;
+ }
+
+ 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;
+ }
+
+ 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;
+ u32 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:
+ val = get_unaligned_be16(&power->value);
+ break;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\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:
+ val = get_unaligned_be16(&power->value);
+ break;
+ case 4:
+ val = power->function_id;
+ break;
+ case 5:
+ val = power->apss_channel;
+ break;
+ }
+
+ 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:
+ val = get_unaligned_be16(&power->system.update_time);
+ break;
+ case 2:
+ val = get_unaligned_be16(&power->system.value);
+ break;
+ case 3:
+ val = get_unaligned_be32(&power->system.update_tag);
+ break;
+ case 4:
+ val = get_unaligned_be64(&power->system.accumulator);
+ break;
+ case 5:
+ val = get_unaligned_be16(&power->proc.update_time);
+ break;
+ case 6:
+ val = get_unaligned_be16(&power->proc.value);
+ break;
+ case 7:
+ val = get_unaligned_be32(&power->proc.update_tag);
+ break;
+ case 8:
+ val = get_unaligned_be64(&power->proc.accumulator);
+ break;
+ case 9:
+ val = get_unaligned_be16(&power->vdd.value);
+ break;
+ case 10:
+ val = get_unaligned_be32(&power->vdd.update_tag);
+ break;
+ case 11:
+ val = get_unaligned_be64(&power->vdd.accumulator);
+ break;
+ case 12:
+ val = get_unaligned_be16(&power->vdn.value);
+ break;
+ case 13:
+ val = get_unaligned_be32(&power->vdn.update_tag);
+ break;
+ case 14:
+ val = get_unaligned_be64(&power->vdn.accumulator);
+ break;
+ }
+
+ 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;
+ }
+
+ 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;
+ }
+
+ 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;
+ }
+
+ 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;
+ }
+
+ 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)
{
@@ -79,6 +666,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 < 0) {
dev_err(occ->bus_dev, "failed to get OCC poll response: %d\n",
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index b5b7f02..f4e1df0 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -15,6 +15,7 @@
#define OCC_RESP_DATA_BYTES 4089
+#define OCC_UPDATE_FREQUENCY msecs_to_jiffies(1000)
#define OCC_TIMEOUT_MS 5000
#define OCC_CMD_IN_PRG_MS 100
@@ -101,6 +102,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;
};
int occ_setup(struct occ *occ, const char *name);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 5/7] drivers/hwmon/occ: Add sensor attributes and register hwmon device
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
` (3 preceding siblings ...)
2017-06-22 22:48 ` [PATCH 4/7] drivers/hwmon/occ: Add sensor types and versions Eddie James
@ 2017-06-22 22:48 ` Eddie James
2017-06-22 22:48 ` [PATCH 6/7] drivers/hwmon/occ: Add non-hwmon attributes Eddie James
` (2 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2017-06-22 22:48 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hwmon, devicetree, linux, jdelvare, mark.rutland, robh+dt,
gregkh, cbostic, jk, joel, andrew, eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Setup the sensor attributes for every OCC sensor found by the first poll
response. Register the attributes with hwmon. Add hwmon documentation
for the driver.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
Documentation/hwmon/occ | 84 ++++++++++
drivers/hwmon/occ/common.c | 395 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/common.h | 14 ++
3 files changed, 493 insertions(+)
create mode 100644 Documentation/hwmon/occ
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
new file mode 100644
index 0000000..85173f2
--- /dev/null
+++ b/Documentation/hwmon/occ
@@ -0,0 +1,84 @@
+Kernel driver occ-hwmon
+=======================
+
+Supported chips:
+ * POWER8
+ * POWER9
+
+Author: Eddie James <eajames@us.ibm.com>
+
+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 will be probed
+automatically 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 if an "ibm,p9-occ-hwmon" compatible device is
+found under the appropriate OCC device node in the device-tree. However, due
+to the way FSI bus probes it's client device drivers, the probe during FSI
+scan may fail by design. In that case, the P9 OCC hwmon driver must be
+instantiated explicitly.
+
+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.
+
+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 watts.
+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_label OCC sensor id.
+power1_system_update_time Time in us that the system power is read.
+power1_system_input Most recent system power reading in watts.
+power1_system_update_tag Number of samples in system accumulator.
+power1_system_accumulator Accumulation of system power readings.
+power1_proc_update_time Time in us that the processor power is read.
+power1_proc_input Most recent processor power reading in watts.
+power1_proc_update_tag Number of samples in processor accumulator.
+power1_proc_accumulator Accumulation of processor power readings.
+power1_vdd_input Most recent proc Vdd power reading in watts.
+power1_vdd_update_tag Number of samples in processor Vdd accumulator.
+power1_vdd_accumulator Accumulation of processor Vdd power readings.
+power1_vdn_input Most recent proc Vdn power reading in watts.
+power1_vdn_update_tag Number of samples in processor Vdn accumulator.
+power1_vnd_accumulator Accumulation of processor Vdn power readings.
+
+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.
diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index b55813a3..eb1f1a1 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -615,6 +615,384 @@ 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;
+ struct device *dev = occ->bus_dev;
+ struct occ_sensors *sensors = &occ->sensors;
+ struct occ_attribute *attr;
+ 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;
+
+ occ->num_attrs = 0;
+
+ switch (sensors->temp.version) {
+ case 1:
+ occ->num_attrs += (sensors->temp.num_sensors * 2);
+ break;
+ case 2:
+ occ->num_attrs += (sensors->temp.num_sensors * 3);
+ 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:
+ occ->num_attrs += (sensors->freq.num_sensors * 2);
+ break;
+ default:
+ sensors->freq.num_sensors = 0;
+ }
+
+ switch (sensors->power.version) {
+ case 1:
+ occ->num_attrs += (sensors->power.num_sensors * 4);
+ break;
+ case 2:
+ occ->num_attrs += (sensors->power.num_sensors * 6);
+ show_power = occ_show_power_2;
+ break;
+ case 0xA0:
+ occ->num_attrs += (sensors->power.num_sensors * 15);
+ show_power = occ_show_power_a0;
+ break;
+ default:
+ sensors->power.num_sensors = 0;
+ }
+
+ switch (sensors->caps.version) {
+ case 1:
+ occ->num_attrs += (sensors->caps.num_sensors * 6);
+ break;
+ case 2:
+ occ->num_attrs += (sensors->caps.num_sensors * 7);
+ show_caps = occ_show_caps_2;
+ break;
+ case 3:
+ occ->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:
+ occ->num_attrs += sensors->extended.num_sensors;
+ break;
+ default:
+ sensors->extended.num_sensors = 0;
+ }
+
+ occ->attrs = devm_kzalloc(dev, sizeof(*occ->attrs) * occ->num_attrs,
+ GFP_KERNEL);
+ if (!occ->attrs)
+ return -ENOMEM;
+
+ /* null-terminated list */
+ occ->group.attrs = devm_kzalloc(dev, sizeof(*occ->group.attrs) *
+ occ->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;
+
+ snprintf(attr->name, sizeof(attr->name), "temp%d_label", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444, show_temp, NULL,
+ 0, i);
+ attr++;
+
+ 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++;
+ }
+ }
+
+ 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) {
+ 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_system_update_time", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 1, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_system_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 2, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_system_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 3, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_system_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 4, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_proc_update_time", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 5, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_proc_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 6, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_proc_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 7, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_proc_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 8, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_vdd_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 9, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_vdd_update_tag", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 10, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_vdd_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 11, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_vdn_input", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 12, i);
+ attr++;
+
+ snprintf(attr->name, sizeof(attr->name),
+ "power%d_vdn_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_vdn_accumulator", s);
+ attr->sensor = OCC_INIT_ATTR(attr->name, 0444,
+ show_power, NULL, 14, 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 < occ->num_attrs; ++i)
+ 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)
{
@@ -667,6 +1045,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);
@@ -678,5 +1057,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 f4e1df0..4bcf3ca 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -94,6 +94,14 @@ 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;
@@ -105,6 +113,12 @@ struct occ {
unsigned long last_update;
struct mutex lock;
+
+ struct device *hwmon;
+ unsigned int num_attrs;
+ 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 6/7] drivers/hwmon/occ: Add non-hwmon attributes
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
` (4 preceding siblings ...)
2017-06-22 22:48 ` [PATCH 5/7] drivers/hwmon/occ: Add sensor attributes and register hwmon device Eddie James
@ 2017-06-22 22:48 ` Eddie James
2017-06-22 22:48 ` [PATCH 7/7] drivers/hwmon/occ: Add error handling Eddie James
2017-06-23 4:52 ` [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Guenter Roeck
7 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2017-06-22 22:48 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hwmon, devicetree, linux, jdelvare, mark.rutland, robh+dt,
gregkh, cbostic, jk, joel, andrew, eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Create device attributes for additional OCC properties that do not
belong as hwmon sensors. These provide additional information as to the
state of the processor and system.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
Documentation/ABI/testing/sysfs-driver-occ-hwmon | 65 +++++++++++++
drivers/hwmon/occ/common.c | 114 +++++++++++++++++++++++
drivers/hwmon/occ/common.h | 14 +++
drivers/hwmon/occ/p8_i2c.c | 8 ++
drivers/hwmon/occ/p9_sbe.c | 8 ++
5 files changed, 209 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-driver-occ-hwmon
diff --git a/Documentation/ABI/testing/sysfs-driver-occ-hwmon b/Documentation/ABI/testing/sysfs-driver-occ-hwmon
new file mode 100644
index 0000000..ddf6cd7
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-occ-hwmon
@@ -0,0 +1,65 @@
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_active
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates (with a "1" or a "0",
+ respectively) whether or not this OCC is in the "active" state.
+
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_dvfs_ot
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates (with a "1" or a "0",
+ respectively) whether or not this OCC has limited the processor
+ frequency due to over-temperature.
+
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_dvfs_power
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates (with a "1" or a "0",
+ respectively) whether or not this OCC has limited the processor
+ frequency due to power usage.
+
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_master
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates (with a "1" or a "0",
+ respectively) whether or not this OCC is the "master" OCC.
+
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_mem_throttle
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates (with a "1" or a "0",
+ respectively) whether or not the OCC has throttled memory due
+ to over-temperature.
+
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_quick_drop
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates (with a "1" or a "0",
+ respectively) whether or not this OCC has asserted the "quick
+ power drop" signal.
+
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_status
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates the current OCC state. The
+ value of the attribute will be one of the following states:
+ 0: Reserved
+ 1: Standby
+ 2: Observation
+ 3: Active
+ 4: Safe
+ 5: Characterization
diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index eb1f1a1..1645776 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -993,6 +993,102 @@ static int occ_setup_sensor_attrs(struct occ *occ)
return 0;
}
+static ssize_t occ_show_status(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int rc;
+ int val;
+ struct occ *occ = dev_get_drvdata(dev);
+ struct occ_poll_response_header *header;
+ struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
+
+ rc = occ_update_response(occ);
+ if (rc)
+ return rc;
+
+ header = (struct occ_poll_response_header *)occ->resp.data;
+
+ switch (sattr->index) {
+ case 0:
+ val = (header->status & OCC_STAT_MASTER) ? 1 : 0;
+ break;
+ case 1:
+ val = (header->status & OCC_STAT_ACTIVE) ? 1 : 0;
+ break;
+ case 2:
+ val = (header->ext_status & OCC_EXT_STAT_DVFS_OT) ? 1 : 0;
+ break;
+ case 3:
+ val = (header->ext_status & OCC_EXT_STAT_DVFS_POWER) ? 1 : 0;
+ break;
+ case 4:
+ val = (header->ext_status & OCC_EXT_STAT_MEM_THROTTLE) ? 1 : 0;
+ break;
+ case 5:
+ val = (header->ext_status & OCC_EXT_STAT_QUICK_DROP) ? 1 : 0;
+ break;
+ case 6:
+ val = header->occ_state;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static int occ_create_status_attrs(struct occ *occ)
+{
+ int rc, i;
+ struct device *dev = occ->bus_dev;
+
+ occ->status_attrs = devm_kzalloc(dev, sizeof(*occ->status_attrs) *
+ OCC_NUM_STATUS_ATTRS, GFP_KERNEL);
+ if (!occ->status_attrs)
+ return -ENOMEM;
+
+ occ->status_attrs[0] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_master, 0444,
+ occ_show_status,
+ NULL, 0);
+ occ->status_attrs[1] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_active, 0444,
+ occ_show_status,
+ NULL, 1);
+ occ->status_attrs[2] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_dvfs_ot, 0444,
+ occ_show_status,
+ NULL, 2);
+ occ->status_attrs[3] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_dvfs_power,
+ 0444,
+ occ_show_status,
+ NULL, 3);
+ occ->status_attrs[4] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_mem_throttle,
+ 0444,
+ occ_show_status,
+ NULL, 4);
+ occ->status_attrs[5] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_quick_drop,
+ 0444,
+ occ_show_status,
+ NULL, 5);
+ occ->status_attrs[6] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_status, 0444,
+ occ_show_status,
+ NULL, 6);
+
+ for (i = 0; i < OCC_NUM_STATUS_ATTRS; ++i) {
+ rc = device_create_file(dev, &occ->status_attrs[i].dev_attr);
+ if (rc)
+ dev_warn(dev, "error %d creating status attr %d\n", rc,
+ i);
+ }
+
+ 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)
{
@@ -1073,5 +1169,23 @@ int occ_setup(struct occ *occ, const char *name)
return rc;
}
+ rc = occ_create_status_attrs(occ);
+ if (rc) {
+ dev_err(occ->bus_dev, "failed to setup status attrs: %d\n",
+ rc);
+ return rc;
+ }
+
+ return 0;
+}
+
+int occ_shutdown(struct occ *occ)
+{
+ int i;
+
+ for (i = 0; i < OCC_NUM_STATUS_ATTRS; ++i)
+ device_remove_file(occ->bus_dev,
+ &occ->status_attrs[i].dev_attr);
+
return 0;
}
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index 4bcf3ca..dd23eac 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -13,6 +13,8 @@
#include <linux/hwmon-sysfs.h>
#include <linux/sysfs.h>
+#define OCC_NUM_STATUS_ATTRS 7
+
#define OCC_RESP_DATA_BYTES 4089
#define OCC_UPDATE_FREQUENCY msecs_to_jiffies(1000)
@@ -29,6 +31,14 @@
#define RESP_RETURN_OCC_ERR 0x15
#define RESP_RETURN_STATE 0x16
+/* OCC status bits */
+#define OCC_STAT_MASTER 0x80
+#define OCC_STAT_ACTIVE 0x01
+#define OCC_EXT_STAT_DVFS_OT 0x80
+#define OCC_EXT_STAT_DVFS_POWER 0x40
+#define OCC_EXT_STAT_MEM_THROTTLE 0x20
+#define OCC_EXT_STAT_QUICK_DROP 0x10
+
/* Same response format for all OCC versions.
* Allocate the largest possible response.
*/
@@ -119,8 +129,12 @@ struct occ {
struct occ_attribute *attrs;
struct attribute_group group;
const struct attribute_group *groups[2];
+
+ /* non-hwmon attributes for more OCC properties */
+ struct sensor_device_attribute *status_attrs;
};
int occ_setup(struct occ *occ, const char *name);
+int occ_shutdown(struct occ *occ);
#endif /* OCC_COMMON_H */
diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
index d6d70ce..cab4448 100644
--- a/drivers/hwmon/occ/p8_i2c.c
+++ b/drivers/hwmon/occ/p8_i2c.c
@@ -209,6 +209,13 @@ static int p8_i2c_occ_probe(struct i2c_client *client,
return occ_setup(occ, "p8_occ");
}
+static int p8_i2c_occ_remove(struct i2c_client *client)
+{
+ struct occ *occ = dev_get_drvdata(&client->dev);
+
+ return occ_shutdown(occ);
+}
+
static const struct of_device_id p8_i2c_occ_of_match[] = {
{ .compatible = "ibm,p8-occ-hwmon" },
{}
@@ -224,6 +231,7 @@ static int p8_i2c_occ_probe(struct i2c_client *client,
.of_match_table = p8_i2c_occ_of_match,
},
.probe = p8_i2c_occ_probe,
+ .remove = p8_i2c_occ_remove,
.address_list = p8_i2c_occ_addr,
};
diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 981c53f..72ee9b4 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -109,6 +109,13 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
return occ_setup(occ, "p9_occ");
}
+static int p9_sbe_occ_remove(struct platform_device *pdev)
+{
+ struct occ *occ = platform_get_drvdata(pdev);
+
+ return occ_shutdown(occ);
+}
+
static const struct of_device_id p9_sbe_occ_of_match[] = {
{ .compatible = "ibm,p9-occ-hwmon" },
{ },
@@ -120,6 +127,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
.of_match_table = p9_sbe_occ_of_match,
},
.probe = p9_sbe_occ_probe,
+ .remove = p9_sbe_occ_remove,
};
module_platform_driver(p9_sbe_occ_driver);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 7/7] drivers/hwmon/occ: Add error handling
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
` (5 preceding siblings ...)
2017-06-22 22:48 ` [PATCH 6/7] drivers/hwmon/occ: Add non-hwmon attributes Eddie James
@ 2017-06-22 22:48 ` Eddie James
2017-06-23 4:52 ` [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Guenter Roeck
7 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2017-06-22 22:48 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hwmon, devicetree, linux, jdelvare, mark.rutland, robh+dt,
gregkh, cbostic, jk, joel, andrew, eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Add logic to detect a number of error scenarios on the OCC. Export any
error through an additional non-hwmon device attribute.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
Documentation/ABI/testing/sysfs-driver-occ-hwmon | 12 ++++++
drivers/hwmon/occ/common.c | 53 +++++++++++++++++++++++-
drivers/hwmon/occ/common.h | 13 +++++-
drivers/hwmon/occ/p8_i2c.c | 10 ++++-
drivers/hwmon/occ/p9_sbe.c | 9 +++-
5 files changed, 93 insertions(+), 4 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-occ-hwmon b/Documentation/ABI/testing/sysfs-driver-occ-hwmon
index ddf6cd7..9e2be27 100644
--- a/Documentation/ABI/testing/sysfs-driver-occ-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-occ-hwmon
@@ -24,6 +24,18 @@ Description:
respectively) whether or not this OCC has limited the processor
frequency due to power usage.
+What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_error
+Date: June 2017
+KernelVersion: 4.14
+Contact: eajames@us.ibm.com
+Description:
+ A read-only attribute that indicates any error condition
+ observed by the OCC or detected by the driver. Reading the
+ attribute will return an integer. A positive integer indicates
+ an error response from the OCC. A negative integer indicates a
+ possible bus error or other error condition detected by the
+ driver. A "0" indicates no error.
+
What: /sys/bus/platform/drivers/occ-hwmon/<dev>/occ_master
Date: June 2017
KernelVersion: 4.14
diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 1645776..f124f87 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -11,6 +11,9 @@
#include "common.h"
#include <linux/hwmon.h>
+/* counter so we can verify against count from OCC response */
+static atomic_t occ_num_occs = ATOMIC_INIT(0);
+
/* OCC sensor type and version definitions */
struct temp_sensor_1 {
@@ -112,6 +115,9 @@ struct extended_sensor {
static int occ_poll(struct occ *occ)
{
+ int rc;
+ struct occ_poll_response_header *header =
+ (struct occ_poll_response_header *)occ->resp.data;
u16 checksum = occ->poll_cmd_data + 1;
u8 cmd[8];
@@ -126,7 +132,32 @@ static int occ_poll(struct occ *occ)
cmd[7] = 0;
/* mutex should already be locked if necessary */
- return occ->send_cmd(occ, cmd);
+ rc = occ->send_cmd(occ, cmd);
+ if (rc < 0)
+ return rc;
+
+ /* check for "safe" state */
+ if (header->occ_state == OCC_STATE_SAFE) {
+ if (occ->last_safe) {
+ if (time_after(jiffies,
+ occ->last_safe + OCC_SAFE_TIMEOUT))
+ occ->error = -EHOSTDOWN;
+ } else
+ occ->last_safe = jiffies;
+ } else
+ occ->last_safe = 0;
+
+ /* verify number of present OCCs */
+ if (header->status & OCC_STAT_MASTER) {
+ if (hweight8(header->occs_present) !=
+ atomic_read(&occ_num_occs)) {
+ occ->error = -EXDEV;
+ occ->bad_present_count++;
+ } else
+ occ->bad_present_count = 0;
+ }
+
+ return rc;
}
static int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
@@ -993,6 +1024,19 @@ static int occ_setup_sensor_attrs(struct occ *occ)
return 0;
}
+static ssize_t occ_show_error(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int error = 0;
+ struct occ *occ = dev_get_drvdata(dev);
+
+ if (occ->error_count > OCC_ERROR_COUNT_THRESHOLD || occ->last_safe ||
+ occ->bad_present_count > OCC_ERROR_COUNT_THRESHOLD)
+ error = occ->error;
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", error);
+}
+
static ssize_t occ_show_status(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -1078,6 +1122,10 @@ static int occ_create_status_attrs(struct occ *occ)
(struct sensor_device_attribute)SENSOR_ATTR(occ_status, 0444,
occ_show_status,
NULL, 6);
+ occ->status_attrs[7] =
+ (struct sensor_device_attribute)SENSOR_ATTR(occ_error, 0444,
+ occ_show_error,
+ NULL, 0);
for (i = 0; i < OCC_NUM_STATUS_ATTRS; ++i) {
rc = device_create_file(dev, &occ->status_attrs[i].dev_attr);
@@ -1140,6 +1188,7 @@ int occ_setup(struct occ *occ, const char *name)
{
int rc;
+ atomic_inc(&occ_num_occs);
mutex_init(&occ->lock);
occ->groups[0] = &occ->group;
@@ -1187,5 +1236,7 @@ int occ_shutdown(struct occ *occ)
device_remove_file(occ->bus_dev,
&occ->status_attrs[i].dev_attr);
+ atomic_dec(&occ_num_occs);
+
return 0;
}
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index dd23eac..cd04ee0 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -13,10 +13,13 @@
#include <linux/hwmon-sysfs.h>
#include <linux/sysfs.h>
-#define OCC_NUM_STATUS_ATTRS 7
+#define OCC_ERROR_COUNT_THRESHOLD 2
+
+#define OCC_NUM_STATUS_ATTRS 8
#define OCC_RESP_DATA_BYTES 4089
+#define OCC_SAFE_TIMEOUT msecs_to_jiffies(60000) /* 1 min */
#define OCC_UPDATE_FREQUENCY msecs_to_jiffies(1000)
#define OCC_TIMEOUT_MS 5000
#define OCC_CMD_IN_PRG_MS 100
@@ -39,6 +42,9 @@
#define OCC_EXT_STAT_MEM_THROTTLE 0x20
#define OCC_EXT_STAT_QUICK_DROP 0x10
+/* OCC state enumeration */
+#define OCC_STATE_SAFE 4
+
/* Same response format for all OCC versions.
* Allocate the largest possible response.
*/
@@ -132,6 +138,11 @@ struct occ {
/* non-hwmon attributes for more OCC properties */
struct sensor_device_attribute *status_attrs;
+
+ int error;
+ unsigned int error_count; /* num errors observed */
+ unsigned int bad_present_count; /* num polls w/bad num occs */
+ unsigned long last_safe; /* time entered safe state */
};
int occ_setup(struct occ *occ, const char *name);
diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
index cab4448..a915b79 100644
--- a/drivers/hwmon/occ/p8_i2c.c
+++ b/drivers/hwmon/occ/p8_i2c.c
@@ -161,7 +161,10 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
rc = -EFAULT;
}
+ occ->error = resp->return_status;
+
if (rc < 0) {
+ occ->error_count++;
dev_warn(&client->dev, "occ bad response: %d\n",
resp->return_status);
return rc;
@@ -169,9 +172,11 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
data_length = get_unaligned_be16(&resp->data_length_be);
if (data_length > OCC_RESP_DATA_BYTES) {
+ occ->error_count++;
+ occ->error = -EDOM;
dev_warn(&client->dev, "occ bad data length: %d\n",
data_length);
- return -EDOM;
+ return occ->error;
}
/* read remaining response */
@@ -181,9 +186,12 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
goto err;
}
+ occ->error_count = 0;
return data_length + 7;
err:
+ occ->error_count++;
+ occ->error = rc;
dev_err(&client->dev, "i2c scom op failed rc: %d\n", rc);
return rc;
}
diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 72ee9b4..5b5885e 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -9,6 +9,7 @@
#include "common.h"
#include <linux/init.h>
+#include <linux/hwmon.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/occ.h>
@@ -33,7 +34,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
retry:
client = occ_drv_open(p9_sbe_occ->sbe, 0);
if (!client)
- return -ENODEV;
+ return -ENODEV; /* don't increment error counter */
/* skip first byte (sequence number), OCC driver handles it */
rc = occ_drv_write(client, (const char *)&cmd[1], 7);
@@ -75,15 +76,21 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
rc = -EFAULT;
}
+ occ->error = resp->return_status;
+
if (rc < 0) {
+ occ->error_count++;
dev_warn(occ->bus_dev, "occ bad response: %d\n",
resp->return_status);
return rc;
}
+ occ->error_count = 0;
return 0;
err:
+ occ->error_count++;
+ occ->error = rc;
occ_drv_release(client);
dev_err(occ->bus_dev, "occ bus op failed rc: %d\n", rc);
return rc;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
2017-06-22 22:48 [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Eddie James
` (6 preceding siblings ...)
2017-06-22 22:48 ` [PATCH 7/7] drivers/hwmon/occ: Add error handling Eddie James
@ 2017-06-23 4:52 ` Guenter Roeck
2017-06-23 13:38 ` Eddie James
7 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2017-06-23 4:52 UTC (permalink / raw)
To: Eddie James, linux-kernel
Cc: linux-hwmon, devicetree, jdelvare, mark.rutland, robh+dt, gregkh,
cbostic, jk, joel, andrew, Edward A. James
On 06/22/2017 03:48 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> 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.
>
> This driver has two different platform drivers as a "base" for the
> hwmon stuff, as the means of communicating with the OCC on P8 and P9 is
> completely different. For P8, the driver is an I2C client driver. For P9 the
> driver is an FSI-based OCC client driver, and uses the OCC driver in-kernel
> API.
>
> There was a previous version of this driver that wasn't written with the
> differences in communication methods between the two versions in mind. This
> driver has been considerably simplified.
>
I thought I did see this before.
It is customary to use "v2" in such situations, and add a change log.
You expect me to go into the two versions and compare them to figure
out what changed to evaluate if it makes sense. Do you really believe that
I have enough time to do that, and that I would be willing to spend that
time in the first place ?
Presumably you know what changed. Why not just tell me ?
Guenter
> Edward A. James (7):
> drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
> drivers/hwmon/occ: Add command transport method for P8 and P9
> drivers/hwmon/occ: Parse OCC poll response
> drivers/hwmon/occ: Add sensor types and versions
> drivers/hwmon/occ: Add sensor attributes and register hwmon device
> drivers/hwmon/occ: Add non-hwmon attributes
> drivers/hwmon/occ: Add error handling
>
> Documentation/ABI/testing/sysfs-driver-occ-hwmon | 77 ++
> .../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 +
> .../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 +
> Documentation/hwmon/occ | 84 ++
> drivers/hwmon/Kconfig | 2 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/occ/Kconfig | 28 +
> drivers/hwmon/occ/Makefile | 11 +
> drivers/hwmon/occ/common.c | 1242 ++++++++++++++++++++
> drivers/hwmon/occ/common.h | 151 +++
> drivers/hwmon/occ/p8_i2c.c | 250 ++++
> drivers/hwmon/occ/p9_sbe.c | 144 +++
> 12 files changed, 2033 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-occ-hwmon
> 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 Documentation/hwmon/occ
> 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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
2017-06-23 4:52 ` [PATCH 0/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver Guenter Roeck
@ 2017-06-23 13:38 ` Eddie James
0 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2017-06-23 13:38 UTC (permalink / raw)
To: Guenter Roeck, linux-kernel
Cc: linux-hwmon, devicetree, jdelvare, mark.rutland, robh+dt, gregkh,
cbostic, jk, joel, andrew, Edward A. James
On 06/22/2017 11:52 PM, Guenter Roeck wrote:
> On 06/22/2017 03:48 PM, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> 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.
>>
>> This driver has two different platform drivers as a "base" for the
>> hwmon stuff, as the means of communicating with the OCC on P8 and P9 is
>> completely different. For P8, the driver is an I2C client driver. For
>> P9 the
>> driver is an FSI-based OCC client driver, and uses the OCC driver
>> in-kernel
>> API.
>>
>> There was a previous version of this driver that wasn't written with the
>> differences in communication methods between the two versions in
>> mind. This
>> driver has been considerably simplified.
>>
>
> I thought I did see this before.
>
> It is customary to use "v2" in such situations, and add a change log.
> You expect me to go into the two versions and compare them to figure
> out what changed to evaluate if it makes sense. Do you really believe
> that
> I have enough time to do that, and that I would be willing to spend that
> time in the first place ?
>
> Presumably you know what changed. Why not just tell me ?
Hi,
This driver does not build upon the previous patch set at all, so I
didn't mark it as a v2. It is completely rewritten from scratch. I sent
a note saying that the previous patch set was abandoned. There may be
some similarities with the previous one, simply because the drivers do
the same thing, but I don't think there is any need to compare it with
the old driver I mailed out.
Thanks for your time,
Eddie
>
> Guenter
>
>> Edward A. James (7):
>> drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver
>> drivers/hwmon/occ: Add command transport method for P8 and P9
>> drivers/hwmon/occ: Parse OCC poll response
>> drivers/hwmon/occ: Add sensor types and versions
>> drivers/hwmon/occ: Add sensor attributes and register hwmon device
>> drivers/hwmon/occ: Add non-hwmon attributes
>> drivers/hwmon/occ: Add error handling
>>
>> Documentation/ABI/testing/sysfs-driver-occ-hwmon | 77 ++
>> .../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 +
>> .../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 +
>> Documentation/hwmon/occ | 84 ++
>> drivers/hwmon/Kconfig | 2 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/occ/Kconfig | 28 +
>> drivers/hwmon/occ/Makefile | 11 +
>> drivers/hwmon/occ/common.c | 1242
>> ++++++++++++++++++++
>> drivers/hwmon/occ/common.h | 151 +++
>> drivers/hwmon/occ/p8_i2c.c | 250 ++++
>> drivers/hwmon/occ/p9_sbe.c | 144 +++
>> 12 files changed, 2033 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-driver-occ-hwmon
>> 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 Documentation/hwmon/occ
>> 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
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread