* [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value
2024-12-13 14:49 [PATCH v2 0/3] Extend logging on TMC start / stop errors Mike Leach
@ 2024-12-13 14:49 ` Mike Leach
2024-12-13 16:38 ` Suzuki K Poulose
2024-12-13 14:49 ` [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions Mike Leach
2024-12-13 14:49 ` [PATCH v2 3/3] coresight: etf: etr: Update logging around flush_and_stop() errors Mike Leach
2 siblings, 1 reply; 8+ messages in thread
From: Mike Leach @ 2024-12-13 14:49 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, coresight
Cc: james.clark, mike.leach, suzuki.poulose, alexander.shishkin
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value
2024-12-13 14:49 ` [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value Mike Leach
@ 2024-12-13 16:38 ` Suzuki K Poulose
2024-12-13 18:53 ` Mike Leach
0 siblings, 1 reply; 8+ messages in thread
From: Suzuki K Poulose @ 2024-12-13 16:38 UTC (permalink / raw)
To: Mike Leach, linux-kernel, linux-arm-kernel, coresight
Cc: james.clark, alexander.shishkin
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);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value
2024-12-13 16:38 ` Suzuki K Poulose
@ 2024-12-13 18:53 ` Mike Leach
0 siblings, 0 replies; 8+ messages in thread
From: Mike Leach @ 2024-12-13 18:53 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: linux-kernel, linux-arm-kernel, coresight, james.clark,
alexander.shishkin
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions
2024-12-13 14:49 [PATCH v2 0/3] Extend logging on TMC start / stop errors Mike Leach
2024-12-13 14:49 ` [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value Mike Leach
@ 2024-12-13 14:49 ` Mike Leach
2024-12-16 9:33 ` Suzuki K Poulose
2024-12-13 14:49 ` [PATCH v2 3/3] coresight: etf: etr: Update logging around flush_and_stop() errors Mike Leach
2 siblings, 1 reply; 8+ messages in thread
From: Mike Leach @ 2024-12-13 14:49 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, coresight
Cc: james.clark, mike.leach, suzuki.poulose, alexander.shishkin
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions
2024-12-13 14:49 ` [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions Mike Leach
@ 2024-12-16 9:33 ` Suzuki K Poulose
2024-12-17 11:41 ` Mike Leach
0 siblings, 1 reply; 8+ messages in thread
From: Suzuki K Poulose @ 2024-12-16 9:33 UTC (permalink / raw)
To: Mike Leach, linux-kernel, linux-arm-kernel, coresight
Cc: james.clark, alexander.shishkin
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);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions
2024-12-16 9:33 ` Suzuki K Poulose
@ 2024-12-17 11:41 ` Mike Leach
0 siblings, 0 replies; 8+ messages in thread
From: Mike Leach @ 2024-12-17 11:41 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: linux-kernel, linux-arm-kernel, coresight, james.clark,
alexander.shishkin
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] coresight: etf: etr: Update logging around flush_and_stop() errors
2024-12-13 14:49 [PATCH v2 0/3] Extend logging on TMC start / stop errors Mike Leach
2024-12-13 14:49 ` [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value Mike Leach
2024-12-13 14:49 ` [PATCH v2 2/3] coresight: tmc: Update error logging in tmc common functions Mike Leach
@ 2024-12-13 14:49 ` Mike Leach
2 siblings, 0 replies; 8+ messages in thread
From: Mike Leach @ 2024-12-13 14:49 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, coresight
Cc: james.clark, mike.leach, suzuki.poulose, alexander.shishkin
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
^ permalink raw reply [flat|nested] 8+ messages in thread