| Message ID | 20240122-ipq5332-nsscc-v4-2-19fa30019770@quicinc.com |
|---|---|
| State | New |
| 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 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[] = {