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

[Suzuki K Poulose]

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@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Songwei Chai <songwei.chai@xxxxxxxxxxxxxxxx>
> > > ---
> > >   .../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@xxxxxxxxxxxxxxxx>, Songwei Chai 
> > > <songwei.chai@xxxxxxxxxxxxxxxx>
> > > +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