Quoting Manivannan Sadhasivam (2020-10-28 00:42:30) > From: Naveen Yadav <navee...@codeaurora.org> > > Add Global Clock Controller (GCC) support for SDX55 SoCs from Qualcomm. > > Signed-off-by: Naveen Yadav <navee...@codeaurora.org> > [mani: converted to parent_data, commented critical clocks, cleanups] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org> > --- > drivers/clk/qcom/Kconfig | 8 + > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/gcc-sdx55.c | 1667 ++++++++++++++++++++++++++++++++++ > 3 files changed, 1676 insertions(+) > create mode 100644 drivers/clk/qcom/gcc-sdx55.c > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > index 3a965bd326d5..dbca8debc06f 100644 > --- a/drivers/clk/qcom/Kconfig > +++ b/drivers/clk/qcom/Kconfig > @@ -502,4 +502,12 @@ config KRAITCC > Support for the Krait CPU clocks on Qualcomm devices. > Say Y if you want to support CPU frequency scaling. > > +config GCC_SDX55
Please sort instead of add at end. > + tristate "SDX55 Global Clock Controller" > + depends on ARM Why? > + help > + Support for the global clock controller on SDX55 devices. > + Say Y if you want to use peripheral devices such as UART, > + SPI, I2C, USB, SD/UFS, PCIe etc. > + > endif > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile > index 11ae86febe87..3e27d67f95aa 100644 > --- a/drivers/clk/qcom/Makefile > +++ b/drivers/clk/qcom/Makefile > @@ -75,3 +75,4 @@ obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o > obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o > obj-$(CONFIG_QCOM_HFPLL) += hfpll.o > obj-$(CONFIG_KRAITCC) += krait-cc.o > +obj-$(CONFIG_GCC_SDX55) += gcc-sdx55.o Please sort this instead of add at end. > diff --git a/drivers/clk/qcom/gcc-sdx55.c b/drivers/clk/qcom/gcc-sdx55.c > new file mode 100644 > index 000000000000..75831c829202 > --- /dev/null > +++ b/drivers/clk/qcom/gcc-sdx55.c > @@ -0,0 +1,1667 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2018-2019, The Linux Foundation. All rights reserved. > + * Copyright (c) 2020, Linaro Ltd. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <dt-bindings/clock/qcom,gcc-sdx55.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" > + > +enum { > + P_BI_TCXO, > + P_CORE_BI_PLL_TEST_SE, > + P_GPLL0_OUT_EVEN, > + P_GPLL0_OUT_MAIN, > + P_GPLL4_OUT_EVEN, > + P_GPLL5_OUT_MAIN, > + P_SLEEP_CLK, > +}; > + > +static struct pll_vco lucid_vco[] = { const > + { 249600000, 2000000000, 0 }, > +}; > + > +static struct clk_alpha_pll gpll0 = { > + .offset = 0x0, > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID], > + .vco_table = lucid_vco, > + .num_vco = ARRAY_SIZE(lucid_vco), > + .clkr = { > + .enable_reg = 0x6d000, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gpll0", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "bi_tcxo", > + }, > + .num_parents = 1, > + .ops = &clk_alpha_pll_fixed_lucid_ops, > + }, > + }, > +}; > + > +static const struct clk_div_table post_div_table_lucid_even[] = { > + { 0x0, 1 }, > + { 0x1, 2 }, > + { 0x3, 4 }, > + { 0x7, 8 }, > + { } > +}; I think this table is common to all lucid plls? Maybe we can push it into the clk_ops somehow and stop duplicating it here? > + > +static struct clk_alpha_pll_postdiv gpll0_out_even = { > + .offset = 0x0, > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID], > + .post_div_shift = 8, > + .post_div_table = post_div_table_lucid_even, > + .num_post_div = ARRAY_SIZE(post_div_table_lucid_even), > + .width = 4, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "gpll0_out_even", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "gpll0", > + }, If this is gpll0 in this file, then this should be a clk_hws pointer instead and directly pointing to the parent. > + .num_parents = 1, > + .ops = &clk_alpha_pll_postdiv_lucid_ops, > + }, > +}; > + > +static struct clk_alpha_pll gpll4 = { > + .offset = 0x76000, > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID], > + .vco_table = lucid_vco, > + .num_vco = ARRAY_SIZE(lucid_vco), > + .clkr = { > + .enable_reg = 0x6d000, > + .enable_mask = BIT(4), > + .hw.init = &(struct clk_init_data){ > + .name = "gpll4", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "bi_tcxo", > + }, > + .num_parents = 1, > + .ops = &clk_alpha_pll_fixed_lucid_ops, > + }, > + }, > +}; > + > +static struct clk_alpha_pll_postdiv gpll4_out_even = { > + .offset = 0x76000, > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID], > + .post_div_shift = 8, > + .post_div_table = post_div_table_lucid_even, > + .num_post_div = ARRAY_SIZE(post_div_table_lucid_even), > + .width = 4, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "gpll4_out_even", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "gpll4", If this is gpll4 in this file, then this should be a clk_hws pointer instead and directly pointing to the parent. > + }, > + .num_parents = 1, > + .ops = &clk_alpha_pll_postdiv_lucid_ops, > + }, > +}; > + > +static struct clk_alpha_pll gpll5 = { > + .offset = 0x74000, > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID], > + .vco_table = lucid_vco, > + .num_vco = ARRAY_SIZE(lucid_vco), [...] > + .name = "gcc_sdcc1_ahb_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_sdcc1_apps_clk = { > + .halt_reg = 0xf004, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0xf004, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_sdcc1_apps_clk", > + .parent_hws = (const struct clk_hw *[]){ > + &gcc_sdcc1_apps_clk_src.clkr.hw }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +/* For CPUSS functionality the SYS NOC clock needs to be left enabled */ > +static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = { > + .halt_reg = 0x4010, > + .halt_check = BRANCH_HALT_VOTED, > + .clkr = { > + .enable_reg = 0x6d008, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_sys_noc_cpuss_ahb_clk", > + .parent_hws = (const struct clk_hw *[]){ > + &gcc_cpuss_ahb_clk_src.clkr.hw }, > + .num_parents = 1, > + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, These CLK_IS_CRITICAL clks can't be set once at driver probe time and forgotten about? It would be nice to not allocate memory for things that never matter. > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_usb30_master_clk = { > + .halt_reg = 0xb010, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0xb010, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_usb30_master_clk", > + .parent_hws = (const struct clk_hw *[]){ > + &gcc_usb30_master_clk_src.clkr.hw }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, [...] > + > +static const struct qcom_cc_desc gcc_sdx55_desc = { > + .config = &gcc_sdx55_regmap_config, > + .clks = gcc_sdx55_clocks, > + .num_clks = ARRAY_SIZE(gcc_sdx55_clocks), > + .resets = gcc_sdx55_resets, > + .num_resets = ARRAY_SIZE(gcc_sdx55_resets), No gdscs? > +}; > + > +static const struct of_device_id gcc_sdx55_match_table[] = { > + { .compatible = "qcom,gcc-sdx55" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, gcc_sdx55_match_table); > + > +static int gcc_sdx55_probe(struct platform_device *pdev) > +{ > + return qcom_cc_probe(pdev, &gcc_sdx55_desc); Wow haven't seen this in some time. There isn't some sort of PLL that needs configuring or some clks that need to be slammed on with a couple register writes? > +} > +