* [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC
@ 2016-05-12 13:50 Rajneesh Bhardwaj
2016-05-12 15:02 ` Andy Shevchenko
2016-05-21 0:34 ` dbasehore .
0 siblings, 2 replies; 6+ messages in thread
From: Rajneesh Bhardwaj @ 2016-05-12 13:50 UTC (permalink / raw)
To: platform-driver-x86
Cc: dvhart, linux-kernel, olof, vishwanath.somayaji, rajneesh.bhardwaj
This patch adds the Power Management Controller driver as a pci driver
for Intel Core SOC architecture.
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
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
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>
---
Changes in v4:
* Moved the header file to drivers/platform/x86 directory.
* Updated the same in MAINTAINERS.
* Removed 'default y' option from Kconfig.
* Introduced static inline dummy functions for debugfs register
and unregister case when CONFIG_DEBUG_FS is not set. Removed
#if IS_ENABLED(CONFIG_DEBUG_FS) check from .probe and .remove calls.
* Add CC to LKML (linux-kernel@vger.kernel.org)
* Earlier review comments till v3 are available at:
- http://www.spinics.net/lists/platform-driver-x86/msg08790.html
- http://www.spinics.net/lists/platform-driver-x86/msg08759.html
- http://www.spinics.net/lists/platform-driver-x86/msg08742.html
Changes in v3:
* Updated the commit message, added reference to the chipset datasheet.
* Renamed the header file to intel_pmc_core.h for consistency.
* Added All rights reserved notification after the Copyright message.
* Improved variable names in the header file. Added SPT prefix.
* Fixed kernel-doc related whitespace and comma issues for struct pmc_dev.
* Changed feature_available to bool has_slp_s0_res.
* Enhanced the Kconfig description as per the review suggestions.
* Added error propagating logic to pmc_core_dev_state_show.
* Streamlined intel_pmc_slp_s0_counter_read as per the review comments.
* Streamlined the use of #if IS_ENABLED(CONFIG_DEBUG_FS) while registering
debugfs in the probe call.
* Removed the *duplicate definition* of pmc_core_debugfs_register.
* Added MAINTAINERS related changes.
* Checkpatch warning due to long URL name in the commit message.
Changes in v2:
* Fixed inconsistent spaces in the header file.
* Updated commit message.
* Enhanced acronym SKL CPU to Skylake CPUID Signature.
* Put error check on pci_read_config_dword in probe function.
* Changed goto label (disable) to disable_pci.
* Changed x86_match_cpu error handling for consistency.
MAINTAINERS | 8 ++
drivers/platform/x86/Kconfig | 12 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_pmc_core.c | 201 ++++++++++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 54 +++++++++
5 files changed, 276 insertions(+)
create mode 100644 drivers/platform/x86/intel_pmc_core.c
create mode 100644 drivers/platform/x86/intel_pmc_core.h
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 */
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC
2016-05-12 13:50 [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC Rajneesh Bhardwaj
@ 2016-05-12 15:02 ` Andy Shevchenko
2016-05-16 10:15 ` Rajneesh Bhardwaj
2016-05-21 0:34 ` dbasehore .
1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2016-05-12 15:02 UTC (permalink / raw)
To: Rajneesh Bhardwaj
Cc: platform-driver-x86, dvhart, linux-kernel, Olof Johansson,
vishwanath.somayaji
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);
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC
2016-05-12 15:02 ` Andy Shevchenko
@ 2016-05-16 10:15 ` Rajneesh Bhardwaj
2016-05-16 22:59 ` Darren Hart
0 siblings, 1 reply; 6+ messages in thread
From: Rajneesh Bhardwaj @ 2016-05-16 10:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: platform-driver-x86, dvhart, linux-kernel, Olof Johansson,
vishwanath.somayaji
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
--
Best Regards,
Rajneesh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC
2016-05-16 10:15 ` Rajneesh Bhardwaj
@ 2016-05-16 22:59 ` Darren Hart
2016-05-24 18:27 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Darren Hart @ 2016-05-16 22:59 UTC (permalink / raw)
To: Rajneesh Bhardwaj, H. Peter Anvin, Thomas Gleixner
Cc: Andy Shevchenko, platform-driver-x86, linux-kernel,
Olof Johansson, vishwanath.somayaji
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
>
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC
2016-05-12 13:50 [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC Rajneesh Bhardwaj
2016-05-12 15:02 ` Andy Shevchenko
@ 2016-05-21 0:34 ` dbasehore .
1 sibling, 0 replies; 6+ messages in thread
From: dbasehore . @ 2016-05-21 0:34 UTC (permalink / raw)
To: Rajneesh Bhardwaj
Cc: platform-driver-x86, dvhart, linux-kernel, olof, vishwanath.somayaji
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC
2016-05-16 22:59 ` Darren Hart
@ 2016-05-24 18:27 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2016-05-24 18:27 UTC (permalink / raw)
To: Darren Hart
Cc: Rajneesh Bhardwaj, H. Peter Anvin, Thomas Gleixner,
platform-driver-x86, linux-kernel, Olof Johansson,
vishwanath.somayaji
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-24 18:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-12 13:50 [PATCH v4] platform:x86: Add PMC Driver for Intel Core SOC Rajneesh Bhardwaj
2016-05-12 15:02 ` Andy Shevchenko
2016-05-16 10:15 ` Rajneesh Bhardwaj
2016-05-16 22:59 ` Darren Hart
2016-05-24 18:27 ` Andy Shevchenko
2016-05-21 0:34 ` dbasehore .
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.