[PATCH 0/3] arm: qcom: sm8550: Change camcc power domain from MMCX to MXC

[PATCH 0/3] arm: qcom: sm8550: Change camcc power domain from MMCX to MXC

[Vladimir Zapolskiy]

The problem is trivial to reproduce and the fix is trivial to verify,
it's sufficient to enable SM8550 camera clock controller and a CCI
controller, for instance on SM8550-QRD CCI0 or CCI1 can be enabled:

    &cci0 {
	status = "okay";
    };

I made a special effort to check that the power domain in SM8550 camcc
is sufficient to be replaced, and Titan and other provided GDSCs can
be turned on/off, if the clock controller is disconneced from MMCX and
MMCX is off according to /sys/kernel/debug/pm_genpd/pm_genpd_summary.

Vladimir Zapolskiy (3):
  arm64: dts: qcom: sm8550: Change camcc power domain from MMCX to MXC
  dt-bindings: clock: qcom,sm8450-camcc: do not restrict power domain to MMCX
  dt-bindings: clock: qcom,sm8450-camcc: generalize title and description

 .../devicetree/bindings/clock/qcom,sm8450-camcc.yaml      | 8 ++++----
 arch/arm64/boot/dts/qcom/sm8550.dtsi                      | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)


base-commit: 11a299a7933e03c83818b431e6a1c53ad387423d
-- 
2.45.2

[PATCH 1/3] arm64: dts: qcom: sm8550: Change camcc power domain from MMCX to MXC

[Vladimir Zapolskiy]

Any attempt to enable titan_top_gdsc on SM8550-QRD fails and produces
an error message that the gdsc is stuck at 'off' state, this can be
easily verified just by setting cci0 status on:

    cam_cc_titan_top_gdsc status stuck at 'off'
    WARNING: CPU: 6 PID: 89 at drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x154/0x168

However if MMCX power domain is replaced by MXC one, it allows to turn
titan_top_gdsc on successfully, even if MMCX is remained off according
to /sys/kernel/debug/pm_genpd/pm_genpd_summary report.

Fixes: e271b59e39a6 ("arm64: dts: qcom: sm8550: Add camera clock controller")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index 9dc0ee3eb98f..5c07d1b35615 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -2846,7 +2846,7 @@ camcc: clock-controller@ade0000 {
 				 <&bi_tcxo_div2>,
 				 <&bi_tcxo_ao_div2>,
 				 <&sleep_clk>;
-			power-domains = <&rpmhpd SM8550_MMCX>;
+			power-domains = <&rpmhpd SM8550_MXC>;
 			required-opps = <&rpmhpd_opp_low_svs>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
-- 
2.45.2

Re: [PATCH 1/3] arm64: dts: qcom: sm8550: Change camcc power domain from MMCX to MXC

[Taniya Das]

On 9/27/2024 4:02 PM, Vladimir Zapolskiy wrote:
> Any attempt to enable titan_top_gdsc on SM8550-QRD fails and produces
> an error message that the gdsc is stuck at 'off' state, this can be
> easily verified just by setting cci0 status on:
> 
>      cam_cc_titan_top_gdsc status stuck at 'off'
>      WARNING: CPU: 6 PID: 89 at drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x154/0x168
> 
> However if MMCX power domain is replaced by MXC one, it allows to turn
> titan_top_gdsc on successfully, even if MMCX is remained off according

MMCX is absolutely required for Camera Clock controller as it is the 
main power domain. The access will not go through if this domain is not ON.
While I agree that MXC is also required to be enabled for GDSC powering 
up, but the below is not the correct way to handle the condition.
In your case the MMCX could be left ON in hardware and that could be the 
reason for the access to go through.

I am currently working on the necessary changes to address these 
conditions where a clock controller (GDSC) has multiple power domain 
dependencies.

> to /sys/kernel/debug/pm_genpd/pm_genpd_summary report.
> 
> Fixes: e271b59e39a6 ("arm64: dts: qcom: sm8550: Add camera clock controller")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index 9dc0ee3eb98f..5c07d1b35615 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> @@ -2846,7 +2846,7 @@ camcc: clock-controller@ade0000 {
>   				 <&bi_tcxo_div2>,
>   				 <&bi_tcxo_ao_div2>,
>   				 <&sleep_clk>;
> -			power-domains = <&rpmhpd SM8550_MMCX>;
> +			power-domains = <&rpmhpd SM8550_MXC>;


>   			required-opps = <&rpmhpd_opp_low_svs>;
>   			#clock-cells = <1>;
>   			#reset-cells = <1>;

-- 
Thanks & Regards,
Taniya Das.

Re: [PATCH 1/3] arm64: dts: qcom: sm8550: Change camcc power domain from MMCX to MXC

[Vladimir Zapolskiy]

Hello Taniya.

On 9/30/24 08:57, Taniya Das wrote:
> 
> 
> On 9/27/2024 4:02 PM, Vladimir Zapolskiy wrote:
>> Any attempt to enable titan_top_gdsc on SM8550-QRD fails and produces
>> an error message that the gdsc is stuck at 'off' state, this can be
>> easily verified just by setting cci0 status on:
>>
>>       cam_cc_titan_top_gdsc status stuck at 'off'
>>       WARNING: CPU: 6 PID: 89 at drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x154/0x168
>>
>> However if MMCX power domain is replaced by MXC one, it allows to turn
>> titan_top_gdsc on successfully, even if MMCX is remained off according
> 
> MMCX is absolutely required for Camera Clock controller as it is the
> main power domain. The access will not go through if this domain is not ON.
> While I agree that MXC is also required to be enabled for GDSC powering
> up, but the below is not the correct way to handle the condition.
> In your case the MMCX could be left ON in hardware and that could be the
> reason for the access to go through.

Sure, it's the most probable case.

> I am currently working on the necessary changes to address these
> conditions where a clock controller (GDSC) has multiple power domain
> dependencies.
> 
>> to /sys/kernel/debug/pm_genpd/pm_genpd_summary report.
>>

If you find it possible, I kindly ask you to check and probably correct
the reported MMCX status as 'off', when it's 'on' actually.

Nevertheless the work on GDSCs is kind of dependent on DT bindings
description, I will send v2 shortly with two power domains as the property
value and a new property power-domain-names with "mmcx", "mxc" values,
unless you have objections.

>> Fixes: e271b59e39a6 ("arm64: dts: qcom: sm8550: Add camera clock controller")
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>    arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> index 9dc0ee3eb98f..5c07d1b35615 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> @@ -2846,7 +2846,7 @@ camcc: clock-controller@ade0000 {
>>    				 <&bi_tcxo_div2>,
>>    				 <&bi_tcxo_ao_div2>,
>>    				 <&sleep_clk>;
>> -			power-domains = <&rpmhpd SM8550_MMCX>;
>> +			power-domains = <&rpmhpd SM8550_MXC>;
> 
> 
>>    			required-opps = <&rpmhpd_opp_low_svs>;
>>    			#clock-cells = <1>;
>>    			#reset-cells = <1>;
> 

--
Best wishes,
Vladimir

[PATCH 2/3] dt-bindings: clock: qcom,sm8450-camcc: do not restrict power domain to MMCX

[Vladimir Zapolskiy]

It was unveiled that Qualcomm SM8550 Camera clock controller is attached
to MXC power domain, therefore it has to be reflected and generalized in
the device tree bindings description.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 .../devicetree/bindings/clock/qcom,sm8450-camcc.yaml          | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
index 26afbbe65511..9ef967523987 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
@@ -40,12 +40,12 @@ properties:
   power-domains:
     maxItems: 1
     description:
-      A phandle and PM domain specifier for the MMCX power domain.
+      A phandle and PM domain specifier for the supplying power domain.
 
   required-opps:
     maxItems: 1
     description:
-      A phandle to an OPP node describing required MMCX performance point.
+      A phandle to an OPP node describing the power domain's performance point.
 
   reg:
     maxItems: 1
-- 
2.45.2

Re: [PATCH 2/3] dt-bindings: clock: qcom,sm8450-camcc: do not restrict power domain to MMCX

[Rob Herring]

On Fri, Sep 27, 2024 at 01:32:11PM +0300, Vladimir Zapolskiy wrote:
> It was unveiled that Qualcomm SM8550 Camera clock controller is attached
> to MXC power domain, therefore it has to be reflected and generalized in
> the device tree bindings description.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  .../devicetree/bindings/clock/qcom,sm8450-camcc.yaml          | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
> index 26afbbe65511..9ef967523987 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
> @@ -40,12 +40,12 @@ properties:
>    power-domains:
>      maxItems: 1
>      description:
> -      A phandle and PM domain specifier for the MMCX power domain.
> +      A phandle and PM domain specifier for the supplying power domain.

Just drop as it just describes every 'power-domains' property.

>  
>    required-opps:
>      maxItems: 1
>      description:
> -      A phandle to an OPP node describing required MMCX performance point.
> +      A phandle to an OPP node describing the power domain's performance point.

Same

[PATCH 3/3] dt-bindings: clock: qcom,sm8450-camcc: generalize title and description

[Vladimir Zapolskiy]

Currently the qcom,sm8450-camcc.yaml IP documentation serves as
a description of camera clock controllers of Qualcomm SC8280xp, SM8450,
SM8550, SM8650 and X1E80100 SoCs, and there is a room for more
controllers to be supported by this device tree bindings documentation
in future.

Generalize the title and description in the documentation file similarly
to a number of other Qualcomm clock controller documentation files.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 .../devicetree/bindings/clock/qcom,sm8450-camcc.yaml          | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
index 9ef967523987..11febb5937f3 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/clock/qcom,sm8450-camcc.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Qualcomm Camera Clock & Reset Controller on SM8450
+title: Qualcomm Camera Clock & Reset Controller
 
 maintainers:
   - Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
@@ -12,7 +12,7 @@ maintainers:
 
 description: |
   Qualcomm camera clock control module provides the clocks, resets and power
-  domains on SM8450.
+  domains on Qualcomm SoCs.
 
   See also:
     include/dt-bindings/clock/qcom,sc8280xp-camcc.h
-- 
2.45.2

Re: [PATCH 3/3] dt-bindings: clock: qcom,sm8450-camcc: generalize title and description

[Rob Herring (Arm)]

On Fri, 27 Sep 2024 13:32:12 +0300, Vladimir Zapolskiy wrote:
> Currently the qcom,sm8450-camcc.yaml IP documentation serves as
> a description of camera clock controllers of Qualcomm SC8280xp, SM8450,
> SM8550, SM8650 and X1E80100 SoCs, and there is a room for more
> controllers to be supported by this device tree bindings documentation
> in future.
> 
> Generalize the title and description in the documentation file similarly
> to a number of other Qualcomm clock controller documentation files.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  .../devicetree/bindings/clock/qcom,sm8450-camcc.yaml          | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

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

Re: [PATCH 0/3] arm: qcom: sm8550: Change camcc power domain from MMCX to MXC

[Vladimir Zapolskiy]

On 9/27/24 13:32, Vladimir Zapolskiy wrote:
> The problem is trivial to reproduce and the fix is trivial to verify,
> it's sufficient to enable SM8550 camera clock controller and a CCI
> controller, for instance on SM8550-QRD CCI0 or CCI1 can be enabled:
> 
>      &cci0 {
> 	status = "okay";
>      };
> 
> I made a special effort to check that the power domain in SM8550 camcc
> is sufficient to be replaced, and Titan and other provided GDSCs can
> be turned on/off, if the clock controller is disconneced from MMCX and
> MMCX is off according to /sys/kernel/debug/pm_genpd/pm_genpd_summary.
> 

Additionally it makes sense to mention that the fix alone has been already
sent for a review as an RFT change:

* https://lore.kernel.org/all/20240612214812.1149019-1-vladimir.zapolskiy@linaro.org/

--
Best wishes,
Vladimir