Re: [RFC PATCH v4 01/15] coresight: cti: Initial CoreSight CTI Driver

Hi Mike

On 08/19/2019 10:13 PM, 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

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

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

+#include <linux/of.h>

+#include "coresight-cti.h"

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

•        struct device_node *np,
  

•        struct cti_drvdata *drvdata)
  

nit: The parameter np seems superfluous. You don't use it now. Even if you wanted to use it, you could grab it from the dev already.

+{

• 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(dev, drvdata);
  

• return rc;

+}

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c new file mode 100644 index 000000000000..a832b8c6b866 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c

+/* basic attributes */ +static ssize_t enable_show(struct device *dev,

• 	   struct device_attribute *attr,
  

• 	   char *buf)
  

+{

• int enable_req;
• 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;
• enabled = drvdata->config.hw_enabled;
• spin_unlock(&drvdata->spinlock);
• if (powered) {

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

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

• } else {

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

• 		 enable_req ? "enable req" : "disabled");
  

• }
• return size;

+}

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c new file mode 100644 index 000000000000..b56d9a02c4b4 --- /dev/null +++ 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 irrespective of CPU bound or
• • otherwise.
• • We assume that the non-CPU CTIs are always powered as we do with sinks etc.
• • 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.
• */

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

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

+#define csdev_to_cti_drvdata(csdev) \

• dev_get_drvdata(csdev->dev.parent)

+/* 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 &&

•    (atomic_dec_return(&drvdata->config.enable_req_count) > 0))
  

• goto cti_not_disabled;
  

If you really need to do something like this, you could refactor the function as follows :

/* Called with drvdata->spinlock held */

static int __cti_disable_hw(drvdata) {

• /* no need to do anything if disabled or cpu unpowered */
• if (!config->hw_enabled || !config->hw_powered)

• goto cti_not_disabled;
  

• 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;

}

And make the decision of calling this from cti_disable_hw() based on refcount. And where you may not want to check the refcount call the __cti_disable_hw() directly.

• /* not disabled this call */

+cti_not_disabled:

• spin_unlock(&drvdata->spinlock);
• return 0;

+}

+/*

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

+int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata,

• 	     struct cti_trig_con *tc,
  

• 	     struct coresight_device *csdev,
  

• 	     const char *assoc_dev_name)
  

+{

• struct cti_device *cti_dev = &drvdata->ctidev;
• 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 = devm_kstrdup(dev,
  

• 				dev_name(&csdev->dev),
  

• 				GFP_KERNEL);
  

• else if (assoc_dev_name != NULL)

• tc->con_dev_name = devm_kstrdup(dev,
  

• 				assoc_dev_name, GFP_KERNEL);
  

• 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 |= tc->con_in->used_mask;
• drvdata->config.trig_out_use |= tc->con_out->used_mask;
• return 0;

+}

+/* create a trigger connection with appropriately sized signal groups */ +struct cti_trig_con *cti_allocate_trig_con(struct device *dev, int in_sigs,

• 			   int out_sigs)
  

+{

• struct cti_trig_con *tc = NULL;
• struct cti_trig_grp *in = NULL, *out = NULL;
• tc = devm_kzalloc(dev, sizeof(struct cti_trig_con), GFP_KERNEL);
• if (!tc)

• return tc;
  

• in = devm_kzalloc(dev,

• 	  offsetof(struct cti_trig_grp, sig_types[in_sigs]),
  

• 	  GFP_KERNEL);
  

• if (!in)

• return NULL;
  

• out = devm_kzalloc(dev,

• 	   offsetof(struct cti_trig_grp, sig_types[out_sigs]),
  

• 	   GFP_KERNEL);
  

• if (!out)

• return NULL;
  

• tc->con_in = in;
• tc->con_out = out;
• tc->con_in->nr_sigs = in_sigs;
• tc->con_out->nr_sigs = out_sigs;
• return tc;

+}

+/*

• • 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 device *dev, struct cti_drvdata *drvdata) +{

• int ret = 0;
• int n_trigs = drvdata->config.nr_trig_max;
• u32 n_trig_mask = GENMASK(n_trigs - 1, 0);
• struct cti_trig_con *tc = NULL;
• /*

• * Assume max trigs for in and out,
  

• * all used, default sig types allocated
  

• */
  

• tc = cti_allocate_trig_con(dev, n_trigs, n_trigs);
• if (!tc)

• return -ENOMEM;
  

• tc->con_in->used_mask = n_trig_mask;
• tc->con_out->used_mask = n_trig_mask;
• ret = cti_add_connection_entry(dev, drvdata, tc, NULL, "default");
• return ret;

+}

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

• 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) +{

• 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,

+};

+/*

• • Free up CTI specific resources
• • called by dev->release, need to call down to underlying csdev release.
• */

+static void cti_device_release(struct device *dev) +{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_drvdata *ect_item, *ect_tmp;
• mutex_lock(&ect_mutex);
• /* remove from the list */
• list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, node) {

• if (ect_item == drvdata) {
  

• 	list_del(&ect_item->node);
  

• 	goto ect_list_item_removed;
  

nit: s/goto ../break ?

• }
  

• }

+ect_list_item_removed:

• mutex_unlock(&ect_mu tex);
• if (drvdata->csdev_release)

• drvdata->csdev_release(dev);
  

+}

Rest looks good to me.

Cheers Suzuki