Ampere Computing ETMv4.x Support

PATCH In review Steve Clevenger 5 participants 31 messages Jan 20, 2023 – Mar 7, 2023

Subsystems (4)

ACPI Arm/CoreSight framework and drivers Hardware tracing facilities Arm primecell bus support

lore

Share & tools

lore

Steve Clevenger

author

2023-01-20 00:53 UTC #0 lore raw

cover Ampere Computing ETMv4.x Support

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

Steve Clevenger (3):
coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read coresight etm4x: Add 32-bit read/write option to split 64-bit words coresight etm4x: Add pr_debug statement for Coresight component
PID/CID

 drivers/amba/bus.c                            |   7 ++
 .../coresight/coresight-etm4x-core.c          | 115 ++++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |  34 ++++++
 3 files changed, 131 insertions(+), 25 deletions(-)

Signature · 7 lines

2.25.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-20 00:53 UTC #1 lore raw

PATCH 1/3 coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read

Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Add Ampere ETM PID required for Coresight ETM driver support.

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

---
 .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
 2 files changed, 32 insertions(+), 6 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x-core.c +30 -6

index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
 	drvdata = dev_get_drvdata(init_arg->dev);
 	csa = init_arg->csa;
 
+	/* Detect the support for OS Lock before we actually use it */
+	etm_detect_os_lock(drvdata, csa);
+
+	/*
+	 * For ETM implementations that consider MMIO an external access
+	 * clear TRCOSLAR.OSLK early.
+	 */
+	if (drvdata->mmio_external)
+		etm4_os_unlock_csa(drvdata, csa);
+
 	/*
 	 * If we are unable to detect the access mechanism,
 	 * or unable to detect the trace unit type, fail
-	 * early.
+	 * early. Reset TRCOSLAR.OSLK if cleared.
 	 */
-	if (!etm4_init_csdev_access(drvdata, csa))
+	if (!etm4_init_csdev_access(drvdata, csa)) {
+		if (drvdata->mmio_external)
+			etm4_os_lock(drvdata);
 		return;
+	}
 
-	/* Detect the support for OS Lock before we actually use it */
-	etm_detect_os_lock(drvdata, csa);
+	/*
+	 * Make sure all registers are accessible
+	 * TRCOSLAR.OSLK may already be clear
+	 */
+	if (!drvdata->mmio_external)
+		etm4_os_unlock_csa(drvdata, csa);
 
-	/* 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);

@@ -2027,6 +2042,14 @@ 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 considers MMIO access as external. This mask
+	 * isolates the manufacturer JEP106 ID in the PID.
+	 * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+	 */
+	if ((init_arg.pid & 0x000FF000) == 0x00096000)
+		drvdata->mmio_external = true;
+
 	/*
 	 * Serialize against CPUHP callbacks to avoid race condition
 	 * between the smp call and saving the delayed probe.

@@ -2192,6 +2215,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 */

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

index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
  * @skip_power_up: Indicates if an implementation can skip powering up
  *		   the trace unit.
  * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
  */
 struct etmv4_drvdata {
 	void __iomem			*base;

@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
 	bool				state_needs_restore;
 	bool				skip_power_up;
 	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
+	bool				mmio_external;
 };
 
 /* Address comparator access types */

Signature · 7 lines

2.25.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-20 11:13 UTC #2 lore raw

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

Hi Steve

Thanks for the patches. Have a few comments below.

Quoted 9 lines · Steve Clevenger

Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Please could you add this erratum to the :

Documentation/arm64/silicon-errata.rst ?

Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.

Quoted 26 lines

Add Ampere ETM PID required for Coresight ETM driver support.

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

---
  .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
  drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
  2 files changed, 32 insertions(+), 6 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x-core.c +9 -0

index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
  	drvdata = dev_get_drvdata(init_arg->dev);
  	csa = init_arg->csa;
 
+	/* Detect the support for OS Lock before we actually use it */
+	etm_detect_os_lock(drvdata, csa);
+ > +	/*
+	 * For ETM implementations that consider MMIO an external access
+	 * clear TRCOSLAR.OSLK early.
+	 */
+	if (drvdata->mmio_external)
+		etm4_os_unlock_csa(drvdata, csa);
+

drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1110

Quoted 10 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +4 -2

  	/*
  	 * If we are unable to detect the access mechanism,
  	 * or unable to detect the trace unit type, fail
-	 * early.
+	 * early. Reset TRCOSLAR.OSLK if cleared.
  	 */
-	if (!etm4_init_csdev_access(drvdata, csa))
+	if (!etm4_init_csdev_access(drvdata, csa)) {
+		if (drvdata->mmio_external)
+			etm4_os_lock(drvdata);

Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ?

Or thinking more about it, we could actually move the unlock step early for all ETMs irrespective of whether they are affected by this erratum. Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that.

Quoted 18 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +7 -4

  		return;
+	}
 
-	/* Detect the support for OS Lock before we actually use it */
-	etm_detect_os_lock(drvdata, csa);
+	/*
+	 * Make sure all registers are accessible
+	 * TRCOSLAR.OSLK may already be clear
+	 */
+	if (!drvdata->mmio_external)
+		etm4_os_unlock_csa(drvdata, csa);
 
-	/* 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);

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) · line 2049

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +5 -0

@@ -2027,6 +2042,14 @@ 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 considers MMIO access as external. This mask
+	 * isolates the manufacturer JEP106 ID in the PID.
+	 * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+	 */

Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) · line 2051

Quoted 2 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +2 -0
+	if ((init_arg.pid & 0x000FF000) == 0x00096000)
+		drvdata->mmio_external = true;

Like I said, we may be able to get rid of this flag and do the step for all ETMs. But before all of that, I would like to see if this is problem because we are skipping the system instruction route.

Suzuki

Quoted 24 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +1 -0

  	/*
  	 * Serialize against CPUHP callbacks to avoid race condition
  	 * between the smp call and saving the delayed probe.

@@ -2192,6 +2215,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 */

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

