Re: [PATCH V3 05/14] coresight: ete: Add support for ETE tracing

Suzuki K Poulose
2021-02-02T22:51:56Z

On 2/2/21 6:56 PM, Mathieu Poirier wrote:
> On Wed, Jan 27, 2021 at 02:25:29PM +0530, Anshuman Khandual wrote:
>> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> Add ETE as one of the supported device types we support
>> with ETM4x driver. The devices are named following the
>> existing convention as ete<N>.
>>
>> ETE mandates that the trace resource status register is programmed
>> before the tracing is turned on. For the moment simply write to
>> it indicating TraceActive.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---

...

>> @@ -1834,10 +1854,6 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>> if (drvdata->cpu < 0)
>> return drvdata->cpu;
>>
>> - desc.name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
>> - if (!desc.name)
>> - return -ENOMEM;
>> -
>> init_arg.drvdata = drvdata;
>> init_arg.csa = &desc.access;
>> init_arg.pid = etm_pid;
>> @@ -1853,6 +1869,20 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>> if (!desc.access.io_mem ||
>> fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>> drvdata->skip_power_up = true;
>
> Add a space here...
>
>> + major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
>> + minor = ETM_ARCH_MINOR_VERSION(drvdata->arch);
>
> And here too. Othersiwe it makes a big blob in the middle of the function.
>
>> + if (etm4x_is_ete(drvdata)) {
>> + type_name = "ete";
>> + /* ETE v1 has major version == 5. Adjust this for logging.*/
>> + major -= 4;
>
> I don't have the documentation for the ETE but I would not adjust @major. I
> would simply leave it to what the HW gives us since regardless of the name, the
> major revision of the IP block is 5.
>

At the moment only register definitions are public and can be found here :

https://developer.arm.com/docs/ddi0601/g/aarch64-system-registers/trcdevarch

The ETE is natural extension of the ETM architecture to support future
architecture changes and is designed in a way that the same software
can driver both ETM and ETE without much changes.

>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index ca24ac5..8b90de5 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -128,6 +128,8 @@
>> #define TRCCIDR2 0xFF8
>> #define TRCCIDR3 0xFFC
>>
>> +#define TRCRSR_TA BIT(12)
>> +
>> /*
>> * System instructions to access ETM registers.
>> * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
>> @@ -390,6 +392,9 @@
>> #define ETM_COMMON_SYSREG_LIST_CASES \
>> ETM_COMMON_SYSREG_LIST(NOP, __unused)
>>
>> +#define ETM4x_ONLY_SYSREG_LIST_CASES \
>> + ETM4x_ONLY_SYSREG_LIST(NOP, __unused)
>> +
>> #define ETM4x_SYSREG_LIST_CASES \
>> ETM_COMMON_SYSREG_LIST_CASES \
>> ETM4x_ONLY_SYSREG_LIST(NOP, __unused)
>> @@ -406,7 +411,6 @@
>> ETE_ONLY_SYSREG_LIST(WRITE, (val))
>>
>> #define ETE_ONLY_SYSREG_LIST_CASES \
>> - ETM_COMMON_SYSREG_LIST_CASES \
>
> This goes in patch 04.
>

Sure, will move it.

> With the above:
>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Thanks for the review

Suzuki