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

[Suzuki K Poulose]

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 at 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 at 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