| Message ID | 1463061001-23210-1-git-send-email-rajneesh.bhardwaj@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thu, May 12, 2016 at 4:50 PM, Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> wrote: Sorry for a bit late review, but I think there are still issues need to be addressed. > This patch adds the Power Management Controller driver as a pci driver > for Intel Core SOC architecture. SOC -> SoC > > This driver can utilize debugging capabilities and supported features > as exposed by the Power Management Controller. > > Please refer to the below specification for more details on PMC features. > http://www.intel.in/content/www/in/en/chipsets/100-series-chipset-datasheet-vol-2.html > > The current version of this driver exposes slp_s0_residency counter. > This counter can be used for detecting fragile SLP_S0 signal related > failures and take corrective actions when PCH SLP_S0 signal is not > asserted after kernel freeze as part of suspend to idle flow > (echo freeze > /sys/power/state). > > Intel Platform Controller Hub (PCH) asserts SLP_S0 signal when it > detects favorable conditions to enter its low power mode. As a > pre-requisite the SOC should be in deepest possible Package C-State > and devices should be in low power mode. For example, on Skylake SOC Ditto. Check entire code for this. > the deepest Package C-State is Package C10 or PC10. Suspend to idle > flow generally leads to PC10 state but PC10 state may not be sufficient > for realizing the platform wide power potential which SLP_S0 signal > assertion can provide. > > SLP_S0 signal is often connected to the Embedded Controller (EC) and the > Power Management IC (PMIC) for other platform power management related > optimizations. > > In general, SLP_S0 assertion == PC10 + PCH low power mode + ModPhy Lanes > power gated + PLL Idle. > > As part of this driver, a mechanism to read the slp_s0 residency is exposed > as an api and also debugfs features are added to indicate slp_s0 signal api -> API > assertion residency in microseconds. > > echo freeze > /sys/power/state > wake the system > cat /sys/kernel/debug/pmc_core/slp_s0_residency_usec > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> > Signed-off-by: Vishwanath Somayaji <vishwanath.somayaji@intel.com> Two people (1). > +INTEL PMC CORE DRIVER > +M: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> > +M: Vishwanath Somayaji <vishwanath.somayaji@intel.com> > +L: platform-driver-x86@vger.kernel.org > +S: Maintained > +F: drivers/platform/x86/intel_pmc_core.h > +F: drivers/platform/x86/intel_pmc_core.c So, we have arch/x86/platform/atom/pmc_atom.c This driver looks very similar (by what functional is), so, we have to become clear what our layout is. Either we go with drivers/platform/x86 or with arch/x86/platform. > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -821,6 +821,18 @@ config INTEL_IPS > functionality. If in doubt, say Y here; it will only load on > supported platforms. > > +config INTEL_PMC_CORE Better to locate this in alphabetical order. > + bool "Intel PMC Core driver" > + depends on X86 && PCI > + ---help--- > + The Intel Platform Controller Hub for Intel Core SoCs provides access > + to Power Management Controller registers via a pci interface. This > + driver can utilize debugging capabilities and supported features as > + exposed by the Power Management Controller. > + > + Supported features: > + - slp_s0_residency counter. > + > config INTEL_IMR > bool "Intel Isolated Memory Region support" > default n > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 448443c..9b11b40 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -69,3 +69,4 @@ obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ > intel_telemetry_pltdrv.o \ > intel_telemetry_debugfs.o > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > new file mode 100644 > index 0000000..0b5388e > --- /dev/null > +++ b/drivers/platform/x86/intel_pmc_core.c > @@ -0,0 +1,201 @@ > +/* > + * Intel Core SOC Power Management Controller Driver > + * > + * Copyright (c) 2016, Intel Corporation. > + * All Rights Reserved. > + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@intel.com) (2) In (1) you put two people, here is one. Please, explain who is the author and why SoB is not in align with Author(s). > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + */ > + > +#include <linux/init.h> > +#include <linux/pci.h> > +#include <linux/device.h> > +#include <linux/debugfs.h> > +#include <linux/seq_file.h> > +#include <linux/module.h> > +#include <linux/io.h> Alphabetical order. + empty line > +#include <asm/cpu_device_id.h> + empty line > +#include "intel_pmc_core.h" > + > +static struct pmc_dev pmc; > + > +static const struct pci_device_id pmc_pci_ids[] = { > + { PCI_VDEVICE(INTEL, SPT_PMC_PCI_DEVICE_ID), (kernel_ulong_t)NULL }, No need to have an explicit NULL > + { 0, }, > +}; > +MODULE_DEVICE_TABLE(pci, pmc_pci_ids); > + > +/** > + * intel_pmc_slp_s0_counter_read() - Read slp_s0 residency. > + * @data: Out param that contains current slp_s0 count. > + * > + * This API currently supports Intel Skylake SOC and Sunrise > + * point Platform Controller Hub. Future platform support Either Sunrisepoint or Sunrise Point. SOC -> SoC > + * should be added for platforms that support low power modes > + * beyond Package C10 state. > + * > + * SLP_S0_RESIDENCY counter counts in 100 us granularity per > + * step hence function populates the multiplied value in out > + * parameter @data + dot at the end of sentence. > + * > + * Return: an error code or 0 on success. > + */ > +int intel_pmc_slp_s0_counter_read(u64 *data) > +{ struct pmc_dev *pmcdev = &pmc; > + if (!pmc.has_slp_s0_res) 'pmc.' -> 'pmcdev->' > + return -EACCES; > + > + *data = readl(pmc.regmap + SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); Ditto. > + *data *= SPT_PMC_SLP_S0_RES_COUNTER_STEP; Use temporary variable. u64 value; value = readl(); value *= ; *data = value; This makes only one place where you assign returning value. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); Who is going to be a user of that? > + > +#if IS_ENABLED(CONFIG_DEBUG_FS) > +static int pmc_core_dev_state_show(struct seq_file *s, void *unused) > +{ Pointer to pmc, i.e. pmc_dev *, would be supplied in seq_file. Take it from there... > + u64 counter_val; u64 value; > + int err; > + > + err = intel_pmc_slp_s0_counter_read(&counter_val); ...thus split your function to internal, which takes pmc_dev * as a first parameter and use exported variant for users which takes global variable. int intel_pmc_slp_s0_counter_read(u64 *data) { struct pmc_dev *pmcdev = &pmc; return do_counter_read(pmcdev, data); } > + if (!err) > + seq_printf(s, "%lld\n", counter_val); Please, use positive check if (err) return err; > + > + return err; > +} > + > +static int pmc_core_dev_state_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, pmc_core_dev_state_show, inode->i_private); > +} > + > +static const struct file_operations pmc_core_dev_state_ops = { > + .open = pmc_core_dev_state_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > +}; > + > +static void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) > +{ > + debugfs_remove_recursive(pmc->dbgfs_dir); > +} No need to keep this under #ifdef. > + > +static int pmc_core_dbgfs_register(struct pmc_dev *pmc) > +{ > + struct dentry *dir, *file; > + int ret = 0; Redundant, see below. > + > + dir = debugfs_create_dir("pmc_core", NULL); > + if (!dir) > + return -ENOMEM; > + > + pmc->dbgfs_dir = dir; > + file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO, > + dir, pmc, &pmc_core_dev_state_ops); > + > + if (!file) { > + pmc_core_dbgfs_unregister(pmc); > + ret = -ENODEV; return -ENODEV; > + } > + return ret; return 0; > +} > +#else > +static inline int pmc_core_dbgfs_register(struct pmc_dev *pmc) > +{ > + return 0; /* nothing to register */ > +} > + > +static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) > +{ > + /* nothing to deregister */ > +} Redundant. > +#endif /* CONFIG_DEBUG_FS */ > + > +static const struct x86_cpu_id intel_pmc_core_ids[] = { > + { X86_VENDOR_INTEL, 6, 0x4e, X86_FEATURE_MWAIT, > + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ No explicit NULL. > + { X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT, > + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ Ditto. > + {} > +}; > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); > + > +static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id) > +{ > + int err; > + const struct x86_cpu_id *cpu_id; Swap them. + const struct x86_cpu_id *cpu_id; + int err; struct pmc_dev *pmcdev = &pmc; > + > + cpu_id = x86_match_cpu(intel_pmc_core_ids); > + if (!cpu_id) { > + err = -EINVAL; > + goto exit; > + } > + > + err = pci_enable_device(dev); pcim_enable_device(); > + if (err) { > + dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n"); I doubt this message is useful anyhow. > + goto exit; > + } > + > + err = pci_read_config_dword(dev, > + SPT_PMC_BASE_ADDR_OFFSET, > + &pmc.base_addr); 'pmc.' -> 'pmcdev->' > + if (err) { > + dev_err(&dev->dev, "PMC Core: failed to read pci config space.\n"); > + goto disable_pci; > + } So, and this is not available through BARs? > + dev_dbg(&dev->dev, "PMC Core: PWRMBASE is 0x%x\n", pmc.base_addr); %#x will prefix the number. > + > + pmc.regmap = ioremap_nocache(pmc.base_addr, SPT_PMC_MMIO_REG_LEN); > + if (!pmc.regmap) { > + dev_err(&dev->dev, "PMC Core: ioremap failed\n"); Useful? > + err = -ENOMEM; > + goto disable_pci; > + } > + > + err = pmc_core_dbgfs_register(&pmc); > + if (err) { > + dev_err(&dev->dev, "PMC Core: debugfs register failed\n"); > + iounmap(pmc.regmap); > + goto disable_pci; > + } > + > + pmc.has_slp_s0_res = true; > + return 0; > + > +disable_pci: > + pci_disable_device(dev); Remove after using pcim_*(). > +exit: Redundant. Use return directly. > + return err; > +} > + > +static void intel_pmc_core_remove(struct pci_dev *pdev) > +{ > + pmc_core_dbgfs_unregister(&pmc); > + pci_disable_device(pdev); > + iounmap(pmc.regmap); > +}
On Thu, May 12, 2016 at 06:02:45PM +0300, Andy Shevchenko wrote: > On Thu, May 12, 2016 at 4:50 PM, Rajneesh Bhardwaj > <rajneesh.bhardwaj@intel.com> wrote: > > Sorry for a bit late review, but I think there are still issues need > to be addressed. > Thanks for the detailed review and the feedback. :) > > This patch adds the Power Management Controller driver as a pci driver > > for Intel Core SOC architecture. > > SOC -> SoC > Sure, will fix this throughout the code. > > > > This driver can utilize debugging capabilities and supported features > > as exposed by the Power Management Controller. > > > > Please refer to the below specification for more details on PMC features. > > http://www.intel.in/content/www/in/en/chipsets/100-series-chipset-datasheet-vol-2.html > > > > The current version of this driver exposes slp_s0_residency counter. > > This counter can be used for detecting fragile SLP_S0 signal related > > failures and take corrective actions when PCH SLP_S0 signal is not > > asserted after kernel freeze as part of suspend to idle flow > > (echo freeze > /sys/power/state). > > > > Intel Platform Controller Hub (PCH) asserts SLP_S0 signal when it > > detects favorable conditions to enter its low power mode. As a > > pre-requisite the SOC should be in deepest possible Package C-State > > and devices should be in low power mode. For example, on Skylake SOC > > Ditto. > Check entire code for this. > Same as above. > > the deepest Package C-State is Package C10 or PC10. Suspend to idle > > flow generally leads to PC10 state but PC10 state may not be sufficient > > for realizing the platform wide power potential which SLP_S0 signal > > assertion can provide. > > > > SLP_S0 signal is often connected to the Embedded Controller (EC) and the > > Power Management IC (PMIC) for other platform power management related > > optimizations. > > > > In general, SLP_S0 assertion == PC10 + PCH low power mode + ModPhy Lanes > > power gated + PLL Idle. > > > > As part of this driver, a mechanism to read the slp_s0 residency is exposed > > as an api and also debugfs features are added to indicate slp_s0 signal > > api -> API > Sure, will take care of this change. > > assertion residency in microseconds. > > > > echo freeze > /sys/power/state > > wake the system > > cat /sys/kernel/debug/pmc_core/slp_s0_residency_usec > > > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> > > Signed-off-by: Vishwanath Somayaji <vishwanath.somayaji@intel.com> > > Two people (1). > > Ok. > > +INTEL PMC CORE DRIVER > > +M: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> > > +M: Vishwanath Somayaji <vishwanath.somayaji@intel.com> > > +L: platform-driver-x86@vger.kernel.org > > +S: Maintained > > +F: drivers/platform/x86/intel_pmc_core.h > > +F: drivers/platform/x86/intel_pmc_core.c > > So, we have arch/x86/platform/atom/pmc_atom.c > > This driver looks very similar (by what functional is), so, we have to > become clear what our layout is. > Either we go with drivers/platform/x86 or with arch/x86/platform. > IMHO, the functianality provided by this driver is platform specific and not architecture specific. There are few similar drivers present in this location already i.e. intel_telemetry_core.c, intel_pmc_ipc.c etc. We had initially put this driver along with pmc_atom.c but later we thought that driver/platform/x86 would be a more apt place for it because of the above mentioned reasons. Please advise for the right location for this driver if its not placed in the expected location. > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -821,6 +821,18 @@ config INTEL_IPS > > functionality. If in doubt, say Y here; it will only load on > > supported platforms. > > > > +config INTEL_PMC_CORE > > Better to locate this in alphabetical order. > Agreed, i tried to put it in alphabetical order but there are quite a few entries in Kconfig that are skewed e.g. INTEL_MFLD is placed above INTEL_IPS. Placing INTEL_PMC_CORE after INTEL_IMR would be ok? > > + bool "Intel PMC Core driver" > > + depends on X86 && PCI > > > + ---help--- > > + The Intel Platform Controller Hub for Intel Core SoCs provides access > > + to Power Management Controller registers via a pci interface. This > > + driver can utilize debugging capabilities and supported features as > > + exposed by the Power Management Controller. > > + > > + Supported features: > > + - slp_s0_residency counter. > > + > > config INTEL_IMR > > bool "Intel Isolated Memory Region support" > > default n > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > index 448443c..9b11b40 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -69,3 +69,4 @@ obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o > > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ > > intel_telemetry_pltdrv.o \ > > intel_telemetry_debugfs.o > > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o > > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > > new file mode 100644 > > index 0000000..0b5388e > > --- /dev/null > > +++ b/drivers/platform/x86/intel_pmc_core.c > > @@ -0,0 +1,201 @@ > > +/* > > + * Intel Core SOC Power Management Controller Driver > > + * > > + * Copyright (c) 2016, Intel Corporation. > > + * All Rights Reserved. > > > + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@intel.com) > > (2) > > In (1) you put two people, here is one. Please, explain who is the > author and why SoB is not in align with Author(s). > Its a miss from our end, will update the Author tag. Thanks for pointing it out. > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms and conditions of the GNU General Public License, > > + * version 2, as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope it will be useful, but WITHOUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > > + * more details. > > + * > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/pci.h> > > +#include <linux/device.h> > > +#include <linux/debugfs.h> > > +#include <linux/seq_file.h> > > +#include <linux/module.h> > > +#include <linux/io.h> > > Alphabetical order. > > + empty line > > > +#include <asm/cpu_device_id.h> > > + empty line > > > +#include "intel_pmc_core.h" > > + > Ok for above. > > +static struct pmc_dev pmc; > > + > > +static const struct pci_device_id pmc_pci_ids[] = { > > + { PCI_VDEVICE(INTEL, SPT_PMC_PCI_DEVICE_ID), (kernel_ulong_t)NULL }, > > No need to have an explicit NULL > There is a precedent in the kernel for such usage and in previous reviews we agreed to stick to this format. I hope thats fine? > > + { 0, }, > > +}; > > +MODULE_DEVICE_TABLE(pci, pmc_pci_ids); > > + > > +/** > > + * intel_pmc_slp_s0_counter_read() - Read slp_s0 residency. > > + * @data: Out param that contains current slp_s0 count. > > + * > > + * This API currently supports Intel Skylake SOC and Sunrise > > + * point Platform Controller Hub. Future platform support > > Either Sunrisepoint or Sunrise Point. > SOC -> SoC > Sure. > > + * should be added for platforms that support low power modes > > + * beyond Package C10 state. > > + * > > + * SLP_S0_RESIDENCY counter counts in 100 us granularity per > > + * step hence function populates the multiplied value in out > > + * parameter @data > > + dot at the end of sentence. > Ok. > > + * > > + * Return: an error code or 0 on success. > > + */ > > +int intel_pmc_slp_s0_counter_read(u64 *data) > > +{ > > struct pmc_dev *pmcdev = &pmc; > > > > + if (!pmc.has_slp_s0_res) > > 'pmc.' -> 'pmcdev->' > > > + return -EACCES; > > + > > + *data = readl(pmc.regmap + SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > Ditto. > > > + *data *= SPT_PMC_SLP_S0_RES_COUNTER_STEP; > > Use temporary variable. > > u64 value; > > value = readl(); > value *= ; > > *data = value; > > This makes only one place where you assign returning value. > Ok, will rework on the function and split it into two. One for performing read operation and callable from within the module and the other one to be called from outside. > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); > > Who is going to be a user of that? > This is expected to be used by a framework (upcoming) for detecting slp_s0 signal assertion related failures. > > + > > > +#if IS_ENABLED(CONFIG_DEBUG_FS) > > > > +static int pmc_core_dev_state_show(struct seq_file *s, void *unused) > > +{ > > Pointer to pmc, i.e. pmc_dev *, would be supplied in seq_file. Take it > from there... > > > + u64 counter_val; > > u64 value; > > > + int err; > > + > > + err = intel_pmc_slp_s0_counter_read(&counter_val); > > ...thus split your function to internal, which takes pmc_dev * as a > first parameter and use exported variant for users which takes global > variable. > > int intel_pmc_slp_s0_counter_read(u64 *data) > { > struct pmc_dev *pmcdev = &pmc; > > return do_counter_read(pmcdev, data); > } > > As explained above, will rework on the function split logic and accomodate these suggestions. > > + if (!err) > > + seq_printf(s, "%lld\n", counter_val); > > Please, use positive check > > if (err) > return err; > Ok, any benefit? readability? > > + > > + return err; > > +} > > + > > +static int pmc_core_dev_state_open(struct inode *inode, struct file *file) > > +{ > > + return single_open(file, pmc_core_dev_state_show, inode->i_private); > > +} > > + > > +static const struct file_operations pmc_core_dev_state_ops = { > > + .open = pmc_core_dev_state_open, > > + .read = seq_read, > > + .llseek = seq_lseek, > > + .release = single_release, > > +}; > > + > > > +static void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) > > +{ > > + debugfs_remove_recursive(pmc->dbgfs_dir); > > +} > > No need to keep this under #ifdef. > Based on some previus review comments we decided to put the dummy functions for !CONIG_DEBUG_FS case. Can you please elaborate more on this change request? > > + > > +static int pmc_core_dbgfs_register(struct pmc_dev *pmc) > > +{ > > + struct dentry *dir, *file; > > > + int ret = 0; > > Redundant, see below. > > > + > > + dir = debugfs_create_dir("pmc_core", NULL); > > + if (!dir) > > + return -ENOMEM; > > + > > + pmc->dbgfs_dir = dir; > > + file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO, > > + dir, pmc, &pmc_core_dev_state_ops); > > + > > + if (!file) { > > + pmc_core_dbgfs_unregister(pmc); > > + ret = -ENODEV; > > return -ENODEV; > Ok. > > + } > > > + return ret; > > return 0; > > > +} > > +#else > > +static inline int pmc_core_dbgfs_register(struct pmc_dev *pmc) > > +{ > > + return 0; /* nothing to register */ > > +} > > + > > > +static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) > > +{ > > + /* nothing to deregister */ > > +} > > Redundant. > Same as above. > > +#endif /* CONFIG_DEBUG_FS */ > > + > > +static const struct x86_cpu_id intel_pmc_core_ids[] = { > > + { X86_VENDOR_INTEL, 6, 0x4e, X86_FEATURE_MWAIT, > > + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ > > No explicit NULL. > Same as explained above. > > + { X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT, > > + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ > > Ditto. > Same as explained above. > > + {} > > +}; > > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); > > + > > +static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id) > > +{ > > > + int err; > > + const struct x86_cpu_id *cpu_id; > > Swap them. > > + const struct x86_cpu_id *cpu_id; > + int err; > > struct pmc_dev *pmcdev = &pmc; > Ok. > > + > > + cpu_id = x86_match_cpu(intel_pmc_core_ids); > > + if (!cpu_id) { > > + err = -EINVAL; > > + goto exit; > > + } > > + > > + err = pci_enable_device(dev); > > pcim_enable_device(); > Thanks for this suggestion. > > + if (err) { > > + dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n"); > > I doubt this message is useful anyhow. > > > + goto exit; > > + } > > + > > + err = pci_read_config_dword(dev, > > + SPT_PMC_BASE_ADDR_OFFSET, > > + &pmc.base_addr); > > 'pmc.' -> 'pmcdev->' > > > + if (err) { > > + dev_err(&dev->dev, "PMC Core: failed to read pci config space.\n"); > > + goto disable_pci; > > + } > > So, and this is not available through BARs? > > > + dev_dbg(&dev->dev, "PMC Core: PWRMBASE is 0x%x\n", pmc.base_addr); > > %#x will prefix the number. > > > + Ok. > > + pmc.regmap = ioremap_nocache(pmc.base_addr, SPT_PMC_MMIO_REG_LEN); > > + if (!pmc.regmap) { > > > + dev_err(&dev->dev, "PMC Core: ioremap failed\n"); > > Useful? > Want to keep it for debug purpose. Do you recommend devm_ioremap_nocache as well? > > + err = -ENOMEM; > > + goto disable_pci; > > + } > > + > > + err = pmc_core_dbgfs_register(&pmc); > > + if (err) { > > + dev_err(&dev->dev, "PMC Core: debugfs register failed\n"); > > + iounmap(pmc.regmap); > > + goto disable_pci;Do you recommend devm_ioremap_nocache as well? > > + } > > + > > + pmc.has_slp_s0_res = true; > > > > + return 0; > > + > > > +disable_pci: > > + pci_disable_device(dev); > > Remove after using pcim_*(). > Ok. > > +exit: > > Redundant. Use return directly. > Ok. > > + return err; > > +} > > + > > +static void intel_pmc_core_remove(struct pci_dev *pdev) > > +{ > > + pmc_core_dbgfs_unregister(&pmc); > > + pci_disable_device(pdev); > > + iounmap(pmc.regmap); > > +} > > > -- > With Best Regards, > Andy Shevchenko
On Mon, May 16, 2016 at 03:45:50PM +0530, Rajneesh Bhardwaj wrote: > On Thu, May 12, 2016 at 06:02:45PM +0300, Andy Shevchenko wrote: > > On Thu, May 12, 2016 at 4:50 PM, Rajneesh Bhardwaj > > <rajneesh.bhardwaj@intel.com> wrote: > > > > Sorry for a bit late review, but I think there are still issues need > > to be addressed. > > > > Thanks for the detailed review and the feedback. :) > > > > This patch adds the Power Management Controller driver as a pci driver > > > for Intel Core SOC architecture. > > > > SOC -> SoC > > > > Sure, will fix this throughout the code. > > > > > > > This driver can utilize debugging capabilities and supported features > > > as exposed by the Power Management Controller. > > > > > > Please refer to the below specification for more details on PMC features. > > > http://www.intel.in/content/www/in/en/chipsets/100-series-chipset-datasheet-vol-2.html > > > > > > The current version of this driver exposes slp_s0_residency counter. > > > This counter can be used for detecting fragile SLP_S0 signal related > > > failures and take corrective actions when PCH SLP_S0 signal is not > > > asserted after kernel freeze as part of suspend to idle flow > > > (echo freeze > /sys/power/state). > > > > > > Intel Platform Controller Hub (PCH) asserts SLP_S0 signal when it > > > detects favorable conditions to enter its low power mode. As a > > > pre-requisite the SOC should be in deepest possible Package C-State > > > and devices should be in low power mode. For example, on Skylake SOC > > > > Ditto. > > Check entire code for this. > > > > Same as above. > > > > the deepest Package C-State is Package C10 or PC10. Suspend to idle > > > flow generally leads to PC10 state but PC10 state may not be sufficient > > > for realizing the platform wide power potential which SLP_S0 signal > > > assertion can provide. > > > > > > SLP_S0 signal is often connected to the Embedded Controller (EC) and the > > > Power Management IC (PMIC) for other platform power management related > > > optimizations. > > > > > > In general, SLP_S0 assertion == PC10 + PCH low power mode + ModPhy Lanes > > > power gated + PLL Idle. > > > > > > As part of this driver, a mechanism to read the slp_s0 residency is exposed > > > as an api and also debugfs features are added to indicate slp_s0 signal > > > > api -> API > > > > Sure, will take care of this change. > > > > assertion residency in microseconds. > > > > > > echo freeze > /sys/power/state > > > wake the system > > > cat /sys/kernel/debug/pmc_core/slp_s0_residency_usec > > > > > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> > > > Signed-off-by: Vishwanath Somayaji <vishwanath.somayaji@intel.com> > > > > Two people (1). > > > > > > Ok. > > > > +INTEL PMC CORE DRIVER > > > +M: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> > > > +M: Vishwanath Somayaji <vishwanath.somayaji@intel.com> > > > +L: platform-driver-x86@vger.kernel.org > > > +S: Maintained > > > +F: drivers/platform/x86/intel_pmc_core.h > > > +F: drivers/platform/x86/intel_pmc_core.c > > > > So, we have arch/x86/platform/atom/pmc_atom.c > > > > This driver looks very similar (by what functional is), so, we have to > > become clear what our layout is. > > Either we go with drivers/platform/x86 or with arch/x86/platform. > > > > IMHO, the functianality provided by this driver is platform specific and > not architecture specific. > > There are few similar drivers present in this location already i.e. > intel_telemetry_core.c, intel_pmc_ipc.c etc. > > We had initially put this driver along with pmc_atom.c but later we thought > that driver/platform/x86 would be a more apt place for it because of the > above mentioned reasons. > > Please advise for the right location for this driver if its not placed in the > expected location. We're experiencing some repeat discussion due to some of the reviews having taken place off list. Sorry Andy, I've been trying to steer this back to list, so you're missing some context to no fault of your own. It's possible some of the existing drivers in arch really shouldn't be there. It's not particularly well defined, however, if it isn't used outside the kernel or by other subsystems within the kernel, it seems that drivers/platform/x86 would be the appropriate location. Thomas, Peter - do you have a criteria for what you prefer to have in arch/x86/platform versus drivers/platform/x86? > > > > --- a/drivers/platform/x86/Kconfig > > > +++ b/drivers/platform/x86/Kconfig > > > @@ -821,6 +821,18 @@ config INTEL_IPS > > > functionality. If in doubt, say Y here; it will only load on > > > supported platforms. > > > > > > +config INTEL_PMC_CORE > > > > Better to locate this in alphabetical order. > > > > Agreed, i tried to put it in alphabetical order but there are quite a few entries > in Kconfig that are skewed e.g. INTEL_MFLD is placed above INTEL_IPS. > > Placing INTEL_PMC_CORE after INTEL_IMR would be ok? > > > > + bool "Intel PMC Core driver" > > > + depends on X86 && PCI > > > > > + ---help--- > > > + The Intel Platform Controller Hub for Intel Core SoCs provides access > > > + to Power Management Controller registers via a pci interface. This > > > + driver can utilize debugging capabilities and supported features as > > > + exposed by the Power Management Controller. > > > + > > > + Supported features: > > > + - slp_s0_residency counter. > > > + > > > config INTEL_IMR > > > bool "Intel Isolated Memory Region support" > > > default n > > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > > index 448443c..9b11b40 100644 > > > --- a/drivers/platform/x86/Makefile > > > +++ b/drivers/platform/x86/Makefile > > > @@ -69,3 +69,4 @@ obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o > > > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ > > > intel_telemetry_pltdrv.o \ > > > intel_telemetry_debugfs.o > > > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o > > > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > > > new file mode 100644 > > > index 0000000..0b5388e > > > --- /dev/null > > > +++ b/drivers/platform/x86/intel_pmc_core.c > > > @@ -0,0 +1,201 @@ > > > +/* > > > + * Intel Core SOC Power Management Controller Driver > > > + * > > > + * Copyright (c) 2016, Intel Corporation. > > > + * All Rights Reserved. > > > > > + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@intel.com) > > > > (2) > > > > In (1) you put two people, here is one. Please, explain who is the > > author and why SoB is not in align with Author(s). > > > > Its a miss from our end, will update the Author tag. Thanks for pointing it out. > > > > + * > > > + * This program is free software; you can redistribute it and/or modify it > > > + * under the terms and conditions of the GNU General Public License, > > > + * version 2, as published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope it will be useful, but WITHOUT > > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > > > + * more details. > > > + * > > > + */ > > > + > > > +#include <linux/init.h> > > > +#include <linux/pci.h> > > > +#include <linux/device.h> > > > +#include <linux/debugfs.h> > > > +#include <linux/seq_file.h> > > > +#include <linux/module.h> > > > +#include <linux/io.h> > > > > Alphabetical order. > > > > + empty line > > > > > +#include <asm/cpu_device_id.h> > > > > + empty line > > > > > +#include "intel_pmc_core.h" > > > + > > > > Ok for above. > > > > +static struct pmc_dev pmc; > > > + > > > +static const struct pci_device_id pmc_pci_ids[] = { > > > + { PCI_VDEVICE(INTEL, SPT_PMC_PCI_DEVICE_ID), (kernel_ulong_t)NULL }, > > > > No need to have an explicit NULL > > > > There is a precedent in the kernel for such usage and in previous reviews we agreed > to stick to this format. I hope thats fine? I'm fine with this, even if redundant, so long as you are consistent throughout the driver. This is consistent with the other drivers in drivers/platform/x86. > > > > + { 0, }, > > > +}; > > > +MODULE_DEVICE_TABLE(pci, pmc_pci_ids); > > > + > > > +/** > > > + * intel_pmc_slp_s0_counter_read() - Read slp_s0 residency. > > > + * @data: Out param that contains current slp_s0 count. > > > + * > > > + * This API currently supports Intel Skylake SOC and Sunrise > > > + * point Platform Controller Hub. Future platform support > > > > Either Sunrisepoint or Sunrise Point. > > SOC -> SoC > > > > Sure. > > > > + * should be added for platforms that support low power modes > > > + * beyond Package C10 state. > > > + * > > > + * SLP_S0_RESIDENCY counter counts in 100 us granularity per > > > + * step hence function populates the multiplied value in out > > > + * parameter @data > > > > + dot at the end of sentence. > > > > Ok. > > > > + * > > > + * Return: an error code or 0 on success. > > > + */ > > > +int intel_pmc_slp_s0_counter_read(u64 *data) > > > +{ > > > > struct pmc_dev *pmcdev = &pmc; > > > > > > > + if (!pmc.has_slp_s0_res) > > > > 'pmc.' -> 'pmcdev->' > > > > > + return -EACCES; > > > + > > > + *data = readl(pmc.regmap + SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > > > Ditto. > > > > > + *data *= SPT_PMC_SLP_S0_RES_COUNTER_STEP; > > > > Use temporary variable. > > > > u64 value; > > > > value = readl(); > > value *= ; > > > > *data = value; > > > > This makes only one place where you assign returning value. > > > > Ok, will rework on the function and split it into two. One for performing > read operation and callable from within the module and the other one to be > called from outside. > > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); > > > > Who is going to be a user of that? > > > > This is expected to be used by a framework (upcoming) for detecting slp_s0 > signal assertion related failures. > > > > + > > > > > +#if IS_ENABLED(CONFIG_DEBUG_FS) > > > > > > > +static int pmc_core_dev_state_show(struct seq_file *s, void *unused) > > > +{ > > > > Pointer to pmc, i.e. pmc_dev *, would be supplied in seq_file. Take it > > from there... > > > > > + u64 counter_val; > > > > u64 value; > > > > > + int err; > > > + > > > + err = intel_pmc_slp_s0_counter_read(&counter_val); > > > > ...thus split your function to internal, which takes pmc_dev * as a > > first parameter and use exported variant for users which takes global > > variable. > > > > int intel_pmc_slp_s0_counter_read(u64 *data) > > { > > struct pmc_dev *pmcdev = &pmc; > > > > return do_counter_read(pmcdev, data); > > } > > > > > > As explained above, will rework on the function split logic and accomodate > these suggestions. > > > > + if (!err) > > > + seq_printf(s, "%lld\n", counter_val); > > > > Please, use positive check > > > > if (err) > > return err; > > > > Ok, any benefit? readability? Readability and consistency, yes. Check for errors rather than success is the more common model within the kernel in my experience. > > > > + > > > + return err; > > > +} > > > + > > > +static int pmc_core_dev_state_open(struct inode *inode, struct file *file) > > > +{ > > > + return single_open(file, pmc_core_dev_state_show, inode->i_private); > > > +} > > > + > > > +static const struct file_operations pmc_core_dev_state_ops = { > > > + .open = pmc_core_dev_state_open, > > > + .read = seq_read, > > > + .llseek = seq_lseek, > > > + .release = single_release, > > > +}; > > > + > > > > > +static void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) > > > +{ > > > + debugfs_remove_recursive(pmc->dbgfs_dir); > > > +} > > > > No need to keep this under #ifdef. > > > > Based on some previus review comments we decided to put the dummy functions > for !CONIG_DEBUG_FS case. Can you please elaborate more on this change request? > I think Andy's point is that there is no reason to specialize this function since debugs_remove_recursive checks for null. The problem of course is that dbgfs_dir only exists if CONFIG_DEBUG_FS exists. I don't know if that's worth ifdeffing out of the struct, but there is precendent for doing it that way, and I didn't feel strongly one way or the other, so I left it up to you. If you want to conditionally include the field in the struct, then this is fine as is. -- Darren > > > + > > > +static int pmc_core_dbgfs_register(struct pmc_dev *pmc) > > > +{ > > > + struct dentry *dir, *file; > > > > > + int ret = 0; > > > > Redundant, see below. > > > > > + > > > + dir = debugfs_create_dir("pmc_core", NULL); > > > + if (!dir) > > > + return -ENOMEM; > > > + > > > + pmc->dbgfs_dir = dir; > > > + file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO, > > > + dir, pmc, &pmc_core_dev_state_ops); > > > + > > > + if (!file) { > > > + pmc_core_dbgfs_unregister(pmc); > > > + ret = -ENODEV; > > > > return -ENODEV; > > > > Ok. > > > > + } > > > > > + return ret; > > > > return 0; > > > > > +} > > > +#else > > > +static inline int pmc_core_dbgfs_register(struct pmc_dev *pmc) > > > +{ > > > + return 0; /* nothing to register */ > > > +} > > > + > > > > > +static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) > > > +{ > > > + /* nothing to deregister */ > > > +} > > > > Redundant. > > > > Same as above. > > > > +#endif /* CONFIG_DEBUG_FS */ > > > + > > > +static const struct x86_cpu_id intel_pmc_core_ids[] = { > > > + { X86_VENDOR_INTEL, 6, 0x4e, X86_FEATURE_MWAIT, > > > + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ > > > > No explicit NULL. > > > > Same as explained above. > > > > + { X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT, > > > + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ > > > > Ditto. > > > > Same as explained above. > > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); > > > + > > > +static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id) > > > +{ > > > > > + int err; > > > + const struct x86_cpu_id *cpu_id; > > > > Swap them. > > > > + const struct x86_cpu_id *cpu_id; > > + int err; > > > > struct pmc_dev *pmcdev = &pmc; > > > > Ok. > > > > + > > > + cpu_id = x86_match_cpu(intel_pmc_core_ids); > > > + if (!cpu_id) { > > > + err = -EINVAL; > > > + goto exit; > > > + } > > > + > > > + err = pci_enable_device(dev); > > > > pcim_enable_device(); > > > > Thanks for this suggestion. > > > > + if (err) { > > > + dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n"); > > > > I doubt this message is useful anyhow. > > > > > + goto exit; > > > + } > > > + > > > + err = pci_read_config_dword(dev, > > > + SPT_PMC_BASE_ADDR_OFFSET, > > > + &pmc.base_addr); > > > > 'pmc.' -> 'pmcdev->' > > > > > + if (err) { > > > + dev_err(&dev->dev, "PMC Core: failed to read pci config space.\n"); > > > + goto disable_pci; > > > + } > > > > So, and this is not available through BARs? > > > > > + dev_dbg(&dev->dev, "PMC Core: PWRMBASE is 0x%x\n", pmc.base_addr); > > > > %#x will prefix the number. > > > > > + > > Ok. > > > > + pmc.regmap = ioremap_nocache(pmc.base_addr, SPT_PMC_MMIO_REG_LEN); > > > + if (!pmc.regmap) { > > > > > + dev_err(&dev->dev, "PMC Core: ioremap failed\n"); > > > > Useful? > > > > Want to keep it for debug purpose. If it's really only useful for debug, then use dev_dbg as you did above. > Do you recommend devm_ioremap_nocache as well? > > > > + err = -ENOMEM; > > > + goto disable_pci; > > > + } > > > + > > > + err = pmc_core_dbgfs_register(&pmc); > > > + if (err) { > > > + dev_err(&dev->dev, "PMC Core: debugfs register failed\n"); > > > + iounmap(pmc.regmap); > > > + goto disable_pci;Do you recommend devm_ioremap_nocache as well? > > > + } > > > + > > > + pmc.has_slp_s0_res = true; > > > > > > > + return 0; > > > + > > > > > +disable_pci: > > > + pci_disable_device(dev); > > > > Remove after using pcim_*(). > > > > Ok. > > > > +exit: > > > > Redundant. Use return directly. > > > > Ok. > > > > + return err; > > > +} > > > + > > > +static void intel_pmc_core_remove(struct pci_dev *pdev) > > > +{ > > > + pmc_core_dbgfs_unregister(&pmc); > > > + pci_disable_device(pdev); > > > + iounmap(pmc.regmap); > > > +} > > > > > > -- > > With Best Regards, > > Andy Shevchenko > > -- > Best Regards, > Rajneesh >
On Thu, May 12, 2016 at 6:50 AM, Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> wrote: > > + * pmc_dev contains info about power management controller device. > + */ > +struct pmc_dev { > + u32 base_addr; > + void __iomem *regmap; > +#if IS_ENABLED(CONFIG_DEBUG_FS) > + struct dentry *dbgfs_dir; > +#endif /* CONFIG_DEBUG_FS */ > + bool has_slp_s0_res; > +}; > + > +int intel_pmc_slp_s0_counter_read(u64 *data); Can this function be split out into another header file? The struct pmc_dev is internal, but this function is needed as an external interface. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 17, 2016 at 1:59 AM, Darren Hart <dvhart@infradead.org> wrote: > On Mon, May 16, 2016 at 03:45:50PM +0530, Rajneesh Bhardwaj wrote: >> On Thu, May 12, 2016 at 06:02:45PM +0300, Andy Shevchenko wrote: >> > On Thu, May 12, 2016 at 4:50 PM, Rajneesh Bhardwaj >> > <rajneesh.bhardwaj@intel.com> wrote: >> > > +INTEL PMC CORE DRIVER >> > > +M: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> >> > > +M: Vishwanath Somayaji <vishwanath.somayaji@intel.com> >> > > +L: platform-driver-x86@vger.kernel.org >> > > +S: Maintained >> > > +F: drivers/platform/x86/intel_pmc_core.h >> > > +F: drivers/platform/x86/intel_pmc_core.c >> > >> > So, we have arch/x86/platform/atom/pmc_atom.c >> > >> > This driver looks very similar (by what functional is), so, we have to >> > become clear what our layout is. >> > Either we go with drivers/platform/x86 or with arch/x86/platform. >> > >> >> IMHO, the functianality provided by this driver is platform specific and >> not architecture specific. >> >> There are few similar drivers present in this location already i.e. >> intel_telemetry_core.c, intel_pmc_ipc.c etc. >> >> We had initially put this driver along with pmc_atom.c but later we thought >> that driver/platform/x86 would be a more apt place for it because of the >> above mentioned reasons. >> >> Please advise for the right location for this driver if its not placed in the >> expected location. > > We're experiencing some repeat discussion due to some of the reviews having > taken place off list. Sorry Andy, I've been trying to steer this back to list, > so you're missing some context to no fault of your own. > > It's possible some of the existing drivers in arch really shouldn't be there. > It's not particularly well defined, however, if it isn't used outside the kernel > or by other subsystems within the kernel, it seems that drivers/platform/x86 > would be the appropriate location. Roughly it's what I also expected, but here we export function to the users. If they are all be located under pdx86 I'm pretty okay with current approach. Otherwise might be good to think a bit. > Thomas, Peter - do you have a criteria for what you prefer to have in > arch/x86/platform versus drivers/platform/x86? +1 to the question. It would be nice to have a formal criteria to choose a location. >> > > --- a/drivers/platform/x86/Kconfig >> > > +++ b/drivers/platform/x86/Kconfig >> > > @@ -821,6 +821,18 @@ config INTEL_IPS >> > > functionality. If in doubt, say Y here; it will only load on >> > > supported platforms. >> > > >> > > +config INTEL_PMC_CORE >> > >> > Better to locate this in alphabetical order. >> > >> >> Agreed, i tried to put it in alphabetical order but there are quite a few entries >> in Kconfig that are skewed e.g. INTEL_MFLD is placed above INTEL_IPS. >> >> Placing INTEL_PMC_CORE after INTEL_IMR would be ok? At least. At some point we might sort entire file alphabetically. >> > > +static struct pmc_dev pmc; >> > > + >> > > +static const struct pci_device_id pmc_pci_ids[] = { >> > > + { PCI_VDEVICE(INTEL, SPT_PMC_PCI_DEVICE_ID), (kernel_ulong_t)NULL }, >> > >> > No need to have an explicit NULL >> > >> >> There is a precedent in the kernel for such usage and in previous reviews we agreed >> to stick to this format. I hope thats fine? > > I'm fine with this, even if redundant, so long as you are consistent throughout > the driver. This is consistent with the other drivers in drivers/platform/x86. Yeah, it's minor, so just one format for all entries. >> > > +EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); >> > >> > Who is going to be a user of that? >> > >> >> This is expected to be used by a framework (upcoming) for detecting slp_s0 >> signal assertion related failures. It might be good to put few words in the commit message about expecting user(s). >> > > +static void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) >> > > +{ >> > > + debugfs_remove_recursive(pmc->dbgfs_dir); >> > > +} >> > >> > No need to keep this under #ifdef. >> > >> >> Based on some previus review comments we decided to put the dummy functions >> for !CONIG_DEBUG_FS case. Can you please elaborate more on this change request? >> > > I think Andy's point is that there is no reason to specialize this function > since debugs_remove_recursive checks for null. > > The problem of course is that dbgfs_dir only exists if CONFIG_DEBUG_FS exists. I > don't know if that's worth ifdeffing out of the struct, but there is precendent > for doing it that way, and I didn't feel strongly one way or the other, so I > left it up to you. > If you want to conditionally include the field in the struct, then this is fine > as is. Yeah, for the consistency either way is okay. I prefer to have less #ifdef:s, but here it's a not bit burden. >> > > + pmc.regmap = ioremap_nocache(pmc.base_addr, SPT_PMC_MMIO_REG_LEN); >> Do you recommend devm_ioremap_nocache as well? Good point.
diff --git a/MAINTAINERS b/MAINTAINERS index 1d5b4be..9164949 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5879,6 +5879,14 @@ S: Maintained F: arch/x86/include/asm/intel_telemetry.h F: drivers/platform/x86/intel_telemetry* +INTEL PMC CORE DRIVER +M: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> +M: Vishwanath Somayaji <vishwanath.somayaji@intel.com> +L: platform-driver-x86@vger.kernel.org +S: Maintained +F: drivers/platform/x86/intel_pmc_core.h +F: drivers/platform/x86/intel_pmc_core.c + IOC3 ETHERNET DRIVER M: Ralf Baechle <ralf@linux-mips.org> L: linux-mips@linux-mips.org diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index ed2004b..2d2b3f6 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -821,6 +821,18 @@ config INTEL_IPS functionality. If in doubt, say Y here; it will only load on supported platforms. +config INTEL_PMC_CORE + bool "Intel PMC Core driver" + depends on X86 && PCI + ---help--- + The Intel Platform Controller Hub for Intel Core SoCs provides access + to Power Management Controller registers via a pci interface. This + driver can utilize debugging capabilities and supported features as + exposed by the Power Management Controller. + + Supported features: + - slp_s0_residency counter. + config INTEL_IMR bool "Intel Isolated Memory Region support" default n diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 448443c..9b11b40 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -69,3 +69,4 @@ obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ intel_telemetry_pltdrv.o \ intel_telemetry_debugfs.o +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c new file mode 100644 index 0000000..0b5388e --- /dev/null +++ b/drivers/platform/x86/intel_pmc_core.c @@ -0,0 +1,201 @@ +/* + * Intel Core SOC Power Management Controller Driver + * + * Copyright (c) 2016, Intel Corporation. + * All Rights Reserved. + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@intel.com) + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#include <linux/init.h> +#include <linux/pci.h> +#include <linux/device.h> +#include <linux/debugfs.h> +#include <linux/seq_file.h> +#include <linux/module.h> +#include <linux/io.h> +#include <asm/cpu_device_id.h> +#include "intel_pmc_core.h" + +static struct pmc_dev pmc; + +static const struct pci_device_id pmc_pci_ids[] = { + { PCI_VDEVICE(INTEL, SPT_PMC_PCI_DEVICE_ID), (kernel_ulong_t)NULL }, + { 0, }, +}; +MODULE_DEVICE_TABLE(pci, pmc_pci_ids); + +/** + * intel_pmc_slp_s0_counter_read() - Read slp_s0 residency. + * @data: Out param that contains current slp_s0 count. + * + * This API currently supports Intel Skylake SOC and Sunrise + * point Platform Controller Hub. Future platform support + * should be added for platforms that support low power modes + * beyond Package C10 state. + * + * SLP_S0_RESIDENCY counter counts in 100 us granularity per + * step hence function populates the multiplied value in out + * parameter @data + * + * Return: an error code or 0 on success. + */ +int intel_pmc_slp_s0_counter_read(u64 *data) +{ + if (!pmc.has_slp_s0_res) + return -EACCES; + + *data = readl(pmc.regmap + SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); + *data *= SPT_PMC_SLP_S0_RES_COUNTER_STEP; + + return 0; +} +EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); + +#if IS_ENABLED(CONFIG_DEBUG_FS) +static int pmc_core_dev_state_show(struct seq_file *s, void *unused) +{ + u64 counter_val; + int err; + + err = intel_pmc_slp_s0_counter_read(&counter_val); + if (!err) + seq_printf(s, "%lld\n", counter_val); + + return err; +} + +static int pmc_core_dev_state_open(struct inode *inode, struct file *file) +{ + return single_open(file, pmc_core_dev_state_show, inode->i_private); +} + +static const struct file_operations pmc_core_dev_state_ops = { + .open = pmc_core_dev_state_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) +{ + debugfs_remove_recursive(pmc->dbgfs_dir); +} + +static int pmc_core_dbgfs_register(struct pmc_dev *pmc) +{ + struct dentry *dir, *file; + int ret = 0; + + dir = debugfs_create_dir("pmc_core", NULL); + if (!dir) + return -ENOMEM; + + pmc->dbgfs_dir = dir; + file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO, + dir, pmc, &pmc_core_dev_state_ops); + + if (!file) { + pmc_core_dbgfs_unregister(pmc); + ret = -ENODEV; + } + return ret; +} +#else +static inline int pmc_core_dbgfs_register(struct pmc_dev *pmc) +{ + return 0; /* nothing to register */ +} + +static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmc) +{ + /* nothing to deregister */ +} +#endif /* CONFIG_DEBUG_FS */ + +static const struct x86_cpu_id intel_pmc_core_ids[] = { + { X86_VENDOR_INTEL, 6, 0x4e, X86_FEATURE_MWAIT, + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ + { X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT, + (kernel_ulong_t)NULL}, /* Skylake CPUID Signature */ + {} +}; +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); + +static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id) +{ + int err; + const struct x86_cpu_id *cpu_id; + + cpu_id = x86_match_cpu(intel_pmc_core_ids); + if (!cpu_id) { + err = -EINVAL; + goto exit; + } + + err = pci_enable_device(dev); + if (err) { + dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n"); + goto exit; + } + + err = pci_read_config_dword(dev, + SPT_PMC_BASE_ADDR_OFFSET, + &pmc.base_addr); + if (err) { + dev_err(&dev->dev, "PMC Core: failed to read pci config space.\n"); + goto disable_pci; + } + dev_dbg(&dev->dev, "PMC Core: PWRMBASE is 0x%x\n", pmc.base_addr); + + pmc.regmap = ioremap_nocache(pmc.base_addr, SPT_PMC_MMIO_REG_LEN); + if (!pmc.regmap) { + dev_err(&dev->dev, "PMC Core: ioremap failed\n"); + err = -ENOMEM; + goto disable_pci; + } + + err = pmc_core_dbgfs_register(&pmc); + if (err) { + dev_err(&dev->dev, "PMC Core: debugfs register failed\n"); + iounmap(pmc.regmap); + goto disable_pci; + } + + pmc.has_slp_s0_res = true; + return 0; + +disable_pci: + pci_disable_device(dev); +exit: + return err; +} + +static void intel_pmc_core_remove(struct pci_dev *pdev) +{ + pmc_core_dbgfs_unregister(&pmc); + pci_disable_device(pdev); + iounmap(pmc.regmap); +} + +static struct pci_driver intel_pmc_core_driver = { + .name = "intel_pmc_core", + .id_table = pmc_pci_ids, + .probe = pmc_core_probe, + .remove = intel_pmc_core_remove, +}; +module_pci_driver(intel_pmc_core_driver); + +MODULE_AUTHOR("Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>"); +MODULE_AUTHOR("Vishwanath Somayaji <vishwanath.somayaji@intel.com>"); +MODULE_DESCRIPTION("Intel CORE SOC Power Management Controller Interface"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h new file mode 100644 index 0000000..a6d7bba --- /dev/null +++ b/drivers/platform/x86/intel_pmc_core.h @@ -0,0 +1,54 @@ +/* + * Intel Core SOC Power Management Controller Header File + * + * Copyright (c) 2016, Intel Corporation. + * All Rights Reserved. + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@intel.com) + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#ifndef PMC_CORE_H +#define PMC_CORE_H + +/* Sunrise Point Power Management Controller PCI Device ID */ +#define SPT_PMC_PCI_DEVICE_ID 0x9d21 +#define SPT_PMC_BASE_ADDR_OFFSET 0x48 +#define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET 0x13c +#define SPT_PMC_MMIO_REG_LEN 0x100 +#define SPT_PMC_REG_BIT_WIDTH 0x20 +#define SPT_PMC_SLP_S0_RES_COUNTER_STEP 0x64 + +/** + * struct pmc_dev - pmc device structure + * @base_addr: comtains pmc base address + * @regmap: pointer to io-remapped memory location + * @dbgfs_dir: path to debug fs interface + * @feature_available: flag to indicate whether + * the feature is available + * on a particular platform or not. + * + * pmc_dev contains info about power management controller device. + */ +struct pmc_dev { + u32 base_addr; + void __iomem *regmap; +#if IS_ENABLED(CONFIG_DEBUG_FS) + struct dentry *dbgfs_dir; +#endif /* CONFIG_DEBUG_FS */ + bool has_slp_s0_res; +}; + +int intel_pmc_slp_s0_counter_read(u64 *data); + +#endif + +/* PMC_CORE_H */