Good day Mike,
On Thu, 23 May 2019 at 09:18, Mike Leach mike.leach@linaro.org wrote:
Hi Mathieu,
On Mon, 6 May 2019 at 22:50, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Wed, May 01, 2019 at 09:49:33AM +0100, 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
Compiling this patch gave me the following compilation warning:
/home/mpoirier/work/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-cti-sysfs.c: In function ‘enable_show’: /home/mpoirier/work/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-cti-sysfs.c:52:19: warning: comparison of constant ‘0’ with boolean expression is always true [-Wbool-compare] } else if (cpuid >= 0) {
mpoirier@xps15:~/work/coresight/kernel-maint$ aarch64-linux-gnu-gcc --version aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11) 7.2.1 20171011 Copyright (C) 2017 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
OK - looks like it was fixed in the next patch, will fix here.
Checkpatch.pl also found problems with several patches and I had to rebase your work on the latest coresight-next tree.
The set was base on upstream Linux 5.1-rc5. Passed through checkpatch.pl with no errors on my side. (exception being Kconfig where it complained about lack of decsription even though one present ) I'll recheck next release.
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 || ARM64select CORESIGHT_LINKS_AND_SINKShelpThis driver provides support for CoreSight CTI and CTM components.These provide hardware triggering events between CoreSight tracesource and sink components. These can be used to halt trace ors/"used to halt"/"used to halt"
inject events into the trace stream. CTI also provides a softwarecontrol to trigger same halt events. This can provide fast traces/"to trigger same halt"/"to trigger the same halt"
halt compared to disabling sources and sinks normally in driversoftware.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.oIt is probably a good idea to base your work on Suzuki's ACPI pathset [1] where he got rid of of_coresight.c to favour a firmware agnostic naming convention. While doing so skip patches 34 to 36 as they relate to sysfs topology and we need to deal with that separately. Better yet, include patches 34 to 36 and propose a sysfs topology representation that works for both "data" and "signal" components.
I am aware of Suzukis work and the implications for re-work - but was not keen on developing against a moving target, rather look to more stable upstream - i.e. rc5 as mentioned above. There is nothing in suzukis patch set that this set _needs_ at present. Yes, there is the question of connection representations - the problem is the stark difference between simple links that are dependent on the trace data path connection that suzuki proposes - where the connection information is implied as a trace bus, and the requirement for multiple data entries per CTI connection. i.e. the CTI connection data includes not only the device connected, but details of input signals, output signals and purposes of those signals on a per connected device basis.
My preference is to do the update work once - especially as the latest of Suzukis patch sets drops all the sysfs work.
Though I haven't had a chance to reviewed it yet, I consider Suzuki's ACPI work to be pretty much done and will likely include his V4 in my tree sometime next week. As such it is probably worth rebasing your work on top of that. If anything changes it will be very small.
[1]. https://patchwork.kernel.org/cover/10901109/
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>
As far as I can tell this file can be compile with the following headers:
coresight.h device.h kernel.h spinlock.h sysfs.h types.h
Everything else can be removed and re-introduced as you need them.
Will do.
+#include "coresight-cti.h"
+/* basic attributes */ +static ssize_t enable_show(struct device *dev,
struct device_attribute *attr,char *buf)Once applied the second and third line don't line up with the 's' of the first line.
will fix
+{
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");}This interface seems a little complex to me. I will wait to see how things evolved in the coming patches - maybe I'll understand better then.
The CTI driver maintains a powered state./ enable ref count to know if it is safe to write through any programming - otherwise the internal state is updated but not written to the hardware till powered and enabled.
I had the same dilemma when working on the etmv3/4 drivers. I ended up choosing to write the configuration to the HW the next time it would be possible to do so. For example, if device enabled write new config, if not simply cache the new value and write to HW when possible.
this data here makes it easy to see that state - though it is a little different from the other drivers. At this point handy for testing, but the cpu / powered info could appear elsewhere / in there RO ovn sysfs attributes?
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);if (ret)return ret;if (val)ret = cti_enable(drvdata->csdev, NULL);elseret = cti_disable(drvdata->csdev, NULL);if (ret)return ret;return size;+} +static DEVICE_ATTR_RW(enable);
+/* attribute and group sysfs tables. */ +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 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (c) 2018 Linaro Limited, All rights reserved.
s/2018/2019
- Author: Mike Leach mike.leach@linaro.org
- */
+#include <linux/amba/bus.h> +#include <linux/clk.h> +#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 <asm/local.h> +#include <asm/sections.h>
+#include "coresight-cti.h"
Same comment as above - please get rid of all the headers that aren't currenlty needed.
+/**
- 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
- PE power domain.
- At this point we assume that the none CPU CTIs are always powered as
- we do with sinks etc.
s/none/non
- 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.
- */
While reading the bindings and the documentation included in this set (many thanks for that), I was wondering if you have seen and tested your work on systems where more than one CTM is present.
I believe there are multi-socket server systems that will have separate CTM connections ( and separate trace subsystems for that matter). Not got access to such a system - but driver functional testing is trivial - simply temporarily assign CTIs on a one CTM system to different virtual CTMs. Which I have yet to do.
Since predicting how things will work is guaranteed to fail, I am weary of adding code we don't use or hasn't been tested on a real HW platform.
+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];
+/* 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",drvdata->ctidev.cpu);rc = smp_call_function_single(drvdata->ctidev.cpu,cti_enable_hw_smp_call,drvdata, 1);if (rc)goto cti_not_enabled;} else {dev_info(drvdata->dev, "cti enable not cpu call\n");cti_write_all_hw_regs(drvdata);}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);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;Please use the macro GENMASK().
Will do
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)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;struct cti_trig_grp *in = 0, *out = 0;int n_trigs = drvdata->config.nr_trig_max;u32 n_trig_mask = (0x1 << n_trigs) - 1;GENMASK().
int *in_sig_types = 0, *out_sig_types = 0;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);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);Every time you use hard coded values like this, please introduce a comments that explains what you are doing. For example:
"DEVID.NUM_TRIG [15:8] : Indicates the maximum number of triggers."
That way we don't always have to go back to the TRM to understand the code.
/** 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);/* 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;Out of curiosity, do you expect other ECT device types?
CTM is another ECT device type though currently does not require a driver.
Right right - we've been there with funnel and replicators. It is only a matter of time before CTMs to require a driver.
There are a number of ARM designed CTIs - which are all currently covered by this CTI driver, though we cannot exclude future / partner designed devices needing there own. Finally - this is the correct terminology defined by the ARM TRM - so I feel it is best to use it accurately.
Ok
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);