Skip to contentSkip to conversation

Create Intel PMC SSRAM Telemetry driver

PATCH Superseded Xi Pardee 2 participants 12 messages Apr 9, 2025 – Apr 15, 2025

Subsystems (2)
x86 platform drivers Intel PMC core driver

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Xi Pardee

author maintainer
#0 lore raw

v3 cover Create Intel PMC SSRAM Telemetry driver

This patch series removes the SSRAM support from Intel PMC Core driver and creates a separate PCI driver for SSRAM device. The new Intel PMC SSRAM driver provides the following functionalities:

1. Search and store the PMC information in a structure, including PWRMBASE address and devid for each available PMC. Then Intel PMC Core driver achieves the PMC information using the API provided by the new driver.

2. Search and register Intel Platform Monitoring Techology telemetry regions so they would by available for read through sysfs and Intel PMT API. Intel PMC Core driver can achieve Low Power Mode requirement information from a telemetry region registered by the new driver.

The above functionalities was previously handled by Intel PMC Core driver. Intel PMC Core driver returns -EPROBE_DEFER when trying to read data from a telem region that is not available yet. This setup may result in an infinite loop of .probe() calls as Intel PMC Core driver creates child devices. Creating a separate PCI driver avoids the infinite loop possibility.

v3->v2:
- Add memory barriers to the new driver to ensure write/read order of device_probed variable.
- Minor grammar changes: add needed white space and end of life new line.
- Add patch to move error handling to init function.
- Remove patch to enable SSRAM support of LNL platforms. This patch will be included in a separate series.

v2->v1:
- Rearrange and restructure patches completely based on feedback from v1
- Here are the patches:

Preparation for the new SSRAM Telemetry driver:
platform/x86:intel/pmc: Move PMC Core related functions platform/x86:intel/pmc: Rename core_ssram to ssram_telemetry platform/x86:intel/pmc: Move PMC devid to core.h

Minor bug fix:
platform/x86:intel/pmc: Convert index variables to be unsigned platform/x86:intel/pmc: Remove unneeded h file inclusion platform/x86:intel/pmc: Remove unneeded io operations

Create new driver and convert PMC Core to use the API:
platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver

Enhancement:
platform/x86:intel/pmc: Move error handling to init function

Xi Pardee (8):
platform/x86:intel/pmc: Move PMC Core related functions platform/x86:intel/pmc: Rename core_ssram to ssram_telemetry platform/x86:intel/pmc: Move PMC devid to core.h platform/x86:intel/pmc: Convert index variables to be unsigned platform/x86:intel/pmc: Remove unneeded header file inclusion platform/x86:intel/pmc: Remove unneeded io operations platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver platform/x86:intel/pmc: Move error handling to init function

 drivers/platform/x86/intel/pmc/Kconfig        |  14 +
 drivers/platform/x86/intel/pmc/Makefile       |   8 +-
 drivers/platform/x86/intel/pmc/arl.c          |  13 +-
 drivers/platform/x86/intel/pmc/core.c         | 238 ++++++++++++-
 drivers/platform/x86/intel/pmc/core.h         |  22 +-
 drivers/platform/x86/intel/pmc/core_ssram.c   | 332 ------------------
 drivers/platform/x86/intel/pmc/mtl.c          |  10 +-
 .../platform/x86/intel/pmc/ssram_telemetry.c  | 195 ++++++++++
 .../platform/x86/intel/pmc/ssram_telemetry.h  |  35 ++
 9 files changed, 501 insertions(+), 366 deletions(-)
 delete mode 100644 drivers/platform/x86/intel/pmc/core_ssram.c
 create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.c
 create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h
Signature · 1 line
2.43.0
author maintainer
#1 lore raw

PATCH v3 1/8 platform/x86:intel/pmc: Move PMC Core related functions

Move functions that implements PMC Core feature from core_ssram.c to core.c. This patch is a preparation step to introduce a new SSRAM Telemetry driver for the SSRAM device.

Signed-off-by Xi Pardee <[email protected]>
Reviewed-by Ilpo Järvinen <[email protected]>
---
 drivers/platform/x86/intel/pmc/core.c       | 168 +++++++++++++++++++
 drivers/platform/x86/intel/pmc/core.h       |   9 +-
 drivers/platform/x86/intel/pmc/core_ssram.c | 175 --------------------
 3 files changed, 176 insertions(+), 176 deletions(-)
drivers/platform/x86/intel/pmc/core.c +168 -0
index 7a1d11f2914f..a42dc62d70da 100644
@@ -1345,6 +1345,173 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 	}
 }
 
+static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *map)
+{
+	for (; list->map; ++list)
+		if (list->map == map)
+			return list->guid;
+
+	return 0;
+}
+
+static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
+{
+	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(pmcdev->ssram_pcidev, guid, 0);
+	if (IS_ERR(ep)) {
+		dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
+			PTR_ERR(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;
+	}
+
+	/*
+	 * PMC Low Power Mode (LPM) table
+	 *
+	 * In telemetry space, the LPM table contains a 4 byte header followed
+	 * by 8 consecutive mode blocks (one for each LPM mode). Each block
+	 * has a 4 byte header followed by a set of registers that describe the
+	 * IP state requirements for the given mode. The IP mapping is platform
+	 * specific but the same for each block, making for easy analysis.
+	 * Platforms only use a subset of the space to track the requirements
+	 * for their IPs. Callers provide the requirement registers they use as
+	 * a list of indices. Each requirement register is associated with an
+	 * IP map that's maintained by the caller.
+	 *
+	 * Header
+	 * +----+----------------------------+----------------------------+
+	 * |  0 |      REVISION              |      ENABLED MODES         |
+	 * +----+--------------+-------------+-------------+--------------+
+	 *
+	 * Low Power Mode 0 Block
+	 * +----+--------------+-------------+-------------+--------------+
+	 * |  1 |     SUB ID   |     SIZE    |   MAJOR     |   MINOR      |
+	 * +----+--------------+-------------+-------------+--------------+
+	 * |  2 |           LPM0 Requirements 0                           |
+	 * +----+---------------------------------------------------------+
+	 * |    |                  ...                                    |
+	 * +----+---------------------------------------------------------+
+	 * | 29 |           LPM0 Requirements 27                          |
+	 * +----+---------------------------------------------------------+
+	 *
+	 * ...
+	 *
+	 * Low Power Mode 7 Block
+	 * +----+--------------+-------------+-------------+--------------+
+	 * |    |     SUB ID   |     SIZE    |   MAJOR     |   MINOR      |
+	 * +----+--------------+-------------+-------------+--------------+
+	 * | 60 |           LPM7 Requirements 0                           |
+	 * +----+---------------------------------------------------------+
+	 * |    |                  ...                                    |
+	 * +----+---------------------------------------------------------+
+	 * | 87 |           LPM7 Requirements 27                          |
+	 * +----+---------------------------------------------------------+
+	 *
+	 */
+	mode_offset = LPM_HEADER_OFFSET + LPM_MODE_OFFSET;
+	pmc_for_each_mode(mode, pmcdev) {
+		u32 *req_offset = pmc->lpm_req_regs + (mode * num_maps);
+		int m;
+
+		for (m = 0; m < num_maps; m++) {
+			u8 sample_id = lpm_indices[m] + mode_offset;
+
+			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);
+				devm_kfree(&pmcdev->pdev->dev, pmc->lpm_req_regs);
+				goto unregister_ep;
+			}
+			++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 ret, i;
+
+	if (!pmcdev->ssram_pcidev)
+		return -ENODEV;
+
+	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
+		if (!pmcdev->pmcs[i])
+			continue;
+
+		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
+{
+	for (; list->map; ++list)
+		if (devid == list->devid)
+			return list->map;
+
+	return NULL;
+}
+
+int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
+		     const struct pmc_reg_map *reg_map, int pmc_index)
+{
+	struct pmc *pmc = pmcdev->pmcs[pmc_index];
+
+	if (!pwrm_base)
+		return -ENODEV;
+
+	/* Memory for primary PMC has been allocated in core.c */
+	if (!pmc) {
+		pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
+		if (!pmc)
+			return -ENOMEM;
+	}
+
+	pmc->map = reg_map;
+	pmc->base_addr = pwrm_base;
+	pmc->regbase = ioremap(pmc->base_addr, pmc->map->regmap_length);
+
+	if (!pmc->regbase) {
+		devm_kfree(&pmcdev->pdev->dev, pmc);
+		return -ENOMEM;
+	}
+
+	pmcdev->pmcs[pmc_index] = pmc;
+
+	return 0;
+}
+
 /*
  * When supported, ssram init is used to achieve all available PMCs.
  * If ssram init fails, this function uses legacy method to at least get the

@@ -1719,5 +1886,6 @@ static struct platform_driver pmc_core_driver = {
 
 module_platform_driver(pmc_core_driver);
 
+MODULE_IMPORT_NS("INTEL_PMT_TELEMETRY");
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Intel PMC Core Driver");
drivers/platform/x86/intel/pmc/core.h +8 -1
index 945a1c440cca..09aac6daabbd 100644
@@ -24,6 +24,11 @@ struct telem_endpoint;
 #define MAX_NUM_PMC			3
 #define S0IX_BLK_SIZE			4
 
+/* PCH query */
+#define LPM_HEADER_OFFSET	1
+#define LPM_REG_COUNT		28
+#define LPM_MODE_OFFSET		1
+
 /* Sunrise Point Power Management Controller PCI Device ID */
 #define SPT_PMC_PCI_DEVICE_ID			0x9d21
 #define SPT_PMC_BASE_ADDR_OFFSET		0x48

@@ -485,7 +490,6 @@ extern const struct pmc_reg_map mtl_socm_reg_map;
 extern const struct pmc_reg_map mtl_ioep_reg_map;
 
 void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
-int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev);
 int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore);
 
 int pmc_core_resume_common(struct pmc_dev *pmcdev);

@@ -497,6 +501,9 @@ void pmc_core_set_device_d3(unsigned int device);
 int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func);
 
 int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
+const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid);
+int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
+		     const struct pmc_reg_map *reg_map, int pmc_index);
 
 extern struct pmc_dev_info spt_pmc_dev;
 extern struct pmc_dev_info cnp_pmc_dev;
drivers/platform/x86/intel/pmc/core_ssram.c +0 -175

203 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer
#2 lore raw

Re: [PATCH v3 1/8] platform/x86:intel/pmc: Move PMC Core related functions

Quoted 40 lines · Xi Pardee

Move functions that implements PMC Core feature from core_ssram.c to core.c. This patch is a preparation step to introduce a new SSRAM Telemetry driver for the SSRAM device.

Signed-off-by Xi Pardee <[email protected]>
Reviewed-by Ilpo Järvinen <[email protected]>
---
 drivers/platform/x86/intel/pmc/core.c       | 168 +++++++++++++++++++
 drivers/platform/x86/intel/pmc/core.h       |   9 +-
 drivers/platform/x86/intel/pmc/core_ssram.c | 175 --------------------
 3 files changed, 176 insertions(+), 176 deletions(-)
drivers/platform/x86/intel/pmc/core.c +20 -0
index 7a1d11f2914f..a42dc62d70da 100644
@@ -1345,6 +1345,173 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 	}
 }
 
+static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *map)
+{
+	for (; list->map; ++list)
+		if (list->map == map)
+			return list->guid;
+
+	return 0;
+}
+
+static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
+{
+	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;

drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) · line 1377

Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10 -0
+	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(pmcdev->ssram_pcidev, guid, 0);
+	if (IS_ERR(ep)) {
+		dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
+			PTR_ERR(ep));

It seems I started to suggest changes into a move only patch (so those suggestions below would be better to implement as separate patches).

Please use %pe.

Quoted 44 lines
drivers/platform/x86/intel/pmc/core.c +44 -0
+		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;
+	}
+
+	/*
+	 * PMC Low Power Mode (LPM) table
+	 *
+	 * In telemetry space, the LPM table contains a 4 byte header followed
+	 * by 8 consecutive mode blocks (one for each LPM mode). Each block
+	 * has a 4 byte header followed by a set of registers that describe the
+	 * IP state requirements for the given mode. The IP mapping is platform
+	 * specific but the same for each block, making for easy analysis.
+	 * Platforms only use a subset of the space to track the requirements
+	 * for their IPs. Callers provide the requirement registers they use as
+	 * a list of indices. Each requirement register is associated with an
+	 * IP map that's maintained by the caller.
+	 *
+	 * Header
+	 * +----+----------------------------+----------------------------+
+	 * |  0 |      REVISION              |      ENABLED MODES         |
+	 * +----+--------------+-------------+-------------+--------------+
+	 *
+	 * Low Power Mode 0 Block
+	 * +----+--------------+-------------+-------------+--------------+
+	 * |  1 |     SUB ID   |     SIZE    |   MAJOR     |   MINOR      |
+	 * +----+--------------+-------------+-------------+--------------+
+	 * |  2 |           LPM0 Requirements 0                           |
+	 * +----+---------------------------------------------------------+
+	 * |    |                  ...                                    |
+	 * +----+---------------------------------------------------------+
+	 * | 29 |           LPM0 Requirements 27                          |
+	 * +----+---------------------------------------------------------+
+	 *
+	 * ...
+	 *
+	 * Low Power Mode 7 Block
+	 * +----+--------------+-------------+-------------+--------------+

drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) · line 1431

Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10 -0
+	 * |    |     SUB ID   |     SIZE    |   MAJOR     |   MINOR      |
+	 * +----+--------------+-------------+-------------+--------------+
+	 * | 60 |           LPM7 Requirements 0                           |
+	 * +----+---------------------------------------------------------+
+	 * |    |                  ...                                    |
+	 * +----+---------------------------------------------------------+
+	 * | 87 |           LPM7 Requirements 27                          |
+	 * +----+---------------------------------------------------------+
+	 *
+	 */

It would be better to have such a long comment above the function as now it breaks the entire function badly in half.

drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) · line 1444

Quoted 13 lines
drivers/platform/x86/intel/pmc/core.c +13 -0
+	mode_offset = LPM_HEADER_OFFSET + LPM_MODE_OFFSET;
+	pmc_for_each_mode(mode, pmcdev) {
+		u32 *req_offset = pmc->lpm_req_regs + (mode * num_maps);
+		int m;
+
+		for (m = 0; m < num_maps; m++) {
+			u8 sample_id = lpm_indices[m] + mode_offset;
+
+			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);
+				devm_kfree(&pmcdev->pdev->dev, pmc->lpm_req_regs);

Won't the error propagate and devm_ will take care of freeing? Why is it done explictly here?

Quoted 12 lines
drivers/platform/x86/intel/pmc/core.c +12 -0
+				goto unregister_ep;
+			}
+			++req_offset;
+		}
+		mode_offset += LPM_REG_COUNT + LPM_MODE_OFFSET;
+	}
+
+unregister_ep:
+	pmt_telem_unregister_endpoint(ep);
+
+	return ret;
+}
Signature · 1 line
 i.
Quoted 314 lines
drivers/platform/x86/intel/pmc/core.c +59 -0
+
+static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
+{
+	int ret, i;
+
+	if (!pmcdev->ssram_pcidev)
+		return -ENODEV;
+
+	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
+		if (!pmcdev->pmcs[i])
+			continue;
+
+		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
+{
+	for (; list->map; ++list)
+		if (devid == list->devid)
+			return list->map;
+
+	return NULL;
+}
+
+int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
+		     const struct pmc_reg_map *reg_map, int pmc_index)
+{
+	struct pmc *pmc = pmcdev->pmcs[pmc_index];
+
+	if (!pwrm_base)
+		return -ENODEV;
+
+	/* Memory for primary PMC has been allocated in core.c */
+	if (!pmc) {
+		pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
+		if (!pmc)
+			return -ENOMEM;
+	}
+
+	pmc->map = reg_map;
+	pmc->base_addr = pwrm_base;
+	pmc->regbase = ioremap(pmc->base_addr, pmc->map->regmap_length);
+
+	if (!pmc->regbase) {
+		devm_kfree(&pmcdev->pdev->dev, pmc);
+		return -ENOMEM;
+	}
+
+	pmcdev->pmcs[pmc_index] = pmc;
+
+	return 0;
+}
+
 /*
  * When supported, ssram init is used to achieve all available PMCs.
  * If ssram init fails, this function uses legacy method to at least get the

@@ -1719,5 +1886,6 @@ static struct platform_driver pmc_core_driver = {
 
 module_platform_driver(pmc_core_driver);
 
+MODULE_IMPORT_NS("INTEL_PMT_TELEMETRY");
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Intel PMC Core Driver");
drivers/platform/x86/intel/pmc/core.h +8 -1
index 945a1c440cca..09aac6daabbd 100644
@@ -24,6 +24,11 @@ struct telem_endpoint;
 #define MAX_NUM_PMC			3
 #define S0IX_BLK_SIZE			4
 
+/* PCH query */
+#define LPM_HEADER_OFFSET	1
+#define LPM_REG_COUNT		28
+#define LPM_MODE_OFFSET		1
+
 /* Sunrise Point Power Management Controller PCI Device ID */
 #define SPT_PMC_PCI_DEVICE_ID			0x9d21
 #define SPT_PMC_BASE_ADDR_OFFSET		0x48

@@ -485,7 +490,6 @@ extern const struct pmc_reg_map mtl_socm_reg_map;
 extern const struct pmc_reg_map mtl_ioep_reg_map;
 
 void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
-int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev);
 int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore);
 
 int pmc_core_resume_common(struct pmc_dev *pmcdev);

@@ -497,6 +501,9 @@ void pmc_core_set_device_d3(unsigned int device);
 int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func);
 
 int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
+const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid);
+int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
+		     const struct pmc_reg_map *reg_map, int pmc_index);
 
 extern struct pmc_dev_info spt_pmc_dev;
 extern struct pmc_dev_info cnp_pmc_dev;
drivers/platform/x86/intel/pmc/core_ssram.c +0 -175
index 739569803017..e1a83425d802 100644
@@ -14,7 +14,6 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 
 #include "core.h"
-#include "../pmt/telemetry.h"
 
 #define SSRAM_HDR_SIZE		0x100
 #define SSRAM_PWRM_OFFSET	0x14

@@ -24,142 +23,8 @@
 #define SSRAM_IOE_OFFSET	0x68
 #define SSRAM_DEVID_OFFSET	0x70
 
-/* PCH query */
-#define LPM_HEADER_OFFSET	1
-#define LPM_REG_COUNT		28
-#define LPM_MODE_OFFSET		1
-
 DEFINE_FREE(pmc_core_iounmap, void __iomem *, if (_T) iounmap(_T))
 
-static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *map)
-{
-	for (; list->map; ++list)
-		if (list->map == map)
-			return list->guid;
-
-	return 0;
-}
-
-static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
-{
-	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(pmcdev->ssram_pcidev, guid, 0);
-	if (IS_ERR(ep)) {
-		dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
-			PTR_ERR(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;
-	}
-
-	/*
-	 * PMC Low Power Mode (LPM) table
-	 *
-	 * In telemetry space, the LPM table contains a 4 byte header followed
-	 * by 8 consecutive mode blocks (one for each LPM mode). Each block
-	 * has a 4 byte header followed by a set of registers that describe the
-	 * IP state requirements for the given mode. The IP mapping is platform
-	 * specific but the same for each block, making for easy analysis.
-	 * Platforms only use a subset of the space to track the requirements
-	 * for their IPs. Callers provide the requirement registers they use as
-	 * a list of indices. Each requirement register is associated with an
-	 * IP map that's maintained by the caller.
-	 *
-	 * Header
-	 * +----+----------------------------+----------------------------+
-	 * |  0 |      REVISION              |      ENABLED MODES         |
-	 * +----+--------------+-------------+-------------+--------------+
-	 *
-	 * Low Power Mode 0 Block
-	 * +----+--------------+-------------+-------------+--------------+
-	 * |  1 |     SUB ID   |     SIZE    |   MAJOR     |   MINOR      |
-	 * +----+--------------+-------------+-------------+--------------+
-	 * |  2 |           LPM0 Requirements 0                           |
-	 * +----+---------------------------------------------------------+
-	 * |    |                  ...                                    |
-	 * +----+---------------------------------------------------------+
-	 * | 29 |           LPM0 Requirements 27                          |
-	 * +----+---------------------------------------------------------+
-	 *
-	 * ...
-	 *
-	 * Low Power Mode 7 Block
-	 * +----+--------------+-------------+-------------+--------------+
-	 * |    |     SUB ID   |     SIZE    |   MAJOR     |   MINOR      |
-	 * +----+--------------+-------------+-------------+--------------+
-	 * | 60 |           LPM7 Requirements 0                           |
-	 * +----+---------------------------------------------------------+
-	 * |    |                  ...                                    |
-	 * +----+---------------------------------------------------------+
-	 * | 87 |           LPM7 Requirements 27                          |
-	 * +----+---------------------------------------------------------+
-	 *
-	 */
-	mode_offset = LPM_HEADER_OFFSET + LPM_MODE_OFFSET;
-	pmc_for_each_mode(mode, pmcdev) {
-		u32 *req_offset = pmc->lpm_req_regs + (mode * num_maps);
-		int m;
-
-		for (m = 0; m < num_maps; m++) {
-			u8 sample_id = lpm_indices[m] + mode_offset;
-
-			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);
-				devm_kfree(&pmcdev->pdev->dev, pmc->lpm_req_regs);
-				goto unregister_ep;
-			}
-			++req_offset;
-		}
-		mode_offset += LPM_REG_COUNT + LPM_MODE_OFFSET;
-	}
-
-unregister_ep:
-	pmt_telem_unregister_endpoint(ep);
-
-	return ret;
-}
-
-int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
-{
-	int ret, i;
-
-	if (!pmcdev->ssram_pcidev)
-		return -ENODEV;
-
-	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
-		if (!pmcdev->pmcs[i])
-			continue;
-
-		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 static void
 pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
 {

@@ -203,50 +68,11 @@ pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
 	intel_vsec_register(pcidev, &info);
 }
 
-static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
-{
-	for (; list->map; ++list)
-		if (devid == list->devid)
-			return list->map;
-
-	return NULL;
-}
-
 static inline u64 get_base(void __iomem *addr, u32 offset)
 {
 	return lo_hi_readq(addr + offset) & GENMASK_ULL(63, 3);
 }
 
-static int
-pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		 const struct pmc_reg_map *reg_map, int pmc_index)
-{
-	struct pmc *pmc = pmcdev->pmcs[pmc_index];
-
-	if (!pwrm_base)
-		return -ENODEV;
-
-	/* Memory for primary PMC has been allocated in core.c */
-	if (!pmc) {
-		pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
-		if (!pmc)
-			return -ENOMEM;
-	}
-
-	pmc->map = reg_map;
-	pmc->base_addr = pwrm_base;
-	pmc->regbase = ioremap(pmc->base_addr, pmc->map->regmap_length);
-
-	if (!pmc->regbase) {
-		devm_kfree(&pmcdev->pdev->dev, pmc);
-		return -ENOMEM;
-	}
-
-	pmcdev->pmcs[pmc_index] = pmc;
-
-	return 0;
-}
-
 static int
 pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, int pmc_idx, u32 offset)
 {

@@ -329,4 +155,3 @@ int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func)
 	return ret;
 }
 MODULE_IMPORT_NS("INTEL_VSEC");
