[PATCH RESEND 0/3] MSM8996 Pro CBF scaling support

[PATCH RESEND 0/3] MSM8996 Pro CBF scaling support

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

MSM8996 Pro has a /4 post divisor on its CBF PLL instead of /2, allowing
it to reach 192000000Hz on the lowest CPU OPPs (compared to 307200000Hz
on MSM8996). Add a compatible string to differentiate between the two and
handle the different divisor in the driver. Finally, add peak bandwidths
to CPU OPPs in msm8996pro.dtsi.

This series depends on the following patch series:
  clk: qcom: msm8996: add support for the CBF clock
  https://lore.kernel.org/linux-arm-msm/20230410200014.432418-1-dmitry.baryshkov@linaro.org/

Yassine Oudjana (3):
  dt-bindings: clock: qcom,msm8996-cbf: Add compatible for MSM8996 Pro
  arm64: dts: qcom: msm8996pro: Add CBF scaling support
  clk: qcom: cbf-msm8996: Add support for MSM8996 Pro

 .../bindings/clock/qcom,msm8996-cbf.yaml      |   4 +-
 arch/arm64/boot/dts/qcom/msm8996pro.dtsi      |  51 ++++++++
 drivers/clk/qcom/clk-cbf-8996.c               | 121 +++++++++++++++---
 3 files changed, 160 insertions(+), 16 deletions(-)

-- 
2.40.0

[PATCH RESEND 1/3] dt-bindings: clock: qcom,msm8996-cbf: Add compatible for MSM8996 Pro

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

The CBF clock on MSM8996 Pro has a different divisor compared to MSM8996
and is therefore not fully compatible with it. Add a new compatible string
to differentiate between them.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
index 3ffe69d8cdd5..0dfbd8c4d465 100644
--- a/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
@@ -15,7 +15,9 @@ description: >
 
 properties:
   compatible:
-    const: qcom,msm8996-cbf
+    enum:
+      - qcom,msm8996-cbf
+      - qcom,msm8996pro-cbf
 
   reg:
     maxItems: 1
-- 
2.40.0

Re: [PATCH RESEND 1/3] dt-bindings: clock: qcom,msm8996-cbf: Add compatible for MSM8996 Pro

[Krzysztof Kozlowski]

On 03/05/2023 15:00, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> The CBF clock on MSM8996 Pro has a different divisor compared to MSM8996
> and is therefore not fully compatible with it. Add a new compatible string
> to differentiate between them.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>  Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

Best regards,
Krzysztof

[PATCH RESEND 2/3] arm64: dts: qcom: msm8996pro: Add CBF scaling support

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

Add opp-peak-kBps to CPU OPPs to allow for CBF scaling, and change the
CBF compatible to reflect the difference between it and the one on MSM8996.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 arch/arm64/boot/dts/qcom/msm8996pro.dtsi | 51 ++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996pro.dtsi b/arch/arm64/boot/dts/qcom/msm8996pro.dtsi
index a679a9c0cf99..b74cff06f300 100644
--- a/arch/arm64/boot/dts/qcom/msm8996pro.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996pro.dtsi
@@ -24,101 +24,121 @@ opp-307200000 {
 			opp-hz = /bits/ 64 <307200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-384000000 {
 			opp-hz = /bits/ 64 <384000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-460800000 {
 			opp-hz = /bits/ 64 <460800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-537600000 {
 			opp-hz = /bits/ 64 <537600000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-614400000 {
 			opp-hz = /bits/ 64 <614400000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-691200000 {
 			opp-hz = /bits/ 64 <691200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-768000000 {
 			opp-hz = /bits/ 64 <768000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-844800000 {
 			opp-hz = /bits/ 64 <844800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <384000>;
 		};
 		opp-902400000 {
 			opp-hz = /bits/ 64 <902400000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <441600>;
 		};
 		opp-979200000 {
 			opp-hz = /bits/ 64 <979200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <537600>;
 		};
 		opp-1056000000 {
 			opp-hz = /bits/ 64 <1056000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <614400>;
 		};
 		opp-1132800000 {
 			opp-hz = /bits/ 64 <1132800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <691200>;
 		};
 		opp-1209600000 {
 			opp-hz = /bits/ 64 <1209600000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <768000>;
 		};
 		opp-1286400000 {
 			opp-hz = /bits/ 64 <1286400000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <844800>;
 		};
 		opp-1363200000 {
 			opp-hz = /bits/ 64 <1363200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <902400>;
 		};
 		opp-1440000000 {
 			opp-hz = /bits/ 64 <1440000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <979200>;
 		};
 		opp-1516800000 {
 			opp-hz = /bits/ 64 <1516800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1132800>;
 		};
 		opp-1593600000 {
 			opp-hz = /bits/ 64 <1593600000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1190400>;
 		};
 		opp-1996800000 {
 			opp-hz = /bits/ 64 <1996800000>;
 			opp-supported-hw = <0x20>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1516800>;
 		};
 		opp-2188800000 {
 			opp-hz = /bits/ 64 <2188800000>;
 			opp-supported-hw = <0x10>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1593600>;
 		};
 	};
 
@@ -131,136 +151,163 @@ opp-307200000 {
 			opp-hz = /bits/ 64 <307200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-384000000 {
 			opp-hz = /bits/ 64 <384000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-460800000 {
 			opp-hz = /bits/ 64 <460800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-537600000 {
 			opp-hz = /bits/ 64 <537600000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-614400000 {
 			opp-hz = /bits/ 64 <614400000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <192000>;
 		};
 		opp-691200000 {
 			opp-hz = /bits/ 64 <691200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-748800000 {
 			opp-hz = /bits/ 64 <748800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-825600000 {
 			opp-hz = /bits/ 64 <825600000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <384000>;
 		};
 		opp-902400000 {
 			opp-hz = /bits/ 64 <902400000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <441600>;
 		};
 		opp-979200000 {
 			opp-hz = /bits/ 64 <979200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <441600>;
 		};
 		opp-1056000000 {
 			opp-hz = /bits/ 64 <1056000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <537600>;
 		};
 		opp-1132800000 {
 			opp-hz = /bits/ 64 <1132800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <614400>;
 		};
 		opp-1209600000 {
 			opp-hz = /bits/ 64 <1209600000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <691200>;
 		};
 		opp-1286400000 {
 			opp-hz = /bits/ 64 <1286400000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <768000>;
 		};
 		opp-1363200000 {
 			opp-hz = /bits/ 64 <1363200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <844800>;
 		};
 		opp-1440000000 {
 			opp-hz = /bits/ 64 <1440000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <902400>;
 		};
 		opp-1516800000 {
 			opp-hz = /bits/ 64 <1516800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <979200>;
 		};
 		opp-1593600000 {
 			opp-hz = /bits/ 64 <1593600000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1056000>;
 		};
 		opp-1670400000 {
 			opp-hz = /bits/ 64 <1670400000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1132800>;
 		};
 		opp-1747200000 {
 			opp-hz = /bits/ 64 <1747200000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1190400>;
 		};
 		opp-1824000000 {
 			opp-hz = /bits/ 64 <1824000000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1286400>;
 		};
 		opp-1900800000 {
 			opp-hz = /bits/ 64 <1900800000>;
 			opp-supported-hw = <0x70>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1363200>;
 		};
 		opp-1977600000 {
 			opp-hz = /bits/ 64 <1977600000>;
 			opp-supported-hw = <0x30>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1440000>;
 		};
 		opp-2054400000 {
 			opp-hz = /bits/ 64 <2054400000>;
 			opp-supported-hw = <0x30>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1516800>;
 		};
 		opp-2150400000 {
 			opp-hz = /bits/ 64 <2150400000>;
 			opp-supported-hw = <0x30>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1593600>;
 		};
 		opp-2246400000 {
 			opp-hz = /bits/ 64 <2246400000>;
 			opp-supported-hw = <0x10>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1593600>;
 		};
 		opp-2342400000 {
 			opp-hz = /bits/ 64 <2342400000>;
 			opp-supported-hw = <0x10>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1593600>;
 		};
 	};
 };
@@ -289,3 +336,7 @@ opp-560000000 {
 	};
 	/* The rest is inherited from msm8996 */
 };
+
+&cbf {
+	compatible = "qcom,msm8996pro-cbf";
+};
-- 
2.40.0

Re: [PATCH RESEND 2/3] arm64: dts: qcom: msm8996pro: Add CBF scaling support

[Dmitry Baryshkov]

On Wed, 3 May 2023 at 16:02, Yassine Oudjana <yassine.oudjana@gmail.com> wrote:
>
> From: Yassine Oudjana <y.oudjana@protonmail.com>
>
> Add opp-peak-kBps to CPU OPPs to allow for CBF scaling, and change the
> CBF compatible to reflect the difference between it and the one on MSM8996.
>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>  arch/arm64/boot/dts/qcom/msm8996pro.dtsi | 51 ++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH RESEND 3/3] clk: qcom: cbf-msm8996: Add support for MSM8996 Pro

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

The CBF PLL on MSM8996 Pro has a /4 post divisor instead of /2. Handle the
difference accordingly.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 drivers/clk/qcom/clk-cbf-8996.c | 121 ++++++++++++++++++++++++++++----
 1 file changed, 106 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
index 1bb2cd956d68..a3e96578ddd9 100644
--- a/drivers/clk/qcom/clk-cbf-8996.c
+++ b/drivers/clk/qcom/clk-cbf-8996.c
@@ -65,6 +65,19 @@ static const struct alpha_pll_config cbfpll_config = {
 	.early_output_mask = BIT(3),
 };
 
+static const struct alpha_pll_config cbfpll_pro_config = {
+	.l = 72,
+	.config_ctl_val = 0x200d4828,
+	.config_ctl_hi_val = 0x006,
+	.test_ctl_val = 0x1c000000,
+	.test_ctl_hi_val = 0x00004000,
+	.pre_div_mask = BIT(12),
+	.post_div_mask = 0x3 << 8,
+	.post_div_val = 0x3 << 8,
+	.main_output_mask = BIT(0),
+	.early_output_mask = BIT(3),
+};
+
 static struct clk_alpha_pll cbf_pll = {
 	.offset = CBF_PLL_OFFSET,
 	.regs = cbf_pll_regs,
@@ -93,6 +106,20 @@ static struct clk_fixed_factor cbf_pll_postdiv = {
 	},
 };
 
+static struct clk_fixed_factor cbf_pro_pll_postdiv = {
+	.mult = 1,
+	.div = 4,
+	.hw.init = &(struct clk_init_data){
+		.name = "cbf_pll_postdiv",
+		.parent_hws = (const struct clk_hw*[]){
+			&cbf_pll.clkr.hw
+		},
+		.num_parents = 1,
+		.ops = &clk_fixed_factor_ops,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
 static const struct clk_parent_data cbf_mux_parent_data[] = {
 	{ .index = DT_XO },
 	{ .hw = &cbf_pll.clkr.hw },
@@ -100,6 +127,13 @@ static const struct clk_parent_data cbf_mux_parent_data[] = {
 	{ .index = DT_APCS_AUX },
 };
 
+static const struct clk_parent_data cbf_pro_mux_parent_data[] = {
+	{ .index = DT_XO },
+	{ .hw = &cbf_pll.clkr.hw },
+	{ .hw = &cbf_pro_pll_postdiv.hw },
+	{ .index = DT_APCS_AUX },
+};
+
 struct clk_cbf_8996_mux {
 	u32 reg;
 	struct notifier_block nb;
@@ -140,12 +174,14 @@ static int clk_cbf_8996_mux_determine_rate(struct clk_hw *hw,
 					   struct clk_rate_request *req)
 {
 	struct clk_hw *parent;
+	struct clk_hw *post_div_hw = clk_hw_get_parent_by_index(hw, CBF_DIV_INDEX);
+	struct clk_fixed_factor *post_div = to_clk_fixed_factor(post_div_hw);
 
-	if (req->rate < (DIV_THRESHOLD / 2))
+	if (req->rate < (DIV_THRESHOLD / post_div->div))
 		return -EINVAL;
 
 	if (req->rate < DIV_THRESHOLD)
-		parent = clk_hw_get_parent_by_index(hw, CBF_DIV_INDEX);
+		parent = post_div_hw;
 	else
 		parent = clk_hw_get_parent_by_index(hw, CBF_PLL_INDEX);
 
@@ -177,10 +213,24 @@ static struct clk_cbf_8996_mux cbf_mux = {
 	},
 };
 
+static struct clk_cbf_8996_mux cbf_pro_mux = {
+	.reg = CBF_MUX_OFFSET,
+	.nb.notifier_call = cbf_clk_notifier_cb,
+	.clkr.hw.init = &(struct clk_init_data) {
+		.name = "cbf_mux",
+		.parent_data = cbf_pro_mux_parent_data,
+		.num_parents = ARRAY_SIZE(cbf_pro_mux_parent_data),
+		.ops = &clk_cbf_8996_mux_ops,
+		/* CPU clock is critical and should never be gated */
+		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
+	},
+};
+
 static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
 			       void *data)
 {
 	struct clk_notifier_data *cnd = data;
+	struct clk_hw *hw = __clk_get_hw(cnd->clk);
 
 	switch (event) {
 	case PRE_RATE_CHANGE:
@@ -188,19 +238,19 @@ static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
 		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
 		 * to bottom, so it will change the rate of the PLL before
 		 * chaging the parent of PMUX. This can result in pmux getting
-		 * clocked twice the expected rate.
+		 * clocked twice (or 4 times) the expected rate.
 		 *
-		 * Manually switch to PLL/2 here.
+		 * Manually switch to PLL/DIV here.
 		 */
 		if (cnd->old_rate > DIV_THRESHOLD &&
 		    cnd->new_rate < DIV_THRESHOLD)
-			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_DIV_INDEX);
+			clk_cbf_8996_mux_set_parent(hw, CBF_DIV_INDEX);
 		break;
 	case ABORT_RATE_CHANGE:
 		/* Revert manual change */
 		if (cnd->new_rate < DIV_THRESHOLD &&
 		    cnd->old_rate > DIV_THRESHOLD)
-			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_PLL_INDEX);
+			clk_cbf_8996_mux_set_parent(hw, CBF_PLL_INDEX);
 		break;
 	default:
 		break;
@@ -213,11 +263,50 @@ static struct clk_hw *cbf_msm8996_hw_clks[] = {
 	&cbf_pll_postdiv.hw,
 };
 
+static struct clk_hw *cbf_msm8996pro_hw_clks[] = {
+	&cbf_pro_pll_postdiv.hw,
+};
+
 static struct clk_regmap *cbf_msm8996_clks[] = {
 	&cbf_pll.clkr,
 	&cbf_mux.clkr,
 };
 
+static struct clk_regmap *cbf_msm8996pro_clks[] = {
+	&cbf_pll.clkr,
+	&cbf_pro_mux.clkr,
+};
+
+struct cbf_match_data {
+	const struct alpha_pll_config *config;
+	struct clk_fixed_factor *cbf_pll_postdiv;
+	struct clk_cbf_8996_mux *cbf_mux;
+	struct clk_hw **hw_clks;
+	size_t nr_hw_clks;
+	struct clk_regmap **clks;
+	size_t nr_clks;
+};
+
+static const struct cbf_match_data cbf_msm8996_match_data = {
+	.config = &cbfpll_config,
+	.cbf_pll_postdiv = &cbf_pll_postdiv,
+	.cbf_mux = &cbf_mux,
+	.hw_clks = cbf_msm8996_hw_clks,
+	.nr_hw_clks = ARRAY_SIZE(cbf_msm8996_hw_clks),
+	.clks = cbf_msm8996_clks,
+	.nr_clks = ARRAY_SIZE(cbf_msm8996_clks)
+};
+
+static const struct cbf_match_data cbf_msm8996pro_match_data = {
+	.config = &cbfpll_pro_config,
+	.cbf_pll_postdiv = &cbf_pro_pll_postdiv,
+	.cbf_mux = &cbf_pro_mux,
+	.hw_clks = cbf_msm8996pro_hw_clks,
+	.nr_hw_clks = ARRAY_SIZE(cbf_msm8996pro_hw_clks),
+	.clks = cbf_msm8996pro_clks,
+	.nr_clks = ARRAY_SIZE(cbf_msm8996pro_clks)
+};
+
 static const struct regmap_config cbf_msm8996_regmap_config = {
 	.reg_bits		= 32,
 	.reg_stride		= 4,
@@ -274,6 +363,7 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
 	void __iomem *base;
 	struct regmap *regmap;
 	struct device *dev = &pdev->dev;
+	const struct cbf_match_data *data = of_device_get_match_data(dev);
 	int i, ret;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
@@ -295,7 +385,7 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
 			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
 			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
 
-	clk_alpha_pll_configure(&cbf_pll, regmap, &cbfpll_config);
+	clk_alpha_pll_configure(&cbf_pll, regmap, data->config);
 
 	/* Wait for PLL(s) to lock */
 	udelay(50);
@@ -311,27 +401,27 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
 	/* Switch CBF to use the primary PLL */
 	regmap_update_bits(regmap, CBF_MUX_OFFSET, CBF_MUX_PARENT_MASK, 0x1);
 
-	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_hw_clks); i++) {
-		ret = devm_clk_hw_register(dev, cbf_msm8996_hw_clks[i]);
+	for (i = 0; i < data->nr_hw_clks; i++) {
+		ret = devm_clk_hw_register(dev, data->hw_clks[i]);
 		if (ret)
 			return ret;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_clks); i++) {
-		ret = devm_clk_register_regmap(dev, cbf_msm8996_clks[i]);
+	for (i = 0; i < data->nr_clks; i++) {
+		ret = devm_clk_register_regmap(dev, data->clks[i]);
 		if (ret)
 			return ret;
 	}
 
-	ret = devm_clk_notifier_register(dev, cbf_mux.clkr.hw.clk, &cbf_mux.nb);
+	ret = devm_clk_notifier_register(dev, data->cbf_mux->clkr.hw.clk, &data->cbf_mux->nb);
 	if (ret)
 		return ret;
 
-	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &data->cbf_mux->clkr.hw);
 	if (ret)
 		return ret;
 
-	return qcom_msm8996_cbf_icc_register(pdev, &cbf_mux.clkr.hw);
+	return qcom_msm8996_cbf_icc_register(pdev, &data->cbf_mux->clkr.hw);
 }
 
 static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
@@ -340,7 +430,8 @@ static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
-	{ .compatible = "qcom,msm8996-cbf" },
+	{ .compatible = "qcom,msm8996-cbf", .data = &cbf_msm8996_match_data },
+	{ .compatible = "qcom,msm8996pro-cbf", .data = &cbf_msm8996pro_match_data },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
-- 
2.40.0

Re: [PATCH RESEND 3/3] clk: qcom: cbf-msm8996: Add support for MSM8996 Pro

[Dmitry Baryshkov]

On Wed, 3 May 2023 at 16:02, Yassine Oudjana <yassine.oudjana@gmail.com> wrote:
>
> From: Yassine Oudjana <y.oudjana@protonmail.com>
>
> The CBF PLL on MSM8996 Pro has a /4 post divisor instead of /2. Handle the
> difference accordingly.
>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>  drivers/clk/qcom/clk-cbf-8996.c | 121 ++++++++++++++++++++++++++++----
>  1 file changed, 106 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
> index 1bb2cd956d68..a3e96578ddd9 100644
> --- a/drivers/clk/qcom/clk-cbf-8996.c
> +++ b/drivers/clk/qcom/clk-cbf-8996.c
> @@ -65,6 +65,19 @@ static const struct alpha_pll_config cbfpll_config = {
>         .early_output_mask = BIT(3),
>  };
>
> +static const struct alpha_pll_config cbfpll_pro_config = {
> +       .l = 72,
> +       .config_ctl_val = 0x200d4828,
> +       .config_ctl_hi_val = 0x006,
> +       .test_ctl_val = 0x1c000000,
> +       .test_ctl_hi_val = 0x00004000,
> +       .pre_div_mask = BIT(12),
> +       .post_div_mask = 0x3 << 8,
> +       .post_div_val = 0x3 << 8,
> +       .main_output_mask = BIT(0),
> +       .early_output_mask = BIT(3),
> +};

Granted that the difference between this and the non-pro is just the
post_div_val, would it be easier to just patch it in the probe()?

> +
>  static struct clk_alpha_pll cbf_pll = {
>         .offset = CBF_PLL_OFFSET,
>         .regs = cbf_pll_regs,
> @@ -93,6 +106,20 @@ static struct clk_fixed_factor cbf_pll_postdiv = {
>         },
>  };
>
> +static struct clk_fixed_factor cbf_pro_pll_postdiv = {
> +       .mult = 1,
> +       .div = 4,
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cbf_pll_postdiv",
> +               .parent_hws = (const struct clk_hw*[]){
> +                       &cbf_pll.clkr.hw
> +               },
> +               .num_parents = 1,
> +               .ops = &clk_fixed_factor_ops,
> +               .flags = CLK_SET_RATE_PARENT,
> +       },
> +};

Same question here.

Then we can get rid of all the duplication below.

> +
>  static const struct clk_parent_data cbf_mux_parent_data[] = {
>         { .index = DT_XO },
>         { .hw = &cbf_pll.clkr.hw },
> @@ -100,6 +127,13 @@ static const struct clk_parent_data cbf_mux_parent_data[] = {
>         { .index = DT_APCS_AUX },
>  };
>
> +static const struct clk_parent_data cbf_pro_mux_parent_data[] = {
> +       { .index = DT_XO },
> +       { .hw = &cbf_pll.clkr.hw },
> +       { .hw = &cbf_pro_pll_postdiv.hw },
> +       { .index = DT_APCS_AUX },
> +};
> +
>  struct clk_cbf_8996_mux {
>         u32 reg;
>         struct notifier_block nb;
> @@ -140,12 +174,14 @@ static int clk_cbf_8996_mux_determine_rate(struct clk_hw *hw,
>                                            struct clk_rate_request *req)
>  {
>         struct clk_hw *parent;
> +       struct clk_hw *post_div_hw = clk_hw_get_parent_by_index(hw, CBF_DIV_INDEX);
> +       struct clk_fixed_factor *post_div = to_clk_fixed_factor(post_div_hw);
>
> -       if (req->rate < (DIV_THRESHOLD / 2))
> +       if (req->rate < (DIV_THRESHOLD / post_div->div))
>                 return -EINVAL;
>
>         if (req->rate < DIV_THRESHOLD)
> -               parent = clk_hw_get_parent_by_index(hw, CBF_DIV_INDEX);
> +               parent = post_div_hw;
>         else
>                 parent = clk_hw_get_parent_by_index(hw, CBF_PLL_INDEX);
>
> @@ -177,10 +213,24 @@ static struct clk_cbf_8996_mux cbf_mux = {
>         },
>  };
>
> +static struct clk_cbf_8996_mux cbf_pro_mux = {
> +       .reg = CBF_MUX_OFFSET,
> +       .nb.notifier_call = cbf_clk_notifier_cb,
> +       .clkr.hw.init = &(struct clk_init_data) {
> +               .name = "cbf_mux",
> +               .parent_data = cbf_pro_mux_parent_data,
> +               .num_parents = ARRAY_SIZE(cbf_pro_mux_parent_data),
> +               .ops = &clk_cbf_8996_mux_ops,
> +               /* CPU clock is critical and should never be gated */
> +               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> +       },
> +};
> +
>  static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>                                void *data)
>  {
>         struct clk_notifier_data *cnd = data;
> +       struct clk_hw *hw = __clk_get_hw(cnd->clk);
>
>         switch (event) {
>         case PRE_RATE_CHANGE:
> @@ -188,19 +238,19 @@ static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>                  * Avoid overvolting. clk_core_set_rate_nolock() walks from top
>                  * to bottom, so it will change the rate of the PLL before
>                  * chaging the parent of PMUX. This can result in pmux getting
> -                * clocked twice the expected rate.
> +                * clocked twice (or 4 times) the expected rate.
>                  *
> -                * Manually switch to PLL/2 here.
> +                * Manually switch to PLL/DIV here.
>                  */
>                 if (cnd->old_rate > DIV_THRESHOLD &&
>                     cnd->new_rate < DIV_THRESHOLD)
> -                       clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_DIV_INDEX);
> +                       clk_cbf_8996_mux_set_parent(hw, CBF_DIV_INDEX);
>                 break;
>         case ABORT_RATE_CHANGE:
>                 /* Revert manual change */
>                 if (cnd->new_rate < DIV_THRESHOLD &&
>                     cnd->old_rate > DIV_THRESHOLD)
> -                       clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_PLL_INDEX);
> +                       clk_cbf_8996_mux_set_parent(hw, CBF_PLL_INDEX);
>                 break;
>         default:
>                 break;
> @@ -213,11 +263,50 @@ static struct clk_hw *cbf_msm8996_hw_clks[] = {
>         &cbf_pll_postdiv.hw,
>  };
>
> +static struct clk_hw *cbf_msm8996pro_hw_clks[] = {
> +       &cbf_pro_pll_postdiv.hw,
> +};
> +
>  static struct clk_regmap *cbf_msm8996_clks[] = {
>         &cbf_pll.clkr,
>         &cbf_mux.clkr,
>  };
>
> +static struct clk_regmap *cbf_msm8996pro_clks[] = {
> +       &cbf_pll.clkr,
> +       &cbf_pro_mux.clkr,
> +};
> +
> +struct cbf_match_data {
> +       const struct alpha_pll_config *config;
> +       struct clk_fixed_factor *cbf_pll_postdiv;
> +       struct clk_cbf_8996_mux *cbf_mux;
> +       struct clk_hw **hw_clks;
> +       size_t nr_hw_clks;
> +       struct clk_regmap **clks;
> +       size_t nr_clks;
> +};
> +
> +static const struct cbf_match_data cbf_msm8996_match_data = {
> +       .config = &cbfpll_config,
> +       .cbf_pll_postdiv = &cbf_pll_postdiv,
> +       .cbf_mux = &cbf_mux,
> +       .hw_clks = cbf_msm8996_hw_clks,
> +       .nr_hw_clks = ARRAY_SIZE(cbf_msm8996_hw_clks),
> +       .clks = cbf_msm8996_clks,
> +       .nr_clks = ARRAY_SIZE(cbf_msm8996_clks)
> +};
> +
> +static const struct cbf_match_data cbf_msm8996pro_match_data = {
> +       .config = &cbfpll_pro_config,
> +       .cbf_pll_postdiv = &cbf_pro_pll_postdiv,
> +       .cbf_mux = &cbf_pro_mux,
> +       .hw_clks = cbf_msm8996pro_hw_clks,
> +       .nr_hw_clks = ARRAY_SIZE(cbf_msm8996pro_hw_clks),
> +       .clks = cbf_msm8996pro_clks,
> +       .nr_clks = ARRAY_SIZE(cbf_msm8996pro_clks)
> +};
> +
>  static const struct regmap_config cbf_msm8996_regmap_config = {
>         .reg_bits               = 32,
>         .reg_stride             = 4,
> @@ -274,6 +363,7 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>         void __iomem *base;
>         struct regmap *regmap;
>         struct device *dev = &pdev->dev;
> +       const struct cbf_match_data *data = of_device_get_match_data(dev);
>         int i, ret;
>
>         base = devm_platform_ioremap_resource(pdev, 0);
> @@ -295,7 +385,7 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>                            CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
>                            CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
>
> -       clk_alpha_pll_configure(&cbf_pll, regmap, &cbfpll_config);
> +       clk_alpha_pll_configure(&cbf_pll, regmap, data->config);
>
>         /* Wait for PLL(s) to lock */
>         udelay(50);
> @@ -311,27 +401,27 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>         /* Switch CBF to use the primary PLL */
>         regmap_update_bits(regmap, CBF_MUX_OFFSET, CBF_MUX_PARENT_MASK, 0x1);
>
> -       for (i = 0; i < ARRAY_SIZE(cbf_msm8996_hw_clks); i++) {
> -               ret = devm_clk_hw_register(dev, cbf_msm8996_hw_clks[i]);
> +       for (i = 0; i < data->nr_hw_clks; i++) {
> +               ret = devm_clk_hw_register(dev, data->hw_clks[i]);
>                 if (ret)
>                         return ret;
>         }
>
> -       for (i = 0; i < ARRAY_SIZE(cbf_msm8996_clks); i++) {
> -               ret = devm_clk_register_regmap(dev, cbf_msm8996_clks[i]);
> +       for (i = 0; i < data->nr_clks; i++) {
> +               ret = devm_clk_register_regmap(dev, data->clks[i]);
>                 if (ret)
>                         return ret;
>         }
>
> -       ret = devm_clk_notifier_register(dev, cbf_mux.clkr.hw.clk, &cbf_mux.nb);
> +       ret = devm_clk_notifier_register(dev, data->cbf_mux->clkr.hw.clk, &data->cbf_mux->nb);
>         if (ret)
>                 return ret;
>
> -       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
> +       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &data->cbf_mux->clkr.hw);
>         if (ret)
>                 return ret;
>
> -       return qcom_msm8996_cbf_icc_register(pdev, &cbf_mux.clkr.hw);
> +       return qcom_msm8996_cbf_icc_register(pdev, &data->cbf_mux->clkr.hw);
>  }
>
>  static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
> @@ -340,7 +430,8 @@ static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
>  }
>
>  static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
> -       { .compatible = "qcom,msm8996-cbf" },
> +       { .compatible = "qcom,msm8996-cbf", .data = &cbf_msm8996_match_data },
> +       { .compatible = "qcom,msm8996pro-cbf", .data = &cbf_msm8996pro_match_data },
>         { /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
> --
> 2.40.0
>


-- 
With best wishes
Dmitry

Re: [PATCH RESEND 3/3] clk: qcom: cbf-msm8996: Add support for MSM8996 Pro

[Yassine Oudjana]

On Wed, May 3 2023 at 04:20:00 PM +03:00:00, Dmitry Baryshkov 
<dmitry.baryshkov@linaro.org> wrote:
> On Wed, 3 May 2023 at 16:02, Yassine Oudjana 
> <yassine.oudjana@gmail.com> wrote:
>> 
>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>>  The CBF PLL on MSM8996 Pro has a /4 post divisor instead of /2. 
>> Handle the
>>  difference accordingly.
>> 
>>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>  ---
>>   drivers/clk/qcom/clk-cbf-8996.c | 121 
>> ++++++++++++++++++++++++++++----
>>   1 file changed, 106 insertions(+), 15 deletions(-)
>> 
>>  diff --git a/drivers/clk/qcom/clk-cbf-8996.c 
>> b/drivers/clk/qcom/clk-cbf-8996.c
>>  index 1bb2cd956d68..a3e96578ddd9 100644
>>  --- a/drivers/clk/qcom/clk-cbf-8996.c
>>  +++ b/drivers/clk/qcom/clk-cbf-8996.c
>>  @@ -65,6 +65,19 @@ static const struct alpha_pll_config 
>> cbfpll_config = {
>>          .early_output_mask = BIT(3),
>>   };
>> 
>>  +static const struct alpha_pll_config cbfpll_pro_config = {
>>  +       .l = 72,
>>  +       .config_ctl_val = 0x200d4828,
>>  +       .config_ctl_hi_val = 0x006,
>>  +       .test_ctl_val = 0x1c000000,
>>  +       .test_ctl_hi_val = 0x00004000,
>>  +       .pre_div_mask = BIT(12),
>>  +       .post_div_mask = 0x3 << 8,
>>  +       .post_div_val = 0x3 << 8,
>>  +       .main_output_mask = BIT(0),
>>  +       .early_output_mask = BIT(3),
>>  +};
> 
> Granted that the difference between this and the non-pro is just the
> post_div_val, would it be easier to just patch it in the probe()?

I wasn't sure if that approach would've been accepted since it relies 
on the special case that only one instance of the CBF device will exist 
in normal conditions, but if you say it's fine then sure, I can do that.

> 
>>  +
>>   static struct clk_alpha_pll cbf_pll = {
>>          .offset = CBF_PLL_OFFSET,
>>          .regs = cbf_pll_regs,
>>  @@ -93,6 +106,20 @@ static struct clk_fixed_factor cbf_pll_postdiv 
>> = {
>>          },
>>   };
>> 
>>  +static struct clk_fixed_factor cbf_pro_pll_postdiv = {
>>  +       .mult = 1,
>>  +       .div = 4,
>>  +       .hw.init = &(struct clk_init_data){
>>  +               .name = "cbf_pll_postdiv",
>>  +               .parent_hws = (const struct clk_hw*[]){
>>  +                       &cbf_pll.clkr.hw
>>  +               },
>>  +               .num_parents = 1,
>>  +               .ops = &clk_fixed_factor_ops,
>>  +               .flags = CLK_SET_RATE_PARENT,
>>  +       },
>>  +};
> 
> Same question here.
> 
> Then we can get rid of all the duplication below.
> 
>>  +
>>   static const struct clk_parent_data cbf_mux_parent_data[] = {
>>          { .index = DT_XO },
>>          { .hw = &cbf_pll.clkr.hw },
>>  @@ -100,6 +127,13 @@ static const struct clk_parent_data 
>> cbf_mux_parent_data[] = {
>>          { .index = DT_APCS_AUX },
>>   };
>> 
>>  +static const struct clk_parent_data cbf_pro_mux_parent_data[] = {
>>  +       { .index = DT_XO },
>>  +       { .hw = &cbf_pll.clkr.hw },
>>  +       { .hw = &cbf_pro_pll_postdiv.hw },
>>  +       { .index = DT_APCS_AUX },
>>  +};
>>  +
>>   struct clk_cbf_8996_mux {
>>          u32 reg;
>>          struct notifier_block nb;
>>  @@ -140,12 +174,14 @@ static int 
>> clk_cbf_8996_mux_determine_rate(struct clk_hw *hw,
>>                                             struct clk_rate_request 
>> *req)
>>   {
>>          struct clk_hw *parent;
>>  +       struct clk_hw *post_div_hw = clk_hw_get_parent_by_index(hw, 
>> CBF_DIV_INDEX);
>>  +       struct clk_fixed_factor *post_div = 
>> to_clk_fixed_factor(post_div_hw);
>> 
>>  -       if (req->rate < (DIV_THRESHOLD / 2))
>>  +       if (req->rate < (DIV_THRESHOLD / post_div->div))
>>                  return -EINVAL;
>> 
>>          if (req->rate < DIV_THRESHOLD)
>>  -               parent = clk_hw_get_parent_by_index(hw, 
>> CBF_DIV_INDEX);
>>  +               parent = post_div_hw;
>>          else
>>                  parent = clk_hw_get_parent_by_index(hw, 
>> CBF_PLL_INDEX);
>> 
>>  @@ -177,10 +213,24 @@ static struct clk_cbf_8996_mux cbf_mux = {
>>          },
>>   };
>> 
>>  +static struct clk_cbf_8996_mux cbf_pro_mux = {
>>  +       .reg = CBF_MUX_OFFSET,
>>  +       .nb.notifier_call = cbf_clk_notifier_cb,
>>  +       .clkr.hw.init = &(struct clk_init_data) {
>>  +               .name = "cbf_mux",
>>  +               .parent_data = cbf_pro_mux_parent_data,
>>  +               .num_parents = ARRAY_SIZE(cbf_pro_mux_parent_data),
>>  +               .ops = &clk_cbf_8996_mux_ops,
>>  +               /* CPU clock is critical and should never be gated 
>> */
>>  +               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>  +       },
>>  +};
>>  +
>>   static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned 
>> long event,
>>                                 void *data)
>>   {
>>          struct clk_notifier_data *cnd = data;
>>  +       struct clk_hw *hw = __clk_get_hw(cnd->clk);
>> 
>>          switch (event) {
>>          case PRE_RATE_CHANGE:
>>  @@ -188,19 +238,19 @@ static int cbf_clk_notifier_cb(struct 
>> notifier_block *nb, unsigned long event,
>>                   * Avoid overvolting. clk_core_set_rate_nolock() 
>> walks from top
>>                   * to bottom, so it will change the rate of the PLL 
>> before
>>                   * chaging the parent of PMUX. This can result in 
>> pmux getting
>>  -                * clocked twice the expected rate.
>>  +                * clocked twice (or 4 times) the expected rate.
>>                   *
>>  -                * Manually switch to PLL/2 here.
>>  +                * Manually switch to PLL/DIV here.
>>                   */
>>                  if (cnd->old_rate > DIV_THRESHOLD &&
>>                      cnd->new_rate < DIV_THRESHOLD)
>>  -                       
>> clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_DIV_INDEX);
>>  +                       clk_cbf_8996_mux_set_parent(hw, 
>> CBF_DIV_INDEX);
>>                  break;
>>          case ABORT_RATE_CHANGE:
>>                  /* Revert manual change */
>>                  if (cnd->new_rate < DIV_THRESHOLD &&
>>                      cnd->old_rate > DIV_THRESHOLD)
>>  -                       
>> clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_PLL_INDEX);
>>  +                       clk_cbf_8996_mux_set_parent(hw, 
>> CBF_PLL_INDEX);
>>                  break;
>>          default:
>>                  break;
>>  @@ -213,11 +263,50 @@ static struct clk_hw *cbf_msm8996_hw_clks[] = 
>> {
>>          &cbf_pll_postdiv.hw,
>>   };
>> 
>>  +static struct clk_hw *cbf_msm8996pro_hw_clks[] = {
>>  +       &cbf_pro_pll_postdiv.hw,
>>  +};
>>  +
>>   static struct clk_regmap *cbf_msm8996_clks[] = {
>>          &cbf_pll.clkr,
>>          &cbf_mux.clkr,
>>   };
>> 
>>  +static struct clk_regmap *cbf_msm8996pro_clks[] = {
>>  +       &cbf_pll.clkr,
>>  +       &cbf_pro_mux.clkr,
>>  +};
>>  +
>>  +struct cbf_match_data {
>>  +       const struct alpha_pll_config *config;
>>  +       struct clk_fixed_factor *cbf_pll_postdiv;
>>  +       struct clk_cbf_8996_mux *cbf_mux;
>>  +       struct clk_hw **hw_clks;
>>  +       size_t nr_hw_clks;
>>  +       struct clk_regmap **clks;
>>  +       size_t nr_clks;
>>  +};
>>  +
>>  +static const struct cbf_match_data cbf_msm8996_match_data = {
>>  +       .config = &cbfpll_config,
>>  +       .cbf_pll_postdiv = &cbf_pll_postdiv,
>>  +       .cbf_mux = &cbf_mux,
>>  +       .hw_clks = cbf_msm8996_hw_clks,
>>  +       .nr_hw_clks = ARRAY_SIZE(cbf_msm8996_hw_clks),
>>  +       .clks = cbf_msm8996_clks,
>>  +       .nr_clks = ARRAY_SIZE(cbf_msm8996_clks)
>>  +};
>>  +
>>  +static const struct cbf_match_data cbf_msm8996pro_match_data = {
>>  +       .config = &cbfpll_pro_config,
>>  +       .cbf_pll_postdiv = &cbf_pro_pll_postdiv,
>>  +       .cbf_mux = &cbf_pro_mux,
>>  +       .hw_clks = cbf_msm8996pro_hw_clks,
>>  +       .nr_hw_clks = ARRAY_SIZE(cbf_msm8996pro_hw_clks),
>>  +       .clks = cbf_msm8996pro_clks,
>>  +       .nr_clks = ARRAY_SIZE(cbf_msm8996pro_clks)
>>  +};
>>  +
>>   static const struct regmap_config cbf_msm8996_regmap_config = {
>>          .reg_bits               = 32,
>>          .reg_stride             = 4,
>>  @@ -274,6 +363,7 @@ static int qcom_msm8996_cbf_probe(struct 
>> platform_device *pdev)
>>          void __iomem *base;
>>          struct regmap *regmap;
>>          struct device *dev = &pdev->dev;
>>  +       const struct cbf_match_data *data = 
>> of_device_get_match_data(dev);
>>          int i, ret;
>> 
>>          base = devm_platform_ioremap_resource(pdev, 0);
>>  @@ -295,7 +385,7 @@ static int qcom_msm8996_cbf_probe(struct 
>> platform_device *pdev)
>>                             CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
>>                             
>> CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
>> 
>>  -       clk_alpha_pll_configure(&cbf_pll, regmap, &cbfpll_config);
>>  +       clk_alpha_pll_configure(&cbf_pll, regmap, data->config);
>> 
>>          /* Wait for PLL(s) to lock */
>>          udelay(50);
>>  @@ -311,27 +401,27 @@ static int qcom_msm8996_cbf_probe(struct 
>> platform_device *pdev)
>>          /* Switch CBF to use the primary PLL */
>>          regmap_update_bits(regmap, CBF_MUX_OFFSET, 
>> CBF_MUX_PARENT_MASK, 0x1);
>> 
>>  -       for (i = 0; i < ARRAY_SIZE(cbf_msm8996_hw_clks); i++) {
>>  -               ret = devm_clk_hw_register(dev, 
>> cbf_msm8996_hw_clks[i]);
>>  +       for (i = 0; i < data->nr_hw_clks; i++) {
>>  +               ret = devm_clk_hw_register(dev, data->hw_clks[i]);
>>                  if (ret)
>>                          return ret;
>>          }
>> 
>>  -       for (i = 0; i < ARRAY_SIZE(cbf_msm8996_clks); i++) {
>>  -               ret = devm_clk_register_regmap(dev, 
>> cbf_msm8996_clks[i]);
>>  +       for (i = 0; i < data->nr_clks; i++) {
>>  +               ret = devm_clk_register_regmap(dev, data->clks[i]);
>>                  if (ret)
>>                          return ret;
>>          }
>> 
>>  -       ret = devm_clk_notifier_register(dev, cbf_mux.clkr.hw.clk, 
>> &cbf_mux.nb);
>>  +       ret = devm_clk_notifier_register(dev, 
>> data->cbf_mux->clkr.hw.clk, &data->cbf_mux->nb);
>>          if (ret)
>>                  return ret;
>> 
>>  -       ret = devm_of_clk_add_hw_provider(dev, 
>> of_clk_hw_simple_get, &cbf_mux.clkr.hw);
>>  +       ret = devm_of_clk_add_hw_provider(dev, 
>> of_clk_hw_simple_get, &data->cbf_mux->clkr.hw);
>>          if (ret)
>>                  return ret;
>> 
>>  -       return qcom_msm8996_cbf_icc_register(pdev, 
>> &cbf_mux.clkr.hw);
>>  +       return qcom_msm8996_cbf_icc_register(pdev, 
>> &data->cbf_mux->clkr.hw);
>>   }
>> 
>>   static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
>>  @@ -340,7 +430,8 @@ static int qcom_msm8996_cbf_remove(struct 
>> platform_device *pdev)
>>   }
>> 
>>   static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
>>  -       { .compatible = "qcom,msm8996-cbf" },
>>  +       { .compatible = "qcom,msm8996-cbf", .data = 
>> &cbf_msm8996_match_data },
>>  +       { .compatible = "qcom,msm8996pro-cbf", .data = 
>> &cbf_msm8996pro_match_data },
>>          { /* sentinel */ },
>>   };
>>   MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
>>  --
>>  2.40.0
>> 
> 
> 
> --
> With best wishes
> Dmitry

Re: [PATCH RESEND 3/3] clk: qcom: cbf-msm8996: Add support for MSM8996 Pro

[Dmitry Baryshkov]

On Mon, 22 May 2023 at 20:49, Yassine Oudjana <yassine.oudjana@gmail.com> wrote:
>
>
> On Wed, May 3 2023 at 04:20:00 PM +03:00:00, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> > On Wed, 3 May 2023 at 16:02, Yassine Oudjana
> > <yassine.oudjana@gmail.com> wrote:
> >>
> >>  From: Yassine Oudjana <y.oudjana@protonmail.com>
> >>
> >>  The CBF PLL on MSM8996 Pro has a /4 post divisor instead of /2.
> >> Handle the
> >>  difference accordingly.
> >>
> >>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> >>  ---
> >>   drivers/clk/qcom/clk-cbf-8996.c | 121
> >> ++++++++++++++++++++++++++++----
> >>   1 file changed, 106 insertions(+), 15 deletions(-)
> >>
> >>  diff --git a/drivers/clk/qcom/clk-cbf-8996.c
> >> b/drivers/clk/qcom/clk-cbf-8996.c
> >>  index 1bb2cd956d68..a3e96578ddd9 100644
> >>  --- a/drivers/clk/qcom/clk-cbf-8996.c
> >>  +++ b/drivers/clk/qcom/clk-cbf-8996.c
> >>  @@ -65,6 +65,19 @@ static const struct alpha_pll_config
> >> cbfpll_config = {
> >>          .early_output_mask = BIT(3),
> >>   };
> >>
> >>  +static const struct alpha_pll_config cbfpll_pro_config = {
> >>  +       .l = 72,
> >>  +       .config_ctl_val = 0x200d4828,
> >>  +       .config_ctl_hi_val = 0x006,
> >>  +       .test_ctl_val = 0x1c000000,
> >>  +       .test_ctl_hi_val = 0x00004000,
> >>  +       .pre_div_mask = BIT(12),
> >>  +       .post_div_mask = 0x3 << 8,
> >>  +       .post_div_val = 0x3 << 8,
> >>  +       .main_output_mask = BIT(0),
> >>  +       .early_output_mask = BIT(3),
> >>  +};
> >
> > Granted that the difference between this and the non-pro is just the
> > post_div_val, would it be easier to just patch it in the probe()?
>
> I wasn't sure if that approach would've been accepted since it relies
> on the special case that only one instance of the CBF device will exist
> in normal conditions, but if you say it's fine then sure, I can do that.

We do this for other clock controllers, which we know to be singleton.
So this should be fine too.

--
With best wishes
Dmitry