diff mbox series

[v2,3/9] coresight-tpdm: Initialize DSB subunit configuration

Message ID 1674114105-16651-4-git-send-email-quic_taozha@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add support to configure TPDM DSB subunit | expand

Commit Message

Tao Zhang Jan. 19, 2023, 7:41 a.m. UTC 
DSB subunit need to be configured in enablement and disablement.
A struct that specifics associated to dsb dataset is needed. It
saves the configuration and parameters of the dsb datasets. This
change is to add this struct and initialize the configuration of
DSB subunit.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Tao Zhang <taozha@qti.qualcomm.com>
---
 drivers/hwtracing/coresight/coresight-tpdm.c | 57 ++++++++++++++++++++++++++--
 drivers/hwtracing/coresight/coresight-tpdm.h | 17 +++++++++
 2 files changed, 70 insertions(+), 4 deletions(-)

Comments

Suzuki K Poulose Feb. 28, 2023, 11:16 a.m. UTC | #1 
On 19/01/2023 07:41, Tao Zhang wrote:
> DSB subunit need to be configured in enablement and disablement.
> A struct that specifics associated to dsb dataset is needed. It
> saves the configuration and parameters of the dsb datasets. This
> change is to add this struct and initialize the configuration of
> DSB subunit.

Please could add a line about the type of things you can do with DSB.
e.g, Timestamp, trigger type etc ? The description seems to be 
describing the code, rather than the functionality of the code.

> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Tao Zhang <taozha@qti.qualcomm.com>

As pointed out on the previous response by someone else, please fix the
above to single.


> ---
>   drivers/hwtracing/coresight/coresight-tpdm.c | 57 ++++++++++++++++++++++++++--
>   drivers/hwtracing/coresight/coresight-tpdm.h | 17 +++++++++
>   2 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index d85ca96..6befc87 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -24,13 +24,35 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   {
>   	u32 val;
>   
> +	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
> +	/* Set trigger timestamp */
> +	if (drvdata->dsb->trig_ts)
> +		val |= TPDM_DSB_XTRIG_TSENAB;
> +	else
> +		val &= ~TPDM_DSB_XTRIG_TSENAB;
> +	writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
> +
> +	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
> +	/* Set trigger type */
> +	if (drvdata->dsb->trig_type)
> +		val |= TPDM_DSB_TRIG_TYPE;
> +	else
> +		val &= ~TPDM_DSB_TRIG_TYPE;
> +	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
> +
>   	/* Set the enable bit of DSB control register to 1 */
>   	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>   	val |= TPDM_DSB_CR_ENA;
>   	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);

Do they have to be written out separately ? Why not combine the value 
updates to the TPDM_DSB_CR ?


>   }
>   
> -/* TPDM enable operations */
> +/* TPDM enable operations

Minor nit: Comment style issues.


> + * The TPDM or Monitor serves as data collection component for various
> + * dataset types. It covers Basic Counts(BC), Tenure Counts(TC),
> + * Continuous Multi-Bit(CMB), Multi-lane CMB(MCMB) and Discrete Single
> + * Bit(DSB). This function will initialize the configuration according
> + * to the dataset type supported by the TPDM.
> + */
>   static void __tpdm_enable(struct tpdm_drvdata *drvdata)
>   {
>   	CS_UNLOCK(drvdata->base);
> @@ -110,15 +132,33 @@ static const struct coresight_ops tpdm_cs_ops = {
>   	.source_ops	= &tpdm_source_ops,
>   };
>   
> -static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
> +static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
>   {
>   	u32 pidr;
>   
> -	CS_UNLOCK(drvdata->base);
>   	/*  Get the datasets present on the TPDM. */
>   	pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
>   	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
> -	CS_LOCK(drvdata->base);
> +}
> +
> +static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata)
> +{
> +	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
> +		drvdata->dsb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->dsb),
> +					    GFP_KERNEL);
> +		if (!drvdata->dsb)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +

Couldn't this be moved into the init_default_data() ?

> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
> +{
> +	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
> +		drvdata->dsb->trig_ts = true;
> +		drvdata->dsb->trig_type = false;



> +	}
>   }

It looks a bit silly to move the initialisation to a separate function. 
Please could you fold this to tpdm_datasets_alloc() and may be even
rename that function to tpdm_init_datasets() ?

Suzuki

