Ampere Computing ETMv4.x Support

An Ampere Computing design decision is MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is left set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read should be valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.

The Ampere work around is to early clear ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Do not distinguish between manufacturers. AC03_DEBUG_06 is described in the AmpereOne Developer Errata:

Add Ampere ETM PID required for Coresight ETM driver support.

https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Signed-off-by Steve Clevenger <[email protected]>

Hi Steve

Quoted 10 lines · Steve Clevenger

An Ampere Computing design decision is MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is left set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read should be valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.

The Ampere work around is to early clear ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Do not distinguish between manufacturers. AC03_DEBUG_06 is described in the AmpereOne Developer Errata:

Quoted 5 lines

Add Ampere ETM PID required for Coresight ETM driver support.

https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Signed-off-by Steve Clevenger <[email protected]>

I went back to the CoreSight ETM4x architecture and this is not an erratum at your end. This is actually a bug in the ETMv4 driver.

TRCIDR1 is part of the "Trace" and not "Management" registers of ETMv4. Access to "Trace" registers without OSLK is going to cause issues. (Un)Fortunately, we never hit this on existing systems, but that doesn't mean it can stay there.

So we should really fix our detection and only rely on accessing the TRCDEVARCH to detect the device to be ETMv4.

I have a sent out the fix here  [0], are you able to test it :

[0]
https://lkml.kernel.org/r/[email protected]

Suzuki

An Ampere Computing design decision to not support 64-bit read/write access in the ETMv4.6 implementation.

The Ampere work around is to split ETMv4.6 64-bit register access into 2 ea. 32-bit read/write accesses.
AC03_DEBUG_10 is described in the AmpereOne Developer Errata document:

https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Signed-off-by Steve Clevenger <[email protected]>

---
 drivers/hwtracing/coresight/coresight-etm4x.h | 58 ++++++++++++++-----
 1 file changed, 44 insertions(+), 14 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x.h +11 -14

index 434f4e95ee17..17457ec71876 100644
@@ -539,11 +539,6 @@
 		 readl_relaxed((csa)->base + (offset)) :		\
 		 read_etm4x_sysreg_offset((offset), false)))
 
-#define etm4x_relaxed_read64(csa, offset)				\
-	((u64)((csa)->io_mem ?						\
-		 readq_relaxed((csa)->base + (offset)) :		\
-		 read_etm4x_sysreg_offset((offset), true)))
-
 #define etm4x_read32(csa, offset)					\
 	({								\
 		u32 __val = etm4x_relaxed_read32((csa), (offset));	\

@@ -567,15 +562,6 @@
 						  false);		\
 	} 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((val), (offset),	\
-						  true);		\
-	} while (0)
-
 #define etm4x_write32(csa, val, offset)					\
 	do {								\
 		__io_bw();						\

@@ -1091,6 +1077,50 @@ void etm4_config_trace_mode(struct etmv4_config *config);
 u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit);
 void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit);
 
+/* 64-bit aligned to convert 64-bit access to 2 ea. 32-bit access */
+#pragma pack(push, 8)
+
+struct etm_quad_split {
+	u32 lsw;
+	u32 msw;
+};
+
+#pragma pack(pop)
+
+static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)

drivers/hwtracing/coresight/coresight-etm4x.h +7 -0

+}
+
+static inline void etm4x_relaxed_write64(struct csdev_access *csa, u64 quad, unsigned int offset)
+{
+	if (csa->io_mem) {
+		/* split 64-bit writes into 2 consecutive 32-bit writes */
+		if (csa->no_quad_mmio) {

drivers/hwtracing/coresight/coresight-etm4x.h +2 -0

+}
+
 static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)
 {
 	return drvdata->arch >= ETM_ARCH_ETE;

Signature · 1 line

2.25.1

Hi Steve,

Quoted 34 lines · Steve Clevenger

An Ampere Computing design decision to not support 64-bit read/write access in the ETMv4.6 implementation.

The Ampere work around is to split ETMv4.6 64-bit register access into 2 ea. 32-bit read/write accesses.
AC03_DEBUG_10 is described in the AmpereOne Developer Errata document:

https://solutions.amperecomputing.com/customer-connect/products/Ampere
One-device-documentation

Signed-off-by Steve Clevenger <[email protected]>

---
 drivers/hwtracing/coresight/coresight-etm4x.h | 58
++++++++++++++-----
 1 file changed, 44 insertions(+), 14 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x.h +0 -5

index 434f4e95ee17..17457ec71876 100644
@@ -539,11 +539,6 @@
 		 readl_relaxed((csa)->base + (offset)) :		\
 		 read_etm4x_sysreg_offset((offset), false)))
 
-#define etm4x_relaxed_read64(csa, offset)				\
-	((u64)((csa)->io_mem ?						\
-		 readq_relaxed((csa)->base + (offset)) :		\
-		 read_etm4x_sysreg_offset((offset), true)))
-
 #define etm4x_read32(csa, offset)					\
 	({								\
 		u32 __val = etm4x_relaxed_read32((csa), (offset));	\

drivers/hwtracing/coresight/coresight-etm4x.h line 570

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +0 -1

@@ -567,15 +562,6 @@
 						  false);		\
 	} while (0)
 
-#define etm4x_relaxed_write64(csa, val, offset)

	\

drivers/hwtracing/coresight/coresight-etm4x.h

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x.h +0 -8

-	do {								\
-		if ((csa)->io_mem)					\
-			writeq_relaxed((val), (csa)->base + (offset));	\
-		else							\
-			write_etm4x_sysreg_offset((val), (offset),	\
-						  true);		\
-	} while (0)
-
 #define etm4x_write32(csa, val, offset)

	\

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm4x.h +0 -0

 	do {								\
 		__io_bw();						\

@@ -1091,6 +1077,50 @@ void etm4_config_trace_mode(struct

etmv4_config

Quoted 13 lines

drivers/hwtracing/coresight/coresight-etm4x.h +9 -0

 *config);
 u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit);  void
 etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit);
 
+/* 64-bit aligned to convert 64-bit access to 2 ea. 32-bit access */
+#pragma pack(push, 8)
+
+struct etm_quad_split {
+	u32 lsw;
+	u32 msw;
+};
+
+#pragma pack(pop)

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 10 lines

drivers/hwtracing/coresight/coresight-etm4x.h +10 -0

+
+static inline u64 etm4x_relaxed_read64(struct csdev_access *csa,
+unsigned int offset) {
+	if (csa->io_mem) {
+		if (csa->no_quad_mmio) {
+			/* split 64-bit reads into 2 consecutive 32-bit reads */
+			struct etm_quad_split container;
+
+			container.lsw = etm4x_read32(csa, offset);
+			container.msw = etm4x_read32(csa, offset +

sizeof(u32));

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct · line 1101

Quoted 2 lines

drivers/hwtracing/coresight/coresight-etm4x.h +2 -0

+
+			return *(u64 *) &container;

To be honest, I am not familiar with this part, just want to remind two things.

Firstly, here you could use hi_lo_readq_relaxed(), seems to me, hi_lo_readq_relaxed() does the same thing with above section and you don't need to define the structure etm_quad_split().

Secondly, IIUC, a main problem with splitting 64-bit access into two 32-bit accesses is breaking atomicity. If here have race condition between reading and writing 64-bit registers, we need to consider to use spinlock for register accessing.

I'd like to leave the second issue to Suzuki/Mike/James for comfirmation in case I introduced complexity.

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1104

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+		} else
+			return readq_relaxed(csa->base + offset);
+	} else
+		return read_etm4x_sysreg_offset(offset, true);

Here need to add brackets:

        } else {
                return read_etm4x_sysreg_offset(offset, true);
        }

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 13 lines

drivers/hwtracing/coresight/coresight-etm4x.h +13 -0

+}
+
+static inline void etm4x_relaxed_write64(struct csdev_access *csa,
+u64 quad, unsigned int offset) {
+	if (csa->io_mem) {
+		/* split 64-bit writes into 2 consecutive 32-bit writes */
+		if (csa->no_quad_mmio) {
+			struct etm_quad_split container;
+
+			*(u64 *) &container = quad;
+
+			etm4x_relaxed_write32(csa, container.lsw, offset);
+			etm4x_relaxed_write32(csa, container.msw, offset +

sizeof(u32));

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1121

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+		} else
+			writeq_relaxed(quad, csa->base + offset);
+	} else
+		write_etm4x_sysreg_offset(quad, offset, true);		\

Ditto. Please drop the character '\' at the end of the line.

Thanks,
Leo

P.s. I have a ADLink AVA platform (Ampere Altra SoC with 32 CPUs), I am glad to give a test if you can confirm this patch set can apply on it (and please clarify if there have any prerequisite for firmwares).

Quoted 7 lines

drivers/hwtracing/coresight/coresight-etm4x.h +2 -1

+}
+
 static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)  {
 	return drvdata->arch >= ETM_ARCH_ETE;
--

2.25.1

_______________________________________________
CoreSight mailing list -- [email protected] To unsubscribe send an email to [email protected]

Quoted 86 lines

Hi Steve,

Quoted 34 lines · Steve Clevenger

An Ampere Computing design decision to not support 64-bit read/write access in the ETMv4.6 implementation.

The Ampere work around is to split ETMv4.6 64-bit register access into 2 ea. 32-bit read/write accesses.
AC03_DEBUG_10 is described in the AmpereOne Developer Errata document:

https://solutions.amperecomputing.com/customer-connect/products/Ampere
One-device-documentation

Signed-off-by Steve Clevenger <[email protected]>

---
 drivers/hwtracing/coresight/coresight-etm4x.h | 58
++++++++++++++-----
 1 file changed, 44 insertions(+), 14 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x.h +0 -5

index 434f4e95ee17..17457ec71876 100644
@@ -539,11 +539,6 @@
             readl_relaxed((csa)->base + (offset)) :                \
             read_etm4x_sysreg_offset((offset), false)))
 
-#define etm4x_relaxed_read64(csa, offset)                          \
-   ((u64)((csa)->io_mem ?                                          \
-            readq_relaxed((csa)->base + (offset)) :                \
-            read_etm4x_sysreg_offset((offset), true)))
-
 #define etm4x_read32(csa, offset)                                  \
    ({                                                              \
            u32 __val = etm4x_relaxed_read32((csa), (offset));      \

drivers/hwtracing/coresight/coresight-etm4x.h line 570

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +0 -1

@@ -567,15 +562,6 @@
                                              false);               \
    } while (0)
 
-#define etm4x_relaxed_write64(csa, val, offset)

      \

drivers/hwtracing/coresight/coresight-etm4x.h

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x.h +0 -8

-   do {                                                            \
-           if ((csa)->io_mem)                                      \
-                   writeq_relaxed((val), (csa)->base + (offset));  \
-           else                                                    \
-                   write_etm4x_sysreg_offset((val), (offset),      \
-                                             true);                \
-   } while (0)
-
 #define etm4x_write32(csa, val, offset)

      \

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm4x.h +0 -0

    do {                                                            \
            __io_bw();                                              \

@@ -1091,6 +1077,50 @@ void etm4_config_trace_mode(struct

etmv4_config

Quoted 13 lines

drivers/hwtracing/coresight/coresight-etm4x.h +9 -0

 *config);
 u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit);  void
 etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit);
 
+/* 64-bit aligned to convert 64-bit access to 2 ea. 32-bit access */
+#pragma pack(push, 8)
+
+struct etm_quad_split {
+   u32 lsw;
+   u32 msw;
+};
+
+#pragma pack(pop)

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 10 lines

drivers/hwtracing/coresight/coresight-etm4x.h +10 -0

+
+static inline u64 etm4x_relaxed_read64(struct csdev_access *csa,
+unsigned int offset) {
+   if (csa->io_mem) {
+           if (csa->no_quad_mmio) {
+                   /* split 64-bit reads into 2 consecutive 32-bit reads */
+                   struct etm_quad_split container;
+
+                   container.lsw = etm4x_read32(csa, offset);
+                   container.msw = etm4x_read32(csa, offset +

sizeof(u32));

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct · line 1101

Quoted 2 lines

drivers/hwtracing/coresight/coresight-etm4x.h +2 -0

+
+                   return *(u64 *) &container;

To be honest, I am not familiar with this part, just want to remind two things.

Quoted 8 lines

Firstly, here you could use hi_lo_readq_relaxed(), seems to me, hi_lo_readq_relaxed() does the same thing with above section and you don't need to define the structure etm_quad_split().

Secondly, IIUC, a main problem with splitting 64-bit access into two 32-bit accesses is breaking atomicity. If here have race condition between reading and writing 64-bit registers, we need to consider to use spinlock for register accessing.

That shouldn't be an issue. 32-bit memory-mapped interface is normal for ETMs. The peripheral bus interface to ETMs (debug APB) is maximum 32-bit. The ETM architecture defines that registers are only single-copy atomic up to 32-bit. This is generally true of CoreSight programming registers.

What typically happens to a 64-bit read/write from a CPU is that it is split into two 32-bit accesses by a downsizing bridge on the path from the main system interconnect to the debug APB (see 4.3.5 in the ETM architecture spec). In this case, it sounds like there is no downsizing bridge. But because there should be no existing code relying on 64-bit atomic access to ETM registers, it should be safe to split the accesses in software without worrying about atomicity.

Overall the ETM configuration involves complicated interactions between multiple registers, so if you've got one CPU reading out the configuration while another CPU is writing it, you've likely got much bigger problems than 64-bit atomicity.

Al

Quoted 53 lines

I'd like to leave the second issue to Suzuki/Mike/James for comfirmation in case I introduced complexity.

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1104

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+           } else
+                   return readq_relaxed(csa->base + offset);
+   } else
+           return read_etm4x_sysreg_offset(offset, true);

Here need to add brackets:

        } else {
                return read_etm4x_sysreg_offset(offset, true);
        }

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 13 lines

drivers/hwtracing/coresight/coresight-etm4x.h +13 -0

