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

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

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

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 <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Daniel Walker <dwalker@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/char/hvc_dcc.c |   43 +++++++------------------------------------
 1 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
index 6470f63..435f6fa 100644
--- a/drivers/char/hvc_dcc.c
+++ b/drivers/char/hvc_dcc.c
@@ -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;
 }
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Daniel Walker]

On Fri, 2010-12-17 at 21:16 -0800, Stephen Boyd wrote:
> 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.

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

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

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

[Stephen Boyd]

On 12/20/2010 09:51 AM, Daniel Walker wrote:
>
> 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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Nicolas Pitre]

On Mon, 20 Dec 2010, Stephen Boyd wrote:

> On 12/20/2010 09:51 AM, Daniel Walker wrote:
> >
> > 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

[PATCH 0/3] hvc_dcc cleanups and fixes

[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(-)

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

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

Cc: Tony Lindgren <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Daniel Walker <dwalker@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/char/hvc_dcc.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
index 6470f63..155ec10 100644
--- a/drivers/char/hvc_dcc.c
+++ b/drivers/char/hvc_dcc.c
@@ -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));
 }
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Nicolas Pitre]

On Mon, 20 Dec 2010, Stephen Boyd wrote:

> 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 <tony@atomide.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Daniel Walker <dwalker@codeaurora.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>


> ---
>  drivers/char/hvc_dcc.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> index 6470f63..155ec10 100644
> --- a/drivers/char/hvc_dcc.c
> +++ b/drivers/char/hvc_dcc.c
> @@ -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));
>  }
> -- 
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> 

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

[Pavel Machek]

Hi!

> > diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> > index 6470f63..155ec10 100644
> > --- a/drivers/char/hvc_dcc.c
> > +++ b/drivers/char/hvc_dcc.c
> > @@ -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;

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


> > @@ -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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[David Brown]

On Sun, Jan 02 2011, Pavel Machek wrote:

>> > diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
>> > index 6470f63..155ec10 100644
>> > --- a/drivers/char/hvc_dcc.c
>> > +++ b/drivers/char/hvc_dcc.c
>> > @@ -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;
>
> 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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Pavel Machek]

On Sun 2011-01-02 10:49:32, David Brown wrote:
> On Sun, Jan 02 2011, Pavel Machek wrote:
> 
> >> > diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> >> > index 6470f63..155ec10 100644
> >> > --- a/drivers/char/hvc_dcc.c
> >> > +++ b/drivers/char/hvc_dcc.c
> >> > @@ -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;
> >
> > 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.

Ok, thanks for explanation.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Tony Lindgren]

* Nicolas Pitre <nico@fluxnic.net> [101220 13:38]:
> On Mon, 20 Dec 2010, Stephen Boyd wrote:
> 
> > 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 <tony@atomide.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Cc: Daniel Walker <dwalker@codeaurora.org>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> 
> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

Acked-by: Tony Lindgren <tony@atomide.com>

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

[Arnaud Lacombe]

Hi,

On Mon, Dec 20, 2010 at 3:08 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 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

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

[Stephen Boyd]

On 12/20/2010 01:49 PM, Arnaud Lacombe wrote:
> Hi,
>
> On Mon, Dec 20, 2010 at 3:08 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> 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)".

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Nicolas Pitre]

On Mon, 20 Dec 2010, Stephen Boyd wrote:

> On 12/20/2010 01:49 PM, Arnaud Lacombe wrote:
> > Hi,
> >
> > On Mon, Dec 20, 2010 at 3:08 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >> 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)".

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

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

[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 <dwalker@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/char/hvc_dcc.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
index 155ec10..ad23cc8 100644
--- a/drivers/char/hvc_dcc.c
+++ b/drivers/char/hvc_dcc.c
@@ -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;
 }
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[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 <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Daniel Walker <dwalker@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/char/hvc_dcc.c |   24 ------------------------
 1 files changed, 0 insertions(+), 24 deletions(-)

diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
index ad23cc8..435f6fa 100644
--- a/drivers/char/hvc_dcc.c
+++ b/drivers/char/hvc_dcc.c
@@ -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)
 {
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Nicolas Pitre]

On Mon, 20 Dec 2010, Stephen Boyd wrote:

> 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 <tony@atomide.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Daniel Walker <dwalker@codeaurora.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

> ---
>  drivers/char/hvc_dcc.c |   24 ------------------------
>  1 files changed, 0 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> index ad23cc8..435f6fa 100644
> --- a/drivers/char/hvc_dcc.c
> +++ b/drivers/char/hvc_dcc.c
> @@ -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)
>  {
> -- 
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> 

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

[Tony Lindgren]

* Nicolas Pitre <nico@fluxnic.net> [101220 13:44]:
> On Mon, 20 Dec 2010, Stephen Boyd wrote:
> 
> > 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 <tony@atomide.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Cc: Daniel Walker <dwalker@codeaurora.org>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> 
> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

Acked-by: Tony Lindgren <tony@atomide.com>

[PATCH 0/3] hvc_dcc cleanups and fixes

[Stephen Boyd]

Greg,

On 12/20/2010 12:08 PM, Stephen Boyd wrote:
> Here are the split patches.

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

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 0/3] hvc_dcc cleanups and fixes

[Greg KH]

On Wed, Jan 05, 2011 at 05:49:17PM -0800, Stephen Boyd wrote:
> Greg,
> 
> On 12/20/2010 12:08 PM, Stephen Boyd wrote:
> > 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

[PATCH 0/3] hvc_dcc cleanups and fixes

[Greg KH]

On Mon, Dec 20, 2010 at 12:08:35PM -0800, Stephen Boyd wrote:
> 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

[PATCH 0/3] hvc_dcc cleanups and fixes

[Stephen Boyd]

> On Mon, Dec 20, 2010 at 12:08:35PM -0800, Stephen Boyd wrote:
>> 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?
> 

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.

[PATCH 0/3] hvc_dcc cleanups and fixes

[Greg KH]

On Thu, Feb 03, 2011 at 03:19:07PM -0800, Stephen Boyd wrote:
> > On Mon, Dec 20, 2010 at 12:08:35PM -0800, Stephen Boyd wrote:
> >> 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?
> > 
> 
> 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

[PATCHv2 0/3] hvc_dcc cleanups and fixes

[Stephen Boyd]

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

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

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

Acked-by: Tony Lindgren <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Daniel Walker <dwalker@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/tty/hvc/hvc_dcc.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index 6470f63..155ec10 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -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));
 }
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCHv2 2/3] hvc_dcc: Simplify put_chars()/get_chars() loops

[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 <dwalker@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/tty/hvc/hvc_dcc.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index 155ec10..ad23cc8 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -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;
 }
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[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 <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Daniel Walker <dwalker@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/tty/hvc/hvc_dcc.c |   24 ------------------------
 1 files changed, 0 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index ad23cc8..435f6fa 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -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)
 {
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.