[PATCH v4 2/9] coresight: etm4x: exclude ss_status from drvdata->config
Leo Yan
leo.yan at arm.com
Tue Apr 14 09:04:41 PDT 2026
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 at 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}
>
More information about the linux-arm-kernel
mailing list