Message-ID: <0810bc98-ea48-4d30-bfb1-bd009ee485de@linux.intel.com>
Date: Fri, 20 Dec 2024 11:55:39 -0800
From: Xi Pardee <xi.pardee@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: rajvi0912@...il.com, irenic.rajneesh@...il.com,
 david.e.box@...ux.intel.com, Hans de Goede <hdegoede@...hat.com>,
 platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
 linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 2/3] platform/x86:intel/pmc: Create generic_core_init()
 for all platforms

Thanks!

My comments are inline.

Xi

On 12/20/2024 3:52 AM, Ilpo Järvinen wrote:
> On Thu, 19 Dec 2024, Xi Pardee wrote:
>
>> 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 <xi.pardee@...ux.intel.com>
> This looks much better!
>
>> ---
>>   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(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/adl.c b/drivers/platform/x86/intel/pmc/adl.c
>> index e7878558fd909..ac37f4ece9c70 100644
>> --- a/drivers/platform/x86/intel/pmc/adl.c
>> +++ b/drivers/platform/x86/intel/pmc/adl.c
>> @@ -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);
>>   }
>> diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
>> index 05dec4f5019f3..137a1fdfee715 100644
>> --- a/drivers/platform/x86/intel/pmc/arl.c
>> +++ b/drivers/platform/x86/intel/pmc/arl.c
>> @@ -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.

Will rename the fixup field to platform_specifc (more explanation at the 
end).

>
>> +	.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);
>>   }
>> +
>> diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c
>> index fc5193fdf8a88..6d268058e40b9 100644
>> --- a/drivers/platform/x86/intel/pmc/cnp.c
>> +++ b/drivers/platform/x86/intel/pmc/cnp.c
>> @@ -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);
>>   }
>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>> index 3e7f99ac8c941..8b73101dcfe95 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -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;
>> +
>> +	/*
>> +	 * 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.
>> +	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) {
> 		...
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.
>> +		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),
>> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
>> index a1886d8e1ef3e..82be953db9463 100644
>> --- a/drivers/platform/x86/intel/pmc/core.h
>> +++ b/drivers/platform/x86/intel/pmc/core.h
>> @@ -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.
>
>> + * @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);
>> diff --git a/drivers/platform/x86/intel/pmc/icl.c b/drivers/platform/x86/intel/pmc/icl.c
>> index 71b0fd6cb7d84..f044546e1aa5e 100644
>> --- a/drivers/platform/x86/intel/pmc/icl.c
>> +++ b/drivers/platform/x86/intel/pmc/icl.c
>> @@ -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);
>>   }
>> +
>> diff --git a/drivers/platform/x86/intel/pmc/lnl.c b/drivers/platform/x86/intel/pmc/lnl.c
>> index be029f12cdf40..8f6b2a8d30438 100644
>> --- a/drivers/platform/x86/intel/pmc/lnl.c
>> +++ b/drivers/platform/x86/intel/pmc/lnl.c
>> @@ -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];
> 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?
Will remove PMC_IDX_SOC from enum in next version.
>
>> -
>> -	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);
>>   }
>> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
>> index 02949fed76e91..b7a752e8adbc6 100644
>> --- a/drivers/platform/x86/intel/pmc/mtl.c
>> +++ b/drivers/platform/x86/intel/pmc/mtl.c
>> @@ -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);
>>   }
>> diff --git a/drivers/platform/x86/intel/pmc/spt.c b/drivers/platform/x86/intel/pmc/spt.c
>> index ab993a69e33ee..09d3ce09af736 100644
>> --- a/drivers/platform/x86/intel/pmc/spt.c
>> +++ b/drivers/platform/x86/intel/pmc/spt.c
>> @@ -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);
>>   }
>> +
>> diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c
>> index 4fec43d212d01..43a2aec4a5673 100644
>> --- a/drivers/platform/x86/intel/pmc/tgl.c
>> +++ b/drivers/platform/x86/intel/pmc/tgl.c
>> @@ -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);
>>   }
>>
> 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

>

Powered by blists - more mailing lists