[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Lokesh Vutla]

Commit {9a6eb31 ARM: hw_breakpoint: Debug powerdown support for self-hosted
debug} introduces debug powerdown support for self-hosted debug.
While merging the patch 'has_ossr' check was removed which
was needed for hardwares which doesn't support self-hosted debug.
Pandaboard (A9) is one such hardware and Dietmar's orginial
patch did mention this issue.
Without that check on Panda with CPUIDLE enabled, a flood of
below messages thrown.

[ 3.597930] hw-breakpoint: CPU 0 failed to disable vector catch
[ 3.597991] hw-breakpoint: CPU 1 failed to disable vector catch

So restore that check back to avoid the mentioned issue.

Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/kernel/hw_breakpoint.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 96093b7..0ba062d 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -1049,7 +1049,8 @@ static struct notifier_block __cpuinitdata dbg_cpu_pm_nb = {
 
 static void __init pm_init(void)
 {
-	cpu_pm_register_notifier(&dbg_cpu_pm_nb);
+	if (has_ossr)
+		cpu_pm_register_notifier(&dbg_cpu_pm_nb);
 }
 #else
 static inline void pm_init(void)
-- 
1.7.9.5

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Dietmar Eggemann]

On 13/03/13 06:52, Lokesh Vutla wrote:
> Commit {9a6eb31 ARM: hw_breakpoint: Debug powerdown support for self-hosted
> debug} introduces debug powerdown support for self-hosted debug.
> While merging the patch 'has_ossr' check was removed which
> was needed for hardwares which doesn't support self-hosted debug.
> Pandaboard (A9) is one such hardware and Dietmar's orginial
> patch did mention this issue.
> Without that check on Panda with CPUIDLE enabled, a flood of
> below messages thrown.
>
> [ 3.597930] hw-breakpoint: CPU 0 failed to disable vector catch
> [ 3.597991] hw-breakpoint: CPU 1 failed to disable vector catch
>

Hi Lokesh,

I confirm that this has_ossr condition has to go back into the
pm_init(void) call. I just verified it again on my Panda board and I get
the same issue like you without it.

I guess what was happening is that Will asked me if this check is really
necessary and I said No without re-testing on my Panda board as an V7
debug architecture example where OSSR is not implemented. Since then I
only tested in on V7.1 debug architectures where OSSR is mandatory.
Sorry about this and thanks for catching this.

You can add my Acked-by: if you wish.

-- Dietmar

> So restore that check back to avoid the mentioned issue.
>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>   arch/arm/kernel/hw_breakpoint.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index 96093b7..0ba062d 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -1049,7 +1049,8 @@ static struct notifier_block __cpuinitdata dbg_cpu_pm_nb = {
>
>   static void __init pm_init(void)
>   {
> -     cpu_pm_register_notifier(&dbg_cpu_pm_nb);
> +     if (has_ossr)
> +             cpu_pm_register_notifier(&dbg_cpu_pm_nb);
>   }
>   #else
>   static inline void pm_init(void)
>

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Lokesh Vutla]

Hi Dietmar,
On Wednesday 13 March 2013 05:35 PM, Dietmar Eggemann wrote:
> On 13/03/13 06:52, Lokesh Vutla wrote:
>> Commit {9a6eb31 ARM: hw_breakpoint: Debug powerdown support for
>> self-hosted
>> debug} introduces debug powerdown support for self-hosted debug.
>> While merging the patch 'has_ossr' check was removed which
>> was needed for hardwares which doesn't support self-hosted debug.
>> Pandaboard (A9) is one such hardware and Dietmar's orginial
>> patch did mention this issue.
>> Without that check on Panda with CPUIDLE enabled, a flood of
>> below messages thrown.
>>
>> [ 3.597930] hw-breakpoint: CPU 0 failed to disable vector catch
>> [ 3.597991] hw-breakpoint: CPU 1 failed to disable vector catch
>>
>
> Hi Lokesh,
>
> I confirm that this has_ossr condition has to go back into the
> pm_init(void) call. I just verified it again on my Panda board and I get
> the same issue like you without it.
>
> I guess what was happening is that Will asked me if this check is really
> necessary and I said No without re-testing on my Panda board as an V7
> debug architecture example where OSSR is not implemented. Since then I
> only tested in on V7.1 debug architectures where OSSR is mandatory.
> Sorry about this and thanks for catching this.
Thanks for confirming..:)

