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 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