Re: [PATCH v4 4/6] platform/x86/intel/pmc: Create architecture specific callback

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 9 Jan 2025, Xi Pardee wrote:

> Add architecture specific callback field in pmc_dev_info structure.
> Architecture specific action could be handled in this callback instead
> of per architecture init functions. Convert Arrow Lake, Lunar Lake,
> Meteor Lake and Tiger Lake platforms to use this field.
> 
> Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/arl.c  | 15 +++++++--------
>  drivers/platform/x86/intel/pmc/core.c |  3 +++
>  drivers/platform/x86/intel/pmc/core.h |  7 +++++++
>  drivers/platform/x86/intel/pmc/lnl.c  | 15 +++++++--------
>  drivers/platform/x86/intel/pmc/mtl.c  | 15 +++++++--------
>  drivers/platform/x86/intel/pmc/tgl.c  | 15 +++++----------
>  6 files changed, 36 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
> index dedf752237ca0..9ff90d32a635f 100644
> --- a/drivers/platform/x86/intel/pmc/arl.c
> +++ b/drivers/platform/x86/intel/pmc/arl.c
> @@ -698,16 +698,15 @@ static struct pmc_dev_info arl_pmc_dev = {
>  	.map = &arl_socs_reg_map,
>  	.suspend = cnl_suspend,
>  	.resume = arl_resume,
> +	.arch_specific = arl_specific_init,
>  };
>  
> -int arl_core_init(struct pmc_dev *pmcdev)
> +void arl_specific_init(struct pmc_dev *pmcdev)
>  {
> -	int ret;
> -
> -	ret = generic_core_init(pmcdev, &arl_pmc_dev);
> -	if (ret)
> -		return ret;
> -
>  	arl_d3_fixup();
> -	return 0;
> +}

As I tried to explain already earlier I think the older form is better 
here because it would allow arch specific things in any order:

void xx_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
{
	int ret;

	xx_pre_fixup();

	ret = generic_core_init(pmcdev, pmc_dev_info);
	if (ret)
		return ret;

	xx_post_fixup();

	return 0;
}

If you make it a callback, you have pick either pre or post but cannot do 
both with a single callback. My suggestion would also allow replacing 
generic_core_init() completely if needed in future.

Note how I pass the info parameter above to xx_init() so it can call into
generic_core_init(). In the core, you'd do this to pick which init 
function to use:

	if (pmc_dev_info->init)
		ret = pmc_dev_info->init(pmcdev, pmc_dev_info);
	else
		ret = generic_core_init(pmcdev, pmc_dev_info);

-- 
 i.

> +
> +int arl_core_init(struct pmc_dev *pmcdev)
> +{
> +	return generic_core_init(pmcdev, &arl_pmc_dev);
>  }
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 64b1c15e0c81d..45efe0e948831 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -1380,6 +1380,9 @@ 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 (pmc_dev_info->arch_specific)
> +		pmc_dev_info->arch_specific(pmcdev);
> +
>  	if (ssram)
>  		return pmc_core_ssram_get_lpm_reqs(pmcdev);
>  
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index 80adae582ce5f..9430e4209ce97 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -446,6 +446,7 @@ enum pmc_index {
>   *			specific attributes of the primary PMC
>   * @suspend:		Function to perform platform specific suspend
>   * @resume:		Function to perform platform specific resume
> + * @arch_specific:	Function to perform platform specific init action
>   */
>  struct pmc_dev_info {
>  	u8 func;
> @@ -454,6 +455,7 @@ struct pmc_dev_info {
>  	const struct pmc_reg_map *map;
>  	void (*suspend)(struct pmc_dev *pmcdev);
>  	int (*resume)(struct pmc_dev *pmcdev);
> +	void (*arch_specific)(struct pmc_dev *pmcdev);
>  };
>  
>  extern const struct pmc_bit_map msr_map[];
> @@ -623,6 +625,11 @@ int mtl_core_init(struct pmc_dev *pmcdev);
>  int arl_core_init(struct pmc_dev *pmcdev);
>  int lnl_core_init(struct pmc_dev *pmcdev);
>  
> +void arl_specific_init(struct pmc_dev *pmcdev);
> +void mtl_specific_init(struct pmc_dev *pmcdev);
> +void lnl_specific_init(struct pmc_dev *pmcdev);
> +void tgl_specific_init(struct pmc_dev *pmcdev);
> +
>  void cnl_suspend(struct pmc_dev *pmcdev);
>  int cnl_resume(struct pmc_dev *pmcdev);
>  
> diff --git a/drivers/platform/x86/intel/pmc/lnl.c b/drivers/platform/x86/intel/pmc/lnl.c
> index 2e6d4fddd2858..957d7a9062397 100644
> --- a/drivers/platform/x86/intel/pmc/lnl.c
> +++ b/drivers/platform/x86/intel/pmc/lnl.c
> @@ -554,16 +554,15 @@ static struct pmc_dev_info lnl_pmc_dev = {
>  	.map = &lnl_socm_reg_map,
>  	.suspend = cnl_suspend,
>  	.resume = lnl_resume,
> +	.arch_specific = lnl_specific_init,
>  };
>  
> -int lnl_core_init(struct pmc_dev *pmcdev)
> +void lnl_specific_init(struct pmc_dev *pmcdev)
>  {
> -	int ret;
> -
> -	ret = generic_core_init(pmcdev, &lnl_pmc_dev);
> -	if (ret)
> -		return ret;
> -
>  	lnl_d3_fixup();
> -	return 0;
> +}
> +
> +int lnl_core_init(struct pmc_dev *pmcdev)
> +{
> +	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 3bc0b64d19141..2a7d79dd37d6a 100644
> --- a/drivers/platform/x86/intel/pmc/mtl.c
> +++ b/drivers/platform/x86/intel/pmc/mtl.c
> @@ -997,16 +997,15 @@ static struct pmc_dev_info mtl_pmc_dev = {
>  	.map = &mtl_socm_reg_map,
>  	.suspend = cnl_suspend,
>  	.resume = mtl_resume,
> +	.arch_specific = mtl_specific_init,
>  };
>  
> -int mtl_core_init(struct pmc_dev *pmcdev)
> +void mtl_specific_init(struct pmc_dev *pmcdev)
>  {
> -	int ret;
> -
> -	ret = generic_core_init(pmcdev, &mtl_pmc_dev);
> -	if (ret)
> -		return ret;
> -
>  	mtl_d3_fixup();
> -	return 0;
> +}
> +
> +int mtl_core_init(struct pmc_dev *pmcdev)
> +{
> +	return generic_core_init(pmcdev, &mtl_pmc_dev);
>  }
> diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c
> index bc3cb949c672e..29a9109afc782 100644
> --- a/drivers/platform/x86/intel/pmc/tgl.c
> +++ b/drivers/platform/x86/intel/pmc/tgl.c
> @@ -289,32 +289,27 @@ static struct pmc_dev_info tgl_l_pmc_dev = {
>  	.map = &tgl_reg_map,
>  	.suspend = cnl_suspend,
>  	.resume = cnl_resume,
> +	.arch_specific = tgl_specific_init,
>  };
>  
>  static struct pmc_dev_info tgl_pmc_dev = {
>  	.map = &tgl_h_reg_map,
>  	.suspend = cnl_suspend,
>  	.resume = cnl_resume,
> +	.arch_specific = tgl_specific_init,
>  };
>  
> -static int tgl_core_generic_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
> +void tgl_specific_init(struct pmc_dev *pmcdev)
>  {
> -	int ret;
> -
> -	ret = generic_core_init(pmcdev, pmc_dev_info);
> -	if (ret)
> -		return ret;
> -
>  	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, &tgl_l_pmc_dev);
> +	return generic_core_init(pmcdev, &tgl_l_pmc_dev);
>  }
>  
>  int tgl_core_init(struct pmc_dev *pmcdev)
>  {
> -	return tgl_core_generic_init(pmcdev, &tgl_pmc_dev);
> +	return generic_core_init(pmcdev, &tgl_pmc_dev);
>  }
>