Hi Mathieu,
On 11/5/20 8:52 PM, Mathieu Poirier wrote:
On Wed, Oct 28, 2020 at 10:09:33PM +0000, Suzuki K Poulose wrote:
ETMv4.4 architecture defines the system instructions for accessing ETM via register accesses. Add basic support for accessing a given register via system instructions.
Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com
.../coresight/coresight-etm4x-core.c | 39 ++ drivers/hwtracing/coresight/coresight-etm4x.h | 348 ++++++++++++++++-- 2 files changed, 365 insertions(+), 22 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 4af7d45dfe63..90b80982c615 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -56,6 +56,45 @@ static u64 etm4_get_access_type(struct etmv4_config *config); static enum cpuhp_state hp_online; +u64 etm4x_sysreg_read(struct csdev_access *csa,
u32 offset,bool _relaxed,bool _64bit)Please fix the stacking.
Sure.
+void etm4x_sysreg_write(struct csdev_access *csa,
u64 val,u32 offset,bool _relaxed,bool _64bit)Here too.
Sure.
/* Writing 0 to TRCOSLAR unlocks the trace registers */ diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 510828c73db6..5cf71b30a652 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.
+#define ETM4x_READ_CASES(res) CASE_LIST(READ, (res)) +#define ETM4x_WRITE_CASES(val) CASE_LIST(WRITE, (val))
+#define read_etm4x_sysreg_offset(csa, offset, _64bit) \
- ({ \
u64 __val; \\if (__builtin_constant_p((offset))) \Neat trick - I wonder how you stumbled on that one.
:-). There are plenty of uses in the kernel and glibc.
__val = read_etm4x_sysreg_const_offset((offset)); \else \__val = etm4x_sysreg_read((csa), (offset), \true, _64bit); \__val; \})+#define write_etm4x_sysreg_offset(csa, val, offset, _64bit) \
- do { \
if (__builtin_constant_p((offset))) \write_etm4x_sysreg_const_offset((val), \(offset)); \else \etm4x_sysreg_write((csa), (val), (offset), \true, _64bit); \- } while (0)
+#define etm4x_relaxed_read32(csa, offset) \
- ((u32)((csa)->io_mem ? \
readl_relaxed((csa)->base + (offset)) : \read_etm4x_sysreg_offset((csa), (offset), false)))Please add an extra new line - otherwise it is very hard to read.
Sure
+#define etm4x_relaxed_read64(csa, offset) \
- ((u64)((csa)->io_mem ? \
readq_relaxed((csa)->base + (offset)) : \read_etm4x_sysreg_offset((csa), (offset), true)))Here too.
sure
+#define etm4x_read32(csa, offset) \
- ({ \
u32 __val = etm4x_relaxed_read32((csa), (offset)); \__iormb(__val); \__val; \})+#define etm4x_read64(csa, offset) \
- ({ \
u64 __val = etm4x_relaxed_read64((csa), (offset)); \__iormb(__val); \__val; \})+#define etm4x_relaxed_write32(csa, val, offset) \
- do { \
if ((csa)->io_mem) \writel_relaxed((val), (csa)->base + (offset)); \else \write_etm4x_sysreg_offset((csa), (val), \(offset), false); \Why using an if/else statement and above the '?' condition marker? I would really like a uniform approach. Otherwise the reader keeps looking for something subtle when there isn't.
The write variants do not return a result, unlike the read. So, we cant use the '?'
- } while (0)
+#define etm4x_relaxed_write64(csa, val, offset) \
- do { \
if ((csa)->io_mem) \writeq_relaxed((val), (csa)->base + (offset)); \else \write_etm4x_sysreg_offset((csa), (val), \(offset), true); \- } while (0)
+#define etm4x_write32(csa, val, offset) \
- do { \
__iowmb(); \etm4x_relaxed_write32((csa), (val), (offset)); \- } while (0)
+#define etm4x_write64(csa, val, offset) \
- do { \
__iowmb(); \etm4x_relaxed_write64((csa), (val), (offset)); \- } while (0)
-#define etm4x_write64(csa, val, offset) \
- writeq((val), (csa)->base + (offset))
/* ETMv4 resources */ #define ETM_MAX_NR_PE 8 @@ -512,4 +806,14 @@ enum etm_addr_ctxtype { extern const struct attribute_group *coresight_etmv4_groups[]; void etm4_config_trace_mode(struct etmv4_config *config);
+u64 etm4x_sysreg_read(struct csdev_access *csa,
u32 offset,bool _relaxed,bool _64bit);+void etm4x_sysreg_write(struct csdev_access *csa,
u64 val,u32 offset,bool _relaxed,bool _64bit);With the above:
Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org
Thanks !
This patch holds together well. I commend you on rendering something that is quite complex into a manageable implementation. That being said it will impact Mike's complex configuration patchset (or Mike's complex configuration patchset will have an impact on this).
I understand. Will see when we get to it.
Cheers Suzuki