diff mbox series

[v3,04/11] coresight-tpdm: Add reset node to TPDM node

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

Commit Message

Tao Zhang March 23, 2023, 6:04 a.m. UTC 
  TPDM device need a node to reset the configurations and status of
it. 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>
---
 drivers/hwtracing/coresight/coresight-tpdm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Suzuki K Poulose March 23, 2023, 2:41 p.m. UTC | #1 
On 23/03/2023 06:04, Tao Zhang wrote:
> TPDM device need a node to reset the configurations and status of
> it. 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>
> ---
>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 5e1e2ba..104638d 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
>   	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>   }
>   
> +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, 10, &val);
> +	if (ret || val != 1)
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +	/* Reset all datasets to ZERO, and init the default data*/
> +	tpdm_init_datasets(drvdata);

With the suggested rename in the previous patch, you wouldn't need
a comment here.

> +
> +	spin_unlock(&drvdata->spinlock);
> +


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

Couldn't this be done via disable_source ? Please don't overload
the sysfs handle.

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

Documentation for the sysfs node please ?

Suzuki
Suzuki K Poulose March 23, 2023, 2:48 p.m. UTC | #2 
On 23/03/2023 14:41, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> TPDM device need a node to reset the configurations and status of
>> it. This change provides a node to reset the configurations and
>> disable the TPDM if it has been enabled.

Please justify why this "do everything" magic knob is required
when there are tunables for individual controls in the later
patches.

Suzuki

>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 
>> ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 5e1e2ba..104638d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct 
>> tpdm_drvdata *drvdata)
>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>>   }
>> +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, 10, &val);
>> +    if (ret || val != 1)
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    /* Reset all datasets to ZERO, and init the default data*/
>> +    tpdm_init_datasets(drvdata);
> 
> With the suggested rename in the previous patch, you wouldn't need
> a comment here.
> 
>> +
>> +    spin_unlock(&drvdata->spinlock);
>> +
> 
> 
>> +    /* Disable tpdm if enabled */
>> +    if (drvdata->enable)
>> +        coresight_disable(drvdata->csdev);
> 
> Couldn't this be done via disable_source ? Please don't overload
> the sysfs handle.
> 
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_WO(reset);
> 
> Documentation for the sysfs node please ?
> 
> Suzuki
>
Tao Zhang March 27, 2023, 6:59 a.m. UTC | #3 
Hi Suzuki,

On 3/23/2023 10:41 PM, Suzuki K Poulose wrote:
> On 23/03/2023 06:04, Tao Zhang wrote:
>> TPDM device need a node to reset the configurations and status of
>> it. 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>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 
>> ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 5e1e2ba..104638d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct 
>> tpdm_drvdata *drvdata)
>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>>   }
>>   +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, 10, &val);
>> +    if (ret || val != 1)
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    /* Reset all datasets to ZERO, and init the default data*/
>> +    tpdm_init_datasets(drvdata);
>
> With the suggested rename in the previous patch, you wouldn't need
> a comment here.
I will update this in the next patch series.
>
>> +
>> +    spin_unlock(&drvdata->spinlock);
>> +
>
>
>> +    /* Disable tpdm if enabled */
>> +    if (drvdata->enable)
>> +        coresight_disable(drvdata->csdev);
>
> Couldn't this be done via disable_source ? Please don't overload
> the sysfs handle.
I will update this in the next patch series.
>
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_WO(reset);
>
> Documentation for the sysfs node please ?
I will update this in the next patch series.
>
> Suzuki
>
Tao Zhang March 27, 2023, 7:11 a.m. UTC | #4 
Hi Suzuki,

On 3/23/2023 10:48 PM, Suzuki K Poulose wrote:
> On 23/03/2023 14:41, Suzuki K Poulose wrote:
>> On 23/03/2023 06:04, Tao Zhang wrote:
>>> TPDM device need a node to reset the configurations and status of
>>> it. This change provides a node to reset the configurations and
>>> disable the TPDM if it has been enabled.
>
> Please justify why this "do everything" magic knob is required
> when there are tunables for individual controls in the later
> patches.

We want to have a single knob that resets all the datasets, which saves the

need to configure the individual controls one by one. Since it is often 
necessary

to reset all the datasets, this knob will be more user-friendly.


Tao

>
> Suzuki
>
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-tpdm.c | 28 
>>> ++++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> index 5e1e2ba..104638d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> @@ -161,6 +161,33 @@ static void tpdm_datasets_setup(struct 
>>> tpdm_drvdata *drvdata)
>>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>>>   }
>>> +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, 10, &val);
>>> +    if (ret || val != 1)
>>> +        return -EINVAL;
>>> +
>>> +    spin_lock(&drvdata->spinlock);
>>> +    /* Reset all datasets to ZERO, and init the default data*/
>>> +    tpdm_init_datasets(drvdata);
>>
>> With the suggested rename in the previous patch, you wouldn't need
>> a comment here.
>>
>>> +
>>> +    spin_unlock(&drvdata->spinlock);
>>> +
>>
>>
>>> +    /* Disable tpdm if enabled */
>>> +    if (drvdata->enable)
>>> +        coresight_disable(drvdata->csdev);
>>
>> Couldn't this be done via disable_source ? Please don't overload
>> the sysfs handle.
>>
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_WO(reset);
>>
>> Documentation for the sysfs node please ?
>>
>> Suzuki
>>
>
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 5e1e2ba..104638d 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -161,6 +161,33 @@  static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
 	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
 }
 
+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, 10, &val);
+	if (ret || val != 1)
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	/* Reset all datasets to ZERO, and init the default data*/
+	tpdm_init_datasets(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 +228,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,
 };