Re: [PATCH v1 2/3] coresight: tpda: add function to configure TPDA_SYNCR register

[James Clark <james.clark@linaro.org>]

On 26/08/2025 8:01 am, Jie Gan wrote:
> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
> 
> The TPDA_SYNCR register defines the frequency at which TPDA generates
> ASYNC packets, enabling userspace tools to accurately parse each valid
> packet.
> 
> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpda.c | 15 +++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpda.h |  1 +
>   2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index cc254d53b8ec..9e623732d1e7 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -189,6 +189,18 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>   		writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>   }
>   
> +static void tpda_enable_post_port(struct tpda_drvdata *drvdata)
> +{
> +	uint32_t val;

Minor nit: this is inconsistent with u32 used elsewhere in this file.

> +
> +	val = readl_relaxed(drvdata->base + TPDA_SYNCR);
> +	/* Clear the mode */
> +	val = val & ~TPDA_MODE_CTRL;

&=

> +	/* Program the counter value */
> +	val = val | 0xFFF;

|=

Defining a field would be a bit nicer here. Like:

val |= FIELD_PREP(TPDA_SYNCR_COUNTER, UINT32_MAX);

Assuming you wanted to set all bits, and 0xFFF isn't some specific value.

> +	writel_relaxed(val, drvdata->base + TPDA_SYNCR);
> +}
> +
>   static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>   {
>   	u32 val;
> @@ -227,6 +239,9 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
>   		tpda_enable_pre_port(drvdata);
>   
>   	ret = tpda_enable_port(drvdata, port);
> +	if (!drvdata->csdev->refcnt)
> +		tpda_enable_post_port(drvdata);

Any reason this can't be done on tpda_enable_pre_port()? It has the same 
logic where it's only done once for the first port.

If it can't be done there you should add a comment saying why it must be 
done after enabling the first port.

> +
>   	CS_LOCK(drvdata->base);
>   
>   	return ret;
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index b651372d4c88..00d146960d81 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.h
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -9,6 +9,7 @@
>   #define TPDA_CR			(0x000)
>   #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
>   #define TPDA_FPID_CR		(0x084)
> +#define TPDA_SYNCR		(0x08C)
>   
>   /* Cross trigger FREQ packets timestamp bit */
>   #define TPDA_CR_FREQTS		BIT(2)