Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

On Thu, Mar 30, 2017 at 04:56:52PM +0100, Sudeep Holla wrote:

[...]

> > +static struct pm_qos_request debug_qos_req;
> > +static int idle_constraint = PM_QOS_DEFAULT_VALUE;
> > +module_param(idle_constraint, int, 0600);
> > +MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for 
> > CPU "
> > +"idle states (default is -1, which means have no limiation "
> > +"to CPU idle states; 0 means disabling all idle states; user "
> > +"can choose other platform dependent values so can disable "
> > +"specific idle states for the platform)");
> > +
> 
> NACK for this. Why you want the policy inside the driver. You can always
> do that from the user-space. I have mentioned it several times now.
> What can't you do these ?
> 
> 1. echo "what_ever_latency_you_need_in_uS" > /dev/cpu_dma_latency
> 2. echo 1 > /sys/devices/system/cpu/cpu$cpu/cpuidle/state$state/disable
>(for all cpus and their states) (1) is definitely simpler way to
> disable deeper idle if latency = 0uS
> 
> You can always warn user about that when it's enabled via debugfs/sysfs

Thanks for suggestion, now it's clear for me.

> -- 
> Regards,
> Sudeep
--
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 v5 6/9] coresight: add support for CPU debug module

[Sudeep Holla]

On 30/03/17 16:46, Mathieu Poirier wrote:
> On 29 March 2017 at 19:59, Leo Yan  wrote:
>> On Wed, Mar 29, 2017 at 10:55:35AM -0600, Mathieu Poirier wrote:
>>
>> [...]
>>
 So this is why add "idle_constraint" as a central place to control
 power domain for CPU debug purpose and I also think this is more
 friendly for hardware design, e.g. some platforms can enable partial
 low power states to save power and avoid overheat after using this
 driver.

 How about you think for this?
>>>
>>> Like Sudeep pointed out we should concentrate on doing the right thing,
>>> that is work with EDPRSR.PU, EDPRCR.COREPURQ and EDPRCR.CORENPDRQ.
>>
>> Agree, and I think we have aligned for this.
>>
>>> Anything outside of that becomes platform specific and can't be handled in
>>> this driver.
>>
>> Sorry I argue a bit for this just want to make things more clear and
>> if can have better method.
>>
>> Though the issue is platform specific, but the code is to seek common
>> method to handle them. So the driver has no any platform specific code.
> 
> Seeking a common way to handle platform specific problems doesn't
> scale and will never be encompassing.  There will always be a quirk
> somewhere to deal with, hence the idea of keeping things separate.
> 

I completely agree and just responded to the original patch.

>>
>> I read again for Suziki's suggestion: "4) Should document the fact that,
>> on some platforms, the user may have to disable CPUidle explicitly to
>> get the driver working. But let us not make it the default. The user
>> with a not so ideal platform could add "nohlt" and get it working."
> 
> Suzuki and I are expressing the same view using different words.
> 

+1, as I just mentioned on the patch, we can warn user to take action
when this feature gets enabled to get desired result and *nothing more*
than that. Please drop all these pm_qos stuff.

-- 
Regards,
Sudeep
--
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 v5 6/9] coresight: add support for CPU debug module

[Sudeep Holla]

On 25/03/17 18:23, 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 driver introduces module parameter "idle_constraint".
> Setting this parameter for latency requirement in microseconds, finally
> we can constrain all or partial idle states to ensure the CPU power
> domain is enabled, this is a backup method to access coresight CPU
> debug component safely.
> 
> Signed-off-by: Leo Yan 
> ---
>  drivers/hwtracing/coresight/Kconfig   |  11 +
>  drivers/hwtracing/coresight/Makefile  |   1 +
>  drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 
> ++
>  3 files changed, 716 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..18d7931 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,15 @@ 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.
> +
>  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..fbec1d1
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,704 @@
> +/*
> + * 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 
> 

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Sudeep Holla]

On 29/03/17 15:56, Mike Leach wrote:

[...]
> 
> No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and 
> purposes
> 
> EDPRCR_COREPURQ is in the debug power domain an is tied to an external
> debug request that should be an input to the external (to the PE)
> system power controller.
> The requirement is that the system power controller powers up the core
> domain and does not power it down while it remains asserted.
> 
> EDPRCR_CORENPDRQ is in the core power domain and thus to the specific
> core only. This ensures that any power control software running on
> that core should emulate a power down if this is set to one.
> 
> We cannot know the power control design of the system, so the safe
> solution is to set both bits.
> 

+1

I agree that's the safe bet.

-- 
Regards,
Sudeep
--
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 v5 6/9] coresight: add support for CPU debug module

[Mathieu Poirier]

On 29 March 2017 at 19:59, Leo Yan  wrote:
> On Wed, Mar 29, 2017 at 10:55:35AM -0600, Mathieu Poirier wrote:
>
> [...]
>
>> > So this is why add "idle_constraint" as a central place to control
>> > power domain for CPU debug purpose and I also think this is more
>> > friendly for hardware design, e.g. some platforms can enable partial
>> > low power states to save power and avoid overheat after using this
>> > driver.
>> >
>> > How about you think for this?
>>
>> Like Sudeep pointed out we should concentrate on doing the right thing,
>> that is work with EDPRSR.PU, EDPRCR.COREPURQ and EDPRCR.CORENPDRQ.
>
> Agree, and I think we have aligned for this.
>
>> Anything outside of that becomes platform specific and can't be handled in
>> this driver.
>
> Sorry I argue a bit for this just want to make things more clear and
> if can have better method.
>
> Though the issue is platform specific, but the code is to seek common
> method to handle them. So the driver has no any platform specific code.

Seeking a common way to handle platform specific problems doesn't
scale and will never be encompassing.  There will always be a quirk
somewhere to deal with, hence the idea of keeping things separate.

>
> I read again for Suziki's suggestion: "4) Should document the fact that,
> on some platforms, the user may have to disable CPUidle explicitly to
> get the driver working. But let us not make it the default. The user
> with a not so ideal platform could add "nohlt" and get it working."

Suzuki and I are expressing the same view using different words.

>
> So I'm not strong to resist and if this is alignment yet, I should
> document well for this but doesn't handle it in driver (keep driver
> simple).
>
> 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 v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

On Thu, Mar 30, 2017 at 10:00:30AM +0100, Suzuki K Poulose wrote:

[...]

> Leo,
> 
> Also, it would be good to restore the PRCR register back to the original state
> after we read the registers (if we changed them). I am exploring ways to make
> use of this feature on demand (e.g, tie it to sysrq-t or sysrq-l) and not just
> at panic time. So it would be good to have the state restored to not affect 
> the
> normal functioning even after dumping the registers.

Got it. Will take care for this.

Thanks,
Leo Yan

> PS: I have a small hack to hook this into a sysrq-x at runtime, but on a 
> second thought
> it is better not to consume the key and rather tie it to something which 
> already exist,
> as mentioned above.
> 
> Thanks
> 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 v5 6/9] coresight: add support for CPU debug module

[Suzuki K Poulose]

On 30/03/17 02:03, Leo Yan wrote:

On Wed, Mar 29, 2017 at 03:56:23PM +0100, Mike Leach wrote:

[...]


+   /*
+* Unfortunately the CPU cannot be powered up, so return
+* back and later has no permission to access other
+* registers. For this case, should set 'idle_constraint'
+* to ensure CPU power domain is enabled!
+*/
+   if (!(drvdata->edprsr & EDPRSR_PU)) {
+   pr_err("%s: power up request for CPU%d failed\n",
+   __func__, drvdata->cpu);
+   goto out;
+   }
+
+out_powered_up:
+   debug_os_unlock(drvdata);
+
+   /*
+* At this point the CPU is powered up, so set the no powerdown
+* request bit so we don't lose power and emulate power down.
+*/
+   drvdata->edprsr = readl(drvdata->base + EDPRCR);
+   drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;


If we are here the core is already up.  Shouldn't we need to set
EDPRCR_CORENPDRQ only?


Yeah. Will fix.


No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and purposes

EDPRCR_COREPURQ is in the debug power domain an is tied to an external
debug request that should be an input to the external (to the PE)
system power controller.
The requirement is that the system power controller powers up the core
domain and does not power it down while it remains asserted.

EDPRCR_CORENPDRQ is in the core power domain and thus to the specific
core only. This ensures that any power control software running on
that core should emulate a power down if this is set to one.


I'm curious the exact meaning for "power control software".

Does it mean EDPRCR_CORENPDRQ should be checked by kernel or PSCI
liked code in ARM trusted firmware to avoid to run CPU power off flow?

Or will EDPRCR_CORENPDRQ assert CPU external signal to notify power
controller so power controller emulate a power down?


We cannot know the power control design of the system, so the safe
solution is to set both bits.


Thanks a lot for the suggestion. Will set both bits.


Leo,

Also, it would be good to restore the PRCR register back to the original state
after we read the registers (if we changed them). I am exploring ways to make
use of this feature on demand (e.g, tie it to sysrq-t or sysrq-l) and not just
at panic time. So it would be good to have the state restored to not affect the
normal functioning even after dumping the registers.

PS: I have a small hack to hook this into a sysrq-x at runtime, but on a second 
thought
it is better not to consume the key and rather tie it to something which 
already exist,
as mentioned above.

Thanks
Suzuki



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 v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

On Wed, Mar 29, 2017 at 10:55:35AM -0600, Mathieu Poirier wrote:

[...]

> > So this is why add "idle_constraint" as a central place to control
> > power domain for CPU debug purpose and I also think this is more
> > friendly for hardware design, e.g. some platforms can enable partial
> > low power states to save power and avoid overheat after using this
> > driver.
> > 
> > How about you think for this?
> 
> Like Sudeep pointed out we should concentrate on doing the right thing,
> that is work with EDPRSR.PU, EDPRCR.COREPURQ and EDPRCR.CORENPDRQ.

Agree, and I think we have aligned for this.

> Anything outside of that becomes platform specific and can't be handled in
> this driver.

Sorry I argue a bit for this just want to make things more clear and
if can have better method.

Though the issue is platform specific, but the code is to seek common
method to handle them. So the driver has no any platform specific code.

I read again for Suziki's suggestion: "4) Should document the fact that,
on some platforms, the user may have to disable CPUidle explicitly to
get the driver working. But let us not make it the default. The user
with a not so ideal platform could add "nohlt" and get it working."

So I'm not strong to resist and if this is alignment yet, I should
document well for this but doesn't handle it in driver (keep driver
simple).

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 v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

On Wed, Mar 29, 2017 at 04:17:19PM +0100, Mike Leach wrote:

[...]

> >> >+out_powered_up:
> >> >+debug_os_unlock(drvdata);
> >>
> >> Question: Do we need a matching debug_os_lock() once we are done ?
> >
> > I have checked ARM ARMv8, but there have no detailed description for
> > this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
> > no debug_os_lock() related operations.
> >
> > Mike, Mathieu, could you also help confirm this?
> >
> 
> Debug OS lock / unlock allows the power management code running on the
> core to lock out the external debugger while the debug registers are
> saved/restored during a core power event.
> 
> e.g. A sequence such as this might occur in a correctly programmed system
> 
> debug_os_lock()
> save_debug_regs() // visible from core power domain - incl breakpoints etc
> save_etm_regs()
> ... // other stuff prior to core power down,
> 
> 
> Followed by...
> 
> 
> restore_etm_regs()
> restore_debug_regs() // visible from core power domain - incl breakpoints etc
> debug_os_unlock()
> 
> The value is 1 (locked) if cold resetting into AArch64 - it is
> expected that some system software will set this to 0 as part of the
> boot process.
> The lock prevents write access to the external debug registers so we
> need to clear it to set up the external debug registers we are using.

This description is conflict with upper restoring flows. During
restore_debug_regs(), the os lock is locked so how it can write
external debug register to restore context?

> This suggests that it should be restored as we found it when done.

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 v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

On Wed, Mar 29, 2017 at 03:56:23PM +0100, Mike Leach wrote:

[...]

> >> > +   /*
> >> > +* Unfortunately the CPU cannot be powered up, so return
> >> > +* back and later has no permission to access other
> >> > +* registers. For this case, should set 'idle_constraint'
> >> > +* to ensure CPU power domain is enabled!
> >> > +*/
> >> > +   if (!(drvdata->edprsr & EDPRSR_PU)) {
> >> > +   pr_err("%s: power up request for CPU%d failed\n",
> >> > +   __func__, drvdata->cpu);
> >> > +   goto out;
> >> > +   }
> >> > +
> >> > +out_powered_up:
> >> > +   debug_os_unlock(drvdata);
> >> > +
> >> > +   /*
> >> > +* At this point the CPU is powered up, so set the no powerdown
> >> > +* request bit so we don't lose power and emulate power down.
> >> > +*/
> >> > +   drvdata->edprsr = readl(drvdata->base + EDPRCR);
> >> > +   drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> >>
> >> If we are here the core is already up.  Shouldn't we need to set
> >> EDPRCR_CORENPDRQ only?
> >
> > Yeah. Will fix.
> 
> No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and 
> purposes
> 
> EDPRCR_COREPURQ is in the debug power domain an is tied to an external
> debug request that should be an input to the external (to the PE)
> system power controller.
> The requirement is that the system power controller powers up the core
> domain and does not power it down while it remains asserted.
> 
> EDPRCR_CORENPDRQ is in the core power domain and thus to the specific
> core only. This ensures that any power control software running on
> that core should emulate a power down if this is set to one.

I'm curious the exact meaning for "power control software".

Does it mean EDPRCR_CORENPDRQ should be checked by kernel or PSCI
liked code in ARM trusted firmware to avoid to run CPU power off flow?

Or will EDPRCR_CORENPDRQ assert CPU external signal to notify power
controller so power controller emulate a power down?

> We cannot know the power control design of the system, so the safe
> solution is to set both bits.

Thanks a lot for the suggestion. Will set both bits.

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 v5 6/9] coresight: add support for CPU debug module

[Mathieu Poirier]

On Wed, Mar 29, 2017 at 09:54:23AM +0800, Leo Yan wrote:
> Hi Mathieu,
> 
> On Tue, Mar 28, 2017 at 10:50:10AM -0600, Mathieu Poirier wrote:
> > On Sun, Mar 26, 2017 at 02:23:14AM +0800, Leo Yan wrote:
> 
> [...]
> 
> > > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> > > +{
> > > + int timeout = DEBUG_WAIT_TIMEOUT;
> > > +
> > > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > > +
> > > + CS_UNLOCK(drvdata->base);
> > > +
> > > + /* Bail out if CPU is powered up yet */
> > > + if (drvdata->edprsr & EDPRSR_PU)
> > > + goto out_powered_up;
> > > +
> > > + /*
> > > +  * Send request to power management controller and assert
> > > +  * DBGPWRUPREQ signal; if power management controller has
> > > +  * sane implementation, it should enable CPU power domain
> > > +  * in case CPU is in low power state.
> > > +  */
> > > + drvdata->edprsr = readl(drvdata->base + EDPRCR);
> > > + drvdata->edprsr |= EDPRCR_COREPURQ;
> > > + writel(drvdata->edprsr, drvdata->base + EDPRCR);
> > 
> > Here ->edprsr is used but EDPRCR is accessed.  Is this intentional or a
> > copy/paste error?  The same is true for accesses in the out_powered_up 
> > section.
> 
> Thanks for pointing out. This is a typo error and will fix.
> 
> > > +
> > > + /* Wait for CPU to be powered up (timeout~=32ms) */
> > > + while (timeout--) {
> > > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > > + if (drvdata->edprsr & EDPRSR_PU)
> > > + break;
> > > +
> > > + usleep_range(1000, 2000);
> > > + }
> > 
> > See if function readx_poll_timeout() can be used.
> 
> Will use it.
> 
> > > +
> > > + /*
> > > +  * Unfortunately the CPU cannot be powered up, so return
> > > +  * back and later has no permission to access other
> > > +  * registers. For this case, should set 'idle_constraint'
> > > +  * to ensure CPU power domain is enabled!
> > > +  */
> > > + if (!(drvdata->edprsr & EDPRSR_PU)) {
> > > + pr_err("%s: power up request for CPU%d failed\n",
> > > + __func__, drvdata->cpu);
> > > + goto out;
> > > + }
> > > +
> > > +out_powered_up:
> > > + debug_os_unlock(drvdata);
> > > +
> > > + /*
> > > +  * At this point the CPU is powered up, so set the no powerdown
> > > +  * request bit so we don't lose power and emulate power down.
> > > +  */
> > > + drvdata->edprsr = readl(drvdata->base + EDPRCR);
> > > + drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> > 
> > If we are here the core is already up.  Shouldn't we need to set
> > EDPRCR_CORENPDRQ only?
> 
> Yeah. Will fix.
> 
> > > + writel(drvdata->edprsr, drvdata->base + EDPRCR);
> > 
> > This section is a little racy - between the time the PU bit has been
> > checked and the time COREPDRQ has been flipped, the state of PU may have
> > changed.  You can probably get around this by checking edprsr.PU rigth 
> > here.  If
> > it is not set you go through the process again.  Note that doing this will
> > probably force a refactoring of the whole function.  
> 
> Agree. Will handle this.
> 
> [...]
> 
> > > +static ssize_t debug_func_knob_write(struct file *f,
> > > + const char __user *buf, size_t count, loff_t *ppos)
> > > +{
> > > e u8 on;
> > > + int ret;
> > > +
> > > + ret = kstrtou8_from_user(buf, count, 2, );
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(_lock);
> > > +
> > > + if (!on ^ debug_enable)
> > > + goto out;
> > 
> > I had to read this condition too many times - please refactor.
> 
> Will do it.
> 
> > > +
> > > + if (on) {
> > > + ret = debug_enable_func();
> > > + if (ret) {
> > > + pr_err("%s: unable to disable debug function: %d\n",
> > > +__func__, ret);
> > 
> > Based on the semantic this is the wrong error message.
> 
> Yeah. Will fix.
> 
> > > + goto err;
> > > + }
> > > + } else
> > > + debug_disable_func();
> > 
> > Did checkpatch.pl complain about extra curly braces?  If not please add 
> > them.
> 
> checkpatch.pl doesn't report for this. Will add.
> 
> > > +
> > > + debug_enable = on;
> > 
> > Here we can't set debug_enable if we just called debug_disable_func().  
> > Maybe
> > I'm missing something.  If that's the case a comment in the code would be 
> > worth
> > it.
> 
> After called debug_disable_func(), debug_enable is set to 0 (on = 0).
> 
> > > +
> > > +out:
> > > + ret = count;
> > > +err:
> > > + mutex_unlock(_lock);
> > > + return ret;
> > > +}
> > > +
> > > +static ssize_t debug_func_knob_read(struct file *f,
> > > + char __user *ubuf, size_t count, loff_t *ppos)
> > > +{
> > > + char val[] = { '0' + debug_enable, '\n' };
> > > +
> > > + return simple_read_from_buffer(ubuf, count, ppos, val, sizeof(val));
> > 
> > Use the debug_lock to avoid race conditions.
> 
> Will do it.
> 
> > > +}
> > > +
> > > +static ssize_t debug_idle_constraint_write(struct file *f,
> > > + const char __user 

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Suzuki K Poulose]

On 29/03/17 11:37, Leo Yan wrote:

On Wed, Mar 29, 2017 at 11:31:03AM +0100, Suzuki K Poulose wrote:

On 29/03/17 11:27, Leo Yan wrote:

On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:

[...]


+   if (mode == EDDEVID_IMPL_NONE) {
+   drvdata->edpcsr_present  = false;
+   drvdata->edcidsr_present = false;
+   drvdata->edvidsr_present = false;
+   } else if (mode == EDDEVID_IMPL_EDPCSR) {
+   drvdata->edpcsr_present  = true;
+   drvdata->edcidsr_present = false;
+   drvdata->edvidsr_present = false;
+   } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
+   if (!IS_ENABLED(CONFIG_64BIT) &&
+   (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
+   drvdata->edpcsr_present = false;
+   else
+   drvdata->edpcsr_present = true;


Sorry, I forgot why we do this check only in this mode. Shouldn't this be
common to all modes (of course which implies PCSR is present) ?


No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
simplize PCSROffset value :
 - Sample offset applies based on the instruction state (indicated by 
PCSR[0])
0001 - No offset applies.
0010 - No offset applies, but do not use in AArch32 mode!

So we need handle the corner case is when CPU runs AArch32 mode and
PCSRoffset = 'b0010. Other cases the pcsr should be present.


I understand that reasoning. But my question is, why do we check for PCSROffset
only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == 
EDDEVID_IMPL_EDPCSR or
any other mode where PCSR is present.


Sorry I misunderstood your question.

I made mistake when I analyzed the possbile combination for mode and
PCSROffset so I thought it's the only case should handle:
{ EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }

Below three combinations are possible to exist; so you are right, I
should move this out for the checking:
{ EDDEVID_IMPL_NONE,   EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }


That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
don't worry about anything as the functionality is missing. This should rather 
be:
EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.


I think below combination doesn't really exist:
{ EDDEVID_IMPL_EDPCSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 };

EDDEVID_IMPL_EDPCSR is only defined in ARMv7 ARM, and
EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 is only defined in ARMv8 ARM.


It is not wrong to check the PCSROffset in all cases where PCSR is available, 
as if
we hit PCSR on ARMv7 then PCSROffset shouldn't be DIS_AARCH32. And in fact that
would make the code a bit more cleaner. Anyways, I am not particular about this.

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 v5 6/9] coresight: add support for CPU debug module

[Mathieu Poirier]

On Wed, Mar 29, 2017 at 11:07:35AM +0800, Leo Yan wrote:
> Hi Suzuki,
> 
> On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
> > On 25/03/17 18:23, Leo Yan wrote:
> 
> [...]
> 
> > Leo,
> > 
> > Thanks a lot for the quick rework. I don't fully understand (yet!) why we 
> > need the
> > idle_constraint. I will leave it for Sudeep to comment on it, as he is the 
> > expert
> > in that area. Some minor comments below.
> 
> Thanks a lot for quick reviewing :)
> 
> > >Signed-off-by: Leo Yan 
> > >---
> > > drivers/hwtracing/coresight/Kconfig   |  11 +
> > > drivers/hwtracing/coresight/Makefile  |   1 +
> > > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 
> > > ++
> > > 3 files changed, 716 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..18d7931 100644
> > >--- a/drivers/hwtracing/coresight/Kconfig
> > >+++ b/drivers/hwtracing/coresight/Kconfig
> > >@@ -89,4 +89,15 @@ 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.
> > 
> > May be we should mention/warn the user about the possible caveats of using
> > this feature to help him make a better decision ? And / Or we should add a 
> > documentation
> > for it. We have collected some real good information over the discussions 
> > and
> > it is a good idea to capture it somewhere.
> 
> Sure, I will add a documentation for this.
> 
> [...]
> 
> > >+static struct pm_qos_request debug_qos_req;
> > >+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
> > >+module_param(idle_constraint, int, 0600);
> > >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds 
> > >for CPU "
> > >+   "idle states (default is -1, which means have no limiation "
> > >+   "to CPU idle states; 0 means disabling all idle states; user "
> > >+   "can choose other platform dependent values so can disable "
> > >+   "specific idle states for the platform)");
> > 
> > Correct me if I am wrong,
> > 
> > All we want to do is disable the CPUIdle explicitly if the user knows that 
> > this
> > could be a problem to use CPU debug on his platform. So, in effect, we 
> > should
> > only be using idle_constraint = 0 or -1.
> > 
> > In which case, we could make it easier for the user to tell us, either
> > 
> >  0 - Don't do anything with CPUIdle (default)
> >  1 - Disable CPUIdle for me as I know the platform has issues with CPU 
> > debug and CPUidle.
> 
> The reason for not using bool flag is: usually SoC may have many idle
> states, so if user wants to partially enable some states then can set
> the latency to constraint.
> 
> But of course, we can change this to binary value as you suggested,
> this means turn on of turn off all states. The only one reason to use
> latency value is it is more friendly for hardware design, e.g. some
> platforms can enable partial states to save power and avoid overheat
> after using this driver.
> 
> If you guys think this is a bit over design, I will follow up your
> suggestion. I also have some replying in Mathieu's reviewing, please
> help review as well.
> 
> > than explaining the miscrosecond latency etc and make the appropriate calls 
> > underneath.
> > something like (not necessarily the same name) :
> > 
> > module_param(broken_with_cpuidle, bool, 0600);
> > MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has 
> > issues with CPUIdle on"
> >" the platform. Non-zero value implies 
> > CPUIdle has to be"
> >" explicitly disabled.",);
> 
> [...]
> 
> > >+  /*
> > >+   * Unfortunately the CPU cannot be powered up, so return
> > >+   * back and later has no permission to access other
> > >+   * registers. For this case, should set 'idle_constraint'
> > >+   * to ensure CPU power domain is enabled!
> > >+   */
> > >+  if (!(drvdata->edprsr & EDPRSR_PU)) {
> > >+  pr_err("%s: power up request for CPU%d failed\n",
> > >+  __func__, drvdata->cpu);
> > >+  goto out;
> > >+  }
> > >+
> > >+out_powered_up:
> > >+  debug_os_unlock(drvdata);
> > 
> > Question: Do we need a matching debug_os_lock() once we are done ?
> 
> I have checked ARM ARMv8, but there have no 

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Mike Leach]

