| Message ID | 20240328075936.223461-5-quic_varada@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect driver for IPQ9574 SoC | expand |
On Fri, Mar 29, 2024 at 01:10:03PM +0100, Krzysztof Kozlowski wrote: > On 29/03/2024 11:55, Varadarajan Narayanan wrote: > >>> + > >>> +enum { > >>> + ICC_ANOC_PCIE0, > >>> + ICC_SNOC_PCIE0, > >>> + ICC_ANOC_PCIE1, > >>> + ICC_SNOC_PCIE1, > >>> + ICC_ANOC_PCIE2, > >>> + ICC_SNOC_PCIE2, > >>> + ICC_ANOC_PCIE3, > >>> + ICC_SNOC_PCIE3, > >>> + ICC_SNOC_USB, > >>> + ICC_ANOC_USB_AXI, > >>> + ICC_NSSNOC_NSSCC, > >>> + ICC_NSSNOC_SNOC_0, > >>> + ICC_NSSNOC_SNOC_1, > >>> + ICC_NSSNOC_PCNOC_1, > >>> + ICC_NSSNOC_QOSGEN_REF, > >>> + ICC_NSSNOC_TIMEOUT_REF, > >>> + ICC_NSSNOC_XO_DCD, > >>> + ICC_NSSNOC_ATB, > >>> + ICC_MEM_NOC_NSSNOC, > >>> + ICC_NSSNOC_MEMNOC, > >>> + ICC_NSSNOC_MEM_NOC_1, > >>> +}; > >> > >> Are these supposed to be in a dt-binding header? > > > > Since these don't directly relate to the ids in the dt-bindings > > not sure if they will be permitted there. Will move and post a > > new version and get feedback. > > You can answer this by yourself by looking at your DTS. Do you use them > as well in the DTS? I can use them in the DTS. The icc-clk framework automatically creates master and slave nodes as 'n' and 'n+1'. Hence I can have something like this in the dt-bindings include file #define ICC_ANOC_PCIE0 0 #define ICC_SNOC_PCIE0 1 . . . #define ICC_NSSNOC_MEM_NOC_1 20 #define MASTER(x) ((ICC_ ## x) * 2) #define SLAVE(x) (MASTER(x) + 1) > It's a pity we see here only parts of DTS, instead of full interconnect > usage. Unfortunately cannot include the pcie dts changes with this patch, but you can refer to them at https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@quicinc.com/ The above macros will be used in the pcie node as follows pcie0: pci@28000000 { compatible = "qcom,pcie-ipq9574"; . . . interconnects = <&gcc MASTER(ANOC_PCIE0) &gcc SLAVE(ANOC_PCIE0)>, <&gcc MASTER(SNOC_PCIE0) &gcc SLAVE(SNOC_PCIE0)>; interconnect-names = "pcie-mem", "cpu-pcie"; . . . }; Hope this is acceptable. Thanks Varada
On 30/03/2024 10:30, Varadarajan Narayanan wrote: > On Fri, Mar 29, 2024 at 01:10:03PM +0100, Krzysztof Kozlowski wrote: >> On 29/03/2024 11:55, Varadarajan Narayanan wrote: >>>>> + >>>>> +enum { >>>>> + ICC_ANOC_PCIE0, >>>>> + ICC_SNOC_PCIE0, >>>>> + ICC_ANOC_PCIE1, >>>>> + ICC_SNOC_PCIE1, >>>>> + ICC_ANOC_PCIE2, >>>>> + ICC_SNOC_PCIE2, >>>>> + ICC_ANOC_PCIE3, >>>>> + ICC_SNOC_PCIE3, >>>>> + ICC_SNOC_USB, >>>>> + ICC_ANOC_USB_AXI, >>>>> + ICC_NSSNOC_NSSCC, >>>>> + ICC_NSSNOC_SNOC_0, >>>>> + ICC_NSSNOC_SNOC_1, >>>>> + ICC_NSSNOC_PCNOC_1, >>>>> + ICC_NSSNOC_QOSGEN_REF, >>>>> + ICC_NSSNOC_TIMEOUT_REF, >>>>> + ICC_NSSNOC_XO_DCD, >>>>> + ICC_NSSNOC_ATB, >>>>> + ICC_MEM_NOC_NSSNOC, >>>>> + ICC_NSSNOC_MEMNOC, >>>>> + ICC_NSSNOC_MEM_NOC_1, >>>>> +}; >>>> >>>> Are these supposed to be in a dt-binding header? >>> >>> Since these don't directly relate to the ids in the dt-bindings >>> not sure if they will be permitted there. Will move and post a >>> new version and get feedback. >> >> You can answer this by yourself by looking at your DTS. Do you use them >> as well in the DTS? > > I can use them in the DTS. The icc-clk framework automatically > creates master and slave nodes as 'n' and 'n+1'. Hence I can have > something like this in the dt-bindings include file > > #define ICC_ANOC_PCIE0 0 > #define ICC_SNOC_PCIE0 1 > . > . > . > #define ICC_NSSNOC_MEM_NOC_1 20 > > #define MASTER(x) ((ICC_ ## x) * 2) > #define SLAVE(x) (MASTER(x) + 1) I don't understand this or maybe I misunderstood the purpose of this define. It does not matter if you "can" use something in DT. The question is: do you use them. > >> It's a pity we see here only parts of DTS, instead of full interconnect >> usage. > > Unfortunately cannot include the pcie dts changes with this > patch, but you can refer to them at https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@quicinc.com/ > > The above macros will be used in the pcie node as follows > > pcie0: pci@28000000 { > compatible = "qcom,pcie-ipq9574"; > . . . > interconnects = <&gcc MASTER(ANOC_PCIE0) &gcc SLAVE(ANOC_PCIE0)>, > <&gcc MASTER(SNOC_PCIE0) &gcc SLAVE(SNOC_PCIE0)>; > interconnect-names = "pcie-mem", "cpu-pcie"; Then why did you add header which is not used? I will respond there... Best regards, Krzysztof
On Sat, Mar 30, 2024 at 11:28:09AM +0100, Krzysztof Kozlowski wrote: > On 30/03/2024 10:30, Varadarajan Narayanan wrote: > > On Fri, Mar 29, 2024 at 01:10:03PM +0100, Krzysztof Kozlowski wrote: > >> On 29/03/2024 11:55, Varadarajan Narayanan wrote: > >>>>> + > >>>>> +enum { > >>>>> + ICC_ANOC_PCIE0, > >>>>> + ICC_SNOC_PCIE0, > >>>>> + ICC_ANOC_PCIE1, > >>>>> + ICC_SNOC_PCIE1, > >>>>> + ICC_ANOC_PCIE2, > >>>>> + ICC_SNOC_PCIE2, > >>>>> + ICC_ANOC_PCIE3, > >>>>> + ICC_SNOC_PCIE3, > >>>>> + ICC_SNOC_USB, > >>>>> + ICC_ANOC_USB_AXI, > >>>>> + ICC_NSSNOC_NSSCC, > >>>>> + ICC_NSSNOC_SNOC_0, > >>>>> + ICC_NSSNOC_SNOC_1, > >>>>> + ICC_NSSNOC_PCNOC_1, > >>>>> + ICC_NSSNOC_QOSGEN_REF, > >>>>> + ICC_NSSNOC_TIMEOUT_REF, > >>>>> + ICC_NSSNOC_XO_DCD, > >>>>> + ICC_NSSNOC_ATB, > >>>>> + ICC_MEM_NOC_NSSNOC, > >>>>> + ICC_NSSNOC_MEMNOC, > >>>>> + ICC_NSSNOC_MEM_NOC_1, > >>>>> +}; > >>>> > >>>> Are these supposed to be in a dt-binding header? > >>> > >>> Since these don't directly relate to the ids in the dt-bindings > >>> not sure if they will be permitted there. Will move and post a > >>> new version and get feedback. > >> > >> You can answer this by yourself by looking at your DTS. Do you use them > >> as well in the DTS? > > > > I can use them in the DTS. The icc-clk framework automatically > > creates master and slave nodes as 'n' and 'n+1'. Hence I can have > > something like this in the dt-bindings include file > > > > #define ICC_ANOC_PCIE0 0 > > #define ICC_SNOC_PCIE0 1 > > . > > . > > . > > #define ICC_NSSNOC_MEM_NOC_1 20 > > > > #define MASTER(x) ((ICC_ ## x) * 2) > > #define SLAVE(x) (MASTER(x) + 1) > > I don't understand this or maybe I misunderstood the purpose of this > define. It does not matter if you "can" use something in DT. The > question is: do you use them. Yes. It will be used fot the pcie nodes. These defines are for specifying the endpoints. The icc driver identifies a path that can connect these endpoints. The peripheral drivers' DT nodes will make use of these defines. > >> It's a pity we see here only parts of DTS, instead of full interconnect > >> usage. > > > > Unfortunately cannot include the pcie dts changes with this > > patch, but you can refer to them at https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@quicinc.com/ > > > > The above macros will be used in the pcie node as follows > > > > pcie0: pci@28000000 { > > compatible = "qcom,pcie-ipq9574"; > > . . . > > interconnects = <&gcc MASTER(ANOC_PCIE0) &gcc SLAVE(ANOC_PCIE0)>, > > <&gcc MASTER(SNOC_PCIE0) &gcc SLAVE(SNOC_PCIE0)>; > > interconnect-names = "pcie-mem", "cpu-pcie"; > > Then why did you add header which is not used? Since they are a part of the interconnect driver. The peripherals that use the interconnects will make use of them to specify the endpoints. After changing per Boyd's comments, the header will be used from gcc driver. Will post a new version. Kindly review that. Thanks Varada > I will respond there... > > Best regards, > Krzysztof >
Hi Varadarajan,
kernel test robot noticed the following build errors:
[auto build test ERROR on clk/clk-next]
[also build test ERROR on robh/for-next linus/master v6.9-rc2 next-20240328]
[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/20240328-160522
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link: https://lore.kernel.org/r/20240328075936.223461-5-quic_varada%40quicinc.com
patch subject: [PATCH v5 4/5] clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks
config: powerpc64-randconfig-r132-20240331 (https://download.01.org/0day-ci/archive/20240401/202404012258.MFriF5BV-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240401/202404012258.MFriF5BV-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/202404012258.MFriF5BV-lkp@intel.com/
All errors (new ones prefixed by >>):
powerpc64-linux-ld: drivers/clk/qcom/common.o: in function `qcom_cc_really_probe':
>> common.c:(.text+0x980): undefined reference to `devm_icc_clk_register'
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index 8ab08e7b5b6c..af73a0b396eb 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -243,6 +243,8 @@ config IPQ_GCC_8074 config IPQ_GCC_9574 tristate "IPQ9574 Global Clock Controller" + select INTERCONNECT + select INTERCONNECT_CLK help Support for global clock controller on ipq9574 devices. Say Y if you want to use peripheral devices such as UART, SPI, diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c index 0a3f846695b8..187fd9dcdf49 100644 --- a/drivers/clk/qcom/gcc-ipq9574.c +++ b/drivers/clk/qcom/gcc-ipq9574.c @@ -12,6 +12,7 @@ #include <dt-bindings/clock/qcom,ipq9574-gcc.h> #include <dt-bindings/reset/qcom,ipq9574-gcc.h> +#include <dt-bindings/interconnect/qcom,ipq9574.h> #include "clk-alpha-pll.h" #include "clk-branch.h" @@ -4301,6 +4302,56 @@ static const struct qcom_reset_map gcc_ipq9574_resets[] = { [GCC_WCSS_Q6_TBU_BCR] = { 0x12054, 0 }, }; +#define IPQ_APPS_ID 9574 /* some unique value */ + +enum { + ICC_ANOC_PCIE0, + ICC_SNOC_PCIE0, + ICC_ANOC_PCIE1, + ICC_SNOC_PCIE1, + ICC_ANOC_PCIE2, + ICC_SNOC_PCIE2, + ICC_ANOC_PCIE3, + ICC_SNOC_PCIE3, + ICC_SNOC_USB, + ICC_ANOC_USB_AXI, + ICC_NSSNOC_NSSCC, + ICC_NSSNOC_SNOC_0, + ICC_NSSNOC_SNOC_1, + ICC_NSSNOC_PCNOC_1, + ICC_NSSNOC_QOSGEN_REF, + ICC_NSSNOC_TIMEOUT_REF, + ICC_NSSNOC_XO_DCD, + ICC_NSSNOC_ATB, + ICC_MEM_NOC_NSSNOC, + ICC_NSSNOC_MEMNOC, + ICC_NSSNOC_MEM_NOC_1, +}; + +static struct clk_hw *icc_ipq9574_hws[] = { + [ICC_ANOC_PCIE0] = &gcc_anoc_pcie0_1lane_m_clk.clkr.hw, + [ICC_SNOC_PCIE0] = &gcc_anoc_pcie1_1lane_m_clk.clkr.hw, + [ICC_ANOC_PCIE1] = &gcc_anoc_pcie2_2lane_m_clk.clkr.hw, + [ICC_SNOC_PCIE1] = &gcc_anoc_pcie3_2lane_m_clk.clkr.hw, + [ICC_ANOC_PCIE2] = &gcc_snoc_pcie0_1lane_s_clk.clkr.hw, + [ICC_SNOC_PCIE2] = &gcc_snoc_pcie1_1lane_s_clk.clkr.hw, + [ICC_ANOC_PCIE3] = &gcc_snoc_pcie2_2lane_s_clk.clkr.hw, + [ICC_SNOC_PCIE3] = &gcc_snoc_pcie3_2lane_s_clk.clkr.hw, + [ICC_SNOC_USB] = &gcc_snoc_usb_clk.clkr.hw, + [ICC_ANOC_USB_AXI] = &gcc_anoc_usb_axi_clk.clkr.hw, + [ICC_NSSNOC_NSSCC] = &gcc_nssnoc_nsscc_clk.clkr.hw, + [ICC_NSSNOC_SNOC_0] = &gcc_nssnoc_snoc_clk.clkr.hw, + [ICC_NSSNOC_SNOC_1] = &gcc_nssnoc_snoc_1_clk.clkr.hw, + [ICC_NSSNOC_PCNOC_1] = &gcc_nssnoc_pcnoc_1_clk.clkr.hw, + [ICC_NSSNOC_QOSGEN_REF] = &gcc_nssnoc_qosgen_ref_clk.clkr.hw, + [ICC_NSSNOC_TIMEOUT_REF] = &gcc_nssnoc_timeout_ref_clk.clkr.hw, + [ICC_NSSNOC_XO_DCD] = &gcc_nssnoc_xo_dcd_clk.clkr.hw, + [ICC_NSSNOC_ATB] = &gcc_nssnoc_atb_clk.clkr.hw, + [ICC_MEM_NOC_NSSNOC] = &gcc_mem_noc_nssnoc_clk.clkr.hw, + [ICC_NSSNOC_MEMNOC] = &gcc_nssnoc_memnoc_clk.clkr.hw, + [ICC_NSSNOC_MEM_NOC_1] = &gcc_nssnoc_mem_noc_1_clk.clkr.hw, +}; + static const struct of_device_id gcc_ipq9574_match_table[] = { { .compatible = "qcom,ipq9574-gcc" }, { } @@ -4323,6 +4374,9 @@ static const struct qcom_cc_desc gcc_ipq9574_desc = { .num_resets = ARRAY_SIZE(gcc_ipq9574_resets), .clk_hws = gcc_ipq9574_hws, .num_clk_hws = ARRAY_SIZE(gcc_ipq9574_hws), + .icc_hws = icc_ipq9574_hws, + .num_icc_hws = ARRAY_SIZE(icc_ipq9574_hws), + .first_id = IPQ_APPS_ID, }; static int gcc_ipq9574_probe(struct platform_device *pdev)
Use the icc-clk framework to enable few clocks to be able to create paths and use the peripherals connected on those NoCs. Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> --- v5: Split from common.c changes into separate patch No functional changes --- drivers/clk/qcom/Kconfig | 2 ++ drivers/clk/qcom/gcc-ipq9574.c | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)