Re: [PATCH v4 0/7] Add SDM670 camera subsystem

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

Hi Richard.

On 9/28/24 01:23, Richard Acayan wrote:
> 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).

Good, let the assigned clock frequency be dropped from the dtsi file then.

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

I grasp it from the problem description, thank you for the explanation.

For sake of simplicity please make camcc-sdm845 as a built-in driver while
testing, it will allow to progress with the platform CAMSS support.

The issue with the observed ETIMEDOUT is generic and it's kind of unrelated
to the CCI/CAMSS support on SDM670.

--
Best wishes,
Vladimir