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

[Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>]

On 2/14/2024 8:14 PM, Andrew Lunn wrote:
> On Wed, Feb 14, 2024 at 02:49:41PM +0530, Kathiravan Thirumoorthy wrote:
>>
>>
>> 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.
> 
> I've never used QSDK u-boot, so i can only make comments based on my
> experience with other vendors build of u-boot. That experience is, its
> broken for my use cases, and i try to replace it as soon as possible
> with upstream.
> 
> I generally want to TFTP boot the kernel and the DT blob. Sometimes
> vendor u-boot has networking disabled. Or the TFTP client is
> missing. If it is there, the IP addresses are fixed, and i don't want
> to modify my network to make it compatible with the vendor
> requirements. If the IP addresses can be configured, sometimes there
> is no FLASH support so its not possible to actually write the
> configuration to FLASH so that it does the right thing on reboot
> etc...
> 
> Often the vendor u-boot is a black box, no sources. Can you give me a
> git URL for the u-boot in QSDK? If the sources are open, i could at
> least rebuild it with everything turned on.


You can get the source at 
https://git.codelinaro.org/clo/qsdk/oss/boot/u-boot-2016/-/tree/NHSS.QSDK.12.2?ref_type=heads

You should be able to TFTP the images, write into the flash and 
configure the IP and so on...


> 
> But still, it is better that Linux makes no assumptions about what the
> boot loader has done. That makes it much easier to change the
> bootloader.
> 
>>>> 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).
> 
> If there is no network drivers, you don't need clocks to the
> networking hardware. So CCF turning them off seems correct.


Yeah agree with your comments.

QSDK's u-boot enables the network support, so the required NSSCC clocks 
are turned ON and left it in ON state. CCF tries to disables the unused 
NSSCC clocks but system goes for reboot.


Reason being, to access the NSSCC clocks, these GCC clocks 
(gcc_snoc_nssnoc_clk, gcc_snoc_nssnoc_1_clk, gcc_nssnoc_nsscc_clk)
should be turned ON. But CCF disables these clocks as well due to the 
lack of consumer.


> 
> Once you have actual drivers, this should solve itself, the drivers
> will consume the clocks.


Given that, NSSCC is being built as module, there is no issue in booting 
the kernel. But if you do insmod of the nsscc-ipq5332.ko, system will 
reset.

Without the networking drivers, there is no need to install this module. 
And as you stated, once the drivers are available, there will be no issues.

So can I explain the shortcomings of installing this module without the 
networking drivers in cover letter and drop this patch all together?



> 
>> 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?
> 
> But why should you remove them. If nothing is using them, they should
> be turned off.
> 
>     Andrew