On-Chip Controller (OCC) hwmon driver

This patchset provides a new OCC hwmon driver. There are a number of issues with the existing driver. Firstly, i2c access was embedded throughout the driver. Secondly, there is no way to easily differentiate versions of the OCC.

This patchset addresses the first issue by abstracting the bus transfer protocol into a modular structure. In this way, any low level transfer method may be easily implemented.

The second issue is addressed by separating the "version specific" code for the OCC and the common hwmon code. This task is not yet complete, but the general structure is in place. Ultimately, different OCC versions could be probed up using the device tree.

Edward A. James (5):
Revert "hwmon: Add Power8 OCC hwmon driver"
drivers: Add hwmon occ driver framework
drivers: hwmon OCC scom bus operations
drivers: Add hwmon OCC version specific functions
drivers: OCC hwmon driver and sysfs

The BMC queries the OCC on POWER8 chips with SCOMs over I2C.

Signed-off-by Andrew Jeffery <[email protected]>

---
 drivers/hwmon/occ/occ_scom_i2c.c | 68 ++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ_scom_i2c.h | 20 ++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h

drivers/hwmon/occ/occ_scom_i2c.c new file +68 -0

new file mode 100644
index 000000000000..b1a060331c4a
@@ -0,0 +1,68 @@
+/*
+ * occ_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "scom.h"
+#include "occ_scom_i2c.h"
+
+#define OCC_I2C_NAME   "occ-p8-i2c"
+
+int occ_i2c_getscom(void *bus, u32 address, u8 *data, size_t offset)
+{
+       ssize_t rc;
+       u64 buf;
+       struct i2c_client *client = bus;
+
+       rc = i2c_master_send(client, (const char *)&address, sizeof(u32));
+       if (rc != sizeof(u32))
+               return -SCOM_WRITE_ERROR;
+
+       rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
+       if (rc != sizeof(u64))
+               return -SCOM_READ_ERROR;
+
+       *((u64 *)&data[offset]) = be64_to_cpu(buf);
+
+       return 0;
+}
+EXPORT_SYMBOL(occ_i2c_getscom);
+
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+       u32 buf[3];
+       ssize_t rc;
+       struct i2c_client *client = bus;
+
+       buf[0] = address;
+       buf[1] = data1;
+       buf[2] = data0;
+
+       rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
+       if (rc != sizeof(u32) * 3)
+               return -SCOM_WRITE_ERROR;
+
+       return 0;
+}
+EXPORT_SYMBOL(occ_i2c_putscom);
+
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("I2C OCC SCOM transport");
+MODULE_LICENSE("GPL");

drivers/hwmon/occ/occ_scom_i2c.h new file +20 -1

new file mode 100644
index 000000000000..92225f4cd584
@@ -0,0 +1,20 @@
+/*
+ * occ_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+int occ_i2c_getscom(void *bus, u32 address, u8 *data, size_t offset);
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
--

2.9.3

Hi Andrew,

Just a general comment on this one. I'm not sure it's worth having an extra layer just to isolate these functions from the p8_i2c_getscom/putscom. Those just do the address shift for P8. But there's no situation where i2c is used that's not on the P8. So this seems like unnecessary complexity to me. I'm not sure, just a thought.

Thanks,
Eddie

Quoted 114 lines · Andrew Jeffery

The BMC queries the OCC on POWER8 chips with SCOMs over I2C.

Quoted 1 line

Quoted 1 line

Signed-off-by Andrew Jeffery <[email protected]>

---
 drivers/hwmon/occ/occ_scom_i2c.c | 68 ++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_scom_i2c.h | 20 ++++++++++++

 2 files changed, 88 insertions(+)

 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h

drivers/hwmon/occ/occ_scom_i2c.c new file +65 -0

new file mode 100644
index 000000000000..b1a060331c4a
@@ -0,0 +1,68 @@
+/*
+ * occ_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "scom.h"
+#include "occ_scom_i2c.h"
+
+#define OCC_I2C_NAME   "occ-p8-i2c"
+
+int occ_i2c_getscom(void *bus, u32 address, u8 *data, size_t offset)
+{
+       ssize_t rc;
+       u64 buf;
+       struct i2c_client *client = bus;
+
+       rc = i2c_master_send(client, (const char *)&address, sizeof(u32));
+       if (rc != sizeof(u32))
+               return -SCOM_WRITE_ERROR;
+
+       rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
+       if (rc != sizeof(u64))
+               return -SCOM_READ_ERROR;
+
+       *((u64 *)&data[offset]) = be64_to_cpu(buf);
+
+       return 0;
+}
+EXPORT_SYMBOL(occ_i2c_getscom);
+
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+       u32 buf[3];
+       ssize_t rc;
+       struct i2c_client *client = bus;
+
+       buf[0] = address;
+       buf[1] = data1;
+       buf[2] = data0;
+
+       rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
+       if (rc != sizeof(u32) * 3)
+               return -SCOM_WRITE_ERROR;
+
+       return 0;
+}
+EXPORT_SYMBOL(occ_i2c_putscom);
+

Quoted 1 line

Quoted 1 line

+MODULE_AUTHOR("Eddie James <[email protected]>");

drivers/hwmon/occ/occ_scom_i2c.c new file +2 -0

+MODULE_DESCRIPTION("I2C OCC SCOM transport");
+MODULE_LICENSE("GPL");

drivers/hwmon/occ/occ_scom_i2c.h new file +20 -1

new file mode 100644
index 000000000000..92225f4cd584
@@ -0,0 +1,20 @@
+/*
+ * occ_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+int occ_i2c_getscom(void *bus, u32 address, u8 *data, size_t offset);
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
--

2.9.3

Signed-off-by Andrew Jeffery <[email protected]>

---
 drivers/hwmon/occ/Kconfig      |  14 +++++
 drivers/hwmon/occ/Makefile     |   1 +
 drivers/hwmon/occ/occ_p8_i2c.c | 133 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+)
 create mode 100644 drivers/hwmon/occ/occ_p8_i2c.c

drivers/hwmon/occ/Kconfig +14 -0

index cdb64a71af6e..085e1f370c42 100644
@@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
 
          This driver can also be built as a module. If so, the module
          will be called occ.
+
+if SENSORS_PPC_OCC
+
+config SENSORS_PPC_OCC_P8_I2C
+       tristate "POWER8 OCC hwmon support"
+       depends on I2C
+       help
+         Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
+         exposing temperature, frequency and power measurements.
+
+         This driver can also be built as a module. If so, the module will be
+         called occ-p8-i2c.
+
+endif

drivers/hwmon/occ/Makefile +1 -0

index a6881f93067c..05a84e987127 100644
@@ -1 +1,2 @@
 obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
+obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o occ_p8_i2c.o

drivers/hwmon/occ/occ_p8_i2c.c new file +33 -0

new file mode 100644
index 000000000000..1b66f302fe16
@@ -0,0 +1,133 @@
+/*
+ * occ_p8_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "scom.h"
+#include "occ_scom_i2c.h"
+#include "occ_p8.h"
+#include "occ_sysfs.h"
+

drivers/hwmon/occ/occ_p8_i2c.c new file +16 -0

+
+int p8_i2c_getscom(void *bus, u32 address, u8 *data, size_t offset)
+{
+       /* P8 i2c slave requires address to be shifted by 1 */
+       address = address << 1;
+
+       return occ_i2c_getscom(bus, address, data, offset);
+}
+
+int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+       /* P8 i2c slave requires address to be shifted by 1 */
+       address = address << 1;
+
+       return occ_i2c_putscom(bus, address, data0, data1);
+}

