[PATCH 0/6] Add APSS clock controller support for IPQ9574

[PATCH 0/6] Add APSS clock controller support for IPQ9574

[devi priya]

The APSS clock controller in IPQ9574 based devices supports cpu with
frequencies above 800Mhz

This patch series adds the support for the same

This series depends on the below patchset
https://lore.kernel.org/linux-arm-msm/20230110121316.24892-1-quic_devipriy@quicinc.com/

devi priya (6):
  dt-bindings: clock: Add YAML schemas for QCOM A73 PLL
  clk: qcom: ipq9574: Enable APSS clock driver
  arm64: defconfig: Enable ipq6018 apss clock and PLL controller
  arm64: dts: qcom: ipq9574: Add support for APSS clock controller
  dt-bindings: mailbox: Add compatible for IPQ9574
  clk: qcom: Fix APSS PLL and RCG Configuration

 .../bindings/clock/qcom,a73pll.yaml           | 52 +++++++++++++++++++
 .../mailbox/qcom,apcs-kpss-global.yaml        |  3 ++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         | 17 ++++++
 arch/arm64/configs/defconfig                  |  1 +
 drivers/clk/qcom/apss-ipq-pll.c               | 14 +++++
 drivers/clk/qcom/apss-ipq6018.c               |  8 ++-
 drivers/mailbox/qcom-apcs-ipc-mailbox.c       |  5 ++
 7 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a73pll.yaml


base-commit: 1fe4fd6f5cad346e598593af36caeadc4f5d4fa9
-- 
2.17.1

[PATCH 1/6] dt-bindings: clock: Add YAML schemas for QCOM A73 PLL

[devi priya]

Add schema for primary CPU PLL found on few Qualcomm platforms.

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: devi priya <quic_devipriy@quicinc.com>
---
 .../bindings/clock/qcom,a73pll.yaml           | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a73pll.yaml

diff --git a/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
new file mode 100644
index 000000000000..a0e81094db8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,a73pll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm A73 PLL clock
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+
+description:
+  The A73 PLL on few Qualcomm platforms is the main CPU PLL used for
+  frequencies above 1GHz.
+
+properties:
+  compatible:
+    enum:
+      - qcom,ipq9574-a73pll
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 0
+
+  clocks:
+    items:
+      - description: board XO clock
+
+  clock-names:
+    items:
+      - const: xo
+
+  operating-points-v2: true
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    a73pll: clock@b116000 {
+            compatible = "qcom,ipq9574-a73pll";
+            reg = <0x0b116000 0x40>;
+            #clock-cells = <0>;
+            clocks = <&xo_board_clk>;
+            clock-names = "xo";
+    };
-- 
2.17.1

[PATCH 2/6] clk: qcom: ipq9574: Enable APSS clock driver

[devi priya]

Enable APSS clock driver for IPQ9574 based devices

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: devi priya <quic_devipriy@quicinc.com>
---
 drivers/clk/qcom/apss-ipq-pll.c         | 13 +++++++++++++
 drivers/mailbox/qcom-apcs-ipc-mailbox.c |  5 +++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index a5aea27eb867..dd0c01bf5a98 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -61,6 +61,18 @@ static const struct alpha_pll_config ipq8074_pll_config = {
 	.test_ctl_hi_val = 0x4000,
 };
 
+static const struct alpha_pll_config ipq9574_pll_config = {
+	.l = 0x3b,
+	.config_ctl_val = 0x200D4828,
+	.config_ctl_hi_val = 0x6,
+	.early_output_mask = BIT(3),
+	.aux2_output_mask = BIT(2),
+	.aux_output_mask = BIT(1),
+	.main_output_mask = BIT(0),
+	.test_ctl_val = 0x0,
+	.test_ctl_hi_val = 0x4000,
+};
+
 static const struct regmap_config ipq_pll_regmap_config = {
 	.reg_bits		= 32,
 	.reg_stride		= 4,
@@ -102,6 +114,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
 static const struct of_device_id apss_ipq_pll_match_table[] = {
 	{ .compatible = "qcom,ipq6018-a53pll", .data = &ipq6018_pll_config },
 	{ .compatible = "qcom,ipq8074-a53pll", .data = &ipq8074_pll_config },
+	{ .compatible = "qcom,ipq9574-a73pll", .data = &ipq9574_pll_config },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 0e9f9cba8668..90e74f9d7cb3 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -33,6 +33,10 @@ static const struct qcom_apcs_ipc_data ipq6018_apcs_data = {
 	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
 };
 
+static const struct qcom_apcs_ipc_data ipq9574_apcs_data = {
+	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
+};
+
 static const struct qcom_apcs_ipc_data msm8916_apcs_data = {
 	.offset = 8, .clk_name = "qcom-apcs-msm8916-clk"
 };
@@ -143,6 +147,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev)
 static const struct of_device_id qcom_apcs_ipc_of_match[] = {
 	{ .compatible = "qcom,ipq6018-apcs-apps-global", .data = &ipq6018_apcs_data },
 	{ .compatible = "qcom,ipq8074-apcs-apps-global", .data = &ipq6018_apcs_data },
+	{ .compatible = "qcom,ipq9574-apcs-apps-global", .data = &ipq9574_apcs_data },
 	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = &msm8916_apcs_data },
 	{ .compatible = "qcom,msm8939-apcs-kpss-global", .data = &msm8916_apcs_data },
 	{ .compatible = "qcom,msm8953-apcs-kpss-global", .data = &msm8994_apcs_data },
-- 
2.17.1

[PATCH 3/6] arm64: defconfig: Enable ipq6018 apss clock and PLL controller

[devi priya]

Enable the PLL controller and IPQ6018 APSS clock controller

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: devi priya <quic_devipriy@quicinc.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index e0ae0996d5ad..8de3979b10a3 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1092,6 +1092,7 @@ CONFIG_QCOM_CLK_APCS_MSM8916=y
 CONFIG_QCOM_CLK_APCC_MSM8996=y
 CONFIG_QCOM_CLK_SMD_RPM=y
 CONFIG_QCOM_CLK_RPMH=y
+CONFIG_IPQ_APSS_6018=y
 CONFIG_IPQ_GCC_6018=y
 CONFIG_IPQ_GCC_8074=y
 CONFIG_IPQ_GCC_9574=y
-- 
2.17.1

[PATCH 4/6] arm64: dts: qcom: ipq9574: Add support for APSS clock controller

[devi priya]

Enable APSS clock controller in IPQ9574

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: devi priya <quic_devipriy@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 188d18688a77..5a2244b437ed 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -247,6 +247,23 @@
 			};
 		};
 
+		apcs_glb: mailbox@b111000 {
+			compatible = "qcom,ipq9574-apcs-apps-global";
+			reg = <0x0b111000 0x1000>;
+			#clock-cells = <1>;
+			clocks = <&a73pll>, <&xo_board_clk>;
+			clock-names = "pll", "xo";
+			#mbox-cells = <1>;
+		};
+
+		a73pll: clock@b116000 {
+			compatible = "qcom,ipq9574-a73pll";
+			reg = <0x0b116000 0x40>;
+			#clock-cells = <0>;
+			clocks = <&xo_board_clk>;
+			clock-names = "xo";
+		};
+
 		timer@b120000 {
 			compatible = "arm,armv7-timer-mem";
 			reg = <0xb120000 0x1000>;
-- 
2.17.1

[PATCH 5/6] dt-bindings: mailbox: Add compatible for IPQ9574

[devi priya]

Add the mailbox compatible string for IPQ9574 SoC

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: devi priya <quic_devipriy@quicinc.com>
---
 .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml     | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
index 943f9472ae10..9e076758a58a 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -20,6 +20,7 @@ properties:
           - enum:
               - qcom,ipq6018-apcs-apps-global
               - qcom,ipq8074-apcs-apps-global
+              - qcom,ipq9574-apcs-apps-global
               - qcom,msm8976-apcs-kpss-global
               - qcom,msm8996-apcs-hmss-global
               - qcom,msm8998-apcs-hmss-global
@@ -113,6 +114,7 @@ allOf:
           enum:
             - qcom,ipq6018-apcs-apps-global
             - qcom,ipq8074-apcs-apps-global
+            - qcom,ipq9574-apcs-apps-global
     then:
       properties:
         clocks:
@@ -129,6 +131,7 @@ allOf:
           enum:
             - qcom,ipq6018-apcs-apps-global
             - qcom,ipq8074-apcs-apps-global
+            - qcom,ipq9574-apcs-apps-global
     then:
       properties:
         '#clock-cells':
-- 
2.17.1

[PATCH 6/6] clk: qcom: Fix APSS PLL and RCG Configuration

[devi priya]

Included CLK_IS_CRITICAL flag which helps to properly enable
the APSS PLL during bootup.
clk_rcg2_ops should be used for APSS clock RCG, as other ops
will not configure the RCG register

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: devi priya <quic_devipriy@quicinc.com>
---
 drivers/clk/qcom/apss-ipq-pll.c | 1 +
 drivers/clk/qcom/apss-ipq6018.c | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index dd0c01bf5a98..75486a124fcd 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -33,6 +33,7 @@ static struct clk_alpha_pll ipq_pll = {
 			},
 			.num_parents = 1,
 			.ops = &clk_alpha_pll_huayra_ops,
+			.flags = CLK_IS_CRITICAL,
 		},
 	},
 };
diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
index f2f502e2d5a4..0d0e7196a4dc 100644
--- a/drivers/clk/qcom/apss-ipq6018.c
+++ b/drivers/clk/qcom/apss-ipq6018.c
@@ -33,15 +33,21 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
 	{ P_APSS_PLL_EARLY, 5 },
 };
 
+static const struct freq_tbl ftbl_apcs_alias0_clk_src[] = {
+	{ .src = P_APSS_PLL_EARLY, .pre_div = 1 },
+	{ }
+};
+
 static struct clk_rcg2 apcs_alias0_clk_src = {
 	.cmd_rcgr = 0x0050,
+	.freq_tbl = ftbl_apcs_alias0_clk_src,
 	.hid_width = 5,
 	.parent_map = parents_apcs_alias0_clk_src_map,
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "apcs_alias0_clk_src",
 		.parent_data = parents_apcs_alias0_clk_src,
 		.num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
-		.ops = &clk_rcg2_mux_closest_ops,
+		.ops = &clk_rcg2_ops,
 		.flags = CLK_SET_RATE_PARENT,
 	},
 };
-- 
2.17.1

Re: [PATCH 1/6] dt-bindings: clock: Add YAML schemas for QCOM A73 PLL

[Krzysztof Kozlowski]

On 13/01/2023 15:36, devi priya wrote:
> Add schema for primary CPU PLL found on few Qualcomm platforms.

Subject: drop redundant "YAML schemas for"


> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---
>  .../bindings/clock/qcom,a73pll.yaml           | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
> new file mode 100644
> index 000000000000..a0e81094db8d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/qcom,a73pll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm A73 PLL clock
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +
> +description:
> +  The A73 PLL on few Qualcomm platforms is the main CPU PLL used for
> +  frequencies above 1GHz.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq9574-a73pll
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 0
> +
> +  clocks:
> +    items:
> +      - description: board XO clock
> +
> +  clock-names:
> +    items:
> +      - const: xo
> +
> +  operating-points-v2: true

Drop. I'll fix the other bindings.

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    a73pll: clock@b116000 {
> +            compatible = "qcom,ipq9574-a73pll";

Use 4 spaces for example indentation.

> +            reg = <0x0b116000 0x40>;
> +            #clock-cells = <0>;
> +            clocks = <&xo_board_clk>;
> +            clock-names = "xo";
> +    };

Best regards,
Krzysztof

Re: [PATCH 5/6] dt-bindings: mailbox: Add compatible for IPQ9574

[Krzysztof Kozlowski]

On 13/01/2023 15:36, devi priya wrote:
> Add the mailbox compatible string for IPQ9574 SoC
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---

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

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: clock: Add YAML schemas for QCOM A73 PLL

[Konrad Dybcio]

On 13.01.2023 15:36, devi priya wrote:
> Add schema for primary CPU PLL found on few Qualcomm platforms.
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---
Doesn't this belong in Documentation/devicetree/bindings/clock/qcom,a53pll.yaml?

It looks identical, so it may be as simple as adding your
new compatible there..

Konrad
>  .../bindings/clock/qcom,a73pll.yaml           | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
> new file mode 100644
> index 000000000000..a0e81094db8d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/qcom,a73pll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm A73 PLL clock
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +
> +description:
> +  The A73 PLL on few Qualcomm platforms is the main CPU PLL used for
> +  frequencies above 1GHz.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq9574-a73pll
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 0
> +
> +  clocks:
> +    items:
> +      - description: board XO clock
> +
> +  clock-names:
> +    items:
> +      - const: xo
> +
> +  operating-points-v2: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    a73pll: clock@b116000 {
> +            compatible = "qcom,ipq9574-a73pll";
> +            reg = <0x0b116000 0x40>;
> +            #clock-cells = <0>;
> +            clocks = <&xo_board_clk>;
> +            clock-names = "xo";
> +    };

Re: [PATCH 2/6] clk: qcom: ipq9574: Enable APSS clock driver

[Konrad Dybcio]

On 13.01.2023 15:36, devi priya wrote:
> Enable APSS clock driver for IPQ9574 based devices
Please be more descriptive of what you're doing and why
you're doing it.

clk: qcom: apss-ipq-pll: Add IPQ9574 support

Add IPQ9574-specific APSS PLL configuration values.


mailbox: qcom-apcs-ipc: Add IPQ9574 support

Add a compatible for IPQ9574's mailbox. The SoC, similarly
to other IPQs uses the APSS IPQ PLL driver for CPU scaling.


> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---
>  drivers/clk/qcom/apss-ipq-pll.c         | 13 +++++++++++++
>  drivers/mailbox/qcom-apcs-ipc-mailbox.c |  5 +++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> index a5aea27eb867..dd0c01bf5a98 100644
> --- a/drivers/clk/qcom/apss-ipq-pll.c
> +++ b/drivers/clk/qcom/apss-ipq-pll.c
> @@ -61,6 +61,18 @@ static const struct alpha_pll_config ipq8074_pll_config = {
>  	.test_ctl_hi_val = 0x4000,
>  };
>  
> +static const struct alpha_pll_config ipq9574_pll_config = {
> +	.l = 0x3b,
> +	.config_ctl_val = 0x200D4828,
Lowercase hex, please.

> +	.config_ctl_hi_val = 0x6,
> +	.early_output_mask = BIT(3),
> +	.aux2_output_mask = BIT(2),
> +	.aux_output_mask = BIT(1),
> +	.main_output_mask = BIT(0),
> +	.test_ctl_val = 0x0,
> +	.test_ctl_hi_val = 0x4000,
> +};
> +
>  static const struct regmap_config ipq_pll_regmap_config = {
>  	.reg_bits		= 32,
>  	.reg_stride		= 4,
> @@ -102,6 +114,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>  static const struct of_device_id apss_ipq_pll_match_table[] = {
>  	{ .compatible = "qcom,ipq6018-a53pll", .data = &ipq6018_pll_config },
>  	{ .compatible = "qcom,ipq8074-a53pll", .data = &ipq8074_pll_config },
> +	{ .compatible = "qcom,ipq9574-a73pll", .data = &ipq9574_pll_config },
>  	{ }
>  };
These are very small changes, so maybe they'll pass, but generally
it's preferred to split changes per-file if possible (and here it is
possible if you change the APSS PLL driver first and then bind it in
APCS mbox afterwards).

>  MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index 0e9f9cba8668..90e74f9d7cb3 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -33,6 +33,10 @@ static const struct qcom_apcs_ipc_data ipq6018_apcs_data = {
>  	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
>  };
>  
> +static const struct qcom_apcs_ipc_data ipq9574_apcs_data = {
> +	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
> +};
Please reuse ipq6018_apcs_data, it's identical.

Konrad
> +
>  static const struct qcom_apcs_ipc_data msm8916_apcs_data = {
>  	.offset = 8, .clk_name = "qcom-apcs-msm8916-clk"
>  };
> @@ -143,6 +147,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev)
>  static const struct of_device_id qcom_apcs_ipc_of_match[] = {
>  	{ .compatible = "qcom,ipq6018-apcs-apps-global", .data = &ipq6018_apcs_data },
>  	{ .compatible = "qcom,ipq8074-apcs-apps-global", .data = &ipq6018_apcs_data },
> +	{ .compatible = "qcom,ipq9574-apcs-apps-global", .data = &ipq9574_apcs_data },
>  	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = &msm8916_apcs_data },
>  	{ .compatible = "qcom,msm8939-apcs-kpss-global", .data = &msm8916_apcs_data },
>  	{ .compatible = "qcom,msm8953-apcs-kpss-global", .data = &msm8994_apcs_data },

Re: [PATCH 5/6] dt-bindings: mailbox: Add compatible for IPQ9574

[Konrad Dybcio]

On 13.01.2023 15:36, devi priya wrote:
> Add the mailbox compatible string for IPQ9574 SoC
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---
Binding changes should come before driver changes, so that
you're not introducing an "illegal" compatible and only
"legalize" it later - please reorder the patch.

Konrad
>  .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml     | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
> index 943f9472ae10..9e076758a58a 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
> @@ -20,6 +20,7 @@ properties:
>            - enum:
>                - qcom,ipq6018-apcs-apps-global
>                - qcom,ipq8074-apcs-apps-global
> +              - qcom,ipq9574-apcs-apps-global
>                - qcom,msm8976-apcs-kpss-global
>                - qcom,msm8996-apcs-hmss-global
>                - qcom,msm8998-apcs-hmss-global
> @@ -113,6 +114,7 @@ allOf:
>            enum:
>              - qcom,ipq6018-apcs-apps-global
>              - qcom,ipq8074-apcs-apps-global
> +            - qcom,ipq9574-apcs-apps-global
>      then:
>        properties:
>          clocks:
> @@ -129,6 +131,7 @@ allOf:
>            enum:
>              - qcom,ipq6018-apcs-apps-global
>              - qcom,ipq8074-apcs-apps-global
> +            - qcom,ipq9574-apcs-apps-global
>      then:
>        properties:
>          '#clock-cells':

Re: [PATCH 6/6] clk: qcom: Fix APSS PLL and RCG Configuration

[Konrad Dybcio]

On 13.01.2023 15:36, devi priya wrote:
> Included CLK_IS_CRITICAL flag which helps to properly enable
> the APSS PLL during bootup.
Please describe the issue and not only the user-visible impact it
makes. Does the PLL get shut down by clk_ignore_unused? Maybe you
would be interested in the sync_state changes that landed in recent
-next that may solve it for you?

I don't think it should be always-on, as you have an alternate source
for low power modes, adding CLK_IS_CRITICAL will keep the PLL enabled
even if you're not using it.

> clk_rcg2_ops should be used for APSS clock RCG, as other ops
> will not configure the RCG register
RCG register meaning RCG register*s*, meaning in this case M/N/D
which would be required for proper rate setting and not only input
switching (which arguably doesn't seem to be of much concern on a
single-parent clock)? This all is not obvious..

Konrad
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---
>  drivers/clk/qcom/apss-ipq-pll.c | 1 +
>  drivers/clk/qcom/apss-ipq6018.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> index dd0c01bf5a98..75486a124fcd 100644
> --- a/drivers/clk/qcom/apss-ipq-pll.c
> +++ b/drivers/clk/qcom/apss-ipq-pll.c
> @@ -33,6 +33,7 @@ static struct clk_alpha_pll ipq_pll = {
>  			},
>  			.num_parents = 1,
>  			.ops = &clk_alpha_pll_huayra_ops,
> +			.flags = CLK_IS_CRITICAL,
>  		},
>  	},
>  };
> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> index f2f502e2d5a4..0d0e7196a4dc 100644
> --- a/drivers/clk/qcom/apss-ipq6018.c
> +++ b/drivers/clk/qcom/apss-ipq6018.c
> @@ -33,15 +33,21 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
>  	{ P_APSS_PLL_EARLY, 5 },
>  };
>  
> +static const struct freq_tbl ftbl_apcs_alias0_clk_src[] = {
> +	{ .src = P_APSS_PLL_EARLY, .pre_div = 1 },
> +	{ }
> +};
> +
>  static struct clk_rcg2 apcs_alias0_clk_src = {
>  	.cmd_rcgr = 0x0050,
> +	.freq_tbl = ftbl_apcs_alias0_clk_src,
>  	.hid_width = 5,
>  	.parent_map = parents_apcs_alias0_clk_src_map,
>  	.clkr.hw.init = &(struct clk_init_data){
>  		.name = "apcs_alias0_clk_src",
>  		.parent_data = parents_apcs_alias0_clk_src,
>  		.num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
> -		.ops = &clk_rcg2_mux_closest_ops,
> +		.ops = &clk_rcg2_ops,
>  		.flags = CLK_SET_RATE_PARENT,
>  	},
>  };

Re: [PATCH 6/6] clk: qcom: Fix APSS PLL and RCG Configuration

[Robert Marko]

On 13. 01. 2023. 16:20, Konrad Dybcio wrote:
>
> On 13.01.2023 15:36, devi priya wrote:
>> Included CLK_IS_CRITICAL flag which helps to properly enable
>> the APSS PLL during bootup.
> Please describe the issue and not only the user-visible impact it
> makes. Does the PLL get shut down by clk_ignore_unused? Maybe you
> would be interested in the sync_state changes that landed in recent
> -next that may solve it for you?
>
> I don't think it should be always-on, as you have an alternate source
> for low power modes, adding CLK_IS_CRITICAL will keep the PLL enabled
> even if you're not using it.

I have the same opinion, as this is working fine on IPQ6018 and IPQ8074
and I have not experienced any issues with it.

>
>> clk_rcg2_ops should be used for APSS clock RCG, as other ops
>> will not configure the RCG register
> RCG register meaning RCG register*s*, meaning in this case M/N/D
> which would be required for proper rate setting and not only input
> switching (which arguably doesn't seem to be of much concern on a
> single-parent clock)? This all is not obvious..
Same question from me as well, why do you need clk_rcg2_ops with
a dummy frequency table since this is just a mux using RCG2 control
bits?

Regards,
Robert
>
> Konrad
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
>>   drivers/clk/qcom/apss-ipq-pll.c | 1 +
>>   drivers/clk/qcom/apss-ipq6018.c | 8 +++++++-
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
>> index dd0c01bf5a98..75486a124fcd 100644
>> --- a/drivers/clk/qcom/apss-ipq-pll.c
>> +++ b/drivers/clk/qcom/apss-ipq-pll.c
>> @@ -33,6 +33,7 @@ static struct clk_alpha_pll ipq_pll = {
>>   			},
>>   			.num_parents = 1,
>>   			.ops = &clk_alpha_pll_huayra_ops,
>> +			.flags = CLK_IS_CRITICAL,
>>   		},
>>   	},
>>   };
>> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
>> index f2f502e2d5a4..0d0e7196a4dc 100644
>> --- a/drivers/clk/qcom/apss-ipq6018.c
>> +++ b/drivers/clk/qcom/apss-ipq6018.c
>> @@ -33,15 +33,21 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
>>   	{ P_APSS_PLL_EARLY, 5 },
>>   };
>>   
>> +static const struct freq_tbl ftbl_apcs_alias0_clk_src[] = {
>> +	{ .src = P_APSS_PLL_EARLY, .pre_div = 1 },
>> +	{ }
>> +};
>> +
>>   static struct clk_rcg2 apcs_alias0_clk_src = {
>>   	.cmd_rcgr = 0x0050,
>> +	.freq_tbl = ftbl_apcs_alias0_clk_src,
>>   	.hid_width = 5,
>>   	.parent_map = parents_apcs_alias0_clk_src_map,
>>   	.clkr.hw.init = &(struct clk_init_data){
>>   		.name = "apcs_alias0_clk_src",
>>   		.parent_data = parents_apcs_alias0_clk_src,
>>   		.num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
>> -		.ops = &clk_rcg2_mux_closest_ops,
>> +		.ops = &clk_rcg2_ops,
>>   		.flags = CLK_SET_RATE_PARENT,
>>   	},
>>   };
>

Re: [PATCH 1/6] dt-bindings: clock: Add YAML schemas for QCOM A73 PLL

[Devi Priya]

Thanks for taking time to review the patch!

On 1/13/2023 8:27 PM, Krzysztof Kozlowski wrote:
> On 13/01/2023 15:36, devi priya wrote:
>> Add schema for primary CPU PLL found on few Qualcomm platforms.
> 
> Subject: drop redundant "YAML schemas for"
> 
Sure, okay
> 
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
>>   .../bindings/clock/qcom,a73pll.yaml           | 52 +++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
>> new file mode 100644
>> index 000000000000..a0e81094db8d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/qcom,a73pll.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm A73 PLL clock
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +
>> +description:
>> +  The A73 PLL on few Qualcomm platforms is the main CPU PLL used for
>> +  frequencies above 1GHz.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq9574-a73pll
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#clock-cells':
>> +    const: 0
>> +
>> +  clocks:
>> +    items:
>> +      - description: board XO clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xo
>> +
>> +  operating-points-v2: true
> 
> Drop. I'll fix the other bindings.
> 
As suggested by konrad, will drop this file change and add the 
compatible in qcom,a53pll.yaml
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#clock-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    a73pll: clock@b116000 {
>> +            compatible = "qcom,ipq9574-a73pll";
> 
> Use 4 spaces for example indentation.
> 
>> +            reg = <0x0b116000 0x40>;
>> +            #clock-cells = <0>;
>> +            clocks = <&xo_board_clk>;
>> +            clock-names = "xo";
>> +    };
> 
> Best regards,
> Krzysztof
> 
Best Regards,
Devi Priya

Re: [PATCH 1/6] dt-bindings: clock: Add YAML schemas for QCOM A73 PLL

[Devi Priya]

Thanks for taking time to review the patch!

On 1/13/2023 8:35 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 15:36, devi priya wrote:
>> Add schema for primary CPU PLL found on few Qualcomm platforms.
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
> Doesn't this belong in Documentation/devicetree/bindings/clock/qcom,a53pll.yaml?
> 
> It looks identical, so it may be as simple as adding your
> new compatible there..
> 
As the name was specific to a53pll, added a new yaml for a73.
Will add the a73 compatible in qcom,a53pll.yaml if that's accepted!
> Konrad
>>   .../bindings/clock/qcom,a73pll.yaml           | 52 +++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
>> new file mode 100644
>> index 000000000000..a0e81094db8d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,a73pll.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/qcom,a73pll.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm A73 PLL clock
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +
>> +description:
>> +  The A73 PLL on few Qualcomm platforms is the main CPU PLL used for
>> +  frequencies above 1GHz.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq9574-a73pll
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#clock-cells':
>> +    const: 0
>> +
>> +  clocks:
>> +    items:
>> +      - description: board XO clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xo
>> +
>> +  operating-points-v2: true
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#clock-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    a73pll: clock@b116000 {
>> +            compatible = "qcom,ipq9574-a73pll";
>> +            reg = <0x0b116000 0x40>;
>> +            #clock-cells = <0>;
>> +            clocks = <&xo_board_clk>;
>> +            clock-names = "xo";
>> +    };
Best Regards,
Devi Priya

Re: [PATCH 2/6] clk: qcom: ipq9574: Enable APSS clock driver

[Devi Priya]

On 1/13/2023 8:42 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 15:36, devi priya wrote:
>> Enable APSS clock driver for IPQ9574 based devices
> Please be more descriptive of what you're doing and why
> you're doing it.
> 
> clk: qcom: apss-ipq-pll: Add IPQ9574 support
> 
> Add IPQ9574-specific APSS PLL configuration values.
> 
> 
> mailbox: qcom-apcs-ipc: Add IPQ9574 support
> 
> Add a compatible for IPQ9574's mailbox. The SoC, similarly
> to other IPQs uses the APSS IPQ PLL driver for CPU scaling.
> 
Sure, okay
> 
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
>>   drivers/clk/qcom/apss-ipq-pll.c         | 13 +++++++++++++
>>   drivers/mailbox/qcom-apcs-ipc-mailbox.c |  5 +++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
>> index a5aea27eb867..dd0c01bf5a98 100644
>> --- a/drivers/clk/qcom/apss-ipq-pll.c
>> +++ b/drivers/clk/qcom/apss-ipq-pll.c
>> @@ -61,6 +61,18 @@ static const struct alpha_pll_config ipq8074_pll_config = {
>>   	.test_ctl_hi_val = 0x4000,
>>   };
>>   
>> +static const struct alpha_pll_config ipq9574_pll_config = {
>> +	.l = 0x3b,
>> +	.config_ctl_val = 0x200D4828,
> Lowercase hex, please.
Okay
> 
>> +	.config_ctl_hi_val = 0x6,
>> +	.early_output_mask = BIT(3),
>> +	.aux2_output_mask = BIT(2),
>> +	.aux_output_mask = BIT(1),
>> +	.main_output_mask = BIT(0),
>> +	.test_ctl_val = 0x0,
>> +	.test_ctl_hi_val = 0x4000,
>> +};
>> +
>>   static const struct regmap_config ipq_pll_regmap_config = {
>>   	.reg_bits		= 32,
>>   	.reg_stride		= 4,
>> @@ -102,6 +114,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>>   static const struct of_device_id apss_ipq_pll_match_table[] = {
>>   	{ .compatible = "qcom,ipq6018-a53pll", .data = &ipq6018_pll_config },
>>   	{ .compatible = "qcom,ipq8074-a53pll", .data = &ipq8074_pll_config },
>> +	{ .compatible = "qcom,ipq9574-a73pll", .data = &ipq9574_pll_config },
>>   	{ }
>>   };
> These are very small changes, so maybe they'll pass, but generally
> it's preferred to split changes per-file if possible (and here it is
> possible if you change the APSS PLL driver first and then bind it in
> APCS mbox afterwards).
> 
Sure, will split the file changes in V2

>>   MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
>> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> index 0e9f9cba8668..90e74f9d7cb3 100644
>> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> @@ -33,6 +33,10 @@ static const struct qcom_apcs_ipc_data ipq6018_apcs_data = {
>>   	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
>>   };
>>   
>> +static const struct qcom_apcs_ipc_data ipq9574_apcs_data = {
>> +	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
>> +};
> Please reuse ipq6018_apcs_data, it's identical.
> 
> Konrad
Okay
>> +
>>   static const struct qcom_apcs_ipc_data msm8916_apcs_data = {
>>   	.offset = 8, .clk_name = "qcom-apcs-msm8916-clk"
>>   };
>> @@ -143,6 +147,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev)
>>   static const struct of_device_id qcom_apcs_ipc_of_match[] = {
>>   	{ .compatible = "qcom,ipq6018-apcs-apps-global", .data = &ipq6018_apcs_data },
>>   	{ .compatible = "qcom,ipq8074-apcs-apps-global", .data = &ipq6018_apcs_data },
>> +	{ .compatible = "qcom,ipq9574-apcs-apps-global", .data = &ipq9574_apcs_data },
>>   	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = &msm8916_apcs_data },
>>   	{ .compatible = "qcom,msm8939-apcs-kpss-global", .data = &msm8916_apcs_data },
>>   	{ .compatible = "qcom,msm8953-apcs-kpss-global", .data = &msm8994_apcs_data },
Best Regards,
Devi Priya

Re: [PATCH 5/6] dt-bindings: mailbox: Add compatible for IPQ9574

[Devi Priya]

On 1/13/2023 8:43 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 15:36, devi priya wrote:
>> Add the mailbox compatible string for IPQ9574 SoC
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
> Binding changes should come before driver changes, so that
> you're not introducing an "illegal" compatible and only
> "legalize" it later - please reorder the patch.
> 
> Konrad
Understood, will reorder the patches in V2
>>   .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml     | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
>> index 943f9472ae10..9e076758a58a 100644
>> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
>> @@ -20,6 +20,7 @@ properties:
>>             - enum:
>>                 - qcom,ipq6018-apcs-apps-global
>>                 - qcom,ipq8074-apcs-apps-global
>> +              - qcom,ipq9574-apcs-apps-global
>>                 - qcom,msm8976-apcs-kpss-global
>>                 - qcom,msm8996-apcs-hmss-global
>>                 - qcom,msm8998-apcs-hmss-global
>> @@ -113,6 +114,7 @@ allOf:
>>             enum:
>>               - qcom,ipq6018-apcs-apps-global
>>               - qcom,ipq8074-apcs-apps-global
>> +            - qcom,ipq9574-apcs-apps-global
>>       then:
>>         properties:
>>           clocks:
>> @@ -129,6 +131,7 @@ allOf:
>>             enum:
>>               - qcom,ipq6018-apcs-apps-global
>>               - qcom,ipq8074-apcs-apps-global
>> +            - qcom,ipq9574-apcs-apps-global
>>       then:
>>         properties:
>>           '#clock-cells':
Best Regards,
Devi Priya

Re: [PATCH 6/6] clk: qcom: Fix APSS PLL and RCG Configuration

[Devi Priya]

Thanks for taking time to review the patch

On 1/13/2023 8:50 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 15:36, devi priya wrote:
>> Included CLK_IS_CRITICAL flag which helps to properly enable
>> the APSS PLL during bootup.
> Please describe the issue and not only the user-visible impact it
> makes. Does the PLL get shut down by clk_ignore_unused? Maybe you
> would be interested in the sync_state changes that landed in recent
> -next that may solve it for you?
> 
> I don't think it should be always-on, as you have an alternate source
> for low power modes, adding CLK_IS_CRITICAL will keep the PLL enabled
> even if you're not using it.
Yeah, got it. Will drop the critical flag
> 
>> clk_rcg2_ops should be used for APSS clock RCG, as other ops
>> will not configure the RCG register
> RCG register meaning RCG register*s*, meaning in this case M/N/D
> which would be required for proper rate setting and not only input
> switching (which arguably doesn't seem to be of much concern on a
> single-parent clock)? This all is not obvious..
> 
> Konrad
The source selection is done by configuring the RCGR config register 
with the source entry (P_APSS_PLL_EARLY) added to the frequency table. 
Proper rate is achieved by configuring the PLL and hence M/N/D values 
are not configured
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
>>   drivers/clk/qcom/apss-ipq-pll.c | 1 +
>>   drivers/clk/qcom/apss-ipq6018.c | 8 +++++++-
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
>> index dd0c01bf5a98..75486a124fcd 100644
>> --- a/drivers/clk/qcom/apss-ipq-pll.c
>> +++ b/drivers/clk/qcom/apss-ipq-pll.c
>> @@ -33,6 +33,7 @@ static struct clk_alpha_pll ipq_pll = {
>>   			},
>>   			.num_parents = 1,
>>   			.ops = &clk_alpha_pll_huayra_ops,
>> +			.flags = CLK_IS_CRITICAL,
>>   		},
>>   	},
>>   };
>> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
>> index f2f502e2d5a4..0d0e7196a4dc 100644
>> --- a/drivers/clk/qcom/apss-ipq6018.c
>> +++ b/drivers/clk/qcom/apss-ipq6018.c
>> @@ -33,15 +33,21 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
>>   	{ P_APSS_PLL_EARLY, 5 },
>>   };
>>   
>> +static const struct freq_tbl ftbl_apcs_alias0_clk_src[] = {
>> +	{ .src = P_APSS_PLL_EARLY, .pre_div = 1 },
>> +	{ }
>> +};
>> +
>>   static struct clk_rcg2 apcs_alias0_clk_src = {
>>   	.cmd_rcgr = 0x0050,
>> +	.freq_tbl = ftbl_apcs_alias0_clk_src,
>>   	.hid_width = 5,
>>   	.parent_map = parents_apcs_alias0_clk_src_map,
>>   	.clkr.hw.init = &(struct clk_init_data){
>>   		.name = "apcs_alias0_clk_src",
>>   		.parent_data = parents_apcs_alias0_clk_src,
>>   		.num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
>> -		.ops = &clk_rcg2_mux_closest_ops,
>> +		.ops = &clk_rcg2_ops,
>>   		.flags = CLK_SET_RATE_PARENT,
>>   	},
>>   };
Best Regards,
Devi Priya

Re: [PATCH 6/6] clk: qcom: Fix APSS PLL and RCG Configuration

[Devi Priya]

Thanks for taking time to review the patch!

On 1/13/2023 9:47 PM, Robert Marko wrote:
> 
> On 13. 01. 2023. 16:20, Konrad Dybcio wrote:
>>
>> On 13.01.2023 15:36, devi priya wrote:
>>> Included CLK_IS_CRITICAL flag which helps to properly enable
>>> the APSS PLL during bootup.
>> Please describe the issue and not only the user-visible impact it
>> makes. Does the PLL get shut down by clk_ignore_unused? Maybe you
>> would be interested in the sync_state changes that landed in recent
>> -next that may solve it for you?
>>
>> I don't think it should be always-on, as you have an alternate source
>> for low power modes, adding CLK_IS_CRITICAL will keep the PLL enabled
>> even if you're not using it.
> 
> I have the same opinion, as this is working fine on IPQ6018 and IPQ8074
> and I have not experienced any issues with it.
> 
Sure, will drop the critical flag
>>
>>> clk_rcg2_ops should be used for APSS clock RCG, as other ops
>>> will not configure the RCG register
>> RCG register meaning RCG register*s*, meaning in this case M/N/D
>> which would be required for proper rate setting and not only input
>> switching (which arguably doesn't seem to be of much concern on a
>> single-parent clock)? This all is not obvious..
> Same question from me as well, why do you need clk_rcg2_ops with
> a dummy frequency table since this is just a mux using RCG2 control
> bits?
> 
> Regards,
> Robert
>>
The RCGR is used for source selection whereas the rate setting is done 
by configuring the PLL. The source is configured in the RCGR using the 
source entry (P_APSS_PLL_EARLY) added to the frequency table.

>> Konrad
>>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>>> ---
>>>   drivers/clk/qcom/apss-ipq-pll.c | 1 +
>>>   drivers/clk/qcom/apss-ipq6018.c | 8 +++++++-
>>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c 
>>> b/drivers/clk/qcom/apss-ipq-pll.c
>>> index dd0c01bf5a98..75486a124fcd 100644
>>> --- a/drivers/clk/qcom/apss-ipq-pll.c
>>> +++ b/drivers/clk/qcom/apss-ipq-pll.c
>>> @@ -33,6 +33,7 @@ static struct clk_alpha_pll ipq_pll = {
>>>               },
>>>               .num_parents = 1,
>>>               .ops = &clk_alpha_pll_huayra_ops,
>>> +            .flags = CLK_IS_CRITICAL,
>>>           },
>>>       },
>>>   };
>>> diff --git a/drivers/clk/qcom/apss-ipq6018.c 
>>> b/drivers/clk/qcom/apss-ipq6018.c
>>> index f2f502e2d5a4..0d0e7196a4dc 100644
>>> --- a/drivers/clk/qcom/apss-ipq6018.c
>>> +++ b/drivers/clk/qcom/apss-ipq6018.c
>>> @@ -33,15 +33,21 @@ static const struct parent_map 
>>> parents_apcs_alias0_clk_src_map[] = {
>>>       { P_APSS_PLL_EARLY, 5 },
>>>   };
>>> +static const struct freq_tbl ftbl_apcs_alias0_clk_src[] = {
>>> +    { .src = P_APSS_PLL_EARLY, .pre_div = 1 },
>>> +    { }
>>> +};
>>> +
>>>   static struct clk_rcg2 apcs_alias0_clk_src = {
>>>       .cmd_rcgr = 0x0050,
>>> +    .freq_tbl = ftbl_apcs_alias0_clk_src,
>>>       .hid_width = 5,
>>>       .parent_map = parents_apcs_alias0_clk_src_map,
>>>       .clkr.hw.init = &(struct clk_init_data){
>>>           .name = "apcs_alias0_clk_src",
>>>           .parent_data = parents_apcs_alias0_clk_src,
>>>           .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
>>> -        .ops = &clk_rcg2_mux_closest_ops,
>>> +        .ops = &clk_rcg2_ops,
>>>           .flags = CLK_SET_RATE_PARENT,
>>>       },
>>>   };
>>
Best Regards,
Devi Priya

Re: [PATCH 2/6] clk: qcom: ipq9574: Enable APSS clock driver

[Dmitry Baryshkov]

On 13/01/2023 16:36, devi priya wrote:
> Enable APSS clock driver for IPQ9574 based devices
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---
>   drivers/clk/qcom/apss-ipq-pll.c         | 13 +++++++++++++
>   drivers/mailbox/qcom-apcs-ipc-mailbox.c |  5 +++++

Note, the drivers/mailbox isn't a part of the 'drivers/clk', so it 
should go to a separate patch (and it will be handled by a different 
maintainer).

>   2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> index a5aea27eb867..dd0c01bf5a98 100644
> --- a/drivers/clk/qcom/apss-ipq-pll.c
> +++ b/drivers/clk/qcom/apss-ipq-pll.c
> @@ -61,6 +61,18 @@ static const struct alpha_pll_config ipq8074_pll_config = {
>   	.test_ctl_hi_val = 0x4000,
>   };
>   
> +static const struct alpha_pll_config ipq9574_pll_config = {
> +	.l = 0x3b,
> +	.config_ctl_val = 0x200D4828,
> +	.config_ctl_hi_val = 0x6,
> +	.early_output_mask = BIT(3),
> +	.aux2_output_mask = BIT(2),
> +	.aux_output_mask = BIT(1),
> +	.main_output_mask = BIT(0),
> +	.test_ctl_val = 0x0,
> +	.test_ctl_hi_val = 0x4000,
> +};
> +
>   static const struct regmap_config ipq_pll_regmap_config = {
>   	.reg_bits		= 32,
>   	.reg_stride		= 4,
> @@ -102,6 +114,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>   static const struct of_device_id apss_ipq_pll_match_table[] = {
>   	{ .compatible = "qcom,ipq6018-a53pll", .data = &ipq6018_pll_config },
>   	{ .compatible = "qcom,ipq8074-a53pll", .data = &ipq8074_pll_config },
> +	{ .compatible = "qcom,ipq9574-a73pll", .data = &ipq9574_pll_config },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index 0e9f9cba8668..90e74f9d7cb3 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -33,6 +33,10 @@ static const struct qcom_apcs_ipc_data ipq6018_apcs_data = {
>   	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
>   };
>   
> +static const struct qcom_apcs_ipc_data ipq9574_apcs_data = {
> +	.offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
> +};

As the data is identical to ipq6018's one, please don't add a duplicate 
of it.

> +
>   static const struct qcom_apcs_ipc_data msm8916_apcs_data = {
>   	.offset = 8, .clk_name = "qcom-apcs-msm8916-clk"
>   };
> @@ -143,6 +147,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev)
>   static const struct of_device_id qcom_apcs_ipc_of_match[] = {
>   	{ .compatible = "qcom,ipq6018-apcs-apps-global", .data = &ipq6018_apcs_data },
>   	{ .compatible = "qcom,ipq8074-apcs-apps-global", .data = &ipq6018_apcs_data },
> +	{ .compatible = "qcom,ipq9574-apcs-apps-global", .data = &ipq9574_apcs_data },
>   	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = &msm8916_apcs_data },
>   	{ .compatible = "qcom,msm8939-apcs-kpss-global", .data = &msm8916_apcs_data },
>   	{ .compatible = "qcom,msm8953-apcs-kpss-global", .data = &msm8994_apcs_data },

-- 
With best wishes
Dmitry

Re: [PATCH 3/6] arm64: defconfig: Enable ipq6018 apss clock and PLL controller

[Dmitry Baryshkov]

On 13/01/2023 16:36, devi priya wrote:
> Enable the PLL controller and IPQ6018 APSS clock controller

... it is used on several IPQ platforms to clock the CPU so it should be 
enabled and built-in.

> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>

Just to check: is the capitalization correct in your name here and 
everywhere else? (please excuse my ignorance here, I do not know all the 
spelling/capitalization rules).

> ---
>   arch/arm64/configs/defconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index e0ae0996d5ad..8de3979b10a3 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1092,6 +1092,7 @@ CONFIG_QCOM_CLK_APCS_MSM8916=y
>   CONFIG_QCOM_CLK_APCC_MSM8996=y
>   CONFIG_QCOM_CLK_SMD_RPM=y
>   CONFIG_QCOM_CLK_RPMH=y
> +CONFIG_IPQ_APSS_6018=y
>   CONFIG_IPQ_GCC_6018=y
>   CONFIG_IPQ_GCC_8074=y
>   CONFIG_IPQ_GCC_9574=y

-- 
With best wishes
Dmitry

Re: [PATCH 2/6] clk: qcom: ipq9574: Enable APSS clock driver

[Devi Priya]

Thanks Dmitry for taking time to review the patch!

On 1/31/2023 2:59 PM, Dmitry Baryshkov wrote:
> On 13/01/2023 16:36, devi priya wrote:
>> Enable APSS clock driver for IPQ9574 based devices
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
>>   drivers/clk/qcom/apss-ipq-pll.c         | 13 +++++++++++++
>>   drivers/mailbox/qcom-apcs-ipc-mailbox.c |  5 +++++
> 
> Note, the drivers/mailbox isn't a part of the 'drivers/clk', so it 
> should go to a separate patch (and it will be handled by a different 
> maintainer).
> 
Sure, got it. will split the changes in V2
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c 
>> b/drivers/clk/qcom/apss-ipq-pll.c
>> index a5aea27eb867..dd0c01bf5a98 100644
>> --- a/drivers/clk/qcom/apss-ipq-pll.c
>> +++ b/drivers/clk/qcom/apss-ipq-pll.c
>> @@ -61,6 +61,18 @@ static const struct alpha_pll_config 
>> ipq8074_pll_config = {
>>       .test_ctl_hi_val = 0x4000,
>>   };
>> +static const struct alpha_pll_config ipq9574_pll_config = {
>> +    .l = 0x3b,
>> +    .config_ctl_val = 0x200D4828,
>> +    .config_ctl_hi_val = 0x6,
>> +    .early_output_mask = BIT(3),
>> +    .aux2_output_mask = BIT(2),
>> +    .aux_output_mask = BIT(1),
>> +    .main_output_mask = BIT(0),
>> +    .test_ctl_val = 0x0,
>> +    .test_ctl_hi_val = 0x4000,
>> +};
>> +
>>   static const struct regmap_config ipq_pll_regmap_config = {
>>       .reg_bits        = 32,
>>       .reg_stride        = 4,
>> @@ -102,6 +114,7 @@ static int apss_ipq_pll_probe(struct 
>> platform_device *pdev)
>>   static const struct of_device_id apss_ipq_pll_match_table[] = {
>>       { .compatible = "qcom,ipq6018-a53pll", .data = 
>> &ipq6018_pll_config },
>>       { .compatible = "qcom,ipq8074-a53pll", .data = 
>> &ipq8074_pll_config },
>> +    { .compatible = "qcom,ipq9574-a73pll", .data = 
>> &ipq9574_pll_config },
>>       { }
>>   };
>>   MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
>> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c 
>> b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> index 0e9f9cba8668..90e74f9d7cb3 100644
>> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> @@ -33,6 +33,10 @@ static const struct qcom_apcs_ipc_data 
>> ipq6018_apcs_data = {
>>       .offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
>>   };
>> +static const struct qcom_apcs_ipc_data ipq9574_apcs_data = {
>> +    .offset = 8, .clk_name = "qcom,apss-ipq6018-clk"
>> +};
> 
> As the data is identical to ipq6018's one, please don't add a duplicate 
> of it.
> 
Sure, okay
>> +
>>   static const struct qcom_apcs_ipc_data msm8916_apcs_data = {
>>       .offset = 8, .clk_name = "qcom-apcs-msm8916-clk"
>>   };
>> @@ -143,6 +147,7 @@ static int qcom_apcs_ipc_remove(struct 
>> platform_device *pdev)
>>   static const struct of_device_id qcom_apcs_ipc_of_match[] = {
>>       { .compatible = "qcom,ipq6018-apcs-apps-global", .data = 
>> &ipq6018_apcs_data },
>>       { .compatible = "qcom,ipq8074-apcs-apps-global", .data = 
>> &ipq6018_apcs_data },
>> +    { .compatible = "qcom,ipq9574-apcs-apps-global", .data = 
>> &ipq9574_apcs_data },
>>       { .compatible = "qcom,msm8916-apcs-kpss-global", .data = 
>> &msm8916_apcs_data },
>>       { .compatible = "qcom,msm8939-apcs-kpss-global", .data = 
>> &msm8916_apcs_data },
>>       { .compatible = "qcom,msm8953-apcs-kpss-global", .data = 
>> &msm8994_apcs_data },
> 
Best Regards,
Devi Priya

Re: [PATCH 6/6] clk: qcom: Fix APSS PLL and RCG Configuration

[Dmitry Baryshkov]

On 31/01/2023 11:17, Devi Priya wrote:
> Thanks for taking time to review the patch
> 
> On 1/13/2023 8:50 PM, Konrad Dybcio wrote:
>>
>>
>> On 13.01.2023 15:36, devi priya wrote:
>>> Included CLK_IS_CRITICAL flag which helps to properly enable
>>> the APSS PLL during bootup.
>> Please describe the issue and not only the user-visible impact it
>> makes. Does the PLL get shut down by clk_ignore_unused? Maybe you
>> would be interested in the sync_state changes that landed in recent
>> -next that may solve it for you?
>>
>> I don't think it should be always-on, as you have an alternate source
>> for low power modes, adding CLK_IS_CRITICAL will keep the PLL enabled
>> even if you're not using it.
> Yeah, got it. Will drop the critical flag
>>
>>> clk_rcg2_ops should be used for APSS clock RCG, as other ops
>>> will not configure the RCG register
>> RCG register meaning RCG register*s*, meaning in this case M/N/D
>> which would be required for proper rate setting and not only input
>> switching (which arguably doesn't seem to be of much concern on a
>> single-parent clock)? This all is not obvious..
>>
>> Konrad
> The source selection is done by configuring the RCGR config register 
> with the source entry (P_APSS_PLL_EARLY) added to the frequency table. 
> Proper rate is achieved by configuring the PLL and hence M/N/D values 
> are not configured

But the clk_rcg2_mux_closest_ops also programs the parent for the clock. 
So from your description it isn't obvious what is wrong with the current 
_ops used for the clock.

>>>
>>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>>> ---
>>>   drivers/clk/qcom/apss-ipq-pll.c | 1 +
>>>   drivers/clk/qcom/apss-ipq6018.c | 8 +++++++-
>>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c 
>>> b/drivers/clk/qcom/apss-ipq-pll.c
>>> index dd0c01bf5a98..75486a124fcd 100644
>>> --- a/drivers/clk/qcom/apss-ipq-pll.c
>>> +++ b/drivers/clk/qcom/apss-ipq-pll.c
>>> @@ -33,6 +33,7 @@ static struct clk_alpha_pll ipq_pll = {
>>>               },
>>>               .num_parents = 1,
>>>               .ops = &clk_alpha_pll_huayra_ops,
>>> +            .flags = CLK_IS_CRITICAL,
>>>           },
>>>       },
>>>   };
>>> diff --git a/drivers/clk/qcom/apss-ipq6018.c 
>>> b/drivers/clk/qcom/apss-ipq6018.c
>>> index f2f502e2d5a4..0d0e7196a4dc 100644
>>> --- a/drivers/clk/qcom/apss-ipq6018.c
>>> +++ b/drivers/clk/qcom/apss-ipq6018.c
>>> @@ -33,15 +33,21 @@ static const struct parent_map 
>>> parents_apcs_alias0_clk_src_map[] = {
>>>       { P_APSS_PLL_EARLY, 5 },
>>>   };
>>> +static const struct freq_tbl ftbl_apcs_alias0_clk_src[] = {
>>> +    { .src = P_APSS_PLL_EARLY, .pre_div = 1 },
>>> +    { }
>>> +};
>>> +
>>>   static struct clk_rcg2 apcs_alias0_clk_src = {
>>>       .cmd_rcgr = 0x0050,
>>> +    .freq_tbl = ftbl_apcs_alias0_clk_src,
>>>       .hid_width = 5,
>>>       .parent_map = parents_apcs_alias0_clk_src_map,
>>>       .clkr.hw.init = &(struct clk_init_data){
>>>           .name = "apcs_alias0_clk_src",
>>>           .parent_data = parents_apcs_alias0_clk_src,
>>>           .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
>>> -        .ops = &clk_rcg2_mux_closest_ops,
>>> +        .ops = &clk_rcg2_ops,
>>>           .flags = CLK_SET_RATE_PARENT,
>>>       },
>>>   };
> Best Regards,
> Devi Priya

-- 
With best wishes
Dmitry

Re: [PATCH 3/6] arm64: defconfig: Enable ipq6018 apss clock and PLL controller

[Devi Priya]

Thanks for taking time to review the patch

On 1/31/2023 3:01 PM, Dmitry Baryshkov wrote:
> On 13/01/2023 16:36, devi priya wrote:
>> Enable the PLL controller and IPQ6018 APSS clock controller
> 
> ... it is used on several IPQ platforms to clock the CPU so it should be 
> enabled and built-in.
> 
Okay, got it. Will update the commit message as suggested in V2
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> 
> Just to check: is the capitalization correct in your name here and 
> everywhere else? (please excuse my ignorance here, I do not know all the 
> spelling/capitalization rules).
> 
Sure, will change the naming style to Camel Case
>> ---
>>   arch/arm64/configs/defconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index e0ae0996d5ad..8de3979b10a3 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -1092,6 +1092,7 @@ CONFIG_QCOM_CLK_APCS_MSM8916=y
>>   CONFIG_QCOM_CLK_APCC_MSM8996=y
>>   CONFIG_QCOM_CLK_SMD_RPM=y
>>   CONFIG_QCOM_CLK_RPMH=y
>> +CONFIG_IPQ_APSS_6018=y
>>   CONFIG_IPQ_GCC_6018=y
>>   CONFIG_IPQ_GCC_8074=y
>>   CONFIG_IPQ_GCC_9574=y
> 
Best Regards,
Devi Priya

Re: [PATCH 6/6] clk: qcom: Fix APSS PLL and RCG Configuration

[Devi Priya]

On 1/31/2023 3:10 PM, Dmitry Baryshkov wrote:
> On 31/01/2023 11:17, Devi Priya wrote:
>> Thanks for taking time to review the patch
>>
>> On 1/13/2023 8:50 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 13.01.2023 15:36, devi priya wrote:
>>>> Included CLK_IS_CRITICAL flag which helps to properly enable
>>>> the APSS PLL during bootup.
>>> Please describe the issue and not only the user-visible impact it
>>> makes. Does the PLL get shut down by clk_ignore_unused? Maybe you
>>> would be interested in the sync_state changes that landed in recent
>>> -next that may solve it for you?
>>>
>>> I don't think it should be always-on, as you have an alternate source
>>> for low power modes, adding CLK_IS_CRITICAL will keep the PLL enabled
>>> even if you're not using it.
>> Yeah, got it. Will drop the critical flag
>>>
>>>> clk_rcg2_ops should be used for APSS clock RCG, as other ops
>>>> will not configure the RCG register
>>> RCG register meaning RCG register*s*, meaning in this case M/N/D
>>> which would be required for proper rate setting and not only input
>>> switching (which arguably doesn't seem to be of much concern on a
>>> single-parent clock)? This all is not obvious..
>>>
>>> Konrad
>> The source selection is done by configuring the RCGR config register 
>> with the source entry (P_APSS_PLL_EARLY) added to the frequency table. 
>> Proper rate is achieved by configuring the PLL and hence M/N/D values 
>> are not configured
> 
> But the clk_rcg2_mux_closest_ops also programs the parent for the clock. 
> So from your description it isn't obvious what is wrong with the current 
> _ops used for the clock.
> 
Okay, understood & agreed.
Will re-verify it once and update it accordingly in V2
>>>>
>>>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>>>> ---
>>>>   drivers/clk/qcom/apss-ipq-pll.c | 1 +
>>>>   drivers/clk/qcom/apss-ipq6018.c | 8 +++++++-
>>>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c 
>>>> b/drivers/clk/qcom/apss-ipq-pll.c
>>>> index dd0c01bf5a98..75486a124fcd 100644
>>>> --- a/drivers/clk/qcom/apss-ipq-pll.c
>>>> +++ b/drivers/clk/qcom/apss-ipq-pll.c
>>>> @@ -33,6 +33,7 @@ static struct clk_alpha_pll ipq_pll = {
>>>>               },
>>>>               .num_parents = 1,
>>>>               .ops = &clk_alpha_pll_huayra_ops,
>>>> +            .flags = CLK_IS_CRITICAL,
>>>>           },
>>>>       },
>>>>   };
>>>> diff --git a/drivers/clk/qcom/apss-ipq6018.c 
>>>> b/drivers/clk/qcom/apss-ipq6018.c
>>>> index f2f502e2d5a4..0d0e7196a4dc 100644
>>>> --- a/drivers/clk/qcom/apss-ipq6018.c
>>>> +++ b/drivers/clk/qcom/apss-ipq6018.c
>>>> @@ -33,15 +33,21 @@ static const struct parent_map 
>>>> parents_apcs_alias0_clk_src_map[] = {
>>>>       { P_APSS_PLL_EARLY, 5 },
>>>>   };
>>>> +static const struct freq_tbl ftbl_apcs_alias0_clk_src[] = {
>>>> +    { .src = P_APSS_PLL_EARLY, .pre_div = 1 },
>>>> +    { }
>>>> +};
>>>> +
>>>>   static struct clk_rcg2 apcs_alias0_clk_src = {
>>>>       .cmd_rcgr = 0x0050,
>>>> +    .freq_tbl = ftbl_apcs_alias0_clk_src,
>>>>       .hid_width = 5,
>>>>       .parent_map = parents_apcs_alias0_clk_src_map,
>>>>       .clkr.hw.init = &(struct clk_init_data){
>>>>           .name = "apcs_alias0_clk_src",
>>>>           .parent_data = parents_apcs_alias0_clk_src,
>>>>           .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
>>>> -        .ops = &clk_rcg2_mux_closest_ops,
>>>> +        .ops = &clk_rcg2_ops,
>>>>           .flags = CLK_SET_RATE_PARENT,
>>>>       },
>>>>   };
>> Best Regards,
>> Devi Priya
> 
Best Regards,
Devi Priya