[PATCH 0/4] MFD: intel_pmt: Split OOBMSM from intel_pmt driver

[PATCH 0/4] MFD: intel_pmt: Split OOBMSM from intel_pmt driver

[David E. Box]

Most of the devices in the intel_pmt driver are PMT only devices. However,
the Out of Band Management Services Module (OOBMSM) can also be used
access non-PMT devices. In order to better support this without the
confusion of a dependency on MFD_INTEL_PMT, this patch set rewrites the
intel_pmt driver to split the functionality into 3 components:

	1. intel_pmt - driver for PMT only devices
	2. intel-oobmsm - driver for devices using the OOBMSM IP which can
	   support multiple types of capabilities, including PMT
	3. intel-extended-cap - symbols to handle adding platform device
	   for both drivers

Additionally, this patch set provides additional DVSEC macros in the PCI
header as well as support for reading capabilities from a VSEC structure.

Patch 1 - Adds PCI defines for DVSEC registers
Patch 2 - Removes OOBMSM from intel_pmt and creates intel-extended-cap.c
	  support the creation of platform devices for capabilities from
	  both intel_pmt and intel-oobmsm (Patch 3)
Patch 2 - Creates a separate driver for OOBMSM
Patch 3 - Adds support for reading capabilities from PCIe VSEC structures

For submission through MFD branch.

David E. Box (4):
  PCI: Add #defines for accessing PCIE DVSEC fields
  MFD: intel_pmt: Remove OOBMSM device for placement in own driver
  MFD: Add the Intel Out of Band Management Services Module (OOBMSM)
    driver
  MFD: intel-extended-cap: Add support for PCIe VSEC structures

 MAINTAINERS                                |   2 +
 drivers/mfd/Kconfig                        |  15 ++
 drivers/mfd/Makefile                       |   2 +
 drivers/mfd/intel_extended_caps.c          | 267 +++++++++++++++++++++
 drivers/mfd/intel_extended_caps.h          |  40 +++
 drivers/mfd/intel_oobmsm.c                 |  61 +++++
 drivers/mfd/intel_pmt.c                    | 198 ++-------------
 drivers/platform/x86/Kconfig               |   4 +-
 drivers/platform/x86/intel_pmt_crashlog.c  |   2 +-
 drivers/platform/x86/intel_pmt_telemetry.c |   2 +-
 include/uapi/linux/pci_regs.h              |   4 +
 11 files changed, 409 insertions(+), 188 deletions(-)
 create mode 100644 drivers/mfd/intel_extended_caps.c
 create mode 100644 drivers/mfd/intel_extended_caps.h
 create mode 100644 drivers/mfd/intel_oobmsm.c

-- 
2.25.1

[PATCH 1/4] PCI: Add #defines for accessing PCIE DVSEC fields

[David E. Box]

Add #defines for accessing Vendor ID, Revision, Length, and ID offsets
in the Designated Vendor Specific Extended Capability (DVSEC). Defined
in PCIe r5.0, sec 7.9.6.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 include/uapi/linux/pci_regs.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e709ae8235e7..57ee51f19283 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1080,7 +1080,11 @@
 
 /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
 #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
+#define  PCI_DVSEC_HEADER1_VID(x)	((x) & 0xffff)
+#define  PCI_DVSEC_HEADER1_REV(x)	(((x) >> 16) & 0xf)
+#define  PCI_DVSEC_HEADER1_LEN(x)	(((x) >> 20) & 0xfff)
 #define PCI_DVSEC_HEADER2		0x8 /* Designated Vendor-Specific Header2 */
+#define  PCI_DVSEC_HEADER2_ID(x)		((x) & 0xffff)
 
 /* Data Link Feature */
 #define PCI_DLF_CAP		0x04	/* Capabilities Register */
-- 
2.25.1

Re: [PATCH 1/4] PCI: Add #defines for accessing PCIE DVSEC fields

[Bjorn Helgaas]

[+cc Dan, Jonathan]

On Thu, Jun 17, 2021 at 02:54:05PM -0700, David E. Box wrote:
> Add #defines for accessing Vendor ID, Revision, Length, and ID offsets
> in the Designated Vendor Specific Extended Capability (DVSEC). Defined
> in PCIe r5.0, sec 7.9.6.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I don't have time right now to really look at the
intel_extended_caps.c patch [1], but I wonder if there's anything
there that could be abstracted and shared with CXL, etc?  If not, no
worries.

[1] https://lore.kernel.org/r/20210617215408.1412409-5-david.e.box@linux.intel.com

> ---
>  include/uapi/linux/pci_regs.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e709ae8235e7..57ee51f19283 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1080,7 +1080,11 @@
>  
>  /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
>  #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
> +#define  PCI_DVSEC_HEADER1_VID(x)	((x) & 0xffff)
> +#define  PCI_DVSEC_HEADER1_REV(x)	(((x) >> 16) & 0xf)
> +#define  PCI_DVSEC_HEADER1_LEN(x)	(((x) >> 20) & 0xfff)
>  #define PCI_DVSEC_HEADER2		0x8 /* Designated Vendor-Specific Header2 */
> +#define  PCI_DVSEC_HEADER2_ID(x)		((x) & 0xffff)
>  
>  /* Data Link Feature */
>  #define PCI_DLF_CAP		0x04	/* Capabilities Register */
> -- 
> 2.25.1
> 

[PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[David E. Box]

Unlike the other devices in intel_pmt, the Out of Band Management Services
Module (OOBMSM) is actually not a PMT dedicated device. It can also be used
to describe non-PMT capabilities. Like PMT, these capabilities are also
enumerated using PCIe Vendor Specific registers in config space. In order
to better support these devices without the confusion of a dependency on
MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it can be
later placed in its own driver. Since much of the same code will be used by
intel_pmt and the new driver, create a new file with symbols to be used by
both.

While performing this split we need to also handle the creation of platform
devices for the non-PMT capabilities. Currently PMT devices are named by
their capability (e.g. pmt_telemetry). Instead, generically name them by
their capability ID (e.g. intel_extnd_cap_2). This allows the IDs to be
created automatically.  However, to ensure that unsupported devices aren't
created, use an allow list to specify supported capabilities.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 MAINTAINERS                                |   1 +
 drivers/mfd/Kconfig                        |   4 +
 drivers/mfd/Makefile                       |   1 +
 drivers/mfd/intel_extended_caps.c          | 208 +++++++++++++++++++++
 drivers/mfd/intel_extended_caps.h          |  40 ++++
 drivers/mfd/intel_pmt.c                    | 198 ++------------------
 drivers/platform/x86/intel_pmt_crashlog.c  |   2 +-
 drivers/platform/x86/intel_pmt_telemetry.c |   2 +-
 8 files changed, 270 insertions(+), 186 deletions(-)
 create mode 100644 drivers/mfd/intel_extended_caps.c
 create mode 100644 drivers/mfd/intel_extended_caps.h

diff --git a/MAINTAINERS b/MAINTAINERS
index bc0ceef87b73..ebdc2a0f794b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9355,6 +9355,7 @@ F:	include/linux/mfd/intel_soc_pmic*
 INTEL PMT DRIVER
 M:	"David E. Box" <david.e.box@linux.intel.com>
 S:	Maintained
+F:	drivers/mfd/intel_extended_cap.c
 F:	drivers/mfd/intel_pmt.c
 F:	drivers/platform/x86/intel_pmt_*
 
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 5c7f2b100191..4dde8e223a9e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -673,10 +673,14 @@ config MFD_INTEL_PMC_BXT
 	  Register and P-unit access. In addition this creates devices
 	  for iTCO watchdog and telemetry that are part of the PMC.
 
+config MFD_INTEL_EXTENDED_CAPS
+	tristate
+
 config MFD_INTEL_PMT
 	tristate "Intel Platform Monitoring Technology (PMT) support"
 	depends on PCI
 	select MFD_CORE
+	select MFD_INTEL_EXTENDED_CAPS
 	help
 	  The Intel Platform Monitoring Technology (PMT) is an interface that
 	  provides access to hardware monitor registers. This driver supports
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 4f6d2b8a5f76..7fa35399ec76 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -209,6 +209,7 @@ obj-$(CONFIG_MFD_AT91_USART)	+= at91-usart.o
 obj-$(CONFIG_MFD_ATMEL_FLEXCOM)	+= atmel-flexcom.o
 obj-$(CONFIG_MFD_ATMEL_HLCDC)	+= atmel-hlcdc.o
 obj-$(CONFIG_MFD_ATMEL_SMC)	+= atmel-smc.o
+obj-$(CONFIG_MFD_INTEL_EXTENDED_CAPS)	+= intel_extended_caps.o
 obj-$(CONFIG_MFD_INTEL_LPSS)	+= intel-lpss.o
 obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o
 obj-$(CONFIG_MFD_INTEL_LPSS_ACPI)	+= intel-lpss-acpi.o
diff --git a/drivers/mfd/intel_extended_caps.c b/drivers/mfd/intel_extended_caps.c
new file mode 100644
index 000000000000..89cf1ae6f65b
--- /dev/null
+++ b/drivers/mfd/intel_extended_caps.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Extended Capabilities module
+ *
+ * Copyright (c) 2021, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: David E. Box <david.e.box@linux.intel.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include "intel_extended_caps.h"
+
+/* Intel DVSEC capability vendor space offsets */
+#define INTEL_DVSEC_ENTRIES		0xA
+#define INTEL_DVSEC_SIZE		0xB
+#define INTEL_DVSEC_TABLE		0xC
+#define INTEL_DVSEC_TABLE_BAR(x)	((x) & GENMASK(2, 0))
+#define INTEL_DVSEC_TABLE_OFFSET(x)	((x) & GENMASK(31, 3))
+
+/* Intel Extended Features */
+#define INTEL_EXT_CAP_ID_TELEMETRY	2
+#define INTEL_EXT_CAP_ID_WATCHER	3
+#define INTEL_EXT_CAP_ID_CRASHLOG	4
+
+#define INTEL_EXT_CAP_PREFIX		"intel_extnd_cap"
+#define FEATURE_ID_NAME_LENGTH		25
+
+static int intel_ext_cap_allow_list[] = {
+	INTEL_EXT_CAP_ID_TELEMETRY,
+	INTEL_EXT_CAP_ID_WATCHER,
+	INTEL_EXT_CAP_ID_CRASHLOG,
+};
+
+static bool intel_ext_cap_allowed(u16 id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(intel_ext_cap_allow_list); i++)
+		if (intel_ext_cap_allow_list[i] == id)
+			return true;
+
+	return false;
+}
+
+static bool intel_ext_cap_disabled(u16 id, unsigned long quirks)
+{
+	switch (id) {
+	case INTEL_EXT_CAP_ID_WATCHER:
+		return !!(quirks & EXT_CAP_QUIRK_NO_WATCHER);
+
+	case INTEL_EXT_CAP_ID_CRASHLOG:
+		return !!(quirks & EXT_CAP_QUIRK_NO_CRASHLOG);
+
+	default:
+		return false;
+	}
+}
+
+static int intel_ext_cap_add_dev(struct pci_dev *pdev, struct intel_ext_cap_header *header,
+				 unsigned long quirks)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res, *tmp;
+	struct mfd_cell *cell;
+	char feature_id_name[FEATURE_ID_NAME_LENGTH];
+	int count = header->num_entries;
+	int size = header->entry_size;
+	int id = header->id;
+	int i;
+
+	if (!intel_ext_cap_allowed(id))
+		return -EINVAL;
+
+	if (intel_ext_cap_disabled(id, quirks))
+		return -EINVAL;
+
+	snprintf(feature_id_name, sizeof(feature_id_name), "%s_%d", INTEL_EXT_CAP_PREFIX, id);
+
+	if (!header->num_entries) {
+		dev_err(dev, "Invalid 0 entry count for %s header\n", feature_id_name);
+		return -EINVAL;
+	}
+
+	if (!header->entry_size) {
+		dev_err(dev, "Invalid 0 entry size for %s header\n", feature_id_name);
+		return -EINVAL;
+	}
+
+	cell = devm_kzalloc(dev, sizeof(*cell), GFP_KERNEL);
+	if (!cell)
+		return -ENOMEM;
+
+	res = devm_kcalloc(dev, count, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+
+	if (quirks & EXT_CAP_QUIRK_TABLE_SHIFT)
+		header->offset >>= 3;
+
+	/*
+	 * The DVSEC contains the starting offset and count for a block of
+	 * discovery tables, each providing access to monitoring facilities for
+	 * a section of the device. Create a resource list of these tables to
+	 * provide to the driver.
+	 */
+	for (i = 0, tmp = res; i < count; i++, tmp++) {
+		tmp->start = pdev->resource[header->tbir].start +
+			     header->offset + i * (size * sizeof(u32));
+		tmp->end = tmp->start + (size * sizeof(u32)) - 1;
+		tmp->flags = IORESOURCE_MEM;
+	}
+
+	cell->resources = res;
+	cell->num_resources = count;
+	cell->name = feature_id_name;
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cell, 1, NULL, 0, NULL);
+}
+
+int intel_ext_cap_probe(struct pci_dev *pdev, struct intel_ext_cap_platform_info *info)
+{
+	unsigned long quirks = 0;
+	bool found_devices = false;
+	int ret, pos;
+
+	if (info)
+		quirks = info->quirks;
+
+	if (info && (info->quirks & EXT_CAP_QUIRK_NO_DVSEC)) {
+		struct intel_ext_cap_header **header;
+
+		header = info->capabilities;
+		while (*header) {
+			ret = intel_ext_cap_add_dev(pdev, *header, quirks);
+			if (ret)
+				dev_warn(&pdev->dev,
+					 "Failed to add device for DVSEC id %d\n",
+					 (*header)->id);
+			else
+				found_devices = true;
+
+			header++;
+		}
+	} else {
+		/* Find DVSEC features */
+		pos = 0;
+		do {
+			struct intel_ext_cap_header header;
+			u32 table, hdr;
+			u16 vid;
+
+			pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
+			if (!pos)
+				break;
+
+			pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER1, &hdr);
+			vid = PCI_DVSEC_HEADER1_VID(hdr);
+			if (vid != PCI_VENDOR_ID_INTEL)
+				continue;
+
+			/* Support only revision 1 */
+			header.rev = PCI_DVSEC_HEADER1_REV(hdr);
+			if (header.rev != 1) {
+				dev_warn(&pdev->dev, "Unsupported DVSEC revision %d\n",
+					 header.rev);
+				continue;
+			}
+
+			header.length = PCI_DVSEC_HEADER1_LEN(hdr);
+
+			pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
+					     &header.num_entries);
+			pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
+					     &header.entry_size);
+			pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
+					      &table);
+
+			header.tbir = INTEL_DVSEC_TABLE_BAR(table);
+			header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
+
+			pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER2, &hdr);
+			header.id = PCI_DVSEC_HEADER2_ID(hdr);
+
+			ret = intel_ext_cap_add_dev(pdev, &header, quirks);
+			if (ret)
+				continue;
+
+			found_devices = true;
+		} while (true);
+	}
+
+	if (!found_devices)
+		return -ENODEV;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_ext_cap_probe);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Intel Extended Capability Core driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mfd/intel_extended_caps.h b/drivers/mfd/intel_extended_caps.h
new file mode 100644
index 000000000000..c6722d4cb5e3
--- /dev/null
+++ b/drivers/mfd/intel_extended_caps.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef INTEL_EXTENDED_CAPS_H
+#define INTEL_EXTENDED_CAPS_H
+
+/* Intel Extended Features */
+#define INTEL_EXT_CAP_ID_TELEMETRY	2
+#define INTEL_EXT_CAP_ID_WATCHER	3
+#define INTEL_EXT_CAP_ID_CRASHLOG	4
+
+struct intel_ext_cap_header {
+	u8	rev;
+	u16	length;
+	u16	id;
+	u8	num_entries;
+	u8	entry_size;
+	u8	tbir;
+	u32	offset;
+};
+
+enum intel_ext_cap_quirks {
+	/* Watcher capability not supported */
+	EXT_CAP_QUIRK_NO_WATCHER	= BIT(0),
+
+	/* Crashlog capability not supported */
+	EXT_CAP_QUIRK_NO_CRASHLOG	= BIT(1),
+
+	/* Use shift instead of mask to read discovery table offset */
+	EXT_CAP_QUIRK_TABLE_SHIFT	= BIT(2),
+
+	/* DVSEC not present (provided in driver data) */
+	EXT_CAP_QUIRK_NO_DVSEC	= BIT(3),
+};
+
+struct intel_ext_cap_platform_info {
+	unsigned long quirks;
+	struct intel_ext_cap_header **capabilities;
+};
+
+int intel_ext_cap_probe(struct pci_dev *pdev, struct intel_ext_cap_platform_info *info);
+#endif
diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
index dd7eb614c28e..f5205d6b5c47 100644
--- a/drivers/mfd/intel_pmt.c
+++ b/drivers/mfd/intel_pmt.c
@@ -8,64 +8,19 @@
  * Author: David E. Box <david.e.box@linux.intel.com>
  */
 
-#include <linux/bits.h>
-#include <linux/kernel.h>
-#include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/pci.h>
-#include <linux/platform_device.h>
-#include <linux/pm.h>
 #include <linux/pm_runtime.h>
-#include <linux/types.h>
 
-/* Intel DVSEC capability vendor space offsets */
-#define INTEL_DVSEC_ENTRIES		0xA
-#define INTEL_DVSEC_SIZE		0xB
-#define INTEL_DVSEC_TABLE		0xC
-#define INTEL_DVSEC_TABLE_BAR(x)	((x) & GENMASK(2, 0))
-#define INTEL_DVSEC_TABLE_OFFSET(x)	((x) & GENMASK(31, 3))
-#define INTEL_DVSEC_ENTRY_SIZE		4
+#include "intel_extended_caps.h"
 
