[PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Jae Hyun Yoo]

From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>

Hello,

This patch set provides support for PECI of AST2400/2500 which can give us PECI
functionalities such as temperature monitoring, platform manageability,
processor diagnostics and failure analysis. Also provides generic peci.h and
peci_ioctl.h headers to provide compatibility to peci drivers that can be
implemented later e.g. Nuvoton's BMC SoC family.

The misc peci driver can be used as a multi-purpose PECI controller driver which
serializes all PECI transactions that coming from user space and from other
kernel modules. This misc peci driver could be replaced with other BMC chipsets'
implementation if the implementation provide compatible 'peci_ioctl'
inter-module call and ioctl scheme defined in peci.h and peci_ioctl.h files.

The hwmon peci driver implements a generic PECI hwmon feature which is running
with a PECI misc driver supports compatible native PECI command suite for
retrieving temperatures of the CPU package, CPU cores and DIMM components.

Please review.

-Jae

Jae Hyun Yoo (6):
  Documentation: dt-bindings: Add Aspeed PECI
  ARM: dts: aspeed: peci: Add Aspeed PECI
  drivers/misc: Add driver for Aspeed PECI and generic PECI headers
  Documentation: dt-bindings: Add a generic PECI hwmon
  Documentation: hwmon: Add a generic PECI hwmon
  drivers/hwmon: Add a driver for a generic PECI hwmon

 .../devicetree/bindings/hwmon/peci-hwmon.txt       |   33 +
 .../devicetree/bindings/misc/aspeed-peci.txt       |   55 +
 Documentation/hwmon/peci-hwmon                     |   74 ++
 arch/arm/boot/dts/aspeed-g4.dtsi                   |   14 +
 arch/arm/boot/dts/aspeed-g5.dtsi                   |   14 +
 drivers/hwmon/Kconfig                              |    6 +
 drivers/hwmon/Makefile                             |    1 +
 drivers/hwmon/peci-hwmon.c                         |  953 +++++++++++++++++
 drivers/misc/Kconfig                               |    9 +
 drivers/misc/Makefile                              |    1 +
 drivers/misc/aspeed-peci.c                         | 1130 ++++++++++++++++++++
 include/misc/peci.h                                |   11 +
 include/uapi/linux/Kbuild                          |    1 +
 include/uapi/linux/peci_ioctl.h                    |  270 +++++
 14 files changed, 2572 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-peci.txt
 create mode 100644 Documentation/hwmon/peci-hwmon
 create mode 100644 drivers/hwmon/peci-hwmon.c
 create mode 100644 drivers/misc/aspeed-peci.c
 create mode 100644 include/misc/peci.h
 create mode 100644 include/uapi/linux/peci_ioctl.h

-- 
2.7.4

[PATCH linux dev-4.10 1/6] Documentation: dt-bindings: Add Aspeed PECI

[Jae Hyun Yoo]

This commit adds a dt-bindings document for Aspeed PECI.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 .../devicetree/bindings/misc/aspeed-peci.txt       | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-peci.txt

diff --git a/Documentation/devicetree/bindings/misc/aspeed-peci.txt b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
new file mode 100644
index 0000000..d277c73
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
@@ -0,0 +1,55 @@
+* ASPEED PECI (Platform Environment Control Interface) misc driver.
+
+Hardware Interfaces:
+- This driver implements support for the ASPEED AST2400/2500 PECI which has the
+  following features:
+	- Directly connected to APB bus
+	- Intel PECI 3.1 compliant (PECI 3.0 for AST2400)
+	- Maximum packet length is 256 bytes (Baseline transmission unit)
+	- Support up to 8 CPUs and 2 domains per CPU
+	- Integrate PECI compliant I/O buffers, can connect to PECI bus directly
+	- Transmit buffer 32 bytes and receive buffer 32 bytes
+
+Required properties:
+- compatible: "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
+	- aspeed,ast2400-peci: Aspeed AST2400 family PECI control interface
+	- aspeed,ast2500-peci: Aspeed AST2500 family PECI control interface
+- reg: Should contain PECI registers location and length
+- interrupts: Should contain PECI interrupt
+- clocks: Should contain clock source. = <&clk_clkin>;
+- clock_frequency: Should contain the operation frequency of PECI controller.
+	187500 ~ 24000000
+
+Optional properties:
+- msg-timing-nego: Message timing negotiation period.
+	This value will determine the period of message timing negotiation to be
+	issued by PECI controller. The unit of the programmed value is four
+	times of PECI clock period.
+	0 ~ 255 (default: 1)
+- addr-timing-nego: Address timing negotiation period.
+	This value will determine the period of address timing negotiation to be
+	issued by PECI controller. The unit of the programmed value is four
+	times of PECI clock period.
+	0 ~ 255 (default: 1)
+- rd-sampling-point: Read sampling point selection.
+	The whole period of a bit time will be divided into 16 time frames.
+	This value will determine which time frame this controller will sample
+	PECI signal for data read back. Usually in the middle of a bit time is
+	the best.
+	0 ~ 15 (default: 8)
+- cmd_timeout_ms: Command timeout in units of ms
+	1 ~ 60000 (default: 1000)
+
+Example:
+	peci: peci@1e78b000 {
+		compatible = "aspeed,ast2500-peci";
+		reg = <0x1e78b000 0x60>;
+		interrupt-controller;
+		interrupts = <15>;
+		clocks = <&clk_clkin>;
+		clock-frequency = <24000000>;
+		msg-timing-nego = <1>;
+		addr-timing-nego = <1>;
+		rd-sampling-point = <8>;
+		cmd-timeout-ms = <1000>;
+	};
-- 
2.7.4

[PATCH linux dev-4.10 2/6] ARM: dts: aspeed: peci: Add Aspeed PECI

[Jae Hyun Yoo]

This commit adds Aspeed PECI node into aspeed-g4 and aspeed-g5.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 arch/arm/boot/dts/aspeed-g4.dtsi | 14 ++++++++++++++
 arch/arm/boot/dts/aspeed-g5.dtsi | 14 ++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index b82ebef..7ecc7b2 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -238,6 +238,20 @@
 				clocks = <&clk_hpll>;
 			};
 
+			peci: peci@1e78b000 {
+				compatible = "aspeed,ast2400-peci";
+				reg = <0x1e78b000 0x60>;
+				interrupt-controller;
+				interrupts = <15>;
+				clocks = <&clk_clkin>;
+				clock-frequency = <24000000>;
+				msg-timing-nego = <1>;
+				addr-timing-nego = <1>;
+				rd-sampling-point = <8>;
+				cmd-timeout-ms = <1000>;
+				status = "disabled";
+			};
+
 			sgpio: gpio@0x1e780200 {
 				#gpio-cells = <2>;
 				gpio-controller;
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index ba3607c..b4e8d51 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -289,6 +289,20 @@
 				clocks = <&clk_hpll>;
 			};
 