drivers/hwmon/occ/occ_p8_i2c.c new file +52 -1

+       if (IS_ERR(hwmon))
+               return PTR_ERR(hwmon);
+
+       i2c_set_clientdata(client, hwmon);
+
+       return 0;
+}
+
+static int occ_p8_remove(struct i2c_client *client)
+{
+       return occ_p8_stop(i2c_get_clientdata(client));
+}
+
+/* used by old-style board info. */
+static const struct i2c_device_id occ_ids[] = {
+       { OCC_I2C_NAME, 0 },
+       {}
+};
+MODULE_DEVICE_TABLE(i2c, occ_ids);
+
+/* used by device table */
+static const struct of_device_id occ_of_match[] = {
+       { .compatible = "ibm,occ-p8-i2c" },
+       {}
+};
+MODULE_DEVICE_TABLE(of, occ_of_match);
+
+/*
+ * i2c-core uses i2c-detect() to detect device in below address list.
+ * If exists, address will be assigned to client.
+ * It is also possible to read address from device table.
+ */
+static const unsigned short normal_i2c[] = {0x50, 0x51, I2C_CLIENT_END };
+
+static struct i2c_driver occ_p8_driver = {
+       .class = I2C_CLASS_HWMON,
+       .driver = {
+               .name = OCC_I2C_NAME,
+               .pm = NULL,
+               .of_match_table = occ_of_match,
+       },
+       .probe = occ_p8_probe,
+       .remove = occ_p8_remove,
+       .id_table = occ_ids,
+       .address_list = normal_i2c,
+};
+
+module_i2c_driver(occ_p8_driver);
+
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
+MODULE_LICENSE("GPL");
--

