Add Arrow Lake U/H support

PATCH Superseded Xi Pardee 2 participants 6 messages Dec 7, 2024 – Dec 9, 2024

Subsystems (2)

x86 platform drivers Intel PMC core driver

lore

Share & tools

lore

Xi Pardee

author maintainer

2024-12-07 05:36 UTC #0 lore raw

cover Add Arrow Lake U/H support

This patch series contains three patches to add Arrow Lake U/H support in intel_pmc_core driver. The first two patches are preparation patches.

v2->v1:
- Create an info structure for platform variation information
- Make generic init function to static in tgl.c
- Fix typo

Xi Pardee (3):
platform/x86:intel/pmc: Make tgl_core_generic_init() static platform/x86:intel/pmc: Create info structure for pmc device platform/x86/intel/pmc: Add Arrow Lake U/H support to intel_pmc_core
driver

 drivers/platform/x86/intel/pmc/arl.c  | 64 +++++++++++++++++++++++----
 drivers/platform/x86/intel/pmc/core.c |  2 +
 drivers/platform/x86/intel/pmc/core.h | 17 ++++++-
 drivers/platform/x86/intel/pmc/tgl.c  | 45 ++++++++++---------
 4 files changed, 98 insertions(+), 30 deletions(-)

Signature · 1 line

2.43.0

Xi Pardee

author maintainer

2024-12-07 05:36 UTC #1 lore raw

PATCH 1/3 platform/x86:intel/pmc: Make tgl_core_generic_init() static

Make tgl_core_generic_init() a static function as the function has no callers outside of tgl.c. Remove the prototype in core.h and reorder the code in tgl.c.

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

---
 drivers/platform/x86/intel/pmc/core.h |  1 -
 drivers/platform/x86/intel/pmc/tgl.c  | 22 +++++++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

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

index b9d3291d0bf2c..a1886d8e1ef3e 100644
@@ -597,7 +597,6 @@ int cnp_core_init(struct pmc_dev *pmcdev);
 int icl_core_init(struct pmc_dev *pmcdev);
 int tgl_core_init(struct pmc_dev *pmcdev);
 int tgl_l_core_init(struct pmc_dev *pmcdev);
-int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp);
 int adl_core_init(struct pmc_dev *pmcdev);
 int mtl_core_init(struct pmc_dev *pmcdev);
 int arl_core_init(struct pmc_dev *pmcdev);

drivers/platform/x86/intel/pmc/tgl.c +11 ~~-11~~

index e0580de180773..4fec43d212d01 100644
@@ -285,17 +285,7 @@ void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
 	ACPI_FREE(out_obj);
 }
 
-int tgl_l_core_init(struct pmc_dev *pmcdev)
-{
-	return tgl_core_generic_init(pmcdev, PCH_LP);
-}
-
-int tgl_core_init(struct pmc_dev *pmcdev)
-{
-	return tgl_core_generic_init(pmcdev, PCH_H);
-}
-
-int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
+static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
 {
 	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
 	int ret;

@@ -317,3 +307,13 @@ int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
 
 	return 0;
 }
+
+int tgl_l_core_init(struct pmc_dev *pmcdev)
+{
+	return tgl_core_generic_init(pmcdev, PCH_LP);
+}
+
+int tgl_core_init(struct pmc_dev *pmcdev)
+{
+	return tgl_core_generic_init(pmcdev, PCH_H);
+}

Signature · 1 line

2.43.0

Ilpo Järvinen

maintainer Reviewed-by on v1 1/3

2024-12-09 13:37 UTC #2 lore raw

Re: [PATCH 1/3] platform/x86:intel/pmc: Make tgl_core_generic_init() static

Quoted 50 lines · Xi Pardee

Make tgl_core_generic_init() a static function as the function has no callers outside of tgl.c. Remove the prototype in core.h and reorder the code in tgl.c.

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

---
 drivers/platform/x86/intel/pmc/core.h |  1 -
 drivers/platform/x86/intel/pmc/tgl.c  | 22 +++++++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

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

index b9d3291d0bf2c..a1886d8e1ef3e 100644
@@ -597,7 +597,6 @@ int cnp_core_init(struct pmc_dev *pmcdev);
 int icl_core_init(struct pmc_dev *pmcdev);
 int tgl_core_init(struct pmc_dev *pmcdev);
 int tgl_l_core_init(struct pmc_dev *pmcdev);
-int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp);
 int adl_core_init(struct pmc_dev *pmcdev);
 int mtl_core_init(struct pmc_dev *pmcdev);
 int arl_core_init(struct pmc_dev *pmcdev);

drivers/platform/x86/intel/pmc/tgl.c +1 ~~-11~~

index e0580de180773..4fec43d212d01 100644
@@ -285,17 +285,7 @@ void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
 	ACPI_FREE(out_obj);
 }
 
-int tgl_l_core_init(struct pmc_dev *pmcdev)
-{
-	return tgl_core_generic_init(pmcdev, PCH_LP);
-}
-
-int tgl_core_init(struct pmc_dev *pmcdev)
-{
-	return tgl_core_generic_init(pmcdev, PCH_H);
-}
-
-int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
+static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
 {
 	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
 	int ret;

@@ -317,3 +307,13 @@ int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
 
 	return 0;
 }

drivers/platform/x86/intel/pmc/tgl.c int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp) · line 319

Quoted 10 lines

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

+
+int tgl_l_core_init(struct pmc_dev *pmcdev)
+{
+	return tgl_core_generic_init(pmcdev, PCH_LP);
+}
+
+int tgl_core_init(struct pmc_dev *pmcdev)
+{
+	return tgl_core_generic_init(pmcdev, PCH_H);
+}

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

Signature · 1 line

i.

Xi Pardee

author maintainer

2024-12-07 05:36 UTC #3 lore raw

PATCH 2/3 platform/x86:intel/pmc: Create info structure for pmc device

In reply to Xi Pardee

Create an info structure to store platform specific information. For Tiger Lake and Arrow Lake platforms, multiple platform variations could share one generic init function. Using info structure could avoid if () forest. Modify tgl.c to use the info structure.

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

---
 drivers/platform/x86/intel/pmc/core.h | 15 ++++++++++++
 drivers/platform/x86/intel/pmc/tgl.c  | 33 +++++++++++++++------------
 2 files changed, 33 insertions(+), 15 deletions(-)

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

index a1886d8e1ef3e..3124315d2b925 100644
@@ -428,6 +428,21 @@ struct pmc_dev {
 	struct pmc_info *regmap_list;
 };
 
