[PATCH 0/2] media: dt-bindings: media: sm8250-camss: Add power-domain-names property

[PATCH 0/2] media: dt-bindings: media: sm8250-camss: Add power-domain-names property

[Vladimir Zapolskiy]

QCOM SM8250 camera subsystem depends on three power domains, at the moment
all of them are not differentiated one from another, however the power
domains compose a hierarchical structure with vfe0 and vfe1 as subdomains
of titan_top, also managing vfe0 and vfe1 separately allows to get more
fine-grained power control in runtime.

The change relates to my review comment for v2 of CAMSS on SM8250 submission:

   https://lore.kernel.org/all/13ad033e-cd5d-3a8c-b036-50a3ac4245c0@linaro.org/

Apparently it becomes important to manage CAMSS power domains much better for
newer platforms, this referes to platforms with Titan GDSC, for instance CAMSS
on SM8450 has 6 power domains, and dealing with them in bulk is not an option.

There was a note in commit 2f6f8af67203 ("media: camss: Refactor VFE power
domain toggling") about problems with power VFE domains on/off, but perhaps
it's related to the fact that Titan GDSC is a special power domain and VFE
are subdomains, the latter shall not be enabled earlier than the Titan, but
the driver did not construct a proper hierarchy and leaves a room for races.

The change should have no implications on any SM8250 CAMSS users, since
none of the supported in upstream boards enables the camss device tree node.
The correspondent changes in the driver will follow this dt specific series.

Most likely a similar change is required for SDM845 platform, but it would
need additional investigation and testing.

Vladimir Zapolskiy (2):
  media: dt-bindings: media: sm8250-camss: Add power-domain-names property
  arm64: dts: qcom: sm8250: camss: Add power-domain-names property

 .../devicetree/bindings/media/qcom,sm8250-camss.yaml       | 7 +++++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi                       | 1 +
 2 files changed, 8 insertions(+)

-- 
2.33.0

[PATCH 1/2] media: dt-bindings: media: sm8250-camss: Add power-domain-names property

[Vladimir Zapolskiy]

QCOM SM8250 camera subsystem depends on three power domains, at the moment
all of them are not differentiated one from another, however the power
domains compose a hierarchical structure with vfe0 and vfe1 as subdomains
of titan_top, also managing vfe0 and vfe1 separately allows to get more
fine-grained power control in runtime.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 .../devicetree/bindings/media/qcom,sm8250-camss.yaml       | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
index 07a2af12f37d..1ef7ea985ce2 100644
--- a/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
@@ -103,6 +103,12 @@ properties:
       - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
       - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
 
+  power-domain-names:
+    items:
+      - const: vfe0
+      - const: vfe1
+      - const: titan_top
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
@@ -361,6 +367,7 @@ examples:
             power-domains = <&camcc IFE_0_GDSC>,
                             <&camcc IFE_1_GDSC>,
                             <&camcc TITAN_TOP_GDSC>;
+            power-domain-names = "vfe0", "vfe1", "titan_top";
 
             clocks = <&gcc GCC_CAMERA_AHB_CLK>,
                      <&gcc GCC_CAMERA_HF_AXI_CLK>,
-- 
2.33.0

[PATCH 2/2] arm64: dts: qcom: sm8250: camss: Add power-domain-names property

[Vladimir Zapolskiy]

QCOM SM8250 camera subsystem depends on three power domains, at the moment
all of them are not differentiated one from another, however the power
domains compose a hierarchical structure with vfe0 and vfe1 as subdomains
of titan_top, also managing vfe0 and vfe1 separately allows to get more
fine-grained power control in runtime.

The change should have no implications on any SM8250 CAMSS users, since
none of the boards supported in upstream enables the camss device tree node.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 26afaa4f98fe..d7bd20412f06 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -3289,6 +3289,7 @@ camss: camss@ac6a000 {
 			power-domains = <&camcc IFE_0_GDSC>,
 					<&camcc IFE_1_GDSC>,
 					<&camcc TITAN_TOP_GDSC>;
+			power-domain-names = "vfe0", "vfe1", "titan_top";
 
 			clocks = <&gcc GCC_CAMERA_AHB_CLK>,
 				 <&gcc GCC_CAMERA_HF_AXI_CLK>,
-- 
2.33.0

Re: [PATCH 0/2] media: dt-bindings: media: sm8250-camss: Add power-domain-names property

[Bryan O'Donoghue]

On 18/05/2022 13:11, Vladimir Zapolskiy wrote:
> QCOM SM8250 camera subsystem depends on three power domains, at the moment
> all of them are not differentiated one from another, however the power
> domains compose a hierarchical structure with vfe0 and vfe1 as subdomains
> of titan_top, also managing vfe0 and vfe1 separately allows to get more
> fine-grained power control in runtime.
> 
> The change relates to my review comment for v2 of CAMSS on SM8250 submission:
> 
>     https://lore.kernel.org/all/13ad033e-cd5d-3a8c-b036-50a3ac4245c0@linaro.org/
> 
> Apparently it becomes important to manage CAMSS power domains much better for
> newer platforms, this referes to platforms with Titan GDSC, for instance CAMSS
> on SM8450 has 6 power domains, and dealing with them in bulk is not an option.
> 
> There was a note in commit 2f6f8af67203 ("media: camss: Refactor VFE power
> domain toggling") about problems with power VFE domains on/off, but perhaps
> it's related to the fact that Titan GDSC is a special power domain and VFE
> are subdomains, the latter shall not be enabled earlier than the Titan, but
> the driver did not construct a proper hierarchy and leaves a room for races.
> 
> The change should have no implications on any SM8250 CAMSS users, since
> none of the supported in upstream boards enables the camss device tree node.
> The correspondent changes in the driver will follow this dt specific series.
> 
> Most likely a similar change is required for SDM845 platform, but it would
> need additional investigation and testing.
> 
> Vladimir Zapolskiy (2):
>    media: dt-bindings: media: sm8250-camss: Add power-domain-names property
>    arm64: dts: qcom: sm8250: camss: Add power-domain-names property
> 
>   .../devicetree/bindings/media/qcom,sm8250-camss.yaml       | 7 +++++++
>   arch/arm64/boot/dts/qcom/sm8250.dtsi                       | 1 +
>   2 files changed, 8 insertions(+)
> 

This doesn't break anything for me on sm8250

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

for both

Re: [PATCH 0/2] media: dt-bindings: media: sm8250-camss: Add power-domain-names property

[Vladimir Zapolskiy]

On 5/18/22 15:11, Vladimir Zapolskiy wrote:
> QCOM SM8250 camera subsystem depends on three power domains, at the moment
> all of them are not differentiated one from another, however the power
> domains compose a hierarchical structure with vfe0 and vfe1 as subdomains
> of titan_top, also managing vfe0 and vfe1 separately allows to get more
> fine-grained power control in runtime.
> 
> The change relates to my review comment for v2 of CAMSS on SM8250 submission:
> 
>     https://lore.kernel.org/all/13ad033e-cd5d-3a8c-b036-50a3ac4245c0@linaro.org/
> 
> Apparently it becomes important to manage CAMSS power domains much better for
> newer platforms, this referes to platforms with Titan GDSC, for instance CAMSS
> on SM8450 has 6 power domains, and dealing with them in bulk is not an option.
> 
> There was a note in commit 2f6f8af67203 ("media: camss: Refactor VFE power
> domain toggling") about problems with power VFE domains on/off, but perhaps
> it's related to the fact that Titan GDSC is a special power domain and VFE
> are subdomains, the latter shall not be enabled earlier than the Titan, but
> the driver did not construct a proper hierarchy and leaves a room for races.
> 
> The change should have no implications on any SM8250 CAMSS users, since
> none of the supported in upstream boards enables the camss device tree node.
> The correspondent changes in the driver will follow this dt specific series.
> 
> Most likely a similar change is required for SDM845 platform, but it would
> need additional investigation and testing.
> 
> Vladimir Zapolskiy (2):
>    media: dt-bindings: media: sm8250-camss: Add power-domain-names property
>    arm64: dts: qcom: sm8250: camss: Add power-domain-names property
> 
>   .../devicetree/bindings/media/qcom,sm8250-camss.yaml       | 7 +++++++
>   arch/arm64/boot/dts/qcom/sm8250.dtsi                       | 1 +
>   2 files changed, 8 insertions(+)
> 

These changes will be unneeded, if it is reliable to state that the order
of 'power-domains' array values is fixed.

 From Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml

   power-domains:
     items:
       - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
       - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
       - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.

Apparently it's insufficient to ensure the fixed order of the power domains
by running a check against the schema, and likely it can not be improved,
but please correct me here, if I'm wrong.

That's said, what is the preferred way here? Leave everything as is and rely
on the order of item descriptions, or add a new power-domain-names property?

--
Best wishes,
Vladimir

Re: [PATCH 0/2] media: dt-bindings: media: sm8250-camss: Add power-domain-names property

[Rob Herring]

On Fri, May 20, 2022 at 12:53:02AM +0300, Vladimir Zapolskiy wrote:
> On 5/18/22 15:11, Vladimir Zapolskiy wrote:
> > QCOM SM8250 camera subsystem depends on three power domains, at the moment
> > all of them are not differentiated one from another, however the power
> > domains compose a hierarchical structure with vfe0 and vfe1 as subdomains
> > of titan_top, also managing vfe0 and vfe1 separately allows to get more
> > fine-grained power control in runtime.
> > 
> > The change relates to my review comment for v2 of CAMSS on SM8250 submission:
> > 
> >     https://lore.kernel.org/all/13ad033e-cd5d-3a8c-b036-50a3ac4245c0@linaro.org/
> > 
> > Apparently it becomes important to manage CAMSS power domains much better for
> > newer platforms, this referes to platforms with Titan GDSC, for instance CAMSS
> > on SM8450 has 6 power domains, and dealing with them in bulk is not an option.
> > 
> > There was a note in commit 2f6f8af67203 ("media: camss: Refactor VFE power
> > domain toggling") about problems with power VFE domains on/off, but perhaps
> > it's related to the fact that Titan GDSC is a special power domain and VFE
> > are subdomains, the latter shall not be enabled earlier than the Titan, but
> > the driver did not construct a proper hierarchy and leaves a room for races.
> > 
> > The change should have no implications on any SM8250 CAMSS users, since
> > none of the supported in upstream boards enables the camss device tree node.
> > The correspondent changes in the driver will follow this dt specific series.
> > 
> > Most likely a similar change is required for SDM845 platform, but it would
> > need additional investigation and testing.
> > 
> > Vladimir Zapolskiy (2):
> >    media: dt-bindings: media: sm8250-camss: Add power-domain-names property
> >    arm64: dts: qcom: sm8250: camss: Add power-domain-names property
> > 
> >   .../devicetree/bindings/media/qcom,sm8250-camss.yaml       | 7 +++++++
> >   arch/arm64/boot/dts/qcom/sm8250.dtsi                       | 1 +
> >   2 files changed, 8 insertions(+)
> > 
> 
> These changes will be unneeded, if it is reliable to state that the order
> of 'power-domains' array values is fixed.
> 
> From Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
> 
>   power-domains:
>     items:
>       - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
>       - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
>       - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
> 
> Apparently it's insufficient to ensure the fixed order of the power domains
> by running a check against the schema, and likely it can not be improved,
> but please correct me here, if I'm wrong.

Right, the schemas can't check that the order is correct.

> That's said, what is the preferred way here? Leave everything as is and rely
> on the order of item descriptions, or add a new power-domain-names property?

Well, you can't start requiring power-domain-names without breaking the 
ABI and it has to be required to allow any order. Even then, defined 
order is preferred unless there's too many variations and we're stuck 
with no defined order.

Rob

Re: [PATCH 0/2] media: dt-bindings: media: sm8250-camss: Add power-domain-names property

[Hans Verkuil]

Hi Vladimir,

On 18/05/2022 14:11, Vladimir Zapolskiy wrote:
> QCOM SM8250 camera subsystem depends on three power domains, at the moment
> all of them are not differentiated one from another, however the power
> domains compose a hierarchical structure with vfe0 and vfe1 as subdomains
> of titan_top, also managing vfe0 and vfe1 separately allows to get more
> fine-grained power control in runtime.
> 
> The change relates to my review comment for v2 of CAMSS on SM8250 submission:
> 
>    https://lore.kernel.org/all/13ad033e-cd5d-3a8c-b036-50a3ac4245c0@linaro.org/
> 
> Apparently it becomes important to manage CAMSS power domains much better for
> newer platforms, this referes to platforms with Titan GDSC, for instance CAMSS
> on SM8450 has 6 power domains, and dealing with them in bulk is not an option.
> 
> There was a note in commit 2f6f8af67203 ("media: camss: Refactor VFE power
> domain toggling") about problems with power VFE domains on/off, but perhaps
> it's related to the fact that Titan GDSC is a special power domain and VFE
> are subdomains, the latter shall not be enabled earlier than the Titan, but
> the driver did not construct a proper hierarchy and leaves a room for races.
> 
> The change should have no implications on any SM8250 CAMSS users, since
> none of the supported in upstream boards enables the camss device tree node.
> The correspondent changes in the driver will follow this dt specific series.
> 
> Most likely a similar change is required for SDM845 platform, but it would
> need additional investigation and testing.
> 
> Vladimir Zapolskiy (2):
>   media: dt-bindings: media: sm8250-camss: Add power-domain-names property
>   arm64: dts: qcom: sm8250: camss: Add power-domain-names property
> 
>  .../devicetree/bindings/media/qcom,sm8250-camss.yaml       | 7 +++++++
>  arch/arm64/boot/dts/qcom/sm8250.dtsi                       | 1 +
>  2 files changed, 8 insertions(+)
> 

I am marking this series as 'Obsoleted' in patchwork. If you believe this
(or a variant of it) is still needed, then please repost.

Regards,

	Hans