lkml.org 
[lkml]   [2017]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [v3 3/5] coresight: add support for debug module
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

\
 
 \ /
  Last update: 2017-03-15 21:42    [W:0.105 / U:0.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site