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

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

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