.../coresight/coresight-etm4x-core.c | 13 ++++- drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++ include/linux/coresight.h | 6 +++ 3 files changed, 71 insertions(+), 1 deletion(-)
Similar to ETE, TRBE may lose its context when a CPU enters low
power state. To make things worse, if ETE state is restored without
restoring TRBE state, it can lead to a stuck CPU due to an enabled
source device with no enabled sink devices.
This patch introduces support for "arm,coresight-loses-context-with-cpu"
in the TRBE driver. When present, TRBE registers are saved before
and restored after CPU low power state. To prevent CPU hangs, TRBE
state is always saved after ETE state and restored after ETE state.
Signed-off-by: Yabin Cui <yabinc@google.com>
---
.../coresight/coresight-etm4x-core.c | 13 ++++-
drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++
include/linux/coresight.h | 6 +++
3 files changed, 71 insertions(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index e5972f16abff..1bbaa1249206 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
{
int ret = 0;
+ struct coresight_device *sink;
/* Save the TRFCR irrespective of whether the ETM is ON */
if (drvdata->trfcr)
@@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
* Save and restore the ETM Trace registers only if
* the ETM is active.
*/
- if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
+ if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
ret = __etm4_cpu_save(drvdata);
+ if (ret == 0) {
+ sink = coresight_get_percpu_sink(drvdata->cpu);
+ if (sink && sink_ops(sink)->percpu_save)
+ sink_ops(sink)->percpu_save(sink);
+ }
+ }
return ret;
}
@@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
{
+ struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
+
+ if (sink && sink_ops(sink)->percpu_restore)
+ sink_ops(sink)->percpu_restore(sink);
if (drvdata->trfcr)
write_trfcr(drvdata->save_trfcr);
if (drvdata->state_needs_restore)
diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index fff67aac8418..38bf46951a82 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
*/
#define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES 256
+struct trbe_save_state {
+ u64 trblimitr;
+ u64 trbbaser;
+ u64 trbptr;
+ u64 trbsr;
+};
+
/*
* struct trbe_cpudata: TRBE instance specific data
* @trbe_flag - TRBE dirty/access flag support
@@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
* @cpu - CPU this TRBE belongs to.
* @mode - Mode of current operation. (perf/disabled)
* @drvdata - TRBE specific drvdata
+ * @state_needs_save - Need to save trace registers when entering cpu idle
+ * @state_needs_restore - Need to restore trace registers when exiting cpu idle
+ * @save_state - Saved trace registers
* @errata - Bit map for the errata on this TRBE.
*/
struct trbe_cpudata {
@@ -133,6 +143,9 @@ struct trbe_cpudata {
enum cs_mode mode;
struct trbe_buf *buf;
struct trbe_drvdata *drvdata;
+ bool state_needs_save;
+ bool state_needs_restore;
+ struct trbe_save_state save_state;
DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
};
@@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
return IRQ_HANDLED;
}
+static void arm_trbe_cpu_save(struct coresight_device *csdev)
+{
+ struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
+ struct trbe_save_state *state = &cpudata->save_state;
+
+ if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
+ return;
+
+ state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
+ state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
+ state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
+ state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
+ cpudata->state_needs_restore = true;
+}
+
+static void arm_trbe_cpu_restore(struct coresight_device *csdev)
+{
+ struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
+ struct trbe_save_state *state = &cpudata->save_state;
+
+ if (cpudata->state_needs_restore) {
+ /*
+ * To avoid disruption of normal tracing, restore trace
+ * registers only when TRBE lost power (TRBLIMITR == 0).
+ */
+ if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
+ write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
+ write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
+ write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
+ set_trbe_enabled(cpudata, state->trblimitr);
+ }
+ cpudata->state_needs_restore = false;
+ }
+}
+
static const struct coresight_ops_sink arm_trbe_sink_ops = {
.enable = arm_trbe_enable,
.disable = arm_trbe_disable,
.alloc_buffer = arm_trbe_alloc_buffer,
.free_buffer = arm_trbe_free_buffer,
.update_buffer = arm_trbe_update_buffer,
+ .percpu_save = arm_trbe_cpu_save,
+ .percpu_restore = arm_trbe_cpu_restore,
};
static const struct coresight_ops arm_trbe_cs_ops = {
@@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
cpudata->trbe_flag = get_trbe_flag_update(trbidr);
cpudata->cpu = cpu;
cpudata->drvdata = drvdata;
+ cpudata->state_needs_save = coresight_loses_context_with_cpu(
+ &drvdata->pdev->dev);
+ cpudata->state_needs_restore = false;
return;
cpu_clear:
cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index d79a242b271d..fec375d02535 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -362,6 +362,10 @@ enum cs_mode {
* @alloc_buffer: initialises perf's ring buffer for trace collection.
* @free_buffer: release memory allocated in @get_config.
* @update_buffer: update buffer pointers after a trace session.
+ * @percpu_save: saves state when CPU enters idle state.
+ * Only set for percpu sink.
+ * @percpu_restore: restores state when CPU exits idle state.
+ * only set for percpu sink.
*/
struct coresight_ops_sink {
int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
@@ -374,6 +378,8 @@ struct coresight_ops_sink {
unsigned long (*update_buffer)(struct coresight_device *csdev,
struct perf_output_handle *handle,
void *sink_config);
+ void (*percpu_save)(struct coresight_device *csdev);
+ void (*percpu_restore)(struct coresight_device *csdev);
};
/**
--
2.49.0.901.g37484f566f-googOn 4/24/25 04:30, Yabin Cui wrote:
> Similar to ETE, TRBE may lose its context when a CPU enters low
> power state. To make things worse, if ETE state is restored without
> restoring TRBE state, it can lead to a stuck CPU due to an enabled
> source device with no enabled sink devices.
Could you please provide some more details about when the cpu gets
stuck e.g dmesg, traces etc. Also consider adding those details in
the commit message as well to establish the problem, this patch is
trying to address.
>
> This patch introduces support for "arm,coresight-loses-context-with-cpu"
> in the TRBE driver. When present, TRBE registers are saved before
> and restored after CPU low power state. To prevent CPU hangs, TRBE
> state is always saved after ETE state and restored after ETE state.
The save and restore order here is
1. Save ETE
2. Save TRBE
3. Restore ETE
4. Restore TRBE
>
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> .../coresight/coresight-etm4x-core.c | 13 ++++-
> drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++
> include/linux/coresight.h | 6 +++
> 3 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e5972f16abff..1bbaa1249206 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> {
> int ret = 0;
> + struct coresight_device *sink;
>
> /* Save the TRFCR irrespective of whether the ETM is ON */
> if (drvdata->trfcr)
> @@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> * Save and restore the ETM Trace registers only if
> * the ETM is active.
> */
> - if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
> + if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
> ret = __etm4_cpu_save(drvdata);
> + if (ret == 0) {
> + sink = coresight_get_percpu_sink(drvdata->cpu);
> + if (sink && sink_ops(sink)->percpu_save)
> + sink_ops(sink)->percpu_save(sink);
> + }
> + }
> return ret;
> }
>
> @@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>
> static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> {
> + struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
> +
> + if (sink && sink_ops(sink)->percpu_restore)
> + sink_ops(sink)->percpu_restore(sink);
TRBE gets restored first which contradicts the order mentioned
earlier in the commit message ?
> if (drvdata->trfcr)
> write_trfcr(drvdata->save_trfcr);
> if (drvdata->state_needs_restore)
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index fff67aac8418..38bf46951a82 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
> */
> #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES 256
>
> +struct trbe_save_state {
> + u64 trblimitr;
> + u64 trbbaser;
> + u64 trbptr;
> + u64 trbsr;
> +};
This seems to accommodate all required TRBE registers. Although this is
very intuitive could you please also add a comment above this structure
explaining the elements like other existing structures in the file ?
> +
> /*
> * struct trbe_cpudata: TRBE instance specific data
> * @trbe_flag - TRBE dirty/access flag support
> @@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
> * @cpu - CPU this TRBE belongs to.
> * @mode - Mode of current operation. (perf/disabled)
> * @drvdata - TRBE specific drvdata
> + * @state_needs_save - Need to save trace registers when entering cpu idle
> + * @state_needs_restore - Need to restore trace registers when exiting cpu idle
> + * @save_state - Saved trace registers
> * @errata - Bit map for the errata on this TRBE.
> */
> struct trbe_cpudata {
> @@ -133,6 +143,9 @@ struct trbe_cpudata {
> enum cs_mode mode;
> struct trbe_buf *buf;
> struct trbe_drvdata *drvdata;
> + bool state_needs_save;
This tracks whether coresight_loses_context_with_cpu() is supported
on the CPU or not.
> + bool state_needs_restore;
This tracks whether the state has been saved earlier and hence can
be restored later on when required.
> + struct trbe_save_state save_state;
> DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
> };
>
> @@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> return IRQ_HANDLED;
> }
>
> +static void arm_trbe_cpu_save(struct coresight_device *csdev)
> +{
> + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> + struct trbe_save_state *state = &cpudata->save_state;
Please move the above statement after the following conditional
block. Because struct trbe_save_state is not going to be required
if arm_trbe_cpu_save() returns prematurely from here.
> +
> + if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
> + return;> +
> + state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
> + state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
> + state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> + state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> + cpudata->state_needs_restore = true;
This looks right.
> +}
> +
> +static void arm_trbe_cpu_restore(struct coresight_device *csdev)
> +{
> + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> + struct trbe_save_state *state = &cpudata->save_state;
Please move this assignment inside the block where these registers
actually get restored after all checks are done.
> +
> + if (cpudata->state_needs_restore) {
> + /*
> + * To avoid disruption of normal tracing, restore trace
> + * registers only when TRBE lost power (TRBLIMITR == 0).
> + */
Although this sounds like a reasonable condition, but what happens
when TRBE restoration is skipped ?
> + if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
state = &cpudata->save_state
> + write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
> + write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
> + write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
> + set_trbe_enabled(cpudata, state->trblimitr);
> + }
> + cpudata->state_needs_restore = false;
That means earlier captured state is dropped without a restoration.
> + }
> +}
> +
> static const struct coresight_ops_sink arm_trbe_sink_ops = {
> .enable = arm_trbe_enable,
> .disable = arm_trbe_disable,
> .alloc_buffer = arm_trbe_alloc_buffer,
> .free_buffer = arm_trbe_free_buffer,
> .update_buffer = arm_trbe_update_buffer,
> + .percpu_save = arm_trbe_cpu_save,
> + .percpu_restore = arm_trbe_cpu_restore,
Why this percpu_* prefix ?
> };
>
> static const struct coresight_ops arm_trbe_cs_ops = {
> @@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
> cpudata->trbe_flag = get_trbe_flag_update(trbidr);
> cpudata->cpu = cpu;
> cpudata->drvdata = drvdata;
> + cpudata->state_needs_save = coresight_loses_context_with_cpu(
> + &drvdata->pdev->dev);
> + cpudata->state_needs_restore = false;
The init here looks good.
> return;
> cpu_clear:
> cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index d79a242b271d..fec375d02535 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -362,6 +362,10 @@ enum cs_mode {
> * @alloc_buffer: initialises perf's ring buffer for trace collection.
> * @free_buffer: release memory allocated in @get_config.
> * @update_buffer: update buffer pointers after a trace session.
> + * @percpu_save: saves state when CPU enters idle state.
> + * Only set for percpu sink.
> + * @percpu_restore: restores state when CPU exits idle state.
> + * only set for percpu sink.
> */
> struct coresight_ops_sink {
> int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> unsigned long (*update_buffer)(struct coresight_device *csdev,
> struct perf_output_handle *handle,
> void *sink_config);
> + void (*percpu_save)(struct coresight_device *csdev);
> + void (*percpu_restore)(struct coresight_device *csdev);
Again - why this percpu_* prefix ?
> };
>
> /**
On Thu, Apr 24, 2025 at 9:16 PM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> On 4/24/25 04:30, Yabin Cui wrote:
> > Similar to ETE, TRBE may lose its context when a CPU enters low
> > power state. To make things worse, if ETE state is restored without
> > restoring TRBE state, it can lead to a stuck CPU due to an enabled
> > source device with no enabled sink devices.
>
> Could you please provide some more details about when the cpu gets
> stuck e.g dmesg, traces etc. Also consider adding those details in
> the commit message as well to establish the problem, this patch is
> trying to address.
This is already the best I know. When experimenting TRBE locally
(on Pixel 9), if I don't save TRBE state across low power state, when
recording system wide ETM data using TRBE on a CPU core. In a
few seconds, the CPU state becomes unknown (no message in dmesg).
Since the core may hold locks needed by other cores, it soon locks
other cores and causes a watchdog reset. I found the coresight driver
always carefully enables sink before source, and disables sink after
source. I guess there is some risk in not doing so, like the CPU hang
here. Maybe you know why?
>
> >
> > This patch introduces support for "arm,coresight-loses-context-with-cpu"
> > in the TRBE driver. When present, TRBE registers are saved before
> > and restored after CPU low power state. To prevent CPU hangs, TRBE
> > state is always saved after ETE state and restored after ETE state.
>
> The save and restore order here is
>
> 1. Save ETE
> 2. Save TRBE
> 3. Restore ETE
> 4. Restore TRBE
>
> >
> > Signed-off-by: Yabin Cui <yabinc@google.com>
> > ---
> > .../coresight/coresight-etm4x-core.c | 13 ++++-
> > drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++
> > include/linux/coresight.h | 6 +++
> > 3 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e5972f16abff..1bbaa1249206 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > {
> > int ret = 0;
> > + struct coresight_device *sink;
> >
> > /* Save the TRFCR irrespective of whether the ETM is ON */
> > if (drvdata->trfcr)
> > @@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > * Save and restore the ETM Trace registers only if
> > * the ETM is active.
> > */
> > - if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
> > + if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
> > ret = __etm4_cpu_save(drvdata);
> > + if (ret == 0) {
> > + sink = coresight_get_percpu_sink(drvdata->cpu);
> > + if (sink && sink_ops(sink)->percpu_save)
> > + sink_ops(sink)->percpu_save(sink);
> > + }
> > + }
> > return ret;
> > }
> >
> > @@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> >
> > static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> > {
> > + struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
> > +
> > + if (sink && sink_ops(sink)->percpu_restore)
> > + sink_ops(sink)->percpu_restore(sink);
>
> TRBE gets restored first which contradicts the order mentioned
> earlier in the commit message ?
>
An error in the commit message, should be:
To prevent CPU hangs, TRBE state is always saved after ETE state and
restored before ETE state.
>
> > if (drvdata->trfcr)
> > write_trfcr(drvdata->save_trfcr);
> > if (drvdata->state_needs_restore)
> > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > index fff67aac8418..38bf46951a82 100644
> > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > @@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
> > */
> > #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES 256
> >
> > +struct trbe_save_state {
> > + u64 trblimitr;
> > + u64 trbbaser;
> > + u64 trbptr;
> > + u64 trbsr;
> > +};
>
> This seems to accommodate all required TRBE registers. Although this is
> very intuitive could you please also add a comment above this structure
> explaining the elements like other existing structures in the file ?
Will do in the v2 patch.
>
> > +
> > /*
> > * struct trbe_cpudata: TRBE instance specific data
> > * @trbe_flag - TRBE dirty/access flag support
> > @@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
> > * @cpu - CPU this TRBE belongs to.
> > * @mode - Mode of current operation. (perf/disabled)
> > * @drvdata - TRBE specific drvdata
> > + * @state_needs_save - Need to save trace registers when entering cpu idle
> > + * @state_needs_restore - Need to restore trace registers when exiting cpu idle
> > + * @save_state - Saved trace registers
> > * @errata - Bit map for the errata on this TRBE.
> > */
> > struct trbe_cpudata {
> > @@ -133,6 +143,9 @@ struct trbe_cpudata {
> > enum cs_mode mode;
> > struct trbe_buf *buf;
> > struct trbe_drvdata *drvdata;
> > + bool state_needs_save;
>
> This tracks whether coresight_loses_context_with_cpu() is supported
> on the CPU or not.
>
> > + bool state_needs_restore;
>
> This tracks whether the state has been saved earlier and hence can
> be restored later on when required.
Will update the comment in the v2 patch.
>
> > + struct trbe_save_state save_state;
> > DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
> > };
> >
> > @@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> > return IRQ_HANDLED;
> > }
> >
> > +static void arm_trbe_cpu_save(struct coresight_device *csdev)
> > +{
> > + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > + struct trbe_save_state *state = &cpudata->save_state;
>
> Please move the above statement after the following conditional
> block. Because struct trbe_save_state is not going to be required
> if arm_trbe_cpu_save() returns prematurely from here.
>
Will do in the v2 patch.
> > +
> > + if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
> > + return;> +
> > + state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
> > + state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
> > + state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> > + state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> > + cpudata->state_needs_restore = true;
>
> This looks right.
>
> > +}
> > +
> > +static void arm_trbe_cpu_restore(struct coresight_device *csdev)
> > +{
> > + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > + struct trbe_save_state *state = &cpudata->save_state;
> Please move this assignment inside the block where these registers
> actually get restored after all checks are done.
>
Will do in the v2 patch.
> > +
> > + if (cpudata->state_needs_restore) {
> > + /*
> > + * To avoid disruption of normal tracing, restore trace
> > + * registers only when TRBE lost power (TRBLIMITR == 0).
> > + */
>
> Although this sounds like a reasonable condition, but what happens
> when TRBE restoration is skipped ?
>
There are cases when a CPU fails to enter a low power state or only enters
a shallow low power state. So TRBE state may not be lost when calling
arm_trbe_cpu_restore(). In this case, if we write the TRBE registers, probably
we lose ETM data written between save and restore. This may not matter,
but could there be a race condition between this restore function and TRBE
interrupt handler (which could also write TRBE registers)? I can't think
this up very well. So I only restore registers when state is lost. If
the state isn't
lost, TRBE should stay in enabled mode, and continue receiving data from ETE.
> > + if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
>
> state = &cpudata->save_state
>
> > + write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
> > + write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
> > + write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
> > + set_trbe_enabled(cpudata, state->trblimitr);
> > + }
> > + cpudata->state_needs_restore = false;
>
> That means earlier captured state is dropped without a restoration.
>
> > + }
> > +}
> > +
> > static const struct coresight_ops_sink arm_trbe_sink_ops = {
> > .enable = arm_trbe_enable,
> > .disable = arm_trbe_disable,
> > .alloc_buffer = arm_trbe_alloc_buffer,
> > .free_buffer = arm_trbe_free_buffer,
> > .update_buffer = arm_trbe_update_buffer,
> > + .percpu_save = arm_trbe_cpu_save,
> > + .percpu_restore = arm_trbe_cpu_restore,
>
> Why this percpu_* prefix ?
>
Added percpu_ prefix because it's only for percpu_sink
(coresight_get_percpu_sink).
Please let me know if you have a better name.
> > };
> >
> > static const struct coresight_ops arm_trbe_cs_ops = {
> > @@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
> > cpudata->trbe_flag = get_trbe_flag_update(trbidr);
> > cpudata->cpu = cpu;
> > cpudata->drvdata = drvdata;
> > + cpudata->state_needs_save = coresight_loses_context_with_cpu(
> > + &drvdata->pdev->dev);
> > + cpudata->state_needs_restore = false;
>
> The init here looks good.
>
> > return;
> > cpu_clear:
> > cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index d79a242b271d..fec375d02535 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -362,6 +362,10 @@ enum cs_mode {
> > * @alloc_buffer: initialises perf's ring buffer for trace collection.
> > * @free_buffer: release memory allocated in @get_config.
> > * @update_buffer: update buffer pointers after a trace session.
> > + * @percpu_save: saves state when CPU enters idle state.
> > + * Only set for percpu sink.
> > + * @percpu_restore: restores state when CPU exits idle state.
> > + * only set for percpu sink.
> > */
> > struct coresight_ops_sink {
> > int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> > unsigned long (*update_buffer)(struct coresight_device *csdev,
> > struct perf_output_handle *handle,
> > void *sink_config);
> > + void (*percpu_save)(struct coresight_device *csdev);
> > + void (*percpu_restore)(struct coresight_device *csdev);
>
> Again - why this percpu_* prefix ?
>
> > };
> >
> > /**
Hi
On Fri, 25 Apr 2025 at 23:05, Yabin Cui <yabinc@google.com> wrote:
>
> On Thu, Apr 24, 2025 at 9:16 PM Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
> >
> > On 4/24/25 04:30, Yabin Cui wrote:
> > > Similar to ETE, TRBE may lose its context when a CPU enters low
> > > power state. To make things worse, if ETE state is restored without
> > > restoring TRBE state, it can lead to a stuck CPU due to an enabled
> > > source device with no enabled sink devices.
> >
> > Could you please provide some more details about when the cpu gets
> > stuck e.g dmesg, traces etc. Also consider adding those details in
> > the commit message as well to establish the problem, this patch is
> > trying to address.
>
> This is already the best I know. When experimenting TRBE locally
> (on Pixel 9), if I don't save TRBE state across low power state, when
> recording system wide ETM data using TRBE on a CPU core. In a
> few seconds, the CPU state becomes unknown (no message in dmesg).
> Since the core may hold locks needed by other cores, it soon locks
> other cores and causes a watchdog reset. I found the coresight driver
> always carefully enables sink before source, and disables sink after
> source. I guess there is some risk in not doing so, like the CPU hang
> here. Maybe you know why?
>
>
> >
> > >
> > > This patch introduces support for "arm,coresight-loses-context-with-cpu"
> > > in the TRBE driver. When present, TRBE registers are saved before
> > > and restored after CPU low power state. To prevent CPU hangs, TRBE
> > > state is always saved after ETE state and restored after ETE state.
> >
> > The save and restore order here is
> >
> > 1. Save ETE
> > 2. Save TRBE
> > 3. Restore ETE
> > 4. Restore TRBE
> >
> > >
> > > Signed-off-by: Yabin Cui <yabinc@google.com>
> > > ---
> > > .../coresight/coresight-etm4x-core.c | 13 ++++-
> > > drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++
> > > include/linux/coresight.h | 6 +++
> > > 3 files changed, 71 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > index e5972f16abff..1bbaa1249206 100644
> > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > @@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > > static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > > {
> > > int ret = 0;
> > > + struct coresight_device *sink;
> > >
> > > /* Save the TRFCR irrespective of whether the ETM is ON */
> > > if (drvdata->trfcr)
> > > @@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > > * Save and restore the ETM Trace registers only if
> > > * the ETM is active.
> > > */
> > > - if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
> > > + if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
> > > ret = __etm4_cpu_save(drvdata);
> > > + if (ret == 0) {
> > > + sink = coresight_get_percpu_sink(drvdata->cpu);
> > > + if (sink && sink_ops(sink)->percpu_save)
> > > + sink_ops(sink)->percpu_save(sink);
> > > + }
> > > + }
> > > return ret;
> > > }
> > >
> > > @@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> > >
> > > static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> > > {
> > > + struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
> > > +
> > > + if (sink && sink_ops(sink)->percpu_restore)
> > > + sink_ops(sink)->percpu_restore(sink);
> >
> > TRBE gets restored first which contradicts the order mentioned
> > earlier in the commit message ?
> >
> An error in the commit message, should be:
> To prevent CPU hangs, TRBE state is always saved after ETE state and
> restored before ETE state.
>
> >
> > > if (drvdata->trfcr)
> > > write_trfcr(drvdata->save_trfcr);
> > > if (drvdata->state_needs_restore)
> > > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > > index fff67aac8418..38bf46951a82 100644
> > > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > > @@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
> > > */
> > > #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES 256
> > >
> > > +struct trbe_save_state {
> > > + u64 trblimitr;
> > > + u64 trbbaser;
> > > + u64 trbptr;
> > > + u64 trbsr;
> > > +};
> >
> > This seems to accommodate all required TRBE registers. Although this is
> > very intuitive could you please also add a comment above this structure
> > explaining the elements like other existing structures in the file ?
>
> Will do in the v2 patch.
> >
> > > +
> > > /*
> > > * struct trbe_cpudata: TRBE instance specific data
> > > * @trbe_flag - TRBE dirty/access flag support
> > > @@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
> > > * @cpu - CPU this TRBE belongs to.
> > > * @mode - Mode of current operation. (perf/disabled)
> > > * @drvdata - TRBE specific drvdata
> > > + * @state_needs_save - Need to save trace registers when entering cpu idle
> > > + * @state_needs_restore - Need to restore trace registers when exiting cpu idle
> > > + * @save_state - Saved trace registers
> > > * @errata - Bit map for the errata on this TRBE.
> > > */
> > > struct trbe_cpudata {
> > > @@ -133,6 +143,9 @@ struct trbe_cpudata {
> > > enum cs_mode mode;
> > > struct trbe_buf *buf;
> > > struct trbe_drvdata *drvdata;
> > > + bool state_needs_save;
> >
> > This tracks whether coresight_loses_context_with_cpu() is supported
> > on the CPU or not.
> >
> > > + bool state_needs_restore;
> >
> > This tracks whether the state has been saved earlier and hence can
> > be restored later on when required.
>
> Will update the comment in the v2 patch.
>
> >
> > > + struct trbe_save_state save_state;
> > > DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
> > > };
> > >
> > > @@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> > > return IRQ_HANDLED;
> > > }
> > >
> > > +static void arm_trbe_cpu_save(struct coresight_device *csdev)
> > > +{
> > > + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > > + struct trbe_save_state *state = &cpudata->save_state;
> >
> > Please move the above statement after the following conditional
> > block. Because struct trbe_save_state is not going to be required
> > if arm_trbe_cpu_save() returns prematurely from here.
> >
> Will do in the v2 patch.
> > > +
> > > + if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
> > > + return;> +
> > > + state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
> > > + state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
> > > + state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> > > + state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> > > + cpudata->state_needs_restore = true;
> >
> > This looks right.
> >
> > > +}
> > > +
> > > +static void arm_trbe_cpu_restore(struct coresight_device *csdev)
> > > +{
> > > + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > > + struct trbe_save_state *state = &cpudata->save_state;
> > Please move this assignment inside the block where these registers
> > actually get restored after all checks are done.
> >
> Will do in the v2 patch.
> > > +
> > > + if (cpudata->state_needs_restore) {
> > > + /*
> > > + * To avoid disruption of normal tracing, restore trace
> > > + * registers only when TRBE lost power (TRBLIMITR == 0).
> > > + */
> >
> > Although this sounds like a reasonable condition, but what happens
> > when TRBE restoration is skipped ?
> >
> There are cases when a CPU fails to enter a low power state or only enters
> a shallow low power state. So TRBE state may not be lost when calling
> arm_trbe_cpu_restore(). In this case, if we write the TRBE registers, probably
> we lose ETM data written between save and restore. This may not matter,
> but could there be a race condition between this restore function and TRBE
> interrupt handler (which could also write TRBE registers)? I can't think
> this up very well. So I only restore registers when state is lost. If
> the state isn't
> lost, TRBE should stay in enabled mode, and continue receiving data from ETE.
>
> > > + if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
> >
> > state = &cpudata->save_state
> >
> > > + write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
> > > + write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
> > > + write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
> > > + set_trbe_enabled(cpudata, state->trblimitr);
> > > + }
> > > + cpudata->state_needs_restore = false;
> >
> > That means earlier captured state is dropped without a restoration.
> >
> > > + }
> > > +}
> > > +
> > > static const struct coresight_ops_sink arm_trbe_sink_ops = {
> > > .enable = arm_trbe_enable,
> > > .disable = arm_trbe_disable,
> > > .alloc_buffer = arm_trbe_alloc_buffer,
> > > .free_buffer = arm_trbe_free_buffer,
> > > .update_buffer = arm_trbe_update_buffer,
> > > + .percpu_save = arm_trbe_cpu_save,
> > > + .percpu_restore = arm_trbe_cpu_restore,
> >
> > Why this percpu_* prefix ?
> >
> Added percpu_ prefix because it's only for percpu_sink
> (coresight_get_percpu_sink).
> Please let me know if you have a better name.
> > > };
> > >
> > > static const struct coresight_ops arm_trbe_cs_ops = {
> > > @@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
> > > cpudata->trbe_flag = get_trbe_flag_update(trbidr);
> > > cpudata->cpu = cpu;
> > > cpudata->drvdata = drvdata;
> > > + cpudata->state_needs_save = coresight_loses_context_with_cpu(
> > > + &drvdata->pdev->dev);
> > > + cpudata->state_needs_restore = false;
> >
> > The init here looks good.
> >
> > > return;
> > > cpu_clear:
> > > cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
> > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > > index d79a242b271d..fec375d02535 100644
> > > --- a/include/linux/coresight.h
> > > +++ b/include/linux/coresight.h
> > > @@ -362,6 +362,10 @@ enum cs_mode {
> > > * @alloc_buffer: initialises perf's ring buffer for trace collection.
> > > * @free_buffer: release memory allocated in @get_config.
> > > * @update_buffer: update buffer pointers after a trace session.
> > > + * @percpu_save: saves state when CPU enters idle state.
> > > + * Only set for percpu sink.
> > > + * @percpu_restore: restores state when CPU exits idle state.
> > > + * only set for percpu sink.
> > > */
> > > struct coresight_ops_sink {
> > > int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> > > unsigned long (*update_buffer)(struct coresight_device *csdev,
> > > struct perf_output_handle *handle,
> > > void *sink_config);
> > > + void (*percpu_save)(struct coresight_device *csdev);
> > > + void (*percpu_restore)(struct coresight_device *csdev);
> >
> > Again - why this percpu_* prefix ?
> >
> > > };
> > >
> > > /**
I do not think this is the best approach.
The TRBE driver has its own power management registration functions,
so is it not possible for the save and restore should be handled
there, through a PM notifier, just as the ETM/ETE is?
Drop the unnecessary DT entry - TRBE is a per cpu architectural device
- if a TRBE is present, we know it will power down with the PE.
The CoreSight architecture permits an ETE to drive trace to an
external sink - so the TRBE might be present but unused, therefore
hooking into a source driver that may not be driving trace into the
device does not seem wise..
The TRBE PM can follow the model of the ETE / ETMv4 and save and
restore if currently in use.
This approach will isolate the changes to the TRBE driver where they should be.
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Hi all,
On Mon, Apr 28, 2025 at 11:53:26AM +0100, Mike Leach wrote:
[...]
> > > > @@ -362,6 +362,10 @@ enum cs_mode {
> > > > * @alloc_buffer: initialises perf's ring buffer for trace collection.
> > > > * @free_buffer: release memory allocated in @get_config.
> > > > * @update_buffer: update buffer pointers after a trace session.
> > > > + * @percpu_save: saves state when CPU enters idle state.
> > > > + * Only set for percpu sink.
> > > > + * @percpu_restore: restores state when CPU exits idle state.
> > > > + * only set for percpu sink.
> > > > */
> > > > struct coresight_ops_sink {
> > > > int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > > > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> > > > unsigned long (*update_buffer)(struct coresight_device *csdev,
> > > > struct perf_output_handle *handle,
> > > > void *sink_config);
> > > > + void (*percpu_save)(struct coresight_device *csdev);
> > > > + void (*percpu_restore)(struct coresight_device *csdev);
> > >
> > > Again - why this percpu_* prefix ?
> > >
> > > > };
> > > >
> > > > /**
>
> I do not think this is the best approach.
>
> The TRBE driver has its own power management registration functions,
> so is it not possible for the save and restore should be handled
> there, through a PM notifier, just as the ETM/ETE is?
>
> Drop the unnecessary DT entry - TRBE is a per cpu architectural device
> - if a TRBE is present, we know it will power down with the PE.
>
> The CoreSight architecture permits an ETE to drive trace to an
> external sink - so the TRBE might be present but unused, therefore
> hooking into a source driver that may not be driving trace into the
> device does not seem wise..
Sorry I jumped in a bit late (I saw the patch at last week and it is
on my review list).
I would suggest to hold on for this patch. I am refactoring CPU PM and
hotplug in CoreSight based on the CoreSight path.
The idea is when we do CPU power management for CoreSight devices, we
cannot simply control individual devices. Alternatively, we need to
control logics based on the linkages on CoreSight path as it can reflect
dependencies crossing the components. For example, for a CoreSight
path, when running into CPU low power state, we need firstly disable
tracer, then links, and at the end sinks. When CPU resumes back, we
need to use the reversed ordering for turning on devices.
As a result, except the tracers (ETM / ETE) should be saved and
restored contexts during CPU power states, other components on the
path will be controlled by traversing CoreSight paths. The benefit is
if a component (e.g., a link or a sink module) is shared by multiple
CoreSight paths, then the component can be disabled or enabled based on
reference counter.
I know I am a bit lagged - as I also need to support the locking on
CoreSight paths. Please expect in next 1~2 weeks I will send out
patches for public review.
> The TRBE PM can follow the model of the ETE / ETMv4 and save and
> restore if currently in use.
If TRBE PM is registered as a seperate PM notifier, a prominent issue is
it cannot promise the depedency between ETE and TRBE when execute CPU
power management. E.g., when entering CPU idle states, ETE should be
disabled prior to switch off TRBE, otherwise, it might cause lockup
issue in hardware. If ETE and TRBE register PM notifier separately,
we cannot ensure the sequence between ETE and TRBE, this is why we need
to do the operations based on CoreSight paths.
Thanks,
Leo
Hi Leo
On Mon, 28 Apr 2025 at 13:24, Leo Yan <leo.yan@arm.com> wrote:
>
> Hi all,
>
> On Mon, Apr 28, 2025 at 11:53:26AM +0100, Mike Leach wrote:
>
> [...]
>
> > > > > @@ -362,6 +362,10 @@ enum cs_mode {
> > > > > * @alloc_buffer: initialises perf's ring buffer for trace collection.
> > > > > * @free_buffer: release memory allocated in @get_config.
> > > > > * @update_buffer: update buffer pointers after a trace session.
> > > > > + * @percpu_save: saves state when CPU enters idle state.
> > > > > + * Only set for percpu sink.
> > > > > + * @percpu_restore: restores state when CPU exits idle state.
> > > > > + * only set for percpu sink.
> > > > > */
> > > > > struct coresight_ops_sink {
> > > > > int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > > > > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> > > > > unsigned long (*update_buffer)(struct coresight_device *csdev,
> > > > > struct perf_output_handle *handle,
> > > > > void *sink_config);
> > > > > + void (*percpu_save)(struct coresight_device *csdev);
> > > > > + void (*percpu_restore)(struct coresight_device *csdev);
> > > >
> > > > Again - why this percpu_* prefix ?
> > > >
> > > > > };
> > > > >
> > > > > /**
> >
> > I do not think this is the best approach.
> >
> > The TRBE driver has its own power management registration functions,
> > so is it not possible for the save and restore should be handled
> > there, through a PM notifier, just as the ETM/ETE is?
> >
> > Drop the unnecessary DT entry - TRBE is a per cpu architectural device
> > - if a TRBE is present, we know it will power down with the PE.
> >
> > The CoreSight architecture permits an ETE to drive trace to an
> > external sink - so the TRBE might be present but unused, therefore
> > hooking into a source driver that may not be driving trace into the
> > device does not seem wise..
>
> Sorry I jumped in a bit late (I saw the patch at last week and it is
> on my review list).
>
> I would suggest to hold on for this patch. I am refactoring CPU PM and
> hotplug in CoreSight based on the CoreSight path.
>
> The idea is when we do CPU power management for CoreSight devices, we
> cannot simply control individual devices. Alternatively, we need to
> control logics based on the linkages on CoreSight path as it can reflect
> dependencies crossing the components. For example, for a CoreSight
> path, when running into CPU low power state, we need firstly disable
> tracer, then links, and at the end sinks. When CPU resumes back, we
> need to use the reversed ordering for turning on devices.
>
> As a result, except the tracers (ETM / ETE) should be saved and
> restored contexts during CPU power states, other components on the
> path will be controlled by traversing CoreSight paths. The benefit is
> if a component (e.g., a link or a sink module) is shared by multiple
> CoreSight paths, then the component can be disabled or enabled based on
> reference counter.
>
> I know I am a bit lagged - as I also need to support the locking on
> CoreSight paths. Please expect in next 1~2 weeks I will send out
> patches for public review.
>
> > The TRBE PM can follow the model of the ETE / ETMv4 and save and
> > restore if currently in use.
>
> If TRBE PM is registered as a seperate PM notifier, a prominent issue is
> it cannot promise the depedency between ETE and TRBE when execute CPU
> power management. E.g., when entering CPU idle states, ETE should be
> disabled prior to switch off TRBE, otherwise, it might cause lockup
> issue in hardware. If ETE and TRBE register PM notifier separately,
> we cannot ensure the sequence between ETE and TRBE, this is why we need
> to do the operations based on CoreSight paths.
>
I believe that the architecture requires that if the disabled TRBE
cannot receive trace then the ETE should regard the trace as having
been output (A1.4 ETE spec). Incorrect sequencing should only result
in missing trace, not a core lockup - unless the implementation is not
compliant.
Mike
> Thanks,
> Leo
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
On Mon, Apr 28, 2025 at 01:55:29PM +0100, Mike Leach wrote: [...] > > > The TRBE PM can follow the model of the ETE / ETMv4 and save and > > > restore if currently in use. > > > > If TRBE PM is registered as a seperate PM notifier, a prominent issue is > > it cannot promise the depedency between ETE and TRBE when execute CPU > > power management. E.g., when entering CPU idle states, ETE should be > > disabled prior to switch off TRBE, otherwise, it might cause lockup > > issue in hardware. If ETE and TRBE register PM notifier separately, > > we cannot ensure the sequence between ETE and TRBE, this is why we need > > to do the operations based on CoreSight paths. > > > > I believe that the architecture requires that if the disabled TRBE > cannot receive trace then the ETE should regard the trace as having > been output (A1.4 ETE spec). Incorrect sequencing should only result > in missing trace, not a core lockup - unless the implementation is not > compliant. Thanks for clarification, Mike. I would prefer to stick with the CoreSight path approach, as this will help us to resolve the issue in a general way - not just for ETE / TRBE case, this would be applicable for other types of links and sinks. Thanks, Leo
On Mon, Apr 28, 2025 at 6:05 AM Leo Yan <leo.yan@arm.com> wrote: > > On Mon, Apr 28, 2025 at 01:55:29PM +0100, Mike Leach wrote: > > [...] > > > > > The TRBE PM can follow the model of the ETE / ETMv4 and save and > > > > restore if currently in use. > > > > > > If TRBE PM is registered as a seperate PM notifier, a prominent issue is > > > it cannot promise the depedency between ETE and TRBE when execute CPU > > > power management. E.g., when entering CPU idle states, ETE should be > > > disabled prior to switch off TRBE, otherwise, it might cause lockup > > > issue in hardware. If ETE and TRBE register PM notifier separately, > > > we cannot ensure the sequence between ETE and TRBE, this is why we need > > > to do the operations based on CoreSight paths. > > > > > > > I believe that the architecture requires that if the disabled TRBE > > cannot receive trace then the ETE should regard the trace as having > > been output (A1.4 ETE spec). Incorrect sequencing should only result > > in missing trace, not a core lockup - unless the implementation is not > > compliant. > I tried registering a separate CPU PM notifier for TRBE, but CPU hangs persisted on Pixel 9, albeit less frequently. After switching to the current patch, CPU hangs are no longer observed. I see the line in ETE spec: While all trace outputs are disabled, the trace unit considers any generated trace data as having been output. Maybe the enable/disable order issue only happens on some CPU models or SOCs? From Android, I hope the coresight driver can solve this issue. But I'm not sure if ARM has other suggestions. > Thanks for clarification, Mike. > > I would prefer to stick with the CoreSight path approach, as this will > help us to resolve the issue in a general way - not just for ETE / TRBE > case, this would be applicable for other types of links and sinks. > In my experience, coresight funnels, ETFs, and ETRs don't lose context with CPU low power state. So probably don't need to disable the whole coresight path when entering CPU low power state. In Android, I have a tight project schedule. So I hope this patch is not blocked by refactors. And I want to backport the patch to previous Android kernel versions. A fix before refactor is easier to backport. > Thanks, > Leo
On 4/24/25 04:30, Yabin Cui wrote:
> Similar to ETE, TRBE may lose its context when a CPU enters low
> power state. To make things worse, if ETE state is restored without
> restoring TRBE state, it can lead to a stuck CPU due to an enabled
> source device with no enabled sink devices.
>
> This patch introduces support for "arm,coresight-loses-context-with-cpu"
But could "arm,coresight-loses-context-with-cpu" device tree property
be associated wit TRBE devices ? OR this state save and restore needs
to be handled in the firmware.
> in the TRBE driver. When present, TRBE registers are saved before
> and restored after CPU low power state. To prevent CPU hangs, TRBE
> state is always saved after ETE state and restored after ETE state.
>
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> .../coresight/coresight-etm4x-core.c | 13 ++++-
> drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++
> include/linux/coresight.h | 6 +++
> 3 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e5972f16abff..1bbaa1249206 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> {
> int ret = 0;
> + struct coresight_device *sink;
>
> /* Save the TRFCR irrespective of whether the ETM is ON */
> if (drvdata->trfcr)
> @@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> * Save and restore the ETM Trace registers only if
> * the ETM is active.
> */
> - if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
> + if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
> ret = __etm4_cpu_save(drvdata);
> + if (ret == 0) {
> + sink = coresight_get_percpu_sink(drvdata->cpu);
> + if (sink && sink_ops(sink)->percpu_save)
> + sink_ops(sink)->percpu_save(sink);
> + }
> + }
> return ret;
> }
>
> @@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>
> static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> {
> + struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
> +
> + if (sink && sink_ops(sink)->percpu_restore)
> + sink_ops(sink)->percpu_restore(sink);
> if (drvdata->trfcr)
> write_trfcr(drvdata->save_trfcr);
> if (drvdata->state_needs_restore)
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index fff67aac8418..38bf46951a82 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
> */
> #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES 256
>
> +struct trbe_save_state {
> + u64 trblimitr;
> + u64 trbbaser;
> + u64 trbptr;
> + u64 trbsr;
> +};
> +
> /*
> * struct trbe_cpudata: TRBE instance specific data
> * @trbe_flag - TRBE dirty/access flag support
> @@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
> * @cpu - CPU this TRBE belongs to.
> * @mode - Mode of current operation. (perf/disabled)
> * @drvdata - TRBE specific drvdata
> + * @state_needs_save - Need to save trace registers when entering cpu idle
> + * @state_needs_restore - Need to restore trace registers when exiting cpu idle
> + * @save_state - Saved trace registers
> * @errata - Bit map for the errata on this TRBE.
> */
> struct trbe_cpudata {
> @@ -133,6 +143,9 @@ struct trbe_cpudata {
> enum cs_mode mode;
> struct trbe_buf *buf;
> struct trbe_drvdata *drvdata;
> + bool state_needs_save;
> + bool state_needs_restore;
> + struct trbe_save_state save_state;
> DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
> };
>
> @@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> return IRQ_HANDLED;
> }
>
> +static void arm_trbe_cpu_save(struct coresight_device *csdev)
> +{
> + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> + struct trbe_save_state *state = &cpudata->save_state;
> +
> + if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
> + return;
> +
> + state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
> + state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
> + state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> + state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> + cpudata->state_needs_restore = true;
> +}
> +
> +static void arm_trbe_cpu_restore(struct coresight_device *csdev)
> +{
> + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> + struct trbe_save_state *state = &cpudata->save_state;
> +
> + if (cpudata->state_needs_restore) {
> + /*
> + * To avoid disruption of normal tracing, restore trace
> + * registers only when TRBE lost power (TRBLIMITR == 0).
> + */
> + if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
> + write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
> + write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
> + write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
> + set_trbe_enabled(cpudata, state->trblimitr);
> + }
> + cpudata->state_needs_restore = false;
> + }
> +}
> +
> static const struct coresight_ops_sink arm_trbe_sink_ops = {
> .enable = arm_trbe_enable,
> .disable = arm_trbe_disable,
> .alloc_buffer = arm_trbe_alloc_buffer,
> .free_buffer = arm_trbe_free_buffer,
> .update_buffer = arm_trbe_update_buffer,
> + .percpu_save = arm_trbe_cpu_save,
> + .percpu_restore = arm_trbe_cpu_restore,
> };
>
> static const struct coresight_ops arm_trbe_cs_ops = {
> @@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
> cpudata->trbe_flag = get_trbe_flag_update(trbidr);
> cpudata->cpu = cpu;
> cpudata->drvdata = drvdata;
> + cpudata->state_needs_save = coresight_loses_context_with_cpu(
> + &drvdata->pdev->dev);
> + cpudata->state_needs_restore = false;
> return;
> cpu_clear:
> cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index d79a242b271d..fec375d02535 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -362,6 +362,10 @@ enum cs_mode {
> * @alloc_buffer: initialises perf's ring buffer for trace collection.
> * @free_buffer: release memory allocated in @get_config.
> * @update_buffer: update buffer pointers after a trace session.
> + * @percpu_save: saves state when CPU enters idle state.
> + * Only set for percpu sink.
> + * @percpu_restore: restores state when CPU exits idle state.
> + * only set for percpu sink.
> */
> struct coresight_ops_sink {
> int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> unsigned long (*update_buffer)(struct coresight_device *csdev,
> struct perf_output_handle *handle,
> void *sink_config);
> + void (*percpu_save)(struct coresight_device *csdev);
> + void (*percpu_restore)(struct coresight_device *csdev);
> };
>
> /**
On Thu, Apr 24, 2025 at 1:46 AM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> On 4/24/25 04:30, Yabin Cui wrote:
> > Similar to ETE, TRBE may lose its context when a CPU enters low
> > power state. To make things worse, if ETE state is restored without
> > restoring TRBE state, it can lead to a stuck CPU due to an enabled
> > source device with no enabled sink devices.
> >
> > This patch introduces support for "arm,coresight-loses-context-with-cpu"
>
> But could "arm,coresight-loses-context-with-cpu" device tree property
> be associated wit TRBE devices ? OR this state save and restore needs
> to be handled in the firmware.
This property is handled by ETM/ETE driver. In ETM/ETE driver, the state has
options to be saved by firmware or by the driver. On my test device, which
is Pixel 9, the state is saved by the driver. I have also tested that TRBE state
can be saved by the TRBE driver.
>
> > in the TRBE driver. When present, TRBE registers are saved before
> > and restored after CPU low power state. To prevent CPU hangs, TRBE
> > state is always saved after ETE state and restored after ETE state.
> >
> > Signed-off-by: Yabin Cui <yabinc@google.com>
> > ---
> > .../coresight/coresight-etm4x-core.c | 13 ++++-
> > drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++
> > include/linux/coresight.h | 6 +++
> > 3 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e5972f16abff..1bbaa1249206 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > {
> > int ret = 0;
> > + struct coresight_device *sink;
> >
> > /* Save the TRFCR irrespective of whether the ETM is ON */
> > if (drvdata->trfcr)
> > @@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> > * Save and restore the ETM Trace registers only if
> > * the ETM is active.
> > */
> > - if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
> > + if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
> > ret = __etm4_cpu_save(drvdata);
> > + if (ret == 0) {
> > + sink = coresight_get_percpu_sink(drvdata->cpu);
> > + if (sink && sink_ops(sink)->percpu_save)
> > + sink_ops(sink)->percpu_save(sink);
> > + }
> > + }
> > return ret;
> > }
> >
> > @@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> >
> > static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> > {
> > + struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
> > +
> > + if (sink && sink_ops(sink)->percpu_restore)
> > + sink_ops(sink)->percpu_restore(sink);
> > if (drvdata->trfcr)
> > write_trfcr(drvdata->save_trfcr);
> > if (drvdata->state_needs_restore)
> > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > index fff67aac8418..38bf46951a82 100644
> > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > @@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
> > */
> > #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES 256
> >
> > +struct trbe_save_state {
> > + u64 trblimitr;
> > + u64 trbbaser;
> > + u64 trbptr;
> > + u64 trbsr;
> > +};
> > +
> > /*
> > * struct trbe_cpudata: TRBE instance specific data
> > * @trbe_flag - TRBE dirty/access flag support
> > @@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
> > * @cpu - CPU this TRBE belongs to.
> > * @mode - Mode of current operation. (perf/disabled)
> > * @drvdata - TRBE specific drvdata
> > + * @state_needs_save - Need to save trace registers when entering cpu idle
> > + * @state_needs_restore - Need to restore trace registers when exiting cpu idle
> > + * @save_state - Saved trace registers
> > * @errata - Bit map for the errata on this TRBE.
> > */
> > struct trbe_cpudata {
> > @@ -133,6 +143,9 @@ struct trbe_cpudata {
> > enum cs_mode mode;
> > struct trbe_buf *buf;
> > struct trbe_drvdata *drvdata;
> > + bool state_needs_save;
> > + bool state_needs_restore;
> > + struct trbe_save_state save_state;
> > DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
> > };
> >
> > @@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> > return IRQ_HANDLED;
> > }
> >
> > +static void arm_trbe_cpu_save(struct coresight_device *csdev)
> > +{
> > + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > + struct trbe_save_state *state = &cpudata->save_state;
> > +
> > + if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
> > + return;
> > +
> > + state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
> > + state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
> > + state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> > + state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> > + cpudata->state_needs_restore = true;
> > +}
> > +
> > +static void arm_trbe_cpu_restore(struct coresight_device *csdev)
> > +{
> > + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > + struct trbe_save_state *state = &cpudata->save_state;
> > +
> > + if (cpudata->state_needs_restore) {
> > + /*
> > + * To avoid disruption of normal tracing, restore trace
> > + * registers only when TRBE lost power (TRBLIMITR == 0).
> > + */
> > + if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
> > + write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
> > + write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
> > + write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
> > + set_trbe_enabled(cpudata, state->trblimitr);
> > + }
> > + cpudata->state_needs_restore = false;
> > + }
> > +}
> > +
> > static const struct coresight_ops_sink arm_trbe_sink_ops = {
> > .enable = arm_trbe_enable,
> > .disable = arm_trbe_disable,
> > .alloc_buffer = arm_trbe_alloc_buffer,
> > .free_buffer = arm_trbe_free_buffer,
> > .update_buffer = arm_trbe_update_buffer,
> > + .percpu_save = arm_trbe_cpu_save,
> > + .percpu_restore = arm_trbe_cpu_restore,
> > };
> >
> > static const struct coresight_ops arm_trbe_cs_ops = {
> > @@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
> > cpudata->trbe_flag = get_trbe_flag_update(trbidr);
> > cpudata->cpu = cpu;
> > cpudata->drvdata = drvdata;
> > + cpudata->state_needs_save = coresight_loses_context_with_cpu(
> > + &drvdata->pdev->dev);
> > + cpudata->state_needs_restore = false;
> > return;
> > cpu_clear:
> > cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index d79a242b271d..fec375d02535 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -362,6 +362,10 @@ enum cs_mode {
> > * @alloc_buffer: initialises perf's ring buffer for trace collection.
> > * @free_buffer: release memory allocated in @get_config.
> > * @update_buffer: update buffer pointers after a trace session.
> > + * @percpu_save: saves state when CPU enters idle state.
> > + * Only set for percpu sink.
> > + * @percpu_restore: restores state when CPU exits idle state.
> > + * only set for percpu sink.
> > */
> > struct coresight_ops_sink {
> > int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> > unsigned long (*update_buffer)(struct coresight_device *csdev,
> > struct perf_output_handle *handle,
> > void *sink_config);
> > + void (*percpu_save)(struct coresight_device *csdev);
> > + void (*percpu_restore)(struct coresight_device *csdev);
> > };
> >
> > /**
On 4/25/25 00:07, Yabin Cui wrote:
> On Thu, Apr 24, 2025 at 1:46 AM Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
>>
>> On 4/24/25 04:30, Yabin Cui wrote:
>>> Similar to ETE, TRBE may lose its context when a CPU enters low
>>> power state. To make things worse, if ETE state is restored without
>>> restoring TRBE state, it can lead to a stuck CPU due to an enabled
>>> source device with no enabled sink devices.
>>>
>>> This patch introduces support for "arm,coresight-loses-context-with-cpu"
>>
>> But could "arm,coresight-loses-context-with-cpu" device tree property
>> be associated wit TRBE devices ? OR this state save and restore needs
>> to be handled in the firmware.
>
> This property is handled by ETM/ETE driver. In ETM/ETE driver, the state has
> options to be saved by firmware or by the driver. On my test device, which
> is Pixel 9, the state is saved by the driver. I have also tested that TRBE state
> can be saved by the TRBE driver.
Basically if the state is saved and restored in the driver for the source
ETM devices, then the same should also happen for corresponding TRBE sink
devices as well ?
>
>>
>>> in the TRBE driver. When present, TRBE registers are saved before
>>> and restored after CPU low power state. To prevent CPU hangs, TRBE
>>> state is always saved after ETE state and restored after ETE state.
>>>
>>> Signed-off-by: Yabin Cui <yabinc@google.com>
>>> ---
>>> .../coresight/coresight-etm4x-core.c | 13 ++++-
>>> drivers/hwtracing/coresight/coresight-trbe.c | 53 +++++++++++++++++++
>>> include/linux/coresight.h | 6 +++
>>> 3 files changed, 71 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index e5972f16abff..1bbaa1249206 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
>>> static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
>>> {
>>> int ret = 0;
>>> + struct coresight_device *sink;
>>>
>>> /* Save the TRFCR irrespective of whether the ETM is ON */
>>> if (drvdata->trfcr)
>>> @@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
>>> * Save and restore the ETM Trace registers only if
>>> * the ETM is active.
>>> */
>>> - if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
>>> + if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
>>> ret = __etm4_cpu_save(drvdata);
>>> + if (ret == 0) {
>>> + sink = coresight_get_percpu_sink(drvdata->cpu);
>>> + if (sink && sink_ops(sink)->percpu_save)
>>> + sink_ops(sink)->percpu_save(sink);
>>> + }
>>> + }
>>> return ret;
>>> }
>>>
>>> @@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>>>
>>> static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>>> {
>>> + struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
>>> +
>>> + if (sink && sink_ops(sink)->percpu_restore)
>>> + sink_ops(sink)->percpu_restore(sink);
>>> if (drvdata->trfcr)
>>> write_trfcr(drvdata->save_trfcr);
>>> if (drvdata->state_needs_restore)
>>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>>> index fff67aac8418..38bf46951a82 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>>> @@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
>>> */
>>> #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES 256
>>>
>>> +struct trbe_save_state {
>>> + u64 trblimitr;
>>> + u64 trbbaser;
>>> + u64 trbptr;
>>> + u64 trbsr;
>>> +};
>>> +
>>> /*
>>> * struct trbe_cpudata: TRBE instance specific data
>>> * @trbe_flag - TRBE dirty/access flag support
>>> @@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
>>> * @cpu - CPU this TRBE belongs to.
>>> * @mode - Mode of current operation. (perf/disabled)
>>> * @drvdata - TRBE specific drvdata
>>> + * @state_needs_save - Need to save trace registers when entering cpu idle
>>> + * @state_needs_restore - Need to restore trace registers when exiting cpu idle
>>> + * @save_state - Saved trace registers
>>> * @errata - Bit map for the errata on this TRBE.
>>> */
>>> struct trbe_cpudata {
>>> @@ -133,6 +143,9 @@ struct trbe_cpudata {
>>> enum cs_mode mode;
>>> struct trbe_buf *buf;
>>> struct trbe_drvdata *drvdata;
>>> + bool state_needs_save;
>>> + bool state_needs_restore;
>>> + struct trbe_save_state save_state;
>>> DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
>>> };
>>>
>>> @@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> +static void arm_trbe_cpu_save(struct coresight_device *csdev)
>>> +{
>>> + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
>>> + struct trbe_save_state *state = &cpudata->save_state;
>>> +
>>> + if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
>>> + return;
>>> +
>>> + state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
>>> + state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
>>> + state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
>>> + state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
>>> + cpudata->state_needs_restore = true;
>>> +}
>>> +
>>> +static void arm_trbe_cpu_restore(struct coresight_device *csdev)
>>> +{
>>> + struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
>>> + struct trbe_save_state *state = &cpudata->save_state;
>>> +
>>> + if (cpudata->state_needs_restore) {
>>> + /*
>>> + * To avoid disruption of normal tracing, restore trace
>>> + * registers only when TRBE lost power (TRBLIMITR == 0).
>>> + */
>>> + if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
>>> + write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
>>> + write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
>>> + write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
>>> + set_trbe_enabled(cpudata, state->trblimitr);
>>> + }
>>> + cpudata->state_needs_restore = false;
>>> + }
>>> +}
>>> +
>>> static const struct coresight_ops_sink arm_trbe_sink_ops = {
>>> .enable = arm_trbe_enable,
>>> .disable = arm_trbe_disable,
>>> .alloc_buffer = arm_trbe_alloc_buffer,
>>> .free_buffer = arm_trbe_free_buffer,
>>> .update_buffer = arm_trbe_update_buffer,
>>> + .percpu_save = arm_trbe_cpu_save,
>>> + .percpu_restore = arm_trbe_cpu_restore,
>>> };
>>>
>>> static const struct coresight_ops arm_trbe_cs_ops = {
>>> @@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
>>> cpudata->trbe_flag = get_trbe_flag_update(trbidr);
>>> cpudata->cpu = cpu;
>>> cpudata->drvdata = drvdata;
>>> + cpudata->state_needs_save = coresight_loses_context_with_cpu(
>>> + &drvdata->pdev->dev);
>>> + cpudata->state_needs_restore = false;
>>> return;
>>> cpu_clear:
>>> cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>> index d79a242b271d..fec375d02535 100644
>>> --- a/include/linux/coresight.h
>>> +++ b/include/linux/coresight.h
>>> @@ -362,6 +362,10 @@ enum cs_mode {
>>> * @alloc_buffer: initialises perf's ring buffer for trace collection.
>>> * @free_buffer: release memory allocated in @get_config.
>>> * @update_buffer: update buffer pointers after a trace session.
>>> + * @percpu_save: saves state when CPU enters idle state.
>>> + * Only set for percpu sink.
>>> + * @percpu_restore: restores state when CPU exits idle state.
>>> + * only set for percpu sink.
>>> */
>>> struct coresight_ops_sink {
>>> int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
>>> @@ -374,6 +378,8 @@ struct coresight_ops_sink {
>>> unsigned long (*update_buffer)(struct coresight_device *csdev,
>>> struct perf_output_handle *handle,
>>> void *sink_config);
>>> + void (*percpu_save)(struct coresight_device *csdev);
>>> + void (*percpu_restore)(struct coresight_device *csdev);
>>> };
>>>
>>> /**
© 2016 - 2026 Red Hat, Inc.