Re: [PATCH v16 3/7] qcom-tgu: Add signal priority support

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 05/11/2026 14:14, Suzuki K Poulose wrote:
> 
> On 05/11/2026 13:01, Suzuki K Poulose wrote:
>> On 30/07/2026 09:07, Songwei Chai wrote:
>>> Like circuit of a Logic analyzer, in TGU, the requirement could be
>>> configured in each step and the trigger will be created once the
>>> requirements are met. Add priority functionality here to sort the
>>> signals into different priorities. The signal which is wanted could
>>> be configured in each step's priority node, the larger number means
>>> the higher priority and the signal with higher priority will be sensed
>>> more preferentially.
>>>
>>> Reviewed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>> Signed-off-by: Songwei Chai <songwei.chai@oss.qualcomm.com>
>>> ---
>>>   .../ABI/testing/sysfs-bus-amba-devices-tgu    |   7 +
>>>   drivers/hwtracing/qcom/tgu.c                  | 189 +++++++++++++++++-
>>>   drivers/hwtracing/qcom/tgu.h                  | 133 ++++++++++++
>>>   3 files changed, 328 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu b/ 
>>> Documentation/ABI/testing/sysfs-bus-amba-devices-tgu
>>> index 36ab64f1e301..7dd24314592d 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu
>>> +++ b/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu
>>> @@ -7,3 +7,10 @@ Description:
>>>           Accepts only one of the 2 values -  0 or 1.
>>>           0 : disable TGU.
>>>           1 : enable TGU.
>>> +
>>> +What:        /sys/bus/amba/devices/<tgu-name>/ 
>>> step[0:7]_priority[0:3]/reg[0:17]
>>> +Date:        August 2026
>>> +KernelVersion:    7.2
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Songwei Chai 
>>> <songwei.chai@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Set/Get the sensed signal with specific step and 
>>> priority for TGU.
>>> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
>>> index 9c7dd4a09f02..0e88b437f191 100644
>>> --- a/drivers/hwtracing/qcom/tgu.c
>>> +++ b/drivers/hwtracing/qcom/tgu.c
>>> @@ -15,14 +15,144 @@
>>>   #include "tgu.h"
>>> +static int calculate_array_location(struct tgu_drvdata *drvdata,
>>> +                    int step_index, int operation_index,
>>> +                    int reg_index)
>>> +{

>>> +    return operation_index * (drvdata->num_step) * (drvdata->num_reg) +
>>> +        step_index * (drvdata->num_reg) + reg_index;

I am confused by this, probably amplified by the bad naming of num_step,
num_reg ?

I perceive num_step => numbe of steps in the hardware

num_reg => number of registers (or controls) in a given priority field ?

If that is the case, shouldn't the index be :

step_index *  (drvdata->num_step) * (drvdata->num_reg) +
op_index * (drvdata->num_reg) + reg_index ?

...

>>> +
>>> +struct value_table {
>>> +    unsigned int *priority;
>>> +};
>>>   static inline void TGU_LOCK(void __iomem *addr)
>>>   {
>>> @@ -36,6 +163,9 @@ static inline void TGU_UNLOCK(void __iomem *addr)
>>>    * @lock: Mutex serialising enable/disable and the associated PM 
>>> runtime
>>>    *        reference counting against concurrent sysfs access
>>>    * @enabled: Flag indicating whether the TGU device is enabled
>>> + * @value_table: Store given value based on relevant parameters
>>> + * @num_reg: Maximum number of registers

Where ? in a priority ? in a step ? Why is this maximum ? Isn't it the
number of "registers implemented on this hardware"

>>> + * @num_step: Maximum step size

Number of steps ? not step size ?

Suzuki
>>>    *
>>>    * This structure defines the data associated with a TGU device,
>>>    * including its base address, device pointer, the lock serialising
>>> @@ -46,6 +176,9 @@ struct tgu_drvdata {
>>>       struct device *dev;
>>>       struct mutex lock;
>>>       bool enabled;
>>> +    struct value_table *value_table;
>>
>> Why this ? why not
>>
>>      struct value_table value_table ?
>>
>> Surely, we seem to be dynamically allocating the priorities etc from 
>> there. But do we need dynamic allocation for value_table field as
>> such ?
>>
>> Suzuki
>>
>>> +    int num_reg;
>>> +    int num_step;
>>>   };
>>>   #endif
>>
>