index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
   * @skip_power_up: Indicates if an implementation can skip powering up
   *		   the trace unit.
   * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
   */
  struct etmv4_drvdata {
  	void __iomem			*base;

drivers/hwtracing/coresight/coresight-etm4x.h struct etmv4_drvdata { · line 1074

Quoted 7 lines

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

@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
  	bool				state_needs_restore;
  	bool				skip_power_up;
  	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
+	bool				mmio_external;
  };
 
  /* Address comparator access types */

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-21 07:32 UTC #3 lore raw

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

Hi Suzuki,

Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for non-Ampere parts.

Steve

Quoted 15 lines · Suzuki K Poulose

Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 9 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Quoted 8 lines

Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.

That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've read in the driver code, the MMIO read access to TRCIDR1 occurs after a TRCDEVARCH access. The comments suggest this was to accommodate potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless the TRCOSLAR.OSLK lock is cleared first.

Quoted 41 lines

Quoted 29 lines
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 36 +++++++++++++++---- drivers/hwtracing/coresight/coresight-etm4x.h | 2 ++
  2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
       drvdata = dev_get_drvdata(init_arg->dev);
       csa = init_arg->csa;
+    /* Detect the support for OS Lock before we actually use it */ +    etm_detect_os_lock(drvdata, csa);
+ > +    /*
+     * For ETM implementations that consider MMIO an external access +     * clear TRCOSLAR.OSLK early.
+     */
+    if (drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
+
      /*
       * If we are unable to detect the access mechanism,
Quoted 8 lines
       * or unable to detect the trace unit type, fail -     * early.
+     * early. Reset TRCOSLAR.OSLK if cleared. */
-    if (!etm4_init_csdev_access(drvdata, csa)) +    if (!etm4_init_csdev_access(drvdata, csa)) { +        if (drvdata->mmio_external)
+            etm4_os_lock(drvdata);
Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ?

Quoted 4 lines

Or thinking more about it, we could actually move the unlock step early for all ETMs irrespective of whether they are affected by this erratum. Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that.

I agree the lock could be cleared earlier in the code. That's what this patch does for Ampere. If it's decided ok to do for other (or all) manufacturers, then the Ampere specific ID check goes away in this place. The Ampere ID check (and flag) to determine whether the [Patch 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or use an existing feature mask as suggested by Mike Leach in a later review.

Quoted 19 lines

Quoted 19 lines
          return;
+    }
-    /* Detect the support for OS Lock before we actually use it */ -    etm_detect_os_lock(drvdata, csa);
+    /*
+     * Make sure all registers are accessible
+     * TRCOSLAR.OSLK may already be clear
+     */
+    if (!drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
-    /* 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);
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
       init_arg.csa = &access;
       init_arg.pid = etm_pid;

Quoted 8 lines

Quoted 5 lines
+    /*
+     * Ampere ETM v4.6 considers MMIO access as external. This mask +     * isolates the manufacturer JEP106 ID in the PID.
+     * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8) +     */
Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.

This is the first Ampere ETMv4.x implementation. I wrote the ID check like this specifically because Ampere does not intend to address this for ETM designs in progress.

Quoted 7 lines

Quoted 3 lines
+ if ((init_arg.pid & 0x000FF000) == 0x00096000) + drvdata->mmio_external = true;
Like I said, we may be able to get rid of this flag and do the step for all ETMs. But before all of that, I would like to see if this is problem because we are skipping the system instruction route.

We understand MMIO access is deprecated going forward. There is other Linux code to be concerned about. For example, AMBA code reads the component PID/CID. This discovery code uses mapped values digested from the CoreSight ACPI which are the descriptions and graphs for the manufacturer trace implementation. There may be other Linux code I'm not aware. Note the ASL examples in ARM Document number: DEN0067 specify MMIO locations for every CoreSight component.

Quoted 25 lines

Suzuki
Quoted 23 lines
      /*
       * Serialize against CPUHP callbacks to avoid race condition
       * between the smp call and saving the delayed probe.
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -2192,6 +2215,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 */
drivers/hwtracing/coresight/coresight-etm4x.h +1 -0
index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
    * @skip_power_up: Indicates if an implementation can skip powering up
    *           the trace unit.
    * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
    */
   struct etmv4_drvdata {

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x.h struct etmv4_drvdata { · line 1073

Quoted 8 lines

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

       void __iomem            *base;

@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
       bool                state_needs_restore;
       bool                skip_power_up;
       DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
+    bool                mmio_external;
   };
     /* Address comparator access types */

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-23 10:55 UTC #4 lore raw

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

Quoted 29 lines · Steve Clevenger

Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for non-Ampere parts.
Steve
Quoted 19 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 9 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?

Quoted 7 lines

Quoted 4 lines
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've

That doesn't answe the question. Please could you confirm the value of ID_AA64DFR0_EL1 on your system ?

Or, are you able to try this on your ACPI based system and see if you are able to use the etm ? (UNTESTED hack !)

drivers/acpi/acpi_amba.c +0 -1

index f5b443ab01c2..099966cbac5a 100644
@@ -22,7 +22,6 @@
 static const struct acpi_device_id amba_id_list[] = {
 	{"ARMH0061", 0}, /* PL061 GPIO Device */
 	{"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */
-	{"ARMHC500", 0}, /* ARM CoreSight ETM4x */
 	{"ARMHC501", 0}, /* ARM CoreSight ETR */
 	{"ARMHC502", 0}, /* ARM CoreSight STM */
 	{"ARMHC503", 0}, /* ARM CoreSight Debug */

drivers/hwtracing/coresight/coresight-etm4x-core.c +11 -0

index 1ea8f173cca0..66670533fd54 100644
@@ -3,6 +3,7 @@
  * Copyright (c) 2014, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/acpi.h>
 #include <linux/bitops.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>

@@ -2286,12 +2287,22 @@ static const struct of_device_id etm4_sysreg_match[] = {
 	{}
 };
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id etm4x_acpi_ids[] = {
+	{"ARMHC500", 0}, /* ARM CoreSight ETM4x */
+	{}
+};
+
+MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
+#endif
+
 static struct platform_driver etm4_platform_driver = {
 	.probe		= etm4_probe_platform_dev,
 	.remove		= etm4_remove_platform_dev,
 	.driver			= {
 		.name			= "coresight-etm4x",
 		.of_match_table		= etm4_sysreg_match,
+		.acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
 		.suppress_bind_attrs	= true,
 	},
 };

Quoted 88 lines

read in the driver code, the MMIO read access to TRCIDR1 occurs after a TRCDEVARCH access. The comments suggest this was to accommodate potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless the TRCOSLAR.OSLK lock is cleared first.
Quoted 41 lines
Quoted 27 lines
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
   .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
   drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
   2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +8 -0
index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
       drvdata = dev_get_drvdata(init_arg->dev);
       csa = init_arg->csa;
   +    /* Detect the support for OS Lock before we actually use it */
+    etm_detect_os_lock(drvdata, csa);
+ > +    /*
+     * For ETM implementations that consider MMIO an external access
+     * clear TRCOSLAR.OSLK early.
+     */
+    if (drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
+
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1109
Quoted 10 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +4 -2
       /*
        * If we are unable to detect the access mechanism,
        * or unable to detect the trace unit type, fail
-     * early.
+     * early. Reset TRCOSLAR.OSLK if cleared.
        */
-    if (!etm4_init_csdev_access(drvdata, csa))
+    if (!etm4_init_csdev_access(drvdata, csa)) {
+        if (drvdata->mmio_external)
+            etm4_os_lock(drvdata);
Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ?
Quoted 4 lines
Or thinking more about it, we could actually move the unlock step early for all ETMs irrespective of whether they are affected by this erratum. Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that.
I agree the lock could be cleared earlier in the code. That's what this patch does for Ampere. If it's decided ok to do for other (or all) manufacturers, then the Ampere specific ID check goes away in this place. The Ampere ID check (and flag) to determine whether the [Patch 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or use an existing feature mask as suggested by Mike Leach in a later review.
Quoted 19 lines
Quoted 19 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +7 -2
           return;
+    }
   -    /* Detect the support for OS Lock before we actually use it */
-    etm_detect_os_lock(drvdata, csa);
+    /*
+     * Make sure all registers are accessible
+     * TRCOSLAR.OSLK may already be clear
+     */
+    if (!drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
   -    /* 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);
@@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
       init_arg.csa = &access;
       init_arg.pid = etm_pid;
Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void · line 2049
Quoted 5 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +4 -0
   +    /*
+     * Ampere ETM v4.6 considers MMIO access as external. This mask
+     * isolates the manufacturer JEP106 ID in the PID.
+     * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+     */
Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.

Quoted 3 lines

This is the first Ampere ETMv4.x implementation. I wrote the ID check like this specifically because Ampere does not intend to address this for ETM designs in progress.

I would recommend to make this mask stricter and apply this to the current implementation. When there are more, we could add this here, rather than having to leave this work around for all the possible cores.

Quoted 9 lines

Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void · line 2051
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +2 -0
+    if ((init_arg.pid & 0x000FF000) == 0x00096000)
+        drvdata->mmio_external = true;
Like I said, we may be able to get rid of this flag and do the step for all ETMs. But before all of that, I would like to see if this is problem because we are skipping the system instruction route.

Quoted 4 lines

We understand MMIO access is deprecated going forward. There is other Linux code to be concerned about. For example, AMBA code reads the component PID/CID. This discovery code uses mapped values digested from the CoreSight ACPI which are the descriptions and graphs for the

With the "proposed" ACPI support for system register, AMBA would not be involved at all.

Quoted 3 lines

manufacturer trace implementation. There may be other Linux code I'm not aware. Note the ASL examples in ARM Document number: DEN0067 specify MMIO locations for every CoreSight component.

Yes, but this was never updated to cover the system register based implementations. I will chase this up.

Suzuki

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-23 17:24 UTC #5 lore raw

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

Quoted 35 lines · Suzuki K Poulose

Quoted 33 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for non-Ampere parts.
Steve
Quoted 23 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 9 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.

Quoted 5 lines

Quoted 2 lines
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
That doesn't answe the question. Please could you confirm the value of ID_AA64DFR0_EL1 on your system ?

This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register interface must be implemented on all ETMv4.6 parts.

Quoted 177 lines

Or, are you able to try this on your ACPI based system and see if you are able to use the etm ? (UNTESTED hack !)
drivers/acpi/acpi_amba.c +0 -0
index f5b443ab01c2..099966cbac5a 100644
@@ -22,7 +22,6 @@
  static const struct acpi_device_id amba_id_list[] = {
{"ARMH0061", 0}, /* PL061 GPIO Device */ {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ {"ARMHC501", 0}, /* ARM CoreSight ETR */ {"ARMHC502", 0}, /* ARM CoreSight STM */ {"ARMHC503", 0}, /* ARM CoreSight Debug */
drivers/hwtracing/coresight/coresight-etm4x-core.c +11 -0
index 1ea8f173cca0..66670533fd54 100644
@@ -3,6 +3,7 @@
   * Copyright (c) 2014, The Linux Foundation. All rights reserved.
   */
 
+#include <linux/acpi.h>
  #include <linux/bitops.h>
  #include <linux/kernel.h>
  #include <linux/moduleparam.h>
@@ -2286,12 +2287,22 @@ static const struct of_device_id
 etm4_sysreg_match[] = {
      {}
  };
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id etm4x_acpi_ids[] = {
+    {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
+    {}
+};
+
+MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
+#endif
+
  static struct platform_driver etm4_platform_driver = {
      .probe        = etm4_probe_platform_dev,
      .remove        = etm4_remove_platform_dev,
      .driver            = {
          .name            = "coresight-etm4x",
          .of_match_table        = etm4_sysreg_match,
+        .acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
          .suppress_bind_attrs    = true,
      },
};
Quoted 90 lines
read in the driver code, the MMIO read access to TRCIDR1 occurs after a TRCDEVARCH access. The comments suggest this was to accommodate potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless the TRCOSLAR.OSLK lock is cleared first.
Quoted 42 lines
Quoted 30 lines
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 36
+++++++++++++++----
drivers/hwtracing/coresight/coresight-etm4x.h | 2 ++
   2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
        drvdata = dev_get_drvdata(init_arg->dev);
        csa = init_arg->csa;
   +    /* Detect the support for OS Lock before we actually use it */ +    etm_detect_os_lock(drvdata, csa);
+ > +    /*
+     * For ETM implementations that consider MMIO an external access +     * clear TRCOSLAR.OSLK early.
+     */
+    if (drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
+
       /*
        * If we are unable to detect the access mechanism,
Quoted 8 lines
        * or unable to detect the trace unit type, fail -     * early.
+     * early. Reset TRCOSLAR.OSLK if cleared. */
-    if (!etm4_init_csdev_access(drvdata, csa)) +    if (!etm4_init_csdev_access(drvdata, csa)) { +        if (drvdata->mmio_external)
+            etm4_os_lock(drvdata);
Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ?
Quoted 4 lines
Or thinking more about it, we could actually move the unlock step early for all ETMs irrespective of whether they are affected by this erratum. Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that.
I agree the lock could be cleared earlier in the code. That's what this patch does for Ampere. If it's decided ok to do for other (or all) manufacturers, then the Ampere specific ID check goes away in this place. The Ampere ID check (and flag) to determine whether the [Patch 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or use an existing feature mask as suggested by Mike Leach in a later review.
Quoted 19 lines
Quoted 19 lines
           return;
+    }
   -    /* Detect the support for OS Lock before we actually use it */ -    etm_detect_os_lock(drvdata, csa);
+    /*
+     * Make sure all registers are accessible
+     * TRCOSLAR.OSLK may already be clear
+     */
+    if (!drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
   -    /* 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);
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
        init_arg.csa = &access;
        init_arg.pid = etm_pid;
Quoted 8 lines
Quoted 5 lines
   +    /*
+     * Ampere ETM v4.6 considers MMIO access as external. This mask +     * isolates the manufacturer JEP106 ID in the PID.
+     * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8) +     */
Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.
Quoted 3 lines
This is the first Ampere ETMv4.x implementation. I wrote the ID check like this specifically because Ampere does not intend to address this for ETM designs in progress.
I would recommend to make this mask stricter and apply this to the current implementation. When there are more, we could add this here, rather than having to leave this work around for all the possible cores.
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
+ if ((init_arg.pid & 0x000FF000) == 0x00096000) + drvdata->mmio_external = true;
Like I said, we may be able to get rid of this flag and do the step for all ETMs. But before all of that, I would like to see if this is problem because we are skipping the system instruction route.
Quoted 4 lines
We understand MMIO access is deprecated going forward. There is other Linux code to be concerned about. For example, AMBA code reads the component PID/CID. This discovery code uses mapped values digested from the CoreSight ACPI which are the descriptions and graphs for the
With the "proposed" ACPI support for system register, AMBA would not be involved at all.
Quoted 3 lines
manufacturer trace implementation. There may be other Linux code I'm not aware. Note the ASL examples in ARM Document number: DEN0067 specify MMIO locations for every CoreSight component.

Quoted 6 lines

Yes, but this was never updated to cover the system register based implementations. I will chase this up.
Suzuki

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-23 17:34 UTC #6 lore raw

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

Quoted 41 lines · Steve Clevenger

Quoted 38 lines · Suzuki K Poulose
Quoted 36 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for non-Ampere parts.
Steve
Quoted 15 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 9 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Quoted 8 lines
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've

Quoted 6 lines

Quoted 2 lines
That doesn't answe the question. Please could you confirm the value of ID_AA64DFR0_EL1 on your system ?
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register interface must be implemented on all ETMv4.6 parts.

So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs
with sysreg access and ETEs.

Why not stick to the system register access* ?

* PS: The ACPI support for the ETM/ETE needs additional changes to the
CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on this at the moment and will be available soon.

The hack patch below should be sufficient to give it a try and if it works.

Kind regards
Suzuki

Quoted 178 lines

Quoted 178 lines
Or, are you able to try this on your ACPI based system and see if you are able to use the etm ? (UNTESTED hack !)
drivers/acpi/acpi_amba.c +0 -1
index f5b443ab01c2..099966cbac5a 100644
@@ -22,7 +22,6 @@
  static const struct acpi_device_id amba_id_list[] = {
      {"ARMH0061", 0}, /* PL061 GPIO Device */
      {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */
-    {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
      {"ARMHC501", 0}, /* ARM CoreSight ETR */
      {"ARMHC502", 0}, /* ARM CoreSight STM */
      {"ARMHC503", 0}, /* ARM CoreSight Debug */
drivers/hwtracing/coresight/coresight-etm4x-core.c +11 -0
index 1ea8f173cca0..66670533fd54 100644
@@ -3,6 +3,7 @@
   * Copyright (c) 2014, The Linux Foundation. All rights reserved.
   */
 
+#include <linux/acpi.h>
  #include <linux/bitops.h>
  #include <linux/kernel.h>
  #include <linux/moduleparam.h>
@@ -2286,12 +2287,22 @@ static const struct of_device_id
 etm4_sysreg_match[] = {
      {}
  };
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id etm4x_acpi_ids[] = {
+    {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
+    {}
+};
+
+MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
+#endif
+
  static struct platform_driver etm4_platform_driver = {
      .probe        = etm4_probe_platform_dev,
      .remove        = etm4_remove_platform_dev,
      .driver            = {
          .name            = "coresight-etm4x",
          .of_match_table        = etm4_sysreg_match,
+        .acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
          .suppress_bind_attrs    = true,
      },
};
Quoted 90 lines
read in the driver code, the MMIO read access to TRCIDR1 occurs after a TRCDEVARCH access. The comments suggest this was to accommodate potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless the TRCOSLAR.OSLK lock is cleared first.
Quoted 42 lines
Quoted 28 lines
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
    .../coresight/coresight-etm4x-core.c          | 36
+++++++++++++++----
    drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
    2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +8 -0
index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
        drvdata = dev_get_drvdata(init_arg->dev);
        csa = init_arg->csa;
    +    /* Detect the support for OS Lock before we actually use it */
+    etm_detect_os_lock(drvdata, csa);
+ > +    /*
+     * For ETM implementations that consider MMIO an external access
+     * clear TRCOSLAR.OSLK early.
+     */
+    if (drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
+
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1109
Quoted 10 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +4 -2
        /*
         * If we are unable to detect the access mechanism,
         * or unable to detect the trace unit type, fail
-     * early.
+     * early. Reset TRCOSLAR.OSLK if cleared.
         */
-    if (!etm4_init_csdev_access(drvdata, csa))
+    if (!etm4_init_csdev_access(drvdata, csa)) {
+        if (drvdata->mmio_external)
+            etm4_os_lock(drvdata);
Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ?
Quoted 4 lines
Or thinking more about it, we could actually move the unlock step early for all ETMs irrespective of whether they are affected by this erratum. Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that.
I agree the lock could be cleared earlier in the code. That's what this patch does for Ampere. If it's decided ok to do for other (or all) manufacturers, then the Ampere specific ID check goes away in this place. The Ampere ID check (and flag) to determine whether the [Patch 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or use an existing feature mask as suggested by Mike Leach in a later review.
Quoted 19 lines
Quoted 19 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +7 -2
            return;
+    }
    -    /* Detect the support for OS Lock before we actually use it */
-    etm_detect_os_lock(drvdata, csa);
+    /*
+     * Make sure all registers are accessible
+     * TRCOSLAR.OSLK may already be clear
+     */
+    if (!drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
    -    /* 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);
@@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
        init_arg.csa = &access;
        init_arg.pid = etm_pid;
Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void · line 2049
Quoted 5 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +4 -0
    +    /*
+     * Ampere ETM v4.6 considers MMIO access as external. This mask
+     * isolates the manufacturer JEP106 ID in the PID.
+     * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+     */
Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.
Quoted 3 lines
This is the first Ampere ETMv4.x implementation. I wrote the ID check like this specifically because Ampere does not intend to address this for ETM designs in progress.
I would recommend to make this mask stricter and apply this to the current implementation. When there are more, we could add this here, rather than having to leave this work around for all the possible cores.
Quoted 9 lines
Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void · line 2051
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +2 -0
+    if ((init_arg.pid & 0x000FF000) == 0x00096000)
+        drvdata->mmio_external = true;
Like I said, we may be able to get rid of this flag and do the step for all ETMs. But before all of that, I would like to see if this is problem because we are skipping the system instruction route.
Quoted 4 lines
We understand MMIO access is deprecated going forward. There is other Linux code to be concerned about. For example, AMBA code reads the component PID/CID. This discovery code uses mapped values digested from the CoreSight ACPI which are the descriptions and graphs for the
With the "proposed" ACPI support for system register, AMBA would not be involved at all.
Quoted 3 lines
manufacturer trace implementation. There may be other Linux code I'm not aware. Note the ASL examples in ARM Document number: DEN0067 specify MMIO locations for every CoreSight component.

Quoted 6 lines

Quoted 6 lines
Yes, but this was never updated to cover the system register based implementations. I will chase this up.
Suzuki

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-23 19:49 UTC #7 lore raw

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

Quoted 55 lines · Suzuki K Poulose

Quoted 42 lines · Steve Clevenger
Quoted 39 lines · Suzuki K Poulose
Quoted 37 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for
non-Ampere parts.
Steve
Quoted 15 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 9 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Quoted 8 lines
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
Quoted 6 lines
Quoted 2 lines
That doesn't answe the question. Please could you confirm the value of ID_AA64DFR0_EL1 on your system ?
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.

Quoted 7 lines

Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on this at the moment and will be available soon.
The hack patch below should be sufficient to give it a try and if it works.

I don't understand your postscript. Certainly there's driver work to be done, but I also think the DEN0067 CoreSight ACPI specification needs update. There's no example for defining a trace implementation without memory mapped component descriptions. Considering the ACPI as it exists, the component graphs are the most important. Please see my last exchange with Mike Leach.

My patches were submitted based on the existing CoreSight driver, not what it should be. Ampere does not have the flexibility to wait for a decision on some details we've discussed. I'm available to work through any concerns the maintainers have with my submissions. What is the best way forward here?

Quoted 195 lines

Kind regards
Suzuki
Quoted 191 lines
Quoted 191 lines
Or, are you able to try this on your ACPI based system and see if you are able to use the etm ? (UNTESTED hack !)
drivers/acpi/acpi_amba.c +0 -0
index f5b443ab01c2..099966cbac5a 100644
@@ -22,7 +22,6 @@
   static const struct acpi_device_id amba_id_list[] = {
{"ARMH0061", 0}, /* PL061 GPIO Device */ {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ {"ARMHC501", 0}, /* ARM CoreSight ETR */ {"ARMHC502", 0}, /* ARM CoreSight STM */ {"ARMHC503", 0}, /* ARM CoreSight Debug */
drivers/hwtracing/coresight/coresight-etm4x-core.c +11 -0
index 1ea8f173cca0..66670533fd54 100644
@@ -3,6 +3,7 @@
    * Copyright (c) 2014, The Linux Foundation. All rights reserved.
    */
 
+#include <linux/acpi.h>
   #include <linux/bitops.h>
   #include <linux/kernel.h>
   #include <linux/moduleparam.h>
@@ -2286,12 +2287,22 @@ static const struct of_device_id
 etm4_sysreg_match[] = {
       {}
   };
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id etm4x_acpi_ids[] = {
+    {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
+    {}
+};
+
+MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
+#endif
+
   static struct platform_driver etm4_platform_driver = {
       .probe        = etm4_probe_platform_dev,
       .remove        = etm4_remove_platform_dev,
       .driver            = {
           .name            = "coresight-etm4x",
           .of_match_table        = etm4_sysreg_match,
+        .acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
           .suppress_bind_attrs    = true,
       },
};
Quoted 94 lines
read in the driver code, the MMIO read access to TRCIDR1 occurs after a TRCDEVARCH access. The comments suggest this was to accommodate potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless the TRCOSLAR.OSLK lock is cleared first.
Quoted 43 lines
Quoted 31 lines
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 36
+++++++++++++++----
drivers/hwtracing/coresight/coresight-etm4x.h | 2 ++
    2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
         drvdata = dev_get_drvdata(init_arg->dev);
         csa = init_arg->csa;
    +    /* Detect the support for OS Lock before we actually use it */
+    etm_detect_os_lock(drvdata, csa);
+ > +    /*
+     * For ETM implementations that consider MMIO an external access +     * clear TRCOSLAR.OSLK early.
+     */
+    if (drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
+
        /*
         * If we are unable to detect the access mechanism,
Quoted 8 lines
         * or unable to detect the trace unit type, fail -     * early.
+     * early. Reset TRCOSLAR.OSLK if cleared. */
-    if (!etm4_init_csdev_access(drvdata, csa)) +    if (!etm4_init_csdev_access(drvdata, csa)) { +        if (drvdata->mmio_external)
+            etm4_os_lock(drvdata);
Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ?
Quoted 6 lines
Or thinking more about it, we could actually move the unlock step early
for all ETMs irrespective of whether they are affected by this erratum.
Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that.
I agree the lock could be cleared earlier in the code. That's what this patch does for Ampere. If it's decided ok to do for other (or all) manufacturers, then the Ampere specific ID check goes away in this place. The Ampere ID check (and flag) to determine whether the [Patch 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or use an existing feature mask as suggested by Mike Leach in a later review.
Quoted 20 lines
Quoted 20 lines
            return;
+    }
    -    /* Detect the support for OS Lock before we actually use it */
-    etm_detect_os_lock(drvdata, csa);
+    /*
+     * Make sure all registers are accessible
+     * TRCOSLAR.OSLK may already be clear
+     */
+    if (!drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
    -    /* 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);
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
         init_arg.csa = &access;
         init_arg.pid = etm_pid;
Quoted 8 lines
Quoted 5 lines
    +    /*
+     * Ampere ETM v4.6 considers MMIO access as external. This mask +     * isolates the manufacturer JEP106 ID in the PID.
+     * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8) +     */
Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.
Quoted 3 lines
This is the first Ampere ETMv4.x implementation. I wrote the ID check like this specifically because Ampere does not intend to address this for ETM designs in progress.
I would recommend to make this mask stricter and apply this to the current implementation. When there are more, we could add this here, rather than having to leave this work around for all the possible cores.
Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
+ if ((init_arg.pid & 0x000FF000) == 0x00096000) + drvdata->mmio_external = true;
Like I said, we may be able to get rid of this flag and do the step for
all ETMs. But before all of that, I would like to see if this is problem
because we are skipping the system instruction route.
Quoted 4 lines
We understand MMIO access is deprecated going forward. There is other Linux code to be concerned about. For example, AMBA code reads the component PID/CID. This discovery code uses mapped values digested from the CoreSight ACPI which are the descriptions and graphs for the
With the "proposed" ACPI support for system register, AMBA would not be involved at all.
Quoted 4 lines
manufacturer trace implementation. There may be other Linux code I'm not
aware. Note the ASL examples in ARM Document number: DEN0067 specify MMIO locations for every CoreSight component.
Yes, but this was never updated to cover the system register based implementations. I will chase this up.
Suzuki

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-23 22:19 UTC #8 lore raw

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

Quoted 60 lines · Steve Clevenger

Quoted 57 lines · Suzuki K Poulose
Quoted 42 lines · Steve Clevenger
Quoted 39 lines · Suzuki K Poulose
Quoted 37 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for
non-Ampere parts.
Steve
Quoted 15 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 9 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Quoted 8 lines
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
Quoted 6 lines
Quoted 2 lines
That doesn't answe the question. Please could you confirm the value of ID_AA64DFR0_EL1 on your system ?
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?

Quoted 8 lines

Quoted 5 lines
* PS: The ACPI support for the ETM/ETE needs additional changes to the CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on this at the moment and will be available soon.
The hack patch below should be sufficient to give it a try and if it works.
I don't understand your postscript. Certainly there's driver work to be done, but I also think the DEN0067 CoreSight ACPI specification needs

The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver) compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].

So, that needs a bit of ground work to move ETM driver away from AMBA and then we can have a single driver handling both devices. It is much easier on DT based systems, as we have different compatible strings.

Quoted 2 lines

update. There's no example for defining a trace implementation without memory mapped component descriptions. Considering the ACPI as it exists,

I have raised this with the concerned people in Arm and can share the update via other channels once we have it. Basically we should be able to reuse the one for MMIO based systems, except the memory resource.
The Graph connections remain unaffected by this change. This never
made it to the document.

Quoted 5 lines

the component graphs are the most important. Please see my last exchange with Mike Leach.
My patches were submitted based on the existing CoreSight driver, not
what it should be. Ampere does not have the flexibility to wait for a > decision on some details we've discussed. I'm available to work through

Understand. But that doesn't mean we can push something in that is not meant to work the way it should.

Quoted 2 lines

any concerns the maintainers have with my submissions. What is the best way forward here?

If this is a system with system instruction access, it should be described as such and can be supported when the above mentioned change is in the kernel.

Suzuki

Quoted 189 lines

Quoted 189 lines
Kind regards
Suzuki
Quoted 185 lines
Quoted 185 lines
Or, are you able to try this on your ACPI based system and see if you are able to use the etm ? (UNTESTED hack !)
drivers/acpi/acpi_amba.c +0 -1
index f5b443ab01c2..099966cbac5a 100644
@@ -22,7 +22,6 @@
   static const struct acpi_device_id amba_id_list[] = {
       {"ARMH0061", 0}, /* PL061 GPIO Device */
       {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */
-    {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
       {"ARMHC501", 0}, /* ARM CoreSight ETR */
       {"ARMHC502", 0}, /* ARM CoreSight STM */
       {"ARMHC503", 0}, /* ARM CoreSight Debug */
drivers/hwtracing/coresight/coresight-etm4x-core.c +11 -0
index 1ea8f173cca0..66670533fd54 100644
@@ -3,6 +3,7 @@
    * Copyright (c) 2014, The Linux Foundation. All rights reserved.
    */
 
+#include <linux/acpi.h>
   #include <linux/bitops.h>
   #include <linux/kernel.h>
   #include <linux/moduleparam.h>
@@ -2286,12 +2287,22 @@ static const struct of_device_id
 etm4_sysreg_match[] = {
       {}
   };
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id etm4x_acpi_ids[] = {
+    {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
+    {}
+};
+
+MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
+#endif
+
   static struct platform_driver etm4_platform_driver = {
       .probe        = etm4_probe_platform_dev,
       .remove        = etm4_remove_platform_dev,
       .driver            = {
           .name            = "coresight-etm4x",
           .of_match_table        = etm4_sysreg_match,
+        .acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
           .suppress_bind_attrs    = true,
       },
};
Quoted 94 lines
read in the driver code, the MMIO read access to TRCIDR1 occurs after a TRCDEVARCH access. The comments suggest this was to accommodate potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless the TRCOSLAR.OSLK lock is cleared first.
Quoted 43 lines
Quoted 31 lines
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
     .../coresight/coresight-etm4x-core.c          | 36
+++++++++++++++----
     drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
     2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
         drvdata = dev_get_drvdata(init_arg->dev);
         csa = init_arg->csa;
     +    /* Detect the support for OS Lock before we actually use
it */
+    etm_detect_os_lock(drvdata, csa);
+ > +    /*
+     * For ETM implementations that consider MMIO an external access +     * clear TRCOSLAR.OSLK early.
+     */
+    if (drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
+
        /*
         * If we are unable to detect the access mechanism,
Quoted 8 lines
         * or unable to detect the trace unit type, fail
-     * early.
+     * early. Reset TRCOSLAR.OSLK if cleared.
         */
-    if (!etm4_init_csdev_access(drvdata, csa)) +    if (!etm4_init_csdev_access(drvdata, csa)) { +        if (drvdata->mmio_external)
+            etm4_os_lock(drvdata);
Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ?
Quoted 6 lines
Or thinking more about it, we could actually move the unlock step early
for all ETMs irrespective of whether they are affected by this erratum.
Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that.
I agree the lock could be cleared earlier in the code. That's what this patch does for Ampere. If it's decided ok to do for other (or all) manufacturers, then the Ampere specific ID check goes away in this place. The Ampere ID check (and flag) to determine whether the [Patch 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or use an existing feature mask as suggested by Mike Leach in a later review.
Quoted 20 lines
Quoted 20 lines
            return;
+    }
    -    /* Detect the support for OS Lock before we actually use
it */
-    etm_detect_os_lock(drvdata, csa);
+    /*
+     * Make sure all registers are accessible
+     * TRCOSLAR.OSLK may already be clear
+     */
+    if (!drvdata->mmio_external)
+        etm4_os_unlock_csa(drvdata, csa);
    -    /* 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);
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
         init_arg.csa = &access;
         init_arg.pid = etm_pid;
Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void · line 2049
Quoted 5 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +4 -0
     +    /*
+     * Ampere ETM v4.6 considers MMIO access as external. This mask
+     * isolates the manufacturer JEP106 ID in the PID.
+     * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+     */
Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.
Quoted 3 lines
This is the first Ampere ETMv4.x implementation. I wrote the ID check like this specifically because Ampere does not intend to address this for ETM designs in progress.
I would recommend to make this mask stricter and apply this to the current implementation. When there are more, we could add this here, rather than having to leave this work around for all the possible cores.
Quoted 11 lines
Quoted 10 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_probe(struct device *dev, void · line 2051
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +2 -0
+    if ((init_arg.pid & 0x000FF000) == 0x00096000)
+        drvdata->mmio_external = true;
Like I said, we may be able to get rid of this flag and do the step for
all ETMs. But before all of that, I would like to see if this is problem
because we are skipping the system instruction route.
Quoted 4 lines
We understand MMIO access is deprecated going forward. There is other Linux code to be concerned about. For example, AMBA code reads the component PID/CID. This discovery code uses mapped values digested from the CoreSight ACPI which are the descriptions and graphs for the
With the "proposed" ACPI support for system register, AMBA would not be involved at all.
Quoted 4 lines
manufacturer trace implementation. There may be other Linux code I'm not
aware. Note the ASL examples in ARM Document number: DEN0067 specify MMIO locations for every CoreSight component.

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Yes, but this was never updated to cover the system register based implementations. I will chase this up.
Suzuki

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-23 22:52 UTC #9 lore raw

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

Missed the reference, see below.

Quoted 67 lines · Suzuki K Poulose

Quoted 65 lines · Steve Clevenger
Quoted 62 lines · Suzuki K Poulose
Quoted 42 lines · Steve Clevenger
Quoted 39 lines · Suzuki K Poulose
Quoted 37 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for
non-Ampere parts.
Steve
Quoted 15 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 9 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Quoted 8 lines
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support.
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
Quoted 6 lines
Quoted 2 lines
That doesn't answe the question. Please could you confirm the value of ID_AA64DFR0_EL1 on your system ?
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on this at the moment and will be available soon.
The hack patch below should be sufficient to give it a try and if it works.

Quoted 8 lines

Quoted 2 lines
I don't understand your postscript. Certainly there's driver work to be done, but I also think the DEN0067 CoreSight ACPI specification needs
The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver) compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].

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

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-02-02 05:21 UTC #10 lore raw

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

Hi Suzuki,

I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch and added references to the Ampere erratum in silicon-errata.rst. These will be submitted as separate patches.

The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem for all manufacturers. I think this is what you asked for. The no_quad_mmio flag has moved to struct csdev_access, and the split 64-bit read/write logic has been implemented entirely in the header file coresight-etm4x.h is the existing calls.
I'd like to retire this patch thread, and submit these as a new thread. Is there an objection?

Thanks,
Steve

Quoted 82 lines · Suzuki K Poulose

Missed the reference, see below.
Quoted 78 lines · Suzuki K Poulose
Quoted 76 lines · Steve Clevenger
Quoted 63 lines · Suzuki K Poulose
Quoted 46 lines · Steve Clevenger
Quoted 43 lines · Suzuki K Poulose
Quoted 41 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in
the Ampere support was to otherwise not disturb existing driver code for
non-Ampere parts.
Steve
Quoted 18 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 10 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access.
Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Quoted 7 lines
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ?
If it does, I think we should drop all of this and simply wire the system instruction access support.
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
Quoted 8 lines
Quoted 3 lines
That doesn't answe the question. Please could you confirm the value of
ID_AA64DFR0_EL1 on your system ?
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register
interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?
Quoted 7 lines
* PS: The ACPI support for the ETM/ETE needs additional changes to the CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on
this at the moment and will be available soon.
The hack patch below should be sufficient to give it a try and if it works.
I don't understand your postscript. Certainly there's driver work to be done, but I also think the DEN0067 CoreSight ACPI specification needs

Quoted 9 lines

Quoted 5 lines
The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver) compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].
[0] https://lkml.kernel.org/r/[email protected]

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-02-02 11:18 UTC #11 lore raw

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

Quoted 4 lines · Steve Clevenger

Hi Suzuki,
I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch

Thanks for that.

Quoted 10 lines

and added references to the Ampere erratum in silicon-errata.rst. These will be submitted as separate patches.
The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem for all manufacturers. I think this is what you asked for. The no_quad_mmio flag has moved to struct csdev_access, and the split 64-bit read/write logic has been implemented entirely in the header file coresight-etm4x.h is the existing calls.
I'd like to retire this patch thread, and submit these as a new thread. Is there an objection?

I would still like to use the system instructions method for the ETM, than hacking the MMIO access for something that is obsolete. The ACPI document for CoreSight will be published soon for review to
accommodate the description for ETMs without MMIO and it no longer
mandates the MemoryResource.

What is the objection to using system instruction access here ?

Thanks
Suzuki

Quoted 96 lines

Thanks,
Steve
Quoted 91 lines · Suzuki K Poulose
Missed the reference, see below.
Quoted 75 lines · Suzuki K Poulose
Quoted 73 lines · Steve Clevenger
Quoted 70 lines · Suzuki K Poulose
Quoted 46 lines · Steve Clevenger
Quoted 43 lines · Suzuki K Poulose
Quoted 41 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in
the Ampere support was to otherwise not disturb existing driver code for
non-Ampere parts.
Steve
Quoted 18 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 10 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access.
Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Quoted 7 lines
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ?
If it does, I think we should drop all of this and simply wire the system instruction access support.
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
Quoted 8 lines
Quoted 3 lines
That doesn't answe the question. Please could you confirm the value of
ID_AA64DFR0_EL1 on your system ?
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register
interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on
this at the moment and will be available soon.
The hack patch below should be sufficient to give it a try and if it works.
Quoted 8 lines
Quoted 2 lines
I don't understand your postscript. Certainly there's driver work to be done, but I also think the DEN0067 CoreSight ACPI specification needs
The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver) compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].
[0] https://lkml.kernel.org/r/[email protected]

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-02-02 17:13 UTC #12 lore raw

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

Hi Suzuki,

Commented in-line.

Steve C.

Quoted 19 lines · Suzuki K Poulose

Quoted 4 lines · Steve Clevenger
Hi Suzuki,
I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch
Thanks for that.
Quoted 10 lines
and added references to the Ampere erratum in silicon-errata.rst. These will be submitted as separate patches.
The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem for all manufacturers. I think this is what you asked for. The no_quad_mmio flag has moved to struct csdev_access, and the split 64-bit read/write logic has been implemented entirely in the header file coresight-etm4x.h is the existing calls.
I'd like to retire this patch thread, and submit these as a new thread. Is there an objection?

Quoted 7 lines

I would still like to use the system instructions method for the ETM, than hacking the MMIO access for something that is obsolete. The ACPI document for CoreSight will be published soon for review to accommodate the description for ETMs without MMIO and it no longer mandates the MemoryResource.
What is the objection to using system instruction access here ?

No objection going forward. For our initial release, we're not in a position to change the CoreSight DSDT based on a specification which is incomplete.
Based on a quick sysreg only build, I didn't collect trace samples. I haven't had time to chase this, but the reported error is "timeout while waiting for Trace Idle Status" on a TRCSTATR read. More testing is required. If this isn't related to an Ampere sysreg access problem (doubtful), the next place I'd look is as a synchronization issue.

Quoted 113 lines

Thanks
Suzuki
Quoted 106 lines
Thanks,
Steve
Quoted 101 lines · Suzuki K Poulose
Missed the reference, see below.
Quoted 86 lines · Suzuki K Poulose
Quoted 78 lines · Steve Clevenger
Quoted 75 lines · Suzuki K Poulose
Quoted 54 lines · Steve Clevenger
Quoted 44 lines · Suzuki K Poulose
Quoted 42 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in
the Ampere support was to otherwise not disturb existing driver code
for
non-Ampere parts.
Steve
Quoted 29 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 5 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access.
Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an
Quoted 8 lines
external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access
results in a bus fault followed by a kernel panic. A TRCIDR1 read
is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ?
If it does, I think we should drop all of this and simply wire the
system instruction access support.
Quoted 7 lines
Quoted 3 lines
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
That doesn't answe the question. Please could you confirm the value of
ID_AA64DFR0_EL1 on your system ?
Quoted 6 lines
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected
to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register
interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the
CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on
this at the moment and will be available soon.
Quoted 6 lines
Quoted 2 lines
The hack patch below should be sufficient to give it a try and if it works.
I don't understand your postscript. Certainly there's driver work to be
done, but I also think the DEN0067 CoreSight ACPI specification needs
Quoted 6 lines
The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver)
compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].
[0]
https://lkml.kernel.org/r/[email protected]

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-02-02 17:29 UTC #13 lore raw

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

Quoted 32 lines · Steve Clevenger

Hi Suzuki,
Commented in-line.
Steve C.
Quoted 25 lines · Suzuki K Poulose
Quoted 4 lines · Steve Clevenger
Hi Suzuki,
I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch
Thanks for that.
Quoted 10 lines
and added references to the Ampere erratum in silicon-errata.rst. These will be submitted as separate patches.
The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem for all manufacturers. I think this is what you asked for. The no_quad_mmio flag has moved to struct csdev_access, and the split 64-bit read/write logic has been implemented entirely in the header file coresight-etm4x.h is the existing calls.
I'd like to retire this patch thread, and submit these as a new thread. Is there an objection?
I would still like to use the system instructions method for the ETM, than hacking the MMIO access for something that is obsolete. The ACPI document for CoreSight will be published soon for review to accommodate the description for ETMs without MMIO and it no longer mandates the MemoryResource.

Quoted 4 lines

Quoted 1 line
What is the objection to using system instruction access here ?
No objection going forward. For our initial release, we're not in a position to change the CoreSight DSDT based on a specification which is incomplete.

There is on change to the CoreSight DSDT specification as such. The only change to the "spec" is along the lines of :

"MMIO interface is mandatory only if not accessible via system instruction access "

Quoted 3 lines

Based on a quick sysreg only build, I didn't collect trace samples. I haven't had time to chase this, but the reported error is "timeout while waiting for Trace Idle Status" on a TRCSTATR read. More testing is

Are you able to access the other registers ?

e.g,

$ cat /sys/bus/coresight/devices/etm0/mgmt/trcpidr0

Suzuki

Quoted 116 lines

required. If this isn't related to an Ampere sysreg access problem (doubtful), the next place I'd look is as a synchronization issue.
Quoted 113 lines
Thanks
Suzuki
Quoted 106 lines
Thanks,
Steve
Quoted 101 lines · Suzuki K Poulose
Missed the reference, see below.
Quoted 86 lines · Suzuki K Poulose
Quoted 78 lines · Steve Clevenger
Quoted 75 lines · Suzuki K Poulose
Quoted 54 lines · Steve Clevenger
Quoted 44 lines · Suzuki K Poulose
Quoted 42 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in
the Ampere support was to otherwise not disturb existing driver code
for
non-Ampere parts.
Steve
Quoted 29 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 5 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access.
Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an
Quoted 8 lines
external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access
results in a bus fault followed by a kernel panic. A TRCIDR1 read
is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ?
If it does, I think we should drop all of this and simply wire the
system instruction access support.
Quoted 7 lines
Quoted 3 lines
That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've
That doesn't answe the question. Please could you confirm the value of
ID_AA64DFR0_EL1 on your system ?
Quoted 6 lines
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected
to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register
interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the
CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on
this at the moment and will be available soon.
Quoted 6 lines
Quoted 2 lines
The hack patch below should be sufficient to give it a try and if it works.
I don't understand your postscript. Certainly there's driver work to be
done, but I also think the DEN0067 CoreSight ACPI specification needs
Quoted 6 lines
The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver)
compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].
[0]
https://lkml.kernel.org/r/[email protected]

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-02-02 23:05 UTC #14 lore raw

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

Quoted 50 lines · Suzuki K Poulose

Quoted 33 lines · Steve Clevenger
Hi Suzuki,
Commented in-line.
Steve C.
Quoted 26 lines · Suzuki K Poulose
Quoted 4 lines · Steve Clevenger
Hi Suzuki,
I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch
Thanks for that.
Quoted 11 lines
and added references to the Ampere erratum in silicon-errata.rst. These will be submitted as separate patches.
The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem for all manufacturers. I think this is what you asked for. The no_quad_mmio flag has moved to struct csdev_access, and the split 64-bit read/write logic has been implemented entirely in the header file
coresight-etm4x.h is the existing calls.
I'd like to retire this patch thread, and submit these as a new thread. Is there an objection?
I would still like to use the system instructions method for the ETM, than hacking the MMIO access for something that is obsolete. The ACPI document for CoreSight will be published soon for review to accommodate the description for ETMs without MMIO and it no longer mandates the MemoryResource.
Quoted 4 lines
Quoted 1 line
What is the objection to using system instruction access here ?
No objection going forward. For our initial release, we're not in a position to change the CoreSight DSDT based on a specification which is incomplete.
There is on change to the CoreSight DSDT specification as such. The only change to the "spec" is along the lines of :
"MMIO interface is mandatory only if not accessible via system instruction access "
Quoted 3 lines
Based on a quick sysreg only build, I didn't collect trace samples. I haven't had time to chase this, but the reported error is "timeout while waiting for Trace Idle Status" on a TRCSTATR read. More testing is

Quoted 6 lines

Are you able to access the other registers ?
e.g,
$ cat /sys/bus/coresight/devices/etm0/mgmt/trcpidr0

I'll need to determine which of the 3 coresight_timeout calls which read the TRCSTATR idle bit times out. In each case a number of sysreg accesses have occurred prior to any of these 3 calls.

Quoted 124 lines

Suzuki
Quoted 120 lines
required. If this isn't related to an Ampere sysreg access problem (doubtful), the next place I'd look is as a synchronization issue.
Quoted 117 lines
Thanks
Suzuki
Quoted 110 lines
Thanks,
Steve
Quoted 105 lines · Suzuki K Poulose
Missed the reference, see below.
Quoted 90 lines · Suzuki K Poulose
Quoted 80 lines · Steve Clevenger
Quoted 77 lines · Suzuki K Poulose
Quoted 56 lines · Steve Clevenger
Quoted 45 lines · Suzuki K Poulose
Quoted 43 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in
the Ampere support was to otherwise not disturb existing driver code
for
non-Ampere parts.
Steve
Quoted 30 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
Quoted 13 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1
access.
Ampere Computing erratum AC03_DEBUG_06 describes an Ampere
Computing design decision MMIO reads are considered the same
as an
external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1
access
results in a bus fault followed by a kernel panic. A TRCIDR1
read
is valid regardless of TRCOSLAR.OSLK provided MMIO access
(now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system instructions and
that is causing this issue of "MMIO access is considered external" ?
If it does, I think we should drop all of this and simply wire the
system instruction access support.
Quoted 8 lines
Quoted 4 lines
That's not the issue in this case. This MMIO access should've been
allowed by the Ampere ETMv4.6 implementation. Based on comments I've
That doesn't answe the question. Please could you confirm the value of
ID_AA64DFR0_EL1 on your system ?
Quoted 6 lines
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected
to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register
interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the
CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on
this at the moment and will be available soon.
Quoted 8 lines
Quoted 3 lines
The hack patch below should be sufficient to give it a try and if it
works.
I don't understand your postscript. Certainly there's driver work to be
done, but I also think the DEN0067 CoreSight ACPI specification needs
Quoted 6 lines
The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver)
compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].
[0]
https://lkml.kernel.org/r/[email protected]

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-03-01 10:08 UTC #15 lore raw

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

Hi Steve

fyi, the ACPI spec for CoreSight has been updated to relax the MMIO region requirement for ETMv4x and ETE. Please find it here :

https://developer.arm.com/documentation/den0067/latest

Kind regards
Suzuki

Quoted 55 lines · Steve Clevenger

Quoted 52 lines · Suzuki K Poulose
Quoted 33 lines · Steve Clevenger
Hi Suzuki,
Commented in-line.
Steve C.
Quoted 26 lines · Suzuki K Poulose
Quoted 4 lines · Steve Clevenger
Hi Suzuki,
I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch
Thanks for that.
Quoted 11 lines
and added references to the Ampere erratum in silicon-errata.rst. These will be submitted as separate patches.
The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem for all manufacturers. I think this is what you asked for. The no_quad_mmio flag has moved to struct csdev_access, and the split 64-bit read/write logic has been implemented entirely in the header file
coresight-etm4x.h is the existing calls.
I'd like to retire this patch thread, and submit these as a new thread. Is there an objection?
I would still like to use the system instructions method for the ETM, than hacking the MMIO access for something that is obsolete. The ACPI document for CoreSight will be published soon for review to accommodate the description for ETMs without MMIO and it no longer mandates the MemoryResource.
Quoted 4 lines
Quoted 1 line
What is the objection to using system instruction access here ?
No objection going forward. For our initial release, we're not in a position to change the CoreSight DSDT based on a specification which is incomplete.
There is on change to the CoreSight DSDT specification as such. The only change to the "spec" is along the lines of :
"MMIO interface is mandatory only if not accessible via system instruction access "
Quoted 3 lines
Based on a quick sysreg only build, I didn't collect trace samples. I haven't had time to chase this, but the reported error is "timeout while waiting for Trace Idle Status" on a TRCSTATR read. More testing is
Are you able to access the other registers ?

Quoted 132 lines

Quoted 4 lines
e.g,
$ cat /sys/bus/coresight/devices/etm0/mgmt/trcpidr0
I'll need to determine which of the 3 coresight_timeout calls which read the TRCSTATR idle bit times out. In each case a number of sysreg accesses have occurred prior to any of these 3 calls.
Quoted 124 lines
Suzuki
Quoted 120 lines
required. If this isn't related to an Ampere sysreg access problem (doubtful), the next place I'd look is as a synchronization issue.
Quoted 117 lines
Thanks
Suzuki
Quoted 110 lines
Thanks,
Steve
Quoted 105 lines · Suzuki K Poulose
Missed the reference, see below.
Quoted 90 lines · Suzuki K Poulose
Quoted 80 lines · Steve Clevenger
Quoted 77 lines · Suzuki K Poulose
Quoted 56 lines · Steve Clevenger
Quoted 45 lines · Suzuki K Poulose
Quoted 43 lines · Steve Clevenger
Hi Suzuki,
Comments in-line. Please note the approach I attempted while adding in
the Ampere support was to otherwise not disturb existing driver code
for
non-Ampere parts.
Steve
Quoted 30 lines · Suzuki K Poulose
Hi Steve
Thanks for the patches. Have a few comments below.
On 20/01/2023 00:51, Steve Clevenger wrote:
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1
access.
Ampere Computing erratum AC03_DEBUG_06 describes an Ampere
Computing design decision MMIO reads are considered the same
as an
external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1
access
results in a bus fault followed by a kernel panic. A TRCIDR1
read
is valid regardless of TRCOSLAR.OSLK provided MMIO access
(now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Please could you add this erratum to the :
Documentation/arm64/silicon-errata.rst ?
Given the ETM is v4.6, doesn't it support system
instructions and
that is causing this issue of "MMIO access is considered
external" ?
If it does, I think we should drop all of this and simply wire
the
system instruction access support.
Quoted 8 lines
Quoted 4 lines
That's not the issue in this case. This MMIO access should've been
allowed by the Ampere ETMv4.6 implementation. Based on comments I've
That doesn't answe the question. Please could you confirm the value of
ID_AA64DFR0_EL1 on your system ?
Quoted 6 lines
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected
to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register
interface must be implemented on all ETMv4.6 parts.
So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs.
Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the
CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on
this at the moment and will be available soon.
Quoted 8 lines
Quoted 3 lines
The hack patch below should be sufficient to give it a try and if it
works.
I don't understand your postscript. Certainly there's driver work to be
done, but I also think the DEN0067 CoreSight ACPI specification needs
Quoted 6 lines
The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver)
compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0].
[0]
https://lkml.kernel.org/r/[email protected]

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Mike Leach

2023-01-20 11:50 UTC #16 lore raw

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

In reply to Steve Clevenger

Hi Steve,

Quoted 24 lines · Steve Clevenger

Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
 .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
 2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644

drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1093

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0

@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
        drvdata = dev_get_drvdata(init_arg->dev);
        csa = init_arg->csa;

As far as I can tell there appears to be an initialisation issue here. etm_probe()
...
struct csdev_access access = { 0 };
...
init_arg.csa = &access

::call=> etm4_init_arch_data(init_arg)

Thus csa is uninitialised?

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +8 -0

+       /* Detect the support for OS Lock before we actually use it */
+       etm_detect_os_lock(drvdata, csa);
+
+       /*
+        * For ETM implementations that consider MMIO an external access
+        * clear TRCOSLAR.OSLK early.
+        */
+       if (drvdata->mmio_external)

drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1109

Quoted 10 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +4 -2

+               etm4_os_unlock_csa(drvdata, csa);
+
        /*
         * If we are unable to detect the access mechanism,
         * or unable to detect the trace unit type, fail
-        * early.
+        * early. Reset TRCOSLAR.OSLK if cleared.
         */
-       if (!etm4_init_csdev_access(drvdata, csa))
+       if (!etm4_init_csdev_access(drvdata, csa)) {

This call initialises csa according to sysreg / iomem access requirements

Quoted 55 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +18 -4

+               if (drvdata->mmio_external)
+                       etm4_os_lock(drvdata);
                return;
+       }
 
-       /* Detect the support for OS Lock before we actually use it */
-       etm_detect_os_lock(drvdata, csa);
+       /*
+        * Make sure all registers are accessible
+        * TRCOSLAR.OSLK may already be clear
+        */
+       if (!drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
 
-       /* 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);

@@ -2027,6 +2042,14 @@ 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 considers MMIO access as external. This mask
+        * isolates the manufacturer JEP106 ID in the PID.
+        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+        */
+       if ((init_arg.pid & 0x000FF000) == 0x00096000)
+               drvdata->mmio_external = true;
+
        /*
         * Serialize against CPUHP callbacks to avoid race condition
         * between the smp call and saving the delayed probe.

@@ -2192,6 +2215,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 */

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

index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
  * @skip_power_up: Indicates if an implementation can skip powering up
  *                the trace unit.
  * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
  */
 struct etmv4_drvdata {
        void __iomem                    *base;

drivers/hwtracing/coresight/coresight-etm4x.h struct etmv4_drvdata { · line 1070

Quoted 3 lines

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

@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
        bool                            state_needs_restore;
        bool                            skip_power_up;
        DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);

Rather than continue to add bools - is it not worthwhile adding to the bitmap above and extending the arch features API to allow a "has_feature" call?

Quoted 7 lines

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

+       bool                            mmio_external;
 };
 
 /* Address comparator access types */
--

2.25.1

Regards

Mike

Signature · 8 lines

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

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-21 07:32 UTC #17 lore raw

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

Hi Mike,

Comments in-line.

Steve

Quoted 39 lines · Mike Leach

Hi Steve,
Quoted 24 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
 .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
 2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1093
Quoted 3 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
        drvdata = dev_get_drvdata(init_arg->dev);
        csa = init_arg->csa;
 
As far as I can tell there appears to be an initialisation issue here. etm_probe()
...
struct csdev_access access = { 0 };
...
init_arg.csa = &access

Quoted 3 lines

::call=> etm4_init_arch_data(init_arg)
Thus csa is uninitialised?

It looks to me csa is intended to be initialized to zero? In any case, the Ampere check uses only the ETM pid, which is initialized directly above.

Quoted 13 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +10 -0

+       /* Detect the support for OS Lock before we actually use it */
+       etm_detect_os_lock(drvdata, csa);
+
+       /*
+        * For ETM implementations that consider MMIO an external access
+        * clear TRCOSLAR.OSLK early.
+        */
+       if (drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
+
        /*
         * If we are unable to detect the access mechanism,

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1109
Quoted 6 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +2 -2
         * or unable to detect the trace unit type, fail
-        * early.
+        * early. Reset TRCOSLAR.OSLK if cleared.
         */
-       if (!etm4_init_csdev_access(drvdata, csa))
+       if (!etm4_init_csdev_access(drvdata, csa)) {
This call initialises csa according to sysreg / iomem access requirements

csa is initialized only when no drvdata->base exists. Under what circumstance would there be no ETM base given the recommended CoreSight ACPI implementation? See the examples in ARM Document number: DEN0067.

Quoted 57 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +18 -4

+               if (drvdata->mmio_external)
+                       etm4_os_lock(drvdata);
                return;
+       }
 
-       /* Detect the support for OS Lock before we actually use it */
-       etm_detect_os_lock(drvdata, csa);
+       /*
+        * Make sure all registers are accessible
+        * TRCOSLAR.OSLK may already be clear
+        */
+       if (!drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
 
-       /* 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);

@@ -2027,6 +2042,14 @@ 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 considers MMIO access as external. This mask
+        * isolates the manufacturer JEP106 ID in the PID.
+        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+        */
+       if ((init_arg.pid & 0x000FF000) == 0x00096000)
+               drvdata->mmio_external = true;
+
        /*
         * Serialize against CPUHP callbacks to avoid race condition
         * between the smp call and saving the delayed probe.

@@ -2192,6 +2215,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 */

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

index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
  * @skip_power_up: Indicates if an implementation can skip powering up
  *                the trace unit.
  * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
  */
 struct etmv4_drvdata {
        void __iomem                    *base;

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x.h struct etmv4_drvdata { · line 1070
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x.h +0 -0
@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
        bool                            state_needs_restore;
        bool                            skip_power_up;
        DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
Rather than continue to add bools - is it not worthwhile adding to the bitmap above and extending the arch features API to allow a "has_feature" call?

I can look into this. I agree using a bool for every exception doesn't scale well. Referring to one Suzuki Poulose review comment, his proposal to clear TRCOSLAR.OSLK early for all parts would mean one of these bools could go away. Otherwise, possibly add one (or more) bit definitions for use by the etm4_disable_arch_specific call. The order of this call would need to change, depending.

Quoted 11 lines

Quoted 8 lines

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

+       bool                            mmio_external;
 };
 
 /* Address comparator access types */
--

2.25.1

Regards

Mike

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Mike Leach

2023-01-23 10:55 UTC #18 lore raw

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

Hi Steve,

Quoted 48 lines · Steve Clevenger

Hi Mike,
Comments in-line.
Steve
Quoted 39 lines · Mike Leach
Hi Steve,
Quoted 24 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
 .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
 2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1093
Quoted 3 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
        drvdata = dev_get_drvdata(init_arg->dev);
        csa = init_arg->csa;
 
As far as I can tell there appears to be an initialisation issue here. etm_probe()
...
struct csdev_access access = { 0 };
...
init_arg.csa = &access

Quoted 6 lines

Quoted 3 lines
::call=> etm4_init_arch_data(init_arg)
Thus csa is uninitialised?
It looks to me csa is intended to be initialized to zero? In any case, the Ampere check uses only the ETM pid, which is initialized directly above.

Sorry, I should have been more explicit.

csa is the addressing abstraction used by all the underlying register read/write code.

It is initialised to {0} in the calling code, probably to avoid the kernel tests complaining about uninitialised use of a variable.

However in the etm4_init_csdev_access() function we are using a base address then it is initialised to:-

struct csdev_access {
    io_mem = true;
    *base = io_mem_base_addr;
};

and in the access using system registers for an etm4 to:

struct csdev_access {
    io_mem = false;
    *read = etm4x_sysreg_read()
    *write = etm4x_sysreg_write()
};

Thus all underlying register access can use the correct method for the device.

Quoted 4 lines

Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1096
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +3 -0
+       /* Detect the support for OS Lock before we actually use it */
+       etm_detect_os_lock(drvdata, csa);
+

Thus passing a 0 init csa object to the etm_detect_os_lock() fn above seems to be suspicious.

Quoted 16 lines

Quoted 15 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +9 -2

+       /*
+        * For ETM implementations that consider MMIO an external access
+        * clear TRCOSLAR.OSLK early.
+        */
+       if (drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
+
        /*
         * If we are unable to detect the access mechanism,
         * or unable to detect the trace unit type, fail
-        * early.
+        * early. Reset TRCOSLAR.OSLK if cleared.
         */
-       if (!etm4_init_csdev_access(drvdata, csa))
+       if (!etm4_init_csdev_access(drvdata, csa)) {

Quoted 3 lines

Quoted 1 line
This call initialises csa according to sysreg / iomem access requirements
csa is initialized only when no drvdata->base exists.

Not so - csa is initialised in both circumstances as described above.

Quoted 3 lines

Under what
circumstance would there be no ETM base given the recommended CoreSight ACPI implementation? See the examples in ARM Document number: DEN0067.

This will be used in the ETE devices (which share the etm4 driver), or any ETM4.6+ that uses the "arm,coresight-etm4x-sysreg" device tree binding (not sure what the ACPI equivalent is).

So, either way, you need an init csa, before passing it to the driver calls.

Later in the initialisation sequence we generate a coresight_device object which the csa is bound to, and finally if all is well the coresight_device is bound to drvdata at which point the device is ready for use.

It is unfortunate, but to handle the two methods of register access, the initilialisation process for the driver has become more complicated with ordering dependencies - to ensure that the rest of the driver remains simpler when accessing device registers.

As Suzuki mentioned - moving this specific lock requirement into the _init function would be clearer and ensure that the initialisation sequences were observed.

Regards

Mike

Quoted 83 lines

Quoted 57 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +18 -4

+               if (drvdata->mmio_external)
+                       etm4_os_lock(drvdata);
                return;
+       }
 
-       /* Detect the support for OS Lock before we actually use it */
-       etm_detect_os_lock(drvdata, csa);
+       /*
+        * Make sure all registers are accessible
+        * TRCOSLAR.OSLK may already be clear
+        */
+       if (!drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
 
-       /* 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);

@@ -2027,6 +2042,14 @@ 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 considers MMIO access as external. This mask
+        * isolates the manufacturer JEP106 ID in the PID.
+        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+        */
+       if ((init_arg.pid & 0x000FF000) == 0x00096000)
+               drvdata->mmio_external = true;
+
        /*
         * Serialize against CPUHP callbacks to avoid race condition
         * between the smp call and saving the delayed probe.

@@ -2192,6 +2215,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 */

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

index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
  * @skip_power_up: Indicates if an implementation can skip powering up
  *                the trace unit.
  * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
  */
 struct etmv4_drvdata {
        void __iomem                    *base;

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x.h struct etmv4_drvdata { · line 1070
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x.h +0 -0
@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
        bool                            state_needs_restore;
        bool                            skip_power_up;
        DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
Rather than continue to add bools - is it not worthwhile adding to the bitmap above and extending the arch features API to allow a "has_feature" call?

Quoted 11 lines

Quoted 8 lines

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

+       bool                            mmio_external;
 };
 
 /* Address comparator access types */
--

2.25.1

Regards

Mike

Signature · 8 lines

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

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-23 19:49 UTC #19 lore raw

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

Quoted 75 lines · Mike Leach

Hi Steve,
Quoted 48 lines · Steve Clevenger
Hi Mike,
Comments in-line.
Steve
Quoted 39 lines · Mike Leach
Hi Steve,
Quoted 24 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
 .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
 2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1093
Quoted 3 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
        drvdata = dev_get_drvdata(init_arg->dev);
        csa = init_arg->csa;
 
As far as I can tell there appears to be an initialisation issue here. etm_probe()
...
struct csdev_access access = { 0 };
...
init_arg.csa = &access
Quoted 6 lines
Quoted 3 lines
::call=> etm4_init_arch_data(init_arg)
Thus csa is uninitialised?
It looks to me csa is intended to be initialized to zero? In any case, the Ampere check uses only the ETM pid, which is initialized directly above.
Sorry, I should have been more explicit.
csa is the addressing abstraction used by all the underlying register read/write code.
It is initialised to {0} in the calling code, probably to avoid the kernel tests complaining about uninitialised use of a variable.
However in the etm4_init_csdev_access() function we are using a base address then it is initialised to:-
struct csdev_access {
    io_mem = true;
    *base = io_mem_base_addr;
};

Quoted 7 lines

and in the access using system registers for an etm4 to:
struct csdev_access {
    io_mem = false;
    *read = etm4x_sysreg_read()
    *write = etm4x_sysreg_write()
};

Yes, csa is initialized indirectly in etm4_init_csdev_access via calls to etm4_init_iomem_access and etm4_init_sysreg_access. So, you are correct. csa is zero initialized for the call to etm4_detect_os_lock (TRCOSLSR read) prior to my patch which clears TRCOSLAR.OSLK. The TRCOSLSR read and TRCOSLAR.OSLK clear default to sysreg access. The csa initialization problem is an existing bug I didn't see.

Quoted 38 lines

Thus all underlying register access can use the correct method for the device.
Quoted 4 lines
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1096
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +3 -0
+       /* Detect the support for OS Lock before we actually use it */
+       etm_detect_os_lock(drvdata, csa);
+
Thus passing a 0 init csa object to the etm_detect_os_lock() fn above seems to be suspicious.
Quoted 16 lines
Quoted 16 lines
Quoted 15 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +9 -2
+       /*
+        * For ETM implementations that consider MMIO an external access
+        * clear TRCOSLAR.OSLK early.
+        */
+       if (drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
+
        /*
         * If we are unable to detect the access mechanism,
         * or unable to detect the trace unit type, fail
-        * early.
+        * early. Reset TRCOSLAR.OSLK if cleared.
         */
-       if (!etm4_init_csdev_access(drvdata, csa))
+       if (!etm4_init_csdev_access(drvdata, csa)) {
Quoted 3 lines
Quoted 1 line
This call initialises csa according to sysreg / iomem access requirements
csa is initialized only when no drvdata->base exists.
Not so - csa is initialised in both circumstances as described above.
Quoted 3 lines
Under what
circumstance would there be no ETM base given the recommended CoreSight ACPI implementation? See the examples in ARM Document number: DEN0067.

Quoted 5 lines

This will be used in the ETE devices (which share the etm4 driver), or any ETM4.6+ that uses the "arm,coresight-etm4x-sysreg" device tree binding (not sure what the ACPI equivalent is).
So, either way, you need an init csa, before passing it to the driver calls.

Agreed. See my summary above.

Quoted 6 lines

Later in the initialisation sequence we generate a coresight_device object which the csa is bound to, and finally if all is well the coresight_device is bound to drvdata at which point the device is ready for use.

Quoted 8 lines

It is unfortunate, but to handle the two methods of register access, the initilialisation process for the driver has become more complicated with ordering dependencies - to ensure that the rest of the driver remains simpler when accessing device registers.
As Suzuki mentioned - moving this specific lock requirement into the _init function would be clearer and ensure that the initialisation sequences were observed.

Getting back to the etm4_init_csdev_access implementation, if a drvdata->base exists, etm4_init_sysreg_access is never called. The driver doesn't initialize sysreg access if a memory map exists by design. The existing Coresight ACPI specification only has the option of describing the manufacturer trace implementation using descriptions of memory mapped components.

Quoted 91 lines

Regards

Mike

Quoted 83 lines

Quoted 57 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +18 -4

+               if (drvdata->mmio_external)
+                       etm4_os_lock(drvdata);
                return;
+       }
 
-       /* Detect the support for OS Lock before we actually use it */
-       etm_detect_os_lock(drvdata, csa);
+       /*
+        * Make sure all registers are accessible
+        * TRCOSLAR.OSLK may already be clear
+        */
+       if (!drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
 
-       /* 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);

@@ -2027,6 +2042,14 @@ 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 considers MMIO access as external. This mask
+        * isolates the manufacturer JEP106 ID in the PID.
+        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+        */
+       if ((init_arg.pid & 0x000FF000) == 0x00096000)
+               drvdata->mmio_external = true;
+
        /*
         * Serialize against CPUHP callbacks to avoid race condition
         * between the smp call and saving the delayed probe.

@@ -2192,6 +2215,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 */

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

index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
  * @skip_power_up: Indicates if an implementation can skip powering up
  *                the trace unit.
  * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
  */
 struct etmv4_drvdata {
        void __iomem                    *base;

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x.h struct etmv4_drvdata { · line 1070
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x.h +0 -0
@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
        bool                            state_needs_restore;
        bool                            skip_power_up;
        DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
Rather than continue to add bools - is it not worthwhile adding to the bitmap above and extending the arch features API to allow a "has_feature" call?

Quoted 11 lines

Quoted 8 lines

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

+       bool                            mmio_external;
 };
 
 /* Address comparator access types */
--

2.25.1

Regards

Mike

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-23 22:50 UTC #20 lore raw

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

Quoted 150 lines · Steve Clevenger

Quoted 75 lines · Mike Leach
Hi Steve,
Quoted 48 lines · Steve Clevenger
Hi Mike,
Comments in-line.
Steve
Quoted 39 lines · Mike Leach
Hi Steve,
Quoted 24 lines · Steve Clevenger
Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access. Ampere Computing erratum AC03_DEBUG_06 describes an Ampere Computing design decision MMIO reads are considered the same as an external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access results in a bus fault followed by a kernel panic. A TRCIDR1 read is valid regardless of TRCOSLAR.OSLK provided MMIO access (now deprecated) is supported.
AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Add Ampere ETM PID required for Coresight ETM driver support.
Signed-off-by Steve Clevenger <[email protected]>
---
  .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
  drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
  2 files changed, 32 insertions(+), 6 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
index 1cc052979e01..533be1928a09 100644
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1093
Quoted 3 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -0
@@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
         drvdata = dev_get_drvdata(init_arg->dev);
         csa = init_arg->csa;
 
As far as I can tell there appears to be an initialisation issue here. etm_probe()
...
struct csdev_access access = { 0 };
...
init_arg.csa = &access
Quoted 6 lines
Quoted 3 lines
::call=> etm4_init_arch_data(init_arg)
Thus csa is uninitialised?
It looks to me csa is intended to be initialized to zero? In any case, the Ampere check uses only the ETM pid, which is initialized directly above.
Sorry, I should have been more explicit.
csa is the addressing abstraction used by all the underlying register read/write code.
It is initialised to {0} in the calling code, probably to avoid the kernel tests complaining about uninitialised use of a variable.
However in the etm4_init_csdev_access() function we are using a base address then it is initialised to:-
struct csdev_access {
     io_mem = true;
     *base = io_mem_base_addr;
};
Quoted 7 lines
and in the access using system registers for an etm4 to:
struct csdev_access {
     io_mem = false;
     *read = etm4x_sysreg_read()
     *write = etm4x_sysreg_write()
};
Yes, csa is initialized indirectly in etm4_init_csdev_access via calls to etm4_init_iomem_access and etm4_init_sysreg_access. So, you are correct. csa is zero initialized for the call to etm4_detect_os_lock (TRCOSLSR read) prior to my patch which clears TRCOSLAR.OSLK. The TRCOSLSR read and TRCOSLAR.OSLK clear default to sysreg access. The csa initialization problem is an existing bug I didn't see.
Quoted 38 lines
Thus all underlying register access can use the correct method for the device.
Quoted 4 lines
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_init_arch_data(void *info) · line 1096
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +3 -0
+       /* Detect the support for OS Lock before we actually use it */
+       etm_detect_os_lock(drvdata, csa);
+
Thus passing a 0 init csa object to the etm_detect_os_lock() fn above seems to be suspicious.
Quoted 16 lines
Quoted 16 lines
Quoted 15 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +9 -2
+       /*
+        * For ETM implementations that consider MMIO an external access
+        * clear TRCOSLAR.OSLK early.
+        */
+       if (drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
+
         /*
          * If we are unable to detect the access mechanism,
          * or unable to detect the trace unit type, fail
-        * early.
+        * early. Reset TRCOSLAR.OSLK if cleared.
          */
-       if (!etm4_init_csdev_access(drvdata, csa))
+       if (!etm4_init_csdev_access(drvdata, csa)) {
Quoted 3 lines
Quoted 1 line
This call initialises csa according to sysreg / iomem access requirements
csa is initialized only when no drvdata->base exists.
Not so - csa is initialised in both circumstances as described above.
Quoted 3 lines
Under what
circumstance would there be no ETM base given the recommended CoreSight ACPI implementation? See the examples in ARM Document number: DEN0067.
Quoted 5 lines
This will be used in the ETE devices (which share the etm4 driver), or any ETM4.6+ that uses the "arm,coresight-etm4x-sysreg" device tree binding (not sure what the ACPI equivalent is).
So, either way, you need an init csa, before passing it to the driver calls.
Agreed. See my summary above.
Quoted 6 lines
Later in the initialisation sequence we generate a coresight_device object which the csa is bound to, and finally if all is well the coresight_device is bound to drvdata at which point the device is ready for use.
Quoted 8 lines
It is unfortunate, but to handle the two methods of register access, the initilialisation process for the driver has become more complicated with ordering dependencies - to ensure that the rest of the driver remains simpler when accessing device registers.
As Suzuki mentioned - moving this specific lock requirement into the _init function would be clearer and ensure that the initialisation sequences were observed.

Quoted 6 lines

Getting back to the etm4_init_csdev_access implementation, if a drvdata->base exists, etm4_init_sysreg_access is never called. The driver doesn't initialize sysreg access if a memory map exists by design. The existing Coresight ACPI specification only has the option of describing the manufacturer trace implementation using descriptions of memory mapped components.

As mentioned in my previous comments, this is only a matter of updating the spec. You should be able to use everything same for the ETM, excluding the "MMIO" region.

i.e,

     Device(CPU0) {
        Name(_HID, "ACPI0010"),
        ...
        Device(ETM0) {
          Name (_HID, "ARMHC500")      // ETM
          Name (_CID, "ARMHC500")      // ETM

          /* No _CRS Memory Resource  for sysreg access */
	  /* Graph Connections as usual, if any ATB is connected */
	  Name (_DSD , Package () {
	  ...
	  })
          ...
        } // ETM0
    } // CPU0

Suzuki

Quoted 86 lines

Regards

Mike

Quoted 80 lines

Quoted 57 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +18 -4

+               if (drvdata->mmio_external)
+                       etm4_os_lock(drvdata);
                 return;
+       }
 
-       /* Detect the support for OS Lock before we actually use it */
-       etm_detect_os_lock(drvdata, csa);
+       /*
+        * Make sure all registers are accessible
+        * TRCOSLAR.OSLK may already be clear
+        */
+       if (!drvdata->mmio_external)
+               etm4_os_unlock_csa(drvdata, csa);
 
-       /* 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);

@@ -2027,6 +2042,14 @@ 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 considers MMIO access as external. This mask
+        * isolates the manufacturer JEP106 ID in the PID.
+        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
+        */
+       if ((init_arg.pid & 0x000FF000) == 0x00096000)
+               drvdata->mmio_external = true;
+
         /*
          * Serialize against CPUHP callbacks to avoid race condition
          * between the smp call and saving the delayed probe.

@@ -2192,6 +2215,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 */

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

index 4b21bb79f168..cf4f9f2e1807 100644
@@ -1015,6 +1015,7 @@ struct etmv4_save_state {
   * @skip_power_up: Indicates if an implementation can skip powering up
   *                the trace unit.
   * @arch_features: Bitmap of arch features of etmv4 devices.
+ * @mmio_external: True if ETM considers MMIO an external access.
   */
  struct etmv4_drvdata {
         void __iomem                    *base;

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x.h struct etmv4_drvdata { · line 1070
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x.h +0 -0
@@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
         bool                            state_needs_restore;
         bool                            skip_power_up;
         DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
Rather than continue to add bools - is it not worthwhile adding to the bitmap above and extending the arch features API to allow a "has_feature" call?

Quoted 8 lines

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

+       bool                            mmio_external;
  };
 
  /* Address comparator access types */
--

2.25.1

Quoted 6 lines

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Regards
Mike

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-20 00:54 UTC #21 lore raw

PATCH 2/3 coresight etm4x: Add 32-bit read/write option to split 64-bit words

In reply to Steve Clevenger

Add 32-bit read/write access option for Ampere ETMv4.6 64-bit registers. Ampere Computing erratum AC03_DEBUG_10 describes a design decision where 64-bit read/write access is not supported for the ETMv4.6 implementation. These 64-bit registers must be accessed as 2 ea. 32-bit registers. AC03_DEBUG_10 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Fix drvdata->nr_addr_cmp for() loop range bug to drvdata->nr_addr_cmp * 2 in etm_enable_hw.

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

---
 .../coresight/coresight-etm4x-core.c          | 81 ++++++++++++++-----
 drivers/hwtracing/coresight/coresight-etm4x.h | 32 ++++++++
 2 files changed, 93 insertions(+), 20 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x-core.c +61 ~~-20~~

index 533be1928a09..bf4daa649cdf 100644
@@ -452,18 +452,31 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 		if (etm4x_sspcicrn_present(drvdata, i))
 			etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i));
 	}
-	for (i = 0; i < drvdata->nr_addr_cmp; i++) {
-		etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));
-		etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+	for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
+		if (drvdata->no_quad_mmio) {
+			etm4x_split_write64(csa, config->addr_val[i], TRCACVRn(i));
+			etm4x_split_write64(csa, config->addr_acc[i], TRCACATRn(i));
+		} else {
+			etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));
+			etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+		}
+	}
+	for (i = 0; i < drvdata->numcidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
 	}
-	for (i = 0; i < drvdata->numcidc; i++)
-		etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
 	etm4x_relaxed_write32(csa, config->ctxid_mask0, TRCCIDCCTLR0);
 	if (drvdata->numcidc > 4)
 		etm4x_relaxed_write32(csa, config->ctxid_mask1, TRCCIDCCTLR1);
 
-	for (i = 0; i < drvdata->numvmidc; i++)
-		etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+	for (i = 0; i < drvdata->numvmidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+	}
 	etm4x_relaxed_write32(csa, config->vmid_mask0, TRCVMIDCCTLR0);
 	if (drvdata->numvmidc > 4)
 		etm4x_relaxed_write32(csa, config->vmid_mask1, TRCVMIDCCTLR1);

@@ -1670,8 +1683,13 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
 	}
 
 	for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-		state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
-		state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+		if (drvdata->no_quad_mmio) {
+			state->trcacvr[i] = etm4x_split_read64(csa, TRCACVRn(i));
+			state->trcacatr[i] = etm4x_split_read64(csa, TRCACATRn(i));
+		} else {
+			state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
+			state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+		}
 	}
 
 	/*

@@ -1681,11 +1699,19 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
 	 * unit") of ARM IHI 0064D.
 	 */
 
-	for (i = 0; i < drvdata->numcidc; i++)
-		state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+	for (i = 0; i < drvdata->numcidc; i++) {
+		if (drvdata->no_quad_mmio) 
+			state->trccidcvr[i] = etm4x_split_read64(csa, TRCCIDCVRn(i));
+		else
+			state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+	}
 
-	for (i = 0; i < drvdata->numvmidc; i++)
-		state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+	for (i = 0; i < drvdata->numvmidc; i++) {
+		if (drvdata->no_quad_mmio)
+			state->trcvmidcvr[i] = etm4x_split_read64(csa, TRCVMIDCVRn(i));
+		else
+			state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+	}
 
 	state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0);
 	if (drvdata->numcidc > 4)

@@ -1799,15 +1825,28 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 	}
 
 	for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-		etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
-		etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
+		if (drvdata->no_quad_mmio) {
+			etm4x_split_write64(csa, state->trcacvr[i], TRCACVRn(i));
+			etm4x_split_write64(csa, state->trcacatr[i], TRCACATRn(i));
+		} else {
+			etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
+			etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
+		}
 	}
 
-	for (i = 0; i < drvdata->numcidc; i++)
-		etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+	for (i = 0; i < drvdata->numcidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+	}
 
-	for (i = 0; i < drvdata->numvmidc; i++)
-		etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
+	for (i = 0; i < drvdata->numvmidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
+	}
 
 	etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0);
 	if (drvdata->numcidc > 4)

@@ -2047,8 +2086,10 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
 	 * isolates the manufacturer JEP106 ID in the PID.
 	 * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
 	 */
-	if ((init_arg.pid & 0x000FF000) == 0x00096000)
+	if ((init_arg.pid & 0x000FF000) == 0x00096000) {
 		drvdata->mmio_external = true;
+		drvdata->no_quad_mmio = true;
+	}
 
 	/*
 	 * Serialize against CPUHP callbacks to avoid race condition

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

index cf4f9f2e1807..0650bcdff410 100644
@@ -1016,6 +1016,7 @@ struct etmv4_save_state {
  *		   the trace unit.
  * @arch_features: Bitmap of arch features of etmv4 devices.
  * @mmio_external: True if ETM considers MMIO an external access.
+ * @no_quad_mmio:  True if ETM does not support 64-bit (quad) access.
  */
 struct etmv4_drvdata {
 	void __iomem			*base;

@@ -1069,6 +1070,7 @@ struct etmv4_drvdata {
 	bool				skip_power_up;
 	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
 	bool				mmio_external;
+	bool				no_quad_mmio;
 };
 
 /* Address comparator access types */

@@ -1093,6 +1095,36 @@ 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_split_read64(struct csdev_access *csa, unsigned int offset)
+{
+	struct etm_quad_split container;
+
+	container.lsw = etm4x_read32(csa, offset);
+	container.msw = etm4x_read32(csa, offset + sizeof(u32));
+
+	return *(u64 *) &container;
+}
+
+static inline void etm4x_split_write64(struct csdev_access *csa, u64 quad, unsigned int offset)
+{
+	struct etm_quad_split container;
+
+	*(u64 *) &container = quad;
+
+	etm4x_relaxed_write32(csa, container.lsw, offset);
+	etm4x_relaxed_write32(csa, container.msw, offset + sizeof(u32));
+}
+
 static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)
 {
 	return drvdata->arch >= ETM_ARCH_ETE;

Signature · 7 lines

2.25.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-20 11:20 UTC #22 lore raw

Re: [PATCH 2/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

Quoted 6 lines · Steve Clevenger

Add 32-bit read/write access option for Ampere ETMv4.6 64-bit registers. Ampere Computing erratum AC03_DEBUG_10 describes a design decision where 64-bit read/write access is not supported for the ETMv4.6 implementation. These 64-bit registers must be accessed as 2 ea. 32-bit registers. AC03_DEBUG_10 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

As with the previous comment, please :
a) If this is because of the system instruction access support
b) Document the erratum

Quoted 3 lines

Fix drvdata->nr_addr_cmp for() loop range bug to drvdata->nr_addr_cmp * 2 in etm_enable_hw.

Good catch ! Please separate this out and send it as a fix. I can queue this.

Quoted 17 lines

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

---
  .../coresight/coresight-etm4x-core.c          | 81 ++++++++++++++-----
  drivers/hwtracing/coresight/coresight-etm4x.h | 32 ++++++++
  2 files changed, 93 insertions(+), 20 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x-core.c +0 -2

index 533be1928a09..bf4daa649cdf 100644
@@ -452,18 +452,31 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
  		if (etm4x_sspcicrn_present(drvdata, i))
  			etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i));
  	}
-	for (i = 0; i < drvdata->nr_addr_cmp; i++) {
-		etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_enable_hw(struct etmv4_drvdata *drvdata) · line 463

Quoted 10 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +9 -1

-		etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+	for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
+		if (drvdata->no_quad_mmio) {
+			etm4x_split_write64(csa, config->addr_val[i], TRCACVRn(i));
+			etm4x_split_write64(csa, config->addr_acc[i], TRCACATRn(i));
+		} else {
+			etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));
+			etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+		}
+	}

Something like this can be leave some places out. I think we could hide it under the generic helpers and handle it there. May be "struct csdev_access" can cache this "no_quad_mmio" and do the right thing ?

Quoted 142 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +52 ~~-17~~

+	for (i = 0; i < drvdata->numcidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
  	}
-	for (i = 0; i < drvdata->numcidc; i++)
-		etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
  	etm4x_relaxed_write32(csa, config->ctxid_mask0, TRCCIDCCTLR0);
  	if (drvdata->numcidc > 4)
  		etm4x_relaxed_write32(csa, config->ctxid_mask1, TRCCIDCCTLR1);
 
-	for (i = 0; i < drvdata->numvmidc; i++)
-		etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+	for (i = 0; i < drvdata->numvmidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+	}
  	etm4x_relaxed_write32(csa, config->vmid_mask0, TRCVMIDCCTLR0);
  	if (drvdata->numvmidc > 4)
  		etm4x_relaxed_write32(csa, config->vmid_mask1, TRCVMIDCCTLR1);

@@ -1670,8 +1683,13 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
  	}
 
  	for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-		state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
-		state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+		if (drvdata->no_quad_mmio) {
+			state->trcacvr[i] = etm4x_split_read64(csa, TRCACVRn(i));
+			state->trcacatr[i] = etm4x_split_read64(csa, TRCACATRn(i));
+		} else {
+			state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
+			state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+		}
  	}
 
  	/*

@@ -1681,11 +1699,19 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
  	 * unit") of ARM IHI 0064D.
  	 */
 
-	for (i = 0; i < drvdata->numcidc; i++)
-		state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+	for (i = 0; i < drvdata->numcidc; i++) {
+		if (drvdata->no_quad_mmio)
+			state->trccidcvr[i] = etm4x_split_read64(csa, TRCCIDCVRn(i));
+		else
+			state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+	}
 
-	for (i = 0; i < drvdata->numvmidc; i++)
-		state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+	for (i = 0; i < drvdata->numvmidc; i++) {
+		if (drvdata->no_quad_mmio)
+			state->trcvmidcvr[i] = etm4x_split_read64(csa, TRCVMIDCVRn(i));
+		else
+			state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+	}
 
  	state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0);
  	if (drvdata->numcidc > 4)

@@ -1799,15 +1825,28 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
  	}
 
  	for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-		etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
-		etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
+		if (drvdata->no_quad_mmio) {
+			etm4x_split_write64(csa, state->trcacvr[i], TRCACVRn(i));
+			etm4x_split_write64(csa, state->trcacatr[i], TRCACATRn(i));
+		} else {
+			etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
+			etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
+		}
  	}
 
-	for (i = 0; i < drvdata->numcidc; i++)
-		etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+	for (i = 0; i < drvdata->numcidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+	}
 
-	for (i = 0; i < drvdata->numvmidc; i++)
-		etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
+	for (i = 0; i < drvdata->numvmidc; i++) {
+		if (drvdata->no_quad_mmio)
+			etm4x_split_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
+		else
+			etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
+	}
 
  	etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0);
  	if (drvdata->numcidc > 4)

@@ -2047,8 +2086,10 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
  	 * isolates the manufacturer JEP106 ID in the PID.
  	 * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
  	 */
-	if ((init_arg.pid & 0x000FF000) == 0x00096000)
+	if ((init_arg.pid & 0x000FF000) == 0x00096000) {
  		drvdata->mmio_external = true;
+		drvdata->no_quad_mmio = true;
+	}
 
  	/*
  	 * Serialize against CPUHP callbacks to avoid race condition

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

index cf4f9f2e1807..0650bcdff410 100644
@@ -1016,6 +1016,7 @@ struct etmv4_save_state {
   *		   the trace unit.
   * @arch_features: Bitmap of arch features of etmv4 devices.
   * @mmio_external: True if ETM considers MMIO an external access.
+ * @no_quad_mmio:  True if ETM does not support 64-bit (quad) access.
   */
  struct etmv4_drvdata {
  	void __iomem			*base;

@@ -1069,6 +1070,7 @@ struct etmv4_drvdata {
  	bool				skip_power_up;
  	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
  	bool				mmio_external;
+	bool				no_quad_mmio;
  };
 
  /* Address comparator access types */

@@ -1093,6 +1095,36 @@ 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;
+};
+

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

Quoted 10 lines

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

+#pragma pack(pop)
+
+static inline u64 etm4x_split_read64(struct csdev_access *csa, unsigned int offset)
+{
+	struct etm_quad_split container;
+
+	container.lsw = etm4x_read32(csa, offset);
+	container.msw = etm4x_read32(csa, offset + sizeof(u32));
+
+	return *(u64 *) &container;

Wouldn't this break with the "endianness" flip ? (Not that we have BE implementations). Could we not combine the two values to a 64bit value and pass that instead ?

Similarly below.

Suzuki

Quoted 5 lines

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

+}
+
+static inline void etm4x_split_write64(struct csdev_access *csa, u64 quad, unsigned int offset)
+{
+	struct etm_quad_split container;

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

Quoted 10 lines

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

+
+	*(u64 *) &container = quad;
+
+	etm4x_relaxed_write32(csa, container.lsw, offset);
+	etm4x_relaxed_write32(csa, container.msw, offset + sizeof(u32));
+}
+
  static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)
  {
  	return drvdata->arch >= ETM_ARCH_ETE;

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-22 08:34 UTC #23 lore raw

Re: [PATCH 2/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

Hi Suzuki,

Comments in-line.

Steve

Quoted 8 lines · Suzuki K Poulose

Quoted 6 lines · Steve Clevenger
Add 32-bit read/write access option for Ampere ETMv4.6 64-bit registers. Ampere Computing erratum AC03_DEBUG_10 describes a design decision where 64-bit read/write access is not supported for the ETMv4.6 implementation. These 64-bit registers must be accessed as 2 ea. 32-bit registers. AC03_DEBUG_10 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Quoted 4 lines

As with the previous comment, please :
a) If this is because of the system instruction access support
b) Document the erratum

I presume you're referring to your previous comment about adding these errata to "Documentation/arm64/silicon-errata.rst". Let me see if there's any heartburn with this internal to Ampere. I don't expect there to be.

Quoted 6 lines

Quoted 3 lines
Fix drvdata->nr_addr_cmp for() loop range bug to drvdata->nr_addr_cmp * 2 in etm_enable_hw.
Good catch ! Please separate this out and send it as a fix. I can queue this.

I'll submit it as a separate patch.

Quoted 29 lines

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

---
.../coresight/coresight-etm4x-core.c | 81 ++++++++++++++----- drivers/hwtracing/coresight/coresight-etm4x.h | 32 ++++++++

  2 files changed, 93 insertions(+), 20 deletions(-)

drivers/hwtracing/coresight/coresight-etm4x-core.c +6 -3

index 533be1928a09..bf4daa649cdf 100644
@@ -452,18 +452,31 @@ static int etm4_enable_hw(struct etmv4_drvdata
 *drvdata)
           if (etm4x_sspcicrn_present(drvdata, i))
               etm4x_relaxed_write32(csa, config->ss_pe_cmp[i],
 TRCSSPCICRn(i));
       }
-    for (i = 0; i < drvdata->nr_addr_cmp; i++) {
-        etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));
-        etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+    for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
+        if (drvdata->no_quad_mmio) {
+            etm4x_split_write64(csa, config->addr_val[i], TRCACVRn(i));
+            etm4x_split_write64(csa, config->addr_acc[i], TRCACATRn(i));
+        } else {
+            etm4x_relaxed_write64(csa, config->addr_val[i],
 TRCACVRn(i));

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_enable_hw(struct etmv4_drvdata *drvdata) · line 463
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +3 -0
+            etm4x_relaxed_write64(csa, config->addr_acc[i],
 TRCACATRn(i));
+        }
+    }
Something like this can be leave some places out. I think we could hide it under the generic helpers and handle it there. May be "struct csdev_access" can cache this "no_quad_mmio" and do the right thing ?

I'm not sure what you're suggesting here. Please be more specific.

Quoted 157 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +5 -2

+    for (i = 0; i < drvdata->numcidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, config->ctxid_pid[i],
 TRCCIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, config->ctxid_pid[i],
 TRCCIDCVRn(i));
       }
-    for (i = 0; i < drvdata->numcidc; i++)
-        etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
       etm4x_relaxed_write32(csa, config->ctxid_mask0, TRCCIDCCTLR0);
       if (drvdata->numcidc > 4)
           etm4x_relaxed_write32(csa, config->ctxid_mask1, TRCCIDCCTLR1);

-    for (i = 0; i < drvdata->numvmidc; i++)
-        etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i)); +    for (i = 0; i < drvdata->numvmidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+    }
      etm4x_relaxed_write32(csa, config->vmid_mask0, TRCVMIDCCTLR0); if (drvdata->numvmidc > 4)
          etm4x_relaxed_write32(csa, config->vmid_mask1, TRCVMIDCCTLR1);

drivers/hwtracing/coresight/coresight-etm4x-core.c +19 -4

@@ -1670,8 +1683,13 @@ static int __etm4_cpu_save(struct etmv4_drvdata
 *drvdata)
       }
         for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-        state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
-        state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+        if (drvdata->no_quad_mmio) {
+            state->trcacvr[i] = etm4x_split_read64(csa, TRCACVRn(i));
+            state->trcacatr[i] = etm4x_split_read64(csa, TRCACATRn(i));
+        } else {
+            state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
+            state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+        }
       }
         /*

@@ -1681,11 +1699,19 @@ static int __etm4_cpu_save(struct
 etmv4_drvdata *drvdata)
        * unit") of ARM IHI 0064D.
        */
   -    for (i = 0; i < drvdata->numcidc; i++)
-        state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+    for (i = 0; i < drvdata->numcidc; i++) {
+        if (drvdata->no_quad_mmio)
+            state->trccidcvr[i] = etm4x_split_read64(csa,
 TRCCIDCVRn(i));
+        else
+            state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+    }
   -    for (i = 0; i < drvdata->numvmidc; i++)
-        state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+    for (i = 0; i < drvdata->numvmidc; i++) {
+        if (drvdata->no_quad_mmio)
+            state->trcvmidcvr[i] = etm4x_split_read64(csa,
 TRCVMIDCVRn(i));
+        else
+            state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+    }

state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0); if (drvdata->numcidc > 4)

drivers/hwtracing/coresight/coresight-etm4x-core.c +18 -4

@@ -1799,15 +1825,28 @@ static void __etm4_cpu_restore(struct
 etmv4_drvdata *drvdata)
       }
         for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-        etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
-        etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
+        if (drvdata->no_quad_mmio) {
+            etm4x_split_write64(csa, state->trcacvr[i], TRCACVRn(i));
+            etm4x_split_write64(csa, state->trcacatr[i], TRCACATRn(i));
+        } else {
+            etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
+            etm4x_relaxed_write64(csa, state->trcacatr[i],
 TRCACATRn(i));
+        }
       }
   -    for (i = 0; i < drvdata->numcidc; i++)
-        etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+    for (i = 0; i < drvdata->numcidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, state->trccidcvr[i],
 TRCCIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, state->trccidcvr[i],
 TRCCIDCVRn(i));
+    }
   -    for (i = 0; i < drvdata->numvmidc; i++)
-        etm4x_relaxed_write64(csa, state->trcvmidcvr[i],
 TRCVMIDCVRn(i));
+    for (i = 0; i < drvdata->numvmidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, state->trcvmidcvr[i],
 TRCVMIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, state->trcvmidcvr[i],

TRCVMIDCVRn(i));
+ }
etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0); if (drvdata->numcidc > 4)

drivers/hwtracing/coresight/coresight-etm4x-core.c +3 -1

@@ -2047,8 +2086,10 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
        * isolates the manufacturer JEP106 ID in the PID.
        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
        */
-    if ((init_arg.pid & 0x000FF000) == 0x00096000)
+    if ((init_arg.pid & 0x000FF000) == 0x00096000) {
           drvdata->mmio_external = true;
+        drvdata->no_quad_mmio = true;
+    }
         /*
        * Serialize against CPUHP callbacks to avoid race condition

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

index cf4f9f2e1807..0650bcdff410 100644
@@ -1016,6 +1016,7 @@ struct etmv4_save_state {
    *           the trace unit.
    * @arch_features: Bitmap of arch features of etmv4 devices.
    * @mmio_external: True if ETM considers MMIO an external access.
+ * @no_quad_mmio:  True if ETM does not support 64-bit (quad) access.
    */
   struct etmv4_drvdata {
       void __iomem            *base;

@@ -1069,6 +1070,7 @@ struct etmv4_drvdata {
       bool                skip_power_up;
       DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
       bool                mmio_external;
+    bool                no_quad_mmio;
   };
     /* Address comparator access types */

@@ -1093,6 +1095,36 @@ 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 */

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

+#pragma pack(push, 8)
+
+struct etm_quad_split {
+    u32 lsw;
+    u32 msw;
+};
+
+#pragma pack(pop)
+
+static inline u64 etm4x_split_read64(struct csdev_access *csa,

unsigned int offset)

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

+{
+    struct etm_quad_split container;
+

Quoted 8 lines

Quoted 4 lines
+    container.lsw = etm4x_read32(csa, offset);
+    container.msw = etm4x_read32(csa, offset + sizeof(u32)); +
+    return *(u64 *) &container;
Wouldn't this break with the "endianness" flip ? (Not that we have BE implementations). Could we not combine the two values to a 64bit value and pass that instead ?

The split implementation writes/reads 32-bit words to/from 2 consecutive 32-bit aligned memory addresses independent of endianness so it doesn't care. I'm not sure I understand what you're getting at by combining the 2 ea. 32-bit values into a 1 ea. 64-bit value. The etm4x_split_read64 and etm4x_split_write64 calls both use 64-bit values in and out. Internal to this code, both read and write accesses must use 32-bit values.

Quoted 5 lines

Similarly below.
Suzuki

Quoted 17 lines

Quoted 13 lines
drivers/hwtracing/coresight/coresight-etm4x.h +3 -0
+}
+
+static inline void etm4x_split_write64(struct csdev_access *csa, u64
quad, unsigned int offset)
drivers/hwtracing/coresight/coresight-etm4x.h +9 -0
+{
+    struct etm_quad_split container;
+
+    *(u64 *) &container = quad;
+
+    etm4x_relaxed_write32(csa, container.lsw, offset);
+    etm4x_relaxed_write32(csa, container.msw, offset + sizeof(u32));
+}
+
Quoted 3 lines
static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata) {
return drvdata->arch >= ETM_ARCH_ETE;

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-23 18:00 UTC #24 lore raw

Re: [PATCH 2/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

Quoted 70 lines · Steve Clevenger

Hi Suzuki,
Comments in-line.
Steve
Quoted 8 lines · Suzuki K Poulose
Quoted 6 lines · Steve Clevenger
Add 32-bit read/write access option for Ampere ETMv4.6 64-bit registers. Ampere Computing erratum AC03_DEBUG_10 describes a design decision where 64-bit read/write access is not supported for the ETMv4.6 implementation. These 64-bit registers must be accessed as 2 ea. 32-bit registers. AC03_DEBUG_10 is described in the AmpereOne Developer Errata:
https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
Quoted 4 lines
As with the previous comment, please :
a) If this is because of the system instruction access support
b) Document the erratum
I presume you're referring to your previous comment about adding these errata to "Documentation/arm64/silicon-errata.rst". Let me see if there's any heartburn with this internal to Ampere. I don't expect there to be.
Quoted 6 lines
Quoted 3 lines
Fix drvdata->nr_addr_cmp for() loop range bug to drvdata->nr_addr_cmp * 2 in etm_enable_hw.
Good catch ! Please separate this out and send it as a fix. I can queue this.
I'll submit it as a separate patch.
Quoted 29 lines
Quoted 29 lines
Signed-off-by Steve Clevenger <[email protected]>
---
   .../coresight/coresight-etm4x-core.c          | 81 ++++++++++++++-----
   drivers/hwtracing/coresight/coresight-etm4x.h | 32 ++++++++
   2 files changed, 93 insertions(+), 20 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +6 -3
