| Message ID | 20240927103212.4154273-1-vladimir.zapolskiy@linaro.org |
|---|---|
| Headers | show |
| Series | arm: qcom: sm8550: Change camcc power domain from MMCX to MXC | expand |
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
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>;
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
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