| Message ID | fa72dfad9282e2c24b99327d08cbe032d7034bbf.1627710766.git.gayatri.kammela@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Alder Lake PCH-S support to PMC core driver | expand |
On Sat, Jul 31, 2021 at 9:10 AM Gayatri Kammela <gayatri.kammela@intel.com> wrote: > > As part of collecting Intel x86 specific drivers in their own > folder, move intel_pmc_core* files to its own subfolder there. ... > .../pmc/pltdrv.c} | 0 I would go further and spell it as platform.c. ... > -F: drivers/platform/x86/intel_pmc_core* > +F: drivers/platform/x86/intel/pmc/core* This seems incorrect. ... > + Supported features: > + - SLP_S0_RESIDENCY counter > + - PCH IP Power Gating status > + - LTR Ignore / LTR Show > + - MPHY/PLL gating status (Sunrisepoint PCH only) > + - SLPS0 Debug registers (Cannonlake/Icelake PCH) > + - Low Power Mode registers (Tigerlake and beyond) Perhaps you may use the opportunity to spell codenames in a better way, i.e. Sunrise Point Cannon Lake Ice Lake Tiger Lake as it's done almost everywhere else in the kernel. ... > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o > +intel_pmc_core-objs += core.o objs suffix is not for kernel modules. Moreover, := has a difference to +=. Why is the latter in use? > +obj-$(CONFIG_INTEL_PMC_CORE) += pltdrv.o This will have the very same issue as with the core module. On top of that, do you need a separate module for it? If so, why?
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Saturday, July 31, 2021 1:42 AM > To: Kammela, Gayatri <gayatri.kammela@intel.com> > Cc: Platform Driver <platform-driver-x86@vger.kernel.org>; Mark Gross > <mgross@linux.intel.com>; Hans de Goede <hdegoede@redhat.com>; > Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; You-Sheng Yang > <vicamo.yang@canonical.com>; Pandruvada, Srinivas > <srinivas.pandruvada@intel.com>; Box, David E <david.e.box@intel.com>; > Qin, Chao <chao.qin@intel.com>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; Mashiah, Tamar <tamar.mashiah@intel.com>; > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rajat Jain > <rajatja@google.com>; Shyam Sundar S K <Shyam-sundar.S-k@amd.com>; > Alex Deucher <Alexander.Deucher@amd.com>; mlimonci@amd.com > Subject: Re: [PATCH v6 1/5] platform/x86/intel: intel_pmc_core: Move > intel_pmc_core* files to pmc subfolder > > On Sat, Jul 31, 2021 at 9:10 AM Gayatri Kammela > <gayatri.kammela@intel.com> wrote: > > > > As part of collecting Intel x86 specific drivers in their own folder, > > move intel_pmc_core* files to its own subfolder there. > > ... > > > .../pmc/pltdrv.c} | 0 > > I would go further and spell it as platform.c. Hi Andy, sure! I have renamed the file as core_platform.c in the next version , so that it is consistent with the rest of the file names under intel/pmc/ (core.c, core.h , core_platform.c) > > ... > > > -F: drivers/platform/x86/intel_pmc_core* > > +F: drivers/platform/x86/intel/pmc/core* > > This seems incorrect. Yeah I agree. With the above change in the filename (core_platform.c), I think, this line in MAINTAINERS would make sense. > > ... > > > + Supported features: > > + - SLP_S0_RESIDENCY counter > > + - PCH IP Power Gating status > > + - LTR Ignore / LTR Show > > > + - MPHY/PLL gating status (Sunrisepoint PCH only) > > + - SLPS0 Debug registers (Cannonlake/Icelake PCH) > > + - Low Power Mode registers (Tigerlake and beyond) > > Perhaps you may use the opportunity to spell codenames in a better way, i.e. > Sunrise Point > Cannon Lake > Ice Lake > Tiger Lake > > as it's done almost everywhere else in the kernel. Sure! I have made changes as per your suggestion. Thanks! > > ... > > > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o > > +intel_pmc_core-objs += core.o > > objs suffix is not for kernel modules. > Moreover, := has a difference to +=. Why is the latter in use? Yeah, it is not needed for kernel modules. I have made changes accordingly. > > > +obj-$(CONFIG_INTEL_PMC_CORE) += pltdrv.o > > This will have the very same issue as with the core module. On top of that, > do you need a separate module for it? If so, why? I made changes, so that we won't have the same issue as with the core module. core_pltdrv has always been made as a separate module, Andy. Do you suggest that we don't need it as a separate module? > > -- > With Best Regards, > Andy Shevchenko
diff --git a/MAINTAINERS b/MAINTAINERS index c9467d2839f5..0dcf765682fc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9477,7 +9477,7 @@ M: David E Box <david.e.box@intel.com> L: platform-driver-x86@vger.kernel.org S: Maintained F: Documentation/ABI/testing/sysfs-platform-intel-pmc -F: drivers/platform/x86/intel_pmc_core* +F: drivers/platform/x86/intel/pmc/core* INTEL PMIC GPIO DRIVERS M: Andy Shevchenko <andy@kernel.org> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 7d385c3b2239..cae72922f448 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1184,27 +1184,6 @@ config INTEL_MRFLD_PWRBTN To compile this driver as a module, choose M here: the module will be called intel_mrfld_pwrbtn. -config INTEL_PMC_CORE - tristate "Intel PMC Core driver" - depends on PCI - depends on ACPI - help - The Intel Platform Controller Hub for Intel Core SoCs provides access - to Power Management Controller registers via various interfaces. This - driver can utilize debugging capabilities and supported features as - exposed by the Power Management Controller. It also may perform some - tasks in the PMC in order to enable transition into the SLPS0 state. - It should be selected on all Intel platforms supported by the driver. - - Supported features: - - SLP_S0_RESIDENCY counter - - PCH IP Power Gating status - - LTR Ignore / LTR Show - - MPHY/PLL gating status (Sunrisepoint PCH only) - - SLPS0 Debug registers (Cannonlake/Icelake PCH) - - Low Power Mode registers (Tigerlake and beyond) - - PMC quirks as needed to enable SLPS0/S0ix - config INTEL_PMT_CLASS tristate help diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 7ee369aab10d..43d36f8c36f1 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -128,7 +128,6 @@ obj-$(CONFIG_INTEL_UNCORE_FREQ_CONTROL) += intel-uncore-frequency.o obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN) += intel_chtdc_ti_pwrbtn.o obj-$(CONFIG_INTEL_MRFLD_PWRBTN) += intel_mrfld_pwrbtn.o -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_pltdrv.o obj-$(CONFIG_INTEL_PMT_CLASS) += intel_pmt_class.o obj-$(CONFIG_INTEL_PMT_TELEMETRY) += intel_pmt_telemetry.o obj-$(CONFIG_INTEL_PMT_CRASHLOG) += intel_pmt_crashlog.o diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig index f2eef337eb98..8ca021785f67 100644 --- a/drivers/platform/x86/intel/Kconfig +++ b/drivers/platform/x86/intel/Kconfig @@ -18,5 +18,6 @@ if X86_PLATFORM_DRIVERS_INTEL source "drivers/platform/x86/intel/int33fe/Kconfig" source "drivers/platform/x86/intel/int3472/Kconfig" +source "drivers/platform/x86/intel/pmc/Kconfig" endif # X86_PLATFORM_DRIVERS_INTEL diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile index 0653055942d5..49962f4dfdec 100644 --- a/drivers/platform/x86/intel/Makefile +++ b/drivers/platform/x86/intel/Makefile @@ -6,3 +6,4 @@ obj-$(CONFIG_INTEL_CHT_INT33FE) += int33fe/ obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/ +obj-$(CONFIG_INTEL_PMC_CORE) += pmc/ diff --git a/drivers/platform/x86/intel/pmc/Kconfig b/drivers/platform/x86/intel/pmc/Kconfig new file mode 100644 index 000000000000..b4c955a35674 --- /dev/null +++ b/drivers/platform/x86/intel/pmc/Kconfig @@ -0,0 +1,22 @@ +# SPDX-License-Identifier: GPL-2.0-only + +config INTEL_PMC_CORE + tristate "Intel PMC Core driver" + depends on PCI + depends on ACPI + help + The Intel Platform Controller Hub for Intel Core SoCs provides access + to Power Management Controller registers via various interfaces. This + driver can utilize debugging capabilities and supported features as + exposed by the Power Management Controller. It also may perform some + tasks in the PMC in order to enable transition into the SLPS0 state. + It should be selected on all Intel platforms supported by the driver. + + Supported features: + - SLP_S0_RESIDENCY counter + - PCH IP Power Gating status + - LTR Ignore / LTR Show + - MPHY/PLL gating status (Sunrisepoint PCH only) + - SLPS0 Debug registers (Cannonlake/Icelake PCH) + - Low Power Mode registers (Tigerlake and beyond) + - PMC quirks as needed to enable SLPS0/S0ix diff --git a/drivers/platform/x86/intel/pmc/Makefile b/drivers/platform/x86/intel/pmc/Makefile new file mode 100644 index 000000000000..d819955543fe --- /dev/null +++ b/drivers/platform/x86/intel/pmc/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0 +# + +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o +intel_pmc_core-objs += core.o +obj-$(CONFIG_INTEL_PMC_CORE) += pltdrv.o diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel/pmc/core.c similarity index 99% rename from drivers/platform/x86/intel_pmc_core.c rename to drivers/platform/x86/intel/pmc/core.c index b0e486a6bdfb..f9de78b08e5d 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel/pmc/core.c @@ -31,7 +31,7 @@ #include <asm/msr.h> #include <asm/tsc.h> -#include "intel_pmc_core.h" +#include "core.h" #define ACPI_S0IX_DSM_UUID "57a6512e-3979-4e9d-9708-ff13b2508972" #define ACPI_GET_LOW_MODE_REGISTERS 1 diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel/pmc/core.h similarity index 100% rename from drivers/platform/x86/intel_pmc_core.h rename to drivers/platform/x86/intel/pmc/core.h diff --git a/drivers/platform/x86/intel_pmc_core_pltdrv.c b/drivers/platform/x86/intel/pmc/pltdrv.c similarity index 100% rename from drivers/platform/x86/intel_pmc_core_pltdrv.c rename to drivers/platform/x86/intel/pmc/pltdrv.c