[PATCH] platform:x86: Add PMC Driver for Intel Core SOC

[PATCH] platform:x86: Add PMC Driver for Intel Core SOC

[Rajneesh Bhardwaj]

This patch adds the Power Management Controller driver as a pci driver
for Intel Core SOC architecture.

This driver can be utilized for debugging as well as for detecting
fragile SLP_S0 failures and take corrective actions for scenarios
when PCH SLP_S0 signal is not asserted after kernel freeze as part
of suspend to idle flow i.e. 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
the deepest Package C-State is PC10 so the suspend to idle results in
package C-10 state but PC10 is not sufficient for realizing the best
possible power potential which SLP_S0 signal assertion can provide.

In general, SLP_S0 assertion == PC10 + PCH low power mode + ModPhy Lanes
power gated + PLL Idle.

SLP_S0 signal is often connected to Embedded Controller and Power
Management IC for other platform power management related optimizations.

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
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>
---
 arch/x86/include/asm/pmc_core.h       |  53 +++++++++
 drivers/platform/x86/Kconfig          |  10 ++
 drivers/platform/x86/Makefile         |   1 +
 drivers/platform/x86/intel_pmc_core.c | 200 ++++++++++++++++++++++++++++++++++
 4 files changed, 264 insertions(+)
 create mode 100644 arch/x86/include/asm/pmc_core.h
 create mode 100644 drivers/platform/x86/intel_pmc_core.c

diff --git a/arch/x86/include/asm/pmc_core.h b/arch/x86/include/asm/pmc_core.h
new file mode 100644
index 0000000..3ea61bf
--- /dev/null
+++ b/arch/x86/include/asm/pmc_core.h
@@ -0,0 +1,53 @@
+/*
+ * Intel Core SOC Power Management Controller Header File
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ * 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
+
+/* Skylake Power Management Controller PCI Device ID */
+#define PCI_DEVICE_ID_SKL_PMC   0x9d21
+#define PMC_BASE_ADDR_OFFSET    0x48
+#define PMC_SLP_S0_RESIDENCY_COUNTER 0x13c
+#define PMC_MMIO_REG_LEN        0x100
+#define PMC_REG_BIT_WIDTH       0x20
+#define SLP_S0_RESIDENCY_COUNTER_GRANULARITY 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 */
+	int feature_available;
+};
+
+int intel_pmc_slp_s0_counter_read(u64 *data);
+
+#endif
+
+/* PMC_CORE_H */
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ed2004b..5db364c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -821,6 +821,16 @@ 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
+	default y
+	---help---
+	  This driver exposes the features provided by Power Management
+	  Controller for Intel Core SOC. Intel Platform Controller Hub
+	  for Intel Core SOCs provides access to Power Management Controller
+	  registers via pci interface.
+
 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..42cee87
--- /dev/null
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -0,0 +1,200 @@
+/*
+ * Intel Core SOC Power Management Controller Driver
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ * 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 <asm/pmc_core.h>
+
+static struct pmc_dev pmc;
+
+static const struct pci_device_id pmc_pci_ids[] = {
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_SKL_PMC), (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)
+{
+	int ret = 0;
+
+	if (pmc.feature_available) {
+		*data = readl(pmc.regmap + PMC_SLP_S0_RESIDENCY_COUNTER);
+		*data *= SLP_S0_RESIDENCY_COUNTER_GRANULARITY;
+	} else {
+		ret = -EACCES;
+	}
+
+	return ret;
+}
+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) {
+		counter_val = 0;
+		seq_printf(s, "%lld\n", counter_val);
+	} else {
+		seq_printf(s, "%lld\n", counter_val);
+	}
+
+	return 0;
+}
+
+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 int pmc_core_dbgfs_register(struct pmc_dev *pmc)
+{
+	return 0; /* nothing to register */
+}
+
+#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}, /* SKL CPU*/
+	{ X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT,
+		(kernel_ulong_t)NULL}, /* SKL CPU*/
+	{}
+};
+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)
+		return -EINVAL;
+
+	err = pci_enable_device(dev);
+	if (err) {
+		dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n");
+		goto exit;
+	}
+
+	pci_read_config_dword(dev, PMC_BASE_ADDR_OFFSET, &pmc.base_addr);
+	dev_dbg(&dev->dev, "PMC Core: PWRMBASE is 0x%x\n", pmc.base_addr);
+
+	pmc.regmap = ioremap_nocache(pmc.base_addr, PMC_MMIO_REG_LEN);
+	if (!pmc.regmap) {
+		dev_err(&dev->dev, "PMC Core: ioremap failed\n");
+		err = -ENOMEM;
+		goto disable;
+	}
+
+	err = pmc_core_dbgfs_register(&pmc);
+	if (err) {
+		dev_err(&dev->dev, "PMC Core: debugfs register failed\n");
+		iounmap(pmc.regmap);
+		goto disable;
+	}
+
+	pmc.feature_available = 1;
+	return 0;
+
+disable:
+	pci_disable_device(dev);
+exit:
+	return err;
+}
+
+static void intel_pmc_core_remove(struct pci_dev *pdev)
+{
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	pmc_core_dbgfs_unregister(&pmc);
+#endif
+	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");
-- 
1.9.1

Re: [PATCH] platform:x86: Add PMC Driver for Intel Core SOC

[Giedrius Statkevičius]

On Thu, Apr 21, 2016 at 04:33:25PM +0530, Rajneesh Bhardwaj wrote:
[...]

Just some minor things I've spotted and one error is probably unchecked.

[...]
> diff --git a/arch/x86/include/asm/pmc_core.h b/arch/x86/include/asm/pmc_core.h
> new file mode 100644
> index 0000000..3ea61bf
> --- /dev/null
> +++ b/arch/x86/include/asm/pmc_core.h
> @@ -0,0 +1,53 @@
> +/*
> + * Intel Core SOC Power Management Controller Header File
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + * 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
> +
> +/* Skylake Power Management Controller PCI Device ID */
> +#define PCI_DEVICE_ID_SKL_PMC   0x9d21
> +#define PMC_BASE_ADDR_OFFSET    0x48
> +#define PMC_SLP_S0_RESIDENCY_COUNTER 0x13c
> +#define PMC_MMIO_REG_LEN        0x100
> +#define PMC_REG_BIT_WIDTH       0x20
> +#define SLP_S0_RESIDENCY_COUNTER_GRANULARITY 0x64

Inconsistent spaces. Maybe just use one space.

[...]

> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> new file mode 100644
> index 0000000..42cee87
> --- /dev/null
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -0,0 +1,200 @@
> +/*
> + * Intel Core SOC Power Management Controller Driver
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + * 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 <asm/pmc_core.h>
> +
> +static struct pmc_dev pmc;
> +
> +static const struct pci_device_id pmc_pci_ids[] = {
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_SKL_PMC), (kernel_ulong_t)NULL },

Minor thing here but maybe just use a simple 0 here instead of
(kernel_ulong_t)NULL? In the other places as well.

[...]

> +static int pmc_core_dbgfs_register(struct pmc_dev *pmc)
> +{
> +	return 0; /* nothing to register */
> +}
> +
> +#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}, /* SKL CPU*/
> +	{ X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT,
> +		(kernel_ulong_t)NULL}, /* SKL CPU*/
> +	{}

A space after CPU? SKL? I assume this means skylake so perhaps add the whole
name.

> +};
> +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)
> +		return -EINVAL;
> +
> +	err = pci_enable_device(dev);
> +	if (err) {
> +		dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n");
> +		goto exit;
> +	}
> +
> +	pci_read_config_dword(dev, PMC_BASE_ADDR_OFFSET, &pmc.base_addr);

Shouldn't the return value of this function be checked if an error occured? Also
while we are at it maybe the label names could be improved. For example:
err_disable_dev; err_return

> +	err = pmc_core_dbgfs_register(&pmc);
> +	if (err) {
> +		dev_err(&dev->dev, "PMC Core: debugfs register failed\n");
> +		iounmap(pmc.regmap);
> +		goto disable;
> +	}
> +
> +	pmc.feature_available = 1;
> +	return 0;
> +
> +disable:
> +	pci_disable_device(dev);
> +exit:
> +	return err;
> +}
> +

Re: [PATCH] platform:x86: Add PMC Driver for Intel Core SOC

[Darren Hart]

On Thu, Apr 21, 2016 at 03:47:29PM +0300, Giedrius Statkevičius wrote:
> On Thu, Apr 21, 2016 at 04:33:25PM +0530, Rajneesh Bhardwaj wrote:
> [...]
> 
> Just some minor things I've spotted and one error is probably unchecked.
> 
> [...]
> > diff --git a/arch/x86/include/asm/pmc_core.h b/arch/x86/include/asm/pmc_core.h
> > new file mode 100644
> > index 0000000..3ea61bf
> > --- /dev/null
> > +++ b/arch/x86/include/asm/pmc_core.h
> > @@ -0,0 +1,53 @@
> > +/*
> > + * Intel Core SOC Power Management Controller Header File
> > + *
> > + * Copyright (c) 2016, Intel Corporation.
> > + * 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
> > +
> > +/* Skylake Power Management Controller PCI Device ID */
> > +#define PCI_DEVICE_ID_SKL_PMC   0x9d21
> > +#define PMC_BASE_ADDR_OFFSET    0x48
> > +#define PMC_SLP_S0_RESIDENCY_COUNTER 0x13c
> > +#define PMC_MMIO_REG_LEN        0x100
> > +#define PMC_REG_BIT_WIDTH       0x20
> > +#define SLP_S0_RESIDENCY_COUNTER_GRANULARITY 0x64
> 
> Inconsistent spaces. Maybe just use one space.
> 
> [...]
> 
> > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> > new file mode 100644
> > index 0000000..42cee87
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Intel Core SOC Power Management Controller Driver
> > + *
> > + * Copyright (c) 2016, Intel Corporation.
> > + * 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 <asm/pmc_core.h>
> > +
> > +static struct pmc_dev pmc;
> > +
> > +static const struct pci_device_id pmc_pci_ids[] = {
> > +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_SKL_PMC), (kernel_ulong_t)NULL },
> 
> Minor thing here but maybe just use a simple 0 here instead of
> (kernel_ulong_t)NULL? In the other places as well.
> 
> [...]
> 

Please provide the rational for a request for change. What do you think is wrong
with the above? There is precedent in the kernel today for this usage. Is it
wrong? Unnecessary? Subjectively ugly?

> > +static int pmc_core_dbgfs_register(struct pmc_dev *pmc)
> > +{
> > +	return 0; /* nothing to register */
> > +}
> > +
> > +#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}, /* SKL CPU*/
> > +	{ X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT,
> > +		(kernel_ulong_t)NULL}, /* SKL CPU*/
> > +	{}
> 
> A space after CPU? SKL? I assume this means skylake so perhaps add the whole
> name.
> 

The three letter CPU code is very common throughout the kernel of Intel CPUs.
NHM,WSM,SNB,IVB,BYT,HSW,BDW,SKL, etc.

Those are becoming more and more difficult to grep for though, so a full
expansion now and then would certainly be nice :-)

> > +};
> > +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)
> > +		return -EINVAL;
> > +
> > +	err = pci_enable_device(dev);
> > +	if (err) {
> > +		dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n");
> > +		goto exit;
> > +	}
> > +
> > +	pci_read_config_dword(dev, PMC_BASE_ADDR_OFFSET, &pmc.base_addr);
> 
> Shouldn't the return value of this function be checked if an error occured? Also

Yes please.

> while we are at it maybe the label names could be improved. For example:
> err_disable_dev; err_return
> 

This is more typical, although "out" or "err_out" if only used in the error
case. However, if the label does nothing but return with an error code, be sure
to be consistent in it's usage. Don't do "return -EINVAL" and "ret = -EINVAL;
goto err_out;" in the same function.


Thanks,

> > +	err = pmc_core_dbgfs_register(&pmc);
> > +	if (err) {
> > +		dev_err(&dev->dev, "PMC Core: debugfs register failed\n");
> > +		iounmap(pmc.regmap);
> > +		goto disable;
> > +	}
> > +
> > +	pmc.feature_available = 1;
> > +	return 0;
> > +
> > +disable:
> > +	pci_disable_device(dev);
> > +exit:
> > +	return err;
> > +}
> > +
> 

-- 
Darren Hart
Intel Open Source Technology Center

RE: [PATCH] platform:x86: Add PMC Driver for Intel Core SOC

[Bhardwaj, Rajneesh]

Thanks for the review , will send the revised patch v2
 with all review comments addressed so far.

Regards
Rajneesh


