| Message ID | 20220518103554.949511-1-vladimir.zapolskiy@linaro.org |
|---|---|
| State | Accepted |
| Commit | c4f40351901a10cd662ac2c081396d8fb04f584d |
| Headers | show |
| Series | clk: qcom: camcc-sm8250: Fix halt on boot by reducing driver's init level | expand |
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
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"); >>
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
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"); >>>
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,
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");
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(-)