diff mbox series

[16/19] coresight: etm4x: Detect system instructions support

Message ID 20200911084119.1080694-17-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
ETM v4.4 onwards adds support for system instruction access
to the ETM. Detect the support on an ETM and switch to using the
mode when available.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x.c | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

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:
>
> ETM v4.4 onwards adds support for system instruction access
> to the ETM. Detect the support on an ETM and switch to using the
> mode when available.
>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 0fce9fb12cff..dc5ac171db35 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -693,11 +693,39 @@ static void etm_detect_lock_status(struct etmv4_drvdata *drvdata,
>         drvdata->os_lock_model = TRCOSLSR_OSM(os_lsr);
>  }
>
> +static inline bool cpu_supports_sysreg_trace(void)
> +{
> +       u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> +
> +       return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) > 0;
> +}
> +

This will be an issue if you have an aarch32 device (eg Cortex-A32 or
similar, with ETM support but no aarch64)

>  static inline bool trace_unit_supported(u32 devarch)
>  {
>         return (devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH;
>  }
>
> +static bool etm_init_sysreg_access(struct etmv4_drvdata *drvdata,
> +                                  struct csdev_access *csa)
> +{
> +       u32 devarch;
> +
> +       if (!cpu_supports_sysreg_trace())
> +               return false;
> +
> +       devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH);
> +       if (!trace_unit_supported(devarch))
> +               return false;
> +       *csa = (struct csdev_access) {
> +               .io_mem = false,
> +               .read   = etm4x_sysreg_read,
> +               .write  = etm4x_sysreg_write,
> +       };
> +
> +       drvdata->arch = devarch;
> +       return true;
> +}
> +
>  static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
>                                   struct csdev_access *csa)
>  {
> @@ -716,6 +744,9 @@ static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
>  static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
>                                   struct csdev_access *csa)
>  {
> +       if (etm_init_sysreg_access(drvdata, csa))
> +

Don't think we should enforce system instruction access if the device
tree has defined memory access. The driver cannot possibly know if
this is a mistake or deliberate (e.g. test / implementation bug fix).>
+               return true;

>         if (drvdata->base)
>                 return etm_init_iomem_access(drvdata, csa);
>
> --
> 2.24.1
>

The device tree bindings define the access support intended - and
there is access specific probing. i.e. the next patch splits amba (mem
access) / platform (sys access) driver probes, followed by the common
probe section. The  register / memory access support used should be
made there, and the detection of a compatible device for the register
access i.e. check TRCDEVARCH should be in the platform probe path too
- possibly simplifying things and ensuring the common code changes are
reduced.

Regards

Mike



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Suzuki K Poulose Sept. 22, 2020, 11:59 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:
>>
>> ETM v4.4 onwards adds support for system instruction access
>> to the ETM. Detect the support on an ETM and switch to using the
>> mode when available.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm4x.c | 31 +++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index 0fce9fb12cff..dc5ac171db35 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -693,11 +693,39 @@ static void etm_detect_lock_status(struct etmv4_drvdata *drvdata,
>>          drvdata->os_lock_model = TRCOSLSR_OSM(os_lsr);
>>   }
>>
>> +static inline bool cpu_supports_sysreg_trace(void)
>> +{
>> +       u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
>> +
>> +       return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) > 0;
>> +}
>> +
> 
> This will be an issue if you have an aarch32 device (eg Cortex-A32 or
> similar, with ETM support but no aarch64)

Agreed and in fact this was part of the header file in my initial versions.
I could move it back there. However, the ETM4x even without this series doesn't
support aarch32. The compilation would fail for various reasons (e.g, readq()).

> 
>>   static inline bool trace_unit_supported(u32 devarch)
>>   {
>>          return (devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH;
>>   }
>>
>> +static bool etm_init_sysreg_access(struct etmv4_drvdata *drvdata,
>> +                                  struct csdev_access *csa)
>> +{
>> +       u32 devarch;
>> +
>> +       if (!cpu_supports_sysreg_trace())
>> +               return false;
>> +
>> +       devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH);
>> +       if (!trace_unit_supported(devarch))
>> +               return false;
>> +       *csa = (struct csdev_access) {
>> +               .io_mem = false,
>> +               .read   = etm4x_sysreg_read,
>> +               .write  = etm4x_sysreg_write,
>> +       };
>> +
>> +       drvdata->arch = devarch;
>> +       return true;
>> +}
>> +
>>   static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
>>                                    struct csdev_access *csa)
>>   {
>> @@ -716,6 +744,9 @@ static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
>>   static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
>>                                    struct csdev_access *csa)
>>   {
>> +       if (etm_init_sysreg_access(drvdata, csa))
>> +
> 
> Don't think we should enforce system instruction access if the device
> tree has defined memory access. The driver cannot possibly know if
> this is a mistake or deliberate (e.g. test / implementation bug fix).>
> +               return true;

Agreed, will fix it.

> 
>>          if (drvdata->base)
>>                  return etm_init_iomem_access(drvdata, csa);
>>
>> --
>> 2.24.1
>>
> 
> The device tree bindings define the access support intended - and
> there is access specific probing. i.e. the next patch splits amba (mem
> access) / platform (sys access) driver probes, followed by the common
> probe section. The  register / memory access support used should be
> made there, and the detection of a compatible device for the register
> access i.e. check TRCDEVARCH should be in the platform probe path too
> - possibly simplifying things and ensuring the common code changes are
> reduced.

I will address this in the next version. I believe we could work around
the problem of missing TRCDEVARCH on older platforms.

Thanks a lot for the review !

Suzuki
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 0fce9fb12cff..dc5ac171db35 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -693,11 +693,39 @@  static void etm_detect_lock_status(struct etmv4_drvdata *drvdata,
 	drvdata->os_lock_model = TRCOSLSR_OSM(os_lsr);
 }
 
+static inline bool cpu_supports_sysreg_trace(void)
+{
+	u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
+
+	return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) > 0;
+}
+
 static inline bool trace_unit_supported(u32 devarch)
 {
 	return (devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH;
 }
 
+static bool etm_init_sysreg_access(struct etmv4_drvdata *drvdata,
+				   struct csdev_access *csa)
+{
+	u32 devarch;
+
+	if (!cpu_supports_sysreg_trace())
+		return false;
+
+	devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH);
+	if (!trace_unit_supported(devarch))
+		return false;
+	*csa = (struct csdev_access) {
+		.io_mem	= false,
+		.read	= etm4x_sysreg_read,
+		.write	= etm4x_sysreg_write,
+	};
+
+	drvdata->arch = devarch;
+	return true;
+}
+
 static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
 				  struct csdev_access *csa)
 {
@@ -716,6 +744,9 @@  static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
 static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
 				  struct csdev_access *csa)
 {
+	if (etm_init_sysreg_access(drvdata, csa))
+		return true;
+
 	if (drvdata->base)
 		return etm_init_iomem_access(drvdata, csa);