clk: qcom: Add support for SM8150 GCC

We want users to code properly and fix the post_div_table missing and not reply on core to check. So remove the post_div_table check.

Signed-off-by Vinod Koul <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

---

Quoting Vinod Koul (2019-06-24 23:31:38)

Quoted 6 lines

We want users to code properly and fix the post_div_table missing and not reply on core to check. So remove the post_div_table check.

Signed-off-by Vinod Koul <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

---

This doesn't apply. Not sure why. Can you please format-patch with

--base= so I can know what baseline commit you've based your patches on? Helps me avoid needing to ask, like right now.

We want users to code properly and fix the post_div_table missing and not reply on core to check. So remove the post_div_table check.

Signed-off-by Vinod Koul <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

---
 drivers/clk/qcom/clk-alpha-pll.c | 15 ---------------
 1 file changed, 15 deletions(-)

drivers/clk/qcom/clk-alpha-pll.c +0 -15

index b48707693ffd..2c6773188761 100644
@@ -1036,11 +1036,6 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,
 	u32 i, div = 1, val;
 	int ret;
 
-	if (!pll->post_div_table) {
-		pr_err("Missing the post_div_table for the PLL\n");
-		return -EINVAL;
-	}
-
 	ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val);
 	if (ret)
 		return ret;

@@ -1063,11 +1058,6 @@ static long clk_alpha_pll_postdiv_fabia_round_rate(struct clk_hw *hw,
 {
 	struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
 
-	if (!pll->post_div_table) {
-		pr_err("Missing the post_div_table for the PLL\n");
-		return -EINVAL;
-	}
-
 	return divider_round_rate(hw, rate, prate, pll->post_div_table,
 				pll->width, CLK_DIVIDER_ROUND_CLOSEST);
 }

@@ -1089,11 +1079,6 @@ static int clk_alpha_pll_postdiv_fabia_set_rate(struct clk_hw *hw,
 	if (val & PLL_VOTE_FSM_ENA)
 		return 0;
 
-	if (!pll->post_div_table) {
-		pr_err("Missing the post_div_table for the PLL\n");
-		return -EINVAL;
-	}
-
 	div = DIV_ROUND_UP_ULL(parent_rate, rate);
 	for (i = 0; i < pll->num_post_div; i++) {
 		if (pll->post_div_table[i].div == div) {

Signature · 1 line

2.20.1

Quoted 2 lines · Vinod Koul

We want users to code properly and fix the post_div_table missing and not reply on core to check. So remove the post_div_table check.

s/reply/rely/

drivers/clk/qcom/clk-alpha-pll.c +1 -0

index 2c6773188761..30210f5c6726 100644
@@ -32,6 +32,7 @@
 # define PLL_LOCK_DET          BIT(31)
 
 #define PLL_L_VAL(p)           ((p)->offset + (p)->regs[PLL_OFF_L_VAL])
+#define PLL_CAL_L_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_CAL_L_VAL])
 #define PLL_ALPHA_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL])
 #define PLL_ALPHA_VAL_U(p)     ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL_U])
 

drivers/clk/qcom/clk-alpha-pll.c +1 -0

 #define PLL_TEST_CTL(p)                ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
 #define PLL_TEST_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
 #define PLL_STATUS(p)          ((p)->offset + (p)->regs[PLL_OFF_STATUS])
 #define PLL_OPMODE(p)          ((p)->offset + (p)->regs[PLL_OFF_OPMODE])
 #define PLL_FRAC(p)            ((p)->offset + (p)->regs[PLL_OFF_FRAC])
+#define PLL_CAL_VAL(p)         ((p)->offset + (p)->regs[PLL_OFF_CAL_VAL])
 
 const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
        [CLK_ALPHA_PLL_TYPE_DEFAULT] =  {

const struct clk_ops clk_alpha_pll_ops = {
.enable = clk_alpha_pll_enable,
.disable = clk_alpha_pll_disable,

drivers/clk/qcom/clk-alpha-pll.c +0 -0

@@ -1053,6 +1210,77 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,

drivers/clk/qcom/clk-alpha-pll.c +20 -0

+        */
+       ret = regmap_read(regmap, PLL_MODE(pll), &val);
+       if (ret)
+               return ret;
+
+       if (val & PLL_VOTE_FSM_ENA)
+               return 0;
+
+       div = DIV_ROUND_UP_ULL(parent_rate, rate);
+       for (i = 0; i < pll->num_post_div; i++) {
+               if (pll->post_div_table[i].div == div) {
+                       val = pll->post_div_table[i].val;
+                       break;
+               }
+       }
+
+       return regmap_update_bits(regmap, PLL_USER_CTL(pll),
+                                 PLL_POST_DIV_MASK(pll) << PLL_POST_DIV_SHIFT,
+                                 val << PLL_POST_DIV_SHIFT);
+}

Quoting Vinod Koul (2019-06-24 23:31:39)

Quoted 18 lines

drivers/clk/qcom/clk-alpha-pll.c +2 -0

index 2c6773188761..30210f5c6726 100644
@@ -32,6 +32,7 @@
 # define PLL_LOCK_DET          BIT(31)
 
 #define PLL_L_VAL(p)           ((p)->offset + (p)->regs[PLL_OFF_L_VAL])
+#define PLL_CAL_L_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_CAL_L_VAL])
 #define PLL_ALPHA_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL])
 #define PLL_ALPHA_VAL_U(p)     ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL_U])
 

@@ -44,14 +45,17 @@
 # define PLL_VCO_MASK          0x3
 
 #define PLL_USER_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U])
+#define PLL_USER_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U1])
 

drivers/clk/qcom/clk-alpha-pll.c line 52

Quoted 3 lines

drivers/clk/qcom/clk-alpha-pll.c +1 -0

 #define PLL_CONFIG_CTL(p)      ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL])
 #define PLL_CONFIG_CTL_U(p)    ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U])
+#define PLL_CONFIG_CTL_U1(p)   ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U11])

This looks like a typo, U11 vs U1. So I don't think this has been compile tested....

Quoted 13 lines

drivers/clk/qcom/clk-alpha-pll.c +1 -0

 #define PLL_TEST_CTL(p)                ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
 #define PLL_TEST_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
 #define PLL_STATUS(p)          ((p)->offset + (p)->regs[PLL_OFF_STATUS])
 #define PLL_OPMODE(p)          ((p)->offset + (p)->regs[PLL_OFF_OPMODE])
 #define PLL_FRAC(p)            ((p)->offset + (p)->regs[PLL_OFF_FRAC])
+#define PLL_CAL_VAL(p)         ((p)->offset + (p)->regs[PLL_OFF_CAL_VAL])
 
 const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
        [CLK_ALPHA_PLL_TYPE_DEFAULT] =  {

const struct clk_ops clk_alpha_pll_ops = {
.enable = clk_alpha_pll_enable,
.disable = clk_alpha_pll_disable,

drivers/clk/qcom/clk-alpha-pll.c +0 -0

@@ -1053,6 +1210,77 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,

[...]

drivers/clk/qcom/clk-alpha-pll.c static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw, · line 1255

Quoted 12 lines

drivers/clk/qcom/clk-alpha-pll.c +12 -0

+
+static int
+clk_trion_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate,
+                              unsigned long parent_rate)
+{
+       struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
+       struct regmap *regmap = pll->clkr.regmap;
+       int i, val = 0, div, ret;
+
+       /*
+        * If the PLL is in FSM mode, then treat the set_rate callback
+        * as a no-operation.

And this is OK? Shouldn't we fail because we can't change to the rate that's desired?

From: Deepak Katragadda <[email protected]>

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 <[email protected]>

Signed-off-by Taniya Das <[email protected]>

[vkoul: port to upstream and tidy-up]

Signed-off-by Vinod Koul <[email protected]>

---
 .../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

Documentation/devicetree/bindings/clock/qcom,gcc.txt +0 -0

index 8661c3cd3ccf..1b39d949e63d 100644

drivers/clk/qcom/Kconfig +0 -0

index 18bdf34d5e64..076872d195fd 100644

drivers/clk/qcom/Kconfig +1 -0

+
 config SPMI_PMIC_CLKDIV
        tristate "SPMI PMIC clkdiv Support"
        depends on SPMI || COMPILE_TEST

drivers/clk/qcom/Makefile +1 -0

index f0768fb1f037..4a813b4055d0 100644
@@ -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

drivers/clk/qcom/gcc-sm8150.c new file +0 -0

new file mode 100644
index 000000000000..11cd9e19f18d

drivers/clk/qcom/gcc-sm8150.c new file +6 -0

+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+#include <dt-bindings/clock/qcom,gcc-sm8150.h>
+

drivers/clk/qcom/gcc-sm8150.c new file +87 -0

+
+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 },

drivers/clk/qcom/gcc-sm8150.c new file +27 -0

+};
+
+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 },
+};

drivers/clk/qcom/gcc-sm8150.c new file +33 -0

+       { 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,

drivers/clk/qcom/gcc-sm8150.c new file +6 -0

+               },
+               .num_parents = 1,
+               .ops = &clk_trion_pll_postdiv_ops,
+       },
+};
+

drivers/clk/qcom/gcc-sm8150.c new file +37 -0

+                       .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),

drivers/clk/qcom/gcc-sm8150.c new file +11 -0

+       .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,
+               },
+       },
+};
+

Quoting Vinod Koul (2019-06-24 23:31:40)

Quoted 9 lines

From: Deepak Katragadda <[email protected]>

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 <[email protected]>

Signed-off-by Taniya Das <[email protected]>

[vkoul: port to upstream and tidy-up]

This can say "changed to parent_data scheme"?

Quoted 16 lines

Signed-off-by Vinod Koul <[email protected]>

---
 .../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

Documentation/devicetree/bindings/clock/qcom,gcc.txt +0 -0

index 8661c3cd3ccf..1b39d949e63d 100644

Documentation/devicetree/bindings/clock/qcom,gcc.txt Required properties : · line 29

Quoted 7 lines

Documentation/devicetree/bindings/clock/qcom,gcc.txt +1 -0

@@ -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'.

Quoted 5 lines

drivers/clk/qcom/Kconfig +0 -0

index 18bdf34d5e64..076872d195fd 100644

drivers/clk/qcom/Kconfig config SDM_LPASSCC_845 · line 299

Quoted 9 lines

drivers/clk/qcom/Kconfig +6 -0

@@ -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?

Quoted 19 lines

drivers/clk/qcom/Kconfig +1 -0

+
 config SPMI_PMIC_CLKDIV
        tristate "SPMI PMIC clkdiv Support"
        depends on SPMI || COMPILE_TEST

drivers/clk/qcom/Makefile +1 -0

index f0768fb1f037..4a813b4055d0 100644
@@ -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

drivers/clk/qcom/gcc-sm8150.c new file +0 -0

new file mode 100644
index 000000000000..11cd9e19f18d

drivers/clk/qcom/gcc-sm8150.c line 3

Quoted 6 lines

drivers/clk/qcom/gcc-sm8150.c new file +3 -0

@@ -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?

drivers/clk/qcom/gcc-sm8150.c line 13

Quoted 11 lines

drivers/clk/qcom/gcc-sm8150.c new file +10 -0

+ */
+
+#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?

Quoted 11 lines

drivers/clk/qcom/gcc-sm8150.c new file +10 -0

+#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"

drivers/clk/qcom/gcc-sm8150.c line 29

Quoted 6 lines

drivers/clk/qcom/gcc-sm8150.c new file +6 -0

+#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.

drivers/clk/qcom/gcc-sm8150.c line 34

Quoted 5 lines

drivers/clk/qcom/gcc-sm8150.c new file +5 -0

+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.

drivers/clk/qcom/gcc-sm8150.c line 35

Quoted 1 line

drivers/clk/qcom/gcc-sm8150.c new file +1 -0

+       { .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.

drivers/clk/qcom/gcc-sm8150.c line 44

Quoted 9 lines

drivers/clk/qcom/gcc-sm8150.c new file +9 -0

+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?

drivers/clk/qcom/gcc-sm8150.c line 56

Quoted 12 lines

drivers/clk/qcom/gcc-sm8150.c new file +12 -0

+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.

drivers/clk/qcom/gcc-sm8150.c line 67

Quoted 11 lines

drivers/clk/qcom/gcc-sm8150.c new file +11 -0

+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?

Quoted 10 lines

drivers/clk/qcom/gcc-sm8150.c new file +10 -0

+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,
+               },

drivers/clk/qcom/gcc-sm8150.c line 83

Quoted 6 lines

drivers/clk/qcom/gcc-sm8150.c new file +6 -0

+       },
+};
+
+static struct clk_branch gcc_video_xo_clk = {
+       .halt_reg = 0xb040,
+       .halt_check = BRANCH_HALT_DELAY,

Why doesn't the halt bit work?

Quoted 9 lines

Quoting Vinod Koul (2019-06-24 23:31:40)

Quoted 8 lines

drivers/clk/qcom/Kconfig +0 -0

index 18bdf34d5e64..076872d195fd 100644
@@ -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.
 

Quoted 9 lines

drivers/clk/qcom/Kconfig config SDM_LPASSCC_845 · line 299

Quoted 6 lines

drivers/clk/qcom/Kconfig +6 -0

+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?

Quoted 17 lines

Quoted 17 lines

drivers/clk/qcom/Makefile +1 -0

index f0768fb1f037..4a813b4055d0 100644
@@ -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

drivers/clk/qcom/gcc-sm8150.c new file +0 -0

new file mode 100644
index 000000000000..11cd9e19f18d

Quoted 8 lines

drivers/clk/qcom/gcc-sm8150.c

Quoted 5 lines

drivers/clk/qcom/gcc-sm8150.c new file +5 -0

+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

Quoting Vinod Koul (2019-06-29 06:51:19)

Quoted 10 lines · Stephen Boyd

Quoted 9 lines

Quoting Vinod Koul (2019-06-24 23:31:40)

Quoted 8 lines

drivers/clk/qcom/Kconfig +0 -0

index 18bdf34d5e64..076872d195fd 100644
@@ -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.
 

Quoted 8 lines

Quoted 8 lines

drivers/clk/qcom/Kconfig config SDM_LPASSCC_845 · line 299

Quoted 6 lines

drivers/clk/qcom/Kconfig +6 -0

+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?

Quoted 27 lines

Quoted 18 lines

Quoted 18 lines

drivers/clk/qcom/Makefile +1 -0

index f0768fb1f037..4a813b4055d0 100644
@@ -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

drivers/clk/qcom/gcc-sm8150.c new file +0 -0

new file mode 100644
index 000000000000..11cd9e19f18d

Quoted 8 lines

drivers/clk/qcom/gcc-sm8150.c

Quoted 5 lines

drivers/clk/qcom/gcc-sm8150.c new file +5 -0

+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.

Quoted 4 lines

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.