Skip to contentSkip to conversation

drivers: char: hvc: add arm JTAG DCC console support

PATCH In review Daniel Walker 13 participants 39 messages Nov 30, 2010 – Apr 7, 2011

Subsystems (4)
Arm Qualcomm mailing list TTY layer and serial drivers Char and misc drivers Hypervisor virtual console driver
lore
Share & tools
lore

Daniel Walker

author
#0 lore raw

PATCH 1/1 drivers: char: hvc: add arm JTAG DCC console support

This driver adds a basic console that uses the arm JTAG DCC to transfer data back and forth. It has support for ARMv6 and ARMv7.

This console is created under the HVC driver, and should be named /dev/hvcX (or /dev/hvc0 for example).

Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Mike Frysinger <[email protected]>
Signed-off-by Daniel Walker <[email protected]>
---
 drivers/char/Kconfig   |    9 +++
 drivers/char/Makefile  |    1 +
 drivers/char/hvc_dcc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/hvc_dcc.c
drivers/char/Kconfig +9 -0
index 43d3395..d4a7776 100644
@@ -682,6 +682,15 @@ config HVC_UDBG
        select HVC_DRIVER
        default n
 
+config HVC_DCC
+       bool "ARM JTAG DCC console"
+       depends on ARM
+       select HVC_DRIVER
+       help
+         This console uses the JTAG DCC on ARM to create a console under the HVC
+	 driver. This console is used through a JTAG only on ARM. If you don't have
+	 a JTAG then you probably don't want this option.
+
 config VIRTIO_CONSOLE
 	tristate "Virtio console"
 	depends on VIRTIO
drivers/char/Makefile +1 -0
index ba53ec9..fa0b824 100644
@@ -34,6 +34,7 @@ obj-$(CONFIG_HVC_CONSOLE)	+= hvc_vio.o hvsi.o
 obj-$(CONFIG_HVC_ISERIES)	+= hvc_iseries.o
 obj-$(CONFIG_HVC_RTAS)		+= hvc_rtas.o
 obj-$(CONFIG_HVC_TILE)		+= hvc_tile.o
+obj-$(CONFIG_HVC_DCC)		+= hvc_dcc.o
 obj-$(CONFIG_HVC_BEAT)		+= hvc_beat.o
 obj-$(CONFIG_HVC_DRIVER)	+= hvc_console.o
 obj-$(CONFIG_HVC_IRQ)		+= hvc_irq.o
drivers/char/hvc_dcc.c new file +133 -0

133 lines not on this page · raw message

Signature · 5 lines
1.7.1

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · Daniel Walker

This driver adds a basic console that uses the arm JTAG DCC to transfer data back and forth. It has support for ARMv6 and ARMv7.

This console is created under the HVC driver, and should be named /dev/hvcX (or /dev/hvc0 for example).

Quoted 6 lines
Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Mike Frysinger <[email protected]>
Signed-off-by Daniel Walker <[email protected]>
Acked-by Nicolas Pitre <[email protected]>

This doesn't support both ARMv6 and ARMv7 at run time, but this can trivially be added later when needed.

Nicolas

maintainer Acked-by
#2 lore raw
Quoted 8 lines · Nicolas Pitre
Quoted 6 lines
Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Mike Frysinger <[email protected]>
Signed-off-by Daniel Walker <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Acked-by Arnd Bergmann <[email protected]>
Quoted 2 lines

This doesn't support both ARMv6 and ARMv7 at run time, but this can trivially be added later when needed.

I was about to make a similar comment when I saw yours ;-)

#3 lore raw
In reply to Daniel Walker

drivers/char/Kconfig config HVC_UDBG · line 690

Quoted 10 lines · Daniel Walker
drivers/char/Kconfig +6 -0
@@ -682,6 +682,15 @@ config HVC_UDBG
        select HVC_DRIVER
        default n
 
+config HVC_DCC
+       bool "ARM JTAG DCC console"
+       depends on ARM
+       select HVC_DRIVER
+       help
+         This console uses the JTAG DCC on ARM to create a console under the HVC

Looks like you added one too many spaces for indent here.

Quoted 5 lines
drivers/char/hvc_dcc.c new file +0 -0
new file mode 100644
index 0000000..6470f63

drivers/char/hvc_dcc.c line 41

Quoted 9 lines
drivers/char/hvc_dcc.c new file +9 -0
+static inline u32 __dcc_getstatus(void)
+{
+	u32 __ret;
+
+	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+		: "=r" (__ret) : : "cc");
+
+	return __ret;
+}

Without marking this asm volatile my compiler decides it can cache the value of __ret in a register and then check the value of it continually in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).

00000000 <hvc_dcc_put_chars>:

   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX (bit 29) and then stored in r3 for later use. Marking this volatile produces the following:

00000000 <hvc_dcc_put_chars>:

   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr

which looks better.

I marked all the asm in this driver as volatile. Is that correct?

Quoted 31 lines
drivers/char/hvc_dcc.c new file +31 -0
+#if defined(CONFIG_CPU_V7)
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
+			bne get_wait                                       \n\
+			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c) : : "cc");
+
+	return __c;
+}
+#else
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c));
+
+	return __c;
+}
+#endif
+
+#if defined(CONFIG_CPU_V7)
+static inline void __dcc_putchar(char c)
+{
+	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
+			bcs put_wait                              \n\
+			mcr p14, 0, %0, c0, c5, 0                   "
+	: : "r" (c) : "cc");

drivers/char/hvc_dcc.c line 84

Quoted 10 lines
drivers/char/hvc_dcc.c new file +10 -0
+}
+#else
+static inline void __dcc_putchar(char c)
+{
+	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+		: /* no output register */
+		: "r" (c));
+}
+#endif
+

I don't think both the v7 and v6 functions are necessary. It seems I can get away with just the second version of __dcc_(get|put)char() on a v7. The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will also do the same thing if I read the manuals right. The test in the inline assembly is saying, wait for a character to be ready or wait for a character to be read then actually write a character or read one. The code in hvc_dcc_put_chars() is already doing the same thing, albeit in a slightly different form. Instead of getting the status bits put into the condition codes and looping with bne or bcs it will read the register, and it with bit 29 or bit 28 to see if it should wait and then continue with the writing/reading. I think you can just drop the looping for the v7 version of the functions and have this driver work on v6 and v7. Alternatively, you can make some function that says tx buffer is empty, rx buffer is full or something but I don't see how saving a couple instructions buys us much when we can have one driver for v6 and v7.

I see that Tony Lindgren modified the DCC macros for v7 in commit 200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not sure why though, since it seems that a v6 and a v7 should really do the same thing by waiting for the buffers to be ready before filling them or reading them. Which probably means we can get low-level dcc debugging on all targets if I'm not mistaken.

