[PATCH v4] x86: punit_atom: punit device state debug driver

[PATCH v4] x86: punit_atom: punit device state debug driver

[Srinivas Pandruvada]

v4
- Created arch/x86/platform/atom/ and added this module here
- Removed static global and use struct seq_file->private
- Handle case when directory is created by create file in it failed
- Use cpuid in decimal and naming it

v3
- Fixed biggest blunder for using global static variable increment

v2
Addressed Ingo Molnar's comments
- Fix commit message
- Added punit explanation
- Formatting comments
- Moved to arch/x86/platform/intel_mid
- changed the debugfs file name

v1
 Based on review comments
 - Changed to tristate instead of bool
 - Moved config to kconfig.debug
 - Added debug in module name
 - Returning -ENXIO on debugfs file create error

v0:
Base version

Srinivas Pandruvada (1):
  x86: punit_atom: punit device state debug driver

 arch/x86/Kconfig.debug                    |  11 ++
 arch/x86/platform/Makefile                |   1 +
 arch/x86/platform/atom/Makefile           |   1 +
 arch/x86/platform/atom/punit_atom_debug.c | 183 ++++++++++++++++++++++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 arch/x86/platform/atom/Makefile
 create mode 100644 arch/x86/platform/atom/punit_atom_debug.c

-- 
1.9.1

[PATCH v4] x86: punit_atom: punit device state debug driver

[Srinivas Pandruvada]

The patch adds a debug driver, which dumps the power states
of all the North complex (NC) devices. This debug interface is
useful to figure out the devices,  which blocks the S0ix
transitions on the platform. This is extremely useful during
enabling PM on customer platforms and derivatives.

This submission is based on the submission from
Kumar P, Mahesh <mahesh.kumar.p.intel.com>.
https://lkml.org/lkml/2014/11/5/367
The changes done on top of the above submission:
- Dropped changes to config for PMC_ATOM, as PMC_ATOM
is not just a debug driver as suggested by the change. It has
additional power off interface also.
- Instead of just using nc ("North complex") use punit_..
similar to south complex PMC.
- Removed pmc_config structure,  as we don't need to predefine
number of register, we want to dump. This way new register
can be added without changing NC_NUM_DEVICES.
- prefixed function with punit_
- The debugfs directory will be punit_atom, which is NC equivalent
of pmc_atom, which we already exposed by pmc_atom driver.
- Added explanation for register and shift defines
- Will not create debugfs if the cpuid doesn't match
- Formatting changes to match compliance to coding convention
- Address review comments

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 arch/x86/Kconfig.debug                    |  11 ++
 arch/x86/platform/Makefile                |   1 +
 arch/x86/platform/atom/Makefile           |   1 +
 arch/x86/platform/atom/punit_atom_debug.c | 183 ++++++++++++++++++++++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 arch/x86/platform/atom/Makefile
 create mode 100644 arch/x86/platform/atom/punit_atom_debug.c

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 20028da..b525d86 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -336,4 +336,15 @@ config X86_DEBUG_STATIC_CPU_HAS
 
 	  If unsure, say N.
 
+config PUNIT_ATOM_DEBUG
+	tristate "ATOM Punit debug driver"
+	select DEBUG_FS
+	select IOSF_MBI
+	---help---
+	  This is a debug driver, which gets the power states
+	  of all Punit North Complex devices. The power states of
+	  each device is exposed as part of the debugfs interface.
+	  The current power state can be read from
+	  /sys/kernel/debug/punit_atom/dev_power_state
+
 endmenu
diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
index a62e0be..f1a6c8e 100644
--- a/arch/x86/platform/Makefile
+++ b/arch/x86/platform/Makefile
@@ -1,4 +1,5 @@
 # Platform specific code goes here
+obj-y	+= atom/
 obj-y	+= ce4100/
 obj-y	+= efi/
 obj-y	+= geode/
