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

Groups > linux.kernel > #1601542 > unrolled thread

Re: [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: [v3 3/5] coresight: add support for debug module Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2017-03-15 17:50 +0100
    Re: [v3 3/5] coresight: add support for debug module Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-03-15 21:50 +0100
      Re: [v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-17 11:50 +0100
        Re: [v3 3/5] coresight: add support for debug module Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-03-17 17:00 +0100
          Re: [v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-17 17:40 +0100
            Re: [v3 3/5] coresight: add support for debug module Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2017-03-17 17:50 +0100
              Re: [v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-20 13:40 +0100
              Re: [v3 3/5] coresight: add support for debug module Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-03-20 17:50 +0100
                Re: [v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-21 04:10 +0100
                  Re: [v3 3/5] coresight: add support for debug module Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2017-03-21 11:20 +0100
                    Re: [v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-21 12:50 +0100
                      Re: [v3 3/5] coresight: add support for debug module Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-03-21 16:30 +0100

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

On 13/03/17 16:56, Mathieu Poirier wrote:
> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote:
>>>>> +
>>>>> +	put_online_cpus();
>>>>> +
>>>>> +	if (!debug_count++)
>>>>> +		atomic_notifier_chain_register(&panic_notifier_list,
>>>>> +					       &debug_notifier);
>>>>> +
>>>>
>>>>> +	sprintf(buf, (char *)id->data, drvdata->cpu);
>>>>> +	dev_info(dev, "%s initialized\n", buf);
>>>>
>>>> This could simply be :
>>>> 	dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
>>>>
>>>> and get rid of the static string and the buffer, see below.
>>
>> Also we need pm_runtime_put() here to balance the pm_runtime_get_ from AMBA
>> device probe.
>
> Good point.
>
>> More on that below.
>>
>>>>
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static struct amba_id debug_ids[] = {
>>>>> +	{       /* Debug for Cortex-A53 */
>>>>> +		.id	= 0x000bbd03,
>>>>> +		.mask	= 0x000fffff,
>>>>
>>>> ...
>>>>
>>>>> +		.data   = "Coresight debug-CPU%d",
>>>>
>>>> I think this is pointless, as the debug area we are interested in is always associated
>>>> with a CPU, we could as well figure out what to print from the drvdata->cpu above.
>>>
>>> I prefer to follow your suggestion for upper two comments; but I'd like
>>> check with Mathieu, due I followed up Mathieu's suggestion to write
>>> current code.
>>
>> Btw, I don't see any PM calls to make sure the power domain (at least the debug domain)
>> is up, which could cause problems with accesses to some of these registers (leave alone the
>> ones in CPU power domain), especially the EDPRSR. We could also do pm_runtime_get on the
>> CPU's power domain, if the CPU is online, before we access the pcsr.
>
> I thought about PM runtime operations a little while back but wondered if it is
> really a good thing to have them around.  When this code is called the system
> has crashed and as such making PM runtimes call isn't a good idea.

You are right. It is not safe to make such calls when we have crashed.
The other side effect is, if we don't have the debug power domain up,
we could possibly hang the system and prevent other registered notifiers
from running, which doesn't sound good either.

>
> One thing we could do is _not_ call pm_runtime_put() at the end of the probe()
> operation.  That way we wouldn't have to mess around with PM runtime operations
> on an unstable system.  This, of course, is costly in terms of power consumption
> but the system is under test/debug anyway.

May be control the behavior via kernel command line ? Something like coresight_debug={on or 1} or
even use the "nohlt" ?

Suzuki

[toc] | [next] | [standalone]

#1601730

On 15 March 2017 at 10:44, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 13/03/17 16:56, Mathieu Poirier wrote:
>>
>> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote:
>>>>>>
>>>>>> +
>>>>>> +       put_online_cpus();
>>>>>> +
>>>>>> +       if (!debug_count++)
>>>>>> +               atomic_notifier_chain_register(&panic_notifier_list,
>>>>>> +                                              &debug_notifier);
>>>>>> +
>>>>>
>>>>>
>>>>>> +       sprintf(buf, (char *)id->data, drvdata->cpu);
>>>>>> +       dev_info(dev, "%s initialized\n", buf);
>>>>>
>>>>>
>>>>> This could simply be :
>>>>>         dev_info(dev, "Coresight debug-CPU%d initialized\n",
>>>>> drvdata->cpu);
>>>>>
>>>>> and get rid of the static string and the buffer, see below.
>>>
>>>
>>> Also we need pm_runtime_put() here to balance the pm_runtime_get_ from
>>> AMBA
>>> device probe.
>>
>>
>> Good point.
>>
>>> More on that below.
>>>
>>>>>
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static struct amba_id debug_ids[] = {
>>>>>> +       {       /* Debug for Cortex-A53 */
>>>>>> +               .id     = 0x000bbd03,
>>>>>> +               .mask   = 0x000fffff,
>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>> +               .data   = "Coresight debug-CPU%d",
>>>>>
>>>>>
>>>>> I think this is pointless, as the debug area we are interested in is
>>>>> always associated
>>>>> with a CPU, we could as well figure out what to print from the
>>>>> drvdata->cpu above.
>>>>
>>>>
>>>> I prefer to follow your suggestion for upper two comments; but I'd like
>>>> check with Mathieu, due I followed up Mathieu's suggestion to write
>>>> current code.
>>>
>>>
>>> Btw, I don't see any PM calls to make sure the power domain (at least the
>>> debug domain)
>>> is up, which could cause problems with accesses to some of these
>>> registers (leave alone the
>>> ones in CPU power domain), especially the EDPRSR. We could also do
>>> pm_runtime_get on the
>>> CPU's power domain, if the CPU is online, before we access the pcsr.
>>
>>
>> I thought about PM runtime operations a little while back but wondered if
>> it is
>> really a good thing to have them around.  When this code is called the
>> system
>> has crashed and as such making PM runtimes call isn't a good idea.
>
>
> You are right. It is not safe to make such calls when we have crashed.
> The other side effect is, if we don't have the debug power domain up,
> we could possibly hang the system and prevent other registered notifiers
> from running, which doesn't sound good either.
>
>>
>> One thing we could do is _not_ call pm_runtime_put() at the end of the
>> probe()
>> operation.  That way we wouldn't have to mess around with PM runtime
>> operations
>> on an unstable system.  This, of course, is costly in terms of power
>> consumption
>> but the system is under test/debug anyway.
>
>
> May be control the behavior via kernel command line ? Something like
> coresight_debug={on or 1} or
> even use the "nohlt" ?

We need to deal with the debug and CPU power domains.

For the former I suggest we do what coresight does and use the
"power-domains" binding[1].  For the CPU power domain we can re-use
the "nohlt" flag.  In the probe function if the "nohlt" cmd line flag
is not set the code bails out.  If it is set pm_runtime_put() is _not_
called and the driver can be used without worries of hanging the
system when the panic handler is invoked.

Am I forgetting something?

[1]. http://lxr.free-electrons.com/source/arch/arm64/boot/dts/arm/juno-base.dtsi#L137

>
> Suzuki

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

#1603179

On Wed, Mar 15, 2017 at 02:41:59PM -0600, Mathieu Poirier wrote:
> On 15 March 2017 at 10:44, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> > On 13/03/17 16:56, Mathieu Poirier wrote:
> >> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote:

[...]

> >>> Btw, I don't see any PM calls to make sure the power domain (at least the
> >>> debug domain)
> >>> is up, which could cause problems with accesses to some of these
> >>> registers (leave alone the
> >>> ones in CPU power domain), especially the EDPRSR. We could also do
> >>> pm_runtime_get on the
> >>> CPU's power domain, if the CPU is online, before we access the pcsr.
> >>
> >>
> >> I thought about PM runtime operations a little while back but wondered if
> >> it is
> >> really a good thing to have them around.  When this code is called the
> >> system
> >> has crashed and as such making PM runtimes call isn't a good idea.
> >
> >
> > You are right. It is not safe to make such calls when we have crashed.
> > The other side effect is, if we don't have the debug power domain up,
> > we could possibly hang the system and prevent other registered notifiers
> > from running, which doesn't sound good either.
> >
> >>
> >> One thing we could do is _not_ call pm_runtime_put() at the end of the
> >> probe()
> >> operation.  That way we wouldn't have to mess around with PM runtime
> >> operations
> >> on an unstable system.  This, of course, is costly in terms of power
> >> consumption
> >> but the system is under test/debug anyway.
> >
> >
> > May be control the behavior via kernel command line ? Something like
> > coresight_debug={on or 1} or
> > even use the "nohlt" ?
> 
> We need to deal with the debug and CPU power domains.
> 
> For the former I suggest we do what coresight does and use the
> "power-domains" binding[1].  For the CPU power domain we can re-use
> the "nohlt" flag.  In the probe function if the "nohlt" cmd line flag
> is not set the code bails out.  If it is set pm_runtime_put() is _not_
> called and the driver can be used without worries of hanging the
> system when the panic handler is invoked.
> 
> Am I forgetting something?

I tested this drvier on Hikey and DB410c. For Hikey we must pass
"nohlt" to disable low power states, otherwise the kernel will hang
during initialization. But for DB410c, this driver even can work well
without "nohlt" and I checked the CPUIdle has been really enabled with
below sysfs entries:

root@linaro-developer:~# cat /sys/devices/system/cpu/cpu*/cpuidle/state*/usage
5992
6988
4225
4547
2790
23696
4202
3899

And from my previous experience, I'm quite sure some SoCs can access
the debug module registers with CPUIdle enabled, and it will read
back 0xFFFF_FFFF_FFFF_FFFF when the CPU stays in low power state.
So I prefer we could keep current method to suggest to use "nohlt" in
Kconfig's help description but it's not mandotory to check this in
the code. How about you think for this?

Thanks,
Leo Yan

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

#1603425

On Fri, Mar 17, 2017 at 06:13:28PM +0800, Leo Yan wrote:
> On Wed, Mar 15, 2017 at 02:41:59PM -0600, Mathieu Poirier wrote:
> > On 15 March 2017 at 10:44, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> > > On 13/03/17 16:56, Mathieu Poirier wrote:
> > >> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote:
> 
> [...]
> 
> > >>> Btw, I don't see any PM calls to make sure the power domain (at least the
> > >>> debug domain)
> > >>> is up, which could cause problems with accesses to some of these
> > >>> registers (leave alone the
> > >>> ones in CPU power domain), especially the EDPRSR. We could also do
> > >>> pm_runtime_get on the
> > >>> CPU's power domain, if the CPU is online, before we access the pcsr.
> > >>
> > >>
> > >> I thought about PM runtime operations a little while back but wondered if
> > >> it is
> > >> really a good thing to have them around.  When this code is called the
> > >> system
> > >> has crashed and as such making PM runtimes call isn't a good idea.
> > >
> > >
> > > You are right. It is not safe to make such calls when we have crashed.
> > > The other side effect is, if we don't have the debug power domain up,
> > > we could possibly hang the system and prevent other registered notifiers
> > > from running, which doesn't sound good either.
> > >
> > >>
> > >> One thing we could do is _not_ call pm_runtime_put() at the end of the
> > >> probe()
> > >> operation.  That way we wouldn't have to mess around with PM runtime
> > >> operations
> > >> on an unstable system.  This, of course, is costly in terms of power
> > >> consumption
> > >> but the system is under test/debug anyway.
> > >
> > >
> > > May be control the behavior via kernel command line ? Something like
> > > coresight_debug={on or 1} or
> > > even use the "nohlt" ?
> > 
> > We need to deal with the debug and CPU power domains.
> > 
> > For the former I suggest we do what coresight does and use the
> > "power-domains" binding[1].  For the CPU power domain we can re-use
> > the "nohlt" flag.  In the probe function if the "nohlt" cmd line flag
> > is not set the code bails out.  If it is set pm_runtime_put() is _not_
> > called and the driver can be used without worries of hanging the
> > system when the panic handler is invoked.
> > 
> > Am I forgetting something?
> 
> I tested this drvier on Hikey and DB410c. For Hikey we must pass
> "nohlt" to disable low power states, otherwise the kernel will hang
> during initialization. But for DB410c, this driver even can work well
> without "nohlt" and I checked the CPUIdle has been really enabled with
> below sysfs entries:
> 
> root@linaro-developer:~# cat /sys/devices/system/cpu/cpu*/cpuidle/state*/usage
> 5992
> 6988
> 4225
> 4547
> 2790
> 23696
> 4202
> 3899
> 
> And from my previous experience, I'm quite sure some SoCs can access
> the debug module registers with CPUIdle enabled, and it will read
> back 0xFFFF_FFFF_FFFF_FFFF when the CPU stays in low power state.
> So I prefer we could keep current method to suggest to use "nohlt" in
> Kconfig's help description but it's not mandotory to check this in
> the code. How about you think for this?

If we don't check for "nohlt" some platform may freeze, others may work.  If we
mandate that "nohlt" be present on the kernel cmd line it works in all cases.
As such mandating that "nohlt" be present is a better way to go.

Mathieu

> 
> Thanks,
> Leo Yan

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

#1603446

On Fri, Mar 17, 2017 at 09:50:07AM -0600, Mathieu Poirier wrote:

[...]

> If we don't check for "nohlt" some platform may freeze, others may work.  If we
> mandate that "nohlt" be present on the kernel cmd line it works in all cases.
> As such mandating that "nohlt" be present is a better way to go.

Sure, so I will add below checking code in the probe function, please
let me know if you have alter better way to implement this:

+       if (IS_ENABLED(CONFIG_CPU_IDLE) &&
+                       !strstr(boot_command_line, "nohlt")) {
+               dev_err(dev, "May not be accessible in CPU power domain.\n");
+               return -EPERM;
+       }

Thanks,
Leo Yan

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

#1603455

On 17/03/17 16:28, Leo Yan wrote:
> On Fri, Mar 17, 2017 at 09:50:07AM -0600, Mathieu Poirier wrote:
>
> [...]
>
>> If we don't check for "nohlt" some platform may freeze, others may work.  If we
>> mandate that "nohlt" be present on the kernel cmd line it works in all cases.
>> As such mandating that "nohlt" be present is a better way to go.
>
> Sure, so I will add below checking code in the probe function, please
> let me know if you have alter better way to implement this:
>
> +       if (IS_ENABLED(CONFIG_CPU_IDLE) &&
> +                       !strstr(boot_command_line, "nohlt")) {
> +               dev_err(dev, "May not be accessible in CPU power domain.\n");
> +               return -EPERM;
> +       }
>

There is an API which kind of achieves what "nohlt" does at runtime :

   cpu_idle_poll_ctrl(true)

So may be we could use that instead of depending on "nohlt". The other side
of the issues is "when do we decide to use the API". May be we could add something
like : enable_debug, which could then trigger the panic notifier registrations
and the above. That would still leave us with a case where the system crashes
even before the user gets a terminal. May be the following is the best option :

1) Dedicated kernel command line parameter for enabling the CPU debug at boot/probe.

and

2) Runtime enable method via sysfs.

What do you think ?

Suzuki

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

#1604473

On Fri, Mar 17, 2017 at 04:47:54PM +0000, Suzuki K Poulose wrote:
> On 17/03/17 16:28, Leo Yan wrote:
> >On Fri, Mar 17, 2017 at 09:50:07AM -0600, Mathieu Poirier wrote:
> >
> >[...]
> >
> >>If we don't check for "nohlt" some platform may freeze, others may work.  If we
> >>mandate that "nohlt" be present on the kernel cmd line it works in all cases.
> >>As such mandating that "nohlt" be present is a better way to go.
> >
> >Sure, so I will add below checking code in the probe function, please
> >let me know if you have alter better way to implement this:
> >
> >+       if (IS_ENABLED(CONFIG_CPU_IDLE) &&
> >+                       !strstr(boot_command_line, "nohlt")) {
> >+               dev_err(dev, "May not be accessible in CPU power domain.\n");
> >+               return -EPERM;
> >+       }
> >
> 
> There is an API which kind of achieves what "nohlt" does at runtime :
> 
>   cpu_idle_poll_ctrl(true)
> 
> So may be we could use that instead of depending on "nohlt". The other side
> of the issues is "when do we decide to use the API". May be we could add something
> like : enable_debug, which could then trigger the panic notifier registrations
> and the above. That would still leave us with a case where the system crashes
> even before the user gets a terminal. May be the following is the best option :
> 
> 1) Dedicated kernel command line parameter for enabling the CPU debug at boot/probe.
> and
> 
> 2) Runtime enable method via sysfs.

For upper methods, I think the benefit is the user can use very
explict method to enable debug module and can easily aware this
will totally disable CPU low power states as sequential result..

For sysfs, we need send IPIs to all CPUs to ensure all CPUs will not
stay in low power state anymore.

Now I have one questions mainly related with how to registser sysfs
node. Should register sysfs node based on coresight API
coresight_register()? If so, we should extend coresight core
structures to support CPU debug module. Or directly register sysfs
node by using sysfs node like /sys/bus/coresight/devices/cpu_debug?

Thanks,
Leo Yan

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

#1604788

On 17 March 2017 at 10:47, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 17/03/17 16:28, Leo Yan wrote:
>>
>> On Fri, Mar 17, 2017 at 09:50:07AM -0600, Mathieu Poirier wrote:
>>
>> [...]
>>
>>> If we don't check for "nohlt" some platform may freeze, others may work.
>>> If we
>>> mandate that "nohlt" be present on the kernel cmd line it works in all
>>> cases.
>>> As such mandating that "nohlt" be present is a better way to go.
>>
>>
>> Sure, so I will add below checking code in the probe function, please
>> let me know if you have alter better way to implement this:
>>
>> +       if (IS_ENABLED(CONFIG_CPU_IDLE) &&
>> +                       !strstr(boot_command_line, "nohlt")) {
>> +               dev_err(dev, "May not be accessible in CPU power
>> domain.\n");
>> +               return -EPERM;
>> +       }
>>
>
> There is an API which kind of achieves what "nohlt" does at runtime :
>
>   cpu_idle_poll_ctrl(true)
>
> So may be we could use that instead of depending on "nohlt". The other side
> of the issues is "when do we decide to use the API". May be we could add
> something
> like : enable_debug, which could then trigger the panic notifier
> registrations
> and the above. That would still leave us with a case where the system
> crashes
> even before the user gets a terminal. May be the following is the best
> option :
>
> 1) Dedicated kernel command line parameter for enabling the CPU debug at
> boot/probe.
>
> and
>
> 2) Runtime enable method via sysfs.
>
> What do you think ?

In my opinion booting with "nohlt" on the cmd line is sufficient to
determine if we should use the driver or not.  That way we also avoid
declaring yet another sysfs flag, something I really want to avoid.

Thanks,
Mathieu

>
> Suzuki
>

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

#1605271

On Mon, Mar 20, 2017 at 10:40:00AM -0600, Mathieu Poirier wrote:

[...]

> >>> If we don't check for "nohlt" some platform may freeze, others may work.
> >>> If we
> >>> mandate that "nohlt" be present on the kernel cmd line it works in all
> >>> cases.
> >>> As such mandating that "nohlt" be present is a better way to go.
> >>
> >>
> >> Sure, so I will add below checking code in the probe function, please
> >> let me know if you have alter better way to implement this:
> >>
> >> +       if (IS_ENABLED(CONFIG_CPU_IDLE) &&
> >> +                       !strstr(boot_command_line, "nohlt")) {
> >> +               dev_err(dev, "May not be accessible in CPU power
> >> domain.\n");
> >> +               return -EPERM;
> >> +       }
> >>
> >
> > There is an API which kind of achieves what "nohlt" does at runtime :
> >
> >   cpu_idle_poll_ctrl(true)
> >
> > So may be we could use that instead of depending on "nohlt". The other side
> > of the issues is "when do we decide to use the API". May be we could add
> > something
> > like : enable_debug, which could then trigger the panic notifier
> > registrations
> > and the above. That would still leave us with a case where the system
> > crashes
> > even before the user gets a terminal. May be the following is the best
> > option :
> >
> > 1) Dedicated kernel command line parameter for enabling the CPU debug at
> > boot/probe.
> >
> > and
> >
> > 2) Runtime enable method via sysfs.
> >
> > What do you think ?
> 
> In my opinion booting with "nohlt" on the cmd line is sufficient to
> determine if we should use the driver or not.  That way we also avoid
> declaring yet another sysfs flag, something I really want to avoid.

Agree.

I did spend some time to implement coresight core framework to support
debug module, you could see it on:http://termbin.com/k2fj; this also
gives me more sense which is better choice. If declaring another sysfs
flag to support debug module in coresight framework, this lets the
codes and interfaces more complex. E.g. for best fit into coresight
framework, finally we can get 8 sysfs nodes for 8 CPUs in system; so
that means we need enable every CPU one by one.

root@linaro-developer:~# ls /sys/bus/coresight/devices/
f6590000.debug  f6594000.debug  f65d0000.debug  f65d4000.debug
f6592000.debug  f6596000.debug  f65d2000.debug  f65d6000.debug

This is not quite reasonable to introduce complexity for either code
or using it. If we review coresight topology, some components need to
interact with each other and coresight framework should handle them
properly to enable/disable path, etc. But as Mathieu meantioned, from
the point of the hardware topology, the CPU debug module is quite
standalone under the coresight umbrella, and it has no any dependency
with other tracing or bus modules. So it's good to keep it simple,
this also matches with hardware implementation.

Thanks,
Leo Yan

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

#1605484

On 21/03/17 02:59, Leo Yan wrote:
> On Mon, Mar 20, 2017 at 10:40:00AM -0600, Mathieu Poirier wrote:
>
> [...]
>
>>>>> If we don't check for "nohlt" some platform may freeze, others may work.
>>>>> If we
>>>>> mandate that "nohlt" be present on the kernel cmd line it works in all
>>>>> cases.
>>>>> As such mandating that "nohlt" be present is a better way to go.
>>>>
>>>>
>>>> Sure, so I will add below checking code in the probe function, please
>>>> let me know if you have alter better way to implement this:
>>>>
>>>> +       if (IS_ENABLED(CONFIG_CPU_IDLE) &&
>>>> +                       !strstr(boot_command_line, "nohlt")) {
>>>> +               dev_err(dev, "May not be accessible in CPU power
>>>> domain.\n");
>>>> +               return -EPERM;
>>>> +       }
>>>>
>>>
>>> There is an API which kind of achieves what "nohlt" does at runtime :
>>>
>>>   cpu_idle_poll_ctrl(true)
>>>
>>> So may be we could use that instead of depending on "nohlt". The other side
>>> of the issues is "when do we decide to use the API". May be we could add
>>> something
>>> like : enable_debug, which could then trigger the panic notifier
>>> registrations
>>> and the above. That would still leave us with a case where the system
>>> crashes
>>> even before the user gets a terminal. May be the following is the best
>>> option :
>>>
>>> 1) Dedicated kernel command line parameter for enabling the CPU debug at
>>> boot/probe.
>>>
>>> and
>>>
>>> 2) Runtime enable method via sysfs.
>>>
>>> What do you think ?
>>
>> In my opinion booting with "nohlt" on the cmd line is sufficient to
>> determine if we should use the driver or not.  That way we also avoid
>> declaring yet another sysfs flag, something I really want to avoid.
>
> Agree.
>
> I did spend some time to implement coresight core framework to support
> debug module, you could see it on:http://termbin.com/k2fj; this also
> gives me more sense which is better choice. If declaring another sysfs
> flag to support debug module in coresight framework, this lets the
> codes and interfaces more complex. E.g. for best fit into coresight
> framework, finally we can get 8 sysfs nodes for 8 CPUs in system; so
> that means we need enable every CPU one by one.

