Re: [PATCH v3 4/7] coresight-tgu: Add TGU decode support

Hi,

On Thu, 27 Feb 2025 at 09:27, songchai quic_songchai@quicinc.com wrote:

From: Songwei Chai quic_songchai@quicinc.com

Decoding is when all the potential pieces for creating a trigger are brought together for a given step. Example - there may be a counter keeping track of some occurrences and a priority-group that is being used to detect a pattern on the sense inputs. These 2 inputs to condition_decode must be programmed, for a given step, to establish the condition for the trigger, or movement to another step.

Signed-off-by: Songwei Chai quic_songchai@quicinc.com Signed-off-by: songchai quic_songchai@quicinc.com

---

.../testing/sysfs-bus-coresight-devices-tgu | 7 ++ drivers/hwtracing/coresight/coresight-tgu.c | 113 ++++++++++++++++-- drivers/hwtracing/coresight/coresight-tgu.h | 29 ++++- 3 files changed, 136 insertions(+), 13 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu index af7332153833..dd6cc1184d52 100644 --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu @@ -14,3 +14,10 @@ KernelVersion 6.15 Contact: Jinlong Mao (QUIC) quic_jinlmao@quicinc.com, Sam Chai (QUIC) quic_songchai@quicinc.com Description: (RW) Set/Get the sensed siganal with specific step and priority for TGU.

+What: /sys/bus/coresight/devices/<tgu-name>/step[0:7]_condition_decode/reg[0:3] +Date: February 2025 +KernelVersion 6.15 +Contact: Jinlong Mao (QUIC) quic_jinlmao@quicinc.com, Sam Chai (QUIC) quic_songchai@quicinc.com +Description:

•            (RW) Set/Get the decode mode with specific step for TGU.
  

diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c index f28761619ebe..5eebf5eecbbb 100644 --- a/drivers/hwtracing/coresight/coresight-tgu.c +++ b/drivers/hwtracing/coresight/coresight-tgu.c @@ -22,9 +22,21 @@ static int calculate_array_location(struct tgu_drvdata *drvdata, int step_index, { int ret = -EINVAL;

•   ret = operation_index * (drvdata->max_step) *
  

•                 (drvdata->max_reg) + step_index * (drvdata->max_reg)
  

•                           + reg_index;
  

•   switch (operation_index) {
  

•   case TGU_PRIORITY0:
  

•   case TGU_PRIORITY1:
  

•   case TGU_PRIORITY2:
  

•   case TGU_PRIORITY3:
  

•           ret = operation_index * (drvdata->max_step) *
  

•                   (drvdata->max_reg) + step_index * (drvdata->max_reg)
  

•                   + reg_index;
  

•           break;
  

•   case TGU_CONDITION_DECODE:
  

•           ret = step_index * (drvdata->max_condition_decode) + reg_index;
  

•           break;
  

•   default:
  

•           break;
  

•   }
  
    return ret;
  

} @@ -36,10 +48,23 @@ static ssize_t tgu_dataset_show(struct device *dev, struct tgu_attribute *tgu_attr = container_of(attr, struct tgu_attribute, attr);

•   return sysfs_emit(buf, "0x%x\n",
  

•                     drvdata->value_table->priority[calculate_array_location(
  

•   switch (tgu_attr->operation_index) {
  

•   case TGU_PRIORITY0:
  

•   case TGU_PRIORITY1:
  

•   case TGU_PRIORITY2:
  

•   case TGU_PRIORITY3:
  

•           return sysfs_emit(buf, "0x%x\n",
  

•                             drvdata->value_table->priority[calculate_array_location(
  

calculate_array_location() can return -EINVAL - you could be referencing array value drvdata->value_table->priority[-EINVAL] here. Test the return before referencing the array.

Same for all following occurrences.

                              drvdata, tgu_attr->step_index,
                              tgu_attr->operation_index, tgu_attr->reg_num)]);

•   case TGU_CONDITION_DECODE:
  

•           return sysfs_emit(buf, "0x%x\n",
  

•                             drvdata->value_table->condition_decode[calculate_array_location(
  

•                             drvdata, tgu_attr->step_index, tgu_attr->operation_index,
  

•                             tgu_attr->reg_num)]);
  

missing default - did this code compile without warnings?

•   }
  

•   return -EINVAL;
  
  

}

@@ -58,11 +83,25 @@ static ssize_t tgu_dataset_store(struct device *dev, return ret;

    guard(spinlock)(&tgu_drvdata->spinlock);

•   tgu_drvdata->value_table->priority[calculate_array_location(
  

•           tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index,
  

•           tgu_attr->reg_num)] = val;
  

•   ret = size;
  

•   switch (tgu_attr->operation_index) {
  

•   case TGU_PRIORITY0:
  

•   case TGU_PRIORITY1:
  

•   case TGU_PRIORITY2:
  

•   case TGU_PRIORITY3:
  

•           tgu_drvdata->value_table->priority[calculate_array_location(
  

•                   tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index,
  

•                   tgu_attr->reg_num)] = val;
  

•           ret = size;
  

•           break;
  

•   case TGU_CONDITION_DECODE:
  

•           tgu_drvdata->value_table->condition_decode[calculate_array_location(
  

•                   tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index,
  

•                   tgu_attr->reg_num)] = val;
  

•           ret = size;
  

•           break;
  

•   default:
  

•           break;
  

•   }
    return ret;
  

}

@@ -79,8 +118,23 @@ static umode_t tgu_node_visible(struct kobject *kobject, struct attribute *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;
  

•           switch (tgu_attr->operation_index) {
  

•           case TGU_PRIORITY0:
  

•           case TGU_PRIORITY1:
  

•           case TGU_PRIORITY2:
  

•           case TGU_PRIORITY3:
  

•                   ret = (tgu_attr->reg_num < drvdata->max_reg) ?
  

•                                 attr->mode : 0;
  

•                   break;
  

•           case TGU_CONDITION_DECODE:
  

•                   ret = (tgu_attr->reg_num <
  

•                                   drvdata->max_condition_decode) ?
  

•                                           attr->mode : 0;
  

•                   break;
  

•           default:
  

•                   break;
  

•           }
  

•           return ret;
    }
    return SYSFS_GROUP_INVISIBLE;
  

@@ -103,6 +157,17 @@ static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata) } }

•   for (i = 0; i < drvdata->max_step; i++) {
  

•           for (j = 0; j < drvdata->max_condition_decode; j++) {
  

•                   tgu_writel(drvdata,
  

•                              drvdata->value_table
  

•                                      ->condition_decode[calculate_array_location(
  

•                                              drvdata, i,
  

•                                              TGU_CONDITION_DECODE, j)],
  

•                              CONDITION_DECODE_STEP(i, j));
  

•           }
  

•   }
  

•   /* Enable TGU to program the triggers */
    tgu_writel(drvdata, 1, TGU_CONTROL);
    CS_LOCK(drvdata->base);
  

@@ -245,6 +310,14 @@ static const struct attribute_group *tgu_attr_groups[] = { PRIORITY_ATTRIBUTE_GROUP_INIT(7, 1), PRIORITY_ATTRIBUTE_GROUP_INIT(7, 2), PRIORITY_ATTRIBUTE_GROUP_INIT(7, 3),

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(0),
  

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(1),
  

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(2),
  

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(3),
  

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(4),
  

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(5),
  

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(6),
  

•   CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(7),
    NULL,
  

};

@@ -289,6 +362,13 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) if (ret) return -EINVAL;

•   ret = of_property_read_u32(adev->dev.of_node, "tgu-conditions",
  

•                              &drvdata->max_condition);
  

•   if (ret)
  

•           return -EINVAL;
  

•   drvdata->max_condition_decode = drvdata->max_condition;
  

•   drvdata->value_table =
            devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL);
    if (!drvdata->value_table)
  

@@ -303,6 +383,15 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) if (!drvdata->value_table->priority) return -ENOMEM;

