Re: [PATCH v5 2/7] coresight: Add coresight TGU driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 29 May 2025 16:19:43 +0800
Songwei Chai <quic_songchai@quicinc.com> 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 <quic_songchai@quicinc.com>
Hi

Drive by review as I was curious and might as well comment whilst
taking a look.

Jonathan

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

> +};
> +
> +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
> 
>