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

Hi Leo,

On Fri, 24 May 2019 at 09:44, Leo Yan leo.yan@linaro.org wrote:

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?

This flag can be altered in the cpu hotplug callbacks in a later patch.

...

• /* 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?

Will do.

...

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

A spinlock is set in the cti_enable_hw() call, which does nothing if the hw is already enabled. While I was testing I was getting warnings from the kernel if I manipulate the atomic inside the spinlock - but I think I have to revisit this flow to re-test this.

...

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

CTI is reference counted as it can be associated with more than one coresight device. e.g. in Juno r1/2 CTI0 takes triggers from the ETR, TPIU, STM and ETF0. So as each device in the path is enabled, then the count is incremented, and then decremented as each device is disabled. Thus the code is designed to enable the CTI when the first device requests enabling, and disable after the last device is disabled. Thus if the count is non zero then this is not an error condition - so we don't want to propagate an error code.

...

+}

+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.

correct - at this point a minimal CTI device is set up based on the hardware registers probed. Later we develop connection associations.

...

• /* 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'?

yes - for this patch - and re-introduce where it is used.

...

• 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

Thanks for the feedback,

Mike