Having a node for each debug area indeed doesn't look good. We could
as will stick a single node under /sys/kernel/debug/ which would enable/disable
the debug component.

I am OK with it being tied to nohlt. In that case we will have to add
a Kconfig dependency on GENERIC_IDLE_POLL_SETUP (though it is selected
by default on ARM/ARM64). Parsing the boot command line for nohlt doesn't
look like a good idea. We may have to figure out a way to do that. Also,
please could you add support for building this as a module ? Since it
doesn't depend on the coresight bus anyway, it should be pretty straight
forward.

Suzuki

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

#1605531

On Tue, Mar 21, 2017 at 10:16:45AM +0000, Suzuki K Poulose wrote:

[...]

> >>In my opinion booting with "nohlt" on the cmd line is sufficient to
> >>determine if we should use the driver or not.  That way we also avoid
> >>declaring yet another sysfs flag, something I really want to avoid.
> >
> >Agree.
> >
> >I did spend some time to implement coresight core framework to support
> >debug module, you could see it on:http://termbin.com/k2fj; this also
> >gives me more sense which is better choice. If declaring another sysfs
> >flag to support debug module in coresight framework, this lets the
> >codes and interfaces more complex. E.g. for best fit into coresight
> >framework, finally we can get 8 sysfs nodes for 8 CPUs in system; so
> >that means we need enable every CPU one by one.
> 
> Having a node for each debug area indeed doesn't look good. We could
> as will stick a single node under /sys/kernel/debug/ which would enable/disable
> the debug component.
> 
> I am OK with it being tied to nohlt. In that case we will have to add
> a Kconfig dependency on GENERIC_IDLE_POLL_SETUP (though it is selected
> by default on ARM/ARM64). Parsing the boot command line for nohlt doesn't
> look like a good idea. We may have to figure out a way to do that.

I remembered that we can use QoS constraint for CPUIdle:
        pm_qos_add_request(pm_qos_req, PM_QOS_CPU_DMA_LATENCY, 0);

Using this way we can disable all low power states and don't depend on
nohlt anymore.

> Also, please could you add support for building this as a module ? Since it
> doesn't depend on the coresight bus anyway, it should be pretty straight
> forward.

Yeah. Will support module building.

Thanks,
Leo Yan

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

#1605736

On Tue, Mar 21, 2017 at 07:47:11PM +0800, Leo Yan wrote:
> On Tue, Mar 21, 2017 at 10:16:45AM +0000, Suzuki K Poulose wrote:
> 
> [...]
> 
> > >>In my opinion booting with "nohlt" on the cmd line is sufficient to
> > >>determine if we should use the driver or not.  That way we also avoid
> > >>declaring yet another sysfs flag, something I really want to avoid.
> > >
> > >Agree.
> > >
> > >I did spend some time to implement coresight core framework to support
> > >debug module, you could see it on:http://termbin.com/k2fj; this also
> > >gives me more sense which is better choice. If declaring another sysfs
> > >flag to support debug module in coresight framework, this lets the
> > >codes and interfaces more complex. E.g. for best fit into coresight
> > >framework, finally we can get 8 sysfs nodes for 8 CPUs in system; so
> > >that means we need enable every CPU one by one.
> > 
> > Having a node for each debug area indeed doesn't look good. We could
> > as will stick a single node under /sys/kernel/debug/ which would enable/disable
> > the debug component.
> > 
> > I am OK with it being tied to nohlt. In that case we will have to add
> > a Kconfig dependency on GENERIC_IDLE_POLL_SETUP (though it is selected
> > by default on ARM/ARM64). Parsing the boot command line for nohlt doesn't
> > look like a good idea. We may have to figure out a way to do that.
> 
> I remembered that we can use QoS constraint for CPUIdle:
>         pm_qos_add_request(pm_qos_req, PM_QOS_CPU_DMA_LATENCY, 0);
> 
> Using this way we can disable all low power states and don't depend on
> nohlt anymore.

The idea is to use "nohlt" as a trigger option.  If it is set then we use this
driver - if it isn't then it is kept under wrap.

> 
> > Also, please could you add support for building this as a module ? Since it
> > doesn't depend on the coresight bus anyway, it should be pretty straight
> > forward.
> 
> Yeah. Will support module building.
> 
> Thanks,
> Leo Yan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web