Re: [PATCH] clk: qcom: camcc-sm8250: Fix halt on boot by reducing driver's init level

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 18/05/2022 11:35, Vladimir Zapolskiy wrote:
> Access to I/O of SM8250 camera clock controller IP depends on enabled
> GCC_CAMERA_AHB_CLK clock supplied by global clock controller, the latter
> one is inited on subsys level, so, to satisfy the dependency, it would
> make sense to deprive the init level of camcc-sm8250 driver.
> 
> If both drivers are compiled as built-in, there is a change that a board
> won't boot up due to a race, which happens on the same init level.
> 
> Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/clk/qcom/camcc-sm8250.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/qcom/camcc-sm8250.c b/drivers/clk/qcom/camcc-sm8250.c
> index 439eaafdcc86..ae4e9774f36e 100644
> --- a/drivers/clk/qcom/camcc-sm8250.c
> +++ b/drivers/clk/qcom/camcc-sm8250.c
> @@ -2440,17 +2440,7 @@ static struct platform_driver cam_cc_sm8250_driver = {
>   	},
>   };
>   
> -static int __init cam_cc_sm8250_init(void)
> -{
> -	return platform_driver_register(&cam_cc_sm8250_driver);
> -}
> -subsys_initcall(cam_cc_sm8250_init);
> -
> -static void __exit cam_cc_sm8250_exit(void)
> -{
> -	platform_driver_unregister(&cam_cc_sm8250_driver);
> -}
> -module_exit(cam_cc_sm8250_exit);
> +module_platform_driver(cam_cc_sm8250_driver);
>   
>   MODULE_DESCRIPTION("QTI CAMCC SM8250 Driver");
>   MODULE_LICENSE("GPL v2");

So I tried this

-                       clocks = <&gcc GCC_CAMERA_AHB_CLK>,
-                                <&rpmhcc RPMH_CXO_CLK>,
+                       clocks = <&rpmhcc RPMH_CXO_CLK>,
                                  <&rpmhcc RPMH_CXO_CLK_A>,
                                  <&sleep_clk>;
-                       clock-names = "iface", "bi_tcxo", "bi_tcxo_ao", 
"sleep_clk";
+                       clock-names = "bi_tcxo", "bi_tcxo_ao", "sleep_clk";

and the system wouldn't boot @ * 736ee37e2e8e - (tag: next-20220518, 
linux-next/master) Add linux-next specific files for 20220518 (2 hours ago)

If we do a grep

grep subsys_init drivers/clk/qcom/camcc-*
drivers/clk/qcom/camcc-sc7180.c:subsys_initcall(cam_cc_sc7180_init);
drivers/clk/qcom/camcc-sc7280.c:subsys_initcall(cam_cc_sc7280_init);
drivers/clk/qcom/camcc-sdm845.c:subsys_initcall(cam_cc_sdm845_init);
drivers/clk/qcom/camcc-sm8250.c:subsys_initcall(cam_cc_sm8250_init);

and

arch/arm64/boot/dts/qcom/sc7180.dtsi:			       <&gcc GCC_CAMERA_AHB_CLK>,
arch/arm64/boot/dts/qcom/sm8250.dtsi:			clocks = <&gcc GCC_CAMERA_AHB_CLK>,

I think the sc7180 has this same dependency loop. Probably needs the 
same fix.

Also not sure why sdm845 camcc doesn't declare a depends on 
GCC_CAMERA_AHB_CLK - should it ?

Recommend applying this same fix to sc718x

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

---
bod