[PATCH v4 1/3] coresight: tpda: add sysfs nodes for tpda cross-trigger configuration
Jie Gan
jie.gan at oss.qualcomm.com
Thu Dec 18 20:53:59 PST 2025
On 12/18/2025 7:20 PM, Suzuki K Poulose wrote:
> On 18/12/2025 11:10, Suzuki K Poulose wrote:
>> On 28/10/2025 02:02, Jie Gan wrote:
>>> From: Tao Zhang <tao.zhang at oss.qualcomm.com>
>>>
>>> Introduce sysfs nodes to configure cross-trigger parameters for TPDA.
>>> These registers define the characteristics of cross-trigger packets,
>>> including generation frequency and flag values.
>>>
>>> Signed-off-by: Tao Zhang <tao.zhang at oss.qualcomm.com>
>>> Reviewed-by: James Clark <james.clark at linaro.org>
>>> Co-developed-by: Jie Gan <jie.gan at oss.qualcomm.com>
>>> Signed-off-by: Jie Gan <jie.gan at oss.qualcomm.com>
>>> ---
>>> .../ABI/testing/sysfs-bus-coresight-devices-tpda | 43 ++++
>>> drivers/hwtracing/coresight/coresight-tpda.c | 230 ++++++++++
>>> + ++++++++++
>>> drivers/hwtracing/coresight/coresight-tpda.h | 27 ++-
>>> 3 files changed, 299 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-
>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> new file mode 100644
>>> index 000000000000..80e4b05a1ab4
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> @@ -0,0 +1,43 @@
>>> +What: /sys/bus/coresight/devices/<tpda-name>/trig_async_enable
>>> +Date: October 2025
>>> +KernelVersion: 6.19
>>> +Contact: Jinlong Mao <jinlong.mao at oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang at oss.qualcomm.com>, Jie Gan <jie.gan at oss.qualcomm.com>
>>> +Description:
>>> + (RW) Enable/disable cross trigger synchronization sequence
>>> interface.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpda-name>/trig_flag_ts_enable
>>> +Date: October 2025
>>> +KernelVersion: 6.19
>>> +Contact: Jinlong Mao <jinlong.mao at oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang at oss.qualcomm.com>, Jie Gan <jie.gan at oss.qualcomm.com>
>>> +Description:
>>> + (RW) Enable/disable cross trigger FLAG packet request
>>> interface.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpda-name>/trig_freq_enable
>>> +Date: October 2025
>>> +KernelVersion: 6.19
>>> +Contact: Jinlong Mao <jinlong.mao at oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang at oss.qualcomm.com>, Jie Gan <jie.gan at oss.qualcomm.com>
>>> +Description:
>>> + (RW) Enable/disable cross trigger FREQ packet request
>>> interface.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpda-name>/freq_ts_enable
>>> +Date: October 2025
>>> +KernelVersion: 6.19
>>> +Contact: Jinlong Mao <jinlong.mao at oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang at oss.qualcomm.com>, Jie Gan <jie.gan at oss.qualcomm.com>
>>> +Description:
>>> + (RW) Enable/disable the timestamp for all FREQ packets.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpda-name>/global_flush_req
>>> +Date: October 2025
>>> +KernelVersion: 6.19
>>> +Contact: Jinlong Mao <jinlong.mao at oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang at oss.qualcomm.com>, Jie Gan <jie.gan at oss.qualcomm.com>
>>> +Description:
>>> + (RW) Set global (all ports) flush request bit. The bit
>>> remains set until a
>>> + global flush request sequence completes.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpda-name>/cmbchan_mode
>>> +Date: October 2025
>>> +KernelVersion: 6.19
>>> +Contact: Jinlong Mao <jinlong.mao at oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang at oss.qualcomm.com>, Jie Gan <jie.gan at oss.qualcomm.com>
>>> +Description:
>>> + (RW) Configure the CMB/MCMB channel mode for all enabled ports.
>>> + Value 0 means raw channel mapping mode. Value 1 means
>>> channel pair marking mode.
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/
>>> hwtracing/coresight/coresight-tpda.c
>>> index 333b3cb23685..a9a27bcc65a1 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -147,9 +147,37 @@ static void tpda_enable_pre_port(struct
>>> tpda_drvdata *drvdata)
>>> u32 val;
>>> val = readl_relaxed(drvdata->base + TPDA_CR);
>>> + val &= ~TPDA_CR_MID;
>>> val &= ~TPDA_CR_ATID;
>>
>> See, below.
>>
>>> val |= FIELD_PREP(TPDA_CR_ATID, drvdata->atid);
>>> + if (drvdata->trig_async)
>>> + val |= TPDA_CR_SRIE;
>>> + else
>>> + val &= ~TPDA_CR_SRIE;
>>> + if (drvdata->trig_flag_ts)
>>> + val |= TPDA_CR_FLRIE;
>>> + else
>>> + val &= ~TPDA_CR_FLRIE;
>>> + if (drvdata->trig_freq)
>>> + val |= TPDA_CR_FRIE;
>>> + else
>>> + val &= ~TPDA_CR_FRIE;
>>> + if (drvdata->freq_ts)
>>> + val |= TPDA_CR_FREQTS;
>>> + else
>>> + val &= ~TPDA_CR_FREQTS;
>>> + if (drvdata->cmbchan_mode)
>>> + val |= TPDA_CR_CMBCHANMODE;
>>> + else
>>> + val &= ~TPDA_CR_CMBCHANMODE;
>>
>> Could we clear all of the bits that are configurable in one go in the
>> beginning and set the appropriate ones based on the setting ? i.e.:
>>
>> Do we really need to retain any values ? And if not, why not start
>> with a fresh set of values and avoid the read ?
>>
>>> writel_relaxed(val, drvdata->base + TPDA_CR);
>>> +
>>> + /*
>>> + * If FLRIE bit is set, set the master and channel
>>> + * id as zero
>>> + */
>>> + if (drvdata->trig_flag_ts)
>>> + writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>>> }
>>> static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>>> @@ -265,6 +293,206 @@ static const struct coresight_ops tpda_cs_ops = {
>>> .link_ops = &tpda_link_ops,
>>> };
>>> +static ssize_t trig_async_enable_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> + return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_async);
>>> +}
>>> +
>>> +static ssize_t trig_async_enable_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf,
>>> + size_t size)
>>> +{
>>> + struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + unsigned long val;
>>> +
>>> + if (kstrtoul(buf, 0, &val))
>>> + return -EINVAL;
>>> +
>>> + guard(spinlock)(&drvdata->spinlock);
>>> + drvdata->trig_async = !!val;
>>> +
>>
>>
>>
>>> + return size;
>>> +}
>>> +static DEVICE_ATTR_RW(trig_async_enable);
>>
>> ...
>>
>>> +static DEVICE_ATTR_RW(trig_flag_ts_enable);
>>
>> ...
>>
>>> +static DEVICE_ATTR_RW(trig_freq_enable);
>>
>> ...
>>> +static DEVICE_ATTR_RW(freq_ts_enable);
>>
>> These attribute are boolean and looks like we could save some space on
>> code by using dev_ext_attribute.
>> see tpdm_simple_dataset_rw()/tpdm_simple_dataset_ro() . You could
>>
>> #define TPDA_TRIG_ASYNC 0
>> #define TPDA_TRIG_FLAG_TS 1
>> #define TPDA_TRIG_FREQ 2
>>
>>
>> tpda_trig_sysfs_show/store()
>>
>> bool *ptr;
>> switch (eattr->var) {
>> case TPDA_TRIG_ASYNC:
>> ptr = &drvdata->trig_async;
>> break;
>> case TPDA_TRIG_FLAG_TS:
>> ptr = &drvdata->trig_flag_ts;
>> break;
>> ...
>>
>> }
>>
>>
>>
>
> -->8--
>
> Cut here
>
>>> +
>>> +static ssize_t global_flush_req_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + unsigned long val;
>>> +
>>> + if (!drvdata->csdev->refcnt)
>>> + return -EINVAL;
>>> +
>>> + guard(spinlock)(&drvdata->spinlock);
>>> + CS_UNLOCK(drvdata->base);
>>> + val = readl_relaxed(drvdata->base + TPDA_CR);
>>> + CS_LOCK(drvdata->base);
>>> + /* Only read value for bit 0 */
>>> + val &= BIT(0);
>>> +
>>> + return sysfs_emit(buf, "%lu\n", val);
>>> +}
>>> +
>>> +static ssize_t global_flush_req_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf,
>>> + size_t size)
>>> +{
>>> + struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + unsigned long val;
>>> +
>>> + if (kstrtoul(buf, 0, &val))
>>> + return -EINVAL;
>>> +
>>> + if (!drvdata->csdev->refcnt || !val)
>>> + return -EINVAL;
>>> +
>>> + guard(spinlock)(&drvdata->spinlock);
>>> + CS_UNLOCK(drvdata->base);
>>> + val = readl_relaxed(drvdata->base + TPDA_CR);
>>> + /* Only set bit 0 */
>>> + val |= BIT(0);
>>
>> What is BIT 0 ? Please document it
>>
>>> + writel_relaxed(val, drvdata->base + TPDA_CR);
>>> + CS_LOCK(drvdata->base);
>>> +
>>> + return size;
>>> +}
>
> Also this, global_flush_req seems to be a separate change from the rest
> of the additions in the patch. Why not split it into a separate patch
> with appropriate description of what this is for ?
Hi Suzuki,
global_flush_req is part of the TPDA_CR register, it's the bit0 of the
register.
I should document it in header file:
/* Cross trigger Global (all ports) flush request bit */
#define TPDA_CR_FLREQ BIT(0)
We also have below bit that is not in use:
#define TPDA_CR_FREQREQ BIT(1)
It's frequency request bit, that determines the frequency for generating
frequency packets.
Thanks,
Jie
>
> Suzuki
More information about the linux-arm-kernel
mailing list