On Tue, Nov 10, 2020 at 10:11:23AM +0000, Suzuki K Poulose wrote: > 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.le...@linaro.org> > > > Cc: Mathieu Poirier <mathieu.poir...@linaro.org> > > > Signed-off-by: Suzuki K Poulose <suzuki.poul...@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.
I think John has given us the final answer here - the current implementation stands. > > > 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.poir...@linaro.org> > > > > Thanks > Suzuki
