Re: [PATCH v2 6/9] coresight: Avoid enable programming clock duplicately

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

On 5/6/25 14:48, Leo Yan wrote:
> Hi Anshuman,
> 
> On Fri, May 02, 2025 at 12:08:55PM +0530, Anshuman Khandual wrote:
>> On 4/23/25 20:47, Leo Yan wrote:
>>> The programming clock is enabled by AMBA bus driver before a dynamic
>>> probe.  As a result, a CoreSight driver may redundantly enable the same
>>> clock.
>>
>> Are you sure AMBA bus driver always enables such clocks in all scenarios ?
> 
> Yes.  I confirmed that AMBA bus driver enables the programming clock
> prior to calling CoreSight device's probes (see amba_probe()).
> 
> I checked other AMBA device drivers (e.g., drivers/dma/amba-pl08x.c)
> never touch APB programming clock and the clock by default is covered
> by AMAB bus driver.

Alright.

> 
>> Even if that is true - why cannot coresight_get_enable_apb_pclk() ensured
>> to be called only for the platform drivers cases via code re-organization,
>> rather than changing the coresight_get_enable_apb_pclk() helper itself.
> 
> The purpose is to unify the clock enabling for both static probe and
> dynamic (AMBA) probe.
> 
> Let us take funnel driver as an example.  With the change in this patch,
> the clock operations will be consolidated in a central place
> (e.g., funnel_probe()).  Therefore, we can avoid to spread the drvdata
> allocation and clock operations into dynamic probe and static (platform)
> probe separately.
> 
>   funnel_probe()
>   {
>       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> 
>       drvdata->pclk = coresight_get_enable_apb_pclk();
>   }
> 
>   dynamic_funnel_probe()
>   {
>       funnel_probe();
>   }
> 
>   funnel_platform_probe()
>   {
>       funnel_probe();
>   }

Makes sense.

> 
> Thanks,
> Leo
> 
>>> To avoid this, add a check for device type and skip enabling the
>>> programming clock for AMBA devices.  The returned NULL pointer will be
>>> tolerated by the drivers.
>>>
>>> Fixes: 73d779a03a76 ("coresight: etm4x: Change etm4_platform_driver driver for MMIO devices")
>>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>>> ---
>>>  include/linux/coresight.h | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>> index b888f6ed59b2..26eb4a61b992 100644
>>> --- a/include/linux/coresight.h
>>> +++ b/include/linux/coresight.h
>>> @@ -476,15 +476,18 @@ static inline bool is_coresight_device(void __iomem *base)
>>>   * Returns:
>>>   *
>>>   * 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
>>>   */
>>>  static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
>>>  {
>>> -	struct clk *pclk;
>>> +	struct clk *pclk = NULL;
>>>  
>>> -	pclk = devm_clk_get_enabled(dev, "apb_pclk");
>>> -	if (IS_ERR(pclk))
>>> -		pclk = devm_clk_get_enabled(dev, "apb");
>>> +	if (!dev_is_amba(dev)) {
>>> +		pclk = devm_clk_get_enabled(dev, "apb_pclk");
>>> +		if (IS_ERR(pclk))
>>> +			pclk = devm_clk_get_enabled(dev, "apb");
>>> +	}
>>>  
>>>  	return pclk;
>>>  }