index 533be1928a09..bf4daa649cdf 100644
@@ -452,18 +452,31 @@ static int etm4_enable_hw(struct etmv4_drvdata
 *drvdata)
           if (etm4x_sspcicrn_present(drvdata, i))
               etm4x_relaxed_write32(csa, config->ss_pe_cmp[i],
 TRCSSPCICRn(i));
       }
-    for (i = 0; i < drvdata->nr_addr_cmp; i++) {
-        etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));
-        etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+    for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
+        if (drvdata->no_quad_mmio) {
+            etm4x_split_write64(csa, config->addr_val[i], TRCACVRn(i));
+            etm4x_split_write64(csa, config->addr_acc[i], TRCACATRn(i));
+        } else {
+            etm4x_relaxed_write64(csa, config->addr_val[i],
 TRCACVRn(i));
Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_enable_hw(struct etmv4_drvdata *drvdata) · line 463
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +3 -0
+            etm4x_relaxed_write64(csa, config->addr_acc[i],
 TRCACATRn(i));
+        }
+    }
Something like this can be leave some places out. I think we could hide it under the generic helpers and handle it there. May be "struct csdev_access" can cache this "no_quad_mmio" and do the right thing ?

Quoted 2 lines

I'm not sure what you're suggesting here. Please be more specific.