On 29 March 2017 at 04:07, Leo Yan  wrote:
> Hi Suzuki,
>
> On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
>> On 25/03/17 18:23, Leo Yan wrote:
>
> [...]
>
>> Leo,
>>
>> Thanks a lot for the quick rework. I don't fully understand (yet!) why we 
>> need the
>> idle_constraint. I will leave it for Sudeep to comment on it, as he is the 
>> expert
>> in that area. Some minor comments below.
>
> Thanks a lot for quick reviewing :)
>
>> >Signed-off-by: Leo Yan 
>> >---
>> > drivers/hwtracing/coresight/Kconfig   |  11 +
>> > drivers/hwtracing/coresight/Makefile  |   1 +
>> > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 
>> > ++
>> > 3 files changed, 716 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..18d7931 100644
>> >--- a/drivers/hwtracing/coresight/Kconfig
>> >+++ b/drivers/hwtracing/coresight/Kconfig
>> >@@ -89,4 +89,15 @@ 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.
>>
>> May be we should mention/warn the user about the possible caveats of using
>> this feature to help him make a better decision ? And / Or we should add a 
>> documentation
>> for it. We have collected some real good information over the discussions and
>> it is a good idea to capture it somewhere.
>
> Sure, I will add a documentation for this.
>
> [...]
>
>> >+static struct pm_qos_request debug_qos_req;
>> >+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
>> >+module_param(idle_constraint, int, 0600);
>> >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for 
>> >CPU "
>> >+ "idle states (default is -1, which means have no limiation "
>> >+ "to CPU idle states; 0 means disabling all idle states; user "
>> >+ "can choose other platform dependent values so can disable "
>> >+ "specific idle states for the platform)");
>>
>> Correct me if I am wrong,
>>
>> All we want to do is disable the CPUIdle explicitly if the user knows that 
>> this
>> could be a problem to use CPU debug on his platform. So, in effect, we should
>> only be using idle_constraint = 0 or -1.
>>
>> In which case, we could make it easier for the user to tell us, either
>>
>>  0 - Don't do anything with CPUIdle (default)
>>  1 - Disable CPUIdle for me as I know the platform has issues with CPU debug 
>> and CPUidle.
>
> The reason for not using bool flag is: usually SoC may have many idle
> states, so if user wants to partially enable some states then can set
> the latency to constraint.
>
> But of course, we can change this to binary value as you suggested,
> this means turn on of turn off all states. The only one reason to use
> latency value is it is more friendly for hardware design, e.g. some
> platforms can enable partial states to save power and avoid overheat
> after using this driver.
>
> If you guys think this is a bit over design, I will follow up your
> suggestion. I also have some replying in Mathieu's reviewing, please
> help review as well.
>
>> than explaining the miscrosecond latency etc and make the appropriate calls 
>> underneath.
>> something like (not necessarily the same name) :
>>
>> module_param(broken_with_cpuidle, bool, 0600);
>> MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has 
>> issues with CPUIdle on"
>>  " the platform. Non-zero value implies 
>> CPUIdle has to be"
>>  " explicitly disabled.",);
>
> [...]
>
>> >+/*
>> >+ * Unfortunately the CPU cannot be powered up, so return
>> >+ * back and later has no permission to access other
>> >+ * registers. For this case, should set 'idle_constraint'
>> >+ * to ensure CPU power domain is enabled!
>> >+ */
>> >+if (!(drvdata->edprsr & EDPRSR_PU)) {
>> >+pr_err("%s: power up request for CPU%d failed\n",
>> >+__func__, drvdata->cpu);
>> >+goto out;
>> >+}
>> >+
>> >+out_powered_up:
>> >+debug_os_unlock(drvdata);
>>
>> Question: Do we need a matching debug_os_lock() once we are done ?
>
> I have checked ARM ARMv8, but there have no detailed description for
> this. I refered coresight-etmv4 code 

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Mike Leach]

 Hi Leo

