diff mbox

fix DEBUG_LL DCC race condition

Message ID 20121029151832.GA2162@sig21.net (mailing list archive)
State New, archived
Headers show

Commit Message

Johannes Stezenbach Oct. 29, 2012, 3:18 p.m. UTC
Trying to boot a kernel with I- and D-caches disabled
sometimes hangs when DEBUG_LL output to DCC is enabled.
Apparently the JTAG debugger sometimes reads the
DCC register before busyuart could see the wDTRfull flag,
thus busyuart spins in an endless loop.

The reason seems to be a misunderstanding of the purpose
of the busyuart macro.  For UART, waituart waits until
there is space in the FIFO, and busyuart waits until
the FIFO is empty (all data is sent).
For DCC, busyuart should be identical to waituart since
there is no FIFO.

Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
Only tested on ARMv6.
e.g. arch/arm/mach-at91/include/mach/debug-macro.S has some
comments which clarify what busyuart is supposed to be doing.

Comments

Tony Lindgren Oct. 31, 2012, 4:27 p.m. UTC | #1
* Johannes Stezenbach <js@sig21.net> [121029 08:20]:
> Trying to boot a kernel with I- and D-caches disabled
> sometimes hangs when DEBUG_LL output to DCC is enabled.
> Apparently the JTAG debugger sometimes reads the
> DCC register before busyuart could see the wDTRfull flag,
> thus busyuart spins in an endless loop.
> 
> The reason seems to be a misunderstanding of the purpose
> of the busyuart macro.  For UART, waituart waits until
> there is space in the FIFO, and busyuart waits until
> the FIFO is empty (all data is sent).
> For DCC, busyuart should be identical to waituart since
> there is no FIFO.
> 
> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> ---
> Only tested on ARMv6.
> e.g. arch/arm/mach-at91/include/mach/debug-macro.S has some
> comments which clarify what busyuart is supposed to be doing.

Your explanation makes sense to me:

Acked-by: Tony Lindgren <tony@atomide.com>
Russell King - ARM Linux Oct. 31, 2012, 4:36 p.m. UTC | #2
On Mon, Oct 29, 2012 at 04:18:32PM +0100, Johannes Stezenbach wrote:
> Trying to boot a kernel with I- and D-caches disabled
> sometimes hangs when DEBUG_LL output to DCC is enabled.
> Apparently the JTAG debugger sometimes reads the
> DCC register before busyuart could see the wDTRfull flag,
> thus busyuart spins in an endless loop.

This makes no sense.  Why is busyuart spinning if it does _not_ see a
full flag?  busyuart is supposed to spin _if_ the UART has data to be
sent.  It's not supposed to spin if the data has been sent.

> The reason seems to be a misunderstanding of the purpose
> of the busyuart macro.  For UART, waituart waits until
> there is space in the FIFO, and busyuart waits until
> the FIFO is empty (all data is sent).
> For DCC, busyuart should be identical to waituart since
> there is no FIFO.

Not quite.  waituart is supposed to check that the flow control allows
the character to be sent _or_ it's supposed to do nothing.  Look at
the 8250 implementation debug-8250.S - this is the first implementation,
authored by me, and therefore is the definitive implementation for this
stuff.
Johannes Stezenbach Oct. 31, 2012, 5:08 p.m. UTC | #3
On Wed, Oct 31, 2012 at 04:36:15PM +0000, Russell King - ARM Linux wrote:
> On Mon, Oct 29, 2012 at 04:18:32PM +0100, Johannes Stezenbach wrote:
> > Trying to boot a kernel with I- and D-caches disabled
> > sometimes hangs when DEBUG_LL output to DCC is enabled.
> > Apparently the JTAG debugger sometimes reads the
> > DCC register before busyuart could see the wDTRfull flag,
> > thus busyuart spins in an endless loop.
> 
> This makes no sense.  Why is busyuart spinning if it does _not_ see a
> full flag?  busyuart is supposed to spin _if_ the UART has data to be
> sent.  It's not supposed to spin if the data has been sent.

Maybe my wording didn't make it clear that the current code
spins if the data has been sent (which indeed does not
make sense).  My fix changes it to spin _until_ the data has been send.

> > The reason seems to be a misunderstanding of the purpose
> > of the busyuart macro.  For UART, waituart waits until
> > there is space in the FIFO, and busyuart waits until
> > the FIFO is empty (all data is sent).
> > For DCC, busyuart should be identical to waituart since
> > there is no FIFO.
> 
> Not quite.  waituart is supposed to check that the flow control allows
> the character to be sent _or_ it's supposed to do nothing.  Look at
> the 8250 implementation debug-8250.S - this is the first implementation,
> authored by me, and therefore is the definitive implementation for this
> stuff.

OK, in the case of DCC "flow-control" would mean JTAG has
read the DCC register.  So it seems correct that both
busyuart and waituart do the same:  wait for empty DCC register.
But I agree that the double wait is superflous, waituart
could be changed to do nothing.  This would be in line with
debug-8250.S which doesn't check for space in the FIFO
in waituart.

I'll send an updated patch.

Thanks,
Johannes
diff mbox

Patch

diff --git a/arch/arm/include/debug/icedcc.S b/arch/arm/include/debug/icedcc.S
index 43afcb0..7204064 100644
--- a/arch/arm/include/debug/icedcc.S
+++ b/arch/arm/include/debug/icedcc.S
@@ -21,10 +21,7 @@ 
 		.endm
 
 		.macro	busyuart, rd, rx
-1001:
-		mrc	p14, 0, \rx, c0, c1, 0
-		tst	\rx, #0x20000000
-		beq	1001b
+		waituart \rd, \rx
 		.endm
 
 		.macro	waituart, rd, rx
@@ -45,10 +42,7 @@ 
 		.endm
 
 		.macro	busyuart, rd, rx
-1001:
-		mrc	p14, 0, \rx, c14, c0, 0
-		tst	\rx, #0x10000000
-		beq	1001b
+		waituart \rd, \rx
 		.endm
 
 		.macro	waituart, rd, rx
@@ -69,11 +63,7 @@ 
 		.endm
 
 		.macro	busyuart, rd, rx
-1001:
-		mrc	p14, 0, \rx, c0, c0, 0
-		tst	\rx, #2
-		beq	1001b
-
+		waituart \rd, \rx
 		.endm
 
 		.macro	waituart, rd, rx