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

Hi Mike,

Except Mathieu's comments, below are my some minor comments.

On Wed, May 01, 2019 at 09:49:33AM +0100, Mike Leach wrote:

[...]

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.
• • 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"

+/**

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

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

'config->hw_powered' has been assigned to 'true' in the function cti_probe(), thus the checking at here is redundant?

• /* 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);
  

Maybe we can remove the debugging logs, same as other CoreSight drivers?

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

Seems to me we can increment the counter after enabling hardware successfully, thus we can avoid to decrease it for failed case:

rc = cti_enable_hw(drvdata); if (rc) return rc;

atomic_inc(&drvdata->config.enable_req_count); return 0;

BTW, please consider to use spinlock to protect the flow, e.g. if use sysfs node to enable/disable CTI device concurrently, it might introduce mistaching between the counter and hardware enabling.

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

Same at here, should we return -EBUSY if counter is not zero?

+}

+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);
• /*

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

I don't see any additional parsing dts for connections in this patch, will assume the sequential patches will add related code.

• /* 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++;

Can we remove '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);
  

We can use a bit more neat code as below?

list_del_init(&tc->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); 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) {

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

• 	list_del(&ect_item->next);
  

• 	kfree(ect_item);
  

• 	goto ect_list_item_removed;
  

• }
  

• }

+ect_list_item_removed:

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

+} +EXPORT_SYMBOL_GPL(cti_device_release);

[...]

diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h

[...]

+/**

• • struct cti_config - configuration of the CTI device hardware
• • hardware description from RO ID regs
• • @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.

Please add comment for @asicctl.

• */

+struct cti_config {

• /* hardware description */
• int nr_ctm_channels;
• int nr_trig_max;
• /* 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;
• /* cti cross trig programmable regs */
• u32 ctiappset;
• u8 ctiinout_sel;
• u32 ctiinen[CTIINOUTEN_MAX];
• u32 ctiouten[CTIINOUTEN_MAX];
• u32 ctigate;
• u32 asicctl;

+};

[...]

Thanks, Leo Yan