diff --git a/arch/x86/platform/atom/Makefile b/arch/x86/platform/atom/Makefile
new file mode 100644
index 0000000..0a3a40c
--- /dev/null
+++ b/arch/x86/platform/atom/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PUNIT_ATOM_DEBUG) += punit_atom_debug.o
diff --git a/arch/x86/platform/atom/punit_atom_debug.c b/arch/x86/platform/atom/punit_atom_debug.c
new file mode 100644
index 0000000..5ca8ead
--- /dev/null
+++ b/arch/x86/platform/atom/punit_atom_debug.c
@@ -0,0 +1,183 @@
+/*
+ * Intel SOC Punit device state debug driver
+ * Punit controls power management for North Complex devices (Graphics
+ * blocks, Image Signal Processing, video processing, display, DSP etc.)
+ *
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/io.h>
+#include <asm/cpu_device_id.h>
+#include <asm/iosf_mbi.h>
+
+/* Side band Interface port */
+#define PUNIT_PORT		0x04
+/* Power gate status reg */
+#define PWRGT_STATUS		0x61
+/* Subsystem config/status Video processor */
+#define VED_SS_PM0		0x32
+/* Subsystem config/status ISP (Image Signal Processor) */
+#define ISP_SS_PM0		0x39
+/* Subsystem config/status Input/output controller */
+#define MIO_SS_PM		0x3B
+/* Shift bits for getting status for video, isp and i/o */
+#define SSS_SHIFT		24
+/* Shift bits for getting status for graphics rendering */
+#define RENDER_POS		0
+/* Shift bits for getting status for media control */
+#define MEDIA_POS		2
+/* Shift bits for getting status for Valley View/Baytrail display */
+#define VLV_DISPLAY_POS		6
+/* Subsystem config/status display for Cherry Trail SOC */
+#define CHT_DSP_SSS		0x36
+/* Shift bits for getting status for display */
+#define CHT_DSP_SSS_POS		16
+
+struct punit_device {
+	char *name;
+	int reg;
+	int sss_pos;
+};
+
+static const struct punit_device punit_device_byt[] = {
+	{ "GFX RENDER",	PWRGT_STATUS,	RENDER_POS },
+	{ "GFX MEDIA",	PWRGT_STATUS,	MEDIA_POS },
+	{ "DISPLAY",	PWRGT_STATUS,	VLV_DISPLAY_POS },
+	{ "VED",	VED_SS_PM0,	SSS_SHIFT },
+	{ "ISP",	ISP_SS_PM0,	SSS_SHIFT },
+	{ "MIO",	MIO_SS_PM,	SSS_SHIFT },
+	{ NULL }
+};
+
+static const struct punit_device punit_device_cht[] = {
+	{ "GFX RENDER",	PWRGT_STATUS,	RENDER_POS },
+	{ "GFX MEDIA",	PWRGT_STATUS,	MEDIA_POS },
+	{ "DISPLAY",	CHT_DSP_SSS,	CHT_DSP_SSS_POS },
+	{ "VED",	VED_SS_PM0,	SSS_SHIFT },
+	{ "ISP",	ISP_SS_PM0,	SSS_SHIFT },
+	{ "MIO",	MIO_SS_PM,	SSS_SHIFT },
+	{ NULL }
+};
+
+static const char * const dstates[] = {"D0", "D0i1", "D0i2", "D0i3"};
+
+static int punit_dev_state_show(struct seq_file *seq_file, void *unused)
+{
+	u32 punit_pwr_status;
+	struct punit_device *punit_devp = seq_file->private;
+	int index;
+	int status;
+
+	seq_puts(seq_file, "\n\nPUNIT NORTH COMPLEX DEVICES :\n");
+	while (punit_devp->name) {
+		status = iosf_mbi_read(PUNIT_PORT, BT_MBI_PMC_READ,
+				       punit_devp->reg,
+				       &punit_pwr_status);
+		if (status) {
+			seq_printf(seq_file, "%9s : Read Failed\n",
+				   punit_devp->name);
+		} else  {
+			index = (punit_pwr_status >> punit_devp->sss_pos) & 3;
+			seq_printf(seq_file, "%9s : %s\n", punit_devp->name,
+				   dstates[index]);
+		}
+		punit_devp++;
+	}
+
+	return 0;
+}
+
+static int punit_dev_state_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, punit_dev_state_show, inode->i_private);
+}
+
+static const struct file_operations punit_dev_state_ops = {
+	.open		= punit_dev_state_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static struct dentry *punit_dbg_file;
+
+static int punit_dbgfs_register(struct punit_device *punit_device)
+{
+	static struct dentry *dev_state;
+
+	punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
+	if (!punit_dbg_file)
+		return -ENXIO;
+
+	dev_state = debugfs_create_file("dev_power_state", S_IFREG | S_IRUGO,
+					punit_dbg_file, punit_device,
+					&punit_dev_state_ops);
+	if (!dev_state) {
+		pr_err("punit_dev_state register failed\n");
+		debugfs_remove(punit_dbg_file);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static void punit_dbgfs_unregister(void)
+{
+	debugfs_remove_recursive(punit_dbg_file);
+}
+
+#define ICPU(model, drv_data) \
+	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT,\
+	  (kernel_ulong_t)&drv_data }
+
+static const struct x86_cpu_id intel_punit_cpu_ids[] = {
+	ICPU(55, punit_device_byt), /* Valleyview, Bay Trail */
+	ICPU(76, punit_device_cht), /* Braswell, Cherry Trail */
+	{}
+};
+
+MODULE_DEVICE_TABLE(x86cpu, intel_punit_cpu_ids);
+
+static int __init punit_atom_debug_init(void)
+{
+	const struct x86_cpu_id *id;
+	int ret;
+
+	id = x86_match_cpu(intel_punit_cpu_ids);
+	if (!id)
+		return -ENODEV;
+
+	ret = punit_dbgfs_register((struct punit_device *)id->driver_data);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static void __exit punit_atom_debug_exit(void)
+{
+	punit_dbgfs_unregister();
+}
+
+module_init(punit_atom_debug_init);
+module_exit(punit_atom_debug_exit);
+
+MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p@intel.com>");
+MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>");
+MODULE_DESCRIPTION("Driver for Punit devices states debugging");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[tip:x86/platform] x86/platform/atom/punit: Add Punit device state debug driver

[tip-bot for Srinivas Pandruvada]

Commit-ID:  956079e081427fe0c929eb284ab7e39f9b8e2023
Gitweb:     http://git.kernel.org/tip/956079e081427fe0c929eb284ab7e39f9b8e2023
Author:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
AuthorDate: Wed, 6 May 2015 12:15:54 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 7 May 2015 11:18:27 +0200

x86/platform/atom/punit: Add Punit device state debug driver

The patch adds a debug driver, which dumps the power states
of all the North complex (NC) devices. This debug interface is
useful to figure out the devices,  which blocks the S0ix
transitions on the platform. This is extremely useful during
enabling PM on customer platforms and derivatives.

This submission is based on the submission from Mahesh Kumar P:

  https://lkml.org/lkml/2014/11/5/367

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Mahesh Kumar P <mahesh.kumar.p@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: pebolle@tiscali.nl
Link: http://lkml.kernel.org/r/1430939754-6900-2-git-send-email-srinivas.pandruvada@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/Kconfig.debug                    |  11 ++
 arch/x86/platform/Makefile                |   1 +
 arch/x86/platform/atom/Makefile           |   1 +
 arch/x86/platform/atom/punit_atom_debug.c | 183 ++++++++++++++++++++++++++++++
 4 files changed, 196 insertions(+)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 72484a6..a5973f8 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -332,4 +332,15 @@ config X86_DEBUG_STATIC_CPU_HAS
 
 	  If unsure, say N.
 
+config PUNIT_ATOM_DEBUG
+	tristate "ATOM Punit debug driver"
+	select DEBUG_FS
+	select IOSF_MBI
+	---help---
+	  This is a debug driver, which gets the power states
+	  of all Punit North Complex devices. The power states of
+	  each device is exposed as part of the debugfs interface.
+	  The current power state can be read from
+	  /sys/kernel/debug/punit_atom/dev_power_state
+
 endmenu
diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
index a62e0be..f1a6c8e 100644
--- a/arch/x86/platform/Makefile
+++ b/arch/x86/platform/Makefile
@@ -1,4 +1,5 @@
 # Platform specific code goes here
+obj-y	+= atom/
 obj-y	+= ce4100/
 obj-y	+= efi/
 obj-y	+= geode/
diff --git a/arch/x86/platform/atom/Makefile b/arch/x86/platform/atom/Makefile
new file mode 100644
index 0000000..0a3a40c
--- /dev/null
+++ b/arch/x86/platform/atom/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PUNIT_ATOM_DEBUG) += punit_atom_debug.o
diff --git a/arch/x86/platform/atom/punit_atom_debug.c b/arch/x86/platform/atom/punit_atom_debug.c
new file mode 100644
index 0000000..5ca8ead
--- /dev/null
+++ b/arch/x86/platform/atom/punit_atom_debug.c
@@ -0,0 +1,183 @@
+/*
+ * Intel SOC Punit device state debug driver
+ * Punit controls power management for North Complex devices (Graphics
+ * blocks, Image Signal Processing, video processing, display, DSP etc.)
+ *
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/io.h>
+#include <asm/cpu_device_id.h>
+#include <asm/iosf_mbi.h>
+
+/* Side band Interface port */
+#define PUNIT_PORT		0x04
+/* Power gate status reg */
+#define PWRGT_STATUS		0x61
+/* Subsystem config/status Video processor */
+#define VED_SS_PM0		0x32
+/* Subsystem config/status ISP (Image Signal Processor) */
+#define ISP_SS_PM0		0x39
+/* Subsystem config/status Input/output controller */
+#define MIO_SS_PM		0x3B
+/* Shift bits for getting status for video, isp and i/o */
+#define SSS_SHIFT		24
+/* Shift bits for getting status for graphics rendering */
+#define RENDER_POS		0
+/* Shift bits for getting status for media control */
+#define MEDIA_POS		2
+/* Shift bits for getting status for Valley View/Baytrail display */
+#define VLV_DISPLAY_POS		6
+/* Subsystem config/status display for Cherry Trail SOC */
+#define CHT_DSP_SSS		0x36
+/* Shift bits for getting status for display */
+#define CHT_DSP_SSS_POS		16
+
+struct punit_device {
+	char *name;
+	int reg;
+	int sss_pos;
+};
+
+static const struct punit_device punit_device_byt[] = {
+	{ "GFX RENDER",	PWRGT_STATUS,	RENDER_POS },
+	{ "GFX MEDIA",	PWRGT_STATUS,	MEDIA_POS },
+	{ "DISPLAY",	PWRGT_STATUS,	VLV_DISPLAY_POS },
+	{ "VED",	VED_SS_PM0,	SSS_SHIFT },
+	{ "ISP",	ISP_SS_PM0,	SSS_SHIFT },
+	{ "MIO",	MIO_SS_PM,	SSS_SHIFT },
+	{ NULL }
+};
+
+static const struct punit_device punit_device_cht[] = {
+	{ "GFX RENDER",	PWRGT_STATUS,	RENDER_POS },
+	{ "GFX MEDIA",	PWRGT_STATUS,	MEDIA_POS },
+	{ "DISPLAY",	CHT_DSP_SSS,	CHT_DSP_SSS_POS },
+	{ "VED",	VED_SS_PM0,	SSS_SHIFT },
+	{ "ISP",	ISP_SS_PM0,	SSS_SHIFT },
+	{ "MIO",	MIO_SS_PM,	SSS_SHIFT },
+	{ NULL }
+};
+
+static const char * const dstates[] = {"D0", "D0i1", "D0i2", "D0i3"};
+
+static int punit_dev_state_show(struct seq_file *seq_file, void *unused)
+{
+	u32 punit_pwr_status;
+	struct punit_device *punit_devp = seq_file->private;
+	int index;
+	int status;
+
+	seq_puts(seq_file, "\n\nPUNIT NORTH COMPLEX DEVICES :\n");
+	while (punit_devp->name) {
+		status = iosf_mbi_read(PUNIT_PORT, BT_MBI_PMC_READ,
+				       punit_devp->reg,
+				       &punit_pwr_status);
+		if (status) {
+			seq_printf(seq_file, "%9s : Read Failed\n",
+				   punit_devp->name);
+		} else  {
+			index = (punit_pwr_status >> punit_devp->sss_pos) & 3;
+			seq_printf(seq_file, "%9s : %s\n", punit_devp->name,
+				   dstates[index]);
+		}
+		punit_devp++;
+	}
+
+	return 0;
+}
+
+static int punit_dev_state_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, punit_dev_state_show, inode->i_private);
+}
+
+static const struct file_operations punit_dev_state_ops = {
+	.open		= punit_dev_state_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static struct dentry *punit_dbg_file;
+
+static int punit_dbgfs_register(struct punit_device *punit_device)
+{
+	static struct dentry *dev_state;
+
+	punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
+	if (!punit_dbg_file)
+		return -ENXIO;
+
+	dev_state = debugfs_create_file("dev_power_state", S_IFREG | S_IRUGO,
+					punit_dbg_file, punit_device,
+					&punit_dev_state_ops);
+	if (!dev_state) {
+		pr_err("punit_dev_state register failed\n");
+		debugfs_remove(punit_dbg_file);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static void punit_dbgfs_unregister(void)
+{
+	debugfs_remove_recursive(punit_dbg_file);
+}
+
+#define ICPU(model, drv_data) \
+	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT,\
+	  (kernel_ulong_t)&drv_data }
+
+static const struct x86_cpu_id intel_punit_cpu_ids[] = {
+	ICPU(55, punit_device_byt), /* Valleyview, Bay Trail */
+	ICPU(76, punit_device_cht), /* Braswell, Cherry Trail */
+	{}
+};
+
+MODULE_DEVICE_TABLE(x86cpu, intel_punit_cpu_ids);
+
+static int __init punit_atom_debug_init(void)
+{
+	const struct x86_cpu_id *id;
+	int ret;
+
+	id = x86_match_cpu(intel_punit_cpu_ids);
+	if (!id)
+		return -ENODEV;
+
+	ret = punit_dbgfs_register((struct punit_device *)id->driver_data);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static void __exit punit_atom_debug_exit(void)
+{
+	punit_dbgfs_unregister();
+}
+
+module_init(punit_atom_debug_init);
+module_exit(punit_atom_debug_exit);
+
+MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p@intel.com>");
+MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>");
+MODULE_DESCRIPTION("Driver for Punit devices states debugging");
+MODULE_LICENSE("GPL v2");

Re: [PATCH v4] x86: punit_atom: punit device state debug driver

[Andy Shevchenko]

On Wed, May 6, 2015 at 10:15 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> The patch adds a debug driver, which dumps the power states
> of all the North complex (NC) devices. This debug interface is
> useful to figure out the devices,  which blocks the S0ix
> transitions on the platform. This is extremely useful during
> enabling PM on customer platforms and derivatives.
>
> This submission is based on the submission from
> Kumar P, Mahesh <mahesh.kumar.p.intel.com>.
> https://lkml.org/lkml/2014/11/5/367
> The changes done on top of the above submission:
> - Dropped changes to config for PMC_ATOM, as PMC_ATOM
> is not just a debug driver as suggested by the change. It has
> additional power off interface also.
> - Instead of just using nc ("North complex") use punit_..
> similar to south complex PMC.
> - Removed pmc_config structure,  as we don't need to predefine
> number of register, we want to dump. This way new register
> can be added without changing NC_NUM_DEVICES.
> - prefixed function with punit_
> - The debugfs directory will be punit_atom, which is NC equivalent
> of pmc_atom, which we already exposed by pmc_atom driver.
> - Added explanation for register and shift defines
> - Will not create debugfs if the cpuid doesn't match
> - Formatting changes to match compliance to coding convention
> - Address review comments

Sorry for late review. Hope my following comments will be addressed.

>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  arch/x86/Kconfig.debug                    |  11 ++
>  arch/x86/platform/Makefile                |   1 +
>  arch/x86/platform/atom/Makefile           |   1 +
>  arch/x86/platform/atom/punit_atom_debug.c | 183 ++++++++++++++++++++++++++++++

Should be pmc_atom.c moved here as well?

>  4 files changed, 196 insertions(+)
>  create mode 100644 arch/x86/platform/atom/Makefile
>  create mode 100644 arch/x86/platform/atom/punit_atom_debug.c
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 20028da..b525d86 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -336,4 +336,15 @@ config X86_DEBUG_STATIC_CPU_HAS
>
>           If unsure, say N.
>
> +config PUNIT_ATOM_DEBUG
> +       tristate "ATOM Punit debug driver"

"Intel Atom Punit debug driver"

> +       select DEBUG_FS
> +       select IOSF_MBI
> +       ---help---
> +         This is a debug driver, which gets the power states
> +         of all Punit North Complex devices. The power states of
> +         each device is exposed as part of the debugfs interface.
> +         The current power state can be read from
> +         /sys/kernel/debug/punit_atom/dev_power_state

I hope you may use wider lines here.

> +
>  endmenu
> diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
> index a62e0be..f1a6c8e 100644
> --- a/arch/x86/platform/Makefile
> +++ b/arch/x86/platform/Makefile
> @@ -1,4 +1,5 @@
>  # Platform specific code goes here
> +obj-y  += atom/
>  obj-y  += ce4100/
>  obj-y  += efi/
>  obj-y  += geode/
> diff --git a/arch/x86/platform/atom/Makefile b/arch/x86/platform/atom/Makefile
> new file mode 100644
> index 0000000..0a3a40c
> --- /dev/null
> +++ b/arch/x86/platform/atom/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_PUNIT_ATOM_DEBUG) += punit_atom_debug.o
> diff --git a/arch/x86/platform/atom/punit_atom_debug.c b/arch/x86/platform/atom/punit_atom_debug.c
> new file mode 100644
> index 0000000..5ca8ead
> --- /dev/null
> +++ b/arch/x86/platform/atom/punit_atom_debug.c
> @@ -0,0 +1,183 @@
> +/*
> + * Intel SOC Punit device state debug driver

SoC

And perhaps empty line to the next paragraph.

> + * Punit controls power management for North Complex devices (Graphics
> + * blocks, Image Signal Processing, video processing, display, DSP etc.)

* Punit controls power management for North Complex devices: Graphics
* blocks, Image Signal Processing, video processing, display, DSP, etc.

> + *
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * 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.

Do we need the above paragraph at all?

> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/io.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/iosf_mbi.h>
> +
> +/* Side band Interface port */
> +#define PUNIT_PORT             0x04

+ empty line ?

> +/* Power gate status reg */

reg -> register

> +#define PWRGT_STATUS           0x61

Should be sorted regarding to the offset?

> +/* Subsystem config/status Video processor */
> +#define VED_SS_PM0             0x32
> +/* Subsystem config/status ISP (Image Signal Processor) */
> +#define ISP_SS_PM0             0x39
> +/* Subsystem config/status Input/output controller */
> +#define MIO_SS_PM              0x3B

+ empty line ?

> +/* Shift bits for getting status for video, isp and i/o */
> +#define SSS_SHIFT              24


> +/* Shift bits for getting status for graphics rendering */
> +#define RENDER_POS             0
> +/* Shift bits for getting status for media control */
> +#define MEDIA_POS              2
> +/* Shift bits for getting status for Valley View/Baytrail display */

I would prefer to see BYT prefix and BayTrail in the comment.

> +#define VLV_DISPLAY_POS                6

Should all shift bits be sorted by number?

> +/* Subsystem config/status display for Cherry Trail SOC */
> +#define CHT_DSP_SSS            0x36

This is part of  above group.

I think better naming scheme is something like:

PUNIT_SS_…[_MACHINE], where MACHINE might be CHT, BYT, and so on if needed.

Here I can't see necessity of suffix at all. At least right now in the
current implementation.

> +/* Shift bits for getting status for display */
> +#define CHT_DSP_SSS_POS                16
> +
> +struct punit_device {

punit_island if to be correct, isn't it?

> +       char *name;
> +       int reg;

u32 ? Since IOSF interface take it as u32.

> +       int sss_pos;

unsigned int shift; ?

> +};
> +
> +static const struct punit_device punit_device_byt[] = {
> +       { "GFX RENDER", PWRGT_STATUS,   RENDER_POS },
> +       { "GFX MEDIA",  PWRGT_STATUS,   MEDIA_POS },
> +       { "DISPLAY",    PWRGT_STATUS,   VLV_DISPLAY_POS },
> +       { "VED",        VED_SS_PM0,     SSS_SHIFT },
> +       { "ISP",        ISP_SS_PM0,     SSS_SHIFT },
> +       { "MIO",        MIO_SS_PM,      SSS_SHIFT },
> +       { NULL }
> +};
> +
> +static const struct punit_device punit_device_cht[] = {
> +       { "GFX RENDER", PWRGT_STATUS,   RENDER_POS },
> +       { "GFX MEDIA",  PWRGT_STATUS,   MEDIA_POS },
> +       { "DISPLAY",    CHT_DSP_SSS,    CHT_DSP_SSS_POS },
> +       { "VED",        VED_SS_PM0,     SSS_SHIFT },
> +       { "ISP",        ISP_SS_PM0,     SSS_SHIFT },
> +       { "MIO",        MIO_SS_PM,      SSS_SHIFT },
> +       { NULL }
> +};
> +
> +static const char * const dstates[] = {"D0", "D0i1", "D0i2", "D0i3"};
> +
> +static int punit_dev_state_show(struct seq_file *seq_file, void *unused)
> +{
> +       u32 punit_pwr_status;
> +       struct punit_device *punit_devp = seq_file->private;

punit_devp -> island (including change punit_device -> punit_island).

> +       int index;
> +       int status;
> +
> +       seq_puts(seq_file, "\n\nPUNIT NORTH COMPLEX DEVICES :\n");

Maybe
"Punit North Complex Islands:\n" ?

> +       while (punit_devp->name) {
> +               status = iosf_mbi_read(PUNIT_PORT, BT_MBI_PMC_READ,
> +                                      punit_devp->reg,
> +                                      &punit_pwr_status);
> +               if (status) {
> +                       seq_printf(seq_file, "%9s : Read Failed\n",
> +                                  punit_devp->name);
> +               } else  {

Indentation?

> +                       index = (punit_pwr_status >> punit_devp->sss_pos) & 3;

3 is a magic number.

> +                       seq_printf(seq_file, "%9s : %s\n", punit_devp->name,
> +                                  dstates[index]);
> +               }
> +               punit_devp++;
> +       }
> +
> +       return 0;
> +}
> +
> +static int punit_dev_state_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, punit_dev_state_show, inode->i_private);
> +}
> +
> +static const struct file_operations punit_dev_state_ops = {
> +       .open           = punit_dev_state_open,
> +       .read           = seq_read,
> +       .llseek         = seq_lseek,
> +       .release        = single_release,
> +};
> +
> +static struct dentry *punit_dbg_file;

