[PATCH v3 00/10] Crescent Island PMT support

[PATCH v3 00/10] Crescent Island PMT support

[Michael J. Ruhl]

The Crescent Island (CRI) GPU supports PMT via Xe driver
registration.

The CRI PMT MMIO mapping is shared for each PMT feature and it is
necessary to set an index register value to access the correct memory
space.

The PMT driver has a callback infrastructure to access data areas with
driver specific access. It is does not support the PMT control access.

CRI discovery/control spaces are part of the access window so a driver
callback is necessary to allow for the correct window to be exposed.

Add control path callback support to the PMT driver.

Add CRI PMT discovery information.

Update the Xe driver to support the CRI PMT access.

The crashlog access needs to be done with power enabled (telem is only
available when the device is powered).

Determine which access is requested, and do the appropriate power
request.

CRI FW is loaded at device power.  Support the late-binding API to
wait for FW readiness. (pending sysctl patch set implements the API).

v2:
  Address AI feedback:
  - add missing entry->cb check
  - address SRIOV VF concerns
  - use xe_pm_runtime_put instead of _get in error path

  Address review comments:
  - redo power/lock sequencing
  - support device hotplug events

v3:
  - cleanup power manangement flow
  - fixed an out of order power management locking issue
  - add a GUID caching mechanism, for GUID read
  - removed unused parameter (mmio)


Michael J. Ruhl (10):
  platform/x86/intel/pmt: complete pcidev to device update
  platform/x86/intel/pmt: Add register access callbacks
  drm/xe/vsec: Protect against missing config
  drm/xe/vsec: Use correct pm state get
  drm/xe/vsec: Support possible hotplug exit
  drm/xe/vsec: Support Crescent Island PMT
  drm/xe/vsec: Crescent Island PMT decode
  drm/xe/vsec: Crescent Island PMT callbacks
  drm/xe/vsec: Support late bind fw information
  drm/xe/vsec: Update PMT internal access for CRI

 drivers/gpu/drm/xe/regs/xe_pmt.h           |   9 +-
 drivers/gpu/drm/xe/xe_debugfs.c            |  44 +-
 drivers/gpu/drm/xe/xe_device.c             |   4 +-
 drivers/gpu/drm/xe/xe_device_types.h       |   8 +
 drivers/gpu/drm/xe/xe_hwmon.c              |  10 +-
 drivers/gpu/drm/xe/xe_pcode.c              |  10 +-
 drivers/gpu/drm/xe/xe_vsec.c               | 536 +++++++++++++++++++--
 drivers/gpu/drm/xe/xe_vsec.h               |   3 +-
 drivers/platform/x86/intel/pmt/class.c     |   5 +-
 drivers/platform/x86/intel/pmt/class.h     |   3 +-
 drivers/platform/x86/intel/pmt/crashlog.c  |  39 +-
 drivers/platform/x86/intel/pmt/discovery.c |   2 +-
 include/linux/intel_vsec.h                 |  14 +-
 13 files changed, 607 insertions(+), 80 deletions(-)

-- 
2.43.0

[PATCH v3 01/10] platform/x86/intel/pmt: complete pcidev to device update

[Michael J. Ruhl]

Previously the pcidev usage was moved to the struct device.
The struct device is set for only the telemetry endpoints.

This usage prevents other PMT features (crashlog) from using
the callback mechanism.

Use struct device in the intel_pmt_entry.
Update callback usage to os the pmt entry rather than the
telemetry endpoint.

Fixes: 353042d54d82 ("platform/x86/intel/vsec: Switch exported helpers from pci_dev to device")
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/class.c     | 5 ++---
 drivers/platform/x86/intel/pmt/class.h     | 3 +--
 drivers/platform/x86/intel/pmt/discovery.c | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index d0ab8e33c62a..402d51df834a 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -100,7 +100,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
 	if (count > entry->size - off)
 		count = entry->size - off;
 
-	count = pmt_telem_read_mmio(entry->ep->dev, entry->cb, entry->header.guid, buf,
+	count = pmt_telem_read_mmio(entry->dev, entry->cb, entry->header.guid, buf,
 				    entry->base, off, count);
 
 	return count;
@@ -286,8 +286,6 @@ static int pmt_resolve_access_pci(struct intel_pmt_entry *entry,
 		return -EINVAL;
 	}
 
-	entry->pcidev = pci_dev;
-
 	return 0;
 }
 
@@ -365,6 +363,7 @@ static int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
 	entry->guid = header->guid;
 	entry->size = header->size;
 	entry->cb = ivdev->priv_data;
+	entry->dev = ivdev->dev;
 
 	return 0;
 }
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index a0ece4fc3837..258cb460e61c 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -20,7 +20,6 @@
 #define GET_ADDRESS(v)		((v) & GENMASK(31, 3))
 
 struct device;
