Re: [PATCH RFC/WIP v2 4/9] arm64: dts: qcom: sa8775p: Add support for camss

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

On 10/05/2025 08:14, Suresh Vankadara wrote:
> 
> 
> On 4/27/2025 12:31 PM, Vikram Sharma wrote:
>> Add changes to support the camera subsystem on the SA8775P.
>>
>> Co-developed-by: Suresh Vankadara <quic_svankada@quicinc.com>
>> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sa8775p.dtsi | 187 ++++++++++++++++++++++++++
>>   1 file changed, 187 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/ 
>> dts/qcom/sa8775p.dtsi
>> index 5bd0c03476b1..81eadb2bb663 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> @@ -7,6 +7,7 @@
>>   #include <dt-bindings/interconnect/qcom,icc.h>
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   #include <dt-bindings/clock/qcom,rpmh.h>
>> +#include <dt-bindings/clock/qcom,sa8775p-camcc.h>
>>   #include <dt-bindings/clock/qcom,sa8775p-dispcc.h>
>>   #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
>>   #include <dt-bindings/clock/qcom,sa8775p-gpucc.h>
>> @@ -3940,6 +3941,192 @@ videocc: clock-controller@abf0000 {
>>               #power-domain-cells = <1>;
>>           };
>> +        camss: isp@ac7a000 {
>> +            compatible = "qcom,sa8775p-camss";
> If more number of nodes are added for CAMSS, adding isp in compatible 
> string helps to differentiate.

We need to keep a consistent upstream schema.

If we were adding other hardware blocks - say the BPS it would just be 
appended to the end here, declared as another v4l2 device and then 
wired-together from user-space via likely a qcom specific libcamera 
pipeline.

> 
>> +            reg-names = "csid0",
>> +                    "csid1",
>> +                    "csid_lite0",
>> +                    "csid_lite1",
>> +                    "csid_lite2",
>> +                    "csid_lite3",
>> +                    "csid_lite4",
>> +                    "csid_wrapper",
> csid wrapper is top register set, which is applicable for both csid 0 
> and csid 1. It is logical to keep along with csid0 and csid1, instead of 
> alpha numerical order.

We've had it feels like an eternity of debates about this and 
compromised on alphanum sort of of node-names as the most consistent 
with prior art.

> 
>> +
>> +            clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>> +                 <&camcc CAM_CC_CORE_AHB_CLK>,
>> +                 <&camcc CAM_CC_CPAS_AHB_CLK>,
>> +                 <&camcc CAM_CC_CPAS_FAST_AHB_CLK>,
>> +                 <&camcc CAM_CC_CPAS_IFE_LITE_CLK>,
>> +                 <&camcc CAM_CC_CPAS_IFE_0_CLK>,
>> +                 <&camcc CAM_CC_CPAS_IFE_1_CLK>,
>> +                 <&camcc CAM_CC_CSID_CLK>,
>> +                 <&camcc CAM_CC_CSIPHY0_CLK>,
>> +                 <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
>> +                 <&camcc CAM_CC_CSIPHY1_CLK>,
>> +                 <&camcc CAM_CC_CSI1PHYTIMER_CLK>,
>> +                 <&camcc CAM_CC_CSIPHY2_CLK>,
>> +                 <&camcc CAM_CC_CSI2PHYTIMER_CLK>,
>> +                 <&camcc CAM_CC_CSIPHY3_CLK>,
>> +                 <&camcc CAM_CC_CSI3PHYTIMER_CLK>,
>> +                 <&camcc CAM_CC_CSID_CSIPHY_RX_CLK>,
>> +                 <&gcc GCC_CAMERA_HF_AXI_CLK>,
>> +                 <&gcc GCC_CAMERA_SF_AXI_CLK>,
>> +                 <&camcc CAM_CC_ICP_AHB_CLK>,
>> +                 <&camcc CAM_CC_IFE_0_CLK>,
>> +                 <&camcc CAM_CC_IFE_0_FAST_AHB_CLK>,
>> +                 <&camcc CAM_CC_IFE_1_CLK>,
>> +                 <&camcc CAM_CC_IFE_1_FAST_AHB_CLK>,
>> +                 <&camcc CAM_CC_IFE_LITE_CLK>,
>> +                 <&camcc CAM_CC_IFE_LITE_AHB_CLK>,
>> +                 <&camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>,
>> +                 <&camcc CAM_CC_IFE_LITE_CSID_CLK>;
>> +            clock-names = "camnoc_axi",
>> +                      "core_ahb",
>> +                      "cpas_ahb",
>> +                      "cpas_fast_ahb_clk",
>> +                      "cpas_ife_lite",
>> +                      "cpas_vfe0",
>> +                      "cpas_vfe1",
> Maintain consistency on vfe/ife in complete camss node. In reg section, 
> vfe is used for full and lite version. in clock-names section ife lite 
> and vfe are used. As clock IDs upstream and ife is used for full and 
> lite, this convention will be followed in camss node as well.
> 
>> +                      "csid",
>> +                      "csiphy0",
>> +                      "csiphy0_timer",
>> +                      "csiphy1",
>> +                      "csiphy1_timer",
>> +                      "csiphy2",
>> +                      "csiphy2_timer",
>> +                      "csiphy3",
>> +                      "csiphy3_timer",
>> +                      "csiphy_rx",
>> +                      "gcc_axi_hf",
>> +                      "gcc_axi_sf",
>> +                      "icp_ahb",
> sf and icp_ahb clocks needed?
> 
>> +
>> +            interconnects = <&gem_noc MASTER_APPSS_PROC 
>> QCOM_ICC_TAG_ACTIVE_ONLY
>> +                     &config_noc SLAVE_CAMERA_CFG 
>> QCOM_ICC_TAG_ACTIVE_ONLY>,
>> +                    <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
>> +                     &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>> +                    <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ACTIVE_ONLY
>> +                     &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
>> +            interconnect-names = "ahb",
>> +                         "hf_0",
>> +                         "sf_0";
> sf_0 needed?
> 
>> +
>> +            iommus = <&apps_smmu 0x3400 0x20>;
> 
> 
> Regards,
> Suresh Vankadara.