[PATCH v2 0/3] Extend logging on TMC start / stop errors

[PATCH v2 0/3] Extend logging on TMC start / stop errors

[Mike Leach]

Extend logging on TMC start / stop errors

Recent issues with trying to debug TMC timeouts and flush issues shows
a general lack of logging and context around the possible errors

Add logging to general wait for stop coresight routines and return values of
watched registers.

Update TMC to use this logging.

Changes since v1:
Rebase to coresight/next (kernel 6.13-rc2)

Mike Leach (3):
  coresight: Update timeout functions to allow return of test register
    value
  coresight: tmc: Update error logging in tmc common functions
  coresight: etf: etr: Update logging around flush_and_stop() errors

 drivers/hwtracing/coresight/coresight-core.c  | 50 +++++++++++++++----
 .../hwtracing/coresight/coresight-tmc-core.c  | 37 +++++++++++---
 .../hwtracing/coresight/coresight-tmc-etf.c   | 12 +++--
 .../hwtracing/coresight/coresight-tmc-etr.c   |  8 ++-
 drivers/hwtracing/coresight/coresight-tmc.h   |  2 +-
 include/linux/coresight.h                     |  2 +
 6 files changed, 86 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value

[Mike Leach]

Current coresight_timeout function spins on a bit on a test register,
till bit value achieved or timeout hit.

Add another function to return the full value of the register being
tested.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight-core.c | 50 +++++++++++++++-----
 include/linux/coresight.h                    |  2 +
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index ea38ecf26fcb..feb1a1db355f 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1017,32 +1017,37 @@ static void coresight_remove_conns(struct coresight_device *csdev)
 }
 
 /**
- * coresight_timeout - loop until a bit has changed to a specific register
- *			state.
+ * coresight_timeout_retval - loop until a bit has changed to a specific register
+ *			      state. Return final register value
  * @csa: coresight device access for the device
  * @offset: Offset of the register from the base of the device.
  * @position: the position of the bit of interest.
  * @value: the value the bit should have.
+ * @rval:  the last read value of the register being tested.
  *
  * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
  * TIMEOUT_US has elapsed, which ever happens first.
  */