-MODULE_IMPORT_NS("INTEL_PMT_TELEMETRY");
author maintainer
#3 lore raw

PATCH v3 2/8 platform/x86:intel/pmc: Rename core_ssram to ssram_telemetry

In reply to Xi Pardee

Rename core_ssram.c to ssram_telemetry.c. This patch is a preparation step to introduce a new SSRAM Telemetry driver for the SSRAM device.

Signed-off-by Xi Pardee <[email protected]>
Reviewed-by Ilpo Järvinen <[email protected]>
---
 drivers/platform/x86/intel/pmc/Makefile                         | 2 +-
 .../platform/x86/intel/pmc/{core_ssram.c => ssram_telemetry.c}  | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/platform/x86/intel/pmc/{core_ssram.c => ssram_telemetry.c} (100%)
drivers/platform/x86/intel/pmc/Makefile +1 -1
index b148b40d09f5..e842647d3ced 100644
@@ -3,7 +3,7 @@
 # Intel x86 Platform-Specific Drivers
 #
 
-intel_pmc_core-y			:= core.o core_ssram.o spt.o cnp.o \
+intel_pmc_core-y			:= core.o ssram_telemetry.o spt.o cnp.o \
 					   icl.o tgl.o adl.o mtl.o arl.o lnl.o ptl.o
 obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core.o
 intel_pmc_core_pltdrv-y			:= pltdrv.o
drivers/platform/x86/intel/pmc/ssram_telemetry.c renamed from drivers/platform/x86/intel/pmc/core_ssram.c +0 -0
similarity index 100%
rename from drivers/platform/x86/intel/pmc/core_ssram.c
rename to drivers/platform/x86/intel/pmc/ssram_telemetry.c
Signature · 1 line
2.43.0
author maintainer
#4 lore raw

PATCH v3 3/8 platform/x86:intel/pmc: Move PMC devid to core.h

In reply to Xi Pardee

Move PMC devid definition for each PMC of Arrow Lake and Meteor Lake platforms to core.h. This patch is a preparation step to introduce a new SSRAM Telemetry driver which will be using the PMC devid.

Signed-off-by Xi Pardee <[email protected]>
Reviewed-by Ilpo Järvinen <[email protected]>
---
 drivers/platform/x86/intel/pmc/arl.c  | 12 ++++--------
 drivers/platform/x86/intel/pmc/core.h | 12 ++++++++++++
 drivers/platform/x86/intel/pmc/mtl.c  |  9 +++------
 3 files changed, 19 insertions(+), 14 deletions(-)
drivers/platform/x86/intel/pmc/arl.c +4 -8
index 320993bd6d31..a6b2eb850ff0 100644
@@ -651,29 +651,25 @@ static const struct pmc_reg_map arl_pchs_reg_map = {
 	.etr3_offset = ETR3_OFFSET,
 };
 
-#define PMC_DEVID_SOCM 0x777f
-#define PMC_DEVID_SOCS 0xae7f
-#define PMC_DEVID_IOEP 0x7ecf
-#define PMC_DEVID_PCHS 0x7f27
 static struct pmc_info arl_pmc_info_list[] = {
 	{
 		.guid	= IOEP_LPM_REQ_GUID,
-		.devid	= PMC_DEVID_IOEP,
+		.devid	= PMC_DEVID_ARL_IOEP,
 		.map	= &mtl_ioep_reg_map,
 	},
 	{
 		.guid	= SOCS_LPM_REQ_GUID,
-		.devid	= PMC_DEVID_SOCS,
+		.devid	= PMC_DEVID_ARL_SOCS,
 		.map	= &arl_socs_reg_map,
 	},
 	{
 		.guid	= PCHS_LPM_REQ_GUID,
-		.devid	= PMC_DEVID_PCHS,
+		.devid	= PMC_DEVID_ARL_PCHS,
 		.map	= &arl_pchs_reg_map,
 	},
 	{
 		.guid	= SOCM_LPM_REQ_GUID,
-		.devid	= PMC_DEVID_SOCM,
+		.devid	= PMC_DEVID_ARL_SOCM,
 		.map	= &mtl_socm_reg_map,
 	},
 	{}
drivers/platform/x86/intel/pmc/core.h +12 -0
index 09aac6daabbd..ce1b949b1004 100644
@@ -298,6 +298,18 @@ enum ppfear_regs {
 #define PTL_PMC_LTR_CUR_PLT			0x1C2C
 #define PTL_PCD_PMC_MMIO_REG_LEN		0x31A8
 
+/* SSRAM PMC Device ID */
+/* ARL */
+#define PMC_DEVID_ARL_SOCM	0x777f
+#define PMC_DEVID_ARL_SOCS	0xae7f
+#define PMC_DEVID_ARL_IOEP	0x7ecf
+#define PMC_DEVID_ARL_PCHS	0x7f27
+
+/* MTL */
+#define PMC_DEVID_MTL_SOCM	0x7e7f
+#define PMC_DEVID_MTL_IOEP	0x7ecf
+#define PMC_DEVID_MTL_IOEM	0x7ebf
+
 extern const char *pmc_lpm_modes[];
 
 struct pmc_bit_map {
drivers/platform/x86/intel/pmc/mtl.c +3 -6
index 8862829694a7..8f1b01657277 100644
@@ -947,23 +947,20 @@ static const struct pmc_reg_map mtl_ioem_reg_map = {
 	.lpm_reg_index = MTL_LPM_REG_INDEX,
 };
 
-#define PMC_DEVID_SOCM	0x7e7f
-#define PMC_DEVID_IOEP	0x7ecf
-#define PMC_DEVID_IOEM	0x7ebf
 static struct pmc_info mtl_pmc_info_list[] = {
 	{
 		.guid	= SOCP_LPM_REQ_GUID,
-		.devid	= PMC_DEVID_SOCM,
+		.devid	= PMC_DEVID_MTL_SOCM,
 		.map	= &mtl_socm_reg_map,
 	},
 	{
 		.guid	= IOEP_LPM_REQ_GUID,
-		.devid	= PMC_DEVID_IOEP,
+		.devid	= PMC_DEVID_MTL_IOEP,
 		.map	= &mtl_ioep_reg_map,
 	},
 	{
 		.guid	= IOEM_LPM_REQ_GUID,
-		.devid	= PMC_DEVID_IOEM,
+		.devid	= PMC_DEVID_MTL_IOEM,
 		.map	= &mtl_ioem_reg_map
 	},
 	{}
Signature · 1 line
2.43.0
author maintainer
#5 lore raw

PATCH v3 4/8 platform/x86:intel/pmc: Convert index variables to be unsigned

In reply to Xi Pardee

Convert the index variables type to be unsigned to avoid confusion and error.

Signed-off-by Xi Pardee <[email protected]>
Reviewed-by Ilpo Järvinen <[email protected]>
---
 drivers/platform/x86/intel/pmc/core.c            | 5 +++--
 drivers/platform/x86/intel/pmc/core.h            | 2 +-
 drivers/platform/x86/intel/pmc/ssram_telemetry.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)
drivers/platform/x86/intel/pmc/core.c +3 -2
index a42dc62d70da..a53a7677122c 100644
@@ -1457,7 +1457,8 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
 
 static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
 {
-	int ret, i;
+	unsigned int i;
+	int ret;
 
 	if (!pmcdev->ssram_pcidev)
 		return -ENODEV;

@@ -1484,7 +1485,7 @@ const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
 }
 
 int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		     const struct pmc_reg_map *reg_map, int pmc_index)
+		     const struct pmc_reg_map *reg_map, unsigned int pmc_index)
 {
 	struct pmc *pmc = pmcdev->pmcs[pmc_index];
drivers/platform/x86/intel/pmc/core.h +1 -1
index ce1b949b1004..c3b07075d017 100644
@@ -515,7 +515,7 @@ int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func);
 int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
 const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid);
 int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		     const struct pmc_reg_map *reg_map, int pmc_index);
+		     const struct pmc_reg_map *reg_map, unsigned int pmc_index);
 
 extern struct pmc_dev_info spt_pmc_dev;
 extern struct pmc_dev_info cnp_pmc_dev;
drivers/platform/x86/intel/pmc/ssram_telemetry.c +1 -1
index e1a83425d802..10ead4398a68 100644
@@ -74,7 +74,7 @@ static inline u64 get_base(void __iomem *addr, u32 offset)
 }
 
 static int
-pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, int pmc_idx, u32 offset)
+pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, unsigned int pmc_idx, u32 offset)
 {
 	struct pci_dev *ssram_pcidev = pmcdev->ssram_pcidev;
 	void __iomem __free(pmc_core_iounmap) *tmp_ssram = NULL;
Signature · 1 line
2.43.0
author maintainer
#6 lore raw

PATCH v3 5/8 platform/x86:intel/pmc: Remove unneeded header file inclusion

In reply to Xi Pardee

telemetry.h header file from PMT is not needed in arl.c or mtl.c. Remove the cross-driver include to avoid confusion.

Signed-off-by Xi Pardee <[email protected]>
Reviewed-by Ilpo Järvinen <[email protected]>
---
 drivers/platform/x86/intel/pmc/arl.c | 1 -
 drivers/platform/x86/intel/pmc/mtl.c | 1 -
 2 files changed, 2 deletions(-)
drivers/platform/x86/intel/pmc/arl.c +0 -1
index a6b2eb850ff0..0708221d3af1 100644
@@ -10,7 +10,6 @@
 
 #include <linux/pci.h>
 #include "core.h"
-#include "../pmt/telemetry.h"
 
 /* PMC SSRAM PMT Telemetry GUID */
 #define IOEP_LPM_REQ_GUID	0x5077612
drivers/platform/x86/intel/pmc/mtl.c +0 -1
index 8f1b01657277..faa13a7ee688 100644
@@ -10,7 +10,6 @@
 
 #include <linux/pci.h>
 #include "core.h"
-#include "../pmt/telemetry.h"
 
 /* PMC SSRAM PMT Telemetry GUIDS */
 #define SOCP_LPM_REQ_GUID	0x2625030
Signature · 1 line
2.43.0
author maintainer
#7 lore raw

PATCH v3 6/8 platform/x86:intel/pmc: Remove unneeded io operations

In reply to Xi Pardee

Remove ioremap and iounmap operations that are not needed. ioremap and iounmap operations are handled by the caller of the pmc_add_pmt function.

Signed-off-by Xi Pardee <[email protected]>
---
 drivers/platform/x86/intel/pmc/ssram_telemetry.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/platform/x86/intel/pmc/ssram_telemetry.c +0 -6
index 10ead4398a68..7b8443092b20 100644
@@ -36,13 +36,7 @@ pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
 	u32 dvsec_offset;
 	u32 table, hdr;
 
-	ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
-	if (!ssram)
-		return;
-
 	dvsec_offset = readl(ssram + SSRAM_DVSEC_OFFSET);
-	iounmap(ssram);
-
 	dvsec = ioremap(ssram_base + dvsec_offset, SSRAM_DVSEC_SIZE);
 	if (!dvsec)
 		return;
Signature · 1 line
2.43.0
author maintainer
#8 lore raw

PATCH v3 7/8 platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver

In reply to Xi Pardee

Convert ssram device related functionalities to a new driver named Intel PMC SSRAM Telemetry driver. Modify PMC Core driver to use API exported by the driver to discover and achieve devid and PWRMBASE address information for each available PMC. PMC Core driver needs to get PCI device when reading from telemetry regions.

The new SSRAM driver binds to the SSRAM device and provides the following functionalities:
1. Look for and register telemetry regions available in SSRAM device.
2. Provide devid and PWRMBASE address information for the corresponding PMCs.

Signed-off-by Xi Pardee <[email protected]>
---
 drivers/platform/x86/intel/pmc/Kconfig        |  14 ++
 drivers/platform/x86/intel/pmc/Makefile       |   8 +-
 drivers/platform/x86/intel/pmc/core.c         |  80 +++++++----
 drivers/platform/x86/intel/pmc/core.h         |   7 -
 .../platform/x86/intel/pmc/ssram_telemetry.c  | 132 ++++++++++++------
 .../platform/x86/intel/pmc/ssram_telemetry.h  |  35 +++++
 6 files changed, 198 insertions(+), 78 deletions(-)
 create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h
drivers/platform/x86/intel/pmc/Kconfig +14 -0
index d2f651fbec2c..f5fb1bdc266e 100644
@@ -8,6 +8,7 @@ config INTEL_PMC_CORE
 	depends on PCI
 	depends on ACPI
 	depends on INTEL_PMT_TELEMETRY
+	select INTEL_PMC_SSRAM_TELEMETRY
 	help
 	  The Intel Platform Controller Hub for Intel Core SoCs provides access
 	  to Power Management Controller registers via various interfaces. This

@@ -24,3 +25,16 @@ config INTEL_PMC_CORE
 		- SLPS0 Debug registers (Cannonlake/Icelake PCH)
 		- Low Power Mode registers (Tigerlake and beyond)
 		- PMC quirks as needed to enable SLPS0/S0ix
+
+config INTEL_PMC_SSRAM_TELEMETRY
+	tristate
+	help
+	  The PMC SSRAM device contains counters structured in Intel Platform
+	  Monitoring Techology (PMT) telemetry regions. The driver  provides
+	  API to expose information of PMCs available in the platform. This
+	  driver also looks for and register telemetry regions so they would
+	  be available for read through sysfs and Intel PMT API.
+
+	  The driver needs INTEL_VSEC driver to register for telemetry regions.
+	  This requirement has been fulfilled by INTEL_PMC_CORE driver which
+	  selects INTEL_PMC_SSRAM_TELEMETRY driver.
drivers/platform/x86/intel/pmc/Makefile +6 -2
index e842647d3ced..5f68c8503a56 100644
@@ -3,8 +3,12 @@
 # Intel x86 Platform-Specific Drivers
 #
 
-intel_pmc_core-y			:= core.o ssram_telemetry.o spt.o cnp.o \
-					   icl.o tgl.o adl.o mtl.o arl.o lnl.o ptl.o
+intel_pmc_core-y			:= core.o spt.o cnp.o icl.o \
+					   tgl.o adl.o mtl.o arl.o lnl.o ptl.o
 obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core.o
 intel_pmc_core_pltdrv-y			:= pltdrv.o
 obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core_pltdrv.o
+
+# Intel PMC SSRAM driver
+intel_pmc_ssram_telemetry-y		+= ssram_telemetry.o
+obj-$(CONFIG_INTEL_PMC_SSRAM_TELEMETRY)	+= intel_pmc_ssram_telemetry.o
drivers/platform/x86/intel/pmc/core.c +55 -25

163 lines not on this page · raw message

drivers/platform/x86/intel/pmc/core.h +0 -7
index c3b07075d017..e136d18b1d38 100644
@@ -413,7 +413,6 @@ struct pmc {
  * struct pmc_dev - pmc device structure
  * @devs:		pointer to an array of pmc pointers
  * @pdev:		pointer to platform_device struct
- * @ssram_pcidev:	pointer to pci device struct for the PMC SSRAM
  * @crystal_freq:	crystal frequency from cpuid
  * @dbgfs_dir:		path to debugfs interface
  * @pmc_xram_read_bit:	flag to indicate whether PMC XRAM shadow registers

@@ -433,7 +432,6 @@ struct pmc_dev {
 	struct pmc *pmcs[MAX_NUM_PMC];
 	struct dentry *dbgfs_dir;
 	struct platform_device *pdev;
-	struct pci_dev *ssram_pcidev;
 	unsigned int crystal_freq;
 	int pmc_xram_read_bit;
 	struct mutex lock; /* generic mutex lock for PMC Core */

@@ -510,12 +508,7 @@ void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev);
 void pmc_core_punit_pmt_init(struct pmc_dev *pmcdev, u32 guid);
 void pmc_core_set_device_d3(unsigned int device);
 
-int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func);
-
 int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
-const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid);
-int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		     const struct pmc_reg_map *reg_map, unsigned int pmc_index);
 
 extern struct pmc_dev_info spt_pmc_dev;
 extern struct pmc_dev_info cnp_pmc_dev;
drivers/platform/x86/intel/pmc/ssram_telemetry.c +88 -44

190 lines not on this page · raw message

drivers/platform/x86/intel/pmc/ssram_telemetry.h new file +35 -0
new file mode 100644
index 000000000000..459a29efebea
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Intel PMC SSRAM Telemetry PCI Driver Header File
+ *
+ * Copyright (c) 2024, Intel Corporation.
+ */
+
+#ifndef PMC_SSRAM_H
+#define PMC_SSRAM_H
+
+/**
+ * struct pmc_ssram_telemetry - Structure to keep pmc info in ssram device
+ * @devid:		device id of the pmc device
+ * @base_addr:		contains PWRM base address
+ */
+struct pmc_ssram_telemetry {
+	u16 devid;
+	u64 base_addr;
+};
+
+/**
+ * pmc_ssram_telemetry_get_pmc_info() - Get a PMC devid and base_addr information
+ * @pmc_idx:               Index of the PMC
+ * @pmc_ssram_telemetry:   pmc_ssram_telemetry structure to store the PMC information
+ *
+ * Return:
+ * * 0           - Success
+ * * -EAGAIN     - Probe function has not finished yet. Try again.
+ * * -EINVAL     - Invalid pmc_idx
+ * * -ENODEV     - PMC device is not available
+ */
+int pmc_ssram_telemetry_get_pmc_info(unsigned int pmc_idx,
+				     struct pmc_ssram_telemetry *pmc_ssram_telemetry);
+
+#endif /* PMC_SSRAM_H */
Signature · 1 line
2.43.0
maintainer
#9 lore raw

Re: [PATCH v3 7/8] platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver

Quoted 6 lines · Xi Pardee

Convert ssram device related functionalities to a new driver named Intel PMC SSRAM Telemetry driver. Modify PMC Core driver to use API exported by the driver to discover and achieve devid and PWRMBASE address information for each available PMC. PMC Core driver needs to get PCI device when reading from telemetry regions.

Quoted 7 lines

The new SSRAM driver binds to the SSRAM device and provides the following functionalities:
1. Look for and register telemetry regions available in SSRAM device.
2. Provide devid and PWRMBASE address information for the corresponding PMCs.

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

I took patches 1-6 into review-ilpo-next now, but please consider the making the changes I suggested to 1/8 as separate patches.

This one has a few things that should be addressed before inclusion.

Quoted 29 lines
---
 drivers/platform/x86/intel/pmc/Kconfig        |  14 ++
 drivers/platform/x86/intel/pmc/Makefile       |   8 +-
 drivers/platform/x86/intel/pmc/core.c         |  80 +++++++----
 drivers/platform/x86/intel/pmc/core.h         |   7 -
 .../platform/x86/intel/pmc/ssram_telemetry.c  | 132 ++++++++++++------
 .../platform/x86/intel/pmc/ssram_telemetry.h  |  35 +++++
 6 files changed, 198 insertions(+), 78 deletions(-)
 create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h
drivers/platform/x86/intel/pmc/Kconfig +4 -0
index d2f651fbec2c..f5fb1bdc266e 100644
@@ -8,6 +8,7 @@ config INTEL_PMC_CORE
 	depends on PCI
 	depends on ACPI
 	depends on INTEL_PMT_TELEMETRY
+	select INTEL_PMC_SSRAM_TELEMETRY
 	help
 	  The Intel Platform Controller Hub for Intel Core SoCs provides access
 	  to Power Management Controller registers via various interfaces. This

@@ -24,3 +25,16 @@ config INTEL_PMC_CORE
 		- SLPS0 Debug registers (Cannonlake/Icelake PCH)
 		- Low Power Mode registers (Tigerlake and beyond)
 		- PMC quirks as needed to enable SLPS0/S0ix
+
+config INTEL_PMC_SSRAM_TELEMETRY
+	tristate

drivers/platform/x86/intel/pmc/Kconfig config INTEL_PMC_CORE · line 40

Quoted 10 lines
drivers/platform/x86/intel/pmc/Kconfig +10 -0
+	help
+	  The PMC SSRAM device contains counters structured in Intel Platform
+	  Monitoring Techology (PMT) telemetry regions. The driver  provides
+	  API to expose information of PMCs available in the platform. This
+	  driver also looks for and register telemetry regions so they would
+	  be available for read through sysfs and Intel PMT API.
+
+	  The driver needs INTEL_VSEC driver to register for telemetry regions.
+	  This requirement has been fulfilled by INTEL_PMC_CORE driver which
+	  selects INTEL_PMC_SSRAM_TELEMETRY driver.

