From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751465AbdFZPHL (ORCPT ); Mon, 26 Jun 2017 11:07:11 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:49376 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751410AbdFZPHE (ORCPT ); Mon, 26 Jun 2017 11:07:04 -0400 Subject: Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver To: Guenter Roeck , linux-kernel@vger.kernel.org Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org, jdelvare@suse.com, mark.rutland@arm.com, robh+dt@kernel.org, gregkh@linuxfoundation.org, cbostic@linux.vnet.ibm.com, jk@ozlabs.org, joel@jms.id.au, andrew@aj.id.au, "Edward A. James" References: <1498171716-26620-1-git-send-email-eajames@linux.vnet.ibm.com> <1498171716-26620-2-git-send-email-eajames@linux.vnet.ibm.com> <8994a4fd-3949-f1a9-3f70-9fa7d406c17a@roeck-us.net> From: Eddie James Date: Mon, 26 Jun 2017 10:06:50 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <8994a4fd-3949-f1a9-3f70-9fa7d406c17a@roeck-us.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 17062615-0004-0000-0000-000012736FA1 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007280; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000214; SDB=6.00878992; UDB=6.00438043; IPR=6.00659146; BA=6.00005443; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015955; XFM=3.00000015; UTC=2017-06-26 15:07:00 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17062615-0005-0000-0000-00007FEF1CF8 Message-Id: <191fdaaa-95e3-5c9b-9439-6f561cac556d@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-26_11:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706260253 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/24/2017 09:15 AM, Guenter Roeck wrote: > On 06/22/2017 03:48 PM, Eddie James wrote: >> From: "Edward A. James" >> >> The OCC is a device embedded on a POWER processor that collects and >> aggregates sensor data from the processor and system. The OCC can >> provide the raw sensor data as well as perform thermal and power >> management on the system. >> >> This driver provides a hwmon interface to the OCC from a service >> processor (e.g. a BMC). The driver supports both POWER8 and POWER9 OCCs. >> Communications with the POWER8 OCC are established over standard I2C >> bus. The driver communicates with the POWER9 OCC through the FSI-based >> OCC driver, which handles the lower-level communication details. >> >> This patch lays out the structure of the OCC hwmon driver. There are two >> platform drivers, one each for P8 and P9 OCCs. These are probed through >> the I2C tree and the FSI-based OCC driver, respectively. The patch also > > Where is that driver ? My apologies Guenter, here is the series: https://patchwork.kernel.org/patch/9802807/ https://patchwork.kernel.org/patch/9802801/ https://patchwork.kernel.org/patch/9802805/ https://patchwork.kernel.org/patch/9802803/ Do these FSI drivers need to be into linux-next before you want to look at this hwmon driver? Just a couple questions below on your comments. Thanks, Eddie > >> defines the first common structures and methods between the two OCC >> versions. >> >> Signed-off-by: Edward A. James >> --- >> .../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 ++++++ >> .../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 ++++++++ >> drivers/hwmon/Kconfig | 2 + >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/occ/Kconfig | 28 +++++++++ >> drivers/hwmon/occ/Makefile | 11 ++++ >> drivers/hwmon/occ/common.c | 43 +++++++++++++ >> drivers/hwmon/occ/common.h | 41 +++++++++++++ >> drivers/hwmon/occ/p8_i2c.c | 70 >> ++++++++++++++++++++++ >> drivers/hwmon/occ/p9_sbe.c | 65 >> ++++++++++++++++++++ >> 10 files changed, 304 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt >> create mode 100644 >> Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> create mode 100644 drivers/hwmon/occ/Kconfig >> create mode 100644 drivers/hwmon/occ/Makefile >> create mode 100644 drivers/hwmon/occ/common.c >> create mode 100644 drivers/hwmon/occ/common.h >> create mode 100644 drivers/hwmon/occ/p8_i2c.c >> create mode 100644 drivers/hwmon/occ/p9_sbe.c >> >> diff --git >> a/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt >> b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt >> new file mode 100644 >> index 0000000..0ecebb7 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > > This needs to be approved by a DT maintainer, if for nothing else for > the new directory and file naming. For my part I have no idea how this > relates to the "fsi" directory introduced in -next. > > >> @@ -0,0 +1,18 @@ >> +Device-tree bindings for FSI-based On-Chip Controller hwmon driver >> +------------------------------------------------------------------ >> + >> +This node MUST be a child node of an OCC driver node. >> + >> +Required properties: >> + - compatible = "ibm,p9-occ-hwmon"; >> + >> +Examples: >> + >> + occ@1 { >> + compatible = "ibm,p9-occ"; > > I don't see "ibm,p9-occ" documented anywhere (including linux-next). > >> + reg = <1>; >> + >> + occ-hwmon@1 { >> + compatible = "ibm,p9-occ-hwmon"; >> + }; >> + }; >> diff --git >> a/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> new file mode 100644 >> index 0000000..8c765d0 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> @@ -0,0 +1,25 @@ >> +Device-tree bindings for I2C-based On-Chip Controller hwmon driver >> +------------------------------------------------------------------ >> + >> +Required properties: >> + - compatible = "ibm,p8-occ-hwmon"; >> + - reg = ; : I2C bus address >> + >> +Examples: >> + >> + i2c-bus@100 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + clock-frequency = <100000>; >> + < more properties > >> + >> + occ-hwmon@1 { >> + compatible = "ibm,p8-occ-hwmon"; >> + reg = <0x50>; >> + }; >> + >> + occ-hwmon@2 { >> + compatible = "ibm,p8-occ-hwmon"; >> + reg = <0x51>; >> + }; >> + }; >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 5ef2814..08add7b 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1250,6 +1250,8 @@ config SENSORS_NSA320 >> This driver can also be built as a module. If so, the module >> will be called nsa320-hwmon. >> +source drivers/hwmon/occ/Kconfig >> + >> config SENSORS_PCF8591 >> tristate "Philips PCF8591 ADC/DAC" >> depends on I2C >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index d4641a9..0b342d0 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o >> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o >> obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o >> +obj-$(CONFIG_SENSORS_OCC) += occ/ >> obj-$(CONFIG_PMBUS) += pmbus/ >> ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG >> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig >> new file mode 100644 >> index 0000000..0501280 >> --- /dev/null >> +++ b/drivers/hwmon/occ/Kconfig >> @@ -0,0 +1,28 @@ >> +# >> +# On-Chip Controller configuration >> +# >> + >> +config SENSORS_OCC >> + tristate "POWER On-Chip Controller" >> + ---help--- >> + This option enables support for monitoring a variety of system >> sensors >> + provided by the On-Chip Controller (OCC) on a POWER processor. >> + >> + This driver can also be built as a module. If so, the module >> will be >> + called occ-hwmon. >> + >> +config SENSORS_OCC_P8_I2C >> + bool "POWER8 OCC through I2C" >> + depends on I2C && SENSORS_OCC >> + ---help--- >> + This option enables support for monitoring sensors provided by >> the OCC >> + on a POWER8 processor. Communications with the OCC are established >> + through I2C bus. >> + >> +config SENSORS_OCC_P9_SBE >> + bool "POWER9 OCC through SBE" >> + depends on OCCFIFO && SENSORS_OCC > > OCCFIFO is not declared anywhere in the kernel, including -next. This > leads me to > believe that I am missing some context. As a result, I can not even > compile this driver. > Please provide the missing context. > >> + ---help--- >> + This option enables support for monitoring sensors provided by >> the OCC >> + on a POWER9 processor. Communications with the OCC are established >> + through SBE engine on an FSI bus. >> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile >> new file mode 100644 >> index 0000000..ab5c3e9 >> --- /dev/null >> +++ b/drivers/hwmon/occ/Makefile >> @@ -0,0 +1,11 @@ >> +occ-hwmon-objs := common.o >> + >> +ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y) >> +occ-hwmon-objs += p9_sbe.o >> +endif >> + >> +ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y) >> +occ-hwmon-objs += p8_i2c.o >> +endif >> + >> +obj-$(CONFIG_SENSORS_OCC) += occ-hwmon.o >> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c >> new file mode 100644 >> index 0000000..60c808c >> --- /dev/null >> +++ b/drivers/hwmon/occ/common.c >> @@ -0,0 +1,43 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include "common.h" > > Please include local include files last, and include files needed here > from here, not indirectly. > >> +#include >> + >> +static int occ_poll(struct occ *occ) >> +{ >> + u16 checksum = occ->poll_cmd_data + 1; >> + u8 cmd[8]; >> + >> + /* big endian */ >> + cmd[0] = 0; /* sequence number */ >> + cmd[1] = 0; /* cmd type */ >> + cmd[2] = 0; /* data length msb */ >> + cmd[3] = 1; /* data length lsb */ >> + cmd[4] = occ->poll_cmd_data; /* data */ >> + cmd[5] = checksum >> 8; /* checksum msb */ >> + cmd[6] = checksum & 0xFF; /* checksum lsb */ >> + cmd[7] = 0; >> + >> + return occ->send_cmd(occ, cmd); >> +} >> + >> +int occ_setup(struct occ *occ, const char *name) >> +{ >> + int rc; >> + >> + rc = occ_poll(occ); >> + if (rc < 0) { >> + dev_err(occ->bus_dev, "failed to get OCC poll response: %d\n", >> + rc); >> + return rc; >> + } >> + >> + return 0; >> +} >> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h >> new file mode 100644 >> index 0000000..dca642a >> --- /dev/null >> +++ b/drivers/hwmon/occ/common.h >> @@ -0,0 +1,41 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#ifndef OCC_COMMON_H >> +#define OCC_COMMON_H >> + >> +#include >> +#include > > Those include files are not needed here. Other include files, > which are needed, are missing. You can not expect the above > to include everything you need for you. > >> + >> +#define OCC_RESP_DATA_BYTES 4089 >> + >> +/* Same response format for all OCC versions. >> + * Allocate the largest possible response. >> + */ >> +struct occ_response { >> + u8 seq_no; >> + u8 cmd_type; >> + u8 return_status; >> + u16 data_length_be; > > Why not "__be16 data_length" if that is what it is ? > >> + u8 data[OCC_RESP_DATA_BYTES]; >> + u16 checksum_be; > > Same here. > >> +} __packed; >> + >> +struct occ { >> + struct device *bus_dev; >> + >> + struct occ_response resp; >> + >> + u8 poll_cmd_data; /* to perform OCC poll command */ >> + int (*send_cmd)(struct occ *occ, u8 *cmd); >> +}; >> + >> +int occ_setup(struct occ *occ, const char *name); >> + >> +#endif /* OCC_COMMON_H */ >> diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c >> new file mode 100644 >> index 0000000..5075146 >> --- /dev/null >> +++ b/drivers/hwmon/occ/p8_i2c.c >> @@ -0,0 +1,70 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include "common.h" >> +#include >> +#include >> +#include > > Please include files in alphabetic order, and local include file(s) last. > >> + >> +struct p8_i2c_occ { >> + struct occ occ; >> + struct i2c_client *client; >> +}; >> + >> +#define to_p8_i2c_occ(x) container_of((x), struct p8_i2c_occ, occ) >> + >> +static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +static int p8_i2c_occ_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct occ *occ; >> + struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev, >> + sizeof(*p8_i2c_occ), >> + GFP_KERNEL); > > I am quite sure this results in a checkpatch warning. It is also > clumsy, with > all those continuation lines. I would sugegst to split the variable > declaration > and the assignment. > >> + if (!p8_i2c_occ) >> + return -ENOMEM; >> + >> + p8_i2c_occ->client = client; >> + occ = &p8_i2c_occ->occ; >> + occ->bus_dev = &client->dev; >> + dev_set_drvdata(&client->dev, occ); >> + >> + occ->poll_cmd_data = 0x10; /* P8 OCC poll data */ > > It would be beneficial to define those commands in the include file. I can add a #define for them, but I'm not sure it makes sense to have the P8/P9 specific command in the common include file? They don't need to be used anywhere else. > >> + occ->send_cmd = p8_i2c_occ_send_cmd; >> + >> + return occ_setup(occ, "p8_occ"); >> +} >> + >> +static const struct of_device_id p8_i2c_occ_of_match[] = { >> + { .compatible = "ibm,p8-occ-hwmon" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match); >> + >> +static const unsigned short p8_i2c_occ_addr[] = { 0x50, 0x51, >> I2C_CLIENT_END }; > > Those addresses should never be listed for auto-detection. Why not? I see lots of drivers in the kernel with a list of I2C addresses to scan. > >> + >> +static struct i2c_driver p8_i2c_occ_driver = { >> + .class = I2C_CLASS_HWMON, > > FWIW, this only adds value if the driver has a detect function. > >> + .driver = { >> + .name = "occ-hwmon", >> + .of_match_table = p8_i2c_occ_of_match, >> + }, >> + .probe = p8_i2c_occ_probe, >> + .address_list = p8_i2c_occ_addr, > > Same here. > >> +}; >> + >> +module_i2c_driver(p8_i2c_occ_driver); >> + >> +MODULE_AUTHOR("Eddie James "); >> +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c >> new file mode 100644 >> index 0000000..0cef428 >> --- /dev/null >> +++ b/drivers/hwmon/occ/p9_sbe.c >> @@ -0,0 +1,65 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include "common.h" >> +#include >> +#include >> +#include >> +#include >> + > Alphabetic order, and local include file(s) last. > >> +struct p9_sbe_occ { >> + struct occ occ; >> + struct device *sbe; >> +}; >> + >> +#define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ) >> + >> +static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +static int p9_sbe_occ_probe(struct platform_device *pdev) >> +{ >> + struct occ *occ; >> + struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev, >> + sizeof(*p9_sbe_occ), >> + GFP_KERNEL); > > Same as above. > >> + if (!p9_sbe_occ) >> + return -ENOMEM; >> + >> + p9_sbe_occ->sbe = pdev->dev.parent; >> + occ = &p9_sbe_occ->occ; >> + occ->bus_dev = &pdev->dev; >> + platform_set_drvdata(pdev, occ); >> + >> + occ->poll_cmd_data = 0x20; /* P9 OCC poll data */ >> + occ->send_cmd = p9_sbe_occ_send_cmd; >> + >> + return occ_setup(occ, "p9_occ"); >> +} >> + >> +static const struct of_device_id p9_sbe_occ_of_match[] = { >> + { .compatible = "ibm,p9-occ-hwmon" }, >> + { }, >> +}; >> + >> +static struct platform_driver p9_sbe_occ_driver = { >> + .driver = { >> + .name = "occ-hwmon", >> + .of_match_table = p9_sbe_occ_of_match, >> + }, >> + .probe = p9_sbe_occ_probe, >> +}; >> + >> +module_platform_driver(p9_sbe_occ_driver); >> + >> +MODULE_AUTHOR("Eddie James "); >> +MODULE_DESCRIPTION("BMC P9 OCC hwmon driver"); >> +MODULE_LICENSE("GPL"); >> >