Hi Suzuki,
On Tue, 11 Jun 2019 at 10:31, Suzuki K Poulose suzuki.poulose@arm.com wrote:
On 05/06/2019 20:02, Mike Leach wrote:
This introduces a baseline CTI driver and associated configuration files.
Uses the platform agnostic naming standard for CoreSight devices, along with a generic platform probing method that currently supports device tree descriptions, but allows for the ACPI bindings to be added once these have been defined for the CTI devices.
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 + .../coresight/coresight-cti-platform.c | 100 ++++ .../hwtracing/coresight/coresight-cti-sysfs.c | 82 +++ drivers/hwtracing/coresight/coresight-cti.c | 499 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.h | 211 ++++++++ drivers/hwtracing/coresight/coresight-priv.h | 6 + drivers/hwtracing/coresight/coresight.c | 9 + include/linux/coresight.h | 26 + 9 files changed, 950 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-cti-platform.c 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
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 18e8d03321d6..49b894241520 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -107,4 +107,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_SINKSDoes it need this dependency, given that CTI could be linked to non CoreSight device and it is perfectly possible to use for them alone without SINKS ?
You are correct - have removed.
+/*
- 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 */minor nit: extra space ^^
return (cpu < 0) ? -1 : cpu;nit: We could simply return cpu ?
This entire function disappears as we are now using a common get CPU fn that covers OF/ ACPI variants.
..
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c new file mode 100644 index 000000000000..deb882a6a367 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -0,0 +1,82 @@ +// 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 "coresight-cti.h"
+/* basic attributes */ +static ssize_t enable_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
int enable_req, cpuid;bool enabled, powered;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;This is confusing. "hw" above means the associated device to the CTI
enabled = drvdata->config.hw_enabled;And here it means the actual CTI hw. Could we rename the above hw_powered to cpu_powered explicitly to avoid the confusion ?
CTI are not always CPU bound - so the labels mean precisely what they say:- hw_powered == CTI hw is powered. hw_enabled == CTI hardware is enabled.
granted a CPU bound CTI will have power dependencies on a cpu, but this is not the same thing.
+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);As mentioned in the previous version, pleas use 0 instead of 16.
OK
diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
+#include "coresight-cti.h"
+/**
- 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
s/pwr/power
- PE power domain.
- At this point we assume that the non-CPU CTIs are always powered as
- we do with sinks etc.
s/At this point we/We/
- 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;+};
You may not need this extra structure, instead you could add a list_head to struct cti_drvdata.
Good point - fixed for next spin
+/*
- CTI naming. CTI bound to cores will have the name cti_cpu<N> where
- N is the CPU ID. System CTIs will have the name cti_sys<I> where I
- is an index allocated by order of discovery.
- CTI device name list - for CTI not bound to cores.
- */
+DEFINE_CORESIGHT_DEVLIST(cti_sys_devs, "cti_sys"); +/* write regs to hardware and enable */ +static int cti_enable_hw(struct cti_drvdata *drvdata, bool update_refcount) +{
struct cti_config *config = &drvdata->config;struct device *dev = &drvdata->csdev->dev;int rc = 0;pm_runtime_get_sync(dev->parent);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_err_not_enabled;if (drvdata->ctidev.cpu >= 0) {rc = smp_call_function_single(drvdata->ctidev.cpu,cti_enable_hw_smp_call,drvdata, 1);if (rc)goto cti_err_not_enabled;} elsecti_write_all_hw_regs(drvdata);config->hw_enabled = true;if (update_refcount)atomic_inc(&drvdata->config.enable_req_count);spin_unlock(&drvdata->spinlock);return rc;/* not enabled in this call */+cti_not_enabled:
That label unfortunately means the opposite of what it could have been. Could we simply make it cti_already_enabled ? That way, we could get rid of the comment above.
changed for next version
if (update_refcount)atomic_inc(&drvdata->config.enable_req_count);/* cannot enable due to error */+cti_err_not_enabled:
spin_unlock(&drvdata->spinlock);pm_runtime_put(dev->parent);return rc;+}
+/* disable hardware */ +static int cti_disable_hw(struct cti_drvdata *drvdata, bool update_refcount) +{
struct cti_config *config = &drvdata->config;struct device *dev = &drvdata->csdev->dev;spin_lock(&drvdata->spinlock);/* check refcount - disable on 0 */ > + if (update_refcount &&I don't like the idea of passing "update_refcount" which could override the refcounts on the device for enable/disable. I think we could refactor the enable/disable operations to __enable() and __disable(). i.e,
enable_hw() { if (atomic_dec_return(refcount) == 0) __enable_hw() }
disable_hw() { if (atomic_dec_return(refcount) == 0) __disable_hw() }
Or simply rename the "update_refcount" to "force" with inverted loging to imply that we want to enable/disable forcefully irrespective of the refcount. Btw, I don't see why we need this bypassing, may be it will be clear in the coming patches.
The CTI driver is power aware - it can be programmed if unpowered - and will assume that programmed state once it is powered. However this means that enable / disable can be called from two contexts:- 1) an enable/disable request context - i.e. sysfs / perf according to whatever trace operation is being prepared. In this case we count the requests as a CTI can be associated with multiple CS devices - each of which will ask for enable / disable CTI as they are enabled in the trace path 2) a power management context - when we want to ensure it is in the programmed state on power up / down. Here we must enable / disable according to the current request count, but must no affect that count.
The request maniipulation is done inside the spinlock of each of these functions, so that there is a single critical section controlling the hardware. This avoids the a race if CTI on CPU3 being hw disabled at the same time as CPU 3 is being powered down. Power management comes later but I want to avoid re-writiing the function logic later in the patchset which would get flagged as an issue.
(atomic_dec_return(&drvdata->config.enable_req_count) > 0))goto cti_not_disabled;/* no need to do anything if disabled or cpu unpowered*/nit: missing space "unpowered*/"
if (!config->hw_enabled || !config->hw_powered)goto cti_not_disabled;This may need to be moved up before dropping the refcount, otherwise we may end up in "negative" refcounts if someone accidentally disables the device more times than it was enabled.
Moving this breaks the programming model - we are allowed to alter the enable request counter, even if currently unpowered or physically disabled. There is a risk I guess of a sysfs user disabling this more than enabling - but largely associated devices will be controlled as part of the path. we could add an explicit atomic_write of 0 on disable to ensure no overrun.
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(dev);return 0;/* not disabled this call */nit: the label is self explanatory, you may drop this comment.
+cti_not_disabled:
spin_unlock(&drvdata->spinlock);return 0;+}
+static void cti_set_default_config(struct device *dev,
struct cti_drvdata *drvdata)+{
struct cti_config *config = &drvdata->config;u32 devid;/** Look at the HW DEVID register for some of the HW settings.* DEVID[15:8] - max number of in / out triggers.*/devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);config->nr_trig_max = (int)((devid & 0xFF00) >> 8);As mentioned in the other thread, please hide it under a macro.
/** no current hardware should exceed this, but protect the driver* in case of fault / out of spec hw*/if (config->nr_trig_max > CTIINOUTEN_MAX) {dev_warn_once(dev,"Limiting HW MaxTrig value(%d) to driver max(%d)\n",config->nr_trig_max, CTIINOUTEN_MAX);config->nr_trig_max = CTIINOUTEN_MAX;}/* DEVID[19:16] - number of CTM channels */config->nr_ctm_channels = (int)((devid & 0xF0000) >> 16);Same here.
Done
/* Most regs default to 0 as zalloc'ed except...*/config->trig_filter_enable = 1;config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0);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)nit: You may as well pass a cti_trig_con* as the argument and let the caller fill in that appropriately as it has to anyway allocate and fill in most of the data.
This is reused in later patches but could probably be reworked.
+{
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);We need to check the result here.
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 = GENMASK(n_trigs - 1, 0);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;}I think we may be able to consolidate the cti_trig_grp struct by emebedding the sig_types into it.
i.e, struct cti_trig_grp { int nr_sigs; ... int sig_types[0]; }; and then :
in = kzalloc(offsetof(struct cti_trig_grp, sig_types[nr_trig], GFP_KERNEL);
That's a useful optimisation.
/* 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");As mentioned above it may be a good idea to pass in the trig_con to the helper. Also, given that we may need to do this for other connections, you may add a helper to allocate a trig_con for a given number of "n_trigs" which will allocate all the bits needed for a trig_con including the trig_groups.
For specified connections n_trigs may and frequently does differ for in an out triggers in range 0 - max_trigs but this still could work.
if (ret)goto conn_add_err;return ret;+conn_add_err:
kfree(in);kfree(out);kfree(out_sig_types);kfree(in_sig_types);And that will cleanu up all of the above individual free ups. May be even add a helper to free a trig_con.
return ret;+}
+/** cti ect operations **/ +int cti_enable(struct coresight_device *csdev, void *__unused) +{
struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);/* enable hardware with refcount */return cti_enable_hw(drvdata, true);+}
+int cti_disable(struct coresight_device *csdev, void *__unused) +{
struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);/* disable hardware with refcount */return cti_disable_hw(drvdata, true);+}
+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,+};
That looks like the normal "helper" ops, we use for CATU. We could see if there is a way to consolidate this under Helper type.
These have changed per Mathieus suggestion - no void * which the CATU help ops require.
+static int cti_probe(struct amba_device *adev, const struct amba_id *id) +{
int ret = 0;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 ect_node *ect_nd = NULL;/* 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;}As discussed ect_nd can be removed.
Yes
/* driver data*/drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);if (!drvdata) {ret = -ENOMEM;dev_info(dev, "%s, mem err\n", __func__);You don't need this message here, you may keep them for debugging and drop it from the final series.
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__);same here.
goto err_out;}drvdata->base = base;/* links between dev and drvdata*/nit: superfluous comment
dev_set_drvdata(dev, drvdata);/* default CTI device info */drvdata->ctidev.cpu = -1;drvdata->ctidev.nr_trig_con = 0;drvdata->ctidev.ctm_id = 0;INIT_LIST_HEAD(&drvdata->ctidev.trig_cons);spin_lock_init(&drvdata->spinlock);/* initialise CTI driver config values */cti_set_default_config(dev, drvdata);/* Parse the .dts for connections and signals */pdata = coresight_cti_get_platform_data(dev);if (IS_ERR(pdata)) {dev_info(dev, "coresight_cti_get_platform_data err\n");ret = PTR_ERR(pdata);goto err_out;}/* default to powered - could change on PM notifications */drvdata->config.hw_powered = true;/* set up device name - will depend if cpu bound or otherwise */if (drvdata->ctidev.cpu >= 0)cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",drvdata->ctidev.cpu);elsecti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);if (!cti_desc.name) {ret = -ENOMEM;goto err_out;}/* 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);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);mutex_unlock(&ect_mutex);/* all done - dec pm refcount */pm_runtime_put(&adev->dev);if (drvdata->ctidev.cpu >= 0) {dev_info(dev, "%s: cti-CPU%d initialized\n", cti_desc.name,drvdata->ctidev.cpu);} else {dev_info(dev, "%s: cti-system initialized\n", cti_desc.name);}The names already give you a clue as to whether it is CPU bound or system component. So, you may unify it :
dev_info(dev, "%s: initialized\n", cti_desc.name);
indeed.
+/* 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);If you unify the allocation/free of a "cti_trig_con" this could look a lot better and cleaner.
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;mutex_lock(&ect_mutex);/* clear the connection list items */cti_free_conn_info(drvdata);/* remove from the list */list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, next) {if (ect_item->cti_drv == drvdata) {list_del(&ect_item->next);goto ect_list_item_removed;}}+ect_list_item_removed:
mutex_unlock(&ect_mutex);+} +EXPORT_SYMBOL_GPL(cti_device_release);
+static struct amba_cs_uci_id uci_id_cti[] = {
{/* CTI UCI data */.devarch = 0x47701a14, /* devarch value for CTI v2 */nit: simply say /* CTI v2 */
.devarch_mask = 0xfff0ffff,.devtype = 0x00000014, /* maj(0x4-debug) min(0x1-ECT) */}+};
+static const struct amba_id cti_ids[] = {
CS_AMBA_ID(0x000bb906), /* Coresight CTI (SoC 400), C-A72, C-A57 */CS_AMBA_ID(0x000bb922), /* CTI - C-A8 */CS_AMBA_ID(0x000bb9a8), /* CTI - C-A53 */CS_AMBA_ID(0x000bb9aa), /* CTI - C-A73 */CS_AMBA_UCI_ID(0x000bb9da, uci_id_cti), /* CTI - C-A35 */CS_AMBA_UCI_ID(0x000bb9ed, uci_id_cti), /* Coresight CTI (SoC 600) */{ 0, 0},+};
+static struct amba_driver cti_driver = {
.drv = {.name = "coresight-cti",.owner = THIS_MODULE,.suppress_bind_attrs = true,},.probe = cti_probe,.id_table = cti_ids,+}; +builtin_amba_driver(cti_driver); diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h new file mode 100644 index 000000000000..bc4c9bdf745e --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -0,0 +1,211 @@ +/* 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 */ +/* 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.
Is this true ? Do we need this at all here ? It doesn't affect the MAX number anyways.
This is the theoretical hardware maximum for CTI CSSoc 600, plus the driver maximum.
- */
+#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
- */
+#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;See above, we could turn this into : int sig_types[0];
+};
+/**
- 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.
- @asicctl: asic control register.
- */
+struct cti_config {
/* hardware description */int nr_ctm_channels;int nr_trig_max;You may leave a new line here, if you want to use a comment like this.
/* cti enable control */atomic_t enable_req_count;bool hw_enabled;bool hw_powered;
/* registered triggers and filtering */u32 trig_in_use;u32 trig_out_use;u32 trig_out_filter;int trig_filter_enable;u8 xtrig_rchan_sel;
/* cti cross trig programmable regs */u32 ctiappset;u8 ctiinout_sel;u32 ctiinen[CTIINOUTEN_MAX];u32 ctiouten[CTIINOUTEN_MAX];u32 ctigate;u32 asicctl;+};
Those lines improves the readability a lot.
+/**
- struct cti_drvdata - specifics for the CTI device
- @base: Memory mapped base address for 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 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); +struct coresight_platform_data * +coresight_cti_get_platform_data(struct device *dev);
+#endif /* _CORESIGHT_CORESIGHT_CTI_H */ diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 8b07fe55395a..dd2991dfbf4c 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -161,6 +161,12 @@ 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 86d1fc2c1bd4..1c9ba2fd6879 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -972,6 +972,9 @@ static struct device_type coresight_dev_type[] = { { .name = "helper", },
{.name = "ect",},};
static void coresight_device_release(struct device *dev)
@@ -979,6 +982,12 @@ static void coresight_device_release(struct device *dev) struct coresight_device *csdev = to_coresight_device(dev);
fwnode_handle_put(csdev->dev.fwnode);
/* 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);As discussed in the other thread, please introduce a "release" method for csdev and use it here. i.e,
if (csdev->release) csdev->release(csdev);instead of hardcoding the ECT check in generic code.
Did think the point of csdev->type was to allow just this - seems to be elsewhere. As this is CTI specific release function, this is now done by hijacking csdev->dev.release, & calling back into here from cti_device_release - which is neater than adding a "common" function to csdev that only CTI needs.
kfree(csdev->refcnt); kfree(csdev);} diff --git a/include/linux/coresight.h b/include/linux/coresight.h index a2b68823717b..0a2a69b5ade8 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -41,6 +41,7 @@ enum coresight_dev_type { CORESIGHT_DEV_TYPE_LINKSINK, CORESIGHT_DEV_TYPE_SOURCE, CORESIGHT_DEV_TYPE_HELPER,
CORESIGHT_DEV_TYPE_ECT,I am not convinced that we need a new type.
ECT is a distinct subsystem from the normal trace data path and can connect to non coresight devices.
};
enum coresight_dev_subtype_sink { @@ -68,6 +69,12 @@ enum coresight_dev_subtype_helper { CORESIGHT_DEV_SUBTYPE_HELPER_CATU, };
+/* Embedded Cross Trigger (ECT) sub-types */ +enum coresight_dev_subtype_ect {
CORESIGHT_DEV_SUBTYPE_ECT_NONE,CORESIGHT_DEV_SUBTYPE_ECT_CTI,+};
- /**
- union coresight_dev_subtype - further characterisation of a type
- @sink_subtype: type of sink this component is, as defined
@@ -78,6 +85,8 @@ enum coresight_dev_subtype_helper {
by @coresight_dev_subtype_source.- @helper_subtype: type of helper this component is, as defined
by @coresight_dev_subtype_helper.
- @ect_subtype: type of cross trigger this component is, as
*/ union coresight_dev_subtype { /* We have some devices which acts as LINK and SINK */
defined by @coresight_dev_subtype_ect@@ -87,6 +96,7 @@ union coresight_dev_subtype { }; enum coresight_dev_subtype_source source_subtype; enum coresight_dev_subtype_helper helper_subtype;
enum coresight_dev_subtype_ect ect_subtype;};
/**
@@ -196,6 +206,7 @@ static struct coresight_dev_list (var) = { \ #define sink_ops(csdev) csdev->ops->sink_ops #define link_ops(csdev) csdev->ops->link_ops #define helper_ops(csdev) csdev->ops->helper_ops +#define ect_ops(csdev) csdev->ops->ect_ops
/**
- struct coresight_ops_sink - basic operations for a sink
@@ -262,11 +273,26 @@ struct coresight_ops_helper { int (*disable)(struct coresight_device *csdev, void *data); };
+/**
- struct coresight_ops_ect - Ops for an embedded cross trigger device
- All operations could pass in a device specific data, which could
- help the ect device to determine what to do.
- @enable : Enable the device
- @disable : Disable the device
- */
+struct coresight_ops_ect {
int (*enable)(struct coresight_device *csdev, void *data);int (*disable)(struct coresight_device *csdev, void *data);+};
These look similar to the "helper" ops. Could we not make the ECT a subtype of helper instead ?
These have changed - no void * as it was not used.
Thanks for the feedback
Mike
Suzuki