drivers: hwmon: Add On-Chip Controller driver
Subsystems (3)
v7 cover drivers: hwmon: Add On-Chip Controller driver
From: "Edward A. James" <[email protected]>
This patchset adds a hwmon driver to support 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.
The driver provides an interface on a BMC to poll OCC sensor data, set user power caps, and perform some basic OCC error handling. It interfaces with userspace through hwmon.
The driver is currently functional only for the OCC on POWER8 chips. Communicating with the POWER9 OCC requries FSI support.
Edward A. James (6):
hwmon: Add core On-Chip Controller support for POWER CPUs
hwmon: occ: Add sysfs interface
hwmon: occ: Add I2C transport implementation for SCOM operations
hwmon: occ: Add callbacks for parsing P8 OCC datastructures
hwmon: occ: Add hwmon implementation for the P8 OCC
hwmon: occ: Add callbacks for parsing P9 OCC datastructures
Documentation/devicetree/bindings/hwmon/occ.txt | 13 + Documentation/hwmon/occ | 114 ++++++ MAINTAINERS | 7 + drivers/hwmon/Kconfig | 2 + drivers/hwmon/Makefile | 1 + drivers/hwmon/occ/Kconfig | 29 ++ drivers/hwmon/occ/Makefile | 2 + drivers/hwmon/occ/occ.c | 448 ++++++++++++++++++++++++ drivers/hwmon/occ/occ.h | 81 +++++ drivers/hwmon/occ/occ_p8.c | 248 +++++++++++++ drivers/hwmon/occ/occ_p8.h | 30 ++ drivers/hwmon/occ/occ_p9.c | 309 ++++++++++++++++ drivers/hwmon/occ/occ_p9.h | 30 ++ drivers/hwmon/occ/occ_scom_i2c.c | 77 ++++ drivers/hwmon/occ/occ_scom_i2c.h | 26 ++ drivers/hwmon/occ/occ_sysfs.c | 251 +++++++++++++ drivers/hwmon/occ/occ_sysfs.h | 30 ++ drivers/hwmon/occ/p8_occ_i2c.c | 104 ++++++ drivers/hwmon/occ/scom.h | 47 +++ 19 files changed, 1849 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt create mode 100644 Documentation/hwmon/occ create mode 100644 drivers/hwmon/occ/Kconfig create mode 100644 drivers/hwmon/occ/Makefile create mode 100644 drivers/hwmon/occ/occ.c create mode 100644 drivers/hwmon/occ/occ.h create mode 100644 drivers/hwmon/occ/occ_p8.c create mode 100644 drivers/hwmon/occ/occ_p8.h create mode 100644 drivers/hwmon/occ/occ_p9.c create mode 100644 drivers/hwmon/occ/occ_p9.h create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h create mode 100644 drivers/hwmon/occ/occ_sysfs.c create mode 100644 drivers/hwmon/occ/occ_sysfs.h create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c create mode 100644 drivers/hwmon/occ/scom.h
Signature · 1 line
1.8.3.1
PATCH v7 1/6 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 ++++ MAINTAINERS | 7 + drivers/hwmon/Kconfig | 2 + drivers/hwmon/Makefile | 1 + drivers/hwmon/occ/Kconfig | 15 ++ drivers/hwmon/occ/Makefile | 1 + drivers/hwmon/occ/occ.c | 448 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ.h | 81 ++++++++ drivers/hwmon/occ/scom.h | 47 +++++ 9 files changed, 642 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.
MAINTAINERS +7 -0
@@ -9128,6 +9128,13 @@ T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov7670.c +ON-CHIP CONTROLLER HWMON DRIVER +M: Eddie James <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/hwmon/occ +F: drivers/hwmon/occ/ + ONENAND FLASH DRIVER M: Kyungmin Park <[email protected]> L: [email protected]
drivers/hwmon/Kconfig +2 -0
@@ -1240,6 +1240,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
@@ -169,6 +169,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 +448 -0
448 lines not on this page · raw message
drivers/hwmon/occ/occ.h new file +81 -0
81 lines not on this page · raw message
drivers/hwmon/occ/scom.h new file +47 -0
47 lines not on this page · raw message
Signature · 1 line
1.8.3.1
Re: [PATCH linux v7 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs
Documentation/hwmon/occ
Quoted 5 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
Documentation/hwmon/occ new file +0
-0
The kernel is using reStructuredText these days. You should consider following suit:
https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html#writing-documentation
Documentation/hwmon/occ line 6
Quoted 7 lines
Documentation/hwmon/occ new file +6
-0@@ -0,0 +1,40 @@ +Kernel driver occ +================= + +Supported chips: + * ASPEED AST2400 + * ASPEED AST2500
Not really - this will run on any chip that's connected to a P8 or P9.
MAINTAINERS T: git git://linuxtv.org/media_tree.git · line 9131
Quoted 9 lines
MAINTAINERS +1
-0@@ -9128,6 +9128,13 @@ T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov7670.c +ON-CHIP CONTROLLER HWMON DRIVER
Mention P8 and P9?
MAINTAINERS T: git git://linuxtv.org/media_tree.git · line 9133
Quoted 2 lines
MAINTAINERS +2
-0+M: Eddie James <[email protected]> +L: [email protected]
Have you subscribed to this list? Would you prefer the mail to come to the openbmc list?
Quoted 45 lines
MAINTAINERS +4
-0+S: Maintained +F: Documentation/hwmon/occ +F: drivers/hwmon/occ/ + ONENAND FLASH DRIVER M: Kyungmin Park <[email protected]> L: [email protected]drivers/hwmon/Kconfig +2
-0@@ -1240,6 +1240,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 I2Cdrivers/hwmon/Makefile +1
-0@@ -169,6 +169,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) := -DDEBUGdrivers/hwmon/occ/occ.c new file +5
-0+ sensors = &resp->data.blocks[b].sensors; +if (!sensors) { +/* first poll response */ + sensors = driver->ops.alloc_sensor (dev, sensor_type, + block->num_sensors);
drivers/hwmon/occ/occ.c line 198
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+if (!sensors) +return -ENOMEM ; + + resp->data.blocks[b].sensors = sensors; + resp->data.sensor_block_id[sensor_type] = b; + resp->data.blocks[b].header = *block; + }else if (block->sensor_length != + resp->data.blocks[b].header.sensor_length) { +dev_warn (dev, +"different sensor length than first poll\n" );
The driver has changed behaviour from your previous version; now it discards data if the sensors change.
Under what circumstances does the sensor data change?
Quoted 30 lines
drivers/hwmon/occ/occ.c new file +30
-0+continue ; + } + +for (s =0 ; s < block->num_sensors && + s < resp->data.blocks[b].header.num_sensors; s++) { +if (offset + block->sensor_length > num_bytes) { +dev_warn (dev,"exceeded data length\n" ); +return 0 ; + } + + driver->ops.parse_sensor (data, sensors, sensor_type, + offset, s); + offset += block->sensor_length; + } + } + +return 0 ; +} + +static u8 occ_send_cmd (struct occ *driver,u8 seq,u8 type,u16 length, +const u8 *data,u8 *resp) +{ +u32 cmd1, cmd2 =0 ; +u16 checksum =0 ; +bool retry = false; +int i, rc, tries =0 ; + + cmd1 = (seq <<24 ) | (type <<16 ) | length; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 );
drivers/hwmon/occ/occ.c line 238
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+ +/* checksum: sum of every bytes of cmd1, cmd2 */ +for (i =0 ; i <4 ; i++) { + checksum += (cmd1 >> (i *8 )) &0xFF ; + checksum += (cmd2 >> (i *8 )) &0xFF ; + } + + cmd2 |= checksum << ((2 - length) *8 ); + +/* Init OCB */
You should mention what OCB means in your documentation.
Quoted 19 lines
drivers/hwmon/occ/occ.c new file +17
-0+ rc = driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_OR , +OCB_OR_INIT0 ,OCB_OR_INIT1 ); +if (rc) + goto err; + +int occ_set_user_powercap (struct occ *occ,u16 cap) +{ +u8 resp[8 ]; + + cap =cpu_to_be16 (cap); + +return occ_send_cmd (occ,0 ,OCC_SET_USER_POWR_CAP ,2 , (const u8 *)&cap, + resp); +} +EXPORT_SYMBOL (occ_set_user_powercap); + +struct occ *occ_start(struct device *dev, void *bus,
Quoted 1 line
From what I can tell this doesn't start anything. Call it
occ_init()
or something.
drivers/hwmon/occ/occ.c line 424
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)
Create a structure with all of these details in it. Some of them don't need to be broken out into their own, for instance:
struct occ *occ_start(struct device *dev, const struct occ_init_context *init) {
driver->dev = dev; driver->bus = init->bus; driver->bus_read = init->bus_read; driver->bus_write = init->bus_write; driver->config = init->config; driver->cmd_addr = init->cmd_addr;
etc.
Quoted 31 lines
drivers/hwmon/occ/occ.c new file +24
-0+{ +struct occ *driver =devm_kzalloc (dev,sizeof (struct occ ),GFP_KERNEL ); + +if (!driver) +return ERR_PTR (-ENOMEM ); + + driver->dev = dev; + driver->bus = bus; + driver->bus_ops = *bus_ops; + driver->ops = *ops; + driver->config = *config; + driver->raw_data =devm_kzalloc (dev,OCC_DATA_MAX ,GFP_KERNEL ); +if (!driver->raw_data) +return ERR_PTR (-ENOMEM ); + +mutex_init (&driver->update_lock); + +return driver; +} +EXPORT_SYMBOL (occ_start); + +MODULE_AUTHOR ("Eddie James <[email protected]>" ); +MODULE_DESCRIPTION ("OCC hwmon core driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/scom.h new file +0
-0
drivers/hwmon/occ/scom.h line 4
Quoted 4 lines
drivers/hwmon/occ/scom.h new file +4
-0@@ -0,0 +1,47 @@ +/* +* scom.h - hwmon OCC driver +* +* This file contains data structures for scom operations to the OCC
Are these really SCOM operations?
I think they're better described read and write callbacks, as the operation is may be talking i2c or FSI or in the future some other kind of access.
They do take scom addresses as parameters, so I can see the argument for calling them getscom/putscom.
Quoted 16 lines
drivers/hwmon/occ/scom.h new file +16
-0+* +* 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__
drivers/hwmon/occ/scom.h line 30
Quoted 10 lines
drivers/hwmon/occ/scom.h new file +10
-0+ +/* +* occ_bus_ops - represent the low-level transfer methods to communicate with +* 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 at least +* eight bytes.
Are there situations where it's more than 8 bytes?
Would it be safer to add a length argument so the read call so we don't put more data in the buffer than the caller expects?
Quoted 17 lines
drivers/hwmon/occ/scom.h new file +17
-0+* +* Returns 0 on success or a negative errno on error +* +* putscom - OCC scom write +* @bus: handle to slave device +* @address: address +* @data0: first data word to write +* @data1: second data word to write +* +* Returns 0 on success or a negative errno on error +*/ +struct occ_bus_ops { +int (*getscom)(void *bus,u32 address,u64 *data); +int (*putscom)(void *bus,u32 address,u32 data0,u32 data1); +}; + +#endif /* __SCOM_H__ */
Re: [PATCH linux v7 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs
Quoted 95 lines · Joel Stanley
Documentation/hwmon/occ
Quoted 5 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
Documentation/hwmon/occ new file +0
-0The kernel is using reStructuredText these days. You should consider following suit:
https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html#writing-documentation
Documentation/hwmon/occ line 6
Quoted 7 lines
Documentation/hwmon/occ new file +6
-0@@ -0,0 +1,40 @@ +Kernel driver occ +================= + +Supported chips: + * ASPEED AST2400 + * ASPEED AST2500Not really - this will run on any chip that's connected to a P8 or P9.
MAINTAINERS T: git git://linuxtv.org/media_tree.git · line 9131
Quoted 9 lines
MAINTAINERS +1
-0@@ -9128,6 +9128,13 @@ T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov7670.c +ON-CHIP CONTROLLER HWMON DRIVERMention P8 and P9?
MAINTAINERS T: git git://linuxtv.org/media_tree.git · line 9133
Quoted 2 lines
MAINTAINERS +2
-0+M: Eddie James <[email protected]> +L: [email protected]Have you subscribed to this list? Would you prefer the mail to come to the openbmc list?
Quoted 43 lines
MAINTAINERS +4
-0+S: Maintained +F: Documentation/hwmon/occ +F: drivers/hwmon/occ/ + ONENAND FLASH DRIVER M: Kyungmin Park <[email protected]> L: [email protected]drivers/hwmon/Kconfig +2
-0@@ -1240,6 +1240,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 I2Cdrivers/hwmon/Makefile +1
-0@@ -169,6 +169,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) := -DDEBUGdrivers/hwmon/occ/occ.c new file +5
-0+ sensors = &resp->data.blocks[b].sensors; +if (!sensors) { +/* first poll response */ + sensors = driver->ops.alloc_sensor (dev, sensor_type, + block->num_sensors);drivers/hwmon/occ/occ.c line 198
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+if (!sensors) +return -ENOMEM ; + + resp->data.blocks[b].sensors = sensors; + resp->data.sensor_block_id[sensor_type] = b; + resp->data.blocks[b].header = *block; + }else if (block->sensor_length != + resp->data.blocks[b].header.sensor_length) { +dev_warn (dev, +"different sensor length than first poll\n" );The driver has changed behaviour from your previous version; now it discards data if the sensors change.
Quoted 1 line
Under what circumstances does the sensor data change?
Yes. The sensor data shouldn't change, as far as I know. I think something would be wrong if it did.
Quoted 118 lines
Quoted 31 lines
drivers/hwmon/occ/occ.c new file +30
-0+continue ; + } + +for (s =0 ; s < block->num_sensors && + s < resp->data.blocks[b].header.num_sensors; s++) { +if (offset + block->sensor_length > num_bytes) { +dev_warn (dev,"exceeded data length\n" ); +return 0 ; + } + + driver->ops.parse_sensor (data, sensors, sensor_type, + offset, s); + offset += block->sensor_length; + } + } + +return 0 ; +} + +static u8 occ_send_cmd (struct occ *driver,u8 seq,u8 type,u16 length, +const u8 *data,u8 *resp) +{ +u32 cmd1, cmd2 =0 ; +u16 checksum =0 ; +bool retry = false; +int i, rc, tries =0 ; + + cmd1 = (seq <<24 ) | (type <<16 ) | length; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 );drivers/hwmon/occ/occ.c line 238
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+ +/* checksum: sum of every bytes of cmd1, cmd2 */ +for (i =0 ; i <4 ; i++) { + checksum += (cmd1 >> (i *8 )) &0xFF ; + checksum += (cmd2 >> (i *8 )) &0xFF ; + } + + cmd2 |= checksum << ((2 - length) *8 ); + +/* Init OCB */ You should mention what OCB means in your documentation.
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +7
-0+ rc = driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_OR , +OCB_OR_INIT0 ,OCB_OR_INIT1 ); +if (rc) + goto err; + +int occ_set_user_powercap (struct occ *occ,u16 cap) +{drivers/hwmon/occ/occ.c line 422
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+u8 resp[8 ]; + + cap =cpu_to_be16 (cap); + +return occ_send_cmd (occ,0 ,OCC_SET_USER_POWR_CAP ,2 , (const u8 *)&cap, + resp); +} +EXPORT_SYMBOL (occ_set_user_powercap); + +struct occ *occ_start(struct device *dev, void *bus,From what I can tell this doesn't start anything. Call it
occ_init()
or something.drivers/hwmon/occ/occ.c line 424
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)Create a structure with all of these details in it. Some of them don't need to be broken out into their own, for instance:
struct occ *occ_start(struct device *dev, const struct
occ_init_context*init) {driver->dev = dev; driver->bus = init->bus; driver->bus_read = init->bus_read; driver->bus_write = init->bus_write; driver->config = init->config; driver->cmd_addr = init->cmd_addr;etc.
Quoted 30 lines
drivers/hwmon/occ/occ.c new file +24
-0+{ +struct occ *driver =devm_kzalloc (dev,sizeof (struct occ ),GFP_KERNEL ); + +if (!driver) +return ERR_PTR (-ENOMEM ); + + driver->dev = dev; + driver->bus = bus; + driver->bus_ops = *bus_ops; + driver->ops = *ops; + driver->config = *config; + driver->raw_data =devm_kzalloc (dev,OCC_DATA_MAX ,GFP_KERNEL ); +if (!driver->raw_data) +return ERR_PTR (-ENOMEM ); + +mutex_init (&driver->update_lock); + +return driver; +} +EXPORT_SYMBOL (occ_start); + +MODULE_AUTHOR ("Eddie James <[email protected]>" ); +MODULE_DESCRIPTION ("OCC hwmon core driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/scom.h new file +0
-0drivers/hwmon/occ/scom.h line 4
Quoted 4 lines
drivers/hwmon/occ/scom.h new file +4
-0@@ -0,0 +1,47 @@ +/* +* scom.h - hwmon OCC driver +* +* This file contains data structures for scom operations to the OCC Are these really SCOM operations?
Quoted 6 lines
I think they're better described read and write callbacks, as the operation is may be talking i2c or FSI or in the future some other kind of access.
They do take scom addresses as parameters, so I can see the argument for calling them getscom/putscom.
These are scom operations. The only way to talk to the OCC is with a scom operation as I understand it. The transport mechanism can change (i2c or fsi/sbe), but it's still a "scom."
Quoted 30 lines
Quoted 18 lines
drivers/hwmon/occ/scom.h new file +16
-0+* +* 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__drivers/hwmon/occ/scom.h line 30
Quoted 10 lines
drivers/hwmon/occ/scom.h new file +10
-0+ +/* +* occ_bus_ops - represent the low-level transfer methods to communicate with +* 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 at least +* eight bytes. Are there situations where it's more than 8 bytes?
Quoted 2 lines
Would it be safer to add a length argument so the read call so we don't put more data in the buffer than the caller expects?
I guess the documentation isn't that clear, but it's always an 8 byte operation. I said "at least" 8 bytes because the calling code calls this function repeatedly with one large buffer at different offsets. I'll add a comment that it's always 8 bytes.
Quoted 19 lines
Quoted 19 lines
drivers/hwmon/occ/scom.h new file +17
-0+* +* Returns 0 on success or a negative errno on error +* +* putscom - OCC scom write +* @bus: handle to slave device +* @address: address +* @data0: first data word to write +* @data1: second data word to write +* +* Returns 0 on success or a negative errno on error +*/ +struct occ_bus_ops { +int (*getscom)(void *bus,u32 address,u64 *data); +int (*putscom)(void *bus,u32 address,u32 data0,u32 data1); +}; + +#endif /* __SCOM_H__ */
Re: [PATCH linux v7 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs
In reply to Joel StanleyQuoted 167 lines · Joel Stanley
Documentation/hwmon/occ
Quoted 5 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
Documentation/hwmon/occ new file +0
-0The kernel is using reStructuredText these days. You should consider following suit:
https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html#writing-documentation
Documentation/hwmon/occ line 6
Quoted 7 lines
Documentation/hwmon/occ new file +6
-0@@ -0,0 +1,40 @@ +Kernel driver occ +================= + +Supported chips: + * ASPEED AST2400 + * ASPEED AST2500Not really - this will run on any chip that's connected to a P8 or P9.
MAINTAINERS T: git git://linuxtv.org/media_tree.git · line 9131
Quoted 9 lines
MAINTAINERS +1
-0@@ -9128,6 +9128,13 @@ T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov7670.c +ON-CHIP CONTROLLER HWMON DRIVERMention P8 and P9?
MAINTAINERS T: git git://linuxtv.org/media_tree.git · line 9133
Quoted 2 lines
MAINTAINERS +2
-0+M: Eddie James <[email protected]> +L: [email protected]Have you subscribed to this list? Would you prefer the mail to come to the openbmc list?
Quoted 43 lines
MAINTAINERS +4
-0+S: Maintained +F: Documentation/hwmon/occ +F: drivers/hwmon/occ/ + ONENAND FLASH DRIVER M: Kyungmin Park <[email protected]> L: [email protected]drivers/hwmon/Kconfig +2
-0@@ -1240,6 +1240,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 I2Cdrivers/hwmon/Makefile +1
-0@@ -169,6 +169,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) := -DDEBUGdrivers/hwmon/occ/occ.c new file +5
-0+ sensors = &resp->data.blocks[b].sensors; +if (!sensors) { +/* first poll response */ + sensors = driver->ops.alloc_sensor (dev, sensor_type, + block->num_sensors);drivers/hwmon/occ/occ.c line 198
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+if (!sensors) +return -ENOMEM ; + + resp->data.blocks[b].sensors = sensors; + resp->data.sensor_block_id[sensor_type] = b; + resp->data.blocks[b].header = *block; + }else if (block->sensor_length != + resp->data.blocks[b].header.sensor_length) { +dev_warn (dev, +"different sensor length than first poll\n" );The driver has changed behaviour from your previous version; now it discards data if the sensors change.
Under what circumstances does the sensor data change?
Quoted 30 lines
drivers/hwmon/occ/occ.c new file +30
-0+continue ; + } + +for (s =0 ; s < block->num_sensors && + s < resp->data.blocks[b].header.num_sensors; s++) { +if (offset + block->sensor_length > num_bytes) { +dev_warn (dev,"exceeded data length\n" ); +return 0 ; + } + + driver->ops.parse_sensor (data, sensors, sensor_type, + offset, s); + offset += block->sensor_length; + } + } + +return 0 ; +} + +static u8 occ_send_cmd (struct occ *driver,u8 seq,u8 type,u16 length, +const u8 *data,u8 *resp) +{ +u32 cmd1, cmd2 =0 ; +u16 checksum =0 ; +bool retry = false; +int i, rc, tries =0 ; + + cmd1 = (seq <<24 ) | (type <<16 ) | length; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 );drivers/hwmon/occ/occ.c line 238
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+ +/* checksum: sum of every bytes of cmd1, cmd2 */ +for (i =0 ; i <4 ; i++) { + checksum += (cmd1 >> (i *8 )) &0xFF ; + checksum += (cmd2 >> (i *8 )) &0xFF ; + } + + cmd2 |= checksum << ((2 - length) *8 ); + +/* Init OCB */ You should mention what OCB means in your documentation.
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +7
-0+ rc = driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_OR , +OCB_OR_INIT0 ,OCB_OR_INIT1 ); +if (rc) + goto err; + +int occ_set_user_powercap (struct occ *occ,u16 cap) +{drivers/hwmon/occ/occ.c line 422
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+u8 resp[8 ]; + + cap =cpu_to_be16 (cap); + +return occ_send_cmd (occ,0 ,OCC_SET_USER_POWR_CAP ,2 , (const u8 *)&cap, + resp); +} +EXPORT_SYMBOL (occ_set_user_powercap); + +struct occ *occ_start(struct device *dev, void *bus,From what I can tell this doesn't start anything. Call it
occ_init()
or something.drivers/hwmon/occ/occ.c line 424
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)Create a structure with all of these details in it. Some of them don't need to be broken out into their own, for instance:
struct occ *occ_start(struct device *dev, const struct
occ_init_context*init) {
Quoted 8 lines
driver->dev = dev; driver->bus = init->bus; driver->bus_read = init->bus_read; driver->bus_write = init->bus_write; driver->config = init->config; driver->cmd_addr = init->cmd_addr;etc.
Tried this but it doesn't make that much sense, as half the args come from occ_p8.c and the other half from p8_occ_i2c.c. Can't make the structure const then, and it seems strange to init the structure in two places...
Quoted 96 lines
Quoted 33 lines
drivers/hwmon/occ/occ.c new file +24
-0+{ +struct occ *driver =devm_kzalloc (dev,sizeof (struct occ ),GFP_KERNEL ); + +if (!driver) +return ERR_PTR (-ENOMEM ); + + driver->dev = dev; + driver->bus = bus; + driver->bus_ops = *bus_ops; + driver->ops = *ops; + driver->config = *config; + driver->raw_data =devm_kzalloc (dev,OCC_DATA_MAX ,GFP_KERNEL ); +if (!driver->raw_data) +return ERR_PTR (-ENOMEM ); + +mutex_init (&driver->update_lock); + +return driver; +} +EXPORT_SYMBOL (occ_start); + +MODULE_AUTHOR ("Eddie James <[email protected]>" ); +MODULE_DESCRIPTION ("OCC hwmon core driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/scom.h new file +0
-0drivers/hwmon/occ/scom.h line 4
Quoted 4 lines
drivers/hwmon/occ/scom.h new file +4
-0@@ -0,0 +1,47 @@ +/* +* scom.h - hwmon OCC driver +* +* This file contains data structures for scom operations to the OCC Are these really SCOM operations?
I think they're better described read and write callbacks, as the operation is may be talking i2c or FSI or in the future some other kind of access.
They do take scom addresses as parameters, so I can see the argument for calling them getscom/putscom.
Quoted 16 lines
drivers/hwmon/occ/scom.h new file +16
-0+* +* 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__drivers/hwmon/occ/scom.h line 30
Quoted 10 lines
drivers/hwmon/occ/scom.h new file +10
-0+ +/* +* occ_bus_ops - represent the low-level transfer methods to communicate with +* 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 at least +* eight bytes. Are there situations where it's more than 8 bytes?
Would it be safer to add a length argument so the read call so we don't put more data in the buffer than the caller expects?
Quoted 17 lines
drivers/hwmon/occ/scom.h new file +17
-0+* +* Returns 0 on success or a negative errno on error +* +* putscom - OCC scom write +* @bus: handle to slave device +* @address: address +* @data0: first data word to write +* @data1: second data word to write +* +* Returns 0 on success or a negative errno on error +*/ +struct occ_bus_ops { +int (*getscom)(void *bus,u32 address,u64 *data); +int (*putscom)(void *bus,u32 address,u32 data0,u32 data1); +}; + +#endif /* __SCOM_H__ */
PATCH v7 2/6 hwmon: occ: Add sysfs interface
In reply to [email protected]From: "Edward A. James" <[email protected]>
Add a generic mechanism to expose the sensors provided by the OCC in sysfs.
--- Documentation/hwmon/occ | 62 +++++++++++ drivers/hwmon/occ/Makefile | 2 +- drivers/hwmon/occ/occ_sysfs.c | 251 ++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_sysfs.h | 30 +++++ 4 files changed, 344 insertions(+), 1 deletion(-) create mode 100644 drivers/hwmon/occ/occ_sysfs.c create mode 100644 drivers/hwmon/occ/occ_sysfs.h
Documentation/hwmon/occ +62 -0
@@ -25,6 +25,68 @@ 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. +sysfs Entries +------------- + +The OCC driver uses the hwmon sysfs framework to provide data to userspace. + +The driver exports a number of sysfs files for each type of sensor. The +sensor-specific files vary depending on the processor type, though many of the +attributes are common for both the POWER8 and POWER9. + +The hwmon interface cannot define every type of sensor that may be used. +Therefore, the frequency sensor on the OCC uses the "input" type sensor defined +by the hwmon interface, rather than defining a new type of custom sensor. + +Below are detailed the names and meaning of each sensor file for both types of +processors. All sensors are read-only unless otherwise specified. <x> indicates +the hwmon index. sensor id indicates the unique internal OCC identifer. Please +see the POWER OCC specification for details on all these sensor values. + +frequency: + all processors: + in<x>_input - frequency value + in<x>_label - sensor id +temperature: + POWER8: + temp<x>_input - temperature value + temp<x>_label - sensor id + POWER9 (in addition to above): + temp<x>_type - FRU type + +power: + POWER8: + power<x>_input - power value + power<x>_label - sensor id + power<x>_average - accumulator + power<x>_average_interval - update tag (number of samples in + accumulator) + POWER9: + power<x>_input - power value + power<x>_label - sensor id + power<x>_average_min - accumulator[0] + power<x>_average_max - accumulator[1] (64 bits total) + power<x>_average_interval - update tag + power<x>_reset_history - (function_id | (apss_channel << 8) + +caps: + POWER8: + power<x>_cap - current powercap + power<x>_cap_max - max powercap + power<x>_cap_min - min powercap + power<x>_max - normal powercap + power<x>_alarm - user powercap, r/w + POWER9: + power<x>_cap_alarm - user powercap source + +The driver also provides two sysfs entries through hwmon to better +control the driver and monitor the master OCC. Though there may be multiple +OCCs present on the system, these two files are only present for the "master" +OCC. + name - read the name of the driver + update_interval - read or write the minimum interval for polling the + OCC. + BMC - Host Communications -------------------------
drivers/hwmon/occ/Makefile +1 -1
@@ -1 +1 @@ -obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
drivers/hwmon/occ/occ_sysfs.c new file +251 -0
251 lines not on this page · raw message
drivers/hwmon/occ/occ_sysfs.h new file +30 -0
@@ -0,0 +1,30 @@ +/* +* occ_sysfs.h - OCC sysfs interface +* +* This file contains the data structures and function prototypes for the OCC +* hwmon sysfs entries. +* +* 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_SYSFS_H__ +#define __OCC_SYSFS_H__ + +struct occ ; +struct device ; +struct occ_sysfs ; + +struct occ_sysfs *occ_sysfs_start (struct device *dev,struct occ *occ, +const u32 *sensor_hwmon_configs, +const char *name); +#endif /* __OCC_SYSFS_H__ */
Signature · 1 line
1.8.3.1
Re: [PATCH linux v7 2/6] hwmon: occ: Add sysfs interface
Quoted 216 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
From: "Edward A. James" <[email protected]>
Add a generic mechanism to expose the sensors provided by the OCC in sysfs.
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>--- Documentation/hwmon/occ | 62 +++++++++++ drivers/hwmon/occ/Makefile | 2 +- drivers/hwmon/occ/occ_sysfs.c | 251 ++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_sysfs.h | 30 +++++ 4 files changed, 344 insertions(+), 1 deletion(-) create mode 100644 drivers/hwmon/occ/occ_sysfs.c create mode 100644 drivers/hwmon/occ/occ_sysfs.hDocumentation/hwmon/occ +62
-0@@ -25,6 +25,68 @@ 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. +sysfs Entries +------------- + +The OCC driver uses the hwmon sysfs framework to provide data to userspace. + +The driver exports a number of sysfs files for each type of sensor. The +sensor-specific files vary depending on the processor type, though many of the +attributes are common for both the POWER8 and POWER9. + +The hwmon interface cannot define every type of sensor that may be used. +Therefore, the frequency sensor on the OCC uses the "input" type sensor defined +by the hwmon interface, rather than defining a new type of custom sensor. + +Below are detailed the names and meaning of each sensor file for both types of +processors. All sensors are read-only unless otherwise specified. <x> indicates +the hwmon index. sensor id indicates the unique internal OCC identifer. Please +see the POWER OCC specification for details on all these sensor values. + +frequency: + all processors: + in<x>_input - frequency value + in<x>_label - sensor id +temperature: + POWER8: + temp<x>_input - temperature value + temp<x>_label - sensor id + POWER9 (in addition to above): + temp<x>_type - FRU type + +power: + POWER8: + power<x>_input - power value + power<x>_label - sensor id + power<x>_average - accumulator + power<x>_average_interval - update tag (number of samples in + accumulator) + POWER9: + power<x>_input - power value + power<x>_label - sensor id + power<x>_average_min - accumulator[0] + power<x>_average_max - accumulator[1] (64 bits total) + power<x>_average_interval - update tag + power<x>_reset_history - (function_id | (apss_channel << 8) + +caps: + POWER8: + power<x>_cap - current powercap + power<x>_cap_max - max powercap + power<x>_cap_min - min powercap + power<x>_max - normal powercap + power<x>_alarm - user powercap, r/w + POWER9: + power<x>_cap_alarm - user powercap source + +The driver also provides two sysfs entries through hwmon to better +control the driver and monitor the master OCC. Though there may be multiple +OCCs present on the system, these two files are only present for the "master" +OCC. + name - read the name of the driver + update_interval - read or write the minimum interval for polling the + OCC. + BMC - Host Communications -------------------------drivers/hwmon/occ/Makefile +1
-1@@ -1 +1 @@ -obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.odrivers/hwmon/occ/occ_sysfs.c new file +114
-0@@ -0,0 +1,251 @@ +/* +* occ_sysfs.c - OCC sysfs interface +* +* This file contains the methods and data structures for implementing the OCC +* hwmon sysfs entries. +* +* 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/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/init.h> +#include <linux/jiffies.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include "occ.h" +#include "occ_sysfs.h" + +#define OCC_HWMON_NAME_LENGTH 32 + +struct occ_sysfs { +struct device *dev; +struct occ *occ; + +char hwmon_name[OCC_HWMON_NAME_LENGTH +1 ]; +const u32 *sensor_hwmon_configs; +struct hwmon_channel_info **occ_sensors; +struct hwmon_chip_info occ_info; +u16 user_powercap; +}; + +static int occ_hwmon_read (struct device *dev,enum hwmon_sensor_types type, +u32 attr,int channel,long *val) +{ +int rc; +struct occ_sysfs *driver =dev_get_drvdata (dev); +struct occ *occ = driver->occ; + +switch (type) { +case hwmon_in: + rc =occ_get_sensor_field (occ,FREQ , channel, attr, val); +break ; +case hwmon_temp: + rc =occ_get_sensor_field (occ,TEMP , channel, attr, val); +break ; +case hwmon_power: + rc =occ_get_sensor_field (occ,POWER , channel, attr, val); +break ; +default : + rc = -EOPNOTSUPP ; + } + +return rc; +} + +static int occ_hwmon_read_string (struct device *dev, +enum hwmon_sensor_types type,u32 attr, +int channel,char **str) +{ +int rc; +unsigned long val =0 ; + +if (!((type == hwmon_in && attr == hwmon_in_label) || + (type == hwmon_temp && attr == hwmon_temp_label) || + (type == hwmon_power && attr == hwmon_power_label))) +return -EOPNOTSUPP ; + +/* will fetch the "label", the sensor_id */ + rc =occ_hwmon_read (dev, type, attr, channel, &val); +if (rc <0 ) +return rc; + + rc =snprintf (*str,PAGE_SIZE -1 ,"%lu" , val); +if (rc >0 ) + rc =0 ; + +return rc; +} + +static int occ_hwmon_write (struct device *dev,enum hwmon_sensor_types type, +u32 attr,int channel,long val) +{ +int rc; +struct occ_sysfs *driver =dev_get_drvdata (dev); + +if (type == hwmon_power && attr == hwmon_power_alarm) { + rc =occ_set_user_powercap (driver->occ, val); +if (rc) { +dev_err (dev,"set user powercap failed:%d\n" , rc); +return rc; + } + + driver->user_powercap = val; + +return rc; + } + +return -EOPNOTSUPP ; +}
drivers/hwmon/occ/occ_sysfs.c line 124
Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10
-0+ +static umode_t occ_is_visible (const void *data,enum hwmon_sensor_types type, +u32 attr,int channel) +{ +const struct occ_sysfs *driver = data; + +switch (type) { +case hwmon_chip: +if (attr == hwmon_chip_update_interval) +return 00644 ;
Perhaps 0644?
Quoted 166 lines
drivers/hwmon/occ/occ_sysfs.c new file +127
-0+break ; +case hwmon_in: +if (BIT (attr) & driver->sensor_hwmon_configs[0 ]) +return 00444 ; +break ; +case hwmon_temp: +if (BIT (attr) & driver->sensor_hwmon_configs[1 ]) +return 00444 ; +break ; +case hwmon_power: +/* user power limit */ +if (attr == hwmon_power_alarm) +return 00644 ; +else if ((BIT (attr) & driver->sensor_hwmon_configs[2 ]) || + (BIT (attr) & driver->sensor_hwmon_configs[3 ])) +return 00444 ; +break ; +default : +return 0 ; + } + +return 0 ; +} + +static const struct hwmon_ops occ_hwmon_ops = { + .is_visible = occ_is_visible, + .read = occ_hwmon_read, + .read_string = occ_hwmon_read_string, + .write = occ_hwmon_write, +}; + +static const enum hwmon_sensor_types occ_sensor_types[MAX_OCC_SENSOR_TYPE ] = { + hwmon_in, + hwmon_temp, + hwmon_power, + hwmon_power +}; + +struct occ_sysfs *occ_sysfs_start (struct device *dev,struct occ *occ, +const u32 *sensor_hwmon_configs, +const char *name) +{ +int rc, i, j, num_sensors, index =0 , id; +char *brk; +struct occ_blocks *resp =NULL ; +u32 *sensor_config; +struct occ_sysfs *hwmon =devm_kzalloc (dev,sizeof (struct occ_sysfs ), +GFP_KERNEL ); +if (!hwmon) +return ERR_PTR (-ENOMEM ); + +/* need space for null-termination */ + hwmon->occ_sensors = +devm_kzalloc (dev,sizeof (struct hwmon_channel_info *) * + (MAX_OCC_SENSOR_TYPE +1 ),GFP_KERNEL ); +if (!hwmon->occ_sensors) +return ERR_PTR (-ENOMEM ); + + hwmon->occ = occ; + hwmon->sensor_hwmon_configs = sensor_hwmon_configs; + hwmon->occ_info.ops = &occ_hwmon_ops; + hwmon->occ_info.info = + (const struct hwmon_channel_info **)hwmon->occ_sensors; + +occ_get_response_blocks (occ, &resp); + +for (i =0 ; i <MAX_OCC_SENSOR_TYPE ; ++i) + resp->sensor_block_id[i] =-1 ; + +/* read sensor data from occ */ + rc =occ_update_device (occ); +if (rc) +return ERR_PTR (rc); + +for (i =0 ; i <MAX_OCC_SENSOR_TYPE ; i++) { + id = resp->sensor_block_id[i]; +if (id <0 ) +continue ; + + num_sensors = resp->blocks[id].header.num_sensors; +/* need null-termination */ + sensor_config =devm_kzalloc (dev, +sizeof (u32) * (num_sensors +1 ), +GFP_KERNEL ); +if (!sensor_config) +return ERR_PTR (-ENOMEM ); + +for (j =0 ; j < num_sensors; j++) + sensor_config[j] = sensor_hwmon_configs[i]; + + hwmon->occ_sensors[index] = +devm_kzalloc (dev,sizeof (struct hwmon_channel_info ), +GFP_KERNEL ); +if (!hwmon->occ_sensors[index]) +return ERR_PTR (-ENOMEM ); + + hwmon->occ_sensors[index]->type = occ_sensor_types[i]; + hwmon->occ_sensors[index]->config = sensor_config; + index++; + } + +/* search for bad chars */ +strncpy (hwmon->hwmon_name, name,OCC_HWMON_NAME_LENGTH ); + brk =strpbrk (hwmon->hwmon_name,"-* \t\n" ); +while (brk) { + *brk ='_' ; + brk =strpbrk (brk,"-* \t\n" ); + } + + hwmon->dev =devm_hwmon_device_register_with_info (dev, + hwmon->hwmon_name, + hwmon, + &hwmon->occ_info, +NULL ); +if (IS_ERR (hwmon->dev)) { +dev_err (dev,"cannot register hwmon device %s: %ld\n" , + hwmon->hwmon_name,PTR_ERR (hwmon->dev)); +return ERR_CAST (hwmon->dev); + } + +return hwmon; +} +EXPORT_SYMBOL (occ_sysfs_start); + +MODULE_AUTHOR ("Eddie James <[email protected]>" ); +MODULE_DESCRIPTION ("OCC sysfs driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/occ_sysfs.h new file +30
-1@@ -0,0 +1,30 @@ +/* +* occ_sysfs.h - OCC sysfs interface +* +* This file contains the data structures and function prototypes for the OCC +* hwmon sysfs entries. +* +* 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_SYSFS_H__ +#define __OCC_SYSFS_H__ + +struct occ ; +struct device ; +struct occ_sysfs ; + +struct occ_sysfs *occ_sysfs_start (struct device *dev,struct occ *occ, +const u32 *sensor_hwmon_configs, +const char *name); +#endif /* __OCC_SYSFS_H__ */ --1.8.3.1
PATCH v7 3/6 hwmon: occ: Add I2C transport implementation for SCOM operations
In reply to [email protected]From: "Edward A. James" <eajames-r/[email protected]>
Add functions to send SCOM operations over I2C bus. The BMC can communicate with the Power8 host processor over I2C, but needs to use SCOM operations in order to access the OCC register space.
--- drivers/hwmon/occ/occ_scom_i2c.c | 77 ++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
drivers/hwmon/occ/occ_scom_i2c.c new file +77 -0
@@ -0,0 +1,77 @@ +/* +* occ_scom_i2c.c - hwmon OCC driver +* +* This file contains the functions for performing SCOM operations over I2C bus +* to access 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. +*/ + +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include "occ_scom_i2c.h" +#include "scom.h" + +int occ_i2c_getscom (void *bus,u32 address,u64 *data) +{ +ssize_t rc; +u64 buf; +struct i2c_client *client = bus; +struct i2c_msg msgs[2 ]; + + msgs[0 ].addr = client->addr; + msgs[0 ].flags = client->flags &I2C_M_TEN ; + msgs[0 ].len =sizeof (u32); + msgs[0 ].buf = (char *)&address; + + msgs[1 ].addr = client->addr; + msgs[1 ].flags = client->flags &I2C_M_TEN ; + msgs[1 ].flags |=I2C_M_RD ; + msgs[1 ].len =sizeof (u64); + msgs[1 ].buf = (char *)&buf; + + rc =i2c_transfer (client->adapter, msgs,2 ); +if (rc <0 ) +return rc; + + *data =be64_to_cpu (buf); + +return 0 ; +} +EXPORT_SYMBOL (occ_i2c_getscom); + +int occ_i2c_putscom (void *bus,u32 address,u32 data0,u32 data1) +{ +u32 buf[3 ]; +ssize_t rc; +struct i2c_client *client = bus; + +/* send raw data, user can handle endian */ + buf[0 ] = address; + buf[1 ] = data1; + buf[2 ] = data0; + + rc =i2c_master_send (client, (const char *)buf,sizeof (u32) *3 ); +if (rc <0 ) +return rc; +else if (rc !=sizeof (u32) *3 ) +return -EIO ; + +return 0 ; +} +EXPORT_SYMBOL (occ_i2c_putscom); + +MODULE_AUTHOR ("Eddie James <eajames-r/[email protected]>" ); +MODULE_DESCRIPTION ("I2C OCC SCOM transport" ); +MODULE_LICENSE ("GPL" );
drivers/hwmon/occ/occ_scom_i2c.h new file +26 -0
@@ -0,0 +1,26 @@ +/* +* occ_scom_i2c.h - hwmon OCC driver +* +* This file contains function protoypes for peforming SCOM operations over I2C +* bus to access 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 __OCC_SCOM_I2C_H__ +#define __OCC_SCOM_I2C_H__ + +int occ_i2c_getscom (void *bus,u32 address,u64 *data); +int occ_i2c_putscom (void *bus,u32 address,u32 data0,u32 data1); + +#endif /* __OCC_SCOM_I2C_H__ */
Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH linux v7 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations
Quoted 5 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
From: "Edward A. James" <[email protected]>
Add functions to send SCOM operations over I2C bus. The BMC can communicate with the Power8 host processor over I2C, but needs to use SCOM operations in order to access the OCC register space.
This doesn't need to be separate from the p8_occ_i2c.c file. You can remove a layer of function calls by merging this in and having these be your getscom putscom bus_ops callbacks.
Quoted 15 lines
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>--- drivers/hwmon/occ/occ_scom_i2c.c | 77 ++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c create mode 100644 drivers/hwmon/occ/occ_scom_i2c.hdrivers/hwmon/occ/occ_scom_i2c.c new file +0
-0
drivers/hwmon/occ/occ_scom_i2c.c line 29
Quoted 6 lines
drivers/hwmon/occ/occ_scom_i2c.c new file +5
-0@@ -0,0 +1,77 @@ + +int occ_i2c_getscom (void *bus,u32 address,u64 *data) +{ +ssize_t rc; +u64 buf;
If you add endianness annotations sparse can check your types are consistent. The warning looks like this:
make C=2 drivers/hwmon/occ/occ_scom_i2c.o drivers/hwmon/occ/occ_scom_i2c.c:48:17: warning: cast to restricted __be64
Which tells you it expects the type you pass to be64_to_cpu to be __be64.
drivers/hwmon/occ/occ_scom_i2c.c line 40
Quoted 11 lines
drivers/hwmon/occ/occ_scom_i2c.c new file +11
-0+struct i2c_client *client = bus; +struct i2c_msg msgs[2 ]; + + msgs[0 ].addr = client->addr; + msgs[0 ].flags = client->flags &I2C_M_TEN ; + msgs[0 ].len =sizeof (u32); + msgs[0 ].buf = (char *)&address; + + msgs[1 ].addr = client->addr; + msgs[1 ].flags = client->flags &I2C_M_TEN ; + msgs[1 ].flags |=I2C_M_RD ;
I first thought you had made a mistake here. Instead you could do:
msgs[1].flags = client->flags & I2C_M_TEN | I2C_M_RD;
Quoted 7 lines
drivers/hwmon/occ/occ_scom_i2c.c new file +7
-0+ msgs[1 ].len =sizeof (u64); + msgs[1 ].buf = (char *)&buf; + + rc =i2c_transfer (client->adapter, msgs,2 ); +if (rc <0 ) +return rc; +
Re: [PATCH linux v7 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations
Quoted 9 lines · Joel Stanley
Quoted 5 lines · On Wed, Feb 8, 2017 at 9:40 AM,<[email protected]> wrote:
From: "Edward A. James"<[email protected]>
Add functions to send SCOM operations over I2C bus. The BMC can communicate with the Power8 host processor over I2C, but needs to use SCOM operations in order to access the OCC register space.
This doesn't need to be separate from the
p8_occ_i2c.cfile. You can remove a layer of function calls by merging this in and having these be your getscom putscombus_opscallbacks.
The purpose of having this separate was so that we could do the scom address shift for p8 separately.
Quoted 42 lines
Quoted 15 lines
Signed-off-by Edward A. James<[email protected]>Signed-off-by Andrew Jeffery<[email protected]>--- drivers/hwmon/occ/occ_scom_i2c.c | 77 ++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++ 2 files changed, 103 insertions(+)create mode 100644
drivers/hwmon/occ/occ_scom_i2c.ccreate mode 100644drivers/hwmon/occ/occ_scom_i2c.hdrivers/hwmon/occ/occ_scom_i2c.c new file +0
-0drivers/hwmon/occ/occ_scom_i2c.c line 5
Quoted 5 lines
drivers/hwmon/occ/occ_scom_i2c.c new file +5
-0@@ -0,0 +1,77 @@ + +int occ_i2c_getscom (void *bus,u32 address,u64 *data) +{ +ssize_t rc; +u64 buf;If you add endianness annotations sparse can check your types are consistent. The warning looks like this:
make C=2
drivers/hwmon/occ/occ_scom_i2c.odrivers/hwmon/occ/occ_scom_i2c.c:48:17: warning: cast to restricted__be64Which tells you it expects the type you pass to
be64_to_cputo be__be64.drivers/hwmon/occ/occ_scom_i2c.c line 40
Quoted 11 lines
drivers/hwmon/occ/occ_scom_i2c.c new file +11
-0+struct i2c_client *client = bus; +struct i2c_msg msgs[2 ]; + + msgs[0 ].addr = client->addr; + msgs[0 ].flags = client->flags &I2C_M_TEN ; + msgs[0 ].len =sizeof (u32); + msgs[0 ].buf = (char *)&address; + + msgs[1 ].addr = client->addr; + msgs[1 ].flags = client->flags &I2C_M_TEN ; + msgs[1 ].flags |=I2C_M_RD ;I first thought you had made a mistake here. Instead you could do:
Quoted 1 line
msgs[1].flags = client->flags & I2C_M_TEN | I2C_M_RD;
Sure. Was just copying i2c_master_recv.
Quoted 7 lines
Quoted 7 lines
drivers/hwmon/occ/occ_scom_i2c.c new file +7
-0+ msgs[1 ].len =sizeof (u64); + msgs[1 ].buf = (char *)&buf; + + rc =i2c_transfer (client->adapter, msgs,2 ); +if (rc <0 ) +return rc; +
Re: [PATCH linux v7 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations
Quoted 7 lines · Eddie James
Quoted 6 lines · Joel Stanley
Quoted 6 lines · On Wed, Feb 8, 2017 at 9:40 AM,<[email protected]> wrote:
Quoted 1 line
From: "Edward A. James"<[email protected]>
Add functions to send SCOM operations over I2C bus. The BMC can communicate with the Power8 host processor over I2C, but needs to use SCOM operations in order to access the OCC register space.
Quoted 6 lines
Quoted 3 lines
This doesn't need to be separate from the
p8_occ_i2c.cfile. You can remove a layer of function calls by merging this in and having these be your getscom putscombus_opscallbacks.The purpose of having this separate was so that we could do the scom address shift for p8 separately.
Separation was my suggestion. It makes the I2C transport implementation independent of any P8 details, and having it separate* is no more abstract than the core performing SCOMs through the FSI interface when that's available. I feel like it's muddying the waters conceptually to bury P8 details in the implementation of a SCOM transport layer.
However, in our less abstract world the P8 will be the only system that uses the I2C transport, so while I don't think merging the functions is a good idea from an abstraction perspective it won't have a big impact.
Andrew
* Maybe the I2C SCOM transport even deserves to live outside drivers/hwmon/occ?
PATCH v7 4/6 hwmon: occ: Add callbacks for parsing P8 OCC datastructures
In reply to [email protected]From: "Edward A. James" <[email protected]>
Add functions to parse the data structures that are specific to the OCC on the POWER8 processor. These are the sensor data structures, including temperature, frequency, power, and "caps."
--- Documentation/hwmon/occ | 9 ++ drivers/hwmon/occ/occ_p8.c | 248 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_p8.h | 30 ++++++ 3 files changed, 287 insertions(+) create mode 100644 drivers/hwmon/occ/occ_p8.c create mode 100644 drivers/hwmon/occ/occ_p8.h
Documentation/hwmon/occ +9 -0
@@ -25,6 +25,15 @@ 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. +The format for the POWER8 OCC sensor data can be found in the P8 OCC +specification: +github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf +This document provides the details of the OCC sensors: power, frequency, +temperature, and caps. These sensor formats are specific to the POWER8 OCC. A +number of data structures, such as command format, response headers, and the +like, are also defined in this specification, and are common to both POWER8 and +POWER9 OCCs. + sysfs Entries -------------
drivers/hwmon/occ/occ_p8.c new file +248 -0
248 lines not on this page · raw message
drivers/hwmon/occ/occ_p8.h new file +30 -0
@@ -0,0 +1,30 @@ +/* +* occ_p8.h - OCC hwmon driver +* +* This file contains Power8-specific function prototypes +* +* 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_P8_H__ +#define __OCC_P8_H__ + +#include "scom.h" + +struct device ; + +const u32 *p8_get_sensor_hwmon_configs (void ); +struct occ *p8_occ_start (struct device *dev,void *bus, +struct occ_bus_ops *bus_ops); + +#endif /* __OCC_P8_H__ */
Signature · 1 line
1.8.3.1
Re: [PATCH linux v7 4/6] hwmon: occ: Add callbacks for parsing P8 OCC datastructures
Quoted 35 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
From: "Edward A. James" <[email protected]>
Add functions to parse the data structures that are specific to the OCC on the POWER8 processor. These are the sensor data structures, including temperature, frequency, power, and "caps."
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>--- Documentation/hwmon/occ | 9 ++ drivers/hwmon/occ/occ_p8.c | 248 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_p8.h | 30 ++++++ 3 files changed, 287 insertions(+) create mode 100644 drivers/hwmon/occ/occ_p8.c create mode 100644 drivers/hwmon/occ/occ_p8.hdrivers/hwmon/occ/occ_p8.c new file +12
-0+void p8_parse_sensor (u8 *data,void *sensor,int sensor_type,int off, +int snum) +{ +switch (sensor_type) { +case FREQ : +case TEMP : + { +struct p8_occ_sensor *os = + &(((struct p8_occ_sensor *)sensor)[snum]); + + os->sensor_id =be16_to_cpu (get_unaligned ((u16 *)&data[off])); + os->value =be16_to_cpu (get_unaligned ((u16 *)&data[off +2 ]));
drivers/hwmon/occ/occ_p8.c line 87
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +10
-0+ } +break ; +case POWER : + { +struct p8_power_sensor *ps = + &(((struct p8_power_sensor *)sensor)[snum]); + + ps->sensor_id =be16_to_cpu (get_unaligned ((u16 *)&data[off])); + ps->update_tag = +be32_to_cpu (get_unaligned ((u32 *)&data[off +2 ]));
This might be more readable if you wrote acast_get_unaliged_be32_to_cpu() macro.
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +9
-0+ ps->accumulator = +be32_to_cpu (get_unaligned ((u32 *)&data[off +6 ])); + ps->value =be16_to_cpu (get_unaligned ((u16 *)&data[off +10 ])); + } +break ; +case CAPS : + { +const u32 *p8_get_sensor_hwmon_configs () +{
drivers/hwmon/occ/occ_p8.c line 244
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +10
-0+return p8_sensor_hwmon_configs; +} +EXPORT_SYMBOL (p8_get_sensor_hwmon_configs); + +struct occ *p8_occ_start (struct device *dev,void *bus, +struct occ_bus_ops *bus_ops) +{ +return occ_start (dev, bus, bus_ops, &p8_ops, &p8_config); +} +EXPORT_SYMBOL (p8_occ_start);
We don't need to export these symbols; they're not used outside of the OCC module. The same goes for all of the exports you've made in this series.
I suggest we re-architect the drivers so we build all of the objects and link them into one module for each platform, instead of having an occ module and occ-p8/occ-p9 modules and i2c modules that all depend on each other. The Makefile could look like this:
obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += hwmon_occ_p8.o obj-$(CONFIG_SENSORS_PPC_OCC_P9) += hwmon_occ_p9.o
hwmon_occ_p8-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o occ_sysfs.o occ.o hwmon_occ_p9-$(CONFIG_SENSORS_PPC_OCC_P9) += occ_p9.o occ_sysfs.o occ.o
And the Kbuild like this:
menuconfig SENSORS_PPC_OCC
bool "PPC On-Chip Controller"
if SENSORS_PPC_OCC
config SENSORS_PPC_OCC_P8_I2C
bool "POWER8 OCC hwmon support"
depends on I2C
config SENSORS_PPC_OCC_P9
bool "POWER9 OCC hwmon support"
endif
Re: [PATCH linux v7 4/6] hwmon: occ: Add callbacks for parsing P8 OCC datastructures
Quoted 63 lines · Joel Stanley
Quoted 34 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
Quoted 1 line
Quoted 1 line
From: "Edward A. James" <[email protected]>
Add functions to parse the data structures that are specific to the OCC on the POWER8 processor. These are the sensor data structures, including temperature, frequency, power, and "caps."
Quoted 2 lines
Quoted 2 lines
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>---
Documentation/hwmon/occ | 9 ++
drivers/hwmon/occ/occ_p8.c | 248 +++++++++++++++++++++++++++++++++++++++++++++drivers/hwmon/occ/occ_p8.h| 30 ++++++3 files changed, 287 insertions(+)create mode 100644
drivers/hwmon/occ/occ_p8.c
create mode 100644drivers/hwmon/occ/occ_p8.hdrivers/hwmon/occ/occ_p8.c new file +12
-0+void p8_parse_sensor (u8 *data,void *sensor,int sensor_type,int off, + int snum) +{ + switch (sensor_type) { + case FREQ: + case TEMP: + { + struct p8_occ_sensor *os = + &(((struct p8_occ_sensor *)sensor)[snum]); + + os->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off])); + os->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));drivers/hwmon/occ/occ_p8.c line 87
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +10
-0+ } + break; + case POWER: + { + struct p8_power_sensor *ps = + &(((struct p8_power_sensor *)sensor)[snum]); + + ps->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off])); + ps->update_tag = + be32_to_cpu(get_unaligned((u32 *)&data[off + 2]));This might be more readable if you wrote a
cast_get_unaliged_be32_to_cpu()macro.Quoted 15 lines
+ ps->accumulator =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 6])); + ps->value =be16_to_cpu(get_unaligned((u16 *)&data[off + 10])); + }
+ break;
+ case CAPS:
+ {drivers/hwmon/occ/occ_p8.c new file +8
-0+const u32 *p8_get_sensor_hwmon_configs () +{ + return p8_sensor_hwmon_configs; +} +EXPORT_SYMBOL (p8_get_sensor_hwmon_configs); + +struct occ *p8_occ_start (struct device *dev,void *bus, + struct occ_bus_ops *bus_ops)
Quoted 8 lines
drivers/hwmon/occ/occ_p8.c line 244
Quoted 4 lines
drivers/hwmon/occ/occ_p8.c new file +4
-0+{ + return occ_start(dev, bus, bus_ops, &p8_ops, &p8_config); +} +EXPORT_SYMBOL (p8_occ_start);We don't need to export these symbols; they're not used outside of the OCC module. The same goes for all of the exports you've made in this series.
Sorry, this was my doing in an attempt to get everything to build as modules rather than just built-in. I should have studied Documentation/kbuild/modules.txt a bit more.
Quoted 20 lines
I suggest we re-architect the drivers so we build all of the objects and link them into one module for each platform, instead of having an occ module and occ-p8/occ-p9 modules and i2c modules that all depend on each other. The
Makefilecould look like this:obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += hwmon_occ_p8.o obj-$(CONFIG_SENSORS_PPC_OCC_P9) += hwmon_occ_p9.o
hwmon_occ_p8-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o occ_sysfs.o occ.o hwmon_occ_p9-$(CONFIG_SENSORS_PPC_OCC_P9) += occ_p9.o occ_sysfs.o occ.o
And the
Kbuildlike this:menuconfig
SENSORS_PPC_OCC
bool "PPC On-Chip Controller"if
SENSORS_PPC_OCC
Quoted 8 lines
config
SENSORS_PPC_OCC_P8_I2C
bool "POWER8 OCC hwmon support"
depends on I2Cconfig
SENSORS_PPC_OCC_P9
bool "POWER9 OCC hwmon support"endif
Given we can drop the exports that's a much more sensible idea.
Andrew
Guenter Roeck
maintainerRe: [PATCH linux v7 4/6] hwmon: occ: Add callbacks for parsing P8 OCC datastructures
Quoted 73 lines · Andrew Jeffery
Quoted 63 lines · Joel Stanley
Quoted 34 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
Quoted 1 line
Quoted 1 line
From: "Edward A. James" <[email protected]>
Add functions to parse the data structures that are specific to the OCC on the POWER8 processor. These are the sensor data structures, including temperature, frequency, power, and "caps."
Quoted 2 lines
Quoted 2 lines
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>---
Documentation/hwmon/occ | 9 ++
drivers/hwmon/occ/occ_p8.c | 248 +++++++++++++++++++++++++++++++++++++++++++++drivers/hwmon/occ/occ_p8.h| 30 ++++++3 files changed, 287 insertions(+)create mode 100644
drivers/hwmon/occ/occ_p8.c
create mode 100644drivers/hwmon/occ/occ_p8.hdrivers/hwmon/occ/occ_p8.c new file +12
-0+void p8_parse_sensor (u8 *data,void *sensor,int sensor_type,int off, + int snum) +{ + switch (sensor_type) { + case FREQ: + case TEMP: + { + struct p8_occ_sensor *os = + &(((struct p8_occ_sensor *)sensor)[snum]); + + os->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off])); + os->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));drivers/hwmon/occ/occ_p8.c line 87
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +10
-0+ } + break; + case POWER: + { + struct p8_power_sensor *ps = + &(((struct p8_power_sensor *)sensor)[snum]); + + ps->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off])); + ps->update_tag = + be32_to_cpu(get_unaligned((u32 *)&data[off + 2]));This might be more readable if you wrote a
cast_get_unaliged_be32_to_cpu()macro.Quoted 15 lines
+ ps->accumulator =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 6])); + ps->value =be16_to_cpu(get_unaligned((u16 *)&data[off + 10])); + }
+ break;
+ case CAPS:
+ {drivers/hwmon/occ/occ_p8.c new file +8
-0+const u32 *p8_get_sensor_hwmon_configs () +{ + return p8_sensor_hwmon_configs; +} +EXPORT_SYMBOL (p8_get_sensor_hwmon_configs); + +struct occ *p8_occ_start (struct device *dev,void *bus, + struct occ_bus_ops *bus_ops)Quoted 8 lines
drivers/hwmon/occ/occ_p8.c line 244
Quoted 4 lines
drivers/hwmon/occ/occ_p8.c new file +4
-0+{ + return occ_start(dev, bus, bus_ops, &p8_ops, &p8_config); +} +EXPORT_SYMBOL (p8_occ_start);We don't need to export these symbols; they're not used outside of the OCC module. The same goes for all of the exports you've made in this series.
Quoted 17 lines
Sorry, this was my doing in an attempt to get everything to build as modules rather than just built-in. I should have studied
Documentation/kbuild/modules.txta bit more.Quoted 13 lines
I suggest we re-architect the drivers so we build all of the objects and link them into one module for each platform, instead of having an occ module and occ-p8/occ-p9 modules and i2c modules that all depend on each other. The
Makefilecould look like this:obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += hwmon_occ_p8.o obj-$(CONFIG_SENSORS_PPC_OCC_P9) += hwmon_occ_p9.o
hwmon_occ_p8-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o occ_sysfs.o occ.o hwmon_occ_p9-$(CONFIG_SENSORS_PPC_OCC_P9) += occ_p9.o occ_sysfs.o occ.o
Please note that the above will still result in separate modules, one per object file. Is this really what you want ? I don't even know what happens if an object file is specified twice, so you may also have to make sure that CONFIG_SENSORS_PPC_OCC_P8_I2C and CONFIG_SENSORS_PPC_OCC_P9 are either-or configurations. Again, I am not really sure if that is what you want.
Quoted 7 lines
Quoted 7 lines
And the
Kbuildlike this:menuconfig
SENSORS_PPC_OCC
bool "PPC On-Chip Controller"if
SENSORS_PPC_OCC
Quoted 7 lines
Quoted 7 lines
config
SENSORS_PPC_OCC_P8_I2C
bool "POWER8 OCC hwmon support"
depends on I2Cconfig
SENSORS_PPC_OCC_P9
bool "POWER9 OCC hwmon support"
If both are bool, ie there won't be any module support, I don't really see the point of specifying occ_sysfs.o and occ.o twice above.
Quoted 4 lines
Quoted 1 line
endif
Given we can drop the exports that's a much more sensible idea.
Please make sure that both "allmodconfig" and "allyesconfig" still build after this change.
Thanks,
Guenter
PATCH v7 5/6 hwmon: occ: Add hwmon implementation for the P8 OCC
In reply to [email protected]From: "Edward A. James" <[email protected]>
Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as well as probe the entire driver from the I2C bus. I2C is the communication method between the BMC and the P8 OCC.
--- Documentation/devicetree/bindings/hwmon/occ.txt | 13 +++ drivers/hwmon/occ/Kconfig | 14 ++++ drivers/hwmon/occ/Makefile | 1 + drivers/hwmon/occ/p8_occ_i2c.c | 104 ++++++++++++++++++++++++ 4 files changed, 132 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
Documentation/devicetree/bindings/hwmon/occ.txt new file +13 -0
@@ -0,0 +1,13 @@ +HWMON I2C driver for IBM POWER CPU OCC (On Chip Controller) + +Required properties: + - compatible: must be "ibm,p8-occ-i2c" + - reg: physical address + +Example: +i2c3: i2c-bus@100 { + occ@50 { + compatible = "ibm,p8-occ-i2c"; + reg = <0x50>; + }; +};
drivers/hwmon/occ/Kconfig +14 -0
@@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC This driver can also be built as a module. If so, the module will be called occ. + +if SENSORS_PPC_OCC + +config SENSORS_PPC_OCC_P8_I2C + tristate "POWER8 OCC hwmon support" + depends on I2C + help + Provide a hwmon sysfs interface for the POWER8 On-Chip Controller, + exposing temperature, frequency and power measurements. + + This driver can also be built as a module. If so, the module will be + called p8-occ-i2c. + +endif
drivers/hwmon/occ/Makefile +1 -0
@@ -1 +1,2 @@ obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o +obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o
drivers/hwmon/occ/p8_occ_i2c.c new file +104 -0
104 lines not on this page · raw message
Signature · 1 line
1.8.3.1
Re: [PATCH linux v7 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
Quoted 6 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
From: "Edward A. James" <[email protected]>
Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as well as probe the entire driver from the I2C bus. I2C is the communication method between the BMC and the P8 OCC.
Quoted 8 lines
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>Acked-by Rob Herring <[email protected]>--- Documentation/devicetree/bindings/hwmon/occ.txt | 13 +++ drivers/hwmon/occ/Kconfig | 14 ++++ drivers/hwmon/occ/Makefile | 1 + drivers/hwmon/occ/p8_occ_i2c.c | 104 ++++++++++++++++++++++++
For consistency, how about we call this occ_p8_i2c.c? The other files have the machine name second.
Quoted 15 lines
4 files changed, 132 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt create mode 100644 drivers/hwmon/occ/p8_occ_i2c.cdrivers/hwmon/occ/Kconfig +2
-0@@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC This driver can also be built as a module. If so, the module will be called occ. + +if SENSORS_PPC_OCC
drivers/hwmon/occ/Kconfig menuconfig SENSORS_PPC_OCC · line 27
Quoted 10 lines
drivers/hwmon/occ/Kconfig +10
-0+ +config SENSORS_PPC_OCC_P8_I2C + tristate "POWER8 OCC hwmon support" + depends on I2C + help + Provide a hwmon sysfs interface for the POWER8 On-Chip Controller, + exposing temperature, frequency and power measurements. + + This driver can also be built as a module. If so, the module will be + called p8-occ-i2c.
Mention that this driver is for the BMC (or just "service processor"), and is not useful in the Power8 kernel.
I'm trying to think of a better prefix than PPC. PPC means much more than Power8 and Power9, which is what we mean. It's also confusing as we don't run this on any PowerPC machine - it's for an ARM chip.
drivers/hwmon/occ/Kconfig menuconfig SENSORS_PPC_OCC
Quoted 5 lines
drivers/hwmon/occ/Kconfig +5
-0+ +int p8_i2c_getscom(void *bus, u32 address, u64 *data) +{ + /* P8 i2c slave requires address to be shifted by 1 */ + address = address << 1;
I think these are scom addresses? Please indicate that so we don't get them confused with i2c.
Or are they scom operations?
drivers/hwmon/occ/Kconfig
Quoted 5 lines
drivers/hwmon/occ/Kconfig +5
-0+ + return occ_i2c_getscom(bus, address, data); +} + +int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
The functions can be static.
Quoted 6 lines
drivers/hwmon/occ/Kconfig +6
-0+{ + /* P8 i2c slave requires address to be shifted by 1 */ + address = address << 1; + + return occ_i2c_putscom(bus, address, data0, data1); +}
PATCH v7 6/6 hwmon: occ: Add callbacks for parsing P9 OCC datastructures
In reply to [email protected]From: "Edward A. James" <[email protected]>
Add functions to parse the data structures that are specific to the OCC on the POWER9 processor. These are the sensor data structures, including temperature, frequency, power, and "caps."
--- Documentation/hwmon/occ | 3 + drivers/hwmon/occ/occ_p9.c | 309 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_p9.h | 30 +++++ 3 files changed, 342 insertions(+) create mode 100644 drivers/hwmon/occ/occ_p9.c create mode 100644 drivers/hwmon/occ/occ_p9.h
Documentation/hwmon/occ +3 -0
@@ -34,6 +34,9 @@ number of data structures, such as command format, response headers, and the like, are also defined in this specification, and are common to both POWER8 and POWER9 OCCs. +There is currently no public P9 OCC specification, and the data structures +defined in the POWER9 OCC driver are subject to change. + sysfs Entries -------------
drivers/hwmon/occ/occ_p9.c new file +309 -0
309 lines not on this page · raw message
drivers/hwmon/occ/occ_p9.h new file +30 -0
@@ -0,0 +1,30 @@ +/* +* occ_p9.h - OCC hwmon driver +* +* This file contains Power9-specific function prototypes +* +* 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_P9_H__ +#define __OCC_P9_H__ + +#include "scom.h" + +struct device ; + +const u32 *p9_get_sensor_hwmon_configs (void ); +struct occ *p9_occ_start (struct device *dev,void *bus, +struct occ_bus_ops *bus_ops); + +#endif /* __OCC_P9_H__ */
Signature · 1 line
1.8.3.1
Re: [PATCH linux v7 6/6] hwmon: occ: Add callbacks for parsing P9 OCC datastructures
Quoted 22 lines · On Wed, Feb 8, 2017 at 9:40 AM, <[email protected]> wrote:
From: "Edward A. James" <[email protected]>
Add functions to parse the data structures that are specific to the OCC on the POWER9 processor. These are the sensor data structures, including temperature, frequency, power, and "caps."
Signed-off-by Edward A. James <[email protected]>Signed-off-by Andrew Jeffery <[email protected]>--- Documentation/hwmon/occ | 3 + drivers/hwmon/occ/occ_p9.c | 309 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_p9.h | 30 +++++ 3 files changed, 342 insertions(+) create mode 100644 drivers/hwmon/occ/occ_p9.c create mode 100644 drivers/hwmon/occ/occ_p9.hdrivers/hwmon/occ/occ_p9.c new file +0
-0
drivers/hwmon/occ/occ_p9.c line 7
Quoted 7 lines
drivers/hwmon/occ/occ_p9.c new file +7
-0@@ -0,0 +1,309 @@ +/* +* occ_p9.c - OCC hwmon driver +* +* This file contains the Power9-specific methods and data structures for +* the OCC hwmon driver. +* +* Copyright 2016 IBM Corp.
It's 2017.
drivers/hwmon/occ/occ_p9.c line 17
Quoted 10 lines
drivers/hwmon/occ/occ_p9.c new file +10
-0+* +* 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.
We generally just include the first paragraph. Same goes for all of the files.
Quoted 4 lines
drivers/hwmon/occ/occ_p9.c new file +4
-0+ +static const u32 p9_sensor_hwmon_configs[MAX_OCC_SENSOR_TYPE ] = { +HWMON_I_INPUT |HWMON_I_LABEL ,/* freq: value | label */ +/* temp: value | label | fru_type */
drivers/hwmon/occ/occ_p9.c line 77
Quoted 10 lines
drivers/hwmon/occ/occ_p9.c new file +10
-0+HWMON_T_INPUT |HWMON_T_LABEL |HWMON_T_TYPE , +/* power: value | label | accum[0] | accum[1] | update_tag | +* (function_id | (apss_channel << 8)) +*/ +HWMON_P_INPUT |HWMON_P_LABEL |HWMON_P_AVERAGE_MIN | +HWMON_P_AVERAGE_MAX |HWMON_P_AVERAGE_INTERVAL | +HWMON_P_RESET_HISTORY , +/* caps: curr | max | min | norm | user | source */ +HWMON_P_CAP |HWMON_P_CAP_MAX |HWMON_P_CAP_MIN |HWMON_P_MAX | + HWMON_P_ALARM | HWMON_P_CAP_ALARM,
I find this really hard to read. Perhaps something like this:
#define FREQ_CONFIG (HWMON_I_INPUT | HWMON_I_LABEL) #deifne TEMP_CONFIG (HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_TYPE)
#define POWER_CONFIG ( HWMON_P_INPUT | HWMON_P_LABEL |HWMON_P_AVERAGE_MIN | \
HWMON_P_AVERAGE_MAX |HWMON_P_AVERAGE_INTERVAL | \
HWMON_P_RESET_HISTORY)
etc. Do the same in the p8 driver.
drivers/hwmon/occ/occ_p9.h line 24
Quoted 13 lines
drivers/hwmon/occ/occ_p9.h new file +7
-0+ +#ifndef __OCC_P9_H__ +#define __OCC_P9_H__ + +#include "scom.h" + +struct device ;
Include the header for struct device instead.
Did you consider the one header file for all of your shared functions? I don't think there's much value in having a whole heap of small ones.
Quoted 9 lines
drivers/hwmon/occ/occ_p9.h new file +6
-1+ +const u32 *p9_get_sensor_hwmon_configs (void ); +struct occ *p9_occ_start (struct device *dev,void *bus, +struct occ_bus_ops *bus_ops); + +#endif /* __OCC_P9_H__ */ --1.8.3.1
Re: [PATCH linux v7 6/6] hwmon: occ: Add callbacks for parsing P9 OCC datastructures
Quoted 10 lines · Joel Stanley
drivers/hwmon/occ/occ_p9.h line 24
Quoted 7 lines
drivers/hwmon/occ/occ_p9.h new file +7
-0+ +#ifndef __OCC_P9_H__ +#define __OCC_P9_H__ + +#include "scom.h" + +struct device ;Include the header for struct device instead.
Quoted 2 lines
Did you consider the one header file for all of your shared functions? I don't think there's much value in having a whole heap of small ones.
My bias is against monolithic headers. While it would be no linux/sched.h[1] so the impact won't be great, I prefer keeping headers to only describing the abstract data type at hand. A collection of small, relevant headers makes it easier for me to understand the abstraction boundaries.
Andrew