[07/13] dt-bindings: media: camss: Add qcom,sm8550-camss binding

Message ID 20240812144131.369378-8-quic_depengs@quicinc.com (mailing list archive)
State New
Headers
Series media: qcom: camss: Add sm8550 support |

Commit Message

Depeng Shao Aug. 12, 2024, 2:41 p.m. UTC
  Add bindings for qcom,sm8550-camss in order to support the camera
subsystem for sm8550.

Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
 .../bindings/media/qcom,sm8550-camss.yaml     | 517 ++++++++++++++++++
 1 file changed, 517 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,sm8550-camss.yaml
  

Comments

Krzysztof Kozlowski Aug. 16, 2024, 7:01 a.m. UTC | #1
On 12/08/2024 16:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
> 
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>

...

> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - interconnects
> +  - interconnect-names
> +  - interrupts
> +  - interrupt-names
> +  - iommus
> +  - power-domains
> +  - power-domain-names
> +  - reg
> +  - reg-names
> +  - vdda-phy-supply
> +  - vdda-pll-supply

Order is still not as expected. I already commented on this - keep the
same order as in "properties:" block.

With the order fixed:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
  
Depeng Shao Aug. 16, 2024, 7:45 a.m. UTC | #2
Hi Krzysztof,

On 8/16/2024 3:01 PM, Krzysztof Kozlowski wrote:

>> +required:
>> +  - compatible
>> +  - clocks
>> +  - clock-names
>> +  - interconnects
>> +  - interconnect-names
>> +  - interrupts
>> +  - interrupt-names
>> +  - iommus
>> +  - power-domains
>> +  - power-domain-names
>> +  - reg
>> +  - reg-names
>> +  - vdda-phy-supply
>> +  - vdda-pll-supply
> 
> Order is still not as expected. I already commented on this - keep the
> same order as in "properties:" block.
> 
> With the order fixed:
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 

Thanks for catching this, the order was correct in my local build, then 
Vladimir posted a new comment, so I updated it again and forgot to 
update the required item, I will correct the order in next version series.

Vladimir: "I would suggest to put 'compatible', 'reg' and 'reg-names' 
properties as the first ones. 'clock-names' should follow 'clocks' 
property in the list."

Thanks,
Depeng
  
Vladimir Zapolskiy Sept. 5, 2024, 3:20 p.m. UTC | #3
Hello Depeng.

On 8/12/24 17:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
> 
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>

<snip>

> +
> +            interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 431 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 277 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 688 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>;

Please change all interrupt types to IRQ_TYPE_EDGE_RISING, this
will match the type set by the camss driver itself, and I believe
a rising edge interrupt here is correct.

A similar change would be needed in the dts file change.

> +
> +            interrupt-names = "csid0",
> +                              "csid1",
> +                              "csid2",
> +                              "csid_lite0",
> +                              "csid_lite1",
> +                              "csiphy0",
> +                              "csiphy1",
> +                              "csiphy2",
> +                              "csiphy3",
> +                              "csiphy4",
> +                              "csiphy5",
> +                              "csiphy6",
> +                              "csiphy7",
> +                              "vfe0",
> +                              "vfe1",
> +                              "vfe2",
> +                              "vfe_lite0",
> +                              "vfe_lite1";

--
Best wishes,
Vladimir
  
Depeng Shao Sept. 5, 2024, 3:54 p.m. UTC | #4
Hi Vladimir,

On 9/5/2024 11:20 PM, Vladimir Zapolskiy wrote:
> Hello Depeng.
> 
> On 8/12/24 17:41, Depeng Shao wrote:
>> Add bindings for qcom,sm8550-camss in order to support the camera
>> subsystem for sm8550.
>>
>> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> 
> <snip>
> 
>> +
>> +            interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 431 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 277 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 688 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>;
> 
> Please change all interrupt types to IRQ_TYPE_EDGE_RISING, this
> will match the type set by the camss driver itself, and I believe
> a rising edge interrupt here is correct.
> 
> A similar change would be needed in the dts file change.
> 

Sure, I will try this change. Thanks for the comments.

>> +
>> +            interrupt-names = "csid0",
>> +                              "csid1",
>> +                              "csid2",
>> +                              "csid_lite0",
>> +                              "csid_lite1",
>> +                              "csiphy0",
>> +                              "csiphy1",
>> +                              "csiphy2",
>> +                              "csiphy3",
>> +                              "csiphy4",
>> +                              "csiphy5",
>> +                              "csiphy6",
>> +                              "csiphy7",
>> +                              "vfe0",
>> +                              "vfe1",
>> +                              "vfe2",
>> +                              "vfe_lite0",
>> +                              "vfe_lite1";
> 
> -- 
> Best wishes,
> Vladimir

Thanks,
Depeng
  
Vladimir Zapolskiy Sept. 6, 2024, 3:56 p.m. UTC | #5
Hi Depeng,

On 8/12/24 17:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
> 
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>

there are a few more things, which I noticed.

> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        camss: camss@ace4000 {
> +            compatible = "qcom,sm8550-camss";
> +
> +            reg = <0 0x0acb7000 0 0xd00>,
> +                  <0 0x0acb9000 0 0xd00>,
> +                  <0 0x0acbb000 0 0xd00>,
> +                  <0 0x0acca000 0 0xa00>,
> +                  <0 0x0acce000 0 0xa00>,
> +                  <0 0x0acb6000 0 0x1000>,
> +                  <0 0x0ace4000 0 0x2000>,
> +                  <0 0x0ace6000 0 0x2000>,
> +                  <0 0x0ace8000 0 0x2000>,
> +                  <0 0x0acea000 0 0x2000>,
> +                  <0 0x0acec000 0 0x2000>,
> +                  <0 0x0acee000 0 0x2000>,
> +                  <0 0x0acf0000 0 0x2000>,
> +                  <0 0x0acf2000 0 0x2000>,
> +                  <0 0x0ac62000 0 0xf000>,
> +                  <0 0x0ac71000 0 0xf000>,
> +                  <0 0x0ac80000 0 0xf000>,
> +                  <0 0x0accb000 0 0x2800>,
> +                  <0 0x0accf000 0 0x2800>;

Please sort the list above in numerical order, this will change positions
of "vfe_lite0", "vfe_lite1" etc.

Another note, since it's not possible to map less than a page, so I believe
it might make sense to align all sizes to 0x1000.

> +            reg-names = "csid0",
> +                        "csid1",
> +                        "csid2",
> +                        "csid_lite0",
> +                        "csid_lite1",
> +                        "csid_top",
> +                        "csiphy0",
> +                        "csiphy1",
> +                        "csiphy2",
> +                        "csiphy3",
> +                        "csiphy4",
> +                        "csiphy5",
> +                        "csiphy6",
> +                        "csiphy7",
> +                        "vfe0",
> +                        "vfe1",
> +                        "vfe2",
> +                        "vfe_lite0",
> +                        "vfe_lite1";
> +
> +            clocks = <&camcc CAM_CC_CAMNOC_AXI_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_CPAS_IFE_2_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_CSIPHY4_CLK>,
> +                     <&camcc CAM_CC_CSI4PHYTIMER_CLK>,
> +                     <&camcc CAM_CC_CSIPHY5_CLK>,
> +                     <&camcc CAM_CC_CSI5PHYTIMER_CLK>,
> +                     <&camcc CAM_CC_CSIPHY6_CLK>,
> +                     <&camcc CAM_CC_CSI6PHYTIMER_CLK>,
> +                     <&camcc CAM_CC_CSIPHY7_CLK>,
> +                     <&camcc CAM_CC_CSI7PHYTIMER_CLK>,
> +                     <&camcc CAM_CC_CSID_CSIPHY_RX_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_2_CLK>,
> +                     <&camcc CAM_CC_IFE_2_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>,
> +                     <&gcc GCC_CAMERA_HF_AXI_CLK>;

Could you please put the &gcc provided clock as the first one in the list?

> +
> +            clock-names = "camnoc_axi",
> +                          "cpas_ahb",
> +                          "cpas_fast_ahb_clk",
> +                          "cpas_ife_lite",
> +                          "cpas_vfe0",
> +                          "cpas_vfe1",
> +                          "cpas_vfe2",
> +                          "csid",
> +                          "csiphy0",
> +                          "csiphy0_timer",
> +                          "csiphy1",
> +                          "csiphy1_timer",
> +                          "csiphy2",
> +                          "csiphy2_timer",
> +                          "csiphy3",
> +                          "csiphy3_timer",
> +                          "csiphy4",
> +                          "csiphy4_timer",
> +                          "csiphy5",
> +                          "csiphy5_timer",
> +                          "csiphy6",
> +                          "csiphy6_timer",
> +                          "csiphy7",
> +                          "csiphy7_timer",
> +                          "csiphy_rx",
> +                          "vfe0",
> +                          "vfe0_fast_ahb",
> +                          "vfe1",
> +                          "vfe1_fast_ahb",
> +                          "vfe2",
> +                          "vfe2_fast_ahb",
> +                          "vfe_lite",
> +                          "vfe_lite_ahb",
> +                          "vfe_lite_cphy_rx",
> +                          "vfe_lite_csid",
> +                          "gcc_axi_hf";
> +
> +            interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_CAMERA_CFG 0>,
> +                            <&mmss_noc MASTER_CAMNOC_HF 0 &mc_virt SLAVE_EBI1 0>,
> +                            <&mmss_noc MASTER_CAMNOC_ICP 0 &mc_virt SLAVE_EBI1 0>,
> +                            <&mmss_noc MASTER_CAMNOC_SF 0 &mc_virt SLAVE_EBI1 0>;
> +            interconnect-names = "ahb",
> +                                 "hf_0_mnoc",
> +                                 "icp_mnoc",
> +                                 "sf_0_mnoc";

Just a note for myself, interconnect names lost "cam_" prefix, and it should
be fine.

> +
> +            interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 431 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 277 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 688 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>;
> +
> +            interrupt-names = "csid0",
> +                              "csid1",
> +                              "csid2",
> +                              "csid_lite0",
> +                              "csid_lite1",
> +                              "csiphy0",
> +                              "csiphy1",
> +                              "csiphy2",
> +                              "csiphy3",
> +                              "csiphy4",
> +                              "csiphy5",
> +                              "csiphy6",
> +                              "csiphy7",
> +                              "vfe0",
> +                              "vfe1",
> +                              "vfe2",
> +                              "vfe_lite0",
> +                              "vfe_lite1";
> +
> +            iommus = <&apps_smmu 0x800 0x20>;
> +
> +            power-domains = <&camcc CAM_CC_IFE_0_GDSC>,
> +                            <&camcc CAM_CC_IFE_1_GDSC>,
> +                            <&camcc CAM_CC_IFE_2_GDSC>,
> +                            <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +
> +            power-domain-names = "ife0",
> +                                 "ife1",
> +                                 "ife2",
> +                                 "top";
> +
> +            vdda-phy-supply = <&vreg_l1e_0p88>;
> +            vdda-pll-supply = <&vreg_l3e_1p2>;
> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;

In case of a single child node #address-cells/#size-cells could be omitted,
if I'm not mistaken about it...

> +                port@0 {
> +                    reg = <0>;
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;

Same as above.

> +
> +                    csiphy_ep0: endpoint@0 {
> +                        reg = <0>;
> +                        clock-lanes = <7>;
> +                        data-lanes = <0 1>;
> +                        remote-endpoint = <&sensor_ep>;
> +                    };
> +                };
> +            };
> +        };
> +    };

--
Best wishes,
Vladimir
  
Vladimir Zapolskiy Sept. 12, 2024, 8:22 a.m. UTC | #6
Hi Depeng,

I do have one more ask for a change.

On 8/12/24 17:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
> 
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>

<snip>

> +
> +  vdda-phy-supply:
> +    description:
> +      Phandle to a regulator supply to PHY core block.
> +
> +  vdda-pll-supply:
> +    description:
> +      Phandle to 1.2V regulator supply to PHY refclk pll block.
> +

Here the supplies should be split into ones, which are specific to CSI blocks,
and I believe they shall be set as optional.

The proposed names are:

vdda-phy-01-supply
vdda-pll-01-supply
vdda-phy-23-supply
vdda-pll-23-supply
vdda-phy-46-supply
vdda-pll-46-supply
vdda-phy-57-supply
vdda-pll-57-supply

I understand that what I ask is much more clumsy, and it could be seen even as
unneeded, however it'll be the right set of properties to describe the CAMSS IP
in this respect.

--
Best wishes,
Vladimir
  
Bryan O'Donoghue Sept. 12, 2024, 11:41 a.m. UTC | #7
On 12/09/2024 09:22, Vladimir Zapolskiy wrote:
>> +
>> +  vdda-phy-supply:
>> +    description:
>> +      Phandle to a regulator supply to PHY core block.
>> +
>> +  vdda-pll-supply:
>> +    description:
>> +      Phandle to 1.2V regulator supply to PHY refclk pll block.
>> +
> 
> Here the supplies should be split into ones, which are specific to CSI 
> blocks,
> and I believe they shall be set as optional.

In principle I agree with that, each CSIPHY should have its own vdda-phy 
and vdda-pll regulator specified.

In practice though I don't believe its necessary, below.

> The proposed names are:
> 
> vdda-phy-01-supply
> vdda-pll-01-supply
> vdda-phy-23-supply
> vdda-pll-23-supply
> vdda-phy-46-supply
> vdda-pll-46-supply
> vdda-phy-57-supply
> vdda-pll-57-supply

In principle, you're right, we need to expand the name set here.

> I understand that what I ask is much more clumsy, and it could be seen 
> even as
> unneeded, however it'll be the right set of properties to describe the 
> CAMSS IP
> in this respect
I think the following naming would be better as it matches the 
power-grid naming in the docs.

csiphyX-vdda-phy-supply
csiphyX-vdda-pll-supply

=>

// voltage domain = vdd_a_csi_01_09 = regulator l1e
csiphy0-vdda-phy-supply = <&vreg_l1e_0p9>;

// voltage domain = vdd_a_csi_01_1p2 = regulator l3e
csiphy0-vdda-pll-supply = <&vreg_l3e_1p2>;

//
csiphy1-vdda-phy-supply = <&vreg_l1e_0p9>;
csiphy1-vdda-pll-supply = <&vreg_l3e_1p2>;

Where X indicates the CSIPHY number.

So in fact, in practice we don't need to differentiate these entries.

Checking x1e80100 ...

csiphy0

vdda-phy-supply = <&vreg_l2c_0p9>;
vdda-pll-supply = <&vreg_l1c_1p2>;

This is also the case for csiphy 1/2/4

So, I _don't_ believe this is work we need to do, since its the same 
regulator for each PHY.

---
bod
  
Vladimir Zapolskiy Sept. 12, 2024, 12:44 p.m. UTC | #8
Hi Bryan.

On 9/12/24 14:41, Bryan O'Donoghue wrote:
> On 12/09/2024 09:22, Vladimir Zapolskiy wrote:
>>> +
>>> +  vdda-phy-supply:
>>> +    description:
>>> +      Phandle to a regulator supply to PHY core block.
>>> +
>>> +  vdda-pll-supply:
>>> +    description:
>>> +      Phandle to 1.2V regulator supply to PHY refclk pll block.
>>> +
>>
>> Here the supplies should be split into ones, which are specific to CSI
>> blocks,
>> and I believe they shall be set as optional.
> 
> In principle I agree with that, each CSIPHY should have its own vdda-phy
> and vdda-pll regulator specified.
> 
> In practice though I don't believe its necessary, below.
> 
>> The proposed names are:
>>
>> vdda-phy-01-supply
>> vdda-pll-01-supply
>> vdda-phy-23-supply
>> vdda-pll-23-supply
>> vdda-phy-46-supply
>> vdda-pll-46-supply
>> vdda-phy-57-supply
>> vdda-pll-57-supply
> 
> In principle, you're right, we need to expand the name set here.
> 
>> I understand that what I ask is much more clumsy, and it could be seen
>> even as
>> unneeded, however it'll be the right set of properties to describe the
>> CAMSS IP
>> in this respect
> I think the following naming would be better as it matches the
> power-grid naming in the docs.
> 
> csiphyX-vdda-phy-supply
> csiphyX-vdda-pll-supply

I have no opinion about the names, any reason for name selection is
good for me.

> =>
> 
> // voltage domain = vdd_a_csi_01_09 = regulator l1e
> csiphy0-vdda-phy-supply = <&vreg_l1e_0p9>;
> 
> // voltage domain = vdd_a_csi_01_1p2 = regulator l3e
> csiphy0-vdda-pll-supply = <&vreg_l3e_1p2>;
> 
> //
> csiphy1-vdda-phy-supply = <&vreg_l1e_0p9>;
> csiphy1-vdda-pll-supply = <&vreg_l3e_1p2>;
> 
> Where X indicates the CSIPHY number.
> 
> So in fact, in practice we don't need to differentiate these entries.
> 
> Checking x1e80100 ...

Checking some particular board, right?

> csiphy0
> 
> vdda-phy-supply = <&vreg_l2c_0p9>;
> vdda-pll-supply = <&vreg_l1c_1p2>;
> 
> This is also the case for csiphy 1/2/4
> 
> So, I _don't_ believe this is work we need to do, since its the same
> regulator for each PHY.

This is board specific, and even if the separation is not needed on the boards
you have just checked, still it may be needed on some boards, which are not yet
checked/not yet known.

It's needed to make the best predictions about all possible usage of hardware,
fortunately it's easy in this particular case, and it's trivial to correct it now
than on some day later on.

--
Best wishes,
Vladimir
  
Neil Armstrong Sept. 12, 2024, 1:48 p.m. UTC | #9
On 12/09/2024 13:41, Bryan O'Donoghue wrote:
> On 12/09/2024 09:22, Vladimir Zapolskiy wrote:
>>> +
>>> +  vdda-phy-supply:
>>> +    description:
>>> +      Phandle to a regulator supply to PHY core block.
>>> +
>>> +  vdda-pll-supply:
>>> +    description:
>>> +      Phandle to 1.2V regulator supply to PHY refclk pll block.
>>> +
>>
>> Here the supplies should be split into ones, which are specific to CSI blocks,
>> and I believe they shall be set as optional.
> 
> In principle I agree with that, each CSIPHY should have its own vdda-phy and vdda-pll regulator specified.
> 
> In practice though I don't believe its necessary, below.
> 
>> The proposed names are:
>>
>> vdda-phy-01-supply
>> vdda-pll-01-supply
>> vdda-phy-23-supply
>> vdda-pll-23-supply
>> vdda-phy-46-supply
>> vdda-pll-46-supply
>> vdda-phy-57-supply
>> vdda-pll-57-supply
> 
> In principle, you're right, we need to expand the name set here.
> 
>> I understand that what I ask is much more clumsy, and it could be seen even as
>> unneeded, however it'll be the right set of properties to describe the CAMSS IP
>> in this respect
> I think the following naming would be better as it matches the power-grid naming in the docs.
> 
> csiphyX-vdda-phy-supply
> csiphyX-vdda-pll-supply
> 
> =>
> 
> // voltage domain = vdd_a_csi_01_09 = regulator l1e
> csiphy0-vdda-phy-supply = <&vreg_l1e_0p9>;
> 
> // voltage domain = vdd_a_csi_01_1p2 = regulator l3e
> csiphy0-vdda-pll-supply = <&vreg_l3e_1p2>;
> 
> //
> csiphy1-vdda-phy-supply = <&vreg_l1e_0p9>;
> csiphy1-vdda-pll-supply = <&vreg_l3e_1p2>;
> 
> Where X indicates the CSIPHY number.
> 
> So in fact, in practice we don't need to differentiate these entries.
> 
> Checking x1e80100 ...
> 
> csiphy0
> 
> vdda-phy-supply = <&vreg_l2c_0p9>;
> vdda-pll-supply = <&vreg_l1c_1p2>;
> 
> This is also the case for csiphy 1/2/4
> 
> So, I _don't_ believe this is work we need to do, since its the same regulator for each PHY.

Except when it's not the case, like on the SM8650 HDK:
VDD_A_CSI_01_0P9	=> VREG_L2I_0P88
VDD_A_CSI_01_1P2	=> VREG_L3I_1P2
VDD_A_CSI_24_0P9	=> VREG_L1I_0P88
VDD_A_CSI_24_1P2	=> VREG_L3I_1P2
VDD_A_CSI_35_0P9	=> VREG_L2I_0P88
VDD_A_CSI_35_1P2	=> VREG_L3I_1P2

the 1P2 all uses VREG_L3I_1P2, while the 0P9 are using VREG_L2I_0P88 or VREG_L1I_0P88

Not declaring the exact supplies is pure lazyness, it will bounce back at our faces at some point.

Neil

> 
> ---
> bod
  
Bryan O'Donoghue Sept. 12, 2024, 3:11 p.m. UTC | #10
On 12/09/2024 13:44, Vladimir Zapolskiy wrote:
>> csiphy0
>>
>> vdda-phy-supply = <&vreg_l2c_0p9>;
>> vdda-pll-supply = <&vreg_l1c_1p2>;
>>
>> This is also the case for csiphy 1/2/4
>>
>> So, I _don't_ believe this is work we need to do, since its the same
>> regulator for each PHY.
> 
> This is board specific, and even if the separation is not needed on the 
> boards
> you have just checked, still it may be needed on some boards, which are 
> not yet
> checked/not yet known.

There is a Power Grid Analysis document which specifies these rails @ 
the SoC level and assumes you've used the Qcom PMIC to power, moreover 
the PGA re-uses the same regulator over and over again.

You _could_ provide that power from your own PMIC which provides the 
same voltage range as the Qcom PMIC you haven't used. Even if you did 
provide that from your own PMIC you'd have to provide _separate_ rails 
for the various CSIPHYs before it would be required to have a per PHY 
rail requirement on this SoC.

Are people really powering these SoCs with their own PMICs ?
No probably not.

Should we add the support for it anyway ?
Maybe.

So to reiterate:

1. csiphyX-vdda-phy-supply
    csiphyX-vdda-pll-supply

    In the dts and yaml

    => The names should be generic from the perspective of the driver

2. camss.c::csiphy_res_sm8550
    [0].regulators = { "csiphy0-vdda-phy-supply",
                       "csiphy0-vdda-pll-supply" }
    ...

    [N].regulators = { "csiphyN-vdda-phy-supply",
                       "csiphyN-vdda-pll-supply" }

    => The regulators for the PHY should be defined in the
       PHY resources description

3. Required not optional in the yaml

    => You can't use the PHY without its regulators

---
bod
  
Vladimir Zapolskiy Sept. 12, 2024, 8:57 p.m. UTC | #11
Hi Bryan,

On 9/12/24 18:11, Bryan O'Donoghue wrote:
> On 12/09/2024 13:44, Vladimir Zapolskiy wrote:
>>> csiphy0
>>>
>>> vdda-phy-supply = <&vreg_l2c_0p9>;
>>> vdda-pll-supply = <&vreg_l1c_1p2>;
>>>
>>> This is also the case for csiphy 1/2/4
>>>
>>> So, I _don't_ believe this is work we need to do, since its the same
>>> regulator for each PHY.
>>
>> This is board specific, and even if the separation is not needed on the
>> boards
>> you have just checked, still it may be needed on some boards, which are
>> not yet
>> checked/not yet known.
> 
> There is a Power Grid Analysis document which specifies these rails @
> the SoC level and assumes you've used the Qcom PMIC to power, moreover
> the PGA re-uses the same regulator over and over again.
> 
> You _could_ provide that power from your own PMIC which provides the
> same voltage range as the Qcom PMIC you haven't used. Even if you did
> provide that from your own PMIC you'd have to provide _separate_ rails
> for the various CSIPHYs before it would be required to have a per PHY
> rail requirement on this SoC.
> 
> Are people really powering these SoCs with their own PMICs ?
> No probably not.
> 
> Should we add the support for it anyway ?
> Maybe.

To have a set of regulators is a matter of proper IC/IP description, actually
here I see very little option for a divergence or disagreement.

> So to reiterate:
> 
> 1. csiphyX-vdda-phy-supply
>      csiphyX-vdda-pll-supply
> 
>      In the dts and yaml
> 
>      => The names should be generic from the perspective of the driver

As for me I don't care about the particular names, somebody else can care.

> 2. camss.c::csiphy_res_sm8550
>      [0].regulators = { "csiphy0-vdda-phy-supply",
>                         "csiphy0-vdda-pll-supply" }
>      ...
> 
>      [N].regulators = { "csiphyN-vdda-phy-supply",
>                         "csiphyN-vdda-pll-supply" }
> 
>      => The regulators for the PHY should be defined in the
>         PHY resources description

This is obvious.

> 3. Required not optional in the yaml
> 
>      => You can't use the PHY without its regulators

No, the supplies shall be optional, since it's absolutely possible to have
such a board, where supplies are merely not connected to the SoC.

Hence there shall be no requirement to describe any non-present supplies,
which is a legit case, if there is no connection and usage of the
correspondent non-supplied PHY.

--
Best wishes,
Vladimir
  
Bryan O'Donoghue Sept. 12, 2024, 10:41 p.m. UTC | #12
On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>> 3. Required not optional in the yaml
>>
>>      => You can't use the PHY without its regulators
> 
> No, the supplies shall be optional, since it's absolutely possible to have
> such a board, where supplies are merely not connected to the SoC.

For any _used_ PHY both supplies are certainly required.

That's what the yaml/dts check for this should achieve.

---
bod
  
Dmitry Baryshkov Sept. 13, 2024, 4:17 a.m. UTC | #13
On Thu, Sep 12, 2024 at 04:11:58PM GMT, Bryan O'Donoghue wrote:
> On 12/09/2024 13:44, Vladimir Zapolskiy wrote:
> > > csiphy0
> > > 
> > > vdda-phy-supply = <&vreg_l2c_0p9>;
> > > vdda-pll-supply = <&vreg_l1c_1p2>;
> > > 
> > > This is also the case for csiphy 1/2/4
> > > 
> > > So, I _don't_ believe this is work we need to do, since its the same
> > > regulator for each PHY.
> > 
> > This is board specific, and even if the separation is not needed on the
> > boards
> > you have just checked, still it may be needed on some boards, which are
> > not yet
> > checked/not yet known.
> 
> There is a Power Grid Analysis document which specifies these rails @ the
> SoC level and assumes you've used the Qcom PMIC to power, moreover the PGA
> re-uses the same regulator over and over again.
> 
> You _could_ provide that power from your own PMIC which provides the same
> voltage range as the Qcom PMIC you haven't used. Even if you did provide
> that from your own PMIC you'd have to provide _separate_ rails for the
> various CSIPHYs before it would be required to have a per PHY rail
> requirement on this SoC.
> 
> Are people really powering these SoCs with their own PMICs ?
> No probably not.

Yes, they are.
  
Vladimir Zapolskiy Sept. 13, 2024, 5:06 a.m. UTC | #14
On 9/13/24 01:41, Bryan O'Donoghue wrote:
> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>> 3. Required not optional in the yaml
>>>
>>>       => You can't use the PHY without its regulators
>>
>> No, the supplies shall be optional, since it's absolutely possible to have
>> such a board, where supplies are merely not connected to the SoC.
> 
> For any _used_ PHY both supplies are certainly required.
> 
> That's what the yaml/dts check for this should achieve.

I believe it is technically possible by writing an enormously complex
scheme, when all possible "port" cases and combinations are listed.

Do you see any simpler way? Do you insist that it is utterly needed?

In any case, there are optional and required device tree properties,
the CAMSS supplies shall be split into multiple ones and become optional.

That's exactly the point of my first message in the discussion, so far
nothing has been added or changed.

--
Best wishes,
Vladimir
  

Patch

diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-camss.yaml
new file mode 100644
index 000000000000..2d6c5a42eeda
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,sm8550-camss.yaml
@@ -0,0 +1,517 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,sm8550-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SM8550 Camera Subsystem (CAMSS)
+
+maintainers:
+  - Depeng Shao <quic_depengs@quicinc.com>
+
+description:
+  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
+
+properties:
+  compatible:
+    const: qcom,sm8550-camss
+
+  reg:
+    maxItems: 19
+
+  reg-names:
+    items:
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csid_lite0
+      - const: csid_lite1
+      - const: csid_top
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: csiphy4
+      - const: csiphy5
+      - const: csiphy6
+      - const: csiphy7
+      - const: vfe0
+      - const: vfe1
+      - const: vfe2
+      - const: vfe_lite0
+      - const: vfe_lite1
+
+  clocks:
+    maxItems: 36
+
+  clock-names:
+    items:
+      - const: camnoc_axi
+      - const: cpas_ahb
+      - const: cpas_fast_ahb_clk
+      - const: cpas_ife_lite
+      - const: cpas_vfe0
+      - const: cpas_vfe1
+      - const: cpas_vfe2
+      - const: csid
+      - const: csiphy0
+      - const: csiphy0_timer
+      - const: csiphy1
+      - const: csiphy1_timer
+      - const: csiphy2
+      - const: csiphy2_timer
+      - const: csiphy3
+      - const: csiphy3_timer
+      - const: csiphy4
+      - const: csiphy4_timer
+      - const: csiphy5
+      - const: csiphy5_timer
+      - const: csiphy6
+      - const: csiphy6_timer
+      - const: csiphy7
+      - const: csiphy7_timer
+      - const: csiphy_rx
+      - const: vfe0
+      - const: vfe0_fast_ahb
+      - const: vfe1
+      - const: vfe1_fast_ahb
+      - const: vfe2
+      - const: vfe2_fast_ahb
+      - const: vfe_lite
+      - const: vfe_lite_ahb
+      - const: vfe_lite_cphy_rx
+      - const: vfe_lite_csid
+      - const: gcc_axi_hf
+
+  interconnects:
+    maxItems: 4
+
+  interconnect-names:
+    items:
+      - const: ahb
+      - const: hf_0_mnoc
+      - const: icp_mnoc
+      - const: sf_0_mnoc
+
+  interrupts:
+    maxItems: 18
+
+  interrupt-names:
+    items:
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csid_lite0
+      - const: csid_lite1
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: csiphy4
+      - const: csiphy5
+      - const: csiphy6
+      - const: csiphy7
+      - const: vfe0
+      - const: vfe1
+      - const: vfe2
+      - const: vfe_lite0
+      - const: vfe_lite1
+
+  iommus:
+    maxItems: 1
+
+  power-domains:
+    items:
+      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
+
+  power-domain-names:
+    items:
+      - const: ife0
+      - const: ife1
+      - const: ife2
+      - const: top
+
+  vdda-phy-supply:
+    description:
+      Phandle to a regulator supply to PHY core block.
+
+  vdda-pll-supply:
+    description:
+      Phandle to 1.2V regulator supply to PHY refclk pll block.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    description:
+      CSI input ports.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@1:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@2:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@3:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@4:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@5:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+  - interconnects
+  - interconnect-names
+  - interrupts
+  - interrupt-names
+  - iommus
+  - power-domains
+  - power-domain-names
+  - reg
+  - reg-names
+  - vdda-phy-supply
+  - vdda-pll-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/clock/qcom,sm8550-camcc.h>
+    #include <dt-bindings/clock/qcom,sm8550-gcc.h>
+    #include <dt-bindings/interconnect/qcom,sm8550-rpmh.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        camss: camss@ace4000 {
+            compatible = "qcom,sm8550-camss";
+
+            reg = <0 0x0acb7000 0 0xd00>,
+                  <0 0x0acb9000 0 0xd00>,
+                  <0 0x0acbb000 0 0xd00>,
+                  <0 0x0acca000 0 0xa00>,
+                  <0 0x0acce000 0 0xa00>,
+                  <0 0x0acb6000 0 0x1000>,
+                  <0 0x0ace4000 0 0x2000>,
+                  <0 0x0ace6000 0 0x2000>,
+                  <0 0x0ace8000 0 0x2000>,
+                  <0 0x0acea000 0 0x2000>,
+                  <0 0x0acec000 0 0x2000>,
+                  <0 0x0acee000 0 0x2000>,
+                  <0 0x0acf0000 0 0x2000>,
+                  <0 0x0acf2000 0 0x2000>,
+                  <0 0x0ac62000 0 0xf000>,
+                  <0 0x0ac71000 0 0xf000>,
+                  <0 0x0ac80000 0 0xf000>,
+                  <0 0x0accb000 0 0x2800>,
+                  <0 0x0accf000 0 0x2800>;
+            reg-names = "csid0",
+                        "csid1",
+                        "csid2",
+                        "csid_lite0",
+                        "csid_lite1",
+                        "csid_top",
+                        "csiphy0",
+                        "csiphy1",
+                        "csiphy2",
+                        "csiphy3",
+                        "csiphy4",
+                        "csiphy5",
+                        "csiphy6",
+                        "csiphy7",
+                        "vfe0",
+                        "vfe1",
+                        "vfe2",
+                        "vfe_lite0",
+                        "vfe_lite1";
+
+            clocks = <&camcc CAM_CC_CAMNOC_AXI_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_CPAS_IFE_2_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_CSIPHY4_CLK>,
+                     <&camcc CAM_CC_CSI4PHYTIMER_CLK>,
+                     <&camcc CAM_CC_CSIPHY5_CLK>,
+                     <&camcc CAM_CC_CSI5PHYTIMER_CLK>,
+                     <&camcc CAM_CC_CSIPHY6_CLK>,
+                     <&camcc CAM_CC_CSI6PHYTIMER_CLK>,
+                     <&camcc CAM_CC_CSIPHY7_CLK>,
+                     <&camcc CAM_CC_CSI7PHYTIMER_CLK>,
+                     <&camcc CAM_CC_CSID_CSIPHY_RX_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_2_CLK>,
+                     <&camcc CAM_CC_IFE_2_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>,
+                     <&gcc GCC_CAMERA_HF_AXI_CLK>;
+
+            clock-names = "camnoc_axi",
+                          "cpas_ahb",
+                          "cpas_fast_ahb_clk",
+                          "cpas_ife_lite",
+                          "cpas_vfe0",
+                          "cpas_vfe1",
+                          "cpas_vfe2",
+                          "csid",
+                          "csiphy0",
+                          "csiphy0_timer",
+                          "csiphy1",
+                          "csiphy1_timer",
+                          "csiphy2",
+                          "csiphy2_timer",
+                          "csiphy3",
+                          "csiphy3_timer",
+                          "csiphy4",
+                          "csiphy4_timer",
+                          "csiphy5",
+                          "csiphy5_timer",
+                          "csiphy6",
+                          "csiphy6_timer",
+                          "csiphy7",
+                          "csiphy7_timer",
+                          "csiphy_rx",
+                          "vfe0",
+                          "vfe0_fast_ahb",
+                          "vfe1",
+                          "vfe1_fast_ahb",
+                          "vfe2",
+                          "vfe2_fast_ahb",
+                          "vfe_lite",
+                          "vfe_lite_ahb",
+                          "vfe_lite_cphy_rx",
+                          "vfe_lite_csid",
+                          "gcc_axi_hf";
+
+            interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_CAMERA_CFG 0>,
+                            <&mmss_noc MASTER_CAMNOC_HF 0 &mc_virt SLAVE_EBI1 0>,
+                            <&mmss_noc MASTER_CAMNOC_ICP 0 &mc_virt SLAVE_EBI1 0>,
+                            <&mmss_noc MASTER_CAMNOC_SF 0 &mc_virt SLAVE_EBI1 0>;
+            interconnect-names = "ahb",
+                                 "hf_0_mnoc",
+                                 "icp_mnoc",
+                                 "sf_0_mnoc";
+
+            interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 431 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 277 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 688 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>;
+
+            interrupt-names = "csid0",
+                              "csid1",
+                              "csid2",
+                              "csid_lite0",
+                              "csid_lite1",
+                              "csiphy0",
+                              "csiphy1",
+                              "csiphy2",
+                              "csiphy3",
+                              "csiphy4",
+                              "csiphy5",
+                              "csiphy6",
+                              "csiphy7",
+                              "vfe0",
+                              "vfe1",
+                              "vfe2",
+                              "vfe_lite0",
+                              "vfe_lite1";
+
+            iommus = <&apps_smmu 0x800 0x20>;
+
+            power-domains = <&camcc CAM_CC_IFE_0_GDSC>,
+                            <&camcc CAM_CC_IFE_1_GDSC>,
+                            <&camcc CAM_CC_IFE_2_GDSC>,
+                            <&camcc CAM_CC_TITAN_TOP_GDSC>;
+
+            power-domain-names = "ife0",
+                                 "ife1",
+                                 "ife2",
+                                 "top";
+
+            vdda-phy-supply = <&vreg_l1e_0p88>;
+            vdda-pll-supply = <&vreg_l3e_1p2>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+
+                    csiphy_ep0: endpoint@0 {
+                        reg = <0>;
+                        clock-lanes = <7>;
+                        data-lanes = <0 1>;
+                        remote-endpoint = <&sensor_ep>;
+                    };
+                };
+            };
+        };
+    };