Re: [PATCH v4 1/3] coresight: tpda: add sysfs nodes for tpda cross-trigger configuration

[Jie Gan <jie.gan@oss.qualcomm.com>]

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@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@oss.qualcomm.com>
>>> Reviewed-by: James Clark <james.clark@linaro.org>
>>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>> Signed-off-by: Jie Gan <jie.gan@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@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@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@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@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@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@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@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@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@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@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@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@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