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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxx>");
> +MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>");
> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/