Messages in this thread | | | Subject | Re: [PATCH v3 22/26] coresight: etm4x: Add necessary synchronization for sysreg access | From | Suzuki K Poulose <> | Date | Tue, 10 Nov 2020 10:11:23 +0000 |
| |
On 11/9/20 6:32 PM, Mathieu Poirier wrote: > On Wed, Oct 28, 2020 at 10:09:41PM +0000, Suzuki K Poulose wrote: >> As per the specification any update to the TRCPRGCTLR must be synchronized >> by a context synchronization event (in our case an explicist ISB) before >> the TRCSTATR is checked. >> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index e36bc1c722c7..4bc2f15b6332 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -178,6 +178,15 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) >> /* Disable the trace unit before programming trace registers */ >> etm4x_relaxed_write32(csa, 0, TRCPRGCTLR); >> >> + /* >> + * If we use system instructions, we need to synchronize the >> + * write to the TRCPRGCTLR, before accessing the TRCSTATR. >> + * See ARM IHI0064F, section >> + * "4.3.7 Synchronization of register updates" >> + */ >> + if (!csa->io_mem) >> + isb(); >> + > > When I first read the documentation on system instruction section 4.3.7 really > got me thinking... > > At the very top, right after the title "Synchronization of register updates" one > can read "Software running on the PE...". Later in the text, when specifying > the synchronisation rules, the term "trace analyzer" is used. _Typically_ a trace > analyzer is an external box. >
Very good point. The trace analyzer could still use the system register to program the ETM and causing a context synchronization event is tricky from within the trace analyzer. And I agree that there is a bit of confusion around the synchronization from a self-hosted point of view. I believe this is true for the self-hosted case too and should be clarified in the TRM.
> Arm documentation is precise and usually doesn't overlook that kind of detail. > The question is to understand if a context synchronisation event is also needed > when programming is done on the PE. If so I think the documentation should be > amended. > > In that case: > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> >
Thanks Suzuki
| |