+			peci: peci@1e78b000 {
+				compatible = "aspeed,ast2500-peci";
+				reg = <0x1e78b000 0x60>;
+				interrupt-controller;
+				interrupts = <15>;
+				clocks = <&clk_clkin>;
+				clock-frequency = <24000000>;
+				msg-timing-nego = <1>;
+				addr-timing-nego = <1>;
+				rd-sampling-point = <8>;
+				cmd-timeout-ms = <1000>;
+				status = "disabled";
+			};
+
 			timer: timer@1e782000 {
 				compatible = "aspeed,ast2400-timer";
 				reg = <0x1e782000 0x90>;
-- 
2.7.4

[PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

This commit adds driver implementation for Aspeed PECI. Also adds
generic peci.h and peci_ioctl.h files to provide compatibility
to peci drivers that can be implemented later e.g. Nuvoton's BMC
SoC family.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 drivers/misc/Kconfig            |    9 +
 drivers/misc/Makefile           |    1 +
 drivers/misc/aspeed-peci.c      | 1130 +++++++++++++++++++++++++++++++++++++++
 include/misc/peci.h             |   11 +
 include/uapi/linux/Kbuild       |    1 +
 include/uapi/linux/peci_ioctl.h |  270 ++++++++++
 6 files changed, 1422 insertions(+)
 create mode 100644 drivers/misc/aspeed-peci.c
 create mode 100644 include/misc/peci.h
 create mode 100644 include/uapi/linux/peci_ioctl.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 02ffdd1..96e1e04 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -782,6 +782,15 @@ config ASPEED_LPC_SNOOP
 	  allows the BMC to listen on and save the data written by
 	  the host to an arbitrary LPC I/O port.
 
+config ASPEED_PECI
+	tristate "Aspeed AST2400/AST2500 PECI support"
+	select CRC8
+	select REGMAP_MMIO
+	depends on ARCH_ASPEED || COMPILE_TEST
+	help
+	  Provides a driver for Platform Environment Control Interface (PECI)
+	  controller on Aspeed AST2400/AST2500 SoC.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index ab8af76..8a22455 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
 obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)	+= aspeed-lpc-snoop.o
+obj-$(CONFIG_ASPEED_PECI)       += aspeed-peci.o
 
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
diff --git a/drivers/misc/aspeed-peci.c b/drivers/misc/aspeed-peci.c
new file mode 100644
index 0000000..04fb794
--- /dev/null
+++ b/drivers/misc/aspeed-peci.c
@@ -0,0 +1,1130 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2012-2020 ASPEED Technology Inc.
+// Copyright (c) 2017 Intel Corporation
+
+#include <linux/clk.h>
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/peci_ioctl.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/semaphore.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+#define SOC_NAME "aspeed"
+#define DEVICE_NAME "peci"
+
+#define DUMP_DEBUG 0
+
+/* Aspeed PECI Registers */
+#define AST_PECI_CTRL     0x00
+#define AST_PECI_TIMING   0x04
+#define AST_PECI_CMD      0x08
+#define AST_PECI_CMD_CTRL 0x0c
+#define AST_PECI_EXP_FCS  0x10
+#define AST_PECI_CAP_FCS  0x14
+#define AST_PECI_INT_CTRL 0x18
+#define AST_PECI_INT_STS  0x1c
+#define AST_PECI_W_DATA0  0x20
+#define AST_PECI_W_DATA1  0x24
+#define AST_PECI_W_DATA2  0x28
+#define AST_PECI_W_DATA3  0x2c
+#define AST_PECI_R_DATA0  0x30
+#define AST_PECI_R_DATA1  0x34
+#define AST_PECI_R_DATA2  0x38
+#define AST_PECI_R_DATA3  0x3c
+#define AST_PECI_W_DATA4  0x40
+#define AST_PECI_W_DATA5  0x44
+#define AST_PECI_W_DATA6  0x48
+#define AST_PECI_W_DATA7  0x4c
+#define AST_PECI_R_DATA4  0x50
+#define AST_PECI_R_DATA5  0x54
+#define AST_PECI_R_DATA6  0x58
+#define AST_PECI_R_DATA7  0x5c
+
+/* AST_PECI_CTRL - 0x00 : Control Register */
+#define PECI_CTRL_SAMPLING_MASK     GENMASK(19, 16)
+#define PECI_CTRL_SAMPLING(x)       ((x << 16) & PECI_CTRL_SAMPLING_MASK)
+#define PECI_CTRL_SAMPLING_GET(x)   ((x & PECI_CTRL_SAMPLING_MASK) >> 16)
+#define PECI_CTRL_READ_MODE_MASK    GENMASK(13, 12)
+#define PECI_CTRL_READ_MODE(x)      ((x << 12) & PECI_CTRL_READ_MODE_MASK)
+#define PECI_CTRL_READ_MODE_GET(x)  ((x & PECI_CTRL_READ_MODE_MASK) >> 12)
+#define PECI_CTRL_READ_MODE_COUNT   BIT(12)
+#define PECI_CTRL_READ_MODE_DBG     BIT(13)
+#define PECI_CTRL_CLK_SOURCE_MASK   BIT(11)
+#define PECI_CTRL_CLK_SOURCE(x)     ((x << 11) & PECI_CTRL_CLK_SOURCE_MASK)
+#define PECI_CTRL_CLK_SOURCE_GET(x) ((x & PECI_CTRL_CLK_SOURCE_MASK) >> 11)
+#define PECI_CTRL_CLK_DIV_MASK      GENMASK(10, 8)
+#define PECI_CTRL_CLK_DIV(x)        ((x << 8) & PECI_CTRL_CLK_DIV_MASK)
+#define PECI_CTRL_CLK_DIV_GET(x)    ((x & PECI_CTRL_CLK_DIV_MASK) >> 8)
+#define PECI_CTRL_INVERT_OUT        BIT(7)
+#define PECI_CTRL_INVERT_IN         BIT(6)
+#define PECI_CTRL_BUS_CONTENT_EN    BIT(5)
+#define PECI_CTRL_PECI_EN           BIT(4)
+#define PECI_CTRL_PECI_CLK_EN       BIT(0)
+
+/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */
+#define PECI_TIMING_MESSAGE_MASK   GENMASK(15, 8)
+#define PECI_TIMING_MESSAGE(x)     ((x << 8) & PECI_TIMING_MESSAGE_MASK)
+#define PECI_TIMING_MESSAGE_GET(x) ((x & PECI_TIMING_MESSAGE_MASK) >> 8)
+#define PECI_TIMING_ADDRESS_MASK   GENMASK(7, 0)
+#define PECI_TIMING_ADDRESS(x)     (x & PECI_TIMING_ADDRESS_MASK)
+#define PECI_TIMING_ADDRESS_GET(x) (x & PECI_TIMING_ADDRESS_MASK)
+
+/* AST_PECI_CMD - 0x08 : Command Register */
+#define PECI_CMD_PIN_MON    BIT(31)
+#define PECI_CMD_STS_MASK   GENMASK(27, 24)
+#define PECI_CMD_STS_GET(x) ((x & PECI_CMD_STS_MASK) >> 24)
+#define PECI_CMD_FIRE       BIT(0)
+
+/* AST_PECI_LEN - 0x0C : Read/Write Length Register */
+#define PECI_AW_FCS_EN       BIT(31)
+#define PECI_READ_LEN_MASK   GENMASK(23, 16)
+#define PECI_READ_LEN(x)     ((x << 16) & PECI_READ_LEN_MASK)
+#define PECI_WRITE_LEN_MASK  GENMASK(15, 8)
+#define PECI_WRITE_LEN(x)    ((x << 8) & PECI_WRITE_LEN_MASK)
+#define PECI_TAGET_ADDR_MASK GENMASK(7, 0)
+#define PECI_TAGET_ADDR(x)   ((x) & PECI_TAGET_ADDR_MASK)
+
+/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register  */
+#define PECI_EXPECT_READ_FCS_MASK      GENMASK(23, 16)
+#define PECI_EXPECT_READ_FCS_GET(x)    ((x & PECI_EXPECT_READ_FCS_MASK) >> 16)
+#define PECI_EXPECT_AW_FCS_AUTO_MASK   GENMASK(15, 8)
+#define PECI_EXPECT_AW_FCS_AUTO_GET(x) ((x & PECI_EXPECT_AW_FCS_AUTO_MASK) >> 8)
+#define PECI_EXPECT_WRITE_FCS_MASK     GENMASK(7, 0)
+#define PECI_EXPECT_WRITE_FCS_GET(x)   (x & PECI_EXPECT_WRITE_FCS_MASK)
+
+/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */
+#define PECI_CAPTURE_READ_FCS_MASK    GENMASK(23, 16)
+#define PECI_CAPTURE_READ_FCS_GET(x)  ((x & PECI_CAPTURE_READ_FCS_MASK) >> 16)
+#define PECI_CAPTURE_WRITE_FCS_MASK   GENMASK(7, 0)
+#define PECI_CAPTURE_WRITE_FCS_GET(x) (x & PECI_CAPTURE_WRITE_FCS_MASK)
+
+/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */
+#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30)
+#define PECI_INT_TIMEOUT            BIT(4)
+#define PECI_INT_CONNECT            BIT(3)
+#define PECI_INT_W_FCS_BAD          BIT(2)
+#define PECI_INT_W_FCS_ABORT        BIT(1)
+#define PECI_INT_CMD_DONE           BIT(0)
+
+struct aspeed_peci {
+	struct miscdevice miscdev;
+	struct device *dev;
+	struct regmap *regmap;
+	atomic_t ref_count;
+	int irq;
+	struct completion xfer_complete;
+	u32 sts;
+	u32 cmd_timeout_ms;
+	bool initialized;
+	bool cmd_support[PECI_CMD_MAX];
+	struct mutex mutex;
+};
+
+#define PECI_INT_MASK  (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \
+			PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \
+			PECI_INT_CMD_DONE)
+
+#define PECI_IDLE_CHECK_TIMEOUT         5
+
+#define PECI_RD_SAMPLING_POINT_DEFAULT  8
+#define PECI_RD_SAMPLING_POINT_MAX      15
+#define PECI_CLK_DIV_DEFAULT            0
+#define PECI_CLK_DIV_MAX                7
+#define PECI_MSG_TIMING_NEGO_DEFAULT    1
+#define PECI_MSG_TIMING_NEGO_MAX        255
+#define PECI_ADDR_TIMING_NEGO_DEFAULT   1
+#define PECI_ADDR_TIMING_NEGO_MAX       255
+#define PECI_CMD_TIMEOUT_MS_DEFAULT     1000
+#define PECI_CMD_TIMEOUT_MS_MAX         60000
+
+#define PECI_CRC8_POLYNOMIAL            0x07
+
+DECLARE_CRC8_TABLE(aspeed_peci_crc8_table);
+
+static struct aspeed_peci *aspeed_peci_priv;
+
+
+static u8 compute_aw_fcs(u8 *data, int len)
+{
+	return crc8(aspeed_peci_crc8_table, data, (size_t)len, 0);
+}
+
+static int ioctl_xfer_msg(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_xfer_msg *pumsg = pmsg;
+	u32 peci_head;
+	u32 peci_state;
+	u32 rx_data;
+	uint reg;
+	long timeout;
+	int i;
+	int rc = 0;
+
+	reinit_completion(&priv->xfer_complete);
+
+	peci_head = PECI_TAGET_ADDR(pumsg->client_addr) |
+				    PECI_WRITE_LEN(pumsg->tx_len) |
+				    PECI_READ_LEN(pumsg->rx_len);
+
+	rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < pumsg->tx_len; i += 4) {
+		reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 :
+			       AST_PECI_W_DATA4 + i % 16;
+		rc = regmap_write(priv->regmap, reg,
+				  (pumsg->tx_buf[i + 3] << 24) |
+				  (pumsg->tx_buf[i + 2] << 16) |
+				  (pumsg->tx_buf[i + 1] << 8) |
+				  pumsg->tx_buf[i + 0]);
+		if (rc)
+			return rc;
+	}
+
+	dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
+#if DUMP_DEBUG
+	print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1,
+		       pumsg->tx_buf, pumsg->tx_len, true);
+#endif
+
+	rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE);
+	if (rc)
+		return rc;
+
+	timeout = wait_for_completion_interruptible_timeout(
+					&priv->xfer_complete,
+					msecs_to_jiffies(priv->cmd_timeout_ms));
+
+	dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
+	if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
+		dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
+			PECI_CMD_STS_GET(peci_state));
+	else
+		dev_dbg(priv->dev, "PECI_STATE : read error\n");
+
+	if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
+		if (timeout <= 0) {
+			dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+			rc = -ETIME;
+		} else {
+			dev_dbg(priv->dev, "No valid response!\n");
+			rc = -EFAULT;
+		}
+		return rc;
+	}
+
+	for (i = 0; i < pumsg->rx_len; i++) {
+		u8 byte_offset = i % 4;
+
+		if (byte_offset == 0) {
+			reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 :
+				       AST_PECI_R_DATA4 + i % 16;
+			rc = regmap_read(priv->regmap, reg, &rx_data);
+			if (rc)
+				return rc;
+		}
+
+		pumsg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3));
+	}
+
+#if DUMP_DEBUG
+	print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1,
+		       pumsg->rx_buf, pumsg->rx_len, true);
+#endif
+	if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
+		dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
+			PECI_CMD_STS_GET(peci_state));
+	else
+		dev_dbg(priv->dev, "PECI_STATE : read error\n");
+	dev_dbg(priv->dev, "------------------------\n");
+
+	return rc;
+}
+
+static int
+xfer_msg_with_retries(struct aspeed_peci *priv, void *pmsg, bool has_aw_fcs)
+{
+	struct peci_xfer_msg *pumsg = pmsg;
+	uint retries = DEV_PECI_RETRY_ATTEMPTS;
+	int rc = 0;
+
+	/* Per the PECI spec, need to retry any commands that return 0x8x */
+	do {
+		rc = ioctl_xfer_msg(priv, pumsg);
+		if (!(!rc && ((pumsg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
+			      DEV_PECI_CC_TIMEOUT)))
+			break;
+
+		/* Set the retry bit to indicate a retry attempt */
+		pumsg->tx_buf[1] |= DEV_PECI_RETRY_BIT;
+
+		/* Recalculate the AW FCS if it has one */
+		if (has_aw_fcs)
+			pumsg->tx_buf[pumsg->tx_len - 1] = 0x80 ^
+					compute_aw_fcs((u8 *)pumsg,
+					2 + pumsg->tx_len);
+
+		/*
+		 * Retry for at least 250ms before returning an error.
+		 * Retry interval guideline:
+		 *   No minimum < Retry Interval < No maximum
+		 *                (recommend 10ms)
+		 */
+		usleep_range(DEV_PECI_RETRY_DELAY_MS * 1000,
+			     (DEV_PECI_RETRY_DELAY_MS * 1000) + 1000);
+	} while (retries--);
+
+	return rc;
+}
+
+static int initialize(struct aspeed_peci *priv)
+{
+	struct peci_xfer_msg msg;
+	u32 dib;
+	int rc = 0;
+
+	/* Initialize it just once. */
+	if (priv->initialized)
+		return 0;
+
+	/* Update command table just once. */
+	if (priv->cmd_support[PECI_CMD_PING])
+		return 0;
+
+	msg.client_addr = PECI_BASE_ADDR;
+	msg.tx_len      = GET_DIB_WR_LEN;
+	msg.rx_len      = GET_DIB_RD_LEN;
+	msg.tx_buf[0]   = GET_DIB_PECI_CMD;
+
+	rc = ioctl_xfer_msg(priv, &msg);
+	if (rc < 0) {
+		dev_dbg(priv->dev, "PECI xfer error, rc : %d\n", rc);
+		return rc;
+	}
+
+	dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
+	      (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
+
+	/* Check special case for Get DIB command */
+	if (dib == 0x00) {
+		dev_dbg(priv->dev, "DIB read as 0x00\n");
+		return -1;
+	}
+
+	if (!rc) {
+		/*
+		 * setting up the supporting commands based on minor rev#
+		 * see PECI Spec Table 3-1
+		 */
+		priv->cmd_support[PECI_CMD_PING] = true;
+		priv->cmd_support[PECI_CMD_GET_TEMP] = true;
+		priv->cmd_support[PECI_CMD_GET_DIB] = true;
+
+		/* get minor rev# */
+		dib = (dib >> 8) & 0xF;
+
+		if (dib >= 0x1) {
+			priv->cmd_support[PECI_CMD_RD_PKG_CFG] = true;
+			priv->cmd_support[PECI_CMD_WR_PKG_CFG] = true;
+		}
+
+		if (dib >= 0x2)
+			priv->cmd_support[PECI_CMD_RD_IA_MSR] = true;
+
+		if (dib >= 0x3) {
+			priv->cmd_support[PECI_CMD_RD_PCI_CFG_LOCAL] = true;
+			priv->cmd_support[PECI_CMD_WR_PCI_CFG_LOCAL] = true;
+		}
+
+		if (dib >= 0x4)
+			priv->cmd_support[PECI_CMD_RD_PCI_CFG] = true;
+
+		if (dib >= 0x5)
+			priv->cmd_support[PECI_CMD_WR_PCI_CFG] = true;
+
+		if (dib >= 0x6)
+			priv->cmd_support[PECI_CMD_WR_IA_MSR] = true;
+
+		priv->initialized = true;
+	} else {
+		dev_dbg(priv->dev, "Error reading DIB, rc : %d\n", rc);
+	}
+
+	return rc;
+}
+
+static int ioctl_ping(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_ping_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	int rc;
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_PING]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len      = 0;
+	msg.rx_len      = 0;
+
+	rc = ioctl_xfer_msg(priv, &msg);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static int ioctl_get_dib(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_get_dib_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	int rc;
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_GET_DIB]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len      = GET_DIB_WR_LEN;
+	msg.rx_len      = GET_DIB_RD_LEN;
+	msg.tx_buf[0]   = GET_DIB_PECI_CMD;
+
+	rc = ioctl_xfer_msg(priv, &msg);
+	if (rc < 0)
+		return rc;
+
+	pumsg->dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
+		     (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
+
+	return 0;
+}
+
+static int ioctl_get_temp(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_get_temp_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	int rc;
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_GET_TEMP]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len      = GET_TEMP_WR_LEN;
+	msg.rx_len      = GET_TEMP_RD_LEN;
+	msg.tx_buf[0]   = GET_TEMP_PECI_CMD;
+
+	rc = ioctl_xfer_msg(priv, &msg);
+	if (rc < 0)
+		return rc;
+
+	pumsg->temp_raw = (signed short)(msg.rx_buf[0] | (msg.rx_buf[1] << 8));
+
+	return 0;
+}
+
+static int ioctl_rd_pkg_cfg(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_rd_pkg_cfg_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	int rc = 0;
+
+	/* Per the PECI spec, the read length must be a byte, word, or dword */
+	if (pumsg->rx_len != 1 && pumsg->rx_len != 2 && pumsg->rx_len != 4) {
+		dev_dbg(priv->dev, "Invalid read length, rx_len: %d\n",
+			pumsg->rx_len);
+		return -EINVAL;
+	}
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_RD_PKG_CFG]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len = RDPKGCFG_WRITE_LEN;
+	/* read lengths of 1 and 2 result in an error, so only use 4 for now */
+	msg.rx_len = RDPKGCFG_READ_LEN_BASE + pumsg->rx_len;
+	msg.tx_buf[0] = RDPKGCFG_PECI_CMD;
+	msg.tx_buf[1] = 0x00;         /* request byte for Host ID / Retry bit */
+				      /* Host ID is 0 for PECI 3.0 */
+	msg.tx_buf[2] = pumsg->index;            /* RdPkgConfig index */
+	msg.tx_buf[3] = (u8)pumsg->param;        /* LSB - Config parameter */
+	msg.tx_buf[4] = (u8)(pumsg->param >> 8); /* MSB - Config parameter */
+
+	rc = xfer_msg_with_retries(priv, &msg, false);
+	if (rc || msg.rx_buf[0] != DEV_PECI_CC_SUCCESS) {
+		dev_dbg(priv->dev, "ioctl error, rc : %d\n", rc);
+		return -EIO;
+	}
+
+	memcpy(pumsg->pkg_config, &msg.rx_buf[1], pumsg->rx_len);
+
+	return rc;
+}
+
+static int ioctl_wr_pkg_cfg(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_wr_pkg_cfg_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	int rc = 0, i;
+
+	/* Per the PECI spec, the write length must be a dword */
+	if (pumsg->tx_len != 4) {
+		dev_dbg(priv->dev, "Invalid write length, tx_len: %d\n",
+			pumsg->tx_len);
+		return -EINVAL;
+	}
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_WR_PKG_CFG]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len = WRPKGCFG_WRITE_LEN_BASE + pumsg->tx_len;
+	/* read lengths of 1 and 2 result in an error, so only use 4 for now */
+	msg.rx_len = WRPKGCFG_READ_LEN;
+	msg.tx_buf[0] = WRPKGCFG_PECI_CMD;
+	msg.tx_buf[1] = 0x00;         /* request byte for Host ID / Retry bit */
+				      /* Host ID is 0 for PECI 3.0 */
+	msg.tx_buf[2] = pumsg->index;            /* RdPkgConfig index */
+	msg.tx_buf[3] = (u8)pumsg->param;        /* LSB - Config parameter */
+	msg.tx_buf[4] = (u8)(pumsg->param >> 8); /* MSB - Config parameter */
+	for (i = 0; i < pumsg->tx_len; i++)
+		msg.tx_buf[5 + i] = ((u8 *)&pumsg->value)[i];
+
+	/* Add an Assure Write Frame Check Sequence byte */
+	msg.tx_buf[5 + i] = 0x80 ^
+			    compute_aw_fcs((u8 *)&msg, 8 + pumsg->tx_len);
+
+	rc = xfer_msg_with_retries(priv, &msg, true);
+	if (rc || msg.rx_buf[0] != DEV_PECI_CC_SUCCESS) {
+		dev_dbg(priv->dev, "ioctl error, rc : %d\n", rc);
+		return -EIO;
+	}
+
+	return rc;
+}
+
+static int ioctl_rd_ia_msr(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_rd_ia_msr_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	int rc = 0;
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_RD_IA_MSR]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len = RDIAMSR_WRITE_LEN;
+	msg.rx_len = RDIAMSR_READ_LEN;
+	msg.tx_buf[0] = RDIAMSR_PECI_CMD;
+	msg.tx_buf[1] = 0x00;
+	msg.tx_buf[2] = pumsg->thread_id;
+	msg.tx_buf[3] = (u8)pumsg->address;
+	msg.tx_buf[4] = (u8)(pumsg->address >> 8);
+
+	rc = xfer_msg_with_retries(priv, &msg, false);
+	if (rc || msg.rx_buf[0] != DEV_PECI_CC_SUCCESS) {
+		dev_dbg(priv->dev, "ioctl error, rc : %d\n", rc);
+		return -EIO;
+	}
+
+	memcpy(&pumsg->value, &msg.rx_buf[1], sizeof(uint64_t));
+
+	return rc;
+}
+
+static int ioctl_rd_pci_cfg(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_rd_pci_cfg_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	u32 address;
+	int rc = 0;
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_RD_PCI_CFG]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	address = pumsg->reg;                  /* [11:0]  - Register */
+	address |= (u32)pumsg->function << 12; /* [14:12] - Function */
+	address |= (u32)pumsg->device << 15;   /* [19:15] - Device   */
+	address |= (u32)pumsg->bus << 20;      /* [27:20] - Bus      */
+					       /* [31:28] - Reserved */
+	msg.client_addr = pumsg->target;
+	msg.tx_len = RDPCICFG_WRITE_LEN;
+	msg.rx_len = RDPCICFG_READ_LEN;
+	msg.tx_buf[0] = RDPCICFG_PECI_CMD;
+	msg.tx_buf[1] = 0x00;         /* request byte for Host ID / Retry bit */
+				      /* Host ID is 0 for PECI 3.0 */
+	msg.tx_buf[2] = (u8)address;         /* LSB - PCI Config Address */
+	msg.tx_buf[3] = (u8)(address >> 8);  /* PCI Config Address */
+	msg.tx_buf[4] = (u8)(address >> 16); /* PCI Config Address */
+	msg.tx_buf[5] = (u8)(address >> 24); /* MSB - PCI Config Address */
+
+	rc = xfer_msg_with_retries(priv, &msg, false);
+	if (rc || msg.rx_buf[0] != DEV_PECI_CC_SUCCESS) {
+		dev_dbg(priv->dev, "ioctl error, rc : %d\n", rc);
+		return -EIO;
+	}
+
+	memcpy(pumsg->pci_config, &msg.rx_buf[1], 4);
+
+	return rc;
+}
+
+static int ioctl_rd_pci_cfg_local(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_rd_pci_cfg_local_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	u32 address;
+	int rc = 0;
+
+	/* Per the PECI spec, the read length must be a byte, word, or dword */
+	if (pumsg->rx_len != 1 && pumsg->rx_len != 2 && pumsg->rx_len != 4) {
+		dev_dbg(priv->dev, "Invalid read length, rx_len: %d\n",
+			pumsg->rx_len);
+		return -EINVAL;
+	}
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_RD_PCI_CFG_LOCAL]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	address = pumsg->reg;                  /* [11:0]  - Register */
+	address |= (u32)pumsg->function << 12; /* [14:12] - Function */
+	address |= (u32)pumsg->device << 15;   /* [19:15] - Device   */
+	address |= (u32)pumsg->bus << 20;      /* [23:20] - Bus      */
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len = RDPCICFGLOCAL_WRITE_LEN;
+	msg.rx_len = RDPCICFGLOCAL_READ_LEN_BASE + pumsg->rx_len;
+	msg.tx_buf[0] = RDPCICFGLOCAL_PECI_CMD;
+	msg.tx_buf[1] = 0x00;         /* request byte for Host ID / Retry bit */
+				      /* Host ID is 0 for PECI 3.0 */
+	msg.tx_buf[2] = (u8)address;       /* LSB - PCI Configuration Address */
+	msg.tx_buf[3] = (u8)(address >> 8);  /* PCI Configuration Address */
+	msg.tx_buf[4] = (u8)(address >> 16); /* PCI Configuration Address */
+
+	rc = xfer_msg_with_retries(priv, &msg, false);
+	if (rc || msg.rx_buf[0] != DEV_PECI_CC_SUCCESS) {
+		dev_dbg(priv->dev, "ioctl error, rc : %d\n", rc);
+		return -EIO;
+	}
+
+	memcpy(pumsg->pci_config, &msg.rx_buf[1], pumsg->rx_len);
+
+	return rc;
+}
+
+static int ioctl_wr_pci_cfg_local(struct aspeed_peci *priv, void *pmsg)
+{
+	struct peci_wr_pci_cfg_local_msg *pumsg = pmsg;
+	struct peci_xfer_msg msg;
+	u32 address;
+	int rc = 0, i;
+
+	/* Per the PECI spec, the write length must be a byte, word, or dword */
+	if (pumsg->tx_len != 1 && pumsg->tx_len != 2 && pumsg->tx_len != 4) {
+		dev_dbg(priv->dev, "Invalid write length, tx_len: %d\n",
+			pumsg->tx_len);
+		return -EINVAL;
+	}
+
+	if (!priv->initialized && initialize(priv) < 0) {
+		dev_dbg(priv->dev, "Failed to initialize peci\n");
+		return -EIO;
+	}
+
+	if (!priv->cmd_support[PECI_CMD_RD_PCI_CFG_LOCAL]) {
+		dev_dbg(priv->dev, "Command is not supported\n");
+		return -EBADRQC;
+	}
+
+	address = pumsg->reg;                  /* [11:0]  - Register */
+	address |= (u32)pumsg->function << 12; /* [14:12] - Function */
+	address |= (u32)pumsg->device << 15;   /* [19:15] - Device   */
+	address |= (u32)pumsg->bus << 20;      /* [23:20] - Bus      */
+
+	msg.client_addr = pumsg->target;
+	msg.tx_len = WRPCICFGLOCAL_WRITE_LEN_BASE + pumsg->tx_len;
+	msg.rx_len = WRPCICFGLOCAL_READ_LEN;
+	msg.tx_buf[0] = WRPCICFGLOCAL_PECI_CMD;
+	msg.tx_buf[1] = 0x00;         /* request byte for Host ID / Retry bit */
+				      /* Host ID is 0 for PECI 3.0 */
+	msg.tx_buf[2] = (u8)address;       /* LSB - PCI Configuration Address */
+	msg.tx_buf[3] = (u8)(address >> 8);  /* PCI Configuration Address */
+	msg.tx_buf[4] = (u8)(address >> 16); /* PCI Configuration Address */
+	for (i = 0; i < pumsg->tx_len; i++)
+		msg.tx_buf[5 + i] = ((u8 *)&pumsg->value)[i];
+
+	/* Add an Assure Write Frame Check Sequence byte */
+	msg.tx_buf[5 + i] = 0x80 ^
+			    compute_aw_fcs((u8 *)&msg, 8 + pumsg->tx_len);
+
+	rc = xfer_msg_with_retries(priv, &msg, true);
+	if (rc || msg.rx_buf[0] != DEV_PECI_CC_SUCCESS) {
+		dev_dbg(priv->dev, "ioctl error, rc : %d\n", rc);
+		return -EIO;
+	}
+
+	return rc;
+}
+
+
+typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
+
+static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
+	ioctl_xfer_msg,
+	ioctl_ping,
+	ioctl_get_dib,
+	ioctl_get_temp,
+	ioctl_rd_pkg_cfg,
+	ioctl_wr_pkg_cfg,
+	ioctl_rd_ia_msr,
+	NULL, /* Reserved */
+	ioctl_rd_pci_cfg,
+	NULL, /* Reserved */
+	ioctl_rd_pci_cfg_local,
+	ioctl_wr_pci_cfg_local,
+};
+
+
+long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct aspeed_peci *priv;
+	long ret = 0;
+	void __user *argp = (void __user *)arg;
+	int timeout = PECI_IDLE_CHECK_TIMEOUT;
+	u8 msg[sizeof(struct peci_xfer_msg)];
+	unsigned int peci_cmd, msg_size;
+	u32 cmd_sts;
+
+	/*
+	 * Treat it as an inter module call when filp is null but only in case
+	 * the private data is initialized.
+	 */
+	if (filp)
+		priv = container_of(filp->private_data,
+				    struct aspeed_peci, miscdev);
+	else
+		priv = aspeed_peci_priv;
+
+	if (!priv)
+		return -ENXIO;
+
+	switch (cmd) {
+	case PECI_IOC_XFER:
+	case PECI_IOC_PING:
+	case PECI_IOC_GET_DIB:
+	case PECI_IOC_GET_TEMP:
+	case PECI_IOC_RD_PKG_CFG:
+	case PECI_IOC_WR_PKG_CFG:
+	case PECI_IOC_RD_IA_MSR:
+	case PECI_IOC_RD_PCI_CFG:
+	case PECI_IOC_RD_PCI_CFG_LOCAL:
+	case PECI_IOC_WR_PCI_CFG_LOCAL:
+		peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
+		msg_size = _IOC_SIZE(cmd);
+		break;
+
+	default:
+		dev_dbg(priv->dev, "Invalid ioctl cmd : 0x%08x\n", cmd);
+		return -EINVAL;
+	}
+
+	if (!peci_ioctl_fn[peci_cmd])
+		return -EPERM;
+
+	mutex_lock(&priv->mutex);
+
+	dev_dbg(priv->dev, "CMD : 0x%08x, peci_cmd : %d, msg_size : %d\n",
+		cmd, peci_cmd, msg_size);
+
+	/* Check command sts and bus idle state */
+	while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts)
+	       && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
+		if (timeout-- < 0) {
+			dev_dbg(priv->dev, "Timeout waiting for idle state!\n");
+			ret = -ETIME;
+			goto out;
+		}
+		usleep_range(10000, 11000);
+	};
+
+	if (msg_size &&
+	    (filp ? copy_from_user(&msg, argp, msg_size) :
+		    memcpy(&msg, (const void *)arg, msg_size) != &msg)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = peci_ioctl_fn[peci_cmd](priv, &msg);
+
+	if (ret == 0 && msg_size &&
+	    (filp ? copy_to_user(argp, &msg, msg_size) :
+		    memcpy((void *)arg, &msg, msg_size) != (void *)arg))
+		ret = -EFAULT;
+
+out:
+	mutex_unlock(&priv->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(peci_ioctl);
+
+static int aspeed_peci_open(struct inode *inode, struct file *filp)
+{
+	struct aspeed_peci *priv =
+		container_of(filp->private_data, struct aspeed_peci, miscdev);
+
+	atomic_inc(&priv->ref_count);
+
+	dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
+
+	return 0;
+}
+
+static int aspeed_peci_release(struct inode *inode, struct file *filp)
+{
+	struct aspeed_peci *priv =
+		container_of(filp->private_data, struct aspeed_peci, miscdev);
+
+	atomic_dec(&priv->ref_count);
+
+	dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
+
+	return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+	struct aspeed_peci *priv = arg;
+	bool valid_irq = true;
+
+	if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->sts))
+		return IRQ_NONE;
+
+	switch (priv->sts & PECI_INT_MASK) {
+	case PECI_INT_TIMEOUT:
+		dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n");
+		if (regmap_write(priv->regmap, AST_PECI_INT_STS,
+				 PECI_INT_TIMEOUT))
+			return IRQ_NONE;
+		break;
+	case PECI_INT_CONNECT:
+		dev_dbg(priv->dev, "PECI_INT_CONNECT\n");
+		if (regmap_write(priv->regmap, AST_PECI_INT_STS,
+			     PECI_INT_CONNECT))
+			return IRQ_NONE;
+		break;
+	case PECI_INT_W_FCS_BAD:
+		dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n");
+		if (regmap_write(priv->regmap, AST_PECI_INT_STS,
+			     PECI_INT_W_FCS_BAD))
+			return IRQ_NONE;
+		break;
+	case PECI_INT_W_FCS_ABORT:
+		dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n");
+		if (regmap_write(priv->regmap, AST_PECI_INT_STS,
+			     PECI_INT_W_FCS_ABORT))
+			return IRQ_NONE;
+		break;
+	case PECI_INT_CMD_DONE:
+		dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n");
+		if (regmap_write(priv->regmap, AST_PECI_INT_STS,
+			     PECI_INT_CMD_DONE) ||
+		    regmap_write(priv->regmap, AST_PECI_CMD, 0))
+			return IRQ_NONE;
+		break;
+	default:
+		dev_dbg(priv->dev, "Unknown PECI interrupt : 0x%08x\n",
+			priv->sts);
+		if (regmap_write(priv->regmap, AST_PECI_INT_STS, priv->sts))
+			return IRQ_NONE;
+		valid_irq = false;
+		break;
+	}
+
+	if (valid_irq)
+		complete(&priv->xfer_complete);
+
+	return IRQ_HANDLED;
+}
+
+static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
+{
+	struct clk *clkin;
+	u32 clk_freq, clk_divisor, clk_div_val = 0;
+	u32 msg_timing_nego, addr_timing_nego, rd_sampling_point;
+	int ret;
+
+	clkin = devm_clk_get(priv->dev, NULL);
+	if (IS_ERR(clkin)) {
+		dev_err(priv->dev, "Failed to get clk source.\n");
+		return PTR_ERR(clkin);
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "clock-frequency",
+				   &clk_freq);
+	if (ret < 0) {
+		dev_err(priv->dev,
+			"Could not read clock-frequency property.\n");
+		return ret;
+	}
+
+	clk_divisor = clk_get_rate(clkin) / clk_freq;
+	devm_clk_put(priv->dev, clkin);
+
+	while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX))
+		clk_div_val++;
+
+	ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego",
+				   &msg_timing_nego);
+	if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) {
+		dev_warn(priv->dev,
+			 "Invalid msg-timing-nego : %u, Use default : %u\n",
+			 msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT);
+		msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT;
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego",
+				   &addr_timing_nego);
+	if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) {
+		dev_warn(priv->dev,
+			 "Invalid addr-timing-nego : %u, Use default : %u\n",
+			 addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT);
+		addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT;
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point",
+				   &rd_sampling_point);
+	if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) {
+		dev_warn(priv->dev,
+			 "Invalid rd-sampling-point : %u. Use default : %u\n",
+			 rd_sampling_point,
+			 PECI_RD_SAMPLING_POINT_DEFAULT);
+		rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT;
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms",
+				   &priv->cmd_timeout_ms);
+	if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX ||
+		priv->cmd_timeout_ms == 0) {
+		dev_warn(priv->dev,
+			 "Invalid cmd-timeout-ms : %u. Use default : %u\n",
+			 priv->cmd_timeout_ms,
+			 PECI_CMD_TIMEOUT_MS_DEFAULT);
+		priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT;
+	}
+
+	ret = regmap_write(priv->regmap, AST_PECI_CTRL,
+			   PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) |
+			   PECI_CTRL_PECI_CLK_EN);
+	if (ret)
+		return ret;
+
+	usleep_range(1000, 5000);
+
+	/*
+	 * Timing negotiation period setting.
+	 * The unit of the programmed value is 4 times of PECI clock period.
+	 */
+	ret = regmap_write(priv->regmap, AST_PECI_TIMING,
+			   PECI_TIMING_MESSAGE(msg_timing_nego) |
+			   PECI_TIMING_ADDRESS(addr_timing_nego));
+	if (ret)
+		return ret;
+
+	/* Clear interrupts. */
+	ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK);
+	if (ret)
+		return ret;
+
+	/* Enable interrupts. */
+	ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK);
+	if (ret)
+		return ret;
+
+	/* Read sampling point and clock speed setting. */
+	ret = regmap_write(priv->regmap, AST_PECI_CTRL,
+			   PECI_CTRL_SAMPLING(rd_sampling_point) |
+			   PECI_CTRL_CLK_DIV(clk_div_val) |
+			   PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct regmap_config aspeed_peci_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = AST_PECI_R_DATA7,
+	.fast_io = true,
+};
+
+static const struct file_operations aspeed_peci_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.unlocked_ioctl = peci_ioctl,
+	.open = aspeed_peci_open,
+	.release = aspeed_peci_release,
+};
+
+static int __init aspeed_peci_probe(struct platform_device *pdev)
+{
+	struct aspeed_peci *priv;
+	struct device *dev;
+	struct resource *res;
+	void __iomem *base;
+	int ret = 0;
+
+	dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, priv);
+	priv->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->regmap = devm_regmap_init_mmio(dev, base,
+					     &aspeed_peci_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (!priv->irq)
+		return -ENODEV;
+
+	ret = devm_request_irq(dev, priv->irq, aspeed_peci_irq_handler,
+			       IRQF_SHARED,
+			       SOC_NAME "-" DEVICE_NAME "-irq",
+			       priv);
+	if (ret < 0)
+		return ret;
+
+	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
+	priv->miscdev.name = DEVICE_NAME;
+	priv->miscdev.parent = dev;
+	priv->miscdev.fops = &aspeed_peci_fops;
+
+	ret = misc_register(&priv->miscdev);
+	if (ret) {
+		dev_err(dev, "Failed to request interrupt.\n");
+		return ret;
+	}
+
+	mutex_init(&priv->mutex);
+	init_completion(&priv->xfer_complete);
+
+	crc8_populate_msb(aspeed_peci_crc8_table, PECI_CRC8_POLYNOMIAL);
+
+	ret = aspeed_peci_init_ctrl(priv);
+	if (ret < 0)
+		return ret;
+
+	aspeed_peci_priv = priv;
+
+	dev_info(dev, "peci registered, IRQ %d\n", priv->irq);
+
+	return 0;
+}
+
+static int aspeed_peci_remove(struct platform_device *pdev)
+{
+	struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
+
+	aspeed_peci_priv = NULL;
+	dev_set_drvdata(&pdev->dev, NULL);
+	misc_deregister(&priv->miscdev);
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_peci_of_table[] = {
+	{ .compatible = "aspeed,ast2400-peci", },
+	{ .compatible = "aspeed,ast2500-peci", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
+
+static struct platform_driver aspeed_peci_driver = {
+	.probe  = aspeed_peci_probe,
+	.remove = aspeed_peci_remove,
+	.driver = {
+		.name           = SOC_NAME "-" DEVICE_NAME,
+		.of_match_table = aspeed_peci_of_table,
+	},
+};
+module_platform_driver(aspeed_peci_driver);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
+MODULE_DESCRIPTION("Aspeed PECI driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/misc/peci.h b/include/misc/peci.h
new file mode 100644
index 0000000..66322c6
--- /dev/null
+++ b/include/misc/peci.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Intel Corporation
+
+#ifndef __PECI_H
+#define __PECI_H
+
+#include <linux/peci_ioctl.h>
+
+long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+
+#endif /* __PECI_H */
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index f330ba4..b34960b 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -327,6 +327,7 @@ header-y += packet_diag.h
 header-y += param.h
 header-y += parport.h
 header-y += patchkey.h
+header-y += peci_ioctl.h
 header-y += pci.h
 header-y += pci_regs.h
 header-y += perf_event.h
diff --git a/include/uapi/linux/peci_ioctl.h b/include/uapi/linux/peci_ioctl.h
new file mode 100644
index 0000000..8386848
--- /dev/null
+++ b/include/uapi/linux/peci_ioctl.h
@@ -0,0 +1,270 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Intel Corporation
+
+#ifndef __PECI_IOCTL_H
+#define __PECI_IOCTL_H
+
+#include <linux/ioctl.h>
+
+/* Base Address of 48d */
+#define PECI_BASE_ADDR  0x30  /* The PECI client's default address of 0x30 */
+#define PECI_OFFSET_MAX 8     /* Max numver of CPU clients */
+
+/* PCI Access */
+#define MAX_PCI_READ_LEN 24  /* Number of bytes of the PCI Space read */
+
+#define PCI_BUS0_CPU0      0x00
+#define PCI_BUS0_CPU1      0x80
+#define PCI_CPUBUSNO_BUS   0x00
+#define PCI_CPUBUSNO_DEV   0x08
+#define PCI_CPUBUSNO_FUNC  0x02
+#define PCI_CPUBUSNO       0xcc
+#define PCI_CPUBUSNO_1     0xd0
+#define PCI_CPUBUSNO_VALID 0xd4
+
+/* Package Identifier Read Parameter Value */
+#define PKG_ID_CPU_ID               0x0000  /* 0 - CPUID Info */
+#define PKG_ID_PLATFORM_ID          0x0001  /* 1 - Platform ID */
+#define PKG_ID_UNCORE_ID            0x0002  /* 2 - Uncore Device ID */
+#define PKG_ID_MAX_THREAD_ID        0x0003  /* 3 - Max Thread ID */
+#define PKG_ID_MICROCODE_REV        0x0004  /* 4 - CPU Microcode Update Revision */
+#define PKG_ID_MACHINE_CHECK_STATUS 0x0005  /* 5 - Machine Check Status */
+
+/* RdPkgConfig Index */
+#define MBX_INDEX_CPU_ID            0   /* Package Identifier Read */
+#define MBX_INDEX_VR_DEBUG          1   /* VR Debug */
+#define MBX_INDEX_PKG_TEMP_READ     2   /* Package Temperature Read */
+#define MBX_INDEX_ENERGY_COUNTER    3   /* Energy counter */
+#define MBX_INDEX_ENERGY_STATUS     4   /* DDR Energy Status */
+#define MBX_INDEX_WAKE_MODE_BIT     5   /* "Wake on PECI" Mode bit */
+#define MBX_INDEX_EPI               6   /* Efficient Performance Indication */
+#define MBX_INDEX_PKG_RAPL_PERF     8   /* Package RAPL Performance Status Read */
+#define MBX_INDEX_PER_CORE_DTS_TEMP 9   /* Per Core DTS Temperature Read */
+#define MBX_INDEX_DTS_MARGIN        10  /* DTS thermal margin */
+#define MBX_INDEX_SKT_PWR_THRTL_DUR 11  /* Socket Power Throttled Duration */
+#define MBX_INDEX_CFG_TDP_CONTROL   12  /* TDP Config Control */
+#define MBX_INDEX_CFG_TDP_LEVELS    13  /* TDP Config Levels */
+#define MBX_INDEX_DDR_DIMM_TEMP     14  /* DDR DIMM Temperature */
+#define MBX_INDEX_CFG_ICCMAX        15  /* Configurable ICCMAX */
+#define MBX_INDEX_TEMP_TARGET       16  /* Temperature Target Read */
+#define MBX_INDEX_CURR_CFG_LIMIT    17  /* Current Config Limit */
+#define MBX_INDEX_DIMM_TEMP_READ    20  /* Package Thermal Status Read */
+#define MBX_INDEX_DRAM_IMC_TMP_READ 22  /* DRAM IMC Temperature Read */
+#define MBX_INDEX_DDR_CH_THERM_STAT 23  /* DDR Channel Thermal Status */
+#define MBX_INDEX_PKG_POWER_LIMIT1  26  /* Package Power Limit1 */
+#define MBX_INDEX_PKG_POWER_LIMIT2  27  /* Package Power Limit2 */
+#define MBX_INDEX_TDP               28  /* Thermal design power minimum */
+#define MBX_INDEX_TDP_HIGH          29  /* Thermal design power maximum */
+#define MBX_INDEX_TDP_UNITS         30  /* Units for power and energy registers */
+#define MBX_INDEX_RUN_TIME          31  /* Accumulated Run Time */
+#define MBX_INDEX_CONSTRAINED_TIME  32  /* Thermally Constrained Time Read */
+#define MBX_INDEX_TURBO_RATIO       33  /* Turbo Activation Ratio */
+#define MBX_INDEX_DDR_RAPL_PL1      34  /* DDR RAPL PL1 */
+#define MBX_INDEX_DDR_PWR_INFO_HIGH 35  /* DRAM Power Info Read (high) */
+#define MBX_INDEX_DDR_PWR_INFO_LOW  36  /* DRAM Power Info Read (low) */
+#define MBX_INDEX_DDR_RAPL_PL2      37  /* DDR RAPL PL2 */
+#define MBX_INDEX_DDR_RAPL_STATUS   38  /* DDR RAPL Performance Status */
+#define MBX_INDEX_DDR_HOT_ABSOLUTE  43  /* DDR Hottest Dimm Absolute Temperature */
+#define MBX_INDEX_DDR_HOT_RELATIVE  44  /* DDR Hottest Dimm Relative Temperature */
+#define MBX_INDEX_DDR_THROTTLE_TIME 45  /* DDR Throttle Time */
+#define MBX_INDEX_DDR_THERM_STATUS  46  /* DDR Thermal Status */
+#define MBX_INDEX_TIME_AVG_TEMP     47  /* Package time-averaged temperature */
+#define MBX_INDEX_TURBO_RATIO_LIMIT 49  /* Turbo Ratio Limit Read */
+#define MBX_INDEX_HWP_AUTO_OOB      53  /* HWP Autonomous Out-of-band */
+#define MBX_INDEX_DDR_WARM_BUDGET   55  /* DDR Warm Power Budget */
+#define MBX_INDEX_DDR_HOT_BUDGET    56  /* DDR Hot Power Budget */
+#define MBX_INDEX_PKG_PSYS_PWR_LIM3 57  /* Package/Psys Power Limit3 */
+#define MBX_INDEX_PKG_PSYS_PWR_LIM1 58  /* Package/Psys Power Limit1 */
+#define MBX_INDEX_PKG_PSYS_PWR_LIM2 59  /* Package/Psys Power Limit2 */
+#define MBX_INDEX_PKG_PSYS_PWR_LIM4 60  /* Package/Psys Power Limit4 */
+#define MBX_INDEX_PERF_LIMIT_REASON 65  /* Performance Limit Reasons */
+
+/* WrPkgConfig Index */
+#define MBX_INDEX_DIMM_ABIENT 19
+#define MBX_INDEX_DIMM_TEMP   24
+
+/* Device Specific Completion Code (CC) Definition */
+#define DEV_PECI_CC_RETRY_ERR_MASK  0xf0
+#define DEV_PECI_CC_SUCCESS         0x40
+#define DEV_PECI_CC_TIMEOUT         0x80
+#define DEV_PECI_CC_OUT_OF_RESOURCE 0x81
+#define DEV_PECI_CC_INVALID_REQ     0x90
+
+/* Skylake EDS says to retry for 250ms */
+#define DEV_PECI_RETRY_ATTEMPTS 25
+#define DEV_PECI_RETRY_DELAY_MS 10
+#define DEV_PECI_RETRY_BIT      0x01
+
+#define GET_TEMP_WR_LEN   1
+#define GET_TEMP_RD_LEN   2
+#define GET_TEMP_PECI_CMD 0x01
+
+#define GET_DIB_WR_LEN   1
+#define GET_DIB_RD_LEN   8
+#define GET_DIB_PECI_CMD 0xf7
+
+#define RDPKGCFG_WRITE_LEN     5
+#define RDPKGCFG_READ_LEN_BASE 1
+#define RDPKGCFG_PECI_CMD      0xa1
+
+#define WRPKGCFG_WRITE_LEN_BASE 6
+#define WRPKGCFG_READ_LEN       1
+#define WRPKGCFG_PECI_CMD       0xa5
+
+#define RDIAMSR_WRITE_LEN 5
+#define RDIAMSR_READ_LEN  9
+#define RDIAMSR_PECI_CMD  0xb1
+
+#define WRIAMSR_PECI_CMD  0xb5
+
+#define RDPCICFG_WRITE_LEN 6
+#define RDPCICFG_READ_LEN  5
+#define RDPCICFG_PECI_CMD  0x61
+
+#define WRPCICFG_PECI_CMD  0x65
+
+#define RDPCICFGLOCAL_WRITE_LEN     5
+#define RDPCICFGLOCAL_READ_LEN_BASE 1
+#define RDPCICFGLOCAL_PECI_CMD      0xe1
+
+#define WRPCICFGLOCAL_WRITE_LEN_BASE 6
+#define WRPCICFGLOCAL_READ_LEN       1
+#define WRPCICFGLOCAL_PECI_CMD       0xe5
+
+enum PECI_CMD {
+	PECI_CMD_XFER = 0,
+	PECI_CMD_PING,
+	PECI_CMD_GET_DIB,
+	PECI_CMD_GET_TEMP,
+	PECI_CMD_RD_PKG_CFG,
+	PECI_CMD_WR_PKG_CFG,
+	PECI_CMD_RD_IA_MSR,
+	PECI_CMD_WR_IA_MSR,
+	PECI_CMD_RD_PCI_CFG,
+	PECI_CMD_WR_PCI_CFG,
+	PECI_CMD_RD_PCI_CFG_LOCAL,
+	PECI_CMD_WR_PCI_CFG_LOCAL,
+	PECI_CMD_MAX,
+};
+
+#define MAX_BUFFER_SIZE  32
+
+#pragma pack(push, 1)
+struct peci_xfer_msg {
+	unsigned char client_addr;
+	unsigned char tx_len;
+	unsigned char rx_len;
+	unsigned char tx_buf[MAX_BUFFER_SIZE];
+	unsigned char rx_buf[MAX_BUFFER_SIZE];
+};
+#pragma pack(pop)
+
+struct peci_ping_msg {
+	unsigned char target;
+};
+
+struct peci_get_dib_msg {
+	unsigned char target;
+	unsigned int  dib;
+};
+
+struct peci_get_temp_msg {
+	unsigned char target;
+	signed short  temp_raw;
+};
+
+struct peci_rd_pkg_cfg_msg {
+	unsigned char target;
+	unsigned char index;
+	unsigned short param;
+	unsigned char rx_len;
+	unsigned char pkg_config[4];
+};
+
+struct peci_wr_pkg_cfg_msg {
+	unsigned char target;
+	unsigned char index;
+	unsigned short param;
+	unsigned char tx_len;
+	unsigned int value;
+};
+
+struct peci_rd_ia_msr_msg {
+	unsigned char target;
+	unsigned char thread_id;
+	unsigned short address;
+	unsigned long value;
+};
+
+struct peci_rd_pci_cfg_msg {
+	unsigned char target;
+	unsigned char bus;
+	unsigned char device;
+	unsigned char function;
+	unsigned short reg;
+	unsigned char pci_config[4];
+};
+
+struct peci_rd_pci_cfg_local_msg {
+	unsigned char target;
+	unsigned char bus;
+	unsigned char device;
+	unsigned char function;
+	unsigned short reg;
+	unsigned char rx_len;
+	unsigned char pci_config[4];
+};
+
+struct peci_wr_pci_cfg_local_msg {
+	unsigned char target;
+	unsigned char bus;
+	unsigned char device;
+	unsigned char function;
+	unsigned short reg;
+	unsigned char tx_len;
+	unsigned int value;
+};
+
+#define PECI_IOC_BASE  'P'
+
+#define PECI_IOC_XFER \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_XFER, 0, \
+		struct peci_xfer_msg)
+
+#define PECI_IOC_PING \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_PING, 0, \
+		struct peci_ping_msg)
+
+#define PECI_IOC_GET_DIB \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_GET_DIB, 0, \
+		struct peci_get_dib_msg)
+
+#define PECI_IOC_GET_TEMP \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_GET_TEMP, 0, \
+		struct peci_get_temp_msg)
+
+#define PECI_IOC_RD_PKG_CFG \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_RD_PKG_CFG, 0, \
+		struct peci_rd_pkg_cfg_msg)
+
+#define PECI_IOC_WR_PKG_CFG \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_WR_PKG_CFG, 0, \
+		struct peci_wr_pkg_cfg_msg)
+
+#define PECI_IOC_RD_IA_MSR \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_RD_IA_MSR, 0, \
+		struct peci_rd_ia_msr_msg)
+
+#define PECI_IOC_RD_PCI_CFG \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG, 0, \
+		struct peci_rd_pci_cfg_msg)
+
+#define PECI_IOC_RD_PCI_CFG_LOCAL \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG_LOCAL, 0, \
+		struct peci_rd_pci_cfg_local_msg)
+
+#define PECI_IOC_WR_PCI_CFG_LOCAL \
+	_IOWR(PECI_IOC_BASE + PECI_CMD_WR_PCI_CFG_LOCAL, 0, \
+		struct peci_wr_pci_cfg_local_msg)
+
+#endif /* __PECI_IOCTL_H */
-- 
2.7.4

[PATCH linux dev-4.10 4/6] Documentation: dt-bindings: Add a generic PECI hwmon

[Jae Hyun Yoo]

This commit add a dt-bindings document for a generic PECI hwmon
driver.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 .../devicetree/bindings/hwmon/peci-hwmon.txt       | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/peci-hwmon.txt

diff --git a/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
new file mode 100644
index 0000000..20b86f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
@@ -0,0 +1,33 @@
+* Generic PECI (Platform Environment Control Interface) hwmon driver.
+
+Dependency:
+- This driver uses a PECI misc driver as a controller interface so one of PECI
+  misc drivers which provides compatible ioctls has to be enabled.
+
+Required properties:
+- compatible: "peci-hwmon"
+- cpu-id: Should contain CPU socket ID
+	- 0 ~ 7
+
+Optional properties:
+- show-core: If this protperty is defined, core tmeperature attrubites will be
+	     enumerated.
+- dimm-nums: Should contain the number of DIMM slots that attached to each CPU
+	     which is indicated by cpu-id.
+	0 ~ 16 (default: 16)
+	     In case of 0, DIMM temperature attrubites will not be enumerated.
+
+Example:
+	peci-hwmon0 {
+		compatible = "peci-hwmon";
+		cpu-id = <0>;
+		show-core;
+		dimm-nums = <16>;
+	};
+
+	peci-hwmon1 {
+		compatible = "peci-hwmon";
+		cpu-id = <1>;
+		show-core;
+		dimm-nums = <16>;
+	};
-- 
2.7.4

[PATCH linux dev-4.10 5/6] Documentation: hwmon: Add a generic PECI hwmon

[Jae Hyun Yoo]

From: Jae Hyun Yoo <jae.hyun.yoo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This commit add a document for a generic PECI hwmon driver.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 Documentation/hwmon/peci-hwmon | 74 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/hwmon/peci-hwmon

diff --git a/Documentation/hwmon/peci-hwmon b/Documentation/hwmon/peci-hwmon
new file mode 100644
index 0000000..e0155b5
--- /dev/null
+++ b/Documentation/hwmon/peci-hwmon
@@ -0,0 +1,74 @@
+Kernel driver peci-hwmon
+===============================
+
+Supported chips:
+	Generic BMC chips provide PECI controller
+
+Author:
+	Jae Hyun Yoo <jae.hyun.yoo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
+
+
+Hardware Interfaces
+-------------------
+
+This driver uses a PECI misc driver as a controller interface so one of PECI
+misc drivers which provides compatible ioctls has to be enabled.
+
+
+Description
+-----------
+
+This driver implements a generic PECI hwmon feature which is running with a PECI
+controller driver supports native PECI Client Command Suite for retrieving
+temperatures of the CPU package, CPU cores and DIMM components.
+
+All temperature values are given in millidegree Celsius and will be measurable
+only when the target CPU is powered on.
+
+
+sysfs files
+-----------
+
+temp1_input		Provides current die temperature of the CPU package.
+temp1_max		Provides thermal control temperature of the CPU package
+			which is also known as Tcontrol.
+temp1_crit		Provides shutdown temperature of the CPU package which
+			is also known as the maximum processor junction
+			temperature, Tjmax or Tprochot.
+temp1_crit_hyst		Provides the hysteresis value from Tcontrol to Tjmax of
+			the CPU package.
+
+temp2_input		Provides current DTS thermal margin to Tcontrol of the
+			CPU package. Value 0 means it reaches to Tcontrol
+			temperature. Sub-zero value means the die temperature
+			goes across Tconrtol to Tjmax.
+temp2_min		Provides the minimum DTS thermal margin to Tcontrol of
+			the CPU package.
+temp2_lcrit		Provides the value when the CPU package temperature
+			reaches to Tjmax.
+
+temp3_input		Provides current Tcontrol temperature of the CPU
+			package which is also known as Fan Temperature target.
+			Indicates the relative value from thermal monitor trip
+			temperature at which fans should be engaged.
+temp3_crit		Provides Tcontrol critical value of the CPU package
+			which is same to Tjmax.
+
+temp4_input		Provides current Tthrottle temperature of the CPU
+			package. Used for throttling temperature. If this value
+			is allowed and lower than Tjmax - the throttle will
+			occur and reported at lower than Tjmax.
+
+temp[100-127]_input	Provides current core temperature.
+temp[100-127]_max	Provides thermal control temperature of the core.
+temp[100-127]_crit	Provides shutdown temperature of the core.
+temp[100-127]_crit_hyst	Provides the hysteresis value from Tcontrol to Tjmax of
+			the core.
+
+Note:
+	Core temperature group will be appeared when probing the driver if CPU
+	is online or when the first reading on other attr happens because it
+	needs cpu info reading. The number of generated core attrs depends on
+	the number of cores of the cpu package.
+
+temp[200-215]_input	Provides current temperature of the DDR DIMM.
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Jae Hyun Yoo]

This commit adds driver implementation for a generic PECI hwmon.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 drivers/hwmon/Kconfig      |   6 +
 drivers/hwmon/Makefile     |   1 +
 drivers/hwmon/peci-hwmon.c | 953 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 960 insertions(+)
 create mode 100644 drivers/hwmon/peci-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 9256dd0..3a62c60 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1234,6 +1234,12 @@ config SENSORS_NCT7904
 	  This driver can also be built as a module.  If so, the module
 	  will be called nct7904.
 
+config SENSORS_PECI_HWMON
+	tristate "PECI hwmon support"
+	depends on ASPEED_PECI
+	help
+	  If you say yes here you get support for the generic PECI hwmon driver.
+
 config SENSORS_NSA320
 	tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors"
 	depends on GPIOLIB && OF
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 98000fc..41d43a5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
 obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
 obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
 obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
+obj-$(CONFIG_SENSORS_PECI_HWMON)	+= peci-hwmon.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
diff --git a/drivers/hwmon/peci-hwmon.c b/drivers/hwmon/peci-hwmon.c
new file mode 100644
index 0000000..2d2a288
--- /dev/null
+++ b/drivers/hwmon/peci-hwmon.c
@@ -0,0 +1,953 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Intel Corporation
+
+#include <linux/delay.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/syscalls.h>
+#include <misc/peci.h>
+
+#define DEVICE_NAME "peci-hwmon"
+#define HWMON_NAME "peci_hwmon"
+
+#define CPU_ID_MAX           8   /* Max CPU number configured by socket ID */
+#define DIMM_NUMS_MAX        16  /* Max DIMM numbers (channel ranks x 2) */
+#define CORE_NUMS_MAX        28  /* Max core numbers (max on SKX Platinum) */
+#define TEMP_TYPE_PECI       6   /* Sensor type 6: Intel PECI */
+#define CORE_INDEX_OFFSET    100 /* sysfs filename start offset for core temp */
+#define DIMM_INDEX_OFFSET    200 /* sysfs filename start offset for DIMM temp */
+#define TEMP_NAME_HEADER_LEN 4   /* sysfs temp type header length */
+#define OF_DIMM_NUMS_DEFAULT 16  /* default dimm-nums setting */
+
+#define CORE_TEMP_ATTRS      5
+#define DIMM_TEMP_ATTRS      2
+#define ATTR_NAME_LEN        24
+
+#define UPDATE_INTERVAL_MIN  HZ
+
+enum sign_t {
+	POS,
+	NEG
+};
+
+struct cpuinfo_t {
+	bool valid;
+	u32  dib;
+	u8   cpuid;
+	u8   platform_id;
+	u32  microcode;
+	u8   logical_thread_nums;
+};
+
+struct temp_data_t {
+	bool valid;
+	s32  value;
+	unsigned long last_updated;
+};
+
+struct temp_group_t {
+	struct temp_data_t tjmax;
+	struct temp_data_t tcontrol;
+	struct temp_data_t tthrottle;
+	struct temp_data_t dts_margin;
+	struct temp_data_t die;
+	struct temp_data_t core[CORE_NUMS_MAX];
+	struct temp_data_t dimm[DIMM_NUMS_MAX];
+};
+
+struct core_temp_attr_group_t {
+	struct sensor_device_attribute sd_attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS];
+	char attr_name[CORE_NUMS_MAX][CORE_TEMP_ATTRS][ATTR_NAME_LEN];
+	struct attribute *attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS + 1];
+	struct attribute_group attr_group[CORE_NUMS_MAX];
+};
+
+struct dimm_temp_attr_group_t {
+	struct sensor_device_attribute sd_attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS];
+	char attr_name[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
+	struct attribute *attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS + 1];
+	struct attribute_group attr_group[DIMM_NUMS_MAX];
+};
+
+struct peci_hwmon {
+	struct device *dev;
+	struct device *hwmon_dev;
+	char name[NAME_MAX];
+	const struct attribute_group **groups;
+	struct cpuinfo_t cpuinfo;
+	struct temp_group_t temp;
+	u32 cpu_id;
+	bool show_core;
+	u32 core_nums;
+	u32 dimm_nums;
+	atomic_t core_group_created;
+	struct core_temp_attr_group_t core;
+	struct dimm_temp_attr_group_t dimm;
+};
+
+enum label_t {
+	L_DIE,
+	L_DTS,
+	L_TCONTROL,
+	L_TTHROTTLE,
+	L_MAX
+};
+
+static const char *peci_label[L_MAX] = {
+	"Die temperature\n",
+	"DTS thermal margin to Tcontrol\n",
+	"Tcontrol temperature\n",
+	"Tthrottle temperature\n",
+};
+
+static DEFINE_MUTEX(peci_hwmon_lock);
+
+static int create_core_temp_group(struct peci_hwmon *priv, int core_no);
+
+
+static int xfer_peci_msg(int cmd, void *pmsg)
+{
+	int rc;
+
+	mutex_lock(&peci_hwmon_lock);
+	rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
+	mutex_unlock(&peci_hwmon_lock);
+
+	return rc;
+}
+
+static int get_cpuinfo(struct peci_hwmon *priv)
+{
+	struct peci_get_dib_msg dib_msg;
+	struct peci_rd_pkg_cfg_msg cfg_msg;
+	int rc, i;
+
+	if (!priv->cpuinfo.valid) {
+		dib_msg.target = PECI_BASE_ADDR + priv->cpu_id;
+
+		rc = xfer_peci_msg(PECI_IOC_GET_DIB, (void *)&dib_msg);
+		if (rc < 0)
+			return rc;
+
+		priv->cpuinfo.dib = dib_msg.dib;
+
+		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
+		cfg_msg.index = MBX_INDEX_CPU_ID;
+		cfg_msg.param = 0;
+		cfg_msg.rx_len = 4;
+
+		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
+		if (rc < 0)
+			return rc;
+
+		priv->cpuinfo.cpuid = cfg_msg.pkg_config[0];
+
+		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
+		cfg_msg.index = MBX_INDEX_CPU_ID;
+		cfg_msg.param = 1;
+		cfg_msg.rx_len = 4;
+
+		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
+		if (rc < 0)
+			return rc;
+
+		priv->cpuinfo.platform_id = cfg_msg.pkg_config[0];
+
+		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
+		cfg_msg.index = MBX_INDEX_CPU_ID;
+		cfg_msg.param = 3;
+		cfg_msg.rx_len = 4;
+
+		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
+		if (rc < 0)
+			return rc;
+
+		priv->cpuinfo.logical_thread_nums = cfg_msg.pkg_config[0] + 1;
+
+		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
+		cfg_msg.index = MBX_INDEX_CPU_ID;
+		cfg_msg.param = 4;
+		cfg_msg.rx_len = 4;
+
+		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
+		if (rc < 0)
+			return rc;
+
+		priv->cpuinfo.microcode = (cfg_msg.pkg_config[3] << 24) |
+					  (cfg_msg.pkg_config[2] << 16) |
+					  (cfg_msg.pkg_config[1] << 8) |
+					  cfg_msg.pkg_config[0];
+
+		priv->core_nums = priv->cpuinfo.logical_thread_nums / 2;
+
+		if (priv->show_core &&
+		    atomic_inc_return(&priv->core_group_created) == 1) {
+			for (i = 0; i < priv->core_nums; i++) {
+				rc = create_core_temp_group(priv, i);
+				if (rc != 0) {
+					dev_err(priv->dev,
+						"Failed to create core temp group\n");
+					for (--i; i >= 0; i--) {
+						sysfs_remove_group(
+						     &priv->hwmon_dev->kobj,
+						     &priv->core.attr_group[i]);
+					}
+					atomic_set(&priv->core_group_created,
+						   0);
+					return rc;
+				}
+			}
+		}
+
+		priv->cpuinfo.valid = true;
+	}
+
+	return 0;
+}
+
+static int get_tjmax(struct peci_hwmon *priv)
+{
+	struct peci_rd_pkg_cfg_msg msg;
+	int rc;
+
+	rc = get_cpuinfo(priv);
+	if (rc < 0)
+		return rc;
+
+	if (!priv->temp.tjmax.valid) {
+		msg.target = PECI_BASE_ADDR + priv->cpu_id;
+		msg.index = MBX_INDEX_TEMP_TARGET;
+		msg.param = 0;
+		msg.rx_len = 4;
+
+		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
+		if (rc < 0)
+			return rc;
+
+		priv->temp.tjmax.value = (s32)msg.pkg_config[2] * 1000;
+		priv->temp.tjmax.valid = true;
+	}
+
+	return 0;
+}
+
+static int get_tcontrol(struct peci_hwmon *priv)
+{
+	struct peci_rd_pkg_cfg_msg msg;
+	s32 tcontrol_margin;
+	int rc;
+
+	if (priv->temp.tcontrol.valid &&
+	    time_before(jiffies, priv->temp.tcontrol.last_updated +
+				 UPDATE_INTERVAL_MIN))
+		return 0;
+
+	rc = get_tjmax(priv);
+	if (rc < 0)
+		return rc;
+
+	msg.target = PECI_BASE_ADDR + priv->cpu_id;
+	msg.index = MBX_INDEX_TEMP_TARGET;
+	msg.param = 0;
+	msg.rx_len = 4;
+
+	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
+	if (rc < 0)
+		return rc;
+
+	tcontrol_margin = msg.pkg_config[1];
+	tcontrol_margin = ((tcontrol_margin ^ 0x80) - 0x80) * 1000;
+
+	priv->temp.tcontrol.value = priv->temp.tjmax.value - tcontrol_margin;
+
+	if (!priv->temp.tcontrol.valid) {
+		priv->temp.tcontrol.last_updated = INITIAL_JIFFIES;
+		priv->temp.tcontrol.valid = true;
+	} else {
+		priv->temp.tcontrol.last_updated = jiffies;
+	}
+
+	return 0;
+}
+
+static int get_tthrottle(struct peci_hwmon *priv)
+{
+	struct peci_rd_pkg_cfg_msg msg;
+	s32 tthrottle_offset;
+	int rc;
+
+	if (priv->temp.tthrottle.valid &&
+	    time_before(jiffies, priv->temp.tthrottle.last_updated +
+				 UPDATE_INTERVAL_MIN))
+		return 0;
+
+	rc = get_tjmax(priv);
+	if (rc < 0)
+		return rc;
+
+	msg.target = PECI_BASE_ADDR + priv->cpu_id;
+	msg.index = MBX_INDEX_TEMP_TARGET;
+	msg.param = 0;
+	msg.rx_len = 4;
+
+	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
+	if (rc < 0)
+		return rc;
+
+	tthrottle_offset = (msg.pkg_config[3] & 0x2f) * 1000;
+	priv->temp.tthrottle.value = priv->temp.tjmax.value - tthrottle_offset;
+
+	if (!priv->temp.tthrottle.valid) {
+		priv->temp.tthrottle.last_updated = INITIAL_JIFFIES;
+		priv->temp.tthrottle.valid = true;
+	} else {
+		priv->temp.tthrottle.last_updated = jiffies;
+	}
+
+	return 0;
+}
+
+static int get_die_temp(struct peci_hwmon *priv)
+{
+	struct peci_get_temp_msg msg;
+	int rc;
+
+	if (priv->temp.die.valid &&
+	    time_before(jiffies, priv->temp.die.last_updated +
+				 UPDATE_INTERVAL_MIN))
+		return 0;
+
+	rc = get_tjmax(priv);
+	if (rc < 0)
+		return rc;
+
+	msg.target = PECI_BASE_ADDR + priv->cpu_id;
+
+	rc = xfer_peci_msg(PECI_IOC_GET_TEMP, (void *)&msg);
+	if (rc < 0)
+		return rc;
+
+	priv->temp.die.value = priv->temp.tjmax.value +
+			       ((s32)msg.temp_raw * 1000 / 64);
+
+	if (!priv->temp.die.valid) {
+		priv->temp.die.last_updated = INITIAL_JIFFIES;
+		priv->temp.die.valid = true;
+	} else {
+		priv->temp.die.last_updated = jiffies;
+	}
+
+	return 0;
+}
+
+static int get_dts_margin(struct peci_hwmon *priv)
+{
+	struct peci_rd_pkg_cfg_msg msg;
+	s32 dts_margin;
+	int rc;
+
+	if (priv->temp.dts_margin.valid &&
+	    time_before(jiffies, priv->temp.dts_margin.last_updated +
+				 UPDATE_INTERVAL_MIN))
+		return 0;
+
+	rc = get_cpuinfo(priv);
+	if (rc < 0)
+		return rc;
+
+	msg.target = PECI_BASE_ADDR + priv->cpu_id;
+	msg.index = MBX_INDEX_DTS_MARGIN;
+	msg.param = 0;
+	msg.rx_len = 4;
+
+	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
+	if (rc < 0)
+		return rc;
+
+	dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
+
+	/*
+	 * Processors return a value of DTS reading in 10.6 format
+	 * (10 bits signed decimal, 6 bits fractional).
+	 * Error codes:
+	 *   0x8000: General sensor error
+	 *   0x8001: Reserved
+	 *   0x8002: Underflow on reading value
+	 *   0x8003-0x81ff: Reserved
+	 */
+	if (dts_margin >= 0x8000 && dts_margin <= 0x81ff)
+		return -1;
+
+	dts_margin = ((dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
+
+	priv->temp.dts_margin.value = dts_margin;
+
+	if (!priv->temp.dts_margin.valid) {
+		priv->temp.dts_margin.last_updated = INITIAL_JIFFIES;
+		priv->temp.dts_margin.valid = true;
+	} else {
+		priv->temp.dts_margin.last_updated = jiffies;
+	}
+
+	return 0;
+}
+
+static int get_core_temp(struct peci_hwmon *priv, int core_index)
+{
+	struct peci_rd_pkg_cfg_msg msg;
+	s32 core_dts_margin;
+	int rc;
+
+	if (priv->temp.core[core_index].valid &&
+	    time_before(jiffies, priv->temp.core[core_index].last_updated +
+				 UPDATE_INTERVAL_MIN))
+		return 0;
+
+	rc = get_tjmax(priv);
+	if (rc < 0)
+		return rc;
+
+	msg.target = PECI_BASE_ADDR + priv->cpu_id;
+	msg.index = MBX_INDEX_PER_CORE_DTS_TEMP;
+	msg.param = core_index;
+	msg.rx_len = 4;
+
+	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
+	if (rc < 0)
+		return rc;
+
+	core_dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
+
+	/*
+	 * Processors return a value of the core DTS reading in 10.6 format
+	 * (10 bits signed decimal, 6 bits fractional).
+	 * Error codes:
+	 *   0x8000: General sensor error
+	 *   0x8001: Reserved
+	 *   0x8002: Underflow on reading value
+	 *   0x8003-0x81ff: Reserved
+	 */
+	if (core_dts_margin >= 0x8000 && core_dts_margin <= 0x81ff)
+		return -1;
+
+	core_dts_margin = ((core_dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
+
+	priv->temp.core[core_index].value = priv->temp.tjmax.value +
+					    core_dts_margin;
+
+	if (!priv->temp.core[core_index].valid) {
+		priv->temp.core[core_index].last_updated = INITIAL_JIFFIES;
+		priv->temp.core[core_index].valid = true;
+	} else {
+		priv->temp.core[core_index].last_updated = jiffies;
+	}
+
+	return 0;
+}
+
+static int get_dimm_temp(struct peci_hwmon *priv, int dimm_index)
+{
+	struct peci_rd_pkg_cfg_msg msg;
+	int channel_rank = dimm_index / 2;
+	int dimm_order = dimm_index % 2;
+	int rc;
+
+	if (priv->temp.core[dimm_index].valid &&
+	    time_before(jiffies, priv->temp.core[dimm_index].last_updated +
+				 UPDATE_INTERVAL_MIN))
+		return 0;
+
+	rc = get_cpuinfo(priv);
+	if (rc < 0)
+		return rc;
+
+	msg.target = PECI_BASE_ADDR + priv->cpu_id;
+	msg.index = MBX_INDEX_DDR_DIMM_TEMP;
+	msg.param = channel_rank;
+	msg.rx_len = 4;
+
+	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
+	if (rc < 0)
+		return rc;
+
+	priv->temp.dimm[dimm_index].value = msg.pkg_config[dimm_order] * 1000;
+
+	if (!priv->temp.dimm[dimm_index].valid) {
+		priv->temp.dimm[dimm_index].last_updated = INITIAL_JIFFIES;
+		priv->temp.dimm[dimm_index].valid = true;
+	} else {
+		priv->temp.dimm[dimm_index].last_updated = jiffies;
+	}
+
+	return 0;
+}
+
+static ssize_t show_info(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	int rc;
+
+	rc = get_cpuinfo(priv);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "dib         : 0x%08x\n"
+			    "cpuid       : 0x%x\n"
+			    "platform id : %d\n"
+			    "stepping    : %d\n"
+			    "microcode   : 0x%08x\n"
+			    "logical thread nums : %d\n",
+			    priv->cpuinfo.dib,
+			    priv->cpuinfo.cpuid,
+			    priv->cpuinfo.platform_id,
+			    priv->cpuinfo.cpuid & 0xf,
+			    priv->cpuinfo.microcode,
+			    priv->cpuinfo.logical_thread_nums);
+}
+
+static ssize_t show_tcontrol(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	int rc;
+
+	rc = get_tcontrol(priv);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", priv->temp.tcontrol.value);
+}
+
+static ssize_t show_tcontrol_margin(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	int rc;
+
+	rc = get_tcontrol(priv);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", sensor_attr->index == POS ?
+				    priv->temp.tjmax.value -
+				    priv->temp.tcontrol.value :
+				    priv->temp.tcontrol.value -
+				    priv->temp.tjmax.value);
+}
+
+static ssize_t show_tthrottle(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	int rc;
+
+	rc = get_tthrottle(priv);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", priv->temp.tthrottle.value);
+}
+
+static ssize_t show_tjmax(struct device *dev,
+			  struct device_attribute *attr,
+			  char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	int rc;
+
+	rc = get_tjmax(priv);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", priv->temp.tjmax.value);
+}
+
+static ssize_t show_die_temp(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	int rc;
+
+	rc = get_die_temp(priv);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", priv->temp.die.value);
+}
+
+static ssize_t show_dts_therm_margin(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	int rc;
+
+	rc = get_dts_margin(priv);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", priv->temp.dts_margin.value);
+}
+
+static ssize_t show_core_temp(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	int core_index = sensor_attr->index;
+	int rc;
+
+	rc = get_core_temp(priv, core_index);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", priv->temp.core[core_index].value);
+}
+
+static ssize_t show_dimm_temp(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(dev);
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	int dimm_index = sensor_attr->index;
+	int rc;
+
+	rc = get_dimm_temp(priv, dimm_index);
+	if (rc < 0)
+		return rc;
+
+	return sprintf(buf, "%d\n", priv->temp.dimm[dimm_index].value);
+}
+
+static ssize_t show_value(struct device *dev,
+			  struct device_attribute *attr,
+			  char *buf)
+{
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+
+	return sprintf(buf, "%d\n", sensor_attr->index);
+}
+
+static ssize_t show_label(struct device *dev,
+			  struct device_attribute *attr,
+			  char *buf)
+{
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+
+	return sprintf(buf, peci_label[sensor_attr->index]);
+}
+
+static ssize_t show_core_label(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+
+	return sprintf(buf, "Core #%d temperature\n", sensor_attr->index);
+}
+
+static ssize_t show_dimm_label(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+
+	char channel = 'A' + (sensor_attr->index / 2);
+	int index = sensor_attr->index % 2;
+
+	return sprintf(buf, "Channel Rank %c DDR DIMM #%d temperature\n",
+		       channel, index);
+}
+
+/* Die temperature */
+static SENSOR_DEVICE_ATTR(temp1_label, 0444, show_label, NULL, L_DIE);
+static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_die_temp, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_max, 0444, show_tcontrol, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit, 0444, show_tjmax, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0444, show_tcontrol_margin, NULL,
+			  POS);
+
+static struct attribute *die_temp_attrs[] = {
+	&sensor_dev_attr_temp1_label.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group die_temp_attr_group = {
+	.attrs = die_temp_attrs,
+};
+
+/* DTS thermal margin temperature */
+static SENSOR_DEVICE_ATTR(temp2_label, 0444, show_label, NULL, L_DTS);
+static SENSOR_DEVICE_ATTR(temp2_input, 0444, show_dts_therm_margin, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp2_min, 0444, show_value, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp2_lcrit, 0444, show_tcontrol_margin, NULL, NEG);
+
+static struct attribute *dts_margin_temp_attrs[] = {
+	&sensor_dev_attr_temp2_label.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_lcrit.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group dts_margin_temp_attr_group = {
+	.attrs = dts_margin_temp_attrs,
+};
+
+/* Tcontrol temperature */
+static SENSOR_DEVICE_ATTR(temp3_label, 0444, show_label, NULL, L_TCONTROL);
+static SENSOR_DEVICE_ATTR(temp3_input, 0444, show_tcontrol, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp3_crit, 0444, show_tjmax, NULL, 0);
+
+static struct attribute *tcontrol_temp_attrs[] = {
+	&sensor_dev_attr_temp3_label.dev_attr.attr,
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group tcontrol_temp_attr_group = {
+	.attrs = tcontrol_temp_attrs,
+};
+
+/* Tthrottle temperature */
+static SENSOR_DEVICE_ATTR(temp4_label, 0444, show_label, NULL, L_TTHROTTLE);
+static SENSOR_DEVICE_ATTR(temp4_input, 0444, show_tthrottle, NULL, 0);
+
+static struct attribute *tthrottle_temp_attrs[] = {
+	&sensor_dev_attr_temp4_label.dev_attr.attr,
+	&sensor_dev_attr_temp4_input.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group tthrottle_temp_attr_group = {
+	.attrs = tthrottle_temp_attrs,
+};
+
+/* CPU info */
+static SENSOR_DEVICE_ATTR(info, 0444, show_info, NULL, 0);
+
+static struct attribute *info_attrs[] = {
+	&sensor_dev_attr_info.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group info_attr_group = {
+	.attrs = info_attrs,
+};
+
+const struct attribute_group *peci_hwmon_attr_groups[] = {
+	&info_attr_group,
+	&die_temp_attr_group,
+	&dts_margin_temp_attr_group,
+	&tcontrol_temp_attr_group,
+	&tthrottle_temp_attr_group,
+	NULL
+};
+
+static ssize_t (*const core_show_fn[CORE_TEMP_ATTRS]) (struct device *dev,
+		struct device_attribute *devattr, char *buf) = {
+	show_core_label,
+	show_core_temp,
+	show_tcontrol,
+	show_tjmax,
+	show_tcontrol_margin,
+};
+
+static const char *const core_suffix[CORE_TEMP_ATTRS] = {
+	"label",
+	"input",
+	"max",
+	"crit",
+	"crit_hyst",
+};
+
+static int create_core_temp_group(struct peci_hwmon *priv, int core_no)
+{
+	int i;
+
+	for (i = 0; i < CORE_TEMP_ATTRS; i++) {
+		snprintf(priv->core.attr_name[core_no][i],
+			 ATTR_NAME_LEN, "temp%d_%s",
+			 CORE_INDEX_OFFSET + core_no, core_suffix[i]);
+		sysfs_attr_init(
+			    &priv->core.sd_attrs[core_no][i].dev_attr.attr);
+		priv->core.sd_attrs[core_no][i].dev_attr.attr.name =
+					       priv->core.attr_name[core_no][i];
+		priv->core.sd_attrs[core_no][i].dev_attr.attr.mode = 0444;
+		priv->core.sd_attrs[core_no][i].dev_attr.show = core_show_fn[i];
+		if (i == 0 || i == 1) /* label or temp */
+			priv->core.sd_attrs[core_no][i].index = core_no;
+		priv->core.attrs[core_no][i] =
+				 &priv->core.sd_attrs[core_no][i].dev_attr.attr;
+	}
+
+	priv->core.attr_group[core_no].attrs = priv->core.attrs[core_no];
+
+	return sysfs_create_group(&priv->hwmon_dev->kobj,
+				  &priv->core.attr_group[core_no]);
+}
+
+static ssize_t (*const dimm_show_fn[DIMM_TEMP_ATTRS]) (struct device *dev,
+		struct device_attribute *devattr, char *buf) = {
+	show_dimm_label,
+	show_dimm_temp,
+};
+
+static const char *const dimm_suffix[DIMM_TEMP_ATTRS] = {
+	"label",
+	"input",
+};
+
+static int create_dimm_temp_group(struct peci_hwmon *priv, int dimm_no)
+{
+	int i;
+
+	for (i = 0; i < DIMM_TEMP_ATTRS; i++) {
+		snprintf(priv->dimm.attr_name[dimm_no][i],
+			 ATTR_NAME_LEN, "temp%d_%s",
+			 DIMM_INDEX_OFFSET + dimm_no, dimm_suffix[i]);
+		sysfs_attr_init(&priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr);
+		priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.name =
+					       priv->dimm.attr_name[dimm_no][i];
+		priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.mode = 0444;
+		priv->dimm.sd_attrs[dimm_no][i].dev_attr.show = dimm_show_fn[i];
+		priv->dimm.sd_attrs[dimm_no][i].index = dimm_no;
+		priv->dimm.attrs[dimm_no][i] =
+				 &priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr;
+	}
+
+	priv->dimm.attr_group[dimm_no].attrs = priv->dimm.attrs[dimm_no];
+
+	return sysfs_create_group(&priv->hwmon_dev->kobj,
+				  &priv->dimm.attr_group[dimm_no]);
+}
+
+static int peci_hwmon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct peci_hwmon *priv;
+	struct device *hwmon;
+	int rc, i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, priv);
+	priv->dev = dev;
+
+	rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);
+	if (rc || priv->cpu_id >= CPU_ID_MAX) {
+		dev_err(dev, "Invalid cpu-id configuration\n");
+		return rc;
+	}
+
+	rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);
+	if (rc || priv->dimm_nums > DIMM_NUMS_MAX) {
+		dev_warn(dev, "Invalid dimm-nums : %u. Use default : %u\n",
+			 priv->dimm_nums, OF_DIMM_NUMS_DEFAULT);
+		priv->dimm_nums = OF_DIMM_NUMS_DEFAULT;
+	}
+
+	priv->show_core = of_property_read_bool(np, "show-core");
+
+	priv->groups = peci_hwmon_attr_groups;
+
+	snprintf(priv->name, NAME_MAX, HWMON_NAME ".cpu%d", priv->cpu_id);
+
+	hwmon = devm_hwmon_device_register_with_groups(dev,
+						       priv->name,
+						       priv, priv->groups);
+
+	rc = PTR_ERR_OR_ZERO(hwmon);
+	if (rc != 0) {
+		dev_err(dev, "Failed to register peci hwmon\n");
+		return rc;
+	}
+
+	priv->hwmon_dev = hwmon;
+
+	for (i = 0; i < priv->dimm_nums; i++) {
+		rc = create_dimm_temp_group(priv, i);
+		if (rc != 0) {
+			dev_err(dev, "Failed to create dimm temp group\n");
+			for (--i; i >= 0; i--) {
+				sysfs_remove_group(&priv->hwmon_dev->kobj,
+						   &priv->dimm.attr_group[i]);
+			}
+			return rc;
+		}
+	}
+
+	/*
+	 * Try to create core temp group now. It will be created if CPU is
+	 * curretnly online or it will be created after the first reading of
+	 * cpuinfo from the online CPU otherwise.
+	 */
+	if (priv->show_core)
+		(void) get_cpuinfo(priv);
+
+	dev_info(dev, "peci hwmon for CPU#%d registered\n", priv->cpu_id);
+
+	return rc;
+}
+
+static int peci_hwmon_remove(struct platform_device *pdev)
+{
+	struct peci_hwmon *priv = dev_get_drvdata(&pdev->dev);
+	int i;
+
+	if (atomic_read(&priv->core_group_created))
+		for (i = 0; i < priv->core_nums; i++) {
+			sysfs_remove_group(&priv->hwmon_dev->kobj,
+					   &priv->core.attr_group[i]);
+		}
+
+	for (i = 0; i < priv->dimm_nums; i++) {
+		sysfs_remove_group(&priv->hwmon_dev->kobj,
+				   &priv->dimm.attr_group[i]);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id peci_of_table[] = {
+	{ .compatible = "peci-hwmon", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, peci_of_table);
+
+static struct platform_driver peci_hwmon_driver = {
+	.probe = peci_hwmon_probe,
+	.remove = peci_hwmon_remove,
+	.driver = {
+		.name           = DEVICE_NAME,
+		.of_match_table = peci_of_table,
+	},
+};
+
+module_platform_driver(peci_hwmon_driver);
+MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
+MODULE_DESCRIPTION("PECI hwmon driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Greg KH]

On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
> From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
> 
> Hello,
> 
> This patch set provides support for PECI of AST2400/2500 which can give us PECI
> functionalities such as temperature monitoring, platform manageability,
> processor diagnostics and failure analysis. Also provides generic peci.h and
> peci_ioctl.h headers to provide compatibility to peci drivers that can be
> implemented later e.g. Nuvoton's BMC SoC family.

What is the "dev-4.10" in the subject for?  4.10 is really old and
obsolete :(

thanks,

greg k-h

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Greg KH]

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for Aspeed PECI. Also adds
> generic peci.h and peci_ioctl.h files to provide compatibility
> to peci drivers that can be implemented later e.g. Nuvoton's BMC
> SoC family.

We don't add code that could be used "sometime in the future".  Only
include stuff that we use now.

Please fix up this series based on that and resubmit.  There should not
be any need for any uapi file then, right?

thanks,

greg k-h

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Greg KH]

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> +#pragma pack(push, 1)
> +struct peci_xfer_msg {
> +	unsigned char client_addr;
> +	unsigned char tx_len;
> +	unsigned char rx_len;
> +	unsigned char tx_buf[MAX_BUFFER_SIZE];
> +	unsigned char rx_buf[MAX_BUFFER_SIZE];
> +};
> +#pragma pack(pop)

For any structure that crosses the user/kernel boundry, you _HAVE_ to
use the "__" variant.  So for here you would use __u8 instead of
"unsigned char" in order for things to work properly.

I'm guessing you didn't test this all out on a mixed 32/64 bit system?

Please fix up and test to ensure that it all works properly before
resubmitting.

thanks,

greg k-h

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Arnd Bergmann]

On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
> This commit adds driver implementation for Aspeed PECI. Also adds
> generic peci.h and peci_ioctl.h files to provide compatibility
> to peci drivers that can be implemented later e.g. Nuvoton's BMC
> SoC family.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

> +#include <linux/clk.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/peci_ioctl.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/semaphore.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>

semaphore.h is not used here and can be dropped.

> +static struct aspeed_peci *aspeed_peci_priv;

Try to avoid instance variables like this one. You should always be able to find
that pointer from whatever structure you were called with.


> +       timeout = wait_for_completion_interruptible_timeout(
> +                                       &priv->xfer_complete,
> +                                       msecs_to_jiffies(priv->cmd_timeout_ms));
> +
> +       dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
> +       if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
> +               dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
> +                       PECI_CMD_STS_GET(peci_state));
> +       else
> +               dev_dbg(priv->dev, "PECI_STATE : read error\n");
> +
> +       if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
> +               if (timeout <= 0) {
> +                       dev_dbg(priv->dev, "Timeout waiting for a response!\n");
> +                       rc = -ETIME;
> +               } else {
> +                       dev_dbg(priv->dev, "No valid response!\n");
> +                       rc = -EFAULT;
> +               }
> +               return rc;
> +       }

You don't seem to handle -ERESTARTSYS correct here. Either do it
right, or drop the _interruptible part above.

> +typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
> +
> +static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
> +       ioctl_xfer_msg,
> +       ioctl_ping,
> +       ioctl_get_dib,
> +       ioctl_get_temp,
> +       ioctl_rd_pkg_cfg,
> +       ioctl_wr_pkg_cfg,
> +       ioctl_rd_ia_msr,
> +       NULL, /* Reserved */
> +       ioctl_rd_pci_cfg,
> +       NULL, /* Reserved */
> +       ioctl_rd_pci_cfg_local,
> +       ioctl_wr_pci_cfg_local,
> +};
> +
> +
> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +       struct aspeed_peci *priv;
> +       long ret = 0;
> +       void __user *argp = (void __user *)arg;
> +       int timeout = PECI_IDLE_CHECK_TIMEOUT;
> +       u8 msg[sizeof(struct peci_xfer_msg)];
> +       unsigned int peci_cmd, msg_size;
> +       u32 cmd_sts;
> +
> +       /*
> +        * Treat it as an inter module call when filp is null but only in case
> +        * the private data is initialized.
> +        */
> +       if (filp)
> +               priv = container_of(filp->private_data,
> +                                   struct aspeed_peci, miscdev);
> +       else
> +               priv = aspeed_peci_priv;

Drop this.

> +       if (!priv)
> +               return -ENXIO;
> +
> +       switch (cmd) {
> +       case PECI_IOC_XFER:
> +       case PECI_IOC_PING:
> +       case PECI_IOC_GET_DIB:
> +       case PECI_IOC_GET_TEMP:
> +       case PECI_IOC_RD_PKG_CFG:
> +       case PECI_IOC_WR_PKG_CFG:
> +       case PECI_IOC_RD_IA_MSR:
> +       case PECI_IOC_RD_PCI_CFG:
> +       case PECI_IOC_RD_PCI_CFG_LOCAL:
> +       case PECI_IOC_WR_PCI_CFG_LOCAL:
> +               peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
> +               msg_size = _IOC_SIZE(cmd);
> +               break;

Having to keep the switch() statement and the array above seems a
little fragile. Can you just do one or the other?

Regarding the command set, you have both a low-level PECI_IOC_XFER
interface and a high-level interface. Can you explain why? I'd think that
generally speaking it's better to have only one of the two.

> +       /* Check command sts and bus idle state */
> +       while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts)
> +              && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
> +               if (timeout-- < 0) {
> +                       dev_dbg(priv->dev, "Timeout waiting for idle state!\n");
> +                       ret = -ETIME;
> +                       goto out;
> +               }
> +               usleep_range(10000, 11000);
> +       };

