On 8/13/2024 9:01 AM, Ilpo Järvinen wrote:
On Fri, 9 Aug 2024, Xi Pardee wrote:
This patch series removes the SSRAM support from Intel PMC Core driver
and creates a separate PCI driver for SSRAM device. The new Intel PMC
SSRAM driver provides the following functionalities:
1. Search and store the PMC information in a structure, including PWRMBASE
address and devid for each available PMC. Then Intel PMC Core driver
achieves the PMC information using the API provided by the new driver.
2. Search and register Intel Platform Monitoring Techology telemetry
regions so they would by available for read through sysfs and Intel PMT
API. Intel PMC Core driver can achieve Low Power Mode requirement
information from a telemetry region registered by the new driver.
The above functionalities was previously handled by Intel PMC Core
driver. Intel PMC Core driver returns -EPROBE_DEFER when trying to read
data from a telem region that is not available yet. This setup may
result in an infinite loop of .probe() calls as Intel PMC Core driver
creates child devices. Creating a separate PCI driver avoids the infinite
loop possibility.
Xi Pardee (7):
platform/x86:intel/pmc: Remove SSRAM support from PMC Core
platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver
platform/x86:intel/pmc: Add support to get PMC information from SSRAM
platform/x86:intel/pmt: Get PMC from SSRAM for available platforms
platform/x86:intel/pmt: Create inline version for telemetry functions
platform/x86:intel/pmc: Add support to Retrieve LPM information
platform/x86:intel/pmc: Get LPM information for available platforms
Hi,
I don't see why the removal first, then re-add approach would be justified
here. You're basically adding the same code back later in many cases with
only very minimal changes, and some changes are entirely pointless such as
pmc_idx -> pmc_index parameter rename. This is just a big pain to review.
I'd suggest you move functions in first patch into core.c. Try to
avoid logic/code changes other than making making the necessary functions
non-static and adding the prototypes for them into a header (temporarily).
Then rename the ssram file to its new name in the second change.
Then do the rework on top of that (and make things back static again).
Try to split the rework into sensible chunks, anything that can be taken
away from the main rework change is less lines to review in that patch.
If you e.g. want to do pcidev -> pdev renames, put them into own separate
change (and do it consistently then, not just for some of the cases like
currently :-/).
The move patches are nearly trivial to review and take large chunk of
diff away from the actual rework itself which doesn't seem that
complicated to review once the 1:1 move bits and trivial rename churn is
eliminated from the diff.
Hi,
Thanks for reviewing the patches. I will rearrange the code in next version.
Xi
drivers/platform/x86/intel/pmc/Kconfig | 13 +-
drivers/platform/x86/intel/pmc/Makefile | 8 +-
drivers/platform/x86/intel/pmc/arl.c | 36 +-
drivers/platform/x86/intel/pmc/core.c | 216 +++++++++++-
drivers/platform/x86/intel/pmc/core.h | 25 +-
drivers/platform/x86/intel/pmc/core_ssram.c | 326 ------------------
drivers/platform/x86/intel/pmc/lnl.c | 36 +-
drivers/platform/x86/intel/pmc/mtl.c | 34 +-
.../platform/x86/intel/pmc/ssram_telemetry.c | 184 ++++++++++
.../platform/x86/intel/pmc/ssram_telemetry.h | 45 +++
drivers/platform/x86/intel/pmt/telemetry.h | 19 +-
11 files changed, 550 insertions(+), 392 deletions(-)
delete mode 100644 drivers/platform/x86/intel/pmc/core_ssram.c
create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.c
create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h
