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

[Jie Gan <quic_jiegan@quicinc.com>]

On 4/1/2025 10:58 PM, James Clark wrote:
> 
> 
> 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@arm.com>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>   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".

Hi James,

The original clock-name for CTCU is apb_pclk, but the dt-binding 
maintainer request me to change it to apb, that's why the clock name is 
different from others.

I am not why we need apb instead of apb_pclk in dt-binding. Maybe some 
rules have changed for dt-binding requirement.

Thanks,
Jie

> 
>>       }
>> -    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))
>