Re: [PATCH v4 2/9] coresight: etm4x: exclude ss_status from drvdata->config

On Mon, Apr 13, 2026 at 03:19:55PM +0100, Yeoreum Yun wrote:

The purpose of TRCSSCSRn register is to show status of the corresponding Single-shot Comparator Control and input supports. That means writable field's purpose for reset or restore from idle status not for configuration.

Therefore, exclude ss_status from drvdata->config, move it to etm4x_caps. This includes remove TRCSSCRn from configurable item and remove saving in etm4_disable_hw().

Signed-off-by: Yeoreum Yun yeoreum.yun@arm.com

.../hwtracing/coresight/coresight-etm4x-cfg.c | 1 - .../hwtracing/coresight/coresight-etm4x-core.c | 18 +++++------------- .../coresight/coresight-etm4x-sysfs.c | 7 ++----- drivers/hwtracing/coresight/coresight-etm4x.h | 4 +++- 4 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c index c302072b293a..d14d7c8a23e5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c @@ -86,7 +86,6 @@ static int etm4_cfg_map_reg_offset(struct etmv4_drvdata *drvdata, off_mask = (offset & GENMASK(11, 5)); do { CHECKREGIDX(TRCSSCCRn(0), ss_ctrl, idx, off_mask);

• 	CHECKREGIDX(TRCSSCSRn(0), ss_status, idx, off_mask);
  CHECKREGIDX(TRCSSPCICRn(0), ss_pe_cmp, idx, off_mask);
  

  } while (0); } else if ((offset >= TRCCIDCVRn(0)) && (offset <= TRCVMIDCVRn(7))) {

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 6443f3717b37..8ebfd3924143 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -91,7 +91,7 @@ static bool etm4x_sspcicrn_present(struct etmv4_drvdata *drvdata, int n) const struct etmv4_caps *caps = &drvdata->caps; return (n < caps->nr_ss_cmp) && caps->nr_pe &&

•       (drvdata->config.ss_status[n] & TRCSSCSRn_PC);
  

•       (caps->ss_status[n] & TRCSSCSRn_PC);
  

Nitpick: The naming 'ss_status' is a bit confused for capability. Could we rename 'ss_status' to 'ss_comparator' or a simple one 'ss_cmp' ?

} u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit) @@ -571,11 +571,9 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i)); for (i = 0; i < caps->nr_ss_cmp; i++) {

• /* always clear status bit on restart if using single-shot */
  

• if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
  

• 	config->ss_status[i] &= ~TRCSSCSRn_STATUS;
  

  etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));

• etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
  

• /* always clear status and pending bits on restart if using single-shot */
  

• etm4x_relaxed_write32(csa, caps->ss_status[i], TRCSSCSRn(i));
  

It is a bit weird to use caps to write a register. A smooth way is to clear STATUS and PENDING bits based on the read back value:

val = etm4x_relaxed_read32(csa, TRCSSCSRn(i)); val &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING); etm4x_relaxed_write32(csa, val, TRCSSCSRn(i));

if (etm4x_sspcicrn_present(drvdata, i))
	etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i));

} @@ -1053,12 +1051,6 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) etm4_disable_trace_unit(drvdata);

• /* read the status of the single shot comparators */
• for (i = 0; i < caps->nr_ss_cmp; i++) {

• config->ss_status[i] =
  

• 	etm4x_relaxed_read32(csa, TRCSSCSRn(i));
  

• }
• /* read back the current counter values */ for (i = 0; i < caps->nr_cntr; i++) { config->cntr_val[i] =

@@ -1501,8 +1493,8 @@ static void etm4_init_arch_data(void *info) */ caps->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4); for (i = 0; i < caps->nr_ss_cmp; i++) {

• drvdata->config.ss_status[i] =
  

• 	etm4x_relaxed_read32(csa, TRCSSCSRn(i));
  

• caps->ss_status[i] = etm4x_relaxed_read32(csa, TRCSSCSRn(i));
  

• caps->ss_status[i] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
  

For init cap, we can use explict way:

caps->ss_cmp &= (TRCSSCSRn_PC | TRCSSCSRn_DV | TRCSSCSRn_DA | TRCSSCSRn_INST);

} /* NUMCIDC, bits[27:24] number of Context ID comparators for tracing */ caps->numcidc = FIELD_GET(TRCIDR4_NUMCIDC_MASK, etmidr4); diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index 50408215d1ac..dd62f01674cf 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -1827,8 +1827,6 @@ static ssize_t sshot_ctrl_store(struct device *dev, raw_spin_lock(&drvdata->spinlock); idx = config->ss_idx; config->ss_ctrl[idx] = FIELD_PREP(TRCSSCCRn_SAC_ARC_RST_MASK, val);

• /* must clear bit 31 in related status register on programming */
• config->ss_status[idx] &= ~TRCSSCSRn_STATUS; raw_spin_unlock(&drvdata->spinlock); return size;

} @@ -1839,10 +1837,11 @@ static ssize_t sshot_status_show(struct device *dev, { unsigned long val; struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);

• const struct etmv4_caps *caps = &drvdata->caps; struct etmv4_config *config = &drvdata->config;

raw_spin_lock(&drvdata->spinlock);

• val = config->ss_status[config->ss_idx];

• val = caps->ss_status[config->ss_idx];

I think cap->ss_cmp is a good refactoring, but for legacy reason, I am just wandering if we still need config->ss_status so that it can record the lastest status (mainly for STATUS bit and PENDING bit).

Otherwise, this Sysfs interface can only provide capability rather than status value.

Thanks, Leo

raw_spin_unlock(&drvdata->spinlock); return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); } @@ -1877,8 +1876,6 @@ static ssize_t sshot_pe_ctrl_store(struct device *dev, raw_spin_lock(&drvdata->spinlock); idx = config->ss_idx; config->ss_pe_cmp[idx] = FIELD_PREP(TRCSSPCICRn_PC_MASK, val);

• /* must clear bit 31 in related status register on programming */
• config->ss_status[idx] &= ~TRCSSCSRn_STATUS; raw_spin_unlock(&drvdata->spinlock); return size;

} diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 8168676f2945..8864cfb76bad 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -213,6 +213,7 @@ #define TRCACATRn_EXLEVEL_MASK GENMASK(14, 8) #define TRCSSCSRn_STATUS BIT(31) +#define TRCSSCSRn_PENDING BIT(30) #define TRCSSCCRn_SAC_ARC_RST_MASK GENMASK(24, 0) #define TRCSSPCICRn_PC_MASK GENMASK(7, 0) @@ -861,6 +862,7 @@ enum etm_impdef_type {

• @lpoverride: If the implementation can support low-power state over.
• @skip_power_up: Indicates if an implementation can skip powering up

•    the trace unit.
  

• • @ss_status: The status of the corresponding single-shot comparator.
  */

struct etmv4_caps { u8 nr_pe; @@ -899,6 +901,7 @@ struct etmv4_caps { bool atbtrig : 1; bool lpoverride : 1; bool skip_power_up : 1;

• u32 ss_status[ETM_MAX_SS_CMP];

}; /** @@ -977,7 +980,6 @@ struct etmv4_config { u32 res_ctrl[ETM_MAX_RES_SEL]; /* TRCRSCTLRn */ u8 ss_idx; u32 ss_ctrl[ETM_MAX_SS_CMP];

• u32 ss_status[ETM_MAX_SS_CMP]; u32 ss_pe_cmp[ETM_MAX_SS_CMP]; u8 addr_idx; u64 addr_val[ETM_MAX_SINGLE_ADDR_CMP];

-- LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}