Re: [PATCH v4 2/4] platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver

[Xi Pardee]

On 4/24/2025 7:01 AM, Ilpo Järvinen wrote:
> On Mon, 21 Apr 2025, Xi Pardee wrote:
>
> > Convert ssram device related functionalities to a new driver named Intel
> > PMC SSRAM Telemetry driver. Modify PMC Core driver to use API exported by
> > the driver to discover and achieve devid and PWRMBASE address information
> > for each available PMC. PMC Core driver needs to get PCI device when
> > reading from telemetry regions.
> >
> > The new SSRAM driver binds to the SSRAM device and provides the following
> > functionalities:
> > 1. Look for and register telemetry regions available in SSRAM device.
> > 2. Provide devid and PWRMBASE address information for the corresponding
> >     PMCs.
> >
> > Signed-off-by: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/platform/x86/intel/pmc/Kconfig        |   4 +
> >   drivers/platform/x86/intel/pmc/Makefile       |   8 +-
> >   drivers/platform/x86/intel/pmc/core.c         |  79 +++++++---
> >   drivers/platform/x86/intel/pmc/core.h         |   7 -
> >   .../platform/x86/intel/pmc/ssram_telemetry.c  | 147 ++++++++++++------
> >   .../platform/x86/intel/pmc/ssram_telemetry.h  |  24 +++
> >   6 files changed, 188 insertions(+), 81 deletions(-)
> >   create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h
> >
> > diff --git a/drivers/platform/x86/intel/pmc/Kconfig b/drivers/platform/x86/intel/pmc/Kconfig
> > index d2f651fbec2c..c6ef0bcf76af 100644
> > --- a/drivers/platform/x86/intel/pmc/Kconfig
> > +++ b/drivers/platform/x86/intel/pmc/Kconfig
> > @@ -8,6 +8,7 @@ config INTEL_PMC_CORE
> >   	depends on PCI
> >   	depends on ACPI
> >   	depends on INTEL_PMT_TELEMETRY
> > +	select INTEL_PMC_SSRAM_TELEMETRY
> >   	help
> >   	  The Intel Platform Controller Hub for Intel Core SoCs provides access
> >   	  to Power Management Controller registers via various interfaces. This
> > @@ -24,3 +25,6 @@ config INTEL_PMC_CORE
> >   		- SLPS0 Debug registers (Cannonlake/Icelake PCH)
> >   		- Low Power Mode registers (Tigerlake and beyond)
> >   		- PMC quirks as needed to enable SLPS0/S0ix
> > +
> > +config INTEL_PMC_SSRAM_TELEMETRY
> > +	tristate
> > diff --git a/drivers/platform/x86/intel/pmc/Makefile b/drivers/platform/x86/intel/pmc/Makefile
> > index e842647d3ced..5f68c8503a56 100644
> > --- a/drivers/platform/x86/intel/pmc/Makefile
> > +++ b/drivers/platform/x86/intel/pmc/Makefile
> > @@ -3,8 +3,12 @@
> >   # Intel x86 Platform-Specific Drivers
> >   #
> >   
> > -intel_pmc_core-y			:= core.o ssram_telemetry.o spt.o cnp.o \
> > -					   icl.o tgl.o adl.o mtl.o arl.o lnl.o ptl.o
> > +intel_pmc_core-y			:= core.o spt.o cnp.o icl.o \
> > +					   tgl.o adl.o mtl.o arl.o lnl.o ptl.o
> >   obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core.o
> >   intel_pmc_core_pltdrv-y			:= pltdrv.o
> >   obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core_pltdrv.o
> > +
> > +# Intel PMC SSRAM driver
> > +intel_pmc_ssram_telemetry-y		+= ssram_telemetry.o
> > +obj-$(CONFIG_INTEL_PMC_SSRAM_TELEMETRY)	+= intel_pmc_ssram_telemetry.o
> > diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> > index a53a7677122c..042b60c1185f 100644
> > --- a/drivers/platform/x86/intel/pmc/core.c
> > +++ b/drivers/platform/x86/intel/pmc/core.c
> > @@ -29,6 +29,7 @@
> >   #include <asm/tsc.h>
> >   
> >   #include "core.h"
> > +#include "ssram_telemetry.h"
> >   #include "../pmt/telemetry.h"
> >   
> >   /* Maximum number of modes supported by platfoms that has low power mode capability */
> > @@ -1354,7 +1355,7 @@ static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *m
> >   	return 0;
> >   }
> >   
> > -static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
> > +static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct pci_dev *pcidev)
> >   {
> >   	struct telem_endpoint *ep;
> >   	const u8 *lpm_indices;
> > @@ -1371,7 +1372,7 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
> >   	if (!guid)
> >   		return -ENXIO;
> >   
> > -	ep = pmt_telem_find_and_register_endpoint(pmcdev->ssram_pcidev, guid, 0);
> > +	ep = pmt_telem_find_and_register_endpoint(pcidev, guid, 0);
> >   	if (IS_ERR(ep)) {
> >   		dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
> >   			PTR_ERR(ep));
> > @@ -1455,27 +1456,29 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
> >   	return ret;
> >   }
> >   
> > -static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
> > +static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev, int func)
> >   {
> > +	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
> >   	unsigned int i;
> > -	int ret;
> > +	int ret = 0;
> After you remove the return changes below, this change is not needed
> either.
>
> >   
> > -	if (!pmcdev->ssram_pcidev)
> > +	pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, func));
> > +	if (!pcidev)
> >   		return -ENODEV;
> >   
> >   	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
> >   		if (!pmcdev->pmcs[i])
> >   			continue;
> >   
> > -		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
> > +		ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i], pcidev);
> >   		if (ret)
> > -			return ret;
> > +			break;
> These two return changes are unnecessary.

Thanks for the comments.

Will remove these return changes in next version.

Xi

> >   	}
> >   
> > -	return 0;
> > +	return ret;