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

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

On 11/5/2026 10:32 PM, Suzuki K Poulose wrote:
>
>
> 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 ?

yes, correct.


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

It can be thought of as a flattened three-dimensional array, with the 
three dimensions representing which priority, which step, and which 
register, respectively.

Hope this helps clarify the layout and the indexing logic.

>
> ...
>
>>>> +
>>>> +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"
In a priority,

The comment will be updated to clarify this.

>
>>>> + * @num_step: Maximum step size
>
> Number of steps ? not step size ?
It should be "Number of steps implemented", will correct it.
>
> 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
>>>
>>
>