[PATCH v1 0/2] coresight: Add remote etm support

[PATCH v1 0/2] coresight: Add remote etm support

[Mao Jinlong]

The system on chip (SoC) consists of main APSS(Applications processor
subsytem) and additional processors like modem, lpass. There is
coresight-etm driver for etm trace of APSS. Coresight remote etm driver
is for enabling and disabling the etm trace of remote processors.
It uses QMI interface to communicate with remote processors' software
and uses coresight framework to configure the connection from remote
etm source to TMC sinks.

Example to capture the remote etm trace:

Enable source:
echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink
echo 1 > /sys/bus/coresight/devices/remote_etm0/enable_source

Capture the trace:
cat /dev/tmc_etf0 > /data/remote_etm.bin

Disable source:
echo 0 > /sys/bus/coresight/devices/remote_etm0/enable_source

Mao Jinlong (2):
  coresight: Add remote etm support
  dt-bindings: arm: Add remote etm driver

 .../arm/qcom,coresight-remote-etm.yaml        |  59 ++++
 drivers/hwtracing/coresight/Kconfig           |   9 +
 drivers/hwtracing/coresight/Makefile          |   1 +
 drivers/hwtracing/coresight/coresight-core.c  |   3 +
 drivers/hwtracing/coresight/coresight-qmi.h   | 109 ++++++
 .../coresight/coresight-remote-etm.c          | 325 ++++++++++++++++++
 include/linux/coresight.h                     |   1 +
 7 files changed, 507 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
 create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h
 create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c

-- 
2.41.0


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

[PATCH v1 1/2] coresight: Add remote etm support

[Mao Jinlong]

Add support for ETM trace collection on remote processor using
coreSight framework.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/coresight/Kconfig           |   9 +
 drivers/hwtracing/coresight/Makefile          |   1 +
 drivers/hwtracing/coresight/coresight-core.c  |   3 +
 drivers/hwtracing/coresight/coresight-qmi.h   | 109 ++++++
 .../coresight/coresight-remote-etm.c          | 325 ++++++++++++++++++
 include/linux/coresight.h                     |   1 +
 6 files changed, 448 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h
 create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 06f0a7594169..425886ab7401 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -247,4 +247,13 @@ config CORESIGHT_DUMMY
 
 	  To compile this driver as a module, choose M here: the module will be
 	  called coresight-dummy.
+
+config CORESIGHT_REMOTE_ETM
+	tristate "Remote processor ETM trace support"
+	select QCOM_QMI_HELPERS
+	help
+	  Enables support for ETM trace collection on remote processor using
+	  CoreSight framework. Enabling this will allow turning on ETM
+	  tracing on remote processor via sysfs by configuring the required
+	  CoreSight components.
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 995d3b2c76df..a5283cab0bc0 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -29,5 +29,6 @@ obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o
 obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o
 coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
 		   coresight-cti-sysfs.o
+obj-$(CONFIG_CORESIGHT_REMOTE_ETM) += coresight-remote-etm.o
 obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
 obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index d7f0e231feb9..f365a3899821 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1094,6 +1094,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
 	if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
 	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
 	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
+	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC &&
 	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
 		dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
 		return -EINVAL;
@@ -1164,6 +1165,7 @@ int coresight_enable(struct coresight_device *csdev)
 		break;
 	case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
 	case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
+	case CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC:
 	case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
 		/*
 		 * Use the hash of source's device name as ID
@@ -1215,6 +1217,7 @@ void coresight_disable(struct coresight_device *csdev)
 		break;
 	case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
 	case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
+	case CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC:
 	case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
 		hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
 		/* Find the path by the hash. */
