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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel