Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 14/11/2025 17:06, Vladimir Zapolskiy wrote:
> On 11/14/25 18:09, Bryan O'Donoghue wrote:
>> On 14/11/2025 13:06, Luca Weiss wrote:
>>> Hi Vladimir,
>>>
>>> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>>>> Hi Luca.
>>>>
>>>> On 11/14/25 13:15, Luca Weiss wrote:
>>>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>>>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>     .../bindings/media/qcom,sm6350-camss.yaml          | 349 ++++++ 
>>>>> +++++++++++++++
>>>>>     1 file changed, 349 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350- 
>>>>> camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350- 
>>>>> camss.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..d812b5b50c05
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>>>> @@ -0,0 +1,349 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>>>> +
>>>>> +maintainers:
>>>>> +  - Luca Weiss <luca.weiss@fairphone.com>
>>>>> +
>>>>> +description:
>>>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm 
>>>>> platforms.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: qcom,sm6350-camss
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 12
>>>>> +
>>>>> +  reg-names:
>>>>> +    items:
>>>>> +      - const: csid0
>>>>> +      - const: csid1
>>>>> +      - const: csid2
>>>>> +      - const: csid_lite
>>>>> +      - const: csiphy0
>>>>> +      - const: csiphy1
>>>>> +      - const: csiphy2
>>>>> +      - const: csiphy3
>>>>> +      - const: vfe0
>>>>> +      - const: vfe1
>>>>> +      - const: vfe2
>>>>> +      - const: vfe_lite
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 30
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: cam_ahb_clk
>>>>> +      - const: cam_axi
>>>>> +      - const: soc_ahb
>>>>> +      - const: camnoc_axi
>>>>> +      - const: core_ahb
>>>>> +      - const: cpas_ahb
>>>>> +      - const: csiphy0
>>>>> +      - const: csiphy0_timer
>>>>> +      - const: csiphy1
>>>>> +      - const: csiphy1_timer
>>>>> +      - const: csiphy2
>>>>> +      - const: csiphy2_timer
>>>>> +      - const: csiphy3
>>>>> +      - const: csiphy3_timer
>>>>> +      - const: slow_ahb_src
>>>>> +      - const: vfe0_axi
>>>>> +      - const: vfe0
>>>>> +      - const: vfe0_cphy_rx
>>>>> +      - const: vfe0_csid
>>>>> +      - const: vfe1_axi
>>>>> +      - const: vfe1
>>>>> +      - const: vfe1_cphy_rx
>>>>> +      - const: vfe1_csid
>>>>> +      - const: vfe2_axi
>>>>> +      - const: vfe2
>>>>> +      - const: vfe2_cphy_rx
>>>>> +      - const: vfe2_csid
>>>>> +      - const: vfe_lite
>>>>> +      - const: vfe_lite_cphy_rx
>>>>> +      - const: vfe_lite_csid
>>>>
>>>> The sorting order of this list does not follow the sorting order 
>>>> accepted
>>>> in the past.
>>>
>>> What file should I best reference?
>>
>> Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
>>
>>>>
>>>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>>>> sorting order name, because it triggers people.
>>>>
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 12
>>>>> +
>>>>> +  interrupt-names:
>>>>> +    items:
>>>>> +      - const: csid0
>>>>> +      - const: csid1
>>>>> +      - const: csid2
>>>>> +      - const: csid_lite
>>>>> +      - const: csiphy0
>>>>> +      - const: csiphy1
>>>>> +      - const: csiphy2
>>>>> +      - const: csiphy3
>>>>> +      - const: vfe0
>>>>> +      - const: vfe1
>>>>> +      - const: vfe2
>>>>> +      - const: vfe_lite
>>>>> +
>>>>> +  interconnects:
>>>>> +    maxItems: 4
>>>>> +
>>>>> +  interconnect-names:
>>>>> +    items:
>>>>> +      - const: ahb
>>>>> +      - const: hf_mnoc
>>>>> +      - const: sf_mnoc
>>>>> +      - const: sf_icp_mnoc
>>>>
>>>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>>>> IP to produce raw images, and one day you may use them somewhere else.
>>>
>>> Ack, will give it a try.
>>
>> Disagree with this.
>>
>> See the Kanaapali patches. I'm asking new submissions to be as complete
>> as possible, instead of limiting the hardware description to the RDI.
>>
>> So listing the ICP noc is the right thing to do.
>>
>> So please include register banks for
>>
>> - bps
>> - cdm
>> - icp
>> - ipe
>> - jpeg
>> - lrme
> 
> This suggests to get a DT backward compatibility lock forever, 

I'm not entirely sure what this comment means.

The objective here is to get a full and complete DT describing camera 
hardware that can be consumed by

- CAMSS RDI
- CAMSS RDI + future goodness
- FreeBSD
- Any other DT consumer of upstream data perhaps even CamX
which
> makes it
> a very bad advice, which is favourable for just the single interested 
> party,
> which offers an alternative to the upstream CAMSS.
> 
> Anybody who wants to get support of any CAMSS ISP functionality above RAW
> images shall not add anything unrelated and unsupported.
> 
> The asked inclusion shall be done later on safely, if ever needed.

As I say the objective is to fully describe the hardware in DT, not to 
describe only the RDI path.

>>>> I believe this clock is critical, and it is set so in the SM6350 GCC 
>>>> driver,
>>>> therefore it should not be added here.
>>>
>>> True, gcc_camera_ahb_clk has CLK_IS_CRITICAL in gcc-sm6350.c
>>
>> DT describes hardware, not the happenstance of Linux driver setup.
>>
>> On that basis omitting <&gcc GCC_CAMERA_AHB_CLK> from the clock list is
>> not correct.
> 
> This has been already discussed, an enumerous amount of Qualcomm/MSM
> specific resourced like clocks enabled in ABL, Linux etc. are considered
> critical and not described in the dtb.

I still think the argument for that is tenuous wrong in fact but, I know 
you are right, this is a lost argument.

@Luca please _do_ include the full array of registers/clocks/nocs as you 
find them and yeah drop the AXI clock from that description because reasons.

https://lore.kernel.org/linux-arm-msm/20251113-add-support-for-camss-on-kaanapali-v6-1-1e6038785a8e@oss.qualcomm.com/

  ---
bod