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

Commit Message

Santosh Shilimkar March 14, 2013, 7:38 a.m. UTC

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 <[e-mail]>
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 <[e-mail]>
---
 arch/arm/kernel/hw_breakpoint.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Will Deacon March 15, 2013, 5 a.m. UTC | #1

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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