diff mbox series

[v2,4/9] coresight-tpdm: Add reset node to TPDM node

Message ID 1674114105-16651-5-git-send-email-quic_taozha@quicinc.com
State New
Headers
Series Add support to configure TPDM DSB subunit |

Commit Message

Tao Zhang Jan. 19, 2023, 7:41 a.m. UTC 
  TPDM device need a node to reset the configurations and status of
it. So as to avoid the previous configurations affecting the
current use, the configurations need to be reset first. And in
some scenarios, it may be necessary to reset the TPDM
configurations to complete the verification of certain function.
This change provides a node to reset the configurations and
disable the TPDM if it has been enabled.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Tao Zhang <taozha@qti.qualcomm.com>
---
 drivers/hwtracing/coresight/coresight-tpdm.c | 32 ++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Suzuki K Poulose Feb. 28, 2023, 11:22 a.m. UTC | #1 
On 19/01/2023 07:41, Tao Zhang wrote:
> TPDM device need a node to reset the configurations and status of
> it. So as to avoid the previous configurations affecting the
> current use, the configurations need to be reset first. And in
> some scenarios, it may be necessary to reset the TPDM
> configurations to complete the verification of certain function.
> This change provides a node to reset the configurations and
> disable the TPDM if it has been enabled.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Tao Zhang <taozha@qti.qualcomm.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpdm.c | 32 ++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 6befc87..c29d667d 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -161,6 +161,37 @@ static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>   	}
>   }
>   
> +static ssize_t reset_store(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf,
> +					  size_t size)
> +{
> +	int ret = 0;
> +	unsigned long val;
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	ret = kstrtoul(buf, 0, &val);
> +	if (ret || (val != 1))
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	/* Reset all datasets to ZERO */
> +	if (drvdata->dsb != NULL)
> +		memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
> +
> +	/* Init the default data */

Code is self explanatory, don't need a comment here.

> +	tpdm_init_default_data(drvdata);

Could this be helper be renamed to

	tpdm_reset_dsb_data() ?

Because, that is what it does now.

> +
> +	spin_unlock(&drvdata->spinlock);
> +
> +	/* Disable tpdm if enabled */
> +	if (drvdata->enable)
> +		coresight_disable(drvdata->csdev);

Woh, where did that come from ? Don't you have a disable handle ?

> +
> +	return size;
> +}
> +static DEVICE_ATTR_WO(reset);
> +

This looks a bit pointless to me, given we have separate controls for 
all that is being achieved above.

Suzuki

>   /*
>    * value 1: 64 bits test data
>    * value 2: 32 bits test data
> @@ -201,6 +232,7 @@ static ssize_t integration_test_store(struct device *dev,
>   static DEVICE_ATTR_WO(integration_test);
>   
>   static struct attribute *tpdm_attrs[] = {
> +	&dev_attr_reset.attr,
>   	&dev_attr_integration_test.attr,
>   	NULL,
>   };
Tao Zhang March 1, 2023, 6:35 a.m. UTC | #2 
在 2/28/2023 7:22 PM, Suzuki K Poulose 写道:
> On 19/01/2023 07:41, Tao Zhang wrote:
>> TPDM device need a node to reset the configurations and status of
>> it. So as to avoid the previous configurations affecting the
>> current use, the configurations need to be reset first. And in
>> some scenarios, it may be necessary to reset the TPDM
>> configurations to complete the verification of certain function.
>> This change provides a node to reset the configurations and
>> disable the TPDM if it has been enabled.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Tao Zhang <taozha@qti.qualcomm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 32 
>> ++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 6befc87..c29d667d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -161,6 +161,37 @@ static void tpdm_init_default_data(struct 
>> tpdm_drvdata *drvdata)
>>       }
>>   }
>>   +static ssize_t reset_store(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      const char *buf,
>> +                      size_t size)
>> +{
>> +    int ret = 0;
>> +    unsigned long val;
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    ret = kstrtoul(buf, 0, &val);
>> +    if (ret || (val != 1))
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    /* Reset all datasets to ZERO */
>> +    if (drvdata->dsb != NULL)
>> +        memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
>> +
>> +    /* Init the default data */
>
> Code is self explanatory, don't need a comment here.
OK, got it.
>
>> +    tpdm_init_default_data(drvdata);
>
> Could this be helper be renamed to
>
>     tpdm_reset_dsb_data() ?
>
> Because, that is what it does now.

Sure, I will change its name as "tpdm_reset_data()" since DSB is only 
one of the data type which TPDM supports.

>
>> +
>> +    spin_unlock(&drvdata->spinlock);
>> +
>> +    /* Disable tpdm if enabled */
>> +    if (drvdata->enable)
>> +        coresight_disable(drvdata->csdev);
>
> Woh, where did that come from ? Don't you have a disable handle ?

The function "coresight_disable" is defined in the file "coresight-core.c".

This is a generic disablement handle.

>
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_WO(reset);
>> +
>
> This looks a bit pointless to me, given we have separate controls for 
> all that is being achieved above.

I will move the function of resetting all datasets to zero to 
"tpdm_disable".

Thanks for your advice.

>
> Suzuki
>
>>   /*
>>    * value 1: 64 bits test data
>>    * value 2: 32 bits test data
>> @@ -201,6 +232,7 @@ static ssize_t integration_test_store(struct 
>> device *dev,
>>   static DEVICE_ATTR_WO(integration_test);
>>     static struct attribute *tpdm_attrs[] = {
>> +    &dev_attr_reset.attr,
>>       &dev_attr_integration_test.attr,
>>       NULL,
>>   };
>
Best,

Tao
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 6befc87..c29d667d 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -161,6 +161,37 @@  static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
 	}
 }
 
+static ssize_t reset_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t size)
+{
+	int ret = 0;
+	unsigned long val;
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret || (val != 1))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	/* Reset all datasets to ZERO */
+	if (drvdata->dsb != NULL)
+		memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
+
+	/* Init the default data */
+	tpdm_init_default_data(drvdata);
+
+	spin_unlock(&drvdata->spinlock);
+
+	/* Disable tpdm if enabled */
+	if (drvdata->enable)
+		coresight_disable(drvdata->csdev);
+
+	return size;
+}
+static DEVICE_ATTR_WO(reset);
+
 /*
  * value 1: 64 bits test data
  * value 2: 32 bits test data
@@ -201,6 +232,7 @@  static ssize_t integration_test_store(struct device *dev,
 static DEVICE_ATTR_WO(integration_test);
 
 static struct attribute *tpdm_attrs[] = {
+	&dev_attr_reset.attr,
 	&dev_attr_integration_test.attr,
 	NULL,
 };