[PATCH] coresight: etm4x: fix issues on trcseqevr access

[PATCH] coresight: etm4x: fix issues on trcseqevr access

[Jonathan Zhou]

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));
 
 	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: fix issues on trcseqevr access

[Mathieu Poirier]

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
trace unit state.

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: fix issues on trcseqevr access

[Mike Leach]

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.

> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: fix issues on trcseqevr access

[Jonathan Zhou]

Hi Mike,

On 08/09/2020 00:51, Mike Leach 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.
> 
>> 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.
> 
Thanks for point out this, I just never thought it may be a typo.

Regards

Jonathan

> 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
>>>
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: fix issues on trcseqevr access

[Mathieu Poirier]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: fix issues on trcseqevr access

[Mike Leach]

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



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: fix issues on trcseqevr access

[Mathieu Poirier]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: fix issues on trcseqevr access

[Mathieu Poirier]

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(-)

I have applied this patch.

Thanks,
Mathieu

> 
> 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));
>  
>  	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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel