[PATCH v2 0/8] Add support to configure TPDM CMB subunit

[PATCH v2 0/8] Add support to configure TPDM CMB subunit

[Tao Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 4538 bytes --]

Introduction of TPDM CMB(Continuous Multi Bit) subunit
CMB subunit is responsible for creating a dataset element, and is also
optionally responsible for packing it to fit multiple elements on a
single ATB transfer if possible in the configuration. The TPDM Core
Datapath requests timestamps be stored by the TPDA and then delivering
ATB sized data (depending on ATB width and element size, this could
be smaller or larger than a dataset element) to the ATB Mast FSM.
The CMB makes trace elements in two modes. In ‘continuous’ mode, every
valid data cycle creates an element. In ‘trace on change’ mode, when
valid data changes on the bus, a trace element is created. In
continuous mode, all cycles where this condition is true create trace
elements. In trace on change mode, a data element is only when the
previously sampled input is different from the current sampled input.

The CMB subunit must be configured prior to enablement. This series
adds support for TPDM to configure the configure CMB subunit.

Once this series patches are applied properly, the new tpdm nodes for
should be observed at the tpdm path /sys/bus/coresight/devices/tpdm*
which supports CMB subunit.
e.g.
root@qemuarm64:/sys/devices/platform/soc@0/684c000.tpdm/tpdm0# ls -l
-rw-r--r--    1 root     root          4096 Jan  1 00:00 cmb_mode
drwxr-xr-x    2 root     root             0 Jan  1 00:00 cmb_msr
drwxr-xr-x    2 root     root             0 Jan  1 00:00 cmb_patt
drwxr-xr-x    2 root     root             0 Jan  1 00:00 cmb_trig_patt
-rw-r--r--    1 root     root          4096 Jan  1 00:00 cmb_trig_ts
-rw-r--r--    1 root     root          4096 Jan  1 00:00 cmb_ts_all
drwxr-xr-x    2 root     root             0 Jan  1 00:00 connections
drwxr-xr-x    2 root     root             0 Jan  1 00:00 dsb_edge
drwxr-xr-x    2 root     root             0 Jan  1 00:00 dsb_msr
drwxr-xr-x    2 root     root             0 Jan  1 00:00 dsb_patt
drwxr-xr-x    2 root     root             0 Jan  1 00:00 dsb_trig_patt
-rw-r--r--    1 root     root          4096 Jan  1 00:00 enable_source
--w-------    1 root     root          4096 Jan  1 00:00 integration_test
drwxr-xr-x    2 root     root             0 Ja?  1 00:00 power
--w-------    1 root     root          4096 Jan  1 00:00 reset_dataset
lrwxrwxrwx    1 root     root             0 Apr  5  2021 subsystem -> ../../../../../bus/coresight
-rw-r--r--    1 root     root          4096 Apr  5  2021 uevent
-r--r--r--    1 root     root          4096 Jan  1 00:00 waiting_for_supplier

We can use the commands are similar to the below to configure the
TPDMs which support CMB subunit. Enable coresight sink first.
echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink
echo 1 > /sys/bus/coresight/devices/tpdm0/reset_dataset
echo 1 > /sys/bus/coresight/devices/tpdm0/cmb_mode
echo 1 > /sys/bus/coresight/devices/tpdm0/cmb_patt/enable_ts
echo 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/cmb_patt/tpmr0
echo 0 > /sys/bus/coresight/devices/tpdm0/cmb_trig_ts
echo 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/cmb_trig_patt/xpr1
echo 1 > /sys/bus/coresight/devices/tpdm0/enable_source

This patch series depends on patch "[v1] coresight-tpdm: Correct the property name of MSR number"
https://patchwork.kernel.org/project/linux-arm-kernel/patch/1698128353-31157-1-git-send-email-quic_taozha@quicinc.com/

codelinaro link:
https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/tpdm-cmb-v2

Changes in V2:
1. Optimizate and modify this patch series based on the patch series
"Add support to configure TPDM CMB subunit".
2. Modify the functions that read the element size of DSB/CMB in TPDA driver.

Tao Zhang (8):
  dt-bindings: arm: Add support for CMB element size
  coresight-tpda: Add support to configure CMB element
  coresight-tpdm: Add CMB dataset support
  coresight-tpdm: Add support to configure CMB
  coresight-tpdm: Add pattern registers support for CMB
  coresight-tpdm: Add timestamp control register support for the CMB
  dt-bindings: arm: Add support for TPDM CMB MSR register
  coresight-tpdm: Add msr register support for CMB

 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  83 +++++
 .../bindings/arm/qcom,coresight-tpdm.yaml          |  37 ++
 drivers/hwtracing/coresight/coresight-tpda.c       | 108 +++---
 drivers/hwtracing/coresight/coresight-tpda.h       |   6 +
 drivers/hwtracing/coresight/coresight-tpdm.c       | 390 ++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tpdm.h       |  87 +++++
 6 files changed, 663 insertions(+), 48 deletions(-)

-- 
2.7.4

[PATCH v2 1/8] dt-bindings: arm: Add support for CMB element size

[Tao Zhang]

Add property "qcom,cmb-elem-size" to support CMB(Continuous
Multi-Bit) element for TPDM. The associated aggregator will read
this size before it is enabled. CMB element size currently only
supports 32-bit and 64-bit.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../bindings/arm/qcom,coresight-tpdm.yaml          | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
index 61ddc3b..f9a2025 100644
--- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
@@ -52,6 +52,14 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint8
     enum: [32, 64]
 
+  qcom,cmb-element-size:
+    description:
+      Specifies the CMB(Continuous Multi-Bit) element size supported by
+      the monitor. The associated aggregator will read this size before it
+      is enabled. CMB element size currently only supports 32-bit and 64-bit.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [8, 32, 64]
+
   qcom,dsb-msrs-num:
     description:
       Specifies the number of DSB(Discrete Single Bit) MSR(mux select register)
@@ -110,4 +118,23 @@ examples:
       };
     };
 
+    tpdm@6c29000 {
+      compatible = "qcom,coresight-tpdm", "arm,primecell";
+      reg = <0x06c29000 0x1000>;
+      reg-names = "tpdm-base";
+
+      qcom,cmb-element-size = /bits/ 8 <64>;
+
+      clocks = <&aoss_qmp>;
+      clock-names = "apb_pclk";
+
+      out-ports {
+        port {
+          tpdm_ipcc_out_funnel_center: endpoint {
+            remote-endpoint =
+              <&funnel_center_in_tpdm_ipcc>;
+          };
+        };
+      };
+    };
 ...
-- 
2.7.4

[PATCH v2 2/8] coresight-tpda: Add support to configure CMB element

[Tao Zhang]

Read the CMB element size from the device tree. Set the register
bit that controls the CMB element size of the corresponding port.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpda.c | 108 ++++++++++++++++-----------
 drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
 2 files changed, 69 insertions(+), 45 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 5f82737..3101d2a 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct coresight_device *csdev)
 			CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
 }
 
+static void tpdm_clear_element_size(struct coresight_device *csdev)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	if (drvdata->dsb_esize)
+		drvdata->dsb_esize = 0;
+	if (drvdata->cmb_esize)
+		drvdata->cmb_esize = 0;
+}
+
+static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32 *val)
+{
+
+	if (drvdata->dsb_esize == 64)
+		*val |= TPDA_Pn_CR_DSBSIZE;
+	else if (drvdata->dsb_esize == 32)
+		*val &= ~TPDA_Pn_CR_DSBSIZE;
+
+	if (drvdata->cmb_esize == 64)
+		*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
+	else if (drvdata->cmb_esize == 32)
+		*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
+	else if (drvdata->cmb_esize == 8)
+		*val &= ~TPDA_Pn_CR_CMBSIZE;
+}
+
 /*
  * Read the DSB element size from the TPDM device
  * Returns
  *    The dsb element size read from the devicetree if available.
  *    0 - Otherwise, with a warning once.
  */
-static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
+static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
+				  struct coresight_device *csdev)
 {
-	int rc = 0;
-	u8 size = 0;
-
-	rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
-			"qcom,dsb-element-size", &size);
+	int rc = -EEXIST;
+
+	if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
+			"qcom,dsb-element-size", &drvdata->dsb_esize))
+		rc &= 0;
+	if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
+			"qcom,cmb-element-size", &drvdata->cmb_esize))
+		rc &= 0;
 	if (rc)
 		dev_warn_once(&csdev->dev,
-			"Failed to read TPDM DSB Element size: %d\n", rc);
+			"Failed to read TPDM Element size: %d\n", rc);
 
-	return size;
+	return rc;
 }
 
 /*
@@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
  * Parameter "inport" is used to pass in the input port number
  * of TPDA, and it is set to -1 in the recursize call.
  */
-static int tpda_get_element_size(struct coresight_device *csdev,
+static int tpda_get_element_size(struct tpda_drvdata *drvdata,
+				 struct coresight_device *csdev,
 				 int inport)
 {
-	int dsb_size = -ENOENT;
-	int i, size;
+	int rc = -ENOENT;
+	int i;
 	struct coresight_device *in;
 
+	if (inport > 0)
+		tpdm_clear_element_size(csdev);
+
 	for (i = 0; i < csdev->pdata->nr_inconns; i++) {
 		in = csdev->pdata->in_conns[i]->src_dev;
 		if (!in)
@@ -74,25 +108,20 @@ static int tpda_get_element_size(struct coresight_device *csdev,
 			continue;
 
 		if (coresight_device_is_tpdm(in)) {
-			size = tpdm_read_dsb_element_size(in);
+			if (rc)
+				rc = tpdm_read_element_size(drvdata, in);
+			else
+				return -EINVAL;
 		} else {
 			/* Recurse down the path */
-			size = tpda_get_element_size(in, -1);
-		}
-
-		if (size < 0)
-			return size;
-
-		if (dsb_size < 0) {
-			/* Found a size, save it. */
-			dsb_size = size;
-		} else {
-			/* Found duplicate TPDMs */
-			return -EEXIST;
+			rc = tpda_get_element_size(drvdata, in, -1);
 		}
 	}
 
-	return dsb_size;
+	if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
+		rc = 0;
+
+	return rc;
 }
 
 /* Settings pre enabling port control register */
@@ -109,7 +138,7 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
 static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
 {
 	u32 val;
-	int size;
+	int rc;
 
 	val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
 	/*
@@ -117,29 +146,18 @@ static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
 	 * Set the bit to 0 if the size is 32
 	 * Set the bit to 1 if the size is 64
 	 */
-	size = tpda_get_element_size(drvdata->csdev, port);
-	switch (size) {
-	case 32:
-		val &= ~TPDA_Pn_CR_DSBSIZE;
-		break;
-	case 64:
-		val |= TPDA_Pn_CR_DSBSIZE;
-		break;
-	case 0:
-		return -EEXIST;
-	case -EEXIST:
+	rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
+	if (!rc) {
+		tpda_set_element_size(drvdata, &val);
+		/* Enable the port */
+		val |= TPDA_Pn_CR_ENA;
+		writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
+	} else if (rc == -EINVAL) {
 		dev_warn_once(&drvdata->csdev->dev,
 			"Detected multiple TPDMs on port %d", -EEXIST);
-		return -EEXIST;
-	default:
-		return -EINVAL;
 	}
 
-	/* Enable the port */
-	val |= TPDA_Pn_CR_ENA;
-	writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
-
-	return 0;
+	return rc;
 }
 
 static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
index b3b38fd..29164fd 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.h
+++ b/drivers/hwtracing/coresight/coresight-tpda.h
@@ -10,6 +10,8 @@
 #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
 /* Aggregator port enable bit */
 #define TPDA_Pn_CR_ENA		BIT(0)
+/* Aggregator port CMB data set element size bit */
+#define TPDA_Pn_CR_CMBSIZE		GENMASK(7, 6)
 /* Aggregator port DSB data set element size bit */
 #define TPDA_Pn_CR_DSBSIZE		BIT(8)
 
@@ -25,6 +27,8 @@
  * @csdev:      component vitals needed by the framework.
  * @spinlock:   lock for the drvdata value.
  * @enable:     enable status of the component.
+ * @dsb_esize   Record the DSB element size.
+ * @cmb_esize   Record the CMB element size.
  */
 struct tpda_drvdata {
 	void __iomem		*base;
@@ -32,6 +36,8 @@ struct tpda_drvdata {
 	struct coresight_device	*csdev;
 	spinlock_t		spinlock;
 	u8			atid;
+	u8			dsb_esize;
+	u8			cmb_esize;
 };
 
 #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
-- 
2.7.4

[PATCH v2 3/8] coresight-tpdm: Add CMB dataset support

[Tao Zhang]

CMB (continuous multi-bit) is one of TPDM's dataset type. CMB subunit
can be enabled for data collection by writing 1 to the first bit of
CMB_CR register. This change is to add enable/disable function for
CMB dataset by writing CMB_CR register.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpdm.c | 31 ++++++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 97654aa..c8bb388 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -131,6 +131,11 @@ static bool tpdm_has_dsb_dataset(struct tpdm_drvdata *drvdata)
 	return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
 }
 
+static bool tpdm_has_cmb_dataset(struct tpdm_drvdata *drvdata)
+{
+	return (drvdata->datasets & TPDM_PIDR0_DS_CMB);
+}
+
 static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
 				   struct attribute *attr, int n)
 {
@@ -267,6 +272,17 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
+static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
+{
+	u32 val;
+
+	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
+	val |= TPDM_CMB_CR_ENA;
+
+	/* Set the enable bit of CMB control register to 1 */
+	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
+}
+
 /*
  * TPDM enable operations
  * The TPDM or Monitor serves as data collection component for various
@@ -281,6 +297,8 @@ static void __tpdm_enable(struct tpdm_drvdata *drvdata)
 
 	if (tpdm_has_dsb_dataset(drvdata))
 		tpdm_enable_dsb(drvdata);
+	if (tpdm_has_cmb_dataset(drvdata))
+		tpdm_enable_cmb(drvdata);
 
 	CS_LOCK(drvdata->base);
 }
@@ -314,6 +332,17 @@ static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
+static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
+{
+	u32 val;
+
+	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
+	val &= ~TPDM_CMB_CR_ENA;
+
+	/* Set the enable bit of CMB control register to 0 */
+	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
+}
+
 /* TPDM disable operations */
 static void __tpdm_disable(struct tpdm_drvdata *drvdata)
 {
@@ -321,6 +350,8 @@ static void __tpdm_disable(struct tpdm_drvdata *drvdata)
 
 	if (tpdm_has_dsb_dataset(drvdata))
 		tpdm_disable_dsb(drvdata);
+	if (tpdm_has_cmb_dataset(drvdata))
+		tpdm_disable_cmb(drvdata);
 
 	CS_LOCK(drvdata->base);
 }
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 4115b2a1..0098c58 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -9,6 +9,12 @@
 /* The max number of the datasets that TPDM supports */
 #define TPDM_DATASETS       7
 
+/* CMB Subunit Registers */
+#define TPDM_CMB_CR		(0xA00)
+
+/* Enable bit for CMB subunit */
+#define TPDM_CMB_CR_ENA		BIT(0)
+
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
@@ -79,10 +85,12 @@
  *
  * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
  * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
+ * PERIPHIDR0[2] : Fix to 1 if CMB subunit present, else 0
  */
 
 #define TPDM_PIDR0_DS_IMPDEF	BIT(0)
 #define TPDM_PIDR0_DS_DSB	BIT(1)
+#define TPDM_PIDR0_DS_CMB	BIT(2)
 
 #define TPDM_DSB_MAX_LINES	256
 /* MAX number of EDCR registers */
-- 
2.7.4

[PATCH v2 4/8] coresight-tpdm: Add support to configure CMB

[Tao Zhang]

TPDM CMB subunits support two forms of CMB data set element creation:
continuous and trace-on-change collection mode. Continuous change
creates CMB data set elements on every CMBCLK edge. Trace-on-change
creates CMB data set elements only when a new data set element differs
in value from the previous element in a CMB data set. Set CMB_CR.MODE
to 0 for continuous CMB collection mode. Set CMB_CR.MODE to 1 for
trace-on-change CMB collection mode

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 10 +++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 71 ++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h       | 12 ++++
 3 files changed, 93 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index f07218e..ace7231 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -170,3 +170,13 @@ Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_t
 Description:
 		(RW) Set/Get the MSR(mux select register) for the DSB subunit
 		TPDM.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_mode
+Date:		March 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:	(Write) Set the data collection mode of CMB tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Continuous CMB collection mode.
+		1 : Trace-on-change CMB collection mode.
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index c8bb388..efb376e 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -148,6 +148,18 @@ static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
 	return 0;
 }
 
+static umode_t tpdm_cmb_is_visible(struct kobject *kobj,
+				   struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	if (drvdata && tpdm_has_cmb_dataset(drvdata))
+		return attr->mode;
+
+	return 0;
+}
+
 static umode_t tpdm_dsb_msr_is_visible(struct kobject *kobj,
 				       struct attribute *attr, int n)
 {
@@ -172,6 +184,9 @@ static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
 		drvdata->dsb->trig_ts = true;
 		drvdata->dsb->trig_type = false;
 	}
+
+	if (tpdm_has_cmb_dataset(drvdata))
+		memset(drvdata->cmb, 0, sizeof(struct cmb_dataset));
 }
 
 static void set_dsb_mode(struct tpdm_drvdata *drvdata, u32 *val)
@@ -277,6 +292,16 @@ static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 	u32 val;
 
 	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
+	/*
+	 * Set to 0 for continuous CMB collection mode,
+	 * 1 for trace-on-change CMB collection mode.
+	 */
+	if (drvdata->cmb->trace_mode)
+		val |= TPDM_CMB_CR_MODE;
+	else
+		val &= ~TPDM_CMB_CR_MODE;
+
+	/* Set the enable bit of CMB control register to 1 */
 	val |= TPDM_CMB_CR_ENA;
 
 	/* Set the enable bit of CMB control register to 1 */
@@ -397,6 +422,12 @@ static int tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
 		if (!drvdata->dsb)
 			return -ENOMEM;
 	}
+	if (tpdm_has_cmb_dataset(drvdata) && (!drvdata->cmb)) {
+		drvdata->cmb = devm_kzalloc(drvdata->dev,
+						sizeof(*drvdata->cmb), GFP_KERNEL);
+		if (!drvdata->cmb)
+			return -ENOMEM;
+	}
 	tpdm_reset_datasets(drvdata);
 
 	return 0;
@@ -735,6 +766,35 @@ static ssize_t dsb_trig_ts_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(dsb_trig_ts);
 
+static ssize_t cmb_mode_show(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%x\n",
+			  drvdata->cmb->trace_mode);
+
+}
+
+static ssize_t cmb_mode_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf,
+			      size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long trace_mode;
+
+	if ((kstrtoul(buf, 0, &trace_mode)) || (trace_mode & ~1UL))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->cmb->trace_mode = trace_mode;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(cmb_mode);
+
 static struct attribute *tpdm_dsb_edge_attrs[] = {
 	&dev_attr_ctrl_idx.attr,
 	&dev_attr_ctrl_val.attr,
@@ -851,6 +911,11 @@ static struct attribute *tpdm_dsb_attrs[] = {
 	NULL,
 };
 
+static struct attribute *tpdm_cmb_attrs[] = {
+	&dev_attr_cmb_mode.attr,
+	NULL,
+};
+
 static struct attribute_group tpdm_dsb_attr_grp = {
 	.attrs = tpdm_dsb_attrs,
 	.is_visible = tpdm_dsb_is_visible,
@@ -880,6 +945,11 @@ static struct attribute_group tpdm_dsb_msr_grp = {
 	.name = "dsb_msr",
 };
 
+static struct attribute_group tpdm_cmb_attr_grp = {
+	.attrs = tpdm_cmb_attrs,
+	.is_visible = tpdm_cmb_is_visible,
+};
+
 static const struct attribute_group *tpdm_attr_grps[] = {
 	&tpdm_attr_grp,
 	&tpdm_dsb_attr_grp,
@@ -887,6 +957,7 @@ static const struct attribute_group *tpdm_attr_grps[] = {
 	&tpdm_dsb_trig_patt_grp,
 	&tpdm_dsb_patt_grp,
 	&tpdm_dsb_msr_grp,
+	&tpdm_cmb_attr_grp,
 	NULL,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 0098c58..c6b36d2 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -14,6 +14,8 @@
 
 /* Enable bit for CMB subunit */
 #define TPDM_CMB_CR_ENA		BIT(0)
+/* Trace collection mode for CMB subunit */
+#define TPDM_CMB_CR_MODE	BIT(1)
 
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
@@ -182,6 +184,14 @@ struct dsb_dataset {
 };
 
 /**
+ * struct cmb_dataset
+ * @trace_mode:       Dataset collection mode
+ */
+struct cmb_dataset {
+	u32			trace_mode;
+};
+
+/**
  * struct tpdm_drvdata - specifics associated to an TPDM component
  * @base:       memory mapped base address for this component.
  * @dev:        The device entity associated to this component.
@@ -190,6 +200,7 @@ struct dsb_dataset {
  * @enable:     enable status of the component.
  * @datasets:   The datasets types present of the TPDM.
  * @dsb         Specifics associated to TPDM DSB.
+ * @cmb         Specifics associated to TPDM CMB.
  * @dsb_msr_num Number of MSR supported by DSB TPDM
  */
 
@@ -201,6 +212,7 @@ struct tpdm_drvdata {
 	bool			enable;
 	unsigned long		datasets;
 	struct dsb_dataset	*dsb;
+	struct cmb_dataset	*cmb;
 	u32			dsb_msr_num;
 };
 
-- 
2.7.4

[PATCH v2 5/8] coresight-tpdm: Add pattern registers support for CMB

[Tao Zhang]

Timestamps are requested if the monitor’s CMB data set unit input
data matches the value in the Monitor CMB timestamp pattern and mask
registers (M_CMB_TPR and M_CMB_TPMR) when CMB timestamp enabled
via the timestamp insertion enable register bit(CMB_TIER.PATT_TSENAB).
The pattern match trigger output is achieved via setting values into
the CMB trigger pattern and mask registers (CMB_XPR and CMB_XPMR).
After configuring a pattern through these registers, the TPDM subunit
will assert an output trigger every time it receives new input data
that matches the configured pattern value. Values in a given bit
number of the mask register correspond to the same bit number in
the corresponding pattern register.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 30 ++++++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 88 +++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tpdm.h       | 39 ++++++++++
 3 files changed, 156 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index ace7231..c17468f 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -180,3 +180,33 @@ Description:	(Write) Set the data collection mode of CMB tpdm.
 		Accepts only one of the 2 values -  0 or 1.
 		0 : Continuous CMB collection mode.
 		1 : Trace-on-change CMB collection mode.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpr[0:1]
+Date:		March 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Set/Get the value of the trigger pattern for the CMB
+		subunit TPDM.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpmr[0:1]
+Date:		March 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Set/Get the mask of the trigger pattern for the CMB
+		subunit TPDM.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpr[0:1]
+Date:		March 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Set/Get the value of the pattern for the CMB subunit TPDM.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpmr[0:1]
+Date:		March 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index efb376e..894d430 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -66,6 +66,26 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev,
 			return -EINVAL;
 		return sysfs_emit(buf, "0x%x\n",
 				drvdata->dsb->msr[tpdm_attr->idx]);
+	case CMB_TRIG_PATT:
+		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+			return -EINVAL;
+		return sysfs_emit(buf, "0x%x\n",
+			drvdata->cmb->trig_patt[tpdm_attr->idx]);
+	case CMB_TRIG_PATT_MASK:
+		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+			return -EINVAL;
+		return sysfs_emit(buf, "0x%x\n",
+			drvdata->cmb->trig_patt_mask[tpdm_attr->idx]);
+	case CMB_PATT:
+		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+			return -EINVAL;
+		return sysfs_emit(buf, "0x%x\n",
+			drvdata->cmb->patt_val[tpdm_attr->idx]);
+	case CMB_PATT_MASK:
+		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+			return -EINVAL;
+		return sysfs_emit(buf, "0x%x\n",
+			drvdata->cmb->patt_mask[tpdm_attr->idx]);
 	}
 	return -EINVAL;
 }
@@ -118,6 +138,30 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
 		else
 			ret = -EINVAL;
 		break;
+	case CMB_TRIG_PATT:
+		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
+			drvdata->cmb->trig_patt[tpdm_attr->idx] = val;
+		else
+			ret = -EINVAL;
+		break;
+	case CMB_TRIG_PATT_MASK:
+		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
+			drvdata->cmb->trig_patt_mask[tpdm_attr->idx] = val;
+		else
+			ret = -EINVAL;
+		break;
+	case CMB_PATT:
+		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
+			drvdata->cmb->patt_val[tpdm_attr->idx] = val;
+		else
+			ret = -EINVAL;
+		break;
+	case CMB_PATT_MASK:
+		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
+			drvdata->cmb->patt_mask[tpdm_attr->idx] = val;
+		else
+			ret = -EINVAL;
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -289,7 +333,19 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 
 static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 {
-	u32 val;
+	u32 val, i;
+
+	/* Configure pattern registers */
+	for (i = 0; i < TPDM_CMB_MAX_PATT; i++) {
+		writel_relaxed(drvdata->cmb->patt_val[i],
+			    drvdata->base + TPDM_CMB_TPR(i));
+		writel_relaxed(drvdata->cmb->patt_mask[i],
+			    drvdata->base + TPDM_CMB_TPMR(i));
+		writel_relaxed(drvdata->cmb->trig_patt[i],
+			    drvdata->base + TPDM_CMB_XPR(i));
+		writel_relaxed(drvdata->cmb->trig_patt_mask[i],
+			    drvdata->base + TPDM_CMB_XPMR(i));
+	}
 
 	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
 	/*
@@ -904,6 +960,22 @@ static struct attribute *tpdm_dsb_msr_attrs[] = {
 	NULL,
 };
 
+static struct attribute *tpdm_cmb_trig_patt_attrs[] = {
+	CMB_TRIG_PATT_ATTR(0),
+	CMB_TRIG_PATT_ATTR(1),
+	CMB_TRIG_PATT_MASK_ATTR(0),
+	CMB_TRIG_PATT_MASK_ATTR(1),
+	NULL,
+};
+
+static struct attribute *tpdm_cmb_patt_attrs[] = {
+	CMB_PATT_ATTR(0),
+	CMB_PATT_ATTR(1),
+	CMB_PATT_MASK_ATTR(0),
+	CMB_PATT_MASK_ATTR(1),
+	NULL,
+};
+
 static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_mode.attr,
 	&dev_attr_dsb_trig_ts.attr,
@@ -950,6 +1022,18 @@ static struct attribute_group tpdm_cmb_attr_grp = {
 	.is_visible = tpdm_cmb_is_visible,
 };
 
+static struct attribute_group tpdm_cmb_trig_patt_grp = {
+	.attrs = tpdm_cmb_trig_patt_attrs,
+	.is_visible = tpdm_cmb_is_visible,
+	.name = "cmb_trig_patt",
+};
+
+static struct attribute_group tpdm_cmb_patt_grp = {
+	.attrs = tpdm_cmb_patt_attrs,
+	.is_visible = tpdm_cmb_is_visible,
+	.name = "cmb_patt",
+};
+
 static const struct attribute_group *tpdm_attr_grps[] = {
 	&tpdm_attr_grp,
 	&tpdm_dsb_attr_grp,
@@ -958,6 +1042,8 @@ static const struct attribute_group *tpdm_attr_grps[] = {
 	&tpdm_dsb_patt_grp,
 	&tpdm_dsb_msr_grp,
 	&tpdm_cmb_attr_grp,
+	&tpdm_cmb_trig_patt_grp,
+	&tpdm_cmb_patt_grp,
 	NULL,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index c6b36d2..e90d008c 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -11,12 +11,23 @@
 
 /* CMB Subunit Registers */
 #define TPDM_CMB_CR		(0xA00)
+/*CMB subunit timestamp pattern registers*/
+#define TPDM_CMB_TPR(n)		(0xA08 + (n * 4))
+/*CMB subunit timestamp pattern mask registers*/
+#define TPDM_CMB_TPMR(n)	(0xA10 + (n * 4))
+/*CMB subunit trigger pattern registers*/
+#define TPDM_CMB_XPR(n)		(0xA18 + (n * 4))
+/*CMB subunit trigger pattern mask registers*/
+#define TPDM_CMB_XPMR(n)	(0xA20 + (n * 4))
 
 /* Enable bit for CMB subunit */
 #define TPDM_CMB_CR_ENA		BIT(0)
 /* Trace collection mode for CMB subunit */
 #define TPDM_CMB_CR_MODE	BIT(1)
 
+/*Patten register number*/
+#define TPDM_CMB_MAX_PATT		2
+
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
@@ -151,6 +162,22 @@
 		tpdm_simple_dataset_rw(msr##nr,			\
 		DSB_MSR, nr)
 
+#define CMB_TRIG_PATT_ATTR(nr)					\
+		tpdm_simple_dataset_rw(xpr##nr,			\
+		CMB_TRIG_PATT, nr)
+
+#define CMB_TRIG_PATT_MASK_ATTR(nr)				\
+		tpdm_simple_dataset_rw(xpmr##nr,		\
+		CMB_TRIG_PATT_MASK, nr)
+
+#define CMB_PATT_ATTR(nr)					\
+		tpdm_simple_dataset_rw(tpr##nr,			\
+		CMB_PATT, nr)
+
+#define CMB_PATT_MASK_ATTR(nr)					\
+		tpdm_simple_dataset_rw(tpmr##nr,		\
+		CMB_PATT_MASK, nr)
+
 /**
  * struct dsb_dataset - specifics associated to dsb dataset
  * @mode:             DSB programming mode
@@ -186,9 +213,17 @@ struct dsb_dataset {
 /**
  * struct cmb_dataset
  * @trace_mode:       Dataset collection mode
+ * @patt_val:         Save value for pattern
+ * @patt_mask:        Save value for pattern mask
+ * @trig_patt:        Save value for trigger pattern
+ * @trig_patt_mask:   Save value for trigger pattern mask
  */
 struct cmb_dataset {
 	u32			trace_mode;
+	u32			patt_val[TPDM_CMB_MAX_PATT];
+	u32			patt_mask[TPDM_CMB_MAX_PATT];
+	u32			trig_patt[TPDM_CMB_MAX_PATT];
+	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
 };
 
 /**
@@ -225,6 +260,10 @@ enum dataset_mem {
 	DSB_PATT,
 	DSB_PATT_MASK,
 	DSB_MSR,
+	CMB_TRIG_PATT,
+	CMB_TRIG_PATT_MASK,
+	CMB_PATT,
+	CMB_PATT_MASK
 };
 
 /**
-- 
2.7.4

[PATCH v2 6/8] coresight-tpdm: Add timestamp control register support for the CMB

[Tao Zhang]

CMB_TIER register is CMB subunit timestamp insertion enable register.
Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
Set this bit to 1 to request a timestamp following a CMB CTI timestamp
request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
for all packets.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  35 +++++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 116 ++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tpdm.h       |  14 +++
 3 files changed, 162 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index c17468f..d58b33ee 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -210,3 +210,38 @@ KernelVersion	6.7
 Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
 Description:
 		(RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
+Date:		September 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the pattern timestamp of CMB tpdm. Read
+		the pattern timestamp of CMB tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Disable CMB pattern timestamp.
+		1 : Enable CMB pattern timestamp.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
+Date:		September 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Set/Get the trigger timestamp of the CMB for tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Set the CMB trigger type to false
+		1 : Set the CMB trigger type to true
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
+Date:		September 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Read or write the status of timestamp upon all interface.
+		Only value 0 and 1  can be written to this node. Set this node to 1 to requeset
+		timestamp to all trace packet.
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Disable the timestamp of all trace packets.
+		1 : Enable the timestamp of all trace packets.
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 894d430..f6cda56 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
+static void set_cmb_tier(struct tpdm_drvdata *drvdata)
+{
+	u32 val;
+
+	val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
+
+	/* Clear all relevant fields */
+	val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
+		 TPDM_CMB_TIER_XTRIG_TSENAB);
+
+	/* Set pattern timestamp type and enablement */
+	if (drvdata->cmb->patt_ts)
+		val |= TPDM_CMB_TIER_PATT_TSENAB;
+	else
+		val &= ~TPDM_CMB_TIER_PATT_TSENAB;
+
+	/* Set trigger timestamp */
+	if (drvdata->cmb->trig_ts)
+		val |= TPDM_CMB_TIER_XTRIG_TSENAB;
+	else
+		val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
+
+	/* Set all timestamp enablement*/
+	if (drvdata->cmb->ts_all)
+		val |= TPDM_CMB_TIER_TS_ALL;
+	else
+		val &= ~TPDM_CMB_TIER_TS_ALL;
+	writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
+}
+
 static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 {
 	u32 val, i;
@@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 			    drvdata->base + TPDM_CMB_XPMR(i));
 	}
 
+	set_cmb_tier(drvdata);
+
 	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
 	/*
 	 * Set to 0 for continuous CMB collection mode,
@@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device *dev,
 			      char *buf)
 {
 	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
 
-	return sysfs_emit(buf, "%u\n",
-			 (unsigned int)drvdata->dsb->patt_ts);
+	if (tpdm_has_dsb_dataset(drvdata))
+		size = sysfs_emit(buf, "%u\n",
+				 (unsigned int)drvdata->dsb->patt_ts);
+
+	if (tpdm_has_cmb_dataset(drvdata))
+		size = sysfs_emit(buf, "%u\n",
+				 (unsigned int)drvdata->cmb->patt_ts);
+
+	return size;
 }
 
 /*
@@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device *dev,
 		return -EINVAL;
 
 	spin_lock(&drvdata->spinlock);
-	drvdata->dsb->patt_ts = !!val;
+	if (tpdm_has_dsb_dataset(drvdata))
+		drvdata->dsb->patt_ts = !!val;
+
+	if (tpdm_has_cmb_dataset(drvdata))
+		drvdata->cmb->patt_ts = !!val;
 	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(enable_ts);
@@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(cmb_mode);
 
+static ssize_t cmb_ts_all_show(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n",
+			  (unsigned int)drvdata->cmb->ts_all);
+}
+
+static ssize_t cmb_ts_all_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf,
+				size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (val)
+		drvdata->cmb->ts_all = true;
+	else
+		drvdata->cmb->ts_all = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(cmb_ts_all);
+
+static ssize_t cmb_trig_ts_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n",
+			  (unsigned int)drvdata->cmb->trig_ts);
+}
+
+static ssize_t cmb_trig_ts_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf,
+				 size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (val)
+		drvdata->cmb->trig_ts = true;
+	else
+		drvdata->cmb->trig_ts = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(cmb_trig_ts);
+
 static struct attribute *tpdm_dsb_edge_attrs[] = {
 	&dev_attr_ctrl_idx.attr,
 	&dev_attr_ctrl_val.attr,
@@ -973,6 +1080,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
 	CMB_PATT_ATTR(1),
 	CMB_PATT_MASK_ATTR(0),
 	CMB_PATT_MASK_ATTR(1),
+	&dev_attr_enable_ts.attr,
 	NULL,
 };
 
@@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
 
 static struct attribute *tpdm_cmb_attrs[] = {
 	&dev_attr_cmb_mode.attr,
+	&dev_attr_cmb_ts_all.attr,
+	&dev_attr_cmb_trig_ts.attr,
 	NULL,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index e90d008c..65b7ca6 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -11,6 +11,8 @@
 
 /* CMB Subunit Registers */
 #define TPDM_CMB_CR		(0xA00)
+/*CMB subunit timestamp insertion enable register*/
+#define TPDM_CMB_TIER		(0xA04)
 /*CMB subunit timestamp pattern registers*/
 #define TPDM_CMB_TPR(n)		(0xA08 + (n * 4))
 /*CMB subunit timestamp pattern mask registers*/
@@ -24,6 +26,12 @@
 #define TPDM_CMB_CR_ENA		BIT(0)
 /* Trace collection mode for CMB subunit */
 #define TPDM_CMB_CR_MODE	BIT(1)
+/* Timestamp control for pattern match */
+#define TPDM_CMB_TIER_PATT_TSENAB	BIT(0)
+/* CMB CTI timestamp request */
+#define TPDM_CMB_TIER_XTRIG_TSENAB	BIT(1)
+/* For timestamp fo all trace */
+#define TPDM_CMB_TIER_TS_ALL		BIT(2)
 
 /*Patten register number*/
 #define TPDM_CMB_MAX_PATT		2
@@ -217,6 +225,9 @@ struct dsb_dataset {
  * @patt_mask:        Save value for pattern mask
  * @trig_patt:        Save value for trigger pattern
  * @trig_patt_mask:   Save value for trigger pattern mask
+ * @patt_ts:          Indicates if pattern match for timestamp is enabled.
+ * @trig_ts:          Indicates if CTI trigger for timestamp is enabled.
+ * @ts_all:           Indicates if timestamp is enabled for all packets.
  */
 struct cmb_dataset {
 	u32			trace_mode;
@@ -224,6 +235,9 @@ struct cmb_dataset {
 	u32			patt_mask[TPDM_CMB_MAX_PATT];
 	u32			trig_patt[TPDM_CMB_MAX_PATT];
 	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
+	bool			patt_ts;
+	bool			trig_ts;
+	bool			ts_all;
 };
 
 /**
-- 
2.7.4

[PATCH v2 7/8] dt-bindings: arm: Add support for TPDM CMB MSR register

[Tao Zhang]

Add property "qcom,cmb_msr_num" to support CMB MSR(mux select register)
for TPDM. It specifies the number of CMB MSR registers supported by
the TDPM.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
index f9a2025..a586b80a 100644
--- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
@@ -69,6 +69,15 @@ properties:
     minimum: 0
     maximum: 32
 
+  qcom,cmb-msrs-num:
+    description:
+      Specifies the number of CMB MSR(mux select register) registers supported
+      by the monitor. If this property is not configured or set to 0, it means
+      this TPDM doesn't support CMB MSR.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 32
+
   clocks:
     maxItems: 1
 
@@ -124,6 +133,7 @@ examples:
       reg-names = "tpdm-base";
 
       qcom,cmb-element-size = /bits/ 8 <64>;
+      qcom,cmb-msrs-num = <32>;
 
       clocks = <&aoss_qmp>;
       clock-names = "apb_pclk";
-- 
2.7.4

[PATCH v2 8/8] coresight-tpdm: Add msr register support for CMB

[Tao Zhang]

Add the nodes for CMB subunit MSR(mux select register) support.
CMB MSRs(mux select registers) is to separate mux,arbitration,
,interleaving,data packing control from stream filtering control.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  8 ++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 86 ++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h       | 16 +++-
 3 files changed, 109 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index d58b33ee..df0f837 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -245,3 +245,11 @@ Description:
 		Accepts only one of the 2 values -  0 or 1.
 		0 : Disable the timestamp of all trace packets.
 		1 : Enable the timestamp of all trace packets.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
+Date:		September 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Set/Get the MSR(mux select register) for the CMB subunit
+		TPDM.
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index f6cda56..7e331ea 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev,
 			return -EINVAL;
 		return sysfs_emit(buf, "0x%x\n",
 			drvdata->cmb->patt_mask[tpdm_attr->idx]);
+	case CMB_MSR:
+		if (tpdm_attr->idx >= drvdata->cmb_msr_num)
+			return -EINVAL;
+		return sysfs_emit(buf, "0x%x\n",
+				drvdata->cmb->msr[tpdm_attr->idx]);
 	}
 	return -EINVAL;
 }
@@ -162,6 +167,12 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
 		else
 			ret = -EINVAL;
 		break;
+	case CMB_MSR:
+		if (tpdm_attr->idx < drvdata->cmb_msr_num)
+			drvdata->cmb->msr[tpdm_attr->idx] = val;
+		else
+			ret = -EINVAL;
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -220,6 +231,23 @@ static umode_t tpdm_dsb_msr_is_visible(struct kobject *kobj,
 	return 0;
 }
 
+static umode_t tpdm_cmb_msr_is_visible(struct kobject *kobj,
+				       struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	struct device_attribute *dev_attr =
+		container_of(attr, struct device_attribute, attr);
+	struct tpdm_dataset_attribute *tpdm_attr =
+		container_of(dev_attr, struct tpdm_dataset_attribute, attr);
+
+	if (tpdm_attr->idx < drvdata->cmb_msr_num)
+		return attr->mode;
+
+	return 0;
+}
+
 static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
 {
 	if (tpdm_has_dsb_dataset(drvdata)) {
@@ -361,6 +389,15 @@ static void set_cmb_tier(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
 }
 
+static void set_cmb_msr(struct tpdm_drvdata *drvdata)
+{
+	int i;
+
+	for (i = 0; i < drvdata->cmb_msr_num; i++)
+		writel_relaxed(drvdata->cmb->msr[i],
+			   drvdata->base + TPDM_CMB_MSR(i));
+}
+
 static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 {
 	u32 val, i;
@@ -379,6 +416,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 
 	set_cmb_tier(drvdata);
 
+	set_cmb_msr(drvdata);
+
 	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
 	/*
 	 * Set to 0 for continuous CMB collection mode,
@@ -1084,6 +1123,42 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
 	NULL,
 };
 
+static struct attribute *tpdm_cmb_msr_attrs[] = {
+	CMB_MSR_ATTR(0),
+	CMB_MSR_ATTR(1),
+	CMB_MSR_ATTR(2),
+	CMB_MSR_ATTR(3),
+	CMB_MSR_ATTR(4),
+	CMB_MSR_ATTR(5),
+	CMB_MSR_ATTR(6),
+	CMB_MSR_ATTR(7),
+	CMB_MSR_ATTR(8),
+	CMB_MSR_ATTR(9),
+	CMB_MSR_ATTR(10),
+	CMB_MSR_ATTR(11),
+	CMB_MSR_ATTR(12),
+	CMB_MSR_ATTR(13),
+	CMB_MSR_ATTR(14),
+	CMB_MSR_ATTR(15),
+	CMB_MSR_ATTR(16),
+	CMB_MSR_ATTR(17),
+	CMB_MSR_ATTR(18),
+	CMB_MSR_ATTR(19),
+	CMB_MSR_ATTR(20),
+	CMB_MSR_ATTR(21),
+	CMB_MSR_ATTR(22),
+	CMB_MSR_ATTR(23),
+	CMB_MSR_ATTR(24),
+	CMB_MSR_ATTR(25),
+	CMB_MSR_ATTR(26),
+	CMB_MSR_ATTR(27),
+	CMB_MSR_ATTR(28),
+	CMB_MSR_ATTR(29),
+	CMB_MSR_ATTR(30),
+	CMB_MSR_ATTR(31),
+	NULL,
+};
+
 static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_mode.attr,
 	&dev_attr_dsb_trig_ts.attr,
@@ -1144,6 +1219,12 @@ static struct attribute_group tpdm_cmb_patt_grp = {
 	.name = "cmb_patt",
 };
 
+static struct attribute_group tpdm_cmb_msr_grp = {
+	.attrs = tpdm_cmb_msr_attrs,
+	.is_visible = tpdm_cmb_msr_is_visible,
+	.name = "cmb_msr",
+};
+
 static const struct attribute_group *tpdm_attr_grps[] = {
 	&tpdm_attr_grp,
 	&tpdm_dsb_attr_grp,
@@ -1154,6 +1235,7 @@ static const struct attribute_group *tpdm_attr_grps[] = {
 	&tpdm_cmb_attr_grp,
 	&tpdm_cmb_trig_patt_grp,
 	&tpdm_cmb_patt_grp,
+	&tpdm_cmb_msr_grp,
 	NULL,
 };
 
@@ -1192,6 +1274,10 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 		of_property_read_u32(drvdata->dev->of_node,
 			   "qcom,dsb-msrs-num", &drvdata->dsb_msr_num);
 
+	if (drvdata && tpdm_has_cmb_dataset(drvdata))
+		of_property_read_u32(drvdata->dev->of_node,
+			   "qcom,cmb-msrs-num", &drvdata->cmb_msr_num);
+
 	/* Set up coresight component description */
 	desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
 	if (!desc.name)
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 65b7ca6..255104d 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -21,6 +21,8 @@
 #define TPDM_CMB_XPR(n)		(0xA18 + (n * 4))
 /*CMB subunit trigger pattern mask registers*/
 #define TPDM_CMB_XPMR(n)	(0xA20 + (n * 4))
+/* CMB MSR register */
+#define TPDM_CMB_MSR(n)		(0xA80 + (n * 4))
 
 /* Enable bit for CMB subunit */
 #define TPDM_CMB_CR_ENA		BIT(0)
@@ -36,6 +38,9 @@
 /*Patten register number*/
 #define TPDM_CMB_MAX_PATT		2
 
+/* MAX number of DSB MSR */
+#define TPDM_CMB_MAX_MSR 32
+
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
@@ -186,6 +191,10 @@
 		tpdm_simple_dataset_rw(tpmr##nr,		\
 		CMB_PATT_MASK, nr)
 
+#define CMB_MSR_ATTR(nr)					\
+		tpdm_simple_dataset_rw(msr##nr,			\
+		CMB_MSR, nr)
+
 /**
  * struct dsb_dataset - specifics associated to dsb dataset
  * @mode:             DSB programming mode
@@ -225,6 +234,7 @@ struct dsb_dataset {
  * @patt_mask:        Save value for pattern mask
  * @trig_patt:        Save value for trigger pattern
  * @trig_patt_mask:   Save value for trigger pattern mask
+ * @msr               Save value for MSR
  * @patt_ts:          Indicates if pattern match for timestamp is enabled.
  * @trig_ts:          Indicates if CTI trigger for timestamp is enabled.
  * @ts_all:           Indicates if timestamp is enabled for all packets.
@@ -235,6 +245,7 @@ struct cmb_dataset {
 	u32			patt_mask[TPDM_CMB_MAX_PATT];
 	u32			trig_patt[TPDM_CMB_MAX_PATT];
 	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
+	u32			msr[TPDM_CMB_MAX_MSR];
 	bool			patt_ts;
 	bool			trig_ts;
 	bool			ts_all;
@@ -251,6 +262,7 @@ struct cmb_dataset {
  * @dsb         Specifics associated to TPDM DSB.
  * @cmb         Specifics associated to TPDM CMB.
  * @dsb_msr_num Number of MSR supported by DSB TPDM
+ * @cmb_msr_num Number of MSR supported by CMB TPDM
  */
 
 struct tpdm_drvdata {
@@ -263,6 +275,7 @@ struct tpdm_drvdata {
 	struct dsb_dataset	*dsb;
 	struct cmb_dataset	*cmb;
 	u32			dsb_msr_num;
+	u32			cmb_msr_num;
 };
 
 /* Enumerate members of various datasets */
@@ -277,7 +290,8 @@ enum dataset_mem {
 	CMB_TRIG_PATT,
 	CMB_TRIG_PATT_MASK,
 	CMB_PATT,
-	CMB_PATT_MASK
+	CMB_PATT_MASK,
+	CMB_MSR
 };
 
 /**
-- 
2.7.4

Re: [PATCH v2 1/8] dt-bindings: arm: Add support for CMB element size

[Rob Herring]

On Wed, Oct 25, 2023 at 10:53:21AM +0800, Tao Zhang wrote:
> Add property "qcom,cmb-elem-size" to support CMB(Continuous
> Multi-Bit) element for TPDM. The associated aggregator will read
> this size before it is enabled. CMB element size currently only
> supports 32-bit and 64-bit.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../bindings/arm/qcom,coresight-tpdm.yaml          | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> index 61ddc3b..f9a2025 100644
> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> @@ -52,6 +52,14 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint8
>      enum: [32, 64]
>  
> +  qcom,cmb-element-size:

What are the units? Use '-bits' suffix.

> +    description:
> +      Specifies the CMB(Continuous Multi-Bit) element size supported by
> +      the monitor. The associated aggregator will read this size before it
> +      is enabled. CMB element size currently only supports 32-bit and 64-bit.

The enum says 8-bit is supported.

> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    enum: [8, 32, 64]
> +
>    qcom,dsb-msrs-num:
>      description:
>        Specifies the number of DSB(Discrete Single Bit) MSR(mux select register)
> @@ -110,4 +118,23 @@ examples:
>        };
>      };
>  
> +    tpdm@6c29000 {
> +      compatible = "qcom,coresight-tpdm", "arm,primecell";
> +      reg = <0x06c29000 0x1000>;
> +      reg-names = "tpdm-base";
> +
> +      qcom,cmb-element-size = /bits/ 8 <64>;
> +
> +      clocks = <&aoss_qmp>;
> +      clock-names = "apb_pclk";
> +
> +      out-ports {
> +        port {
> +          tpdm_ipcc_out_funnel_center: endpoint {
> +            remote-endpoint =
> +              <&funnel_center_in_tpdm_ipcc>;
> +          };
> +        };
> +      };
> +    };
>  ...
> -- 
> 2.7.4
> 

Re: [PATCH v2 7/8] dt-bindings: arm: Add support for TPDM CMB MSR register

[Rob Herring]

On Wed, Oct 25, 2023 at 10:53:27AM +0800, Tao Zhang wrote:
> Add property "qcom,cmb_msr_num" to support CMB MSR(mux select register)
> for TPDM. It specifies the number of CMB MSR registers supported by
> the TDPM.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> index f9a2025..a586b80a 100644
> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> @@ -69,6 +69,15 @@ properties:
>      minimum: 0
>      maximum: 32
>  
> +  qcom,cmb-msrs-num:
> +    description:
> +      Specifies the number of CMB MSR(mux select register) registers supported
> +      by the monitor. If this property is not configured or set to 0, it means
> +      this TPDM doesn't support CMB MSR.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 32

default: 0

> +
>    clocks:
>      maxItems: 1
>  
> @@ -124,6 +133,7 @@ examples:
>        reg-names = "tpdm-base";
>  
>        qcom,cmb-element-size = /bits/ 8 <64>;
> +      qcom,cmb-msrs-num = <32>;
>  
>        clocks = <&aoss_qmp>;
>        clock-names = "apb_pclk";
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/8] coresight-tpda: Add support to configure CMB element

[James Clark]

On 25/10/2023 03:53, Tao Zhang wrote:
> Read the CMB element size from the device tree. Set the register
> bit that controls the CMB element size of the corresponding port.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-tpda.c | 108 ++++++++++++++++-----------
>  drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
>  2 files changed, 69 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 5f82737..3101d2a 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct coresight_device *csdev)
>  			CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>  }
>  
> +static void tpdm_clear_element_size(struct coresight_device *csdev)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +	if (drvdata->dsb_esize)
> +		drvdata->dsb_esize = 0;
> +	if (drvdata->cmb_esize)
> +		drvdata->cmb_esize = 0;
> +}
> +
> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32 *val)
> +{
> +
> +	if (drvdata->dsb_esize == 64)
> +		*val |= TPDA_Pn_CR_DSBSIZE;
> +	else if (drvdata->dsb_esize == 32)
> +		*val &= ~TPDA_Pn_CR_DSBSIZE;
> +
> +	if (drvdata->cmb_esize == 64)
> +		*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
> +	else if (drvdata->cmb_esize == 32)
> +		*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
> +	else if (drvdata->cmb_esize == 8)
> +		*val &= ~TPDA_Pn_CR_CMBSIZE;
> +}
> +
>  /*
>   * Read the DSB element size from the TPDM device
>   * Returns
>   *    The dsb element size read from the devicetree if available.

Hi Tao,

I think the function description and the return value description above
need updating now.

>   *    0 - Otherwise, with a warning once.
>   */
> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
> +				  struct coresight_device *csdev)
>  {
> -	int rc = 0;
> -	u8 size = 0;
> -
> -	rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> -			"qcom,dsb-element-size", &size);
> +	int rc = -EEXIST;
> +
> +	if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> +			"qcom,dsb-element-size", &drvdata->dsb_esize))
> +		rc &= 0;

Is &= 0 significant? Why not = 0?

> +	if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> +			"qcom,cmb-element-size", &drvdata->cmb_esize))
> +		rc &= 0;
>  	if (rc)
>  		dev_warn_once(&csdev->dev,
> -			"Failed to read TPDM DSB Element size: %d\n", rc);
> +			"Failed to read TPDM Element size: %d\n", rc);
>  
> -	return size;
> +	return rc;
>  }
>  
>  /*
> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>   * Parameter "inport" is used to pass in the input port number
>   * of TPDA, and it is set to -1 in the recursize call.
>   */
> -static int tpda_get_element_size(struct coresight_device *csdev,
> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
> +				 struct coresight_device *csdev,
>  				 int inport)
>  {
> -	int dsb_size = -ENOENT;
> -	int i, size;
> +	int rc = -ENOENT;
> +	int i;
>  	struct coresight_device *in;
>  
> +	if (inport > 0)
> +		tpdm_clear_element_size(csdev);
> +
>  	for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>  		in = csdev->pdata->in_conns[i]->src_dev;
>  		if (!in)
> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct coresight_device *csdev,
>  			continue;
>  
>  		if (coresight_device_is_tpdm(in)) {
> -			size = tpdm_read_dsb_element_size(in);
> +			if (rc)
> +				rc = tpdm_read_element_size(drvdata, in);
> +			else
> +				return -EINVAL;

This is quite hard to follow, is checking rc here before calling
tpdm_read_element_size() some kind of way of only calling it once?

rc isn't actually a return value at this point, it's just default
initialised to -ENOENT.

Then it's not clear why the else condition returns an error?

>  		} else {
>  			/* Recurse down the path */
> -			size = tpda_get_element_size(in, -1);
> -		}
> -
> -		if (size < 0)
> -			return size;
> -
> -		if (dsb_size < 0) {
> -			/* Found a size, save it. */
> -			dsb_size = size;
> -		} else {
> -			/* Found duplicate TPDMs */
> -			return -EEXIST;
> +			rc = tpda_get_element_size(drvdata, in, -1);

And then why it's assigned here but not checked for an error in this case?

Maybe some comments explaining the flow would help. Or to me it seems
like a second variable to track the thing that's actually intended could
be used as well. Like "bool found_element" or something, rather than
re-using rc to also track another thing.

>  		}
>  	}
>  
> -	return dsb_size;
> +	if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
> +		rc = 0;
> +
> +	return rc;
>  }
>  
>  /* Settings pre enabling port control register */
> @@ -109,7 +138,7 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>  static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>  {
>  	u32 val;
> -	int size;
> +	int rc;
>  
>  	val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
>  	/*
> @@ -117,29 +146,18 @@ static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>  	 * Set the bit to 0 if the size is 32
>  	 * Set the bit to 1 if the size is 64
>  	 */
> -	size = tpda_get_element_size(drvdata->csdev, port);
> -	switch (size) {
> -	case 32:
> -		val &= ~TPDA_Pn_CR_DSBSIZE;
> -		break;
> -	case 64:
> -		val |= TPDA_Pn_CR_DSBSIZE;
> -		break;
> -	case 0:
> -		return -EEXIST;
> -	case -EEXIST:
> +	rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
> +	if (!rc) {
> +		tpda_set_element_size(drvdata, &val);
> +		/* Enable the port */
> +		val |= TPDA_Pn_CR_ENA;
> +		writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> +	} else if (rc == -EINVAL) {
>  		dev_warn_once(&drvdata->csdev->dev,
>  			"Detected multiple TPDMs on port %d", -EEXIST);

tpdm_read_element_size() can return EEXIST, but then it gets changed
back to EINVAL in tpda_get_element_size() without caring about the
specific code, before finally being changed back to EEXIST for the
warning here. Can it just be propagated as the original value? Or use
EINVAL or EEXIST all the way through. That would probably be easier to
follow.

And then also a comment about why the other possible error values don't
result in a warning.

Thanks
James

> -		return -EEXIST;
> -	default:
> -		return -EINVAL;
>  	}
>  
> -	/* Enable the port */
> -	val |= TPDA_Pn_CR_ENA;
> -	writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> -
> -	return 0;
> +	return rc;
>  }
>  
>  static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index b3b38fd..29164fd 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.h
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -10,6 +10,8 @@
>  #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
>  /* Aggregator port enable bit */
>  #define TPDA_Pn_CR_ENA		BIT(0)
> +/* Aggregator port CMB data set element size bit */
> +#define TPDA_Pn_CR_CMBSIZE		GENMASK(7, 6)
>  /* Aggregator port DSB data set element size bit */
>  #define TPDA_Pn_CR_DSBSIZE		BIT(8)
>  
> @@ -25,6 +27,8 @@
>   * @csdev:      component vitals needed by the framework.
>   * @spinlock:   lock for the drvdata value.
>   * @enable:     enable status of the component.
> + * @dsb_esize   Record the DSB element size.
> + * @cmb_esize   Record the CMB element size.
>   */
>  struct tpda_drvdata {
>  	void __iomem		*base;
> @@ -32,6 +36,8 @@ struct tpda_drvdata {
>  	struct coresight_device	*csdev;
>  	spinlock_t		spinlock;
>  	u8			atid;
> +	u8			dsb_esize;
> +	u8			cmb_esize;
>  };
>  
>  #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */

Re: [PATCH v2 3/8] coresight-tpdm: Add CMB dataset support

[James Clark]

On 25/10/2023 03:53, Tao Zhang wrote:
> CMB (continuous multi-bit) is one of TPDM's dataset type. CMB subunit
> can be enabled for data collection by writing 1 to the first bit of
> CMB_CR register. This change is to add enable/disable function for
> CMB dataset by writing CMB_CR register.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-tpdm.c | 31 ++++++++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++++++
>  2 files changed, 39 insertions(+)
> 

Reviewed-by: James Clark <james.clark@arm.com>

> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 97654aa..c8bb388 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -131,6 +131,11 @@ static bool tpdm_has_dsb_dataset(struct tpdm_drvdata *drvdata)
>  	return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
>  }
>  
> +static bool tpdm_has_cmb_dataset(struct tpdm_drvdata *drvdata)
> +{
> +	return (drvdata->datasets & TPDM_PIDR0_DS_CMB);
> +}
> +
>  static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
>  				   struct attribute *attr, int n)
>  {
> @@ -267,6 +272,17 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>  	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>  }
>  
> +static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
> +	val |= TPDM_CMB_CR_ENA;
> +
> +	/* Set the enable bit of CMB control register to 1 */
> +	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
> +}
> +
>  /*
>   * TPDM enable operations
>   * The TPDM or Monitor serves as data collection component for various
> @@ -281,6 +297,8 @@ static void __tpdm_enable(struct tpdm_drvdata *drvdata)
>  
>  	if (tpdm_has_dsb_dataset(drvdata))
>  		tpdm_enable_dsb(drvdata);
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		tpdm_enable_cmb(drvdata);
>  
>  	CS_LOCK(drvdata->base);
>  }
> @@ -314,6 +332,17 @@ static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
>  	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>  }
>  
> +static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
> +	val &= ~TPDM_CMB_CR_ENA;
> +
> +	/* Set the enable bit of CMB control register to 0 */
> +	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
> +}
> +
>  /* TPDM disable operations */
>  static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>  {
> @@ -321,6 +350,8 @@ static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>  
>  	if (tpdm_has_dsb_dataset(drvdata))
>  		tpdm_disable_dsb(drvdata);
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		tpdm_disable_cmb(drvdata);
>  
>  	CS_LOCK(drvdata->base);
>  }
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 4115b2a1..0098c58 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -9,6 +9,12 @@
>  /* The max number of the datasets that TPDM supports */
>  #define TPDM_DATASETS       7
>  
> +/* CMB Subunit Registers */
> +#define TPDM_CMB_CR		(0xA00)
> +
> +/* Enable bit for CMB subunit */
> +#define TPDM_CMB_CR_ENA		BIT(0)
> +
>  /* DSB Subunit Registers */
>  #define TPDM_DSB_CR		(0x780)
>  #define TPDM_DSB_TIER		(0x784)
> @@ -79,10 +85,12 @@
>   *
>   * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
>   * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
> + * PERIPHIDR0[2] : Fix to 1 if CMB subunit present, else 0
>   */
>  
>  #define TPDM_PIDR0_DS_IMPDEF	BIT(0)
>  #define TPDM_PIDR0_DS_DSB	BIT(1)
> +#define TPDM_PIDR0_DS_CMB	BIT(2)
>  
>  #define TPDM_DSB_MAX_LINES	256
>  /* MAX number of EDCR registers */

Re: [PATCH v2 4/8] coresight-tpdm: Add support to configure CMB

[James Clark]

On 25/10/2023 03:53, Tao Zhang wrote:
> TPDM CMB subunits support two forms of CMB data set element creation:
> continuous and trace-on-change collection mode. Continuous change
> creates CMB data set elements on every CMBCLK edge. Trace-on-change
> creates CMB data set elements only when a new data set element differs
> in value from the previous element in a CMB data set. Set CMB_CR.MODE
> to 0 for continuous CMB collection mode. Set CMB_CR.MODE to 1 for
> trace-on-change CMB collection mode
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
> ---
>  .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 10 +++
>  drivers/hwtracing/coresight/coresight-tpdm.c       | 71 ++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tpdm.h       | 12 ++++
>  3 files changed, 93 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index f07218e..ace7231 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -170,3 +170,13 @@ Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_t
>  Description:
>  		(RW) Set/Get the MSR(mux select register) for the DSB subunit
>  		TPDM.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_mode
> +Date:		March 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:	(Write) Set the data collection mode of CMB tpdm.

I know it's expanded elsewhere, but it's probably worth expanding the
CMB abbreviation here as well so people reading the docs don't have to
go into the code.

Otherwise:

Reviewed-by: James Clark <james.clark@arm.com>

> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Continuous CMB collection mode.
> +		1 : Trace-on-change CMB collection mode.
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index c8bb388..efb376e 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -148,6 +148,18 @@ static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
>  	return 0;
>  }

[...]

Re: [PATCH v2 5/8] coresight-tpdm: Add pattern registers support for CMB

[James Clark]

On 25/10/2023 03:53, Tao Zhang wrote:
> Timestamps are requested if the monitor’s CMB data set unit input
> data matches the value in the Monitor CMB timestamp pattern and mask
> registers (M_CMB_TPR and M_CMB_TPMR) when CMB timestamp enabled
> via the timestamp insertion enable register bit(CMB_TIER.PATT_TSENAB).
> The pattern match trigger output is achieved via setting values into
> the CMB trigger pattern and mask registers (CMB_XPR and CMB_XPMR).
> After configuring a pattern through these registers, the TPDM subunit
> will assert an output trigger every time it receives new input data
> that matches the configured pattern value. Values in a given bit
> number of the mask register correspond to the same bit number in
> the corresponding pattern register.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>

Reviewed-by: James Clark <james.clark@arm.com>

> ---
>  .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 30 ++++++++
>  drivers/hwtracing/coresight/coresight-tpdm.c       | 88 +++++++++++++++++++++-
>  drivers/hwtracing/coresight/coresight-tpdm.h       | 39 ++++++++++
>  3 files changed, 156 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index ace7231..c17468f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -180,3 +180,33 @@ Description:	(Write) Set the data collection mode of CMB tpdm.
>  		Accepts only one of the 2 values -  0 or 1.
>  		0 : Continuous CMB collection mode.
>  		1 : Trace-on-change CMB collection mode.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpr[0:1]
> +Date:		March 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Set/Get the value of the trigger pattern for the CMB
> +		subunit TPDM.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpmr[0:1]
> +Date:		March 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Set/Get the mask of the trigger pattern for the CMB
> +		subunit TPDM.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpr[0:1]
> +Date:		March 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Set/Get the value of the pattern for the CMB subunit TPDM.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpmr[0:1]
> +Date:		March 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index efb376e..894d430 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -66,6 +66,26 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev,
>  			return -EINVAL;
>  		return sysfs_emit(buf, "0x%x\n",
>  				drvdata->dsb->msr[tpdm_attr->idx]);
> +	case CMB_TRIG_PATT:
> +		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> +			return -EINVAL;
> +		return sysfs_emit(buf, "0x%x\n",
> +			drvdata->cmb->trig_patt[tpdm_attr->idx]);
> +	case CMB_TRIG_PATT_MASK:
> +		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> +			return -EINVAL;
> +		return sysfs_emit(buf, "0x%x\n",
> +			drvdata->cmb->trig_patt_mask[tpdm_attr->idx]);
> +	case CMB_PATT:
> +		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> +			return -EINVAL;
> +		return sysfs_emit(buf, "0x%x\n",
> +			drvdata->cmb->patt_val[tpdm_attr->idx]);
> +	case CMB_PATT_MASK:
> +		if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> +			return -EINVAL;
> +		return sysfs_emit(buf, "0x%x\n",
> +			drvdata->cmb->patt_mask[tpdm_attr->idx]);
>  	}
>  	return -EINVAL;
>  }
> @@ -118,6 +138,30 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
>  		else
>  			ret = -EINVAL;
>  		break;
> +	case CMB_TRIG_PATT:
> +		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
> +			drvdata->cmb->trig_patt[tpdm_attr->idx] = val;
> +		else
> +			ret = -EINVAL;
> +		break;
> +	case CMB_TRIG_PATT_MASK:
> +		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
> +			drvdata->cmb->trig_patt_mask[tpdm_attr->idx] = val;
> +		else
> +			ret = -EINVAL;
> +		break;
> +	case CMB_PATT:
> +		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
> +			drvdata->cmb->patt_val[tpdm_attr->idx] = val;
> +		else
> +			ret = -EINVAL;
> +		break;
> +	case CMB_PATT_MASK:
> +		if (tpdm_attr->idx < TPDM_CMB_MAX_PATT)
> +			drvdata->cmb->patt_mask[tpdm_attr->idx] = val;
> +		else
> +			ret = -EINVAL;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -289,7 +333,19 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>  
>  static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>  {
> -	u32 val;
> +	u32 val, i;
> +
> +	/* Configure pattern registers */
> +	for (i = 0; i < TPDM_CMB_MAX_PATT; i++) {
> +		writel_relaxed(drvdata->cmb->patt_val[i],
> +			    drvdata->base + TPDM_CMB_TPR(i));
> +		writel_relaxed(drvdata->cmb->patt_mask[i],
> +			    drvdata->base + TPDM_CMB_TPMR(i));
> +		writel_relaxed(drvdata->cmb->trig_patt[i],
> +			    drvdata->base + TPDM_CMB_XPR(i));
> +		writel_relaxed(drvdata->cmb->trig_patt_mask[i],
> +			    drvdata->base + TPDM_CMB_XPMR(i));
> +	}
>  
>  	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>  	/*
> @@ -904,6 +960,22 @@ static struct attribute *tpdm_dsb_msr_attrs[] = {
>  	NULL,
>  };
>  
> +static struct attribute *tpdm_cmb_trig_patt_attrs[] = {
> +	CMB_TRIG_PATT_ATTR(0),
> +	CMB_TRIG_PATT_ATTR(1),
> +	CMB_TRIG_PATT_MASK_ATTR(0),
> +	CMB_TRIG_PATT_MASK_ATTR(1),
> +	NULL,
> +};
> +
> +static struct attribute *tpdm_cmb_patt_attrs[] = {
> +	CMB_PATT_ATTR(0),
> +	CMB_PATT_ATTR(1),
> +	CMB_PATT_MASK_ATTR(0),
> +	CMB_PATT_MASK_ATTR(1),
> +	NULL,
> +};
> +
>  static struct attribute *tpdm_dsb_attrs[] = {
>  	&dev_attr_dsb_mode.attr,
>  	&dev_attr_dsb_trig_ts.attr,
> @@ -950,6 +1022,18 @@ static struct attribute_group tpdm_cmb_attr_grp = {
>  	.is_visible = tpdm_cmb_is_visible,
>  };
>  
> +static struct attribute_group tpdm_cmb_trig_patt_grp = {
> +	.attrs = tpdm_cmb_trig_patt_attrs,
> +	.is_visible = tpdm_cmb_is_visible,
> +	.name = "cmb_trig_patt",
> +};
> +
> +static struct attribute_group tpdm_cmb_patt_grp = {
> +	.attrs = tpdm_cmb_patt_attrs,
> +	.is_visible = tpdm_cmb_is_visible,
> +	.name = "cmb_patt",
> +};
> +
>  static const struct attribute_group *tpdm_attr_grps[] = {
>  	&tpdm_attr_grp,
>  	&tpdm_dsb_attr_grp,
> @@ -958,6 +1042,8 @@ static const struct attribute_group *tpdm_attr_grps[] = {
>  	&tpdm_dsb_patt_grp,
>  	&tpdm_dsb_msr_grp,
>  	&tpdm_cmb_attr_grp,
> +	&tpdm_cmb_trig_patt_grp,
> +	&tpdm_cmb_patt_grp,
>  	NULL,
>  };
>  
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index c6b36d2..e90d008c 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -11,12 +11,23 @@
>  
>  /* CMB Subunit Registers */
>  #define TPDM_CMB_CR		(0xA00)
> +/*CMB subunit timestamp pattern registers*/
> +#define TPDM_CMB_TPR(n)		(0xA08 + (n * 4))
> +/*CMB subunit timestamp pattern mask registers*/
> +#define TPDM_CMB_TPMR(n)	(0xA10 + (n * 4))
> +/*CMB subunit trigger pattern registers*/
> +#define TPDM_CMB_XPR(n)		(0xA18 + (n * 4))
> +/*CMB subunit trigger pattern mask registers*/
> +#define TPDM_CMB_XPMR(n)	(0xA20 + (n * 4))
>  
>  /* Enable bit for CMB subunit */
>  #define TPDM_CMB_CR_ENA		BIT(0)
>  /* Trace collection mode for CMB subunit */
>  #define TPDM_CMB_CR_MODE	BIT(1)
>  
> +/*Patten register number*/
> +#define TPDM_CMB_MAX_PATT		2
> +
>  /* DSB Subunit Registers */
>  #define TPDM_DSB_CR		(0x780)
>  #define TPDM_DSB_TIER		(0x784)
> @@ -151,6 +162,22 @@
>  		tpdm_simple_dataset_rw(msr##nr,			\
>  		DSB_MSR, nr)
>  
> +#define CMB_TRIG_PATT_ATTR(nr)					\
> +		tpdm_simple_dataset_rw(xpr##nr,			\
> +		CMB_TRIG_PATT, nr)
> +
> +#define CMB_TRIG_PATT_MASK_ATTR(nr)				\
> +		tpdm_simple_dataset_rw(xpmr##nr,		\
> +		CMB_TRIG_PATT_MASK, nr)
> +
> +#define CMB_PATT_ATTR(nr)					\
> +		tpdm_simple_dataset_rw(tpr##nr,			\
> +		CMB_PATT, nr)
> +
> +#define CMB_PATT_MASK_ATTR(nr)					\
> +		tpdm_simple_dataset_rw(tpmr##nr,		\
> +		CMB_PATT_MASK, nr)
> +
>  /**
>   * struct dsb_dataset - specifics associated to dsb dataset
>   * @mode:             DSB programming mode
> @@ -186,9 +213,17 @@ struct dsb_dataset {
>  /**
>   * struct cmb_dataset
>   * @trace_mode:       Dataset collection mode
> + * @patt_val:         Save value for pattern
> + * @patt_mask:        Save value for pattern mask
> + * @trig_patt:        Save value for trigger pattern
> + * @trig_patt_mask:   Save value for trigger pattern mask
>   */
>  struct cmb_dataset {
>  	u32			trace_mode;
> +	u32			patt_val[TPDM_CMB_MAX_PATT];
> +	u32			patt_mask[TPDM_CMB_MAX_PATT];
> +	u32			trig_patt[TPDM_CMB_MAX_PATT];
> +	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
>  };
>  
>  /**
> @@ -225,6 +260,10 @@ enum dataset_mem {
>  	DSB_PATT,
>  	DSB_PATT_MASK,
>  	DSB_MSR,
> +	CMB_TRIG_PATT,
> +	CMB_TRIG_PATT_MASK,
> +	CMB_PATT,
> +	CMB_PATT_MASK
>  };
>  
>  /**

Re: [PATCH v2 6/8] coresight-tpdm: Add timestamp control register support for the CMB

[James Clark]

On 25/10/2023 03:53, Tao Zhang wrote:
> CMB_TIER register is CMB subunit timestamp insertion enable register.
> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
> Set this bit to 1 to request a timestamp following a CMB CTI timestamp
> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
> for all packets.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
> ---
>  .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  35 +++++++
>  drivers/hwtracing/coresight/coresight-tpdm.c       | 116 ++++++++++++++++++++-
>  drivers/hwtracing/coresight/coresight-tpdm.h       |  14 +++
>  3 files changed, 162 insertions(+), 3 deletions(-)
> 

Reviewed-by: James Clark <james.clark@arm.com>

> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index c17468f..d58b33ee 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -210,3 +210,38 @@ KernelVersion	6.7
>  Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>  Description:
>  		(RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
> +Date:		September 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the pattern timestamp of CMB tpdm. Read
> +		the pattern timestamp of CMB tpdm.
> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Disable CMB pattern timestamp.
> +		1 : Enable CMB pattern timestamp.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
> +Date:		September 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Set/Get the trigger timestamp of the CMB for tpdm.
> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Set the CMB trigger type to false
> +		1 : Set the CMB trigger type to true
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
> +Date:		September 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Read or write the status of timestamp upon all interface.
> +		Only value 0 and 1  can be written to this node. Set this node to 1 to requeset
> +		timestamp to all trace packet.
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Disable the timestamp of all trace packets.
> +		1 : Enable the timestamp of all trace packets.
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 894d430..f6cda56 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>  	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>  }
>  
> +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
> +
> +	/* Clear all relevant fields */
> +	val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
> +		 TPDM_CMB_TIER_XTRIG_TSENAB);
> +
> +	/* Set pattern timestamp type and enablement */
> +	if (drvdata->cmb->patt_ts)
> +		val |= TPDM_CMB_TIER_PATT_TSENAB;
> +	else
> +		val &= ~TPDM_CMB_TIER_PATT_TSENAB;
> +
> +	/* Set trigger timestamp */
> +	if (drvdata->cmb->trig_ts)
> +		val |= TPDM_CMB_TIER_XTRIG_TSENAB;
> +	else
> +		val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
> +
> +	/* Set all timestamp enablement*/
> +	if (drvdata->cmb->ts_all)
> +		val |= TPDM_CMB_TIER_TS_ALL;
> +	else
> +		val &= ~TPDM_CMB_TIER_TS_ALL;
> +	writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
> +}
> +
>  static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>  {
>  	u32 val, i;
> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>  			    drvdata->base + TPDM_CMB_XPMR(i));
>  	}
>  
> +	set_cmb_tier(drvdata);
> +
>  	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>  	/*
>  	 * Set to 0 for continuous CMB collection mode,
> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device *dev,
>  			      char *buf)
>  {
>  	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
>  
> -	return sysfs_emit(buf, "%u\n",
> -			 (unsigned int)drvdata->dsb->patt_ts);
> +	if (tpdm_has_dsb_dataset(drvdata))
> +		size = sysfs_emit(buf, "%u\n",
> +				 (unsigned int)drvdata->dsb->patt_ts);
> +
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		size = sysfs_emit(buf, "%u\n",
> +				 (unsigned int)drvdata->cmb->patt_ts);
> +
> +	return size;
>  }
>  
>  /*
> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device *dev,
>  		return -EINVAL;
>  
>  	spin_lock(&drvdata->spinlock);
> -	drvdata->dsb->patt_ts = !!val;
> +	if (tpdm_has_dsb_dataset(drvdata))
> +		drvdata->dsb->patt_ts = !!val;
> +
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		drvdata->cmb->patt_ts = !!val;
>  	spin_unlock(&drvdata->spinlock);
> +
>  	return size;
>  }
>  static DEVICE_ATTR_RW(enable_ts);
> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(cmb_mode);
>  
> +static ssize_t cmb_ts_all_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n",
> +			  (unsigned int)drvdata->cmb->ts_all);
> +}
> +
> +static ssize_t cmb_ts_all_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf,
> +				size_t size)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	if (val)
> +		drvdata->cmb->ts_all = true;
> +	else
> +		drvdata->cmb->ts_all = false;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(cmb_ts_all);
> +
> +static ssize_t cmb_trig_ts_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n",
> +			  (unsigned int)drvdata->cmb->trig_ts);
> +}
> +
> +static ssize_t cmb_trig_ts_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf,
> +				 size_t size)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	if (val)
> +		drvdata->cmb->trig_ts = true;
> +	else
> +		drvdata->cmb->trig_ts = false;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(cmb_trig_ts);
> +
>  static struct attribute *tpdm_dsb_edge_attrs[] = {
>  	&dev_attr_ctrl_idx.attr,
>  	&dev_attr_ctrl_val.attr,
> @@ -973,6 +1080,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
>  	CMB_PATT_ATTR(1),
>  	CMB_PATT_MASK_ATTR(0),
>  	CMB_PATT_MASK_ATTR(1),
> +	&dev_attr_enable_ts.attr,
>  	NULL,
>  };
>  
> @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>  
>  static struct attribute *tpdm_cmb_attrs[] = {
>  	&dev_attr_cmb_mode.attr,
> +	&dev_attr_cmb_ts_all.attr,
> +	&dev_attr_cmb_trig_ts.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index e90d008c..65b7ca6 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -11,6 +11,8 @@
>  
>  /* CMB Subunit Registers */
>  #define TPDM_CMB_CR		(0xA00)
> +/*CMB subunit timestamp insertion enable register*/
> +#define TPDM_CMB_TIER		(0xA04)
>  /*CMB subunit timestamp pattern registers*/
>  #define TPDM_CMB_TPR(n)		(0xA08 + (n * 4))
>  /*CMB subunit timestamp pattern mask registers*/
> @@ -24,6 +26,12 @@
>  #define TPDM_CMB_CR_ENA		BIT(0)
>  /* Trace collection mode for CMB subunit */
>  #define TPDM_CMB_CR_MODE	BIT(1)
> +/* Timestamp control for pattern match */
> +#define TPDM_CMB_TIER_PATT_TSENAB	BIT(0)
> +/* CMB CTI timestamp request */
> +#define TPDM_CMB_TIER_XTRIG_TSENAB	BIT(1)
> +/* For timestamp fo all trace */
> +#define TPDM_CMB_TIER_TS_ALL		BIT(2)
>  
>  /*Patten register number*/
>  #define TPDM_CMB_MAX_PATT		2
> @@ -217,6 +225,9 @@ struct dsb_dataset {
>   * @patt_mask:        Save value for pattern mask
>   * @trig_patt:        Save value for trigger pattern
>   * @trig_patt_mask:   Save value for trigger pattern mask
> + * @patt_ts:          Indicates if pattern match for timestamp is enabled.
> + * @trig_ts:          Indicates if CTI trigger for timestamp is enabled.
> + * @ts_all:           Indicates if timestamp is enabled for all packets.
>   */
>  struct cmb_dataset {
>  	u32			trace_mode;
> @@ -224,6 +235,9 @@ struct cmb_dataset {
>  	u32			patt_mask[TPDM_CMB_MAX_PATT];
>  	u32			trig_patt[TPDM_CMB_MAX_PATT];
>  	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
> +	bool			patt_ts;
> +	bool			trig_ts;
> +	bool			ts_all;
>  };
>  
>  /**

Re: [PATCH v2 8/8] coresight-tpdm: Add msr register support for CMB

[James Clark]

On 25/10/2023 03:53, Tao Zhang wrote:
> Add the nodes for CMB subunit MSR(mux select register) support.
> CMB MSRs(mux select registers) is to separate mux,arbitration,
> ,interleaving,data packing control from stream filtering control.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  8 ++
>  drivers/hwtracing/coresight/coresight-tpdm.c       | 86 ++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tpdm.h       | 16 +++-
>  3 files changed, 109 insertions(+), 1 deletion(-)
> 

Reviewed-by: James Clark <james.clark@arm.com>

> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index d58b33ee..df0f837 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -245,3 +245,11 @@ Description:
>  		Accepts only one of the 2 values -  0 or 1.
>  		0 : Disable the timestamp of all trace packets.
>  		1 : Enable the timestamp of all trace packets.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
> +Date:		September 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Set/Get the MSR(mux select register) for the CMB subunit
> +		TPDM.
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index f6cda56..7e331ea 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev,
>  			return -EINVAL;
>  		return sysfs_emit(buf, "0x%x\n",
>  			drvdata->cmb->patt_mask[tpdm_attr->idx]);
> +	case CMB_MSR:
> +		if (tpdm_attr->idx >= drvdata->cmb_msr_num)
> +			return -EINVAL;
> +		return sysfs_emit(buf, "0x%x\n",
> +				drvdata->cmb->msr[tpdm_attr->idx]);
>  	}
>  	return -EINVAL;
>  }
> @@ -162,6 +167,12 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
>  		else
>  			ret = -EINVAL;
>  		break;
> +	case CMB_MSR:
> +		if (tpdm_attr->idx < drvdata->cmb_msr_num)
> +			drvdata->cmb->msr[tpdm_attr->idx] = val;
> +		else
> +			ret = -EINVAL;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -220,6 +231,23 @@ static umode_t tpdm_dsb_msr_is_visible(struct kobject *kobj,
>  	return 0;
>  }
>  
> +static umode_t tpdm_cmb_msr_is_visible(struct kobject *kobj,
> +				       struct attribute *attr, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	struct device_attribute *dev_attr =
> +		container_of(attr, struct device_attribute, attr);
> +	struct tpdm_dataset_attribute *tpdm_attr =
> +		container_of(dev_attr, struct tpdm_dataset_attribute, attr);
> +
> +	if (tpdm_attr->idx < drvdata->cmb_msr_num)
> +		return attr->mode;
> +
> +	return 0;
> +}
> +
>  static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
>  {
>  	if (tpdm_has_dsb_dataset(drvdata)) {
> @@ -361,6 +389,15 @@ static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>  	writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>  }
>  
> +static void set_cmb_msr(struct tpdm_drvdata *drvdata)
> +{
> +	int i;
> +
> +	for (i = 0; i < drvdata->cmb_msr_num; i++)
> +		writel_relaxed(drvdata->cmb->msr[i],
> +			   drvdata->base + TPDM_CMB_MSR(i));
> +}
> +
>  static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>  {
>  	u32 val, i;
> @@ -379,6 +416,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>  
>  	set_cmb_tier(drvdata);
>  
> +	set_cmb_msr(drvdata);
> +
>  	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>  	/*
>  	 * Set to 0 for continuous CMB collection mode,
> @@ -1084,6 +1123,42 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
>  	NULL,
>  };
>  
> +static struct attribute *tpdm_cmb_msr_attrs[] = {
> +	CMB_MSR_ATTR(0),
> +	CMB_MSR_ATTR(1),
> +	CMB_MSR_ATTR(2),
> +	CMB_MSR_ATTR(3),
> +	CMB_MSR_ATTR(4),
> +	CMB_MSR_ATTR(5),
> +	CMB_MSR_ATTR(6),
> +	CMB_MSR_ATTR(7),
> +	CMB_MSR_ATTR(8),
> +	CMB_MSR_ATTR(9),
> +	CMB_MSR_ATTR(10),
> +	CMB_MSR_ATTR(11),
> +	CMB_MSR_ATTR(12),
> +	CMB_MSR_ATTR(13),
> +	CMB_MSR_ATTR(14),
> +	CMB_MSR_ATTR(15),
> +	CMB_MSR_ATTR(16),
> +	CMB_MSR_ATTR(17),
> +	CMB_MSR_ATTR(18),
> +	CMB_MSR_ATTR(19),
> +	CMB_MSR_ATTR(20),
> +	CMB_MSR_ATTR(21),
> +	CMB_MSR_ATTR(22),
> +	CMB_MSR_ATTR(23),
> +	CMB_MSR_ATTR(24),
> +	CMB_MSR_ATTR(25),
> +	CMB_MSR_ATTR(26),
> +	CMB_MSR_ATTR(27),
> +	CMB_MSR_ATTR(28),
> +	CMB_MSR_ATTR(29),
> +	CMB_MSR_ATTR(30),
> +	CMB_MSR_ATTR(31),
> +	NULL,
> +};
> +
>  static struct attribute *tpdm_dsb_attrs[] = {
>  	&dev_attr_dsb_mode.attr,
>  	&dev_attr_dsb_trig_ts.attr,
> @@ -1144,6 +1219,12 @@ static struct attribute_group tpdm_cmb_patt_grp = {
>  	.name = "cmb_patt",
>  };
>  
> +static struct attribute_group tpdm_cmb_msr_grp = {
> +	.attrs = tpdm_cmb_msr_attrs,
> +	.is_visible = tpdm_cmb_msr_is_visible,
> +	.name = "cmb_msr",
> +};
> +
>  static const struct attribute_group *tpdm_attr_grps[] = {
>  	&tpdm_attr_grp,
>  	&tpdm_dsb_attr_grp,
> @@ -1154,6 +1235,7 @@ static const struct attribute_group *tpdm_attr_grps[] = {
>  	&tpdm_cmb_attr_grp,
>  	&tpdm_cmb_trig_patt_grp,
>  	&tpdm_cmb_patt_grp,
> +	&tpdm_cmb_msr_grp,
>  	NULL,
>  };
>  
> @@ -1192,6 +1274,10 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>  		of_property_read_u32(drvdata->dev->of_node,
>  			   "qcom,dsb-msrs-num", &drvdata->dsb_msr_num);
>  
> +	if (drvdata && tpdm_has_cmb_dataset(drvdata))
> +		of_property_read_u32(drvdata->dev->of_node,
> +			   "qcom,cmb-msrs-num", &drvdata->cmb_msr_num);
> +
>  	/* Set up coresight component description */
>  	desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>  	if (!desc.name)
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 65b7ca6..255104d 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -21,6 +21,8 @@
>  #define TPDM_CMB_XPR(n)		(0xA18 + (n * 4))
>  /*CMB subunit trigger pattern mask registers*/
>  #define TPDM_CMB_XPMR(n)	(0xA20 + (n * 4))
> +/* CMB MSR register */
> +#define TPDM_CMB_MSR(n)		(0xA80 + (n * 4))
>  
>  /* Enable bit for CMB subunit */
>  #define TPDM_CMB_CR_ENA		BIT(0)
> @@ -36,6 +38,9 @@
>  /*Patten register number*/
>  #define TPDM_CMB_MAX_PATT		2
>  
> +/* MAX number of DSB MSR */
> +#define TPDM_CMB_MAX_MSR 32
> +
>  /* DSB Subunit Registers */
>  #define TPDM_DSB_CR		(0x780)
>  #define TPDM_DSB_TIER		(0x784)
> @@ -186,6 +191,10 @@
>  		tpdm_simple_dataset_rw(tpmr##nr,		\
>  		CMB_PATT_MASK, nr)
>  
> +#define CMB_MSR_ATTR(nr)					\
> +		tpdm_simple_dataset_rw(msr##nr,			\
> +		CMB_MSR, nr)
> +
>  /**
>   * struct dsb_dataset - specifics associated to dsb dataset
>   * @mode:             DSB programming mode
> @@ -225,6 +234,7 @@ struct dsb_dataset {
>   * @patt_mask:        Save value for pattern mask
>   * @trig_patt:        Save value for trigger pattern
>   * @trig_patt_mask:   Save value for trigger pattern mask
> + * @msr               Save value for MSR
>   * @patt_ts:          Indicates if pattern match for timestamp is enabled.
>   * @trig_ts:          Indicates if CTI trigger for timestamp is enabled.
>   * @ts_all:           Indicates if timestamp is enabled for all packets.
> @@ -235,6 +245,7 @@ struct cmb_dataset {
>  	u32			patt_mask[TPDM_CMB_MAX_PATT];
>  	u32			trig_patt[TPDM_CMB_MAX_PATT];
>  	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
> +	u32			msr[TPDM_CMB_MAX_MSR];
>  	bool			patt_ts;
>  	bool			trig_ts;
>  	bool			ts_all;
> @@ -251,6 +262,7 @@ struct cmb_dataset {
>   * @dsb         Specifics associated to TPDM DSB.
>   * @cmb         Specifics associated to TPDM CMB.
>   * @dsb_msr_num Number of MSR supported by DSB TPDM
> + * @cmb_msr_num Number of MSR supported by CMB TPDM
>   */
>  
>  struct tpdm_drvdata {
> @@ -263,6 +275,7 @@ struct tpdm_drvdata {
>  	struct dsb_dataset	*dsb;
>  	struct cmb_dataset	*cmb;
>  	u32			dsb_msr_num;
> +	u32			cmb_msr_num;
>  };
>  
>  /* Enumerate members of various datasets */
> @@ -277,7 +290,8 @@ enum dataset_mem {
>  	CMB_TRIG_PATT,
>  	CMB_TRIG_PATT_MASK,
>  	CMB_PATT,
> -	CMB_PATT_MASK
> +	CMB_PATT_MASK,
> +	CMB_MSR
>  };
>  
>  /**

Re: [PATCH v2 1/8] dt-bindings: arm: Add support for CMB element size

[Tao Zhang]

On 10/27/2023 5:25 AM, Rob Herring wrote:
> On Wed, Oct 25, 2023 at 10:53:21AM +0800, Tao Zhang wrote:
>> Add property "qcom,cmb-elem-size" to support CMB(Continuous
>> Multi-Bit) element for TPDM. The associated aggregator will read
>> this size before it is enabled. CMB element size currently only
>> supports 32-bit and 64-bit.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   .../bindings/arm/qcom,coresight-tpdm.yaml          | 27 ++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> index 61ddc3b..f9a2025 100644
>> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> @@ -52,6 +52,14 @@ properties:
>>       $ref: /schemas/types.yaml#/definitions/uint8
>>       enum: [32, 64]
>>   
>> +  qcom,cmb-element-size:
> What are the units? Use '-bits' suffix.

Yes, its unit should be bit.

Do you mean that you prefer to use "qcom, cmb-element-size-bits"?

Do I also need to replace "qcom, dsb-element-size" with "qcom, 
dsb-element-size-bits".

>
>> +    description:
>> +      Specifies the CMB(Continuous Multi-Bit) element size supported by
>> +      the monitor. The associated aggregator will read this size before it
>> +      is enabled. CMB element size currently only supports 32-bit and 64-bit.
> The enum says 8-bit is supported.

Yes, 8-bit is supported. I will update the description in the next patch 
series.


Best,

Tao

>
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> +    enum: [8, 32, 64]
>> +
>>     qcom,dsb-msrs-num:
>>       description:
>>         Specifies the number of DSB(Discrete Single Bit) MSR(mux select register)
>> @@ -110,4 +118,23 @@ examples:
>>         };
>>       };
>>   
>> +    tpdm@6c29000 {
>> +      compatible = "qcom,coresight-tpdm", "arm,primecell";
>> +      reg = <0x06c29000 0x1000>;
>> +      reg-names = "tpdm-base";
>> +
>> +      qcom,cmb-element-size = /bits/ 8 <64>;
>> +
>> +      clocks = <&aoss_qmp>;
>> +      clock-names = "apb_pclk";
>> +
>> +      out-ports {
>> +        port {
>> +          tpdm_ipcc_out_funnel_center: endpoint {
>> +            remote-endpoint =
>> +              <&funnel_center_in_tpdm_ipcc>;
>> +          };
>> +        };
>> +      };
>> +    };
>>   ...
>> -- 
>> 2.7.4
>>

Re: [PATCH v2 7/8] dt-bindings: arm: Add support for TPDM CMB MSR register

[Tao Zhang]

On 10/27/2023 5:27 AM, Rob Herring wrote:
> On Wed, Oct 25, 2023 at 10:53:27AM +0800, Tao Zhang wrote:
>> Add property "qcom,cmb_msr_num" to support CMB MSR(mux select register)
>> for TPDM. It specifies the number of CMB MSR registers supported by
>> the TDPM.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> index f9a2025..a586b80a 100644
>> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> @@ -69,6 +69,15 @@ properties:
>>       minimum: 0
>>       maximum: 32
>>   
>> +  qcom,cmb-msrs-num:
>> +    description:
>> +      Specifies the number of CMB MSR(mux select register) registers supported
>> +      by the monitor. If this property is not configured or set to 0, it means
>> +      this TPDM doesn't support CMB MSR.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 32
> default: 0

If the TPDM doesn't support CMB MSR, we will not configure this 
property. Set to 0 to indicate

that CMB MSR is not supported and is only an optional method.

Is it necessary to add this "default" value here?


Best,

Tao

>> +
>>     clocks:
>>       maxItems: 1
>>   
>> @@ -124,6 +133,7 @@ examples:
>>         reg-names = "tpdm-base";
>>   
>>         qcom,cmb-element-size = /bits/ 8 <64>;
>> +      qcom,cmb-msrs-num = <32>;
>>   
>>         clocks = <&aoss_qmp>;
>>         clock-names = "apb_pclk";
>> -- 
>> 2.7.4
>>

Re: [PATCH v2 4/8] coresight-tpdm: Add support to configure CMB

[Tao Zhang]

On 10/30/2023 7:29 PM, James Clark wrote:
>
> On 25/10/2023 03:53, Tao Zhang wrote:
>> TPDM CMB subunits support two forms of CMB data set element creation:
>> continuous and trace-on-change collection mode. Continuous change
>> creates CMB data set elements on every CMBCLK edge. Trace-on-change
>> creates CMB data set elements only when a new data set element differs
>> in value from the previous element in a CMB data set. Set CMB_CR.MODE
>> to 0 for continuous CMB collection mode. Set CMB_CR.MODE to 1 for
>> trace-on-change CMB collection mode
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 10 +++
>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 71 ++++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h       | 12 ++++
>>   3 files changed, 93 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index f07218e..ace7231 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -170,3 +170,13 @@ Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_t
>>   Description:
>>   		(RW) Set/Get the MSR(mux select register) for the DSB subunit
>>   		TPDM.
>> +
>> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_mode
>> +Date:		March 2023
>> +KernelVersion	6.7
>> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>> +Description:	(Write) Set the data collection mode of CMB tpdm.
> I know it's expanded elsewhere, but it's probably worth expanding the
> CMB abbreviation here as well so people reading the docs don't have to
> go into the code.

Sure, I will update in the next patch series.


Best,

Tao

>
> Otherwise:
>
> Reviewed-by: James Clark <james.clark@arm.com>
>
>> +
>> +		Accepts only one of the 2 values -  0 or 1.
>> +		0 : Continuous CMB collection mode.
>> +		1 : Trace-on-change CMB collection mode.
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index c8bb388..efb376e 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -148,6 +148,18 @@ static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
>>   	return 0;
>>   }
> [...]

Re: [PATCH v2 2/8] coresight-tpda: Add support to configure CMB element

[James Clark]

On 01/11/2023 08:53, Tao Zhang wrote:
> 
> On 10/30/2023 7:11 PM, James Clark wrote:
>>
>> On 25/10/2023 03:53, Tao Zhang wrote:
>>> Read the CMB element size from the device tree. Set the register
>>> bit that controls the CMB element size of the corresponding port.
>>>
>>> Signed-off-by: Tao Zhang<quic_taozha@quicinc.com>
>>> Signed-off-by: Mao Jinlong<quic_jinlmao@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-tpda.c | 108
>>> ++++++++++++++++-----------
>>>   drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
>>>   2 files changed, 69 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>> index 5f82737..3101d2a 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct
>>> coresight_device *csdev)
>>>               CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>>   }
>>>   +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +
>>> +    if (drvdata->dsb_esize)
>>> +        drvdata->dsb_esize = 0;
>>> +    if (drvdata->cmb_esize)
>>> +        drvdata->cmb_esize = 0;

The ifs don't do anything here, it's just the same as always setting to 0.

>>> +}
>>> +
>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32
>>> *val)
>>> +{
>>> +
>>> +    if (drvdata->dsb_esize == 64)
>>> +        *val |= TPDA_Pn_CR_DSBSIZE;
>>> +    else if (drvdata->dsb_esize == 32)
>>> +        *val &= ~TPDA_Pn_CR_DSBSIZE;
>>> +
>>> +    if (drvdata->cmb_esize == 64)
>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>> +    else if (drvdata->cmb_esize == 32)
>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>> +    else if (drvdata->cmb_esize == 8)
>>> +        *val &= ~TPDA_Pn_CR_CMBSIZE;
>>> +}
>>> +
>>>   /*
>>>    * Read the DSB element size from the TPDM device
>>>    * Returns
>>>    *    The dsb element size read from the devicetree if available.
>> Hi Tao,
>>
>> I think the function description and the return value description above
>> need updating now.
> I will update this in the next patch series.
>>
>>>    *    0 - Otherwise, with a warning once.
>>>    */
>>> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>>> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
>>> +                  struct coresight_device *csdev)
>>>   {
>>> -    int rc = 0;
>>> -    u8 size = 0;
>>> -
>>> -    rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>> -            "qcom,dsb-element-size", &size);
>>> +    int rc = -EEXIST;
>>> +
>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>> +            "qcom,dsb-element-size", &drvdata->dsb_esize))
>>> +        rc &= 0;
>> Is &= 0 significant? Why not = 0?
> I will update this in the next patch series.
>>
>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>> +            "qcom,cmb-element-size", &drvdata->cmb_esize))
>>> +        rc &= 0;
>>>       if (rc)
>>>           dev_warn_once(&csdev->dev,
>>> -            "Failed to read TPDM DSB Element size: %d\n", rc);
>>> +            "Failed to read TPDM Element size: %d\n", rc);
>>>   -    return size;
>>> +    return rc;
>>>   }
>>>     /*
>>> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct
>>> coresight_device *csdev)
>>>    * Parameter "inport" is used to pass in the input port number
>>>    * of TPDA, and it is set to -1 in the recursize call.
>>>    */
>>> -static int tpda_get_element_size(struct coresight_device *csdev,
>>> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
>>> +                 struct coresight_device *csdev,
>>>                    int inport)
>>>   {
>>> -    int dsb_size = -ENOENT;
>>> -    int i, size;
>>> +    int rc = -ENOENT;
>>> +    int i;
>>>       struct coresight_device *in;
>>>   +    if (inport > 0)
>>> +        tpdm_clear_element_size(csdev);
>>> +
>>>       for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>           in = csdev->pdata->in_conns[i]->src_dev;
>>>           if (!in)
>>> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct
>>> coresight_device *csdev,
>>>               continue;
>>>             if (coresight_device_is_tpdm(in)) {
>>> -            size = tpdm_read_dsb_element_size(in);
>>> +            if (rc)
>>> +                rc = tpdm_read_element_size(drvdata, in);
>>> +            else
>>> +                return -EINVAL;
>> This is quite hard to follow, is checking rc here before calling
>> tpdm_read_element_size() some kind of way of only calling it once?
> 
> Yes, there can only be one TPDM on one input port of TPDA. If
> "tpdm_read_element_size" is called
> 
> twice, it means that two TPDMs are found on one input port of TPDA.
> 
>> rc isn't actually a return value at this point, it's just default
>> initialised to -ENOENT.
> 
> In this loop, "tpdm_read_element_size" will be called for the first time
> TPDM found. If rc equals to
> 
> 0, it means that at lease one TPDM has been found on the input port.
> Does it make sense?
> 
>> Then it's not clear why the else condition returns an error?
> The same as the above.
>>
>>>           } else {
>>>               /* Recurse down the path */
>>> -            size = tpda_get_element_size(in, -1);
>>> -        }
>>> -
>>> -        if (size < 0)
>>> -            return size;
>>> -
>>> -        if (dsb_size < 0) {
>>> -            /* Found a size, save it. */
>>> -            dsb_size = size;
>>> -        } else {
>>> -            /* Found duplicate TPDMs */
>>> -            return -EEXIST;
>>> +            rc = tpda_get_element_size(drvdata, in, -1);
>> And then why it's assigned here but not checked for an error in this
>> case?
> 
> |Remote ETM|                           |TPDM|
> 
>             |    branch0                           | branch1
> 
>                         --------------------------
> 
>                                     funnel1
> 
>                         ---------------------------
> 
>                                           | input port 1
> 
> -----------------------------
> 
>                                                        TPDA1
> 
> -----------------------------
> 
> The  branches on some TPDA's input ports may not have TPDM. For example,
> branch 0 doesn't has a
> 
> TPDM connected, tpda_get_element_size will not return 0 here. This is a
> normal situation. We need to
> 
> continue to find TPDM on branch1 through recursion.
> 
>>
>> Maybe some comments explaining the flow would help. Or to me it seems
>> like a second variable to track the thing that's actually intended could
>> be used as well. Like "bool found_element" or something, rather than
>> re-using rc to also track another thing.
> 
> Do you prefer to use "static bool found_element" to mark if we already
> have read an element size in
> 
> the recursion function?
> 

You could add a static, or you could use whether a set dsb or cmb size
exists to mark that one was found, which I think is already partially done.

My confusion comes from the fact that instead of just a recursive
search, the function doesn't stop at the first found one, it continues
to sanity check that there is only one connected across all ports.

And instead of just checking the error condition at the very end, it
does it mid recursion, relying on the rc value from a previous
iteration. IMO the following is a simplification because it always
returns at the first error found, and checks the final condition outside
of the recursive function. But you could probably leave it as you have
but with some comments explaining why:


diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 3101d2a0671d..00b7607d36be 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -90,13 +90,10 @@ static int tpda_get_element_size(struct tpda_drvdata *drvdata,
                                  struct coresight_device *csdev,
                                  int inport)
  {
-       int rc = -ENOENT;
+       int rc = 0;
         int i;
         struct coresight_device *in;
  
-       if (inport > 0)
-               tpdm_clear_element_size(csdev);
-
         for (i = 0; i < csdev->pdata->nr_inconns; i++) {
                 in = csdev->pdata->in_conns[i]->src_dev;
                 if (!in)
@@ -108,19 +105,23 @@ static int tpda_get_element_size(struct tpda_drvdata *drvdata,
                         continue;
  
                 if (coresight_device_is_tpdm(in)) {
-                       if (rc)
-                               rc = tpdm_read_element_size(drvdata, in);
-                       else
+                       if ((drvdata->dsb_esize) || (drvdata->cmb_esize)) {
+                               /* Error if already previously found and initialised one */
+                               dev_warn_once(&drvdata->csdev->dev,
+                                       "Detected multiple TPDMs on port %d", -EEXIST);
                                 return -EINVAL;
+                       }
+                       rc = tpdm_read_element_size(drvdata, in);
+                       if (rc)
+                               return rc;
                 } else {
                         /* Recurse down the path */
                         rc = tpda_get_element_size(drvdata, in, -1);
+                       if (rc)
+                               return rc;
                 }
         }
  
-       if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
-               rc = 0;
-
         return rc;
  }
  
@@ -146,15 +147,17 @@ static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
          * Set the bit to 0 if the size is 32
          * Set the bit to 1 if the size is 64
          */
+       tpdm_clear_element_size(drvdata->csdev);
         rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
-       if (!rc) {
+       if (!rc && ((drvdata->dsb_esize) || (drvdata->cmb_esize)))
+       {
                 tpda_set_element_size(drvdata, &val);
                 /* Enable the port */
                 val |= TPDA_Pn_CR_ENA;
                 writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
-       } else if (rc == -EINVAL) {
-               dev_warn_once(&drvdata->csdev->dev,
-                       "Detected multiple TPDMs on port %d", -EEXIST);
+       } else {
+               dev_warn_once(&drvdata->csdev->dev, "Didn't find TPDM elem size");
+               rc = -EINVAL;
         }
  
         return rc;

Re: [PATCH v2 2/8] coresight-tpda: Add support to configure CMB element

[Tao Zhang]

On 11/1/2023 7:36 PM, James Clark wrote:
>
>
> On 01/11/2023 08:53, Tao Zhang wrote:
>>
>> On 10/30/2023 7:11 PM, James Clark wrote:
>>>
>>> On 25/10/2023 03:53, Tao Zhang wrote:
>>>> Read the CMB element size from the device tree. Set the register
>>>> bit that controls the CMB element size of the corresponding port.
>>>>
>>>> Signed-off-by: Tao Zhang<quic_taozha@quicinc.com>
>>>> Signed-off-by: Mao Jinlong<quic_jinlmao@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 108
>>>> ++++++++++++++++-----------
>>>>   drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
>>>>   2 files changed, 69 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> index 5f82737..3101d2a 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct
>>>> coresight_device *csdev)
>>>>               CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>>>   }
>>>>   +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>>> +{
>>>> +    struct tpda_drvdata *drvdata = 
>>>> dev_get_drvdata(csdev->dev.parent);
>>>> +
>>>> +    if (drvdata->dsb_esize)
>>>> +        drvdata->dsb_esize = 0;
>>>> +    if (drvdata->cmb_esize)
>>>> +        drvdata->cmb_esize = 0;
>
> The ifs don't do anything here, it's just the same as always setting 
> to 0.
I will update in the next patch series.
>
>>>> +}
>>>> +
>>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32
>>>> *val)
>>>> +{
>>>> +
>>>> +    if (drvdata->dsb_esize == 64)
>>>> +        *val |= TPDA_Pn_CR_DSBSIZE;
>>>> +    else if (drvdata->dsb_esize == 32)
>>>> +        *val &= ~TPDA_Pn_CR_DSBSIZE;
>>>> +
>>>> +    if (drvdata->cmb_esize == 64)
>>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>>> +    else if (drvdata->cmb_esize == 32)
>>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>>> +    else if (drvdata->cmb_esize == 8)
>>>> +        *val &= ~TPDA_Pn_CR_CMBSIZE;
>>>> +}
>>>> +
>>>>   /*
>>>>    * Read the DSB element size from the TPDM device
>>>>    * Returns
>>>>    *    The dsb element size read from the devicetree if available.
>>> Hi Tao,
>>>
>>> I think the function description and the return value description above
>>> need updating now.
>> I will update this in the next patch series.
>>>
>>>>    *    0 - Otherwise, with a warning once.
>>>>    */
>>>> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>>>> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
>>>> +                  struct coresight_device *csdev)
>>>>   {
>>>> -    int rc = 0;
>>>> -    u8 size = 0;
>>>> -
>>>> -    rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> -            "qcom,dsb-element-size", &size);
>>>> +    int rc = -EEXIST;
>>>> +
>>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> +            "qcom,dsb-element-size", &drvdata->dsb_esize))
>>>> +        rc &= 0;
>>> Is &= 0 significant? Why not = 0?
>> I will update this in the next patch series.
>>>
>>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> +            "qcom,cmb-element-size", &drvdata->cmb_esize))
>>>> +        rc &= 0;
>>>>       if (rc)
>>>>           dev_warn_once(&csdev->dev,
>>>> -            "Failed to read TPDM DSB Element size: %d\n", rc);
>>>> +            "Failed to read TPDM Element size: %d\n", rc);
>>>>   -    return size;
>>>> +    return rc;
>>>>   }
>>>>     /*
>>>> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct
>>>> coresight_device *csdev)
>>>>    * Parameter "inport" is used to pass in the input port number
>>>>    * of TPDA, and it is set to -1 in the recursize call.
>>>>    */
>>>> -static int tpda_get_element_size(struct coresight_device *csdev,
>>>> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
>>>> +                 struct coresight_device *csdev,
>>>>                    int inport)
>>>>   {
>>>> -    int dsb_size = -ENOENT;
>>>> -    int i, size;
>>>> +    int rc = -ENOENT;
>>>> +    int i;
>>>>       struct coresight_device *in;
>>>>   +    if (inport > 0)
>>>> +        tpdm_clear_element_size(csdev);
>>>> +
>>>>       for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>>           in = csdev->pdata->in_conns[i]->src_dev;
>>>>           if (!in)
>>>> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct
>>>> coresight_device *csdev,
>>>>               continue;
>>>>             if (coresight_device_is_tpdm(in)) {
>>>> -            size = tpdm_read_dsb_element_size(in);
>>>> +            if (rc)
>>>> +                rc = tpdm_read_element_size(drvdata, in);
>>>> +            else
>>>> +                return -EINVAL;
>>> This is quite hard to follow, is checking rc here before calling
>>> tpdm_read_element_size() some kind of way of only calling it once?
>>
>> Yes, there can only be one TPDM on one input port of TPDA. If
>> "tpdm_read_element_size" is called
>>
>> twice, it means that two TPDMs are found on one input port of TPDA.
>>
>>> rc isn't actually a return value at this point, it's just default
>>> initialised to -ENOENT.
>>
>> In this loop, "tpdm_read_element_size" will be called for the first time
>> TPDM found. If rc equals to
>>
>> 0, it means that at lease one TPDM has been found on the input port.
>> Does it make sense?
>>
>>> Then it's not clear why the else condition returns an error?
>> The same as the above.
>>>
>>>>           } else {
>>>>               /* Recurse down the path */
>>>> -            size = tpda_get_element_size(in, -1);
>>>> -        }
>>>> -
>>>> -        if (size < 0)
>>>> -            return size;
>>>> -
>>>> -        if (dsb_size < 0) {
>>>> -            /* Found a size, save it. */
>>>> -            dsb_size = size;
>>>> -        } else {
>>>> -            /* Found duplicate TPDMs */
>>>> -            return -EEXIST;
>>>> +            rc = tpda_get_element_size(drvdata, in, -1);
>>> And then why it's assigned here but not checked for an error in this
>>> case?
>>
>> |Remote ETM|                           |TPDM|
>>
>>             |    branch0                           | branch1
>>
>>                         --------------------------
>>
>>                                     funnel1
>>
>>                         ---------------------------
>>
>>                                           | input port 1
>>
>> -----------------------------
>>
>>                                                        TPDA1
>>
>> -----------------------------
>>
>> The  branches on some TPDA's input ports may not have TPDM. For example,
>> branch 0 doesn't has a
>>
>> TPDM connected, tpda_get_element_size will not return 0 here. This is a
>> normal situation. We need to
>>
>> continue to find TPDM on branch1 through recursion.
>>
>>>
>>> Maybe some comments explaining the flow would help. Or to me it seems
>>> like a second variable to track the thing that's actually intended 
>>> could
>>> be used as well. Like "bool found_element" or something, rather than
>>> re-using rc to also track another thing.
>>
>> Do you prefer to use "static bool found_element" to mark if we already
>> have read an element size in
>>
>> the recursion function?
>>
>
> You could add a static, or you could use whether a set dsb or cmb size
> exists to mark that one was found, which I think is already partially 
> done.
>
> My confusion comes from the fact that instead of just a recursive
> search, the function doesn't stop at the first found one, it continues
> to sanity check that there is only one connected across all ports.
>
> And instead of just checking the error condition at the very end, it
> does it mid recursion, relying on the rc value from a previous
> iteration. IMO the following is a simplification because it always
> returns at the first error found, and checks the final condition outside
> of the recursive function. But you could probably leave it as you have
> but with some comments explaining why:
>
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
> b/drivers/hwtracing/coresight/coresight-tpda.c
> index 3101d2a0671d..00b7607d36be 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -90,13 +90,10 @@ static int tpda_get_element_size(struct 
> tpda_drvdata *drvdata,
>                                  struct coresight_device *csdev,
>                                  int inport)
>  {
> -       int rc = -ENOENT;
> +       int rc = 0;
>         int i;
>         struct coresight_device *in;
>
> -       if (inport > 0)
> -               tpdm_clear_element_size(csdev);
> -
>         for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>                 in = csdev->pdata->in_conns[i]->src_dev;
>                 if (!in)
> @@ -108,19 +105,23 @@ static int tpda_get_element_size(struct 
> tpda_drvdata *drvdata,
>                         continue;
>
>                 if (coresight_device_is_tpdm(in)) {
> -                       if (rc)
> -                               rc = tpdm_read_element_size(drvdata, in);
> -                       else
> +                       if ((drvdata->dsb_esize) || 
> (drvdata->cmb_esize)) {
> +                               /* Error if already previously found 
> and initialised one */
> + dev_warn_once(&drvdata->csdev->dev,
> +                                       "Detected multiple TPDMs on 
> port %d", -EEXIST);
>                                 return -EINVAL;
> +                       }
> +                       rc = tpdm_read_element_size(drvdata, in);
> +                       if (rc)
> +                               return rc;
>                 } else {
>                         /* Recurse down the path */
>                         rc = tpda_get_element_size(drvdata, in, -1);
> +                       if (rc)
> +                               return rc;
>                 }
>         }
>
> -       if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
> -               rc = 0;
> -
>         return rc;
>  }
>
> @@ -146,15 +147,17 @@ static int tpda_enable_port(struct tpda_drvdata 
> *drvdata, int port)
>          * Set the bit to 0 if the size is 32
>          * Set the bit to 1 if the size is 64
>          */
> +       tpdm_clear_element_size(drvdata->csdev);
>         rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
> -       if (!rc) {
> +       if (!rc && ((drvdata->dsb_esize) || (drvdata->cmb_esize)))
> +       {
>                 tpda_set_element_size(drvdata, &val);
>                 /* Enable the port */
>                 val |= TPDA_Pn_CR_ENA;
>                 writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> -       } else if (rc == -EINVAL) {
> -               dev_warn_once(&drvdata->csdev->dev,
> -                       "Detected multiple TPDMs on port %d", -EEXIST);
> +       } else {
> +               dev_warn_once(&drvdata->csdev->dev, "Didn't find TPDM 
> elem size");
> +               rc = -EINVAL;
>         }
>
>         return rc;

Make sense. I will refine and update the code in the next patch series.


Best,

Tao

>
>

Re: [PATCH v2 1/8] dt-bindings: arm: Add support for CMB element size

[Tao Zhang]

On 11/1/2023 2:29 PM, Tao Zhang wrote:
>
> On 10/27/2023 5:25 AM, Rob Herring wrote:
>> On Wed, Oct 25, 2023 at 10:53:21AM +0800, Tao Zhang wrote:
>>> Add property "qcom,cmb-elem-size" to support CMB(Continuous
>>> Multi-Bit) element for TPDM. The associated aggregator will read
>>> this size before it is enabled. CMB element size currently only
>>> supports 32-bit and 64-bit.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>> ---
>>>   .../bindings/arm/qcom,coresight-tpdm.yaml          | 27 
>>> ++++++++++++++++++++++
>>>   1 file changed, 27 insertions(+)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml 
>>> b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>>> index 61ddc3b..f9a2025 100644
>>> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>>> @@ -52,6 +52,14 @@ properties:
>>>       $ref: /schemas/types.yaml#/definitions/uint8
>>>       enum: [32, 64]
>>>   +  qcom,cmb-element-size:
>> What are the units? Use '-bits' suffix.
>
> Yes, its unit should be bit.
>
> Do you mean that you prefer to use "qcom, cmb-element-size-bits"?
>
> Do I also need to replace "qcom, dsb-element-size" with "qcom, 
> dsb-element-size-bits".

I will continue to use the property name "qcom, cmb-element-size" in 
order to be consistent with "qcom, dsb-element-size".

Let me know if you have any concerns about this.


Best,

Tao

>
>>
>>> +    description:
>>> +      Specifies the CMB(Continuous Multi-Bit) element size 
>>> supported by
>>> +      the monitor. The associated aggregator will read this size 
>>> before it
>>> +      is enabled. CMB element size currently only supports 32-bit 
>>> and 64-bit.
>> The enum says 8-bit is supported.
>
> Yes, 8-bit is supported. I will update the description in the next 
> patch series.
>
>
> Best,
>
> Tao
>
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>> +    enum: [8, 32, 64]
>>> +
>>>     qcom,dsb-msrs-num:
>>>       description:
>>>         Specifies the number of DSB(Discrete Single Bit) MSR(mux 
>>> select register)
>>> @@ -110,4 +118,23 @@ examples:
>>>         };
>>>       };
>>>   +    tpdm@6c29000 {
>>> +      compatible = "qcom,coresight-tpdm", "arm,primecell";
>>> +      reg = <0x06c29000 0x1000>;
>>> +      reg-names = "tpdm-base";
>>> +
>>> +      qcom,cmb-element-size = /bits/ 8 <64>;
>>> +
>>> +      clocks = <&aoss_qmp>;
>>> +      clock-names = "apb_pclk";
>>> +
>>> +      out-ports {
>>> +        port {
>>> +          tpdm_ipcc_out_funnel_center: endpoint {
>>> +            remote-endpoint =
>>> +              <&funnel_center_in_tpdm_ipcc>;
>>> +          };
>>> +        };
>>> +      };
>>> +    };
>>>   ...
>>> -- 
>>> 2.7.4
>>>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org