| Message ID | 20240809204648.1124545-1-xi.pardee@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Create Intel PMC SSRAM Telemetry driver | expand |
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. > 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 > >
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 >> >>