???

INTEL_PMC_CORE does not select/depend on INTEL_VSEC. I know it "works" indirectly through INTEL_PMT_TELEMETRY but a) this Kconfig help text is wrong/misleading, b) the way the dependencies are arranged seems fragile to me.

What prevents user from enabling INTEL_PMC_SSRAM_TELEMETRY without INTEL_PMC_CORE? This will likely blow up a randconfig build at some point when stars happen to align right.

Should the help text just be removed? I mean, shouldn't this be selected by INTEL_PMC_CORE and never should be chosen on its own by the user?

Quoted 65 lines
drivers/platform/x86/intel/pmc/Makefile +6 -2
index e842647d3ced..5f68c8503a56 100644
@@ -3,8 +3,12 @@
 # Intel x86 Platform-Specific Drivers
 #
 
-intel_pmc_core-y			:= core.o ssram_telemetry.o spt.o cnp.o \
-					   icl.o tgl.o adl.o mtl.o arl.o lnl.o ptl.o
+intel_pmc_core-y			:= core.o spt.o cnp.o icl.o \
+					   tgl.o adl.o mtl.o arl.o lnl.o ptl.o
 obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core.o
 intel_pmc_core_pltdrv-y			:= pltdrv.o
 obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core_pltdrv.o
+
+# Intel PMC SSRAM driver
+intel_pmc_ssram_telemetry-y		+= ssram_telemetry.o
+obj-$(CONFIG_INTEL_PMC_SSRAM_TELEMETRY)	+= intel_pmc_ssram_telemetry.o
drivers/platform/x86/intel/pmc/core.c +8 -5
index a53a7677122c..26c1c75b709a 100644
@@ -29,6 +29,7 @@
 #include <asm/tsc.h>
 
 #include "core.h"
+#include "ssram_telemetry.h"
 #include "../pmt/telemetry.h"
 
 /* Maximum number of modes supported by platfoms that has low power mode capability */

@@ -1354,7 +1355,7 @@ static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *m
 	return 0;
 }
 
-static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
+static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct pci_dev *pcidev)
 {
 	struct telem_endpoint *ep;
 	const u8 *lpm_indices;

@@ -1371,7 +1372,7 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
 	if (!guid)
 		return -ENXIO;
 
-	ep = pmt_telem_find_and_register_endpoint(pmcdev->ssram_pcidev, guid, 0);
+	ep = pmt_telem_find_and_register_endpoint(pcidev, guid, 0);
 	if (IS_ERR(ep)) {
 		dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
 			PTR_ERR(ep));

@@ -1455,27 +1456,30 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
 	return ret;
 }
 
-static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
+static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev, int func)
 {
+	struct pci_dev *pcidev;
 	unsigned int i;
-	int ret;
+	int ret = 0;
 
-	if (!pmcdev->ssram_pcidev)
+	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));
+	if (!pcidev)
 		return -ENODEV;

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

Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +2 -2
 
 	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
 		if (!pmcdev->pmcs[i])
 			continue;
 
-		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
+		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i], pcidev);
 		if (ret)
-			return ret;
+			break;

DEFINE_FREE for pci_dev_put exists so you can do __free(pci_dev_put), so you can use cleanup.h to handle the cleanup and keep returning directly from here.

Quoted 218 lines
drivers/platform/x86/intel/pmc/core.c +45 -18
 	}
 
-	return 0;
+	pci_dev_put(pcidev);
+	return ret;
 }
 
-const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
+static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
 {
 	for (; list->map; ++list)
 		if (devid == list->devid)

@@ -1484,23 +1488,32 @@ const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
 	return NULL;
 }
 
-int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		     const struct pmc_reg_map *reg_map, unsigned int pmc_index)
+static int pmc_core_pmc_add(struct pmc_dev *pmcdev, unsigned int pmc_index)
+
 {
-	struct pmc *pmc = pmcdev->pmcs[pmc_index];
+	struct pmc_ssram_telemetry pmc_ssram_telemetry;
+	const struct pmc_reg_map *map;
+	struct pmc *pmc;
+	int ret;
+
+	ret = pmc_ssram_telemetry_get_pmc_info(pmc_index, &pmc_ssram_telemetry);
+	if (ret)
+		return ret;
 
-	if (!pwrm_base)
+	map = pmc_core_find_regmap(pmcdev->regmap_list, pmc_ssram_telemetry.devid);
+	if (!map)
 		return -ENODEV;
 
-	/* Memory for primary PMC has been allocated in core.c */
+	pmc = pmcdev->pmcs[pmc_index];
+	/* Memory for primary PMC has been allocated */
 	if (!pmc) {
 		pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
 		if (!pmc)
 			return -ENOMEM;
 	}
 
-	pmc->map = reg_map;
-	pmc->base_addr = pwrm_base;
+	pmc->map = map;
+	pmc->base_addr = pmc_ssram_telemetry.base_addr;
 	pmc->regbase = ioremap(pmc->base_addr, pmc->map->regmap_length);
 
 	if (!pmc->regbase) {

@@ -1513,6 +1526,20 @@ int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
 	return 0;
 }
 
+static int pmc_core_ssram_get_reg_base(struct pmc_dev *pmcdev)
+{
+	int ret;
+
+	ret = pmc_core_pmc_add(pmcdev, PMC_IDX_MAIN);
+	if (ret)
+		return ret;
+
+	pmc_core_pmc_add(pmcdev, PMC_IDX_IOE);
+	pmc_core_pmc_add(pmcdev, PMC_IDX_PCH);
+
+	return 0;
+}
+
 /*
  * When supported, ssram init is used to achieve all available PMCs.
  * If ssram init fails, this function uses legacy method to at least get the

@@ -1530,10 +1557,18 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 	ssram = pmc_dev_info->regmap_list != NULL;
 	if (ssram) {
 		pmcdev->regmap_list = pmc_dev_info->regmap_list;
-		ret = pmc_core_ssram_init(pmcdev, pmc_dev_info->pci_func);
+		ret = pmc_core_ssram_get_reg_base(pmcdev);
+		/*
+		 * EAGAIN error code indicates Intel PMC SSRAM Telemetry driver
+		 * has not finished probe and PMC info is not available yet. Try
+		 * again later.
+		 */
+		if (ret == -EAGAIN)
+			return -EPROBE_DEFER;
+
 		if (ret) {
 			dev_warn(&pmcdev->pdev->dev,
-				 "ssram init failed, %d, using legacy init\n", ret);
+				 "Failed to get PMC info from SSRAM, %d, using legacy init\n", ret);
 			ssram = false;
 		}
 	}

@@ -1550,7 +1585,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)
-		return pmc_core_ssram_get_lpm_reqs(pmcdev);
+		return pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func);
 
 	return 0;
 }

@@ -1639,15 +1674,10 @@ static void pmc_core_clean_structure(struct platform_device *pdev)
 	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
 		struct pmc *pmc = pmcdev->pmcs[i];
 
-		if (pmc)
+		if (pmc && pmc->regbase)
 			iounmap(pmc->regbase);
 	}
 
-	if (pmcdev->ssram_pcidev) {
-		pci_dev_put(pmcdev->ssram_pcidev);
-		pci_disable_device(pmcdev->ssram_pcidev);
-	}
-
 	if (pmcdev->punit_ep)
 		pmt_telem_unregister_endpoint(pmcdev->punit_ep);
drivers/platform/x86/intel/pmc/core.h +0 -7
index c3b07075d017..e136d18b1d38 100644
@@ -413,7 +413,6 @@ struct pmc {
  * struct pmc_dev - pmc device structure
  * @devs:		pointer to an array of pmc pointers
  * @pdev:		pointer to platform_device struct
- * @ssram_pcidev:	pointer to pci device struct for the PMC SSRAM
  * @crystal_freq:	crystal frequency from cpuid
  * @dbgfs_dir:		path to debugfs interface
  * @pmc_xram_read_bit:	flag to indicate whether PMC XRAM shadow registers

@@ -433,7 +432,6 @@ struct pmc_dev {
 	struct pmc *pmcs[MAX_NUM_PMC];
 	struct dentry *dbgfs_dir;
 	struct platform_device *pdev;
-	struct pci_dev *ssram_pcidev;
 	unsigned int crystal_freq;
 	int pmc_xram_read_bit;
 	struct mutex lock; /* generic mutex lock for PMC Core */

@@ -510,12 +508,7 @@ void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev);
 void pmc_core_punit_pmt_init(struct pmc_dev *pmcdev, u32 guid);
 void pmc_core_set_device_d3(unsigned int device);
 
-int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func);
-
 int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
-const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid);
-int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		     const struct pmc_reg_map *reg_map, unsigned int pmc_index);
 
 extern struct pmc_dev_info spt_pmc_dev;
 extern struct pmc_dev_info cnp_pmc_dev;
drivers/platform/x86/intel/pmc/ssram_telemetry.c +10 -13
index 7b8443092b20..232263008030 100644
@@ -1,19 +1,18 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * This file contains functions to handle discovery of PMC metrics located
- * in the PMC SSRAM PCI device.
+ * Intel PMC SSRAM TELEMETRY PCI Driver
  *
  * Copyright (c) 2023, Intel Corporation.
- * All Rights Reserved.
- *
  */
 
 #include <linux/cleanup.h>
 #include <linux/intel_vsec.h>
 #include <linux/pci.h>
+#include <linux/types.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 
 #include "core.h"
+#include "ssram_telemetry.h"
 
 #define SSRAM_HDR_SIZE		0x100
 #define SSRAM_PWRM_OFFSET	0x14

@@ -23,12 +22,14 @@
 #define SSRAM_IOE_OFFSET	0x68
 #define SSRAM_DEVID_OFFSET	0x70
 
-DEFINE_FREE(pmc_core_iounmap, void __iomem *, if (_T) iounmap(_T))
+DEFINE_FREE(pmc_ssram_telemetry_iounmap, void __iomem *, if (_T) iounmap(_T))
+
+static struct pmc_ssram_telemetry *pmc_ssram_telems;
+static bool device_probed;
 
 static void
-pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
+pmc_ssram_telemetry_add_pmt(struct pci_dev *pcidev, u64 ssram_base, void __iomem *ssram)
 {
-	struct pci_dev *pcidev = pmcdev->ssram_pcidev;
 	struct intel_vsec_platform_info info = {};
 	struct intel_vsec_header *headers[2] = {};
 	struct intel_vsec_header header;

@@ -57,7 +58,7 @@ pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
 	info.caps = VSEC_CAP_TELEMETRY;
 	info.headers = headers;
 	info.base_addr = ssram_base;
-	info.parent = &pmcdev->pdev->dev;
+	info.parent = &pcidev->dev;
 
 	intel_vsec_register(pcidev, &info);
 }

@@ -68,19 +69,14 @@ static inline u64 get_base(void __iomem *addr, u32 offset)
 }
 
 static int
-pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, unsigned int pmc_idx, u32 offset)
+pmc_ssram_telemetry_get_pmc(struct pci_dev *pcidev, unsigned int pmc_idx, u32 offset)
 {
-	struct pci_dev *ssram_pcidev = pmcdev->ssram_pcidev;
-	void __iomem __free(pmc_core_iounmap) *tmp_ssram = NULL;
-	void __iomem __free(pmc_core_iounmap) *ssram = NULL;
-	const struct pmc_reg_map *map;

drivers/platform/x86/intel/pmc/ssram_telemetry.c static inline u64 get_base(void __iomem *addr, u32 offset) · line 79

Quoted 10 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.c +3 -4
+	void __iomem __free(pmc_ssram_telemetry_iounmap) * tmp_ssram = NULL;
+	void __iomem __free(pmc_ssram_telemetry_iounmap) * ssram = NULL;
 	u64 ssram_base, pwrm_base;
 	u16 devid;
 
-	if (!pmcdev->regmap_list)
-		return -ENOENT;
-
-	ssram_base = ssram_pcidev->resource[0].start;
+	ssram_base = pcidev->resource[0].start;

pci_resource_start()

Quoted 151 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.c +75 -27
 	tmp_ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
 	if (!tmp_ssram)
 		return -ENOMEM;

@@ -107,45 +103,93 @@ pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, unsigned int pmc_idx, u32 offset)
 	devid = readw(ssram + SSRAM_DEVID_OFFSET);
 
 	/* Find and register and PMC telemetry entries */
-	pmc_add_pmt(pmcdev, ssram_base, ssram);
+	pmc_ssram_telemetry_add_pmt(pcidev, ssram_base, ssram);
 
-	map = pmc_core_find_regmap(pmcdev->regmap_list, devid);
-	if (!map)
+	pmc_ssram_telems[pmc_idx].devid = devid;
+	pmc_ssram_telems[pmc_idx].base_addr = pwrm_base;
+
+	return 0;
+}
+
+int pmc_ssram_telemetry_get_pmc_info(unsigned int pmc_idx,
+				     struct pmc_ssram_telemetry *pmc_ssram_telemetry)
+{
+	/*
+	 * PMCs are discovered in probe function. If this function is called before
+	 * probe function complete, the result would be invalid. Use device_probed
+	 * variable to avoid this case. Return -EAGAIN to inform the consumer to call
+	 * again later.
+	 */
+	if (!device_probed)
+		return -EAGAIN;
+
+	/*
+	 * Memory barrier is used to ensure the correct read order between
+	 * device_probed variable and PMC info.
+	 */
+	smp_rmb();
+	if (pmc_idx >= MAX_NUM_PMC)
+		return -EINVAL;
+
+	if (!pmc_ssram_telems || !pmc_ssram_telems[pmc_idx].devid)
 		return -ENODEV;
 
-	return pmc_core_pmc_add(pmcdev, pwrm_base, map, pmc_idx);
+	pmc_ssram_telemetry->devid = pmc_ssram_telems[pmc_idx].devid;
+	pmc_ssram_telemetry->base_addr = pmc_ssram_telems[pmc_idx].base_addr;
+	return 0;
 }
+EXPORT_SYMBOL_GPL(pmc_ssram_telemetry_get_pmc_info);
 
-int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func)
+static int intel_pmc_ssram_telemetry_probe(struct pci_dev *pcidev, const struct pci_device_id *id)
 {
-	struct pci_dev *pcidev;
 	int ret;
 
-	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));
-	if (!pcidev)
-		return -ENODEV;
+	pmc_ssram_telems = devm_kzalloc(&pcidev->dev, sizeof(*pmc_ssram_telems) * MAX_NUM_PMC,
+					GFP_KERNEL);
+	if (!pmc_ssram_telems) {
+		ret = -ENOMEM;
+		goto probe_finish;
+	}
 
 	ret = pcim_enable_device(pcidev);
-	if (ret)
-		goto release_dev;
-
-	pmcdev->ssram_pcidev = pcidev;
+	if (ret) {
+		dev_dbg(&pcidev->dev, "failed to enable PMC SSRAM device\n");
+		goto probe_finish;
+	}
 
-	ret = pmc_core_ssram_get_pmc(pmcdev, PMC_IDX_MAIN, 0);
+	ret = pmc_ssram_telemetry_get_pmc(pcidev, PMC_IDX_MAIN, 0);
 	if (ret)
-		goto disable_dev;
-
-	pmc_core_ssram_get_pmc(pmcdev, PMC_IDX_IOE, SSRAM_IOE_OFFSET);
-	pmc_core_ssram_get_pmc(pmcdev, PMC_IDX_PCH, SSRAM_PCH_OFFSET);
-
-	return 0;
-
-disable_dev:
-	pmcdev->ssram_pcidev = NULL;
-	pci_disable_device(pcidev);
-release_dev:
-	pci_dev_put(pcidev);
-
+		goto probe_finish;
+
+	pmc_ssram_telemetry_get_pmc(pcidev, PMC_IDX_IOE, SSRAM_IOE_OFFSET);
+	pmc_ssram_telemetry_get_pmc(pcidev, PMC_IDX_PCH, SSRAM_PCH_OFFSET);
+
+probe_finish:
+	/*
+	 * Memory barrier is used to ensure the correct write order between PMC info
+	 * and device_probed variable.
+	 */
+	smp_wmb();
+	device_probed = true;
 	return ret;
 }
+
+static const struct pci_device_id intel_pmc_ssram_telemetry_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PMC_DEVID_MTL_SOCM) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PMC_DEVID_ARL_SOCS) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PMC_DEVID_ARL_SOCM) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, intel_pmc_ssram_telemetry_pci_ids);
+
+static struct pci_driver intel_pmc_ssram_telemetry_driver = {
+	.name = "intel_pmc_ssram_telemetry",
+	.id_table = intel_pmc_ssram_telemetry_pci_ids,
+	.probe = intel_pmc_ssram_telemetry_probe,
+};
+module_pci_driver(intel_pmc_ssram_telemetry_driver);
+
 MODULE_IMPORT_NS("INTEL_VSEC");
+MODULE_AUTHOR("Xi Pardee <[email protected]>");
+MODULE_DESCRIPTION("Intel PMC SSRAM Telemetry driver");
+MODULE_LICENSE("GPL");
drivers/platform/x86/intel/pmc/ssram_telemetry.h new file +21 -0
new file mode 100644
index 000000000000..459a29efebea
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Intel PMC SSRAM Telemetry PCI Driver Header File
+ *
+ * Copyright (c) 2024, Intel Corporation.
+ */
+
+#ifndef PMC_SSRAM_H
+#define PMC_SSRAM_H
+
+/**
+ * struct pmc_ssram_telemetry - Structure to keep pmc info in ssram device
+ * @devid:		device id of the pmc device
+ * @base_addr:		contains PWRM base address
+ */
+struct pmc_ssram_telemetry {
+	u16 devid;
+	u64 base_addr;
+};
+
+/**

drivers/platform/x86/intel/pmc/ssram_telemetry.h line 31

Quoted 10 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.h new file +10 -0
+ * pmc_ssram_telemetry_get_pmc_info() - Get a PMC devid and base_addr information
+ * @pmc_idx:               Index of the PMC
+ * @pmc_ssram_telemetry:   pmc_ssram_telemetry structure to store the PMC information
+ *
+ * Return:
+ * * 0           - Success
+ * * -EAGAIN     - Probe function has not finished yet. Try again.
+ * * -EINVAL     - Invalid pmc_idx
+ * * -ENODEV     - PMC device is not available
+ */

Usually, the function kernel doc is where the code is. This is not an end of the world but it's even less likely the kerneldoc and code will keep in synch if they reside in different files so preferrably put it into the .c file.

Quoted 5 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.h new file +4 -0
+int pmc_ssram_telemetry_get_pmc_info(unsigned int pmc_idx,
+				     struct pmc_ssram_telemetry *pmc_ssram_telemetry);
+
+#endif /* PMC_SSRAM_H */
Signature · 1 line
 i.
author maintainer
#10 lore raw

Re: [PATCH v3 7/8] platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver

Hi Ilpo,

Thanks for the comments. My response are inline.

Quoted 14 lines · Ilpo Järvinen
Quoted 12 lines · Xi Pardee

Convert ssram device related functionalities to a new driver named Intel PMC SSRAM Telemetry driver. Modify PMC Core driver to use API exported by the driver to discover and achieve devid and PWRMBASE address information for each available PMC. PMC Core driver needs to get PCI device when reading from telemetry regions.

The new SSRAM driver binds to the SSRAM device and provides the following functionalities:
1. Look for and register telemetry regions available in SSRAM device.
2. Provide devid and PWRMBASE address information for the corresponding PMCs.

Quoted 5 lines
Quoted 1 line
Signed-off-by Xi Pardee <[email protected]>

I took patches 1-6 into review-ilpo-next now, but please consider the making the changes I suggested to 1/8 as separate patches.

This one has a few things that should be addressed before inclusion.

The changes in 1/8 will be included in the next version.

Quoted 47 lines
Quoted 30 lines
---
  drivers/platform/x86/intel/pmc/Kconfig        |  14 ++
  drivers/platform/x86/intel/pmc/Makefile       |   8 +-
  drivers/platform/x86/intel/pmc/core.c         |  80 +++++++----
  drivers/platform/x86/intel/pmc/core.h         |   7 -
  .../platform/x86/intel/pmc/ssram_telemetry.c  | 132 ++++++++++++------
  .../platform/x86/intel/pmc/ssram_telemetry.h  |  35 +++++
  6 files changed, 198 insertions(+), 78 deletions(-)

create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h

drivers/platform/x86/intel/pmc/Kconfig +4 -0
index d2f651fbec2c..f5fb1bdc266e 100644
@@ -8,6 +8,7 @@ config INTEL_PMC_CORE
  	depends on PCI
  	depends on ACPI
  	depends on INTEL_PMT_TELEMETRY
+	select INTEL_PMC_SSRAM_TELEMETRY
  	help
  	  The Intel Platform Controller Hub for Intel Core SoCs provides access
  	  to Power Management Controller registers via various interfaces. This

@@ -24,3 +25,16 @@ config INTEL_PMC_CORE
  		- SLPS0 Debug registers (Cannonlake/Icelake PCH)
  		- Low Power Mode registers (Tigerlake and beyond)
  		- PMC quirks as needed to enable SLPS0/S0ix
+
+config INTEL_PMC_SSRAM_TELEMETRY
+	tristate

drivers/platform/x86/intel/pmc/Kconfig config INTEL_PMC_CORE · line 40

Quoted 10 lines
drivers/platform/x86/intel/pmc/Kconfig +10 -0
+	help
+	  The PMC SSRAM device contains counters structured in Intel Platform
+	  Monitoring Techology (PMT) telemetry regions. The driver  provides
+	  API to expose information of PMCs available in the platform. This
+	  driver also looks for and register telemetry regions so they would
+	  be available for read through sysfs and Intel PMT API.
+
+	  The driver needs INTEL_VSEC driver to register for telemetry regions.
+	  This requirement has been fulfilled by INTEL_PMC_CORE driver which
+	  selects INTEL_PMC_SSRAM_TELEMETRY driver.

???

INTEL_PMC_CORE does not select/depend on INTEL_VSEC. I know it "works" indirectly through INTEL_PMT_TELEMETRY but a) this Kconfig help text is wrong/misleading, b) the way the dependencies are arranged seems fragile to me.

Quoted 6 lines

What prevents user from enabling INTEL_PMC_SSRAM_TELEMETRY without INTEL_PMC_CORE? This will likely blow up a randconfig build at some point when stars happen to align right.

Should the help text just be removed? I mean, shouldn't this be selected by INTEL_PMC_CORE and never should be chosen on its own by the user?

INTEL_PMC_SSRAM_TELEMETRY driver uses intel_vsec_register() provided by the INTEL_VSEC driver to register telem endpoints. INTEL_VSEC driver does provide an inline version of the API to use when INTEL_VSEC config is not set.

#if IS_ENABLED(CONFIG_INTEL_VSEC)
void intel_vsec_register(struct pci_dev *pdev, struct intel_vsec_platform_info *info); #else
static inline void intel_vsec_register(struct pci_dev *pdev, struct intel_vsec_platform_info *info) {
}
#endif
#endif

This inline version is not returning error currently and it can be changed to return -ENODEV. INTEL_PMC_SSRAM_TELEMETRY can fail the probe function when intel_vsec_register() returns error. In this way, INTEL_PMC_SSRAM_TELEMETRY does not depends on INTEL_VSEC and randconfig build will pass.

The help text for INTEL_PMC_SSRAM_TELEMETRY will be removed in next version.

Quoted 70 lines
Quoted 70 lines
drivers/platform/x86/intel/pmc/Makefile +6 -2
index e842647d3ced..5f68c8503a56 100644
@@ -3,8 +3,12 @@
  # Intel x86 Platform-Specific Drivers
  #
 
-intel_pmc_core-y			:= core.o ssram_telemetry.o spt.o cnp.o \
-					   icl.o tgl.o adl.o mtl.o arl.o lnl.o ptl.o
+intel_pmc_core-y			:= core.o spt.o cnp.o icl.o \
+					   tgl.o adl.o mtl.o arl.o lnl.o ptl.o
  obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core.o
  intel_pmc_core_pltdrv-y			:= pltdrv.o
  obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core_pltdrv.o
+
+# Intel PMC SSRAM driver
+intel_pmc_ssram_telemetry-y		+= ssram_telemetry.o
+obj-$(CONFIG_INTEL_PMC_SSRAM_TELEMETRY)	+= intel_pmc_ssram_telemetry.o
drivers/platform/x86/intel/pmc/core.c +8 -5
index a53a7677122c..26c1c75b709a 100644
@@ -29,6 +29,7 @@
  #include <asm/tsc.h>
 
  #include "core.h"
+#include "ssram_telemetry.h"
  #include "../pmt/telemetry.h"
 
  /* Maximum number of modes supported by platfoms that has low power mode capability */

@@ -1354,7 +1355,7 @@ static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *m
  	return 0;
  }
 
-static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
+static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct pci_dev *pcidev)
  {
  	struct telem_endpoint *ep;
  	const u8 *lpm_indices;

@@ -1371,7 +1372,7 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
  	if (!guid)
  		return -ENXIO;
 
-	ep = pmt_telem_find_and_register_endpoint(pmcdev->ssram_pcidev, guid, 0);
+	ep = pmt_telem_find_and_register_endpoint(pcidev, guid, 0);
  	if (IS_ERR(ep)) {
  		dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
  			PTR_ERR(ep));

@@ -1455,27 +1456,30 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
  	return ret;
  }
 
-static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
+static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev, int func)
  {
+	struct pci_dev *pcidev;
  	unsigned int i;
-	int ret;
+	int ret = 0;
 
-	if (!pmcdev->ssram_pcidev)
+	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));
+	if (!pcidev)
  		return -ENODEV;
 
  	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
  		if (!pmcdev->pmcs[i])
  			continue;
Quoted 8 lines

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

Quoted 5 lines
drivers/platform/x86/intel/pmc/core.c +2 -2
-		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
+		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i], pcidev);
  		if (ret)
-			return ret;
+			break;

DEFINE_FREE for pci_dev_put exists so you can do __free(pci_dev_put), so you can use cleanup.h to handle the cleanup and keep returning directly from here.

Will change to use _free from cleanup.h in next version.

Quoted 224 lines
Quoted 224 lines
drivers/platform/x86/intel/pmc/core.c +45 -18
  	}
 
-	return 0;
+	pci_dev_put(pcidev);
+	return ret;
  }
 
-const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
+static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
  {
  	for (; list->map; ++list)
  		if (devid == list->devid)

@@ -1484,23 +1488,32 @@ const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
  	return NULL;
  }
 
-int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		     const struct pmc_reg_map *reg_map, unsigned int pmc_index)
+static int pmc_core_pmc_add(struct pmc_dev *pmcdev, unsigned int pmc_index)
+
  {
-	struct pmc *pmc = pmcdev->pmcs[pmc_index];
+	struct pmc_ssram_telemetry pmc_ssram_telemetry;
+	const struct pmc_reg_map *map;
+	struct pmc *pmc;
+	int ret;
+
+	ret = pmc_ssram_telemetry_get_pmc_info(pmc_index, &pmc_ssram_telemetry);
+	if (ret)
+		return ret;
 
-	if (!pwrm_base)
+	map = pmc_core_find_regmap(pmcdev->regmap_list, pmc_ssram_telemetry.devid);
+	if (!map)
  		return -ENODEV;
 
-	/* Memory for primary PMC has been allocated in core.c */
+	pmc = pmcdev->pmcs[pmc_index];
+	/* Memory for primary PMC has been allocated */
  	if (!pmc) {
  		pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
  		if (!pmc)
  			return -ENOMEM;
  	}
 
-	pmc->map = reg_map;
-	pmc->base_addr = pwrm_base;
+	pmc->map = map;
+	pmc->base_addr = pmc_ssram_telemetry.base_addr;
  	pmc->regbase = ioremap(pmc->base_addr, pmc->map->regmap_length);
 
  	if (!pmc->regbase) {

@@ -1513,6 +1526,20 @@ int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
  	return 0;
  }
 
+static int pmc_core_ssram_get_reg_base(struct pmc_dev *pmcdev)
+{
+	int ret;
+
+	ret = pmc_core_pmc_add(pmcdev, PMC_IDX_MAIN);
+	if (ret)
+		return ret;
+
+	pmc_core_pmc_add(pmcdev, PMC_IDX_IOE);
+	pmc_core_pmc_add(pmcdev, PMC_IDX_PCH);
+
+	return 0;
+}
+
  /*
   * When supported, ssram init is used to achieve all available PMCs.
   * If ssram init fails, this function uses legacy method to at least get the

@@ -1530,10 +1557,18 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
  	ssram = pmc_dev_info->regmap_list != NULL;
  	if (ssram) {
  		pmcdev->regmap_list = pmc_dev_info->regmap_list;
-		ret = pmc_core_ssram_init(pmcdev, pmc_dev_info->pci_func);
+		ret = pmc_core_ssram_get_reg_base(pmcdev);
+		/*
+		 * EAGAIN error code indicates Intel PMC SSRAM Telemetry driver
+		 * has not finished probe and PMC info is not available yet. Try
+		 * again later.
+		 */
+		if (ret == -EAGAIN)
+			return -EPROBE_DEFER;
+
  		if (ret) {
  			dev_warn(&pmcdev->pdev->dev,
-				 "ssram init failed, %d, using legacy init\n", ret);
+				 "Failed to get PMC info from SSRAM, %d, using legacy init\n", ret);
  			ssram = false;
  		}
  	}

@@ -1550,7 +1585,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)
-		return pmc_core_ssram_get_lpm_reqs(pmcdev);
+		return pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func);
 
  	return 0;
  }

@@ -1639,15 +1674,10 @@ static void pmc_core_clean_structure(struct platform_device *pdev)
  	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
  		struct pmc *pmc = pmcdev->pmcs[i];
 
-		if (pmc)
+		if (pmc && pmc->regbase)
  			iounmap(pmc->regbase);
  	}
 
-	if (pmcdev->ssram_pcidev) {
-		pci_dev_put(pmcdev->ssram_pcidev);
-		pci_disable_device(pmcdev->ssram_pcidev);
-	}
-
  	if (pmcdev->punit_ep)
  		pmt_telem_unregister_endpoint(pmcdev->punit_ep);
drivers/platform/x86/intel/pmc/core.h +0 -7
index c3b07075d017..e136d18b1d38 100644
@@ -413,7 +413,6 @@ struct pmc {
   * struct pmc_dev - pmc device structure
   * @devs:		pointer to an array of pmc pointers
   * @pdev:		pointer to platform_device struct
- * @ssram_pcidev:	pointer to pci device struct for the PMC SSRAM
   * @crystal_freq:	crystal frequency from cpuid
   * @dbgfs_dir:		path to debugfs interface
   * @pmc_xram_read_bit:	flag to indicate whether PMC XRAM shadow registers

@@ -433,7 +432,6 @@ struct pmc_dev {
  	struct pmc *pmcs[MAX_NUM_PMC];
  	struct dentry *dbgfs_dir;
  	struct platform_device *pdev;
-	struct pci_dev *ssram_pcidev;
  	unsigned int crystal_freq;
  	int pmc_xram_read_bit;
  	struct mutex lock; /* generic mutex lock for PMC Core */

@@ -510,12 +508,7 @@ void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev);
  void pmc_core_punit_pmt_init(struct pmc_dev *pmcdev, u32 guid);
  void pmc_core_set_device_d3(unsigned int device);
 
-int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func);
-
  int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
-const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid);
-int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
-		     const struct pmc_reg_map *reg_map, unsigned int pmc_index);
 
  extern struct pmc_dev_info spt_pmc_dev;
  extern struct pmc_dev_info cnp_pmc_dev;
drivers/platform/x86/intel/pmc/ssram_telemetry.c +12 -13
index 7b8443092b20..232263008030 100644
@@ -1,19 +1,18 @@
  // SPDX-License-Identifier: GPL-2.0
  /*
- * This file contains functions to handle discovery of PMC metrics located
- * in the PMC SSRAM PCI device.
+ * Intel PMC SSRAM TELEMETRY PCI Driver
   *
   * Copyright (c) 2023, Intel Corporation.
- * All Rights Reserved.
- *
   */
 
  #include <linux/cleanup.h>
  #include <linux/intel_vsec.h>
  #include <linux/pci.h>
+#include <linux/types.h>
  #include <linux/io-64-nonatomic-lo-hi.h>
 
  #include "core.h"
+#include "ssram_telemetry.h"
 
  #define SSRAM_HDR_SIZE		0x100
  #define SSRAM_PWRM_OFFSET	0x14

@@ -23,12 +22,14 @@
  #define SSRAM_IOE_OFFSET	0x68
  #define SSRAM_DEVID_OFFSET	0x70
 
