Re: [PATCH v8 2/4] coresight: cti: encode trigger register index in register offsets

[Jie Gan <jie.gan@oss.qualcomm.com>]

On 4/26/2026 5:44 PM, Yingchao Deng wrote:
> Introduce a small encoding to carry the register index together with the
> base offset in a single u32, and use a common helper to compute the final
> MMIO address. This refactors register access to be based on the encoded
> (reg, nr) pair, reducing duplicated arithmetic and making it easier to
> support variants that bank or relocate trigger-indexed registers.
> 
> Signed-off-by: Yingchao Deng <yingchao.deng@oss.qualcomm.com>
> ---
>   drivers/hwtracing/coresight/coresight-cti-core.c  | 31 +++++++++++++++--------
>   drivers/hwtracing/coresight/coresight-cti-sysfs.c |  4 +--
>   drivers/hwtracing/coresight/coresight-cti.h       | 16 ++++++++++--
>   3 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index 4e7d12bd2d3e..c4cbeb64365b 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -42,6 +42,14 @@ static DEFINE_MUTEX(ect_mutex);
>   #define csdev_to_cti_drvdata(csdev)	\
>   	dev_get_drvdata(csdev->dev.parent)
>   
> +static void __iomem *cti_reg_addr(struct cti_drvdata *drvdata, int reg)

u32 reg would be better.

