Re: [PATCH v9 2/4] coresight: cti: use __reg_addr() helper for register access

[Leo Yan <leo.yan@arm.com>]

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)
 {