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

[Leo Yan <leo.yan@arm.com>]

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();

>         /* 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();

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).

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

Thanks,
Leo