diff --git a/drivers/hwtracing/coresight/coresight-qmi.h b/drivers/hwtracing/coresight/coresight-qmi.h
new file mode 100644
index 000000000000..4c35ba8c8a05
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-qmi.h
@@ -0,0 +1,109 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _CORESIGHT_QMI_H
+#define _CORESIGHT_QMI_H
+
+#include <linux/soc/qcom/qmi.h>
+
+#define CORESIGHT_QMI_SVC_ID			(0x33)
+#define CORESIGHT_QMI_VERSION			(1)
+
+#define CORESIGHT_QMI_GET_ETM_REQ_V01		(0x002B)
+#define CORESIGHT_QMI_GET_ETM_RESP_V01		(0x002B)
+#define CORESIGHT_QMI_SET_ETM_REQ_V01		(0x002C)
+#define CORESIGHT_QMI_SET_ETM_RESP_V01		(0x002C)
+
+#define CORESIGHT_QMI_GET_ETM_REQ_MAX_LEN	(0)
+#define CORESIGHT_QMI_GET_ETM_RESP_MAX_LEN	(14)
+#define CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN	(7)
+#define CORESIGHT_QMI_SET_ETM_RESP_MAX_LEN	(7)
+
+#define TIMEOUT_MS				(10000)
+
+enum coresight_etm_state_enum_type_v01 {
+	/* To force a 32 bit signed enum. Do not change or use */
+	CORESIGHT_ETM_STATE_ENUM_TYPE_MIN_ENUM_VAL_V01 = INT_MIN,
+	CORESIGHT_ETM_STATE_DISABLED_V01 = 0,
+	CORESIGHT_ETM_STATE_ENABLED_V01 = 1,
+	CORESIGHT_ETM_STATE_ENUM_TYPE_MAX_ENUM_VAL_01 = INT_MAX,
+};
+
+struct coresight_get_etm_req_msg_v01 {
+	/*
+	 * This element is a placeholder to prevent declaration of
+	 * empty struct. Do not change.
+	 */
+	char __placeholder;
+};
+
+struct coresight_get_etm_resp_msg_v01 {
+	/* Mandatory */
+	/* QMI result Code */
+	struct qmi_response_type_v01 resp;
+
+	/* Optional */
+	/* ETM output state, must be set to true if state is being passed */
+	uint8_t state_valid;
+	/* Present when result code is QMI_RESULT_SUCCESS */
+	enum coresight_etm_state_enum_type_v01 state;
+};
+
+struct coresight_set_etm_req_msg_v01 {
+	/* Mandatory */
+	/* ETM output state */
+	enum coresight_etm_state_enum_type_v01 state;
+};
+
+struct coresight_set_etm_resp_msg_v01 {
+	/* Mandatory */
+	struct qmi_response_type_v01 resp;
+};
+
+static struct qmi_elem_info coresight_set_etm_req_msg_v01_ei[] = {
+	{
+		.data_type = QMI_UNSIGNED_4_BYTE,
+		.elem_len  = 1,
+		.elem_size = sizeof(enum coresight_etm_state_enum_type_v01),
+		.array_type  = NO_ARRAY,
+		.tlv_type  = 0x01,
+		.offset    = offsetof(struct coresight_set_etm_req_msg_v01,
+				      state),
+		.ei_array  = NULL,
+	},
+	{
+		.data_type = QMI_EOTI,
+		.elem_len  = 0,
+		.elem_size = 0,
+		.array_type  = NO_ARRAY,
+		.tlv_type  = 0,
+		.offset    = 0,
+		.ei_array  = NULL,
+	},
+};
+
+static struct qmi_elem_info coresight_set_etm_resp_msg_v01_ei[] = {
+	{
+		.data_type = QMI_STRUCT,
+		.elem_len  = 1,
+		.elem_size = sizeof(struct qmi_response_type_v01),
+		.array_type  = NO_ARRAY,
+		.tlv_type  = 0x02,
+		.offset    = offsetof(struct coresight_set_etm_resp_msg_v01,
+				      resp),
+		.ei_array  = qmi_response_type_v01_ei,
+	},
+	{
+		.data_type = QMI_EOTI,
+		.elem_len  = 0,
+		.elem_size = 0,
+		.array_type  = NO_ARRAY,
+		.tlv_type  = 0,
+		.offset    = 0,
+		.ei_array  = NULL,
+	},
+};
+
+#endif
diff --git a/drivers/hwtracing/coresight/coresight-remote-etm.c b/drivers/hwtracing/coresight/coresight-remote-etm.c
new file mode 100644
index 000000000000..d895dc5d14c2
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-remote-etm.c
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/sysfs.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/coresight.h>
+#include "coresight-qmi.h"
+#include "coresight-priv.h"
+
+#define REMOTE_ETM_TRACE_ID_START	192
+
+DEFINE_CORESIGHT_DEVLIST(remote_etm_devs, "remote-etm");
+
+static DEFINE_MUTEX(remote_etm_lock);
+static LIST_HEAD(remote_etm_list);
+
+struct remote_etm_drvdata {
+	struct device			*dev;
+	struct coresight_device		*csdev;
+	struct mutex			mutex;
+	struct qmi_handle		handle;
+	uint32_t			inst_id;
+	bool				enable;
+	bool service_connected;
+	bool security;
+	struct sockaddr_qrtr s_addr;
+	struct list_head link;
+};
+
+static int service_remote_etm_new_server(struct qmi_handle *qmi,
+		struct qmi_service *svc)
+{
+	struct remote_etm_drvdata *drvdata = container_of(qmi,
+					struct remote_etm_drvdata, handle);
+
+	drvdata->s_addr.sq_family = AF_QIPCRTR;
+	drvdata->s_addr.sq_node = svc->node;
+	drvdata->s_addr.sq_port = svc->port;
+	drvdata->service_connected = true;
+	dev_info(drvdata->dev,
+		"Connection established between QMI handle and %d service\n",
+		drvdata->inst_id);
+
+	return 0;
+}
+
+static void service_remote_etm_del_server(struct qmi_handle *qmi,
+		struct qmi_service *svc)
+{
+	struct remote_etm_drvdata *drvdata = container_of(qmi,
+					struct remote_etm_drvdata, handle);
+	drvdata->service_connected = false;
+	dev_info(drvdata->dev,
+		"Connection disconnected between QMI handle and %d service\n",
+		drvdata->inst_id);
+}
+
+static struct qmi_ops server_ops = {
+	.new_server = service_remote_etm_new_server,
+	.del_server = service_remote_etm_del_server,
+};
+
+static int remote_etm_enable(struct coresight_device *csdev,
+			     struct perf_event *event, u32 mode)
+{
+	struct remote_etm_drvdata *drvdata =
+		dev_get_drvdata(csdev->dev.parent);
+	struct coresight_set_etm_req_msg_v01 req;
+	struct coresight_set_etm_resp_msg_v01 resp = { { 0, 0 } };
+	struct qmi_txn txn;
+	int ret;
+
+	mutex_lock(&drvdata->mutex);
+
+	if (!drvdata->service_connected) {
+		dev_err(drvdata->dev, "QMI service not connected!\n");
+		ret = EINVAL;
+		goto err;
+	}
+	/*
+	 * The QMI handle may be NULL in the following scenarios:
+	 * 1. QMI service is not present
+	 * 2. QMI service is present but attempt to enable remote ETM is earlier
+	 *    than service is ready to handle request
+	 * 3. Connection between QMI client and QMI service failed
+	 *
+	 * Enable CoreSight without processing further QMI commands which
+	 * provides the option to enable remote ETM by other means.
+	 */
+	req.state = CORESIGHT_ETM_STATE_ENABLED_V01;
+
+	ret = qmi_txn_init(&drvdata->handle, &txn,
+			coresight_set_etm_resp_msg_v01_ei,
+			&resp);
+
+	if (ret < 0) {
+		dev_err(drvdata->dev, "QMI tx init failed , ret:%d\n",
+				ret);
+		goto err;
+	}
+
+	ret = qmi_send_request(&drvdata->handle, &drvdata->s_addr,
+			&txn, CORESIGHT_QMI_SET_ETM_REQ_V01,
+			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
+			coresight_set_etm_req_msg_v01_ei,
+			&req);
+	if (ret < 0) {
+		dev_err(drvdata->dev, "QMI send ACK failed, ret:%d\n",
+				ret);
+		qmi_txn_cancel(&txn);
+		goto err;
+	}
+
+	ret = qmi_txn_wait(&txn, msecs_to_jiffies(TIMEOUT_MS));
+	if (ret < 0) {
+		dev_err(drvdata->dev, "QMI qmi txn wait failed, ret:%d\n",
+				ret);
+		goto err;
+	}
+
+	/* Check the response */
+	if (resp.resp.result != QMI_RESULT_SUCCESS_V01)
+		dev_err(drvdata->dev, "QMI request failed 0x%x\n",
+				resp.resp.error);
+
+	drvdata->enable = true;
+	mutex_unlock(&drvdata->mutex);
+
+	dev_info(drvdata->dev, "Remote ETM tracing enabled for instance %d\n",
+				drvdata->inst_id);
+	return 0;
+err:
+	mutex_unlock(&drvdata->mutex);
+	return ret;
+}
+
+static void remote_etm_disable(struct coresight_device *csdev,
+			       struct perf_event *event)
+{
+	struct remote_etm_drvdata *drvdata =
+		 dev_get_drvdata(csdev->dev.parent);
+	struct coresight_set_etm_req_msg_v01 req;
+	struct coresight_set_etm_resp_msg_v01 resp = { { 0, 0 } };
+	struct qmi_txn txn;
+	int ret;
+
+	mutex_lock(&drvdata->mutex);
+	if (!drvdata->service_connected) {
+		dev_err(drvdata->dev, "QMI service not connected!\n");
+		goto err;
+	}
+
+	req.state = CORESIGHT_ETM_STATE_DISABLED_V01;
+
+	ret = qmi_txn_init(&drvdata->handle, &txn,
+			coresight_set_etm_resp_msg_v01_ei,
+			&resp);
+
+	if (ret < 0) {
+		dev_err(drvdata->dev, "QMI tx init failed , ret:%d\n",
+				ret);
+		goto err;
+	}
+
+	ret = qmi_send_request(&drvdata->handle, &drvdata->s_addr,
+			&txn, CORESIGHT_QMI_SET_ETM_REQ_V01,
+			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
+			coresight_set_etm_req_msg_v01_ei,
+			&req);
+	if (ret < 0) {
+		dev_err(drvdata->dev, "QMI send req failed, ret:%d\n",
+				 ret);
+		qmi_txn_cancel(&txn);
+		goto err;
+	}
+
+	ret = qmi_txn_wait(&txn, msecs_to_jiffies(TIMEOUT_MS));
+	if (ret < 0) {
+		dev_err(drvdata->dev, "QMI qmi txn wait failed, ret:%d\n",
+				ret);
+		goto err;
+	}
+
+	/* Check the response */
+	if (resp.resp.result != QMI_RESULT_SUCCESS_V01) {
+		dev_err(drvdata->dev, "QMI request failed 0x%x\n",
+				resp.resp.error);
+		goto err;
+	}
+
+	drvdata->enable = false;
+	dev_info(drvdata->dev, "Remote ETM tracing disabled for instance %d\n",
+				drvdata->inst_id);
+err:
+	mutex_unlock(&drvdata->mutex);
+}
+
+static const struct coresight_ops_source remote_etm_source_ops = {
+	.enable		= remote_etm_enable,
+	.disable	= remote_etm_disable,
+};
+
+static const struct coresight_ops remote_cs_ops = {
+	.source_ops	= &remote_etm_source_ops,
+};
+
+static int remote_etm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct coresight_platform_data *pdata;
+	struct remote_etm_drvdata *drvdata;
+	struct coresight_desc desc = {0 };
+	int ret;
+
+	desc.name = coresight_alloc_device_name(&remote_etm_devs, dev);
+	if (!desc.name)
+		return -ENOMEM;
+	pdata = coresight_get_platform_data(dev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+	pdev->dev.platform_data = pdata;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->dev = &pdev->dev;
+	platform_set_drvdata(pdev, drvdata);
+
+	ret = of_property_read_u32(pdev->dev.of_node, "qcom,inst-id",
+			&drvdata->inst_id);
+	if (ret)
+		return ret;
+
+	mutex_init(&drvdata->mutex);
+
+	ret = qmi_handle_init(&drvdata->handle,
+			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
+			&server_ops, NULL);
+	if (ret < 0) {
+		dev_err(dev, "Remote ETM client init failed ret:%d\n", ret);
+		return ret;
+	}
+
+	qmi_add_lookup(&drvdata->handle,
+			CORESIGHT_QMI_SVC_ID,
+			CORESIGHT_QMI_VERSION,
+			drvdata->inst_id);
+
+	desc.type = CORESIGHT_DEV_TYPE_SOURCE;
+	desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC;
+	desc.ops = &remote_cs_ops;
+	desc.pdata = pdev->dev.platform_data;
+	desc.dev = &pdev->dev;
+	drvdata->csdev = coresight_register(&desc);
+	if (IS_ERR(drvdata->csdev)) {
+		ret = PTR_ERR(drvdata->csdev);
+		goto err;
+	}
+	dev_info(dev, "Remote ETM initialized\n");
+
+	pm_runtime_enable(dev);
+	if (drvdata->inst_id >= sizeof(int)*BITS_PER_BYTE)
+		dev_err(dev, "inst_id greater than boot_enable bit mask\n");
+
+	list_add_tail(&drvdata->link, &remote_etm_list);
+
+	return 0;
+err:
+	qmi_handle_release(&drvdata->handle);
+	return ret;
+}
+
+static int remote_etm_remove(struct platform_device *pdev)
+{
+	struct remote_etm_drvdata *drvdata = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+
+	list_del(&drvdata->link);
+	pm_runtime_disable(dev);
+	qmi_handle_release(&drvdata->handle);
+	coresight_unregister(drvdata->csdev);
+	return 0;
+}
+
+static const struct of_device_id remote_etm_match[] = {
+	{.compatible = "qcom,coresight-remote-etm"},
+	{}
+};
+
+static struct platform_driver remote_etm_driver = {
+	.probe          = remote_etm_probe,
+	.remove         = remote_etm_remove,
+	.driver         = {
+		.name   = "coresight-remote-etm",
+		.of_match_table = remote_etm_match,
+	},
+};
+
+int __init remote_etm_init(void)
+{
+	return platform_driver_register(&remote_etm_driver);
+}
+module_init(remote_etm_init);
+
+void __exit remote_etm_exit(void)
+{
+	platform_driver_unregister(&remote_etm_driver);
+}
+module_exit(remote_etm_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("CoreSight Remote ETM driver");
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index a4cb7dd6ca23..f0a947a61680 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -65,6 +65,7 @@ enum coresight_dev_subtype_source {
 	CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
 	CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
 	CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM,
+	CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC,
 	CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS,
 };
 
-- 
2.41.0


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

Re: [PATCH v1 1/2] coresight: Add remote etm support

[Krzysztof Kozlowski]

On 07/11/2023 07:09, Mao Jinlong wrote:
> Add support for ETM trace collection on remote processor using
> coreSight framework.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/Kconfig           |   9 +
>  drivers/hwtracing/coresight/Makefile          |   1 +
>  drivers/hwtracing/coresight/coresight-core.c  |   3 +
>  drivers/hwtracing/coresight/coresight-qmi.h   | 109 ++++++
>  .../coresight/coresight-remote-etm.c          | 325 ++++++++++++++++++
>  include/linux/coresight.h                     |   1 +
>  6 files changed, 448 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h
>  create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 06f0a7594169..425886ab7401 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -247,4 +247,13 @@ config CORESIGHT_DUMMY
>  
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called coresight-dummy.
> +
> +config CORESIGHT_REMOTE_ETM
> +	tristate "Remote processor ETM trace support"
> +	select QCOM_QMI_HELPERS
> +	help
> +	  Enables support for ETM trace collection on remote processor using
> +	  CoreSight framework. Enabling this will allow turning on ETM
> +	  tracing on remote processor via sysfs by configuring the required
> +	  CoreSight components.
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 995d3b2c76df..a5283cab0bc0 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -29,5 +29,6 @@ obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o
>  obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o
>  coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
>  		   coresight-cti-sysfs.o
> +obj-$(CONFIG_CORESIGHT_REMOTE_ETM) += coresight-remote-etm.o
>  obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>  obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index d7f0e231feb9..f365a3899821 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1094,6 +1094,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
>  	if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>  	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>  	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
> +	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC &&
>  	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>  		dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
>  		return -EINVAL;
> @@ -1164,6 +1165,7 @@ int coresight_enable(struct coresight_device *csdev)
>  		break;
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
> +	case CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
>  		/*
>  		 * Use the hash of source's device name as ID
> @@ -1215,6 +1217,7 @@ void coresight_disable(struct coresight_device *csdev)
>  		break;
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
> +	case CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
>  		hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
>  		/* Find the path by the hash. */
> diff --git a/drivers/hwtracing/coresight/coresight-qmi.h b/drivers/hwtracing/coresight/coresight-qmi.h
> new file mode 100644
> index 000000000000..4c35ba8c8a05
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-qmi.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _CORESIGHT_QMI_H
> +#define _CORESIGHT_QMI_H
> +
> +#include <linux/soc/qcom/qmi.h>
> +
> +#define CORESIGHT_QMI_SVC_ID			(0x33)
> +#define CORESIGHT_QMI_VERSION			(1)
> +
> +#define CORESIGHT_QMI_GET_ETM_REQ_V01		(0x002B)
> +#define CORESIGHT_QMI_GET_ETM_RESP_V01		(0x002B)
> +#define CORESIGHT_QMI_SET_ETM_REQ_V01		(0x002C)
> +#define CORESIGHT_QMI_SET_ETM_RESP_V01		(0x002C)
> +
> +#define CORESIGHT_QMI_GET_ETM_REQ_MAX_LEN	(0)
> +#define CORESIGHT_QMI_GET_ETM_RESP_MAX_LEN	(14)
> +#define CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN	(7)
> +#define CORESIGHT_QMI_SET_ETM_RESP_MAX_LEN	(7)
> +
> +#define TIMEOUT_MS				(10000)
> +
> +enum coresight_etm_state_enum_type_v01 {
> +	/* To force a 32 bit signed enum. Do not change or use */
> +	CORESIGHT_ETM_STATE_ENUM_TYPE_MIN_ENUM_VAL_V01 = INT_MIN,
> +	CORESIGHT_ETM_STATE_DISABLED_V01 = 0,
> +	CORESIGHT_ETM_STATE_ENABLED_V01 = 1,
> +	CORESIGHT_ETM_STATE_ENUM_TYPE_MAX_ENUM_VAL_01 = INT_MAX,
> +};
> +
> +struct coresight_get_etm_req_msg_v01 {
> +	/*
> +	 * This element is a placeholder to prevent declaration of
> +	 * empty struct. Do not change.
> +	 */
> +	char __placeholder;
> +};
> +
> +struct coresight_get_etm_resp_msg_v01 {
> +	/* Mandatory */
> +	/* QMI result Code */
> +	struct qmi_response_type_v01 resp;
> +
> +	/* Optional */
> +	/* ETM output state, must be set to true if state is being passed */
> +	uint8_t state_valid;
> +	/* Present when result code is QMI_RESULT_SUCCESS */
> +	enum coresight_etm_state_enum_type_v01 state;
> +};
> +
> +struct coresight_set_etm_req_msg_v01 {
> +	/* Mandatory */
> +	/* ETM output state */
> +	enum coresight_etm_state_enum_type_v01 state;
> +};
> +
> +struct coresight_set_etm_resp_msg_v01 {
> +	/* Mandatory */
> +	struct qmi_response_type_v01 resp;
> +};
> +
> +static struct qmi_elem_info coresight_set_etm_req_msg_v01_ei[] = {
> +	{
> +		.data_type = QMI_UNSIGNED_4_BYTE,
> +		.elem_len  = 1,
> +		.elem_size = sizeof(enum coresight_etm_state_enum_type_v01),
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0x01,
> +		.offset    = offsetof(struct coresight_set_etm_req_msg_v01,
> +				      state),
> +		.ei_array  = NULL,
> +	},
> +	{
> +		.data_type = QMI_EOTI,
> +		.elem_len  = 0,
> +		.elem_size = 0,
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0,
> +		.offset    = 0,
> +		.ei_array  = NULL,
> +	},
> +};
> +
> +static struct qmi_elem_info coresight_set_etm_resp_msg_v01_ei[] = {
> +	{
> +		.data_type = QMI_STRUCT,
> +		.elem_len  = 1,
> +		.elem_size = sizeof(struct qmi_response_type_v01),
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0x02,
> +		.offset    = offsetof(struct coresight_set_etm_resp_msg_v01,
> +				      resp),
> +		.ei_array  = qmi_response_type_v01_ei,
> +	},
> +	{
> +		.data_type = QMI_EOTI,
> +		.elem_len  = 0,
> +		.elem_size = 0,
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0,
> +		.offset    = 0,
> +		.ei_array  = NULL,
> +	},
> +};
> +
> +#endif
> diff --git a/drivers/hwtracing/coresight/coresight-remote-etm.c b/drivers/hwtracing/coresight/coresight-remote-etm.c
> new file mode 100644
> index 000000000000..d895dc5d14c2
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-remote-etm.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/sysfs.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/coresight.h>
> +#include "coresight-qmi.h"
> +#include "coresight-priv.h"
> +
> +#define REMOTE_ETM_TRACE_ID_START	192
> +
> +DEFINE_CORESIGHT_DEVLIST(remote_etm_devs, "remote-etm");


Why do you define file-scope variables?

> +
> +static DEFINE_MUTEX(remote_etm_lock);

Drop, not used.

> +static LIST_HEAD(remote_etm_list);

Drop, not used.

> +
> +struct remote_etm_drvdata {
> +	struct device			*dev;
> +	struct coresight_device		*csdev;
> +	struct mutex			mutex;
> +	struct qmi_handle		handle;
> +	uint32_t			inst_id;
> +	bool				enable;
> +	bool service_connected;

Your indentation is a mess.

> +	bool security;
> +	struct sockaddr_qrtr s_addr;
> +	struct list_head link;
> +};
> +
> +static int service_remote_etm_new_server(struct qmi_handle *qmi,
> +		struct qmi_service *svc)
> +{
> +	struct remote_etm_drvdata *drvdata = container_of(qmi,
> +					struct remote_etm_drvdata, handle);
> +
> +	drvdata->s_addr.sq_family = AF_QIPCRTR;
> +	drvdata->s_addr.sq_node = svc->node;
> +	drvdata->s_addr.sq_port = svc->port;
> +	drvdata->service_connected = true;
> +	dev_info(drvdata->dev,
> +		"Connection established between QMI handle and %d service\n",
> +		drvdata->inst_id);
> +
> +	return 0;
> +}
> +
> +static void service_remote_etm_del_server(struct qmi_handle *qmi,
> +		struct qmi_service *svc)
> +{
> +	struct remote_etm_drvdata *drvdata = container_of(qmi,
> +					struct remote_etm_drvdata, handle);
> +	drvdata->service_connected = false;
> +	dev_info(drvdata->dev,
> +		"Connection disconnected between QMI handle and %d service\n",
> +		drvdata->inst_id);
> +}
> +
> +static struct qmi_ops server_ops = {
> +	.new_server = service_remote_etm_new_server,
> +	.del_server = service_remote_etm_del_server,
> +};
> +
> +static int remote_etm_enable(struct coresight_device *csdev,
> +			     struct perf_event *event, u32 mode)
> +{
> +	struct remote_etm_drvdata *drvdata =
> +		dev_get_drvdata(csdev->dev.parent);
> +	struct coresight_set_etm_req_msg_v01 req;
> +	struct coresight_set_etm_resp_msg_v01 resp = { { 0, 0 } };
> +	struct qmi_txn txn;
> +	int ret;
> +
> +	mutex_lock(&drvdata->mutex);
> +
> +	if (!drvdata->service_connected) {
> +		dev_err(drvdata->dev, "QMI service not connected!\n");
> +		ret = EINVAL;
> +		goto err;
> +	}
> +	/*
> +	 * The QMI handle may be NULL in the following scenarios:
> +	 * 1. QMI service is not present
> +	 * 2. QMI service is present but attempt to enable remote ETM is earlier
> +	 *    than service is ready to handle request
> +	 * 3. Connection between QMI client and QMI service failed
> +	 *
> +	 * Enable CoreSight without processing further QMI commands which
> +	 * provides the option to enable remote ETM by other means.
> +	 */
> +	req.state = CORESIGHT_ETM_STATE_ENABLED_V01;
> +
> +	ret = qmi_txn_init(&drvdata->handle, &txn,
> +			coresight_set_etm_resp_msg_v01_ei,
> +			&resp);
> +
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI tx init failed , ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	ret = qmi_send_request(&drvdata->handle, &drvdata->s_addr,
> +			&txn, CORESIGHT_QMI_SET_ETM_REQ_V01,
> +			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
> +			coresight_set_etm_req_msg_v01_ei,
> +			&req);
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI send ACK failed, ret:%d\n",
> +				ret);
> +		qmi_txn_cancel(&txn);
> +		goto err;
> +	}
> +
> +	ret = qmi_txn_wait(&txn, msecs_to_jiffies(TIMEOUT_MS));
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI qmi txn wait failed, ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	/* Check the response */
> +	if (resp.resp.result != QMI_RESULT_SUCCESS_V01)
> +		dev_err(drvdata->dev, "QMI request failed 0x%x\n",
> +				resp.resp.error);
> +
> +	drvdata->enable = true;
> +	mutex_unlock(&drvdata->mutex);
> +
> +	dev_info(drvdata->dev, "Remote ETM tracing enabled for instance %d\n",
> +				drvdata->inst_id);

Why do you print so many info messages?

> +	return 0;
> +err:
> +	mutex_unlock(&drvdata->mutex);
> +	return ret;
> +}
> +
> +static void remote_etm_disable(struct coresight_device *csdev,
> +			       struct perf_event *event)
> +{
> +	struct remote_etm_drvdata *drvdata =
> +		 dev_get_drvdata(csdev->dev.parent);
> +	struct coresight_set_etm_req_msg_v01 req;
> +	struct coresight_set_etm_resp_msg_v01 resp = { { 0, 0 } };
> +	struct qmi_txn txn;
> +	int ret;
> +
> +	mutex_lock(&drvdata->mutex);
> +	if (!drvdata->service_connected) {
> +		dev_err(drvdata->dev, "QMI service not connected!\n");
> +		goto err;
> +	}
> +
> +	req.state = CORESIGHT_ETM_STATE_DISABLED_V01;
> +
> +	ret = qmi_txn_init(&drvdata->handle, &txn,
> +			coresight_set_etm_resp_msg_v01_ei,
> +			&resp);
> +
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI tx init failed , ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	ret = qmi_send_request(&drvdata->handle, &drvdata->s_addr,
> +			&txn, CORESIGHT_QMI_SET_ETM_REQ_V01,
> +			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
> +			coresight_set_etm_req_msg_v01_ei,
> +			&req);
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI send req failed, ret:%d\n",
> +				 ret);
> +		qmi_txn_cancel(&txn);
> +		goto err;
> +	}
> +
> +	ret = qmi_txn_wait(&txn, msecs_to_jiffies(TIMEOUT_MS));
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI qmi txn wait failed, ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	/* Check the response */
> +	if (resp.resp.result != QMI_RESULT_SUCCESS_V01) {
> +		dev_err(drvdata->dev, "QMI request failed 0x%x\n",
> +				resp.resp.error);
> +		goto err;
> +	}
> +
> +	drvdata->enable = false;
> +	dev_info(drvdata->dev, "Remote ETM tracing disabled for instance %d\n",
> +				drvdata->inst_id);
> +err:
> +	mutex_unlock(&drvdata->mutex);
> +}
> +
> +static const struct coresight_ops_source remote_etm_source_ops = {
> +	.enable		= remote_etm_enable,
> +	.disable	= remote_etm_disable,
> +};
> +
> +static const struct coresight_ops remote_cs_ops = {
> +	.source_ops	= &remote_etm_source_ops,
> +};
> +
> +static int remote_etm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct coresight_platform_data *pdata;
> +	struct remote_etm_drvdata *drvdata;
> +	struct coresight_desc desc = {0 };
> +	int ret;
> +
> +	desc.name = coresight_alloc_device_name(&remote_etm_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +	pdata = coresight_get_platform_data(dev);
> +	if (IS_ERR(pdata))
> +		return PTR_ERR(pdata);
> +	pdev->dev.platform_data = pdata;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "qcom,inst-id",
> +			&drvdata->inst_id);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&drvdata->mutex);
> +
> +	ret = qmi_handle_init(&drvdata->handle,
> +			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
> +			&server_ops, NULL);
> +	if (ret < 0) {
> +		dev_err(dev, "Remote ETM client init failed ret:%d\n", ret);
> +		return ret;

Use return dev_err_probe()

> +	}
> +
> +	qmi_add_lookup(&drvdata->handle,
> +			CORESIGHT_QMI_SVC_ID,
> +			CORESIGHT_QMI_VERSION,
> +			drvdata->inst_id);
> +
> +	desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> +	desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC;
> +	desc.ops = &remote_cs_ops;
> +	desc.pdata = pdev->dev.platform_data;
> +	desc.dev = &pdev->dev;
> +	drvdata->csdev = coresight_register(&desc);
> +	if (IS_ERR(drvdata->csdev)) {
> +		ret = PTR_ERR(drvdata->csdev);
> +		goto err;
> +	}
> +	dev_info(dev, "Remote ETM initialized\n");

Drop, quite useless.

> +
> +	pm_runtime_enable(dev);
> +	if (drvdata->inst_id >= sizeof(int)*BITS_PER_BYTE)
> +		dev_err(dev, "inst_id greater than boot_enable bit mask\n");
> +
> +	list_add_tail(&drvdata->link, &remote_etm_list);
> +
> +	return 0;
> +err:
> +	qmi_handle_release(&drvdata->handle);
> +	return ret;
> +}
> +
> +static int remote_etm_remove(struct platform_device *pdev)
> +{
> +	struct remote_etm_drvdata *drvdata = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	list_del(&drvdata->link);
> +	pm_runtime_disable(dev);
> +	qmi_handle_release(&drvdata->handle);
> +	coresight_unregister(drvdata->csdev);
> +	return 0;
> +}
> +
> +static const struct of_device_id remote_etm_match[] = {
> +	{.compatible = "qcom,coresight-remote-etm"},

Please order your patches correctly, so binding is documented before the
users.

> +	{}
> +};
> +
> +static struct platform_driver remote_etm_driver = {
> +	.probe          = remote_etm_probe,
> +	.remove         = remote_etm_remove,
> +	.driver         = {
> +		.name   = "coresight-remote-etm",
> +		.of_match_table = remote_etm_match,
> +	},
> +};
> +
> +int __init remote_etm_init(void)
> +{
> +	return platform_driver_register(&remote_etm_driver);
> +}
> +module_init(remote_etm_init);
> +
> +void __exit remote_etm_exit(void)
> +{
> +	platform_driver_unregister(&remote_etm_driver);
> +}
> +module_exit(remote_etm_exit);

Why aren't you using module platform driver?

Best regards,
Krzysztof


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

Re: [PATCH v1 1/2] coresight: Add remote etm support

[Jinlong Mao]

On 11/7/2023 11:26 PM, Krzysztof Kozlowski wrote:
> On 07/11/2023 07:09, Mao Jinlong wrote:
>> Add support for ETM trace collection on remote processor using
>> coreSight framework.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig           |   9 +
>>   drivers/hwtracing/coresight/Makefile          |   1 +
>>   drivers/hwtracing/coresight/coresight-core.c  |   3 +
>>   drivers/hwtracing/coresight/coresight-qmi.h   | 109 ++++++
>>   .../coresight/coresight-remote-etm.c          | 325 ++++++++++++++++++
>>   include/linux/coresight.h                     |   1 +
>>   6 files changed, 448 insertions(+)
>>   create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h
>>   create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index 06f0a7594169..425886ab7401 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -247,4 +247,13 @@ config CORESIGHT_DUMMY
>>   
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called coresight-dummy.
>> +
>> +config CORESIGHT_REMOTE_ETM
>> +	tristate "Remote processor ETM trace support"
>> +	select QCOM_QMI_HELPERS
>> +	help
>> +	  Enables support for ETM trace collection on remote processor using
>> +	  CoreSight framework. Enabling this will allow turning on ETM
>> +	  tracing on remote processor via sysfs by configuring the required
>> +	  CoreSight components.
>>   endif
.....
>> +}
>> +
>> +static const struct of_device_id remote_etm_match[] = {
>> +	{.compatible = "qcom,coresight-remote-etm"},
> Please order your patches correctly, so binding is documented before the
> users.
>
>> +	{}
>> +};
>> +
>> +static struct platform_driver remote_etm_driver = {
>> +	.probe          = remote_etm_probe,
>> +	.remove         = remote_etm_remove,
>> +	.driver         = {
>> +		.name   = "coresight-remote-etm",
>> +		.of_match_table = remote_etm_match,
>> +	},
>> +};
>> +
>> +int __init remote_etm_init(void)
>> +{
>> +	return platform_driver_register(&remote_etm_driver);
>> +}
>> +module_init(remote_etm_init);
>> +
>> +void __exit remote_etm_exit(void)
>> +{
>> +	platform_driver_unregister(&remote_etm_driver);
>> +}
>> +module_exit(remote_etm_exit);
> Why aren't you using module platform driver?
>
> Best regards,
> Krzysztof

Thanks for the review. I will check and address your comments.

Thanks
Jinlong Mao

>

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

Re: [PATCH v1 1/2] coresight: Add remote etm support

[kernel test robot]

Hi Mao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20231107]
[cannot apply to robh/for-next linus/master v6.6 v6.6-rc7 v6.6-rc6 v6.6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mao-Jinlong/coresight-Add-remote-etm-support/20231107-141331
base:   next-20231107
patch link:    https://lore.kernel.org/r/20231107060939.13449-2-quic_jinlmao%40quicinc.com
patch subject: [PATCH v1 1/2] coresight: Add remote etm support
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231108/202311081053.fWRkXGH0-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231108/202311081053.fWRkXGH0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311081053.fWRkXGH0-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwtracing/coresight/coresight-remote-etm.c:312:12: warning: no previous prototype for 'remote_etm_init' [-Wmissing-prototypes]
     312 | int __init remote_etm_init(void)
         |            ^~~~~~~~~~~~~~~
>> drivers/hwtracing/coresight/coresight-remote-etm.c:318:13: warning: no previous prototype for 'remote_etm_exit' [-Wmissing-prototypes]
     318 | void __exit remote_etm_exit(void)
         |             ^~~~~~~~~~~~~~~


vim +/remote_etm_init +312 drivers/hwtracing/coresight/coresight-remote-etm.c

   311	
 > 312	int __init remote_etm_init(void)
   313	{
   314		return platform_driver_register(&remote_etm_driver);
   315	}
   316	module_init(remote_etm_init);
   317	
 > 318	void __exit remote_etm_exit(void)
   319	{
   320		platform_driver_unregister(&remote_etm_driver);
   321	}
   322	module_exit(remote_etm_exit);
   323	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

Re: [PATCH v1 1/2] coresight: Add remote etm support

[James Clark]

On 07/11/2023 06:09, Mao Jinlong wrote:
> Add support for ETM trace collection on remote processor using
> coreSight framework.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/Kconfig           |   9 +
>  drivers/hwtracing/coresight/Makefile          |   1 +
>  drivers/hwtracing/coresight/coresight-core.c  |   3 +
>  drivers/hwtracing/coresight/coresight-qmi.h   | 109 ++++++
>  .../coresight/coresight-remote-etm.c          | 325 ++++++++++++++++++
>  include/linux/coresight.h                     |   1 +
>  6 files changed, 448 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h
>  create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 06f0a7594169..425886ab7401 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -247,4 +247,13 @@ config CORESIGHT_DUMMY
>  
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called coresight-dummy.
> +
> +config CORESIGHT_REMOTE_ETM
> +	tristate "Remote processor ETM trace support"
> +	select QCOM_QMI_HELPERS
> +	help
> +	  Enables support for ETM trace collection on remote processor using
> +	  CoreSight framework. Enabling this will allow turning on ETM
> +	  tracing on remote processor via sysfs by configuring the required
> +	  CoreSight components.
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 995d3b2c76df..a5283cab0bc0 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -29,5 +29,6 @@ obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o
>  obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o
>  coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
>  		   coresight-cti-sysfs.o
> +obj-$(CONFIG_CORESIGHT_REMOTE_ETM) += coresight-remote-etm.o
>  obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>  obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index d7f0e231feb9..f365a3899821 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1094,6 +1094,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
>  	if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>  	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>  	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
> +	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC &&

It's not really related to this patch, but it feels like we're starting
to add too many subtypes that all behave in the same way. Sometimes the
subtype is used for identification, and sometimes it's used for
behavior. But when it's used for behavior like in this case they all
mean the same thing.

We should probably have just three subtypes "PROC", "OTHER" and
"SOFTWARE". And add a new ID field for things like "TPDM", or even just
use the names for identification, if that's possible.

In this case your new one would be "OTHER" because it doesn't have
per-CPU behaviour, and it doesn't have the enable refcounting that
"SOFTWARE" has.

>  	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>  		dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
>  		return -EINVAL;
> @@ -1164,6 +1165,7 @@ int coresight_enable(struct coresight_device *csdev)
>  		break;
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
> +	case CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
>  		/*
>  		 * Use the hash of source's device name as ID
> @@ -1215,6 +1217,7 @@ void coresight_disable(struct coresight_device *csdev)
>  		break;
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
> +	case CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC:
>  	case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
>  		hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
>  		/* Find the path by the hash. */
> diff --git a/drivers/hwtracing/coresight/coresight-qmi.h b/drivers/hwtracing/coresight/coresight-qmi.h
> new file mode 100644
> index 000000000000..4c35ba8c8a05
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-qmi.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _CORESIGHT_QMI_H
> +#define _CORESIGHT_QMI_H
> +
> +#include <linux/soc/qcom/qmi.h>
> +
> +#define CORESIGHT_QMI_SVC_ID			(0x33)
> +#define CORESIGHT_QMI_VERSION			(1)
> +
> +#define CORESIGHT_QMI_GET_ETM_REQ_V01		(0x002B)
> +#define CORESIGHT_QMI_GET_ETM_RESP_V01		(0x002B)
> +#define CORESIGHT_QMI_SET_ETM_REQ_V01		(0x002C)
> +#define CORESIGHT_QMI_SET_ETM_RESP_V01		(0x002C)
> +
> +#define CORESIGHT_QMI_GET_ETM_REQ_MAX_LEN	(0)
> +#define CORESIGHT_QMI_GET_ETM_RESP_MAX_LEN	(14)
> +#define CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN	(7)
> +#define CORESIGHT_QMI_SET_ETM_RESP_MAX_LEN	(7)
> +
> +#define TIMEOUT_MS				(10000)
> +
> +enum coresight_etm_state_enum_type_v01 {
> +	/* To force a 32 bit signed enum. Do not change or use */
> +	CORESIGHT_ETM_STATE_ENUM_TYPE_MIN_ENUM_VAL_V01 = INT_MIN,
> +	CORESIGHT_ETM_STATE_DISABLED_V01 = 0,
> +	CORESIGHT_ETM_STATE_ENABLED_V01 = 1,
> +	CORESIGHT_ETM_STATE_ENUM_TYPE_MAX_ENUM_VAL_01 = INT_MAX,
> +};
> +
> +struct coresight_get_etm_req_msg_v01 {
> +	/*
> +	 * This element is a placeholder to prevent declaration of
> +	 * empty struct. Do not change.
> +	 */

But struct coresight_get_etm_req_msg_v01 is never used, so couldn't you
just delete it, then you won't get an issue with the empty struct?

> +	char __placeholder;
> +};
> +
> +struct coresight_get_etm_resp_msg_v01 {

Is this one used either? I couldn't find it. It looks like all of the
get stuff isn't, including the #defines. I know they might be added for
completeness or for use in the future, but if they haven't been tested
it might be better to just delete it all because there is a chance there
is a mistake in it anyway.


> +	/* Mandatory */
> +	/* QMI result Code */
> +	struct qmi_response_type_v01 resp;
> +
> +	/* Optional */
> +	/* ETM output state, must be set to true if state is being passed */
> +	uint8_t state_valid;
> +	/* Present when result code is QMI_RESULT_SUCCESS */
> +	enum coresight_etm_state_enum_type_v01 state;
> +};
> +
> +struct coresight_set_etm_req_msg_v01 {
> +	/* Mandatory */
> +	/* ETM output state */
> +	enum coresight_etm_state_enum_type_v01 state;
> +};
> +
> +struct coresight_set_etm_resp_msg_v01 {
> +	/* Mandatory */
> +	struct qmi_response_type_v01 resp;
> +};
> +
> +static struct qmi_elem_info coresight_set_etm_req_msg_v01_ei[] = {
> +	{
> +		.data_type = QMI_UNSIGNED_4_BYTE,
> +		.elem_len  = 1,
> +		.elem_size = sizeof(enum coresight_etm_state_enum_type_v01),
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0x01,
> +		.offset    = offsetof(struct coresight_set_etm_req_msg_v01,
> +				      state),
> +		.ei_array  = NULL,
> +	},
> +	{
> +		.data_type = QMI_EOTI,
> +		.elem_len  = 0,
> +		.elem_size = 0,
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0,
> +		.offset    = 0,
> +		.ei_array  = NULL,
> +	},
> +};
> +
> +static struct qmi_elem_info coresight_set_etm_resp_msg_v01_ei[] = {
> +	{
> +		.data_type = QMI_STRUCT,
> +		.elem_len  = 1,
> +		.elem_size = sizeof(struct qmi_response_type_v01),
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0x02,
> +		.offset    = offsetof(struct coresight_set_etm_resp_msg_v01,
> +				      resp),
> +		.ei_array  = qmi_response_type_v01_ei,
> +	},
> +	{
> +		.data_type = QMI_EOTI,
> +		.elem_len  = 0,
> +		.elem_size = 0,
> +		.array_type  = NO_ARRAY,
> +		.tlv_type  = 0,
> +		.offset    = 0,
> +		.ei_array  = NULL,
> +	},
> +};
> +
> +#endif
> diff --git a/drivers/hwtracing/coresight/coresight-remote-etm.c b/drivers/hwtracing/coresight/coresight-remote-etm.c
> new file mode 100644
> index 000000000000..d895dc5d14c2
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-remote-etm.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/sysfs.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/coresight.h>
> +#include "coresight-qmi.h"
> +#include "coresight-priv.h"
> +
> +#define REMOTE_ETM_TRACE_ID_START	192

This isn't used.

> +
> +DEFINE_CORESIGHT_DEVLIST(remote_etm_devs, "remote-etm");
> +
> +static DEFINE_MUTEX(remote_etm_lock);
> +static LIST_HEAD(remote_etm_list);
> +
> +struct remote_etm_drvdata {
> +	struct device			*dev;
> +	struct coresight_device		*csdev;
> +	struct mutex			mutex;
> +	struct qmi_handle		handle;
> +	uint32_t			inst_id;
> +	bool				enable;

enable is never read so can probably be dropped. If not, "enabled" would
be better than "enable".

> +	bool service_connected;
> +	bool security;

security is never used either.

> +	struct sockaddr_qrtr s_addr;
> +	struct list_head link;
> +};
> +
> +static int service_remote_etm_new_server(struct qmi_handle *qmi,
> +		struct qmi_service *svc)
> +{
> +	struct remote_etm_drvdata *drvdata = container_of(qmi,
> +					struct remote_etm_drvdata, handle);
> +
> +	drvdata->s_addr.sq_family = AF_QIPCRTR;
> +	drvdata->s_addr.sq_node = svc->node;
> +	drvdata->s_addr.sq_port = svc->port;
> +	drvdata->service_connected = true;
> +	dev_info(drvdata->dev,
> +		"Connection established between QMI handle and %d service\n",
> +		drvdata->inst_id);
> +
> +	return 0;
> +}
> +
> +static void service_remote_etm_del_server(struct qmi_handle *qmi,
> +		struct qmi_service *svc)
> +{
> +	struct remote_etm_drvdata *drvdata = container_of(qmi,
> +					struct remote_etm_drvdata, handle);
> +	drvdata->service_connected = false;
> +	dev_info(drvdata->dev,
> +		"Connection disconnected between QMI handle and %d service\n",
> +		drvdata->inst_id);
> +}
> +
> +static struct qmi_ops server_ops = {
> +	.new_server = service_remote_etm_new_server,
> +	.del_server = service_remote_etm_del_server,
> +};
> +
> +static int remote_etm_enable(struct coresight_device *csdev,
> +			     struct perf_event *event, u32 mode)
> +{
> +	struct remote_etm_drvdata *drvdata =
> +		dev_get_drvdata(csdev->dev.parent);
> +	struct coresight_set_etm_req_msg_v01 req;
> +	struct coresight_set_etm_resp_msg_v01 resp = { { 0, 0 } };
> +	struct qmi_txn txn;
> +	int ret;
> +
> +	mutex_lock(&drvdata->mutex);
> +
> +	if (!drvdata->service_connected) {
> +		dev_err(drvdata->dev, "QMI service not connected!\n");
> +		ret = EINVAL;

= -EINVAL?

> +		goto err;
> +	}
> +	/*
> +	 * The QMI handle may be NULL in the following scenarios:
> +	 * 1. QMI service is not present
> +	 * 2. QMI service is present but attempt to enable remote ETM is earlier
> +	 *    than service is ready to handle request
> +	 * 3. Connection between QMI client and QMI service failed
> +	 *
> +	 * Enable CoreSight without processing further QMI commands which
> +	 * provides the option to enable remote ETM by other means.
> +	 */
> +	req.state = CORESIGHT_ETM_STATE_ENABLED_V01;
> +
> +	ret = qmi_txn_init(&drvdata->handle, &txn,
> +			coresight_set_etm_resp_msg_v01_ei,
> +			&resp);
> +
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI tx init failed , ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	ret = qmi_send_request(&drvdata->handle, &drvdata->s_addr,
> +			&txn, CORESIGHT_QMI_SET_ETM_REQ_V01,
> +			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
> +			coresight_set_etm_req_msg_v01_ei,
> +			&req);
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI send ACK failed, ret:%d\n",
> +				ret);
> +		qmi_txn_cancel(&txn);
> +		goto err;
> +	}
> +
> +	ret = qmi_txn_wait(&txn, msecs_to_jiffies(TIMEOUT_MS));
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI qmi txn wait failed, ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	/* Check the response */
> +	if (resp.resp.result != QMI_RESULT_SUCCESS_V01)
> +		dev_err(drvdata->dev, "QMI request failed 0x%x\n",
> +				resp.resp.error);
> +
> +	drvdata->enable = true;
> +	mutex_unlock(&drvdata->mutex);
> +
> +	dev_info(drvdata->dev, "Remote ETM tracing enabled for instance %d\n",
> +				drvdata->inst_id);
> +	return 0;
> +err:
> +	mutex_unlock(&drvdata->mutex);
> +	return ret;
> +}
> +
> +static void remote_etm_disable(struct coresight_device *csdev,
> +			       struct perf_event *event)
> +{
> +	struct remote_etm_drvdata *drvdata =
> +		 dev_get_drvdata(csdev->dev.parent);
> +	struct coresight_set_etm_req_msg_v01 req;
> +	struct coresight_set_etm_resp_msg_v01 resp = { { 0, 0 } };
> +	struct qmi_txn txn;
> +	int ret;
> +
> +	mutex_lock(&drvdata->mutex);
> +	if (!drvdata->service_connected) {
> +		dev_err(drvdata->dev, "QMI service not connected!\n");
> +		goto err;
> +	}
> +
> +	req.state = CORESIGHT_ETM_STATE_DISABLED_V01;
> +
> +	ret = qmi_txn_init(&drvdata->handle, &txn,
> +			coresight_set_etm_resp_msg_v01_ei,
> +			&resp);
> +
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI tx init failed , ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	ret = qmi_send_request(&drvdata->handle, &drvdata->s_addr,
> +			&txn, CORESIGHT_QMI_SET_ETM_REQ_V01,
> +			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
> +			coresight_set_etm_req_msg_v01_ei,
> +			&req);
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI send req failed, ret:%d\n",
> +				 ret);
> +		qmi_txn_cancel(&txn);
> +		goto err;
> +	}
> +
> +	ret = qmi_txn_wait(&txn, msecs_to_jiffies(TIMEOUT_MS));
> +	if (ret < 0) {
> +		dev_err(drvdata->dev, "QMI qmi txn wait failed, ret:%d\n",
> +				ret);
> +		goto err;
> +	}
> +
> +	/* Check the response */
> +	if (resp.resp.result != QMI_RESULT_SUCCESS_V01) {
> +		dev_err(drvdata->dev, "QMI request failed 0x%x\n",
> +				resp.resp.error);
> +		goto err;
> +	}
> +
> +	drvdata->enable = false;
> +	dev_info(drvdata->dev, "Remote ETM tracing disabled for instance %d\n",
> +				drvdata->inst_id);
> +err:
> +	mutex_unlock(&drvdata->mutex);
> +}
> +
> +static const struct coresight_ops_source remote_etm_source_ops = {
> +	.enable		= remote_etm_enable,
> +	.disable	= remote_etm_disable,
> +};
> +
> +static const struct coresight_ops remote_cs_ops = {
> +	.source_ops	= &remote_etm_source_ops,
> +};
> +
> +static int remote_etm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct coresight_platform_data *pdata;
> +	struct remote_etm_drvdata *drvdata;
> +	struct coresight_desc desc = {0 };
> +	int ret;
> +
> +	desc.name = coresight_alloc_device_name(&remote_etm_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +	pdata = coresight_get_platform_data(dev);
> +	if (IS_ERR(pdata))
> +		return PTR_ERR(pdata);
> +	pdev->dev.platform_data = pdata;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "qcom,inst-id",
> +			&drvdata->inst_id);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&drvdata->mutex);
> +
> +	ret = qmi_handle_init(&drvdata->handle,
> +			CORESIGHT_QMI_SET_ETM_REQ_MAX_LEN,
> +			&server_ops, NULL);
> +	if (ret < 0) {
> +		dev_err(dev, "Remote ETM client init failed ret:%d\n", ret);
> +		return ret;
> +	}
> +
> +	qmi_add_lookup(&drvdata->handle,
> +			CORESIGHT_QMI_SVC_ID,
> +			CORESIGHT_QMI_VERSION,
> +			drvdata->inst_id);
> +
> +	desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> +	desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC;
> +	desc.ops = &remote_cs_ops;
> +	desc.pdata = pdev->dev.platform_data;
> +	desc.dev = &pdev->dev;
> +	drvdata->csdev = coresight_register(&desc);
> +	if (IS_ERR(drvdata->csdev)) {
> +		ret = PTR_ERR(drvdata->csdev);
> +		goto err;
> +	}
> +	dev_info(dev, "Remote ETM initialized\n");
> +
> +	pm_runtime_enable(dev);

