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?
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
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.
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
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
-
Mathieu Poirier -
Mike Leach