diff mbox series

[2/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read

Message ID 143540e5623d4c7393d24833f2b80600d8d745d2.1677881753.git.scclevenger@os.amperecomputing.com (mailing list archive)
State New, archived
Headers show
Series Ampere Computing ETMv4.x Support | expand

Commit Message

Steve Clevenger March 6, 2023, 5:54 a.m. UTC
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 <scclevenger@os.amperecomputing.com>
---
 .../coresight/coresight-etm4x-core.c          | 50 ++++++++++++++-----
 include/linux/coresight.h                     |  3 ++
 2 files changed, 40 insertions(+), 13 deletions(-)

Comments

Suzuki K Poulose March 17, 2023, 11:58 a.m. UTC | #1
Hi Steve

On 06/03/2023 05:54, Steve Clevenger wrote:
> 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 <scclevenger@os.amperecomputing.com>

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/20230317115728.1358368-1-suzuki.poulose@arm.com

Suzuki
Steve Clevenger March 17, 2023, 8:53 p.m. UTC | #2
Hi Suzuki,

I don't need to test this explicitly provided the TRCIDR1 access is
removed in etm4_init_iomem_access. When I first encountered this, I
substituted a hard coded value for the TRCIDR1 read. This hack
eliminated the initialization problem. I later submitted a patch to
clear TRCOSLAR.OSLK before the access. I didn't have visibility to other
implementations which could rely on the fallback.

Thanks,
Steve

On 3/17/2023 4:58 AM, Suzuki K Poulose wrote:
> Hi Steve
> 
> On 06/03/2023 05:54, Steve Clevenger wrote:
>> 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 <scclevenger@os.amperecomputing.com>
> 
> 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/20230317115728.1358368-1-suzuki.poulose@arm.com
> 
> Suzuki
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 104333c2c8a3..6f7a14c4c78c 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1047,6 +1047,7 @@  static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
 	case ETM_DEVARCH_ETMv4x_ARCH:
 		*csa = (struct csdev_access) {
 			.io_mem	= false,
+			.no_quad_mmio = false,
 			.read	= etm4x_sysreg_read,
 			.write	= etm4x_sysreg_write,
 		};
@@ -1054,6 +1055,7 @@  static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
 	case ETM_DEVARCH_ETE_ARCH:
 		*csa = (struct csdev_access) {
 			.io_mem	= false,
+			.no_quad_mmio = false,
 			.read	= ete_sysreg_read,
 			.write	= ete_sysreg_write,
 		};
@@ -1069,8 +1071,20 @@  static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
 static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
 				   struct csdev_access *csa)
 {
-	u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
-	u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
+	u32 devarch;
+	u32 idr1;
+
+	/* Detect the support for OS Lock before we actually use it */
+	etm_detect_os_lock(drvdata, csa);
+
+	/*
+	 * An ETM implementation may consider memory mapped I/O
+	 * an external debug access so clear TRCOSLAR.OSLK early.
+	 */
+	etm4_os_unlock_csa(drvdata, csa);
+
+	devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
+	idr1 = readl_relaxed(drvdata->base + TRCIDR1);
 
 	/*
 	 * All ETMs must implement TRCDEVARCH to indicate that
@@ -1089,7 +1103,10 @@  static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
 		drvdata->arch = etm_trcidr_to_arch(idr1);
 	}
 
-	*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
+	/* preserve pre-initialized fields */
+	csa->io_mem = true;
+	csa->base = drvdata->base;
+
 	return true;
 }
 
@@ -1151,18 +1168,13 @@  static void etm4_init_arch_data(void *info)
 	csa = init_arg->csa;
 
 	/*
-	 * If we are unable to detect the access mechanism,
-	 * or unable to detect the trace unit type, fail
-	 * early.
+	 * Make sure all registers are accessible. If we are unable
+	 * to detect the access mechanism, or unable to detect the
+	 * trace unit type, fail early.
 	 */
-	if (!etm4_init_csdev_access(drvdata, csa))
-		return;
-
-	/* Detect the support for OS Lock before we actually use it */
-	etm_detect_os_lock(drvdata, csa);
+	if (!etm4_init_csdev_access(drvdata, csa)) 
+               return;
 
-	/* Make sure all registers are accessible */
-	etm4_os_unlock_csa(drvdata, csa);
 	etm4_cs_unlock(drvdata, csa);
 
 	etm4_check_arch_features(drvdata, init_arg->pid);
@@ -2084,6 +2096,17 @@  static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
 	init_arg.csa = &access;
 	init_arg.pid = etm_pid;
 
+        /*
+         * Ampere ETM v4.6 does not suport 64-bit MMIO access.
+         * This mask isolates the manufacturer JEP106 ID in the PID.
+         * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8).
+         * Ampere does not currently use the implementation defined
+         * TRCIDR0 PART_0 and/or TRCIDR1 PART_1 fields to further
+         * describe the ETMv4 parts.
+	 */
+	if ((init_arg.pid & 0x000FF000) == 0x00096000)
+		init_arg.csa->no_quad_mmio = true;
+
 	/*
 	 * Serialize against CPUHP callbacks to avoid race condition
 	 * between the smp call and saving the delayed probe.
@@ -2249,6 +2272,7 @@  static const struct amba_id etm4_ids[] = {
 	CS_AMBA_ID(0x000bb95e),			/* Cortex-A57 */
 	CS_AMBA_ID(0x000bb95a),			/* Cortex-A72 */
 	CS_AMBA_ID(0x000bb959),			/* Cortex-A73 */
+	CS_AMBA_UCI_ID(0x00096000, uci_id_etm4),/* Ampere ARMv8 */
 	CS_AMBA_UCI_ID(0x000bb9da, uci_id_etm4),/* Cortex-A35 */
 	CS_AMBA_UCI_ID(0x000bbd05, uci_id_etm4),/* Cortex-A55 */
 	CS_AMBA_UCI_ID(0x000bbd0a, uci_id_etm4),/* Cortex-A75 */
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f19a47b9bb5a..285d7aad6ef4 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -116,12 +116,14 @@  struct coresight_platform_data {
  * struct csdev_access - Abstraction of a CoreSight device access.
  *
  * @io_mem	: True if the device has memory mapped I/O
+ * @no_quad_mmio: True if 64-bit register access is not suported
  * @base	: When io_mem == true, base address of the component
  * @read	: Read from the given "offset" of the given instance.
  * @write	: Write "val" to the given "offset".
  */
 struct csdev_access {
 	bool io_mem;
+	bool no_quad_mmio;
 	union {
 		void __iomem *base;
 		struct {
@@ -135,6 +137,7 @@  struct csdev_access {
 #define CSDEV_ACCESS_IOMEM(_addr)		\
 	((struct csdev_access)	{		\
 		.io_mem		= true,		\
+		.no_quad_mmio	= false,	\
 		.base		= (_addr),	\
 	})