+/**
+ * struct pmc_dev_info - Structure to keep pmc device info
+ * @func:		function number of the primary pmc
+ * @map:		pointer to a pmc_reg_map struct that contains platform
+ *			specific attributes of the primary pmc
+ * @suspend:		Function to perform platform specific suspend
+ * @resume:		Function to perform platform specific resume
+ */
+struct pmc_dev_info {
+	u8 func;
+	const struct pmc_reg_map *map;
+	void (*suspend)(struct pmc_dev *pmcdev);
+	int (*resume)(struct pmc_dev *pmcdev);
+};
+
 enum pmc_index {
 	PMC_IDX_MAIN,
 	PMC_IDX_SOC = PMC_IDX_MAIN,

drivers/platform/x86/intel/pmc/tgl.c +18 ~~-15~~

index 4fec43d212d01..c6fc3a0225a55 100644
@@ -13,11 +13,6 @@
 #define ACPI_S0IX_DSM_UUID		"57a6512e-3979-4e9d-9708-ff13b2508972"
 #define ACPI_GET_LOW_MODE_REGISTERS	1
 
-enum pch_type {
-	PCH_H,
-	PCH_LP
-};
-
 const struct pmc_bit_map tgl_pfear_map[] = {
 	{"PSF9",		BIT(0)},
 	{"RES_66",		BIT(1)},

@@ -285,18 +280,26 @@ void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
 	ACPI_FREE(out_obj);
 }
 
-static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
+static struct pmc_dev_info tgl_l_pmc_dev = {
+	.map = &tgl_reg_map,
+	.suspend = cnl_suspend,
+	.resume = cnl_resume,
+};
+
+static struct pmc_dev_info tgl_pmc_dev = {
+	.map = &tgl_h_reg_map,
+	.suspend = cnl_suspend,
+	.resume = cnl_resume,
+};
+
+static int tgl_core_generic_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
 	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
 	int ret;
 
-	if (pch_tp == PCH_H)
-		pmc->map = &tgl_h_reg_map;
-	else
-		pmc->map = &tgl_reg_map;
-
-	pmcdev->suspend = cnl_suspend;
-	pmcdev->resume = cnl_resume;
+	pmc->map = pmc_dev_info->map;
+	pmcdev->suspend = pmc_dev_info->suspend;
+	pmcdev->resume = pmc_dev_info->resume;
 
 	ret = get_primary_reg_base(pmc);
 	if (ret)

@@ -310,10 +313,10 @@ static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
 
 int tgl_l_core_init(struct pmc_dev *pmcdev)
 {
-	return tgl_core_generic_init(pmcdev, PCH_LP);
+	return tgl_core_generic_init(pmcdev, &tgl_l_pmc_dev);
 }
 
 int tgl_core_init(struct pmc_dev *pmcdev)
 {
-	return tgl_core_generic_init(pmcdev, PCH_H);
+	return tgl_core_generic_init(pmcdev, &tgl_pmc_dev);
 }

Signature · 1 line

2.43.0

Ilpo Järvinen

maintainer

2024-12-09 15:18 UTC #4 lore raw

Re: [PATCH 2/3] platform/x86:intel/pmc: Create info structure for pmc device

Quoted 92 lines · Xi Pardee

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

---
 drivers/platform/x86/intel/pmc/core.h | 15 ++++++++++++
 drivers/platform/x86/intel/pmc/tgl.c  | 33 +++++++++++++++------------
 2 files changed, 33 insertions(+), 15 deletions(-)

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

index a1886d8e1ef3e..3124315d2b925 100644
@@ -428,6 +428,21 @@ struct pmc_dev {
 	struct pmc_info *regmap_list;
 };
 
+/**
+ * struct pmc_dev_info - Structure to keep pmc device info
+ * @func:		function number of the primary pmc
+ * @map:		pointer to a pmc_reg_map struct that contains platform
+ *			specific attributes of the primary pmc
+ * @suspend:		Function to perform platform specific suspend
+ * @resume:		Function to perform platform specific resume
+ */
+struct pmc_dev_info {
+	u8 func;
+	const struct pmc_reg_map *map;
+	void (*suspend)(struct pmc_dev *pmcdev);
+	int (*resume)(struct pmc_dev *pmcdev);
+};
+
 enum pmc_index {
 	PMC_IDX_MAIN,
 	PMC_IDX_SOC = PMC_IDX_MAIN,

drivers/platform/x86/intel/pmc/tgl.c +16 ~~-13~~

index 4fec43d212d01..c6fc3a0225a55 100644
@@ -13,11 +13,6 @@
 #define ACPI_S0IX_DSM_UUID		"57a6512e-3979-4e9d-9708-ff13b2508972"
 #define ACPI_GET_LOW_MODE_REGISTERS	1
 
-enum pch_type {
-	PCH_H,
-	PCH_LP
-};
-
 const struct pmc_bit_map tgl_pfear_map[] = {
 	{"PSF9",		BIT(0)},
 	{"RES_66",		BIT(1)},

@@ -285,18 +280,26 @@ void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
 	ACPI_FREE(out_obj);
 }
 
-static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
+static struct pmc_dev_info tgl_l_pmc_dev = {
+	.map = &tgl_reg_map,
+	.suspend = cnl_suspend,
+	.resume = cnl_resume,
+};
+
+static struct pmc_dev_info tgl_pmc_dev = {
+	.map = &tgl_h_reg_map,
+	.suspend = cnl_suspend,
+	.resume = cnl_resume,
+};
+
+static int tgl_core_generic_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
 	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
 	int ret;
 
-	if (pch_tp == PCH_H)
-		pmc->map = &tgl_h_reg_map;
-	else
-		pmc->map = &tgl_reg_map;
-
-	pmcdev->suspend = cnl_suspend;
-	pmcdev->resume = cnl_resume;
+	pmc->map = pmc_dev_info->map;
+	pmcdev->suspend = pmc_dev_info->suspend;
+	pmcdev->resume = pmc_dev_info->resume;
 
 	ret = get_primary_reg_base(pmc);
 	if (ret)

@@ -310,10 +313,10 @@ static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
 
 int tgl_l_core_init(struct pmc_dev *pmcdev)

drivers/platform/x86/intel/pmc/tgl.c static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp) · line 322

Quoted 10 lines

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

 {
-	return tgl_core_generic_init(pmcdev, PCH_LP);
+	return tgl_core_generic_init(pmcdev, &tgl_l_pmc_dev);
 }
 
 int tgl_core_init(struct pmc_dev *pmcdev)
 {
-	return tgl_core_generic_init(pmcdev, PCH_H);
+	return tgl_core_generic_init(pmcdev, &tgl_pmc_dev);
 }

While adding the struct seems right direction, I don't feel good about the scoping in this patch. It seems that we'll still end up duplicating lots of init code.

If I do (in drivers/platform/x86/intel/pmc):

git grep -A7 -B7 -e 'suspend =' -e 'resume =' -e '->map ='

...I see basically:

- assign those variables (minor variations: PMC_IDX_MAIN/PMC_IDX_SOC index, NULL resume/suspend that is already handled)
- if regmap_list is set, call to pmc_core_ssram_init() (the pointer put into ->regmap_list seems another candidate to be included into the info struct.)
- else (or if ssram init failed), call get_primary_reg_base()
- call pmc_core_get_low_power_modes()

Why cannot we like have a common init function which handles all those and remove all that duplication from per arch files? I don't see anything that looked meaningful variations so the current archs should be all generalizable I think. Why leave all those into per arch init as done in this series?

Signature · 1 line

i.

Xi Pardee

author maintainer

2024-12-07 05:36 UTC #5 lore raw

PATCH 3/3 platform/x86/intel/pmc: Add Arrow Lake U/H support to intel_pmc_core driver

In reply to Xi Pardee

Add Arrow Lake U and Arrow Lake H support in intel_pmc_core driver.

Signed-off-by Rajvi Jingar <[email protected]>

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

---
 drivers/platform/x86/intel/pmc/arl.c  | 64 +++++++++++++++++++++++----
 drivers/platform/x86/intel/pmc/core.c |  2 +
 drivers/platform/x86/intel/pmc/core.h |  1 +
 3 files changed, 59 insertions(+), 8 deletions(-)

drivers/platform/x86/intel/pmc/arl.c +56 -8

index 05dec4f5019f3..d16e1dea63e7d 100644
@@ -16,6 +16,7 @@
 #define IOEP_LPM_REQ_GUID	0x5077612
 #define SOCS_LPM_REQ_GUID	0x8478657
 #define PCHS_LPM_REQ_GUID	0x9684572
+#define SOCM_LPM_REQ_GUID	0x2625030
 
 static const u8 ARL_LPM_REG_INDEX[] = {0, 4, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20};

@@ -650,6 +651,7 @@ 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

@@ -669,11 +671,17 @@ static struct pmc_info arl_pmc_info_list[] = {
 		.devid	= PMC_DEVID_PCHS,
 		.map	= &arl_pchs_reg_map,
 	},
+	{
+		.guid	= SOCM_LPM_REQ_GUID,
+		.devid	= PMC_DEVID_SOCM,
+		.map	= &mtl_socm_reg_map,
+	},
 	{}
 };
 
 #define ARL_NPU_PCI_DEV			0xad1d
 #define ARL_GNA_PCI_DEV			0xae4c
