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

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

[Vladimir Zapolskiy]

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");
-- 
2.33.0

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

[Bryan O'Donoghue]

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

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

[Jonathan Marek]

On 5/18/22 8:48 AM, Bryan O'Donoghue wrote:
> 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)
> 

GCC_CAMERA_AHB_CLK is defined but it isn't actually implemented by the 
upstream gcc driver, and the camcc driver doesn't do anything with it 
either (I didn't include it in the camcc driver because the gcc driver 
didn't implement it, but I have a patch to do things like downstream, 
dispcc/gpucc/videocc drivers all have this problem too). Does having it 
in the dts like this cause the gcc driver to probe first somehow, even 
though the clock isn't used by the camcc driver?

(The sc7180 camcc driver does do something with the "iface" clock, but 
the sc7180 gcc driver also doesn't implement GCC_CAMERA_AHB_CLK either.. 
I guess you get a dummy clock for the unimplemented clocks?)

> 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

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

[Bryan O'Donoghue]

On 18/05/2022 20:46, Jonathan Marek wrote:
> 
> GCC_CAMERA_AHB_CLK is defined but it isn't actually implemented by the 
> upstream gcc driver

*facepalm*

drivers/clk/qcom/gcc-sc8180x.c:	 * GCC_VIDEO_AHB_CLK, 
GCC_CAMERA_AHB_CLK, GCC_DISP_AHB_CLK,
drivers/clk/qcom/gcc-sm8250.c:	 * GCC_VIDEO_AHB_CLK, GCC_CAMERA_AHB_CLK, 
GCC_DISP_AHB_CLK,
drivers/clk/qcom/gcc-sm6350.c:	[GCC_CAMERA_AHB_CLK] = 
&gcc_camera_ahb_clk.clkr,

you're right - and we have this too don't we

         /*
          * Keep the clocks always-ON
          * GCC_VIDEO_AHB_CLK, GCC_CAMERA_AHB_CLK, GCC_DISP_AHB_CLK,
          * GCC_CPUSS_DVM_BUS_CLK, GCC_GPU_CFG_AHB_CLK,
          * GCC_SYS_NOC_CPUSS_AHB_CLK
          */
         regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b008, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x4818c, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x52000, BIT(0), BIT(0));

drivers/clk/qcom/gcc-sm8250.c - so defining or not defining 
GCC_CAMERA_AHB_CLK should be a nop.

And yep we have this on sc7280

         /*
          * Keep the clocks always-ON
          * GCC_CPUSS_GNOC_CLK, GCC_VIDEO_AHB_CLK, GCC_CAMERA_AHB_CLK,
          * GCC_DISP_AHB_CLK, GCC_GPU_CFG_AHB_CLK
          */
         regmap_update_bits(regmap, 0x48004, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b008, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b02c, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b028, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x0b030, BIT(0), BIT(0));
         regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));

at least the bug is consistent :)

>, and the camcc driver doesn't do anything with it 

yep - it has an interconnect with a hopeful name "cam_ahb" we don't have 
an obvious clock on the AP side for that, perhaps the rpmh is doing 
something with the AHB clock because of an interconnect call.

We will have to do a deep dive to find out.

> either (I didn't include it in the camcc driver because the gcc driver 
> didn't implement it, but I have a patch to do things like downstream, 
> dispcc/gpucc/videocc drivers all have this problem too). Does having it 
> in the dts like this cause the gcc driver to probe first somehow, even 
> though the clock isn't used by the camcc driver?
> 
> (The sc7180 camcc driver does do something with the "iface" clock, but 
> the sc7180 gcc driver also doesn't implement GCC_CAMERA_AHB_CLK either.. 
> I guess you get a dummy clock for the unimplemented clocks?)

Yep, I missed that.

Meh. Ok we know we have a bug, we know its replicated on sm8250 and 
sc7280 and we know it doesn't make sense.

My guess is rpmh is switching on the clock. In any case we clearly 
haven't captured the clock dependency right upstream.

---
bod

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

[Konrad Dybcio]

On 18/05/2022 12: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.

Hi,


I believe this is due to the fact that this clock is falsely advertised 
by the header and Linux does not know anything about it, because it is 
handled by a magic write [1] (as I once said in a similar case, this was 
going bite eventually..) instead and the index corresponding to the 
define symbol is not initialized, hence it points to NULL. Adding the 
clock properly in GCC would let the OF clock stuff handle it gracefully. 
If that is the case, your patch disabling the clock controller block 
(which I'm against unless there's abosolute need, as having the hw block 
initialized properly should be possible regardless of the board, as it's 
a generic SoC components) should not be necessary.

That said, I can not test my theory right now.


[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/qcom/gcc-sm8250.c?h=next-20220518#n3647

Konrad

>
> 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>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@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");
>

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

[Vladimir Zapolskiy]

Hi Konrad,

On 5/18/22 19:47, Konrad Dybcio wrote:
> 
> On 18/05/2022 12: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.
> 
> Hi,
> 
> I believe this is due to the fact that this clock is falsely advertised
> by the header and Linux does not know anything about it, because it is
> handled by a magic write [1] (as I once said in a similar case, this was
> going bite eventually..) instead and the index corresponding to the
> define symbol is not initialized, hence it points to NULL. Adding the

your observation is correct in my opinion, however it does not change the
identified root cause of the problem, and my rationale remains the same,
the camera clock controller should be initialized after the GCC, thus
this change, and currently the critical fix, remains valid.

> clock properly in GCC would let the OF clock stuff handle it gracefully.

If/when the clock is properly added in the GCC, then it will open an
option to clk_prepare_enable() it in the CAMCC driver, so at least it's
a point to keep it described in a dts as it's done right from the beginning,
especially because the platform dtsi describes the hardware properly.
To add a real CCF clock would be my preference, but, as I've said above,
even if it happens, it does not belittle the presented change.

> If that is the case, your patch disabling the clock controller block
> (which I'm against unless there's abosolute need, as having the hw block
> initialized properly should be possible regardless of the board, as it's
> a generic SoC components) should not be necessary.

Here I do oppose, I believe board dts files should explicitly describe
enabled IPs in accordance to actual board peripherals. For instance it's
unclear why CAMCC or e.g. CAMSS should be enabled by default on a board
without camera sensors at all. I understand that there is an option to
explicitly disable some particular device tree nodes in board files, but
it is against common practicalities.

Also above I do neglect the fact that the GCC clock is always enabled,
irrelatively of its actual usage by probably disabled CAMCC.

--
Best wishes,
Vladimir

> That said, I can not test my theory right now.
> 
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/qcom/gcc-sm8250.c?h=next-20220518#n3647
> 
> Konrad
> 
>>
>> 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>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@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");
>>

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

[Konrad Dybcio]

On 18/05/2022 19:24, Vladimir Zapolskiy wrote:
> Hi Konrad,
>
> On 5/18/22 19:47, Konrad Dybcio wrote:
>>
>> On 18/05/2022 12: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.
>>
>> Hi,
>>
>> I believe this is due to the fact that this clock is falsely advertised
>> by the header and Linux does not know anything about it, because it is
>> handled by a magic write [1] (as I once said in a similar case, this was
>> going bite eventually..) instead and the index corresponding to the
>> define symbol is not initialized, hence it points to NULL. Adding the
>
> your observation is correct in my opinion, however it does not change the
> identified root cause of the problem, and my rationale remains the same,
> the camera clock controller should be initialized after the GCC, thus
> this change, and currently the critical fix, remains valid.
>
>> clock properly in GCC would let the OF clock stuff handle it gracefully.
>
> If/when the clock is properly added in the GCC, then it will open an
> option to clk_prepare_enable() it in the CAMCC driver, so at least it's
> a point to keep it described in a dts as it's done right from the 
> beginning,
> especially because the platform dtsi describes the hardware properly.
> To add a real CCF clock would be my preference, but, as I've said above,
> even if it happens, it does not belittle the presented change.
>
>> If that is the case, your patch disabling the clock controller block
>> (which I'm against unless there's abosolute need, as having the hw block
>> initialized properly should be possible regardless of the board, as it's
>> a generic SoC components) should not be necessary.
>
> Here I do oppose, I believe board dts files should explicitly describe
> enabled IPs in accordance to actual board peripherals. For instance it's
> unclear why CAMCC or e.g. CAMSS should be enabled by default on a board
> without camera sensors at all. I understand that there is an option to
> explicitly disable some particular device tree nodes in board files, but
> it is against common practicalities.
>
> Also above I do neglect the fact that the GCC clock is always enabled,
> irrelatively of its actual usage by probably disabled CAMCC.

I think there's an opportunity to save some power here by keeping the 
camss ahb disabled by default.. That would include removing the magic 
write from gcc probe, adding the gcc ahb as a proper clock and adding a 
camss consumer to it (possibly with both fw_name and name fields in 
parent_data to keep backwards compat, even though i don't think any 
board uses camera hw upstream, so we may break it here with a good 
justification). Then we could keep camcc enabled by default on all 
boards, and if there are no users, the clocks would just be parked to xo 
or disabled entirely, instead of inheriting an unknown configuration.


Konrad

>
> -- 
> Best wishes,
> Vladimir
>
>> That said, I can not test my theory right now.
>>
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/qcom/gcc-sm8250.c?h=next-20220518#n3647 
>>
>>
>> Konrad
>>
>>>
>>> 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>
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@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");
>>>

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

[Bjorn Andersson]

On Wed, 18 May 2022 13:35:54 +0300, 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.
> 
> [...]

Applied, thanks!

[1/1] clk: qcom: camcc-sm8250: Fix halt on boot by reducing driver's init level
      commit: c4f40351901a10cd662ac2c081396d8fb04f584d

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>