| Message ID | 20240122-ipq5332-nsscc-v4-2-19fa30019770@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add NSS clock controller support for Qualcomm IPQ5332 | expand |
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? > 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. Please correctly describe the clock tree in device tree, not hide clocks because your DT description is not complete. Andrew --- pw-bot: cr
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
On Wed, 14 Feb 2024 at 11:20, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> 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. > > > > >> 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. If there are no in-kernel consumers, there is no need to worry about them at all, nobody is going to access corresponding hardware. If you have out-of-tree modules, you also probably have your out-of-tree overlays. So you can make use of these clocks in your overlay. I don't see a point in dropping the clock if it is going to be readded later.
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. 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. Once you have actual drivers, this should solve itself, the drivers will consume the clocks. > 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
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
On Fri, 16 Feb 2024 at 17:33, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> wrote: > > > > 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. This means that NSSCC is also a consumer of those clocks. Please fix both DT and nsscc driver to handle NSSNOC clocks. > > 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? No. Using allyesconfig or allmodconfig and installing the full modules set should work. > >> 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 >
> 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 Cool, thanks. But is it really u-boot from 2016? > 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. So there is your solution, make NSSCC a consumer of the clocks it actually consumes. If it needs these clocks, it should get and enable them. Andrew
On 2/16/2024 10:46 PM, Andrew Lunn wrote: >> 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 > > Cool, thanks. But is it really u-boot from 2016? Yes, it is. If you want to try on IPQ95xx / IPQ53xx SoCs, you can also use the 2023's u-boot, which is available at [1]. [1] https://git.codelinaro.org/clo/qsdk/oss/boot/u-boot/-/tree/NHSS.QSDK.12.4.5?ref_type=heads > >> 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. > > So there is your solution, make NSSCC a consumer of the clocks it > actually consumes. If it needs these clocks, it should get and enable > them. Thanks for the suggestion. I will include these clocks in NSSCC DT node and enable the same in the NSSCC driver probe. > Andrew
<snip> >> 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. > > This means that NSSCC is also a consumer of those clocks. Please fix > both DT and nsscc driver to handle NSSNOC clocks. Thanks Dmitry. I shall include these clocks in the NSSCC DT node and enable the same in the NSSCC driver probe. > >>> 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? > > No. Using allyesconfig or allmodconfig and installing the full modules > set should work. > Okay, Got it. Thanks for the information. <snip>
On Sat, 17 Feb 2024 at 17:45, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> wrote: > > > <snip> > > >> 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. > > > > This means that NSSCC is also a consumer of those clocks. Please fix > > both DT and nsscc driver to handle NSSNOC clocks. > > > Thanks Dmitry. I shall include these clocks in the NSSCC DT node and > enable the same in the NSSCC driver probe. Or use them through pm_clk. This might be better, as the system doesn't need these clocks if NSSCC is suspended. > > > > >>> 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? > > > > No. Using allyesconfig or allmodconfig and installing the full modules > > set should work. > > > > > Okay, Got it. Thanks for the information. > > <snip>
On 2/17/2024 10:15 PM, Dmitry Baryshkov wrote: > On Sat, 17 Feb 2024 at 17:45, Kathiravan Thirumoorthy > <quic_kathirav@quicinc.com> wrote: >> >> >> <snip> >> >>>> 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. >>> >>> This means that NSSCC is also a consumer of those clocks. Please fix >>> both DT and nsscc driver to handle NSSNOC clocks. >> >> >> Thanks Dmitry. I shall include these clocks in the NSSCC DT node and >> enable the same in the NSSCC driver probe. > > Or use them through pm_clk. This might be better, as the system > doesn't need these clocks if NSSCC is suspended. IPQ53XX SoC doesn't support the PM(suspend / resume) functionality, so that, can I enable these clocks in NSSCC driver probe itself? > >> >>> >>>>> 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? >>> >>> No. Using allyesconfig or allmodconfig and installing the full modules >>> set should work. >>> >> >> >> Okay, Got it. Thanks for the information. >> >> <snip> > > >
On 18.02.2024 05:29, Kathiravan Thirumoorthy wrote: > > > On 2/17/2024 10:15 PM, Dmitry Baryshkov wrote: >> On Sat, 17 Feb 2024 at 17:45, Kathiravan Thirumoorthy >> <quic_kathirav@quicinc.com> wrote: >>> >>> >>> <snip> >>> >>>>> 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. >>>> >>>> This means that NSSCC is also a consumer of those clocks. Please fix >>>> both DT and nsscc driver to handle NSSNOC clocks. >>> >>> >>> Thanks Dmitry. I shall include these clocks in the NSSCC DT node and >>> enable the same in the NSSCC driver probe. >> >> Or use them through pm_clk. This might be better, as the system >> doesn't need these clocks if NSSCC is suspended. > > > IPQ53XX SoC doesn't support the PM(suspend / resume) functionality, so that, can I enable these clocks in NSSCC driver probe itself? Surely the platform can s2idle.. Konrad
On Sun, 18 Feb 2024 at 06:29, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> wrote: > > > > On 2/17/2024 10:15 PM, Dmitry Baryshkov wrote: > > On Sat, 17 Feb 2024 at 17:45, Kathiravan Thirumoorthy > > <quic_kathirav@quicinc.com> wrote: > >> > >> > >> <snip> > >> > >>>> 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. > >>> > >>> This means that NSSCC is also a consumer of those clocks. Please fix > >>> both DT and nsscc driver to handle NSSNOC clocks. > >> > >> > >> Thanks Dmitry. I shall include these clocks in the NSSCC DT node and > >> enable the same in the NSSCC driver probe. > > > > Or use them through pm_clk. This might be better, as the system > > doesn't need these clocks if NSSCC is suspended. > > > IPQ53XX SoC doesn't support the PM(suspend / resume) functionality, so > that, can I enable these clocks in NSSCC driver probe itself? There is a difference between PM (suspend/resume) and runtime PM.
On 2/19/2024 3:53 PM, Dmitry Baryshkov wrote: > On Sun, 18 Feb 2024 at 06:29, Kathiravan Thirumoorthy > <quic_kathirav@quicinc.com> wrote: >> >> >> >> On 2/17/2024 10:15 PM, Dmitry Baryshkov wrote: >>> On Sat, 17 Feb 2024 at 17:45, Kathiravan Thirumoorthy >>> <quic_kathirav@quicinc.com> wrote: >>>> >>>> >>>> <snip> >>>> >>>>>> 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. >>>>> >>>>> This means that NSSCC is also a consumer of those clocks. Please fix >>>>> both DT and nsscc driver to handle NSSNOC clocks. >>>> >>>> >>>> Thanks Dmitry. I shall include these clocks in the NSSCC DT node and >>>> enable the same in the NSSCC driver probe. >>> >>> Or use them through pm_clk. This might be better, as the system >>> doesn't need these clocks if NSSCC is suspended. >> >> >> IPQ53XX SoC doesn't support the PM(suspend / resume) functionality, so >> that, can I enable these clocks in NSSCC driver probe itself? > > There is a difference between PM (suspend/resume) and runtime PM. > > Thanks Dmitry. IIUC your question correctly, runtime PM for the peripherals are not supported (except CPU cores which supports DVFS). Since these are router based products, once system is powered on, all the peripherals are configured to the required frequency and it will be never go into low power modes. Please let me know if this answers your questions.
On Fri, 23 Feb 2024 at 12:18, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> wrote: > > > > On 2/19/2024 3:53 PM, Dmitry Baryshkov wrote: > > On Sun, 18 Feb 2024 at 06:29, Kathiravan Thirumoorthy > > <quic_kathirav@quicinc.com> wrote: > >> > >> > >> > >> On 2/17/2024 10:15 PM, Dmitry Baryshkov wrote: > >>> On Sat, 17 Feb 2024 at 17:45, Kathiravan Thirumoorthy > >>> <quic_kathirav@quicinc.com> wrote: > >>>> > >>>> > >>>> <snip> > >>>> > >>>>>> 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. > >>>>> > >>>>> This means that NSSCC is also a consumer of those clocks. Please fix > >>>>> both DT and nsscc driver to handle NSSNOC clocks. > >>>> > >>>> > >>>> Thanks Dmitry. I shall include these clocks in the NSSCC DT node and > >>>> enable the same in the NSSCC driver probe. > >>> > >>> Or use them through pm_clk. This might be better, as the system > >>> doesn't need these clocks if NSSCC is suspended. > >> > >> > >> IPQ53XX SoC doesn't support the PM(suspend / resume) functionality, so > >> that, can I enable these clocks in NSSCC driver probe itself? > > > > There is a difference between PM (suspend/resume) and runtime PM. > > > > > > Thanks Dmitry. IIUC your question correctly, runtime PM for the > peripherals are not supported (except CPU cores which supports DVFS). > Since these are router based products, once system is powered on, all > the peripherals are configured to the required frequency and it will be > never go into low power modes. > > Please let me know if this answers your questions. It seems there is a misunderstanding somewhere. Runtime PM allows the Linux kernel to disable temporary unused devices at runtime. E.g. if the NSS is switched off, the kernel can switch NSSCC off too, cutting the power. It has nothing to do with the frequency of the device / clock or with the product being a router or a mobile device.
diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c index 66d5399798fe..38a570b68813 100644 --- a/drivers/clk/qcom/gcc-ipq5332.c +++ b/drivers/clk/qcom/gcc-ipq5332.c @@ -1672,24 +1672,6 @@ static struct clk_branch gcc_nssnoc_atb_clk = { }, }; -static struct clk_branch gcc_nssnoc_nsscc_clk = { - .halt_reg = 0x17030, - .halt_check = BRANCH_HALT, - .clkr = { - .enable_reg = 0x17030, - .enable_mask = BIT(0), - .hw.init = &(const struct clk_init_data) { - .name = "gcc_nssnoc_nsscc_clk", - .parent_hws = (const struct clk_hw*[]) { - &gcc_pcnoc_bfdcd_clk_src.clkr.hw, - }, - .num_parents = 1, - .flags = CLK_SET_RATE_PARENT, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gcc_nssnoc_qosgen_ref_clk = { .halt_reg = 0x1701c, .halt_check = BRANCH_HALT, @@ -2585,42 +2567,6 @@ static struct clk_branch gcc_snoc_lpass_cfg_clk = { }, }; -static struct clk_branch gcc_snoc_nssnoc_1_clk = { - .halt_reg = 0x17090, - .halt_check = BRANCH_HALT, - .clkr = { - .enable_reg = 0x17090, - .enable_mask = BIT(0), - .hw.init = &(const struct clk_init_data) { - .name = "gcc_snoc_nssnoc_1_clk", - .parent_hws = (const struct clk_hw*[]) { - &gcc_system_noc_bfdcd_clk_src.clkr.hw, - }, - .num_parents = 1, - .flags = CLK_SET_RATE_PARENT, - .ops = &clk_branch2_ops, - }, - }, -}; - -static struct clk_branch gcc_snoc_nssnoc_clk = { - .halt_reg = 0x17084, - .halt_check = BRANCH_HALT, - .clkr = { - .enable_reg = 0x17084, - .enable_mask = BIT(0), - .hw.init = &(const struct clk_init_data) { - .name = "gcc_snoc_nssnoc_clk", - .parent_hws = (const struct clk_hw*[]) { - &gcc_system_noc_bfdcd_clk_src.clkr.hw, - }, - .num_parents = 1, - .flags = CLK_SET_RATE_PARENT, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gcc_snoc_pcie3_1lane_1_m_clk = { .halt_reg = 0x2e050, .halt_check = BRANCH_HALT, @@ -3330,7 +3276,6 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = { [GCC_NSSCC_CLK] = &gcc_nsscc_clk.clkr, [GCC_NSSCFG_CLK] = &gcc_nsscfg_clk.clkr, [GCC_NSSNOC_ATB_CLK] = &gcc_nssnoc_atb_clk.clkr, - [GCC_NSSNOC_NSSCC_CLK] = &gcc_nssnoc_nsscc_clk.clkr, [GCC_NSSNOC_QOSGEN_REF_CLK] = &gcc_nssnoc_qosgen_ref_clk.clkr, [GCC_NSSNOC_SNOC_1_CLK] = &gcc_nssnoc_snoc_1_clk.clkr, [GCC_NSSNOC_SNOC_CLK] = &gcc_nssnoc_snoc_clk.clkr, @@ -3398,8 +3343,6 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = { [GCC_SDCC1_APPS_CLK_SRC] = &gcc_sdcc1_apps_clk_src.clkr, [GCC_SLEEP_CLK_SRC] = &gcc_sleep_clk_src.clkr, [GCC_SNOC_LPASS_CFG_CLK] = &gcc_snoc_lpass_cfg_clk.clkr, - [GCC_SNOC_NSSNOC_1_CLK] = &gcc_snoc_nssnoc_1_clk.clkr, - [GCC_SNOC_NSSNOC_CLK] = &gcc_snoc_nssnoc_clk.clkr, [GCC_SNOC_PCIE3_1LANE_1_M_CLK] = &gcc_snoc_pcie3_1lane_1_m_clk.clkr, [GCC_SNOC_PCIE3_1LANE_1_S_CLK] = &gcc_snoc_pcie3_1lane_1_s_clk.clkr, [GCC_SNOC_PCIE3_1LANE_M_CLK] = &gcc_snoc_pcie3_1lane_m_clk.clkr, @@ -3660,7 +3603,18 @@ static const struct qcom_cc_desc gcc_ipq5332_desc = { static int gcc_ipq5332_probe(struct platform_device *pdev) { - return qcom_cc_probe(pdev, &gcc_ipq5332_desc); + struct regmap *regmap; + + regmap = qcom_cc_map(pdev, &gcc_ipq5332_desc); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + /* Keep the critical clocks always-On */ + regmap_update_bits(regmap, 0x17030, BIT(0), BIT(0)); /* gcc_nssnoc_nsscc_clk */ + regmap_update_bits(regmap, 0x17084, BIT(0), BIT(0)); /* gcc_snoc_nssnoc_clk */ + regmap_update_bits(regmap, 0x17090, BIT(0), BIT(0)); /* gcc_snoc_nssnoc_1_clk */ + + return qcom_cc_really_probe(pdev, &gcc_ipq5332_desc, regmap); } static const struct of_device_id gcc_ipq5332_match_table[] = {