>   
>   /*
> @@ -181,6 +221,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   	struct coresight_platform_data *pdata;
>   	struct tpdm_drvdata *drvdata;
>   	struct coresight_desc desc = { 0 };
> +	int ret;
>   
>   	pdata = coresight_get_platform_data(dev);
>   	if (IS_ERR(pdata))
> @@ -200,6 +241,8 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   
>   	drvdata->base = base;
>   
> +	tpdm_datasets_setup(drvdata);
> +
>   	/* Set up coresight component description */
>   	desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>   	if (!desc.name)
> @@ -216,7 +259,13 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   		return PTR_ERR(drvdata->csdev);
>   
>   	spin_lock_init(&drvdata->spinlock);
> +	ret = tpdm_datasets_alloc(drvdata);
> +	if (ret) {
> +		coresight_unregister(drvdata->csdev);
> +		return ret;
> +	}
>   	tpdm_init_default_data(drvdata);
> +
>   	/* Decrease pm refcount when probe is done.*/
>   	pm_runtime_put(&adev->dev);
>   
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 5438540..3ad1be5 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -11,8 +11,14 @@
>   
>   /* DSB Subunit Registers */
>   #define TPDM_DSB_CR		(0x780)
> +#define TPDM_DSB_TIER		(0x784)
> +
>   /* Enable bit for DSB subunit */
>   #define TPDM_DSB_CR_ENA		BIT(0)
> +/* Enable bit for DSB subunit trigger timestamp */
> +#define TPDM_DSB_XTRIG_TSENAB		BIT(1)
> +/* Enable bit for DSB subunit trigger type */
> +#define TPDM_DSB_TRIG_TYPE		BIT(12)
>   
>   /* TPDM integration test registers */
>   #define TPDM_ITATBCNTRL		(0xEF0)
> @@ -41,6 +47,16 @@
>   #define TPDM_PIDR0_DS_DSB	BIT(1)
>   
>   /**
> + * struct dsb_dataset - specifics associated to dsb dataset
> + * @trig_ts:          Enable/Disable trigger timestamp.
> + * @trig_type:        Enable/Disable trigger type.
> + */
> +struct dsb_dataset {
> +	bool			trig_ts;
> +	bool			trig_type;
> +};
> +
> +/**
>    * struct tpdm_drvdata - specifics associated to an TPDM component
>    * @base:       memory mapped base address for this component.
>    * @dev:        The device entity associated to this component.
> @@ -57,6 +73,7 @@ struct tpdm_drvdata {
>   	spinlock_t		spinlock;
>   	bool			enable;
>   	unsigned long		datasets;
> +	struct dsb_dataset	*dsb;
>   };
>   
>   #endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
Tao Zhang March 1, 2023, 5:57 a.m. UTC | #2 
Hi，

在 2/28/2023 7:16 PM, Suzuki K Poulose 写道:
> On 19/01/2023 07:41, Tao Zhang wrote:
>> DSB subunit need to be configured in enablement and disablement.
>> A struct that specifics associated to dsb dataset is needed. It
>> saves the configuration and parameters of the dsb datasets. This
>> change is to add this struct and initialize the configuration of
>> DSB subunit.
>
> Please could add a line about the type of things you can do with DSB.
> e.g, Timestamp, trigger type etc ? The description seems to be 
> describing the code, rather than the functionality of the code.
>
Sure, I will add more information according to your advice in the next 
version of the patch.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Tao Zhang <taozha@qti.qualcomm.com>
>
> As pointed out on the previous response by someone else, please fix the
> above to single.
>
>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 57 
>> ++++++++++++++++++++++++++--
>>   drivers/hwtracing/coresight/coresight-tpdm.h | 17 +++++++++
>>   2 files changed, 70 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index d85ca96..6befc87 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -24,13 +24,35 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>> *drvdata)
>>   {
>>       u32 val;
>>   +    val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
>> +    /* Set trigger timestamp */
>> +    if (drvdata->dsb->trig_ts)
>> +        val |= TPDM_DSB_XTRIG_TSENAB;
>> +    else
>> +        val &= ~TPDM_DSB_XTRIG_TSENAB;
>> +    writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
>> +
>> +    val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>> +    /* Set trigger type */
>> +    if (drvdata->dsb->trig_type)
>> +        val |= TPDM_DSB_TRIG_TYPE;
>> +    else
>> +        val &= ~TPDM_DSB_TRIG_TYPE;
>> +    writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>> +
>>       /* Set the enable bit of DSB control register to 1 */
>>       val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>>       val |= TPDM_DSB_CR_ENA;
>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>
> Do they have to be written out separately ? Why not combine the value 
> updates to the TPDM_DSB_CR ?
>
Sure, I will combine the value in the next version of the patch.
>
>>   }
>>   -/* TPDM enable operations */
>> +/* TPDM enable operations
>
> Minor nit: Comment style issues.
>
>
>> + * The TPDM or Monitor serves as data collection component for various
>> + * dataset types. It covers Basic Counts(BC), Tenure Counts(TC),
>> + * Continuous Multi-Bit(CMB), Multi-lane CMB(MCMB) and Discrete Single
>> + * Bit(DSB). This function will initialize the configuration according
>> + * to the dataset type supported by the TPDM.
>> + */
>>   static void __tpdm_enable(struct tpdm_drvdata *drvdata)
>>   {
>>       CS_UNLOCK(drvdata->base);
>> @@ -110,15 +132,33 @@ static const struct coresight_ops tpdm_cs_ops = {
>>       .source_ops    = &tpdm_source_ops,
>>   };
>>   -static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>> +static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
>>   {
>>       u32 pidr;
>>   -    CS_UNLOCK(drvdata->base);
>>       /*  Get the datasets present on the TPDM. */
>>       pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
>>       drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>> -    CS_LOCK(drvdata->base);
>> +}
>> +
>> +static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata)
>> +{
>> +    if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
>> +        drvdata->dsb = devm_kzalloc(drvdata->dev, 
>> sizeof(*drvdata->dsb),
>> +                        GFP_KERNEL);
>> +        if (!drvdata->dsb)
>> +            return -ENOMEM;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>
> Couldn't this be moved into the init_default_data() ?
>
>> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>> +{
>> +    if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
>> +        drvdata->dsb->trig_ts = true;
>> +        drvdata->dsb->trig_type = false;
>
>
>
>> +    }
>>   }
>
> It looks a bit silly to move the initialisation to a separate 
> function. Please could you fold this to tpdm_datasets_alloc() and may 
> be even
> rename that function to tpdm_init_datasets() ?
Sure, I will update the code according to your advice in the next 
version of the patch.
>
> Suzuki
>
>>     /*
>> @@ -181,6 +221,7 @@ static int tpdm_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>       struct coresight_platform_data *pdata;
>>       struct tpdm_drvdata *drvdata;
>>       struct coresight_desc desc = { 0 };
>> +    int ret;
>>         pdata = coresight_get_platform_data(dev);
>>       if (IS_ERR(pdata))
>> @@ -200,6 +241,8 @@ static int tpdm_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>         drvdata->base = base;
>>   +    tpdm_datasets_setup(drvdata);
>> +
>>       /* Set up coresight component description */
>>       desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>>       if (!desc.name)
>> @@ -216,7 +259,13 @@ static int tpdm_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>           return PTR_ERR(drvdata->csdev);
>>         spin_lock_init(&drvdata->spinlock);
>> +    ret = tpdm_datasets_alloc(drvdata);
>> +    if (ret) {
>> +        coresight_unregister(drvdata->csdev);
>> +        return ret;
>> +    }
>>       tpdm_init_default_data(drvdata);
>> +
>>       /* Decrease pm refcount when probe is done.*/
>>       pm_runtime_put(&adev->dev);
>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 5438540..3ad1be5 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -11,8 +11,14 @@
>>     /* DSB Subunit Registers */
>>   #define TPDM_DSB_CR        (0x780)
>> +#define TPDM_DSB_TIER        (0x784)
>> +
>>   /* Enable bit for DSB subunit */
>>   #define TPDM_DSB_CR_ENA        BIT(0)
>> +/* Enable bit for DSB subunit trigger timestamp */
>> +#define TPDM_DSB_XTRIG_TSENAB        BIT(1)
>> +/* Enable bit for DSB subunit trigger type */
>> +#define TPDM_DSB_TRIG_TYPE        BIT(12)
>>     /* TPDM integration test registers */
>>   #define TPDM_ITATBCNTRL        (0xEF0)
>> @@ -41,6 +47,16 @@
>>   #define TPDM_PIDR0_DS_DSB    BIT(1)
>>     /**
>> + * struct dsb_dataset - specifics associated to dsb dataset
>> + * @trig_ts:          Enable/Disable trigger timestamp.
>> + * @trig_type:        Enable/Disable trigger type.
>> + */
>> +struct dsb_dataset {
>> +    bool            trig_ts;
>> +    bool            trig_type;
>> +};
>> +
>> +/**
>>    * struct tpdm_drvdata - specifics associated to an TPDM component
>>    * @base:       memory mapped base address for this component.
>>    * @dev:        The device entity associated to this component.
>> @@ -57,6 +73,7 @@ struct tpdm_drvdata {
>>       spinlock_t        spinlock;
>>       bool            enable;
>>       unsigned long        datasets;
>> +    struct dsb_dataset    *dsb;
>>   };
>>     #endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
>
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index d85ca96..6befc87 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -24,13 +24,35 @@  static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 {
 	u32 val;
 
+	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
+	/* Set trigger timestamp */
+	if (drvdata->dsb->trig_ts)
+		val |= TPDM_DSB_XTRIG_TSENAB;
+	else
+		val &= ~TPDM_DSB_XTRIG_TSENAB;
+	writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
+
+	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
+	/* Set trigger type */
+	if (drvdata->dsb->trig_type)
+		val |= TPDM_DSB_TRIG_TYPE;
+	else
+		val &= ~TPDM_DSB_TRIG_TYPE;
+	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
+
 	/* Set the enable bit of DSB control register to 1 */
 	val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
 	val |= TPDM_DSB_CR_ENA;
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
-/* TPDM enable operations */
+/* TPDM enable operations
+ * The TPDM or Monitor serves as data collection component for various
+ * dataset types. It covers Basic Counts(BC), Tenure Counts(TC),
+ * Continuous Multi-Bit(CMB), Multi-lane CMB(MCMB) and Discrete Single
+ * Bit(DSB). This function will initialize the configuration according
+ * to the dataset type supported by the TPDM.
+ */
 static void __tpdm_enable(struct tpdm_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);
@@ -110,15 +132,33 @@  static const struct coresight_ops tpdm_cs_ops = {
 	.source_ops	= &tpdm_source_ops,
 };
 
-static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
+static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
 {
 	u32 pidr;
 
-	CS_UNLOCK(drvdata->base);
 	/*  Get the datasets present on the TPDM. */
 	pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
 	drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
-	CS_LOCK(drvdata->base);
+}
+
+static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata)
+{
+	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
+		drvdata->dsb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->dsb),
+					    GFP_KERNEL);
+		if (!drvdata->dsb)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
+{
+	if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
+		drvdata->dsb->trig_ts = true;
+		drvdata->dsb->trig_type = false;
+	}
 }
 
 /*
@@ -181,6 +221,7 @@  static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 	struct coresight_platform_data *pdata;
 	struct tpdm_drvdata *drvdata;
 	struct coresight_desc desc = { 0 };
+	int ret;
 
 	pdata = coresight_get_platform_data(dev);
 	if (IS_ERR(pdata))
@@ -200,6 +241,8 @@  static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 
 	drvdata->base = base;
 
+	tpdm_datasets_setup(drvdata);
+
 	/* Set up coresight component description */
 	desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
 	if (!desc.name)
@@ -216,7 +259,13 @@  static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(drvdata->csdev);
 
 	spin_lock_init(&drvdata->spinlock);
+	ret = tpdm_datasets_alloc(drvdata);
+	if (ret) {
+		coresight_unregister(drvdata->csdev);
+		return ret;
+	}
 	tpdm_init_default_data(drvdata);
+
 	/* Decrease pm refcount when probe is done.*/
 	pm_runtime_put(&adev->dev);
 
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 5438540..3ad1be5 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -11,8 +11,14 @@ 
 
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
+#define TPDM_DSB_TIER		(0x784)
+
 /* Enable bit for DSB subunit */
 #define TPDM_DSB_CR_ENA		BIT(0)
+/* Enable bit for DSB subunit trigger timestamp */
+#define TPDM_DSB_XTRIG_TSENAB		BIT(1)
+/* Enable bit for DSB subunit trigger type */
+#define TPDM_DSB_TRIG_TYPE		BIT(12)
 
 /* TPDM integration test registers */
 #define TPDM_ITATBCNTRL		(0xEF0)
@@ -41,6 +47,16 @@ 
 #define TPDM_PIDR0_DS_DSB	BIT(1)
 
 /**
+ * struct dsb_dataset - specifics associated to dsb dataset
+ * @trig_ts:          Enable/Disable trigger timestamp.
+ * @trig_type:        Enable/Disable trigger type.
+ */
+struct dsb_dataset {
+	bool			trig_ts;
+	bool			trig_type;
+};
+
+/**
  * struct tpdm_drvdata - specifics associated to an TPDM component
  * @base:       memory mapped base address for this component.
  * @dev:        The device entity associated to this component.
@@ -57,6 +73,7 @@  struct tpdm_drvdata {
 	spinlock_t		spinlock;
 	bool			enable;
 	unsigned long		datasets;
+	struct dsb_dataset	*dsb;
 };
 
 #endif  /* _CORESIGHT_CORESIGHT_TPDM_H */