Enable SSRAM support in PTL and LNL

PATCH Superseded Xi Pardee 3 participants 17 messages Aug 15, 2025 – Sep 11, 2025

Subsystems (2)

x86 platform drivers Intel PMC core driver

lore

Share & tools

lore

Xi Pardee

author maintainer

2025-08-15 22:46 UTC #0 lore raw

v3 cover Enable SSRAM support in PTL and LNL

This series enables SSRAM support, including achieving PMC information, low power mode substate requirements, and device and function information for S0ix blockers from telemetry region, in Lunar Lake and Panther Lake platforms for Intel PMC Core driver.

The first patch enables SSRAM support for Lunar Lake platforms. The next three patches introduces a new table in telemetry region to get substate requirement information for platforms starting from Panther Lake. The fifth patch enable PMC Core driver to show device and function number associated to a subset of S0ix blockers. The last patch enables SSRAM support for Panther Lake platforms.

---
v3->v2:
- Add a patch to show device and function number.
- Use function pointer in struct pmc_dev_info to determine which substate requirement table to achieve and which show operation to use.
- Fix alignment.
- Move variable declarations to appropriate locations.
- Replace char pointer with enum as function parameter.

v2->v1:
- Change to only check ret variable value when getting substate data in pmc_core_get_telem_info().
- Return immediately when devm_kcalloc() fails.
- Return 0 instead of ret when succeeding in pmc_core_pmt_get_blk_sub_req().
- Replace devm_kzalloc() with devm_kcalloc().
- Add telem_info field of arl_pmc_dev variable.

Xi Pardee (6):
platform/x86:intel/pmc: Enable SSRAM support for Lunar Lake platform/x86:intel/pmc: Move telemetry endpoint register handling platform/x86:intel/pmc: Improve function to show substate header platform/x86:intel/pmc: Show substate requirement for S0ix blockers platform/x86:intel/pmc: Show device and function number platform/x86:intel/pmc: Enable SSRAM support for Panther Lake

 drivers/platform/x86/intel/pmc/arl.c  |   4 +
 drivers/platform/x86/intel/pmc/core.c | 367 +++++++++++++++++++++++---
 drivers/platform/x86/intel/pmc/core.h |  39 +++
 drivers/platform/x86/intel/pmc/lnl.c  |  18 ++
 drivers/platform/x86/intel/pmc/mtl.c  |   2 +
 drivers/platform/x86/intel/pmc/ptl.c  |  33 +++
 6 files changed, 427 insertions(+), 36 deletions(-)

Signature · 1 line

2.43.0

Xi Pardee

author maintainer

2025-08-15 22:46 UTC #1 lore raw

PATCH v3 1/6 platform/x86:intel/pmc: Enable SSRAM support for Lunar Lake

Enable Lunar Lake platforms to achieve PMC information from Intel PMC SSRAM Telemetry driver and substate requirements data from telemetry region.

Signed-off-by Xi Pardee <[email protected]>

Reviewed-by Ilpo Järvinen <[email protected]>

---
 drivers/platform/x86/intel/pmc/lnl.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/platform/x86/intel/pmc/lnl.c +16 -0

index da513c234714b..e08a77c778c2c 100644
@@ -13,6 +13,10 @@
 
 #include "core.h"
 
+#define SOCM_LPM_REQ_GUID	0x15099748
+
+static const u8 LNL_LPM_REG_INDEX[] = {0, 4, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20};
+
 static const struct pmc_bit_map lnl_ltr_show_map[] = {
 	{"SOUTHPORT_A",		CNP_PMC_LTR_SPA},
 	{"SOUTHPORT_B",		CNP_PMC_LTR_SPB},

@@ -528,6 +532,16 @@ static const struct pmc_reg_map lnl_socm_reg_map = {
 	.lpm_live_status_offset = MTL_LPM_LIVE_STATUS_OFFSET,
 	.s0ix_blocker_maps = lnl_blk_maps,
 	.s0ix_blocker_offset = LNL_S0IX_BLOCKER_OFFSET,
+	.lpm_reg_index = LNL_LPM_REG_INDEX,
+};
+
+static struct pmc_info lnl_pmc_info_list[] = {
+	{
+		.guid	= SOCM_LPM_REQ_GUID,
+		.devid	= PMC_DEVID_LNL_SOCM,
+		.map	= &lnl_socm_reg_map,
+	},
+	{}
 };
 
 #define LNL_NPU_PCI_DEV		0x643e

@@ -557,6 +571,8 @@ static int lnl_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_in
 }
 
 struct pmc_dev_info lnl_pmc_dev = {
+	.pci_func = 2,
+	.regmap_list = lnl_pmc_info_list,
 	.map = &lnl_socm_reg_map,
 	.suspend = cnl_suspend,
 	.resume = lnl_resume,

Signature · 1 line

2.43.0

Xi Pardee

author maintainer

2025-08-15 22:46 UTC #2 lore raw

PATCH v3 2/6 platform/x86:intel/pmc: Move telemetry endpoint register handling

In reply to Xi Pardee

Move telemetry endpoint handling to pmc_core_get_telem_info(). This is a preparation patch to introduce a new table to obtain Low Power Mode substate requirement data for platforms starting from Panther Lake.

Signed-off-by Xi Pardee <[email protected]>

Reviewed-by Ilpo Järvinen <[email protected]>

---
 drivers/platform/x86/intel/pmc/core.c | 51 +++++++++++++--------------
 1 file changed, 25 insertions(+), 26 deletions(-)

drivers/platform/x86/intel/pmc/core.c +25 ~~-26~~

index 540cd2fb0673b..a1dd80bdbd413 100644
@@ -1399,36 +1399,23 @@ static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *m
  * +----+---------------------------------------------------------+
  *
  */
-static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct pci_dev *pcidev)
+static int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+				    struct telem_endpoint *ep)
 {
-	struct telem_endpoint *ep;
 	const u8 *lpm_indices;
 	int num_maps, mode_offset = 0;
 	int ret, mode;
 	int lpm_size;
-	u32 guid;
 
 	lpm_indices = pmc->map->lpm_reg_index;
 	num_maps = pmc->map->lpm_num_maps;
 	lpm_size = LPM_MAX_NUM_MODES * num_maps;
 
-	guid = pmc_core_find_guid(pmcdev->regmap_list, pmc->map);
-	if (!guid)
-		return -ENXIO;
-
-	ep = pmt_telem_find_and_register_endpoint(pcidev, guid, 0);
-	if (IS_ERR(ep)) {
-		dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %pe", ep);
-		return -EPROBE_DEFER;
-	}
-
 	pmc->lpm_req_regs = devm_kzalloc(&pmcdev->pdev->dev,
 					 lpm_size * sizeof(u32),
 					 GFP_KERNEL);
-	if (!pmc->lpm_req_regs) {
-		ret = -ENOMEM;
-		goto unregister_ep;
-	}
+	if (!pmc->lpm_req_regs)
+		return -ENOMEM;
 
 	mode_offset = LPM_HEADER_OFFSET + LPM_MODE_OFFSET;
 	pmc_for_each_mode(mode, pmcdev) {

@@ -1442,23 +1429,21 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct
 			if (ret) {
 				dev_err(&pmcdev->pdev->dev,
 					"couldn't read Low Power Mode requirements: %d\n", ret);
-				goto unregister_ep;
+				return ret;
 			}
 			++req_offset;
 		}
 		mode_offset += LPM_REG_COUNT + LPM_MODE_OFFSET;
 	}
-
-unregister_ep:
-	pmt_telem_unregister_endpoint(ep);
-
 	return ret;
 }
 
-static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev, int func)
+static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func)
 {
 	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
+	struct telem_endpoint *ep;
 	unsigned int i;
+	u32 guid;
 	int ret;
 
 	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));

@@ -1466,10 +1451,24 @@ static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev, int func)
 		return -ENODEV;
 
 	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
-		if (!pmcdev->pmcs[i])
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[i];
+		if (!pmc)
 			continue;
 
-		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i], pcidev);
+		guid = pmc_core_find_guid(pmcdev->regmap_list, pmc->map);
+		if (!guid)
+			return -ENXIO;
+
+		ep = pmt_telem_find_and_register_endpoint(pcidev, guid, 0);
+		if (IS_ERR(ep)) {
+			dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %pe", ep);
+			return -EPROBE_DEFER;
+		}
+
+		ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
+		pmt_telem_unregister_endpoint(ep);
 		if (ret)
 			return ret;
 	}

@@ -1583,7 +1582,7 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 		pmc_core_punit_pmt_init(pmcdev, pmc_dev_info->dmu_guid);
 
 	if (ssram) {
-		ret = pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func);
+		ret = pmc_core_get_telem_info(pmcdev, pmc_dev_info->pci_func);
 		if (ret)
 			goto unmap_regbase;
 	}

Signature · 1 line

2.43.0

Xi Pardee

author maintainer

2025-08-15 22:46 UTC #3 lore raw

PATCH v3 3/6 platform/x86:intel/pmc: Improve function to show substate header

In reply to Xi Pardee

Refactor pmc_core_substate_req_header_show() to accept a new argument. This is a preparation patch to introduce a new way to show Low Power Mode substate requirement data for platforms starting from Panther Lake. Increased the size for the name column as the Low Power Mode requirement register name is longer in newer platforms.

Signed-off-by Xi Pardee <[email protected]>

---
 drivers/platform/x86/intel/pmc/core.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

drivers/platform/x86/intel/pmc/core.c +16 -6

index a1dd80bdbd413..cbfdcdc50ad21 100644
@@ -11,6 +11,11 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+enum header_type {
+	HEADER_STATUS,
+	HEADER_VALUE
+};
+
 #include <linux/bitfield.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>

@@ -828,17 +833,22 @@ static int pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
 
-static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index)
+static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
+					      enum header_type type)
 {
 	struct pmc_dev *pmcdev = s->private;
 	int mode;
 
-	seq_printf(s, "%30s |", "Element");
+	seq_printf(s, "%40s |", "Element");
 	pmc_for_each_mode(mode, pmcdev)
 		seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
 
-	seq_printf(s, " %9s |", "Status");
-	seq_printf(s, " %11s |\n", "Live Status");
+	if (type == HEADER_STATUS) {
+		seq_printf(s, " %9s |", "Status");
+		seq_printf(s, " %11s |\n", "Live Status");
+	} else {
+		seq_printf(s, " %9s |\n", "Value");
+	}
 }
 
 static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)

@@ -872,7 +882,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 			continue;
 
 		/* Display the header */
-		pmc_core_substate_req_header_show(s, pmc_index);
+		pmc_core_substate_req_header_show(s, pmc_index, HEADER_STATUS);
 
 		/* Loop over maps */
 		for (mp = 0; mp < num_maps; mp++) {

@@ -910,7 +920,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 				}
 
 				/* Display the element name in the first column */
-				seq_printf(s, "pmc%d: %26s |", pmc_index, map[i].name);
+				seq_printf(s, "pmc%d: %34s |", pmc_index, map[i].name);
 
 				/* Loop over the enabled states and display if required */
 				pmc_for_each_mode(mode, pmcdev) {

Signature · 1 line

2.43.0

Ilpo Järvinen

maintainer

2025-08-28 13:09 UTC #4 lore raw

Re: [PATCH v3 3/6] platform/x86:intel/pmc: Improve function to show substate header

Quoted 16 lines · Xi Pardee

Refactor pmc_core_substate_req_header_show() to accept a new argument. This is a preparation patch to introduce a new way to show Low Power Mode substate requirement data for platforms starting from Panther Lake. Increased the size for the name column as the Low Power Mode requirement register name is longer in newer platforms.
Signed-off-by Xi Pardee <[email protected]>
---
 drivers/platform/x86/intel/pmc/core.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
drivers/platform/x86/intel/pmc/core.c +0 -0
index a1dd80bdbd413..cbfdcdc50ad21 100644

drivers/platform/x86/intel/pmc/core.c line 16

Quoted 6 lines

drivers/platform/x86/intel/pmc/core.c +3 -0

@@ -11,6 +11,11 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+enum header_type {
+	HEADER_STATUS,
+	HEADER_VALUE

Please have the comma in any non-terminating entry.

Quoted 51 lines

drivers/platform/x86/intel/pmc/core.c +13 -6

+};
+
 #include <linux/bitfield.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>

@@ -828,17 +833,22 @@ static int pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
 
-static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index)
+static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
+					      enum header_type type)
 {
 	struct pmc_dev *pmcdev = s->private;
 	int mode;
 
-	seq_printf(s, "%30s |", "Element");
+	seq_printf(s, "%40s |", "Element");
 	pmc_for_each_mode(mode, pmcdev)
 		seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
 
-	seq_printf(s, " %9s |", "Status");
-	seq_printf(s, " %11s |\n", "Live Status");
+	if (type == HEADER_STATUS) {
+		seq_printf(s, " %9s |", "Status");
+		seq_printf(s, " %11s |\n", "Live Status");
+	} else {
+		seq_printf(s, " %9s |\n", "Value");
+	}
 }
 
 static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)

@@ -872,7 +882,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 			continue;
 
 		/* Display the header */
-		pmc_core_substate_req_header_show(s, pmc_index);
+		pmc_core_substate_req_header_show(s, pmc_index, HEADER_STATUS);
 
 		/* Loop over maps */
 		for (mp = 0; mp < num_maps; mp++) {

@@ -910,7 +920,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 				}
 
 				/* Display the element name in the first column */