>-----Original Message-----
>From: Darren Hart [mailto:dvhart@infradead.org]
>Sent: Monday, April 25, 2016 11:41 PM
>To: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
>Cc: Bhardwaj, Rajneesh <rajneesh.bhardwaj@intel.com>; platform-driver-
>x86@vger.kernel.org; Somayaji, Vishwanath
><vishwanath.somayaji@intel.com>
>Subject: Re: [PATCH] platform:x86: Add PMC Driver for Intel Core SOC
>
>On Thu, Apr 21, 2016 at 03:47:29PM +0300, Giedrius Statkevičius wrote:
>> On Thu, Apr 21, 2016 at 04:33:25PM +0530, Rajneesh Bhardwaj wrote:
>> [...]
>>
>> Just some minor things I've spotted and one error is probably unchecked.
>>
>> [...]
>> > diff --git a/arch/x86/include/asm/pmc_core.h
>> > b/arch/x86/include/asm/pmc_core.h new file mode 100644 index
>> > 0000000..3ea61bf
>> > --- /dev/null
>> > +++ b/arch/x86/include/asm/pmc_core.h
>> > @@ -0,0 +1,53 @@
>> > +/*
>> > + * Intel Core SOC Power Management Controller Header File
>> > + *
>> > + * Copyright (c) 2016, Intel Corporation.
>> > + * 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
>> > +
>> > +/* Skylake Power Management Controller PCI Device ID */
>> > +#define PCI_DEVICE_ID_SKL_PMC   0x9d21
>> > +#define PMC_BASE_ADDR_OFFSET    0x48
>> > +#define PMC_SLP_S0_RESIDENCY_COUNTER 0x13c
>> > +#define PMC_MMIO_REG_LEN        0x100
>> > +#define PMC_REG_BIT_WIDTH       0x20
>> > +#define SLP_S0_RESIDENCY_COUNTER_GRANULARITY 0x64
>>
>> Inconsistent spaces. Maybe just use one space.
>>
>> [...]
>>
>> > diff --git a/drivers/platform/x86/intel_pmc_core.c
>> > b/drivers/platform/x86/intel_pmc_core.c
>> > new file mode 100644
>> > index 0000000..42cee87
>> > --- /dev/null
>> > +++ b/drivers/platform/x86/intel_pmc_core.c
>> > @@ -0,0 +1,200 @@
>> > +/*
>> > + * Intel Core SOC Power Management Controller Driver
>> > + *
>> > + * Copyright (c) 2016, Intel Corporation.
>> > + * 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 <asm/pmc_core.h>
>> > +
>> > +static struct pmc_dev pmc;
>> > +
>> > +static const struct pci_device_id pmc_pci_ids[] = {
>> > +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_SKL_PMC),
>(kernel_ulong_t)NULL
>> > +},
>>
>> Minor thing here but maybe just use a simple 0 here instead of
>> (kernel_ulong_t)NULL? In the other places as well.
>>
>> [...]
>>
>
>Please provide the rational for a request for change. What do you think is
>wrong with the above? There is precedent in the kernel today for this usage.
>Is it wrong? Unnecessary? Subjectively ugly?
>
>> > +static int pmc_core_dbgfs_register(struct pmc_dev *pmc) {
>> > +	return 0; /* nothing to register */ }
>> > +
>> > +#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}, /* SKL CPU*/
>> > +	{ X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT,
>> > +		(kernel_ulong_t)NULL}, /* SKL CPU*/
>> > +	{}
>>
>> A space after CPU? SKL? I assume this means skylake so perhaps add the
>> whole name.
>>
>
>The three letter CPU code is very common throughout the kernel of Intel
>CPUs.
>NHM,WSM,SNB,IVB,BYT,HSW,BDW,SKL, etc.
>
>Those are becoming more and more difficult to grep for though, so a full
>expansion now and then would certainly be nice :-)
>
>> > +};
>> > +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)
>> > +		return -EINVAL;
>> > +
>> > +	err = pci_enable_device(dev);
>> > +	if (err) {
>> > +		dev_err(&dev->dev, "PMC Core: failed to enable Power
>Management Controller.\n");
>> > +		goto exit;
>> > +	}
>> > +
>> > +	pci_read_config_dword(dev, PMC_BASE_ADDR_OFFSET,
>&pmc.base_addr);
>>
>> Shouldn't the return value of this function be checked if an error
>> occured? Also
>
>Yes please.
>
>> while we are at it maybe the label names could be improved. For example:
>> err_disable_dev; err_return
>>
>
>This is more typical, although "out" or "err_out" if only used in the error case.
>However, if the label does nothing but return with an error code, be sure to
>be consistent in it's usage. Don't do "return -EINVAL" and "ret = -EINVAL; goto
>err_out;" in the same function.
>
>
>Thanks,
>
>> > +	err = pmc_core_dbgfs_register(&pmc);
>> > +	if (err) {
>> > +		dev_err(&dev->dev, "PMC Core: debugfs register failed\n");
>> > +		iounmap(pmc.regmap);
>> > +		goto disable;
>> > +	}
>> > +
>> > +	pmc.feature_available = 1;
>> > +	return 0;
>> > +
>> > +disable:
>> > +	pci_disable_device(dev);
>> > +exit:
>> > +	return err;
>> > +}
>> > +
>>
>
>--
>Darren Hart
>Intel Open Source Technology Center