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

[Mike Leach]

Hi Leo,

On 2/20/26 17:35, Leo Yan wrote:
> 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.

which for a non volatile register will be 0xab as enable writes all the 
regs from the config to the device - or will have been written through 
the cache to the device by the echo command if the device was powered 
and available .

>   - The third "cat" outputs the last read value before disabling.

Still 0xab unless this is a register that can change do to operation in 
which case it will be whatever the last noted value is.

All of the above is entirely expected and required. We should always 
show the user the latest value of the register. If the CTI is powered 
and available then this will be that directly read from the register.

The ETM is not different in this respect, except that the ETM reads all 
registers back on disable - and registers are architecturally forbidden 
to be written while the device is enabled - unlike the CTI where 
registers can be written after the device is enabled, and for some 
registers it makes sense only to write them when enabled.

With the CTI we do not readback all the regs at disable. They are demand 
read for sysfs, which is more efficient.


> 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.

That is pretty much the definition of reading a hardware register on any 
device. You need to know if it can change during operation or not, if it 
is volatile under the conditions you are reading it.

> 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.

The semantics of the cache are to use it if the registers are 
unavailable if not powered.


> > 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?

RW caching has been around for a long time - not sure 2 lines of code 
could be described as complex!

Regards

Mike

> Thanks a lot for review!
>
> Leo