-				seq_printf(s, "pmc%d: %26s |", pmc_index, map[i].name);
+				seq_printf(s, "pmc%d: %34s |", pmc_index, map[i].name);
 
 				/* Loop over the enabled states and display if required */
 				pmc_for_each_mode(mode, pmcdev) {

Signature · 1 line

i.

Xi Pardee

author maintainer

2025-09-05 16:50 UTC #5 lore raw

Re: [PATCH v3 3/6] platform/x86:intel/pmc: Improve function to show substate header

Hi Ilpo,

Thanks for the view.

Quoted 19 lines · Ilpo Järvinen

Quoted 17 lines · Xi Pardee
Refactor pmc_core_substate_req_header_show() to accept a new argument. This is a preparation patch to introduce a new way to show Low Power Mode substate requirement data for platforms starting from Panther Lake. Increased the size for the name column as the Low Power Mode requirement register name is longer in newer platforms.
Signed-off-by Xi Pardee <[email protected]>
---
  drivers/platform/x86/intel/pmc/core.c | 22 ++++++++++++++++------
  1 file changed, 16 insertions(+), 6 deletions(-)
drivers/platform/x86/intel/pmc/core.c +0 -0
index a1dd80bdbd413..cbfdcdc50ad21 100644
@@ -11,6 +11,11 @@
 

Quoted 6 lines

drivers/platform/x86/intel/pmc/core.c line 16
Quoted 5 lines
drivers/platform/x86/intel/pmc/core.c +3 -0
  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+enum header_type {
+	HEADER_STATUS,
+	HEADER_VALUE
Please have the comma in any non-terminating entry.

Will add comma in next version.

Quoted 52 lines

drivers/platform/x86/intel/pmc/core.c +13 -6

+};
+
  #include <linux/bitfield.h>
  #include <linux/debugfs.h>
  #include <linux/delay.h>

@@ -828,17 +833,22 @@ static int pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
  }
  DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
 
-static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index)
+static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
+					      enum header_type type)
  {
  	struct pmc_dev *pmcdev = s->private;
  	int mode;
 
-	seq_printf(s, "%30s |", "Element");
+	seq_printf(s, "%40s |", "Element");
  	pmc_for_each_mode(mode, pmcdev)
  		seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
 
-	seq_printf(s, " %9s |", "Status");
-	seq_printf(s, " %11s |\n", "Live Status");
+	if (type == HEADER_STATUS) {
+		seq_printf(s, " %9s |", "Status");
+		seq_printf(s, " %11s |\n", "Live Status");
+	} else {
+		seq_printf(s, " %9s |\n", "Value");
+	}
  }
 
  static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)

@@ -872,7 +882,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
  			continue;
 
  		/* Display the header */
-		pmc_core_substate_req_header_show(s, pmc_index);
+		pmc_core_substate_req_header_show(s, pmc_index, HEADER_STATUS);
 
  		/* Loop over maps */
  		for (mp = 0; mp < num_maps; mp++) {

@@ -910,7 +920,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
  				}
 
  				/* Display the element name in the first column */
-				seq_printf(s, "pmc%d: %26s |", pmc_index, map[i].name);
+				seq_printf(s, "pmc%d: %34s |", pmc_index, map[i].name);
 
  				/* Loop over the enabled states and display if required */
  				pmc_for_each_mode(mode, pmcdev) {

Xi Pardee

author maintainer

2025-08-15 22:46 UTC #6 lore raw

PATCH v3 4/6 platform/x86:intel/pmc: Show substate requirement for S0ix blockers

In reply to Xi Pardee

Add support to read and show S0ix blocker substate requirements. Starting from Panther Lake, substate requirement data is provided based on S0ix blockers instead of all low power mode requirements. For platforms that support this new feature, add support to display substate requirements based on S0ix blockers.

Change the "substate_requirements" attribute of Intel PMC Core driver to show the substate requirements for each S0ix blocker and the corresponding S0ix blocker value.

Signed-off-by Xi Pardee <[email protected]>

---
 drivers/platform/x86/intel/pmc/arl.c  |   4 +
 drivers/platform/x86/intel/pmc/core.c | 126 ++++++++++++++++++++++++--
 drivers/platform/x86/intel/pmc/core.h |  14 +++
 drivers/platform/x86/intel/pmc/lnl.c  |   2 +
 drivers/platform/x86/intel/pmc/mtl.c  |   2 +
 5 files changed, 138 insertions(+), 10 deletions(-)

drivers/platform/x86/intel/pmc/arl.c +4 -0

index 9d66d65e75779..17ad87b392abe 100644
@@ -725,9 +725,11 @@ struct pmc_dev_info arl_pmc_dev = {
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &arl_socs_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = arl_resume,
 	.init = arl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };
 
 struct pmc_dev_info arl_h_pmc_dev = {

@@ -735,7 +737,9 @@ struct pmc_dev_info arl_h_pmc_dev = {
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = arl_h_resume,
 	.init = arl_h_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };

drivers/platform/x86/intel/pmc/core.c +116 ~~-10~~

190 lines not on this page · raw message

drivers/platform/x86/intel/pmc/core.h +14 -0

index 4a94a4ee031e6..bfe8fba808063 100644
@@ -344,6 +344,8 @@ struct pmc_bit_map {
  * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
  * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
  * @s0ix_blocker_offset PWRMBASE offset to S0ix blocker counter
+ * @num_s0ix_blocker:	Number of S0ix blockers
+ * @blocker_req_offset:	Telemetry offset to S0ix blocker low power mode substate requirement table
  *
  * Each PCH has unique set of register offsets and bit indexes. This structure
  * captures them to have a common implementation.

@@ -369,6 +371,8 @@ struct pmc_reg_map {
 	const u32 ltr_ignore_max;
 	const u32 pm_vric1_offset;
 	const u32 s0ix_blocker_offset;
+	const u32 num_s0ix_blocker;
+	const u32 blocker_req_offset;
 	/* Low Power Mode registers */
 	const int lpm_num_maps;
 	const int lpm_num_modes;

@@ -474,18 +478,22 @@ enum pmc_index {
  *			SSRAM support.
  * @map:		Pointer to a pmc_reg_map struct that contains platform
  *			specific attributes of the primary PMC
+ * @sub_req_show:	File operations to show substate requirements
  * @suspend:		Function to perform platform specific suspend
  * @resume:		Function to perform platform specific resume
  * @init:		Function to perform platform specific init action
+ * @sub_req:		Function to achieve low power mode substate requirements
  */
 struct pmc_dev_info {
 	u8 pci_func;
 	u32 dmu_guid;
 	struct pmc_info *regmap_list;
 	const struct pmc_reg_map *map;
+	const struct file_operations *sub_req_show;
 	void (*suspend)(struct pmc_dev *pmcdev);
 	int (*resume)(struct pmc_dev *pmcdev);
 	int (*init)(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
+	int (*sub_req)(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
 };
 
 extern const struct pmc_bit_map msr_map[];

@@ -531,6 +539,12 @@ extern struct pmc_dev_info ptl_pmc_dev;
 
 void cnl_suspend(struct pmc_dev *pmcdev);
 int cnl_resume(struct pmc_dev *pmcdev);
+int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
+int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep);
+
+extern const struct file_operations pmc_core_substate_req_regs_fops;
+extern const struct file_operations pmc_core_substate_blk_req_fops;
 
 #define pmc_for_each_mode(mode, pmcdev)						\
 	for (unsigned int __i = 0, __cond;					\

drivers/platform/x86/intel/pmc/lnl.c +2 -0

index e08a77c778c2c..6fa027e7071f4 100644
@@ -574,7 +574,9 @@ struct pmc_dev_info lnl_pmc_dev = {
 	.pci_func = 2,
 	.regmap_list = lnl_pmc_info_list,
 	.map = &lnl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = lnl_resume,
 	.init = lnl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };

drivers/platform/x86/intel/pmc/mtl.c +2 -0

index faa13a7ee688f..0b87e10f864e6 100644
@@ -997,7 +997,9 @@ struct pmc_dev_info mtl_pmc_dev = {
 	.dmu_guid = MTL_PMT_DMU_GUID,
 	.regmap_list = mtl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = mtl_resume,
 	.init = mtl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };

Signature · 1 line

2.43.0

Ilpo Järvinen

maintainer

2025-08-28 13:06 UTC #7 lore raw

Re: [PATCH v3 4/6] platform/x86:intel/pmc: Show substate requirement for S0ix blockers

Quoted 78 lines · Xi Pardee

Change the "substate_requirements" attribute of Intel PMC Core driver to show the substate requirements for each S0ix blocker and the corresponding S0ix blocker value.

Signed-off-by Xi Pardee <[email protected]>

---
 drivers/platform/x86/intel/pmc/arl.c  |   4 +
 drivers/platform/x86/intel/pmc/core.c | 126 ++++++++++++++++++++++++--
 drivers/platform/x86/intel/pmc/core.h |  14 +++
 drivers/platform/x86/intel/pmc/lnl.c  |   2 +
 drivers/platform/x86/intel/pmc/mtl.c  |   2 +
 5 files changed, 138 insertions(+), 10 deletions(-)

drivers/platform/x86/intel/pmc/arl.c +4 -0

index 9d66d65e75779..17ad87b392abe 100644
@@ -725,9 +725,11 @@ struct pmc_dev_info arl_pmc_dev = {
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &arl_socs_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = arl_resume,
 	.init = arl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };
 
 struct pmc_dev_info arl_h_pmc_dev = {

@@ -735,7 +737,9 @@ struct pmc_dev_info arl_h_pmc_dev = {
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = arl_h_resume,
 	.init = arl_h_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };

drivers/platform/x86/intel/pmc/core.c +25 -0

index cbfdcdc50ad21..a0b948a875a5a 100644
@@ -851,6 +851,68 @@ static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
 	}
 }
 
+static int pmc_core_substate_blk_req_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	unsigned int pmc_index;
+
+	for (pmc_index = 0; pmc_index < ARRAY_SIZE(pmcdev->pmcs); pmc_index++) {
+		const struct pmc_bit_map **maps;
+		unsigned int arr_size, r_idx;
+		u32 offset, counter;
+		u32 *lpm_req_regs;
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmc_index];
+		if (!pmc || !pmc->lpm_req_regs)
+			continue;
+
+		lpm_req_regs = pmc->lpm_req_regs;
+		maps = pmc->map->s0ix_blocker_maps;
+		offset = pmc->map->s0ix_blocker_offset;
+		arr_size = pmc_core_lpm_get_arr_size(maps);
+
+		/* Display the header */
+		pmc_core_substate_req_header_show(s, pmc_index, HEADER_VALUE);
+
+		for (r_idx = 0; r_idx < arr_size; r_idx++) {

drivers/platform/x86/intel/pmc/core.c static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index, · line 888

Quoted 10 lines

drivers/platform/x86/intel/pmc/core.c +10 -0

+			const struct pmc_bit_map *map;
+
+			for (map = maps[r_idx]; map->name; map++) {
+				int mode;
+
+				if (!map->blk)
+					continue;
+
+				counter = pmc_core_reg_read(pmc, offset);
+				seq_printf(s, "pmc%d: %34s |", pmc_index, map->name);

For printing unsigned int, use %u.

drivers/platform/x86/intel/pmc/core.c static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index, · line 894

Quoted 6 lines

drivers/platform/x86/intel/pmc/core.c +6 -0

+				pmc_for_each_mode(mode, pmcdev) {
+					bool required = *lpm_req_regs & BIT(mode);
+
+					seq_printf(s, " %9s |", required ? "Required" : " ");
+				}
+				seq_printf(s, " %9d |\n", counter);

Use %u for u32.

Quoted 94 lines

drivers/platform/x86/intel/pmc/core.c +53 -6

+				offset += map->blk * S0IX_BLK_SIZE;
+				lpm_req_regs++;
+			}
+		}
+	}
+	return 0;
+}
+
+static int pmc_core_substate_blk_req_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pmc_core_substate_blk_req_show, inode->i_private);
+}
+
+const struct file_operations pmc_core_substate_blk_req_fops = {
+	.owner		= THIS_MODULE,
+	.open		= pmc_core_substate_blk_req_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmcdev = s->private;

@@ -941,7 +1003,19 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 	}
 	return 0;
 }
-DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
+
+static int pmc_core_substate_req_regs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pmc_core_substate_req_regs_show, inode->i_private);
+}
+
+const struct file_operations pmc_core_substate_req_regs_fops = {
+	.owner		= THIS_MODULE,
+	.open		= pmc_core_substate_req_regs_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
 
 static unsigned int pmc_core_get_crystal_freq(void)
 {

@@ -1274,7 +1348,7 @@ static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 	debugfs_remove_recursive(pmcdev->dbgfs_dir);
 }
 
-static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
+static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
 	struct pmc *primary_pmc = pmcdev->pmcs[PMC_IDX_MAIN];
 	struct dentry *dir;

@@ -1341,7 +1415,7 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 	if (primary_pmc->lpm_req_regs) {
 		debugfs_create_file("substate_requirements", 0444,
 				    pmcdev->dbgfs_dir, pmcdev,
-				    &pmc_core_substate_req_regs_fops);
+				    pmc_dev_info->sub_req_show);
 	}
 
 	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {

@@ -1409,8 +1483,7 @@ static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *m
  * +----+---------------------------------------------------------+
  *
  */
-static int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc,
-				    struct telem_endpoint *ep)
+int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
 {
 	const u8 *lpm_indices;
 	int num_maps, mode_offset = 0;

@@ -1448,7 +1521,37 @@ static int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc,
 	return ret;
 }
 
-static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func)
+int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep)
+{
+	u32 num_blocker, sample_id;
+	unsigned int index;
+	u32 *req_offset;
+	int ret;
+
+	num_blocker = pmc->map->num_s0ix_blocker;
+	sample_id = pmc->map->blocker_req_offset;
+
+	pmc->lpm_req_regs = devm_kcalloc(&pmcdev->pdev->dev, num_blocker,
+					 sizeof(u32), GFP_KERNEL);
+	if (!pmc->lpm_req_regs)
+		return -ENOMEM;
+

drivers/platform/x86/intel/pmc/core.c static int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1549

Quoted 10 lines

drivers/platform/x86/intel/pmc/core.c +10 -0

+	req_offset = pmc->lpm_req_regs;
+	for (index = 0; index < num_blocker; index++) {
+		ret = pmt_telem_read32(ep, sample_id, req_offset, 1);
+		if (ret) {
+			dev_err(&pmcdev->pdev->dev,
+				"couldn't read Low Power Mode requirements: %d\n", ret);
+			return ret;
+		}
+		sample_id++;
+		req_offset++;

These two look superfluous variables as they move in-sync with index and xx + index or xx[index] could be used instead? I can see the pointer one might make things a bit nicer looking, but if you preserve it, it should be incremented inside the for loop's increment, not here.

Quoted 23 lines

drivers/platform/x86/intel/pmc/core.c +7 -1

+	}
+	return 0;
+}
+
+static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
 	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
 	struct telem_endpoint *ep;

@@ -1456,11 +1559,12 @@ static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func)
 	u32 guid;
 	int ret;
 
-	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));
+	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, pmc_dev_info->pci_func));
 	if (!pcidev)
 		return -ENODEV;
 
 	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
+		int (*sub_req)(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
 		struct pmc *pmc;
 
 		pmc = pmcdev->pmcs[i];

drivers/platform/x86/intel/pmc/core.c static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func) · line 1586

Quoted 7 lines

drivers/platform/x86/intel/pmc/core.c +3 -1

@@ -1477,7 +1581,9 @@ static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func)
 			return -EPROBE_DEFER;
 		}
 
-		ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
+		sub_req = pmc_dev_info->sub_req;
+		ret = sub_req(pmcdev, pmc, ep);
+

Please don't separate a call and its error handling with empty lines.

Why is the sub_req variable necessary?

Quoted 108 lines

drivers/platform/x86/intel/pmc/core.c +2 -2

 		pmt_telem_unregister_endpoint(ep);
 		if (ret)
 			return ret;

@@ -1592,7 +1698,7 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 		pmc_core_punit_pmt_init(pmcdev, pmc_dev_info->dmu_guid);
 
 	if (ssram) {
-		ret = pmc_core_get_telem_info(pmcdev, pmc_dev_info->pci_func);
+		ret = pmc_core_get_telem_info(pmcdev, pmc_dev_info);
 		if (ret)
 			goto unmap_regbase;
 	}

@@ -1766,7 +1872,7 @@ static int pmc_core_probe(struct platform_device *pdev)
 	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(primary_pmc);
 	pmc_core_do_dmi_quirks(primary_pmc);
 
-	pmc_core_dbgfs_register(pmcdev);
+	pmc_core_dbgfs_register(pmcdev, pmc_dev_info);
 	pm_report_max_hw_sleep(FIELD_MAX(SLP_S0_RES_COUNTER_MASK) *
 			       pmc_core_adjust_slp_s0_step(primary_pmc, 1));

drivers/platform/x86/intel/pmc/core.h +14 -0

index 4a94a4ee031e6..bfe8fba808063 100644
@@ -344,6 +344,8 @@ struct pmc_bit_map {
  * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
  * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
  * @s0ix_blocker_offset PWRMBASE offset to S0ix blocker counter
+ * @num_s0ix_blocker:	Number of S0ix blockers
+ * @blocker_req_offset:	Telemetry offset to S0ix blocker low power mode substate requirement table
  *
  * Each PCH has unique set of register offsets and bit indexes. This structure
  * captures them to have a common implementation.

@@ -369,6 +371,8 @@ struct pmc_reg_map {
 	const u32 ltr_ignore_max;
 	const u32 pm_vric1_offset;
 	const u32 s0ix_blocker_offset;
+	const u32 num_s0ix_blocker;
+	const u32 blocker_req_offset;
 	/* Low Power Mode registers */
 	const int lpm_num_maps;
 	const int lpm_num_modes;

@@ -474,18 +478,22 @@ enum pmc_index {
  *			SSRAM support.
  * @map:		Pointer to a pmc_reg_map struct that contains platform
  *			specific attributes of the primary PMC
+ * @sub_req_show:	File operations to show substate requirements
  * @suspend:		Function to perform platform specific suspend
  * @resume:		Function to perform platform specific resume
  * @init:		Function to perform platform specific init action
+ * @sub_req:		Function to achieve low power mode substate requirements
  */
 struct pmc_dev_info {
 	u8 pci_func;
 	u32 dmu_guid;
 	struct pmc_info *regmap_list;
 	const struct pmc_reg_map *map;
+	const struct file_operations *sub_req_show;
 	void (*suspend)(struct pmc_dev *pmcdev);
 	int (*resume)(struct pmc_dev *pmcdev);
 	int (*init)(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
+	int (*sub_req)(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
 };
 
 extern const struct pmc_bit_map msr_map[];

@@ -531,6 +539,12 @@ extern struct pmc_dev_info ptl_pmc_dev;
 
 void cnl_suspend(struct pmc_dev *pmcdev);
 int cnl_resume(struct pmc_dev *pmcdev);
+int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
+int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep);
+
+extern const struct file_operations pmc_core_substate_req_regs_fops;
+extern const struct file_operations pmc_core_substate_blk_req_fops;
 
 #define pmc_for_each_mode(mode, pmcdev)						\
 	for (unsigned int __i = 0, __cond;					\

drivers/platform/x86/intel/pmc/lnl.c +2 -0

index e08a77c778c2c..6fa027e7071f4 100644
@@ -574,7 +574,9 @@ struct pmc_dev_info lnl_pmc_dev = {
 	.pci_func = 2,
 	.regmap_list = lnl_pmc_info_list,
 	.map = &lnl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = lnl_resume,
 	.init = lnl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };

drivers/platform/x86/intel/pmc/mtl.c +2 -0

index faa13a7ee688f..0b87e10f864e6 100644
@@ -997,7 +997,9 @@ struct pmc_dev_info mtl_pmc_dev = {
 	.dmu_guid = MTL_PMT_DMU_GUID,
 	.regmap_list = mtl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = mtl_resume,
 	.init = mtl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };

Signature · 1 line

i.

Xi Pardee

author maintainer

2025-09-05 17:31 UTC #8 lore raw

Re: [PATCH v3 4/6] platform/x86:intel/pmc: Show substate requirement for S0ix blockers

Hi Ilpo,

Thanks for the review.

Quoted 83 lines · Ilpo Järvinen

Quoted 81 lines · Xi Pardee

Change the "substate_requirements" attribute of Intel PMC Core driver to show the substate requirements for each S0ix blocker and the corresponding S0ix blocker value.

Signed-off-by Xi Pardee <[email protected]>

---
  drivers/platform/x86/intel/pmc/arl.c  |   4 +
  drivers/platform/x86/intel/pmc/core.c | 126 ++++++++++++++++++++++++--
  drivers/platform/x86/intel/pmc/core.h |  14 +++
  drivers/platform/x86/intel/pmc/lnl.c  |   2 +
  drivers/platform/x86/intel/pmc/mtl.c  |   2 +
  5 files changed, 138 insertions(+), 10 deletions(-)

drivers/platform/x86/intel/pmc/arl.c +4 -0

index 9d66d65e75779..17ad87b392abe 100644
@@ -725,9 +725,11 @@ struct pmc_dev_info arl_pmc_dev = {
  	.dmu_guid = ARL_PMT_DMU_GUID,
  	.regmap_list = arl_pmc_info_list,
  	.map = &arl_socs_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
  	.suspend = cnl_suspend,
  	.resume = arl_resume,
  	.init = arl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
  };
 
  struct pmc_dev_info arl_h_pmc_dev = {

@@ -735,7 +737,9 @@ struct pmc_dev_info arl_h_pmc_dev = {
  	.dmu_guid = ARL_PMT_DMU_GUID,
  	.regmap_list = arl_pmc_info_list,
  	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
  	.suspend = cnl_suspend,
  	.resume = arl_h_resume,
  	.init = arl_h_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
  };

drivers/platform/x86/intel/pmc/core.c +28 -0

index cbfdcdc50ad21..a0b948a875a5a 100644
@@ -851,6 +851,68 @@ static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
  	}
  }
 
+static int pmc_core_substate_blk_req_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	unsigned int pmc_index;
+
+	for (pmc_index = 0; pmc_index < ARRAY_SIZE(pmcdev->pmcs); pmc_index++) {
+		const struct pmc_bit_map **maps;
+		unsigned int arr_size, r_idx;
+		u32 offset, counter;
+		u32 *lpm_req_regs;
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmc_index];
+		if (!pmc || !pmc->lpm_req_regs)
+			continue;
+
+		lpm_req_regs = pmc->lpm_req_regs;
+		maps = pmc->map->s0ix_blocker_maps;
+		offset = pmc->map->s0ix_blocker_offset;
+		arr_size = pmc_core_lpm_get_arr_size(maps);
+
+		/* Display the header */
+		pmc_core_substate_req_header_show(s, pmc_index, HEADER_VALUE);
+
+		for (r_idx = 0; r_idx < arr_size; r_idx++) {
+			const struct pmc_bit_map *map;
+
+			for (map = maps[r_idx]; map->name; map++) {

Quoted 8 lines

drivers/platform/x86/intel/pmc/core.c static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index, · line 888
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +7 -0
+				int mode;
+
+				if (!map->blk)
+					continue;
+
+				counter = pmc_core_reg_read(pmc, offset);
+				seq_printf(s, "pmc%d: %34s |", pmc_index, map->name);
For printing unsigned int, use %u.

Will change to %u in next version.

Quoted 8 lines

drivers/platform/x86/intel/pmc/core.c static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index, · line 894
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +6 -0
+				pmc_for_each_mode(mode, pmcdev) {
+					bool required = *lpm_req_regs & BIT(mode);
+
+					seq_printf(s, " %9s |", required ? "Required" : " ");
+				}
+				seq_printf(s, " %9d |\n", counter);
Use %u for u32.

Will change to %u in next version.

Quoted 101 lines

drivers/platform/x86/intel/pmc/core.c +59 -6

+				offset += map->blk * S0IX_BLK_SIZE;
+				lpm_req_regs++;
+			}
+		}
+	}
+	return 0;
+}
+
+static int pmc_core_substate_blk_req_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pmc_core_substate_blk_req_show, inode->i_private);
+}
+
+const struct file_operations pmc_core_substate_blk_req_fops = {
+	.owner		= THIS_MODULE,
+	.open		= pmc_core_substate_blk_req_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
  static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
  {
  	struct pmc_dev *pmcdev = s->private;

@@ -941,7 +1003,19 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
  	}
  	return 0;
  }
-DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
+
+static int pmc_core_substate_req_regs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pmc_core_substate_req_regs_show, inode->i_private);
+}
+
+const struct file_operations pmc_core_substate_req_regs_fops = {
+	.owner		= THIS_MODULE,
+	.open		= pmc_core_substate_req_regs_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
 
  static unsigned int pmc_core_get_crystal_freq(void)
  {

@@ -1274,7 +1348,7 @@ static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
  	debugfs_remove_recursive(pmcdev->dbgfs_dir);
  }
 
-static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
+static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
  {
  	struct pmc *primary_pmc = pmcdev->pmcs[PMC_IDX_MAIN];
  	struct dentry *dir;

@@ -1341,7 +1415,7 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
  	if (primary_pmc->lpm_req_regs) {
  		debugfs_create_file("substate_requirements", 0444,
  				    pmcdev->dbgfs_dir, pmcdev,
-				    &pmc_core_substate_req_regs_fops);
+				    pmc_dev_info->sub_req_show);
  	}
 
  	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {

@@ -1409,8 +1483,7 @@ static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *m
   * +----+---------------------------------------------------------+
   *
   */
-static int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc,
-				    struct telem_endpoint *ep)
+int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
  {
  	const u8 *lpm_indices;
  	int num_maps, mode_offset = 0;

@@ -1448,7 +1521,37 @@ static int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc,
  	return ret;
  }
 
-static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func)
+int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep)
+{
+	u32 num_blocker, sample_id;
+	unsigned int index;
+	u32 *req_offset;
+	int ret;
+
+	num_blocker = pmc->map->num_s0ix_blocker;
+	sample_id = pmc->map->blocker_req_offset;
+
+	pmc->lpm_req_regs = devm_kcalloc(&pmcdev->pdev->dev, num_blocker,
+					 sizeof(u32), GFP_KERNEL);
+	if (!pmc->lpm_req_regs)
+		return -ENOMEM;
+
+	req_offset = pmc->lpm_req_regs;
+	for (index = 0; index < num_blocker; index++) {
+		ret = pmt_telem_read32(ep, sample_id, req_offset, 1);
+		if (ret) {
+			dev_err(&pmcdev->pdev->dev,
+				"couldn't read Low Power Mode requirements: %d\n", ret);

Quoted 8 lines

drivers/platform/x86/intel/pmc/core.c static int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1549
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +4 -0
+			return ret;
+		}
+		sample_id++;
+		req_offset++;
These two look superfluous variables as they move in-sync with index and xx + index or xx[index] could be used instead? I can see the pointer one might make things a bit nicer looking, but if you preserve it, it should be incremented inside the for loop's increment, not here.

Will rename sample_id to sample_offset and use sample_offset + index as argument in pmt_telem_read32() call instead.

Will move req_offset++ inside the for loop's increment in next version.

Quoted 26 lines

drivers/platform/x86/intel/pmc/core.c +7 -1

+	}
+	return 0;
+}
+
+static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
  {
  	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
  	struct telem_endpoint *ep;

@@ -1456,11 +1559,12 @@ static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func)
  	u32 guid;
  	int ret;
 
-	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));
+	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, pmc_dev_info->pci_func));
  	if (!pcidev)
  		return -ENODEV;
 
  	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
+		int (*sub_req)(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
  		struct pmc *pmc;
 
  		pmc = pmcdev->pmcs[i];

@@ -1477,7 +1581,9 @@ static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func)
  			return -EPROBE_DEFER;
  		}

Quoted 7 lines

drivers/platform/x86/intel/pmc/core.c static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func) · line 1586
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +3 -1
-		ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
+		sub_req = pmc_dev_info->sub_req;
+		ret = sub_req(pmcdev, pmc, ep);
+
Please don't separate a call and its error handling with empty lines.
Why is the sub_req variable necessary?

The sub_req is not necessary. Will remove it in next version.

Thanks!

Quoted 109 lines

drivers/platform/x86/intel/pmc/core.c +2 -2

  		pmt_telem_unregister_endpoint(ep);
  		if (ret)
  			return ret;

@@ -1592,7 +1698,7 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
  		pmc_core_punit_pmt_init(pmcdev, pmc_dev_info->dmu_guid);
 
  	if (ssram) {
-		ret = pmc_core_get_telem_info(pmcdev, pmc_dev_info->pci_func);
+		ret = pmc_core_get_telem_info(pmcdev, pmc_dev_info);
  		if (ret)
  			goto unmap_regbase;
  	}