e.g.,

struct csdev_access {

	bool no_64bit_access;
}

And use the csdev_*_ operations could :

if (csa->no_64bit_access) {
split access
} else {

}

i.e., move the tracking of no_quad_mmio to "csa" from "drvdata"

Suzuki

Quoted 165 lines

Quoted 157 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c +8 -3

+    for (i = 0; i < drvdata->numcidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, config->ctxid_pid[i],
 TRCCIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, config->ctxid_pid[i],
 TRCCIDCVRn(i));
       }
-    for (i = 0; i < drvdata->numcidc; i++)
-        etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i));
       etm4x_relaxed_write32(csa, config->ctxid_mask0, TRCCIDCCTLR0);
       if (drvdata->numcidc > 4)
           etm4x_relaxed_write32(csa, config->ctxid_mask1, TRCCIDCCTLR1);
   -    for (i = 0; i < drvdata->numvmidc; i++)
-        etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+    for (i = 0; i < drvdata->numvmidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, config->vmid_val[i],

TRCVMIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i));
+    }
      etm4x_relaxed_write32(csa, config->vmid_mask0, TRCVMIDCCTLR0); if (drvdata->numvmidc > 4)
          etm4x_relaxed_write32(csa, config->vmid_mask1, TRCVMIDCCTLR1);

drivers/hwtracing/coresight/coresight-etm4x-core.c +37 -8

@@ -1670,8 +1683,13 @@ static int __etm4_cpu_save(struct etmv4_drvdata
 *drvdata)
       }
         for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-        state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
-        state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+        if (drvdata->no_quad_mmio) {
+            state->trcacvr[i] = etm4x_split_read64(csa, TRCACVRn(i));
+            state->trcacatr[i] = etm4x_split_read64(csa, TRCACATRn(i));
+        } else {
+            state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
+            state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
+        }
       }
         /*

@@ -1681,11 +1699,19 @@ static int __etm4_cpu_save(struct
 etmv4_drvdata *drvdata)
        * unit") of ARM IHI 0064D.
        */
   -    for (i = 0; i < drvdata->numcidc; i++)
-        state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+    for (i = 0; i < drvdata->numcidc; i++) {
+        if (drvdata->no_quad_mmio)
+            state->trccidcvr[i] = etm4x_split_read64(csa,
 TRCCIDCVRn(i));
+        else
+            state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
+    }
   -    for (i = 0; i < drvdata->numvmidc; i++)
-        state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+    for (i = 0; i < drvdata->numvmidc; i++) {
+        if (drvdata->no_quad_mmio)
+            state->trcvmidcvr[i] = etm4x_split_read64(csa,
 TRCVMIDCVRn(i));
+        else
+            state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
+    }
         state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0);
       if (drvdata->numcidc > 4)

