Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Mathieu Poirier 
On 6 April 2017 at 07:30, Leo Yan  wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> description for related info in "Part H: External Debug".
>
> Chapter H7 "The Sample-based Profiling Extension" introduces several
> sampling registers, e.g. we can check program counter value with
> combined CPU exception level, secure state, etc. So this is helpful for
> analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> loop with IRQ disabled. In this case the CPU cannot switch context and
> handle any interrupt (including IPIs), as the result it cannot handle
> SMP call for stack dump.
>
> This patch is to enable coresight debug module, so firstly this driver
> is to bind apb clock for debug module and this is to ensure the debug
> module can be accessed from program or external debugger. And the driver
> uses sample-based registers for debug purpose, e.g. when system triggers
> panic, the driver will dump program counter and combined context
> registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> quickly get to know CPU secure state, exception level, etc.
>
> Some of the debug module registers are located in CPU power domain, so
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
>
> Signed-off-by: Leo Yan 
> ---
>  drivers/hwtracing/coresight/Kconfig   |  14 +
>  drivers/hwtracing/coresight/Makefile  |   1 +
>  drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 
> ++
>  3 files changed, 682 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig 
> b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
>   logging useful software events or data coming from various entities
>   in the system, possibly running different OSs
>
> +config CORESIGHT_CPU_DEBUG
> +   tristate "CoreSight CPU Debug driver"
> +   depends on ARM || ARM64
> +   depends on DEBUG_FS
> +   help
> + This driver provides support for coresight debugging module. This
> + is primarily used to dump sample-based profiling registers when
> + system triggers panic, the driver will parse context registers so
> + can quickly get to know program counter (PC), secure state,
> + exception level, etc. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile 
> b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> coresight-etm4x-sysfs.o
>  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
>  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c 
> b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> + *
> + * Author: Leo Yan 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this progr

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Mathieu Poirier 
On 19 April 2017 at 09:30, Leo Yan  wrote:
> On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:
>
> [...]
>
>> >> > +static bool debug_enable;
>> >> > +module_param_named(enable, debug_enable, bool, 0600);
>> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
>> >> > +"(default is 0, which means is disabled by default)");
>> >>
>> >> For this driver we have a debugFS interface so I question the validity of 
>> >> a
>> >> kernel module parameter.  Other than adding complexity to the code it 
>> >> offers no
>> >> real added value.  If a user is to insmod a module, it is just as easy to 
>> >> switch
>> >> on the functionality using debugFS in a second step.
>> >
>> > This module parameter can be used for kernel command line, so
>> > it's useful when user wants to dynamically turn on/off the
>> > functionality at boot up time.
>> >
>> > Does this make sense for you? Removing this parameter is okay for
>> > me, but this means users need to decide if use it by Kernel config
>> > with static building in. This is a bit contradictory with before's
>> > discussion.
>>
>> My hope was to use the kernel command line and the debugFS interface,
>> avoiding the module parameter.  Look at what the baycom_par and
>> blacklist drivers are doing with the "__setup()" function and see if
>> we can void a module parameter.  If not then let it be, unless someone
>> else has a better idea.
>>
>> [1]. drivers/net/hamradio/baycom_par.c
>> [2]. drivers/s390/cio/blacklist.c
>
> This driver supports module mode. So we can choose to use either module
> parameter or __setup(). But as described in the file
> Documentation/admin-guide/kernel-parameters.rst, the module parameter
> is more flexible to be used at boot time or insmod the module:
>
> "Parameters for modules which are built into the kernel need to be
> specified on the kernel command line.  modprobe looks through the
> kernel command line (/proc/cmdline) and collects module parameters
> when it loads a module, so the kernel command line can be used for
> loadable modules too."
>
> __setup() cannot support module mode, and when use __setup(), we need
> register one callback function for it. The callback function is
> friendly to parse complex parameters rather than module parameter.
> but it's not necessary for this case.

__setup() definitely supports module - the baycom driver is a good
example of that.

But as you pointed out kernel-parameters.rst is pretty clear on how to
proceed.  As such disregard my comment and proceed with a module
parameter.

>
> So I'm bias to use module parameter :)
>
> Thanks,
> Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Leo Yan 
On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:

[...]

> >> > +static bool debug_enable;
> >> > +module_param_named(enable, debug_enable, bool, 0600);
> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> >> > +"(default is 0, which means is disabled by default)");
> >>
> >> For this driver we have a debugFS interface so I question the validity of a
> >> kernel module parameter.  Other than adding complexity to the code it 
> >> offers no
> >> real added value.  If a user is to insmod a module, it is just as easy to 
> >> switch
> >> on the functionality using debugFS in a second step.
> >
> > This module parameter can be used for kernel command line, so
> > it's useful when user wants to dynamically turn on/off the
> > functionality at boot up time.
> >
> > Does this make sense for you? Removing this parameter is okay for
> > me, but this means users need to decide if use it by Kernel config
> > with static building in. This is a bit contradictory with before's
> > discussion.
> 
> My hope was to use the kernel command line and the debugFS interface,
> avoiding the module parameter.  Look at what the baycom_par and
> blacklist drivers are doing with the "__setup()" function and see if
> we can void a module parameter.  If not then let it be, unless someone
> else has a better idea.
> 
> [1]. drivers/net/hamradio/baycom_par.c
> [2]. drivers/s390/cio/blacklist.c

This driver supports module mode. So we can choose to use either module
parameter or __setup(). But as described in the file
Documentation/admin-guide/kernel-parameters.rst, the module parameter
is more flexible to be used at boot time or insmod the module:

"Parameters for modules which are built into the kernel need to be
specified on the kernel command line.  modprobe looks through the 
kernel command line (/proc/cmdline) and collects module parameters
when it loads a module, so the kernel command line can be used for 
loadable modules too."

__setup() cannot support module mode, and when use __setup(), we need
register one callback function for it. The callback function is
friendly to parse complex parameters rather than module parameter.
but it's not necessary for this case.

So I'm bias to use module parameter :)

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Leo Yan 
On Wed, Apr 19, 2017 at 03:32:39PM +0100, Suzuki K Poulose wrote:

[...]

> >>>+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> >>>+{
> >>>+  void __iomem *base;
> >>>+  struct device *dev = &adev->dev;
> >>>+  struct debug_drvdata *drvdata;
> >>>+  struct resource *res = &adev->res;
> >>>+  struct device_node *np = adev->dev.of_node;
> >>>+  int ret;
> >>>+
> >>>+  drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >>>+  if (!drvdata)
> >>>+  return -ENOMEM;
> >>>+
> >>>+  drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >>>+  if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >>>+  dev_err(dev, "CPU%d drvdata has been initialized\n",
> >>>+  drvdata->cpu);
> >>
> >>May be we could warn about a possible issue in the DT ?
> >
> >So can I understand the suggestion is to add warning in function
> >of_coresight_get_cpu() when cannot find CPU number, and here directly
> >bail out?
> 
> No. One could have single CPU DT, where he doesn't need to provide the CPU 
> number.
> Hence, it doesn't make sense to WARN  in of_coresight_get_cpu().
> 
> But when we hit the case above, we find that the some node was registered for
> the given CPU (be it 0 or any other), which is definitely an error in DT. Due 
> to
> 
> 1) Hasn't specified the CPU number for more than one node
> 
> OR
> 
> 2) CPU number duplicated in the more than one nodes.

Thanks for explaination. It's clear for me now.

