On Wed, Mar 22, 2017 at 02:07:47PM +0000, Sudeep Holla wrote:

On 22/03/17 12:54, Mike Leach wrote:

quoted

On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com 
<mailto:sudeep.holla@arm.com>> wrote:

[...]

quoted

I disagree with this approach. One of the main usefulness of such
self hosted debug feature is to debug issues around features like
cpuidle. Adding constraints like "cpuidle needs to be disabled" is
not good IMO. There are ways to make it work with cpuidle enabled.
Please explore them. In particular refer H9.2.39 EDPRCR, External
Debug Power/Reset Control Register.

So, "nohlt" option is not an option. I prefer some sysfs option like 
Suzuki suggested to enable this feature on demand if power saving in 
normal usecase is the concern. Using "nohlt" just disables idle and 
doesn't ensure the debug power domain is ON. Using the flag directly
in this driver to enable debug power domain also sounds misuse of
that flag for me.

I think the key issue to remember here is that experience with
external debug shows that CPU Idle means different things to
different SoC designs / power management schemes. (and we are using
external debug in a self hosted way here).

Yes agreed on the point that meaning of "cpuidle" differs on each SoC.

Very appreciate for Mike's summary. It's shame for me this is one thing
I should do better :)

This good summary is quite important.

quoted

Some designs will power down an entire cluster if all CPUs on the 
cluster are powered down - including the parts of the debug
registers that should remain powered in the debug power domain.

Interesting, at-least ETMv4 or some other coresight specification
clearly classify the power domains and the register access. The actual
power domain itself may vary depending on implementation.

quoted

The bits in EDPRCR are not respected in these cases - these designs
do not really support debug over power down in the way that the
CoreSight / Debug designers anticipated. This means that even
checking EDPRSR has the potential to cause a bus hang if the target
register is unpowered. (and if the debug power domain is unpowered
then the PC data is also lost).

Agreed, but can we start supporting the sane designs in sane way first.
We can always add compatible and handle deviations. I agree we may need
to support such deviations but starting with that seems setting a bad
example.

quoted

In these cases, accessing to the debug registers while they are not 
powered is a recipe for disaster - so preventing CPUIdle ?and the
subsequent cluster power down ? allows investigation on this class of
system - ?and allowing the CPUs of interest be interrogated without
hanging the crash log process.?

Agreed. But my point is that many issues are around cpuidle and some
usecase and just eliminating that use-case sounds bad. For me,
core-sight was most useful to debug issues around cpu power management
and lockups where we can't stop cores but examine these registers.
There are other alternatives for other use-cases IMO.

quoted

?On systems that do behave correctly with respect to debug power 
domains, then disabling CPUIdle is unnecessary - these can be
controlled by ?EDPRCR - perhaps; per the specification it is
"implementation defined" if writing bits to this register have an
effect on the system anyway even if the debug domain is correctly
powered.

We can always do that unconditionally. If implementations don't honor
those bits, it's different. If they hang on accessing something which is
on debug power domain and not on core power domain, then you have much
bigger issue to solve. How can you even trust and make any other
register accesses that are in debug power domain then ?

So we can add below code before really access another other registers
are possible in CPU power domain:

        /*
         * Force to power on CPU power domain and assert
         * DBGPWRUPREQ signal
         */
        val = readl(drvdata->base + EDPRCR);
        val |= BIT(3);
        writel(val, drvdata->base + EDPRCR);

quoted

?While it is true to say that disabling CPUIdle does not guarantee
that the debug power domain is on, it does in a certain class of
designs prevent it being powered off (Juno historically - not sure if
that is still the case.).

Again it's completely platform specific. All you need to care is that
the debug power domain is on or not. Disabling CPUIdle to achieve that
is simply wrong and may work only on few platforms.

quoted

However, I do agree that the use of the driver should not be
triggered _only_ on the existence of /nohlt on the command line - ?
there is a class of designs where this will not be required.

Thanks

quoted

When enabing the driver as a kernel config the user needs to
decide:- 1) do I need this to debug the issue I am seeing 2) does the
power management on my system require I use /nohlt as well.

Please don't *misuse* nohlt to disable idle. There are other ways to
do the same either from the user-space or from the driver.

quoted

I think that the use of /nohlt as an option, and the reasons why it 
might be needed should be part of the configuration help in this
case.

There is also a case for considering if there should be an option to 
configure it to be enabled or disabled at boot time. It is easy to 
imagine cases I want to have this running from the start as a crash 
happens early - and cases I can enable it on demand later.

Also consider with cpuidle enabled ;). I can help testing if needed.

I tried to digest these info and below are my understanding from your
suggestion:

### For boot time: add two command line flags

- coresight.cpu_debug: this flag is used to enable cpu debug module at
  boot time, and it relys on sane hardware design (like PRCR can works
  well) to access registers;

- coresight.cpu_debug_pwrup: this flag is used to enable cpu debug
  module at boot time, and it cannot relys on PRCR anymore so we need
  manually constraint CPU power states;

### For runtime: use one sysfs node

- Create sysfs node:
  /sys/kernel/debug/coresight_cpu_debug/enable_debug

  echo 1 > /sys/kernel/debug/coresight_cpu_debug/enable_debug: same
  functionality with boot time's 'coresight.cpu_debug';

  echo 2 > /sys/kernel/debug/coresight_cpu_debug/enable_debug: same
  functionality with boot time's 'coresight.cpu_debug_pwrup';

  echo 0 > /sys/kernel/debug/coresight_cpu_debug/enable_debug: disable
  debug functionality.

Does this make sense?

Thanks,
Leo Yan