2014-06-16 23:58:32

by Abhimanyu Kapur

[permalink] [raw]
Subject: [PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Add support for debug communications channel based
hvc console for arm64 cpus.

Signed-off-by: Abhimanyu Kapur <[email protected]>
---
arch/arm64/include/asm/dcc.h | 41 +++++++++++++++++++++++++++++++++++++++++
drivers/tty/hvc/Kconfig | 2 +-
2 files changed, 42 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/include/asm/dcc.h

diff --git a/arch/arm64/include/asm/dcc.h b/arch/arm64/include/asm/dcc.h
new file mode 100644
index 0000000..ef74324
--- /dev/null
+++ b/arch/arm64/include/asm/dcc.h
@@ -0,0 +1,41 @@
+/* Copyright (c) 2014 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/barrier.h>
+
+static inline u32 __dcc_getstatus(void)
+{
+ u32 __ret;
+
+ asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
+ : : "cc");
+
+ return __ret;
+}
+
+static inline char __dcc_getchar(void)
+{
+ char __c;
+
+ asm volatile("mrs %0, dbgdtrrx_el0" : "=r" (__c));
+ isb();
+
+ return __c;
+}
+
+static inline void __dcc_putchar(char c)
+{
+ asm volatile("msr dbgdtrtx_el0, %0"
+ : /* No output register */
+ : "r" (c));
+ isb();
+}
diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
index 8902f9b..02051a1 100644
--- a/drivers/tty/hvc/Kconfig
+++ b/drivers/tty/hvc/Kconfig
@@ -88,7 +88,7 @@ config HVC_UDBG

config HVC_DCC
bool "ARM JTAG DCC console"
- depends on ARM
+ depends on ARM || ARM64
select HVC_DRIVER
help
This console uses the JTAG DCC on ARM to create a console under the HVC
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation


2014-06-17 09:52:28

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

On Mon, Jun 16, 2014 at 11:29:38PM +0100, Abhimanyu Kapur wrote:
> Add support for debug communications channel based
> hvc console for arm64 cpus.

Should we be setting MDSCR_EL1.TDCC to prevent userspace access to the DCC?

> Signed-off-by: Abhimanyu Kapur <[email protected]>
> ---
> arch/arm64/include/asm/dcc.h | 41 +++++++++++++++++++++++++++++++++++++++++
> drivers/tty/hvc/Kconfig | 2 +-
> 2 files changed, 42 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/include/asm/dcc.h
>
> diff --git a/arch/arm64/include/asm/dcc.h b/arch/arm64/include/asm/dcc.h
> new file mode 100644
> index 0000000..ef74324
> --- /dev/null
> +++ b/arch/arm64/include/asm/dcc.h
> @@ -0,0 +1,41 @@
> +/* Copyright (c) 2014 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <asm/barrier.h>
> +
> +static inline u32 __dcc_getstatus(void)
> +{
> + u32 __ret;

Can this result in the mrs receiving a W register?

> + asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
> + : : "cc");

Why the CC clobber? Why volatile?

> +
> + return __ret;
> +}
> +
> +static inline char __dcc_getchar(void)
> +{
> + char __c;
> +
> + asm volatile("mrs %0, dbgdtrrx_el0" : "=r" (__c));
> + isb();

Why the isb and why volatile??

> +
> + return __c;
> +}
> +
> +static inline void __dcc_putchar(char c)
> +{
> + asm volatile("msr dbgdtrtx_el0, %0"
> + : /* No output register */
> + : "r" (c));

Can you guarantee that GCC hasn't put junk in the upper bits of c?

> + isb();

Why the isb?

Will

2015-06-05 18:35:29

by Timur Tabi

[permalink] [raw]
Subject: Re: [PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

On Tue, Jun 17, 2014 at 4:51 AM, Will Deacon <[email protected]> wrote:
> On Mon, Jun 16, 2014 at 11:29:38PM +0100, Abhimanyu Kapur wrote:
>> Add support for debug communications channel based
>> hvc console for arm64 cpus.

I know it's been a year since this patch was posted, but I'm working
on re-submitting it. Unfortunately, I can't answer most of your
questions, but I want to try to fix this patch up and get it accepted.

> Should we be setting MDSCR_EL1.TDCC to prevent userspace access to the DCC?

I personally don't know enough about ARM to answer that question, but
it does makes sense that we don't want userspace to access the DCC
registers. Where would MDSCR_EL1.TDCC be set? Should we add it to
hvc_dcc_init()?

>> Signed-off-by: Abhimanyu Kapur <[email protected]>
>> ---
>> arch/arm64/include/asm/dcc.h | 41 +++++++++++++++++++++++++++++++++++++++++
>> drivers/tty/hvc/Kconfig | 2 +-
>> 2 files changed, 42 insertions(+), 1 deletion(-)
>> create mode 100644 arch/arm64/include/asm/dcc.h
>>
>> diff --git a/arch/arm64/include/asm/dcc.h b/arch/arm64/include/asm/dcc.h
>> new file mode 100644
>> index 0000000..ef74324
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/dcc.h
>> @@ -0,0 +1,41 @@
>> +/* Copyright (c) 2014 The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <asm/barrier.h>
>> +
>> +static inline u32 __dcc_getstatus(void)
>> +{
>> + u32 __ret;
>
> Can this result in the mrs receiving a W register?

Another question I can't answer. I understand what you're saying, but
I don't know what to do about it.

>> + asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
>> + : : "cc");
>
> Why the CC clobber? Why volatile?

I don't think we need the CC clobber, but the volatiles do appear to
be important. Without that, DCC does not work.

>> +
>> + return __ret;
>> +}
>> +
>> +static inline char __dcc_getchar(void)
>> +{
>> + char __c;
>> +
>> + asm volatile("mrs %0, dbgdtrrx_el0" : "=r" (__c));
>> + isb();
>
> Why the isb and why volatile??

The driver does appear to work without the isb, but there's a reason
it was added. On ARM32, reading/writing the DCC register is
apparently a context change:

http://www.serverphorums.com/read.php?12,395361

I don't know if the same thing is true on ARM64. However, I don't
understand why reading/writing the DCC register is a context change on
ARM32.

>> +
>> + return __c;
>> +}
>> +
>> +static inline void __dcc_putchar(char c)
>> +{
>> + asm volatile("msr dbgdtrtx_el0, %0"
>> + : /* No output register */
>> + : "r" (c));
>
> Can you guarantee that GCC hasn't put junk in the upper bits of c?

Should I change the prototype to:

static inline void __dcc_putchar(unsigned int c)


>
>> + isb();
>
> Why the isb?
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.