I think as soon as coresight_register() returns, the coresight mutex is
unlocked and the device can be used, so this pm_runtime_enable() and the
inst_id check are too late.

> +	if (drvdata->inst_id >= sizeof(int)*BITS_PER_BYTE)
> +		dev_err(dev, "inst_id greater than boot_enable bit mask\n");

I didn't understand this inst_id bit. It's specifically a uint32_t, but
it can't be a higher value than 32? So it could be an 8 bit field? It
also looks like it makes sense to validate this where it's read from the
DT, rather than later.

> +
> +	list_add_tail(&drvdata->link, &remote_etm_list);
> +

Apart from being deleted later, I didn't see this link or
remote_etm_list being used.

> +	return 0;
> +err:
> +	qmi_handle_release(&drvdata->handle);
> +	return ret;
> +}
> +
> +static int remote_etm_remove(struct platform_device *pdev)
> +{
> +	struct remote_etm_drvdata *drvdata = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	list_del(&drvdata->link);
> +	pm_runtime_disable(dev);
> +	qmi_handle_release(&drvdata->handle);
> +	coresight_unregister(drvdata->csdev);
> +	return 0;
> +}
> +
> +static const struct of_device_id remote_etm_match[] = {
> +	{.compatible = "qcom,coresight-remote-etm"},
> +	{}
> +};
> +
> +static struct platform_driver remote_etm_driver = {
> +	.probe          = remote_etm_probe,
> +	.remove         = remote_etm_remove,
> +	.driver         = {
> +		.name   = "coresight-remote-etm",
> +		.of_match_table = remote_etm_match,
> +	},
> +};
> +
> +int __init remote_etm_init(void)
> +{
> +	return platform_driver_register(&remote_etm_driver);
> +}
> +module_init(remote_etm_init);
> +
> +void __exit remote_etm_exit(void)
> +{
> +	platform_driver_unregister(&remote_etm_driver);
> +}
> +module_exit(remote_etm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CoreSight Remote ETM driver");
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index a4cb7dd6ca23..f0a947a61680 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -65,6 +65,7 @@ enum coresight_dev_subtype_source {
>  	CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
>  	CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
>  	CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM,
> +	CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC,
>  	CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS,
>  };
>  

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

Re: [PATCH v1 1/2] coresight: Add remote etm support

[Jinlong Mao]

On 11/8/2023 7:19 PM, James Clark wrote:
>
> On 07/11/2023 06:09, Mao Jinlong wrote:
>> Add support for ETM trace collection on remote processor using
>> coreSight framework.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig           |   9 +
>>   drivers/hwtracing/coresight/Makefile          |   1 +
>>   drivers/hwtracing/coresight/coresight-core.c  |   3 +
>>   drivers/hwtracing/coresight/coresight-qmi.h   | 109 ++++++
>>   .../coresight/coresight-remote-etm.c          | 325 ++++++++++++++++++
>>   include/linux/coresight.h                     |   1 +
>>   6 files changed, 448 insertions(+)
>>   create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h
>>   create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index 06f0a7594169..425886ab7401 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -247,4 +247,13 @@ config CORESIGHT_DUMMY
>>   
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called coresight-dummy.
>> +
>> +config CORESIGHT_REMOTE_ETM
>> +	tristate "Remote processor ETM trace support"
>> +	select QCOM_QMI_HELPERS
>> +	help
>> +	  Enables support for ETM trace collection on remote processor using
>> +	  CoreSight framework. Enabling this will allow turning on ETM
>> +	  tracing on remote processor via sysfs by configuring the required
>> +	  CoreSight components.
>>   endif
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> index 995d3b2c76df..a5283cab0bc0 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -29,5 +29,6 @@ obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o
>>   obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o
>>   coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
>>   		   coresight-cti-sysfs.o
>> +obj-$(CONFIG_CORESIGHT_REMOTE_ETM) += coresight-remote-etm.o
>>   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>>   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index d7f0e231feb9..f365a3899821 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1094,6 +1094,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
>>   	if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>>   	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>>   	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>> +	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC &&
......
> +{
> +	return platform_driver_register(&remote_etm_driver);
> +}
> +module_init(remote_etm_init);
> +
> +void __exit remote_etm_exit(void)
> +{
> +	platform_driver_unregister(&remote_etm_driver);
> +}
> +module_exit(remote_etm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CoreSight Remote ETM driver");
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index a4cb7dd6ca23..f0a947a61680 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -65,6 +65,7 @@ enum coresight_dev_subtype_source {
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM,
> +	CORESIGHT_DEV_SUBTYPE_SOURCE_REMOTE_PROC,
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS,
>   };
>   
Thanks for the review. I will check the comments and address your 
comments in next version.

Thanks
Jinlong Mao


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

[PATCH v1 2/2] dt-bindings: arm: Add remote etm driver

[Mao Jinlong]

Add new coresight-remote-etm.yaml file describing the bindings required
to define coresight remote etm in the device trees.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../arm/qcom,coresight-remote-etm.yaml        | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
new file mode 100644
index 000000000000..04bb57b48d96
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/qcom,coresight-remote-etm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Coresight Remote ETM
+
+maintainers:
+  - Jinlong Mao <quic_jinlmao@quicinc.com>
+  - Tao Zhang <quic_taozha@quicinc.com>
+
+description: |
+  Support for ETM trace collection on remote processor using coresight
+  framework. Enabling this will allow turning on ETM tracing on remote
+  processor via sysfs and collecting the trace via TMC sinks.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,coresight-remote-etm
+
+  qcom,inst-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Instance id of remote etm.
+
+  out-ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    additionalProperties: false
+
+    properties:
+      port:
+        description: Output connection to the CoreSight Trace bus.
+        $ref: /schemas/graph.yaml#/properties/port
+
+required:
+  - compatible
+  - qcom,inst-id
+  - out-ports
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    modem_etm0 {
+        compatible = "qcom,coresight-remote-etm";
+        qcom,inst-id = <2>;
+
+        out-ports {
+            port {
+                modem_etm0_out_funnel_modem: endpoint {
+                remote-endpoint =
+                    <&funnel_modem_in_modem_etm0>;
+                };
+            };
+        };
+    };
+...
-- 
2.41.0


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

Re: [PATCH v1 2/2] dt-bindings: arm: Add remote etm driver

[Krzysztof Kozlowski]

On 07/11/2023 07:09, Mao Jinlong wrote:
> Add new coresight-remote-etm.yaml file describing the bindings required
> to define coresight remote etm in the device trees.

Subject: drop driver. Bindings are about hardware, not drivers.

> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../arm/qcom,coresight-remote-etm.yaml        | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
> new file mode 100644
> index 000000000000..04bb57b48d96
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/qcom,coresight-remote-etm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Coresight Remote ETM

What is ETM?

> +
> +maintainers:
> +  - Jinlong Mao <quic_jinlmao@quicinc.com>
> +  - Tao Zhang <quic_taozha@quicinc.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Support for ETM trace collection on remote processor using coresight
> +  framework. Enabling this will allow turning on ETM tracing on remote
> +  processor via sysfs and collecting the trace via TMC sinks.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,coresight-remote-etm
> +
> +  qcom,inst-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Instance id of remote etm.

It's the first time this property appears... Why do you need to
hard-code it and why would it differ between each board? If you want to
use existing, accepted property, then you must use exactly the same.

> +
> +  out-ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    additionalProperties: false
> +
> +    properties:
> +      port:
> +        description: Output connection to the CoreSight Trace bus.
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +required:
> +  - compatible
> +  - qcom,inst-id
> +  - out-ports
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    modem_etm0 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Underscores are not allowed.

> +        compatible = "qcom,coresight-remote-etm";
> +        qcom,inst-id = <2>;
> +
> +        out-ports {
> +            port {
> +                modem_etm0_out_funnel_modem: endpoint {
> +                remote-endpoint =
> +                    <&funnel_modem_in_modem_etm0>;

Fix wrapping.

> +                };
> +            };
> +        };
> +    };
> +...

Best regards,
Krzysztof


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

Re: [PATCH v1 2/2] dt-bindings: arm: Add remote etm driver

[Krzysztof Kozlowski]

On 07/11/2023 07:09, Mao Jinlong wrote:
> Add new coresight-remote-etm.yaml file describing the bindings required
> to define coresight remote etm in the device trees.
> 

...

> +title: Qualcomm Coresight Remote ETM
> +
> +maintainers:
> +  - Jinlong Mao <quic_jinlmao@quicinc.com>
> +  - Tao Zhang <quic_taozha@quicinc.com>
> +
> +description: |
> +  Support for ETM trace collection on remote processor using coresight
> +  framework. Enabling this will allow turning on ETM tracing on remote
> +  processor via sysfs and collecting the trace via TMC sinks.

BTW, your cover letter says more than commit messages and descriptions.
It should be reversed. Cover letter is meaningless afterwards.

Best regards,
Krzysztof


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

Re: [PATCH v1 2/2] dt-bindings: arm: Add remote etm driver

[Krzysztof Kozlowski]

On 07/11/2023 07:09, Mao Jinlong wrote:
> Add new coresight-remote-etm.yaml file describing the bindings required
> to define coresight remote etm in the device trees.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>

+Cc Trilok,

Several Qualcomm boards have warnings coming from Coresight bindings.
These are big, fat warnings coming usually from ARM bindings (e.g.
dynamic funnel, TMC). I don't know Coresight good enough to fix them by
myself.

I would prefer not to take any new Qualcomm specific Coresight bindings
and definitely no new Coresight device nodes in Qualcomm boards, before
these are fixed.

Therefore I kindly ask to fix all warnings in Qualcomm boards coming
from existing Coresight bindings.

Best regards,
Krzysztof


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

Re: [PATCH v1 2/2] dt-bindings: arm: Add remote etm driver

[Trilok Soni]

On 11/16/2023 9:22 AM, Krzysztof Kozlowski wrote:
> On 07/11/2023 07:09, Mao Jinlong wrote:
>> Add new coresight-remote-etm.yaml file describing the bindings required
>> to define coresight remote etm in the device trees.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> 
> +Cc Trilok,
> 
> Several Qualcomm boards have warnings coming from Coresight bindings.
> These are big, fat warnings coming usually from ARM bindings (e.g.
> dynamic funnel, TMC). I don't know Coresight good enough to fix them by
> myself.
> 
> I would prefer not to take any new Qualcomm specific Coresight bindings
> and definitely no new Coresight device nodes in Qualcomm boards, before
> these are fixed.
> 
> Therefore I kindly ask to fix all warnings in Qualcomm boards coming
> from existing Coresight bindings.

Thanks Krzysztof, we can look into it as first thing to fix. 
 
-- 
---Trilok Soni


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