Re: [PATCH v1] x86: punit_atom: punit device state debug driver
* Srinivas Pandruvada wrote: > > > 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? So to keep it short you could name it arch/x86/platform/atom/? 'Common' is implied, and I think everyone associates Intel from 'Atom', in the platform context. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
* Srinivas Pandruvada srinivas.pandruv...@linux.intel.com wrote: 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? So to keep it short you could name it arch/x86/platform/atom/? 'Common' is implied, and I think everyone associates Intel from 'Atom', in the platform context. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
Hi Ingo and Thomas,
One question inside on location.
On Sat, 2015-05-02 at 08:00 +0200, Ingo Molnar wrote:
> * Srinivas Pandruvada 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 .
> > 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_POS6
> > +#define CHT_DSP_SSS0x36 /* Subsystem config/status DSP */
> > +#define CHT_DSP_SSS_POS16
>
> 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,
> > +_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 ");
> > +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
Hi Ingo and Thomas,
One question inside on location.
On Sat, 2015-05-02 at 08:00 +0200, Ingo Molnar wrote:
* Srinivas Pandruvada srinivas.pandruv...@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_POS6
+#define CHT_DSP_SSS0x36 /* Subsystem config/status DSP */
+#define CHT_DSP_SSS_POS16
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
On Sat, 2015-05-02 at 08:00 +0200, Ingo Molnar wrote:
Thanks for the review
> * Srinivas Pandruvada 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 .
> > https://lkml.org/lkml/2014/11/5/367
>
> This sentence does not parse. Did you mean 'is a complete rewrite'?
Updated the commit message, Meant "updated the submission based on the
previous submission"
>
> > +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'?
Changed to device.
>
> Also:
>
> s/devices.The
>/devices. The
Done.
>
> > +
> > 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/.
Moved to arch/x86/platform/intel-mid
>
> > +/*
> > + * 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.
Done
>
> > +#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_POS6
> > +#define CHT_DSP_SSS0x36 /* Subsystem config/status DSP */
> > +#define CHT_DSP_SSS_POS16
>
> All the magic constants need some explanation for humans, not just
> some.
Added for missing defines
>
> > +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:
>
Aligned.
> > +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,
> > +_dev_state_ops);
>
> So why isn't something that is declaradly a power state dump, called
> "power_state" or "dev_power_state"?
>
Changed to dev_power_state
> > +MODULE_AUTHOR("Kumar P, Mahesh ");
> > +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.
Added myself .
Thanks,
Srinivas
>
> Thanks,
>
> Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
On Sat, 2015-05-02 at 08:00 +0200, Ingo Molnar wrote:
Thanks for the review
* Srinivas Pandruvada srinivas.pandruv...@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'?
Updated the commit message, Meant updated the submission based on the
previous submission
+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'?
Changed to device.
Also:
s/devices.The
/devices. The
Done.
+
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/.
Moved to arch/x86/platform/intel-mid
+/*
+ * 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.
Done
+#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_POS6
+#define CHT_DSP_SSS0x36 /* Subsystem config/status DSP */
+#define CHT_DSP_SSS_POS16
All the magic constants need some explanation for humans, not just
some.
Added for missing defines
+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:
Aligned.
+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?
Changed to 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.
Added myself .
Thanks,
Srinivas
Thanks,
Ingo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
On Sat, May 02, 2015 at 08:00:51AM +0200, Ingo Molnar wrote: > 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. Oh boy, I propose a vararg macro called MODULE_GANG(...) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
* Srinivas Pandruvada 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 .
> 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/.
> +/*
> + * 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_PM0x3B /* Subsystem config/status MIO */
> +#define SSS_SHIFT24
> +#define RENDER_POS 0
> +#define MEDIA_POS2
> +#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,
> + _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 ");
> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
On Sat, May 02, 2015 at 08:00:51AM +0200, Ingo Molnar wrote: 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. Oh boy, I propose a vararg macro called MODULE_GANG(...) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] x86: punit_atom: punit device state debug driver
* Srinivas Pandruvada srinivas.pandruv...@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/.
+/*
+ * 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_PM0x3B /* Subsystem config/status MIO */
+#define SSS_SHIFT24
+#define RENDER_POS 0
+#define MEDIA_POS2
+#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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v1] x86: punit_atom: 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 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 .
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
---
arch/x86/Kconfig.debug | 9 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/punit_atom_debug.c | 170 +
3 files changed, 180 insertions(+)
create mode 100644 arch/x86/kernel/punit_atom_debug.c
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 20028da..9b85dfe 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -336,4 +336,13 @@ config X86_DEBUG_STATIC_CPU_HAS
If unsure, say N.
+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.
+
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
new file mode 100644
index 000..6e7d4e5
--- /dev/null
+++ b/arch/x86/kernel/punit_atom_debug.c
@@ -0,0 +1,170 @@
+/*
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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_POS6
+#define CHT_DSP_SSS0x36 /* Subsystem config/status DSP */
+#define CHT_DSP_SSS_POS16
+
+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");
+
[PATCH v1] x86: punit_atom: punit device state debug driver
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 | 9 ++ arch/x86/kernel/Makefile | 1 + arch/x86/kernel/punit_atom_debug.c | 170 + 3 files changed, 180 insertions(+) create mode 100644 arch/x86/kernel/punit_atom_debug.c -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v1] x86: punit_atom: 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 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.pandruv...@linux.intel.com
---
arch/x86/Kconfig.debug | 9 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/punit_atom_debug.c | 170 +
3 files changed, 180 insertions(+)
create mode 100644 arch/x86/kernel/punit_atom_debug.c
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 20028da..9b85dfe 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -336,4 +336,13 @@ config X86_DEBUG_STATIC_CPU_HAS
If unsure, say N.
+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.
+
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
new file mode 100644
index 000..6e7d4e5
--- /dev/null
+++ b/arch/x86/kernel/punit_atom_debug.c
@@ -0,0 +1,170 @@
+/*
+ * 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_POS6
+#define CHT_DSP_SSS0x36 /* Subsystem config/status DSP */
+#define CHT_DSP_SSS_POS16
+
+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)
+{
+
[PATCH v1] x86: punit_atom: punit device state debug driver
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 | 9 ++ arch/x86/kernel/Makefile | 1 + arch/x86/kernel/punit_atom_debug.c | 170 + 3 files changed, 180 insertions(+) create mode 100644 arch/x86/kernel/punit_atom_debug.c -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
