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

[Songwei Chai <songwei.chai@oss.qualcomm.com>]

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@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +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