Hi Mike,
On Mon, Aug 19, 2019 at 10:13:47PM +0100, Mike Leach wrote:
Since this is big patch, trimed the code significantly when review; hope this will not cause difficulty for reading my comments.
When I applied this patch, git complained as below:
$ git am /tmp/cs_cti_patch_v4/01.patch Applying: coresight: cti: Initial CoreSight CTI Driver .git/rebase-apply/patch:48: new blank line at EOF. + warning: 1 line adds whitespace errors.
[...]
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.
Here should be "Linaro Limited"?
[...]
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 @@ -0,0 +1,451 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (c) 2019 Linaro Limited, All rights reserved.
- Author: Mike Leach mike.leach@linaro.org
- */
+#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)
+/*
- 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 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);
Just curious, usually for switching on/off for module, it's more safe to use writel() rather than writel_relax(), since writel() has a memory barrier prior to access to CTICONTROL register, thus this can give chance to write other configurations to the endpoint before really enable/disable the module.
If you think writel_relaxed() is safe enough, sorry for noise.
- CS_LOCK(drvdata->base);
+}
[...]
+/* disable hardware */ +static int cti_disable_hw(struct cti_drvdata *drvdata, bool update_refcount)
I read the comments in the patch set v3, Suzuki has mentioned "update_refcount" is not really used. In this patch set, "update_refcount" is only set as 'true' when call cti_disable_hw().
Just bring up if you are missing this or you deliberately keep this parameter and will be used later (e.g. for PM notifier?).
+{
- 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;- /* 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;
- /* not disabled this call */
+cti_not_disabled:
- spin_unlock(&drvdata->spinlock);
- return 0;
+}
[...]
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 6453c67a4d01..64ff810394f9 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,7 @@ static void coresight_device_release(struct device *dev) struct coresight_device *csdev = to_coresight_device(dev); fwnode_handle_put(csdev->dev.fwnode);
Redundant new line?
kfree(csdev->refcnt); kfree(csdev); }
[...]
Thanks, Leo Yan