Introduction of TPDM CMB(Continuous Multi Bit) subunit
CMB subunit is responsible for creating a dataset element, and is also
optionally responsible for packing it to fit multiple elements on a
single ATB transfer if possible in the configuration. The TPDM Core
Datapath requests timestamps be stored by the TPDA and then delivering
ATB sized data (depending on ATB width and element size, this could
be smaller or larger than a dataset element) to the ATB Mast FSM.
The CMB makes trace elements in two modes. In ?continuous? mode, every
valid data cycle creates an element. In ?trace on change? mode, when
valid data changes on the bus, a trace element is created. In
continuous mode, all cycles where this condition is true create trace
elements. In trace on change mode, a data element is only when the
previously sampled input is different from the current sampled input.
The CMB subunit must be configured prior to enablement. This series
adds support for TPDM to configure the configure CMB subunit.
Once this series patches are applied properly, the new tpdm nodes for
should be observed at the tpdm path /sys/bus/coresight/devices/tpdm*
which supports CMB subunit.
e.g.
root@qemuarm64:/sys/devices/platform/soc@0/684c000.tpdm/tpdm0# ls -l
-rw-r--r-- 1 root root 4096 Jan 1 00:00 cmb_mode
drwxr-xr-x 2 root root 0 Jan 1 00:00 cmb_msr
drwxr-xr-x 2 root root 0 Jan 1 00:00 cmb_patt
drwxr-xr-x 2 root root 0 Jan 1 00:00 cmb_trig_patt
-rw-r--r-- 1 root root 4096 Jan 1 00:00 cmb_trig_ts
-rw-r--r-- 1 root root 4096 Jan 1 00:00 cmb_ts_all
drwxr-xr-x 2 root root 0 Jan 1 00:00 connections
drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_edge
drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_msr
drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_patt
drwxr-xr-x 2 root root 0 Jan 1 00:00 dsb_trig_patt
-rw-r--r-- 1 root root 4096 Jan 1 00:00 enable_source
--w------- 1 root root 4096 Jan 1 00:00 integration_test
drwxr-xr-x 2 root root 0 Ja? 1 00:00 power
--w------- 1 root root 4096 Jan 1 00:00 reset_dataset
lrwxrwxrwx 1 root root 0 Apr 5 2021 subsystem -> ../../../../../bus/coresight
-rw-r--r-- 1 root root 4096 Apr 5 2021 uevent
-r--r--r-- 1 root root 4096 Jan 1 00:00 waiting_for_supplier
We can use the commands are similar to the below to configure the
TPDMs which support CMB subunit. Enable coresight sink first.
echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink
echo 1 > /sys/bus/coresight/devices/tpdm0/reset_dataset
echo 1 > /sys/bus/coresight/devices/tpdm0/cmb_mode
echo 1 > /sys/bus/coresight/devices/tpdm0/cmb_patt/enable_ts
echo 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/cmb_patt/tpmr0
echo 0 > /sys/bus/coresight/devices/tpdm0/cmb_trig_ts
echo 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/cmb_trig_patt/xpr1
echo 1 > /sys/bus/coresight/devices/tpdm0/enable_source
codelinaro link:
https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/tpdm-cmb-v5
Changes in V5:
1. Return directly to refine the process of "tpdm_enable_dsb" and
"tpdm_disable_dsb" if the TPDM does not support DSB dataset.
-- Suzuki K Poulose
2. Change the new property name from "qcom,cmb-element-size" to
"qcom,cmb-element-bits". And update the property name to the new
tpdm example which supports the CMB dataset.
-- Krzysztof
3. Drop unnecessary () around the drvdata member access in patch#5.
-- Suzuki K Poulose
4. Replace the error code "-EEXIST" with the port number in detecting
multiple TPDMs on one input port. In patch#5.
-- Suzuki K Poulose
5. Due to the modification of the new property name, the searched
property name was changed from "qcom,cmb-element-size" to
"qcom,cmb-element-bits". In patch#5.
6. Replace the judgement "tpdm_has_cmb_dataset(drvdata)" with
"drvdata->cmb" in the function "tpdm_reset_datasets". In patch#6.
-- Suzuki K Poulose
7. Drop unnecessary () around kstrtoul() in the function
"cmb_mode_store". In patch#6.
-- Suzuki K Poulose
8. Update the date and the kernel version in the Document.
-- Suzuki K Poulose
9. Initialize the return value to "-EINVAL" and remove the unnecessary
code in the function "enable_ts_show". In patch#8.
-- Suzuki K Poulose
10. Replace spin lock/unlock with guard(spinlock). In patch#8.
-- Suzuki K Poulose
11. Drop _rw from the macro "tpdm_patt_enable_ts_rw". In patch#8.
-- Suzuki K Poulose
Changes in V4:
1. Replace spin lock/unlock to avoid forgetting to unlock when the
function exits.
-- Suzuki K Poulose
2. Move the helper "tpdm_has_dsb_dataset" to the header file.
-- Suzuki K Poulose
3. Fix the incorrect property of the sample in the documents.
-- James Clark
4. Clear the dsb/cmb element size directly in the clear helper.
-- Suzuki K Poulose
5. Correct the comment of "tpdm_read_element_size".
-- Suzuki K Poulose
6. Call the helper "tpdm_has_dsb/cmb_dataset" in TPDA driver to
check what dataset the TPDM supports.
-- Suzuki K Poulose
7. Refine the dsb/cmb dataset support check in enable/disable functions.
-- Suzuki K Poulose
8. Get rid of redundant code in function "set_cmb_tier".
-- Suzuki K Poulose
9. Since one SysFs file should follow "one value", use "dev_ext_attribute"
to instead of the previous "enable_ts" Sysfs file approach.
-- Suzuki K Poulose
10. Change the kernel version to 6.9 for the MSR related SysFs file.
-- James Clark
11. Refine the function "tpdm_simple_dataset_store".
-- Suzuki K Poulose
Changes in V3:
1. Add 8-bit support to the description in the TPDM devicetree document.
-- Rob Herring
2. Change how the result is produced in "tpdm_read_element_size".
-- James Clark
3. Calling "tpdm_clear_element_size" at the beginning of
"tpda_enable_port".
-- James Clark
4. Use "dsb_esize" and "cmb_esize" to determine whether multiple TPDMs
are detected on a TPDA input port in "tpda_get_element_size".
-- James Clark
5. Modify the judgment logic in "tpda_enable_port".
-- James Clark
6. Add more description of "cmb_mode" to TPDM SysFS document.
-- James Clark
Changes in V2:
1. Optimizate and modify this patch series based on the patch series
"Add support to configure TPDM CMB subunit".
2. Modify the functions that read the element size of DSB/CMB in TPDA driver.
Tao Zhang (10):
coresight-tpdm: Optimize the store function of tpdm simple dataset
coresight-tpdm: Optimize the useage of tpdm_has_dsb_dataset
dt-bindings: arm: qcom,coresight-tpdm: Add support for CMB element
size
coresight-tpdm: Add CMB dataset support
coresight-tpda: Add support to configure CMB element
coresight-tpdm: Add support to configure CMB
coresight-tpdm: Add pattern registers support for CMB
coresight-tpdm: Add timestamp control register support for the CMB
dt-bindings: arm: qcom,coresight-tpdm: Add support for TPDM CMB MSR
register
coresight-tpdm: Add msr register support for CMB
.../testing/sysfs-bus-coresight-devices-tpdm | 87 ++++
.../bindings/arm/qcom,coresight-tpdm.yaml | 35 ++
drivers/hwtracing/coresight/coresight-tpda.c | 123 +++--
drivers/hwtracing/coresight/coresight-tpda.h | 6 +
drivers/hwtracing/coresight/coresight-tpdm.c | 455 ++++++++++++++++--
drivers/hwtracing/coresight/coresight-tpdm.h | 113 +++++
6 files changed, 727 insertions(+), 92 deletions(-)
--
2.17.1
Since the function tpdm_has_dsb_dataset will be called by TPDA
driver in subsequent patches, it is moved to the header file.
And move this judgement form the function __tpdm_{enable/disable}
to the beginning of the function tpdm_{enable/disable}_dsb.
Signed-off-by: Tao Zhang <[email protected]>
---
drivers/hwtracing/coresight/coresight-tpdm.c | 30 +++++++++-----------
drivers/hwtracing/coresight/coresight-tpdm.h | 4 +++
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 0427c0fc0bf3..4b1296d11360 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -125,11 +125,6 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
return ret;
}
-static bool tpdm_has_dsb_dataset(struct tpdm_drvdata *drvdata)
-{
- return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
-}
-
static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
struct attribute *attr, int n)
{
@@ -232,25 +227,27 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
{
u32 val, i;
+ if (!tpdm_has_dsb_dataset(drvdata))
+ return;
+
for (i = 0; i < TPDM_DSB_MAX_EDCR; i++)
writel_relaxed(drvdata->dsb->edge_ctrl[i],
- drvdata->base + TPDM_DSB_EDCR(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));
+ 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));
+ drvdata->base + TPDM_DSB_TPR(i));
writel_relaxed(drvdata->dsb->patt_mask[i],
- drvdata->base + TPDM_DSB_TPMR(i));
+ drvdata->base + TPDM_DSB_TPMR(i));
writel_relaxed(drvdata->dsb->trig_patt[i],
- drvdata->base + TPDM_DSB_XPR(i));
+ drvdata->base + TPDM_DSB_XPR(i));
writel_relaxed(drvdata->dsb->trig_patt_mask[i],
- drvdata->base + TPDM_DSB_XPMR(i));
+ drvdata->base + TPDM_DSB_XPMR(i));
}
set_dsb_tier(drvdata);
-
set_dsb_msr(drvdata);
val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
@@ -278,8 +275,7 @@ static void __tpdm_enable(struct tpdm_drvdata *drvdata)
{
CS_UNLOCK(drvdata->base);
- if (tpdm_has_dsb_dataset(drvdata))
- tpdm_enable_dsb(drvdata);
+ tpdm_enable_dsb(drvdata);
CS_LOCK(drvdata->base);
}
@@ -307,6 +303,9 @@ static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
{
u32 val;
+ if (!tpdm_has_dsb_dataset(drvdata))
+ return;
+
/* Set the enable bit of DSB control register to 0 */
val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
val &= ~TPDM_DSB_CR_ENA;
@@ -318,8 +317,7 @@ static void __tpdm_disable(struct tpdm_drvdata *drvdata)
{
CS_UNLOCK(drvdata->base);
- if (tpdm_has_dsb_dataset(drvdata))
- tpdm_disable_dsb(drvdata);
+ tpdm_disable_dsb(drvdata);
CS_LOCK(drvdata->base);
}
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 4115b2a17b8d..ddaf333fa1c2 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -220,4 +220,8 @@ struct tpdm_dataset_attribute {
u32 idx;
};
+static bool tpdm_has_dsb_dataset(struct tpdm_drvdata *drvdata)
+{
+ return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
+}
#endif /* _CORESIGHT_CORESIGHT_TPDM_H */
--
2.17.1
Add property "qcom,cmb-elem-bits" to support CMB(Continuous
Multi-Bit) element for TPDM. The associated aggregator will read
this size before it is enabled. CMB element size currently only
supports 8-bit, 32-bit and 64-bit. Because the existing example
tpdm "tpdm@684c000" which only supports dsb sub-unit, I introduce
a new example "tpdm@6c29000" to describe the usage of this new
property.
Signed-off-by: Tao Zhang <[email protected]>
Signed-off-by: Mao Jinlong <[email protected]>
---
.../bindings/arm/qcom,coresight-tpdm.yaml | 25 +++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
index 61ddc3b5b247..2320b5445900 100644
--- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
@@ -52,6 +52,14 @@ properties:
$ref: /schemas/types.yaml#/definitions/uint8
enum: [32, 64]
+ qcom,cmb-element-bits:
+ description:
+ Specifies the CMB(Continuous Multi-Bit) element size supported by
+ the monitor. The associated aggregator will read this size before it
+ is enabled. CMB element size currently only supports 8-bit, 32-bit
+ and 64-bit.
+ enum: [8, 32, 64]
+
qcom,dsb-msrs-num:
description:
Specifies the number of DSB(Discrete Single Bit) MSR(mux select register)
@@ -110,4 +118,21 @@ examples:
};
};
+ tpdm@6c29000 {
+ compatible = "qcom,coresight-tpdm", "arm,primecell";
+ reg = <0x06c29000 0x1000>;
+
+ qcom,cmb-element-bits = <64>;
+
+ clocks = <&aoss_qmp>;
+ clock-names = "apb_pclk";
+
+ out-ports {
+ port {
+ tpdm_ipcc_out_funnel_center: endpoint {
+ remote-endpoint = <&funnel_center_in_tpdm_ipcc>;
+ };
+ };
+ };
+ };
...
--
2.17.1
Timestamps are requested if the monitor’s CMB data set unit input
data matches the value in the Monitor CMB timestamp pattern and mask
registers (M_CMB_TPR and M_CMB_TPMR) when CMB timestamp enabled
via the timestamp insertion enable register bit(CMB_TIER.PATT_TSENAB).
The pattern match trigger output is achieved via setting values into
the CMB trigger pattern and mask registers (CMB_XPR and CMB_XPMR).
After configuring a pattern through these registers, the TPDM subunit
will assert an output trigger every time it receives new input data
that matches the configured pattern value. Values in a given bit
number of the mask register correspond to the same bit number in
the corresponding pattern register.
Reviewed-by: James Clark <[email protected]>
Signed-off-by: Tao Zhang <[email protected]>
Signed-off-by: Jinlong Mao <[email protected]>
---
.../testing/sysfs-bus-coresight-devices-tpdm | 30 ++++++
drivers/hwtracing/coresight/coresight-tpdm.c | 96 ++++++++++++++++++-
drivers/hwtracing/coresight/coresight-tpdm.h | 39 ++++++++
3 files changed, 164 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index 6dfb18d6d64a..b6cf050861ed 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -184,3 +184,33 @@ Description: (Write) Set the data collection mode of CMB tpdm. Continuous
Accepts only one of the 2 values - 0 or 1.
0 : Continuous CMB collection mode.
1 : Trace-on-change CMB collection mode.
+
+What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpr[0:1]
+Date: January 2024
+KernelVersion 6.9
+Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
+Description:
+ (RW) Set/Get the value of the trigger pattern for the CMB
+ subunit TPDM.
+
+What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpmr[0:1]
+Date: January 2024
+KernelVersion 6.9
+Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
+Description:
+ (RW) Set/Get the mask of the trigger pattern for the CMB
+ subunit TPDM.
+
+What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpr[0:1]
+Date: January 2024
+KernelVersion 6.9
+Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
+Description:
+ (RW) Set/Get the value of the pattern for the CMB subunit TPDM.
+
+What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpmr[0:1]
+Date: January 2024
+KernelVersion 6.9
+Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
+Description:
+ (RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index b20071460375..07587287d9fa 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -66,6 +66,26 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev,
return -EINVAL;
return sysfs_emit(buf, "0x%x\n",
drvdata->dsb->msr[tpdm_attr->idx]);
+ case CMB_TRIG_PATT:
+ if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+ return -EINVAL;
+ return sysfs_emit(buf, "0x%x\n",
+ drvdata->cmb->trig_patt[tpdm_attr->idx]);
+ case CMB_TRIG_PATT_MASK:
+ if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+ return -EINVAL;
+ return sysfs_emit(buf, "0x%x\n",
+ drvdata->cmb->trig_patt_mask[tpdm_attr->idx]);
+ case CMB_PATT:
+ if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+ return -EINVAL;
+ return sysfs_emit(buf, "0x%x\n",
+ drvdata->cmb->patt_val[tpdm_attr->idx]);
+ case CMB_PATT_MASK:
+ if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
+ return -EINVAL;
+ return sysfs_emit(buf, "0x%x\n",
+ drvdata->cmb->patt_mask[tpdm_attr->idx]);
}
return -EINVAL;
}
@@ -118,6 +138,30 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
ret = size;
}
break;
+ case CMB_TRIG_PATT:
+ if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
+ drvdata->cmb->trig_patt[tpdm_attr->idx] = val;
+ ret = size;
+ }
+ break;
+ case CMB_TRIG_PATT_MASK:
+ if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
+ drvdata->cmb->trig_patt_mask[tpdm_attr->idx] = val;
+ ret = size;
+ }
+ break;
+ case CMB_PATT:
+ if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
+ drvdata->cmb->patt_val[tpdm_attr->idx] = val;
+ ret = size;
+ }
+ break;
+ case CMB_PATT_MASK:
+ if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
+ drvdata->cmb->patt_mask[tpdm_attr->idx] = val;
+ ret = size;
+ }
+ break;
default:
break;
}
@@ -280,12 +324,32 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
{
- u32 val;
+ u32 val, i;
if (!tpdm_has_cmb_dataset(drvdata))
return;
+ /* Configure pattern registers */
+ for (i = 0; i < TPDM_CMB_MAX_PATT; i++) {
+ writel_relaxed(drvdata->cmb->patt_val[i],
+ drvdata->base + TPDM_CMB_TPR(i));
+ writel_relaxed(drvdata->cmb->patt_mask[i],
+ drvdata->base + TPDM_CMB_TPMR(i));
+ writel_relaxed(drvdata->cmb->trig_patt[i],
+ drvdata->base + TPDM_CMB_XPR(i));
+ writel_relaxed(drvdata->cmb->trig_patt_mask[i],
+ drvdata->base + TPDM_CMB_XPMR(i));
+ }
+
val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
+ /*
+ * Set to 0 for continuous CMB collection mode,
+ * 1 for trace-on-change CMB collection mode.
+ */
+ if (drvdata->cmb->trace_mode)
+ val |= TPDM_CMB_CR_MODE;
+ else
+ val &= ~TPDM_CMB_CR_MODE;
/* Set the enable bit of CMB control register to 1 */
val |= TPDM_CMB_CR_ENA;
writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
@@ -887,6 +951,22 @@ static struct attribute *tpdm_dsb_msr_attrs[] = {
NULL,
};
+static struct attribute *tpdm_cmb_trig_patt_attrs[] = {
+ CMB_TRIG_PATT_ATTR(0),
+ CMB_TRIG_PATT_ATTR(1),
+ CMB_TRIG_PATT_MASK_ATTR(0),
+ CMB_TRIG_PATT_MASK_ATTR(1),
+ NULL,
+};
+
+static struct attribute *tpdm_cmb_patt_attrs[] = {
+ CMB_PATT_ATTR(0),
+ CMB_PATT_ATTR(1),
+ CMB_PATT_MASK_ATTR(0),
+ CMB_PATT_MASK_ATTR(1),
+ NULL,
+};
+
static struct attribute *tpdm_dsb_attrs[] = {
&dev_attr_dsb_mode.attr,
&dev_attr_dsb_trig_ts.attr,
@@ -933,6 +1013,18 @@ static struct attribute_group tpdm_cmb_attr_grp = {
.is_visible = tpdm_cmb_is_visible,
};
+static struct attribute_group tpdm_cmb_trig_patt_grp = {
+ .attrs = tpdm_cmb_trig_patt_attrs,
+ .is_visible = tpdm_cmb_is_visible,
+ .name = "cmb_trig_patt",
+};
+
+static struct attribute_group tpdm_cmb_patt_grp = {
+ .attrs = tpdm_cmb_patt_attrs,
+ .is_visible = tpdm_cmb_is_visible,
+ .name = "cmb_patt",
+};
+
static const struct attribute_group *tpdm_attr_grps[] = {
&tpdm_attr_grp,
&tpdm_dsb_attr_grp,
@@ -941,6 +1033,8 @@ static const struct attribute_group *tpdm_attr_grps[] = {
&tpdm_dsb_patt_grp,
&tpdm_dsb_msr_grp,
&tpdm_cmb_attr_grp,
+ &tpdm_cmb_trig_patt_grp,
+ &tpdm_cmb_patt_grp,
NULL,
};
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 2af92c270ed1..8cb8a9b35384 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -11,12 +11,23 @@
/* CMB Subunit Registers */
#define TPDM_CMB_CR (0xA00)
+/*CMB subunit timestamp pattern registers*/
+#define TPDM_CMB_TPR(n) (0xA08 + (n * 4))
+/*CMB subunit timestamp pattern mask registers*/
+#define TPDM_CMB_TPMR(n) (0xA10 + (n * 4))
+/*CMB subunit trigger pattern registers*/
+#define TPDM_CMB_XPR(n) (0xA18 + (n * 4))
+/*CMB subunit trigger pattern mask registers*/
+#define TPDM_CMB_XPMR(n) (0xA20 + (n * 4))
/* Enable bit for CMB subunit */
#define TPDM_CMB_CR_ENA BIT(0)
/* Trace collection mode for CMB subunit */
#define TPDM_CMB_CR_MODE BIT(1)
+/*Patten register number*/
+#define TPDM_CMB_MAX_PATT 2
+
/* DSB Subunit Registers */
#define TPDM_DSB_CR (0x780)
#define TPDM_DSB_TIER (0x784)
@@ -151,6 +162,22 @@
tpdm_simple_dataset_rw(msr##nr, \
DSB_MSR, nr)
+#define CMB_TRIG_PATT_ATTR(nr) \
+ tpdm_simple_dataset_rw(xpr##nr, \
+ CMB_TRIG_PATT, nr)
+
+#define CMB_TRIG_PATT_MASK_ATTR(nr) \
+ tpdm_simple_dataset_rw(xpmr##nr, \
+ CMB_TRIG_PATT_MASK, nr)
+
+#define CMB_PATT_ATTR(nr) \
+ tpdm_simple_dataset_rw(tpr##nr, \
+ CMB_PATT, nr)
+
+#define CMB_PATT_MASK_ATTR(nr) \
+ tpdm_simple_dataset_rw(tpmr##nr, \
+ CMB_PATT_MASK, nr)
+
/**
* struct dsb_dataset - specifics associated to dsb dataset
* @mode: DSB programming mode
@@ -186,9 +213,17 @@ struct dsb_dataset {
/**
* struct cmb_dataset
* @trace_mode: Dataset collection mode
+ * @patt_val: Save value for pattern
+ * @patt_mask: Save value for pattern mask
+ * @trig_patt: Save value for trigger pattern
+ * @trig_patt_mask: Save value for trigger pattern mask
*/
struct cmb_dataset {
u32 trace_mode;
+ u32 patt_val[TPDM_CMB_MAX_PATT];
+ u32 patt_mask[TPDM_CMB_MAX_PATT];
+ u32 trig_patt[TPDM_CMB_MAX_PATT];
+ u32 trig_patt_mask[TPDM_CMB_MAX_PATT];
};
/**
@@ -225,6 +260,10 @@ enum dataset_mem {
DSB_PATT,
DSB_PATT_MASK,
DSB_MSR,
+ CMB_TRIG_PATT,
+ CMB_TRIG_PATT_MASK,
+ CMB_PATT,
+ CMB_PATT_MASK
};
/**
--
2.17.1
Add property "qcom,cmb_msr_num" to support CMB MSR(mux select register)
for TPDM. It specifies the number of CMB MSR registers supported by
the TDPM.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Tao Zhang <[email protected]>
Signed-off-by: Mao Jinlong <[email protected]>
---
.../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 2320b5445900..d0647ffaed71 100644
--- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpdm.yaml
@@ -69,6 +69,15 @@ properties:
minimum: 0
maximum: 32
+ qcom,cmb-msrs-num:
+ description:
+ Specifies the number of CMB MSR(mux select register) registers supported
+ by the monitor. If this property is not configured or set to 0, it means
+ this TPDM doesn't support CMB MSR.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 32
+
clocks:
maxItems: 1
@@ -123,6 +132,7 @@ examples:
reg = <0x06c29000 0x1000>;
qcom,cmb-element-bits = <64>;
+ qcom,cmb-msrs-num = <32>;
clocks = <&aoss_qmp>;
clock-names = "apb_pclk";
--
2.17.1
Read the CMB element size from the device tree. Set the register
bit that controls the CMB element size of the corresponding port.
Reviewed-by: James Clark <[email protected]>
Signed-off-by: Tao Zhang <[email protected]>
Signed-off-by: Mao Jinlong <[email protected]>
---
drivers/hwtracing/coresight/coresight-tpda.c | 123 +++++++++++--------
drivers/hwtracing/coresight/coresight-tpda.h | 6 +
2 files changed, 79 insertions(+), 50 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 4ac954f4bc13..fcddff3ded81 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -18,6 +18,7 @@
#include "coresight-priv.h"
#include "coresight-tpda.h"
#include "coresight-trace-id.h"
+#include "coresight-tpdm.h"
DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
@@ -28,24 +29,57 @@ static bool coresight_device_is_tpdm(struct coresight_device *csdev)
CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
}
+static void tpdm_clear_element_size(struct coresight_device *csdev)
+{
+ struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ drvdata->dsb_esize = 0;
+ drvdata->cmb_esize = 0;
+}
+
+static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32 *val)
+{
+
+ if (drvdata->dsb_esize == 64)
+ *val |= TPDA_Pn_CR_DSBSIZE;
+ else if (drvdata->dsb_esize == 32)
+ *val &= ~TPDA_Pn_CR_DSBSIZE;
+
+ if (drvdata->cmb_esize == 64)
+ *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
+ else if (drvdata->cmb_esize == 32)
+ *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
+ else if (drvdata->cmb_esize == 8)
+ *val &= ~TPDA_Pn_CR_CMBSIZE;
+}
+
/*
- * Read the DSB element size from the TPDM device
+ * Read the element size from the TPDM device. One TPDM must have at least one of the
+ * element size property.
* Returns
- * The dsb element size read from the devicetree if available.
- * 0 - Otherwise, with a warning once.
+ * 0 - The element size property is read
+ * Others - Cannot read the property of the element size
*/
-static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
+static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
+ struct coresight_device *csdev)
{
- int rc = 0;
- u8 size = 0;
+ int rc = -EINVAL;
+ struct tpdm_drvdata *tpdm_data = dev_get_drvdata(csdev->dev.parent);
+
+ if (tpdm_has_dsb_dataset(tpdm_data)) {
+ rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
+ "qcom,dsb-element-size", &drvdata->dsb_esize);
+ }
+ if (tpdm_has_cmb_dataset(tpdm_data)) {
+ rc = fwnode_property_read_u32(dev_fwnode(csdev->dev.parent),
+ "qcom,cmb-element-bits", &drvdata->cmb_esize);
+ }
- rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
- "qcom,dsb-element-size", &size);
if (rc)
dev_warn_once(&csdev->dev,
- "Failed to read TPDM DSB Element size: %d\n", rc);
+ "Failed to read TPDM Element size: %d\n", rc);
- return size;
+ return rc;
}
/*
@@ -56,11 +90,12 @@ static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
* Parameter "inport" is used to pass in the input port number
* of TPDA, and it is set to -1 in the recursize call.
*/
-static int tpda_get_element_size(struct coresight_device *csdev,
+static int tpda_get_element_size(struct tpda_drvdata *drvdata,
+ struct coresight_device *csdev,
int inport)
{
- int dsb_size = -ENOENT;
- int i, size;
+ int rc = 0;
+ int i;
struct coresight_device *in;
for (i = 0; i < csdev->pdata->nr_inconns; i++) {
@@ -69,30 +104,26 @@ static int tpda_get_element_size(struct coresight_device *csdev,
continue;
/* Ignore the paths that do not match port */
- if (inport > 0 &&
+ if (inport >= 0 &&
csdev->pdata->in_conns[i]->dest_port != inport)
continue;
if (coresight_device_is_tpdm(in)) {
- size = tpdm_read_dsb_element_size(in);
+ if (drvdata->dsb_esize || drvdata->cmb_esize)
+ return -EEXIST;
+ rc = tpdm_read_element_size(drvdata, in);
+ if (rc)
+ return rc;
} else {
/* Recurse down the path */
- size = tpda_get_element_size(in, -1);
- }
-
- if (size < 0)
- return size;
-
- if (dsb_size < 0) {
- /* Found a size, save it. */
- dsb_size = size;
- } else {
- /* Found duplicate TPDMs */
- return -EEXIST;
+ rc = tpda_get_element_size(drvdata, in, -1);
+ if (rc)
+ return rc;
}
}
- return dsb_size;
+
+ return rc;
}
/* Settings pre enabling port control register */
@@ -109,7 +140,7 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
{
u32 val;
- int size;
+ int rc;
val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
/*
@@ -117,29 +148,21 @@ static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
* Set the bit to 0 if the size is 32
* Set the bit to 1 if the size is 64
*/
- size = tpda_get_element_size(drvdata->csdev, port);
- switch (size) {
- case 32:
- val &= ~TPDA_Pn_CR_DSBSIZE;
- break;
- case 64:
- val |= TPDA_Pn_CR_DSBSIZE;
- break;
- case 0:
- return -EEXIST;
- case -EEXIST:
+ tpdm_clear_element_size(drvdata->csdev);
+ rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
+ if (!rc && (drvdata->dsb_esize || drvdata->cmb_esize)) {
+ tpda_set_element_size(drvdata, &val);
+ /* Enable the port */
+ val |= TPDA_Pn_CR_ENA;
+ writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
+ } else if (rc == -EEXIST)
dev_warn_once(&drvdata->csdev->dev,
- "Detected multiple TPDMs on port %d", -EEXIST);
- return -EEXIST;
- default:
- return -EINVAL;
- }
-
- /* Enable the port */
- val |= TPDA_Pn_CR_ENA;
- writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
+ "Detected multiple TPDMs on port %d", port);
+ else
+ dev_warn_once(&drvdata->csdev->dev,
+ "Didn't find TPDM element size");
- return 0;
+ return rc;
}
static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
index b3b38fd41b64..19af64120fcf 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.h
+++ b/drivers/hwtracing/coresight/coresight-tpda.h
@@ -10,6 +10,8 @@
#define TPDA_Pn_CR(n) (0x004 + (n * 4))
/* Aggregator port enable bit */
#define TPDA_Pn_CR_ENA BIT(0)
+/* Aggregator port CMB data set element size bit */
+#define TPDA_Pn_CR_CMBSIZE GENMASK(7, 6)
/* Aggregator port DSB data set element size bit */
#define TPDA_Pn_CR_DSBSIZE BIT(8)
@@ -25,6 +27,8 @@
* @csdev: component vitals needed by the framework.
* @spinlock: lock for the drvdata value.
* @enable: enable status of the component.
+ * @dsb_esize Record the DSB element size.
+ * @cmb_esize Record the CMB element size.
*/
struct tpda_drvdata {
void __iomem *base;
@@ -32,6 +36,8 @@ struct tpda_drvdata {
struct coresight_device *csdev;
spinlock_t spinlock;
u8 atid;
+ u8 dsb_esize;
+ u32 cmb_esize;
};
#endif /* _CORESIGHT_CORESIGHT_TPDA_H */
--
2.17.1
On 30/01/2024 09:02, Tao Zhang wrote:
> Read the CMB element size from the device tree. Set the register
> bit that controls the CMB element size of the corresponding port.
>
> Reviewed-by: James Clark <[email protected]>
> Signed-off-by: Tao Zhang <[email protected]>
> Signed-off-by: Mao Jinlong <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight-tpda.c | 123 +++++++++++--------
> drivers/hwtracing/coresight/coresight-tpda.h | 6 +
> 2 files changed, 79 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 4ac954f4bc13..fcddff3ded81 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -18,6 +18,7 @@
> #include "coresight-priv.h"
> #include "coresight-tpda.h"
> #include "coresight-trace-id.h"
> +#include "coresight-tpdm.h"
>
> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>
> @@ -28,24 +29,57 @@ static bool coresight_device_is_tpdm(struct coresight_device *csdev)
> CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
> }
>
> +static void tpdm_clear_element_size(struct coresight_device *csdev)
> +{
> + struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + drvdata->dsb_esize = 0;
> + drvdata->cmb_esize = 0;
> +}
> +
> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32 *val)
> +{
> +
> + if (drvdata->dsb_esize == 64)
> + *val |= TPDA_Pn_CR_DSBSIZE;
We don't seem to be clearing the fields we modify, before updating them.
This may be OK in real world where the device connected to TPDA port
may not change. But it is always safer to clear the bits and set it.
e.g.:
*val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);
> + else if (drvdata->dsb_esize == 32)
> + *val &= ~TPDA_Pn_CR_DSBSIZE;
> +
> + if (drvdata->cmb_esize == 64)
> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
> + else if (drvdata->cmb_esize == 32)
> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
Similarly here ^^^. I am happy to fix it up if you are OK with it
(unless there are other changes that need a respin)
> + else if (drvdata->cmb_esize == 8)
> + *val &= ~TPDA_Pn_CR_CMBSIZE;
> +}
> +
> /*
> - * Read the DSB element size from the TPDM device
> + * Read the element size from the TPDM device. One TPDM must have at least one of the
> + * element size property.
> * Returns
> - * The dsb element size read from the devicetree if available.
> - * 0 - Otherwise, with a warning once.
> + * 0 - The element size property is read
> + * Others - Cannot read the property of the element size
> */
> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
> + struct coresight_device *csdev)
> {
> - int rc = 0;
> - u8 size = 0;
> + int rc = -EINVAL;
> + struct tpdm_drvdata *tpdm_data = dev_get_drvdata(csdev->dev.parent);
> +
> + if (tpdm_has_dsb_dataset(tpdm_data)) {
> + rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> + "qcom,dsb-element-size", &drvdata->dsb_esize);
> + }
> + if (tpdm_has_cmb_dataset(tpdm_data)) {
> + rc = fwnode_property_read_u32(dev_fwnode(csdev->dev.parent),
> + "qcom,cmb-element-bits", &drvdata->cmb_esize);
> + }
>
> - rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> - "qcom,dsb-element-size", &size);
> if (rc)
> dev_warn_once(&csdev->dev,
> - "Failed to read TPDM DSB Element size: %d\n", rc);
> + "Failed to read TPDM Element size: %d\n", rc);
>
> - return size;
> + return rc;
> }
>
> /*
> @@ -56,11 +90,12 @@ static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
> * Parameter "inport" is used to pass in the input port number
> * of TPDA, and it is set to -1 in the recursize call.
> */
> -static int tpda_get_element_size(struct coresight_device *csdev,
> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
> + struct coresight_device *csdev,
> int inport)
> {
> - int dsb_size = -ENOENT;
> - int i, size;
> + int rc = 0;
> + int i;
> struct coresight_device *in;
>
> for (i = 0; i < csdev->pdata->nr_inconns; i++) {
> @@ -69,30 +104,26 @@ static int tpda_get_element_size(struct coresight_device *csdev,
> continue;
>
> /* Ignore the paths that do not match port */
> - if (inport > 0 &&
> + if (inport >= 0 &&
That looks like a bug fix, but if you don't care about fixing this in <
v6.8, I don't mind.
Rest looks fine to me
Suzuki
On 30/01/2024 09:02, Tao Zhang wrote:
> Timestamps are requested if the monitor’s CMB data set unit input
> data matches the value in the Monitor CMB timestamp pattern and mask
> registers (M_CMB_TPR and M_CMB_TPMR) when CMB timestamp enabled
> via the timestamp insertion enable register bit(CMB_TIER.PATT_TSENAB).
> The pattern match trigger output is achieved via setting values into
> the CMB trigger pattern and mask registers (CMB_XPR and CMB_XPMR).
> After configuring a pattern through these registers, the TPDM subunit
> will assert an output trigger every time it receives new input data
> that matches the configured pattern value. Values in a given bit
> number of the mask register correspond to the same bit number in
> the corresponding pattern register.
>
> Reviewed-by: James Clark <[email protected]>
> Signed-off-by: Tao Zhang <[email protected]>
> Signed-off-by: Jinlong Mao <[email protected]>
> ---
> .../testing/sysfs-bus-coresight-devices-tpdm | 30 ++++++
> drivers/hwtracing/coresight/coresight-tpdm.c | 96 ++++++++++++++++++-
> drivers/hwtracing/coresight/coresight-tpdm.h | 39 ++++++++
> 3 files changed, 164 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index 6dfb18d6d64a..b6cf050861ed 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -184,3 +184,33 @@ Description: (Write) Set the data collection mode of CMB tpdm. Continuous
> Accepts only one of the 2 values - 0 or 1.
> 0 : Continuous CMB collection mode.
> 1 : Trace-on-change CMB collection mode.
> +
> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpr[0:1]
> +Date: January 2024
> +KernelVersion 6.9
> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
> +Description:
> + (RW) Set/Get the value of the trigger pattern for the CMB
> + subunit TPDM.
> +
> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpmr[0:1]
> +Date: January 2024
> +KernelVersion 6.9
> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
> +Description:
> + (RW) Set/Get the mask of the trigger pattern for the CMB
> + subunit TPDM.
> +
> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpr[0:1]
> +Date: January 2024
> +KernelVersion 6.9
> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
> +Description:
> + (RW) Set/Get the value of the pattern for the CMB subunit TPDM.
> +
> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpmr[0:1]
> +Date: January 2024
> +KernelVersion 6.9
> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang (QUIC) <[email protected]>
> +Description:
> + (RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index b20071460375..07587287d9fa 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -66,6 +66,26 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev,
> return -EINVAL;
> return sysfs_emit(buf, "0x%x\n",
> drvdata->dsb->msr[tpdm_attr->idx]);
> + case CMB_TRIG_PATT:
> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> + return -EINVAL;
> + return sysfs_emit(buf, "0x%x\n",
> + drvdata->cmb->trig_patt[tpdm_attr->idx]);
> + case CMB_TRIG_PATT_MASK:
> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> + return -EINVAL;
> + return sysfs_emit(buf, "0x%x\n",
> + drvdata->cmb->trig_patt_mask[tpdm_attr->idx]);
> + case CMB_PATT:
> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> + return -EINVAL;
> + return sysfs_emit(buf, "0x%x\n",
> + drvdata->cmb->patt_val[tpdm_attr->idx]);
> + case CMB_PATT_MASK:
> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
> + return -EINVAL;
> + return sysfs_emit(buf, "0x%x\n",
> + drvdata->cmb->patt_mask[tpdm_attr->idx]);
> }
> return -EINVAL;
> }
> @@ -118,6 +138,30 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
> ret = size;
> }
> break;
> + case CMB_TRIG_PATT:
> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
> + drvdata->cmb->trig_patt[tpdm_attr->idx] = val;
> + ret = size;
> + }
> + break;
> + case CMB_TRIG_PATT_MASK:
> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
> + drvdata->cmb->trig_patt_mask[tpdm_attr->idx] = val;
> + ret = size;
> + }
> + break;
> + case CMB_PATT:
> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
> + drvdata->cmb->patt_val[tpdm_attr->idx] = val;
> + ret = size;
> + }
> + break;
> + case CMB_PATT_MASK:
> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
> + drvdata->cmb->patt_mask[tpdm_attr->idx] = val;
> + ret = size;
> + }
> + break;
> default:
> break;
> }
> @@ -280,12 +324,32 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>
> static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
> {
> - u32 val;
> + u32 val, i;
>
> if (!tpdm_has_cmb_dataset(drvdata))
> return;
>
> + /* Configure pattern registers */
> + for (i = 0; i < TPDM_CMB_MAX_PATT; i++) {
> + writel_relaxed(drvdata->cmb->patt_val[i],
> + drvdata->base + TPDM_CMB_TPR(i));
> + writel_relaxed(drvdata->cmb->patt_mask[i],
> + drvdata->base + TPDM_CMB_TPMR(i));
> + writel_relaxed(drvdata->cmb->trig_patt[i],
> + drvdata->base + TPDM_CMB_XPR(i));
> + writel_relaxed(drvdata->cmb->trig_patt_mask[i],
> + drvdata->base + TPDM_CMB_XPMR(i));
> + }
> +
> val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
> + /*
> + * Set to 0 for continuous CMB collection mode,
> + * 1 for trace-on-change CMB collection mode.
> + */
> + if (drvdata->cmb->trace_mode)
> + val |= TPDM_CMB_CR_MODE;
> + else
> + val &= ~TPDM_CMB_CR_MODE;
> /* Set the enable bit of CMB control register to 1 */
> val |= TPDM_CMB_CR_ENA;
> writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
> @@ -887,6 +951,22 @@ static struct attribute *tpdm_dsb_msr_attrs[] = {
> NULL,
> };
>
> +static struct attribute *tpdm_cmb_trig_patt_attrs[] = {
> + CMB_TRIG_PATT_ATTR(0),
> + CMB_TRIG_PATT_ATTR(1),
> + CMB_TRIG_PATT_MASK_ATTR(0),
> + CMB_TRIG_PATT_MASK_ATTR(1),
> + NULL,
> +};
> +
> +static struct attribute *tpdm_cmb_patt_attrs[] = {
> + CMB_PATT_ATTR(0),
> + CMB_PATT_ATTR(1),
> + CMB_PATT_MASK_ATTR(0),
> + CMB_PATT_MASK_ATTR(1),
> + NULL,
> +};
> +
> static struct attribute *tpdm_dsb_attrs[] = {
> &dev_attr_dsb_mode.attr,
> &dev_attr_dsb_trig_ts.attr,
> @@ -933,6 +1013,18 @@ static struct attribute_group tpdm_cmb_attr_grp = {
> .is_visible = tpdm_cmb_is_visible,
> };
>
> +static struct attribute_group tpdm_cmb_trig_patt_grp = {
> + .attrs = tpdm_cmb_trig_patt_attrs,
> + .is_visible = tpdm_cmb_is_visible,
> + .name = "cmb_trig_patt",
> +};
> +
> +static struct attribute_group tpdm_cmb_patt_grp = {
> + .attrs = tpdm_cmb_patt_attrs,
> + .is_visible = tpdm_cmb_is_visible,
> + .name = "cmb_patt",
> +};
> +
> static const struct attribute_group *tpdm_attr_grps[] = {
> &tpdm_attr_grp,
> &tpdm_dsb_attr_grp,
> @@ -941,6 +1033,8 @@ static const struct attribute_group *tpdm_attr_grps[] = {
> &tpdm_dsb_patt_grp,
> &tpdm_dsb_msr_grp,
> &tpdm_cmb_attr_grp,
> + &tpdm_cmb_trig_patt_grp,
> + &tpdm_cmb_patt_grp,
> NULL,
> };
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 2af92c270ed1..8cb8a9b35384 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -11,12 +11,23 @@
>
> /* CMB Subunit Registers */
> #define TPDM_CMB_CR (0xA00)
> +/*CMB subunit timestamp pattern registers*/
> +#define TPDM_CMB_TPR(n) (0xA08 + (n * 4))
> +/*CMB subunit timestamp pattern mask registers*/
> +#define TPDM_CMB_TPMR(n) (0xA10 + (n * 4))
> +/*CMB subunit trigger pattern registers*/
> +#define TPDM_CMB_XPR(n) (0xA18 + (n * 4))
> +/*CMB subunit trigger pattern mask registers*/
minor nit: Leave a space after/before '/*' & '*/'
Suzuki
On 1/30/2024 8:40 PM, Suzuki K Poulose wrote:
> On 30/01/2024 09:02, Tao Zhang wrote:
>> Timestamps are requested if the monitor’s CMB data set unit input
>> data matches the value in the Monitor CMB timestamp pattern and mask
>> registers (M_CMB_TPR and M_CMB_TPMR) when CMB timestamp enabled
>> via the timestamp insertion enable register bit(CMB_TIER.PATT_TSENAB).
>> The pattern match trigger output is achieved via setting values into
>> the CMB trigger pattern and mask registers (CMB_XPR and CMB_XPMR).
>> After configuring a pattern through these registers, the TPDM subunit
>> will assert an output trigger every time it receives new input data
>> that matches the configured pattern value. Values in a given bit
>> number of the mask register correspond to the same bit number in
>> the corresponding pattern register.
>>
>> Reviewed-by: James Clark <[email protected]>
>> Signed-off-by: Tao Zhang <[email protected]>
>> Signed-off-by: Jinlong Mao <[email protected]>
>> ---
>> .../testing/sysfs-bus-coresight-devices-tpdm | 30 ++++++
>> drivers/hwtracing/coresight/coresight-tpdm.c | 96 ++++++++++++++++++-
>> drivers/hwtracing/coresight/coresight-tpdm.h | 39 ++++++++
>> 3 files changed, 164 insertions(+), 1 deletion(-)
>>
>> diff --git
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index 6dfb18d6d64a..b6cf050861ed 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -184,3 +184,33 @@ Description: (Write) Set the data collection
>> mode of CMB tpdm. Continuous
>> Accepts only one of the 2 values - 0 or 1.
>> 0 : Continuous CMB collection mode.
>> 1 : Trace-on-change CMB collection mode.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpr[0:1]
>> +Date: January 2024
>> +KernelVersion 6.9
>> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang
>> (QUIC) <[email protected]>
>> +Description:
>> + (RW) Set/Get the value of the trigger pattern for the CMB
>> + subunit TPDM.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_patt/xpmr[0:1]
>> +Date: January 2024
>> +KernelVersion 6.9
>> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang
>> (QUIC) <[email protected]>
>> +Description:
>> + (RW) Set/Get the mask of the trigger pattern for the CMB
>> + subunit TPDM.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpr[0:1]
>> +Date: January 2024
>> +KernelVersion 6.9
>> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang
>> (QUIC) <[email protected]>
>> +Description:
>> + (RW) Set/Get the value of the pattern for the CMB subunit TPDM.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_patt/tpmr[0:1]
>> +Date: January 2024
>> +KernelVersion 6.9
>> +Contact: Jinlong Mao (QUIC) <[email protected]>, Tao Zhang
>> (QUIC) <[email protected]>
>> +Description:
>> + (RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index b20071460375..07587287d9fa 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -66,6 +66,26 @@ static ssize_t tpdm_simple_dataset_show(struct
>> device *dev,
>> return -EINVAL;
>> return sysfs_emit(buf, "0x%x\n",
>> drvdata->dsb->msr[tpdm_attr->idx]);
>> + case CMB_TRIG_PATT:
>> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
>> + return -EINVAL;
>> + return sysfs_emit(buf, "0x%x\n",
>> + drvdata->cmb->trig_patt[tpdm_attr->idx]);
>> + case CMB_TRIG_PATT_MASK:
>> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
>> + return -EINVAL;
>> + return sysfs_emit(buf, "0x%x\n",
>> + drvdata->cmb->trig_patt_mask[tpdm_attr->idx]);
>> + case CMB_PATT:
>> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
>> + return -EINVAL;
>> + return sysfs_emit(buf, "0x%x\n",
>> + drvdata->cmb->patt_val[tpdm_attr->idx]);
>> + case CMB_PATT_MASK:
>> + if (tpdm_attr->idx >= TPDM_CMB_MAX_PATT)
>> + return -EINVAL;
>> + return sysfs_emit(buf, "0x%x\n",
>> + drvdata->cmb->patt_mask[tpdm_attr->idx]);
>> }
>> return -EINVAL;
>> }
>> @@ -118,6 +138,30 @@ static ssize_t tpdm_simple_dataset_store(struct
>> device *dev,
>> ret = size;
>> }
>> break;
>> + case CMB_TRIG_PATT:
>> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
>> + drvdata->cmb->trig_patt[tpdm_attr->idx] = val;
>> + ret = size;
>> + }
>> + break;
>> + case CMB_TRIG_PATT_MASK:
>> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
>> + drvdata->cmb->trig_patt_mask[tpdm_attr->idx] = val;
>> + ret = size;
>> + }
>> + break;
>> + case CMB_PATT:
>> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
>> + drvdata->cmb->patt_val[tpdm_attr->idx] = val;
>> + ret = size;
>> + }
>> + break;
>> + case CMB_PATT_MASK:
>> + if (tpdm_attr->idx < TPDM_CMB_MAX_PATT) {
>> + drvdata->cmb->patt_mask[tpdm_attr->idx] = val;
>> + ret = size;
>> + }
>> + break;
>> default:
>> break;
>> }
>> @@ -280,12 +324,32 @@ static void tpdm_enable_dsb(struct tpdm_drvdata
>> *drvdata)
>> static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>> {
>> - u32 val;
>> + u32 val, i;
>> if (!tpdm_has_cmb_dataset(drvdata))
>> return;
>> + /* Configure pattern registers */
>> + for (i = 0; i < TPDM_CMB_MAX_PATT; i++) {
>> + writel_relaxed(drvdata->cmb->patt_val[i],
>> + drvdata->base + TPDM_CMB_TPR(i));
>> + writel_relaxed(drvdata->cmb->patt_mask[i],
>> + drvdata->base + TPDM_CMB_TPMR(i));
>> + writel_relaxed(drvdata->cmb->trig_patt[i],
>> + drvdata->base + TPDM_CMB_XPR(i));
>> + writel_relaxed(drvdata->cmb->trig_patt_mask[i],
>> + drvdata->base + TPDM_CMB_XPMR(i));
>> + }
>> +
>> val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>> + /*
>> + * Set to 0 for continuous CMB collection mode,
>> + * 1 for trace-on-change CMB collection mode.
>> + */
>> + if (drvdata->cmb->trace_mode)
>> + val |= TPDM_CMB_CR_MODE;
>> + else
>> + val &= ~TPDM_CMB_CR_MODE;
>> /* Set the enable bit of CMB control register to 1 */
>> val |= TPDM_CMB_CR_ENA;
>> writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>> @@ -887,6 +951,22 @@ static struct attribute *tpdm_dsb_msr_attrs[] = {
>> NULL,
>> };
>> +static struct attribute *tpdm_cmb_trig_patt_attrs[] = {
>> + CMB_TRIG_PATT_ATTR(0),
>> + CMB_TRIG_PATT_ATTR(1),
>> + CMB_TRIG_PATT_MASK_ATTR(0),
>> + CMB_TRIG_PATT_MASK_ATTR(1),
>> + NULL,
>> +};
>> +
>> +static struct attribute *tpdm_cmb_patt_attrs[] = {
>> + CMB_PATT_ATTR(0),
>> + CMB_PATT_ATTR(1),
>> + CMB_PATT_MASK_ATTR(0),
>> + CMB_PATT_MASK_ATTR(1),
>> + NULL,
>> +};
>> +
>> static struct attribute *tpdm_dsb_attrs[] = {
>> &dev_attr_dsb_mode.attr,
>> &dev_attr_dsb_trig_ts.attr,
>> @@ -933,6 +1013,18 @@ static struct attribute_group tpdm_cmb_attr_grp
>> = {
>> .is_visible = tpdm_cmb_is_visible,
>> };
>> +static struct attribute_group tpdm_cmb_trig_patt_grp = {
>> + .attrs = tpdm_cmb_trig_patt_attrs,
>> + .is_visible = tpdm_cmb_is_visible,
>> + .name = "cmb_trig_patt",
>> +};
>> +
>> +static struct attribute_group tpdm_cmb_patt_grp = {
>> + .attrs = tpdm_cmb_patt_attrs,
>> + .is_visible = tpdm_cmb_is_visible,
>> + .name = "cmb_patt",
>> +};
>> +
>> static const struct attribute_group *tpdm_attr_grps[] = {
>> &tpdm_attr_grp,
>> &tpdm_dsb_attr_grp,
>> @@ -941,6 +1033,8 @@ static const struct attribute_group
>> *tpdm_attr_grps[] = {
>> &tpdm_dsb_patt_grp,
>> &tpdm_dsb_msr_grp,
>> &tpdm_cmb_attr_grp,
>> + &tpdm_cmb_trig_patt_grp,
>> + &tpdm_cmb_patt_grp,
>> NULL,
>> };
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 2af92c270ed1..8cb8a9b35384 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -11,12 +11,23 @@
>> /* CMB Subunit Registers */
>> #define TPDM_CMB_CR (0xA00)
>> +/*CMB subunit timestamp pattern registers*/
>> +#define TPDM_CMB_TPR(n) (0xA08 + (n * 4))
>> +/*CMB subunit timestamp pattern mask registers*/
>> +#define TPDM_CMB_TPMR(n) (0xA10 + (n * 4))
>> +/*CMB subunit trigger pattern registers*/
>> +#define TPDM_CMB_XPR(n) (0xA18 + (n * 4))
>> +/*CMB subunit trigger pattern mask registers*/
>
> minor nit: Leave a space after/before '/*' & '*/'
Sure, I will update this to the next patch series.
Best,
Tao
>
> Suzuki
>
>
On 1/30/2024 8:35 PM, Suzuki K Poulose wrote:
> On 30/01/2024 09:02, Tao Zhang wrote:
>> Read the CMB element size from the device tree. Set the register
>> bit that controls the CMB element size of the corresponding port.
>>
>> Reviewed-by: James Clark <[email protected]>
>> Signed-off-by: Tao Zhang <[email protected]>
>> Signed-off-by: Mao Jinlong <[email protected]>
>> ---
>> drivers/hwtracing/coresight/coresight-tpda.c | 123 +++++++++++--------
>> drivers/hwtracing/coresight/coresight-tpda.h | 6 +
>> 2 files changed, 79 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>> b/drivers/hwtracing/coresight/coresight-tpda.c
>> index 4ac954f4bc13..fcddff3ded81 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -18,6 +18,7 @@
>> #include "coresight-priv.h"
>> #include "coresight-tpda.h"
>> #include "coresight-trace-id.h"
>> +#include "coresight-tpdm.h"
>> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>> @@ -28,24 +29,57 @@ static bool coresight_device_is_tpdm(struct
>> coresight_device *csdev)
>> CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>> }
>> +static void tpdm_clear_element_size(struct coresight_device *csdev)
>> +{
>> + struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + drvdata->dsb_esize = 0;
>> + drvdata->cmb_esize = 0;
>> +}
>> +
>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32
>> *val)
>> +{
>> +
>
>
>
>> + if (drvdata->dsb_esize == 64)
>> + *val |= TPDA_Pn_CR_DSBSIZE;
>
> We don't seem to be clearing the fields we modify, before updating
> them. This may be OK in real world where the device connected to TPDA
> port
> may not change. But it is always safer to clear the bits and set it.
>
> e.g.:
> *val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);
>
>
>
>> + else if (drvdata->dsb_esize == 32)
>> + *val &= ~TPDA_Pn_CR_DSBSIZE;
>> +
>> + if (drvdata->cmb_esize == 64)
>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>> + else if (drvdata->cmb_esize == 32)
>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>
> Similarly here ^^^. I am happy to fix it up if you are OK with it
> (unless there are other changes that need a respin)
Thank you. I would be very grateful if you could help for this.
Best,
Tao
>
>> + else if (drvdata->cmb_esize == 8)
>> + *val &= ~TPDA_Pn_CR_CMBSIZE;
>> +}
>
>> +
>> /*
>> - * Read the DSB element size from the TPDM device
>> + * Read the element size from the TPDM device. One TPDM must have at
>> least one of the
>> + * element size property.
>> * Returns
>> - * The dsb element size read from the devicetree if available.
>> - * 0 - Otherwise, with a warning once.
>> + * 0 - The element size property is read
>> + * Others - Cannot read the property of the element size
>> */
>> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
>> + struct coresight_device *csdev)
>> {
>> - int rc = 0;
>> - u8 size = 0;
>> + int rc = -EINVAL;
>> + struct tpdm_drvdata *tpdm_data =
>> dev_get_drvdata(csdev->dev.parent);
>> +
>> + if (tpdm_has_dsb_dataset(tpdm_data)) {
>> + rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>> + "qcom,dsb-element-size", &drvdata->dsb_esize);
>> + }
>> + if (tpdm_has_cmb_dataset(tpdm_data)) {
>> + rc = fwnode_property_read_u32(dev_fwnode(csdev->dev.parent),
>> + "qcom,cmb-element-bits", &drvdata->cmb_esize);
>> + }
>> - rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>> - "qcom,dsb-element-size", &size);
>> if (rc)
>> dev_warn_once(&csdev->dev,
>> - "Failed to read TPDM DSB Element size: %d\n", rc);
>> + "Failed to read TPDM Element size: %d\n", rc);
>> - return size;
>> + return rc;
>> }
>> /*
>> @@ -56,11 +90,12 @@ static int tpdm_read_dsb_element_size(struct
>> coresight_device *csdev)
>> * Parameter "inport" is used to pass in the input port number
>> * of TPDA, and it is set to -1 in the recursize call.
>> */
>> -static int tpda_get_element_size(struct coresight_device *csdev,
>> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
>> + struct coresight_device *csdev,
>> int inport)
>> {
>> - int dsb_size = -ENOENT;
>> - int i, size;
>> + int rc = 0;
>> + int i;
>> struct coresight_device *in;
>> for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>> @@ -69,30 +104,26 @@ static int tpda_get_element_size(struct
>> coresight_device *csdev,
>> continue;
>> /* Ignore the paths that do not match port */
>> - if (inport > 0 &&
>> + if (inport >= 0 &&
>
> That looks like a bug fix, but if you don't care about fixing this in
> < v6.8, I don't mind.
>
>
> Rest looks fine to me
>
> Suzuki
>
On 30/01/2024 10:02, Tao Zhang wrote:
> Add property "qcom,cmb-elem-bits" to support CMB(Continuous
> Multi-Bit) element for TPDM. The associated aggregator will read
> this size before it is enabled. CMB element size currently only
> supports 8-bit, 32-bit and 64-bit. Because the existing example
> tpdm "tpdm@684c000" which only supports dsb sub-unit, I introduce
> a new example "tpdm@6c29000" to describe the usage of this new
> property.
>
> Signed-off-by: Tao Zhang <[email protected]>
> Signed-off-by: Mao Jinlong <[email protected]>
> ---
> .../bindings/arm/qcom,coresight-tpdm.yaml | 25 +++++++++++++++++++
> 1 file changed, 25 insertions(+)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 31/01/2024 01:39, Tao Zhang wrote:
>
> On 1/30/2024 8:35 PM, Suzuki K Poulose wrote:
>> On 30/01/2024 09:02, Tao Zhang wrote:
>>> Read the CMB element size from the device tree. Set the register
>>> bit that controls the CMB element size of the corresponding port.
>>>
>>> Reviewed-by: James Clark <[email protected]>
>>> Signed-off-by: Tao Zhang <[email protected]>
>>> Signed-off-by: Mao Jinlong <[email protected]>
>>> ---
>>> drivers/hwtracing/coresight/coresight-tpda.c | 123 +++++++++++--------
>>> drivers/hwtracing/coresight/coresight-tpda.h | 6 +
>>> 2 files changed, 79 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>> index 4ac954f4bc13..fcddff3ded81 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -18,6 +18,7 @@
>>> #include "coresight-priv.h"
>>> #include "coresight-tpda.h"
>>> #include "coresight-trace-id.h"
>>> +#include "coresight-tpdm.h"
>>> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>> @@ -28,24 +29,57 @@ static bool coresight_device_is_tpdm(struct
>>> coresight_device *csdev)
>>> CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>> }
>>> +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>> +{
>>> + struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +
>>> + drvdata->dsb_esize = 0;
>>> + drvdata->cmb_esize = 0;
>>> +}
>>> +
>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32
>>> *val)
>>> +{
>>> +
>>
>>
>>
>>> + if (drvdata->dsb_esize == 64)
>>> + *val |= TPDA_Pn_CR_DSBSIZE;
>>
>> We don't seem to be clearing the fields we modify, before updating
>> them. This may be OK in real world where the device connected to TPDA
>> port
>> may not change. But it is always safer to clear the bits and set it.
>>
>> e.g.:
>> *val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);
>>
>>
>>
>>> + else if (drvdata->dsb_esize == 32)
>>> + *val &= ~TPDA_Pn_CR_DSBSIZE;
>>> +
>>> + if (drvdata->cmb_esize == 64)
>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>> + else if (drvdata->cmb_esize == 32)
>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>
>> Similarly here ^^^. I am happy to fix it up if you are OK with it
>> (unless there are other changes that need a respin)
>
> Thank you. I would be very grateful if you could help for this.
Given, you need to respin, please incorporate this change too.
Suzuki
On 1/31/2024 6:02 PM, Suzuki K Poulose wrote:
> On 31/01/2024 01:39, Tao Zhang wrote:
>>
>> On 1/30/2024 8:35 PM, Suzuki K Poulose wrote:
>>> On 30/01/2024 09:02, Tao Zhang wrote:
>>>> Read the CMB element size from the device tree. Set the register
>>>> bit that controls the CMB element size of the corresponding port.
>>>>
>>>> Reviewed-by: James Clark <[email protected]>
>>>> Signed-off-by: Tao Zhang <[email protected]>
>>>> Signed-off-by: Mao Jinlong <[email protected]>
>>>> ---
>>>> drivers/hwtracing/coresight/coresight-tpda.c | 123
>>>> +++++++++++--------
>>>> drivers/hwtracing/coresight/coresight-tpda.h | 6 +
>>>> 2 files changed, 79 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> index 4ac954f4bc13..fcddff3ded81 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -18,6 +18,7 @@
>>>> #include "coresight-priv.h"
>>>> #include "coresight-tpda.h"
>>>> #include "coresight-trace-id.h"
>>>> +#include "coresight-tpdm.h"
>>>> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>> @@ -28,24 +29,57 @@ static bool coresight_device_is_tpdm(struct
>>>> coresight_device *csdev)
>>>> CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>>> }
>>>> +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>>> +{
>>>> + struct tpda_drvdata *drvdata =
>>>> dev_get_drvdata(csdev->dev.parent);
>>>> +
>>>> + drvdata->dsb_esize = 0;
>>>> + drvdata->cmb_esize = 0;
>>>> +}
>>>> +
>>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>> u32 *val)
>>>> +{
>>>> +
>>>
>>>
>>>
>>>> + if (drvdata->dsb_esize == 64)
>>>> + *val |= TPDA_Pn_CR_DSBSIZE;
>>>
>>> We don't seem to be clearing the fields we modify, before updating
>>> them. This may be OK in real world where the device connected to
>>> TPDA port
>>> may not change. But it is always safer to clear the bits and set it.
>>>
>>> e.g.:
>>> *val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);
>>>
>>>
>>>
>>>> + else if (drvdata->dsb_esize == 32)
>>>> + *val &= ~TPDA_Pn_CR_DSBSIZE;
>>>> +
>>>> + if (drvdata->cmb_esize == 64)
>>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>>> + else if (drvdata->cmb_esize == 32)
>>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>>
>>> Similarly here ^^^. I am happy to fix it up if you are OK with it
>>> (unless there are other changes that need a respin)
>>
>> Thank you. I would be very grateful if you could help for this.
>
> Given, you need to respin, please incorporate this change too.
Sure.
Is it OK if I modify the code as follow and update to this patch directly?
*val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);
if (drvdata->dsb_esize == 64)
*val |= TPDA_Pn_CR_DSBSIZE;
else if (drvdata->dsb_esize == 32)
*val &= ~TPDA_Pn_CR_DSBSIZE;
if (drvdata->cmb_esize == 64)
*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
else if (drvdata->cmb_esize == 32)
*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
else if (drvdata->cmb_esize == 8)
*val &= ~TPDA_Pn_CR_CMBSIZE;
Best,
Tao
>
> Suzuki
>
>
>
On 01/02/2024 02:25, Tao Zhang wrote:
>
> On 1/31/2024 6:02 PM, Suzuki K Poulose wrote:
>> On 31/01/2024 01:39, Tao Zhang wrote:
>>>
>>> On 1/30/2024 8:35 PM, Suzuki K Poulose wrote:
>>>> On 30/01/2024 09:02, Tao Zhang wrote:
>>>>> Read the CMB element size from the device tree. Set the register
>>>>> bit that controls the CMB element size of the corresponding port.
>>>>>
>>>>> Reviewed-by: James Clark <[email protected]>
>>>>> Signed-off-by: Tao Zhang <[email protected]>
>>>>> Signed-off-by: Mao Jinlong <[email protected]>
>>>>> ---
>>>>> drivers/hwtracing/coresight/coresight-tpda.c | 123
>>>>> +++++++++++--------
>>>>> drivers/hwtracing/coresight/coresight-tpda.h | 6 +
>>>>> 2 files changed, 79 insertions(+), 50 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> index 4ac954f4bc13..fcddff3ded81 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> @@ -18,6 +18,7 @@
>>>>> #include "coresight-priv.h"
>>>>> #include "coresight-tpda.h"
>>>>> #include "coresight-trace-id.h"
>>>>> +#include "coresight-tpdm.h"
>>>>> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>>> @@ -28,24 +29,57 @@ static bool coresight_device_is_tpdm(struct
>>>>> coresight_device *csdev)
>>>>> CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>>>> }
>>>>> +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>>>> +{
>>>>> + struct tpda_drvdata *drvdata =
>>>>> dev_get_drvdata(csdev->dev.parent);
>>>>> +
>>>>> + drvdata->dsb_esize = 0;
>>>>> + drvdata->cmb_esize = 0;
>>>>> +}
>>>>> +
>>>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>>> u32 *val)
>>>>> +{
>>>>> +
>>>>
>>>>
>>>>
>>>>> + if (drvdata->dsb_esize == 64)
>>>>> + *val |= TPDA_Pn_CR_DSBSIZE;
>>>>
>>>> We don't seem to be clearing the fields we modify, before updating
>>>> them. This may be OK in real world where the device connected to
>>>> TPDA port
>>>> may not change. But it is always safer to clear the bits and set it.
>>>>
>>>> e.g.:
>>>> *val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);
>>>>
>>>>
>>>>
>>>>> + else if (drvdata->dsb_esize == 32)
>>>>> + *val &= ~TPDA_Pn_CR_DSBSIZE;
>>>>> +
>>>>> + if (drvdata->cmb_esize == 64)
>>>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>>>> + else if (drvdata->cmb_esize == 32)
>>>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>>>
>>>> Similarly here ^^^. I am happy to fix it up if you are OK with it
>>>> (unless there are other changes that need a respin)
>>>
>>> Thank you. I would be very grateful if you could help for this.
>>
>> Given, you need to respin, please incorporate this change too.
>
> Sure.
>
> Is it OK if I modify the code as follow and update to this patch directly?
>
> *val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);
>
> if (drvdata->dsb_esize == 64)
> *val |= TPDA_Pn_CR_DSBSIZE;
> else if (drvdata->dsb_esize == 32)
> *val &= ~TPDA_Pn_CR_DSBSIZE;
>
> if (drvdata->cmb_esize == 64)
> *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
> else if (drvdata->cmb_esize == 32)
> *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);.
> else if (drvdata->cmb_esize == 8)
> *val &= ~TPDA_Pn_CR_CMBSIZE;
Yes, that looks good to me. Even though you would be clearing the fields
for (DSB=32 and CMB=8) once again, it is good to leave it like that
rather than skipping them, making people stare at it to figure out if
this was correct or not.
Suzuki
>
> Best,
>
> Tao
>
>>
>> Suzuki
>>
>>
>>