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

[Xi Pardee <xi.pardee@linux.intel.com>]

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@linux.intel.com>
>> ---
>>   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;