| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f694390-079a-13e6-5c93-38b938125044@linux.intel.com>
Date: Fri, 20 Dec 2024 13:52:55 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Xi Pardee <xi.pardee@...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
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.
> + .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.
> + 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) {
...
> + 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.
> + * @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?
> -
> - 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.
--
i.
Powered by blists - more mailing lists