diff mbox series

[15/19] coresight: etm4x: Use TRCDEVARCH for component discovery

Message ID 20200911084119.1080694-16-suzuki.poulose@arm.com (mailing list archive)
State New, archived
Headers show
Series coresight: Support for ETMv4.4 system instructions | expand

Commit Message

Suzuki K Poulose Sept. 11, 2020, 8:41 a.m. UTC 
We have been using TRCIDR1 for detecting the ETM version. As
we are about to discover the trace unit on a CPU, let us use a
CoreSight architected register, instead of an ETM architected
register for accurate detection on a CPU. e.g, a CPU might
implement a custom trace unit, not an ETM.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x.c | 50 ++++++++++---------
 drivers/hwtracing/coresight/coresight-etm4x.h |  4 +-
 2 files changed, 29 insertions(+), 25 deletions(-)

Comments

Mike Leach Sept. 18, 2020, 3:35 p.m. UTC | #1 
Hi Suzuki

On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> We have been using TRCIDR1 for detecting the ETM version. As
> we are about to discover the trace unit on a CPU, let us use a
> CoreSight architected register, instead of an ETM architected
> register for accurate detection on a CPU. e.g, a CPU might
> implement a custom trace unit, not an ETM.
>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 50 ++++++++++---------
>  drivers/hwtracing/coresight/coresight-etm4x.h |  4 +-
>  2 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 5880f105268f..0fce9fb12cff 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -61,6 +61,17 @@ struct etm_init_arg {
>         struct csdev_access     *csa;
>  };
>
> +static inline u8 etm_devarch_arch(u32 devarch)
> +{
> +       return (devarch >> ETM_DEVARCH_ARCHID_ARCH_VER_SHIFT) & 0xfUL;
> +}
> +
> +static inline u8 etm_devarch_rev(u32 devarch)
> +{
> +       return (devarch & ETM_DEVARCH_REVISION_MASK) >>
> +               ETM_DEVARCH_REVISION_SHIFT;
> +}
> +
>  u64 etm4x_sysreg_read(struct csdev_access *csa,
>                       u32 offset,
>                       bool _relaxed,
> @@ -149,18 +160,6 @@ static void etm4_cs_unlock(struct etmv4_drvdata *drvdata,
>                 CS_UNLOCK(csa);
>  }
>
> -static bool etm4_arch_supported(u8 arch)
> -{
> -       /* Mask out the minor version number */
> -       switch (arch & 0xf0) {
> -       case ETM_ARCH_V4:
> -               break;
> -       default:
> -               return false;
> -       }
> -       return true;
> -}
> -
>  static int etm4_cpu_id(struct coresight_device *csdev)
>  {
>         struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> @@ -694,10 +693,23 @@ static void etm_detect_lock_status(struct etmv4_drvdata *drvdata,
>         drvdata->os_lock_model = TRCOSLSR_OSM(os_lsr);
>  }
>
> +static inline bool trace_unit_supported(u32 devarch)
> +{
> +       return (devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH;
> +}
> +

This is fine for system reg access - but imposes additional
constraints on memory mapped devices that previously may have worked
just by matching CID/PID. Can you be certain there are no devices out
there that have omitted this register (it does have a present bit
after all)

>  static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
>                                   struct csdev_access *csa)
>  {
> +       u32 devarch;
> +
> +       devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
> +       if (!trace_unit_supported(devarch))
> +               return false;
> +
>         *csa = CSDEV_ACCESS_IOMEM(drvdata->base);
> +       drvdata->arch = devarch;
> +
>         return true;
>  }
>
> @@ -713,7 +725,6 @@ static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
>  static void etm4_init_arch_data(void *info)

The primary task of this routine is to read all the ID registers and
set up the data in regards to resources etc.
We should not be mixing in a secondary task of detection of a valid device.

>  {
>         u32 etmidr0;
> -       u32 etmidr1;
>         u32 etmidr2;
>         u32 etmidr3;
>         u32 etmidr4;
> @@ -784,14 +795,6 @@ static void etm4_init_arch_data(void *info)
>         /* TSSIZE, bits[28:24] Global timestamp size field */
>         drvdata->ts_size = BMVAL(etmidr0, 24, 28);
>
> -       /* base architecture of trace unit */
> -       etmidr1 = etm4x_relaxed_read32(csa, TRCIDR1);
> -       /*
> -        * TRCARCHMIN, bits[7:4] architecture the minor version number
> -        * TRCARCHMAJ, bits[11:8] architecture major versin number
> -        */
> -       drvdata->arch = BMVAL(etmidr1, 4, 11);
> -
>         /* maximum size of resources */
>         etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
>         /* CIDSIZE, bits[9:5] Indicates the Context ID size */
> @@ -1618,7 +1621,7 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>                                 etm4_init_arch_data,  &init_arg, 1))
>                 dev_err(dev, "ETM arch init failed\n");
>
> -       if (etm4_arch_supported(drvdata->arch) == false) {
> +       if (!drvdata->arch) {
>                 ret = -EINVAL;
>                 goto err_arch_supported;
>         }
> @@ -1653,7 +1656,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>
>         pm_runtime_put(&adev->dev);
>         dev_info(&drvdata->csdev->dev, "CPU%d: ETM v%d.%d initialized\n",
> -                drvdata->cpu, drvdata->arch >> 4, drvdata->arch & 0xf);
> +                drvdata->cpu, etm_devarch_arch(drvdata->arch),
> +                etm_devarch_rev(drvdata->arch));
>
>         if (boot_enable) {
>                 coresight_enable(drvdata->csdev);
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 277c22540db6..b217f16ad921 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -730,7 +730,7 @@ struct etmv4_save_state {
>   * @spinlock:   Only one at a time pls.
>   * @mode:      This tracer's mode, i.e sysFS, Perf or disabled.
>   * @cpu:        The cpu this component is affined to.
> - * @arch:       ETM version number.
> + * @arch:       ETM device architecture register.
>   * @nr_pe:     The number of processing entity available for tracing.
>   * @nr_pe_cmp: The number of processing entity comparator inputs that are
>   *             available for tracing.
> @@ -793,7 +793,7 @@ struct etmv4_drvdata {
>         spinlock_t                      spinlock;
>         local_t                         mode;
>         int                             cpu;
> -       u8                              arch;
> +       u32                             arch;
>         u8                              nr_pe;
>         u8                              nr_pe_cmp;
>         u8                              nr_addr_cmp;
> --
> 2.24.1
>

Regards


Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Suzuki K Poulose Sept. 22, 2020, 11:18 a.m. UTC | #2 
On 09/18/2020 04:35 PM, Mike Leach wrote:
> Hi Suzuki
> 
> On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> We have been using TRCIDR1 for detecting the ETM version. As
>> we are about to discover the trace unit on a CPU, let us use a
>> CoreSight architected register, instead of an ETM architected
>> register for accurate detection on a CPU. e.g, a CPU might
>> implement a custom trace unit, not an ETM.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>


>>   static int etm4_cpu_id(struct coresight_device *csdev)
>>   {
>>          struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> @@ -694,10 +693,23 @@ static void etm_detect_lock_status(struct etmv4_drvdata *drvdata,
>>          drvdata->os_lock_model = TRCOSLSR_OSM(os_lsr);
>>   }
>>
>> +static inline bool trace_unit_supported(u32 devarch)
>> +{
>> +       return (devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH;
>> +}
>> +
> 
> This is fine for system reg access - but imposes additional
> constraints on memory mapped devices that previously may have worked
> just by matching CID/PID. Can you be certain there are no devices out
> there that have omitted this register (it does have a present bit
> after all)

Very good point and I agree. I could restrict this to system instruction
based devices and use the TRCIDR1 for

> 
>>   static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
>>                                    struct csdev_access *csa)
>>   {
>> +       u32 devarch;
>> +
>> +       devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
>> +       if (!trace_unit_supported(devarch))
>> +               return false;
>> +
>>          *csa = CSDEV_ACCESS_IOMEM(drvdata->base);
>> +       drvdata->arch = devarch;
>> +
>>          return true;
>>   }
>>
>> @@ -713,7 +725,6 @@ static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
>>   static void etm4_init_arch_data(void *info)
> 
> The primary task of this routine is to read all the ID registers and
> set up the data in regards to resources etc.
> We should not be mixing in a secondary task of detection of a valid device.

I agree that we have to read up the registers. However, for system instructions
based devices, we shouldn't try to access random register space, if they are not
ETM. Moreover, it kind of simplifies the logic, where you don't have to read up
all the registers if this is not a supported device in the first place.

Or the other way around, I think the priority is to make sure we are dealing with
a valid device we support, before we tread into reading the register space to avoid
unknown side effects of the operations.

Thanks

Suzuki
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 5880f105268f..0fce9fb12cff 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -61,6 +61,17 @@  struct etm_init_arg {
 	struct csdev_access	*csa;
 };
 
+static inline u8 etm_devarch_arch(u32 devarch)
+{
+	return (devarch >> ETM_DEVARCH_ARCHID_ARCH_VER_SHIFT) & 0xfUL;
+}
+
+static inline u8 etm_devarch_rev(u32 devarch)
+{
+	return (devarch & ETM_DEVARCH_REVISION_MASK) >>
+		ETM_DEVARCH_REVISION_SHIFT;
+}
+
 u64 etm4x_sysreg_read(struct csdev_access *csa,
 		      u32 offset,
 		      bool _relaxed,
@@ -149,18 +160,6 @@  static void etm4_cs_unlock(struct etmv4_drvdata *drvdata,
 		CS_UNLOCK(csa);
 }
 
-static bool etm4_arch_supported(u8 arch)
-{
-	/* Mask out the minor version number */
-	switch (arch & 0xf0) {
-	case ETM_ARCH_V4:
-		break;
-	default:
-		return false;
-	}
-	return true;
-}
-
 static int etm4_cpu_id(struct coresight_device *csdev)
 {
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -694,10 +693,23 @@  static void etm_detect_lock_status(struct etmv4_drvdata *drvdata,
 	drvdata->os_lock_model = TRCOSLSR_OSM(os_lsr);
 }
 
+static inline bool trace_unit_supported(u32 devarch)
+{
+	return (devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH;
+}
+
 static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
 				  struct csdev_access *csa)
 {
+	u32 devarch;
+
+	devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
+	if (!trace_unit_supported(devarch))
+		return false;
+
 	*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
+	drvdata->arch = devarch;
+
 	return true;
 }
 
@@ -713,7 +725,6 @@  static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
 static void etm4_init_arch_data(void *info)
 {
 	u32 etmidr0;
-	u32 etmidr1;
 	u32 etmidr2;
 	u32 etmidr3;
 	u32 etmidr4;
@@ -784,14 +795,6 @@  static void etm4_init_arch_data(void *info)
 	/* TSSIZE, bits[28:24] Global timestamp size field */
 	drvdata->ts_size = BMVAL(etmidr0, 24, 28);
 
-	/* base architecture of trace unit */
-	etmidr1 = etm4x_relaxed_read32(csa, TRCIDR1);
-	/*
-	 * TRCARCHMIN, bits[7:4] architecture the minor version number
-	 * TRCARCHMAJ, bits[11:8] architecture major versin number
-	 */
-	drvdata->arch = BMVAL(etmidr1, 4, 11);
-
 	/* maximum size of resources */
 	etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
 	/* CIDSIZE, bits[9:5] Indicates the Context ID size */
@@ -1618,7 +1621,7 @@  static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
 				etm4_init_arch_data,  &init_arg, 1))
 		dev_err(dev, "ETM arch init failed\n");
 
-	if (etm4_arch_supported(drvdata->arch) == false) {
+	if (!drvdata->arch) {
 		ret = -EINVAL;
 		goto err_arch_supported;
 	}
@@ -1653,7 +1656,8 @@  static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pm_runtime_put(&adev->dev);
 	dev_info(&drvdata->csdev->dev, "CPU%d: ETM v%d.%d initialized\n",
-		 drvdata->cpu, drvdata->arch >> 4, drvdata->arch & 0xf);
+		 drvdata->cpu, etm_devarch_arch(drvdata->arch),
+		 etm_devarch_rev(drvdata->arch));
 
 	if (boot_enable) {
 		coresight_enable(drvdata->csdev);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 277c22540db6..b217f16ad921 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -730,7 +730,7 @@  struct etmv4_save_state {
  * @spinlock:   Only one at a time pls.
  * @mode:	This tracer's mode, i.e sysFS, Perf or disabled.
  * @cpu:        The cpu this component is affined to.
- * @arch:       ETM version number.
+ * @arch:       ETM device architecture register.
  * @nr_pe:	The number of processing entity available for tracing.
  * @nr_pe_cmp:	The number of processing entity comparator inputs that are
  *		available for tracing.
@@ -793,7 +793,7 @@  struct etmv4_drvdata {
 	spinlock_t			spinlock;
 	local_t				mode;
 	int				cpu;
-	u8				arch;
+	u32				arch;
 	u8				nr_pe;
 	u8				nr_pe_cmp;
 	u8				nr_addr_cmp;