-int coresight_timeout(struct csdev_access *csa, u32 offset,
-		      int position, int value)
+int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
+			     int position, int value, u32 *rval)
 {
-	int i;
-	u32 val;
+	int i, rc = -EAGAIN;
+	u32 val = 0;
 
 	for (i = TIMEOUT_US; i > 0; i--) {
 		val = csdev_access_read32(csa, offset);
 		/* waiting on the bit to go from 0 to 1 */
 		if (value) {
-			if (val & BIT(position))
-				return 0;
+			if (val & BIT(position)) {
+				rc = 0;
+				goto return_rval;
+			}
 		/* waiting on the bit to go from 1 to 0 */
 		} else {
-			if (!(val & BIT(position)))
-				return 0;
+			if (!(val & BIT(position))) {
+				rc = 0;
+				goto return_rval;
+			}
 		}
 
 		/*
@@ -1054,7 +1059,30 @@ int coresight_timeout(struct csdev_access *csa, u32 offset,
 			udelay(1);
 	}
 
-	return -EAGAIN;
+return_rval:
+	*rval = val;
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(coresight_timeout_retval);
+
+/**
+ * coresight_timeout - loop until a bit has changed to a specific register
+ *		       state
+ * @csa: coresight device access for the device
+ * @offset: Offset of the register from the base of the device.
+ * @position: the position of the bit of interest.
+ * @value: the value the bit should have.
+ *
+ * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
+ * TIMEOUT_US has elapsed, which ever happens first.
+ */
+int coresight_timeout(struct csdev_access *csa, u32 offset,
+		      int position, int value)
+{
+	u32 rval = 0;
+
+	return coresight_timeout_retval(csa, offset, position, value, &rval);
 }
 EXPORT_SYMBOL_GPL(coresight_timeout);
 
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 055ce5cd5c44..29cb71e82b0b 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -639,6 +639,8 @@ extern int coresight_enable_sysfs(struct coresight_device *csdev);
 extern void coresight_disable_sysfs(struct coresight_device *csdev);
 extern int coresight_timeout(struct csdev_access *csa, u32 offset,
 			     int position, int value);
+extern int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
+				    int position, int value, u32 *rval);
 
 extern int coresight_claim_device(struct coresight_device *csdev);
 extern int coresight_claim_device_unlocked(struct coresight_device *csdev);
-- 
2.25.1

[PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions

[Mike Leach]

Enhance the error logging in the tmc_wait_for_tmcready() and
tmc_flush_and_stop() to print key tmc  register values on error
conditions to improve hardware debug information.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 .../hwtracing/coresight/coresight-tmc-core.c  | 37 +++++++++++++++----
 drivers/hwtracing/coresight/coresight-tmc.h   |  2 +-
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index e9876252a789..1a9299adae93 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -34,25 +34,36 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
 DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
 DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
 
+#define TMC_WAIT_READY_FMT_STR "timeout while waiting for TMC to be Ready [STS=0x%04x]\n"
+
 int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
 {
 	struct coresight_device *csdev = drvdata->csdev;
 	struct csdev_access *csa = &csdev->access;
+	u32 tmc_sts = 0;
 
 	/* Ensure formatter, unformatter and hardware fifo are empty */
-	if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
-		dev_err(&csdev->dev,
-			"timeout while waiting for TMC to be Ready\n");
+	if (coresight_timeout_retval(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1,
+				     &tmc_sts)) {
+		dev_err(&csdev->dev, TMC_WAIT_READY_FMT_STR, tmc_sts);
 		return -EBUSY;
 	}
 	return 0;
 }
 
-void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
+int tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 {
 	struct coresight_device *csdev = drvdata->csdev;
 	struct csdev_access *csa = &csdev->access;
-	u32 ffcr;
+	u32 ffcr, ffsr, tmc_sts;
+	int rc = 0;
+
+	/* note any MemErr present when stopping TMC */
+	tmc_sts = readl_relaxed(drvdata->base + TMC_STS);
+	if (tmc_sts & TMC_STS_MEMERR)
+		dev_err(&csdev->dev,
+			"MemErr detected before Manual Flush; STS[0x%02x]\n",
+			tmc_sts);
 
 	ffcr = readl_relaxed(drvdata->base + TMC_FFCR);
 	ffcr |= TMC_FFCR_STOP_ON_FLUSH;
@@ -60,12 +71,22 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 	ffcr |= BIT(TMC_FFCR_FLUSHMAN_BIT);
 	writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
 	/* Ensure flush completes */
-	if (coresight_timeout(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
+	if (coresight_timeout_retval(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0,
+				     &ffcr)) {
+		ffsr = readl_relaxed(drvdata->base + TMC_FFSR);
 		dev_err(&csdev->dev,
-		"timeout while waiting for completion of Manual Flush\n");
+			"timeout while waiting for completion of Manual Flush\n");
+		dev_err(&csdev->dev,
+			"regs: FFCR[0x%02x] FFSR[0x%02x] STS[0x%02x]\n",
+			ffcr, ffsr, tmc_sts);
+		rc = -EBUSY;
 	}
 
-	tmc_wait_for_tmcready(drvdata);
+	if (tmc_wait_for_tmcready(drvdata)) {
+		dev_err(&csdev->dev, "TMC ready error after Manual flush\n");
+		rc = -EBUSY;
+	}
+	return rc;
 }
 
 void tmc_enable_hw(struct tmc_drvdata *drvdata)
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 2671926be62a..34513f07c3aa 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -259,7 +259,7 @@ struct tmc_sg_table {
 
 /* Generic functions */
 int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
-void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
+int tmc_flush_and_stop(struct tmc_drvdata *drvdata);
 void tmc_enable_hw(struct tmc_drvdata *drvdata);
 void tmc_disable_hw(struct tmc_drvdata *drvdata);
 u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);
-- 
2.25.1

[PATCH v2 3/3] coresight: etf: etr: Update logging around flush_and_stop() errors

[Mike Leach]

Insert additional context around tmc_flush_and_stop() errors.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 12 +++++++++---
 drivers/hwtracing/coresight/coresight-tmc-etr.c |  8 ++++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index d4f641cd9de6..62b4b685c1a1 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -84,7 +84,9 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);
 
-	tmc_flush_and_stop(drvdata);
+	if (tmc_flush_and_stop(drvdata))
+		dev_err(&drvdata->csdev->dev,
+			"Flush and stop error disabling ETB\n");
 	/*
 	 * When operating in sysFS mode the content of the buffer needs to be
 	 * read before the TMC is disabled.
@@ -146,7 +148,9 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 
 	CS_UNLOCK(drvdata->base);
 
-	tmc_flush_and_stop(drvdata);
+	if (tmc_flush_and_stop(drvdata))
+		dev_err(&drvdata->csdev->dev,
+			"Flush and stop error disabling ETF\n");
 	tmc_disable_hw(drvdata);
 	coresight_disclaim_device_unlocked(csdev);
 	CS_LOCK(drvdata->base);
@@ -496,7 +500,9 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
 
 	CS_UNLOCK(drvdata->base);
 
-	tmc_flush_and_stop(drvdata);
+	if (tmc_flush_and_stop(drvdata))
+		dev_err(&drvdata->csdev->dev,
+			"Flush and stop error updating perf buffer\n");
 
 	read_ptr = tmc_read_rrp(drvdata);
 	write_ptr = tmc_read_rwp(drvdata);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index a48bb85d0e7f..122a067d1bb8 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1139,7 +1139,9 @@ static void __tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);
 
-	tmc_flush_and_stop(drvdata);
+	if (tmc_flush_and_stop(drvdata))
+		dev_err(&drvdata->csdev->dev,
+			"Flush and stop error disabling ETR\n");
 	/*
 	 * When operating in sysFS mode the content of the buffer needs to be
 	 * read before the TMC is disabled.
@@ -1578,7 +1580,9 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
 
 	CS_UNLOCK(drvdata->base);
 
-	tmc_flush_and_stop(drvdata);
+	if (tmc_flush_and_stop(drvdata))
+		dev_err(&csdev->dev,
+			"Flush and Stop error updating perf buffer\n");
 	tmc_sync_etr_buf(drvdata);
 
 	CS_LOCK(drvdata->base);
-- 
2.25.1

Re: [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value

[Suzuki K Poulose]

Hi Mike

On 13/12/2024 14:49, Mike Leach wrote:
> Current coresight_timeout function spins on a bit on a test register,
> till bit value achieved or timeout hit.
> 
> Add another function to return the full value of the register being
> tested.
> 

The patch looks good to me. However, this change doesn't bring much 
value, other than avoiding a read at the caller side. I guess it is
simpler to leave things as is and let the caller read the register value
while logging rather than providing another variant.

I am inclined to keep the interface simpler.


> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 50 +++++++++++++++-----
>   include/linux/coresight.h                    |  2 +
>   2 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index ea38ecf26fcb..feb1a1db355f 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1017,32 +1017,37 @@ static void coresight_remove_conns(struct coresight_device *csdev)
>   }
>   
>   /**
> - * coresight_timeout - loop until a bit has changed to a specific register
> - *			state.
> + * coresight_timeout_retval - loop until a bit has changed to a specific register
> + *			      state. Return final register value
>    * @csa: coresight device access for the device
>    * @offset: Offset of the register from the base of the device.
>    * @position: the position of the bit of interest.
>    * @value: the value the bit should have.
> + * @rval:  the last read value of the register being tested.
>    *
>    * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
>    * TIMEOUT_US has elapsed, which ever happens first.
>    */
> -int coresight_timeout(struct csdev_access *csa, u32 offset,
> -		      int position, int value)
> +int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> +			     int position, int value, u32 *rval)
>   {
> -	int i;
> -	u32 val;
> +	int i, rc = -EAGAIN;
> +	u32 val = 0;
>   
>   	for (i = TIMEOUT_US; i > 0; i--) {
>   		val = csdev_access_read32(csa, offset);
>   		/* waiting on the bit to go from 0 to 1 */
>   		if (value) {
> -			if (val & BIT(position))
> -				return 0;
> +			if (val & BIT(position)) {
> +				rc = 0;
> +				goto return_rval;
> +			}
>   		/* waiting on the bit to go from 1 to 0 */
>   		} else {
> -			if (!(val & BIT(position)))
> -				return 0;
> +			if (!(val & BIT(position))) {
> +				rc = 0;
> +				goto return_rval;
> +			}
>   		}
>   
>   		/*
> @@ -1054,7 +1059,30 @@ int coresight_timeout(struct csdev_access *csa, u32 offset,
>   			udelay(1);
>   	}
>   
> -	return -EAGAIN;
> +return_rval:
> +	*rval = val;

super minor nit: we could be flexible in accepting a "NULL" rval and 
thus keep the normal variants pass the NULL straight in. i.e,

	if (rval)
		*rval = val;

> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(coresight_timeout_retval);
> +
> +/**
> + * coresight_timeout - loop until a bit has changed to a specific register
> + *		       state
> + * @csa: coresight device access for the device
> + * @offset: Offset of the register from the base of the device.
> + * @position: the position of the bit of interest.
> + * @value: the value the bit should have.
> + *
> + * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
> + * TIMEOUT_US has elapsed, which ever happens first.
> + */
> +int coresight_timeout(struct csdev_access *csa, u32 offset,
> +		      int position, int value)
> +{
> +	u32 rval = 0;
> +
> +	return coresight_timeout_retval(csa, offset, position, value, &rval);

	return core_timeout_retval(csa, offset, position, value, NULL);

Suzuki

>   }
>   EXPORT_SYMBOL_GPL(coresight_timeout);
>   
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 055ce5cd5c44..29cb71e82b0b 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -639,6 +639,8 @@ extern int coresight_enable_sysfs(struct coresight_device *csdev);
>   extern void coresight_disable_sysfs(struct coresight_device *csdev);
>   extern int coresight_timeout(struct csdev_access *csa, u32 offset,
>   			     int position, int value);
> +extern int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> +				    int position, int value, u32 *rval);
>   
>   extern int coresight_claim_device(struct coresight_device *csdev);
>   extern int coresight_claim_device_unlocked(struct coresight_device *csdev);

Re: [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value

[Mike Leach]

Hi Suzuki,

On Fri, 13 Dec 2024 at 16:38, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Mike
>
> On 13/12/2024 14:49, Mike Leach wrote:
> > Current coresight_timeout function spins on a bit on a test register,
> > till bit value achieved or timeout hit.
> >
> > Add another function to return the full value of the register being
> > tested.
> >
>
> The patch looks good to me. However, this change doesn't bring much
> value, other than avoiding a read at the caller side. I guess it is
> simpler to leave things as is and let the caller read the register value
> while logging rather than providing another variant.
>

The reason for returning the register value is that you get the exact
read value used at the time of the test - not potentially a different
value read later.


> I am inclined to keep the interface simpler.
>
>
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >   drivers/hwtracing/coresight/coresight-core.c | 50 +++++++++++++++-----
> >   include/linux/coresight.h                    |  2 +
> >   2 files changed, 41 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index ea38ecf26fcb..feb1a1db355f 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -1017,32 +1017,37 @@ static void coresight_remove_conns(struct coresight_device *csdev)
> >   }
> >
> >   /**
> > - * coresight_timeout - loop until a bit has changed to a specific register
> > - *                   state.
> > + * coresight_timeout_retval - loop until a bit has changed to a specific register
> > + *                         state. Return final register value
> >    * @csa: coresight device access for the device
> >    * @offset: Offset of the register from the base of the device.
> >    * @position: the position of the bit of interest.
> >    * @value: the value the bit should have.
> > + * @rval:  the last read value of the register being tested.
> >    *
> >    * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
> >    * TIMEOUT_US has elapsed, which ever happens first.
> >    */
> > -int coresight_timeout(struct csdev_access *csa, u32 offset,
> > -                   int position, int value)
> > +int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> > +                          int position, int value, u32 *rval)
> >   {
> > -     int i;
> > -     u32 val;
> > +     int i, rc = -EAGAIN;
> > +     u32 val = 0;
> >
> >       for (i = TIMEOUT_US; i > 0; i--) {
> >               val = csdev_access_read32(csa, offset);
> >               /* waiting on the bit to go from 0 to 1 */
> >               if (value) {
> > -                     if (val & BIT(position))
> > -                             return 0;
> > +                     if (val & BIT(position)) {
> > +                             rc = 0;
> > +                             goto return_rval;
> > +                     }
> >               /* waiting on the bit to go from 1 to 0 */
> >               } else {
> > -                     if (!(val & BIT(position)))
> > -                             return 0;
> > +                     if (!(val & BIT(position))) {
> > +                             rc = 0;
> > +                             goto return_rval;
> > +                     }
> >               }
> >
> >               /*
> > @@ -1054,7 +1059,30 @@ int coresight_timeout(struct csdev_access *csa, u32 offset,
> >                       udelay(1);
> >       }
> >
> > -     return -EAGAIN;
> > +return_rval:
> > +     *rval = val;
>
> super minor nit: we could be flexible in accepting a "NULL" rval and
> thus keep the normal variants pass the NULL straight in. i.e,
>
>         if (rval)
>                 *rval = val;
>

Sure.

Mike
> > +
> > +     return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(coresight_timeout_retval);
> > +
> > +/**
> > + * coresight_timeout - loop until a bit has changed to a specific register
> > + *                  state
> > + * @csa: coresight device access for the device
> > + * @offset: Offset of the register from the base of the device.
> > + * @position: the position of the bit of interest.
> > + * @value: the value the bit should have.
> > + *
> > + * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
> > + * TIMEOUT_US has elapsed, which ever happens first.
> > + */
> > +int coresight_timeout(struct csdev_access *csa, u32 offset,
> > +                   int position, int value)
> > +{
> > +     u32 rval = 0;
> > +
> > +     return coresight_timeout_retval(csa, offset, position, value, &rval);
>
>         return core_timeout_retval(csa, offset, position, value, NULL);
>
> Suzuki
>
> >   }
> >   EXPORT_SYMBOL_GPL(coresight_timeout);
> >
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index 055ce5cd5c44..29cb71e82b0b 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -639,6 +639,8 @@ extern int coresight_enable_sysfs(struct coresight_device *csdev);
> >   extern void coresight_disable_sysfs(struct coresight_device *csdev);
> >   extern int coresight_timeout(struct csdev_access *csa, u32 offset,
> >                            int position, int value);
> > +extern int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> > +                                 int position, int value, u32 *rval);
> >
> >   extern int coresight_claim_device(struct coresight_device *csdev);
> >   extern int coresight_claim_device_unlocked(struct coresight_device *csdev);
>


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

Re: [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions

[Suzuki K Poulose]

On 13/12/2024 14:49, Mike Leach wrote:
> Enhance the error logging in the tmc_wait_for_tmcready() and
> tmc_flush_and_stop() to print key tmc  register values on error
> conditions to improve hardware debug information.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>   .../hwtracing/coresight/coresight-tmc-core.c  | 37 +++++++++++++++----
>   drivers/hwtracing/coresight/coresight-tmc.h   |  2 +-
>   2 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index e9876252a789..1a9299adae93 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -34,25 +34,36 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
>   DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
>   DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
>   
> +#define TMC_WAIT_READY_FMT_STR "timeout while waiting for TMC to be Ready [STS=0x%04x]\n"
> +
>   int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>   {
>   	struct coresight_device *csdev = drvdata->csdev;
>   	struct csdev_access *csa = &csdev->access;
> +	u32 tmc_sts = 0;
>   
>   	/* Ensure formatter, unformatter and hardware fifo are empty */
> -	if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
> -		dev_err(&csdev->dev,
> -			"timeout while waiting for TMC to be Ready\n");
> +	if (coresight_timeout_retval(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1,
> +				     &tmc_sts)) {
> +		dev_err(&csdev->dev, TMC_WAIT_READY_FMT_STR, tmc_sts);
>   		return -EBUSY;
>   	}
>   	return 0;
>   }
>   
> -void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> +int tmc_flush_and_stop(struct tmc_drvdata *drvdata)
>   {
>   	struct coresight_device *csdev = drvdata->csdev;
>   	struct csdev_access *csa = &csdev->access;
> -	u32 ffcr;
> +	u32 ffcr, ffsr, tmc_sts;
> +	int rc = 0;
> +
> +	/* note any MemErr present when stopping TMC */
> +	tmc_sts = readl_relaxed(drvdata->base + TMC_STS);
> +	if (tmc_sts & TMC_STS_MEMERR)
> +		dev_err(&csdev->dev,
> +			"MemErr detected before Manual Flush; STS[0x%02x]\n",
> +			tmc_sts);
>   
>   	ffcr = readl_relaxed(drvdata->base + TMC_FFCR);
>   	ffcr |= TMC_FFCR_STOP_ON_FLUSH;
> @@ -60,12 +71,22 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
>   	ffcr |= BIT(TMC_FFCR_FLUSHMAN_BIT);
>   	writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
>   	/* Ensure flush completes */
> -	if (coresight_timeout(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
> +	if (coresight_timeout_retval(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0,
> +				     &ffcr)) {
> +		ffsr = readl_relaxed(drvdata->base + TMC_FFSR);
>   		dev_err(&csdev->dev,
> -		"timeout while waiting for completion of Manual Flush\n");
> +			"timeout while waiting for completion of Manual Flush\n");
> +		dev_err(&csdev->dev,
> +			"regs: FFCR[0x%02x] FFSR[0x%02x] STS[0x%02x]\n",
> +			ffcr, ffsr, tmc_sts);
> +		rc = -EBUSY;
>   	}
>   
> -	tmc_wait_for_tmcready(drvdata);
> +	if (tmc_wait_for_tmcready(drvdata)) {
> +		dev_err(&csdev->dev, "TMC ready error after Manual flush\n");
> +		rc = -EBUSY;
> +	}
> +	return rc;
>   }

All of this looks good to me. Do we need to move the MemErr check post 
the flush ? There is a potential chance of hitting a MemERR on flushing
and we could miss reproting those ones ?

Suzuki

>   void tmc_enable_hw(struct tmc_drvdata *drvdata)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 2671926be62a..34513f07c3aa 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -259,7 +259,7 @@ struct tmc_sg_table {
>   
>   /* Generic functions */
>   int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
> -void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
> +int tmc_flush_and_stop(struct tmc_drvdata *drvdata);
>   void tmc_enable_hw(struct tmc_drvdata *drvdata);
>   void tmc_disable_hw(struct tmc_drvdata *drvdata);
>   u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);

Re: [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions

[Mike Leach]

Hi Suzuki,

On Mon, 16 Dec 2024 at 09:33, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 13/12/2024 14:49, Mike Leach wrote:
> > Enhance the error logging in the tmc_wait_for_tmcready() and
> > tmc_flush_and_stop() to print key tmc  register values on error
> > conditions to improve hardware debug information.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >   .../hwtracing/coresight/coresight-tmc-core.c  | 37 +++++++++++++++----
> >   drivers/hwtracing/coresight/coresight-tmc.h   |  2 +-
> >   2 files changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > index e9876252a789..1a9299adae93 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > @@ -34,25 +34,36 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
> >   DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
> >   DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
> >
> > +#define TMC_WAIT_READY_FMT_STR "timeout while waiting for TMC to be Ready [STS=0x%04x]\n"
> > +
> >   int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> >   {
> >       struct coresight_device *csdev = drvdata->csdev;
> >       struct csdev_access *csa = &csdev->access;
> > +     u32 tmc_sts = 0;
> >
> >       /* Ensure formatter, unformatter and hardware fifo are empty */
> > -     if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
> > -             dev_err(&csdev->dev,
> > -                     "timeout while waiting for TMC to be Ready\n");
> > +     if (coresight_timeout_retval(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1,
> > +                                  &tmc_sts)) {
> > +             dev_err(&csdev->dev, TMC_WAIT_READY_FMT_STR, tmc_sts);
> >               return -EBUSY;
> >       }
> >       return 0;
> >   }
> >
> > -void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> > +int tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> >   {
> >       struct coresight_device *csdev = drvdata->csdev;
> >       struct csdev_access *csa = &csdev->access;
> > -     u32 ffcr;
> > +     u32 ffcr, ffsr, tmc_sts;
> > +     int rc = 0;
> > +
> > +     /* note any MemErr present when stopping TMC */
> > +     tmc_sts = readl_relaxed(drvdata->base + TMC_STS);
> > +     if (tmc_sts & TMC_STS_MEMERR)
> > +             dev_err(&csdev->dev,
> > +                     "MemErr detected before Manual Flush; STS[0x%02x]\n",
> > +                     tmc_sts);
> >
> >       ffcr = readl_relaxed(drvdata->base + TMC_FFCR);
> >       ffcr |= TMC_FFCR_STOP_ON_FLUSH;
> > @@ -60,12 +71,22 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> >       ffcr |= BIT(TMC_FFCR_FLUSHMAN_BIT);
> >       writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
> >       /* Ensure flush completes */
> > -     if (coresight_timeout(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
> > +     if (coresight_timeout_retval(csa, TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0,
> > +                                  &ffcr)) {
> > +             ffsr = readl_relaxed(drvdata->base + TMC_FFSR);
> >               dev_err(&csdev->dev,
> > -             "timeout while waiting for completion of Manual Flush\n");
> > +                     "timeout while waiting for completion of Manual Flush\n");
> > +             dev_err(&csdev->dev,
> > +                     "regs: FFCR[0x%02x] FFSR[0x%02x] STS[0x%02x]\n",
> > +                     ffcr, ffsr, tmc_sts);
> > +             rc = -EBUSY;
> >       }
> >
> > -     tmc_wait_for_tmcready(drvdata);
> > +     if (tmc_wait_for_tmcready(drvdata)) {
> > +             dev_err(&csdev->dev, "TMC ready error after Manual flush\n");
> > +             rc = -EBUSY;
> > +     }
> > +     return rc;
> >   }
>
> All of this looks good to me. Do we need to move the MemErr check post
> the flush ? There is a potential chance of hitting a MemERR on flushing
> and we could miss reproting those ones ?
>

We need the one before - if there is already a Memerr then this will
affect the flush operation - on Memerr the formatter will have stopped
etc. But no harm in putting another check afterwards.

Mike

> Suzuki
>
> >   void tmc_enable_hw(struct tmc_drvdata *drvdata)
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> > index 2671926be62a..34513f07c3aa 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc.h
> > +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> > @@ -259,7 +259,7 @@ struct tmc_sg_table {
> >
> >   /* Generic functions */
> >   int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
> > -void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
> > +int tmc_flush_and_stop(struct tmc_drvdata *drvdata);
> >   void tmc_enable_hw(struct tmc_drvdata *drvdata);
> >   void tmc_disable_hw(struct tmc_drvdata *drvdata);
> >   u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);
>


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