[PATCH] coresight-stm: adding driver for CoreSight STM component

On 5 February 2015 at 04:27, Paul Bolle [off-list ref] wrote:

On Wed, 2015-02-04 at 15:22 -0700, mathieu.poirier at linaro.org wrote:

quoted

From: Pratik Patel <redacted>

This driver adds support for the STM CoreSight IP block,
allowing any system compoment (HW or SW) to log and
aggregate messages via a single entity.

The STM exposes an application defined number of channels
called stimulus port.  Configuration is done using entries
in sysfs and channels made available to userspace via devfs.

Signed-off-by: Pratik Patel <redacted>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

This needs "coresight: Adding coresight support for arm64
architecture" (https://lkml.org/lkml/2015/2/3/677 ) in order to get
applied. Perhaps that's obvious to the people working on this.

Right, in my tree the code is tailored to use the other patch you're
referencing but having received no 'ack' for it yet I made this set
for 32 bit only (aside from the 64bit flags in Kconfig).  As such you
don't need https://lkml.org/lkml/2015/2/3/677 for this to compile and
run.

A few comments follow.

quoted

---
 .../ABI/testing/sysfs-bus-coresight-devices-stm    |   62 ++
 Documentation/trace/coresight.txt                  |   88 +-
 drivers/coresight/Kconfig                          |   10 +
 drivers/coresight/Makefile                         |    1 +
 drivers/coresight/coresight-stm.c                  | 1090 ++++++++++++++++++++
 include/linux/coresight-stm.h                      |   35 +
 include/uapi/linux/coresight-stm.h                 |   23 +
 7 files changed, 1307 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
 create mode 100644 drivers/coresight/coresight-stm.c
 create mode 100644 include/linux/coresight-stm.h
 create mode 100644 include/uapi/linux/coresight-stm.h

[...]

diff --git a/drivers/coresight/Kconfig b/drivers/coresight/Kconfig
index fc1f1ae7a49d..08806cc7d737 100644
--- a/drivers/coresight/Kconfig
+++ b/drivers/coresight/Kconfig

@@ -58,4 +58,14 @@ config CORESIGHT_SOURCE_ETM3X
        which allows tracing the instructions that a processor is executing
        This is primarily useful for instruction level tracing.  Depending
        the ETM version data tracing may also be available.
+
+config CORESIGHT_STM
+     bool "CoreSight System Trace Macrocell driver"
+     depends on (ARM && !(CPU_32v4 || CPU_32v4T)) || ARM64 || (64BIT && COMPILE_TEST)

I'm _guessing_ that CPU_32v4 and CPU_32v4T are needed for the ldrd and
strd assembler instructions. If that's right a next _guess_ would be
that you also need to mention CPU_32v3 here.

Furthermore, this file is only sourced by arch/arm/Kconfig.debug and
arch/arm64/Kconfig.debug. So 64BIT should always be equal to ARM64 and
the
     || (64BIT && COMPILE_TEST)

part shouldn't be needed, isn't it?

Things have changed in the driver (and accessor functions) since I
wrote this Kconfig condition - Let me think on the above 2 comments.

quoted

+     select CORESIGHT_LINKS_AND_SINKS
+     help
+       This driver provides support for hardware assisted software
+       instrumentation based tracing. This is primarily used for
+       logging useful software events or data coming from various entities
+       in the system, possibly running different OSs
 endif
[...]

diff --git a/drivers/coresight/coresight-stm.c b/drivers/coresight/coresight-stm.c
new file mode 100644
index 000000000000..e59b0fe01d87
--- /dev/null
+++ b/drivers/coresight/coresight-stm.c

@@ -0,0 +1,1090 @@

[...]
+#ifndef CONFIG_64BIT
+static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
+{
+     asm volatile("strd %1, %0"
+                  : "+Qo" (*(volatile u64 __force *)addr)
+                  : "r" (val));
+}
+
+static inline u64 __raw_readq(const volatile void __iomem *addr)
+{
+     u64 val;
+
+     asm volatile("ldrd %1, %0"
+                  : "+Qo" (*(volatile u64 __force *)addr),
+                    "=r" (val));
+     return val;
+}
+
+#undef readq_relaxed
+#define readq_relaxed(c) ({ u64 __r = le64_to_cpu((__force __le64) \
+                                     __raw_readq(c)); __r; })

I spotted no users of readq_relaxed. Is it needed?

quoted

+#undef writeq_relaxed
+#define writeq_relaxed(v, c) __raw_writeq((__force u64) cpu_to_le64(v), c)
+#endif
+
[...]

quoted

+static ssize_t entities_show(struct device *dev,
+                          struct device_attribute *attr, char *buf)
+{
+     struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+     ssize_t len;
+
+     len = bitmap_scnprintf(buf, PAGE_SIZE, drvdata->entities,
+                            STM_ENTITY_MAX);
+

bitmap_scnprintf is gone in current linux-next. I changed it to
        len = scnprintf(buf, PAGE_SIZE, "%*pb", STM_ENTITY_MAX,
                        drvdata->entities);

to get this file to compile. (On x86_64, that is, but please don't tell
anybody!)

I was not aware that "bitmap_scnprintf()" had disappeared - thanks for
pointing this out.  Did you mean cross-compile from an x86_64 host or
compile for a x86_64 target?  I don't get how you could have in the
latter case.

Thanks for the review,
Mathieu

Paul Bolle