Re: [PATCH v10 6/7] qcom-tgu: Add timer/counter functionality for TGU

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

On 1/13/2026 7:19 PM, Konrad Dybcio wrote:
> On 1/9/26 3:11 AM, Songwei Chai wrote:
>> Add counter and timer node for each step which could be
>> programed if they are to be utilized in trigger event/sequence.
>>
>> Signed-off-by: Songwei Chai <songwei.chai@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +static void tgu_set_timer_counter(struct tgu_drvdata *drvdata)
>> +{
>> +	int num_timers, num_counters;
>> +	u32 devid2;
>> +
>> +	devid2 = readl(drvdata->base + CORESIGHT_DEVID2);
>> +
>> +	if (TGU_DEVID2_TIMER0(devid2) && TGU_DEVID2_TIMER1(devid2))
>> +		num_timers = 2;
>> +	else if (TGU_DEVID2_TIMER0(devid2) || TGU_DEVID2_TIMER1(devid2))
>> +		num_timers = 1;
>> +	else
>> +		num_timers = 0;
>> +
>> +	if (TGU_DEVID2_COUNTER0(devid2) && TGU_DEVID2_COUNTER1(devid2))
>> +		num_counters = 2;
>> +	else if (TGU_DEVID2_COUNTER0(devid2) || TGU_DEVID2_COUNTER1(devid2))
>> +		num_counters = 1;
>> +	else
>> +		num_counters = 0;
>> +
>> +	drvdata->max_timer = num_timers;
>> +	drvdata->max_counter = num_counters;
> 
> int num_timers = 0, num_counters = 0
> 
> if (TGU_DEVID2_TIMER0(devid2))
> 	num_timers++
> 
> if (TGU_DEVID2_TIMER1(devid2))
> 	num_timers++
> 
> etc.
> 
> unless you want to guard against a case where TIMER0 reports as absent
> and TIMER1 as present and you consider that invalid (I don't know)

Based on the current documentation and the hardware we have encountered
so far, this case - "TIMER1 present, TIMER0 absent" does not occur.

> 
> [...]
> 
>> +	timer_size = drvdata->max_step * drvdata->max_timer *
>> +		     sizeof(*(drvdata->value_table->timer));
>> +
>> +	timer = devm_kzalloc(dev, timer_size, GFP_KERNEL);
>> +
>> +	if (!timer)
> 
> stray \n
sure.
> 
>> +		return -ENOMEM;
>> +
>> +	drvdata->value_table->timer = timer;
>> +
>> +	counter_size = drvdata->max_step * drvdata->max_counter *
>> +		       sizeof(*(drvdata->value_table->counter));
>> +
>> +	counter = devm_kzalloc(dev, counter_size, GFP_KERNEL);
> 
> devm_kcalloc, perhaps?
Agreed. Using devm_kcalloc() makes the intent clearer and safer here
> 
>> +
>> +	if (!counter)
> 
> stray \n
sure.
> 
>> +		return -ENOMEM;
>> +
>> +	drvdata->value_table->counter = counter;
>> +
>>   	drvdata->enable = false;
>>   
>>   	pm_runtime_put(&adev->dev);
>> diff --git a/drivers/hwtracing/qcom/tgu.h b/drivers/hwtracing/qcom/tgu.h
>> index 8c92e88d7e2c..94708750b02d 100644
>> --- a/drivers/hwtracing/qcom/tgu.h
>> +++ b/drivers/hwtracing/qcom/tgu.h
>> @@ -11,11 +11,17 @@
>>   #define TGU_LAR		0xfb0
>>   #define TGU_UNLOCK_OFFSET	0xc5acce55
>>   #define TGU_DEVID	0xfc8
>> +#define CORESIGHT_DEVID2       0xfc0
>>   
>>   #define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
> 
> This is NIH FIELD_GET()
> 
ok, will try to use "FIELD_GET".
> [...]
> 
>>   static inline void TGU_LOCK(void __iomem *addr)
>> @@ -197,6 +247,8 @@ static inline void TGU_UNLOCK(void __iomem *addr)
>>    * @max_step: Maximum step size
>>    * @max_condition_decode: Maximum number of condition_decode
>>    * @max_condition_select: Maximum number of condition_select
>> + * @max_timer: Maximum number of timers
>> + * @max_counter: Maximum number of counters
>>    *
>>    * This structure defines the data associated with a TGU device,
>>    * including its base address, device pointers, clock, spinlock for
>> @@ -213,6 +265,8 @@ struct tgu_drvdata {
>>   	int max_step;
>>   	int max_condition_decode;
>>   	int max_condition_select;
>> +	int max_timer;
>> +	int max_counter;
> 
> num_timers, num_counters definitely fits better here
> 
uhh.. yeah.
> Konrad