Tao,

Some additional comments below.

On 02/11/2021 17:59, Mathieu Poirier wrote:

Good morning,


On Thu, Oct 21, 2021 at 03:38:49PM +0800, Tao Zhang wrote:

quoted

Add driver to support Coresight device TPDM. This driver provides
support for configuring monitor. Monitors are primarily responsible
for data set collection and support the ability to collect any
permutation of data set types. Monitors are also responsible for
interaction with system cross triggering.

As far as I can tell there is nothing related to CTIs in this patch.  And if
there is, it is not documented.

Please could you add a separate file documenting  the TPDM and
some of the specific details (what is used by the driver, e.g
PERPHID0/1 et) under Documentation/trace/coresight/ , in a separate
patch

quoted

Signed-off-by: Tao Zhang <redacted>
---
  .../bindings/arm/coresight-tpdm.yaml          |  86 +++

As checkpatch says, this should be in a separate file.

quoted

  MAINTAINERS                                   |   5 +

Since this is a coresight device Suzuki and I will continue the maintenance.
The get_maintainer script will make sure you care CC'ed on patches related to
the TPDM/TPDA drivers, and we would typically requried a "Reviewed-by" tag from
you before merging.

quoted

  drivers/hwtracing/coresight/Kconfig           |   9 +
  drivers/hwtracing/coresight/Makefile          |   1 +
  drivers/hwtracing/coresight/coresight-tpdm.c  | 583 ++++++++++++++++++
  5 files changed, 684 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
  create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c

diff --git a/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
new file mode 100644
index 000000000000..44541075d77f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml

@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/coresight-tpdm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Trace, Profiling and Diagnostics Monitor - TPDM
+
+description: |
+  The TPDM or Monitor serves as data collection component for various dataset
+  types specified in the QPMDA spec. It covers Basic Counts (BC), Tenure
+  Counts (TC), Continuous Multi-Bit (CMB), and Discrete Single Bit (DSB). It
+  performs data collection in the data producing clock domain and transfers it
+  to the data collection time domain, generally ATB clock domain.
+
+  The primary use case of the TPDM is to collect data from different data
+  sources and send it to a TPDA for packetization, timestamping, and funneling.
+
+maintainers:
+  - Tao Zhang <quic_taozha@quicinc.com>
+
+properties:
+  $nodename:
+    pattern: "^tpdm(@[0-9a-f]+)$"
+  compatible:
+    items:
+      - const: arm,primecell

You must have a compatible that identifies this as "tpdm", just like
the other components, even though it is not functional.

quoted

+
+  reg:
+    maxItems: 1
+
+  reg-names:
+    items:
+      - const: tpdm-base

Is the reg-name really necessary ?

quoted

+
+  atid:
+    maxItems: 1
+    description: |
+      The QPMDA specification repurposed the ATID field of the AMBA ATB
+      specification to use it to convey packetization information to the
+      Aggregator.

Please could you describe how this affects the device in the doc
requested above.

quoted

+
+  out-ports:
+    description: |
+      Output connections from the TPDM to legacy CoreSight trace bus.
+    $ref: /schemas/graph.yaml#/properties/ports
+    properties:
+      port:
+        description: Output connection from the TPDM to legacy CoreSight
+          Trace bus.
+        $ref: /schemas/graph.yaml#/properties/port
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - atid
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  # minimum TPDM definition.
+  - |
+    tpdm@6980000 {
+      compatible = "arm,primecell";

Like other components, we must have :
	  compatible = "qcom,<new-compatible>", "arm,primecell";

quoted

+      reg = <0x6980000 0x1000>;
+      reg-names = "tpdm-base";
+
+      clocks = <&aoss_qmp>;
+      clock-names = "apb_pclk";
+
+      atid = <78>;
+      out-ports {
+        port {
+          tpdm_turing_out_funnel_turing: endpoint {
+            remote-endpoint =
+              <&funnel_turing_in_tpdm_turing>;
+          };
+        };
+      };
+    };
+
+...

quoted

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index b6c4a48140ec..e7392a0dddeb 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile

@@ -25,5 +25,6 @@ obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
  obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o
  obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o
  obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o
+obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o
  coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
  		   coresight-cti-sysfs.o

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
new file mode 100644
index 000000000000..906776c859d6
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c

...

quoted

+
+#ifdef CONFIG_CORESIGHT_TPDM_DEFAULT_ENABLE
+static int boot_enable = 1;
+#else
+static int boot_enable;
+#endif

That isn't the proper way to do this.  Look at how it is done in
coresight-etm4x.c

quoted

+
+struct gpr_dataset {
+	DECLARE_BITMAP(gpr_dirty, TPDM_GPR_REGS_MAX);
+	uint32_t		gp_regs[TPDM_GPR_REGS_MAX];

Shouldn't this be u32?

quoted

+};
+
+struct bc_dataset {
+	enum tpdm_mode		capture_mode;
+	enum tpdm_mode		retrieval_mode;
+	uint32_t		sat_mode;
+	uint32_t		enable_counters;
+	uint32_t		clear_counters;
+	uint32_t		enable_irq;
+	uint32_t		clear_irq;
+	uint32_t		trig_val_lo[TPDM_BC_MAX_COUNTERS];
+	uint32_t		trig_val_hi[TPDM_BC_MAX_COUNTERS];
+	uint32_t		enable_ganging;
+	uint32_t		overflow_val[TPDM_BC_MAX_OVERFLOW];
+	uint32_t		msr[TPDM_BC_MAX_MSR];
+};
+
+struct tc_dataset {
+	enum tpdm_mode		capture_mode;
+	enum tpdm_mode		retrieval_mode;
+	bool			sat_mode;
+	uint32_t		enable_counters;
+	uint32_t		clear_counters;
+	uint32_t		enable_irq;
+	uint32_t		clear_irq;
+	uint32_t		trig_sel[TPDM_TC_MAX_TRIG];
+	uint32_t		trig_val_lo[TPDM_TC_MAX_TRIG];
+	uint32_t		trig_val_hi[TPDM_TC_MAX_TRIG];
+	uint32_t		msr[TPDM_TC_MAX_MSR];
+};
+
+struct dsb_dataset {
+	uint32_t		mode;
+	uint32_t		edge_ctrl[TPDM_DSB_MAX_EDCR];
+	uint32_t		edge_ctrl_mask[TPDM_DSB_MAX_EDCR / 2];
+	uint32_t		patt_val[TPDM_DSB_MAX_PATT];
+	uint32_t		patt_mask[TPDM_DSB_MAX_PATT];
+	bool			patt_ts;
+	bool			patt_type;
+	uint32_t		trig_patt_val[TPDM_DSB_MAX_PATT];
+	uint32_t		trig_patt_mask[TPDM_DSB_MAX_PATT];
+	bool			trig_ts;
+	bool			trig_type;
+	uint32_t		select_val[TPDM_DSB_MAX_SELECT];
+	uint32_t		msr[TPDM_DSB_MAX_MSR];
+};
+
+struct mcmb_dataset {
+	uint8_t		mcmb_trig_lane;
+	uint8_t		mcmb_lane_select;
+};
+
+struct cmb_dataset {
+	bool			trace_mode;
+	uint32_t		cycle_acc;
+	uint32_t		patt_val[TPDM_CMB_PATT_CMP];
+	uint32_t		patt_mask[TPDM_CMB_PATT_CMP];
+	bool			patt_ts;
+	uint32_t		trig_patt_val[TPDM_CMB_PATT_CMP];
+	uint32_t		trig_patt_mask[TPDM_CMB_PATT_CMP];
+	bool			trig_ts;
+	bool			ts_all;
+	uint32_t		msr[TPDM_CMB_MAX_MSR];
+	uint8_t			read_ctl_reg;
+	struct mcmb_dataset	*mcmb;
+};
+
+DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
+
+struct tpdm_drvdata {
+	void __iomem		*base;
+	struct device		*dev;
+	struct coresight_device	*csdev;
+	int			nr_tclk;
+	struct clk		**tclk;
+	int			nr_treg;
+	struct regulator	**treg;
+	struct mutex		lock;
+	bool			enable;
+	bool			clk_enable;
+	DECLARE_BITMAP(datasets, TPDM_DATASETS);
+	DECLARE_BITMAP(enable_ds, TPDM_DATASETS);
+	enum tpdm_support_type	tc_trig_type;
+	enum tpdm_support_type	bc_trig_type;
+	enum tpdm_support_type	bc_gang_type;
+	uint32_t		bc_counters_avail;
+	uint32_t		tc_counters_avail;
+	struct gpr_dataset	*gpr;
+	struct bc_dataset	*bc;
+	struct tc_dataset	*tc;
+	struct dsb_dataset	*dsb;
+	struct cmb_dataset	*cmb;
+	int			traceid;
+	uint32_t		version;
+	bool			msr_support;
+	bool			msr_fix_req;
+	bool			cmb_msr_skip;
+};

All of these should also be in a header file and properly documented.

quoted

+
+static void tpdm_init_default_data(struct tpdm_drvdata *drvdata);

This isn't needed.

quoted

+
+static void __tpdm_enable(struct tpdm_drvdata *drvdata)
+{
+	TPDM_UNLOCK(drvdata);
+
+	if (drvdata->clk_enable)
+		tpdm_writel(drvdata, 0x1, TPDM_CLK_CTRL);
+
+	TPDM_LOCK(drvdata);
+}
+

quoted

