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;+}
+static ssize_t tgu_dataset_show(struct device *dev,
struct device_attribute *attr, char *buf)+{
- struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
- struct tgu_attribute *tgu_attr =
container_of(attr, struct tgu_attribute, attr);- int index;
- index = calculate_array_location(drvdata, tgu_attr->step_index,
tgu_attr->operation_index,tgu_attr->reg_num);- /*
* Serialise against tgu_dataset_store() and tgu_enable(), which read* and write the same priority array, so the value is not observed* while being updated concurrently.*/- guard(mutex)(&drvdata->lock);
- 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)+{
- struct tgu_drvdata *tgu_drvdata = dev_get_drvdata(dev);
- struct tgu_attribute *tgu_attr =
container_of(attr, struct tgu_attribute, attr);- unsigned long val;
- int index;
- int ret;
- ret = kstrtoul(buf, 0, &val);
- if (ret)
return ret;- guard(mutex)(&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);
- 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->num_step)
return SYSFS_GROUP_INVISIBLE;- if (tgu_attr->reg_num >= drvdata->num_reg)
return 0;- return attr->mode;
+}
- 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->num_step; i++) {
for (j = 0; j < MAX_PRIORITY; j++) {for (k = 0; k < drvdata->num_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);
- num_reg = (num_sense_input * TGU_BITS_PER_SIGNAL) / LENGTH_REGISTER;
- if ((num_sense_input * TGU_BITS_PER_SIGNAL) % LENGTH_REGISTER)
num_reg++;- if (num_reg > TGU_MAX_PRIORITY_REGS) {
dev_warn(drvdata->dev,"DEVID reports %d priority regs, clamping to %d\n",num_reg, TGU_MAX_PRIORITY_REGS);num_reg = TGU_MAX_PRIORITY_REGS;- }
- drvdata->num_reg = num_reg;
+}
+static void tgu_set_steps(struct tgu_drvdata *drvdata) +{
- u32 devid;
- devid = readl(drvdata->base + TGU_DEVID);
- drvdata->num_step = TGU_DEVID_STEPS(devid);
- if (drvdata->num_step > TGU_MAX_STEPS) {
dev_warn(drvdata->dev,"DEVID reports %d steps, clamping to %d\n",drvdata->num_step, TGU_MAX_STEPS);drvdata->num_step = TGU_MAX_STEPS;- }
+}
Why not combine setting the num_step and num_regs in one go ? You are reading the DEVID anyways, and such macro level functional split is useless.
- static void tgu_enable(struct tgu_drvdata *drvdata) { tgu_write_all_hw_regs(drvdata);
@@ -104,6 +234,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, };
@@ -111,6 +273,8 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) { struct device *dev = &adev->dev; struct tgu_drvdata *drvdata;
- unsigned int *priority;
- size_t priority_size; int ret;
drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); @@ -128,8 +292,31 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) if (ret) return ret;
- drvdata->enabled = false;
- tgu_set_reg_number(drvdata);
- tgu_set_steps(drvdata);
As mentioned above, combine both ?
- drvdata->value_table =
devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL);
Could we not embed this in the drvdata without dynamic allocation ?
- if (!drvdata->value_table)
return -ENOMEM;
- priority_size = MAX_PRIORITY * drvdata->num_reg * drvdata->num_step;
- priority = devm_kcalloc(dev, priority_size,
sizeof(*drvdata->value_table->priority),GFP_KERNEL);- if (!priority)
return -ENOMEM;- drvdata->value_table->priority = priority;
- drvdata->enabled = false;
- /*
* Create the sysfs groups only after the backing storage is fully* allocated, otherwise userspace could open a priority attribute and* have tgu_dataset_show() dereference an uninitialised value_table.* This also keeps the error paths above free of sysfs teardown.*/diff --git a/drivers/hwtracing/qcom/tgu.h b/drivers/hwtracing/qcom/tgu.h index ebba49402a20..7a3e1161c4cf 100644 --- a/drivers/hwtracing/qcom/tgu.h +++ b/drivers/hwtracing/qcom/tgu.h @@ -6,10 +6,137 @@ #ifndef _QCOM_TGU_H #define _QCOM_TGU_H +#include <linux/bitfield.h> +#include <linux/bits.h>
- /* Register addresses */ #define TGU_CONTROL 0x0000 #define TGU_LAR 0xfb0 #define TGU_UNLOCK_OFFSET 0xc5acce55
+#define TGU_DEVID 0xfc8
+#define TGU_DEVID_SENSE_INPUT(devid_val) \
- ((int)FIELD_GET(GENMASK(17, 10), devid_val))
+#define TGU_DEVID_STEPS(devid_val) \
- ((int)FIELD_GET(GENMASK(6, 3), devid_val))
+#define TGU_BITS_PER_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$
- | registers | |---> |priority[3] |$
- | | | |-------------|base+n*0x1D8+0x194$
- | | | |priority[2] |$
- |-------------------------| | |-------------|base+n*0x1D8+0x134$
- | | | |priority[1] |$
- | step[7] | | |-------------|base+n*0x1D8+0xD4$
- |-------------------------|->base+0x40+7*0x1D8 | |priority[0] |$
- | | | |-------------|base+n*0x1D8+0x74$
- | ... | | | condition |$
- | | | | select |$
- |-------------------------|->base+0x40+1*0x1D8 | |-------------|base+n*0x1D8+0x60$
minor nit: ^^^ s/base+0x40+1*0x1D8/step[0] + 0x1D8 and so on, for the above
- | | | | 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$
Also for each frame, you don't have to describe "base+n*..." that makes it unnecessarily crowded and complicated to read.
- */
+#define STEP_OFFSET 0x1D8 +#define PRIORITY_START_OFFSET 0x0074 +#define PRIORITY_OFFSET 0x60
From the diagram, it is CONDITION_SELECT ? Having two offsets named after PRIORITY is confusing.
+#define REG_OFFSET 0x4
+/*
- Upper bounds on the DEVID/DEVID2-derived per-region counts. The TGU register
- layout is fixed by the offset macros above and mirrored by the static sysfs
- attribute tables in tgu.c, so these are the largest counts the driver can
- both address within the mapped window and expose to userspace:
- TGU_MAX_STEPS static tables define step0..step7
- TGU_MAX_PRIORITY_REGS STEP_PRIORITY_LIST defines reg0..reg17
What are these ? PRIORITY regs is 0-3 ? Do you mean each priority has 0-17 registers ? (See, the description using base+n*.. for the "step" frame is making this worse). You may move it down as a separate figure.
From my maths: For pritority[0] -> 0xD4-0x74 = 0x60 => 0x18 => 24 registers! not 18!
What am I missing ?
- A device whose DEVID reports counts above these bounds is clamped, so writes
- never fall outside the ioremap()'d window or spill into an adjacent region.
- */
+#define TGU_MAX_STEPS 8 +#define TGU_MAX_PRIORITY_REGS 18
+/* 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 \- })
Please could you move these macro magic to the .c file ? 1. Nobody else reuses them ? 2. It is really difficult to go back and forth to check what each macro means, given how complex it is.
+enum operation_index {
why are we calling it 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;
u8 priority ?
- u32 reg_num;
u8 reg_num ?
+};
+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
- @num_step: Maximum step size
- 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
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; +}
+static ssize_t tgu_dataset_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct tgu_drvdata *drvdata = dev_get_drvdata(dev); + struct tgu_attribute *tgu_attr = + container_of(attr, struct tgu_attribute, attr); + int index;
+ index = calculate_array_location(drvdata, tgu_attr->step_index, + tgu_attr->operation_index, + tgu_attr->reg_num);
+ /* + * Serialise against tgu_dataset_store() and tgu_enable(), which read + * and write the same priority array, so the value is not observed + * while being updated concurrently. + */ + guard(mutex)(&drvdata->lock);
+ 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) +{ + struct tgu_drvdata *tgu_drvdata = dev_get_drvdata(dev); + struct tgu_attribute *tgu_attr = + container_of(attr, struct tgu_attribute, attr); + unsigned long val; + int index; + int ret;
+ ret = kstrtoul(buf, 0, &val); + if (ret) + return ret;
+ guard(mutex)(&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); + 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->num_step) + return SYSFS_GROUP_INVISIBLE;
+ if (tgu_attr->reg_num >= drvdata->num_reg) + return 0;
+ return attr->mode; +}
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->num_step; i++) { + for (j = 0; j < MAX_PRIORITY; j++) { + for (k = 0; k < drvdata->num_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); + num_reg = (num_sense_input * TGU_BITS_PER_SIGNAL) / LENGTH_REGISTER;
+ if ((num_sense_input * TGU_BITS_PER_SIGNAL) % LENGTH_REGISTER) + num_reg++;
+ if (num_reg > TGU_MAX_PRIORITY_REGS) { + dev_warn(drvdata->dev, + "DEVID reports %d priority regs, clamping to %d\n", + num_reg, TGU_MAX_PRIORITY_REGS); + num_reg = TGU_MAX_PRIORITY_REGS; + }
+ drvdata->num_reg = num_reg; +}
+static void tgu_set_steps(struct tgu_drvdata *drvdata) +{ + u32 devid;
+ devid = readl(drvdata->base + TGU_DEVID);
+ drvdata->num_step = TGU_DEVID_STEPS(devid);
+ if (drvdata->num_step > TGU_MAX_STEPS) { + dev_warn(drvdata->dev, + "DEVID reports %d steps, clamping to %d\n", + drvdata->num_step, TGU_MAX_STEPS); + drvdata->num_step = TGU_MAX_STEPS; + } +}
Why not combine setting the num_step and num_regs in one go ? You are reading the DEVID anyways, and such macro level functional split is useless.
static void tgu_enable(struct tgu_drvdata *drvdata) { tgu_write_all_hw_regs(drvdata); @@ -104,6 +234,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, }; @@ -111,6 +273,8 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) { struct device *dev = &adev->dev; struct tgu_drvdata *drvdata; + unsigned int *priority; + size_t priority_size; int ret; drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); @@ -128,8 +292,31 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) if (ret) return ret; - drvdata->enabled = false;
+ tgu_set_reg_number(drvdata); + tgu_set_steps(drvdata);
As mentioned above, combine both ?
+ drvdata->value_table = + devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL);
Could we not embed this in the drvdata without dynamic allocation ?
+ if (!drvdata->value_table) + return -ENOMEM; + priority_size = MAX_PRIORITY * drvdata->num_reg * drvdata->num_step;
+ priority = devm_kcalloc(dev, priority_size, + sizeof(*drvdata->value_table->priority), + GFP_KERNEL); + if (!priority) + return -ENOMEM;
+ drvdata->value_table->priority = priority;
+ drvdata->enabled = false; + /* + * Create the sysfs groups only after the backing storage is fully + * allocated, otherwise userspace could open a priority attribute and + * have tgu_dataset_show() dereference an uninitialised value_table. + * This also keeps the error paths above free of sysfs teardown. + */ ret = sysfs_create_groups(&dev->kobj, tgu_attr_groups); if (ret) { dev_err(dev, "failed to create sysfs groups: %d\n", ret); diff --git a/drivers/hwtracing/qcom/tgu.h b/drivers/hwtracing/qcom/tgu.h index ebba49402a20..7a3e1161c4cf 100644 --- a/drivers/hwtracing/qcom/tgu.h +++ b/drivers/hwtracing/qcom/tgu.h @@ -6,10 +6,137 @@ #ifndef _QCOM_TGU_H #define _QCOM_TGU_H +#include <linux/bitfield.h> +#include <linux/bits.h>
/* Register addresses */ #define TGU_CONTROL 0x0000 #define TGU_LAR 0xfb0 #define TGU_UNLOCK_OFFSET 0xc5acce55 +#define TGU_DEVID 0xfc8
+#define TGU_DEVID_SENSE_INPUT(devid_val) \ + ((int)FIELD_GET(GENMASK(17, 10), devid_val)) +#define TGU_DEVID_STEPS(devid_val) \ + ((int)FIELD_GET(GENMASK(6, 3), devid_val)) +#define TGU_BITS_PER_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$
- | registers | |---> |
priority[3] |$
- | | |
|-------------|base+n*0x1D8+0x194$
- | | | |
priority[2] |$
- |-------------------------| |
|-------------|base+n*0x1D8+0x134$
- | | | |
priority[1] |$
- | step[7] | |
|-------------|base+n*0x1D8+0xD4$
- |-------------------------|->base+0x40+7*0x1D8 | |
priority[0] |$
- | | |
|-------------|base+n*0x1D8+0x74$
- | ... | | |
condition |$
- | | | |
select |$
- |-------------------------|->base+0x40+1*0x1D8 |
|-------------|base+n*0x1D8+0x60$
minor nit: ^^^ s/base+0x40+1*0x1D8/step[0] + 0x1D8 and so on, for the above
- | | | |
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$
Also for each frame, you don't have to describe "base+n*..." that makes it unnecessarily crowded and complicated to read.
- */
+#define STEP_OFFSET 0x1D8
Looking back at the patch 4, this is not really an offset, but size of a STEP. This is true for all the _OFFSETs below.
+#define PRIORITY_START_OFFSET 0x0074 +#define PRIORITY_OFFSET 0x60
From the diagram, it is CONDITION_SELECT ? Having two offsets named after PRIORITY is confusing.
+#define REG_OFFSET 0x4
this one is more wierd, this rather, size of a REG not OFFSET of a REG.
Please could you rename them as such ?
Cheers Suzuki
+/*
- Upper bounds on the DEVID/DEVID2-derived per-region counts. The
TGU register
- layout is fixed by the offset macros above and mirrored by the
static sysfs
- attribute tables in tgu.c, so these are the largest counts the
driver can
- both address within the mapped window and expose to userspace:
- * TGU_MAX_STEPS static tables define step0..step7
- * TGU_MAX_PRIORITY_REGS STEP_PRIORITY_LIST defines reg0..reg17
What are these ? PRIORITY regs is 0-3 ? Do you mean each priority has 0-17 registers ? (See, the description using base+n*.. for the "step" frame is making this worse). You may move it down as a separate figure.
From my maths: For pritority[0] -> 0xD4-0x74 = 0x60 => 0x18 => 24 registers! not 18!
What am I missing ?
- A device whose DEVID reports counts above these bounds is clamped,
so writes
- never fall outside the ioremap()'d window or spill into an
adjacent region.
- */
+#define TGU_MAX_STEPS 8 +#define TGU_MAX_PRIORITY_REGS 18
+/* 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 \ + })
Please could you move these macro magic to the .c file ?
- Nobody else reuses them ?
- It is really difficult to go back and forth to check what each macro
means, given how complex it is.
+enum operation_index {
why are we calling it 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;
u8 priority ?
+ u32 reg_num;
u8 reg_num ?
+};
+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
- @num_step: Maximum step size
* * 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
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
-
Suzuki K Poulose