> Cheers
> Suzuki
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Mathieu Poirier 
On 18 April 2017 at 18:18, Leo Yan  wrote:
> On Tue, Apr 18, 2017 at 11:40:50AM -0600, Mathieu Poirier wrote:
>> On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote:
>> > Coresight includes debug module and usually the module connects with CPU
>> > debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
>> > description for related info in "Part H: External Debug".
>> >
>> > Chapter H7 "The Sample-based Profiling Extension" introduces several
>> > sampling registers, e.g. we can check program counter value with
>> > combined CPU exception level, secure state, etc. So this is helpful for
>> > analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
>> > loop with IRQ disabled. In this case the CPU cannot switch context and
>> > handle any interrupt (including IPIs), as the result it cannot handle
>> > SMP call for stack dump.
>> >
>> > This patch is to enable coresight debug module, so firstly this driver
>> > is to bind apb clock for debug module and this is to ensure the debug
>> > module can be accessed from program or external debugger. And the driver
>> > uses sample-based registers for debug purpose, e.g. when system triggers
>> > panic, the driver will dump program counter and combined context
>> > registers (EDCIDSR, EDVIDSR); by parsing context registers so can
>> > quickly get to know CPU secure state, exception level, etc.
>> >
>> > Some of the debug module registers are located in CPU power domain, so
>> > this requires the CPU power domain stays on when access related debug
>> > registers, but the power management for CPU power domain is quite
>> > dependent on SoC integration for power management. For the platforms
>> > which with sane power controller implementations, this driver follows
>> > the method to set EDPRCR to try to pull the CPU out of low power state
>> > and then set 'no power down request' bit so the CPU has no chance to
>> > lose power.
>> >
>> > If the SoC has not followed up this design well for power management
>> > controller, the user should use the command line parameter or sysfs
>> > to constrain all or partial idle states to ensure the CPU power
>> > domain is enabled and access coresight CPU debug component safely.
>> >
>> > Signed-off-by: Leo Yan 
>>
>> This is coming along well - a few comment below.  In your next revision 
>> please
>> add GKH to the 'To' list.
>
> Sure. Will send out in this week and add Greg in 'To' list.
>
>> > ---
>> >  drivers/hwtracing/coresight/Kconfig   |  14 +
>> >  drivers/hwtracing/coresight/Makefile  |   1 +
>> >  drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 
>> > ++
>> >  3 files changed, 682 insertions(+)
>> >  create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>> >
>> > diff --git a/drivers/hwtracing/coresight/Kconfig 
>> > b/drivers/hwtracing/coresight/Kconfig
>> > index 130cb21..8d55d6d 100644
>> > --- a/drivers/hwtracing/coresight/Kconfig
>> > +++ b/drivers/hwtracing/coresight/Kconfig
>> > @@ -89,4 +89,18 @@ config CORESIGHT_STM
>> >   logging useful software events or data coming from various entities
>> >   in the system, possibly running different OSs
>> >
>> > +config CORESIGHT_CPU_DEBUG
>> > +   tristate "CoreSight CPU Debug driver"
>> > +   depends on ARM || ARM64
>> > +   depends on DEBUG_FS
>> > +   help
>> > + This driver provides support for coresight debugging module. This
>> > + is primarily used to dump sample-based profiling registers when
>> > + system triggers panic, the driver will parse context registers so
>> > + can quickly get to know program counter (PC), secure state,
>> > + exception level, etc. Before use debugging functionality, platform
>> > + needs to ensure the clock domain and power domain are enabled
>> > + properly, please refer Documentation/trace/coresight-cpu-debug.txt
>> > + for detailed description and the example for usage.
>> > +
>> >  endif
>> > diff --git a/drivers/hwtracing/coresight/Makefile 
>> > b/drivers/hwtracing/coresight/Makefile
>> > index af480d9..433d590 100644
>> > --- a/drivers/hwtracing/coresight/Makefile
>> > +++ b/drivers/hwtracing/coresight/Makefile
>> > @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += 
>> > coresight-etm4x.o \
>> > coresight-etm4x-sysfs.o
>> >  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
>> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
>> > +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
>> > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c 
>> > b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> > new file mode 100644
>> > index 000..8470e31
>> > --- /dev/null
>> > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> > @@ -0,0 +1,667 @@
>> > +/*
>> > + * Copyright (c) 2017 Linaro Limited. All rights reserved.
>> > + *
>> > + * Author: Leo Yan 
>> > + *
>> > + * This program is free software; you can r

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Suzuki K Poulose 

On 19/04/17 15:28, Leo Yan wrote:

Hi Suzuki,

On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:

Hi Leo,

This version looks good to me. I have two minor comments below.


Thanks for reviewing. Will take the suggestions. Just check a bit for
last comment.

[...]


+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
+{
+   void __iomem *base;
+   struct device *dev = &adev->dev;
+   struct debug_drvdata *drvdata;
+   struct resource *res = &adev->res;
+   struct device_node *np = adev->dev.of_node;
+   int ret;
+
+   drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+   if (!drvdata)
+   return -ENOMEM;
+
+   drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
+   if (per_cpu(debug_drvdata, drvdata->cpu)) {
+   dev_err(dev, "CPU%d drvdata has been initialized\n",
+   drvdata->cpu);


May be we could warn about a possible issue in the DT ?


So can I understand the suggestion is to add warning in function
of_coresight_get_cpu() when cannot find CPU number, and here directly
bail out?


No. One could have single CPU DT, where he doesn't need to provide the CPU 
number.
Hence, it doesn't make sense to WARN  in of_coresight_get_cpu().

But when we hit the case above, we find that the some node was registered for
the given CPU (be it 0 or any other), which is definitely an error in DT. Due to

1) Hasn't specified the CPU number for more than one node

OR

2) CPU number duplicated in the more than one nodes.

Cheers
Suzuki

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Leo Yan 
Hi Suzuki,

On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:
> Hi Leo,
> 
> This version looks good to me. I have two minor comments below.

Thanks for reviewing. Will take the suggestions. Just check a bit for
last comment.

[...]

> >+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> >+{
> >+void __iomem *base;
> >+struct device *dev = &adev->dev;
> >+struct debug_drvdata *drvdata;
> >+struct resource *res = &adev->res;
> >+struct device_node *np = adev->dev.of_node;
> >+int ret;
> >+
> >+drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >+if (!drvdata)
> >+return -ENOMEM;
> >+
> >+drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >+if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >+dev_err(dev, "CPU%d drvdata has been initialized\n",
> >+drvdata->cpu);
> 
> May be we could warn about a possible issue in the DT ?

So can I understand the suggestion is to add warning in function
of_coresight_get_cpu() when cannot find CPU number, and here directly
bail out?

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-19 Thread Suzuki K Poulose 

On 06/04/17 14:30, Leo Yan wrote:

Coresight includes debug module and usually the module connects with CPU
debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
description for related info in "Part H: External Debug".

Chapter H7 "The Sample-based Profiling Extension" introduces several
sampling registers, e.g. we can check program counter value with
combined CPU exception level, secure state, etc. So this is helpful for
analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
loop with IRQ disabled. In this case the CPU cannot switch context and
handle any interrupt (including IPIs), as the result it cannot handle
SMP call for stack dump.

This patch is to enable coresight debug module, so firstly this driver
is to bind apb clock for debug module and this is to ensure the debug
module can be accessed from program or external debugger. And the driver
uses sample-based registers for debug purpose, e.g. when system triggers
panic, the driver will dump program counter and combined context
registers (EDCIDSR, EDVIDSR); by parsing context registers so can
quickly get to know CPU secure state, exception level, etc.

Some of the debug module registers are located in CPU power domain, so
this requires the CPU power domain stays on when access related debug
registers, but the power management for CPU power domain is quite
dependent on SoC integration for power management. For the platforms
which with sane power controller implementations, this driver follows
the method to set EDPRCR to try to pull the CPU out of low power state
and then set 'no power down request' bit so the CPU has no chance to
lose power.

If the SoC has not followed up this design well for power management
controller, the user should use the command line parameter or sysfs
to constrain all or partial idle states to ensure the CPU power
domain is enabled and access coresight CPU debug component safely.


Hi Leo,

This version looks good to me. I have two minor comments below.



+
+static struct notifier_block debug_notifier = {
+   .notifier_call = debug_notifier_call,
+};
+
+static int debug_enable_func(void)
+{
+   struct debug_drvdata *drvdata;
+   int cpu;
+
+   for_each_possible_cpu(cpu) {
+   drvdata = per_cpu(debug_drvdata, cpu);
+   if (!drvdata)
+   continue;
+
+   pm_runtime_get_sync(drvdata->dev);
+   }
+
+   return atomic_notifier_chain_register(&panic_notifier_list,
+ &debug_notifier);
+}
+
+static int debug_disable_func(void)
+{
+   struct debug_drvdata *drvdata;
+   int cpu;
+
+   for_each_possible_cpu(cpu) {
+   drvdata = per_cpu(debug_drvdata, cpu);
+   if (!drvdata)
+   continue;
+
+   pm_runtime_put(drvdata->dev);
+   }
+
+   return atomic_notifier_chain_unregister(&panic_notifier_list,
+   &debug_notifier);
+}


I believe you should, reverse the order of these operations in 
debug_disable_func()
to prevent getting a panic notifier after we have released the power domain for 
the
debug.
i.e, :
atomic_notifier_chain_unregister(...);

for_each_possible_cpu(cpu) {}




+
+static ssize_t debug_func_knob_write(struct file *f,
+   const char __user *buf, size_t count, loff_t *ppos)
+{
+   u8 val;
+   int ret;
+
+   ret = kstrtou8_from_user(buf, count, 2, &val);
+   if (ret)
+   return ret;
+
+   mutex_lock(&debug_lock);
+
+   if (val == debug_enable)
+   goto out;
+
+   if (val)
+   ret = debug_enable_func();
+   else
+   ret = debug_disable_func();
+
+   if (ret) {
+   pr_err("%s: unable to %s debug function: %d\n",
+  __func__, val ? "enable" : "disable", ret);
+   goto err;
+   }
+
+   debug_enable = val;
+out:
+   ret = count;
+err:
+   mutex_unlock(&debug_lock);
+   return ret;
+}
+
+static ssize_t debug_func_knob_read(struct file *f,
+   char __user *ubuf, size_t count, loff_t *ppos)
+{
+   ssize_t ret;
+   char buf[2];
+
+   mutex_lock(&debug_lock);
+
+   buf[0] = '0' + debug_enable;
+   buf[1] = '\n';
+   ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
+
+   mutex_unlock(&debug_lock);
+   return ret;
+}
+
+static const struct file_operations debug_func_knob_fops = {
+   .open   = simple_open,
+   .read   = debug_func_knob_read,
+   .write  = debug_func_knob_write,
+};
+
+static int debug_func_init(void)
+{
+   struct dentry *file;
+   int ret;
+
+   /* Create debugfs node */
+   debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
+   if (!debug_debugfs_dir) {
+   pr_err("%s: unable to create debugfs directory\n", __func__);
+   return -ENO

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-18 Thread Leo Yan 
On Tue, Apr 18, 2017 at 11:40:50AM -0600, Mathieu Poirier wrote:
> On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> > description for related info in "Part H: External Debug".
> > 
> > Chapter H7 "The Sample-based Profiling Extension" introduces several
> > sampling registers, e.g. we can check program counter value with
> > combined CPU exception level, secure state, etc. So this is helpful for
> > analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> > loop with IRQ disabled. In this case the CPU cannot switch context and
> > handle any interrupt (including IPIs), as the result it cannot handle
> > SMP call for stack dump.
> > 
> > This patch is to enable coresight debug module, so firstly this driver
> > is to bind apb clock for debug module and this is to ensure the debug
> > module can be accessed from program or external debugger. And the driver
> > uses sample-based registers for debug purpose, e.g. when system triggers
> > panic, the driver will dump program counter and combined context
> > registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> > quickly get to know CPU secure state, exception level, etc.
> > 
> > Some of the debug module registers are located in CPU power domain, so
> > this requires the CPU power domain stays on when access related debug
> > registers, but the power management for CPU power domain is quite
> > dependent on SoC integration for power management. For the platforms
> > which with sane power controller implementations, this driver follows
> > the method to set EDPRCR to try to pull the CPU out of low power state
> > and then set 'no power down request' bit so the CPU has no chance to
> > lose power.
> > 
> > If the SoC has not followed up this design well for power management
> > controller, the user should use the command line parameter or sysfs
> > to constrain all or partial idle states to ensure the CPU power
> > domain is enabled and access coresight CPU debug component safely.
> > 
> > Signed-off-by: Leo Yan 
> 
> This is coming along well - a few comment below.  In your next revision please
> add GKH to the 'To' list.  

Sure. Will send out in this week and add Greg in 'To' list.

> > ---
> >  drivers/hwtracing/coresight/Kconfig   |  14 +
> >  drivers/hwtracing/coresight/Makefile  |   1 +
> >  drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 
> > ++
> >  3 files changed, 682 insertions(+)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
> > 
> > diff --git a/drivers/hwtracing/coresight/Kconfig 
> > b/drivers/hwtracing/coresight/Kconfig
> > index 130cb21..8d55d6d 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -89,4 +89,18 @@ config CORESIGHT_STM
> >   logging useful software events or data coming from various entities
> >   in the system, possibly running different OSs
> >  
> > +config CORESIGHT_CPU_DEBUG
> > +   tristate "CoreSight CPU Debug driver"
> > +   depends on ARM || ARM64
> > +   depends on DEBUG_FS
> > +   help
> > + This driver provides support for coresight debugging module. This
> > + is primarily used to dump sample-based profiling registers when
> > + system triggers panic, the driver will parse context registers so
> > + can quickly get to know program counter (PC), secure state,
> > + exception level, etc. Before use debugging functionality, platform
> > + needs to ensure the clock domain and power domain are enabled
> > + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> > + for detailed description and the example for usage.
> > +
> >  endif
> > diff --git a/drivers/hwtracing/coresight/Makefile 
> > b/drivers/hwtracing/coresight/Makefile
> > index af480d9..433d590 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o 
> > \
> > coresight-etm4x-sysfs.o
> >  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> > +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c 
> > b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> > new file mode 100644
> > index 000..8470e31
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> > @@ -0,0 +1,667 @@
> > +/*
> > + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> > + *
> > + * Author: Leo Yan 
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as 
> > published by
> > + * the Free

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-18 Thread Mathieu Poirier 
On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> description for related info in "Part H: External Debug".
> 
> Chapter H7 "The Sample-based Profiling Extension" introduces several
> sampling registers, e.g. we can check program counter value with
> combined CPU exception level, secure state, etc. So this is helpful for
> analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> loop with IRQ disabled. In this case the CPU cannot switch context and
> handle any interrupt (including IPIs), as the result it cannot handle
> SMP call for stack dump.
> 
> This patch is to enable coresight debug module, so firstly this driver
> is to bind apb clock for debug module and this is to ensure the debug
> module can be accessed from program or external debugger. And the driver
> uses sample-based registers for debug purpose, e.g. when system triggers
> panic, the driver will dump program counter and combined context
> registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> quickly get to know CPU secure state, exception level, etc.
> 
> Some of the debug module registers are located in CPU power domain, so
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
> 
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
> 
> Signed-off-by: Leo Yan 

This is coming along well - a few comment below.  In your next revision please
add GKH to the 'To' list.  

Thanks,
Mathieu

> ---
>  drivers/hwtracing/coresight/Kconfig   |  14 +
>  drivers/hwtracing/coresight/Makefile  |   1 +
>  drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 
> ++
>  3 files changed, 682 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig 
> b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
> logging useful software events or data coming from various entities
> in the system, possibly running different OSs
>  
> +config CORESIGHT_CPU_DEBUG
> + tristate "CoreSight CPU Debug driver"
> + depends on ARM || ARM64
> + depends on DEBUG_FS
> + help
> +   This driver provides support for coresight debugging module. This
> +   is primarily used to dump sample-based profiling registers when
> +   system triggers panic, the driver will parse context registers so
> +   can quickly get to know program counter (PC), secure state,
> +   exception level, etc. Before use debugging functionality, platform
> +   needs to ensure the clock domain and power domain are enabled
> +   properly, please refer Documentation/trace/coresight-cpu-debug.txt
> +   for detailed description and the example for usage.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile 
> b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
>   coresight-etm4x-sysfs.o
>  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
>  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c 
> b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> + *
> + * Author: Leo Yan 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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
> + * mor

[PATCH v6 6/8] coresight: add support for CPU debug module

2017-04-06 Thread Leo Yan 
Coresight includes debug module and usually the module connects with CPU
debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
description for related info in "Part H: External Debug".

Chapter H7 "The Sample-based Profiling Extension" introduces several
sampling registers, e.g. we can check program counter value with
combined CPU exception level, secure state, etc. So this is helpful for
analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
loop with IRQ disabled. In this case the CPU cannot switch context and
handle any interrupt (including IPIs), as the result it cannot handle
SMP call for stack dump.

This patch is to enable coresight debug module, so firstly this driver
is to bind apb clock for debug module and this is to ensure the debug
module can be accessed from program or external debugger. And the driver
uses sample-based registers for debug purpose, e.g. when system triggers
panic, the driver will dump program counter and combined context
registers (EDCIDSR, EDVIDSR); by parsing context registers so can
quickly get to know CPU secure state, exception level, etc.

Some of the debug module registers are located in CPU power domain, so
this requires the CPU power domain stays on when access related debug
registers, but the power management for CPU power domain is quite
dependent on SoC integration for power management. For the platforms
which with sane power controller implementations, this driver follows
the method to set EDPRCR to try to pull the CPU out of low power state
and then set 'no power down request' bit so the CPU has no chance to
lose power.

If the SoC has not followed up this design well for power management
controller, the user should use the command line parameter or sysfs
to constrain all or partial idle states to ensure the CPU power
domain is enabled and access coresight CPU debug component safely.

Signed-off-by: Leo Yan 
---
 drivers/hwtracing/coresight/Kconfig   |  14 +
 drivers/hwtracing/coresight/Makefile  |   1 +
 drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++
 3 files changed, 682 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c

diff --git a/drivers/hwtracing/coresight/Kconfig 
b/drivers/hwtracing/coresight/Kconfig
index 130cb21..8d55d6d 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,18 @@ config CORESIGHT_STM
  logging useful software events or data coming from various entities
  in the system, possibly running different OSs
 
+config CORESIGHT_CPU_DEBUG
+   tristate "CoreSight CPU Debug driver"
+   depends on ARM || ARM64
+   depends on DEBUG_FS
+   help
+ This driver provides support for coresight debugging module. This
+ is primarily used to dump sample-based profiling registers when
+ system triggers panic, the driver will parse context registers so
+ can quickly get to know program counter (PC), secure state,
+ exception level, etc. Before use debugging functionality, platform
+ needs to ensure the clock domain and power domain are enabled
+ properly, please refer Documentation/trace/coresight-cpu-debug.txt
+ for detailed description and the example for usage.
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile 
b/drivers/hwtracing/coresight/Makefile
index af480d9..433d590 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
coresight-etm4x-sysfs.o
 obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
 obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
+obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c 
b/drivers/hwtracing/coresight/coresight-cpu-debug.c
new file mode 100644
index 000..8470e31
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -0,0 +1,667 @@
+/*
+ * Copyright (c) 2017 Linaro Limited. All rights reserved.
+ *
+ * Author: Leo Yan 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include