On Wed, 2021-08-25 at 15:58 -0700, Dan Williams wrote:

On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
[off-list ref] wrote:

quoted

Intel processors provide access for various services designed to support
processor and DRAM thermal management, platform manageability and
processor interface tuning and diagnostics.
Those services are available via the Platform Environment Control
Interface (PECI) that provides a communication channel between the
processor and the Baseboard Management Controller (BMC) or other
platform management device.

This change introduces PECI subsystem by adding the initial core module
and API for controller drivers.

Co-developed-by: Jason M Bills <redacted>
Signed-off-by: Jason M Bills <redacted>
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
 MAINTAINERS             |   9 +++
 drivers/Kconfig         |   3 +
 drivers/Makefile        |   1 +
 drivers/peci/Kconfig    |  15 ++++
 drivers/peci/Makefile   |   5 ++
 drivers/peci/core.c     | 155 ++++++++++++++++++++++++++++++++++++++++
 drivers/peci/internal.h |  16 +++++
 include/linux/peci.h    |  99 +++++++++++++++++++++++++
 8 files changed, 303 insertions(+)
 create mode 100644 drivers/peci/Kconfig
 create mode 100644 drivers/peci/Makefile
 create mode 100644 drivers/peci/core.c
 create mode 100644 drivers/peci/internal.h
 create mode 100644 include/linux/peci.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 7cdab7229651..d411974aaa5e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -14503,6 +14503,15 @@ L:     platform-driver-x86@vger.kernel.org

 S:     Maintained
 F:     drivers/platform/x86/peaq-wmi.c

+PECI SUBSYSTEM
+M:     Iwona Winiarska [off-list ref]
+R:     Jae Hyun Yoo [off-list ref]
+L:     openbmc@lists.ozlabs.org (moderated for non-subscribers)
+S:     Supported
+F:     Documentation/devicetree/bindings/peci/
+F:     drivers/peci/
+F:     include/linux/peci.h
+
 PENSANDO ETHERNET DRIVERS
 M:     Shannon Nelson [off-list ref]
 M:     drivers@pensando.io

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 8bad63417a50..f472b3d972b3 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig

@@ -236,4 +236,7 @@ source "drivers/interconnect/Kconfig"

 source "drivers/counter/Kconfig"

 source "drivers/most/Kconfig"
+
+source "drivers/peci/Kconfig"
+
 endmenu

diff --git a/drivers/Makefile b/drivers/Makefile
index 27c018bdf4de..8d96f0c3dde5 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile

@@ -189,3 +189,4 @@ obj-$(CONFIG_GNSS)          += gnss/

 obj-$(CONFIG_INTERCONNECT)     += interconnect/
 obj-$(CONFIG_COUNTER)          += counter/
 obj-$(CONFIG_MOST)             += most/
+obj-$(CONFIG_PECI)             += peci/

diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
new file mode 100644
index 000000000000..71a4ad81225a
--- /dev/null
+++ b/drivers/peci/Kconfig

@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+menuconfig PECI
+       tristate "PECI support"
+       help
+         The Platform Environment Control Interface (PECI) is an interface
+         that provides a communication channel to Intel processors and
+         chipset components from external monitoring or control devices.
+
+         If you are building a Baseboard Management Controller (BMC) kernel
+         for Intel platform say Y here and also to the specific driver for
+         your adapter(s) below. If unsure say N.
+
+         This support is also available as a module. If so, the module
+         will be called peci.

diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
new file mode 100644
index 000000000000..e789a354e842
--- /dev/null
+++ b/drivers/peci/Makefile

@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+# Core functionality
+peci-y := core.o
+obj-$(CONFIG_PECI) += peci.o

diff --git a/drivers/peci/core.c b/drivers/peci/core.c
new file mode 100644
index 000000000000..7b3938af0396
--- /dev/null
+++ b/drivers/peci/core.c

@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2018-2021 Intel Corporation
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

This looks like overkill for only one print statement in this module,
especially when the dev_ print helpers offer more detail.

Ok, I'll remove it.

quoted

+
+#include <linux/bug.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/peci.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+#include "internal.h"
+
+static DEFINE_IDA(peci_controller_ida);
+
+static void peci_controller_dev_release(struct device *dev)
+{
+       struct peci_controller *controller = to_peci_controller(dev);
+
+       pm_runtime_disable(&controller->dev);

This seems late to be disabling power management, the device is about
to be freed. Keep in mind the lifetime of the this object can be
artificially prolonged. I expect this to be done when the device is
unregistered from the bus.

Makes sense.

quoted

+
+       mutex_destroy(&controller->bus_lock);
+       ida_free(&peci_controller_ida, controller->id);
+       fwnode_handle_put(controller->dev.fwnode);

Shouldn't the get / put of this handle reference be bound to specific
accesses not held for the entire lifetime of the object? At a minimum
it seems to be a reference that can taken at registration and dropped
at unregistration.

I'll move it to take ref at registration and to drop it at unregistration.

quoted

+       kfree(controller);
+}
+
+struct device_type peci_controller_type = {
+       .release        = peci_controller_dev_release,
+};
+
+static struct peci_controller *peci_controller_alloc(struct device *dev,
+                                                    struct
peci_controller_ops *ops)
+{
+       struct fwnode_handle *node = fwnode_handle_get(dev_fwnode(dev));
+       struct peci_controller *controller;
+       int ret;
+
+       if (!ops->xfer)
+               return ERR_PTR(-EINVAL);
+
+       controller = kzalloc(sizeof(*controller), GFP_KERNEL);
+       if (!controller)
+               return ERR_PTR(-ENOMEM);
+
+       ret = ida_alloc_max(&peci_controller_ida, U8_MAX, GFP_KERNEL);
+       if (ret < 0)
+               goto err;
+       controller->id = ret;
+
+       controller->ops = ops;
+
+       controller->dev.parent = dev;
+       controller->dev.bus = &peci_bus_type;
+       controller->dev.type = &peci_controller_type;
+       controller->dev.fwnode = node;
+       controller->dev.of_node = to_of_node(node);
+
+       device_initialize(&controller->dev);
+
+       mutex_init(&controller->bus_lock);
+
+       pm_runtime_no_callbacks(&controller->dev);
+       pm_suspend_ignore_children(&controller->dev, true);
+       pm_runtime_enable(&controller->dev);

Per above, are you sure unregistered devices need pm_runtime enabled?

Rest looks ok to me.

Thanks
-Iwona