[PATCH v2 0/5] Enable SSRAM support in PTL and LNL

[PATCH v2 0/5] Enable SSRAM support in PTL and LNL

[Xi Pardee]

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

The first patch enables SSRAM support for Lunar Lake. The next three
patches introduces a new table in telemetry region to get substate
requirement information for platforms starting from Panther Lake. The
last patch enables SSRAM support for Panther Lake.

---

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 (5):
  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: Enable SSRAM support for Panther Lake

 drivers/platform/x86/intel/pmc/arl.c  |   2 +
 drivers/platform/x86/intel/pmc/core.c | 169 ++++++++++++++++++++------
 drivers/platform/x86/intel/pmc/core.h |  14 +++
 drivers/platform/x86/intel/pmc/lnl.c  |  17 +++
 drivers/platform/x86/intel/pmc/mtl.c  |   1 +
 drivers/platform/x86/intel/pmc/ptl.c  |  30 +++++
 6 files changed, 199 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] platform/x86:intel/pmc: Enable SSRAM support for Lunar Lake

[Xi Pardee]

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 <xi.pardee@linux.intel.com>
---
 drivers/platform/x86/intel/pmc/lnl.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/platform/x86/intel/pmc/lnl.c b/drivers/platform/x86/intel/pmc/lnl.c
index da513c234714b..e08a77c778c2c 100644
--- a/drivers/platform/x86/intel/pmc/lnl.c
+++ b/drivers/platform/x86/intel/pmc/lnl.c
@@ -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,
-- 
2.43.0

Re: [PATCH v2 1/5] platform/x86:intel/pmc: Enable SSRAM support for Lunar Lake

[Ilpo Järvinen]

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

On Tue, 24 Jun 2025, Xi Pardee wrote:

> 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 <xi.pardee@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/lnl.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/pmc/lnl.c b/drivers/platform/x86/intel/pmc/lnl.c
> index da513c234714b..e08a77c778c2c 100644
> --- a/drivers/platform/x86/intel/pmc/lnl.c
> +++ b/drivers/platform/x86/intel/pmc/lnl.c
> @@ -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,
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 2/5] platform/x86:intel/pmc: Move telemetry endpoint register handling

[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 <xi.pardee@linux.intel.com>
---
 drivers/platform/x86/intel/pmc/core.c | 51 +++++++++++++--------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index 540cd2fb0673b..a1dd80bdbd413 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -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;
 	}
-- 
2.43.0

Re: [PATCH v2 2/5] platform/x86:intel/pmc: Move telemetry endpoint register handling

[Ilpo Järvinen]

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

On Tue, 24 Jun 2025, Xi Pardee wrote:

> 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 <xi.pardee@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/core.c | 51 +++++++++++++--------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 540cd2fb0673b..a1dd80bdbd413 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -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;
>  	}
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 3/5] platform/x86:intel/pmc: Improve function to show substate header

[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 <xi.pardee@linux.intel.com>
---
 drivers/platform/x86/intel/pmc/core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index a1dd80bdbd413..47cc5120e7dd6 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -828,17 +828,20 @@ 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, char *name)
 {
 	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 (!strcmp(name, "Status")) {
+		seq_printf(s, " %9s |", name);
+		seq_printf(s, " %11s |\n", "Live Status");
+	} else
+		seq_printf(s, " %9s |\n", name);
 }
 
 static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
@@ -872,7 +875,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, "Status");
 
 		/* Loop over maps */
 		for (mp = 0; mp < num_maps; mp++) {
@@ -910,7 +913,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) {
-- 
2.43.0

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

[Ilpo Järvinen]

On Tue, 24 Jun 2025, Xi Pardee wrote:

> 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 <xi.pardee@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/core.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index a1dd80bdbd413..47cc5120e7dd6 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -828,17 +828,20 @@ 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, char *name)
>  {
>  	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 (!strcmp(name, "Status")) {

I'm not very happy passing this as string. Could we use an 
internal defines/enum instead for this differentiation?

> +		seq_printf(s, " %9s |", name);
> +		seq_printf(s, " %11s |\n", "Live Status");
> +	} else
> +		seq_printf(s, " %9s |\n", name);

Please always use braces symmetrically in both blocks.

>  }
>  
>  static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
> @@ -872,7 +875,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, "Status");
>  
>  		/* Loop over maps */
>  		for (mp = 0; mp < num_maps; mp++) {
> @@ -910,7 +913,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) {
> 

-- 
 i.

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

[Xi Pardee]

Hi Ilpo,

Thanks for reviewing the patch. My response is inline.

On 6/30/2025 4:34 AM, Ilpo Järvinen wrote:
> On Tue, 24 Jun 2025, Xi Pardee wrote:
>
>> 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 <xi.pardee@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel/pmc/core.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>> index a1dd80bdbd413..47cc5120e7dd6 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -828,17 +828,20 @@ 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, char *name)
>>   {
>>   	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 (!strcmp(name, "Status")) {
> I'm not very happy passing this as string. Could we use an
> internal defines/enum instead for this differentiation?
Will use enum in next version.
>
>> +		seq_printf(s, " %9s |", name);
>> +		seq_printf(s, " %11s |\n", "Live Status");
>> +	} else
>> +		seq_printf(s, " %9s |\n", name);
> Please always use braces symmetrically in both blocks.

Will use braces in next version.

Thanks!

Xi

>
>>   }
>>   
>>   static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>> @@ -872,7 +875,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, "Status");
>>   
>>   		/* Loop over maps */
>>   		for (mp = 0; mp < num_maps; mp++) {
>> @@ -910,7 +913,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) {
>>

[PATCH v2 4/5] platform/x86:intel/pmc: Show substate requirement for S0ix blockers

[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 <xi.pardee@linux.intel.com>
---
 drivers/platform/x86/intel/pmc/arl.c  |   2 +
 drivers/platform/x86/intel/pmc/core.c | 111 ++++++++++++++++++++++++--
 drivers/platform/x86/intel/pmc/core.h |  12 +++
 drivers/platform/x86/intel/pmc/lnl.c  |   1 +
 drivers/platform/x86/intel/pmc/mtl.c  |   1 +
 5 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
index 9d66d65e75779..1852876be234f 100644
--- a/drivers/platform/x86/intel/pmc/arl.c
+++ b/drivers/platform/x86/intel/pmc/arl.c
@@ -722,6 +722,7 @@ static int arl_h_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_
 
 struct pmc_dev_info arl_pmc_dev = {
 	.pci_func = 0,
+	.telem_info = SUB_REQ_LPM,
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &arl_socs_reg_map,
@@ -732,6 +733,7 @@ struct pmc_dev_info arl_pmc_dev = {
 
 struct pmc_dev_info arl_h_pmc_dev = {
 	.pci_func = 2,
+	.telem_info = SUB_REQ_LPM,
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index 47cc5120e7dd6..e9d281b064462 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -844,6 +844,56 @@ static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
 		seq_printf(s, " %9s |\n", name);
 }
 
+static int pmc_core_substate_blk_req_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	unsigned int pmc_index;
+	u32 *blk_sub_req_regs;
+
+	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;
+		struct pmc *pmc;
+
+		pmc = pmcdev->pmcs[pmc_index];
+		if (!pmc || !pmc->blk_sub_req_regs)
+			continue;
+
+		blk_sub_req_regs = pmc->blk_sub_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, "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++) {
+				int mode;
+
+				if (!map->blk)
+					continue;
+
+				counter = pmc_core_reg_read(pmc, offset);
+				seq_printf(s, "pmc%d: %34s |", pmc_index, map->name);
+				pmc_for_each_mode(mode, pmcdev) {
+					bool required = *blk_sub_req_regs & BIT(mode);
+
+					seq_printf(s, " %9s |", required ? "Required" : " ");
+				}
+				seq_printf(s, " %9d |\n", counter);
+				offset += map->blk * S0IX_BLK_SIZE;
+				blk_sub_req_regs++;
+			}
+		}
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_blk_req);
+
 static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmcdev = s->private;
@@ -1335,7 +1385,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 		debugfs_create_file("substate_requirements", 0444,
 				    pmcdev->dbgfs_dir, pmcdev,
 				    &pmc_core_substate_req_regs_fops);
-	}
+	} else if (primary_pmc->blk_sub_req_regs)
+		debugfs_create_file("substate_requirements", 0444,
+				    pmcdev->dbgfs_dir, pmcdev,
+				    &pmc_core_substate_blk_req_fops);
 
 	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
 		debugfs_create_file("pson_residency_usec", 0444,
@@ -1441,7 +1494,38 @@ 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)
+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;
+	unsigned int index;
+	u32 *req_offset;
+	int ret;
+
+	num_blocker = pmc->map->num_s0ix_blocker;
+	sample_id = pmc->map->blocker_req_offset;
+
+	pmc->blk_sub_req_regs = devm_kcalloc(&pmcdev->pdev->dev,
+					 num_blocker, sizeof(u32),
+					 GFP_KERNEL);
+	if (!pmc->blk_sub_req_regs)
+		return -ENOMEM;
+
+	req_offset = pmc->blk_sub_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++;
+	}
+	return 0;
+}
+
+static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func, unsigned int telem_info)
 {
 	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
 	struct telem_endpoint *ep;
@@ -1470,13 +1554,26 @@ 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);
+		if (telem_info & SUB_REQ_LPM) {
+			ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
+			if (ret)
+				goto unregister_ep;
+		}
+
+		if (telem_info & SUB_REQ_BLK) {
+			ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
+			if (ret)
+				goto unregister_ep;
+		}
+
 		pmt_telem_unregister_endpoint(ep);
