| Message ID | 1346413645-4593-1-git-send-email-castet.matthieu@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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
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
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
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
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.
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);
Signed-off-by: Matthieu Castet <matthieu.castet@parrot.com> --- drivers/tty/hvc/hvc_dcc.c | 83 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 76 insertions(+), 7 deletions(-)