Re: [PATCH v5 2/7] coresight: Add coresight TGU driver
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Date: 2025-05-29 11:27:02
Also in:
linux-arm-msm, linux-devicetree, lkml
On Thu, 29 May 2025 16:19:43 +0800 Songwei Chai [off-list ref] wrote:
Add driver to support Coresight device TGU (Trigger Generation Unit). TGU is a Data Engine which can be utilized to sense a plurality of signals and create a trigger into the CTI or generate interrupts to processors. Add probe/enable/disable functions for tgu. Signed-off-by: Songwei Chai <redacted>
Hi Drive by review as I was curious and might as well comment whilst taking a look. Jonathan
quoted hunk ↗ jump to hunk
diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c new file mode 100644 index 000000000000..a1a02602f7b3 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-tgu.c@@ -0,0 +1,213 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include <linux/amba/bus.h> +#include <linux/coresight.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h>
Why? Probably after #include <linux/mod_devicetable.h> for struct amba_id
+ +#include "coresight-priv.h" +#include "coresight-tgu.h"
+
+static int tgu_enable(struct coresight_device *csdev, enum cs_mode mode,
+ void *data)
+{
+ struct tgu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ spin_lock(&drvdata->spinlock);Maybe consider use of cleanup.h and guard(spin_lock)(&drvdata->spinlock); if (drvdata->enable) return -EBUSY; tgu_write_all_hw_regs(drvdata); drvdata->enable = true; return 0;
+
+ if (drvdata->enable) {
+ spin_unlock(&drvdata->spinlock);
+ return -EBUSY;
+ }
+ tgu_write_all_hw_regs(drvdata);
+ drvdata->enable = true;
+
+ spin_unlock(&drvdata->spinlock);
+ return 0;
+}+static ssize_t enable_tgu_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ bool enabled;I'd drop this blank line for consistency with other bits of code in this file.
+ + struct tgu_drvdata *drvdata = dev_get_drvdata(dev->parent); + + spin_lock(&drvdata->spinlock); + enabled = drvdata->enable; + spin_unlock(&drvdata->spinlock); + + return sysfs_emit(buf, "%d\n", enabled); +}
+static const struct attribute_group tgu_common_grp = {
+ .attrs = tgu_common_attrs,
+ { NULL }, { }
is a common simpler syntax for the same thing.+};
+
+static const struct attribute_group *tgu_attr_groups[] = {
+ &tgu_common_grp,
+ NULL,Common to not put , after terminating entries. The aim is to make it hard to put things incorrectly after these.
+};
+static void tgu_remove(struct amba_device *adev)
+{
+ struct tgu_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+ coresight_unregister(drvdata->csdev);Would probably benefit from a devm_ version allowing the dropping of the remove() call in this and other drivers. More of a general comment than one specific to this driver though.
+}
+
+static const struct amba_id tgu_ids[] = {
+ {
+ .id = 0x000f0e00,
+ .mask = 0x000fffff,
+ .data = "TGU",
+ },
+ { 0, 0, NULL },{ }
Is effective the same an common form for code setting the sentinel
at the end of an array like this.
quoted hunk ↗ jump to hunk
+}; + +MODULE_DEVICE_TABLE(amba, tgu_ids); + +static struct amba_driver tgu_driver = { + .drv = { + .name = "coresight-tgu", + .suppress_bind_attrs = true, + }, + .probe = tgu_probe, + .remove = tgu_remove, + .id_table = tgu_ids, +}; + +module_amba_driver(tgu_driver); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("CoreSight TGU driver");diff --git a/drivers/hwtracing/coresight/coresight-tgu.h b/drivers/hwtracing/coresight/coresight-tgu.h new file mode 100644 index 000000000000..6c849a2f78fa --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-tgu.h@@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef _CORESIGHT_TGU_H +#define _CORESIGHT_TGU_H + +/* Register addresses */ +#define TGU_CONTROL 0x0000 + +/* Register read/write */ +#define tgu_writel(drvdata, val, off) __raw_writel((val), drvdata->base + off) +#define tgu_readl(drvdata, off) __raw_readl(drvdata->base + off) + +/** + * struct tgu_drvdata - Data structure for a TGU (Trigger Generator Unit) + * @base: Memory-mapped base address of the TGU device + * @dev: Pointer to the associated device structure + * @csdev: Pointer to the associated coresight device + * @spinlock: Spinlock for handling concurrent access + * @enable: Flag indicating whether the TGU device is enabled + * + * This structure defines the data associated with a TGU device, + * including its base address, device pointers, clock, spinlock for + * synchronization, trigger data pointers, maximum limits for various
Not seeing any limits, trigger pointers etc...
+ * trigger-related parameters, and enable status.
+ */
+struct tgu_drvdata {
+ void __iomem *base;
+ struct device *dev;
+ struct coresight_device *csdev;
+ spinlock_t spinlock;
+ bool enable;
+};
+
+#endif