-struct pci_dev;
 extern struct class intel_pmt_class;
 
 struct telem_endpoint {
@@ -42,7 +41,7 @@ struct intel_pmt_header {
 
 struct intel_pmt_entry {
 	struct telem_endpoint	*ep;
-	struct pci_dev		*pcidev;
+	struct device		*dev;
 	struct intel_pmt_header	header;
 	u32			disc_header[PMT_DISC_DWORDS];
 	struct bin_attribute	pmt_bin_attr;
diff --git a/drivers/platform/x86/intel/pmt/discovery.c b/drivers/platform/x86/intel/pmt/discovery.c
index c482368bfaae..f4203d240f54 100644
--- a/drivers/platform/x86/intel/pmt/discovery.c
+++ b/drivers/platform/x86/intel/pmt/discovery.c
@@ -609,7 +609,7 @@ void intel_pmt_get_features(struct intel_pmt_entry *entry)
 
 	mutex_lock(&feature_list_lock);
 	list_for_each_entry(feature, &pmt_feature_list, list) {
-		if (feature->priv->parent != entry->ep->dev)
+		if (feature->priv->parent != entry->dev)
 			continue;
 
 		pmt_get_features(entry, feature);
-- 
2.43.0

[PATCH v3 02/10] platform/x86/intel/pmt: Add register access callbacks

[Michael J. Ruhl]

Some HW does not have direct MMIO access to PMT control and data
features.

Augment the current callback infrastructure (data access) to allow
a registered driver to customize read/write access to the control
paths for PMT usage.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 39 +++++++++++++++++++++--
 include/linux/intel_vsec.h                | 14 +++++++-
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index f936daf99e4d..5923ad7abbd9 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -129,7 +129,19 @@ static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 {
 	const struct crashlog_control *control = &crashlog->info->control;
 	struct intel_pmt_entry *entry = &crashlog->entry;
-	u32 reg = readl(entry->disc_table + control->offset);
+	u32 guid = entry->header.guid;
+	u32 reg;
+	int err;
+
+	if (entry->cb && entry->cb->read_reg) {
+		err = entry->cb->read_reg(entry->dev, guid, &reg, control->offset);
+		if (err) {
+			pr_err("%s: failed to read reg: %d\n", __func__, err);
+			return;
+		}
+	} else {
+		reg = readl(entry->disc_table + control->offset);
+	}
 
 	reg &= ~control->trigger_mask;
 
@@ -138,14 +150,35 @@ static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 	else
 		reg &= ~bit;
 
-	writel(reg, entry->disc_table + control->offset);
+	if (entry->cb && entry->cb->write_reg) {
+		err = entry->cb->write_reg(entry->dev, guid, reg, control->offset);
+		if (err) {
+			pr_err("%s: failed to write reg: %d\n", __func__, err);
+			return;
+		}
+	} else {
+		writel(reg, entry->disc_table + control->offset);
+	}
 }
 
 /* Read the status register and see if the specified @bit is set */
 static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
 {
 	const struct crashlog_status *status = &crashlog->info->status;
-	u32 reg = readl(crashlog->entry.disc_table + status->offset);
+	struct intel_pmt_entry *entry = &crashlog->entry;
+	u32 guid = entry->header.guid;
+	u32 reg;
+	int err;
+
+	if (entry->cb && entry->cb->read_reg) {
+		err = entry->cb->read_reg(entry->dev, guid, &reg, status->offset);
+		if (err) {
+			pr_err("%s: failed to read reg: %d\n", __func__, err);
+			return false;
+		}
+	} else {
+		reg = readl(crashlog->entry.disc_table + status->offset);
+	}
 
 	return !!(reg & bit);
 }
diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
index 843cda8f8644..917d9397a993 100644
--- a/include/linux/intel_vsec.h
+++ b/include/linux/intel_vsec.h
@@ -90,13 +90,25 @@ enum intel_vsec_quirks {
  * @read_telem: when specified, called by client driver to access PMT
  * data (instead of direct copy).
  * * dev:   device reference for the callback's use
- * * guid:  ID of data to acccss
+ * * guid:  ID of data to access
  * * data:  buffer for the data to be copied
  * * off:   offset into the requested buffer
  * * count: size of buffer
+ * @read_reg: when specified called by client driver to read PMT state
+ * * dev:      device reference for the callback's use
+ * * guid:     ID of data to access
+ * * reg_data: register data
+ * * offset:   offset of register to read
+ * @write_reg: when specified called by client driver to write PMT state
+ * * dev:      device reference for the callback's use
+ * * guid:     ID of data to access
+ * * reg_data: register data
+ * * offset:   offset of register to write
  */
 struct pmt_callbacks {
 	int (*read_telem)(struct device *dev, u32 guid, u64 *data, loff_t off, u32 count);
+	int (*read_reg)(struct device *dev, u32 guid, u32 *reg_data, u32 offset);
+	int (*write_reg)(struct device *dev, u32 guid, u32 reg_data, u32 offset);
 };
 
 struct vsec_feature_dependency {
-- 
2.43.0

[PATCH v3 03/10] drm/xe/vsec: Protect against missing config

[Michael J. Ruhl]

Some init paths (VF) will not provide soc-remapper
callbacks.

BMG PMT access requires the soc-remapper.

If the soc-remapper is not configured, do not register for
PMT, and block read access.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_vsec.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index a9baf0bfe572..bd83a33aef6c 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -149,6 +149,9 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 	u32 offset;
 	int ret;
 
+	if (!xe->soc_remapper.set_telem_region)
+		return -ENODEV;
+
 	ret = xe_guid_decode(guid, &mem_region, &offset);
 	if (ret)
 		return ret;
@@ -157,9 +160,6 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	guard(mutex)(&xe->pmt.lock);
 
-	if (!xe->soc_remapper.set_telem_region)
-		return -ENODEV;
-
 	/* indicate that we are not at an appropriate power level */
 	if (!xe_pm_runtime_get_if_active(xe))
 		return -ENODATA;
@@ -210,6 +210,8 @@ void xe_vsec_init(struct xe_device *xe)
 
 	switch (platform) {
 	case XE_VSEC_BMG:
+		if (!xe->soc_remapper.set_telem_region)
+			return;
 		info->priv_data = &xe_pmt_cb;
 		break;
 	default:
-- 
2.43.0

[PATCH v3 04/10] drm/xe/vsec: Use correct pm state get

[Michael J. Ruhl]

Crashlog needs to be collected at all times.  The current pm
check assumes telemetry only.

Update read path to enable device for crashlog instances.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Fixes: 2c402a801c19 ("platform/x86/intel/pmt: support BMG crashlog")
---
 drivers/gpu/drm/xe/xe_vsec.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index bd83a33aef6c..8d99a3770b5a 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -145,6 +145,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 {
 	struct xe_device *xe = kdev_to_xe_device(dev);
 	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
+	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
 	u32 mem_region;
 	u32 offset;
 	int ret;
@@ -158,16 +159,28 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	telem_addr += offset + user_offset;
 
-	guard(mutex)(&xe->pmt.lock);
+	/* Always allow crashlog. Telemetry, only when powered */
+	switch (cap_type) {
+	case CRASHLOG:
+		xe_pm_runtime_get(xe);
+		break;
+	case TELEMETRY:
+		if (!xe_pm_runtime_get_if_active(xe))
+			return -ENODATA;
+		break;
+	case WATCHER:
+		return -EINVAL;
+	}
 
-	/* indicate that we are not at an appropriate power level */
-	if (!xe_pm_runtime_get_if_active(xe))
-		return -ENODATA;
+	mutex_lock(&xe->pmt.lock);
 
 	/* set SoC re-mapper index register based on GUID memory region */
 	xe->soc_remapper.set_telem_region(xe, mem_region);
 
 	memcpy_fromio(data, telem_addr, count);
+
+	mutex_unlock(&xe->pmt.lock);
+
 	xe_pm_runtime_put(xe);
 
 	return count;
-- 
2.43.0

[PATCH v3 05/10] drm/xe/vsec: Support possible hotplug exit

[Michael J. Ruhl]

DRM has an API that will verify that a device is valid in
the hotplug context.

Verify device is valid before access in the VSEC callback
API.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_vsec.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 8d99a3770b5a..8abe11e6312f 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -10,6 +10,8 @@
 #include <linux/pci.h>
 #include <linux/types.h>
 
+#include <drm/drm_drv.h>
+
 #include "xe_device.h"
 #include "xe_device_types.h"
 #include "xe_mmio.h"
@@ -140,6 +142,11 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
 	return 0;
 }
 
+/*
+ * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
+ * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
+ * verified (drm_dev_enter()).
+ */
 int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset,
 		      u32 count)
 {
@@ -148,14 +155,20 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
 	u32 mem_region;
 	u32 offset;
-	int ret;
+	int ret = 0;
+	int idx;
 
-	if (!xe->soc_remapper.set_telem_region)
+	if (!drm_dev_enter(&xe->drm, &idx))
 		return -ENODEV;
 
+	if (!xe->soc_remapper.set_telem_region) {
+		ret = -EINVAL;
+		goto dev_exit;
+	}
+
 	ret = xe_guid_decode(guid, &mem_region, &offset);
 	if (ret)
-		return ret;
+		goto dev_exit;
 
 	telem_addr += offset + user_offset;
 
@@ -165,11 +178,14 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 		xe_pm_runtime_get(xe);
 		break;
 	case TELEMETRY:
-		if (!xe_pm_runtime_get_if_active(xe))
-			return -ENODATA;
+		if (!xe_pm_runtime_get_if_active(xe)) {
+			ret = -ENODATA;
+			goto dev_exit;
+		}
 		break;
 	case WATCHER:
-		return -EINVAL;
+		ret = -EINVAL;
+		goto dev_exit;
 	}
 
 	mutex_lock(&xe->pmt.lock);
@@ -183,7 +199,10 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	xe_pm_runtime_put(xe);
 
-	return count;
+dev_exit:
+	drm_dev_exit(idx);
+
+	return ret == 0 ? count : ret;
 }
 
 static struct pmt_callbacks xe_pmt_cb = {
-- 
2.43.0

[PATCH v3 06/10] drm/xe/vsec: Support Crescent Island PMT

[Michael J. Ruhl]

Crescent Island (CRI) supports PMT telemetry and crashlog.

Add Crescent Island (CRI) discovery structure (DVSEC)
information to allow for Xe registration.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/regs/xe_pmt.h |  5 ++++
 drivers/gpu/drm/xe/xe_vsec.c     | 46 ++++++++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
index a62ab05c6b4c..fc9c9cb6a830 100644
--- a/drivers/gpu/drm/xe/regs/xe_pmt.h
+++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
@@ -20,6 +20,11 @@
 #define BMG_TELEMETRY_BASE_OFFSET	0xE0000
 #define BMG_TELEMETRY_OFFSET		(SOC_BASE + BMG_TELEMETRY_BASE_OFFSET)
 
+#define CRI_TELEMETRY_BASE_OFFSET	0xE0000
+/* for CRI discovery and telemetry are in an indexed window */
+#define CRI_DISCOVERY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
+#define CRI_TELEMETRY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
+
 #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
 #define BMG_G2_RESIDENCY_OFFSET		(0x530)
 #define BMG_G6_RESIDENCY_OFFSET		(0x538)
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 8abe11e6312f..6345b0b4b26b 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -21,8 +21,19 @@
 
 #include "regs/xe_pmt.h"
 
-/* PMT GUID value for BMG devices.  NOTE: this is NOT a PCI id */
+/* PMT GUID value for BMG and CRI devices.  NOTE: this is NOT a PCI id */
 #define BMG_DEVICE_ID 0xE2F8
+#define CRI_DEVICE_ID 0xE2FA
+
+/*
+ * sizeof(Crashlog Type1 Version2) = 0x18 (24) bytes
+ * For BMG and CRI PUNIT and OOBMSMS crashlogs are consecutive.
+ */
+#define BMG_CRASHLOG_PUNIT_DISC_OFFSET (0x60)
+#define BMG_CRASHLOG_OOBMSM_DISC_OFFSET (BMG_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
+
+#define CRI_CRASHLOG_PUNIT_DISC_OFFSET (0x80)
+#define CRI_CRASHLOG_OOBMSM_DISC_OFFSET (CRI_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
 
 static struct intel_vsec_header bmg_telemetry = {
 	.rev = 1,
@@ -41,7 +52,7 @@ static struct intel_vsec_header bmg_crashlog = {
 	.num_entries = 2,
 	.entry_size = 6,
 	.tbir = 0,
-	.offset = BMG_DISCOVERY_OFFSET + 0x60,
+	.offset = BMG_DISCOVERY_OFFSET + BMG_CRASHLOG_PUNIT_DISC_OFFSET,
 };
 
 static struct intel_vsec_header *bmg_capabilities[] = {
@@ -50,9 +61,36 @@ static struct intel_vsec_header *bmg_capabilities[] = {
 	NULL
 };
 
+static struct intel_vsec_header cri_telemetry = {
+	.rev = 1,
+	.length = 0x10,
+	.id = VSEC_ID_TELEMETRY,
+	.num_entries = 3,
+	.entry_size = 4,
+	.tbir = 0,
+	.offset = CRI_DISCOVERY_OFFSET,
+};
+
+static struct intel_vsec_header cri_crashlog = {
+	.rev = 1,
+	.length = 0x10,
+	.id = VSEC_ID_CRASHLOG,
+	.num_entries = 2,
+	.entry_size = 6,
+	.tbir = 0,
+	.offset = CRI_DISCOVERY_OFFSET + CRI_CRASHLOG_PUNIT_DISC_OFFSET,
+};
+
+static struct intel_vsec_header *cri_capabilities[] = {
+	&cri_telemetry,
+	&cri_crashlog,
+	NULL
+};
+
 enum xe_vsec {
 	XE_VSEC_UNKNOWN = 0,
 	XE_VSEC_BMG,
+	XE_VSEC_CRI,
 };
 
 static struct intel_vsec_platform_info xe_vsec_info[] = {
@@ -60,6 +98,10 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
 		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
 		.headers = bmg_capabilities,
 	},
+	[XE_VSEC_CRI] = {
+		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
+		.headers = cri_capabilities,
+	},
 	{ }
 };
 
-- 
2.43.0

[PATCH v3 07/10] drm/xe/vsec: Crescent Island PMT decode

[Michael J. Ruhl]

Crescent Island (CRI) has different index and offset values for
accessing the PMT data area.

Update the decode path to support the CRI device.

Update the data read callback so to support the CRI usage.

Define several magic numbers.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_device_types.h |   2 +
 drivers/gpu/drm/xe/xe_vsec.c         | 135 ++++++++++++++++++++++-----
 2 files changed, 114 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 180d450a6deb..3f1a70813a99 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -466,6 +466,8 @@ struct xe_device {
 	struct {
 		/** @pmt.lock: protect access for telemetry data */
 		struct mutex lock;
+		/** @pmt.base_offset: device specific base offset */
+		u64 base_offset;
 	} pmt;
 
 	/** @soc_remapper: SoC remapper object */
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 6345b0b4b26b..dc42b9492428 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -108,23 +108,47 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
 /*
  * The GUID will have the following bits to decode:
  *   [0:3]   - {Telemetry space iteration number (0,1,..)}
- *   [4:7]   - Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
+ *   [4:7]   - BMG Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
+ *   [4:5]   - CRI Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
+ *   [6:7]   - CRI Instance
  *   [8:11]  - SOC_SKU
  *   [12:27] – Device ID – changes for each down bin SKU’s
  *   [28:29] - Capability Type (Crashlog-0, Telemetry Aggregator-1, Watcher-2)
  *   [30:31] - Record-ID (0-PUNIT, 1-OOBMSM_0, 2-OOBMSM_1)
  */
 #define GUID_TELEM_ITERATION	GENMASK(3, 0)
-#define GUID_SEGMENT		GENMASK(7, 4)
 #define GUID_SOC_SKU		GENMASK(11, 8)
 #define GUID_DEVICE_ID		GENMASK(27, 12)
 #define GUID_CAP_TYPE		GENMASK(29, 28)
 #define GUID_RECORD_ID		GENMASK(31, 30)
 
-#define PUNIT_TELEMETRY_OFFSET		0x0200
-#define PUNIT_WATCHER_OFFSET		0x14A0
-#define OOBMSM_0_WATCHER_OFFSET		0x18D8
-#define OOBMSM_1_TELEMETRY_OFFSET	0x1000
+#define BMG_GUID_SEGMENT	GENMASK(7, 4)
+
+#define CRI_GUID_SEGMENT	GENMASK(5, 4)
+#define CRI_GUID_INSTANCE	GENMASK(7, 6)
+
+#define BMG_IDX_TELEM_PUNIT		0x00
+#define BMG_IDX_TELEM_OOBMSM		0x01
+#define BMG_IDX_CRASHLOG_PUNIT		0x02
+#define BMG_IDX_CRASHLOG_OOBMSM		0x04
+
+#define BMG_PUNIT_TELEMETRY_OFFSET	0x0200
+#define BMG_PUNIT_WATCHER_OFFSET	0x14A0
+#define BMG_OOBMSM_0_WATCHER_OFFSET	0x18D8
+#define BMG_OOBMSM_1_TELEMETRY_OFFSET	0x1000
+
+#define CRI_IDX_TELEM_DISCOVERY		0x00
+#define CRI_IDX_TELEM_PUNIT		0x01
+#define CRI_IDX_TELEM_OOBMSM		0x02
+#define CRI_IDX_CRASHLOG_PUNIT		0x03
+#define CRI_IDX_WATCHER_OOBMSM		0x03  /* PUNIT and OOBMSM share this index */
+#define CRI_IDX_CRASHLOG_OOBMSM		0x04
+
+#define CRI_PUNIT_TELEMETRY_OFFSET		0x0200
+#define CRI_PUNIT_WATCHER_OFFSET		0x08A0
+#define CRI_OOBMSM_WATCHER_OFFSET		0x0CF8
+#define CRI_OOBMSM_GFSP_TELEMETRY_OFFSET	0x1600
+#define CRI_PUNIT_CRASHLOG_OFFSET		0x0E60
 
 enum record_id {
 	PUNIT,
@@ -138,45 +162,92 @@ enum capability {
 	WATCHER,
 };
 
-static int xe_guid_decode(u32 guid, int *index, u32 *offset)
+static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
 {
 	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
 	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
-	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
-
-	if (device_id != BMG_DEVICE_ID)
-		return -ENODEV;
-
-	if (cap_type > WATCHER)
-		return -EINVAL;
 
 	*offset = 0;
 
 	if (cap_type == CRASHLOG) {
-		*index = record_id == PUNIT ? 2 : 4;
+		*index = record_id == PUNIT ? BMG_IDX_CRASHLOG_PUNIT : BMG_IDX_CRASHLOG_OOBMSM;
 		return 0;
 	}
 
 	switch (record_id) {
 	case PUNIT:
-		*index = 0;
+		*index = BMG_IDX_TELEM_PUNIT;
 		if (cap_type == TELEMETRY)
-			*offset = PUNIT_TELEMETRY_OFFSET;
+			*offset = BMG_PUNIT_TELEMETRY_OFFSET;
 		else
-			*offset = PUNIT_WATCHER_OFFSET;
+			*offset = BMG_PUNIT_WATCHER_OFFSET;
 		break;
 
 	case OOBMSM_0:
-		*index = 1;
+		*index = BMG_IDX_TELEM_OOBMSM;
 		if (cap_type == WATCHER)
-			*offset = OOBMSM_0_WATCHER_OFFSET;
+			*offset = BMG_OOBMSM_0_WATCHER_OFFSET;
 		break;
 
 	case OOBMSM_1:
-		*index = 1;
+		*index = BMG_IDX_TELEM_OOBMSM;
+		if (cap_type == TELEMETRY)
+			*offset = BMG_OOBMSM_1_TELEMETRY_OFFSET;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int cri_guid_decode(u32 guid, int *index, u32 *offset)
+{
+	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
+	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
+	u32 instance  = FIELD_GET(CRI_GUID_INSTANCE, guid);
+
+	*offset = 0;
+
+	if (cap_type == CRASHLOG) {
+		if (record_id == PUNIT) {
+			*index = CRI_IDX_CRASHLOG_PUNIT;
+			*offset = CRI_PUNIT_CRASHLOG_OFFSET;
+		} else {
+			*index = CRI_IDX_CRASHLOG_OOBMSM;
+		}
+		return 0;
+	}
+
+	switch (record_id) {
+	case PUNIT:
+		*index = CRI_IDX_TELEM_PUNIT;
 		if (cap_type == TELEMETRY)
-			*offset = OOBMSM_1_TELEMETRY_OFFSET;
+			*offset = CRI_PUNIT_TELEMETRY_OFFSET;
+		else
+			*offset = CRI_PUNIT_WATCHER_OFFSET;
+		break;
+
+	case OOBMSM_0:
+		*index = CRI_IDX_TELEM_OOBMSM;
+		switch (instance) {
+		case 0:
+			if (cap_type == WATCHER) {
+				*index = CRI_IDX_WATCHER_OOBMSM;
+				*offset = CRI_OOBMSM_WATCHER_OFFSET;
+			}
+			break;
+
+		case 1:
+			if (cap_type == TELEMETRY)
+				*offset = CRI_OOBMSM_GFSP_TELEMETRY_OFFSET;
+			break;
+
+		default:
+			return -EINVAL;
+		}
 		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -184,6 +255,23 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
 	return 0;
 }
 
+static int xe_guid_decode(u32 guid, int *index, u32 *offset)
+{
+	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
+	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
+
+	if (cap_type > WATCHER)
+		return -EINVAL;
+
+	if (device_id == BMG_DEVICE_ID)
+		return bmg_guid_decode(guid, index, offset);
+
+	if (device_id == CRI_DEVICE_ID)
+		return cri_guid_decode(guid, index, offset);
+
+	return -ENODEV;
+}
+
 /*
  * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
  * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
@@ -193,8 +281,8 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 		      u32 count)
 {
 	struct xe_device *xe = kdev_to_xe_device(dev);
-	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
 	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
+	void __iomem *telem_addr = xe->mmio.regs + xe->pmt.base_offset;
 	u32 mem_region;
 	u32 offset;
 	int ret = 0;
@@ -286,6 +374,7 @@ void xe_vsec_init(struct xe_device *xe)
 	case XE_VSEC_BMG:
 		if (!xe->soc_remapper.set_telem_region)
 			return;
+		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
 		info->priv_data = &xe_pmt_cb;
 		break;
 	default:
-- 
2.43.0

[PATCH v3 08/10] drm/xe/vsec: Crescent Island PMT callbacks

[Michael J. Ruhl]

CRI PMT support requires callbacks to access the discovery status
and control areas.  Access is a common MMIO area that requires an
index to be set before access is allowed.

Introduce the necessary callbacks to get the status and control
information for CRI PMT usage.

Add the glue logic to register the CRI PMT functionality.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_vsec.c | 118 ++++++++++++++++++++++++++++++++++-
 1 file changed, 115 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index dc42b9492428..578d59048b39 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -335,17 +335,120 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 	return ret == 0 ? count : ret;
 }
 
-static struct pmt_callbacks xe_pmt_cb = {
+/**
+ * xe_pmt_read_reg() - read a crashlog register
+ * @dev: the xe device that registered the callback
+ * @guid: PMT guid of the crashlog instance
+ * @reg: data read from the PMT data structure
+ * @offset: which data to read from the PMT data structure
+ *
+ * Read the requested PMT register based on the pcie device and guid.  The
+ * supported struct is the Crashlog Type1 Version2.
+ *
+ * Currently this is for CRI only.
+ */
+static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
+{
+	struct xe_device *xe = kdev_to_xe_device(dev);
+	void __iomem *disc_addr = xe->mmio.regs;
+	int ret = 0;
+	u32 inst;
+	int idx;
+
+	if (!drm_dev_enter(&xe->drm, &idx))
+		return -ENODEV;
+
+	if (!xe->soc_remapper.set_telem_region) {
+		ret = -ENODEV;
+		goto dev_exit;
+	}
+
+	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
+	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG) {
+		ret = -EINVAL;
+		goto dev_exit;
+	}
+
+	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
+		CRI_CRASHLOG_PUNIT_DISC_OFFSET : CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
+	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
+
+	xe_pm_runtime_get(xe);
+	mutex_lock(&xe->pmt.lock);
+
+	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+
+	memcpy_fromio(reg, disc_addr, sizeof(*reg));
+
+	mutex_unlock(&xe->pmt.lock);
+	xe_pm_runtime_put(xe);
+
+dev_exit:
+	drm_dev_exit(idx);
+
+	return ret;
+}
+
+static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
+{
+	struct xe_device *xe = kdev_to_xe_device(dev);
+	void __iomem *disc_addr = xe->mmio.regs;
+	int ret = 0;
+	u32 inst;
+	int idx;
+
+	if (!drm_dev_enter(&xe->drm, &idx))
+		return -ENODEV;
+
+	if (!xe->soc_remapper.set_telem_region) {
+		ret = -ENODEV;
+		goto dev_exit;
+	}
+
+	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
+	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG) {
+		ret = -EINVAL;
+		goto dev_exit;
+	}
+
+	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
+		CRI_CRASHLOG_PUNIT_DISC_OFFSET : CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
+	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
+
+	xe_pm_runtime_get(xe);
+	mutex_lock(&xe->pmt.lock);
+
+	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+
+	memcpy_toio(disc_addr, &reg, sizeof(reg));
+
+	mutex_unlock(&xe->pmt.lock);
+	xe_pm_runtime_put(xe);
+
+dev_exit:
+	drm_dev_exit(idx);
+
+	return ret;
+}
+
+static struct pmt_callbacks xe_bmg_pmt_cb = {
 	.read_telem = xe_pmt_telem_read,
 };
 
+static struct pmt_callbacks xe_cri_pmt_cb = {
+	.read_telem = xe_pmt_telem_read,
+	.read_reg = xe_pmt_read_reg,
+	.write_reg = xe_pmt_write_reg,
+};
+
 static const int vsec_platforms[] = {
 	[XE_BATTLEMAGE] = XE_VSEC_BMG,
+	[XE_CRESCENTISLAND] = XE_VSEC_CRI,
 };
 
 static enum xe_vsec get_platform_info(struct xe_device *xe)
 {
-	if (xe->info.platform > XE_BATTLEMAGE)
+	if (xe->info.platform > XE_CRESCENTISLAND)
 		return XE_VSEC_UNKNOWN;
 
 	return vsec_platforms[xe->info.platform];
@@ -375,8 +478,17 @@ void xe_vsec_init(struct xe_device *xe)
 		if (!xe->soc_remapper.set_telem_region)
 			return;
 		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
-		info->priv_data = &xe_pmt_cb;
+		info->priv_data = &xe_bmg_pmt_cb;
+		break;
+
+	case XE_VSEC_CRI:
+		if (!xe->soc_remapper.set_telem_region)
+			return;
+		xe->pmt.base_offset = CRI_TELEMETRY_OFFSET;
+		info->priv_data = &xe_cri_pmt_cb;
+		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
 		break;
+
 	default:
 		break;
 	}
-- 
2.43.0

[PATCH v3 09/10] drm/xe/vsec: Support late bind fw information

[Michael J. Ruhl]

CRI FW is loaded on power on.  Because of this, access to
the FW cannot be done until it is running.

Update the XE PMT probe and access to check for late bind
devices, verify, and wait for the appropriate FW state
before probe or access.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_device.c       |   4 +-
 drivers/gpu/drm/xe/xe_device_types.h |   4 +
 drivers/gpu/drm/xe/xe_vsec.c         | 141 +++++++++++++++++++++++++--
 drivers/gpu/drm/xe/xe_vsec.h         |   2 +-
 4 files changed, 143 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 74d566693dfd..bf02f881095f 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -1140,7 +1140,9 @@ int xe_device_probe(struct xe_device *xe)
 	for_each_gt(gt, xe, id)
 		xe_gt_sanitize_freq(gt);
 
-	xe_vsec_init(xe);
+	err = xe_vsec_init(xe);
+	if (err)
+		goto err_unregister_display;
 
 	err = xe_sriov_init_late(xe);
 	if (err)
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 3f1a70813a99..5d9e6e66c665 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -468,6 +468,10 @@ struct xe_device {
 		struct mutex lock;
 		/** @pmt.base_offset: device specific base offset */
 		u64 base_offset;
+		/** @pmt.work: support late-bind probe */
+		struct delayed_work work;
+		/** @pmt.retry_count: late-bind probe retry */
+		u32 retry_count;
 	} pmt;
 
 	/** @soc_remapper: SoC remapper object */
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 578d59048b39..edc20c24137e 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -3,6 +3,7 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/cleanup.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/intel_vsec.h>
 #include <linux/module.h>
@@ -17,6 +18,7 @@
 #include "xe_mmio.h"
 #include "xe_platform_types.h"
 #include "xe_pm.h"
+#include "xe_sysctrl.h"
 #include "xe_vsec.h"
 
 #include "regs/xe_pmt.h"
@@ -162,6 +164,14 @@ enum capability {
 	WATCHER,
 };
 
+/*
+ * Late bind will delay 100msec for up to 20 seconds
+ */
+#define VSEC_LATE_BIND_DELAY_MSEC	(100)
+#define VSEC_LATE_BIND_RETRY		(200)
+
+static void cri_late_bind_probe(struct xe_device *xe);
+
 static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
 {
 	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
@@ -272,6 +282,56 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
 	return -ENODEV;
 }
 
+#define WAITING_FOR_SYCTLR
+#ifdef WAITING_FOR_SYCTLR
+static bool xe_is_oobmsm_fw_ready(struct xe_device *xe)
+{
+	return true;
+}
+#endif
+
+static void cri_late_bind_probe_work(struct work_struct *work)
+{
+	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
+
+	if (xe_is_oobmsm_fw_ready(xe)) {
+		cri_late_bind_probe(xe);
+		xe_pm_runtime_put(xe);
+		return;
+	}
+
+	xe->pmt.retry_count++;
+
+	/* wait up to 20 seconds */
+	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
+		drm_warn(&xe->drm, "PMT probe: Late Binding failed to complete\n");
+		xe_pm_runtime_put(xe);
+		return;
+	}
+
+	if (!schedule_delayed_work(&xe->pmt.work, msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
+		xe_pm_runtime_put(xe);
+}
+
+static bool wait_for_fw(struct xe_device *xe)
+{
+	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
+
+	if (xe->info.platform != XE_CRESCENTISLAND)
+		return true;
+
+	while (retries--) {
+		if (xe_is_oobmsm_fw_ready(xe))
+			return true;
+
+		msleep(VSEC_LATE_BIND_DELAY_MSEC);
+	}
+
+	drm_warn(&xe->drm, "Late Binding failed to complete\n");
+
+	return false;
+}
+
 /*
  * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
  * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
@@ -318,6 +378,11 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 		goto dev_exit;
 	}
 
+	if (!wait_for_fw(xe)) {
+		ret = -ENODATA;
+		goto runtime_exit;
+	}
+
 	mutex_lock(&xe->pmt.lock);
 
 	/* set SoC re-mapper index register based on GUID memory region */
@@ -327,6 +392,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	mutex_unlock(&xe->pmt.lock);
 
+runtime_exit:
 	xe_pm_runtime_put(xe);
 
 dev_exit:
@@ -374,6 +440,10 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
 	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
 
 	xe_pm_runtime_get(xe);
+	if (!wait_for_fw(xe)) {
+		ret = -ENODATA;
+		goto runtime_exit;
+	}
 	mutex_lock(&xe->pmt.lock);
 
 	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
@@ -381,6 +451,8 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
 	memcpy_fromio(reg, disc_addr, sizeof(*reg));
 
 	mutex_unlock(&xe->pmt.lock);
+
+runtime_exit:
 	xe_pm_runtime_put(xe);
 
 dev_exit:
@@ -416,6 +488,10 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
 	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
 
 	xe_pm_runtime_get(xe);
+	if (!wait_for_fw(xe)) {
+		ret = -ENODATA;
+		goto runtime_exit;
+	}
 	mutex_lock(&xe->pmt.lock);
 
 	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
@@ -423,6 +499,8 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
 	memcpy_toio(disc_addr, &reg, sizeof(reg));
 
 	mutex_unlock(&xe->pmt.lock);
+
+runtime_exit:
 	xe_pm_runtime_put(xe);
 
 dev_exit:
@@ -454,12 +532,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
 	return vsec_platforms[xe->info.platform];
 }
 
+static void cri_late_bind_probe(struct xe_device *xe)
+{
+	struct intel_vsec_platform_info *info;
+	struct device *dev = xe->drm.dev;
+	enum xe_vsec platform;
+
+	platform = get_platform_info(xe);
+	if (platform != XE_VSEC_CRI)
+		return;
+
+	info = &xe_vsec_info[platform];
+	if (!info->headers)
+		return;
+
+	info->priv_data = &xe_cri_pmt_cb;
+	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+
+	intel_vsec_register(dev, info);
+}
+
+static void vsec_disable_late_bind_work(void *arg)
+{
+	struct xe_device *xe = arg;
+
+	/*
+	 * If was work was cancelled while it was still pending, we need to
+	 * take care of releasing the runtime reference
+	 */
+	if (disable_delayed_work_sync(&xe->pmt.work))
+		xe_pm_runtime_put(xe);
+}
+
 /**
  * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
  * interface
  * @xe: valid xe instance
  */
-void xe_vsec_init(struct xe_device *xe)
+int xe_vsec_init(struct xe_device *xe)
 {
 	struct intel_vsec_platform_info *info;
 	struct device *dev = xe->drm.dev;
@@ -467,30 +577,44 @@ void xe_vsec_init(struct xe_device *xe)
 
 	platform = get_platform_info(xe);
 	if (platform == XE_VSEC_UNKNOWN)
-		return;
+		return 0;
 
 	info = &xe_vsec_info[platform];
 	if (!info->headers)
-		return;
+		return 0;
 
 	switch (platform) {
 	case XE_VSEC_BMG:
 		if (!xe->soc_remapper.set_telem_region)
-			return;
+			return 0;
 		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
 		info->priv_data = &xe_bmg_pmt_cb;
 		break;
 
 	case XE_VSEC_CRI:
 		if (!xe->soc_remapper.set_telem_region)
-			return;
+			return 0;
 		xe->pmt.base_offset = CRI_TELEMETRY_OFFSET;
+
+		xe->pmt.retry_count = 0;
+		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
+
+		xe_pm_runtime_get_noresume(xe);
+		if (!xe_is_oobmsm_fw_ready(xe)) {
+			schedule_delayed_work(&xe->pmt.work,
+					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
+			return devm_add_action_or_reset(xe->drm.dev,
+							vsec_disable_late_bind_work,
+							xe);
+		}
+
 		info->priv_data = &xe_cri_pmt_cb;
 		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
 		break;
 
 	default:
-		break;
+		drm_err(&xe->drm, "Unsupported platform: %u\n", platform);
+		return 0;
 	}
 
 	/*
@@ -498,5 +622,10 @@ void xe_vsec_init(struct xe_device *xe)
 	 * resources.
 	 */
 	intel_vsec_register(dev, info);
+
+	if (platform == XE_VSEC_CRI)
+		xe_pm_runtime_put(xe);
+
+	return 0;
 }
 MODULE_IMPORT_NS("INTEL_VSEC");
diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
index a25b4e6e681b..c4a1e2fc67d8 100644
--- a/drivers/gpu/drm/xe/xe_vsec.h
+++ b/drivers/gpu/drm/xe/xe_vsec.h
@@ -9,7 +9,7 @@
 struct device;
 struct xe_device;
 
-void xe_vsec_init(struct xe_device *xe);
+int xe_vsec_init(struct xe_device *xe);
 int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
 
 #endif
-- 
2.43.0

[PATCH v3 10/10] drm/xe/vsec: Update PMT internal access for CRI

[Michael J. Ruhl]

Xe access the PMT infrastructure directly. The current usage
is supported ONLY by BMG devices.

CRI has further requirements for access.

Add a new API to access the GUID based on the platform.
Use the API get the GUID for each device.

Minor cleanup for a newly unused parameter (mmio).

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/regs/xe_pmt.h     |  4 ++-
 drivers/gpu/drm/xe/xe_debugfs.c      | 44 ++++++++++++-------------
 drivers/gpu/drm/xe/xe_device_types.h |  2 ++
 drivers/gpu/drm/xe/xe_hwmon.c        | 10 ++++--
 drivers/gpu/drm/xe/xe_pcode.c        | 10 ++++--
 drivers/gpu/drm/xe/xe_vsec.c         | 48 ++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_vsec.h         |  1 +
 7 files changed, 92 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
index fc9c9cb6a830..683bf401dc9d 100644
--- a/drivers/gpu/drm/xe/regs/xe_pmt.h
+++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
@@ -10,7 +10,7 @@
 #define BMG_PMT_BASE_OFFSET		0xDB000
 #define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
 
-#define PUNIT_TELEMETRY_GUID		XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
+#define BMG_PUNIT_TELEMETRY_GUID	XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
 #define BMG_ENERGY_STATUS_PMT_OFFSET	(0x30)
 #define	ENERGY_PKG			REG_GENMASK64(31, 0)
 #define	ENERGY_CARD			REG_GENMASK64(63, 32)
@@ -25,6 +25,8 @@
 #define CRI_DISCOVERY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
 #define CRI_TELEMETRY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
 
+#define CRI_PUNIT_TELEMETRY_GUID	XE_REG(CRI_DISCOVERY_OFFSET + 0x4)
+
 #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
 #define BMG_G2_RESIDENCY_OFFSET		(0x530)
 #define BMG_G6_RESIDENCY_OFFSET		(0x538)
diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
index 28135f84e286..12476549164d 100644
--- a/drivers/gpu/drm/xe/xe_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_debugfs.c
@@ -21,7 +21,6 @@
 #include "xe_gt_printk.h"
 #include "xe_guc_ads.h"
 #include "xe_hw_engine.h"
-#include "xe_mmio.h"
 #include "xe_pagefault.h"
 #include "xe_pcode.h"
 #include "xe_pm.h"
@@ -97,15 +96,20 @@ static void xe_fault_inject_debugfs_register(struct xe_device *xe,
 	}
 }
 
-static void read_residency_counter(struct xe_device *xe, struct xe_mmio *mmio,
-				   u32 offset, const char *name, struct drm_printer *p)
+static void read_residency_counter(struct xe_device *xe, u32 offset, const char *name,
+				   struct drm_printer *p)
 {
 	u64 residency = 0;
+	u32 guid;
 	int ret;
 
-	ret = xe_pmt_telem_read(xe->drm.dev,
-				xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
-				&residency, offset, sizeof(residency));
+	guid = xe_vsec_get_guid(xe);
+	if (!guid) {
+		drm_warn(&xe->drm, "PMT device is not powered\n");
+		return;
+	}
+
+	ret = xe_pmt_telem_read(xe->drm.dev, guid, &residency, offset, sizeof(residency));
 	if (ret != sizeof(residency)) {
 		drm_warn(&xe->drm, "%s counter failed to read, ret %d\n", name, ret);
 		return;
@@ -244,13 +248,12 @@ static int pcode_info(struct seq_file *m, void *data)
 static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
 {
 	struct xe_device *xe;
-	struct xe_mmio *mmio;
 	struct drm_printer p;
 
 	xe = node_to_xe(m->private);
 	p = drm_seq_file_printer(m);
 	guard(xe_pm_runtime)(xe);
-	mmio = xe_root_tile_mmio(xe);
+
 	static const struct {
 		u32 offset;
 		const char *name;
@@ -264,7 +267,7 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
 	};
 
 	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
-		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
+		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
 
 	return 0;
 }
@@ -272,13 +275,11 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
 static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
 {
 	struct xe_device *xe;
-	struct xe_mmio *mmio;
 	struct drm_printer p;
 
 	xe = node_to_xe(m->private);
 	p = drm_seq_file_printer(m);
 	guard(xe_pm_runtime)(xe);
-	mmio = xe_root_tile_mmio(xe);
 
 	static const struct {
 		u32 offset;
@@ -290,7 +291,7 @@ static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
 	};
 
 	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
-		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
+		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
 
 	return 0;
 }
@@ -710,23 +711,22 @@ void xe_debugfs_register(struct xe_device *xe)
 				 ARRAY_SIZE(debugfs_list),
 				 root, minor);
 
-	if (xe->info.platform == XE_BATTLEMAGE && !IS_SRIOV_VF(xe)) {
-		drm_debugfs_create_files(debugfs_residencies,
-					 ARRAY_SIZE(debugfs_residencies),
-					 root, minor);
-	}
-
 	/*
-	 * Pcode version read from PMT is currently only supported on CRI and BMG platforms in PF
-	 * mode, as both platforms support the necessary telemetry read mechanism and have a fixed
-	 * PUNIT_VERSION_OFFSET.
+	 * Residencies and Pcode version read from PMT is currently only supported on CRI and BMG
+	 * platforms in PF mode.  Both platforms support the necessary telemetry read mechanism
+	 * and have a fixed offsets for the required data.
 	 * Attempting this access on other platforms must be verified before enabling support.
 	 */
 	if (!IS_SRIOV_VF(xe) &&
-	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE))
+	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE)) {
+		drm_debugfs_create_files(debugfs_residencies,
+					 ARRAY_SIZE(debugfs_residencies),
+					 root, minor);
+
 		drm_debugfs_create_files(pcode_info_debugfs,
 					 ARRAY_SIZE(pcode_info_debugfs),
 					 root, minor);
+	}
 
 	debugfs_create_file("forcewake_all", 0400, root, xe,
 			    &forcewake_all_fops);
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 5d9e6e66c665..7acc57f6d4fd 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -472,6 +472,8 @@ struct xe_device {
 		struct delayed_work work;
 		/** @pmt.retry_count: late-bind probe retry */
 		u32 retry_count;
+		/** @pmt.punit_guid_cache: cache of the PUINT GUID */
+		u32 punit_guid_cache;
 	} pmt;
 
 	/** @soc_remapper: SoC remapper object */
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index 5284cab6703d..c4b44eb4b220 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -517,9 +517,15 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
 
 	if (hwmon->xe->info.platform == XE_BATTLEMAGE) {
 		u64 pmt_val;
+		u32 guid;
 
-		ret = xe_pmt_telem_read(hwmon->xe->drm.dev,
-					xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
+		guid = xe_vsec_get_guid(hwmon->xe);
+		if (!guid) {
+			drm_warn(&hwmon->xe->drm, "PMT device is not powered\n");
+			*energy = 0;
+			return;
+		}
+		ret = xe_pmt_telem_read(hwmon->xe->drm.dev, guid,
 					&pmt_val, BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
 		if (ret != sizeof(pmt_val)) {
 			drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index e1b8062541a9..7831dd597794 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -366,11 +366,17 @@ ALLOW_ERROR_INJECTION(xe_pcode_probe_early, ERRNO); /* See xe_pci_probe */
 int xe_get_pcode_version(struct xe_device *xe, struct xe_pcode_version *version)
 {
 	int ret = 0;
+	u32 guid;
 
 	guard(xe_pm_runtime)(xe);
 
-	ret = xe_pmt_telem_read(xe->drm.dev,
-				xe_mmio_read32(xe_root_tile_mmio(xe), PUNIT_TELEMETRY_GUID),
+	guid = xe_vsec_get_guid(xe);
+	if (!guid) {
+		xe_warn(xe, "PMT device is not powered\n");
+		return -ENODATA;
+	}
+
+	ret = xe_pmt_telem_read(xe->drm.dev, guid,
 				(u64 *)version, PUNIT_VERSION_OFFSET, sizeof(*version));
 	if (ret != sizeof(*version)) {
 		xe_warn(xe, "pcode version read from PMT failed, ret %pe\n", ERR_PTR(ret));
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index edc20c24137e..5ebb4ee9ef08 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -564,6 +564,54 @@ static void vsec_disable_late_bind_work(void *arg)
 		xe_pm_runtime_put(xe);
 }
 
+u32 xe_vsec_get_guid(struct xe_device *xe)
+{
+	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
+	u32 guid;
+
+	/*
+	 * Both supported platforms (BMG, CRI) require the remapper callback to
+	 * access data. CRI needs it for the GUID.
+	 */
+	if (!xe->soc_remapper.set_telem_region)
+		return 0;
+
+	/* caller must ensure correct power state */
+	if (!xe_pm_runtime_get_if_active(xe))
+		return 0;
+
+	mutex_lock(&xe->pmt.lock);
+
+	if (xe->pmt.punit_guid_cache) {
+		guid = xe->pmt.punit_guid_cache;
+		goto unlock;
+	}
+
+	switch (xe->info.platform) {
+	case XE_BATTLEMAGE:
+		guid = xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID);
+		break;
+
+	case XE_CRESCENTISLAND:
+		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+		guid = xe_mmio_read32(mmio, CRI_PUNIT_TELEMETRY_GUID);
+		break;
+
+	default:
+		guid = 0;
+		drm_err(&xe->drm, "Unsupported platform: %u\n", xe->info.platform);
+		break;
+	}
+
+	xe->pmt.punit_guid_cache = guid;
+
+unlock:
+	mutex_unlock(&xe->pmt.lock);
+	xe_pm_runtime_put(xe);
+
+	return guid;
+}
+
 /**
  * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
  * interface
diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
index c4a1e2fc67d8..43c3e9f227c5 100644
--- a/drivers/gpu/drm/xe/xe_vsec.h
+++ b/drivers/gpu/drm/xe/xe_vsec.h
@@ -10,6 +10,7 @@ struct device;
 struct xe_device;
 
 int xe_vsec_init(struct xe_device *xe);
+u32 xe_vsec_get_guid(struct xe_device *xe);
 int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
 
 #endif
-- 
2.43.0

Re: [PATCH v3 01/10] platform/x86/intel/pmt: complete pcidev to device update

[Rodrigo Vivi]

On Mon, Aug 24, 2026 at 09:23:17AM -0700, Michael J. Ruhl wrote:
> Previously the pcidev usage was moved to the struct device.
> The struct device is set for only the telemetry endpoints.
> 
> This usage prevents other PMT features (crashlog) from using
> the callback mechanism.
> 
> Use struct device in the intel_pmt_entry.
> Update callback usage to os the pmt entry rather than the
> telemetry endpoint.
> 
> Fixes: 353042d54d82 ("platform/x86/intel/vsec: Switch exported helpers from pci_dev to device")
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Dave, Hans, Ilpo, ack on getting this through the drm-xe-next tree?

Thanks,
Rodrigo.

> ---
>  drivers/platform/x86/intel/pmt/class.c     | 5 ++---
>  drivers/platform/x86/intel/pmt/class.h     | 3 +--
>  drivers/platform/x86/intel/pmt/discovery.c | 2 +-
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index d0ab8e33c62a..402d51df834a 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
> @@ -100,7 +100,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
>  	if (count > entry->size - off)
>  		count = entry->size - off;
>  
> -	count = pmt_telem_read_mmio(entry->ep->dev, entry->cb, entry->header.guid, buf,
> +	count = pmt_telem_read_mmio(entry->dev, entry->cb, entry->header.guid, buf,
>  				    entry->base, off, count);
>  
>  	return count;
> @@ -286,8 +286,6 @@ static int pmt_resolve_access_pci(struct intel_pmt_entry *entry,
>  		return -EINVAL;
>  	}
>  
> -	entry->pcidev = pci_dev;
> -
>  	return 0;
>  }
>  
> @@ -365,6 +363,7 @@ static int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
>  	entry->guid = header->guid;
>  	entry->size = header->size;
>  	entry->cb = ivdev->priv_data;
> +	entry->dev = ivdev->dev;
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
> index a0ece4fc3837..258cb460e61c 100644
> --- a/drivers/platform/x86/intel/pmt/class.h
> +++ b/drivers/platform/x86/intel/pmt/class.h
> @@ -20,7 +20,6 @@
>  #define GET_ADDRESS(v)		((v) & GENMASK(31, 3))
>  
>  struct device;
> -struct pci_dev;
>  extern struct class intel_pmt_class;
>  
>  struct telem_endpoint {
> @@ -42,7 +41,7 @@ struct intel_pmt_header {
>  
>  struct intel_pmt_entry {
>  	struct telem_endpoint	*ep;
> -	struct pci_dev		*pcidev;
> +	struct device		*dev;
>  	struct intel_pmt_header	header;
>  	u32			disc_header[PMT_DISC_DWORDS];
>  	struct bin_attribute	pmt_bin_attr;
> diff --git a/drivers/platform/x86/intel/pmt/discovery.c b/drivers/platform/x86/intel/pmt/discovery.c
> index c482368bfaae..f4203d240f54 100644
> --- a/drivers/platform/x86/intel/pmt/discovery.c
> +++ b/drivers/platform/x86/intel/pmt/discovery.c
> @@ -609,7 +609,7 @@ void intel_pmt_get_features(struct intel_pmt_entry *entry)
>  
>  	mutex_lock(&feature_list_lock);
>  	list_for_each_entry(feature, &pmt_feature_list, list) {
> -		if (feature->priv->parent != entry->ep->dev)
> +		if (feature->priv->parent != entry->dev)
>  			continue;
>  
>  		pmt_get_features(entry, feature);
> -- 
> 2.43.0
> 

Re: [PATCH v3 04/10] drm/xe/vsec: Use correct pm state get

[Rodrigo Vivi]

On Mon, Aug 24, 2026 at 09:23:20AM -0700, Michael J. Ruhl wrote:
> Crashlog needs to be collected at all times.  The current pm
> check assumes telemetry only.
> 
> Update read path to enable device for crashlog instances.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Fixes: 2c402a801c19 ("platform/x86/intel/pmt: support BMG crashlog")

I have the feeling that this patch should be split into 2.
1. with the lock fix and another one that would be the feature
scope increase for the crashlog and watcher, no?!

Or at least mention the mutex in the commit message?

> ---
>  drivers/gpu/drm/xe/xe_vsec.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index bd83a33aef6c..8d99a3770b5a 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -145,6 +145,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  {
>  	struct xe_device *xe = kdev_to_xe_device(dev);
>  	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
> +	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
>  	u32 mem_region;
>  	u32 offset;
>  	int ret;
> @@ -158,16 +159,28 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  
>  	telem_addr += offset + user_offset;
>  
> -	guard(mutex)(&xe->pmt.lock);
> +	/* Always allow crashlog. Telemetry, only when powered */
> +	switch (cap_type) {
> +	case CRASHLOG:
> +		xe_pm_runtime_get(xe);
> +		break;
> +	case TELEMETRY:
> +		if (!xe_pm_runtime_get_if_active(xe))
> +			return -ENODATA;
> +		break;
> +	case WATCHER:
> +		return -EINVAL;
> +	}
>  
> -	/* indicate that we are not at an appropriate power level */
> -	if (!xe_pm_runtime_get_if_active(xe))
> -		return -ENODATA;
> +	mutex_lock(&xe->pmt.lock);
>  
>  	/* set SoC re-mapper index register based on GUID memory region */
>  	xe->soc_remapper.set_telem_region(xe, mem_region);
>  
>  	memcpy_fromio(data, telem_addr, count);
> +
> +	mutex_unlock(&xe->pmt.lock);
> +
>  	xe_pm_runtime_put(xe);
>  
>  	return count;
> -- 
> 2.43.0
> 

Re: [PATCH v3 05/10] drm/xe/vsec: Support possible hotplug exit

[Rodrigo Vivi]

On Mon, Aug 24, 2026 at 09:23:21AM -0700, Michael J. Ruhl wrote:
> DRM has an API that will verify that a device is valid in
> the hotplug context.
> 
> Verify device is valid before access in the VSEC callback
> API.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_vsec.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 8d99a3770b5a..8abe11e6312f 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -10,6 +10,8 @@
>  #include <linux/pci.h>
>  #include <linux/types.h>
>  
> +#include <drm/drm_drv.h>
> +
>  #include "xe_device.h"
>  #include "xe_device_types.h"
>  #include "xe_mmio.h"
> @@ -140,6 +142,11 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return 0;
>  }
>  
> +/*
> + * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
> + * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
> + * verified (drm_dev_enter()).
> + */
>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset,
>  		      u32 count)
>  {
> @@ -148,14 +155,20 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
>  	u32 mem_region;
>  	u32 offset;
> -	int ret;
> +	int ret = 0;
> +	int idx;
>  
> -	if (!xe->soc_remapper.set_telem_region)
> +	if (!drm_dev_enter(&xe->drm, &idx))
>  		return -ENODEV;
>  
> +	if (!xe->soc_remapper.set_telem_region) {

a pre-existing component layer violation, but it would be good
to change this patch once you fix that in the other one,
creating the xe_soc_remapper function entry.

> +		ret = -EINVAL;

probably worth to mention the change of the return errno in the commit msg.

with these 2 changes:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +		goto dev_exit;
> +	}
> +
>  	ret = xe_guid_decode(guid, &mem_region, &offset);
>  	if (ret)
> -		return ret;
> +		goto dev_exit;
>  
>  	telem_addr += offset + user_offset;
>  
> @@ -165,11 +178,14 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  		xe_pm_runtime_get(xe);
>  		break;
>  	case TELEMETRY:
> -		if (!xe_pm_runtime_get_if_active(xe))
> -			return -ENODATA;
> +		if (!xe_pm_runtime_get_if_active(xe)) {
> +			ret = -ENODATA;
> +			goto dev_exit;
> +		}
>  		break;
>  	case WATCHER:
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto dev_exit;
>  	}
>  
>  	mutex_lock(&xe->pmt.lock);
> @@ -183,7 +199,10 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  
>  	xe_pm_runtime_put(xe);
>  
> -	return count;
> +dev_exit:
> +	drm_dev_exit(idx);
> +
> +	return ret == 0 ? count : ret;
>  }
>  
>  static struct pmt_callbacks xe_pmt_cb = {
> -- 
> 2.43.0
> 

Re: [PATCH v3 06/10] drm/xe/vsec: Support Crescent Island PMT

[Rodrigo Vivi]

On Mon, Aug 24, 2026 at 09:23:22AM -0700, Michael J. Ruhl wrote:
> Crescent Island (CRI) supports PMT telemetry and crashlog.
> 
> Add Crescent Island (CRI) discovery structure (DVSEC)
> information to allow for Xe registration.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_pmt.h |  5 ++++
>  drivers/gpu/drm/xe/xe_vsec.c     | 46 ++++++++++++++++++++++++++++++--
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
> index a62ab05c6b4c..fc9c9cb6a830 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pmt.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
> @@ -20,6 +20,11 @@
>  #define BMG_TELEMETRY_BASE_OFFSET	0xE0000
>  #define BMG_TELEMETRY_OFFSET		(SOC_BASE + BMG_TELEMETRY_BASE_OFFSET)
>  
> +#define CRI_TELEMETRY_BASE_OFFSET	0xE0000

If it is the same as BMG, let's just reuse that.
The platform prefix means the platform where that started or that got
changed. We don't need to have one define per platform.

And actually in the XE we even try to use XE_ as prefix whenever it is a
unique value with the platform.

> +/* for CRI discovery and telemetry are in an indexed window */
> +#define CRI_DISCOVERY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
> +#define CRI_TELEMETRY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)

same for this right?! probably not needed...

> +
>  #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
>  #define BMG_G2_RESIDENCY_OFFSET		(0x530)
>  #define BMG_G6_RESIDENCY_OFFSET		(0x538)
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 8abe11e6312f..6345b0b4b26b 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -21,8 +21,19 @@
>  
>  #include "regs/xe_pmt.h"
>  
> -/* PMT GUID value for BMG devices.  NOTE: this is NOT a PCI id */
> +/* PMT GUID value for BMG and CRI devices.  NOTE: this is NOT a PCI id */
>  #define BMG_DEVICE_ID 0xE2F8
> +#define CRI_DEVICE_ID 0xE2FA
> +
> +/*
> + * sizeof(Crashlog Type1 Version2) = 0x18 (24) bytes
> + * For BMG and CRI PUNIT and OOBMSMS crashlogs are consecutive.
> + */
> +#define BMG_CRASHLOG_PUNIT_DISC_OFFSET (0x60)
> +#define BMG_CRASHLOG_OOBMSM_DISC_OFFSET (BMG_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
> +
> +#define CRI_CRASHLOG_PUNIT_DISC_OFFSET (0x80)
> +#define CRI_CRASHLOG_OOBMSM_DISC_OFFSET (CRI_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
>  
>  static struct intel_vsec_header bmg_telemetry = {
>  	.rev = 1,
> @@ -41,7 +52,7 @@ static struct intel_vsec_header bmg_crashlog = {
>  	.num_entries = 2,
>  	.entry_size = 6,
>  	.tbir = 0,
> -	.offset = BMG_DISCOVERY_OFFSET + 0x60,
> +	.offset = BMG_DISCOVERY_OFFSET + BMG_CRASHLOG_PUNIT_DISC_OFFSET,
>  };
>  
>  static struct intel_vsec_header *bmg_capabilities[] = {
> @@ -50,9 +61,36 @@ static struct intel_vsec_header *bmg_capabilities[] = {
>  	NULL
>  };
>  
> +static struct intel_vsec_header cri_telemetry = {
> +	.rev = 1,
> +	.length = 0x10,
> +	.id = VSEC_ID_TELEMETRY,
> +	.num_entries = 3,
> +	.entry_size = 4,
> +	.tbir = 0,
> +	.offset = CRI_DISCOVERY_OFFSET,
> +};
> +
> +static struct intel_vsec_header cri_crashlog = {
> +	.rev = 1,
> +	.length = 0x10,
> +	.id = VSEC_ID_CRASHLOG,
> +	.num_entries = 2,
> +	.entry_size = 6,
> +	.tbir = 0,
> +	.offset = CRI_DISCOVERY_OFFSET + CRI_CRASHLOG_PUNIT_DISC_OFFSET,
> +};
> +
> +static struct intel_vsec_header *cri_capabilities[] = {
> +	&cri_telemetry,
> +	&cri_crashlog,
> +	NULL
> +};
> +
>  enum xe_vsec {
>  	XE_VSEC_UNKNOWN = 0,
>  	XE_VSEC_BMG,
> +	XE_VSEC_CRI,
>  };
>  
>  static struct intel_vsec_platform_info xe_vsec_info[] = {
> @@ -60,6 +98,10 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
>  		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
>  		.headers = bmg_capabilities,
>  	},
> +	[XE_VSEC_CRI] = {
> +		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
> +		.headers = cri_capabilities,
> +	},
>  	{ }
>  };
>  
> -- 
> 2.43.0
> 

Re: [PATCH v3 09/10] drm/xe/vsec: Support late bind fw information

[Rodrigo Vivi]

On Mon, Aug 24, 2026 at 09:23:25AM -0700, Michael J. Ruhl wrote:
> CRI FW is loaded on power on.  Because of this, access to
> the FW cannot be done until it is running.
> 
> Update the XE PMT probe and access to check for late bind
> devices, verify, and wait for the appropriate FW state
> before probe or access.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c       |   4 +-
>  drivers/gpu/drm/xe/xe_device_types.h |   4 +
>  drivers/gpu/drm/xe/xe_vsec.c         | 141 +++++++++++++++++++++++++--
>  drivers/gpu/drm/xe/xe_vsec.h         |   2 +-
>  4 files changed, 143 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 74d566693dfd..bf02f881095f 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -1140,7 +1140,9 @@ int xe_device_probe(struct xe_device *xe)
>  	for_each_gt(gt, xe, id)
>  		xe_gt_sanitize_freq(gt);
>  
> -	xe_vsec_init(xe);
> +	err = xe_vsec_init(xe);
> +	if (err)
> +		goto err_unregister_display;
>  
>  	err = xe_sriov_init_late(xe);
>  	if (err)
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 3f1a70813a99..5d9e6e66c665 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -468,6 +468,10 @@ struct xe_device {
>  		struct mutex lock;
>  		/** @pmt.base_offset: device specific base offset */
>  		u64 base_offset;
> +		/** @pmt.work: support late-bind probe */
> +		struct delayed_work work;
> +		/** @pmt.retry_count: late-bind probe retry */
> +		u32 retry_count;
>  	} pmt;
>  
>  	/** @soc_remapper: SoC remapper object */
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 578d59048b39..edc20c24137e 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -3,6 +3,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/cleanup.h>
> +#include <linux/delay.h>
>  #include <linux/errno.h>
>  #include <linux/intel_vsec.h>
>  #include <linux/module.h>
> @@ -17,6 +18,7 @@
>  #include "xe_mmio.h"
>  #include "xe_platform_types.h"
>  #include "xe_pm.h"
> +#include "xe_sysctrl.h"
>  #include "xe_vsec.h"
>  
>  #include "regs/xe_pmt.h"
> @@ -162,6 +164,14 @@ enum capability {
>  	WATCHER,
>  };
>  
> +/*
> + * Late bind will delay 100msec for up to 20 seconds
> + */
> +#define VSEC_LATE_BIND_DELAY_MSEC	(100)
> +#define VSEC_LATE_BIND_RETRY		(200)
> +
> +static void cri_late_bind_probe(struct xe_device *xe);
> +
>  static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
>  {
>  	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
> @@ -272,6 +282,56 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return -ENODEV;
>  }
>  
> +#define WAITING_FOR_SYCTLR
> +#ifdef WAITING_FOR_SYCTLR

I'm afraid you forgot to remove this before sending... or what's the goal of these?

> +static bool xe_is_oobmsm_fw_ready(struct xe_device *xe)
> +{
> +	return true;
> +}
> +#endif
> +
> +static void cri_late_bind_probe_work(struct work_struct *work)
> +{
> +	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
> +
> +	if (xe_is_oobmsm_fw_ready(xe)) {
> +		cri_late_bind_probe(xe);
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +
> +	xe->pmt.retry_count++;
> +
> +	/* wait up to 20 seconds */
> +	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
> +		drm_warn(&xe->drm, "PMT probe: Late Binding failed to complete\n");
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +
> +	if (!schedule_delayed_work(&xe->pmt.work, msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
> +		xe_pm_runtime_put(xe);
> +}
> +
> +static bool wait_for_fw(struct xe_device *xe)
> +{
> +	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
> +
> +	if (xe->info.platform != XE_CRESCENTISLAND)
> +		return true;
> +
> +	while (retries--) {
> +		if (xe_is_oobmsm_fw_ready(xe))
> +			return true;
> +
> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);
> +	}
> +
> +	drm_warn(&xe->drm, "Late Binding failed to complete\n");
> +
> +	return false;
> +}
> +
>  /*
>   * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
>   * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
> @@ -318,6 +378,11 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  		goto dev_exit;
>  	}
>  
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
> +
>  	mutex_lock(&xe->pmt.lock);
>  
>  	/* set SoC re-mapper index register based on GUID memory region */
> @@ -327,6 +392,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  
>  	mutex_unlock(&xe->pmt.lock);
>  
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:
> @@ -374,6 +440,10 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
>  	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>  
>  	xe_pm_runtime_get(xe);
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
>  	mutex_lock(&xe->pmt.lock);
>  
>  	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> @@ -381,6 +451,8 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
>  	memcpy_fromio(reg, disc_addr, sizeof(*reg));
>  
>  	mutex_unlock(&xe->pmt.lock);
> +
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:
> @@ -416,6 +488,10 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
>  	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>  
>  	xe_pm_runtime_get(xe);
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
>  	mutex_lock(&xe->pmt.lock);
>  
>  	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> @@ -423,6 +499,8 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
>  	memcpy_toio(disc_addr, &reg, sizeof(reg));
>  
>  	mutex_unlock(&xe->pmt.lock);
> +
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:
> @@ -454,12 +532,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
>  	return vsec_platforms[xe->info.platform];
>  }
>  
> +static void cri_late_bind_probe(struct xe_device *xe)
> +{
> +	struct intel_vsec_platform_info *info;
> +	struct device *dev = xe->drm.dev;
> +	enum xe_vsec platform;
> +
> +	platform = get_platform_info(xe);
> +	if (platform != XE_VSEC_CRI)
> +		return;
> +
> +	info = &xe_vsec_info[platform];
> +	if (!info->headers)
> +		return;
> +
> +	info->priv_data = &xe_cri_pmt_cb;
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	intel_vsec_register(dev, info);
> +}
> +
> +static void vsec_disable_late_bind_work(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	/*
> +	 * If was work was cancelled while it was still pending, we need to
> +	 * take care of releasing the runtime reference
> +	 */
> +	if (disable_delayed_work_sync(&xe->pmt.work))
> +		xe_pm_runtime_put(xe);
> +}
> +
>  /**
>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>   * interface
>   * @xe: valid xe instance
>   */
> -void xe_vsec_init(struct xe_device *xe)
> +int xe_vsec_init(struct xe_device *xe)
>  {
>  	struct intel_vsec_platform_info *info;
>  	struct device *dev = xe->drm.dev;
> @@ -467,30 +577,44 @@ void xe_vsec_init(struct xe_device *xe)
>  
>  	platform = get_platform_info(xe);
>  	if (platform == XE_VSEC_UNKNOWN)
> -		return;
> +		return 0;
>  
>  	info = &xe_vsec_info[platform];
>  	if (!info->headers)
> -		return;
> +		return 0;
>  
>  	switch (platform) {
>  	case XE_VSEC_BMG:
>  		if (!xe->soc_remapper.set_telem_region)
> -			return;
> +			return 0;
>  		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
>  		info->priv_data = &xe_bmg_pmt_cb;
>  		break;
>  
>  	case XE_VSEC_CRI:
>  		if (!xe->soc_remapper.set_telem_region)
> -			return;
> +			return 0;
>  		xe->pmt.base_offset = CRI_TELEMETRY_OFFSET;
> +
> +		xe->pmt.retry_count = 0;
> +		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
> +
> +		xe_pm_runtime_get_noresume(xe);
> +		if (!xe_is_oobmsm_fw_ready(xe)) {
> +			schedule_delayed_work(&xe->pmt.work,
> +					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
> +			return devm_add_action_or_reset(xe->drm.dev,
> +							vsec_disable_late_bind_work,
> +							xe);
> +		}
> +
>  		info->priv_data = &xe_cri_pmt_cb;
>  		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>  		break;
>  
>  	default:
> -		break;
> +		drm_err(&xe->drm, "Unsupported platform: %u\n", platform);
> +		return 0;
>  	}
>  
>  	/*
> @@ -498,5 +622,10 @@ void xe_vsec_init(struct xe_device *xe)
>  	 * resources.
>  	 */
>  	intel_vsec_register(dev, info);
> +
> +	if (platform == XE_VSEC_CRI)
> +		xe_pm_runtime_put(xe);
> +
> +	return 0;
>  }
>  MODULE_IMPORT_NS("INTEL_VSEC");
> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
> index a25b4e6e681b..c4a1e2fc67d8 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.h
> +++ b/drivers/gpu/drm/xe/xe_vsec.h
> @@ -9,7 +9,7 @@
>  struct device;
>  struct xe_device;
>  
> -void xe_vsec_init(struct xe_device *xe);
> +int xe_vsec_init(struct xe_device *xe);
>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
>  
>  #endif
> -- 
> 2.43.0
> 

Re: [PATCH v3 10/10] drm/xe/vsec: Update PMT internal access for CRI

[Rodrigo Vivi]

On Mon, Aug 24, 2026 at 09:23:26AM -0700, Michael J. Ruhl wrote:
> Xe access the PMT infrastructure directly. The current usage
> is supported ONLY by BMG devices.
> 
> CRI has further requirements for access.
> 
> Add a new API to access the GUID based on the platform.
> Use the API get the GUID for each device.

I have the feeling that this patch is doing much more then advertised,
like the debugfs residencies and all... could you please split the patch
or improve the commit message?

Thanks,
Rodrigo.

> 
> Minor cleanup for a newly unused parameter (mmio).
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_pmt.h     |  4 ++-
>  drivers/gpu/drm/xe/xe_debugfs.c      | 44 ++++++++++++-------------
>  drivers/gpu/drm/xe/xe_device_types.h |  2 ++
>  drivers/gpu/drm/xe/xe_hwmon.c        | 10 ++++--
>  drivers/gpu/drm/xe/xe_pcode.c        | 10 ++++--
>  drivers/gpu/drm/xe/xe_vsec.c         | 48 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_vsec.h         |  1 +
>  7 files changed, 92 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
> index fc9c9cb6a830..683bf401dc9d 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pmt.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
> @@ -10,7 +10,7 @@
>  #define BMG_PMT_BASE_OFFSET		0xDB000
>  #define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
>  
> -#define PUNIT_TELEMETRY_GUID		XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
> +#define BMG_PUNIT_TELEMETRY_GUID	XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
>  #define BMG_ENERGY_STATUS_PMT_OFFSET	(0x30)
>  #define	ENERGY_PKG			REG_GENMASK64(31, 0)
>  #define	ENERGY_CARD			REG_GENMASK64(63, 32)
> @@ -25,6 +25,8 @@
>  #define CRI_DISCOVERY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
>  #define CRI_TELEMETRY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
>  
> +#define CRI_PUNIT_TELEMETRY_GUID	XE_REG(CRI_DISCOVERY_OFFSET + 0x4)
> +
>  #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
>  #define BMG_G2_RESIDENCY_OFFSET		(0x530)
>  #define BMG_G6_RESIDENCY_OFFSET		(0x538)
> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> index 28135f84e286..12476549164d 100644
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> @@ -21,7 +21,6 @@
>  #include "xe_gt_printk.h"
>  #include "xe_guc_ads.h"
>  #include "xe_hw_engine.h"
> -#include "xe_mmio.h"
>  #include "xe_pagefault.h"
>  #include "xe_pcode.h"
>  #include "xe_pm.h"
> @@ -97,15 +96,20 @@ static void xe_fault_inject_debugfs_register(struct xe_device *xe,
>  	}
>  }
>  
> -static void read_residency_counter(struct xe_device *xe, struct xe_mmio *mmio,
> -				   u32 offset, const char *name, struct drm_printer *p)
> +static void read_residency_counter(struct xe_device *xe, u32 offset, const char *name,
> +				   struct drm_printer *p)
>  {
>  	u64 residency = 0;
> +	u32 guid;
>  	int ret;
>  
> -	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
> -				&residency, offset, sizeof(residency));
> +	guid = xe_vsec_get_guid(xe);
> +	if (!guid) {
> +		drm_warn(&xe->drm, "PMT device is not powered\n");
> +		return;
> +	}
> +
> +	ret = xe_pmt_telem_read(xe->drm.dev, guid, &residency, offset, sizeof(residency));
>  	if (ret != sizeof(residency)) {
>  		drm_warn(&xe->drm, "%s counter failed to read, ret %d\n", name, ret);
>  		return;
> @@ -244,13 +248,12 @@ static int pcode_info(struct seq_file *m, void *data)
>  static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
>  {
>  	struct xe_device *xe;
> -	struct xe_mmio *mmio;
>  	struct drm_printer p;
>  
>  	xe = node_to_xe(m->private);
>  	p = drm_seq_file_printer(m);
>  	guard(xe_pm_runtime)(xe);
> -	mmio = xe_root_tile_mmio(xe);
> +
>  	static const struct {
>  		u32 offset;
>  		const char *name;
> @@ -264,7 +267,7 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
>  	};
>  
>  	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
> -		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
> +		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
>  
>  	return 0;
>  }
> @@ -272,13 +275,11 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
>  static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
>  {
>  	struct xe_device *xe;
> -	struct xe_mmio *mmio;
>  	struct drm_printer p;
>  
>  	xe = node_to_xe(m->private);
>  	p = drm_seq_file_printer(m);
>  	guard(xe_pm_runtime)(xe);
> -	mmio = xe_root_tile_mmio(xe);
>  
>  	static const struct {
>  		u32 offset;
> @@ -290,7 +291,7 @@ static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
>  	};
>  
>  	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
> -		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
> +		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
>  
>  	return 0;
>  }
> @@ -710,23 +711,22 @@ void xe_debugfs_register(struct xe_device *xe)
>  				 ARRAY_SIZE(debugfs_list),
>  				 root, minor);
>  
> -	if (xe->info.platform == XE_BATTLEMAGE && !IS_SRIOV_VF(xe)) {
> -		drm_debugfs_create_files(debugfs_residencies,
> -					 ARRAY_SIZE(debugfs_residencies),
> -					 root, minor);
> -	}
> -
>  	/*
> -	 * Pcode version read from PMT is currently only supported on CRI and BMG platforms in PF
> -	 * mode, as both platforms support the necessary telemetry read mechanism and have a fixed
> -	 * PUNIT_VERSION_OFFSET.
> +	 * Residencies and Pcode version read from PMT is currently only supported on CRI and BMG
> +	 * platforms in PF mode.  Both platforms support the necessary telemetry read mechanism
> +	 * and have a fixed offsets for the required data.
>  	 * Attempting this access on other platforms must be verified before enabling support.
>  	 */
>  	if (!IS_SRIOV_VF(xe) &&
> -	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE))
> +	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE)) {
> +		drm_debugfs_create_files(debugfs_residencies,
> +					 ARRAY_SIZE(debugfs_residencies),
> +					 root, minor);
> +
>  		drm_debugfs_create_files(pcode_info_debugfs,
>  					 ARRAY_SIZE(pcode_info_debugfs),
>  					 root, minor);
> +	}
>  
>  	debugfs_create_file("forcewake_all", 0400, root, xe,
>  			    &forcewake_all_fops);
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 5d9e6e66c665..7acc57f6d4fd 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -472,6 +472,8 @@ struct xe_device {
>  		struct delayed_work work;
>  		/** @pmt.retry_count: late-bind probe retry */
>  		u32 retry_count;
> +		/** @pmt.punit_guid_cache: cache of the PUINT GUID */
> +		u32 punit_guid_cache;
>  	} pmt;
>  
>  	/** @soc_remapper: SoC remapper object */
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5284cab6703d..c4b44eb4b220 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -517,9 +517,15 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
>  
>  	if (hwmon->xe->info.platform == XE_BATTLEMAGE) {
>  		u64 pmt_val;
> +		u32 guid;
>  
> -		ret = xe_pmt_telem_read(hwmon->xe->drm.dev,
> -					xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
> +		guid = xe_vsec_get_guid(hwmon->xe);
> +		if (!guid) {
> +			drm_warn(&hwmon->xe->drm, "PMT device is not powered\n");
> +			*energy = 0;
> +			return;
> +		}
> +		ret = xe_pmt_telem_read(hwmon->xe->drm.dev, guid,
>  					&pmt_val, BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
>  		if (ret != sizeof(pmt_val)) {
>  			drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index e1b8062541a9..7831dd597794 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -366,11 +366,17 @@ ALLOW_ERROR_INJECTION(xe_pcode_probe_early, ERRNO); /* See xe_pci_probe */
>  int xe_get_pcode_version(struct xe_device *xe, struct xe_pcode_version *version)
>  {
>  	int ret = 0;
> +	u32 guid;
>  
>  	guard(xe_pm_runtime)(xe);
>  
> -	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(xe_root_tile_mmio(xe), PUNIT_TELEMETRY_GUID),
> +	guid = xe_vsec_get_guid(xe);
> +	if (!guid) {
> +		xe_warn(xe, "PMT device is not powered\n");
> +		return -ENODATA;
> +	}
> +
> +	ret = xe_pmt_telem_read(xe->drm.dev, guid,
>  				(u64 *)version, PUNIT_VERSION_OFFSET, sizeof(*version));
>  	if (ret != sizeof(*version)) {
>  		xe_warn(xe, "pcode version read from PMT failed, ret %pe\n", ERR_PTR(ret));
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index edc20c24137e..5ebb4ee9ef08 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -564,6 +564,54 @@ static void vsec_disable_late_bind_work(void *arg)
>  		xe_pm_runtime_put(xe);
>  }
>  
> +u32 xe_vsec_get_guid(struct xe_device *xe)
> +{
> +	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> +	u32 guid;
> +
> +	/*
> +	 * Both supported platforms (BMG, CRI) require the remapper callback to
> +	 * access data. CRI needs it for the GUID.
> +	 */
> +	if (!xe->soc_remapper.set_telem_region)
> +		return 0;
> +
> +	/* caller must ensure correct power state */
> +	if (!xe_pm_runtime_get_if_active(xe))
> +		return 0;
> +
> +	mutex_lock(&xe->pmt.lock);
> +
> +	if (xe->pmt.punit_guid_cache) {
> +		guid = xe->pmt.punit_guid_cache;
> +		goto unlock;
> +	}
> +
> +	switch (xe->info.platform) {
> +	case XE_BATTLEMAGE:
> +		guid = xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	case XE_CRESCENTISLAND:
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +		guid = xe_mmio_read32(mmio, CRI_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	default:
> +		guid = 0;
> +		drm_err(&xe->drm, "Unsupported platform: %u\n", xe->info.platform);
> +		break;
> +	}
> +
> +	xe->pmt.punit_guid_cache = guid;
> +
> +unlock:
> +	mutex_unlock(&xe->pmt.lock);
> +	xe_pm_runtime_put(xe);
> +
> +	return guid;
> +}
> +
>  /**
>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>   * interface
> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
> index c4a1e2fc67d8..43c3e9f227c5 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.h
> +++ b/drivers/gpu/drm/xe/xe_vsec.h
> @@ -10,6 +10,7 @@ struct device;
>  struct xe_device;
>  
>  int xe_vsec_init(struct xe_device *xe);
> +u32 xe_vsec_get_guid(struct xe_device *xe);
>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
>  
>  #endif
> -- 
> 2.43.0
> 

Re: [PATCH v3 01/10] platform/x86/intel/pmt: complete pcidev to device update

[Ilpo Järvinen]

On Mon, 24 Aug 2026, Michael J. Ruhl wrote:

> Previously the pcidev usage was moved to the struct device.
> The struct device is set for only the telemetry endpoints.

I fail to understand any of this (1st sentence has even broken grammar 
afaict, and we don't set struct x but variables pointing to them). Please 
rephrase.

> This usage prevents other PMT features (crashlog) from using

What "this usage". Perhaps try to avoid using "this" as it often results 
in very vague references.

> the callback mechanism.
> 
> Use struct device in the intel_pmt_entry.

Don't leave lines short like this in middle of a paragraph.

> Update callback usage to os the pmt entry rather than the
> telemetry endpoint.
>
> Fixes: 353042d54d82 ("platform/x86/intel/vsec: Switch exported helpers from pci_dev to device")

With no changelog understandable for mere mortals like me, my assumptions 
is 353042d54d82 was incomplete, is that what you tried to say?

> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/class.c     | 5 ++---
>  drivers/platform/x86/intel/pmt/class.h     | 3 +--
>  drivers/platform/x86/intel/pmt/discovery.c | 2 +-
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index d0ab8e33c62a..402d51df834a 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
> @@ -100,7 +100,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
>  	if (count > entry->size - off)
>  		count = entry->size - off;
>  
> -	count = pmt_telem_read_mmio(entry->ep->dev, entry->cb, entry->header.guid, buf,
> +	count = pmt_telem_read_mmio(entry->dev, entry->cb, entry->header.guid, buf,
>  				    entry->base, off, count);
>  
>  	return count;
> @@ -286,8 +286,6 @@ static int pmt_resolve_access_pci(struct intel_pmt_entry *entry,
>  		return -EINVAL;
>  	}
>  
> -	entry->pcidev = pci_dev;
> -
>  	return 0;
>  }
>  
> @@ -365,6 +363,7 @@ static int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
>  	entry->guid = header->guid;
>  	entry->size = header->size;
>  	entry->cb = ivdev->priv_data;
> +	entry->dev = ivdev->dev;
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
> index a0ece4fc3837..258cb460e61c 100644
> --- a/drivers/platform/x86/intel/pmt/class.h
> +++ b/drivers/platform/x86/intel/pmt/class.h
> @@ -20,7 +20,6 @@
>  #define GET_ADDRESS(v)		((v) & GENMASK(31, 3))
>  
>  struct device;
> -struct pci_dev;
>  extern struct class intel_pmt_class;
>  
>  struct telem_endpoint {
> @@ -42,7 +41,7 @@ struct intel_pmt_header {
>  
>  struct intel_pmt_entry {
>  	struct telem_endpoint	*ep;
> -	struct pci_dev		*pcidev;
> +	struct device		*dev;
>  	struct intel_pmt_header	header;
>  	u32			disc_header[PMT_DISC_DWORDS];
>  	struct bin_attribute	pmt_bin_attr;
> diff --git a/drivers/platform/x86/intel/pmt/discovery.c b/drivers/platform/x86/intel/pmt/discovery.c
> index c482368bfaae..f4203d240f54 100644
> --- a/drivers/platform/x86/intel/pmt/discovery.c
> +++ b/drivers/platform/x86/intel/pmt/discovery.c
> @@ -609,7 +609,7 @@ void intel_pmt_get_features(struct intel_pmt_entry *entry)
>  
>  	mutex_lock(&feature_list_lock);
>  	list_for_each_entry(feature, &pmt_feature_list, list) {
> -		if (feature->priv->parent != entry->ep->dev)
> +		if (feature->priv->parent != entry->dev)
>  			continue;
>  
>  		pmt_get_features(entry, feature);
> 

-- 
 i.

Re: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access callbacks

[Ilpo Järvinen]

On Mon, 24 Aug 2026, Michael J. Ruhl wrote:

> Some HW does not have direct MMIO access to PMT control and data
> features.
> 
> Augment the current callback infrastructure (data access) to allow
> a registered driver to customize read/write access to the control
> paths for PMT usage.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 39 +++++++++++++++++++++--
>  include/linux/intel_vsec.h                | 14 +++++++-
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index f936daf99e4d..5923ad7abbd9 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -129,7 +129,19 @@ static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  {
>  	const struct crashlog_control *control = &crashlog->info->control;
>  	struct intel_pmt_entry *entry = &crashlog->entry;
> -	u32 reg = readl(entry->disc_table + control->offset);
> +	u32 guid = entry->header.guid;
> +	u32 reg;
> +	int err;
> +
> +	if (entry->cb && entry->cb->read_reg) {
> +		err = entry->cb->read_reg(entry->dev, guid, &reg, control->offset);
> +		if (err) {
> +			pr_err("%s: failed to read reg: %d\n", __func__, err);

Never print __func__ in any user consumable message (level > debug) but 
write the message in plain English.

Also, this needs include.

> +			return;
> +		}
> +	} else {
> +		reg = readl(entry->disc_table + control->offset);

Add include.

> +	}
>  
>  	reg &= ~control->trigger_mask;
>  
> @@ -138,14 +150,35 @@ static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  	else
>  		reg &= ~bit;
>  
> -	writel(reg, entry->disc_table + control->offset);
> +	if (entry->cb && entry->cb->write_reg) {
> +		err = entry->cb->write_reg(entry->dev, guid, reg, control->offset);
> +		if (err) {
> +			pr_err("%s: failed to write reg: %d\n", __func__, err);

Rephrase without using __func__.

> +			return;
> +		}
> +	} else {
> +		writel(reg, entry->disc_table + control->offset);
> +	}
>  }
>  
>  /* Read the status register and see if the specified @bit is set */
>  static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
>  {
>  	const struct crashlog_status *status = &crashlog->info->status;
> -	u32 reg = readl(crashlog->entry.disc_table + status->offset);
> +	struct intel_pmt_entry *entry = &crashlog->entry;
> +	u32 guid = entry->header.guid;
> +	u32 reg;
> +	int err;
> +
> +	if (entry->cb && entry->cb->read_reg) {
> +		err = entry->cb->read_reg(entry->dev, guid, &reg, status->offset);
> +		if (err) {
> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
> +			return false;

It seems you need a lerger rework here to properly return error codes.

> +		}
> +	} else {
> +		reg = readl(crashlog->entry.disc_table + status->offset);

entry->disc_table

> +	}

This looks mostly duplicated code, add a helper?

>  
>  	return !!(reg & bit);
>  }
> diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
> index 843cda8f8644..917d9397a993 100644
> --- a/include/linux/intel_vsec.h
> +++ b/include/linux/intel_vsec.h
> @@ -90,13 +90,25 @@ enum intel_vsec_quirks {
>   * @read_telem: when specified, called by client driver to access PMT
>   * data (instead of direct copy).
>   * * dev:   device reference for the callback's use
> - * * guid:  ID of data to acccss
> + * * guid:  ID of data to access
>   * * data:  buffer for the data to be copied
>   * * off:   offset into the requested buffer
>   * * count: size of buffer
> + * @read_reg: when specified called by client driver to read PMT state
> + * * dev:      device reference for the callback's use
> + * * guid:     ID of data to access
> + * * reg_data: register data
> + * * offset:   offset of register to read
> + * @write_reg: when specified called by client driver to write PMT state
> + * * dev:      device reference for the callback's use
> + * * guid:     ID of data to access
> + * * reg_data: register data
> + * * offset:   offset of register to write
>   */
>  struct pmt_callbacks {
>  	int (*read_telem)(struct device *dev, u32 guid, u64 *data, loff_t off, u32 count);
> +	int (*read_reg)(struct device *dev, u32 guid, u32 *reg_data, u32 offset);
> +	int (*write_reg)(struct device *dev, u32 guid, u32 reg_data, u32 offset);
>  };
>  
>  struct vsec_feature_dependency {
> 

-- 
 i.

Re: [PATCH v3 01/10] platform/x86/intel/pmt: complete pcidev to device update

[Ilpo Järvinen]

On Mon, 24 Aug 2026, Rodrigo Vivi wrote:

> On Mon, Aug 24, 2026 at 09:23:17AM -0700, Michael J. Ruhl wrote:
> > Previously the pcidev usage was moved to the struct device.
> > The struct device is set for only the telemetry endpoints.
> > 
> > This usage prevents other PMT features (crashlog) from using
> > the callback mechanism.
> > 
> > Use struct device in the intel_pmt_entry.
> > Update callback usage to os the pmt entry rather than the
> > telemetry endpoint.
> > 
> > Fixes: 353042d54d82 ("platform/x86/intel/vsec: Switch exported helpers from pci_dev to device")
> > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Dave, Hans, Ilpo, ack on getting this through the drm-xe-next tree?

Does David have anything in the horizon for the upcoming for-next cycle 
that could potentially conflict with this?

If not, then it would be fine for me to merge this through the drm-xe-next 
tree (the second patch needs some work though).

> Thanks,
> Rodrigo.
> 
> > ---
> >  drivers/platform/x86/intel/pmt/class.c     | 5 ++---
> >  drivers/platform/x86/intel/pmt/class.h     | 3 +--
> >  drivers/platform/x86/intel/pmt/discovery.c | 2 +-
> >  3 files changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> > index d0ab8e33c62a..402d51df834a 100644
> > --- a/drivers/platform/x86/intel/pmt/class.c
> > +++ b/drivers/platform/x86/intel/pmt/class.c
> > @@ -100,7 +100,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
> >  	if (count > entry->size - off)
> >  		count = entry->size - off;
> >  
> > -	count = pmt_telem_read_mmio(entry->ep->dev, entry->cb, entry->header.guid, buf,
> > +	count = pmt_telem_read_mmio(entry->dev, entry->cb, entry->header.guid, buf,
> >  				    entry->base, off, count);
> >  
> >  	return count;
> > @@ -286,8 +286,6 @@ static int pmt_resolve_access_pci(struct intel_pmt_entry *entry,
> >  		return -EINVAL;
> >  	}
> >  
> > -	entry->pcidev = pci_dev;
> > -
> >  	return 0;
> >  }
> >  
> > @@ -365,6 +363,7 @@ static int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
> >  	entry->guid = header->guid;
> >  	entry->size = header->size;
> >  	entry->cb = ivdev->priv_data;
> > +	entry->dev = ivdev->dev;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
> > index a0ece4fc3837..258cb460e61c 100644
> > --- a/drivers/platform/x86/intel/pmt/class.h
> > +++ b/drivers/platform/x86/intel/pmt/class.h
> > @@ -20,7 +20,6 @@
> >  #define GET_ADDRESS(v)		((v) & GENMASK(31, 3))
> >  
> >  struct device;
> > -struct pci_dev;
> >  extern struct class intel_pmt_class;
> >  
> >  struct telem_endpoint {
> > @@ -42,7 +41,7 @@ struct intel_pmt_header {
> >  
> >  struct intel_pmt_entry {
> >  	struct telem_endpoint	*ep;
> > -	struct pci_dev		*pcidev;
> > +	struct device		*dev;
> >  	struct intel_pmt_header	header;
> >  	u32			disc_header[PMT_DISC_DWORDS];
> >  	struct bin_attribute	pmt_bin_attr;
> > diff --git a/drivers/platform/x86/intel/pmt/discovery.c b/drivers/platform/x86/intel/pmt/discovery.c
> > index c482368bfaae..f4203d240f54 100644
> > --- a/drivers/platform/x86/intel/pmt/discovery.c
> > +++ b/drivers/platform/x86/intel/pmt/discovery.c
> > @@ -609,7 +609,7 @@ void intel_pmt_get_features(struct intel_pmt_entry *entry)
> >  
> >  	mutex_lock(&feature_list_lock);
> >  	list_for_each_entry(feature, &pmt_feature_list, list) {
> > -		if (feature->priv->parent != entry->ep->dev)
> > +		if (feature->priv->parent != entry->dev)
> >  			continue;
> >  
> >  		pmt_get_features(entry, feature);
> > -- 
> > 2.43.0
> > 
> 

-- 
 i.

Re: [PATCH v3 09/10] drm/xe/vsec: Support late bind fw information

[Ilpo Järvinen]

On Mon, 24 Aug 2026, Michael J. Ruhl wrote:

> CRI FW is loaded on power on.  Because of this, access to
> the FW cannot be done until it is running.
> 
> Update the XE PMT probe and access to check for late bind
> devices, verify, and wait for the appropriate FW state
> before probe or access.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c       |   4 +-
>  drivers/gpu/drm/xe/xe_device_types.h |   4 +
>  drivers/gpu/drm/xe/xe_vsec.c         | 141 +++++++++++++++++++++++++--
>  drivers/gpu/drm/xe/xe_vsec.h         |   2 +-
>  4 files changed, 143 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 74d566693dfd..bf02f881095f 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -1140,7 +1140,9 @@ int xe_device_probe(struct xe_device *xe)
>  	for_each_gt(gt, xe, id)
>  		xe_gt_sanitize_freq(gt);
>  
> -	xe_vsec_init(xe);
> +	err = xe_vsec_init(xe);
> +	if (err)
> +		goto err_unregister_display;
>  
>  	err = xe_sriov_init_late(xe);
>  	if (err)
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 3f1a70813a99..5d9e6e66c665 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -468,6 +468,10 @@ struct xe_device {
>  		struct mutex lock;
>  		/** @pmt.base_offset: device specific base offset */
>  		u64 base_offset;
> +		/** @pmt.work: support late-bind probe */
> +		struct delayed_work work;

Not sure if xe driver has some strange policy on headers due to their 
love for local headers... but if there isn't, this one doesn't have a 
direct include in this file.

> +		/** @pmt.retry_count: late-bind probe retry */
> +		u32 retry_count;
>  	} pmt;
>  
>  	/** @soc_remapper: SoC remapper object */
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 578d59048b39..edc20c24137e 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -3,6 +3,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/cleanup.h>
> +#include <linux/delay.h>
>  #include <linux/errno.h>
>  #include <linux/intel_vsec.h>
>  #include <linux/module.h>
> @@ -17,6 +18,7 @@
>  #include "xe_mmio.h"
>  #include "xe_platform_types.h"
>  #include "xe_pm.h"
> +#include "xe_sysctrl.h"
>  #include "xe_vsec.h"
>  
>  #include "regs/xe_pmt.h"
> @@ -162,6 +164,14 @@ enum capability {
>  	WATCHER,
>  };
>  
> +/*
> + * Late bind will delay 100msec for up to 20 seconds
> + */
> +#define VSEC_LATE_BIND_DELAY_MSEC	(100)
> +#define VSEC_LATE_BIND_RETRY		(200)

Remove the parenthesis?

> +
> +static void cri_late_bind_probe(struct xe_device *xe);
> +
>  static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
>  {
>  	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
> @@ -272,6 +282,56 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return -ENODEV;
>  }
>  
> +#define WAITING_FOR_SYCTLR
> +#ifdef WAITING_FOR_SYCTLR

It seems Rodrigo also noted this. Looks debug leftover or something along 
those lines to me.

> +static bool xe_is_oobmsm_fw_ready(struct xe_device *xe)
> +{
> +	return true;

Returning unconditionally true causes some dead code below on the caller 
side.

> +}
> +#endif
> +
> +static void cri_late_bind_probe_work(struct work_struct *work)
> +{
> +	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
> +
> +	if (xe_is_oobmsm_fw_ready(xe)) {
> +		cri_late_bind_probe(xe);
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +	xe->pmt.retry_count++;
> +
> +	/* wait up to 20 seconds */
> +	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
> +		drm_warn(&xe->drm, "PMT probe: Late Binding failed to complete\n");
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +
> +	if (!schedule_delayed_work(&xe->pmt.work, msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
> +		xe_pm_runtime_put(xe);
> +}
> +
> +static bool wait_for_fw(struct xe_device *xe)
> +{
> +	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
> +
> +	if (xe->info.platform != XE_CRESCENTISLAND)
> +		return true;
> +
> +	while (retries--) {
> +		if (xe_is_oobmsm_fw_ready(xe))
> +			return true;
> +
> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);
> +	}
> +
> +	drm_warn(&xe->drm, "Late Binding failed to complete\n");
> +
> +	return false;
> +}
> +
>  /*
>   * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
>   * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
> @@ -318,6 +378,11 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  		goto dev_exit;
>  	}
>  
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
> +
>  	mutex_lock(&xe->pmt.lock);
>  
>  	/* set SoC re-mapper index register based on GUID memory region */
> @@ -327,6 +392,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  
>  	mutex_unlock(&xe->pmt.lock);
>  
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:
> @@ -374,6 +440,10 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
>  	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>  
>  	xe_pm_runtime_get(xe);
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
>  	mutex_lock(&xe->pmt.lock);
>  
>  	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> @@ -381,6 +451,8 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
>  	memcpy_fromio(reg, disc_addr, sizeof(*reg));
>  
>  	mutex_unlock(&xe->pmt.lock);
> +
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:
> @@ -416,6 +488,10 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
>  	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>  
>  	xe_pm_runtime_get(xe);
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
>  	mutex_lock(&xe->pmt.lock);
>  
>  	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> @@ -423,6 +499,8 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
>  	memcpy_toio(disc_addr, &reg, sizeof(reg));
>  
>  	mutex_unlock(&xe->pmt.lock);
> +
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:
> @@ -454,12 +532,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
>  	return vsec_platforms[xe->info.platform];
>  }
>  
> +static void cri_late_bind_probe(struct xe_device *xe)
> +{
> +	struct intel_vsec_platform_info *info;
> +	struct device *dev = xe->drm.dev;
> +	enum xe_vsec platform;
> +
> +	platform = get_platform_info(xe);
> +	if (platform != XE_VSEC_CRI)
> +		return;
> +
> +	info = &xe_vsec_info[platform];
> +	if (!info->headers)
> +		return;
> +
> +	info->priv_data = &xe_cri_pmt_cb;
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	intel_vsec_register(dev, info);
> +}
> +
> +static void vsec_disable_late_bind_work(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	/*
> +	 * If was work was cancelled while it was still pending, we need to

Fix grammar.

> +	 * take care of releasing the runtime reference

Add .

> +	 */
> +	if (disable_delayed_work_sync(&xe->pmt.work))
> +		xe_pm_runtime_put(xe);
> +}
> +
>  /**
>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>   * interface
>   * @xe: valid xe instance
>   */
> -void xe_vsec_init(struct xe_device *xe)
> +int xe_vsec_init(struct xe_device *xe)
>  {
>  	struct intel_vsec_platform_info *info;
>  	struct device *dev = xe->drm.dev;
> @@ -467,30 +577,44 @@ void xe_vsec_init(struct xe_device *xe)
>  
>  	platform = get_platform_info(xe);
>  	if (platform == XE_VSEC_UNKNOWN)
> -		return;
> +		return 0;
>  
>  	info = &xe_vsec_info[platform];
>  	if (!info->headers)
> -		return;
> +		return 0;
>  
>  	switch (platform) {
>  	case XE_VSEC_BMG:
>  		if (!xe->soc_remapper.set_telem_region)
> -			return;
> +			return 0;
>  		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
>  		info->priv_data = &xe_bmg_pmt_cb;
>  		break;
>  
>  	case XE_VSEC_CRI:
>  		if (!xe->soc_remapper.set_telem_region)
> -			return;
> +			return 0;
>  		xe->pmt.base_offset = CRI_TELEMETRY_OFFSET;
> +
> +		xe->pmt.retry_count = 0;
> +		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
> +
> +		xe_pm_runtime_get_noresume(xe);
> +		if (!xe_is_oobmsm_fw_ready(xe)) {
> +			schedule_delayed_work(&xe->pmt.work,
> +					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
> +			return devm_add_action_or_reset(xe->drm.dev,
> +							vsec_disable_late_bind_work,
> +							xe);
> +		}
> +
>  		info->priv_data = &xe_cri_pmt_cb;
>  		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>  		break;
>  
>  	default:
> -		break;
> +		drm_err(&xe->drm, "Unsupported platform: %u\n", platform);
> +		return 0;
>  	}
>  
>  	/*
> @@ -498,5 +622,10 @@ void xe_vsec_init(struct xe_device *xe)
>  	 * resources.
>  	 */
>  	intel_vsec_register(dev, info);
> +
> +	if (platform == XE_VSEC_CRI)
> +		xe_pm_runtime_put(xe);
> +
> +	return 0;
>  }
>  MODULE_IMPORT_NS("INTEL_VSEC");
> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
> index a25b4e6e681b..c4a1e2fc67d8 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.h
> +++ b/drivers/gpu/drm/xe/xe_vsec.h
> @@ -9,7 +9,7 @@
>  struct device;
>  struct xe_device;
>  
> -void xe_vsec_init(struct xe_device *xe);
> +int xe_vsec_init(struct xe_device *xe);
>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
>  
>  #endif
> 

-- 
 i.

Re: [PATCH v3 10/10] drm/xe/vsec: Update PMT internal access for CRI

[Ilpo Järvinen]

On Mon, 24 Aug 2026, Michael J. Ruhl wrote:

> Xe access the PMT infrastructure directly. The current usage
> is supported ONLY by BMG devices.
> 
> CRI has further requirements for access.
> 
> Add a new API to access the GUID based on the platform.
> Use the API get the GUID for each device.
> 
> Minor cleanup for a newly unused parameter (mmio).
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_pmt.h     |  4 ++-
>  drivers/gpu/drm/xe/xe_debugfs.c      | 44 ++++++++++++-------------
>  drivers/gpu/drm/xe/xe_device_types.h |  2 ++
>  drivers/gpu/drm/xe/xe_hwmon.c        | 10 ++++--
>  drivers/gpu/drm/xe/xe_pcode.c        | 10 ++++--
>  drivers/gpu/drm/xe/xe_vsec.c         | 48 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_vsec.h         |  1 +
>  7 files changed, 92 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
> index fc9c9cb6a830..683bf401dc9d 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pmt.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
> @@ -10,7 +10,7 @@
>  #define BMG_PMT_BASE_OFFSET		0xDB000
>  #define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
>  
> -#define PUNIT_TELEMETRY_GUID		XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
> +#define BMG_PUNIT_TELEMETRY_GUID	XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
>  #define BMG_ENERGY_STATUS_PMT_OFFSET	(0x30)
>  #define	ENERGY_PKG			REG_GENMASK64(31, 0)
>  #define	ENERGY_CARD			REG_GENMASK64(63, 32)
> @@ -25,6 +25,8 @@
>  #define CRI_DISCOVERY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
>  #define CRI_TELEMETRY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
>  
> +#define CRI_PUNIT_TELEMETRY_GUID	XE_REG(CRI_DISCOVERY_OFFSET + 0x4)
> +
>  #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
>  #define BMG_G2_RESIDENCY_OFFSET		(0x530)
>  #define BMG_G6_RESIDENCY_OFFSET		(0x538)
> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> index 28135f84e286..12476549164d 100644
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> @@ -21,7 +21,6 @@
>  #include "xe_gt_printk.h"
>  #include "xe_guc_ads.h"
>  #include "xe_hw_engine.h"
> -#include "xe_mmio.h"
>  #include "xe_pagefault.h"
>  #include "xe_pcode.h"
>  #include "xe_pm.h"
> @@ -97,15 +96,20 @@ static void xe_fault_inject_debugfs_register(struct xe_device *xe,
>  	}
>  }
>  
> -static void read_residency_counter(struct xe_device *xe, struct xe_mmio *mmio,
> -				   u32 offset, const char *name, struct drm_printer *p)
> +static void read_residency_counter(struct xe_device *xe, u32 offset, const char *name,
> +				   struct drm_printer *p)
>  {
>  	u64 residency = 0;
> +	u32 guid;
>  	int ret;
>  
> -	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
> -				&residency, offset, sizeof(residency));
> +	guid = xe_vsec_get_guid(xe);
> +	if (!guid) {
> +		drm_warn(&xe->drm, "PMT device is not powered\n");
> +		return;
> +	}
> +
> +	ret = xe_pmt_telem_read(xe->drm.dev, guid, &residency, offset, sizeof(residency));
>  	if (ret != sizeof(residency)) {
>  		drm_warn(&xe->drm, "%s counter failed to read, ret %d\n", name, ret);
>  		return;
> @@ -244,13 +248,12 @@ static int pcode_info(struct seq_file *m, void *data)
>  static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
>  {
>  	struct xe_device *xe;
> -	struct xe_mmio *mmio;
>  	struct drm_printer p;
>  
>  	xe = node_to_xe(m->private);
>  	p = drm_seq_file_printer(m);
>  	guard(xe_pm_runtime)(xe);
> -	mmio = xe_root_tile_mmio(xe);
> +
>  	static const struct {
>  		u32 offset;
>  		const char *name;
> @@ -264,7 +267,7 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
>  	};
>  
>  	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
> -		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
> +		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
>  
>  	return 0;
>  }
> @@ -272,13 +275,11 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
>  static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
>  {
>  	struct xe_device *xe;
> -	struct xe_mmio *mmio;
>  	struct drm_printer p;
>  
>  	xe = node_to_xe(m->private);
>  	p = drm_seq_file_printer(m);
>  	guard(xe_pm_runtime)(xe);
> -	mmio = xe_root_tile_mmio(xe);
>  
>  	static const struct {
>  		u32 offset;
> @@ -290,7 +291,7 @@ static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
>  	};
>  
>  	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
> -		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
> +		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
>  
>  	return 0;
>  }
> @@ -710,23 +711,22 @@ void xe_debugfs_register(struct xe_device *xe)
>  				 ARRAY_SIZE(debugfs_list),
>  				 root, minor);
>  
> -	if (xe->info.platform == XE_BATTLEMAGE && !IS_SRIOV_VF(xe)) {
> -		drm_debugfs_create_files(debugfs_residencies,
> -					 ARRAY_SIZE(debugfs_residencies),
> -					 root, minor);
> -	}
> -
>  	/*
> -	 * Pcode version read from PMT is currently only supported on CRI and BMG platforms in PF
> -	 * mode, as both platforms support the necessary telemetry read mechanism and have a fixed
> -	 * PUNIT_VERSION_OFFSET.
> +	 * Residencies and Pcode version read from PMT is currently only supported on CRI and BMG
> +	 * platforms in PF mode.  Both platforms support the necessary telemetry read mechanism
> +	 * and have a fixed offsets for the required data.
>  	 * Attempting this access on other platforms must be verified before enabling support.
>  	 */
>  	if (!IS_SRIOV_VF(xe) &&
> -	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE))
> +	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE)) {
> +		drm_debugfs_create_files(debugfs_residencies,
> +					 ARRAY_SIZE(debugfs_residencies),

ARRAY_SIZE() has also own include which is missing from this file (looks 
it's already used in this file and if there's no special include policy in 
xe, it would be time to finally add the include).

> +					 root, minor);
> +
>  		drm_debugfs_create_files(pcode_info_debugfs,
>  					 ARRAY_SIZE(pcode_info_debugfs),
>  					 root, minor);
> +	}
>  
>  	debugfs_create_file("forcewake_all", 0400, root, xe,
>  			    &forcewake_all_fops);
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 5d9e6e66c665..7acc57f6d4fd 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -472,6 +472,8 @@ struct xe_device {
>  		struct delayed_work work;
>  		/** @pmt.retry_count: late-bind probe retry */
>  		u32 retry_count;
> +		/** @pmt.punit_guid_cache: cache of the PUINT GUID */
> +		u32 punit_guid_cache;
>  	} pmt;
>  
>  	/** @soc_remapper: SoC remapper object */
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5284cab6703d..c4b44eb4b220 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -517,9 +517,15 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
>  
>  	if (hwmon->xe->info.platform == XE_BATTLEMAGE) {
>  		u64 pmt_val;
> +		u32 guid;
>  
> -		ret = xe_pmt_telem_read(hwmon->xe->drm.dev,
> -					xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
> +		guid = xe_vsec_get_guid(hwmon->xe);
> +		if (!guid) {
> +			drm_warn(&hwmon->xe->drm, "PMT device is not powered\n");
> +			*energy = 0;
> +			return;
> +		}
> +		ret = xe_pmt_telem_read(hwmon->xe->drm.dev, guid,
>  					&pmt_val, BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
>  		if (ret != sizeof(pmt_val)) {
>  			drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index e1b8062541a9..7831dd597794 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -366,11 +366,17 @@ ALLOW_ERROR_INJECTION(xe_pcode_probe_early, ERRNO); /* See xe_pci_probe */
>  int xe_get_pcode_version(struct xe_device *xe, struct xe_pcode_version *version)
>  {
>  	int ret = 0;
> +	u32 guid;
>  
>  	guard(xe_pm_runtime)(xe);
>  
> -	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(xe_root_tile_mmio(xe), PUNIT_TELEMETRY_GUID),
> +	guid = xe_vsec_get_guid(xe);
> +	if (!guid) {
> +		xe_warn(xe, "PMT device is not powered\n");
> +		return -ENODATA;
> +	}
> +
> +	ret = xe_pmt_telem_read(xe->drm.dev, guid,
>  				(u64 *)version, PUNIT_VERSION_OFFSET, sizeof(*version));
>  	if (ret != sizeof(*version)) {
>  		xe_warn(xe, "pcode version read from PMT failed, ret %pe\n", ERR_PTR(ret));
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index edc20c24137e..5ebb4ee9ef08 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -564,6 +564,54 @@ static void vsec_disable_late_bind_work(void *arg)
>  		xe_pm_runtime_put(xe);
>  }
>  
> +u32 xe_vsec_get_guid(struct xe_device *xe)
> +{
> +	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> +	u32 guid;
> +
> +	/*
> +	 * Both supported platforms (BMG, CRI) require the remapper callback to
> +	 * access data. CRI needs it for the GUID.
> +	 */
> +	if (!xe->soc_remapper.set_telem_region)
> +		return 0;
> +
> +	/* caller must ensure correct power state */
> +	if (!xe_pm_runtime_get_if_active(xe))

The comments sounds like it's a coding error somewhere if we're not 
active at this point, which would be easier to catch if there would be 
WARN_ON_ONCE() instead of silently hiding the problem.

(I don't know much about xe, so please keep it in mind when considering 
my comments on xe side code.)

-- 
 i.

> +		return 0;
> +
> +	mutex_lock(&xe->pmt.lock);
> +
> +	if (xe->pmt.punit_guid_cache) {
> +		guid = xe->pmt.punit_guid_cache;
> +		goto unlock;
> +	}
> +
> +	switch (xe->info.platform) {
> +	case XE_BATTLEMAGE:
> +		guid = xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	case XE_CRESCENTISLAND:
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +		guid = xe_mmio_read32(mmio, CRI_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	default:
> +		guid = 0;
> +		drm_err(&xe->drm, "Unsupported platform: %u\n", xe->info.platform);
> +		break;
> +	}
> +
> +	xe->pmt.punit_guid_cache = guid;
> +
> +unlock:
> +	mutex_unlock(&xe->pmt.lock);
> +	xe_pm_runtime_put(xe);
> +
> +	return guid;
> +}
> +
>  /**
>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>   * interface
> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
> index c4a1e2fc67d8..43c3e9f227c5 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.h
> +++ b/drivers/gpu/drm/xe/xe_vsec.h
> @@ -10,6 +10,7 @@ struct device;
>  struct xe_device;
>  
>  int xe_vsec_init(struct xe_device *xe);
> +u32 xe_vsec_get_guid(struct xe_device *xe);
>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
>  
>  #endif
> 

Re: [PATCH v3 06/10] drm/xe/vsec: Support Crescent Island PMT

[Ilpo Järvinen]

On Mon, 24 Aug 2026, Michael J. Ruhl wrote:

> Crescent Island (CRI) supports PMT telemetry and crashlog.
> 
> Add Crescent Island (CRI) discovery structure (DVSEC)
> information to allow for Xe registration.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_pmt.h |  5 ++++
>  drivers/gpu/drm/xe/xe_vsec.c     | 46 ++++++++++++++++++++++++++++++--
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
> index a62ab05c6b4c..fc9c9cb6a830 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pmt.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
> @@ -20,6 +20,11 @@
>  #define BMG_TELEMETRY_BASE_OFFSET	0xE0000
>  #define BMG_TELEMETRY_OFFSET		(SOC_BASE + BMG_TELEMETRY_BASE_OFFSET)
>  
> +#define CRI_TELEMETRY_BASE_OFFSET	0xE0000
> +/* for CRI discovery and telemetry are in an indexed window */
> +#define CRI_DISCOVERY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
> +#define CRI_TELEMETRY_OFFSET		(SOC_BASE + CRI_TELEMETRY_BASE_OFFSET)
> +
>  #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
>  #define BMG_G2_RESIDENCY_OFFSET		(0x530)
>  #define BMG_G6_RESIDENCY_OFFSET		(0x538)
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 8abe11e6312f..6345b0b4b26b 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -21,8 +21,19 @@
>  
>  #include "regs/xe_pmt.h"
>  
> -/* PMT GUID value for BMG devices.  NOTE: this is NOT a PCI id */
> +/* PMT GUID value for BMG and CRI devices.  NOTE: this is NOT a PCI id */
>  #define BMG_DEVICE_ID 0xE2F8
> +#define CRI_DEVICE_ID 0xE2FA
> +
> +/*
> + * sizeof(Crashlog Type1 Version2) = 0x18 (24) bytes
> + * For BMG and CRI PUNIT and OOBMSMS crashlogs are consecutive.
> + */
> +#define BMG_CRASHLOG_PUNIT_DISC_OFFSET (0x60)
> +#define BMG_CRASHLOG_OOBMSM_DISC_OFFSET (BMG_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
> +
> +#define CRI_CRASHLOG_PUNIT_DISC_OFFSET (0x80)
> +#define CRI_CRASHLOG_OOBMSM_DISC_OFFSET (CRI_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
>  
>  static struct intel_vsec_header bmg_telemetry = {
>  	.rev = 1,
> @@ -41,7 +52,7 @@ static struct intel_vsec_header bmg_crashlog = {
>  	.num_entries = 2,
>  	.entry_size = 6,
>  	.tbir = 0,
> -	.offset = BMG_DISCOVERY_OFFSET + 0x60,
> +	.offset = BMG_DISCOVERY_OFFSET + BMG_CRASHLOG_PUNIT_DISC_OFFSET,
>  };
>  
>  static struct intel_vsec_header *bmg_capabilities[] = {
> @@ -50,9 +61,36 @@ static struct intel_vsec_header *bmg_capabilities[] = {
>  	NULL
>  };
>  
> +static struct intel_vsec_header cri_telemetry = {
> +	.rev = 1,
> +	.length = 0x10,

...

> +static struct intel_vsec_header cri_crashlog = {
> +	.rev = 1,
> +	.length = 0x10,

One could consider using SZ_x for these (if you do, make sure you've the 
header for it).

> +	.id = VSEC_ID_CRASHLOG,
> +	.num_entries = 2,
> +	.entry_size = 6,
> +	.tbir = 0,
> +	.offset = CRI_DISCOVERY_OFFSET + CRI_CRASHLOG_PUNIT_DISC_OFFSET,
> +};
> +
> +static struct intel_vsec_header *cri_capabilities[] = {
> +	&cri_telemetry,
> +	&cri_crashlog,
> +	NULL
> +};
> +
>  enum xe_vsec {
>  	XE_VSEC_UNKNOWN = 0,
>  	XE_VSEC_BMG,
> +	XE_VSEC_CRI,
>  };
>  
>  static struct intel_vsec_platform_info xe_vsec_info[] = {
> @@ -60,6 +98,10 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
>  		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
>  		.headers = bmg_capabilities,
>  	},
> +	[XE_VSEC_CRI] = {
> +		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
> +		.headers = cri_capabilities,
> +	},
>  	{ }
>  };
>  
> 

-- 
 i.

Re: [PATCH v3 07/10] drm/xe/vsec: Crescent Island PMT decode

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7750 bytes --]

On Mon, 24 Aug 2026, Michael J. Ruhl wrote:

> Crescent Island (CRI) has different index and offset values for
> accessing the PMT data area.
> 
> Update the decode path to support the CRI device.
> 
> Update the data read callback so to support the CRI usage.
> 
> Define several magic numbers.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device_types.h |   2 +
>  drivers/gpu/drm/xe/xe_vsec.c         | 135 ++++++++++++++++++++++-----
>  2 files changed, 114 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 180d450a6deb..3f1a70813a99 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -466,6 +466,8 @@ struct xe_device {
>  	struct {
>  		/** @pmt.lock: protect access for telemetry data */
>  		struct mutex lock;
> +		/** @pmt.base_offset: device specific base offset */
> +		u64 base_offset;
>  	} pmt;
>  
>  	/** @soc_remapper: SoC remapper object */
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 6345b0b4b26b..dc42b9492428 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -108,23 +108,47 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
>  /*
>   * The GUID will have the following bits to decode:
>   *   [0:3]   - {Telemetry space iteration number (0,1,..)}
> - *   [4:7]   - Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
> + *   [4:7]   - BMG Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
> + *   [4:5]   - CRI Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
> + *   [6:7]   - CRI Instance
>   *   [8:11]  - SOC_SKU
>   *   [12:27] – Device ID – changes for each down bin SKU’s
>   *   [28:29] - Capability Type (Crashlog-0, Telemetry Aggregator-1, Watcher-2)
>   *   [30:31] - Record-ID (0-PUNIT, 1-OOBMSM_0, 2-OOBMSM_1)
>   */
>  #define GUID_TELEM_ITERATION	GENMASK(3, 0)
> -#define GUID_SEGMENT		GENMASK(7, 4)
>  #define GUID_SOC_SKU		GENMASK(11, 8)
>  #define GUID_DEVICE_ID		GENMASK(27, 12)
>  #define GUID_CAP_TYPE		GENMASK(29, 28)
>  #define GUID_RECORD_ID		GENMASK(31, 30)
>  
> -#define PUNIT_TELEMETRY_OFFSET		0x0200
> -#define PUNIT_WATCHER_OFFSET		0x14A0
> -#define OOBMSM_0_WATCHER_OFFSET		0x18D8
> -#define OOBMSM_1_TELEMETRY_OFFSET	0x1000
> +#define BMG_GUID_SEGMENT	GENMASK(7, 4)
> +
> +#define CRI_GUID_SEGMENT	GENMASK(5, 4)
> +#define CRI_GUID_INSTANCE	GENMASK(7, 6)
> +
> +#define BMG_IDX_TELEM_PUNIT		0x00
> +#define BMG_IDX_TELEM_OOBMSM		0x01
> +#define BMG_IDX_CRASHLOG_PUNIT		0x02
> +#define BMG_IDX_CRASHLOG_OOBMSM		0x04
> +
> +#define BMG_PUNIT_TELEMETRY_OFFSET	0x0200
> +#define BMG_PUNIT_WATCHER_OFFSET	0x14A0
> +#define BMG_OOBMSM_0_WATCHER_OFFSET	0x18D8
> +#define BMG_OOBMSM_1_TELEMETRY_OFFSET	0x1000

IMO, it would be better to do the plain renames first to get a clean diff 
for the new stuff.

-- 
 i.

> +#define CRI_IDX_TELEM_DISCOVERY		0x00
> +#define CRI_IDX_TELEM_PUNIT		0x01
> +#define CRI_IDX_TELEM_OOBMSM		0x02
> +#define CRI_IDX_CRASHLOG_PUNIT		0x03
> +#define CRI_IDX_WATCHER_OOBMSM		0x03  /* PUNIT and OOBMSM share this index */
> +#define CRI_IDX_CRASHLOG_OOBMSM		0x04
> +
> +#define CRI_PUNIT_TELEMETRY_OFFSET		0x0200
> +#define CRI_PUNIT_WATCHER_OFFSET		0x08A0
> +#define CRI_OOBMSM_WATCHER_OFFSET		0x0CF8
> +#define CRI_OOBMSM_GFSP_TELEMETRY_OFFSET	0x1600
> +#define CRI_PUNIT_CRASHLOG_OFFSET		0x0E60
>  
>  enum record_id {
>  	PUNIT,
> @@ -138,45 +162,92 @@ enum capability {
>  	WATCHER,
>  };
>  
> -static int xe_guid_decode(u32 guid, int *index, u32 *offset)
> +static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
>  {
>  	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
>  	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
> -	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
> -
> -	if (device_id != BMG_DEVICE_ID)
> -		return -ENODEV;
> -
> -	if (cap_type > WATCHER)
> -		return -EINVAL;
>  
>  	*offset = 0;
>  
>  	if (cap_type == CRASHLOG) {
> -		*index = record_id == PUNIT ? 2 : 4;
> +		*index = record_id == PUNIT ? BMG_IDX_CRASHLOG_PUNIT : BMG_IDX_CRASHLOG_OOBMSM;
>  		return 0;
>  	}
>  
>  	switch (record_id) {
>  	case PUNIT:
> -		*index = 0;
> +		*index = BMG_IDX_TELEM_PUNIT;
>  		if (cap_type == TELEMETRY)
> -			*offset = PUNIT_TELEMETRY_OFFSET;
> +			*offset = BMG_PUNIT_TELEMETRY_OFFSET;
>  		else
> -			*offset = PUNIT_WATCHER_OFFSET;
> +			*offset = BMG_PUNIT_WATCHER_OFFSET;
>  		break;
>  
>  	case OOBMSM_0:
> -		*index = 1;
> +		*index = BMG_IDX_TELEM_OOBMSM;
>  		if (cap_type == WATCHER)
> -			*offset = OOBMSM_0_WATCHER_OFFSET;
> +			*offset = BMG_OOBMSM_0_WATCHER_OFFSET;
>  		break;
>  
>  	case OOBMSM_1:
> -		*index = 1;
> +		*index = BMG_IDX_TELEM_OOBMSM;
> +		if (cap_type == TELEMETRY)
> +			*offset = BMG_OOBMSM_1_TELEMETRY_OFFSET;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cri_guid_decode(u32 guid, int *index, u32 *offset)
> +{
> +	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
> +	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
> +	u32 instance  = FIELD_GET(CRI_GUID_INSTANCE, guid);
> +
> +	*offset = 0;
> +
> +	if (cap_type == CRASHLOG) {
> +		if (record_id == PUNIT) {
> +			*index = CRI_IDX_CRASHLOG_PUNIT;
> +			*offset = CRI_PUNIT_CRASHLOG_OFFSET;
> +		} else {
> +			*index = CRI_IDX_CRASHLOG_OOBMSM;
> +		}
> +		return 0;
> +	}
> +
> +	switch (record_id) {
> +	case PUNIT:
> +		*index = CRI_IDX_TELEM_PUNIT;
>  		if (cap_type == TELEMETRY)
> -			*offset = OOBMSM_1_TELEMETRY_OFFSET;
> +			*offset = CRI_PUNIT_TELEMETRY_OFFSET;
> +		else
> +			*offset = CRI_PUNIT_WATCHER_OFFSET;
> +		break;
> +
> +	case OOBMSM_0:
> +		*index = CRI_IDX_TELEM_OOBMSM;
> +		switch (instance) {
> +		case 0:
> +			if (cap_type == WATCHER) {
> +				*index = CRI_IDX_WATCHER_OOBMSM;
> +				*offset = CRI_OOBMSM_WATCHER_OFFSET;
> +			}
> +			break;
> +
> +		case 1:
> +			if (cap_type == TELEMETRY)
> +				*offset = CRI_OOBMSM_GFSP_TELEMETRY_OFFSET;
> +			break;
> +
> +		default:
> +			return -EINVAL;
> +		}
>  		break;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -184,6 +255,23 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return 0;
>  }
>  
> +static int xe_guid_decode(u32 guid, int *index, u32 *offset)
> +{
> +	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
> +	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
> +
> +	if (cap_type > WATCHER)
> +		return -EINVAL;
> +
> +	if (device_id == BMG_DEVICE_ID)
> +		return bmg_guid_decode(guid, index, offset);
> +
> +	if (device_id == CRI_DEVICE_ID)
> +		return cri_guid_decode(guid, index, offset);
> +
> +	return -ENODEV;
> +}
> +
>  /*
>   * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
>   * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
> @@ -193,8 +281,8 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  		      u32 count)
>  {
>  	struct xe_device *xe = kdev_to_xe_device(dev);
> -	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
>  	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
> +	void __iomem *telem_addr = xe->mmio.regs + xe->pmt.base_offset;
>  	u32 mem_region;
>  	u32 offset;
>  	int ret = 0;
> @@ -286,6 +374,7 @@ void xe_vsec_init(struct xe_device *xe)
>  	case XE_VSEC_BMG:
>  		if (!xe->soc_remapper.set_telem_region)
>  			return;
> +		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
>  		info->priv_data = &xe_pmt_cb;
>  		break;
>  	default:
> 

RE: [PATCH v3 09/10] drm/xe/vsec: Support late bind fw information

[Ruhl, Michael J]

>-----Original Message-----
>From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Sent: Monday, August 24, 2026 3:16 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org;
>hansg@kernel.org; ilpo.jarvinen@linux.intel.com; Brost, Matthew
><matthew.brost@intel.com>; thomas.hellstrom@linux.intel.com;
>airlied@gmail.com; simona@ffwll.ch; david.e.box@linux.intel.com; Vijay,
>Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
><badal.nilawar@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>;
>Ausmus, James <james.ausmus@intel.com>; Poosa, Karthik
><karthik.poosa@intel.com>
>Subject: Re: [PATCH v3 09/10] drm/xe/vsec: Support late bind fw information
>
>On Mon, Aug 24, 2026 at 09:23:25AM -0700, Michael J. Ruhl wrote:
>> CRI FW is loaded on power on.  Because of this, access to
>> the FW cannot be done until it is running.
>>
>> Update the XE PMT probe and access to check for late bind
>> devices, verify, and wait for the appropriate FW state
>> before probe or access.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_device.c       |   4 +-
>>  drivers/gpu/drm/xe/xe_device_types.h |   4 +
>>  drivers/gpu/drm/xe/xe_vsec.c         | 141 +++++++++++++++++++++++++--
>>  drivers/gpu/drm/xe/xe_vsec.h         |   2 +-
>>  4 files changed, 143 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c
>b/drivers/gpu/drm/xe/xe_device.c
>> index 74d566693dfd..bf02f881095f 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -1140,7 +1140,9 @@ int xe_device_probe(struct xe_device *xe)
>>  	for_each_gt(gt, xe, id)
>>  		xe_gt_sanitize_freq(gt);
>>
>> -	xe_vsec_init(xe);
>> +	err = xe_vsec_init(xe);
>> +	if (err)
>> +		goto err_unregister_display;
>>
>>  	err = xe_sriov_init_late(xe);
>>  	if (err)
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
>b/drivers/gpu/drm/xe/xe_device_types.h
>> index 3f1a70813a99..5d9e6e66c665 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -468,6 +468,10 @@ struct xe_device {
>>  		struct mutex lock;
>>  		/** @pmt.base_offset: device specific base offset */
>>  		u64 base_offset;
>> +		/** @pmt.work: support late-bind probe */
>> +		struct delayed_work work;
>> +		/** @pmt.retry_count: late-bind probe retry */
>> +		u32 retry_count;
>>  	} pmt;
>>
>>  	/** @soc_remapper: SoC remapper object */
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 578d59048b39..edc20c24137e 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>> @@ -3,6 +3,7 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/bits.h>
>>  #include <linux/cleanup.h>
>> +#include <linux/delay.h>
>>  #include <linux/errno.h>
>>  #include <linux/intel_vsec.h>
>>  #include <linux/module.h>
>> @@ -17,6 +18,7 @@
>>  #include "xe_mmio.h"
>>  #include "xe_platform_types.h"
>>  #include "xe_pm.h"
>> +#include "xe_sysctrl.h"
>>  #include "xe_vsec.h"
>>
>>  #include "regs/xe_pmt.h"
>> @@ -162,6 +164,14 @@ enum capability {
>>  	WATCHER,
>>  };
>>
>> +/*
>> + * Late bind will delay 100msec for up to 20 seconds
>> + */
>> +#define VSEC_LATE_BIND_DELAY_MSEC	(100)
>> +#define VSEC_LATE_BIND_RETRY		(200)
>> +
>> +static void cri_late_bind_probe(struct xe_device *xe);
>> +
>>  static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
>>  {
>>  	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
>> @@ -272,6 +282,56 @@ static int xe_guid_decode(u32 guid, int *index, u32
>*offset)
>>  	return -ENODEV;
>>  }
>>
>> +#define WAITING_FOR_SYCTLR
>> +#ifdef WAITING_FOR_SYCTLR
>
>I'm afraid you forgot to remove this before sending... or what's the goal of
>these?

Until the sysctrl patches (with this API) land, this will not compile, so this is to allow CI to not
stop looking at the patches based on its inability to compile.

If there is a better way to do this, I will use it.

M

>> +static bool xe_is_oobmsm_fw_ready(struct xe_device *xe)
>> +{
>> +	return true;
>> +}
>> +#endif
>> +
>> +static void cri_late_bind_probe_work(struct work_struct *work)
>> +{
>> +	struct xe_device *xe = container_of(work, struct xe_device,
>pmt.work.work);
>> +
>> +	if (xe_is_oobmsm_fw_ready(xe)) {
>> +		cri_late_bind_probe(xe);
>> +		xe_pm_runtime_put(xe);
>> +		return;
>> +	}
>> +
>> +	xe->pmt.retry_count++;
>> +
>> +	/* wait up to 20 seconds */
>> +	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
>> +		drm_warn(&xe->drm, "PMT probe: Late Binding failed to
>complete\n");
>> +		xe_pm_runtime_put(xe);
>> +		return;
>> +	}
>> +
>> +	if (!schedule_delayed_work(&xe->pmt.work,
>msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
>> +		xe_pm_runtime_put(xe);
>> +}
>> +
>> +static bool wait_for_fw(struct xe_device *xe)
>> +{
>> +	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
>> +
>> +	if (xe->info.platform != XE_CRESCENTISLAND)
>> +		return true;
>> +
>> +	while (retries--) {
>> +		if (xe_is_oobmsm_fw_ready(xe))
>> +			return true;
>> +
>> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);
>> +	}
>> +
>> +	drm_warn(&xe->drm, "Late Binding failed to complete\n");
>> +
>> +	return false;
>> +}
>> +
>>  /*
>>   * xe_pmt_telem_read is a callback API.  I.e this can be accessed external to
>>   * XE driver (PMT driver scope).  Because of this, DRM hotplug needs to be
>> @@ -318,6 +378,11 @@ int xe_pmt_telem_read(struct device *dev, u32
>guid, u64 *data, loff_t user_offse
>>  		goto dev_exit;
>>  	}
>>
>> +	if (!wait_for_fw(xe)) {
>> +		ret = -ENODATA;
>> +		goto runtime_exit;
>> +	}
>> +
>>  	mutex_lock(&xe->pmt.lock);
>>
>>  	/* set SoC re-mapper index register based on GUID memory region */
>> @@ -327,6 +392,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid,
>u64 *data, loff_t user_offse
>>
>>  	mutex_unlock(&xe->pmt.lock);
>>
>> +runtime_exit:
>>  	xe_pm_runtime_put(xe);
>>
>>  dev_exit:
>> @@ -374,6 +440,10 @@ static int xe_pmt_read_reg(struct device *dev, u32
>guid, u32 *reg, u32 offset)
>>  	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>>
>>  	xe_pm_runtime_get(xe);
>> +	if (!wait_for_fw(xe)) {
>> +		ret = -ENODATA;
>> +		goto runtime_exit;
>> +	}
>>  	mutex_lock(&xe->pmt.lock);
>>
>>  	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>> @@ -381,6 +451,8 @@ static int xe_pmt_read_reg(struct device *dev, u32
>guid, u32 *reg, u32 offset)
>>  	memcpy_fromio(reg, disc_addr, sizeof(*reg));
>>
>>  	mutex_unlock(&xe->pmt.lock);
>> +
>> +runtime_exit:
>>  	xe_pm_runtime_put(xe);
>>
>>  dev_exit:
>> @@ -416,6 +488,10 @@ static int xe_pmt_write_reg(struct device *dev, u32
>guid, u32 reg, u32 offset)
>>  	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>>
>>  	xe_pm_runtime_get(xe);
>> +	if (!wait_for_fw(xe)) {
>> +		ret = -ENODATA;
>> +		goto runtime_exit;
>> +	}
>>  	mutex_lock(&xe->pmt.lock);
>>
>>  	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>> @@ -423,6 +499,8 @@ static int xe_pmt_write_reg(struct device *dev, u32
>guid, u32 reg, u32 offset)
>>  	memcpy_toio(disc_addr, &reg, sizeof(reg));
>>
>>  	mutex_unlock(&xe->pmt.lock);
>> +
>> +runtime_exit:
>>  	xe_pm_runtime_put(xe);
>>
>>  dev_exit:
>> @@ -454,12 +532,44 @@ static enum xe_vsec get_platform_info(struct
>xe_device *xe)
>>  	return vsec_platforms[xe->info.platform];
>>  }
>>
>> +static void cri_late_bind_probe(struct xe_device *xe)
>> +{
>> +	struct intel_vsec_platform_info *info;
>> +	struct device *dev = xe->drm.dev;
>> +	enum xe_vsec platform;
>> +
>> +	platform = get_platform_info(xe);
>> +	if (platform != XE_VSEC_CRI)
>> +		return;
>> +
>> +	info = &xe_vsec_info[platform];
>> +	if (!info->headers)
>> +		return;
>> +
>> +	info->priv_data = &xe_cri_pmt_cb;
>> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>> +
>> +	intel_vsec_register(dev, info);
>> +}
>> +
>> +static void vsec_disable_late_bind_work(void *arg)
>> +{
>> +	struct xe_device *xe = arg;
>> +
>> +	/*
>> +	 * If was work was cancelled while it was still pending, we need to
>> +	 * take care of releasing the runtime reference
>> +	 */
>> +	if (disable_delayed_work_sync(&xe->pmt.work))
>> +		xe_pm_runtime_put(xe);
>> +}
>> +
>>  /**
>>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>>   * interface
>>   * @xe: valid xe instance
>>   */
>> -void xe_vsec_init(struct xe_device *xe)
>> +int xe_vsec_init(struct xe_device *xe)
>>  {
>>  	struct intel_vsec_platform_info *info;
>>  	struct device *dev = xe->drm.dev;
>> @@ -467,30 +577,44 @@ void xe_vsec_init(struct xe_device *xe)
>>
>>  	platform = get_platform_info(xe);
>>  	if (platform == XE_VSEC_UNKNOWN)
>> -		return;
>> +		return 0;
>>
>>  	info = &xe_vsec_info[platform];
>>  	if (!info->headers)
>> -		return;
>> +		return 0;
>>
>>  	switch (platform) {
>>  	case XE_VSEC_BMG:
>>  		if (!xe->soc_remapper.set_telem_region)
>> -			return;
>> +			return 0;
>>  		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
>>  		info->priv_data = &xe_bmg_pmt_cb;
>>  		break;
>>
>>  	case XE_VSEC_CRI:
>>  		if (!xe->soc_remapper.set_telem_region)
>> -			return;
>> +			return 0;
>>  		xe->pmt.base_offset = CRI_TELEMETRY_OFFSET;
>> +
>> +		xe->pmt.retry_count = 0;
>> +		INIT_DELAYED_WORK(&xe->pmt.work,
>cri_late_bind_probe_work);
>> +
>> +		xe_pm_runtime_get_noresume(xe);
>> +		if (!xe_is_oobmsm_fw_ready(xe)) {
>> +			schedule_delayed_work(&xe->pmt.work,
>> +
>msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
>> +			return devm_add_action_or_reset(xe->drm.dev,
>> +
>	vsec_disable_late_bind_work,
>> +							xe);
>> +		}
>> +
>>  		info->priv_data = &xe_cri_pmt_cb;
>>  		xe->soc_remapper.set_telem_region(xe,
>CRI_IDX_TELEM_DISCOVERY);
>>  		break;
>>
>>  	default:
>> -		break;
>> +		drm_err(&xe->drm, "Unsupported platform: %u\n", platform);
>> +		return 0;
>>  	}
>>
>>  	/*
>> @@ -498,5 +622,10 @@ void xe_vsec_init(struct xe_device *xe)
>>  	 * resources.
>>  	 */
>>  	intel_vsec_register(dev, info);
>> +
>> +	if (platform == XE_VSEC_CRI)
>> +		xe_pm_runtime_put(xe);
>> +
>> +	return 0;
>>  }
>>  MODULE_IMPORT_NS("INTEL_VSEC");
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
>> index a25b4e6e681b..c4a1e2fc67d8 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.h
>> +++ b/drivers/gpu/drm/xe/xe_vsec.h
>> @@ -9,7 +9,7 @@
>>  struct device;
>>  struct xe_device;
>>
>> -void xe_vsec_init(struct xe_device *xe);
>> +int xe_vsec_init(struct xe_device *xe);
>>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t
>user_offset, u32 count);
>>
>>  #endif
>> --
>> 2.43.0
>>

RE: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access callbacks

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Tuesday, August 25, 2026 5:35 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hansg@kernel.org>; Brost, Matthew <matthew.brost@intel.com>;
>Vivi, Rodrigo <rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com;
>airlied@gmail.com; simona@ffwll.ch; david.e.box@linux.intel.com; Vijay,
>Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
><badal.nilawar@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>;
>Ausmus, James <james.ausmus@intel.com>; Poosa, Karthik
><karthik.poosa@intel.com>
>Subject: Re: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access
>callbacks
>
>On Mon, 24 Aug 2026, Michael J. Ruhl wrote:
>
>> Some HW does not have direct MMIO access to PMT control and data
>> features.
>>
>> Augment the current callback infrastructure (data access) to allow
>> a registered driver to customize read/write access to the control
>> paths for PMT usage.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 39 +++++++++++++++++++++-
>-
>>  include/linux/intel_vsec.h                | 14 +++++++-
>>  2 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index f936daf99e4d..5923ad7abbd9 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -129,7 +129,19 @@ static void pmt_crashlog_rmw(struct
>crashlog_entry *crashlog, u32 bit, bool set)
>>  {
>>  	const struct crashlog_control *control = &crashlog->info->control;
>>  	struct intel_pmt_entry *entry = &crashlog->entry;
>> -	u32 reg = readl(entry->disc_table + control->offset);
>> +	u32 guid = entry->header.guid;
>> +	u32 reg;
>> +	int err;
>> +
>> +	if (entry->cb && entry->cb->read_reg) {
>> +		err = entry->cb->read_reg(entry->dev, guid, &reg, control-
>>offset);
>> +		if (err) {
>> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
>
>Never print __func__ in any user consumable message (level > debug) but
>write the message in plain English.
>
>Also, this needs include.
>
>> +			return;
>> +		}
>> +	} else {
>> +		reg = readl(entry->disc_table + control->offset);
>
>Add include.

I have added the linux/printk.h include... I am not clear on the correct readl inclued.

Should this be <linux/io.h> or <include/asm-generic/io.h>?

>> +	}
>>
>>  	reg &= ~control->trigger_mask;
>>
>> @@ -138,14 +150,35 @@ static void pmt_crashlog_rmw(struct
>crashlog_entry *crashlog, u32 bit, bool set)
>>  	else
>>  		reg &= ~bit;
>>
>> -	writel(reg, entry->disc_table + control->offset);
>> +	if (entry->cb && entry->cb->write_reg) {
>> +		err = entry->cb->write_reg(entry->dev, guid, reg, control-
>>offset);
>> +		if (err) {
>> +			pr_err("%s: failed to write reg: %d\n", __func__, err);
>
>Rephrase without using __func__.

This was for my internal debugging and should have been cleaned up.  Removed.

>> +			return;
>> +		}
>> +	} else {
>> +		writel(reg, entry->disc_table + control->offset);
>> +	}
>>  }
>>
>>  /* Read the status register and see if the specified @bit is set */
>>  static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
>>  {
>>  	const struct crashlog_status *status = &crashlog->info->status;
>> -	u32 reg = readl(crashlog->entry.disc_table + status->offset);
>> +	struct intel_pmt_entry *entry = &crashlog->entry;
>> +	u32 guid = entry->header.guid;
>> +	u32 reg;
>> +	int err;
>> +
>> +	if (entry->cb && entry->cb->read_reg) {
>> +		err = entry->cb->read_reg(entry->dev, guid, &reg, status-
>>offset);
>> +		if (err) {
>> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
>> +			return false;
>
>It seems you need a lerger rework here to properly return error codes.

Yes, there would be a significant rework (almost all internal functions will need to be updated)
to return the error codes.  This seems out of scope for these updates.

Does this need to be addressed to make progress on this patch set?  Or can we work on that
in the future?

>> +		}
>> +	} else {
>> +		reg = readl(crashlog->entry.disc_table + status->offset);
>
>entry->disc_table
>
>> +	}
>
>This looks mostly duplicated code, add a helper?

done

M

>>
>>  	return !!(reg & bit);
>>  }
>> diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
>> index 843cda8f8644..917d9397a993 100644
>> --- a/include/linux/intel_vsec.h
>> +++ b/include/linux/intel_vsec.h
>> @@ -90,13 +90,25 @@ enum intel_vsec_quirks {
>>   * @read_telem: when specified, called by client driver to access PMT
>>   * data (instead of direct copy).
>>   * * dev:   device reference for the callback's use
>> - * * guid:  ID of data to acccss
>> + * * guid:  ID of data to access
>>   * * data:  buffer for the data to be copied
>>   * * off:   offset into the requested buffer
>>   * * count: size of buffer
>> + * @read_reg: when specified called by client driver to read PMT state
>> + * * dev:      device reference for the callback's use
>> + * * guid:     ID of data to access
>> + * * reg_data: register data
>> + * * offset:   offset of register to read
>> + * @write_reg: when specified called by client driver to write PMT state
>> + * * dev:      device reference for the callback's use
>> + * * guid:     ID of data to access
>> + * * reg_data: register data
>> + * * offset:   offset of register to write
>>   */
>>  struct pmt_callbacks {
>>  	int (*read_telem)(struct device *dev, u32 guid, u64 *data, loff_t off,
>u32 count);
>> +	int (*read_reg)(struct device *dev, u32 guid, u32 *reg_data, u32
>offset);
>> +	int (*write_reg)(struct device *dev, u32 guid, u32 reg_data, u32
>offset);
>>  };
>>
>>  struct vsec_feature_dependency {
>>
>
>--
> i.

RE: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access callbacks

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5169 bytes --]

On Wed, 26 Aug 2026, Ruhl, Michael J wrote:

> >-----Original Message-----
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Tuesday, August 25, 2026 5:35 AM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
> >de Goede <hansg@kernel.org>; Brost, Matthew <matthew.brost@intel.com>;
> >Vivi, Rodrigo <rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com;
> >airlied@gmail.com; simona@ffwll.ch; david.e.box@linux.intel.com; Vijay,
> >Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
> ><badal.nilawar@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>;
> >Ausmus, James <james.ausmus@intel.com>; Poosa, Karthik
> ><karthik.poosa@intel.com>
> >Subject: Re: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access
> >callbacks
> >
> >On Mon, 24 Aug 2026, Michael J. Ruhl wrote:
> >
> >> Some HW does not have direct MMIO access to PMT control and data
> >> features.
> >>
> >> Augment the current callback infrastructure (data access) to allow
> >> a registered driver to customize read/write access to the control
> >> paths for PMT usage.
> >>
> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >> ---
> >>  drivers/platform/x86/intel/pmt/crashlog.c | 39 +++++++++++++++++++++-
> >-
> >>  include/linux/intel_vsec.h                | 14 +++++++-
> >>  2 files changed, 49 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> >b/drivers/platform/x86/intel/pmt/crashlog.c
> >> index f936daf99e4d..5923ad7abbd9 100644
> >> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> >> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> >> @@ -129,7 +129,19 @@ static void pmt_crashlog_rmw(struct
> >crashlog_entry *crashlog, u32 bit, bool set)
> >>  {
> >>  	const struct crashlog_control *control = &crashlog->info->control;
> >>  	struct intel_pmt_entry *entry = &crashlog->entry;
> >> -	u32 reg = readl(entry->disc_table + control->offset);
> >> +	u32 guid = entry->header.guid;
> >> +	u32 reg;
> >> +	int err;
> >> +
> >> +	if (entry->cb && entry->cb->read_reg) {
> >> +		err = entry->cb->read_reg(entry->dev, guid, &reg, control-
> >>offset);
> >> +		if (err) {
> >> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
> >
> >Never print __func__ in any user consumable message (level > debug) but
> >write the message in plain English.
> >
> >Also, this needs include.
> >
> >> +			return;
> >> +		}
> >> +	} else {
> >> +		reg = readl(entry->disc_table + control->offset);
> >
> >Add include.
> 
> I have added the linux/printk.h include... I am not clear on the correct readl inclued.
> 
> Should this be <linux/io.h> or <include/asm-generic/io.h>?

Usually it's better to use linux/ one, except in headers that are used 
in many .c files where it may be helpful to use as precise include as 
possible to limit the number of things one include ends up pulling in.

> >> +	}
> >>
> >>  	reg &= ~control->trigger_mask;
> >>
> >> @@ -138,14 +150,35 @@ static void pmt_crashlog_rmw(struct
> >crashlog_entry *crashlog, u32 bit, bool set)
> >>  	else
> >>  		reg &= ~bit;
> >>
> >> -	writel(reg, entry->disc_table + control->offset);
> >> +	if (entry->cb && entry->cb->write_reg) {
> >> +		err = entry->cb->write_reg(entry->dev, guid, reg, control-
> >>offset);
> >> +		if (err) {
> >> +			pr_err("%s: failed to write reg: %d\n", __func__, err);
> >
> >Rephrase without using __func__.
> 
> This was for my internal debugging and should have been cleaned up.  Removed.
> 
> >> +			return;
> >> +		}
> >> +	} else {
> >> +		writel(reg, entry->disc_table + control->offset);
> >> +	}
> >>  }
> >>
> >>  /* Read the status register and see if the specified @bit is set */
> >>  static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
> >>  {
> >>  	const struct crashlog_status *status = &crashlog->info->status;
> >> -	u32 reg = readl(crashlog->entry.disc_table + status->offset);
> >> +	struct intel_pmt_entry *entry = &crashlog->entry;
> >> +	u32 guid = entry->header.guid;
> >> +	u32 reg;
> >> +	int err;
> >> +
> >> +	if (entry->cb && entry->cb->read_reg) {
> >> +		err = entry->cb->read_reg(entry->dev, guid, &reg, status-
> >>offset);
> >> +		if (err) {
> >> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
> >> +			return false;
> >
> >It seems you need a lerger rework here to properly return error codes.
> 
> Yes, there would be a significant rework (almost all internal functions 
> will need to be updated) to return the error codes.  This seems out of 
> scope for these updates.

Your series adds a call that can fail (before there wasn't one) so it 
should be part of this effort. bool -> int conversion should be mostly 
simple (though admittedly a bit tedious) as it mainly just adds the 
error handling ifs to the callchains.

> Does this need to be addressed to make progress on this patch set?  Or can we work on that
> in the future?

I think it should be part of this effort because of the forementioned 
reason.


-- 
 i.

RE: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access callbacks

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Wednesday, August 26, 2026 2:30 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hansg@kernel.org>; Brost, Matthew <matthew.brost@intel.com>;
>Vivi, Rodrigo <rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com;
>airlied@gmail.com; simona@ffwll.ch; david.e.box@linux.intel.com; Vijay,
>Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
><badal.nilawar@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>;
>Ausmus, James <james.ausmus@intel.com>; Poosa, Karthik
><karthik.poosa@intel.com>
>Subject: RE: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access
>callbacks
>
>On Wed, 26 Aug 2026, Ruhl, Michael J wrote:
>
>> >-----Original Message-----
>> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> >Sent: Tuesday, August 25, 2026 5:35 AM
>> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>> >Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org;
>Hans
>> >de Goede <hansg@kernel.org>; Brost, Matthew
><matthew.brost@intel.com>;
>> >Vivi, Rodrigo <rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com;
>> >airlied@gmail.com; simona@ffwll.ch; david.e.box@linux.intel.com; Vijay,
>> >Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
>> ><badal.nilawar@intel.com>; Roper, Matthew D
><matthew.d.roper@intel.com>;
>> >Ausmus, James <james.ausmus@intel.com>; Poosa, Karthik
>> ><karthik.poosa@intel.com>
>> >Subject: Re: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access
>> >callbacks
>> >
>> >On Mon, 24 Aug 2026, Michael J. Ruhl wrote:
>> >
>> >> Some HW does not have direct MMIO access to PMT control and data
>> >> features.
>> >>
>> >> Augment the current callback infrastructure (data access) to allow
>> >> a registered driver to customize read/write access to the control
>> >> paths for PMT usage.
>> >>
>> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> >> ---
>> >>  drivers/platform/x86/intel/pmt/crashlog.c | 39
>+++++++++++++++++++++-
>> >-
>> >>  include/linux/intel_vsec.h                | 14 +++++++-
>> >>  2 files changed, 49 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>> >b/drivers/platform/x86/intel/pmt/crashlog.c
>> >> index f936daf99e4d..5923ad7abbd9 100644
>> >> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> >> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> >> @@ -129,7 +129,19 @@ static void pmt_crashlog_rmw(struct
>> >crashlog_entry *crashlog, u32 bit, bool set)
>> >>  {
>> >>  	const struct crashlog_control *control = &crashlog->info->control;
>> >>  	struct intel_pmt_entry *entry = &crashlog->entry;
>> >> -	u32 reg = readl(entry->disc_table + control->offset);
>> >> +	u32 guid = entry->header.guid;
>> >> +	u32 reg;
>> >> +	int err;
>> >> +
>> >> +	if (entry->cb && entry->cb->read_reg) {
>> >> +		err = entry->cb->read_reg(entry->dev, guid, &reg, control-
>> >>offset);
>> >> +		if (err) {
>> >> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
>> >
>> >Never print __func__ in any user consumable message (level > debug) but
>> >write the message in plain English.
>> >
>> >Also, this needs include.
>> >
>> >> +			return;
>> >> +		}
>> >> +	} else {
>> >> +		reg = readl(entry->disc_table + control->offset);
>> >
>> >Add include.
>>
>> I have added the linux/printk.h include... I am not clear on the correct readl
>inclued.
>>
>> Should this be <linux/io.h> or <include/asm-generic/io.h>?
>
>Usually it's better to use linux/ one, except in headers that are used
>in many .c files where it may be helpful to use as precise include as
>possible to limit the number of things one include ends up pulling in.
>
>> >> +	}
>> >>
>> >>  	reg &= ~control->trigger_mask;
>> >>
>> >> @@ -138,14 +150,35 @@ static void pmt_crashlog_rmw(struct
>> >crashlog_entry *crashlog, u32 bit, bool set)
>> >>  	else
>> >>  		reg &= ~bit;
>> >>
>> >> -	writel(reg, entry->disc_table + control->offset);
>> >> +	if (entry->cb && entry->cb->write_reg) {
>> >> +		err = entry->cb->write_reg(entry->dev, guid, reg, control-
>> >>offset);
>> >> +		if (err) {
>> >> +			pr_err("%s: failed to write reg: %d\n", __func__, err);
>> >
>> >Rephrase without using __func__.
>>
>> This was for my internal debugging and should have been cleaned up.
>Removed.
>>
>> >> +			return;
>> >> +		}
>> >> +	} else {
>> >> +		writel(reg, entry->disc_table + control->offset);
>> >> +	}
>> >>  }
>> >>
>> >>  /* Read the status register and see if the specified @bit is set */
>> >>  static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
>> >>  {
>> >>  	const struct crashlog_status *status = &crashlog->info->status;
>> >> -	u32 reg = readl(crashlog->entry.disc_table + status->offset);
>> >> +	struct intel_pmt_entry *entry = &crashlog->entry;
>> >> +	u32 guid = entry->header.guid;
>> >> +	u32 reg;
>> >> +	int err;
>> >> +
>> >> +	if (entry->cb && entry->cb->read_reg) {
>> >> +		err = entry->cb->read_reg(entry->dev, guid, &reg, status-
>> >>offset);
>> >> +		if (err) {
>> >> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
>> >> +			return false;
>> >
>> >It seems you need a lerger rework here to properly return error codes.
>>
>> Yes, there would be a significant rework (almost all internal functions
>> will need to be updated) to return the error codes.  This seems out of
>> scope for these updates.
>
>Your series adds a call that can fail (before there wasn't one) so it
>should be part of this effort. bool -> int conversion should be mostly
>simple (though admittedly a bit tedious) as it mainly just adds the
>error handling ifs to the callchains.
>
>> Does this need to be addressed to make progress on this patch set?  Or can we work on that
>> in the future?
>
>I think it should be part of this effort because of the forementioned
>reason.

Ok, I will rework the code paths.

M


>
>--
> i.