Regards,
Lokesh

>
> You can add my Acked-by: if you wish.
>
> -- Dietmar
>
>> So restore that check back to avoid the mentioned issue.
>>
>> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>   arch/arm/kernel/hw_breakpoint.c |    3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kernel/hw_breakpoint.c
>> b/arch/arm/kernel/hw_breakpoint.c
>> index 96093b7..0ba062d 100644
>> --- a/arch/arm/kernel/hw_breakpoint.c
>> +++ b/arch/arm/kernel/hw_breakpoint.c
>> @@ -1049,7 +1049,8 @@ static struct notifier_block __cpuinitdata
>> dbg_cpu_pm_nb = {
>>
>>   static void __init pm_init(void)
>>   {
>> -     cpu_pm_register_notifier(&dbg_cpu_pm_nb);
>> +     if (has_ossr)
>> +             cpu_pm_register_notifier(&dbg_cpu_pm_nb);
>>   }
>>   #else
>>   static inline void pm_init(void)
>>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy
> the information in any medium.  Thank you.
>

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Santosh Shilimkar]

Will,

On Wednesday 13 March 2013 05:59 PM, Lokesh Vutla wrote:
> Hi Dietmar,
> On Wednesday 13 March 2013 05:35 PM, Dietmar Eggemann wrote:
>> On 13/03/13 06:52, Lokesh Vutla wrote:
>>> Commit {9a6eb31 ARM: hw_breakpoint: Debug powerdown support for
>>> self-hosted
>>> debug} introduces debug powerdown support for self-hosted debug.
>>> While merging the patch 'has_ossr' check was removed which
>>> was needed for hardwares which doesn't support self-hosted debug.
>>> Pandaboard (A9) is one such hardware and Dietmar's orginial
>>> patch did mention this issue.
>>> Without that check on Panda with CPUIDLE enabled, a flood of
>>> below messages thrown.
>>>
>>> [ 3.597930] hw-breakpoint: CPU 0 failed to disable vector catch
>>> [ 3.597991] hw-breakpoint: CPU 1 failed to disable vector catch
>>>
>>
>> Hi Lokesh,
>>
>> I confirm that this has_ossr condition has to go back into the
>> pm_init(void) call. I just verified it again on my Panda board and I get
>> the same issue like you without it.
>>
>> I guess what was happening is that Will asked me if this check is really
>> necessary and I said No without re-testing on my Panda board as an V7
>> debug architecture example where OSSR is not implemented. Since then I
>> only tested in on V7.1 debug architectures where OSSR is mandatory.
>> Sorry about this and thanks for catching this.
> Thanks for confirming..:)
> 
Can you please queue this one for 3.9-rc2+ ? Without this patch
CPUIDLE is unusable on OMAP4 devices because of those flood
of warning messages.

I was also wondering whether we should just warn once rather
than continuous warnings in the notifier. Patch is end of the
email.

Regards,
Santosh

>From b8db63f786719aef835f1ef4e20f3b3406b99b62 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Thu, 14 Mar 2013 13:03:25 +0530
Subject: [PATCH] ARM: hw_breakpoints: Use warn_once to avoid message flood on
 unsupported ossr machines

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/kernel/hw_breakpoint.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 96093b7..5dc1aa6 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -966,7 +966,7 @@ static void reset_ctrl_regs(void *unused)
 	}
 
 	if (err) {
-		pr_warning("CPU %d debug is powered down!\n", cpu);
+		pr_warn_once("CPU %d debug is powered down!\n", cpu);
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 		return;
 	}
@@ -987,7 +987,7 @@ clear_vcr:
 	isb();
 
 	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
-		pr_warning("CPU %d failed to disable vector catch\n", cpu);
+		pr_warn_once("CPU %d failed to disable vector catch\n", cpu);
 		return;
 	}
 
@@ -1007,7 +1007,7 @@ clear_vcr:
 	}
 
 	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
-		pr_warning("CPU %d failed to clear debug register pairs\n", cpu);
+		pr_warn_once("CPU %d failed to clear debug register pairs\n", cpu);
 		return;
 	}
 
-- 
1.7.9.5

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Will Deacon]

On Thu, Mar 14, 2013 at 01:08:00PM +0530, Santosh Shilimkar wrote:
> Will,

Hi guys,

I'm out of the office at the moment and have really terrible connectivity,
so I can't do too much until next week. However, I don't think adding the
has_ossr check is the right fix for this problem.

> On Wednesday 13 March 2013 05:59 PM, Lokesh Vutla wrote:
> > Hi Dietmar,
> > On Wednesday 13 March 2013 05:35 PM, Dietmar Eggemann wrote:
> >> On 13/03/13 06:52, Lokesh Vutla wrote:
> >>> Commit {9a6eb31 ARM: hw_breakpoint: Debug powerdown support for
> >>> self-hosted
> >>> debug} introduces debug powerdown support for self-hosted debug.
> >>> While merging the patch 'has_ossr' check was removed which
> >>> was needed for hardwares which doesn't support self-hosted debug.
> >>> Pandaboard (A9) is one such hardware and Dietmar's orginial
> >>> patch did mention this issue.
> >>> Without that check on Panda with CPUIDLE enabled, a flood of
> >>> below messages thrown.
> >>>
> >>> [ 3.597930] hw-breakpoint: CPU 0 failed to disable vector catch
> >>> [ 3.597991] hw-breakpoint: CPU 1 failed to disable vector catch

Ok, so this means that we've taken an undefined instruction exception while
trying to reset the debug registers on the PM_EXIT path. Now, the code there
deals with CPUs that don't have the save/restore registers just fine, so
that shouldn't have anything to do with this problem, particularly if the
bit that is tripping us up is related to clearing vector catch.

Furthermore, I was under the impression that hw_breakpoint did actually
work on panda, which implies that a cold boot *does* manage to reset the
registers (can you please confirm this by looking in your dmesg during
boot?). In that case, it seems as though a PM cycle is powering down a
bunch of debug logic that was powered up during boot, and then we trip over
because we can't access the register bank.

The proper solution to this problem requires us to establish exactly what is
turning off the debug registers, and then having an OMAP PM notifier to
enable it again. Assuming this has always been the case, I expect hardware
debug across PM fails silently with older kernels.

> I was also wondering whether we should just warn once rather
> than continuous warnings in the notifier. Patch is end of the
> email.

Could do, but I'd like to see a fix for the real issue before we simply hide
the warnings :)

Cheers,

Will

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Santosh Shilimkar]

On Friday 15 March 2013 10:30 AM, Will Deacon wrote:
> On Thu, Mar 14, 2013 at 01:08:00PM +0530, Santosh Shilimkar wrote:
>> Will,
> 
> Hi guys,
> 
> I'm out of the office at the moment and have really terrible connectivity,
> so I can't do too much until next week. However, I don't think adding the
> has_ossr check is the right fix for this problem.
> 
>> On Wednesday 13 March 2013 05:59 PM, Lokesh Vutla wrote:
>>> Hi Dietmar,
>>> On Wednesday 13 March 2013 05:35 PM, Dietmar Eggemann wrote:
>>>> On 13/03/13 06:52, Lokesh Vutla wrote:
>>>>> Commit {9a6eb31 ARM: hw_breakpoint: Debug powerdown support for
>>>>> self-hosted
>>>>> debug} introduces debug powerdown support for self-hosted debug.
>>>>> While merging the patch 'has_ossr' check was removed which
>>>>> was needed for hardwares which doesn't support self-hosted debug.
>>>>> Pandaboard (A9) is one such hardware and Dietmar's orginial
>>>>> patch did mention this issue.
>>>>> Without that check on Panda with CPUIDLE enabled, a flood of
>>>>> below messages thrown.
>>>>>
>>>>> [ 3.597930] hw-breakpoint: CPU 0 failed to disable vector catch
>>>>> [ 3.597991] hw-breakpoint: CPU 1 failed to disable vector catch
> 
> Ok, so this means that we've taken an undefined instruction exception while
> trying to reset the debug registers on the PM_EXIT path. Now, the code there
> deals with CPUs that don't have the save/restore registers just fine, so
> that shouldn't have anything to do with this problem, particularly if the
> bit that is tripping us up is related to clearing vector catch.
>
Agree.
 
> Furthermore, I was under the impression that hw_breakpoint did actually
> work on panda, which implies that a cold boot *does* manage to reset the
> registers (can you please confirm this by looking in your dmesg during
> boot?). In that case, it seems as though a PM cycle is powering down a
> bunch of debug logic that was powered up during boot, and then we trip over
> because we can't access the register bank.
> 
Actually it seems to be without PM. Thanks to analysis from Lokesh, the issue
can be seen even with just suspend or cpu hotplug. So cold boot as such is
fine.

> The proper solution to this problem requires us to establish exactly what is
> turning off the debug registers, and then having an OMAP PM notifier to
> enable it again. Assuming this has always been the case, I expect hardware
> debug across PM fails silently with older kernels.
> 
This has been always the case it seems with CPU power cycle.
After the CPU is power cycled, 'DBGAUTHSTATUS' reads '0xaa' rather
than '0xaf' which means 'DBGEN = 0' and hence code fails to enable
monitor mode. This happens on both secure and GP devices and it can not
be patched since the secure code is ROM'ed. We didn't notice so far
because hw_breakpoint support was not default enabled on OMAP till the
multi-platform build.

>> I was also wondering whether we should just warn once rather
>> than continuous warnings in the notifier. Patch is end of the
>> email.
> 
> Could do, but I'd like to see a fix for the real issue before we simply hide
> the warnings :)
> 
Agree here too. As evident above, the feature won't work on OMAP4
devices with PM and hence some solution is needed.

What you think of below ?


>From d74b4264f6a5967b0f7ada96aad77ab0ac30dbed Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Mon, 18 Mar 2013 11:59:04 +0530
Subject: [PATCH] ARM: hw_breakpoints: Check for CPU debug availability before
 enabling it

CPU debug features like hardware break, watchpoints can be used only when
the debug mode is enabled and available for non-secure mode.

Hence check 'DBGAUTHSTATUS.DBGEN' before proceeding to enable the
features.

Thanks to Will for pointers and Lokesh for the analysis of the issue on
OMAP4 where after a CPU power cycle, debug mode gets disabled.

Cc: Will Deacon <Will.Deacon@arm.com>

Tested-by: Lokesh Vutla <lokeshvutla@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/kernel/hw_breakpoint.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 96093b7..683a7cf 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -930,6 +930,14 @@ static void reset_ctrl_regs(void *unused)
 	int i, raw_num_brps, err = 0, cpu = smp_processor_id();
 	u32 val;
 
+	/* 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;
+	}
+
 	/*
 	 * v7 debug contains save and restore registers so that debug state
 	 * can be maintained across low-power modes without leaving the debug
-- 
1.7.9.5

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Will Deacon]

Hi Santosh,

On Mon, Mar 18, 2013 at 06:51:30AM +0000, Santosh Shilimkar wrote:
> On Friday 15 March 2013 10:30 AM, Will Deacon wrote:
> > Furthermore, I was under the impression that hw_breakpoint did actually
> > work on panda, which implies that a cold boot *does* manage to reset the
> > registers (can you please confirm this by looking in your dmesg during
> > boot?). In that case, it seems as though a PM cycle is powering down a
> > bunch of debug logic that was powered up during boot, and then we trip over
> > because we can't access the register bank.
> > 
> Actually it seems to be without PM. Thanks to analysis from Lokesh, the issue
> can be seen even with just suspend or cpu hotplug. So cold boot as such is
> fine.

Right, so what you're saying is that monitor-mode hardware debug only works
until the first pm/suspend/hotplug operation, then it's dead in the water?

> > The proper solution to this problem requires us to establish exactly what is
> > turning off the debug registers, and then having an OMAP PM notifier to
> > enable it again. Assuming this has always been the case, I expect hardware
> > debug across PM fails silently with older kernels.
> > 
> This has been always the case it seems with CPU power cycle.
> After the CPU is power cycled, 'DBGAUTHSTATUS' reads '0xaa' rather
> than '0xaf' which means 'DBGEN = 0' and hence code fails to enable
> monitor mode. This happens on both secure and GP devices and it can not
> be patched since the secure code is ROM'ed. We didn't notice so far
> because hw_breakpoint support was not default enabled on OMAP till the
> multi-platform build.

That really sucks :( Does this affect all OMAP-based boards?

> >> I was also wondering whether we should just warn once rather
> >> than continuous warnings in the notifier. Patch is end of the
> >> email.
> > 
> > Could do, but I'd like to see a fix for the real issue before we simply hide
> > the warnings :)
> > 
> Agree here too. As evident above, the feature won't work on OMAP4
> devices with PM and hence some solution is needed.
> 
> What you think of below ?

Comments inline...

> 
> From d74b4264f6a5967b0f7ada96aad77ab0ac30dbed Mon Sep 17 00:00:00 2001
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date: Mon, 18 Mar 2013 11:59:04 +0530
> Subject: [PATCH] ARM: hw_breakpoints: Check for CPU debug availability before
>  enabling it
> 
> CPU debug features like hardware break, watchpoints can be used only when
> the debug mode is enabled and available for non-secure mode.
> 
> Hence check 'DBGAUTHSTATUS.DBGEN' before proceeding to enable the
> features.
> 
> Thanks to Will for pointers and Lokesh for the analysis of the issue on
> OMAP4 where after a CPU power cycle, debug mode gets disabled.
> 
> Cc: Will Deacon <Will.Deacon@arm.com>
> 
> Tested-by: Lokesh Vutla <lokeshvutla@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  arch/arm/kernel/hw_breakpoint.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index 96093b7..683a7cf 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -930,6 +930,14 @@ static void reset_ctrl_regs(void *unused)
>  	int i, raw_num_brps, err = 0, cpu = smp_processor_id();
>  	u32 val;
>  
> +	/* 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.

	2. DBGAUTHSTATUS accesses are UNPREDICTABLE when the double-lock is
	   set for v7.1 processors.

	3. DBGAUTHSTATUS doesn't exist in ARMv6.

	4. CPUs without the security extensions have DBGAUTHSTATUS.NSE == 0

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

Apart from that, it's fine!

Will

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Santosh Shilimkar]

On Monday 18 March 2013 08:37 PM, Will Deacon wrote:
> Hi Santosh,
> 
> On Mon, Mar 18, 2013 at 06:51:30AM +0000, Santosh Shilimkar wrote:
>> On Friday 15 March 2013 10:30 AM, Will Deacon wrote:
>>> Furthermore, I was under the impression that hw_breakpoint did actually
>>> work on panda, which implies that a cold boot *does* manage to reset the
>>> registers (can you please confirm this by looking in your dmesg during
>>> boot?). In that case, it seems as though a PM cycle is powering down a
>>> bunch of debug logic that was powered up during boot, and then we trip over
>>> because we can't access the register bank.
>>>
>> Actually it seems to be without PM. Thanks to analysis from Lokesh, the issue
>> can be seen even with just suspend or cpu hotplug. So cold boot as such is
>> fine.
> 
> Right, so what you're saying is that monitor-mode hardware debug only works
> until the first pm/suspend/hotplug operation, then it's dead in the water?
> 
>>> The proper solution to this problem requires us to establish exactly what is
>>> turning off the debug registers, and then having an OMAP PM notifier to
>>> enable it again. Assuming this has always been the case, I expect hardware
>>> debug across PM fails silently with older kernels.
>>>
>> This has been always the case it seems with CPU power cycle.
>> After the CPU is power cycled, 'DBGAUTHSTATUS' reads '0xaa' rather
>> than '0xaf' which means 'DBGEN = 0' and hence code fails to enable
>> monitor mode. This happens on both secure and GP devices and it can not
>> be patched since the secure code is ROM'ed. We didn't notice so far
>> because hw_breakpoint support was not default enabled on OMAP till the
>> multi-platform build.
> 
> That really sucks :( Does this affect all OMAP-based boards?
> 
All OMAP4 based boards..

>>>> I was also wondering whether we should just warn once rather
>>>> than continuous warnings in the notifier. Patch is end of the
>>>> email.
>>>
>>> Could do, but I'd like to see a fix for the real issue before we simply hide
>>> the warnings :)
>>>
>> Agree here too. As evident above, the feature won't work on OMAP4
>> devices with PM and hence some solution is needed.
>>
>> What you think of below ?
> 
> Comments inline...
> 
>>
>> From d74b4264f6a5967b0f7ada96aad77ab0ac30dbed Mon Sep 17 00:00:00 2001
>> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Date: Mon, 18 Mar 2013 11:59:04 +0530
>> Subject: [PATCH] ARM: hw_breakpoints: Check for CPU debug availability before
>>  enabling it
>>
>> CPU debug features like hardware break, watchpoints can be used only when
>> the debug mode is enabled and available for non-secure mode.
>>
>> Hence check 'DBGAUTHSTATUS.DBGEN' before proceeding to enable the
>> features.
>>
>> Thanks to Will for pointers and Lokesh for the analysis of the issue on
>> OMAP4 where after a CPU power cycle, debug mode gets disabled.
>>
>> Cc: Will Deacon <Will.Deacon@arm.com>
>>
>> Tested-by: Lokesh Vutla <lokeshvutla@ti.com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>  arch/arm/kernel/hw_breakpoint.c |    8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
>> index 96093b7..683a7cf 100644
>> --- a/arch/arm/kernel/hw_breakpoint.c
>> +++ b/arch/arm/kernel/hw_breakpoint.c
>> @@ -930,6 +930,14 @@ static void reset_ctrl_regs(void *unused)
>>  	int i, raw_num_brps, err = 0, cpu = smp_processor_id();
>>  	u32 val;
>>  
>> +	/* 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.

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

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

Regards,
Santosh

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Will Deacon]

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?

> >> +	/* 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.

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

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

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

Will

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports '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

>From 6611d48eb5571e3e094c7a9c2479e652b37d35e3 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Tue, 19 Mar 2013 11:53:41 +0530
Subject: [PATCH] ARM: hw_breakpoints: Use warn_once to avoid debug message
 flood from reset_ctrl_regs()

CPU debug features like hardware break, watchpoints can be used only when
the debug mode is enabled and available. Unfortunately on OMAP4 based device,
after a CPU power cycle, the debug feature gets disabled which leads to
a flood of messages coming from reset_ctrl_regs() which gets called on
every CPU_PM_EXIT with CPUidle enabled.

So make use of warn_once() so that system is usable.

Thanks to Will for pointers and Lokesh for the analysis of the issue.

Cc: Will Deacon <Will.Deacon@arm.com>

Tested-by: Lokesh Vutla <lokeshvutla@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/kernel/hw_breakpoint.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 96093b7..5dc1aa6 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -966,7 +966,7 @@ static void reset_ctrl_regs(void *unused)
 	}
 
 	if (err) {
-		pr_warning("CPU %d debug is powered down!\n", cpu);
+		pr_warn_once("CPU %d debug is powered down!\n", cpu);
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 		return;
 	}
@@ -987,7 +987,7 @@ clear_vcr:
 	isb();
 
 	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
-		pr_warning("CPU %d failed to disable vector catch\n", cpu);
+		pr_warn_once("CPU %d failed to disable vector catch\n", cpu);
 		return;
 	}
 
@@ -1007,7 +1007,7 @@ clear_vcr:
 	}
 
 	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
-		pr_warning("CPU %d failed to clear debug register pairs\n", cpu);
+		pr_warn_once("CPU %d failed to clear debug register pairs\n", cpu);
 		return;
 	}
 
-- 
1.7.9.5

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Will Deacon]

On Tue, Mar 19, 2013 at 06:39:38AM +0000, Santosh Shilimkar wrote:
> On Monday 18 March 2013 10:36 PM, Will Deacon wrote:
> > 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.

Ok, thanks.

Dietmar -- I seem to remember that you thought GDB did actually work with
hardware breakpoints on Pandaboard across low-power states. Can you
confirm/deny this please?

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

Yeah, I'll merge that, but it's a real shame that this breaks hardware debug
on one of the few platforms where it was reported to work.

Will

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Santosh Shilimkar]

Will,

On Tuesday 19 March 2013 03:58 PM, Will Deacon wrote:
> On Tue, Mar 19, 2013 at 06:39:38AM +0000, Santosh Shilimkar wrote:
>> On Monday 18 March 2013 10:36 PM, Will Deacon wrote:

[..]

>>> 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.
> 
> Yeah, I'll merge that, but it's a real shame that this breaks hardware debug
> on one of the few platforms where it was reported to work.
> 
Are you going to send the patch for 3.9-rcx ? As I said before without the
patch OMAP4 CPUILDE is unusable because of that debug noise and hence it
will be good to get that patch in

Regards,
Santosh

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Will Deacon]

On Mon, Mar 25, 2013 at 09:11:00AM +0000, Santosh Shilimkar wrote:
> Will,

Hi Santosh,

> Are you going to send the patch for 3.9-rcx ? As I said before without the
> patch OMAP4 CPUILDE is unusable because of that debug noise and hence it
> will be good to get that patch in

It's in Russell's tree, so should be queued for mainline fairly soon.

Will

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Santosh Shilimkar]

On Monday 25 March 2013 04:19 PM, Will Deacon wrote:
> On Mon, Mar 25, 2013 at 09:11:00AM +0000, Santosh Shilimkar wrote:
>> Will,
> 
> Hi Santosh,
> 
>> Are you going to send the patch for 3.9-rcx ? As I said before without the
>> patch OMAP4 CPUILDE is unusable because of that debug noise and hence it
>> will be good to get that patch in
> 
> It's in Russell's tree, so should be queued for mainline fairly soon.
> 
Thanks Will !!

Regards
Santosh

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Dietmar Eggemann]

On 19/03/13 10:28, Will Deacon wrote:
> On Tue, Mar 19, 2013 at 06:39:38AM +0000, Santosh Shilimkar wrote:
>> On Monday 18 March 2013 10:36 PM, Will Deacon wrote:
>>> 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.
>
> Ok, thanks.
>
> Dietmar -- I seem to remember that you thought GDB did actually work with
> hardware breakpoints on Pandaboard across low-power states. Can you
> confirm/deny this please?

Sorry for the late response.

I did some testing on my Pandaboard trying to set hwbp from gdb.

System is a vanilla Linaro 13.02.
gdb is (GNU gdb (GDB) 7.5.1), build from tag gdb_7_5_1-2012-11-29-release

(gdb) hb *0x00008440
Hardware assisted breakpoint 2 at 0x8440: file test.c, line 14.
(gdb) c
Continuing.
Unexpected error setting breakpoint address: No such device.

With additional kernel logs:

[<c0011597>] (unwind_backtrace+0x1/0x92) from [<c0012603>]
(arch_validate_hwbkpt_settings+0x23/0x1d4)
[<c0012603>] (arch_validate_hwbkpt_settings+0x23/0x1d4) from
[<c0088aa9>] (register_perf_hw_breakpoint+0x19/0x30)
[<c0088aa9>] (register_perf_hw_breakpoint+0x19/0x30) from [<c0088ae9>]
(hw_breakpoint_event_init+0x29/0x48)
[<c0088ae9>] (hw_breakpoint_event_init+0x29/0x48) from [<c0086c81>]
(perf_init_event+0x79/0xcc)
[<c0086c81>] (perf_init_event+0x79/0xcc) from [<c0086e97>]
(perf_event_alloc+0x1c3/0x32c)
[<c0086e97>] (perf_event_alloc+0x1c3/0x32c) from [<c00871eb>]
(perf_event_create_kernel_counter+0x19/0xc6)
[<c00871eb>] (perf_event_create_kernel_counter+0x19/0xc6) from
[<c00885e9>] (register_user_hw_breakpoint+0x2d/0x38)
[<c00885e9>] (register_user_hw_breakpoint+0x2d/0x38) from [<c03dd723>]
(ptrace_hbp_create+0x53/0x5c)
[<c03dd723>] (ptrace_hbp_create+0x53/0x5c) from [<c000e0d7>]
(ptrace_sethbpregs+0x95/0x1a6)
[<c000e0d7>] (ptrace_sethbpregs+0x95/0x1a6) from [<c000e777>]
(arch_ptrace+0x3bf/0x3ec)
[<c000e777>] (arch_ptrace+0x3bf/0x3ec) from [<c002f621>]
(sys_ptrace+0x251/0x278)
[<c002f621>] (sys_ptrace+0x251/0x278) from [<c000c781>]
(ret_fast_syscall+0x1/0x52)
perf_event_create_kernel_counter:6721 ret=-19
ptrace_sethbpregs:557 ret=-19

The call to monitor_mode_enabled() in arch_validate_hwbkpt_settings()
fails since DSCR.15 MDBGen is not set:

monitor_mode_enabled:239 dscr=0x1030002

I don't get the warning "Failed to enable monitor mode on CPU" during
system start-up though.

Doing the same on my TC2 gives me:

monitor_mode_enabled:239 dscr=0x2008002

So I guess that setting hwbp from self-hosted debugger on Pandaboard is
not possible at all.

There is no CPUidle enabled on Linaro 13.02 Pandaboard.

-- Dietmar


>
>>> 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.
>
> Yeah, I'll merge that, but it's a real shame that this breaks hardware debug
> on one of the few platforms where it was reported to work.
>
> Will
>

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

[PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

[Kevin Hilman]

Lokesh Vutla <lokeshvutla@ti.com> writes:

> Commit {9a6eb31 ARM: hw_breakpoint: Debug powerdown support for self-hosted
> debug} introduces debug powerdown support for self-hosted debug.
> While merging the patch 'has_ossr' check was removed which
> was needed for hardwares which doesn't support self-hosted debug.
> Pandaboard (A9) is one such hardware and Dietmar's orginial
> patch did mention this issue.
> Without that check on Panda with CPUIDLE enabled, a flood of
> below messages thrown.
>
> [ 3.597930] hw-breakpoint: CPU 0 failed to disable vector catch
> [ 3.597991] hw-breakpoint: CPU 1 failed to disable vector catch
>
> So restore that check back to avoid the mentioned issue.
>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>

I just noticed this on my Panda boards with CPUidle enabled, and
$SUBJECT patch fixes it.

FWIW,

Tested-by: Kevin Hilman <khilman@linaro.org>

I agree that this should get queued for v3.9-rc.

Kevin