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

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

Hi Leo,

On 12/9/2025 8:19 PM, Leo Yan wrote:
> Hi Yingchao,
>
> On Tue, Dec 09, 2025 at 04:16:28PM +0800, Yingchao Deng wrote:
>> Hi Leo & Mike
>>
>> Based on Leo’s suggestions, I created a new patch, but there are three points that do not fully align with his recommendations:
>>
>>      1. The helper function for returning the register address now returns only the offset, because returning the full address would conflict with cti_write_single_reg.
> No need to change each callsite for cti_write_single_reg().  You could
> update cti_write_single_reg() instead:
>
>    void cti_write_single_reg(struct cti_drvdata *drvdata,
>                              int offset, u32 value)
>    {
>            CS_UNLOCK(drvdata->base);
>            writel_relaxed(value, cti_reg_addr(drvdata, offset));
>            CS_LOCK(drvdata->base);
>    }

However, since we also need to handle cti_reg_addr_with_nr, it will be 
necessary to add an additional parameter "nr" to cti_write_single_reg?

Thanks
Yingchao
>>      2. For registers such as triginstatus1...3, I defined additional macros CTITRIGINSTATUS1...3. This is because CTITRIGINSTATUS + 0x4 equals CTITRIGOUTSTATUS, and to avoid conflicts with existing macros, I chose numbers starting from 0x1000 for the new definitions.
> To avoid the register naming pollution, please don't define the common
> names but only used for Qcom registers.
>
> AFAIK, you even don't need to define these registers.  These registers
> are only used for sysfs knobs, we can define an extra "nr" field (e.g.,
> bits[31..28] for indexing these registers, something like:
>
>    #define CIT_REG_NR_SHIFT          28
>    #define CIT_REG_NR_MASK           GENMASK(31, 28)
>    #define CTI_REG_GET_NR(reg)       FIELD_GET(CIT_REG_NR_MASK, (reg))
>    #define CTI_REG_SET_NR(reg, nr)   ((reg) | FIELD_PREP(CIT_REG_NR_MASK, (nr))
>
>    static struct attribute *coresight_cti_regs_attrs[] = {
>    ...
>      coresight_cti_reg(triginstatus, CTITRIGINSTATUS),
>      coresight_cti_reg(triginstatus1, CTI_REG_SET_NR(CTITRIGINSTATUS, 1)),
>      coresight_cti_reg(triginstatus2, CTI_REG_SET_NR(CTITRIGINSTATUS, 2)),
>      coresight_cti_reg(triginstatus3, CTI_REG_SET_NR(CTITRIGINSTATUS, 3)),
>    ...
>
> Then, you just need to decode "nr" fields in cti_qcom_reg_off().
>
>>      3. Regarding the visibility of attributes for triginstatus1...3, since coresight_cti_reg produces an anonymous variable that cannot be directly referenced, I used coresight_cti_regs_attrs[i] to obtain the attribute corresponding to triginstatus1.
> Okay, I get the meaning for "an anonymous variable" - there have no
> field naming when define attr with the macro coresight_cti_reg().
>
> but you could comparing the attr string?
>
>    if (!strcmp(attr->name, "triginstatus1") ||
>        !strcmp(attr->name, "triginstatus2") ||
>        !strcmp(attr->name, "triginstatus3"))
>        ...
>
> Thanks,
> Leo