[PATCH 0/9] MSM8998 clk cleanups and fixups

[PATCH 0/9] MSM8998 clk cleanups and fixups

[Konrad Dybcio]

The MSM8998 clock controller drivers have some rough edges around whether
and how Linux should touch them, which this series tries to sand down
a bit.

MSM8998 maple seems not to explode, please give it a spin on your boards.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (9):
      dt-bindings: clk: qcom,gcc-msm8998: Add missing GPU/MMSS GPLL0 legs
      dt-bindings: clock: qcom,mmcc: Add GPLL0_DIV for MSM8998
      clk: qcom: gcc-msm8998: Control MMSS and GPUSS GPLL0 outputs properly
      clk: qcom: mmcc-msm8998: Properly consume GPLL0 inputs
      clk: qcom: gpucc-msm8998: Use the correct GPLL0 leg with old DTs
      clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks
      clk: qcom: gcc-msm8998: Don't poke at some BIMC GPU clocks
      arm64: dts: qcom: msm8998: Use the correct GPLL0 leg for GPUCC
      arm64: dts: qcom: msm8998: Use the correct GPLL0_DIV leg for MMCC

 .../devicetree/bindings/clock/qcom,mmcc.yaml       |  2 +
 arch/arm64/boot/dts/qcom/msm8998.dtsi              |  8 +-
 drivers/clk/qcom/gcc-msm8998.c                     | 92 ++++++++++++++--------
 drivers/clk/qcom/gpucc-msm8998.c                   |  2 +-
 drivers/clk/qcom/mmcc-msm8998.c                    | 35 ++------
 include/dt-bindings/clock/qcom,gcc-msm8998.h       |  3 +
 6 files changed, 80 insertions(+), 62 deletions(-)
---
base-commit: c87d46a9e8ebd2f2c3960927b1d21687096d1109
change-id: 20230622-topic-8998clk-4317986f3008

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/9] dt-bindings: clk: qcom,gcc-msm8998: Add missing GPU/MMSS GPLL0 legs

[Konrad Dybcio]

GPLL0 has two separate outputs to both GPUSS and MMSS: one that's
2-divided and one that runs at the same rate as the GPLL0 itself.

Add the missing ones to the binding.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 include/dt-bindings/clock/qcom,gcc-msm8998.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/clock/qcom,gcc-msm8998.h b/include/dt-bindings/clock/qcom,gcc-msm8998.h
index 1badb4f9c58f..b5456a64d421 100644
--- a/include/dt-bindings/clock/qcom,gcc-msm8998.h
+++ b/include/dt-bindings/clock/qcom,gcc-msm8998.h
@@ -190,6 +190,9 @@
 #define AGGRE2_SNOC_NORTH_AXI					181
 #define SSC_XO							182
 #define SSC_CNOC_AHBS_CLK					183
+#define GCC_MMSS_GPLL0_DIV_CLK					184
+#define GCC_GPU_GPLL0_DIV_CLK					185
+#define GCC_GPU_GPLL0_CLK					186
 
 #define PCIE_0_GDSC						0
 #define UFS_GDSC						1

-- 
2.41.0

[PATCH 2/9] dt-bindings: clock: qcom,mmcc: Add GPLL0_DIV for MSM8998

[Konrad Dybcio]

We've not been consuming that clock for no apparent reason. Describe it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 Documentation/devicetree/bindings/clock/qcom,mmcc.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
index 422f5776a771..67e1eae0bbd0 100644
--- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
@@ -297,6 +297,7 @@ allOf:
             - description: HDMI phy PLL clock
             - description: DisplayPort phy PLL link clock
             - description: DisplayPort phy PLL vco clock
+            - description: Global PLL 0 DIV clock
 
         clock-names:
           items:
@@ -309,6 +310,7 @@ allOf:
             - const: hdmipll
             - const: dplink
             - const: dpvco
+            - const: gpll0_div
 
   - if:
       properties:

-- 
2.41.0

[PATCH 3/9] clk: qcom: gcc-msm8998: Control MMSS and GPUSS GPLL0 outputs properly

[Konrad Dybcio]

Up until now, we've been relying on some non-descript hardware magic
to pinkypromise turn the clocks on for us. While new SoCs shine with
that feature, MSM8998 can not always be fully trusted.

Register the MMSS and GPUSS GPLL0 legs with the CCF to allow for manual
enable voting.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-msm8998.c | 58 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
index be024f8093c5..cccb19cae481 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -25,6 +25,9 @@
 #include "reset.h"
 #include "gdsc.h"
 
+#define GCC_MMSS_MISC	0x0902C
+#define GCC_GPU_MISC	0x71028
+
 static struct pll_vco fabia_vco[] = {
 	{ 250000000, 2000000000, 0 },
 	{ 125000000, 1000000000, 1 },
@@ -1367,6 +1370,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk = {
 	},
 };
 
+static struct clk_branch gcc_mmss_gpll0_div_clk = {
+	.halt_check = BRANCH_HALT_DELAY,
+	.clkr = {
+		.enable_reg = 0x5200c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_mmss_gpll0_div_clk",
+			.parent_hws = (const struct clk_hw *[]) {
+				&gpll0_out_main.clkr.hw,
+			},
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_mmss_gpll0_clk = {
 	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
@@ -1395,6 +1414,38 @@ static struct clk_branch gcc_mss_gpll0_div_clk_src = {
 	},
 };
 
+static struct clk_branch gcc_gpu_gpll0_div_clk = {
+	.halt_check = BRANCH_HALT_DELAY,
+	.clkr = {
+		.enable_reg = 0x5200c,
+		.enable_mask = BIT(3),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_gpu_gpll0_div_clk",
+			.parent_hws = (const struct clk_hw *[]) {
+				&gpll0_out_main.clkr.hw,
+			},
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gcc_gpu_gpll0_clk = {
+	.halt_check = BRANCH_HALT_DELAY,
+	.clkr = {
+		.enable_reg = 0x5200c,
+		.enable_mask = BIT(4),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_gpu_gpll0_clk",
+			.parent_hws = (const struct clk_hw *[]) {
+				&gpll0_out_main.clkr.hw,
+			},
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_blsp1_ahb_clk = {
 	.halt_reg = 0x17004,
 	.halt_check = BRANCH_HALT_VOTED,
@@ -3080,6 +3131,9 @@ static struct clk_regmap *gcc_msm8998_clocks[] = {
 	[AGGRE2_SNOC_NORTH_AXI] = &aggre2_snoc_north_axi_clk.clkr,
 	[SSC_XO] = &ssc_xo_clk.clkr,
 	[SSC_CNOC_AHBS_CLK] = &ssc_cnoc_ahbs_clk.clkr,
+	[GCC_MMSS_GPLL0_DIV_CLK] = &gcc_mmss_gpll0_div_clk.clkr,
+	[GCC_GPU_GPLL0_DIV_CLK] = &gcc_gpu_gpll0_div_clk.clkr,
+	[GCC_GPU_GPLL0_CLK] = &gcc_gpu_gpll0_clk.clkr,
 };
 
 static struct gdsc *gcc_msm8998_gdscs[] = {
@@ -3235,6 +3289,10 @@ static int gcc_msm8998_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/* Disable the GPLL0 active input to MMSS and GPU via MISC registers */
+	regmap_write(regmap, GCC_MMSS_MISC, 0x10003);
+	regmap_write(regmap, GCC_GPU_MISC, 0x10003);
+
 	return qcom_cc_really_probe(pdev, &gcc_msm8998_desc, regmap);
 }
 

-- 
2.41.0

[PATCH 4/9] clk: qcom: mmcc-msm8998: Properly consume GPLL0 inputs

[Konrad Dybcio]

Up until now, the GPLL0_DIV MMSS input has been modeled as a fixed
child of MMSS_GPLL0_DIV that's always-on. Properly representing the
former in the GCC driver makes us unable to keep doing so.

Consume MSS_GPLL0_DIV through fw_name ("gpll0_div") as well as add a
fixed .name link to keep backwards compatibility.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/mmcc-msm8998.c | 35 ++++++++---------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
index 4490594bde69..040c495e91e4 100644
--- a/drivers/clk/qcom/mmcc-msm8998.c
+++ b/drivers/clk/qcom/mmcc-msm8998.c
@@ -46,19 +46,6 @@ enum {
 	P_DPLINK,
 };
 
-static struct clk_fixed_factor gpll0_div = {
-	.mult = 1,
-	.div = 2,
-	.hw.init = &(struct clk_init_data){
-		.name = "mmss_gpll0_div",
-		.parent_data = &(const struct clk_parent_data){
-			.fw_name = "gpll0"
-		},
-		.num_parents = 1,
-		.ops = &clk_fixed_factor_ops,
-	},
-};
-
 static const struct clk_div_table post_div_table_fabia_even[] = {
 	{ 0x0, 1 },
 	{ 0x1, 2 },
@@ -354,7 +341,7 @@ static const struct parent_map mmss_xo_gpll0_gpll0_div_map[] = {
 static const struct clk_parent_data mmss_xo_gpll0_gpll0_div[] = {
 	{ .fw_name = "xo" },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static const struct parent_map mmss_xo_mmpll0_gpll0_gpll0_div_map[] = {
@@ -368,7 +355,7 @@ static const struct clk_parent_data mmss_xo_mmpll0_gpll0_gpll0_div[] = {
 	{ .fw_name = "xo" },
 	{ .hw = &mmpll0_out_even.clkr.hw },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static const struct parent_map mmss_xo_mmpll0_mmpll1_gpll0_gpll0_div_map[] = {
@@ -384,7 +371,7 @@ static const struct clk_parent_data mmss_xo_mmpll0_mmpll1_gpll0_gpll0_div[] = {
 	{ .hw = &mmpll0_out_even.clkr.hw },
 	{ .hw = &mmpll1_out_even.clkr.hw },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static const struct parent_map mmss_xo_mmpll0_mmpll5_gpll0_gpll0_div_map[] = {
@@ -400,7 +387,7 @@ static const struct clk_parent_data mmss_xo_mmpll0_mmpll5_gpll0_gpll0_div[] = {
 	{ .hw = &mmpll0_out_even.clkr.hw },
 	{ .hw = &mmpll5_out_even.clkr.hw },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static const struct parent_map mmss_xo_mmpll0_mmpll3_mmpll6_gpll0_gpll0_div_map[] = {
@@ -418,7 +405,7 @@ static const struct clk_parent_data mmss_xo_mmpll0_mmpll3_mmpll6_gpll0_gpll0_div
 	{ .hw = &mmpll3_out_even.clkr.hw },
 	{ .hw = &mmpll6_out_even.clkr.hw },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static const struct parent_map mmss_xo_mmpll4_mmpll7_mmpll10_gpll0_gpll0_div_map[] = {
@@ -436,7 +423,7 @@ static const struct clk_parent_data mmss_xo_mmpll4_mmpll7_mmpll10_gpll0_gpll0_di
 	{ .hw = &mmpll7_out_even.clkr.hw },
 	{ .hw = &mmpll10_out_even.clkr.hw },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static const struct parent_map mmss_xo_mmpll0_mmpll7_mmpll10_gpll0_gpll0_div_map[] = {
@@ -454,7 +441,7 @@ static const struct clk_parent_data mmss_xo_mmpll0_mmpll7_mmpll10_gpll0_gpll0_di
 	{ .hw = &mmpll7_out_even.clkr.hw },
 	{ .hw = &mmpll10_out_even.clkr.hw },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static const struct parent_map mmss_xo_mmpll0_mmpll4_mmpll7_mmpll10_gpll0_gpll0_div_map[] = {
@@ -474,7 +461,7 @@ static const struct clk_parent_data mmss_xo_mmpll0_mmpll4_mmpll7_mmpll10_gpll0_g
 	{ .hw = &mmpll7_out_even.clkr.hw },
 	{ .hw = &mmpll10_out_even.clkr.hw },
 	{ .fw_name = "gpll0" },
-	{ .hw = &gpll0_div.hw },
+	{ .fw_name = "gpll0_div", .name = "gcc_mmss_gpll0_div_clk" },
 };
 
 static struct clk_rcg2 byte0_clk_src = {
@@ -2544,10 +2531,6 @@ static struct clk_branch vmem_ahb_clk = {
 	},
 };
 
-static struct clk_hw *mmcc_msm8998_hws[] = {
-	&gpll0_div.hw,
-};
-
 static struct gdsc video_top_gdsc = {
 	.gdscr = 0x1024,
 	.pd = {
@@ -2855,8 +2838,6 @@ static const struct qcom_cc_desc mmcc_msm8998_desc = {
 	.num_resets = ARRAY_SIZE(mmcc_msm8998_resets),
 	.gdscs = mmcc_msm8998_gdscs,
 	.num_gdscs = ARRAY_SIZE(mmcc_msm8998_gdscs),
-	.clk_hws = mmcc_msm8998_hws,
-	.num_clk_hws = ARRAY_SIZE(mmcc_msm8998_hws),
 };
 
 static const struct of_device_id mmcc_msm8998_match_table[] = {

-- 
2.41.0

[PATCH 5/9] clk: qcom: gpucc-msm8998: Use the correct GPLL0 leg with old DTs

[Konrad Dybcio]

GPUCC has its own GPLL0 legs - one for 1-1 and one for div-2 output.
Add .name lookup to make sure older DTs consume the correct clock.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gpucc-msm8998.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gpucc-msm8998.c b/drivers/clk/qcom/gpucc-msm8998.c
index f929e0f2333f..cc0b43354787 100644
--- a/drivers/clk/qcom/gpucc-msm8998.c
+++ b/drivers/clk/qcom/gpucc-msm8998.c
@@ -98,7 +98,7 @@ static const struct parent_map gpu_xo_gpll0_map[] = {
 
 static const struct clk_parent_data gpu_xo_gpll0[] = {
 	{ .hw = &gpucc_cxo_clk.clkr.hw },
-	{ .fw_name = "gpll0" },
+	{ .fw_name = "gpll0", .name = "gcc_gpu_gpll0_clk" },
 };
 
 static const struct parent_map gpu_xo_gpupll0_map[] = {

-- 
2.41.0

[PATCH 6/9] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks

[Konrad Dybcio]

Some branch clocks are governed externally and we're only supposed to
send a request concerning their shutdown, not actually ensure it happens.

Use the BRANCH_HALT_SKIP define to skip checking the halt bit.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-msm8998.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
index cccb19cae481..ef410f52f09f 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -2112,7 +2112,7 @@ static struct clk_branch gcc_gp3_clk = {
 
 static struct clk_branch gcc_bimc_gfx_clk = {
 	.halt_reg = 0x46040,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x46040,
 		.enable_mask = BIT(0),
@@ -2125,7 +2125,7 @@ static struct clk_branch gcc_bimc_gfx_clk = {
 
 static struct clk_branch gcc_gpu_bimc_gfx_clk = {
 	.halt_reg = 0x71010,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x71010,
 		.enable_mask = BIT(0),
@@ -2151,7 +2151,7 @@ static struct clk_branch gcc_gpu_bimc_gfx_src_clk = {
 
 static struct clk_branch gcc_gpu_cfg_ahb_clk = {
 	.halt_reg = 0x71004,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x71004,
 		.enable_mask = BIT(0),

-- 
2.41.0

[PATCH 7/9] clk: qcom: gcc-msm8998: Don't poke at some BIMC GPU clocks

[Konrad Dybcio]

Linux should apparently not be concerned with gcc_gpu_bimc_gfx_src_clk and
gcc_gpu_bimc_gfx_src_clk on MSM8998, as they're preconfigured for us.
Unregister them to prevent issues.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-msm8998.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
index ef410f52f09f..980b5a1b58ae 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -2136,19 +2136,6 @@ static struct clk_branch gcc_gpu_bimc_gfx_clk = {
 	},
 };
 
-static struct clk_branch gcc_gpu_bimc_gfx_src_clk = {
-	.halt_reg = 0x7100c,
-	.halt_check = BRANCH_HALT,
-	.clkr = {
-		.enable_reg = 0x7100c,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_gpu_bimc_gfx_src_clk",
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_gpu_cfg_ahb_clk = {
 	.halt_reg = 0x71004,
 	.halt_check = BRANCH_HALT_SKIP,
@@ -2168,19 +2155,6 @@ static struct clk_branch gcc_gpu_cfg_ahb_clk = {
 	},
 };
 
-static struct clk_branch gcc_gpu_snoc_dvm_gfx_clk = {
-	.halt_reg = 0x71018,
-	.halt_check = BRANCH_HALT,
-	.clkr = {
-		.enable_reg = 0x71018,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_gpu_snoc_dvm_gfx_clk",
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_hmss_ahb_clk = {
 	.halt_reg = 0x48000,
 	.halt_check = BRANCH_HALT_VOTED,
@@ -3032,9 +3006,7 @@ static struct clk_regmap *gcc_msm8998_clocks[] = {
 	[GCC_GP3_CLK] = &gcc_gp3_clk.clkr,
 	[GCC_BIMC_GFX_CLK] = &gcc_bimc_gfx_clk.clkr,
 	[GCC_GPU_BIMC_GFX_CLK] = &gcc_gpu_bimc_gfx_clk.clkr,
-	[GCC_GPU_BIMC_GFX_SRC_CLK] = &gcc_gpu_bimc_gfx_src_clk.clkr,
 	[GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
-	[GCC_GPU_SNOC_DVM_GFX_CLK] = &gcc_gpu_snoc_dvm_gfx_clk.clkr,
 	[GCC_HMSS_AHB_CLK] = &gcc_hmss_ahb_clk.clkr,
 	[GCC_HMSS_AT_CLK] = &gcc_hmss_at_clk.clkr,
 	[GCC_HMSS_RBCPR_CLK] = &gcc_hmss_rbcpr_clk.clkr,

-- 
2.41.0

[PATCH 8/9] arm64: dts: qcom: msm8998: Use the correct GPLL0 leg for GPUCC

[Konrad Dybcio]

GPUCC has its own GPLL0 leg, switch to it to allow shutting it down
when it's unused.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index f0e943ff0046..74bd05579796 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -1574,7 +1574,7 @@ gpucc: clock-controller@5065000 {
 			reg = <0x05065000 0x9000>;
 
 			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
-				 <&gcc GPLL0_OUT_MAIN>;
+				 <&gcc GCC_GPU_GPLL0_CLK>;
 			clock-names = "xo",
 				      "gpll0";
 		};

-- 
2.41.0

[PATCH 9/9] arm64: dts: qcom: msm8998: Use the correct GPLL0_DIV leg for MMCC

[Konrad Dybcio]

MMCC has its own GPLL0 legs - one for 1-1 and one for div-2 output.
We've already been using the correct one in the non-div case, start
doing so for the other one as well.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 74bd05579796..c4faba092368 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -2718,7 +2718,8 @@ mmcc: clock-controller@c8c0000 {
 				      "dsi1byte",
 				      "hdmipll",
 				      "dplink",
-				      "dpvco";
+				      "dpvco",
+				      "gpll0_div";
 			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
 				 <&gcc GCC_MMSS_GPLL0_CLK>,
 				 <0>,
@@ -2727,7 +2728,8 @@ mmcc: clock-controller@c8c0000 {
 				 <0>,
 				 <0>,
 				 <0>,
-				 <0>;
+				 <0>,
+				 <&gcc GCC_MMSS_GPLL0_DIV_CLK>;
 		};
 
 		mmss_smmu: iommu@cd00000 {

-- 
2.41.0

Re: [PATCH 6/9] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks

[Konrad Dybcio]

On 22.06.2023 13:57, Konrad Dybcio wrote:
> Some branch clocks are governed externally and we're only supposed to
> send a request concerning their shutdown, not actually ensure it happens.
> 
> Use the BRANCH_HALT_SKIP define to skip checking the halt bit.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
Whoops. Again, right after hitting send, I noticed this one was already
sent with 

https://lore.kernel.org/linux-arm-msm/20230531-topic-8998_mmssclk-v1-0-2b5a8fc90991@linaro.org/
>  drivers/clk/qcom/gcc-msm8998.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
> index cccb19cae481..ef410f52f09f 100644
> --- a/drivers/clk/qcom/gcc-msm8998.c
> +++ b/drivers/clk/qcom/gcc-msm8998.c
> @@ -2112,7 +2112,7 @@ static struct clk_branch gcc_gp3_clk = {
>  
>  static struct clk_branch gcc_bimc_gfx_clk = {
>  	.halt_reg = 0x46040,
> -	.halt_check = BRANCH_HALT,
> +	.halt_check = BRANCH_HALT_SKIP,
>  	.clkr = {
>  		.enable_reg = 0x46040,
>  		.enable_mask = BIT(0),
> @@ -2125,7 +2125,7 @@ static struct clk_branch gcc_bimc_gfx_clk = {
>  
>  static struct clk_branch gcc_gpu_bimc_gfx_clk = {
>  	.halt_reg = 0x71010,
> -	.halt_check = BRANCH_HALT,
> +	.halt_check = BRANCH_HALT_SKIP,
>  	.clkr = {
>  		.enable_reg = 0x71010,
>  		.enable_mask = BIT(0),
> @@ -2151,7 +2151,7 @@ static struct clk_branch gcc_gpu_bimc_gfx_src_clk = {
>  
>  static struct clk_branch gcc_gpu_cfg_ahb_clk = {
>  	.halt_reg = 0x71004,
> -	.halt_check = BRANCH_HALT,
> +	.halt_check = BRANCH_HALT_SKIP,
>  	.clkr = {
>  		.enable_reg = 0x71004,
>  		.enable_mask = BIT(0),
> 

Re: [PATCH 1/9] dt-bindings: clk: qcom,gcc-msm8998: Add missing GPU/MMSS GPLL0 legs

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> GPLL0 has two separate outputs to both GPUSS and MMSS: one that's
> 2-divided and one that runs at the same rate as the GPLL0 itself.
> 
> Add the missing ones to the binding.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 2/9] dt-bindings: clock: qcom,mmcc: Add GPLL0_DIV for MSM8998

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> We've not been consuming that clock for no apparent reason. Describe it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---

Nice catch.

Acked-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 2/9] dt-bindings: clock: qcom,mmcc: Add GPLL0_DIV for MSM8998

[Rob Herring]

On Thu, 22 Jun 2023 13:57:42 +0200, Konrad Dybcio wrote:
> We've not been consuming that clock for no apparent reason. Describe it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  Documentation/devicetree/bindings/clock/qcom,mmcc.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 3/9] clk: qcom: gcc-msm8998: Control MMSS and GPUSS GPLL0 outputs properly

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> Up until now, we've been relying on some non-descript hardware magic
> to pinkypromise turn the clocks on for us. While new SoCs shine with
> that feature, MSM8998 can not always be fully trusted.
> 
> Register the MMSS and GPUSS GPLL0 legs with the CCF to allow for manual
> enable voting.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/clk/qcom/gcc-msm8998.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
> index be024f8093c5..cccb19cae481 100644
> --- a/drivers/clk/qcom/gcc-msm8998.c
> +++ b/drivers/clk/qcom/gcc-msm8998.c
> @@ -25,6 +25,9 @@
>   #include "reset.h"
>   #include "gdsc.h"
>   
> +#define GCC_MMSS_MISC	0x0902C
> +#define GCC_GPU_MISC	0x71028
> +
>   static struct pll_vco fabia_vco[] = {
>   	{ 250000000, 2000000000, 0 },
>   	{ 125000000, 1000000000, 1 },
> @@ -1367,6 +1370,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk = {
>   	},
>   };
>   
> +static struct clk_branch gcc_mmss_gpll0_div_clk = {
> +	.halt_check = BRANCH_HALT_DELAY,
> +	.clkr = {
> +		.enable_reg = 0x5200c,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gcc_mmss_gpll0_div_clk",
> +			.parent_hws = (const struct clk_hw *[]) {
> +				&gpll0_out_main.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
>   static struct clk_branch gcc_mmss_gpll0_clk = {
>   	.halt_check = BRANCH_HALT_DELAY,
>   	.clkr = {
> @@ -1395,6 +1414,38 @@ static struct clk_branch gcc_mss_gpll0_div_clk_src = {
>   	},
>   };
>   
> +static struct clk_branch gcc_gpu_gpll0_div_clk = {
> +	.halt_check = BRANCH_HALT_DELAY,
> +	.clkr = {
> +		.enable_reg = 0x5200c,
> +		.enable_mask = BIT(3),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gcc_gpu_gpll0_div_clk",
> +			.parent_hws = (const struct clk_hw *[]) {
> +				&gpll0_out_main.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gcc_gpu_gpll0_clk = {
> +	.halt_check = BRANCH_HALT_DELAY,
> +	.clkr = {
> +		.enable_reg = 0x5200c,
> +		.enable_mask = BIT(4),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gcc_gpu_gpll0_clk",
> +			.parent_hws = (const struct clk_hw *[]) {
> +				&gpll0_out_main.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
>   static struct clk_branch gcc_blsp1_ahb_clk = {
>   	.halt_reg = 0x17004,
>   	.halt_check = BRANCH_HALT_VOTED,
> @@ -3080,6 +3131,9 @@ static struct clk_regmap *gcc_msm8998_clocks[] = {
>   	[AGGRE2_SNOC_NORTH_AXI] = &aggre2_snoc_north_axi_clk.clkr,
>   	[SSC_XO] = &ssc_xo_clk.clkr,
>   	[SSC_CNOC_AHBS_CLK] = &ssc_cnoc_ahbs_clk.clkr,
> +	[GCC_MMSS_GPLL0_DIV_CLK] = &gcc_mmss_gpll0_div_clk.clkr,
> +	[GCC_GPU_GPLL0_DIV_CLK] = &gcc_gpu_gpll0_div_clk.clkr,
> +	[GCC_GPU_GPLL0_CLK] = &gcc_gpu_gpll0_clk.clkr,
>   };
>   
>   static struct gdsc *gcc_msm8998_gdscs[] = {
> @@ -3235,6 +3289,10 @@ static int gcc_msm8998_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	/* Disable the GPLL0 active input to MMSS and GPU via MISC registers */
> +	regmap_write(regmap, GCC_MMSS_MISC, 0x10003);
> +	regmap_write(regmap, GCC_GPU_MISC, 0x10003);

I wonder, does this disrupt a handoff of an active display from the 
bootloader to Linux?

Re: [PATCH 4/9] clk: qcom: mmcc-msm8998: Properly consume GPLL0 inputs

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> Up until now, the GPLL0_DIV MMSS input has been modeled as a fixed
> child of MMSS_GPLL0_DIV that's always-on. Properly representing the
> former in the GCC driver makes us unable to keep doing so.
> 
> Consume MSS_GPLL0_DIV through fw_name ("gpll0_div") as well as add a
> fixed .name link to keep backwards compatibility.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 5/9] clk: qcom: gpucc-msm8998: Use the correct GPLL0 leg with old DTs

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> GPUCC has its own GPLL0 legs - one for 1-1 and one for div-2 output.
> Add .name lookup to make sure older DTs consume the correct clock.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 6/9] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> Some branch clocks are governed externally and we're only supposed to
> send a request concerning their shutdown, not actually ensure it happens.
> 
> Use the BRANCH_HALT_SKIP define to skip checking the halt bit.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 3/9] clk: qcom: gcc-msm8998: Control MMSS and GPUSS GPLL0 outputs properly

[Konrad Dybcio]

On 22.06.2023 16:55, Jeffrey Hugo wrote:
> On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
>> Up until now, we've been relying on some non-descript hardware magic
>> to pinkypromise turn the clocks on for us. While new SoCs shine with
>> that feature, MSM8998 can not always be fully trusted.
>>
>> Register the MMSS and GPUSS GPLL0 legs with the CCF to allow for manual
>> enable voting.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/clk/qcom/gcc-msm8998.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
>> index be024f8093c5..cccb19cae481 100644
>> --- a/drivers/clk/qcom/gcc-msm8998.c
>> +++ b/drivers/clk/qcom/gcc-msm8998.c
>> @@ -25,6 +25,9 @@
>>   #include "reset.h"
>>   #include "gdsc.h"
>>   +#define GCC_MMSS_MISC    0x0902C
>> +#define GCC_GPU_MISC    0x71028
>> +
>>   static struct pll_vco fabia_vco[] = {
>>       { 250000000, 2000000000, 0 },
>>       { 125000000, 1000000000, 1 },
>> @@ -1367,6 +1370,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk = {
>>       },
>>   };
>>   +static struct clk_branch gcc_mmss_gpll0_div_clk = {
>> +    .halt_check = BRANCH_HALT_DELAY,
>> +    .clkr = {
>> +        .enable_reg = 0x5200c,
>> +        .enable_mask = BIT(0),
>> +        .hw.init = &(struct clk_init_data){
>> +            .name = "gcc_mmss_gpll0_div_clk",
>> +            .parent_hws = (const struct clk_hw *[]) {
>> +                &gpll0_out_main.clkr.hw,
>> +            },
>> +            .num_parents = 1,
>> +            .ops = &clk_branch2_ops,
>> +        },
>> +    },
>> +};
>> +
>>   static struct clk_branch gcc_mmss_gpll0_clk = {
>>       .halt_check = BRANCH_HALT_DELAY,
>>       .clkr = {
>> @@ -1395,6 +1414,38 @@ static struct clk_branch gcc_mss_gpll0_div_clk_src = {
>>       },
>>   };
>>   +static struct clk_branch gcc_gpu_gpll0_div_clk = {
>> +    .halt_check = BRANCH_HALT_DELAY,
>> +    .clkr = {
>> +        .enable_reg = 0x5200c,
>> +        .enable_mask = BIT(3),
>> +        .hw.init = &(struct clk_init_data){
>> +            .name = "gcc_gpu_gpll0_div_clk",
>> +            .parent_hws = (const struct clk_hw *[]) {
>> +                &gpll0_out_main.clkr.hw,
>> +            },
>> +            .num_parents = 1,
>> +            .ops = &clk_branch2_ops,
>> +        },
>> +    },
>> +};
>> +
>> +static struct clk_branch gcc_gpu_gpll0_clk = {
>> +    .halt_check = BRANCH_HALT_DELAY,
>> +    .clkr = {
>> +        .enable_reg = 0x5200c,
>> +        .enable_mask = BIT(4),
>> +        .hw.init = &(struct clk_init_data){
>> +            .name = "gcc_gpu_gpll0_clk",
>> +            .parent_hws = (const struct clk_hw *[]) {
>> +                &gpll0_out_main.clkr.hw,
>> +            },
>> +            .num_parents = 1,
>> +            .ops = &clk_branch2_ops,
>> +        },
>> +    },
>> +};
>> +
>>   static struct clk_branch gcc_blsp1_ahb_clk = {
>>       .halt_reg = 0x17004,
>>       .halt_check = BRANCH_HALT_VOTED,
>> @@ -3080,6 +3131,9 @@ static struct clk_regmap *gcc_msm8998_clocks[] = {
>>       [AGGRE2_SNOC_NORTH_AXI] = &aggre2_snoc_north_axi_clk.clkr,
>>       [SSC_XO] = &ssc_xo_clk.clkr,
>>       [SSC_CNOC_AHBS_CLK] = &ssc_cnoc_ahbs_clk.clkr,
>> +    [GCC_MMSS_GPLL0_DIV_CLK] = &gcc_mmss_gpll0_div_clk.clkr,
>> +    [GCC_GPU_GPLL0_DIV_CLK] = &gcc_gpu_gpll0_div_clk.clkr,
>> +    [GCC_GPU_GPLL0_CLK] = &gcc_gpu_gpll0_clk.clkr,
>>   };
>>     static struct gdsc *gcc_msm8998_gdscs[] = {
>> @@ -3235,6 +3289,10 @@ static int gcc_msm8998_probe(struct platform_device *pdev)
>>       if (ret)
>>           return ret;
>>   +    /* Disable the GPLL0 active input to MMSS and GPU via MISC registers */
>> +    regmap_write(regmap, GCC_MMSS_MISC, 0x10003);
>> +    regmap_write(regmap, GCC_GPU_MISC, 0x10003);
> 
> I wonder, does this disrupt a handoff of an active display from the bootloader to Linux?
My phone's bootloader doesn't initialize the display, if you (or Angelo
or Jami, +CC) could test this, it'd be wonderful.

Konrad

Re: [PATCH 3/9] clk: qcom: gcc-msm8998: Control MMSS and GPUSS GPLL0 outputs properly

[Jeffrey Hugo]

On 6/22/2023 9:05 AM, Konrad Dybcio wrote:
> On 22.06.2023 16:55, Jeffrey Hugo wrote:
>> On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
>>> Up until now, we've been relying on some non-descript hardware magic
>>> to pinkypromise turn the clocks on for us. While new SoCs shine with
>>> that feature, MSM8998 can not always be fully trusted.
>>>
>>> Register the MMSS and GPUSS GPLL0 legs with the CCF to allow for manual
>>> enable voting.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/clk/qcom/gcc-msm8998.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 58 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
>>> index be024f8093c5..cccb19cae481 100644
>>> --- a/drivers/clk/qcom/gcc-msm8998.c
>>> +++ b/drivers/clk/qcom/gcc-msm8998.c
>>> @@ -25,6 +25,9 @@
>>>    #include "reset.h"
>>>    #include "gdsc.h"
>>>    +#define GCC_MMSS_MISC    0x0902C
>>> +#define GCC_GPU_MISC    0x71028
>>> +
>>>    static struct pll_vco fabia_vco[] = {
>>>        { 250000000, 2000000000, 0 },
>>>        { 125000000, 1000000000, 1 },
>>> @@ -1367,6 +1370,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk = {
>>>        },
>>>    };
>>>    +static struct clk_branch gcc_mmss_gpll0_div_clk = {
>>> +    .halt_check = BRANCH_HALT_DELAY,
>>> +    .clkr = {
>>> +        .enable_reg = 0x5200c,
>>> +        .enable_mask = BIT(0),
>>> +        .hw.init = &(struct clk_init_data){
>>> +            .name = "gcc_mmss_gpll0_div_clk",
>>> +            .parent_hws = (const struct clk_hw *[]) {
>>> +                &gpll0_out_main.clkr.hw,
>>> +            },
>>> +            .num_parents = 1,
>>> +            .ops = &clk_branch2_ops,
>>> +        },
>>> +    },
>>> +};
>>> +
>>>    static struct clk_branch gcc_mmss_gpll0_clk = {
>>>        .halt_check = BRANCH_HALT_DELAY,
>>>        .clkr = {
>>> @@ -1395,6 +1414,38 @@ static struct clk_branch gcc_mss_gpll0_div_clk_src = {
>>>        },
>>>    };
>>>    +static struct clk_branch gcc_gpu_gpll0_div_clk = {
>>> +    .halt_check = BRANCH_HALT_DELAY,
>>> +    .clkr = {
>>> +        .enable_reg = 0x5200c,
>>> +        .enable_mask = BIT(3),
>>> +        .hw.init = &(struct clk_init_data){
>>> +            .name = "gcc_gpu_gpll0_div_clk",
>>> +            .parent_hws = (const struct clk_hw *[]) {
>>> +                &gpll0_out_main.clkr.hw,
>>> +            },
>>> +            .num_parents = 1,
>>> +            .ops = &clk_branch2_ops,
>>> +        },
>>> +    },
>>> +};
>>> +
>>> +static struct clk_branch gcc_gpu_gpll0_clk = {
>>> +    .halt_check = BRANCH_HALT_DELAY,
>>> +    .clkr = {
>>> +        .enable_reg = 0x5200c,
>>> +        .enable_mask = BIT(4),
>>> +        .hw.init = &(struct clk_init_data){
>>> +            .name = "gcc_gpu_gpll0_clk",
>>> +            .parent_hws = (const struct clk_hw *[]) {
>>> +                &gpll0_out_main.clkr.hw,
>>> +            },
>>> +            .num_parents = 1,
>>> +            .ops = &clk_branch2_ops,
>>> +        },
>>> +    },
>>> +};
>>> +
>>>    static struct clk_branch gcc_blsp1_ahb_clk = {
>>>        .halt_reg = 0x17004,
>>>        .halt_check = BRANCH_HALT_VOTED,
>>> @@ -3080,6 +3131,9 @@ static struct clk_regmap *gcc_msm8998_clocks[] = {
>>>        [AGGRE2_SNOC_NORTH_AXI] = &aggre2_snoc_north_axi_clk.clkr,
>>>        [SSC_XO] = &ssc_xo_clk.clkr,
>>>        [SSC_CNOC_AHBS_CLK] = &ssc_cnoc_ahbs_clk.clkr,
>>> +    [GCC_MMSS_GPLL0_DIV_CLK] = &gcc_mmss_gpll0_div_clk.clkr,
>>> +    [GCC_GPU_GPLL0_DIV_CLK] = &gcc_gpu_gpll0_div_clk.clkr,
>>> +    [GCC_GPU_GPLL0_CLK] = &gcc_gpu_gpll0_clk.clkr,
>>>    };
>>>      static struct gdsc *gcc_msm8998_gdscs[] = {
>>> @@ -3235,6 +3289,10 @@ static int gcc_msm8998_probe(struct platform_device *pdev)
>>>        if (ret)
>>>            return ret;
>>>    +    /* Disable the GPLL0 active input to MMSS and GPU via MISC registers */
>>> +    regmap_write(regmap, GCC_MMSS_MISC, 0x10003);
>>> +    regmap_write(regmap, GCC_GPU_MISC, 0x10003);
>>
>> I wonder, does this disrupt a handoff of an active display from the bootloader to Linux?
> My phone's bootloader doesn't initialize the display, if you (or Angelo
> or Jami, +CC) could test this, it'd be wonderful.

Let me carve out some time to try it on the laptop.

Re: [PATCH 7/9] clk: qcom: gcc-msm8998: Don't poke at some BIMC GPU clocks

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> Linux should apparently not be concerned with gcc_gpu_bimc_gfx_src_clk and
> gcc_gpu_bimc_gfx_src_clk on MSM8998, as they're preconfigured for us.
> Unregister them to prevent issues.

You mention the bimc_gfx clock twice here.  One of them has to be a typo.

Also, can you clarify the reasoning?  The RCG is controlled by the RPM, 
but the branch clock is under the control of Linux.

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/clk/qcom/gcc-msm8998.c | 28 ----------------------------
>   1 file changed, 28 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
> index ef410f52f09f..980b5a1b58ae 100644
> --- a/drivers/clk/qcom/gcc-msm8998.c
> +++ b/drivers/clk/qcom/gcc-msm8998.c
> @@ -2136,19 +2136,6 @@ static struct clk_branch gcc_gpu_bimc_gfx_clk = {
>   	},
>   };
>   
> -static struct clk_branch gcc_gpu_bimc_gfx_src_clk = {
> -	.halt_reg = 0x7100c,
> -	.halt_check = BRANCH_HALT,
> -	.clkr = {
> -		.enable_reg = 0x7100c,
> -		.enable_mask = BIT(0),
> -		.hw.init = &(struct clk_init_data){
> -			.name = "gcc_gpu_bimc_gfx_src_clk",
> -			.ops = &clk_branch2_ops,
> -		},
> -	},
> -};
> -
>   static struct clk_branch gcc_gpu_cfg_ahb_clk = {
>   	.halt_reg = 0x71004,
>   	.halt_check = BRANCH_HALT_SKIP,
> @@ -2168,19 +2155,6 @@ static struct clk_branch gcc_gpu_cfg_ahb_clk = {
>   	},
>   };
>   
> -static struct clk_branch gcc_gpu_snoc_dvm_gfx_clk = {
> -	.halt_reg = 0x71018,
> -	.halt_check = BRANCH_HALT,
> -	.clkr = {
> -		.enable_reg = 0x71018,
> -		.enable_mask = BIT(0),
> -		.hw.init = &(struct clk_init_data){
> -			.name = "gcc_gpu_snoc_dvm_gfx_clk",
> -			.ops = &clk_branch2_ops,
> -		},
> -	},
> -};
> -
>   static struct clk_branch gcc_hmss_ahb_clk = {
>   	.halt_reg = 0x48000,
>   	.halt_check = BRANCH_HALT_VOTED,
> @@ -3032,9 +3006,7 @@ static struct clk_regmap *gcc_msm8998_clocks[] = {
>   	[GCC_GP3_CLK] = &gcc_gp3_clk.clkr,
>   	[GCC_BIMC_GFX_CLK] = &gcc_bimc_gfx_clk.clkr,
>   	[GCC_GPU_BIMC_GFX_CLK] = &gcc_gpu_bimc_gfx_clk.clkr,
> -	[GCC_GPU_BIMC_GFX_SRC_CLK] = &gcc_gpu_bimc_gfx_src_clk.clkr,
>   	[GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
> -	[GCC_GPU_SNOC_DVM_GFX_CLK] = &gcc_gpu_snoc_dvm_gfx_clk.clkr,
>   	[GCC_HMSS_AHB_CLK] = &gcc_hmss_ahb_clk.clkr,
>   	[GCC_HMSS_AT_CLK] = &gcc_hmss_at_clk.clkr,
>   	[GCC_HMSS_RBCPR_CLK] = &gcc_hmss_rbcpr_clk.clkr,
> 

Re: [PATCH 8/9] arm64: dts: qcom: msm8998: Use the correct GPLL0 leg for GPUCC

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> GPUCC has its own GPLL0 leg, switch to it to allow shutting it down
> when it's unused.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 9/9] arm64: dts: qcom: msm8998: Use the correct GPLL0_DIV leg for MMCC

[Jeffrey Hugo]

On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
> MMCC has its own GPLL0 legs - one for 1-1 and one for div-2 output.
> We've already been using the correct one in the non-div case, start
> doing so for the other one as well.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 1/9] dt-bindings: clk: qcom,gcc-msm8998: Add missing GPU/MMSS GPLL0 legs

[Krzysztof Kozlowski]

On 22/06/2023 13:57, Konrad Dybcio wrote:
> GPLL0 has two separate outputs to both GPUSS and MMSS: one that's
> 2-divided and one that runs at the same rate as the GPLL0 itself.
> 
> Add the missing ones to the binding.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/9] dt-bindings: clock: qcom,mmcc: Add GPLL0_DIV for MSM8998

[Krzysztof Kozlowski]

On 22/06/2023 13:57, Konrad Dybcio wrote:
> We've not been consuming that clock for no apparent reason. Describe it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 3/9] clk: qcom: gcc-msm8998: Control MMSS and GPUSS GPLL0 outputs properly

[Jeffrey Hugo]

On 6/22/2023 9:19 AM, Jeffrey Hugo wrote:
> On 6/22/2023 9:05 AM, Konrad Dybcio wrote:
>> On 22.06.2023 16:55, Jeffrey Hugo wrote:
>>> On 6/22/2023 5:57 AM, Konrad Dybcio wrote:
>>>> Up until now, we've been relying on some non-descript hardware magic
>>>> to pinkypromise turn the clocks on for us. While new SoCs shine with
>>>> that feature, MSM8998 can not always be fully trusted.
>>>>
>>>> Register the MMSS and GPUSS GPLL0 legs with the CCF to allow for manual
>>>> enable voting.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/clk/qcom/gcc-msm8998.c | 58 
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 58 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/gcc-msm8998.c 
>>>> b/drivers/clk/qcom/gcc-msm8998.c
>>>> index be024f8093c5..cccb19cae481 100644
>>>> --- a/drivers/clk/qcom/gcc-msm8998.c
>>>> +++ b/drivers/clk/qcom/gcc-msm8998.c
>>>> @@ -25,6 +25,9 @@
>>>>    #include "reset.h"
>>>>    #include "gdsc.h"
>>>>    +#define GCC_MMSS_MISC    0x0902C
>>>> +#define GCC_GPU_MISC    0x71028
>>>> +
>>>>    static struct pll_vco fabia_vco[] = {
>>>>        { 250000000, 2000000000, 0 },
>>>>        { 125000000, 1000000000, 1 },
>>>> @@ -1367,6 +1370,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk 
>>>> = {
>>>>        },
>>>>    };
>>>>    +static struct clk_branch gcc_mmss_gpll0_div_clk = {
>>>> +    .halt_check = BRANCH_HALT_DELAY,
>>>> +    .clkr = {
>>>> +        .enable_reg = 0x5200c,
>>>> +        .enable_mask = BIT(0),
>>>> +        .hw.init = &(struct clk_init_data){
>>>> +            .name = "gcc_mmss_gpll0_div_clk",
>>>> +            .parent_hws = (const struct clk_hw *[]) {
>>>> +                &gpll0_out_main.clkr.hw,
>>>> +            },
>>>> +            .num_parents = 1,
>>>> +            .ops = &clk_branch2_ops,
>>>> +        },
>>>> +    },
>>>> +};
>>>> +
>>>>    static struct clk_branch gcc_mmss_gpll0_clk = {
>>>>        .halt_check = BRANCH_HALT_DELAY,
>>>>        .clkr = {
>>>> @@ -1395,6 +1414,38 @@ static struct clk_branch 
>>>> gcc_mss_gpll0_div_clk_src = {
>>>>        },
>>>>    };
>>>>    +static struct clk_branch gcc_gpu_gpll0_div_clk = {
>>>> +    .halt_check = BRANCH_HALT_DELAY,
>>>> +    .clkr = {
>>>> +        .enable_reg = 0x5200c,
>>>> +        .enable_mask = BIT(3),
>>>> +        .hw.init = &(struct clk_init_data){
>>>> +            .name = "gcc_gpu_gpll0_div_clk",
>>>> +            .parent_hws = (const struct clk_hw *[]) {
>>>> +                &gpll0_out_main.clkr.hw,
>>>> +            },
>>>> +            .num_parents = 1,
>>>> +            .ops = &clk_branch2_ops,
>>>> +        },
>>>> +    },
>>>> +};
>>>> +
>>>> +static struct clk_branch gcc_gpu_gpll0_clk = {
>>>> +    .halt_check = BRANCH_HALT_DELAY,
>>>> +    .clkr = {
>>>> +        .enable_reg = 0x5200c,
>>>> +        .enable_mask = BIT(4),
>>>> +        .hw.init = &(struct clk_init_data){
>>>> +            .name = "gcc_gpu_gpll0_clk",
>>>> +            .parent_hws = (const struct clk_hw *[]) {
>>>> +                &gpll0_out_main.clkr.hw,
>>>> +            },
>>>> +            .num_parents = 1,
>>>> +            .ops = &clk_branch2_ops,
>>>> +        },
>>>> +    },
>>>> +};
>>>> +
>>>>    static struct clk_branch gcc_blsp1_ahb_clk = {
>>>>        .halt_reg = 0x17004,
>>>>        .halt_check = BRANCH_HALT_VOTED,
>>>> @@ -3080,6 +3131,9 @@ static struct clk_regmap *gcc_msm8998_clocks[] 
>>>> = {
>>>>        [AGGRE2_SNOC_NORTH_AXI] = &aggre2_snoc_north_axi_clk.clkr,
>>>>        [SSC_XO] = &ssc_xo_clk.clkr,
>>>>        [SSC_CNOC_AHBS_CLK] = &ssc_cnoc_ahbs_clk.clkr,
>>>> +    [GCC_MMSS_GPLL0_DIV_CLK] = &gcc_mmss_gpll0_div_clk.clkr,
>>>> +    [GCC_GPU_GPLL0_DIV_CLK] = &gcc_gpu_gpll0_div_clk.clkr,
>>>> +    [GCC_GPU_GPLL0_CLK] = &gcc_gpu_gpll0_clk.clkr,
>>>>    };
>>>>      static struct gdsc *gcc_msm8998_gdscs[] = {
>>>> @@ -3235,6 +3289,10 @@ static int gcc_msm8998_probe(struct 
>>>> platform_device *pdev)
>>>>        if (ret)
>>>>            return ret;
>>>>    +    /* Disable the GPLL0 active input to MMSS and GPU via MISC 
>>>> registers */
>>>> +    regmap_write(regmap, GCC_MMSS_MISC, 0x10003);
>>>> +    regmap_write(regmap, GCC_GPU_MISC, 0x10003);
>>>
>>> I wonder, does this disrupt a handoff of an active display from the 
>>> bootloader to Linux?
>> My phone's bootloader doesn't initialize the display, if you (or Angelo
>> or Jami, +CC) could test this, it'd be wonderful.
> 
> Let me carve out some time to try it on the laptop.

Tested on the Lenovo Miix 630, and observed no issues.

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

For patches 3-6 inclusive
Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>