[PATCH v6 11/11] coresight: etm4x: Reuse normal enable and disable logic in CPU idle
Mike Leach
mike.leach at linaro.org
Wed Nov 12 01:16:46 PST 2025
Hi Leo,
The save/restore sequence is only used if the ETM is active at the
time of power down.
However, by removing the specific save/restore structure, and using
the existing drvdata->config structure, the actual state can be
changed by sysfs from another PE, as the sysfs accesses only write to
the drvdata->config structure and not the actual hardware. Thus this
change permits the corruption of the saved state, which will result in
restore != saved..
While using the same code is a good idea avoiding lots of
duplication, it may be necessary to have a copy of the drvdata->config
passed in to the relevant functions.
We have considered in the past if it might be worthwhile isolating the
sysfs and perf config settings in the etm - it may be worth
re-visiting this again with a save restore copy in mind to.
On Tue, 11 Nov 2025 at 18:59, Leo Yan <leo.yan at arm.com> wrote:
>
> The etm4_enable_trace_unit() function is almost identical to the restore
> flow, with a few differences listed below:
>
> 1) TRCQCTLR register
>
> The TRCQCTLR register is saved and restored during CPU idle, but it
> is never touched in the normal flow. Given the Q element is not
> enabled (TRCCONFIGR.QE bits), it is acceptable to omit saving and
> restoring this register during idle.
>
> 2) TRCSSCSRn.STATUS bit
>
> The restore flow does not explicitly clear the TRCSSCSRn.STATUS bit
> but instead directly loads the saved value. In contrast, the normal
> flow clears this bit to 0 when re-enabling single-shot control.
>
> Therefore, the restore function passes the restart_ss argument as
> false to etm4_enable_hw() to avoid re-enabling single-shot mode.
>
> 3) Claim Tag Handling
>
> The claim tag is acquired and released in normal flow, on the other
> hand, the claim protocol is not applied in CPU idle flow - it simply
> restores the saved value.
>
> The claim bits serve two purpose:
>
> * Exclusive access between the kernel driver and an external
> debugger. During CPU idle, the kernel driver locks the OS Lock,
> ensuring that the external debugger cannot access the trace unit.
> Therefore, it is safe to release the claim tag during idle.
>
> * Notification to supervisory software to save/restore context for
> external debuggers. The kernel driver does not touch the external
> debugger's claim bit (ETMCLAIM[0]).
>
> Based on this, it is safe to acquire and release claim tag in the
> idle sequence.
>
> 4) OS Lock Behavior
>
> The OS Lock should be locked during CPU idle states. This differs
> from the normal flow, which unlock it. This special handling remains
> in the CPU save path.
>
> This commit reuses the normal enable and disable logic in the CPU idle
> path. The common disable logic is moved into a new function
> __etm4_disable_hw(), which omits the etm4_cs_{unlock|lock}() pairs and
> is convenient to call from the save callback.
>
Why omit cs lock/unlock? Not unlocking the software lock prevents the
claim tag registers from being written, which contradicts 3) above,
and prevents other writes during the disable sequence.
Regards
Mike.
> The resume callback sets the 'retain_ss_status' flag to avoid
> restarting the single-shot mode, and then directly calls
> etm4_enable_hw().
>
> The save context in the driver data is no longer needed and has been
> removed.
>
> Tested-by: James Clark <james.clark at linaro.org>
> Reviewed-by: Yeoreum Yun <yeoreum.yun at arm.com>
> Signed-off-by: Leo Yan <leo.yan at arm.com>
> ---
> drivers/hwtracing/coresight/coresight-etm4x-core.c | 215 ++-------------------
> drivers/hwtracing/coresight/coresight-etm4x.h | 57 ------
> 2 files changed, 16 insertions(+), 256 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index afee54ac0aec0c0e521fa1885a95f48555a369de..1c17d54729200671ec6339f43755ea4267dd61dc 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1002,7 +1002,7 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata)
> isb();
> }
>
> -static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
> +static void __etm4_disable_hw(struct etmv4_drvdata *drvdata)
> {
> u32 control;
> struct etmv4_config *config = &drvdata->config;
> @@ -1010,7 +1010,6 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
> struct csdev_access *csa = &csdev->access;
> int i;
>
> - etm4_cs_unlock(drvdata, csa);
> etm4_disable_arch_specific(drvdata);
>
> if (!drvdata->skip_power_up) {
> @@ -1039,12 +1038,21 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
> config->seq_state = etm4x_relaxed_read32(csa, TRCSEQSTR);
>
> coresight_disclaim_device_unlocked(csdev);
> - etm4_cs_lock(drvdata, csa);
>
> dev_dbg(&drvdata->csdev->dev,
> "cpu: %d disable smp call done\n", drvdata->cpu);
> }
>
> +static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
> +{
> + struct coresight_device *csdev = drvdata->csdev;
> + struct csdev_access *csa = &csdev->access;
> +
> + etm4_cs_unlock(drvdata, csa);
> + __etm4_disable_hw(drvdata);
> + etm4_cs_lock(drvdata, csa);
> +}
> +
> static void etm4_disable_sysfs_smp_call(void *info)
> {
> struct etmv4_drvdata *drvdata = info;
> @@ -1852,8 +1860,7 @@ static int etm4_dying_cpu(unsigned int cpu)
>
> static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> {
> - int i, ret = 0;
> - struct etmv4_save_state *state;
> + int ret = 0;
> struct coresight_device *csdev = drvdata->csdev;
> struct csdev_access *csa;
> struct device *etm_dev;
> @@ -1884,91 +1891,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> goto out;
> }
>
> - if (!drvdata->paused)
> - etm4_disable_trace_unit(drvdata);
> -
> - state = drvdata->save_state;
> -
> - if (drvdata->nr_pe)
> - state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);
> - state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);
> - state->trcauxctlr = etm4x_read32(csa, TRCAUXCTLR);
> - state->trceventctl0r = etm4x_read32(csa, TRCEVENTCTL0R);
> - state->trceventctl1r = etm4x_read32(csa, TRCEVENTCTL1R);
> - if (drvdata->stallctl)
> - state->trcstallctlr = etm4x_read32(csa, TRCSTALLCTLR);
> - state->trctsctlr = etm4x_read32(csa, TRCTSCTLR);
> - state->trcsyncpr = etm4x_read32(csa, TRCSYNCPR);
> - state->trcccctlr = etm4x_read32(csa, TRCCCCTLR);
> - state->trcbbctlr = etm4x_read32(csa, TRCBBCTLR);
> - state->trctraceidr = etm4x_read32(csa, TRCTRACEIDR);
> - if (drvdata->q_filt)
> - state->trcqctlr = etm4x_read32(csa, TRCQCTLR);
> -
> - state->trcvictlr = etm4x_read32(csa, TRCVICTLR);
> - state->trcviiectlr = etm4x_read32(csa, TRCVIIECTLR);
> - state->trcvissctlr = etm4x_read32(csa, TRCVISSCTLR);
> - if (drvdata->nr_pe_cmp)
> - state->trcvipcssctlr = etm4x_read32(csa, TRCVIPCSSCTLR);
> -
> - for (i = 0; i < drvdata->nrseqstate - 1; i++)
> - state->trcseqevr[i] = etm4x_read32(csa, TRCSEQEVRn(i));
> -
> - if (drvdata->nrseqstate) {
> - state->trcseqrstevr = etm4x_read32(csa, TRCSEQRSTEVR);
> - state->trcseqstr = etm4x_read32(csa, TRCSEQSTR);
> - }
> -
> - if (drvdata->numextinsel)
> - state->trcextinselr = etm4x_read32(csa, TRCEXTINSELR);
> -
> - for (i = 0; i < drvdata->nr_cntr; i++) {
> - state->trccntrldvr[i] = etm4x_read32(csa, TRCCNTRLDVRn(i));
> - state->trccntctlr[i] = etm4x_read32(csa, TRCCNTCTLRn(i));
> - state->trccntvr[i] = etm4x_read32(csa, TRCCNTVRn(i));
> - }
> -
> - /* Resource selector pair 0 is reserved */
> - for (i = 2; i < drvdata->nr_resource * 2; i++)
> - state->trcrsctlr[i] = etm4x_read32(csa, TRCRSCTLRn(i));
> -
> - for (i = 0; i < drvdata->nr_ss_cmp; i++) {
> - state->trcssccr[i] = etm4x_read32(csa, TRCSSCCRn(i));
> - state->trcsscsr[i] = etm4x_read32(csa, TRCSSCSRn(i));
> - if (etm4x_sspcicrn_present(drvdata, i))
> - state->trcsspcicr[i] = etm4x_read32(csa, TRCSSPCICRn(i));
> - }
> -
> - for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
> - state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
> - state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
> - }
> -
> - /*
> - * Data trace stream is architecturally prohibited for A profile cores
> - * so we don't save (or later restore) trcdvcvr and trcdvcmr - As per
> - * section 1.3.4 ("Possible functional configurations of an ETMv4 trace
> - * unit") of ARM IHI 0064D.
> - */
> -
> - for (i = 0; i < drvdata->numcidc; i++)
> - state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
> -
> - for (i = 0; i < drvdata->numvmidc; i++)
> - state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
> -
> - state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0);
> - if (drvdata->numcidc > 4)
> - state->trccidcctlr1 = etm4x_read32(csa, TRCCIDCCTLR1);
> -
> - state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR0);
> - if (drvdata->numvmidc > 4)
> - state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR1);
> -
> - state->trcclaimset = etm4x_read32(csa, TRCCLAIMCLR);
> -
> - if (!drvdata->skip_power_up)
> - state->trcpdcr = etm4x_read32(csa, TRCPDCR);
> + __etm4_disable_hw(drvdata);
>
> /* wait for TRCSTATR.IDLE to go up */
> if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1)) {
> @@ -1976,17 +1899,8 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> "timeout while waiting for Idle Trace Status\n");
> etm4_os_unlock(drvdata);
> ret = -EBUSY;
> - goto out;
> }
>
> - /*
> - * Power can be removed from the trace unit now. We do this to
> - * potentially save power on systems that respect the TRCPDCR_PU
> - * despite requesting software to save/restore state.
> - */
> - if (!drvdata->skip_power_up)
> - etm4x_relaxed_write32(csa, (state->trcpdcr & ~TRCPDCR_PU),
> - TRCPDCR);
> out:
> etm4_cs_lock(drvdata, csa);
> return ret;
> @@ -2010,103 +1924,13 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
>
> static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> {
> - int i;
> - struct etmv4_save_state *state = drvdata->save_state;
> - struct csdev_access *csa = &drvdata->csdev->access;
> -
> if (WARN_ON(!drvdata->csdev))
> return;
>
> - etm4_cs_unlock(drvdata, csa);
> - etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
> -
> - if (drvdata->nr_pe)
> - etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);
> - etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);
> - etm4x_relaxed_write32(csa, state->trcauxctlr, TRCAUXCTLR);
> - etm4x_relaxed_write32(csa, state->trceventctl0r, TRCEVENTCTL0R);
> - etm4x_relaxed_write32(csa, state->trceventctl1r, TRCEVENTCTL1R);
> - if (drvdata->stallctl)
> - etm4x_relaxed_write32(csa, state->trcstallctlr, TRCSTALLCTLR);
> - etm4x_relaxed_write32(csa, state->trctsctlr, TRCTSCTLR);
> - etm4x_relaxed_write32(csa, state->trcsyncpr, TRCSYNCPR);
> - etm4x_relaxed_write32(csa, state->trcccctlr, TRCCCCTLR);
> - etm4x_relaxed_write32(csa, state->trcbbctlr, TRCBBCTLR);
> - etm4x_relaxed_write32(csa, state->trctraceidr, TRCTRACEIDR);
> - if (drvdata->q_filt)
> - etm4x_relaxed_write32(csa, state->trcqctlr, TRCQCTLR);
> -
> - etm4x_relaxed_write32(csa, state->trcvictlr, TRCVICTLR);
> - etm4x_relaxed_write32(csa, state->trcviiectlr, TRCVIIECTLR);
> - etm4x_relaxed_write32(csa, state->trcvissctlr, TRCVISSCTLR);
> - if (drvdata->nr_pe_cmp)
> - etm4x_relaxed_write32(csa, state->trcvipcssctlr, TRCVIPCSSCTLR);
> -
> - for (i = 0; i < drvdata->nrseqstate - 1; i++)
> - etm4x_relaxed_write32(csa, state->trcseqevr[i], TRCSEQEVRn(i));
> -
> - if (drvdata->nrseqstate) {
> - etm4x_relaxed_write32(csa, state->trcseqrstevr, TRCSEQRSTEVR);
> - etm4x_relaxed_write32(csa, state->trcseqstr, TRCSEQSTR);
> - }
> - if (drvdata->numextinsel)
> - etm4x_relaxed_write32(csa, state->trcextinselr, TRCEXTINSELR);
> + /* Instruct etm4_enable_hw() not to restart the single-shot mode */
> + drvdata->config.retain_ss_status = 1;
>
> - for (i = 0; i < drvdata->nr_cntr; i++) {
> - etm4x_relaxed_write32(csa, state->trccntrldvr[i], TRCCNTRLDVRn(i));
> - etm4x_relaxed_write32(csa, state->trccntctlr[i], TRCCNTCTLRn(i));
> - etm4x_relaxed_write32(csa, state->trccntvr[i], TRCCNTVRn(i));
> - }
> -
> - /* Resource selector pair 0 is reserved */
> - for (i = 2; i < drvdata->nr_resource * 2; i++)
> - etm4x_relaxed_write32(csa, state->trcrsctlr[i], TRCRSCTLRn(i));
> -
> - for (i = 0; i < drvdata->nr_ss_cmp; i++) {
> - etm4x_relaxed_write32(csa, state->trcssccr[i], TRCSSCCRn(i));
> - etm4x_relaxed_write32(csa, state->trcsscsr[i], TRCSSCSRn(i));
> - if (etm4x_sspcicrn_present(drvdata, i))
> - etm4x_relaxed_write32(csa, state->trcsspcicr[i], TRCSSPCICRn(i));
> - }
> -
> - for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
> - etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
> - etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
> - }
> -
> - for (i = 0; i < drvdata->numcidc; i++)
> - etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
> -
> - for (i = 0; i < drvdata->numvmidc; i++)
> - etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
> -
> - etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0);
> - if (drvdata->numcidc > 4)
> - etm4x_relaxed_write32(csa, state->trccidcctlr1, TRCCIDCCTLR1);
> -
> - etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR0);
> - if (drvdata->numvmidc > 4)
> - etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR1);
> -
> - etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
> -
> - if (!drvdata->skip_power_up)
> - etm4x_relaxed_write32(csa, state->trcpdcr, TRCPDCR);
> -
> - /*
> - * As recommended by section 4.3.7 ("Synchronization when using the
> - * memory-mapped interface") of ARM IHI 0064D
> - */
> - dsb(sy);
> - isb();
> -
> - /* Unlock the OS lock to re-enable trace and external debug access */
> - etm4_os_unlock(drvdata);
> -
> - if (!drvdata->paused)
> - etm4_enable_trace_unit(drvdata);
> -
> - etm4_cs_lock(drvdata, csa);
> + etm4_enable_hw(drvdata);
> }
>
> static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> @@ -2293,13 +2117,6 @@ static int etm4_probe(struct device *dev)
> pm_save_enable = coresight_loses_context_with_cpu(dev) ?
> PARAM_PM_SAVE_SELF_HOSTED : PARAM_PM_SAVE_NEVER;
>
> - if (pm_save_enable != PARAM_PM_SAVE_NEVER) {
> - drvdata->save_state = devm_kmalloc(dev,
> - sizeof(struct etmv4_save_state), GFP_KERNEL);
> - if (!drvdata->save_state)
> - return -ENOMEM;
> - }
> -
> raw_spin_lock_init(&drvdata->spinlock);
>
> drvdata->cpu = coresight_get_cpu(dev);
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index d82ec1559b34787dffc7debf4643d948e3850b2a..8d4a1f0f1e52a38a280b7fbe978129287a8ba60c 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -864,61 +864,6 @@ struct etmv4_config {
> u8 s_ex_level;
> };
>
> -/**
> - * struct etm4_save_state - state to be preserved when ETM is without power
> - */
> -struct etmv4_save_state {
> - u32 trcprocselr;
> - u32 trcconfigr;
> - u32 trcauxctlr;
> - u32 trceventctl0r;
> - u32 trceventctl1r;
> - u32 trcstallctlr;
> - u32 trctsctlr;
> - u32 trcsyncpr;
> - u32 trcccctlr;
> - u32 trcbbctlr;
> - u32 trctraceidr;
> - u32 trcqctlr;
> -
> - u32 trcvictlr;
> - u32 trcviiectlr;
> - u32 trcvissctlr;
> - u32 trcvipcssctlr;
> -
> - u32 trcseqevr[ETM_MAX_SEQ_STATES];
> - u32 trcseqrstevr;
> - u32 trcseqstr;
> - u32 trcextinselr;
> - u32 trccntrldvr[ETMv4_MAX_CNTR];
> - u32 trccntctlr[ETMv4_MAX_CNTR];
> - u32 trccntvr[ETMv4_MAX_CNTR];
> -
> - u32 trcrsctlr[ETM_MAX_RES_SEL];
> -
> - u32 trcssccr[ETM_MAX_SS_CMP];
> - u32 trcsscsr[ETM_MAX_SS_CMP];
> - u32 trcsspcicr[ETM_MAX_SS_CMP];
> -
> - u64 trcacvr[ETM_MAX_SINGLE_ADDR_CMP];
> - u64 trcacatr[ETM_MAX_SINGLE_ADDR_CMP];
> - u64 trccidcvr[ETMv4_MAX_CTXID_CMP];
> - u64 trcvmidcvr[ETM_MAX_VMID_CMP];
> - u32 trccidcctlr0;
> - u32 trccidcctlr1;
> - u32 trcvmidcctlr0;
> - u32 trcvmidcctlr1;
> -
> - u32 trcclaimset;
> -
> - u32 cntr_val[ETMv4_MAX_CNTR];
> - u32 seq_state;
> - u32 vinst_ctrl;
> - u32 ss_status[ETM_MAX_SS_CMP];
> -
> - u32 trcpdcr;
> -};
> -
> /**
> * struct etm4_drvdata - specifics associated to an ETM component
> * @pclk: APB clock if present, otherwise NULL
> @@ -981,7 +926,6 @@ struct etmv4_save_state {
> * at runtime, due to the additional setting of TRFCR_CX when
> * in EL2. Otherwise, 0.
> * @config: structure holding configuration parameters.
> - * @save_state: State to be preserved across power loss
> * @skip_power_up: Indicates if an implementation can skip powering up
> * the trace unit.
> * @paused: Indicates if the trace unit is paused.
> @@ -1036,7 +980,6 @@ struct etmv4_drvdata {
> bool lpoverride;
> u64 trfcr;
> struct etmv4_config config;
> - struct etmv4_save_state *save_state;
> bool skip_power_up;
> bool paused;
> DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
>
> --
> 2.34.1
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
More information about the linux-arm-kernel
mailing list