On Fri, Sep 06, 2024 at 04:00:32PM +0300, Vladimir Zapolskiy wrote: > Hi Bryan, Richard, > > On 9/6/24 15:19, Bryan O'Donoghue wrote: > > On 06/09/2024 03:36, Richard Acayan wrote: > > > On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote: > > > > Hi Richard, > > > > > > > > On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote: > > > > > This adds support for the camera subsystem on the Snapdragon 670. > > > > > > > > > > As of next-20240902, camss seems to be a bit broken, but the same series > > > > > works on stable (although it is much less reliable now that the CCI clock > > > > > frequency is not being assigned). > > > > > > > > I am not understanding this bit: is this series making it better > > > > or not? Can you please clarify what is broken, what is less > > > > reliable and what works? > > > > > > When applying this camss series and some camera sensor patches on > > > linux-next, the Pixel 3a seems to hang when camera capture starts. > > > > > > When applying the same patches on stable, the camera does not cause the > > > Pixel 3a to hang. > > > > Right so -next isn't stable that's not exactly a revelation. > > > > > > > When these device tree properties from the previous series were removed: > > > > > > assigned-clocks = <&camcc CAM_CC_CCI_CLK>; > > > assigned-clock-rates = <37500000>; > > > > > > the CCI would sometimes fail to probe with the error: > > > > Right, we don't have clk_set_rate in the cci driver. > > > > Maybe just leave the assigned clock for this submission and we can do a > > sweep of fixes to CCI at a later stage including setting the clock > > instead of having it be assigned. > > first of all it would be nice to confirm that the setting of a particular > clock frequency is actually needed. > > Fortunately it's pretty trivial to check it in runtime with a temporary > modification in the board dts file, namely disable CAMSS in board dts file, > but keep CCI enabled, then simply scan the bus with a regular "i2cdetect" > tool in runtime. > > If i2cdetect on the CCI bus works only for 37.5MHz clock frequency, then it > is needed, otherwise (and this is my expectation) it is not needed neither > in the dtsi files nor in the driver. > > > > > > > [ 51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency > > > [ 51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110 > > > > > > On further testing, the rate can be set to 19.2 MHz, and there would be > > > no failure (or rather, it wouldn't happen often enough for me to witness > > > it). > > > > That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part. > > > > I read it as the setting of 37.5MHz clock frequency is not needed, please > correct me. It is not. My test setup just needs specific EPROBE_DEFER behaviour (my setup being postmarketOS with a full-disk encryption password prompt and camcc-sdm845 loaded after mounting the root filesystem). In drivers/base/platform.c, the platform_probe() function calls of_clk_set_defaults() then dev_pm_domain_attach() prior to probing the driver: static int platform_probe(struct device *_dev) { ... ret = of_clk_set_defaults(_dev->of_node, false); if (ret < 0) return ret; ret = dev_pm_domain_attach(_dev, true); if (ret) goto out; if (drv->probe) { ret = drv->probe(dev); if (ret) dev_pm_domain_detach(_dev, true); } ... } When handling the assigned-clock-rates property, of_clk_get_hw_from_clkspec() eventually returns ERR_PTR(-EPROBE_DEFER), being propagated all the way. When handling the power-domains property (if not avoided by deferring with the assigned clock), __genpd_dev_pm_attach() returns a value returned by driver_deferred_probe_check_state(), which is immediately -ETIMEDOUT.