drivers/char/hvc_dcc.c line 93

Quoted 9 lines
drivers/char/hvc_dcc.c new file +9 -0
+static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		while (__dcc_getstatus() & DCC_STATUS_TX)
+			cpu_relax();
+
+		__dcc_putchar((char)(buf[i] & 0xFF));

Is this & 0xFF and cast to char unnecessary? buf is a char array, and chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?

drivers/char/hvc_dcc.c line 111

Quoted 13 lines
drivers/char/hvc_dcc.c new file +13 -0
+static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; ++i) {
+		int c = -1;
+
+		if (__dcc_getstatus() & DCC_STATUS_RX)
+			c = __dcc_getchar();
+		if (c < 0)
+			break;
+		buf[i] = c;
+	}

I think this for loop can be simplified. __dcc_getchar() returns a char. It never returns -1, so the check for c < 0 can't be taken if __dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you can have a simple if-else and assign buf[i] in the if branch and break in the else branch.

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
author
#4 lore raw
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Daniel Walker
drivers/char/Kconfig +0 -0
@@ -682,6 +682,15 @@ config HVC_UDBG
        select HVC_DRIVER
        default n
Quoted 8 lines

drivers/char/Kconfig config HVC_UDBG · line 690

Quoted 6 lines
drivers/char/Kconfig +6 -0
+config HVC_DCC
+       bool "ARM JTAG DCC console"
+       depends on ARM
+       select HVC_DRIVER
+       help
+         This console uses the JTAG DCC on ARM to create a console under the HVC

Looks like you added one too many spaces for indent here.

The first line is fine, but the other two might need an extra space.

Quoted 58 lines
Quoted 5 lines
drivers/char/hvc_dcc.c new file +0 -0
new file mode 100644
index 0000000..6470f63

drivers/char/hvc_dcc.c line 41

Quoted 9 lines
drivers/char/hvc_dcc.c new file +9 -0
+static inline u32 __dcc_getstatus(void)
+{
+	u32 __ret;
+
+	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+		: "=r" (__ret) : : "cc");
+
+	return __ret;
+}

Without marking this asm volatile my compiler decides it can cache the value of __ret in a register and then check the value of it continually in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).

00000000 <hvc_dcc_put_chars>:

   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX (bit 29) and then stored in r3 for later use. Marking this volatile produces the following:

00000000 <hvc_dcc_put_chars>:

   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr
Quoted 3 lines

which looks better.

I marked all the asm in this driver as volatile. Is that correct?

Are you talking about __dcc_getstatus only? I don't think adding volatile is going to hurt anything, if not having it causes problems.

Quoted 47 lines
Quoted 31 lines
drivers/char/hvc_dcc.c new file +31 -0
+#if defined(CONFIG_CPU_V7)
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
+			bne get_wait                                       \n\
+			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c) : : "cc");
+
+	return __c;
+}
+#else
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c));
+
+	return __c;
+}
+#endif
+
+#if defined(CONFIG_CPU_V7)
+static inline void __dcc_putchar(char c)
+{
+	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
+			bcs put_wait                              \n\
+			mcr p14, 0, %0, c0, c5, 0                   "
+	: : "r" (c) : "cc");

drivers/char/hvc_dcc.c line 84

Quoted 10 lines
drivers/char/hvc_dcc.c new file +10 -0
+}
+#else
+static inline void __dcc_putchar(char c)
+{
+	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+		: /* no output register */
+		: "r" (c));
+}
+#endif
+

I don't think both the v7 and v6 functions are necessary. It seems I can get away with just the second version of __dcc_(get|put)char() on a v7. The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will also do the same thing if I read the manuals right. The test in the

Quoted 8 lines

inline assembly is saying, wait for a character to be ready or wait for a character to be read then actually write a character or read one. The code in hvc_dcc_put_chars() is already doing the same thing, albeit in a slightly different form. Instead of getting the status bits put into the condition codes and looping with bne or bcs it will read the register, and it with bit 29 or bit 28 to see if it should wait and then continue with the writing/reading. I think you can just drop the looping for the v7 version of the functions and have this driver work on v6 and v7.

We could maybe drop the looping for TX, but RX has no C based looping even tho for v7 it's recommended that we loop (presumably for v6 it's not recommended).

Quoted 10 lines

Alternatively, you can make some function that says tx buffer is empty, rx buffer is full or something but I don't see how saving a couple instructions buys us much when we can have one driver for v6 and v7.

I see that Tony Lindgren modified the DCC macros for v7 in commit 200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not sure why though, since it seems that a v6 and a v7 should really do the same thing by waiting for the buffers to be ready before filling them or reading them. Which probably means we can get low-level dcc debugging on all targets if I'm not mistaken.

This is primarily why these function look they way they do. I'd like to hear from Tony, because he apparent didn't think they did the same thing.

