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

[songchai]

On 3/7/2025 12:57 AM, Mike Leach wrote:
> Hi,
>
> On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai@xxxxxxxxxxx> wrote:
> > From: Songwei Chai <quic_songchai@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: songchai <quic_songchai@xxxxxxxxxxx>
> > ---
> >   .../testing/sysfs-bus-coresight-devices-tgu   |   9 +
> >   drivers/hwtracing/coresight/Kconfig           |  11 +
> >   drivers/hwtracing/coresight/Makefile          |   1 +
> >   drivers/hwtracing/coresight/coresight-tgu.c   | 218 ++++++++++++++++++
> >   drivers/hwtracing/coresight/coresight-tgu.h   |  36 +++
> >   5 files changed, 275 insertions(+)
> >   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
> >   create mode 100644 drivers/hwtracing/coresight/coresight-tgu.c
> >   create mode 100644 drivers/hwtracing/coresight/coresight-tgu.h
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
> > new file mode 100644
> > index 000000000000..741bc9fd9df5
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
> > @@ -0,0 +1,9 @@
> > +What:          /sys/bus/coresight/devices/<tgu-name>/enable_tgu
> > +Date:          February 2025
> > +KernelVersion  6.15
> > +Contact:       Jinlong Mao (QUIC) <quic_jinlmao@xxxxxxxxxxx>, Sam Chai (QUIC) <quic_songchai@xxxxxxxxxxx>
> > +Description:
> > +               (RW) Set/Get the enable/disable status of TGU
> > +               Accepts only one of the 2 values -  0 or 1.
> > +               0 : disable TGU.
> > +               1 : enable TGU.
> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > index 06f0a7594169..3fe59c745dd4 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -247,4 +247,15 @@ config CORESIGHT_DUMMY
> >
> >            To compile this driver as a module, choose M here: the module will be
> >            called coresight-dummy.
> > +
> > +config CORESIGHT_TGU
> > +       tristate "CoreSight Trigger Generation Unit driver"
> > +       help
> > +         This driver provides support for Trigger Generation Unit that is
> > +         used to detect patterns or sequences on a given set of signals.
> > +         TGU is used to monitor a particular bus within a given region to
> > +         detect illegal transaction sequences or slave responses. It is also
> > +         used to monitor a data stream to detect protocol violations and to
> > +         provide a trigger point for centering data around a specific event
> > +         within the trace data buffer.
> >   endif
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index 4ba478211b31..7c2b9e9cf1cd 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -51,3 +51,4 @@ coresight-cti-y := coresight-cti-core.o       coresight-cti-platform.o \
> >                     coresight-cti-sysfs.o
> >   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
> >   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
> > +obj-$(CONFIG_CORESIGHT_TGU) += coresight-tgu.o
> > diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c
> > new file mode 100644
> > index 000000000000..da4c04ac1097
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-tgu.c
> > @@ -0,0 +1,218 @@
> > +// 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>
> > +
> > +#include "coresight-priv.h"
> > +#include "coresight-tgu.h"
> > +
> > +DEFINE_CORESIGHT_DEVLIST(tgu_devs, "tgu");
> > +
> > +static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
> > +{
> > +       CS_UNLOCK(drvdata->base);
> > +       /* Enable TGU to program the triggers */
> > +       tgu_writel(drvdata, 1, TGU_CONTROL);
> > +       CS_LOCK(drvdata->base);
> > +}
> > +
> > +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);
> > +
> > +       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 int tgu_disable(struct coresight_device *csdev, void *data)
> > +{
> > +       struct tgu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       spin_lock(&drvdata->spinlock);
> > +
> > +       if (!drvdata->enable) {
> Could simplify by changing logic here -
> if (enable) { do disable stuff }
>
> and have a single return point

Done.

> > +               spin_unlock(&drvdata->spinlock);
> > +               return 0;
> > +       }
> > +
> > +       CS_UNLOCK(drvdata->base);
> > +       tgu_writel(drvdata, 0, TGU_CONTROL);
> > +       CS_LOCK(drvdata->base);
> > +
> > +       drvdata->enable = false;
> > +       spin_unlock(&drvdata->spinlock);
> > +       return 0;
> > +}
> > +
> > +static ssize_t enable_tgu_show(struct device *dev,
> > +                              struct device_attribute *attr, char *buf)
> > +{
> > +       bool enabled;
> > +
> > +       struct tgu_drvdata *drvdata = dev_get_drvdata(dev->parent);
> > +
> > +       spin_lock(&drvdata->spinlock);
> > +       enabled = drvdata->enable;
> > +       spin_unlock(&drvdata->spinlock);
> > +
> > +       return sprintf(buf, "%d\n", enabled);
> sysfs_emit() should be used here.
Done.
> > +}
> > +
> > +/* enable_tgu_store - Configure Trace and Gating Unit (TGU) triggers. */
> > +static ssize_t enable_tgu_store(struct device *dev,
> > +                               struct device_attribute *attr, const char *buf,
> > +                               size_t size)
> > +{
> > +       int ret = 0;
> > +       unsigned long val;
> > +       struct tgu_drvdata *drvdata = dev_get_drvdata(dev->parent);
> > +
> > +       ret = kstrtoul(buf, 0, &val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (val) {
> > +               ret = pm_runtime_resume_and_get(dev->parent);
> > +               if (ret)
> > +                       return ret;
> > +               ret = tgu_enable(drvdata->csdev, CS_MODE_SYSFS, NULL);
> > +               if (ret)
> > +                       pm_runtime_put(dev->parent);
> > +       } else {
> > +               ret = tgu_disable(drvdata->csdev, NULL);
> > +               if (!ret)
>
> redundant - tgu_disable always returns 0.
Done. Remove - "if(!ret)"
> > +                       pm_runtime_put(dev->parent);
> > +       }
> > +
> > +       if (ret)
> > +               return ret;
> > +       return size;
> > +}
> > +static DEVICE_ATTR_RW(enable_tgu);
> > +
> > +static const struct coresight_ops_helper tgu_helper_ops = {
> > +       .enable = tgu_enable,
> > +       .disable = tgu_disable,
> > +};
> > +
> > +static const struct coresight_ops tgu_ops = {
> > +       .helper_ops = &tgu_helper_ops,
> > +};
> > +
> > +static struct attribute *tgu_common_attrs[] = {
> > +       &dev_attr_enable_tgu.attr,
> > +       NULL,
> > +};
> > +
> > +static const struct attribute_group tgu_common_grp = {
> > +       .attrs = tgu_common_attrs,
> > +       NULL,
> > +};
> > +
> > +static const struct attribute_group *tgu_attr_groups[] = {
> > +       &tgu_common_grp,
> > +       NULL,
> > +};
> > +
> > +static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
> > +{
> > +       int ret = 0;
> > +       struct device *dev = &adev->dev;
> > +       struct coresight_desc desc = { 0 };
> > +       struct coresight_platform_data *pdata;
> > +       struct tgu_drvdata *drvdata;
> > +
> > +       desc.name = coresight_alloc_device_name(&tgu_devs, dev);
> > +       if (!desc.name)
> > +               return -ENOMEM;
> > +
> > +       pdata = coresight_get_platform_data(dev);
> > +       if (IS_ERR(pdata))
> > +               return PTR_ERR(pdata);
> > +
> > +       adev->dev.platform_data = pdata;
> > +
> > +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +       if (!drvdata)
> > +               return -ENOMEM;
> > +
> > +       drvdata->dev = &adev->dev;
> > +       dev_set_drvdata(dev, drvdata);
> > +
> > +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
> > +       if (!drvdata->base)
> > +               return -ENOMEM;
> > +
> > +       spin_lock_init(&drvdata->spinlock);
> > +
> > +       drvdata->enable = false;
> > +       desc.type = CORESIGHT_DEV_TYPE_HELPER;
> > +       desc.pdata = adev->dev.platform_data;
> > +       desc.dev = &adev->dev;
> > +       desc.ops = &tgu_ops;
> > +       desc.groups = tgu_attr_groups;
> > +
> > +       drvdata->csdev = coresight_register(&desc);
> > +       if (IS_ERR(drvdata->csdev)) {
> > +               ret = PTR_ERR(drvdata->csdev);
> > +               goto err;
> > +       }
> > +
> > +       pm_runtime_put(&adev->dev);
> > +       return 0;
> > +err:
> > +       pm_runtime_put(&adev->dev);
> > +       return ret;
> > +}
> > +
> > +static void tgu_remove(struct amba_device *adev)
> > +{
> > +       struct tgu_drvdata *drvdata = dev_get_drvdata(&adev->dev);
> > +
> > +       coresight_unregister(drvdata->csdev);
> > +}
> > +
> > +static const struct amba_id tgu_ids[] = {
> > +       {
> > +               .id = 0x000f0e00,
> > +               .mask = 0x000fffff,
> > +               .data = "TGU",
> > +       },
> > +       { 0, 0, NULL },
> > +};
> > +
> > +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..380686f94130
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-tgu.h
> > @@ -0,0 +1,36 @@
> > +/* 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) device
> > + * @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,
> I don't see any trigger data pointers or limits here. Comment on what
> is there, if more is added later, expand the comment later.

Setting the trigger generated by TGU(such as when to trigger and what 
trigger to generate) is achieved

by configuring the select/condition/timer/counter registers. Therefor, 
it is not necessary to include trigger

data in the struct.

> > + * maximum limits for various 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
> Regards
>
> Mike