Messages in this thread | | | Subject | Re: [PATCH v1] x86: punit_atom: punit device state debug driver | From | Srinivas Pandruvada <> | Date | Tue, 05 May 2015 10:29:04 -0700 |
| |
Hi Ingo and Thomas, One question inside on location. On Sat, 2015-05-02 at 08:00 +0200, Ingo Molnar wrote: > * 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 NC IPs which blocks the S0ix > > transitions on the platform. This is extremely useful during > > enabling PM on customer platforms and derivatives. > > Looks useful. > > > 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 > > This sentence does not parse. Did you mean 'is a complete rewrite'? > > > +config PUNIT_ATOM_DEBUG > > + tristate "ATOM Punit debug driver" > > + 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. > > What is an 'IP'? > > Also: > > s/devices.The > /devices. The > > > + > > endmenu > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > > index cdb1b70..2ecbd55 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_DEBUG) += punit_atom_debug.o > > > > ### > > # 64 bit specific files > > diff --git a/arch/x86/kernel/punit_atom_debug.c b/arch/x86/kernel/punit_atom_debug.c > > Please put this somewhere suitable in arch/x86/platform/. I moved this to arch/x86/platform/intel-mid, but punit is much more part of common core atom soc platform. This can have both mobile and PC like platform. What about creating arch/x86/platform/intel-atom-common, and add there?
Thanks, Srinivas > > > +/* > > + * Intel SOC Punit device state debug driver > > + * Copyright (c) 2015, Intel Corporation. > > Would be nice to add a blurb about what 'Intel SOC Punit' systems are. > There doesn't seem to be anything in the kernel source about it, so > you have a golden opportunity here. > > > +#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 > > All the magic constants need some explanation for humans, not just > some. > > > +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} > > +}; > > Please align such initialization tables vertically, like you did here: > > > +static const struct file_operations punit_dev_state_ops = { > > + .open = punit_dev_state_open, > > + .read = seq_read, > > + .llseek = seq_lseek, > > + .release = single_release, > > +}; > > > > + punit_dbg_file = debugfs_create_dir("punit_atom", NULL); > > + if (!punit_dbg_file) > > + return -ENXIO; > > + > > + dev_state = debugfs_create_file("dev_state", S_IFREG | S_IRUGO, > > + punit_dbg_file, NULL, > > + &punit_dev_state_ops); > > So why isn't something that is declaradly a power state dump, called > "power_state" or "dev_power_state"? > > > +MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p.intel.com>"); > > +MODULE_DESCRIPTION("Driver for Punit devices states debugging"); > > +MODULE_LICENSE("GPL v2"); > > Btw., you can have multiple MODULE_AUTHOR() lines, so you can credit > yourself as well. Take a look at drivers/net/wireless/at76c50x-usb.c's > MODULE_AUTHOR() for grins. > > Thanks, > > Ingo
| |