Quoted 4 lines
Quoted 4 lines
drivers/char/hvc_dcc.c new file +4 -0
+static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
+{
+	int i;
+
Quoted 8 lines

drivers/char/hvc_dcc.c line 93

Quoted 5 lines
drivers/char/hvc_dcc.c new file +5 -0
+	for (i = 0; i < count; i++) {
+		while (__dcc_getstatus() & DCC_STATUS_TX)
+			cpu_relax();
+
+		__dcc_putchar((char)(buf[i] & 0xFF));

Is this & 0xFF and cast to char unnecessary? buf is a char array, and chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?

Yeah, doesn't seem like it.

Quoted 12 lines
Quoted 12 lines
drivers/char/hvc_dcc.c new file +12 -0
+static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; ++i) {
+		int c = -1;
+
+		if (__dcc_getstatus() & DCC_STATUS_RX)
+			c = __dcc_getchar();
+		if (c < 0)
+			break;
+		buf[i] = c;
Quoted 9 lines

drivers/char/hvc_dcc.c

Quoted 1 line
drivers/char/hvc_dcc.c new file +1 -0
+	}

I think this for loop can be simplified. __dcc_getchar() returns a char. It never returns -1, so the check for c < 0 can't be taken if __dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you can have a simple if-else and assign buf[i] in the if branch and break in the else branch.

Like this?

	for (i = 0; i < count; ++i) {

		if (__dcc_getstatus() & DCC_STATUS_RX)
			buf[i] = __dcc_getchar();
		else
			break;
	}

It's a micro clean up I guess ..

Daniel

Signature · 2 lines
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#5 lore raw
Quoted 15 lines · Daniel Walker
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Daniel Walker
drivers/char/Kconfig +0 -0
@@ -682,6 +682,15 @@ config HVC_UDBG
        select HVC_DRIVER
        default n
Quoted 8 lines

drivers/char/Kconfig config HVC_UDBG · line 690

Quoted 6 lines
drivers/char/Kconfig +6 -0
+config HVC_DCC
+       bool "ARM JTAG DCC console"
+       depends on ARM
+       select HVC_DRIVER
+       help
+         This console uses the JTAG DCC on ARM to create a console under the HVC

Looks like you added one too many spaces for indent here.

Quoted 1 line

The first line is fine, but the other two might need an extra space.

For this, or any other changes you want, I'll gladly take a follow-on patch as this one is already in my tty-next tree.

thanks,

greg k-h

#6 lore raw

PATCH 1/1 hvc_dcc: Simplify assembly for v6 and v7 ARM

The inline assembly differences for v6 vs. v7 in the hvc_dcc driver are purely optimizations. On a v7 processor, an mrc with the pc sets the condition codes to the 28-31 bits of the register being read. It just so happens that the TX/RX full bits the DCC driver is testing for are high enough in the register to be put into the condition codes. On a v6 processor, this "feature" isn't implemented and thus we have to do the usual read, mask, test operations to check for TX/RX full.

Since we already test the RX/TX full bits before calling __dcc_getchar() and __dcc_putchar() we don't actually need to do anything special for v7 over v6. The only difference is in hvc_dcc_get_chars(). We would test RX full, poll RX full, and then read a character from the buffer, whereas now we will test RX full, read a character from the buffer, and then test RX full again for the second iteration of the loop. It doesn't seem possible for the buffer to go from full to empty between testing the RX full and reading a character. Therefore, replace the v7 versions with the v6 versions and everything works the same.

While we're here, cleanup the for loops a bit and mark the inline assembly as volatile. Not marking it volatile causes GCC to cache the results of the status and RX buffer registers causing lockups.

Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/char/hvc_dcc.c |   43 +++++++------------------------------------
 1 files changed, 7 insertions(+), 36 deletions(-)
drivers/char/hvc_dcc.c +7 -36
index 6470f63..435f6fa 100644
@@ -33,54 +33,29 @@
 static inline u32 __dcc_getstatus(void)
 {
 	u32 __ret;
-
-	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
 		: "=r" (__ret) : : "cc");
 
 	return __ret;
 }
 
 
-#if defined(CONFIG_CPU_V7)
 static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
-			bne get_wait                                       \n\
-			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
-		: "=r" (__c) : : "cc");
-
-	return __c;
-}
-#else
-static inline char __dcc_getchar(void)
-{
-	char __c;
-
-	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+	asm volatile("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c));
 
 	return __c;
 }
-#endif
 
-#if defined(CONFIG_CPU_V7)
-static inline void __dcc_putchar(char c)
-{
-	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
-			bcs put_wait                              \n\
-			mcr p14, 0, %0, c0, c5, 0                   "
-	: : "r" (c) : "cc");
-}
-#else
 static inline void __dcc_putchar(char c)
 {
-	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
 		: "r" (c));
 }
-#endif
 
 static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 {

@@ -90,7 +65,7 @@ static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 		while (__dcc_getstatus() & DCC_STATUS_TX)
 			cpu_relax();
 
-		__dcc_putchar((char)(buf[i] & 0xFF));
+		__dcc_putchar(buf[i]);
 	}
 
 	return count;

@@ -100,15 +75,11 @@ static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
 {
 	int i;
 
-	for (i = 0; i < count; ++i) {
-		int c = -1;
-
+	for (i = 0; i < count; ++i)
 		if (__dcc_getstatus() & DCC_STATUS_RX)
-			c = __dcc_getchar();
-		if (c < 0)
+			buf[i] = __dcc_getchar();
+		else
 			break;
-		buf[i] = c;
-	}
 
 	return i;
 }
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
author
#7 lore raw

Re: [PATCH] hvc_dcc: Simplify assembly for v6 and v7 ARM

Quoted 20 lines · Stephen Boyd

The inline assembly differences for v6 vs. v7 in the hvc_dcc driver are purely optimizations. On a v7 processor, an mrc with the pc sets the condition codes to the 28-31 bits of the register being read. It just so happens that the TX/RX full bits the DCC driver is testing for are high enough in the register to be put into the condition codes. On a v6 processor, this "feature" isn't implemented and thus we have to do the usual read, mask, test operations to check for TX/RX full.

Since we already test the RX/TX full bits before calling __dcc_getchar() and __dcc_putchar() we don't actually need to do anything special for v7 over v6. The only difference is in hvc_dcc_get_chars(). We would test RX full, poll RX full, and then read a character from the buffer, whereas now we will test RX full, read a character from the buffer, and then test RX full again for the second iteration of the loop. It doesn't seem possible for the buffer to go from full to empty between testing the RX full and reading a character. Therefore, replace the v7 versions with the v6 versions and everything works the same.

Quoted 4 lines

While we're here, cleanup the for loops a bit and mark the inline assembly as volatile. Not marking it volatile causes GCC to cache the results of the status and RX buffer registers causing lockups.

I would expect to see three patches. One that adds volatile, which appears to be a good fix. Another patch that changes the assembly lines, and another that does the clean up. The last two are more controversial ones.

Daniel

Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.
#8 lore raw

Re: [PATCH] hvc_dcc: Simplify assembly for v6 and v7 ARM

Quoted 5 lines · Daniel Walker

I would expect to see three patches. One that adds volatile, which appears to be a good fix. Another patch that changes the assembly lines, and another that does the clean up. The last two are more controversial ones

Ok. I'll send a series later today to give some more time for feedback.

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#9 lore raw

Re: [PATCH] hvc_dcc: Simplify assembly for v6 and v7 ARM

Quoted 8 lines · Stephen Boyd
Quoted 5 lines · Daniel Walker

I would expect to see three patches. One that adds volatile, which appears to be a good fix. Another patch that changes the assembly lines, and another that does the clean up. The last two are more controversial ones

Ok. I'll send a series later today to give some more time for feedback.

I think you can do that right away. Splitting the patch will make that feedback easier to provide.

Nicolas

#10 lore raw

cover hvc_dcc cleanups and fixes

In reply to Stephen Boyd

Here are the split patches.

The first two patches cleanup and fix the hvc_dcc driver for my compiler. The final patch is more controversial, it removes the v6 and v7 differences in this driver.

Stephen Boyd (3):
hvc_dcc: Fix bad code generation by marking assembly volatile
hvc_dcc: Simplify put_chars()/get_chars() loops
hvc_dcc: Simplify assembly for v6 and v7 ARM

 drivers/char/hvc_dcc.c |   43 +++++++------------------------------------
 1 files changed, 7 insertions(+), 36 deletions(-)
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#11 lore raw