-DEFINE_FREE(pmc_core_iounmap, void __iomem *, if (_T) iounmap(_T))
+DEFINE_FREE(pmc_ssram_telemetry_iounmap, void __iomem *, if (_T) iounmap(_T))
+
+static struct pmc_ssram_telemetry *pmc_ssram_telems;
+static bool device_probed;
 
  static void
-pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
+pmc_ssram_telemetry_add_pmt(struct pci_dev *pcidev, u64 ssram_base, void __iomem *ssram)
  {
-	struct pci_dev *pcidev = pmcdev->ssram_pcidev;
  	struct intel_vsec_platform_info info = {};
  	struct intel_vsec_header *headers[2] = {};
  	struct intel_vsec_header header;

@@ -57,7 +58,7 @@ pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
  	info.caps = VSEC_CAP_TELEMETRY;
  	info.headers = headers;
  	info.base_addr = ssram_base;
-	info.parent = &pmcdev->pdev->dev;
+	info.parent = &pcidev->dev;
 
  	intel_vsec_register(pcidev, &info);
  }

@@ -68,19 +69,14 @@ static inline u64 get_base(void __iomem *addr, u32 offset)
  }
 
  static int
-pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, unsigned int pmc_idx, u32 offset)
+pmc_ssram_telemetry_get_pmc(struct pci_dev *pcidev, unsigned int pmc_idx, u32 offset)
  {
-	struct pci_dev *ssram_pcidev = pmcdev->ssram_pcidev;
-	void __iomem __free(pmc_core_iounmap) *tmp_ssram = NULL;
-	void __iomem __free(pmc_core_iounmap) *ssram = NULL;
-	const struct pmc_reg_map *map;
+	void __iomem __free(pmc_ssram_telemetry_iounmap) * tmp_ssram = NULL;
+	void __iomem __free(pmc_ssram_telemetry_iounmap) * ssram = NULL;
  	u64 ssram_base, pwrm_base;
  	u16 devid;
Quoted 6 lines

drivers/platform/x86/intel/pmc/ssram_telemetry.c static inline u64 get_base(void __iomem *addr, u32 offset) · line 79

Quoted 5 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.c +1 -4
-	if (!pmcdev->regmap_list)
-		return -ENOENT;
-
-	ssram_base = ssram_pcidev->resource[0].start;
+	ssram_base = pcidev->resource[0].start;

pci_resource_start()

Will change in next version.

Quoted 158 lines
Quoted 158 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.c +75 -27
  	tmp_ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
  	if (!tmp_ssram)
  		return -ENOMEM;

@@ -107,45 +103,93 @@ pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, unsigned int pmc_idx, u32 offset)
  	devid = readw(ssram + SSRAM_DEVID_OFFSET);
 
  	/* Find and register and PMC telemetry entries */
-	pmc_add_pmt(pmcdev, ssram_base, ssram);
+	pmc_ssram_telemetry_add_pmt(pcidev, ssram_base, ssram);
 
-	map = pmc_core_find_regmap(pmcdev->regmap_list, devid);
-	if (!map)
+	pmc_ssram_telems[pmc_idx].devid = devid;
+	pmc_ssram_telems[pmc_idx].base_addr = pwrm_base;
+
+	return 0;
+}
+
+int pmc_ssram_telemetry_get_pmc_info(unsigned int pmc_idx,
+				     struct pmc_ssram_telemetry *pmc_ssram_telemetry)
+{
+	/*
+	 * PMCs are discovered in probe function. If this function is called before
+	 * probe function complete, the result would be invalid. Use device_probed
+	 * variable to avoid this case. Return -EAGAIN to inform the consumer to call
+	 * again later.
+	 */
+	if (!device_probed)
+		return -EAGAIN;
+
+	/*
+	 * Memory barrier is used to ensure the correct read order between
+	 * device_probed variable and PMC info.
+	 */
+	smp_rmb();
+	if (pmc_idx >= MAX_NUM_PMC)
+		return -EINVAL;
+
+	if (!pmc_ssram_telems || !pmc_ssram_telems[pmc_idx].devid)
  		return -ENODEV;
 
-	return pmc_core_pmc_add(pmcdev, pwrm_base, map, pmc_idx);
+	pmc_ssram_telemetry->devid = pmc_ssram_telems[pmc_idx].devid;
+	pmc_ssram_telemetry->base_addr = pmc_ssram_telems[pmc_idx].base_addr;
+	return 0;
  }
+EXPORT_SYMBOL_GPL(pmc_ssram_telemetry_get_pmc_info);
 
-int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func)
+static int intel_pmc_ssram_telemetry_probe(struct pci_dev *pcidev, const struct pci_device_id *id)
  {
-	struct pci_dev *pcidev;
  	int ret;
 
-	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));
-	if (!pcidev)
-		return -ENODEV;
+	pmc_ssram_telems = devm_kzalloc(&pcidev->dev, sizeof(*pmc_ssram_telems) * MAX_NUM_PMC,
+					GFP_KERNEL);
+	if (!pmc_ssram_telems) {
+		ret = -ENOMEM;
+		goto probe_finish;
+	}
 
  	ret = pcim_enable_device(pcidev);
-	if (ret)
-		goto release_dev;
-
-	pmcdev->ssram_pcidev = pcidev;
+	if (ret) {
+		dev_dbg(&pcidev->dev, "failed to enable PMC SSRAM device\n");
+		goto probe_finish;
+	}
 
-	ret = pmc_core_ssram_get_pmc(pmcdev, PMC_IDX_MAIN, 0);
+	ret = pmc_ssram_telemetry_get_pmc(pcidev, PMC_IDX_MAIN, 0);
  	if (ret)
-		goto disable_dev;
-
-	pmc_core_ssram_get_pmc(pmcdev, PMC_IDX_IOE, SSRAM_IOE_OFFSET);
-	pmc_core_ssram_get_pmc(pmcdev, PMC_IDX_PCH, SSRAM_PCH_OFFSET);
-
-	return 0;
-
-disable_dev:
-	pmcdev->ssram_pcidev = NULL;
-	pci_disable_device(pcidev);
-release_dev:
-	pci_dev_put(pcidev);
-
+		goto probe_finish;
+
+	pmc_ssram_telemetry_get_pmc(pcidev, PMC_IDX_IOE, SSRAM_IOE_OFFSET);
+	pmc_ssram_telemetry_get_pmc(pcidev, PMC_IDX_PCH, SSRAM_PCH_OFFSET);
+
+probe_finish:
+	/*
+	 * Memory barrier is used to ensure the correct write order between PMC info
+	 * and device_probed variable.
+	 */
+	smp_wmb();
+	device_probed = true;
  	return ret;
  }
+
+static const struct pci_device_id intel_pmc_ssram_telemetry_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PMC_DEVID_MTL_SOCM) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PMC_DEVID_ARL_SOCS) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PMC_DEVID_ARL_SOCM) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, intel_pmc_ssram_telemetry_pci_ids);
+
+static struct pci_driver intel_pmc_ssram_telemetry_driver = {
+	.name = "intel_pmc_ssram_telemetry",
+	.id_table = intel_pmc_ssram_telemetry_pci_ids,
+	.probe = intel_pmc_ssram_telemetry_probe,
+};
+module_pci_driver(intel_pmc_ssram_telemetry_driver);
+
  MODULE_IMPORT_NS("INTEL_VSEC");
+MODULE_AUTHOR("Xi Pardee <[email protected]>");
+MODULE_DESCRIPTION("Intel PMC SSRAM Telemetry driver");
+MODULE_LICENSE("GPL");
drivers/platform/x86/intel/pmc/ssram_telemetry.h new file +27 -0
new file mode 100644
index 000000000000..459a29efebea
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Intel PMC SSRAM Telemetry PCI Driver Header File
+ *
+ * Copyright (c) 2024, Intel Corporation.
+ */
+
+#ifndef PMC_SSRAM_H
+#define PMC_SSRAM_H
+
+/**
+ * struct pmc_ssram_telemetry - Structure to keep pmc info in ssram device
+ * @devid:		device id of the pmc device
+ * @base_addr:		contains PWRM base address
+ */
+struct pmc_ssram_telemetry {
+	u16 devid;
+	u64 base_addr;
+};
+
+/**
+ * pmc_ssram_telemetry_get_pmc_info() - Get a PMC devid and base_addr information
+ * @pmc_idx:               Index of the PMC
+ * @pmc_ssram_telemetry:   pmc_ssram_telemetry structure to store the PMC information
+ *
+ * Return:
+ * * 0           - Success
Quoted 8 lines

drivers/platform/x86/intel/pmc/ssram_telemetry.h line 31

Quoted 4 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.h new file +4 -0
+ * * -EAGAIN     - Probe function has not finished yet. Try again.
+ * * -EINVAL     - Invalid pmc_idx
+ * * -ENODEV     - PMC device is not available
+ */

Usually, the function kernel doc is where the code is. This is not an end of the world but it's even less likely the kerneldoc and code will keep in synch if they reside in different files so preferrably put it into the .c file.

Will move it in ssram_telemetry.c in next version.

Thanks!

Xi

Quoted 6 lines
Quoted 6 lines
drivers/platform/x86/intel/pmc/ssram_telemetry.h new file +4 -0
+int pmc_ssram_telemetry_get_pmc_info(unsigned int pmc_idx,
+				     struct pmc_ssram_telemetry *pmc_ssram_telemetry);
+
+#endif /* PMC_SSRAM_H */
author maintainer
#11 lore raw

PATCH v3 8/8 platform/x86:intel/pmc: Move error handling to init function

In reply to Xi Pardee

Move error handling code to generic_core_init() function. The previous implementation is that init function called for "full cleanup" function when error occurs which is error prone. The init function should handle the error path itself to improve code maintainability.

Signed-off-by Xi Pardee <[email protected]>
---
 drivers/platform/x86/intel/pmc/core.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)
drivers/platform/x86/intel/pmc/core.c +20 -3
index 26c1c75b709a..c39e5f5f1475 100644
@@ -1584,10 +1584,26 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 	if (pmc_dev_info->dmu_guid)
 		pmc_core_punit_pmt_init(pmcdev, pmc_dev_info->dmu_guid);
 
-	if (ssram)
-		return pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func);
+	if (ssram) {
+		ret = pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func);
+		if (ret)
+			goto unmap_regbase;
+	}
 
 	return 0;
+
+unmap_regbase:
+	for (unsigned int i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
+		struct pmc *pmc = pmcdev->pmcs[i];
+
+		if (pmc && pmc->regbase)
+			iounmap(pmc->regbase);
+	}
+
+	if (pmcdev->punit_ep)
+		pmt_telem_unregister_endpoint(pmcdev->punit_ep);
+
+	return ret;
 }
 
 static const struct x86_cpu_id intel_pmc_core_ids[] = {

@@ -1735,7 +1751,8 @@ static int pmc_core_probe(struct platform_device *pdev)
 		ret = generic_core_init(pmcdev, pmc_dev_info);
 
 	if (ret) {
-		pmc_core_clean_structure(pdev);
+		platform_set_drvdata(pdev, NULL);
+		mutex_destroy(&pmcdev->lock);
 		return ret;
 	}
Signature · 1 line
2.43.0