Phone-Devel Archive on lore.kernel.org.
 help / color / Atom feed
* [PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map
@ 2021-02-20 15:56 Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 2/6] clk: qcom: gcc-sdm660: Set HWCG bit to 1 on some clocks Konrad Dybcio
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-20 15:56 UTC (permalink / raw)
  To: phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Rob Herring, Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk,
	linux-kernel, devicetree

The correct one is gcc_parent_map_xo_gpll0.

Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 drivers/clk/qcom/gcc-sdm660.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index 6394257ca8c0..597800ce1bdf 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -626,12 +626,12 @@ static struct clk_rcg2 hmss_gpll0_clk_src = {
 	.cmd_rcgr = 0x4805c,
 	.mnd_width = 0,
 	.hid_width = 5,
-	.parent_map = gcc_parent_map_xo_gpll0_gpll0_early_div,
+	.parent_map = gcc_parent_map_xo_gpll0,
 	.freq_tbl = ftbl_hmss_gpll0_clk_src,
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "hmss_gpll0_clk_src",
-		.parent_names = gcc_parent_names_xo_gpll0_gpll0_early_div,
-		.num_parents = 3,
+		.parent_names = gcc_parent_names_xo_gpll0,
+		.num_parents = 2,
 		.ops = &clk_rcg2_ops,
 	},
 };
-- 
2.30.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/6] clk: qcom: gcc-sdm660: Set HWCG bit to 1 on some clocks
  2021-02-20 15:56 [PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map Konrad Dybcio
@ 2021-02-20 15:56 ` Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 3/6] clk: qcom: gcc-sdm660: Add missing clocks and GDSCs Konrad Dybcio
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-20 15:56 UTC (permalink / raw)
  To: phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Rob Herring, Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk,
	linux-kernel, devicetree

According to the downstream kernel, some clocks need to have
HWCG disabled. Fix it.

Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 drivers/clk/qcom/gcc-sdm660.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index 597800ce1bdf..2bf4e29462e8 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -980,6 +980,8 @@ static struct clk_rcg2 usb3_phy_aux_clk_src = {
 static struct clk_branch gcc_aggre2_ufs_axi_clk = {
 	.halt_reg = 0x75034,
 	.halt_check = BRANCH_HALT,
+	.hwcg_reg = 0x75034,
+	.hwcg_bit = 1,
 	.clkr = {
 		.enable_reg = 0x75034,
 		.enable_mask = BIT(0),
@@ -1027,6 +1029,8 @@ static struct clk_branch gcc_bimc_gfx_clk = {
 static struct clk_branch gcc_bimc_hmss_axi_clk = {
 	.halt_reg = 0x48004,
 	.halt_check = BRANCH_HALT_VOTED,
+	.hwcg_reg = 0x48004,
+	.hwcg_bit = 1,
 	.clkr = {
 		.enable_reg = 0x52004,
 		.enable_mask = BIT(22),
@@ -1698,6 +1702,8 @@ static struct clk_branch gcc_mmss_noc_cfg_ahb_clk = {
 static struct clk_branch gcc_mmss_sys_noc_axi_clk = {
 	.halt_reg = 0x9000,
 	.halt_check = BRANCH_HALT,
+	.hwcg_reg = 0x9000,
+	.hwcg_bit = 1,
 	.clkr = {
 		.enable_reg = 0x9000,
 		.enable_mask = BIT(0),
@@ -1956,6 +1962,8 @@ static struct clk_branch gcc_ufs_ahb_clk = {
 static struct clk_branch gcc_ufs_axi_clk = {
 	.halt_reg = 0x75008,
 	.halt_check = BRANCH_HALT,
+	.hwcg_reg = 0x75008,
+	.hwcg_bit = 1,
 	.clkr = {
 		.enable_reg = 0x75008,
 		.enable_mask = BIT(0),
-- 
2.30.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/6] clk: qcom: gcc-sdm660: Add missing clocks and GDSCs
  2021-02-20 15:56 [PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 2/6] clk: qcom: gcc-sdm660: Set HWCG bit to 1 on some clocks Konrad Dybcio
@ 2021-02-20 15:56 ` Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 4/6] clk: qcom: gcc-sdm660: Remove gds_hw_ctrl from GDSCs Konrad Dybcio
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-20 15:56 UTC (permalink / raw)
  To: phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Rob Herring, Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk,
	linux-kernel, devicetree

Add missing clocks and GDSCs to make sure LPASS, UFS and
MSS can access their respective clock domains.

Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 drivers/clk/qcom/gcc-sdm660.c               | 158 ++++++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-sdm660.h |   6 +
 2 files changed, 164 insertions(+)

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index 2bf4e29462e8..05664d6b612a 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -1580,6 +1580,19 @@ static struct clk_branch gcc_gpu_cfg_ahb_clk = {
 	},
 };
 
+static struct clk_branch gcc_gpll0_out_msscc = {
+	.halt_reg = 0x5200c,
+	.halt_check = BRANCH_HALT_DELAY,
+	.clkr = {
+		.enable_reg = 0x5200c,
+		.enable_mask = BIT(2),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpll0_out_msscc",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_gpu_gpll0_clk = {
 	.halt_reg = 0x5200c,
 	.halt_check = BRANCH_HALT_DELAY,
@@ -1979,6 +1992,23 @@ static struct clk_branch gcc_ufs_axi_clk = {
 	},
 };
 
+static struct clk_branch gcc_ufs_axi_hw_ctl_clk = {
+	.halt_reg = 0x75008,
+	.clkr = {
+		.enable_reg = 0x75008,
+		.enable_mask = BIT(1),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_ufs_axi_hw_ctl_clk",
+			.parent_names = (const char *[]){
+				"gcc_ufs_axi_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_ufs_clkref_clk = {
 	.halt_reg = 0x88008,
 	.halt_check = BRANCH_HALT,
@@ -2010,6 +2040,23 @@ static struct clk_branch gcc_ufs_ice_core_clk = {
 	},
 };
 
+static struct clk_branch gcc_ufs_ice_core_hw_ctl_clk = {
+	.halt_reg = 0x7600c,
+	.clkr = {
+		.enable_reg = 0x7600c,
+		.enable_mask = BIT(1),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_ufs_ice_core_hw_ctl_clk",
+			.parent_names = (const char *[]){
+				"gcc_ufs_ice_core_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_ufs_phy_aux_clk = {
 	.halt_reg = 0x76040,
 	.halt_check = BRANCH_HALT,
@@ -2028,6 +2075,23 @@ static struct clk_branch gcc_ufs_phy_aux_clk = {
 	},
 };
 
+static struct clk_branch gcc_ufs_phy_aux_hw_ctl_clk = {
+	.halt_reg = 0x76040,
+	.clkr = {
+		.enable_reg = 0x76040,
+		.enable_mask = BIT(1),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_ufs_phy_aux_hw_ctl_clk",
+			.parent_names = (const char *[]){
+				"gcc_ufs_phy_aux_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
 	.halt_reg = 0x75014,
 	.halt_check = BRANCH_HALT_SKIP,
@@ -2085,6 +2149,23 @@ static struct clk_branch gcc_ufs_unipro_core_clk = {
 	},
 };
 
+static struct clk_branch gcc_ufs_unipro_core_hw_ctl_clk = {
+	.halt_reg = 0x76008,
+	.clkr = {
+		.enable_reg = 0x76008,
+		.enable_mask = BIT(1),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_ufs_unipro_core_hw_ctl_clk",
+			.parent_names = (const char *[]){
+				"gcc_ufs_unipro_core_clk",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_usb20_master_clk = {
 	.halt_reg = 0x2f004,
 	.halt_check = BRANCH_HALT,
@@ -2240,6 +2321,45 @@ static struct clk_branch gcc_usb_phy_cfg_ahb2phy_clk = {
 	},
 };
 
+static struct clk_branch hlos1_vote_lpass_adsp_smmu_clk = {
+	.halt_reg = 0x7d014,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x7d014,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "hlos1_vote_lpass_adsp_smmu_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch hlos1_vote_turing_adsp_smmu_clk = {
+	.halt_reg = 0x7d048,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x7d048,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "hlos1_vote_turing_adsp_smmu_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch hlos2_vote_turing_adsp_smmu_clk = {
+	.halt_reg = 0x7e048,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x7e048,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "hlos2_vote_turing_adsp_smmu_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct gdsc ufs_gdsc = {
 	.gdscr = 0x75004,
 	.gds_hw_ctrl = 0x0,
@@ -2270,6 +2390,33 @@ static struct gdsc pcie_0_gdsc = {
 	.flags = VOTABLE,
 };
 
+static struct gdsc hlos1_vote_lpass_adsp_gdsc = {
+	.gdscr = 0x7d034,
+	.pd = {
+		.name = "hlos1_vote_lpass_adsp",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
+};
+
+static struct gdsc hlos1_vote_turing_adsp_gdsc = {
+	.gdscr = 0x7d04c,
+	.pd = {
+		.name = "hlos1_vote_turing_adsp",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
+};
+
+static struct gdsc hlos2_vote_turing_adsp_gdsc = {
+	.gdscr = 0x7e04c,
+	.pd = {
+		.name = "hlos2_vote_turing_adsp",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
+};
+
 static struct clk_hw *gcc_sdm660_hws[] = {
 	&xo.hw,
 	&gpll0_early_div.hw,
@@ -2402,12 +2549,23 @@ static struct clk_regmap *gcc_sdm660_clocks[] = {
 	[USB30_MASTER_CLK_SRC] = &usb30_master_clk_src.clkr,
 	[USB30_MOCK_UTMI_CLK_SRC] = &usb30_mock_utmi_clk_src.clkr,
 	[USB3_PHY_AUX_CLK_SRC] = &usb3_phy_aux_clk_src.clkr,
+	[GPLL0_OUT_MSSCC] = &gcc_gpll0_out_msscc.clkr,
+	[GCC_UFS_AXI_HW_CTL_CLK] = &gcc_ufs_axi_hw_ctl_clk.clkr,
+	[GCC_UFS_ICE_CORE_HW_CTL_CLK] = &gcc_ufs_ice_core_hw_ctl_clk.clkr,
+	[GCC_UFS_PHY_AUX_HW_CTL_CLK] = &gcc_ufs_phy_aux_hw_ctl_clk.clkr,
+	[GCC_UFS_UNIPRO_CORE_HW_CTL_CLK] = &gcc_ufs_unipro_core_hw_ctl_clk.clkr,
+	[HLOS1_VOTE_LPASS_ADSP_SMMU_CLK] = &hlos1_vote_lpass_adsp_smmu_clk.clkr,
+	[HLOS1_VOTE_TURING_ADSP_SMMU_CLK] = &hlos1_vote_turing_adsp_smmu_clk.clkr,
+	[HLOS2_VOTE_TURING_ADSP_SMMU_CLK] = &hlos2_vote_turing_adsp_smmu_clk.clkr,
 };
 
 static struct gdsc *gcc_sdm660_gdscs[] = {
 	[UFS_GDSC] = &ufs_gdsc,
 	[USB_30_GDSC] = &usb_30_gdsc,
 	[PCIE_0_GDSC] = &pcie_0_gdsc,
+	[HLOS1_VOTE_LPASS_ADSP_GDSC] = &hlos1_vote_lpass_adsp_gdsc,
+	[HLOS1_VOTE_TURING_ADSP_GDSC] = &hlos1_vote_turing_adsp_gdsc,
+	[HLOS2_VOTE_TURING_ADSP_GDSC] = &hlos2_vote_turing_adsp_gdsc,
 };
 
 static const struct qcom_reset_map gcc_sdm660_resets[] = {
diff --git a/include/dt-bindings/clock/qcom,gcc-sdm660.h b/include/dt-bindings/clock/qcom,gcc-sdm660.h
index df8a6f3d367e..641f0f4d6139 100644
--- a/include/dt-bindings/clock/qcom,gcc-sdm660.h
+++ b/include/dt-bindings/clock/qcom,gcc-sdm660.h
@@ -138,10 +138,16 @@
 #define GCC_UFS_UNIPRO_CORE_HW_CTL_CLK		128
 #define GCC_RX0_USB2_CLKREF_CLK			129
 #define GCC_RX1_USB2_CLKREF_CLK			130
+#define HLOS1_VOTE_LPASS_ADSP_SMMU_CLK 131
+#define HLOS1_VOTE_TURING_ADSP_SMMU_CLK		132
+#define HLOS2_VOTE_TURING_ADSP_SMMU_CLK		133
 
 #define PCIE_0_GDSC	0
 #define UFS_GDSC	1
 #define USB_30_GDSC	2
+#define HLOS1_VOTE_LPASS_ADSP_GDSC		3
+#define HLOS1_VOTE_TURING_ADSP_GDSC		4
+#define HLOS2_VOTE_TURING_ADSP_GDSC		5
 
 #define GCC_QUSB2PHY_PRIM_BCR		0
 #define GCC_QUSB2PHY_SEC_BCR		1
-- 
2.30.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/6] clk: qcom: gcc-sdm660: Remove gds_hw_ctrl from GDSCs
  2021-02-20 15:56 [PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 2/6] clk: qcom: gcc-sdm660: Set HWCG bit to 1 on some clocks Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 3/6] clk: qcom: gcc-sdm660: Add missing clocks and GDSCs Konrad Dybcio
@ 2021-02-20 15:56 ` Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function Konrad Dybcio
  2021-02-20 15:56 ` [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable Konrad Dybcio
  4 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-20 15:56 UTC (permalink / raw)
  To: phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Rob Herring, Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk,
	linux-kernel, devicetree

These GDSCs do not support HW control, so remove the property.

Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 drivers/clk/qcom/gcc-sdm660.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index 05664d6b612a..bc8dfcd6d629 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -2362,7 +2362,6 @@ static struct clk_branch hlos2_vote_turing_adsp_smmu_clk = {
 
 static struct gdsc ufs_gdsc = {
 	.gdscr = 0x75004,
-	.gds_hw_ctrl = 0x0,
 	.pd = {
 		.name = "ufs_gdsc",
 	},
@@ -2372,7 +2371,6 @@ static struct gdsc ufs_gdsc = {
 
 static struct gdsc usb_30_gdsc = {
 	.gdscr = 0xf004,
-	.gds_hw_ctrl = 0x0,
 	.pd = {
 		.name = "usb_30_gdsc",
 	},
@@ -2382,7 +2380,6 @@ static struct gdsc usb_30_gdsc = {
 
 static struct gdsc pcie_0_gdsc = {
 	.gdscr = 0x6b004,
-	.gds_hw_ctrl = 0x0,
 	.pd = {
 		.name = "pcie_0_gdsc",
 	},
-- 
2.30.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function
  2021-02-20 15:56 [PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map Konrad Dybcio
                   ` (2 preceding siblings ...)
  2021-02-20 15:56 ` [PATCH 4/6] clk: qcom: gcc-sdm660: Remove gds_hw_ctrl from GDSCs Konrad Dybcio
@ 2021-02-20 15:56 ` Konrad Dybcio
  2021-02-23  0:39   ` Stephen Boyd
  2021-02-20 15:56 ` [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable Konrad Dybcio
  4 siblings, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-20 15:56 UTC (permalink / raw)
  To: phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Rob Herring, Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk,
	linux-kernel, devicetree

Downstream kernel executes a bunch of commands, such as keeping
GPU/MMSS interface clocks alive to make sure all subsystems can
work properly. Add these to make sure they do.

Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 drivers/clk/qcom/gcc-sdm660.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index bc8dfcd6d629..db2185c88b77 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/regmap.h>
 #include <linux/reset-controller.h>
@@ -2622,7 +2623,27 @@ static int gcc_sdm660_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap);
+	if (ret)
+		return ret;
+
+	/* Disable the GPLL0 active input to MMSS and GPU via MISC registers */
+	regmap_update_bits(regmap, 0x0902c, 0x3, 0x3);
+	regmap_update_bits(regmap, 0x71028, 0x3, 0x3);
+
+	/* This clock is used for all MMSSCC register access */
+	clk_prepare_enable(gcc_mmss_noc_cfg_ahb_clk.clkr.hw.clk);
+
+	/* This clock is used for all GPUCC register access */
+	clk_prepare_enable(gcc_gpu_cfg_ahb_clk.clkr.hw.clk);
+
+	/* Keep bimc gfx clock port on all the time */
+	clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk);
+
+	/* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */
+	clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000);
+
+	return ret;
 }
 
 static struct platform_driver gcc_sdm660_driver = {
-- 
2.30.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable
  2021-02-20 15:56 [PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map Konrad Dybcio
                   ` (3 preceding siblings ...)
  2021-02-20 15:56 ` [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function Konrad Dybcio
@ 2021-02-20 15:56 ` Konrad Dybcio
  2021-02-23  0:42   ` Stephen Boyd
  4 siblings, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-20 15:56 UTC (permalink / raw)
  To: phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Rob Herring, Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk,
	linux-kernel, devicetree

Some branch clocks should explicitly set this flag to make sure
they inherit their frequencies from the parent clock.

Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 drivers/clk/qcom/gcc-sdm660.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index db2185c88b77..2c182936fc09 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -1606,6 +1606,7 @@ static struct clk_branch gcc_gpu_gpll0_clk = {
 				"gpll0",
 			},
 			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -1623,6 +1624,7 @@ static struct clk_branch gcc_gpu_gpll0_div_clk = {
 				"gpll0_early_div",
 			},
 			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -1672,6 +1674,7 @@ static struct clk_branch gcc_mmss_gpll0_clk = {
 				"gpll0",
 			},
 			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -1689,6 +1692,7 @@ static struct clk_branch gcc_mmss_gpll0_div_clk = {
 				"gpll0_early_div",
 			},
 			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
 			.ops = &clk_branch2_ops,
 		},
 	},
-- 
2.30.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function
  2021-02-20 15:56 ` [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function Konrad Dybcio
@ 2021-02-23  0:39   ` Stephen Boyd
  2021-02-25 19:09     ` Konrad Dybcio
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2021-02-23  0:39 UTC (permalink / raw)
  To: Konrad Dybcio, phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Rob Herring,
	Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk, linux-kernel,
	devicetree

Quoting Konrad Dybcio (2021-02-20 07:56:16)
> Downstream kernel executes a bunch of commands, such as keeping
> GPU/MMSS interface clocks alive to make sure all subsystems can
> work properly. Add these to make sure they do.
> 
> Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> ---
>  drivers/clk/qcom/gcc-sdm660.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
> index bc8dfcd6d629..db2185c88b77 100644
> --- a/drivers/clk/qcom/gcc-sdm660.c
> +++ b/drivers/clk/qcom/gcc-sdm660.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/reset-controller.h>
> @@ -2622,7 +2623,27 @@ static int gcc_sdm660_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>  
> -       return qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap);
> +       ret = qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap);
> +       if (ret)
> +               return ret;
> +
> +       /* Disable the GPLL0 active input to MMSS and GPU via MISC registers */
> +       regmap_update_bits(regmap, 0x0902c, 0x3, 0x3);
> +       regmap_update_bits(regmap, 0x71028, 0x3, 0x3);
> +
> +       /* This clock is used for all MMSSCC register access */
> +       clk_prepare_enable(gcc_mmss_noc_cfg_ahb_clk.clkr.hw.clk);
> +
> +       /* This clock is used for all GPUCC register access */
> +       clk_prepare_enable(gcc_gpu_cfg_ahb_clk.clkr.hw.clk);
> +
> +       /* Keep bimc gfx clock port on all the time */
> +       clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk);
> +

Preferably just set these various bits with regmap_update_bits() during
probe. Also, please do it before regsitering the clks, not after.

> +       /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */
> +       clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000);

Is this not already the case?

> +
> +       return ret;
>  }
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable
  2021-02-20 15:56 ` [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable Konrad Dybcio
@ 2021-02-23  0:42   ` Stephen Boyd
  2021-02-25 19:27     ` Konrad Dybcio
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2021-02-23  0:42 UTC (permalink / raw)
  To: Konrad Dybcio, phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Konrad Dybcio,
	Andy Gross, Bjorn Andersson, Michael Turquette, Rob Herring,
	Taniya Das, Craig Tatlor, linux-arm-msm, linux-clk, linux-kernel,
	devicetree

Quoting Konrad Dybcio (2021-02-20 07:56:17)
> Some branch clocks should explicitly set this flag to make sure
> they inherit their frequencies from the parent clock.

This flag doesn't have anything to do with inheriting the rate from the
parent.

> 
> Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function
  2021-02-23  0:39   ` Stephen Boyd
@ 2021-02-25 19:09     ` Konrad Dybcio
  2021-04-01  1:53       ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-25 19:09 UTC (permalink / raw)
  To: Stephen Boyd, phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Andy Gross,
	Bjorn Andersson, Michael Turquette, Rob Herring, Taniya Das,
	Craig Tatlor, linux-arm-msm, linux-clk, linux-kernel, devicetree

Hi and sorry for the late reply,


>> +
>> +       /* Keep bimc gfx clock port on all the time */
>> +       clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk);
>> +
> Preferably just set these various bits with regmap_update_bits() during
> probe. Also, please do it before regsitering the clks, not after.

To be fair, now I think that simply adding CLK_IS_CRITICAL flag to the clocks in question is the smartest thing to do. Magic writes don't tell a whole lot.


>> +       /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */
>> +       clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000);
> Is this not already the case?


This is a mission-critical clock and we cannot trust the bootloader with setting it. Otherwise dragons might appear.


Konrad



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable
  2021-02-23  0:42   ` Stephen Boyd
@ 2021-02-25 19:27     ` Konrad Dybcio
  0 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2021-02-25 19:27 UTC (permalink / raw)
  To: Stephen Boyd, phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Andy Gross,
	Bjorn Andersson, Michael Turquette, Rob Herring, Taniya Das,
	Craig Tatlor, linux-arm-msm, linux-clk, linux-kernel, devicetree


On 23.02.2021 01:42, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2021-02-20 07:56:17)
>> Some branch clocks should explicitly set this flag to make sure
>> they inherit their frequencies from the parent clock.
> This flag doesn't have anything to do with inheriting the rate from the
> parent.
>
Right. "Some branch clocks should explicitly set this flag to make sure the frequency changes are propagated to their respective parents if need be." should sound better.


Konrad


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function
  2021-02-25 19:09     ` Konrad Dybcio
@ 2021-04-01  1:53       ` Stephen Boyd
  2021-04-01 21:10         ` Konrad Dybcio
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2021-04-01  1:53 UTC (permalink / raw)
  To: Konrad Dybcio, phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Andy Gross,
	Bjorn Andersson, Michael Turquette, Rob Herring, Taniya Das,
	Craig Tatlor, linux-arm-msm, linux-clk, linux-kernel, devicetree

Quoting Konrad Dybcio (2021-02-25 11:09:14)
> Hi and sorry for the late reply,
> 

I'm sorry too. This fell off my review queue for some time.

> 
> >> +
> >> +       /* Keep bimc gfx clock port on all the time */
> >> +       clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk);
> >> +
> > Preferably just set these various bits with regmap_update_bits() during
> > probe. Also, please do it before regsitering the clks, not after.
> 
> To be fair, now I think that simply adding CLK_IS_CRITICAL flag to the clocks in question is the smartest thing to do. Magic writes don't tell a whole lot.

This is how it's been done in various other qcom clk drivers. Usually
there is a comment about what is enabled, but really it's just setting
random bits that sadly aren't already set by default.

> 
> 
> >> +       /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */
> >> +       clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000);
> > Is this not already the case?
> 
> 
> This is a mission-critical clock and we cannot trust the bootloader with setting it. Otherwise dragons might appear.
> 

What does the bootloader set it to?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function
  2021-04-01  1:53       ` Stephen Boyd
@ 2021-04-01 21:10         ` Konrad Dybcio
  0 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2021-04-01 21:10 UTC (permalink / raw)
  To: Stephen Boyd, phone-devel
  Cc: ~postmarketos/upstreaming, martin.botka,
	angelogioacchino.delregno, marijn.suijten, Andy Gross,
	Bjorn Andersson, Michael Turquette, Rob Herring, Taniya Das,
	Craig Tatlor, linux-arm-msm, linux-clk, linux-kernel, devicetree


>>>> +
>>>> +       /* Keep bimc gfx clock port on all the time */
>>>> +       clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk);
>>>> +
>>> Preferably just set these various bits with regmap_update_bits() during
>>> probe. Also, please do it before regsitering the clks, not after.
>> To be fair, now I think that simply adding CLK_IS_CRITICAL flag to the clocks in question is the smartest thing to do. Magic writes don't tell a whole lot.
> This is how it's been done in various other qcom clk drivers. Usually
> there is a comment about what is enabled, but really it's just setting
> random bits that sadly aren't already set by default.

But why.. why should we give Linux less information about the hardware it's running on? It's a kernel after all, I know some parties would prefer to keep the hardware away from its users, but cmon, it's OSSLand out there!

Allocating a few bytes more in memory is proooobably a good trade-off for keeping an eye on the state of various clocks instead of simply setting some seemingly random bits and hoping nothing bad happens under the hood.. This isn't only a case for this clock, but for all ones that are effectively pinky-promise-trusted to function properly with no way of checking if they're even still alive other than poking the registers manually.. As of v5.12-rc2, there are *46* such trust credits..

It's NOT easy to track down issues in big monolithic kernels, especially when people submitting changes to common code seem to think that only their hardware uses it (no hard feelings, but drm/msm broke on !a6xx *at least* two times within the last year) and since making sure the clocks are ticking properly is one of the most crucial things when looking into more complex issues, which may or may not randomly happen on a platform that is just being brought up for various reasons (e.g. half of mdm9607 hardware doesn't wanna play along without a ICC driver), I *really* think we should use CLK_IS_CRITICAL instead and give developers a way to check if everything's in tact with the clock, while still keeping the "never turn it off, don't touch it!" aspect.


>>>> +       /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */
>>>> +       clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000);
>>> Is this not already the case?
>>
>> This is a mission-critical clock and we cannot trust the bootloader with setting it. Otherwise dragons might appear.
>>
> What does the bootloader set it to?

Sorry but I can't check, nor do I remember right now. But we still shouldn't add a variable that might come from a lazy OEM not incorporating the fix into their release, especially since this one is used for clocking the AP.


Konrad


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-20 15:56 [PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map Konrad Dybcio
2021-02-20 15:56 ` [PATCH 2/6] clk: qcom: gcc-sdm660: Set HWCG bit to 1 on some clocks Konrad Dybcio
2021-02-20 15:56 ` [PATCH 3/6] clk: qcom: gcc-sdm660: Add missing clocks and GDSCs Konrad Dybcio
2021-02-20 15:56 ` [PATCH 4/6] clk: qcom: gcc-sdm660: Remove gds_hw_ctrl from GDSCs Konrad Dybcio
2021-02-20 15:56 ` [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function Konrad Dybcio
2021-02-23  0:39   ` Stephen Boyd
2021-02-25 19:09     ` Konrad Dybcio
2021-04-01  1:53       ` Stephen Boyd
2021-04-01 21:10         ` Konrad Dybcio
2021-02-20 15:56 ` [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable Konrad Dybcio
2021-02-23  0:42   ` Stephen Boyd
2021-02-25 19:27     ` Konrad Dybcio

Phone-Devel Archive on lore.kernel.org.

Archives are clonable:
	git clone --mirror https://lore.kernel.org/phone-devel/0 phone-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 phone-devel phone-devel/ https://lore.kernel.org/phone-devel \
		phone-devel@vger.kernel.org
	public-inbox-index phone-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.phone-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git