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@xxxxxxxxxxxxxxx>
> ---
> 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);
> }
>
