Re: [PATCH] ARM: hw_breakpoint: Enable debug powerdown only if systemsupports 'has_ossr'

[Santosh Shilimkar]

On Monday 18 March 2013 10:36 PM, Will Deacon wrote:
> On Mon, Mar 18, 2013 at 03:46:28PM +0000, Santosh Shilimkar wrote:
>> On Monday 18 March 2013 08:37 PM, Will Deacon wrote:
>>> That really sucks :( Does this affect all OMAP-based boards?
>>>
>> All OMAP4 based boards..
> 
> Brilliant. Is there any way that the secure code can be fixed in future
> products?
> 
Nope. This can only be done with new silicon rev for GP devices which is
not going to happen. For secure devices, some secure patching is possible
but these are not development devices, so not much point.

>>>> +	/* Check if we have access to CPU debug features */
>>>> +	ARM_DBG_READ(c7, c14, 6, val);
>>>> +	if ((val & 0x1) == 0) {
>>>> +		pr_warn_once("CPU %d debug is unavailable\n", cpu);
>>>> +		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
>>>> +		return;
>>>> +	}
>>>
>>> There are a few of problems here:
>>>
>>> 	1. That is only checking for non-secure access, which precludes
>>> 	   running Linux in secure mode.
>>>
>> We can check bit 4 as well to take care linux running in secure mode.
> 
> It still doesn't help unless we know whether Linux is running secure or
> non-secure.
> 
ok.

>>> 	2. DBGAUTHSTATUS accesses are UNPREDICTABLE when the double-lock is
>>> 	   set for v7.1 processors.
>>>
>>> 	3. DBGAUTHSTATUS doesn't exist in ARMv6.
>>>
>> We cans skip the code for these versions like...
>> 	if (!ARM_DEBUG_ARCH_V7_ECP14 == get_debug_arch())
>> 		goto skip_dbgsts_read;
> 
> Sure, I was just pointing out that the code needs fixing for this.
> 
>>> 	4. CPUs without the security extensions have DBGAUTHSTATUS.NSE == 0
>>>
>> Which bit is that ? I don't find this bit in Cortex-A9 docs. With all
>> these checks, may be a separate function like 'is_debug_available()'
>> would be better.
> 
> NSE is bit 0 (the one you're checking).
> 
ok. So the subject patch might break those devices.

>>  
>>> 	5. Accessing DBGAUTHSTATUS requires DBGEN to be driven high.
>>> 	   Assuming that your pr_warn_once is emitted, this implies that
>>> 	   DBGEN is driven high from cold boot, yet the NSE bit is low,
>>> 	   implying that DBGEN is also low. That's contradictory, so I have
>>> 	   no idea what's going on...
>>>
>> Me neither. The warning is emitted at least.
> 
> Any chance you could follow up with your firmware/hardware guys about this
> please? I'd really like to understand how we end up in this state in case we
> can do something in the architecture to stop it from happening in future.
> 
I will check on this part and update you when I hear from them.

>>> Apart from that, it's fine!
>>>
>> If you agree, I can update patch accordingly.
>> BTW, do you have any better trick to take care of
>> above scenraio ?
> 
> Well, we could just add the warn_once prints but that doesn't stop debug
> from breaking after the first pm/suspend/hotplug operation.
> 
Probably we should drop the $subject patch approach and use warn_once
at least for current rc. Ofcourse it doesn't help to get working
hw_breakpoint support. Patch end of the email with warn_once.

Regards,
Santosh