-		if (ret)
-			return ret;
 	}
 
 	return 0;
+
+unregister_ep:
+	pmt_telem_unregister_endpoint(ep);
+	return ret;
 }
 
 static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
@@ -1585,7 +1682,9 @@ 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->pci_func,
+					      pmc_dev_info->telem_info);
 		if (ret)
 			goto unmap_regbase;
 	}
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index 4a94a4ee031e6..d8c7b28493055 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -29,6 +29,10 @@ struct telem_endpoint;
 #define LPM_REG_COUNT		28
 #define LPM_MODE_OFFSET		1
 
+/* Telemetry Endpoint Info bits */
+#define SUB_REQ_LPM		0x01	/* Substate requirement for low power mode requirements */
+#define SUB_REQ_BLK		0x02	/* Substate requirement for S0ix blockers */
+
 /* Sunrise Point Power Management Controller PCI Device ID */
 #define SPT_PMC_PCI_DEVICE_ID			0x9d21
 #define SPT_PMC_BASE_ADDR_OFFSET		0x48
@@ -344,6 +348,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 +375,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;
@@ -404,6 +412,7 @@ struct pmc_info {
  * @map:		pointer to pmc_reg_map struct that contains platform
  *			specific attributes
  * @lpm_req_regs:	List of substate requirements
+ * @blk_sub_req_reqs:	List of registers showing substate requirements for S0ix blockers
  * @ltr_ign:		Holds LTR ignore data while suspended
  *
  * pmc contains info about one power management controller device.
@@ -413,6 +422,7 @@ struct pmc {
 	void __iomem *regbase;
 	const struct pmc_reg_map *map;
 	u32 *lpm_req_regs;
+	u32 *blk_sub_req_regs;
 	u32 ltr_ign;
 };
 
@@ -468,6 +478,7 @@ enum pmc_index {
 /**
  * struct pmc_dev_info - Structure to keep PMC device info
  * @pci_func:		Function number of the primary PMC
+ * @telem_info:		Bitmask to indicate which telemetry info is available
  * @dmu_guid:		Die Management Unit GUID
  * @regmap_list:	Pointer to a list of pmc_info structure that could be
  *			available for the platform. When set, this field implies
@@ -480,6 +491,7 @@ enum pmc_index {
  */
 struct pmc_dev_info {
 	u8 pci_func;
+	u8 telem_info;
 	u32 dmu_guid;
 	struct pmc_info *regmap_list;
 	const struct pmc_reg_map *map;
diff --git a/drivers/platform/x86/intel/pmc/lnl.c b/drivers/platform/x86/intel/pmc/lnl.c
index e08a77c778c2c..ec9e79f6cd913 100644
--- a/drivers/platform/x86/intel/pmc/lnl.c
+++ b/drivers/platform/x86/intel/pmc/lnl.c
@@ -572,6 +572,7 @@ 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,
+	.telem_info = SUB_REQ_LPM,
 	.regmap_list = lnl_pmc_info_list,
 	.map = &lnl_socm_reg_map,
 	.suspend = cnl_suspend,
diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
index faa13a7ee688f..c58a871e2e0df 100644
--- a/drivers/platform/x86/intel/pmc/mtl.c
+++ b/drivers/platform/x86/intel/pmc/mtl.c
@@ -994,6 +994,7 @@ static int mtl_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_in
 
 struct pmc_dev_info mtl_pmc_dev = {
 	.pci_func = 2,
+	.telem_info = SUB_REQ_LPM,
 	.dmu_guid = MTL_PMT_DMU_GUID,
 	.regmap_list = mtl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
-- 
2.43.0

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

[Ilpo Järvinen]

On Tue, 24 Jun 2025, Xi Pardee wrote:

> 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.

Empty line.

> 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 <xi.pardee@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/arl.c  |   2 +
>  drivers/platform/x86/intel/pmc/core.c | 111 ++++++++++++++++++++++++--
>  drivers/platform/x86/intel/pmc/core.h |  12 +++
>  drivers/platform/x86/intel/pmc/lnl.c  |   1 +
>  drivers/platform/x86/intel/pmc/mtl.c  |   1 +
>  5 files changed, 121 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
> index 9d66d65e75779..1852876be234f 100644
> --- a/drivers/platform/x86/intel/pmc/arl.c
> +++ b/drivers/platform/x86/intel/pmc/arl.c
> @@ -722,6 +722,7 @@ static int arl_h_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_
>  
>  struct pmc_dev_info arl_pmc_dev = {
>  	.pci_func = 0,
> +	.telem_info = SUB_REQ_LPM,
>  	.dmu_guid = ARL_PMT_DMU_GUID,
>  	.regmap_list = arl_pmc_info_list,
>  	.map = &arl_socs_reg_map,
> @@ -732,6 +733,7 @@ struct pmc_dev_info arl_pmc_dev = {
>  
>  struct pmc_dev_info arl_h_pmc_dev = {
>  	.pci_func = 2,
> +	.telem_info = SUB_REQ_LPM,
>  	.dmu_guid = ARL_PMT_DMU_GUID,
>  	.regmap_list = arl_pmc_info_list,
>  	.map = &mtl_socm_reg_map,
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 47cc5120e7dd6..e9d281b064462 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -844,6 +844,56 @@ static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
>  		seq_printf(s, " %9s |\n", name);
>  }
>  
> +static int pmc_core_substate_blk_req_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	unsigned int pmc_index;
> +	u32 *blk_sub_req_regs;

Why is this here and the other variable in the inner block?

> +
> +	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;
> +		struct pmc *pmc;
> +
> +		pmc = pmcdev->pmcs[pmc_index];
> +		if (!pmc || !pmc->blk_sub_req_regs)
> +			continue;
> +
> +		blk_sub_req_regs = pmc->blk_sub_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, "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++) {
> +				int mode;
> +
> +				if (!map->blk)
> +					continue;
> +
> +				counter = pmc_core_reg_read(pmc, offset);
> +				seq_printf(s, "pmc%d: %34s |", pmc_index, map->name);
> +				pmc_for_each_mode(mode, pmcdev) {
> +					bool required = *blk_sub_req_regs & BIT(mode);
> +
> +					seq_printf(s, " %9s |", required ? "Required" : " ");
> +				}
> +				seq_printf(s, " %9d |\n", counter);
> +				offset += map->blk * S0IX_BLK_SIZE;
> +				blk_sub_req_regs++;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_blk_req);
> +
>  static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>  {
>  	struct pmc_dev *pmcdev = s->private;
> @@ -1335,7 +1385,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  		debugfs_create_file("substate_requirements", 0444,
>  				    pmcdev->dbgfs_dir, pmcdev,
>  				    &pmc_core_substate_req_regs_fops);
> -	}
> +	} else if (primary_pmc->blk_sub_req_regs)
> +		debugfs_create_file("substate_requirements", 0444,
> +				    pmcdev->dbgfs_dir, pmcdev,
> +				    &pmc_core_substate_blk_req_fops);
>
>  	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
>  		debugfs_create_file("pson_residency_usec", 0444,
> @@ -1441,7 +1494,38 @@ 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)
> +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;
> +	unsigned int index;
> +	u32 *req_offset;
> +	int ret;
> +
> +	num_blocker = pmc->map->num_s0ix_blocker;
> +	sample_id = pmc->map->blocker_req_offset;
> +
> +	pmc->blk_sub_req_regs = devm_kcalloc(&pmcdev->pdev->dev,
> +					 num_blocker, sizeof(u32),
> +					 GFP_KERNEL);

Correct alignment.

I guess you want to keep those two args on the same line as this would fit 
on 2 lines.

> +	if (!pmc->blk_sub_req_regs)
> +		return -ENOMEM;
> +
> +	req_offset = pmc->blk_sub_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++;
> +	}
> +	return 0;
> +}
> +
> +static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func, unsigned int telem_info)
>  {
>  	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
>  	struct telem_endpoint *ep;
> @@ -1470,13 +1554,26 @@ 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);
> +		if (telem_info & SUB_REQ_LPM) {
> +			ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
> +			if (ret)
> +				goto unregister_ep;
> +		}
> +
> +		if (telem_info & SUB_REQ_BLK) {

These two are mutually exclusive? I'm really wondering cuold pointers be 
used instead to avoid these ifs here and in debugfs fops selection.

> +			ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
> +			if (ret)
> +				goto unregister_ep;
> +		}
> +
>  		pmt_telem_unregister_endpoint(ep);
> -		if (ret)
> -			return ret;
>  	}
>  
>  	return 0;
> +
> +unregister_ep:
> +	pmt_telem_unregister_endpoint(ep);
> +	return ret;
>  }
>  
>  static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
> @@ -1585,7 +1682,9 @@ 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->pci_func,
> +					      pmc_dev_info->telem_info);
>  		if (ret)
>  			goto unmap_regbase;
>  	}
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index 4a94a4ee031e6..d8c7b28493055 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -29,6 +29,10 @@ struct telem_endpoint;
>  #define LPM_REG_COUNT		28
>  #define LPM_MODE_OFFSET		1
>  
> +/* Telemetry Endpoint Info bits */
> +#define SUB_REQ_LPM		0x01	/* Substate requirement for low power mode requirements */
> +#define SUB_REQ_BLK		0x02	/* Substate requirement for S0ix blockers */
> +
>  /* Sunrise Point Power Management Controller PCI Device ID */
>  #define SPT_PMC_PCI_DEVICE_ID			0x9d21
>  #define SPT_PMC_BASE_ADDR_OFFSET		0x48
> @@ -344,6 +348,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 +375,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;
> @@ -404,6 +412,7 @@ struct pmc_info {
>   * @map:		pointer to pmc_reg_map struct that contains platform
>   *			specific attributes
>   * @lpm_req_regs:	List of substate requirements
> + * @blk_sub_req_reqs:	List of registers showing substate requirements for S0ix blockers
>   * @ltr_ign:		Holds LTR ignore data while suspended
>   *
>   * pmc contains info about one power management controller device.
> @@ -413,6 +422,7 @@ struct pmc {
>  	void __iomem *regbase;
>  	const struct pmc_reg_map *map;
>  	u32 *lpm_req_regs;
> +	u32 *blk_sub_req_regs;
>  	u32 ltr_ign;
>  };
>  
> @@ -468,6 +478,7 @@ enum pmc_index {
>  /**
>   * struct pmc_dev_info - Structure to keep PMC device info
>   * @pci_func:		Function number of the primary PMC
> + * @telem_info:		Bitmask to indicate which telemetry info is available
>   * @dmu_guid:		Die Management Unit GUID
>   * @regmap_list:	Pointer to a list of pmc_info structure that could be
>   *			available for the platform. When set, this field implies
> @@ -480,6 +491,7 @@ enum pmc_index {
>   */
>  struct pmc_dev_info {
>  	u8 pci_func;
> +	u8 telem_info;
>  	u32 dmu_guid;
>  	struct pmc_info *regmap_list;
>  	const struct pmc_reg_map *map;
> diff --git a/drivers/platform/x86/intel/pmc/lnl.c b/drivers/platform/x86/intel/pmc/lnl.c
> index e08a77c778c2c..ec9e79f6cd913 100644
> --- a/drivers/platform/x86/intel/pmc/lnl.c
> +++ b/drivers/platform/x86/intel/pmc/lnl.c
> @@ -572,6 +572,7 @@ 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,
> +	.telem_info = SUB_REQ_LPM,
>  	.regmap_list = lnl_pmc_info_list,
>  	.map = &lnl_socm_reg_map,
>  	.suspend = cnl_suspend,
> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
> index faa13a7ee688f..c58a871e2e0df 100644
> --- a/drivers/platform/x86/intel/pmc/mtl.c
> +++ b/drivers/platform/x86/intel/pmc/mtl.c
> @@ -994,6 +994,7 @@ static int mtl_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_in
>  
>  struct pmc_dev_info mtl_pmc_dev = {
>  	.pci_func = 2,
> +	.telem_info = SUB_REQ_LPM,
>  	.dmu_guid = MTL_PMT_DMU_GUID,
>  	.regmap_list = mtl_pmc_info_list,
>  	.map = &mtl_socm_reg_map,
> 

-- 
 i.

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

[Xi Pardee]

Hi Ilpo,

Thanks for reviewing the patch. My response is inline.

On 6/26/2025 9:38 AM, Ilpo Järvinen wrote:
> On Tue, 24 Jun 2025, Xi Pardee wrote:
>
>> 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.
> Empty line.
Will fix in next version.
>
>> 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 <xi.pardee@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel/pmc/arl.c  |   2 +
>>   drivers/platform/x86/intel/pmc/core.c | 111 ++++++++++++++++++++++++--
>>   drivers/platform/x86/intel/pmc/core.h |  12 +++
>>   drivers/platform/x86/intel/pmc/lnl.c  |   1 +
>>   drivers/platform/x86/intel/pmc/mtl.c  |   1 +
>>   5 files changed, 121 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
>> index 9d66d65e75779..1852876be234f 100644
>> --- a/drivers/platform/x86/intel/pmc/arl.c
>> +++ b/drivers/platform/x86/intel/pmc/arl.c
>> @@ -722,6 +722,7 @@ static int arl_h_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_
>>   
>>   struct pmc_dev_info arl_pmc_dev = {
>>   	.pci_func = 0,
>> +	.telem_info = SUB_REQ_LPM,
>>   	.dmu_guid = ARL_PMT_DMU_GUID,
>>   	.regmap_list = arl_pmc_info_list,
>>   	.map = &arl_socs_reg_map,
>> @@ -732,6 +733,7 @@ struct pmc_dev_info arl_pmc_dev = {
>>   
>>   struct pmc_dev_info arl_h_pmc_dev = {
>>   	.pci_func = 2,
>> +	.telem_info = SUB_REQ_LPM,
>>   	.dmu_guid = ARL_PMT_DMU_GUID,
>>   	.regmap_list = arl_pmc_info_list,
>>   	.map = &mtl_socm_reg_map,
>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>> index 47cc5120e7dd6..e9d281b064462 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -844,6 +844,56 @@ static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index,
>>   		seq_printf(s, " %9s |\n", name);
>>   }
>>   
>> +static int pmc_core_substate_blk_req_show(struct seq_file *s, void *unused)
>> +{
>> +	struct pmc_dev *pmcdev = s->private;
>> +	unsigned int pmc_index;
>> +	u32 *blk_sub_req_regs;
> Why is this here and the other variable in the inner block?
Will move the blk_sub_req_regs declaration to the inner block.
>
>> +
>> +	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;
>> +		struct pmc *pmc;
>> +
>> +		pmc = pmcdev->pmcs[pmc_index];
>> +		if (!pmc || !pmc->blk_sub_req_regs)
>> +			continue;
>> +
>> +		blk_sub_req_regs = pmc->blk_sub_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, "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++) {
>> +				int mode;
>> +
>> +				if (!map->blk)
>> +					continue;
>> +
>> +				counter = pmc_core_reg_read(pmc, offset);
>> +				seq_printf(s, "pmc%d: %34s |", pmc_index, map->name);
>> +				pmc_for_each_mode(mode, pmcdev) {
>> +					bool required = *blk_sub_req_regs & BIT(mode);
>> +
>> +					seq_printf(s, " %9s |", required ? "Required" : " ");
>> +				}
>> +				seq_printf(s, " %9d |\n", counter);
>> +				offset += map->blk * S0IX_BLK_SIZE;
>> +				blk_sub_req_regs++;
>> +			}
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_blk_req);
>> +
>>   static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>>   {
>>   	struct pmc_dev *pmcdev = s->private;
>> @@ -1335,7 +1385,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>>   		debugfs_create_file("substate_requirements", 0444,
>>   				    pmcdev->dbgfs_dir, pmcdev,
>>   				    &pmc_core_substate_req_regs_fops);
>> -	}
>> +	} else if (primary_pmc->blk_sub_req_regs)
>> +		debugfs_create_file("substate_requirements", 0444,
>> +				    pmcdev->dbgfs_dir, pmcdev,
>> +				    &pmc_core_substate_blk_req_fops);
>>
>>   	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
>>   		debugfs_create_file("pson_residency_usec", 0444,
>> @@ -1441,7 +1494,38 @@ 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)
>> +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;
>> +	unsigned int index;
>> +	u32 *req_offset;
>> +	int ret;
>> +
>> +	num_blocker = pmc->map->num_s0ix_blocker;
>> +	sample_id = pmc->map->blocker_req_offset;
>> +
>> +	pmc->blk_sub_req_regs = devm_kcalloc(&pmcdev->pdev->dev,
>> +					 num_blocker, sizeof(u32),
>> +					 GFP_KERNEL);
> Correct alignment.
>
> I guess you want to keep those two args on the same line as this would fit
> on 2 lines.
Will change the alignment and move two args to be on one line.
>
>> +	if (!pmc->blk_sub_req_regs)
>> +		return -ENOMEM;
>> +
>> +	req_offset = pmc->blk_sub_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++;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func, unsigned int telem_info)
>>   {
>>   	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
>>   	struct telem_endpoint *ep;
>> @@ -1470,13 +1554,26 @@ 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);
>> +		if (telem_info & SUB_REQ_LPM) {
>> +			ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
>> +			if (ret)
>> +				goto unregister_ep;
>> +		}
>> +
>> +		if (telem_info & SUB_REQ_BLK) {
> These two are mutually exclusive? I'm really wondering cuold pointers be
> used instead to avoid these ifs here and in debugfs fops selection.
The SUB_REQ_BLK and SUB_REQ_LPM are mutually exclusive. We will also 
introduce more
types of data from the telemetry endpoint but not mutually exclusive in 
the next series, therefore
using bit to identify the type of data sounds appropriate to me. I am 
not sure how to use pointer to
select data type in this scenario, Could you please give an example?

Thanks!

Xi

>
>> +			ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
>> +			if (ret)
>> +				goto unregister_ep;
>> +		}
>> +
>>   		pmt_telem_unregister_endpoint(ep);
>> -		if (ret)
>> -			return ret;
>>   	}
>>   
>>   	return 0;
>> +
>> +unregister_ep:
>> +	pmt_telem_unregister_endpoint(ep);
>> +	return ret;
>>   }
>>   
>>   static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
>> @@ -1585,7 +1682,9 @@ 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->pci_func,
>> +					      pmc_dev_info->telem_info);
>>   		if (ret)
>>   			goto unmap_regbase;
>>   	}
>> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
>> index 4a94a4ee031e6..d8c7b28493055 100644
>> --- a/drivers/platform/x86/intel/pmc/core.h
>> +++ b/drivers/platform/x86/intel/pmc/core.h
>> @@ -29,6 +29,10 @@ struct telem_endpoint;
>>   #define LPM_REG_COUNT		28
>>   #define LPM_MODE_OFFSET		1
>>   
>> +/* Telemetry Endpoint Info bits */
>> +#define SUB_REQ_LPM		0x01	/* Substate requirement for low power mode requirements */
>> +#define SUB_REQ_BLK		0x02	/* Substate requirement for S0ix blockers */
>> +
>>   /* Sunrise Point Power Management Controller PCI Device ID */
>>   #define SPT_PMC_PCI_DEVICE_ID			0x9d21
>>   #define SPT_PMC_BASE_ADDR_OFFSET		0x48
>> @@ -344,6 +348,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 +375,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;
>> @@ -404,6 +412,7 @@ struct pmc_info {
>>    * @map:		pointer to pmc_reg_map struct that contains platform
>>    *			specific attributes
>>    * @lpm_req_regs:	List of substate requirements
>> + * @blk_sub_req_reqs:	List of registers showing substate requirements for S0ix blockers
>>    * @ltr_ign:		Holds LTR ignore data while suspended
>>    *
>>    * pmc contains info about one power management controller device.
>> @@ -413,6 +422,7 @@ struct pmc {
>>   	void __iomem *regbase;
>>   	const struct pmc_reg_map *map;
>>   	u32 *lpm_req_regs;
>> +	u32 *blk_sub_req_regs;
>>   	u32 ltr_ign;
>>   };
>>   
>> @@ -468,6 +478,7 @@ enum pmc_index {
>>   /**
>>    * struct pmc_dev_info - Structure to keep PMC device info
>>    * @pci_func:		Function number of the primary PMC
>> + * @telem_info:		Bitmask to indicate which telemetry info is available
>>    * @dmu_guid:		Die Management Unit GUID
>>    * @regmap_list:	Pointer to a list of pmc_info structure that could be
>>    *			available for the platform. When set, this field implies
>> @@ -480,6 +491,7 @@ enum pmc_index {
>>    */
>>   struct pmc_dev_info {
>>   	u8 pci_func;
>> +	u8 telem_info;
>>   	u32 dmu_guid;
>>   	struct pmc_info *regmap_list;
>>   	const struct pmc_reg_map *map;
>> diff --git a/drivers/platform/x86/intel/pmc/lnl.c b/drivers/platform/x86/intel/pmc/lnl.c
>> index e08a77c778c2c..ec9e79f6cd913 100644
>> --- a/drivers/platform/x86/intel/pmc/lnl.c
>> +++ b/drivers/platform/x86/intel/pmc/lnl.c
>> @@ -572,6 +572,7 @@ 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,
>> +	.telem_info = SUB_REQ_LPM,
>>   	.regmap_list = lnl_pmc_info_list,
>>   	.map = &lnl_socm_reg_map,
>>   	.suspend = cnl_suspend,
>> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
>> index faa13a7ee688f..c58a871e2e0df 100644
>> --- a/drivers/platform/x86/intel/pmc/mtl.c
>> +++ b/drivers/platform/x86/intel/pmc/mtl.c
>> @@ -994,6 +994,7 @@ static int mtl_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_in
>>   
>>   struct pmc_dev_info mtl_pmc_dev = {
>>   	.pci_func = 2,
>> +	.telem_info = SUB_REQ_LPM,
>>   	.dmu_guid = MTL_PMT_DMU_GUID,
>>   	.regmap_list = mtl_pmc_info_list,
>>   	.map = &mtl_socm_reg_map,
>>

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

[Ilpo Järvinen]

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

On Thu, 26 Jun 2025, Xi Pardee wrote:

> Hi Ilpo,
> 
> Thanks for reviewing the patch. My response is inline.
> 
> On 6/26/2025 9:38 AM, Ilpo Järvinen wrote:
> > On Tue, 24 Jun 2025, Xi Pardee wrote:
> > 
> > > 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.
> > Empty line.
> Will fix in next version.
> > 
> > > 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 <xi.pardee@linux.intel.com>
> > > ---
> > >   drivers/platform/x86/intel/pmc/arl.c  |   2 +
> > >   drivers/platform/x86/intel/pmc/core.c | 111 ++++++++++++++++++++++++--
> > >   drivers/platform/x86/intel/pmc/core.h |  12 +++
> > >   drivers/platform/x86/intel/pmc/lnl.c  |   1 +
> > >   drivers/platform/x86/intel/pmc/mtl.c  |   1 +
> > >   5 files changed, 121 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/pmc/arl.c
> > > b/drivers/platform/x86/intel/pmc/arl.c
> > > index 9d66d65e75779..1852876be234f 100644
> > > --- a/drivers/platform/x86/intel/pmc/arl.c
> > > +++ b/drivers/platform/x86/intel/pmc/arl.c
> > > @@ -722,6 +722,7 @@ static int arl_h_core_init(struct pmc_dev *pmcdev,
> > > struct pmc_dev_info *pmc_dev_
> > >     struct pmc_dev_info arl_pmc_dev = {
> > >   	.pci_func = 0,
> > > +	.telem_info = SUB_REQ_LPM,
> > >   	.dmu_guid = ARL_PMT_DMU_GUID,
> > >   	.regmap_list = arl_pmc_info_list,
> > >   	.map = &arl_socs_reg_map,
> > > @@ -732,6 +733,7 @@ struct pmc_dev_info arl_pmc_dev = {
> > >     struct pmc_dev_info arl_h_pmc_dev = {
> > >   	.pci_func = 2,
> > > +	.telem_info = SUB_REQ_LPM,
> > >   	.dmu_guid = ARL_PMT_DMU_GUID,
> > >   	.regmap_list = arl_pmc_info_list,
> > >   	.map = &mtl_socm_reg_map,
> > > diff --git a/drivers/platform/x86/intel/pmc/core.c
> > > b/drivers/platform/x86/intel/pmc/core.c
> > > index 47cc5120e7dd6..e9d281b064462 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.c
> > > +++ b/drivers/platform/x86/intel/pmc/core.c
> > > @@ -844,6 +844,56 @@ static void pmc_core_substate_req_header_show(struct
> > > seq_file *s, int pmc_index,
> > >   		seq_printf(s, " %9s |\n", name);
> > >   }
> > >   +static int pmc_core_substate_blk_req_show(struct seq_file *s, void
> > > *unused)
> > > +{
> > > +	struct pmc_dev *pmcdev = s->private;
> > > +	unsigned int pmc_index;
> > > +	u32 *blk_sub_req_regs;
> > Why is this here and the other variable in the inner block?
> Will move the blk_sub_req_regs declaration to the inner block.
> > 
> > > +
> > > +	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;
> > > +		struct pmc *pmc;
> > > +
> > > +		pmc = pmcdev->pmcs[pmc_index];
> > > +		if (!pmc || !pmc->blk_sub_req_regs)
> > > +			continue;
> > > +
> > > +		blk_sub_req_regs = pmc->blk_sub_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, "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++) {
> > > +				int mode;
> > > +
> > > +				if (!map->blk)
> > > +					continue;
> > > +
> > > +				counter = pmc_core_reg_read(pmc, offset);
> > > +				seq_printf(s, "pmc%d: %34s |", pmc_index,
> > > map->name);
> > > +				pmc_for_each_mode(mode, pmcdev) {
> > > +					bool required = *blk_sub_req_regs &
> > > BIT(mode);
> > > +
> > > +					seq_printf(s, " %9s |", required ?
> > > "Required" : " ");
> > > +				}
> > > +				seq_printf(s, " %9d |\n", counter);
> > > +				offset += map->blk * S0IX_BLK_SIZE;
> > > +				blk_sub_req_regs++;
> > > +			}
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_blk_req);
> > > +
> > >   static int pmc_core_substate_req_regs_show(struct seq_file *s, void
> > > *unused)
> > >   {
> > >   	struct pmc_dev *pmcdev = s->private;
> > > @@ -1335,7 +1385,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev
> > > *pmcdev)
> > >   		debugfs_create_file("substate_requirements", 0444,
> > >   				    pmcdev->dbgfs_dir, pmcdev,
> > >   				    &pmc_core_substate_req_regs_fops);
> > > -	}
> > > +	} else if (primary_pmc->blk_sub_req_regs)
> > > +		debugfs_create_file("substate_requirements", 0444,
> > > +				    pmcdev->dbgfs_dir, pmcdev,
> > > +				    &pmc_core_substate_blk_req_fops);
> > > 
> > >   	if (primary_pmc->map->pson_residency_offset &&
> > > pmc_core_is_pson_residency_enabled(pmcdev)) {
> > >   		debugfs_create_file("pson_residency_usec", 0444,
> > > @@ -1441,7 +1494,38 @@ 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)
> > > +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;
> > > +	unsigned int index;
> > > +	u32 *req_offset;
> > > +	int ret;
> > > +
> > > +	num_blocker = pmc->map->num_s0ix_blocker;
> > > +	sample_id = pmc->map->blocker_req_offset;
> > > +
> > > +	pmc->blk_sub_req_regs = devm_kcalloc(&pmcdev->pdev->dev,
> > > +					 num_blocker, sizeof(u32),
> > > +					 GFP_KERNEL);
> > Correct alignment.
> > 
> > I guess you want to keep those two args on the same line as this would fit
> > on 2 lines.
> Will change the alignment and move two args to be on one line.
> > 
> > > +	if (!pmc->blk_sub_req_regs)
> > > +		return -ENOMEM;
> > > +
> > > +	req_offset = pmc->blk_sub_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++;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func,
> > > unsigned int telem_info)
> > >   {
> > >   	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
> > >   	struct telem_endpoint *ep;
> > > @@ -1470,13 +1554,26 @@ 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);
> > > +		if (telem_info & SUB_REQ_LPM) {
> > > +			ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
> > > +			if (ret)
> > > +				goto unregister_ep;
> > > +		}
> > > +
> > > +		if (telem_info & SUB_REQ_BLK) {
> > These two are mutually exclusive? I'm really wondering cuold pointers be
> > used instead to avoid these ifs here and in debugfs fops selection.
> The SUB_REQ_BLK and SUB_REQ_LPM are mutually exclusive. We will also introduce
> more
> types of data from the telemetry endpoint but not mutually exclusive in the
> next series, therefore
> using bit to identify the type of data sounds appropriate to me. I am not sure
> how to use pointer to
> select data type in this scenario, Could you please give an example?

Perhaps there isn't, I didn't have a crystal ball what's coming after this 
series at hand. :-)

I just noted there are these mutually exclusive pointers:

debugfs_create_file("substate_requirements", ..., &pmc_core_substate_req_regs_fops);
vs
debugfs_create_file("substate_requirements", ..., &pmc_core_substate_blk_req_fops);

and

ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
vs
ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);

As both look like they're decided based on what platform we're running on, 
it looks similar to what pmc_dev_info already contains, having these as 
pointers in pmc_dev_info would avoid having to create those two branches 
just to use a different pointer.

(BTW, those two places also use a different way to decide which one to 
use, which feels slightly inconsistent).


Whether using pointers in pmc_dev_info makes sense given the extra info 
you provided now, I'm not any more entirely sure.

> > > +			ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
> > > +			if (ret)
> > > +				goto unregister_ep;
> > > +		}
> > > +
> > >   		pmt_telem_unregister_endpoint(ep);
> > > -		if (ret)
> > > -			return ret;
> > >   	}
> > >     	return 0;
> > > +
> > > +unregister_ep:
> > > +	pmt_telem_unregister_endpoint(ep);
> > > +	return ret;
> > >   }
> > >     static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info
> > > *list, u16 devid)
> > > @@ -1585,7 +1682,9 @@ 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->pci_func,
> > > +					      pmc_dev_info->telem_info);
> > >   		if (ret)
> > >   			goto unmap_regbase;
> > >   	}
> > > diff --git a/drivers/platform/x86/intel/pmc/core.h
> > > b/drivers/platform/x86/intel/pmc/core.h
> > > index 4a94a4ee031e6..d8c7b28493055 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.h
> > > +++ b/drivers/platform/x86/intel/pmc/core.h
> > > @@ -29,6 +29,10 @@ struct telem_endpoint;
> > >   #define LPM_REG_COUNT		28
> > >   #define LPM_MODE_OFFSET		1
> > >   +/* Telemetry Endpoint Info bits */
> > > +#define SUB_REQ_LPM		0x01	/* Substate requirement for
> > > low power mode requirements */
> > > +#define SUB_REQ_BLK		0x02	/* Substate requirement for
> > > S0ix blockers */
> > > +
> > >   /* Sunrise Point Power Management Controller PCI Device ID */
> > >   #define SPT_PMC_PCI_DEVICE_ID			0x9d21
> > >   #define SPT_PMC_BASE_ADDR_OFFSET		0x48
> > > @@ -344,6 +348,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 +375,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;
> > > @@ -404,6 +412,7 @@ struct pmc_info {
> > >    * @map:		pointer to pmc_reg_map struct that contains platform
> > >    *			specific attributes
> > >    * @lpm_req_regs:	List of substate requirements
> > > + * @blk_sub_req_reqs:	List of registers showing substate
> > > requirements for S0ix blockers
> > >    * @ltr_ign:		Holds LTR ignore data while suspended
> > >    *
> > >    * pmc contains info about one power management controller device.
> > > @@ -413,6 +422,7 @@ struct pmc {
> > >   	void __iomem *regbase;
> > >   	const struct pmc_reg_map *map;
> > >   	u32 *lpm_req_regs;
> > > +	u32 *blk_sub_req_regs;
> > >   	u32 ltr_ign;
> > >   };
> > >   @@ -468,6 +478,7 @@ enum pmc_index {
> > >   /**
> > >    * struct pmc_dev_info - Structure to keep PMC device info
> > >    * @pci_func:		Function number of the primary PMC
> > > + * @telem_info:		Bitmask to indicate which telemetry info is
> > > available
> > >    * @dmu_guid:		Die Management Unit GUID
> > >    * @regmap_list:	Pointer to a list of pmc_info structure that could be
> > >    *			available for the platform. When set, this
> > > field implies
> > > @@ -480,6 +491,7 @@ enum pmc_index {
> > >    */
> > >   struct pmc_dev_info {
> > >   	u8 pci_func;
> > > +	u8 telem_info;
> > >   	u32 dmu_guid;
> > >   	struct pmc_info *regmap_list;
> > >   	const struct pmc_reg_map *map;
> > > diff --git a/drivers/platform/x86/intel/pmc/lnl.c
> > > b/drivers/platform/x86/intel/pmc/lnl.c
> > > index e08a77c778c2c..ec9e79f6cd913 100644
> > > --- a/drivers/platform/x86/intel/pmc/lnl.c
> > > +++ b/drivers/platform/x86/intel/pmc/lnl.c
> > > @@ -572,6 +572,7 @@ 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,
> > > +	.telem_info = SUB_REQ_LPM,
> > >   	.regmap_list = lnl_pmc_info_list,
> > >   	.map = &lnl_socm_reg_map,
> > >   	.suspend = cnl_suspend,
> > > diff --git a/drivers/platform/x86/intel/pmc/mtl.c
> > > b/drivers/platform/x86/intel/pmc/mtl.c
> > > index faa13a7ee688f..c58a871e2e0df 100644
> > > --- a/drivers/platform/x86/intel/pmc/mtl.c
> > > +++ b/drivers/platform/x86/intel/pmc/mtl.c
> > > @@ -994,6 +994,7 @@ static int mtl_core_init(struct pmc_dev *pmcdev,
> > > struct pmc_dev_info *pmc_dev_in
> > >     struct pmc_dev_info mtl_pmc_dev = {
> > >   	.pci_func = 2,
> > > +	.telem_info = SUB_REQ_LPM,
> > >   	.dmu_guid = MTL_PMT_DMU_GUID,
> > >   	.regmap_list = mtl_pmc_info_list,
> > >   	.map = &mtl_socm_reg_map,
> > > 
> 

-- 
 i.

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

[Xi Pardee]

Hi Ilpo,

Thanks for reviewing the patch. My response is in line.

On 6/27/2025 1:08 AM, Ilpo Järvinen wrote:
> On Thu, 26 Jun 2025, Xi Pardee wrote:
>
>> Hi Ilpo,
>>
>> Thanks for reviewing the patch. My response is inline.
>>
>> On 6/26/2025 9:38 AM, Ilpo Järvinen wrote:
>>> On Tue, 24 Jun 2025, Xi Pardee wrote:
>>>
>>>> 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.
>>> Empty line.
>> Will fix in next version.
>>>> 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 <xi.pardee@linux.intel.com>
>>>> ---
>>>>    drivers/platform/x86/intel/pmc/arl.c  |   2 +
>>>>    drivers/platform/x86/intel/pmc/core.c | 111 ++++++++++++++++++++++++--
>>>>    drivers/platform/x86/intel/pmc/core.h |  12 +++
>>>>    drivers/platform/x86/intel/pmc/lnl.c  |   1 +
>>>>    drivers/platform/x86/intel/pmc/mtl.c  |   1 +
>>>>    5 files changed, 121 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/intel/pmc/arl.c
>>>> b/drivers/platform/x86/intel/pmc/arl.c
>>>> index 9d66d65e75779..1852876be234f 100644
>>>> --- a/drivers/platform/x86/intel/pmc/arl.c
>>>> +++ b/drivers/platform/x86/intel/pmc/arl.c
>>>> @@ -722,6 +722,7 @@ static int arl_h_core_init(struct pmc_dev *pmcdev,
>>>> struct pmc_dev_info *pmc_dev_
>>>>      struct pmc_dev_info arl_pmc_dev = {
>>>>    	.pci_func = 0,
>>>> +	.telem_info = SUB_REQ_LPM,
>>>>    	.dmu_guid = ARL_PMT_DMU_GUID,
>>>>    	.regmap_list = arl_pmc_info_list,
>>>>    	.map = &arl_socs_reg_map,
>>>> @@ -732,6 +733,7 @@ struct pmc_dev_info arl_pmc_dev = {
>>>>      struct pmc_dev_info arl_h_pmc_dev = {
>>>>    	.pci_func = 2,
>>>> +	.telem_info = SUB_REQ_LPM,
>>>>    	.dmu_guid = ARL_PMT_DMU_GUID,
>>>>    	.regmap_list = arl_pmc_info_list,
>>>>    	.map = &mtl_socm_reg_map,
>>>> diff --git a/drivers/platform/x86/intel/pmc/core.c
>>>> b/drivers/platform/x86/intel/pmc/core.c
>>>> index 47cc5120e7dd6..e9d281b064462 100644
>>>> --- a/drivers/platform/x86/intel/pmc/core.c
>>>> +++ b/drivers/platform/x86/intel/pmc/core.c
>>>> @@ -844,6 +844,56 @@ static void pmc_core_substate_req_header_show(struct
>>>> seq_file *s, int pmc_index,
>>>>    		seq_printf(s, " %9s |\n", name);
>>>>    }
>>>>    +static int pmc_core_substate_blk_req_show(struct seq_file *s, void
>>>> *unused)
>>>> +{
>>>> +	struct pmc_dev *pmcdev = s->private;
>>>> +	unsigned int pmc_index;
>>>> +	u32 *blk_sub_req_regs;
>>> Why is this here and the other variable in the inner block?
>> Will move the blk_sub_req_regs declaration to the inner block.
>>>> +
>>>> +	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;
>>>> +		struct pmc *pmc;
>>>> +
>>>> +		pmc = pmcdev->pmcs[pmc_index];
>>>> +		if (!pmc || !pmc->blk_sub_req_regs)
>>>> +			continue;
>>>> +
>>>> +		blk_sub_req_regs = pmc->blk_sub_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, "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++) {
>>>> +				int mode;
>>>> +
>>>> +				if (!map->blk)
>>>> +					continue;
>>>> +
>>>> +				counter = pmc_core_reg_read(pmc, offset);
>>>> +				seq_printf(s, "pmc%d: %34s |", pmc_index,
>>>> map->name);
>>>> +				pmc_for_each_mode(mode, pmcdev) {
>>>> +					bool required = *blk_sub_req_regs &
>>>> BIT(mode);
>>>> +
>>>> +					seq_printf(s, " %9s |", required ?
>>>> "Required" : " ");
>>>> +				}
>>>> +				seq_printf(s, " %9d |\n", counter);
>>>> +				offset += map->blk * S0IX_BLK_SIZE;
>>>> +				blk_sub_req_regs++;
>>>> +			}
>>>> +		}
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_blk_req);
>>>> +
>>>>    static int pmc_core_substate_req_regs_show(struct seq_file *s, void
>>>> *unused)
>>>>    {
>>>>    	struct pmc_dev *pmcdev = s->private;
>>>> @@ -1335,7 +1385,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev
>>>> *pmcdev)
>>>>    		debugfs_create_file("substate_requirements", 0444,
>>>>    				    pmcdev->dbgfs_dir, pmcdev,
>>>>    				    &pmc_core_substate_req_regs_fops);
>>>> -	}
>>>> +	} else if (primary_pmc->blk_sub_req_regs)
>>>> +		debugfs_create_file("substate_requirements", 0444,
>>>> +				    pmcdev->dbgfs_dir, pmcdev,
>>>> +				    &pmc_core_substate_blk_req_fops);
>>>>
>>>>    	if (primary_pmc->map->pson_residency_offset &&
>>>> pmc_core_is_pson_residency_enabled(pmcdev)) {
>>>>    		debugfs_create_file("pson_residency_usec", 0444,
>>>> @@ -1441,7 +1494,38 @@ 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)
>>>> +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;
>>>> +	unsigned int index;
>>>> +	u32 *req_offset;
>>>> +	int ret;
>>>> +
>>>> +	num_blocker = pmc->map->num_s0ix_blocker;
>>>> +	sample_id = pmc->map->blocker_req_offset;
>>>> +
>>>> +	pmc->blk_sub_req_regs = devm_kcalloc(&pmcdev->pdev->dev,
>>>> +					 num_blocker, sizeof(u32),
>>>> +					 GFP_KERNEL);
>>> Correct alignment.
>>>
>>> I guess you want to keep those two args on the same line as this would fit
>>> on 2 lines.
>> Will change the alignment and move two args to be on one line.
>>>> +	if (!pmc->blk_sub_req_regs)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	req_offset = pmc->blk_sub_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++;
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, int func,
>>>> unsigned int telem_info)
>>>>    {
>>>>    	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
>>>>    	struct telem_endpoint *ep;
>>>> @@ -1470,13 +1554,26 @@ 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);
>>>> +		if (telem_info & SUB_REQ_LPM) {
>>>> +			ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
>>>> +			if (ret)
>>>> +				goto unregister_ep;
>>>> +		}
>>>> +
>>>> +		if (telem_info & SUB_REQ_BLK) {
>>> These two are mutually exclusive? I'm really wondering cuold pointers be
>>> used instead to avoid these ifs here and in debugfs fops selection.
>> The SUB_REQ_BLK and SUB_REQ_LPM are mutually exclusive. We will also introduce
>> more
>> types of data from the telemetry endpoint but not mutually exclusive in the
>> next series, therefore
>> using bit to identify the type of data sounds appropriate to me. I am not sure
>> how to use pointer to
>> select data type in this scenario, Could you please give an example?
> Perhaps there isn't, I didn't have a crystal ball what's coming after this
> series at hand. :-)
>
> I just noted there are these mutually exclusive pointers:
>
> debugfs_create_file("substate_requirements", ..., &pmc_core_substate_req_regs_fops);
> vs
> debugfs_create_file("substate_requirements", ..., &pmc_core_substate_blk_req_fops);
>
> and
>
> ret = pmc_core_pmt_get_lpm_req(pmcdev, pmc, ep);
> vs
> ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
>
> As both look like they're decided based on what platform we're running on,
> it looks similar to what pmc_dev_info already contains, having these as
> pointers in pmc_dev_info would avoid having to create those two branches
> just to use a different pointer.
>
> (BTW, those two places also use a different way to decide which one to
> use, which feels slightly inconsistent).
>
>
> Whether using pointers in pmc_dev_info makes sense given the extra info
> you provided now, I'm not any more entirely sure.
I understand that function pointer is a good solution here to avoid 
using the if branches as these two data types
are mutual exclusive. But this is a special case, we will be introducing 
new data types, which are not mutual exclusive.
In that case, it will be better to use a bit position to represent each 
data type available for the specific platforms.

For example, the next data type is bdf and we will add bdf to core.h and 
core.c:

#define BDF_INFO        0x04    /* Get BDF information */

if (telem_info & BDF_INFO) {
             ret = pmc_core_pmt_get_bdf(pmcdev, pmc, ep);
             if (ret)
                     goto unregister_ep;

}

We use register pointers to differentiate these two cases instead of 
telem_info when creating debugfs file. This is to
keep it consistent with other file creations in 
pmc_core_dbgfs_register(). I could change it to use telem_info in next
version.

Thanks!

Xi

>>>> +			ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
>>>> +			if (ret)
>>>> +				goto unregister_ep;
>>>> +		}
>>>> +
>>>>    		pmt_telem_unregister_endpoint(ep);
>>>> -		if (ret)
>>>> -			return ret;
>>>>    	}
>>>>      	return 0;
>>>> +
>>>> +unregister_ep:
>>>> +	pmt_telem_unregister_endpoint(ep);
>>>> +	return ret;
>>>>    }
>>>>      static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info
>>>> *list, u16 devid)
>>>> @@ -1585,7 +1682,9 @@ 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->pci_func,
>>>> +					      pmc_dev_info->telem_info);
>>>>    		if (ret)
>>>>    			goto unmap_regbase;
>>>>    	}
>>>> diff --git a/drivers/platform/x86/intel/pmc/core.h
>>>> b/drivers/platform/x86/intel/pmc/core.h
>>>> index 4a94a4ee031e6..d8c7b28493055 100644
>>>> --- a/drivers/platform/x86/intel/pmc/core.h
>>>> +++ b/drivers/platform/x86/intel/pmc/core.h
>>>> @@ -29,6 +29,10 @@ struct telem_endpoint;
>>>>    #define LPM_REG_COUNT		28
>>>>    #define LPM_MODE_OFFSET		1
>>>>    +/* Telemetry Endpoint Info bits */
>>>> +#define SUB_REQ_LPM		0x01	/* Substate requirement for
>>>> low power mode requirements */
>>>> +#define SUB_REQ_BLK		0x02	/* Substate requirement for
>>>> S0ix blockers */
>>>> +
>>>>    /* Sunrise Point Power Management Controller PCI Device ID */
>>>>    #define SPT_PMC_PCI_DEVICE_ID			0x9d21
>>>>    #define SPT_PMC_BASE_ADDR_OFFSET		0x48
>>>> @@ -344,6 +348,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 +375,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;
>>>> @@ -404,6 +412,7 @@ struct pmc_info {
>>>>     * @map:		pointer to pmc_reg_map struct that contains platform
>>>>     *			specific attributes
>>>>     * @lpm_req_regs:	List of substate requirements
>>>> + * @blk_sub_req_reqs:	List of registers showing substate
>>>> requirements for S0ix blockers
>>>>     * @ltr_ign:		Holds LTR ignore data while suspended
>>>>     *
>>>>     * pmc contains info about one power management controller device.
>>>> @@ -413,6 +422,7 @@ struct pmc {
>>>>    	void __iomem *regbase;
>>>>    	const struct pmc_reg_map *map;
>>>>    	u32 *lpm_req_regs;
>>>> +	u32 *blk_sub_req_regs;
>>>>    	u32 ltr_ign;
>>>>    };
>>>>    @@ -468,6 +478,7 @@ enum pmc_index {
>>>>    /**
>>>>     * struct pmc_dev_info - Structure to keep PMC device info
>>>>     * @pci_func:		Function number of the primary PMC
>>>> + * @telem_info:		Bitmask to indicate which telemetry info is
>>>> available
>>>>     * @dmu_guid:		Die Management Unit GUID
>>>>     * @regmap_list:	Pointer to a list of pmc_info structure that could be
>>>>     *			available for the platform. When set, this
>>>> field implies
>>>> @@ -480,6 +491,7 @@ enum pmc_index {
>>>>     */
>>>>    struct pmc_dev_info {
>>>>    	u8 pci_func;
>>>> +	u8 telem_info;
>>>>    	u32 dmu_guid;
>>>>    	struct pmc_info *regmap_list;
>>>>    	const struct pmc_reg_map *map;
>>>> diff --git a/drivers/platform/x86/intel/pmc/lnl.c
>>>> b/drivers/platform/x86/intel/pmc/lnl.c
>>>> index e08a77c778c2c..ec9e79f6cd913 100644
>>>> --- a/drivers/platform/x86/intel/pmc/lnl.c
>>>> +++ b/drivers/platform/x86/intel/pmc/lnl.c
>>>> @@ -572,6 +572,7 @@ 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,
>>>> +	.telem_info = SUB_REQ_LPM,
>>>>    	.regmap_list = lnl_pmc_info_list,
>>>>    	.map = &lnl_socm_reg_map,
>>>>    	.suspend = cnl_suspend,
>>>> diff --git a/drivers/platform/x86/intel/pmc/mtl.c
>>>> b/drivers/platform/x86/intel/pmc/mtl.c
>>>> index faa13a7ee688f..c58a871e2e0df 100644
>>>> --- a/drivers/platform/x86/intel/pmc/mtl.c
>>>> +++ b/drivers/platform/x86/intel/pmc/mtl.c
>>>> @@ -994,6 +994,7 @@ static int mtl_core_init(struct pmc_dev *pmcdev,
>>>> struct pmc_dev_info *pmc_dev_in
>>>>      struct pmc_dev_info mtl_pmc_dev = {
>>>>    	.pci_func = 2,
>>>> +	.telem_info = SUB_REQ_LPM,
>>>>    	.dmu_guid = MTL_PMT_DMU_GUID,
>>>>    	.regmap_list = mtl_pmc_info_list,
>>>>    	.map = &mtl_socm_reg_map,
>>>>

[PATCH v2 5/5] platform/x86:intel/pmc: Enable SSRAM support for Panther Lake

[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 <xi.pardee@linux.intel.com>
---
 drivers/platform/x86/intel/pmc/core.h |  2 ++
 drivers/platform/x86/intel/pmc/ptl.c  | 30 +++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index d8c7b28493055..cdb32f2203cff 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -301,6 +301,8 @@ 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
 
 /* SSRAM PMC Device ID */
 /* LNL */
diff --git a/drivers/platform/x86/intel/pmc/ptl.c b/drivers/platform/x86/intel/pmc/ptl.c
index 394515af60d60..48be79b4e769f 100644
--- a/drivers/platform/x86/intel/pmc/ptl.c
+++ b/drivers/platform/x86/intel/pmc/ptl.c
@@ -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,22 @@ 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,
+};
+
+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,6 +570,9 @@ 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,
+	.telem_info = SUB_REQ_BLK,
+	.regmap_list = ptl_pmc_info_list,
 	.map = &ptl_pcdp_reg_map,
 	.suspend = cnl_suspend,
 	.resume = ptl_resume,
-- 
2.43.0

Re: [PATCH v2 5/5] platform/x86:intel/pmc: Enable SSRAM support for Panther Lake

[Ilpo Järvinen]

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

On Tue, 24 Jun 2025, Xi Pardee wrote:

> 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 <xi.pardee@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/core.h |  2 ++
>  drivers/platform/x86/intel/pmc/ptl.c  | 30 +++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index d8c7b28493055..cdb32f2203cff 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -301,6 +301,8 @@ 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
>  
>  /* SSRAM PMC Device ID */
>  /* LNL */
> diff --git a/drivers/platform/x86/intel/pmc/ptl.c b/drivers/platform/x86/intel/pmc/ptl.c
> index 394515af60d60..48be79b4e769f 100644
> --- a/drivers/platform/x86/intel/pmc/ptl.c
> +++ b/drivers/platform/x86/intel/pmc/ptl.c
> @@ -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,22 @@ 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,
> +};
> +
> +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,6 +570,9 @@ 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,
> +	.telem_info = SUB_REQ_BLK,
> +	.regmap_list = ptl_pmc_info_list,
>  	.map = &ptl_pcdp_reg_map,
>  	.suspend = cnl_suspend,
>  	.resume = ptl_resume,
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.