Re: [PATCH v1 9/9] coresight: Consolidate clock enabling

[James Clark]

On 27/03/2025 11:38 am, Leo Yan wrote:
> CoreSight drivers enable pclk and atclk conditionally.  For example,
> pclk is only enabled in the static probe, while atclk is an optional
> clock that it is enabled for both dynamic and static probes, if it is
> present.  In the current CoreSight drivers, these two clocks are
> initialized separately.  This causes complex and duplicate codes.
>
> This patch consolidates clock enabling into a central place.  It renames
> coresight_get_enable_apb_pclk() to coresight_get_enable_clocks() and
> encapsulates clock initialization logic:
>
>   - If a clock is initialized successfully, its clock pointer is assigned
>     to the double pointer passed as an argument.
>   - If pclk is skipped for an AMBA device, or if atclk is not found, the
>     corresponding double pointer is set to NULL.  The function returns
>     Success (0) to guide callers can proceed with no error.
>   - Otherwise, an error number is returned for failures.
>
> CoreSight drivers are refined so that clocks are initialized in one go.
> As a result, driver data no longer needs to be allocated separately in
> the static and dynamic probes.  Moved the allocation into a low-level
> function to avoid code duplication.
>
> Suggested-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxx>
> ---
>   drivers/hwtracing/coresight/coresight-catu.c       | 30 ++++++++++--------------------
>   drivers/hwtracing/coresight/coresight-cpu-debug.c  | 29 +++++++++++------------------
>   drivers/hwtracing/coresight/coresight-ctcu-core.c  |  8 ++++----
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 11 ++++-------
>   drivers/hwtracing/coresight/coresight-funnel.c     | 11 ++++-------
>   drivers/hwtracing/coresight/coresight-replicator.c | 11 ++++-------
>   drivers/hwtracing/coresight/coresight-stm.c        |  9 +++------
>   drivers/hwtracing/coresight/coresight-tmc-core.c   | 30 ++++++++++--------------------
>   drivers/hwtracing/coresight/coresight-tpiu.c       | 10 ++++------
>   include/linux/coresight.h                          | 38 +++++++++++++++++++++++++++-----------
>   10 files changed, 81 insertions(+), 106 deletions(-)
[...]
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 26eb4a61b992..cf3fbbc0076a 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -471,25 +471,41 @@ static inline bool is_coresight_device(void __iomem *base)
>   }
>   
>   /*
> - * Attempt to find and enable "APB clock" for the given device
> + * Attempt to find and enable programming clock (pclk) and trace clock (atclk)
> + * for the given device.
>    *
> - * Returns:
> + * The AMBA bus driver will cover the pclk, to avoid duplicate operations,
> + * skip to get and enable the pclk for an AMBA device.
>    *
> - * clk   - Clock is found and enabled
> - * NULL  - Clock is not needed as it is managed by the AMBA bus driver
> - * ERROR - Clock is found but failed to enable
> + * atclk is an optional clock, it will be only enabled when it is existed.
> + * Otherwise, a NULL pointer will be returned to caller.
> + *
> + * Returns: '0' on Success; Error code otherwise.
>    */
> -static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
> +static inline int coresight_get_enable_clocks(struct device *dev,
> +					      struct clk **pclk,
> +					      struct clk **atclk)

This function has grown a bit now, probably best to remove it from the 
header and export it instead.

>   {
> -	struct clk *pclk = NULL;
> +	WARN_ON(!pclk);
>   
>   	if (!dev_is_amba(dev)) {
> -		pclk = devm_clk_get_enabled(dev, "apb_pclk");
> -		if (IS_ERR(pclk))
> -			pclk = devm_clk_get_enabled(dev, "apb");
> +		*pclk = devm_clk_get_enabled(dev, "apb_pclk");
> +		if (IS_ERR(*pclk))
> +			*pclk = devm_clk_get_enabled(dev, "apb");
> +		if (IS_ERR(*pclk))
> +			return PTR_ERR(*pclk);
> +	} else {
> +		/* Don't enable pclk for an AMBA device */
> +		*pclk = NULL;

Now the "apb" clock won't be enabled for amba devices. I'm assuming 
that's fine if the clock was always called "apb_pclk" for them, but the 
commit that added the new clock name didn't specify any special casing 
either.

Can we have a comment that says it's deliberate? But the more I think 
about it the more I'm confused why CTCU needed a different clock name to 
be defined, when all the other Coresight devices use "apb_pclk".

>   	}
>   
> -	return pclk;
> +	if (atclk) {
> +		*atclk = devm_clk_get_optional_enabled(dev, "atclk");
> +		if (IS_ERR(*atclk))
> +			return PTR_ERR(*atclk);
> +	}
> +
> +	return 0;
>   }
>   
>   #define CORESIGHT_PIDRn(i)	(0xFE0 + ((i) * 4))