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

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

On 4/27/2026 10:22 AM, Jie Gan wrote:
>
>
> 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
   Thanks for the review.
  While current CoreSight components fit within a single 4KB
   block, IHI0029 states that a component can occupy up to 64MB (16,384
   x 4KB blocks), requiring up to 26 bits for the offset. I will change
   CTI_REG_NR_MASK to GENMASK(31, 28) to avoid any potential conflict
   with bits[24:25].


Thanks,
Yingchao
>> +    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
>>    *
>>
>