[PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map

[PATCH 1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map

[Konrad Dybcio]

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

[PATCH 2/6] clk: qcom: gcc-sdm660: Set HWCG bit to 1 on some clocks

[Konrad Dybcio]

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

[PATCH 3/6] clk: qcom: gcc-sdm660: Add missing clocks and GDSCs

[Konrad Dybcio]

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

[PATCH 4/6] clk: qcom: gcc-sdm660: Remove gds_hw_ctrl from GDSCs

[Konrad Dybcio]

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

[PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Konrad Dybcio]

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

Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Stephen Boyd]

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

Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Konrad Dybcio]

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

Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Stephen Boyd]

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?

Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Konrad Dybcio]

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

Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Konrad Dybcio]

Hi,


I am aware that the comments I left are highly controversial and a bit emotional, but I'd still like to get a response. It's been almost two months and I have phones on the desk waiting for things to be merged, so that I can develop more things.


P.S. I still stand by my stance that the more info we can get about what's going on in the black boxes that we are not allowed to get docs for, the better for us, developers.


Konrad

Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Stephen Boyd]

Quoting Konrad Dybcio (2021-05-26 12:11:13)
> Hi,
> 
> 
> I am aware that the comments I left are highly controversial and a bit emotional, but I'd still like to get a response. It's been almost two months and I have phones on the desk waiting for things to be merged, so that I can develop more things.
> 
> 
> P.S. I still stand by my stance that the more info we can get about what's going on in the black boxes that we are not allowed to get docs for, the better for us, developers.
> 

I marked this patch as "changes requested". Please make the changes
requested.

[PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable

[Konrad Dybcio]

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

Re: [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable

[Stephen Boyd]

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>

Re: [PATCH 6/6] clk: qcom: gcc-sdm660: Add CLK_SET_RATE_PARENT where applicable

[Konrad Dybcio]

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