2023-06-22 11:59:50

by Konrad Dybcio

[permalink] [raw]
Subject: [PATCH 0/9] MSM8998 clk cleanups and fixups

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



2023-06-22 11:59:57

by Konrad Dybcio

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

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

Signed-off-by: Konrad Dybcio <[email protected]>
---
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


2023-06-22 12:00:15

by Konrad Dybcio

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

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


2023-06-22 12:00:40

by Konrad Dybcio

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

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


2023-06-22 12:04:59

by Konrad Dybcio

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

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


2023-06-22 12:09:17

by Konrad Dybcio

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

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


2023-06-22 12:16:59

by Konrad Dybcio

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

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


2023-06-22 12:31:45

by Konrad Dybcio

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

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

Signed-off-by: Konrad Dybcio <[email protected]>
---
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


2023-06-22 12:34:09

by Konrad Dybcio

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

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


2023-06-22 12:34:47

by Konrad Dybcio

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

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


2023-06-22 14:45:23

by Jeffrey Hugo

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

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

Reviewed-by: Jeffrey Hugo <[email protected]>


2023-06-22 14:59:12

by Jeffrey Hugo

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

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

Nice catch.

Acked-by: Jeffrey Hugo <[email protected]>

2023-06-22 15:13:03

by Jeffrey Hugo

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

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

2023-06-22 15:14:41

by Jeffrey Hugo

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

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

Reviewed-by: Jeffrey Hugo <[email protected]>

2023-06-22 15:14:49

by Jeffrey Hugo

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

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

Reviewed-by: Jeffrey Hugo <[email protected]>

2023-06-22 15:20:17

by Rob Herring

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


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 <[email protected]>
> ---
> Documentation/devicetree/bindings/clock/qcom,mmcc.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>

Acked-by: Rob Herring <[email protected]>


2023-06-22 15:24:05

by Jeffrey Hugo

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

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

Reviewed-by: Jeffrey Hugo <[email protected]>

2023-06-22 15:37:32

by Konrad Dybcio

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

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

2023-06-22 15:59:29

by Jeffrey Hugo

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

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

2023-06-23 14:35:28

by Jeffrey Hugo

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

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

Reviewed-by: Jeffrey Hugo <[email protected]>

2023-06-23 14:37:21

by Jeffrey Hugo

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

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

Reviewed-by: Jeffrey Hugo <[email protected]>

2023-06-23 14:41:09

by Jeffrey Hugo

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

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


2023-06-23 17:09:14

by Krzysztof Kozlowski

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

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


Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof


2023-06-23 17:13:35

by Krzysztof Kozlowski

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

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


Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof


2023-06-26 16:26:37

by Jeffrey Hugo

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

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

For patches 3-6 inclusive
Tested-by: Jeffrey Hugo <[email protected]>