Quoting Vinod Koul (2019-06-24 23:31:40) > From: Deepak Katragadda <dkatraga@xxxxxxxxxxxxxx> > > Add the clocks supported in global clock controller which clock the > peripherals like BLSPs, SDCC, USB, MDSS etc. Register all the clocks > to the clock framework for the clients to be able to request for them. > > Signed-off-by: Deepak Katragadda <dkatraga@xxxxxxxxxxxxxx> > Signed-off-by: Taniya Das <tdas@xxxxxxxxxxxxxx> > [vkoul: port to upstream and tidy-up] This can say "changed to parent_data scheme"? > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx> > --- > .../devicetree/bindings/clock/qcom,gcc.txt | 1 + > drivers/clk/qcom/Kconfig | 7 + > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/gcc-sm8150.c | 3719 +++++++++++++++++ > include/dt-bindings/clock/qcom,gcc-sm8150.h | 243 ++ > 5 files changed, 3971 insertions(+) > create mode 100644 drivers/clk/qcom/gcc-sm8150.c > create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h > > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt > index 8661c3cd3ccf..1b39d949e63d 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt > @@ -23,6 +23,7 @@ Required properties : > "qcom,gcc-sdm630" > "qcom,gcc-sdm660" > "qcom,gcc-sdm845" > + "qcom,gcc-sm8150" > > - reg : shall contain base register location and length > - #clock-cells : shall contain 1 I'd expect the binding to get a larger update indicating which clks come from outside the clk controller and need to be specified as 'clocks' and 'clock-names'. > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > index 18bdf34d5e64..076872d195fd 100644 > --- a/drivers/clk/qcom/Kconfig > +++ b/drivers/clk/qcom/Kconfig > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845 > Say Y if you want to use the LPASS branch clocks of the LPASS clock > controller to reset the LPASS subsystem. > > +config SM_GCC_8150 > + tristate "SM8150 Global Clock Controller" > + help > + Support for the global clock controller on SM8150 devices. > + Say Y if you want to use peripheral devices such as UART, > + SPI, I2C, USB, SD/eMMC, PCIe etc. Is there eMMC support? > + > config SPMI_PMIC_CLKDIV > tristate "SPMI PMIC clkdiv Support" > depends on SPMI || COMPILE_TEST > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile > index f0768fb1f037..4a813b4055d0 100644 > --- a/drivers/clk/qcom/Makefile > +++ b/drivers/clk/qcom/Makefile > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o > obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o > obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o > obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o > obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o > obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o > obj-$(CONFIG_QCOM_HFPLL) += hfpll.o > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c > new file mode 100644 > index 000000000000..11cd9e19f18d > --- /dev/null > +++ b/drivers/clk/qcom/gcc-sm8150.c > @@ -0,0 +1,3719 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2017-19, The Linux Foundation. All rights reserved. Is it really 2017-19 and not 2017-2019? > + */ > + > +#include <linux/kernel.h> > +#include <linux/bitops.h> > +#include <linux/err.h> > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/clk.h> Is this include used? > +#include <linux/clk-provider.h> > +#include <linux/regmap.h> > +#include <linux/reset-controller.h> > + > +#include <dt-bindings/clock/qcom,gcc-sm8150.h> > + > +#include "common.h" > +#include "clk-alpha-pll.h" > +#include "clk-branch.h" > +#include "clk-pll.h" > +#include "clk-rcg.h" > +#include "clk-regmap.h" > +#include "reset.h" > + > +#define GCC_NPU_MISC 0x4d110 > +#define GCC_GPU_MISC 0x71028 Drop these defines please and inline the values. > + > +enum { > + P_BI_TCXO, > + P_AUD_REF_CLK, > + P_CORE_BI_PLL_TEST_SE, > + P_GPLL0_OUT_EVEN, > + P_GPLL0_OUT_MAIN, > + P_GPLL4_OUT_MAIN, > + P_GPLL7_OUT_MAIN, > + P_GPLL9_OUT_MAIN, > + P_SLEEP_CLK, > +}; > + > +static const struct parent_map gcc_parent_map_0[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_GPLL0_OUT_EVEN, 6 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_0[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "gpll0", .name = "gpll0" }, > + { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static const struct clk_parent_data gcc_parents_0_ao[] = { > + { .fw_name = "bi_tcxo_ao", .name = "bi_tcxo_ao" }, > + { .fw_name = "gpll0", .name = "gpll0" }, > + { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static const struct parent_map gcc_parent_map_1[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_SLEEP_CLK, 5 }, > + { P_GPLL0_OUT_EVEN, 6 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_1[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "gpll0", .name = "gpll0" }, > + { .fw_name = "sleep_clk", .name = "sleep_clk" }, > + { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static const struct parent_map gcc_parent_map_2[] = { > + { P_BI_TCXO, 0 }, > + { P_SLEEP_CLK, 5 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_2[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "sleep_clk", .name = "sleep_clk" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static const struct parent_map gcc_parent_map_3[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_3[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "gpll0", .name = "gpll0" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static const struct parent_map gcc_parent_map_4[] = { > + { P_BI_TCXO, 0 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_4[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static const struct parent_map gcc_parent_map_5[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_GPLL7_OUT_MAIN, 3 }, > + { P_GPLL0_OUT_EVEN, 6 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_5[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "gpll0", .name = "gpll0" }, > + { .fw_name = "gpll7", .name = "gpll7" }, > + { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" }, Aren't these gplls all created in this file? They shouldn't be listed in DT so I'm confused why we have .fw_name for them. > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, If there isn't a legacy string name for core_bi_pll_test_se we should just leave it out and not have a .name field, just the .fw_name one. > +}; > + > +static const struct parent_map gcc_parent_map_6[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_GPLL9_OUT_MAIN, 2 }, > + { P_GPLL4_OUT_MAIN, 5 }, > + { P_GPLL0_OUT_EVEN, 6 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_6[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "gpll0", .name = "gpll0" }, > + { .fw_name = "gpll9", .name = "gpll9" }, > + { .fw_name = "gpll4", .name = "gpll4" }, > + { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static const struct parent_map gcc_parent_map_7[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_AUD_REF_CLK, 2 }, > + { P_GPLL0_OUT_EVEN, 6 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gcc_parents_7[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .fw_name = "gpll0", .name = "gpll0" }, > + { .fw_name = "aud_ref_clk", .name = "aud_ref_clk" }, > + { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; > + > +static struct pll_vco trion_vco[] = { const? > + { 249600000, 2000000000, 0 }, > +}; > + > +static struct clk_alpha_pll gpll0 = { > + .offset = 0x0, > + .vco_table = trion_vco, > + .num_vco = ARRAY_SIZE(trion_vco), > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION], > + .clkr = { > + .enable_reg = 0x52000, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gpll0", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "bi_tcxo", > + .name = "bi_tcxo", > + }, > + .num_parents = 1, > + .ops = &clk_trion_fixed_pll_ops, > + }, > + }, > +}; > + > +static const struct clk_div_table post_div_table_trion_even[] = { > + { 0x0, 1 }, > + { 0x1, 2 }, > + { 0x3, 4 }, > + { 0x7, 8 }, > + { } > +}; > + > +static struct clk_alpha_pll_postdiv gpll0_out_even = { > + .offset = 0x0, > + .post_div_shift = 8, > + .post_div_table = post_div_table_trion_even, > + .num_post_div = ARRAY_SIZE(post_div_table_trion_even), > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION], > + .width = 4, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "gpll0_out_even", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "gpll0", > + .name = "gpll0", Use a direct pointer? This applies in many places in the code. > + }, > + .num_parents = 1, > + .ops = &clk_trion_pll_postdiv_ops, > + }, > +}; > + [...] > + > +static struct clk_branch gcc_video_ahb_clk = { > + .halt_reg = 0xb004, > + .halt_check = BRANCH_HALT_DELAY, > + .hwcg_reg = 0xb004, > + .hwcg_bit = 1, > + .clkr = { > + .enable_reg = 0xb004, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_video_ahb_clk", > + .flags = CLK_IS_CRITICAL, Can you please add a comment around all CLK_IS_CRITICAL flags indicating why these clks can't be turned off? > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_video_axi0_clk = { > + .halt_reg = 0xb024, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0xb024, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_video_axi0_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_video_axi1_clk = { > + .halt_reg = 0xb028, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0xb028, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_video_axi1_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_video_axic_clk = { > + .halt_reg = 0xb02c, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0xb02c, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_video_axic_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_video_xo_clk = { > + .halt_reg = 0xb040, > + .halt_check = BRANCH_HALT_DELAY, Why doesn't the halt bit work? > + .clkr = { > + .enable_reg = 0xb040, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_video_xo_clk", > + .flags = CLK_IS_CRITICAL, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > +