[v3,8/8] coresight-tpdm: Add msr register support for CMB

  Add the nodes for CMB subunit MSR(mux select register) support.
CMB MSRs(mux select registers) is to separate mux,arbitration,
,interleaving,data packing control from stream filtering control.

Reviewed-by: James Clark <james.clark@arm.com>
Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
 drivers/hwtracing/coresight/coresight-tpdm.c  | 86 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
 3 files changed, 109 insertions(+), 1 deletion(-)
  

On 21/11/2023 02:24, Tao Zhang wrote:
> Add the nodes for CMB subunit MSR(mux select register) support.
> CMB MSRs(mux select registers) is to separate mux,arbitration,
> ,interleaving,data packing control from stream filtering control.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>  drivers/hwtracing/coresight/coresight-tpdm.c  | 86 +++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>  3 files changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index e0b77107be13..914f3fd81525 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -249,3 +249,11 @@ Description:
>  		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.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
> +Date:		September 2023
> +KernelVersion	6.7

This probably needs bumping now. Maybe 6.9?
  

On 21/11/2023 02:24, Tao Zhang wrote:
> Add the nodes for CMB subunit MSR(mux select register) support.
> CMB MSRs(mux select registers) is to separate mux,arbitration,
> ,interleaving,data packing control from stream filtering control.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 +++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>   3 files changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index e0b77107be13..914f3fd81525 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -249,3 +249,11 @@ Description:
>   		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.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
> +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 MSR(mux select register) for the CMB subunit
> +		TPDM.
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index f6cda5616e84..7e331ea436cc 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev,
>   			return -EINVAL;
>   		return sysfs_emit(buf, "0x%x\n",
>   			drvdata->cmb->patt_mask[tpdm_attr->idx]);
> +	case CMB_MSR:
> +		if (tpdm_attr->idx >= drvdata->cmb_msr_num)
> +			return -EINVAL;
> +		return sysfs_emit(buf, "0x%x\n",
> +				drvdata->cmb->msr[tpdm_attr->idx]);
>   	}
>   	return -EINVAL;
>   }
> @@ -162,6 +167,12 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev,
>   		else
>   			ret = -EINVAL;
>   		break;
> +	case CMB_MSR:
> +		if (tpdm_attr->idx < drvdata->cmb_msr_num)
> +			drvdata->cmb->msr[tpdm_attr->idx] = val;
> +		else
> +			ret = -EINVAL;


minor nit: Could we not break from here instead of adding return -EINVAL
for each case ? (I understand it has been done for the existing cases.
But I think we should clean up all of that, including the ones you added
in Patch 5. Similarly for the dataset_show()


Suzuki


> +		break;
>   	default:
>   		ret = -EINVAL;
>   	}
> @@ -220,6 +231,23 @@ static umode_t tpdm_dsb_msr_is_visible(struct kobject *kobj,
>   	return 0;
>   }
>   
> +static umode_t tpdm_cmb_msr_is_visible(struct kobject *kobj,
> +				       struct attribute *attr, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	struct device_attribute *dev_attr =
> +		container_of(attr, struct device_attribute, attr);
> +	struct tpdm_dataset_attribute *tpdm_attr =
> +		container_of(dev_attr, struct tpdm_dataset_attribute, attr);
> +
> +	if (tpdm_attr->idx < drvdata->cmb_msr_num)
> +		return attr->mode;
> +
> +	return 0;
> +}
> +
>   static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
>   {
>   	if (tpdm_has_dsb_dataset(drvdata)) {
> @@ -361,6 +389,15 @@ static void set_cmb_tier(struct tpdm_drvdata *drvdata)
>   	writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>   }
>   
> +static void set_cmb_msr(struct tpdm_drvdata *drvdata)
> +{
> +	int i;
> +
> +	for (i = 0; i < drvdata->cmb_msr_num; i++)
> +		writel_relaxed(drvdata->cmb->msr[i],
> +			   drvdata->base + TPDM_CMB_MSR(i));
> +}
> +
>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>   {
>   	u32 val, i;
> @@ -379,6 +416,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>   
>   	set_cmb_tier(drvdata);
>   
> +	set_cmb_msr(drvdata);
> +
>   	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>   	/*
>   	 * Set to 0 for continuous CMB collection mode,
> @@ -1084,6 +1123,42 @@ static struct attribute *tpdm_cmb_patt_attrs[] = {
>   	NULL,
>   };
>   
> +static struct attribute *tpdm_cmb_msr_attrs[] = {
> +	CMB_MSR_ATTR(0),
> +	CMB_MSR_ATTR(1),
> +	CMB_MSR_ATTR(2),
> +	CMB_MSR_ATTR(3),
> +	CMB_MSR_ATTR(4),
> +	CMB_MSR_ATTR(5),
> +	CMB_MSR_ATTR(6),
> +	CMB_MSR_ATTR(7),
> +	CMB_MSR_ATTR(8),
> +	CMB_MSR_ATTR(9),
> +	CMB_MSR_ATTR(10),
> +	CMB_MSR_ATTR(11),
> +	CMB_MSR_ATTR(12),
> +	CMB_MSR_ATTR(13),
> +	CMB_MSR_ATTR(14),
> +	CMB_MSR_ATTR(15),
> +	CMB_MSR_ATTR(16),
> +	CMB_MSR_ATTR(17),
> +	CMB_MSR_ATTR(18),
> +	CMB_MSR_ATTR(19),
> +	CMB_MSR_ATTR(20),
> +	CMB_MSR_ATTR(21),
> +	CMB_MSR_ATTR(22),
> +	CMB_MSR_ATTR(23),
> +	CMB_MSR_ATTR(24),
> +	CMB_MSR_ATTR(25),
> +	CMB_MSR_ATTR(26),
> +	CMB_MSR_ATTR(27),
> +	CMB_MSR_ATTR(28),
> +	CMB_MSR_ATTR(29),
> +	CMB_MSR_ATTR(30),
> +	CMB_MSR_ATTR(31),
> +	NULL,
> +};
> +
>   static struct attribute *tpdm_dsb_attrs[] = {
>   	&dev_attr_dsb_mode.attr,
>   	&dev_attr_dsb_trig_ts.attr,
> @@ -1144,6 +1219,12 @@ static struct attribute_group tpdm_cmb_patt_grp = {
>   	.name = "cmb_patt",
>   };
>   
> +static struct attribute_group tpdm_cmb_msr_grp = {
> +	.attrs = tpdm_cmb_msr_attrs,
> +	.is_visible = tpdm_cmb_msr_is_visible,
> +	.name = "cmb_msr",
> +};
> +
>   static const struct attribute_group *tpdm_attr_grps[] = {
>   	&tpdm_attr_grp,
>   	&tpdm_dsb_attr_grp,
> @@ -1154,6 +1235,7 @@ static const struct attribute_group *tpdm_attr_grps[] = {
>   	&tpdm_cmb_attr_grp,
>   	&tpdm_cmb_trig_patt_grp,
>   	&tpdm_cmb_patt_grp,
> +	&tpdm_cmb_msr_grp,
>   	NULL,
>   };
>   
> @@ -1192,6 +1274,10 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   		of_property_read_u32(drvdata->dev->of_node,
>   			   "qcom,dsb-msrs-num", &drvdata->dsb_msr_num);
>   
> +	if (drvdata && tpdm_has_cmb_dataset(drvdata))
> +		of_property_read_u32(drvdata->dev->of_node,
> +			   "qcom,cmb-msrs-num", &drvdata->cmb_msr_num);
> +
>   	/* Set up coresight component description */
>   	desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>   	if (!desc.name)
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 65b7ca6c4077..255104d024ab 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -21,6 +21,8 @@
>   #define TPDM_CMB_XPR(n)		(0xA18 + (n * 4))
>   /*CMB subunit trigger pattern mask registers*/
>   #define TPDM_CMB_XPMR(n)	(0xA20 + (n * 4))
> +/* CMB MSR register */
> +#define TPDM_CMB_MSR(n)		(0xA80 + (n * 4))
>   
>   /* Enable bit for CMB subunit */
>   #define TPDM_CMB_CR_ENA		BIT(0)
> @@ -36,6 +38,9 @@
>   /*Patten register number*/
>   #define TPDM_CMB_MAX_PATT		2
>   
> +/* MAX number of DSB MSR */
> +#define TPDM_CMB_MAX_MSR 32
> +
>   /* DSB Subunit Registers */
>   #define TPDM_DSB_CR		(0x780)
>   #define TPDM_DSB_TIER		(0x784)
> @@ -186,6 +191,10 @@
>   		tpdm_simple_dataset_rw(tpmr##nr,		\
>   		CMB_PATT_MASK, nr)
>   
> +#define CMB_MSR_ATTR(nr)					\
> +		tpdm_simple_dataset_rw(msr##nr,			\
> +		CMB_MSR, nr)
> +
>   /**
>    * struct dsb_dataset - specifics associated to dsb dataset
>    * @mode:             DSB programming mode
> @@ -225,6 +234,7 @@ 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
> + * @msr               Save value for MSR
>    * @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.
> @@ -235,6 +245,7 @@ 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];
> +	u32			msr[TPDM_CMB_MAX_MSR];
>   	bool			patt_ts;
>   	bool			trig_ts;
>   	bool			ts_all;
> @@ -251,6 +262,7 @@ struct cmb_dataset {
>    * @dsb         Specifics associated to TPDM DSB.
>    * @cmb         Specifics associated to TPDM CMB.
>    * @dsb_msr_num Number of MSR supported by DSB TPDM
> + * @cmb_msr_num Number of MSR supported by CMB TPDM
>    */
>   
>   struct tpdm_drvdata {
> @@ -263,6 +275,7 @@ struct tpdm_drvdata {
>   	struct dsb_dataset	*dsb;
>   	struct cmb_dataset	*cmb;
>   	u32			dsb_msr_num;
> +	u32			cmb_msr_num;
>   };
>   
>   /* Enumerate members of various datasets */
> @@ -277,7 +290,8 @@ enum dataset_mem {
>   	CMB_TRIG_PATT,
>   	CMB_TRIG_PATT_MASK,
>   	CMB_PATT,
> -	CMB_PATT_MASK
> +	CMB_PATT_MASK,
> +	CMB_MSR
>   };
>   
>   /**
  

On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
> On 21/11/2023 02:24, Tao Zhang wrote:
>> Add the nodes for CMB subunit MSR(mux select register) support.
>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>> ,interleaving,data packing control from stream filtering control.
>>
>> Reviewed-by: James Clark <james.clark@arm.com>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index e0b77107be13..914f3fd81525 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -249,3 +249,11 @@ Description:
>>           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.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>> +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 MSR(mux select register) for the CMB subunit
>> +        TPDM.
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index f6cda5616e84..7e331ea436cc 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct 
>> device *dev,
>>               return -EINVAL;
>>           return sysfs_emit(buf, "0x%x\n",
>>               drvdata->cmb->patt_mask[tpdm_attr->idx]);
>> +    case CMB_MSR:
>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>> +            return -EINVAL;
>> +        return sysfs_emit(buf, "0x%x\n",
>> +                drvdata->cmb->msr[tpdm_attr->idx]);
>>       }
>>       return -EINVAL;
>>   }
>> @@ -162,6 +167,12 @@ static ssize_t tpdm_simple_dataset_store(struct 
>> device *dev,
>>           else
>>               ret = -EINVAL;
>>           break;
>> +    case CMB_MSR:
>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>> +            drvdata->cmb->msr[tpdm_attr->idx] = val;
>> +        else
>> +            ret = -EINVAL;
>
>
> minor nit: Could we not break from here instead of adding return -EINVAL
> for each case ? (I understand it has been done for the existing cases.
> But I think we should clean up all of that, including the ones you added
> in Patch 5. Similarly for the dataset_show()

Sure, do I also need to change the DSB corresponding code? If so, how about

if I add a new patch to the next patch series to change the previous 
existing cases?


Best,

Tao

>
>
> Suzuki
>
>
>> +        break;
>>       default:
>>           ret = -EINVAL;
>>       }
>> @@ -220,6 +231,23 @@ static umode_t tpdm_dsb_msr_is_visible(struct 
>> kobject *kobj,
>>       return 0;
>>   }
>>   +static umode_t tpdm_cmb_msr_is_visible(struct kobject *kobj,
>> +                       struct attribute *attr, int n)
>> +{
>> +    struct device *dev = kobj_to_dev(kobj);
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    struct device_attribute *dev_attr =
>> +        container_of(attr, struct device_attribute, attr);
>> +    struct tpdm_dataset_attribute *tpdm_attr =
>> +        container_of(dev_attr, struct tpdm_dataset_attribute, attr);
>> +
>> +    if (tpdm_attr->idx < drvdata->cmb_msr_num)
>> +        return attr->mode;
>> +
>> +    return 0;
>> +}
>> +
>>   static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
>>   {
>>       if (tpdm_has_dsb_dataset(drvdata)) {
>> @@ -361,6 +389,15 @@ static void set_cmb_tier(struct tpdm_drvdata 
>> *drvdata)
>>       writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>   }
>>   +static void set_cmb_msr(struct tpdm_drvdata *drvdata)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < drvdata->cmb_msr_num; i++)
>> +        writel_relaxed(drvdata->cmb->msr[i],
>> +               drvdata->base + TPDM_CMB_MSR(i));
>> +}
>> +
>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>   {
>>       u32 val, i;
>> @@ -379,6 +416,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata 
>> *drvdata)
>>         set_cmb_tier(drvdata);
>>   +    set_cmb_msr(drvdata);
>> +
>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>       /*
>>        * Set to 0 for continuous CMB collection mode,
>> @@ -1084,6 +1123,42 @@ static struct attribute *tpdm_cmb_patt_attrs[] 
>> = {
>>       NULL,
>>   };
>>   +static struct attribute *tpdm_cmb_msr_attrs[] = {
>> +    CMB_MSR_ATTR(0),
>> +    CMB_MSR_ATTR(1),
>> +    CMB_MSR_ATTR(2),
>> +    CMB_MSR_ATTR(3),
>> +    CMB_MSR_ATTR(4),
>> +    CMB_MSR_ATTR(5),
>> +    CMB_MSR_ATTR(6),
>> +    CMB_MSR_ATTR(7),
>> +    CMB_MSR_ATTR(8),
>> +    CMB_MSR_ATTR(9),
>> +    CMB_MSR_ATTR(10),
>> +    CMB_MSR_ATTR(11),
>> +    CMB_MSR_ATTR(12),
>> +    CMB_MSR_ATTR(13),
>> +    CMB_MSR_ATTR(14),
>> +    CMB_MSR_ATTR(15),
>> +    CMB_MSR_ATTR(16),
>> +    CMB_MSR_ATTR(17),
>> +    CMB_MSR_ATTR(18),
>> +    CMB_MSR_ATTR(19),
>> +    CMB_MSR_ATTR(20),
>> +    CMB_MSR_ATTR(21),
>> +    CMB_MSR_ATTR(22),
>> +    CMB_MSR_ATTR(23),
>> +    CMB_MSR_ATTR(24),
>> +    CMB_MSR_ATTR(25),
>> +    CMB_MSR_ATTR(26),
>> +    CMB_MSR_ATTR(27),
>> +    CMB_MSR_ATTR(28),
>> +    CMB_MSR_ATTR(29),
>> +    CMB_MSR_ATTR(30),
>> +    CMB_MSR_ATTR(31),
>> +    NULL,
>> +};
>> +
>>   static struct attribute *tpdm_dsb_attrs[] = {
>>       &dev_attr_dsb_mode.attr,
>>       &dev_attr_dsb_trig_ts.attr,
>> @@ -1144,6 +1219,12 @@ static struct attribute_group 
>> tpdm_cmb_patt_grp = {
>>       .name = "cmb_patt",
>>   };
>>   +static struct attribute_group tpdm_cmb_msr_grp = {
>> +    .attrs = tpdm_cmb_msr_attrs,
>> +    .is_visible = tpdm_cmb_msr_is_visible,
>> +    .name = "cmb_msr",
>> +};
>> +
>>   static const struct attribute_group *tpdm_attr_grps[] = {
>>       &tpdm_attr_grp,
>>       &tpdm_dsb_attr_grp,
>> @@ -1154,6 +1235,7 @@ static const struct attribute_group 
>> *tpdm_attr_grps[] = {
>>       &tpdm_cmb_attr_grp,
>>       &tpdm_cmb_trig_patt_grp,
>>       &tpdm_cmb_patt_grp,
>> +    &tpdm_cmb_msr_grp,
>>       NULL,
>>   };
>>   @@ -1192,6 +1274,10 @@ static int tpdm_probe(struct amba_device 
>> *adev, const struct amba_id *id)
>>           of_property_read_u32(drvdata->dev->of_node,
>>                  "qcom,dsb-msrs-num", &drvdata->dsb_msr_num);
>>   +    if (drvdata && tpdm_has_cmb_dataset(drvdata))
>> +        of_property_read_u32(drvdata->dev->of_node,
>> +               "qcom,cmb-msrs-num", &drvdata->cmb_msr_num);
>> +
>>       /* Set up coresight component description */
>>       desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>>       if (!desc.name)
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 65b7ca6c4077..255104d024ab 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -21,6 +21,8 @@
>>   #define TPDM_CMB_XPR(n)        (0xA18 + (n * 4))
>>   /*CMB subunit trigger pattern mask registers*/
>>   #define TPDM_CMB_XPMR(n)    (0xA20 + (n * 4))
>> +/* CMB MSR register */
>> +#define TPDM_CMB_MSR(n)        (0xA80 + (n * 4))
>>     /* Enable bit for CMB subunit */
>>   #define TPDM_CMB_CR_ENA        BIT(0)
>> @@ -36,6 +38,9 @@
>>   /*Patten register number*/
>>   #define TPDM_CMB_MAX_PATT        2
>>   +/* MAX number of DSB MSR */
>> +#define TPDM_CMB_MAX_MSR 32
>> +
>>   /* DSB Subunit Registers */
>>   #define TPDM_DSB_CR        (0x780)
>>   #define TPDM_DSB_TIER        (0x784)
>> @@ -186,6 +191,10 @@
>>           tpdm_simple_dataset_rw(tpmr##nr,        \
>>           CMB_PATT_MASK, nr)
>>   +#define CMB_MSR_ATTR(nr)                    \
>> +        tpdm_simple_dataset_rw(msr##nr,            \
>> +        CMB_MSR, nr)
>> +
>>   /**
>>    * struct dsb_dataset - specifics associated to dsb dataset
>>    * @mode:             DSB programming mode
>> @@ -225,6 +234,7 @@ 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
>> + * @msr               Save value for MSR
>>    * @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.
>> @@ -235,6 +245,7 @@ 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];
>> +    u32            msr[TPDM_CMB_MAX_MSR];
>>       bool            patt_ts;
>>       bool            trig_ts;
>>       bool            ts_all;
>> @@ -251,6 +262,7 @@ struct cmb_dataset {
>>    * @dsb         Specifics associated to TPDM DSB.
>>    * @cmb         Specifics associated to TPDM CMB.
>>    * @dsb_msr_num Number of MSR supported by DSB TPDM
>> + * @cmb_msr_num Number of MSR supported by CMB TPDM
>>    */
>>     struct tpdm_drvdata {
>> @@ -263,6 +275,7 @@ struct tpdm_drvdata {
>>       struct dsb_dataset    *dsb;
>>       struct cmb_dataset    *cmb;
>>       u32            dsb_msr_num;
>> +    u32            cmb_msr_num;
>>   };
>>     /* Enumerate members of various datasets */
>> @@ -277,7 +290,8 @@ enum dataset_mem {
>>       CMB_TRIG_PATT,
>>       CMB_TRIG_PATT_MASK,
>>       CMB_PATT,
>> -    CMB_PATT_MASK
>> +    CMB_PATT_MASK,
>> +    CMB_MSR
>>   };
>>     /**
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
  

On 19/12/2023 06:58, Tao Zhang wrote:
> 
> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>> On 21/11/2023 02:24, Tao Zhang wrote:
>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>> ,interleaving,data packing control from stream filtering control.
>>>
>>> Reviewed-by: James Clark <james.clark@arm.com>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>> ---
>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 +++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>
>>> diff --git 
>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> index e0b77107be13..914f3fd81525 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> @@ -249,3 +249,11 @@ Description:
>>>           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.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>> +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 MSR(mux select register) for the CMB subunit
>>> +        TPDM.
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> index f6cda5616e84..7e331ea436cc 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct 
>>> device *dev,
>>>               return -EINVAL;
>>>           return sysfs_emit(buf, "0x%x\n",
>>>               drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>> +    case CMB_MSR:
>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>> +            return -EINVAL;
>>> +        return sysfs_emit(buf, "0x%x\n",
>>> +                drvdata->cmb->msr[tpdm_attr->idx]);
>>>       }
>>>       return -EINVAL;
>>>   }
>>> @@ -162,6 +167,12 @@ static ssize_t tpdm_simple_dataset_store(struct 
>>> device *dev,
>>>           else
>>>               ret = -EINVAL;
>>>           break;
>>> +    case CMB_MSR:
>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>> +            drvdata->cmb->msr[tpdm_attr->idx] = val;
>>> +        else
>>> +            ret = -EINVAL;
>>
>>
>> minor nit: Could we not break from here instead of adding return -EINVAL
>> for each case ? (I understand it has been done for the existing cases.
>> But I think we should clean up all of that, including the ones you added
>> in Patch 5. Similarly for the dataset_show()
> 
> Sure, do I also need to change the DSB corresponding code? If so, how about
> 
> if I add a new patch to the next patch series to change the previous 
> existing cases?

You could fix the existing cases as a preparatory patch of the next 
version of this series. I can pick it up and push it to next as I see fit.

Suzuki


> 
> 
> Best,
> 
> Tao
> 
>>
>>
>> Suzuki
>>
>>
>>> +        break;
>>>       default:
>>>           ret = -EINVAL;
>>>       }
>>> @@ -220,6 +231,23 @@ static umode_t tpdm_dsb_msr_is_visible(struct 
>>> kobject *kobj,
>>>       return 0;
>>>   }
>>>   +static umode_t tpdm_cmb_msr_is_visible(struct kobject *kobj,
>>> +                       struct attribute *attr, int n)
>>> +{
>>> +    struct device *dev = kobj_to_dev(kobj);
>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    struct device_attribute *dev_attr =
>>> +        container_of(attr, struct device_attribute, attr);
>>> +    struct tpdm_dataset_attribute *tpdm_attr =
>>> +        container_of(dev_attr, struct tpdm_dataset_attribute, attr);
>>> +
>>> +    if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>> +        return attr->mode;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
>>>   {
>>>       if (tpdm_has_dsb_dataset(drvdata)) {
>>> @@ -361,6 +389,15 @@ static void set_cmb_tier(struct tpdm_drvdata 
>>> *drvdata)
>>>       writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>   }
>>>   +static void set_cmb_msr(struct tpdm_drvdata *drvdata)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < drvdata->cmb_msr_num; i++)
>>> +        writel_relaxed(drvdata->cmb->msr[i],
>>> +               drvdata->base + TPDM_CMB_MSR(i));
>>> +}
>>> +
>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>   {
>>>       u32 val, i;
>>> @@ -379,6 +416,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata 
>>> *drvdata)
>>>         set_cmb_tier(drvdata);
>>>   +    set_cmb_msr(drvdata);
>>> +
>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>       /*
>>>        * Set to 0 for continuous CMB collection mode,
>>> @@ -1084,6 +1123,42 @@ static struct attribute *tpdm_cmb_patt_attrs[] 
>>> = {
>>>       NULL,
>>>   };
>>>   +static struct attribute *tpdm_cmb_msr_attrs[] = {
>>> +    CMB_MSR_ATTR(0),
>>> +    CMB_MSR_ATTR(1),
>>> +    CMB_MSR_ATTR(2),
>>> +    CMB_MSR_ATTR(3),
>>> +    CMB_MSR_ATTR(4),
>>> +    CMB_MSR_ATTR(5),
>>> +    CMB_MSR_ATTR(6),
>>> +    CMB_MSR_ATTR(7),
>>> +    CMB_MSR_ATTR(8),
>>> +    CMB_MSR_ATTR(9),
>>> +    CMB_MSR_ATTR(10),
>>> +    CMB_MSR_ATTR(11),
>>> +    CMB_MSR_ATTR(12),
>>> +    CMB_MSR_ATTR(13),
>>> +    CMB_MSR_ATTR(14),
>>> +    CMB_MSR_ATTR(15),
>>> +    CMB_MSR_ATTR(16),
>>> +    CMB_MSR_ATTR(17),
>>> +    CMB_MSR_ATTR(18),
>>> +    CMB_MSR_ATTR(19),
>>> +    CMB_MSR_ATTR(20),
>>> +    CMB_MSR_ATTR(21),
>>> +    CMB_MSR_ATTR(22),
>>> +    CMB_MSR_ATTR(23),
>>> +    CMB_MSR_ATTR(24),
>>> +    CMB_MSR_ATTR(25),
>>> +    CMB_MSR_ATTR(26),
>>> +    CMB_MSR_ATTR(27),
>>> +    CMB_MSR_ATTR(28),
>>> +    CMB_MSR_ATTR(29),
>>> +    CMB_MSR_ATTR(30),
>>> +    CMB_MSR_ATTR(31),
>>> +    NULL,
>>> +};
>>> +
>>>   static struct attribute *tpdm_dsb_attrs[] = {
>>>       &dev_attr_dsb_mode.attr,
>>>       &dev_attr_dsb_trig_ts.attr,
>>> @@ -1144,6 +1219,12 @@ static struct attribute_group 
>>> tpdm_cmb_patt_grp = {
>>>       .name = "cmb_patt",
>>>   };
>>>   +static struct attribute_group tpdm_cmb_msr_grp = {
>>> +    .attrs = tpdm_cmb_msr_attrs,
>>> +    .is_visible = tpdm_cmb_msr_is_visible,
>>> +    .name = "cmb_msr",
>>> +};
>>> +
>>>   static const struct attribute_group *tpdm_attr_grps[] = {
>>>       &tpdm_attr_grp,
>>>       &tpdm_dsb_attr_grp,
>>> @@ -1154,6 +1235,7 @@ static const struct attribute_group 
>>> *tpdm_attr_grps[] = {
>>>       &tpdm_cmb_attr_grp,
>>>       &tpdm_cmb_trig_patt_grp,
>>>       &tpdm_cmb_patt_grp,
>>> +    &tpdm_cmb_msr_grp,
>>>       NULL,
>>>   };
>>>   @@ -1192,6 +1274,10 @@ static int tpdm_probe(struct amba_device 
>>> *adev, const struct amba_id *id)
>>>           of_property_read_u32(drvdata->dev->of_node,
>>>                  "qcom,dsb-msrs-num", &drvdata->dsb_msr_num);
>>>   +    if (drvdata && tpdm_has_cmb_dataset(drvdata))
>>> +        of_property_read_u32(drvdata->dev->of_node,
>>> +               "qcom,cmb-msrs-num", &drvdata->cmb_msr_num);
>>> +
>>>       /* Set up coresight component description */
>>>       desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>>>       if (!desc.name)
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> index 65b7ca6c4077..255104d024ab 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> @@ -21,6 +21,8 @@
>>>   #define TPDM_CMB_XPR(n)        (0xA18 + (n * 4))
>>>   /*CMB subunit trigger pattern mask registers*/
>>>   #define TPDM_CMB_XPMR(n)    (0xA20 + (n * 4))
>>> +/* CMB MSR register */
>>> +#define TPDM_CMB_MSR(n)        (0xA80 + (n * 4))
>>>     /* Enable bit for CMB subunit */
>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>> @@ -36,6 +38,9 @@
>>>   /*Patten register number*/
>>>   #define TPDM_CMB_MAX_PATT        2
>>>   +/* MAX number of DSB MSR */
>>> +#define TPDM_CMB_MAX_MSR 32
>>> +
>>>   /* DSB Subunit Registers */
>>>   #define TPDM_DSB_CR        (0x780)
>>>   #define TPDM_DSB_TIER        (0x784)
>>> @@ -186,6 +191,10 @@
>>>           tpdm_simple_dataset_rw(tpmr##nr,        \
>>>           CMB_PATT_MASK, nr)
>>>   +#define CMB_MSR_ATTR(nr)                    \
>>> +        tpdm_simple_dataset_rw(msr##nr,            \
>>> +        CMB_MSR, nr)
>>> +
>>>   /**
>>>    * struct dsb_dataset - specifics associated to dsb dataset
>>>    * @mode:             DSB programming mode
>>> @@ -225,6 +234,7 @@ 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
>>> + * @msr               Save value for MSR
>>>    * @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.
>>> @@ -235,6 +245,7 @@ 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];
>>> +    u32            msr[TPDM_CMB_MAX_MSR];
>>>       bool            patt_ts;
>>>       bool            trig_ts;
>>>       bool            ts_all;
>>> @@ -251,6 +262,7 @@ struct cmb_dataset {
>>>    * @dsb         Specifics associated to TPDM DSB.
>>>    * @cmb         Specifics associated to TPDM CMB.
>>>    * @dsb_msr_num Number of MSR supported by DSB TPDM
>>> + * @cmb_msr_num Number of MSR supported by CMB TPDM
>>>    */
>>>     struct tpdm_drvdata {
>>> @@ -263,6 +275,7 @@ struct tpdm_drvdata {
>>>       struct dsb_dataset    *dsb;
>>>       struct cmb_dataset    *cmb;
>>>       u32            dsb_msr_num;
>>> +    u32            cmb_msr_num;
>>>   };
>>>     /* Enumerate members of various datasets */
>>> @@ -277,7 +290,8 @@ enum dataset_mem {
>>>       CMB_TRIG_PATT,
>>>       CMB_TRIG_PATT_MASK,
>>>       CMB_PATT,
>>> -    CMB_PATT_MASK
>>> +    CMB_PATT_MASK,
>>> +    CMB_MSR
>>>   };
>>>     /**
>>
>> _______________________________________________
>> CoreSight mailing list -- coresight@lists.linaro.org
>> To unsubscribe send an email to coresight-leave@lists.linaro.org
  

On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
> On 19/12/2023 06:58, Tao Zhang wrote:
>>
>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>> ,interleaving,data packing control from stream filtering control.
>>>>
>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>> ---
>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 
>>>> +++++++++++++++++++
>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git 
>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> index e0b77107be13..914f3fd81525 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> @@ -249,3 +249,11 @@ Description:
>>>>           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.
>>>> +
>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>> +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 MSR(mux select register) for the CMB subunit
>>>> +        TPDM.
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> index f6cda5616e84..7e331ea436cc 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct 
>>>> device *dev,
>>>>               return -EINVAL;
>>>>           return sysfs_emit(buf, "0x%x\n",
>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>> +    case CMB_MSR:
>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>> +            return -EINVAL;
>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>       }
>>>>       return -EINVAL;
>>>>   }
>>>> @@ -162,6 +167,12 @@ static ssize_t 
>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>           else
>>>>               ret = -EINVAL;
>>>>           break;
>>>> +    case CMB_MSR:
>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>> +            drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>> +        else
>>>> +            ret = -EINVAL;
>>>
>>>
>>> minor nit: Could we not break from here instead of adding return 
>>> -EINVAL
>>> for each case ? (I understand it has been done for the existing cases.
>>> But I think we should clean up all of that, including the ones you 
>>> added
>>> in Patch 5. Similarly for the dataset_show()
>>
>> Sure, do I also need to change the DSB corresponding code? If so, how 
>> about
>>
>> if I add a new patch to the next patch series to change the previous 
>> existing cases?
>
> You could fix the existing cases as a preparatory patch of the next 
> version of this series. I can pick it up and push it to next as I see 
> fit.

Got it. I will update this to the next patch series.


Best,

Tao

>
> Suzuki
>
>
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>>
>>> Suzuki
>>>
>>>
>>>> +        break;
>>>>       default:
>>>>           ret = -EINVAL;
>>>>       }
>>>> @@ -220,6 +231,23 @@ static umode_t tpdm_dsb_msr_is_visible(struct 
>>>> kobject *kobj,
>>>>       return 0;
>>>>   }
>>>>   +static umode_t tpdm_cmb_msr_is_visible(struct kobject *kobj,
>>>> +                       struct attribute *attr, int n)
>>>> +{
>>>> +    struct device *dev = kobj_to_dev(kobj);
>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +
>>>> +    struct device_attribute *dev_attr =
>>>> +        container_of(attr, struct device_attribute, attr);
>>>> +    struct tpdm_dataset_attribute *tpdm_attr =
>>>> +        container_of(dev_attr, struct tpdm_dataset_attribute, attr);
>>>> +
>>>> +    if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>> +        return attr->mode;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
>>>>   {
>>>>       if (tpdm_has_dsb_dataset(drvdata)) {
>>>> @@ -361,6 +389,15 @@ static void set_cmb_tier(struct tpdm_drvdata 
>>>> *drvdata)
>>>>       writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>   }
>>>>   +static void set_cmb_msr(struct tpdm_drvdata *drvdata)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < drvdata->cmb_msr_num; i++)
>>>> +        writel_relaxed(drvdata->cmb->msr[i],
>>>> +               drvdata->base + TPDM_CMB_MSR(i));
>>>> +}
>>>> +
>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>   {
>>>>       u32 val, i;
>>>> @@ -379,6 +416,8 @@ static void tpdm_enable_cmb(struct tpdm_drvdata 
>>>> *drvdata)
>>>>         set_cmb_tier(drvdata);
>>>>   +    set_cmb_msr(drvdata);
>>>> +
>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>       /*
>>>>        * Set to 0 for continuous CMB collection mode,
>>>> @@ -1084,6 +1123,42 @@ static struct attribute 
>>>> *tpdm_cmb_patt_attrs[] = {
>>>>       NULL,
>>>>   };
>>>>   +static struct attribute *tpdm_cmb_msr_attrs[] = {
>>>> +    CMB_MSR_ATTR(0),
>>>> +    CMB_MSR_ATTR(1),
>>>> +    CMB_MSR_ATTR(2),
>>>> +    CMB_MSR_ATTR(3),
>>>> +    CMB_MSR_ATTR(4),
>>>> +    CMB_MSR_ATTR(5),
>>>> +    CMB_MSR_ATTR(6),
>>>> +    CMB_MSR_ATTR(7),
>>>> +    CMB_MSR_ATTR(8),
>>>> +    CMB_MSR_ATTR(9),
>>>> +    CMB_MSR_ATTR(10),
>>>> +    CMB_MSR_ATTR(11),
>>>> +    CMB_MSR_ATTR(12),
>>>> +    CMB_MSR_ATTR(13),
>>>> +    CMB_MSR_ATTR(14),
>>>> +    CMB_MSR_ATTR(15),
>>>> +    CMB_MSR_ATTR(16),
>>>> +    CMB_MSR_ATTR(17),
>>>> +    CMB_MSR_ATTR(18),
>>>> +    CMB_MSR_ATTR(19),
>>>> +    CMB_MSR_ATTR(20),
>>>> +    CMB_MSR_ATTR(21),
>>>> +    CMB_MSR_ATTR(22),
>>>> +    CMB_MSR_ATTR(23),
>>>> +    CMB_MSR_ATTR(24),
>>>> +    CMB_MSR_ATTR(25),
>>>> +    CMB_MSR_ATTR(26),
>>>> +    CMB_MSR_ATTR(27),
>>>> +    CMB_MSR_ATTR(28),
>>>> +    CMB_MSR_ATTR(29),
>>>> +    CMB_MSR_ATTR(30),
>>>> +    CMB_MSR_ATTR(31),
>>>> +    NULL,
>>>> +};
>>>> +
>>>>   static struct attribute *tpdm_dsb_attrs[] = {
>>>>       &dev_attr_dsb_mode.attr,
>>>>       &dev_attr_dsb_trig_ts.attr,
>>>> @@ -1144,6 +1219,12 @@ static struct attribute_group 
>>>> tpdm_cmb_patt_grp = {
>>>>       .name = "cmb_patt",
>>>>   };
>>>>   +static struct attribute_group tpdm_cmb_msr_grp = {
>>>> +    .attrs = tpdm_cmb_msr_attrs,
>>>> +    .is_visible = tpdm_cmb_msr_is_visible,
>>>> +    .name = "cmb_msr",
>>>> +};
>>>> +
>>>>   static const struct attribute_group *tpdm_attr_grps[] = {
>>>>       &tpdm_attr_grp,
>>>>       &tpdm_dsb_attr_grp,
>>>> @@ -1154,6 +1235,7 @@ static const struct attribute_group 
>>>> *tpdm_attr_grps[] = {
>>>>       &tpdm_cmb_attr_grp,
>>>>       &tpdm_cmb_trig_patt_grp,
>>>>       &tpdm_cmb_patt_grp,
>>>> +    &tpdm_cmb_msr_grp,
>>>>       NULL,
>>>>   };
>>>>   @@ -1192,6 +1274,10 @@ static int tpdm_probe(struct amba_device 
>>>> *adev, const struct amba_id *id)
>>>>           of_property_read_u32(drvdata->dev->of_node,
>>>>                  "qcom,dsb-msrs-num", &drvdata->dsb_msr_num);
>>>>   +    if (drvdata && tpdm_has_cmb_dataset(drvdata))
>>>> +        of_property_read_u32(drvdata->dev->of_node,
>>>> +               "qcom,cmb-msrs-num", &drvdata->cmb_msr_num);
>>>> +
>>>>       /* Set up coresight component description */
>>>>       desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>>>>       if (!desc.name)
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> index 65b7ca6c4077..255104d024ab 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> @@ -21,6 +21,8 @@
>>>>   #define TPDM_CMB_XPR(n)        (0xA18 + (n * 4))
>>>>   /*CMB subunit trigger pattern mask registers*/
>>>>   #define TPDM_CMB_XPMR(n)    (0xA20 + (n * 4))
>>>> +/* CMB MSR register */
>>>> +#define TPDM_CMB_MSR(n)        (0xA80 + (n * 4))
>>>>     /* Enable bit for CMB subunit */
>>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>> @@ -36,6 +38,9 @@
>>>>   /*Patten register number*/
>>>>   #define TPDM_CMB_MAX_PATT        2
>>>>   +/* MAX number of DSB MSR */
>>>> +#define TPDM_CMB_MAX_MSR 32
>>>> +
>>>>   /* DSB Subunit Registers */
>>>>   #define TPDM_DSB_CR        (0x780)
>>>>   #define TPDM_DSB_TIER        (0x784)
>>>> @@ -186,6 +191,10 @@
>>>>           tpdm_simple_dataset_rw(tpmr##nr,        \
>>>>           CMB_PATT_MASK, nr)
>>>>   +#define CMB_MSR_ATTR(nr)                    \
>>>> +        tpdm_simple_dataset_rw(msr##nr,            \
>>>> +        CMB_MSR, nr)
>>>> +
>>>>   /**
>>>>    * struct dsb_dataset - specifics associated to dsb dataset
>>>>    * @mode:             DSB programming mode
>>>> @@ -225,6 +234,7 @@ 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
>>>> + * @msr               Save value for MSR
>>>>    * @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.
>>>> @@ -235,6 +245,7 @@ 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];
>>>> +    u32            msr[TPDM_CMB_MAX_MSR];
>>>>       bool            patt_ts;
>>>>       bool            trig_ts;
>>>>       bool            ts_all;
>>>> @@ -251,6 +262,7 @@ struct cmb_dataset {
>>>>    * @dsb         Specifics associated to TPDM DSB.
>>>>    * @cmb         Specifics associated to TPDM CMB.
>>>>    * @dsb_msr_num Number of MSR supported by DSB TPDM
>>>> + * @cmb_msr_num Number of MSR supported by CMB TPDM
>>>>    */
>>>>     struct tpdm_drvdata {
>>>> @@ -263,6 +275,7 @@ struct tpdm_drvdata {
>>>>       struct dsb_dataset    *dsb;
>>>>       struct cmb_dataset    *cmb;
>>>>       u32            dsb_msr_num;
>>>> +    u32            cmb_msr_num;
>>>>   };
>>>>     /* Enumerate members of various datasets */
>>>> @@ -277,7 +290,8 @@ enum dataset_mem {
>>>>       CMB_TRIG_PATT,
>>>>       CMB_TRIG_PATT_MASK,
>>>>       CMB_PATT,
>>>> -    CMB_PATT_MASK
>>>> +    CMB_PATT_MASK,
>>>> +    CMB_MSR
>>>>   };
>>>>     /**
>>>
>>> _______________________________________________
>>> CoreSight mailing list -- coresight@lists.linaro.org
>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
>
  

On 12/20/2023 5:06 PM, Tao Zhang wrote:
>
> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>
>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>
>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>> ---
>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 
>>>>> +++++++++++++++++++
>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git 
>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>> index e0b77107be13..914f3fd81525 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>           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.
>>>>> +
>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>> +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 MSR(mux select register) for the CMB 
>>>>> subunit
>>>>> +        TPDM.
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct 
>>>>> device *dev,
>>>>>               return -EINVAL;
>>>>>           return sysfs_emit(buf, "0x%x\n",
>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>> +    case CMB_MSR:
>>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>> +            return -EINVAL;
>>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>       }
>>>>>       return -EINVAL;
>>>>>   }
>>>>> @@ -162,6 +167,12 @@ static ssize_t 
>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>           else
>>>>>               ret = -EINVAL;
>>>>>           break;
>>>>> +    case CMB_MSR:
>>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>> +            drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>> +        else
>>>>> +            ret = -EINVAL;
>>>>
>>>>
>>>> minor nit: Could we not break from here instead of adding return 
>>>> -EINVAL
>>>> for each case ? (I understand it has been done for the existing cases.
>>>> But I think we should clean up all of that, including the ones you 
>>>> added
>>>> in Patch 5. Similarly for the dataset_show()
>>>
>>> Sure, do I also need to change the DSB corresponding code? If so, 
>>> how about
>>>
>>> if I add a new patch to the next patch series to change the previous 
>>> existing cases?
>>
>> You could fix the existing cases as a preparatory patch of the next 
>> version of this series. I can pick it up and push it to next as I see 
>> fit.
>
> Got it. I will update this to the next patch series.

Hi Suzuki,


Since the dataset data is configured with spin lock protection, it needs 
to be unlock before return.

List my modification below. Would you mind help review to see if it is 
good for you.

static ssize_t tpdm_simple_dataset_store(struct device *dev,
                      struct device_attribute *attr,
                      const char *buf,
                      size_t size)
{
     unsigned long val;

     struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
     struct tpdm_dataset_attribute *tpdm_attr =
         container_of(attr, struct tpdm_dataset_attribute, attr);

     if (kstrtoul(buf, 0, &val))
         return -EINVAL;

     spin_lock(&drvdata->spinlock);
     switch (tpdm_attr->mem) {
     case DSB_TRIG_PATT:
         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
             drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
         else {
             spin_unlock(&drvdata->spinlock);
             return -EINVAL;
         }
     case DSB_TRIG_PATT_MASK:
         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
             drvdata->dsb->trig_patt_mask[tpdm_attr->idx] = val;
         else{
             spin_unlock(&drvdata->spinlock);
             return -EINVAL;
         }
     case DSB_PATT:
         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
             drvdata->dsb->patt_val[tpdm_attr->idx] = val;
         else{
             spin_unlock(&drvdata->spinlock);
             return -EINVAL;
         }
     case DSB_PATT_MASK:
         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
             drvdata->dsb->patt_mask[tpdm_attr->idx] = val;
         else{
             spin_unlock(&drvdata->spinlock);
             return -EINVAL;
         }
     case DSB_MSR:
         if (tpdm_attr->idx < drvdata->dsb_msr_num)
             drvdata->dsb->msr[tpdm_attr->idx] = val;
         else{
             spin_unlock(&drvdata->spinlock);
             return -EINVAL;
         }
     default:
         spin_unlock(&drvdata->spinlock);
         return -EINVAL;
     }
     return size;


Best,

Tao

>
>
> Best,
>
> Tao
>
>>
>> Suzuki
>>
>>
>>>
>>>
>>> Best,
>>>
>>> Tao
>>>
>>>>
>>>>
>>>> Suzuki
>>>>
>>>>
>>>>> +        break;
>>>>>       default:
>>>>>           ret = -EINVAL;
>>>>>       }
>>>>> @@ -220,6 +231,23 @@ static umode_t tpdm_dsb_msr_is_visible(struct 
>>>>> kobject *kobj,
>>>>>       return 0;
>>>>>   }
>>>>>   +static umode_t tpdm_cmb_msr_is_visible(struct kobject *kobj,
>>>>> +                       struct attribute *attr, int n)
>>>>> +{
>>>>> +    struct device *dev = kobj_to_dev(kobj);
>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +
>>>>> +    struct device_attribute *dev_attr =
>>>>> +        container_of(attr, struct device_attribute, attr);
>>>>> +    struct tpdm_dataset_attribute *tpdm_attr =
>>>>> +        container_of(dev_attr, struct tpdm_dataset_attribute, attr);
>>>>> +
>>>>> +    if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>> +        return attr->mode;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata)
>>>>>   {
>>>>>       if (tpdm_has_dsb_dataset(drvdata)) {
>>>>> @@ -361,6 +389,15 @@ static void set_cmb_tier(struct tpdm_drvdata 
>>>>> *drvdata)
>>>>>       writel_relaxed(val, drvdata->base + TPDM_CMB_TIER);
>>>>>   }
>>>>>   +static void set_cmb_msr(struct tpdm_drvdata *drvdata)
>>>>> +{
>>>>> +    int i;
>>>>> +
>>>>> +    for (i = 0; i < drvdata->cmb_msr_num; i++)
>>>>> +        writel_relaxed(drvdata->cmb->msr[i],
>>>>> +               drvdata->base + TPDM_CMB_MSR(i));
>>>>> +}
>>>>> +
>>>>>   static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>>>   {
>>>>>       u32 val, i;
>>>>> @@ -379,6 +416,8 @@ static void tpdm_enable_cmb(struct 
>>>>> tpdm_drvdata *drvdata)
>>>>>         set_cmb_tier(drvdata);
>>>>>   +    set_cmb_msr(drvdata);
>>>>> +
>>>>>       val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>>>       /*
>>>>>        * Set to 0 for continuous CMB collection mode,
>>>>> @@ -1084,6 +1123,42 @@ static struct attribute 
>>>>> *tpdm_cmb_patt_attrs[] = {
>>>>>       NULL,
>>>>>   };
>>>>>   +static struct attribute *tpdm_cmb_msr_attrs[] = {
>>>>> +    CMB_MSR_ATTR(0),
>>>>> +    CMB_MSR_ATTR(1),
>>>>> +    CMB_MSR_ATTR(2),
>>>>> +    CMB_MSR_ATTR(3),
>>>>> +    CMB_MSR_ATTR(4),
>>>>> +    CMB_MSR_ATTR(5),
>>>>> +    CMB_MSR_ATTR(6),
>>>>> +    CMB_MSR_ATTR(7),
>>>>> +    CMB_MSR_ATTR(8),
>>>>> +    CMB_MSR_ATTR(9),
>>>>> +    CMB_MSR_ATTR(10),
>>>>> +    CMB_MSR_ATTR(11),
>>>>> +    CMB_MSR_ATTR(12),
>>>>> +    CMB_MSR_ATTR(13),
>>>>> +    CMB_MSR_ATTR(14),
>>>>> +    CMB_MSR_ATTR(15),
>>>>> +    CMB_MSR_ATTR(16),
>>>>> +    CMB_MSR_ATTR(17),
>>>>> +    CMB_MSR_ATTR(18),
>>>>> +    CMB_MSR_ATTR(19),
>>>>> +    CMB_MSR_ATTR(20),
>>>>> +    CMB_MSR_ATTR(21),
>>>>> +    CMB_MSR_ATTR(22),
>>>>> +    CMB_MSR_ATTR(23),
>>>>> +    CMB_MSR_ATTR(24),
>>>>> +    CMB_MSR_ATTR(25),
>>>>> +    CMB_MSR_ATTR(26),
>>>>> +    CMB_MSR_ATTR(27),
>>>>> +    CMB_MSR_ATTR(28),
>>>>> +    CMB_MSR_ATTR(29),
>>>>> +    CMB_MSR_ATTR(30),
>>>>> +    CMB_MSR_ATTR(31),
>>>>> +    NULL,
>>>>> +};
>>>>> +
>>>>>   static struct attribute *tpdm_dsb_attrs[] = {
>>>>>       &dev_attr_dsb_mode.attr,
>>>>>       &dev_attr_dsb_trig_ts.attr,
>>>>> @@ -1144,6 +1219,12 @@ static struct attribute_group 
>>>>> tpdm_cmb_patt_grp = {
>>>>>       .name = "cmb_patt",
>>>>>   };
>>>>>   +static struct attribute_group tpdm_cmb_msr_grp = {
>>>>> +    .attrs = tpdm_cmb_msr_attrs,
>>>>> +    .is_visible = tpdm_cmb_msr_is_visible,
>>>>> +    .name = "cmb_msr",
>>>>> +};
>>>>> +
>>>>>   static const struct attribute_group *tpdm_attr_grps[] = {
>>>>>       &tpdm_attr_grp,
>>>>>       &tpdm_dsb_attr_grp,
>>>>> @@ -1154,6 +1235,7 @@ static const struct attribute_group 
>>>>> *tpdm_attr_grps[] = {
>>>>>       &tpdm_cmb_attr_grp,
>>>>>       &tpdm_cmb_trig_patt_grp,
>>>>>       &tpdm_cmb_patt_grp,
>>>>> +    &tpdm_cmb_msr_grp,
>>>>>       NULL,
>>>>>   };
>>>>>   @@ -1192,6 +1274,10 @@ static int tpdm_probe(struct amba_device 
>>>>> *adev, const struct amba_id *id)
>>>>> of_property_read_u32(drvdata->dev->of_node,
>>>>>                  "qcom,dsb-msrs-num", &drvdata->dsb_msr_num);
>>>>>   +    if (drvdata && tpdm_has_cmb_dataset(drvdata))
>>>>> +        of_property_read_u32(drvdata->dev->of_node,
>>>>> +               "qcom,cmb-msrs-num", &drvdata->cmb_msr_num);
>>>>> +
>>>>>       /* Set up coresight component description */
>>>>>       desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>>>>>       if (!desc.name)
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> index 65b7ca6c4077..255104d024ab 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> @@ -21,6 +21,8 @@
>>>>>   #define TPDM_CMB_XPR(n)        (0xA18 + (n * 4))
>>>>>   /*CMB subunit trigger pattern mask registers*/
>>>>>   #define TPDM_CMB_XPMR(n)    (0xA20 + (n * 4))
>>>>> +/* CMB MSR register */
>>>>> +#define TPDM_CMB_MSR(n)        (0xA80 + (n * 4))
>>>>>     /* Enable bit for CMB subunit */
>>>>>   #define TPDM_CMB_CR_ENA        BIT(0)
>>>>> @@ -36,6 +38,9 @@
>>>>>   /*Patten register number*/
>>>>>   #define TPDM_CMB_MAX_PATT        2
>>>>>   +/* MAX number of DSB MSR */
>>>>> +#define TPDM_CMB_MAX_MSR 32
>>>>> +
>>>>>   /* DSB Subunit Registers */
>>>>>   #define TPDM_DSB_CR        (0x780)
>>>>>   #define TPDM_DSB_TIER        (0x784)
>>>>> @@ -186,6 +191,10 @@
>>>>>           tpdm_simple_dataset_rw(tpmr##nr,        \
>>>>>           CMB_PATT_MASK, nr)
>>>>>   +#define CMB_MSR_ATTR(nr)                    \
>>>>> +        tpdm_simple_dataset_rw(msr##nr,            \
>>>>> +        CMB_MSR, nr)
>>>>> +
>>>>>   /**
>>>>>    * struct dsb_dataset - specifics associated to dsb dataset
>>>>>    * @mode:             DSB programming mode
>>>>> @@ -225,6 +234,7 @@ 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
>>>>> + * @msr               Save value for MSR
>>>>>    * @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.
>>>>> @@ -235,6 +245,7 @@ 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];
>>>>> +    u32            msr[TPDM_CMB_MAX_MSR];
>>>>>       bool            patt_ts;
>>>>>       bool            trig_ts;
>>>>>       bool            ts_all;
>>>>> @@ -251,6 +262,7 @@ struct cmb_dataset {
>>>>>    * @dsb         Specifics associated to TPDM DSB.
>>>>>    * @cmb         Specifics associated to TPDM CMB.
>>>>>    * @dsb_msr_num Number of MSR supported by DSB TPDM
>>>>> + * @cmb_msr_num Number of MSR supported by CMB TPDM
>>>>>    */
>>>>>     struct tpdm_drvdata {
>>>>> @@ -263,6 +275,7 @@ struct tpdm_drvdata {
>>>>>       struct dsb_dataset    *dsb;
>>>>>       struct cmb_dataset    *cmb;
>>>>>       u32            dsb_msr_num;
>>>>> +    u32            cmb_msr_num;
>>>>>   };
>>>>>     /* Enumerate members of various datasets */
>>>>> @@ -277,7 +290,8 @@ enum dataset_mem {
>>>>>       CMB_TRIG_PATT,
>>>>>       CMB_TRIG_PATT_MASK,
>>>>>       CMB_PATT,
>>>>> -    CMB_PATT_MASK
>>>>> +    CMB_PATT_MASK,
>>>>> +    CMB_MSR
>>>>>   };
>>>>>     /**
>>>>
>>>> _______________________________________________
>>>> 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
  

On 12/01/2024 09:12, Tao Zhang wrote:
> 
> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>
>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>
>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>
>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>> ---
>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 
>>>>>> +++++++++++++++++++
>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git 
>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>           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.
>>>>>> +
>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>> +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 MSR(mux select register) for the CMB 
>>>>>> subunit
>>>>>> +        TPDM.
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct 
>>>>>> device *dev,
>>>>>>               return -EINVAL;
>>>>>>           return sysfs_emit(buf, "0x%x\n",
>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>> +    case CMB_MSR:
>>>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>> +            return -EINVAL;
>>>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>       }
>>>>>>       return -EINVAL;
>>>>>>   }
>>>>>> @@ -162,6 +167,12 @@ static ssize_t 
>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>           else
>>>>>>               ret = -EINVAL;
>>>>>>           break;
>>>>>> +    case CMB_MSR:
>>>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>> +            drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>> +        else
>>>>>> +            ret = -EINVAL;
>>>>>
>>>>>
>>>>> minor nit: Could we not break from here instead of adding return 
>>>>> -EINVAL
>>>>> for each case ? (I understand it has been done for the existing cases.
>>>>> But I think we should clean up all of that, including the ones you 
>>>>> added
>>>>> in Patch 5. Similarly for the dataset_show()
>>>>
>>>> Sure, do I also need to change the DSB corresponding code? If so, 
>>>> how about
>>>>
>>>> if I add a new patch to the next patch series to change the previous 
>>>> existing cases?
>>>
>>> You could fix the existing cases as a preparatory patch of the next 
>>> version of this series. I can pick it up and push it to next as I see 
>>> fit.
>>
>> Got it. I will update this to the next patch series.
> 
> Hi Suzuki,
> 
> 
> Since the dataset data is configured with spin lock protection, it needs 
> to be unlock before return.
> 
> List my modification below. Would you mind help review to see if it is 
> good for you.
> 
> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>                       struct device_attribute *attr,
>                       const char *buf,
>                       size_t size)
> {
>      unsigned long val;
> 
>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>      struct tpdm_dataset_attribute *tpdm_attr =
>          container_of(attr, struct tpdm_dataset_attribute, attr);
> 
>      if (kstrtoul(buf, 0, &val))
>          return -EINVAL;
> 
>      spin_lock(&drvdata->spinlock);

Use guard() to avoid explicit unlock on return and then you don't need 
the spin_unlock() everywhere. It would be done on return from the
function implicitly.


>      switch (tpdm_attr->mem) {
>      case DSB_TRIG_PATT:

With guard() in place:

	ret = -EINVAL;

	switch () {
	case XXX:

	   if (tpdm_attr->idx < TPDM_XXXX_IDX) {
		   drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
		   ret = size;
	   }
	   break;
	case ...
		...
	}

	return ret;


Suzuki
  

On 1/12/2024 5:43 PM, Suzuki K Poulose wrote:
> On 12/01/2024 09:12, Tao Zhang wrote:
>>
>> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>>
>>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>>
>>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>>
>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>>> ---
>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 
>>>>>>> +++++++++++++++++++
>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git 
>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>>           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.
>>>>>>> +
>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>>> +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 MSR(mux select register) for the CMB 
>>>>>>> subunit
>>>>>>> +        TPDM.
>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> @@ -86,6 +86,11 @@ static ssize_t 
>>>>>>> tpdm_simple_dataset_show(struct device *dev,
>>>>>>>               return -EINVAL;
>>>>>>>           return sysfs_emit(buf, "0x%x\n",
>>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>>> +    case CMB_MSR:
>>>>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>>> +            return -EINVAL;
>>>>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>>       }
>>>>>>>       return -EINVAL;
>>>>>>>   }
>>>>>>> @@ -162,6 +167,12 @@ static ssize_t 
>>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>>           else
>>>>>>>               ret = -EINVAL;
>>>>>>>           break;
>>>>>>> +    case CMB_MSR:
>>>>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>>> +        else
>>>>>>> +            ret = -EINVAL;
>>>>>>
>>>>>>
>>>>>> minor nit: Could we not break from here instead of adding return 
>>>>>> -EINVAL
>>>>>> for each case ? (I understand it has been done for the existing 
>>>>>> cases.
>>>>>> But I think we should clean up all of that, including the ones 
>>>>>> you added
>>>>>> in Patch 5. Similarly for the dataset_show()
>>>>>
>>>>> Sure, do I also need to change the DSB corresponding code? If so, 
>>>>> how about
>>>>>
>>>>> if I add a new patch to the next patch series to change the 
>>>>> previous existing cases?
>>>>
>>>> You could fix the existing cases as a preparatory patch of the next 
>>>> version of this series. I can pick it up and push it to next as I 
>>>> see fit.
>>>
>>> Got it. I will update this to the next patch series.
>>
>> Hi Suzuki,
>>
>>
>> Since the dataset data is configured with spin lock protection, it 
>> needs to be unlock before return.
>>
>> List my modification below. Would you mind help review to see if it 
>> is good for you.
>>
>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>>                       struct device_attribute *attr,
>>                       const char *buf,
>>                       size_t size)
>> {
>>      unsigned long val;
>>
>>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>      struct tpdm_dataset_attribute *tpdm_attr =
>>          container_of(attr, struct tpdm_dataset_attribute, attr);
>>
>>      if (kstrtoul(buf, 0, &val))
>>          return -EINVAL;
>>
>>      spin_lock(&drvdata->spinlock);
>
> Use guard() to avoid explicit unlock on return and then you don't need 
> the spin_unlock() everywhere. It would be done on return from the
> function implicitly.
>
>
>>      switch (tpdm_attr->mem) {
>>      case DSB_TRIG_PATT:
>
> With guard() in place:
>
>     ret = -EINVAL;
>
>     switch () {
>     case XXX:
>
>        if (tpdm_attr->idx < TPDM_XXXX_IDX) {
>            drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>            ret = size;
>        }
>        break;
>     case ...
>         ...
>     }
>
>     return ret;

Thanks for your suggestion. I will update the code like below.

I will update it in the next version of the patch series if it meets 
your expectation.

static ssize_t tpdm_simple_dataset_store(struct device *dev,
                      struct device_attribute *attr,
                      const char *buf,
                      size_t size)
{
     unsigned long val;
     ssize_t ret = -EINVAL;

     struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
     struct tpdm_dataset_attribute *tpdm_attr =
         container_of(attr, struct tpdm_dataset_attribute, attr);

     if (kstrtoul(buf, 0, &val))
         return ret;

     guard(spinlock)(&drvdata->spinlock);
     switch (tpdm_attr->mem) {
     case DSB_TRIG_PATT:
         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) {
             drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
             ret =size;
         }
         break;
     case ...

         ...
     }
     return ret;
}

Best,

Tao

>
>
> Suzuki
>
  

On 15/01/2024 06:20, Tao Zhang wrote:
> 
> On 1/12/2024 5:43 PM, Suzuki K Poulose wrote:
>> On 12/01/2024 09:12, Tao Zhang wrote:
>>>
>>> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>>>
>>>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>>>
>>>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>>>
>>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>>>> ---
>>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 
>>>>>>>> +++++++++++++++++++
>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git 
>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>>>           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.
>>>>>>>> +
>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>>>> +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 MSR(mux select register) for the CMB 
>>>>>>>> subunit
>>>>>>>> +        TPDM.
>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> @@ -86,6 +86,11 @@ static ssize_t 
>>>>>>>> tpdm_simple_dataset_show(struct device *dev,
>>>>>>>>               return -EINVAL;
>>>>>>>>           return sysfs_emit(buf, "0x%x\n",
>>>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>>>> +    case CMB_MSR:
>>>>>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>>>> +            return -EINVAL;
>>>>>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>>>       }
>>>>>>>>       return -EINVAL;
>>>>>>>>   }
>>>>>>>> @@ -162,6 +167,12 @@ static ssize_t 
>>>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>>>           else
>>>>>>>>               ret = -EINVAL;
>>>>>>>>           break;
>>>>>>>> +    case CMB_MSR:
>>>>>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>>>> +        else
>>>>>>>> +            ret = -EINVAL;
>>>>>>>
>>>>>>>
>>>>>>> minor nit: Could we not break from here instead of adding return 
>>>>>>> -EINVAL
>>>>>>> for each case ? (I understand it has been done for the existing 
>>>>>>> cases.
>>>>>>> But I think we should clean up all of that, including the ones 
>>>>>>> you added
>>>>>>> in Patch 5. Similarly for the dataset_show()
>>>>>>
>>>>>> Sure, do I also need to change the DSB corresponding code? If so, 
>>>>>> how about
>>>>>>
>>>>>> if I add a new patch to the next patch series to change the 
>>>>>> previous existing cases?
>>>>>
>>>>> You could fix the existing cases as a preparatory patch of the next 
>>>>> version of this series. I can pick it up and push it to next as I 
>>>>> see fit.
>>>>
>>>> Got it. I will update this to the next patch series.
>>>
>>> Hi Suzuki,
>>>
>>>
>>> Since the dataset data is configured with spin lock protection, it 
>>> needs to be unlock before return.
>>>
>>> List my modification below. Would you mind help review to see if it 
>>> is good for you.
>>>
>>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>>>                       struct device_attribute *attr,
>>>                       const char *buf,
>>>                       size_t size)
>>> {
>>>      unsigned long val;
>>>
>>>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>      struct tpdm_dataset_attribute *tpdm_attr =
>>>          container_of(attr, struct tpdm_dataset_attribute, attr);
>>>
>>>      if (kstrtoul(buf, 0, &val))
>>>          return -EINVAL;
>>>
>>>      spin_lock(&drvdata->spinlock);
>>
>> Use guard() to avoid explicit unlock on return and then you don't need 
>> the spin_unlock() everywhere. It would be done on return from the
>> function implicitly.
>>
>>
>>>      switch (tpdm_attr->mem) {
>>>      case DSB_TRIG_PATT:
>>
>> With guard() in place:
>>
>>     ret = -EINVAL;
>>
>>     switch () {
>>     case XXX:
>>
>>        if (tpdm_attr->idx < TPDM_XXXX_IDX) {
>>            drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>>            ret = size;
>>        }
>>        break;
>>     case ...
>>         ...
>>     }
>>
>>     return ret;
> 
> Thanks for your suggestion. I will update the code like below.
> 
> I will update it in the next version of the patch series if it meets 
> your expectation.
> 
> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>                       struct device_attribute *attr,
>                       const char *buf,
>                       size_t size)
> {
>      unsigned long val;
>      ssize_t ret = -EINVAL;
> 
>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>      struct tpdm_dataset_attribute *tpdm_attr =
>          container_of(attr, struct tpdm_dataset_attribute, attr);
> 
>      if (kstrtoul(buf, 0, &val))
>          return ret;
> 
>      guard(spinlock)(&drvdata->spinlock);
>      switch (tpdm_attr->mem) {
>      case DSB_TRIG_PATT:
>          if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) {
>              drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>              ret =size;
>          }
>          break;
>      case ...
> 
>          ...
>      }
>      return ret;
> }
> 

Yes that looks good to me. Please rebase this on to for-next/queue 
branch on the coresight repository.

Suzuki
  

On 12/01/2024 09:12, Tao Zhang wrote:
> 
> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>
>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>
>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>
>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>> ---
>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86
>>>>>> +++++++++++++++++++
>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>           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.
>>>>>> +
>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>> +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 MSR(mux select register) for the CMB
>>>>>> subunit
>>>>>> +        TPDM.
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct
>>>>>> device *dev,
>>>>>>               return -EINVAL;
>>>>>>           return sysfs_emit(buf, "0x%x\n",
>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>> +    case CMB_MSR:
>>>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>> +            return -EINVAL;
>>>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>       }
>>>>>>       return -EINVAL;
>>>>>>   }
>>>>>> @@ -162,6 +167,12 @@ static ssize_t
>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>           else
>>>>>>               ret = -EINVAL;
>>>>>>           break;
>>>>>> +    case CMB_MSR:
>>>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>> +            drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>> +        else
>>>>>> +            ret = -EINVAL;
>>>>>
>>>>>
>>>>> minor nit: Could we not break from here instead of adding return
>>>>> -EINVAL
>>>>> for each case ? (I understand it has been done for the existing cases.
>>>>> But I think we should clean up all of that, including the ones you
>>>>> added
>>>>> in Patch 5. Similarly for the dataset_show()
>>>>
>>>> Sure, do I also need to change the DSB corresponding code? If so,
>>>> how about
>>>>
>>>> if I add a new patch to the next patch series to change the previous
>>>> existing cases?
>>>
>>> You could fix the existing cases as a preparatory patch of the next
>>> version of this series. I can pick it up and push it to next as I see
>>> fit.
>>
>> Got it. I will update this to the next patch series.
> 
> Hi Suzuki,
> 
> 
> Since the dataset data is configured with spin lock protection, it needs
> to be unlock before return.
> 
> List my modification below. Would you mind help review to see if it is
> good for you.
> 
> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>                      struct device_attribute *attr,
>                      const char *buf,
>                      size_t size)
> {
>     unsigned long val;
> 
>     struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>     struct tpdm_dataset_attribute *tpdm_attr =
>         container_of(attr, struct tpdm_dataset_attribute, attr);
> 
>     if (kstrtoul(buf, 0, &val))
>         return -EINVAL;
> 
>     spin_lock(&drvdata->spinlock);
>     switch (tpdm_attr->mem) {
>     case DSB_TRIG_PATT:
>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>             drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>         else {
>             spin_unlock(&drvdata->spinlock);
>             return -EINVAL;
>         }
>     case DSB_TRIG_PATT_MASK:
>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>             drvdata->dsb->trig_patt_mask[tpdm_attr->idx] = val;
>         else{
>             spin_unlock(&drvdata->spinlock);
>             return -EINVAL;
>         }
>     case DSB_PATT:
>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>             drvdata->dsb->patt_val[tpdm_attr->idx] = val;
>         else{
>             spin_unlock(&drvdata->spinlock);
>             return -EINVAL;
>         }
>     case DSB_PATT_MASK:
>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>             drvdata->dsb->patt_mask[tpdm_attr->idx] = val;
>         else{
>             spin_unlock(&drvdata->spinlock);
>             return -EINVAL;
>         }
>     case DSB_MSR:
>         if (tpdm_attr->idx < drvdata->dsb_msr_num)
>             drvdata->dsb->msr[tpdm_attr->idx] = val;
>         else{
>             spin_unlock(&drvdata->spinlock);
>             return -EINVAL;
>         }
>     default:
>         spin_unlock(&drvdata->spinlock);
>         return -EINVAL;
>     }
>     return size;
> 
> 
> Best,
> 
> Tao
> 

This looks like a good fit for the new
guard(spinlock)(&drvdata->spinlock) thing. Then there is no need to do
all the manual unlocking.
  

On 15/01/2024 09:55, James Clark wrote:
> 
> 
> On 12/01/2024 09:12, Tao Zhang wrote:
>>
>> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>>
>>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>>
>>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>>
>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>>> ---
>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  |  8 ++
>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86
>>>>>>> +++++++++++++++++++
>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>>           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.
>>>>>>> +
>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>>> +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 MSR(mux select register) for the CMB
>>>>>>> subunit
>>>>>>> +        TPDM.
>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>> @@ -86,6 +86,11 @@ static ssize_t tpdm_simple_dataset_show(struct
>>>>>>> device *dev,
>>>>>>>               return -EINVAL;
>>>>>>>           return sysfs_emit(buf, "0x%x\n",
>>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>>> +    case CMB_MSR:
>>>>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>>> +            return -EINVAL;
>>>>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>>       }
>>>>>>>       return -EINVAL;
>>>>>>>   }
>>>>>>> @@ -162,6 +167,12 @@ static ssize_t
>>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>>           else
>>>>>>>               ret = -EINVAL;
>>>>>>>           break;
>>>>>>> +    case CMB_MSR:
>>>>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>>> +            drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>>> +        else
>>>>>>> +            ret = -EINVAL;
>>>>>>
>>>>>>
>>>>>> minor nit: Could we not break from here instead of adding return
>>>>>> -EINVAL
>>>>>> for each case ? (I understand it has been done for the existing cases.
>>>>>> But I think we should clean up all of that, including the ones you
>>>>>> added
>>>>>> in Patch 5. Similarly for the dataset_show()
>>>>>
>>>>> Sure, do I also need to change the DSB corresponding code? If so,
>>>>> how about
>>>>>
>>>>> if I add a new patch to the next patch series to change the previous
>>>>> existing cases?
>>>>
>>>> You could fix the existing cases as a preparatory patch of the next
>>>> version of this series. I can pick it up and push it to next as I see
>>>> fit.
>>>
>>> Got it. I will update this to the next patch series.
>>
>> Hi Suzuki,
>>
>>
>> Since the dataset data is configured with spin lock protection, it needs
>> to be unlock before return.
>>
>> List my modification below. Would you mind help review to see if it is
>> good for you.
>>
>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>>                      struct device_attribute *attr,
>>                      const char *buf,
>>                      size_t size)
>> {
>>     unsigned long val;
>>
>>     struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>     struct tpdm_dataset_attribute *tpdm_attr =
>>         container_of(attr, struct tpdm_dataset_attribute, attr);
>>
>>     if (kstrtoul(buf, 0, &val))
>>         return -EINVAL;
>>
>>     spin_lock(&drvdata->spinlock);
>>     switch (tpdm_attr->mem) {
>>     case DSB_TRIG_PATT:
>>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>>             drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>>         else {
>>             spin_unlock(&drvdata->spinlock);
>>             return -EINVAL;
>>         }
>>     case DSB_TRIG_PATT_MASK:
>>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>>             drvdata->dsb->trig_patt_mask[tpdm_attr->idx] = val;
>>         else{
>>             spin_unlock(&drvdata->spinlock);
>>             return -EINVAL;
>>         }
>>     case DSB_PATT:
>>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>>             drvdata->dsb->patt_val[tpdm_attr->idx] = val;
>>         else{
>>             spin_unlock(&drvdata->spinlock);
>>             return -EINVAL;
>>         }
>>     case DSB_PATT_MASK:
>>         if (tpdm_attr->idx < TPDM_DSB_MAX_PATT)
>>             drvdata->dsb->patt_mask[tpdm_attr->idx] = val;
>>         else{
>>             spin_unlock(&drvdata->spinlock);
>>             return -EINVAL;
>>         }
>>     case DSB_MSR:
>>         if (tpdm_attr->idx < drvdata->dsb_msr_num)
>>             drvdata->dsb->msr[tpdm_attr->idx] = val;
>>         else{
>>             spin_unlock(&drvdata->spinlock);
>>             return -EINVAL;
>>         }
>>     default:
>>         spin_unlock(&drvdata->spinlock);
>>         return -EINVAL;
>>     }
>>     return size;
>>
>>
>> Best,
>>
>> Tao
>>
> 
> This looks like a good fit for the new
> guard(spinlock)(&drvdata->spinlock) thing. Then there is no need to do
> all the manual unlocking.
> 

Oh I see Suzuki already suggested it, nevermind.
  

On 1/15/2024 5:20 PM, Suzuki K Poulose wrote:
> On 15/01/2024 06:20, Tao Zhang wrote:
>>
>> On 1/12/2024 5:43 PM, Suzuki K Poulose wrote:
>>> On 12/01/2024 09:12, Tao Zhang wrote:
>>>>
>>>> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>>>>
>>>>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>>>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>>>>
>>>>>>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>>>>> ---
>>>>>>>>>   .../testing/sysfs-bus-coresight-devices-tpdm  | 8 ++
>>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 86 
>>>>>>>>> +++++++++++++++++++
>>>>>>>>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 16 +++-
>>>>>>>>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git 
>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>>>>           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.
>>>>>>>>> +
>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>>>>> +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 MSR(mux select register) for the CMB 
>>>>>>>>> subunit
>>>>>>>>> +        TPDM.
>>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> @@ -86,6 +86,11 @@ static ssize_t 
>>>>>>>>> tpdm_simple_dataset_show(struct device *dev,
>>>>>>>>>               return -EINVAL;
>>>>>>>>>           return sysfs_emit(buf, "0x%x\n",
>>>>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>>>>> +    case CMB_MSR:
>>>>>>>>> +        if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>>>>> +            return -EINVAL;
>>>>>>>>> +        return sysfs_emit(buf, "0x%x\n",
>>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>>>>       }
>>>>>>>>>       return -EINVAL;
>>>>>>>>>   }
>>>>>>>>> @@ -162,6 +167,12 @@ static ssize_t 
>>>>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>>>>           else
>>>>>>>>>               ret = -EINVAL;
>>>>>>>>>           break;
>>>>>>>>> +    case CMB_MSR:
>>>>>>>>> +        if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>>>>> +        else
>>>>>>>>> +            ret = -EINVAL;
>>>>>>>>
>>>>>>>>
>>>>>>>> minor nit: Could we not break from here instead of adding 
>>>>>>>> return -EINVAL
>>>>>>>> for each case ? (I understand it has been done for the existing 
>>>>>>>> cases.
>>>>>>>> But I think we should clean up all of that, including the ones 
>>>>>>>> you added
>>>>>>>> in Patch 5. Similarly for the dataset_show()
>>>>>>>
>>>>>>> Sure, do I also need to change the DSB corresponding code? If 
>>>>>>> so, how about
>>>>>>>
>>>>>>> if I add a new patch to the next patch series to change the 
>>>>>>> previous existing cases?
>>>>>>
>>>>>> You could fix the existing cases as a preparatory patch of the 
>>>>>> next version of this series. I can pick it up and push it to next 
>>>>>> as I see fit.
>>>>>
>>>>> Got it. I will update this to the next patch series.
>>>>
>>>> Hi Suzuki,
>>>>
>>>>
>>>> Since the dataset data is configured with spin lock protection, it 
>>>> needs to be unlock before return.
>>>>
>>>> List my modification below. Would you mind help review to see if it 
>>>> is good for you.
>>>>
>>>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>>>>                       struct device_attribute *attr,
>>>>                       const char *buf,
>>>>                       size_t size)
>>>> {
>>>>      unsigned long val;
>>>>
>>>>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>      struct tpdm_dataset_attribute *tpdm_attr =
>>>>          container_of(attr, struct tpdm_dataset_attribute, attr);
>>>>
>>>>      if (kstrtoul(buf, 0, &val))
>>>>          return -EINVAL;
>>>>
>>>>      spin_lock(&drvdata->spinlock);
>>>
>>> Use guard() to avoid explicit unlock on return and then you don't 
>>> need the spin_unlock() everywhere. It would be done on return from the
>>> function implicitly.
>>>
>>>
>>>>      switch (tpdm_attr->mem) {
>>>>      case DSB_TRIG_PATT:
>>>
>>> With guard() in place:
>>>
>>>     ret = -EINVAL;
>>>
>>>     switch () {
>>>     case XXX:
>>>
>>>        if (tpdm_attr->idx < TPDM_XXXX_IDX) {
>>>            drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>>>            ret = size;
>>>        }
>>>        break;
>>>     case ...
>>>         ...
>>>     }
>>>
>>>     return ret;
>>
>> Thanks for your suggestion. I will update the code like below.
>>
>> I will update it in the next version of the patch series if it meets 
>> your expectation.
>>
>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>>                       struct device_attribute *attr,
>>                       const char *buf,
>>                       size_t size)
>> {
>>      unsigned long val;
>>      ssize_t ret = -EINVAL;
>>
>>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>      struct tpdm_dataset_attribute *tpdm_attr =
>>          container_of(attr, struct tpdm_dataset_attribute, attr);
>>
>>      if (kstrtoul(buf, 0, &val))
>>          return ret;
>>
>>      guard(spinlock)(&drvdata->spinlock);
>>      switch (tpdm_attr->mem) {
>>      case DSB_TRIG_PATT:
>>          if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) {
>>              drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>>              ret =size;
>>          }
>>          break;
>>      case ...
>>
>>          ...
>>      }
>>      return ret;
>> }
>>
>
> Yes that looks good to me. Please rebase this on to for-next/queue 
> branch on the coresight repository.

Got it. Thanks for your mention.


Best,

Tao

>
> Suzuki
>