Re: [PATCH v2 3/3] coresight: tpda: add sysfs node to flush specific port

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

On 8/27/2025 6:16 PM, James Clark wrote:
> 
> 
> On 27/08/2025 10:48 am, Jie Gan wrote:
>>
>>
>> On 8/27/2025 5:17 PM, James Clark wrote:
>>>
>>>
>>> On 27/08/2025 5:20 am, Jie Gan wrote:
>>>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>>
>>>> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
>>>> for port i, forcing the data to synchronize and be transmitted to the
>>>> sink device.
>>>>
>>>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>> ---
>>>>   .../testing/sysfs-bus-coresight-devices-tpda  |  7 ++++
>>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 42 +++++++++++++++ 
>>>> ++++
>>>>   drivers/hwtracing/coresight/coresight-tpda.h  |  2 +
>>>>   3 files changed, 51 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>> index fb651aebeb31..2cf2dcfc13c8 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>> @@ -41,3 +41,10 @@ Contact:    Jinlong Mao 
>>>> <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
>>>>   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.
>>>> +
>>>> +What:        /sys/bus/coresight/devices/<tpda-name>/port_flush_req
>>>> +Date:        August 2025
>>>> +KernelVersion:    6.17
>>>> +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 bit i to requests a flush operation of 
>>>> port i on the TPDA.
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>>>> hwtracing/coresight/coresight-tpda.c
>>>> index 430f76c559f2..8b1fe128881d 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -487,6 +487,47 @@ static ssize_t cmbchan_mode_store(struct device 
>>>> *dev,
>>>>   }
>>>>   static DEVICE_ATTR_RW(cmbchan_mode);
>>>> +static ssize_t port_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_FLUSH_CR);
>>>> +    CS_LOCK(drvdata->base);
>>>> +    return sysfs_emit(buf, "%lx\n", val);
>>>
>>> Still missing the 0x prefix
>>
>> Will re-check rest of the codes and add prefix. Sorry I missed it 
>> during the review process.
>>
>>>
>>>> +}
>>>> +
>>>> +static ssize_t port_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;
>>>> +
>>>> +    val |= FIELD_PREP(TPDA_MAX_INPORTS_MASK, val);
>>>
>>> Using FIELD_PREP() now that it's the full width of the register makes 
>>> less sense. Especially when there is no corresponding FIELD_FIT() 
>>> check,   which is fine because everything always fits. But if you 
>>> didn't know the mask was the full width you'd wonder if the check is 
>>> missing.
>>>
>>> I would just write val directly to TPDA_FLUSH_CR so it's simpler.
>>>
>>> It should also have been val = FIELD_PREP(...)
>>
>> Yeah, it should have been val = FIELD_PREP(...) here... sorry for the 
>> mistake here..
>>
>> I was thinking the unsigned long here could be 64 or 32 bits and we 
>> only need the value of the lower 32 bits. So that's why I am using val 
>> = FIELD_PREP(...) here. We shouldn't write a value greater than 
>> UINT32_MAX to the register.
>>
>> Thanks,
>> Jie
>>
> 
> writel_relaxed() is always 32 bits though so it is a bit confusing if 
> you truncate the user value without an error. Also a reason to use u32 
> instead of unsigned long types.
> 
> Are you trying to support arm and arm64 with tpda? Or just arm64? For it 
> to be consistent you can use kstrtou32(), or use kstrtoull() and then 
> FIELD_FIT() to error on truncation. kstrtou32() is probably the cleanest.

Only arm64. Will try kstrtou32() with u32.

Thanks,
Jie

> 
>>>
>>>> +    guard(spinlock)(&drvdata->spinlock);
>>>> +    CS_UNLOCK(drvdata->base);
>>>> +    writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
>>>> +    CS_LOCK(drvdata->base);
>>>> +
>>>> +    return size;
>>>> +}
>>>> +static DEVICE_ATTR_RW(port_flush_req);
>>>> +
>>>>   static struct attribute *tpda_attrs[] = {
>>>>       &dev_attr_trig_async_enable.attr,
>>>>       &dev_attr_trig_flag_ts_enable.attr,
>>>> @@ -494,6 +535,7 @@ static struct attribute *tpda_attrs[] = {
>>>>       &dev_attr_freq_ts_enable.attr,
>>>>       &dev_attr_global_flush_req.attr,
>>>>       &dev_attr_cmbchan_mode.attr,
>>>> +    &dev_attr_port_flush_req.attr,
>>>>       NULL,
>>>>   };
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>>>> hwtracing/coresight/coresight-tpda.h
>>>> index 8e1b66115ad1..56d3ad293e46 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>>>> @@ -10,6 +10,7 @@
>>>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>>>   #define TPDA_FPID_CR        (0x084)
>>>>   #define TPDA_SYNCR        (0x08C)
>>>> +#define TPDA_FLUSH_CR        (0x090)
>>>>   /* Cross trigger FREQ packets timestamp bit */
>>>>   #define TPDA_CR_FREQTS        BIT(2)
>>>> @@ -35,6 +36,7 @@
>>>>   #define TPDA_SYNCR_MAX_COUNTER_VAL    (0xFFF)
>>>>   #define TPDA_MAX_INPORTS    32
>>>> +#define TPDA_MAX_INPORTS_MASK    GENMASK(31, 0)
>>>>   /* Bits 6 ~ 12 is for atid value */
>>>>   #define TPDA_CR_ATID        GENMASK(12, 6)
>>>
>>>
>>
> 
>