2.9.3

drivers/hwmon/occ/occ_p8_i2c.c

Quoted 12 lines

drivers/hwmon/occ/occ_p8_i2c.c new file +12 -0

+
+static int occ_p8_probe(struct i2c_client *client,
+                        const struct i2c_device_id *id)
+{
+       struct occ *occ;
+       struct occ_sysfs *hwmon;
+
+       occ = occ_p8_start(&client->dev, client, p8_bus_ops);
+       if (IS_ERR(occ))
+               return PTR_ERR(occ);
+
+       hwmon = occ_sysfs_start(&client->dev, occ,

&p8_sysfs_config);

I see why you defined those here. But I guess I don't see why occ_sysfs_config has to be separate from occ_config structure. It could be populated in occ_p8_start and then passed to occ_sysfs_start here.

Quoted 14 lines · Eddie James

drivers/hwmon/occ/occ_p8_i2c.c

Quoted 12 lines

drivers/hwmon/occ/occ_p8_i2c.c new file +12 -0

+
+static int occ_p8_probe(struct i2c_client *client,
+                        const struct i2c_device_id *id)
+{
+       struct occ *occ;
+       struct occ_sysfs *hwmon;
+
+       occ = occ_p8_start(&client->dev, client, p8_bus_ops);
+       if (IS_ERR(occ))
+               return PTR_ERR(occ);
+
+       hwmon = occ_sysfs_start(&client->dev, occ,

&p8_sysfs_config);

Quoted 4 lines

I see why you defined those here. But I guess I don't see why occ_sysfs_config has to be separate from occ_config structure. It could be populated in occ_p8_start and then passed to occ_sysfs_start here.

I admit getting to this point was a little tricky, and I'm not completely satisfied by what I've come up with. Here's my reasoning:

I felt that only half of your struct occ_protocol dealt with SCOM protocol problems, specifically the command_addr and response_addr members. num_caps_fields and caps_names were only used in the sysfs code where they are relevant for interpretation of the payload, but are not in transacting SCOMs. Therefore I removed them from struct occ_protocol.

I also renamed struct occ_protocol to occ_config. I could live with either name but my feeling is it's more obvious that the consumer of the API needs to populate a struct with 'config' in its name.

Given I'd removed num_caps_fields and caps_names I had to put the information somewhere. One place might be in struct occ as I've interpreted your suggestion above. However, I was trying to keep struct occ opaque so in occ.c could fully encapsulate its behaviour. As struct occ is opaque, occ_sysfs.c has no visibility into its members and thus can't access anything we stash in it, e.g. num_caps_fields and caps_names.

That leaves us with needing some other solution, one of which I've implemented above. It achieves the goals I was outlining above and keeps the sysfs code free of any implementation specifics, at the cost of describing the information in a slightly weird spot.

An alternative might be to define struct occ_sysfs_config in occ_p8.c and add an extern to occ_p8.h. That bakes sysfs knowledge into the p8 header which to me feels more unfortunate than my current approach. Or, define say p8_caps_names and p8_num_caps in occ_p8.c and extern those in occ_p8.h and use them to define a struct occ_sysfs_config here. What do you think?

Signed-off-by Andrew Jeffery <[email protected]>

---
 drivers/hwmon/occ/Kconfig      |  9 ++++
 drivers/hwmon/occ/Makefile     |  1 +
 drivers/hwmon/occ/occ_p9_fsi.c | 93 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 drivers/hwmon/occ/occ_p9_fsi.c

drivers/hwmon/occ/Kconfig +9 -0

index 085e1f370c42..97b97b391bf9 100644
@@ -26,4 +26,13 @@ config SENSORS_PPC_OCC_P8_I2C
          This driver can also be built as a module. If so, the module will be
          called occ-p8-i2c.
 
+config SENSORS_PPC_OCC_P9_FSI
+       tristate "POWER9 OCC hwmon support"
+       help
+         Provide a hwmon sysfs interface for the POWER9 On-Chip Controller,
+         exposing temperature, frequency and power measurements.
+
+         This driver can also be built as a module. If so, the module will be
+         called occ-p9-fsi.
+
 endif

drivers/hwmon/occ/Makefile +1 -0

index 05a84e987127..77e47e10195e 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
 obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o occ_p8_i2c.o
+obj-$(CONFIG_SENSORS_PPC_OCC_P9_FSI) += occ_p9.o occ_p9_fsi.o

drivers/hwmon/occ/occ_p9_fsi.c new file +93 -1

new file mode 100644
index 000000000000..2f651d1b16bf
@@ -0,0 +1,93 @@
+/*
+ * occ_p9_fsi.c - hwmon OCC driver
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "scom.h"
+#include "occ_p9.h"
+#include "occ_sysfs.h"
+
+#define OCC_I2C_NAME   "occ-p9-fsi"
+
+static char *caps_sensor_names[] = {
+       "curr_powercap",
+       "curr_powerreading",
+       "norm_powercap",
+       "max_powercap",
+       "min_powercap",
+       "user_powerlimit"
+};
+
+int p9_fsi_getscom(void *bus, u32 address, u8 *data, size_t offset)
+{
+       return -SCOM_READ_ERROR;
+}
+
+int p9_fsi_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+       return -SCOM_WRITE_ERROR;
+}
+
+static struct occ_bus_ops p9_bus_ops = {
+       .getscom = p9_fsi_getscom,
+       .putscom = p9_fsi_putscom,
+};
+
+static struct occ_sysfs_config p9_sysfs_config = {
+       .num_caps_fields = ARRAY_SIZE(caps_sensor_names),
+       .caps_names = caps_sensor_names,
+};
+
+int occ_p9_probe(struct device *dev, void *client)
+{
+       struct occ *occ;
+       struct occ_sysfs *hwmon;
+
+       occ = occ_p9_start(dev, client, p9_bus_ops);
+       if (IS_ERR(occ))
+               return PTR_ERR(occ);
+
+       hwmon = occ_sysfs_start(dev, occ, &p9_sysfs_config);
+       if (IS_ERR(hwmon))
+               return PTR_ERR(hwmon);
+
+       return 0;
+}
+
+int occ_p9_remove(void)
+{
+       struct occ *occ = NULL;
+
+       return occ_p9_stop(occ);
+}
+
+/* used by device table */
+static const struct of_device_id occ_of_match[] = {
+       { .compatible = "ibm,occ-p9-fsi" },
+       {}
+};
+MODULE_DEVICE_TABLE(of, occ_of_match);
+
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
+MODULE_LICENSE("GPL");
--

2.9.3

Hi Andrew,

Looks pretty good. Do we need to get this finalized before getting into our kernel or submitting it upstream? I don't think the SBE or FSI interfaces are nailed down, are they?

Eddie

Quoted 147 lines · Andrew Jeffery

Signed-off-by Andrew Jeffery <[email protected]>

---
 drivers/hwmon/occ/Kconfig      |  9 ++++
 drivers/hwmon/occ/Makefile     |  1 +
 drivers/hwmon/occ/occ_p9_fsi.c | 93
++++++++++++++++++++++++++++++++++++++++++

 3 files changed, 103 insertions(+)

 create mode 100644 drivers/hwmon/occ/occ_p9_fsi.c

drivers/hwmon/occ/Kconfig +0 -0

index 085e1f370c42..97b97b391bf9 100644
@@ -26,4 +26,13 @@ config SENSORS_PPC_OCC_P8_I2C

          This driver can also be built as a module. If so, the module will be
          called occ-p8-i2c.

+config SENSORS_PPC_OCC_P9_FSI
+       tristate "POWER9 OCC hwmon support"
+       help
+         Provide a hwmon sysfs interface for the POWER9 On-Chip Controller,
+         exposing temperature, frequency and power measurements. +
+         This driver can also be built as a module. If so, the module will be
+         called occ-p9-fsi.
+
 endif

drivers/hwmon/occ/Makefile +0 -0

index 05a84e987127..77e47e10195e 100644
@@ -1,2 +1,3 @@

 obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o occ_p8_i2c.o

drivers/hwmon/occ/Makefile +1 -0

+obj-$(CONFIG_SENSORS_PPC_OCC_P9_FSI) += occ_p9.o occ_p9_fsi.o

drivers/hwmon/occ/occ_p9_fsi.c new file +6 -0

new file mode 100644
index 000000000000..2f651d1b16bf
@@ -0,0 +1,93 @@
+/*
+ * occ_p9_fsi.c - hwmon OCC driver
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or

modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */

drivers/hwmon/occ/occ_p9_fsi.c new file +26 -0

+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "scom.h"
+#include "occ_p9.h"
+#include "occ_sysfs.h"
+
+#define OCC_I2C_NAME   "occ-p9-fsi"
+
+static char *caps_sensor_names[] = {
+       "curr_powercap",
+       "curr_powerreading",
+       "norm_powercap",
+       "max_powercap",
+       "min_powercap",
+       "user_powerlimit"
+};
+
+int p9_fsi_getscom(void *bus, u32 address, u8 *data, size_t

offset)

drivers/hwmon/occ/occ_p9_fsi.c new file +52 -1

+{
+       return -SCOM_READ_ERROR;
+}
+
+int p9_fsi_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+       return -SCOM_WRITE_ERROR;
+}
+
+static struct occ_bus_ops p9_bus_ops = {
+       .getscom = p9_fsi_getscom,
+       .putscom = p9_fsi_putscom,
+};
+
+static struct occ_sysfs_config p9_sysfs_config = {
+       .num_caps_fields = ARRAY_SIZE(caps_sensor_names),
+       .caps_names = caps_sensor_names,
+};
+
+int occ_p9_probe(struct device *dev, void *client)
+{
+       struct occ *occ;
+       struct occ_sysfs *hwmon;
+
+       occ = occ_p9_start(dev, client, p9_bus_ops);
+       if (IS_ERR(occ))
+               return PTR_ERR(occ);
+
+       hwmon = occ_sysfs_start(dev, occ, &p9_sysfs_config);
+       if (IS_ERR(hwmon))
+               return PTR_ERR(hwmon);
+
+       return 0;
+}
+
+int occ_p9_remove(void)
+{
+       struct occ *occ = NULL;
+
+       return occ_p9_stop(occ);
+}
+
+/* used by device table */
+static const struct of_device_id occ_of_match[] = {
+       { .compatible = "ibm,occ-p9-fsi" },
+       {}
+};
+MODULE_DEVICE_TABLE(of, occ_of_match);
+
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
+MODULE_LICENSE("GPL");
--

2.9.3

So I have reworked them into what is now 8 patches (see below) and tried

tell a bit more of a story with them (hopefully I succeeded!) Second, I

my hand at reworking the layering as mentioned above, such that hardware interaction is split from sysfs, and that the SCOM transport and sensor configuration is decoupled from the data parsing. You largely had all of

in-place already, I just cut and pasted code around a bit.

Please review the patches and provide feedback! I'd like to hear

or against both of our approaches. Note though that I've only tested that

patches compile, I absolutely have not done any further testing.

Joel: Any chance you could chime in with an architectural review?

When the dust has settled on the bigger issues we can start to look at the finer details of the APIs (there are some bugs, oddities and complexities

I would like to address before they are applied to openbmc/linux).

Hi Eddie,

Quoted 5 lines

This patchset provides a new OCC hwmon driver. There are a number of issues with the existing driver. Firstly, i2c access was embedded throughout the driver. Secondly, there is no way to easily differentiate versions of the OCC.

Quoted 8 lines

This patchset addresses the first issue by abstracting the bus transfer protocol into a modular structure. In this way, any low level transfer method may be easily implemented.

The second issue is addressed by separating the "version specific" code for the OCC and the common hwmon code. This task is not yet complete, but the general structure is in place. Ultimately, different OCC versions could be probed up using the device tree.

I agree with all of this. However, I thought some of the layering could still be improved, so rather than provide a review through prose I figured it might be better for both of us if I mangled the patches directly.

I've attacked two things. The first is I found the series hard to review due to the way the patches were split, as the patches went from trivial to huge (and complex) in a fairly short step:

Quoted 7 lines

Edward A. James (5):
Revert "hwmon: Add Power8 OCC hwmon driver"
drivers: Add hwmon occ driver framework
drivers: hwmon OCC scom bus operations
drivers: Add hwmon OCC version specific functions
drivers: OCC hwmon driver and sysfs

So I have reworked them into what is now 8 patches (see below) and tried to tell a bit more of a story with them (hopefully I succeeded!) Second, I tried my hand at reworking the layering as mentioned above, such that hardware interaction is split from sysfs, and that the SCOM transport and sensor configuration is decoupled from the data parsing. You largely had all of these in-place already, I just cut and pasted code around a bit.

Please review the patches and provide feedback! I'd like to hear arguments for or against both of our approaches. Note though that I've only tested that these patches compile, I absolutely have not done any further testing.

Joel: Any chance you could chime in with an architectural review?

When the dust has settled on the bigger issues we can start to look at the finer details of the APIs (there are some bugs, oddities and complexities that I would like to address before they are applied to openbmc/linux).

Cheers,

Andrew

Andrew Jeffery (8):
hwmon: Add core On-Chip Controller support for POWER CPUs
hwmon: occ: Add sysfs interface
hwmon: occ: Add I2C SCOM transport implementation
hwmon: occ: Add callbacks for parsing P8 OCC datastructures
hwmon: occ: Add hwmon implementation for the P8 OCC
arm: aspeed: dt: Fix OCC compatible strings
hwmon: occ: Add callbacks for parsing P9 OCC datastructures
wip: hwmon: occ: Add sketch of the hwmon implementation for the P9 OCC

Edward A. James (1):
Revert "hwmon: Add Power8 OCC hwmon driver"

 .../devicetree/bindings/i2c/i2c-ibm-occ.txt        |   13 -
 arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts     |    4 +-
 arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts     |    4 +-
 arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts      |    4 +-
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts      |    2 +-
 drivers/hwmon/Kconfig                              |   13 +-
 drivers/hwmon/Makefile                             |    2 +-
 drivers/hwmon/occ/Kconfig                          |   38 +
 drivers/hwmon/occ/Makefile                         |    3 +
 drivers/hwmon/occ/occ.c                            |  494 ++++++++
 drivers/hwmon/occ/occ.h                            |   79 ++
 drivers/hwmon/occ/occ_p8.c                         |  219 ++++
 drivers/hwmon/occ/occ_p8.h                         |   30 +
 drivers/hwmon/occ/occ_p8_i2c.c                     |  133 +++
 drivers/hwmon/occ/occ_p9.c                         |  261 ++++
 drivers/hwmon/occ/occ_p9.h                         |   30 +
 drivers/hwmon/occ/occ_p9_fsi.c                     |   93 ++
 drivers/hwmon/occ/occ_scom_i2c.c                   |   68 ++
 drivers/hwmon/occ/occ_scom_i2c.h                   |   20 +
 drivers/hwmon/occ/occ_sysfs.c                      |  498 ++++++++
 drivers/hwmon/occ/occ_sysfs.h                      |   51 +
 drivers/hwmon/occ/scom.h                           |   50 +
 drivers/hwmon/power8_occ_i2c.c                     | 1254 --------------------
 23 files changed, 2076 insertions(+), 1287 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
 create mode 100644 drivers/hwmon/occ/Kconfig
 create mode 100644 drivers/hwmon/occ/Makefile
 create mode 100644 drivers/hwmon/occ/occ.c
 create mode 100644 drivers/hwmon/occ/occ.h
 create mode 100644 drivers/hwmon/occ/occ_p8.c
 create mode 100644 drivers/hwmon/occ/occ_p8.h
 create mode 100644 drivers/hwmon/occ/occ_p8_i2c.c
 create mode 100644 drivers/hwmon/occ/occ_p9.c
 create mode 100644 drivers/hwmon/occ/occ_p9.h
 create mode 100644 drivers/hwmon/occ/occ_p9_fsi.c
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
 create mode 100644 drivers/hwmon/occ/occ_sysfs.c
 create mode 100644 drivers/hwmon/occ/occ_sysfs.h
 create mode 100644 drivers/hwmon/occ/scom.h
 delete mode 100644 drivers/hwmon/power8_occ_i2c.c

--
2.9.3

Hi Andrew,

Thanks a lot for putting together this new patchset! On the whole, it looks very good! The split between sysfs, occ, and hardware transport layers is solid. I'm pretty satisfied with your modified structuring, although I had a couple of comments and questions. I'll send those out in the applicable patches.

I also tested this on palmetto and barreleye systems and fixed a couple of bugs.

I went ahead and put them in a new patchset, which I'm
about to send out. For reference, here's my only diffs though:

Quoted 7 lines · Eddie James

Hi Andrew,

Thanks a lot for putting together this new patchset! On the whole, it looks very good! The split between sysfs, occ, and hardware transport layers is solid. I'm pretty satisfied with your modified structuring, although I had a couple of comments and questions. I'll send those out in the applicable patches.

I've just finished replying to your comments. The most controversial one was the cap_names/num_cap_names problem, but we have some alternatives there.

Quoted 3 lines

I also tested this on palmetto and barreleye systems and fixed a couple of bugs.

Great!

Quoted 2 lines

I went ahead and put them in a new patchset, which I'm
about to send out. For reference, here's my only diffs though:

Good ol' void conversions eh.

Thanks for testing!

Andrew

Hi Andrew,

What's the status on this? I last sent out a tested patch set that I'm pretty happy with. I'm not sure what the next step for me is, any ideas?