hwmon: Add core On-Chip Controller support for POWER CPUs
Subsystems (2)
PATCH 1/7 hwmon: Add core On-Chip Controller support for POWER CPUs
From: "Edward A. James" <[email protected]>
Add core support for polling the OCC for it's sensor data and parsing that data into sensor-specific information.
--- Documentation/hwmon/occ | 40 ++++ drivers/hwmon/Kconfig | 2 + drivers/hwmon/Makefile | 1 + drivers/hwmon/occ/Kconfig | 15 ++ drivers/hwmon/occ/Makefile | 1 + drivers/hwmon/occ/occ.c | 519 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ.h | 86 ++++++++ drivers/hwmon/occ/scom.h | 48 +++++ 8 files changed, 712 insertions(+) create mode 100644 Documentation/hwmon/occ create mode 100644 drivers/hwmon/occ/Kconfig create mode 100644 drivers/hwmon/occ/Makefile create mode 100644 drivers/hwmon/occ/occ.c create mode 100644 drivers/hwmon/occ/occ.h create mode 100644 drivers/hwmon/occ/scom.h
Documentation/hwmon/occ new file +40 -0
@@ -0,0 +1,40 @@ +Kernel driver occ +================= + +Supported chips: + * ASPEED AST2400 + * ASPEED AST2500 + +Please note that the chip must be connected to a POWER8 or POWER9 processor +(see the BMC - Host Communications section). + +Author: Eddie James <[email protected]> + +Description +----------- + +This driver implements support for the OCC (On-Chip Controller) on the IBM +POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The +OCC is an embedded processor that provides real time power and thermal +monitoring. + +This driver provides an interface on a BMC to poll OCC sensor data, set user +power caps, and perform some basic OCC error handling. + +Currently, all versions of the OCC support four types of sensor data: power, +temperature, frequency, and "caps," which indicate limits and thresholds used +internally on the OCC. + +BMC - Host Communications +------------------------- + +For the POWER8 application, the BMC can communicate with the P8 over I2C bus. +However, to access the OCC register space, any data transfer must use a SCOM +operation. SCOM is a procedure to initiate a data transfer, typically of 8 +bytes. SCOMs consist of writing a 32-bit command register and then +reading/writing two 32-bit data registers. This driver implements these +SCOM operations over I2C bus in order to communicate with the OCC. + +For the POWER9 application, the BMC can communicate with the P9 over FSI bus +and SBE engine. Once again, SCOM operations are required. This driver will +implement SCOM ops over FSI/SBE. This will require the FSI driver.
drivers/hwmon/Kconfig +2 -0
@@ -1229,6 +1229,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
drivers/hwmon/Makefile +1 -0
@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o +obj-$(CONFIG_SENSORS_PPC_OCC) += occ/ obj-$(CONFIG_PMBUS) += pmbus/ ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
drivers/hwmon/occ/Kconfig new file +15 -0
@@ -0,0 +1,15 @@ +# +# On Chip Controller configuration +# + +menuconfig SENSORS_PPC_OCC + bool "PPC On-Chip Controller" + help + If you say yes here you get support to monitor Power CPU + sensors via the On-Chip Controller (OCC). + + Generally this is used by management controllers such as a BMC + on an OpenPower system. + + This driver can also be built as a module. If so, the module + will be called occ.
drivers/hwmon/occ/Makefile new file +1 -0
@@ -0,0 +1 @@ +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
drivers/hwmon/occ/occ.c new file +519 -0
519 lines not on this page · raw message
drivers/hwmon/occ/occ.h new file +86 -0
86 lines not on this page · raw message
drivers/hwmon/occ/scom.h new file +48 -0
48 lines not on this page · raw message
Signature · 1 line
1.9.1
Hi Eddie,
I think this is looking good. A few comments below.
Quoted 83 lines · On Fri, 2016-12-16 at 14:34 -0600, [email protected] wrote:
Quoted 1 line
From: "Edward A. James" <[email protected]>
Add core support for polling the OCC for it's sensor data and parsing that
data into sensor-specific information.Quoted 2 lines
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>---
Documentation/hwmon/occ | 40 ++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 15 ++
drivers/hwmon/occ/Makefile | 1 +
drivers/hwmon/occ/occ.c | 519
+++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ.h | 86 ++++++++
drivers/hwmon/occ/scom.h | 48 +++++8 files changed, 712 insertions(+)create mode 100644
Documentation/hwmon/occ
create mode 100644drivers/hwmon/occ/Kconfig
create mode 100644drivers/hwmon/occ/Makefile
create mode 100644drivers/hwmon/occ/occ.c
create mode 100644drivers/hwmon/occ/occ.h
create mode 100644drivers/hwmon/occ/scom.hDocumentation/hwmon/occ new file +8
-0@@ -0,0 +1,40 @@ +Kernel driver occ +================= + +Supported chips: + * ASPEED AST2400 + * ASPEED AST2500 + +Please note that the chip must be connected to a POWER8 or POWER9processor
Documentation/hwmon/occ new file +2
-0+(see the BMC - Host Communications section). +Quoted 1 line
+Author: Eddie James <[email protected]>
Documentation/hwmon/occ new file +5
-0+ +Description +----------- + +This driver implements support for the OCC (On-Chip Controller) onthe IBM
+POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The
+OCC is an embedded processor that provides real time power and thermalDocumentation/hwmon/occ new file +3
-0+monitoring. + +This driver provides an interface on a BMC to poll OCC sensor data,set user
Documentation/hwmon/occ new file +3
-0+power caps, and perform some basic OCC error handling. + +Currently, all versions of the OCC support four types of sensordata: power,
+temperature, frequency, and "caps," which indicate limits and thresholds usedDocumentation/hwmon/occ new file +6
-0+internally on the OCC. + +BMC - Host Communications +------------------------- + +For the POWER8 application, the BMC can communicate with the P8 overI2C bus.
+However, to access the OCC register space, any data transfer must use a SCOM
+operation. SCOM is a procedure to initiate a data transfer, typically of 8Documentation/hwmon/occ new file +2
-0+bytes. SCOMs consist of writing a 32-bit command register and then +reading/writing two 32-bit data registers. This driver implementsthese
Documentation/hwmon/occ new file +3
-0+SCOM operations over I2C bus in order to communicate with the OCC. + +For the POWER9 application, the BMC can communicate with the P9 over
Quoted 4 lines
FSI bus
+and SBE engine. Once again, SCOM operations are required. This driver will
+implement SCOM ops over FSI/SBE. This will require the FSI driver.
Great!
Quoted 136 lines
drivers/hwmon/Kconfig +0
-0@@ -1229,6 +1229,8 @@ config SENSORS_NSA320Quoted 3 lines
This driver can also be built as a module. If so, the module
will be called nsa320-hwmon.drivers/hwmon/Kconfig +2
-0+source drivers/hwmon/occ/Kconfig +config
SENSORS_PCF8591Quoted 2 lines
tristate "Philips PCF8591 ADC/DAC"
depends on I2Cdrivers/hwmon/Makefile +0
-0Quoted 4 lines
(quoted diff) +0
-0@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.oQuoted 2 lines
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ/
obj-$(CONFIG_PMBUS) += pmbus/
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUGdrivers/hwmon/occ/Kconfig new file +5
-0@@ -0,0 +1,15 @@ +# +# On Chip Controller configuration +# + +menuconfig SENSORS_PPC_OCCdrivers/hwmon/occ/Kconfig line 9
Quoted 4 lines
drivers/hwmon/occ/Kconfig new file +4
-0+ bool "PPC On-Chip Controller" + help + If you say yes here you get support to monitor Power CPU + sensors via the On-Chip Controller (OCC).+
drivers/hwmon/occ/Kconfig line 12
Quoted 3 lines
(quoted diff) +1
-0+ Generally this is used by management controllers such asa BMC
drivers/hwmon/occ/Kconfig new file +1
-0+ on an OpenPower system.+
Quoted 3 lines
(quoted diff) +1
-0+ This driver can also be built as a module. If so, themodule
drivers/hwmon/occ/Kconfig new file +1
-0+ will be called occ.drivers/hwmon/occ/Makefile new file +1
-0@@ -0,0 +1 @@ +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.odrivers/hwmon/occ/occ.c new file +4
-0@@ -0,0 +1,519 @@ +/* +* occ.c - OCC hwmon driver +* +* This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */drivers/hwmon/occ/occ.c new file +16
-0+ +#include <linux/module.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/jiffies.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/err.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/kernel.h> +#include <linux/device.h> +#include <asm/unaligned.h> + +#include "occ.h" +Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define OCC_DATA_MAX 4096 +#define OCC_BMC_TIMEOUT_S 20drivers/hwmon/occ/occ.c new file +2
-0+ +/* To generate attn to OCC */ Quoted 1 line
+#define
ATTN_DATA0x0006B035drivers/hwmon/occ/occ.c new file +2
-0+ +/* For BMC to read/write SRAM */ Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+#define OCB_ADDRESS 0x0006B070 +#define OCB_DATA 0x0006B075 +#define OCB_STATUS_CONTROL_AND 0x0006B072 +#define OCB_STATUS_CONTROL_OR 0x0006B073drivers/hwmon/occ/occ.c new file +2
-0+ +/* To init OCB */ Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+#define OCB_AND_INIT0 0xFBFFFFFF +#define OCB_AND_INIT1 0xFFFFFFFF +#define OCB_OR_INIT0 0x08000000 +#define OCB_OR_INIT1 0x00000000drivers/hwmon/occ/occ.c new file +2
-0+ +/* To generate attention on OCC */ Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define ATTN0 0x01010000 +#define ATTN1 0x00000000drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC return status for "command in progress" */ Quoted 1 line
+#define
RESP_RETURN_CMD_IN_PRG0xFF
drivers/hwmon/occ/occ.c line 59
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+/* time interval to retry on "command in progress" return status */ +#define CMD_IN_PRG_INT_MS 100
Out of curiosity, why are we defining one period in MS and another in S (OCC_BMC_TIMEOUT_S)? It feels like it would make sense to use consistent units.
Regardless, thanks for defining the macros above rather than using the magic numbers.
Quoted 155 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC command definitions */ Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define OCC_POLL 0 +#define OCC_SET_USER_POWR_CAP 0x22drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC poll command data */ Quoted 1 line
+#define
OCC_POLL_STAT_SENSOR0x10drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC response data offsets */ Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+#define RESP_RETURN_STATUS 2 +#define RESP_DATA_LENGTH 3 +#define RESP_HEADER_OFFSET 5 +#define SENSOR_STR_OFFSET 37 +#define SENSOR_BLOCK_NUM_OFFSET 43 +#define SENSOR_BLOCK_OFFSET 45drivers/hwmon/occ/occ.c new file +5
-0+ +/* occ_poll_header +* structure to match the raw occ poll response data +*/ +struct occ_poll_header {Quoted 16 lines
drivers/hwmon/occ/occ.c new file +16
-0+u8 status; +u8 ext_status; +u8 occs_present; +u8 config; +u8 occ_state; +u8 mode; +u8 ips_status; +u8 error_log_id; +u32 error_log_addr_start; +u16 error_log_length; +u8 reserved2; +u8 reserved3; +u8 occ_code_level[16 ]; +u8 sensor_eye_catcher[6 ]; +u8 sensor_block_num; +u8 sensor_data_version;drivers/hwmon/occ/occ.c new file +3
-0+} __attribute__((packed, aligned(4))); + +struct occ_response {Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+struct occ_poll_header header; +struct occ_blocks data;drivers/hwmon/occ/occ.c new file +3
-0+}; + +struct occ {Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+struct device *dev; +void *bus; +struct occ_bus_ops bus_ops; +struct occ_ops ops; +struct occ_config config; +unsigned long update_interval; +unsigned long last_updated; +struct mutex update_lock; +struct occ_response response; +bool valid;drivers/hwmon/occ/occ.c new file +4
-0+}; + +static void deinit_occ_resp_buf (struct occ_response *resp) +{drivers/hwmon/occ/occ.c line 118
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp) + return;+
drivers/hwmon/occ/occ.c line 124
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!resp->data.blocks) +return ;+
drivers/hwmon/occ/occ.c line 127
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+for (i =0 ; i < resp->header.sensor_block_num; ++i) +kfree (resp->data.blocks[i].sensors);+
drivers/hwmon/occ/occ.c line 129
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+kfree (resp->data.blocks);+
drivers/hwmon/occ/occ.c line 131
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+memset (resp,0 ,sizeof (struct occ_response ));+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+for (i =0 ; i <MAX_OCC_SENSOR_TYPE ; ++i) + resp->data.sensor_block_id[i] =-1 ;drivers/hwmon/occ/occ.c new file +3
-0+} + +static void *occ_get_sensor_by_type(struct occ_response *resp,drivers/hwmon/occ/occ.c line 138
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ enum sensor_type t)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp->data.blocks) + return NULL;+
drivers/hwmon/occ/occ.c line 144
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (resp->data.sensor_block_id[t] ==-1 ) +return NULL ;+
Quoted 2 lines
(quoted diff) +1
-0+ return resp->data.blocks[resp-Quoted 1 line
data.sensor_block_id[t]].sensors;
drivers/hwmon/occ/occ.c new file +3
-0+} + +static int occ_check_sensor(struct occ *driver, u8 sensor_length,Quoted 2 lines
(quoted diff) +1
-0+ u8 sensor_num, enum sensor_type t, intblock)
+{
drivers/hwmon/occ/occ.c line 154
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+void *sensor; +int type_block_id; +struct occ_response *resp = &driver->response;+
drivers/hwmon/occ/occ.c line 156
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ sensor =occ_get_sensor_by_type (resp, t);+
Review
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +4
-0+/* empty sensor block, release older sensor data */ +if (sensor_num ==0 || sensor_length ==0 ) { +kfree (sensor); + dev_err(driver->dev, "no sensor blocksavailable\n");
(quoted diff) +2
-0+ return -ENODATA; + }+
drivers/hwmon/occ/occ.c line 173
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +9
-0+ type_block_id = resp->data.sensor_block_id[t]; +if (!sensor || sensor_num != + resp->data.blocks[type_block_id].header.sensor_num) { +kfree (sensor); + resp->data.blocks[block].sensors = + driver->ops.alloc_sensor (t, sensor_num); +if (!resp->data.blocks[block].sensors) +return -ENOMEM ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/hwmon/occ/occ.c new file +3
-0+} + +static int parse_occ_response(struct occ *driver, u8 *data,drivers/hwmon/occ/occ.c line 179
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ struct occ_response *resp)+{
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+int b; +int s; +int rc; +int offset =SENSOR_BLOCK_OFFSET ; +int sensor_type; +u8 sensor_block_num;
drivers/hwmon/occ/occ.c line 187
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+char sensor_type_string[5 ] = {0 };
Given how you rearranged things below, do you think we can get away with removing sensor_type_string? Also, struct sensor_data_block_type defines sensor_type as a char * that can't be null-terminated as its size is 4, which still makes me feel uncomfortable. What are your thoughts here?
Quoted 45 lines
drivers/hwmon/occ/occ.c new file +1
-0+struct sensor_data_block_header *block;Review
Quoted 1 line
(quoted diff) +1
-0+ struct device *dev = driver->dev;+
drivers/hwmon/occ/occ.c line 196
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+/* check if the data is valid */ +if (strncmp (&data[SENSOR_STR_OFFSET ],"SENSOR" ,6 ) !=0 ) { +dev_err (dev,"no SENSOR string in response\n" ); + rc = -ENODATA ; + goto err; + }+
drivers/hwmon/occ/occ.c line 203
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET ]; +if (sensor_block_num ==0 ) { +dev_err (dev,"no sensor blocks available\n" ); + rc = -ENODATA ; + goto err; + }+
drivers/hwmon/occ/occ.c line 213
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +5
-0+/* if number of sensor block has changed, re-malloc */ +if (sensor_block_num != resp->header.sensor_block_num) { +deinit_occ_resp_buf (resp); + resp->data.blocks = kcalloc(sensor_block_num, + sizeof(struct
sensor_data_block),drivers/hwmon/occ/occ.c new file +4
-0+ GFP_KERNEL); +if (!resp->data.blocks) +return -ENOMEM ; + }+
drivers/hwmon/occ/occ.c line 220
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+memcpy (&resp->header, &data[RESP_HEADER_OFFSET ], + sizeof(struct occ_poll_header)); + resp->header.error_log_addr_start = +be32_to_cpu (resp->header.error_log_addr_start); + resp->header.error_log_length = +be16_to_cpu (resp->header.error_log_length);+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+dev_dbg (dev,"Reading %d sensor blocks\n" , + resp->header.sensor_block_num); +for (b =0 ; b < sensor_block_num; b++) {drivers/hwmon/occ/occ.c new file +1
-0+ block = (struct sensor_data_block_header
Quoted 1 line
*)&data[offset];
+ /* copy to a null terminated string */
Quoted 74 lines
drivers/hwmon/occ/occ.c line 228
Quoted 3 lines
(quoted diff) +1
-0+ strncpy(sensor_type_string, block->sensor_type,4);
drivers/hwmon/occ/occ.c new file +1
-0+ offset +=8 ;+
drivers/hwmon/occ/occ.c line 231
Quoted 3 lines
(quoted diff) +1
-0+ dev_dbg(dev, "sensor block[%d]: type: %s,
sensor_num: %d\n", b,drivers/hwmon/occ/occ.c new file +1
-0+ sensor_type_string, block->sensor_num);+
Quoted 14 lines
drivers/hwmon/occ/occ.c new file +3
-0+if (strncmp (block->sensor_type,"FREQ" ,4 ) ==0 ) + sensor_type =FREQ ; + else if (strncmp(block->sensor_type, "TEMP", 4) ==0)
(quoted diff) +2
-0+ sensor_type = TEMP; + else if (strncmp(block->sensor_type, "POWR", 4) ==0)
(quoted diff) +2
-0+ sensor_type = POWER; + else if (strncmp(block->sensor_type, "CAPS", 4) ==0)
drivers/hwmon/occ/occ.c new file +3
-0+ sensor_type =CAPS ; + else { + dev_err(dev, "sensor type not supported%s\n",
drivers/hwmon/occ/occ.c line 245
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ sensor_type_string); +continue ; + }+
drivers/hwmon/occ/occ.c line 252
Quoted 8 lines
(quoted diff) +1
-0+ rc = occ_check_sensor(driver, block-Quoted 1 line
sensor_length,(quoted diff) +1
-0+ block->sensor_num,
sensor_type, b);drivers/hwmon/occ/occ.c new file +4
-0+if (rc == -ENOMEM ) + goto err; +else if (rc) +continue ;+
Review
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +4
-0+ resp->data.sensor_block_id[sensor_type] = b; +for (s =0 ; s < block->sensor_num; s++) { + driver->ops.parse_sensor(data, + resp-Quoted 1 line
data.blocks[b].sensors,
(quoted diff) +1
-0+ sensor_type,offset, s);
(quoted diff) +2
-0+ offset += block->sensor_length; + }+
drivers/hwmon/occ/occ.c line 264
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* copy block data over to response pointer */ + resp->data.blocks[b].header = *block; + }+
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;+err:
Quoted 2 lines
(quoted diff) +2
-0+ deinit_occ_resp_buf(resp); + return rc;drivers/hwmon/occ/occ.c new file +3
-0+} + +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16length,
drivers/hwmon/occ/occ.c line 273
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ u8 *data, u8 *resp)+{
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+u32 cmd1, cmd2; +u16 checksum =0 ; +u16 length_be =cpu_to_be16 (length); +int i, rc, tries =0 ;drivers/hwmon/occ/occ.c new file +1
-0+ const int max_tries = (OCC_BMC_TIMEOUT_S * 1000) /
Quoted 1 line
CMD_IN_PRG_INT_MS;
I expect we should just #define the expression?
drivers/hwmon/occ/occ.c line 288
Quoted 11 lines
+
drivers/hwmon/occ/occ.c line 283
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ cmd1 = (seq <<24 ) | (type <<16 ) | length_be; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 );+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* checksum: sum of every bytes of cmd1, cmd2 */ +for (i =0 ; i <4 ; i++) { + checksum += (cmd1 >> (i *8 )) &0xFF ;drivers/hwmon/occ/occ.c new file +1
-0+ checksum += (cmd2 >> (i *8 )) &0xFF ;
I have to apologise here, as my review was wrong: Addition is commutative, but not when we're masking the result! So the original approach is probably more correct, and it only occurred to me when reading over this patch. Sorry! Did you get checksum errors as a result?
Quoted 35 lines
drivers/hwmon/occ/occ.c new file +2
-0+ } +drivers/hwmon/occ/occ.c line 291
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ cmd2 |= checksum << ((2 - length) *8 );+
drivers/hwmon/occ/occ.c line 297
Quoted 6 lines
(quoted diff) +2
-0+ /* Init OCB */ + rc = driver->bus_ops.putscom(driver->bus,
OCB_STATUS_CONTROL_OR,drivers/hwmon/occ/occ.c new file +3
-0+ OCB_OR_INIT0, OCB_OR_INIT1); +if (rc) + goto err;+
Review
Quoted 6 lines
(quoted diff) +1
-0+ rc = driver->bus_ops.putscom(driver->bus,
OCB_STATUS_CONTROL_AND,(quoted diff) +1
-0+ OCB_AND_INIT0,
OCB_AND_INIT1);(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 7 lines
(quoted diff) +1
-0+ /* Send command, 2nd half of the 64-bit addr is unused(write 0) */
(quoted diff) +2
-0+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS, + driver->config.command_addr,0);
(quoted diff) +2
-0+ if (rc) + goto err;+
Quoted 4 lines
(quoted diff) +2
-0+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS, + driver->config.command_addr,0);
(quoted diff) +1
-0+ if (rc)
drivers/hwmon/occ/occ.c
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ goto err;
This change highlighted to me that we're sending command_addr twice - is this necessary?
Quoted 40 lines
+
Review
Quoted 4 lines
(quoted diff) +1
-0+ rc = driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1,cmd2);
(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 5 lines
(quoted diff) +2
-0+ /* Trigger attention */ + rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA,ATTN0, ATTN1);
(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* Get response data */ + rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS, + driver->config.response_addr,0);
(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 5 lines
(quoted diff) +2
-0+ do { + rc = driver->bus_ops.getscom(driver->bus,
OCB_DATA, resp);(quoted diff) +2
-0+ if (rc) + goto err;+
Quoted 10 lines
(quoted diff) +1
-0+ /* retry if we get "command in progress" returnstatus */
(quoted diff) +1
-0+ if (resp[RESP_RETURN_STATUS] ==
RESP_RETURN_CMD_IN_PRG) {(quoted diff) +2
-0+ set_current_state(TASK_INTERRUPTIBLE); + schedule_timeout(msecs_to_jiffies(CMD_IN_P
RG_INT_MS));drivers/hwmon/occ/occ.c new file +3
-0+ tries++; + }else +break ;
drivers/hwmon/occ/occ.c line 342
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ }while (tries < max_tries);
I'll bikeshed this to try save the break and a timeout, which is really picking at the nits but reduces cyclomatic complexity and saves some time. What do you think of:
bool retry = false;
int tries = 0;
...
do {
if (retry) { set_current_state(TASK_INTERRUPTIBLE); shedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS)); }
rc = driver->bus_ops.getscom(driver->bus, OCB_DATA, resp); if (rc)
goto err;
retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) && tries++ < max_tries); } while (retry);
Are we okay with returning RESP_RETURN_CMD_IN_PRG to the caller? Because that's what happens in either case if we exceed max_tries.
Quoted 72 lines
+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return resp[RESP_RETURN_STATUS ];drivers/hwmon/occ/occ.c new file +2
-0+ +err:Quoted 2 lines
(quoted diff) +2
-0+ dev_err(driver->dev, "scom op failed rc:%d\n", rc); + return rc;drivers/hwmon/occ/occ.c new file +4
-0+} + +static int occ_get_all (struct occ *driver) +{drivers/hwmon/occ/occ.c line 358
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+int i =0 , rc; +u8 *occ_data; +u16 num_bytes; +const u8 poll_cmd_data =OCC_POLL_STAT_SENSOR ; +struct device *dev = driver->dev; +struct occ_response *resp = &driver->response;+
drivers/hwmon/occ/occ.c line 362
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ occ_data =devm_kzalloc (dev,OCC_DATA_MAX ,GFP_KERNEL ); +if (!occ_data) +return -ENOMEM ;+
drivers/hwmon/occ/occ.c line 369
Quoted 7 lines
(quoted diff) +1
-0+ rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data,
occ_data);drivers/hwmon/occ/occ.c new file +5
-0+if (rc) { +dev_err (dev,"OCC poll failed: 0x%x\n" , rc); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 373
Quoted 4 lines
(quoted diff) +1
-0+ num_bytes = get_unaligned((u16*)&occ_data[RESP_DATA_LENGTH]);
drivers/hwmon/occ/occ.c new file +2
-0+ num_bytes =be16_to_cpu (num_bytes); +dev_dbg (dev,"OCC data length: %d\n" , num_bytes);+
drivers/hwmon/occ/occ.c line 379
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes >OCC_DATA_MAX ) { +dev_err (dev,"OCC data length must be < 4KB\n" ); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 385
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes <=0 ) { +dev_err (dev,"OCC data length is zero\n" ); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 379
Quoted 11 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* read remaining data */ +for (i =8 ; i < num_bytes +8 ; i +=8 ) { + rc = driver->bus_ops.getscom(driver->bus,
OCB_DATA,drivers/hwmon/occ/occ.c new file +3
-0+ &occ_data[i]); +if (rc) { + dev_err(dev, "scom op failed rc:%d\n",rc);
drivers/hwmon/occ/occ.c new file +3
-0+ goto out; + } + }+
Quoted 4 lines
(quoted diff) +1
-0+ /* don't need more sanity checks; buffer is alloc'd formax response
(quoted diff) +1
-0+ * size so we just check for valid data in
parse_occ_response
drivers/hwmon/occ/occ.c
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+*/
Thanks for the comment.
Quoted 304 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =parse_occ_response (driver, occ_data, resp); + +out:Quoted 2 lines
(quoted diff) +2
-0+ devm_kfree(dev, occ_data); + return rc;drivers/hwmon/occ/occ.c new file +4
-0+} + +int occ_update_device (struct occ *driver) +{drivers/hwmon/occ/occ.c line 409
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+int rc =0 ;+
drivers/hwmon/occ/occ.c line 411
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+mutex_lock (&driver->update_lock);+
drivers/hwmon/occ/occ.c line 415
Quoted 4 lines
(quoted diff) +1
-0+ if (time_after(jiffies, driver->last_updated + driver-Quoted 1 line
update_interval)drivers/hwmon/occ/occ.c new file +2
-0+ || !driver->valid) { + driver->valid =1 ;+
drivers/hwmon/occ/occ.c line 419
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =occ_get_all (driver); +if (rc) + driver->valid =0 ;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ driver->last_updated = jiffies; + }+
drivers/hwmon/occ/occ.c line 424
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+mutex_unlock (&driver->update_lock);+
Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_update_device); + +void *occ_get_sensor (struct occ *driver,int sensor_type) +{Review
Quoted 1 line
(quoted diff) +1
-0+ int rc;+
drivers/hwmon/occ/occ.c line 440
Quoted 8 lines
drivers/hwmon/occ/occ.c new file +4
-0+/* occ_update_device locks the update lock */ + rc =occ_update_device (driver); +if (rc !=0 ) { + dev_err(driver->dev, "cannot get occ sensor data:%d\n",
drivers/hwmon/occ/occ.c new file +3
-0+ rc); +return NULL ; + }+
Quoted 2 lines
(quoted diff) +1
-0+ return occ_get_sensor_by_type(&driver->response,
sensor_type);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor); + +int occ_get_sensor_value (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_sensor_value (occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor_value); + +int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_sensor_id (occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_get_sensor_id); + +int occ_get_caps_value(struct occ *occ, void *sensor, int snum, int
caps_field)
+{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_caps_value (sensor, snum, caps_field);drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_get_caps_value); + +void occ_get_response_blocks(struct occ *occ, struct occ_blocks**blocks)
+{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ *blocks = &occ->response.data;drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_get_response_blocks); + +void occ_set_update_interval(struct occ *occ, unsigned longinterval)
+{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ occ->update_interval =msecs_to_jiffies (interval);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_set_update_interval); + +int occ_set_user_powercap (struct occ *occ,u16 cap) +{drivers/hwmon/occ/occ.c line 478
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+u8 resp[8 ];+
drivers/hwmon/occ/occ.c line 480
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ cap =cpu_to_be16 (cap);+
Quoted 3 lines
(quoted diff) +1
-0+ return occ_send_cmd(occ, 0, OCC_SET_USER_POWR_CAP, 2, (u8*)&cap,
drivers/hwmon/occ/occ.c new file +1
-0+ resp);drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_set_user_powercap); + +struct occ *occ_start(struct device *dev, void *bus,drivers/hwmon/occ/occ.c line 489
Quoted 3 lines
(quoted diff) +1
-0+ struct occ_bus_ops *bus_ops, const struct
occ_ops*ops,drivers/hwmon/occ/occ.c new file +1
-0+ const struct occ_config *config)+{
Quoted 2 lines
(quoted diff) +1
-0+ struct occ *driver = devm_kzalloc(dev, sizeof(struct occ),
GFP_KERNEL);+
drivers/hwmon/occ/occ.c line 494
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!driver) +return ERR_PTR (-ENOMEM );+
drivers/hwmon/occ/occ.c line 500
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+ driver->dev = dev; + driver->bus = bus; + driver->bus_ops = *bus_ops; + driver->ops = *ops; + driver->config = *config;+
drivers/hwmon/occ/occ.c line 503
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ driver->update_interval =HZ ; +mutex_init (&driver->update_lock);+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return driver;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_start); + +int occ_stop (struct occ *occ) +{drivers/hwmon/occ/occ.c line 511
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+devm_kfree (occ->dev, occ);+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/hwmon/occ/occ.c new file +3
-0+} +EXPORT_SYMBOL (occ_stop); +Quoted 1 line
+MODULE_AUTHOR("Eddie James <[email protected]>");
drivers/hwmon/occ/occ.c new file +2
-0+MODULE_DESCRIPTION ("OCC hwmon core driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/occ.h new file +4
-0@@ -0,0 +1,86 @@ +/* +* occ.h - hwmon OCC driver +* +* This file contains data structures and function prototypes for common access
+ * between different bus protocols and host systems.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */drivers/hwmon/occ/occ.h new file +13
-0+ +#ifndef __OCC_H__ +#define __OCC_H__ + +#include "scom.h" + +struct device ; +struct occ ; + +/* sensor_data_block_header +* structure to match the raw occ sensor block header +*/ +struct sensor_data_block_header {Quoted 5 lines
drivers/hwmon/occ/occ.h new file +5
-0+u8 sensor_type[4 ]; +u8 reserved0; +u8 sensor_format; +u8 sensor_length; +u8 sensor_num;drivers/hwmon/occ/occ.h new file +3
-0+} __attribute__((packed, aligned(4))); + +struct sensor_data_block {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+struct sensor_data_block_header header; +void *sensors;drivers/hwmon/occ/occ.h new file +3
-0+}; + +enum sensor_type {Quoted 5 lines
drivers/hwmon/occ/occ.h new file +5
-0+FREQ =0 , +TEMP , +POWER , +CAPS , +MAX_OCC_SENSOR_TYPE drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_ops {Quoted 10 lines
(quoted diff) +1
-0+ void (*parse_sensor)(u8 *data, void *sensor, int
sensor_type, int off,drivers/hwmon/occ/occ.h new file +3
-0+ int snum); +void *(*alloc_sensor)(int sensor_type,int num_sensors); + int (*get_sensor_value)(struct occ *driver, int
sensor_type, int snum);(quoted diff) +1
-0+ int (*get_sensor_id)(struct occ *driver, int sensor_type,int snum);
(quoted diff) +1
-0+ int (*get_caps_value)(void *sensor, int snum, int
caps_field);drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_config {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+u32 command_addr; +u32 response_addr;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_blocks {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+int sensor_block_id[MAX_OCC_SENSOR_TYPE ]; +struct sensor_data_block *blocks;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ *occ_start(struct device *dev, void *bus,Quoted 3 lines
(quoted diff) +1
-0+ struct occ_bus_ops *bus_ops, const struct
occ_ops*ops,drivers/hwmon/occ/occ.h new file +1
-0+ const struct occ_config *config);drivers/hwmon/occ/occ.h new file +4
-0+int occ_stop (struct occ *occ); + +void *occ_get_sensor (struct occ *occ,int sensor_type); +int occ_get_sensor_value(struct occ *occ, int sensor_type, intsnum);
drivers/hwmon/occ/occ.h new file +2
-0+int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum); +int occ_get_caps_value(struct occ *occ, void *sensor, int snum,drivers/hwmon/occ/occ.h line 80
Quoted 1 line
drivers/hwmon/occ/occ.h new file +1
-0+ int caps_field);+void
occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks);drivers/hwmon/occ/occ.h new file +2
-0+int occ_update_device (struct occ *driver); +void occ_set_update_interval(struct occ *occ, unsigned longinterval);
drivers/hwmon/occ/occ.h new file +3
-0+int occ_set_user_powercap (struct occ *occ,u16 cap); + +#endif /* __OCC_H__ */ drivers/hwmon/occ/scom.h new file +8
-0@@ -0,0 +1,48 @@ +/* +* scom.h - hwmon OCC driver +* +* This file contains data structures for scom operations to the OCC +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */drivers/hwmon/occ/scom.h new file +6
-0+ +#ifndef __SCOM_H__ +#define __SCOM_H__ + +/* +* occ_bus_ops - represent the low-level transfer methods to communicate with
+ * the OCC.
+ *
+ * getscom - OCC scom read
+ * @bus: handle to slave device
Quoted 8 lines
+ * @address: address
+ * @data: where to store data read from slave; buffer size must be greater than
+ * or equal to offset + 8 bytes.
+ * @offset: offset into data pointer
+ *
+ * Returns 0 on success or one of -SCOM_READ_ERROR, -SCOM_WRITE_ERROR.
We deleted these macros. What's the plan?
Quoted 8 lines
+ *
+ * putscom - OCC scom write
+ * @bus: handle to slave device
+ * @address: address
+ * @data0: first data byte to write
+ * @data1: second data byte to write
+ *
+ * Returns 0 on success or -SCOM_WRITE_ERROR on error
Again, this macro was deleted.
drivers/hwmon/occ/scom.h line 48
Quoted 9 lines
+ */
+structocc_bus_ops{Quoted 3 lines
(quoted diff) +2
-0+ int (*getscom)(void *bus, u32 address, u8 *data); + int (*putscom)(void *bus, u32 address, u32 data0, u32data1);
drivers/hwmon/occ/scom.h new file +3
-0+}; + +#endif /* __SCOM_H__ */
Cheers,
Andrew
Hi Andrew,
Thanks for the comments, I will address most of these and push up a new set.
Quoted 396 lines · Andrew Jeffery
Hi Eddie,
I think this is looking good. A few comments below.
Quoted 83 lines · On Fri, 2016-12-16 at 14:34 -0600, [email protected] wrote:
Quoted 1 line
From: "Edward A. James" <[email protected]>
Add core support for polling the OCC for it's sensor data and parsing that
data into sensor-specific information.Quoted 2 lines
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>--- Documentation/hwmon/occ | 40 ++++ drivers/hwmon/Kconfig | 2 + drivers/hwmon/Makefile | 1 + drivers/hwmon/occ/Kconfig | 15 ++ drivers/hwmon/occ/Makefile | 1 + drivers/hwmon/occ/occ.c | 519 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ.h | 86 ++++++++ drivers/hwmon/occ/scom.h | 48 +++++ 8 files changed, 712 insertions(+) create mode 100644 Documentation/hwmon/occ create mode 100644 drivers/hwmon/occ/Kconfig create mode 100644 drivers/hwmon/occ/Makefile create mode 100644 drivers/hwmon/occ/occ.c create mode 100644 drivers/hwmon/occ/occ.h create mode 100644 drivers/hwmon/occ/scom.hDocumentation/hwmon/occ new file +8
-0@@ -0,0 +1,40 @@ +Kernel driver occ +================= + +Supported chips: + * ASPEED AST2400 + * ASPEED AST2500 + +Please note that the chip must be connected to a POWER8 or POWER9processor
Documentation/hwmon/occ new file +2
-0+(see the BMC - Host Communications section). +Quoted 1 line
+Author: Eddie James <[email protected]>
Documentation/hwmon/occ new file +5
-0+ +Description +----------- + +This driver implements support for the OCC (On-Chip Controller) onthe IBM
+POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The
+OCC is an embedded processor that provides real time power and thermalDocumentation/hwmon/occ new file +3
-0+monitoring. + +This driver provides an interface on a BMC to poll OCC sensor data,set user
Documentation/hwmon/occ new file +3
-0+power caps, and perform some basic OCC error handling. + +Currently, all versions of the OCC support four types of sensordata: power,
+temperature, frequency, and "caps," which indicate limits and thresholds usedDocumentation/hwmon/occ new file +6
-0+internally on the OCC. + +BMC - Host Communications +------------------------- + +For the POWER8 application, the BMC can communicate with the P8 overI2C bus.
+However, to access the OCC register space, any data transfer must use a SCOM
+operation. SCOM is a procedure to initiate a data transfer, typically of 8Documentation/hwmon/occ new file +2
-0+bytes. SCOMs consist of writing a 32-bit command register and then +reading/writing two 32-bit data registers. This driver implementsthese
Documentation/hwmon/occ new file +3
-0+SCOM operations over I2C bus in order to communicate with the OCC. + +For the POWER9 application, the BMC can communicate with the P9 overQuoted 4 lines
FSI bus
+and SBE engine. Once again, SCOM operations are required. This driver will
+implement SCOM ops over FSI/SBE. This will require the FSI driver.Great!
Quoted 136 lines
drivers/hwmon/Kconfig +0
-0@@ -1229,6 +1229,8 @@ config SENSORS_NSA320Quoted 3 lines
This driver can also be built as a module. If so, the
module
will be called nsa320-hwmon.drivers/hwmon/Kconfig +2
-0+source drivers/hwmon/occ/Kconfig + config SENSORS_PCF8591Quoted 2 lines
tristate "Philips PCF8591 ADC/DAC"
depends on I2Cdrivers/hwmon/Makefile +0
-0Quoted 8 lines
(quoted diff) +0
-0@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o+obj-$(CONFIG_SENSORS_PPC_OCC) += occ/
obj-$(CONFIG_PMBUS) += pmbus/ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
drivers/hwmon/occ/Kconfig new file +5
-0@@ -0,0 +1,15 @@ +# +# On Chip Controller configuration +# + +menuconfig SENSORS_PPC_OCCdrivers/hwmon/occ/Kconfig line 9
Quoted 4 lines
drivers/hwmon/occ/Kconfig new file +4
-0+ bool "PPC On-Chip Controller" + help + If you say yes here you get support to monitor Power CPU + sensors via the On-Chip Controller (OCC).+
Quoted 3 lines
+ Generally this is used by management controllers such as
a BMC
+ on an OpenPower system.+
Quoted 3 lines
+ This driver can also be built as a module. If so, the
module
+ will be called occ.drivers/hwmon/occ/Makefile new file +1
-0@@ -0,0 +1 @@ +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.odrivers/hwmon/occ/occ.c new file +4
-0@@ -0,0 +1,519 @@ +/* +* occ.c - OCC hwmon driver +* +* This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */drivers/hwmon/occ/occ.c new file +16
-0+ +#include <linux/module.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/jiffies.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/err.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/kernel.h> +#include <linux/device.h> +#include <asm/unaligned.h> + +#include "occ.h" +Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define OCC_DATA_MAX 4096 +#define OCC_BMC_TIMEOUT_S 20drivers/hwmon/occ/occ.c new file +2
-0+ +/* To generate attn to OCC */ Quoted 1 line
+#define
ATTN_DATA0x0006B035drivers/hwmon/occ/occ.c new file +2
-0+ +/* For BMC to read/write SRAM */ Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+#define OCB_ADDRESS 0x0006B070 +#define OCB_DATA 0x0006B075 +#define OCB_STATUS_CONTROL_AND 0x0006B072 +#define OCB_STATUS_CONTROL_OR 0x0006B073drivers/hwmon/occ/occ.c new file +2
-0+ +/* To init OCB */ Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+#define OCB_AND_INIT0 0xFBFFFFFF +#define OCB_AND_INIT1 0xFFFFFFFF +#define OCB_OR_INIT0 0x08000000 +#define OCB_OR_INIT1 0x00000000drivers/hwmon/occ/occ.c new file +2
-0+ +/* To generate attention on OCC */ Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define ATTN0 0x01010000 +#define ATTN1 0x00000000drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC return status for "command in progress" */ Quoted 1 line
+#define
RESP_RETURN_CMD_IN_PRG0xFFdrivers/hwmon/occ/occ.c line 59
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+/* time interval to retry on "command in progress" return status */ +#define CMD_IN_PRG_INT_MS 100Out of curiosity, why are we defining one period in MS and another in S (
OCC_BMC_TIMEOUT_S)? It feels like it would make sense to use consistent units.Regardless, thanks for defining the macros above rather than using the magic numbers.
Quoted 155 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC command definitions */ Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define OCC_POLL 0 +#define OCC_SET_USER_POWR_CAP 0x22drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC poll command data */ Quoted 1 line
+#define
OCC_POLL_STAT_SENSOR0x10drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC response data offsets */ Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+#define RESP_RETURN_STATUS 2 +#define RESP_DATA_LENGTH 3 +#define RESP_HEADER_OFFSET 5 +#define SENSOR_STR_OFFSET 37 +#define SENSOR_BLOCK_NUM_OFFSET 43 +#define SENSOR_BLOCK_OFFSET 45drivers/hwmon/occ/occ.c new file +5
-0+ +/* occ_poll_header +* structure to match the raw occ poll response data +*/ +struct occ_poll_header {Quoted 16 lines
drivers/hwmon/occ/occ.c new file +16
-0+u8 status; +u8 ext_status; +u8 occs_present; +u8 config; +u8 occ_state; +u8 mode; +u8 ips_status; +u8 error_log_id; +u32 error_log_addr_start; +u16 error_log_length; +u8 reserved2; +u8 reserved3; +u8 occ_code_level[16 ]; +u8 sensor_eye_catcher[6 ]; +u8 sensor_block_num; +u8 sensor_data_version;drivers/hwmon/occ/occ.c new file +3
-0+} __attribute__((packed, aligned(4))); + +struct occ_response {Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+struct occ_poll_header header; +struct occ_blocks data;drivers/hwmon/occ/occ.c new file +3
-0+}; + +struct occ {Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+struct device *dev; +void *bus; +struct occ_bus_ops bus_ops; +struct occ_ops ops; +struct occ_config config; +unsigned long update_interval; +unsigned long last_updated; +struct mutex update_lock; +struct occ_response response; +bool valid;drivers/hwmon/occ/occ.c new file +4
-0+}; + +static void deinit_occ_resp_buf (struct occ_response *resp) +{Quoted 1 line
+ int i;
+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp) + return;+
drivers/hwmon/occ/occ.c line 124
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!resp->data.blocks) +return ;+
drivers/hwmon/occ/occ.c line 127
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+for (i =0 ; i < resp->header.sensor_block_num; ++i) +kfree (resp->data.blocks[i].sensors);+
Quoted 1 line
+
kfree(resp->data.blocks);+
Quoted 1 line
+
memset(resp, 0, sizeof(struct occ_response));+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+for (i =0 ; i <MAX_OCC_SENSOR_TYPE ; ++i) + resp->data.sensor_block_id[i] =-1 ;drivers/hwmon/occ/occ.c new file +3
-0+} + +static void *occ_get_sensor_by_type(struct occ_response *resp,Quoted 1 line
+ enum
sensor_typet)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp->data.blocks) + return NULL;+
drivers/hwmon/occ/occ.c line 144
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (resp->data.sensor_block_id[t] ==-1 ) +return NULL ;+
Quoted 2 lines
+ return resp->data.blocks[resp-
Quoted 1 line
data.sensor_block_id[t]].sensors;
drivers/hwmon/occ/occ.c new file +3
-0+} + +static int occ_check_sensor(struct occ *driver, u8 sensor_length,Quoted 2 lines
+ u8
sensor_num, enumsensor_typet, int
block)+{
drivers/hwmon/occ/occ.c line 154
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+void *sensor; +int type_block_id; +struct occ_response *resp = &driver->response;+
Quoted 1 line
+ sensor =
occ_get_sensor_by_type(resp, t);+
Review
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +4
-0+/* empty sensor block, release older sensor data */ +if (sensor_num ==0 || sensor_length ==0 ) { +kfree (sensor); + dev_err(driver->dev, "no sensor blocksavailable\n");
(quoted diff) +2
-0+ return -ENODATA; + }+
drivers/hwmon/occ/occ.c line 173
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +9
-0+ type_block_id = resp->data.sensor_block_id[t]; +if (!sensor || sensor_num != + resp->data.blocks[type_block_id].header.sensor_num) { +kfree (sensor); + resp->data.blocks[block].sensors = + driver->ops.alloc_sensor (t, sensor_num); +if (!resp->data.blocks[block].sensors) +return -ENOMEM ; + }+
Quoted 1 line
+ return 0;
drivers/hwmon/occ/occ.c new file +3
-0+} + +static int parse_occ_response(struct occ *driver, u8 *data,Quoted 1 line
+ struct
occ_response*resp)+{
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+int b; +int s; +int rc; +int offset =SENSOR_BLOCK_OFFSET ; +int sensor_type; +u8 sensor_block_num;
Quoted 7 lines
Quoted 1 line
+ char sensor_type_string[5] = { 0 };
Given how you rearranged things below, do you think we can get away with removing
sensor_type_string? Also, structsensor_data_block_typedefinessensor_typeas a char * that can't be null-terminated as its size is 4, which still makes me feel uncomfortable. What are your thoughts here?
The idea of sensor_type_string was to have a null-terminated string that I could use in the debug statements below. sensor_type is restricted to 4 chars because that's how its' formatted in the OCC data block. Anywhere I use sensor_type I use the length-limited string function, such as strncmp or the like, and restrict it to 4 chars. I feel this is an acceptable solution, but you're right, sensor_type_string isn't strictly necessary, just useful to be able to print that out.
Quoted 137 lines
Quoted 40 lines
+ struct
sensor_data_block_header*block;Quoted 1 line
+ struct device *dev =
driver->dev;+
drivers/hwmon/occ/occ.c line 196
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+/* check if the data is valid */ +if (strncmp (&data[SENSOR_STR_OFFSET ],"SENSOR" ,6 ) !=0 ) { +dev_err (dev,"no SENSOR string in response\n" ); + rc = -ENODATA ; + goto err; + }+
drivers/hwmon/occ/occ.c line 203
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET ]; +if (sensor_block_num ==0 ) { +dev_err (dev,"no sensor blocks available\n" ); + rc = -ENODATA ; + goto err; + }+
drivers/hwmon/occ/occ.c line 213
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +5
-0+/* if number of sensor block has changed, re-malloc */ +if (sensor_block_num != resp->header.sensor_block_num) { +deinit_occ_resp_buf (resp); + resp->data.blocks = kcalloc(sensor_block_num, + sizeof(struct
sensor_data_block),drivers/hwmon/occ/occ.c new file +4
-0+ GFP_KERNEL); +if (!resp->data.blocks) +return -ENOMEM ; + }+
drivers/hwmon/occ/occ.c line 220
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+memcpy (&resp->header, &data[RESP_HEADER_OFFSET ], +sizeof (struct occ_poll_header )); + resp->header.error_log_addr_start = +be32_to_cpu (resp->header.error_log_addr_start); + resp->header.error_log_length = +be16_to_cpu (resp->header.error_log_length);Quoted 7 lines
+
drivers/hwmon/occ/occ.c line 224
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+dev_dbg (dev,"Reading %d sensor blocks\n" , + resp->header.sensor_block_num); +for (b =0 ; b < sensor_block_num; b++) {+ block = (struct
sensor_data_block_header
*)&data[offset];+ /* copy to a null terminated string */
Quoted 67 lines
Quoted 3 lines
+ strncpy(sensor_type_string,
block->sensor_type,
4);
+ offset += 8;+
Quoted 3 lines
+ dev_dbg(dev, "sensor block[%d]: type: %s,
sensor_num: %d\n", b,
+sensor_type_string,block->sensor_num);+
Quoted 14 lines
drivers/hwmon/occ/occ.c new file +3
-0+if (strncmp (block->sensor_type,"FREQ" ,4 ) ==0 ) + sensor_type =FREQ ; + else if (strncmp(block->sensor_type, "TEMP", 4) ==0)
(quoted diff) +2
-0+ sensor_type = TEMP; + else if (strncmp(block->sensor_type, "POWR", 4) ==0)
(quoted diff) +2
-0+ sensor_type = POWER; + else if (strncmp(block->sensor_type, "CAPS", 4) ==0)
drivers/hwmon/occ/occ.c new file +3
-0+ sensor_type =CAPS ; + else { + dev_err(dev, "sensor type not supported%s\n",
drivers/hwmon/occ/occ.c line 245
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ sensor_type_string); +continue ; + }+
drivers/hwmon/occ/occ.c line 252
Quoted 8 lines
+ rc = occ_check_sensor(driver, block-
Quoted 1 line
sensor_length,+
block->sensor_num,sensor_type, b);drivers/hwmon/occ/occ.c new file +4
-0+if (rc == -ENOMEM ) + goto err; +else if (rc) +continue ;+
Review
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +4
-0+ resp->data.sensor_block_id[sensor_type] = b; +for (s =0 ; s < block->sensor_num; s++) { + driver->ops.parse_sensor(data, + resp-Quoted 1 line
data.blocks[b].sensors,
+
sensor_type,
offset, s);(quoted diff) +2
-0+ offset += block->sensor_length; + }+
drivers/hwmon/occ/occ.c line 264
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* copy block data over to response pointer */ + resp->data.blocks[b].header = *block; + }+
Quoted 1 line
+ return 0;
+err:
Quoted 2 lines
(quoted diff) +2
-0+ deinit_occ_resp_buf(resp); + return rc;drivers/hwmon/occ/occ.c new file +3
-0+} + +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16length,
Quoted 1 line
+ u8 *data, u8 *resp)
Quoted 8 lines
+{
drivers/hwmon/occ/occ.c line 278
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+u32 cmd1, cmd2; +u16 checksum =0 ; +u16 length_be =cpu_to_be16 (length); +int i, rc, tries =0 ;+ const int
max_tries= (OCC_BMC_TIMEOUT_S* 1000) /CMD_IN_PRG_INT_MS;I expect we should just #define the expression?
Quoted 10 lines
+
drivers/hwmon/occ/occ.c line 283
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ cmd1 = (seq <<24 ) | (type <<16 ) | length_be; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 );+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* checksum: sum of every bytes of cmd1, cmd2 */ +for (i =0 ; i <4 ; i++) { + checksum += (cmd1 >> (i *8 )) &0xFF ;
Quoted 7 lines
Quoted 1 line
+ checksum += (cmd2 >> (i * 8)) &
0xFF;I have to apologise here, as my review was wrong: Addition is commutative, but not when we're masking the result! So the original approach is probably more correct, and it only occurred to me when reading over this patch. Sorry! Did you get checksum errors as a result?
No worries, I didn't catch it either, but I think it worked out OK since the mask is really applied to the shifted cmd, but I will test both ways to make sure... Thanks!
Quoted 36 lines
Quoted 36 lines
drivers/hwmon/occ/occ.c new file +2
-0+ } +Quoted 1 line
+ cmd2 |= checksum << ((2 - length) * 8);
+
drivers/hwmon/occ/occ.c line 297
Quoted 6 lines
(quoted diff) +2
-0+ /* Init OCB */ + rc = driver->bus_ops.putscom(driver->bus,
OCB_STATUS_CONTROL_OR,drivers/hwmon/occ/occ.c new file +3
-0+ OCB_OR_INIT0, OCB_OR_INIT1); +if (rc) + goto err;+
Review
Quoted 6 lines
+ rc = driver->bus_ops.putscom(driver->bus,
OCB_STATUS_CONTROL_AND,
+OCB_AND_INIT0,OCB_AND_INIT1);(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 7 lines
+ /* Send command, 2nd half of the 64-bit addr is unused
(write 0) */(quoted diff) +2
-0+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS, + driver->config.command_addr,0);
(quoted diff) +2
-0+ if (rc) + goto err;+
Quoted 4 lines
(quoted diff) +2
-0+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS, + driver->config.command_addr,0);
+ if (rc)
Quoted 4 lines
Quoted 1 line
+ goto err;
This change highlighted to me that we're sending
command_addrtwice - is this necessary?
I'm not sure... I'll test both without and see if everything works OK. I noticed it was sent twice, but I assumed Yi had some reason for doing it.
Quoted 65 lines
Quoted 41 lines
+
Review
Quoted 4 lines
+ rc = driver->bus_ops.putscom(driver->bus,
OCB_DATA, cmd1,
cmd2);(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 5 lines
(quoted diff) +2
-0+ /* Trigger attention */ + rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA,ATTN0, ATTN1);
(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* Get response data */ + rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS, + driver->config.response_addr,0);
(quoted diff) +2
-0+ if (rc) + goto err;+
Review
Quoted 5 lines
(quoted diff) +2
-0+ do { + rc = driver->bus_ops.getscom(driver->bus,
OCB_DATA, resp);(quoted diff) +2
-0+ if (rc) + goto err;+
drivers/hwmon/occ/occ.c line 341
Quoted 10 lines
+ /* retry if we get "command in progress" return
status */
+ if (resp[RESP_RETURN_STATUS] ==RESP_RETURN_CMD_IN_PRG) {(quoted diff) +2
-0+ set_current_state(TASK_INTERRUPTIBLE); + schedule_timeout(msecs_to_jiffies(CMD_IN_P
RG_INT_MS));drivers/hwmon/occ/occ.c new file +3
-0+ tries++; + }else +break ;Quoted 1 line
+ } while (tries <
max_tries);I'll bikeshed this to try save the break and a timeout, which is really picking at the nits but reduces cyclomatic complexity and saves some time. What do you think of:
bool retry = false; int tries = 0; ... do { if (retry) { set_current_state(TASK_INTERRUPTIBLE); shedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS)); } rc = driver->bus_ops.getscom(driver->bus, OCB_DATA, resp); if (rc) goto err; retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) && tries++ < max_tries); } while (retry);
Quoted 2 lines
Are we okay with returning
RESP_RETURN_CMD_IN_PRGto the caller? Because that's what happens in either case if we exceedmax_tries.
Looks a little bit cleaner, I'll incorporate this. And yes, any non-zero return will be flagged as an error. If we timeout and are are still getting the busy response we do want an error.
Quoted 409 lines
Quoted 67 lines
+
Quoted 1 line
+ return resp[RESP_RETURN_STATUS];
drivers/hwmon/occ/occ.c new file +2
-0+ +err:Quoted 2 lines
(quoted diff) +2
-0+ dev_err(driver->dev, "scom op failed rc:%d\n", rc); + return rc;drivers/hwmon/occ/occ.c new file +4
-0+} + +static int occ_get_all (struct occ *driver) +{drivers/hwmon/occ/occ.c line 358
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+int i =0 , rc; +u8 *occ_data; +u16 num_bytes; +const u8 poll_cmd_data =OCC_POLL_STAT_SENSOR ; +struct device *dev = driver->dev; +struct occ_response *resp = &driver->response;+
drivers/hwmon/occ/occ.c line 362
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ occ_data =devm_kzalloc (dev,OCC_DATA_MAX ,GFP_KERNEL ); +if (!occ_data) +return -ENOMEM ;+
drivers/hwmon/occ/occ.c line 369
Quoted 7 lines
+ rc = occ_send_cmd(driver, 0,
OCC_POLL, 1,&poll_cmd_data,occ_data);drivers/hwmon/occ/occ.c new file +5
-0+if (rc) { +dev_err (dev,"OCC poll failed: 0x%x\n" , rc); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 373
Quoted 4 lines
+
num_bytes= get_unaligned((u16
*)&occ_data[RESP_DATA_LENGTH]);drivers/hwmon/occ/occ.c new file +2
-0+ num_bytes =be16_to_cpu (num_bytes); +dev_dbg (dev,"OCC data length: %d\n" , num_bytes);+
drivers/hwmon/occ/occ.c line 379
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes >OCC_DATA_MAX ) { +dev_err (dev,"OCC data length must be < 4KB\n" ); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 385
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes <=0 ) { +dev_err (dev,"OCC data length is zero\n" ); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 379
Quoted 11 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* read remaining data */ +for (i =8 ; i < num_bytes +8 ; i +=8 ) { + rc = driver->bus_ops.getscom(driver->bus,
OCB_DATA,drivers/hwmon/occ/occ.c new file +3
-0+ &occ_data[i]); +if (rc) { + dev_err(dev, "scom op failed rc:%d\n",rc);
drivers/hwmon/occ/occ.c new file +3
-0+ goto out; + } + }Quoted 7 lines
+
Quoted 4 lines
+ /* don't need more sanity checks; buffer is alloc'd for
max response
+ * size so we just check for valid data inparse_occ_response+ */
Thanks for the comment.
Quoted 304 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =parse_occ_response (driver, occ_data, resp); + +out:Quoted 2 lines
(quoted diff) +2
-0+ devm_kfree(dev, occ_data); + return rc;drivers/hwmon/occ/occ.c new file +4
-0+} + +int occ_update_device (struct occ *driver) +{Quoted 1 line
+ int rc = 0;
+
Quoted 1 line
+
mutex_lock(&driver->update_lock);+
drivers/hwmon/occ/occ.c line 415
Quoted 4 lines
+ if (time_after(jiffies,
driver->last_updated+ driver-Quoted 1 line
update_interval)drivers/hwmon/occ/occ.c new file +2
-0+ || !driver->valid) { + driver->valid =1 ;+
drivers/hwmon/occ/occ.c line 419
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =occ_get_all (driver); +if (rc) + driver->valid =0 ;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ driver->last_updated = jiffies; + }+
Quoted 1 line
+
mutex_unlock(&driver->update_lock);+
Quoted 1 line
+ return rc;
drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_update_device); + +void *occ_get_sensor (struct occ *driver,int sensor_type) +{Quoted 1 line
+ int rc;
+
drivers/hwmon/occ/occ.c line 440
Quoted 8 lines
drivers/hwmon/occ/occ.c new file +4
-0+/* occ_update_device locks the update lock */ + rc =occ_update_device (driver); +if (rc !=0 ) { + dev_err(driver->dev, "cannot get occ sensor data:%d\n",
drivers/hwmon/occ/occ.c new file +3
-0+ rc); +return NULL ; + }+
Quoted 2 lines
+ return occ_get_sensor_by_type(&driver->response,
sensor_type);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor); + +int occ_get_sensor_value (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
+ return
occ->ops.get_sensor_value(occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor_value); + +int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
+ return
occ->ops.get_sensor_id(occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_get_sensor_id); + +int occ_get_caps_value(struct occ *occ, void *sensor, int snum, int
caps_field)
+{Quoted 1 line
+ return
occ->ops.get_caps_value(sensor, snum, caps_field);drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_get_caps_value); + +void occ_get_response_blocks(struct occ *occ, struct occ_blocks**blocks)
+{Quoted 1 line
+ *blocks =
&occ->response.data;drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_get_response_blocks); + +void occ_set_update_interval(struct occ *occ, unsigned longinterval)
+{Quoted 1 line
+
occ->update_interval=msecs_to_jiffies(interval);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_set_update_interval); + +int occ_set_user_powercap (struct occ *occ,u16 cap) +{Quoted 1 line
+ u8 resp[8];
+
Quoted 1 line
+ cap =
cpu_to_be16(cap);+
Quoted 3 lines
+ return occ_send_cmd(occ, 0,
OCC_SET_USER_POWR_CAP, 2, (u8
*)&cap,
+ resp);drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_set_user_powercap); + +struct occ *occ_start(struct device *dev, void *bus,Quoted 3 lines
+ struct
occ_bus_ops*bus_ops, const structocc_ops*ops,
+ const structocc_config*config)+{
Quoted 2 lines
+ struct occ *driver = devm_kzalloc(dev, sizeof(struct occ),
GFP_KERNEL);+
drivers/hwmon/occ/occ.c line 494
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!driver) +return ERR_PTR (-ENOMEM );+
drivers/hwmon/occ/occ.c line 500
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+ driver->dev = dev; + driver->bus = bus; + driver->bus_ops = *bus_ops; + driver->ops = *ops; + driver->config = *config;+
drivers/hwmon/occ/occ.c line 503
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ driver->update_interval =HZ ; +mutex_init (&driver->update_lock);+
Quoted 1 line
+ return driver;
drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_start); + +int occ_stop (struct occ *occ) +{Quoted 1 line
+
devm_kfree(occ->dev, occ);+
Quoted 1 line
+ return 0;
drivers/hwmon/occ/occ.c new file +3
-0+} +EXPORT_SYMBOL (occ_stop); +Quoted 1 line
+MODULE_AUTHOR("Eddie James <[email protected]>");
drivers/hwmon/occ/occ.c new file +2
-0+MODULE_DESCRIPTION ("OCC hwmon core driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/occ.h new file +4
-0@@ -0,0 +1,86 @@ +/* +* occ.h - hwmon OCC driver +* +* This file contains data structures and function prototypes for common access
+ * between different bus protocols and host systems.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */drivers/hwmon/occ/occ.h new file +13
-0+ +#ifndef __OCC_H__ +#define __OCC_H__ + +#include "scom.h" + +struct device ; +struct occ ; + +/* sensor_data_block_header +* structure to match the raw occ sensor block header +*/ +struct sensor_data_block_header {Quoted 5 lines
drivers/hwmon/occ/occ.h new file +5
-0+u8 sensor_type[4 ]; +u8 reserved0; +u8 sensor_format; +u8 sensor_length; +u8 sensor_num;drivers/hwmon/occ/occ.h new file +3
-0+} __attribute__((packed, aligned(4))); + +struct sensor_data_block {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+struct sensor_data_block_header header; +void *sensors;drivers/hwmon/occ/occ.h new file +3
-0+}; + +enum sensor_type {Quoted 5 lines
drivers/hwmon/occ/occ.h new file +5
-0+FREQ =0 , +TEMP , +POWER , +CAPS , +MAX_OCC_SENSOR_TYPE drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_ops {Quoted 10 lines
+ void (*parse_sensor)(u8 *data, void *sensor, int
sensor_type, int off,drivers/hwmon/occ/occ.h new file +3
-0+ int snum); +void *(*alloc_sensor)(int sensor_type,int num_sensors); + int (*get_sensor_value)(struct occ *driver, int
sensor_type, int snum);
+ int (*get_sensor_id)(struct occ *driver, intsensor_type,
int snum);
+ int (*get_caps_value)(void *sensor, int snum, intcaps_field);drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_config {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+u32 command_addr; +u32 response_addr;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_blocks {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+int sensor_block_id[MAX_OCC_SENSOR_TYPE ]; +struct sensor_data_block *blocks;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ *occ_start(struct device *dev, void *bus,Quoted 3 lines
+ struct
occ_bus_ops*bus_ops, const structocc_ops*ops,
+ const structocc_config*config);drivers/hwmon/occ/occ.h new file +4
-0+int occ_stop (struct occ *occ); + +void *occ_get_sensor (struct occ *occ,int sensor_type); +int occ_get_sensor_value(struct occ *occ, int sensor_type, intsnum);
drivers/hwmon/occ/occ.h new file +2
-0+int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum); +int occ_get_caps_value(struct occ *occ, void *sensor, int snum,Quoted 1 line
+ int
caps_field);+void
occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks);drivers/hwmon/occ/occ.h new file +2
-0+int occ_update_device (struct occ *driver); +void occ_set_update_interval(struct occ *occ, unsigned longinterval);
drivers/hwmon/occ/occ.h new file +3
-0+int occ_set_user_powercap (struct occ *occ,u16 cap); + +#endif /* __OCC_H__ */ drivers/hwmon/occ/scom.h new file +8
-0@@ -0,0 +1,48 @@ +/* +* scom.h - hwmon OCC driver +* +* This file contains data structures for scom operations to the OCC +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */drivers/hwmon/occ/scom.h new file +6
-0+ +#ifndef __SCOM_H__ +#define __SCOM_H__ + +/* +* occ_bus_ops - represent the low-level transfer methods to communicate with
+ * the OCC.
+ *
+ * getscom - OCC scom read
+ * @bus: handle to slave deviceQuoted 8 lines
+ * @address: address
+ * @data: where to store data read from slave; buffer size must be greater than
+ * or equal to offset + 8 bytes.
+ * @offset: offset into data pointer
+ *
+ * Returns 0 on success or one of -SCOM_READ_ERROR, -SCOM_WRITE_ERROR.We deleted these macros. What's the plan?
Quoted 8 lines
+ *
+ * putscom - OCC scom write
+ * @bus: handle to slave device
+ * @address: address
+ * @data0: first data byte to write
+ * @data1: second data byte to write
+ *
+ * Returns 0 on success or -SCOM_WRITE_ERROR on errorAgain, this macro was deleted.
Quoted 6 lines
+ */
+structocc_bus_ops{Quoted 3 lines
(quoted diff) +2
-0+ int (*getscom)(void *bus, u32 address, u8 *data); + int (*putscom)(void *bus, u32 address, u32 data0, u32data1);
Quoted 7 lines
drivers/hwmon/occ/scom.h line 48
Quoted 3 lines
drivers/hwmon/occ/scom.h new file +3
-0+}; + +#endif /* __SCOM_H__ */ Cheers,
Andrew
Thanks,
Eddie
Hi Eddie,
On Mon, 2016-12-19 at 17:05 -0600, Eddie James wrote:
...
Quoted 20 lines
Quoted 12 lines
Quoted 12 lines
(quoted diff) +2
-0+ +static int parse_occ_response(struct occ *driver, u8 *data,Quoted 1 line
+ struct
occ_response*resp)+{
Quoted 6 lines
+ int b;
+ int s;
+ int rc;
+ int offset =SENSOR_BLOCK_OFFSET;
+ intsensor_type;
+ u8sensor_block_num;Quoted 7 lines
Quoted 1 line
+ char sensor_type_string[5] = { 0 };
Given how you rearranged things below, do you think we can get away with removing
sensor_type_string? Also, structsensor_data_block_typedefinessensor_typeas a char * that can't be null-terminated as its size is 4, which still makes me feel uncomfortable. What are your thoughts here?
Quoted 8 lines
The idea of
sensor_type_stringwas to have a null-terminated string that I could use in the debug statements below.sensor_typeis restricted to 4 chars because that's how its' formatted in the OCC data block. Anywhere I usesensor_typeI use the length-limited string function, such as strncmp or the like, and restrict it to 4 chars. I feel this is an acceptable solution, but you're right,sensor_type_stringisn't strictly necessary, just useful to be able to print that out.
Okay, I'm happy with that.
Quoted 29 lines
Quoted 29 lines
Quoted 6 lines
(quoted diff) +2
-0+ +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16length,
Quoted 1 line
+ u8 *data, u8 *resp)
Quoted 8 lines
+{
Quoted 4 lines
+ u32 cmd1, cmd2;
+ u16 checksum = 0;
+ u16length_be=cpu_to_be16(length);
+ int i, rc, tries = 0;+ const int
max_tries= (OCC_BMC_TIMEOUT_S* 1000) /CMD_IN_PRG_INT_MS;I expect we should just #define the expression?
Quoted 11 lines
+
Quoted 3 lines
+ cmd1 = (seq << 24) | (type << 16) |
length_be; + memcpy(&cmd2, data, length);
+ cmd2 <<= ((4 - length) * 8);+
Quoted 3 lines
+ /* checksum: sum of every bytes of cmd1, cmd2 */ + for (i = 0; i < 4; i++) {
+ checksum += (cmd1 >> (i * 8)) &0xFF;+ checksum += (cmd2 >> (i * 8)) &
0xFF;
Quoted 8 lines
Quoted 5 lines
I have to apologise here, as my review was wrong: Addition is commutative, but not when we're masking the result! So the original approach is probably more correct, and it only occurred to me when reading over this patch. Sorry! Did you get checksum errors as a result?
No worries, I didn't catch it either, but I think it worked out OK since the mask is really applied to the shifted cmd,
Yes, I thought about this further after I sent the email and I agree.
Quoted 39 lines
but I will test
both ways to make sure... Thanks!Quoted 36 lines
Quoted 36 lines
+ }
+Quoted 1 line
+ cmd2 |= checksum << ((2 - length) * 8);
+
Quoted 6 lines
+ /* Init OCB */
+ rc = driver->bus_ops.putscom(driver->bus,OCB_STATUS_CONTROL_OR,
+ OCB_OR_INIT0,OCB_OR_INIT1); + if (rc)
+ goto err;+
Quoted 6 lines
+ rc = driver->bus_ops.putscom(driver->bus,
OCB_STATUS_CONTROL_AND,
+ OCB_AND_INIT0,OCB_AND_INIT1);
+ if (rc)
+ goto err;+
Quoted 7 lines
+ /* Send command, 2nd half of the 64-bit addr is unused (write 0) */
+ rc = driver->bus_ops.putscom(driver->bus,OCB_ADDRESS, + driver->config.command_addr, 0);
+ if (rc)
+ goto err;+
Quoted 4 lines
+ rc = driver->bus_ops.putscom(driver->bus,
OCB_ADDRESS, + driver->config.command_addr, 0);
+ if (rc)
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
+ goto err;
This change highlighted to me that we're sending
command_addrtwice - is this necessary?I'm not sure... I'll test both without and see if everything works OK. I noticed it was sent twice, but I assumed Yi had some reason for doing it.
I think this will be a win either way: Either we remove one, or we document why we send it twice.
Quoted 66 lines
Quoted 66 lines
Quoted 42 lines
+
Quoted 4 lines
+ rc =
driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1, cmd2);
+ if (rc)
+ goto err;+
Quoted 5 lines
+ /* Trigger attention */
+ rc =driver->bus_ops.putscom(driver->bus, ATTN_DATA, ATTN0, ATTN1);
+ if (rc)
+ goto err;+
Quoted 6 lines
+ /* Get response data */
+ rc = driver->bus_ops.putscom(driver->bus,OCB_ADDRESS, + driver->config.response_addr, 0);
+ if (rc)
+ goto err;+
Quoted 5 lines
+ do {
+ rc =driver->bus_ops.getscom(driver->bus, OCB_DATA, resp);
+ if (rc)
+ goto err;+
Quoted 10 lines
+ /* retry if we get "command in progress" return status */
+ if (resp[RESP_RETURN_STATUS] ==RESP_RETURN_CMD_IN_PRG) {
+ set_current_state(TASK_INTERRUPTIBLE); + schedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
+ tries++;
+ } else
+ break;Quoted 1 line
+ } while (tries <
max_tries);I'll bikeshed this to try save the break and a timeout, which is really picking at the nits but reduces cyclomatic complexity and saves some time. What do you think of:
bool retry = false;
int tries = 0;...
do {
if (retry) {set_current_state(TASK_INTERRUPTIBLE); shedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS)); }rc =
driver->bus_ops.getscom(driver->bus, OCB_DATA, resp); if (rc)
goto err;retry = (resp[RESP_RETURN_STATUS] ==
RESP_RETURN_CMD_IN_PRG) && tries++ <max_tries); } while (retry);
Quoted 6 lines
Quoted 2 lines
Are we okay with returning
RESP_RETURN_CMD_IN_PRGto the caller? Because that's what happens in either case if we exceedmax_tries.Looks a little bit cleaner, I'll incorporate this. And yes, any non-zero return will be flagged as an error. If we timeout and are are still getting the busy response we do want an error.
But a "kernel" error? Or the value RESP_RETURN_CMD_IN_PRG?
Andrew
Hi Eddie,
I think this is looking good. A few comments below.
Quoted 60 lines · On Fri, 2016-12-16 at 14:34 -0600, [email protected] wrote:
Quoted 1 line
From: "Edward A. James" <[email protected]>
Add core support for polling the OCC for it's sensor data and parsing that data into sensor-specific information.
Quoted 2 lines
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>---
Documentation/hwmon/occ | 40 ++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 15 ++
drivers/hwmon/occ/Makefile | 1 +
drivers/hwmon/occ/occ.c | 519 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ.h | 86 ++++++++ drivers/hwmon/occ/scom.h | 48 +++++8 files changed, 712 insertions(+)create mode 100644
Documentation/hwmon/occ
create mode 100644drivers/hwmon/occ/Kconfig
create mode 100644drivers/hwmon/occ/Makefile
create mode 100644drivers/hwmon/occ/occ.c
create mode 100644drivers/hwmon/occ/occ.h
create mode 100644drivers/hwmon/occ/scom.hDocumentation/hwmon/occ new file +10
-0@@ -0,0 +1,40 @@ +Kernel driver occ +================= + +Supported chips: + * ASPEED AST2400 + * ASPEED AST2500 + +Please note that the chip must be connected to a POWER8 or POWER9 processor +(see the BMC - Host Communications section). +Quoted 1 line
+Author: Eddie James <[email protected]>
Documentation/hwmon/occ new file +19
-0+ +Description +----------- + +This driver implements support for the OCC (On-Chip Controller) on the IBM +POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The +OCC is an embedded processor that provides real time power and thermal +monitoring. + +This driver provides an interface on a BMC to poll OCC sensor data, set user +power caps, and perform some basic OCC error handling. + +Currently, all versions of the OCC support four types of sensor data: power, +temperature, frequency, and "caps," which indicate limits and thresholds used +internally on the OCC. + +BMC - Host Communications +------------------------- +
Documentation/hwmon/occ line 40
Quoted 10 lines
Documentation/hwmon/occ new file +10
-0+For the POWER8 application, the BMC can communicate with the P8 over I2C bus. +However, to access the OCC register space, any data transfer must use a SCOM +operation. SCOM is a procedure to initiate a data transfer, typically of 8 +bytes. SCOMs consist of writing a 32-bit command register and then +reading/writing two 32-bit data registers. This driver implements these +SCOM operations over I2C bus in order to communicate with the OCC. + +For the POWER9 application, the BMC can communicate with the P9 over FSI bus +and SBE engine. Once again, SCOM operations are required. This driver will +implement SCOM ops over FSI/SBE. This will require the FSI driver.
Great!
Quoted 116 lines
drivers/hwmon/Kconfig +0
-0@@ -1229,6 +1229,8 @@ config SENSORS_NSA320Quoted 2 lines
This driver can also be built as a module. If so, the module will be called nsa320-hwmon.
drivers/hwmon/Kconfig +2
-0+source drivers/hwmon/occ/Kconfig +config
SENSORS_PCF8591Quoted 2 lines
tristate "Philips PCF8591 ADC/DAC"
depends on I2Cdrivers/hwmon/Makefile +0
-0Quoted 3 lines
(quoted diff) +0
-0@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.oobj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.oQuoted 2 lines
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ/
obj-$(CONFIG_PMBUS) += pmbus/
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUGdrivers/hwmon/occ/Kconfig new file +5
-0@@ -0,0 +1,15 @@ +# +# On Chip Controller configuration +# + +menuconfig SENSORS_PPC_OCCdrivers/hwmon/occ/Kconfig line 9
Quoted 4 lines
drivers/hwmon/occ/Kconfig new file +4
-0+ bool "PPC On-Chip Controller" + help + If you say yes here you get support to monitor Power CPU + sensors via the On-Chip Controller (OCC).+
drivers/hwmon/occ/Kconfig line 12
Quoted 2 lines
drivers/hwmon/occ/Kconfig new file +2
-0+ Generally this is used by management controllers such as a BMC + on an OpenPower system.+
Quoted 2 lines
drivers/hwmon/occ/Kconfig new file +2
-0+ This driver can also be built as a module. If so, the module + will be called occ.drivers/hwmon/occ/Makefile new file +1
-0@@ -0,0 +1 @@ +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.odrivers/hwmon/occ/occ.c new file +33
-0@@ -0,0 +1,519 @@ +/* +* occ.c - OCC hwmon driver +* +* This file contains the methods and data structures for the OCC hwmon driver. +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/jiffies.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/err.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/kernel.h> +#include <linux/device.h> +#include <asm/unaligned.h> + +#include "occ.h" +Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define OCC_DATA_MAX 4096 +#define OCC_BMC_TIMEOUT_S 20drivers/hwmon/occ/occ.c new file +2
-0+ +/* To generate attn to OCC */ Quoted 1 line
+#define
ATTN_DATA0x0006B035drivers/hwmon/occ/occ.c new file +2
-0+ +/* For BMC to read/write SRAM */ Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+#define OCB_ADDRESS 0x0006B070 +#define OCB_DATA 0x0006B075 +#define OCB_STATUS_CONTROL_AND 0x0006B072 +#define OCB_STATUS_CONTROL_OR 0x0006B073drivers/hwmon/occ/occ.c new file +2
-0+ +/* To init OCB */ Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+#define OCB_AND_INIT0 0xFBFFFFFF +#define OCB_AND_INIT1 0xFFFFFFFF +#define OCB_OR_INIT0 0x08000000 +#define OCB_OR_INIT1 0x00000000drivers/hwmon/occ/occ.c new file +2
-0+ +/* To generate attention on OCC */ Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define ATTN0 0x01010000 +#define ATTN1 0x00000000drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC return status for "command in progress" */ Quoted 1 line
+#define
RESP_RETURN_CMD_IN_PRG0xFF
drivers/hwmon/occ/occ.c line 59
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+/* time interval to retry on "command in progress" return status */ +#define CMD_IN_PRG_INT_MS 100
Out of curiosity, why are we defining one period in MS and another in S (OCC_BMC_TIMEOUT_S)? It feels like it would make sense to use consistent units.
Regardless, thanks for defining the macros above rather than using the magic numbers.
Quoted 127 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC command definitions */ Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+#define OCC_POLL 0 +#define OCC_SET_USER_POWR_CAP 0x22drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC poll command data */ Quoted 1 line
+#define
OCC_POLL_STAT_SENSOR0x10drivers/hwmon/occ/occ.c new file +2
-0+ +/* OCC response data offsets */ Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+#define RESP_RETURN_STATUS 2 +#define RESP_DATA_LENGTH 3 +#define RESP_HEADER_OFFSET 5 +#define SENSOR_STR_OFFSET 37 +#define SENSOR_BLOCK_NUM_OFFSET 43 +#define SENSOR_BLOCK_OFFSET 45drivers/hwmon/occ/occ.c new file +5
-0+ +/* occ_poll_header +* structure to match the raw occ poll response data +*/ +struct occ_poll_header {Quoted 16 lines
drivers/hwmon/occ/occ.c new file +16
-0+u8 status; +u8 ext_status; +u8 occs_present; +u8 config; +u8 occ_state; +u8 mode; +u8 ips_status; +u8 error_log_id; +u32 error_log_addr_start; +u16 error_log_length; +u8 reserved2; +u8 reserved3; +u8 occ_code_level[16 ]; +u8 sensor_eye_catcher[6 ]; +u8 sensor_block_num; +u8 sensor_data_version;drivers/hwmon/occ/occ.c new file +3
-0+} __attribute__((packed, aligned(4))); + +struct occ_response {Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+struct occ_poll_header header; +struct occ_blocks data;drivers/hwmon/occ/occ.c new file +3
-0+}; + +struct occ {Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+struct device *dev; +void *bus; +struct occ_bus_ops bus_ops; +struct occ_ops ops; +struct occ_config config; +unsigned long update_interval; +unsigned long last_updated; +struct mutex update_lock; +struct occ_response response; +bool valid;drivers/hwmon/occ/occ.c new file +4
-0+}; + +static void deinit_occ_resp_buf (struct occ_response *resp) +{drivers/hwmon/occ/occ.c line 118
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp) + return;+
drivers/hwmon/occ/occ.c line 124
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!resp->data.blocks) +return ;+
drivers/hwmon/occ/occ.c line 127
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+for (i =0 ; i < resp->header.sensor_block_num; ++i) +kfree (resp->data.blocks[i].sensors);+
drivers/hwmon/occ/occ.c line 129
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+kfree (resp->data.blocks);+
drivers/hwmon/occ/occ.c line 131
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+memset (resp,0 ,sizeof (struct occ_response ));+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+for (i =0 ; i <MAX_OCC_SENSOR_TYPE ; ++i) + resp->data.sensor_block_id[i] =-1 ;drivers/hwmon/occ/occ.c new file +3
-0+} + +static void *occ_get_sensor_by_type(struct occ_response *resp,drivers/hwmon/occ/occ.c line 138
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ enum sensor_type t)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp->data.blocks) + return NULL;+
drivers/hwmon/occ/occ.c line 144
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (resp->data.sensor_block_id[t] ==-1 ) +return NULL ;+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return resp->data.blocks[resp->data.sensor_block_id[t]].sensors;drivers/hwmon/occ/occ.c new file +3
-0+} + +static int occ_check_sensor(struct occ *driver, u8 sensor_length,drivers/hwmon/occ/occ.c line 150
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ u8 sensor_num, enum sensor_type t, int block)+{
drivers/hwmon/occ/occ.c line 154
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+void *sensor; +int type_block_id; +struct occ_response *resp = &driver->response;+
drivers/hwmon/occ/occ.c line 156
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ sensor =occ_get_sensor_by_type (resp, t);+
drivers/hwmon/occ/occ.c line 163
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+/* empty sensor block, release older sensor data */ +if (sensor_num ==0 || sensor_length ==0 ) { +kfree (sensor); +dev_err (driver->dev,"no sensor blocks available\n" ); +return -ENODATA ; + }+
drivers/hwmon/occ/occ.c line 173
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +9
-0+ type_block_id = resp->data.sensor_block_id[t]; +if (!sensor || sensor_num != + resp->data.blocks[type_block_id].header.sensor_num) { +kfree (sensor); + resp->data.blocks[block].sensors = + driver->ops.alloc_sensor (t, sensor_num); +if (!resp->data.blocks[block].sensors) +return -ENOMEM ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/hwmon/occ/occ.c new file +3
-0+} + +static int parse_occ_response(struct occ *driver, u8 *data,drivers/hwmon/occ/occ.c line 179
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ struct occ_response *resp)+{
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+int b; +int s; +int rc; +int offset =SENSOR_BLOCK_OFFSET ; +int sensor_type; +u8 sensor_block_num;
drivers/hwmon/occ/occ.c line 187
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+char sensor_type_string[5 ] = {0 };
Given how you rearranged things below, do you think we can get away with removing sensor_type_string? Also, struct sensor_data_block_type defines sensor_type as a char * that can't be null-terminated as its size is 4, which still makes me feel uncomfortable. What are your thoughts here?
Quoted 37 lines
drivers/hwmon/occ/occ.c new file +1
-0+struct sensor_data_block_header *block;Review
Quoted 1 line
(quoted diff) +1
-0+ struct device *dev = driver->dev;+
drivers/hwmon/occ/occ.c line 196
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+/* check if the data is valid */ +if (strncmp (&data[SENSOR_STR_OFFSET ],"SENSOR" ,6 ) !=0 ) { +dev_err (dev,"no SENSOR string in response\n" ); + rc = -ENODATA ; + goto err; + }+
drivers/hwmon/occ/occ.c line 203
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET ]; +if (sensor_block_num ==0 ) { +dev_err (dev,"no sensor blocks available\n" ); + rc = -ENODATA ; + goto err; + }+
drivers/hwmon/occ/occ.c line 213
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +9
-0+/* if number of sensor block has changed, re-malloc */ +if (sensor_block_num != resp->header.sensor_block_num) { +deinit_occ_resp_buf (resp); + resp->data.blocks =kcalloc (sensor_block_num, + sizeof(struct sensor_data_block), + GFP_KERNEL); +if (!resp->data.blocks) +return -ENOMEM ; + }+
drivers/hwmon/occ/occ.c line 220
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+memcpy (&resp->header, &data[RESP_HEADER_OFFSET ], + sizeof(struct occ_poll_header)); + resp->header.error_log_addr_start = +be32_to_cpu (resp->header.error_log_addr_start); + resp->header.error_log_length = +be16_to_cpu (resp->header.error_log_length);+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+dev_dbg (dev,"Reading %d sensor blocks\n" , + resp->header.sensor_block_num); +for (b =0 ; b < sensor_block_num; b++) {
drivers/hwmon/occ/occ.c line 225
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ block = (struct sensor_data_block_header *)&data[offset];
+ /* copy to a null terminated string */
Quoted 52 lines
drivers/hwmon/occ/occ.c line 228
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+strncpy (sensor_type_string, block->sensor_type,4 ); + offset +=8 ;+
drivers/hwmon/occ/occ.c line 231
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+dev_dbg (dev,"sensor block[%d]: type: %s, sensor_num: %d\n" , b, + sensor_type_string, block->sensor_num);+
drivers/hwmon/occ/occ.c line 245
Quoted 13 lines
drivers/hwmon/occ/occ.c new file +13
-0+if (strncmp (block->sensor_type,"FREQ" ,4 ) ==0 ) + sensor_type =FREQ ; +else if (strncmp (block->sensor_type,"TEMP" ,4 ) ==0 ) + sensor_type =TEMP ; +else if (strncmp (block->sensor_type,"POWR" ,4 ) ==0 ) + sensor_type =POWER ; +else if (strncmp (block->sensor_type,"CAPS" ,4 ) ==0 ) + sensor_type =CAPS ; +else { +dev_err (dev,"sensor type not supported %s\n" , + sensor_type_string); +continue ; + }+
drivers/hwmon/occ/occ.c line 252
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ rc =occ_check_sensor (driver, block->sensor_length, + block->sensor_num, sensor_type, b); +if (rc == -ENOMEM ) + goto err; +else if (rc) +continue ;+
drivers/hwmon/occ/occ.c line 260
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +7
-0+ resp->data.sensor_block_id[sensor_type] = b; +for (s =0 ; s < block->sensor_num; s++) { + driver->ops.parse_sensor (data, + resp->data.blocks[b].sensors, + sensor_type, offset, s); + offset += block->sensor_length; + }+
drivers/hwmon/occ/occ.c line 264
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* copy block data over to response pointer */ + resp->data.blocks[b].header = *block; + }+
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;+err:
Quoted 2 lines
(quoted diff) +2
-0+ deinit_occ_resp_buf(resp); + return rc;drivers/hwmon/occ/occ.c new file +3
-0+} + +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,drivers/hwmon/occ/occ.c line 273
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ u8 *data, u8 *resp)+{
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+u32 cmd1, cmd2; +u16 checksum =0 ; +u16 length_be =cpu_to_be16 (length); +int i, rc, tries =0 ;
drivers/hwmon/occ/occ.c line 279
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+const int max_tries = (OCC_BMC_TIMEOUT_S *1000 ) /CMD_IN_PRG_INT_MS ;
I expect we should just #define the expression?
drivers/hwmon/occ/occ.c line 288
Quoted 9 lines
+
drivers/hwmon/occ/occ.c line 283
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ cmd1 = (seq <<24 ) | (type <<16 ) | length_be; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 );+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* checksum: sum of every bytes of cmd1, cmd2 */ +for (i =0 ; i <4 ; i++) { + checksum += (cmd1 >> (i *8 )) &0xFF ;drivers/hwmon/occ/occ.c new file +1
-0+ checksum += (cmd2 >> (i *8 )) &0xFF ;
I have to apologise here, as my review was wrong: Addition is commutative, but not when we're masking the result (so the original approach is probably more correct)! It only occurred to me when reading over this patch. Sorry! Did you get checksum errors as a result?
Quoted 24 lines
drivers/hwmon/occ/occ.c new file +2
-0+ } +drivers/hwmon/occ/occ.c line 291
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ cmd2 |= checksum << ((2 - length) *8 );+
drivers/hwmon/occ/occ.c line 297
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+/* Init OCB */ + rc = driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_OR , + OCB_OR_INIT0, OCB_OR_INIT1); +if (rc) + goto err;+
drivers/hwmon/occ/occ.c line 302
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+ rc = driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_AND , + OCB_AND_INIT0, OCB_AND_INIT1); +if (rc) + goto err;+
drivers/hwmon/occ/occ.c line 308
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+/* Send command, 2nd half of the 64-bit addr is unused (write 0) */ + rc = driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr, 0); +if (rc) + goto err;+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc = driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr, 0); +if (rc)
drivers/hwmon/occ/occ.c
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ goto err;
This change highlighted to me that we're sending command_addr twice - is this necessary?
Quoted 28 lines
+
drivers/hwmon/occ/occ.c line 317
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc = driver->bus_ops.putscom (driver->bus,OCB_DATA , cmd1, cmd2); +if (rc) + goto err;+
drivers/hwmon/occ/occ.c line 322
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+/* Trigger attention */ + rc = driver->bus_ops.putscom (driver->bus,ATTN_DATA ,ATTN0 ,ATTN1 ); +if (rc) + goto err;+
drivers/hwmon/occ/occ.c line 328
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+/* Get response data */ + rc = driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.response_addr, 0); +if (rc) + goto err;+
drivers/hwmon/occ/occ.c line 333
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+ do { + rc = driver->bus_ops.getscom(driver->bus, OCB_DATA, resp); + if (rc) + goto err;+
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +7
-0+/* retry if we get "command in progress" return status */ +if (resp[RESP_RETURN_STATUS ] ==RESP_RETURN_CMD_IN_PRG ) { +set_current_state (TASK_INTERRUPTIBLE ); +schedule_timeout (msecs_to_jiffies (CMD_IN_PRG_INT_MS )); + tries++; + }else +break ;
drivers/hwmon/occ/occ.c line 342
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ }while (tries < max_tries);
I'll bikeshed this to try save the break and a timeout, which is really picking at the nits but reduces cyclomatic complexity and saves some time. What do you think of:
bool retry = false;
int tries = 0;
...
do {
if (retry) {
set_current_state(TASK_INTERRUPTIBLE);
shedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
}
rc = driver->bus_ops.getscom(driver->bus, OCB_DATA, resp);
if (rc)
goto err;
retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) && tries++ < max_tries);
} while (retry);Are we okay with returning RESP_RETURN_CMD_IN_PRG to the caller? Because that's what happens in either case if we exceed max_tries.
Quoted 56 lines
+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return resp[RESP_RETURN_STATUS ];drivers/hwmon/occ/occ.c new file +2
-0+ +err:Quoted 2 lines
(quoted diff) +2
-0+ dev_err(driver->dev, "scom op failed rc:%d\n", rc); + return rc;drivers/hwmon/occ/occ.c new file +4
-0+} + +static int occ_get_all (struct occ *driver) +{drivers/hwmon/occ/occ.c line 358
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+int i =0 , rc; +u8 *occ_data; +u16 num_bytes; +const u8 poll_cmd_data =OCC_POLL_STAT_SENSOR ; +struct device *dev = driver->dev; +struct occ_response *resp = &driver->response;+
drivers/hwmon/occ/occ.c line 362
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ occ_data =devm_kzalloc (dev,OCC_DATA_MAX ,GFP_KERNEL ); +if (!occ_data) +return -ENOMEM ;+
drivers/hwmon/occ/occ.c line 369
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ rc =occ_send_cmd (driver,0 ,OCC_POLL ,1 , &poll_cmd_data, occ_data); +if (rc) { +dev_err (dev,"OCC poll failed: 0x%x\n" , rc); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 373
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ num_bytes =get_unaligned ((u16 *)&occ_data[RESP_DATA_LENGTH ]); + num_bytes =be16_to_cpu (num_bytes); +dev_dbg (dev,"OCC data length: %d\n" , num_bytes);+
drivers/hwmon/occ/occ.c line 379
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes >OCC_DATA_MAX ) { +dev_err (dev,"OCC data length must be < 4KB\n" ); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 385
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes <=0 ) { +dev_err (dev,"OCC data length is zero\n" ); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 395
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +9
-0+/* read remaining data */ +for (i =8 ; i < num_bytes +8 ; i +=8 ) { + rc = driver->bus_ops.getscom (driver->bus,OCB_DATA , + &occ_data[i]); +if (rc) { +dev_err (dev,"scom op failed rc:%d\n" , rc); + goto out; + } + }+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+/* don't need more sanity checks; buffer is alloc'd for max response +* size so we just check for valid data in parse_occ_response
drivers/hwmon/occ/occ.c
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+*/
Thanks for the comment.
Quoted 241 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =parse_occ_response (driver, occ_data, resp); + +out:Quoted 2 lines
(quoted diff) +2
-0+ devm_kfree(dev, occ_data); + return rc;drivers/hwmon/occ/occ.c new file +4
-0+} + +int occ_update_device (struct occ *driver) +{drivers/hwmon/occ/occ.c line 409
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+int rc =0 ;+
drivers/hwmon/occ/occ.c line 411
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+mutex_lock (&driver->update_lock);+
drivers/hwmon/occ/occ.c line 415
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+if (time_after (jiffies, driver->last_updated + driver->update_interval) + || !driver->valid) { + driver->valid =1 ;+
drivers/hwmon/occ/occ.c line 419
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =occ_get_all (driver); +if (rc) + driver->valid =0 ;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ driver->last_updated = jiffies; + }+
drivers/hwmon/occ/occ.c line 424
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+mutex_unlock (&driver->update_lock);+
Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_update_device); + +void *occ_get_sensor (struct occ *driver,int sensor_type) +{Review
Quoted 1 line
(quoted diff) +1
-0+ int rc;+
drivers/hwmon/occ/occ.c line 440
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +7
-0+/* occ_update_device locks the update lock */ + rc =occ_update_device (driver); +if (rc !=0 ) { +dev_err (driver->dev,"cannot get occ sensor data: %d\n" , + rc); +return NULL ; + }+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ_get_sensor_by_type (&driver->response, sensor_type);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor); + +int occ_get_sensor_value (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_sensor_value (occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor_value); + +int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_sensor_id (occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor_id); + +int occ_get_caps_value (struct occ *occ,void *sensor,int snum,int caps_field) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_caps_value (sensor, snum, caps_field);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_caps_value); + +void occ_get_response_blocks (struct occ *occ,struct occ_blocks **blocks) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ *blocks = &occ->response.data;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_response_blocks); + +void occ_set_update_interval (struct occ *occ,unsigned long interval) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ occ->update_interval =msecs_to_jiffies (interval);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_set_update_interval); + +int occ_set_user_powercap (struct occ *occ,u16 cap) +{drivers/hwmon/occ/occ.c line 478
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+u8 resp[8 ];+
drivers/hwmon/occ/occ.c line 480
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ cap =cpu_to_be16 (cap);+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+return occ_send_cmd (occ,0 ,OCC_SET_USER_POWR_CAP ,2 , (u8 *)&cap, + resp);drivers/hwmon/occ/occ.c new file +4
-0+} +EXPORT_SYMBOL (occ_set_user_powercap); + +struct occ *occ_start(struct device *dev, void *bus,drivers/hwmon/occ/occ.c line 489
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ struct occ_bus_ops *bus_ops, const struct occ_ops *ops, + const struct occ_config *config)+{
drivers/hwmon/occ/occ.c line 491
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+struct occ *driver =devm_kzalloc (dev,sizeof (struct occ ),GFP_KERNEL );+
drivers/hwmon/occ/occ.c line 494
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!driver) +return ERR_PTR (-ENOMEM );+
drivers/hwmon/occ/occ.c line 500
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+ driver->dev = dev; + driver->bus = bus; + driver->bus_ops = *bus_ops; + driver->ops = *ops; + driver->config = *config;+
drivers/hwmon/occ/occ.c line 503
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ driver->update_interval =HZ ; +mutex_init (&driver->update_lock);+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return driver;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_start); + +int occ_stop (struct occ *occ) +{drivers/hwmon/occ/occ.c line 511
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+devm_kfree (occ->dev, occ);+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/hwmon/occ/occ.c new file +3
-0+} +EXPORT_SYMBOL (occ_stop); +Quoted 1 line
+MODULE_AUTHOR("Eddie James <[email protected]>");
drivers/hwmon/occ/occ.c new file +2
-0+MODULE_DESCRIPTION ("OCC hwmon core driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/occ.h new file +31
-0@@ -0,0 +1,86 @@ +/* +* occ.h - hwmon OCC driver +* +* This file contains data structures and function prototypes for common access +* between different bus protocols and host systems. +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __OCC_H__ +#define __OCC_H__ + +#include "scom.h" + +struct device ; +struct occ ; + +/* sensor_data_block_header +* structure to match the raw occ sensor block header +*/ +struct sensor_data_block_header {Quoted 5 lines
drivers/hwmon/occ/occ.h new file +5
-0+u8 sensor_type[4 ]; +u8 reserved0; +u8 sensor_format; +u8 sensor_length; +u8 sensor_num;drivers/hwmon/occ/occ.h new file +3
-0+} __attribute__((packed, aligned(4))); + +struct sensor_data_block {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+struct sensor_data_block_header header; +void *sensors;drivers/hwmon/occ/occ.h new file +3
-0+}; + +enum sensor_type {Quoted 5 lines
drivers/hwmon/occ/occ.h new file +5
-0+FREQ =0 , +TEMP , +POWER , +CAPS , +MAX_OCC_SENSOR_TYPE drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_ops {Quoted 6 lines
drivers/hwmon/occ/occ.h new file +6
-0+void (*parse_sensor)(u8 *data,void *sensor,int sensor_type,int off, + int snum); +void *(*alloc_sensor)(int sensor_type,int num_sensors); +int (*get_sensor_value)(struct occ *driver,int sensor_type,int snum); +int (*get_sensor_id)(struct occ *driver,int sensor_type,int snum); +int (*get_caps_value)(void *sensor,int snum,int caps_field);drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_config {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+u32 command_addr; +u32 response_addr;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_blocks {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+int sensor_block_id[MAX_OCC_SENSOR_TYPE ]; +struct sensor_data_block *blocks;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ *occ_start(struct device *dev, void *bus,Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+ struct occ_bus_ops *bus_ops, const struct occ_ops *ops, + const struct occ_config *config);drivers/hwmon/occ/occ.h new file +6
-0+int occ_stop (struct occ *occ); + +void *occ_get_sensor (struct occ *occ,int sensor_type); +int occ_get_sensor_value (struct occ *occ,int sensor_type,int snum); +int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum); +int occ_get_caps_value(struct occ *occ, void *sensor, int snum,Quoted 1 line
drivers/hwmon/occ/occ.h new file +1
-0+ int caps_field);drivers/hwmon/occ/occ.h new file +6
-0+void occ_get_response_blocks (struct occ *occ,struct occ_blocks **blocks); +int occ_update_device (struct occ *driver); +void occ_set_update_interval (struct occ *occ,unsigned long interval); +int occ_set_user_powercap (struct occ *occ,u16 cap); + +#endif /* __OCC_H__ */ drivers/hwmon/occ/scom.h new file +23
-0@@ -0,0 +1,48 @@ +/* +* scom.h - hwmon OCC driver +* +* This file contains data structures for scom operations to the OCC +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __SCOM_H__ +#define __SCOM_H__ + +/* +* occ_bus_ops - represent the low-level transfer methods to communicate with
drivers/hwmon/occ/scom.h line 33
Quoted 10 lines
drivers/hwmon/occ/scom.h new file +10
-0+* the OCC. +* +* getscom - OCC scom read +* @bus: handle to slave device +* @address: address +* @data: where to store data read from slave; buffer size must be greater than +* or equal to offset + 8 bytes. +* @offset: offset into data pointer +* +* Returns 0 on success or one of -SCOM_READ_ERROR, -SCOM_WRITE_ERROR.
We deleted these macros. What's the plan?
drivers/hwmon/occ/scom.h line 41
Quoted 8 lines
drivers/hwmon/occ/scom.h new file +8
-0+* +* putscom - OCC scom write +* @bus: handle to slave device +* @address: address +* @data0: first data byte to write +* @data1: second data byte to write +* +* Returns 0 on success or -SCOM_WRITE_ERROR on error
Again, this macro was deleted.
drivers/hwmon/occ/scom.h line 48
Quoted 7 lines
drivers/hwmon/occ/scom.h new file +2
-0+*/ +struct occ_bus_ops {Quoted 2 lines
drivers/hwmon/occ/scom.h new file +2
-0+int (*getscom)(void *bus,u32 address,u8 *data); +int (*putscom)(void *bus,u32 address,u32 data0,u32 data1);drivers/hwmon/occ/scom.h new file +3
-0+}; + +#endif /* __SCOM_H__ */
Cheers,
Andrew