lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJ9a7Vi9CWGKGYe6jBELFN_X6dSG4EfQQd5moju5ekbrLWxi2g@mail.gmail.com>
Date: Fri, 7 Mar 2025 12:17:05 +0000
From: Mike Leach <mike.leach@...aro.org>
To: songchai <quic_songchai@...cinc.com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>, James Clark <james.clark@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Andy Gross <agross@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org, 
	coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org, 
	linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 3/7] coresight-tgu: Add signal priority support

Hi

On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai@...cinc.com> wrote:
>
> From: Songwei Chai <quic_songchai@...cinc.com>
>
> 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 <quic_songchai@...cinc.com>
> Signed-off-by: songchai <quic_songchai@...cinc.com>
> ---
>  .../testing/sysfs-bus-coresight-devices-tgu   |   7 +
>  drivers/hwtracing/coresight/coresight-tgu.c   | 139 ++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tgu.h   | 110 ++++++++++++++
>  3 files changed, 256 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
> index 741bc9fd9df5..af7332153833 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-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/coresight/devices/<tgu-name>/step[0:7]_priority[0:3]/reg[0:17]
> +Date:           February 2025
> +KernelVersion   6.15
> +Contact:        Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Sam Chai (QUIC) <quic_songchai@...cinc.com>
> +Description:
> +                (RW) Set/Get the sensed siganal with specific step and priority for TGU.

sp/siganal/signal

> diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c
> index da4c04ac1097..f28761619ebe 100644
> --- a/drivers/hwtracing/coresight/coresight-tgu.c
> +++ b/drivers/hwtracing/coresight/coresight-tgu.c
> @@ -17,9 +17,92 @@
>
>  DEFINE_CORESIGHT_DEVLIST(tgu_devs, "tgu");
>
> +static int calculate_array_location(struct tgu_drvdata *drvdata, int step_index,
> +                         int operation_index, int reg_index)
> +{
> +       int ret = -EINVAL;
> +
> +       ret = operation_index * (drvdata->max_step) *
> +                     (drvdata->max_reg) + step_index * (drvdata->max_reg)
> +                               + reg_index;
> +
> +       return ret;
> +}
> +
> +static ssize_t tgu_dataset_show(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct tgu_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +       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(
> +                                 drvdata, tgu_attr->step_index,
> +                                 tgu_attr->operation_index, tgu_attr->reg_num)]);
> +
> +}
> +
> +static ssize_t tgu_dataset_store(struct device *dev,
> +                                struct device_attribute *attr, const char *buf,
> +                                size_t size)
> +{
> +       unsigned long val;
> +       ssize_t ret = -EINVAL;
> +
> +       struct tgu_drvdata *tgu_drvdata = dev_get_drvdata(dev->parent);
> +       struct tgu_attribute *tgu_attr =
> +               container_of(attr, struct tgu_attribute, attr);
> +
> +       if (kstrtoul(buf, 0, &val))
> +               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;
> +
> +       return ret;

ret is unneeded - directly return either size or -EINVAL.

> +}
> +
> +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->parent);
> +       int ret = 0;
> +
> +       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;
> +       }
> +       return SYSFS_GROUP_INVISIBLE;
> +}

default ret as SYSFS_GROUP_INVISIBLE, and returnret at end

> +
>  static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>  {
> +       int i, j, k;
> +
>         CS_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++) {
> +                               tgu_writel(drvdata,
> +                                          drvdata->value_table->priority
> +                                                  [calculate_array_location(drvdata, i, j, k)],
> +                                          PRIORITY_REG_STEP(i, j, k));
> +                       }
> +               }
> +       }
> +
>         /* Enable TGU to program the triggers */
>         tgu_writel(drvdata, 1, TGU_CONTROL);
>         CS_LOCK(drvdata->base);
> @@ -130,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,
>  };
>
> @@ -164,6 +279,30 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>
>         spin_lock_init(&drvdata->spinlock);
>
> +       ret = of_property_read_u32(adev->dev.of_node, "tgu-regs",
> +                                  &drvdata->max_reg);
> +       if (ret)
> +               return -EINVAL;
> +
> +       ret = of_property_read_u32(adev->dev.of_node, "tgu-steps",
> +                                  &drvdata->max_step);
> +       if (ret)
> +               return -EINVAL;
> +
> +       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);
> +
> +       if (!drvdata->value_table->priority)
> +               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 380686f94130..6e5d465117df 100644
> --- a/drivers/hwtracing/coresight/coresight-tgu.h
> +++ b/drivers/hwtracing/coresight/coresight-tgu.h
> @@ -13,6 +13,110 @@
>  #define tgu_writel(drvdata, val, off) __raw_writel((val), drvdata->base + off)
>  #define tgu_readl(drvdata, off) __raw_readl(drvdata->base + off)
>
> +/*
> + *  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$
> + *
> + */
> +
> +/* Calculate compare step addresses */
> +#define PRIORITY_REG_STEP(step, priority, reg)\
> +       (0x0074 + 0x60 * priority + 0x4 * reg + 0x1D8 * step)
> +

use #defines + explanation instead of arbitrary magic numbers

> +#define tgu_dataset_ro(name, step_index, type, reg_num)     \
> +       (&((struct tgu_attribute[]){ {                      \
> +               __ATTR(name, 0444, tgu_dataset_show, NULL), \
> +               step_index,                                 \
> +               type,                                       \
> +               reg_num,                                    \
> +       } })[0].attr.attr)
> +


This define unused in this patch, Drop till it is used.

> +#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;
> +};
> +
>  /**
>   * struct tgu_drvdata - Data structure for a TGU (Trigger Generator Unit) device
>   * @base: Memory-mapped base address of the TGU device
> @@ -20,6 +124,9 @@
>   * @csdev: Pointer to the associated coresight device
>   * @spinlock: 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 synchronization, trigger data pointers,
> @@ -31,6 +138,9 @@ struct tgu_drvdata {
>         struct coresight_device *csdev;
>         spinlock_t spinlock;
>         bool enable;
> +       struct value_table *value_table;
> +       int max_reg;
> +       int max_step;
>  };
>
>  #endif

Regards

Mike

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