Re: [PATCH V2 5/7] coresight: trbe: Work around the ignored system register writes

[Anshuman Khandual]

On 1/10/22 4:33 PM, Suzuki K Poulose wrote:
> On 07/01/2022 01:10, Anshuman Khandual wrote:
>> TRBE implementations affected by Arm erratum #2064142 might fail to write
>> into certain system registers after the TRBE has been disabled. Under some
>> conditions after TRBE has been disabled, writes into certain TRBE registers
>> TRBLIMITR_EL1, TRBPTR_EL1, TRBBASER_EL1, TRBSR_EL1 and TRBTRG_EL1 will be
>> ignored and not be effected.
>>
>> Work around this problem in the TRBE driver by executing TSB CSYNC and DSB
>> just after the trace collection has stopped and before performing a system
>> register write to one of the affected registers. This just updates the TRBE
>> driver as required.
>>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> Cc: Suzuki Poulose <suzuki.poulose@xxxxxxx>
>> Cc: coresight@xxxxxxxxxxxxxxxx
>> Cc: linux-doc@xxxxxxxxxxxxxxx
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>>   arch/arm64/Kconfig                           |  2 +-
>>   drivers/hwtracing/coresight/coresight-trbe.c | 54 ++++++++++++++------
>>   drivers/hwtracing/coresight/coresight-trbe.h |  8 ---
>>   3 files changed, 39 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index f1651cb71ef3..b6d62672bf7d 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -780,7 +780,7 @@ config ARM64_ERRATUM_2224489
>>     config ARM64_ERRATUM_2064142
>>       bool "Cortex-A510: 2064142: workaround TRBE register writes while disabled"
>> -    depends on COMPILE_TEST # Until the CoreSight TRBE driver changes are in
>> +    depends on CORESIGHT_TRBE
>>       default y
>>       help
>>         This option adds the workaround for ARM Cortex-A510 erratum 2064142.
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index 276862c07e32..850e9fca6847 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -91,10 +91,12 @@ struct trbe_buf {
>>    */
>>   #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE    0
>>   #define TRBE_WORKAROUND_WRITE_OUT_OF_RANGE    1
>> +#define TRBE_NEEDS_DRAIN_AFTER_DISABLE        2
>>     static int trbe_errata_cpucaps[] = {
>>       [TRBE_WORKAROUND_OVERWRITE_FILL_MODE] = ARM64_WORKAROUND_TRBE_OVERWRITE_FILL_MODE,
>>       [TRBE_WORKAROUND_WRITE_OUT_OF_RANGE] = ARM64_WORKAROUND_TRBE_WRITE_OUT_OF_RANGE,
>> +    [TRBE_NEEDS_DRAIN_AFTER_DISABLE] = ARM64_WORKAROUND_2064142,
>>       -1,        /* Sentinel, must be the last entry */
>>   };
>>   @@ -167,6 +169,11 @@ static inline bool trbe_may_write_out_of_range(struct trbe_cpudata *cpudata)
>>       return trbe_has_erratum(cpudata, TRBE_WORKAROUND_WRITE_OUT_OF_RANGE);
>>   }
>>   +static inline bool trbe_needs_drain_after_disable(struct trbe_cpudata *cpudata)
>> +{
>> +    return trbe_has_erratum(cpudata, TRBE_NEEDS_DRAIN_AFTER_DISABLE);
>> +}
>> +
>>   static int trbe_alloc_node(struct perf_event *event)
>>   {
>>       if (event->cpu == -1)
>> @@ -174,30 +181,42 @@ static int trbe_alloc_node(struct perf_event *event)
>>       return cpu_to_node(event->cpu);
>>   }
>>   -static void trbe_drain_buffer(void)
>> +static inline void trbe_drain_buffer(void)
>>   {
>>       tsb_csync();
>>       dsb(nsh);
>>   }
>>   -static void trbe_drain_and_disable_local(void)
>> +static inline void set_trbe_disabled(struct trbe_cpudata *cpudata)
>>   {
>>       u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
>>   -    trbe_drain_buffer();
>> -
>>       /*
>>        * Disable the TRBE without clearing LIMITPTR which
>>        * might be required for fetching the buffer limits.
>>        */
>>       trblimitr &= ~TRBLIMITR_ENABLE;
>>       write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
>> +
>> +    /*
>> +     * Errata affected TRBE implementation will need TSB CSYNC and
>> +     * DSB in order to prevent subsequent writes into certain TRBE
>> +     * system registers from being ignored and not effected.
>> +     */
> 
> minor nit: This comment could be moved to the definition of the
> function "trbe_needs_drain_after_disable()" to make more sense.
> The name is implicit here indicating, why we are doing a drain.

Sure, will move the comment just before the function definition.

> 
> Either ways:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>