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

[Jie Gan <quic_jiegan@quicinc.com>]

On 4/2/2025 5:01 PM, Leo Yan wrote:
> Hi Jie,
> 
> [ + Rob ]
> 
> On Wed, Apr 02, 2025 at 08:55:51AM +0800, Jie Gan wrote:
> 
> [...]
> 
>>>>    {
>>>> -    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.
> 
> My conclusion is that if a device is an Arm Primecell peripheral, it
> should use the clock name "apb_pclk" (See the DT binding doc [1]).
> 
> CTCU is not an Arm Primecell peripheral, so it does not need to strictly
> follow up the clock naming for Primecell peripheral.
> 
> In Arm CoreSight framework, for code consistency, I would suggest
> using the clock naming "apb_pclk" for the APB clock for a newly added
> device that even it is not a Primecell peripheral.
> 
> (We don't need to make any change to the CTCU driver, as we need to
> remain compatible with existed DTB blobs).
> 
> Cc'ing Rob in case he has any suggestions.

Hi Leo,

Thanks for the explanation. I agree with you, we should use the 
"apb_pclk" for the APB clock for a newly added device.

Thanks,
Jie

> 
> Thanks,
> Leo
> 
> [1] Documentation/devicetree/bindings/arm/primecell.yaml