+#define ARL_H_GNA_PCI_DEV		0x774c
 /*
  * Set power state of select devices that do not have drivers to D3
  * so that they do not block Package C entry.

@@ -684,6 +692,12 @@ static void arl_d3_fixup(void)
 	pmc_core_set_device_d3(ARL_GNA_PCI_DEV);
 }
 
+static void arl_h_d3_fixup(void)
+{
+	pmc_core_set_device_d3(ARL_NPU_PCI_DEV);
+	pmc_core_set_device_d3(ARL_H_GNA_PCI_DEV);
+}
+
 static int arl_resume(struct pmc_dev *pmcdev)
 {
 	arl_d3_fixup();

@@ -691,26 +705,46 @@ static int arl_resume(struct pmc_dev *pmcdev)
 	return cnl_resume(pmcdev);
 }
 
-int arl_core_init(struct pmc_dev *pmcdev)
+static int arl_h_resume(struct pmc_dev *pmcdev)
+{
+	arl_h_d3_fixup();
+
+	return cnl_resume(pmcdev);
+}
+
+static struct pmc_dev_info arl_pmc_dev = {
+	.func = 0,
+	.map = &arl_socs_reg_map,
+	.suspend = cnl_suspend,
+	.resume = arl_resume,
+};
+
+static struct pmc_dev_info arl_h_pmc_dev = {
+	.func = 2,
+	.map = &mtl_socm_reg_map,
+	.suspend = cnl_suspend,
+	.resume = arl_h_resume,
+};
+
+static int arl_core_generic_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
-	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC];
 	int ret;
-	int func = 0;
 	bool ssram_init = true;
+	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC];
+
+	pmcdev->suspend = pmc_dev_info->suspend;
+	pmcdev->resume = pmc_dev_info->resume;
 
-	arl_d3_fixup();
-	pmcdev->suspend = cnl_suspend;
-	pmcdev->resume = arl_resume;
 	pmcdev->regmap_list = arl_pmc_info_list;
 
 	/*
 	 * If ssram init fails use legacy method to at least get the
 	 * primary PMC
 	 */
-	ret = pmc_core_ssram_init(pmcdev, func);
+	ret = pmc_core_ssram_init(pmcdev, pmc_dev_info->func);
 	if (ret) {
 		ssram_init = false;
-		pmc->map = &arl_socs_reg_map;
+		pmc->map = pmc_dev_info->map;
 
 		ret = get_primary_reg_base(pmc);
 		if (ret)

@@ -728,3 +762,17 @@ int arl_core_init(struct pmc_dev *pmcdev)
 
 	return 0;
 }
+
+int arl_h_core_init(struct pmc_dev *pmcdev)
+{
+	arl_h_d3_fixup();
+
+	return arl_core_generic_init(pmcdev, &arl_h_pmc_dev);
+}
+
+int arl_core_init(struct pmc_dev *pmcdev)
+{
+	arl_d3_fixup();
+
+	return arl_core_generic_init(pmcdev, &arl_pmc_dev);
+}

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

index 3e7f99ac8c941..fbc43e090e7cc 100644
@@ -1367,6 +1367,8 @@ static const struct x86_cpu_id intel_pmc_core_ids[] = {
 	X86_MATCH_VFM(INTEL_RAPTORLAKE_S,	adl_core_init),
 	X86_MATCH_VFM(INTEL_METEORLAKE_L,	mtl_core_init),
 	X86_MATCH_VFM(INTEL_ARROWLAKE,		arl_core_init),
+	X86_MATCH_VFM(INTEL_ARROWLAKE_H,	arl_h_core_init),
+	X86_MATCH_VFM(INTEL_ARROWLAKE_U,	arl_h_core_init),
 	X86_MATCH_VFM(INTEL_LUNARLAKE_M,	lnl_core_init),
 	{}
 };

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

index 3124315d2b925..76ef2f56d897b 100644
@@ -615,6 +615,7 @@ int tgl_l_core_init(struct pmc_dev *pmcdev);
 int adl_core_init(struct pmc_dev *pmcdev);
 int mtl_core_init(struct pmc_dev *pmcdev);
 int arl_core_init(struct pmc_dev *pmcdev);
+int arl_h_core_init(struct pmc_dev *pmcdev);
 int lnl_core_init(struct pmc_dev *pmcdev);
 
 void cnl_suspend(struct pmc_dev *pmcdev);

Signature · 1 line

2.43.0