| Message ID | 20201028220945.3826358-24-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Support for ETM system instructions | expand |
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. 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> > /* wait for TRCSTATR.IDLE to go up */ > if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 1)) > dev_err(etm_dev, > @@ -256,6 +265,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > /* Enable the trace unit */ > etm4x_relaxed_write32(csa, 1, TRCPRGCTLR); > > + /* Synchronize the register updates for sysreg access */ > + if (!csa->io_mem) > + isb(); > + > /* wait for TRCSTATR.IDLE to go back down to '0' */ > if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 0)) > dev_err(etm_dev, > -- > 2.24.1 >
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
On 11/10/20 10:11 AM, 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.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. > The ETM architecture uses "trace analyzer" to mean self-hosted software and an external debugger. It's a useful term that generically covers "the thing that's in charge of tracing" and "the thing that's capturing and/or decoding the trace", regardless of whether either of these are external or self-hosted (or even a mixture!). So in 4.3.7, yes this does mean that context synchronization events are needed to synchronize register updates when using system instructions to program the trace unit. I'll take a look at what we can improve here :-) Cheers, John. >> 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 >_______________________________________________ >CoreSight mailing list >CoreSight@lists.linaro.org >https://lists.linaro.org/mailman/listinfo/coresight
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.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. 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.poirier@linaro.org> > > > > Thanks > Suzuki
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(); + /* wait for TRCSTATR.IDLE to go up */ if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 1)) dev_err(etm_dev, @@ -256,6 +265,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) /* Enable the trace unit */ etm4x_relaxed_write32(csa, 1, TRCPRGCTLR); + /* Synchronize the register updates for sysreg access */ + if (!csa->io_mem) + isb(); + /* wait for TRCSTATR.IDLE to go back down to '0' */ if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 0)) dev_err(etm_dev,
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(+)