PATCH 1/3 hvc_dcc: Fix bad code generation by marking assembly volatile

Without marking the asm __dcc_getstatus() volatile my compiler decides it can cache the value of __ret in a register and then check the value of it continually in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).

00000000 <hvc_dcc_put_chars>:

   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX (bit 29) and then stored in r3 for later use. Marking the asm volatile produces the following:

00000000 <hvc_dcc_put_chars>:

   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr

which looks better and actually works. Mark all the inline assembly in this file as volatile since we don't want the compiler to optimize away these statements or move them around in any way.

Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/char/hvc_dcc.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/char/hvc_dcc.c +5 -6
index 6470f63..155ec10 100644
@@ -33,8 +33,7 @@
 static inline u32 __dcc_getstatus(void)
 {
 	u32 __ret;
-
-	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
 		: "=r" (__ret) : : "cc");
 
 	return __ret;

@@ -46,7 +45,7 @@ static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
+	asm volatile("get_wait:	mrc p14, 0, pc, c0, c1, 0                  \n\
 			bne get_wait                                       \n\
 			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c) : : "cc");

@@ -58,7 +57,7 @@ static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+	asm volatile("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c));
 
 	return __c;

@@ -68,7 +67,7 @@ static inline char __dcc_getchar(void)
 #if defined(CONFIG_CPU_V7)
 static inline void __dcc_putchar(char c)
 {
-	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
+	asm volatile("put_wait:	mrc p14, 0, pc, c0, c1, 0         \n\
 			bcs put_wait                              \n\
 			mcr p14, 0, %0, c0, c5, 0                   "
 	: : "r" (c) : "cc");

@@ -76,7 +75,7 @@ static inline void __dcc_putchar(char c)
 #else
 static inline void __dcc_putchar(char c)
 {
-	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
 		: "r" (c));
 }
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Acked-by on v1 1/3
#12 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

Quoted 48 lines · Stephen Boyd

Without marking the asm __dcc_getstatus() volatile my compiler decides it can cache the value of __ret in a register and then check the value of it continually in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).

00000000 <hvc_dcc_put_chars>:

   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX (bit 29) and then stored in r3 for later use. Marking the asm volatile produces the following:

00000000 <hvc_dcc_put_chars>:

   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr

which looks better and actually works. Mark all the inline assembly in this file as volatile since we don't want the compiler to optimize away these statements or move them around in any way.

Quoted 5 lines
Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Quoted 58 lines
---
 drivers/char/hvc_dcc.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/char/hvc_dcc.c +5 -6
index 6470f63..155ec10 100644
@@ -33,8 +33,7 @@
 static inline u32 __dcc_getstatus(void)
 {
 	u32 __ret;
-
-	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
 		: "=r" (__ret) : : "cc");
 
 	return __ret;

@@ -46,7 +45,7 @@ static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
+	asm volatile("get_wait:	mrc p14, 0, pc, c0, c1, 0                  \n\
 			bne get_wait                                       \n\
 			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c) : : "cc");

@@ -58,7 +57,7 @@ static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+	asm volatile("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c));
 
 	return __c;

@@ -68,7 +67,7 @@ static inline char __dcc_getchar(void)
 #if defined(CONFIG_CPU_V7)
 static inline void __dcc_putchar(char c)
 {
-	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
+	asm volatile("put_wait:	mrc p14, 0, pc, c0, c1, 0         \n\
 			bcs put_wait                              \n\
 			mcr p14, 0, %0, c0, c5, 0                   "
 	: : "r" (c) : "cc");

@@ -76,7 +75,7 @@ static inline void __dcc_putchar(char c)
 #else
 static inline void __dcc_putchar(char c)
 {
-	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
 		: "r" (c));
 }
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#13 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

Hi!

Quoted 13 lines
Quoted 13 lines
drivers/char/hvc_dcc.c +1 -2
index 6470f63..155ec10 100644
@@ -33,8 +33,7 @@
 static inline u32 __dcc_getstatus(void)
 {
 	u32 __ret;
-
-	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
 		: "=r" (__ret) : : "cc");
Quoted 1 line

drivers/char/hvc_dcc.c line 39

Quoted 1 line
drivers/char/hvc_dcc.c +0 -0
 	return __ret;

Is volatile needed here? If __dcc_getstatus() return value is discarded, we want assembly discarded, right?

Quoted 9 lines

drivers/char/hvc_dcc.c static inline char __dcc_getchar(void) · line 63

Quoted 9 lines
drivers/char/hvc_dcc.c +1 -1
@@ -58,7 +57,7 @@ static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+	asm volatile("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c));
 
 	return __c;

Same here?
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#14 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

Quoted 13 lines · Pavel Machek
Quoted 13 lines
Quoted 13 lines
drivers/char/hvc_dcc.c +1 -2
index 6470f63..155ec10 100644
@@ -33,8 +33,7 @@
 static inline u32 __dcc_getstatus(void)
 {
 	u32 __ret;
-
-	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
 		: "=r" (__ret) : : "cc");
Quoted 4 lines
Quoted 1 line

drivers/char/hvc_dcc.c line 39

Quoted 1 line
drivers/char/hvc_dcc.c +0 -0
 	return __ret;

Is volatile needed here? If __dcc_getstatus() return value is discarded, we want assembly discarded, right?

That's not really the issue being fixed. Without the volatile, the compiler is free to cache and reuse a previously loaded status value. It is important that the status be read each time.

I don't think there is a way of indicating that assembly needs to happen for each use, but that it is OK to discard if the value isn't used. 'volatile' is a bit overloaded.

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#15 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

Quoted 20 lines · David Brown
Quoted 13 lines · Pavel Machek
Quoted 13 lines
Quoted 13 lines
drivers/char/hvc_dcc.c +1 -2
index 6470f63..155ec10 100644
@@ -33,8 +33,7 @@
 static inline u32 __dcc_getstatus(void)
 {
 	u32 __ret;
-
-	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
 		: "=r" (__ret) : : "cc");
Quoted 4 lines
Quoted 1 line

drivers/char/hvc_dcc.c line 39

Quoted 1 line
drivers/char/hvc_dcc.c +0 -0
 	return __ret;

Is volatile needed here? If __dcc_getstatus() return value is discarded, we want assembly discarded, right?

Quoted 7 lines

That's not really the issue being fixed. Without the volatile, the compiler is free to cache and reuse a previously loaded status value. It is important that the status be read each time.

I don't think there is a way of indicating that assembly needs to happen for each use, but that it is OK to discard if the value isn't used. 'volatile' is a bit overloaded.

Ok, thanks for explanation.
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Acked-by
#16 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

In reply to Nicolas Pitre
Quoted 50 lines · Nicolas Pitre
Quoted 48 lines · Stephen Boyd

Without marking the asm __dcc_getstatus() volatile my compiler decides it can cache the value of __ret in a register and then check the value of it continually in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).

00000000 <hvc_dcc_put_chars>:

   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX (bit 29) and then stored in r3 for later use. Marking the asm volatile produces the following:

00000000 <hvc_dcc_put_chars>:

   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr

which looks better and actually works. Mark all the inline assembly in this file as volatile since we don't want the compiler to optimize away these statements or move them around in any way.

Quoted 7 lines
Quoted 5 lines
Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Acked-by Tony Lindgren <[email protected]>
#17 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

In reply to Stephen Boyd

Hi,

Quoted 2 lines · Stephen Boyd

Without marking the asm __dcc_getstatus() volatile my compiler decides [...]

What compiler ? That might be a usefull information to know, espectially 5 years from now when tracing code history. There has been similar issue with gcc 4.5 recently. AFAIK, in the same idea, the final change has been to mark the asm volatile.

Thanks,
- Arnaud

#18 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

Quoted 9 lines · Arnaud Lacombe

Hi,

Quoted 2 lines · Stephen Boyd

Without marking the asm __dcc_getstatus() volatile my compiler decides [...]

What compiler ? That might be a usefull information to know, espectially 5 years from now when tracing code history. There has been similar issue with gcc 4.5 recently. AFAIK, in the same idea, the final change has been to mark the asm volatile.

Sure, we can replace "my compiler" with "my compiler (arm-eabi-gcc (GCC) 4.4.0)".

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#19 lore raw

Re: [PATCH 1/3] hvc_dcc: Fix bad code generation by marking assembly volatile

Quoted 6 lines · Stephen Boyd
Quoted 5 lines · Arnaud Lacombe

Hi,

Quoted 2 lines · Stephen Boyd

Without marking the asm __dcc_getstatus() volatile my compiler decides [...]

Quoted 7 lines
Quoted 4 lines

What compiler ? That might be a usefull information to know, espectially 5 years from now when tracing code history. There has been similar issue with gcc 4.5 recently. AFAIK, in the same idea, the final change has been to mark the asm volatile.

Sure, we can replace "my compiler" with "my compiler (arm-eabi-gcc (GCC) 4.4.0)".

Compiler version doesn't matter -- this is a simple correctness issue.

If an inline asm statement provides an output value then the compiler is free to cache that value in a register, unless the inline asm is marked so that the value may change from one invocation to another. Also, the compiler is free to eliminate the inline asm statement entirely as well if the output value provided by that inline asm is never used... unless if the inline asm is marked as having side effects. In both cases the volatile qualifier does indicate that the returned value may change (new status flag state) and
that the asm code therein has side effects (e.g. pop a character off a FIFO).

Sometimes the desired effect can be indicated by clever usage of parameter constraints, but in this case the volatile keyword is most appropriate.

Nicolas

#20 lore raw

PATCH 2/3 hvc_dcc: Simplify put_chars()/get_chars() loops

In reply to Stephen Boyd

Casting and anding with 0xff is unnecessary in hvc_dcc_put_chars() since buf is already a char[]. __dcc_get_char() can't return an int less than 0 since it only returns a char. Simplify the if statement in hvc_dcc_get_chars() to take this into account.

Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/char/hvc_dcc.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
drivers/char/hvc_dcc.c +4 -8
index 155ec10..ad23cc8 100644
@@ -89,7 +89,7 @@ static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 		while (__dcc_getstatus() & DCC_STATUS_TX)
 			cpu_relax();
 
-		__dcc_putchar((char)(buf[i] & 0xFF));
+		__dcc_putchar(buf[i]);
 	}
 
 	return count;

@@ -99,15 +99,11 @@ static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
 {
 	int i;
 
-	for (i = 0; i < count; ++i) {
-		int c = -1;
-
+	for (i = 0; i < count; ++i)
 		if (__dcc_getstatus() & DCC_STATUS_RX)
-			c = __dcc_getchar();
-		if (c < 0)
+			buf[i] = __dcc_getchar();
+		else
 			break;
-		buf[i] = c;
-	}
 
 	return i;
 }
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#21 lore raw

PATCH 3/3 hvc_dcc: Simplify assembly for v6 and v7 ARM

In reply to Stephen Boyd

The inline assembly differences for v6 vs. v7 in the hvc_dcc driver are purely optimizations. On a v7 processor, an mrc with the pc sets the condition codes to the 28-31 bits of the register being read. It just so happens that the TX/RX full bits the DCC driver is testing for are high enough in the register to be put into the condition codes. On a v6 processor, this "feature" isn't implemented and thus we have to do the usual read, mask, test operations to check for TX/RX full.

Since we already test the RX/TX full bits before calling __dcc_getchar() and __dcc_putchar() we don't actually need to do anything special for v7 over v6. The only difference is in hvc_dcc_get_chars(). We would test RX full, poll RX full, and then read a character from the buffer, whereas now we will test RX full, read a character from the buffer, and then test RX full again for the second iteration of the loop. It doesn't seem possible for the buffer to go from full to empty between testing the RX full and reading a character. Therefore, replace the v7 versions with the v6 versions and everything works the same.

Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/char/hvc_dcc.c |   24 ------------------------
 1 files changed, 0 insertions(+), 24 deletions(-)
drivers/char/hvc_dcc.c +0 -24
index ad23cc8..435f6fa 100644
@@ -40,19 +40,6 @@ static inline u32 __dcc_getstatus(void)
 }
 
 
-#if defined(CONFIG_CPU_V7)
-static inline char __dcc_getchar(void)
-{
-	char __c;
-
-	asm volatile("get_wait:	mrc p14, 0, pc, c0, c1, 0                  \n\
-			bne get_wait                                       \n\
-			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
-		: "=r" (__c) : : "cc");
-
-	return __c;
-}
-#else
 static inline char __dcc_getchar(void)
 {
 	char __c;

@@ -62,24 +49,13 @@ static inline char __dcc_getchar(void)
 
 	return __c;
 }
-#endif
 
-#if defined(CONFIG_CPU_V7)
-static inline void __dcc_putchar(char c)
-{
-	asm volatile("put_wait:	mrc p14, 0, pc, c0, c1, 0         \n\
-			bcs put_wait                              \n\
-			mcr p14, 0, %0, c0, c5, 0                   "
-	: : "r" (c) : "cc");
-}
-#else
 static inline void __dcc_putchar(char c)
 {
 	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
 		: "r" (c));
 }
-#endif
 
 static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 {
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Acked-by on v1 3/3
#22 lore raw

Re: [PATCH 3/3] hvc_dcc: Simplify assembly for v6 and v7 ARM

Quoted 20 lines · Stephen Boyd

The inline assembly differences for v6 vs. v7 in the hvc_dcc driver are purely optimizations. On a v7 processor, an mrc with the pc sets the condition codes to the 28-31 bits of the register being read. It just so happens that the TX/RX full bits the DCC driver is testing for are high enough in the register to be put into the condition codes. On a v6 processor, this "feature" isn't implemented and thus we have to do the usual read, mask, test operations to check for TX/RX full.

Since we already test the RX/TX full bits before calling __dcc_getchar() and __dcc_putchar() we don't actually need to do anything special for v7 over v6. The only difference is in hvc_dcc_get_chars(). We would test RX full, poll RX full, and then read a character from the buffer, whereas now we will test RX full, read a character from the buffer, and then test RX full again for the second iteration of the loop. It doesn't seem possible for the buffer to go from full to empty between testing the RX full and reading a character. Therefore, replace the v7 versions with the v6 versions and everything works the same.

Quoted 5 lines
Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Quoted 57 lines
---
 drivers/char/hvc_dcc.c |   24 ------------------------
 1 files changed, 0 insertions(+), 24 deletions(-)
drivers/char/hvc_dcc.c +0 -24
index ad23cc8..435f6fa 100644
@@ -40,19 +40,6 @@ static inline u32 __dcc_getstatus(void)
 }
 
 