+static const struct coresight_ops_source tpdm_source_ops = {
+	.trace_id	= tpdm_trace_id,
+	.enable		= tpdm_enable,
+	.disable	= tpdm_disable,
+};
+
+static const struct coresight_ops tpdm_cs_ops = {
+	.source_ops	= &tpdm_source_ops,
+};
+
+static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata)
+{
+	if (test_bit(TPDM_DS_GPR, drvdata->datasets)) {
+		drvdata->gpr = devm_kzalloc(drvdata->dev, sizeof(*drvdata->gpr),
+					    GFP_KERNEL);
+		if (!drvdata->gpr)
+			return -ENOMEM;
+	}
+	if (test_bit(TPDM_DS_BC, drvdata->datasets)) {
+		drvdata->bc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->bc),
+					   GFP_KERNEL);
+		if (!drvdata->bc)
+			return -ENOMEM;
+	}
+	if (test_bit(TPDM_DS_TC, drvdata->datasets)) {
+		drvdata->tc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->tc),
+					   GFP_KERNEL);
+		if (!drvdata->tc)
+			return -ENOMEM;
+	}
+	if (test_bit(TPDM_DS_DSB, drvdata->datasets)) {
+		drvdata->dsb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->dsb),
+					    GFP_KERNEL);
+		if (!drvdata->dsb)
+			return -ENOMEM;
+	}
+	if (test_bit(TPDM_DS_CMB, drvdata->datasets)) {
+		drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb),
+					    GFP_KERNEL);
+		if (!drvdata->cmb)
+			return -ENOMEM;
+	} else if (test_bit(TPDM_DS_MCMB, drvdata->datasets)) {
+		drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb),
+					    GFP_KERNEL);
+		if (!drvdata->cmb)
+			return -ENOMEM;
+		drvdata->cmb->mcmb = devm_kzalloc(drvdata->dev,
+						  sizeof(*drvdata->cmb->mcmb),
+						  GFP_KERNEL);
+		if (!drvdata->cmb->mcmb)
+			return -ENOMEM;

How can I understand what the above does when:

1) There isn't a single line of comments.
2) I don't know the HW.
3) I don't have access to the documentation.

+1

quoted

+	}
+	return 0;
+}
+
+static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
+{
+	if (test_bit(TPDM_DS_BC, drvdata->datasets))
+		drvdata->bc->retrieval_mode = TPDM_MODE_ATB;
+
+	if (test_bit(TPDM_DS_TC, drvdata->datasets))
+		drvdata->tc->retrieval_mode = TPDM_MODE_ATB;
+
+	if (test_bit(TPDM_DS_DSB, drvdata->datasets)) {
+		drvdata->dsb->trig_ts = true;
+		drvdata->dsb->trig_type = false;
+	}
+
+	if (test_bit(TPDM_DS_CMB, drvdata->datasets) ||
+	    test_bit(TPDM_DS_MCMB, drvdata->datasets))
+		drvdata->cmb->trig_ts = true;
+}
+
+static int tpdm_parse_of_data(struct tpdm_drvdata *drvdata)
+{
+	int i, ret;
+	const char *tclk_name, *treg_name;
+	struct device_node *node = drvdata->dev->of_node;
+
+	drvdata->clk_enable = of_property_read_bool(node, "qcom,clk-enable");
+	drvdata->msr_fix_req = of_property_read_bool(node, "qcom,msr-fix-req");
+	drvdata->cmb_msr_skip = of_property_read_bool(node,
+					"qcom,cmb-msr-skip");
+

These properties must be listed as optional/mandatory in the DT binding
with proper description.

quoted

+	drvdata->nr_tclk = of_property_count_strings(node, "qcom,tpdm-clks");
+	if (drvdata->nr_tclk > 0) {
+		drvdata->tclk = devm_kzalloc(drvdata->dev, drvdata->nr_tclk *
+					     sizeof(*drvdata->tclk),
+					     GFP_KERNEL);
+		if (!drvdata->tclk)
+			return -ENOMEM;
+
+		for (i = 0; i < drvdata->nr_tclk; i++) {
+			ret = of_property_read_string_index(node,
+					    "qcom,tpdm-clks", i, &tclk_name);
+			if (ret)
+				return ret;
+
+			drvdata->tclk[i] = devm_clk_get(drvdata->dev,
+							tclk_name);
+			if (IS_ERR(drvdata->tclk[i]))
+				return PTR_ERR(drvdata->tclk[i]);
+		}
+	}
+
+	drvdata->nr_treg = of_property_count_strings(node, "qcom,tpdm-regs");

Where is this documented in the DT ?

quoted

+	if (drvdata->nr_treg > 0) {
+		drvdata->treg = devm_kzalloc(drvdata->dev, drvdata->nr_treg *
+					     sizeof(*drvdata->treg),
+					     GFP_KERNEL);
+		if (!drvdata->treg)
+			return -ENOMEM;
+
+		for (i = 0; i < drvdata->nr_treg; i++) {
+			ret = of_property_read_string_index(node,
+					    "qcom,tpdm-regs", i, &treg_name);
+			if (ret)
+				return ret;
+
+			drvdata->treg[i] = devm_regulator_get(drvdata->dev,
+							treg_name);
+			if (IS_ERR(drvdata->treg[i]))
+				return PTR_ERR(drvdata->treg[i]);
+		}
+	}

_None_ of the above are defined in the yaml file and/or part of the example that
is shown there.  Moreover they don't appear in patch 10/10 where TPDM and TPDA are
supposed to be introduced.  I will comment on patch 10/10 when I'm done with
this one.

+1

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel