Re: [RFC PATCH v2 01/13] drivers: coresight: cti: Initial CoreSight CTI Driver

Hi Mike,

On 01/05/2019 09:49, Mike Leach wrote:

This introduces a baseline CTI driver and associated configuration files.

Driver will probe for the device on the AMBA bus, and load the CTI driver on CoreSight ID match to CTI IDs in tables.

Initial sysfs support for enable / disable provided.

Default CTI interconnection data is generated based on hardware register signal counts, with no additional connection information

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/Kconfig | 13 + drivers/hwtracing/coresight/Makefile | 4 + .../hwtracing/coresight/coresight-cti-sysfs.c | 97 ++++ drivers/hwtracing/coresight/coresight-cti.c | 522 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.h | 228 ++++++++ drivers/hwtracing/coresight/coresight-priv.h | 7 + drivers/hwtracing/coresight/coresight.c | 8 + .../hwtracing/coresight/of_coresight-cti.c | 81 +++ include/linux/coresight.h | 26 + 9 files changed, 986 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-cti-sysfs.c create mode 100644 drivers/hwtracing/coresight/coresight-cti.c create mode 100644 drivers/hwtracing/coresight/coresight-cti.h create mode 100644 drivers/hwtracing/coresight/of_coresight-cti.c

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index ad34380cac49..9f1cac64f357 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -114,4 +114,17 @@ config CORESIGHT_CPU_DEBUG properly, please refer Documentation/trace/coresight-cpu-debug.txt for detailed description and the example for usage. +config CORESIGHT_CTI

• bool "CoreSight Cross Trigger Interface (CTI) driver"
• depends on ARM || ARM64
• select CORESIGHT_LINKS_AND_SINKS
• help

•  This driver provides support for CoreSight CTI and CTM components.
  

•  These provide hardware triggering events between CoreSight trace
  

•  source and sink components. These can be used  to halt trace or
  

•  inject events into the trace stream. CTI also provides a software
  

•  control to trigger same halt events. This can provide fast trace
  

•  halt compared to disabling sources and sinks normally in driver
  

•  software.
  

• endif

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 41870ded51a3..77402cfc7174 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -19,3 +19,7 @@ obj-$(CONFIG_CORESIGHT_DYNAMIC_REPLICATOR) += coresight-dynamic-replicator.o obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o +obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o \

• 		of_coresight-cti.o \
  

• 		coresight-cti-sysfs.o
  

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c new file mode 100644 index 000000000000..9b6749621dcb --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2019 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#include <linux/coresight.h> +#include <linux/coresight-pmu.h> +#include <linux/cpu.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/fs.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/moduleparam.h> +#include <linux/of.h> +#include <linux/perf_event.h> +#include <linux/pm_runtime.h> +#include <linux/pm_wakeup.h> +#include <linux/seq_file.h> +#include <linux/smp.h> +#include <linux/slab.h> +#include <linux/stat.h> +#include <linux/sysfs.h> +#include <linux/types.h> +#include <linux/uaccess.h>

+#include "coresight-cti.h"

+/* basic attributes */

nit: pointless comment, could be dropped.

+static ssize_t enable_show(struct device *dev,

• 	      struct device_attribute *attr,
  

• 	      char *buf)
  

+{

• int enable_req;
• bool enabled, powered, cpuid;
• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• ssize_t size = 0;
• enable_req = atomic_read(&drvdata->config.enable_req_count);
• spin_lock(&drvdata->spinlock);
• powered = drvdata->config.hw_powered;
• enabled = drvdata->config.hw_enabled;
• cpuid = drvdata->ctidev.cpu;
• spin_unlock(&drvdata->spinlock);
• if (powered) {

• size = scnprintf(buf, PAGE_SIZE, "cti %s; cpu%d powered;\n",
  

• 	enabled ? "enabled" : "disabled", cpuid);
  

• } else if (cpuid >= 0) {

• size = scnprintf(buf, PAGE_SIZE, "cti %s; cpu%d unpowered;\n",
  

• 	enable_req ? "enable req" : "disable req", cpuid);
  

• } else {

• size = scnprintf(buf, PAGE_SIZE, "cti %s; no assoc cpu;\n",
  

• 	enabled ? "enabled" : "disabled");
  

• }
• return size;

+}

+static ssize_t enable_store(struct device *dev,

• 	       struct device_attribute *attr,
  

• 	       const char *buf, size_t size)
  

+{

• int ret = 0;
• unsigned long val;
• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• ret = kstrtoul(buf, 16, &val);

nit: I would leave it 0, instead of hard coding 16 to accept any base.

• if (ret)

• return ret;
  

• if (val)

• ret = cti_enable(drvdata->csdev, NULL);
  

• else

• ret = cti_disable(drvdata->csdev, NULL);
  

• if (ret)

• return ret;
  

• return size;

+} +static DEVICE_ATTR_RW(enable);

+/* attribute and group sysfs tables. */

nit: same as above, may be dropped.

+static struct attribute *coresight_cti_attrs[] = {

• &dev_attr_enable.attr,
• NULL,

+};

+static const struct attribute_group coresight_cti_group = {

• .attrs = coresight_cti_attrs,

+};

+const struct attribute_group *coresight_cti_groups[] = {

• &coresight_cti_group,
• NULL,

+}; diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c new file mode 100644 index 000000000000..0a0fb6d48237 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -0,0 +1,522 @@

...

+/**

• • CTI devices can be associated with a PE, or be connected to CoreSight
• • hardware. We have a list of all CTIs, and a subset array associated
• • with individual PEs for pwr management as they will power up in the

nit: s/pwr/power/

• • PE power domain.
• • At this point we assume that the none CPU CTIs are always powered as
• • we do with sinks etc.

nit: s/At this point//

• • We leave the client to figure out if all the CTIs are interconnected with
• • the same CTM, in general this is the case but does not always have to be.
• */

+struct ect_node {

• struct cti_drvdata *cti_drv;
• struct list_head next;

+};

+/* net of CTI devices connected via CTM */ +LIST_HEAD(ect_net);

+/* protect the list */ +static DEFINE_MUTEX(ect_mutex);

+/* quick reference for CPU CTIs */ +static struct cti_drvdata *cti_cpu_drv[NR_CPUS];

nit: Could this be made __percpu instead ?

+/* number of registered CTI devices */ +static int cti_count;

+/* number of cpu associated CTI devices in use */ +static int cti_cpu_count;

+#define csdev_to_cti_drvdata(csdev) \

• dev_get_drvdata(csdev->dev.parent)

+/* write set of regs to hardware - call with spinlock claimed */ +static void cti_write_all_hw_regs(struct cti_drvdata *drvdata) +{

• struct cti_config *config = &drvdata->config;
• int i;
• CS_UNLOCK(drvdata->base);
• /* disable CTI before writing registers */
• writel_relaxed(0, drvdata->base + CTICONTROL);
• /* write the CTI trigger registers */
• for (i = 0; i < config->nr_trig_max; i++) {

• writel_relaxed(config->ctiinen[i], drvdata->base + CTIINEN(i));
  

• writel_relaxed(config->ctiouten[i],
  

• 	drvdata->base + CTIOUTEN(i));
  

• }
• /* other regs */
• writel_relaxed(config->ctigate, drvdata->base + CTIGATE);
• writel_relaxed(config->asicctl, drvdata->base + ASICCTL);
• writel_relaxed(config->ctiappset, drvdata->base + CTIAPPSET);
• /* re-enable CTI */
• writel_relaxed(1, drvdata->base + CTICONTROL);
• CS_LOCK(drvdata->base);

+}

+static void cti_enable_hw_smp_call(void *info) +{

• struct cti_drvdata *drvdata = info;
• cti_write_all_hw_regs(drvdata);

+}

+/* write regs to hardware and enable */ +static int cti_enable_hw(void *info) +{

• struct cti_drvdata *drvdata = info;
• struct cti_config *config = &drvdata->config;
• int rc = 0;
• pm_runtime_get_sync(drvdata->dev);
• spin_lock(&drvdata->spinlock);
• /* no need to do anything if enabled or unpowered*/
• if (config->hw_enabled || !config->hw_powered)

• goto cti_not_enabled;
  

• /* claim the device */
• rc = coresight_claim_device(drvdata->base);
• if (rc)

• goto cti_not_enabled;
  

• if (drvdata->ctidev.cpu >= 0) {

• dev_info(drvdata->dev, "cti enable smp call for cpu %d\n",
  

Please could we make it dev_dbg() instead ?

• 	 drvdata->ctidev.cpu);
  

• rc = smp_call_function_single(drvdata->ctidev.cpu,
  

• 			 cti_enable_hw_smp_call,
  

• 			 drvdata, 1);
  

Is there any reason why these need to be bound to the CPU ?

• if (rc)
  

• 	goto cti_not_enabled;
  

• } else {

• dev_info(drvdata->dev, "cti enable not cpu call\n");
  

Same as above, dev_dbg() ?

• cti_write_all_hw_regs(drvdata);
  

• }

--- Cut here ---

• config->hw_enabled = true;
• spin_unlock(&drvdata->spinlock);
• return rc;
• /* not enabled in this call */

+cti_not_enabled:

• spin_unlock(&drvdata->spinlock);
• pm_runtime_put(drvdata->dev);

nit: You may be able to consolidate the unlock/exit above as:

config->hw_enabled = true;

unlock: spin_unlock(&drvdata->spinlock); if (rc) pm_runtime_put(drvdata->dev); return rc;

• return rc;

+}

+/* disable hardware */ +static int cti_disable_hw(void *info) +{

• struct cti_drvdata *drvdata = info;
• struct cti_config *config = &drvdata->config;
• spin_lock(&drvdata->spinlock);
• /* no need to do anything if disabled or cpu unpowered*/
• if (!config->hw_enabled || !config->hw_powered) {

• spin_unlock(&drvdata->spinlock);
  

• return 0;
  

• }
• CS_UNLOCK(drvdata->base);
• /* disable CTI */
• writel_relaxed(0, drvdata->base + CTICONTROL);
• config->hw_enabled = false;
• coresight_disclaim_device_unlocked(drvdata->base);
• CS_LOCK(drvdata->base);
• spin_unlock(&drvdata->spinlock);
• pm_runtime_put(drvdata->dev);
• return 0;

+}

+static void cti_set_default_config(struct cti_config *config) +{

• /* Most regs default to 0 as zalloc'ed except...*/
• config->trig_filter_enable = 1;
• config->ctigate = ((u32)0x1 << config->nr_ctm_channels) - 1;
• atomic_set(&config->enable_req_count, 0);

+}

+/*

• • Add a connection entry to the list of connections for this
• • CTI device.
• */

+static int cti_add_connection_entry(struct cti_drvdata *drvdata,

• 		    struct cti_trig_grp *in_info,
  

• 		    struct cti_trig_grp *out_info,
  

• 		    struct coresight_device *csdev,
  

• 		    const char *assoc_dev_name)
  

+{

• struct cti_trig_con *tc;
• struct cti_device *cti_dev = &drvdata->ctidev;
• tc = kzalloc(sizeof(struct cti_trig_con), GFP_KERNEL);
• if (tc == 0)

Does this need to be NULL explicitly for consistency ?

• return -ENOMEM;
  

• tc->con_dev = csdev;
• /*

• * Prefer actual associated CS device dev name to supplied value -
  

• * which is likely to be node name / other conn name.
  

• */
  

• if (csdev)

• tc->con_dev_name = kstrdup(dev_name(&csdev->dev), GFP_KERNEL);
  

• else if (assoc_dev_name != NULL)

• tc->con_dev_name = kstrdup(assoc_dev_name, GFP_KERNEL);
  

• tc->con_in = in_info;
• tc->con_out = out_info;
• list_add_tail(&tc->node, &cti_dev->trig_cons);
• cti_dev->nr_trig_con++;
• /* add connection usage bit info to overall info */
• drvdata->config.trig_in_use |= in_info->used_mask;
• drvdata->config.trig_out_use |= out_info->used_mask;
• return 0;

+}

+/*

• • Add a default connection if nothing else is specified.
• • single connection based on max in/out info, no assoc device
• */

+int cti_add_default_connection(struct cti_drvdata *drvdata) +{

• int ret = 0, idx;

nit: If you set ret = -ENOMEM here or further down before the mem allocs you could save some lines and make it easier to follow.

• struct cti_trig_grp *in = 0, *out = 0;

Please could we use NULL instead ?

• int n_trigs = drvdata->config.nr_trig_max;
• u32 n_trig_mask = (0x1 << n_trigs) - 1;

nit: n_trig_mask = GENMASK(n_trigs - 1, 0);

• int *in_sig_types = 0, *out_sig_types = 0;

Same here.

• in = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
• if (!in) {

• ret = -ENOMEM;
  

• goto conn_add_err;
  

• }
• out = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
• if (!out) {

• ret = -ENOMEM;
  

• goto conn_add_err;
  

• }
• in_sig_types = kzalloc(sizeof(int) * n_trigs, GFP_KERNEL);
• if (!in_sig_types) {

• ret = -ENOMEM;
  

• goto conn_add_err;
  

• }
• out_sig_types = kzalloc(sizeof(int) * n_trigs, GFP_KERNEL);
• if (!out_sig_types) {

• ret = -ENOMEM;
  

• goto conn_add_err;
  

• }
• /* signal types marked as default CTITRIG_GEN_IO */
• for (idx = 0; idx < n_trigs; idx++) {

• in_sig_types[idx] = CTITRIG_GEN_IO;
  

• out_sig_types[idx] = CTITRIG_GEN_IO;
  

• }
• /*

• * Assume max trigs for in and out,
  

• * all used, default sig types allocated
  

• */
  

• in->nr_sigs = n_trigs;
• in->used_mask = n_trig_mask;
• in->sig_types = in_sig_types;
• out->nr_sigs = n_trigs;
• out->used_mask = n_trig_mask;
• out->sig_types = out_sig_types;

• ret = cti_add_connection_entry(drvdata, in, out, NULL, "default");
• if (ret)

• goto conn_add_err;
  

• return ret;

+conn_add_err:

• kfree(in);
• kfree(out);
• kfree(out_sig_types);
• kfree(in_sig_types);
• return ret;

+}

+/** cti ect operations **/ +int cti_enable(struct coresight_device *csdev, void *__unused) +{

• int rc;
• struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
• atomic_inc(&drvdata->config.enable_req_count);
• rc = cti_enable_hw(drvdata);
• if (rc)

• atomic_dec(&drvdata->config.enable_req_count);
  

• return rc;

+}

+int cti_disable(struct coresight_device *csdev, void *__unused) +{

• int rc = 0;
• struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
• if (!atomic_dec_return(&drvdata->config.enable_req_count)) {

• rc = cti_disable_hw(drvdata);
  

• if (rc)
  

• 	atomic_inc(&drvdata->config.enable_req_count);
  

• }
• return rc;

+}

+const struct coresight_ops_ect cti_ops_ect = {

• .enable = cti_enable,
• .disable = cti_disable,

+};

+const struct coresight_ops cti_ops = {

• .ect_ops = &cti_ops_ect,

+};

+static int cti_probe(struct amba_device *adev, const struct amba_id *id) +{

• int ret = 0;
• u32 devid;
• void __iomem *base;
• struct device *dev = &adev->dev;
• struct cti_drvdata *drvdata = NULL;
• struct coresight_desc cti_desc;
• struct coresight_platform_data *pdata = NULL;
• struct resource *res = &adev->res;
• struct device_node *np = adev->dev.of_node;
• struct ect_node *ect_nd = NULL;
• /* boilerplate code to set up the basics */
• if (np) {

• pdata = of_get_coresight_cti_platform_data(dev, np);
  

• if (IS_ERR(pdata)) {
  

• 	dev_info(dev, "of_get_coresight_ect_platform err\n");
  

• 	return PTR_ERR(pdata);
  

• }
  

• dev->platform_data = pdata;
  

• }
• /* node to keep track of CTI net */
• ect_nd = devm_kzalloc(dev, sizeof(struct ect_node), GFP_KERNEL);

nit: If you move the ect_node under the drvdata, you could save some lines of code and runtime cost of looking up the ect_node while removing the device.

• if (!ect_nd) {

• ret = -ENOMEM;
  

• goto err_out;
  

• }
• /* driver data*/
• drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
• if (!drvdata) {

• ret = -ENOMEM;
  

• dev_info(dev, "%s, mem err\n", __func__);
  

• goto err_out;
  

• }
• /* Validity for the resource is already checked by the AMBA core */
• base = devm_ioremap_resource(dev, res);
• if (IS_ERR(base)) {

• ret = PTR_ERR(base);
  

• dev_info(dev, "%s, remap err\n", __func__);
  

• goto err_out;
  

• }
• drvdata->base = base;
• /* links between dev and drvdata*/
• drvdata->dev = dev;
• dev_set_drvdata(dev, drvdata);
• /* default CTI device info */
• drvdata->ctidev.cpu = pdata->cpu;
• drvdata->ctidev.nr_trig_con = 0;
• drvdata->ctidev.ctm_id = 0;
• INIT_LIST_HEAD(&drvdata->ctidev.trig_cons);
• spin_lock_init(&drvdata->spinlock);
• /* look at the HW DEVID register for some of the HW settings */
• devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
• drvdata->config.nr_trig_max = (int)((devid & 0xFF00) >> 8);

Please could we have proper macros to do these exraction ? e.g,

CTI_DEVID_NTRIG(devid)

• /*

• * no current hardware should exceed this, but protect the driver
  

• * in case of fault / out of spec hw
  

• */
  

• if (drvdata->config.nr_trig_max > CTIINOUTEN_MAX) {

• dev_warn_once(dev,
  

• 	"Limiting HW MaxTrig value(%d) to driver max(%d)\n",
  

• 	drvdata->config.nr_trig_max, CTIINOUTEN_MAX);
  

• drvdata->config.nr_trig_max = CTIINOUTEN_MAX;
  

• }
• drvdata->config.nr_ctm_channels = (int)((devid & 0xF0000) >> 16);

CTI_DEVID_NCTM_CHANS(devid)

• /* additional parse the .dts for connections and signals */
• of_cti_get_hw_data(dev, np, drvdata);
• /* initialise default driver values */
• cti_set_default_config(&drvdata->config);
• /* setup cpu related CTI devices, otherwise assume powered */
• drvdata->config.hw_powered = true;
• /* set up coresight component description */
• cti_desc.pdata = pdata;
• cti_desc.type = CORESIGHT_DEV_TYPE_ECT;
• cti_desc.subtype.ect_subtype = CORESIGHT_DEV_SUBTYPE_ECT_CTI;
• cti_desc.ops = &cti_ops;
• cti_desc.groups = coresight_cti_groups;
• cti_desc.dev = dev;
• drvdata->csdev = coresight_register(&cti_desc);
• if (IS_ERR(drvdata->csdev)) {

• ret = PTR_ERR(drvdata->csdev);
  

• pr_err("%s: CS register failed\n", pdata->name);
  

• goto err_out;
  

• }
• /* add to list of CTI devices */
• mutex_lock(&ect_mutex);
• ect_nd->cti_drv = drvdata;
• list_add(&ect_nd->next, &ect_net);
• /* if cpu bound add to CPU array */
• if (drvdata->ctidev.cpu >= 0) {

• cti_cpu_drv[drvdata->ctidev.cpu] = drvdata;
  

• cti_cpu_count++;
  

• }
• mutex_unlock(&ect_mutex);
• /* all done - dec pm refcount */
• pm_runtime_put(&adev->dev);
• dev_info(dev, "%s: initialized\n", pdata->name);
• cti_count++;
• return 0;

+err_out:

• kfree(drvdata);
• kfree(ect_nd);
• return ret;

+}

+/* free all connection info from the driver */ +void cti_free_conn_info(struct cti_drvdata *drvdata) +{

• struct cti_trig_con *tc, *tc_tmp;
• list_for_each_entry_safe(tc, tc_tmp,

• 		  &drvdata->ctidev.trig_cons, node) {
  

• kfree(tc->con_in->sig_types);
  

• kfree(tc->con_in);
  

• kfree(tc->con_out->sig_types);
  

• kfree(tc->con_out);
  

• kfree(tc->con_dev_name);
  

• list_del(&tc->node);
  

• kfree(tc);
  

• }

+}

+/*

• • Free up CTI specific resources
• • • called from coresight_device_release on coresight_unregister.
• */

+void cti_device_release(struct device *dev) +{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct ect_node *ect_item, *ect_tmp;
• /* free up resources associated with the cti connections */
• if (drvdata->ctidev.cpu >= 0)

• cti_cpu_drv[drvdata->ctidev.cpu] = 0;
  

• /* clear the connection list items */
• cti_free_conn_info(drvdata);
• /* remove from the list */
• mutex_lock(&ect_mutex);
• list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, next) {

As mentioned above, if ect_node is embedded in drvdata, you could avoid the list walk.

• if (ect_item->cti_drv == drvdata) {
  

• 	list_del(&ect_item->next);
  

• 	kfree(ect_item);
  

• 	goto ect_list_item_removed;
  

nit : break; ?

• }
  

• }

+ect_list_item_removed:

• mutex_unlock(&ect_mutex);
• kfree(drvdata);

+} +EXPORT_SYMBOL_GPL(cti_device_release);

Why does this need to be exported ? May be I will find the answer in later patches. I will keep an eye.

diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h new file mode 100644 index 000000000000..c1af6cf9c2fe --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -0,0 +1,228 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (c) 2018 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#ifndef _CORESIGHT_CORESIGHT_CTI_H +#define _CORESIGHT_CORESIGHT_CTI_H

+#include <asm/local.h> +#include <linux/spinlock.h> +#include "coresight-priv.h"

+/*

• • Device registers
• • 0x000 - 0x144: CTI programming and status
• • 0xEDC - 0xEF8: CTI integration test.
• • 0xF00 - 0xFFC: Coresight management registers.
• */

+/* CTI programming registers */ +#define CTICONTROL 0x000 +#define CTIINTACK 0x010 +#define CTIAPPSET 0x014 +#define CTIAPPCLEAR 0x018 +#define CTIAPPPULSE 0x01C +#define CTIINEN(n) (0x020 + (4 * n)) +#define CTIOUTEN(n) (0x0A0 + (4 * n)) +#define CTITRIGINSTATUS 0x130 +#define CTITRIGOUTSTATUS 0x134 +#define CTICHINSTATUS 0x138 +#define CTICHOUTSTATUS 0x13C +#define CTIGATE 0x140 +#define ASICCTL 0x144 +/* Integration test registers */ +#define ITCHINACK 0xEDC /* WO CTI CSSoc 400 only*/ +#define ITTRIGINACK 0xEE0 /* WO CTI CSSoc 400 only*/ +#define ITCHOUT 0xEE4 /* WO RW-600 */ +#define ITTRIGOUT 0xEE8 /* WO RW-600 */ +#define ITCHOUTACK 0xEEC /* RO CTI CSSoc 400 only*/ +#define ITTRIGOUTACK 0xEF0 /* RO CTI CSSoc 400 only*/ +#define ITCHIN 0xEF4 /* RO */ +#define ITTRIGIN 0xEF8 /* RO */

Do we use the Integration Test registers at all ? If not we could remove them.

+/* management registers */ +#define CTIDEVAFF0 0xFA8 +#define CTIDEVAFF1 0xFAC

+/*

• • CTI CSSoc 600 has a max of 32 trigger signals per direction.
• • CTI CSSoc 400 has 8 IO triggers - other CTIs can be impl def.
• • Max of in and out defined in the DEVID register.
• • • pick up actual number used from .dts parameters if present.
• */

+#define CTIINOUTEN_MAX 32

+/*

• • CTI Trigger signal type definitions.
• • Labels for certain trigger interconnections between the CTI
• • and standard CS components.
• • Must match values in ./include/dt-bindings/arm/coresight-cti.h

Why not have these common bits in a shared header file ?

• */

+#define CTITRIG_GEN_IO 0 +#define CTITRIG_GEN_INTREQ 1 +#define CTITRIG_GEN_INTACK 2 +#define CTITRIG_GEN_HALTREQ 3 +#define CTITRIG_GEN_RESTARTREQ 4 +#define CTITRIG_PE_EDBGREQ 5 +#define CTITRIG_PE_DBGRESTART 6 +#define CTITRIG_PE_CTIIRQ 7 +#define CTITRIG_PE_PMUIRQ 8 +#define CTITRIG_PE_DBGTRIGGER 9 +#define CTITRIG_ETM_EXTOUT 10 +#define CTITRIG_ETM_EXTIN 11 +#define CTITRIG_SNK_FULL 12 +#define CTITRIG_SNK_ACQCOMP 13 +#define CTITRIG_SNK_FLUSHCOMP 14 +#define CTITRIG_SNK_FLUSHIN 15 +#define CTITRIG_SNK_TRIGIN 16 +#define CTITRIG_STM_ASYNCOUT 17 +#define CTITRIG_STM_TOUT_SPTE 18 +#define CTITRIG_STM_TOUT_SW 19 +#define CTITRIG_STM_TOUT_HETE 20 +#define CTITRIG_STM_HWEVENT 21 +#define CTITRIG_ELA_TSTART 22 +#define CTITRIG_ELA_TSTOP 23 +#define CTITRIG_ELA_DBGREQ 24