Perhaps
static struct dentry *punit_debugfs;
since it's a folder.

> +
> +static int punit_dbgfs_register(struct punit_device *punit_device)
> +{
> +       static struct dentry *dev_state;

Should it be static?

dev_state -> state ?

> +
> +       punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
> +       if (!punit_dbg_file)
> +               return -ENXIO;
> +
> +       dev_state = debugfs_create_file("dev_power_state", S_IFREG | S_IRUGO,

dev_power_state -> state ?

> +                                       punit_dbg_file, punit_device,
> +                                       &punit_dev_state_ops);
> +       if (!dev_state) {
> +               pr_err("punit_dev_state register failed\n");
> +               debugfs_remove(punit_dbg_file);
> +               return -ENXIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static void punit_dbgfs_unregister(void)
> +{
> +       debugfs_remove_recursive(punit_dbg_file);
> +}
> +
> +#define ICPU(model, drv_data) \
> +       { X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT,\
> +         (kernel_ulong_t)&drv_data }
> +
> +static const struct x86_cpu_id intel_punit_cpu_ids[] = {
> +       ICPU(55, punit_device_byt), /* Valleyview, Bay Trail */
> +       ICPU(76, punit_device_cht), /* Braswell, Cherry Trail */
> +       {}
> +};
> +
> +MODULE_DEVICE_TABLE(x86cpu, intel_punit_cpu_ids);
> +
> +static int __init punit_atom_debug_init(void)
> +{
> +       const struct x86_cpu_id *id;
> +       int ret;
> +
> +       id = x86_match_cpu(intel_punit_cpu_ids);
> +       if (!id)
> +               return -ENODEV;
> +
> +       ret = punit_dbgfs_register((struct punit_device *)id->driver_data);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static void __exit punit_atom_debug_exit(void)
> +{
> +       punit_dbgfs_unregister();
> +}
> +
> +module_init(punit_atom_debug_init);
> +module_exit(punit_atom_debug_exit);
> +
> +MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p@intel.com>");
> +MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>");
> +MODULE_DESCRIPTION("Driver for Punit devices states debugging");
> +MODULE_LICENSE("GPL v2");

Just noticed it's already in the tip tree.
Though, please, comment my mail I can do patch by myself if there is
no objections.

-- 
With Best Regards,
Andy Shevchenko