Message-ID: <1464259989.27624.6.camel@linux.intel.com>
Date:	Thu, 26 May 2016 13:53:09 +0300
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>,
	platform-driver-x86@...r.kernel.org
Cc:	dvhart@...radead.org, linux-kernel@...r.kernel.org, olof@...om.net,
	tglx@...utronix.de, hpa@...or.com, dbasehore@...gle.com,
	vishwanath.somayaji@...el.com
Subject: Re: [PATCH v7] platform:x86: Add PMC Driver for Intel Core SoC

On Thu, 2016-05-26 at 14:41 +0530, Rajneesh Bhardwaj wrote:
> 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-data
> sheet-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@...el.com>
> Signed-off-by: Vishwanath Somayaji <vishwanath.somayaji@...el.com>

Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

> ---
> Changes in v7:
>  * Using builtin_pci_driver instead of device_initcall to fix build
> warning
>  * Deleted .remove line from struct intel_pmc_core_driver
> 
> Changes in v6:
>  * Removed module specific code:
> 	- removed module.h, .remove callback.
> 	- replaced module_pci_driver with device_initcall.
> 	- removed MODULE_*
>  * Address style related review comments.
>  * Updated Authors, MAINTAINERS.
>  * Removed unused #define from intel_pmc_core.h:
> 	- SPT_PMC_REG_BIT_WIDTH
> 
> Changes in v5:
>  * Addressed generic review comments for v4.
>  * Added generic read function "pmc_core_reg_read".
>  * Split the header file into two:
> 	- arch/x86/include/asm/pmc_core.h => exposes API.
> 	- driver/platform/x86/intel_pmc_core.h => used by the driver.
>  * Updated signature for intel_pmc_slp_s0_counter_read as slp_s0
>    counter is a 32 bit counter.
>  * Updated SOB, Authors and MAINTAINERS.
>  * Using managed APIs:
> 	- pcim_enable_device
> 	- devm_ioremap_nocache
>  * Updated probe and remove code.
> 
> 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@...r.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 ++
>  arch/x86/include/asm/pmc_core.h       |  27 +++++
>  drivers/platform/x86/Kconfig          |  12 ++
>  drivers/platform/x86/Makefile         |   1 +
>  drivers/platform/x86/intel_pmc_core.c | 200
> ++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h |  51 +++++++++
>  6 files changed, 299 insertions(+)
>  create mode 100644 arch/x86/include/asm/pmc_core.h
>  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 3302006..db18359 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6096,6 +6096,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@...el.com>
> +M:	Vishwanath Somayaji <vishwanath.somayaji@...el.com>
> +L:	platform-driver-x86@...r.kernel.org
> +S:	Maintained
> +F:	arch/x86/include/asm/pmc_core.h
> +F:	drivers/platform/x86/intel_pmc_core*
> +
>  IOC3 ETHERNET DRIVER
>  M:	Ralf Baechle <ralf@...ux-mips.org>
>  L:	linux-mips@...ux-mips.org
> diff --git a/arch/x86/include/asm/pmc_core.h
> b/arch/x86/include/asm/pmc_core.h
> new file mode 100644
> index 0000000..d4855f1
> --- /dev/null
> +++ b/arch/x86/include/asm/pmc_core.h
> @@ -0,0 +1,27 @@
> +/*
> + * Intel Core SoC Power Management Controller Header File
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + * All Rights Reserved.
> + *
> + * Authors: Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>
> + *          Vishwanath Somayaji <vishwanath.somayaji@...el.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 _ASM_PMC_CORE_H
> +#define _ASM_PMC_CORE_H
> +
> +/* API to read SLP_S0_RESIDENCY counter */
> +int intel_pmc_slp_s0_counter_read(u32 *data);
> +
> +#endif /* _ASM_PMC_CORE_H */
> diff --git a/drivers/platform/x86/Kconfig
> b/drivers/platform/x86/Kconfig
> index ed2004b..c06bb85 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -846,6 +846,18 @@ config INTEL_IMR
>  
>  	  If you are running on a Galileo/Quark say Y here.
>  
> +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 IBM_RTL
>  	tristate "Device driver to enable PRTL support"
>  	depends on X86 && PCI
> 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..2776bec
> --- /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.
> + * All Rights Reserved.
> + *
> + * Authors: Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>
> + *          Vishwanath Somayaji <vishwanath.somayaji@...el.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/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/pci.h>
> +#include <linux/seq_file.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/pmc_core.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, },
> +};
> +
> +static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int
> reg_offset)
> +{
> +	return readl(pmcdev->regbase + reg_offset);
> +}
> +
> +static inline u32 pmc_core_adjust_slp_s0_step(u32 value)
> +{
> +	return value * SPT_PMC_SLP_S0_RES_COUNTER_STEP;
> +}
> +
> +/**
> + * 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(u32 *data)
> +{
> +	struct pmc_dev *pmcdev = &pmc;
> +	u32 value;
> +
> +	if (!pmcdev->has_slp_s0_res)
> +		return -EACCES;
> +
> +	value = pmc_core_reg_read(pmcdev,
> SPT_PMC_SLP_S0_RES_COUNTER_OFFSET);
> +	*data = pmc_core_adjust_slp_s0_step(value);
> +
> +	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)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	u32 counter_val;
> +
> +	counter_val = pmc_core_reg_read(pmcdev,
> +					SPT_PMC_SLP_S0_RES_COUNTER_OF
> FSET);
> +	seq_printf(s, "%u\n",
> pmc_core_adjust_slp_s0_step(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 *pmcdev)
> +{
> +	debugfs_remove_recursive(pmcdev->dbgfs_dir);
> +}
> +
> +static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> +{
> +	struct dentry *dir, *file;
> +
> +	dir = debugfs_create_dir("pmc_core", NULL);
> +	if (!dir)
> +		return -ENOMEM;
> +
> +	pmcdev->dbgfs_dir = dir;
> +	file = debugfs_create_file("slp_s0_residency_usec", S_IFREG |
> S_IRUGO,
> +				   dir, pmcdev,
> &pmc_core_dev_state_ops);
> +
> +	if (!file) {
> +		pmc_core_dbgfs_unregister(pmcdev);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +#else
> +static inline int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> +{
> +	return 0;
> +}
> +
> +static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> +{
> +}
> +#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 */
> +	{}
> +};
> +
> +static int pmc_core_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> +{
> +	struct device *ptr_dev = &dev->dev;
> +	struct pmc_dev *pmcdev = &pmc;
> +	const struct x86_cpu_id *cpu_id;
> +	int err;
> +
> +	cpu_id = x86_match_cpu(intel_pmc_core_ids);
> +	if (!cpu_id) {
> +		dev_dbg(&dev->dev, "PMC Core: cpuid mismatch.\n");
> +		return -EINVAL;
> +	}
> +
> +	err = pcim_enable_device(dev);
> +	if (err < 0) {
> +		dev_dbg(&dev->dev, "PMC Core: failed to enable Power
> Management Controller.\n");
> +		return err;
> +	}
> +
> +	err = pci_read_config_dword(dev,
> +				    SPT_PMC_BASE_ADDR_OFFSET,
> +				    &pmcdev->base_addr);
> +	if (err < 0) {
> +		dev_dbg(&dev->dev, "PMC Core: failed to read PCI
> config space.\n");
> +		return err;
> +	}
> +	dev_dbg(&dev->dev, "PMC Core: PWRMBASE is %#x\n", pmcdev-
> >base_addr);
> +
> +	pmcdev->regbase = devm_ioremap_nocache(ptr_dev,
> +					      pmcdev->base_addr,
> +					      SPT_PMC_MMIO_REG_LEN);
> +	if (!pmcdev->regbase) {
> +		dev_dbg(&dev->dev, "PMC Core: ioremap failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	err = pmc_core_dbgfs_register(pmcdev);
> +	if (err < 0) {
> +		dev_err(&dev->dev, "PMC Core: debugfs register
> failed.\n");
> +		return err;
> +	}
> +
> +	pmc.has_slp_s0_res = true;
> +	return 0;
> +}
> +
> +static struct pci_driver intel_pmc_core_driver = {
> +	.name = "intel_pmc_core",
> +	.id_table = pmc_pci_ids,
> +	.probe = pmc_core_probe,
> +};
> +
> +builtin_pci_driver(intel_pmc_core_driver);
> diff --git a/drivers/platform/x86/intel_pmc_core.h
> b/drivers/platform/x86/intel_pmc_core.h
> new file mode 100644
> index 0000000..a9dadaf
> --- /dev/null
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -0,0 +1,51 @@
> +/*
> + * Intel Core SoC Power Management Controller Header File
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + * All Rights Reserved.
> + *
> + * Authors: Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>
> + *          Vishwanath Somayaji <vishwanath.somayaji@...el.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_SLP_S0_RES_COUNTER_STEP		0x64
> +
> +/**
> + * struct pmc_dev - pmc device structure
> + * @base_addr:		comtains pmc base address
> + * @regbase:		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 *regbase;
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +	struct dentry *dbgfs_dir;
> +#endif /* CONFIG_DEBUG_FS */
> +	bool has_slp_s0_res;
> +};
> +
> +#endif /* PMC_CORE_H */

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy

Powered by blists - more mailing lists