To implement timeout, it's better to replace the counter with a
jiffies/time_before or ktime_get()/ktime_before() check, since usleep_range()
is might sleep considerably longer than expected.

> +EXPORT_SYMBOL_GPL(peci_ioctl);

No user of this, so drop it.

> +static int aspeed_peci_open(struct inode *inode, struct file *filp)
> +{
> +       struct aspeed_peci *priv =
> +               container_of(filp->private_data, struct aspeed_peci, miscdev);
> +
> +       atomic_inc(&priv->ref_count);
> +
> +       dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
> +
> +       return 0;
> +}
> +
> +static int aspeed_peci_release(struct inode *inode, struct file *filp)
> +{
> +       struct aspeed_peci *priv =
> +               container_of(filp->private_data, struct aspeed_peci, miscdev);
> +
> +       atomic_dec(&priv->ref_count);
> +
> +       dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
> +
> +       return 0;
> +}

Nothing uses that reference count, drop it.

> new file mode 100644
> index 0000000..66322c6
> --- /dev/null
> +++ b/include/misc/peci.h
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017 Intel Corporation
> +
> +#ifndef __PECI_H
> +#define __PECI_H
> +
> +#include <linux/peci_ioctl.h>
> +
> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> +
> +#endif /* __PECI_H */

Not used anywhere.

> diff --git a/include/uapi/linux/peci_ioctl.h b/include/uapi/linux/peci_ioctl.h
> new file mode 100644
> index 0000000..8386848
> --- /dev/null
> +++ b/include/uapi/linux/peci_ioctl.h
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017 Intel Corporation
> +
> +#ifndef __PECI_IOCTL_H
> +#define __PECI_IOCTL_H
> +
> +#include <linux/ioctl.h>
> +
> +/* Base Address of 48d */
> +#define PECI_BASE_ADDR  0x30  /* The PECI client's default address of 0x30 */
> +#define PECI_OFFSET_MAX 8     /* Max numver of CPU clients */
> +
> +/* PCI Access */
> +#define MAX_PCI_READ_LEN 24  /* Number of bytes of the PCI Space read */
> +
> +#define PCI_BUS0_CPU0      0x00
> +#define PCI_BUS0_CPU1      0x80
> +#define PCI_CPUBUSNO_BUS   0x00
> +#define PCI_CPUBUSNO_DEV   0x08
> +#define PCI_CPUBUSNO_FUNC  0x02
> +#define PCI_CPUBUSNO       0xcc
> +#define PCI_CPUBUSNO_1     0xd0
> +#define PCI_CPUBUSNO_VALID 0xd4

I can't tell for sure, but this file seems to be mixing the kernel API with
hardware specific macros that are not needed in user space. Can you move
some of this file into the driver itself?

This might go back to the previous question about the high-level and
low-level interfaces: if you can drop the low-level ioctl interface, more
of this header can become private to the driver.

> +/* Package Identifier Read Parameter Value */
> +#define PKG_ID_CPU_ID               0x0000  /* 0 - CPUID Info */
> +#define PKG_ID_PLATFORM_ID          0x0001  /* 1 - Platform ID */
> +#define PKG_ID_UNCORE_ID            0x0002  /* 2 - Uncore Device ID */
> +#define PKG_ID_MAX_THREAD_ID        0x0003  /* 3 - Max Thread ID */
> +#define PKG_ID_MICROCODE_REV        0x0004  /* 4 - CPU Microcode Update Revision */
> +#define PKG_ID_MACHINE_CHECK_STATUS 0x0005  /* 5 - Machine Check Status */
> +
> +/* RdPkgConfig Index */
> +#define MBX_INDEX_CPU_ID            0   /* Package Identifier Read */
> +#define MBX_INDEX_VR_DEBUG          1   /* VR Debug */
> +#define MBX_INDEX_PKG_TEMP_READ     2   /* Package Temperature Read */
> +#define MBX_INDEX_ENERGY_COUNTER    3   /* Energy counter */
> +#define MBX_INDEX_ENERGY_STATUS     4   /* DDR Energy Status */
> +#define MBX_INDEX_WAKE_MODE_BIT     5   /* "Wake on PECI" Mode bit */
> +#define MBX_INDEX_EPI               6   /* Efficient Performance Indication */

Who defines these constants? Are they specific to the aspeed BMC, to the HECI
protocol, or to a particular version of the remote endpoint?

> +#pragma pack(push, 1)
> +struct peci_xfer_msg {
> +       unsigned char client_addr;
> +       unsigned char tx_len;
> +       unsigned char rx_len;
> +       unsigned char tx_buf[MAX_BUFFER_SIZE];
> +       unsigned char rx_buf[MAX_BUFFER_SIZE];
> +};
> +#pragma pack(pop)
> +
> +struct peci_ping_msg {
> +       unsigned char target;
> +};
> +
> +struct peci_get_dib_msg {
> +       unsigned char target;
> +       unsigned int  dib;
> +};
> +
> +struct peci_get_temp_msg {
> +       unsigned char target;
> +       signed short  temp_raw;
> +};

Aside from what Greg already said about the types, please be careful to
also avoid implicit padding in the API data structures, including the end of the
structure.

> +#define PECI_IOC_RD_PCI_CFG \
> +       _IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG, 0, \
> +               struct peci_rd_pci_cfg_msg)
> +
> +#define PECI_IOC_RD_PCI_CFG_LOCAL \
> +       _IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG_LOCAL, 0, \
> +               struct peci_rd_pci_cfg_local_msg)
> +
> +#define PECI_IOC_WR_PCI_CFG_LOCAL \
> +       _IOWR(PECI_IOC_BASE + PECI_CMD_WR_PCI_CFG_LOCAL, 0, \
> +               struct peci_wr_pci_cfg_local_msg)

Can you give some background on what these do? In particular, who
is configuring whose PCI devices?

        Arnd

Re: [PATCH linux dev-4.10 4/6] Documentation: dt-bindings: Add a generic PECI hwmon

[Arnd Bergmann]

On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
> This commit add a dt-bindings document for a generic PECI hwmon
> driver.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>  .../devicetree/bindings/hwmon/peci-hwmon.txt       | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
>
> diff --git a/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
> new file mode 100644
> index 0000000..20b86f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
> @@ -0,0 +1,33 @@
> +* Generic PECI (Platform Environment Control Interface) hwmon driver.
> +
> +Dependency:
> +- This driver uses a PECI misc driver as a controller interface so one of PECI
> +  misc drivers which provides compatible ioctls has to be enabled.

The binding should only explain how we describe the hardware in an
operating-system
independent way, but not talk about how an OS is supposed to implement those
drivers.

Having multiple drivers each provide an exported function is not possible
in Linux: it immediately breaks building an 'allyesconfig' kernel, and prevents
you from running the same kernel across multiple implementations, so that
has to be redesigned anyway.

> +Required properties:
> +- compatible: "peci-hwmon"
> +- cpu-id: Should contain CPU socket ID
> +       - 0 ~ 7
> +
> +Optional properties:
> +- show-core: If this protperty is defined, core tmeperature attrubites will be

s/protperty/property/
s/tmeperature/temperature/
s/attrubites/attributes/

> +            enumerated.
> +- dimm-nums: Should contain the number of DIMM slots that attached to each CPU
> +            which is indicated by cpu-id.
> +       0 ~ 16 (default: 16)
> +            In case of 0, DIMM temperature attrubites will not be enumerated.

Is this only an initial list that you expect to be extended in the
future, or is this
a complete list of sensors that can ever be connected to PECI?

Should this be PECI version specific?

      Arnd

Re: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Arnd Bergmann]

On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
> This commit adds driver implementation for a generic PECI hwmon.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

> +static int xfer_peci_msg(int cmd, void *pmsg)
> +{
> +       int rc;
> +
> +       mutex_lock(&peci_hwmon_lock);
> +       rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
> +       mutex_unlock(&peci_hwmon_lock);
> +
> +       return rc;
> +}

I said earlier that peci_ioctl() looked unused, that was obviously
wrong, but what you have here
is not a proper way to abstract a bus.

Maybe this can be done more like an i2c bus: make the peci controller
a bus device
and register all known target/index pairs as devices with the peci bus
type, and have
them probed from DT. The driver can then bind to each of those individually.
Not sure if that is getting to granular at that point, I'd have to
understand better
how it is expected to get used, and what the variances are between
implementations.

       Arnd

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Jae Hyun Yoo]

On 1/10/2018 2:17 AM, Greg KH wrote:
> On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
>> From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
>>
>> Hello,
>>
>> This patch set provides support for PECI of AST2400/2500 which can give us PECI
>> functionalities such as temperature monitoring, platform manageability,
>> processor diagnostics and failure analysis. Also provides generic peci.h and
>> peci_ioctl.h headers to provide compatibility to peci drivers that can be
>> implemented later e.g. Nuvoton's BMC SoC family.
> 
> What is the "dev-4.10" in the subject for?  4.10 is really old and
> obsolete :(
> 
> thanks,
> 
> greg k-h
> 

I made this patch set on top of the v4.10 which OpenBmc project is 
currently using. I'll rebase this patch set onto the current kernel.org 
mainline.

Thanks,
Jae

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Greg KH]

On Wed, Jan 10, 2018 at 11:14:34AM -0800, Jae Hyun Yoo wrote:
> On 1/10/2018 2:17 AM, Greg KH wrote:
> > On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
> > > From: Jae Hyun Yoo <jae.hyun.yoo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > 
> > > Hello,
> > > 
> > > This patch set provides support for PECI of AST2400/2500 which can give us PECI
> > > functionalities such as temperature monitoring, platform manageability,
> > > processor diagnostics and failure analysis. Also provides generic peci.h and
> > > peci_ioctl.h headers to provide compatibility to peci drivers that can be
> > > implemented later e.g. Nuvoton's BMC SoC family.
> > 
> > What is the "dev-4.10" in the subject for?  4.10 is really old and
> > obsolete :(
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> I made this patch set on top of the v4.10 which OpenBmc project is currently
> using. I'll rebase this patch set onto the current kernel.org mainline.

What is "OpenBmc", and why are they using an obsolete and insecure
kernel for their project?  That seems like a very foolish thing to do...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Jae Hyun Yoo]

On 1/10/2018 11:17 AM, Greg KH wrote:
> On Wed, Jan 10, 2018 at 11:14:34AM -0800, Jae Hyun Yoo wrote:
>> On 1/10/2018 2:17 AM, Greg KH wrote:
>>> On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
>>>> From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
>>>>
>>>> Hello,
>>>>
>>>> This patch set provides support for PECI of AST2400/2500 which can give us PECI
>>>> functionalities such as temperature monitoring, platform manageability,
>>>> processor diagnostics and failure analysis. Also provides generic peci.h and
>>>> peci_ioctl.h headers to provide compatibility to peci drivers that can be
>>>> implemented later e.g. Nuvoton's BMC SoC family.
>>>
>>> What is the "dev-4.10" in the subject for?  4.10 is really old and
>>> obsolete :(
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>>
>> I made this patch set on top of the v4.10 which OpenBmc project is currently
>> using. I'll rebase this patch set onto the current kernel.org mainline.
> 
> What is "OpenBmc", and why are they using an obsolete and insecure
> kernel for their project?  That seems like a very foolish thing to do...
> 
> thanks,
> 
> greg k-h
> 

OpenBmc is an open source project to create a highly extensible 
framework for BMC (Board Management Controller) software for data-center 
computer systems:
https://github.com/openbmc

Its current mainline is v4.10 but it is being kept upgrading so it will 
be upgraded to the latest stable or long-term version soon.

Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 2:18 AM, Greg KH wrote:
> On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
>> This commit adds driver implementation for Aspeed PECI. Also adds
>> generic peci.h and peci_ioctl.h files to provide compatibility
>> to peci drivers that can be implemented later e.g. Nuvoton's BMC
>> SoC family.
> 
> We don't add code that could be used "sometime in the future".  Only
> include stuff that we use now.
> 
> Please fix up this series based on that and resubmit.  There should not
> be any need for any uapi file then, right?
> 
> thanks,
> 
> greg k-h
> 

These header files are being used in this patch set as well. I meant, 
these files also can be used for the future implementation to provide 
compatibility. I will update the commit message.

Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 2:20 AM, Greg KH wrote:
> On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
>> +#pragma pack(push, 1)
>> +struct peci_xfer_msg {
>> +	unsigned char client_addr;
>> +	unsigned char tx_len;
>> +	unsigned char rx_len;
>> +	unsigned char tx_buf[MAX_BUFFER_SIZE];
>> +	unsigned char rx_buf[MAX_BUFFER_SIZE];
>> +};
>> +#pragma pack(pop)
> 
> For any structure that crosses the user/kernel boundry, you _HAVE_ to
> use the "__" variant.  So for here you would use __u8 instead of
> "unsigned char" in order for things to work properly.
> 
> I'm guessing you didn't test this all out on a mixed 32/64 bit system?
> 
> Please fix up and test to ensure that it all works properly before
> resubmitting.
> 
> thanks,
> 
> greg k-h
> 

Thanks for your pointing it out. I'll fix this.

Thanks a lot,
Jae

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Greg KH]

On Wed, Jan 10, 2018 at 11:30:05AM -0800, Jae Hyun Yoo wrote:
> On 1/10/2018 11:17 AM, Greg KH wrote:
> > On Wed, Jan 10, 2018 at 11:14:34AM -0800, Jae Hyun Yoo wrote:
> > > On 1/10/2018 2:17 AM, Greg KH wrote:
> > > > On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
> > > > > From: Jae Hyun Yoo <jae.hyun.yoo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > This patch set provides support for PECI of AST2400/2500 which can give us PECI
> > > > > functionalities such as temperature monitoring, platform manageability,
> > > > > processor diagnostics and failure analysis. Also provides generic peci.h and
> > > > > peci_ioctl.h headers to provide compatibility to peci drivers that can be
> > > > > implemented later e.g. Nuvoton's BMC SoC family.
> > > > 
> > > > What is the "dev-4.10" in the subject for?  4.10 is really old and
> > > > obsolete :(
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > > 
> > > 
> > > I made this patch set on top of the v4.10 which OpenBmc project is currently
> > > using. I'll rebase this patch set onto the current kernel.org mainline.
> > 
> > What is "OpenBmc", and why are they using an obsolete and insecure
> > kernel for their project?  That seems like a very foolish thing to do...
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> OpenBmc is an open source project to create a highly extensible framework
> for BMC (Board Management Controller) software for data-center computer
> systems:
> https://github.com/openbmc
> 
> Its current mainline is v4.10 but it is being kept upgrading so it will be
> upgraded to the latest stable or long-term version soon.

Why hasn't it been updated in the year since 4.10 was released?  That's
a _very_ long time to be running on a totally insecure kernel, and no
new development should ever be done on old kernels, that's even crazier
(as we can't go back in time and accept patches for new features to old
releases...)

It sounds like the openbmc project needs to learn how to manage their
kernels a whole lot better, who do I need to go poke about this?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Jae Hyun Yoo]

On 1/10/2018 12:27 PM, Greg KH wrote:
> On Wed, Jan 10, 2018 at 11:30:05AM -0800, Jae Hyun Yoo wrote:
>> On 1/10/2018 11:17 AM, Greg KH wrote:
>>> On Wed, Jan 10, 2018 at 11:14:34AM -0800, Jae Hyun Yoo wrote:
>>>> On 1/10/2018 2:17 AM, Greg KH wrote:
>>>>> On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
>>>>>> From: Jae Hyun Yoo <jae.hyun.yoo-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> This patch set provides support for PECI of AST2400/2500 which can give us PECI
>>>>>> functionalities such as temperature monitoring, platform manageability,
>>>>>> processor diagnostics and failure analysis. Also provides generic peci.h and
>>>>>> peci_ioctl.h headers to provide compatibility to peci drivers that can be
>>>>>> implemented later e.g. Nuvoton's BMC SoC family.
>>>>>
>>>>> What is the "dev-4.10" in the subject for?  4.10 is really old and
>>>>> obsolete :(
>>>>>
>>>>> thanks,
>>>>>
>>>>> greg k-h
>>>>>
>>>>
>>>> I made this patch set on top of the v4.10 which OpenBmc project is currently
>>>> using. I'll rebase this patch set onto the current kernel.org mainline.
>>>
>>> What is "OpenBmc", and why are they using an obsolete and insecure
>>> kernel for their project?  That seems like a very foolish thing to do...
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>>
>> OpenBmc is an open source project to create a highly extensible framework
>> for BMC (Board Management Controller) software for data-center computer
>> systems:
>> https://github.com/openbmc
>>
>> Its current mainline is v4.10 but it is being kept upgrading so it will be
>> upgraded to the latest stable or long-term version soon.
> 
> Why hasn't it been updated in the year since 4.10 was released?  That's
> a _very_ long time to be running on a totally insecure kernel, and no
> new development should ever be done on old kernels, that's even crazier
> (as we can't go back in time and accept patches for new features to old
> releases...)
> 

Thanks for your pointing it out and I totally agree with you. Actually, 
we are preparing 4.13 update for now and an another update will be 
followed up. As I answered above, I'll rebase this patch set onto the 
latest kernel.org mainline. Sorry for my misunderstanding of upstream 
process.

> It sounds like the openbmc project needs to learn how to manage their
> kernels a whole lot better, who do I need to go poke about this?
>  > thanks,
> 
> greg k-h
> 

I've already cc'ed openbmc developers so they are also seeing this 
thread. Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org> is the openbmc kernel maintainer.

Thanks,
Jae
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Guenter Roeck]

On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for a generic PECI hwmon.
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>  drivers/hwmon/Kconfig      |   6 +
>  drivers/hwmon/Makefile     |   1 +
>  drivers/hwmon/peci-hwmon.c | 953 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 960 insertions(+)
>  create mode 100644 drivers/hwmon/peci-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 9256dd0..3a62c60 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1234,6 +1234,12 @@ config SENSORS_NCT7904
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called nct7904.
>  
> +config SENSORS_PECI_HWMON
> +	tristate "PECI hwmon support"
> +	depends on ASPEED_PECI
> +	help
> +	  If you say yes here you get support for the generic PECI hwmon driver.
> +
>  config SENSORS_NSA320
>  	tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors"
>  	depends on GPIOLIB && OF
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 98000fc..41d43a5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -131,6 +131,7 @@ obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
>  obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
>  obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
>  obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
> +obj-$(CONFIG_SENSORS_PECI_HWMON)	+= peci-hwmon.o
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> diff --git a/drivers/hwmon/peci-hwmon.c b/drivers/hwmon/peci-hwmon.c
> new file mode 100644
> index 0000000..2d2a288
> --- /dev/null
> +++ b/drivers/hwmon/peci-hwmon.c
> @@ -0,0 +1,953 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017 Intel Corporation
> +
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/syscalls.h>
> +#include <misc/peci.h>

misc, not linux ? That seems wrong.

> +
> +#define DEVICE_NAME "peci-hwmon"
> +#define HWMON_NAME "peci_hwmon"
> +
> +#define CPU_ID_MAX           8   /* Max CPU number configured by socket ID */
> +#define DIMM_NUMS_MAX        16  /* Max DIMM numbers (channel ranks x 2) */
> +#define CORE_NUMS_MAX        28  /* Max core numbers (max on SKX Platinum) */

I won't insist, but it would be better if this were dynamic,
otherwise we'll end up having to increase the defines in the future.

> +#define TEMP_TYPE_PECI       6   /* Sensor type 6: Intel PECI */
> +#define CORE_INDEX_OFFSET    100 /* sysfs filename start offset for core temp */
> +#define DIMM_INDEX_OFFSET    200 /* sysfs filename start offset for DIMM temp */

Did you test with the "sensors" command to ensure that this works,
with the large gaps in index values ?

Overall, I am not very happy with the indexing. Since each sensor as
a label, it might be better to just make it dynamic.

> +#define TEMP_NAME_HEADER_LEN 4   /* sysfs temp type header length */
> +#define OF_DIMM_NUMS_DEFAULT 16  /* default dimm-nums setting */
> +
> +#define CORE_TEMP_ATTRS      5
> +#define DIMM_TEMP_ATTRS      2
> +#define ATTR_NAME_LEN        24
> +
> +#define UPDATE_INTERVAL_MIN  HZ
> +
> +enum sign_t {
> +	POS,
> +	NEG
> +};
> +
> +struct cpuinfo_t {
> +	bool valid;
> +	u32  dib;
> +	u8   cpuid;
> +	u8   platform_id;
> +	u32  microcode;
> +	u8   logical_thread_nums;
> +};
> +
> +struct temp_data_t {
> +	bool valid;
> +	s32  value;
> +	unsigned long last_updated;
> +};
> +
> +struct temp_group_t {
> +	struct temp_data_t tjmax;
> +	struct temp_data_t tcontrol;
> +	struct temp_data_t tthrottle;
> +	struct temp_data_t dts_margin;
> +	struct temp_data_t die;
> +	struct temp_data_t core[CORE_NUMS_MAX];
> +	struct temp_data_t dimm[DIMM_NUMS_MAX];
> +};
> +
> +struct core_temp_attr_group_t {
> +	struct sensor_device_attribute sd_attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS];
> +	char attr_name[CORE_NUMS_MAX][CORE_TEMP_ATTRS][ATTR_NAME_LEN];
> +	struct attribute *attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS + 1];
> +	struct attribute_group attr_group[CORE_NUMS_MAX];
> +};
> +
> +struct dimm_temp_attr_group_t {
> +	struct sensor_device_attribute sd_attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS];
> +	char attr_name[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
> +	struct attribute *attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS + 1];
> +	struct attribute_group attr_group[DIMM_NUMS_MAX];
> +};
> +
> +struct peci_hwmon {
> +	struct device *dev;
> +	struct device *hwmon_dev;
> +	char name[NAME_MAX];
> +	const struct attribute_group **groups;
> +	struct cpuinfo_t cpuinfo;
> +	struct temp_group_t temp;
> +	u32 cpu_id;
> +	bool show_core;
> +	u32 core_nums;
> +	u32 dimm_nums;
> +	atomic_t core_group_created;
> +	struct core_temp_attr_group_t core;
> +	struct dimm_temp_attr_group_t dimm;
> +};
> +
> +enum label_t {
> +	L_DIE,
> +	L_DTS,
> +	L_TCONTROL,
> +	L_TTHROTTLE,
> +	L_MAX
> +};
> +
> +static const char *peci_label[L_MAX] = {
> +	"Die temperature\n",
> +	"DTS thermal margin to Tcontrol\n",
> +	"Tcontrol temperature\n",
> +	"Tthrottle temperature\n",

"temperature" is redundant for a temperature label.

> +};
> +
> +static DEFINE_MUTEX(peci_hwmon_lock);
> +
> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no);

Please avoid forward declarations.

> +
> +

Please run your patches throuch checkpatch --strict and fix what it reports,
or provide a reason why you don't.

