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

["Yingchao Deng (Consultant)" <quic_yingdeng@quicinc.com>]

On 4/28/2026 1:48 AM, Leo Yan wrote:
> On Sun, Apr 26, 2026 at 05:44:39PM +0800, 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 offset = CTI_REG_CLR_NR(reg);
>> +	u32 nr = CTI_REG_GET_NR(reg);
>> +
>> +	return drvdata->base + offset + sizeof(u32) * nr;
>> +}
> Could you try below change, which is more straightforward?
>
>    static void __iomem *__reg_addr(struct cti_drvdata *drvdata, int off,
>                                    int index)
>    {
>    	return drvdata->base + offset + sizeof(u32) * index;
>    }
>
>    #define reg_addr(drvdata, off)                 \
>          __reg_addr((drvdata), (off), 0)
>
>    #define reg_index_addr(drvdata, off, i)        \
>          __reg_addr((drvdata), (off), (i))
>
>> +
>>   /* 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->ctiinen[i],
>                                 reg_index_addr(drvdata, CTIINEN, i));
>
>>   		writel_relaxed(config->ctiouten[i],
>> -			       drvdata->base + CTIOUTEN(i));
>> +			       cti_reg_addr(drvdata, CTI_REG_SET_NR(CTIOUTEN, i)));
>                  writel_relaxed(config->ctiouten[i],
>                                 reg_index_addr(drvdata, CTIOUTEN, i));
>
> [...]
Will try.

Thanks,
Yingchao
>> +/*
>> + * 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))
> I know this might come from my suggestion, and it is also will be
> heavily used in patch 04.  We can have strightforward way to
> implement this, please drop these macros.
>
> I will reply in patch 04 separately.  Sorry my review might cause
> extra effort.
>
> Thanks,
> Leo