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

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 5/6/25 16:23, Leo Yan wrote:
> On Mon, May 05, 2025 at 12:58:06PM +0530, Anshuman Khandual wrote:
> 
> [...]
> 
>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>> @@ -1645,6 +1645,51 @@ int coresight_etm_get_trace_id(struct coresight_device *csdev, enum cs_mode mode
>>>  }
>>>  EXPORT_SYMBOL_GPL(coresight_etm_get_trace_id);
>>>  
>>> +/*
>>> + * 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)
>>
>> These arguments probably could be arranged better as pclk and atclk are
>> always contained inside 'xxx_drvdata' structure, which could be derived
>> from the 'dev' structure itself, if [dev|platform]_set_drvdata() always
>> ensured to be called earlier.
> 
> Seems to me, the conclusion "pclk and atclk ... could be derived from
> the 'dev' structure itself" is not true.
> 
> The reason is the coresight_get_enable_clocks() function is located in
> the CoreSight core layer, it has no knowledge for driver data
> definitions (see etmv4_drvdata, funnel_drvdata, etc).  as a result, it
> cannot directly access the fields "drvdata->pclk" and "drvdata->atclk".

That's correct because all device's drvdata structure definitions are
local to their drivers and not really available in the core coresight
layer. Trying to make them available might create more code churn and
also break the abstraction.

I guess then a struct device and two clock double pointers are needed
for these optional clocks to be assigned and enabled as proposed here.

> 
>> Currently there are only two instances where a NULL is being passed to
>> indicate that 'atclk' clock is not to be probed or enabled. Could not
>> individual clock requirements be passed in a flag argument instead ?
>>
>> #define CORESIGHT_ENABLE_PCLK	0x1
>> #define CORESIGHT_ENABLE_ATCLK	0x2
>>
>> coresight_get_enable_clocks(struct device *dev, unsigned long flags)
>>
>> - atclk/pclk derived from drdvata which in turn from dev
>> - flags can be checked for pclk/atclk requirements
>>
>> Even better - as atlck is the only optional clock here, it could just
>> have a boolean flag argument to indicate for atclk clock.
>>
>>> +{
>>> +	WARN_ON(!pclk);
>>> +
>>> +	if (!dev_is_amba(dev)) {
>>> +		/*
>>> +		 * "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);
>>> +	} else {
>>> +		/* Don't enable pclk for an AMBA device */
>>> +		*pclk = NULL;
>>> +	}
>>
>> Might be better to invert this conditional check as if (dev_is_amba(dev))
>> for better readability.
> 
> Will refine code for this.

Sure