Add Arrow Lake U/H support
Subsystems (2)
Xi Pardee
maintainerv3 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. The second patch creates a generic init function for all platforms to reduce duplicate code.
v3->v2:
- Create a generic init function for all platforms
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 generic_core_init() for all platforms platform/x86/intel/pmc: Add Arrow Lake U/H support to intel_pmc_core
driver
drivers/platform/x86/intel/pmc/adl.c | 21 +++---- drivers/platform/x86/intel/pmc/arl.c | 81 ++++++++++++++++----------- drivers/platform/x86/intel/pmc/cnp.c | 21 +++---- drivers/platform/x86/intel/pmc/core.c | 47 ++++++++++++++++ drivers/platform/x86/intel/pmc/core.h | 27 ++++++++- drivers/platform/x86/intel/pmc/icl.c | 18 ++---- drivers/platform/x86/intel/pmc/lnl.c | 24 +++----- drivers/platform/x86/intel/pmc/mtl.c | 45 ++++----------- drivers/platform/x86/intel/pmc/spt.c | 18 ++---- drivers/platform/x86/intel/pmc/tgl.c | 43 +++++++------- 10 files changed, 189 insertions(+), 156 deletions(-)
Signature · 1 line
2.43.0
Xi Pardee
maintainerPATCH v3 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]>
Reviewed-by Ilpo Järvinen <[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
@@ -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
@@ -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
Xi Pardee
maintainerPATCH v3 2/3 platform/x86:intel/pmc: Create generic_core_init() for all platforms
In reply to Xi PardeeCreate a generic_core_init() function for all architecture to reduce duplicate code in each architecture file. Create an info structure to catch the variations between each architecture and pass it to the generic init function.
Convert all architectures to call the generic core init function.
Signed-off-by Xi Pardee <[email protected]>
--- drivers/platform/x86/intel/pmc/adl.c | 21 ++++-------- drivers/platform/x86/intel/pmc/arl.c | 47 ++++++++------------------- drivers/platform/x86/intel/pmc/cnp.c | 21 ++++-------- drivers/platform/x86/intel/pmc/core.c | 45 +++++++++++++++++++++++++ drivers/platform/x86/intel/pmc/core.h | 25 ++++++++++++++ drivers/platform/x86/intel/pmc/icl.c | 18 ++++------ drivers/platform/x86/intel/pmc/lnl.c | 24 +++++--------- drivers/platform/x86/intel/pmc/mtl.c | 45 +++++++------------------ drivers/platform/x86/intel/pmc/spt.c | 18 ++++------ drivers/platform/x86/intel/pmc/tgl.c | 31 +++++++++--------- 10 files changed, 145 insertions(+), 150 deletions(-)
drivers/platform/x86/intel/pmc/adl.c +7 -14
@@ -311,20 +311,13 @@ const struct pmc_reg_map adl_reg_map = { .pson_residency_counter_step = TGL_PSON_RES_COUNTER_STEP, }; +static struct pmc_dev_info adl_pmc_dev = { + .map = &adl_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; +int adl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; - - pmc->map = &adl_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &adl_pmc_dev); }
drivers/platform/x86/intel/pmc/arl.c +13 -34
53 lines not on this page · raw message
drivers/platform/x86/intel/pmc/cnp.c +7 -14
@@ -274,20 +274,13 @@ int cnl_resume(struct pmc_dev *pmcdev)return pmc_core_resume_common (pmcdev); } +static struct pmc_dev_info cnp_pmc_dev = { + .map = &cnp_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; +int cnp_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; - - pmc->map = &cnp_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &cnp_pmc_dev); }
drivers/platform/x86/intel/pmc/core.c +45 -0
51 lines not on this page · raw message
drivers/platform/x86/intel/pmc/core.h +25 -0
@@ -436,6 +436,30 @@ enum pmc_index { PMC_IDX_MAX }; +/** +* struct pmc_dev_info - Structure to keep pmc device info +* @func: Function number of the primary pmc +* @ssram: Bool shows if platform has ssram support +* @dmu_guid: DMU GUID +* @regmap_list: Pointer to a list of pmc_info structure that could be +* available for the platform +* @map: Pointer to a pmc_reg_map struct that contains platform +* specific attributes of the primary pmc +* @fixup: Function to perform platform specific fixup +* @suspend: Function to perform platform specific suspend +* @resume: Function to perform platform specific resume +*/ +struct pmc_dev_info { +u8 func; +bool ssram; +u32 dmu_guid; +struct pmc_info *regmap_list; +const struct pmc_reg_map *map; +void (*fixup)(void ); +void (*suspend)(struct pmc_dev *pmcdev); +int (*resume)(struct pmc_dev *pmcdev); +}; +extern const struct pmc_bit_map msr_map[];extern const struct pmc_bit_map spt_pll_map[];extern const struct pmc_bit_map spt_mphy_map[];
@@ -592,6 +616,7 @@ extern void pmc_core_set_device_d3(unsigned int device);extern 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);int spt_core_init (struct pmc_dev *pmcdev);int cnp_core_init (struct pmc_dev *pmcdev);int icl_core_init (struct pmc_dev *pmcdev);
drivers/platform/x86/intel/pmc/icl.c +6 -12
@@ -50,18 +50,12 @@ const struct pmc_reg_map icl_reg_map = { .etr3_offset = ETR3_OFFSET, }; +static struct pmc_dev_info icl_pmc_dev = { + .map = &icl_reg_map, +}; +int icl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmc->map = &icl_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return ret; +return generic_core_init (pmcdev, &icl_pmc_dev); } +
drivers/platform/x86/intel/pmc/lnl.c +8 -16
@@ -550,22 +550,14 @@ static int lnl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); } +static struct pmc_dev_info lnl_pmc_dev = { + .map = &lnl_socm_reg_map, + .fixup = lnl_d3_fixup, + .suspend = cnl_suspend, + .resume = lnl_resume, +}; +int lnl_core_init (struct pmc_dev *pmcdev) { -int ret; -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC ]; - -lnl_d3_fixup (); - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = lnl_resume; - - pmc->map = &lnl_socm_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &lnl_pmc_dev); }
drivers/platform/x86/intel/pmc/mtl.c +12 -33
51 lines not on this page · raw message
drivers/platform/x86/intel/pmc/spt.c +6 -12
@@ -134,18 +134,12 @@ const struct pmc_reg_map spt_reg_map = { .pm_vric1_offset = SPT_PMC_VRIC1_OFFSET, }; +static struct pmc_dev_info spt_pmc_dev = { + .map = &spt_reg_map, +}; +int spt_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmc->map = &spt_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return ret; +return generic_core_init (pmcdev, &spt_pmc_dev); } +
drivers/platform/x86/intel/pmc/tgl.c +16 -15
@@ -285,35 +285,36 @@ 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) -{ -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; +static struct pmc_dev_info tgl_l_pmc_dev = { + .map = &tgl_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; -if (pch_tp ==PCH_H ) - pmc->map = &tgl_h_reg_map; -else - pmc->map = &tgl_reg_map; +static struct pmc_dev_info tgl_pmc_dev = { + .map = &tgl_h_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; +static int tgl_core_generic_init (struct pmc_dev *pmcdev,struct pmc_dev_info *pmc_dev_info) +{ +int ret; - ret =get_primary_reg_base (pmc); + ret =generic_core_init (pmcdev, &tgl_l_pmc_dev);if (ret)return ret; -pmc_core_get_low_power_modes (pmcdev);pmc_core_get_tgl_lpm_reqs (pmcdev->pdev); -return 0 ; }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
maintainerRe: [PATCH v3 2/3] platform/x86:intel/pmc: Create generic_core_init() for all platforms
Quoted 8 lines · Xi Pardee
Create a
generic_core_init()function for all architecture to reduce duplicate code in each architecture file. Create an info structure to catch the variations between each architecture and pass it to the generic init function.Convert all architectures to call the generic core init function.
Signed-off-by Xi Pardee <[email protected]>
This looks much better!
Quoted 50 lines
--- drivers/platform/x86/intel/pmc/adl.c | 21 ++++-------- drivers/platform/x86/intel/pmc/arl.c | 47 ++++++++------------------- drivers/platform/x86/intel/pmc/cnp.c | 21 ++++-------- drivers/platform/x86/intel/pmc/core.c | 45 +++++++++++++++++++++++++ drivers/platform/x86/intel/pmc/core.h | 25 ++++++++++++++ drivers/platform/x86/intel/pmc/icl.c | 18 ++++------ drivers/platform/x86/intel/pmc/lnl.c | 24 +++++--------- drivers/platform/x86/intel/pmc/mtl.c | 45 +++++++------------------ drivers/platform/x86/intel/pmc/spt.c | 18 ++++------ drivers/platform/x86/intel/pmc/tgl.c | 31 +++++++++--------- 10 files changed, 145 insertions(+), 150 deletions(-)drivers/platform/x86/intel/pmc/adl.c +7
-14@@ -311,20 +311,13 @@ const struct pmc_reg_map adl_reg_map = { .pson_residency_counter_step = TGL_PSON_RES_COUNTER_STEP, }; +static struct pmc_dev_info adl_pmc_dev = { + .map = &adl_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; +int adl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; - - pmc->map = &adl_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &adl_pmc_dev); }drivers/platform/x86/intel/pmc/arl.c +0
-0
drivers/platform/x86/intel/pmc/arl.c static int arl_resume(struct pmc_dev *pmcdev) · line 700
Quoted 10 lines
drivers/platform/x86/intel/pmc/arl.c +7
-0@@ -691,40 +691,19 @@ static int arl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); } +static struct pmc_dev_info arl_pmc_dev = { + .func =0 , + .ssram = true, + .dmu_guid =ARL_PMT_DMU_GUID , + .regmap_list = arl_pmc_info_list, + .map = &arl_socs_reg_map, + .fixup = arl_d3_fixup,
I think the fixups should be left to be called from the per architecture init funcs.
Quoted 90 lines
drivers/platform/x86/intel/pmc/arl.c +6
-34+ .suspend = cnl_suspend, + .resume = arl_resume, +}; +int arl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC ]; -int ret; -int func =0 ; -bool ssram_init = true; - -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); -if (ret) { - ssram_init = false; - pmc->map = &arl_socs_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - } - -pmc_core_get_low_power_modes (pmcdev); -pmc_core_punit_pmt_init (pmcdev,ARL_PMT_DMU_GUID ); - -if (ssram_init) { - ret =pmc_core_ssram_get_lpm_reqs (pmcdev); -if (ret) -return ret; - } - -return 0 ; +return generic_core_init (pmcdev, &arl_pmc_dev); } +drivers/platform/x86/intel/pmc/cnp.c +7
-14@@ -274,20 +274,13 @@ int cnl_resume(struct pmc_dev *pmcdev)return pmc_core_resume_common (pmcdev); } +static struct pmc_dev_info cnp_pmc_dev = { + .map = &cnp_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; +int cnp_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; - - pmc->map = &cnp_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &cnp_pmc_dev); }drivers/platform/x86/intel/pmc/core.c +7
-0@@ -1344,6 +1344,51 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) } } +int generic_core_init (struct pmc_dev *pmcdev,struct pmc_dev_info *pmc_dev_info) +{ +struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; +void (*fixup)(void ) = pmc_dev_info->fixup; +bool ssram; +int ret; +
drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) · line 1363
Quoted 10 lines
drivers/platform/x86/intel/pmc/core.c +10
-0+if (fixup) +fixup (); + + pmcdev->suspend = pmc_dev_info->suspend; + pmcdev->resume = pmc_dev_info->resume; + +/* +* If ssram init fails use legacy method to at least get the +* primary PMC +*/
This comment feels misplaced. I think you could simply make it a function comment instead as it describes the general level behavior within the function.
drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) · line 1366
Quoted 3 lines
drivers/platform/x86/intel/pmc/core.c +3
-0+ ssram = pmc_dev_info->ssram; +if (ssram) { + pmcdev->regmap_list = pmc_dev_info->regmap_list;
I wonder why the pmc_dev_info->ssram is necessary, doesn't ->regmap_list != NULL tell the same information already? You might also want to mention it in the struct pmc_dev_info documentation that it implies SSRAM.
So you could do:
ssram = pmc_dev_info->ssram != NULL;
if (ssram) {
...Quoted 33 lines
drivers/platform/x86/intel/pmc/core.c +25
-0+ ret =pmc_core_ssram_init (pmcdev, pmc_dev_info->func); +if (ret) { +dev_warn (&pmcdev->pdev->dev, +"ssram init failed, %d, using legacy init\n" , ret); + ssram = false; + } + } + +if (!ssram) { + pmc->map = pmc_dev_info->map; + ret =get_primary_reg_base (pmc); +if (ret) +return ret; + } + +pmc_core_get_low_power_modes (pmcdev); +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); + +return 0 ; +} +static const struct x86_cpu_id intel_pmc_core_ids[] = {X86_MATCH_VFM (INTEL_SKYLAKE_L , spt_core_init),X86_MATCH_VFM (INTEL_SKYLAKE , spt_core_init),drivers/platform/x86/intel/pmc/core.h +0
-0
drivers/platform/x86/intel/pmc/core.h enum pmc_index { · line 441
Quoted 6 lines
drivers/platform/x86/intel/pmc/core.h +3
-0@@ -436,6 +436,30 @@ enum pmc_index { PMC_IDX_MAX }; +/** +* struct pmc_dev_info - Structure to keep pmc device info +* @func: Function number of the primary pmc
Capitalize "PMC" in the comments.
Quoted 70 lines
drivers/platform/x86/intel/pmc/core.h +22
-0+* @ssram: Bool shows if platform has ssram support +* @dmu_guid: DMU GUID +* @regmap_list: Pointer to a list of pmc_info structure that could be +* available for the platform +* @map: Pointer to a pmc_reg_map struct that contains platform +* specific attributes of the primary pmc +* @fixup: Function to perform platform specific fixup +* @suspend: Function to perform platform specific suspend +* @resume: Function to perform platform specific resume +*/ +struct pmc_dev_info { +u8 func; +bool ssram; +u32 dmu_guid; +struct pmc_info *regmap_list; +const struct pmc_reg_map *map; +void (*fixup)(void ); +void (*suspend)(struct pmc_dev *pmcdev); +int (*resume)(struct pmc_dev *pmcdev); +}; +extern const struct pmc_bit_map msr_map[];extern const struct pmc_bit_map spt_pll_map[];extern const struct pmc_bit_map spt_mphy_map[];@@ -592,6 +616,7 @@ extern void pmc_core_set_device_d3(unsigned int device);extern 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);int spt_core_init (struct pmc_dev *pmcdev);int cnp_core_init (struct pmc_dev *pmcdev);int icl_core_init (struct pmc_dev *pmcdev);drivers/platform/x86/intel/pmc/icl.c +6
-12@@ -50,18 +50,12 @@ const struct pmc_reg_map icl_reg_map = { .etr3_offset = ETR3_OFFSET, }; +static struct pmc_dev_info icl_pmc_dev = { + .map = &icl_reg_map, +}; +int icl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmc->map = &icl_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return ret; +return generic_core_init (pmcdev, &icl_pmc_dev); } +drivers/platform/x86/intel/pmc/lnl.c +1
-0@@ -550,22 +550,14 @@ static int lnl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); } +static struct pmc_dev_info lnl_pmc_dev = {
drivers/platform/x86/intel/pmc/lnl.c static int lnl_resume(struct pmc_dev *pmcdev) · line 556
Quoted 10 lines
drivers/platform/x86/intel/pmc/lnl.c +6
-2+ .map = &lnl_socm_reg_map, + .fixup = lnl_d3_fixup, + .suspend = cnl_suspend, + .resume = lnl_resume, +}; +int lnl_core_init (struct pmc_dev *pmcdev) { -int ret; -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC ];
Hmm, so PMC_IDX_SOC and PMC_IDX_MAIN are set to same value which I haven't noticed before. I don't know why they were separate to begin with but I think you just removed all user of PMC_IDX_SOC so perhaps that it should be removed from enum as well?
Quoted 148 lines
drivers/platform/x86/intel/pmc/lnl.c +1
-14- -lnl_d3_fixup (); - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = lnl_resume; - - pmc->map = &lnl_socm_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &lnl_pmc_dev); }drivers/platform/x86/intel/pmc/mtl.c +12
-33@@ -990,39 +990,18 @@ static int mtl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); } +static struct pmc_dev_info mtl_pmc_dev = { + .func =2 , + .ssram = true, + .dmu_guid =MTL_PMT_DMU_GUID , + .regmap_list = mtl_pmc_info_list, + .map = &mtl_socm_reg_map, + .fixup = mtl_d3_fixup, + .suspend = cnl_suspend, + .resume = mtl_resume, +}; +int mtl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC ]; -int ret; -int func =2 ; -bool ssram_init = true; - -mtl_d3_fixup (); - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = mtl_resume; - pmcdev->regmap_list = mtl_pmc_info_list; - -/* -* If ssram init fails use legacy method to at least get the -* primary PMC -*/ - ret =pmc_core_ssram_init (pmcdev, func); -if (ret) { - ssram_init = false; -dev_warn (&pmcdev->pdev->dev, -"ssram init failed, %d, using legacy init\n" , ret); - pmc->map = &mtl_socm_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - } - -pmc_core_get_low_power_modes (pmcdev); -pmc_core_punit_pmt_init (pmcdev,MTL_PMT_DMU_GUID ); - -if (ssram_init) -return pmc_core_ssram_get_lpm_reqs (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &mtl_pmc_dev); }drivers/platform/x86/intel/pmc/spt.c +6
-12@@ -134,18 +134,12 @@ const struct pmc_reg_map spt_reg_map = { .pm_vric1_offset = SPT_PMC_VRIC1_OFFSET, }; +static struct pmc_dev_info spt_pmc_dev = { + .map = &spt_reg_map, +}; +int spt_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmc->map = &spt_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return ret; +return generic_core_init (pmcdev, &spt_pmc_dev); } +drivers/platform/x86/intel/pmc/tgl.c +14
-13@@ -285,35 +285,36 @@ 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) -{ -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; +static struct pmc_dev_info tgl_l_pmc_dev = { + .map = &tgl_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; -if (pch_tp ==PCH_H ) - pmc->map = &tgl_h_reg_map; -else - pmc->map = &tgl_reg_map; +static struct pmc_dev_info tgl_pmc_dev = { + .map = &tgl_h_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; +static int tgl_core_generic_init (struct pmc_dev *pmcdev,struct pmc_dev_info *pmc_dev_info) +{ +int ret; - ret =get_primary_reg_base (pmc); + ret =generic_core_init (pmcdev, &tgl_l_pmc_dev);if (ret)return ret; -pmc_core_get_low_power_modes (pmcdev);pmc_core_get_tgl_lpm_reqs (pmcdev->pdev); -return 0 ; }int tgl_l_core_init (struct pmc_dev *pmcdev)
drivers/platform/x86/intel/pmc/tgl.c void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev) · line 320
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); }
It might be also worth to consider what is stored into those X86_MATCH_VFM()s so that the simple init functions could be removed entirely but it could be done in another patch on top of this one.
Signature · 1 line
i.
Xi Pardee
maintainerRe: [PATCH v3 2/3] platform/x86:intel/pmc: Create generic_core_init() for all platforms
Thanks!
My comments are inline.
Xi
Quoted 65 lines · Ilpo Järvinen
Quoted 8 lines · Xi Pardee
Create a
generic_core_init()function for all architecture to reduce duplicate code in each architecture file. Create an info structure to catch the variations between each architecture and pass it to the generic init function.Convert all architectures to call the generic core init function.
Signed-off-by Xi Pardee <[email protected]>This looks much better!
Quoted 53 lines
--- drivers/platform/x86/intel/pmc/adl.c | 21 ++++-------- drivers/platform/x86/intel/pmc/arl.c | 47 ++++++++------------------- drivers/platform/x86/intel/pmc/cnp.c | 21 ++++-------- drivers/platform/x86/intel/pmc/core.c | 45 +++++++++++++++++++++++++ drivers/platform/x86/intel/pmc/core.h | 25 ++++++++++++++ drivers/platform/x86/intel/pmc/icl.c | 18 ++++------ drivers/platform/x86/intel/pmc/lnl.c | 24 +++++--------- drivers/platform/x86/intel/pmc/mtl.c | 45 +++++++------------------ drivers/platform/x86/intel/pmc/spt.c | 18 ++++------ drivers/platform/x86/intel/pmc/tgl.c | 31 +++++++++--------- 10 files changed, 145 insertions(+), 150 deletions(-)drivers/platform/x86/intel/pmc/adl.c +7
-14@@ -311,20 +311,13 @@ const struct pmc_reg_map adl_reg_map = { .pson_residency_counter_step = TGL_PSON_RES_COUNTER_STEP, }; +static struct pmc_dev_info adl_pmc_dev = { + .map = &adl_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; +int adl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; - - pmc->map = &adl_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &adl_pmc_dev); }drivers/platform/x86/intel/pmc/arl.c +0
-0@@ -691,40 +691,19 @@ static int arl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); }
Quoted 9 lines
drivers/platform/x86/intel/pmc/arl.c static int arl_resume(struct pmc_dev *pmcdev) · line 700
Quoted 7 lines
drivers/platform/x86/intel/pmc/arl.c +7
-0+static struct pmc_dev_info arl_pmc_dev = { + .func =0 , + .ssram = true, + .dmu_guid =ARL_PMT_DMU_GUID , + .regmap_list = arl_pmc_info_list, + .map = &arl_socs_reg_map, + .fixup = arl_d3_fixup,I think the fixups should be left to be called from the per architecture init funcs.
Will rename the fixup field to platform_specifc (more explanation at the end).
Quoted 96 lines
Quoted 96 lines
drivers/platform/x86/intel/pmc/arl.c +6
-34+ .suspend = cnl_suspend, + .resume = arl_resume, +}; +int arl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC ]; -int ret; -int func =0 ; -bool ssram_init = true; - -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); -if (ret) { - ssram_init = false; - pmc->map = &arl_socs_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - } - -pmc_core_get_low_power_modes (pmcdev); -pmc_core_punit_pmt_init (pmcdev,ARL_PMT_DMU_GUID ); - -if (ssram_init) { - ret =pmc_core_ssram_get_lpm_reqs (pmcdev); -if (ret) -return ret; - } - -return 0 ; +return generic_core_init (pmcdev, &arl_pmc_dev); } +drivers/platform/x86/intel/pmc/cnp.c +7
-14@@ -274,20 +274,13 @@ int cnl_resume(struct pmc_dev *pmcdev)return pmc_core_resume_common (pmcdev); } +static struct pmc_dev_info cnp_pmc_dev = { + .map = &cnp_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; +int cnp_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; - - pmc->map = &cnp_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &cnp_pmc_dev); }drivers/platform/x86/intel/pmc/core.c +12
-0@@ -1344,6 +1344,51 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) } } +int generic_core_init (struct pmc_dev *pmcdev,struct pmc_dev_info *pmc_dev_info) +{ +struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; +void (*fixup)(void ) = pmc_dev_info->fixup; +bool ssram; +int ret; + +if (fixup) +fixup (); + + pmcdev->suspend = pmc_dev_info->suspend; + pmcdev->resume = pmc_dev_info->resume;
Quoted 8 lines
drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) · line 1363
Quoted 5 lines
drivers/platform/x86/intel/pmc/core.c +5
-0+ +/* +* If ssram init fails use legacy method to at least get the +* primary PMC +*/ This comment feels misplaced. I think you could simply make it a function comment instead as it describes the general level behavior within the function.
Will move the comment to be a function comment for the generic_core_init() function.
Quoted 7 lines
drivers/platform/x86/intel/pmc/core.c static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev) · line 1366
Quoted 3 lines
drivers/platform/x86/intel/pmc/core.c +3
-0+ ssram = pmc_dev_info->ssram; +if (ssram) { + pmcdev->regmap_list = pmc_dev_info->regmap_list;I wonder why the
pmc_dev_info->ssramis necessary, doesn't ->regmap_list != NULL tell the same information already? You might also want to mention it in the structpmc_dev_infodocumentation that it implies SSRAM.
Quoted 5 lines
So you could do:
ssram = pmc_dev_info->ssram != NULL; if (ssram) { ...
Thanks! We don't need ssram field as regmap_list indicates that if ssram support is enabled for each architecture. Will remove ssram in the next version.
Quoted 36 lines
Quoted 36 lines
drivers/platform/x86/intel/pmc/core.c +25
-0+ ret =pmc_core_ssram_init (pmcdev, pmc_dev_info->func); +if (ret) { +dev_warn (&pmcdev->pdev->dev, +"ssram init failed, %d, using legacy init\n" , ret); + ssram = false; + } + } + +if (!ssram) { + pmc->map = pmc_dev_info->map; + ret =get_primary_reg_base (pmc); +if (ret) +return ret; + } + +pmc_core_get_low_power_modes (pmcdev); +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); + +return 0 ; +} +static const struct x86_cpu_id intel_pmc_core_ids[] = {X86_MATCH_VFM (INTEL_SKYLAKE_L , spt_core_init),X86_MATCH_VFM (INTEL_SKYLAKE , spt_core_init),drivers/platform/x86/intel/pmc/core.h +0
-0@@ -436,6 +436,30 @@ enum pmc_index { PMC_IDX_MAX };
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.h enum pmc_index { · line 441
Quoted 3 lines
drivers/platform/x86/intel/pmc/core.h +3
-0+/** +* struct pmc_dev_info - Structure to keep pmc device info +* @func: Function number of the primary pmc Capitalize "PMC" in the comments.
Will change it.
Quoted 70 lines
Quoted 70 lines
drivers/platform/x86/intel/pmc/core.h +22
-0+* @ssram: Bool shows if platform has ssram support +* @dmu_guid: DMU GUID +* @regmap_list: Pointer to a list of pmc_info structure that could be +* available for the platform +* @map: Pointer to a pmc_reg_map struct that contains platform +* specific attributes of the primary pmc +* @fixup: Function to perform platform specific fixup +* @suspend: Function to perform platform specific suspend +* @resume: Function to perform platform specific resume +*/ +struct pmc_dev_info { +u8 func; +bool ssram; +u32 dmu_guid; +struct pmc_info *regmap_list; +const struct pmc_reg_map *map; +void (*fixup)(void ); +void (*suspend)(struct pmc_dev *pmcdev); +int (*resume)(struct pmc_dev *pmcdev); +}; +extern const struct pmc_bit_map msr_map[];extern const struct pmc_bit_map spt_pll_map[];extern const struct pmc_bit_map spt_mphy_map[];@@ -592,6 +616,7 @@ extern void pmc_core_set_device_d3(unsigned int device);extern 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);int spt_core_init (struct pmc_dev *pmcdev);int cnp_core_init (struct pmc_dev *pmcdev);int icl_core_init (struct pmc_dev *pmcdev);drivers/platform/x86/intel/pmc/icl.c +6
-12@@ -50,18 +50,12 @@ const struct pmc_reg_map icl_reg_map = { .etr3_offset = ETR3_OFFSET, }; +static struct pmc_dev_info icl_pmc_dev = { + .map = &icl_reg_map, +}; +int icl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmc->map = &icl_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return ret; +return generic_core_init (pmcdev, &icl_pmc_dev); } +drivers/platform/x86/intel/pmc/lnl.c +0
-0@@ -550,22 +550,14 @@ static int lnl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); }
Quoted 15 lines
drivers/platform/x86/intel/pmc/lnl.c static int lnl_resume(struct pmc_dev *pmcdev) · line 556
Quoted 11 lines
drivers/platform/x86/intel/pmc/lnl.c +7
-2+static struct pmc_dev_info lnl_pmc_dev = { + .map = &lnl_socm_reg_map, + .fixup = lnl_d3_fixup, + .suspend = cnl_suspend, + .resume = lnl_resume, +}; +int lnl_core_init (struct pmc_dev *pmcdev) { -int ret; -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC ];Hmm, so
PMC_IDX_SOCandPMC_IDX_MAINare set to same value which I haven't noticed before. I don't know why they were separate to begin with but I think you just removed all user ofPMC_IDX_SOCso perhaps that it should be removed from enum as well?
Will remove PMC_IDX_SOC from enum in next version.
Quoted 153 lines
Quoted 153 lines
drivers/platform/x86/intel/pmc/lnl.c +1
-14- -lnl_d3_fixup (); - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = lnl_resume; - - pmc->map = &lnl_socm_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &lnl_pmc_dev); }drivers/platform/x86/intel/pmc/mtl.c +12
-33@@ -990,39 +990,18 @@ static int mtl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); } +static struct pmc_dev_info mtl_pmc_dev = { + .func =2 , + .ssram = true, + .dmu_guid =MTL_PMT_DMU_GUID , + .regmap_list = mtl_pmc_info_list, + .map = &mtl_socm_reg_map, + .fixup = mtl_d3_fixup, + .suspend = cnl_suspend, + .resume = mtl_resume, +}; +int mtl_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC ]; -int ret; -int func =2 ; -bool ssram_init = true; - -mtl_d3_fixup (); - - pmcdev->suspend = cnl_suspend; - pmcdev->resume = mtl_resume; - pmcdev->regmap_list = mtl_pmc_info_list; - -/* -* If ssram init fails use legacy method to at least get the -* primary PMC -*/ - ret =pmc_core_ssram_init (pmcdev, func); -if (ret) { - ssram_init = false; -dev_warn (&pmcdev->pdev->dev, -"ssram init failed, %d, using legacy init\n" , ret); - pmc->map = &mtl_socm_reg_map; - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - } - -pmc_core_get_low_power_modes (pmcdev); -pmc_core_punit_pmt_init (pmcdev,MTL_PMT_DMU_GUID ); - -if (ssram_init) -return pmc_core_ssram_get_lpm_reqs (pmcdev); - -return 0 ; +return generic_core_init (pmcdev, &mtl_pmc_dev); }drivers/platform/x86/intel/pmc/spt.c +6
-12@@ -134,18 +134,12 @@ const struct pmc_reg_map spt_reg_map = { .pm_vric1_offset = SPT_PMC_VRIC1_OFFSET, }; +static struct pmc_dev_info spt_pmc_dev = { + .map = &spt_reg_map, +}; +int spt_core_init (struct pmc_dev *pmcdev) { -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; - - pmc->map = &spt_reg_map; - - ret =get_primary_reg_base (pmc); -if (ret) -return ret; - -pmc_core_get_low_power_modes (pmcdev); - -return ret; +return generic_core_init (pmcdev, &spt_pmc_dev); } +drivers/platform/x86/intel/pmc/tgl.c +15
-14@@ -285,35 +285,36 @@ 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) -{ -struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN ]; -int ret; +static struct pmc_dev_info tgl_l_pmc_dev = { + .map = &tgl_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; -if (pch_tp ==PCH_H ) - pmc->map = &tgl_h_reg_map; -else - pmc->map = &tgl_reg_map; +static struct pmc_dev_info tgl_pmc_dev = { + .map = &tgl_h_reg_map, + .suspend = cnl_suspend, + .resume = cnl_resume, +}; - pmcdev->suspend = cnl_suspend; - pmcdev->resume = cnl_resume; +static int tgl_core_generic_init (struct pmc_dev *pmcdev,struct pmc_dev_info *pmc_dev_info) +{ +int ret; - ret =get_primary_reg_base (pmc); + ret =generic_core_init (pmcdev, &tgl_l_pmc_dev);if (ret)return ret; -pmc_core_get_low_power_modes (pmcdev);pmc_core_get_tgl_lpm_reqs (pmcdev->pdev); -return 0 ; }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); }
Quoted 9 lines
drivers/platform/x86/intel/pmc/tgl.c void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev) · line 320
Quoted 6 lines
drivers/platform/x86/intel/pmc/tgl.c +1
-1int tgl_core_init (struct pmc_dev *pmcdev) { -return tgl_core_generic_init (pmcdev,PCH_H ); +return tgl_core_generic_init (pmcdev, &tgl_pmc_dev); }It might be also worth to consider what is stored into those X86_MATCH_VFM()s so that the simple init functions could be removed entirely but it could be done in another patch on top of this one.
Right now we store the init function pointer for each architecture in X86_MATCH_VFM() structure. We could change it to be a pointer to the pmc_dev_info structure instead. Most of the per architecture init function call the generic_init function directly except for TGL init function. The TGL case can be handled by adding a new callback function pointer field named platform_specific and this field can also be used to replace the fixup field.
Thanks!
Xi
Quoted 1 line
Ilpo Järvinen
maintainerRe: [PATCH v3 2/3] platform/x86:intel/pmc: Create generic_core_init() for all platforms
Quoted 95 lines · Xi Pardee
Quoted 40 lines · Ilpo Järvinen
Quoted 8 lines · Xi Pardee
Create a
generic_core_init()function for all architecture to reduce duplicate code in each architecture file. Create an info structure to catch the variations between each architecture and pass it to the generic init function.Convert all architectures to call the generic core init function.
Signed-off-by Xi Pardee <[email protected]>This looks much better!
Quoted 28 lines
--- drivers/platform/x86/intel/pmc/adl.c | 21 ++++-------- drivers/platform/x86/intel/pmc/arl.c | 47 ++++++++------------------- drivers/platform/x86/intel/pmc/cnp.c | 21 ++++-------- drivers/platform/x86/intel/pmc/core.c | 45 +++++++++++++++++++++++++ drivers/platform/x86/intel/pmc/core.h | 25 ++++++++++++++ drivers/platform/x86/intel/pmc/icl.c | 18 ++++------ drivers/platform/x86/intel/pmc/lnl.c | 24 +++++--------- drivers/platform/x86/intel/pmc/mtl.c | 45 +++++++------------------ drivers/platform/x86/intel/pmc/spt.c | 18 ++++------ drivers/platform/x86/intel/pmc/tgl.c | 31 +++++++++--------- 10 files changed, 145 insertions(+), 150 deletions(-)drivers/platform/x86/intel/pmc/adl.c +0
-0drivers/platform/x86/intel/pmc/arl.c +0
-0@@ -691,40 +691,19 @@ static int arl_resume(struct pmc_dev *pmcdev)return cnl_resume (pmcdev); } +static struct pmc_dev_info arl_pmc_dev = {Quoted 8 lines
drivers/platform/x86/intel/pmc/arl.c static int arl_resume(struct pmc_dev *pmcdev) · line 700
Quoted 6 lines
drivers/platform/x86/intel/pmc/arl.c +6
-0+ .func = 0, + .ssram = true, + .dmu_guid = ARL_PMT_DMU_GUID, + .regmap_list = arl_pmc_info_list, + .map = &arl_socs_reg_map, + .fixup = arl_d3_fixup,I think the fixups should be left to be called from the per architecture init funcs.
Will rename the fixup field to
platform_specifc(more explanation at the end).Quoted 4 lines
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.h +0
-1--- a/drivers/platform/x86/intel/pmc/core.hQuoted 8 lines
drivers/platform/x86/intel/pmc/core.h enum pmc_index { · line 440
Quoted 7 lines
+++
b/drivers/platform/x86/intel/pmc/core.hdrivers/platform/x86/intel/pmc/core.h +2
-0@@ -436,6 +436,30 @@ enum pmc_index {PMC_IDX_MAX }; +/** +* struct pmc_dev_info - Structure to keep pmc device info +* @func: Function number of the primary pmc Capitalize "PMC" in the comments.
Will change it.
Quoted 22 lines
Quoted 21 lines
drivers/platform/x86/intel/pmc/core.h +20
-0+* @ssram: Bool shows if platform has ssram support +* @dmu_guid: DMU GUID +* @regmap_list: Pointer to a list of pmc_info structure that could be +* available for the platform +* @map: Pointer to a pmc_reg_map struct that contains platform +* specific attributes of the primary pmc +* @fixup: Function to perform platform specific fixup +* @suspend: Function to perform platform specific suspend +* @resume: Function to perform platform specific resume +*/ +struct pmc_dev_info { +u8 func; +bool ssram; +u32 dmu_guid; +struct pmc_info *regmap_list; +const struct pmc_reg_map *map; +void (*fixup)(void ); +void (*suspend)(struct pmc_dev *pmcdev); +int (*resume)(struct pmc_dev *pmcdev); +};Quoted 7 lines
drivers/platform/x86/intel/pmc/core.h enum pmc_index { · line 463
Quoted 4 lines
drivers/platform/x86/intel/pmc/core.h +1
-1-return tgl_core_generic_init (pmcdev,PCH_H ); +return tgl_core_generic_init (pmcdev, &tgl_pmc_dev); }It might be also worth to consider what is stored into those X86_MATCH_VFM()s so that the simple init functions could be removed entirely but it could be done in another patch on top of this one.
Quoted 9 lines
Right now we store the init function pointer for each architecture in
X86_MATCH_VFM()
structure. We could change it to be a pointer to thepmc_dev_infostructure instead. Most
of the per architecture init function call thegeneric_initfunction directly except for TGL
init function. The TGL case can be handled by adding a new callback function pointer field namedplatform_specificand this field can also be used to replace the fixup field.
To preserve generality, the function pointer should be a real replacement of the generic init function (if non-NULL), not some platform specific hook is called from one point of the generic init function.
If the init function is NULL, just call the generic init function directly.
If not NULL, that per platform replacement function in can then do the fixup like it currently does (and whatever extra is needed) and call the generic init function.
Signature · 1 line
i.
Xi Pardee
maintainerPATCH v3 3/3 platform/x86/intel/pmc: Add Arrow Lake U/H support to intel_pmc_core driver
In reply to Xi PardeeAdd 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 | 36 +++++++++++++++++++++++++++ drivers/platform/x86/intel/pmc/core.c | 2 ++ drivers/platform/x86/intel/pmc/core.h | 1 + 3 files changed, 39 insertions(+)
drivers/platform/x86/intel/pmc/arl.c +36 -0
@@ -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 0x2625030static 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,6 +705,13 @@ static int arl_resume(struct pmc_dev *pmcdev)return cnl_resume (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 , .ssram = true,
@@ -702,8 +723,23 @@ static struct pmc_dev_info arl_pmc_dev = { .resume = arl_resume, }; +static struct pmc_dev_info arl_h_pmc_dev = { + .func =2 , + .ssram = true, + .dmu_guid =ARL_PMT_DMU_GUID , + .regmap_list = arl_pmc_info_list, + .map = &mtl_socm_reg_map, + .fixup = arl_h_d3_fixup, + .suspend = cnl_suspend, + .resume = arl_h_resume, +}; +int arl_core_init (struct pmc_dev *pmcdev) {return generic_core_init (pmcdev, &arl_pmc_dev); } +int arl_h_core_init (struct pmc_dev *pmcdev) +{ +return generic_core_init (pmcdev, &arl_h_pmc_dev); +}
drivers/platform/x86/intel/pmc/core.c +2 -0
@@ -1412,6 +1412,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
@@ -625,6 +625,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