[v2,2/8] coresight-tpda: Add support to configure CMB element

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

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpda.c | 108 ++++++++++++++++-----------
 drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
 2 files changed, 69 insertions(+), 45 deletions(-)
  

On 25/10/2023 03:53, Tao Zhang wrote:
> Read the CMB element size from the device tree. Set the register
> bit that controls the CMB element size of the corresponding port.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-tpda.c | 108 ++++++++++++++++-----------
>  drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
>  2 files changed, 69 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 5f82737..3101d2a 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct coresight_device *csdev)
>  			CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>  }
>  
> +static void tpdm_clear_element_size(struct coresight_device *csdev)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +	if (drvdata->dsb_esize)
> +		drvdata->dsb_esize = 0;
> +	if (drvdata->cmb_esize)
> +		drvdata->cmb_esize = 0;
> +}
> +
> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32 *val)
> +{
> +
> +	if (drvdata->dsb_esize == 64)
> +		*val |= TPDA_Pn_CR_DSBSIZE;
> +	else if (drvdata->dsb_esize == 32)
> +		*val &= ~TPDA_Pn_CR_DSBSIZE;
> +
> +	if (drvdata->cmb_esize == 64)
> +		*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
> +	else if (drvdata->cmb_esize == 32)
> +		*val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
> +	else if (drvdata->cmb_esize == 8)
> +		*val &= ~TPDA_Pn_CR_CMBSIZE;
> +}
> +
>  /*
>   * Read the DSB element size from the TPDM device
>   * Returns
>   *    The dsb element size read from the devicetree if available.

Hi Tao,

I think the function description and the return value description above
need updating now.

>   *    0 - Otherwise, with a warning once.
>   */
> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
> +				  struct coresight_device *csdev)
>  {
> -	int rc = 0;
> -	u8 size = 0;
> -
> -	rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> -			"qcom,dsb-element-size", &size);
> +	int rc = -EEXIST;
> +
> +	if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> +			"qcom,dsb-element-size", &drvdata->dsb_esize))
> +		rc &= 0;

Is &= 0 significant? Why not = 0?

