| Message ID | 20190625063140.17106-1-vkoul@kernel.org |
|---|---|
| Headers | show |
| Series | clk: qcom: Add support for SM8150 GCC | expand |
Quoting Vinod Koul (2019-06-24 23:31:40) > From: Deepak Katragadda <dkatraga@codeaurora.org> > > 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@codeaurora.org> > Signed-off-by: Taniya Das <tdas@codeaurora.org> > [vkoul: port to upstream and tidy-up] This can say "changed to parent_data scheme"? > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > .../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, > + }, > + }, > +}; > +
On 27-06-19, 14:31, Stephen Boyd wrote: > Quoting Vinod Koul (2019-06-24 23:31:40) > > From: Deepak Katragadda <dkatraga@codeaurora.org> > > > > 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@codeaurora.org> > > Signed-off-by: Taniya Das <tdas@codeaurora.org> > > [vkoul: port to upstream and tidy-up] > > This can say "changed to parent_data scheme"? > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > --- > > .../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? will update > > > + */ > > + > > +#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? will check and update > > > +#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. Sure > > +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. Yes they are and the DT doesnt provide these clock. From what I understood from the name conversion to new schema was we should add it like above, let me know if I missed something > > + { .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. Okay > > +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? will add > > +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. okay will check here and other places > > +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? okay will do > > +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? Not sure, will check and update -- ~Vinod
Quoting Vinod Koul (2019-06-29 06:51:19) > On 27-06-19, 14:31, Stephen Boyd wrote: > > Quoting Vinod Koul (2019-06-24 23:31:40) > > > 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? > > I guess no? > > > 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 > > > +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. > > Yes they are and the DT doesnt provide these clock. From what I > understood from the name conversion to new schema was we should add it > like above, let me know if I missed something > Yes, you should use the direct clk_hw pointer part of the clk_parent_data structure instead of having a .fw_name or .name member in these rows. Any clk inside of the clk controller shouldn't be exposed into DT just so that we can find it again through string comparisons.
On 15-07-19, 16:01, Stephen Boyd wrote: > Quoting Vinod Koul (2019-06-29 06:51:19) > > On 27-06-19, 14:31, Stephen Boyd wrote: > > > Quoting Vinod Koul (2019-06-24 23:31:40) > > > > 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? > > I guess no? Nope, copy paste error, have fixed it up :) > > > > > 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 > > > > +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. > > > > Yes they are and the DT doesnt provide these clock. From what I > > understood from the name conversion to new schema was we should add it > > like above, let me know if I missed something > > > > Yes, you should use the direct clk_hw pointer part of the > clk_parent_data structure instead of having a .fw_name or .name member > in these rows. Any clk inside of the clk controller shouldn't be exposed > into DT just so that we can find it again through string comparisons. Thanks, I think I have fixed it up. The parent clk in DT would be cxo and sleep_clk and creating bi_tcxo, gpll0, gpll7 etc in driver I have updated the patchset and will post shortly Thanks -- ~Vinod