> +static int xfer_peci_msg(int cmd, void *pmsg)
> +{
> +	int rc;
> +
> +	mutex_lock(&peci_hwmon_lock);
> +	rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
> +	mutex_unlock(&peci_hwmon_lock);
> +
> +	return rc;
> +}
> +
> +static int get_cpuinfo(struct peci_hwmon *priv)
> +{
> +	struct peci_get_dib_msg dib_msg;
> +	struct peci_rd_pkg_cfg_msg cfg_msg;
> +	int rc, i;
> +
> +	if (!priv->cpuinfo.valid) {
> +		dib_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +
> +		rc = xfer_peci_msg(PECI_IOC_GET_DIB, (void *)&dib_msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		priv->cpuinfo.dib = dib_msg.dib;
> +
> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +		cfg_msg.index = MBX_INDEX_CPU_ID;
> +		cfg_msg.param = 0;
> +		cfg_msg.rx_len = 4;
> +
> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		priv->cpuinfo.cpuid = cfg_msg.pkg_config[0];
> +
> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +		cfg_msg.index = MBX_INDEX_CPU_ID;
> +		cfg_msg.param = 1;
> +		cfg_msg.rx_len = 4;
> +
> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		priv->cpuinfo.platform_id = cfg_msg.pkg_config[0];
> +
> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +		cfg_msg.index = MBX_INDEX_CPU_ID;
> +		cfg_msg.param = 3;
> +		cfg_msg.rx_len = 4;
> +
> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		priv->cpuinfo.logical_thread_nums = cfg_msg.pkg_config[0] + 1;
> +
> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +		cfg_msg.index = MBX_INDEX_CPU_ID;
> +		cfg_msg.param = 4;
> +		cfg_msg.rx_len = 4;
> +
> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		priv->cpuinfo.microcode = (cfg_msg.pkg_config[3] << 24) |
> +					  (cfg_msg.pkg_config[2] << 16) |
> +					  (cfg_msg.pkg_config[1] << 8) |
> +					  cfg_msg.pkg_config[0];
> +
> +		priv->core_nums = priv->cpuinfo.logical_thread_nums / 2;

This seems to assume a 1:2 relationship between number of threads and
number of CPUs, which is incorrect.

> +
> +		if (priv->show_core &&
> +		    atomic_inc_return(&priv->core_group_created) == 1) {
> +			for (i = 0; i < priv->core_nums; i++) {
> +				rc = create_core_temp_group(priv, i);

This is messy. Sensor groups should be created before or during
hwmon registration, not at some arbitrary later time.

I don't know the logic behind this, but if it is supposed to track CPUs
coming online and going offline it is the wrong approach.

> +				if (rc != 0) {
> +					dev_err(priv->dev,
> +						"Failed to create core temp group\n");
> +					for (--i; i >= 0; i--) {
> +						sysfs_remove_group(
> +						     &priv->hwmon_dev->kobj,
> +						     &priv->core.attr_group[i]);
> +					}
> +					atomic_set(&priv->core_group_created,
> +						   0);
> +					return rc;
> +				}
> +			}
> +		}
> +
> +		priv->cpuinfo.valid = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_tjmax(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	int rc;
> +
> +	rc = get_cpuinfo(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (!priv->temp.tjmax.valid) {
> +		msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +		msg.index = MBX_INDEX_TEMP_TARGET;
> +		msg.param = 0;
> +		msg.rx_len = 4;
> +
> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		priv->temp.tjmax.value = (s32)msg.pkg_config[2] * 1000;
> +		priv->temp.tjmax.valid = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_tcontrol(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 tcontrol_margin;
> +	int rc;
> +
> +	if (priv->temp.tcontrol.valid &&
> +	    time_before(jiffies, priv->temp.tcontrol.last_updated +
> +				 UPDATE_INTERVAL_MIN))
> +		return 0;
> +

Is the delay necessary ? Otherwise I would suggest to drop it.
It adds a lot of complexity to the driver. Also, if the user polls
values more often, that is presumably on purpose.

> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +	msg.index = MBX_INDEX_TEMP_TARGET;
> +	msg.param = 0;
> +	msg.rx_len = 4;
> +
> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	tcontrol_margin = msg.pkg_config[1];
> +	tcontrol_margin = ((tcontrol_margin ^ 0x80) - 0x80) * 1000;
> +
> +	priv->temp.tcontrol.value = priv->temp.tjmax.value - tcontrol_margin;
> +
> +	if (!priv->temp.tcontrol.valid) {
> +		priv->temp.tcontrol.last_updated = INITIAL_JIFFIES;
> +		priv->temp.tcontrol.valid = true;
> +	} else {
> +		priv->temp.tcontrol.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_tthrottle(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 tthrottle_offset;
> +	int rc;
> +
> +	if (priv->temp.tthrottle.valid &&
> +	    time_before(jiffies, priv->temp.tthrottle.last_updated +
> +				 UPDATE_INTERVAL_MIN))
> +		return 0;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +	msg.index = MBX_INDEX_TEMP_TARGET;
> +	msg.param = 0;
> +	msg.rx_len = 4;
> +
> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	tthrottle_offset = (msg.pkg_config[3] & 0x2f) * 1000;
> +	priv->temp.tthrottle.value = priv->temp.tjmax.value - tthrottle_offset;
> +
> +	if (!priv->temp.tthrottle.valid) {
> +		priv->temp.tthrottle.last_updated = INITIAL_JIFFIES;
> +		priv->temp.tthrottle.valid = true;
> +	} else {
> +		priv->temp.tthrottle.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_die_temp(struct peci_hwmon *priv)
> +{
> +	struct peci_get_temp_msg msg;
> +	int rc;
> +
> +	if (priv->temp.die.valid &&
> +	    time_before(jiffies, priv->temp.die.last_updated +
> +				 UPDATE_INTERVAL_MIN))
> +		return 0;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +
> +	rc = xfer_peci_msg(PECI_IOC_GET_TEMP, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	priv->temp.die.value = priv->temp.tjmax.value +
> +			       ((s32)msg.temp_raw * 1000 / 64);
> +
> +	if (!priv->temp.die.valid) {
> +		priv->temp.die.last_updated = INITIAL_JIFFIES;
> +		priv->temp.die.valid = true;
> +	} else {
> +		priv->temp.die.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_dts_margin(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 dts_margin;
> +	int rc;
> +
> +	if (priv->temp.dts_margin.valid &&
> +	    time_before(jiffies, priv->temp.dts_margin.last_updated +
> +				 UPDATE_INTERVAL_MIN))
> +		return 0;
> +
Are all those values expected to change dynamically, or are some static ?
Static values do not have to be re-read repeatedly but can be cached
permanently.

> +	rc = get_cpuinfo(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +	msg.index = MBX_INDEX_DTS_MARGIN;
> +	msg.param = 0;
> +	msg.rx_len = 4;
> +
> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
> +
> +	/*
> +	 * Processors return a value of DTS reading in 10.6 format
> +	 * (10 bits signed decimal, 6 bits fractional).
> +	 * Error codes:
> +	 *   0x8000: General sensor error
> +	 *   0x8001: Reserved
> +	 *   0x8002: Underflow on reading value
> +	 *   0x8003-0x81ff: Reserved
> +	 */
> +	if (dts_margin >= 0x8000 && dts_margin <= 0x81ff)
> +		return -1;
> +
> +	dts_margin = ((dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
> +
The above code is repeated several times. Please consider moving it
into a function to reduce duplication.

> +	priv->temp.dts_margin.value = dts_margin;
> +
> +	if (!priv->temp.dts_margin.valid) {
> +		priv->temp.dts_margin.last_updated = INITIAL_JIFFIES;
> +		priv->temp.dts_margin.valid = true;
> +	} else {
> +		priv->temp.dts_margin.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_core_temp(struct peci_hwmon *priv, int core_index)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 core_dts_margin;
> +	int rc;
> +
> +	if (priv->temp.core[core_index].valid &&
> +	    time_before(jiffies, priv->temp.core[core_index].last_updated +
> +				 UPDATE_INTERVAL_MIN))
> +		return 0;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +	msg.index = MBX_INDEX_PER_CORE_DTS_TEMP;
> +	msg.param = core_index;
> +	msg.rx_len = 4;
> +
> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	core_dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
> +
> +	/*
> +	 * Processors return a value of the core DTS reading in 10.6 format
> +	 * (10 bits signed decimal, 6 bits fractional).
> +	 * Error codes:
> +	 *   0x8000: General sensor error
> +	 *   0x8001: Reserved
> +	 *   0x8002: Underflow on reading value
> +	 *   0x8003-0x81ff: Reserved
> +	 */
> +	if (core_dts_margin >= 0x8000 && core_dts_margin <= 0x81ff)
> +		return -1;
> +
> +	core_dts_margin = ((core_dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
> +
> +	priv->temp.core[core_index].value = priv->temp.tjmax.value +
> +					    core_dts_margin;
> +
> +	if (!priv->temp.core[core_index].valid) {
> +		priv->temp.core[core_index].last_updated = INITIAL_JIFFIES;
> +		priv->temp.core[core_index].valid = true;
> +	} else {
> +		priv->temp.core[core_index].last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_dimm_temp(struct peci_hwmon *priv, int dimm_index)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	int channel_rank = dimm_index / 2;
> +	int dimm_order = dimm_index % 2;
> +	int rc;
> +
> +	if (priv->temp.core[dimm_index].valid &&
> +	    time_before(jiffies, priv->temp.core[dimm_index].last_updated +
> +				 UPDATE_INTERVAL_MIN))
> +		return 0;
> +
> +	rc = get_cpuinfo(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +	msg.index = MBX_INDEX_DDR_DIMM_TEMP;
> +	msg.param = channel_rank;
> +	msg.rx_len = 4;
> +
> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	priv->temp.dimm[dimm_index].value = msg.pkg_config[dimm_order] * 1000;
> +
> +	if (!priv->temp.dimm[dimm_index].valid) {
> +		priv->temp.dimm[dimm_index].last_updated = INITIAL_JIFFIES;
> +		priv->temp.dimm[dimm_index].valid = true;
> +	} else {
> +		priv->temp.dimm[dimm_index].last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t show_info(struct device *dev,
> +			 struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_cpuinfo(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "dib         : 0x%08x\n"
> +			    "cpuid       : 0x%x\n"
> +			    "platform id : %d\n"
> +			    "stepping    : %d\n"
> +			    "microcode   : 0x%08x\n"
> +			    "logical thread nums : %d\n",
> +			    priv->cpuinfo.dib,
> +			    priv->cpuinfo.cpuid,
> +			    priv->cpuinfo.platform_id,
> +			    priv->cpuinfo.cpuid & 0xf,
> +			    priv->cpuinfo.microcode,
> +			    priv->cpuinfo.logical_thread_nums);
> +}

Please no non-standard attributes, much less attributes not following sysfs
attribute rules. If you want to display such information, consider using
debugfs. Besides, this information specifically appears to duplicate
the content of /proc/cpuid, which doesn't really add any value at all.

> +
> +static ssize_t show_tcontrol(struct device *dev,
> +			     struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_tcontrol(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.tcontrol.value);
> +}
> +
> +static ssize_t show_tcontrol_margin(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int rc;
> +
> +	rc = get_tcontrol(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", sensor_attr->index == POS ?
> +				    priv->temp.tjmax.value -
> +				    priv->temp.tcontrol.value :
> +				    priv->temp.tcontrol.value -
> +				    priv->temp.tjmax.value);
> +}
> +
> +static ssize_t show_tthrottle(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_tthrottle(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.tthrottle.value);
> +}
> +
> +static ssize_t show_tjmax(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.tjmax.value);
> +}
> +
> +static ssize_t show_die_temp(struct device *dev,
> +			     struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_die_temp(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.die.value);
> +}
> +
> +static ssize_t show_dts_therm_margin(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_dts_margin(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.dts_margin.value);
> +}
> +
> +static ssize_t show_core_temp(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int core_index = sensor_attr->index;
> +	int rc;
> +
> +	rc = get_core_temp(priv, core_index);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.core[core_index].value);
> +}
> +
> +static ssize_t show_dimm_temp(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int dimm_index = sensor_attr->index;
> +	int rc;
> +
> +	rc = get_dimm_temp(priv, dimm_index);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.dimm[dimm_index].value);
> +}
> +
> +static ssize_t show_value(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	return sprintf(buf, "%d\n", sensor_attr->index);
> +}
> +
> +static ssize_t show_label(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	return sprintf(buf, peci_label[sensor_attr->index]);
> +}
> +
> +static ssize_t show_core_label(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	return sprintf(buf, "Core #%d temperature\n", sensor_attr->index);
> +}

Your label strings are quite long. How does that look like with the
sensors command ?

Plus, again, "temperature" in a temperature label is redundant.

> +
> +static ssize_t show_dimm_label(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	char channel = 'A' + (sensor_attr->index / 2);
> +	int index = sensor_attr->index % 2;
> +
> +	return sprintf(buf, "Channel Rank %c DDR DIMM #%d temperature\n",
> +		       channel, index);
> +}
> +
> +/* Die temperature */
> +static SENSOR_DEVICE_ATTR(temp1_label, 0444, show_label, NULL, L_DIE);
> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_die_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max, 0444, show_tcontrol, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, 0444, show_tjmax, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0444, show_tcontrol_margin, NULL,
> +			  POS);
> +
> +static struct attribute *die_temp_attrs[] = {
> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group die_temp_attr_group = {
> +	.attrs = die_temp_attrs,
> +};
> +
> +/* DTS thermal margin temperature */
> +static SENSOR_DEVICE_ATTR(temp2_label, 0444, show_label, NULL, L_DTS);
> +static SENSOR_DEVICE_ATTR(temp2_input, 0444, show_dts_therm_margin, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_min, 0444, show_value, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_lcrit, 0444, show_tcontrol_margin, NULL, NEG);
> +
> +static struct attribute *dts_margin_temp_attrs[] = {
> +	&sensor_dev_attr_temp2_label.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_min.dev_attr.attr,
> +	&sensor_dev_attr_temp2_lcrit.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dts_margin_temp_attr_group = {
> +	.attrs = dts_margin_temp_attrs,
> +};
> +
> +/* Tcontrol temperature */
> +static SENSOR_DEVICE_ATTR(temp3_label, 0444, show_label, NULL, L_TCONTROL);
> +static SENSOR_DEVICE_ATTR(temp3_input, 0444, show_tcontrol, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp3_crit, 0444, show_tjmax, NULL, 0);
> +
> +static struct attribute *tcontrol_temp_attrs[] = {
> +	&sensor_dev_attr_temp3_label.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group tcontrol_temp_attr_group = {
> +	.attrs = tcontrol_temp_attrs,
> +};
> +
> +/* Tthrottle temperature */
> +static SENSOR_DEVICE_ATTR(temp4_label, 0444, show_label, NULL, L_TTHROTTLE);
> +static SENSOR_DEVICE_ATTR(temp4_input, 0444, show_tthrottle, NULL, 0);
> +
> +static struct attribute *tthrottle_temp_attrs[] = {
> +	&sensor_dev_attr_temp4_label.dev_attr.attr,
> +	&sensor_dev_attr_temp4_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group tthrottle_temp_attr_group = {
> +	.attrs = tthrottle_temp_attrs,
> +};
> +
> +/* CPU info */
> +static SENSOR_DEVICE_ATTR(info, 0444, show_info, NULL, 0);
> +
> +static struct attribute *info_attrs[] = {
> +	&sensor_dev_attr_info.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group info_attr_group = {
> +	.attrs = info_attrs,
> +};
> +
> +const struct attribute_group *peci_hwmon_attr_groups[] = {
> +	&info_attr_group,
> +	&die_temp_attr_group,
> +	&dts_margin_temp_attr_group,
> +	&tcontrol_temp_attr_group,
> +	&tthrottle_temp_attr_group,
> +	NULL
> +};
> +
> +static ssize_t (*const core_show_fn[CORE_TEMP_ATTRS]) (struct device *dev,
> +		struct device_attribute *devattr, char *buf) = {
> +	show_core_label,
> +	show_core_temp,
> +	show_tcontrol,
> +	show_tjmax,
> +	show_tcontrol_margin,
> +};
> +
> +static const char *const core_suffix[CORE_TEMP_ATTRS] = {
> +	"label",
> +	"input",
> +	"max",
> +	"crit",
> +	"crit_hyst",
> +};
> +
> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no)
> +{
> +	int i;
> +
> +	for (i = 0; i < CORE_TEMP_ATTRS; i++) {
> +		snprintf(priv->core.attr_name[core_no][i],
> +			 ATTR_NAME_LEN, "temp%d_%s",
> +			 CORE_INDEX_OFFSET + core_no, core_suffix[i]);
> +		sysfs_attr_init(
> +			    &priv->core.sd_attrs[core_no][i].dev_attr.attr);
> +		priv->core.sd_attrs[core_no][i].dev_attr.attr.name =
> +					       priv->core.attr_name[core_no][i];
> +		priv->core.sd_attrs[core_no][i].dev_attr.attr.mode = 0444;
> +		priv->core.sd_attrs[core_no][i].dev_attr.show = core_show_fn[i];
> +		if (i == 0 || i == 1) /* label or temp */
> +			priv->core.sd_attrs[core_no][i].index = core_no;
> +		priv->core.attrs[core_no][i] =
> +				 &priv->core.sd_attrs[core_no][i].dev_attr.attr;
> +	}
> +
> +	priv->core.attr_group[core_no].attrs = priv->core.attrs[core_no];
> +
> +	return sysfs_create_group(&priv->hwmon_dev->kobj,
> +				  &priv->core.attr_group[core_no]);
> +}
> +
> +static ssize_t (*const dimm_show_fn[DIMM_TEMP_ATTRS]) (struct device *dev,
> +		struct device_attribute *devattr, char *buf) = {
> +	show_dimm_label,
> +	show_dimm_temp,
> +};
> +
> +static const char *const dimm_suffix[DIMM_TEMP_ATTRS] = {
> +	"label",
> +	"input",
> +};
> +
> +static int create_dimm_temp_group(struct peci_hwmon *priv, int dimm_no)
> +{
> +	int i;
> +
> +	for (i = 0; i < DIMM_TEMP_ATTRS; i++) {
> +		snprintf(priv->dimm.attr_name[dimm_no][i],
> +			 ATTR_NAME_LEN, "temp%d_%s",
> +			 DIMM_INDEX_OFFSET + dimm_no, dimm_suffix[i]);
> +		sysfs_attr_init(&priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr);
> +		priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.name =
> +					       priv->dimm.attr_name[dimm_no][i];
> +		priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.mode = 0444;
> +		priv->dimm.sd_attrs[dimm_no][i].dev_attr.show = dimm_show_fn[i];
> +		priv->dimm.sd_attrs[dimm_no][i].index = dimm_no;
> +		priv->dimm.attrs[dimm_no][i] =
> +				 &priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr;
> +	}
> +
> +	priv->dimm.attr_group[dimm_no].attrs = priv->dimm.attrs[dimm_no];
> +
> +	return sysfs_create_group(&priv->hwmon_dev->kobj,
> +				  &priv->dimm.attr_group[dimm_no]);
> +}
> +
> +static int peci_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct peci_hwmon *priv;
> +	struct device *hwmon;
> +	int rc, i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, priv);
> +	priv->dev = dev;
> +
> +	rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);

What entity determines cpu-id ?

> +	if (rc || priv->cpu_id >= CPU_ID_MAX) {
> +		dev_err(dev, "Invalid cpu-id configuration\n");
> +		return rc;
> +	}
> +
> +	rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);

This is an odd devicetree attribute. Normally the number of DIMMs
is dynamic. Isn't there a means to get all that information dynamically
instead of having to set it through devicetree ? What if someone adds
or removes a DIMM ? Who updates the devicetree ?

> +	if (rc || priv->dimm_nums > DIMM_NUMS_MAX) {
> +		dev_warn(dev, "Invalid dimm-nums : %u. Use default : %u\n",
> +			 priv->dimm_nums, OF_DIMM_NUMS_DEFAULT);
> +		priv->dimm_nums = OF_DIMM_NUMS_DEFAULT;
> +	}
> +
> +	priv->show_core = of_property_read_bool(np, "show-core");

This does not look like an appropriate devicetree attribute.

> +
> +	priv->groups = peci_hwmon_attr_groups;
> +

This assignment (and the ->groups variable) is quite pointless.

> +	snprintf(priv->name, NAME_MAX, HWMON_NAME ".cpu%d", priv->cpu_id);
> +
> +	hwmon = devm_hwmon_device_register_with_groups(dev,
> +						       priv->name,
> +						       priv, priv->groups);

Please rewrite the driver to use devm_hwmon_device_register_with_info(),
and avoid dynamic attributes.

> +
> +	rc = PTR_ERR_OR_ZERO(hwmon);
> +	if (rc != 0) {
> +		dev_err(dev, "Failed to register peci hwmon\n");
> +		return rc;
> +	}
> +
> +	priv->hwmon_dev = hwmon;

Something is logically wrong if you need to store hwmon_dev in the
private data structure. Specifically, creating attributes dynamically
after hwmon registration is wrong.

> +
> +	for (i = 0; i < priv->dimm_nums; i++) {
> +		rc = create_dimm_temp_group(priv, i);

No. See earlier comments. All attribute groups must be created during
registration (or before, but I am not inclined to accept a new driver
doing that).

> +		if (rc != 0) {
> +			dev_err(dev, "Failed to create dimm temp group\n");
> +			for (--i; i >= 0; i--) {
> +				sysfs_remove_group(&priv->hwmon_dev->kobj,
> +						   &priv->dimm.attr_group[i]);
> +			}
> +			return rc;
> +		}
> +	}
> +
> +	/*
> +	 * Try to create core temp group now. It will be created if CPU is
> +	 * curretnly online or it will be created after the first reading of
> +	 * cpuinfo from the online CPU otherwise.

This is not how CPUs are supposed to be detected, and it does not handle CPUs
taken offline. If the driver is instantiated as a CPU comes online, or as it
goes offline, the driver should use the appropriate kernel interfaces to
trigger that instantiation or removal. However, if so, it may be inappropriate
to associate CPU temperatures with other system temperatures in the same
instance of the driver; after all, those are all independent of each other.

Overall, I suspect that there should be a callback or some other mechanism
in the peci core to trigger instantiation and removal of this driver, and
I am not sure if any of the devicetree properties makes sense at all.

For example, if an instance of this driver is associated with a PECI
agent (with assorted CPU/DIMM temperature reporting), the instantiation
could be triggered as soon as the PECI core detects that the agent is
available, and the PECI core could report what exactly that instance
supports.

> +	 */
> +	if (priv->show_core)
> +		(void) get_cpuinfo(priv);
> +
> +	dev_info(dev, "peci hwmon for CPU#%d registered\n", priv->cpu_id);

Is this logging noise necessary ? Besides, some of it is redundant.

> +
> +	return rc;
> +}
> +
> +static int peci_hwmon_remove(struct platform_device *pdev)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(&pdev->dev);
> +	int i;
> +
> +	if (atomic_read(&priv->core_group_created))
> +		for (i = 0; i < priv->core_nums; i++) {
> +			sysfs_remove_group(&priv->hwmon_dev->kobj,
> +					   &priv->core.attr_group[i]);
> +		}
> +
> +	for (i = 0; i < priv->dimm_nums; i++) {
> +		sysfs_remove_group(&priv->hwmon_dev->kobj,
> +				   &priv->dimm.attr_group[i]);
> +	}

If you need to call sysfs_remove_group from here,
something is conceptually wrong in your driver.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id peci_of_table[] = {
> +	{ .compatible = "peci-hwmon", },

This does not look like a reference to some piece of hardware.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, peci_of_table);
> +
> +static struct platform_driver peci_hwmon_driver = {
> +	.probe = peci_hwmon_probe,
> +	.remove = peci_hwmon_remove,
> +	.driver = {
> +		.name           = DEVICE_NAME,
> +		.of_match_table = peci_of_table,
> +	},
> +};
> +
> +module_platform_driver(peci_hwmon_driver);
> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> +MODULE_DESCRIPTION("PECI hwmon driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 3:55 AM, Arnd Bergmann wrote:
> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>> This commit adds driver implementation for Aspeed PECI. Also adds
>> generic peci.h and peci_ioctl.h files to provide compatibility
>> to peci drivers that can be implemented later e.g. Nuvoton's BMC
>> SoC family.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> 
>> +#include <linux/clk.h>
>> +#include <linux/crc8.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/peci_ioctl.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/semaphore.h>
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
> 
> semaphore.h is not used here and can be dropped.
> 

You are right. Will drop it.

>> +static struct aspeed_peci *aspeed_peci_priv;
> 
> Try to avoid instance variables like this one. You should always be able to find
> that pointer from whatever structure you were called with.
> 
> 

Okay. I will use driver_data instead.

>> +       timeout = wait_for_completion_interruptible_timeout(
>> +                                       &priv->xfer_complete,
>> +                                       msecs_to_jiffies(priv->cmd_timeout_ms));
>> +
>> +       dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
>> +       if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
>> +               dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
>> +                       PECI_CMD_STS_GET(peci_state));
>> +       else
>> +               dev_dbg(priv->dev, "PECI_STATE : read error\n");
>> +
>> +       if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
>> +               if (timeout <= 0) {
>> +                       dev_dbg(priv->dev, "Timeout waiting for a response!\n");
>> +                       rc = -ETIME;
>> +               } else {
>> +                       dev_dbg(priv->dev, "No valid response!\n");
>> +                       rc = -EFAULT;
>> +               }
>> +               return rc;
>> +       }
> 
> You don't seem to handle -ERESTARTSYS correct here. Either do it
> right, or drop the _interruptible part above.
> 

Will add a handling logic for the -ERESTARTSYS.

>> +typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
>> +
>> +static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
>> +       ioctl_xfer_msg,
>> +       ioctl_ping,
>> +       ioctl_get_dib,
>> +       ioctl_get_temp,
>> +       ioctl_rd_pkg_cfg,
>> +       ioctl_wr_pkg_cfg,
>> +       ioctl_rd_ia_msr,
>> +       NULL, /* Reserved */
>> +       ioctl_rd_pci_cfg,
>> +       NULL, /* Reserved */
>> +       ioctl_rd_pci_cfg_local,
>> +       ioctl_wr_pci_cfg_local,
>> +};
>> +
>> +
>> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> +{
>> +       struct aspeed_peci *priv;
>> +       long ret = 0;
>> +       void __user *argp = (void __user *)arg;
>> +       int timeout = PECI_IDLE_CHECK_TIMEOUT;
>> +       u8 msg[sizeof(struct peci_xfer_msg)];
>> +       unsigned int peci_cmd, msg_size;
>> +       u32 cmd_sts;
>> +
>> +       /*
>> +        * Treat it as an inter module call when filp is null but only in case
>> +        * the private data is initialized.
>> +        */
>> +       if (filp)
>> +               priv = container_of(filp->private_data,
>> +                                   struct aspeed_peci, miscdev);
>> +       else
>> +               priv = aspeed_peci_priv;
> 
> Drop this.
> 

peci_ioctl is being called from peci_hwmon as an inter-module call so it 
is needed, but as you suggested in the other patch, I'll consider 
redesign it with adding a peci device class.

>> +       if (!priv)
>> +               return -ENXIO;
>> +
>> +       switch (cmd) {
>> +       case PECI_IOC_XFER:
>> +       case PECI_IOC_PING:
>> +       case PECI_IOC_GET_DIB:
>> +       case PECI_IOC_GET_TEMP:
>> +       case PECI_IOC_RD_PKG_CFG:
>> +       case PECI_IOC_WR_PKG_CFG:
>> +       case PECI_IOC_RD_IA_MSR:
>> +       case PECI_IOC_RD_PCI_CFG:
>> +       case PECI_IOC_RD_PCI_CFG_LOCAL:
>> +       case PECI_IOC_WR_PCI_CFG_LOCAL:
>> +               peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
>> +               msg_size = _IOC_SIZE(cmd);
>> +               break;
> 
> Having to keep the switch() statement and the array above seems a
> little fragile. Can you just do one or the other?
> 
> Regarding the command set, you have both a low-level PECI_IOC_XFER
> interface and a high-level interface. Can you explain why? I'd think that
> generally speaking it's better to have only one of the two.
> 

I was intended to provide generic peci command set, also the low level 
PECI_IOC_XFER to provide flexibility for a case when compose a custom 
peci command which cannot be covered by the high-level command set. As 
you said, all other commands can be implemented in the upper layer but 
the benefit of when this driver has the implementation is, it's easy to 
manage retry logic since peci is retrial based protocol intends to do 
not disturb a CPU if the CPU is doing more important task.

However, your thought also makes sense. I'll check the spec again 
whether the high-level command set can cover all cases. If so, I'll 
remove the low-level command.

>> +       /* Check command sts and bus idle state */
>> +       while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts)
>> +              && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
>> +               if (timeout-- < 0) {
>> +                       dev_dbg(priv->dev, "Timeout waiting for idle state!\n");
>> +                       ret = -ETIME;
>> +                       goto out;
>> +               }
>> +               usleep_range(10000, 11000);
>> +       };
> 
> To implement timeout, it's better to replace the counter with a
> jiffies/time_before or ktime_get()/ktime_before() check, since usleep_range()
> is might sleep considerably longer than expected.
> 

Thanks for the suggestion. Will rewrite it using ktime_get()/ktime_before().

>> +EXPORT_SYMBOL_GPL(peci_ioctl);
> 
> No user of this, so drop it.
> 

peci_hwmon is using it.

>> +static int aspeed_peci_open(struct inode *inode, struct file *filp)
>> +{
>> +       struct aspeed_peci *priv =
>> +               container_of(filp->private_data, struct aspeed_peci, miscdev);
>> +
>> +       atomic_inc(&priv->ref_count);
>> +
>> +       dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_peci_release(struct inode *inode, struct file *filp)
>> +{
>> +       struct aspeed_peci *priv =
>> +               container_of(filp->private_data, struct aspeed_peci, miscdev);
>> +
>> +       atomic_dec(&priv->ref_count);
>> +
>> +       dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
>> +
>> +       return 0;
>> +}
> 
> Nothing uses that reference count, drop it.
> 

You are right. Will drop it.

>> new file mode 100644
>> index 0000000..66322c6
>> --- /dev/null
>> +++ b/include/misc/peci.h
>> @@ -0,0 +1,11 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017 Intel Corporation
>> +
>> +#ifndef __PECI_H
>> +#define __PECI_H
>> +
>> +#include <linux/peci_ioctl.h>
>> +
>> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
>> +
>> +#endif /* __PECI_H */
> 
> Not used anywhere.
> 

peci_hwmon is using it.

>> diff --git a/include/uapi/linux/peci_ioctl.h b/include/uapi/linux/peci_ioctl.h
>> new file mode 100644
>> index 0000000..8386848
>> --- /dev/null
>> +++ b/include/uapi/linux/peci_ioctl.h
>> @@ -0,0 +1,270 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017 Intel Corporation
>> +
>> +#ifndef __PECI_IOCTL_H
>> +#define __PECI_IOCTL_H
>> +
>> +#include <linux/ioctl.h>
>> +
>> +/* Base Address of 48d */
>> +#define PECI_BASE_ADDR  0x30  /* The PECI client's default address of 0x30 */
>> +#define PECI_OFFSET_MAX 8     /* Max numver of CPU clients */
>> +
>> +/* PCI Access */
>> +#define MAX_PCI_READ_LEN 24  /* Number of bytes of the PCI Space read */
>> +
>> +#define PCI_BUS0_CPU0      0x00
>> +#define PCI_BUS0_CPU1      0x80
>> +#define PCI_CPUBUSNO_BUS   0x00
>> +#define PCI_CPUBUSNO_DEV   0x08
>> +#define PCI_CPUBUSNO_FUNC  0x02
>> +#define PCI_CPUBUSNO       0xcc
>> +#define PCI_CPUBUSNO_1     0xd0
>> +#define PCI_CPUBUSNO_VALID 0xd4
> 
> I can't tell for sure, but this file seems to be mixing the kernel API with
> hardware specific macros that are not needed in user space. Can you move
> some of this file into the driver itself?
> 
> This might go back to the previous question about the high-level and
> low-level interfaces: if you can drop the low-level ioctl interface, more
> of this header can become private to the driver.
> 

As I answered above, I'll check the spec again and remove the low-level 
command if the high-level command set covers all cases.

>> +/* Package Identifier Read Parameter Value */
>> +#define PKG_ID_CPU_ID               0x0000  /* 0 - CPUID Info */
>> +#define PKG_ID_PLATFORM_ID          0x0001  /* 1 - Platform ID */
>> +#define PKG_ID_UNCORE_ID            0x0002  /* 2 - Uncore Device ID */
>> +#define PKG_ID_MAX_THREAD_ID        0x0003  /* 3 - Max Thread ID */
>> +#define PKG_ID_MICROCODE_REV        0x0004  /* 4 - CPU Microcode Update Revision */
>> +#define PKG_ID_MACHINE_CHECK_STATUS 0x0005  /* 5 - Machine Check Status */
>> +
>> +/* RdPkgConfig Index */
>> +#define MBX_INDEX_CPU_ID            0   /* Package Identifier Read */
>> +#define MBX_INDEX_VR_DEBUG          1   /* VR Debug */
>> +#define MBX_INDEX_PKG_TEMP_READ     2   /* Package Temperature Read */
>> +#define MBX_INDEX_ENERGY_COUNTER    3   /* Energy counter */
>> +#define MBX_INDEX_ENERGY_STATUS     4   /* DDR Energy Status */
>> +#define MBX_INDEX_WAKE_MODE_BIT     5   /* "Wake on PECI" Mode bit */
>> +#define MBX_INDEX_EPI               6   /* Efficient Performance Indication */
> 
> Who defines these constants? Are they specific to the aspeed BMC, to the HECI
> protocol, or to a particular version of the remote endpoint?
> 

These are common peci definitions, not the aspeed BMC specific.

>> +#pragma pack(push, 1)
>> +struct peci_xfer_msg {
>> +       unsigned char client_addr;
>> +       unsigned char tx_len;
>> +       unsigned char rx_len;
>> +       unsigned char tx_buf[MAX_BUFFER_SIZE];
>> +       unsigned char rx_buf[MAX_BUFFER_SIZE];
>> +};
>> +#pragma pack(pop)
>> +
>> +struct peci_ping_msg {
>> +       unsigned char target;
>> +};
>> +
>> +struct peci_get_dib_msg {
>> +       unsigned char target;
>> +       unsigned int  dib;
>> +};
>> +
>> +struct peci_get_temp_msg {
>> +       unsigned char target;
>> +       signed short  temp_raw;
>> +};
> 
> Aside from what Greg already said about the types, please be careful to
> also avoid implicit padding in the API data structures, including the end of the
> structure.
> 

Okay, I'll expand the pack() scope for all these definition.

>> +#define PECI_IOC_RD_PCI_CFG \
>> +       _IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG, 0, \
>> +               struct peci_rd_pci_cfg_msg)
>> +
>> +#define PECI_IOC_RD_PCI_CFG_LOCAL \
>> +       _IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG_LOCAL, 0, \
>> +               struct peci_rd_pci_cfg_local_msg)
>> +
>> +#define PECI_IOC_WR_PCI_CFG_LOCAL \
>> +       _IOWR(PECI_IOC_BASE + PECI_CMD_WR_PCI_CFG_LOCAL, 0, \
>> +               struct peci_wr_pci_cfg_local_msg)
> 
> Can you give some background on what these do? In particular, who
> is configuring whose PCI devices?
> 
>          Arnd
> 

These are commands to read/write a client CPU's PCI configuration which 
could be an end-point of the physical PECI interface connection. BMC 
controller will be a host and a CPU will be a client.

Thanks,
Jae

Re: [PATCH linux dev-4.10 4/6] Documentation: dt-bindings: Add a generic PECI hwmon

[Jae Hyun Yoo]

On 1/10/2018 4:20 AM, Arnd Bergmann wrote:
> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>> This commit add a dt-bindings document for a generic PECI hwmon
>> driver.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>>   .../devicetree/bindings/hwmon/peci-hwmon.txt       | 33 ++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
>> new file mode 100644
>> index 0000000..20b86f5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
>> @@ -0,0 +1,33 @@
>> +* Generic PECI (Platform Environment Control Interface) hwmon driver.
>> +
>> +Dependency:
>> +- This driver uses a PECI misc driver as a controller interface so one of PECI
>> +  misc drivers which provides compatible ioctls has to be enabled.
> 
> The binding should only explain how we describe the hardware in an
> operating-system
> independent way, but not talk about how an OS is supposed to implement those
> drivers.
> 
> Having multiple drivers each provide an exported function is not possible
> in Linux: it immediately breaks building an 'allyesconfig' kernel, and prevents
> you from running the same kernel across multiple implementations, so that
> has to be redesigned anyway.
> 

Agreed, I'll consider redesigning of it.

>> +Required properties:
>> +- compatible: "peci-hwmon"
>> +- cpu-id: Should contain CPU socket ID
>> +       - 0 ~ 7
>> +
>> +Optional properties:
>> +- show-core: If this protperty is defined, core tmeperature attrubites will be
> 
> s/protperty/property/
> s/tmeperature/temperature/
> s/attrubites/attributes/
> 

Oops! I made this many typos in this single line. Thanks for your 
pointing it out. Will fix these.

>> +            enumerated.
>> +- dimm-nums: Should contain the number of DIMM slots that attached to each CPU
>> +            which is indicated by cpu-id.
>> +       0 ~ 16 (default: 16)
>> +            In case of 0, DIMM temperature attrubites will not be enumerated.
> 
> Is this only an initial list that you expect to be extended in the
> future, or is this
> a complete list of sensors that can ever be connected to PECI?
> 
> Should this be PECI version specific?
> 
>        Arnd
> 

The maximum supportable number of dimm slots is not PECI version 
specific but depends on CPU architecture. Currently IA supports up to 16 
slot as the maximum but it could vary in the future architecture.

Thanks,
Jae

Re: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Jae Hyun Yoo]

On 1/10/2018 4:29 AM, Arnd Bergmann wrote:
> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>> This commit adds driver implementation for a generic PECI hwmon.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> 
>> +static int xfer_peci_msg(int cmd, void *pmsg)
>> +{
>> +       int rc;
>> +
>> +       mutex_lock(&peci_hwmon_lock);
>> +       rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
>> +       mutex_unlock(&peci_hwmon_lock);
>> +
>> +       return rc;
>> +}
> 
> I said earlier that peci_ioctl() looked unused, that was obviously
> wrong, but what you have here
> is not a proper way to abstract a bus.
> 
> Maybe this can be done more like an i2c bus: make the peci controller
> a bus device
> and register all known target/index pairs as devices with the peci bus
> type, and have
> them probed from DT. The driver can then bind to each of those individually.
> Not sure if that is getting to granular at that point, I'd have to
> understand better
> how it is expected to get used, and what the variances are between
> implementations.
> 
>         Arnd
> 

Thanks for sharing your opinion. In fact, this was also suggested by 
openbmc community so I should consider of redesigning it. I'm currently 
thinking about adding a new PECI device class as an abstract layer and 
any BMC chipset specific driver could be attached to the PECI class 
driver. Then, each CPU client could be registered as an individual 
device as you suggested. Will consider your suggestion.

Thanks a lot!
Jae

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Greg KH]

On Wed, Jan 10, 2018 at 01:46:34PM -0800, Jae Hyun Yoo wrote:
> On 1/10/2018 12:27 PM, Greg KH wrote:
> > On Wed, Jan 10, 2018 at 11:30:05AM -0800, Jae Hyun Yoo wrote:
> > > On 1/10/2018 11:17 AM, Greg KH wrote:
> > > > On Wed, Jan 10, 2018 at 11:14:34AM -0800, Jae Hyun Yoo wrote:
> > > > > On 1/10/2018 2:17 AM, Greg KH wrote:
> > > > > > On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
> > > > > > > From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
> > > > > > > 
> > > > > > > Hello,
> > > > > > > 
> > > > > > > This patch set provides support for PECI of AST2400/2500 which can give us PECI
> > > > > > > functionalities such as temperature monitoring, platform manageability,
> > > > > > > processor diagnostics and failure analysis. Also provides generic peci.h and
> > > > > > > peci_ioctl.h headers to provide compatibility to peci drivers that can be
> > > > > > > implemented later e.g. Nuvoton's BMC SoC family.
> > > > > > 
> > > > > > What is the "dev-4.10" in the subject for?  4.10 is really old and
> > > > > > obsolete :(
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > greg k-h
> > > > > > 
> > > > > 
> > > > > I made this patch set on top of the v4.10 which OpenBmc project is currently
> > > > > using. I'll rebase this patch set onto the current kernel.org mainline.
> > > > 
> > > > What is "OpenBmc", and why are they using an obsolete and insecure
> > > > kernel for their project?  That seems like a very foolish thing to do...
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > > 
> > > 
> > > OpenBmc is an open source project to create a highly extensible framework
> > > for BMC (Board Management Controller) software for data-center computer
> > > systems:
> > > https://github.com/openbmc
> > > 
> > > Its current mainline is v4.10 but it is being kept upgrading so it will be
> > > upgraded to the latest stable or long-term version soon.
> > 
> > Why hasn't it been updated in the year since 4.10 was released?  That's
> > a _very_ long time to be running on a totally insecure kernel, and no
> > new development should ever be done on old kernels, that's even crazier
> > (as we can't go back in time and accept patches for new features to old
> > releases...)
> > 
> 
> Thanks for your pointing it out and I totally agree with you. Actually, we
> are preparing 4.13 update for now and an another update will be followed up.
> As I answered above, I'll rebase this patch set onto the latest kernel.org
> mainline. Sorry for my misunderstanding of upstream process.

4.13?  Why that kernel?  It too is obsolete and insecure and
unsupported.

What keeps you all from just always tracking the latest tree from Linus?
What is in your tree that is not upstream that requires you to have a
kernel tree at all?

And if you do have out-of-tree code, why not use a process that makes it
trivial to update the base kernel version so that you can keep up to
date very easily?  (hint, just using 'git' is not a good way to do
this...)

thanks,

greg k-h

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Joel Stanley]

On Wed, Jan 10, 2018 at 11:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 10, 2018 at 01:46:34PM -0800, Jae Hyun Yoo wrote:
>> Thanks for your pointing it out and I totally agree with you. Actually, we
>> are preparing 4.13 update for now and an another update will be followed up.
>> As I answered above, I'll rebase this patch set onto the latest kernel.org
>> mainline. Sorry for my misunderstanding of upstream process.
>
> 4.13?  Why that kernel?  It too is obsolete and insecure and
> unsupported.

It contains support for our hardware that I have integrated from work
in progress patches and upstream commits.

The OpenBMC project, with myself as the kernel maintainer, have
intentions to regularly move to upstream releases. This takes time and
effort. This time and effort is balanced with submitting our drivers
upstream.

> What keeps you all from just always tracking the latest tree from Linus?

Linus' tree does not contain all of the drivers required to boot
systems. Many of them are still under review on lkml, and others still
require rewrite from the vendor tree.

> What is in your tree that is not upstream that requires you to have a
> kernel tree at all?

We have PECI, video compression, crypto, USB CDC, DRM (graphics),
serial GPIO, LPC mailbox for the ASPEED SoC.

Another silicon vendor has recently joined the project and that brings
an entire SoC that is not upstream. We have patches on the ARM that
are under review for this SoC, with more drivers undergoing cleanup in
order to submit them to the relevant maintainers.

>
> And if you do have out-of-tree code, why not use a process that makes it
> trivial to update the base kernel version so that you can keep up to
> date very easily?  (hint, just using 'git' is not a good way to do
> this...)

We have a process that we've been developing under for the past few
years. I find git to be a great tool for managing Linux kernel trees.

What would you recommend for managing kernel trees?

Cheers,

Joel

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Greg KH]

On Thu, Jan 11, 2018 at 12:28:48AM -0800, Joel Stanley wrote:
> On Wed, Jan 10, 2018 at 11:30 PM, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Wed, Jan 10, 2018 at 01:46:34PM -0800, Jae Hyun Yoo wrote:
> >> Thanks for your pointing it out and I totally agree with you. Actually, we
> >> are preparing 4.13 update for now and an another update will be followed up.
> >> As I answered above, I'll rebase this patch set onto the latest kernel.org
> >> mainline. Sorry for my misunderstanding of upstream process.
> >
> > 4.13?  Why that kernel?  It too is obsolete and insecure and
> > unsupported.
> 
> It contains support for our hardware that I have integrated from work
> in progress patches and upstream commits.
> 
> The OpenBMC project, with myself as the kernel maintainer, have
> intentions to regularly move to upstream releases. This takes time and
> effort. This time and effort is balanced with submitting our drivers
> upstream.

Of course, but please do not have your "users" use a kernel that is
known to have bugs and can not be supported.  That would not be good at
all, don't you think?

> > What keeps you all from just always tracking the latest tree from Linus?
> 
> Linus' tree does not contain all of the drivers required to boot
> systems. Many of them are still under review on lkml, and others still
> require rewrite from the vendor tree.

Merging vendor trees into your tree has got to be a complicated mess.
Why try to keep it all together in one place?

And who is responsible for getting the vendor code upstream?  The
individual drivers?  Individual driver submissions should be quite easy,
what is preventing them from getting merged?

> > What is in your tree that is not upstream that requires you to have a
> > kernel tree at all?
> 
> We have PECI, video compression, crypto, USB CDC, DRM (graphics),
> serial GPIO, LPC mailbox for the ASPEED SoC.

What "USB CDC" do you have that is not upstream?  I'll pick on this one
specifically as I don't think I've seen any patches recently submitted
for that driver at all.  Am I just missing them?

The other ones should also all be easy to get merged, with maybe the
exception of the drm stuff due to the speed that subsystem moves at.
But even there, the community is very helpful in getting stuff upstream,
have you asked for help?

> Another silicon vendor has recently joined the project and that brings
> an entire SoC that is not upstream. We have patches on the ARM that
> are under review for this SoC, with more drivers undergoing cleanup in
> order to submit them to the relevant maintainers.

Why are you merging all SoC trees together into one place?  That seems
like a nightmare to manage, especially with git.

> > And if you do have out-of-tree code, why not use a process that makes it
> > trivial to update the base kernel version so that you can keep up to
> > date very easily?  (hint, just using 'git' is not a good way to do
> > this...)
> 
> We have a process that we've been developing under for the past few
> years. I find git to be a great tool for managing Linux kernel trees.
> 
> What would you recommend for managing kernel trees?

quilt is best for a tree that you can not rebase (i.e. a public git
tree).  Otherwise you end up getting patches all mushed together and
hard to extract in any simple way.

Take a clue from the distros that have been managing kernels for decades
and deal with an updated kernel all the time easily.

Good luck, it sounds like you will need it :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Benjamin Herrenschmidt]

On Thu, 2018-01-11 at 08:30 +0100, Greg KH wrote:
> 4.13?  Why that kernel?  It too is obsolete and insecure and
> unsupported.

Haha, it's n-1. come on :-)


> What keeps you all from just always tracking the latest tree from Linus?
> What is in your tree that is not upstream that requires you to have a
> kernel tree at all?

There are a couple of ARM based SoC families for which we are in the
process of rewriting all the driver in upstreamable form. This takes
time.

To respond to your other email about the USB CDC, it's mine, I haven't
resubmited it yet because it had a dependency on some the aspeed clk
driver to function properly (so is unusable without it) and it took 2
kernel versions to get that clk stuff upstream for a number of reasons.

So it's all getting upstream and eventually there will be (we hope) no
"OpenBMC" kernel, it's just a way for us to get functional code with
non-upstream-quality (read: vendor) drivers until we are one rewriting
& upstreaming them all.

> And if you do have out-of-tree code, why not use a process that makes it
> trivial to update the base kernel version so that you can keep up to
> date very easily?  (hint, just using 'git' is not a good way to do
> this...)

Joel and I both find git perfectly fine for that. I've not touched
quilt in eons and frankly don't regret it ;-)

That said, Jae should definitely submit a driver against upstream, not
against some random OpenBMC tree.

Jae, for example when I submitted the original USB stuff back then, I
did it from a local upstream based branch (with just a few hacks to
work around the lack of the clk stuff).

I will rebase it in the next few days to upstream merged with Stephen's
clk tree to get the finally merged clk stuff, verify it works, and
submit patches against upstream.

There should be no mention of dev-4.10 or 4.13 on lkml or other
upstream submission lists. Development work should happen upstream
*first* and eventually be backported to our older kernels while they
exist (hopefully I prefer if we are more aggressive at forward porting
the crappy drivers so we can keep our tree more up to date but that's a
different discussion).

Cheers,
Ben.

> thanks,
> 
> greg k-h

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Benjamin Herrenschmidt]

On Wed, 2018-01-10 at 11:18 +0100, Greg KH wrote:
> On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> > This commit adds driver implementation for Aspeed PECI. Also adds
> > generic peci.h and peci_ioctl.h files to provide compatibility
> > to peci drivers that can be implemented later e.g. Nuvoton's BMC
> > SoC family.
> 
> We don't add code that could be used "sometime in the future".  Only
> include stuff that we use now.
> 
> Please fix up this series based on that and resubmit.  There should not
> be any need for any uapi file then, right?

No Greg, I think you misunderstood (unless I misread myself).

What Jae means is that since PECI is a standard and other drivers
implementing the same ioctl interface and messages will eventually go
upstream, instead of having the ioctl definitions in a driver specific
locations, they go in a generic spot, as they define a generic API for
all PECI drivers, including the one that's getting merged now.

IE. This doesn't add unused stuff, it just puts the API parts of it
into a generic location.

At least that's my understanding from a, granted cursory, look at the
patch.

That said, I do have a problem with the structure definitions of the
various packet types as they use "long" which has a variable size and
unclear alignment. It should be using __u8, __u16 and __u32...

Cheers,
Ben.

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Benjamin Herrenschmidt]

On Tue, 2018-01-09 at 14:31 -0800, Jae Hyun Yoo wrote:
> +struct peci_rd_ia_msr_msg {
> +       unsigned char target;
> +       unsigned char thread_id;
> +       unsigned short address;
> +       unsigned long value;
> +};

Those types are representing messages on the wire ?

In that case those types aren't suitable. For example "long" will have
a different size and alignment for 32 and 64-bit userspace. There are
size-explicit userspace types available.

Also I didn't see any endianness annotations in there. Is that expected
? IE are those wire format packets ?

Cheers,
Ben.

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Arnd Bergmann]

On Thu, Jan 11, 2018 at 9:41 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Jan 11, 2018 at 12:28:48AM -0800, Joel Stanley wrote:
>> On Wed, Jan 10, 2018 at 11:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Jan 10, 2018 at 01:46:34PM -0800, Jae Hyun Yoo wrote:
>> >> Thanks for your pointing it out and I totally agree with you. Actually, we
>> >> are preparing 4.13 update for now and an another update will be followed up.
>> >> As I answered above, I'll rebase this patch set onto the latest kernel.org
>> >> mainline. Sorry for my misunderstanding of upstream process.
>> >
>> > 4.13?  Why that kernel?  It too is obsolete and insecure and
>> > unsupported.
>>
>> It contains support for our hardware that I have integrated from work
>> in progress patches and upstream commits.
>>
>> The OpenBMC project, with myself as the kernel maintainer, have
>> intentions to regularly move to upstream releases. This takes time and
>> effort. This time and effort is balanced with submitting our drivers
>> upstream.
>
> Of course, but please do not have your "users" use a kernel that is
> known to have bugs and can not be supported.  That would not be good at
> all, don't you think?

I've been pretty happy with the progress in merging drivers upstream
for OpenBMC. Of course things always take longer than planned,
but they are getting there. Most servers today are probably running
the aspeed vendor kernel based on linux-2.6.28.10, at least that's
what my workstation runs (and no, I did not connect the BMC to my
home network).

The particular choices of mainline versions (4.10 and 4.13) may be
unfortunate as they are both one off from a longterm release, but
not being stuck on 2.6 is the important first step in order to upstream
stuff.

>> Another silicon vendor has recently joined the project and that brings
>> an entire SoC that is not upstream. We have patches on the ARM that
>> are under review for this SoC, with more drivers undergoing cleanup in
>> order to submit them to the relevant maintainers.
>
> Why are you merging all SoC trees together into one place?  That seems
> like a nightmare to manage, especially with git.

Why would anyone want to have multiple kernel trees just to run
things on different SoCs? ;-)

It's just a collection of device drivers in different stages of getting
upstreamed.

>> > And if you do have out-of-tree code, why not use a process that makes it
>> > trivial to update the base kernel version so that you can keep up to
>> > date very easily?  (hint, just using 'git' is not a good way to do
>> > this...)
>>
>> We have a process that we've been developing under for the past few
>> years. I find git to be a great tool for managing Linux kernel trees.
>>
>> What would you recommend for managing kernel trees?
>
> quilt is best for a tree that you can not rebase (i.e. a public git
> tree).  Otherwise you end up getting patches all mushed together and
> hard to extract in any simple way.

I'm ususally happy with having git with topic branches to make the
rebasing easier. In many cases, you can just leave a topic branch
for a particular subsystem unchanged between versions and just
merge the latest version of those branches until the branch goes
away after upstreaming.

        Arnd

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Benjamin Herrenschmidt]

On Thu, 2018-01-11 at 09:41 +0100, Greg KH wrote:
> On Thu, Jan 11, 2018 at 12:28:48AM -0800, Joel Stanley wrote:
> > On Wed, Jan 10, 2018 at 11:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Wed, Jan 10, 2018 at 01:46:34PM -0800, Jae Hyun Yoo wrote:
> > > > Thanks for your pointing it out and I totally agree with you. Actually, we
> > > > are preparing 4.13 update for now and an another update will be followed up.
> > > > As I answered above, I'll rebase this patch set onto the latest kernel.org
> > > > mainline. Sorry for my misunderstanding of upstream process.
> > > 
> > > 4.13?  Why that kernel?  It too is obsolete and insecure and
> > > unsupported.
> > 
> > It contains support for our hardware that I have integrated from work
> > in progress patches and upstream commits.
> > 
> > The OpenBMC project, with myself as the kernel maintainer, have
> > intentions to regularly move to upstream releases. This takes time and
> > effort. This time and effort is balanced with submitting our drivers
> > upstream.
> 
> Of course, but please do not have your "users" use a kernel that is
> known to have bugs and can not be supported.  That would not be good at
> all, don't you think?

There is little choice, we don't have the manpower to rewrite/upstream
all the drivers in a day, and rebasing to newer kernel takes time due
to various dependencies, testing requirements etc.

That being said, 4.13 is N-1, not too bad.

> > > What keeps you all from just always tracking the latest tree from Linus?
> > 
> > Linus' tree does not contain all of the drivers required to boot
> > systems. Many of them are still under review on lkml, and others still
> > require rewrite from the vendor tree.
> 
> Merging vendor trees into your tree has got to be a complicated mess.
> Why try to keep it all together in one place?

There are no vendor trees to speak of that we could merge. At least not
yet. We've been teaching vendors about doing proper drivers that can
work with upstream, device-tree based platforms etc... but it takes
time for them to get up to speed.

So while we are rewriting the drivers (sometimes with the vendor's
help), we keep a tree with the "sub-standard" ones hacked up to work on
our DT based platform and with our other bits and pieces until we can
ditch it.

> And who is responsible for getting the vendor code upstream?  The
> individual drivers?  Individual driver submissions should be quite easy,
> what is preventing them from getting merged?

It just takes time Greg. The original vendor drivers are in no shape to
get anywhere near upstream. They have to be mostly rewritten one by
one. Sometimes we can teach the vendor and help them along, some times
we do it ourselves, but it's a time consuming process. It took 2 or 3
kernel versions just to get the clk drivers that Joel had written
upstream for example. It took me a long time as well to get ftgmac100
sorted.

> > > What is in your tree that is not upstream that requires you to have a
> > > kernel tree at all?
> > 
> > We have PECI, video compression, crypto, USB CDC, DRM (graphics),
> > serial GPIO, LPC mailbox for the ASPEED SoC.
> 
> What "USB CDC" do you have that is not upstream? 

See my other email :) 

>  I'll pick on this one
> specifically as I don't think I've seen any patches recently submitted
> for that driver at all.  Am I just missing them?
> 
> The other ones should also all be easy to get merged, with maybe the
> exception of the drm stuff due to the speed that subsystem moves at.
> But even there, the community is very helpful in getting stuff upstream,
> have you asked for help?

We can't expect the community to rewrite the drivers for us. Some of
the clk and pinmux stuff is intrinsically very complex, and took time
(and we did get valuable feedback).

Now that these basic pieces of infrastructure are in, it's a matter of
tackling the remaining drivers one at a time.

> > Another silicon vendor has recently joined the project and that brings
> > an entire SoC that is not upstream. We have patches on the ARM that
> > are under review for this SoC, with more drivers undergoing cleanup in
> > order to submit them to the relevant maintainers.
> 
> Why are you merging all SoC trees together into one place?  That seems
> like a nightmare to manage, especially with git.

There are no SoC trees per-se.

There is the OpenBMC tree which has hand-hacked vendor drivers plugged
into it, which are going away one at a time as we clean them up.
There's really one SoC family only in use (aspeed) at the moment with
one other coming around the corner (Nuvoton). For the latter, my
understanding is that we are trying to get the vendor to get their
stuff upstream directly.

> > > And if you do have out-of-tree code, why not use a process that makes it
> > > trivial to update the base kernel version so that you can keep up to
> > > date very easily?  (hint, just using 'git' is not a good way to do
> > > this...)
> > 
> > We have a process that we've been developing under for the past few
> > years. I find git to be a great tool for managing Linux kernel trees.
> > 
> > What would you recommend for managing kernel trees?
> 
> quilt is best for a tree that you can not rebase (i.e. a public git
> tree).  Otherwise you end up getting patches all mushed together and
> hard to extract in any simple way.
>
> Take a clue from the distros that have been managing kernels for decades
> and deal with an updated kernel all the time easily.
> 
> Good luck, it sounds like you will need it :)

Nah, not luck, we just need to get those drivers done one at a time,
it's not a matter of luck.

And I find git to be just fine :)

Cheers,
Ben.

> thanks,
> 
> greg k-h

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Greg KH]

On Thu, Jan 11, 2018 at 07:56:51PM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2018-01-11 at 08:30 +0100, Greg KH wrote:
> > 4.13?  Why that kernel?  It too is obsolete and insecure and
> > unsupported.
> 
> Haha, it's n-1. come on :-)

And, if you use it in a device, it's still totally unsupported and
insecure.  Seriously, does no one actually pay attention to the patches
I merge in the stable trees anymore?

Anyway, your other comments are good, glad to see work is progressing
well, and yes it's better than a 2.6.y based kernel, but really, that's
a low bar...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Arnd Bergmann]

On Thu, Jan 11, 2018 at 12:45 AM, Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
> On 1/10/2018 4:29 AM, Arnd Bergmann wrote:
>>
>> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>
>>> This commit adds driver implementation for a generic PECI hwmon.
>>>
>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>
>>
>>> +static int xfer_peci_msg(int cmd, void *pmsg)
>>> +{
>>> +       int rc;
>>> +
>>> +       mutex_lock(&peci_hwmon_lock);
>>> +       rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
>>> +       mutex_unlock(&peci_hwmon_lock);
>>> +
>>> +       return rc;
>>> +}
>>
>>
>> I said earlier that peci_ioctl() looked unused, that was obviously
>> wrong, but what you have here
>> is not a proper way to abstract a bus.
>>
>> Maybe this can be done more like an i2c bus: make the peci controller
>> a bus device
>> and register all known target/index pairs as devices with the peci bus
>> type, and have
>> them probed from DT. The driver can then bind to each of those
>> individually.
>> Not sure if that is getting to granular at that point, I'd have to
>> understand better
>> how it is expected to get used, and what the variances are between
>> implementations.
>>
>
> Thanks for sharing your opinion. In fact, this was also suggested by openbmc
> community so I should consider of redesigning it. I'm currently thinking
> about adding a new PECI device class as an abstract layer and any BMC
> chipset specific driver could be attached to the PECI class driver. Then,
> each CPU client could be registered as an individual device as you
> suggested. Will consider your suggestion.

Another idea might be to pretend that PECI was I2C. We already have a few
drivers for hardware that is not I2C but whose software interface looks
similar enough that it just works. No idea if that is the case for PECI, but
xfer_peci_msg might be close enough to i2c_xfer to make it work. If you
are able to do that, then the PECI controller would just register itself
as an i2c controller and it can be accessed using /dev/i2c from user space
or a high-level i2c_driver.

      Arnd

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Jae Hyun Yoo]

On 1/10/2018 1:47 PM, Guenter Roeck wrote:
> On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
>> This commit adds driver implementation for a generic PECI hwmon.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>   drivers/hwmon/Kconfig      |   6 +
>>   drivers/hwmon/Makefile     |   1 +
>>   drivers/hwmon/peci-hwmon.c | 953 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 960 insertions(+)
>>   create mode 100644 drivers/hwmon/peci-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 9256dd0..3a62c60 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1234,6 +1234,12 @@ config SENSORS_NCT7904
>>   	  This driver can also be built as a module.  If so, the module
>>   	  will be called nct7904.
>>   
>> +config SENSORS_PECI_HWMON
>> +	tristate "PECI hwmon support"
>> +	depends on ASPEED_PECI
>> +	help
>> +	  If you say yes here you get support for the generic PECI hwmon driver.
>> +
>>   config SENSORS_NSA320
>>   	tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors"
>>   	depends on GPIOLIB && OF
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 98000fc..41d43a5 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -131,6 +131,7 @@ obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
>>   obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
>>   obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
>>   obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
>> +obj-$(CONFIG_SENSORS_PECI_HWMON)	+= peci-hwmon.o
>>   obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>>   obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>>   obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
>> diff --git a/drivers/hwmon/peci-hwmon.c b/drivers/hwmon/peci-hwmon.c
>> new file mode 100644
>> index 0000000..2d2a288
>> --- /dev/null
>> +++ b/drivers/hwmon/peci-hwmon.c
>> @@ -0,0 +1,953 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017 Intel Corporation
>> +
>> +#include <linux/delay.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/syscalls.h>
>> +#include <misc/peci.h>
> 
> misc, not linux ? That seems wrong.
> 

You are right. I'll fix it.

>> +
>> +#define DEVICE_NAME "peci-hwmon"
>> +#define HWMON_NAME "peci_hwmon"
>> +
>> +#define CPU_ID_MAX           8   /* Max CPU number configured by socket ID */
>> +#define DIMM_NUMS_MAX        16  /* Max DIMM numbers (channel ranks x 2) */
>> +#define CORE_NUMS_MAX        28  /* Max core numbers (max on SKX Platinum) */
> 
> I won't insist, but it would be better if this were dynamic,
> otherwise we'll end up having to increase the defines in the future.
> 

Right. As you said, these values should be manually adjusted in the 
future if CPU architecture has been changed so better implement it as 
dynamic. I will check again a way of getting these values from client 
CPU thru PECI connection.

>> +#define TEMP_TYPE_PECI       6   /* Sensor type 6: Intel PECI */
>> +#define CORE_INDEX_OFFSET    100 /* sysfs filename start offset for core temp */
>> +#define DIMM_INDEX_OFFSET    200 /* sysfs filename start offset for DIMM temp */
> 
> Did you test with the "sensors" command to ensure that this works,
> with the large gaps in index values ?
> 
> Overall, I am not very happy with the indexing. Since each sensor as
> a label, it might be better to just make it dynamic.
> 

Okay, that makes sense. Since all attributes has its own label, indexing 
gap wouldn't be needed even in case of CPU architecture change happens. 
I'll remove the indexing gap.

>> +#define TEMP_NAME_HEADER_LEN 4   /* sysfs temp type header length */
>> +#define OF_DIMM_NUMS_DEFAULT 16  /* default dimm-nums setting */
>> +
>> +#define CORE_TEMP_ATTRS      5
>> +#define DIMM_TEMP_ATTRS      2
>> +#define ATTR_NAME_LEN        24
>> +
>> +#define UPDATE_INTERVAL_MIN  HZ
>> +
>> +enum sign_t {
>> +	POS,
>> +	NEG
>> +};
>> +
>> +struct cpuinfo_t {
>> +	bool valid;
>> +	u32  dib;
>> +	u8   cpuid;
>> +	u8   platform_id;
>> +	u32  microcode;
>> +	u8   logical_thread_nums;
>> +};
>> +
>> +struct temp_data_t {
>> +	bool valid;
>> +	s32  value;
>> +	unsigned long last_updated;
>> +};
>> +
>> +struct temp_group_t {
>> +	struct temp_data_t tjmax;
>> +	struct temp_data_t tcontrol;
>> +	struct temp_data_t tthrottle;
>> +	struct temp_data_t dts_margin;
>> +	struct temp_data_t die;
>> +	struct temp_data_t core[CORE_NUMS_MAX];
>> +	struct temp_data_t dimm[DIMM_NUMS_MAX];
>> +};
>> +
>> +struct core_temp_attr_group_t {
>> +	struct sensor_device_attribute sd_attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS];
>> +	char attr_name[CORE_NUMS_MAX][CORE_TEMP_ATTRS][ATTR_NAME_LEN];
>> +	struct attribute *attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS + 1];
>> +	struct attribute_group attr_group[CORE_NUMS_MAX];
>> +};
>> +
>> +struct dimm_temp_attr_group_t {
>> +	struct sensor_device_attribute sd_attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS];
>> +	char attr_name[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
>> +	struct attribute *attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS + 1];
>> +	struct attribute_group attr_group[DIMM_NUMS_MAX];
>> +};
>> +
>> +struct peci_hwmon {
>> +	struct device *dev;
>> +	struct device *hwmon_dev;
>> +	char name[NAME_MAX];
>> +	const struct attribute_group **groups;
>> +	struct cpuinfo_t cpuinfo;
>> +	struct temp_group_t temp;
>> +	u32 cpu_id;
>> +	bool show_core;
>> +	u32 core_nums;
>> +	u32 dimm_nums;
>> +	atomic_t core_group_created;
>> +	struct core_temp_attr_group_t core;
>> +	struct dimm_temp_attr_group_t dimm;
>> +};
>> +
>> +enum label_t {
>> +	L_DIE,
>> +	L_DTS,
>> +	L_TCONTROL,
>> +	L_TTHROTTLE,
>> +	L_MAX
>> +};
>> +
>> +static const char *peci_label[L_MAX] = {
>> +	"Die temperature\n",
>> +	"DTS thermal margin to Tcontrol\n",
>> +	"Tcontrol temperature\n",
>> +	"Tthrottle temperature\n",
> 
> "temperature" is redundant for a temperature label.
> 

Agreed. Will remove the redundant string.

>> +};
>> +
>> +static DEFINE_MUTEX(peci_hwmon_lock);
>> +
>> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no);
> 
> Please avoid forward declarations.
> 

Will fix it.

>> +
>> +
> 
> Please run your patches throuch checkpatch --strict and fix what it reports,
> or provide a reason why you don't.
> 

Thanks for the tip. I didn't know about the --strict option before. 
Checked that the option reports more check points such as this multiple 
blank line case. I will run all patches again using --strict option.

>> +static int xfer_peci_msg(int cmd, void *pmsg)
>> +{
>> +	int rc;
>> +
>> +	mutex_lock(&peci_hwmon_lock);
>> +	rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
>> +	mutex_unlock(&peci_hwmon_lock);
>> +
>> +	return rc;
>> +}
>> +
>> +static int get_cpuinfo(struct peci_hwmon *priv)
>> +{
>> +	struct peci_get_dib_msg dib_msg;
>> +	struct peci_rd_pkg_cfg_msg cfg_msg;
>> +	int rc, i;
>> +
>> +	if (!priv->cpuinfo.valid) {
>> +		dib_msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +
>> +		rc = xfer_peci_msg(PECI_IOC_GET_DIB, (void *)&dib_msg);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		priv->cpuinfo.dib = dib_msg.dib;
>> +
>> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +		cfg_msg.index = MBX_INDEX_CPU_ID;
>> +		cfg_msg.param = 0;
>> +		cfg_msg.rx_len = 4;
>> +
>> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		priv->cpuinfo.cpuid = cfg_msg.pkg_config[0];
>> +
>> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +		cfg_msg.index = MBX_INDEX_CPU_ID;
>> +		cfg_msg.param = 1;
>> +		cfg_msg.rx_len = 4;
>> +
>> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		priv->cpuinfo.platform_id = cfg_msg.pkg_config[0];
>> +
>> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +		cfg_msg.index = MBX_INDEX_CPU_ID;
>> +		cfg_msg.param = 3;
>> +		cfg_msg.rx_len = 4;
>> +
>> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		priv->cpuinfo.logical_thread_nums = cfg_msg.pkg_config[0] + 1;
>> +
>> +		cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +		cfg_msg.index = MBX_INDEX_CPU_ID;
>> +		cfg_msg.param = 4;
>> +		cfg_msg.rx_len = 4;
>> +
>> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		priv->cpuinfo.microcode = (cfg_msg.pkg_config[3] << 24) |
>> +					  (cfg_msg.pkg_config[2] << 16) |
>> +					  (cfg_msg.pkg_config[1] << 8) |
>> +					  cfg_msg.pkg_config[0];
>> +
>> +		priv->core_nums = priv->cpuinfo.logical_thread_nums / 2;
> 
> This seems to assume a 1:2 relationship between number of threads and
> number of CPUs, which is incorrect.
> 

You are right. This can't cover all CPUs. Will find a proper way.

>> +
>> +		if (priv->show_core &&
>> +		    atomic_inc_return(&priv->core_group_created) == 1) {
>> +			for (i = 0; i < priv->core_nums; i++) {
>> +				rc = create_core_temp_group(priv, i);
> 
> This is messy. Sensor groups should be created before or during
> hwmon registration, not at some arbitrary later time.
> 
> I don't know the logic behind this, but if it is supposed to track CPUs
> coming online and going offline it is the wrong approach.
> 

Agreed. This driver wouldn't make communication with CPUs at all if CPUs 
are powered down so we don't need to leave this driver as inserted. As 
you commented below, if a agent does insert/remove this driver module 
while tracking CPU power state, this delayed creation logic wouldn't be 
needed. I will rewrite it.

>> +				if (rc != 0) {
>> +					dev_err(priv->dev,
>> +						"Failed to create core temp group\n");
>> +					for (--i; i >= 0; i--) {
>> +						sysfs_remove_group(
>> +						     &priv->hwmon_dev->kobj,
>> +						     &priv->core.attr_group[i]);
>> +					}
>> +					atomic_set(&priv->core_group_created,
>> +						   0);
>> +					return rc;
>> +				}
>> +			}
>> +		}
>> +
>> +		priv->cpuinfo.valid = true;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_tjmax(struct peci_hwmon *priv)
>> +{
>> +	struct peci_rd_pkg_cfg_msg msg;
>> +	int rc;
>> +
>> +	rc = get_cpuinfo(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	if (!priv->temp.tjmax.valid) {
>> +		msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +		msg.index = MBX_INDEX_TEMP_TARGET;
>> +		msg.param = 0;
>> +		msg.rx_len = 4;
>> +
>> +		rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		priv->temp.tjmax.value = (s32)msg.pkg_config[2] * 1000;
>> +		priv->temp.tjmax.valid = true;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_tcontrol(struct peci_hwmon *priv)
>> +{
>> +	struct peci_rd_pkg_cfg_msg msg;
>> +	s32 tcontrol_margin;
>> +	int rc;
>> +
>> +	if (priv->temp.tcontrol.valid &&
>> +	    time_before(jiffies, priv->temp.tcontrol.last_updated +
>> +				 UPDATE_INTERVAL_MIN))
>> +		return 0;
>> +
> 
> Is the delay necessary ? Otherwise I would suggest to drop it.
> It adds a lot of complexity to the driver. Also, if the user polls
> values more often, that is presumably on purpose.
> 

I was intended to reduce traffic on PECI bus because it's low speed 
single wired bus, and temperature values don't change frequently because 
the value is sampled and averaged in CPU itself. I'll keep this.

>> +	rc = get_tjmax(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +	msg.index = MBX_INDEX_TEMP_TARGET;
>> +	msg.param = 0;
>> +	msg.rx_len = 4;
>> +
>> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	tcontrol_margin = msg.pkg_config[1];
>> +	tcontrol_margin = ((tcontrol_margin ^ 0x80) - 0x80) * 1000;
>> +
>> +	priv->temp.tcontrol.value = priv->temp.tjmax.value - tcontrol_margin;
>> +
>> +	if (!priv->temp.tcontrol.valid) {
>> +		priv->temp.tcontrol.last_updated = INITIAL_JIFFIES;
>> +		priv->temp.tcontrol.valid = true;
>> +	} else {
>> +		priv->temp.tcontrol.last_updated = jiffies;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_tthrottle(struct peci_hwmon *priv)
>> +{
>> +	struct peci_rd_pkg_cfg_msg msg;
>> +	s32 tthrottle_offset;
>> +	int rc;
>> +
>> +	if (priv->temp.tthrottle.valid &&
>> +	    time_before(jiffies, priv->temp.tthrottle.last_updated +
>> +				 UPDATE_INTERVAL_MIN))
>> +		return 0;
>> +
>> +	rc = get_tjmax(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +	msg.index = MBX_INDEX_TEMP_TARGET;
>> +	msg.param = 0;
>> +	msg.rx_len = 4;
>> +
>> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	tthrottle_offset = (msg.pkg_config[3] & 0x2f) * 1000;
>> +	priv->temp.tthrottle.value = priv->temp.tjmax.value - tthrottle_offset;
>> +
>> +	if (!priv->temp.tthrottle.valid) {
>> +		priv->temp.tthrottle.last_updated = INITIAL_JIFFIES;
>> +		priv->temp.tthrottle.valid = true;
>> +	} else {
>> +		priv->temp.tthrottle.last_updated = jiffies;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_die_temp(struct peci_hwmon *priv)
>> +{
>> +	struct peci_get_temp_msg msg;
>> +	int rc;
>> +
>> +	if (priv->temp.die.valid &&
>> +	    time_before(jiffies, priv->temp.die.last_updated +
>> +				 UPDATE_INTERVAL_MIN))
>> +		return 0;
>> +
>> +	rc = get_tjmax(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +
>> +	rc = xfer_peci_msg(PECI_IOC_GET_TEMP, (void *)&msg);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	priv->temp.die.value = priv->temp.tjmax.value +
>> +			       ((s32)msg.temp_raw * 1000 / 64);
>> +
>> +	if (!priv->temp.die.valid) {
>> +		priv->temp.die.last_updated = INITIAL_JIFFIES;
>> +		priv->temp.die.valid = true;
>> +	} else {
>> +		priv->temp.die.last_updated = jiffies;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_dts_margin(struct peci_hwmon *priv)
>> +{
>> +	struct peci_rd_pkg_cfg_msg msg;
>> +	s32 dts_margin;
>> +	int rc;
>> +
>> +	if (priv->temp.dts_margin.valid &&
>> +	    time_before(jiffies, priv->temp.dts_margin.last_updated +
>> +				 UPDATE_INTERVAL_MIN))
>> +		return 0;
>> +
> Are all those values expected to change dynamically, or are some static ?
> Static values do not have to be re-read repeatedly but can be cached
> permanently.
> 

All values expected to change dynamically except the Tjmax value which 
is fused in CPU package, but the Tjmax varies on each CPU package so we 
need to read the Tjmax at least once. Current implementation uses cached 
Tjmax value after the first reading on the value.

>> +	rc = get_cpuinfo(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +	msg.index = MBX_INDEX_DTS_MARGIN;
>> +	msg.param = 0;
>> +	msg.rx_len = 4;
>> +
>> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
>> +
>> +	/*
>> +	 * Processors return a value of DTS reading in 10.6 format
>> +	 * (10 bits signed decimal, 6 bits fractional).
>> +	 * Error codes:
>> +	 *   0x8000: General sensor error
>> +	 *   0x8001: Reserved
>> +	 *   0x8002: Underflow on reading value
>> +	 *   0x8003-0x81ff: Reserved
>> +	 */
>> +	if (dts_margin >= 0x8000 && dts_margin <= 0x81ff)
>> +		return -1;
>> +
>> +	dts_margin = ((dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
>> +
> The above code is repeated several times. Please consider moving it
> into a function to reduce duplication.
> 

Okay. I will move it to a function.

>> +	priv->temp.dts_margin.value = dts_margin;
>> +
>> +	if (!priv->temp.dts_margin.valid) {
>> +		priv->temp.dts_margin.last_updated = INITIAL_JIFFIES;
>> +		priv->temp.dts_margin.valid = true;
>> +	} else {
>> +		priv->temp.dts_margin.last_updated = jiffies;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_core_temp(struct peci_hwmon *priv, int core_index)
>> +{
>> +	struct peci_rd_pkg_cfg_msg msg;
>> +	s32 core_dts_margin;
>> +	int rc;
>> +
>> +	if (priv->temp.core[core_index].valid &&
>> +	    time_before(jiffies, priv->temp.core[core_index].last_updated +
>> +				 UPDATE_INTERVAL_MIN))
>> +		return 0;
>> +
>> +	rc = get_tjmax(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +	msg.index = MBX_INDEX_PER_CORE_DTS_TEMP;
>> +	msg.param = core_index;
>> +	msg.rx_len = 4;
>> +
>> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	core_dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
>> +
>> +	/*
>> +	 * Processors return a value of the core DTS reading in 10.6 format
>> +	 * (10 bits signed decimal, 6 bits fractional).
>> +	 * Error codes:
>> +	 *   0x8000: General sensor error
>> +	 *   0x8001: Reserved
>> +	 *   0x8002: Underflow on reading value
>> +	 *   0x8003-0x81ff: Reserved
>> +	 */
>> +	if (core_dts_margin >= 0x8000 && core_dts_margin <= 0x81ff)
>> +		return -1;
>> +
>> +	core_dts_margin = ((core_dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
>> +
>> +	priv->temp.core[core_index].value = priv->temp.tjmax.value +
>> +					    core_dts_margin;
>> +
>> +	if (!priv->temp.core[core_index].valid) {
>> +		priv->temp.core[core_index].last_updated = INITIAL_JIFFIES;
>> +		priv->temp.core[core_index].valid = true;
>> +	} else {
>> +		priv->temp.core[core_index].last_updated = jiffies;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_dimm_temp(struct peci_hwmon *priv, int dimm_index)
>> +{
>> +	struct peci_rd_pkg_cfg_msg msg;
>> +	int channel_rank = dimm_index / 2;
>> +	int dimm_order = dimm_index % 2;
>> +	int rc;
>> +
>> +	if (priv->temp.core[dimm_index].valid &&
>> +	    time_before(jiffies, priv->temp.core[dimm_index].last_updated +
>> +				 UPDATE_INTERVAL_MIN))
>> +		return 0;
>> +
>> +	rc = get_cpuinfo(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	msg.target = PECI_BASE_ADDR + priv->cpu_id;
>> +	msg.index = MBX_INDEX_DDR_DIMM_TEMP;
>> +	msg.param = channel_rank;
>> +	msg.rx_len = 4;
>> +
>> +	rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	priv->temp.dimm[dimm_index].value = msg.pkg_config[dimm_order] * 1000;
>> +
>> +	if (!priv->temp.dimm[dimm_index].valid) {
>> +		priv->temp.dimm[dimm_index].last_updated = INITIAL_JIFFIES;
>> +		priv->temp.dimm[dimm_index].valid = true;
>> +	} else {
>> +		priv->temp.dimm[dimm_index].last_updated = jiffies;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t show_info(struct device *dev,
>> +			 struct device_attribute *attr,
>> +			 char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = get_cpuinfo(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "dib         : 0x%08x\n"
>> +			    "cpuid       : 0x%x\n"
>> +			    "platform id : %d\n"
>> +			    "stepping    : %d\n"
>> +			    "microcode   : 0x%08x\n"
>> +			    "logical thread nums : %d\n",
>> +			    priv->cpuinfo.dib,
>> +			    priv->cpuinfo.cpuid,
>> +			    priv->cpuinfo.platform_id,
>> +			    priv->cpuinfo.cpuid & 0xf,
>> +			    priv->cpuinfo.microcode,
>> +			    priv->cpuinfo.logical_thread_nums);
>> +}
> 
> Please no non-standard attributes, much less attributes not following sysfs
> attribute rules. If you want to display such information, consider using
> debugfs. Besides, this information specifically appears to duplicate
> the content of /proc/cpuid, which doesn't really add any value at all.
> 

Got it. Will drop this non-standard attribute.

>> +
>> +static ssize_t show_tcontrol(struct device *dev,
>> +			     struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = get_tcontrol(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", priv->temp.tcontrol.value);
>> +}
>> +
>> +static ssize_t show_tcontrol_margin(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +	int rc;
>> +
>> +	rc = get_tcontrol(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", sensor_attr->index == POS ?
>> +				    priv->temp.tjmax.value -
>> +				    priv->temp.tcontrol.value :
>> +				    priv->temp.tcontrol.value -
>> +				    priv->temp.tjmax.value);
>> +}
>> +
>> +static ssize_t show_tthrottle(struct device *dev,
>> +			      struct device_attribute *attr,
>> +			      char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = get_tthrottle(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", priv->temp.tthrottle.value);
>> +}
>> +
>> +static ssize_t show_tjmax(struct device *dev,
>> +			  struct device_attribute *attr,
>> +			  char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = get_tjmax(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", priv->temp.tjmax.value);
>> +}
>> +
>> +static ssize_t show_die_temp(struct device *dev,
>> +			     struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = get_die_temp(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", priv->temp.die.value);
>> +}
>> +
>> +static ssize_t show_dts_therm_margin(struct device *dev,
>> +				     struct device_attribute *attr,
>> +				     char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	int rc;
>> +
>> +	rc = get_dts_margin(priv);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", priv->temp.dts_margin.value);
>> +}
>> +
>> +static ssize_t show_core_temp(struct device *dev,
>> +			      struct device_attribute *attr,
>> +			      char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +	int core_index = sensor_attr->index;
>> +	int rc;
>> +
>> +	rc = get_core_temp(priv, core_index);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", priv->temp.core[core_index].value);
>> +}
>> +
>> +static ssize_t show_dimm_temp(struct device *dev,
>> +			      struct device_attribute *attr,
>> +			      char *buf)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +	int dimm_index = sensor_attr->index;
>> +	int rc;
>> +
>> +	rc = get_dimm_temp(priv, dimm_index);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return sprintf(buf, "%d\n", priv->temp.dimm[dimm_index].value);
>> +}
>> +
>> +static ssize_t show_value(struct device *dev,
>> +			  struct device_attribute *attr,
>> +			  char *buf)
>> +{
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +
>> +	return sprintf(buf, "%d\n", sensor_attr->index);
>> +}
>> +
>> +static ssize_t show_label(struct device *dev,
>> +			  struct device_attribute *attr,
>> +			  char *buf)
>> +{
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +
>> +	return sprintf(buf, peci_label[sensor_attr->index]);
>> +}
>> +
>> +static ssize_t show_core_label(struct device *dev,
>> +			       struct device_attribute *attr,
>> +			       char *buf)
>> +{
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +
>> +	return sprintf(buf, "Core #%d temperature\n", sensor_attr->index);
>> +}
> 
> Your label strings are quite long. How does that look like with the
> sensors command ?
> 
> Plus, again, "temperature" in a temperature label is redundant.
> 

Agreed. Will remove the redundant string.

>> +
>> +static ssize_t show_dimm_label(struct device *dev,
>> +			       struct device_attribute *attr,
>> +			       char *buf)
>> +{
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +
>> +	char channel = 'A' + (sensor_attr->index / 2);
>> +	int index = sensor_attr->index % 2;
>> +
>> +	return sprintf(buf, "Channel Rank %c DDR DIMM #%d temperature\n",
>> +		       channel, index);
>> +}
>> +
>> +/* Die temperature */
>> +static SENSOR_DEVICE_ATTR(temp1_label, 0444, show_label, NULL, L_DIE);
>> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_die_temp, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max, 0444, show_tcontrol, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit, 0444, show_tjmax, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0444, show_tcontrol_margin, NULL,
>> +			  POS);
>> +
>> +static struct attribute *die_temp_attrs[] = {
>> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
>> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
>> +	&sensor_dev_attr_temp1_crit.dev_attr.attr,
>> +	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group die_temp_attr_group = {
>> +	.attrs = die_temp_attrs,
>> +};
>> +
>> +/* DTS thermal margin temperature */
>> +static SENSOR_DEVICE_ATTR(temp2_label, 0444, show_label, NULL, L_DTS);
>> +static SENSOR_DEVICE_ATTR(temp2_input, 0444, show_dts_therm_margin, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp2_min, 0444, show_value, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp2_lcrit, 0444, show_tcontrol_margin, NULL, NEG);
>> +
>> +static struct attribute *dts_margin_temp_attrs[] = {
>> +	&sensor_dev_attr_temp2_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
>> +	&sensor_dev_attr_temp2_min.dev_attr.attr,
>> +	&sensor_dev_attr_temp2_lcrit.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group dts_margin_temp_attr_group = {
>> +	.attrs = dts_margin_temp_attrs,
>> +};
>> +
>> +/* Tcontrol temperature */
>> +static SENSOR_DEVICE_ATTR(temp3_label, 0444, show_label, NULL, L_TCONTROL);
>> +static SENSOR_DEVICE_ATTR(temp3_input, 0444, show_tcontrol, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp3_crit, 0444, show_tjmax, NULL, 0);
>> +
>> +static struct attribute *tcontrol_temp_attrs[] = {
>> +	&sensor_dev_attr_temp3_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
>> +	&sensor_dev_attr_temp3_crit.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group tcontrol_temp_attr_group = {
>> +	.attrs = tcontrol_temp_attrs,
>> +};
>> +
>> +/* Tthrottle temperature */
>> +static SENSOR_DEVICE_ATTR(temp4_label, 0444, show_label, NULL, L_TTHROTTLE);
>> +static SENSOR_DEVICE_ATTR(temp4_input, 0444, show_tthrottle, NULL, 0);
>> +
>> +static struct attribute *tthrottle_temp_attrs[] = {
>> +	&sensor_dev_attr_temp4_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp4_input.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group tthrottle_temp_attr_group = {
>> +	.attrs = tthrottle_temp_attrs,
>> +};
>> +
>> +/* CPU info */
>> +static SENSOR_DEVICE_ATTR(info, 0444, show_info, NULL, 0);
>> +
>> +static struct attribute *info_attrs[] = {
>> +	&sensor_dev_attr_info.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group info_attr_group = {
>> +	.attrs = info_attrs,
>> +};
>> +
>> +const struct attribute_group *peci_hwmon_attr_groups[] = {
>> +	&info_attr_group,
>> +	&die_temp_attr_group,
>> +	&dts_margin_temp_attr_group,
>> +	&tcontrol_temp_attr_group,
>> +	&tthrottle_temp_attr_group,
>> +	NULL
>> +};
>> +
>> +static ssize_t (*const core_show_fn[CORE_TEMP_ATTRS]) (struct device *dev,
>> +		struct device_attribute *devattr, char *buf) = {
>> +	show_core_label,
>> +	show_core_temp,
>> +	show_tcontrol,
>> +	show_tjmax,
>> +	show_tcontrol_margin,
>> +};
>> +
>> +static const char *const core_suffix[CORE_TEMP_ATTRS] = {
>> +	"label",
>> +	"input",
>> +	"max",
>> +	"crit",
>> +	"crit_hyst",
>> +};
>> +
>> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < CORE_TEMP_ATTRS; i++) {
>> +		snprintf(priv->core.attr_name[core_no][i],
>> +			 ATTR_NAME_LEN, "temp%d_%s",
>> +			 CORE_INDEX_OFFSET + core_no, core_suffix[i]);
>> +		sysfs_attr_init(
>> +			    &priv->core.sd_attrs[core_no][i].dev_attr.attr);
>> +		priv->core.sd_attrs[core_no][i].dev_attr.attr.name =
>> +					       priv->core.attr_name[core_no][i];
>> +		priv->core.sd_attrs[core_no][i].dev_attr.attr.mode = 0444;
>> +		priv->core.sd_attrs[core_no][i].dev_attr.show = core_show_fn[i];
>> +		if (i == 0 || i == 1) /* label or temp */
>> +			priv->core.sd_attrs[core_no][i].index = core_no;
>> +		priv->core.attrs[core_no][i] =
>> +				 &priv->core.sd_attrs[core_no][i].dev_attr.attr;
>> +	}
>> +
>> +	priv->core.attr_group[core_no].attrs = priv->core.attrs[core_no];
>> +
>> +	return sysfs_create_group(&priv->hwmon_dev->kobj,
>> +				  &priv->core.attr_group[core_no]);
>> +}
>> +
>> +static ssize_t (*const dimm_show_fn[DIMM_TEMP_ATTRS]) (struct device *dev,
>> +		struct device_attribute *devattr, char *buf) = {
>> +	show_dimm_label,
>> +	show_dimm_temp,
>> +};
>> +
>> +static const char *const dimm_suffix[DIMM_TEMP_ATTRS] = {
>> +	"label",
>> +	"input",
>> +};
>> +
>> +static int create_dimm_temp_group(struct peci_hwmon *priv, int dimm_no)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < DIMM_TEMP_ATTRS; i++) {
>> +		snprintf(priv->dimm.attr_name[dimm_no][i],
>> +			 ATTR_NAME_LEN, "temp%d_%s",
>> +			 DIMM_INDEX_OFFSET + dimm_no, dimm_suffix[i]);
>> +		sysfs_attr_init(&priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr);
>> +		priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.name =
>> +					       priv->dimm.attr_name[dimm_no][i];
>> +		priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.mode = 0444;
>> +		priv->dimm.sd_attrs[dimm_no][i].dev_attr.show = dimm_show_fn[i];
>> +		priv->dimm.sd_attrs[dimm_no][i].index = dimm_no;
>> +		priv->dimm.attrs[dimm_no][i] =
>> +				 &priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr;
>> +	}
>> +
>> +	priv->dimm.attr_group[dimm_no].attrs = priv->dimm.attrs[dimm_no];
>> +
>> +	return sysfs_create_group(&priv->hwmon_dev->kobj,
>> +				  &priv->dimm.attr_group[dimm_no]);
>> +}
>> +
>> +static int peci_hwmon_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct peci_hwmon *priv;
>> +	struct device *hwmon;
>> +	int rc, i;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(dev, priv);
>> +	priv->dev = dev;
>> +
>> +	rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);
> 
> What entity determines cpu-id ?
> 

CPU ID numbering is determined by hardware SOCKET_ID strap pins. In this 
driver implementation, cpu-id is being used as CPU client indexing.

>> +	if (rc || priv->cpu_id >= CPU_ID_MAX) {
>> +		dev_err(dev, "Invalid cpu-id configuration\n");
>> +		return rc;
>> +	}
>> +
>> +	rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);
> 
> This is an odd devicetree attribute. Normally the number of DIMMs
> is dynamic. Isn't there a means to get all that information dynamically
> instead of having to set it through devicetree ? What if someone adds
> or removes a DIMM ? Who updates the devicetree ?
> 

It means the number of DIMM slots each CPU has, doesn't mean the number 
of currently installed DIMM components. If a DIMM is inserted a slot, 
CPU reports its actual temperature but on empty slot, CPU reports 0 
instead of reporting an error so it is the reason why this driver 
enumerates all DIMM slots' attribute.

>> +	if (rc || priv->dimm_nums > DIMM_NUMS_MAX) {
>> +		dev_warn(dev, "Invalid dimm-nums : %u. Use default : %u\n",
>> +			 priv->dimm_nums, OF_DIMM_NUMS_DEFAULT);
>> +		priv->dimm_nums = OF_DIMM_NUMS_DEFAULT;
>> +	}
>> +
>> +	priv->show_core = of_property_read_bool(np, "show-core");
> 
> This does not look like an appropriate devicetree attribute.
> 

Okay. I will remove this devicetree attribute and make this driver 
enable core temperature attributes always.

>> +
>> +	priv->groups = peci_hwmon_attr_groups;
>> +
> 
> This assignment (and the ->groups variable) is quite pointless.
> 

Will fix it.

>> +	snprintf(priv->name, NAME_MAX, HWMON_NAME ".cpu%d", priv->cpu_id);
>> +
>> +	hwmon = devm_hwmon_device_register_with_groups(dev,
>> +						       priv->name,
>> +						       priv, priv->groups);
> 
> Please rewrite the driver to use devm_hwmon_device_register_with_info(),
> and avoid dynamic attributes.
> 

I will rewrite it.

>> +
>> +	rc = PTR_ERR_OR_ZERO(hwmon);
>> +	if (rc != 0) {
>> +		dev_err(dev, "Failed to register peci hwmon\n");
>> +		return rc;
>> +	}
>> +
>> +	priv->hwmon_dev = hwmon;
> 
> Something is logically wrong if you need to store hwmon_dev in the
> private data structure. Specifically, creating attributes dynamically
> after hwmon registration is wrong.
> 
>> +
>> +	for (i = 0; i < priv->dimm_nums; i++) {
>> +		rc = create_dimm_temp_group(priv, i);
> 
> No. See earlier comments. All attribute groups must be created during
> registration (or before, but I am not inclined to accept a new driver
> doing that).
> 
>> +		if (rc != 0) {
>> +			dev_err(dev, "Failed to create dimm temp group\n");
>> +			for (--i; i >= 0; i--) {
>> +				sysfs_remove_group(&priv->hwmon_dev->kobj,
>> +						   &priv->dimm.attr_group[i]);
>> +			}
>> +			return rc;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Try to create core temp group now. It will be created if CPU is
>> +	 * curretnly online or it will be created after the first reading of
>> +	 * cpuinfo from the online CPU otherwise.
> 
> This is not how CPUs are supposed to be detected, and it does not handle CPUs
> taken offline. If the driver is instantiated as a CPU comes online, or as it
> goes offline, the driver should use the appropriate kernel interfaces to
> trigger that instantiation or removal. However, if so, it may be inappropriate
> to associate CPU temperatures with other system temperatures in the same
> instance of the driver; after all, those are all independent of each other.
> 
> Overall, I suspect that there should be a callback or some other mechanism
> in the peci core to trigger instantiation and removal of this driver, and
> I am not sure if any of the devicetree properties makes sense at all.
> 
> For example, if an instance of this driver is associated with a PECI
> agent (with assorted CPU/DIMM temperature reporting), the instantiation
> could be triggered as soon as the PECI core detects that the agent is
> available, and the PECI core could report what exactly that instance
> supports.
> 

Thanks for sharing your detailed comment. In fact, PECI doesn't have any 
mechanism to get a callback for checking whether CPU is online or not. 
The only way PECI can do is, polling a CPU using the Ping PECI command. 
Also, a BMC controller can't make any PECI communication with offline 
CPU so this implementation uses delayed creation for some attributes 
which is messy as you said.

Like you suggested, a PECI agent could check CPU power state using some 
other mechanism and this driver module could be dynamically 
inserted/removed by the agent according to the CPU power state. This way 
could make all attributes creation possible at probing time so no need 
to use the delayed creation.

Also, I'll check feasibility of dynamic checking for maximum supportable 
numbers on each component type so that we can dynamically set the values 
as you suggested above.

>> +	 */
>> +	if (priv->show_core)
>> +		(void) get_cpuinfo(priv);
>> +
>> +	dev_info(dev, "peci hwmon for CPU#%d registered\n", priv->cpu_id);
> 
> Is this logging noise necessary ? Besides, some of it is redundant.
> 

No, it isn't necessary. I will remove it.

>> +
>> +	return rc;
>> +}
>> +
>> +static int peci_hwmon_remove(struct platform_device *pdev)
>> +{
>> +	struct peci_hwmon *priv = dev_get_drvdata(&pdev->dev);
>> +	int i;
>> +
>> +	if (atomic_read(&priv->core_group_created))
>> +		for (i = 0; i < priv->core_nums; i++) {
>> +			sysfs_remove_group(&priv->hwmon_dev->kobj,
>> +					   &priv->core.attr_group[i]);
>> +		}
>> +
>> +	for (i = 0; i < priv->dimm_nums; i++) {
>> +		sysfs_remove_group(&priv->hwmon_dev->kobj,
>> +				   &priv->dimm.attr_group[i]);
>> +	}
> 
> If you need to call sysfs_remove_group from here,
> something is conceptually wrong in your driver.
> 

I'll remove it while rewriting the driver.

>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id peci_of_table[] = {
>> +	{ .compatible = "peci-hwmon", },
> 
> This does not look like a reference to some piece of hardware.
> 

This driver provides generic PECI hwmon function to which controller has 
PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a 
specific hardware. Should I remove this or any suggestion?

>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, peci_of_table);
>> +
>> +static struct platform_driver peci_hwmon_driver = {
>> +	.probe = peci_hwmon_probe,
>> +	.remove = peci_hwmon_remove,
>> +	.driver = {
>> +		.name           = DEVICE_NAME,
>> +		.of_match_table = peci_of_table,
>> +	},
>> +};
>> +
>> +module_platform_driver(peci_hwmon_driver);
>> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>");
>> +MODULE_DESCRIPTION("PECI hwmon driver");
>> +MODULE_LICENSE("GPL v2");
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Jae Hyun Yoo]

On 1/11/2018 12:56 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2018-01-11 at 08:30 +0100, Greg KH wrote:
>> 4.13?  Why that kernel?  It too is obsolete and insecure and
>> unsupported.
> 
> Haha, it's n-1. come on :-)
> 
> 
>> What keeps you all from just always tracking the latest tree from Linus?
>> What is in your tree that is not upstream that requires you to have a
>> kernel tree at all?
> 
> There are a couple of ARM based SoC families for which we are in the
> process of rewriting all the driver in upstreamable form. This takes
> time.
> 
> To respond to your other email about the USB CDC, it's mine, I haven't
> resubmited it yet because it had a dependency on some the aspeed clk
> driver to function properly (so is unusable without it) and it took 2
> kernel versions to get that clk stuff upstream for a number of reasons.
> 
> So it's all getting upstream and eventually there will be (we hope) no
> "OpenBMC" kernel, it's just a way for us to get functional code with
> non-upstream-quality (read: vendor) drivers until we are one rewriting
> & upstreaming them all.
> 
>> And if you do have out-of-tree code, why not use a process that makes it
>> trivial to update the base kernel version so that you can keep up to
>> date very easily?  (hint, just using 'git' is not a good way to do
>> this...)
> 
> Joel and I both find git perfectly fine for that. I've not touched
> quilt in eons and frankly don't regret it ;-)
> 
> That said, Jae should definitely submit a driver against upstream, not
> against some random OpenBMC tree.
> 
> Jae, for example when I submitted the original USB stuff back then, I
> did it from a local upstream based branch (with just a few hacks to
> work around the lack of the clk stuff).
> 
> I will rebase it in the next few days to upstream merged with Stephen's
> clk tree to get the finally merged clk stuff, verify it works, and
> submit patches against upstream.
> 
> There should be no mention of dev-4.10 or 4.13 on lkml or other
> upstream submission lists. Development work should happen upstream
> *first* and eventually be backported to our older kernels while they
> exist (hopefully I prefer if we are more aggressive at forward porting
> the crappy drivers so we can keep our tree more up to date but that's a
> different discussion).
> 
> Cheers,
> Ben.
> 

Thanks for your reminding me the upstream process. I'll do like you said 
afterwards.

Thanks,
Jae

>> thanks,
>>
>> greg k-h
> 

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/11/2018 1:02 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2018-01-10 at 11:18 +0100, Greg KH wrote:
>> On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
>>> This commit adds driver implementation for Aspeed PECI. Also adds
>>> generic peci.h and peci_ioctl.h files to provide compatibility
>>> to peci drivers that can be implemented later e.g. Nuvoton's BMC
>>> SoC family.
>>
>> We don't add code that could be used "sometime in the future".  Only
>> include stuff that we use now.
>>
>> Please fix up this series based on that and resubmit.  There should not
>> be any need for any uapi file then, right?
> 
> No Greg, I think you misunderstood (unless I misread myself).
> 
> What Jae means is that since PECI is a standard and other drivers
> implementing the same ioctl interface and messages will eventually go
> upstream, instead of having the ioctl definitions in a driver specific
> locations, they go in a generic spot, as they define a generic API for
> all PECI drivers, including the one that's getting merged now.
> 
> IE. This doesn't add unused stuff, it just puts the API parts of it
> into a generic location.
> 
> At least that's my understanding from a, granted cursory, look at the
> patch.
> 
> That said, I do have a problem with the structure definitions of the
> various packet types as they use "long" which has a variable size and
> unclear alignment. It should be using __u8, __u16 and __u32...
> 
> Cheers,
> Ben.
> 

Thanks for your clear explanation. That is what I actually intended to. 
However, the structure definitions you and Greg pointed out need to be 
corrected. I will fix it.

Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/11/2018 1:06 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2018-01-09 at 14:31 -0800, Jae Hyun Yoo wrote:
>> +struct peci_rd_ia_msr_msg {
>> +       unsigned char target;
>> +       unsigned char thread_id;
>> +       unsigned short address;
>> +       unsigned long value;
>> +};
> 
> Those types are representing messages on the wire ?
> 
> In that case those types aren't suitable. For example "long" will have
> a different size and alignment for 32 and 64-bit userspace. There are
> size-explicit userspace types available.
> 
> Also I didn't see any endianness annotations in there. Is that expected
> ? IE are those wire format packets ?
> 
> Cheers,
> Ben.
> 

Only the 'peci_xfer_msg' struct is representing messages on the wire. 
All userspace messages which is using other struct definitions will be 
copied into the 'peci_xfer_msg' for each member variable in driver, but 
anyway, type definitions of each member variable should be fixed as you 
said. Will fix it.

Thanks,
Jae
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Jae Hyun Yoo]

On 1/11/2018 5:22 AM, Arnd Bergmann wrote:
> On Thu, Jan 11, 2018 at 12:45 AM, Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>> On 1/10/2018 4:29 AM, Arnd Bergmann wrote:
>>>
>>> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
>>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>>
>>>> This commit adds driver implementation for a generic PECI hwmon.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>
>>>
>>>> +static int xfer_peci_msg(int cmd, void *pmsg)
>>>> +{
>>>> +       int rc;
>>>> +
>>>> +       mutex_lock(&peci_hwmon_lock);
>>>> +       rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
>>>> +       mutex_unlock(&peci_hwmon_lock);
>>>> +
>>>> +       return rc;
>>>> +}
>>>
>>>
>>> I said earlier that peci_ioctl() looked unused, that was obviously
>>> wrong, but what you have here
>>> is not a proper way to abstract a bus.
>>>
>>> Maybe this can be done more like an i2c bus: make the peci controller
>>> a bus device
>>> and register all known target/index pairs as devices with the peci bus
>>> type, and have
>>> them probed from DT. The driver can then bind to each of those
>>> individually.
>>> Not sure if that is getting to granular at that point, I'd have to
>>> understand better
>>> how it is expected to get used, and what the variances are between
>>> implementations.
>>>
>>
>> Thanks for sharing your opinion. In fact, this was also suggested by openbmc
>> community so I should consider of redesigning it. I'm currently thinking
>> about adding a new PECI device class as an abstract layer and any BMC
>> chipset specific driver could be attached to the PECI class driver. Then,
>> each CPU client could be registered as an individual device as you
>> suggested. Will consider your suggestion.
> 
> Another idea might be to pretend that PECI was I2C. We already have a few
> drivers for hardware that is not I2C but whose software interface looks
> similar enough that it just works. No idea if that is the case for PECI, but
> xfer_peci_msg might be close enough to i2c_xfer to make it work. If you
> are able to do that, then the PECI controller would just register itself
> as an i2c controller and it can be accessed using /dev/i2c from user space
> or a high-level i2c_driver.
> 
>        Arnd
> 

Thanks for the good idea. It looks like one of possible options. I'll 
check this idea as well. :)

Thanks,
Jae

Re: [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers

[Benjamin Herrenschmidt]

On Thu, 2018-01-11 at 10:59 +0100, Greg KH wrote:
> And, if you use it in a device, it's still totally unsupported and
> insecure.  Seriously, does no one actually pay attention to the patches
> I merge in the stable trees anymore?

Yeah not sure why we aren't picking an LTC here, it could be that it's
just a dev branch, not release, Joel, what's the process in that case ?

For anything going into products, if we're going to maintain a non-
upstream branch, we should definitely stick it to an LTS release or
we'll be missing a ton of security fixes.

Cheers,
Ben.

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Guenter Roeck]

On Thu, Jan 11, 2018 at 11:47:01AM -0800, Jae Hyun Yoo wrote:
> On 1/10/2018 1:47 PM, Guenter Roeck wrote:
> >On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
> >>This commit adds driver implementation for a generic PECI hwmon.
> >>
> >>Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

[ ... ]

> >>+
> >>+	if (priv->temp.tcontrol.valid &&
> >>+	    time_before(jiffies, priv->temp.tcontrol.last_updated +
> >>+				 UPDATE_INTERVAL_MIN))
> >>+		return 0;
> >>+
> >
> >Is the delay necessary ? Otherwise I would suggest to drop it.
> >It adds a lot of complexity to the driver. Also, if the user polls
> >values more often, that is presumably on purpose.
> >
> 
> I was intended to reduce traffic on PECI bus because it's low speed single
> wired bus, and temperature values don't change frequently because the value
> is sampled and averaged in CPU itself. I'll keep this.
> 
Then please try to move the common code into a single function.

[ ... ]

> >>+
> >>+	rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);
> >
> >What entity determines cpu-id ?
> >
> 
> CPU ID numbering is determined by hardware SOCKET_ID strap pins. In this
> driver implementation, cpu-id is being used as CPU client indexing.
> 
Seems to me the necessary information to identify a given CPU should
be provided by the PECI core. Also, there are already "cpu" nodes
in devicetree which, if I recall correctly, may include information
such as CPU Ids.

> >>+	if (rc || priv->cpu_id >= CPU_ID_MAX) {
> >>+		dev_err(dev, "Invalid cpu-id configuration\n");
> >>+		return rc;
> >>+	}
> >>+
> >>+	rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);
> >
> >This is an odd devicetree attribute. Normally the number of DIMMs
> >is dynamic. Isn't there a means to get all that information dynamically
> >instead of having to set it through devicetree ? What if someone adds
> >or removes a DIMM ? Who updates the devicetree ?
> >
> 
> It means the number of DIMM slots each CPU has, doesn't mean the number of
> currently installed DIMM components. If a DIMM is inserted a slot, CPU
> reports its actual temperature but on empty slot, CPU reports 0 instead of
> reporting an error so it is the reason why this driver enumerates all DIMM
> slots' attribute.
> 
And there is no other means to get the number of DIMM slots per CPU ?
It just seems to be that this is the wrong location to provide such
information.

[ ... ]

> >>+
> >>+static const struct of_device_id peci_of_table[] = {
> >>+	{ .compatible = "peci-hwmon", },
> >
> >This does not look like a reference to some piece of hardware.
> >
> 
> This driver provides generic PECI hwmon function to which controller has
> PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
> specific hardware. Should I remove this or any suggestion?
> 

I don't really know enough about the system to make a recommendation.
It seems to me that the PECI core should identify which functionality
it supports and instantiate the necessary driver(s). Maybe there should
be sub-nodes to the peci node with relevant information. Those sub-nodes
should specify the supported functionality in more detail, though -
such as indicating the supported CPU and/or DIMM sensors.

Guenter

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Andrew Lunn]

> > >>+static const struct of_device_id peci_of_table[] = {
> > >>+	{ .compatible = "peci-hwmon", },
> > >
> > >This does not look like a reference to some piece of hardware.
> > >
> > 
> > This driver provides generic PECI hwmon function to which controller has
> > PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
> > specific hardware. Should I remove this or any suggestion?

PECI seems to be an Intel thing. So at least it should be

 { .compatible = "intel,peci-hwmon", }

assuming it is actually compatible with the Intel specification.

	 Andrew

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Jae Hyun Yoo]

On 1/11/2018 1:40 PM, Guenter Roeck wrote:
> On Thu, Jan 11, 2018 at 11:47:01AM -0800, Jae Hyun Yoo wrote:
>> On 1/10/2018 1:47 PM, Guenter Roeck wrote:
>>> On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
>>>> This commit adds driver implementation for a generic PECI hwmon.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> [ ... ]
> 
>>>> +
>>>> +	if (priv->temp.tcontrol.valid &&
>>>> +	    time_before(jiffies, priv->temp.tcontrol.last_updated +
>>>> +				 UPDATE_INTERVAL_MIN))
>>>> +		return 0;
>>>> +
>>>
>>> Is the delay necessary ? Otherwise I would suggest to drop it.
>>> It adds a lot of complexity to the driver. Also, if the user polls
>>> values more often, that is presumably on purpose.
>>>
>>
>> I was intended to reduce traffic on PECI bus because it's low speed single
>> wired bus, and temperature values don't change frequently because the value
>> is sampled and averaged in CPU itself. I'll keep this.
>>
> Then please try to move the common code into a single function.
> 

That makes sense. I'll move the common code into a single function.

> [ ... ]
> 
>>>> +
>>>> +	rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);
>>>
>>> What entity determines cpu-id ?
>>>
>>
>> CPU ID numbering is determined by hardware SOCKET_ID strap pins. In this
>> driver implementation, cpu-id is being used as CPU client indexing.
>>
> Seems to me the necessary information to identify a given CPU should
> be provided by the PECI core. Also, there are already "cpu" nodes
> in devicetree which, if I recall correctly, may include information
> such as CPU Ids.
> 

This driver is implemented to support only BMC (Baseboard Management 
Controllers) chipset which is running on a separated linux kernel from a 
host server system. Through a PECI connection between them, this driver 
collects the host server system's CPU and DIMM temperature information 
which is running on a separated OS. That could be a linux, a Windows OS 
or any other OSes. I mean, there is no shared devicetree data between 
them so it is why the 'cpu_id' was added into dt configuration of this 
driver.

Using quite limited hardware connections such as PECI, KSC, eSPI and 
SMBus, the BMC manages the host server and this hwmon is one of features 
of BMC.

>>>> +	if (rc || priv->cpu_id >= CPU_ID_MAX) {
>>>> +		dev_err(dev, "Invalid cpu-id configuration\n");
>>>> +		return rc;
>>>> +	}
>>>> +
>>>> +	rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);
>>>
>>> This is an odd devicetree attribute. Normally the number of DIMMs
>>> is dynamic. Isn't there a means to get all that information dynamically
>>> instead of having to set it through devicetree ? What if someone adds
>>> or removes a DIMM ? Who updates the devicetree ?
>>>
>>
>> It means the number of DIMM slots each CPU has, doesn't mean the number of
>> currently installed DIMM components. If a DIMM is inserted a slot, CPU
>> reports its actual temperature but on empty slot, CPU reports 0 instead of
>> reporting an error so it is the reason why this driver enumerates all DIMM
>> slots' attribute.
>>
> And there is no other means to get the number of DIMM slots per CPU ?
> It just seems to be that this is the wrong location to provide such
> information.
> 

This devicetree attribute will be configured at runtime using dt overlay 
based on the host server's hardware configuration which will be parsed 
and managed by a userspace BMC service.

> [ ... ]
> 
>>>> +
>>>> +static const struct of_device_id peci_of_table[] = {
>>>> +	{ .compatible = "peci-hwmon", },
>>>
>>> This does not look like a reference to some piece of hardware.
>>>
>>
>> This driver provides generic PECI hwmon function to which controller has
>> PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
>> specific hardware. Should I remove this or any suggestion?
>>
> 
> I don't really know enough about the system to make a recommendation.
> It seems to me that the PECI core should identify which functionality
> it supports and instantiate the necessary driver(s). Maybe there should
> be sub-nodes to the peci node with relevant information. Those sub-nodes
> should specify the supported functionality in more detail, though -
> such as indicating the supported CPU and/or DIMM sensors.
> 
> Guenter
> 

As I explained above, BMC and host server are running on separated OSes 
so this driver cannot be (actually, doesn't need to be) directly 
associated with other kernel modules in the BMC side OS for identifying 
the host server system's functionality. My thought is, this driver will 
use PECI only for identifying the host server's CPU and DIMM information 
and the userspace BMC service could deliver any additional host server 
information thru dt overlay if needed before the BMC service initiates 
this driver at runtime.

Thanks a lot,

Jae

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Jae Hyun Yoo]

On 1/11/2018 2:18 PM, Andrew Lunn wrote:
>>>>> +static const struct of_device_id peci_of_table[] = {
>>>>> +	{ .compatible = "peci-hwmon", },
>>>>
>>>> This does not look like a reference to some piece of hardware.
>>>>
>>>
>>> This driver provides generic PECI hwmon function to which controller has
>>> PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
>>> specific hardware. Should I remove this or any suggestion?
> 
> PECI seems to be an Intel thing. So at least it should be
> 
>   { .compatible = "intel,peci-hwmon", }
> 
> assuming it is actually compatible with the Intel specification.
> 
> 	 Andrew
> 

Yes, PECI is an Intel thing but this driver is running on an ARM kernel 
on Aspeed or Nuvoton chipsets for now. This driver will be monitoring a 
host server's Intel CPU and DIMM which is running on a separated OS.

Thanks,
Jae

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Andrew Lunn]

On Thu, Jan 11, 2018 at 03:14:37PM -0800, Jae Hyun Yoo wrote:
> On 1/11/2018 2:18 PM, Andrew Lunn wrote:
> >>>>>+static const struct of_device_id peci_of_table[] = {
> >>>>>+	{ .compatible = "peci-hwmon", },
> >>>>
> >>>>This does not look like a reference to some piece of hardware.
> >>>>
> >>>
> >>>This driver provides generic PECI hwmon function to which controller has
> >>>PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
> >>>specific hardware. Should I remove this or any suggestion?
> >
> >PECI seems to be an Intel thing. So at least it should be
> >
> >  { .compatible = "intel,peci-hwmon", }
> >
> >assuming it is actually compatible with the Intel specification.
> >
> >	 Andrew
> >
> 
> Yes, PECI is an Intel thing but this driver is running on an ARM kernel on
> Aspeed or Nuvoton chipsets for now. This driver will be monitoring a host
> server's Intel CPU and DIMM which is running on a separated OS.

Hi Jae

You need to be careful with the name then. You should not claim the
name 'peci' in case somebody actually implements a PECI driver which
is compatible with Intel PECI.

However, looking at other comments, it seems like this part is going
away, if you turn your code into a bus driver.

      Andrew

Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon

[Jae Hyun Yoo]

On 1/11/2018 3:53 PM, Andrew Lunn wrote:
> On Thu, Jan 11, 2018 at 03:14:37PM -0800, Jae Hyun Yoo wrote:
>> On 1/11/2018 2:18 PM, Andrew Lunn wrote:
>>>>>>> +static const struct of_device_id peci_of_table[] = {
>>>>>>> +	{ .compatible = "peci-hwmon", },
>>>>>>
>>>>>> This does not look like a reference to some piece of hardware.
>>>>>>
>>>>>
>>>>> This driver provides generic PECI hwmon function to which controller has
>>>>> PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
>>>>> specific hardware. Should I remove this or any suggestion?
>>>
>>> PECI seems to be an Intel thing. So at least it should be
>>>
>>>   { .compatible = "intel,peci-hwmon", }
>>>
>>> assuming it is actually compatible with the Intel specification.
>>>
>>> 	 Andrew
>>>
>>
>> Yes, PECI is an Intel thing but this driver is running on an ARM kernel on
>> Aspeed or Nuvoton chipsets for now. This driver will be monitoring a host
>> server's Intel CPU and DIMM which is running on a separated OS.
> 
> Hi Jae
> 
> You need to be careful with the name then. You should not claim the
> name 'peci' in case somebody actually implements a PECI driver which
> is compatible with Intel PECI.
> 
> However, looking at other comments, it seems like this part is going
> away, if you turn your code into a bus driver.
> 
>        Andrew
> 

Hi Andrew,

I see. I'll keep that in mind and will keep finding if there is any 
better way. Thanks for sharing your thought.

Jae