Re: [PATCH v2] coresight-etm4x: add isb() before reading the TRCSTATR

[Yuanfang Zhang <quic_yuanfang@quicinc.com>]

On 12/4/2024 11:16 PM, Leo Yan wrote:
> Hi Yuanfang,
> 
> Recently I just acrossed this part, so some comments.
> 
> On Wed, Dec 04, 2024 at 07:23:32PM +0800, yuanfang zhang wrote:
>>
>> From: Yuanfang Zhang <quic_yuanfang@quicinc.com>
>>
>> As recommended by section 4.3.7 ("Synchronization when using system
>> instructions to progrom the trace unit") of ARM IHI 0064H.b, the
>> self-hosted trace analyzer must perform a Context synchronization
>> event between writing to the TRCPRGCTLR and reading the TRCSTATR.
>>
>> Fixes: ebddaad09e10 ("coresight: etm4x: Add missing single-shot control API to sysfs")
>> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
>> ---
>> Change in V2:
>> Added comments in the code.
>>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 66d44a404ad0..decb3a87e27e 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -906,6 +906,13 @@ static void etm4_disable_hw(void *info)
>>         tsb_csync();
>>         etm4x_relaxed_write32(csa, control, TRCPRGCTLR);
>>
>> +       /*
>> +        * As recommended by section 4.3.7 ("Synchronization when using system
>> +        * instructions to progrom the trace unit") of ARM IHI 0064H.b, the
>> +        * self-hosted trace analyzer must perform a Context synchronization
>> +        * event between writing to the TRCPRGCTLR and reading the TRCSTATR.
>> +        */
>> +       isb();
> 
> As described in the doc, the "isb" is only required for system
> instructions case.  It is good to only apply the ISB on system
> register case:
> 
>         if (!csa->io_mem)
>                 isb();
> 

updated in PATCH V3.

>>         /* wait for TRCSTATR.PMSTABLE to go to '1' */
>>         if (coresight_timeout(csa, TRCSTATR, TRCSTATR_PMSTABLE_BIT, 1))
>>                 dev_err(etm_dev,
> 
> As mentioned in system register case: "Arm recommends that the
> self-hosted trace analyzer always executes an ISB instruction after
> programming the trace unit registers, to ensure that all updates are
> committed to the trace unit before normal code execution resumes."
> 
> And for MMIO case:
> 
> "When the memory is marked as Device-nGnRE or stronger.
>  ...
>  - Poll TRCSTATR to ensure the previous write has completed.
>  — Execute an ISB operation."
> 
> Thus we need to add an ISB after polling.
> 
>         isb();
> 

updated in PATCH V3.

> For consistent behaviour, a relevant thing is the dsb(sy) in
> etm4_enable_hw().  I do not think the dsb(sy) is necessary, as the
> driver uses the sequence "write TRCPRGCTLR + polling TRCSTATR" to
> ensure the data has been populated to trace unit, the polling
> operations somehow act as a read back.  And the ETM manual does not
> mention the requirement for DSB when enabling trace unit.  Thus, we
> should remove dsb(sy) (maybe use a separte patch).
> 

updated in PATCH V3.

> Mike / Suzuki / James, please confirm if my conclusions are valid.
> 
> Thanks,
> Leo