[PATCH 0/3] media: camss: Link CAMSS power domain on MSM8996

[PATCH 0/3] media: camss: Link CAMSS power domain on MSM8996

[Yassine Oudjana]

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

CAMSS on MSM8996 has been broken since commit
46cc03175498 (media: camss: Split power domain management, 2022-07-04).

This would happen when trying to start streaming:

[  199.097810] ------------[ cut here ]------------
[  199.097893] camss_top_ahb_clk status stuck at 'off'
[  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
...
[  199.100064]  clk_branch_wait+0x140/0x160
[  199.100112]  clk_branch2_enable+0x30/0x40
[  199.100159]  clk_core_enable+0x6c/0xb0
[  199.100211]  clk_enable+0x2c/0x50
[  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
[  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
...
[  199.101594] ---[ end trace 0000000000000000 ]---
[  199.101736] qcom-camss a34000.camss: clock enable failed: -16
[  199.101813] qcom-camss a34000.camss: Failed to power up pipeline: -16

Turns out camss_top_ahb_clk needs the CAMSS power domain to be on. Before the change,
VFE power domains were enabled before CSIPHY enabled clocks, and since the CAMSS power
domain was their parent, it got enabled as well. With the VFE power domains now enabled
after CSIPHY is powered on, the CAMSS power domain remains off and things go south when
CSIPHY tries to enable camss_top_ahb_clk.

Link the CAMSS power domain in camss_configure_pd to make sure it gets enabled before
CSIPHY tries to enable clocks.

Yassine Oudjana (3):
  media: dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power
    domain and power-domain-names
  arm64: dts: qcom: msm8996: Add CAMSS power domain and
    power-domain-names to CAMSS
  media: camss: Link CAMSS power domain

 .../bindings/media/qcom,msm8996-camss.yaml          | 13 ++++++++++++-
 arch/arm64/boot/dts/qcom/msm8996.dtsi               |  4 +++-
 drivers/media/platform/qcom/camss/camss.c           |  9 ++++++++-
 3 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.40.0

[PATCH 1/3] media: dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain and power-domain-names

[Yassine Oudjana]

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

Add the CAMSS power domain which is needed for the proper operation of CAMSS, and add
power-domain-names to ease fetching it as well as the other power domains.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 .../bindings/media/qcom,msm8996-camss.yaml          | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
index 8a10aa1cafc5..27c9a11f0df9 100644
--- a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
@@ -85,6 +85,13 @@ properties:
     items:
       - description: VFE0 GDSC - Video Front End, Global Distributed Switch Controller.
       - description: VFE1 GDSC - Video Front End, Global Distributed Switch Controller.
+      - description: CAMSS GDSC - Camera Subsystem, Global Distributed Switch Controller.
+
+  power-domain-names:
+    items:
+      - const: vfe0
+      - const: vfe1
+      - const: camss
 
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
@@ -209,6 +216,7 @@ required:
   - interrupts
   - iommus
   - power-domains
+  - power-domain-names
   - reg
   - reg-names
   - vdda-supply
@@ -326,7 +334,10 @@ examples:
          <&vfe_smmu 3>;
 
       power-domains = <&mmcc VFE0_GDSC>,
-        <&mmcc VFE1_GDSC>;
+        <&mmcc VFE1_GDSC>,
+        <&mmcc CAMSS_GDSC>;
+
+      power-domain-names = "vfe0", "vfe1", "camss";
 
       reg = <0x00a34000 0x1000>,
         <0x00a00030 0x4>,
-- 
2.40.0

Re: [PATCH 1/3] media: dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain and power-domain-names

[Bryan O'Donoghue]

On 03/05/2023 08:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add the CAMSS power domain which is needed for the proper operation of CAMSS, and add
> power-domain-names to ease fetching it as well as the other power domains.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   .../bindings/media/qcom,msm8996-camss.yaml          | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
> index 8a10aa1cafc5..27c9a11f0df9 100644
> --- a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
> @@ -85,6 +85,13 @@ properties:
>       items:
>         - description: VFE0 GDSC - Video Front End, Global Distributed Switch Controller.
>         - description: VFE1 GDSC - Video Front End, Global Distributed Switch Controller.
> +      - description: CAMSS GDSC - Camera Subsystem, Global Distributed Switch Controller.
> +
> +  power-domain-names:
> +    items:
> +      - const: vfe0
> +      - const: vfe1
> +      - const: camss
>   
>     ports:
>       $ref: /schemas/graph.yaml#/properties/ports
> @@ -209,6 +216,7 @@ required:
>     - interrupts
>     - iommus
>     - power-domains
> +  - power-domain-names
>     - reg
>     - reg-names
>     - vdda-supply
> @@ -326,7 +334,10 @@ examples:
>            <&vfe_smmu 3>;
>   
>         power-domains = <&mmcc VFE0_GDSC>,
> -        <&mmcc VFE1_GDSC>;
> +        <&mmcc VFE1_GDSC>,
> +        <&mmcc CAMSS_GDSC>;
> +
> +      power-domain-names = "vfe0", "vfe1", "camss";
>   
>         reg = <0x00a34000 0x1000>,
>           <0x00a00030 0x4>,

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

Re: [PATCH 1/3] media: dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain and power-domain-names

[Krzysztof Kozlowski]

On 03/05/2023 09:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add the CAMSS power domain which is needed for the proper operation of CAMSS, and add
> power-domain-names to ease fetching it as well as the other power domains.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586

Same for all other patches.


Subject: you have double media, drop the latter one.

Your subject is anyway too long, so shorten it. camss: is also a
redundant prefix.

Best regards,
Krzysztof

[PATCH 2/3] arm64: dts: qcom: msm8996: Add CAMSS power domain and power-domain-names to CAMSS

[Yassine Oudjana]

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

Add the CAMSS power domain as well as power-domain-names for all CAMSS power domains.

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

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 30f6ebc4bd11..0168a086f57d 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -2118,7 +2118,9 @@ camss: camss@a00000 {
 				"vfe0",
 				"vfe1";
 			power-domains = <&mmcc VFE0_GDSC>,
-					<&mmcc VFE1_GDSC>;
+					<&mmcc VFE1_GDSC>,
+					<&mmcc CAMSS_GDSC>;
+			power-domain-names = "vfe0", "vfe1", "camss";
 			clocks = <&mmcc CAMSS_TOP_AHB_CLK>,
 				<&mmcc CAMSS_ISPIF_AHB_CLK>,
 				<&mmcc CAMSS_CSI0PHYTIMER_CLK>,
-- 
2.40.0

Re: [PATCH 2/3] arm64: dts: qcom: msm8996: Add CAMSS power domain and power-domain-names to CAMSS

[Bryan O'Donoghue]

On 03/05/2023 08:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add the CAMSS power domain as well as power-domain-names for all CAMSS power domains.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 30f6ebc4bd11..0168a086f57d 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -2118,7 +2118,9 @@ camss: camss@a00000 {
>   				"vfe0",
>   				"vfe1";
>   			power-domains = <&mmcc VFE0_GDSC>,
> -					<&mmcc VFE1_GDSC>;
> +					<&mmcc VFE1_GDSC>,
> +					<&mmcc CAMSS_GDSC>;
> +			power-domain-names = "vfe0", "vfe1", "camss";
>   			clocks = <&mmcc CAMSS_TOP_AHB_CLK>,
>   				<&mmcc CAMSS_ISPIF_AHB_CLK>,
>   				<&mmcc CAMSS_CSI0PHYTIMER_CLK>,

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

Re: [PATCH 2/3] arm64: dts: qcom: msm8996: Add CAMSS power domain and power-domain-names to CAMSS

[Krzysztof Kozlowski]

On 03/05/2023 09:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add the CAMSS power domain as well as power-domain-names for all CAMSS power domains.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586

You should explain why you are adding it.

> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>  arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 30f6ebc4bd11..0168a086f57d 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -2118,7 +2118,9 @@ camss: camss@a00000 {
>  				"vfe0",
>  				"vfe1";
>  			power-domains = <&mmcc VFE0_GDSC>,
> -					<&mmcc VFE1_GDSC>;
> +					<&mmcc VFE1_GDSC>,
> +					<&mmcc CAMSS_GDSC>;
> +			power-domain-names = "vfe0", "vfe1", "camss";
>  			clocks = <&mmcc CAMSS_TOP_AHB_CLK>,
>  				<&mmcc CAMSS_ISPIF_AHB_CLK>,
>  				<&mmcc CAMSS_CSI0PHYTIMER_CLK>,

Best regards,
Krzysztof

[PATCH 3/3] media: camss: Link CAMSS power domain

[Yassine Oudjana]

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

The CAMSS power domain was previously enabled implicitly when the VFE power domains
were enabled. Commit 46cc03175498 (media: camss: Split power domain management, 2022-07-04)
delayed enabling VFE power domains which in turn delayed enabling the CAMSS power domain.
This made CSIPHY fail to enable camss_top_ahb_clk which requires the CAMSS power domain to
be on:

[  199.097810] ------------[ cut here ]------------
[  199.097893] camss_top_ahb_clk status stuck at 'off'
[  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
...
[  199.100064]  clk_branch_wait+0x140/0x160
[  199.100112]  clk_branch2_enable+0x30/0x40
[  199.100159]  clk_core_enable+0x6c/0xb0
[  199.100211]  clk_enable+0x2c/0x50
[  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
[  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
...
[  199.101594] ---[ end trace 0000000000000000 ]---

Link the CAMSS power domain in camss_configure_pd to make sure it gets enabled before
CSIPHY tries to enable clocks.

Fixes: 02afa816dbbf (media: camss: Add basic runtime PM support, 2018-07-25)
Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1ef26aea3eae..9aea8220d923 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1453,6 +1453,7 @@ static const struct media_device_ops camss_media_ops = {
 static int camss_configure_pd(struct camss *camss)
 {
 	struct device *dev = camss->dev;
+	int camss_pd_index;
 	int i;
 	int ret;
 
@@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss *camss)
 		}
 	}
 
-	if (i > camss->vfe_num) {
+	/* Link CAMSS power domain if available */
+	camss_pd_index = device_property_match_string(camss->dev, "power-domain-names", "camss");
+	if (camss_pd_index >= 0)
+		device_link_add(camss->dev, camss->genpd[camss_pd_index], DL_FLAG_STATELESS |
+				DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
+
+	if (i > camss->vfe_num && i != camss_pd_index) {
 		camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
 							   DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
 							   DL_FLAG_RPM_ACTIVE);
-- 
2.40.0

Re: [PATCH 3/3] media: camss: Link CAMSS power domain

[Bryan O'Donoghue]

On 03/05/2023 08:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> The CAMSS power domain was previously enabled implicitly when the VFE power domains
> were enabled. Commit 46cc03175498 (media: camss: Split power domain management, 2022-07-04)
> delayed enabling VFE power domains which in turn delayed enabling the CAMSS power domain.
> This made CSIPHY fail to enable camss_top_ahb_clk which requires the CAMSS power domain to
> be on:
> 
> [  199.097810] ------------[ cut here ]------------
> [  199.097893] camss_top_ahb_clk status stuck at 'off'
> [  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
> ...
> [  199.100064]  clk_branch_wait+0x140/0x160
> [  199.100112]  clk_branch2_enable+0x30/0x40
> [  199.100159]  clk_core_enable+0x6c/0xb0
> [  199.100211]  clk_enable+0x2c/0x50
> [  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
> [  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
> ...
> [  199.101594] ---[ end trace 0000000000000000 ]---
> 
> Link the CAMSS power domain in camss_configure_pd to make sure it gets enabled before
> CSIPHY tries to enable clocks.
> 
> Fixes: 02afa816dbbf (media: camss: Add basic runtime PM support, 2018-07-25)
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>

Same comment as in the cover letter, your Fixes: tag is broken

WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> 
("<title line>")' - ie: 'Fixes: 02afa816dbbf ("media: camss: Add basic 
runtime PM support")'
#28:
Fixes: 02afa816dbbf (media: camss: Add basic runtime PM support, 2018-07-25)

> ---
>   drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1ef26aea3eae..9aea8220d923 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1453,6 +1453,7 @@ static const struct media_device_ops camss_media_ops = {
>   static int camss_configure_pd(struct camss *camss)
>   {
>   	struct device *dev = camss->dev;
> +	int camss_pd_index;
>   	int i;
>   	int ret;
>   
> @@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss *camss)
>   		}
>   	}
>   
> -	if (i > camss->vfe_num) {
> +	/* Link CAMSS power domain if available */
> +	camss_pd_index = device_property_match_string(camss->dev, "power-domain-names", "camss");
> +	if (camss_pd_index >= 0)
> +		device_link_add(camss->dev, camss->genpd[camss_pd_index], DL_FLAG_STATELESS |
> +				DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
> +
> +	if (i > camss->vfe_num && i != camss_pd_index) {
>   		camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
>   							   DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
>   							   DL_FLAG_RPM_ACTIVE);

The rest of it seems reasonable to me. I'll give it a R/B T/B on your 
next iteration - including Fixes: fix as I'm OOO ATM.

---
bod

Re: [PATCH 0/3] media: camss: Link CAMSS power domain on MSM8996

[Bryan O'Donoghue]

On 03/05/2023 08:25, Yassine Oudjana wrote:
> CAMSS on MSM8996 has been broken since commit
> 46cc03175498 (media: camss: Split power domain management, 2022-07-04).

Don't forget to run your patches through checkpatch --strict

ERROR: Please use git commit description style 'commit <12+ chars of 
sha1> ("<title line>")' - ie: 'Commit 46cc03175498 ("media: camss: Split 
power domain management")'