+/**

• • Group of related trigger signals
• • @nr_sigs: number of signals in the group.
• • @used_mask: bitmask representing the signal indexes in the group.
• • @sig_types: array of types for the signals, length nr_sigs.
• */

+struct cti_trig_grp {

• int nr_sigs;
• u32 used_mask;
• int *sig_types;

minor nit: If you make this: int sig_types[0];

You may be able to allocate in one shot and freeing up becomes easier.

cti_trig_grp *grp = kzalloc(sizeof(cti_trig_grp) + sizeof(int) * nr_trigs, GFP_KERNEL);

+};

+/**

• • Trigger connection - connection between a CTI and other (coresight) device
• • lists input and output trigger signals for the device
• • @con_in: connected CTIIN signals for the device.
• • @con_out: connected CTIOUT signals for the device.
• • @con_dev: coresight device connected to the CTI, NULL if not CS device
• • @con_dev_name: name of connected device (CS or CPU)
• • @node: entry node in list of connections.
• */

+struct cti_trig_con {

• struct cti_trig_grp *con_in;
• struct cti_trig_grp *con_out;
• struct coresight_device *con_dev;
• char *con_dev_name;
• struct list_head node;

+};

+/**

• • struct cti_device - description of CTI device properties.
• • @nt_trig_con: Number of external devices connected to this device.
• • @ctm_id: which CTM this device is connected to (by default it is

• •      assumed there is a single CTM per SoC, ID 0).
    

• • @trig_cons: list of connections to this device.
• • @cpu: CPU ID if associated with CPU, -1 otherwise.
• */

+struct cti_device {

• int nr_trig_con;
• u32 ctm_id;
• struct list_head trig_cons;
• int cpu;

+};

+/**

• • struct cti_config - configuration of the CTI device hardware
• • hardware description from RO ID regs

nit: s/hardware/Hardware

nit: Please could you leave a TAB at the beginning to show that it is a subtopic. And also, if you align the field description by TABs, it will be better readable:

e.g * Hardware description from ID regs * @nr_trig_max: Max number of .... * and the description continues here. * ... * * CTI Enable Control * @enable_req_count: ....

• • @nr_trig_max: Max number of trigger signals implemented on device.

• • (max of trig_in or trig_out)
    

• • @nr_ctm_channels: number of available CTM channels
• • cti enable control
• • @enable_req_count: CTI is enabled alongside >=1 associated devices.
• • @hw_enabled: true if hw is currently enabled.
• • @hw_powered: true if associated cpu powered on, or no cpu.
• • registered triggers and filtering

• • @trig_in_use: bitfield of in triggers registered as in use.
• • @trig_out_use: bitfield of out triggers registered as in use.
• • @trig_out_filter: bitfield of out triggers that are blocked if filter
• • enabled. Typically this would be dbgreq / restart on a core CTI.
• • @trig_filter_enable: 1 if filtering enabled.
• • cti software programmable regs:
• • @ctiappset: CTI Software application channel set.
• • @ctiinout_sel: register selector for INEN and OUTEN regs.
• • @ctiinen: enable input trigger to a channel.
• • @ctiouten: enable output trigger from a channel.
• • @ctigate: gate channel output from CTI to CTM.
• */

...

+/**

• • struct cti_drvdata - specifics for the CTI device
• • @base: Memory mapped base address for this component.
• • @dev: The device entity associated to this component.
• • @csdev: Standard CoreSight device information.
• • @ctidev: Extra information needed by the CTI/CTM framework.
• • @spinlock: Control data access to one at a time.
• • @config: Configuration data for this CTI device.
• */

+struct cti_drvdata {

• void __iomem *base;
• struct device *dev;
• struct coresight_device *csdev;
• struct cti_device ctidev;
• spinlock_t spinlock;
• struct cti_config config;

+};

+/* private cti driver fns & vars */ +extern const struct attribute_group *coresight_cti_groups[]; +int cti_add_default_connection(struct cti_drvdata *drvdata); +int cti_enable(struct coresight_device *csdev, void *__unused); +int cti_disable(struct coresight_device *csdev, void *__unused);

+#ifdef CONFIG_OF +extern int of_cti_get_hw_data(struct device *dev,

•       struct device_node *np,
  

•        struct cti_drvdata *drvdata);
  

+extern struct coresight_platform_data * +of_get_coresight_cti_platform_data(struct device *dev,

• 		   const struct device_node *node);
  

+#else +static inline int of_cti_get_hw_data(struct device *dev,

•       struct device_node *np,
  

•        struct cti_drvdata *drvdata)
  

+{ return 0; }; +static inline struct coresight_platform_data * +of_get_coresight_cti_platform_data(struct device *dev,

• 	   const struct device_node *node) { return NULL; }
  

+#endif

+#endif /* _CORESIGHT_CORESIGHT_CTI_H */ diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index e0684d06e9ee..8e9e5b6ad866 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -161,6 +161,13 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; } static inline int etm_writel_cp14(u32 off, u32 val) { return 0; } #endif +#ifdef CONFIG_CORESIGHT_CTI +extern void cti_device_release(struct device *dev); +#else +static inline void cti_device_release(struct device *dev) {}; +#endif

/*

• Macros and inline functions to handle CoreSight UCI data and driver
• private data in AMBA ID table entries, and extract data values.

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 29cef898afba..8db3d1a5314f 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -967,12 +967,20 @@ static struct device_type coresight_dev_type[] = { { .name = "helper", },

• {

• .name = "ect",
  

• }, };

static void coresight_device_release(struct device *dev) { struct coresight_device *csdev = to_coresight_device(dev);

• /* additional info to release for CTI */
• if ((csdev->type == CORESIGHT_DEV_TYPE_ECT) &&

•    (csdev->subtype.ect_subtype == CORESIGHT_DEV_SUBTYPE_ECT_CTI))
  

• cti_device_release(dev);
  

I would prefer, you add a new csdev->release hook instead of hard coding it like this and then do :

if (csdev->release) csdev->release(csdev);

kfree(csdev->conns); kfree(csdev->refcnt); kfree(csdev); diff --git a/drivers/hwtracing/coresight/of_coresight-cti.c b/drivers/hwtracing/coresight/of_coresight-cti.c new file mode 100644 index 000000000000..379ca1113deb --- /dev/null +++ b/drivers/hwtracing/coresight/of_coresight-cti.c @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2019, The Linux Foundation. All rights reserved.
• */

+#include <linux/amba/bus.h> +#include <linux/clk.h> +#include <linux/coresight.h> +#include <linux/cpumask.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_graph.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/types.h>

+#include "coresight-cti.h"

+#ifdef CONFIG_OF

+/*

• • CTI can be bound to a CPU, or a system device.
• • Reflect this in the return value and do not default to cpu 0
• */

+static int of_cti_get_cpu(const struct device_node *node) +{

• int cpu;
• struct device_node *dn;
• dn = of_parse_phandle(node, "cpu", 0);
• /* CTI Affinity defaults to no cpu */
• if (!dn)

• return -1;
  

• cpu = of_cpu_node_to_id(dn);
• of_node_put(dn);
• /* No Affinity if no cpu nodes are found */
• return (cpu < 0) ? -1 : cpu;

I think it is time to make the of_coresigth_get_cpu() return the real result than defaulting it to 0. The callers can decide what they want to do about it. And thus you may be able to reuse it here.

+}

+/* get the hardware configuration & connection data. */ +int of_cti_get_hw_data(struct device *dev,

•       struct device_node *np,
  

•       struct cti_drvdata *drvdata)
  

+{

• int rc = 0;
• struct cti_device *cti_dev = &drvdata->ctidev;
• /* if no connections, just add a single default based on max IN-OUT */
• if (cti_dev->nr_trig_con == 0)

• rc = cti_add_default_connection(drvdata);
  

• return rc;

+}

This looks out of place to me, but may be valid in the following patches.

Suzuki