diff mbox series

[v2,3/4] coresight: etm3x: Deal with CLAIM tag before and after accessing HW

Message ID 1541632132-1252-4-git-send-email-mathieu.poirier@linaro.org
State Superseded
Headers show
Series coresight: Fix miscellaneous problems with CLAIM tags | expand

Commit Message

Mathieu Poirier Nov. 7, 2018, 11:08 p.m. UTC 
This patch moves access to the CLAIM tag so that no modification to the HW
happens before and after the CLAIM operation has been carried.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

---
 drivers/hwtracing/coresight/coresight-etm3x.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.7.4

Comments

Suzuki K Poulose Nov. 8, 2018, 5:13 p.m. UTC | #1 
Hi,

On 07/11/2018 23:08, Mathieu Poirier wrote:
> This patch moves access to the CLAIM tag so that no modification to the HW

> happens before and after the CLAIM operation has been carried.

> 

> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> ---

>   drivers/hwtracing/coresight/coresight-etm3x.c | 16 ++++++++--------

>   1 file changed, 8 insertions(+), 8 deletions(-)

> 

> diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c

> index fd5c4cca7db5..4f638d81a66a 100644

> --- a/drivers/hwtracing/coresight/coresight-etm3x.c

> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c

> @@ -363,15 +363,16 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)

>   

>   	CS_UNLOCK(drvdata->base);

>   

> +	rc = coresight_claim_device_unlocked(drvdata->base);

> +	if (rc)

> +		goto done;

> +

>   	/* Turn engine on */

>   	etm_clr_pwrdwn(drvdata);

>   	/* Apply power to trace registers */

>   	etm_set_pwrup(drvdata);

>   	/* Make sure all registers are accessible */

>   	etm_os_unlock(drvdata);

> -	rc = coresight_claim_device_unlocked(drvdata->base);

> -	if (rc)

> -		goto done;

>   

>   	etm_set_prog(drvdata);

>   

> @@ -422,12 +423,11 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)

>   	etm_clr_prog(drvdata);

>   

>   done:

> -	if (rc)

> -		etm_set_pwrdwn(drvdata);

>   	CS_LOCK(drvdata->base);

>   

> -	dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",

> -		drvdata->cpu, rc);

> +	if (!rc)

> +		dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",

> +			drvdata->cpu, rc);


Isn't it good to report the failure case too ? Anyway it is dev_dbg and
will be a useful info when we debug issues. Otherwise,

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>


>   	return rc;

>   }

>   

> @@ -577,9 +577,9 @@ static void etm_disable_hw(void *info)

>   	for (i = 0; i < drvdata->nr_cntr; i++)

>   		config->cntr_val[i] = etm_readl(drvdata, ETMCNTVRn(i));

>   

> +	etm_set_pwrdwn(drvdata);

>   	coresight_disclaim_device_unlocked(drvdata->base);

>   

> -	etm_set_pwrdwn(drvdata);

>   	CS_LOCK(drvdata->base);

>   

>   	dev_dbg(drvdata->dev, "cpu: %d disable smp call done\n", drvdata->cpu);

>
Mathieu Poirier Nov. 9, 2018, 5:59 p.m. UTC | #2 
On Thu, 8 Nov 2018 at 10:13, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>

> Hi,

>

> On 07/11/2018 23:08, Mathieu Poirier wrote:

> > This patch moves access to the CLAIM tag so that no modification to the HW

> > happens before and after the CLAIM operation has been carried.

> >

> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> > ---

> >   drivers/hwtracing/coresight/coresight-etm3x.c | 16 ++++++++--------

> >   1 file changed, 8 insertions(+), 8 deletions(-)

> >

> > diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c

> > index fd5c4cca7db5..4f638d81a66a 100644

> > --- a/drivers/hwtracing/coresight/coresight-etm3x.c

> > +++ b/drivers/hwtracing/coresight/coresight-etm3x.c

> > @@ -363,15 +363,16 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)

> >

> >       CS_UNLOCK(drvdata->base);

> >

> > +     rc = coresight_claim_device_unlocked(drvdata->base);

> > +     if (rc)

> > +             goto done;

> > +

> >       /* Turn engine on */

> >       etm_clr_pwrdwn(drvdata);

> >       /* Apply power to trace registers */

> >       etm_set_pwrup(drvdata);

> >       /* Make sure all registers are accessible */

> >       etm_os_unlock(drvdata);

> > -     rc = coresight_claim_device_unlocked(drvdata->base);

> > -     if (rc)

> > -             goto done;

> >

> >       etm_set_prog(drvdata);

> >

> > @@ -422,12 +423,11 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)

> >       etm_clr_prog(drvdata);

> >

> >   done:

> > -     if (rc)

> > -             etm_set_pwrdwn(drvdata);

> >       CS_LOCK(drvdata->base);

> >

> > -     dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",

> > -             drvdata->cpu, rc);

> > +     if (!rc)

> > +             dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",

> > +                     drvdata->cpu, rc);

>

> Isn't it good to report the failure case too ? Anyway it is dev_dbg and

> will be a useful info when we debug issues. Otherwise,


Simply removing the "if (!rc)" will do the trick.  Can I do the
modification and add your tag or you prefer to see another revision?

>

> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

>

> >       return rc;

> >   }

> >

> > @@ -577,9 +577,9 @@ static void etm_disable_hw(void *info)

> >       for (i = 0; i < drvdata->nr_cntr; i++)

