| Message ID | 20240321043149.2739204-3-quic_varada@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect driver for IPQ9574 SoC | expand |
On 21/03/2024 05:31, Varadarajan Narayanan wrote: > Unlike MSM platforms that manage NoC related clocks and scaling > from RPM, IPQ SoCs dont involve RPM in managing NoC related > clocks and there is no NoC scaling. If these are clocks, expose them as clocks, not as interconnects. > > However, there is a requirement to enable some NoC interface > clocks for accessing the peripheral controllers present on > these NoCs. > > Hence adding a minimalistic interconnect driver that can enable > the relevant clocks. This is similar to msm8996-cbf's usage of > icc-clk framework. > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 2 + DTS is always, ALWAYS, separate. > drivers/clk/qcom/gcc-ipq9574.c | 75 ++++++++++++++++++++++++++- > 2 files changed, 76 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > index 7f2e5cbf3bbb..efffbd085715 100644 > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > @@ -11,6 +11,7 @@ > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/reset/qcom,ipq9574-gcc.h> > #include <dt-bindings/thermal/thermal.h> > +#include <dt-bindings/interconnect/qcom,ipq9574.h> Keep the order, Best regards, Krzysztof
On Thu, Mar 21, 2024 at 08:25:15AM +0100, Krzysztof Kozlowski wrote: > On 21/03/2024 05:31, Varadarajan Narayanan wrote: > > Unlike MSM platforms that manage NoC related clocks and scaling > > from RPM, IPQ SoCs dont involve RPM in managing NoC related > > clocks and there is no NoC scaling. > > If these are clocks, expose them as clocks, not as interconnects. Earlier IPQ9574 PCIe patches were NAK-ed when these were exposed as clocks. Please refer to the following discussions https://lore.kernel.org/linux-arm-msm/CAA8EJpq0uawrOBHA8XHygEpGYF--HyxJWxKG44iiFdAZZz7O2w@mail.gmail.com/ https://lore.kernel.org/linux-arm-msm/CAA8EJppabK8j9T40waMv=t-1aksXfqJibWuS41GhruzLhpatrg@mail.gmail.com/ Dmitry had said <quote> I'd kindly suggest implementing the NoC attachment properly. In the end, other Qualcomm platforms use ICC drivers, so by following this pattern we will have more common code paths. </quote> Hence posted these patches to get feedback. > > However, there is a requirement to enable some NoC interface > > clocks for accessing the peripheral controllers present on > > these NoCs. > > > > Hence adding a minimalistic interconnect driver that can enable > > the relevant clocks. This is similar to msm8996-cbf's usage of > > icc-clk framework. > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 2 + > > DTS is always, ALWAYS, separate. Ok. > > > drivers/clk/qcom/gcc-ipq9574.c | 75 ++++++++++++++++++++++++++- > > 2 files changed, 76 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > index 7f2e5cbf3bbb..efffbd085715 100644 > > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > @@ -11,6 +11,7 @@ > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > #include <dt-bindings/reset/qcom,ipq9574-gcc.h> > > #include <dt-bindings/thermal/thermal.h> > > +#include <dt-bindings/interconnect/qcom,ipq9574.h> > > Keep the order, Ok. Thanks Varada
On 21/03/2024 10:56, Varadarajan Narayanan wrote: > On Thu, Mar 21, 2024 at 08:25:15AM +0100, Krzysztof Kozlowski wrote: >> On 21/03/2024 05:31, Varadarajan Narayanan wrote: >>> Unlike MSM platforms that manage NoC related clocks and scaling >>> from RPM, IPQ SoCs dont involve RPM in managing NoC related >>> clocks and there is no NoC scaling. >> >> If these are clocks, expose them as clocks, not as interconnects. > > Earlier IPQ9574 PCIe patches were NAK-ed when these were exposed > as clocks. Please refer to the following discussions > > https://lore.kernel.org/linux-arm-msm/CAA8EJpq0uawrOBHA8XHygEpGYF--HyxJWxKG44iiFdAZZz7O2w@mail.gmail.com/ > https://lore.kernel.org/linux-arm-msm/CAA8EJppabK8j9T40waMv=t-1aksXfqJibWuS41GhruzLhpatrg@mail.gmail.com/ > > Dmitry had said > <quote> > I'd kindly suggest implementing the NoC attachment > properly. In the end, other Qualcomm platforms use ICC > drivers, so by following this pattern we will have more > common code paths. > </quote> > > Hence posted these patches to get feedback. Then explain the rationale in commit msg. Best regards, Krzysztof
Hi Varadarajan,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on clk/clk-next linus/master v6.8 next-20240321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Varadarajan-Narayanan/dt-bindings-interconnect-Add-Qualcomm-IPQ9574-support/20240321-123508
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240321043149.2739204-3-quic_varada%40quicinc.com
patch subject: [PATCH 2/2] clk: qcom: add IPQ9574 interconnect clocks support
config: csky-randconfig-001-20240321 (https://download.01.org/0day-ci/archive/20240322/202403221357.pOXvpS3O-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240322/202403221357.pOXvpS3O-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403221357.pOXvpS3O-lkp@intel.com/
All errors (new ones prefixed by >>):
csky-linux-ld: drivers/clk/qcom/gcc-ipq9574.o: in function `gcc_ipq9574_probe':
gcc-ipq9574.c:(.text+0xc2): undefined reference to `icc_clk_register'
>> csky-linux-ld: gcc-ipq9574.c:(.text+0x10c): undefined reference to `icc_clk_register'
Hi Varadarajan, kernel test robot noticed the following build errors: [auto build test ERROR on robh/for-next] [also build test ERROR on clk/clk-next linus/master v6.8 next-20240322] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Varadarajan-Narayanan/dt-bindings-interconnect-Add-Qualcomm-IPQ9574-support/20240321-123508 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20240321043149.2739204-3-quic_varada%40quicinc.com patch subject: [PATCH 2/2] clk: qcom: add IPQ9574 interconnect clocks support config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240322/202403221944.SAbczEhw-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240322/202403221944.SAbczEhw-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403221944.SAbczEhw-lkp@intel.com/ All errors (new ones prefixed by >>): alpha-linux-ld: drivers/clk/qcom/gcc-ipq9574.o: in function `gcc_ipq9574_probe': >> (.text+0x1a0): undefined reference to `icc_clk_register' >> alpha-linux-ld: (.text+0x1ac): undefined reference to `icc_clk_register'
On 21.03.2024 05:31, Varadarajan Narayanan wrote: > Unlike MSM platforms that manage NoC related clocks and scaling > from RPM, IPQ SoCs dont involve RPM in managing NoC related > clocks and there is no NoC scaling. > > However, there is a requirement to enable some NoC interface > clocks for accessing the peripheral controllers present on > these NoCs. > > Hence adding a minimalistic interconnect driver that can enable > the relevant clocks. This is similar to msm8996-cbf's usage of > icc-clk framework. > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- [...] > @@ -9,9 +9,16 @@ > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > +#if IS_ENABLED(CONFIG_INTERCONNECT) This is bad practice, especially given the reasoning for your changes. It's best if you add a dependency on interconnect to this driver, otherwise things will go into uncountable EPROBE_DEFERs if there are nodes consuming icc handles, but the supplier never registers. [...] > > static int gcc_ipq9574_probe(struct platform_device *pdev) ..and that approach could save the probe func from the absolute mess it has become with this patch Konrad
On Sat, Mar 23, 2024 at 01:29:00AM +0100, Konrad Dybcio wrote: > On 21.03.2024 05:31, Varadarajan Narayanan wrote: > > Unlike MSM platforms that manage NoC related clocks and scaling > > from RPM, IPQ SoCs dont involve RPM in managing NoC related > > clocks and there is no NoC scaling. > > > > However, there is a requirement to enable some NoC interface > > clocks for accessing the peripheral controllers present on > > these NoCs. > > > > Hence adding a minimalistic interconnect driver that can enable > > the relevant clocks. This is similar to msm8996-cbf's usage of > > icc-clk framework. > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > [...] > > > @@ -9,9 +9,16 @@ > > #include <linux/of.h> > > #include <linux/platform_device.h> > > #include <linux/regmap.h> > > +#if IS_ENABLED(CONFIG_INTERCONNECT) > > This is bad practice, especially given the reasoning for your changes. > > It's best if you add a dependency on interconnect to this driver, > otherwise things will go into uncountable EPROBE_DEFERs if there are > nodes consuming icc handles, but the supplier never registers. > > [...] > > > > > static int gcc_ipq9574_probe(struct platform_device *pdev) > > ..and that approach could save the probe func from the absolute mess it > has become with this patch > > Konrad Thanks for the feedback. Have addressed these and other reviewers comments and posted v2. Please take a look. -Varada
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi index 7f2e5cbf3bbb..efffbd085715 100644 --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi @@ -11,6 +11,7 @@ #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/reset/qcom,ipq9574-gcc.h> #include <dt-bindings/thermal/thermal.h> +#include <dt-bindings/interconnect/qcom,ipq9574.h> / { interrupt-parent = <&intc>; @@ -306,6 +307,7 @@ gcc: clock-controller@1800000 { #clock-cells = <1>; #reset-cells = <1>; #power-domain-cells = <1>; + #interconnect-cells = <1>; }; tcsr_mutex: hwlock@1905000 { diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c index 0a3f846695b8..edbf223719e4 100644 --- a/drivers/clk/qcom/gcc-ipq9574.c +++ b/drivers/clk/qcom/gcc-ipq9574.c @@ -9,9 +9,16 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/regmap.h> +#if IS_ENABLED(CONFIG_INTERCONNECT) +#include <linux/interconnect-clk.h> +#include <linux/interconnect-provider.h> +#endif #include <dt-bindings/clock/qcom,ipq9574-gcc.h> #include <dt-bindings/reset/qcom,ipq9574-gcc.h> +#if IS_ENABLED(CONFIG_INTERCONNECT) +#include <dt-bindings/interconnect/qcom,ipq9574.h> +#endif #include "clk-alpha-pll.h" #include "clk-branch.h" @@ -4301,6 +4308,35 @@ static const struct qcom_reset_map gcc_ipq9574_resets[] = { [GCC_WCSS_Q6_TBU_BCR] = { 0x12054, 0 }, }; + +#if IS_ENABLED(CONFIG_INTERCONNECT) +static struct icc_clk_data *icc_ipq9574; + +static int noc_clks[] = { + GCC_ANOC_PCIE0_1LANE_M_CLK, + GCC_SNOC_PCIE0_1LANE_S_CLK, + GCC_ANOC_PCIE1_1LANE_M_CLK, + GCC_SNOC_PCIE1_1LANE_S_CLK, + GCC_ANOC_PCIE2_2LANE_M_CLK, + GCC_SNOC_PCIE2_2LANE_S_CLK, + GCC_ANOC_PCIE3_2LANE_M_CLK, + GCC_SNOC_PCIE3_2LANE_S_CLK, + GCC_SNOC_USB_CLK, + GCC_ANOC_USB_AXI_CLK, + GCC_NSSNOC_NSSCC_CLK, + GCC_NSSNOC_SNOC_CLK, + GCC_NSSNOC_SNOC_1_CLK, + GCC_NSSNOC_PCNOC_1_CLK, + GCC_NSSNOC_QOSGEN_REF_CLK, + GCC_NSSNOC_TIMEOUT_REF_CLK, + GCC_NSSNOC_XO_DCD_CLK, + GCC_NSSNOC_ATB_CLK, + GCC_MEM_NOC_NSSNOC_CLK, + GCC_NSSNOC_MEMNOC_CLK, + GCC_NSSNOC_MEM_NOC_1_CLK, +}; +#endif + static const struct of_device_id gcc_ipq9574_match_table[] = { { .compatible = "qcom,ipq9574-gcc" }, { } @@ -4327,7 +4363,44 @@ static const struct qcom_cc_desc gcc_ipq9574_desc = { static int gcc_ipq9574_probe(struct platform_device *pdev) { - return qcom_cc_probe(pdev, &gcc_ipq9574_desc); + int ret = qcom_cc_probe(pdev, &gcc_ipq9574_desc); +#if IS_ENABLED(CONFIG_INTERCONNECT) + struct icc_provider *provider; + struct icc_clk_data *icd; + int i; +#endif + + if (ret) + return dev_err_probe(&pdev->dev, ret, "%s failed\n", __func__); + +#if IS_ENABLED(CONFIG_INTERCONNECT) + icd = devm_kmalloc(&pdev->dev, ARRAY_SIZE(noc_clks) * sizeof(*icd), + GFP_KERNEL); + + if (IS_ERR_OR_NULL(icd)) + return dev_err_probe(&pdev->dev, PTR_ERR(icd), + "%s malloc failed\n", __func__); + + icc_ipq9574 = icd; + + for (i = 0; i < ARRAY_SIZE(noc_clks); i++, icd++) { + icd->clk = gcc_ipq9574_clks[noc_clks[i]]->hw.clk; + if (IS_ERR_OR_NULL(icd->clk)) { + dev_err(&pdev->dev, "%s: %d clock not found\n", + __func__, noc_clks[i]); + return -ENOENT; + } + icd->name = clk_hw_get_name(&gcc_ipq9574_clks[noc_clks[i]]->hw); + } + + provider = icc_clk_register(&pdev->dev, IPQ_APPS_ID, + ARRAY_SIZE(noc_clks), icc_ipq9574); + if (IS_ERR_OR_NULL(provider)) + return dev_err_probe(&pdev->dev, PTR_ERR(provider), + "%s: icc_clk_register failed\n", __func__); +#endif + + return 0; } static struct platform_driver gcc_ipq9574_driver = {
Unlike MSM platforms that manage NoC related clocks and scaling from RPM, IPQ SoCs dont involve RPM in managing NoC related clocks and there is no NoC scaling. However, there is a requirement to enable some NoC interface clocks for accessing the peripheral controllers present on these NoCs. Hence adding a minimalistic interconnect driver that can enable the relevant clocks. This is similar to msm8996-cbf's usage of icc-clk framework. Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> --- arch/arm64/boot/dts/qcom/ipq9574.dtsi | 2 + drivers/clk/qcom/gcc-ipq9574.c | 75 ++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-)