Re: [PATCH 8/8] coresight: cti: Refactor cti_reg32_{show|store}()

[Leo Yan]

Hi Mike,

On Fri, Feb 20, 2026 at 02:44:03PM +0000, Mike Leach wrote:

[...]

> > @@ -252,14 +252,14 @@ static ssize_t cti_reg32_show(struct device *dev,
> > char *buf,
> >  	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >  	struct cti_config *config = &drvdata->config;
> > 
> > +	if (reg_offset < 0)
> > +		return -EINVAL;
> > +
> >  	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {
> > -		if ((reg_offset >= 0) && cti_is_active(config)) {
> > +		if (cti_is_active(config))
> >  			val = cti_read_single_reg(drvdata, reg_offset);
> > -			if (pcached_val)
> > -				*pcached_val = val;
> 
> I think we still need this. If a register with an none-zero default
> value / status that can change over time is read when active, then
> read when inactive, but never written, then the inactive value will
> not reflect the last read value.

Let us consider below operations using your suggested approach:

  echo 0xab > /sys/bus/coresight/devices/cti_cpu0/regs/gate
  cat /sys/bus/coresight/devices/cti_cpu0/regs/gate

  echo 1 > /sys/bus/coresight/devices/cti_cpu0/enable
  cat /sys/bus/coresight/devices/cti_cpu0/regs/gate

  echo 0 > /sys/bus/coresight/devices/cti_cpu0/enable
  cat /sys/bus/coresight/devices/cti_cpu0/regs/gate

If a user reads the "gate" knob three times in the above flow, it ends
up having three different semantics.

 - The first "cat ../gate" outputs the user specified value (0xab).
 - The second "cat" outputs the real-time register value after the
   module has been enabled.
 - The third "cat" outputs the last read value before disabling.

This could be confusing for users (even for ourseleves), as it needs to
understand the meaning of the value under specific conditions and based
on prior operations.

To simplify the semantics in this patch, the "cat" returns the realtime
register value when the module is enabled; otherwise, it returns the
user configured value.  Since "*pcached_val" always stores the user
configuration, it remains consistent configurations across multiple
enable/disable cycles.

> Relatively minor issue but does represent a potential change in
> functionality for the driver - even if I cannot see specific issues
> for current ARM CTIs. This is a R/W cache so should be updated on
> both R and W.

"R/W cache" mixes the cached user configured value with the hardware
register dump and may become inconsistent across multiple
enable/disable cycles.

It seems to me that this is a bit overdesigned and complex. thoughts?

Thanks a lot for review!

Leo