> +	if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> +			"qcom,cmb-element-size", &drvdata->cmb_esize))
> +		rc &= 0;
>  	if (rc)
>  		dev_warn_once(&csdev->dev,
> -			"Failed to read TPDM DSB Element size: %d\n", rc);
> +			"Failed to read TPDM Element size: %d\n", rc);
>  
> -	return size;
> +	return rc;
>  }
>  
>  /*
> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>   * Parameter "inport" is used to pass in the input port number
>   * of TPDA, and it is set to -1 in the recursize call.
>   */
> -static int tpda_get_element_size(struct coresight_device *csdev,
> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
> +				 struct coresight_device *csdev,
>  				 int inport)
>  {
> -	int dsb_size = -ENOENT;
> -	int i, size;
> +	int rc = -ENOENT;
> +	int i;
>  	struct coresight_device *in;
>  
> +	if (inport > 0)
> +		tpdm_clear_element_size(csdev);
> +
>  	for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>  		in = csdev->pdata->in_conns[i]->src_dev;
>  		if (!in)
> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct coresight_device *csdev,
>  			continue;
>  
>  		if (coresight_device_is_tpdm(in)) {
> -			size = tpdm_read_dsb_element_size(in);
> +			if (rc)
> +				rc = tpdm_read_element_size(drvdata, in);
> +			else
> +				return -EINVAL;

This is quite hard to follow, is checking rc here before calling
tpdm_read_element_size() some kind of way of only calling it once?

rc isn't actually a return value at this point, it's just default
initialised to -ENOENT.

Then it's not clear why the else condition returns an error?

>  		} else {
>  			/* Recurse down the path */
> -			size = tpda_get_element_size(in, -1);
> -		}
> -
> -		if (size < 0)
> -			return size;
> -
> -		if (dsb_size < 0) {
> -			/* Found a size, save it. */
> -			dsb_size = size;
> -		} else {
> -			/* Found duplicate TPDMs */
> -			return -EEXIST;
> +			rc = tpda_get_element_size(drvdata, in, -1);

And then why it's assigned here but not checked for an error in this case?

Maybe some comments explaining the flow would help. Or to me it seems
like a second variable to track the thing that's actually intended could
be used as well. Like "bool found_element" or something, rather than
re-using rc to also track another thing.

>  		}
>  	}
>  
> -	return dsb_size;
> +	if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
> +		rc = 0;
> +
> +	return rc;
>  }
>  
>  /* Settings pre enabling port control register */
> @@ -109,7 +138,7 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>  static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>  {
>  	u32 val;
> -	int size;
> +	int rc;
>  
>  	val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
>  	/*
> @@ -117,29 +146,18 @@ static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>  	 * Set the bit to 0 if the size is 32
>  	 * Set the bit to 1 if the size is 64
>  	 */
> -	size = tpda_get_element_size(drvdata->csdev, port);
> -	switch (size) {
> -	case 32:
> -		val &= ~TPDA_Pn_CR_DSBSIZE;
> -		break;
> -	case 64:
> -		val |= TPDA_Pn_CR_DSBSIZE;
> -		break;
> -	case 0:
> -		return -EEXIST;
> -	case -EEXIST:
> +	rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
> +	if (!rc) {
> +		tpda_set_element_size(drvdata, &val);
> +		/* Enable the port */
> +		val |= TPDA_Pn_CR_ENA;
> +		writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> +	} else if (rc == -EINVAL) {
>  		dev_warn_once(&drvdata->csdev->dev,
>  			"Detected multiple TPDMs on port %d", -EEXIST);

tpdm_read_element_size() can return EEXIST, but then it gets changed
back to EINVAL in tpda_get_element_size() without caring about the
specific code, before finally being changed back to EEXIST for the
warning here. Can it just be propagated as the original value? Or use
EINVAL or EEXIST all the way through. That would probably be easier to
follow.

And then also a comment about why the other possible error values don't
result in a warning.

Thanks
James

> -		return -EEXIST;
> -	default:
> -		return -EINVAL;
>  	}
>  
> -	/* Enable the port */
> -	val |= TPDA_Pn_CR_ENA;
> -	writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> -
> -	return 0;
> +	return rc;
>  }
>  
>  static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index b3b38fd..29164fd 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 CMB data set element size bit */
> +#define TPDA_Pn_CR_CMBSIZE		GENMASK(7, 6)
>  /* Aggregator port DSB data set element size bit */
>  #define TPDA_Pn_CR_DSBSIZE		BIT(8)
>  
> @@ -25,6 +27,8 @@
>   * @csdev:      component vitals needed by the framework.
>   * @spinlock:   lock for the drvdata value.
>   * @enable:     enable status of the component.
> + * @dsb_esize   Record the DSB element size.
> + * @cmb_esize   Record the CMB element size.
>   */
>  struct tpda_drvdata {
>  	void __iomem		*base;
> @@ -32,6 +36,8 @@ struct tpda_drvdata {
>  	struct coresight_device	*csdev;
>  	spinlock_t		spinlock;
>  	u8			atid;
> +	u8			dsb_esize;
> +	u8			cmb_esize;
>  };
>  
>  #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
  

On 01/11/2023 08:53, Tao Zhang wrote:
> 
> On 10/30/2023 7:11 PM, James Clark wrote:
>>
>> On 25/10/2023 03:53, Tao Zhang wrote:
>>> Read the CMB element size from the device tree. Set the register
>>> bit that controls the CMB element size of the corresponding port.
>>>
>>> Signed-off-by: Tao Zhang<quic_taozha@quicinc.com>
>>> Signed-off-by: Mao Jinlong<quic_jinlmao@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-tpda.c | 108
>>> ++++++++++++++++-----------
>>>   drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
>>>   2 files changed, 69 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>> index 5f82737..3101d2a 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct
>>> coresight_device *csdev)
>>>               CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>>   }
>>>   +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +
>>> +    if (drvdata->dsb_esize)
>>> +        drvdata->dsb_esize = 0;
>>> +    if (drvdata->cmb_esize)
>>> +        drvdata->cmb_esize = 0;

The ifs don't do anything here, it's just the same as always setting to 0.

>>> +}
>>> +
>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32
>>> *val)
>>> +{
>>> +
>>> +    if (drvdata->dsb_esize == 64)
>>> +        *val |= TPDA_Pn_CR_DSBSIZE;
>>> +    else if (drvdata->dsb_esize == 32)
>>> +        *val &= ~TPDA_Pn_CR_DSBSIZE;
>>> +
>>> +    if (drvdata->cmb_esize == 64)
>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>> +    else if (drvdata->cmb_esize == 32)
>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>> +    else if (drvdata->cmb_esize == 8)
>>> +        *val &= ~TPDA_Pn_CR_CMBSIZE;
>>> +}
>>> +
>>>   /*
>>>    * Read the DSB element size from the TPDM device
>>>    * Returns
>>>    *    The dsb element size read from the devicetree if available.
>> Hi Tao,
>>
>> I think the function description and the return value description above
>> need updating now.
> I will update this in the next patch series.
>>
>>>    *    0 - Otherwise, with a warning once.
>>>    */
>>> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>>> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
>>> +                  struct coresight_device *csdev)
>>>   {
>>> -    int rc = 0;
>>> -    u8 size = 0;
>>> -
>>> -    rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>> -            "qcom,dsb-element-size", &size);
>>> +    int rc = -EEXIST;
>>> +
>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>> +            "qcom,dsb-element-size", &drvdata->dsb_esize))
>>> +        rc &= 0;
>> Is &= 0 significant? Why not = 0?
> I will update this in the next patch series.
>>
>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>> +            "qcom,cmb-element-size", &drvdata->cmb_esize))
>>> +        rc &= 0;
>>>       if (rc)
>>>           dev_warn_once(&csdev->dev,
>>> -            "Failed to read TPDM DSB Element size: %d\n", rc);
>>> +            "Failed to read TPDM Element size: %d\n", rc);
>>>   -    return size;
>>> +    return rc;
>>>   }
>>>     /*
>>> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct
>>> coresight_device *csdev)
>>>    * Parameter "inport" is used to pass in the input port number
>>>    * of TPDA, and it is set to -1 in the recursize call.
>>>    */
>>> -static int tpda_get_element_size(struct coresight_device *csdev,
>>> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
>>> +                 struct coresight_device *csdev,
>>>                    int inport)
>>>   {
>>> -    int dsb_size = -ENOENT;
>>> -    int i, size;
>>> +    int rc = -ENOENT;
>>> +    int i;
>>>       struct coresight_device *in;
>>>   +    if (inport > 0)
>>> +        tpdm_clear_element_size(csdev);
>>> +
>>>       for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>           in = csdev->pdata->in_conns[i]->src_dev;
>>>           if (!in)
>>> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct
>>> coresight_device *csdev,
>>>               continue;
>>>             if (coresight_device_is_tpdm(in)) {
>>> -            size = tpdm_read_dsb_element_size(in);
>>> +            if (rc)
>>> +                rc = tpdm_read_element_size(drvdata, in);
>>> +            else
>>> +                return -EINVAL;
>> This is quite hard to follow, is checking rc here before calling
>> tpdm_read_element_size() some kind of way of only calling it once?
> 
> Yes, there can only be one TPDM on one input port of TPDA. If
> "tpdm_read_element_size" is called
> 
> twice, it means that two TPDMs are found on one input port of TPDA.
> 
>> rc isn't actually a return value at this point, it's just default
>> initialised to -ENOENT.
> 
> In this loop, "tpdm_read_element_size" will be called for the first time
> TPDM found. If rc equals to
> 
> 0, it means that at lease one TPDM has been found on the input port.
> Does it make sense?
> 
>> Then it's not clear why the else condition returns an error?
> The same as the above.
>>
>>>           } else {
>>>               /* Recurse down the path */
>>> -            size = tpda_get_element_size(in, -1);
>>> -        }
>>> -
>>> -        if (size < 0)
>>> -            return size;
>>> -
>>> -        if (dsb_size < 0) {
>>> -            /* Found a size, save it. */
>>> -            dsb_size = size;
>>> -        } else {
>>> -            /* Found duplicate TPDMs */
>>> -            return -EEXIST;
>>> +            rc = tpda_get_element_size(drvdata, in, -1);
>> And then why it's assigned here but not checked for an error in this
>> case?
> 
> |Remote ETM|                           |TPDM|
> 
>             |    branch0                           | branch1
> 
>                         --------------------------
> 
>                                     funnel1
> 
>                         ---------------------------
> 
>                                           | input port 1
> 
> -----------------------------
> 
>                                                        TPDA1
> 
> -----------------------------
> 
> The  branches on some TPDA's input ports may not have TPDM. For example,
> branch 0 doesn't has a
> 
> TPDM connected, tpda_get_element_size will not return 0 here. This is a
> normal situation. We need to
> 
> continue to find TPDM on branch1 through recursion.
> 
>>
>> Maybe some comments explaining the flow would help. Or to me it seems
>> like a second variable to track the thing that's actually intended could
>> be used as well. Like "bool found_element" or something, rather than
>> re-using rc to also track another thing.
> 
> Do you prefer to use "static bool found_element" to mark if we already
> have read an element size in
> 
> the recursion function?
> 

You could add a static, or you could use whether a set dsb or cmb size
exists to mark that one was found, which I think is already partially done.

My confusion comes from the fact that instead of just a recursive
search, the function doesn't stop at the first found one, it continues
to sanity check that there is only one connected across all ports.

And instead of just checking the error condition at the very end, it
does it mid recursion, relying on the rc value from a previous
iteration. IMO the following is a simplification because it always
returns at the first error found, and checks the final condition outside
of the recursive function. But you could probably leave it as you have
but with some comments explaining why:


diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 3101d2a0671d..00b7607d36be 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -90,13 +90,10 @@ static int tpda_get_element_size(struct tpda_drvdata *drvdata,
                                  struct coresight_device *csdev,
                                  int inport)
  {
-       int rc = -ENOENT;
+       int rc = 0;
         int i;
         struct coresight_device *in;
  
-       if (inport > 0)
-               tpdm_clear_element_size(csdev);
-
         for (i = 0; i < csdev->pdata->nr_inconns; i++) {
                 in = csdev->pdata->in_conns[i]->src_dev;
                 if (!in)
@@ -108,19 +105,23 @@ static int tpda_get_element_size(struct tpda_drvdata *drvdata,
                         continue;
  
                 if (coresight_device_is_tpdm(in)) {
-                       if (rc)
-                               rc = tpdm_read_element_size(drvdata, in);
-                       else
+                       if ((drvdata->dsb_esize) || (drvdata->cmb_esize)) {
+                               /* Error if already previously found and initialised one */
+                               dev_warn_once(&drvdata->csdev->dev,
+                                       "Detected multiple TPDMs on port %d", -EEXIST);
                                 return -EINVAL;
+                       }
+                       rc = tpdm_read_element_size(drvdata, in);
+                       if (rc)
+                               return rc;
                 } else {
                         /* Recurse down the path */
                         rc = tpda_get_element_size(drvdata, in, -1);
+                       if (rc)
+                               return rc;
                 }
         }
  
-       if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
-               rc = 0;
-
         return rc;
  }
  
@@ -146,15 +147,17 @@ static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
          * Set the bit to 0 if the size is 32
          * Set the bit to 1 if the size is 64
          */
+       tpdm_clear_element_size(drvdata->csdev);
         rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
-       if (!rc) {
+       if (!rc && ((drvdata->dsb_esize) || (drvdata->cmb_esize)))
+       {
                 tpda_set_element_size(drvdata, &val);
                 /* Enable the port */
                 val |= TPDA_Pn_CR_ENA;
                 writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
-       } else if (rc == -EINVAL) {
-               dev_warn_once(&drvdata->csdev->dev,
-                       "Detected multiple TPDMs on port %d", -EEXIST);
+       } else {
+               dev_warn_once(&drvdata->csdev->dev, "Didn't find TPDM elem size");
+               rc = -EINVAL;
         }
  
         return rc;
  

On 11/1/2023 7:36 PM, James Clark wrote:
>
>
> On 01/11/2023 08:53, Tao Zhang wrote:
>>
>> On 10/30/2023 7:11 PM, James Clark wrote:
>>>
>>> On 25/10/2023 03:53, Tao Zhang wrote:
>>>> Read the CMB element size from the device tree. Set the register
>>>> bit that controls the CMB element size of the corresponding port.
>>>>
>>>> Signed-off-by: Tao Zhang<quic_taozha@quicinc.com>
>>>> Signed-off-by: Mao Jinlong<quic_jinlmao@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 108
>>>> ++++++++++++++++-----------
>>>>   drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
>>>>   2 files changed, 69 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> index 5f82737..3101d2a 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct
>>>> coresight_device *csdev)
>>>>               CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>>>   }
>>>>   +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>>> +{
>>>> +    struct tpda_drvdata *drvdata = 
>>>> dev_get_drvdata(csdev->dev.parent);
>>>> +
>>>> +    if (drvdata->dsb_esize)
>>>> +        drvdata->dsb_esize = 0;
>>>> +    if (drvdata->cmb_esize)
>>>> +        drvdata->cmb_esize = 0;
>
> The ifs don't do anything here, it's just the same as always setting 
> to 0.
I will update in the next patch series.
>
>>>> +}
>>>> +
>>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32
>>>> *val)
>>>> +{
>>>> +
>>>> +    if (drvdata->dsb_esize == 64)
>>>> +        *val |= TPDA_Pn_CR_DSBSIZE;
>>>> +    else if (drvdata->dsb_esize == 32)
>>>> +        *val &= ~TPDA_Pn_CR_DSBSIZE;
>>>> +
>>>> +    if (drvdata->cmb_esize == 64)
>>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>>> +    else if (drvdata->cmb_esize == 32)
>>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>>> +    else if (drvdata->cmb_esize == 8)
>>>> +        *val &= ~TPDA_Pn_CR_CMBSIZE;
>>>> +}
>>>> +
>>>>   /*
>>>>    * Read the DSB element size from the TPDM device
>>>>    * Returns
>>>>    *    The dsb element size read from the devicetree if available.
>>> Hi Tao,
>>>
>>> I think the function description and the return value description above
>>> need updating now.
>> I will update this in the next patch series.
>>>
>>>>    *    0 - Otherwise, with a warning once.
>>>>    */
>>>> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>>>> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
>>>> +                  struct coresight_device *csdev)
>>>>   {
>>>> -    int rc = 0;
>>>> -    u8 size = 0;
>>>> -
>>>> -    rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> -            "qcom,dsb-element-size", &size);
>>>> +    int rc = -EEXIST;
>>>> +
>>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> +            "qcom,dsb-element-size", &drvdata->dsb_esize))
>>>> +        rc &= 0;
>>> Is &= 0 significant? Why not = 0?
>> I will update this in the next patch series.
>>>
>>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> +            "qcom,cmb-element-size", &drvdata->cmb_esize))
>>>> +        rc &= 0;
>>>>       if (rc)
>>>>           dev_warn_once(&csdev->dev,
>>>> -            "Failed to read TPDM DSB Element size: %d\n", rc);
>>>> +            "Failed to read TPDM Element size: %d\n", rc);
>>>>   -    return size;
>>>> +    return rc;
>>>>   }
>>>>     /*
>>>> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct
>>>> coresight_device *csdev)
>>>>    * Parameter "inport" is used to pass in the input port number
>>>>    * of TPDA, and it is set to -1 in the recursize call.
>>>>    */
>>>> -static int tpda_get_element_size(struct coresight_device *csdev,
>>>> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
>>>> +                 struct coresight_device *csdev,
>>>>                    int inport)
>>>>   {
>>>> -    int dsb_size = -ENOENT;
>>>> -    int i, size;
>>>> +    int rc = -ENOENT;
>>>> +    int i;
>>>>       struct coresight_device *in;
>>>>   +    if (inport > 0)
>>>> +        tpdm_clear_element_size(csdev);
>>>> +
>>>>       for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>>           in = csdev->pdata->in_conns[i]->src_dev;
>>>>           if (!in)
>>>> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct
>>>> coresight_device *csdev,
>>>>               continue;
>>>>             if (coresight_device_is_tpdm(in)) {
>>>> -            size = tpdm_read_dsb_element_size(in);
>>>> +            if (rc)
>>>> +                rc = tpdm_read_element_size(drvdata, in);
>>>> +            else
>>>> +                return -EINVAL;
>>> This is quite hard to follow, is checking rc here before calling
>>> tpdm_read_element_size() some kind of way of only calling it once?
>>
>> Yes, there can only be one TPDM on one input port of TPDA. If
>> "tpdm_read_element_size" is called
>>
>> twice, it means that two TPDMs are found on one input port of TPDA.
>>
>>> rc isn't actually a return value at this point, it's just default
>>> initialised to -ENOENT.
>>
>> In this loop, "tpdm_read_element_size" will be called for the first time
>> TPDM found. If rc equals to
>>
>> 0, it means that at lease one TPDM has been found on the input port.
>> Does it make sense?
>>
>>> Then it's not clear why the else condition returns an error?
>> The same as the above.
>>>
>>>>           } else {
>>>>               /* Recurse down the path */
>>>> -            size = tpda_get_element_size(in, -1);
>>>> -        }
>>>> -
>>>> -        if (size < 0)
>>>> -            return size;
>>>> -
>>>> -        if (dsb_size < 0) {
>>>> -            /* Found a size, save it. */
>>>> -            dsb_size = size;
>>>> -        } else {
>>>> -            /* Found duplicate TPDMs */
>>>> -            return -EEXIST;
>>>> +            rc = tpda_get_element_size(drvdata, in, -1);
>>> And then why it's assigned here but not checked for an error in this
>>> case?
>>
>> |Remote ETM|                           |TPDM|
>>
>>             |    branch0                           | branch1
>>
>>                         --------------------------
>>
>>                                     funnel1
>>
>>                         ---------------------------
>>
>>                                           | input port 1
>>
>> -----------------------------
>>
>>                                                        TPDA1
>>
>> -----------------------------
>>
>> The  branches on some TPDA's input ports may not have TPDM. For example,
>> branch 0 doesn't has a
>>
>> TPDM connected, tpda_get_element_size will not return 0 here. This is a
>> normal situation. We need to
>>
>> continue to find TPDM on branch1 through recursion.
>>
>>>
>>> Maybe some comments explaining the flow would help. Or to me it seems
>>> like a second variable to track the thing that's actually intended 
>>> could
>>> be used as well. Like "bool found_element" or something, rather than
>>> re-using rc to also track another thing.
>>
>> Do you prefer to use "static bool found_element" to mark if we already
>> have read an element size in
>>
>> the recursion function?
>>
>
> You could add a static, or you could use whether a set dsb or cmb size
> exists to mark that one was found, which I think is already partially 
> done.
>
> My confusion comes from the fact that instead of just a recursive
> search, the function doesn't stop at the first found one, it continues
> to sanity check that there is only one connected across all ports.
>
> And instead of just checking the error condition at the very end, it
> does it mid recursion, relying on the rc value from a previous
> iteration. IMO the following is a simplification because it always
> returns at the first error found, and checks the final condition outside
> of the recursive function. But you could probably leave it as you have
> but with some comments explaining why:
>
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
> b/drivers/hwtracing/coresight/coresight-tpda.c
> index 3101d2a0671d..00b7607d36be 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -90,13 +90,10 @@ static int tpda_get_element_size(struct 
> tpda_drvdata *drvdata,
>                                  struct coresight_device *csdev,
>                                  int inport)
>  {
> -       int rc = -ENOENT;
> +       int rc = 0;
>         int i;
>         struct coresight_device *in;
>
> -       if (inport > 0)
> -               tpdm_clear_element_size(csdev);
> -
>         for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>                 in = csdev->pdata->in_conns[i]->src_dev;
>                 if (!in)
> @@ -108,19 +105,23 @@ static int tpda_get_element_size(struct 
> tpda_drvdata *drvdata,
>                         continue;
>
>                 if (coresight_device_is_tpdm(in)) {
> -                       if (rc)
> -                               rc = tpdm_read_element_size(drvdata, in);
> -                       else
> +                       if ((drvdata->dsb_esize) || 
> (drvdata->cmb_esize)) {
> +                               /* Error if already previously found 
> and initialised one */
> + dev_warn_once(&drvdata->csdev->dev,
> +                                       "Detected multiple TPDMs on 
> port %d", -EEXIST);
>                                 return -EINVAL;
> +                       }
> +                       rc = tpdm_read_element_size(drvdata, in);
> +                       if (rc)
> +                               return rc;
>                 } else {
>                         /* Recurse down the path */
>                         rc = tpda_get_element_size(drvdata, in, -1);
> +                       if (rc)
> +                               return rc;
>                 }
>         }
>
> -       if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
> -               rc = 0;
> -
>         return rc;
>  }
>
> @@ -146,15 +147,17 @@ static int tpda_enable_port(struct tpda_drvdata 
> *drvdata, int port)
>          * Set the bit to 0 if the size is 32
>          * Set the bit to 1 if the size is 64
>          */
> +       tpdm_clear_element_size(drvdata->csdev);
>         rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
> -       if (!rc) {
> +       if (!rc && ((drvdata->dsb_esize) || (drvdata->cmb_esize)))
> +       {
>                 tpda_set_element_size(drvdata, &val);
>                 /* Enable the port */
>                 val |= TPDA_Pn_CR_ENA;
>                 writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> -       } else if (rc == -EINVAL) {
> -               dev_warn_once(&drvdata->csdev->dev,
> -                       "Detected multiple TPDMs on port %d", -EEXIST);
> +       } else {
> +               dev_warn_once(&drvdata->csdev->dev, "Didn't find TPDM 
> elem size");
> +               rc = -EINVAL;
>         }
>
>         return rc;

Make sense. I will refine and update the code in the next patch series.


Best,

Tao

>
>