-#if defined(CONFIG_CPU_V7)
-static inline char __dcc_getchar(void)
-{
-	char __c;
-
-	asm volatile("get_wait:	mrc p14, 0, pc, c0, c1, 0                  \n\
-			bne get_wait                                       \n\
-			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
-		: "=r" (__c) : : "cc");
-
-	return __c;
-}
-#else
 static inline char __dcc_getchar(void)
 {
 	char __c;

@@ -62,24 +49,13 @@ static inline char __dcc_getchar(void)
 
 	return __c;
 }
-#endif
 
-#if defined(CONFIG_CPU_V7)
-static inline void __dcc_putchar(char c)
-{
-	asm volatile("put_wait:	mrc p14, 0, pc, c0, c1, 0         \n\
-			bcs put_wait                              \n\
-			mcr p14, 0, %0, c0, c5, 0                   "
-	: : "r" (c) : "cc");
-}
-#else
 static inline void __dcc_putchar(char c)
 {
 	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
 		: "r" (c));
 }
-#endif
 
 static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 {
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Acked-by
#23 lore raw

Re: [PATCH 3/3] hvc_dcc: Simplify assembly for v6 and v7 ARM

Quoted 22 lines · Nicolas Pitre
Quoted 20 lines · Stephen Boyd

The inline assembly differences for v6 vs. v7 in the hvc_dcc driver are purely optimizations. On a v7 processor, an mrc with the pc sets the condition codes to the 28-31 bits of the register being read. It just so happens that the TX/RX full bits the DCC driver is testing for are high enough in the register to be put into the condition codes. On a v6 processor, this "feature" isn't implemented and thus we have to do the usual read, mask, test operations to check for TX/RX full.

Since we already test the RX/TX full bits before calling __dcc_getchar() and __dcc_putchar() we don't actually need to do anything special for v7 over v6. The only difference is in hvc_dcc_get_chars(). We would test RX full, poll RX full, and then read a character from the buffer, whereas now we will test RX full, read a character from the buffer, and then test RX full again for the second iteration of the loop. It doesn't seem possible for the buffer to go from full to empty between testing the RX full and reading a character. Therefore, replace the v7 versions with the v6 versions and everything works the same.

Quoted 7 lines
Quoted 5 lines
Cc Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Acked-by Tony Lindgren <[email protected]>
#24 lore raw

Re: [PATCH 0/3] hvc_dcc cleanups and fixes

In reply to Stephen Boyd

Greg,

Quoted 1 line · Stephen Boyd

Here are the split patches.

Should I resend these with the proper acks or can/have you picked these patches up?

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#25 lore raw

Re: [PATCH 0/3] hvc_dcc cleanups and fixes

Quoted 7 lines · Stephen Boyd

Greg,

Quoted 1 line · Stephen Boyd

Here are the split patches.

Should I resend these with the proper acks or can/have you picked these patches up?

They are in my to-apply queue after .38-rc1 is out.

thanks,

greg k-h

#26 lore raw

Re: [PATCH 0/3] hvc_dcc cleanups and fixes

In reply to Stephen Boyd
Quoted 10 lines · Stephen Boyd

Here are the split patches.

The first two patches cleanup and fix the hvc_dcc driver for my compiler. The final patch is more controversial, it removes the v6 and v7 differences in this driver.

Stephen Boyd (3):
hvc_dcc: Fix bad code generation by marking assembly volatile
hvc_dcc: Simplify put_chars()/get_chars() loops
hvc_dcc: Simplify assembly for v6 and v7 ARM

Can you redo these against the linux-next tree as they don't apply anymore and resend them so I can apply them?

thanks,

greg k-h

#27 lore raw

Re: [PATCH 0/3] hvc_dcc cleanups and fixes

Quoted 7 lines · Stephen Boyd
Quoted 6 lines

Here are the split patches.

The first two patches cleanup and fix the hvc_dcc driver for my compiler. The final patch is more controversial, it removes the v6 and v7 differences in this driver.

Quoted 8 lines
Quoted 4 lines

Stephen Boyd (3):
hvc_dcc: Fix bad code generation by marking assembly volatile
hvc_dcc: Simplify put_chars()/get_chars() loops
hvc_dcc: Simplify assembly for v6 and v7 ARM

Can you redo these against the linux-next tree as they don't apply anymore and resend them so I can apply them?

Have you tried 'git am -3' on them? I assume it's the movement of the file that's causing the problem.

Either way, I'll resend and collect the acks in a bit.

#28 lore raw

Re: [PATCH 0/3] hvc_dcc cleanups and fixes

Quoted 12 lines · Stephen Boyd
Quoted 12 lines · Stephen Boyd
Quoted 10 lines

Here are the split patches.

The first two patches cleanup and fix the hvc_dcc driver for my compiler. The final patch is more controversial, it removes the v6 and v7 differences in this driver.

Stephen Boyd (3):
hvc_dcc: Fix bad code generation by marking assembly volatile
hvc_dcc: Simplify put_chars()/get_chars() loops
hvc_dcc: Simplify assembly for v6 and v7 ARM

Quoted 8 lines
Quoted 3 lines

Can you redo these against the linux-next tree as they don't apply anymore and resend them so I can apply them?

Have you tried 'git am -3' on them? I assume it's the movement of the file that's causing the problem.

Either way, I'll resend and collect the acks in a bit.

thanks, that would be great.

greg k-h

#29 lore raw

v2 cover hvc_dcc cleanups and fixes

In reply to Greg KH

Redone against tty-next and collected acks from Nicolas and Tony.

Stephen Boyd (3):
hvc_dcc: Fix bad code generation by marking assembly volatile
hvc_dcc: Simplify put_chars()/get_chars() loops
hvc_dcc: Simplify assembly for v6 and v7 ARM

 drivers/tty/hvc/hvc_dcc.c |   43 +++++++------------------------------------
 1 files changed, 7 insertions(+), 36 deletions(-)
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#30 lore raw

PATCH v2 1/3 hvc_dcc: Fix bad code generation by marking assembly volatile

Without marking the asm __dcc_getstatus() volatile my compiler decides it can cache the value of __ret in a register and then check the value of it continually in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).

