From: Yassine Oudjana <[email protected]>
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
From: Yassine Oudjana <[email protected]>
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 <[email protected]>
---
.../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
From: Yassine Oudjana <[email protected]>
Add the CAMSS power domain as well as power-domain-names for all CAMSS power domains.
Signed-off-by: Yassine Oudjana <[email protected]>
---
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
From: Yassine Oudjana <[email protected]>
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 <[email protected]>
---
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
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")'
On 03/05/2023 08:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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 <[email protected]>
On 03/05/2023 08:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> Add the CAMSS power domain as well as power-domain-names for all CAMSS power domains.
>
> Signed-off-by: Yassine Oudjana <[email protected]>
> ---
> 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 <[email protected]>
On 03/05/2023 08:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> 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 <[email protected]>
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
On 03/05/2023 09:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> 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
On 03/05/2023 09:25, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> 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 <[email protected]>
> ---
> 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