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

[Songwei Chai]

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@xxxxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +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