+}
+
+static inline void etm4x_relaxed_write64(struct csdev_access *csa,
+u64 quad, unsigned int offset) {
+   if (csa->io_mem) {
+           /* split 64-bit writes into 2 consecutive 32-bit writes */
+           if (csa->no_quad_mmio) {
+                   struct etm_quad_split container;
+
+                   *(u64 *) &container = quad;
+
+                   etm4x_relaxed_write32(csa, container.lsw, offset);
+                   etm4x_relaxed_write32(csa, container.msw, offset +

sizeof(u32));

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1121

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+           } else
+                   writeq_relaxed(quad, csa->base + offset);
+   } else
+           write_etm4x_sysreg_offset(quad, offset, true);          \

Ditto. Please drop the character '\' at the end of the line.

Thanks,
Leo

P.s. I have a ADLink AVA platform (Ampere Altra SoC with 32 CPUs), I am glad to give a test if you can confirm this patch set can apply on it (and please clarify if there have any prerequisite for firmwares).

Quoted 7 lines

drivers/hwtracing/coresight/coresight-etm4x.h +2 -1

+}
+
 static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)  {
    return drvdata->arch >= ETM_ARCH_ETE;
--

2.25.1

_______________________________________________
CoreSight mailing list -- [email protected] To unsubscribe send an email to [email protected]

Quoted 15 lines

Quoted 15 lines

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x.h +9 -0

+static inline u64 etm4x_relaxed_read64(struct csdev_access *csa,
+unsigned int offset) {
+   if (csa->io_mem) {
+           if (csa->no_quad_mmio) {
+                   /* split 64-bit reads into 2 consecutive 32-bit reads */
+                   struct etm_quad_split container;
+
+                   container.lsw = etm4x_read32(csa, offset);
+                   container.msw = etm4x_read32(csa, offset +

sizeof(u32));

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+                   return *(u64 *) &container;

To be honest, I am not familiar with this part, just want to remind two things.

Quoted 8 lines

Quoted 8 lines

Firstly, here you could use hi_lo_readq_relaxed(), seems to me, hi_lo_readq_relaxed() does the same thing with above section and you don't need to define the structure etm_quad_split().

Secondly, IIUC, a main problem with splitting 64-bit access into two 32-bit accesses is breaking atomicity. If here have race condition between reading and writing 64-bit registers, we need to consider to use spinlock for register accessing.

The drivers ensure that:-
a) the ETM is only accessed by the CPU it is associated with.
b) there are appropriate locks in place while the ETM is programmed up.

Moreover, the 64 bit registers are programmed only when the device is disabled - as required by the spec, so there cannot be a case of using "half" an updated value as all the required registers are programmed before the device is finally enabled to begin tracing

Regards

Mike

Quoted 66 lines

That shouldn't be an issue. 32-bit memory-mapped interface is normal for ETMs. The peripheral bus interface to ETMs (debug APB) is maximum 32-bit. The ETM architecture defines that registers are only single-copy atomic up to 32-bit. This is generally true of CoreSight programming registers.

What typically happens to a 64-bit read/write from a CPU is that it is split into two 32-bit accesses by a downsizing bridge on the path from the main system interconnect to the debug APB (see 4.3.5 in the ETM architecture spec). In this case, it sounds like there is no downsizing bridge. But because there should be no existing code relying on 64-bit atomic access to ETM registers, it should be safe to split the accesses in software without worrying about atomicity.

Overall the ETM configuration involves complicated interactions between multiple registers, so if you've got one CPU reading out the configuration while another CPU is writing it, you've likely got much bigger problems than 64-bit atomicity.

Al

Quoted 43 lines

I'd like to leave the second issue to Suzuki/Mike/James for comfirmation in case I introduced complexity.

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1104

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+           } else
+                   return readq_relaxed(csa->base + offset);
+   } else
+           return read_etm4x_sysreg_offset(offset, true);

Here need to add brackets:

        } else {
                return read_etm4x_sysreg_offset(offset, true);
        }

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 13 lines

drivers/hwtracing/coresight/coresight-etm4x.h +13 -0

+}
+
+static inline void etm4x_relaxed_write64(struct csdev_access *csa,
+u64 quad, unsigned int offset) {
+   if (csa->io_mem) {
+           /* split 64-bit writes into 2 consecutive 32-bit writes */
+           if (csa->no_quad_mmio) {
+                   struct etm_quad_split container;
+
+                   *(u64 *) &container = quad;
+
+                   etm4x_relaxed_write32(csa, container.lsw, offset);
+                   etm4x_relaxed_write32(csa, container.msw, offset +

sizeof(u32));

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1121

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+           } else
+                   writeq_relaxed(quad, csa->base + offset);
+   } else
+           write_etm4x_sysreg_offset(quad, offset, true);          \

Ditto. Please drop the character '\' at the end of the line.

Thanks,
Leo

P.s. I have a ADLink AVA platform (Ampere Altra SoC with 32 CPUs), I am glad to give a test if you can confirm this patch set can apply on it (and please clarify if there have any prerequisite for firmwares).

Quoted 10 lines

Quoted 10 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight-etm4x.h +2 -1

+}
+
 static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)  {
    return drvdata->arch >= ETM_ARCH_ETE;
--

2.25.1

_______________________________________________
CoreSight mailing list -- [email protected] To unsubscribe send an email to [email protected]

Signature · 3 lines

Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

On Wed, Mar 08, 2023 at 11:26:48AM +0000, Mike Leach wrote:

[...]

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x.h +9 -0

+static inline u64 etm4x_relaxed_read64(struct csdev_access *csa,
+unsigned int offset) {
+   if (csa->io_mem) {
+           if (csa->no_quad_mmio) {
+                   /* split 64-bit reads into 2 consecutive 32-bit reads */
+                   struct etm_quad_split container;
+
+                   container.lsw = etm4x_read32(csa, offset);
+                   container.msw = etm4x_read32(csa, offset +

sizeof(u32));

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+                   return *(u64 *) &container;

To be honest, I am not familiar with this part, just want to remind two things.

Firstly, here you could use hi_lo_readq_relaxed(), seems to me, hi_lo_readq_relaxed() does the same thing with above section and you don't need to define the structure etm_quad_split().

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

Secondly, IIUC, a main problem with splitting 64-bit access into two 32-bit accesses is breaking atomicity. If here have race condition between reading and writing 64-bit registers, we need to consider to use spinlock for register accessing.

Quoted 8 lines

The drivers ensure that:-
a) the ETM is only accessed by the CPU it is associated with.
b) there are appropriate locks in place while the ETM is programmed up.

Moreover, the 64 bit registers are programmed only when the device is disabled - as required by the spec, so there cannot be a case of using "half" an updated value as all the required registers are programmed before the device is finally enabled to begin tracing

Thanks a lot for confirmation, Al and Mike. So the second issue is not valid anymore and please ignore it.

Thanks,
Leo

Quoted 88 lines

Regards

Mike

Quoted 76 lines

That shouldn't be an issue. 32-bit memory-mapped interface is normal for ETMs. The peripheral bus interface to ETMs (debug APB) is maximum 32-bit. The ETM architecture defines that registers are only single-copy atomic up to 32-bit. This is generally true of CoreSight programming registers.

What typically happens to a 64-bit read/write from a CPU is that it is split into two 32-bit accesses by a downsizing bridge on the path from the main system interconnect to the debug APB (see 4.3.5 in the ETM architecture spec). In this case, it sounds like there is no downsizing bridge. But because there should be no existing code relying on 64-bit atomic access to ETM registers, it should be safe to split the accesses in software without worrying about atomicity.

Overall the ETM configuration involves complicated interactions between multiple registers, so if you've got one CPU reading out the configuration while another CPU is writing it, you've likely got much bigger problems than 64-bit atomicity.

Al

Quoted 53 lines

I'd like to leave the second issue to Suzuki/Mike/James for comfirmation in case I introduced complexity.

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1104

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+           } else
+                   return readq_relaxed(csa->base + offset);
+   } else
+           return read_etm4x_sysreg_offset(offset, true);

Here need to add brackets:

        } else {
                return read_etm4x_sysreg_offset(offset, true);
        }

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config);

Quoted 13 lines

drivers/hwtracing/coresight/coresight-etm4x.h +13 -0

+}
+
+static inline void etm4x_relaxed_write64(struct csdev_access *csa,
+u64 quad, unsigned int offset) {
+   if (csa->io_mem) {
+           /* split 64-bit writes into 2 consecutive 32-bit writes */
+           if (csa->no_quad_mmio) {
+                   struct etm_quad_split container;
+
+                   *(u64 *) &container = quad;
+
+                   etm4x_relaxed_write32(csa, container.lsw, offset);
+                   etm4x_relaxed_write32(csa, container.msw, offset +

sizeof(u32));

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1121

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x.h +4 -0

+           } else
+                   writeq_relaxed(quad, csa->base + offset);
+   } else
+           write_etm4x_sysreg_offset(quad, offset, true);          \

Ditto. Please drop the character '\' at the end of the line.

Thanks,
Leo

P.s. I have a ADLink AVA platform (Ampere Altra SoC with 32 CPUs), I am glad to give a test if you can confirm this patch set can apply on it (and please clarify if there have any prerequisite for firmwares).

Quoted 7 lines

drivers/hwtracing/coresight/coresight-etm4x.h +2 -1

+}
+
 static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)  {
    return drvdata->arch >= ETM_ARCH_ETE;
--

2.25.1

_______________________________________________
CoreSight mailing list -- [email protected] To unsubscribe send an email to [email protected]

Signature · 3 lines

Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

I'm familiar with this interface. The reason I chose not to use it is this solution is configured at compile time. The writeq_relaxed (used by etm4x_relaxed_write64) and readq_relaxed (used by etm4x_relaxed_read64) otherwise default to 64-bit access. I was uncertain how a compile time change would go over with the maintainers. Correct me, but compile time configurations in the kernel seem to be related to ARM64 erratum, versus manufacturer specific?

My take (based on limited knowledge) of the changes necessary to support a compile time decision seemed to exceed the changes compared with the implementation I submitted upstream. If this becomes a sticking point, let me know.

Hi Steve,

On Wed, Mar 08, 2023 at 11:08:34AM -0800, Steve Clevenger wrote:

[...]

Quoted 8 lines

I'm familiar with this interface. The reason I chose not to use it is this solution is configured at compile time. The writeq_relaxed (used by etm4x_relaxed_write64) and readq_relaxed (used by etm4x_relaxed_read64) otherwise default to 64-bit access. I was uncertain how a compile time change would go over with the maintainers. Correct me, but compile time configurations in the kernel seem to be related to ARM64 erratum, versus manufacturer specific?

Quoted 4 lines

My take (based on limited knowledge) of the changes necessary to support a compile time decision seemed to exceed the changes compared with the implementation I submitted upstream. If this becomes a sticking point, let me know.

I am suggesting something like below:

#include <linux/io-64-nonatomic-hi-lo.h>

static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)

