Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1605743 > unrolled thread

Re: [PATCH v3 3/5] coresight: add support for debug module

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-21 16:50 +0100
    Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 15:20 +0100
      Re: [PATCH v3 3/5] coresight: add support for debug module Mike Leach <mike.leach@linaro.org> - 2017-03-22 17:00 +0100
        Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 17:20 +0100
          Re: [PATCH v3 3/5] coresight: add support for debug module Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2017-03-22 18:30 +0100
            Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 18:40 +0100
              Re: [PATCH v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-23 08:40 +0100
                Re: [PATCH v3 3/5] coresight: add support for debug module Mike Leach <mike.leach@linaro.org> - 2017-03-23 13:30 +0100
      Re: [PATCH v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-22 17:10 +0100
        Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 18:10 +0100

#1605743 — Re: [PATCH v3 3/5] coresight: add support for debug module

On 03/03/17 06:00, 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 detects
> the CPU lockup and trigger 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
> in the driver it has checked the power state for CPU before accessing
> registers within CPU power domain. For most safe way to use this driver,
> it's suggested to disable CPU low power states, this can simply set
> "nohlt" in kernel command line.
> 

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.

-- 
Regards,
Sudeep

[toc] | [next] | [standalone]

#1606547

On 22/03/17 12:54, Mike Leach wrote:
> 
> 
> On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com 
> <mailto:sudeep.holla@arm.com>> wrote:
> 
[...]

> 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.

> 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.

> 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.

> 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.

> 
> ​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 ?

> ​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.

> 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

> 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.

> 
> 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.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1606698

On 22 March 2017 at 14:07, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 22/03/17 12:54, Mike Leach wrote:
>>
>>
>> On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com
>> <mailto:sudeep.holla@arm.com>> wrote:
>>
> [...]
>
>> 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.
>
>> 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.

Yes - the ETMv4 spec defines what should be in the core / debug power
domains, but there is no architectural requirement for these to be
separate.
Most of power management is "implementation defined", & hw designers
seem to have different criteria than sw engineers wanting to debug
stuff.

>
>> 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.
>

From a pragmatic point of view, we have to support the designs that we
have and are currently using.
Sadly this might include some that do not behave in an ideal way.

I'm not saying disabling CPUIdle is right for all cases, or perhaps
many, but it has in the past been useful in specific instances - not
just for external debug, but to use CoreSight trace etc, were powering
down the a CPU / cluster takes out ETM accesses and breaks stuff.

The key is that to use this driver, the user has to be aware of the PM
implications on their specific system - the kernel may not take care
of it all for them - as SCP type power controllers are often external
and may have unique firmware and capabilites.

Historically CPUIdle disable has been used as a blunt instrument to
handle power management problems in real debug use cases - but it is
one that has been successful. Where there are better methods then I am
all for using these.


>> 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.

For your case, removing what you are interested in debugging is
evidently counter productive, so other techniques need to be used to
ensure the CS regs remain alive.
But equally there could be use cases where this might be just fine or
even the only way.

>
>>
>> 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 ?
>

It is difficult and highly platform dependent. For external debug we
might have per-platform rules built into the debugger on what can and
cannot be done and when, plus on occasion some power management
scripts. Those platforms that get closest to the "standard" CoreSight
power management are easiest to debug.

>> 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.
>
>> 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
>
>> 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.

I'm not advocating /nohlt above anything else - it's just what is
being discussed here. Furthermore, no one debug technique is ever
going to be appropriate in all circumstances - debug and trace are
always a compromise.

I initially raised the issue of clusters powering down, and
possibility that no CPUIdle might prevent this, to ensure that
awareness is built in to driver / config / help text /documentation
that these are real issues seen in the external debug world.

The key point is that the caveat in using this driver is that the
power management has to be considered on a platform specific basis
before it is configured; and appropriate actions may be needed for it
to work correctly. Without this then the driver could cause more
issues than it debugs. A user selecting this _must_ be told about
these issues

>
>>
>> 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.
>
> --
> Regards,
> Sudeep

Regards

Mike

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK

[toc] | [prev] | [next] | [standalone]

#1606714

On 22/03/17 15:45, Mike Leach wrote:
> On 22 March 2017 at 14:07, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 22/03/17 12:54, Mike Leach wrote:
>>>
>>>
>>> On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com
>>> <mailto:sudeep.holla@arm.com>> wrote:
>>>
>> [...]
>>
>>> 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.
>>
>>> 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.
> 
> Yes - the ETMv4 spec defines what should be in the core / debug power
> domains, but there is no architectural requirement for these to be
> separate.
> Most of power management is "implementation defined", & hw designers
> seem to have different criteria than sw engineers wanting to debug
> stuff.
> 

Yes I agree, no argument there.

>>> 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.
>>
> 
> From a pragmatic point of view, we have to support the designs that we
> have and are currently using.
> Sadly this might include some that do not behave in an ideal way.
> 

We will have to support them. But I don't want that to be defacto
standard. They should be treated as deviations. Though specification
says the behavior is IMPDEF, it does provide standard interface(EDPRCR)
to use and we should have that in the driver IMO.

> I'm not saying disabling CPUIdle is right for all cases, or perhaps
> many, but it has in the past been useful in specific instances - not
> just for external debug, 

Yes I know but that's either issue with the firmware or the debugger
and we can work them around in user-space too instead of baking the
solution to kernel.

> but to use CoreSight trace etc, were powering
> down the a CPU / cluster takes out ETM accesses and breaks stuff.
> 

FYI, I added support in ETMv4 to emulate power-down during active trace
session and so far I have not seen any report on things breaking because
of that register access(may be no one has tested it on other platforms
yet :()

> The key is that to use this driver, the user has to be aware of the PM
> implications on their specific system - the kernel may not take care
> of it all for them - as SCP type power controllers are often external
> and may have unique firmware and capabilites.
> 

Sure

> Historically CPUIdle disable has been used as a blunt instrument to
> handle power management problems in real debug use cases - but it is
> one that has been successful. Where there are better methods then I am
> all for using these.
> 

Cool, thanks. Lets try out that and see if it helps first before bluntly
advertising CPUIdle disabling and that too in misleading ways like "nohlt"

>>> 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.
> 
> For your case, removing what you are interested in debugging is
> evidently counter productive, so other techniques need to be used to
> ensure the CS regs remain alive.
> But equally there could be use cases where this might be just fine or
> even the only way.
>

Again not arguing on that. Just saying we can try out if that solves the
issue. As I said there are other ways to disable idle and we can
advertise those instead of such misuse. Also those alternative methods
are runtime and can be used when you need them.

>>>
>>> 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 ?
>>
> 
> It is difficult and highly platform dependent. For external debug we
> might have per-platform rules built into the debugger on what can and
> cannot be done and when, plus on occasion some power management
> scripts. Those platforms that get closest to the "standard" CoreSight
> power management are easiest to debug.
> 

Absolutely, so just mandating might just solve issue *accidentally*
not *intentionally*. So I don't want it to be advertised in that way
and that becomes defacto.

[...]
> 
> I'm not advocating /nohlt above anything else - it's just what is
> being discussed here. Furthermore, no one debug technique is ever
> going to be appropriate in all circumstances - debug and trace are
> always a compromise.
> 

True.

> I initially raised the issue of clusters powering down, and
> possibility that no CPUIdle might prevent this, to ensure that
> awareness is built in to driver / config / help text /documentation
> that these are real issues seen in the external debug world.
> 

Point taken. So we could just specify that all necessary power domains
need to be on for proper functionality for this feature and that it's
highly platform specific instead of mixing cpu/cluster idle details here.

> The key point is that the caveat in using this driver is that the
> power management has to be considered on a platform specific basis
> before it is configured; and appropriate actions may be needed for it
> to work correctly. Without this then the driver could cause more
> issues than it debugs. A user selecting this _must_ be told about
> these issues
> 

Exactly and the help text could be something on the similar lines as you
mention/summarize above.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1606795

On 22/03/17 16:17, Sudeep Holla wrote:
>
>
> On 22/03/17 15:45, Mike Leach wrote:
>> On 22 March 2017 at 14:07, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 22/03/17 12:54, Mike Leach wrote:
>>>>
>>>>
>>>> On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com
>>>> <mailto:sudeep.holla@arm.com>> wrote:
>>>>
>>> [...]
>>>
>>>> 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.
>>>
>>>> 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.
>>
>> Yes - the ETMv4 spec defines what should be in the core / debug power
>> domains, but there is no architectural requirement for these to be
>> separate.
>> Most of power management is "implementation defined", & hw designers
>> seem to have different criteria than sw engineers wanting to debug
>> stuff.
>>
>
> Yes I agree, no argument there.
>
>>>> 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.
>>>
>>
>> From a pragmatic point of view, we have to support the designs that we
>> have and are currently using.
>> Sadly this might include some that do not behave in an ideal way.
>>
>
> We will have to support them. But I don't want that to be defacto
> standard. They should be treated as deviations. Though specification
> says the behavior is IMPDEF, it does provide standard interface(EDPRCR)
> to use and we should have that in the driver IMO.
>
>> I'm not saying disabling CPUIdle is right for all cases, or perhaps
>> many, but it has in the past been useful in specific instances - not
>> just for external debug,
>
> Yes I know but that's either issue with the firmware or the debugger
> and we can work them around in user-space too instead of baking the
> solution to kernel.
>
>> but to use CoreSight trace etc, were powering
>> down the a CPU / cluster takes out ETM accesses and breaks stuff.
>>
>
> FYI, I added support in ETMv4 to emulate power-down during active trace
> session and so far I have not seen any report on things breaking because
> of that register access(may be no one has tested it on other platforms
> yet :()
>
>> The key is that to use this driver, the user has to be aware of the PM
>> implications on their specific system - the kernel may not take care
>> of it all for them - as SCP type power controllers are often external
>> and may have unique firmware and capabilites.
>>
>
> Sure
>
>> Historically CPUIdle disable has been used as a blunt instrument to
>> handle power management problems in real debug use cases - but it is
>> one that has been successful. Where there are better methods then I am
>> all for using these.
>>
>
> Cool, thanks. Lets try out that and see if it helps first before bluntly
> advertising CPUIdle disabling and that too in misleading ways like "nohlt"
>
>>>> 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.
>>
>> For your case, removing what you are interested in debugging is
>> evidently counter productive, so other techniques need to be used to
>> ensure the CS regs remain alive.
>> But equally there could be use cases where this might be just fine or
>> even the only way.
>>
>
> Again not arguing on that. Just saying we can try out if that solves the
> issue. As I said there are other ways to disable idle and we can
> advertise those instead of such misuse. Also those alternative methods
> are runtime and can be used when you need them.
>
>>>>
>>>> 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 ?
>>>
>>
>> It is difficult and highly platform dependent. For external debug we
>> might have per-platform rules built into the debugger on what can and
>> cannot be done and when, plus on occasion some power management
>> scripts. Those platforms that get closest to the "standard" CoreSight
>> power management are easiest to debug.
>>
>
> Absolutely, so just mandating might just solve issue *accidentally*
> not *intentionally*. So I don't want it to be advertised in that way
> and that becomes defacto.
>
> [...]
>>
>> I'm not advocating /nohlt above anything else - it's just what is
>> being discussed here. Furthermore, no one debug technique is ever
>> going to be appropriate in all circumstances - debug and trace are
>> always a compromise.
>>
>
> True.
>
>> I initially raised the issue of clusters powering down, and
>> possibility that no CPUIdle might prevent this, to ensure that
>> awareness is built in to driver / config / help text /documentation
>> that these are real issues seen in the external debug world.
>>
>
> Point taken. So we could just specify that all necessary power domains
> need to be on for proper functionality for this feature and that it's
> highly platform specific instead of mixing cpu/cluster idle details here.
>
>> The key point is that the caveat in using this driver is that the
>> power management has to be considered on a platform specific basis
>> before it is configured; and appropriate actions may be needed for it
>> to work correctly. Without this then the driver could cause more
>> issues than it debugs. A user selecting this _must_ be told about
>> these issues
>>

So given all the possible caveats, I think we :

1) Shouldn't enable the driver by default at runtime even if it is built-in.
2) Should provide mechanisms to turn it on at boot (via kernel commandline)
    or anytime later (via sysfs), which kind of puts the responsibility back on
    the user : "You know what you are doing".
3) Shouldn't turn the driver on based on "nohlt" which the user could use it for
    some other purposes, without explicit intention of turning this driver on).
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
    

[toc] | [prev] | [next] | [standalone]

#1606807

On 22/03/17 17:09, Suzuki K Poulose wrote:
> On 22/03/17 16:17, Sudeep Holla wrote:

[...]

>> 
>> Point taken. So we could just specify that all necessary power
>> domains need to be on for proper functionality for this feature and
>> that it's highly platform specific instead of mixing cpu/cluster
>> idle details here.
>> 
>>> The key point is that the caveat in using this driver is that
>>> the power management has to be considered on a platform specific
>>> basis before it is configured; and appropriate actions may be
>>> needed for it to work correctly. Without this then the driver
>>> could cause more issues than it debugs. A user selecting this
>>> _must_ be told about these issues
>>> 
> 
> So given all the possible caveats, I think we :
> 
> 1) Shouldn't enable the driver by default at runtime even if it is 
> built-in. 
> 2) Should provide mechanisms to turn it on at boot (via
> kernel commandline) or anytime later (via sysfs), which kind of puts
> the responsibility back on the user : "You know what you are doing". 
> 3) Shouldn't turn the driver on based on "nohlt" which the user
> could use it for some other purposes, without explicit intention of
> turning this driver on).
> 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.
> 

Agreed on all points and well summarized.
I would like to highlight (3) and (4) as it needs to be well understood.

