* [PATCH] fix DEBUG_LL DCC race condition @ 2012-10-29 15:18 Johannes Stezenbach 2012-10-31 16:27 ` Tony Lindgren 2012-10-31 16:36 ` Russell King - ARM Linux 0 siblings, 2 replies; 5+ messages in thread From: Johannes Stezenbach @ 2012-10-29 15:18 UTC (permalink / raw) To: linux-arm-kernel 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. 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] fix DEBUG_LL DCC race condition 2012-10-29 15:18 [PATCH] fix DEBUG_LL DCC race condition Johannes Stezenbach @ 2012-10-31 16:27 ` Tony Lindgren 2012-10-31 16:36 ` Russell King - ARM Linux 1 sibling, 0 replies; 5+ messages in thread From: Tony Lindgren @ 2012-10-31 16:27 UTC (permalink / raw) To: linux-arm-kernel * 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] fix DEBUG_LL DCC race condition 2012-10-29 15:18 [PATCH] fix DEBUG_LL DCC race condition Johannes Stezenbach 2012-10-31 16:27 ` Tony Lindgren @ 2012-10-31 16:36 ` Russell King - ARM Linux 2012-10-31 17:08 ` Johannes Stezenbach 1 sibling, 1 reply; 5+ messages in thread From: Russell King - ARM Linux @ 2012-10-31 16:36 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] fix DEBUG_LL DCC race condition 2012-10-31 16:36 ` Russell King - ARM Linux @ 2012-10-31 17:08 ` Johannes Stezenbach 2012-10-31 17:25 ` [PATCH v2] " Johannes Stezenbach 0 siblings, 1 reply; 5+ messages in thread From: Johannes Stezenbach @ 2012-10-31 17:08 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] fix DEBUG_LL DCC race condition 2012-10-31 17:08 ` Johannes Stezenbach @ 2012-10-31 17:25 ` Johannes Stezenbach 0 siblings, 0 replies; 5+ messages in thread From: Johannes Stezenbach @ 2012-10-31 17:25 UTC (permalink / raw) To: linux-arm-kernel 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 the current busyuart implementation 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 should wait until flow control allows to send the character, or do nothing. busyuart should wait until the FIFO is empty (all data is sent). For DCC, busyuart should wait until the JTAG debugger has read the DCC register (wait for wDTRfull == 0 on ARMv6), and waituart does nothing. Signed-off-by: Johannes Stezenbach <js@sig21.net> --- v2: - update description - waituart is empty (no flow control) diff --git a/arch/arm/include/debug/icedcc.S b/arch/arm/include/debug/icedcc.S index 43afcb0..30e18e5 100644 --- a/arch/arm/include/debug/icedcc.S +++ b/arch/arm/include/debug/icedcc.S @@ -21,13 +21,6 @@ .endm .macro busyuart, rd, rx -1001: - mrc p14, 0, \rx, c0, c1, 0 - tst \rx, #0x20000000 - beq 1001b - .endm - - .macro waituart, rd, rx mov \rd, #0x2000000 1001: subs \rd, \rd, #1 @@ -38,6 +31,9 @@ 1002: .endm + .macro waituart, rd, rx + .endm + #elif defined(CONFIG_CPU_XSCALE) .macro senduart, rd, rx @@ -45,13 +41,6 @@ .endm .macro busyuart, rd, rx -1001: - mrc p14, 0, \rx, c14, c0, 0 - tst \rx, #0x10000000 - beq 1001b - .endm - - .macro waituart, rd, rx mov \rd, #0x10000000 1001: subs \rd, \rd, #1 @@ -62,6 +51,9 @@ 1002: .endm + .macro waituart, rd, rx + .endm + #else .macro senduart, rd, rx @@ -69,14 +61,6 @@ .endm .macro busyuart, rd, rx -1001: - mrc p14, 0, \rx, c0, c0, 0 - tst \rx, #2 - beq 1001b - - .endm - - .macro waituart, rd, rx mov \rd, #0x2000000 1001: subs \rd, \rd, #1 @@ -87,4 +71,7 @@ 1002: .endm + .macro waituart, rd, rx + .endm + #endif /* CONFIG_CPU_V6 */ ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-10-31 17:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-29 15:18 [PATCH] fix DEBUG_LL DCC race condition Johannes Stezenbach 2012-10-31 16:27 ` Tony Lindgren 2012-10-31 16:36 ` Russell King - ARM Linux 2012-10-31 17:08 ` Johannes Stezenbach 2012-10-31 17:25 ` [PATCH v2] " Johannes Stezenbach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).