{
	if (csa->io_mem) {
		if (csa->no_quad_mmio)
                        return hi_lo_readq_relaxed(csa->base + offset);
		else
			return readq_relaxed(csa->base + offset);
	} else {
		return read_etm4x_sysreg_offset(offset, true);
        }
}

I'm not saying to play magic on {writeq|readq}_relaxed(), would it be fine to directly call hi_lo_readq_relaxed()?

Thanks,
Leo

Quoted 14 lines

#include <linux/io-64-nonatomic-hi-lo.h>

static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)

{
	if (csa->io_mem) {
		if (csa->no_quad_mmio)
                        return hi_lo_readq_relaxed(csa->base + offset);
		else
			return readq_relaxed(csa->base + offset);
	} else {
		return read_etm4x_sysreg_offset(offset, true);
        }
}

Quoted 2 lines

I'm not saying to play magic on {writeq|readq}_relaxed(), would it be fine to directly call hi_lo_readq_relaxed()?

I can do this, and it would work fine. Without compile protection, one concern is the silent inclusion of io-64-nonatomic-hi-lo.h definitions for readq_relaxed/writeq_relaxed if the include hierarchy gets changed. The only (minor) risk I see is to performance.

Second, it seems to me the hi_lo and lo_hi macros were intended to deal with endianness. IMO, the implementation I offered is agnostic without a hint to endianness or atomicity.

The CoreSight maintainer audience is
limited and already aware of these issues, so I'll make the change for you.

On Thu, Mar 09, 2023 at 12:46:55PM -0800, Steve Clevenger wrote:

[...]

Quoted 14 lines

Quoted 14 lines

#include <linux/io-64-nonatomic-hi-lo.h>

static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)

{
	if (csa->io_mem) {
		if (csa->no_quad_mmio)
                        return hi_lo_readq_relaxed(csa->base + offset);
		else
			return readq_relaxed(csa->base + offset);
	} else {
		return read_etm4x_sysreg_offset(offset, true);
        }
}

Quoted 7 lines

Quoted 2 lines

I'm not saying to play magic on {writeq|readq}_relaxed(), would it be fine to directly call hi_lo_readq_relaxed()?

I can do this, and it would work fine. Without compile protection, one concern is the silent inclusion of io-64-nonatomic-hi-lo.h definitions for readq_relaxed/writeq_relaxed if the include hierarchy gets changed. The only (minor) risk I see is to performance.

Yeah, we could include io.h ahead io-64-nonatomic-hi-lo.h:

  #include <linux/io.h>
  #include <linux/io-64-nonatomic-lo-hi.h>

Thus we can get the definitions for {readq|writeq}_relaxed() from "io.h".

Quoted 4 lines

Second, it seems to me the hi_lo and lo_hi macros were intended to deal with endianness. IMO, the implementation I offered is agnostic without a hint to endianness or atomicity.

Seems to me, kernel functions with explict endianness naming is not bad thing :)

Quoted 2 lines

The CoreSight maintainer audience is
limited and already aware of these issues, so I'll make the change for you.

Thanks! Suzuki, Mike, I think it's good to get your opinion before Steve can proceed for updating patch.

Leo

Quoted 31 lines · Leo Yan

On Thu, Mar 09, 2023 at 12:46:55PM -0800, Steve Clevenger wrote:

[...]

Quoted 14 lines

Quoted 14 lines

#include <linux/io-64-nonatomic-hi-lo.h>

static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)