•   drvdata->value_table->condition_decode = devm_kzalloc(
  

•           dev,
  

•           drvdata->max_condition_decode * drvdata->max_step *
  

•                   sizeof(*(drvdata->value_table->condition_decode)),
  

•           GFP_KERNEL);
  

•   if (!drvdata->value_table->condition_decode)
  

•           return -ENOMEM;
  

•   drvdata->enable = false;
    desc.type = CORESIGHT_DEV_TYPE_HELPER;
    desc.pdata = adev->dev.platform_data;
  

diff --git a/drivers/hwtracing/coresight/coresight-tgu.h b/drivers/hwtracing/coresight/coresight-tgu.h index 6e5d465117df..c2a9ce38b44f 100644 --- a/drivers/hwtracing/coresight/coresight-tgu.h +++ b/drivers/hwtracing/coresight/coresight-tgu.h @@ -46,6 +46,9 @@ #define PRIORITY_REG_STEP(step, priority, reg)\ (0x0074 + 0x60 * priority + 0x4 * reg + 0x1D8 * step)

+#define CONDITION_DECODE_STEP(step, decode) \

•   (0x0050 + 0x4 * decode + 0x1D8 * step)
  

use #define constants with explanations of what they are rather than arbitrary magic numbers.

#define tgu_dataset_ro(name, step_index, type, reg_num) \ (&((struct tgu_attribute[]){ { \ __ATTR(name, 0444, tgu_dataset_show, NULL), \ @@ -66,6 +69,9 @@ tgu_dataset_rw(reg##reg_num, step_index, TGU_PRIORITY##priority, \ reg_num)

+#define STEP_DECODE(step_index, reg_num) \

•   tgu_dataset_rw(reg##reg_num, step_index, TGU_CONDITION_DECODE, reg_num)
  

#define STEP_PRIORITY_LIST(step_index, priority) \ {STEP_PRIORITY(step_index, 0, priority), \ STEP_PRIORITY(step_index, 1, priority), \ @@ -88,6 +94,14 @@ NULL \ }

+#define STEP_DECODE_LIST(n) \

•   {STEP_DECODE(n, 0), \
  

•    STEP_DECODE(n, 1), \
  

•    STEP_DECODE(n, 2), \
  

•    STEP_DECODE(n, 3), \
  

•    NULL           \
  

•   }
  

#define PRIORITY_ATTRIBUTE_GROUP_INIT(step, priority)\ (&(const struct attribute_group){\ .attrs = (struct attribute*[])STEP_PRIORITY_LIST(step, priority),\ @@ -95,11 +109,19 @@ .name = "step" #step "_priority" #priority \ })

+#define CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(step)\

•   (&(const struct attribute_group){\
  

•           .attrs = (struct attribute*[])STEP_DECODE_LIST(step),\
  

•           .is_visible = tgu_node_visible,\
  

•           .name = "step" #step "_condition_decode" \
  

•   })
  

enum operation_index { TGU_PRIORITY0, TGU_PRIORITY1, TGU_PRIORITY2,

•   TGU_PRIORITY3
  

•   TGU_PRIORITY3,
  

•   TGU_CONDITION_DECODE
  
  

};

@@ -115,6 +137,7 @@ struct tgu_attribute {

struct value_table { unsigned int *priority;

•   unsigned int *condition_decode;
  

};

/** @@ -127,6 +150,8 @@ struct value_table {

• @value_table: Store given value based on relevant parameters.
• @max_reg: Maximum number of registers
• @max_step: Maximum step size

• • @max_condition: Maximum number of condition
• • @max_condition_decode: Maximum number of condition_decode
  • This structure defines the data associated with a TGU device, including its base
  • address, device pointers, clock, spinlock for synchronization, trigger data pointers,

@@ -141,6 +166,8 @@ struct tgu_drvdata { struct value_table *value_table; int max_reg; int max_step;

•   int max_condition;
  

•   int max_condition_decode;
  

};

#endif

Regards

Mike