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

[Jie Gan]

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