From: "Yingchao Deng (Consultant)" <quic_yingdeng@quicinc•com>
To: Leo Yan <leo.yan@arm•com>,
Yingchao Deng <yingchao.deng@oss•qualcomm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm•com>,
Mike Leach <mike.leach@arm•com>,
James Clark <james.clark@linaro•org>,
"Alexander Shishkin" <alexander.shishkin@linux•intel.com>,
<coresight@lists•linaro.org>,
<linux-arm-kernel@lists•infradead.org>,
<linux-kernel@vger•kernel.org>, <linux-arm-msm@vger•kernel.org>,
<jinlong.mao@oss•qualcomm.com>, <tingwei.zhang@oss•qualcomm.com>,
<jie.gan@oss•qualcomm.com>,
Yingchao Deng <quic_yingdeng@quicinc•com>
Subject: Re: [PATCH v9 2/4] coresight: cti: use __reg_addr() helper for register access
Date: Mon, 1 Jun 2026 09:53:09 +0800 [thread overview]
Message-ID: <e4b25ab7-a50e-416c-b2dd-1c8e8ef40019@quicinc.com> (raw)
In-Reply-To: <20260529144055.GK101133@e132581.arm.com>
On 5/29/2026 10:40 PM, Leo Yan wrote:
> On Thu, May 21, 2026 at 08:16:28PM +0800, Yingchao Deng wrote:
>
> [...]
>
>> +static void __iomem *__reg_addr(struct cti_drvdata *drvdata, u32 off,
>> + u32 index)
>> +{
>> + return drvdata->base + off + 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))
>> -u32 cti_read_single_reg(struct cti_drvdata *drvdata, int offset)
>> +u32 cti_read_single_reg(struct cti_drvdata *drvdata, u32 off, u32 index)
>> {
>> - int val;
>> + u32 val;
>>
>> CS_UNLOCK(drvdata->base);
>> - val = readl_relaxed(drvdata->base + offset);
>> + val = readl_relaxed(reg_index_addr(drvdata, off, index));
>> CS_LOCK(drvdata->base);
>>
>> return val;
>> }
>>
>> -void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value)
>> +void cti_write_single_reg(struct cti_drvdata *drvdata, u32 off, u32 index,
>> + u32 value)
>> {
>> CS_UNLOCK(drvdata->base);
>> - writel_relaxed(value, drvdata->base + offset);
>> + writel_relaxed(value, reg_index_addr(drvdata, off, index));
>> CS_LOCK(drvdata->base);
>> }
> I prefer to move the register helpers into coresight-cti.h and add two
> additional helpers: cti_read_single_reg_index() and
> cti_write_single_reg_index().
>
> I also found circular dependency between coresight-cti.h and qcom-cti.h.
> Since qcom-cti.h only contains register definitions and a small inline
> function for register conversion, we can simply fold it into
> coresight-cti.h and dismiss the circular dependency.
>
> Based on this idea, I played around with the code a bit (see the snippet
> below). Would you be okay with applying this approach to patches 02/03?
> If have any questions, please let me know.
>
> ---8<---
>
> --- a/drivers/hwtracing/coresight/coresight-cti.h
> +++ b/drivers/hwtracing/coresight/coresight-cti.h
> @@ -60,6 +60,31 @@ struct fwnode_handle;
> */
> #define CTIINOUTEN_MAX 128
>
> +/* QCOM CTI extension */
> +#define QCOM_ARCHITECT 0x477
> +
> +#define QCOM_CTIINTACK 0x020
> +#define QCOM_CTIAPPSET 0x004
> +#define QCOM_CTIAPPCLEAR 0x008
> +#define QCOM_CTIAPPPULSE 0x00C
> +#define QCOM_CTIINEN 0x400
> +#define QCOM_CTIOUTEN 0x800
> +#define QCOM_CTITRIGINSTATUS 0x040
> +#define QCOM_CTITRIGOUTSTATUS 0x060
> +#define QCOM_CTICHINSTATUS 0x080
> +#define QCOM_CTICHOUTSTATUS 0x084
> +#define QCOM_CTIGATE 0x088
> +#define QCOM_ASICCTL 0x08C
> +/* Integration test registers */
> +#define QCOM_ITCHINACK 0xE70
> +#define QCOM_ITTRIGINACK 0xE80
> +#define QCOM_ITCHOUT 0xE74
> +#define QCOM_ITTRIGOUT 0xEA0
> +#define QCOM_ITCHOUTACK 0xE78
> +#define QCOM_ITTRIGOUTACK 0xEC0
> +#define QCOM_ITCHIN 0xE7C
> +#define QCOM_ITTRIGIN 0xEE0
> +
> /**
> * Group of related trigger signals
> *
> @@ -222,9 +247,8 @@ int cti_enable(struct coresight_device *csdev, enum cs_mode mode,
> int cti_disable(struct coresight_device *csdev, struct coresight_path *path);
> void cti_write_all_hw_regs(struct cti_drvdata *drvdata);
> void cti_write_intack(struct device *dev, u32 ackval);
> -void cti_write_single_reg(struct cti_drvdata *drvdata, u32 off, u32 index,
> - u32 value);
> -u32 cti_read_single_reg(struct cti_drvdata *drvdata, u32 off, u32 index);
> int cti_channel_trig_op(struct device *dev, enum cti_chan_op op,
> enum cti_trig_dir direction, u32 channel_idx,
> u32 trigger_idx);
> @@ -237,6 +261,78 @@ struct coresight_platform_data *
> coresight_cti_get_platform_data(struct device *dev);
> const char *cti_plat_get_node_name(struct fwnode_handle *fwnode);
>
> +static inline u32 cti_qcom_reg_off(u32 offset)
> +{
> + switch (offset) {
> + case CTIINTACK: return QCOM_CTIINTACK;
> + case CTIAPPSET: return QCOM_CTIAPPSET;
> + case CTIAPPCLEAR: return QCOM_CTIAPPCLEAR;
> + case CTIAPPPULSE: return QCOM_CTIAPPPULSE;
> + case CTIINEN: return QCOM_CTIINEN;
> + case CTIOUTEN: return QCOM_CTIOUTEN;
> + case CTITRIGINSTATUS: return QCOM_CTITRIGINSTATUS;
> + case CTITRIGOUTSTATUS: return QCOM_CTITRIGOUTSTATUS;
> + case CTICHINSTATUS: return QCOM_CTICHINSTATUS;
> + case CTICHOUTSTATUS: return QCOM_CTICHOUTSTATUS;
> + case CTIGATE: return QCOM_CTIGATE;
> + case ASICCTL: return QCOM_ASICCTL;
> + case ITCHINACK: return QCOM_ITCHINACK;
> + case ITTRIGINACK: return QCOM_ITTRIGINACK;
> + case ITCHOUT: return QCOM_ITCHOUT;
> + case ITTRIGOUT: return QCOM_ITTRIGOUT;
> + case ITCHOUTACK: return QCOM_ITCHOUTACK;
> + case ITTRIGOUTACK: return QCOM_ITTRIGOUTACK;
> + case ITCHIN: return QCOM_ITCHIN;
> + case ITTRIGIN: return QCOM_ITTRIGIN;
> +
> + default:
> + return offset;
> + }
> +}
> +
> +static inline void __iomem *__reg_addr(struct cti_drvdata *drvdata,
> + u32 off, u32 index)
> +{
> + if (unlikely(drvdata->is_qcom_cti))
> + off = cti_qcom_reg_off(off);
> +
> + return drvdata->base + off + index * sizeof(u32);
> +}
> +
> +#define reg_addr(drvdata, off) __reg_addr((drvdata), (off), 0)
> +#define reg_index_addr(drvdata, off, i) __reg_addr((drvdata), (off), (i))
> +
> +static inline u32 cti_read_single_reg_index(struct cti_drvdata *drvdata,
> + u32 off, u32 index)
> +{
> + u32 val;
> +
> + CS_UNLOCK(drvdata->base);
> + val = readl_relaxed(reg_index_addr(drvdata, off, index));
> + CS_LOCK(drvdata->base);
> +
> + return val;
> +}
> +
> +static inline u32 cti_read_single_reg(struct cti_drvdata *drvdata, u32 off)
> +{
> + return cti_read_single_reg_index(drvdata, off, 0);
> +}
> +
> +static inline void cti_write_single_reg_index(struct cti_drvdata *drvdata,
> + u32 off, u32 index, u32 value)
> +{
> + CS_UNLOCK(drvdata->base);
> + writel_relaxed(value, reg_index_addr(drvdata, off, index));
> + CS_LOCK(drvdata->base);
> +}
> +
> +static inline void cti_write_single_reg(struct cti_drvdata *drvdata,
> + u32 off, u32 value)
> +{
> + cti_write_single_reg_index(drvdata, off, 0, value);
> +}
> +
> /* Check if a cti device is enabled */
> static inline bool cti_is_active(struct cti_config *cfg)
> {
Thanks for the detailed suggestion and the reference implementation,
the approach looks clean. I'll adopt it in v11.
Thanks,
Yingchao
next prev parent reply other threads:[~2026-06-01 1:53 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-21 12:16 [PATCH v9 0/4] Add Qualcomm extended CTI support Yingchao Deng
2026-05-21 12:16 ` [PATCH v9 1/4] coresight: cti: Convert trigger usage fields to dynamic Yingchao Deng
2026-05-28 18:01 ` Leo Yan
2026-05-29 1:31 ` Yingchao Deng (Consultant)
2026-05-21 12:16 ` [PATCH v9 2/4] coresight: cti: use __reg_addr() helper for register access Yingchao Deng
2026-05-29 14:40 ` Leo Yan
2026-06-01 1:53 ` Yingchao Deng (Consultant) [this message]
2026-05-21 12:16 ` [PATCH v9 3/4] coresight: cti: add Qualcomm extended CTI identification and quirks Yingchao Deng
2026-05-22 2:38 ` Jie Gan
2026-05-28 17:11 ` Leo Yan
2026-05-29 1:42 ` Yingchao Deng (Consultant)
2026-05-21 12:16 ` [PATCH v9 4/4] coresight: cti: expose banked sysfs registers for Qualcomm extended CTI Yingchao Deng
2026-05-29 15:32 ` Leo Yan
2026-06-01 2:08 ` Yingchao Deng (Consultant)
2026-06-01 10:48 ` Leo Yan
2026-06-02 9:53 ` Yingchao Deng (Consultant)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e4b25ab7-a50e-416c-b2dd-1c8e8ef40019@quicinc.com \
--to=quic_yingdeng@quicinc$(echo .)com \
--cc=alexander.shishkin@linux$(echo .)intel.com \
--cc=coresight@lists$(echo .)linaro.org \
--cc=james.clark@linaro$(echo .)org \
--cc=jie.gan@oss$(echo .)qualcomm.com \
--cc=jinlong.mao@oss$(echo .)qualcomm.com \
--cc=leo.yan@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-arm-msm@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=mike.leach@arm$(echo .)com \
--cc=suzuki.poulose@arm$(echo .)com \
--cc=tingwei.zhang@oss$(echo .)qualcomm.com \
--cc=yingchao.deng@oss$(echo .)qualcomm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox