[PATCH v3 0/11] Add support to configure TPDM DSB subunit

[PATCH v3 0/11] Add support to configure TPDM DSB subunit

[Tao Zhang]

Introduction of TPDM DSB subunit
DSB 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 DSB subunit must be configured prior to enablement. This series
adds support for TPDM to configure the configure DSB 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 DSB subunit.
e.g.
/sys/devices/platform/soc@0/69d0000.tpdm/tpdm0#ls -l | grep dsb
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_edge_ctrl
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_edge_ctrl_mask
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_mode
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_patt_mask
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_patt_ts
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_patt_type
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_patt_val
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_trig_patt_mask
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_trig_patt_val
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_trig_ts
-rw-r--r--    1 root     root      4096 Jan  1 00:01 dsb_trig_type

We can use the commands are similar to the below to configure the
TPDMs which support DSB subunit. Enable coresight sink first.
echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink
echo 1 > /sys/bus/coresight/devices/tpdm0/reset
echo 0x3 0x3 0x1 > /sys/bus/coresight/devices/tpdm0/dsb_edge_ctrl_mask
echo 0x6d 0x6d 0 > /sys/bus/coresight/devices/tpdm0/dsb_edge_ctrl
echo 1 > /sys/bus/coresight/devices/tpdm0/dsb_patt_ts
echo 1 > /sys/bus/coresight/devices/tpdm0/dsb_patt_type
echo 0 > /sys/bus/coresight/devices/tpdm0/dsb_trig_ts
echo 0 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/dsb_patt_mask
echo 0 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/dsb_trig_patt_val

This patch series depends on patch series "[PATCH v2 0/9] coresight:
Fix CTI module refcount leak by making it a helper device"
https://patchwork.kernel.org/project/linux-arm-kernel/cover/20230310160610.742382-1-james.clark@arm.com/

TPDM_DSB commit tree:
https://git.codelinaro.org/clo/linux-kernel/coresight/-/tree/tpdm-dsb-v3
https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/tpdm-dsb-v3

Changes in V3:
1. Move the property "qcom,dsb-element-size" to TPDM
devicetree and update the TPDM yaml file for this item.
-- Suzuki K Poulose
2. Add the error message when the DSB element size is not set to
32-bit or 64-bit. -- Suzuki K Poulose
3. Add more information to the comments of patch #3
-- Suzuki K Poulose
4. Combine the value updates to the TPDM_DSB_CR for TPDM.
-- Suzuki K Poulose
5. Remove the function "tpdm_datasets_alloc", and fold its code
to a new function "tpdm_init_datasets". It will complete the
initialization of TPDM.  -- Suzuki K Poulose
6. Change the method of qualifying input values.
-- Suzuki K Poulose
7. Add the documentation of the new sysfs handles.
-- Suzuki K Poulose
8. Provide the separate handles for the "mode bits".
-- Suzuki K Poulose

Changes in V2:
1. Change the name of the property "qcom,dsb-elem-size" to
"qcom,dsb-element-size" -- Suzuki K Poulose
2. Update the TPDA yaml file for the item "qcom,dsb-elem-size".
-- Krzysztof Kozlowski
3. Add the full name of DSB in the description of the item
"qcom,dsb-elem-size". -- Rob Herring

Changes in V1:
1. Change the definition of the property "qcom,dsb-elem-size" from
"uint32-array" to "uint32-matrix". -- Krzysztof Kozlowski
2. Add the full name of DSB. -- Rob Herring
3. Deal with 2 entries in an iteration in TPDA driver. -- Suzuki K Poulose
4. Divide the function "tpdm_datasets_alloc" into two functions,
"tpdm_datasets_setup" and "tpdm_datasets_alloc".
5. Detecte the input string with the conventional semantics automatically,
and constrain the size of the input value. -- Suzuki K Poulose
6. Use the hook function "is_visible()" to hide the DSB related knobs if
the data sets are missing. -- Suzuki K Poulose
7. Use the macros "FIELD_GET" and "FIELD_PREP" to set the values.
-- Suzuki K Poulose
8. Update the definition of the macros in TPDM driver.
9. Update the comments of the values for the nodes which are for DSB
element creation and onfigure pattern match output. -- Suzuki K Poulose
10. Use API "sysfs_emit" to "replace scnprintf". -- Suzuki K Poulose

Tao Zhang (10):
  dt-bindings: arm: Add support for DSB element size
  coresight-tpda: Add DSB dataset support
  coresight-tpdm: Initialize DSB subunit configuration
  coresight-tpdm: Add reset node to TPDM node
  coresight-tpdm: Add nodes to set trigger timestamp and type
  coresight-tpdm: Add node to set dsb programming mode
  coresight-tpdm: Add nodes for dsb edge control
  coresight-tpdm: Add nodes to configure pattern match output
  coresight-tpdm: Add nodes for timestamp request
  dt-bindings: arm: Add support for DSB MSR register
  coresight-tpdm: Add nodes for dsb msr support

 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 144 +++++
 .../bindings/arm/qcom,coresight-tpdm.yaml          |  21 +
 drivers/hwtracing/coresight/coresight-tpda.c       |  58 ++
 drivers/hwtracing/coresight/coresight-tpda.h       |   4 +
 drivers/hwtracing/coresight/coresight-tpdm.c       | 694 ++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tpdm.h       |  69 ++
 6 files changed, 984 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v3 01/11] dt-bindings: arm: Add support for DSB element size

[Tao Zhang]

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

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

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
index 5c08342..d9b6b613 100644
--- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
@@ -44,6 +44,15 @@ properties:
     minItems: 1
     maxItems: 2
 
+  qcom,dsb-element-size:
+    description:
+      Specifies the DSB(Discrete Single Bit) element size supported by
+      the monitor. The associated aggregator will read this size before it
+      is enabled. DSB element size currently only supports 32-bit and 64-bit.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 32
+    maximum: 64
+
   clocks:
     maxItems: 1
 
@@ -77,6 +86,8 @@ examples:
       compatible = "qcom,coresight-tpdm", "arm,primecell";
       reg = <0x0684c000 0x1000>;
 
+      qcom,dsb-element-size = <32>;
+
       clocks = <&aoss_qmp>;
       clock-names = "apb_pclk";
 
-- 
2.7.4

[PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Tao Zhang]

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

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpda.c | 58 ++++++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
 2 files changed, 62 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index f712e11..8dcfc4a 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -21,6 +21,47 @@
 
 DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
 
+/* Search and read element data size from the TPDM node in
+ * the devicetree. Each input port of TPDA is connected to
+ * a TPDM. Different TPDM supports different types of dataset,
+ * and some may support more than one type of dataset.
+ * Parameter "inport" is used to pass in the input port number
+ * of TPDA, and it is set to 0 in the recursize call.
+ * Parameter "parent" is used to pass in the original call.
+ */
+static int tpda_set_element_size(struct tpda_drvdata *drvdata,
+			   struct coresight_device *csdev, int inport, bool parent)
+{
+	static int nr_inport;
+	int i;
+	struct coresight_device *in_csdev;
+
+	if (inport > (TPDA_MAX_INPORTS - 1))
+		return -EINVAL;
+
+	if (parent)
+		nr_inport = inport;
+
+	for (i = 0; i < csdev->pdata->nr_inconns; i++) {
+		in_csdev = csdev->pdata->in_conns[i].remote_dev;
+		if (!in_csdev)
+			break;
+
+		if (parent)
+			if (csdev->pdata->in_conns[i].port != inport)
+				continue;
+
+		if (in_csdev && strstr(dev_name(&in_csdev->dev), "tpdm")) {
+			of_property_read_u32(in_csdev->dev.parent->of_node,
+					"qcom,dsb-element-size", &drvdata->dsb_esize[nr_inport]);
+			break;
+		}
+		tpda_set_element_size(drvdata, in_csdev, 0, false);
+	}
+
+	return 0;
+}
+
 /* Settings pre enabling port control register */
 static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
 {
@@ -37,6 +78,18 @@ static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
 	u32 val;
 
 	val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
+	/*
+	 * Configure aggregator port n DSB data set element size
+	 * Set the bit to 0 if the size is 32
+	 * Set the bit to 1 if the size is 64
+	 */
+	if (drvdata->dsb_esize[port] == 32)
+		val &= ~TPDA_Pn_CR_DSBSIZE;
+	else if (drvdata->dsb_esize[port] == 64)
+		val |= TPDA_Pn_CR_DSBSIZE;
+	else
+		dev_err(drvdata->dev,
+			"DSB data size input from port[%d] is invalid\n", port);
 	/* Enable the port */
 	val |= TPDA_Pn_CR_ENA;
 	writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
@@ -57,6 +110,11 @@ static void __tpda_enable(struct tpda_drvdata *drvdata, int port)
 static int tpda_enable(struct coresight_device *csdev, int inport, int outport)
 {
 	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	int ret;
+
+	ret = tpda_set_element_size(drvdata, csdev, inport, true);
+	if (ret)
+		return ret;
 
 	spin_lock(&drvdata->spinlock);
 	if (atomic_read(&csdev->refcnt[inport]) == 0)
diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
index 0399678..9ec5870 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 DSB data set element size bit */
+#define TPDA_Pn_CR_DSBSIZE		BIT(8)
 
 #define TPDA_MAX_INPORTS	32
 
@@ -23,6 +25,7 @@
  * @csdev:      component vitals needed by the framework.
  * @spinlock:   lock for the drvdata value.
  * @enable:     enable status of the component.
+ * @dsb_esize:   DSB element size
  */
 struct tpda_drvdata {
 	void __iomem		*base;
@@ -30,6 +33,7 @@ struct tpda_drvdata {
 	struct coresight_device	*csdev;
 	spinlock_t		spinlock;
 	u8			atid;
+	u32			dsb_esize[TPDA_MAX_INPORTS];
 };
 
 #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
-- 
2.7.4

[PATCH v3 03/11] coresight-tpdm: Initialize DSB subunit configuration

[Tao Zhang]

DSB is used for monitoring “events”. Events are something that
occurs at some point in time. It could be a state decode, the
act of writing/reading a particular address, a FIFO being empty,
etc. This decoding of the event desired is done outside TPDM.
DSB subunit need to be configured in enablement and disablement.
A struct that specifics associated to dsb dataset is needed. It
saves the configuration and parameters of the dsb datasets. This
change is to add this struct and initialize the configuration of
DSB subunit.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpdm.c | 58 +++++++++++++++++++++++++---
 drivers/hwtracing/coresight/coresight-tpdm.h | 17 ++++++++
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index f4854af..5e1e2ba 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -20,17 +20,59 @@
 
 DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
 
+static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
+{
+	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
+		if (!drvdata->dsb) {
+			drvdata->dsb = devm_kzalloc(drvdata->dev,
+						    sizeof(*drvdata->dsb), GFP_KERNEL);
+			if (!drvdata->dsb)
+				return -ENOMEM;
+		} else
+			memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
+
+		drvdata->dsb->trig_ts = true;
+		drvdata->dsb->trig_type = false;
+	}
+
+	return 0;
+}
+
+static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
+{
+	if (drvdata->dsb->trig_type)
+		*val |= TPDM_DSB_CR_TRIG_TYPE;
+	else
+		*val &= ~TPDM_DSB_CR_TRIG_TYPE;
+}
+
 static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 {
 	u32 val;
 
-	/* Set the enable bit of DSB control register to 1 */
+	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
+	/* Set trigger timestamp */
+	if (drvdata->dsb->trig_ts)
+		val |= TPDM_DSB_TIER_XTRIG_TSENAB;
+	else
+		val &= ~TPDM_DSB_TIER_XTRIG_TSENAB;
+	writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
+
 	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
+	/* Set trigger type */
+	set_trigger_type(drvdata, &val);
+	/* Set the enable bit of DSB control register to 1 */
 	val |= TPDM_DSB_CR_ENA;
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
 /* TPDM enable operations */
+/* The TPDM or Monitor serves as data collection component for various
+ * dataset types. It covers Basic Counts(BC), Tenure Counts(TC),
+ * Continuous Multi-Bit(CMB), Multi-lane CMB(MCMB) and Discrete Single
+ * Bit(DSB). This function will initialize the configuration according
+ * to the dataset type supported by the TPDM.
+ */
 static void __tpdm_enable(struct tpdm_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);
@@ -110,15 +152,13 @@ static const struct coresight_ops tpdm_cs_ops = {
 	.source_ops	= &tpdm_source_ops,
 };
 
-static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
+static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
 {
 	u32 pidr;
 
-	CS_UNLOCK(drvdata->base);
 	/*  Get the datasets present on the TPDM. */
 	pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
 	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
-	CS_LOCK(drvdata->base);
 }
 
 /*
@@ -181,6 +221,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 	struct coresight_platform_data *pdata;
 	struct tpdm_drvdata *drvdata;
 	struct coresight_desc desc = { 0 };
+	int ret;
 
 	pdata = coresight_get_platform_data(dev);
 	if (IS_ERR(pdata))
@@ -200,6 +241,8 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 
 	drvdata->base = base;
 
+	tpdm_datasets_setup(drvdata);
+
 	/* Set up coresight component description */
 	desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
 	if (!desc.name)
@@ -216,7 +259,12 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(drvdata->csdev);
 
 	spin_lock_init(&drvdata->spinlock);
-	tpdm_init_default_data(drvdata);
+	ret = tpdm_init_datasets(drvdata);
+	if (ret) {
+		coresight_unregister(drvdata->csdev);
+		return ret;
+	}
+
 	/* Decrease pm refcount when probe is done.*/
 	pm_runtime_put(&adev->dev);
 
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 5438540..68f33bd 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -11,8 +11,14 @@
 
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
+#define TPDM_DSB_TIER		(0x784)
+
 /* Enable bit for DSB subunit */
 #define TPDM_DSB_CR_ENA		BIT(0)
+/* Enable bit for DSB subunit trigger type */
+#define TPDM_DSB_CR_TRIG_TYPE		BIT(12)
+/* Enable bit for DSB subunit trigger timestamp */
+#define TPDM_DSB_TIER_XTRIG_TSENAB		BIT(1)
 
 /* TPDM integration test registers */
 #define TPDM_ITATBCNTRL		(0xEF0)
@@ -41,6 +47,16 @@
 #define TPDM_PIDR0_DS_DSB	BIT(1)
 
 /**
+ * struct dsb_dataset - specifics associated to dsb dataset
+ * @trig_ts:          Enable/Disable trigger timestamp.
+ * @trig_type:        Enable/Disable trigger type.
+ */
+struct dsb_dataset {
+	bool			trig_ts;
+	bool			trig_type;
+};
+
+/**
  * 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.
@@ -57,6 +73,7 @@ struct tpdm_drvdata {
 	spinlock_t		spinlock;
 	bool			enable;
 	unsigned long		datasets;
+	struct dsb_dataset	*dsb;
 };
 
 #endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
-- 
2.7.4

[PATCH v3 04/11] coresight-tpdm: Add reset node to TPDM node

[Tao Zhang]

TPDM device need a node to reset the configurations and status of
it. This change provides a node to reset the configurations and
disable the TPDM if it has been enabled.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpdm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 5e1e2ba..104638d 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
 	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
 }
 
+static ssize_t reset_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t size)
+{
+	int ret = 0;
+	unsigned long val;
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret || val != 1)
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	/* Reset all datasets to ZERO, and init the default data*/
+	tpdm_init_datasets(drvdata);
+
+	spin_unlock(&drvdata->spinlock);
+
+	/* Disable tpdm if enabled */
+	if (drvdata->enable)
+		coresight_disable(drvdata->csdev);
+
+	return size;
+}
+static DEVICE_ATTR_WO(reset);
+
 /*
  * value 1: 64 bits test data
  * value 2: 32 bits test data
@@ -201,6 +228,7 @@ static ssize_t integration_test_store(struct device *dev,
 static DEVICE_ATTR_WO(integration_test);
 
 static struct attribute *tpdm_attrs[] = {
+	&dev_attr_reset.attr,
 	&dev_attr_integration_test.attr,
 	NULL,
 };
-- 
2.7.4

[PATCH v3 05/11] coresight-tpdm: Add nodes to set trigger timestamp and type

[Tao Zhang]

The nodes are needed to set or show the trigger timestamp and
trigger type. This change is to add these nodes to achieve these
function.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 24 ++++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 95 ++++++++++++++++++++++
 2 files changed, 119 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index 4a58e64..27ce681 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -11,3 +11,27 @@ Description:
 		Accepts only one of the 2 values -  1 or 2.
 		1 : Generate 64 bits data
 		2 : Generate 32 bits data
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_trig_type
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the trigger type of DSB tpdm. Read the trigger
+		type of DSB tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Set the DSB trigger type to false
+		1 : Set the DSB trigger type to true
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_trig_ts
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the trigger timestamp of DSB tpdm. Read the
+		trigger timestamp of DSB tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Set the DSB trigger type to false
+		1 : Set the DSB trigger type to true
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 104638d..e28cf10 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -20,6 +20,19 @@
 
 DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
 
+static umode_t tpdm_dsb_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)
+		if (drvdata->datasets & TPDM_PIDR0_DS_DSB)
+			return attr->mode;
+
+	return 0;
+}
+
 static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
 {
 	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
@@ -237,8 +250,90 @@ static struct attribute_group tpdm_attr_grp = {
 	.attrs = tpdm_attrs,
 };
 
+static ssize_t dsb_trig_type_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->dsb->trig_type);
+}
+
+/*
+ * value 0: set trigger type as enablement
+ * value 1: set trigger type as disablement
+ */
+static ssize_t dsb_trig_type_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->dsb->trig_type = true;
+	else
+		drvdata->dsb->trig_type = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_trig_type);
+
+static ssize_t dsb_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->dsb->trig_ts);
+}
+
+/*
+ * value 0: set trigger timestamp as enablement
+ * value 1: set trigger timestamp as disablement
+ */
+static ssize_t dsb_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->dsb->trig_ts = true;
+	else
+		drvdata->dsb->trig_ts = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_trig_ts);
+
+static struct attribute *tpdm_dsb_attrs[] = {
+	&dev_attr_dsb_trig_ts.attr,
+	&dev_attr_dsb_trig_type.attr,
+	NULL,
+};
+
+static struct attribute_group tpdm_dsb_attr_grp = {
+	.attrs = tpdm_dsb_attrs,
+	.is_visible = tpdm_dsb_is_visible,
+};
+
 static const struct attribute_group *tpdm_attr_grps[] = {
 	&tpdm_attr_grp,
+	&tpdm_dsb_attr_grp,
 	NULL,
 };
 
-- 
2.7.4

[PATCH v3 06/11] coresight-tpdm: Add node to set dsb programming mode

[Tao Zhang]

Add node to set and show programming mode for TPDM DSB subunit.
Once the DSB programming mode is set, it will be written to the
register DSB_CR. Bit[10:9] of the DSB_CR register is used to set
the DSB test mode.

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

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index 27ce681..f13e282 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -35,3 +35,13 @@ Description:
 		Accepts only one of the 2 values -  0 or 1.
 		0 : Set the DSB trigger type to false
 		1 : Set the DSB trigger type to true
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_mode
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the mode of DSB tpdm. Read the mode of DSB
+		tpdm.
+
+		Accepts the value needs to be greater than 0.
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index e28cf10..8cd822f 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/amba/bus.h>
+#include <linux/bitfield.h>
 #include <linux/bitmap.h>
 #include <linux/coresight.h>
 #include <linux/coresight-pmu.h>
@@ -51,6 +52,32 @@ static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
 	return 0;
 }
 
+static void set_dsb_cycacc_mode(struct tpdm_drvdata *drvdata, u32 *val)
+{
+	u32 mode;
+
+	mode = TPDM_DSB_MODE_CYCACC(drvdata->dsb->mode);
+	*val &= ~TPDM_DSB_TEST_MODE;
+	*val |= FIELD_PREP(TPDM_DSB_TEST_MODE, mode);
+}
+
+static void set_dsb_hpsel_mode(struct tpdm_drvdata *drvdata, u32 *val)
+{
+	u32 mode;
+
+	mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode);
+	*val &= ~TPDM_DSB_HPSEL;
+	*val |= FIELD_PREP(TPDM_DSB_HPSEL, mode);
+}
+
+static void set_dsb_perf_mode(struct tpdm_drvdata *drvdata, u32 *val)
+{
+	if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF)
+		*val |= TPDM_DSB_CR_MODE;
+	else
+		*val &= ~TPDM_DSB_CR_MODE;
+}
+
 static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
 {
 	if (drvdata->dsb->trig_type)
@@ -72,6 +99,12 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
 
 	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
+	/* Set the cycle accurate mode */
+	set_dsb_cycacc_mode(drvdata, &val);
+	/* Set the byte lane for high-performance mode */
+	set_dsb_hpsel_mode(drvdata, &val);
+	/* Set the performance mode */
+	set_dsb_perf_mode(drvdata, &val);
 	/* Set trigger type */
 	set_trigger_type(drvdata, &val);
 	/* Set the enable bit of DSB control register to 1 */
@@ -250,6 +283,34 @@ static struct attribute_group tpdm_attr_grp = {
 	.attrs = tpdm_attrs,
 };
 
+static ssize_t dsb_mode_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%lx\n",
+			 (unsigned long)drvdata->dsb->mode);
+}
+
+static ssize_t dsb_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 val;
+
+	if ((kstrtoul(buf, 0, &val)) || val < 0)
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->dsb->mode = val & TPDM_MODE_ALL;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_mode);
+
 static ssize_t dsb_trig_type_show(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)
@@ -321,6 +382,7 @@ static ssize_t dsb_trig_ts_store(struct device *dev,
 static DEVICE_ATTR_RW(dsb_trig_ts);
 
 static struct attribute *tpdm_dsb_attrs[] = {
+	&dev_attr_dsb_mode.attr,
 	&dev_attr_dsb_trig_ts.attr,
 	&dev_attr_dsb_trig_type.attr,
 	NULL,
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 68f33bd..8fee562 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -15,11 +15,22 @@
 
 /* Enable bit for DSB subunit */
 #define TPDM_DSB_CR_ENA		BIT(0)
+/* Enable bit for DSB subunit perfmance mode */
+#define TPDM_DSB_CR_MODE		BIT(1)
 /* Enable bit for DSB subunit trigger type */
 #define TPDM_DSB_CR_TRIG_TYPE		BIT(12)
+
 /* Enable bit for DSB subunit trigger timestamp */
 #define TPDM_DSB_TIER_XTRIG_TSENAB		BIT(1)
 
+/* DSB programming modes */
+#define TPDM_DSB_MODE_CYCACC(val)	(val & GENMASK(2, 0))
+#define TPDM_DSB_MODE_PERF		BIT(3)
+#define TPDM_DSB_MODE_HPBYTESEL(val)	(val & GENMASK(8, 4))
+#define TPDM_MODE_ALL			(0xFFFFFFF)
+#define TPDM_DSB_TEST_MODE		GENMASK(11, 9)
+#define TPDM_DSB_HPSEL		GENMASK(6, 2)
+
 /* TPDM integration test registers */
 #define TPDM_ITATBCNTRL		(0xEF0)
 #define TPDM_ITCNTRL		(0xF00)
@@ -48,10 +59,12 @@
 
 /**
  * struct dsb_dataset - specifics associated to dsb dataset
+ * @mode:             DSB programming mode
  * @trig_ts:          Enable/Disable trigger timestamp.
  * @trig_type:        Enable/Disable trigger type.
  */
 struct dsb_dataset {
+	u32				mode;
 	bool			trig_ts;
 	bool			trig_type;
 };
-- 
2.7.4

[PATCH v3 07/11] coresight-tpdm: Add nodes for dsb edge control

[Tao Zhang]

Add the nodes to set value for DSB edge control and DSB edge
control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
resgisters to configure edge control. DSB edge detection control
00: Rising edge detection
01: Falling edge detection
10: Rising and falling edge detection (toggle detection)
And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
configure mask. Eight 32 bit registers providing DSB interface
edge detection mask control.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  26 ++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 142 ++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tpdm.h       |  14 ++
 3 files changed, 181 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index f13e282..094d624 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -45,3 +45,29 @@ Description:
 		tpdm.
 
 		Accepts the value needs to be greater than 0.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the edge control of DSB tpdm. Read the
+		edge control of DSB tpdm.
+
+		Accepts the following three values.
+		value 1: Start EDCR register number
+		value 2: End EDCR register number
+		value 3: The value need to be written
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the edge control mask of DSB tpdm. Read
+		the edge control mask of DSB tpdm.
+
+		Accepts the following three values.
+		value 1: Start EDCMR register number
+		value 2: End EDCMR register number
+		value 3: The value need to be written
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 8cd822f..2a0b36c 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -88,7 +88,14 @@ static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
 
 static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 {
-	u32 val;
+	u32 val, i;
+
+	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++)
+		writel_relaxed(drvdata->dsb->edge_ctrl[i],
+			   drvdata->base + TPDM_DSB_EDCR(i));
+	for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++)
+		writel_relaxed(drvdata->dsb->edge_ctrl_mask[i],
+			   drvdata->base + TPDM_DSB_EDCMR(i));
 
 	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
 	/* Set trigger timestamp */
@@ -311,6 +318,137 @@ static ssize_t dsb_mode_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(dsb_mode);
 
+static ssize_t dsb_edge_ctrl_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
+		size += sysfs_emit_at(buf, size,
+				  "Index:0x%x Val:0x%x\n", i,
+				  drvdata->dsb->edge_ctrl[i]);
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+/*
+ * value 1: Start EDCR register number
+ * value 2: End EDCR register number
+ * value 3: The value need to be written
+ * The EDCR registers can include up to 16 32-bit registers, and each
+ * one can be configured to control up to 16 edge detections(2 bits
+ * control one edge detection). So a total 256 edge detections can be
+ * configured. So the starting number(value 1) and ending number(value 2)
+ * cannot be greater than 256, and value 1 should be less than value 2.
+ * The following values are the rage of value 3.
+ * 0 - Rising edge detection
+ * 1 - Falling edge detection
+ * 2 - Rising and falling edge detection (toggle detection)
+ */
+static ssize_t dsb_edge_ctrl_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 start, end, edge_ctrl;
+	uint32_t val;
+	int i, index, bit, reg;
+
+	if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3)
+		return -EINVAL;
+	if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) ||
+	    edge_ctrl > 0x2)
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = start; i <= end; i++) {
+		/*
+		 * The 32-bit register has 32 bits(NUM_OF_BITS).
+		 * Each one register can be configured to control 16
+		 * (NUM_OF_BITS / 2) edge detectioins.
+		 */
+		reg = i / (NUM_OF_BITS / 2);
+		index = i % (NUM_OF_BITS / 2);
+		bit = index * 2;
+
+		val = drvdata->dsb->edge_ctrl[reg];
+		val &= ~GENMASK((bit + 1), bit);
+		val |= (edge_ctrl << bit);
+		drvdata->dsb->edge_ctrl[reg] = val;
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_edge_ctrl);
+
+static ssize_t dsb_edge_ctrl_mask_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_EDCR / 2; i++) {
+		size += sysfs_emit_at(buf, size,
+				  "Index:0x%x Val:0x%x\n", i,
+				  drvdata->dsb->edge_ctrl_mask[i]);
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+/*
+ * value 1: Start EDCMR register number
+ * value 2: End EDCMR register number
+ * value 3: The value need to be written
+ */
+static ssize_t dsb_edge_ctrl_mask_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 start, end, val;
+	u32 set;
+	int i, index, reg;
+
+	if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3)
+		return -EINVAL;
+	if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES)
+		|| (val & ~1UL))
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = start; i <= end; i++) {
+		/*
+		 * The 32-bit register has 32 bits(NUM_OF_BITS).
+		 * Each one register can be configured to control 32
+		 * (NUM_OF_BITS) edge detectioin masks.
+		 */
+		reg = i / NUM_OF_BITS;
+		index = (i % NUM_OF_BITS);
+
+		set = drvdata->dsb->edge_ctrl_mask[reg];
+		if (val)
+			set |= BIT(index);
+		else
+			set &= ~BIT(index);
+		drvdata->dsb->edge_ctrl_mask[reg] = set;
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
+
 static ssize_t dsb_trig_type_show(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)
@@ -383,6 +521,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts);
 
 static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_mode.attr,
+	&dev_attr_dsb_edge_ctrl.attr,
+	&dev_attr_dsb_edge_ctrl_mask.attr,
 	&dev_attr_dsb_trig_ts.attr,
 	&dev_attr_dsb_trig_type.attr,
 	NULL,
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 8fee562..342ef23 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -12,6 +12,8 @@
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
+#define TPDM_DSB_EDCR(n)	(0x808 + (n * 4))
+#define TPDM_DSB_EDCMR(n)	(0x848 + (n * 4))
 
 /* Enable bit for DSB subunit */
 #define TPDM_DSB_CR_ENA		BIT(0)
@@ -31,6 +33,8 @@
 #define TPDM_DSB_TEST_MODE		GENMASK(11, 9)
 #define TPDM_DSB_HPSEL		GENMASK(6, 2)
 
+#define NUM_OF_BITS		32
+
 /* TPDM integration test registers */
 #define TPDM_ITATBCNTRL		(0xEF0)
 #define TPDM_ITCNTRL		(0xF00)
@@ -57,14 +61,24 @@
 #define TPDM_PIDR0_DS_IMPDEF	BIT(0)
 #define TPDM_PIDR0_DS_DSB	BIT(1)
 
+#define TPDM_DSB_MAX_LINES	256
+/* MAX number of EDCR registers */
+#define TPDM_DSB_MAX_EDCR	16
+/* MAX number of EDCMR registers */
+#define TPDM_DSB_MAX_EDCMR	8
+
 /**
  * struct dsb_dataset - specifics associated to dsb dataset
  * @mode:             DSB programming mode
+ * @edge_ctrl:        Save value for edge control
+ * @edge_ctrl_mask:   Save value for edge control mask
  * @trig_ts:          Enable/Disable trigger timestamp.
  * @trig_type:        Enable/Disable trigger type.
  */
 struct dsb_dataset {
 	u32				mode;
+	u32				edge_ctrl[TPDM_DSB_MAX_EDCR];
+	u32				edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
 	bool			trig_ts;
 	bool			trig_type;
 };
-- 
2.7.4

[PATCH v3 08/11] coresight-tpdm: Add nodes to configure pattern match output

[Tao Zhang]

Add nodes to configure trigger pattern and trigger pattern mask.
Each DSB subunit TPDM has maximum of n(n<7) XPR registers to
configure trigger pattern match output. Eight 32 bit registers
providing DSB interface trigger output pattern match comparison.
And each DSB subunit TPDM has maximum of m(m<7) XPMR registers to
configure trigger pattern mask match output. Eight 32 bit
registers providing DSB interface trigger output pattern match
mask.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 24 +++++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 84 ++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h       |  8 +++
 3 files changed, 116 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index 094d624..c06374f 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -71,3 +71,27 @@ Description:
 		value 1: Start EDCMR register number
 		value 2: End EDCMR register number
 		value 3: The value need to be written
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_trig_patt_val
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the trigger pattern value of DSB tpdm.
+		Read the trigger pattern value of DSB tpdm.
+
+		Accepts the following two values.
+		value 1: Index number of XPR register
+		value 2: The value need to be written
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_trig_patt_mask
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the trigger pattern mask of DSB tpdm.
+		Read the trigger pattern mask of DSB tpdm.
+
+		Accepts the following two values.
+		value 1: Index number of XPMR register
+		value 2: The value need to be written
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 2a0b36c..d6cc6b5 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -97,6 +97,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 		writel_relaxed(drvdata->dsb->edge_ctrl_mask[i],
 			   drvdata->base + TPDM_DSB_EDCMR(i));
 
+	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
+		writel_relaxed(drvdata->dsb->trig_patt_val[i],
+			    drvdata->base + TPDM_DSB_XPR(i));
+		writel_relaxed(drvdata->dsb->trig_patt_mask[i],
+			    drvdata->base + TPDM_DSB_XPMR(i));
+	}
+
 	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
 	/* Set trigger timestamp */
 	if (drvdata->dsb->trig_ts)
@@ -448,6 +455,81 @@ static ssize_t dsb_edge_ctrl_mask_store(struct device *dev,
 	return size;
 }
 static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
+static ssize_t dsb_trig_patt_val_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i = 0;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
+		size += sysfs_emit_at(buf, size,
+				  "Index: 0x%x Value: 0x%x\n", i,
+				  drvdata->dsb->trig_patt_val[i]);
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+static ssize_t dsb_trig_patt_val_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 index, val;
+
+	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
+		return -EINVAL;
+	if (index >= TPDM_DSB_MAX_PATT)
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->dsb->trig_patt_val[index] = val;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_trig_patt_val);
+
+static ssize_t dsb_trig_patt_mask_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i = 0;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
+		size += sysfs_emit_at(buf, size,
+				  "Index: 0x%x Value: 0x%x\n", i,
+				  drvdata->dsb->trig_patt_mask[i]);
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+static ssize_t dsb_trig_patt_mask_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 index, val;
+
+	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
+		return -EINVAL;
+	if (index >= TPDM_DSB_MAX_PATT)
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->dsb->trig_patt_mask[index] = val;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_trig_patt_mask);
 
 static ssize_t dsb_trig_type_show(struct device *dev,
 				     struct device_attribute *attr,
@@ -523,6 +605,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_mode.attr,
 	&dev_attr_dsb_edge_ctrl.attr,
 	&dev_attr_dsb_edge_ctrl_mask.attr,
+	&dev_attr_dsb_trig_patt_val.attr,
+	&dev_attr_dsb_trig_patt_mask.attr,
 	&dev_attr_dsb_trig_ts.attr,
 	&dev_attr_dsb_trig_type.attr,
 	NULL,
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 342ef23..2e8020e 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -12,6 +12,8 @@
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
+#define TPDM_DSB_XPR(n)		(0x7C8 + (n * 4))
+#define TPDM_DSB_XPMR(n)	(0x7E8 + (n * 4))
 #define TPDM_DSB_EDCR(n)	(0x808 + (n * 4))
 #define TPDM_DSB_EDCMR(n)	(0x848 + (n * 4))
 
@@ -66,12 +68,16 @@
 #define TPDM_DSB_MAX_EDCR	16
 /* MAX number of EDCMR registers */
 #define TPDM_DSB_MAX_EDCMR	8
+/* MAX number of DSB pattern */
+#define TPDM_DSB_MAX_PATT	8
 
 /**
  * struct dsb_dataset - specifics associated to dsb dataset
  * @mode:             DSB programming mode
  * @edge_ctrl:        Save value for edge control
  * @edge_ctrl_mask:   Save value for edge control mask
+ * @trig_patt_val:    Save value for trigger pattern
+ * @trig_patt_mask:   Save value for trigger pattern mask
  * @trig_ts:          Enable/Disable trigger timestamp.
  * @trig_type:        Enable/Disable trigger type.
  */
@@ -79,6 +85,8 @@ struct dsb_dataset {
 	u32				mode;
 	u32				edge_ctrl[TPDM_DSB_MAX_EDCR];
 	u32				edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
+	u32				trig_patt_val[TPDM_DSB_MAX_PATT];
+	u32				trig_patt_mask[TPDM_DSB_MAX_PATT];
 	bool			trig_ts;
 	bool			trig_type;
 };
-- 
2.7.4

[PATCH v3 09/11] coresight-tpdm: Add nodes for timestamp request

[Tao Zhang]

Add nodes to configure the timestamp request based on input
pattern match. Each TPDM that support DSB subunit has maximum of
n(n<7) TPR registers to configure value for timestamp request
based on input pattern match. Eight 32 bit registers providing
DSB interface timestamp request  pattern match comparison. And
each TPDM that support DSB subunit has maximum of m(m<7) TPMR
registers to configure pattern mask for timestamp request. Eight
32 bit registers providing DSB interface timestamp request
pattern match mask generation. Add nodes to enable/disable
pattern timestamp and set pattern timestamp type.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  48 ++++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 172 +++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h       |  14 ++
 3 files changed, 234 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index c06374f..60ff660 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -95,3 +95,51 @@ Description:
 		Accepts the following two values.
 		value 1: Index number of XPMR register
 		value 2: The value need to be written
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_val
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the pattern value of DSB tpdm. Read
+		the pattern value of DSB tpdm.
+
+		Accepts the following two values.
+		value 1: Index number of TPR register
+		value 2: The value need to be written
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_mask
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the pattern mask of DSB tpdm. Read
+		the pattern mask of DSB tpdm.
+
+		Accepts the following two values.
+		value 1: Index number of TPMR register
+		value 2: The value need to be written
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_ts
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the pattern timestamp of DSB tpdm. Read
+		the pattern timestamp of DSB tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Set the DSB pattern timestamp to false
+		1 : Set the DSB pattern timestamp to true
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_type
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the pattern type of DSB tpdm. Read
+		the pattern type of DSB tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Set the DSB pattern type to false
+		1 : Set the DSB pattern type to true
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index d6cc6b5..c740681 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -98,6 +98,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 			   drvdata->base + TPDM_DSB_EDCMR(i));
 
 	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
+		writel_relaxed(drvdata->dsb->patt_val[i],
+			    drvdata->base + TPDM_DSB_TPR(i));
+		writel_relaxed(drvdata->dsb->patt_mask[i],
+			    drvdata->base + TPDM_DSB_TPMR(i));
+	}
+
+	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
 		writel_relaxed(drvdata->dsb->trig_patt_val[i],
 			    drvdata->base + TPDM_DSB_XPR(i));
 		writel_relaxed(drvdata->dsb->trig_patt_mask[i],
@@ -105,6 +112,16 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 	}
 
 	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
+	/* Set pattern timestamp type and enablement */
+	if (drvdata->dsb->patt_ts) {
+		val |= TPDM_DSB_TIER_PATT_TSENAB;
+		if (drvdata->dsb->patt_type)
+			val |= TPDM_DSB_TIER_PATT_TYPE;
+		else
+			val &= ~TPDM_DSB_TIER_PATT_TYPE;
+	} else {
+		val &= ~TPDM_DSB_TIER_PATT_TSENAB;
+	}
 	/* Set trigger timestamp */
 	if (drvdata->dsb->trig_ts)
 		val |= TPDM_DSB_TIER_XTRIG_TSENAB;
@@ -455,6 +472,157 @@ static ssize_t dsb_edge_ctrl_mask_store(struct device *dev,
 	return size;
 }
 static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
+
+static ssize_t dsb_patt_val_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i = 0;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
+		size += sysfs_emit_at(buf, size,
+				  "Index: 0x%x Value: 0x%x\n", i,
+				  drvdata->dsb->patt_val[i]);
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+/*
+ * value 1: Index of TPR register
+ * value 2: Value need to be written
+ */
+static ssize_t dsb_patt_val_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 index, val;
+
+	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
+		return -EINVAL;
+	if (index >= TPDM_DSB_MAX_PATT)
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->dsb->patt_val[index] = val;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_patt_val);
+
+static ssize_t dsb_patt_mask_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i = 0;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
+		size += sysfs_emit_at(buf, size,
+				  "Index: 0x%x Value: 0x%x\n", i,
+				  drvdata->dsb->patt_mask[i]);
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+/*
+ * value 1: Index of TPMR register
+ * value 2: Value need to be written
+ */
+static ssize_t dsb_patt_mask_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 index, val;
+
+	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
+		return -EINVAL;
+	if (index >= TPDM_DSB_MAX_PATT)
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->dsb->patt_mask[index] = val;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_patt_mask);
+
+static ssize_t dsb_patt_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->dsb->patt_ts);
+}
+
+/*
+ * value 1: Enable/Disable DSB pattern timestamp
+ */
+static ssize_t dsb_patt_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->dsb->patt_ts = true;
+	else
+		drvdata->dsb->patt_ts = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_patt_ts);
+
+static ssize_t dsb_patt_type_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->dsb->patt_type);
+}
+
+/*
+ * value 1: Set DSB pattern type
+ */
+static ssize_t dsb_patt_type_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->dsb->patt_type = true;
+	else
+		drvdata->dsb->patt_type = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_patt_type);
+
 static ssize_t dsb_trig_patt_val_show(struct device *dev,
 					   struct device_attribute *attr,
 					   char *buf)
@@ -605,6 +773,10 @@ static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_mode.attr,
 	&dev_attr_dsb_edge_ctrl.attr,
 	&dev_attr_dsb_edge_ctrl_mask.attr,
+	&dev_attr_dsb_patt_val.attr,
+	&dev_attr_dsb_patt_mask.attr,
+	&dev_attr_dsb_patt_ts.attr,
+	&dev_attr_dsb_patt_type.attr,
 	&dev_attr_dsb_trig_patt_val.attr,
 	&dev_attr_dsb_trig_patt_mask.attr,
 	&dev_attr_dsb_trig_ts.attr,
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 2e8020e..f9d4dd9 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -12,6 +12,8 @@
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
+#define TPDM_DSB_TPR(n)		(0x788 + (n * 4))
+#define TPDM_DSB_TPMR(n)	(0x7A8 + (n * 4))
 #define TPDM_DSB_XPR(n)		(0x7C8 + (n * 4))
 #define TPDM_DSB_XPMR(n)	(0x7E8 + (n * 4))
 #define TPDM_DSB_EDCR(n)	(0x808 + (n * 4))
@@ -24,8 +26,12 @@
 /* Enable bit for DSB subunit trigger type */
 #define TPDM_DSB_CR_TRIG_TYPE		BIT(12)
 
+/* Enable bit for DSB subunit pattern timestamp */
+#define TPDM_DSB_TIER_PATT_TSENAB		BIT(0)
 /* Enable bit for DSB subunit trigger timestamp */
 #define TPDM_DSB_TIER_XTRIG_TSENAB		BIT(1)
+/* Bit for DSB subunit pattern type */
+#define TPDM_DSB_TIER_PATT_TYPE		BIT(2)
 
 /* DSB programming modes */
 #define TPDM_DSB_MODE_CYCACC(val)	(val & GENMASK(2, 0))
@@ -76,6 +82,10 @@
  * @mode:             DSB programming mode
  * @edge_ctrl:        Save value for edge control
  * @edge_ctrl_mask:   Save value for edge control mask
+ * @patt_val:         Save value for pattern
+ * @patt_mask:        Save value for pattern mask
+ * @patt_ts:          Enable/Disable pattern timestamp
+ * @patt_type:        Set pattern type
  * @trig_patt_val:    Save value for trigger pattern
  * @trig_patt_mask:   Save value for trigger pattern mask
  * @trig_ts:          Enable/Disable trigger timestamp.
@@ -85,6 +95,10 @@ struct dsb_dataset {
 	u32				mode;
 	u32				edge_ctrl[TPDM_DSB_MAX_EDCR];
 	u32				edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
+	u32				patt_val[TPDM_DSB_MAX_PATT];
+	u32				patt_mask[TPDM_DSB_MAX_PATT];
+	bool			patt_ts;
+	bool			patt_type;
 	u32				trig_patt_val[TPDM_DSB_MAX_PATT];
 	u32				trig_patt_mask[TPDM_DSB_MAX_PATT];
 	bool			trig_ts;
-- 
2.7.4

[PATCH v3 10/11] dt-bindings: arm: Add support for DSB MSR register

[Tao Zhang]

Add property "qcom,dsb_msr_num" to support DSB(Discrete Single
Bit) MSR(mux select register) for TPDM. It specifies the number
of MSR registers supported by the DSB TDPM.

Signed-off-by: Tao Zhang <quic_taozha@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 d9b6b613..691c7ba 100644
--- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
@@ -53,6 +53,15 @@ properties:
     minimum: 32
     maximum: 64
 
+  qcom,dsb_msr_num:
+    description:
+      Specifies the number of DSB(Discrete Single Bit) MSR(mux select register)
+      registers supported by the monitor. If this property is not configured
+      or set to 0, it means this DSB TPDM doesn't support MSR.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 32
+
   clocks:
     maxItems: 1
 
@@ -87,6 +96,7 @@ examples:
       reg = <0x0684c000 0x1000>;
 
       qcom,dsb-element-size = <32>;
+      qcom,dsb_msr_num = <16>;
 
       clocks = <&aoss_qmp>;
       clock-names = "apb_pclk";
-- 
2.7.4

[PATCH v3 11/11] coresight-tpdm: Add nodes for dsb msr support

[Tao Zhang]

Add the nodes for DSB subunit MSR(mux select register) support.
The TPDM MSR (mux select register) interface is an optional
interface and associated bank of registers per TPDM subunit.
The intent of mux select registers is to control muxing structures
driving the TPDM’s’ various subunit interfaces.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 12 +++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 53 ++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h       | 17 ++++---
 3 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index 60ff660..6bdba7d 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -143,3 +143,15 @@ Description:
 		Accepts only one of the 2 values -  0 or 1.
 		0 : Set the DSB pattern type to false
 		1 : Set the DSB pattern type to true
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_msr
+Date:		March 2023
+KernelVersion	6.3
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the MSR(mux select register) of DSB tpdm. Read
+		the MSR(mux select register) of DSB tpdm.
+
+		Accepts the following two values.
+		value 1: Index number of MSR register
+		value 2: The value need to be written
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index c740681..5aaee06 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -42,6 +42,14 @@ static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
 						    sizeof(*drvdata->dsb), GFP_KERNEL);
 			if (!drvdata->dsb)
 				return -ENOMEM;
+			if (!of_property_read_u32(drvdata->dev->of_node,
+					   "qcom,dsb_msr_num", &drvdata->dsb->msr_num)) {
+				drvdata->dsb->msr = devm_kzalloc(drvdata->dev,
+					(drvdata->dsb->msr_num * sizeof(*drvdata->dsb->msr)),
+					GFP_KERNEL);
+					if (!drvdata->dsb->msr)
+						return -ENOMEM;
+				}
 		} else
 			memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
 
@@ -769,6 +777,50 @@ static ssize_t dsb_trig_ts_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(dsb_trig_ts);
 
+static ssize_t dsb_msr_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned int i;
+	ssize_t size = 0;
+
+	if (drvdata->dsb->msr_num == 0)
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
+		size += sysfs_emit_at(buf, size,
+				  "%u 0x%x\n", i, drvdata->dsb->msr[i]);
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+
+static ssize_t dsb_msr_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned int num, val;
+	int nval;
+
+	if (drvdata->dsb->msr_num == 0)
+		return -EINVAL;
+
+	nval = sscanf(buf, "%u %x", &num, &val);
+	if ((nval != 2) || (num >= (drvdata->dsb->msr_num - 1)))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->dsb->msr[num] = val;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_msr);
+
 static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_mode.attr,
 	&dev_attr_dsb_edge_ctrl.attr,
@@ -781,6 +833,7 @@ static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_trig_patt_mask.attr,
 	&dev_attr_dsb_trig_ts.attr,
 	&dev_attr_dsb_trig_type.attr,
+	&dev_attr_dsb_msr.attr,
 	NULL,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index f9d4dd9..1872f26 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -18,6 +18,7 @@
 #define TPDM_DSB_XPMR(n)	(0x7E8 + (n * 4))
 #define TPDM_DSB_EDCR(n)	(0x808 + (n * 4))
 #define TPDM_DSB_EDCMR(n)	(0x848 + (n * 4))
+#define TPDM_DSB_MSR(n)		(0x980 + (n * 4))
 
 /* Enable bit for DSB subunit */
 #define TPDM_DSB_CR_ENA		BIT(0)
@@ -92,17 +93,19 @@
  * @trig_type:        Enable/Disable trigger type.
  */
 struct dsb_dataset {
-	u32				mode;
-	u32				edge_ctrl[TPDM_DSB_MAX_EDCR];
-	u32				edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
-	u32				patt_val[TPDM_DSB_MAX_PATT];
-	u32				patt_mask[TPDM_DSB_MAX_PATT];
+	u32			mode;
+	u32			edge_ctrl[TPDM_DSB_MAX_EDCR];
+	u32			edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
+	u32			patt_val[TPDM_DSB_MAX_PATT];
+	u32			patt_mask[TPDM_DSB_MAX_PATT];
 	bool			patt_ts;
 	bool			patt_type;
-	u32				trig_patt_val[TPDM_DSB_MAX_PATT];
-	u32				trig_patt_mask[TPDM_DSB_MAX_PATT];
+	u32			trig_patt_val[TPDM_DSB_MAX_PATT];
+	u32			trig_patt_mask[TPDM_DSB_MAX_PATT];
 	bool			trig_ts;
 	bool			trig_type;
+	u32			msr_num;
+	u32			*msr;
 };
 
 /**
-- 
2.7.4

Re: [PATCH v3 01/11] dt-bindings: arm: Add support for DSB element size

[Suzuki K Poulose]

On 23/03/2023 06:03, Tao Zhang wrote:
> Add property "qcom,dsb-elem-size" to support DSB(Discrete Single
> Bit) element for TPDM. The associated aggregator will read this
> size before it is enabled. DSB element size currently only
> supports 32-bit and 64-bit.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   .../devicetree/bindings/arm/qcom,coresight-tpdm.yaml          | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> index 5c08342..d9b6b613 100644
> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> @@ -44,6 +44,15 @@ properties:
>       minItems: 1
>       maxItems: 2


>   
> +  qcom,dsb-element-size:
> +    description:
> +      Specifies the DSB(Discrete Single Bit) element size supported by
> +      the monitor. The associated aggregator will read this size before it
> +      is enabled. DSB element size currently only supports 32-bit and 64-bit.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 32
> +    maximum: 64

Shouldn't this be something like oneOf ? It is not a range, but one of
those two specific values ?

Suzuki

Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Suzuki K Poulose]

On 23/03/2023 06:03, Tao Zhang wrote:
> Read the DSB element size from the device tree. Set the register
> bit that controls the DSB element size of the corresponding port.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpda.c | 58 ++++++++++++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>   2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index f712e11..8dcfc4a 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -21,6 +21,47 @@
>   
>   DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>   
> +/* Search and read element data size from the TPDM node in
> + * the devicetree. Each input port of TPDA is connected to
> + * a TPDM. Different TPDM supports different types of dataset,
> + * and some may support more than one type of dataset.
> + * Parameter "inport" is used to pass in the input port number
> + * of TPDA, and it is set to 0 in the recursize call.
> + * Parameter "parent" is used to pass in the original call.
> + */

I am still not clear why we need to do this recursively ?

> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
> +			   struct coresight_device *csdev, int inport, bool parent)

Please could we renamse csdev => tpda_dev

> +{
> +	static int nr_inport;
> +	int i;
> +	struct coresight_device *in_csdev;

similarly tpdm_dev ?

Could we not add a check here to see if the dsb_esize[inport] is already
set and then bail out, reading this over and over ?

> +
> +	if (inport > (TPDA_MAX_INPORTS - 1))
> +		return -EINVAL;
> +
> +	if (parent)
> +		nr_inport = inport;
> +
> +	for (i = 0; i < csdev->pdata->nr_inconns; i++) {
> +		in_csdev = csdev->pdata->in_conns[i].remote_dev;

Please note, the names of the structure field might change in the
next version of James' series

> +		if (!in_csdev)
> +			break;
> +
> +		if (parent)
> +			if (csdev->pdata->in_conns[i].port != inport)
> +				continue;
> +
> +		if (in_csdev && strstr(dev_name(&in_csdev->dev), "tpdm")) {

Isn't there a better way to distinguish a device to be TPDM ? May be we
could even add a source_sub_type - SOURCE_TPDM instead of using
SOURCE_OTHERS ? Do you expect other sources to be connected to TPDA?
e.g., STMs ?

> +			of_property_read_u32(in_csdev->dev.parent->of_node,
> +					"qcom,dsb-element-size", &drvdata->dsb_esize[nr_inport]);
> +			break;
> +		}
> +		tpda_set_element_size(drvdata, in_csdev, 0, false);

What is the point of this ? Is this for covering the a TPDA connected to
another TPDA ?

e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?

And you want to figure out the DSB size of TPDM0 when you want to enable
TPDA1 ? How do you choose between that size of TPDM0 vs TPDM1 ?

Please add a proper documentation for this function ? If TPDA0 is in the
the path, it should have been enabled before you reach TPDA1. Thus,
the dsb_esize array must have been initialised for TPDA0 and thus, you
could simply read it from the dsb_esize[] of TPDA0.
You could always look at the device_type and sub_type to detect a
TPDA{0} connected into TPDA{1}



> +	}
> +
> +	return 0;
> +}
> +
>   /* Settings pre enabling port control register */
>   static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>   {
> @@ -37,6 +78,18 @@ static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>   	u32 val;
>   
>   	val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
> +	/*
> +	 * Configure aggregator port n DSB data set element size
> +	 * Set the bit to 0 if the size is 32
> +	 * Set the bit to 1 if the size is 64
> +	 */
> +	if (drvdata->dsb_esize[port] == 32)
> +		val &= ~TPDA_Pn_CR_DSBSIZE;
> +	else if (drvdata->dsb_esize[port] == 64)
> +		val |= TPDA_Pn_CR_DSBSIZE;
> +	else
> +		dev_err(drvdata->dev,
> +			"DSB data size input from port[%d] is invalid\n", port);

WARN_ON_ONCE() and abort the enable opration ? Or say, "fallback to 
32bit or 64bit" if one of them is a safer option ? Please don't
leave it unknown.

>   	/* Enable the port */
>   	val |= TPDA_Pn_CR_ENA;
>   	writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> @@ -57,6 +110,11 @@ static void __tpda_enable(struct tpda_drvdata *drvdata, int port)
>   static int tpda_enable(struct coresight_device *csdev, int inport, int outport)
>   {
>   	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	int ret;
> +
> +	ret = tpda_set_element_size(drvdata, csdev, inport, true);
> +	if (ret)
> +		return ret;
>   
>   	spin_lock(&drvdata->spinlock);
>   	if (atomic_read(&csdev->refcnt[inport]) == 0)
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index 0399678..9ec5870 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 DSB data set element size bit */
> +#define TPDA_Pn_CR_DSBSIZE		BIT(8)
>   
>   #define TPDA_MAX_INPORTS	32
>   
> @@ -23,6 +25,7 @@
>    * @csdev:      component vitals needed by the framework.
>    * @spinlock:   lock for the drvdata value.
>    * @enable:     enable status of the component.
> + * @dsb_esize:   DSB element size

Please state, must be 32 or 64.

>    */
>   struct tpda_drvdata {
>   	void __iomem		*base;
> @@ -30,6 +33,7 @@ struct tpda_drvdata {
>   	struct coresight_device	*csdev;
>   	spinlock_t		spinlock;
>   	u8			atid;
> +	u32			dsb_esize[TPDA_MAX_INPORTS];

Couldn't this be u8 ?


Suzuki

>   };
>   
>   #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */

Re: [PATCH v3 03/11] coresight-tpdm: Initialize DSB subunit configuration

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> DSB is used for monitoring “events”. Events are something that
> occurs at some point in time. It could be a state decode, the
> act of writing/reading a particular address, a FIFO being empty,
> etc. This decoding of the event desired is done outside TPDM.
> DSB subunit need to be configured in enablement and disablement.
> A struct that specifics associated to dsb dataset is needed. It
> saves the configuration and parameters of the dsb datasets. This
> change is to add this struct and initialize the configuration of
> DSB subunit.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpdm.c | 58 +++++++++++++++++++++++++---
>   drivers/hwtracing/coresight/coresight-tpdm.h | 17 ++++++++
>   2 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index f4854af..5e1e2ba 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -20,17 +20,59 @@
>   
>   DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
>   
> +static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)


> +{
> +	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
> +		if (!drvdata->dsb) {
> +			drvdata->dsb = devm_kzalloc(drvdata->dev,
> +						    sizeof(*drvdata->dsb), GFP_KERNEL);
> +			if (!drvdata->dsb)
> +				return -ENOMEM;

Please do not club init/allocation of datasets to "resetting" the
datasets. Why don't we move the allocation to tpmd_datasets_setup() ?
And this function could simply be called :

tpdm_reset_datasets()

and can be called from the tpdm_datasets_setup() too.


> +		} else
> +			memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
> +
> +		drvdata->dsb->trig_ts = true;
> +		drvdata->dsb->trig_type = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
> +{
> +	if (drvdata->dsb->trig_type)
> +		*val |= TPDM_DSB_CR_TRIG_TYPE;
> +	else
> +		*val &= ~TPDM_DSB_CR_TRIG_TYPE;
> +}
> +

Do we really need a function for this ? How is it different from trig_ts ?

>   static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   {
>   	u32 val;
>   
> -	/* Set the enable bit of DSB control register to 1 */
> +	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
> +	/* Set trigger timestamp */
> +	if (drvdata->dsb->trig_ts)
> +		val |= TPDM_DSB_TIER_XTRIG_TSENAB;
> +	else
> +		val &= ~TPDM_DSB_TIER_XTRIG_TSENAB;
> +	writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
> +
>   	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
> +	/* Set trigger type */
> +	set_trigger_type(drvdata, &val);
> +	/* Set the enable bit of DSB control register to 1 */
>   	val |= TPDM_DSB_CR_ENA;
>   	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>   }
>   
>   /* TPDM enable operations */
> +/* The TPDM or Monitor serves as data collection component for various
> + * dataset types. It covers Basic Counts(BC), Tenure Counts(TC),
> + * Continuous Multi-Bit(CMB), Multi-lane CMB(MCMB) and Discrete Single
> + * Bit(DSB). This function will initialize the configuration according
> + * to the dataset type supported by the TPDM.
> + */
>   static void __tpdm_enable(struct tpdm_drvdata *drvdata)
>   {
>   	CS_UNLOCK(drvdata->base);
> @@ -110,15 +152,13 @@ static const struct coresight_ops tpdm_cs_ops = {
>   	.source_ops	= &tpdm_source_ops,
>   };
>   
> -static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
> +static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
>   {
>   	u32 pidr;
>   
> -	CS_UNLOCK(drvdata->base);

Why is this removed ?

>   	/*  Get the datasets present on the TPDM. */
>   	pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
>   	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
> -	CS_LOCK(drvdata->base);
>   }
>   
>   /*
> @@ -181,6 +221,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   	struct coresight_platform_data *pdata;
>   	struct tpdm_drvdata *drvdata;
>   	struct coresight_desc desc = { 0 };
> +	int ret;
>   
>   	pdata = coresight_get_platform_data(dev);
>   	if (IS_ERR(pdata))
> @@ -200,6 +241,8 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   
>   	drvdata->base = base;
>   
> +	tpdm_datasets_setup(drvdata);
> +
>   	/* Set up coresight component description */
>   	desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>   	if (!desc.name)
> @@ -216,7 +259,12 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   		return PTR_ERR(drvdata->csdev);
>   
>   	spin_lock_init(&drvdata->spinlock);
> -	tpdm_init_default_data(drvdata);
> +	ret = tpdm_init_datasets(drvdata);

Couldn't this be done before the coresight_register() ? As such
I don't see any dependency on having a csdev ?

Suzuki

Re: [PATCH v3 04/11] coresight-tpdm: Add reset node to TPDM node

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> TPDM device need a node to reset the configurations and status of
> it. This change provides a node to reset the configurations and
> disable the TPDM if it has been enabled.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 5e1e2ba..104638d 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
>   	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>   }
>   
> +static ssize_t reset_store(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf,
> +					  size_t size)
> +{
> +	int ret = 0;
> +	unsigned long val;
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret || val != 1)
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	/* Reset all datasets to ZERO, and init the default data*/
> +	tpdm_init_datasets(drvdata);

With the suggested rename in the previous patch, you wouldn't need
a comment here.

> +
> +	spin_unlock(&drvdata->spinlock);
> +


> +	/* Disable tpdm if enabled */
> +	if (drvdata->enable)
> +		coresight_disable(drvdata->csdev);

Couldn't this be done via disable_source ? Please don't overload
the sysfs handle.

> +
> +	return size;
> +}
> +static DEVICE_ATTR_WO(reset);

Documentation for the sysfs node please ?

Suzuki

Re: [PATCH v3 04/11] coresight-tpdm: Add reset node to TPDM node

[Suzuki K Poulose]

On 23/03/2023 14:41, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> TPDM device need a node to reset the configurations and status of
>> it. This change provides a node to reset the configurations and
>> disable the TPDM if it has been enabled.

Please justify why this "do everything" magic knob is required
when there are tunables for individual controls in the later
patches.

Suzuki

>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 
>> ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 5e1e2ba..104638d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct 
>> tpdm_drvdata *drvdata)
>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>>   }
>> +static ssize_t reset_store(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      const char *buf,
>> +                      size_t size)
>> +{
>> +    int ret = 0;
>> +    unsigned long val;
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    ret = kstrtoul(buf, 10, &val);
>> +    if (ret || val != 1)
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    /* Reset all datasets to ZERO, and init the default data*/
>> +    tpdm_init_datasets(drvdata);
> 
> With the suggested rename in the previous patch, you wouldn't need
> a comment here.
> 
>> +
>> +    spin_unlock(&drvdata->spinlock);
>> +
> 
> 
>> +    /* Disable tpdm if enabled */
>> +    if (drvdata->enable)
>> +        coresight_disable(drvdata->csdev);
> 
> Couldn't this be done via disable_source ? Please don't overload
> the sysfs handle.
> 
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_WO(reset);
> 
> Documentation for the sysfs node please ?
> 
> Suzuki
> 

Re: [PATCH v3 06/11] coresight-tpdm: Add node to set dsb programming mode

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> Add node to set and show programming mode for TPDM DSB subunit.
> Once the DSB programming mode is set, it will be written to the
> register DSB_CR. Bit[10:9] of the DSB_CR register is used to set
> the DSB test mode.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 10 ++++
>   drivers/hwtracing/coresight/coresight-tpdm.c       | 62 ++++++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpdm.h       | 13 +++++
>   3 files changed, 85 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index 27ce681..f13e282 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -35,3 +35,13 @@ Description:
>   		Accepts only one of the 2 values -  0 or 1.
>   		0 : Set the DSB trigger type to false
>   		1 : Set the DSB trigger type to true
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_mode
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the mode of DSB tpdm. Read the mode of DSB
> +		tpdm.
> +
> +		Accepts the value needs to be greater than 0.

Please could you document the values ?

> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index e28cf10..8cd822f 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -4,6 +4,7 @@
>    */
>   
>   #include <linux/amba/bus.h>
> +#include <linux/bitfield.h>
>   #include <linux/bitmap.h>
>   #include <linux/coresight.h>
>   #include <linux/coresight-pmu.h>
> @@ -51,6 +52,32 @@ static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
>   	return 0;
>   }
>   
> +static void set_dsb_cycacc_mode(struct tpdm_drvdata *drvdata, u32 *val)
> +{
> +	u32 mode;
> +
> +	mode = TPDM_DSB_MODE_CYCACC(drvdata->dsb->mode);
> +	*val &= ~TPDM_DSB_TEST_MODE;
> +	*val |= FIELD_PREP(TPDM_DSB_TEST_MODE, mode);
> +}
> +
> +static void set_dsb_hpsel_mode(struct tpdm_drvdata *drvdata, u32 *val)
> +{
> +	u32 mode;
> +
> +	mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode);
> +	*val &= ~TPDM_DSB_HPSEL;
> +	*val |= FIELD_PREP(TPDM_DSB_HPSEL, mode);
> +}
> +
> +static void set_dsb_perf_mode(struct tpdm_drvdata *drvdata, u32 *val)
> +{
> +	if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF)
> +		*val |= TPDM_DSB_CR_MODE;
> +	else
> +		*val &= ~TPDM_DSB_CR_MODE;
> +}
> +
>   static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
>   {
>   	if (drvdata->dsb->trig_type)
> @@ -72,6 +99,12 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   	writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
>   
>   	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
> +	/* Set the cycle accurate mode */
> +	set_dsb_cycacc_mode(drvdata, &val);
> +	/* Set the byte lane for high-performance mode */
> +	set_dsb_hpsel_mode(drvdata, &val);
> +	/* Set the performance mode */
> +	set_dsb_perf_mode(drvdata, &val);
>   	/* Set trigger type */
>   	set_trigger_type(drvdata, &val);
>   	/* Set the enable bit of DSB control register to 1 */
> @@ -250,6 +283,34 @@ static struct attribute_group tpdm_attr_grp = {
>   	.attrs = tpdm_attrs,
>   };
>   
> +static ssize_t dsb_mode_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%lx\n",
> +			 (unsigned long)drvdata->dsb->mode);
> +}
> +
> +static ssize_t dsb_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 val;
> +
> +	if ((kstrtoul(buf, 0, &val)) || val < 0)
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	drvdata->dsb->mode = val & TPDM_MODE_ALL;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_mode);
> +
>   static ssize_t dsb_trig_type_show(struct device *dev,
>   				     struct device_attribute *attr,
>   				     char *buf)
> @@ -321,6 +382,7 @@ static ssize_t dsb_trig_ts_store(struct device *dev,
>   static DEVICE_ATTR_RW(dsb_trig_ts);
>   
>   static struct attribute *tpdm_dsb_attrs[] = {
> +	&dev_attr_dsb_mode.attr,
>   	&dev_attr_dsb_trig_ts.attr,
>   	&dev_attr_dsb_trig_type.attr,
>   	NULL,
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 68f33bd..8fee562 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -15,11 +15,22 @@
>   
>   /* Enable bit for DSB subunit */
>   #define TPDM_DSB_CR_ENA		BIT(0)
> +/* Enable bit for DSB subunit perfmance mode */
> +#define TPDM_DSB_CR_MODE		BIT(1)
>   /* Enable bit for DSB subunit trigger type */
>   #define TPDM_DSB_CR_TRIG_TYPE		BIT(12)
> +
>   /* Enable bit for DSB subunit trigger timestamp */
>   #define TPDM_DSB_TIER_XTRIG_TSENAB		BIT(1)
>   
> +/* DSB programming modes */
> +#define TPDM_DSB_MODE_CYCACC(val)	(val & GENMASK(2, 0))

What do the values for the CYCACC mode mean ?

> +#define TPDM_DSB_MODE_PERF		BIT(3)
> +#define TPDM_DSB_MODE_HPBYTESEL(val)	(val & GENMASK(8, 4))
> +#define TPDM_MODE_ALL			(0xFFFFFFF)
> +#define TPDM_DSB_TEST_MODE		GENMASK(11, 9)
> +#define TPDM_DSB_HPSEL		GENMASK(6, 2)

Again what do the values mean ? Even if the kernel doesn't use them
it would be good to document it.

Suzuki

> +
>   /* TPDM integration test registers */
>   #define TPDM_ITATBCNTRL		(0xEF0)
>   #define TPDM_ITCNTRL		(0xF00)
> @@ -48,10 +59,12 @@
>   
>   /**
>    * struct dsb_dataset - specifics associated to dsb dataset
> + * @mode:             DSB programming mode
>    * @trig_ts:          Enable/Disable trigger timestamp.
>    * @trig_type:        Enable/Disable trigger type.
>    */
>   struct dsb_dataset {
> +	u32				mode;
>   	bool			trig_ts;
>   	bool			trig_type;
>   };

Re: [PATCH v3 07/11] coresight-tpdm: Add nodes for dsb edge control

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> Add the nodes to set value for DSB edge control and DSB edge
> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
> resgisters to configure edge control. DSB edge detection control
> 00: Rising edge detection
> 01: Falling edge detection
> 10: Rising and falling edge detection (toggle detection) > And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
> configure mask. Eight 32 bit registers providing DSB interface
> edge detection mask control.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  26 ++++
>   drivers/hwtracing/coresight/coresight-tpdm.c       | 142 ++++++++++++++++++++-
>   drivers/hwtracing/coresight/coresight-tpdm.h       |  14 ++
>   3 files changed, 181 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index f13e282..094d624 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -45,3 +45,29 @@ Description:
>   		tpdm.
>   
>   		Accepts the value needs to be greater than 0.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the edge control of DSB tpdm. Read the
> +		edge control of DSB tpdm.

Could we not say :
	Read / Write a set of the edge control registers of the DSB in
         TPDM

> +
> +		Accepts the following three values.

This is a bit confusing, at least to me. This could either mean,
a single write of values 1-3 are accepted.

> +		value 1: Start EDCR register number
> +		value 2: End EDCR register number
> +		value 3: The value need to be written

But you really mean to say, the writes must be of the following format
	<integer1> <integer2> <integer3>

where :
		<integer1> : Start EDCR ...
		....
	
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the edge control mask of DSB tpdm. Read
> +		the edge control mask of DSB tpdm.
> +
> +		Accepts the following three values.
> +		value 1: Start EDCMR register number
> +		value 2: End EDCMR register number
> +		value 3: The value need to be written

Similarly here.

> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 8cd822f..2a0b36c 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -88,7 +88,14 @@ static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
>   
>   static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   {
> -	u32 val;
> +	u32 val, i;
> +
> +	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++)
> +		writel_relaxed(drvdata->dsb->edge_ctrl[i],
> +			   drvdata->base + TPDM_DSB_EDCR(i));
> +	for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++)
> +		writel_relaxed(drvdata->dsb->edge_ctrl_mask[i],
> +			   drvdata->base + TPDM_DSB_EDCMR(i));
>   
>   	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
>   	/* Set trigger timestamp */
> @@ -311,6 +318,137 @@ static ssize_t dsb_mode_store(struct device *dev,
>   }
>   static DEVICE_ATTR_RW(dsb_mode);
>   
> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
> +	int i;
> +
> +	spin_lock(&drvdata->spinlock);
> +	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
> +		size += sysfs_emit_at(buf, size,
> +				  "Index:0x%x Val:0x%x\n", i,
> +				  drvdata->dsb->edge_ctrl[i]);
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +
> +/*
> + * value 1: Start EDCR register number
> + * value 2: End EDCR register number
> + * value 3: The value need to be written
> + * The EDCR registers can include up to 16 32-bit registers, and each
> + * one can be configured to control up to 16 edge detections(2 bits
> + * control one edge detection). So a total 256 edge detections can be
> + * configured. So the starting number(value 1) and ending number(value 2)
> + * cannot be greater than 256, and value 1 should be less than value 2.
> + * The following values are the rage of value 3.
> + * 0 - Rising edge detection
> + * 1 - Falling edge detection
> + * 2 - Rising and falling edge detection (toggle detection)
> + */
> +static ssize_t dsb_edge_ctrl_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 start, end, edge_ctrl;
> +	uint32_t val;
> +	int i, index, bit, reg;
> +
> +	if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3)
> +		return -EINVAL;
> +	if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) ||
> +	    edge_ctrl > 0x2)
> +		return -EPERM;

	Isn't it an error to provide start > end ?

> +
> +	spin_lock(&drvdata->spinlock);
> +	for (i = start; i <= end; i++) {
> +		/*
> +		 * The 32-bit register has 32 bits(NUM_OF_BITS).
> +		 * Each one register can be configured to control 16
> +		 * (NUM_OF_BITS / 2) edge detectioins.
> +		 */

		/*
		 * Each DSB Edge control line requires 2bits.
		 * Thus we have 16 lines in a 32bit word.
		 */

Could we please define something like:

#define EDCRS_PER_WORD			16
#define EDCR_TO_WORD_IDX(r)		((r) / EDCRS_PER_WORD)
#define EDCR_TO_WORD_SHIFT(r)		((r) % EDCRS_PER_WORD)
#define EDCR_TO_WORD_MASK(r)		(0x3 << EDCR_TO_WORD_SHIFT((r)))

> +		reg = i / (NUM_OF_BITS / 2);
> +		index = i % (NUM_OF_BITS / 2);
> +		bit = index * 2;
> +

	reg = EDCR_TO_WORD_IDX(i);
	mask = EDCR_TO_WORD_MASK(i);
	
	val &= ~mask;
	val |= FIELD_PREP(mask, edge_ctrl);

> +		val = drvdata->dsb->edge_ctrl[reg];
> +		val &= ~GENMASK((bit + 1), bit);
> +		val |= (edge_ctrl << bit);


> +		drvdata->dsb->edge_ctrl[reg] = val;
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_edge_ctrl);
> +
> +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
> +	int i;
> +
> +	spin_lock(&drvdata->spinlock);
> +	for (i = 0; i < TPDM_DSB_MAX_EDCR / 2; i++) {

Why is this not `i < TPDM_DSB_MAX_EDCMR` ?

> +		size += sysfs_emit_at(buf, size,
> +				  "Index:0x%x Val:0x%x\n", i,
> +				  drvdata->dsb->edge_ctrl_mask[i]);
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +
> +/*
> + * value 1: Start EDCMR register number
> + * value 2: End EDCMR register number
> + * value 3: The value need to be written
> + */
> +static ssize_t dsb_edge_ctrl_mask_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 start, end, val;
> +	u32 set;
> +	int i, index, reg;
> +
> +	if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3)
> +		return -EINVAL;
> +	if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES)
> +		|| (val & ~1UL))
> +		return -EPERM;
> +
> +	spin_lock(&drvdata->spinlock);
> +	for (i = start; i <= end; i++) {
> +		/*
> +		 * The 32-bit register has 32 bits(NUM_OF_BITS).
> +		 * Each one register can be configured to control 32
> +		 * (NUM_OF_BITS) edge detectioin masks.

minor nit: detection.

You could simply say:
		/*
		 * There is 1 bit per DSB Edge Control line.
		 * Thus we have 32 lines in a 32bit word.
		 */
	 	reg = i / 32;

> +		reg = i / NUM_OF_BITS;
> +		index = (i % NUM_OF_BITS);
> +
> +		set = drvdata->dsb->edge_ctrl_mask[reg];
> +		if (val)
> +			set |= BIT(index);
> +		else
> +			set &= ~BIT(index);
> +		drvdata->dsb->edge_ctrl_mask[reg] = set;
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
> +
>   static ssize_t dsb_trig_type_show(struct device *dev,
>   				     struct device_attribute *attr,
>   				     char *buf)
> @@ -383,6 +521,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts);
>   
>   static struct attribute *tpdm_dsb_attrs[] = {
>   	&dev_attr_dsb_mode.attr,
> +	&dev_attr_dsb_edge_ctrl.attr,
> +	&dev_attr_dsb_edge_ctrl_mask.attr,
>   	&dev_attr_dsb_trig_ts.attr,
>   	&dev_attr_dsb_trig_type.attr,
>   	NULL,
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 8fee562..342ef23 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -12,6 +12,8 @@
>   /* DSB Subunit Registers */
>   #define TPDM_DSB_CR		(0x780)
>   #define TPDM_DSB_TIER		(0x784)
> +#define TPDM_DSB_EDCR(n)	(0x808 + (n * 4))
> +#define TPDM_DSB_EDCMR(n)	(0x848 + (n * 4))
>   
>   /* Enable bit for DSB subunit */
>   #define TPDM_DSB_CR_ENA		BIT(0)
> @@ -31,6 +33,8 @@
>   #define TPDM_DSB_TEST_MODE		GENMASK(11, 9)
>   #define TPDM_DSB_HPSEL		GENMASK(6, 2)
>   
> +#define NUM_OF_BITS		32

Please don't use such generic names. Instead define TPDM specific
helper definitions. See above.


Suzuki


> +
>   /* TPDM integration test registers */
>   #define TPDM_ITATBCNTRL		(0xEF0)
>   #define TPDM_ITCNTRL		(0xF00)
> @@ -57,14 +61,24 @@
>   #define TPDM_PIDR0_DS_IMPDEF	BIT(0)
>   #define TPDM_PIDR0_DS_DSB	BIT(1)
>   
> +#define TPDM_DSB_MAX_LINES	256
> +/* MAX number of EDCR registers */
> +#define TPDM_DSB_MAX_EDCR	16 > +/* MAX number of EDCMR registers */
> +#define TPDM_DSB_MAX_EDCMR	8
> +
>   /**
>    * struct dsb_dataset - specifics associated to dsb dataset
>    * @mode:             DSB programming mode
> + * @edge_ctrl:        Save value for edge control
> + * @edge_ctrl_mask:   Save value for edge control mask
>    * @trig_ts:          Enable/Disable trigger timestamp.
>    * @trig_type:        Enable/Disable trigger type.
>    */
>   struct dsb_dataset {
>   	u32				mode;
> +	u32				edge_ctrl[TPDM_DSB_MAX_EDCR];
> +	u32				edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
>   	bool			trig_ts;
>   	bool			trig_type;
>   };

Re: [PATCH v3 08/11] coresight-tpdm: Add nodes to configure pattern match output

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> Add nodes to configure trigger pattern and trigger pattern mask.
> Each DSB subunit TPDM has maximum of n(n<7) XPR registers to
> configure trigger pattern match output. Eight 32 bit registers
> providing DSB interface trigger output pattern match comparison.
> And each DSB subunit TPDM has maximum of m(m<7) XPMR registers to
> configure trigger pattern mask match output. Eight 32 bit
> registers providing DSB interface trigger output pattern match
> mask.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 24 +++++++
>   drivers/hwtracing/coresight/coresight-tpdm.c       | 84 ++++++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpdm.h       |  8 +++
>   3 files changed, 116 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index 094d624..c06374f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -71,3 +71,27 @@ Description:
>   		value 1: Start EDCMR register number
>   		value 2: End EDCMR register number
>   		value 3: The value need to be written
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_trig_patt_val
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the trigger pattern value of DSB tpdm.
> +		Read the trigger pattern value of DSB tpdm.
> +
> +		Accepts the following two values.
> +		value 1: Index number of XPR register
> +		value 2: The value need to be written

minor nit: What values are acceptable ? Otherwise looks fine.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH v3 09/11] coresight-tpdm: Add nodes for timestamp request

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> Add nodes to configure the timestamp request based on input
> pattern match. Each TPDM that support DSB subunit has maximum of
> n(n<7) TPR registers to configure value for timestamp request
> based on input pattern match. Eight 32 bit registers providing
> DSB interface timestamp request  pattern match comparison. And
> each TPDM that support DSB subunit has maximum of m(m<7) TPMR
> registers to configure pattern mask for timestamp request. Eight
> 32 bit registers providing DSB interface timestamp request
> pattern match mask generation. Add nodes to enable/disable
> pattern timestamp and set pattern timestamp type.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  48 ++++++
>   drivers/hwtracing/coresight/coresight-tpdm.c       | 172 +++++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpdm.h       |  14 ++
>   3 files changed, 234 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index c06374f..60ff660 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -95,3 +95,51 @@ Description:
>   		Accepts the following two values.
>   		value 1: Index number of XPMR register
>   		value 2: The value need to be written
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_val
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the pattern value of DSB tpdm. Read
> +		the pattern value of DSB tpdm.
> +
> +		Accepts the following two values.
> +		value 1: Index number of TPR register
> +		value 2: The value need to be written
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_mask
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the pattern mask of DSB tpdm. Read
> +		the pattern mask of DSB tpdm.
> +
> +		Accepts the following two values.
> +		value 1: Index number of TPMR register
> +		value 2: The value need to be written
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_ts
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the pattern timestamp of DSB tpdm. Read
> +		the pattern timestamp of DSB tpdm.
> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Set the DSB pattern timestamp to false
> +		1 : Set the DSB pattern timestamp to true
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_patt_type
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the pattern type of DSB tpdm. Read
> +		the pattern type of DSB tpdm.
> +

Sounds a bit strange for "pattern type" to be bool. What does this do ?
Does it enable/disable something  ?

> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Set the DSB pattern type to false
> +		1 : Set the DSB pattern type to true
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index d6cc6b5..c740681 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -98,6 +98,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   			   drvdata->base + TPDM_DSB_EDCMR(i));
>   
>   	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
> +		writel_relaxed(drvdata->dsb->patt_val[i],
> +			    drvdata->base + TPDM_DSB_TPR(i));
> +		writel_relaxed(drvdata->dsb->patt_mask[i],
> +			    drvdata->base + TPDM_DSB_TPMR(i));
> +	}
> +
> +	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {

Why not do all the pattern related writing in one shot, instead
of two loops ?

>   		writel_relaxed(drvdata->dsb->trig_patt_val[i],
>   			    drvdata->base + TPDM_DSB_XPR(i));
>   		writel_relaxed(drvdata->dsb->trig_patt_mask[i],
> @@ -105,6 +112,16 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   	}
>   
>   	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
> +	/* Set pattern timestamp type and enablement */
> +	if (drvdata->dsb->patt_ts) {
> +		val |= TPDM_DSB_TIER_PATT_TSENAB;
> +		if (drvdata->dsb->patt_type)
> +			val |= TPDM_DSB_TIER_PATT_TYPE;
> +		else
> +			val &= ~TPDM_DSB_TIER_PATT_TYPE;
> +	} else {
> +		val &= ~TPDM_DSB_TIER_PATT_TSENAB;
> +	}

set_dsb_pattern_ts() in line with the other helper functions ?
Rest looks fine to me.

Suzuki


>   	/* Set trigger timestamp */
>   	if (drvdata->dsb->trig_ts)
>   		val |= TPDM_DSB_TIER_XTRIG_TSENAB;
> @@ -455,6 +472,157 @@ static ssize_t dsb_edge_ctrl_mask_store(struct device *dev,
>   	return size;
>   }
>   static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
> +
> +static ssize_t dsb_patt_val_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
> +	int i = 0;
> +
> +	spin_lock(&drvdata->spinlock);
> +	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
> +		size += sysfs_emit_at(buf, size,
> +				  "Index: 0x%x Value: 0x%x\n", i,
> +				  drvdata->dsb->patt_val[i]);
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +
> +/*
> + * value 1: Index of TPR register
> + * value 2: Value need to be written
> + */
> +static ssize_t dsb_patt_val_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 index, val;
> +
> +	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
> +		return -EINVAL;
> +	if (index >= TPDM_DSB_MAX_PATT)
> +		return -EPERM;
> +
> +	spin_lock(&drvdata->spinlock);
> +	drvdata->dsb->patt_val[index] = val;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_patt_val);
> +
> +static ssize_t dsb_patt_mask_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
> +	int i = 0;
> +
> +	spin_lock(&drvdata->spinlock);
> +	for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
> +		size += sysfs_emit_at(buf, size,
> +				  "Index: 0x%x Value: 0x%x\n", i,
> +				  drvdata->dsb->patt_mask[i]);
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +
> +/*
> + * value 1: Index of TPMR register
> + * value 2: Value need to be written
> + */
> +static ssize_t dsb_patt_mask_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 index, val;
> +
> +	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
> +		return -EINVAL;
> +	if (index >= TPDM_DSB_MAX_PATT)
> +		return -EPERM;
> +
> +	spin_lock(&drvdata->spinlock);
> +	drvdata->dsb->patt_mask[index] = val;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_patt_mask);
> +
> +static ssize_t dsb_patt_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->dsb->patt_ts);
> +}
> +
> +/*
> + * value 1: Enable/Disable DSB pattern timestamp
> + */

> +static ssize_t dsb_patt_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->dsb->patt_ts = true;
> +	else
> +		drvdata->dsb->patt_ts = false;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_patt_ts);
> +
> +static ssize_t dsb_patt_type_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->dsb->patt_type);
> +}
> +
> +/*
> + * value 1: Set DSB pattern type


> + */ > +static ssize_t dsb_patt_type_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->dsb->patt_type = true;
> +	else
> +		drvdata->dsb->patt_type = false;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_patt_type);
> +
>   static ssize_t dsb_trig_patt_val_show(struct device *dev,
>   					   struct device_attribute *attr,
>   					   char *buf)
> @@ -605,6 +773,10 @@ static struct attribute *tpdm_dsb_attrs[] = {
>   	&dev_attr_dsb_mode.attr,
>   	&dev_attr_dsb_edge_ctrl.attr,
>   	&dev_attr_dsb_edge_ctrl_mask.attr,
> +	&dev_attr_dsb_patt_val.attr,
> +	&dev_attr_dsb_patt_mask.attr,
> +	&dev_attr_dsb_patt_ts.attr,
> +	&dev_attr_dsb_patt_type.attr,
>   	&dev_attr_dsb_trig_patt_val.attr,
>   	&dev_attr_dsb_trig_patt_mask.attr,
>   	&dev_attr_dsb_trig_ts.attr,
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 2e8020e..f9d4dd9 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -12,6 +12,8 @@
>   /* DSB Subunit Registers */
>   #define TPDM_DSB_CR		(0x780)
>   #define TPDM_DSB_TIER		(0x784)
> +#define TPDM_DSB_TPR(n)		(0x788 + (n * 4))
> +#define TPDM_DSB_TPMR(n)	(0x7A8 + (n * 4))
>   #define TPDM_DSB_XPR(n)		(0x7C8 + (n * 4))
>   #define TPDM_DSB_XPMR(n)	(0x7E8 + (n * 4))
>   #define TPDM_DSB_EDCR(n)	(0x808 + (n * 4))
> @@ -24,8 +26,12 @@
>   /* Enable bit for DSB subunit trigger type */
>   #define TPDM_DSB_CR_TRIG_TYPE		BIT(12)
>   
> +/* Enable bit for DSB subunit pattern timestamp */
> +#define TPDM_DSB_TIER_PATT_TSENAB		BIT(0)
>   /* Enable bit for DSB subunit trigger timestamp */
>   #define TPDM_DSB_TIER_XTRIG_TSENAB		BIT(1)
> +/* Bit for DSB subunit pattern type */
> +#define TPDM_DSB_TIER_PATT_TYPE		BIT(2)
>   
>   /* DSB programming modes */
>   #define TPDM_DSB_MODE_CYCACC(val)	(val & GENMASK(2, 0))
> @@ -76,6 +82,10 @@
>    * @mode:             DSB programming mode
>    * @edge_ctrl:        Save value for edge control
>    * @edge_ctrl_mask:   Save value for edge control mask
> + * @patt_val:         Save value for pattern
> + * @patt_mask:        Save value for pattern mask
> + * @patt_ts:          Enable/Disable pattern timestamp
> + * @patt_type:        Set pattern type
>    * @trig_patt_val:    Save value for trigger pattern
>    * @trig_patt_mask:   Save value for trigger pattern mask
>    * @trig_ts:          Enable/Disable trigger timestamp.
> @@ -85,6 +95,10 @@ struct dsb_dataset {
>   	u32				mode;
>   	u32				edge_ctrl[TPDM_DSB_MAX_EDCR];
>   	u32				edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
> +	u32				patt_val[TPDM_DSB_MAX_PATT];
> +	u32				patt_mask[TPDM_DSB_MAX_PATT];
> +	bool			patt_ts;
> +	bool			patt_type;
>   	u32				trig_patt_val[TPDM_DSB_MAX_PATT];
>   	u32				trig_patt_mask[TPDM_DSB_MAX_PATT];
>   	bool			trig_ts;

Re: [PATCH v3 10/11] dt-bindings: arm: Add support for DSB MSR register

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> Add property "qcom,dsb_msr_num" to support DSB(Discrete Single
> Bit) MSR(mux select register) for TPDM. It specifies the number
> of MSR registers supported by the DSB TDPM.
> 
> Signed-off-by: Tao Zhang <quic_taozha@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 d9b6b613..691c7ba 100644
> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> @@ -53,6 +53,15 @@ properties:
>       minimum: 32
>       maximum: 64
>   
> +  qcom,dsb_msr_num:

nit: "qcom,dsb-msr-num" ? or even "qcom,dsb-num-msrs" ?

Suzuki

Re: [PATCH v3 01/11] dt-bindings: arm: Add support for DSB element size

[Tao Zhang]

Hi Suzuki,

在 3/23/2023 7:18 PM, Suzuki K Poulose 写道:
> On 23/03/2023 06:03, Tao Zhang wrote:
>> Add property "qcom,dsb-elem-size" to support DSB(Discrete Single
>> Bit) element for TPDM. The associated aggregator will read this
>> size before it is enabled. DSB element size currently only
>> supports 32-bit and 64-bit.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   .../devicetree/bindings/arm/qcom,coresight-tpdm.yaml | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml 
>> b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> index 5c08342..d9b6b613 100644
>> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>> @@ -44,6 +44,15 @@ properties:
>>       minItems: 1
>>       maxItems: 2
>
>
>>   +  qcom,dsb-element-size:
>> +    description:
>> +      Specifies the DSB(Discrete Single Bit) element size supported by
>> +      the monitor. The associated aggregator will read this size 
>> before it
>> +      is enabled. DSB element size currently only supports 32-bit 
>> and 64-bit.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 32
>> +    maximum: 64
>
> Shouldn't this be something like oneOf ? It is not a range, but one of
> those two specific values ?

Yes, "qcom,dsb-element-size" should be an optional option required in TPDM

devicetree. Other properties like "qcom,cmb-element-size", 
"qcom,tc-element-size"

and etc. will be added in a later patch series.

I will update this doc according to your advice in the next version of 
the patch.

Tao

>
> Suzuki
>
>

Re: [PATCH v3 01/11] dt-bindings: arm: Add support for DSB element size

[Tao Zhang]

Hi Suzuki,

在 3/24/2023 4:25 PM, Tao Zhang 写道:
> Hi Suzuki,
>
> 在 3/23/2023 7:18 PM, Suzuki K Poulose 写道:
>> On 23/03/2023 06:03, Tao Zhang wrote:
>>> Add property "qcom,dsb-elem-size" to support DSB(Discrete Single
>>> Bit) element for TPDM. The associated aggregator will read this
>>> size before it is enabled. DSB element size currently only
>>> supports 32-bit and 64-bit.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/arm/qcom,coresight-tpdm.yaml | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml 
>>> b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>>> index 5c08342..d9b6b613 100644
>>> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
>>> @@ -44,6 +44,15 @@ properties:
>>>       minItems: 1
>>>       maxItems: 2
>>
>>
>>>   +  qcom,dsb-element-size:
>>> +    description:
>>> +      Specifies the DSB(Discrete Single Bit) element size supported by
>>> +      the monitor. The associated aggregator will read this size 
>>> before it
>>> +      is enabled. DSB element size currently only supports 32-bit 
>>> and 64-bit.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 32
>>> +    maximum: 64
>>
>> Shouldn't this be something like oneOf ? It is not a range, but one of
>> those two specific values ?
>
> Yes, "qcom,dsb-element-size" should be an optional option required in 
> TPDM
>
> devicetree. Other properties like "qcom,cmb-element-size", 
> "qcom,tc-element-size"
>
> and etc. will be added in a later patch series.
>
> I will update this doc according to your advice in the next version of 
> the patch.
>
> Tao
>
Correct my misunderstanding in the mail above.

You are right, DSB element size should be 32-bit or 64-bit. I will 
update this in the next

patch series.


Tao

>>
>> Suzuki
>>
>>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v3 01/11] dt-bindings: arm: Add support for DSB element size

[Krzysztof Kozlowski]

On 24/03/2023 10:15, Tao Zhang wrote:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    minimum: 32
>>>> +    maximum: 64
>>>
>>> Shouldn't this be something like oneOf ? It is not a range, but one of
>>> those two specific values ?
>>
>> Yes, "qcom,dsb-element-size" should be an optional option required in 
>> TPDM
>>
>> devicetree. Other properties like "qcom,cmb-element-size", 
>> "qcom,tc-element-size"
>>
>> and etc. will be added in a later patch series.
>>
>> I will update this doc according to your advice in the next version of 
>> the patch.
>>
>> Tao
>>
> Correct my misunderstanding in the mail above.
> 
> You are right, DSB element size should be 32-bit or 64-bit. I will 
> update this in the next

Then 'enum', not 'oneOf'.

Best regards,
Krzysztof

Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Suzuki K Poulose]

On 24/03/2023 14:58, Tao Zhang wrote:
> Hi Suzuki,
> 
> 在 3/23/2023 7:51 PM, Suzuki K Poulose 写道:
>> On 23/03/2023 06:03, Tao Zhang wrote:
>>> Read the DSB element size from the device tree. Set the register
>>> bit that controls the DSB element size of the corresponding port.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-tpda.c | 58 
>>> ++++++++++++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>   2 files changed, 62 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>> index f712e11..8dcfc4a 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -21,6 +21,47 @@
>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>   +/* Search and read element data size from the TPDM node in
>>> + * the devicetree. Each input port of TPDA is connected to
>>> + * a TPDM. Different TPDM supports different types of dataset,
>>> + * and some may support more than one type of dataset.
>>> + * Parameter "inport" is used to pass in the input port number
>>> + * of TPDA, and it is set to 0 in the recursize call.
>>> + * Parameter "parent" is used to pass in the original call.
>>> + */
>>
>> I am still not clear why we need to do this recursively ?
> 
> Some TPDMs are not directly output connected to the TPDAs. So here I
> 
> use a recursive method to check from the TPDA input port until I find
> 
> the connected TPDM.
> 
> Do you have a better suggestion besides a recursive method?
> 
>>
>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>> +               struct coresight_device *csdev, int inport, bool parent)
>>
>> Please could we renamse csdev => tpda_dev
> 
> Since this is a recursively called function, this Coresight device is not
> 
> necessarily TPDA, it can be other Coresight device.
> 
>>
>>> +{
>>> +    static int nr_inport;
>>> +    int i;
>>> +    struct coresight_device *in_csdev;
>>
>> similarly tpdm_dev ?
> Same as above, this variable may not necessarily be a TPDM.
>>
>> Could we not add a check here to see if the dsb_esize[inport] is already
>> set and then bail out, reading this over and over ?
>>
> I will update this in the next patch series.
>>> +
>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>> +        return -EINVAL;
>>> +
>>> +    if (parent)
>>> +        nr_inport = inport;
>>> +
>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>> +        in_csdev = csdev->pdata->in_conns[i].remote_dev;
>>
>> Please note, the names of the structure field might change in the
>> next version of James' series
> Got it. I will keep an eye out for the James' patch series.
>>
>>> +        if (!in_csdev)
>>> +            break;
>>> +
>>> +        if (parent)
>>> +            if (csdev->pdata->in_conns[i].port != inport)
>>> +                continue;
>>> +
>>> +        if (in_csdev && strstr(dev_name(&in_csdev->dev), "tpdm")) {
>>
>> Isn't there a better way to distinguish a device to be TPDM ? May be we
>> could even add a source_sub_type - SOURCE_TPDM instead of using
>> SOURCE_OTHERS ? Do you expect other sources to be connected to TPDA?
>> e.g., STMs ?
> 
> I can add "SOURCE_TPDM" as a source_sub_type, but SOURCE_OTHERS needs
> 
> to be kept since the other Coresight component we will upstream later may
> 
> need it.
> 
>>
>>> + of_property_read_u32(in_csdev->dev.parent->of_node,
>>> +                    "qcom,dsb-element-size", 
>>> &drvdata->dsb_esize[nr_inport]);
>>> +            break;
>>> +        }
>>> +        tpda_set_element_size(drvdata, in_csdev, 0, false);
>>
>> What is the point of this ? Is this for covering the a TPDA connected to
>> another TPDA ?
>>
>> e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?
> 
> A TPDM may not connect to the TPDA directly, for example,
> 
> TPDM0 ->FUNNEL0->FUNNEL1->TPDA0
> 
> And many TPDMs can connect to one TPDA, one input port on TPDA only has
> 
> one TPDM connected. Therefore, we use a recursive method to find the TPDM
> 
> corresponding to the input port of TPDA.

How do you find out decide what to choose, if there are multiple TPDMs
connected to FUNNEL0 or even FUNNEL1 ?

e.g

TPDM0->FUNNEL0->FUNNEL1->TPDA0
                 /
           TPDM1
Suzuki

Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Tao Zhang]

On 3/26/2023 3:31 AM, Suzuki K Poulose wrote:
> On 24/03/2023 14:58, Tao Zhang wrote:
>> Hi Suzuki,
>>
>> 在 3/23/2023 7:51 PM, Suzuki K Poulose 写道:
>>> On 23/03/2023 06:03, Tao Zhang wrote:
>>>> Read the DSB element size from the device tree. Set the register
>>>> bit that controls the DSB element size of the corresponding port.
>>>>
>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 58 
>>>> ++++++++++++++++++++++++++++
>>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>>   2 files changed, 62 insertions(+)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> index f712e11..8dcfc4a 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -21,6 +21,47 @@
>>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>>   +/* Search and read element data size from the TPDM node in
>>>> + * the devicetree. Each input port of TPDA is connected to
>>>> + * a TPDM. Different TPDM supports different types of dataset,
>>>> + * and some may support more than one type of dataset.
>>>> + * Parameter "inport" is used to pass in the input port number
>>>> + * of TPDA, and it is set to 0 in the recursize call.
>>>> + * Parameter "parent" is used to pass in the original call.
>>>> + */
>>>
>>> I am still not clear why we need to do this recursively ?
>>
>> Some TPDMs are not directly output connected to the TPDAs. So here I
>>
>> use a recursive method to check from the TPDA input port until I find
>>
>> the connected TPDM.
>>
>> Do you have a better suggestion besides a recursive method?
>>
>>>
>>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>> +               struct coresight_device *csdev, int inport, bool 
>>>> parent)
>>>
>>> Please could we renamse csdev => tpda_dev
>>
>> Since this is a recursively called function, this Coresight device is 
>> not
>>
>> necessarily TPDA, it can be other Coresight device.
>>
>>>
>>>> +{
>>>> +    static int nr_inport;
>>>> +    int i;
>>>> +    struct coresight_device *in_csdev;
>>>
>>> similarly tpdm_dev ?
>> Same as above, this variable may not necessarily be a TPDM.
>>>
>>> Could we not add a check here to see if the dsb_esize[inport] is 
>>> already
>>> set and then bail out, reading this over and over ?
>>>
>> I will update this in the next patch series.
>>>> +
>>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (parent)
>>>> +        nr_inport = inport;
>>>> +
>>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>> +        in_csdev = csdev->pdata->in_conns[i].remote_dev;
>>>
>>> Please note, the names of the structure field might change in the
>>> next version of James' series
>> Got it. I will keep an eye out for the James' patch series.
>>>
>>>> +        if (!in_csdev)
>>>> +            break;
>>>> +
>>>> +        if (parent)
>>>> +            if (csdev->pdata->in_conns[i].port != inport)
>>>> +                continue;
>>>> +
>>>> +        if (in_csdev && strstr(dev_name(&in_csdev->dev), "tpdm")) {
>>>
>>> Isn't there a better way to distinguish a device to be TPDM ? May be we
>>> could even add a source_sub_type - SOURCE_TPDM instead of using
>>> SOURCE_OTHERS ? Do you expect other sources to be connected to TPDA?
>>> e.g., STMs ?
>>
>> I can add "SOURCE_TPDM" as a source_sub_type, but SOURCE_OTHERS needs
>>
>> to be kept since the other Coresight component we will upstream later 
>> may
>>
>> need it.
>>
>>>
>>>> + of_property_read_u32(in_csdev->dev.parent->of_node,
>>>> +                    "qcom,dsb-element-size", 
>>>> &drvdata->dsb_esize[nr_inport]);
>>>> +            break;
>>>> +        }
>>>> +        tpda_set_element_size(drvdata, in_csdev, 0, false);
>>>
>>> What is the point of this ? Is this for covering the a TPDA 
>>> connected to
>>> another TPDA ?
>>>
>>> e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?
>>
>> A TPDM may not connect to the TPDA directly, for example,
>>
>> TPDM0 ->FUNNEL0->FUNNEL1->TPDA0
>>
>> And many TPDMs can connect to one TPDA, one input port on TPDA only has
>>
>> one TPDM connected. Therefore, we use a recursive method to find the 
>> TPDM
>>
>> corresponding to the input port of TPDA.
>
> How do you find out decide what to choose, if there are multiple TPDMs
> connected to FUNNEL0 or even FUNNEL1 ?
>
> e.g
>
> TPDM0->FUNNEL0->FUNNEL1->TPDA0
>                 /
>           TPDM1

We can find out the corresponding TPDM by the input port number of TPDA.

Each input port is connected to a TPDM. So we have an input port number in

the input parameter of the recursive lookup function 
"tpda_set_element_size".

> Suzuki
>
>

Re: [PATCH v3 03/11] coresight-tpdm: Initialize DSB subunit configuration

[Tao Zhang]

Hi Suzuki,

On 3/23/2023 10:23 PM, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> DSB is used for monitoring “events”. Events are something that
>> occurs at some point in time. It could be a state decode, the
>> act of writing/reading a particular address, a FIFO being empty,
>> etc. This decoding of the event desired is done outside TPDM.
>> DSB subunit need to be configured in enablement and disablement.
>> A struct that specifics associated to dsb dataset is needed. It
>> saves the configuration and parameters of the dsb datasets. This
>> change is to add this struct and initialize the configuration of
>> DSB subunit.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 58 
>> +++++++++++++++++++++++++---
>>   drivers/hwtracing/coresight/coresight-tpdm.h | 17 ++++++++
>>   2 files changed, 70 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index f4854af..5e1e2ba 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -20,17 +20,59 @@
>>     DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
>>   +static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
>
>
>> +{
>> +    if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
>> +        if (!drvdata->dsb) {
>> +            drvdata->dsb = devm_kzalloc(drvdata->dev,
>> +                            sizeof(*drvdata->dsb), GFP_KERNEL);
>> +            if (!drvdata->dsb)
>> +                return -ENOMEM;
>
> Please do not club init/allocation of datasets to "resetting" the
> datasets. Why don't we move the allocation to tpmd_datasets_setup() ?
> And this function could simply be called :
>
> tpdm_reset_datasets()
>
> and can be called from the tpdm_datasets_setup() too.
>
I will update this in the next patch series.
>
>> +        } else
>> +            memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
>> +
>> +        drvdata->dsb->trig_ts = true;
>> +        drvdata->dsb->trig_type = false;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
>> +{
>> +    if (drvdata->dsb->trig_type)
>> +        *val |= TPDM_DSB_CR_TRIG_TYPE;
>> +    else
>> +        *val &= ~TPDM_DSB_CR_TRIG_TYPE;
>> +}
>> +
>
> Do we really need a function for this ? How is it different from 
> trig_ts ?

"trig_type" is for setting trigger type, and "trig_ts" is for setting 
trigger

timestamp. These two variables are configured in two different registers.

Do you mean we don't need to wrap it as a function here?

>
>>   static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>>   {
>>       u32 val;
>>   -    /* Set the enable bit of DSB control register to 1 */
>> +    val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
>> +    /* Set trigger timestamp */
>> +    if (drvdata->dsb->trig_ts)
>> +        val |= TPDM_DSB_TIER_XTRIG_TSENAB;
>> +    else
>> +        val &= ~TPDM_DSB_TIER_XTRIG_TSENAB;
>> +    writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
>> +
>>       val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>> +    /* Set trigger type */
>> +    set_trigger_type(drvdata, &val);
>> +    /* Set the enable bit of DSB control register to 1 */
>>       val |= TPDM_DSB_CR_ENA;
>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>   }
>>     /* TPDM enable operations */
>> +/* The TPDM or Monitor serves as data collection component for various
>> + * dataset types. It covers Basic Counts(BC), Tenure Counts(TC),
>> + * Continuous Multi-Bit(CMB), Multi-lane CMB(MCMB) and Discrete Single
>> + * Bit(DSB). This function will initialize the configuration according
>> + * to the dataset type supported by the TPDM.
>> + */
>>   static void __tpdm_enable(struct tpdm_drvdata *drvdata)
>>   {
>>       CS_UNLOCK(drvdata->base);
>> @@ -110,15 +152,13 @@ static const struct coresight_ops tpdm_cs_ops = {
>>       .source_ops    = &tpdm_source_ops,
>>   };
>>   -static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>> +static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
>>   {
>>       u32 pidr;
>>   -    CS_UNLOCK(drvdata->base);
>
> Why is this removed ?
We only need to read the register here, don't need to unlock the 
registers here, right?
>
>>       /*  Get the datasets present on the TPDM. */
>>       pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>> -    CS_LOCK(drvdata->base);
>>   }
>>     /*
>> @@ -181,6 +221,7 @@ static int tpdm_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>       struct coresight_platform_data *pdata;
>>       struct tpdm_drvdata *drvdata;
>>       struct coresight_desc desc = { 0 };
>> +    int ret;
>>         pdata = coresight_get_platform_data(dev);
>>       if (IS_ERR(pdata))
>> @@ -200,6 +241,8 @@ static int tpdm_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>         drvdata->base = base;
>>   +    tpdm_datasets_setup(drvdata);
>> +
>>       /* Set up coresight component description */
>>       desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>>       if (!desc.name)
>> @@ -216,7 +259,12 @@ static int tpdm_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>           return PTR_ERR(drvdata->csdev);
>>         spin_lock_init(&drvdata->spinlock);
>> -    tpdm_init_default_data(drvdata);
>> +    ret = tpdm_init_datasets(drvdata);
>
> Couldn't this be done before the coresight_register() ? As such
> I don't see any dependency on having a csdev ?

I will update this in the next patch series.


Tao

>
> Suzuki

Re: [PATCH v3 04/11] coresight-tpdm: Add reset node to TPDM node

[Tao Zhang]

Hi Suzuki,

On 3/23/2023 10:41 PM, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> TPDM device need a node to reset the configurations and status of
>> it. This change provides a node to reset the configurations and
>> disable the TPDM if it has been enabled.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 
>> ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 5e1e2ba..104638d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct 
>> tpdm_drvdata *drvdata)
>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>>   }
>>   +static ssize_t reset_store(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      const char *buf,
>> +                      size_t size)
>> +{
>> +    int ret = 0;
>> +    unsigned long val;
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    ret = kstrtoul(buf, 10, &val);
>> +    if (ret || val != 1)
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    /* Reset all datasets to ZERO, and init the default data*/
>> +    tpdm_init_datasets(drvdata);
>
> With the suggested rename in the previous patch, you wouldn't need
> a comment here.
I will update this in the next patch series.
>
>> +
>> +    spin_unlock(&drvdata->spinlock);
>> +
>
>
>> +    /* Disable tpdm if enabled */
>> +    if (drvdata->enable)
>> +        coresight_disable(drvdata->csdev);
>
> Couldn't this be done via disable_source ? Please don't overload
> the sysfs handle.
I will update this in the next patch series.
>
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_WO(reset);
>
> Documentation for the sysfs node please ?
I will update this in the next patch series.
>
> Suzuki
>

Re: [PATCH v3 04/11] coresight-tpdm: Add reset node to TPDM node

[Tao Zhang]

Hi Suzuki,

On 3/23/2023 10:48 PM, Suzuki K Poulose wrote:
> On 23/03/2023 14:41, Suzuki K Poulose wrote:
>> On 23/03/2023 06:04, Tao Zhang wrote:
>>> TPDM device need a node to reset the configurations and status of
>>> it. This change provides a node to reset the configurations and
>>> disable the TPDM if it has been enabled.
>
> Please justify why this "do everything" magic knob is required
> when there are tunables for individual controls in the later
> patches.

We want to have a single knob that resets all the datasets, which saves the

need to configure the individual controls one by one. Since it is often 
necessary

to reset all the datasets, this knob will be more user-friendly.


Tao

>
> Suzuki
>
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 
>>> ++++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> index 5e1e2ba..104638d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct 
>>> tpdm_drvdata *drvdata)
>>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>>>   }
>>> +static ssize_t reset_store(struct device *dev,
>>> +                      struct device_attribute *attr,
>>> +                      const char *buf,
>>> +                      size_t size)
>>> +{
>>> +    int ret = 0;
>>> +    unsigned long val;
>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    ret = kstrtoul(buf, 10, &val);
>>> +    if (ret || val != 1)
>>> +        return -EINVAL;
>>> +
>>> +    spin_lock(&drvdata->spinlock);
>>> +    /* Reset all datasets to ZERO, and init the default data*/
>>> +    tpdm_init_datasets(drvdata);
>>
>> With the suggested rename in the previous patch, you wouldn't need
>> a comment here.
>>
>>> +
>>> +    spin_unlock(&drvdata->spinlock);
>>> +
>>
>>
>>> +    /* Disable tpdm if enabled */
>>> +    if (drvdata->enable)
>>> +        coresight_disable(drvdata->csdev);
>>
>> Couldn't this be done via disable_source ? Please don't overload
>> the sysfs handle.
>>
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_WO(reset);
>>
>> Documentation for the sysfs node please ?
>>
>> Suzuki
>>
>

Re: [PATCH v3 06/11] coresight-tpdm: Add node to set dsb programming mode

[Tao Zhang]

Hi Suzuki,

On 3/23/2023 10:55 PM, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> Add node to set and show programming mode for TPDM DSB subunit.
>> Once the DSB programming mode is set, it will be written to the
>> register DSB_CR. Bit[10:9] of the DSB_CR register is used to set
>> the DSB test mode.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 10 ++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 62 
>> ++++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h       | 13 +++++
>>   3 files changed, 85 insertions(+)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index 27ce681..f13e282 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -35,3 +35,13 @@ Description:
>>           Accepts only one of the 2 values -  0 or 1.
>>           0 : Set the DSB trigger type to false
>>           1 : Set the DSB trigger type to true
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_mode
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the mode of DSB tpdm. Read the mode of DSB
>> +        tpdm.
>> +
>> +        Accepts the value needs to be greater than 0.
>
> Please could you document the values ?
I will update this in the next patch series.
>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index e28cf10..8cd822f 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -4,6 +4,7 @@
>>    */
>>     #include <linux/amba/bus.h>
>> +#include <linux/bitfield.h>
>>   #include <linux/bitmap.h>
>>   #include <linux/coresight.h>
>>   #include <linux/coresight-pmu.h>
>> @@ -51,6 +52,32 @@ static int tpdm_init_datasets(struct tpdm_drvdata 
>> *drvdata)
>>       return 0;
>>   }
>>   +static void set_dsb_cycacc_mode(struct tpdm_drvdata *drvdata, u32 
>> *val)
>> +{
>> +    u32 mode;
>> +
>> +    mode = TPDM_DSB_MODE_CYCACC(drvdata->dsb->mode);
>> +    *val &= ~TPDM_DSB_TEST_MODE;
>> +    *val |= FIELD_PREP(TPDM_DSB_TEST_MODE, mode);
>> +}
>> +
>> +static void set_dsb_hpsel_mode(struct tpdm_drvdata *drvdata, u32 *val)
>> +{
>> +    u32 mode;
>> +
>> +    mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode);
>> +    *val &= ~TPDM_DSB_HPSEL;
>> +    *val |= FIELD_PREP(TPDM_DSB_HPSEL, mode);
>> +}
>> +
>> +static void set_dsb_perf_mode(struct tpdm_drvdata *drvdata, u32 *val)
>> +{
>> +    if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF)
>> +        *val |= TPDM_DSB_CR_MODE;
>> +    else
>> +        *val &= ~TPDM_DSB_CR_MODE;
>> +}
>> +
>>   static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
>>   {
>>       if (drvdata->dsb->trig_type)
>> @@ -72,6 +99,12 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>> *drvdata)
>>       writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
>>         val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>> +    /* Set the cycle accurate mode */
>> +    set_dsb_cycacc_mode(drvdata, &val);
>> +    /* Set the byte lane for high-performance mode */
>> +    set_dsb_hpsel_mode(drvdata, &val);
>> +    /* Set the performance mode */
>> +    set_dsb_perf_mode(drvdata, &val);
>>       /* Set trigger type */
>>       set_trigger_type(drvdata, &val);
>>       /* Set the enable bit of DSB control register to 1 */
>> @@ -250,6 +283,34 @@ static struct attribute_group tpdm_attr_grp = {
>>       .attrs = tpdm_attrs,
>>   };
>>   +static ssize_t dsb_mode_show(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%lx\n",
>> +             (unsigned long)drvdata->dsb->mode);
>> +}
>> +
>> +static ssize_t dsb_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 val;
>> +
>> +    if ((kstrtoul(buf, 0, &val)) || val < 0)
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    drvdata->dsb->mode = val & TPDM_MODE_ALL;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_mode);
>> +
>>   static ssize_t dsb_trig_type_show(struct device *dev,
>>                        struct device_attribute *attr,
>>                        char *buf)
>> @@ -321,6 +382,7 @@ static ssize_t dsb_trig_ts_store(struct device *dev,
>>   static DEVICE_ATTR_RW(dsb_trig_ts);
>>     static struct attribute *tpdm_dsb_attrs[] = {
>> +    &dev_attr_dsb_mode.attr,
>>       &dev_attr_dsb_trig_ts.attr,
>>       &dev_attr_dsb_trig_type.attr,
>>       NULL,
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 68f33bd..8fee562 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -15,11 +15,22 @@
>>     /* Enable bit for DSB subunit */
>>   #define TPDM_DSB_CR_ENA        BIT(0)
>> +/* Enable bit for DSB subunit perfmance mode */
>> +#define TPDM_DSB_CR_MODE        BIT(1)
>>   /* Enable bit for DSB subunit trigger type */
>>   #define TPDM_DSB_CR_TRIG_TYPE        BIT(12)
>> +
>>   /* Enable bit for DSB subunit trigger timestamp */
>>   #define TPDM_DSB_TIER_XTRIG_TSENAB        BIT(1)
>>   +/* DSB programming modes */
>> +#define TPDM_DSB_MODE_CYCACC(val)    (val & GENMASK(2, 0))
>
> What do the values for the CYCACC mode mean ?

It means cycle accurate mode.  The targets may no longer have that mode. 
I will

make a final check and update this in the next patch series.

>
>> +#define TPDM_DSB_MODE_PERF        BIT(3)
>> +#define TPDM_DSB_MODE_HPBYTESEL(val)    (val & GENMASK(8, 4))
>> +#define TPDM_MODE_ALL            (0xFFFFFFF)
>> +#define TPDM_DSB_TEST_MODE        GENMASK(11, 9)
>> +#define TPDM_DSB_HPSEL        GENMASK(6, 2)
>
> Again what do the values mean ? Even if the kernel doesn't use them
> it would be good to document it.

I will update this in the next patch series.


Tao

>
> Suzuki
>
>> +
>>   /* TPDM integration test registers */
>>   #define TPDM_ITATBCNTRL        (0xEF0)
>>   #define TPDM_ITCNTRL        (0xF00)
>> @@ -48,10 +59,12 @@
>>     /**
>>    * struct dsb_dataset - specifics associated to dsb dataset
>> + * @mode:             DSB programming mode
>>    * @trig_ts:          Enable/Disable trigger timestamp.
>>    * @trig_type:        Enable/Disable trigger type.
>>    */
>>   struct dsb_dataset {
>> +    u32                mode;
>>       bool            trig_ts;
>>       bool            trig_type;
>>   };
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v3 07/11] coresight-tpdm: Add nodes for dsb edge control

[Tao Zhang]

Hi Suzuki,

On 3/24/2023 1:04 AM, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> Add the nodes to set value for DSB edge control and DSB edge
>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>> resgisters to configure edge control. DSB edge detection control
>> 00: Rising edge detection
>> 01: Falling edge detection
>> 10: Rising and falling edge detection (toggle detection) > And each 
>> DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>> configure mask. Eight 32 bit registers providing DSB interface
>> edge detection mask control.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  26 ++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 142 
>> ++++++++++++++++++++-
>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  14 ++
>>   3 files changed, 181 insertions(+), 1 deletion(-)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index f13e282..094d624 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -45,3 +45,29 @@ Description:
>>           tpdm.
>>             Accepts the value needs to be greater than 0.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the edge control of DSB tpdm. Read the
>> +        edge control of DSB tpdm.
>
> Could we not say :
>     Read / Write a set of the edge control registers of the DSB in
>         TPDM
I will update this in the next patch series.
>> +
>> +        Accepts the following three values.
>
> This is a bit confusing, at least to me. This could either mean,
> a single write of values 1-3 are accepted.

Got it. The following three values are required.

>> +        value 1: Start EDCR register number
>> +        value 2: End EDCR register number
>> +        value 3: The value need to be written
>
> But you really mean to say, the writes must be of the following format
>     <integer1> <integer2> <integer3>
>
> where :
>         <integer1> : Start EDCR ...
>         ....
>
I will update this in the next patch series.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the edge control mask of DSB tpdm. Read
>> +        the edge control mask of DSB tpdm.
>> +
>> +        Accepts the following three values.
>> +        value 1: Start EDCMR register number
>> +        value 2: End EDCMR register number
>> +        value 3: The value need to be written
>
> Similarly here.
I will update this in the next patch series.
>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 8cd822f..2a0b36c 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -88,7 +88,14 @@ static void set_trigger_type(struct tpdm_drvdata 
>> *drvdata, u32 *val)
>>     static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>>   {
>> -    u32 val;
>> +    u32 val, i;
>> +
>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++)
>> +        writel_relaxed(drvdata->dsb->edge_ctrl[i],
>> +               drvdata->base + TPDM_DSB_EDCR(i));
>> +    for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++)
>> +        writel_relaxed(drvdata->dsb->edge_ctrl_mask[i],
>> +               drvdata->base + TPDM_DSB_EDCMR(i));
>>         val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
>>       /* Set trigger timestamp */
>> @@ -311,6 +318,137 @@ static ssize_t dsb_mode_store(struct device *dev,
>>   }
>>   static DEVICE_ATTR_RW(dsb_mode);
>>   +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>> +                       struct device_attribute *attr,
>> +                       char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    ssize_t size = 0;
>> +    int i;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>> +        size += sysfs_emit_at(buf, size,
>> +                  "Index:0x%x Val:0x%x\n", i,
>> +                  drvdata->dsb->edge_ctrl[i]);
>> +    }
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +
>> +/*
>> + * value 1: Start EDCR register number
>> + * value 2: End EDCR register number
>> + * value 3: The value need to be written
>> + * The EDCR registers can include up to 16 32-bit registers, and each
>> + * one can be configured to control up to 16 edge detections(2 bits
>> + * control one edge detection). So a total 256 edge detections can be
>> + * configured. So the starting number(value 1) and ending 
>> number(value 2)
>> + * cannot be greater than 256, and value 1 should be less than value 2.
>> + * The following values are the rage of value 3.
>> + * 0 - Rising edge detection
>> + * 1 - Falling edge detection
>> + * 2 - Rising and falling edge detection (toggle detection)
>> + */
>> +static ssize_t dsb_edge_ctrl_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 start, end, edge_ctrl;
>> +    uint32_t val;
>> +    int i, index, bit, reg;
>> +
>> +    if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3)
>> +        return -EINVAL;
>> +    if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) ||
>> +        edge_ctrl > 0x2)
>> +        return -EPERM;
>
>     Isn't it an error to provide start > end ?
I will add this check in the next patch series.
>
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    for (i = start; i <= end; i++) {
>> +        /*
>> +         * The 32-bit register has 32 bits(NUM_OF_BITS).
>> +         * Each one register can be configured to control 16
>> +         * (NUM_OF_BITS / 2) edge detectioins.
>> +         */
>
>         /*
>          * Each DSB Edge control line requires 2bits.
>          * Thus we have 16 lines in a 32bit word.
>          */
>
> Could we please define something like:
>
> #define EDCRS_PER_WORD            16
> #define EDCR_TO_WORD_IDX(r)        ((r) / EDCRS_PER_WORD)
> #define EDCR_TO_WORD_SHIFT(r)        ((r) % EDCRS_PER_WORD)
> #define EDCR_TO_WORD_MASK(r)        (0x3 << EDCR_TO_WORD_SHIFT((r)))
>
Sure, I will update this in the next patch series.
>> +        reg = i / (NUM_OF_BITS / 2);
>> +        index = i % (NUM_OF_BITS / 2);
>> +        bit = index * 2;
>> +
>
>     reg = EDCR_TO_WORD_IDX(i);
>     mask = EDCR_TO_WORD_MASK(i);
>
>     val &= ~mask;
>     val |= FIELD_PREP(mask, edge_ctrl);
Got it.
>
>> +        val = drvdata->dsb->edge_ctrl[reg];
>> +        val &= ~GENMASK((bit + 1), bit);
>> +        val |= (edge_ctrl << bit);
>
>
>> + drvdata->dsb->edge_ctrl[reg] = val;
>> +    }
>> +    spin_unlock(&drvdata->spinlock);
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_edge_ctrl);
>> +
>> +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev,
>> +                        struct device_attribute *attr,
>> +                        char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    ssize_t size = 0;
>> +    int i;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR / 2; i++) {
>
> Why is this not `i < TPDM_DSB_MAX_EDCMR` ?
I will update this in the next patch series.
>
>> +        size += sysfs_emit_at(buf, size,
>> +                  "Index:0x%x Val:0x%x\n", i,
>> +                  drvdata->dsb->edge_ctrl_mask[i]);
>> +    }
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +
>> +/*
>> + * value 1: Start EDCMR register number
>> + * value 2: End EDCMR register number
>> + * value 3: The value need to be written
>> + */
>> +static ssize_t dsb_edge_ctrl_mask_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 start, end, val;
>> +    u32 set;
>> +    int i, index, reg;
>> +
>> +    if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3)
>> +        return -EINVAL;
>> +    if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES)
>> +        || (val & ~1UL))
>> +        return -EPERM;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    for (i = start; i <= end; i++) {
>> +        /*
>> +         * The 32-bit register has 32 bits(NUM_OF_BITS).
>> +         * Each one register can be configured to control 32
>> +         * (NUM_OF_BITS) edge detectioin masks.
>
> minor nit: detection.
>
> You could simply say:
>         /*
>          * There is 1 bit per DSB Edge Control line.
>          * Thus we have 32 lines in a 32bit word.
>          */
>          reg = i / 32;
I will update this in the next patch series.
>
>> +        reg = i / NUM_OF_BITS;
>> +        index = (i % NUM_OF_BITS);
>> +
>> +        set = drvdata->dsb->edge_ctrl_mask[reg];
>> +        if (val)
>> +            set |= BIT(index);
>> +        else
>> +            set &= ~BIT(index);
>> +        drvdata->dsb->edge_ctrl_mask[reg] = set;
>> +    }
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
>> +
>>   static ssize_t dsb_trig_type_show(struct device *dev,
>>                        struct device_attribute *attr,
>>                        char *buf)
>> @@ -383,6 +521,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts);
>>     static struct attribute *tpdm_dsb_attrs[] = {
>>       &dev_attr_dsb_mode.attr,
>> +    &dev_attr_dsb_edge_ctrl.attr,
>> +    &dev_attr_dsb_edge_ctrl_mask.attr,
>>       &dev_attr_dsb_trig_ts.attr,
>>       &dev_attr_dsb_trig_type.attr,
>>       NULL,
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 8fee562..342ef23 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -12,6 +12,8 @@
>>   /* DSB Subunit Registers */
>>   #define TPDM_DSB_CR        (0x780)
>>   #define TPDM_DSB_TIER        (0x784)
>> +#define TPDM_DSB_EDCR(n)    (0x808 + (n * 4))
>> +#define TPDM_DSB_EDCMR(n)    (0x848 + (n * 4))
>>     /* Enable bit for DSB subunit */
>>   #define TPDM_DSB_CR_ENA        BIT(0)
>> @@ -31,6 +33,8 @@
>>   #define TPDM_DSB_TEST_MODE        GENMASK(11, 9)
>>   #define TPDM_DSB_HPSEL        GENMASK(6, 2)
>>   +#define NUM_OF_BITS        32
>
> Please don't use such generic names. Instead define TPDM specific
> helper definitions. See above.
>
I will update this in the next patch series.


Tao

>
> Suzuki
>
>
>> +
>>   /* TPDM integration test registers */
>>   #define TPDM_ITATBCNTRL        (0xEF0)
>>   #define TPDM_ITCNTRL        (0xF00)
>> @@ -57,14 +61,24 @@
>>   #define TPDM_PIDR0_DS_IMPDEF    BIT(0)
>>   #define TPDM_PIDR0_DS_DSB    BIT(1)
>>   +#define TPDM_DSB_MAX_LINES    256
>> +/* MAX number of EDCR registers */
>> +#define TPDM_DSB_MAX_EDCR    16 > +/* MAX number of EDCMR registers */
>> +#define TPDM_DSB_MAX_EDCMR    8
>> +
>>   /**
>>    * struct dsb_dataset - specifics associated to dsb dataset
>>    * @mode:             DSB programming mode
>> + * @edge_ctrl:        Save value for edge control
>> + * @edge_ctrl_mask:   Save value for edge control mask
>>    * @trig_ts:          Enable/Disable trigger timestamp.
>>    * @trig_type:        Enable/Disable trigger type.
>>    */
>>   struct dsb_dataset {
>>       u32                mode;
>> +    u32                edge_ctrl[TPDM_DSB_MAX_EDCR];
>> +    u32                edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
>>       bool            trig_ts;
>>       bool            trig_type;
>>   };
>

Re: [PATCH v3 08/11] coresight-tpdm: Add nodes to configure pattern match output

[Tao Zhang]

Hi Suzuki,

On 3/24/2023 1:27 AM, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> Add nodes to configure trigger pattern and trigger pattern mask.
>> Each DSB subunit TPDM has maximum of n(n<7) XPR registers to
>> configure trigger pattern match output. Eight 32 bit registers
>> providing DSB interface trigger output pattern match comparison.
>> And each DSB subunit TPDM has maximum of m(m<7) XPMR registers to
>> configure trigger pattern mask match output. Eight 32 bit
>> registers providing DSB interface trigger output pattern match
>> mask.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 24 +++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 84 
>> ++++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  8 +++
>>   3 files changed, 116 insertions(+)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index 094d624..c06374f 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -71,3 +71,27 @@ Description:
>>           value 1: Start EDCMR register number
>>           value 2: End EDCMR register number
>>           value 3: The value need to be written
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_trig_patt_val
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the trigger pattern value of DSB tpdm.
>> +        Read the trigger pattern value of DSB tpdm.
>> +
>> +        Accepts the following two values.
>> +        value 1: Index number of XPR register
>> +        value 2: The value need to be written
>
> minor nit: What values are acceptable ? Otherwise looks fine.

I will update this in the next patch series.


Tao

>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>

Re: [PATCH v3 09/11] coresight-tpdm: Add nodes for timestamp request

[Tao Zhang]

Hi Suzuki,

On 3/24/2023 2:41 AM, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> Add nodes to configure the timestamp request based on input
>> pattern match. Each TPDM that support DSB subunit has maximum of
>> n(n<7) TPR registers to configure value for timestamp request
>> based on input pattern match. Eight 32 bit registers providing
>> DSB interface timestamp request  pattern match comparison. And
>> each TPDM that support DSB subunit has maximum of m(m<7) TPMR
>> registers to configure pattern mask for timestamp request. Eight
>> 32 bit registers providing DSB interface timestamp request
>> pattern match mask generation. Add nodes to enable/disable
>> pattern timestamp and set pattern timestamp type.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  48 ++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 172 
>> +++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  14 ++
>>   3 files changed, 234 insertions(+)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index c06374f..60ff660 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -95,3 +95,51 @@ Description:
>>           Accepts the following two values.
>>           value 1: Index number of XPMR register
>>           value 2: The value need to be written
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt_val
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the pattern value of DSB tpdm. Read
>> +        the pattern value of DSB tpdm.
>> +
>> +        Accepts the following two values.
>> +        value 1: Index number of TPR register
>> +        value 2: The value need to be written
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt_mask
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the pattern mask of DSB tpdm. Read
>> +        the pattern mask of DSB tpdm.
>> +
>> +        Accepts the following two values.
>> +        value 1: Index number of TPMR register
>> +        value 2: The value need to be written
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt_ts
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the pattern timestamp of DSB tpdm. Read
>> +        the pattern timestamp of DSB tpdm.
>> +
>> +        Accepts only one of the 2 values -  0 or 1.
>> +        0 : Set the DSB pattern timestamp to false
>> +        1 : Set the DSB pattern timestamp to true
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt_type
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the pattern type of DSB tpdm. Read
>> +        the pattern type of DSB tpdm.
>> +
>
> Sounds a bit strange for "pattern type" to be bool. What does this do ?
> Does it enable/disable something  ?

I think there are two pattern types and controlled by 1 bit in the 
register. It doesn't

really make sense to use Boolean values. I will confirm this and update 
it in the next

patch series.

>
>> +        Accepts only one of the 2 values -  0 or 1.
>> +        0 : Set the DSB pattern type to false
>> +        1 : Set the DSB pattern type to true
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index d6cc6b5..c740681 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -98,6 +98,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>> *drvdata)
>>                  drvdata->base + TPDM_DSB_EDCMR(i));
>>         for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
>> +        writel_relaxed(drvdata->dsb->patt_val[i],
>> +                drvdata->base + TPDM_DSB_TPR(i));
>> +        writel_relaxed(drvdata->dsb->patt_mask[i],
>> +                drvdata->base + TPDM_DSB_TPMR(i));
>> +    }
>> +
>> +    for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
>
> Why not do all the pattern related writing in one shot, instead
> of two loops ?
I will combine these two loops to one.
>
>> writel_relaxed(drvdata->dsb->trig_patt_val[i],
>>                   drvdata->base + TPDM_DSB_XPR(i));
>>           writel_relaxed(drvdata->dsb->trig_patt_mask[i],
>> @@ -105,6 +112,16 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>> *drvdata)
>>       }
>>         val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
>> +    /* Set pattern timestamp type and enablement */
>> +    if (drvdata->dsb->patt_ts) {
>> +        val |= TPDM_DSB_TIER_PATT_TSENAB;
>> +        if (drvdata->dsb->patt_type)
>> +            val |= TPDM_DSB_TIER_PATT_TYPE;
>> +        else
>> +            val &= ~TPDM_DSB_TIER_PATT_TYPE;
>> +    } else {
>> +        val &= ~TPDM_DSB_TIER_PATT_TSENAB;
>> +    }
>
> set_dsb_pattern_ts() in line with the other helper functions ?
> Rest looks fine to me.
>
Sure, I will update this in the next patch series.