> +{
> +	u32 offset = CTI_REG_CLR_NR(reg);

No functional error but a little bit tricky here.

CTI_REG_CLR_NR(reg) will produce a offset for the bits[0:23], but in the 
comment, you mentioned the base register offset ranges from [0:11].

With my understanding, all CTI register offsets fall within the range b 
0 to 0XFAC, that's why we have bits[0:11]?

Thanks,
Jie

> +	u32 nr = CTI_REG_GET_NR(reg);
> +
> +	return drvdata->base + offset + sizeof(u32) * nr;
> +}
> +
>   /* write set of regs to hardware - call with spinlock claimed */
>   void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
>   {
> @@ -55,16 +63,17 @@ void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
>   
>   	/* write the CTI trigger registers */
>   	for (i = 0; i < config->nr_trig_max; i++) {
> -		writel_relaxed(config->ctiinen[i], drvdata->base + CTIINEN(i));
> +		writel_relaxed(config->ctiinen[i],
> +			       cti_reg_addr(drvdata, CTI_REG_SET_NR(CTIINEN, i)));
>   		writel_relaxed(config->ctiouten[i],
> -			       drvdata->base + CTIOUTEN(i));
> +			       cti_reg_addr(drvdata, CTI_REG_SET_NR(CTIOUTEN, i)));
>   	}
>   
>   	/* other regs */
> -	writel_relaxed(config->ctigate, drvdata->base + CTIGATE);
> +	writel_relaxed(config->ctigate, cti_reg_addr(drvdata, CTIGATE));
>   	if (config->asicctl_impl)
> -		writel_relaxed(config->asicctl, drvdata->base + ASICCTL);
> -	writel_relaxed(config->ctiappset, drvdata->base + CTIAPPSET);
> +		writel_relaxed(config->asicctl, cti_reg_addr(drvdata, ASICCTL));
> +	writel_relaxed(config->ctiappset, cti_reg_addr(drvdata, CTIAPPSET));
>   
>   	/* re-enable CTI */
>   	writel_relaxed(1, drvdata->base + CTICONTROL);
> @@ -127,7 +136,7 @@ u32 cti_read_single_reg(struct cti_drvdata *drvdata, int offset)
>   	int val;
>   
>   	CS_UNLOCK(drvdata->base);
> -	val = readl_relaxed(drvdata->base + offset);
> +	val = readl_relaxed(cti_reg_addr(drvdata, offset));
>   	CS_LOCK(drvdata->base);
>   
>   	return val;
> @@ -136,7 +145,7 @@ u32 cti_read_single_reg(struct cti_drvdata *drvdata, int offset)
>   void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value)
>   {
>   	CS_UNLOCK(drvdata->base);
> -	writel_relaxed(value, drvdata->base + offset);
> +	writel_relaxed(value, cti_reg_addr(drvdata, offset));
>   	CS_LOCK(drvdata->base);
>   }
>   
> @@ -344,8 +353,7 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op,
>   
>   	/* update the local register values */
>   	chan_bitmask = BIT(channel_idx);
> -	reg_offset = (direction == CTI_TRIG_IN ? CTIINEN(trigger_idx) :
> -		      CTIOUTEN(trigger_idx));
> +	reg_offset = (direction == CTI_TRIG_IN ? CTIINEN : CTIOUTEN);
>   
>   	guard(raw_spinlock_irqsave)(&drvdata->spinlock);
>   
> @@ -365,8 +373,9 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op,
>   
>   	/* write through if enabled */
>   	if (cti_is_active(config))
> -		cti_write_single_reg(drvdata, reg_offset, reg_value);
> -
> +		cti_write_single_reg(drvdata,
> +				     CTI_REG_SET_NR(reg_offset, trigger_idx),
> +				     reg_value);
>   	return 0;
>   }
>   
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index 2bbfa405cb6b..8b70e7e38ea3 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -386,7 +386,7 @@ static ssize_t inen_store(struct device *dev,
>   
>   	/* write through if enabled */
>   	if (cti_is_active(config))
> -		cti_write_single_reg(drvdata, CTIINEN(index), val);
> +		cti_write_single_reg(drvdata, CTI_REG_SET_NR(CTIINEN, index), val);
>   
>   	return size;
>   }
> @@ -427,7 +427,7 @@ static ssize_t outen_store(struct device *dev,
>   
>   	/* write through if enabled */
>   	if (cti_is_active(config))
> -		cti_write_single_reg(drvdata, CTIOUTEN(index), val);
> +		cti_write_single_reg(drvdata, CTI_REG_SET_NR(CTIOUTEN, index), val);
>   
>   	return size;
>   }
> diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
> index ef079fc18b72..dd1ba44518c4 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.h
> +++ b/drivers/hwtracing/coresight/coresight-cti.h
> @@ -7,6 +7,7 @@
>   #ifndef _CORESIGHT_CORESIGHT_CTI_H
>   #define _CORESIGHT_CORESIGHT_CTI_H
>   
> +#include <linux/bitfield.h>
>   #include <linux/coresight.h>
>   #include <linux/device.h>
>   #include <linux/list.h>
> @@ -30,8 +31,8 @@ struct fwnode_handle;
>   #define CTIAPPSET		0x014
>   #define CTIAPPCLEAR		0x018
>   #define CTIAPPPULSE		0x01C
> -#define CTIINEN(n)		(0x020 + (4 * n))
> -#define CTIOUTEN(n)		(0x0A0 + (4 * n))
> +#define CTIINEN			0x020
> +#define CTIOUTEN		0x0A0
>   #define CTITRIGINSTATUS		0x130
>   #define CTITRIGOUTSTATUS	0x134
>   #define CTICHINSTATUS		0x138
> @@ -59,6 +60,17 @@ struct fwnode_handle;
>    */
>   #define CTIINOUTEN_MAX		32
>   
> +/*
> + * Encode CTI register offset and register index in one u32:
> + *   - bits[0:11]  : base register offset (0x000 to 0xFFF)
> + *   - bits[24:31] : register index (nr)
> + */
> +#define CTI_REG_NR_MASK			GENMASK(31, 24)
> +#define CTI_REG_GET_NR(reg)		FIELD_GET(CTI_REG_NR_MASK, (reg))
> +#define CTI_REG_SET_NR_CONST(reg, nr)	((reg) | FIELD_PREP_CONST(CTI_REG_NR_MASK, (nr)))
> +#define CTI_REG_SET_NR(reg, nr)		((reg) | FIELD_PREP(CTI_REG_NR_MASK, (nr)))
> +#define CTI_REG_CLR_NR(reg)		((reg) & (~CTI_REG_NR_MASK))
> +
>   /**
>    * Group of related trigger signals
>    *
>