Re: [PATCH v6 2/2] coresight: cti: Add Qualcomm extended CTI support

[Leo Yan <leo.yan@arm.com>]

On Thu, Dec 04, 2025 at 09:04:03AM +0000, Mike Leach wrote:
> Hi,
> 
> On Thu, 4 Dec 2025 at 08:38, Leo Yan <leo.yan@arm.com> wrote:
> >
> > On Wed, Dec 03, 2025 at 06:29:44PM +0000, Coresight ML wrote:
> >
> > [...]
> >
> > > > +/* Read registers with power check only (no enable check). */
> > > > +static ssize_t coresight_cti_reg_show(struct device *dev,
> > > > +                                 struct device_attribute *attr, char *buf)
> > > > +{
> > > > +   struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
> > > > +   struct cs_off_attribute *cti_attr = container_of(attr, struct cs_off_attribute, attr);
> > > > +   u32 idx, val = 0;
> > > > +
> > > > +   pm_runtime_get_sync(dev->parent);
> > > > +   raw_spin_lock(&drvdata->spinlock);
> > > > +   idx = drvdata->config.ext_reg_sel;
> > > > +   if (drvdata->config.hw_powered) {
> > > > +           switch (cti_attr->off) {
> > > > +           case INDEX_CTITRIGINSTATUS:
> > > > +           case INDEX_CTITRIGOUTSTATUS:
> > > > +           case INDEX_ITTRIGINACK:
> > > > +           case INDEX_ITTRIGOUT:
> > > > +           case INDEX_ITTRIGOUTACK:
> > > > +           case INDEX_ITTRIGIN:
> >
> > I read again and now I understand why you need "config.ext_reg_sel"
> > as an index for these expending registers.
> >
> 
> Having this index for these extended registers matches what we do for
> the INEN and OUTEN registers. This gives the user a consistent
> approach. We do not want the unnecessary attributes as  it will
> increase the memory footprint for all cti instances, not just the qcom
> ones.

I agree with using index for CTI triggers, but it is not necessary to
add a new index for other registers (status, mode setting, ACK, etc).

It would be directive to present the status and mode setting
registers, given these registers are only from 0-3.  This will be easy
accessed from userspace, and avoid complexity in the driver.

> The first patch in this series works to reduce the memory footprint by
> only allocating resource based on the actual configuration. For
> example for an ARM designed CTI with 8 trigger registers, we no longer
> declare static 128 x 32 bit arrays for each of INEN and OUTEN which
> were required by the original design.
> 
> Given that there can be 10s or 100s of CTIs in a large multicore
> system, reducing the footprint to match the actual configuration, and
> offering a level of compression by using an index + single file to
> access a set of registers improves the efficiency of the driver.

It is good for reducing footprint, but I would give priority for a
neat implementation and easy use interfaces.

And the sysfs attr code and global structures (e.g. register conversion
struct) can shared by all instances, so I don't worry much the scale
issue if we extend them.

Thanks,
Leo