Tao

> Suzuki
>
>
>>       /* Set trigger timestamp */
>>       if (drvdata->dsb->trig_ts)
>>           val |= TPDM_DSB_TIER_XTRIG_TSENAB;
>> @@ -455,6 +472,157 @@ static ssize_t dsb_edge_ctrl_mask_store(struct 
>> device *dev,
>>       return size;
>>   }
>>   static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
>> +
>> +static ssize_t dsb_patt_val_show(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    ssize_t size = 0;
>> +    int i = 0;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
>> +        size += sysfs_emit_at(buf, size,
>> +                  "Index: 0x%x Value: 0x%x\n", i,
>> +                  drvdata->dsb->patt_val[i]);
>> +    }
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +
>> +/*
>> + * value 1: Index of TPR register
>> + * value 2: Value need to be written
>> + */
>> +static ssize_t dsb_patt_val_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 index, val;
>> +
>> +    if (sscanf(buf, "%lx %lx", &index, &val) != 2)
>> +        return -EINVAL;
>> +    if (index >= TPDM_DSB_MAX_PATT)
>> +        return -EPERM;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    drvdata->dsb->patt_val[index] = val;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_patt_val);
>> +
>> +static ssize_t dsb_patt_mask_show(struct device *dev,
>> +                       struct device_attribute *attr,
>> +                       char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    ssize_t size = 0;
>> +    int i = 0;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
>> +        size += sysfs_emit_at(buf, size,
>> +                  "Index: 0x%x Value: 0x%x\n", i,
>> +                  drvdata->dsb->patt_mask[i]);
>> +    }
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +
>> +/*
>> + * value 1: Index of TPMR register
>> + * value 2: Value need to be written
>> + */
>> +static ssize_t dsb_patt_mask_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 index, val;
>> +
>> +    if (sscanf(buf, "%lx %lx", &index, &val) != 2)
>> +        return -EINVAL;
>> +    if (index >= TPDM_DSB_MAX_PATT)
>> +        return -EPERM;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    drvdata->dsb->patt_mask[index] = val;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_patt_mask);
>> +
>> +static ssize_t dsb_patt_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->dsb->patt_ts);
>> +}
>> +
>> +/*
>> + * value 1: Enable/Disable DSB pattern timestamp
>> + */
>
>> +static ssize_t dsb_patt_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->dsb->patt_ts = true;
>> +    else
>> +        drvdata->dsb->patt_ts = false;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_patt_ts);
>> +
>> +static ssize_t dsb_patt_type_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->dsb->patt_type);
>> +}
>> +
>> +/*
>> + * value 1: Set DSB pattern type
>
>
>> + */ > +static ssize_t dsb_patt_type_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->dsb->patt_type = true;
>> +    else
>> +        drvdata->dsb->patt_type = false;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_patt_type);
>> +
>>   static ssize_t dsb_trig_patt_val_show(struct device *dev,
>>                          struct device_attribute *attr,
>>                          char *buf)
>> @@ -605,6 +773,10 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>       &dev_attr_dsb_mode.attr,
>>       &dev_attr_dsb_edge_ctrl.attr,
>>       &dev_attr_dsb_edge_ctrl_mask.attr,
>> +    &dev_attr_dsb_patt_val.attr,
>> +    &dev_attr_dsb_patt_mask.attr,
>> +    &dev_attr_dsb_patt_ts.attr,
>> +    &dev_attr_dsb_patt_type.attr,
>>       &dev_attr_dsb_trig_patt_val.attr,
>>       &dev_attr_dsb_trig_patt_mask.attr,
>>       &dev_attr_dsb_trig_ts.attr,
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 2e8020e..f9d4dd9 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -12,6 +12,8 @@
>>   /* DSB Subunit Registers */
>>   #define TPDM_DSB_CR        (0x780)
>>   #define TPDM_DSB_TIER        (0x784)
>> +#define TPDM_DSB_TPR(n)        (0x788 + (n * 4))
>> +#define TPDM_DSB_TPMR(n)    (0x7A8 + (n * 4))
>>   #define TPDM_DSB_XPR(n)        (0x7C8 + (n * 4))
>>   #define TPDM_DSB_XPMR(n)    (0x7E8 + (n * 4))
>>   #define TPDM_DSB_EDCR(n)    (0x808 + (n * 4))
>> @@ -24,8 +26,12 @@
>>   /* Enable bit for DSB subunit trigger type */
>>   #define TPDM_DSB_CR_TRIG_TYPE        BIT(12)
>>   +/* Enable bit for DSB subunit pattern timestamp */
>> +#define TPDM_DSB_TIER_PATT_TSENAB        BIT(0)
>>   /* Enable bit for DSB subunit trigger timestamp */
>>   #define TPDM_DSB_TIER_XTRIG_TSENAB        BIT(1)
>> +/* Bit for DSB subunit pattern type */
>> +#define TPDM_DSB_TIER_PATT_TYPE        BIT(2)
>>     /* DSB programming modes */
>>   #define TPDM_DSB_MODE_CYCACC(val)    (val & GENMASK(2, 0))
>> @@ -76,6 +82,10 @@
>>    * @mode:             DSB programming mode
>>    * @edge_ctrl:        Save value for edge control
>>    * @edge_ctrl_mask:   Save value for edge control mask
>> + * @patt_val:         Save value for pattern
>> + * @patt_mask:        Save value for pattern mask
>> + * @patt_ts:          Enable/Disable pattern timestamp
>> + * @patt_type:        Set pattern type
>>    * @trig_patt_val:    Save value for trigger pattern
>>    * @trig_patt_mask:   Save value for trigger pattern mask
>>    * @trig_ts:          Enable/Disable trigger timestamp.
>> @@ -85,6 +95,10 @@ struct dsb_dataset {
>>       u32                mode;
>>       u32                edge_ctrl[TPDM_DSB_MAX_EDCR];
>>       u32                edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
>> +    u32                patt_val[TPDM_DSB_MAX_PATT];
>> +    u32                patt_mask[TPDM_DSB_MAX_PATT];
>> +    bool            patt_ts;
>> +    bool            patt_type;
>>       u32                trig_patt_val[TPDM_DSB_MAX_PATT];
>>       u32                trig_patt_mask[TPDM_DSB_MAX_PATT];
>>       bool            trig_ts;
>

Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Suzuki K Poulose]

On 27/03/2023 04:31, Tao Zhang wrote:
> 
> On 3/26/2023 3:31 AM, Suzuki K Poulose wrote:
>> On 24/03/2023 14:58, Tao Zhang wrote:
>>> Hi Suzuki,
>>>
>>> 在 3/23/2023 7:51 PM, Suzuki K Poulose 写道:
>>>> On 23/03/2023 06:03, Tao Zhang wrote:
>>>>> Read the DSB element size from the device tree. Set the register
>>>>> bit that controls the DSB element size of the corresponding port.
>>>>>
>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>> ---
>>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 58 
>>>>> ++++++++++++++++++++++++++++
>>>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>>>   2 files changed, 62 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> index f712e11..8dcfc4a 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> @@ -21,6 +21,47 @@
>>>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>>>   +/* Search and read element data size from the TPDM node in
>>>>> + * the devicetree. Each input port of TPDA is connected to
>>>>> + * a TPDM. Different TPDM supports different types of dataset,
>>>>> + * and some may support more than one type of dataset.
>>>>> + * Parameter "inport" is used to pass in the input port number
>>>>> + * of TPDA, and it is set to 0 in the recursize call.
>>>>> + * Parameter "parent" is used to pass in the original call.
>>>>> + */
>>>>
>>>> I am still not clear why we need to do this recursively ?
>>>
>>> Some TPDMs are not directly output connected to the TPDAs. So here I
>>>
>>> use a recursive method to check from the TPDA input port until I find
>>>
>>> the connected TPDM.
>>>
>>> Do you have a better suggestion besides a recursive method?
>>>
>>>>
>>>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>>> +               struct coresight_device *csdev, int inport, bool 
>>>>> parent)
>>>>
>>>> Please could we renamse csdev => tpda_dev
>>>
>>> Since this is a recursively called function, this Coresight device is 
>>> not
>>>
>>> necessarily TPDA, it can be other Coresight device.
>>>
>>>>
>>>>> +{
>>>>> +    static int nr_inport;
>>>>> +    int i;
>>>>> +    struct coresight_device *in_csdev;
>>>>
>>>> similarly tpdm_dev ?
>>> Same as above, this variable may not necessarily be a TPDM.
>>>>
>>>> Could we not add a check here to see if the dsb_esize[inport] is 
>>>> already
>>>> set and then bail out, reading this over and over ?
>>>>
>>> I will update this in the next patch series.
>>>>> +
>>>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if (parent)
>>>>> +        nr_inport = inport;
>>>>> +
>>>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>>> +        in_csdev = csdev->pdata->in_conns[i].remote_dev;
>>>>
>>>> Please note, the names of the structure field might change in the
>>>> next version of James' series
>>> Got it. I will keep an eye out for the James' patch series.
>>>>
>>>>> +        if (!in_csdev)
>>>>> +            break;
>>>>> +
>>>>> +        if (parent)
>>>>> +            if (csdev->pdata->in_conns[i].port != inport)
>>>>> +                continue;
>>>>> +
>>>>> +        if (in_csdev && strstr(dev_name(&in_csdev->dev), "tpdm")) {
>>>>
>>>> Isn't there a better way to distinguish a device to be TPDM ? May be we
>>>> could even add a source_sub_type - SOURCE_TPDM instead of using
>>>> SOURCE_OTHERS ? Do you expect other sources to be connected to TPDA?
>>>> e.g., STMs ?
>>>
>>> I can add "SOURCE_TPDM" as a source_sub_type, but SOURCE_OTHERS needs
>>>
>>> to be kept since the other Coresight component we will upstream later 
>>> may
>>>
>>> need it.
>>>
>>>>
>>>>> + of_property_read_u32(in_csdev->dev.parent->of_node,
>>>>> +                    "qcom,dsb-element-size", 
>>>>> &drvdata->dsb_esize[nr_inport]);
>>>>> +            break;
>>>>> +        }
>>>>> +        tpda_set_element_size(drvdata, in_csdev, 0, false);
>>>>
>>>> What is the point of this ? Is this for covering the a TPDA 
>>>> connected to
>>>> another TPDA ?
>>>>
>>>> e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?
>>>
>>> A TPDM may not connect to the TPDA directly, for example,
>>>
>>> TPDM0 ->FUNNEL0->FUNNEL1->TPDA0
>>>
>>> And many TPDMs can connect to one TPDA, one input port on TPDA only has
>>>
>>> one TPDM connected. Therefore, we use a recursive method to find the 
>>> TPDM
>>>
>>> corresponding to the input port of TPDA.
>>
>> How do you find out decide what to choose, if there are multiple TPDMs
>> connected to FUNNEL0 or even FUNNEL1 ?
>>
>> e.g
>>
>> TPDM0->FUNNEL0->FUNNEL1->TPDA0
>>                 /
>>           TPDM1
> 
> We can find out the corresponding TPDM by the input port number of TPDA.
> 
> Each input port is connected to a TPDM. So we have an input port number in
> 
> the input parameter of the recursive lookup function 
> "tpda_set_element_size".

I don't understand, how you would figure out, in the above situation.
i.e., FUNNEL1 is connected to TPDA0, but there are two TPDMs that could
be pumping the trace. They both arrive via FUNNEL1. So, how does that
solve your problem ?

Suzuki


> 
>> Suzuki
>>
>>

Re: [PATCH v3 01/11] dt-bindings: arm: Add support for DSB element size

[Tao Zhang]

Hi Krzysztof,

On 3/25/2023 7:35 PM, Krzysztof Kozlowski wrote:
> On 24/03/2023 10:15, Tao Zhang wrote:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    minimum: 32
>>>>> +    maximum: 64
>>>> Shouldn't this be something like oneOf ? It is not a range, but one of
>>>> those two specific values ?
>>> Yes, "qcom,dsb-element-size" should be an optional option required in
>>> TPDM
>>>
>>> devicetree. Other properties like "qcom,cmb-element-size",
>>> "qcom,tc-element-size"
>>>
>>> and etc. will be added in a later patch series.
>>>
>>> I will update this doc according to your advice in the next version of
>>> the patch.
>>>
>>> Tao
>>>
>> Correct my misunderstanding in the mail above.
>>
>> You are right, DSB element size should be 32-bit or 64-bit. I will
>> update this in the next
> Then 'enum', not 'oneOf'.

Got it.


Tao

>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Tao Zhang]

Hi Suzuki,

On 3/27/2023 5:43 PM, Suzuki K Poulose wrote:
> On 27/03/2023 04:31, Tao Zhang wrote:
>>
>> On 3/26/2023 3:31 AM, Suzuki K Poulose wrote:
>>> On 24/03/2023 14:58, Tao Zhang wrote:
>>>> Hi Suzuki,
>>>>
>>>> 在 3/23/2023 7:51 PM, Suzuki K Poulose 写道:
>>>>> On 23/03/2023 06:03, Tao Zhang wrote:
>>>>>> Read the DSB element size from the device tree. Set the register
>>>>>> bit that controls the DSB element size of the corresponding port.
>>>>>>
>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>> ---
>>>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 58 
>>>>>> ++++++++++++++++++++++++++++
>>>>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>>>>   2 files changed, 62 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>> index f712e11..8dcfc4a 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>> @@ -21,6 +21,47 @@
>>>>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>>>>   +/* Search and read element data size from the TPDM node in
>>>>>> + * the devicetree. Each input port of TPDA is connected to
>>>>>> + * a TPDM. Different TPDM supports different types of dataset,
>>>>>> + * and some may support more than one type of dataset.
>>>>>> + * Parameter "inport" is used to pass in the input port number
>>>>>> + * of TPDA, and it is set to 0 in the recursize call.
>>>>>> + * Parameter "parent" is used to pass in the original call.
>>>>>> + */
>>>>>
>>>>> I am still not clear why we need to do this recursively ?
>>>>
>>>> Some TPDMs are not directly output connected to the TPDAs. So here I
>>>>
>>>> use a recursive method to check from the TPDA input port until I find
>>>>
>>>> the connected TPDM.
>>>>
>>>> Do you have a better suggestion besides a recursive method?
>>>>
>>>>>
>>>>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>>>> +               struct coresight_device *csdev, int inport, bool 
>>>>>> parent)
>>>>>
>>>>> Please could we renamse csdev => tpda_dev
>>>>
>>>> Since this is a recursively called function, this Coresight device 
>>>> is not
>>>>
>>>> necessarily TPDA, it can be other Coresight device.
>>>>
>>>>>
>>>>>> +{
>>>>>> +    static int nr_inport;
>>>>>> +    int i;
>>>>>> +    struct coresight_device *in_csdev;
>>>>>
>>>>> similarly tpdm_dev ?
>>>> Same as above, this variable may not necessarily be a TPDM.
>>>>>
>>>>> Could we not add a check here to see if the dsb_esize[inport] is 
>>>>> already
>>>>> set and then bail out, reading this over and over ?
>>>>>
>>>> I will update this in the next patch series.
>>>>>> +
>>>>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (parent)
>>>>>> +        nr_inport = inport;
>>>>>> +
>>>>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>>>> +        in_csdev = csdev->pdata->in_conns[i].remote_dev;
>>>>>
>>>>> Please note, the names of the structure field might change in the
>>>>> next version of James' series
>>>> Got it. I will keep an eye out for the James' patch series.
>>>>>
>>>>>> +        if (!in_csdev)
>>>>>> +            break;
>>>>>> +
>>>>>> +        if (parent)
>>>>>> +            if (csdev->pdata->in_conns[i].port != inport)
>>>>>> +                continue;
>>>>>> +
>>>>>> +        if (in_csdev && strstr(dev_name(&in_csdev->dev), "tpdm")) {
>>>>>
>>>>> Isn't there a better way to distinguish a device to be TPDM ? May 
>>>>> be we
>>>>> could even add a source_sub_type - SOURCE_TPDM instead of using
>>>>> SOURCE_OTHERS ? Do you expect other sources to be connected to TPDA?
>>>>> e.g., STMs ?
>>>>
>>>> I can add "SOURCE_TPDM" as a source_sub_type, but SOURCE_OTHERS needs
>>>>
>>>> to be kept since the other Coresight component we will upstream 
>>>> later may
>>>>
>>>> need it.
>>>>
>>>>>
>>>>>> + of_property_read_u32(in_csdev->dev.parent->of_node,
>>>>>> +                    "qcom,dsb-element-size", 
>>>>>> &drvdata->dsb_esize[nr_inport]);
>>>>>> +            break;
>>>>>> +        }
>>>>>> +        tpda_set_element_size(drvdata, in_csdev, 0, false);
>>>>>
>>>>> What is the point of this ? Is this for covering the a TPDA 
>>>>> connected to
>>>>> another TPDA ?
>>>>>
>>>>> e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?
>>>>
>>>> A TPDM may not connect to the TPDA directly, for example,
>>>>
>>>> TPDM0 ->FUNNEL0->FUNNEL1->TPDA0
>>>>
>>>> And many TPDMs can connect to one TPDA, one input port on TPDA only 
>>>> has
>>>>
>>>> one TPDM connected. Therefore, we use a recursive method to find 
>>>> the TPDM
>>>>
>>>> corresponding to the input port of TPDA.
>>>
>>> How do you find out decide what to choose, if there are multiple TPDMs
>>> connected to FUNNEL0 or even FUNNEL1 ?
>>>
>>> e.g
>>>
>>> TPDM0->FUNNEL0->FUNNEL1->TPDA0
>>>                 /
>>>           TPDM1
>>
>> We can find out the corresponding TPDM by the input port number of TPDA.
>>
>> Each input port is connected to a TPDM. So we have an input port 
>> number in
>>
>> the input parameter of the recursive lookup function 
>> "tpda_set_element_size".
>
> I don't understand, how you would figure out, in the above situation.
> i.e., FUNNEL1 is connected to TPDA0, but there are two TPDMs that could
> be pumping the trace. They both arrive via FUNNEL1. So, how does that
> solve your problem ?

In our HW design, the input ports of TPDA and TPDM are one-one-one 
corresponding.  Only one

TPDM can be found connected from one TPDA's input port. The path to a 
TPDA input port doesn't

connect more than one TPDM. It's by HW design.


Tao

>
> Suzuki
>
>
>>
>>> Suzuki
>>>
>>>
>

Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Suzuki K Poulose]

On 28/03/2023 12:31, Tao Zhang wrote:
> Hi Suzuki,
> 
> On 3/27/2023 5:43 PM, Suzuki K Poulose wrote:
>> On 27/03/2023 04:31, Tao Zhang wrote:
>>>
>>> On 3/26/2023 3:31 AM, Suzuki K Poulose wrote:
>>>> On 24/03/2023 14:58, Tao Zhang wrote:
>>>>> Hi Suzuki,
>>>>>
>>>>> 在 3/23/2023 7:51 PM, Suzuki K Poulose 写道:
>>>>>> On 23/03/2023 06:03, Tao Zhang wrote:
>>>>>>> Read the DSB element size from the device tree. Set the register
>>>>>>> bit that controls the DSB element size of the corresponding port.
>>>>>>>
>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>> ---
>>>>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 58 
>>>>>>> ++++++++++++++++++++++++++++
>>>>>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>>>>>   2 files changed, 62 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>>>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>>> index f712e11..8dcfc4a 100644
>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>>> @@ -21,6 +21,47 @@
>>>>>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>>>>>   +/* Search and read element data size from the TPDM node in
>>>>>>> + * the devicetree. Each input port of TPDA is connected to
>>>>>>> + * a TPDM. Different TPDM supports different types of dataset,
>>>>>>> + * and some may support more than one type of dataset.
>>>>>>> + * Parameter "inport" is used to pass in the input port number
>>>>>>> + * of TPDA, and it is set to 0 in the recursize call.
>>>>>>> + * Parameter "parent" is used to pass in the original call.
>>>>>>> + */
>>>>>>
>>>>>> I am still not clear why we need to do this recursively ?
>>>>>
>>>>> Some TPDMs are not directly output connected to the TPDAs. So here I
>>>>>
>>>>> use a recursive method to check from the TPDA input port until I find
>>>>>
>>>>> the connected TPDM.
>>>>>
>>>>> Do you have a better suggestion besides a recursive method?
>>>>>
>>>>>>
>>>>>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>>>>> +               struct coresight_device *csdev, int inport, bool 
>>>>>>> parent)
>>>>>>
>>>>>> Please could we renamse csdev => tpda_dev
>>>>>
>>>>> Since this is a recursively called function, this Coresight device 
>>>>> is not
>>>>>
>>>>> necessarily TPDA, it can be other Coresight device.
>>>>>
>>>>>>
>>>>>>> +{
>>>>>>> +    static int nr_inport;
>>>>>>> +    int i;
>>>>>>> +    struct coresight_device *in_csdev;
>>>>>>
>>>>>> similarly tpdm_dev ?
>>>>> Same as above, this variable may not necessarily be a TPDM.
>>>>>>
>>>>>> Could we not add a check here to see if the dsb_esize[inport] is 
>>>>>> already
>>>>>> set and then bail out, reading this over and over ?
>>>>>>
>>>>> I will update this in the next patch series.
>>>>>>> +
>>>>>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    if (parent)
>>>>>>> +        nr_inport = inport;
>>>>>>> +
>>>>>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>>>>> +        in_csdev = csdev->pdata->in_conns[i].remote_dev;
>>>>>>
>>>>>> Please note, the names of the structure field might change in the
>>>>>> next version of James' series
>>>>> Got it. I will keep an eye out for the James' patch series.
>>>>>>
>>>>>>> +        if (!in_csdev)
>>>>>>> +            break;
>>>>>>> +
>>>>>>> +        if (parent)
>>>>>>> +            if (csdev->pdata->in_conns[i].port != inport)
>>>>>>> +                continue;
>>>>>>> +
>>>>>>> +        if (in_csdev && strstr(dev_name(&in_csdev->dev), "tpdm")) {
>>>>>>
>>>>>> Isn't there a better way to distinguish a device to be TPDM ? May 
>>>>>> be we
>>>>>> could even add a source_sub_type - SOURCE_TPDM instead of using
>>>>>> SOURCE_OTHERS ? Do you expect other sources to be connected to TPDA?
>>>>>> e.g., STMs ?
>>>>>
>>>>> I can add "SOURCE_TPDM" as a source_sub_type, but SOURCE_OTHERS needs
>>>>>
>>>>> to be kept since the other Coresight component we will upstream 
>>>>> later may
>>>>>
>>>>> need it.
>>>>>
>>>>>>
>>>>>>> + of_property_read_u32(in_csdev->dev.parent->of_node,
>>>>>>> +                    "qcom,dsb-element-size", 
>>>>>>> &drvdata->dsb_esize[nr_inport]);
>>>>>>> +            break;
>>>>>>> +        }
>>>>>>> +        tpda_set_element_size(drvdata, in_csdev, 0, false);
>>>>>>
>>>>>> What is the point of this ? Is this for covering the a TPDA 
>>>>>> connected to
>>>>>> another TPDA ?
>>>>>>
>>>>>> e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?
>>>>>
>>>>> A TPDM may not connect to the TPDA directly, for example,
>>>>>
>>>>> TPDM0 ->FUNNEL0->FUNNEL1->TPDA0
>>>>>
>>>>> And many TPDMs can connect to one TPDA, one input port on TPDA only 
>>>>> has
>>>>>
>>>>> one TPDM connected. Therefore, we use a recursive method to find 
>>>>> the TPDM
>>>>>
>>>>> corresponding to the input port of TPDA.
>>>>
>>>> How do you find out decide what to choose, if there are multiple TPDMs
>>>> connected to FUNNEL0 or even FUNNEL1 ?
>>>>
>>>> e.g
>>>>
>>>> TPDM0->FUNNEL0->FUNNEL1->TPDA0
>>>>                 /
>>>>           TPDM1
>>>
>>> We can find out the corresponding TPDM by the input port number of TPDA.
>>>
>>> Each input port is connected to a TPDM. So we have an input port 
>>> number in
>>>
>>> the input parameter of the recursive lookup function 
>>> "tpda_set_element_size".
>>
>> I don't understand, how you would figure out, in the above situation.
>> i.e., FUNNEL1 is connected to TPDA0, but there are two TPDMs that could
>> be pumping the trace. They both arrive via FUNNEL1. So, how does that
>> solve your problem ?
> 
> In our HW design, the input ports of TPDA and TPDM are one-one-one 
> corresponding.  Only one
> 
> TPDM can be found connected from one TPDA's input port. The path to a 
> TPDA input port doesn't
> 
> connect more than one TPDM. It's by HW design.

Your current designs may be like that. But as far as the driver is
concerned, I would like to add in extra measures to ensure that it
encounters a variation from the above on a future platform. So, please
could you add a check to detect this case and add a WARNING ?

Suzuki


> 
> 
> Tao
> 
>>
>> Suzuki
>>
>>
>>>
>>>> Suzuki
>>>>
>>>>
>>

Re: [PATCH v3 10/11] dt-bindings: arm: Add support for DSB MSR register

[Krzysztof Kozlowski]

On 23/03/2023 07:04, Tao Zhang wrote:
> Add property "qcom,dsb_msr_num" to support DSB(Discrete Single
> Bit) MSR(mux select register) for TPDM. It specifies the number
> of MSR registers supported by the DSB TDPM.
> 
> Signed-off-by: Tao Zhang <quic_taozha@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 d9b6b613..691c7ba 100644
> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
> @@ -53,6 +53,15 @@ properties:
>      minimum: 32
>      maximum: 64
>  
> +  qcom,dsb_msr_num:

Underscores are not allowed.


Best regards,
Krzysztof

Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Tao Zhang]

Hi Suzuki,

On 3/28/2023 8:33 PM, Suzuki K Poulose wrote:
> On 28/03/2023 12:31, Tao Zhang wrote:
>> Hi Suzuki,
>>
>> On 3/27/2023 5:43 PM, Suzuki K Poulose wrote:
>>> On 27/03/2023 04:31, Tao Zhang wrote:
>>>>
>>>> On 3/26/2023 3:31 AM, Suzuki K Poulose wrote:
>>>>> On 24/03/2023 14:58, Tao Zhang wrote:
>>>>>> Hi Suzuki,
>>>>>>
>>>>>> 在 3/23/2023 7:51 PM, Suzuki K Poulose 写道:
>>>>>>> On 23/03/2023 06:03, Tao Zhang wrote:
>>>>>>>> Read the DSB element size from the device tree. Set the register
>>>>>>>> bit that controls the DSB element size of the corresponding port.
>>>>>>>>
>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>> ---
>>>>>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 58 
>>>>>>>> ++++++++++++++++++++++++++++
>>>>>>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>>>>>>   2 files changed, 62 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>>>> index f712e11..8dcfc4a 100644
>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>>>> @@ -21,6 +21,47 @@
>>>>>>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>>>>>>   +/* Search and read element data size from the TPDM node in
>>>>>>>> + * the devicetree. Each input port of TPDA is connected to
>>>>>>>> + * a TPDM. Different TPDM supports different types of dataset,
>>>>>>>> + * and some may support more than one type of dataset.
>>>>>>>> + * Parameter "inport" is used to pass in the input port number
>>>>>>>> + * of TPDA, and it is set to 0 in the recursize call.
>>>>>>>> + * Parameter "parent" is used to pass in the original call.
>>>>>>>> + */
>>>>>>>
>>>>>>> I am still not clear why we need to do this recursively ?
>>>>>>
>>>>>> Some TPDMs are not directly output connected to the TPDAs. So here I
>>>>>>
>>>>>> use a recursive method to check from the TPDA input port until I 
>>>>>> find
>>>>>>
>>>>>> the connected TPDM.
>>>>>>
>>>>>> Do you have a better suggestion besides a recursive method?
>>>>>>
>>>>>>>
>>>>>>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>>>>>> +               struct coresight_device *csdev, int inport, 
>>>>>>>> bool parent)
>>>>>>>
>>>>>>> Please could we renamse csdev => tpda_dev
>>>>>>
>>>>>> Since this is a recursively called function, this Coresight 
>>>>>> device is not
>>>>>>
>>>>>> necessarily TPDA, it can be other Coresight device.
>>>>>>
>>>>>>>
>>>>>>>> +{
>>>>>>>> +    static int nr_inport;
>>>>>>>> +    int i;
>>>>>>>> +    struct coresight_device *in_csdev;
>>>>>>>
>>>>>>> similarly tpdm_dev ?
>>>>>> Same as above, this variable may not necessarily be a TPDM.
>>>>>>>
>>>>>>> Could we not add a check here to see if the dsb_esize[inport] is 
>>>>>>> already
>>>>>>> set and then bail out, reading this over and over ?
>>>>>>>
>>>>>> I will update this in the next patch series.
>>>>>>>> +
>>>>>>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>>>>>>> +        return -EINVAL;
>>>>>>>> +
>>>>>>>> +    if (parent)
>>>>>>>> +        nr_inport = inport;
>>>>>>>> +
>>>>>>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>>>>>> +        in_csdev = csdev->pdata->in_conns[i].remote_dev;
>>>>>>>
>>>>>>> Please note, the names of the structure field might change in the
>>>>>>> next version of James' series
>>>>>> Got it. I will keep an eye out for the James' patch series.
>>>>>>>
>>>>>>>> +        if (!in_csdev)
>>>>>>>> +            break;
>>>>>>>> +
>>>>>>>> +        if (parent)
>>>>>>>> +            if (csdev->pdata->in_conns[i].port != inport)
>>>>>>>> +                continue;
>>>>>>>> +
>>>>>>>> +        if (in_csdev && strstr(dev_name(&in_csdev->dev), 
>>>>>>>> "tpdm")) {
>>>>>>>
>>>>>>> Isn't there a better way to distinguish a device to be TPDM ? 
>>>>>>> May be we
>>>>>>> could even add a source_sub_type - SOURCE_TPDM instead of using
>>>>>>> SOURCE_OTHERS ? Do you expect other sources to be connected to 
>>>>>>> TPDA?
>>>>>>> e.g., STMs ?
>>>>>>
>>>>>> I can add "SOURCE_TPDM" as a source_sub_type, but SOURCE_OTHERS 
>>>>>> needs
>>>>>>
>>>>>> to be kept since the other Coresight component we will upstream 
>>>>>> later may
>>>>>>
>>>>>> need it.
>>>>>>
>>>>>>>
>>>>>>>> + of_property_read_u32(in_csdev->dev.parent->of_node,
>>>>>>>> +                    "qcom,dsb-element-size", 
>>>>>>>> &drvdata->dsb_esize[nr_inport]);
>>>>>>>> +            break;
>>>>>>>> +        }
>>>>>>>> +        tpda_set_element_size(drvdata, in_csdev, 0, false);
>>>>>>>
>>>>>>> What is the point of this ? Is this for covering the a TPDA 
>>>>>>> connected to
>>>>>>> another TPDA ?
>>>>>>>
>>>>>>> e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?
>>>>>>
>>>>>> A TPDM may not connect to the TPDA directly, for example,
>>>>>>
>>>>>> TPDM0 ->FUNNEL0->FUNNEL1->TPDA0
>>>>>>
>>>>>> And many TPDMs can connect to one TPDA, one input port on TPDA 
>>>>>> only has
>>>>>>
>>>>>> one TPDM connected. Therefore, we use a recursive method to find 
>>>>>> the TPDM
>>>>>>
>>>>>> corresponding to the input port of TPDA.
>>>>>
>>>>> How do you find out decide what to choose, if there are multiple 
>>>>> TPDMs
>>>>> connected to FUNNEL0 or even FUNNEL1 ?
>>>>>
>>>>> e.g
>>>>>
>>>>> TPDM0->FUNNEL0->FUNNEL1->TPDA0
>>>>>                 /
>>>>>           TPDM1
>>>>
>>>> We can find out the corresponding TPDM by the input port number of 
>>>> TPDA.
>>>>
>>>> Each input port is connected to a TPDM. So we have an input port 
>>>> number in
>>>>
>>>> the input parameter of the recursive lookup function 
>>>> "tpda_set_element_size".
>>>
>>> I don't understand, how you would figure out, in the above situation.
>>> i.e., FUNNEL1 is connected to TPDA0, but there are two TPDMs that could
>>> be pumping the trace. They both arrive via FUNNEL1. So, how does that
>>> solve your problem ?
>>
>> In our HW design, the input ports of TPDA and TPDM are one-one-one 
>> corresponding.  Only one
>>
>> TPDM can be found connected from one TPDA's input port. The path to a 
>> TPDA input port doesn't
>>
>> connect more than one TPDM. It's by HW design.
>
> Your current designs may be like that. But as far as the driver is
> concerned, I would like to add in extra measures to ensure that it
> encounters a variation from the above on a future platform. So, please
> could you add a check to detect this case and add a WARNING ?

Got it, I will update it according to your advice in the next patch series.


Tao

>
> Suzuki
>
>
>>
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>>>
>>>>
>>>>> Suzuki
>>>>>
>>>>>
>>>
>

Re: [PATCH v3 05/11] coresight-tpdm: Add nodes to set trigger timestamp and type

[Suzuki K Poulose]

On 23/03/2023 06:04, Tao Zhang wrote:
> The nodes are needed to set or show the trigger timestamp and
> trigger type. This change is to add these nodes to achieve these
> function.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 24 ++++++
>   drivers/hwtracing/coresight/coresight-tpdm.c       | 95 ++++++++++++++++++++++
>   2 files changed, 119 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index 4a58e64..27ce681 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -11,3 +11,27 @@ Description:
>   		Accepts only one of the 2 values -  1 or 2.
>   		1 : Generate 64 bits data
>   		2 : Generate 32 bits data
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_trig_type
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the trigger type of DSB tpdm. Read the trigger
> +		type of DSB tpdm.
> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Set the DSB trigger type to false
> +		1 : Set the DSB trigger type to true
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_trig_ts
> +Date:		March 2023
> +KernelVersion	6.3
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the trigger timestamp of DSB tpdm. Read the
> +		trigger timestamp of DSB tpdm.
> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Set the DSB trigger type to false
> +		1 : Set the DSB trigger type to true
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 104638d..e28cf10 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -20,6 +20,19 @@
>   
>   DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
>   
> +static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
> +							struct attribute *attr, int n)

minor nit: alignment. extra tabs

> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	if (drvdata)
> +		if (drvdata->datasets & TPDM_PIDR0_DS_DSB)
> +			return attr->mode;

minor nit:
	if (drvdata && drvdata->dataset & TPDM_PIDR0_DS_DSB) ?

> +
> +	return 0;
> +}
> +
>   static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
>   {
>   	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
> @@ -237,8 +250,90 @@ static struct attribute_group tpdm_attr_grp = {
>   	.attrs = tpdm_attrs,
>   };
>   
> +static ssize_t dsb_trig_type_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->dsb->trig_type);
> +}
> +
> +/*
> + * value 0: set trigger type as enablement
> + * value 1: set trigger type as disablement
> + */

minor nit: The above looks confusing.

   /*
    * Trigger type (boolean):
    *   false - Disable trigger type.
    *   true  - Enable trigger type.
    */

> +static ssize_t dsb_trig_type_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->dsb->trig_type = true;
> +	else
> +		drvdata->dsb->trig_type = false;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(dsb_trig_type);
> +
> +static ssize_t dsb_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->dsb->trig_ts);
> +}
> +
> +/*
> + * value 0: set trigger timestamp as enablement
> + * value 1: set trigger timestamp as disablement
> + */

Same as above.


Otherwise looks good to me

Suzuki