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

[PATCH] 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 NC IPs which blocks the S0ix
transitions on the platform. This is extremely useful during
enabling PM on customer platforms and derivatives.

This submission not a complete rewrite from the original
submission from Kumar P, Mahesh <mahesh.kumar.p.intel.com>.
https://lkml.org/lkml/2014/11/5/367
The changes are:
- 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. All the interfaces exposed
by this driver are provided by PUNIT.
- 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.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Kumar P Mahesh <mahesh.kumar.p@intel.com>
Reviewed-by: Li, Aubrey <aubrey.li@linux.intel.com>
---
 arch/x86/Kconfig             |  10 +++
 arch/x86/kernel/Makefile     |   1 +
 arch/x86/kernel/punit_atom.c | 167 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 arch/x86/kernel/punit_atom.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b7d31ca..b8e6de6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2578,6 +2578,16 @@ config PMC_ATOM
 	def_bool y
         depends on PCI
 
+config PUNIT_ATOM
+	bool "ATOM Punit debug driver"
+	def_bool n
+	depends on 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 IP is exposed as part of the debugfs interface.
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index cdb1b70..8ee5fb3 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o
 obj-$(CONFIG_TRACING)			+= tracepoint.o
 obj-$(CONFIG_IOSF_MBI)			+= iosf_mbi.o
 obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
+obj-$(CONFIG_PUNIT_ATOM)		+= punit_atom.o
 
 ###
 # 64 bit specific files
diff --git a/arch/x86/kernel/punit_atom.c b/arch/x86/kernel/punit_atom.c
new file mode 100644
index 0000000..f05d603
--- /dev/null
+++ b/arch/x86/kernel/punit_atom.c
@@ -0,0 +1,167 @@
+/*
+ * Intel SOC Punit device state debug driver
+ * 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>
+
+#define PUNIT_PORT		0x04
+#define PWRGT_STATUS		0x61 /* Power gate status reg */
+#define VED_SS_PM0		0x32 /* Subsystem config/status Video */
+#define ISP_SS_PM0		0x39 /* Subsystem config/status ISP */
+#define MIO_SS_PM		0x3B /* Subsystem config/status MIO */
+#define SSS_SHIFT		24
+#define RENDER_POS		0
+#define MEDIA_POS		2
+#define VLV_DISPLAY_POS		6
+#define CHT_DSP_SSS		0x36 /* Subsystem config/status DSP */
+#define CHT_DSP_SSS_POS		16
+
+struct punit_device {
+	char *name;
+	int reg;
+	int sss_pos;
+};
+
+static struct punit_device *punit_device;
+
+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 },
+	{ "DSP", 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;
+	int index;
+	int status;
+
+	seq_puts(seq_file, "\n\nPUNIT NORTH COMPLEX DEVICES :\n");
+	while (punit_device->name) {
+		status = iosf_mbi_read(PUNIT_PORT, BT_MBI_PMC_READ,
+				       punit_device->reg,
+				       &punit_pwr_status);
+		if (status)
+			seq_printf(seq_file, "%9s : Read Failed\n",
+				   punit_device->name);
+		else  {
+			index = (punit_pwr_status >> punit_device->sss_pos) & 3;
+			seq_printf(seq_file, "%9s : %s\n", punit_device->name,
+				   dstates[index]);
+		}
+		punit_device++;
+	}
+
+	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(void)
+{
+	punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
+	if (!punit_dbg_file)
+		return -ENOMEM;
+
+	punit_dbg_file = debugfs_create_file("dev_state", S_IFREG | S_IRUGO,
+					     punit_dbg_file, NULL,
+					     &punit_dev_state_ops);
+	if (!punit_dbg_file) {
+		pr_err("punit_dev_state register failed\n");
+		return -ENODEV;
+	}
+	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(0x4c, punit_device_cht),
+	ICPU(0x37, punit_device_byt),
+	{}
+};
+
+MODULE_DEVICE_TABLE(x86cpu, intel_punit_cpu_ids);
+
+static int __init punit_atom_init(void)
+{
+	const struct x86_cpu_id *id;
+	int ret;
+
+	id = x86_match_cpu(intel_punit_cpu_ids);
+	if (!id)
+		return -ENODEV;
+
+	punit_device = (struct punit_device *)id->driver_data;
+
+	ret = punit_dbgfs_register();
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static void __exit punit_atom_exit(void)
+{
+	punit_dbgfs_unregister();
+}
+
+module_init(punit_atom_init);
+module_exit(punit_atom_exit);
+
+MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p.intel.com>");
+MODULE_DESCRIPTION("Driver for Punit devices states debugging");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[Paul Bolle]

On Fri, 2015-04-24 at 14:42 -0700, Srinivas Pandruvada wrote:
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
>  
> +config PUNIT_ATOM
> +	bool "ATOM Punit debug driver"
> +	def_bool n

The kconfig tools will do what you mean here. But just
	default n

is neater. And, moreover, even that is probably not needed, as n is the
default anyway.

> +	depends on 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 IP is exposed as part of the debugfs interface.

(This menu will show up in menuconfig's main menu. Shouldn't this be put
in some debug related menu?)

> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile

> +obj-$(CONFIG_PUNIT_ATOM)		+= punit_atom.o

PUNIT_ATOM is a bool symbol, so punit_atom.o will never be part of a
module, correct?

> --- /dev/null
> +++ b/arch/x86/kernel/punit_atom.c

> +#include <linux/module.h>

Is this include needed (except for the stuff that will be preprocessed
away, see below)?

> +MODULE_DEVICE_TABLE(x86cpu, intel_punit_cpu_ids);

MODULE_DEVICE_TABLE will be preprocessed away for built-in code,
according to include/linux/module.h.

> +static void __exit punit_atom_exit(void)
> +{
> +	punit_dbgfs_unregister();
> +}
> +
> +module_init(punit_atom_init);

According to include/linux/init.h this is equal to
	device_initcall(punit_atom_init);

for built-in code.

> +module_exit(punit_atom_exit);

Built-in only code will never call that function.

> +MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p.intel.com>");

(<mahesh.kumar.p@intel.com>?)

> +MODULE_DESCRIPTION("Driver for Punit devices states debugging");
> +MODULE_LICENSE("GPL v2");

And these three macros will be effectively preprocessed away for
built-in only code.

But, on the other hand, changing PUNIT_ATOM to tristate allows it to be
built as a module (I just did that). So perhaps that was your intention?


Paul Bolle

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

[Srinivas Pandruvada]

On Sat, 2015-04-25 at 11:43 +0200, Paul Bolle wrote:
> On Fri, 2015-04-24 at 14:42 -0700, Srinivas Pandruvada wrote:
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> >  
> > +config PUNIT_ATOM
> > +	bool "ATOM Punit debug driver"
> > +	def_bool n
> 
> The kconfig tools will do what you mean here. But just
> 	default n
> 
> is neater. And, moreover, even that is probably not needed, as n is the
> default anyway.
> 
> > +	depends on 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 IP is exposed as part of the debugfs interface.
> 
> (This menu will show up in menuconfig's main menu. Shouldn't this be put
> in some debug related menu?)
The initial intention is to make a debug driver only, but there is
possibility that some register setting be allowed from user space for PM
decision. So leaving open this possibility to keep out of debug menus.

<snip>

> And these three macros will be effectively preprocessed away for
> built-in only code.
> 
> But, on the other hand, changing PUNIT_ATOM to tristate allows it to be
> built as a module (I just did that). So perhaps that was your intention?
Making tristate makes sense. So I will send an update with this.

Thanks,
Srinivas
> 
> 
> Paul Bolle
>