{
	if (csa->io_mem) {
		if (csa->no_quad_mmio)
                        return hi_lo_readq_relaxed(csa->base + offset);
		else
			return readq_relaxed(csa->base + offset);
	} else {
		return read_etm4x_sysreg_offset(offset, true);
        }
}

Quoted 7 lines

Quoted 2 lines

I'm not saying to play magic on {writeq|readq}_relaxed(), would it be fine to directly call hi_lo_readq_relaxed()?

I can do this, and it would work fine. Without compile protection, one concern is the silent inclusion of io-64-nonatomic-hi-lo.h definitions for readq_relaxed/writeq_relaxed if the include hierarchy gets changed. The only (minor) risk I see is to performance.

Yeah, we could include io.h ahead io-64-nonatomic-hi-lo.h:

  #include <linux/io.h>
  #include <linux/io-64-nonatomic-lo-hi.h>

Quoted 2 lines

Thus we can get the definitions for {readq|writeq}_relaxed() from "io.h".

It seems the actual read_relaxed/write_relaxed definitions come from the asm-generic/io.h version already in the include hierarchy without adding linux/io.h. A more foolproof protection is to check whether these are defined before the io-64-nonatomic-hi-lo.h include and error out if not.

Quoted 15 lines

Quoted 4 lines

Second, it seems to me the hi_lo and lo_hi macros were intended to deal with endianness. IMO, the implementation I offered is agnostic without a hint to endianness or atomicity.

Seems to me, kernel functions with explict endianness naming is not bad thing :)

Quoted 2 lines

The CoreSight maintainer audience is
limited and already aware of these issues, so I'll make the change for you.

Thanks! Suzuki, Mike, I think it's good to get your opinion before Steve can proceed for updating patch.

Leo

Ampere ETMv4.x support. Added Ampere ETM ID, and changes required by the Ampere ETMv4.x hardware implementation.

Steve Clevenger (3):
Add known list of Ampere ETMv4 errata
coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read coresight etm4x: Add 32-bit read/write option to split 64-bit words

  Documentation/arm64/silicon-errata.rst        |  6 +-
  .../coresight/coresight-etm4x-core.c          | 50 +++++++++++-----
  drivers/hwtracing/coresight/coresight-etm4x.h | 58 ++++++++++++++-----
  include/linux/coresight.h                     |  3 +
  4 files changed, 89 insertions(+), 28 deletions(-)

Hi Steve,

Quoted 3 lines · Steve Clevenger

Ampere ETMv4.x support. Added Ampere ETM ID, and changes required by the Ampere ETMv4.x hardware implementation.

I don't see any mention of the access via system instructions. Where did that end up ? What is the conclusion on that front ?

Kind regards
Suzuki

Quoted 11 lines

Steve Clevenger (3):
   Add known list of Ampere ETMv4 errata
   coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read coresight etm4x: Add 32-bit read/write option to split 64-bit words

  Documentation/arm64/silicon-errata.rst        |  6 +- .../coresight/coresight-etm4x-core.c          | 50 +++++++++++----- drivers/hwtracing/coresight/coresight-etm4x.h | 58 ++++++++++++++----- include/linux/coresight.h                     |  3 +

  4 files changed, 89 insertions(+), 28 deletions(-)

Hi Suzuki,

To answer your question, Ampere plans to use the existing MMIO implementation to introduce CoreSight HW Assisted Trace since we're preparing for release. As a minimum, we know this would require a respin

of our CoreSight DSDT. Also, if I didn't misunderstand, it sounded like you planned supporting work (e.g. ETMv4 not handled as an AMBA device). Since our ETMv4 sink components (ETF, ETR, +CATU) remain memory mapped, do these remain AMBA?

We understand ETMv4/ETE MMIO is going away. As a sysreg quick test, I

bypassed the code which checks for an ETMv4 base address in order to default to sysreg access. Trace collection failed with an error. I don't have the time to chase after this right now, but I intend to budget the time in the near future.

Thanks and regards,
Steve

Quoted 26 lines · Suzuki K Poulose

Hi Steve,

Quoted 3 lines · Steve Clevenger

Ampere ETMv4.x support. Added Ampere ETM ID, and changes required by the Ampere ETMv4.x hardware implementation.

I don't see any mention of the access via system instructions. Where did that end up ? What is the conclusion on that front ?

Kind regards
Suzuki

Quoted 11 lines

Steve Clevenger (3):
   Add known list of Ampere ETMv4 errata
   coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read coresight etm4x: Add 32-bit read/write option to split 64-bit words

   Documentation/arm64/silicon-errata.rst        |  6 +-
   .../coresight/coresight-etm4x-core.c          | 50 +++++++++++-----
   drivers/hwtracing/coresight/coresight-etm4x.h | 58 ++++++++++++++-----
   include/linux/coresight.h                     |  3 +
   4 files changed, 89 insertions(+), 28 deletions(-)

Hi Steve

Quoted 3 lines · Steve Clevenger

Hi Suzuki,

Quoted 3 lines

To answer your question, Ampere plans to use the existing MMIO implementation to introduce CoreSight HW Assisted Trace since we're preparing for release. As a minimum, we know this would require a respin

Please note that MMIO interface is for "external debug" not for self-hosted usecase, with system instruction support. This is why you need to work around the driver "as an errata".

Quoted 5 lines

of our CoreSight DSDT. Also, if I didn't misunderstand, it sounded like you planned supporting work (e.g. ETMv4 not handled as an AMBA device). Since our ETMv4 sink components (ETF, ETR, +CATU) remain memory mapped, do these remain AMBA?

Just to be clear, all of these components will be MMIO. We are simply moving the "Linux" driver framework from AMBA driver to platform device. This will be done in stages. ETMv4x would be moved in the first pass to allow us to support system instruction based devices seemlessly with ACPI.

Quoted 1 line

Quoted 1 line

We understand ETMv4/ETE MMIO is going away. As a sysreg quick test, I

As mentioned above it is not going away. MMIO is the way to access if that is *the only* access mode. For ETMv4.4+ and ETE MMIO system instructions is *the mode* of access for self-hosted. Please be aware that, going down this route may prevent them being virtualised (when we add the support in the near future).

Quoted 5 lines

bypassed the code which checks for an ETMv4 base address in order to default to sysreg access. Trace collection failed with an error. I don't have the time to chase after this right now, but I intend to budget the time in the near future.

If we can get to the bottom of this, we should be able to support the platform in a future proof way than adding this ugly work around of accessing via the *external MMIO* interface.

Suzuki

Quoted 32 lines

Thanks and regards,
Steve

Quoted 27 lines · Suzuki K Poulose

Hi Steve,

Quoted 3 lines · Steve Clevenger

Ampere ETMv4.x support. Added Ampere ETM ID, and changes required by the Ampere ETMv4.x hardware implementation.

I don't see any mention of the access via system instructions. Where did that end up ? What is the conclusion on that front ?

Kind regards
Suzuki

Quoted 12 lines

Steve Clevenger (3):
    Add known list of Ampere ETMv4 errata
    coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read coresight etm4x: Add 32-bit read/write option to split 64-bit words

   Documentation/arm64/silicon-errata.rst        |  6 +- .../coresight/coresight-etm4x-core.c          | 50 +++++++++++----- drivers/hwtracing/coresight/coresight-etm4x.h | 58
++++++++++++++-----
   include/linux/coresight.h                     |  3 +

   4 files changed, 89 insertions(+), 28 deletions(-)

Ampere ETMv4.x support. Added Ampere ETM ID, and changes required by the Ampere ETMv4.x hardware implementation.

Steve Clevenger (3):
Add known list of Ampere ETMv4 errata
coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read coresight etm4x: Add 32-bit read/write option to split 64-bit words

  Documentation/arm64/silicon-errata.rst        |  6 +-
  .../coresight/coresight-etm4x-core.c          | 50 +++++++++++-----
  drivers/hwtracing/coresight/coresight-etm4x.h | 58 ++++++++++++++-----
  include/linux/coresight.h                     |  3 +
  4 files changed, 89 insertions(+), 28 deletions(-)

Quoted 3 lines · Steve Clevenger

Ampere ETMv4.x support. Added Ampere ETM ID, and changes required by the Ampere ETMv4.x hardware implementation.

nit: Subject should be :

[PATCH <version> 0/X] <subsystem>: <component>: <Title>

in this case case:

<version> = NULL if version == 1
          = version, otherwise
Helps us to keep track of different versions of the postings.

<subsystem> == coresight

This helps the reviewers to "notice" or "ignore" a given series depending on their interest.

<component> == etm4x
This further helps the reviewers to filter the mails within a subsystem.

[PATCH v2 0/3] coresight: etm4x: Support for Ampere computing

Also, please include a changelog from the previous version to indicate what has changed. e.g,

"Changes since v1:
  - Modified xyz
  - Dropped abc
  - Addressed comments on ijk (<name of the requester> "

That helps the reviewers to get a picture of what they should be looking at and better spend their time.

Kind regards
Suzuki

Quoted 11 lines

Steve Clevenger (3):
   Add known list of Ampere ETMv4 errata
   coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read coresight etm4x: Add 32-bit read/write option to split 64-bit words

  Documentation/arm64/silicon-errata.rst        |  6 +- .../coresight/coresight-etm4x-core.c          | 50 +++++++++++----- drivers/hwtracing/coresight/coresight-etm4x.h | 58 ++++++++++++++----- include/linux/coresight.h                     |  3 +

  4 files changed, 89 insertions(+), 28 deletions(-)