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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

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