[PATCH 0/5] clk: qcom: Add support to attach multiple power domains in cc probe

[PATCH 0/5] clk: qcom: Add support to attach multiple power domains in cc probe

[Jagadeesh Kona]

During boot-up, the PLL configuration might be missed even after
calling pll_configure() from the clock controller probe. This can
happen because the PLL is connected to one or more rails that
are turned off, and the current clock controller code cannot
enable multiple rails during probe. Consequently, the PLL may
be activated with suboptimal settings, causing functional issues.

The support to attach multiple power domains to clock controllers
was recently added in Bryan's series[1] but it currently doesn't
enable all clock controller power domains during probe which are
needed for PLL configuration.

This series adds required support to enable the multiple power
domains during clock controllers probe and adds support to enable
both MMCX & MXC rails during probe for videocc on SM8450, SM8475,
SM8550 and SM8650 platforms to configure the video PLLs properly.

This fixes the below warning reported in SM8550 venus testing due
to video_cc_pll0 not properly getting configured during videocc probe

[   46.535132] Lucid PLL latch failed. Output may be unstable!

[1]: https://lore.kernel.org/all/20250117-b4-linux-next-24-11-18-clock-multiple-power-domains-v10-0-13f2bb656dad@linaro.org/ 

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
Jagadeesh Kona (4):
      dt-bindings: clock: qcom,sm8450-videocc: Add MXC power domain
      clk: qcom: common: Attach clock power domains conditionally
      clk: qcom: videocc: Add support to attach multiple power domains
      arm64: dts: qcom: Add MXC power domain to videocc nodes

Taniya Das (1):
      clk: qcom: common: Add support to attach multiple power domains

 .../bindings/clock/qcom,sm8450-videocc.yaml         |  9 ++++++---
 arch/arm64/boot/dts/qcom/sm8450.dtsi                |  3 ++-
 arch/arm64/boot/dts/qcom/sm8550.dtsi                |  3 ++-
 arch/arm64/boot/dts/qcom/sm8650.dtsi                |  3 ++-
 drivers/clk/qcom/common.c                           | 21 ++++++++++++++++++---
 drivers/clk/qcom/common.h                           |  2 ++
 drivers/clk/qcom/videocc-sm8450.c                   |  4 ++++
 drivers/clk/qcom/videocc-sm8550.c                   |  4 ++++
 8 files changed, 40 insertions(+), 9 deletions(-)
---
base-commit: e5d3fd687aac5eceb1721fa92b9f49afcf4c3717
change-id: 20250218-videocc-pll-multi-pd-voting-d614dce910e7

Best regards,
-- 
Jagadeesh Kona <quic_jkona@quicinc.com>

[PATCH 1/5] dt-bindings: clock: qcom,sm8450-videocc: Add MXC power domain

[Jagadeesh Kona]

To configure the video PLLs and enable the video GDSCs on SM8450,
SM8475, SM8550 and SM8650 platforms, the MXC rail must be ON along
with MMCX. Therefore, update the videocc bindings to include
the MXC power domain on these platforms.

Fixes: 1e910b2ba0ed ("dt-bindings: clock: qcom: Add SM8450 video clock controller")
Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
index 62714fa54db82491a7a108f7f18a253d737f8d61..737efc4b46564c1e475b02873d2dc124329fb775 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
@@ -32,9 +32,11 @@ properties:
       - description: Video AHB clock from GCC
 
   power-domains:
-    maxItems: 1
     description:
-      MMCX power domain.
+      Power domains required for the clock controller to operate
+    items:
+      - description: MMCX power domain
+      - description: MXC power domain
 
   required-opps:
     maxItems: 1
@@ -72,7 +74,8 @@ examples:
       reg = <0x0aaf0000 0x10000>;
       clocks = <&rpmhcc RPMH_CXO_CLK>,
                <&gcc GCC_VIDEO_AHB_CLK>;
-      power-domains = <&rpmhpd RPMHPD_MMCX>;
+      power-domains = <&rpmhpd RPMHPD_MMCX>,
+                      <&rpmhpd RPMHPD_MXC>;
       required-opps = <&rpmhpd_opp_low_svs>;
       #clock-cells = <1>;
       #reset-cells = <1>;

-- 
2.34.1

[PATCH 2/5] clk: qcom: common: Add support to attach multiple power domains

[Jagadeesh Kona]

From: Taniya Das <quic_tdas@quicinc.com>

In the latest chipset clock controllers, multiple power domains
can be required to access and configure the PLLs in probe. Therefore,
add support for an API to attach to multiple power domains in the
clock controller probe before configuring the PLLs.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 drivers/clk/qcom/common.c | 12 ++++++++++++
 drivers/clk/qcom/common.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 9e3380fd718198c9fe63d7361615a91c3ecb3d60..ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -391,5 +391,17 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
 }
 EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
 
+int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc)
+{
+	int ret;
+
+	ret = devm_pm_domain_attach_list(dev, NULL, &desc->pd_list);
+	if (ret < 0 && ret != -EEXIST)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(qcom_cc_attach_pds);
+
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("QTI Common Clock module");
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 7ace5d7f5836aa81431153ba92d8f14f2ffe8147..45f1f53fb407d4600f5059b792564b68cd8c244d 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -38,6 +38,7 @@ struct qcom_cc_desc {
 	const struct qcom_icc_hws_data *icc_hws;
 	size_t num_icc_hws;
 	unsigned int icc_first_node_id;
+	struct dev_pm_domain_list *pd_list;
 };
 
 /**
@@ -76,5 +77,6 @@ extern int qcom_cc_probe(struct platform_device *pdev,
 			 const struct qcom_cc_desc *desc);
 extern int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
 				  const struct qcom_cc_desc *desc);
+extern int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc);
 
 #endif

-- 
2.34.1

[PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Jagadeesh Kona]

Attach clock power domains in qcom_cc_really_probe() only
if the clock controller has not already attached to them.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 drivers/clk/qcom/common.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
 	if (!cc)
 		return -ENOMEM;
 
-	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
-	if (ret < 0 && ret != -EEXIST)
-		return ret;
+	cc->pd_list = desc->pd_list;
+	if (!cc->pd_list) {
+		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
+		if (ret < 0 && ret != -EEXIST)
+			return ret;
+	}
 
 	reset = &cc->reset;
 	reset->rcdev.of_node = dev->of_node;

-- 
2.34.1

[PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

During boot-up, the PLL configuration might be missed even after
calling pll_configure() from the clock controller probe. This can
happen because the PLL is connected to one or more rails that are
turned off, and the current clock controller code cannot enable
multiple rails during probe. Consequently, the PLL may be activated
with suboptimal settings, causing functional issues.

To properly configure the video PLLs in the probe on SM8450, SM8475,
SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
Therefore, add support to attach multiple power domains to videocc on
these platforms.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 drivers/clk/qcom/videocc-sm8450.c | 4 ++++
 drivers/clk/qcom/videocc-sm8550.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
--- a/drivers/clk/qcom/videocc-sm8450.c
+++ b/drivers/clk/qcom/videocc-sm8450.c
@@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	int ret;
 
+	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
+	if (ret)
+		return ret;
+
 	ret = devm_pm_runtime_enable(&pdev->dev);
 	if (ret)
 		return ret;
diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
--- a/drivers/clk/qcom/videocc-sm8550.c
+++ b/drivers/clk/qcom/videocc-sm8550.c
@@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
 	int ret;
 	u32 sleep_clk_offset = 0x8140;
 
+	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
+	if (ret)
+		return ret;
+
 	ret = devm_pm_runtime_enable(&pdev->dev);
 	if (ret)
 		return ret;

-- 
2.34.1

[PATCH 5/5] arm64: dts: qcom: Add MXC power domain to videocc nodes

[Jagadeesh Kona]

Videocc requires both MMCX and MXC rails to be powered ON
to configure the video PLLs on SM8450, SM8550 and SM8650
platforms. Hence add MXC power domain to videocc node on
these platforms.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ++-
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 9c809fc5fa45a98ff5441a0b6809931588897243..4f8dca8fc64212191780067c5d8815e3a2bb137f 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -3136,7 +3136,8 @@ videocc: clock-controller@aaf0000 {
 			reg = <0 0x0aaf0000 0 0x10000>;
 			clocks = <&rpmhcc RPMH_CXO_CLK>,
 				 <&gcc GCC_VIDEO_AHB_CLK>;
-			power-domains = <&rpmhpd RPMHPD_MMCX>;
+			power-domains = <&rpmhpd RPMHPD_MMCX>,
+					<&rpmhpd RPMHPD_MXC>;
 			required-opps = <&rpmhpd_opp_low_svs>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index eac8de4005d82f246bc50f64f09515631d895c99..a039ae71e1b7bba8124128d19de5e00c65217770 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -2889,7 +2889,8 @@ videocc: clock-controller@aaf0000 {
 			reg = <0 0x0aaf0000 0 0x10000>;
 			clocks = <&bi_tcxo_div2>,
 				 <&gcc GCC_VIDEO_AHB_CLK>;
-			power-domains = <&rpmhpd RPMHPD_MMCX>;
+			power-domains = <&rpmhpd RPMHPD_MMCX>,
+					<&rpmhpd RPMHPD_MXC>;
 			required-opps = <&rpmhpd_opp_low_svs>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 86684cb9a9325618ddb74458621cf4bbdc1cc0d1..32af2a0f7a0030f155b7d8c93faeffa384a42768 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -3524,7 +3524,8 @@ videocc: clock-controller@aaf0000 {
 			reg = <0 0x0aaf0000 0 0x10000>;
 			clocks = <&bi_tcxo_div2>,
 				 <&gcc GCC_VIDEO_AHB_CLK>;
-			power-domains = <&rpmhpd RPMHPD_MMCX>;
+			power-domains = <&rpmhpd RPMHPD_MMCX>,
+					<&rpmhpd RPMHPD_MXC>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;

-- 
2.34.1

Re: [PATCH 5/5] arm64: dts: qcom: Add MXC power domain to videocc nodes

[Bryan O'Donoghue]

On 18/02/2025 14:26, Jagadeesh Kona wrote:
> Videocc requires both MMCX and MXC rails to be powered ON
> to configure the video PLLs on SM8450, SM8550 and SM8650
> platforms. Hence add MXC power domain to videocc node on
> these platforms.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ++-
>   arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++-
>   3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 9c809fc5fa45a98ff5441a0b6809931588897243..4f8dca8fc64212191780067c5d8815e3a2bb137f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -3136,7 +3136,8 @@ videocc: clock-controller@aaf0000 {
>   			reg = <0 0x0aaf0000 0 0x10000>;
>   			clocks = <&rpmhcc RPMH_CXO_CLK>,
>   				 <&gcc GCC_VIDEO_AHB_CLK>;
> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
> +					<&rpmhpd RPMHPD_MXC>;
>   			required-opps = <&rpmhpd_opp_low_svs>;
>   			#clock-cells = <1>;
>   			#reset-cells = <1>;
> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index eac8de4005d82f246bc50f64f09515631d895c99..a039ae71e1b7bba8124128d19de5e00c65217770 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> @@ -2889,7 +2889,8 @@ videocc: clock-controller@aaf0000 {
>   			reg = <0 0x0aaf0000 0 0x10000>;
>   			clocks = <&bi_tcxo_div2>,
>   				 <&gcc GCC_VIDEO_AHB_CLK>;
> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
> +					<&rpmhpd RPMHPD_MXC>;
>   			required-opps = <&rpmhpd_opp_low_svs>;
>   			#clock-cells = <1>;
>   			#reset-cells = <1>;
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 86684cb9a9325618ddb74458621cf4bbdc1cc0d1..32af2a0f7a0030f155b7d8c93faeffa384a42768 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -3524,7 +3524,8 @@ videocc: clock-controller@aaf0000 {
>   			reg = <0 0x0aaf0000 0 0x10000>;
>   			clocks = <&bi_tcxo_div2>,
>   				 <&gcc GCC_VIDEO_AHB_CLK>;
> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
> +					<&rpmhpd RPMHPD_MXC>;
>   			#clock-cells = <1>;
>   			#reset-cells = <1>;
>   			#power-domain-cells = <1>;
> 

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 18/02/2025 14:26, Jagadeesh Kona wrote:
> During boot-up, the PLL configuration might be missed even after
> calling pll_configure() from the clock controller probe. This can
> happen because the PLL is connected to one or more rails that are
> turned off, and the current clock controller code cannot enable
> multiple rails during probe. Consequently, the PLL may be activated
> with suboptimal settings, causing functional issues.
> 
> To properly configure the video PLLs in the probe on SM8450, SM8475,
> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
> Therefore, add support to attach multiple power domains to videocc on
> these platforms.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>   2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
> --- a/drivers/clk/qcom/videocc-sm8450.c
> +++ b/drivers/clk/qcom/videocc-sm8450.c
> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>   	struct regmap *regmap;
>   	int ret;
>   
> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
> +	if (ret)
> +		return ret;
> +
>   	ret = devm_pm_runtime_enable(&pdev->dev);
>   	if (ret)
>   		return ret;
> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
> --- a/drivers/clk/qcom/videocc-sm8550.c
> +++ b/drivers/clk/qcom/videocc-sm8550.c
> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>   	int ret;
>   	u32 sleep_clk_offset = 0x8140;
>   
> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
> +	if (ret)
> +		return ret;
> +
>   	ret = devm_pm_runtime_enable(&pdev->dev);
>   	if (ret)
>   		return ret;
> 

What's the difference between doing the attach here or doing it in 
really_probe() ?

There doesn't seem to be any difference except that we will have an 
additional delay introduced.

Are you describing a race condition ?

I don't see _logic_ here to moving the call into the controller's higher 
level probe.

Can you describe some more ?

---
bod

Re: [PATCH 1/5] dt-bindings: clock: qcom,sm8450-videocc: Add MXC power domain

[Bryan O'Donoghue]

On 18/02/2025 14:26, Jagadeesh Kona wrote:
> To configure the video PLLs and enable the video GDSCs on SM8450,
> SM8475, SM8550 and SM8650 platforms, the MXC rail must be ON along
> with MMCX. Therefore, update the videocc bindings to include
> the MXC power domain on these platforms.
> 
> Fixes: 1e910b2ba0ed ("dt-bindings: clock: qcom: Add SM8450 video clock controller")
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>   Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> index 62714fa54db82491a7a108f7f18a253d737f8d61..737efc4b46564c1e475b02873d2dc124329fb775 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> @@ -32,9 +32,11 @@ properties:
>         - description: Video AHB clock from GCC
>   
>     power-domains:
> -    maxItems: 1
>       description:
> -      MMCX power domain.
> +      Power domains required for the clock controller to operate
> +    items:
> +      - description: MMCX power domain
> +      - description: MXC power domain
>   
>     required-opps:
>       maxItems: 1
> @@ -72,7 +74,8 @@ examples:
>         reg = <0x0aaf0000 0x10000>;
>         clocks = <&rpmhcc RPMH_CXO_CLK>,
>                  <&gcc GCC_VIDEO_AHB_CLK>;
> -      power-domains = <&rpmhpd RPMHPD_MMCX>;
> +      power-domains = <&rpmhpd RPMHPD_MMCX>,
> +                      <&rpmhpd RPMHPD_MXC>;
>         required-opps = <&rpmhpd_opp_low_svs>;
>         #clock-cells = <1>;
>         #reset-cells = <1>;
> 

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 18/02/2025 15:46, Bryan O'Donoghue wrote:
>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>> +    if (ret)
>> +        return ret;
>> +
>>       ret = devm_pm_runtime_enable(&pdev->dev);
>>       if (ret)
>>           return ret;
>>
> 
> What's the difference between doing the attach here or doing it in 
> really_probe() ?
> 
> There doesn't seem to be any difference except that we will have an 
> additional delay introduced.
> 
> Are you describing a race condition ?
> 
> I don't see _logic_ here to moving the call into the controller's higher 
> level probe.

I see you're saying do this before waking up the local PLLs prior to 
really_probe.

hmm.. the existing code works for me on the CRD but not on the Dell 
Insprion14 so I missed this on my series.

Instead of pushing qcom_cc_attach_pds() back up one level and having to 
do that over and over again for each clock controller that has multiple 
power domains, we could just move the configure pll logic to a callback.

Add a new callback to qcom_cc_desc()->configure_plls()

Then qcom_cc_really_probe() would look like:

ret = devm_pm_domain_attach_list();
if (ret < 0 && ret != -EEXIST);
	return ret;

desc->configure_plls();

There's no point in having devm_pm_domain_attach_list() twice within < 
20 LOC we should just force the sequencing in the right order once and 
IMO do it in really_probe() so that we don't keep adding custom logic to 
multi-pd controllers.

Its a generic problem we can solve closer to the core logic.

---
bod

Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Dmitry Baryshkov]

On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
> Attach clock power domains in qcom_cc_really_probe() only
> if the clock controller has not already attached to them.

Squash this to the previous patch and call the new function. No need to
duplicate the code.

> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>  drivers/clk/qcom/common.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
>  	if (!cc)
>  		return -ENOMEM;
>  
> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> -	if (ret < 0 && ret != -EEXIST)
> -		return ret;
> +	cc->pd_list = desc->pd_list;
> +	if (!cc->pd_list) {
> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> +		if (ret < 0 && ret != -EEXIST)
> +			return ret;
> +	}
>  
>  	reset = &cc->reset;
>  	reset->rcdev.of_node = dev->of_node;
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Dmitry Baryshkov]

On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
> On 18/02/2025 14:26, Jagadeesh Kona wrote:
> > During boot-up, the PLL configuration might be missed even after
> > calling pll_configure() from the clock controller probe. This can
> > happen because the PLL is connected to one or more rails that are
> > turned off, and the current clock controller code cannot enable
> > multiple rails during probe. Consequently, the PLL may be activated
> > with suboptimal settings, causing functional issues.
> > 
> > To properly configure the video PLLs in the probe on SM8450, SM8475,
> > SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
> > Therefore, add support to attach multiple power domains to videocc on
> > these platforms.
> > 
> > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> > ---
> >   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
> >   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
> >   2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
> > index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
> > --- a/drivers/clk/qcom/videocc-sm8450.c
> > +++ b/drivers/clk/qcom/videocc-sm8450.c
> > @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
> >   	struct regmap *regmap;
> >   	int ret;
> > +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
> > +	if (ret)
> > +		return ret;
> > +
> >   	ret = devm_pm_runtime_enable(&pdev->dev);
> >   	if (ret)
> >   		return ret;
> > diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
> > index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
> > --- a/drivers/clk/qcom/videocc-sm8550.c
> > +++ b/drivers/clk/qcom/videocc-sm8550.c
> > @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
> >   	int ret;
> >   	u32 sleep_clk_offset = 0x8140;
> > +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
> > +	if (ret)
> > +		return ret;
> > +
> >   	ret = devm_pm_runtime_enable(&pdev->dev);
> >   	if (ret)
> >   		return ret;
> > 
> 
> What's the difference between doing the attach here or doing it in
> really_probe() ?

I'd second this. If the domains are to be attached before calling any
other functions, move the call to the qcom_cc_map(), so that all drivers
get all domains attached before configuring PLLs instead of manually
calling the function.

> There doesn't seem to be any difference except that we will have an
> additional delay introduced.
> 
> Are you describing a race condition ?
> 
> I don't see _logic_ here to moving the call into the controller's higher
> level probe.
> 
> Can you describe some more ?
> 
> ---
> bod

-- 
With best wishes
Dmitry

Re: [PATCH 5/5] arm64: dts: qcom: Add MXC power domain to videocc nodes

[Dmitry Baryshkov]

On Tue, Feb 18, 2025 at 07:56:50PM +0530, Jagadeesh Kona wrote:
> Videocc requires both MMCX and MXC rails to be powered ON
> to configure the video PLLs on SM8450, SM8550 and SM8650
> platforms. Hence add MXC power domain to videocc node on
> these platforms.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ++-
>  arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)

Three separate patches, please. With that in mind:

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

> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 9c809fc5fa45a98ff5441a0b6809931588897243..4f8dca8fc64212191780067c5d8815e3a2bb137f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -3136,7 +3136,8 @@ videocc: clock-controller@aaf0000 {
>  			reg = <0 0x0aaf0000 0 0x10000>;
>  			clocks = <&rpmhcc RPMH_CXO_CLK>,
>  				 <&gcc GCC_VIDEO_AHB_CLK>;
> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
> +					<&rpmhpd RPMHPD_MXC>;
>  			required-opps = <&rpmhpd_opp_low_svs>;
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index eac8de4005d82f246bc50f64f09515631d895c99..a039ae71e1b7bba8124128d19de5e00c65217770 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> @@ -2889,7 +2889,8 @@ videocc: clock-controller@aaf0000 {
>  			reg = <0 0x0aaf0000 0 0x10000>;
>  			clocks = <&bi_tcxo_div2>,
>  				 <&gcc GCC_VIDEO_AHB_CLK>;
> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
> +					<&rpmhpd RPMHPD_MXC>;
>  			required-opps = <&rpmhpd_opp_low_svs>;
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 86684cb9a9325618ddb74458621cf4bbdc1cc0d1..32af2a0f7a0030f155b7d8c93faeffa384a42768 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -3524,7 +3524,8 @@ videocc: clock-controller@aaf0000 {
>  			reg = <0 0x0aaf0000 0 0x10000>;
>  			clocks = <&bi_tcxo_div2>,
>  				 <&gcc GCC_VIDEO_AHB_CLK>;
> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
> +					<&rpmhpd RPMHPD_MXC>;
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
>  			#power-domain-cells = <1>;
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 18/02/2025 17:19, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>> During boot-up, the PLL configuration might be missed even after
>>> calling pll_configure() from the clock controller probe. This can
>>> happen because the PLL is connected to one or more rails that are
>>> turned off, and the current clock controller code cannot enable
>>> multiple rails during probe. Consequently, the PLL may be activated
>>> with suboptimal settings, causing functional issues.
>>>
>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>> Therefore, add support to attach multiple power domains to videocc on
>>> these platforms.
>>>
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>> ---
>>>    drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>    drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>    2 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>    	struct regmap *regmap;
>>>    	int ret;
>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>    	ret = devm_pm_runtime_enable(&pdev->dev);
>>>    	if (ret)
>>>    		return ret;
>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>    	int ret;
>>>    	u32 sleep_clk_offset = 0x8140;
>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>    	ret = devm_pm_runtime_enable(&pdev->dev);
>>>    	if (ret)
>>>    		return ret;
>>>
>>
>> What's the difference between doing the attach here or doing it in
>> really_probe() ?
> 
> I'd second this. If the domains are to be attached before calling any
> other functions, move the call to the qcom_cc_map(), so that all drivers
> get all domains attached before configuring PLLs instead of manually
> calling the function.
> 
>> There doesn't seem to be any difference except that we will have an
>> additional delay introduced.
>>
>> Are you describing a race condition ?
>>
>> I don't see _logic_ here to moving the call into the controller's higher
>> level probe.
>>
>> Can you describe some more ?
>>
>> ---
>> bod
> 

Here's one way this could work

Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Date:   Tue Feb 18 19:46:55 2025 +0000

     clk: qcom: common: Add configure_plls callback prototype

     Add a configure_plls() callback so that we can stage 
qcom_cc_attach_pds()
     before configuring PLLs and ensure that the power-domain rail list is
     switched on prior to configuring PLLs.

     Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 9e3380fd71819..1924130814600 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
         if (ret < 0 && ret != -EEXIST)
                 return ret;

+       if (desc->configure_plls)
+               desc->configure_plls(regmap);
+
         reset = &cc->reset;
         reset->rcdev.of_node = dev->of_node;
         reset->rcdev.ops = &qcom_reset_ops;
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 7ace5d7f5836a..4955085ff8669 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -38,6 +38,7 @@ struct qcom_cc_desc {
         const struct qcom_icc_hws_data *icc_hws;
         size_t num_icc_hws;
         unsigned int icc_first_node_id;
+       void (*configure_plls)(struct regmap *regmap);
  };

and

% git diff drivers/clk/qcom/camcc-x1e80100.c
diff --git a/drivers/clk/qcom/camcc-x1e80100.c 
b/drivers/clk/qcom/camcc-x1e80100.c
index b73524ae64b1b..c9748d1f8a15b 100644
--- a/drivers/clk/qcom/camcc-x1e80100.c
+++ b/drivers/clk/qcom/camcc-x1e80100.c
@@ -2426,6 +2426,21 @@ static const struct regmap_config 
cam_cc_x1e80100_regmap_config = {
         .fast_io = true,
  };

+static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
+{
+       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, 
&cam_cc_pll0_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, 
&cam_cc_pll1_config);
+       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, 
&cam_cc_pll2_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, 
&cam_cc_pll3_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, 
&cam_cc_pll4_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, 
&cam_cc_pll6_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, 
&cam_cc_pll8_config);
+
+       /* Keep clocks always enabled */
+       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
+       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
+}
+
  static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
         .config = &cam_cc_x1e80100_regmap_config,
         .clks = cam_cc_x1e80100_clocks,
@@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc 
cam_cc_x1e80100_desc = {
         .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
         .gdscs = cam_cc_x1e80100_gdscs,
         .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
+       .configure_plls = cam_cc_x1e80100_configure_plls,
  };

  static const struct of_device_id cam_cc_x1e80100_match_table[] = {
@@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct 
platform_device *pdev)
                 return PTR_ERR(regmap);
         }

-       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, 
&cam_cc_pll0_config);
-       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, 
&cam_cc_pll1_config);
-       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, 
&cam_cc_pll2_config);
-       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, 
&cam_cc_pll3_config);
-       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, 
&cam_cc_pll4_config);
-       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, 
&cam_cc_pll6_config);
-       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, 
&cam_cc_pll8_config);
-
-       /* Keep clocks always enabled */
-       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
-       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
-
         ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, 
regmap);

         pm_runtime_put(&pdev->dev);

Or a least it works for me.

New clock controllers would then use this callback mechanism and 
potentially all of the controllers to have uniformity.

---
bod

Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Jagadeesh Kona]

On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
>> Attach clock power domains in qcom_cc_really_probe() only
>> if the clock controller has not already attached to them.
> 
> Squash this to the previous patch and call the new function. No need to
> duplicate the code.
> 

I tried calling the new function here instead of duplicating code, but that
is leading to below warning since the desc passed to qcom_cc_really_probe()
has a const qualifier and hence we cannot update desc->pd_list inside
qcom_cc_really_probe().

drivers/clk/qcom/common.c:305:33:   WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

Thanks,
Jagadeesh

>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
>>  drivers/clk/qcom/common.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
>> --- a/drivers/clk/qcom/common.c
>> +++ b/drivers/clk/qcom/common.c
>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
>>  	if (!cc)
>>  		return -ENOMEM;
>>  
>> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>> -	if (ret < 0 && ret != -EEXIST)
>> -		return ret;
>> +	cc->pd_list = desc->pd_list;
>> +	if (!cc->pd_list) {
>> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>> +		if (ret < 0 && ret != -EEXIST)
>> +			return ret;
>> +	}
>>  
>>  	reset = &cc->reset;
>>  	reset->rcdev.of_node = dev->of_node;
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

On 2/18/2025 10:49 PM, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>> During boot-up, the PLL configuration might be missed even after
>>> calling pll_configure() from the clock controller probe. This can
>>> happen because the PLL is connected to one or more rails that are
>>> turned off, and the current clock controller code cannot enable
>>> multiple rails during probe. Consequently, the PLL may be activated
>>> with suboptimal settings, causing functional issues.
>>>
>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>> Therefore, add support to attach multiple power domains to videocc on
>>> these platforms.
>>>
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>> ---
>>>   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>   	struct regmap *regmap;
>>>   	int ret;
>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>   	ret = devm_pm_runtime_enable(&pdev->dev);
>>>   	if (ret)
>>>   		return ret;
>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>   	int ret;
>>>   	u32 sleep_clk_offset = 0x8140;
>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>   	ret = devm_pm_runtime_enable(&pdev->dev);
>>>   	if (ret)
>>>   		return ret;
>>>
>>
>> What's the difference between doing the attach here or doing it in
>> really_probe() ?
> 
> I'd second this. If the domains are to be attached before calling any
> other functions, move the call to the qcom_cc_map(), so that all drivers
> get all domains attached before configuring PLLs instead of manually
> calling the function.
> 

I earlier tried moving the attach PDs call to qcom_cc_map(), but I faced the below issues
1. desc passed to qcom_cc_map() has const qualifier, so updating desc->pd_list
   inside qcom_cc_map() is leading to a warning.
2. If we attach the PDs after calling get_sync() on device, I observed
   that PDS are not getting enabled during probe. Currently qcom_cc_map()
   is called after get_sync() is already called on device.

Probably, we can add a new function qcom_cc_attach_pds_map() where we can
attach PDs and call qcom_cc_map() inside it. We can then invoke this new
function at the start of probe before get_sync(). I will post this change
in next version if this aligns with your thoughts.

Thanks,
Jagadeesh

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>> During boot-up, the PLL configuration might be missed even after
>>>> calling pll_configure() from the clock controller probe. This can
>>>> happen because the PLL is connected to one or more rails that are
>>>> turned off, and the current clock controller code cannot enable
>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>> with suboptimal settings, causing functional issues.
>>>>
>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>> Therefore, add support to attach multiple power domains to videocc on
>>>> these platforms.
>>>>
>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>> ---
>>>>    drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>    drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>    2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>        struct regmap *regmap;
>>>>        int ret;
>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>>        ret = devm_pm_runtime_enable(&pdev->dev);
>>>>        if (ret)
>>>>            return ret;
>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>        int ret;
>>>>        u32 sleep_clk_offset = 0x8140;
>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>>        ret = devm_pm_runtime_enable(&pdev->dev);
>>>>        if (ret)
>>>>            return ret;
>>>>
>>>
>>> What's the difference between doing the attach here or doing it in
>>> really_probe() ?
>>
>> I'd second this. If the domains are to be attached before calling any
>> other functions, move the call to the qcom_cc_map(), so that all drivers
>> get all domains attached before configuring PLLs instead of manually
>> calling the function.
>>
>>> There doesn't seem to be any difference except that we will have an
>>> additional delay introduced.
>>>
>>> Are you describing a race condition ?
>>>
>>> I don't see _logic_ here to moving the call into the controller's higher
>>> level probe.
>>>
>>> Can you describe some more ?
>>>
>>> ---
>>> bod
>>
> 
> Here's one way this could work
> 
> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Date:   Tue Feb 18 19:46:55 2025 +0000
> 
>     clk: qcom: common: Add configure_plls callback prototype
> 
>     Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>     before configuring PLLs and ensure that the power-domain rail list is
>     switched on prior to configuring PLLs.
> 
>     Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 9e3380fd71819..1924130814600 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
>         if (ret < 0 && ret != -EEXIST)
>                 return ret;
> 
> +       if (desc->configure_plls)
> +               desc->configure_plls(regmap);
> +
>         reset = &cc->reset;
>         reset->rcdev.of_node = dev->of_node;
>         reset->rcdev.ops = &qcom_reset_ops;
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 7ace5d7f5836a..4955085ff8669 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
>         const struct qcom_icc_hws_data *icc_hws;
>         size_t num_icc_hws;
>         unsigned int icc_first_node_id;
> +       void (*configure_plls)(struct regmap *regmap);
>  };
> 
> and
> 
> % git diff drivers/clk/qcom/camcc-x1e80100.c
> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
> index b73524ae64b1b..c9748d1f8a15b 100644
> --- a/drivers/clk/qcom/camcc-x1e80100.c
> +++ b/drivers/clk/qcom/camcc-x1e80100.c
> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
>         .fast_io = true,
>  };
> 
> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
> +{
> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
> +
> +       /* Keep clocks always enabled */
> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> +}
> +
>  static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>         .config = &cam_cc_x1e80100_regmap_config,
>         .clks = cam_cc_x1e80100_clocks,
> @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>         .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>         .gdscs = cam_cc_x1e80100_gdscs,
>         .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>  };
> 
>  static const struct of_device_id cam_cc_x1e80100_match_table[] = {
> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
>                 return PTR_ERR(regmap);
>         }
> 
> -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
> -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
> -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
> -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
> -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
> -
> -       /* Keep clocks always enabled */
> -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
> -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> -
>         ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
> 
>         pm_runtime_put(&pdev->dev);
> 
> Or a least it works for me.
> 

This patch will not work in all cases, maybe in your case required power domains might be ON
from bootloaders so it might be working.

> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
> 

No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
with this than the current approach. 

Thanks,
Jagadeesh

Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Dmitry Baryshkov]

On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote:
> > On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
> >> Attach clock power domains in qcom_cc_really_probe() only
> >> if the clock controller has not already attached to them.
> > 
> > Squash this to the previous patch and call the new function. No need to
> > duplicate the code.
> > 
> 
> I tried calling the new function here instead of duplicating code, but that
> is leading to below warning since the desc passed to qcom_cc_really_probe()
> has a const qualifier and hence we cannot update desc->pd_list inside
> qcom_cc_really_probe().
> 
> drivers/clk/qcom/common.c:305:33:   WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

It sounds like this can be fixed with a one-line patch.

> 
> Thanks,
> Jagadeesh
> 
> >>
> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >> ---
> >>  drivers/clk/qcom/common.c | 9 ++++++---
> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> >> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
> >> --- a/drivers/clk/qcom/common.c
> >> +++ b/drivers/clk/qcom/common.c
> >> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
> >>  	if (!cc)
> >>  		return -ENOMEM;
> >>  
> >> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> >> -	if (ret < 0 && ret != -EEXIST)
> >> -		return ret;
> >> +	cc->pd_list = desc->pd_list;
> >> +	if (!cc->pd_list) {
> >> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> >> +		if (ret < 0 && ret != -EEXIST)
> >> +			return ret;
> >> +	}
> >>  
> >>  	reset = &cc->reset;
> >>  	reset->rcdev.of_node = dev->of_node;
> >>
> >> -- 
> >> 2.34.1
> >>
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Dmitry Baryshkov]

On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
> > On 18/02/2025 17:19, Dmitry Baryshkov wrote:
> >> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
> >>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
> >>>> During boot-up, the PLL configuration might be missed even after
> >>>> calling pll_configure() from the clock controller probe. This can
> >>>> happen because the PLL is connected to one or more rails that are
> >>>> turned off, and the current clock controller code cannot enable
> >>>> multiple rails during probe. Consequently, the PLL may be activated
> >>>> with suboptimal settings, causing functional issues.
> >>>>
> >>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
> >>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
> >>>> Therefore, add support to attach multiple power domains to videocc on
> >>>> these platforms.
> >>>>
> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>> ---
> >>>>    drivers/clk/qcom/videocc-sm8450.c | 4 ++++
> >>>>    drivers/clk/qcom/videocc-sm8550.c | 4 ++++
> >>>>    2 files changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
> >>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
> >>>> --- a/drivers/clk/qcom/videocc-sm8450.c
> >>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
> >>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
> >>>>        struct regmap *regmap;
> >>>>        int ret;
> >>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
> >>>> +    if (ret)
> >>>> +        return ret;
> >>>> +
> >>>>        ret = devm_pm_runtime_enable(&pdev->dev);
> >>>>        if (ret)
> >>>>            return ret;
> >>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
> >>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
> >>>> --- a/drivers/clk/qcom/videocc-sm8550.c
> >>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
> >>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
> >>>>        int ret;
> >>>>        u32 sleep_clk_offset = 0x8140;
> >>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
> >>>> +    if (ret)
> >>>> +        return ret;
> >>>> +
> >>>>        ret = devm_pm_runtime_enable(&pdev->dev);
> >>>>        if (ret)
> >>>>            return ret;
> >>>>
> >>>
> >>> What's the difference between doing the attach here or doing it in
> >>> really_probe() ?
> >>
> >> I'd second this. If the domains are to be attached before calling any
> >> other functions, move the call to the qcom_cc_map(), so that all drivers
> >> get all domains attached before configuring PLLs instead of manually
> >> calling the function.
> >>
> >>> There doesn't seem to be any difference except that we will have an
> >>> additional delay introduced.
> >>>
> >>> Are you describing a race condition ?
> >>>
> >>> I don't see _logic_ here to moving the call into the controller's higher
> >>> level probe.
> >>>
> >>> Can you describe some more ?
> >>>
> >>> ---
> >>> bod
> >>
> > 
> > Here's one way this could work
> > 
> > Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > Date:   Tue Feb 18 19:46:55 2025 +0000
> > 
> >     clk: qcom: common: Add configure_plls callback prototype
> > 
> >     Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
> >     before configuring PLLs and ensure that the power-domain rail list is
> >     switched on prior to configuring PLLs.
> > 
> >     Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > 
> > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > index 9e3380fd71819..1924130814600 100644
> > --- a/drivers/clk/qcom/common.c
> > +++ b/drivers/clk/qcom/common.c
> > @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
> >         if (ret < 0 && ret != -EEXIST)
> >                 return ret;
> > 
> > +       if (desc->configure_plls)
> > +               desc->configure_plls(regmap);
> > +
> >         reset = &cc->reset;
> >         reset->rcdev.of_node = dev->of_node;
> >         reset->rcdev.ops = &qcom_reset_ops;
> > diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> > index 7ace5d7f5836a..4955085ff8669 100644
> > --- a/drivers/clk/qcom/common.h
> > +++ b/drivers/clk/qcom/common.h
> > @@ -38,6 +38,7 @@ struct qcom_cc_desc {
> >         const struct qcom_icc_hws_data *icc_hws;
> >         size_t num_icc_hws;
> >         unsigned int icc_first_node_id;
> > +       void (*configure_plls)(struct regmap *regmap);
> >  };
> > 
> > and
> > 
> > % git diff drivers/clk/qcom/camcc-x1e80100.c
> > diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
> > index b73524ae64b1b..c9748d1f8a15b 100644
> > --- a/drivers/clk/qcom/camcc-x1e80100.c
> > +++ b/drivers/clk/qcom/camcc-x1e80100.c
> > @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
> >         .fast_io = true,
> >  };
> > 
> > +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
> > +{
> > +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> > +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> > +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
> > +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
> > +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
> > +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
> > +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
> > +
> > +       /* Keep clocks always enabled */
> > +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
> > +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> > +}
> > +
> >  static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
> >         .config = &cam_cc_x1e80100_regmap_config,
> >         .clks = cam_cc_x1e80100_clocks,
> > @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
> >         .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
> >         .gdscs = cam_cc_x1e80100_gdscs,
> >         .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
> > +       .configure_plls = cam_cc_x1e80100_configure_plls,
> >  };
> > 
> >  static const struct of_device_id cam_cc_x1e80100_match_table[] = {
> > @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
> >                 return PTR_ERR(regmap);
> >         }
> > 
> > -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> > -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> > -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
> > -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
> > -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
> > -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
> > -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
> > -
> > -       /* Keep clocks always enabled */
> > -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
> > -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> > -
> >         ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
> > 
> >         pm_runtime_put(&pdev->dev);
> > 
> > Or a least it works for me.
> > 
> 
> This patch will not work in all cases, maybe in your case required power domains might be ON
> from bootloaders so it might be working.

But with his patch domains are attached before configuring the PLLs, are
they not?

> 
> > New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
> > 
> 
> No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
> with this than the current approach. 

Bryan's proposal moves us towards having a common code, so it's better.

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Dmitry Baryshkov]

On Wed, Feb 19, 2025 at 05:08:52PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 2/18/2025 10:49 PM, Dmitry Baryshkov wrote:
> > On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
> >> On 18/02/2025 14:26, Jagadeesh Kona wrote:
> >>> During boot-up, the PLL configuration might be missed even after
> >>> calling pll_configure() from the clock controller probe. This can
> >>> happen because the PLL is connected to one or more rails that are
> >>> turned off, and the current clock controller code cannot enable
> >>> multiple rails during probe. Consequently, the PLL may be activated
> >>> with suboptimal settings, causing functional issues.
> >>>
> >>> To properly configure the video PLLs in the probe on SM8450, SM8475,
> >>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
> >>> Therefore, add support to attach multiple power domains to videocc on
> >>> these platforms.
> >>>
> >>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>> ---
> >>>   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
> >>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
> >>>   2 files changed, 8 insertions(+)
> >>>
> >>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
> >>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
> >>> --- a/drivers/clk/qcom/videocc-sm8450.c
> >>> +++ b/drivers/clk/qcom/videocc-sm8450.c
> >>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
> >>>   	struct regmap *regmap;
> >>>   	int ret;
> >>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>>   	ret = devm_pm_runtime_enable(&pdev->dev);
> >>>   	if (ret)
> >>>   		return ret;
> >>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
> >>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
> >>> --- a/drivers/clk/qcom/videocc-sm8550.c
> >>> +++ b/drivers/clk/qcom/videocc-sm8550.c
> >>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
> >>>   	int ret;
> >>>   	u32 sleep_clk_offset = 0x8140;
> >>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>>   	ret = devm_pm_runtime_enable(&pdev->dev);
> >>>   	if (ret)
> >>>   		return ret;
> >>>
> >>
> >> What's the difference between doing the attach here or doing it in
> >> really_probe() ?
> > 
> > I'd second this. If the domains are to be attached before calling any
> > other functions, move the call to the qcom_cc_map(), so that all drivers
> > get all domains attached before configuring PLLs instead of manually
> > calling the function.
> > 
> 
> I earlier tried moving the attach PDs call to qcom_cc_map(), but I faced the below issues
> 1. desc passed to qcom_cc_map() has const qualifier, so updating desc->pd_list
>    inside qcom_cc_map() is leading to a warning.

And? Can you fix the warning?

> 2. If we attach the PDs after calling get_sync() on device, I observed
>    that PDS are not getting enabled during probe. Currently qcom_cc_map()
>    is called after get_sync() is already called on device.

Move PM handling into qcom_cc_map(). Then together with the Bryan's
proposal most of the probe() functions can just call qcom_cc_probe()

> 
> Probably, we can add a new function qcom_cc_attach_pds_map() where we can
> attach PDs and call qcom_cc_map() inside it. We can then invoke this new
> function at the start of probe before get_sync(). I will post this change
> in next version if this aligns with your thoughts.
> 
> Thanks,
> Jagadeesh

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 19/02/2025 11:59, Dmitry Baryshkov wrote:
> On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
>>> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>>> During boot-up, the PLL configuration might be missed even after
>>>>>> calling pll_configure() from the clock controller probe. This can
>>>>>> happen because the PLL is connected to one or more rails that are
>>>>>> turned off, and the current clock controller code cannot enable
>>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>>> with suboptimal settings, causing functional issues.
>>>>>>
>>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>>> these platforms.
>>>>>>
>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>> ---
>>>>>>     drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>>     drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>>     2 files changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>>         struct regmap *regmap;
>>>>>>         int ret;
>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>>         ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>         if (ret)
>>>>>>             return ret;
>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>>         int ret;
>>>>>>         u32 sleep_clk_offset = 0x8140;
>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>>         ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>         if (ret)
>>>>>>             return ret;
>>>>>>
>>>>>
>>>>> What's the difference between doing the attach here or doing it in
>>>>> really_probe() ?
>>>>
>>>> I'd second this. If the domains are to be attached before calling any
>>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>>> get all domains attached before configuring PLLs instead of manually
>>>> calling the function.
>>>>
>>>>> There doesn't seem to be any difference except that we will have an
>>>>> additional delay introduced.
>>>>>
>>>>> Are you describing a race condition ?
>>>>>
>>>>> I don't see _logic_ here to moving the call into the controller's higher
>>>>> level probe.
>>>>>
>>>>> Can you describe some more ?
>>>>>
>>>>> ---
>>>>> bod
>>>>
>>>
>>> Here's one way this could work
>>>
>>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>>
>>>      clk: qcom: common: Add configure_plls callback prototype
>>>
>>>      Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>>      before configuring PLLs and ensure that the power-domain rail list is
>>>      switched on prior to configuring PLLs.
>>>
>>>      Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>
>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>> index 9e3380fd71819..1924130814600 100644
>>> --- a/drivers/clk/qcom/common.c
>>> +++ b/drivers/clk/qcom/common.c
>>> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
>>>          if (ret < 0 && ret != -EEXIST)
>>>                  return ret;
>>>
>>> +       if (desc->configure_plls)
>>> +               desc->configure_plls(regmap);
>>> +
>>>          reset = &cc->reset;
>>>          reset->rcdev.of_node = dev->of_node;
>>>          reset->rcdev.ops = &qcom_reset_ops;
>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>> index 7ace5d7f5836a..4955085ff8669 100644
>>> --- a/drivers/clk/qcom/common.h
>>> +++ b/drivers/clk/qcom/common.h
>>> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
>>>          const struct qcom_icc_hws_data *icc_hws;
>>>          size_t num_icc_hws;
>>>          unsigned int icc_first_node_id;
>>> +       void (*configure_plls)(struct regmap *regmap);
>>>   };
>>>
>>> and
>>>
>>> % git diff drivers/clk/qcom/camcc-x1e80100.c
>>> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
>>> index b73524ae64b1b..c9748d1f8a15b 100644
>>> --- a/drivers/clk/qcom/camcc-x1e80100.c
>>> +++ b/drivers/clk/qcom/camcc-x1e80100.c
>>> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
>>>          .fast_io = true,
>>>   };
>>>
>>> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
>>> +{
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>> +
>>> +       /* Keep clocks always enabled */
>>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>> +}
>>> +
>>>   static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>          .config = &cam_cc_x1e80100_regmap_config,
>>>          .clks = cam_cc_x1e80100_clocks,
>>> @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>          .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>>          .gdscs = cam_cc_x1e80100_gdscs,
>>>          .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>>   };
>>>
>>>   static const struct of_device_id cam_cc_x1e80100_match_table[] = {
>>> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
>>>                  return PTR_ERR(regmap);
>>>          }
>>>
>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>> -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>> -
>>> -       /* Keep clocks always enabled */
>>> -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>> -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>> -
>>>          ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
>>>
>>>          pm_runtime_put(&pdev->dev);
>>>
>>> Or a least it works for me.
>>>
>>
>> This patch will not work in all cases, maybe in your case required power domains might be ON
>> from bootloaders so it might be working.
> 
> But with his patch domains are attached before configuring the PLLs, are
> they not?

Yes, its logically the same just done in core code.

>>
>>> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
>>>
>>
>> No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
>> with this than the current approach.
> 
> Bryan's proposal moves us towards having a common code, so it's better.
> 

I can take the time to do the whole sweep and publish a RFC.

---
bod

Re: [PATCH 5/5] arm64: dts: qcom: Add MXC power domain to videocc nodes

[Jagadeesh Kona]

On 2/18/2025 11:02 PM, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 07:56:50PM +0530, Jagadeesh Kona wrote:
>> Videocc requires both MMCX and MXC rails to be powered ON
>> to configure the video PLLs on SM8450, SM8550 and SM8650
>> platforms. Hence add MXC power domain to videocc node on
>> these platforms.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ++-
>>  arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
>>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++-
>>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> Three separate patches, please. With that in mind:
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 

Sure, will split this into separate patches.

Thanks,
Jagadeesh

>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 9c809fc5fa45a98ff5441a0b6809931588897243..4f8dca8fc64212191780067c5d8815e3a2bb137f 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -3136,7 +3136,8 @@ videocc: clock-controller@aaf0000 {
>>  			reg = <0 0x0aaf0000 0 0x10000>;
>>  			clocks = <&rpmhcc RPMH_CXO_CLK>,
>>  				 <&gcc GCC_VIDEO_AHB_CLK>;
>> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
>> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
>> +					<&rpmhpd RPMHPD_MXC>;
>>  			required-opps = <&rpmhpd_opp_low_svs>;
>>  			#clock-cells = <1>;
>>  			#reset-cells = <1>;
>> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> index eac8de4005d82f246bc50f64f09515631d895c99..a039ae71e1b7bba8124128d19de5e00c65217770 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> @@ -2889,7 +2889,8 @@ videocc: clock-controller@aaf0000 {
>>  			reg = <0 0x0aaf0000 0 0x10000>;
>>  			clocks = <&bi_tcxo_div2>,
>>  				 <&gcc GCC_VIDEO_AHB_CLK>;
>> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
>> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
>> +					<&rpmhpd RPMHPD_MXC>;
>>  			required-opps = <&rpmhpd_opp_low_svs>;
>>  			#clock-cells = <1>;
>>  			#reset-cells = <1>;
>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> index 86684cb9a9325618ddb74458621cf4bbdc1cc0d1..32af2a0f7a0030f155b7d8c93faeffa384a42768 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> @@ -3524,7 +3524,8 @@ videocc: clock-controller@aaf0000 {
>>  			reg = <0 0x0aaf0000 0 0x10000>;
>>  			clocks = <&bi_tcxo_div2>,
>>  				 <&gcc GCC_VIDEO_AHB_CLK>;
>> -			power-domains = <&rpmhpd RPMHPD_MMCX>;
>> +			power-domains = <&rpmhpd RPMHPD_MMCX>,
>> +					<&rpmhpd RPMHPD_MXC>;
>>  			#clock-cells = <1>;
>>  			#reset-cells = <1>;
>>  			#power-domain-cells = <1>;
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

On 2/19/2025 5:32 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 19, 2025 at 05:08:52PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 2/18/2025 10:49 PM, Dmitry Baryshkov wrote:
>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>> During boot-up, the PLL configuration might be missed even after
>>>>> calling pll_configure() from the clock controller probe. This can
>>>>> happen because the PLL is connected to one or more rails that are
>>>>> turned off, and the current clock controller code cannot enable
>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>> with suboptimal settings, causing functional issues.
>>>>>
>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>> these platforms.
>>>>>
>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>> ---
>>>>>   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>   2 files changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>   	struct regmap *regmap;
>>>>>   	int ret;
>>>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>>   	ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>   	if (ret)
>>>>>   		return ret;
>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>   	int ret;
>>>>>   	u32 sleep_clk_offset = 0x8140;
>>>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>>   	ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>   	if (ret)
>>>>>   		return ret;
>>>>>
>>>>
>>>> What's the difference between doing the attach here or doing it in
>>>> really_probe() ?
>>>
>>> I'd second this. If the domains are to be attached before calling any
>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>> get all domains attached before configuring PLLs instead of manually
>>> calling the function.
>>>
>>
>> I earlier tried moving the attach PDs call to qcom_cc_map(), but I faced the below issues
>> 1. desc passed to qcom_cc_map() has const qualifier, so updating desc->pd_list
>>    inside qcom_cc_map() is leading to a warning.
> 
> And? Can you fix the warning?
> 

I can remove the const qualifier in qcom_cc_map() prototype to fix this, but that requires changes
in many other clock drivers also since they are currently passing const descriptor to qcom_cc_map().
So would like to keep the qcom_cc_map() unchanged.

>> 2. If we attach the PDs after calling get_sync() on device, I observed
>>    that PDS are not getting enabled during probe. Currently qcom_cc_map()
>>    is called after get_sync() is already called on device.
> 
> Move PM handling into qcom_cc_map(). Then together with the Bryan's
> proposal most of the probe() functions can just call qcom_cc_probe()
> 

I agree with this approach to move entire PM handling to qcom_cc_map() but one concern is const
qualifier mentioned above and it also enables runtime PM for clock controllers that doesn't need
any runtime PM(e.g:- GCC/GPUCC). That may not cause any issue but we also need to see from where
we need to call pm_runtime_put().

We may have to add pm_runtime_put() at the end of both qcom_cc_probe() and qcom_cc_really_probe()
to move the device back to suspend after probe. But ideally runtime PM is not required for most
clock controllers except MMCC's that have MMCX dependency. Please let me know your thoughts on this.

Thanks,
Jagadeesh

>>
>> Probably, we can add a new function qcom_cc_attach_pds_map() where we can
>> attach PDs and call qcom_cc_map() inside it. We can then invoke this new
>> function at the start of probe before get_sync(). I will post this change
>> in next version if this aligns with your thoughts.
>>
>> Thanks,
>> Jagadeesh
> 

Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Jagadeesh Kona]

On 2/19/2025 5:27 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote:
>>> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
>>>> Attach clock power domains in qcom_cc_really_probe() only
>>>> if the clock controller has not already attached to them.
>>>
>>> Squash this to the previous patch and call the new function. No need to
>>> duplicate the code.
>>>
>>
>> I tried calling the new function here instead of duplicating code, but that
>> is leading to below warning since the desc passed to qcom_cc_really_probe()
>> has a const qualifier and hence we cannot update desc->pd_list inside
>> qcom_cc_really_probe().
>>
>> drivers/clk/qcom/common.c:305:33:   WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 
> It sounds like this can be fixed with a one-line patch.
> 

Removing const qualifier to qcom_cc_really_probe() will fix this, but that requires changes in
many other drivers which are currently passing const descriptor to it.

But I can squash this to my previous patch by updating my qcom_cc_attach_pds() function
prototype as below and then calling that new function here

-int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc)
+int qcom_cc_attach_pds(struct device *dev, struct dev_pm_domain_list *pd_list)

-               ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
-               if (ret < 0 && ret != -EEXIST)
+               ret = qcom_cc_attach_pds(dev, cc->pd_list);
+               if (ret)

Thanks,
Jagadeesh

>>
>> Thanks,
>> Jagadeesh
>>
>>>>
>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>> ---
>>>>  drivers/clk/qcom/common.c | 9 ++++++---
>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
>>>> --- a/drivers/clk/qcom/common.c
>>>> +++ b/drivers/clk/qcom/common.c
>>>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
>>>>  	if (!cc)
>>>>  		return -ENOMEM;
>>>>  
>>>> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>>>> -	if (ret < 0 && ret != -EEXIST)
>>>> -		return ret;
>>>> +	cc->pd_list = desc->pd_list;
>>>> +	if (!cc->pd_list) {
>>>> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>>>> +		if (ret < 0 && ret != -EEXIST)
>>>> +			return ret;
>>>> +	}
>>>>  
>>>>  	reset = &cc->reset;
>>>>  	reset->rcdev.of_node = dev->of_node;
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>
> 

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

On 2/19/2025 5:37 PM, Bryan O'Donoghue wrote:
> On 19/02/2025 11:59, Dmitry Baryshkov wrote:
>> On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote:
>>>
>>>
>>> On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
>>>> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>>>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>>>> During boot-up, the PLL configuration might be missed even after
>>>>>>> calling pll_configure() from the clock controller probe. This can
>>>>>>> happen because the PLL is connected to one or more rails that are
>>>>>>> turned off, and the current clock controller code cannot enable
>>>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>>>> with suboptimal settings, causing functional issues.
>>>>>>>
>>>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>>>> these platforms.
>>>>>>>
>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>>> ---
>>>>>>>     drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>>>     drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>>>     2 files changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>>>         struct regmap *regmap;
>>>>>>>         int ret;
>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>>> +
>>>>>>>         ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>         if (ret)
>>>>>>>             return ret;
>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>>>         int ret;
>>>>>>>         u32 sleep_clk_offset = 0x8140;
>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>>> +
>>>>>>>         ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>         if (ret)
>>>>>>>             return ret;
>>>>>>>
>>>>>>
>>>>>> What's the difference between doing the attach here or doing it in
>>>>>> really_probe() ?
>>>>>
>>>>> I'd second this. If the domains are to be attached before calling any
>>>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>>>> get all domains attached before configuring PLLs instead of manually
>>>>> calling the function.
>>>>>
>>>>>> There doesn't seem to be any difference except that we will have an
>>>>>> additional delay introduced.
>>>>>>
>>>>>> Are you describing a race condition ?
>>>>>>
>>>>>> I don't see _logic_ here to moving the call into the controller's higher
>>>>>> level probe.
>>>>>>
>>>>>> Can you describe some more ?
>>>>>>
>>>>>> ---
>>>>>> bod
>>>>>
>>>>
>>>> Here's one way this could work
>>>>
>>>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>>>
>>>>      clk: qcom: common: Add configure_plls callback prototype
>>>>
>>>>      Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>>>      before configuring PLLs and ensure that the power-domain rail list is
>>>>      switched on prior to configuring PLLs.
>>>>
>>>>      Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>
>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>> index 9e3380fd71819..1924130814600 100644
>>>> --- a/drivers/clk/qcom/common.c
>>>> +++ b/drivers/clk/qcom/common.c
>>>> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
>>>>          if (ret < 0 && ret != -EEXIST)
>>>>                  return ret;
>>>>
>>>> +       if (desc->configure_plls)
>>>> +               desc->configure_plls(regmap);
>>>> +
>>>>          reset = &cc->reset;
>>>>          reset->rcdev.of_node = dev->of_node;
>>>>          reset->rcdev.ops = &qcom_reset_ops;
>>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>>> index 7ace5d7f5836a..4955085ff8669 100644
>>>> --- a/drivers/clk/qcom/common.h
>>>> +++ b/drivers/clk/qcom/common.h
>>>> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
>>>>          const struct qcom_icc_hws_data *icc_hws;
>>>>          size_t num_icc_hws;
>>>>          unsigned int icc_first_node_id;
>>>> +       void (*configure_plls)(struct regmap *regmap);
>>>>   };
>>>>
>>>> and
>>>>
>>>> % git diff drivers/clk/qcom/camcc-x1e80100.c
>>>> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
>>>> index b73524ae64b1b..c9748d1f8a15b 100644
>>>> --- a/drivers/clk/qcom/camcc-x1e80100.c
>>>> +++ b/drivers/clk/qcom/camcc-x1e80100.c
>>>> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
>>>>          .fast_io = true,
>>>>   };
>>>>
>>>> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
>>>> +{
>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>> +
>>>> +       /* Keep clocks always enabled */
>>>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>> +}
>>>> +
>>>>   static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>          .config = &cam_cc_x1e80100_regmap_config,
>>>>          .clks = cam_cc_x1e80100_clocks,
>>>> @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>          .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>>>          .gdscs = cam_cc_x1e80100_gdscs,
>>>>          .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>>>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>>>   };
>>>>
>>>>   static const struct of_device_id cam_cc_x1e80100_match_table[] = {
>>>> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
>>>>                  return PTR_ERR(regmap);
>>>>          }
>>>>
>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>> -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>> -
>>>> -       /* Keep clocks always enabled */
>>>> -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>> -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>> -
>>>>          ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
>>>>
>>>>          pm_runtime_put(&pdev->dev);
>>>>
>>>> Or a least it works for me.
>>>>
>>>
>>> This patch will not work in all cases, maybe in your case required power domains might be ON
>>> from bootloaders so it might be working.
>>
>> But with his patch domains are attached before configuring the PLLs, are
>> they not?
> 
> Yes, its logically the same just done in core code.
> 

Yes, this code attaches domains before configuring the PLLs, but it attaches PDs after get_sync()
is called on device. As I mentioned in other patch earlier, if we attach PDS after get_sync() is
already called on device, then power domains are not getting enabled during the probe, leading to
the same improper PLL configuration issue. But the current patch series posted will fix this issue

>>>
>>>> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
>>>>
>>>
>>> No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
>>> with this than the current approach.
>>
>> Bryan's proposal moves us towards having a common code, so it's better.
>>
> 
> I can take the time to do the whole sweep and publish a RFC.
> 

Yes, but moving the PLL configuration to callback will not solve the actual PLL configuration
issue being discussed here. 

Thanks,
Jagadeesh

> ---
> bod

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 20/02/2025 07:15, Jagadeesh Kona wrote:
>> Yes, its logically the same just done in core code.
>>
> Yes, this code attaches domains before configuring the PLLs, but it attaches PDs after get_sync()
> is called on device. As I mentioned in other patch earlier, if we attach PDS after get_sync() is
> already called on device, then power domains are not getting enabled during the probe, leading to
> the same improper PLL configuration issue. But the current patch series posted will fix this issue

That's not what I see.

The PLLs start and the GDSCs which depend also start.

Perhaps you could give the code a try and comment ?

---
bod

Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Dmitry Baryshkov]

On Thu, Feb 20, 2025 at 12:43:42PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 2/19/2025 5:27 PM, Dmitry Baryshkov wrote:
> > On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote:
> >>
> >>
> >> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote:
> >>> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
> >>>> Attach clock power domains in qcom_cc_really_probe() only
> >>>> if the clock controller has not already attached to them.
> >>>
> >>> Squash this to the previous patch and call the new function. No need to
> >>> duplicate the code.
> >>>
> >>
> >> I tried calling the new function here instead of duplicating code, but that
> >> is leading to below warning since the desc passed to qcom_cc_really_probe()
> >> has a const qualifier and hence we cannot update desc->pd_list inside
> >> qcom_cc_really_probe().
> >>
> >> drivers/clk/qcom/common.c:305:33:   WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> > 
> > It sounds like this can be fixed with a one-line patch.
> > 
> 
> Removing const qualifier to qcom_cc_really_probe() will fix this, but that requires changes in
> many other drivers which are currently passing const descriptor to it.

And this points out that the pd_list should not be a part of the
struct qcom_cc_desc. You are not using it in the code, so allocate that
memory on the fly, pass it to devm_pm_domain_attach_list() and then
forget about it.

> 
> But I can squash this to my previous patch by updating my qcom_cc_attach_pds() function
> prototype as below and then calling that new function here
> 
> -int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc)
> +int qcom_cc_attach_pds(struct device *dev, struct dev_pm_domain_list *pd_list)
> 
> -               ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> -               if (ret < 0 && ret != -EEXIST)
> +               ret = qcom_cc_attach_pds(dev, cc->pd_list);
> +               if (ret)
> 
> Thanks,
> Jagadeesh
> 
> >>
> >> Thanks,
> >> Jagadeesh
> >>
> >>>>
> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>> ---
> >>>>  drivers/clk/qcom/common.c | 9 ++++++---
> >>>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> >>>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
> >>>> --- a/drivers/clk/qcom/common.c
> >>>> +++ b/drivers/clk/qcom/common.c
> >>>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
> >>>>  	if (!cc)
> >>>>  		return -ENOMEM;
> >>>>  
> >>>> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> >>>> -	if (ret < 0 && ret != -EEXIST)
> >>>> -		return ret;
> >>>> +	cc->pd_list = desc->pd_list;
> >>>> +	if (!cc->pd_list) {
> >>>> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> >>>> +		if (ret < 0 && ret != -EEXIST)
> >>>> +			return ret;
> >>>> +	}
> >>>>  
> >>>>  	reset = &cc->reset;
> >>>>  	reset->rcdev.of_node = dev->of_node;
> >>>>
> >>>> -- 
> >>>> 2.34.1
> >>>>
> >>>
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 20/02/2025 07:15, Jagadeesh Kona wrote:
> 
> 
> On 2/19/2025 5:37 PM, Bryan O'Donoghue wrote:
>> On 19/02/2025 11:59, Dmitry Baryshkov wrote:
>>> On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote:
>>>>
>>>>
>>>> On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
>>>>> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>>>>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>>>>> During boot-up, the PLL configuration might be missed even after
>>>>>>>> calling pll_configure() from the clock controller probe. This can
>>>>>>>> happen because the PLL is connected to one or more rails that are
>>>>>>>> turned off, and the current clock controller code cannot enable
>>>>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>>>>> with suboptimal settings, causing functional issues.
>>>>>>>>
>>>>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>>>>> these platforms.
>>>>>>>>
>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>>>> ---
>>>>>>>>      drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>>>>      drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>>>>      2 files changed, 8 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>>>>          struct regmap *regmap;
>>>>>>>>          int ret;
>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>>>>> +    if (ret)
>>>>>>>> +        return ret;
>>>>>>>> +
>>>>>>>>          ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>          if (ret)
>>>>>>>>              return ret;
>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>>>>          int ret;
>>>>>>>>          u32 sleep_clk_offset = 0x8140;
>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>>>>> +    if (ret)
>>>>>>>> +        return ret;
>>>>>>>> +
>>>>>>>>          ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>          if (ret)
>>>>>>>>              return ret;
>>>>>>>>
>>>>>>>
>>>>>>> What's the difference between doing the attach here or doing it in
>>>>>>> really_probe() ?
>>>>>>
>>>>>> I'd second this. If the domains are to be attached before calling any
>>>>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>>>>> get all domains attached before configuring PLLs instead of manually
>>>>>> calling the function.
>>>>>>
>>>>>>> There doesn't seem to be any difference except that we will have an
>>>>>>> additional delay introduced.
>>>>>>>
>>>>>>> Are you describing a race condition ?
>>>>>>>
>>>>>>> I don't see _logic_ here to moving the call into the controller's higher
>>>>>>> level probe.
>>>>>>>
>>>>>>> Can you describe some more ?
>>>>>>>
>>>>>>> ---
>>>>>>> bod
>>>>>>
>>>>>
>>>>> Here's one way this could work
>>>>>
>>>>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>>>>
>>>>>       clk: qcom: common: Add configure_plls callback prototype
>>>>>
>>>>>       Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>>>>       before configuring PLLs and ensure that the power-domain rail list is
>>>>>       switched on prior to configuring PLLs.
>>>>>
>>>>>       Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>
>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>>> index 9e3380fd71819..1924130814600 100644
>>>>> --- a/drivers/clk/qcom/common.c
>>>>> +++ b/drivers/clk/qcom/common.c
>>>>> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
>>>>>           if (ret < 0 && ret != -EEXIST)
>>>>>                   return ret;
>>>>>
>>>>> +       if (desc->configure_plls)
>>>>> +               desc->configure_plls(regmap);
>>>>> +
>>>>>           reset = &cc->reset;
>>>>>           reset->rcdev.of_node = dev->of_node;
>>>>>           reset->rcdev.ops = &qcom_reset_ops;
>>>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>>>> index 7ace5d7f5836a..4955085ff8669 100644
>>>>> --- a/drivers/clk/qcom/common.h
>>>>> +++ b/drivers/clk/qcom/common.h
>>>>> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
>>>>>           const struct qcom_icc_hws_data *icc_hws;
>>>>>           size_t num_icc_hws;
>>>>>           unsigned int icc_first_node_id;
>>>>> +       void (*configure_plls)(struct regmap *regmap);
>>>>>    };
>>>>>
>>>>> and
>>>>>
>>>>> % git diff drivers/clk/qcom/camcc-x1e80100.c
>>>>> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
>>>>> index b73524ae64b1b..c9748d1f8a15b 100644
>>>>> --- a/drivers/clk/qcom/camcc-x1e80100.c
>>>>> +++ b/drivers/clk/qcom/camcc-x1e80100.c
>>>>> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
>>>>>           .fast_io = true,
>>>>>    };
>>>>>
>>>>> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
>>>>> +{
>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>> +
>>>>> +       /* Keep clocks always enabled */
>>>>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>> +}
>>>>> +
>>>>>    static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>           .config = &cam_cc_x1e80100_regmap_config,
>>>>>           .clks = cam_cc_x1e80100_clocks,
>>>>> @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>           .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>>>>           .gdscs = cam_cc_x1e80100_gdscs,
>>>>>           .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>>>>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>>>>    };
>>>>>
>>>>>    static const struct of_device_id cam_cc_x1e80100_match_table[] = {
>>>>> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
>>>>>                   return PTR_ERR(regmap);
>>>>>           }
>>>>>
>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>> -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>> -
>>>>> -       /* Keep clocks always enabled */
>>>>> -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>> -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>> -
>>>>>           ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
>>>>>
>>>>>           pm_runtime_put(&pdev->dev);
>>>>>
>>>>> Or a least it works for me.
>>>>>
>>>>
>>>> This patch will not work in all cases, maybe in your case required power domains might be ON
>>>> from bootloaders so it might be working.
>>>
>>> But with his patch domains are attached before configuring the PLLs, are
>>> they not?
>>
>> Yes, its logically the same just done in core code.
>>
> 
> Yes, this code attaches domains before configuring the PLLs, but it attaches PDs after get_sync()
> is called on device. As I mentioned in other patch earlier, if we attach PDS after get_sync() is
> already called on device, then power domains are not getting enabled during the probe, leading to
> the same improper PLL configuration issue. But the current patch series posted will fix this issue
> 
>>>>
>>>>> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
>>>>>
>>>>
>>>> No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
>>>> with this than the current approach.
>>>
>>> Bryan's proposal moves us towards having a common code, so it's better.
>>>
>>
>> I can take the time to do the whole sweep and publish a RFC.
>>
> 
> Yes, but moving the PLL configuration to callback will not solve the actual PLL configuration
> issue being discussed here.
> 
> Thanks,
> Jagadeesh
> 

Right what you are really saying is that the power-rails for the clock 
controller need to remain always on at the moment.

Where we can zap the GDSCs the power-rails for the block should be 
always on because the initial PLL configuration we typically do in 
probe() would be negated as soon as the power rail for the block is 
switched off.

True.

In my opinion:

- We should only do the pd list addition in one place
   Either that or push it into each driver.

   I don't favour doing it in each driver since it is boilerplate
   code that we basically just end up copy/pasting again and again.

- We can start off by only including a configure_pll callback
   for the 2-3 blocks where we know we have multiple rails

This here works well for me on x1e:

Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Date:   Tue Feb 18 19:46:55 2025 +0000

     clk: qcom: common: Add configure_plls callback prototype

     Add a configure_plls() callback so that we can stage 
qcom_cc_attach_pds()
     before configuring PLLs and ensure that the power-domain rail list is
     switched on prior to configuring PLLs.

     Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 9e3380fd71819..4aa00ad51c2f6 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -304,6 +304,12 @@ int qcom_cc_really_probe(struct device *dev,
         if (ret < 0 && ret != -EEXIST)
                 return ret;

+       if (desc->configure_plls) {
+               ret = desc->configure_plls(dev, desc, regmap);
+               if (ret)
+                       return ret;
+       }
+
         reset = &cc->reset;
         reset->rcdev.of_node = dev->of_node;
         reset->rcdev.ops = &qcom_reset_ops;
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 7ace5d7f5836a..77002e39337d7 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -38,6 +38,9 @@ struct qcom_cc_desc {
         const struct qcom_icc_hws_data *icc_hws;
         size_t num_icc_hws;
         unsigned int icc_first_node_id;
+       int (*configure_plls)(struct device *dev,
+                             const struct qcom_cc_desc *desc,
+                             struct regmap *regmap);
  };

+static int cam_cc_x1e80100_configure_plls(struct device *dev,
+                                         const struct qcom_cc_desc *desc,
+                                         struct regmap *regmap)
+{
+       int ret;
+
+       ret = devm_pm_runtime_enable(dev);
+       if (ret)
+               return ret;
+
+       ret = pm_runtime_resume_and_get(dev);
+       if (ret)
+               return ret;
+
+       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, 
&cam_cc_pll0_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, 
&cam_cc_pll1_config);
+       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, 
&cam_cc_pll2_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, 
&cam_cc_pll3_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, 
&cam_cc_pll4_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, 
&cam_cc_pll6_config);
+       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, 
&cam_cc_pll8_config);
+
+       /* Keep clocks always enabled */
+       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
+       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
+
+       pm_runtime_put(dev);
+
+       return 0;
+}
+
  static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
         .config = &cam_cc_x1e80100_regmap_config,
         .clks = cam_cc_x1e80100_clocks,
@@ -2434,6 +2465,7 @@ static const struct qcom_cc_desc 
cam_cc_x1e80100_desc = {
         .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
         .gdscs = cam_cc_x1e80100_gdscs,
         .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
+       .configure_plls = cam_cc_x1e80100_configure_plls,
  };

This has the same effect as you were alluding to and in fact we could 
probably even move the pm_runtime_enable/resume_and_get and 
pm_runtime_put into really_probe().

It seems to me anyway we should try to push as much of this into core 
logic to be reused as possible.

---
bod

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 20/02/2025 22:31, Bryan O'Donoghue wrote:
> It seems to me anyway we should try to push as much of this into core 
> logic to be reused as possible.

But there's no valid use-case for doing

ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);

in the driver and then conditionally doing it again in really_probe().

Its an either/or.

---
bod

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Dmitry Baryshkov]

On Thu, Feb 20, 2025 at 10:31:44PM +0000, Bryan O'Donoghue wrote:
> 
> Where we can zap the GDSCs the power-rails for the block should be always on
> because the initial PLL configuration we typically do in probe() would be
> negated as soon as the power rail for the block is switched off.
> 
> True.
> 
> In my opinion:
> 
> - We should only do the pd list addition in one place
>   Either that or push it into each driver.
> 
>   I don't favour doing it in each driver since it is boilerplate
>   code that we basically just end up copy/pasting again and again.
> 
> - We can start off by only including a configure_pll callback
>   for the 2-3 blocks where we know we have multiple rails
> 
> This here works well for me on x1e:
> 
> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Date:   Tue Feb 18 19:46:55 2025 +0000
> 
>     clk: qcom: common: Add configure_plls callback prototype
> 
>     Add a configure_plls() callback so that we can stage
> qcom_cc_attach_pds()
>     before configuring PLLs and ensure that the power-domain rail list is
>     switched on prior to configuring PLLs.
> 
>     Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 9e3380fd71819..4aa00ad51c2f6 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -304,6 +304,12 @@ int qcom_cc_really_probe(struct device *dev,
>         if (ret < 0 && ret != -EEXIST)
>                 return ret;
> 
> +       if (desc->configure_plls) {
> +               ret = desc->configure_plls(dev, desc, regmap);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         reset = &cc->reset;
>         reset->rcdev.of_node = dev->of_node;
>         reset->rcdev.ops = &qcom_reset_ops;
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 7ace5d7f5836a..77002e39337d7 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -38,6 +38,9 @@ struct qcom_cc_desc {
>         const struct qcom_icc_hws_data *icc_hws;
>         size_t num_icc_hws;
>         unsigned int icc_first_node_id;
> +       int (*configure_plls)(struct device *dev,
> +                             const struct qcom_cc_desc *desc,
> +                             struct regmap *regmap);
>  };
> 
> +static int cam_cc_x1e80100_configure_plls(struct device *dev,
> +                                         const struct qcom_cc_desc *desc,
> +                                         struct regmap *regmap)
> +{
> +       int ret;
> +
> +       ret = devm_pm_runtime_enable(dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = pm_runtime_resume_and_get(dev);
> +       if (ret)
> +               return ret;

I think, it's better to add desc->use_rpm. Then these two calls and
pm_runtime_put() can go to a generic code.

Or maybe we can enable RPM for all clock controllers?

> +
> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap,
> &cam_cc_pll0_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap,
> &cam_cc_pll1_config);
> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap,
> &cam_cc_pll2_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap,
> &cam_cc_pll3_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap,
> &cam_cc_pll4_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap,
> &cam_cc_pll6_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap,
> &cam_cc_pll8_config);
> +
> +       /* Keep clocks always enabled */
> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> +
> +       pm_runtime_put(dev);
> +
> +       return 0;
> +}
> +
>  static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>         .config = &cam_cc_x1e80100_regmap_config,
>         .clks = cam_cc_x1e80100_clocks,
> @@ -2434,6 +2465,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc
> = {
>         .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>         .gdscs = cam_cc_x1e80100_gdscs,
>         .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>  };
> 
> This has the same effect as you were alluding to and in fact we could
> probably even move the pm_runtime_enable/resume_and_get and pm_runtime_put
> into really_probe().
> 
> It seems to me anyway we should try to push as much of this into core logic
> to be reused as possible.
> 
> ---
> bod

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 21/02/2025 00:10, Dmitry Baryshkov wrote:
>> +static int cam_cc_x1e80100_configure_plls(struct device *dev,
>> +                                         const struct qcom_cc_desc *desc,
>> +                                         struct regmap *regmap)
>> +{
>> +       int ret;
>> +
>> +       ret = devm_pm_runtime_enable(dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = pm_runtime_resume_and_get(dev);
>> +       if (ret)
>> +               return ret;
> I think, it's better to add desc->use_rpm. Then these two calls and
> pm_runtime_put() can go to a generic code.
> 
> Or maybe we can enable RPM for all clock controllers?

That second point is pretty interesting - I think at this stage ~ all of 
them do this boilerplate stuff over and over again ..

---
bod

Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Jagadeesh Kona]

On 2/20/2025 4:18 PM, Dmitry Baryshkov wrote:
> On Thu, Feb 20, 2025 at 12:43:42PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 2/19/2025 5:27 PM, Dmitry Baryshkov wrote:
>>> On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote:
>>>>
>>>>
>>>> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
>>>>>> Attach clock power domains in qcom_cc_really_probe() only
>>>>>> if the clock controller has not already attached to them.
>>>>>
>>>>> Squash this to the previous patch and call the new function. No need to
>>>>> duplicate the code.
>>>>>
>>>>
>>>> I tried calling the new function here instead of duplicating code, but that
>>>> is leading to below warning since the desc passed to qcom_cc_really_probe()
>>>> has a const qualifier and hence we cannot update desc->pd_list inside
>>>> qcom_cc_really_probe().
>>>>
>>>> drivers/clk/qcom/common.c:305:33:   WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>>>
>>> It sounds like this can be fixed with a one-line patch.
>>>
>>
>> Removing const qualifier to qcom_cc_really_probe() will fix this, but that requires changes in
>> many other drivers which are currently passing const descriptor to it.
> 
> And this points out that the pd_list should not be a part of the
> struct qcom_cc_desc. You are not using it in the code, so allocate that
> memory on the fly, pass it to devm_pm_domain_attach_list() and then
> forget about it.
> 

Above suggestion looks good, but we need to store the pd_list to pass it to GDSC driver to attach
the power domains as GDSC parent domains. Instead, we can add a new API wrapper for attaching PDs
and to map the regmap(qcom_cc_attach_pds_map) and all clock drivers that need multiple power domains
support can update to use below new API and all new clock drivers can just use the new API.

The implementation would be something like below

--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
+struct regmap *qcom_cc_attach_pds_map(struct platform_device *pdev, struct qcom_cc_desc *desc)
+{
+       int ret;
+
+       ret = devm_pm_domain_attach_list(&pdev->dev, NULL, &desc->pd_list);
+       if (ret < 0 && ret != -EEXIST)
+               return ERR_PTR(ret);
+
+       return qcom_cc_map(pdev, desc);
+}
+EXPORT_SYMBOL_GPL(qcom_cc_attach_pds_map);
+


--- a/drivers/clk/qcom/videocc-sm8550.c
+++ b/drivers/clk/qcom/videocc-sm8550.c
@@ -542,6 +542,12 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
        int ret;
        u32 sleep_clk_offset = 0x8140;

+       regmap = qcom_cc_attach_pds_map(pdev, &video_cc_sm8550_desc);
+       if (IS_ERR(regmap)) {
+               pm_runtime_put(&pdev->dev);
+               return PTR_ERR(regmap);
+       }
+
        ret = devm_pm_runtime_enable(&pdev->dev);
        if (ret)
                return ret;
@@ -550,12 +556,6 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
        if (ret)
                return ret;

-       regmap = qcom_cc_map(pdev, &video_cc_sm8550_desc);
-       if (IS_ERR(regmap)) {
-               pm_runtime_put(&pdev->dev);
-               return PTR_ERR(regmap);
-       }
-

This way also, we are aligning more towards common code and the code will be uniform across all
clock drivers and this doesn't require separate callback in each individual clock driver.

Thanks,
Jagadeesh 

>>
>> But I can squash this to my previous patch by updating my qcom_cc_attach_pds() function
>> prototype as below and then calling that new function here
>>
>> -int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc)
>> +int qcom_cc_attach_pds(struct device *dev, struct dev_pm_domain_list *pd_list)
>>
>> -               ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>> -               if (ret < 0 && ret != -EEXIST)
>> +               ret = qcom_cc_attach_pds(dev, cc->pd_list);
>> +               if (ret)
>>
>> Thanks,
>> Jagadeesh
>>
>>>>
>>>> Thanks,
>>>> Jagadeesh
>>>>
>>>>>>
>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>> ---
>>>>>>  drivers/clk/qcom/common.c | 9 ++++++---
>>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>>>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
>>>>>> --- a/drivers/clk/qcom/common.c
>>>>>> +++ b/drivers/clk/qcom/common.c
>>>>>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
>>>>>>  	if (!cc)
>>>>>>  		return -ENOMEM;
>>>>>>  
>>>>>> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>>>>>> -	if (ret < 0 && ret != -EEXIST)
>>>>>> -		return ret;
>>>>>> +	cc->pd_list = desc->pd_list;
>>>>>> +	if (!cc->pd_list) {
>>>>>> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>>>>>> +		if (ret < 0 && ret != -EEXIST)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>>  
>>>>>>  	reset = &cc->reset;
>>>>>>  	reset->rcdev.of_node = dev->of_node;
>>>>>>
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>>>
>>>
> 

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

On 2/21/2025 4:01 AM, Bryan O'Donoghue wrote:
> On 20/02/2025 07:15, Jagadeesh Kona wrote:
>>
>>
>> On 2/19/2025 5:37 PM, Bryan O'Donoghue wrote:
>>> On 19/02/2025 11:59, Dmitry Baryshkov wrote:
>>>> On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote:
>>>>>
>>>>>
>>>>> On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
>>>>>> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>>>>>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>>>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>>>>>> During boot-up, the PLL configuration might be missed even after
>>>>>>>>> calling pll_configure() from the clock controller probe. This can
>>>>>>>>> happen because the PLL is connected to one or more rails that are
>>>>>>>>> turned off, and the current clock controller code cannot enable
>>>>>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>>>>>> with suboptimal settings, causing functional issues.
>>>>>>>>>
>>>>>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>>>>>> these platforms.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>>>>> ---
>>>>>>>>>      drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>>>>>      drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>>>>>      2 files changed, 8 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>>>>>          struct regmap *regmap;
>>>>>>>>>          int ret;
>>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>>>>>> +    if (ret)
>>>>>>>>> +        return ret;
>>>>>>>>> +
>>>>>>>>>          ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>>          if (ret)
>>>>>>>>>              return ret;
>>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>>>>>          int ret;
>>>>>>>>>          u32 sleep_clk_offset = 0x8140;
>>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>>>>>> +    if (ret)
>>>>>>>>> +        return ret;
>>>>>>>>> +
>>>>>>>>>          ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>>          if (ret)
>>>>>>>>>              return ret;
>>>>>>>>>
>>>>>>>>
>>>>>>>> What's the difference between doing the attach here or doing it in
>>>>>>>> really_probe() ?
>>>>>>>
>>>>>>> I'd second this. If the domains are to be attached before calling any
>>>>>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>>>>>> get all domains attached before configuring PLLs instead of manually
>>>>>>> calling the function.
>>>>>>>
>>>>>>>> There doesn't seem to be any difference except that we will have an
>>>>>>>> additional delay introduced.
>>>>>>>>
>>>>>>>> Are you describing a race condition ?
>>>>>>>>
>>>>>>>> I don't see _logic_ here to moving the call into the controller's higher
>>>>>>>> level probe.
>>>>>>>>
>>>>>>>> Can you describe some more ?
>>>>>>>>
>>>>>>>> ---
>>>>>>>> bod
>>>>>>>
>>>>>>
>>>>>> Here's one way this could work
>>>>>>
>>>>>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>>>>>
>>>>>>       clk: qcom: common: Add configure_plls callback prototype
>>>>>>
>>>>>>       Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>>>>>       before configuring PLLs and ensure that the power-domain rail list is
>>>>>>       switched on prior to configuring PLLs.
>>>>>>
>>>>>>       Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>>
>>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>>>> index 9e3380fd71819..1924130814600 100644
>>>>>> --- a/drivers/clk/qcom/common.c
>>>>>> +++ b/drivers/clk/qcom/common.c
>>>>>> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
>>>>>>           if (ret < 0 && ret != -EEXIST)
>>>>>>                   return ret;
>>>>>>
>>>>>> +       if (desc->configure_plls)
>>>>>> +               desc->configure_plls(regmap);
>>>>>> +
>>>>>>           reset = &cc->reset;
>>>>>>           reset->rcdev.of_node = dev->of_node;
>>>>>>           reset->rcdev.ops = &qcom_reset_ops;
>>>>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>>>>> index 7ace5d7f5836a..4955085ff8669 100644
>>>>>> --- a/drivers/clk/qcom/common.h
>>>>>> +++ b/drivers/clk/qcom/common.h
>>>>>> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
>>>>>>           const struct qcom_icc_hws_data *icc_hws;
>>>>>>           size_t num_icc_hws;
>>>>>>           unsigned int icc_first_node_id;
>>>>>> +       void (*configure_plls)(struct regmap *regmap);
>>>>>>    };
>>>>>>
>>>>>> and
>>>>>>
>>>>>> % git diff drivers/clk/qcom/camcc-x1e80100.c
>>>>>> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
>>>>>> index b73524ae64b1b..c9748d1f8a15b 100644
>>>>>> --- a/drivers/clk/qcom/camcc-x1e80100.c
>>>>>> +++ b/drivers/clk/qcom/camcc-x1e80100.c
>>>>>> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
>>>>>>           .fast_io = true,
>>>>>>    };
>>>>>>
>>>>>> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
>>>>>> +{
>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>>> +
>>>>>> +       /* Keep clocks always enabled */
>>>>>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>>> +}
>>>>>> +
>>>>>>    static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>>           .config = &cam_cc_x1e80100_regmap_config,
>>>>>>           .clks = cam_cc_x1e80100_clocks,
>>>>>> @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>>           .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>>>>>           .gdscs = cam_cc_x1e80100_gdscs,
>>>>>>           .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>>>>>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>>>>>    };
>>>>>>
>>>>>>    static const struct of_device_id cam_cc_x1e80100_match_table[] = {
>>>>>> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
>>>>>>                   return PTR_ERR(regmap);
>>>>>>           }
>>>>>>
>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>>> -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>>> -
>>>>>> -       /* Keep clocks always enabled */
>>>>>> -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>>> -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>>> -
>>>>>>           ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
>>>>>>
>>>>>>           pm_runtime_put(&pdev->dev);
>>>>>>
>>>>>> Or a least it works for me.
>>>>>>
>>>>>
>>>>> This patch will not work in all cases, maybe in your case required power domains might be ON
>>>>> from bootloaders so it might be working.
>>>>
>>>> But with his patch domains are attached before configuring the PLLs, are
>>>> they not?
>>>
>>> Yes, its logically the same just done in core code.
>>>
>>
>> Yes, this code attaches domains before configuring the PLLs, but it attaches PDs after get_sync()
>> is called on device. As I mentioned in other patch earlier, if we attach PDS after get_sync() is
>> already called on device, then power domains are not getting enabled during the probe, leading to
>> the same improper PLL configuration issue. But the current patch series posted will fix this issue
>>
>>>>>
>>>>>> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
>>>>>>
>>>>>
>>>>> No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
>>>>> with this than the current approach.
>>>>
>>>> Bryan's proposal moves us towards having a common code, so it's better.
>>>>
>>>
>>> I can take the time to do the whole sweep and publish a RFC.
>>>
>>
>> Yes, but moving the PLL configuration to callback will not solve the actual PLL configuration
>> issue being discussed here.
>>
>> Thanks,
>> Jagadeesh
>>
> 
> Right what you are really saying is that the power-rails for the clock controller need to remain always on at the moment.
> 
> Where we can zap the GDSCs the power-rails for the block should be always on because the initial PLL configuration we typically do in probe() would be negated as soon as the power rail for the block is switched off.
> 
> True.
> 
> In my opinion:
> 
> - We should only do the pd list addition in one place
>   Either that or push it into each driver.
> 
>   I don't favour doing it in each driver since it is boilerplate
>   code that we basically just end up copy/pasting again and again.
> 
> - We can start off by only including a configure_pll callback
>   for the 2-3 blocks where we know we have multiple rails
> 
> This here works well for me on x1e:
> 
> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Date:   Tue Feb 18 19:46:55 2025 +0000
> 
>     clk: qcom: common: Add configure_plls callback prototype
> 
>     Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>     before configuring PLLs and ensure that the power-domain rail list is
>     switched on prior to configuring PLLs.
> 
>     Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 9e3380fd71819..4aa00ad51c2f6 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -304,6 +304,12 @@ int qcom_cc_really_probe(struct device *dev,
>         if (ret < 0 && ret != -EEXIST)
>                 return ret;
> 
> +       if (desc->configure_plls) {
> +               ret = desc->configure_plls(dev, desc, regmap);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         reset = &cc->reset;
>         reset->rcdev.of_node = dev->of_node;
>         reset->rcdev.ops = &qcom_reset_ops;
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 7ace5d7f5836a..77002e39337d7 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -38,6 +38,9 @@ struct qcom_cc_desc {
>         const struct qcom_icc_hws_data *icc_hws;
>         size_t num_icc_hws;
>         unsigned int icc_first_node_id;
> +       int (*configure_plls)(struct device *dev,
> +                             const struct qcom_cc_desc *desc,
> +                             struct regmap *regmap);
>  };
> 
> +static int cam_cc_x1e80100_configure_plls(struct device *dev,
> +                                         const struct qcom_cc_desc *desc,
> +                                         struct regmap *regmap)
> +{
> +       int ret;
> +
> +       ret = devm_pm_runtime_enable(dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = pm_runtime_resume_and_get(dev);
> +       if (ret)
> +               return ret;
> +
> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
> +
> +       /* Keep clocks always enabled */
> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> +
> +       pm_runtime_put(dev);
> +
> +       return 0;
> +}
> +
>  static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>         .config = &cam_cc_x1e80100_regmap_config,
>         .clks = cam_cc_x1e80100_clocks,
> @@ -2434,6 +2465,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>         .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>         .gdscs = cam_cc_x1e80100_gdscs,
>         .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>  };
> 
> This has the same effect as you were alluding to and in fact we could probably even move the pm_runtime_enable/resume_and_get and pm_runtime_put into really_probe().
> 
> It seems to me anyway we should try to push as much of this into core logic to be reused as possible.
> 

As per the issue I pointer earlier, I see now you moved the get_sync() call to after the attach_pds().
But this PLL callback approach also requires changes in each individual clock driver, and adding a callback
in each clock controller driver to configure the PLLs doesn't reduce any boiler plate code in my opinion.
Infact I feel this is harder to maintain as the code in callback is not constant and vary from one
driver to another. Instead the current approach to explicitly attach pds if we have multiple power
domains is much easier to maintain since it is the same function call in every driver.

Kindly review the discussion in another thread[PATCH 3/5] as that will avoid these extra callbacks in
each driver and the code will be uniform across all the clock drivers

Thanks,
Jagadeesh

> ---
> bod

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Bryan O'Donoghue]

On 21/02/2025 11:43, Jagadeesh Kona wrote:
> 
> 
> On 2/21/2025 4:01 AM, Bryan O'Donoghue wrote:
>> On 20/02/2025 07:15, Jagadeesh Kona wrote:
>>>
>>>
>>> On 2/19/2025 5:37 PM, Bryan O'Donoghue wrote:
>>>> On 19/02/2025 11:59, Dmitry Baryshkov wrote:
>>>>> On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote:
>>>>>>
>>>>>>
>>>>>> On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
>>>>>>> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>>>>>>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>>>>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>>>>>>> During boot-up, the PLL configuration might be missed even after
>>>>>>>>>> calling pll_configure() from the clock controller probe. This can
>>>>>>>>>> happen because the PLL is connected to one or more rails that are
>>>>>>>>>> turned off, and the current clock controller code cannot enable
>>>>>>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>>>>>>> with suboptimal settings, causing functional issues.
>>>>>>>>>>
>>>>>>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>>>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>>>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>>>>>>> these platforms.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>>>>>>       drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>>>>>>       2 files changed, 8 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>>>>>>           struct regmap *regmap;
>>>>>>>>>>           int ret;
>>>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>>>>>>> +    if (ret)
>>>>>>>>>> +        return ret;
>>>>>>>>>> +
>>>>>>>>>>           ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>>>           if (ret)
>>>>>>>>>>               return ret;
>>>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>>>>>>           int ret;
>>>>>>>>>>           u32 sleep_clk_offset = 0x8140;
>>>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>>>>>>> +    if (ret)
>>>>>>>>>> +        return ret;
>>>>>>>>>> +
>>>>>>>>>>           ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>>>           if (ret)
>>>>>>>>>>               return ret;
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> What's the difference between doing the attach here or doing it in
>>>>>>>>> really_probe() ?
>>>>>>>>
>>>>>>>> I'd second this. If the domains are to be attached before calling any
>>>>>>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>>>>>>> get all domains attached before configuring PLLs instead of manually
>>>>>>>> calling the function.
>>>>>>>>
>>>>>>>>> There doesn't seem to be any difference except that we will have an
>>>>>>>>> additional delay introduced.
>>>>>>>>>
>>>>>>>>> Are you describing a race condition ?
>>>>>>>>>
>>>>>>>>> I don't see _logic_ here to moving the call into the controller's higher
>>>>>>>>> level probe.
>>>>>>>>>
>>>>>>>>> Can you describe some more ?
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> bod
>>>>>>>>
>>>>>>>
>>>>>>> Here's one way this could work
>>>>>>>
>>>>>>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>>>>>>
>>>>>>>        clk: qcom: common: Add configure_plls callback prototype
>>>>>>>
>>>>>>>        Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>>>>>>        before configuring PLLs and ensure that the power-domain rail list is
>>>>>>>        switched on prior to configuring PLLs.
>>>>>>>
>>>>>>>        Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>>>
>>>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>>>>> index 9e3380fd71819..1924130814600 100644
>>>>>>> --- a/drivers/clk/qcom/common.c
>>>>>>> +++ b/drivers/clk/qcom/common.c
>>>>>>> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
>>>>>>>            if (ret < 0 && ret != -EEXIST)
>>>>>>>                    return ret;
>>>>>>>
>>>>>>> +       if (desc->configure_plls)
>>>>>>> +               desc->configure_plls(regmap);
>>>>>>> +
>>>>>>>            reset = &cc->reset;
>>>>>>>            reset->rcdev.of_node = dev->of_node;
>>>>>>>            reset->rcdev.ops = &qcom_reset_ops;
>>>>>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>>>>>> index 7ace5d7f5836a..4955085ff8669 100644
>>>>>>> --- a/drivers/clk/qcom/common.h
>>>>>>> +++ b/drivers/clk/qcom/common.h
>>>>>>> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
>>>>>>>            const struct qcom_icc_hws_data *icc_hws;
>>>>>>>            size_t num_icc_hws;
>>>>>>>            unsigned int icc_first_node_id;
>>>>>>> +       void (*configure_plls)(struct regmap *regmap);
>>>>>>>     };
>>>>>>>
>>>>>>> and
>>>>>>>
>>>>>>> % git diff drivers/clk/qcom/camcc-x1e80100.c
>>>>>>> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
>>>>>>> index b73524ae64b1b..c9748d1f8a15b 100644
>>>>>>> --- a/drivers/clk/qcom/camcc-x1e80100.c
>>>>>>> +++ b/drivers/clk/qcom/camcc-x1e80100.c
>>>>>>> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
>>>>>>>            .fast_io = true,
>>>>>>>     };
>>>>>>>
>>>>>>> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
>>>>>>> +{
>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>>>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>>>> +
>>>>>>> +       /* Keep clocks always enabled */
>>>>>>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>>>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>>>> +}
>>>>>>> +
>>>>>>>     static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>>>            .config = &cam_cc_x1e80100_regmap_config,
>>>>>>>            .clks = cam_cc_x1e80100_clocks,
>>>>>>> @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>>>            .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>>>>>>            .gdscs = cam_cc_x1e80100_gdscs,
>>>>>>>            .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>>>>>>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>>>>>>     };
>>>>>>>
>>>>>>>     static const struct of_device_id cam_cc_x1e80100_match_table[] = {
>>>>>>> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
>>>>>>>                    return PTR_ERR(regmap);
>>>>>>>            }
>>>>>>>
>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>>>> -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>>>> -
>>>>>>> -       /* Keep clocks always enabled */
>>>>>>> -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>>>> -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>>>> -
>>>>>>>            ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
>>>>>>>
>>>>>>>            pm_runtime_put(&pdev->dev);
>>>>>>>
>>>>>>> Or a least it works for me.
>>>>>>>
>>>>>>
>>>>>> This patch will not work in all cases, maybe in your case required power domains might be ON
>>>>>> from bootloaders so it might be working.
>>>>>
>>>>> But with his patch domains are attached before configuring the PLLs, are
>>>>> they not?
>>>>
>>>> Yes, its logically the same just done in core code.
>>>>
>>>
>>> Yes, this code attaches domains before configuring the PLLs, but it attaches PDs after get_sync()
>>> is called on device. As I mentioned in other patch earlier, if we attach PDS after get_sync() is
>>> already called on device, then power domains are not getting enabled during the probe, leading to
>>> the same improper PLL configuration issue. But the current patch series posted will fix this issue
>>>
>>>>>>
>>>>>>> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
>>>>>>>
>>>>>>
>>>>>> No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
>>>>>> with this than the current approach.
>>>>>
>>>>> Bryan's proposal moves us towards having a common code, so it's better.
>>>>>
>>>>
>>>> I can take the time to do the whole sweep and publish a RFC.
>>>>
>>>
>>> Yes, but moving the PLL configuration to callback will not solve the actual PLL configuration
>>> issue being discussed here.
>>>
>>> Thanks,
>>> Jagadeesh
>>>
>>
>> Right what you are really saying is that the power-rails for the clock controller need to remain always on at the moment.
>>
>> Where we can zap the GDSCs the power-rails for the block should be always on because the initial PLL configuration we typically do in probe() would be negated as soon as the power rail for the block is switched off.
>>
>> True.
>>
>> In my opinion:
>>
>> - We should only do the pd list addition in one place
>>    Either that or push it into each driver.
>>
>>    I don't favour doing it in each driver since it is boilerplate
>>    code that we basically just end up copy/pasting again and again.
>>
>> - We can start off by only including a configure_pll callback
>>    for the 2-3 blocks where we know we have multiple rails
>>
>> This here works well for me on x1e:
>>
>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>
>>      clk: qcom: common: Add configure_plls callback prototype
>>
>>      Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>      before configuring PLLs and ensure that the power-domain rail list is
>>      switched on prior to configuring PLLs.
>>
>>      Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>
>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>> index 9e3380fd71819..4aa00ad51c2f6 100644
>> --- a/drivers/clk/qcom/common.c
>> +++ b/drivers/clk/qcom/common.c
>> @@ -304,6 +304,12 @@ int qcom_cc_really_probe(struct device *dev,
>>          if (ret < 0 && ret != -EEXIST)
>>                  return ret;
>>
>> +       if (desc->configure_plls) {
>> +               ret = desc->configure_plls(dev, desc, regmap);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>>          reset = &cc->reset;
>>          reset->rcdev.of_node = dev->of_node;
>>          reset->rcdev.ops = &qcom_reset_ops;
>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>> index 7ace5d7f5836a..77002e39337d7 100644
>> --- a/drivers/clk/qcom/common.h
>> +++ b/drivers/clk/qcom/common.h
>> @@ -38,6 +38,9 @@ struct qcom_cc_desc {
>>          const struct qcom_icc_hws_data *icc_hws;
>>          size_t num_icc_hws;
>>          unsigned int icc_first_node_id;
>> +       int (*configure_plls)(struct device *dev,
>> +                             const struct qcom_cc_desc *desc,
>> +                             struct regmap *regmap);
>>   };
>>
>> +static int cam_cc_x1e80100_configure_plls(struct device *dev,
>> +                                         const struct qcom_cc_desc *desc,
>> +                                         struct regmap *regmap)
>> +{
>> +       int ret;
>> +
>> +       ret = devm_pm_runtime_enable(dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = pm_runtime_resume_and_get(dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>> +
>> +       /* Keep clocks always enabled */
>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>> +
>> +       pm_runtime_put(dev);
>> +
>> +       return 0;
>> +}
>> +
>>   static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>          .config = &cam_cc_x1e80100_regmap_config,
>>          .clks = cam_cc_x1e80100_clocks,
>> @@ -2434,6 +2465,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>          .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>          .gdscs = cam_cc_x1e80100_gdscs,
>>          .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>   };
>>
>> This has the same effect as you were alluding to and in fact we could probably even move the pm_runtime_enable/resume_and_get and pm_runtime_put into really_probe().
>>
>> It seems to me anyway we should try to push as much of this into core logic to be reused as possible.
>>
> 
> As per the issue I pointer earlier, I see now you moved the get_sync() call to after the attach_pds().
> But this PLL callback approach also requires changes in each individual clock driver,

That's up for discussion.

We can do it for new drivers and for existing drivers where we know we 
have multiple rails. It need not be a blanket sweep of all of the older 
drivers - for example 8996 or 8916.

That's why the example code I sent you checks for the validity of the 
callback.

Right now the only places we _require_ this sequencing are what

- sm8450 videocc/camcc
- x1e videocc/camcc
- sm8550/sm8650/sm8750 videocc/camcc ?

Certainly not the ~ 80 something clock drivers we have.

  and adding a callback
> in each clock controller driver to configure the PLLs doesn't reduce any boiler plate code in my opinion.
> Infact I feel this is harder to maintain as the code in callback is not constant and vary from one
> driver to another. Instead the current approach to explicitly attach pds if we have multiple power
> domains is much easier to maintain since it is the same function call in every driver.

I understand your reluctance to change 80 drivers but, that's not the 
proposal.

We need only fix for new and existing - where its required.

> Kindly review the discussion in another thread[PATCH 3/5] as that will avoid these extra callbacks in
> each driver and the code will be uniform across all the clock drivers

My feedback is - still:

- Don't do qcom_cc_attach_pds twice, that is incorrect.
- Move the sequencing into core because it is replicated over and over
   again so it is a waste of time just copy/pasting over and over again.
- Describe the change correctly - you need the power rails to stay
   always on so you need to do qcom_cc_attach_pds prior to
   devm_pm_runtime_enable

And in fact this whole dance with pm_rumtime_dostuff() should go into 
common code - so that it gets fixed _once_

That's my honest and unfiltered feedback.

---
bod

Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Dmitry Baryshkov]

On Fri, Feb 21, 2025 at 05:12:58PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 2/20/2025 4:18 PM, Dmitry Baryshkov wrote:
> > On Thu, Feb 20, 2025 at 12:43:42PM +0530, Jagadeesh Kona wrote:
> >>
> >>
> >> On 2/19/2025 5:27 PM, Dmitry Baryshkov wrote:
> >>> On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote:
> >>>>
> >>>>
> >>>> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote:
> >>>>> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
> >>>>>> Attach clock power domains in qcom_cc_really_probe() only
> >>>>>> if the clock controller has not already attached to them.
> >>>>>
> >>>>> Squash this to the previous patch and call the new function. No need to
> >>>>> duplicate the code.
> >>>>>
> >>>>
> >>>> I tried calling the new function here instead of duplicating code, but that
> >>>> is leading to below warning since the desc passed to qcom_cc_really_probe()
> >>>> has a const qualifier and hence we cannot update desc->pd_list inside
> >>>> qcom_cc_really_probe().
> >>>>
> >>>> drivers/clk/qcom/common.c:305:33:   WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >>>
> >>> It sounds like this can be fixed with a one-line patch.
> >>>
> >>
> >> Removing const qualifier to qcom_cc_really_probe() will fix this, but that requires changes in
> >> many other drivers which are currently passing const descriptor to it.
> > 
> > And this points out that the pd_list should not be a part of the
> > struct qcom_cc_desc. You are not using it in the code, so allocate that
> > memory on the fly, pass it to devm_pm_domain_attach_list() and then
> > forget about it.
> > 
> 
> Above suggestion looks good, but we need to store the pd_list to pass it to GDSC driver to attach
> the power domains as GDSC parent domains. Instead, we can add a new API wrapper for attaching PDs
> and to map the regmap(qcom_cc_attach_pds_map) and all clock drivers that need multiple power domains
> support can update to use below new API and all new clock drivers can just use the new API.
> 
> The implementation would be something like below
> 
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> +struct regmap *qcom_cc_attach_pds_map(struct platform_device *pdev, struct qcom_cc_desc *desc)

I think it was established that qcom_cc_desc should be read-only. In the
end it is a description. If you want to pass this data to gdsc probing,
add new argument to qcom_cc_really_probe().

> +{
> +       int ret;
> +
> +       ret = devm_pm_domain_attach_list(&pdev->dev, NULL, &desc->pd_list);
> +       if (ret < 0 && ret != -EEXIST)
> +               return ERR_PTR(ret);
> +
> +       return qcom_cc_map(pdev, desc);
> +}
> +EXPORT_SYMBOL_GPL(qcom_cc_attach_pds_map);
> +
> 
> 
> --- a/drivers/clk/qcom/videocc-sm8550.c
> +++ b/drivers/clk/qcom/videocc-sm8550.c
> @@ -542,6 +542,12 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>         int ret;
>         u32 sleep_clk_offset = 0x8140;
> 
> +       regmap = qcom_cc_attach_pds_map(pdev, &video_cc_sm8550_desc);
> +       if (IS_ERR(regmap)) {
> +               pm_runtime_put(&pdev->dev);
> +               return PTR_ERR(regmap);
> +       }
> +
>         ret = devm_pm_runtime_enable(&pdev->dev);
>         if (ret)
>                 return ret;
> @@ -550,12 +556,6 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
> 
> -       regmap = qcom_cc_map(pdev, &video_cc_sm8550_desc);
> -       if (IS_ERR(regmap)) {
> -               pm_runtime_put(&pdev->dev);
> -               return PTR_ERR(regmap);
> -       }
> -
> 
> This way also, we are aligning more towards common code and the code will be uniform across all
> clock drivers and this doesn't require separate callback in each individual clock driver.
> 
> Thanks,
> Jagadeesh 
> 
> >>
> >> But I can squash this to my previous patch by updating my qcom_cc_attach_pds() function
> >> prototype as below and then calling that new function here
> >>
> >> -int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc)
> >> +int qcom_cc_attach_pds(struct device *dev, struct dev_pm_domain_list *pd_list)
> >>
> >> -               ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> >> -               if (ret < 0 && ret != -EEXIST)
> >> +               ret = qcom_cc_attach_pds(dev, cc->pd_list);
> >> +               if (ret)
> >>
> >> Thanks,
> >> Jagadeesh
> >>
> >>>>
> >>>> Thanks,
> >>>> Jagadeesh
> >>>>
> >>>>>>
> >>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>>>> ---
> >>>>>>  drivers/clk/qcom/common.c | 9 ++++++---
> >>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> >>>>>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
> >>>>>> --- a/drivers/clk/qcom/common.c
> >>>>>> +++ b/drivers/clk/qcom/common.c
> >>>>>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
> >>>>>>  	if (!cc)
> >>>>>>  		return -ENOMEM;
> >>>>>>  
> >>>>>> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> >>>>>> -	if (ret < 0 && ret != -EEXIST)
> >>>>>> -		return ret;
> >>>>>> +	cc->pd_list = desc->pd_list;
> >>>>>> +	if (!cc->pd_list) {
> >>>>>> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> >>>>>> +		if (ret < 0 && ret != -EEXIST)
> >>>>>> +			return ret;
> >>>>>> +	}
> >>>>>>  
> >>>>>>  	reset = &cc->reset;
> >>>>>>  	reset->rcdev.of_node = dev->of_node;
> >>>>>>
> >>>>>> -- 
> >>>>>> 2.34.1
> >>>>>>
> >>>>>
> >>>
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Konrad Dybcio]

On 18.02.2025 4:46 PM, Bryan O'Donoghue wrote:
> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>> During boot-up, the PLL configuration might be missed even after
>> calling pll_configure() from the clock controller probe. This can
>> happen because the PLL is connected to one or more rails that are
>> turned off, and the current clock controller code cannot enable
>> multiple rails during probe. Consequently, the PLL may be activated
>> with suboptimal settings, causing functional issues.
>>
>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>> Therefore, add support to attach multiple power domains to videocc on
>> these platforms.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
>>   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>> --- a/drivers/clk/qcom/videocc-sm8450.c
>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>       struct regmap *regmap;
>>       int ret;
>>   +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>> +    if (ret)
>> +        return ret;
>> +
>>       ret = devm_pm_runtime_enable(&pdev->dev);
>>       if (ret)
>>           return ret;
>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>> --- a/drivers/clk/qcom/videocc-sm8550.c
>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>       int ret;
>>       u32 sleep_clk_offset = 0x8140;
>>   +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>> +    if (ret)
>> +        return ret;
>> +
>>       ret = devm_pm_runtime_enable(&pdev->dev);
>>       if (ret)
>>           return ret;
>>
> 
> What's the difference between doing the attach here or doing it in really_probe() ?
> 
> There doesn't seem to be any difference except that we will have an additional delay introduced.
> 
> Are you describing a race condition ?
> 
> I don't see _logic_ here to moving the call into the controller's higher level probe.
> 
> Can you describe some more ?

I think this is a sane course of action:

1. associate pll config with the pll (treewide change)
2. add a generic pll_configure call that parses the type
3. store PLLs to be configured in an array of dt-bindings indices
4. move PLL configuration to really_probe
5. move RPM enablement to really_probe, make it conditional to limit resource
   waste on e.g. gcc
6. move attaching GDSCs to really_probe

Konrad

Re: [PATCH 1/5] dt-bindings: clock: qcom,sm8450-videocc: Add MXC power domain

[Rob Herring (Arm)]

On Tue, 18 Feb 2025 19:56:46 +0530, Jagadeesh Kona wrote:
> To configure the video PLLs and enable the video GDSCs on SM8450,
> SM8475, SM8550 and SM8650 platforms, the MXC rail must be ON along
> with MMCX. Therefore, update the videocc bindings to include
> the MXC power domain on these platforms.
> 
> Fixes: 1e910b2ba0ed ("dt-bindings: clock: qcom: Add SM8450 video clock controller")
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>  Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

On 2/22/2025 12:49 AM, Konrad Dybcio wrote:
> On 18.02.2025 4:46 PM, Bryan O'Donoghue wrote:
>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>> During boot-up, the PLL configuration might be missed even after
>>> calling pll_configure() from the clock controller probe. This can
>>> happen because the PLL is connected to one or more rails that are
>>> turned off, and the current clock controller code cannot enable
>>> multiple rails during probe. Consequently, the PLL may be activated
>>> with suboptimal settings, causing functional issues.
>>>
>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>> Therefore, add support to attach multiple power domains to videocc on
>>> these platforms.
>>>
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>> ---
>>>   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>       struct regmap *regmap;
>>>       int ret;
>>>   +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>       ret = devm_pm_runtime_enable(&pdev->dev);
>>>       if (ret)
>>>           return ret;
>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>       int ret;
>>>       u32 sleep_clk_offset = 0x8140;
>>>   +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>       ret = devm_pm_runtime_enable(&pdev->dev);
>>>       if (ret)
>>>           return ret;
>>>
>>
>> What's the difference between doing the attach here or doing it in really_probe() ?
>>
>> There doesn't seem to be any difference except that we will have an additional delay introduced.
>>
>> Are you describing a race condition ?
>>
>> I don't see _logic_ here to moving the call into the controller's higher level probe.
>>
>> Can you describe some more ?
> 
> I think this is a sane course of action:
> 
> 1. associate pll config with the pll (treewide change)
> 2. add a generic pll_configure call that parses the type
> 3. store PLLs to be configured in an array of dt-bindings indices
> 4. move PLL configuration to really_probe
> 5. move RPM enablement to really_probe, make it conditional to limit resource
>    waste on e.g. gcc
> 6. move attaching GDSCs to really_probe
> 
> Konrad

Thanks Konrad for your feedback and suggestion.

I will check and work on this approach of moving PLL configure and RPM enablement
to a common code.

Thanks,
Jagadeesh

Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

[Jagadeesh Kona]

On 2/21/2025 6:02 PM, Bryan O'Donoghue wrote:
> On 21/02/2025 11:43, Jagadeesh Kona wrote:
>>
>>
>> On 2/21/2025 4:01 AM, Bryan O'Donoghue wrote:
>>> On 20/02/2025 07:15, Jagadeesh Kona wrote:
>>>>
>>>>
>>>> On 2/19/2025 5:37 PM, Bryan O'Donoghue wrote:
>>>>> On 19/02/2025 11:59, Dmitry Baryshkov wrote:
>>>>>> On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
>>>>>>>> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>>>>>>>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>>>>>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>>>>>>>> During boot-up, the PLL configuration might be missed even after
>>>>>>>>>>> calling pll_configure() from the clock controller probe. This can
>>>>>>>>>>> happen because the PLL is connected to one or more rails that are
>>>>>>>>>>> turned off, and the current clock controller code cannot enable
>>>>>>>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>>>>>>>> with suboptimal settings, causing functional issues.
>>>>>>>>>>>
>>>>>>>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>>>>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>>>>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>>>>>>>> these platforms.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>>>>>>>       drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>>>>>>>       2 files changed, 8 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>>>>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>>>>>>>           struct regmap *regmap;
>>>>>>>>>>>           int ret;
>>>>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>>>>>>>> +    if (ret)
>>>>>>>>>>> +        return ret;
>>>>>>>>>>> +
>>>>>>>>>>>           ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>>>>           if (ret)
>>>>>>>>>>>               return ret;
>>>>>>>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>>>>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>>>>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>>>>>>>           int ret;
>>>>>>>>>>>           u32 sleep_clk_offset = 0x8140;
>>>>>>>>>>> +    ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>>>>>>>> +    if (ret)
>>>>>>>>>>> +        return ret;
>>>>>>>>>>> +
>>>>>>>>>>>           ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>>>>           if (ret)
>>>>>>>>>>>               return ret;
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> What's the difference between doing the attach here or doing it in
>>>>>>>>>> really_probe() ?
>>>>>>>>>
>>>>>>>>> I'd second this. If the domains are to be attached before calling any
>>>>>>>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>>>>>>>> get all domains attached before configuring PLLs instead of manually
>>>>>>>>> calling the function.
>>>>>>>>>
>>>>>>>>>> There doesn't seem to be any difference except that we will have an
>>>>>>>>>> additional delay introduced.
>>>>>>>>>>
>>>>>>>>>> Are you describing a race condition ?
>>>>>>>>>>
>>>>>>>>>> I don't see _logic_ here to moving the call into the controller's higher
>>>>>>>>>> level probe.
>>>>>>>>>>
>>>>>>>>>> Can you describe some more ?
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> bod
>>>>>>>>>
>>>>>>>>
>>>>>>>> Here's one way this could work
>>>>>>>>
>>>>>>>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>>>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>>>>>>>
>>>>>>>>        clk: qcom: common: Add configure_plls callback prototype
>>>>>>>>
>>>>>>>>        Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>>>>>>>        before configuring PLLs and ensure that the power-domain rail list is
>>>>>>>>        switched on prior to configuring PLLs.
>>>>>>>>
>>>>>>>>        Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>>>>
>>>>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>>>>>> index 9e3380fd71819..1924130814600 100644
>>>>>>>> --- a/drivers/clk/qcom/common.c
>>>>>>>> +++ b/drivers/clk/qcom/common.c
>>>>>>>> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
>>>>>>>>            if (ret < 0 && ret != -EEXIST)
>>>>>>>>                    return ret;
>>>>>>>>
>>>>>>>> +       if (desc->configure_plls)
>>>>>>>> +               desc->configure_plls(regmap);
>>>>>>>> +
>>>>>>>>            reset = &cc->reset;
>>>>>>>>            reset->rcdev.of_node = dev->of_node;
>>>>>>>>            reset->rcdev.ops = &qcom_reset_ops;
>>>>>>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>>>>>>> index 7ace5d7f5836a..4955085ff8669 100644
>>>>>>>> --- a/drivers/clk/qcom/common.h
>>>>>>>> +++ b/drivers/clk/qcom/common.h
>>>>>>>> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
>>>>>>>>            const struct qcom_icc_hws_data *icc_hws;
>>>>>>>>            size_t num_icc_hws;
>>>>>>>>            unsigned int icc_first_node_id;
>>>>>>>> +       void (*configure_plls)(struct regmap *regmap);
>>>>>>>>     };
>>>>>>>>
>>>>>>>> and
>>>>>>>>
>>>>>>>> % git diff drivers/clk/qcom/camcc-x1e80100.c
>>>>>>>> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
>>>>>>>> index b73524ae64b1b..c9748d1f8a15b 100644
>>>>>>>> --- a/drivers/clk/qcom/camcc-x1e80100.c
>>>>>>>> +++ b/drivers/clk/qcom/camcc-x1e80100.c
>>>>>>>> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
>>>>>>>>            .fast_io = true,
>>>>>>>>     };
>>>>>>>>
>>>>>>>> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
>>>>>>>> +{
>>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>>>>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>>>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>>>>> +
>>>>>>>> +       /* Keep clocks always enabled */
>>>>>>>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>>>>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>     static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>>>>            .config = &cam_cc_x1e80100_regmap_config,
>>>>>>>>            .clks = cam_cc_x1e80100_clocks,
>>>>>>>> @@ -2434,6 +2449,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>>>>>>            .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>>>>>>>            .gdscs = cam_cc_x1e80100_gdscs,
>>>>>>>>            .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>>>>>>>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>>>>>>>     };
>>>>>>>>
>>>>>>>>     static const struct of_device_id cam_cc_x1e80100_match_table[] = {
>>>>>>>> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
>>>>>>>>                    return PTR_ERR(regmap);
>>>>>>>>            }
>>>>>>>>
>>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>>>>>>> -       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>>>>>>> -       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>>>>>>> -
>>>>>>>> -       /* Keep clocks always enabled */
>>>>>>>> -       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>>>>>>> -       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>>>>>>> -
>>>>>>>>            ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
>>>>>>>>
>>>>>>>>            pm_runtime_put(&pdev->dev);
>>>>>>>>
>>>>>>>> Or a least it works for me.
>>>>>>>>
>>>>>>>
>>>>>>> This patch will not work in all cases, maybe in your case required power domains might be ON
>>>>>>> from bootloaders so it might be working.
>>>>>>
>>>>>> But with his patch domains are attached before configuring the PLLs, are
>>>>>> they not?
>>>>>
>>>>> Yes, its logically the same just done in core code.
>>>>>
>>>>
>>>> Yes, this code attaches domains before configuring the PLLs, but it attaches PDs after get_sync()
>>>> is called on device. As I mentioned in other patch earlier, if we attach PDS after get_sync() is
>>>> already called on device, then power domains are not getting enabled during the probe, leading to
>>>> the same improper PLL configuration issue. But the current patch series posted will fix this issue
>>>>
>>>>>>>
>>>>>>>> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
>>>>>>>>
>>>>>>>
>>>>>>> No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
>>>>>>> with this than the current approach.
>>>>>>
>>>>>> Bryan's proposal moves us towards having a common code, so it's better.
>>>>>>
>>>>>
>>>>> I can take the time to do the whole sweep and publish a RFC.
>>>>>
>>>>
>>>> Yes, but moving the PLL configuration to callback will not solve the actual PLL configuration
>>>> issue being discussed here.
>>>>
>>>> Thanks,
>>>> Jagadeesh
>>>>
>>>
>>> Right what you are really saying is that the power-rails for the clock controller need to remain always on at the moment.
>>>
>>> Where we can zap the GDSCs the power-rails for the block should be always on because the initial PLL configuration we typically do in probe() would be negated as soon as the power rail for the block is switched off.
>>>
>>> True.
>>>
>>> In my opinion:
>>>
>>> - We should only do the pd list addition in one place
>>>    Either that or push it into each driver.
>>>
>>>    I don't favour doing it in each driver since it is boilerplate
>>>    code that we basically just end up copy/pasting again and again.
>>>
>>> - We can start off by only including a configure_pll callback
>>>    for the 2-3 blocks where we know we have multiple rails
>>>
>>> This here works well for me on x1e:
>>>
>>> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Date:   Tue Feb 18 19:46:55 2025 +0000
>>>
>>>      clk: qcom: common: Add configure_plls callback prototype
>>>
>>>      Add a configure_plls() callback so that we can stage qcom_cc_attach_pds()
>>>      before configuring PLLs and ensure that the power-domain rail list is
>>>      switched on prior to configuring PLLs.
>>>
>>>      Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>
>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>> index 9e3380fd71819..4aa00ad51c2f6 100644
>>> --- a/drivers/clk/qcom/common.c
>>> +++ b/drivers/clk/qcom/common.c
>>> @@ -304,6 +304,12 @@ int qcom_cc_really_probe(struct device *dev,
>>>          if (ret < 0 && ret != -EEXIST)
>>>                  return ret;
>>>
>>> +       if (desc->configure_plls) {
>>> +               ret = desc->configure_plls(dev, desc, regmap);
>>> +               if (ret)
>>> +                       return ret;
>>> +       }
>>> +
>>>          reset = &cc->reset;
>>>          reset->rcdev.of_node = dev->of_node;
>>>          reset->rcdev.ops = &qcom_reset_ops;
>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>> index 7ace5d7f5836a..77002e39337d7 100644
>>> --- a/drivers/clk/qcom/common.h
>>> +++ b/drivers/clk/qcom/common.h
>>> @@ -38,6 +38,9 @@ struct qcom_cc_desc {
>>>          const struct qcom_icc_hws_data *icc_hws;
>>>          size_t num_icc_hws;
>>>          unsigned int icc_first_node_id;
>>> +       int (*configure_plls)(struct device *dev,
>>> +                             const struct qcom_cc_desc *desc,
>>> +                             struct regmap *regmap);
>>>   };
>>>
>>> +static int cam_cc_x1e80100_configure_plls(struct device *dev,
>>> +                                         const struct qcom_cc_desc *desc,
>>> +                                         struct regmap *regmap)
>>> +{
>>> +       int ret;
>>> +
>>> +       ret = devm_pm_runtime_enable(dev);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       ret = pm_runtime_resume_and_get(dev);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>> +       clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
>>> +       clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
>>> +
>>> +       /* Keep clocks always enabled */
>>> +       qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>>> +       qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
>>> +
>>> +       pm_runtime_put(dev);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>          .config = &cam_cc_x1e80100_regmap_config,
>>>          .clks = cam_cc_x1e80100_clocks,
>>> @@ -2434,6 +2465,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
>>>          .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
>>>          .gdscs = cam_cc_x1e80100_gdscs,
>>>          .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
>>> +       .configure_plls = cam_cc_x1e80100_configure_plls,
>>>   };
>>>
>>> This has the same effect as you were alluding to and in fact we could probably even move the pm_runtime_enable/resume_and_get and pm_runtime_put into really_probe().
>>>
>>> It seems to me anyway we should try to push as much of this into core logic to be reused as possible.
>>>
>>
>> As per the issue I pointer earlier, I see now you moved the get_sync() call to after the attach_pds().
>> But this PLL callback approach also requires changes in each individual clock driver,
> 
> That's up for discussion.
> 
> We can do it for new drivers and for existing drivers where we know we have multiple rails. It need not be a blanket sweep of all of the older drivers - for example 8996 or 8916.
> 
> That's why the example code I sent you checks for the validity of the callback.
> 
> Right now the only places we _require_ this sequencing are what
> 
> - sm8450 videocc/camcc
> - x1e videocc/camcc
> - sm8550/sm8650/sm8750 videocc/camcc ?
> 
> Certainly not the ~ 80 something clock drivers we have.
> 
>  and adding a callback
>> in each clock controller driver to configure the PLLs doesn't reduce any boiler plate code in my opinion.
>> Infact I feel this is harder to maintain as the code in callback is not constant and vary from one
>> driver to another. Instead the current approach to explicitly attach pds if we have multiple power
>> domains is much easier to maintain since it is the same function call in every driver.
> 
> I understand your reluctance to change 80 drivers but, that's not the proposal.
> 
> We need only fix for new and existing - where its required.
> 
>> Kindly review the discussion in another thread[PATCH 3/5] as that will avoid these extra callbacks in
>> each driver and the code will be uniform across all the clock drivers
> 
> My feedback is - still:
> 
> - Don't do qcom_cc_attach_pds twice, that is incorrect.
> - Move the sequencing into core because it is replicated over and over
>   again so it is a waste of time just copy/pasting over and over again.
> - Describe the change correctly - you need the power rails to stay
>   always on so you need to do qcom_cc_attach_pds prior to
>   devm_pm_runtime_enable
> 
> And in fact this whole dance with pm_rumtime_dostuff() should go into common code - so that it gets fixed _once_
> 
> That's my honest and unfiltered feedback.
> 
> ---
> bod

Thanks Bryan for your feedback.

I will work on the changes as per above and Konrad's suggestions
to handle the things from common code.

Thanks,
Jagadeesh