On Thu, May 12, 2016 at 4:50 PM, Rajneesh Bhardwaj <rajneesh.bhardwaj@xxxxxxxxx> 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@xxxxxxxxx> > Signed-off-by: Vishwanath Somayaji <vishwanath.somayaji@xxxxxxxxx> Two people (1). > +INTEL PMC CORE DRIVER > +M: Rajneesh Bhardwaj <rajneesh.bhardwaj@xxxxxxxxx> > +M: Vishwanath Somayaji <vishwanath.somayaji@xxxxxxxxx> > +L: platform-driver-x86@xxxxxxxxxxxxxxx > +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@xxxxxxxxx) (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); > +} -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