@@ -1799,15 +1825,28 @@ static void __etm4_cpu_restore(struct
 etmv4_drvdata *drvdata)
       }
         for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
-        etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
-        etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
+        if (drvdata->no_quad_mmio) {
+            etm4x_split_write64(csa, state->trcacvr[i], TRCACVRn(i));
+            etm4x_split_write64(csa, state->trcacatr[i], TRCACATRn(i));
+        } else {
+            etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
+            etm4x_relaxed_write64(csa, state->trcacatr[i],
 TRCACATRn(i));
+        }
       }
   -    for (i = 0; i < drvdata->numcidc; i++)
-        etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
+    for (i = 0; i < drvdata->numcidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, state->trccidcvr[i],
 TRCCIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, state->trccidcvr[i],
 TRCCIDCVRn(i));
+    }
   -    for (i = 0; i < drvdata->numvmidc; i++)
-        etm4x_relaxed_write64(csa, state->trcvmidcvr[i],
 TRCVMIDCVRn(i));
+    for (i = 0; i < drvdata->numvmidc; i++) {
+        if (drvdata->no_quad_mmio)
+            etm4x_split_write64(csa, state->trcvmidcvr[i],
 TRCVMIDCVRn(i));
+        else
+            etm4x_relaxed_write64(csa, state->trcvmidcvr[i],

TRCVMIDCVRn(i));
+ }
etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0); if (drvdata->numcidc > 4)