-/* PMT capabilities */
-#define DVSEC_INTEL_ID_TELEMETRY	2
-#define DVSEC_INTEL_ID_WATCHER		3
-#define DVSEC_INTEL_ID_CRASHLOG		4
-
-struct intel_dvsec_header {
-	u16	length;
-	u16	id;
-	u8	num_entries;
-	u8	entry_size;
-	u8	tbir;
-	u32	offset;
-};
-
-enum pmt_quirks {
-	/* Watcher capability not supported */
-	PMT_QUIRK_NO_WATCHER	= BIT(0),
-
-	/* Crashlog capability not supported */
-	PMT_QUIRK_NO_CRASHLOG	= BIT(1),
-
-	/* Use shift instead of mask to read discovery table offset */
-	PMT_QUIRK_TABLE_SHIFT	= BIT(2),
-
-	/* DVSEC not present (provided in driver data) */
-	PMT_QUIRK_NO_DVSEC	= BIT(3),
-};
-
-struct pmt_platform_info {
-	unsigned long quirks;
-	struct intel_dvsec_header **capabilities;
-};
-
-static const struct pmt_platform_info tgl_info = {
-	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
-		  PMT_QUIRK_TABLE_SHIFT,
+static const struct intel_ext_cap_platform_info tgl_info = {
+	.quirks = EXT_CAP_QUIRK_NO_WATCHER | EXT_CAP_QUIRK_NO_CRASHLOG |
+		  EXT_CAP_QUIRK_TABLE_SHIFT,
 };
 
 /* DG1 Platform with DVSEC quirk*/
-static struct intel_dvsec_header dg1_telemetry = {
+static struct intel_ext_cap_header dg1_telemetry = {
 	.length = 0x10,
 	.id = 2,
 	.num_entries = 1,
@@ -74,154 +29,31 @@ static struct intel_dvsec_header dg1_telemetry = {
 	.offset = 0x466000,
 };
 
-static struct intel_dvsec_header *dg1_capabilities[] = {
+static struct intel_ext_cap_header *dg1_capabilities[] = {
 	&dg1_telemetry,
 	NULL
 };
 
-static const struct pmt_platform_info dg1_info = {
-	.quirks = PMT_QUIRK_NO_DVSEC,
+static const struct intel_ext_cap_platform_info dg1_info = {
+	.quirks = EXT_CAP_QUIRK_NO_DVSEC,
 	.capabilities = dg1_capabilities,
 };
 
-static int pmt_add_dev(struct pci_dev *pdev, struct intel_dvsec_header *header,
-		       unsigned long quirks)
-{
-	struct device *dev = &pdev->dev;
-	struct resource *res, *tmp;
-	struct mfd_cell *cell;
-	const char *name;
-	int count = header->num_entries;
-	int size = header->entry_size;
-	int id = header->id;
-	int i;
-
-	switch (id) {
-	case DVSEC_INTEL_ID_TELEMETRY:
-		name = "pmt_telemetry";
-		break;
-	case DVSEC_INTEL_ID_WATCHER:
-		if (quirks & PMT_QUIRK_NO_WATCHER) {
-			dev_info(dev, "Watcher not supported\n");
-			return -EINVAL;
-		}
-		name = "pmt_watcher";
-		break;
-	case DVSEC_INTEL_ID_CRASHLOG:
-		if (quirks & PMT_QUIRK_NO_CRASHLOG) {
-			dev_info(dev, "Crashlog not supported\n");
-			return -EINVAL;
-		}
-		name = "pmt_crashlog";
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	if (!header->num_entries || !header->entry_size) {
-		dev_err(dev, "Invalid count or size for %s header\n", name);
-		return -EINVAL;
-	}
-
-	cell = devm_kzalloc(dev, sizeof(*cell), GFP_KERNEL);
-	if (!cell)
-		return -ENOMEM;
-
-	res = devm_kcalloc(dev, count, sizeof(*res), GFP_KERNEL);
-	if (!res)
-		return -ENOMEM;
-
-	if (quirks & PMT_QUIRK_TABLE_SHIFT)
-		header->offset >>= 3;
-
-	/*
-	 * The PMT DVSEC contains the starting offset and count for a block of
-	 * discovery tables, each providing access to monitoring facilities for
-	 * a section of the device. Create a resource list of these tables to
-	 * provide to the driver.
-	 */
-	for (i = 0, tmp = res; i < count; i++, tmp++) {
-		tmp->start = pdev->resource[header->tbir].start +
-			     header->offset + i * (size << 2);
-		tmp->end = tmp->start + (size << 2) - 1;
-		tmp->flags = IORESOURCE_MEM;
-	}
-
-	cell->resources = res;
-	cell->num_resources = count;
-	cell->name = name;
-
-	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cell, 1, NULL, 0,
-				    NULL);
-}
 
 static int pmt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
-	struct pmt_platform_info *info;
-	unsigned long quirks = 0;
-	bool found_devices = false;
-	int ret, pos = 0;
+	struct intel_ext_cap_platform_info *info;
+	int ret;
 
 	ret = pcim_enable_device(pdev);
 	if (ret)
 		return ret;
 
-	info = (struct pmt_platform_info *)id->driver_data;
-
-	if (info)
-		quirks = info->quirks;
-
-	if (info && (info->quirks & PMT_QUIRK_NO_DVSEC)) {
-		struct intel_dvsec_header **header;
-
-		header = info->capabilities;
-		while (*header) {
-			ret = pmt_add_dev(pdev, *header, quirks);
-			if (ret)
-				dev_warn(&pdev->dev,
-					 "Failed to add device for DVSEC id %d\n",
-					 (*header)->id);
-			else
-				found_devices = true;
-
-			++header;
-		}
-	} else {
-		do {
-			struct intel_dvsec_header header;
-			u32 table;
-			u16 vid;
+	info = (struct intel_ext_cap_platform_info *)id->driver_data;
 
-			pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
-			if (!pos)
-				break;
-
-			pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vid);
-			if (vid != PCI_VENDOR_ID_INTEL)
-				continue;
-
-			pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER2,
-					     &header.id);
-			pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
-					     &header.num_entries);
-			pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
-					     &header.entry_size);
-			pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
-					      &table);
-
-			header.tbir = INTEL_DVSEC_TABLE_BAR(table);
-			header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
-
-			ret = pmt_add_dev(pdev, &header, quirks);
-			if (ret)
-				continue;
-
-			found_devices = true;
-		} while (true);
-	}
-
-	if (!found_devices)
-		return -ENODEV;
+	ret = intel_ext_cap_probe(pdev, info);
+	if (ret)
+		return ret;
 
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_allow(&pdev->dev);
@@ -237,12 +69,10 @@ static void pmt_pci_remove(struct pci_dev *pdev)
 
 #define PCI_DEVICE_ID_INTEL_PMT_ADL	0x467d
 #define PCI_DEVICE_ID_INTEL_PMT_DG1	0x490e
-#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
 #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
 static const struct pci_device_id pmt_pci_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, PMT_ADL, &tgl_info) },
 	{ PCI_DEVICE_DATA(INTEL, PMT_DG1, &dg1_info) },
-	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, NULL) },
 	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
 	{ }
 };
diff --git a/drivers/platform/x86/intel_pmt_crashlog.c b/drivers/platform/x86/intel_pmt_crashlog.c
index 92d315a16cfd..6662454af5bf 100644
--- a/drivers/platform/x86/intel_pmt_crashlog.c
+++ b/drivers/platform/x86/intel_pmt_crashlog.c
@@ -17,7 +17,7 @@
 
 #include "intel_pmt_class.h"
 
-#define DRV_NAME		"pmt_crashlog"
+#define DRV_NAME		"intel_extnd_cap_4"
 
 /* Crashlog discovery header types */
 #define CRASH_TYPE_OOBMSM	1
diff --git a/drivers/platform/x86/intel_pmt_telemetry.c b/drivers/platform/x86/intel_pmt_telemetry.c
index 9b95ef050457..f281d1d71058 100644
--- a/drivers/platform/x86/intel_pmt_telemetry.c
+++ b/drivers/platform/x86/intel_pmt_telemetry.c
@@ -17,7 +17,7 @@
 
 #include "intel_pmt_class.h"
 
-#define TELEM_DEV_NAME		"pmt_telemetry"
+#define TELEM_DEV_NAME		"intel_extnd_cap_2"
 
 #define TELEM_SIZE_OFFSET	0x0
 #define TELEM_GUID_OFFSET	0x4
-- 
2.25.1

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[Lee Jones]

On Thu, 17 Jun 2021, David E. Box wrote:

> Unlike the other devices in intel_pmt, the Out of Band Management Services
> Module (OOBMSM) is actually not a PMT dedicated device. It can also be used
> to describe non-PMT capabilities. Like PMT, these capabilities are also
> enumerated using PCIe Vendor Specific registers in config space. In order
> to better support these devices without the confusion of a dependency on
> MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it can be
> later placed in its own driver. Since much of the same code will be used by
> intel_pmt and the new driver, create a new file with symbols to be used by
> both.
> 
> While performing this split we need to also handle the creation of platform
> devices for the non-PMT capabilities. Currently PMT devices are named by
> their capability (e.g. pmt_telemetry). Instead, generically name them by
> their capability ID (e.g. intel_extnd_cap_2). This allows the IDs to be
> created automatically.  However, to ensure that unsupported devices aren't
> created, use an allow list to specify supported capabilities.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  MAINTAINERS                                |   1 +
>  drivers/mfd/Kconfig                        |   4 +
>  drivers/mfd/Makefile                       |   1 +
>  drivers/mfd/intel_extended_caps.c          | 208 +++++++++++++++++++++

Please consider moving this <whatever this is> out to either
drivers/pci or drivers/platform/x86.

I suggest Andy should also be on Cc.

>  drivers/mfd/intel_extended_caps.h          |  40 ++++
>  drivers/mfd/intel_pmt.c                    | 198 ++------------------
>  drivers/platform/x86/intel_pmt_crashlog.c  |   2 +-
>  drivers/platform/x86/intel_pmt_telemetry.c |   2 +-
>  8 files changed, 270 insertions(+), 186 deletions(-)
>  create mode 100644 drivers/mfd/intel_extended_caps.c
>  create mode 100644 drivers/mfd/intel_extended_caps.h

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[David E. Box]

On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
> On Thu, 17 Jun 2021, David E. Box wrote:
> 
> > Unlike the other devices in intel_pmt, the Out of Band Management
> > Services
> > Module (OOBMSM) is actually not a PMT dedicated device. It can also
> > be used
> > to describe non-PMT capabilities. Like PMT, these capabilities are
> > also
> > enumerated using PCIe Vendor Specific registers in config space. In
> > order
> > to better support these devices without the confusion of a
> > dependency on
> > MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it
> > can be
> > later placed in its own driver. Since much of the same code will be
> > used by
> > intel_pmt and the new driver, create a new file with symbols to be
> > used by
> > both.
> > 
> > While performing this split we need to also handle the creation of
> > platform
> > devices for the non-PMT capabilities. Currently PMT devices are
> > named by
> > their capability (e.g. pmt_telemetry). Instead, generically name
> > them by
> > their capability ID (e.g. intel_extnd_cap_2). This allows the IDs
> > to be
> > created automatically.  However, to ensure that unsupported devices
> > aren't
> > created, use an allow list to specify supported capabilities.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  MAINTAINERS                                |   1 +
> >  drivers/mfd/Kconfig                        |   4 +
> >  drivers/mfd/Makefile                       |   1 +
> >  drivers/mfd/intel_extended_caps.c          | 208
> > +++++++++++++++++++++
> 
> Please consider moving this <whatever this is> out to either
> drivers/pci or drivers/platform/x86.

None of the cell drivers are in MFD, only the PCI drivers from which
the cells are created. I understood that these should be in MFD. But
moving it to drivers/platform/x86 would be fine with me. That keeps the
code together in the same subsystem. Comment from Hans or Andy? 

> 
> I suggest Andy should also be on Cc.
> 
> >  drivers/mfd/intel_extended_caps.h          |  40 ++++
> >  drivers/mfd/intel_pmt.c                    | 198 ++---------------
> > ---
> >  drivers/platform/x86/intel_pmt_crashlog.c  |   2 +-
> >  drivers/platform/x86/intel_pmt_telemetry.c |   2 +-
> >  8 files changed, 270 insertions(+), 186 deletions(-)
> >  create mode 100644 drivers/mfd/intel_extended_caps.c
> >  create mode 100644 drivers/mfd/intel_extended_caps.h
> 

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[Lee Jones]

On Wed, 30 Jun 2021, David E. Box wrote:

> On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
> > On Thu, 17 Jun 2021, David E. Box wrote:
> > 
> > > Unlike the other devices in intel_pmt, the Out of Band Management
> > > Services
> > > Module (OOBMSM) is actually not a PMT dedicated device. It can also
> > > be used
> > > to describe non-PMT capabilities. Like PMT, these capabilities are
> > > also
> > > enumerated using PCIe Vendor Specific registers in config space. In
> > > order
> > > to better support these devices without the confusion of a
> > > dependency on
> > > MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it
> > > can be
> > > later placed in its own driver. Since much of the same code will be
> > > used by
> > > intel_pmt and the new driver, create a new file with symbols to be
> > > used by
> > > both.
> > > 
> > > While performing this split we need to also handle the creation of
> > > platform
> > > devices for the non-PMT capabilities. Currently PMT devices are
> > > named by
> > > their capability (e.g. pmt_telemetry). Instead, generically name
> > > them by
> > > their capability ID (e.g. intel_extnd_cap_2). This allows the IDs
> > > to be
> > > created automatically.  However, to ensure that unsupported devices
> > > aren't
> > > created, use an allow list to specify supported capabilities.
> > > 
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > ---
> > >  MAINTAINERS                                |   1 +
> > >  drivers/mfd/Kconfig                        |   4 +
> > >  drivers/mfd/Makefile                       |   1 +
> > >  drivers/mfd/intel_extended_caps.c          | 208
> > > +++++++++++++++++++++
> > 
> > Please consider moving this <whatever this is> out to either
> > drivers/pci or drivers/platform/x86.
> 
> None of the cell drivers are in MFD, only the PCI drivers from which
> the cells are created. I understood that these should be in MFD. But
> moving it to drivers/platform/x86 would be fine with me. That keeps the
> code together in the same subsystem. Comment from Hans or Andy? 

You don't need to move everything there.  If a driver uses the MFD API
(like intel_pmt.c does), it can stay.  But all of this PCI/hardware/
platform specific capability craziness has no place here AFAICT.

> > I suggest Andy should also be on Cc.
> > 
> > >  drivers/mfd/intel_extended_caps.h          |  40 ++++
> > >  drivers/mfd/intel_pmt.c                    | 198 ++---------------
> > > ---
> > >  drivers/platform/x86/intel_pmt_crashlog.c  |   2 +-
> > >  drivers/platform/x86/intel_pmt_telemetry.c |   2 +-
> > >  8 files changed, 270 insertions(+), 186 deletions(-)
> > >  create mode 100644 drivers/mfd/intel_extended_caps.c
> > >  create mode 100644 drivers/mfd/intel_extended_caps.h
> > 
> 
> 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[Hans de Goede]

Hi,

On 6/30/21 11:11 PM, David E. Box wrote:
> On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
>> On Thu, 17 Jun 2021, David E. Box wrote:
>>
>>> Unlike the other devices in intel_pmt, the Out of Band Management
>>> Services
>>> Module (OOBMSM) is actually not a PMT dedicated device. It can also
>>> be used
>>> to describe non-PMT capabilities. Like PMT, these capabilities are
>>> also
>>> enumerated using PCIe Vendor Specific registers in config space. In
>>> order
>>> to better support these devices without the confusion of a
>>> dependency on
>>> MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it
>>> can be
>>> later placed in its own driver. Since much of the same code will be
>>> used by
>>> intel_pmt and the new driver, create a new file with symbols to be
>>> used by
>>> both.
>>>
>>> While performing this split we need to also handle the creation of
>>> platform
>>> devices for the non-PMT capabilities. Currently PMT devices are
>>> named by
>>> their capability (e.g. pmt_telemetry). Instead, generically name
>>> them by
>>> their capability ID (e.g. intel_extnd_cap_2). This allows the IDs
>>> to be
>>> created automatically.  However, to ensure that unsupported devices
>>> aren't
>>> created, use an allow list to specify supported capabilities.
>>>
>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>> ---
>>>  MAINTAINERS                                |   1 +
>>>  drivers/mfd/Kconfig                        |   4 +
>>>  drivers/mfd/Makefile                       |   1 +
>>>  drivers/mfd/intel_extended_caps.c          | 208
>>> +++++++++++++++++++++
>>
>> Please consider moving this <whatever this is> out to either
>> drivers/pci or drivers/platform/x86.
> 
> None of the cell drivers are in MFD, only the PCI drivers from which
> the cells are created. I understood that these should be in MFD. But
> moving it to drivers/platform/x86 would be fine with me. That keeps the
> code together in the same subsystem. Comment from Hans or Andy? 

I'm fine with moving everything to drivers/platform/x86, but AFAIK
usually the actual code which has the MFD cells and creates the
child devices usually lives under drivers/mfd

Regards,

Hans



> 
>>
>> I suggest Andy should also be on Cc.
>>
>>>  drivers/mfd/intel_extended_caps.h          |  40 ++++
>>>  drivers/mfd/intel_pmt.c                    | 198 ++---------------
>>> ---
>>>  drivers/platform/x86/intel_pmt_crashlog.c  |   2 +-
>>>  drivers/platform/x86/intel_pmt_telemetry.c |   2 +-
>>>  8 files changed, 270 insertions(+), 186 deletions(-)
>>>  create mode 100644 drivers/mfd/intel_extended_caps.c
>>>  create mode 100644 drivers/mfd/intel_extended_caps.h
>>
> 
> 

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[Lee Jones]

On Thu, 01 Jul 2021, Hans de Goede wrote:

> Hi,
> 
> On 6/30/21 11:11 PM, David E. Box wrote:
> > On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
> >> On Thu, 17 Jun 2021, David E. Box wrote:
> >>
> >>> Unlike the other devices in intel_pmt, the Out of Band Management
> >>> Services
> >>> Module (OOBMSM) is actually not a PMT dedicated device. It can also
> >>> be used
> >>> to describe non-PMT capabilities. Like PMT, these capabilities are
> >>> also
> >>> enumerated using PCIe Vendor Specific registers in config space. In
> >>> order
> >>> to better support these devices without the confusion of a
> >>> dependency on
> >>> MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it
> >>> can be
> >>> later placed in its own driver. Since much of the same code will be
> >>> used by
> >>> intel_pmt and the new driver, create a new file with symbols to be
> >>> used by
> >>> both.
> >>>
> >>> While performing this split we need to also handle the creation of
> >>> platform
> >>> devices for the non-PMT capabilities. Currently PMT devices are
> >>> named by
> >>> their capability (e.g. pmt_telemetry). Instead, generically name
> >>> them by
> >>> their capability ID (e.g. intel_extnd_cap_2). This allows the IDs
> >>> to be
> >>> created automatically.  However, to ensure that unsupported devices
> >>> aren't
> >>> created, use an allow list to specify supported capabilities.
> >>>
> >>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> >>> ---
> >>>  MAINTAINERS                                |   1 +
> >>>  drivers/mfd/Kconfig                        |   4 +
> >>>  drivers/mfd/Makefile                       |   1 +
> >>>  drivers/mfd/intel_extended_caps.c          | 208
> >>> +++++++++++++++++++++
> >>
> >> Please consider moving this <whatever this is> out to either
> >> drivers/pci or drivers/platform/x86.
> > 
> > None of the cell drivers are in MFD, only the PCI drivers from which
> > the cells are created. I understood that these should be in MFD. But
> > moving it to drivers/platform/x86 would be fine with me. That keeps the
> > code together in the same subsystem. Comment from Hans or Andy? 
> 
> I'm fine with moving everything to drivers/platform/x86, but AFAIK
> usually the actual code which has the MFD cells and creates the
> child devices usually lives under drivers/mfd

Correct.  It must.

No MFD API users outside of drivers/mfd please.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[Lee Jones]

On Thu, 01 Jul 2021, Andy Shevchenko wrote:

> On Thursday, July 1, 2021, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Thu, 01 Jul 2021, Hans de Goede wrote:
> >
> > > Hi,
> > >
> > > On 6/30/21 11:11 PM, David E. Box wrote:
> > > > On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
> > > >> On Thu, 17 Jun 2021, David E. Box wrote:
> > > >>
> > > >>> Unlike the other devices in intel_pmt, the Out of Band Management
> > > >>> Services
> > > >>> Module (OOBMSM) is actually not a PMT dedicated device. It can also
> > > >>> be used
> > > >>> to describe non-PMT capabilities. Like PMT, these capabilities are
> > > >>> also
> > > >>> enumerated using PCIe Vendor Specific registers in config space. In
> > > >>> order
> > > >>> to better support these devices without the confusion of a
> > > >>> dependency on
> > > >>> MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it
> > > >>> can be
> > > >>> later placed in its own driver. Since much of the same code will be
> > > >>> used by
> > > >>> intel_pmt and the new driver, create a new file with symbols to be
> > > >>> used by
> > > >>> both.
> > > >>>
> > > >>> While performing this split we need to also handle the creation of
> > > >>> platform
> > > >>> devices for the non-PMT capabilities. Currently PMT devices are
> > > >>> named by
> > > >>> their capability (e.g. pmt_telemetry). Instead, generically name
> > > >>> them by
> > > >>> their capability ID (e.g. intel_extnd_cap_2). This allows the IDs
> > > >>> to be
> > > >>> created automatically.  However, to ensure that unsupported devices
> > > >>> aren't
> > > >>> created, use an allow list to specify supported capabilities.
> > > >>>
> > > >>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > >>> ---
> > > >>>  MAINTAINERS                                |   1 +
> > > >>>  drivers/mfd/Kconfig                        |   4 +
> > > >>>  drivers/mfd/Makefile                       |   1 +
> > > >>>  drivers/mfd/intel_extended_caps.c          | 208
> > > >>> +++++++++++++++++++++
> > > >>
> > > >> Please consider moving this <whatever this is> out to either
> > > >> drivers/pci or drivers/platform/x86.
> > > >
> > > > None of the cell drivers are in MFD, only the PCI drivers from which
> > > > the cells are created. I understood that these should be in MFD. But
> > > > moving it to drivers/platform/x86 would be fine with me. That keeps the
> > > > code together in the same subsystem. Comment from Hans or Andy?
> > >
> > > I'm fine with moving everything to drivers/platform/x86, but AFAIK
> > > usually the actual code which has the MFD cells and creates the
> > > child devices usually lives under drivers/mfd
> >
> > Correct.  It must.
> 
> It’s definitely not the first time you are talking about, but it may be the
> first time I asked why it’s not enforced overall. Last time I have checked
> it was like 5-7 MFD uses outside the MFD folder. Are you going to fix that?

Because I can't NACK patches that weren't sent to me. :)

I'll probably look into 'fixing' it when I get some free time.

> > No MFD API users outside of drivers/mfd please.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[Lee Jones]

On Thu, 01 Jul 2021, Andy Shevchenko wrote:

> On Thursday, July 1, 2021, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Thu, 01 Jul 2021, Andy Shevchenko wrote:
> >
> > > On Thursday, July 1, 2021, Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > > On Thu, 01 Jul 2021, Hans de Goede wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On 6/30/21 11:11 PM, David E. Box wrote:
> > > > > > On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
> > > > > >> On Thu, 17 Jun 2021, David E. Box wrote:
> > > > > >>
> > > > > >>> Unlike the other devices in intel_pmt, the Out of Band Management
> > > > > >>> Services
> > > > > >>> Module (OOBMSM) is actually not a PMT dedicated device. It can
> > also
> > > > > >>> be used
> > > > > >>> to describe non-PMT capabilities. Like PMT, these capabilities
> > are
> > > > > >>> also
> > > > > >>> enumerated using PCIe Vendor Specific registers in config space.
> > In
> > > > > >>> order
> > > > > >>> to better support these devices without the confusion of a
> > > > > >>> dependency on
> > > > > >>> MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so that it
> > > > > >>> can be
> > > > > >>> later placed in its own driver. Since much of the same code will
> > be
> > > > > >>> used by
> > > > > >>> intel_pmt and the new driver, create a new file with symbols to
> > be
> > > > > >>> used by
> > > > > >>> both.
> > > > > >>>
> > > > > >>> While performing this split we need to also handle the creation
> > of
> > > > > >>> platform
> > > > > >>> devices for the non-PMT capabilities. Currently PMT devices are
> > > > > >>> named by
> > > > > >>> their capability (e.g. pmt_telemetry). Instead, generically name
> > > > > >>> them by
> > > > > >>> their capability ID (e.g. intel_extnd_cap_2). This allows the IDs
> > > > > >>> to be
> > > > > >>> created automatically.  However, to ensure that unsupported
> > devices
> > > > > >>> aren't
> > > > > >>> created, use an allow list to specify supported capabilities.
> > > > > >>>
> > > > > >>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > >>> ---
> > > > > >>>  MAINTAINERS                                |   1 +
> > > > > >>>  drivers/mfd/Kconfig                        |   4 +
> > > > > >>>  drivers/mfd/Makefile                       |   1 +
> > > > > >>>  drivers/mfd/intel_extended_caps.c          | 208
> > > > > >>> +++++++++++++++++++++
> > > > > >>
> > > > > >> Please consider moving this <whatever this is> out to either
> > > > > >> drivers/pci or drivers/platform/x86.
> > > > > >
> > > > > > None of the cell drivers are in MFD, only the PCI drivers from
> > which
> > > > > > the cells are created. I understood that these should be in MFD.
> > But
> > > > > > moving it to drivers/platform/x86 would be fine with me. That
> > keeps the
> > > > > > code together in the same subsystem. Comment from Hans or Andy?
> > > > >
> > > > > I'm fine with moving everything to drivers/platform/x86, but AFAIK
> > > > > usually the actual code which has the MFD cells and creates the
> > > > > child devices usually lives under drivers/mfd
> > > >
> > > > Correct.  It must.
> > >
> > > It’s definitely not the first time you are talking about, but it may be
> > the
> > > first time I asked why it’s not enforced overall. Last time I have
> > checked
> > > it was like 5-7 MFD uses outside the MFD folder. Are you going to fix
> > that?
> >
> > Because I can't NACK patches that weren't sent to me. :)
> >
> >
> Hint: you may add regexp match to the maintainers database and you will see
> them more often

Good idea.  I'll add it to my TODO.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[David E. Box]

On Thu, 2021-07-01 at 12:23 +0100, Lee Jones wrote:
> On Thu, 01 Jul 2021, Hans de Goede wrote:
> 
> > Hi,
> > 
> > On 6/30/21 11:11 PM, David E. Box wrote:
> > > On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
> > > > On Thu, 17 Jun 2021, David E. Box wrote:
> > > > 
> > > > > Unlike the other devices in intel_pmt, the Out of Band
> > > > > Management
> > > > > Services
> > > > > Module (OOBMSM) is actually not a PMT dedicated device. It
> > > > > can also
> > > > > be used
> > > > > to describe non-PMT capabilities. Like PMT, these
> > > > > capabilities are
> > > > > also
> > > > > enumerated using PCIe Vendor Specific registers in config
> > > > > space. In
> > > > > order
> > > > > to better support these devices without the confusion of a
> > > > > dependency on
> > > > > MFD_INTEL_PMT, remove the OOBMSM device from intel_pmt so
> > > > > that it
> > > > > can be
> > > > > later placed in its own driver. Since much of the same code
> > > > > will be
> > > > > used by
> > > > > intel_pmt and the new driver, create a new file with symbols
> > > > > to be
> > > > > used by
> > > > > both.
> > > > > 
> > > > > While performing this split we need to also handle the
> > > > > creation of
> > > > > platform
> > > > > devices for the non-PMT capabilities. Currently PMT devices
> > > > > are
> > > > > named by
> > > > > their capability (e.g. pmt_telemetry). Instead, generically
> > > > > name
> > > > > them by
> > > > > their capability ID (e.g. intel_extnd_cap_2). This allows the
> > > > > IDs
> > > > > to be
> > > > > created automatically.  However, to ensure that unsupported
> > > > > devices
> > > > > aren't
> > > > > created, use an allow list to specify supported capabilities.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > ---
> > > > >  MAINTAINERS                                |   1 +
> > > > >  drivers/mfd/Kconfig                        |   4 +
> > > > >  drivers/mfd/Makefile                       |   1 +
> > > > >  drivers/mfd/intel_extended_caps.c          | 208
> > > > > +++++++++++++++++++++
> > > > 
> > > > Please consider moving this <whatever this is> out to either
> > > > drivers/pci or drivers/platform/x86.
> > > 
> > > None of the cell drivers are in MFD, only the PCI drivers from
> > > which
> > > the cells are created. I understood that these should be in MFD.
> > > But
> > > moving it to drivers/platform/x86 would be fine with me. That
> > > keeps the
> > > code together in the same subsystem. Comment from Hans or Andy? 
> > 
> > I'm fine with moving everything to drivers/platform/x86, but AFAIK
> > usually the actual code which has the MFD cells and creates the
> > child devices usually lives under drivers/mfd
> 
> Correct.  It must.
> 
> No MFD API users outside of drivers/mfd please.
> 

No problem. But these patches are not child device drivers. They take
the existing intel_pmt MFD code and split it from the device driver
(similar to how intel-lpss core code is split from the acpi and pci bus
drivers). There are 2 drivers now, PMT-only and OOBMSM, that use a
common MFD API. This is why they all reside in MFD in this patchset.
But I could move the API callers to platform/x86.

But I'd like feedback on whether this split is even needed. I'm trying
to manage the fact that one of the devices in intel_pmt will now need
support for new, non-PMT, child devices. So there would be a mismatch
between what the driver and Kconfig are named vs what it actually
supports. I considered adding all the new cells to intel_pmt and
renaming the driver to something more generic, but I understand this
will be messy for OSVs managing Kconfig options. Thanks.

David

Re: [PATCH 2/4] MFD: intel_pmt: Remove OOBMSM device

[Andy Shevchenko]

On Thu, Jul 1, 2021 at 12:11 AM David E. Box
<david.e.box@linux.intel.com> wrote:
> On Wed, 2021-06-30 at 11:15 +0100, Lee Jones wrote:
> > On Thu, 17 Jun 2021, David E. Box wrote:

...

> > Please consider moving this <whatever this is> out to either
> > drivers/pci or drivers/platform/x86.
>
> None of the cell drivers are in MFD, only the PCI drivers from which
> the cells are created. I understood that these should be in MFD. But
> moving it to drivers/platform/x86 would be fine with me. That keeps the
> code together in the same subsystem. Comment from Hans or Andy?

I'm fine with the PDx86 location, but please consider moving PMT stuff
under drivers/platform/x86/intel/pmt/.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/4] MFD: Intel Out of Band Management Services Module (OOBMSM) driver

[David E. Box]

The Intel Out of Band Management Services Module (OOBMSM) is a device
that provides access to Intel capabilities described in PCIE vendor
specific extended capability registers (both VSEC and DVSEC). These
capabilities include features like Intel Platform Monitoring Technology
as well as others that are not supported by the intel_pmt driver. Add a
driver for creating platform devices for these capabilities coming from
OOBMSM.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 MAINTAINERS                  |  1 +
 drivers/mfd/Kconfig          | 11 +++++++
 drivers/mfd/Makefile         |  1 +
 drivers/mfd/intel_oobmsm.c   | 61 ++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/Kconfig |  4 +--
 5 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mfd/intel_oobmsm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ebdc2a0f794b..0961e3f89497 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9356,6 +9356,7 @@ INTEL PMT DRIVER
 M:	"David E. Box" <david.e.box@linux.intel.com>
 S:	Maintained
 F:	drivers/mfd/intel_extended_cap.c
+F:	drivers/mfd/intel_oobmsm.c
 F:	drivers/mfd/intel_pmt.c
 F:	drivers/platform/x86/intel_pmt_*
 
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4dde8e223a9e..269312de2666 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -687,6 +687,17 @@ config MFD_INTEL_PMT
 	  Telemetry, Watcher, and Crashlog PMT capabilities/devices for
 	  platforms starting from Tiger Lake.
 
+config MFD_INTEL_OOBMSM
+	tristate "Intel Out Of Band Management Services Module (OOBMSM) support"
+	depends on PCI
+	select MFD_INTEL_EXTENDED_CAPS
+	help
+	  The Intel Out of Band Management Service Module driver is used to
+	  enumerate auxiliary platform features described in both Vendor
+	  Specific and Designated Vendor Specific PCIe config space. Supported
+	  features include Intel Platform Monitoring Technology (PMT) as well
+	  as other non-PMT capabilities.
+
 config MFD_IPAQ_MICRO
 	bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
 	depends on SA1100_H3100 || SA1100_H3600
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 7fa35399ec76..50fa38810bbd 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -213,6 +213,7 @@ obj-$(CONFIG_MFD_INTEL_EXTENDED_CAPS)	+= intel_extended_caps.o
 obj-$(CONFIG_MFD_INTEL_LPSS)	+= intel-lpss.o
 obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o
 obj-$(CONFIG_MFD_INTEL_LPSS_ACPI)	+= intel-lpss-acpi.o
+obj-$(CONFIG_MFD_INTEL_OOBMSM)	+= intel_oobmsm.o
 obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
 obj-$(CONFIG_MFD_INTEL_PMT)	+= intel_pmt.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
diff --git a/drivers/mfd/intel_oobmsm.c b/drivers/mfd/intel_oobmsm.c
new file mode 100644
index 000000000000..c66532f11c29
--- /dev/null
+++ b/drivers/mfd/intel_oobmsm.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Out of Band Management Services Module driver
+ *
+ * Copyright (c) 2021, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: David E. Box <david.e.box@linux.intel.com>
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+
+#include "intel_extended_caps.h"
+
+static int intel_oobmsm_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct intel_ext_cap_platform_info *info;
+	int ret;
+
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	info = (struct intel_ext_cap_platform_info *)id->driver_data;
+
+	ret = intel_ext_cap_probe(pdev, info);
+	if (ret)
+		return ret;
+
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_allow(&pdev->dev);
+
+	return 0;
+}
+
+static void intel_oobmsm_pci_remove(struct pci_dev *pdev)
+{
+	pm_runtime_forbid(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+}
+
+#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
+static const struct pci_device_id intel_oobmsm_pci_ids[] = {
+	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, NULL) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, intel_oobmsm_pci_ids);
+
+static struct pci_driver intel_oobmsm_pci_driver = {
+	.name = "intel-oobmsm",
+	.id_table = intel_oobmsm_pci_ids,
+	.probe = intel_oobmsm_pci_probe,
+	.remove = intel_oobmsm_pci_remove,
+};
+module_pci_driver(intel_oobmsm_pci_driver);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Intel Out of Band Management Services Module driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 60592fb88e7a..4dd3af9f848e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1226,7 +1226,7 @@ config INTEL_PMT_CLASS
 
 config INTEL_PMT_TELEMETRY
 	tristate "Intel Platform Monitoring Technology (PMT) Telemetry driver"
-	depends on MFD_INTEL_PMT
+	depends on MFD_INTEL_PMT || MFD_INTEL_OOBMSM
 	select INTEL_PMT_CLASS
 	help
 	  The Intel Platform Monitory Technology (PMT) Telemetry driver provides
@@ -1238,7 +1238,7 @@ config INTEL_PMT_TELEMETRY
 
 config INTEL_PMT_CRASHLOG
 	tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
-	depends on MFD_INTEL_PMT
+	depends on MFD_INTEL_PMT || MFD_INTEL_OOBMSM
 	select INTEL_PMT_CLASS
 	help
 	  The Intel Platform Monitoring Technology (PMT) crashlog driver provides
-- 
2.25.1

Re: [PATCH 3/4] MFD: Intel Out of Band Management Services Module (OOBMSM) driver

[Lee Jones]

On Thu, 17 Jun 2021, David E. Box wrote:

> The Intel Out of Band Management Services Module (OOBMSM) is a device
> that provides access to Intel capabilities described in PCIE vendor
> specific extended capability registers (both VSEC and DVSEC). These
> capabilities include features like Intel Platform Monitoring Technology
> as well as others that are not supported by the intel_pmt driver. Add a
> driver for creating platform devices for these capabilities coming from
> OOBMSM.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  MAINTAINERS                  |  1 +
>  drivers/mfd/Kconfig          | 11 +++++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/intel_oobmsm.c   | 61 ++++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/Kconfig |  4 +--
>  5 files changed, 76 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/mfd/intel_oobmsm.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ebdc2a0f794b..0961e3f89497 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9356,6 +9356,7 @@ INTEL PMT DRIVER
>  M:	"David E. Box" <david.e.box@linux.intel.com>
>  S:	Maintained
>  F:	drivers/mfd/intel_extended_cap.c
> +F:	drivers/mfd/intel_oobmsm.c
>  F:	drivers/mfd/intel_pmt.c
>  F:	drivers/platform/x86/intel_pmt_*
>  
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 4dde8e223a9e..269312de2666 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -687,6 +687,17 @@ config MFD_INTEL_PMT
>  	  Telemetry, Watcher, and Crashlog PMT capabilities/devices for
>  	  platforms starting from Tiger Lake.
>  
> +config MFD_INTEL_OOBMSM
> +	tristate "Intel Out Of Band Management Services Module (OOBMSM) support"
> +	depends on PCI
> +	select MFD_INTEL_EXTENDED_CAPS
> +	help
> +	  The Intel Out of Band Management Service Module driver is used to
> +	  enumerate auxiliary platform features described in both Vendor
> +	  Specific and Designated Vendor Specific PCIe config space. Supported
> +	  features include Intel Platform Monitoring Technology (PMT) as well
> +	  as other non-PMT capabilities.
> +
>  config MFD_IPAQ_MICRO
>  	bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
>  	depends on SA1100_H3100 || SA1100_H3600
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7fa35399ec76..50fa38810bbd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -213,6 +213,7 @@ obj-$(CONFIG_MFD_INTEL_EXTENDED_CAPS)	+= intel_extended_caps.o
>  obj-$(CONFIG_MFD_INTEL_LPSS)	+= intel-lpss.o
>  obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o
>  obj-$(CONFIG_MFD_INTEL_LPSS_ACPI)	+= intel-lpss-acpi.o
> +obj-$(CONFIG_MFD_INTEL_OOBMSM)	+= intel_oobmsm.o
>  obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
>  obj-$(CONFIG_MFD_INTEL_PMT)	+= intel_pmt.o
>  obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
> diff --git a/drivers/mfd/intel_oobmsm.c b/drivers/mfd/intel_oobmsm.c
> new file mode 100644
> index 000000000000..c66532f11c29
> --- /dev/null
> +++ b/drivers/mfd/intel_oobmsm.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Out of Band Management Services Module driver
> + *
> + * Copyright (c) 2021, Intel Corporation.
> + * All Rights Reserved.
> + *
> + * Author: David E. Box <david.e.box@linux.intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>

This doesn't appear to have anything to do with MFD?

> +#include "intel_extended_caps.h"
> +
> +static int intel_oobmsm_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct intel_ext_cap_platform_info *info;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	info = (struct intel_ext_cap_platform_info *)id->driver_data;
> +
> +	ret = intel_ext_cap_probe(pdev, info);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_put(&pdev->dev);
> +	pm_runtime_allow(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static void intel_oobmsm_pci_remove(struct pci_dev *pdev)
> +{
> +	pm_runtime_forbid(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +}
> +
> +#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
> +static const struct pci_device_id intel_oobmsm_pci_ids[] = {
> +	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, NULL) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(pci, intel_oobmsm_pci_ids);
> +
> +static struct pci_driver intel_oobmsm_pci_driver = {
> +	.name = "intel-oobmsm",
> +	.id_table = intel_oobmsm_pci_ids,
> +	.probe = intel_oobmsm_pci_probe,
> +	.remove = intel_oobmsm_pci_remove,
> +};
> +module_pci_driver(intel_oobmsm_pci_driver);
> +
> +MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
> +MODULE_DESCRIPTION("Intel Out of Band Management Services Module driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 60592fb88e7a..4dd3af9f848e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1226,7 +1226,7 @@ config INTEL_PMT_CLASS
>  
>  config INTEL_PMT_TELEMETRY
>  	tristate "Intel Platform Monitoring Technology (PMT) Telemetry driver"
> -	depends on MFD_INTEL_PMT
> +	depends on MFD_INTEL_PMT || MFD_INTEL_OOBMSM
>  	select INTEL_PMT_CLASS
>  	help
>  	  The Intel Platform Monitory Technology (PMT) Telemetry driver provides
> @@ -1238,7 +1238,7 @@ config INTEL_PMT_TELEMETRY
>  
>  config INTEL_PMT_CRASHLOG
>  	tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
> -	depends on MFD_INTEL_PMT
> +	depends on MFD_INTEL_PMT || MFD_INTEL_OOBMSM
>  	select INTEL_PMT_CLASS
>  	help
>  	  The Intel Platform Monitoring Technology (PMT) crashlog driver provides

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/4] MFD: intel-extended-cap: Add support for PCIe VSEC structures

[David E. Box]

Adds support for discovering Intel extended capability features from
Vendor Specific Extended Capability (VSEC) registers in PCIe config space.
While doing so place the existing DVSEC and new VSEC code in separate
functions.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/mfd/intel_extended_caps.c | 159 ++++++++++++++++++++----------
 1 file changed, 109 insertions(+), 50 deletions(-)

diff --git a/drivers/mfd/intel_extended_caps.c b/drivers/mfd/intel_extended_caps.c
index 89cf1ae6f65b..9b60defda856 100644
--- a/drivers/mfd/intel_extended_caps.c
+++ b/drivers/mfd/intel_extended_caps.c
@@ -105,7 +105,7 @@ static int intel_ext_cap_add_dev(struct pci_dev *pdev, struct intel_ext_cap_head
 		header->offset >>= 3;
 
 	/*
-	 * The DVSEC contains the starting offset and count for a block of
+	 * The DVSEC/VSEC contains the starting offset and count for a block of
 	 * discovery tables, each providing access to monitoring facilities for
 	 * a section of the device. Create a resource list of these tables to
 	 * provide to the driver.
@@ -124,11 +124,112 @@ static int intel_ext_cap_add_dev(struct pci_dev *pdev, struct intel_ext_cap_head
 	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cell, 1, NULL, 0, NULL);
 }
 
+static bool intel_ext_cap_walk_dvsec(struct pci_dev *pdev, unsigned long quirks)
+{
+	int count = 0;
+	int pos = 0;
+
+	do {
+		struct intel_ext_cap_header header;
+		u32 table, hdr;
+		u16 vid;
+		int ret;
+
+		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
+		if (!pos)
+			break;
+
+		pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER1, &hdr);
+		vid = PCI_DVSEC_HEADER1_VID(hdr);
+		if (vid != PCI_VENDOR_ID_INTEL)
+			continue;
+
+		/* Support only revision 1 */
+		header.rev = PCI_DVSEC_HEADER1_REV(hdr);
+		if (header.rev != 1) {
+			dev_warn(&pdev->dev, "Unsupported DVSEC revision %d\n",
+				 header.rev);
+			continue;
+		}
+
+		header.length = PCI_DVSEC_HEADER1_LEN(hdr);
+
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
+				     &header.num_entries);
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
+				     &header.entry_size);
+		pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
+				      &table);
+
+		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
+		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
+
+		pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER2, &hdr);
+		header.id = PCI_DVSEC_HEADER2_ID(hdr);
+
+		ret = intel_ext_cap_add_dev(pdev, &header, quirks);
+		if (ret)
+			continue;
+
+		count++;
+	} while (true);
+
+	return count;
+}
+
+static bool intel_ext_cap_walk_vsec(struct pci_dev *pdev, unsigned long quirks)
+{
+	int count = 0;
+	int pos = 0;
+
+	do {
+		struct intel_ext_cap_header header;
+		u32 table, hdr;
+		int ret;
+
+		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_VNDR);
+		if (!pos)
+			break;
+
+		pci_read_config_dword(pdev, pos + PCI_VNDR_HEADER, &hdr);
+
+		/* Support only revision 1 */
+		header.rev = PCI_VNDR_HEADER_REV(hdr);
+		if (header.rev != 1) {
+			dev_warn(&pdev->dev, "Unsupported VSEC revision %d\n",
+				 header.rev);
+			continue;
+		}
+
+		header.id = PCI_VNDR_HEADER_ID(hdr);
+		header.length = PCI_VNDR_HEADER_LEN(hdr);
+
+		/* entry, size, and table offset are the same as DVSEC */
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
+				     &header.num_entries);
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
+				     &header.entry_size);
+		pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
+				      &table);
+
+		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
+		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
+
+		ret = intel_ext_cap_add_dev(pdev, &header, quirks);
+		if (ret)
+			continue;
+
+		count++;
+	} while (true);
+
+	return count;
+}
+
 int intel_ext_cap_probe(struct pci_dev *pdev, struct intel_ext_cap_platform_info *info)
 {
 	unsigned long quirks = 0;
-	bool found_devices = false;
-	int ret, pos;
+	int device_count = 0;
+	int ret;
 
 	if (info)
 		quirks = info->quirks;
@@ -144,59 +245,17 @@ int intel_ext_cap_probe(struct pci_dev *pdev, struct intel_ext_cap_platform_info
 					 "Failed to add device for DVSEC id %d\n",
 					 (*header)->id);
 			else
-				found_devices = true;
+				device_count++;
 
 			header++;
 		}
-	} else {
-		/* Find DVSEC features */
-		pos = 0;
-		do {
-			struct intel_ext_cap_header header;
-			u32 table, hdr;
-			u16 vid;
-
-			pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
-			if (!pos)
-				break;
-
-			pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER1, &hdr);
-			vid = PCI_DVSEC_HEADER1_VID(hdr);
-			if (vid != PCI_VENDOR_ID_INTEL)
-				continue;
-
-			/* Support only revision 1 */
-			header.rev = PCI_DVSEC_HEADER1_REV(hdr);
-			if (header.rev != 1) {
-				dev_warn(&pdev->dev, "Unsupported DVSEC revision %d\n",
-					 header.rev);
-				continue;
-			}
-
-			header.length = PCI_DVSEC_HEADER1_LEN(hdr);
-
-			pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
-					     &header.num_entries);
-			pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
-					     &header.entry_size);
-			pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
-					      &table);
-
-			header.tbir = INTEL_DVSEC_TABLE_BAR(table);
-			header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
-
-			pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER2, &hdr);
-			header.id = PCI_DVSEC_HEADER2_ID(hdr);
-
-			ret = intel_ext_cap_add_dev(pdev, &header, quirks);
-			if (ret)
-				continue;
 
-			found_devices = true;
-		} while (true);
+	} else {
+		device_count += intel_ext_cap_walk_dvsec(pdev, quirks);
+		device_count += intel_ext_cap_walk_vsec(pdev, quirks);
 	}
 
-	if (!found_devices)
+	if (!device_count)
 		return -ENODEV;
 
 	return 0;
-- 
2.25.1

[PATCH 0/4] Auxiliary bus driver support for Intel PCIe VSEC/DVSEC

[David E. Box]

This series makes changes to the current intel_pmt driver to give it
broader support for Intel defined PCIe VSEC and DVSEC features. It
moves the implementation from MFD to the auxiliary bus and creates a
generic framework for enumerating the extended capabilities. It also
adds support for a new VSEC, Software Defined Silicon (SDSi).

David E. Box (4):
  PCI: Add #defines for accessing PCIe DVSEC fields
  driver core: auxiliary bus: Add driver data helpers
  platform/x86/intel: Move intel_pmt from MFD to Auxiliary Bus
  platform/x86: Add Intel Software Defined Silicon driver

 .../ABI/testing/sysfs-driver-intel_sdsi       |  75 +++
 MAINTAINERS                                   |  17 +-
 drivers/mfd/Kconfig                           |  10 -
 drivers/mfd/Makefile                          |   1 -
 drivers/mfd/intel_pmt.c                       | 261 --------
 drivers/platform/x86/intel/Kconfig            |  23 +
 drivers/platform/x86/intel/Makefile           |   4 +
 drivers/platform/x86/intel/pmt/Kconfig        |   4 +-
 drivers/platform/x86/intel/pmt/class.c        |  21 +-
 drivers/platform/x86/intel/pmt/class.h        |   5 +-
 drivers/platform/x86/intel/pmt/crashlog.c     |  47 +-
 drivers/platform/x86/intel/pmt/telemetry.c    |  46 +-
 drivers/platform/x86/intel/sdsi.c             | 571 ++++++++++++++++++
 drivers/platform/x86/intel/vsec.c             | 418 +++++++++++++
 drivers/platform/x86/intel/vsec.h             |  43 ++
 include/linux/auxiliary_bus.h                 |  10 +
 include/uapi/linux/pci_regs.h                 |   4 +
 17 files changed, 1225 insertions(+), 335 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel_sdsi
 delete mode 100644 drivers/mfd/intel_pmt.c
 create mode 100644 drivers/platform/x86/intel/sdsi.c
 create mode 100644 drivers/platform/x86/intel/vsec.c
 create mode 100644 drivers/platform/x86/intel/vsec.h

-- 
2.25.1

[PATCH 1/4] PCI: Add #defines for accessing PCIe DVSEC fields

[David E. Box]

Add #defines for accessing Vendor ID, Revision, Length, and ID offsets
in the Designated Vendor Specific Extended Capability (DVSEC). Defined
in PCIe r5.0, sec 7.9.6.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/uapi/linux/pci_regs.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index ff6ccbc6efe9..318f3f1f9e92 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1086,7 +1086,11 @@
 
 /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
 #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
+#define  PCI_DVSEC_HEADER1_VID(x)	((x) & 0xffff)
+#define  PCI_DVSEC_HEADER1_REV(x)	(((x) >> 16) & 0xf)
+#define  PCI_DVSEC_HEADER1_LEN(x)	(((x) >> 20) & 0xfff)
 #define PCI_DVSEC_HEADER2		0x8 /* Designated Vendor-Specific Header2 */
+#define  PCI_DVSEC_HEADER2_ID(x)		((x) & 0xffff)
 
 /* Data Link Feature */
 #define PCI_DLF_CAP		0x04	/* Capabilities Register */
-- 
2.25.1

Re: [PATCH 1/4] PCI: Add #defines for accessing PCIe DVSEC fields

[Greg KH]

On Sat, Nov 20, 2021 at 03:17:02PM -0800, David E. Box wrote:
> Add #defines for accessing Vendor ID, Revision, Length, and ID offsets
> in the Designated Vendor Specific Extended Capability (DVSEC). Defined
> in PCIe r5.0, sec 7.9.6.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  include/uapi/linux/pci_regs.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index ff6ccbc6efe9..318f3f1f9e92 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1086,7 +1086,11 @@
>  
>  /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
>  #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
> +#define  PCI_DVSEC_HEADER1_VID(x)	((x) & 0xffff)
> +#define  PCI_DVSEC_HEADER1_REV(x)	(((x) >> 16) & 0xf)
> +#define  PCI_DVSEC_HEADER1_LEN(x)	(((x) >> 20) & 0xfff)
>  #define PCI_DVSEC_HEADER2		0x8 /* Designated Vendor-Specific Header2 */
> +#define  PCI_DVSEC_HEADER2_ID(x)		((x) & 0xffff)

Why does userspace need to have these defines?  What userspace tool is
going to use these?

thanks,

greg k-h

Re: [PATCH 1/4] PCI: Add #defines for accessing PCIe DVSEC fields

[David E. Box]

On Sun, 2021-11-21 at 13:24 +0100, Greg KH wrote:
> On Sat, Nov 20, 2021 at 03:17:02PM -0800, David E. Box wrote:
> > Add #defines for accessing Vendor ID, Revision, Length, and ID offsets
> > in the Designated Vendor Specific Extended Capability (DVSEC). Defined
> > in PCIe r5.0, sec 7.9.6.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  include/uapi/linux/pci_regs.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index ff6ccbc6efe9..318f3f1f9e92 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -1086,7 +1086,11 @@
> >  
> >  /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
> >  #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific
> > Header1 */
> > +#define  PCI_DVSEC_HEADER1_VID(x)	((x) & 0xffff)
> > +#define  PCI_DVSEC_HEADER1_REV(x)	(((x) >> 16) & 0xf)
> > +#define  PCI_DVSEC_HEADER1_LEN(x)	(((x) >> 20) & 0xfff)
> >  #define PCI_DVSEC_HEADER2		0x8 /* Designated Vendor-Specific
> > Header2 */
> > +#define  PCI_DVSEC_HEADER2_ID(x)		((x) & 0xffff)
> 
> Why does userspace need to have these defines?  What userspace tool is
> going to use these?

These headers should useful for userspace tools that access PCI devices. DVSEC
is also used by CXL so they should come in handy for tools accesses those
devices.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/4] PCI: Add #defines for accessing PCIe DVSEC fields

[Bjorn Helgaas]

On Sun, Nov 21, 2021 at 01:24:09PM +0100, Greg KH wrote:
> On Sat, Nov 20, 2021 at 03:17:02PM -0800, David E. Box wrote:
> > Add #defines for accessing Vendor ID, Revision, Length, and ID offsets
> > in the Designated Vendor Specific Extended Capability (DVSEC). Defined
> > in PCIe r5.0, sec 7.9.6.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  include/uapi/linux/pci_regs.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index ff6ccbc6efe9..318f3f1f9e92 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -1086,7 +1086,11 @@
> >  
> >  /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
> >  #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
> > +#define  PCI_DVSEC_HEADER1_VID(x)	((x) & 0xffff)
> > +#define  PCI_DVSEC_HEADER1_REV(x)	(((x) >> 16) & 0xf)
> > +#define  PCI_DVSEC_HEADER1_LEN(x)	(((x) >> 20) & 0xfff)
> >  #define PCI_DVSEC_HEADER2		0x8 /* Designated Vendor-Specific Header2 */
> > +#define  PCI_DVSEC_HEADER2_ID(x)		((x) & 0xffff)
> 
> Why does userspace need to have these defines?  What userspace tool is
> going to use these?

This is pretty typical of other content in pci_regs.h.  There's lots
of existing content that may not be directly useful to userspace, but
it's there because it's convenient to have definitions prescribed by
the spec all together in one place.

They probably shouldn't go in drivers/pci/pci.h because by definition
this vendor-specific stuff will be used by vendor-specific drivers but
not by the PCI core.

I guess these *could* go in include/linux/pci.h, but that wouldn't be
my first choice because there's nothing similar there.

Bjorn