diff mbox series

[v3,6/8] coresight-tpdm: Add timestamp control register support for the CMB

Message ID 1700533494-19276-7-git-send-email-quic_taozha@quicinc.com
State New
Headers
Series [v3,1/8] dt-bindings: arm: Add support for CMB element size |

Commit Message

Tao Zhang Nov. 21, 2023, 2:24 a.m. UTC 
  CMB_TIER register is CMB subunit timestamp insertion enable register.
Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
Set this bit to 1 to request a timestamp following a CMB CTI timestamp
request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
for all packets.

Reviewed-by: James Clark <james.clark@arm.com>
Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
---
 .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
 drivers/hwtracing/coresight/coresight-tpdm.c  | 116 +++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
 3 files changed, 162 insertions(+), 3 deletions(-)

Comments

Suzuki K Poulose Dec. 18, 2023, 10:46 a.m. UTC | #1 
On 21/11/2023 02:24, Tao Zhang wrote:
> CMB_TIER register is CMB subunit timestamp insertion enable register.
> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
> Set this bit to 1 to request a timestamp following a CMB CTI timestamp
> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
> for all packets.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
> ---
>   .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 +++++++++++++++++-
>   drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
>   3 files changed, 162 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index 53662ce7c2d0..e0b77107be13 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -214,3 +214,38 @@ KernelVersion	6.7
>   Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>   Description:
>   		(RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
> +Date:		September 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(Write) Set the pattern timestamp of CMB tpdm. Read
> +		the pattern timestamp of CMB tpdm.
> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Disable CMB pattern timestamp.
> +		1 : Enable CMB pattern timestamp.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
> +Date:		September 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Set/Get the trigger timestamp of the CMB for tpdm.
> +
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Set the CMB trigger type to false
> +		1 : Set the CMB trigger type to true
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
> +Date:		September 2023
> +KernelVersion	6.7
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		(RW) Read or write the status of timestamp upon all interface.
> +		Only value 0 and 1  can be written to this node. Set this node to 1 to requeset
> +		timestamp to all trace packet.
> +		Accepts only one of the 2 values -  0 or 1.
> +		0 : Disable the timestamp of all trace packets.
> +		1 : Enable the timestamp of all trace packets.
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 894d4309f1c7..f6cda5616e84 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>   }
>   
> +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
> +
> +	/* Clear all relevant fields */
> +	val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
> +		 TPDM_CMB_TIER_XTRIG_TSENAB);
> +
> +	/* Set pattern timestamp type and enablement */
> +	if (drvdata->cmb->patt_ts)
> +		val |= TPDM_CMB_TIER_PATT_TSENAB;

  -- cut --
> +	else
> +		val &= ~TPDM_CMB_TIER_PATT_TSENAB;


All the else cases in this function are superfluous. Please remove all
of them.

> +
> +	/* Set trigger timestamp */
> +	if (drvdata->cmb->trig_ts)
> +		val |= TPDM_CMB_TIER_XTRIG_TSENAB;
> +	else
> +		val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
> +
> +	/* Set all timestamp enablement*/
> +	if (drvdata->cmb->ts_all)
> +		val |= TPDM_CMB_TIER_TS_ALL;
> +	else
> +		val &= ~TPDM_CMB_TIER_TS_ALL;
> +	writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
> +}
> +
>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>   {
>   	u32 val, i;
> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>   			    drvdata->base + TPDM_CMB_XPMR(i));
>   	}
>   
> +	set_cmb_tier(drvdata);
> +
>   	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>   	/*
>   	 * Set to 0 for continuous CMB collection mode,
> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device *dev,
>   			      char *buf)
>   {
>   	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
>   
> -	return sysfs_emit(buf, "%u\n",
> -			 (unsigned int)drvdata->dsb->patt_ts);
> +	if (tpdm_has_dsb_dataset(drvdata))
> +		size = sysfs_emit(buf, "%u\n",
> +				 (unsigned int)drvdata->dsb->patt_ts);
> +
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		size = sysfs_emit(buf, "%u\n",
> +				 (unsigned int)drvdata->cmb->patt_ts);

Why does this need to show two values ? This must only show ONE value.
How you deduce that might be based on the availability of the feature
set. Or store the TS value in the drvdata and use that instead for
controlling CMB/DSB.

Also, the sysfs documentation needs update, if this is going to
control the CMB.

Suzuki


> +
> +	return size;
>   }
>   
>   /*
> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device *dev,
>   		return -EINVAL;
>   
>   	spin_lock(&drvdata->spinlock);
> -	drvdata->dsb->patt_ts = !!val;
> +	if (tpdm_has_dsb_dataset(drvdata))
> +		drvdata->dsb->patt_ts = !!val;
> +
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		drvdata->cmb->patt_ts = !!val;
>   	spin_unlock(&drvdata->spinlock);
> +
>   	return size;
>   }
>   static DEVICE_ATTR_RW(enable_ts);
> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device *dev,
>   }
>   static DEVICE_ATTR_RW(cmb_mode);
>   
> +static ssize_t cmb_ts_all_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n",
> +			  (unsigned int)drvdata->cmb->ts_all);
> +}
> +
> +static ssize_t cmb_ts_all_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf,
> +				size_t size)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	if (val)
> +		drvdata->cmb->ts_all = true;
> +	else
> +		drvdata->cmb->ts_all = false;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(cmb_ts_all);
> +
> +static ssize_t cmb_trig_ts_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n",
> +			  (unsigned int)drvdata->cmb->trig_ts);
> +}
> +
> +static ssize_t cmb_trig_ts_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf,
> +				 size_t size)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	if (val)
> +		drvdata->cmb->trig_ts = true;
> +	else
> +		drvdata->cmb->trig_ts = false;
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_RW(cmb_trig_ts);
> +
>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>   	&dev_attr_ctrl_idx.attr,
>   	&dev_attr_ctrl_val.attr,
> @@ -973,6 +1080,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
>   	CMB_PATT_ATTR(1),
>   	CMB_PATT_MASK_ATTR(0),
>   	CMB_PATT_MASK_ATTR(1),
> +	&dev_attr_enable_ts.attr,
>   	NULL,
>   };
>   
> @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>   
>   static struct attribute *tpdm_cmb_attrs[] = {
>   	&dev_attr_cmb_mode.attr,
> +	&dev_attr_cmb_ts_all.attr,
> +	&dev_attr_cmb_trig_ts.attr,
>   	NULL,
>   };
>   
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index e90d008c1cb2..65b7ca6c4077 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -11,6 +11,8 @@
>   
>   /* CMB Subunit Registers */
>   #define TPDM_CMB_CR		(0xA00)
> +/*CMB subunit timestamp insertion enable register*/
> +#define TPDM_CMB_TIER		(0xA04)
>   /*CMB subunit timestamp pattern registers*/
>   #define TPDM_CMB_TPR(n)		(0xA08 + (n * 4))
>   /*CMB subunit timestamp pattern mask registers*/
> @@ -24,6 +26,12 @@
>   #define TPDM_CMB_CR_ENA		BIT(0)
>   /* Trace collection mode for CMB subunit */
>   #define TPDM_CMB_CR_MODE	BIT(1)
> +/* Timestamp control for pattern match */
> +#define TPDM_CMB_TIER_PATT_TSENAB	BIT(0)
> +/* CMB CTI timestamp request */
> +#define TPDM_CMB_TIER_XTRIG_TSENAB	BIT(1)
> +/* For timestamp fo all trace */
> +#define TPDM_CMB_TIER_TS_ALL		BIT(2)
>   
>   /*Patten register number*/
>   #define TPDM_CMB_MAX_PATT		2
> @@ -217,6 +225,9 @@ struct dsb_dataset {
>    * @patt_mask:        Save value for pattern mask
>    * @trig_patt:        Save value for trigger pattern
>    * @trig_patt_mask:   Save value for trigger pattern mask
> + * @patt_ts:          Indicates if pattern match for timestamp is enabled.
> + * @trig_ts:          Indicates if CTI trigger for timestamp is enabled.
> + * @ts_all:           Indicates if timestamp is enabled for all packets.
>    */
>   struct cmb_dataset {
>   	u32			trace_mode;
> @@ -224,6 +235,9 @@ struct cmb_dataset {
>   	u32			patt_mask[TPDM_CMB_MAX_PATT];
>   	u32			trig_patt[TPDM_CMB_MAX_PATT];
>   	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
> +	bool			patt_ts;
> +	bool			trig_ts;
> +	bool			ts_all;
>   };
>   
>   /**
Tao Zhang Dec. 19, 2023, 2:43 a.m. UTC | #2 
On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
> On 21/11/2023 02:24, Tao Zhang wrote:
>> CMB_TIER register is CMB subunit timestamp insertion enable register.
>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
>> Set this bit to 1 to request a timestamp following a CMB CTI timestamp
>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
>> for all packets.
>>
>> Reviewed-by: James Clark <james.clark@arm.com>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>> ---
>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 +++++++++++++++++-
>>   drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index 53662ce7c2d0..e0b77107be13 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>   Description:
>>           (RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>> +Date:        September 2023
>> +KernelVersion    6.7
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>> +        the pattern timestamp of CMB tpdm.
>> +
>> +        Accepts only one of the 2 values -  0 or 1.
>> +        0 : Disable CMB pattern timestamp.
>> +        1 : Enable CMB pattern timestamp.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>> +Date:        September 2023
>> +KernelVersion    6.7
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>> +
>> +        Accepts only one of the 2 values -  0 or 1.
>> +        0 : Set the CMB trigger type to false
>> +        1 : Set the CMB trigger type to true
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>> +Date:        September 2023
>> +KernelVersion    6.7
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (RW) Read or write the status of timestamp upon all interface.
>> +        Only value 0 and 1  can be written to this node. Set this 
>> node to 1 to requeset
>> +        timestamp to all trace packet.
>> +        Accepts only one of the 2 values -  0 or 1.
>> +        0 : Disable the timestamp of all trace packets.
>> +        1 : Enable the timestamp of all trace packets.
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 894d4309f1c7..f6cda5616e84 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>> *drvdata)
>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>   }
>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>> +{
>> +    u32 val;
>> +
>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>> +
>> +    /* Clear all relevant fields */
>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>> +
>> +    /* Set pattern timestamp type and enablement */
>> +    if (drvdata->cmb->patt_ts)
>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>
>  -- cut --
>> +    else
>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>
>
> All the else cases in this function are superfluous. Please remove all
> of them.
I will update this in the next patch.
>
>> +
>> +    /* Set trigger timestamp */
>> +    if (drvdata->cmb->trig_ts)
>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>> +    else
>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>> +
>> +    /* Set all timestamp enablement*/
>> +    if (drvdata->cmb->ts_all)
>> +        val |= TPDM_CMB_TIER_TS_ALL;
>> +    else
>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>> +}
>> +
>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>   {
>>       u32 val, i;
>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata 
>> *drvdata)
>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>       }
>>   +    set_cmb_tier(drvdata);
>> +
>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>       /*
>>        * Set to 0 for continuous CMB collection mode,
>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device *dev,
>>                     char *buf)
>>   {
>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    ssize_t size = 0;
>>   -    return sysfs_emit(buf, "%u\n",
>> -             (unsigned int)drvdata->dsb->patt_ts);
>> +    if (tpdm_has_dsb_dataset(drvdata))
>> +        size = sysfs_emit(buf, "%u\n",
>> +                 (unsigned int)drvdata->dsb->patt_ts);
>> +
>> +    if (tpdm_has_cmb_dataset(drvdata))
>> +        size = sysfs_emit(buf, "%u\n",
>> +                 (unsigned int)drvdata->cmb->patt_ts);
>
> Why does this need to show two values ? This must only show ONE value.
> How you deduce that might be based on the availability of the feature
> set. Or store the TS value in the drvdata and use that instead for
> controlling CMB/DSB.

Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
separate them

as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.

tpdm0/dsb_patt/enable_dsb_ts

tpdm1/cmb_patt/enable_cmb_ts

Is this design appropriate?

>
> Also, the sysfs documentation needs update, if this is going to
> control the CMB.

Sure. I will update the SysFs documentation according to the 
modification in the

next patch series.


Best,

Tao

>
> Suzuki
>
>
>> +
>> +    return size;
>>   }
>>     /*
>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device *dev,
>>           return -EINVAL;
>>         spin_lock(&drvdata->spinlock);
>> -    drvdata->dsb->patt_ts = !!val;
>> +    if (tpdm_has_dsb_dataset(drvdata))
>> +        drvdata->dsb->patt_ts = !!val;
>> +
>> +    if (tpdm_has_cmb_dataset(drvdata))
>> +        drvdata->cmb->patt_ts = !!val;
>>       spin_unlock(&drvdata->spinlock);
>> +
>>       return size;
>>   }
>>   static DEVICE_ATTR_RW(enable_ts);
>> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device *dev,
>>   }
>>   static DEVICE_ATTR_RW(cmb_mode);
>>   +static ssize_t cmb_ts_all_show(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%u\n",
>> +              (unsigned int)drvdata->cmb->ts_all);
>> +}
>> +
>> +static ssize_t cmb_ts_all_store(struct device *dev,
>> +                struct device_attribute *attr,
>> +                const char *buf,
>> +                size_t size)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    if (val)
>> +        drvdata->cmb->ts_all = true;
>> +    else
>> +        drvdata->cmb->ts_all = false;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(cmb_ts_all);
>> +
>> +static ssize_t cmb_trig_ts_show(struct device *dev,
>> +                struct device_attribute *attr,
>> +                char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%u\n",
>> +              (unsigned int)drvdata->cmb->trig_ts);
>> +}
>> +
>> +static ssize_t cmb_trig_ts_store(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 const char *buf,
>> +                 size_t size)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    if (val)
>> +        drvdata->cmb->trig_ts = true;
>> +    else
>> +        drvdata->cmb->trig_ts = false;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(cmb_trig_ts);
>> +
>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>       &dev_attr_ctrl_idx.attr,
>>       &dev_attr_ctrl_val.attr,
>> @@ -973,6 +1080,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
>>       CMB_PATT_ATTR(1),
>>       CMB_PATT_MASK_ATTR(0),
>>       CMB_PATT_MASK_ATTR(1),
>> +    &dev_attr_enable_ts.attr,
>>       NULL,
>>   };
>>   @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>     static struct attribute *tpdm_cmb_attrs[] = {
>>       &dev_attr_cmb_mode.attr,
>> +    &dev_attr_cmb_ts_all.attr,
>> +    &dev_attr_cmb_trig_ts.attr,
>>       NULL,
>>   };
>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index e90d008c1cb2..65b7ca6c4077 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -11,6 +11,8 @@
>>     /* CMB Subunit Registers */
>>   #define TPDM_CMB_CR        (0xA00)
>> +/*CMB subunit timestamp insertion enable register*/
>> +#define TPDM_CMB_TIER        (0xA04)
>>   /*CMB subunit timestamp pattern registers*/
>>   #define TPDM_CMB_TPR(n)        (0xA08 + (n * 4))
>>   /*CMB subunit timestamp pattern mask registers*/
>> @@ -24,6 +26,12 @@
>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>   /* Trace collection mode for CMB subunit */
>>   #define TPDM_CMB_CR_MODE    BIT(1)
>> +/* Timestamp control for pattern match */
>> +#define TPDM_CMB_TIER_PATT_TSENAB    BIT(0)
>> +/* CMB CTI timestamp request */
>> +#define TPDM_CMB_TIER_XTRIG_TSENAB    BIT(1)
>> +/* For timestamp fo all trace */
>> +#define TPDM_CMB_TIER_TS_ALL        BIT(2)
>>     /*Patten register number*/
>>   #define TPDM_CMB_MAX_PATT        2
>> @@ -217,6 +225,9 @@ struct dsb_dataset {
>>    * @patt_mask:        Save value for pattern mask
>>    * @trig_patt:        Save value for trigger pattern
>>    * @trig_patt_mask:   Save value for trigger pattern mask
>> + * @patt_ts:          Indicates if pattern match for timestamp is 
>> enabled.
>> + * @trig_ts:          Indicates if CTI trigger for timestamp is 
>> enabled.
>> + * @ts_all:           Indicates if timestamp is enabled for all 
>> packets.
>>    */
>>   struct cmb_dataset {
>>       u32            trace_mode;
>> @@ -224,6 +235,9 @@ struct cmb_dataset {
>>       u32            patt_mask[TPDM_CMB_MAX_PATT];
>>       u32            trig_patt[TPDM_CMB_MAX_PATT];
>>       u32            trig_patt_mask[TPDM_CMB_MAX_PATT];
>> +    bool            patt_ts;
>> +    bool            trig_ts;
>> +    bool            ts_all;
>>   };
>>     /**
>
Suzuki K Poulose Dec. 19, 2023, 1:51 p.m. UTC | #3 
On 19/12/2023 02:43, Tao Zhang wrote:
> 
> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>> On 21/11/2023 02:24, Tao Zhang wrote:
>>> CMB_TIER register is CMB subunit timestamp insertion enable register.
>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
>>> Set this bit to 1 to request a timestamp following a CMB CTI timestamp
>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
>>> for all packets.
>>>
>>> Reviewed-by: James Clark <james.clark@arm.com>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>> ---
>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 +++++++++++++++++-
>>>   drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>
>>> diff --git 
>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> index 53662ce7c2d0..e0b77107be13 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>   Description:
>>>           (RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>> +Date:        September 2023
>>> +KernelVersion    6.7
>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>>> (QUIC) <quic_taozha@quicinc.com>
>>> +Description:
>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>> +        the pattern timestamp of CMB tpdm.
>>> +
>>> +        Accepts only one of the 2 values -  0 or 1.
>>> +        0 : Disable CMB pattern timestamp.
>>> +        1 : Enable CMB pattern timestamp.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>> +Date:        September 2023
>>> +KernelVersion    6.7
>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>>> (QUIC) <quic_taozha@quicinc.com>
>>> +Description:
>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>> +
>>> +        Accepts only one of the 2 values -  0 or 1.
>>> +        0 : Set the CMB trigger type to false
>>> +        1 : Set the CMB trigger type to true
>>> +
>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>> +Date:        September 2023
>>> +KernelVersion    6.7
>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>>> (QUIC) <quic_taozha@quicinc.com>
>>> +Description:
>>> +        (RW) Read or write the status of timestamp upon all interface.
>>> +        Only value 0 and 1  can be written to this node. Set this 
>>> node to 1 to requeset
>>> +        timestamp to all trace packet.
>>> +        Accepts only one of the 2 values -  0 or 1.
>>> +        0 : Disable the timestamp of all trace packets.
>>> +        1 : Enable the timestamp of all trace packets.
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> index 894d4309f1c7..f6cda5616e84 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>>> *drvdata)
>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>   }
>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>> +
>>> +    /* Clear all relevant fields */
>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>> +
>>> +    /* Set pattern timestamp type and enablement */
>>> +    if (drvdata->cmb->patt_ts)
>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>
>>  -- cut --
>>> +    else
>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>
>>
>> All the else cases in this function are superfluous. Please remove all
>> of them.
> I will update this in the next patch.
>>
>>> +
>>> +    /* Set trigger timestamp */
>>> +    if (drvdata->cmb->trig_ts)
>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>> +    else
>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>> +
>>> +    /* Set all timestamp enablement*/
>>> +    if (drvdata->cmb->ts_all)
>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>> +    else
>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>> +}
>>> +
>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>   {
>>>       u32 val, i;
>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata 
>>> *drvdata)
>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>       }
>>>   +    set_cmb_tier(drvdata);
>>> +
>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>       /*
>>>        * Set to 0 for continuous CMB collection mode,
>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device *dev,
>>>                     char *buf)
>>>   {
>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    ssize_t size = 0;
>>>   -    return sysfs_emit(buf, "%u\n",
>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>> +        size = sysfs_emit(buf, "%u\n",
>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>> +
>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>> +        size = sysfs_emit(buf, "%u\n",
>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>
>> Why does this need to show two values ? This must only show ONE value.
>> How you deduce that might be based on the availability of the feature
>> set. Or store the TS value in the drvdata and use that instead for
>> controlling CMB/DSB.
> 
> Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
> separate them

The question really is, do we need fine grained control. i.e.,

enable TS for DSB but not for CMB or vice versa.

I am not an expert on the usage scenario of the same. So, if you/Qcomm
thinks the users need separate, fine grained control for timestamp
for the DSB and CMB, then yes, follow your recommendation below.
i.e., tpdm.../dsb_patt/enable_ts

> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.
> 
> tpdm0/dsb_patt/enable_dsb_ts

You don't need enable_dsb_ts. It could be "enable_ts"

> 
> tpdm1/cmb_patt/enable_cmb_ts
> 
> Is this design appropriate?


Otherwise, stick to single enable_ts : which enables the ts for both
CMB/DSB. And it only ever show one value : 0 (TS is disabled for both
CMB/DSB) 1 : TS enabled for both.

Suzuki


>>

>> Also, the sysfs documentation needs update, if this is going to
>> control the CMB.
> 
> Sure. I will update the SysFs documentation according to the 
> modification in the
> 
> next patch series.
> 
> 
> Best,
> 
> Tao
> 
>>
>> Suzuki
>>
>>
>>> +
>>> +    return size;
>>>   }
>>>     /*
>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device *dev,
>>>           return -EINVAL;
>>>         spin_lock(&drvdata->spinlock);
>>> -    drvdata->dsb->patt_ts = !!val;
>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>> +        drvdata->dsb->patt_ts = !!val;
>>> +
>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>> +        drvdata->cmb->patt_ts = !!val;
>>>       spin_unlock(&drvdata->spinlock);
>>> +
>>>       return size;
>>>   }
>>>   static DEVICE_ATTR_RW(enable_ts);
>>> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device *dev,
>>>   }
>>>   static DEVICE_ATTR_RW(cmb_mode);
>>>   +static ssize_t cmb_ts_all_show(struct device *dev,
>>> +                   struct device_attribute *attr,
>>> +                   char *buf)
>>> +{
>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    return sysfs_emit(buf, "%u\n",
>>> +              (unsigned int)drvdata->cmb->ts_all);
>>> +}
>>> +
>>> +static ssize_t cmb_ts_all_store(struct device *dev,
>>> +                struct device_attribute *attr,
>>> +                const char *buf,
>>> +                size_t size)
>>> +{
>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>> +        return -EINVAL;
>>> +
>>> +    spin_lock(&drvdata->spinlock);
>>> +    if (val)
>>> +        drvdata->cmb->ts_all = true;
>>> +    else
>>> +        drvdata->cmb->ts_all = false;
>>> +    spin_unlock(&drvdata->spinlock);
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(cmb_ts_all);
>>> +
>>> +static ssize_t cmb_trig_ts_show(struct device *dev,
>>> +                struct device_attribute *attr,
>>> +                char *buf)
>>> +{
>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    return sysfs_emit(buf, "%u\n",
>>> +              (unsigned int)drvdata->cmb->trig_ts);
>>> +}
>>> +
>>> +static ssize_t cmb_trig_ts_store(struct device *dev,
>>> +                 struct device_attribute *attr,
>>> +                 const char *buf,
>>> +                 size_t size)
>>> +{
>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>> +        return -EINVAL;
>>> +
>>> +    spin_lock(&drvdata->spinlock);
>>> +    if (val)
>>> +        drvdata->cmb->trig_ts = true;
>>> +    else
>>> +        drvdata->cmb->trig_ts = false;
>>> +    spin_unlock(&drvdata->spinlock);
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(cmb_trig_ts);
>>> +
>>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>>       &dev_attr_ctrl_idx.attr,
>>>       &dev_attr_ctrl_val.attr,
>>> @@ -973,6 +1080,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
>>>       CMB_PATT_ATTR(1),
>>>       CMB_PATT_MASK_ATTR(0),
>>>       CMB_PATT_MASK_ATTR(1),
>>> +    &dev_attr_enable_ts.attr,
>>>       NULL,
>>>   };
>>>   @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>>     static struct attribute *tpdm_cmb_attrs[] = {
>>>       &dev_attr_cmb_mode.attr,
>>> +    &dev_attr_cmb_ts_all.attr,
>>> +    &dev_attr_cmb_trig_ts.attr,
>>>       NULL,
>>>   };
>>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> index e90d008c1cb2..65b7ca6c4077 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> @@ -11,6 +11,8 @@
>>>     /* CMB Subunit Registers */
>>>   #define TPDM_CMB_CR        (0xA00)
>>> +/*CMB subunit timestamp insertion enable register*/
>>> +#define TPDM_CMB_TIER        (0xA04)
>>>   /*CMB subunit timestamp pattern registers*/
>>>   #define TPDM_CMB_TPR(n)        (0xA08 + (n * 4))
>>>   /*CMB subunit timestamp pattern mask registers*/
>>> @@ -24,6 +26,12 @@
>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>   /* Trace collection mode for CMB subunit */
>>>   #define TPDM_CMB_CR_MODE    BIT(1)
>>> +/* Timestamp control for pattern match */
>>> +#define TPDM_CMB_TIER_PATT_TSENAB    BIT(0)
>>> +/* CMB CTI timestamp request */
>>> +#define TPDM_CMB_TIER_XTRIG_TSENAB    BIT(1)
>>> +/* For timestamp fo all trace */
>>> +#define TPDM_CMB_TIER_TS_ALL        BIT(2)
>>>     /*Patten register number*/
>>>   #define TPDM_CMB_MAX_PATT        2
>>> @@ -217,6 +225,9 @@ struct dsb_dataset {
>>>    * @patt_mask:        Save value for pattern mask
>>>    * @trig_patt:        Save value for trigger pattern
>>>    * @trig_patt_mask:   Save value for trigger pattern mask
>>> + * @patt_ts:          Indicates if pattern match for timestamp is 
>>> enabled.
>>> + * @trig_ts:          Indicates if CTI trigger for timestamp is 
>>> enabled.
>>> + * @ts_all:           Indicates if timestamp is enabled for all 
>>> packets.
>>>    */
>>>   struct cmb_dataset {
>>>       u32            trace_mode;
>>> @@ -224,6 +235,9 @@ struct cmb_dataset {
>>>       u32            patt_mask[TPDM_CMB_MAX_PATT];
>>>       u32            trig_patt[TPDM_CMB_MAX_PATT];
>>>       u32            trig_patt_mask[TPDM_CMB_MAX_PATT];
>>> +    bool            patt_ts;
>>> +    bool            trig_ts;
>>> +    bool            ts_all;
>>>   };
>>>     /**
>>
Tao Zhang Dec. 20, 2023, 9:51 a.m. UTC | #4 
On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
> On 19/12/2023 02:43, Tao Zhang wrote:
>>
>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>> CMB_TIER register is CMB subunit timestamp insertion enable register.
>>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>>>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
>>>> Set this bit to 1 to request a timestamp following a CMB CTI timestamp
>>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
>>>> for all packets.
>>>>
>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>> ---
>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 
>>>> +++++++++++++++++-
>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
>>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git 
>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> index 53662ce7c2d0..e0b77107be13 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>   Description:
>>>>           (RW) Set/Get the mask of the pattern for the CMB subunit 
>>>> TPDM.
>>>> +
>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>>> +Date:        September 2023
>>>> +KernelVersion    6.7
>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>> +Description:
>>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>>> +        the pattern timestamp of CMB tpdm.
>>>> +
>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>> +        0 : Disable CMB pattern timestamp.
>>>> +        1 : Enable CMB pattern timestamp.
>>>> +
>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>>> +Date:        September 2023
>>>> +KernelVersion    6.7
>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>> +Description:
>>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>>> +
>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>> +        0 : Set the CMB trigger type to false
>>>> +        1 : Set the CMB trigger type to true
>>>> +
>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>>> +Date:        September 2023
>>>> +KernelVersion    6.7
>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>> +Description:
>>>> +        (RW) Read or write the status of timestamp upon all 
>>>> interface.
>>>> +        Only value 0 and 1  can be written to this node. Set this 
>>>> node to 1 to requeset
>>>> +        timestamp to all trace packet.
>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>> +        0 : Disable the timestamp of all trace packets.
>>>> +        1 : Enable the timestamp of all trace packets.
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> index 894d4309f1c7..f6cda5616e84 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct 
>>>> tpdm_drvdata *drvdata)
>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>   }
>>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>>> +
>>>> +    /* Clear all relevant fields */
>>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>>> +
>>>> +    /* Set pattern timestamp type and enablement */
>>>> +    if (drvdata->cmb->patt_ts)
>>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>>
>>>  -- cut --
>>>> +    else
>>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>>
>>>
>>> All the else cases in this function are superfluous. Please remove all
>>> of them.
>> I will update this in the next patch.
>>>
>>>> +
>>>> +    /* Set trigger timestamp */
>>>> +    if (drvdata->cmb->trig_ts)
>>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>>> +    else
>>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>>> +
>>>> +    /* Set all timestamp enablement*/
>>>> +    if (drvdata->cmb->ts_all)
>>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>>> +    else
>>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>> +}
>>>> +
>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>   {
>>>>       u32 val, i;
>>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata 
>>>> *drvdata)
>>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>>       }
>>>>   +    set_cmb_tier(drvdata);
>>>> +
>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>       /*
>>>>        * Set to 0 for continuous CMB collection mode,
>>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device *dev,
>>>>                     char *buf)
>>>>   {
>>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    ssize_t size = 0;
>>>>   -    return sysfs_emit(buf, "%u\n",
>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>> +        size = sysfs_emit(buf, "%u\n",
>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>> +
>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>> +        size = sysfs_emit(buf, "%u\n",
>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>
>>> Why does this need to show two values ? This must only show ONE value.
>>> How you deduce that might be based on the availability of the feature
>>> set. Or store the TS value in the drvdata and use that instead for
>>> controlling CMB/DSB.
>>
>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
>> separate them
>
> The question really is, do we need fine grained control. i.e.,
>
> enable TS for DSB but not for CMB or vice versa.
>
> I am not an expert on the usage scenario of the same. So, if you/Qcomm
> thinks the users need separate, fine grained control for timestamp
> for the DSB and CMB, then yes, follow your recommendation below.
> i.e., tpdm.../dsb_patt/enable_ts
>
>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.
>>
>> tpdm0/dsb_patt/enable_dsb_ts
>
> You don't need enable_dsb_ts. It could be "enable_ts"
>
>>
>> tpdm1/cmb_patt/enable_cmb_ts
>>
>> Is this design appropriate?
>
>
> Otherwise, stick to single enable_ts : which enables the ts for both
> CMB/DSB. And it only ever show one value : 0 (TS is disabled for both
> CMB/DSB) 1 : TS enabled for both.

We have a very special case, such as the TPDM supporting both CMB and

DSB datasets. Although this case is very rare, it still exists.

Can I use the data bit to instruct whether timestamp is enabled for 
CMB/DSB or not? For example,

size = sysfs_emit(buf, "%u\n",
                 (unsigned int)(drvdata->dsb->patt_ts << 1 | 
drvdata->cmb->patt_ts));

Thus, this value can instruct the following situations.

0 - TS is disabled for both CMB/DSB

1 - TS is enabled for CMB

2 - TS is enabled for DSB

3 - TS is enabled for both

Is this approach acceptable?


Best,

Tao

>
> Suzuki
>
>
>>>
>
>>> Also, the sysfs documentation needs update, if this is going to
>>> control the CMB.
>>
>> Sure. I will update the SysFs documentation according to the 
>> modification in the
>>
>> next patch series.
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>>>
>>>> +
>>>> +    return size;
>>>>   }
>>>>     /*
>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device 
>>>> *dev,
>>>>           return -EINVAL;
>>>>         spin_lock(&drvdata->spinlock);
>>>> -    drvdata->dsb->patt_ts = !!val;
>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>> +        drvdata->dsb->patt_ts = !!val;
>>>> +
>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>       spin_unlock(&drvdata->spinlock);
>>>> +
>>>>       return size;
>>>>   }
>>>>   static DEVICE_ATTR_RW(enable_ts);
>>>> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device *dev,
>>>>   }
>>>>   static DEVICE_ATTR_RW(cmb_mode);
>>>>   +static ssize_t cmb_ts_all_show(struct device *dev,
>>>> +                   struct device_attribute *attr,
>>>> +                   char *buf)
>>>> +{
>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +
>>>> +    return sysfs_emit(buf, "%u\n",
>>>> +              (unsigned int)drvdata->cmb->ts_all);
>>>> +}
>>>> +
>>>> +static ssize_t cmb_ts_all_store(struct device *dev,
>>>> +                struct device_attribute *attr,
>>>> +                const char *buf,
>>>> +                size_t size)
>>>> +{
>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    unsigned long val;
>>>> +
>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>> +        return -EINVAL;
>>>> +
>>>> +    spin_lock(&drvdata->spinlock);
>>>> +    if (val)
>>>> +        drvdata->cmb->ts_all = true;
>>>> +    else
>>>> +        drvdata->cmb->ts_all = false;
>>>> +    spin_unlock(&drvdata->spinlock);
>>>> +    return size;
>>>> +}
>>>> +static DEVICE_ATTR_RW(cmb_ts_all);
>>>> +
>>>> +static ssize_t cmb_trig_ts_show(struct device *dev,
>>>> +                struct device_attribute *attr,
>>>> +                char *buf)
>>>> +{
>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +
>>>> +    return sysfs_emit(buf, "%u\n",
>>>> +              (unsigned int)drvdata->cmb->trig_ts);
>>>> +}
>>>> +
>>>> +static ssize_t cmb_trig_ts_store(struct device *dev,
>>>> +                 struct device_attribute *attr,
>>>> +                 const char *buf,
>>>> +                 size_t size)
>>>> +{
>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    unsigned long val;
>>>> +
>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>> +        return -EINVAL;
>>>> +
>>>> +    spin_lock(&drvdata->spinlock);
>>>> +    if (val)
>>>> +        drvdata->cmb->trig_ts = true;
>>>> +    else
>>>> +        drvdata->cmb->trig_ts = false;
>>>> +    spin_unlock(&drvdata->spinlock);
>>>> +    return size;
>>>> +}
>>>> +static DEVICE_ATTR_RW(cmb_trig_ts);
>>>> +
>>>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>>>       &dev_attr_ctrl_idx.attr,
>>>>       &dev_attr_ctrl_val.attr,
>>>> @@ -973,6 +1080,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] 
>>>> = {
>>>>       CMB_PATT_ATTR(1),
>>>>       CMB_PATT_MASK_ATTR(0),
>>>>       CMB_PATT_MASK_ATTR(1),
>>>> +    &dev_attr_enable_ts.attr,
>>>>       NULL,
>>>>   };
>>>>   @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>>>     static struct attribute *tpdm_cmb_attrs[] = {
>>>>       &dev_attr_cmb_mode.attr,
>>>> +    &dev_attr_cmb_ts_all.attr,
>>>> +    &dev_attr_cmb_trig_ts.attr,
>>>>       NULL,
>>>>   };
>>>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> index e90d008c1cb2..65b7ca6c4077 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> @@ -11,6 +11,8 @@
>>>>     /* CMB Subunit Registers */
>>>>   #define TPDM_CMB_CR        (0xA00)
>>>> +/*CMB subunit timestamp insertion enable register*/
>>>> +#define TPDM_CMB_TIER        (0xA04)
>>>>   /*CMB subunit timestamp pattern registers*/
>>>>   #define TPDM_CMB_TPR(n)        (0xA08 + (n * 4))
>>>>   /*CMB subunit timestamp pattern mask registers*/
>>>> @@ -24,6 +26,12 @@
>>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>>   /* Trace collection mode for CMB subunit */
>>>>   #define TPDM_CMB_CR_MODE    BIT(1)
>>>> +/* Timestamp control for pattern match */
>>>> +#define TPDM_CMB_TIER_PATT_TSENAB    BIT(0)
>>>> +/* CMB CTI timestamp request */
>>>> +#define TPDM_CMB_TIER_XTRIG_TSENAB    BIT(1)
>>>> +/* For timestamp fo all trace */
>>>> +#define TPDM_CMB_TIER_TS_ALL        BIT(2)
>>>>     /*Patten register number*/
>>>>   #define TPDM_CMB_MAX_PATT        2
>>>> @@ -217,6 +225,9 @@ struct dsb_dataset {
>>>>    * @patt_mask:        Save value for pattern mask
>>>>    * @trig_patt:        Save value for trigger pattern
>>>>    * @trig_patt_mask:   Save value for trigger pattern mask
>>>> + * @patt_ts:          Indicates if pattern match for timestamp is 
>>>> enabled.
>>>> + * @trig_ts:          Indicates if CTI trigger for timestamp is 
>>>> enabled.
>>>> + * @ts_all:           Indicates if timestamp is enabled for all 
>>>> packets.
>>>>    */
>>>>   struct cmb_dataset {
>>>>       u32            trace_mode;
>>>> @@ -224,6 +235,9 @@ struct cmb_dataset {
>>>>       u32            patt_mask[TPDM_CMB_MAX_PATT];
>>>>       u32            trig_patt[TPDM_CMB_MAX_PATT];
>>>>       u32            trig_patt_mask[TPDM_CMB_MAX_PATT];
>>>> +    bool            patt_ts;
>>>> +    bool            trig_ts;
>>>> +    bool            ts_all;
>>>>   };
>>>>     /**
>>>
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
Suzuki K Poulose Dec. 20, 2023, 11:07 a.m. UTC | #5 
On 20/12/2023 09:51, Tao Zhang wrote:
> 
> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>
>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>> CMB_TIER register is CMB subunit timestamp insertion enable register.
>>>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>>>>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
>>>>> Set this bit to 1 to request a timestamp following a CMB CTI timestamp
>>>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
>>>>> for all packets.
>>>>>
>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>>> ---
>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 
>>>>> +++++++++++++++++-
>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
>>>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git 
>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>> index 53662ce7c2d0..e0b77107be13 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>   Description:
>>>>>           (RW) Set/Get the mask of the pattern for the CMB subunit 
>>>>> TPDM.
>>>>> +
>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>>>> +Date:        September 2023
>>>>> +KernelVersion    6.7
>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>> +Description:
>>>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>>>> +        the pattern timestamp of CMB tpdm.
>>>>> +
>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>> +        0 : Disable CMB pattern timestamp.
>>>>> +        1 : Enable CMB pattern timestamp.
>>>>> +
>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>>>> +Date:        September 2023
>>>>> +KernelVersion    6.7
>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>> +Description:
>>>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>>>> +
>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>> +        0 : Set the CMB trigger type to false
>>>>> +        1 : Set the CMB trigger type to true
>>>>> +
>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>>>> +Date:        September 2023
>>>>> +KernelVersion    6.7
>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>> +Description:
>>>>> +        (RW) Read or write the status of timestamp upon all 
>>>>> interface.
>>>>> +        Only value 0 and 1  can be written to this node. Set this 
>>>>> node to 1 to requeset
>>>>> +        timestamp to all trace packet.
>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>> +        0 : Disable the timestamp of all trace packets.
>>>>> +        1 : Enable the timestamp of all trace packets.
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>> index 894d4309f1c7..f6cda5616e84 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct 
>>>>> tpdm_drvdata *drvdata)
>>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>>   }
>>>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>>>> +{
>>>>> +    u32 val;
>>>>> +
>>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>>>> +
>>>>> +    /* Clear all relevant fields */
>>>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>>>> +
>>>>> +    /* Set pattern timestamp type and enablement */
>>>>> +    if (drvdata->cmb->patt_ts)
>>>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>>>
>>>>  -- cut --
>>>>> +    else
>>>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>>>
>>>>
>>>> All the else cases in this function are superfluous. Please remove all
>>>> of them.
>>> I will update this in the next patch.
>>>>
>>>>> +
>>>>> +    /* Set trigger timestamp */
>>>>> +    if (drvdata->cmb->trig_ts)
>>>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>> +    else
>>>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>> +
>>>>> +    /* Set all timestamp enablement*/
>>>>> +    if (drvdata->cmb->ts_all)
>>>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>>>> +    else
>>>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>> +}
>>>>> +
>>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>>   {
>>>>>       u32 val, i;
>>>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata 
>>>>> *drvdata)
>>>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>>>       }
>>>>>   +    set_cmb_tier(drvdata);
>>>>> +
>>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>>       /*
>>>>>        * Set to 0 for continuous CMB collection mode,
>>>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device *dev,
>>>>>                     char *buf)
>>>>>   {
>>>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +    ssize_t size = 0;
>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>> +
>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>
>>>> Why does this need to show two values ? This must only show ONE value.
>>>> How you deduce that might be based on the availability of the feature
>>>> set. Or store the TS value in the drvdata and use that instead for
>>>> controlling CMB/DSB.
>>>
>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
>>> separate them
>>
>> The question really is, do we need fine grained control. i.e.,
>>
>> enable TS for DSB but not for CMB or vice versa.
>>
>> I am not an expert on the usage scenario of the same. So, if you/Qcomm
>> thinks the users need separate, fine grained control for timestamp
>> for the DSB and CMB, then yes, follow your recommendation below.
>> i.e., tpdm.../dsb_patt/enable_ts
>>
>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.
>>>
>>> tpdm0/dsb_patt/enable_dsb_ts
>>
>> You don't need enable_dsb_ts. It could be "enable_ts"
>>
>>>
>>> tpdm1/cmb_patt/enable_cmb_ts
>>>
>>> Is this design appropriate?
>>
>>
>> Otherwise, stick to single enable_ts : which enables the ts for both
>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for both
>> CMB/DSB) 1 : TS enabled for both.
> 
> We have a very special case, such as the TPDM supporting both CMB and
> 
> DSB datasets. Although this case is very rare, it still exists.
> 
> Can I use the data bit to instruct whether timestamp is enabled for 
> CMB/DSB or not? For example,
> 
> size = sysfs_emit(buf, "%u\n",
>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
> drvdata->cmb->patt_ts));
> 
> Thus, this value can instruct the following situations.
> 
> 0 - TS is disabled for both CMB/DSB
> 
> 1 - TS is enabled for CMB
> 
> 2 - TS is enabled for DSB
> 
> 3 - TS is enabled for both
> 
> Is this approach acceptable?
> 

No, please stick to separate controls for TS. Do not complicate
the user interface.

i.e.,
tpdm0/dsb_patt/enable_ts
tpdm0/cmb_patt/enable_ts

Suzuki


> 
> Best,
> 
> Tao
> 
>>
>> Suzuki
>>
>>
>>>>
>>
>>>> Also, the sysfs documentation needs update, if this is going to
>>>> control the CMB.
>>>
>>> Sure. I will update the SysFs documentation according to the 
>>> modification in the
>>>
>>> next patch series.
>>>
>>>
>>> Best,
>>>
>>> Tao
>>>
>>>>
>>>> Suzuki
>>>>
>>>>
>>>>> +
>>>>> +    return size;
>>>>>   }
>>>>>     /*
>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device 
>>>>> *dev,
>>>>>           return -EINVAL;
>>>>>         spin_lock(&drvdata->spinlock);
>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>> +
>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>       spin_unlock(&drvdata->spinlock);
>>>>> +
>>>>>       return size;
>>>>>   }
>>>>>   static DEVICE_ATTR_RW(enable_ts);
>>>>> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device *dev,
>>>>>   }
>>>>>   static DEVICE_ATTR_RW(cmb_mode);
>>>>>   +static ssize_t cmb_ts_all_show(struct device *dev,
>>>>> +                   struct device_attribute *attr,
>>>>> +                   char *buf)
>>>>> +{
>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +
>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>> +              (unsigned int)drvdata->cmb->ts_all);
>>>>> +}
>>>>> +
>>>>> +static ssize_t cmb_ts_all_store(struct device *dev,
>>>>> +                struct device_attribute *attr,
>>>>> +                const char *buf,
>>>>> +                size_t size)
>>>>> +{
>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +    unsigned long val;
>>>>> +
>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    spin_lock(&drvdata->spinlock);
>>>>> +    if (val)
>>>>> +        drvdata->cmb->ts_all = true;
>>>>> +    else
>>>>> +        drvdata->cmb->ts_all = false;
>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>> +    return size;
>>>>> +}
>>>>> +static DEVICE_ATTR_RW(cmb_ts_all);
>>>>> +
>>>>> +static ssize_t cmb_trig_ts_show(struct device *dev,
>>>>> +                struct device_attribute *attr,
>>>>> +                char *buf)
>>>>> +{
>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +
>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>> +              (unsigned int)drvdata->cmb->trig_ts);
>>>>> +}
>>>>> +
>>>>> +static ssize_t cmb_trig_ts_store(struct device *dev,
>>>>> +                 struct device_attribute *attr,
>>>>> +                 const char *buf,
>>>>> +                 size_t size)
>>>>> +{
>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +    unsigned long val;
>>>>> +
>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    spin_lock(&drvdata->spinlock);
>>>>> +    if (val)
>>>>> +        drvdata->cmb->trig_ts = true;
>>>>> +    else
>>>>> +        drvdata->cmb->trig_ts = false;
>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>> +    return size;
>>>>> +}
>>>>> +static DEVICE_ATTR_RW(cmb_trig_ts);
>>>>> +
>>>>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>>>>       &dev_attr_ctrl_idx.attr,
>>>>>       &dev_attr_ctrl_val.attr,
>>>>> @@ -973,6 +1080,7 @@ static struct attribute *tpdm_cmb_patt_attrs[] 
>>>>> = {
>>>>>       CMB_PATT_ATTR(1),
>>>>>       CMB_PATT_MASK_ATTR(0),
>>>>>       CMB_PATT_MASK_ATTR(1),
>>>>> +    &dev_attr_enable_ts.attr,
>>>>>       NULL,
>>>>>   };
>>>>>   @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>>>>     static struct attribute *tpdm_cmb_attrs[] = {
>>>>>       &dev_attr_cmb_mode.attr,
>>>>> +    &dev_attr_cmb_ts_all.attr,
>>>>> +    &dev_attr_cmb_trig_ts.attr,
>>>>>       NULL,
>>>>>   };
>>>>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> index e90d008c1cb2..65b7ca6c4077 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> @@ -11,6 +11,8 @@
>>>>>     /* CMB Subunit Registers */
>>>>>   #define TPDM_CMB_CR        (0xA00)
>>>>> +/*CMB subunit timestamp insertion enable register*/
>>>>> +#define TPDM_CMB_TIER        (0xA04)
>>>>>   /*CMB subunit timestamp pattern registers*/
>>>>>   #define TPDM_CMB_TPR(n)        (0xA08 + (n * 4))
>>>>>   /*CMB subunit timestamp pattern mask registers*/
>>>>> @@ -24,6 +26,12 @@
>>>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>>>   /* Trace collection mode for CMB subunit */
>>>>>   #define TPDM_CMB_CR_MODE    BIT(1)
>>>>> +/* Timestamp control for pattern match */
>>>>> +#define TPDM_CMB_TIER_PATT_TSENAB    BIT(0)
>>>>> +/* CMB CTI timestamp request */
>>>>> +#define TPDM_CMB_TIER_XTRIG_TSENAB    BIT(1)
>>>>> +/* For timestamp fo all trace */
>>>>> +#define TPDM_CMB_TIER_TS_ALL        BIT(2)
>>>>>     /*Patten register number*/
>>>>>   #define TPDM_CMB_MAX_PATT        2
>>>>> @@ -217,6 +225,9 @@ struct dsb_dataset {
>>>>>    * @patt_mask:        Save value for pattern mask
>>>>>    * @trig_patt:        Save value for trigger pattern
>>>>>    * @trig_patt_mask:   Save value for trigger pattern mask
>>>>> + * @patt_ts:          Indicates if pattern match for timestamp is 
>>>>> enabled.
>>>>> + * @trig_ts:          Indicates if CTI trigger for timestamp is 
>>>>> enabled.
>>>>> + * @ts_all:           Indicates if timestamp is enabled for all 
>>>>> packets.
>>>>>    */
>>>>>   struct cmb_dataset {
>>>>>       u32            trace_mode;
>>>>> @@ -224,6 +235,9 @@ struct cmb_dataset {
>>>>>       u32            patt_mask[TPDM_CMB_MAX_PATT];
>>>>>       u32            trig_patt[TPDM_CMB_MAX_PATT];
>>>>>       u32            trig_patt_mask[TPDM_CMB_MAX_PATT];
>>>>> +    bool            patt_ts;
>>>>> +    bool            trig_ts;
>>>>> +    bool            ts_all;
>>>>>   };
>>>>>     /**
>>>>
>>
>> _______________________________________________
>> CoreSight mailing list -- coresight@lists.linaro.org
>> To unsubscribe send an email to coresight-leave@lists.linaro.org
Tao Zhang Dec. 25, 2023, 1:55 a.m. UTC | #6 
On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
> On 20/12/2023 09:51, Tao Zhang wrote:
>>
>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>
>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>> CMB_TIER register is CMB subunit timestamp insertion enable 
>>>>>> register.
>>>>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>>>>>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
>>>>>> Set this bit to 1 to request a timestamp following a CMB CTI 
>>>>>> timestamp
>>>>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
>>>>>> for all packets.
>>>>>>
>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>>>> ---
>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 
>>>>>> +++++++++++++++++-
>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
>>>>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git 
>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> index 53662ce7c2d0..e0b77107be13 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>   Description:
>>>>>>           (RW) Set/Get the mask of the pattern for the CMB 
>>>>>> subunit TPDM.
>>>>>> +
>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>>>>> +Date:        September 2023
>>>>>> +KernelVersion    6.7
>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>> +Description:
>>>>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>>>>> +        the pattern timestamp of CMB tpdm.
>>>>>> +
>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>> +        0 : Disable CMB pattern timestamp.
>>>>>> +        1 : Enable CMB pattern timestamp.
>>>>>> +
>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>>>>> +Date:        September 2023
>>>>>> +KernelVersion    6.7
>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>> +Description:
>>>>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>>>>> +
>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>> +        0 : Set the CMB trigger type to false
>>>>>> +        1 : Set the CMB trigger type to true
>>>>>> +
>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>>>>> +Date:        September 2023
>>>>>> +KernelVersion    6.7
>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>> +Description:
>>>>>> +        (RW) Read or write the status of timestamp upon all 
>>>>>> interface.
>>>>>> +        Only value 0 and 1  can be written to this node. Set 
>>>>>> this node to 1 to requeset
>>>>>> +        timestamp to all trace packet.
>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>> +        0 : Disable the timestamp of all trace packets.
>>>>>> +        1 : Enable the timestamp of all trace packets.
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> index 894d4309f1c7..f6cda5616e84 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct 
>>>>>> tpdm_drvdata *drvdata)
>>>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>>>   }
>>>>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>>>>> +{
>>>>>> +    u32 val;
>>>>>> +
>>>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>>>>> +
>>>>>> +    /* Clear all relevant fields */
>>>>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>>>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>>>>> +
>>>>>> +    /* Set pattern timestamp type and enablement */
>>>>>> +    if (drvdata->cmb->patt_ts)
>>>>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>>>>
>>>>>  -- cut --
>>>>>> +    else
>>>>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>>>>
>>>>>
>>>>> All the else cases in this function are superfluous. Please remove 
>>>>> all
>>>>> of them.
>>>> I will update this in the next patch.
>>>>>
>>>>>> +
>>>>>> +    /* Set trigger timestamp */
>>>>>> +    if (drvdata->cmb->trig_ts)
>>>>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>> +    else
>>>>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>> +
>>>>>> +    /* Set all timestamp enablement*/
>>>>>> +    if (drvdata->cmb->ts_all)
>>>>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>>>>> +    else
>>>>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>>> +}
>>>>>> +
>>>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>>>   {
>>>>>>       u32 val, i;
>>>>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct 
>>>>>> tpdm_drvdata *drvdata)
>>>>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>>>>       }
>>>>>>   +    set_cmb_tier(drvdata);
>>>>>> +
>>>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>>>       /*
>>>>>>        * Set to 0 for continuous CMB collection mode,
>>>>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device 
>>>>>> *dev,
>>>>>>                     char *buf)
>>>>>>   {
>>>>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +    ssize_t size = 0;
>>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>>> +
>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>>
>>>>> Why does this need to show two values ? This must only show ONE 
>>>>> value.
>>>>> How you deduce that might be based on the availability of the feature
>>>>> set. Or store the TS value in the drvdata and use that instead for
>>>>> controlling CMB/DSB.
>>>>
>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
>>>> separate them
>>>
>>> The question really is, do we need fine grained control. i.e.,
>>>
>>> enable TS for DSB but not for CMB or vice versa.
>>>
>>> I am not an expert on the usage scenario of the same. So, if you/Qcomm
>>> thinks the users need separate, fine grained control for timestamp
>>> for the DSB and CMB, then yes, follow your recommendation below.
>>> i.e., tpdm.../dsb_patt/enable_ts
>>>
>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.
>>>>
>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>
>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>
>>>>
>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>
>>>> Is this design appropriate?
>>>
>>>
>>> Otherwise, stick to single enable_ts : which enables the ts for both
>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for both
>>> CMB/DSB) 1 : TS enabled for both.
>>
>> We have a very special case, such as the TPDM supporting both CMB and
>>
>> DSB datasets. Although this case is very rare, it still exists.
>>
>> Can I use the data bit to instruct whether timestamp is enabled for 
>> CMB/DSB or not? For example,
>>
>> size = sysfs_emit(buf, "%u\n",
>>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
>> drvdata->cmb->patt_ts));
>>
>> Thus, this value can instruct the following situations.
>>
>> 0 - TS is disabled for both CMB/DSB
>>
>> 1 - TS is enabled for CMB
>>
>> 2 - TS is enabled for DSB
>>
>> 3 - TS is enabled for both
>>
>> Is this approach acceptable?
>>
>
> No, please stick to separate controls for TS. Do not complicate
> the user interface.
>
> i.e.,
> tpdm0/dsb_patt/enable_ts
> tpdm0/cmb_patt/enable_ts

We need to be able to control/show dsb and cmb timestamp enablement 
independently.

Can we achieve this requirement if we use a sysfs file with the same name?

I'm not able to find a good way to achieve it. Do you have any 
suggestions for this?


Best,

Tao

>
> Suzuki
>
>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>>>
>>>>>
>>>
>>>>> Also, the sysfs documentation needs update, if this is going to
>>>>> control the CMB.
>>>>
>>>> Sure. I will update the SysFs documentation according to the 
>>>> modification in the
>>>>
>>>> next patch series.
>>>>
>>>>
>>>> Best,
>>>>
>>>> Tao
>>>>
>>>>>
>>>>> Suzuki
>>>>>
>>>>>
>>>>>> +
>>>>>> +    return size;
>>>>>>   }
>>>>>>     /*
>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device 
>>>>>> *dev,
>>>>>>           return -EINVAL;
>>>>>>         spin_lock(&drvdata->spinlock);
>>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>>> +
>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>>       spin_unlock(&drvdata->spinlock);
>>>>>> +
>>>>>>       return size;
>>>>>>   }
>>>>>>   static DEVICE_ATTR_RW(enable_ts);
>>>>>> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device 
>>>>>> *dev,
>>>>>>   }
>>>>>>   static DEVICE_ATTR_RW(cmb_mode);
>>>>>>   +static ssize_t cmb_ts_all_show(struct device *dev,
>>>>>> +                   struct device_attribute *attr,
>>>>>> +                   char *buf)
>>>>>> +{
>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +
>>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>>> +              (unsigned int)drvdata->cmb->ts_all);
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t cmb_ts_all_store(struct device *dev,
>>>>>> +                struct device_attribute *attr,
>>>>>> +                const char *buf,
>>>>>> +                size_t size)
>>>>>> +{
>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +    unsigned long val;
>>>>>> +
>>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>> +    if (val)
>>>>>> +        drvdata->cmb->ts_all = true;
>>>>>> +    else
>>>>>> +        drvdata->cmb->ts_all = false;
>>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>>> +    return size;
>>>>>> +}
>>>>>> +static DEVICE_ATTR_RW(cmb_ts_all);
>>>>>> +
>>>>>> +static ssize_t cmb_trig_ts_show(struct device *dev,
>>>>>> +                struct device_attribute *attr,
>>>>>> +                char *buf)
>>>>>> +{
>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +
>>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>>> +              (unsigned int)drvdata->cmb->trig_ts);
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t cmb_trig_ts_store(struct device *dev,
>>>>>> +                 struct device_attribute *attr,
>>>>>> +                 const char *buf,
>>>>>> +                 size_t size)
>>>>>> +{
>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +    unsigned long val;
>>>>>> +
>>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>> +    if (val)
>>>>>> +        drvdata->cmb->trig_ts = true;
>>>>>> +    else
>>>>>> +        drvdata->cmb->trig_ts = false;
>>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>>> +    return size;
>>>>>> +}
>>>>>> +static DEVICE_ATTR_RW(cmb_trig_ts);
>>>>>> +
>>>>>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>>>>>       &dev_attr_ctrl_idx.attr,
>>>>>>       &dev_attr_ctrl_val.attr,
>>>>>> @@ -973,6 +1080,7 @@ static struct attribute 
>>>>>> *tpdm_cmb_patt_attrs[] = {
>>>>>>       CMB_PATT_ATTR(1),
>>>>>>       CMB_PATT_MASK_ATTR(0),
>>>>>>       CMB_PATT_MASK_ATTR(1),
>>>>>> +    &dev_attr_enable_ts.attr,
>>>>>>       NULL,
>>>>>>   };
>>>>>>   @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>>>>>     static struct attribute *tpdm_cmb_attrs[] = {
>>>>>>       &dev_attr_cmb_mode.attr,
>>>>>> +    &dev_attr_cmb_ts_all.attr,
>>>>>> +    &dev_attr_cmb_trig_ts.attr,
>>>>>>       NULL,
>>>>>>   };
>>>>>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>> index e90d008c1cb2..65b7ca6c4077 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>> @@ -11,6 +11,8 @@
>>>>>>     /* CMB Subunit Registers */
>>>>>>   #define TPDM_CMB_CR        (0xA00)
>>>>>> +/*CMB subunit timestamp insertion enable register*/
>>>>>> +#define TPDM_CMB_TIER        (0xA04)
>>>>>>   /*CMB subunit timestamp pattern registers*/
>>>>>>   #define TPDM_CMB_TPR(n)        (0xA08 + (n * 4))
>>>>>>   /*CMB subunit timestamp pattern mask registers*/
>>>>>> @@ -24,6 +26,12 @@
>>>>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>>>>   /* Trace collection mode for CMB subunit */
>>>>>>   #define TPDM_CMB_CR_MODE    BIT(1)
>>>>>> +/* Timestamp control for pattern match */
>>>>>> +#define TPDM_CMB_TIER_PATT_TSENAB    BIT(0)
>>>>>> +/* CMB CTI timestamp request */
>>>>>> +#define TPDM_CMB_TIER_XTRIG_TSENAB    BIT(1)
>>>>>> +/* For timestamp fo all trace */
>>>>>> +#define TPDM_CMB_TIER_TS_ALL        BIT(2)
>>>>>>     /*Patten register number*/
>>>>>>   #define TPDM_CMB_MAX_PATT        2
>>>>>> @@ -217,6 +225,9 @@ struct dsb_dataset {
>>>>>>    * @patt_mask:        Save value for pattern mask
>>>>>>    * @trig_patt:        Save value for trigger pattern
>>>>>>    * @trig_patt_mask:   Save value for trigger pattern mask
>>>>>> + * @patt_ts:          Indicates if pattern match for timestamp 
>>>>>> is enabled.
>>>>>> + * @trig_ts:          Indicates if CTI trigger for timestamp is 
>>>>>> enabled.
>>>>>> + * @ts_all:           Indicates if timestamp is enabled for all 
>>>>>> packets.
>>>>>>    */
>>>>>>   struct cmb_dataset {
>>>>>>       u32            trace_mode;
>>>>>> @@ -224,6 +235,9 @@ struct cmb_dataset {
>>>>>>       u32            patt_mask[TPDM_CMB_MAX_PATT];
>>>>>>       u32            trig_patt[TPDM_CMB_MAX_PATT];
>>>>>>       u32            trig_patt_mask[TPDM_CMB_MAX_PATT];
>>>>>> +    bool            patt_ts;
>>>>>> +    bool            trig_ts;
>>>>>> +    bool            ts_all;
>>>>>>   };
>>>>>>     /**
>>>>>
>>>
>>> _______________________________________________
>>> CoreSight mailing list -- coresight@lists.linaro.org
>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
Suzuki K Poulose Dec. 30, 2023, 9:39 a.m. UTC | #7 
On 25/12/2023 01:55, Tao Zhang wrote:
> 
> On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
>> On 20/12/2023 09:51, Tao Zhang wrote:
>>>
>>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>>
>>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>> CMB_TIER register is CMB subunit timestamp insertion enable 
>>>>>>> register.
>>>>>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>>>>>>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB bit.
>>>>>>> Set this bit to 1 to request a timestamp following a CMB CTI 
>>>>>>> timestamp
>>>>>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request timestamp
>>>>>>> for all packets.
>>>>>>>
>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>>>>> ---
>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  35 ++++++
>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 
>>>>>>> +++++++++++++++++-
>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  |  14 +++
>>>>>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git 
>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> index 53662ce7c2d0..e0b77107be13 100644
>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>>>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>   Description:
>>>>>>>           (RW) Set/Get the mask of the pattern for the CMB 
>>>>>>> subunit TPDM.
>>>>>>> +
>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>>>>>> +Date:        September 2023
>>>>>>> +KernelVersion    6.7
>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>> +Description:
>>>>>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>>>>>> +        the pattern timestamp of CMB tpdm.
>>>>>>> +
>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>> +        0 : Disable CMB pattern timestamp.
>>>>>>> +        1 : Enable CMB pattern timestamp.
>>>>>>> +
>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>>>>>> +Date:        September 2023
>>>>>>> +KernelVersion    6.7
>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>> +Description:
>>>>>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>>>>>> +
>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>> +        0 : Set the CMB trigger type to false
>>>>>>> +        1 : Set the CMB trigger type to true
>>>>>>> +
>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>>>>>> +Date:        September 2023
>>>>>>> +KernelVersion    6.7
>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>> +Description:
>>>>>>> +        (RW) Read or write the status of timestamp upon all 
>>>>>>> interface.
>>>>>>> +        Only value 0 and 1  can be written to this node. Set 
>>>>>>> this node to 1 to requeset
>>>>>>> +        timestamp to all trace packet.
>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>> +        0 : Disable the timestamp of all trace packets.
>>>>>>> +        1 : Enable the timestamp of all trace packets.
>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> index 894d4309f1c7..f6cda5616e84 100644
>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct 
>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>>>>   }
>>>>>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>>>>>> +{
>>>>>>> +    u32 val;
>>>>>>> +
>>>>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>>>>>> +
>>>>>>> +    /* Clear all relevant fields */
>>>>>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>>>>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>>>>>> +
>>>>>>> +    /* Set pattern timestamp type and enablement */
>>>>>>> +    if (drvdata->cmb->patt_ts)
>>>>>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>
>>>>>>  -- cut --
>>>>>>> +    else
>>>>>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>
>>>>>>
>>>>>> All the else cases in this function are superfluous. Please remove 
>>>>>> all
>>>>>> of them.
>>>>> I will update this in the next patch.
>>>>>>
>>>>>>> +
>>>>>>> +    /* Set trigger timestamp */
>>>>>>> +    if (drvdata->cmb->trig_ts)
>>>>>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>> +    else
>>>>>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>> +
>>>>>>> +    /* Set all timestamp enablement*/
>>>>>>> +    if (drvdata->cmb->ts_all)
>>>>>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>>>>>> +    else
>>>>>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>>>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>>>> +}
>>>>>>> +
>>>>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>>>>   {
>>>>>>>       u32 val, i;
>>>>>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct 
>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>>>>>       }
>>>>>>>   +    set_cmb_tier(drvdata);
>>>>>>> +
>>>>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>>>>       /*
>>>>>>>        * Set to 0 for continuous CMB collection mode,
>>>>>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct device 
>>>>>>> *dev,
>>>>>>>                     char *buf)
>>>>>>>   {
>>>>>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>> +    ssize_t size = 0;
>>>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>>>> +
>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>>>
>>>>>> Why does this need to show two values ? This must only show ONE 
>>>>>> value.
>>>>>> How you deduce that might be based on the availability of the feature
>>>>>> set. Or store the TS value in the drvdata and use that instead for
>>>>>> controlling CMB/DSB.
>>>>>
>>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
>>>>> separate them
>>>>
>>>> The question really is, do we need fine grained control. i.e.,
>>>>
>>>> enable TS for DSB but not for CMB or vice versa.
>>>>
>>>> I am not an expert on the usage scenario of the same. So, if you/Qcomm
>>>> thinks the users need separate, fine grained control for timestamp
>>>> for the DSB and CMB, then yes, follow your recommendation below.
>>>> i.e., tpdm.../dsb_patt/enable_ts
>>>>
>>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.
>>>>>
>>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>>
>>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>>
>>>>>
>>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>>
>>>>> Is this design appropriate?
>>>>
>>>>
>>>> Otherwise, stick to single enable_ts : which enables the ts for both
>>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for both
>>>> CMB/DSB) 1 : TS enabled for both.
>>>
>>> We have a very special case, such as the TPDM supporting both CMB and
>>>
>>> DSB datasets. Although this case is very rare, it still exists.
>>>
>>> Can I use the data bit to instruct whether timestamp is enabled for 
>>> CMB/DSB or not? For example,
>>>
>>> size = sysfs_emit(buf, "%u\n",
>>>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
>>> drvdata->cmb->patt_ts));
>>>
>>> Thus, this value can instruct the following situations.
>>>
>>> 0 - TS is disabled for both CMB/DSB
>>>
>>> 1 - TS is enabled for CMB
>>>
>>> 2 - TS is enabled for DSB
>>>
>>> 3 - TS is enabled for both
>>>
>>> Is this approach acceptable?
>>>
>>
>> No, please stick to separate controls for TS. Do not complicate
>> the user interface.
>>
>> i.e.,
>> tpdm0/dsb_patt/enable_ts
>> tpdm0/cmb_patt/enable_ts
> 
> We need to be able to control/show dsb and cmb timestamp enablement 
> independently.
> 
> Can we achieve this requirement if we use a sysfs file with the same name?

They are independent and in their respective directory (group) for CMB 
and DSB. What am I missing ?
e.g., if you want to enable TS for DSB, you do :

$ echo 1 > dsb_patt/enable_ts

And that only works for DSB not for CMB.

Suzuki

> 
> I'm not able to find a good way to achieve it. Do you have any 
> suggestions for this?
> 


> 
> Best,
> 
> Tao
> 
>>
>> Suzuki
>>
>>
>>>
>>> Best,
>>>
>>> Tao
>>>
>>>>
>>>> Suzuki
>>>>
>>>>
>>>>>>
>>>>
>>>>>> Also, the sysfs documentation needs update, if this is going to
>>>>>> control the CMB.
>>>>>
>>>>> Sure. I will update the SysFs documentation according to the 
>>>>> modification in the
>>>>>
>>>>> next patch series.
>>>>>
>>>>>
>>>>> Best,
>>>>>
>>>>> Tao
>>>>>
>>>>>>
>>>>>> Suzuki
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +    return size;
>>>>>>>   }
>>>>>>>     /*
>>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct device 
>>>>>>> *dev,
>>>>>>>           return -EINVAL;
>>>>>>>         spin_lock(&drvdata->spinlock);
>>>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>>>> +
>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>>>       spin_unlock(&drvdata->spinlock);
>>>>>>> +
>>>>>>>       return size;
>>>>>>>   }
>>>>>>>   static DEVICE_ATTR_RW(enable_ts);
>>>>>>> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct device 
>>>>>>> *dev,
>>>>>>>   }
>>>>>>>   static DEVICE_ATTR_RW(cmb_mode);
>>>>>>>   +static ssize_t cmb_ts_all_show(struct device *dev,
>>>>>>> +                   struct device_attribute *attr,
>>>>>>> +                   char *buf)
>>>>>>> +{
>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>> +
>>>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>>>> +              (unsigned int)drvdata->cmb->ts_all);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static ssize_t cmb_ts_all_store(struct device *dev,
>>>>>>> +                struct device_attribute *attr,
>>>>>>> +                const char *buf,
>>>>>>> +                size_t size)
>>>>>>> +{
>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>> +    unsigned long val;
>>>>>>> +
>>>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>> +    if (val)
>>>>>>> +        drvdata->cmb->ts_all = true;
>>>>>>> +    else
>>>>>>> +        drvdata->cmb->ts_all = false;
>>>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>>>> +    return size;
>>>>>>> +}
>>>>>>> +static DEVICE_ATTR_RW(cmb_ts_all);
>>>>>>> +
>>>>>>> +static ssize_t cmb_trig_ts_show(struct device *dev,
>>>>>>> +                struct device_attribute *attr,
>>>>>>> +                char *buf)
>>>>>>> +{
>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>> +
>>>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>>>> +              (unsigned int)drvdata->cmb->trig_ts);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static ssize_t cmb_trig_ts_store(struct device *dev,
>>>>>>> +                 struct device_attribute *attr,
>>>>>>> +                 const char *buf,
>>>>>>> +                 size_t size)
>>>>>>> +{
>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>> +    unsigned long val;
>>>>>>> +
>>>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>> +    if (val)
>>>>>>> +        drvdata->cmb->trig_ts = true;
>>>>>>> +    else
>>>>>>> +        drvdata->cmb->trig_ts = false;
>>>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>>>> +    return size;
>>>>>>> +}
>>>>>>> +static DEVICE_ATTR_RW(cmb_trig_ts);
>>>>>>> +
>>>>>>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>>>>>>       &dev_attr_ctrl_idx.attr,
>>>>>>>       &dev_attr_ctrl_val.attr,
>>>>>>> @@ -973,6 +1080,7 @@ static struct attribute 
>>>>>>> *tpdm_cmb_patt_attrs[] = {
>>>>>>>       CMB_PATT_ATTR(1),
>>>>>>>       CMB_PATT_MASK_ATTR(0),
>>>>>>>       CMB_PATT_MASK_ATTR(1),
>>>>>>> +    &dev_attr_enable_ts.attr,
>>>>>>>       NULL,
>>>>>>>   };
>>>>>>>   @@ -985,6 +1093,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>>>>>>     static struct attribute *tpdm_cmb_attrs[] = {
>>>>>>>       &dev_attr_cmb_mode.attr,
>>>>>>> +    &dev_attr_cmb_ts_all.attr,
>>>>>>> +    &dev_attr_cmb_trig_ts.attr,
>>>>>>>       NULL,
>>>>>>>   };
>>>>>>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>>> index e90d008c1cb2..65b7ca6c4077 100644
>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>>> @@ -11,6 +11,8 @@
>>>>>>>     /* CMB Subunit Registers */
>>>>>>>   #define TPDM_CMB_CR        (0xA00)
>>>>>>> +/*CMB subunit timestamp insertion enable register*/
>>>>>>> +#define TPDM_CMB_TIER        (0xA04)
>>>>>>>   /*CMB subunit timestamp pattern registers*/
>>>>>>>   #define TPDM_CMB_TPR(n)        (0xA08 + (n * 4))
>>>>>>>   /*CMB subunit timestamp pattern mask registers*/
>>>>>>> @@ -24,6 +26,12 @@
>>>>>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>>>>>   /* Trace collection mode for CMB subunit */
>>>>>>>   #define TPDM_CMB_CR_MODE    BIT(1)
>>>>>>> +/* Timestamp control for pattern match */
>>>>>>> +#define TPDM_CMB_TIER_PATT_TSENAB    BIT(0)
>>>>>>> +/* CMB CTI timestamp request */
>>>>>>> +#define TPDM_CMB_TIER_XTRIG_TSENAB    BIT(1)
>>>>>>> +/* For timestamp fo all trace */
>>>>>>> +#define TPDM_CMB_TIER_TS_ALL        BIT(2)
>>>>>>>     /*Patten register number*/
>>>>>>>   #define TPDM_CMB_MAX_PATT        2
>>>>>>> @@ -217,6 +225,9 @@ struct dsb_dataset {
>>>>>>>    * @patt_mask:        Save value for pattern mask
>>>>>>>    * @trig_patt:        Save value for trigger pattern
>>>>>>>    * @trig_patt_mask:   Save value for trigger pattern mask
>>>>>>> + * @patt_ts:          Indicates if pattern match for timestamp 
>>>>>>> is enabled.
>>>>>>> + * @trig_ts:          Indicates if CTI trigger for timestamp is 
>>>>>>> enabled.
>>>>>>> + * @ts_all:           Indicates if timestamp is enabled for all 
>>>>>>> packets.
>>>>>>>    */
>>>>>>>   struct cmb_dataset {
>>>>>>>       u32            trace_mode;
>>>>>>> @@ -224,6 +235,9 @@ struct cmb_dataset {
>>>>>>>       u32            patt_mask[TPDM_CMB_MAX_PATT];
>>>>>>>       u32            trig_patt[TPDM_CMB_MAX_PATT];
>>>>>>>       u32            trig_patt_mask[TPDM_CMB_MAX_PATT];
>>>>>>> +    bool            patt_ts;
>>>>>>> +    bool            trig_ts;
>>>>>>> +    bool            ts_all;
>>>>>>>   };
>>>>>>>     /**
>>>>>>
>>>>
>>>> _______________________________________________
>>>> CoreSight mailing list -- coresight@lists.linaro.org
>>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
>>
>> _______________________________________________
>> CoreSight mailing list -- coresight@lists.linaro.org
>> To unsubscribe send an email to coresight-leave@lists.linaro.org
Tao Zhang Jan. 5, 2024, 7:49 a.m. UTC | #8 
On 12/30/2023 5:39 PM, Suzuki K Poulose wrote:
> On 25/12/2023 01:55, Tao Zhang wrote:
>>
>> On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
>>> On 20/12/2023 09:51, Tao Zhang wrote:
>>>>
>>>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>>>
>>>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>>> CMB_TIER register is CMB subunit timestamp insertion enable 
>>>>>>>> register.
>>>>>>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>>>>>>>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB 
>>>>>>>> bit.
>>>>>>>> Set this bit to 1 to request a timestamp following a CMB CTI 
>>>>>>>> timestamp
>>>>>>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request 
>>>>>>>> timestamp
>>>>>>>> for all packets.
>>>>>>>>
>>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>>>>>> ---
>>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  | 35 ++++++
>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 
>>>>>>>> +++++++++++++++++-
>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 14 +++
>>>>>>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git 
>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> index 53662ce7c2d0..e0b77107be13 100644
>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>>>>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, 
>>>>>>>> Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>   Description:
>>>>>>>>           (RW) Set/Get the mask of the pattern for the CMB 
>>>>>>>> subunit TPDM.
>>>>>>>> +
>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>>>>>>> +Date:        September 2023
>>>>>>>> +KernelVersion    6.7
>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>> +Description:
>>>>>>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>>>>>>> +        the pattern timestamp of CMB tpdm.
>>>>>>>> +
>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>> +        0 : Disable CMB pattern timestamp.
>>>>>>>> +        1 : Enable CMB pattern timestamp.
>>>>>>>> +
>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>>>>>>> +Date:        September 2023
>>>>>>>> +KernelVersion    6.7
>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>> +Description:
>>>>>>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>>>>>>> +
>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>> +        0 : Set the CMB trigger type to false
>>>>>>>> +        1 : Set the CMB trigger type to true
>>>>>>>> +
>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>>>>>>> +Date:        September 2023
>>>>>>>> +KernelVersion    6.7
>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>> +Description:
>>>>>>>> +        (RW) Read or write the status of timestamp upon all 
>>>>>>>> interface.
>>>>>>>> +        Only value 0 and 1  can be written to this node. Set 
>>>>>>>> this node to 1 to requeset
>>>>>>>> +        timestamp to all trace packet.
>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>> +        0 : Disable the timestamp of all trace packets.
>>>>>>>> +        1 : Enable the timestamp of all trace packets.
>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> index 894d4309f1c7..f6cda5616e84 100644
>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct 
>>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>>>>>   }
>>>>>>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>>>>>>> +{
>>>>>>>> +    u32 val;
>>>>>>>> +
>>>>>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>>>>>>> +
>>>>>>>> +    /* Clear all relevant fields */
>>>>>>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>>>>>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>>>>>>> +
>>>>>>>> +    /* Set pattern timestamp type and enablement */
>>>>>>>> +    if (drvdata->cmb->patt_ts)
>>>>>>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>>
>>>>>>>  -- cut --
>>>>>>>> +    else
>>>>>>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>>
>>>>>>>
>>>>>>> All the else cases in this function are superfluous. Please 
>>>>>>> remove all
>>>>>>> of them.
>>>>>> I will update this in the next patch.
>>>>>>>
>>>>>>>> +
>>>>>>>> +    /* Set trigger timestamp */
>>>>>>>> +    if (drvdata->cmb->trig_ts)
>>>>>>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>>> +    else
>>>>>>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>>> +
>>>>>>>> +    /* Set all timestamp enablement*/
>>>>>>>> +    if (drvdata->cmb->ts_all)
>>>>>>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>>>>>>> +    else
>>>>>>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>>>>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>>>>>   {
>>>>>>>>       u32 val, i;
>>>>>>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct 
>>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>>>>>>       }
>>>>>>>>   +    set_cmb_tier(drvdata);
>>>>>>>> +
>>>>>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>>>>>       /*
>>>>>>>>        * Set to 0 for continuous CMB collection mode,
>>>>>>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct 
>>>>>>>> device *dev,
>>>>>>>>                     char *buf)
>>>>>>>>   {
>>>>>>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>> +    ssize_t size = 0;
>>>>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>> +
>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>>>>
>>>>>>> Why does this need to show two values ? This must only show ONE 
>>>>>>> value.
>>>>>>> How you deduce that might be based on the availability of the 
>>>>>>> feature
>>>>>>> set. Or store the TS value in the drvdata and use that instead for
>>>>>>> controlling CMB/DSB.
>>>>>>
>>>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
>>>>>> separate them
>>>>>
>>>>> The question really is, do we need fine grained control. i.e.,
>>>>>
>>>>> enable TS for DSB but not for CMB or vice versa.
>>>>>
>>>>> I am not an expert on the usage scenario of the same. So, if 
>>>>> you/Qcomm
>>>>> thinks the users need separate, fine grained control for timestamp
>>>>> for the DSB and CMB, then yes, follow your recommendation below.
>>>>> i.e., tpdm.../dsb_patt/enable_ts
>>>>>
>>>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.
>>>>>>
>>>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>>>
>>>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>>>
>>>>>>
>>>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>>>
>>>>>> Is this design appropriate?
>>>>>
>>>>>
>>>>> Otherwise, stick to single enable_ts : which enables the ts for both
>>>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for both
>>>>> CMB/DSB) 1 : TS enabled for both.
>>>>
>>>> We have a very special case, such as the TPDM supporting both CMB and
>>>>
>>>> DSB datasets. Although this case is very rare, it still exists.
>>>>
>>>> Can I use the data bit to instruct whether timestamp is enabled for 
>>>> CMB/DSB or not? For example,
>>>>
>>>> size = sysfs_emit(buf, "%u\n",
>>>>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
>>>> drvdata->cmb->patt_ts));
>>>>
>>>> Thus, this value can instruct the following situations.
>>>>
>>>> 0 - TS is disabled for both CMB/DSB
>>>>
>>>> 1 - TS is enabled for CMB
>>>>
>>>> 2 - TS is enabled for DSB
>>>>
>>>> 3 - TS is enabled for both
>>>>
>>>> Is this approach acceptable?
>>>>
>>>
>>> No, please stick to separate controls for TS. Do not complicate
>>> the user interface.
>>>
>>> i.e.,
>>> tpdm0/dsb_patt/enable_ts
>>> tpdm0/cmb_patt/enable_ts
>>
>> We need to be able to control/show dsb and cmb timestamp enablement 
>> independently.
>>
>> Can we achieve this requirement if we use a sysfs file with the same 
>> name?
>
> They are independent and in their respective directory (group) for CMB 
> and DSB. What am I missing ?
> e.g., if you want to enable TS for DSB, you do :
>
> $ echo 1 > dsb_patt/enable_ts
>
> And that only works for DSB not for CMB.

We have a special case that the TPDM supports both DSB and CMB dataset. 
In this special case, when we

issue this command to enable timestamp, it will call enable_ts_store 
API, right?

     if (tpdm_has_dsb_dataset(drvdata))
         drvdata->dsb->patt_ts = !!val;

     if (tpdm_has_cmb_dataset(drvdata))
         drvdata->cmb->patt_ts = !!val;

Since this special TPDM supports both DSB and CMB dataset, both DSB 
patt_ts and CMB patt_ts will be set

in this case even if I only configure the file in the DSB directory, right?

This is the problem we have now.


Best,

Tao

>
> Suzuki
>
>>
>> I'm not able to find a good way to achieve it. Do you have any 
>> suggestions for this?
>>
>
>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>>>
>>>>
>>>> Best,
>>>>
>>>> Tao
>>>>
>>>>>
>>>>> Suzuki
>>>>>
>>>>>
>>>>>>>
>>>>>
>>>>>>> Also, the sysfs documentation needs update, if this is going to
>>>>>>> control the CMB.
>>>>>>
>>>>>> Sure. I will update the SysFs documentation according to the 
>>>>>> modification in the
>>>>>>
>>>>>> next patch series.
>>>>>>
>>>>>>
>>>>>> Best,
>>>>>>
>>>>>> Tao
>>>>>>
>>>>>>>
>>>>>>> Suzuki
>>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +    return size;
>>>>>>>>   }
>>>>>>>>     /*
>>>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct 
>>>>>>>> device *dev,
>>>>>>>>           return -EINVAL;
>>>>>>>>         spin_lock(&drvdata->spinlock);
>>>>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>>>>> +
>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>>>>       spin_unlock(&drvdata->spinlock);
>>>>>>>> +
>>>>>>>>       return size;
>>>>>>>>   }
>>>>>>>>   static DEVICE_ATTR_RW(enable_ts);
>>>>>>>> @@ -851,6 +896,68 @@ static ssize_t cmb_mode_store(struct 
>>>>>>>> device *dev,
>>>>>>>>   }
>>>>>>>>   static DEVICE_ATTR_RW(cmb_mode);
>>>>>>>>   +static ssize_t cmb_ts_all_show(struct device *dev,
>>>>>>>> +                   struct device_attribute *attr,
>>>>>>>> +                   char *buf)
>>>>>>>> +{
>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>> +
>>>>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>>>>> +              (unsigned int)drvdata->cmb->ts_all);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static ssize_t cmb_ts_all_store(struct device *dev,
>>>>>>>> +                struct device_attribute *attr,
>>>>>>>> +                const char *buf,
>>>>>>>> +                size_t size)
>>>>>>>> +{
>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>> +    unsigned long val;
>>>>>>>> +
>>>>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>>>>> +        return -EINVAL;
>>>>>>>> +
>>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>>> +    if (val)
>>>>>>>> +        drvdata->cmb->ts_all = true;
>>>>>>>> +    else
>>>>>>>> +        drvdata->cmb->ts_all = false;
>>>>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>>>>> +    return size;
>>>>>>>> +}
>>>>>>>> +static DEVICE_ATTR_RW(cmb_ts_all);
>>>>>>>> +
>>>>>>>> +static ssize_t cmb_trig_ts_show(struct device *dev,
>>>>>>>> +                struct device_attribute *attr,
>>>>>>>> +                char *buf)
>>>>>>>> +{
>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>> +
>>>>>>>> +    return sysfs_emit(buf, "%u\n",
>>>>>>>> +              (unsigned int)drvdata->cmb->trig_ts);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static ssize_t cmb_trig_ts_store(struct device *dev,
>>>>>>>> +                 struct device_attribute *attr,
>>>>>>>> +                 const char *buf,
>>>>>>>> +                 size_t size)
>>>>>>>> +{
>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>> +    unsigned long val;
>>>>>>>> +
>>>>>>>> +    if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>>>>>>>> +        return -EINVAL;
>>>>>>>> +
>>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>>> +    if (val)
>>>>>>>> +        drvdata->cmb->trig_ts = true;
>>>>>>>> +    else
>>>>>>>> +        drvdata->cmb->trig_ts = false;
>>>>>>>> +    spin_unlock(&drvdata->spinlock);
>>>>>>>> +    return size;
>>>>>>>> +}
>>>>>>>> +static DEVICE_ATTR_RW(cmb_trig_ts);
>>>>>>>> +
>>>>>>>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>>>>>>>       &dev_attr_ctrl_idx.attr,
>>>>>>>>       &dev_attr_ctrl_val.attr,
>>>>>>>> @@ -973,6 +1080,7 @@ static struct attribute 
>>>>>>>> *tpdm_cmb_patt_attrs[] = {
>>>>>>>>       CMB_PATT_ATTR(1),
>>>>>>>>       CMB_PATT_MASK_ATTR(0),
>>>>>>>>       CMB_PATT_MASK_ATTR(1),
>>>>>>>> +    &dev_attr_enable_ts.attr,
>>>>>>>>       NULL,
>>>>>>>>   };
>>>>>>>>   @@ -985,6 +1093,8 @@ static struct attribute 
>>>>>>>> *tpdm_dsb_attrs[] = {
>>>>>>>>     static struct attribute *tpdm_cmb_attrs[] = {
>>>>>>>>       &dev_attr_cmb_mode.attr,
>>>>>>>> +    &dev_attr_cmb_ts_all.attr,
>>>>>>>> +    &dev_attr_cmb_trig_ts.attr,
>>>>>>>>       NULL,
>>>>>>>>   };
>>>>>>>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>>>> index e90d008c1cb2..65b7ca6c4077 100644
>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>>>>> @@ -11,6 +11,8 @@
>>>>>>>>     /* CMB Subunit Registers */
>>>>>>>>   #define TPDM_CMB_CR        (0xA00)
>>>>>>>> +/*CMB subunit timestamp insertion enable register*/
>>>>>>>> +#define TPDM_CMB_TIER        (0xA04)
>>>>>>>>   /*CMB subunit timestamp pattern registers*/
>>>>>>>>   #define TPDM_CMB_TPR(n)        (0xA08 + (n * 4))
>>>>>>>>   /*CMB subunit timestamp pattern mask registers*/
>>>>>>>> @@ -24,6 +26,12 @@
>>>>>>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>>>>>>   /* Trace collection mode for CMB subunit */
>>>>>>>>   #define TPDM_CMB_CR_MODE    BIT(1)
>>>>>>>> +/* Timestamp control for pattern match */
>>>>>>>> +#define TPDM_CMB_TIER_PATT_TSENAB    BIT(0)
>>>>>>>> +/* CMB CTI timestamp request */
>>>>>>>> +#define TPDM_CMB_TIER_XTRIG_TSENAB    BIT(1)
>>>>>>>> +/* For timestamp fo all trace */
>>>>>>>> +#define TPDM_CMB_TIER_TS_ALL        BIT(2)
>>>>>>>>     /*Patten register number*/
>>>>>>>>   #define TPDM_CMB_MAX_PATT        2
>>>>>>>> @@ -217,6 +225,9 @@ struct dsb_dataset {
>>>>>>>>    * @patt_mask:        Save value for pattern mask
>>>>>>>>    * @trig_patt:        Save value for trigger pattern
>>>>>>>>    * @trig_patt_mask:   Save value for trigger pattern mask
>>>>>>>> + * @patt_ts:          Indicates if pattern match for timestamp 
>>>>>>>> is enabled.
>>>>>>>> + * @trig_ts:          Indicates if CTI trigger for timestamp 
>>>>>>>> is enabled.
>>>>>>>> + * @ts_all:           Indicates if timestamp is enabled for 
>>>>>>>> all packets.
>>>>>>>>    */
>>>>>>>>   struct cmb_dataset {
>>>>>>>>       u32            trace_mode;
>>>>>>>> @@ -224,6 +235,9 @@ struct cmb_dataset {
>>>>>>>>       u32            patt_mask[TPDM_CMB_MAX_PATT];
>>>>>>>>       u32            trig_patt[TPDM_CMB_MAX_PATT];
>>>>>>>>       u32 trig_patt_mask[TPDM_CMB_MAX_PATT];
>>>>>>>> +    bool            patt_ts;
>>>>>>>> +    bool            trig_ts;
>>>>>>>> +    bool            ts_all;
>>>>>>>>   };
>>>>>>>>     /**
>>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> CoreSight mailing list -- coresight@lists.linaro.org
>>>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
>>>
>>> _______________________________________________
>>> CoreSight mailing list -- coresight@lists.linaro.org
>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
Suzuki K Poulose Jan. 8, 2024, 10:42 a.m. UTC | #9 
On 05/01/2024 07:49, Tao Zhang wrote:
> 
> On 12/30/2023 5:39 PM, Suzuki K Poulose wrote:
>> On 25/12/2023 01:55, Tao Zhang wrote:
>>>
>>> On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
>>>> On 20/12/2023 09:51, Tao Zhang wrote:
>>>>>
>>>>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>>>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>>>> CMB_TIER register is CMB subunit timestamp insertion enable 
>>>>>>>>> register.
>>>>>>>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a timestamp
>>>>>>>>> following a CMB interface pattern match. Bit 1 is XTRIG_TSENAB 
>>>>>>>>> bit.
>>>>>>>>> Set this bit to 1 to request a timestamp following a CMB CTI 
>>>>>>>>> timestamp
>>>>>>>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request 
>>>>>>>>> timestamp
>>>>>>>>> for all packets.
>>>>>>>>>
>>>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>>>>>>> ---
>>>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  | 35 ++++++
>>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 116 
>>>>>>>>> +++++++++++++++++-
>>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 14 +++
>>>>>>>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git 
>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> index 53662ce7c2d0..e0b77107be13 100644
>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>>>>>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, 
>>>>>>>>> Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>>   Description:
>>>>>>>>>           (RW) Set/Get the mask of the pattern for the CMB 
>>>>>>>>> subunit TPDM.
>>>>>>>>> +
>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>>>>>>>> +Date:        September 2023
>>>>>>>>> +KernelVersion    6.7
>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>> +Description:
>>>>>>>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>>>>>>>> +        the pattern timestamp of CMB tpdm.
>>>>>>>>> +
>>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>>> +        0 : Disable CMB pattern timestamp.
>>>>>>>>> +        1 : Enable CMB pattern timestamp.
>>>>>>>>> +
>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>>>>>>>> +Date:        September 2023
>>>>>>>>> +KernelVersion    6.7
>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>> +Description:
>>>>>>>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>>>>>>>> +
>>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>>> +        0 : Set the CMB trigger type to false
>>>>>>>>> +        1 : Set the CMB trigger type to true
>>>>>>>>> +
>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>>>>>>>> +Date:        September 2023
>>>>>>>>> +KernelVersion    6.7
>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>> +Description:
>>>>>>>>> +        (RW) Read or write the status of timestamp upon all 
>>>>>>>>> interface.
>>>>>>>>> +        Only value 0 and 1  can be written to this node. Set 
>>>>>>>>> this node to 1 to requeset
>>>>>>>>> +        timestamp to all trace packet.
>>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>>> +        0 : Disable the timestamp of all trace packets.
>>>>>>>>> +        1 : Enable the timestamp of all trace packets.
>>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> index 894d4309f1c7..f6cda5616e84 100644
>>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct 
>>>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>>>>>>   }
>>>>>>>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>>>>>>>> +{
>>>>>>>>> +    u32 val;
>>>>>>>>> +
>>>>>>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>>>>>>>> +
>>>>>>>>> +    /* Clear all relevant fields */
>>>>>>>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>>>>>>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>>>>>>>> +
>>>>>>>>> +    /* Set pattern timestamp type and enablement */
>>>>>>>>> +    if (drvdata->cmb->patt_ts)
>>>>>>>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>>>
>>>>>>>>  -- cut --
>>>>>>>>> +    else
>>>>>>>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>>>
>>>>>>>>
>>>>>>>> All the else cases in this function are superfluous. Please 
>>>>>>>> remove all
>>>>>>>> of them.
>>>>>>> I will update this in the next patch.
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +    /* Set trigger timestamp */
>>>>>>>>> +    if (drvdata->cmb->trig_ts)
>>>>>>>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>>>> +    else
>>>>>>>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>>>> +
>>>>>>>>> +    /* Set all timestamp enablement*/
>>>>>>>>> +    if (drvdata->cmb->ts_all)
>>>>>>>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>>>>>>>> +    else
>>>>>>>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>>>>>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>>>>>>   {
>>>>>>>>>       u32 val, i;
>>>>>>>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct 
>>>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>>>>>>>       }
>>>>>>>>>   +    set_cmb_tier(drvdata);
>>>>>>>>> +
>>>>>>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>>>>>>       /*
>>>>>>>>>        * Set to 0 for continuous CMB collection mode,
>>>>>>>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct 
>>>>>>>>> device *dev,
>>>>>>>>>                     char *buf)
>>>>>>>>>   {
>>>>>>>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>> +
>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>>>>>
>>>>>>>> Why does this need to show two values ? This must only show ONE 
>>>>>>>> value.
>>>>>>>> How you deduce that might be based on the availability of the 
>>>>>>>> feature
>>>>>>>> set. Or store the TS value in the drvdata and use that instead for
>>>>>>>> controlling CMB/DSB.
>>>>>>>
>>>>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can I 
>>>>>>> separate them
>>>>>>
>>>>>> The question really is, do we need fine grained control. i.e.,
>>>>>>
>>>>>> enable TS for DSB but not for CMB or vice versa.
>>>>>>
>>>>>> I am not an expert on the usage scenario of the same. So, if 
>>>>>> you/Qcomm
>>>>>> thinks the users need separate, fine grained control for timestamp
>>>>>> for the DSB and CMB, then yes, follow your recommendation below.
>>>>>> i.e., tpdm.../dsb_patt/enable_ts
>>>>>>
>>>>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like below.
>>>>>>>
>>>>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>>>>
>>>>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>>>>
>>>>>>>
>>>>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>>>>
>>>>>>> Is this design appropriate?
>>>>>>
>>>>>>
>>>>>> Otherwise, stick to single enable_ts : which enables the ts for both
>>>>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for both
>>>>>> CMB/DSB) 1 : TS enabled for both.
>>>>>
>>>>> We have a very special case, such as the TPDM supporting both CMB and
>>>>>
>>>>> DSB datasets. Although this case is very rare, it still exists.
>>>>>
>>>>> Can I use the data bit to instruct whether timestamp is enabled for 
>>>>> CMB/DSB or not? For example,
>>>>>
>>>>> size = sysfs_emit(buf, "%u\n",
>>>>>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
>>>>> drvdata->cmb->patt_ts));
>>>>>
>>>>> Thus, this value can instruct the following situations.
>>>>>
>>>>> 0 - TS is disabled for both CMB/DSB
>>>>>
>>>>> 1 - TS is enabled for CMB
>>>>>
>>>>> 2 - TS is enabled for DSB
>>>>>
>>>>> 3 - TS is enabled for both
>>>>>
>>>>> Is this approach acceptable?
>>>>>
>>>>
>>>> No, please stick to separate controls for TS. Do not complicate
>>>> the user interface.
>>>>
>>>> i.e.,
>>>> tpdm0/dsb_patt/enable_ts
>>>> tpdm0/cmb_patt/enable_ts
>>>
>>> We need to be able to control/show dsb and cmb timestamp enablement 
>>> independently.
>>>
>>> Can we achieve this requirement if we use a sysfs file with the same 
>>> name?
>>
>> They are independent and in their respective directory (group) for CMB 
>> and DSB. What am I missing ?
>> e.g., if you want to enable TS for DSB, you do :
>>
>> $ echo 1 > dsb_patt/enable_ts
>>
>> And that only works for DSB not for CMB.
> 
> We have a special case that the TPDM supports both DSB and CMB dataset. 
> In this special case, when we
> 
> issue this command to enable timestamp, it will call enable_ts_store 
> API, right?
> 
>      if (tpdm_has_dsb_dataset(drvdata))
>          drvdata->dsb->patt_ts = !!val;
> 
>      if (tpdm_has_cmb_dataset(drvdata))
>          drvdata->cmb->patt_ts = !!val;

I don't understand. If they both are under different subgroups, why
should they be conflicting ? Are you not able to distinguish them, when
  you creat those attributes ? i.e., create two different "attributes" ?

See below.

> Since this special TPDM supports both DSB and CMB dataset, both DSB 
> patt_ts and CMB patt_ts will be set
> 
> in this case even if I only configure the file in the DSB directory, right?
> 
> This is the problem we have now.
> 


>>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +    return size;
>>>>>>>>>   }
>>>>>>>>>     /*
>>>>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct 
>>>>>>>>> device *dev,
>>>>>>>>>           return -EINVAL;
>>>>>>>>>         spin_lock(&drvdata->spinlock);
>>>>>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>>>>>> +
>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>>>>>       spin_unlock(&drvdata->spinlock);
>>>>>>>>> +
>>>>>>>>>       return size;
>>>>>>>>>   }
>>>>>>>>>   static DEVICE_ATTR_RW(enable_ts);

Do not overload the same for both DSB and CMB. Create one for each in 
DSB and CMB ? They could share the same show/store routines, but could
be done with additional variable to indicate which attribute they are 
controlling. Like the other attributes, using dev_ext_attribute or such.


Suzuki
Tao Zhang Jan. 12, 2024, 2:42 a.m. UTC | #10 
On 1/8/2024 6:42 PM, Suzuki K Poulose wrote:
> On 05/01/2024 07:49, Tao Zhang wrote:
>>
>> On 12/30/2023 5:39 PM, Suzuki K Poulose wrote:
>>> On 25/12/2023 01:55, Tao Zhang wrote:
>>>>
>>>> On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
>>>>> On 20/12/2023 09:51, Tao Zhang wrote:
>>>>>>
>>>>>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>>>>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>>>>>
>>>>>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>>>>> CMB_TIER register is CMB subunit timestamp insertion enable 
>>>>>>>>>> register.
>>>>>>>>>> Bit 0 is PATT_TSENAB bit. Set this bit to 1 to request a 
>>>>>>>>>> timestamp
>>>>>>>>>> following a CMB interface pattern match. Bit 1 is 
>>>>>>>>>> XTRIG_TSENAB bit.
>>>>>>>>>> Set this bit to 1 to request a timestamp following a CMB CTI 
>>>>>>>>>> timestamp
>>>>>>>>>> request. Bit 2 is TS_ALL bit. Set this bit to 1 to request 
>>>>>>>>>> timestamp
>>>>>>>>>> for all packets.
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm | 35 ++++++
>>>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c | 116 
>>>>>>>>>> +++++++++++++++++-
>>>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h | 14 +++
>>>>>>>>>>   3 files changed, 162 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git 
>>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>> index 53662ce7c2d0..e0b77107be13 100644
>>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>> @@ -214,3 +214,38 @@ KernelVersion    6.7
>>>>>>>>>>   Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, 
>>>>>>>>>> Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>>>   Description:
>>>>>>>>>>           (RW) Set/Get the mask of the pattern for the CMB 
>>>>>>>>>> subunit TPDM.
>>>>>>>>>> +
>>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
>>>>>>>>>> +Date:        September 2023
>>>>>>>>>> +KernelVersion    6.7
>>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, 
>>>>>>>>>> Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>>> +Description:
>>>>>>>>>> +        (Write) Set the pattern timestamp of CMB tpdm. Read
>>>>>>>>>> +        the pattern timestamp of CMB tpdm.
>>>>>>>>>> +
>>>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>>>> +        0 : Disable CMB pattern timestamp.
>>>>>>>>>> +        1 : Enable CMB pattern timestamp.
>>>>>>>>>> +
>>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
>>>>>>>>>> +Date:        September 2023
>>>>>>>>>> +KernelVersion    6.7
>>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, 
>>>>>>>>>> Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>>> +Description:
>>>>>>>>>> +        (RW) Set/Get the trigger timestamp of the CMB for tpdm.
>>>>>>>>>> +
>>>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>>>> +        0 : Set the CMB trigger type to false
>>>>>>>>>> +        1 : Set the CMB trigger type to true
>>>>>>>>>> +
>>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
>>>>>>>>>> +Date:        September 2023
>>>>>>>>>> +KernelVersion    6.7
>>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, 
>>>>>>>>>> Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>>> +Description:
>>>>>>>>>> +        (RW) Read or write the status of timestamp upon all 
>>>>>>>>>> interface.
>>>>>>>>>> +        Only value 0 and 1  can be written to this node. Set 
>>>>>>>>>> this node to 1 to requeset
>>>>>>>>>> +        timestamp to all trace packet.
>>>>>>>>>> +        Accepts only one of the 2 values -  0 or 1.
>>>>>>>>>> +        0 : Disable the timestamp of all trace packets.
>>>>>>>>>> +        1 : Enable the timestamp of all trace packets.
>>>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>>> index 894d4309f1c7..f6cda5616e84 100644
>>>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>>> @@ -331,6 +331,36 @@ static void tpdm_enable_dsb(struct 
>>>>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>>>>>>>   }
>>>>>>>>>>   +static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>>>>>>>>>> +{
>>>>>>>>>> +    u32 val;
>>>>>>>>>> +
>>>>>>>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
>>>>>>>>>> +
>>>>>>>>>> +    /* Clear all relevant fields */
>>>>>>>>>> +    val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
>>>>>>>>>> +         TPDM_CMB_TIER_XTRIG_TSENAB);
>>>>>>>>>> +
>>>>>>>>>> +    /* Set pattern timestamp type and enablement */
>>>>>>>>>> +    if (drvdata->cmb->patt_ts)
>>>>>>>>>> +        val |= TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>>>>
>>>>>>>>>  -- cut --
>>>>>>>>>> +    else
>>>>>>>>>> +        val &= ~TPDM_CMB_TIER_PATT_TSENAB;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> All the else cases in this function are superfluous. Please 
>>>>>>>>> remove all
>>>>>>>>> of them.
>>>>>>>> I will update this in the next patch.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +    /* Set trigger timestamp */
>>>>>>>>>> +    if (drvdata->cmb->trig_ts)
>>>>>>>>>> +        val |= TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>>>>> +    else
>>>>>>>>>> +        val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
>>>>>>>>>> +
>>>>>>>>>> +    /* Set all timestamp enablement*/
>>>>>>>>>> +    if (drvdata->cmb->ts_all)
>>>>>>>>>> +        val |= TPDM_CMB_TIER_TS_ALL;
>>>>>>>>>> +    else
>>>>>>>>>> +        val &= ~TPDM_CMB_TIER_TS_ALL;
>>>>>>>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>>>>>>>   {
>>>>>>>>>>       u32 val, i;
>>>>>>>>>> @@ -347,6 +377,8 @@ static void tpdm_enable_cmb(struct 
>>>>>>>>>> tpdm_drvdata *drvdata)
>>>>>>>>>>                   drvdata->base + TPDM_CMB_XPMR(i));
>>>>>>>>>>       }
>>>>>>>>>>   +    set_cmb_tier(drvdata);
>>>>>>>>>> +
>>>>>>>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>>>>>>>       /*
>>>>>>>>>>        * Set to 0 for continuous CMB collection mode,
>>>>>>>>>> @@ -695,9 +727,17 @@ static ssize_t enable_ts_show(struct 
>>>>>>>>>> device *dev,
>>>>>>>>>>                     char *buf)
>>>>>>>>>>   {
>>>>>>>>>>       struct tpdm_drvdata *drvdata = 
>>>>>>>>>> dev_get_drvdata(dev->parent);
>>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>>>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>> +
>>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>>>>>>
>>>>>>>>> Why does this need to show two values ? This must only show 
>>>>>>>>> ONE value.
>>>>>>>>> How you deduce that might be based on the availability of the 
>>>>>>>>> feature
>>>>>>>>> set. Or store the TS value in the drvdata and use that instead 
>>>>>>>>> for
>>>>>>>>> controlling CMB/DSB.
>>>>>>>>
>>>>>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can 
>>>>>>>> I separate them
>>>>>>>
>>>>>>> The question really is, do we need fine grained control. i.e.,
>>>>>>>
>>>>>>> enable TS for DSB but not for CMB or vice versa.
>>>>>>>
>>>>>>> I am not an expert on the usage scenario of the same. So, if 
>>>>>>> you/Qcomm
>>>>>>> thinks the users need separate, fine grained control for timestamp
>>>>>>> for the DSB and CMB, then yes, follow your recommendation below.
>>>>>>> i.e., tpdm.../dsb_patt/enable_ts
>>>>>>>
>>>>>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like 
>>>>>>>> below.
>>>>>>>>
>>>>>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>>>>>
>>>>>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>>>>>
>>>>>>>>
>>>>>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>>>>>
>>>>>>>> Is this design appropriate?
>>>>>>>
>>>>>>>
>>>>>>> Otherwise, stick to single enable_ts : which enables the ts for 
>>>>>>> both
>>>>>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for 
>>>>>>> both
>>>>>>> CMB/DSB) 1 : TS enabled for both.
>>>>>>
>>>>>> We have a very special case, such as the TPDM supporting both CMB 
>>>>>> and
>>>>>>
>>>>>> DSB datasets. Although this case is very rare, it still exists.
>>>>>>
>>>>>> Can I use the data bit to instruct whether timestamp is enabled 
>>>>>> for CMB/DSB or not? For example,
>>>>>>
>>>>>> size = sysfs_emit(buf, "%u\n",
>>>>>>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
>>>>>> drvdata->cmb->patt_ts));
>>>>>>
>>>>>> Thus, this value can instruct the following situations.
>>>>>>
>>>>>> 0 - TS is disabled for both CMB/DSB
>>>>>>
>>>>>> 1 - TS is enabled for CMB
>>>>>>
>>>>>> 2 - TS is enabled for DSB
>>>>>>
>>>>>> 3 - TS is enabled for both
>>>>>>
>>>>>> Is this approach acceptable?
>>>>>>
>>>>>
>>>>> No, please stick to separate controls for TS. Do not complicate
>>>>> the user interface.
>>>>>
>>>>> i.e.,
>>>>> tpdm0/dsb_patt/enable_ts
>>>>> tpdm0/cmb_patt/enable_ts
>>>>
>>>> We need to be able to control/show dsb and cmb timestamp enablement 
>>>> independently.
>>>>
>>>> Can we achieve this requirement if we use a sysfs file with the 
>>>> same name?
>>>
>>> They are independent and in their respective directory (group) for 
>>> CMB and DSB. What am I missing ?
>>> e.g., if you want to enable TS for DSB, you do :
>>>
>>> $ echo 1 > dsb_patt/enable_ts
>>>
>>> And that only works for DSB not for CMB.
>>
>> We have a special case that the TPDM supports both DSB and CMB 
>> dataset. In this special case, when we
>>
>> issue this command to enable timestamp, it will call enable_ts_store 
>> API, right?
>>
>>      if (tpdm_has_dsb_dataset(drvdata))
>>          drvdata->dsb->patt_ts = !!val;
>>
>>      if (tpdm_has_cmb_dataset(drvdata))
>>          drvdata->cmb->patt_ts = !!val;
>
> I don't understand. If they both are under different subgroups, why
> should they be conflicting ? Are you not able to distinguish them, when
>  you creat those attributes ? i.e., create two different "attributes" ?

Yes, although some TPDMs can support both CMB dataset and DSB dataset, 
we need to configure them separately

in some scenarios. Based on your suggestion, I want to use the following 
approach to implement it.

See below.

>
> See below.
>
>> Since this special TPDM supports both DSB and CMB dataset, both DSB 
>> patt_ts and CMB patt_ts will be set
>>
>> in this case even if I only configure the file in the DSB directory, 
>> right?
>>
>> This is the problem we have now.
>>
>
>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +    return size;
>>>>>>>>>>   }
>>>>>>>>>>     /*
>>>>>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct 
>>>>>>>>>> device *dev,
>>>>>>>>>>           return -EINVAL;
>>>>>>>>>>         spin_lock(&drvdata->spinlock);
>>>>>>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>>>>>>> +
>>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>>>>>>       spin_unlock(&drvdata->spinlock);
>>>>>>>>>> +
>>>>>>>>>>       return size;
>>>>>>>>>>   }
>>>>>>>>>>   static DEVICE_ATTR_RW(enable_ts);
>
> Do not overload the same for both DSB and CMB. Create one for each in 
> DSB and CMB ? They could share the same show/store routines, but could
> be done with additional variable to indicate which attribute they are 
> controlling. Like the other attributes, using dev_ext_attribute or such.

New approach below, please help review to see if it is acceptable?

#define tpdm_patt_enable_ts_rw(name, mem)                \
     (&((struct tpdm_dataset_attribute[]) {            \
        {                                \
         __ATTR(name, 0644, enable_ts_show,        \
         enable_ts_store),        \
         mem,                            \
        }                                \
     })[0].attr.attr)


#define DSB_PATT_ENABLE_TS                    \
         tpdm_patt_enable_ts_rw(enable_ts,        \
         DSB_PATT)


#define CMB_PATT_ENABLE_TS                    \
         tpdm_patt_enable_ts_rw(enable_ts,        \
         CMB_PATT)


static ssize_t enable_ts_show(struct device *dev,
                   struct device_attribute *attr,
                   char *buf)
{
     struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
     struct tpdm_dataset_attribute *tpdm_attr =
         container_of(attr, struct tpdm_dataset_attribute, attr);
     ssize_t size = 0;

     if (tpdm_attr->mem == DSB_PATT) {
         size = sysfs_emit(buf, "%u\n",
                  (unsigned int)drvdata->dsb->patt_ts);
     } else if (tpdm_attr->mem == CMB_PATT) {
         size = sysfs_emit(buf, "%u\n",
                 (unsigned int)drvdata->cmb->patt_ts);
     } else
         return -EINVAL;

     return size;
}

static ssize_t enable_ts_store(struct device *dev,
                    struct device_attribute *attr,
                    const char *buf,
                    size_t size)
{
     struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
     struct tpdm_dataset_attribute *tpdm_attr =
         container_of(attr, struct tpdm_dataset_attribute, attr);
     unsigned long val;

     if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
         return -EINVAL;

     spin_lock(&drvdata->spinlock);
     if (tpdm_attr->mem == DSB_PATT) {
         drvdata->dsb->patt_ts = !!val;
     } else if (tpdm_attr->mem == CMB_PATT) {
         drvdata->cmb->patt_ts = !!val;
     } else
         return -EINVAL;
     spin_unlock(&drvdata->spinlock);

     return size;
}


Best,

Tao

>
>
> Suzuki
>
Suzuki K Poulose Jan. 12, 2024, 9:29 a.m. UTC | #11 
On 12/01/2024 02:42, Tao Zhang wrote:
> 
> On 1/8/2024 6:42 PM, Suzuki K Poulose wrote:
>> On 05/01/2024 07:49, Tao Zhang wrote:
>>>
>>> On 12/30/2023 5:39 PM, Suzuki K Poulose wrote:
>>>> On 25/12/2023 01:55, Tao Zhang wrote:
>>>>>
>>>>> On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
>>>>>> On 20/12/2023 09:51, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>>>>>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>>>>>>
>>>>>>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:

..

>>>>>>>>>>>                     char *buf)
>>>>>>>>>>>   {
>>>>>>>>>>>       struct tpdm_drvdata *drvdata = 
>>>>>>>>>>> dev_get_drvdata(dev->parent);
>>>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>>>>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>>> +
>>>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>>>>>>>
>>>>>>>>>> Why does this need to show two values ? This must only show 
>>>>>>>>>> ONE value.
>>>>>>>>>> How you deduce that might be based on the availability of the 
>>>>>>>>>> feature
>>>>>>>>>> set. Or store the TS value in the drvdata and use that instead 
>>>>>>>>>> for
>>>>>>>>>> controlling CMB/DSB.
>>>>>>>>>
>>>>>>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can 
>>>>>>>>> I separate them
>>>>>>>>
>>>>>>>> The question really is, do we need fine grained control. i.e.,
>>>>>>>>
>>>>>>>> enable TS for DSB but not for CMB or vice versa.
>>>>>>>>
>>>>>>>> I am not an expert on the usage scenario of the same. So, if 
>>>>>>>> you/Qcomm
>>>>>>>> thinks the users need separate, fine grained control for timestamp
>>>>>>>> for the DSB and CMB, then yes, follow your recommendation below.
>>>>>>>> i.e., tpdm.../dsb_patt/enable_ts
>>>>>>>>
>>>>>>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like 
>>>>>>>>> below.
>>>>>>>>>
>>>>>>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>>>>>>
>>>>>>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>>>>>>
>>>>>>>>>
>>>>>>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>>>>>>
>>>>>>>>> Is this design appropriate?
>>>>>>>>
>>>>>>>>
>>>>>>>> Otherwise, stick to single enable_ts : which enables the ts for 
>>>>>>>> both
>>>>>>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for 
>>>>>>>> both
>>>>>>>> CMB/DSB) 1 : TS enabled for both.
>>>>>>>
>>>>>>> We have a very special case, such as the TPDM supporting both CMB 
>>>>>>> and
>>>>>>>
>>>>>>> DSB datasets. Although this case is very rare, it still exists.
>>>>>>>
>>>>>>> Can I use the data bit to instruct whether timestamp is enabled 
>>>>>>> for CMB/DSB or not? For example,
>>>>>>>
>>>>>>> size = sysfs_emit(buf, "%u\n",
>>>>>>>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
>>>>>>> drvdata->cmb->patt_ts));
>>>>>>>
>>>>>>> Thus, this value can instruct the following situations.
>>>>>>>
>>>>>>> 0 - TS is disabled for both CMB/DSB
>>>>>>>
>>>>>>> 1 - TS is enabled for CMB
>>>>>>>
>>>>>>> 2 - TS is enabled for DSB
>>>>>>>
>>>>>>> 3 - TS is enabled for both
>>>>>>>
>>>>>>> Is this approach acceptable?
>>>>>>>
>>>>>>
>>>>>> No, please stick to separate controls for TS. Do not complicate
>>>>>> the user interface.
>>>>>>
>>>>>> i.e.,
>>>>>> tpdm0/dsb_patt/enable_ts
>>>>>> tpdm0/cmb_patt/enable_ts
>>>>>
>>>>> We need to be able to control/show dsb and cmb timestamp enablement 
>>>>> independently.
>>>>>
>>>>> Can we achieve this requirement if we use a sysfs file with the 
>>>>> same name?
>>>>
>>>> They are independent and in their respective directory (group) for 
>>>> CMB and DSB. What am I missing ?
>>>> e.g., if you want to enable TS for DSB, you do :
>>>>
>>>> $ echo 1 > dsb_patt/enable_ts
>>>>
>>>> And that only works for DSB not for CMB.
>>>
>>> We have a special case that the TPDM supports both DSB and CMB 
>>> dataset. In this special case, when we
>>>
>>> issue this command to enable timestamp, it will call enable_ts_store 
>>> API, right?
>>>
>>>      if (tpdm_has_dsb_dataset(drvdata))
>>>          drvdata->dsb->patt_ts = !!val;
>>>
>>>      if (tpdm_has_cmb_dataset(drvdata))
>>>          drvdata->cmb->patt_ts = !!val;
>>
>> I don't understand. If they both are under different subgroups, why
>> should they be conflicting ? Are you not able to distinguish them, when
>>  you creat those attributes ? i.e., create two different "attributes" ?
> 
> Yes, although some TPDMs can support both CMB dataset and DSB dataset, 
> we need to configure them separately
> 
> in some scenarios. Based on your suggestion, I want to use the following 
> approach to implement it.
> 
> See below.
> 
>>
>> See below.
>>
>>> Since this special TPDM supports both DSB and CMB dataset, both DSB 
>>> patt_ts and CMB patt_ts will be set
>>>
>>> in this case even if I only configure the file in the DSB directory, 
>>> right?
>>>
>>> This is the problem we have now.
>>>
>>
>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +    return size;
>>>>>>>>>>>   }
>>>>>>>>>>>     /*
>>>>>>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct 
>>>>>>>>>>> device *dev,
>>>>>>>>>>>           return -EINVAL;
>>>>>>>>>>>         spin_lock(&drvdata->spinlock);
>>>>>>>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>>>>>>>> +
>>>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>>>>>>>       spin_unlock(&drvdata->spinlock);
>>>>>>>>>>> +
>>>>>>>>>>>       return size;
>>>>>>>>>>>   }
>>>>>>>>>>>   static DEVICE_ATTR_RW(enable_ts);
>>
>> Do not overload the same for both DSB and CMB. Create one for each in 
>> DSB and CMB ? They could share the same show/store routines, but could
>> be done with additional variable to indicate which attribute they are 
>> controlling. Like the other attributes, using dev_ext_attribute or such.
> 
> New approach below, please help review to see if it is acceptable?
> 
> #define tpdm_patt_enable_ts_rw(name, mem)                \
>      (&((struct tpdm_dataset_attribute[]) {            \
>         {                                \
>          __ATTR(name, 0644, enable_ts_show,        \
>          enable_ts_store),        \
>          mem,                            \
>         }                                \
>      })[0].attr.attr)
> 
> 
> #define DSB_PATT_ENABLE_TS                    \
>          tpdm_patt_enable_ts_rw(enable_ts,        \
>          DSB_PATT)
> 
> 
> #define CMB_PATT_ENABLE_TS                    \
>          tpdm_patt_enable_ts_rw(enable_ts,        \
>          CMB_PATT)
> 
> 
> static ssize_t enable_ts_show(struct device *dev,
>                    struct device_attribute *attr,
>                    char *buf)
> {
>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>      struct tpdm_dataset_attribute *tpdm_attr =
>          container_of(attr, struct tpdm_dataset_attribute, attr);
>      ssize_t size = 0;
> 
>      if (tpdm_attr->mem == DSB_PATT) {
>          size = sysfs_emit(buf, "%u\n",
>                   (unsigned int)drvdata->dsb->patt_ts);
>      } else if (tpdm_attr->mem == CMB_PATT) {
>          size = sysfs_emit(buf, "%u\n",
>                  (unsigned int)drvdata->cmb->patt_ts);
>      } else
>          return -EINVAL;
> 
>      return size;
> }
> 
> static ssize_t enable_ts_store(struct device *dev,
>                     struct device_attribute *attr,
>                     const char *buf,
>                     size_t size)
> {
>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>      struct tpdm_dataset_attribute *tpdm_attr =
>          container_of(attr, struct tpdm_dataset_attribute, attr);
>      unsigned long val;
> 
>      if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>          return -EINVAL;
> 
>      spin_lock(&drvdata->spinlock);
>      if (tpdm_attr->mem == DSB_PATT) {
>          drvdata->dsb->patt_ts = !!val;
>      } else if (tpdm_attr->mem == CMB_PATT) {
>          drvdata->cmb->patt_ts = !!val;
>      } else
>          return -EINVAL;
>      spin_unlock(&drvdata->spinlock);
> 
>      return size;
> }
> 
> 

Yes, that is what I had in mind.

Thanks
Suzuki

> Best,
> 
> Tao
> 
>>
>>
>> Suzuki
>>
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index 53662ce7c2d0..e0b77107be13 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -214,3 +214,38 @@  KernelVersion	6.7
 Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
 Description:
 		(RW) Set/Get the mask of the pattern for the CMB subunit TPDM.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_patt/enable_ts
+Date:		September 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(Write) Set the pattern timestamp of CMB tpdm. Read
+		the pattern timestamp of CMB tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Disable CMB pattern timestamp.
+		1 : Enable CMB pattern timestamp.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_trig_ts
+Date:		September 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Set/Get the trigger timestamp of the CMB for tpdm.
+
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Set the CMB trigger type to false
+		1 : Set the CMB trigger type to true
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_ts_all
+Date:		September 2023
+KernelVersion	6.7
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		(RW) Read or write the status of timestamp upon all interface.
+		Only value 0 and 1  can be written to this node. Set this node to 1 to requeset
+		timestamp to all trace packet.
+		Accepts only one of the 2 values -  0 or 1.
+		0 : Disable the timestamp of all trace packets.
+		1 : Enable the timestamp of all trace packets.
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 894d4309f1c7..f6cda5616e84 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -331,6 +331,36 @@  static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
+static void set_cmb_tier(struct tpdm_drvdata *drvdata)
+{
+	u32 val;
+
+	val = readl_relaxed(drvdata->base + TPDM_CMB_TIER);
+
+	/* Clear all relevant fields */
+	val &= ~(TPDM_CMB_TIER_PATT_TSENAB | TPDM_CMB_TIER_TS_ALL |
+		 TPDM_CMB_TIER_XTRIG_TSENAB);
+
+	/* Set pattern timestamp type and enablement */
+	if (drvdata->cmb->patt_ts)
+		val |= TPDM_CMB_TIER_PATT_TSENAB;
+	else
+		val &= ~TPDM_CMB_TIER_PATT_TSENAB;
+
+	/* Set trigger timestamp */
+	if (drvdata->cmb->trig_ts)
+		val |= TPDM_CMB_TIER_XTRIG_TSENAB;
+	else
+		val &= ~TPDM_CMB_TIER_XTRIG_TSENAB;
+
+	/* Set all timestamp enablement*/
+	if (drvdata->cmb->ts_all)
+		val |= TPDM_CMB_TIER_TS_ALL;
+	else
+		val &= ~TPDM_CMB_TIER_TS_ALL;
+	writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
+}
+
 static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 {
 	u32 val, i;
@@ -347,6 +377,8 @@  static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
 			    drvdata->base + TPDM_CMB_XPMR(i));
 	}
 
+	set_cmb_tier(drvdata);
+
 	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
 	/*
 	 * Set to 0 for continuous CMB collection mode,
@@ -695,9 +727,17 @@  static ssize_t enable_ts_show(struct device *dev,
 			      char *buf)
 {
 	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
 
-	return sysfs_emit(buf, "%u\n",
-			 (unsigned int)drvdata->dsb->patt_ts);
+	if (tpdm_has_dsb_dataset(drvdata))
+		size = sysfs_emit(buf, "%u\n",
+				 (unsigned int)drvdata->dsb->patt_ts);
+
+	if (tpdm_has_cmb_dataset(drvdata))
+		size = sysfs_emit(buf, "%u\n",
+				 (unsigned int)drvdata->cmb->patt_ts);
+
+	return size;
 }
 
 /*
@@ -715,8 +755,13 @@  static ssize_t enable_ts_store(struct device *dev,
 		return -EINVAL;
 
 	spin_lock(&drvdata->spinlock);
-	drvdata->dsb->patt_ts = !!val;
+	if (tpdm_has_dsb_dataset(drvdata))
+		drvdata->dsb->patt_ts = !!val;
+
+	if (tpdm_has_cmb_dataset(drvdata))
+		drvdata->cmb->patt_ts = !!val;
 	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(enable_ts);
@@ -851,6 +896,68 @@  static ssize_t cmb_mode_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(cmb_mode);
 
+static ssize_t cmb_ts_all_show(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n",
+			  (unsigned int)drvdata->cmb->ts_all);
+}
+
+static ssize_t cmb_ts_all_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf,
+				size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (val)
+		drvdata->cmb->ts_all = true;
+	else
+		drvdata->cmb->ts_all = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(cmb_ts_all);
+
+static ssize_t cmb_trig_ts_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n",
+			  (unsigned int)drvdata->cmb->trig_ts);
+}
+
+static ssize_t cmb_trig_ts_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf,
+				 size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (val)
+		drvdata->cmb->trig_ts = true;
+	else
+		drvdata->cmb->trig_ts = false;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(cmb_trig_ts);
+
 static struct attribute *tpdm_dsb_edge_attrs[] = {
 	&dev_attr_ctrl_idx.attr,
 	&dev_attr_ctrl_val.attr,
@@ -973,6 +1080,7 @@  static struct attribute *tpdm_cmb_patt_attrs[] = {
 	CMB_PATT_ATTR(1),
 	CMB_PATT_MASK_ATTR(0),
 	CMB_PATT_MASK_ATTR(1),
+	&dev_attr_enable_ts.attr,
 	NULL,
 };
 
@@ -985,6 +1093,8 @@  static struct attribute *tpdm_dsb_attrs[] = {
 
 static struct attribute *tpdm_cmb_attrs[] = {
 	&dev_attr_cmb_mode.attr,
+	&dev_attr_cmb_ts_all.attr,
+	&dev_attr_cmb_trig_ts.attr,
 	NULL,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index e90d008c1cb2..65b7ca6c4077 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -11,6 +11,8 @@ 
 
 /* CMB Subunit Registers */
 #define TPDM_CMB_CR		(0xA00)
+/*CMB subunit timestamp insertion enable register*/
+#define TPDM_CMB_TIER		(0xA04)
 /*CMB subunit timestamp pattern registers*/
 #define TPDM_CMB_TPR(n)		(0xA08 + (n * 4))
 /*CMB subunit timestamp pattern mask registers*/
@@ -24,6 +26,12 @@ 
 #define TPDM_CMB_CR_ENA		BIT(0)
 /* Trace collection mode for CMB subunit */
 #define TPDM_CMB_CR_MODE	BIT(1)
+/* Timestamp control for pattern match */
+#define TPDM_CMB_TIER_PATT_TSENAB	BIT(0)
+/* CMB CTI timestamp request */
+#define TPDM_CMB_TIER_XTRIG_TSENAB	BIT(1)
+/* For timestamp fo all trace */
+#define TPDM_CMB_TIER_TS_ALL		BIT(2)
 
 /*Patten register number*/
 #define TPDM_CMB_MAX_PATT		2
@@ -217,6 +225,9 @@  struct dsb_dataset {
  * @patt_mask:        Save value for pattern mask
  * @trig_patt:        Save value for trigger pattern
  * @trig_patt_mask:   Save value for trigger pattern mask
+ * @patt_ts:          Indicates if pattern match for timestamp is enabled.
+ * @trig_ts:          Indicates if CTI trigger for timestamp is enabled.
+ * @ts_all:           Indicates if timestamp is enabled for all packets.
  */
 struct cmb_dataset {
 	u32			trace_mode;
@@ -224,6 +235,9 @@  struct cmb_dataset {
 	u32			patt_mask[TPDM_CMB_MAX_PATT];
 	u32			trig_patt[TPDM_CMB_MAX_PATT];
 	u32			trig_patt_mask[TPDM_CMB_MAX_PATT];
+	bool			patt_ts;
+	bool			trig_ts;
+	bool			ts_all;
 };
 
 /**