> >               config->cntr_val[i] = etm_readl(drvdata, ETMCNTVRn(i));

> >

> > +     etm_set_pwrdwn(drvdata);

> >       coresight_disclaim_device_unlocked(drvdata->base);

> >

> > -     etm_set_pwrdwn(drvdata);

> >       CS_LOCK(drvdata->base);

> >

> >       dev_dbg(drvdata->dev, "cpu: %d disable smp call done\n", drvdata->cpu);

> >

>
Suzuki K Poulose Nov. 9, 2018, 6:06 p.m. UTC | #3 
On 11/09/2018 05:59 PM, Mathieu Poirier wrote:
> On Thu, 8 Nov 2018 at 10:13, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:

>>

>> Hi,

>>

>> On 07/11/2018 23:08, Mathieu Poirier wrote:

>>> This patch moves access to the CLAIM tag so that no modification to the HW

>>> happens before and after the CLAIM operation has been carried.

>>>

>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>>> ---

>>>    drivers/hwtracing/coresight/coresight-etm3x.c | 16 ++++++++--------

>>>    1 file changed, 8 insertions(+), 8 deletions(-)

>>>

>>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c

>>> index fd5c4cca7db5..4f638d81a66a 100644

>>> --- a/drivers/hwtracing/coresight/coresight-etm3x.c

>>> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c

>>> @@ -363,15 +363,16 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)

>>>

>>>        CS_UNLOCK(drvdata->base);

>>>

>>> +     rc = coresight_claim_device_unlocked(drvdata->base);

>>> +     if (rc)

>>> +             goto done;

>>> +

>>>        /* Turn engine on */

>>>        etm_clr_pwrdwn(drvdata);

>>>        /* Apply power to trace registers */

>>>        etm_set_pwrup(drvdata);

>>>        /* Make sure all registers are accessible */

>>>        etm_os_unlock(drvdata);

>>> -     rc = coresight_claim_device_unlocked(drvdata->base);

>>> -     if (rc)

>>> -             goto done;

>>>

>>>        etm_set_prog(drvdata);

>>>

>>> @@ -422,12 +423,11 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)

>>>        etm_clr_prog(drvdata);

>>>

>>>    done:

>>> -     if (rc)

>>> -             etm_set_pwrdwn(drvdata);

>>>        CS_LOCK(drvdata->base);

>>>

>>> -     dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",

>>> -             drvdata->cpu, rc);

>>> +     if (!rc)

>>> +             dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",

>>> +                     drvdata->cpu, rc);

>>

>> Isn't it good to report the failure case too ? Anyway it is dev_dbg and

>> will be a useful info when we debug issues. Otherwise,

> 

> Simply removing the "if (!rc)" will do the trick.  Can I do the

> modification and add your tag or you prefer to see another revision?


I am fine with that change folded in, no need to respin it.

> 

>>

>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>



Cheers
Suzuki


>>

>>>        return rc;

>>>    }

>>>

>>> @@ -577,9 +577,9 @@ static void etm_disable_hw(void *info)

>>>        for (i = 0; i < drvdata->nr_cntr; i++)

>>>                config->cntr_val[i] = etm_readl(drvdata, ETMCNTVRn(i));

>>>

>>> +     etm_set_pwrdwn(drvdata);

>>>        coresight_disclaim_device_unlocked(drvdata->base);

>>>

>>> -     etm_set_pwrdwn(drvdata);

>>>        CS_LOCK(drvdata->base);

>>>

>>>        dev_dbg(drvdata->dev, "cpu: %d disable smp call done\n", drvdata->cpu);

>>>

>>
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c
index fd5c4cca7db5..4f638d81a66a 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x.c
@@ -363,15 +363,16 @@  static int etm_enable_hw(struct etm_drvdata *drvdata)
 
 	CS_UNLOCK(drvdata->base);
 
+	rc = coresight_claim_device_unlocked(drvdata->base);
+	if (rc)
+		goto done;
+
 	/* Turn engine on */
 	etm_clr_pwrdwn(drvdata);
 	/* Apply power to trace registers */
 	etm_set_pwrup(drvdata);
 	/* Make sure all registers are accessible */
 	etm_os_unlock(drvdata);
-	rc = coresight_claim_device_unlocked(drvdata->base);
-	if (rc)
-		goto done;
 
 	etm_set_prog(drvdata);
 
@@ -422,12 +423,11 @@  static int etm_enable_hw(struct etm_drvdata *drvdata)
 	etm_clr_prog(drvdata);
 
 done:
-	if (rc)
-		etm_set_pwrdwn(drvdata);
 	CS_LOCK(drvdata->base);
 
-	dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",
-		drvdata->cpu, rc);
+	if (!rc)
+		dev_dbg(drvdata->dev, "cpu: %d enable smp call done: %d\n",
+			drvdata->cpu, rc);
 	return rc;
 }
 
@@ -577,9 +577,9 @@  static void etm_disable_hw(void *info)
 	for (i = 0; i < drvdata->nr_cntr; i++)
 		config->cntr_val[i] = etm_readl(drvdata, ETMCNTVRn(i));
 
+	etm_set_pwrdwn(drvdata);
 	coresight_disclaim_device_unlocked(drvdata->base);
 
-	etm_set_pwrdwn(drvdata);
 	CS_LOCK(drvdata->base);
 
 	dev_dbg(drvdata->dev, "cpu: %d disable smp call done\n", drvdata->cpu);