Re: [PATCH v5 05/10] coresight-tpda: Add support to configure CMB element

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 30/01/2024 09:02, 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.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpda.c | 123 +++++++++++--------
>   drivers/hwtracing/coresight/coresight-tpda.h |   6 +
>   2 files changed, 79 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 4ac954f4bc13..fcddff3ded81 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -18,6 +18,7 @@
>   #include "coresight-priv.h"
>   #include "coresight-tpda.h"
>   #include "coresight-trace-id.h"
> +#include "coresight-tpdm.h"
>   
>   DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>   
> @@ -28,24 +29,57 @@ 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);
> +
> +	drvdata->dsb_esize = 0;
> +	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;

We don't seem to be clearing the fields we modify, before updating them. 
This may be OK in real world where the device connected to TPDA port
may not change. But it is always safer to clear the bits and set it.

e.g.:
	*val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);

	

> +	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);

Similarly here ^^^. I am happy to fix it up if you are OK with it 
(unless there are other changes that need a respin)

> +	else if (drvdata->cmb_esize == 8)
> +		*val &= ~TPDA_Pn_CR_CMBSIZE;
> +}

> +
>   /*
> - * Read the DSB element size from the TPDM device
> + * Read the element size from the TPDM device. One TPDM must have at least one of the
> + * element size property.
>    * Returns
> - *    The dsb element size read from the devicetree if available.
> - *    0 - Otherwise, with a warning once.
> + *    0 - The element size property is read
> + *    Others - Cannot read the property of the element size
>    */
> -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;
> +	int rc = -EINVAL;
> +	struct tpdm_drvdata *tpdm_data = dev_get_drvdata(csdev->dev.parent);
> +
> +	if (tpdm_has_dsb_dataset(tpdm_data)) {
> +		rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> +				"qcom,dsb-element-size", &drvdata->dsb_esize);
> +	}
> +	if (tpdm_has_cmb_dataset(tpdm_data)) {
> +		rc = fwnode_property_read_u32(dev_fwnode(csdev->dev.parent),
> +				"qcom,cmb-element-bits", &drvdata->cmb_esize);
> +	}
>   
> -	rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
> -			"qcom,dsb-element-size", &size);
>   	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,11 +90,12 @@ 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 = 0;
> +	int i;
>   	struct coresight_device *in;
>   
>   	for (i = 0; i < csdev->pdata->nr_inconns; i++) {
> @@ -69,30 +104,26 @@ static int tpda_get_element_size(struct coresight_device *csdev,
>   			continue;
>   
>   		/* Ignore the paths that do not match port */
> -		if (inport > 0 &&
> +		if (inport >= 0 &&

That looks like a bug fix, but if you don't care about fixing this in < 
v6.8,  I don't mind.


Rest looks fine to me

Suzuki