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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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)

[...]

> +	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

> +		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?

> +
> +	if (!counter)

stray \n

> +		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()

[...]

>  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

Konrad