From: Yeoreum Yun <yeoreum.yun@arm•com>
To: Suzuki K Poulose <suzuki.poulose@arm•com>
Cc: coresight@lists•linaro.org, linux-arm-kernel@lists•infradead.org,
linux-kernel@vger•kernel.org, mike.leach@arm•com,
james.clark@linaro•org, alexander.shishkin@linux•intel.com,
leo.yan@arm•com, jie.gan@oss•qualcomm.com
Subject: Re: [PATCH v7 05/13] coresight: etm4x: exclude ss_status from drvdata->config
Date: Mon, 1 Jun 2026 12:52:37 +0100 [thread overview]
Message-ID: <ah1yhaZgddq+8fCW@e129823.arm.com> (raw)
In-Reply-To: <55b306e3-1e33-4a5b-a2ce-043132db7154@arm.com>
Hi Suzuki,
> On 19/05/2026 16:48, 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 and move it to drvdata.
>
> TRCSSCSRn has two different types of fields.
>
> 1. Capability
> 2. Status
>
> Moving the entire register from "config" to a common dangling place looks
> like shifting the problem to me.
>
> Could we do something like :
>
> 1. Split the fields to two.
> a. Store only the capability information in etmv4_caps. Bits[4:0]
> b. Store the Status bit in drvdata->config , as it is really matters
> to the session ? Bits[31:30]
>
> Add a helper to write/read the TRCSSSRn which would apply/mask the bits from
> drvdata->etmv4_caps->fields to the Status/Pending
Well. I'm not sure about whether we need to this.
When I remind of discussion with Leo, The Status and Pending bits should
be preserved in the *same* session theoretically However, Unitil today
In case of *perf* it doesn't keep but always cleared at the time of
resuming session and this behavior haven't reported any issue yet.
TBH, I don't have any idea how perf could utilise those bits except
the current use case for the sysfs to show the bit via *status* file.
Futhermore, IMHO, config isn't good place for the Status/Pending bits
since those fields are not *configrable* fields so I think it would be
better to locate saving information for the status and pending bits on
drvdata or by defining other fields.
But, until we find some usage of those bits in perf, I think we can
delay this separation you suggest and it might be better to keep as-is
for the purpose of showing the status/pending bits when sysfs session is
finished.
Am I missing something?
>
> Suzuki
>
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm•com>
> > ---
> > .../hwtracing/coresight/coresight-etm4x-cfg.c | 1 -
> > .../hwtracing/coresight/coresight-etm4x-core.c | 16 +++++++++-------
> > .../hwtracing/coresight/coresight-etm4x-sysfs.c | 10 +++++-----
> > drivers/hwtracing/coresight/coresight-etm4x.h | 7 ++++++-
> > 4 files changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> > index e1a59b434505..9b4947d75fde 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 53fbc4826628..7783c97b53e8 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -95,7 +95,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_cmp &&
> > - (drvdata->config.ss_status[n] & TRCSSCSRn_PC);
> > + (drvdata->ss_status[n] & TRCSSCSRn_PC);
> > }
> > u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit)
> > @@ -571,11 +571,11 @@ 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 */
> > + /* always clear status and pending bits on restart if using single-shot */
> > if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> > - config->ss_status[i] &= ~TRCSSCSRn_STATUS;
> > + drvdata->ss_status[i] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> > etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));
> > - etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
> > + etm4x_relaxed_write32(csa, drvdata->ss_status[i], TRCSSCSRn(i));
> > if (etm4x_sspcicrn_present(drvdata, i))
> > etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i));
> > }
> > @@ -772,6 +772,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
> > /* Clear configuration from previous run */
> > memset(config, 0, sizeof(struct etmv4_config));
> > +
> > if (attr->exclude_kernel)
> > config->mode = ETM_MODE_EXCL_KERN;
> > @@ -1064,7 +1065,7 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
> > /* read the status of the single shot comparators */
> > for (i = 0; i < caps->nr_ss_cmp; i++) {
> > - config->ss_status[i] =
> > + drvdata->ss_status[i] =
> > etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> > }
> > @@ -1497,8 +1498,9 @@ 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));
> > + drvdata->ss_status[i] = etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> > + drvdata->ss_status[i] &= (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 7de3c58a47b4..71e95d152ee6 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > @@ -1829,8 +1829,8 @@ 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;
> > + /* must clear bit 31 and 30 in related status register on programming */
> > + drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> > raw_spin_unlock(&drvdata->spinlock);
> > return size;
> > }
> > @@ -1844,7 +1844,7 @@ static ssize_t sshot_status_show(struct device *dev,
> > struct etmv4_config *config = &drvdata->config;
> > raw_spin_lock(&drvdata->spinlock);
> > - val = config->ss_status[config->ss_idx];
> > + val = drvdata->ss_status[config->ss_idx];
> > raw_spin_unlock(&drvdata->spinlock);
> > return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
> > }
> > @@ -1879,8 +1879,8 @@ 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;
> > + /* must clear bit 31 and 30 in related status register on programming */
> > + drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> > raw_spin_unlock(&drvdata->spinlock);
> > return size;
> > }
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> > index 6d4fbae78448..43db1eb6f8cd 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)
> > @@ -730,6 +731,9 @@ static inline u32 etm4_res_sel_pair(u8 res_sel_idx)
> > #define ETM_DEFAULT_ADDR_COMP 0
> > #define TRCSSCSRn_PC BIT(3)
> > +#define TRCSSCSRn_DV BIT(2)
> > +#define TRCSSCSRn_DA BIT(1)
> > +#define TRCSSCSRn_INST BIT(0)
> > /* PowerDown Control Register bits */
> > #define TRCPDCR_PU BIT(3)
> > @@ -978,7 +982,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];
> > @@ -1073,6 +1076,7 @@ struct etmv4_save_state {
> > * @config: structure holding configuration parameters.
> > * @save_state: State to be preserved across power loss
> > * @paused: Indicates if the trace unit is paused.
> > + * @ss_status: The status of the corresponding single-shot comparator.
> > * @arch_features: Bitmap of arch features of etmv4 devices.
> > */
> > struct etmv4_drvdata {
> > @@ -1092,6 +1096,7 @@ struct etmv4_drvdata {
> > u64 trfcr;
> > struct etmv4_config config;
> > struct etmv4_save_state *save_state;
> > + u32 ss_status[ETM_MAX_SS_CMP];
> > DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
> > };
>
--
Sincerely,
Yeoreum Yun
next prev parent reply other threads:[~2026-06-01 11:52 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-19 15:47 [PATCH v7 00/13] fix several inconsistencies with sysfs configuration in etmX Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 01/13] coresight: etm4x: fix wrong check of etm4x_sspcicrn_present() Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 02/13] coresight: etm4x: fix underflow for nrseqstate Yeoreum Yun
2026-06-01 9:40 ` Suzuki K Poulose
2026-06-01 10:08 ` Yeoreum Yun
2026-06-01 10:13 ` Suzuki K Poulose
2026-06-01 10:15 ` Yeoreum Yun
2026-06-01 9:52 ` Suzuki K Poulose
2026-06-01 10:12 ` Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 03/13] coresight: etm4x: introduce ETM_MAX_SEQ_TRANSITIONS Yeoreum Yun
2026-05-28 12:58 ` Leo Yan
2026-05-28 13:47 ` Yeoreum Yun
2026-06-01 9:53 ` Suzuki K Poulose
2026-06-01 11:14 ` Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 04/13] coresight: etm4x: introduce struct etm4_caps Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 05/13] coresight: etm4x: exclude ss_status from drvdata->config Yeoreum Yun
2026-05-28 13:30 ` Leo Yan
2026-05-28 13:46 ` Yeoreum Yun
2026-06-01 11:27 ` Suzuki K Poulose
2026-06-01 11:52 ` Yeoreum Yun [this message]
2026-06-04 9:15 ` Suzuki K Poulose
2026-05-19 15:48 ` [PATCH v7 06/13] coresight: etm4x: remove redundant fields in etmv4_save_state Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 07/13] coresight: etm4x: fix leaked trace id Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 08/13] coresight: etm4x: fix inconsistencies with sysfs configuration Yeoreum Yun
2026-05-28 14:09 ` Leo Yan
2026-05-28 14:26 ` Yeoreum Yun
2026-05-28 14:56 ` Leo Yan
2026-05-28 15:22 ` Yeoreum Yun
2026-06-01 15:38 ` Suzuki K Poulose
2026-05-19 15:48 ` [PATCH v7 09/13] coresight: etm4x: missing cscfg_csdev_disable_active_config() in perf enable Yeoreum Yun
2026-05-28 14:33 ` Leo Yan
2026-05-28 14:43 ` Yeoreum Yun
2026-05-28 15:26 ` Leo Yan
2026-05-28 16:01 ` Yeoreum Yun
2026-06-01 16:11 ` Suzuki K Poulose
2026-06-01 16:41 ` Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 10/13] coresight: etm3x: change drvdata->spinlock type to raw_spin_lock_t Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 11/13] coresight: etm3x: introduce struct etm_caps Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 12/13] coresight: etm3x: fix inconsistencies with sysfs configuration Yeoreum Yun
2026-05-19 15:48 ` [PATCH v7 13/13] coresight: etm3x: remove redundant cpu online check on etm_enable_sysfs() Yeoreum Yun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ah1yhaZgddq+8fCW@e129823.arm.com \
--to=yeoreum.yun@arm$(echo .)com \
--cc=alexander.shishkin@linux$(echo .)intel.com \
--cc=coresight@lists$(echo .)linaro.org \
--cc=james.clark@linaro$(echo .)org \
--cc=jie.gan@oss$(echo .)qualcomm.com \
--cc=leo.yan@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=mike.leach@arm$(echo .)com \
--cc=suzuki.poulose@arm$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox