Re: [PATCH v6 02/13] coresight: etm4x: fix underflow for nrseqstate

Re: [PATCH v6 02/13] coresight: etm4x: fix underflow for nrseqstate

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:52PM +0100, Yeoreum Yun wrote:
> TCRSEQEVR<n> is implemented only when TCRIDR5.NUMSEQSTATE is 0b100,
> in which case n ranges from 0 to 2; otherwise, TCRIDR5.NUMSEQSTATE is 0b000.
> 
> Therefore, drvdata->nrseqstate should be checked before entering the loop.

Since TRCSEQEVRn (n=0~2), to avoid confusion, we also need to rename
ETM_MAX_SEQ_STATES to ETM_MAX_SEQ_TRANSITIONS and define it as 3:

   #define ETM_MAX_SEQ_TRANSITIONS      3

Then we don't allocate 4 items but use only 3 of them.

Thanks,
Leo

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:54PM +0100, Yeoreum Yun wrote:

[...]

> @@ -573,11 +573,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, 0x0, TRCSSCSRn(i));

I am not confident what is the right way to handle the pending bit.
I looked a bit Arm ARM but still no clue. In particular, I suspect it
may need to be handled when disabling the trace in etm4_disable_hw().

Let's make it clear with some internal check.

> @@ -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);

Similarly, the question is: if it is in a pending state, how can we
ensure the state machine works properly with the new settings?

>  	raw_spin_unlock(&drvdata->spinlock);
>  	return size;
>  }

> @@ -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);

Ditto.

Thanks,
Leo

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:54PM +0100, Yeoreum Yun wrote:

[...]

> @@ -573,11 +573,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, 0x0, TRCSSCSRn(i));

After confirmed with hardware team, we should preserve status bits
(including STATUS and PENDING bits) during a session. So here we should
set drvdata->ss_status to TRCSSCSRn.

> @@ -1503,8 +1501,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);

It is fine for read these capacity bits when probe, but we need to clear
status when a session is starting to avoid the stale value left from
previous session:

  drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);

We can do this in etm4_parse_event_config() for perf mode, and might
create a new function (say etm4_parse_sysfs_config()) for preparing
config for sysfs mode?

The rest looks good to me.

Thanks,
Leo

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

[Yeoreum Yun]

Hi Leo,

> On Wed, Apr 22, 2026 at 02:21:54PM +0100, Yeoreum Yun wrote:
> 
> [...]
> 
> > @@ -573,11 +573,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, 0x0, TRCSSCSRn(i));
> 
> After confirmed with hardware team, we should preserve status bits
> (including STATUS and PENDING bits) during a session. So here we should
> set drvdata->ss_status to TRCSSCSRn.
> 
> > @@ -1503,8 +1501,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);
> 
> It is fine for read these capacity bits when probe, but we need to clear
> status when a session is starting to avoid the stale value left from
> previous session:
> 
>   drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> 
> We can do this in etm4_parse_event_config() for perf mode, and might
> create a new function (say etm4_parse_sysfs_config()) for preparing
> config for sysfs mode?
> 

As we discussed in offline, those bits should be cleared at the begining
of the session. so clearing drvdata->ss_status at start of session
in each mode is fine for me and for future integration for cpu suspend/resume.

But, I want to clarify that the perf is one of exceptional case
since the "etm4_parse_event_config()" is called at the "resume" of session
for per-thread mode event.

TBH, We don't have some specific usage how STATUS or PENDING bit could
be used with perf session and until now those bits are always cleared 
at the time of "sched-in" though we need to keep those bits theoretically.

Anyway as we discussed, since now there have been no issue
relavant for those bits, let the clear drvdata->ss_status at the
etm4_parse_event_config() or when setting a active config for start/resume
in this patchset and let me fix this with another patchset.

-- 
Sincerely,
Yeoreum Yun