00000000 <hvc_dcc_put_chars>:

   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX (bit 29) and then stored in r3 for later use. Marking the asm volatile produces the following:

00000000 <hvc_dcc_put_chars>:

   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr

which looks better and actually works. Mark all the inline assembly in this file as volatile since we don't want the compiler to optimize away these statements or move them around in any way.

Acked-by Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/tty/hvc/hvc_dcc.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/tty/hvc/hvc_dcc.c +5 -6
index 6470f63..155ec10 100644
@@ -33,8 +33,7 @@
 static inline u32 __dcc_getstatus(void)
 {
 	u32 __ret;
-
-	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
 		: "=r" (__ret) : : "cc");
 
 	return __ret;

@@ -46,7 +45,7 @@ static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
+	asm volatile("get_wait:	mrc p14, 0, pc, c0, c1, 0                  \n\
 			bne get_wait                                       \n\
 			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c) : : "cc");

@@ -58,7 +57,7 @@ static inline char __dcc_getchar(void)
 {
 	char __c;
 
-	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+	asm volatile("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
 		: "=r" (__c));
 
 	return __c;

@@ -68,7 +67,7 @@ static inline char __dcc_getchar(void)
 #if defined(CONFIG_CPU_V7)
 static inline void __dcc_putchar(char c)
 {
-	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
+	asm volatile("put_wait:	mrc p14, 0, pc, c0, c1, 0         \n\
 			bcs put_wait                              \n\
 			mcr p14, 0, %0, c0, c5, 0                   "
 	: : "r" (c) : "cc");

@@ -76,7 +75,7 @@ static inline void __dcc_putchar(char c)
 #else
 static inline void __dcc_putchar(char c)
 {
-	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
 		: "r" (c));
 }
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#31 lore raw

PATCH v2 2/3 hvc_dcc: Simplify put_chars()/get_chars() loops

In reply to Stephen Boyd

Casting and anding with 0xff is unnecessary in hvc_dcc_put_chars() since buf is already a char[]. __dcc_get_char() can't return an int less than 0 since it only returns a char. Simplify the if statement in hvc_dcc_get_chars() to take this into account.

Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/tty/hvc/hvc_dcc.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
drivers/tty/hvc/hvc_dcc.c +4 -8
index 155ec10..ad23cc8 100644
@@ -89,7 +89,7 @@ static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 		while (__dcc_getstatus() & DCC_STATUS_TX)
 			cpu_relax();
 
-		__dcc_putchar((char)(buf[i] & 0xFF));
+		__dcc_putchar(buf[i]);
 	}
 
 	return count;

@@ -99,15 +99,11 @@ static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
 {
 	int i;
 
-	for (i = 0; i < count; ++i) {
-		int c = -1;
-
+	for (i = 0; i < count; ++i)
 		if (__dcc_getstatus() & DCC_STATUS_RX)
-			c = __dcc_getchar();
-		if (c < 0)
+			buf[i] = __dcc_getchar();
+		else
 			break;
-		buf[i] = c;
-	}
 
 	return i;
 }
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#32 lore raw

PATCH v2 3/3 hvc_dcc: Simplify assembly for v6 and v7 ARM

In reply to Stephen Boyd

The inline assembly differences for v6 vs. v7 in the hvc_dcc driver are purely optimizations. On a v7 processor, an mrc with the pc sets the condition codes to the 28-31 bits of the register being read. It just so happens that the TX/RX full bits the DCC driver is testing for are high enough in the register to be put into the condition codes. On a v6 processor, this "feature" isn't implemented and thus we have to do the usual read, mask, test operations to check for TX/RX full.

Since we already test the RX/TX full bits before calling __dcc_getchar() and __dcc_putchar() we don't actually need to do anything special for v7 over v6. The only difference is in hvc_dcc_get_chars(). We would test RX full, poll RX full, and then read a character from the buffer, whereas now we will test RX full, read a character from the buffer, and then test RX full again for the second iteration of the loop. It doesn't seem possible for the buffer to go from full to empty between testing the RX full and reading a character. Therefore, replace the v7 versions with the v6 versions and everything works the same.

Acked-by Tony Lindgren <[email protected]>
Cc Arnd Bergmann <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Cc Daniel Walker <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/tty/hvc/hvc_dcc.c |   24 ------------------------
 1 files changed, 0 insertions(+), 24 deletions(-)
drivers/tty/hvc/hvc_dcc.c +0 -24
index ad23cc8..435f6fa 100644
@@ -40,19 +40,6 @@ static inline u32 __dcc_getstatus(void)
 }
 
 
-#if defined(CONFIG_CPU_V7)
-static inline char __dcc_getchar(void)
-{
-	char __c;
-
-	asm volatile("get_wait:	mrc p14, 0, pc, c0, c1, 0                  \n\
-			bne get_wait                                       \n\
-			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
-		: "=r" (__c) : : "cc");
-
-	return __c;
-}
-#else
 static inline char __dcc_getchar(void)
 {
 	char __c;

@@ -62,24 +49,13 @@ static inline char __dcc_getchar(void)
 
 	return __c;
 }
-#endif
 
-#if defined(CONFIG_CPU_V7)
-static inline void __dcc_putchar(char c)
-{
-	asm volatile("put_wait:	mrc p14, 0, pc, c0, c1, 0         \n\
-			bcs put_wait                              \n\
-			mcr p14, 0, %0, c0, c5, 0                   "
-	: : "r" (c) : "cc");
-}
-#else
 static inline void __dcc_putchar(char c)
 {
 	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
 		: "r" (c));
 }
-#endif
 
 static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 {
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#33 lore raw
In reply to Daniel Walker
Quoted 3 lines · Daniel Walker

Are you talking about __dcc_getstatus only? I don't think adding volatile is going to hurt anything, if not having it causes problems.

Just marking __dcc_getstatus volatile gives me

00000038 <hvc_dcc_get_chars>:

  38:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  3c:   1afffffd        bne     38 <hvc_dcc_get_chars>
  40:   ee103e15        mrc     14, 0, r3, cr0, cr5, {0}
  44:   e3a00000        mov     r0, #0  ; 0x0
  48:   e6ef3073        uxtb    r3, r3
  4c:   ea000004        b       64 <hvc_dcc_get_chars+0x2c>
  50:   ee10ce11        mrc     14, 0, ip, cr0, cr1, {0}
  54:   e31c0101        tst     ip, #1073741824 ; 0x40000000
  58:   012fff1e        bxeq    lr
  5c:   e7c13000        strb    r3, [r1, r0]
  60:   e2800001        add     r0, r0, #1      ; 0x1
  64:   e1500002        cmp     r0, r2
  68:   bafffff8        blt     50 <hvc_dcc_get_chars+0x18>
  6c:   e12fff1e        bx      lr

Seems the compiler keeps the value of __dcc_getchar() in r3 for the duration of the loop. So we need to mark that one volatile too. I don't think __dcc_putchar() needs to be marked volatile but it probably doesn't hurt.

Quoted 5 lines

We could maybe drop the looping for TX, but RX has no C based looping even tho for v7 it's recommended that we loop (presumably for v6 it's not recommended).

Definitely for TX since it seems like a redundant loop, but I agree RX code has changed. Instead of

If RX buffer full
Poll for RX buffer full
Read character from RX buffer

we would have

If RX buffer full
Read character from RX buffer

which doesn't seem all that different assuming the RX buffer doesn't go from full to empty between the If and Poll steps. Hopefully Tony knows more.

Quoted 11 lines

Like this?

	for (i = 0; i < count; ++i) {

		if (__dcc_getstatus() & DCC_STATUS_RX)
			buf[i] = __dcc_getchar();
		else
			break;
	}

It's a micro clean up I guess ..

Yes, it's much clearer that way.

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#34 lore raw
Quoted 7 lines · Stephen Boyd

Definitely for TX since it seems like a redundant loop, but I agree RX code has changed. Instead of

If RX buffer full
Poll for RX buffer full
Read character from RX buffer

Quoted 8 lines

we would have

If RX buffer full
Read character from RX buffer

which doesn't seem all that different assuming the RX buffer doesn't go from full to empty between the If and Poll steps. Hopefully Tony knows more.

Tony, any thoughts?

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#35 lore raw
Quoted 10 lines · Stephen Boyd
Quoted 9 lines · Stephen Boyd

Definitely for TX since it seems like a redundant loop, but I agree RX code has changed. Instead of

If RX buffer full
Poll for RX buffer full
Read character from RX buffer

we would have

Quoted 8 lines
Quoted 6 lines

If RX buffer full
Read character from RX buffer

which doesn't seem all that different assuming the RX buffer doesn't go from full to empty between the If and Poll steps. Hopefully Tony knows more.

Tony, any thoughts?

Sorry for the delay, looks like I'm only 1 month behind with email.. Sounds like it should work to me. I can try it out if you point me to a patch.

Regards,

Tony

#36 lore raw
Quoted 18 lines · Tony Lindgren
Quoted 17 lines · Stephen Boyd
Quoted 15 lines · Stephen Boyd

Definitely for TX since it seems like a redundant loop, but I agree RX code has changed. Instead of

If RX buffer full
Poll for RX buffer full
Read character from RX buffer

we would have

If RX buffer full
Read character from RX buffer

which doesn't seem all that different assuming the RX buffer doesn't go from full to empty between the If and Poll steps. Hopefully Tony knows more.

Quoted 5 lines
Quoted 1 line

Tony, any thoughts?

Sorry for the delay, looks like I'm only 1 month behind with email.. Sounds like it should work to me. I can try it out if you point me to a patch.

I think you acked the patches to make this change. You can test them out by applying the "hvc_dcc cleanups and fixes" patches (Message-Id:
<[email protected]>) on top of this patch. They were sent as a reply to this thread.

Stephen

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#37 lore raw
In reply to Daniel Walker

Hello,

Quoted 13 lines · Daniel Walker

This driver adds a basic console that uses the arm JTAG DCC to transfer data back and forth. It has support for ARMv6 and ARMv7.

This console is created under the HVC driver, and should be named /dev/hvcX (or /dev/hvc0 for example).

Cc Tony Lindgren<[email protected]>
Cc Arnd Bergmann<[email protected]>
Cc Nicolas Pitre<[email protected]>
Cc Greg Kroah-Hartman<[email protected]>
Cc Mike Frysinger<[email protected]>
Signed-off-by Daniel Walker<[email protected]>
Quoted 4 lines
---
  drivers/char/Kconfig   |    9 +++
  drivers/char/Makefile  |    1 +
  drivers/char/hvc_dcc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++++++

...

this DCC driver implements "one channel", but what about implementing "multiple channels". For example reserve few(3) bits for channel number, and two bits for carried data, then fill the rest bytes with with some data and send the word(32 bits) over DCC. On the Linux side writing on /dev/hvcX puts the number X as channel number, and on the other side the CPU emulator gets the data and redistribute it to TCP/IP socket.

I have started write some code implementing this. Are there any one interested in this multiple channels, and are there any one started to work on it?

Regards,
Asen

#38 lore raw
Quoted 22 lines · RONETIX - Asen Dimov
Quoted 11 lines · Daniel Walker

This driver adds a basic console that uses the arm JTAG DCC to transfer data back and forth. It has support for ARMv6 and ARMv7.

This console is created under the HVC driver, and should be named /dev/hvcX (or /dev/hvc0 for example).

 drivers/char/Kconfig   |    9 +++
 drivers/char/Makefile  |    1 +
 drivers/char/hvc_dcc.c |  133
++++++++++++++++++++++++++++++++++++++++++++++++

...

this DCC driver implements "one channel", but what about implementing "multiple channels". For example reserve few(3) bits for channel number, and two bits for carried data, then fill the rest bytes with with some data and send the word(32 bits) over DCC. On the Linux side writing on /dev/hvcX puts the number X as channel number, and on the other side the CPU emulator gets the data and redistribute it to TCP/IP socket.

Quoted 3 lines

I have started write some code implementing this. Are there any one interested
in this multiple channels, and are there any one started to work on it?

this sort of multiplexing of the data stream sounds like the job for userspace ? or maybe a line discipline ? inserting structured data into the kernel driver doesnt sound right to me ...
-mike