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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c142d3bc-5365-4eab-a613-8f2a06ee9788@oss.qualcomm.com>
Date: Tue, 27 Jan 2026 11:02:30 +0800
From: Songwei Chai <songwei.chai@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, andersson@...nel.org,
        alexander.shishkin@...ux.intel.com, mike.leach@...aro.org,
        suzuki.poulose@....com, james.clark@....com, krzk+dt@...nel.org,
        conor+dt@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, coresight@...ts.linaro.org,
        devicetree@...r.kernel.org, gregkh@...uxfoundation.org
Subject: Re: [PATCH v10 6/7] qcom-tgu: Add timer/counter functionality for TGU



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