[v4,02/11] coresight-tpda: Add DSB dataset support

  Read the DSB element size from the device tree. Set the register
bit that controls the DSB element size of the corresponding port.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-core.c |  1 +
 drivers/hwtracing/coresight/coresight-tpda.c | 92 +++++++++++++++++++++++++---
 drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
 drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
 include/linux/coresight.h                    |  1 +
 5 files changed, 90 insertions(+), 10 deletions(-)
  

On 27/04/2023 10:00, Tao Zhang wrote:
> Read the DSB element size from the device tree. Set the register
> bit that controls the DSB element size of the corresponding port.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c |  1 +
>   drivers/hwtracing/coresight/coresight-tpda.c | 92 +++++++++++++++++++++++++---
>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>   drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
>   include/linux/coresight.h                    |  1 +
>   5 files changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 2af416b..f1eacbb 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
>   
>   	if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>   	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
> +		subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>   	    subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>   		dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
>   		return -EINVAL;

Please see the comment at the bottom.

> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 8d2b9d2..af9c72f 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -21,6 +21,56 @@
>   
>   DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>   
> +/* Search and read element data size from the TPDM node in
> + * the devicetree. Each input port of TPDA is connected to
> + * a TPDM. Different TPDM supports different types of dataset,
> + * and some may support more than one type of dataset.
> + * Parameter "inport" is used to pass in the input port number
> + * of TPDA, and it is set to 0 in the recursize call.
> + * Parameter "parent" is used to pass in the original call.
> + */
> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
> +			   struct coresight_device *csdev, int inport, bool parent)
> +{
> +	static int nr_inport;
> +	int i;
> +	static bool tpdm_found;
> +	struct coresight_device *in_csdev;
> +
> +	if (inport > (TPDA_MAX_INPORTS - 1))
> +		return -EINVAL;
> +
> +	if (parent) {
> +		nr_inport = inport;
> +		tpdm_found = false;
> +	}
> +
> +	for (i = 0; i < csdev->pdata->nr_inconns; i++) {
> +		in_csdev = csdev->pdata->in_conns[i]->src_dev;
> +		if (!in_csdev)
> +			break;
> +
> +		if (parent)
> +			if (csdev->pdata->in_conns[i]->dest_port != inport)
> +				continue;
> +
> +		if (in_csdev->subtype.source_subtype

We must match the in_csdev->type to be SOURCE && the subtype.

> +				   == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) {
> +			of_property_read_u8(in_csdev->dev.parent->of_node,
> +					"qcom,dsb-element-size", &drvdata->dsb_esize[nr_inport]);
> +			if (!tpdm_found)
> +				tpdm_found = true;
> +			else
> +				dev_warn(drvdata->dev,
> +					"More than one TPDM is mapped to the TPDA input port %d.\n",
> +					nr_inport);

When we know, we have found a source device, we don't need to recurse
down and could simply 'continue' to the next one in the list and skip
the call below.

> +		}
> +		tpda_set_element_size(drvdata, in_csdev, 0, false);
> +	}
> +
> +	return 0;
> +}
> +
>   /* Settings pre enabling port control register */
>   static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>   {
> @@ -32,26 +82,43 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>   	writel_relaxed(val, drvdata->base + TPDA_CR);
>   }
>   
> -static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
> +static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>   {
>   	u32 val;
>   
>   	val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
> +	/*
> +	 * Configure aggregator port n DSB data set element size
> +	 * Set the bit to 0 if the size is 32
> +	 * Set the bit to 1 if the size is 64
> +	 */
> +	if (drvdata->dsb_esize[port] == 32)
> +		val &= ~TPDA_Pn_CR_DSBSIZE;
> +	else if (drvdata->dsb_esize[port] == 64)
> +		val |= TPDA_Pn_CR_DSBSIZE;
> +	else
> +		return -EINVAL;
> +
>   	/* Enable the port */
>   	val |= TPDA_Pn_CR_ENA;
>   	writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> +
> +	return 0;
>   }
>   
> -static void __tpda_enable(struct tpda_drvdata *drvdata, int port)
> +static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
>   {
> +	int ret;
> +
>   	CS_UNLOCK(drvdata->base);
>   
>   	if (!drvdata->csdev->enable)
>   		tpda_enable_pre_port(drvdata);
>   
> -	tpda_enable_port(drvdata, port);
> -
> +	ret = tpda_enable_port(drvdata, port);
>   	CS_LOCK(drvdata->base);
> +
> +	return ret;
>   }
>   
>   static int tpda_enable(struct coresight_device *csdev,
> @@ -59,16 +126,23 @@ static int tpda_enable(struct coresight_device *csdev,
>   		       struct coresight_connection *out)
>   {
>   	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	int ret;
> +
> +	ret = tpda_set_element_size(drvdata, csdev, in->dest_port, true);
> +	if (ret)
> +		return ret;
>   
>   	spin_lock(&drvdata->spinlock);
> -	if (atomic_read(&in->dest_refcnt) == 0)
> +	if (atomic_read(&in->dest_refcnt) == 0) {
>   		__tpda_enable(drvdata, in->dest_port);

ret = ... ?

> +		if (!ret) {



> +			atomic_inc(&in->dest_refcnt);
> +			dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
> +		}
> +	}

>   
> -	atomic_inc(&in->dest_refcnt);

This seems wrong, as we may fail to hold additional refcounts for the
additional sessions ?

>   	spin_unlock(&drvdata->spinlock);
> -
> -	dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
> -	return 0;
> +	return ret;
>   }
>   
>   static void __tpda_disable(struct tpda_drvdata *drvdata, int port)
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index 0399678..7332e9c 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.h
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -10,6 +10,8 @@
>   #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
>   /* Aggregator port enable bit */
>   #define TPDA_Pn_CR_ENA		BIT(0)
> +/* Aggregator port DSB data set element size bit */
> +#define TPDA_Pn_CR_DSBSIZE		BIT(8)
>   
>   #define TPDA_MAX_INPORTS	32
>   
> @@ -23,6 +25,7 @@
>    * @csdev:      component vitals needed by the framework.
>    * @spinlock:   lock for the drvdata value.
>    * @enable:     enable status of the component.
> + * @dsb_esize:  DSB element size, it must be 32 or 64.

minor nit:

DSB element size for each inport, it must be 32 or 64

>    */
>   struct tpda_drvdata {
>   	void __iomem		*base;
> @@ -30,6 +33,7 @@ struct tpda_drvdata {
>   	struct coresight_device	*csdev;
>   	spinlock_t		spinlock;
>   	u8			atid;
> +	u8			dsb_esize[TPDA_MAX_INPORTS];
>   };
>   
>   #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index f4854af..ba1867f 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -205,7 +205,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>   	if (!desc.name)
>   		return -ENOMEM;
>   	desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> -	desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
> +	desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM;
>   	desc.ops = &tpdm_cs_ops;
>   	desc.pdata = adev->dev.platform_data;
>   	desc.dev = &adev->dev;

Please could you split this change, i.e., introduction of
SUBTYPE_SOURCE_TPDM and using this in TPDM driver,
  in a separate patch before this change.

> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 225a5fa..6563896 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -60,6 +60,7 @@ enum coresight_dev_subtype_source {
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_PROC,
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
> +	CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM,
>   	CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS,
>   };
>
  

On 23/05/2023 11:07, Suzuki K Poulose wrote:
> On 27/04/2023 10:00, Tao Zhang wrote:
>> Read the DSB element size from the device tree. Set the register
>> bit that controls the DSB element size of the corresponding port.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c |  1 +
>>   drivers/hwtracing/coresight/coresight-tpda.c | 92 
>> +++++++++++++++++++++++++---
>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>   drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
>>   include/linux/coresight.h                    |  1 +
>>   5 files changed, 90 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 2af416b..f1eacbb 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct 
>> coresight_device *csdev,
>>       if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>> +        subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>>           dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
>>           return -EINVAL;
> 
> Please see the comment at the bottom.
> 
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>> b/drivers/hwtracing/coresight/coresight-tpda.c
>> index 8d2b9d2..af9c72f 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -21,6 +21,56 @@
>>   DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>> +/* Search and read element data size from the TPDM node in
>> + * the devicetree. Each input port of TPDA is connected to
>> + * a TPDM. Different TPDM supports different types of dataset,
>> + * and some may support more than one type of dataset.
>> + * Parameter "inport" is used to pass in the input port number
>> + * of TPDA, and it is set to 0 in the recursize call.
>> + * Parameter "parent" is used to pass in the original call.
>> + */
>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>> +               struct coresight_device *csdev, int inport, bool parent)

The name parent is a bit confusing. It could imply parent device ? That
is kind of inverse ? because, parent = true, indicates the parent device
of tpda, which is not true. Could we simply say

bool match_inport => When true, the dest_port of the connection from the
csdev must match the inport ? And ...

>> +{
>> +    static int nr_inport;
>> +    int i;
>> +    static bool tpdm_found;
>> +    struct coresight_device *in_csdev;
>> +
>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>> +        return -EINVAL;
>> +
>> +    if (parent) {
>> +        nr_inport = inport;
>> +        tpdm_found = false;
>> +    }
>> +
>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>> +        in_csdev = csdev->pdata->in_conns[i]->src_dev;
>> +        if (!in_csdev)
>> +            break;
>> +
>> +        if (parent)
>> +            if (csdev->pdata->in_conns[i]->dest_port != inport)
>> +                continue;

The above can become :

	    if (match_inport &&
		csdev->pdata->in_conns[i]->dest_port != inport)
		continue;

Suzuki
  

On 5/23/2023 6:07 PM, Suzuki K Poulose wrote:
> On 27/04/2023 10:00, Tao Zhang wrote:
>> Read the DSB element size from the device tree. Set the register
>> bit that controls the DSB element size of the corresponding port.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c |  1 +
>>   drivers/hwtracing/coresight/coresight-tpda.c | 92 
>> +++++++++++++++++++++++++---
>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>   drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
>>   include/linux/coresight.h                    |  1 +
>>   5 files changed, 90 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 2af416b..f1eacbb 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct 
>> coresight_device *csdev,
>>         if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>> +        subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>>           dev_err(&csdev->dev, "wrong device subtype in %s\n", 
>> function);
>>           return -EINVAL;
>
> Please see the comment at the bottom.
>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>> b/drivers/hwtracing/coresight/coresight-tpda.c
>> index 8d2b9d2..af9c72f 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -21,6 +21,56 @@
>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>   +/* Search and read element data size from the TPDM node in
>> + * the devicetree. Each input port of TPDA is connected to
>> + * a TPDM. Different TPDM supports different types of dataset,
>> + * and some may support more than one type of dataset.
>> + * Parameter "inport" is used to pass in the input port number
>> + * of TPDA, and it is set to 0 in the recursize call.
>> + * Parameter "parent" is used to pass in the original call.
>> + */
>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>> +               struct coresight_device *csdev, int inport, bool parent)
>> +{
>> +    static int nr_inport;
>> +    int i;
>> +    static bool tpdm_found;
>> +    struct coresight_device *in_csdev;
>> +
>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>> +        return -EINVAL;
>> +
>> +    if (parent) {
>> +        nr_inport = inport;
>> +        tpdm_found = false;
>> +    }
>> +
>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>> +        in_csdev = csdev->pdata->in_conns[i]->src_dev;
>> +        if (!in_csdev)
>> +            break;
>> +
>> +        if (parent)
>> +            if (csdev->pdata->in_conns[i]->dest_port != inport)
>> +                continue;
>> +
>> +        if (in_csdev->subtype.source_subtype
>
> We must match the in_csdev->type to be SOURCE && the subtype.
Sure, I will update it in the next patch series.
>
>> +                   == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) {
>> + of_property_read_u8(in_csdev->dev.parent->of_node,
>> +                    "qcom,dsb-element-size", 
>> &drvdata->dsb_esize[nr_inport]);
>> +            if (!tpdm_found)
>> +                tpdm_found = true;
>> +            else
>> +                dev_warn(drvdata->dev,
>> +                    "More than one TPDM is mapped to the TPDA input 
>> port %d.\n",
>> +                    nr_inport);
>
> When we know, we have found a source device, we don't need to recurse
> down and could simply 'continue' to the next one in the list and skip
> the call below.

Actually, one input port on TPDA only can connect to one TPDM. In the 
current design, it will

find out all the TPDMs on one input port and warn the users all the 
TPDMs it found. If we

replace 'recurse down' as 'continue' here, it may not find some TPDMs 
that might be connected

incorrectly.

>
>> +        }
>> +        tpda_set_element_size(drvdata, in_csdev, 0, false);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /* Settings pre enabling port control register */
>>   static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>>   {
>> @@ -32,26 +82,43 @@ static void tpda_enable_pre_port(struct 
>> tpda_drvdata *drvdata)
>>       writel_relaxed(val, drvdata->base + TPDA_CR);
>>   }
>>   -static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>> +static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>>   {
>>       u32 val;
>>         val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
>> +    /*
>> +     * Configure aggregator port n DSB data set element size
>> +     * Set the bit to 0 if the size is 32
>> +     * Set the bit to 1 if the size is 64
>> +     */
>> +    if (drvdata->dsb_esize[port] == 32)
>> +        val &= ~TPDA_Pn_CR_DSBSIZE;
>> +    else if (drvdata->dsb_esize[port] == 64)
>> +        val |= TPDA_Pn_CR_DSBSIZE;
>> +    else
>> +        return -EINVAL;
>> +
>>       /* Enable the port */
>>       val |= TPDA_Pn_CR_ENA;
>>       writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
>> +
>> +    return 0;
>>   }
>>   -static void __tpda_enable(struct tpda_drvdata *drvdata, int port)
>> +static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
>>   {
>> +    int ret;
>> +
>>       CS_UNLOCK(drvdata->base);
>>         if (!drvdata->csdev->enable)
>>           tpda_enable_pre_port(drvdata);
>>   -    tpda_enable_port(drvdata, port);
>> -
>> +    ret = tpda_enable_port(drvdata, port);
>>       CS_LOCK(drvdata->base);
>> +
>> +    return ret;
>>   }
>>     static int tpda_enable(struct coresight_device *csdev,
>> @@ -59,16 +126,23 @@ static int tpda_enable(struct coresight_device 
>> *csdev,
>>                  struct coresight_connection *out)
>>   {
>>       struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    int ret;
>> +
>> +    ret = tpda_set_element_size(drvdata, csdev, in->dest_port, true);
>> +    if (ret)
>> +        return ret;
>>         spin_lock(&drvdata->spinlock);
>> -    if (atomic_read(&in->dest_refcnt) == 0)
>> +    if (atomic_read(&in->dest_refcnt) == 0) {
>>           __tpda_enable(drvdata, in->dest_port);
>
> ret = ... ?
Sure, I will update it in the next patch series.
>
>> +        if (!ret) {
>
>
>
>> + atomic_inc(&in->dest_refcnt);
>> +            dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", 
>> in->dest_port);
>> +        }
>> +    }
>
>>   - atomic_inc(&in->dest_refcnt);
>
> This seems wrong, as we may fail to hold additional refcounts for the
> additional sessions ?

In the current design, if the TPDA is enabled successfully, it will run 
"atomic_inc(&in->dest_refcnt);"

Otherwise, it will not run "atomic_inc(&in->dest_refcnt);" to avoid 
additional refcounts. Is it right?

>
>> spin_unlock(&drvdata->spinlock);
>> -
>> -    dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
>> -    return 0;
>> +    return ret;
>>   }
>>     static void __tpda_disable(struct tpda_drvdata *drvdata, int port)
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h 
>> b/drivers/hwtracing/coresight/coresight-tpda.h
>> index 0399678..7332e9c 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>> @@ -10,6 +10,8 @@
>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>   /* Aggregator port enable bit */
>>   #define TPDA_Pn_CR_ENA        BIT(0)
>> +/* Aggregator port DSB data set element size bit */
>> +#define TPDA_Pn_CR_DSBSIZE        BIT(8)
>>     #define TPDA_MAX_INPORTS    32
>>   @@ -23,6 +25,7 @@
>>    * @csdev:      component vitals needed by the framework.
>>    * @spinlock:   lock for the drvdata value.
>>    * @enable:     enable status of the component.
>> + * @dsb_esize:  DSB element size, it must be 32 or 64.
>
> minor nit:
>
> DSB element size for each inport, it must be 32 or 64
Sure, I will update it in the next patch series.
>
>>    */
>>   struct tpda_drvdata {
>>       void __iomem        *base;
>> @@ -30,6 +33,7 @@ struct tpda_drvdata {
>>       struct coresight_device    *csdev;
>>       spinlock_t        spinlock;
>>       u8            atid;
>> +    u8            dsb_esize[TPDA_MAX_INPORTS];
>>   };
>>     #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index f4854af..ba1867f 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -205,7 +205,7 @@ static int tpdm_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>       if (!desc.name)
>>           return -ENOMEM;
>>       desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>> -    desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>> +    desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM;
>>       desc.ops = &tpdm_cs_ops;
>>       desc.pdata = adev->dev.platform_data;
>>       desc.dev = &adev->dev;
>
> Please could you split this change, i.e., introduction of
> SUBTYPE_SOURCE_TPDM and using this in TPDM driver,
>  in a separate patch before this change.

Sure, I will update it in the next patch series.


Best,

Tao

>
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index 225a5fa..6563896 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -60,6 +60,7 @@ enum coresight_dev_subtype_source {
>>       CORESIGHT_DEV_SUBTYPE_SOURCE_PROC,
>>       CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
>>       CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
>> +    CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM,
>>       CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS,
>>   };
>
  

On 5/23/2023 10:48 PM, Suzuki K Poulose wrote:
> On 23/05/2023 11:07, Suzuki K Poulose wrote:
>> On 27/04/2023 10:00, Tao Zhang wrote:
>>> Read the DSB element size from the device tree. Set the register
>>> bit that controls the DSB element size of the corresponding port.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-core.c |  1 +
>>>   drivers/hwtracing/coresight/coresight-tpda.c | 92 
>>> +++++++++++++++++++++++++---
>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>   drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
>>>   include/linux/coresight.h                    |  1 +
>>>   5 files changed, 90 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>>> b/drivers/hwtracing/coresight/coresight-core.c
>>> index 2af416b..f1eacbb 100644
>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct 
>>> coresight_device *csdev,
>>>       if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>>> +        subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>>>           dev_err(&csdev->dev, "wrong device subtype in %s\n", 
>>> function);
>>>           return -EINVAL;
>>
>> Please see the comment at the bottom.
>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>> index 8d2b9d2..af9c72f 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -21,6 +21,56 @@
>>>   DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>> +/* Search and read element data size from the TPDM node in
>>> + * the devicetree. Each input port of TPDA is connected to
>>> + * a TPDM. Different TPDM supports different types of dataset,
>>> + * and some may support more than one type of dataset.
>>> + * Parameter "inport" is used to pass in the input port number
>>> + * of TPDA, and it is set to 0 in the recursize call.
>>> + * Parameter "parent" is used to pass in the original call.
>>> + */
>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>> +               struct coresight_device *csdev, int inport, bool 
>>> parent)
>
> The name parent is a bit confusing. It could imply parent device ? That
> is kind of inverse ? because, parent = true, indicates the parent device
> of tpda, which is not true. Could we simply say
>
> bool match_inport => When true, the dest_port of the connection from the
> csdev must match the inport ? And ...
>
Sure, I will update this in the next patch series.
>>> +{
>>> +    static int nr_inport;
>>> +    int i;
>>> +    static bool tpdm_found;
>>> +    struct coresight_device *in_csdev;
>>> +
>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>> +        return -EINVAL;
>>> +
>>> +    if (parent) {
>>> +        nr_inport = inport;
>>> +        tpdm_found = false;
>>> +    }
>>> +
>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>> +        in_csdev = csdev->pdata->in_conns[i]->src_dev;
>>> +        if (!in_csdev)
>>> +            break;
>>> +
>>> +        if (parent)
>>> +            if (csdev->pdata->in_conns[i]->dest_port != inport)
>>> +                continue;
>
> The above can become :
>
>         if (match_inport &&
>         csdev->pdata->in_conns[i]->dest_port != inport)
>         continue;

Sure, I will update this in the next patch series.


Best,

Tao

>
> Suzuki
>
  

On 25/05/2023 08:16, Tao Zhang wrote:
> 
> On 5/23/2023 6:07 PM, Suzuki K Poulose wrote:
>> On 27/04/2023 10:00, Tao Zhang wrote:
>>> Read the DSB element size from the device tree. Set the register
>>> bit that controls the DSB element size of the corresponding port.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-core.c |  1 +
>>>   drivers/hwtracing/coresight/coresight-tpda.c | 92 
>>> +++++++++++++++++++++++++---
>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>   drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
>>>   include/linux/coresight.h                    |  1 +
>>>   5 files changed, 90 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>>> b/drivers/hwtracing/coresight/coresight-core.c
>>> index 2af416b..f1eacbb 100644
>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct 
>>> coresight_device *csdev,
>>>         if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>>> +        subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>>>           dev_err(&csdev->dev, "wrong device subtype in %s\n", 
>>> function);
>>>           return -EINVAL;
>>
>> Please see the comment at the bottom.
>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>> index 8d2b9d2..af9c72f 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -21,6 +21,56 @@
>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>   +/* Search and read element data size from the TPDM node in
>>> + * the devicetree. Each input port of TPDA is connected to
>>> + * a TPDM. Different TPDM supports different types of dataset,
>>> + * and some may support more than one type of dataset.
>>> + * Parameter "inport" is used to pass in the input port number
>>> + * of TPDA, and it is set to 0 in the recursize call.
>>> + * Parameter "parent" is used to pass in the original call.
>>> + */
>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>> +               struct coresight_device *csdev, int inport, bool parent)
>>> +{
>>> +    static int nr_inport;
>>> +    int i;
>>> +    static bool tpdm_found;
>>> +    struct coresight_device *in_csdev;
>>> +
>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>> +        return -EINVAL;
>>> +
>>> +    if (parent) {
>>> +        nr_inport = inport;
>>> +        tpdm_found = false;
>>> +    }
>>> +
>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>> +        in_csdev = csdev->pdata->in_conns[i]->src_dev;
>>> +        if (!in_csdev)
>>> +            break;
>>> +
>>> +        if (parent)
>>> +            if (csdev->pdata->in_conns[i]->dest_port != inport)
>>> +                continue;
>>> +
>>> +        if (in_csdev->subtype.source_subtype
>>
>> We must match the in_csdev->type to be SOURCE && the subtype.
> Sure, I will update it in the next patch series.
>>
>>> +                   == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) {
>>> + of_property_read_u8(in_csdev->dev.parent->of_node,
>>> +                    "qcom,dsb-element-size", 
>>> &drvdata->dsb_esize[nr_inport]);
>>> +            if (!tpdm_found)
>>> +                tpdm_found = true;
>>> +            else
>>> +                dev_warn(drvdata->dev,
>>> +                    "More than one TPDM is mapped to the TPDA input 
>>> port %d.\n",
>>> +                    nr_inport);
>>
>> When we know, we have found a source device, we don't need to recurse
>> down and could simply 'continue' to the next one in the list and skip
>> the call below.
> 
> Actually, one input port on TPDA only can connect to one TPDM. In the 
> current design, it will
> 
> find out all the TPDMs on one input port and warn the users all the 
> TPDMs it found. If we
> 
> replace 'recurse down' as 'continue' here, it may not find some TPDMs 
> that might be connected
> 
> incorrectly.


What do you mean ? When you enter the if () above, the in_csdev is a
source and it is TPDM. There must be no input connections TPDM, i.e.
in_csdev, so no need to go further up the connection chain looking at
the in_csdev. The loop will continue to analyse this device (where we
found one TPDM already) and detect any further duplicate TPDMs
connected.

Suzuki
  

On 5/25/2023 5:08 PM, Suzuki K Poulose wrote:
> On 25/05/2023 08:16, Tao Zhang wrote:
>>
>> On 5/23/2023 6:07 PM, Suzuki K Poulose wrote:
>>> On 27/04/2023 10:00, Tao Zhang wrote:
>>>> Read the DSB element size from the device tree. Set the register
>>>> bit that controls the DSB element size of the corresponding port.
>>>>
>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/coresight-core.c |  1 +
>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 92 
>>>> +++++++++++++++++++++++++---
>>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>>   drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
>>>>   include/linux/coresight.h                    |  1 +
>>>>   5 files changed, 90 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>>>> b/drivers/hwtracing/coresight/coresight-core.c
>>>> index 2af416b..f1eacbb 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct 
>>>> coresight_device *csdev,
>>>>         if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>>>> +        subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>>>>           dev_err(&csdev->dev, "wrong device subtype in %s\n", 
>>>> function);
>>>>           return -EINVAL;
>>>
>>> Please see the comment at the bottom.
>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> index 8d2b9d2..af9c72f 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -21,6 +21,56 @@
>>>>     DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>>>>   +/* Search and read element data size from the TPDM node in
>>>> + * the devicetree. Each input port of TPDA is connected to
>>>> + * a TPDM. Different TPDM supports different types of dataset,
>>>> + * and some may support more than one type of dataset.
>>>> + * Parameter "inport" is used to pass in the input port number
>>>> + * of TPDA, and it is set to 0 in the recursize call.
>>>> + * Parameter "parent" is used to pass in the original call.
>>>> + */
>>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>>> +               struct coresight_device *csdev, int inport, bool 
>>>> parent)
>>>> +{
>>>> +    static int nr_inport;
>>>> +    int i;
>>>> +    static bool tpdm_found;
>>>> +    struct coresight_device *in_csdev;
>>>> +
>>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (parent) {
>>>> +        nr_inport = inport;
>>>> +        tpdm_found = false;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>> +        in_csdev = csdev->pdata->in_conns[i]->src_dev;
>>>> +        if (!in_csdev)
>>>> +            break;
>>>> +
>>>> +        if (parent)
>>>> +            if (csdev->pdata->in_conns[i]->dest_port != inport)
>>>> +                continue;
>>>> +
>>>> +        if (in_csdev->subtype.source_subtype
>>>
>>> We must match the in_csdev->type to be SOURCE && the subtype.
>> Sure, I will update it in the next patch series.
>>>
>>>> +                   == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) {
>>>> + of_property_read_u8(in_csdev->dev.parent->of_node,
>>>> +                    "qcom,dsb-element-size", 
>>>> &drvdata->dsb_esize[nr_inport]);
>>>> +            if (!tpdm_found)
>>>> +                tpdm_found = true;
>>>> +            else
>>>> +                dev_warn(drvdata->dev,
>>>> +                    "More than one TPDM is mapped to the TPDA 
>>>> input port %d.\n",
>>>> +                    nr_inport);
>>>
>>> When we know, we have found a source device, we don't need to recurse
>>> down and could simply 'continue' to the next one in the list and skip
>>> the call below.
>>
>> Actually, one input port on TPDA only can connect to one TPDM. In the 
>> current design, it will
>>
>> find out all the TPDMs on one input port and warn the users all the 
>> TPDMs it found. If we
>>
>> replace 'recurse down' as 'continue' here, it may not find some TPDMs 
>> that might be connected
>>
>> incorrectly.
>
>
> What do you mean ? When you enter the if () above, the in_csdev is a
> source and it is TPDM. There must be no input connections TPDM, i.e.
> in_csdev, so no need to go further up the connection chain looking at
> the in_csdev. The loop will continue to analyse this device (where we
> found one TPDM already) and detect any further duplicate TPDMs
> connected.

Got it. You're right.

I will update this in the next patch series as well.

>
> Suzuki
>
>