Re: [PATCH v4 13/15] coresight: trbe: Save and restore state across CPU low power state

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

On 10/11/2025 12:01, James Clark wrote:
> 
> 
> On 04/11/2025 3:21 pm, Leo Yan wrote:
>> From: Yabin Cui <yabinc@google.com>
>>
>> Similar to ETE, TRBE may lose its context when a CPU enters low power
>> state. To make things worse, if ETE is restored without TRBE being
>> restored, an enabled source device with no enabled sink devices can
>> cause CPU hang on some devices (e.g., Pixel 9).
>>
>> The save and restore flows are described in the section K5.5 "Context
>> switching" of Arm ARM (ARM DDI 0487 L.a). This commit adds save and
>> restore callbacks with following the software usages defined in the
>> architecture manual.
>>
>> Signed-off-by: Yabin Cui <yabinc@google.com>
>> Co-developed-by: Leo Yan <leo.yan@arm.com>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-trbe.c | 84 ++++++++++++++++++ 
>> +++++++++-
>>   1 file changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/ 
>> hwtracing/coresight/coresight-trbe.c
>> index 
>> 3c82ab4394fde1e3dd4371a9b1703da4d8f6db9d..9888a153bc2f99b4826f6103bdbc12304c9c94cb 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -116,6 +116,20 @@ static int trbe_errata_cpucaps[] = {
>>    */
>>   #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES    256
>> +/*
>> + * struct trbe_save_state: Register values representing TRBE state
>> + * @trblimitr        - Trace Buffer Limit Address Register value
>> + * @trbbaser        - Trace Buffer Base Register value
>> + * @trbptr        - Trace Buffer Write Pointer Register value
>> + * @trbsr        - Trace Buffer Status Register value
>> + */
>> +struct trbe_save_state {
>> +    u64 trblimitr;
>> +    u64 trbbaser;
>> +    u64 trbptr;
>> +    u64 trbsr;
>> +};
>> +
>>   /*
>>    * struct trbe_cpudata: TRBE instance specific data
>>    * @trbe_flag        - TRBE dirty/access flag support
>> @@ -134,6 +148,7 @@ struct trbe_cpudata {
>>       enum cs_mode mode;
>>       struct trbe_buf *buf;
>>       struct trbe_drvdata *drvdata;
>> +    struct trbe_save_state save_state;
>>       DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
>>   };
>> @@ -1189,6 +1204,71 @@ static irqreturn_t arm_trbe_irq_handler(int 
>> irq, void *dev)
>>       return IRQ_HANDLED;
>>   }
>> +static int arm_trbe_save(struct coresight_device *csdev)
>> +{
>> +    struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
>> +    struct trbe_save_state *state = &cpudata->save_state;
>> +
>> +    if (cpudata->mode == CS_MODE_DISABLED)
>> +        return 0;
>> +
>> +    /*
>> +     * According to the section K5.5 Context switching, Arm ARM (ARM DDI
>> +     * 0487 L.a), the software usage VKHHY requires a TSB CSYNC 
>> instruction
>> +     * to ensure the program-flow trace is flushed, which has been 
>> executed
>> +     * in ETM driver.
>> +     */
>> +
>> +    /* Disable trace buffer unit */
>> +    state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
>> +    write_sysreg_s(state->trblimitr & ~TRBLIMITR_EL1_E, 
>> SYS_TRBLIMITR_EL1);
>> +
>> +    /*
>> +     * Execute a further Context synchronization event. Ensure the 
>> writes to
>> +     * memory are complete.
>> +     */
>> +    trbe_drain_buffer();
>> +
>> +    /* Synchronize the TRBE disabling */
>> +    isb();
> 
> Can we use set_trbe_disabled() here, and set_trbe_enabled() for restore 
> below. There is a bit of duplication and they do the same thing except 
> this version here always has trbe_drain_buffer() rather than it being 
> conditional. I'm not sure if that is correct as they should both be the 
> same?
> 
> set_trbe_disabled() already reads TRBLIMITR_EL1 so you can return it.

+1

>> +    if (trbe_needs_ctxt_sync_after_enable(cpudata))
>> +        isb();
> 
> Especially this double isb() part shouldn't be duplicated as that could 
> get broken in a refactor in the future.

+1

Suzuki