"nohlt" has a *different* meaning already, so using that in this
driver for something else is simple wrong as it affects the system in
unintended ways. And yes if user (mis)uses it to get things working,
it's fine but shouldn't be recommended way.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1607190

On Wed, Mar 22, 2017 at 05:25:50PM +0000, Sudeep Holla wrote:
> 
> 
> On 22/03/17 17:09, Suzuki K Poulose wrote:
> > On 22/03/17 16:17, Sudeep Holla wrote:
> 
> [...]
> 
> >> 
> >> Point taken. So we could just specify that all necessary power
> >> domains need to be on for proper functionality for this feature and
> >> that it's highly platform specific instead of mixing cpu/cluster
> >> idle details here.
> >> 
> >>> The key point is that the caveat in using this driver is that
> >>> the power management has to be considered on a platform specific
> >>> basis before it is configured; and appropriate actions may be
> >>> needed for it to work correctly. Without this then the driver
> >>> could cause more issues than it debugs. A user selecting this
> >>> _must_ be told about these issues
> >>> 
> > 
> > So given all the possible caveats, I think we :
> > 
> > 1) Shouldn't enable the driver by default at runtime even if it is 
> > built-in. 
> > 2) Should provide mechanisms to turn it on at boot (via
> > kernel commandline) or anytime later (via sysfs), which kind of puts
> > the responsibility back on the user : "You know what you are doing". 
> > 3) Shouldn't turn the driver on based on "nohlt" which the user
> > could use it for some other purposes, without explicit intention of
> > turning this driver on).
> > 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.
> > 
> 
> Agreed on all points and well summarized.
> I would like to highlight (3) and (4) as it needs to be well understood.
> 
> "nohlt" has a *different* meaning already, so using that in this
> driver for something else is simple wrong as it affects the system in
> unintended ways. And yes if user (mis)uses it to get things working,
> it's fine but shouldn't be recommended way.

Understand this point.

I will try to use general way to constraint CPUIdle like other
drivers.

Thanks all for these good suggestions :)

Thanks,
Leo Yan

[toc] | [prev] | [next] | [standalone]

#1607437

Also agree with Suzuki's excellent summary.

A couple of notes on using EDPRCR....

The logic for this assuming a correctly implemented system should be
something like.....

if (prsr() == powered_down) { // if we are powered down request power up.
    sw_unlock()   // ensure that the sw lock register on the device is unlocked
    write(prcr(bit3=1))
    wait_with_timeout(prsr()==powered_up)   // must wait to see if the
system does power up - otherwise bad things happen!
    if(timed_out)
       fail("power up request for CPU N failed.")  // nothing to do
after this point though if a CPU can't be powered is should not be
included in the list to be checked on crash.
}
ensure_os_lock_unlocked()  // os lock has to be unlocked for bit 0 of
prcr to be writeable.
sw_unlock()   // ensure that the sw lock register on the device is unlocked
write(prcr(bit3=1 | bit0=1)) // the core is powered, set the
nopowerdown request bit so we don't lose it & emulate power down


I think that some of the above logic is already in the driver, so
needs to be adapted for the PRCR handling.


On the "bad" systems, the initial prsr check is likely to fail (crash
/ buslock) if the debug logic is not correctly powered. The user will
then have to use one or more of the PM mitigations previously
discussed that are appropriate for that specific platform and retry

Regards

Mike


On 23 March 2017 at 05:43, Leo Yan <leo.yan@linaro.org> wrote:
> On Wed, Mar 22, 2017 at 05:25:50PM +0000, Sudeep Holla wrote:
>>
>>
>> On 22/03/17 17:09, Suzuki K Poulose wrote:
>> > On 22/03/17 16:17, Sudeep Holla wrote:
>>
>> [...]
>>
>> >>
>> >> Point taken. So we could just specify that all necessary power
>> >> domains need to be on for proper functionality for this feature and
>> >> that it's highly platform specific instead of mixing cpu/cluster
>> >> idle details here.
>> >>
>> >>> The key point is that the caveat in using this driver is that
>> >>> the power management has to be considered on a platform specific
>> >>> basis before it is configured; and appropriate actions may be
>> >>> needed for it to work correctly. Without this then the driver
>> >>> could cause more issues than it debugs. A user selecting this
>> >>> _must_ be told about these issues
>> >>>
>> >
>> > So given all the possible caveats, I think we :
>> >
>> > 1) Shouldn't enable the driver by default at runtime even if it is
>> > built-in.
>> > 2) Should provide mechanisms to turn it on at boot (via
>> > kernel commandline) or anytime later (via sysfs), which kind of puts
>> > the responsibility back on the user : "You know what you are doing".
>> > 3) Shouldn't turn the driver on based on "nohlt" which the user
>> > could use it for some other purposes, without explicit intention of
>> > turning this driver on).
>> > 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.
>> >
>>
>> Agreed on all points and well summarized.
>> I would like to highlight (3) and (4) as it needs to be well understood.
>>
>> "nohlt" has a *different* meaning already, so using that in this
>> driver for something else is simple wrong as it affects the system in
>> unintended ways. And yes if user (mis)uses it to get things working,
>> it's fine but shouldn't be recommended way.
>
> Understand this point.
>
> I will try to use general way to constraint CPUIdle like other
> drivers.
>
> Thanks all for these good suggestions :)
>
> Thanks,
> Leo Yan



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK

[toc] | [prev] | [next] | [standalone]

#1606703

On Wed, Mar 22, 2017 at 02:07:47PM +0000, Sudeep Holla wrote:
> On 22/03/17 12:54, Mike Leach wrote:
> > On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com 
> > <mailto:sudeep.holla@arm.com>> wrote:
> > 
> [...]
> 
> > 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.

> > 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.
> 
> > 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.
> 
> > 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.
> 
> > 
> > ​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);

> > ​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.
> 
> > 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
> 
> > 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.
> 
> > 
> > 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

[toc] | [prev] | [next] | [standalone]

#1606773

On 22/03/17 16:01, Leo Yan wrote:
> On Wed, Mar 22, 2017 at 02:07:47PM +0000, Sudeep Holla wrote:

[...]

>>
>> 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);
> 

Yes worth trying it out.

[...]

> 
> I tried to digest these info and below are my understanding from your
> suggestion:
> 
> ### For boot time: add two command line flags
> 

I am not really sure about boot flags as there are dependency on power
domains and expecting them to be powered on quite earlier is too much to
ask. I am not sure if we need special case for boot time. But that's
just my opinion. If someone has found it *really* useful and no other
alternative exists, then go for it.

[...]

> ### 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';
> 

My argument was this to be default without any need for flags.
We can skip it as and when we find broken implementation if required.

>   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.
>

So it can be simple boolean to force setup the power domain requirements
for it to work whenever you need to activate it. I may be missing some
use-case, but IIUC simple boolean flag should be fine as suggested
initially.

-- 
Regards,
Sudeep

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web