On 29 March 2017 at 02:54, Leo Yan  wrote:
> Hi Mathieu,
>
> On Tue, Mar 28, 2017 at 10:50:10AM -0600, Mathieu Poirier wrote:
>> On Sun, Mar 26, 2017 at 02:23:14AM +0800, Leo Yan wrote:
>
> [...]
>
>> > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
>> > +{
>> > +   int timeout = DEBUG_WAIT_TIMEOUT;
>> > +
>> > +   drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > +
>> > +   CS_UNLOCK(drvdata->base);
>> > +
>> > +   /* Bail out if CPU is powered up yet */
>> > +   if (drvdata->edprsr & EDPRSR_PU)
>> > +   goto out_powered_up;
>> > +
>> > +   /*
>> > +* Send request to power management controller and assert
>> > +* DBGPWRUPREQ signal; if power management controller has
>> > +* sane implementation, it should enable CPU power domain
>> > +* in case CPU is in low power state.
>> > +*/
>> > +   drvdata->edprsr = readl(drvdata->base + EDPRCR);
>> > +   drvdata->edprsr |= EDPRCR_COREPURQ;
>> > +   writel(drvdata->edprsr, drvdata->base + EDPRCR);
>>
>> Here ->edprsr is used but EDPRCR is accessed.  Is this intentional or a
>> copy/paste error?  The same is true for accesses in the out_powered_up 
>> section.
>
> Thanks for pointing out. This is a typo error and will fix.
>
>> > +
>> > +   /* Wait for CPU to be powered up (timeout~=32ms) */
>> > +   while (timeout--) {
>> > +   drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > +   if (drvdata->edprsr & EDPRSR_PU)
>> > +   break;
>> > +
>> > +   usleep_range(1000, 2000);
>> > +   }
>>
>> See if function readx_poll_timeout() can be used.
>
> Will use it.
>
>> > +
>> > +   /*
>> > +* Unfortunately the CPU cannot be powered up, so return
>> > +* back and later has no permission to access other
>> > +* registers. For this case, should set 'idle_constraint'
>> > +* to ensure CPU power domain is enabled!
>> > +*/
>> > +   if (!(drvdata->edprsr & EDPRSR_PU)) {
>> > +   pr_err("%s: power up request for CPU%d failed\n",
>> > +   __func__, drvdata->cpu);
>> > +   goto out;
>> > +   }
>> > +
>> > +out_powered_up:
>> > +   debug_os_unlock(drvdata);
>> > +
>> > +   /*
>> > +* At this point the CPU is powered up, so set the no powerdown
>> > +* request bit so we don't lose power and emulate power down.
>> > +*/
>> > +   drvdata->edprsr = readl(drvdata->base + EDPRCR);
>> > +   drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
>>
>> If we are here the core is already up.  Shouldn't we need to set
>> EDPRCR_CORENPDRQ only?
>
> Yeah. Will fix.

No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and purposes

EDPRCR_COREPURQ is in the debug power domain an is tied to an external
debug request that should be an input to the external (to the PE)
system power controller.
The requirement is that the system power controller powers up the core
domain and does not power it down while it remains asserted.

EDPRCR_CORENPDRQ is in the core power domain and thus to the specific
core only. This ensures that any power control software running on
that core should emulate a power down if this is set to one.

We cannot know the power control design of the system, so the safe
solution is to set both bits.

Mike
>
>> > +   writel(drvdata->edprsr, drvdata->base + EDPRCR);
>>
>> This section is a little racy - between the time the PU bit has been
>> checked and the time COREPDRQ has been flipped, the state of PU may have
>> changed.  You can probably get around this by checking edprsr.PU rigth here. 
>>  If
>> it is not set you go through the process again.  Note that doing this will
>> probably force a refactoring of the whole function.
>
> Agree. Will handle this.
>
> [...]
>
>> > +static ssize_t debug_func_knob_write(struct file *f,
>> > +   const char __user *buf, size_t count, loff_t *ppos)
>> > +{
>> > e   u8 on;
>> > +   int ret;
>> > +
>> > +   ret = kstrtou8_from_user(buf, count, 2, );
>> > +   if (ret)
>> > +   return ret;
>> > +
>> > +   mutex_lock(_lock);
>> > +
>> > +   if (!on ^ debug_enable)
>> > +   goto out;
>>
>> I had to read this condition too many times - please refactor.
>
> Will do it.
>
>> > +
>> > +   if (on) {
>> > +   ret = debug_enable_func();
>> > +   if (ret) {
>> > +   pr_err("%s: unable to disable debug function: %d\n",
>> > +  __func__, ret);
>>
>> Based on the semantic this is the wrong error message.
>
> Yeah. Will fix.
>
>> > +   goto err;
>> > +   }
>> > +   } else
>> > +   debug_disable_func();
>>
>> Did checkpatch.pl complain about extra curly braces?  If not please add them.
>
> checkpatch.pl doesn't report for this. Will add.
>
>> > +
>> > +   debug_enable = on;
>>
>> Here we can't set debug_enable if we just called debug_disable_func().  Maybe
>> I'm missing something.  If that's the case a comment in the code would be 
>> 

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

On Wed, Mar 29, 2017 at 11:31:03AM +0100, Suzuki K Poulose wrote:
> On 29/03/17 11:27, Leo Yan wrote:
> >On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:
> >
> >[...]
> >
> >+if (mode == EDDEVID_IMPL_NONE) {
> >+drvdata->edpcsr_present  = false;
> >+drvdata->edcidsr_present = false;
> >+drvdata->edvidsr_present = false;
> >+} else if (mode == EDDEVID_IMPL_EDPCSR) {
> >+drvdata->edpcsr_present  = true;
> >+drvdata->edcidsr_present = false;
> >+drvdata->edvidsr_present = false;
> >+} else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
> >+if (!IS_ENABLED(CONFIG_64BIT) &&
> >+(pcsr_offset == 
> >EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
> >+drvdata->edpcsr_present = false;
> >+else
> >+drvdata->edpcsr_present = true;
> 
> Sorry, I forgot why we do this check only in this mode. Shouldn't this be
> common to all modes (of course which implies PCSR is present) ?
> >>>
> >>>No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
> >>>simplize PCSROffset value :
> >>> - Sample offset applies based on the instruction state (indicated by 
> >>>PCSR[0])
> >>>0001 - No offset applies.
> >>>0010 - No offset applies, but do not use in AArch32 mode!
> >>>
> >>>So we need handle the corner case is when CPU runs AArch32 mode and
> >>>PCSRoffset = 'b0010. Other cases the pcsr should be present.
> >>
> >>I understand that reasoning. But my question is, why do we check for 
> >>PCSROffset
> >>only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == 
> >>EDDEVID_IMPL_EDPCSR or
> >>any other mode where PCSR is present.
> >
> >Sorry I misunderstood your question.
> >
> >I made mistake when I analyzed the possbile combination for mode and
> >PCSROffset so I thought it's the only case should handle:
> >{ EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
> >
> >Below three combinations are possible to exist; so you are right, I
> >should move this out for the checking:
> >{ EDDEVID_IMPL_NONE,   EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
> 
> That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
> don't worry about anything as the functionality is missing. This should 
> rather be:
> EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.

I think below combination doesn't really exist:
{ EDDEVID_IMPL_EDPCSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 };

EDDEVID_IMPL_EDPCSR is only defined in ARMv7 ARM, and
EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 is only defined in ARMv8 ARM.

> My switch...case suggestion makes it easier to do all this checking.

Agree. Will do this.

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 v5 6/9] coresight: add support for CPU debug module

[Suzuki K Poulose]

On 29/03/17 11:27, Leo Yan wrote:

On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:

[...]


+   if (mode == EDDEVID_IMPL_NONE) {
+   drvdata->edpcsr_present  = false;
+   drvdata->edcidsr_present = false;
+   drvdata->edvidsr_present = false;
+   } else if (mode == EDDEVID_IMPL_EDPCSR) {
+   drvdata->edpcsr_present  = true;
+   drvdata->edcidsr_present = false;
+   drvdata->edvidsr_present = false;
+   } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
+   if (!IS_ENABLED(CONFIG_64BIT) &&
+   (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
+   drvdata->edpcsr_present = false;
+   else
+   drvdata->edpcsr_present = true;


Sorry, I forgot why we do this check only in this mode. Shouldn't this be
common to all modes (of course which implies PCSR is present) ?


No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
simplize PCSROffset value :
 - Sample offset applies based on the instruction state (indicated by 
PCSR[0])
0001 - No offset applies.
0010 - No offset applies, but do not use in AArch32 mode!

So we need handle the corner case is when CPU runs AArch32 mode and
PCSRoffset = 'b0010. Other cases the pcsr should be present.


I understand that reasoning. But my question is, why do we check for PCSROffset
only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == 
EDDEVID_IMPL_EDPCSR or
any other mode where PCSR is present.


Sorry I misunderstood your question.

I made mistake when I analyzed the possbile combination for mode and
PCSROffset so I thought it's the only case should handle:
{ EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }

Below three combinations are possible to exist; so you are right, I
should move this out for the checking:
{ EDDEVID_IMPL_NONE,   EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }


That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
don't worry about anything as the functionality is missing. This should rather 
be:
EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.

My switch...case suggestion makes it easier to do all this checking.


Thanks
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 v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:

[...]

> >>>+  if (mode == EDDEVID_IMPL_NONE) {
> >>>+  drvdata->edpcsr_present  = false;
> >>>+  drvdata->edcidsr_present = false;
> >>>+  drvdata->edvidsr_present = false;
> >>>+  } else if (mode == EDDEVID_IMPL_EDPCSR) {
> >>>+  drvdata->edpcsr_present  = true;
> >>>+  drvdata->edcidsr_present = false;
> >>>+  drvdata->edvidsr_present = false;
> >>>+  } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
> >>>+  if (!IS_ENABLED(CONFIG_64BIT) &&
> >>>+  (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
> >>>+  drvdata->edpcsr_present = false;
> >>>+  else
> >>>+  drvdata->edpcsr_present = true;
> >>
> >>Sorry, I forgot why we do this check only in this mode. Shouldn't this be
> >>common to all modes (of course which implies PCSR is present) ?
> >
> >No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
> >simplize PCSROffset value :
> > - Sample offset applies based on the instruction state (indicated by 
> >PCSR[0])
> >0001 - No offset applies.
> >0010 - No offset applies, but do not use in AArch32 mode!
> >
> >So we need handle the corner case is when CPU runs AArch32 mode and
> >PCSRoffset = 'b0010. Other cases the pcsr should be present.
> 
> I understand that reasoning. But my question is, why do we check for 
> PCSROffset
> only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == 
> EDDEVID_IMPL_EDPCSR or
> any other mode where PCSR is present.

Sorry I misunderstood your question.

I made mistake when I analyzed the possbile combination for mode and
PCSROffset so I thought it's the only case should handle:
{ EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }

Below three combinations are possible to exist; so you are right, I
should move this out for the checking:
{ EDDEVID_IMPL_NONE,   EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
{ EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
{ EDDEVID_IMPL_FULL,   EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }

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 v5 6/9] coresight: add support for CPU debug module

[Suzuki K Poulose]

On 29/03/17 04:07, Leo Yan wrote:

Hi Suzuki,

On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:

On 25/03/17 18:23, Leo Yan wrote:


[...]


Leo,

Thanks a lot for the quick rework. I don't fully understand (yet!) why we need 
the
idle_constraint. I will leave it for Sudeep to comment on it, as he is the 
expert
in that area. Some minor comments below.


Thanks a lot for quick reviewing :)


Signed-off-by: Leo Yan 
---
drivers/hwtracing/coresight/Kconfig   |  11 +
drivers/hwtracing/coresight/Makefile  |   1 +
drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++
3 files changed, 716 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..18d7931 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,15 @@ 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.


May be we should mention/warn the user about the possible caveats of using
this feature to help him make a better decision ? And / Or we should add a 
documentation
for it. We have collected some real good information over the discussions and
it is a good idea to capture it somewhere.


Sure, I will add a documentation for this.

[...]


+static struct pm_qos_request debug_qos_req;
+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
+module_param(idle_constraint, int, 0600);
+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU 
"
+"idle states (default is -1, which means have no limiation "
+"to CPU idle states; 0 means disabling all idle states; user "
+"can choose other platform dependent values so can disable "
+"specific idle states for the platform)");


Correct me if I am wrong,

All we want to do is disable the CPUIdle explicitly if the user knows that this
could be a problem to use CPU debug on his platform. So, in effect, we should
only be using idle_constraint = 0 or -1.

In which case, we could make it easier for the user to tell us, either

 0 - Don't do anything with CPUIdle (default)
 1 - Disable CPUIdle for me as I know the platform has issues with CPU debug 
and CPUidle.


The reason for not using bool flag is: usually SoC may have many idle
states, so if user wants to partially enable some states then can set
the latency to constraint.

But of course, we can change this to binary value as you suggested,
this means turn on of turn off all states. The only one reason to use
latency value is it is more friendly for hardware design, e.g. some
platforms can enable partial states to save power and avoid overheat
after using this driver.

If you guys think this is a bit over design, I will follow up your
suggestion. I also have some replying in Mathieu's reviewing, please
help review as well.


than explaining the miscrosecond latency etc and make the appropriate calls 
underneath.
something like (not necessarily the same name) :

module_param(broken_with_cpuidle, bool, 0600);
MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues 
with CPUIdle on"
   " the platform. Non-zero value implies 
CPUIdle has to be"
   " explicitly disabled.",);


[...]


+   /*
+* Unfortunately the CPU cannot be powered up, so return
+* back and later has no permission to access other
+* registers. For this case, should set 'idle_constraint'
+* to ensure CPU power domain is enabled!
+*/
+   if (!(drvdata->edprsr & EDPRSR_PU)) {
+   pr_err("%s: power up request for CPU%d failed\n",
+   __func__, drvdata->cpu);
+   goto out;
+   }
+
+out_powered_up:
+   debug_os_unlock(drvdata);


Question: Do we need a matching debug_os_lock() once we are done ?


I have checked ARM ARMv8, but there have no detailed description for
this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
no debug_os_lock() related operations.

Mike, Mathieu, could you also help confirm this?

[...]


+static void debug_init_arch_data(void *info)
+{
+   struct debug_drvdata *drvdata = info;
+   u32 mode, pcsr_offset;
+
+   CS_UNLOCK(drvdata->base);
+
+   

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

Hi Suzuki,

On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
> On 25/03/17 18:23, Leo Yan wrote:

[...]

> Leo,
> 
> Thanks a lot for the quick rework. I don't fully understand (yet!) why we 
> need the
> idle_constraint. I will leave it for Sudeep to comment on it, as he is the 
> expert
> in that area. Some minor comments below.

Thanks a lot for quick reviewing :)

> >Signed-off-by: Leo Yan 
> >---
> > drivers/hwtracing/coresight/Kconfig   |  11 +
> > drivers/hwtracing/coresight/Makefile  |   1 +
> > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 
> > ++
> > 3 files changed, 716 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..18d7931 100644
> >--- a/drivers/hwtracing/coresight/Kconfig
> >+++ b/drivers/hwtracing/coresight/Kconfig
> >@@ -89,4 +89,15 @@ 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.
> 
> May be we should mention/warn the user about the possible caveats of using
> this feature to help him make a better decision ? And / Or we should add a 
> documentation
> for it. We have collected some real good information over the discussions and
> it is a good idea to capture it somewhere.

Sure, I will add a documentation for this.

[...]

> >+static struct pm_qos_request debug_qos_req;
> >+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
> >+module_param(idle_constraint, int, 0600);
> >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for 
> >CPU "
> >+ "idle states (default is -1, which means have no limiation "
> >+ "to CPU idle states; 0 means disabling all idle states; user "
> >+ "can choose other platform dependent values so can disable "
> >+ "specific idle states for the platform)");
> 
> Correct me if I am wrong,
> 
> All we want to do is disable the CPUIdle explicitly if the user knows that 
> this
> could be a problem to use CPU debug on his platform. So, in effect, we should
> only be using idle_constraint = 0 or -1.
> 
> In which case, we could make it easier for the user to tell us, either
> 
>  0 - Don't do anything with CPUIdle (default)
>  1 - Disable CPUIdle for me as I know the platform has issues with CPU debug 
> and CPUidle.

The reason for not using bool flag is: usually SoC may have many idle
states, so if user wants to partially enable some states then can set
the latency to constraint.

But of course, we can change this to binary value as you suggested,
this means turn on of turn off all states. The only one reason to use
latency value is it is more friendly for hardware design, e.g. some
platforms can enable partial states to save power and avoid overheat
after using this driver.

If you guys think this is a bit over design, I will follow up your
suggestion. I also have some replying in Mathieu's reviewing, please
help review as well.

> than explaining the miscrosecond latency etc and make the appropriate calls 
> underneath.
> something like (not necessarily the same name) :
> 
> module_param(broken_with_cpuidle, bool, 0600);
> MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has 
> issues with CPUIdle on"
>  " the platform. Non-zero value implies 
> CPUIdle has to be"
>  " explicitly disabled.",);

[...]

> >+/*
> >+ * Unfortunately the CPU cannot be powered up, so return
> >+ * back and later has no permission to access other
> >+ * registers. For this case, should set 'idle_constraint'
> >+ * to ensure CPU power domain is enabled!
> >+ */
> >+if (!(drvdata->edprsr & EDPRSR_PU)) {
> >+pr_err("%s: power up request for CPU%d failed\n",
> >+__func__, drvdata->cpu);
> >+goto out;
> >+}
> >+
> >+out_powered_up:
> >+debug_os_unlock(drvdata);
> 
> Question: Do we need a matching debug_os_lock() once we are done ?

I have checked ARM ARMv8, but there have no detailed description for
this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
no debug_os_lock() related operations.

Mike, Mathieu, could you also help confirm this?

[...]

> >+static void debug_init_arch_data(void *info)
> >+{
> >+struct 

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Leo Yan]

Hi Mathieu,

On Tue, Mar 28, 2017 at 10:50:10AM -0600, Mathieu Poirier wrote:
> On Sun, Mar 26, 2017 at 02:23:14AM +0800, Leo Yan wrote:

[...]

> > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> > +{
> > +   int timeout = DEBUG_WAIT_TIMEOUT;
> > +
> > +   drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > +
> > +   CS_UNLOCK(drvdata->base);
> > +
> > +   /* Bail out if CPU is powered up yet */
> > +   if (drvdata->edprsr & EDPRSR_PU)
> > +   goto out_powered_up;
> > +
> > +   /*
> > +* Send request to power management controller and assert
> > +* DBGPWRUPREQ signal; if power management controller has
> > +* sane implementation, it should enable CPU power domain
> > +* in case CPU is in low power state.
> > +*/
> > +   drvdata->edprsr = readl(drvdata->base + EDPRCR);
> > +   drvdata->edprsr |= EDPRCR_COREPURQ;
> > +   writel(drvdata->edprsr, drvdata->base + EDPRCR);
> 
> Here ->edprsr is used but EDPRCR is accessed.  Is this intentional or a
> copy/paste error?  The same is true for accesses in the out_powered_up 
> section.

Thanks for pointing out. This is a typo error and will fix.

> > +
> > +   /* Wait for CPU to be powered up (timeout~=32ms) */
> > +   while (timeout--) {
> > +   drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > +   if (drvdata->edprsr & EDPRSR_PU)
> > +   break;
> > +
> > +   usleep_range(1000, 2000);
> > +   }
> 
> See if function readx_poll_timeout() can be used.

Will use it.

> > +
> > +   /*
> > +* Unfortunately the CPU cannot be powered up, so return
> > +* back and later has no permission to access other
> > +* registers. For this case, should set 'idle_constraint'
> > +* to ensure CPU power domain is enabled!
> > +*/
> > +   if (!(drvdata->edprsr & EDPRSR_PU)) {
> > +   pr_err("%s: power up request for CPU%d failed\n",
> > +   __func__, drvdata->cpu);
> > +   goto out;
> > +   }
> > +
> > +out_powered_up:
> > +   debug_os_unlock(drvdata);
> > +
> > +   /*
> > +* At this point the CPU is powered up, so set the no powerdown
> > +* request bit so we don't lose power and emulate power down.
> > +*/
> > +   drvdata->edprsr = readl(drvdata->base + EDPRCR);
> > +   drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> 
> If we are here the core is already up.  Shouldn't we need to set
> EDPRCR_CORENPDRQ only?

Yeah. Will fix.

> > +   writel(drvdata->edprsr, drvdata->base + EDPRCR);
> 
> This section is a little racy - between the time the PU bit has been
> checked and the time COREPDRQ has been flipped, the state of PU may have
> changed.  You can probably get around this by checking edprsr.PU rigth here.  
> If
> it is not set you go through the process again.  Note that doing this will
> probably force a refactoring of the whole function.  

Agree. Will handle this.

[...]

> > +static ssize_t debug_func_knob_write(struct file *f,
> > +   const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > e   u8 on;
> > +   int ret;
> > +
> > +   ret = kstrtou8_from_user(buf, count, 2, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   mutex_lock(_lock);
> > +
> > +   if (!on ^ debug_enable)
> > +   goto out;
> 
> I had to read this condition too many times - please refactor.

Will do it.

> > +
> > +   if (on) {
> > +   ret = debug_enable_func();
> > +   if (ret) {
> > +   pr_err("%s: unable to disable debug function: %d\n",
> > +  __func__, ret);
> 
> Based on the semantic this is the wrong error message.

Yeah. Will fix.

> > +   goto err;
> > +   }
> > +   } else
> > +   debug_disable_func();
> 
> Did checkpatch.pl complain about extra curly braces?  If not please add them.

checkpatch.pl doesn't report for this. Will add.

> > +
> > +   debug_enable = on;
> 
> Here we can't set debug_enable if we just called debug_disable_func().  Maybe
> I'm missing something.  If that's the case a comment in the code would be 
> worth
> it.

After called debug_disable_func(), debug_enable is set to 0 (on = 0).

> > +
> > +out:
> > +   ret = count;
> > +err:
> > +   mutex_unlock(_lock);
> > +   return ret;
> > +}
> > +
> > +static ssize_t debug_func_knob_read(struct file *f,
> > +   char __user *ubuf, size_t count, loff_t *ppos)
> > +{
> > +   char val[] = { '0' + debug_enable, '\n' };
> > +
> > +   return simple_read_from_buffer(ubuf, count, ppos, val, sizeof(val));
> 
> Use the debug_lock to avoid race conditions.

Will do it.

> > +}
> > +
> > +static ssize_t debug_idle_constraint_write(struct file *f,
> > +   const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > +   int val;
> > +   ssize_t ret;
> > +
> > +   ret = kstrtoint_from_user(buf, count, 10, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   mutex_lock(_lock);
> > +   idle_constraint = val;
> > +
> > +   

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Mathieu Poirier]

Hi Leo,

On Sun, Mar 26, 2017 at 02:23:14AM +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 driver introduces module parameter "idle_constraint".
> Setting this parameter for latency requirement in microseconds, finally
> we can constrain all or partial idle states to ensure the CPU power
> domain is enabled, this is a backup method to access coresight CPU
> debug component safely.
> 
> Signed-off-by: Leo Yan 
> ---
>  drivers/hwtracing/coresight/Kconfig   |  11 +
>  drivers/hwtracing/coresight/Makefile  |   1 +
>  drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 
> ++
>  3 files changed, 716 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..18d7931 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,15 @@ 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.
> +
>  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..fbec1d1
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,704 @@
> +/*
> + * 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 
> 

Re: [PATCH v5 6/9] coresight: add support for CPU debug module

[Suzuki K Poulose]

On 25/03/17 18:23, 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 driver introduces module parameter "idle_constraint".
Setting this parameter for latency requirement in microseconds, finally
we can constrain all or partial idle states to ensure the CPU power
domain is enabled, this is a backup method to access coresight CPU
debug component safely.


Leo,

Thanks a lot for the quick rework. I don't fully understand (yet!) why we need 
the
idle_constraint. I will leave it for Sudeep to comment on it, as he is the 
expert
in that area. Some minor comments below.



Signed-off-by: Leo Yan 
---
 drivers/hwtracing/coresight/Kconfig   |  11 +
 drivers/hwtracing/coresight/Makefile  |   1 +
 drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++
 3 files changed, 716 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..18d7931 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,15 @@ 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.


May be we should mention/warn the user about the possible caveats of using
this feature to help him make a better decision ? And / Or we should add a 
documentation
for it. We have collected some real good information over the discussions and
it is a good idea to capture it somewhere.


+
 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..fbec1d1
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c



+#define EDPCSR 0x0A0
+#define EDCIDSR0x0A4
+#define EDVIDSR0x0A8
+#define EDPCSR_HI  0x0AC
+#define EDOSLAR0x300
+#define EDPRCR 0x310
+#define EDPRSR 0x314
+#define EDDEVID1   0xFC4
+#define EDDEVID0xFC8
+
+#define EDPCSR_PROHIBITED

[PATCH v5 6/9] coresight: add support for CPU debug module

[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 driver introduces module parameter "idle_constraint".
Setting this parameter for latency requirement in microseconds, finally
we can constrain all or partial idle states to ensure the CPU power
domain is enabled, this is a backup method to access coresight CPU
debug component safely.

Signed-off-by: Leo Yan 
---
 drivers/hwtracing/coresight/Kconfig   |  11 +
 drivers/hwtracing/coresight/Makefile  |   1 +
 drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++
 3 files changed, 716 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..18d7931 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,15 @@ 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.
+
 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..fbec1d1
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -0,0 +1,704 @@
+/*
+ * 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 
+
+#include "coresight-priv.h"
+
+#define EDPCSR 0x0A0
+#define EDCIDSR