Re: [PATCH v4 2/8] clk: qcom: ipq5332: enable few nssnoc clocks in driver probe

[Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>]

On 1/26/2024 1:35 AM, Andrew Lunn wrote:
> On Mon, Jan 22, 2024 at 11:26:58AM +0530, Kathiravan Thirumoorthy wrote:
>> gcc_snoc_nssnoc_clk, gcc_snoc_nssnoc_1_clk, gcc_nssnoc_nsscc_clk are
>> enabled by default and it's RCG is properly configured by bootloader.
> 
> Which bootloader? Mainline barebox?


Thanks for taking time to review the patches. I couldn't get time to 
respond back, sorry for the delay.

I was referring to the U-boot which is delivered as part of the QSDK. I 
will call it out explicitly in the next patch.

> 
>> Some of the NSS clocks needs these clocks to be enabled. To avoid
>> these clocks being disabled by clock framework, drop these entries
>> from the clock table and enable it in the driver probe itself.
> 
> If they are critical clocks, i would expect a device to reference
> them. The CCF only disabled unused clocks in late_initcall_sync(),
> which means all drivers should of probed and taken a reference on any
> clocks they require.


Some of the NSSCC clocks are enabled by bootloaders and CCF disables the 
same (because currently there are no consumers for these clocks 
available in the tree. These clocks are consumed by the Networking 
drivers which are being upstreamed). To access the NSSCC clocks, 
gcc_snoc_nssnoc_clk, gcc_snoc_nssnoc_1_clk, gcc_nssnoc_nsscc_clk clocks 
needs to be enabled, else system is going to reboot. To prevent this, I 
enabled it in probe.

However looking back, gcc_snoc_nssnoc_clk, gcc_snoc_nssnoc_1_clk, 
gcc_nssnoc_nsscc_clk are consumed by the networking drivers only. So is 
it okay to drop these clocks from the GCC driver and add it back once 
the actual consumer needs it? So that we don't have to enable it in probe.

Please let me know your thoughts.


> 
> Please correctly describe the clock tree in device tree, not hide
> clocks because your DT description is not complete.
> 
>      Andrew
> 
> ---
> pw-bot: cr