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

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

On 12/25/2025 11:20 AM, Jie Gan wrote:
> 
> 
> On 12/19/2025 2:58 PM, 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.
>>
>> Signed-off-by: Songwei Chai <songwei.chai@oss.qualcomm.com>
>> ---
>>   .../ABI/testing/sysfs-bus-amba-devices-tgu    |   7 +
>>   drivers/hwtracing/qcom/tgu.c                  | 156 ++++++++++++++++++
>>   drivers/hwtracing/qcom/tgu.h                  | 113 +++++++++++++
>>   3 files changed, 276 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu b/ 
>> Documentation/ABI/testing/sysfs-bus-amba-devices-tgu
>> index 24dcdf1d70cc..d04a01368089 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:        December 2025
>> +KernelVersion    6.19
>> +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 dbd1acbd2fa5..447d7e68d132 100644
>> --- a/drivers/hwtracing/qcom/tgu.c
>> +++ b/drivers/hwtracing/qcom/tgu.c
>> @@ -14,14 +14,121 @@
>>   #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->max_step) * (drvdata->max_reg) +
>> +        step_index * (drvdata->max_reg) + reg_index;
>> +}
>> +
>> +static ssize_t tgu_dataset_show(struct device *dev,
>> +                struct device_attribute *attr, char *buf)
>> +{
>> +    int index;
> 
> Prefer reverse Christmas tree order
Will update.>
>> +    struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
>> +    struct tgu_attribute *tgu_attr =
>> +            container_of(attr, struct tgu_attribute, attr);
>> +
>> +    index = calculate_array_location(drvdata, tgu_attr->step_index,
>> +                     tgu_attr->operation_index,
>> +                     tgu_attr->reg_num);
>> +
>> +    return sysfs_emit(buf, "0x%x\n",
>> +              drvdata->value_table->priority[index]);
>> +}
>> +
>> +static ssize_t tgu_dataset_store(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 const char *buf, size_t size)
>> +{
>> +    int index;
>> +    unsigned long val;
> 
> Prefer reverse Christmas tree order
Ditto.>
>> +
>> +    struct tgu_drvdata *tgu_drvdata = dev_get_drvdata(dev);
>> +    struct tgu_attribute *tgu_attr =
>> +        container_of(attr, struct tgu_attribute, attr);
>> +
>> +    if (kstrtoul(buf, 0, &val))
>> +        return -EINVAL;
>> +
>> +    guard(spinlock)(&tgu_drvdata->lock);
>> +    index = calculate_array_location(tgu_drvdata, tgu_attr->step_index,
>> +                     tgu_attr->operation_index,
>> +                     tgu_attr->reg_num);
>> +
>> +    tgu_drvdata->value_table->priority[index] = val;
>> +    return size;
>> +}
>> +
>> +static umode_t tgu_node_visible(struct kobject *kobject,
>> +                struct attribute *attr,
>> +                int n)
>> +{
>> +    struct device *dev = kobj_to_dev(kobject);
>> +    struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
>> +    int ret = SYSFS_GROUP_INVISIBLE;
>> +
>> +    struct device_attribute *dev_attr =
>> +        container_of(attr, struct device_attribute, attr);
>> +    struct tgu_attribute *tgu_attr =
>> +        container_of(dev_attr, struct tgu_attribute, attr);
>> +
>> +    if (tgu_attr->step_index < drvdata->max_step) {
>> +        ret = (tgu_attr->reg_num < drvdata->max_reg) ?
>> +            attr->mode : 0;
>> +    }
>> +    return ret;
> 
> if ((tgu_attr->step_index < drvdata->max_step) &&
>      (tgu_attr->reg_num < drvdata->max_reg)
>      return attr->mode;
> 
> return 0;
These two approaches are similar, but the original one might better 
align with the directory organization logic in sysfs. At the same time, 
centralizing the return operations in one place could be better.>
>> +}
>> +
>>   static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>>   {
>> +    int i, j, k, index;
>> +
>>       TGU_UNLOCK(drvdata->base);
>> +    for (i = 0; i < drvdata->max_step; i++) {
>> +        for (j = 0; j < MAX_PRIORITY; j++) {
>> +            for (k = 0; k < drvdata->max_reg; k++) {
>> +                index = calculate_array_location(
>> +                            drvdata, i, j, k);
>> +
>> +                writel(drvdata->value_table->priority[index],
>> +                    drvdata->base +
>> +                    PRIORITY_REG_STEP(i, j, k));
>> +            }
>> +        }
>> +    }
>>       /* Enable TGU to program the triggers */
>>       writel(1, drvdata->base + TGU_CONTROL);
>>       TGU_LOCK(drvdata->base);
>>   }
>> +static void tgu_set_reg_number(struct tgu_drvdata *drvdata)
>> +{
>> +    int num_sense_input;
>> +    int num_reg;
>> +    u32 devid;
>> +
>> +    devid = readl(drvdata->base + TGU_DEVID);
>> +
>> +    num_sense_input = TGU_DEVID_SENSE_INPUT(devid);
>> +    if (((num_sense_input * NUMBER_BITS_EACH_SIGNAL) % 
>> LENGTH_REGISTER) == 0)
>> +        num_reg = (num_sense_input * NUMBER_BITS_EACH_SIGNAL) / 
>> LENGTH_REGISTER;
>> +    else
>> +        num_reg = ((num_sense_input * NUMBER_BITS_EACH_SIGNAL) / 
>> LENGTH_REGISTER) + 1;
>> +    drvdata->max_reg = num_reg;
>> +
>> +}
>> +
>> +static void tgu_set_steps(struct tgu_drvdata *drvdata)
>> +{
>> +    u32 devid;
>> +
>> +    devid = readl(drvdata->base + TGU_DEVID);
>> +
>> +    drvdata->max_step = TGU_DEVID_STEPS(devid);
>> +}
>> +
>>   static int tgu_enable(struct device *dev)
>>   {
>>       struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
>> @@ -106,6 +213,38 @@ static const struct attribute_group 
>> tgu_common_grp = {
>>   static const struct attribute_group *tgu_attr_groups[] = {
>>       &tgu_common_grp,
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(0, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(0, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(0, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(0, 3),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(1, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(1, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(1, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(1, 3),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(2, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(2, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(2, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(2, 3),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(3, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(3, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(3, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(3, 3),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(4, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(4, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(4, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(4, 3),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(5, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(5, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(5, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(5, 3),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(6, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(6, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(6, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(6, 3),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(7, 0),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(7, 1),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(7, 2),
>> +    PRIORITY_ATTRIBUTE_GROUP_INIT(7, 3),
>>       NULL,
>>   };
>> @@ -128,12 +267,29 @@ static int tgu_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>       spin_lock_init(&drvdata->lock);
>> +    tgu_set_reg_number(drvdata);
>> +    tgu_set_steps(drvdata);
>> +
>>       ret = sysfs_create_groups(&dev->kobj, tgu_attr_groups);
>>       if (ret) {
>>           dev_err(dev, "failed to create sysfs groups: %d\n", ret);
>>           return ret;
>>       }
>> +    drvdata->value_table =
>> +        devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL);
>> +    if (!drvdata->value_table)
>> +        return -ENOMEM;
>> +
>> +    drvdata->value_table->priority = devm_kzalloc(
>> +        dev,
>> +        MAX_PRIORITY * drvdata->max_reg * drvdata->max_step *
>> +            sizeof(*(drvdata->value_table->priority)),
>> +        GFP_KERNEL);
> 
> can we declare a pri_size for better reading?
> 
> size_t pri_size;
> unsigned int *priority
> 
> pri_size = MAX_PRIORITY * drvdata->max_reg * drvdata->max_step *
>          sizeof(*(drvdata->value_table->priority));
> 
> priority = devm_kzalloc(dev, pri_size, GFP_KERNEL)
> if (!priority)
>      return -ENOMEM;
> 
> drvdata->value_table->priority = priority;
Will try.>
>> +
>> +    if (!drvdata->value_table->priority)
>> +        return -ENOMEM;
>> +
>>       drvdata->enable = false;
>>       pm_runtime_put(&adev->dev);
>> diff --git a/drivers/hwtracing/qcom/tgu.h b/drivers/hwtracing/qcom/tgu.h
>> index abc732f91dfc..2cf95c239ee7 100644
>> --- a/drivers/hwtracing/qcom/tgu.h
>> +++ b/drivers/hwtracing/qcom/tgu.h
>> @@ -10,6 +10,113 @@
>>   #define TGU_CONTROL 0x0000
>>   #define TGU_LAR        0xfb0
>>   #define TGU_UNLOCK_OFFSET    0xc5acce55
>> +#define TGU_DEVID    0xfc8
>> +
>> +#define BMVAL(val, lsb, msb)    ((val & GENMASK(msb, lsb)) >> lsb)
>> +#define TGU_DEVID_SENSE_INPUT(devid_val) ((int) BMVAL(devid_val, 10, 
>> 17))
>> +#define TGU_DEVID_STEPS(devid_val) ((int)BMVAL(devid_val, 3, 6))
>> +#define NUMBER_BITS_EACH_SIGNAL 4
>> +#define LENGTH_REGISTER 32
>> +
>> +/*
>> + *  TGU configuration space                              Step 
>> configuration
>> + *  offset table                                         space layout
>> + * x-------------------------x$                          
>> x-------------x$
>> + * |                         |$                          | 
>>              |$
>> + * |                         |                           |   
>> reserve   |$
>> + * |                         |                           | 
>>              |$
>> + * |coresight management     |                           
>> |-------------|base+n*0x1D8+0x1F4$
>> + * |     registe             |                     |---> | 
>> prioroty[3]  |$
>> + * |                         |                     |     
>> |-------------|base+n*0x1D8+0x194$
>> + * |                         |                     |     | 
>> prioroty[2]  |$
>> + * |-------------------------|                     |     
>> |-------------|base+n*0x1D8+0x134$
>> + * |                         |                     |     | 
>> prioroty[1]  |$
>> + * |         step[7]         |                     |     
>> |-------------|base+n*0x1D8+0xD4$
>> + * |-------------------------|->base+0x40+7*0x1D8  |     | 
>> prioroty[0]  |$
>> + * |                         |                     |     
>> |-------------|base+n*0x1D8+0x74$
>> + * |         ...             |                     |     |  
>> condition  |$
>> + * |                         |                     |     |   
>> select    |$
>> + * |-------------------------|->base+0x40+1*0x1D8  |     
>> |-------------|base+n*0x1D8+0x60$
>> + * |                         |                     |     |  
>> condition  |$
>> + * |         step[0]         |-------------------->      |   
>> decode    |$
>> + * |-------------------------|-> base+0x40               
>> |-------------|base+n*0x1D8+0x50$
>> + * |                         |                           | 
>>              |$
>> + * | Control and status space|                           |Timer/ 
>> Counter|$
>> + * |        space            |                           | 
>>              |$
>> + * x-------------------------x->base                     
>> x-------------x base+n*0x1D8+0x40$
>> + *
>> + */
>> +#define STEP_OFFSET 0x1D8
>> +#define PRIORITY_START_OFFSET 0x0074
>> +#define PRIORITY_OFFSET 0x60
>> +#define REG_OFFSET 0x4
>> +
>> +/* Calculate compare step addresses */
>> +#define PRIORITY_REG_STEP(step, priority, reg)\
>> +    (PRIORITY_START_OFFSET + PRIORITY_OFFSET * priority +\
>> +     REG_OFFSET * reg + STEP_OFFSET * step)
>> +
>> +#define tgu_dataset_rw(name, step_index, type, 
>> reg_num)                  \
>> +    (&((struct tgu_attribute[]){ {                                   \
>> +        __ATTR(name, 0644, tgu_dataset_show, tgu_dataset_store), \
>> +        step_index,                                              \
>> +        type,                                                    \
>> +        reg_num,                                                 \
>> +    } })[0].attr.attr)
>> +
>> +#define STEP_PRIORITY(step_index, reg_num, 
>> priority)                     \
>> +    tgu_dataset_rw(reg##reg_num, step_index, TGU_PRIORITY##priority, \
>> +            reg_num)
>> +
>> +#define STEP_PRIORITY_LIST(step_index, priority)  \
>> +    {STEP_PRIORITY(step_index, 0, priority),  \
>> +     STEP_PRIORITY(step_index, 1, priority),  \
>> +     STEP_PRIORITY(step_index, 2, priority),  \
>> +     STEP_PRIORITY(step_index, 3, priority),  \
>> +     STEP_PRIORITY(step_index, 4, priority),  \
>> +     STEP_PRIORITY(step_index, 5, priority),  \
>> +     STEP_PRIORITY(step_index, 6, priority),  \
>> +     STEP_PRIORITY(step_index, 7, priority),  \
>> +     STEP_PRIORITY(step_index, 8, priority),  \
>> +     STEP_PRIORITY(step_index, 9, priority),  \
>> +     STEP_PRIORITY(step_index, 10, priority), \
>> +     STEP_PRIORITY(step_index, 11, priority), \
>> +     STEP_PRIORITY(step_index, 12, priority), \
>> +     STEP_PRIORITY(step_index, 13, priority), \
>> +     STEP_PRIORITY(step_index, 14, priority), \
>> +     STEP_PRIORITY(step_index, 15, priority), \
>> +     STEP_PRIORITY(step_index, 16, priority), \
>> +     STEP_PRIORITY(step_index, 17, priority), \
>> +     NULL                   \
>> +    }
>> +
>> +#define PRIORITY_ATTRIBUTE_GROUP_INIT(step, priority)\
>> +    (&(const struct attribute_group){\
>> +        .attrs = (struct attribute*[])STEP_PRIORITY_LIST(step, 
>> priority),\
>> +        .is_visible = tgu_node_visible,\
>> +        .name = "step" #step "_priority" #priority \
>> +    })
>> +
>> +enum operation_index {
>> +    TGU_PRIORITY0,
>> +    TGU_PRIORITY1,
>> +    TGU_PRIORITY2,
>> +    TGU_PRIORITY3,
>> +};
>> +
>> +/* Maximum priority that TGU supports */
>> +#define MAX_PRIORITY 4
>> +
>> +struct tgu_attribute {
>> +    struct device_attribute attr;
>> +    u32 step_index;
>> +    enum operation_index operation_index;
>> +    u32 reg_num;
>> +};
>> +
>> +struct value_table {
>> +    unsigned int *priority;
> 
> priority here is an array? can we declare it as unsigned int array to 
> limit the allocated memory?
> 
> With the declared array, we can avoid one more memory allocation.
> 
> priority[index] that is used in code looks like a trick.
Yes, it should be an array, The reason it's defined as a pointer is that 
its size is determined at runtime.>
>> +};
> 
> only has one member, better integrate with tgu_drvdata with proper nameing.
  Maybe this approach provides better scalability and enhances 
structural clarity?

> 
> Thanks,
> Jie
> 
>>   static inline void TGU_LOCK(void __iomem *addr)
>>   {
>> @@ -35,6 +142,9 @@ static inline void TGU_UNLOCK(void __iomem *addr)
>>    * @dev: Pointer to the associated device structure
>>    * @lock: Spinlock for handling concurrent access
>>    * @enable: Flag indicating whether the TGU device is enabled
>> + * @value_table: Store given value based on relevant parameters.
>> + * @max_reg: Maximum number of registers
>> + * @max_step: Maximum step size
>>    *
>>    * This structure defines the data associated with a TGU device,
>>    * including its base address, device pointers, clock, spinlock for
>> @@ -46,6 +156,9 @@ struct tgu_drvdata {
>>       struct device *dev;
>>       spinlock_t lock;
>>       bool enable;
>> +    struct value_table *value_table;
>> +    int max_reg;
>> +    int max_step;
>>   };
>>   #endif
>