Re: [PATCH v10 2/7] qcom-tgu: Add TGU driver

[Songwei Chai]

Hi Konrad,

Sorry for the late reply.

On 1/13/2026 6:33 PM, Konrad Dybcio wrote:
> On 1/9/26 3:11 AM, Songwei Chai wrote:
> > Add driver to support 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 <songwei.chai@xxxxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +static void tgu_disable(struct device *dev)
> > +{
> > +	struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
> > +
> > +	guard(spinlock)(&drvdata->lock);
> > +	if (drvdata->enable) {
>
> if (!drvdata->enabled)
> 	return
Marked, will update.
> > +		TGU_UNLOCK(drvdata->base);
> > +		writel(0, drvdata->base + TGU_CONTROL);
> > +		TGU_LOCK(drvdata->base);
> > +
> > +		drvdata->enable = false;
> > +	}
> > +}
> > +
> > +static ssize_t enable_tgu_show(struct device *dev,
>
> 'enabled', in all references to this, please
sure.
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
> > +	bool enabled;
> > +
> > +	guard(spinlock)(&drvdata->lock);
> > +	enabled = drvdata->enable;
> > +
> > +	return sysfs_emit(buf, "%d\n", enabled ? 1 : 0);
>
> !!enabled
Marked, will update.
> > +}
> > +
> > +/* 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)
> > +{
> > +	unsigned long val;
> > +	int ret = 0;
>
> Drop the initialization, you override it immediately a line below
Sure.
> [...]
>
> > +static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
> > +{
> > +	struct device *dev = &adev->dev;
> > +	struct tgu_drvdata *drvdata;
> > +	int ret;
> > +
> > +	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 (IS_ERR(drvdata->base))
> > +		return PTR_ERR(drvdata->base);
> > +
> > +	spin_lock_init(&drvdata->lock);
> > +
> > +	ret = sysfs_create_groups(&dev->kobj, tgu_attr_groups);
> > +	if (ret) {
> > +		dev_err(dev, "failed to create sysfs groups: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	drvdata->enable = false;
> > +
> > +	pm_runtime_put(&adev->dev);
> > +	return 0;
>
> Please keep a consistent \n above return as you did in all other places
> throughout this file
Sure.
> [...]
>
> > +/* Register addresses */
> > +#define TGU_CONTROL 0x0000
> > +#define TGU_LAR		0xfb0
> > +#define TGU_UNLOCK_OFFSET	0xc5acce55
>
> Please align the values (it's easier with tabs)
Sure.
> > +
> > +static inline void TGU_LOCK(void __iomem *addr)
> > +{
> > +	do {
> > +		/* Wait for things to settle */
> > +		mb();
>
> What are we waiting for here?
>
> > +		writel_relaxed(0x0, addr + TGU_LAR);
>
> If you do a prompt TGU_LOCK()-TGU_UNLOCK() the writes may arrive in
> the order opposite to what you want, I'd say this shouldn't be _relaxed()
> and we should probably have a readback here to make sure the effect has
> taken place immediately
>
> > +	} while (0);
> > +}
> > +
> > +static inline void TGU_UNLOCK(void __iomem *addr)
> > +{
> > +	do {
> > +		writel_relaxed(TGU_UNLOCK_OFFSET, addr + TGU_LAR);
> > +		/* Make sure everyone has seen this */
> > +		mb();
>
> I believe this should be a readback instead
>
> > +	} while (0);
> > +}
This lock/unlock sequence is intentionally modelled after the existing 
CoreSight CS_LOCK/CS_UNLOCK helpers, which have been in mainline for a
long time and are widely used on ARM systems.

The barriers here are meant to provide CPU-side ordering guarantees
around the LAR access rather than to wait for the hardware lock/unlock
to complete. In particular, the intent is to prevent configuration
accesses from being reordered across the lock/unlock boundary, matching
the CoreSight programming model.

I agree that the comments may be misleading in that regard, and I can
update them to clarify the ordering intent.

If you still prefer a stricter write + readback sequence here, I’m also
happy to switch to that for additional conservatism.
> > +
> > +/**
> > + * 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
> > + * @lock: Spinlock for handling concurrent access
>
> Concurrent access to what? (presumably the TGU?)
 Concurrent access to private data, such as drvdata->enabled here.
I will update the comment to avoid the confusion.
> > + * @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
> > + * trigger-related parameters, and enable status.
> > + */
> > +struct tgu_drvdata {
> > +	void __iomem *base;
> > +	struct device *dev;
> > +	spinlock_t lock;
> > +	bool enable;
>
> "enabled", please
sure.
> Konrad