drivers/hwtracing/coresight/coresight-etm4x-core.c +3 -1

@@ -2047,8 +2086,10 @@ static int etm4_probe(struct device *dev, void
 __iomem *base, u32 etm_pid)
        * isolates the manufacturer JEP106 ID in the PID.
        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
        */
-    if ((init_arg.pid & 0x000FF000) == 0x00096000)
+    if ((init_arg.pid & 0x000FF000) == 0x00096000) {
           drvdata->mmio_external = true;
+        drvdata->no_quad_mmio = true;
+    }
         /*
        * Serialize against CPUHP callbacks to avoid race condition

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

index cf4f9f2e1807..0650bcdff410 100644
@@ -1016,6 +1016,7 @@ struct etmv4_save_state {
    *           the trace unit.
    * @arch_features: Bitmap of arch features of etmv4 devices.
    * @mmio_external: True if ETM considers MMIO an external access.
+ * @no_quad_mmio:  True if ETM does not support 64-bit (quad) access.
    */
   struct etmv4_drvdata {
       void __iomem            *base;

@@ -1069,6 +1070,7 @@ struct etmv4_drvdata {
       bool                skip_power_up;
       DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
       bool                mmio_external;
+    bool                no_quad_mmio;
   };
     /* Address comparator access types */

@@ -1093,6 +1095,36 @@ 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_split_read64(struct csdev_access *csa,
 unsigned int offset)
+{
+    struct etm_quad_split container;
+

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x.h void etm4_config_trace_mode(struct etmv4_config *config); · line 1115
Quoted 4 lines
drivers/hwtracing/coresight/coresight-etm4x.h +4 -0
+    container.lsw = etm4x_read32(csa, offset);
+    container.msw = etm4x_read32(csa, offset + sizeof(u32));
+
+    return *(u64 *) &container;
Wouldn't this break with the "endianness" flip ? (Not that we have BE implementations). Could we not combine the two values to a 64bit value and pass that instead ?

Quoted 29 lines

The split implementation writes/reads 32-bit words to/from 2 consecutive 32-bit aligned memory addresses independent of endianness so it doesn't care. I'm not sure I understand what you're getting at by combining the 2 ea. 32-bit values into a 1 ea. 64-bit value. The etm4x_split_read64 and etm4x_split_write64 calls both use 64-bit values in and out. Internal to this code, both read and write accesses must use 32-bit values.
Quoted 22 lines
Similarly below.
Suzuki
Quoted 16 lines
drivers/hwtracing/coresight/coresight-etm4x.h +3 -0
+}
+
+static inline void etm4x_split_write64(struct csdev_access *csa, u64
quad, unsigned int offset)
drivers/hwtracing/coresight/coresight-etm4x.h +9 -0
+{
+    struct etm_quad_split container;
+
+    *(u64 *) &container = quad;
+
+    etm4x_relaxed_write32(csa, container.lsw, offset);
+    etm4x_relaxed_write32(csa, container.msw, offset + sizeof(u32));
+}
+
   static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)
   {
       return drvdata->arch >= ETM_ARCH_ETE;

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

James Clark

2023-03-06 10:38 UTC #25 lore raw

Re: [PATCH 2/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

In reply to Suzuki K Poulose

Quoted 1 line · Suzuki K Poulose

On 20/01/2023 00:51, Steve Clevenger wrote:

[...]

Quoted 16 lines

Quoted 7 lines
      }
-    for (i = 0; i < drvdata->nr_addr_cmp; i++) {
-        etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i)); -        etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i)); +    for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
+        if (drvdata->no_quad_mmio) {
+            etm4x_split_write64(csa, config->addr_val[i], TRCACVRn(i));
Quoted 8 lines
+            etm4x_split_write64(csa, config->addr_acc[i], TRCACATRn(i)); +        } else {
+            etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));
+            etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+        }
+    }

Quoted 3 lines

Something like this can be leave some places out. I think we could hide it under the generic helpers and handle it there. May be "struct csdev_access" can cache this "no_quad_mmio" and do the right thing ?

+1 for this, or just pass drvdata to etm4x_relaxed_write64() and then it can decide what to do. I'd prefer that to caching the value in csdev_access because it would just be a copy of some other value and might go stale or not be set at some point.

James

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-03-07 01:25 UTC #26 lore raw

Re: [PATCH 2/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

Hi James,

Thanks for the feedback. I did hide the split 64-bit implementation under the generic helpers. Note these helpers are now static inline code instead of macro implementations. This significantly reduced the number of changes where the etm4x_relaxed write64 and etm4x_relaxed_read64 calls were used.

Steve

Quoted 25 lines · James Clark

Quoted 1 line · Suzuki K Poulose
On 20/01/2023 00:51, Steve Clevenger wrote:
[...]
Quoted 16 lines
Quoted 7 lines
      }
-    for (i = 0; i < drvdata->nr_addr_cmp; i++) {
-        etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i)); -        etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i)); +    for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
+        if (drvdata->no_quad_mmio) {
+            etm4x_split_write64(csa, config->addr_val[i], TRCACVRn(i));
Quoted 8 lines
+            etm4x_split_write64(csa, config->addr_acc[i], TRCACATRn(i)); +        } else {
+            etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i));
+            etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i));
+        }
+    }
Quoted 3 lines
Something like this can be leave some places out. I think we could hide it under the generic helpers and handle it there. May be "struct csdev_access" can cache this "no_quad_mmio" and do the right thing ?

Quoted 6 lines

+1 for this, or just pass drvdata to etm4x_relaxed_write64() and then it can decide what to do. I'd prefer that to caching the value in csdev_access because it would just be a copy of some other value and might go stale or not be set at some point.
James

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-20 00:54 UTC #27 lore raw

PATCH 3/3 coresight etm4x: Add pr_debug statement for Coresight component PID/CID

In reply to Steve Clevenger

Add pr_debug statement to provide visibility into Coresight component PID and CID settings. This helped debug an intermittent clock related issue resulting in bad PID/CID values.

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

---
 drivers/amba/bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/amba/bus.c +7 -0

index ff7454a38058..7c432442862c 100644
@@ -136,6 +136,7 @@ static int amba_read_periphid(struct amba_device *dev)
 	u32 size, pid, cid;
 	void __iomem *tmp;
 	int i, ret;
+	u32 cid_addr, pid_addr;
 
 	ret = dev_pm_domain_attach(&dev->dev, true);
 	if (ret) {

@@ -178,6 +179,12 @@ static int amba_read_periphid(struct amba_device *dev)
 	for (cid = 0, i = 0; i < 4; i++)
 		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
 
+	/* physical address as meaningful */
+	cid_addr = (u64)(dev->res.start + size - 0x20);
+	pid_addr = (u64)(dev->res.start + size - 0x10);
+
+	pr_debug("pid  (%llX): %08X  cid  (%llX): %08X\n", pid_addr, pid, cid_addr, cid);
+
 	if (cid == CORESIGHT_CID) {
 		/* set the base to the start of the last 4k block */
 		void __iomem *csbase = tmp + size - 4096;

Signature · 7 lines

2.25.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

maintainer

2023-01-20 11:25 UTC #28 lore raw

Re: [PATCH 3/3] coresight etm4x: Add pr_debug statement for Coresight component PID/CID

Cc Russell

nit: Subject line doesn't match the patch. This could be :

"amba: bus: Add pr_debug for AMBA PID/CID"

Quoted 3 lines · Steve Clevenger

Add pr_debug statement to provide visibility into Coresight component PID and CID settings. This helped debug an intermittent clock related issue resulting in bad PID/CID values.

And this change belongs to the AMBA subsystem. Please run :

scripts/get_maintainer.pl on your patch and add the necessary people from that list for your patch.

As such, I don't think brings any value to be added to the tree. I will leave it for the maintainers to comment.

Suzuki

Quoted 21 lines

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

---
  drivers/amba/bus.c | 7 +++++++
  1 file changed, 7 insertions(+)

drivers/amba/bus.c +1 -0

index ff7454a38058..7c432442862c 100644
@@ -136,6 +136,7 @@ static int amba_read_periphid(struct amba_device *dev)
  	u32 size, pid, cid;
  	void __iomem *tmp;
  	int i, ret;
+	u32 cid_addr, pid_addr;
 
  	ret = dev_pm_domain_attach(&dev->dev, true);
  	if (ret) {

@@ -178,6 +179,12 @@ static int amba_read_periphid(struct amba_device *dev)
  	for (cid = 0, i = 0; i < 4; i++)
  		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);

drivers/amba/bus.c static int amba_read_periphid(struct amba_device *dev) · line 190

Quoted 10 lines

drivers/amba/bus.c +6 -0

 
+	/* physical address as meaningful */
+	cid_addr = (u64)(dev->res.start + size - 0x20);
+	pid_addr = (u64)(dev->res.start + size - 0x10);
+
+	pr_debug("pid  (%llX): %08X  cid  (%llX): %08X\n", pid_addr, pid, cid_addr, cid);
+
  	if (cid == CORESIGHT_CID) {
  		/* set the base to the start of the last 4k block */
  		void __iomem *csbase = tmp + size - 4096;

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Russell King (Oracle)

maintainer

2023-01-20 12:42 UTC #29 lore raw

Re: [PATCH 3/3] coresight etm4x: Add pr_debug statement for Coresight component PID/CID

Quoted 12 lines · Suzuki K Poulose

Cc Russell
nit: Subject line doesn't match the patch. This could be :
"amba: bus: Add pr_debug for AMBA PID/CID"
Quoted 3 lines · Steve Clevenger
Add pr_debug statement to provide visibility into Coresight component PID and CID settings. This helped debug an intermittent clock related issue resulting in bad PID/CID values.

Quoted 7 lines

And this change belongs to the AMBA subsystem. Please run :
scripts/get_maintainer.pl on your patch and add the necessary people from that list for your patch.
As such, I don't think brings any value to be added to the tree. I will leave it for the maintainers to comment.

Looking at the context in this patch, I see code that is reading at least the CID but likely also the PID from the device, duplicating the code that is already in the bus layer, and stored in the amba_device's periphid and cid members...

Signature · 7 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Steve Clevenger

author

2023-01-21 07:32 UTC #30 lore raw

Re: [PATCH 3/3] coresight etm4x: Add pr_debug statement for Coresight component PID/CID

Hi Russell,

Comments inline.

Steve

Quoted 18 lines · Russell King (Oracle)

Quoted 17 lines · Suzuki K Poulose
Cc Russell
nit: Subject line doesn't match the patch. This could be :
"amba: bus: Add pr_debug for AMBA PID/CID"
Quoted 3 lines · Steve Clevenger
Add pr_debug statement to provide visibility into Coresight component PID and CID settings. This helped debug an intermittent clock related issue resulting in bad PID/CID values.
And this change belongs to the AMBA subsystem. Please run :
scripts/get_maintainer.pl on your patch and add the necessary people from that list for your patch.

Quoted 8 lines

Quoted 2 lines
As such, I don't think brings any value to be added to the tree. I will leave it for the maintainers to comment.
Looking at the context in this patch, I see code that is reading at least the CID but likely also the PID from the device, duplicating the code that is already in the bus layer, and stored in the amba_device's periphid and cid members...

The idea here was to capture and report an intermittent Ampere PID/CID read error at the first read. It's not going to disturb me if this patch is not considered value added, but it served its' purpose in this case so I thought it might be useful for others to turn on under debug. I can resubmit the patch for AMBA, or not.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel