[PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[Matthieu CASTET]

Signed-off-by: Matthieu Castet <matthieu.castet@parrot.com>
---
 drivers/tty/hvc/hvc_dcc.c |   83 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 76 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index 44fbeba..5f8827f 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -27,10 +27,10 @@
 #include "hvc_console.h"
 
 /* DCC Status Bits */
-#define DCC_STATUS_RX		(1 << 30)
-#define DCC_STATUS_TX		(1 << 29)
+#define DCC_STATUS_RX_V6		(1 << 30)
+#define DCC_STATUS_TX_V6		(1 << 29)
 
-static inline u32 __dcc_getstatus(void)
+static inline u32 __dcc_getstatus_v6(void)
 {
 	u32 __ret;
 	asm volatile("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
@@ -40,7 +40,7 @@ static inline u32 __dcc_getstatus(void)
 }
 
 
-static inline char __dcc_getchar(void)
+static inline char __dcc_getchar_v6(void)
 {
 	char __c;
 
@@ -51,7 +51,7 @@ static inline char __dcc_getchar(void)
 	return __c;
 }
 
-static inline void __dcc_putchar(char c)
+static inline void __dcc_putchar_v6(char c)
 {
 	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
 		: /* no output register */
@@ -59,6 +59,69 @@ static inline void __dcc_putchar(char c)
 	isb();
 }
 
+static int hvc_dcc_put_chars_v6(uint32_t vt, const char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		while (__dcc_getstatus_v6() & DCC_STATUS_TX_V6)
+			cpu_relax();
+
+		__dcc_putchar_v6(buf[i]);
+	}
+
+	return count;
+}
+
+static int hvc_dcc_get_chars_v6(uint32_t vt, char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; ++i)
+		if (__dcc_getstatus_v6() & DCC_STATUS_RX_V6)
+			buf[i] = __dcc_getchar_v6();
+		else
+			break;
+
+	return i;
+}
+
+static const struct hv_ops hvc_dcc_get_put_ops_v6 = {
+	.get_chars = hvc_dcc_get_chars_v6,
+	.put_chars = hvc_dcc_put_chars_v6,
+};
+
+#define DCC_STATUS_RX		(1 << 0)
+#define DCC_STATUS_TX		(1 << 1)
+
+/* primitive JTAG1 protocol utilities */
+static inline u32 __dcc_getstatus(void)
+{
+	u32 ret;
+
+	asm __volatile__ ("mrc p14, 0, %0, c0, c0	@ read comms ctrl reg"
+		: "=r" (ret));
+
+	return ret;
+}
+
+static inline char __dcc_getchar(void)
+{
+	char c;
+
+	asm __volatile__ ("mrc p14, 0, %0, c1, c0	@ read comms data reg"
+		: "=r" (c));
+
+	return c;
+}
+
+static inline void __dcc_putchar(unsigned char c)
+{
+	asm __volatile__ ("mcr p14, 0, %0, c1, c0	@ write a char"
+		: /* no output register */
+		: "r" (c));
+}
+
 static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
 {
 	int i;
@@ -93,14 +156,20 @@ static const struct hv_ops hvc_dcc_get_put_ops = {
 
 static int __init hvc_dcc_console_init(void)
 {
-	hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
+	if (cpu_architecture() >= CPU_ARCH_ARMv6)
+		hvc_instantiate(0, 0, &hvc_dcc_get_put_ops_v6);
+	else
+		hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
 	return 0;
 }
 console_initcall(hvc_dcc_console_init);
 
 static int __init hvc_dcc_init(void)
 {
-	hvc_alloc(0, 0, &hvc_dcc_get_put_ops, 128);
+	if (cpu_architecture() >= CPU_ARCH_ARMv6)
+		hvc_alloc(0, 0, &hvc_dcc_get_put_ops_v6, 128);
+	else
+		hvc_alloc(0, 0, &hvc_dcc_get_put_ops, 128);
 	return 0;
 }
 device_initcall(hvc_dcc_init);
-- 
1.7.10.4

Re: [PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[Stephen Boyd]

On 8/31/2012 4:47 AM, Matthieu CASTET wrote:
> Signed-off-by: Matthieu Castet <matthieu.castet@parrot.com>

Please consider adding some sort of commit text. Does this add some new
feature I may want on some downstream distro kernel?

> @@ -51,7 +51,7 @@ static inline char __dcc_getchar(void)
>  	return __c;
>  }
>  
> -static inline void __dcc_putchar(char c)
> +static inline void __dcc_putchar_v6(char c)
>  {
>  	asm volatile("mcr p14, 0, %0, c0, c5, 0	@ write a char"
>  		: /* no output register */
> @@ -59,6 +59,69 @@ static inline void __dcc_putchar(char c)
>  	isb();
>  }
>  
> +static int hvc_dcc_put_chars_v6(uint32_t vt, const char *buf, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		while (__dcc_getstatus_v6() & DCC_STATUS_TX_V6)
> +			cpu_relax();
> +
> +		__dcc_putchar_v6(buf[i]);
> +	}
> +
> +	return count;
> +}

It's unfortunate that the main logic is duplicated. I wonder if we could
push the runtime decision slightly lower into the accessor functions
instead and make some new functions dcc_tx_busy() and dcc_rx_busy() or
something. Then these loops stay the same.

> +
> +static int hvc_dcc_get_chars_v6(uint32_t vt, char *buf, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; ++i)
> +		if (__dcc_getstatus_v6() & DCC_STATUS_RX_V6)
> +			buf[i] = __dcc_getchar_v6();
> +		else
> +			break;
> +
> +	return i;
> +}
> +
> +static const struct hv_ops hvc_dcc_get_put_ops_v6 = {
> +	.get_chars = hvc_dcc_get_chars_v6,
> +	.put_chars = hvc_dcc_put_chars_v6,
> +};
> +
> +#define DCC_STATUS_RX		(1 << 0)
> +#define DCC_STATUS_TX		(1 << 1)
> +
> +/* primitive JTAG1 protocol utilities */

This comment doesn't tell me much. Remove it?

> +static inline u32 __dcc_getstatus(void)
> +{
> +	u32 ret;
> +
> +	asm __volatile__ ("mrc p14, 0, %0, c0, c0	@ read comms ctrl reg"
> +		: "=r" (ret));

Can you use volatile instead of __volatile__ so that the file is consistent?

> +
> +	return ret;
> +}
> +
> +static inline char __dcc_getchar(void)
> +{
> +	char c;
> +
> +	asm __volatile__ ("mrc p14, 0, %0, c1, c0	@ read comms data reg"
> +		: "=r" (c));
> +

Do you see any multiple character inputs? I think you may need an isb
here similar to the v6/7 code and in the putchar as well.

> +	return c;
> +}
> +
> +static inline void __dcc_putchar(unsigned char c)
> +{
> +	asm __volatile__ ("mcr p14, 0, %0, c1, c0	@ write a char"
> +		: /* no output register */
> +		: "r" (c));
> +}
> +
>  static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
>  {
>  	int i;
>

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

Re: [PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[Arnd Bergmann]

On Friday 31 August 2012, Stephen Boyd wrote:
> > +static int hvc_dcc_put_chars_v6(uint32_t vt, const char *buf, int count)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < count; i++) {
> > +             while (__dcc_getstatus_v6() & DCC_STATUS_TX_V6)
> > +                     cpu_relax();
> > +
> > +             __dcc_putchar_v6(buf[i]);
> > +     }
> > +
> > +     return count;
> > +}
> 
> It's unfortunate that the main logic is duplicated. I wonder if we could
> push the runtime decision slightly lower into the accessor functions
> instead and make some new functions dcc_tx_busy() and dcc_rx_busy() or
> something. Then these loops stay the same.

Agreed. Ideally, you should be able to get the code to be compiled into
the same binary as before for ARMv6+. If only the inline assembly differs,
you can do something like

static inline char __dcc_getchar(void)
{
        char __c;

        if (__LINUX_ARM_ARCH >= 6)
		asm volatile("mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
	                : "=r" (__c));
	else
		asm volatile ("mrc p14, 0, %0, c1, c0  @ read comms data reg"
			: "=r" (ret));
        isb();

        return __c;
}

	Arnd

Re: [PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[Matthieu CASTET]

Arnd Bergmann a écrit :
> On Friday 31 August 2012, Stephen Boyd wrote:
>>> +static int hvc_dcc_put_chars_v6(uint32_t vt, const char *buf, int count)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = 0; i < count; i++) {
>>> +             while (__dcc_getstatus_v6() & DCC_STATUS_TX_V6)
>>> +                     cpu_relax();
>>> +
>>> +             __dcc_putchar_v6(buf[i]);
>>> +     }
>>> +
>>> +     return count;
>>> +}
>> It's unfortunate that the main logic is duplicated. I wonder if we could
>> push the runtime decision slightly lower into the accessor functions
>> instead and make some new functions dcc_tx_busy() and dcc_rx_busy() or
>> something. Then these loops stay the same.
The code is so small (30 asm + 30 C code) that I wonder if worth adding
complexity in the code.
Also calling cpu_architecture isn't free and if the want to put the runtime
decision into the hot path, this means we need to cache the result.

> 
> Agreed. Ideally, you should be able to get the code to be compiled into
> the same binary as before for ARMv6+. If only the inline assembly differs,
> you can do something like
> 
> static inline char __dcc_getchar(void)
> {
>         char __c;
> 
>         if (__LINUX_ARM_ARCH >= 6)
> 		asm volatile("mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
> 	                : "=r" (__c));
> 	else
> 		asm volatile ("mrc p14, 0, %0, c1, c0  @ read comms data reg"
> 			: "=r" (ret));
>         isb();
> 
>         return __c;
> }
> 
> 	Arnd
> 
Yes doing that will be great!

But Alan wanted "all be runtime handled".

May be we can do something like:


static int cpu_arch;

static inline char __dcc_getchar(void)
{
         char __c;

         if (cpu_arch >= 6)
 		asm volatile("mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
 	                : "=r" (__c));
 	else
 		asm volatile ("mrc p14, 0, %0, c1, c0  @ read comms data reg"
 			: "=r" (ret));
         isb();

         return __c;
}

static int __init hvc_dcc_console_init(void)
{
	cpu_arch = cpu_architecture();
...

}
Matthieu

Re: [PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[Matthieu CASTET]

Stephen Boyd a écrit :
> On 8/31/2012 4:47 AM, Matthieu CASTET wrote:
>> Signed-off-by: Matthieu Castet <matthieu.castet@parrot.com>
> 
> Please consider adding some sort of commit text. Does this add some new
> feature I may want on some downstream distro kernel?
> 
ok
> 
> It's unfortunate that the main logic is duplicated. I wonder if we could
> push the runtime decision slightly lower into the accessor functions
> instead and make some new functions dcc_tx_busy() and dcc_rx_busy() or
> something. Then these loops stay the same.
see my previous mail

>> +static inline char __dcc_getchar(void)
>> +{
>> +	char c;
>> +
>> +	asm __volatile__ ("mrc p14, 0, %0, c1, c0	@ read comms data reg"
>> +		: "=r" (c));
>> +
> 
> Do you see any multiple character inputs? I think you may need an isb
> here similar to the v6/7 code and in the putchar as well.
I don't see multiple character.
On armv5 isb is only a memory barrier (__asm__ __volatile__ ("" : : : "memory"))
 and it may be not need for dcc operation.


Matthieu

Re: [PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[Arnd Bergmann]

On Tuesday 25 September 2012, Matthieu CASTET wrote:
> > static inline char __dcc_getchar(void)
> > {
> >         char __c;
> > 
> >         if (__LINUX_ARM_ARCH >= 6)
> >               asm volatile("mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
> >                       : "=r" (__c));
> >       else
> >               asm volatile ("mrc p14, 0, %0, c1, c0  @ read comms data reg"
> >                       : "=r" (ret));
> >         isb();
> > 
> >         return __c;
> > }
> > 
> >       Arnd
> > 
> Yes doing that will be great!
> 
> But Alan wanted "all be runtime handled".
> 
> May be we can do something like:
> 
> 
> static int cpu_arch;
> 
> static inline char __dcc_getchar(void)
> {
>          char __c;
> 
>          if (cpu_arch >= 6)
>                 asm volatile("mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
>                         : "=r" (__c));
>         else
>                 asm volatile ("mrc p14, 0, %0, c1, c0  @ read comms data reg"
>                         : "=r" (ret));
>          isb();
> 
>          return __c;
> }

It's not possible to build a kernel that runs on ARMv5 (or below) and also on
ARMv6 (or above), so the effect would be exactly the same. While we are putting
a lot of effort into making all sorts of ARMv6 and ARMv7 based systems work
with the same kernel binary, it's highly unlikely we would ever need the
above to be runtime selected, because there are a lot of other differences
at assembly level.

	Arnd

Re: [PATCH v2] hvc_dcc : add support to armv4 and armv5 core

[matthieu castet]

Le Tue, 25 Sep 2012 15:44:57 +0000,
Arnd Bergmann <arnd@arndb.de> a écrit :

> 
> It's not possible to build a kernel that runs on ARMv5 (or below) and
> also on ARMv6 (or above), so the effect would be exactly the same.
> While we are putting a lot of effort into making all sorts of ARMv6
> and ARMv7 based systems work with the same kernel binary, it's highly
> unlikely we would ever need the above to be runtime selected, because
> there are a lot of other differences at assembly level.
> 
I know but Alan was not happy with the static version with ifdef [1]
[2]. That why a dynamic version was done.


Matthieu


[1]
> > There are no plans to ever make it possible; there are too many
> > significant differences between ARMv4, v5 architectures and
> > ARMv6,v7 architectures to warrant making this runtime selectable.  
> 
> Then bury this crap in platform files please not in the drivers/tty
> layer code. Make it a platform driver provided callback or something.
> 
> Alan

[2]
> > I posted a new v2 patch that make the selection dynamic.  
> 
> Thanks - fine with that one - or with burying it in headers etc in
> the arm subdirs.