@@ -1766,7 +1872,7 @@ static int pmc_core_probe(struct platform_device *pdev)
  	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(primary_pmc);
  	pmc_core_do_dmi_quirks(primary_pmc);
 
-	pmc_core_dbgfs_register(pmcdev);
+	pmc_core_dbgfs_register(pmcdev, pmc_dev_info);
  	pm_report_max_hw_sleep(FIELD_MAX(SLP_S0_RES_COUNTER_MASK) *
  			       pmc_core_adjust_slp_s0_step(primary_pmc, 1));

drivers/platform/x86/intel/pmc/core.h +14 -0

index 4a94a4ee031e6..bfe8fba808063 100644
@@ -344,6 +344,8 @@ struct pmc_bit_map {
   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
   * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
   * @s0ix_blocker_offset PWRMBASE offset to S0ix blocker counter
+ * @num_s0ix_blocker:	Number of S0ix blockers
+ * @blocker_req_offset:	Telemetry offset to S0ix blocker low power mode substate requirement table
   *
   * Each PCH has unique set of register offsets and bit indexes. This structure
   * captures them to have a common implementation.

@@ -369,6 +371,8 @@ struct pmc_reg_map {
  	const u32 ltr_ignore_max;
  	const u32 pm_vric1_offset;
  	const u32 s0ix_blocker_offset;
+	const u32 num_s0ix_blocker;
+	const u32 blocker_req_offset;
  	/* Low Power Mode registers */
  	const int lpm_num_maps;
  	const int lpm_num_modes;

@@ -474,18 +478,22 @@ enum pmc_index {
   *			SSRAM support.
   * @map:		Pointer to a pmc_reg_map struct that contains platform
   *			specific attributes of the primary PMC
+ * @sub_req_show:	File operations to show substate requirements
   * @suspend:		Function to perform platform specific suspend
   * @resume:		Function to perform platform specific resume
   * @init:		Function to perform platform specific init action
+ * @sub_req:		Function to achieve low power mode substate requirements
   */
  struct pmc_dev_info {
  	u8 pci_func;
  	u32 dmu_guid;
  	struct pmc_info *regmap_list;
  	const struct pmc_reg_map *map;
+	const struct file_operations *sub_req_show;
  	void (*suspend)(struct pmc_dev *pmcdev);
  	int (*resume)(struct pmc_dev *pmcdev);
  	int (*init)(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
+	int (*sub_req)(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
  };
 
  extern const struct pmc_bit_map msr_map[];

@@ -531,6 +539,12 @@ extern struct pmc_dev_info ptl_pmc_dev;
 
  void cnl_suspend(struct pmc_dev *pmcdev);
  int cnl_resume(struct pmc_dev *pmcdev);
+int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
+int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep);
+
+extern const struct file_operations pmc_core_substate_req_regs_fops;
+extern const struct file_operations pmc_core_substate_blk_req_fops;
 
  #define pmc_for_each_mode(mode, pmcdev)						\
  	for (unsigned int __i = 0, __cond;					\

drivers/platform/x86/intel/pmc/lnl.c +2 -0

index e08a77c778c2c..6fa027e7071f4 100644
@@ -574,7 +574,9 @@ struct pmc_dev_info lnl_pmc_dev = {
  	.pci_func = 2,
  	.regmap_list = lnl_pmc_info_list,
  	.map = &lnl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
  	.suspend = cnl_suspend,
  	.resume = lnl_resume,
  	.init = lnl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
  };

drivers/platform/x86/intel/pmc/mtl.c +2 -0

index faa13a7ee688f..0b87e10f864e6 100644
@@ -997,7 +997,9 @@ struct pmc_dev_info mtl_pmc_dev = {
  	.dmu_guid = MTL_PMT_DMU_GUID,
  	.regmap_list = mtl_pmc_info_list,
  	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
  	.suspend = cnl_suspend,
  	.resume = mtl_resume,
  	.init = mtl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
  };

Russell Haley

2025-09-01 23:52 UTC #9 lore raw

Re: [PATCH v3 4/6] platform/x86:intel/pmc: Show substate requirement for S0ix blockers

In reply to Xi Pardee

Quoted 36 lines · Xi Pardee

Add support to read and show S0ix blocker substate requirements. Starting from Panther Lake, substate requirement data is provided based on S0ix blockers instead of all low power mode requirements. For platforms that support this new feature, add support to display substate requirements based on S0ix blockers.
Change the "substate_requirements" attribute of Intel PMC Core driver to show the substate requirements for each S0ix blocker and the corresponding S0ix blocker value.
Signed-off-by Xi Pardee <[email protected]>
---
 drivers/platform/x86/intel/pmc/arl.c  |   4 +
 drivers/platform/x86/intel/pmc/core.c | 126 ++++++++++++++++++++++++--
 drivers/platform/x86/intel/pmc/core.h |  14 +++
 drivers/platform/x86/intel/pmc/lnl.c  |   2 +
 drivers/platform/x86/intel/pmc/mtl.c  |   2 +
 5 files changed, 138 insertions(+), 10 deletions(-)
drivers/platform/x86/intel/pmc/arl.c +2 -0
index 9d66d65e75779..17ad87b392abe 100644
@@ -725,9 +725,11 @@ struct pmc_dev_info arl_pmc_dev = {
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &arl_socs_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = arl_resume,
 	.init = arl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };
 
 struct pmc_dev_info arl_h_pmc_dev = {

drivers/platform/x86/intel/pmc/arl.c struct pmc_dev_info arl_h_pmc_dev = { · line 745

Quoted 9 lines

drivers/platform/x86/intel/pmc/arl.c +2 -0

@@ -735,7 +737,9 @@ struct pmc_dev_info arl_h_pmc_dev = {
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
 	.suspend = cnl_suspend,
 	.resume = arl_h_resume,
 	.init = arl_h_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
 };

The cover letter and commit messages say Panther Lake, but I see a bunch of "arl" here. Is this just confusing file and struct names, or is my Arrow Lake 265K getting idle-blocker-cause debug info? If so it should probably be mentioned in the changelog.

Same question about patch 5.

- Russell Haley

Xi Pardee

author maintainer

2025-09-05 18:43 UTC #10 lore raw

Re: [PATCH v3 4/6] platform/x86:intel/pmc: Show substate requirement for S0ix blockers

Hi Russell,

Thanks for the review.

Quoted 51 lines · Russell Haley

Quoted 36 lines · Xi Pardee
Add support to read and show S0ix blocker substate requirements. Starting from Panther Lake, substate requirement data is provided based on S0ix blockers instead of all low power mode requirements. For platforms that support this new feature, add support to display substate requirements based on S0ix blockers.
Change the "substate_requirements" attribute of Intel PMC Core driver to show the substate requirements for each S0ix blocker and the corresponding S0ix blocker value.
Signed-off-by Xi Pardee <[email protected]>
---
  drivers/platform/x86/intel/pmc/arl.c  |   4 +
  drivers/platform/x86/intel/pmc/core.c | 126 ++++++++++++++++++++++++--
  drivers/platform/x86/intel/pmc/core.h |  14 +++
  drivers/platform/x86/intel/pmc/lnl.c  |   2 +
  drivers/platform/x86/intel/pmc/mtl.c  |   2 +
  5 files changed, 138 insertions(+), 10 deletions(-)
drivers/platform/x86/intel/pmc/arl.c +2 -0
index 9d66d65e75779..17ad87b392abe 100644
@@ -725,9 +725,11 @@ struct pmc_dev_info arl_pmc_dev = {
  	.dmu_guid = ARL_PMT_DMU_GUID,
  	.regmap_list = arl_pmc_info_list,
  	.map = &arl_socs_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
  	.suspend = cnl_suspend,
  	.resume = arl_resume,
  	.init = arl_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
  };
 
  struct pmc_dev_info arl_h_pmc_dev = {
drivers/platform/x86/intel/pmc/arl.c struct pmc_dev_info arl_h_pmc_dev = { · line 745
Quoted 9 lines
drivers/platform/x86/intel/pmc/arl.c +2 -0
@@ -735,7 +737,9 @@ struct pmc_dev_info arl_h_pmc_dev = {
  	.dmu_guid = ARL_PMT_DMU_GUID,
  	.regmap_list = arl_pmc_info_list,
  	.map = &mtl_socm_reg_map,
+	.sub_req_show = &pmc_core_substate_req_regs_fops,
  	.suspend = cnl_suspend,
  	.resume = arl_h_resume,
  	.init = arl_h_core_init,
+	.sub_req = pmc_core_pmt_get_lpm_req,
  };
The cover letter and commit messages say Panther Lake, but I see a bunch of "arl" here. Is this just confusing file and struct names, or is my Arrow Lake 265K getting idle-blocker-cause debug info? If so it should probably be mentioned in the changelog.

Quoted 1 line

Same question about patch 5.

This patch series enables SSRAM supports of Lunar Lake and Panther Lake platforms in intel_pmc_core driver. It would not modify the functionality of intel_pmc_core driver in Arrow Lake platforms. Arrow Lake would not get s0ix_blocker information with this patch series.

Starting from Panther Lake, the "substate_requirements" attribute in the newer platforms will show only the requirements of s0ix blockers instead of every substate status register. In order to support these two mutual exclusive features, additional fields have been added to pmc_dev_info struct. This change would not change the functionality of intel_pmc_core driver in Arrow Lake platforms.

Thanks for the suggestion, I will add this information to the changelog of next version.

Thanks!

Quoted 3 lines

- Russell Haley

Xi Pardee

author maintainer

2025-08-15 22:46 UTC #11 lore raw

PATCH v3 5/6 platform/x86:intel/pmc: Show device and function number

In reply to Xi Pardee

Add support to show device and function number for S0ix blockers. This feature depends on S0ix blocker substate requirement table and BDF association table. This feature is available for platforms starting from Pather Lake.

Only a subset of S0ix blockers has device and function number associated to it. Get the availability information from the substate requirement table. Get the device and function number mapping information for each S0ix blocker from the BDF association table.

Signed-off-by Xi Pardee <[email protected]>

---
 drivers/platform/x86/intel/pmc/core.c | 182 +++++++++++++++++++++++++-
 drivers/platform/x86/intel/pmc/core.h |  23 +++-
 2 files changed, 203 insertions(+), 2 deletions(-)

drivers/platform/x86/intel/pmc/core.c +181 -1

index a0b948a875a5a..69ee40cbb8b8a 100644
@@ -1017,6 +1017,38 @@ const struct file_operations pmc_core_substate_req_regs_fops = {
 	.release	= single_release,
 };
 
+static int pmc_core_bdf_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	unsigned int pmcidx;
+
+	seq_printf(s, "%36s | %15s | %15s |\n", "Element", "Device Number", "Function Number");
+	for (pmcidx = 0; pmcidx < ARRAY_SIZE(pmcdev->pmcs); pmcidx++) {
+		const char *name = NULL;
+		struct list_head *cur;
+		struct bdf_entry *bdf;
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmcidx];
+		if (!pmc)
+			continue;
+
+		list_for_each(cur, pmc->bdf_list) {
+			bdf = list_entry(cur, struct bdf_entry, node);
+			if (bdf->name != name) {
+				seq_printf(s, "pmc%d: %30s | %15x | %15x |\n", pmcidx,
+					   bdf->name, bdf->dev_num, bdf->fun_num);
+				name = bdf->name;
+			} else {
+				seq_printf(s, "%54x | %15x |\n",
+					   bdf->dev_num, bdf->fun_num);
+			}
+		}
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_bdf);
+
 static unsigned int pmc_core_get_crystal_freq(void)
 {
 	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;

@@ -1418,6 +1450,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info
 				    pmc_dev_info->sub_req_show);
 	}
 
+	if (primary_pmc->bdf_list) {
+		debugfs_create_file("bdf", 0444, pmcdev->dbgfs_dir, pmcdev, &pmc_core_bdf_fops);
+	}
+
 	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
 		debugfs_create_file("pson_residency_usec", 0444,
 				    pmcdev->dbgfs_dir, primary_pmc, &pmc_core_pson_residency);

@@ -1521,7 +1557,7 @@ int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct tel
 	return ret;
 }
 
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+static int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
 				 struct telem_endpoint *ep)
 {
 	u32 num_blocker, sample_id;

@@ -1551,6 +1587,150 @@ int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
 	return 0;
 }
 
+static const char *pmc_core_get_next_bdf_ip_name(struct pmc *pmc, unsigned int *r_idx,
+						 unsigned int *i_idx, u32 **lpm_req_regs)
+{
+	const struct pmc_bit_map **maps;
+	unsigned int arr_size;
+	bool reset = FALSE;
+
+	maps = pmc->map->s0ix_blocker_maps;
+	arr_size = pmc_core_lpm_get_arr_size(maps);
+
+	// Iteration reaches the end of the bitmap array
+	if (!maps[*r_idx][*i_idx].name)
+		(*r_idx)++;
+
+	// Iteration reaches the end of the maps
+	if (*r_idx >= arr_size)
+		return NULL;
+
+	for (; *r_idx < arr_size; (*r_idx)++) {
+		const char *ip_name;
+
+		if (reset)
+			*i_idx = 0;
+
+		for (; maps[*r_idx][*i_idx].name; reset = TRUE, (*i_idx)++) {
+			if (!maps[*r_idx][*i_idx].blk)
+				continue;
+
+			bool exist = **lpm_req_regs & BIT(BDF_EXIST_BIT);
+			(*lpm_req_regs)++;
+			if (exist) {
+				ip_name = maps[*r_idx][*i_idx].name;
+				(*i_idx)++;
+				return ip_name;
+			}
+		}
+	}
+	return NULL;
+}
+
+static int pmc_core_process_bdf(struct pmc_dev *pmcdev,  struct pmc *pmc, u32 data,
+				unsigned int *r_idx, unsigned int *i_idx, u32 **lpm_req_regs,
+				const char **name)
+{
+	unsigned int i;
+
+	if (!data)
+		return 0;
+
+	if (!*name)
+		return -EINVAL;
+
+	for (i = BDF_FUN_LOW_BIT; i <= BDF_FUN_HIGH_BIT; i++) {
+		struct bdf_entry *b_entry;
+		u32 function_data;
+
+		function_data = (data & BIT(i));
+		if (function_data) {
+			b_entry = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*b_entry), GFP_KERNEL);
+			if (!b_entry)
+				return -ENOMEM;
+			b_entry->dev_num = data & GENMASK(BDF_DEV_HIGH_BIT, BDF_DEV_LOW_BIT);
+			b_entry->fun_num = i - BDF_FUN_LOW_BIT;
+			b_entry->name = *name;
+			list_add_tail(&b_entry->node, pmc->bdf_list);
+		}
+	}
+
+	if (!(data & BIT(BDF_REQ_BIT)))
+		*name = pmc_core_get_next_bdf_ip_name(pmc, r_idx, i_idx, lpm_req_regs);
+
+	return 0;
+}
+
+static int pmc_core_pmt_get_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
+{
+	unsigned int sample_id, max_sample_id, header_id, size, r_idx, i_idx;
+	struct bdf_entry *entry;
+	u32 *lpm_reg_regs;
+	const char *name;
+	int ret;
+
+	header_id = pmc->map->bdf_offset;
+	sample_id = header_id;
+	max_sample_id = sample_id + pmc->map->bdf_table_size;
+	lpm_reg_regs = pmc->lpm_req_regs;
+	r_idx = 0;
+	i_idx = 0;
+
+	name = pmc_core_get_next_bdf_ip_name(pmc, &r_idx, &i_idx, &lpm_reg_regs);
+	if (!name)
+		return -EINVAL;
+
+	pmc->bdf_list = devm_kzalloc(&pmcdev->pdev->dev, sizeof(struct list_head), GFP_KERNEL);
+	if (!pmc->bdf_list)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(pmc->bdf_list);
+
+	for (; sample_id < max_sample_id; sample_id++) {
+		u32 data;
+
+		ret = pmt_telem_read32(ep, sample_id, &data, 1);
+		if (ret) {
+			dev_err(&pmcdev->pdev->dev,
+				"couldn't read bdf: %d\n", ret);
+			return ret;
+		}
+
+		if (sample_id == header_id) {
+			size = (data & GENMASK(BDF_SIZE_HIGH_BIT, BDF_SIZE_LOW_BIT))
+			       >> BDF_SIZE_LOW_BIT;
+			header_id += size + 1;
+			continue;
+		}
+
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+		data = data >> BDF_SIZE;
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+	}
+
+	list_for_each_entry(entry, pmc->bdf_list, node) {
+		dev_dbg(&pmcdev->pdev->dev, "bdf info: name %s, dev_num %x, fun_num %x",
+			entry->name, entry->dev_num, entry->fun_num);
+	}
+	return 0;
+}
+
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep)
+{
+	int ret;
+
+	ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
+	if (ret)
+		return ret;
+
+	return pmc_core_pmt_get_bdf(pmcdev, pmc, ep);
+}
+
 static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
 	struct pci_dev *pcidev __free(pci_dev_put) = NULL;

drivers/platform/x86/intel/pmc/core.h +22 -1

index bfe8fba808063..6ff2d171dc2ba 100644
@@ -317,6 +317,24 @@ enum ppfear_regs {
 #define PMC_DEVID_MTL_IOEP	0x7ecf
 #define PMC_DEVID_MTL_IOEM	0x7ebf
 
+/* BDF offset */
+#define BDF_EXIST_BIT		3
+#define BDF_SIZE_HIGH_BIT	23
+#define BDF_SIZE_LOW_BIT	16
+#define BDF_DEV_HIGH_BIT	4
+#define BDF_DEV_LOW_BIT		0
+#define BDF_FUN_HIGH_BIT	12
+#define BDF_FUN_LOW_BIT		5
+#define BDF_REQ_BIT		15
+#define BDF_SIZE		16
+
+struct bdf_entry {
+	struct list_head node;
+	const char *name;
+	u32 dev_num;
+	u32 fun_num;
+};
+
 extern const char *pmc_lpm_modes[];
 
 struct pmc_bit_map {

@@ -373,6 +391,8 @@ struct pmc_reg_map {
 	const u32 s0ix_blocker_offset;
 	const u32 num_s0ix_blocker;
 	const u32 blocker_req_offset;
+	const u32 bdf_offset;
+	const u32 bdf_table_size;
 	/* Low Power Mode registers */
 	const int lpm_num_maps;
 	const int lpm_num_modes;

@@ -418,6 +438,7 @@ struct pmc {
 	const struct pmc_reg_map *map;
 	u32 *lpm_req_regs;
 	u32 ltr_ign;
+	struct list_head *bdf_list;
 };
 
 /**

@@ -540,7 +561,7 @@ extern struct pmc_dev_info ptl_pmc_dev;
 void cnl_suspend(struct pmc_dev *pmcdev);
 int cnl_resume(struct pmc_dev *pmcdev);
 int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
 				 struct telem_endpoint *ep);
 
 extern const struct file_operations pmc_core_substate_req_regs_fops;

Signature · 1 line

2.43.0

Ilpo Järvinen

maintainer

2025-08-28 13:56 UTC #12 lore raw

Re: [PATCH v3 5/6] platform/x86:intel/pmc: Show device and function number

Quoted 21 lines · Xi Pardee

Add support to show device and function number for S0ix blockers. This feature depends on S0ix blocker substate requirement table and BDF association table. This feature is available for platforms starting from Pather Lake.
Only a subset of S0ix blockers has device and function number associated to it. Get the availability information from the substate requirement table. Get the device and function number mapping information for each S0ix blocker from the BDF association table.
Signed-off-by Xi Pardee <[email protected]>
---
 drivers/platform/x86/intel/pmc/core.c | 182 +++++++++++++++++++++++++-
 drivers/platform/x86/intel/pmc/core.h |  23 +++-
 2 files changed, 203 insertions(+), 2 deletions(-)
drivers/platform/x86/intel/pmc/core.c +0 -0
index a0b948a875a5a..69ee40cbb8b8a 100644

drivers/platform/x86/intel/pmc/core.c const struct file_operations pmc_core_substate_req_regs_fops = { · line 1026

Quoted 10 lines

drivers/platform/x86/intel/pmc/core.c +7 -0

@@ -1017,6 +1017,38 @@ const struct file_operations pmc_core_substate_req_regs_fops = {
 	.release	= single_release,
 };
 
+static int pmc_core_bdf_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	unsigned int pmcidx;
+
+	seq_printf(s, "%36s | %15s | %15s |\n", "Element", "Device Number", "Function Number");
+	for (pmcidx = 0; pmcidx < ARRAY_SIZE(pmcdev->pmcs); pmcidx++) {

Change this to pmc_idx and lets make it the only form that are added from this point on.

The other ones should be converted to it eventually, I once had a cleanup patch to rename them but IIRC I dropped it to not conflict with some feature worked. Maybe you can fit a rename change into some series so I won't end up conflicting your feature work :-).

drivers/platform/x86/intel/pmc/core.c const struct file_operations pmc_core_substate_req_regs_fops = { · line 1039

Quoted 13 lines

drivers/platform/x86/intel/pmc/core.c +13 -0

+		const char *name = NULL;
+		struct list_head *cur;
+		struct bdf_entry *bdf;
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmcidx];
+		if (!pmc)
+			continue;
+
+		list_for_each(cur, pmc->bdf_list) {
+			bdf = list_entry(cur, struct bdf_entry, node);
+			if (bdf->name != name) {
+				seq_printf(s, "pmc%d: %30s | %15x | %15x |\n", pmcidx,

Quoted 16 lines

drivers/platform/x86/intel/pmc/core.c +12 -0

+					   bdf->name, bdf->dev_num, bdf->fun_num);
+				name = bdf->name;
+			} else {
+				seq_printf(s, "%54x | %15x |\n",
+					   bdf->dev_num, bdf->fun_num);
+			}
+		}
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_bdf);
+
 static unsigned int pmc_core_get_crystal_freq(void)
 {
 	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;

drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info · line 1455

Quoted 6 lines

drivers/platform/x86/intel/pmc/core.c +3 -0

@@ -1418,6 +1450,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info
 				    pmc_dev_info->sub_req_show);
 	}
 
+	if (primary_pmc->bdf_list) {
+		debugfs_create_file("bdf", 0444, pmcdev->dbgfs_dir, pmcdev, &pmc_core_bdf_fops);
+	}

Unnecessary braces.

Quoted 14 lines

drivers/platform/x86/intel/pmc/core.c +2 -1

+
 	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
 		debugfs_create_file("pson_residency_usec", 0444,
 				    pmcdev->dbgfs_dir, primary_pmc, &pmc_core_pson_residency);

@@ -1521,7 +1557,7 @@ int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct tel
 	return ret;
 }
 
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+static int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
 				 struct telem_endpoint *ep)
 {
 	u32 num_blocker, sample_id;

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1595

Quoted 9 lines

drivers/platform/x86/intel/pmc/core.c +6 -0

@@ -1551,6 +1587,150 @@ int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
 	return 0;
 }
 
+static const char *pmc_core_get_next_bdf_ip_name(struct pmc *pmc, unsigned int *r_idx,
+						 unsigned int *i_idx, u32 **lpm_req_regs)
+{
+	const struct pmc_bit_map **maps;
+	unsigned int arr_size;
+	bool reset = FALSE;

FALSE is a define in some obscure header (which you probably didn't include intentionally anyway ;-)).

Please use false.

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1600

Quoted 5 lines

drivers/platform/x86/intel/pmc/core.c +5 -0

+
+	maps = pmc->map->s0ix_blocker_maps;
+	arr_size = pmc_core_lpm_get_arr_size(maps);
+
+	// Iteration reaches the end of the bitmap array

This driver has used exclusively /* */ comments.

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1609

Quoted 9 lines

drivers/platform/x86/intel/pmc/core.c +9 -0

+	if (!maps[*r_idx][*i_idx].name)
+		(*r_idx)++;
+
+	// Iteration reaches the end of the maps
+	if (*r_idx >= arr_size)
+		return NULL;
+
+	for (; *r_idx < arr_size; (*r_idx)++) {
+		const char *ip_name;

Can't you put this to the innermost block?

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1611

Quoted 1 line

drivers/platform/x86/intel/pmc/core.c +1 -0
+		if (reset)

Why you need this?

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1614

Quoted 3 lines

drivers/platform/x86/intel/pmc/core.c +3 -0

+			*i_idx = 0;
+
+		for (; maps[*r_idx][*i_idx].name; reset = TRUE, (*i_idx)++) {

true

This is hard enough to understand even without that "for (;". Would probably be better to use while () instead.

Quoted 4 lines

drivers/platform/x86/intel/pmc/core.c +4 -0

+			if (!maps[*r_idx][*i_idx].blk)
+				continue;
+
+			bool exist = **lpm_req_regs & BIT(BDF_EXIST_BIT);

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1628

Quoted 10 lines

drivers/platform/x86/intel/pmc/core.c +10 -0

+			(*lpm_req_regs)++;
+			if (exist) {
+				ip_name = maps[*r_idx][*i_idx].name;
+				(*i_idx)++;
+				return ip_name;
+			}
+		}
+	}
+	return NULL;
+}

TBH, this entire function is horrible mess, two nested iterators as pointers, etc.

I'm very far from following all what going on here.

I suppose I've not seen this patch previously?

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1642

Quoted 13 lines

drivers/platform/x86/intel/pmc/core.c +13 -0

+static int pmc_core_process_bdf(struct pmc_dev *pmcdev,  struct pmc *pmc, u32 data,
+				unsigned int *r_idx, unsigned int *i_idx, u32 **lpm_req_regs,
+				const char **name)
+{
+	unsigned int i;
+
+	if (!data)
+		return 0;
+
+	if (!*name)
+		return -EINVAL;
+
+	for (i = BDF_FUN_LOW_BIT; i <= BDF_FUN_HIGH_BIT; i++) {

I think you can iterate 0 ... __fls(FIELD_MAX()).

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1647

Quoted 5 lines

drivers/platform/x86/intel/pmc/core.c +5 -0

+		struct bdf_entry *b_entry;
+		u32 function_data;
+
+		function_data = (data & BIT(i));
+		if (function_data) {

Why the extra variable???

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1652

Quoted 5 lines

drivers/platform/x86/intel/pmc/core.c +5 -0

+			b_entry = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*b_entry), GFP_KERNEL);
+			if (!b_entry)
+				return -ENOMEM;
+			b_entry->dev_num = data & GENMASK(BDF_DEV_HIGH_BIT, BDF_DEV_LOW_BIT);
+			b_entry->fun_num = i - BDF_FUN_LOW_BIT;

What "fun" stands for? Should it be "func" as is the typical short for "function" in BDF?

Quoted 21 lines

drivers/platform/x86/intel/pmc/core.c +21 -0

+			b_entry->name = *name;
+			list_add_tail(&b_entry->node, pmc->bdf_list);
+		}
+	}
+
+	if (!(data & BIT(BDF_REQ_BIT)))
+		*name = pmc_core_get_next_bdf_ip_name(pmc, r_idx, i_idx, lpm_req_regs);
+
+	return 0;
+}
+
+static int pmc_core_pmt_get_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
+{
+	unsigned int sample_id, max_sample_id, header_id, size, r_idx, i_idx;
+	struct bdf_entry *entry;
+	u32 *lpm_reg_regs;
+	const char *name;
+	int ret;
+
+	header_id = pmc->map->bdf_offset;
+	sample_id = header_id;

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1683

Quoted 10 lines

drivers/platform/x86/intel/pmc/core.c +10 -0

+	max_sample_id = sample_id + pmc->map->bdf_table_size;
+	lpm_reg_regs = pmc->lpm_req_regs;
+	r_idx = 0;
+	i_idx = 0;
+
+	name = pmc_core_get_next_bdf_ip_name(pmc, &r_idx, &i_idx, &lpm_reg_regs);
+	if (!name)
+		return -EINVAL;
+
+	pmc->bdf_list = devm_kzalloc(&pmcdev->pdev->dev, sizeof(struct list_head), GFP_KERNEL);

Should use sizeof(*xx).

But why you need to allocate the list head and not have it in place within the pmc's struct?

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1695

Quoted 12 lines

drivers/platform/x86/intel/pmc/core.c +12 -0

+	if (!pmc->bdf_list)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(pmc->bdf_list);
+
+	for (; sample_id < max_sample_id; sample_id++) {
+		u32 data;
+
+		ret = pmt_telem_read32(ep, sample_id, &data, 1);
+		if (ret) {
+			dev_err(&pmcdev->pdev->dev,
+				"couldn't read bdf: %d\n", ret);

One line.

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1701

Quoted 6 lines

drivers/platform/x86/intel/pmc/core.c +6 -0

+			return ret;
+		}
+
+		if (sample_id == header_id) {
+			size = (data & GENMASK(BDF_SIZE_HIGH_BIT, BDF_SIZE_LOW_BIT))
+			       >> BDF_SIZE_LOW_BIT;

Define the field and use FIELD_GET().

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1702

Quoted 1 line

drivers/platform/x86/intel/pmc/core.c +1 -0
+			header_id += size + 1;

No, I just cannot understand what's going on here, it's hopeless. Always when I think I've finally understood what its all about you throw a curve ball like this.

In case this series is in any kind of hurry. I suggest you send the series without this patch and we work out this patch separately on top of the applied patches (I expect the patch 1-5 to be fine on next iteration).

Quoted 42 lines

drivers/platform/x86/intel/pmc/core.c +31 -0

+			continue;
+		}
+
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+		data = data >> BDF_SIZE;
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+	}
+
+	list_for_each_entry(entry, pmc->bdf_list, node) {
+		dev_dbg(&pmcdev->pdev->dev, "bdf info: name %s, dev_num %x, fun_num %x",
+			entry->name, entry->dev_num, entry->fun_num);
+	}
+	return 0;
+}
+
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep)
+{
+	int ret;
+
+	ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
+	if (ret)
+		return ret;
+
+	return pmc_core_pmt_get_bdf(pmcdev, pmc, ep);
+}
+
 static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
 	struct pci_dev *pcidev __free(pci_dev_put) = NULL;

drivers/platform/x86/intel/pmc/core.h +0 -0

index bfe8fba808063..6ff2d171dc2ba 100644
@@ -317,6 +317,24 @@ enum ppfear_regs {
 #define PMC_DEVID_MTL_IOEP	0x7ecf
 #define PMC_DEVID_MTL_IOEM	0x7ebf

drivers/platform/x86/intel/pmc/core.h enum ppfear_regs { · line 329

Quoted 10 lines

drivers/platform/x86/intel/pmc/core.h +10 -0

+/* BDF offset */
+#define BDF_EXIST_BIT		3
+#define BDF_SIZE_HIGH_BIT	23
+#define BDF_SIZE_LOW_BIT	16
+#define BDF_DEV_HIGH_BIT	4
+#define BDF_DEV_LOW_BIT		0
+#define BDF_FUN_HIGH_BIT	12
+#define BDF_FUN_LOW_BIT		5
+#define BDF_REQ_BIT		15
+#define BDF_SIZE		16

Use BIT(), GENMASK() for most right here. All?

Quoted 38 lines

drivers/platform/x86/intel/pmc/core.h +12 -1

+
+struct bdf_entry {
+	struct list_head node;
+	const char *name;
+	u32 dev_num;
+	u32 fun_num;
+};
+
 extern const char *pmc_lpm_modes[];
 
 struct pmc_bit_map {

@@ -373,6 +391,8 @@ struct pmc_reg_map {
 	const u32 s0ix_blocker_offset;
 	const u32 num_s0ix_blocker;
 	const u32 blocker_req_offset;
+	const u32 bdf_offset;
+	const u32 bdf_table_size;
 	/* Low Power Mode registers */
 	const int lpm_num_maps;
 	const int lpm_num_modes;

@@ -418,6 +438,7 @@ struct pmc {
 	const struct pmc_reg_map *map;
 	u32 *lpm_req_regs;
 	u32 ltr_ign;
+	struct list_head *bdf_list;
 };
 
 /**

@@ -540,7 +561,7 @@ extern struct pmc_dev_info ptl_pmc_dev;
 void cnl_suspend(struct pmc_dev *pmcdev);
 int cnl_resume(struct pmc_dev *pmcdev);
 int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
 				 struct telem_endpoint *ep);
 
 extern const struct file_operations pmc_core_substate_req_regs_fops;

Signature · 1 line

i.

Xi Pardee

author maintainer

2025-09-07 21:57 UTC #13 lore raw

Re: [PATCH v3 5/6] platform/x86:intel/pmc: Show device and function number

Hi Ilpo,

Thanks for the review.

Quoted 36 lines · Ilpo Järvinen

Quoted 21 lines · Xi Pardee
Add support to show device and function number for S0ix blockers. This feature depends on S0ix blocker substate requirement table and BDF association table. This feature is available for platforms starting from Pather Lake.
Only a subset of S0ix blockers has device and function number associated to it. Get the availability information from the substate requirement table. Get the device and function number mapping information for each S0ix blocker from the BDF association table.
Signed-off-by Xi Pardee <[email protected]>
---
  drivers/platform/x86/intel/pmc/core.c | 182 +++++++++++++++++++++++++-
  drivers/platform/x86/intel/pmc/core.h |  23 +++-
  2 files changed, 203 insertions(+), 2 deletions(-)
drivers/platform/x86/intel/pmc/core.c +0 -0
index a0b948a875a5a..69ee40cbb8b8a 100644
drivers/platform/x86/intel/pmc/core.c const struct file_operations pmc_core_substate_req_regs_fops = { · line 1026
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +7 -0
@@ -1017,6 +1017,38 @@ const struct file_operations pmc_core_substate_req_regs_fops = {
  	.release	= single_release,
  };
 
+static int pmc_core_bdf_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	unsigned int pmcidx;
+
+	seq_printf(s, "%36s | %15s | %15s |\n", "Element", "Device Number", "Function Number");
+	for (pmcidx = 0; pmcidx < ARRAY_SIZE(pmcdev->pmcs); pmcidx++) {
Change this to pmc_idx and lets make it the only form that are added from this point on.

Quoted 4 lines

The other ones should be converted to it eventually, I once had a cleanup patch to rename them but IIRC I dropped it to not conflict with some feature worked. Maybe you can fit a rename change into some series so I won't end up conflicting your feature work :-).

I will remove this patch from this patch series and send this one separately later and include a cleanup patch to rename all pmc index to pmc_idx.

Quoted 15 lines

Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +3 -0
+		const char *name = NULL;
+		struct list_head *cur;
+		struct bdf_entry *bdf;
drivers/platform/x86/intel/pmc/core.c const struct file_operations pmc_core_substate_req_regs_fops = { · line 1039
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10 -0
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmcidx];
+		if (!pmc)
+			continue;
+
+		list_for_each(cur, pmc->bdf_list) {
+			bdf = list_entry(cur, struct bdf_entry, node);
+			if (bdf->name != name) {
+				seq_printf(s, "pmc%d: %30s | %15x | %15x |\n", pmcidx,
%u

Will change it in next version.

Quoted 20 lines

drivers/platform/x86/intel/pmc/core.c +12 -0

+					   bdf->name, bdf->dev_num, bdf->fun_num);
+				name = bdf->name;
+			} else {
+				seq_printf(s, "%54x | %15x |\n",
+					   bdf->dev_num, bdf->fun_num);
+			}
+		}
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_bdf);
+
  static unsigned int pmc_core_get_crystal_freq(void)
  {
  	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;

@@ -1418,6 +1450,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info
  				    pmc_dev_info->sub_req_show);
  	}

Quoted 4 lines

drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info · line 1455
Quoted 3 lines
drivers/platform/x86/intel/pmc/core.c +3 -0
+	if (primary_pmc->bdf_list) {
+		debugfs_create_file("bdf", 0444, pmcdev->dbgfs_dir, pmcdev, &pmc_core_bdf_fops);
+	}
Unnecessary braces.

Will change it in next version.

Quoted 27 lines

Quoted 15 lines

drivers/platform/x86/intel/pmc/core.c +2 -1

+
  	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
  		debugfs_create_file("pson_residency_usec", 0444,
  				    pmcdev->dbgfs_dir, primary_pmc, &pmc_core_pson_residency);

@@ -1521,7 +1557,7 @@ int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct tel
  	return ret;
  }
 
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+static int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
  				 struct telem_endpoint *ep)
  {
  	u32 num_blocker, sample_id;

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1595

Quoted 9 lines

drivers/platform/x86/intel/pmc/core.c +6 -0

@@ -1551,6 +1587,150 @@ int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
  	return 0;
  }
 
+static const char *pmc_core_get_next_bdf_ip_name(struct pmc *pmc, unsigned int *r_idx,
+						 unsigned int *i_idx, u32 **lpm_req_regs)
+{
+	const struct pmc_bit_map **maps;
+	unsigned int arr_size;
+	bool reset = FALSE;

FALSE is a define in some obscure header (which you probably didn't include intentionally anyway ;-)).

Quoted 1 line

Please use false.

Will change it to false in next version.

Quoted 7 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1600
Quoted 6 lines
drivers/platform/x86/intel/pmc/core.c +5 -0
+
+	maps = pmc->map->s0ix_blocker_maps;
+	arr_size = pmc_core_lpm_get_arr_size(maps);
+
+	// Iteration reaches the end of the bitmap array
This driver has used exclusively /* */ comments.

Will change it in next version.

Quoted 11 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1609
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +9 -0
+	if (!maps[*r_idx][*i_idx].name)
+		(*r_idx)++;
+
+	// Iteration reaches the end of the maps
+	if (*r_idx >= arr_size)
+		return NULL;
+
+	for (; *r_idx < arr_size; (*r_idx)++) {
+		const char *ip_name;
Can't you put this to the innermost block?

Will move it in next version.

Quoted 3 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1611
Quoted 2 lines
drivers/platform/x86/intel/pmc/core.c +1 -0
+		if (reset)
Why you need this?

The purpose of this function is to return the name of the NEXT s0ix blocker with BDF information.

r_idx and i_idx are used to keep track of the current position of the iteration, therefore i_idx could not be reset to 0 at the first run of the inner for loop. After the first run of inner for loop reset should be set to true so in next run of the outer for loop i_idx could be reset to 0 (which mean the iteration reaches the next s0ix blocker map).

Quoted 8 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1614
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +3 -0
+			*i_idx = 0;
+
+		for (; maps[*r_idx][*i_idx].name; reset = TRUE, (*i_idx)++) {
true
This is hard enough to understand even without that "for (;". Would probably be better to use while () instead.

Will change to while loop in next version.

Quoted 17 lines

Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +4 -0
+			if (!maps[*r_idx][*i_idx].blk)
+				continue;
+
+			bool exist = **lpm_req_regs & BIT(BDF_EXIST_BIT);
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1628
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10 -0
+			(*lpm_req_regs)++;
+			if (exist) {
+				ip_name = maps[*r_idx][*i_idx].name;
+				(*i_idx)++;
+				return ip_name;
+			}
+		}
+	}
+	return NULL;
+}
TBH, this entire function is horrible mess, two nested iterators as pointers, etc.

Quoted 3 lines

I'm very far from following all what going on here.
I suppose I've not seen this patch previously?

To achieve the NEXT name of the s0ix blocker with BDF information we need to iterate through two (set of) maps in parallel. The s0ix_blocker_maps provide names of all s0ix blockers and the lpm_req_regs map shows that which s0ix blocker has associated BDF information.

if maps[*r_idx][*i_idx].blk is set, that means it is a s0ix blocker. For each s0ix blocker, if **lpm_req_regs & BIT(BDF_EXIST_BIT) is set, that means this blocker has associated BDF information. Pointers are used to keep track of the current position of the two (set of) maps so the function provides the NEXT name of the s0ix blocker with associated BDF info.

I will probably switch to use a temporary data structure, such as list, to store all s0ix blockers with BDF info and then iterate through this list in pmc_core_process_bdf() call instead. That will make the logic easier to follow and maintain. I will also add more comments to next version of this patch.

Quoted 15 lines

Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +3 -0
+static int pmc_core_process_bdf(struct pmc_dev *pmcdev,  struct pmc *pmc, u32 data,
+				unsigned int *r_idx, unsigned int *i_idx, u32 **lpm_req_regs,
+				const char **name)
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1642
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10 -0
+{
+	unsigned int i;
+
+	if (!data)
+		return 0;
+
+	if (!*name)
+		return -EINVAL;
+
+	for (i = BDF_FUN_LOW_BIT; i <= BDF_FUN_HIGH_BIT; i++) {
I think you can iterate 0 ... __fls(FIELD_MAX()).

Each 16 bit represents one device and the associated function numbers for one s0ix blocker.

Bit 4-0 indicates the device number.

Bit 12-5 indicates function numbers.

Bit 15 indicates if the next 16 bit is associated to the same s0ix blocker as the current word.

Between bit 12 and bit 5, each bit position represents one function number. Bit 5 represents function 0 and bit 12 represents function 7. I will add more comments in the next version.

Will change to use __fls(FIELD_MAX()) in next version.

Quoted 7 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1647
Quoted 6 lines
drivers/platform/x86/intel/pmc/core.c +5 -0
+		struct bdf_entry *b_entry;
+		u32 function_data;
+
+		function_data = (data & BIT(i));
+		if (function_data) {
Why the extra variable???

Will remove the function_data variable in next version.

Quoted 8 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1652
Quoted 6 lines
drivers/platform/x86/intel/pmc/core.c +5 -0
+			b_entry = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*b_entry), GFP_KERNEL);
+			if (!b_entry)
+				return -ENOMEM;
+			b_entry->dev_num = data & GENMASK(BDF_DEV_HIGH_BIT, BDF_DEV_LOW_BIT);
+			b_entry->fun_num = i - BDF_FUN_LOW_BIT;
What "fun" stands for? Should it be "func" as is the typical short for "function" in BDF?

Will change to func_num in next version.

Quoted 34 lines

Quoted 22 lines

drivers/platform/x86/intel/pmc/core.c +21 -0

+			b_entry->name = *name;
+			list_add_tail(&b_entry->node, pmc->bdf_list);
+		}
+	}
+
+	if (!(data & BIT(BDF_REQ_BIT)))
+		*name = pmc_core_get_next_bdf_ip_name(pmc, r_idx, i_idx, lpm_req_regs);
+
+	return 0;
+}
+
+static int pmc_core_pmt_get_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
+{
+	unsigned int sample_id, max_sample_id, header_id, size, r_idx, i_idx;
+	struct bdf_entry *entry;
+	u32 *lpm_reg_regs;
+	const char *name;
+	int ret;
+
+	header_id = pmc->map->bdf_offset;
+	sample_id = header_id;

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1683

Quoted 10 lines

drivers/platform/x86/intel/pmc/core.c +10 -0

+	max_sample_id = sample_id + pmc->map->bdf_table_size;
+	lpm_reg_regs = pmc->lpm_req_regs;
+	r_idx = 0;
+	i_idx = 0;
+
+	name = pmc_core_get_next_bdf_ip_name(pmc, &r_idx, &i_idx, &lpm_reg_regs);
+	if (!name)
+		return -EINVAL;
+
+	pmc->bdf_list = devm_kzalloc(&pmcdev->pdev->dev, sizeof(struct list_head), GFP_KERNEL);

Should use sizeof(*xx).

Quoted 2 lines

But why you need to allocate the list head and not have it in place within the pmc's struct?

The memory for bdf_list is only allocated when the bdf information is available to achieve.

intel_pmc_core driver can check if the memory is allocated to decide whether to create a file in debugfs for bdf in pmc_core_dbgfs_register().

Quoted 14 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1695
Quoted 13 lines
drivers/platform/x86/intel/pmc/core.c +12 -0
+	if (!pmc->bdf_list)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(pmc->bdf_list);
+
+	for (; sample_id < max_sample_id; sample_id++) {
+		u32 data;
+
+		ret = pmt_telem_read32(ep, sample_id, &data, 1);
+		if (ret) {
+			dev_err(&pmcdev->pdev->dev,
+				"couldn't read bdf: %d\n", ret);
One line.

Will change it in next version.

Quoted 8 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1701
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +6 -0
+			return ret;
+		}
+
+		if (sample_id == header_id) {
+			size = (data & GENMASK(BDF_SIZE_HIGH_BIT, BDF_SIZE_LOW_BIT))
+			       >> BDF_SIZE_LOW_BIT;
Define the field and use FIELD_GET().

Will change it in next version.

Quoted 5 lines

drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1702
Quoted 2 lines
drivers/platform/x86/intel/pmc/core.c +1 -0
+			header_id += size + 1;
No, I just cannot understand what's going on here, it's hopeless. Always when I think I've finally understood what its all about you throw a curve ball like this.

There is one header line (32 bit) between each type of s0ix blocker in the bdf association table. The bit 23-26 in header line indicates the size of each section of one specific type of s0ix blocker in this table.

header_id is used to keep track of the id of each header so we will process the header line differently from the other lines.

I will add more detailed comment in next version.

Quoted 4 lines

In case this series is in any kind of hurry. I suggest you send the series without this patch and we work out this patch separately on top of the applied patches (I expect the patch 1-5 to be fine on next iteration).

I will send this patch separately from the other patches in this series.

Quoted 43 lines

drivers/platform/x86/intel/pmc/core.c +31 -0

+			continue;
+		}
+
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+		data = data >> BDF_SIZE;
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+	}
+
+	list_for_each_entry(entry, pmc->bdf_list, node) {
+		dev_dbg(&pmcdev->pdev->dev, "bdf info: name %s, dev_num %x, fun_num %x",
+			entry->name, entry->dev_num, entry->fun_num);
+	}
+	return 0;
+}
+
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep)
+{
+	int ret;
+
+	ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
+	if (ret)
+		return ret;
+
+	return pmc_core_pmt_get_bdf(pmcdev, pmc, ep);
+}
+
  static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
  {
  	struct pci_dev *pcidev __free(pci_dev_put) = NULL;

drivers/platform/x86/intel/pmc/core.h +0 -0

index bfe8fba808063..6ff2d171dc2ba 100644
@@ -317,6 +317,24 @@ enum ppfear_regs {
  #define PMC_DEVID_MTL_IOEP	0x7ecf
  #define PMC_DEVID_MTL_IOEM	0x7ebf

Quoted 11 lines

drivers/platform/x86/intel/pmc/core.h enum ppfear_regs { · line 329
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.h +10 -0
+/* BDF offset */
+#define BDF_EXIST_BIT		3
+#define BDF_SIZE_HIGH_BIT	23
+#define BDF_SIZE_LOW_BIT	16
+#define BDF_DEV_HIGH_BIT	4
+#define BDF_DEV_LOW_BIT		0
+#define BDF_FUN_HIGH_BIT	12
+#define BDF_FUN_LOW_BIT		5
+#define BDF_REQ_BIT		15
+#define BDF_SIZE		16
Use BIT(), GENMASK() for most right here. All?

Will change to use BIT() and GENMASK() in next version.

Thanks for reviewing the patch.

Quoted 39 lines

drivers/platform/x86/intel/pmc/core.h +12 -1

+
+struct bdf_entry {
+	struct list_head node;
+	const char *name;
+	u32 dev_num;
+	u32 fun_num;
+};
+
  extern const char *pmc_lpm_modes[];
 
  struct pmc_bit_map {

@@ -373,6 +391,8 @@ struct pmc_reg_map {
  	const u32 s0ix_blocker_offset;
  	const u32 num_s0ix_blocker;
  	const u32 blocker_req_offset;
+	const u32 bdf_offset;
+	const u32 bdf_table_size;
  	/* Low Power Mode registers */
  	const int lpm_num_maps;
  	const int lpm_num_modes;

@@ -418,6 +438,7 @@ struct pmc {
  	const struct pmc_reg_map *map;
  	u32 *lpm_req_regs;
  	u32 ltr_ign;
+	struct list_head *bdf_list;
  };
 
  /**

@@ -540,7 +561,7 @@ extern struct pmc_dev_info ptl_pmc_dev;
  void cnl_suspend(struct pmc_dev *pmcdev);
  int cnl_resume(struct pmc_dev *pmcdev);
  int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
  				 struct telem_endpoint *ep);
 
  extern const struct file_operations pmc_core_substate_req_regs_fops;

Ilpo Järvinen

maintainer

2025-09-08 11:03 UTC #14 lore raw

Re: [PATCH v3 5/6] platform/x86:intel/pmc: Show device and function number

Quoted 45 lines · Xi Pardee

Hi Ilpo,
Thanks for the review.
Quoted 25 lines · Ilpo Järvinen
Quoted 21 lines · Xi Pardee
Add support to show device and function number for S0ix blockers. This feature depends on S0ix blocker substate requirement table and BDF association table. This feature is available for platforms starting from Pather Lake.
Only a subset of S0ix blockers has device and function number associated to it. Get the availability information from the substate requirement table. Get the device and function number mapping information for each S0ix blocker from the BDF association table.
Signed-off-by Xi Pardee <[email protected]>
---
  drivers/platform/x86/intel/pmc/core.c | 182 +++++++++++++++++++++++++-
  drivers/platform/x86/intel/pmc/core.h |  23 +++-
  2 files changed, 203 insertions(+), 2 deletions(-)
drivers/platform/x86/intel/pmc/core.c +0 -0
index a0b948a875a5a..69ee40cbb8b8a 100644
Quoted 15 lines
Quoted 13 lines
drivers/platform/x86/intel/pmc/core.c +13 -0
+		const char *name = NULL;
+		struct list_head *cur;
+		struct bdf_entry *bdf;
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmcidx];
+		if (!pmc)
+			continue;
+
+		list_for_each(cur, pmc->bdf_list) {
+			bdf = list_entry(cur, struct bdf_entry, node);
+			if (bdf->name != name) {
+				seq_printf(s, "pmc%d: %30s | %15x | %15x |\n",
Quoted 1 line
pmcidx,
%u

Quoted 1 line

Will change it in next version.

I don't remember if I mentioned it earlier but if you're going to address the review comment fully. There's no need to "ack" them like this. I trust you make the changes you don't contest.

By doing so, we can both save time by only focusing on the points which are contested or need further discussion. :-)

Quoted 15 lines

Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1609
Quoted 9 lines
drivers/platform/x86/intel/pmc/core.c +9 -0
+	if (!maps[*r_idx][*i_idx].name)
+		(*r_idx)++;
+
+	// Iteration reaches the end of the maps
+	if (*r_idx >= arr_size)
+		return NULL;
+
+	for (; *r_idx < arr_size; (*r_idx)++) {
+		const char *ip_name;
Can't you put this to the innermost block?
Will move it in next version.
Quoted 3 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1611
Quoted 2 lines
drivers/platform/x86/intel/pmc/core.c +1 -0
+		if (reset)
Why you need this?

Quoted 8 lines

The purpose of this function is to return the name of the NEXT s0ix blocker with BDF information.
r_idx and i_idx are used to keep track of the current position of the iteration, therefore i_idx could not be reset to 0 at the first run of the inner for loop. After the first run of inner for loop reset should be set to true so in next run of the outer for loop i_idx could be reset to 0 (which mean the iteration reaches the next s0ix blocker map).

But why you cannot reset i_idx after the inner for () loop and drop this reset variable entirely?

Quoted 40 lines

Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1614
Quoted 3 lines
drivers/platform/x86/intel/pmc/core.c +3 -0
+			*i_idx = 0;
+
+		for (; maps[*r_idx][*i_idx].name; reset = TRUE, (*i_idx)++) {
true
This is hard enough to understand even without that "for (;". Would probably be better to use while () instead.
Will change to while loop in next version.
Quoted 17 lines
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +4 -0
+			if (!maps[*r_idx][*i_idx].blk)
+				continue;
+
+			bool exist = **lpm_req_regs & BIT(BDF_EXIST_BIT);
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1628
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10 -0
+			(*lpm_req_regs)++;
+			if (exist) {
+				ip_name = maps[*r_idx][*i_idx].name;
+				(*i_idx)++;
+				return ip_name;
+			}
+		}
+	}
+	return NULL;
+}
TBH, this entire function is horrible mess, two nested iterators as pointers, etc.
Quoted 3 lines
I'm very far from following all what going on here.
I suppose I've not seen this patch previously?
To achieve the NEXT name of the s0ix blocker with BDF information we need to iterate through two (set of) maps in parallel. The s0ix_blocker_maps provide names of all s0ix blockers and the lpm_req_regs map shows that which s0ix blocker has associated BDF information.
if maps[*r_idx][*i_idx].blk is set, that means it is a s0ix blocker. For each s0ix blocker, if **lpm_req_regs & BIT(BDF_EXIST_BIT) is set, that means this blocker has associated BDF information. Pointers are used to keep track of the current position of the two (set of) maps so the function provides the NEXT name of the s0ix blocker with associated BDF info.

Quoted 4 lines

I will probably switch to use a temporary data structure, such as list, to store all s0ix blockers with BDF info and then iterate through this list in pmc_core_process_bdf() call instead. That will make the logic easier to follow and maintain. I will also add more comments to next version of this patch.

My out-of-band suggestion was to convert i_idx into a correctly typed pointer as it's the last-level array, you only need to do two things for the pointer:

- set it to start of the next array when r_idx increases.
- increment the pointer with ++.

Quoted 24 lines

Quoted 8 lines
Quoted 8 lines
+static int pmc_core_process_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, u32 data,
drivers/platform/x86/intel/pmc/core.c +1 -0
+				unsigned int *r_idx, unsigned int *i_idx, u32
**lpm_req_regs,
drivers/platform/x86/intel/pmc/core.c +4 -0
+				const char **name)
+{
+	unsigned int i;
+
Quoted 8 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1642
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +7 -0
+	if (!data)
+		return 0;
+
+	if (!*name)
+		return -EINVAL;
+
+	for (i = BDF_FUN_LOW_BIT; i <= BDF_FUN_HIGH_BIT; i++) {
I think you can iterate 0 ... __fls(FIELD_MAX()).
Each 16 bit represents one device and the associated function numbers for one s0ix blocker.
Bit 4-0 indicates the device number.
Bit 12-5 indicates function numbers.

Quoted 8 lines

Bit 15 indicates if the next 16 bit is associated to the same s0ix blocker as the current word.
Between bit 12 and bit 5, each bit position represents one function number. Bit 5 represents function 0 and bit 12 represents function 7. I will add more comments in the next version.
Will change to use __fls(FIELD_MAX()) in next version.

Yes, these are fields which are to be defined with GENMASK()/BIT(). Then this code just has to figure out how to deal with that change and my suggestion was to use fls construct. If you find better approach, those can be used as well but my point is that this iteration should be sourced from the GENMASK_U16(12, 5).

Quoted 9 lines

Quoted 9 lines
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +1 -0
+	name = pmc_core_get_next_bdf_ip_name(pmc, &r_idx, &i_idx,
&lpm_reg_regs);
drivers/platform/x86/intel/pmc/core.c +4 -0
+	if (!name)
+		return -EINVAL;
+
+	pmc->bdf_list = devm_kzalloc(&pmcdev->pdev->dev, sizeof(struct
list_head), GFP_KERNEL);
Should use sizeof(*xx).

Quoted 8 lines

Quoted 2 lines
But why you need to allocate the list head and not have it in place within the pmc's struct?
The memory for bdf_list is only allocated when the bdf information is available to achieve.
intel_pmc_core driver can check if the memory is allocated to decide whether to create a file in debugfs for bdf in pmc_core_dbgfs_register().

But can't you use empty list check for that?

Quoted 31 lines

Quoted 5 lines
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +4 -0
+	if (!pmc->bdf_list)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(pmc->bdf_list);
Quoted 9 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1695
Quoted 8 lines
drivers/platform/x86/intel/pmc/core.c +8 -0
+
+	for (; sample_id < max_sample_id; sample_id++) {
+		u32 data;
+
+		ret = pmt_telem_read32(ep, sample_id, &data, 1);
+		if (ret) {
+			dev_err(&pmcdev->pdev->dev,
+				"couldn't read bdf: %d\n", ret);
One line.
Will change it in next version.
Quoted 9 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1701
Quoted 8 lines
drivers/platform/x86/intel/pmc/core.c +5 -0
+			return ret;
+		}
+
+		if (sample_id == header_id) {
+			size = (data & GENMASK(BDF_SIZE_HIGH_BIT,
BDF_SIZE_LOW_BIT))
drivers/platform/x86/intel/pmc/core.c +1 -0
+			       >> BDF_SIZE_LOW_BIT;
Define the field and use FIELD_GET().
Will change it in next version.
Quoted 5 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1702
Quoted 2 lines
drivers/platform/x86/intel/pmc/core.c +1 -0
+			header_id += size + 1;
No, I just cannot understand what's going on here, it's hopeless. Always when I think I've finally understood what its all about you throw a curve ball like this.

Quoted 8 lines

There is one header line (32 bit) between each type of s0ix blocker in the bdf association table. The bit 23-26 in header line indicates the size of each section of one specific type of s0ix blocker in this table.
header_id is used to keep track of the id of each header so we will process the header line differently from the other lines.
I will add more detailed comment in next version.

I suspect naming the fields with defines and using FIELD_GET() will already get you far.

Perhaps BDF_SIZE (=what remains when you take those custom coded bit field postfix out of the current naming) should be renamed into something like BDF_SECTION_SIZE for better clarity.

Signature · 1 line

i.

Xi Pardee

author maintainer

2025-09-11 18:33 UTC #15 lore raw

Re: [PATCH v3 5/6] platform/x86:intel/pmc: Show device and function number

Hi Ilpo,

Thanks for the review. I will fix the patch based on your comments and send another version separately.

Quoted 71 lines · Ilpo Järvinen

Quoted 44 lines · Xi Pardee
Hi Ilpo,
Thanks for the review.
Quoted 24 lines · Ilpo Järvinen
Quoted 21 lines · Xi Pardee
Add support to show device and function number for S0ix blockers. This feature depends on S0ix blocker substate requirement table and BDF association table. This feature is available for platforms starting from Pather Lake.
Only a subset of S0ix blockers has device and function number associated to it. Get the availability information from the substate requirement table. Get the device and function number mapping information for each S0ix blocker from the BDF association table.
Signed-off-by Xi Pardee <[email protected]>
---
   drivers/platform/x86/intel/pmc/core.c | 182 +++++++++++++++++++++++++-
   drivers/platform/x86/intel/pmc/core.h |  23 +++-
   2 files changed, 203 insertions(+), 2 deletions(-)
drivers/platform/x86/intel/pmc/core.c +0 -0
index a0b948a875a5a..69ee40cbb8b8a 100644
Quoted 15 lines
Quoted 13 lines
drivers/platform/x86/intel/pmc/core.c +13 -0
+		const char *name = NULL;
+		struct list_head *cur;
+		struct bdf_entry *bdf;
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmcidx];
+		if (!pmc)
+			continue;
+
+		list_for_each(cur, pmc->bdf_list) {
+			bdf = list_entry(cur, struct bdf_entry, node);
+			if (bdf->name != name) {
+				seq_printf(s, "pmc%d: %30s | %15x | %15x |\n",
Quoted 1 line
pmcidx,
%u
Quoted 1 line
Will change it in next version.
I don't remember if I mentioned it earlier but if you're going to address the review comment fully. There's no need to "ack" them like this. I trust you make the changes you don't contest.
By doing so, we can both save time by only focusing on the points which are contested or need further discussion. :-)
Quoted 16 lines
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1609
Quoted 9 lines
drivers/platform/x86/intel/pmc/core.c +9 -0
+	if (!maps[*r_idx][*i_idx].name)
+		(*r_idx)++;
+
+	// Iteration reaches the end of the maps
+	if (*r_idx >= arr_size)
+		return NULL;
+
+	for (; *r_idx < arr_size; (*r_idx)++) {
+		const char *ip_name;
Can't you put this to the innermost block?
Will move it in next version.
Quoted 2 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1611
Quoted 1 line
drivers/platform/x86/intel/pmc/core.c +1 -0
+		if (reset)
Why you need this?
The purpose of this function is to return the name of the NEXT s0ix blocker with BDF information.

Quoted 7 lines

Quoted 5 lines
r_idx and i_idx are used to keep track of the current position of the iteration, therefore i_idx could not be reset to 0 at the first run of the inner for loop. After the first run of inner for loop reset should be set to true so in next run of the outer for loop i_idx could be reset to 0 (which mean the iteration reaches the next s0ix blocker map).
But why you cannot reset i_idx after the inner for () loop and drop this reset variable entirely?

Yes, I will do that in next version.

Quoted 96 lines

Quoted 40 lines
Quoted 8 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1614
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +3 -0
+			*i_idx = 0;
+
+		for (; maps[*r_idx][*i_idx].name; reset = TRUE, (*i_idx)++) {
true
This is hard enough to understand even without that "for (;". Would probably be better to use while () instead.
Will change to while loop in next version.
Quoted 17 lines
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c +4 -0
+			if (!maps[*r_idx][*i_idx].blk)
+				continue;
+
+			bool exist = **lpm_req_regs & BIT(BDF_EXIST_BIT);
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1628
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10 -0
+			(*lpm_req_regs)++;
+			if (exist) {
+				ip_name = maps[*r_idx][*i_idx].name;
+				(*i_idx)++;
+				return ip_name;
+			}
+		}
+	}
+	return NULL;
+}
TBH, this entire function is horrible mess, two nested iterators as pointers, etc.
Quoted 3 lines
I'm very far from following all what going on here.
I suppose I've not seen this patch previously?
To achieve the NEXT name of the s0ix blocker with BDF information we need to iterate through two (set of) maps in parallel. The s0ix_blocker_maps provide names of all s0ix blockers and the lpm_req_regs map shows that which s0ix blocker has associated BDF information.
if maps[*r_idx][*i_idx].blk is set, that means it is a s0ix blocker. For each s0ix blocker, if **lpm_req_regs & BIT(BDF_EXIST_BIT) is set, that means this blocker has associated BDF information. Pointers are used to keep track of the current position of the two (set of) maps so the function provides the NEXT name of the s0ix blocker with associated BDF info.
Quoted 4 lines
I will probably switch to use a temporary data structure, such as list, to store all s0ix blockers with BDF info and then iterate through this list in pmc_core_process_bdf() call instead. That will make the logic easier to follow and maintain. I will also add more comments to next version of this patch.
My out-of-band suggestion was to convert i_idx into a correctly typed pointer as it's the last-level array, you only need to do two things for the pointer:
- set it to start of the next array when r_idx increases.
- increment the pointer with ++.
Quoted 23 lines
Quoted 8 lines
Quoted 8 lines
+static int pmc_core_process_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, u32 data,
drivers/platform/x86/intel/pmc/core.c +1 -0
+				unsigned int *r_idx, unsigned int *i_idx, u32
**lpm_req_regs,
drivers/platform/x86/intel/pmc/core.c +4 -0
+				const char **name)
+{
+	unsigned int i;
+
Quoted 8 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1642
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +7 -0
+	if (!data)
+		return 0;
+
+	if (!*name)
+		return -EINVAL;
+
+	for (i = BDF_FUN_LOW_BIT; i <= BDF_FUN_HIGH_BIT; i++) {
I think you can iterate 0 ... __fls(FIELD_MAX()).
Each 16 bit represents one device and the associated function numbers for one s0ix blocker.
Bit 4-0 indicates the device number.
Bit 12-5 indicates function numbers.
Quoted 8 lines
Bit 15 indicates if the next 16 bit is associated to the same s0ix blocker as the current word.
Between bit 12 and bit 5, each bit position represents one function number. Bit 5 represents function 0 and bit 12 represents function 7. I will add more comments in the next version.
Will change to use __fls(FIELD_MAX()) in next version.
Yes, these are fields which are to be defined with GENMASK()/BIT(). Then this code just has to figure out how to deal with that change and my suggestion was to use fls construct. If you find better approach, those can be used as well but my point is that this iteration should be sourced from the GENMASK_U16(12, 5).
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +1 -0
+	name = pmc_core_get_next_bdf_ip_name(pmc, &r_idx, &i_idx,
&lpm_reg_regs);
drivers/platform/x86/intel/pmc/core.c +4 -0
+	if (!name)
+		return -EINVAL;
+
+	pmc->bdf_list = devm_kzalloc(&pmcdev->pdev->dev, sizeof(struct
list_head), GFP_KERNEL);
Should use sizeof(*xx).

Quoted 8 lines

Quoted 7 lines
Quoted 2 lines
But why you need to allocate the list head and not have it in place within the pmc's struct?
The memory for bdf_list is only allocated when the bdf information is available to achieve.
intel_pmc_core driver can check if the memory is allocated to decide whether to create a file in debugfs for bdf in pmc_core_dbgfs_register().
But can't you use empty list check for that?

I can use the empty list check in pmc_core_dbgfs_register() instead.

Quoted 35 lines

Quoted 35 lines
Quoted 14 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1695
Quoted 13 lines
drivers/platform/x86/intel/pmc/core.c +12 -0
+	if (!pmc->bdf_list)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(pmc->bdf_list);
+
+	for (; sample_id < max_sample_id; sample_id++) {
+		u32 data;
+
+		ret = pmt_telem_read32(ep, sample_id, &data, 1);
+		if (ret) {
+			dev_err(&pmcdev->pdev->dev,
+				"couldn't read bdf: %d\n", ret);
One line.
Will change it in next version.
Quoted 8 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1701
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c +5 -0
+			return ret;
+		}
+
+		if (sample_id == header_id) {
+			size = (data & GENMASK(BDF_SIZE_HIGH_BIT,
BDF_SIZE_LOW_BIT))
drivers/platform/x86/intel/pmc/core.c +1 -0
+			       >> BDF_SIZE_LOW_BIT;
Define the field and use FIELD_GET().
Will change it in next version.
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.c int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc, · line 1702
Quoted 1 line
drivers/platform/x86/intel/pmc/core.c +1 -0
+			header_id += size + 1;
No, I just cannot understand what's going on here, it's hopeless. Always when I think I've finally understood what its all about you throw a curve ball like this.
There is one header line (32 bit) between each type of s0ix blocker in the bdf association table. The bit 23-26 in header line indicates the size of each section of one specific type of s0ix blocker in this table.
header_id is used to keep track of the id of each header so we will process the header line differently from the other lines.

Quoted 7 lines

Quoted 1 line
I will add more detailed comment in next version.
I suspect naming the fields with defines and using FIELD_GET() will already get you far.
Perhaps BDF_SIZE (=what remains when you take those custom coded bit field postfix out of the current naming) should be renamed into something like BDF_SECTION_SIZE for better clarity.

Will use FIELD_GET() to get the information in next version.

Thanks for reviewing the patch.

Quoted 1 line

Xi Pardee

author maintainer

2025-08-15 22:46 UTC #16 lore raw

PATCH v3 6/6 platform/x86:intel/pmc: Enable SSRAM support for Panther Lake

In reply to Xi Pardee

Enable Panther Lake platforms to achieve PMC information from Intel PMC SSRAM Telemetry driver and substate requirements data from telemetry region.

Signed-off-by Xi Pardee <[email protected]>

---
 drivers/platform/x86/intel/pmc/core.h |  4 ++++
 drivers/platform/x86/intel/pmc/ptl.c  | 33 +++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

drivers/platform/x86/intel/pmc/core.h +4 -0

index 6ff2d171dc2ba..381755aaeafaf 100644
@@ -297,6 +297,10 @@ enum ppfear_regs {
 #define PTL_PMC_LTR_CUR_ASLT			0x1C28
 #define PTL_PMC_LTR_CUR_PLT			0x1C2C
 #define PTL_PCD_PMC_MMIO_REG_LEN		0x31A8
+#define PTL_NUM_S0IX_BLOCKER			106
+#define PTL_BLK_REQ_OFFSET			55
+#define PTL_BDF_OFFSET				0x0
+#define PTL_BDF_TABLE_SIZE			54
 
 /* SSRAM PMC Device ID */
 /* LNL */

drivers/platform/x86/intel/pmc/ptl.c +33 -0

index 394515af60d60..20c6dac7a6729 100644
@@ -10,6 +10,17 @@
 
 #include "core.h"
 
+/* PMC SSRAM PMT Telemetry GUIDS */
+#define PCDP_LPM_REQ_GUID 0x47179370
+
+/*
+ * Die Mapping to Product.
+ * Product PCDDie
+ * PTL-H   PCD-H
+ * PTL-P   PCD-P
+ * PTL-U   PCD-P
+ */
+
 static const struct pmc_bit_map ptl_pcdp_pfear_map[] = {
 	{"PMC_0",               BIT(0)},
 	{"FUSE_OSSE",           BIT(1)},

@@ -515,6 +526,24 @@ static const struct pmc_reg_map ptl_pcdp_reg_map = {
 	.lpm_live_status_offset = MTL_LPM_LIVE_STATUS_OFFSET,
 	.s0ix_blocker_maps = ptl_pcdp_blk_maps,
 	.s0ix_blocker_offset = LNL_S0IX_BLOCKER_OFFSET,
+	.num_s0ix_blocker = PTL_NUM_S0IX_BLOCKER,
+	.blocker_req_offset = PTL_BLK_REQ_OFFSET,
+	.bdf_offset = PTL_BDF_OFFSET,
+	.bdf_table_size = PTL_BDF_TABLE_SIZE,
+};
+
+static struct pmc_info ptl_pmc_info_list[] = {
+	{
+		.guid	= PCDP_LPM_REQ_GUID,
+		.devid	= PMC_DEVID_PTL_PCDH,
+		.map	= &ptl_pcdp_reg_map,
+	},
+	{
+		.guid   = PCDP_LPM_REQ_GUID,
+		.devid  = PMC_DEVID_PTL_PCDP,
+		.map    = &ptl_pcdp_reg_map,
+	},
+	{}
 };
 
 #define PTL_NPU_PCI_DEV                0xb03e

@@ -543,8 +572,12 @@ static int ptl_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_in
 }
 
 struct pmc_dev_info ptl_pmc_dev = {
+	.pci_func = 2,
+	.regmap_list = ptl_pmc_info_list,
 	.map = &ptl_pcdp_reg_map,
+	.sub_req_show = &pmc_core_substate_blk_req_fops,
 	.suspend = cnl_suspend,
 	.resume = ptl_resume,
 	.init = ptl_core_init,
+	.sub_req = pmc_core_pmt_get_sub_req_bdf,
 };

Signature · 1 line

2.43.0