From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: "David E. Box" <david.e.box@linux.intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
platform-driver-x86@vger.kernel.org,
rajvi.jingar@linux.intel.com
Subject: Re: [PATCH V4 11/17] platform/x86/intel/pmc: Split pmc_core_ssram_get_pmc()
Date: Mon, 23 Oct 2023 19:01:32 +0300 (EEST) [thread overview]
Message-ID: <b788487-df7c-78ac-e82a-fb48d36bbd26@linux.intel.com> (raw)
In-Reply-To: <20231018231624.1044633-12-david.e.box@linux.intel.com>
On Wed, 18 Oct 2023, David E. Box wrote:
> On supported hardware, each PMC may have an associated SSRAM device for
> accessing additional counters. However, only the SSRAM of the first
> (primary) PMC is discoverable as a PCI device to the OS. The remaining
> (secondary) devices are hidden but their BARs are still accessible and
> their addresses are stored in the BAR of the exposed device. Clean up the
> code handling the SSRAM discovery. Create two separate functions for
> accessing the primary and secondary SSRAM devices.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> V4 - Add checking the return value from pmc_core_sram_init() to mtl.c
> - Use iounmap cleanup from io.h
>
> V3 - New patch split from previous PATCH 2
> - Update changelog
> - Use cleanup.h to cleanup ioremap
>
> V2 - no change
>
> drivers/platform/x86/intel/pmc/core_ssram.c | 91 +++++++++++++--------
> drivers/platform/x86/intel/pmc/mtl.c | 12 ++-
> 2 files changed, 67 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/pmc/core_ssram.c b/drivers/platform/x86/intel/pmc/core_ssram.c
> index 815950713e25..ccb3748dbed9 100644
> --- a/drivers/platform/x86/intel/pmc/core_ssram.c
> +++ b/drivers/platform/x86/intel/pmc/core_ssram.c
> @@ -8,6 +8,7 @@
> *
> */
>
> +#include <linux/cleanup.h>
> #include <linux/pci.h>
> #include <linux/io-64-nonatomic-lo-hi.h>
>
> @@ -65,44 +66,74 @@ pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
> return 0;
> }
>
> -static void
> -pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, void __iomem *ssram, u32 offset,
> - int pmc_idx)
> +static int
> +pmc_core_get_secondary_pmc(struct pmc_dev *pmcdev, int pmc_idx, u32 offset)
> {
> - u64 pwrm_base;
> + struct pci_dev *ssram_pcidev = pmcdev->ssram_pcidev;
> + void __iomem __free(iounmap) *main_ssram = NULL;
> + void __iomem __free(iounmap) *secondary_ssram = NULL;
> + const struct pmc_reg_map *map;
> + u64 ssram_base, pwrm_base;
> u16 devid;
>
> - if (pmc_idx != PMC_IDX_SOC) {
> - u64 ssram_base = get_base(ssram, offset);
> + if (!pmcdev->regmap_list)
> + return -ENOENT;
>
> - if (!ssram_base)
> - return;
> + /*
> + * The secondary PMC BARS (which are behind hidden PCI devices) are read
> + * from fixed offsets in MMIO of the primary PMC BAR.
> + */
> + ssram_base = ssram_pcidev->resource[0].start;
> + main_ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
> + if (!main_ssram)
> + return -ENOMEM;
>
> - ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
> - if (!ssram)
> - return;
> - }
> + ssram_base = get_base(main_ssram, offset);
> + secondary_ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
> + if (!secondary_ssram)
> + return -ENOMEM;
> +
> + pwrm_base = get_base(secondary_ssram, SSRAM_PWRM_OFFSET);
> + devid = readw(secondary_ssram + SSRAM_DEVID_OFFSET);
> +
> + map = pmc_core_find_regmap(pmcdev->regmap_list, devid);
> + if (!map)
> + return -ENODEV;
> +
> + return pmc_core_pmc_add(pmcdev, pwrm_base, map, pmc_idx);
> +}
> +
> +static int
> +pmc_core_get_primary_pmc(struct pmc_dev *pmcdev)
> +{
> + struct pci_dev *ssram_pcidev = pmcdev->ssram_pcidev;
> + void __iomem __free(iounmap) *ssram;
> + const struct pmc_reg_map *map;
> + u64 ssram_base, pwrm_base;
> + u16 devid;
> +
> + if (!pmcdev->regmap_list)
> + return -ENOENT;
> +
> + /* The primary PMC (SOC die) BAR is BAR 0 in config space. */
> + ssram_base = ssram_pcidev->resource[0].start;
> + ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
> + if (!ssram)
> + return -ENOMEM;
>
> pwrm_base = get_base(ssram, SSRAM_PWRM_OFFSET);
> devid = readw(ssram + SSRAM_DEVID_OFFSET);
>
> - if (pmcdev->regmap_list) {
> - const struct pmc_reg_map *map;
> + map = pmc_core_find_regmap(pmcdev->regmap_list, devid);
> + if (!map)
> + return -ENODEV;
>
> - map = pmc_core_find_regmap(pmcdev->regmap_list, devid);
> - if (map)
> - pmc_core_pmc_add(pmcdev, pwrm_base, map, pmc_idx);
> - }
> -
> - if (pmc_idx != PMC_IDX_SOC)
> - iounmap(ssram);
> + return pmc_core_pmc_add(pmcdev, pwrm_base, map, PMC_IDX_MAIN);
While I very much like the new way pmc_core_get_*_pmc() is structured with
early returns and use of cleanup.h, it feels somethat unnecessary to split
the main logic into two functions given how little there is different.
I'd just handle the differences with if (secondary) { ... } and create
pmc_ssram local variable so as much as possible can be kept in common.
It probably makes still sense to preserve
pmc_core_get_primary/secondary_pmc() functions from the caller point so
those two functions just become wrappers passing correct parameters to
pmc_core_get_pmc().
--
i.
> }
>
> int pmc_core_ssram_init(struct pmc_dev *pmcdev)
> {
> - void __iomem *ssram;
> struct pci_dev *pcidev;
> - u64 ssram_base;
> int ret;
>
> pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, 2));
> @@ -113,18 +144,14 @@ int pmc_core_ssram_init(struct pmc_dev *pmcdev)
> if (ret)
> goto release_dev;
>
> - ssram_base = pcidev->resource[0].start;
> - ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
> - if (!ssram)
> - goto disable_dev;
> -
> pmcdev->ssram_pcidev = pcidev;
>
> - pmc_core_ssram_get_pmc(pmcdev, ssram, 0, PMC_IDX_SOC);
> - pmc_core_ssram_get_pmc(pmcdev, ssram, SSRAM_IOE_OFFSET, PMC_IDX_IOE);
> - pmc_core_ssram_get_pmc(pmcdev, ssram, SSRAM_PCH_OFFSET, PMC_IDX_PCH);
> + ret = pmc_core_get_primary_pmc(pmcdev);
> + if (ret)
> + goto disable_dev;
>
> - iounmap(ssram);
> + pmc_core_get_secondary_pmc(pmcdev, PMC_IDX_IOE, SSRAM_IOE_OFFSET);
> + pmc_core_get_secondary_pmc(pmcdev, PMC_IDX_PCH, SSRAM_PCH_OFFSET);
>
> return 0;
>
> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
> index c3b5f4fe01d1..d1d3d33fb4b8 100644
> --- a/drivers/platform/x86/intel/pmc/mtl.c
> +++ b/drivers/platform/x86/intel/pmc/mtl.c
> @@ -990,12 +990,16 @@ int mtl_core_init(struct pmc_dev *pmcdev)
> mtl_d3_fixup();
>
> pmcdev->resume = mtl_resume;
> -
> pmcdev->regmap_list = mtl_pmc_info_list;
> - pmc_core_ssram_init(pmcdev);
>
> - /* If regbase not assigned, set map and discover using legacy method */
> - if (!pmc->regbase) {
> + /*
> + * If ssram init fails use legacy method to at least get the
> + * primary PMC
> + */
> + ret = pmc_core_ssram_init(pmcdev);
> + if (ret) {
> + 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)
>
next prev parent reply other threads:[~2023-10-23 16:02 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 23:16 [PATCH V4 00/17] intel_pmc: Add telemetry API to read counters David E. Box
2023-10-18 23:16 ` [PATCH V4 01/17] platform/x86/intel/vsec: Move structures to header David E. Box
2023-10-18 23:16 ` [PATCH V4 02/17] platform/x86/intel/vsec: remove platform_info from vsec device structure David E. Box
2023-10-18 23:16 ` [PATCH V4 03/17] platform/x86/intel/vsec: Use cleanup.h David E. Box
2023-10-19 1:03 ` kernel test robot
2023-10-23 15:15 ` Ilpo Järvinen
2023-10-18 23:16 ` [PATCH V4 04/17] platform/x86/intel/vsec: Add intel_vsec_register David E. Box
2023-10-23 15:21 ` Ilpo Järvinen
2023-10-24 23:25 ` David E. Box
2023-10-18 23:16 ` [PATCH V4 05/17] platform/x86/intel/vsec: Add base address field David E. Box
2023-10-18 23:16 ` [PATCH V4 06/17] platform/x86/intel/pmt: Add header to struct intel_pmt_entry David E. Box
2023-10-23 15:14 ` Ilpo Järvinen
2023-10-18 23:16 ` [PATCH V4 07/17] platform/x86/intel/pmt: telemetry: Export API to read telemetry David E. Box
2023-10-18 23:16 ` [PATCH V4 08/17] platform/x86:intel/pmc: Call pmc_get_low_power_modes from platform init David E. Box
2023-10-18 23:16 ` [PATCH V4 09/17] platform/x86/intel/pmc: Allow pmc_core_ssram_init to fail David E. Box
2023-10-23 15:17 ` Ilpo Järvinen
2023-10-18 23:16 ` [PATCH V4 10/17] linux/io.h: iounmap/ioport_unmap cleanup.h support David E. Box
2023-10-19 2:12 ` kernel test robot
2023-10-19 2:23 ` kernel test robot
2023-10-23 15:25 ` Ilpo Järvinen
2023-10-18 23:16 ` [PATCH V4 11/17] platform/x86/intel/pmc: Split pmc_core_ssram_get_pmc() David E. Box
2023-10-23 16:01 ` Ilpo Järvinen [this message]
2023-10-18 23:16 ` [PATCH V4 12/17] platform/x86/intel/pmc: Find and register PMC telemetry entries David E. Box
2023-10-18 23:16 ` [PATCH V4 13/17] platform/x86/intel/pmc: Display LPM requirements for multiple PMCs David E. Box
2023-10-23 16:15 ` Ilpo Järvinen
2023-10-18 23:16 ` [PATCH V4 14/17] platform/x86/intel/pmc: Retrieve LPM information using Intel PMT David E. Box
2023-10-23 16:19 ` Ilpo Järvinen
2023-10-18 23:16 ` [PATCH V4 15/17] platform/x86/intel/pmc: Read low power mode requirements for MTL-M and MTL-P David E. Box
2023-10-18 23:16 ` [PATCH V4 16/17] platform/x86/intel/pmc: Add debug attribute for Die C6 counter David E. Box
2023-10-23 16:31 ` Ilpo Järvinen
2023-10-26 20:28 ` David E. Box
2023-10-18 23:16 ` [PATCH V4 17/17] platform/x86/intel/pmc: Show Die C6 counter on Meteor Lake David E. Box
2023-10-23 16:33 ` Ilpo Järvinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b788487-df7c-78ac-e82a-fb48d36bbd26@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=david.e.box@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rajvi.jingar@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox