Re: [PATCH v3 7/9] coresight: Consolidate clock enabling

[Leo Yan <leo.yan@arm.com>]

On Tue, Jun 24, 2025 at 11:14:17AM +0530, Anshuman Khandual wrote:

[...]

> > 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.
> 
> But why should this change be included here in this patch that
> consolidates pclk and atclk clock initialization ? Should this
> be done in a separate patch instead ?

Good point. It indeed we can divide into two smaller patches, one is
for clock consolidations, and another patch is for refining driver data
allocation.

I will do this in next spin.

> > +/*
> > + * Attempt to find and enable programming clock (pclk) and trace clock (atclk)
> > + * for the given device.
> > + *
> > + * The AMBA bus driver will cover the pclk, to avoid duplicate operations,
> > + * skip to get and enable the pclk for an AMBA device.
> > + *
> > + * 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.
> > + */
> > +int coresight_get_enable_clocks(struct device *dev, struct clk **pclk,
> > +				struct clk **atclk)
> Moving this helper function here does make sense.
> 
> > +{
> > +	WARN_ON(!pclk);
> 
> That is because pclk will be populated in all possible scenarios
> including the one assigned as NULL - hence it needs to have been
> allocated.
>  > +
> > +	if (dev_is_amba(dev)) {
> > +		/* Don't enable pclk for an AMBA device */
> > +		*pclk = NULL;
> > +	} else {
> > +		/*
> > +		 * "apb_pclk" is the default clock name for an Arm Primecell
> > +		 * peripheral, while "apb" is used only by the CTCU driver.
> > +		 *
> > +		 * For easier maintenance, CoreSight drivers should use
> > +		 * "apb_pclk" as the programming clock name.
> > +		 */
> > +		*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);
> > +	}
> > +
> > +	if (atclk) {
> 
> But an allocated atclk indicates need for atclk clock init instead.

To be clear, we strictly follow up DT binding doc, atclk clock is
always optional. For a atclk pointer:

- If atclk is NULL: the driver does not require atclk at all.
- If atclk is not NULL: the driver requires atclk optional.

> Probably a 'which all clocks' flag based approach might been better ?
> But I guess this proposal will create less code churn.

So far, CoreSight driver has a fixed requirement for clocks.

- APB clock (pclk): it is mandatory. It can be either controlled by
  AMBA bus driver or CoreSight driver.

- atclk: It is not required (CTCU) or it is optional.

Given the pattern is fixed, I don't think an extra flag would be helpful
at here. But I understand the code is not very strightforward, I will
add comments for easier understanding.

> > +		*atclk = devm_clk_get_optional_enabled(dev, "atclk");
> > +		if (IS_ERR(*atclk))
> > +			return PTR_ERR(*atclk);
> > +	}
> 
> atclk when requested - either will have a valid clock or an error
> pointer but never a NULL pointer unlike the pclk clock ?

As mentioned, atclk can be a NULL pointer - it is optional and may be
absent in the device tree binding. This is why we use the optional
variant of the clock API to initialize it. If it returns a NULL
pointer, it is tolerated by IS_ERR(*atclk), and this is considered a
successful case.

Thanks,
Leo