On Wed, Sep 09, 2020 at 10:41:27AM +0100, Mike Leach wrote:
Hi Mathieu,
On Tue, 8 Sep 2020 at 18:43, Mathieu Poirier mathieu.poirier@linaro.org wrote:
Hi Mike,
On Mon, 7 Sep 2020 at 10:52, Mike Leach mike.leach@linaro.org wrote:
Hi,
On Thu, 3 Sep 2020 at 18:42, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Wed, Sep 02, 2020 at 06:37:13PM +0800, Jonathan Zhou wrote:
The TRCSEQEVR(3) is reserved, using '@nrseqstate - 1' instead to avoid accessing the reserved register.
Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Suzuki K Poulose suzuki.poulose@arm.com Cc: Mike Leach mike.leach@linaro.org Cc: Shaokun Zhang zhangshaokun@hisilicon.com Cc: lizixian@hisilicon.com
Signed-off-by: Jonathan Zhou jonathan.zhouwen@huawei.com
drivers/hwtracing/coresight/coresight-etm4x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 96425e818fc2..44e44c817bf8 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -1183,7 +1183,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata) state->trcvdsacctlr = readl(drvdata->base + TRCVDSACCTLR); state->trcvdarcctlr = readl(drvdata->base + TRCVDARCCTLR);
for (i = 0; i < drvdata->nrseqstate; i++)
for (i = 0; i < drvdata->nrseqstate - 1; i++) state->trcseqevr[i] = readl(drvdata->base + TRCSEQEVRn(i));The section 3.4.3 "Guidelines for trace unit registers to be saved and restored" of the ETM4 Architecture Specification (ARM IHI0064F ID042818) is clear on the fact that registers TRCSEQEVR0-3 have to be taken into account when saving the
I think this is a typo in the TRM. I'll ping the docs people in ARM.
Did you get an answer from the document people? Is this really a typographical problem?
Haven't had a reply, but this is a clear error. All other parts of the spec, including programming the device and register descriptions give a valid n=0-2 for TRCSEQEVRn. TRCSEQEVR3 does not exist, and the assumed location is indeed reserved. That said, reserved registers are marked as RES0H - so no actual harm can come from reading / writing the register - but we should be consistent.
I would regard section 3.4.3 as a list of registers to save / restore as correct with the caveat - "should they exist in the implementation". Quite a few of the register ranges defined in this list show the maximum extent of registers where the actual number of registers is implementation dependent.
Very well - I will proceed with this patch.
Thanks
Mike
Thanks, Mathieu
trace unit state.
Looking @ the register descriptions for TRCSEQEVRn (7.3.63) in the above document n=0-2. The number of states is set by TRCIDR5.NUMSEQSTATE (7.3.35). This can take the value 0 or 4. If 4 then there are 3 TRCSEQEVR(n) registers - 0 to 2 - one for each state transition.
Thus this patch is correct in using nrseqstate - 1.
Regards
Mike
Thanks, Mathieu
state->trcseqrstevr = readl(drvdata->base + TRCSEQRSTEVR);@@ -1288,7 +1288,7 @@ static void etm4_cpu_restore(struct etmv4_drvdata *drvdata) writel_relaxed(state->trcvdsacctlr, drvdata->base + TRCVDSACCTLR); writel_relaxed(state->trcvdarcctlr, drvdata->base + TRCVDARCCTLR);
for (i = 0; i < drvdata->nrseqstate; i++)
for (i = 0; i < drvdata->nrseqstate - 1; i++) writel_relaxed(state->trcseqevr[i], drvdata->base + TRCSEQEVRn(i));-- 1.9.1
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK