| Message ID | 20240812144131.369378-8-quic_depengs@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: qcom: camss: Add sm8550 support | expand |
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
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
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
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
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
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
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
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
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
On 13/09/2024 06:06, Vladimir Zapolskiy wrote: > 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? I asked Krzysztof about this offline. He said something like Quote: This is possible, but I think not between child nodes. https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/devicetree/bindings/example-schema.yaml#L194 You could require something in children, but not in parent node. For children something around: https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/devicetree/bindings/net/qcom,ipa.yaml#L174 allOf: - if: required: - something-in-parent then: properties: child-node: required: - something-in-child I will see if I can turn that into a workable proposal/patch. --- bod
Hi Bryan, On 9/18/24 01:40, Bryan O'Donoghue wrote: > On 13/09/2024 06:06, Vladimir Zapolskiy wrote: >> 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? > > I asked Krzysztof about this offline. > > He said something like > > Quote: > This is possible, but I think not between child nodes. > https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/devicetree/bindings/example-schema.yaml#L194 > > You could require something in children, but not in parent node. For > children something around: > https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/devicetree/bindings/net/qcom,ipa.yaml#L174 > > allOf: > - if: > required: > - something-in-parent > then: > properties: > child-node: > required: > - something-in-child > > I will see if I can turn that into a workable proposal/patch. > thank you for pushing my review request forward. Overall I believe making supply properties as optional ones is sufficient, technically straightforward and merely good enough, thus please let me ask you to ponder on this particular variant one more time. -- Best wishes, Vladimir
Hi Vladimir, On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote: >> + 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. > Sure, I previously sorted by the alphabetical order of reg_name. I will update it based on your suggestion. And will also make sure the align all sizes to 0x1000. >> + <&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? > Sure, will do. >> + >> + 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. Krzysztof posted a comment in a SC7280 camss change and asked to remove the "cam_" prefix. https://lore.kernel.org/all/087e7f29-1fa8-4bc2-bb3d-acb941432381@kernel.org/ >> + 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... > I tried it, but dt_binding_check report below warning. Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:221.29-39: Warning (reg_format): /example-0/soc/camss@ace4000/ports/port@0/endpoint@0:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1) Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: Warning (avoid_default_addr_size): /example-0/soc/camss@ace4000/ports/port@0/endpoint@0: Relying on default #address-cells value Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: Warning (avoid_default_addr_size): /example-0/soc/camss@ace4000/ports/port@0/endpoint@0: Relying on default #size-cells value Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size' Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: Warning (graph_endpoint): /example-0/soc/camss@ace4000/ports/port@0/endpoint@0: graph node '#address-cells' is -1, must be 1 Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: Warning (graph_endpoint): /example-0/soc/camss@ace4000/ports/port@0/endpoint@0: graph node '#size-cells' is -1, must be 0 >> + port@0 { >> + reg = <0>; >> + #address-cells = <1>; >> + #size-cells = <0>; > > Same as above. > Same warning.. >> + >> + csiphy_ep0: endpoint@0 { >> + reg = <0>; >> + clock-lanes = <7>; >> + data-lanes = <0 1>; >> + remote-endpoint = <&sensor_ep>; >> + }; >> + }; >> + }; >> + }; >> + }; > --- Thanks, Depeng
Hi Vladimir, Bryan, On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote: > Hi Bryan, > > On 9/18/24 01:40, Bryan O'Donoghue wrote: >> On 13/09/2024 06:06, Vladimir Zapolskiy wrote: >>> 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? >> >> I asked Krzysztof about this offline. >> >> He said something like >> >> Quote: >> This is possible, but I think not between child nodes. >> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/ >> devicetree/bindings/example-schema.yaml#L194 >> >> You could require something in children, but not in parent node. For >> children something around: >> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/ >> devicetree/bindings/net/qcom,ipa.yaml#L174 >> >> allOf: >> - if: >> required: >> - something-in-parent >> then: >> properties: >> child-node: >> required: >> - something-in-child >> >> I will see if I can turn that into a workable proposal/patch. >> > > thank you for pushing my review request forward. > > Overall I believe making supply properties as optional ones is sufficient, > technically straightforward and merely good enough, thus please let me > ask you to ponder on this particular variant one more time. > So, we are discussing two things. 1# Use separate supplies for each CSI block, looks like there is no doubt about it anymore. So, I will update it just like based on suggestion. csiphyX-vdda-phy-supply csiphyX-vdda-pll-supply Then I will need below items in the required list if they are required. required: - csiphy0-vdda-phy-supply - csiphy0-vdda-pll-supply - csiphy1-vdda-phy-supply - csiphy1-vdda-pll-supply ... - csiphy7-vdda-phy-supply - csiphy7-vdda-pll-supply 2# Regarding the CSI supplies, if they need to be making as optional? Looks like there is no conclusion now. @Bryan, do you agree with this? Thanks, Depeng
On 25/09/2024 17:13, Depeng Shao wrote: > Hi Vladimir, > > On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote: > >>> + 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. >> > > Sure, I previously sorted by the alphabetical order of reg_name. > I will update it based on your suggestion. And will also make sure the > align all sizes to 0x1000. If I understood correctly, you want to change the order from existing devices, so no. You are supposed to keep the same order, as much as possible. Best regards, Krzysztof
On 30/09/2024 09:46, Vladimir Zapolskiy wrote: > Hello Krzysztof, > > On 9/30/24 10:16, Krzysztof Kozlowski wrote: >> On 25/09/2024 17:13, Depeng Shao wrote: >>> Hi Vladimir, >>> >>> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote: >>> >>>>> + 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. >>>> >>> >>> Sure, I previously sorted by the alphabetical order of reg_name. >>> I will update it based on your suggestion. And will also make sure the >>> align all sizes to 0x1000. >> >> If I understood correctly, you want to change the order from existing >> devices, so no. You are supposed to keep the same order, as much as >> possible. > > Please elaborate, what do you mean here by the "existing evices"? > > The list is not sorted by reg values, I ask to sort the list by reg values. > > -- > Best wishes, > Vladimir We always sort by address: camss: camss@ac5a000 { compatible = "qcom,sc8280xp-camss"; reg = <0 0x0ac5a000 0 0x2000>, <0 0x0ac5c000 0 0x2000>, <0 0x0ac65000 0 0x2000>, <0 0x0ac67000 0 0x2000>, <0 0x0acaf000 0 0x4000>, <0 0x0acb3000 0 0x1000>, <0 0x0acb6000 0 0x4000>, <0 0x0acba000 0 0x1000>, <0 0x0acbd000 0 0x4000>, <0 0x0acc1000 0 0x1000>, <0 0x0acc4000 0 0x4000>, <0 0x0acc8000 0 0x1000>, <0 0x0accb000 0 0x4000>, <0 0x0accf000 0 0x1000>, <0 0x0acd2000 0 0x4000>, <0 0x0acd6000 0 0x1000>, <0 0x0acd9000 0 0x4000>, <0 0x0acdd000 0 0x1000>, <0 0x0ace0000 0 0x4000>, <0 0x0ace4000 0 0x1000>; reg-names = "csiphy2", "csiphy3", "csiphy0", "csiphy1", "vfe0", "csid0", "vfe1", "csid1", "vfe2", "csid2", "vfe_lite0", "csid0_lite", "vfe_lite1", "csid1_lite", "vfe_lite2", "csid2_lite", "vfe_lite3", "csid3_lite", "vfe3", "csid3"; This is the way. --- bod
On 9/30/24 11:55, Bryan O'Donoghue wrote: > On 30/09/2024 09:46, Vladimir Zapolskiy wrote: >> Hello Krzysztof, >> >> On 9/30/24 10:16, Krzysztof Kozlowski wrote: >>> On 25/09/2024 17:13, Depeng Shao wrote: >>>> Hi Vladimir, >>>> >>>> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote: >>>> >>>>>> + 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. >>>>> >>>> >>>> Sure, I previously sorted by the alphabetical order of reg_name. >>>> I will update it based on your suggestion. And will also make sure the >>>> align all sizes to 0x1000. >>> >>> If I understood correctly, you want to change the order from existing >>> devices, so no. You are supposed to keep the same order, as much as >>> possible. >> >> Please elaborate, what do you mean here by the "existing evices"? >> >> The list is not sorted by reg values, I ask to sort the list by reg values. >> >> -- >> Best wishes, >> Vladimir > > We always sort by address: > Thank you for the given confirmation that there is a need to make the change requested by me. -- Best wishes, Vladimir
Hi Bryan, On 9/25/2024 11:40 PM, Depeng Shao wrote: > Hi Vladimir, Bryan, > > On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote: >> Hi Bryan, >> >> On 9/18/24 01:40, Bryan O'Donoghue wrote: >>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote: >>>> 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? >>> >>> I asked Krzysztof about this offline. >>> >>> He said something like >>> >>> Quote: >>> This is possible, but I think not between child nodes. >>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/ >>> devicetree/bindings/example-schema.yaml#L194 >>> >>> You could require something in children, but not in parent node. For >>> children something around: >>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/ >>> devicetree/bindings/net/qcom,ipa.yaml#L174 >>> >>> allOf: >>> - if: >>> required: >>> - something-in-parent >>> then: >>> properties: >>> child-node: >>> required: >>> - something-in-child >>> >>> I will see if I can turn that into a workable proposal/patch. >>> >> >> thank you for pushing my review request forward. >> >> Overall I believe making supply properties as optional ones is >> sufficient, >> technically straightforward and merely good enough, thus please let me >> ask you to ponder on this particular variant one more time. >> > > So, we are discussing two things. > > 1# Use separate supplies for each CSI block, looks like there is no > doubt about it anymore. So, I will update it just like based on suggestion. > > csiphyX-vdda-phy-supply > csiphyX-vdda-pll-supply > > Then I will need below items in the required list if they are required. > required: > - csiphy0-vdda-phy-supply > - csiphy0-vdda-pll-supply > - csiphy1-vdda-phy-supply > - csiphy1-vdda-pll-supply > ... > - csiphy7-vdda-phy-supply > - csiphy7-vdda-pll-supply > > 2# Regarding the CSI supplies, if they need to be making as optional? > Looks like there is no conclusion now. > > @Bryan, do you agree with this? > I'm preparing the new version patches, and will send out for reviewing in few days. I will follow Vladimir's comments if you have no response, it means making supply properties as optional one, so they won't be added to the required list. Thanks, Depeng
On 25/09/2024 16:40, Depeng Shao wrote: > > 2# Regarding the CSI supplies, if they need to be making as optional? > Looks like there is no conclusion now. > > @Bryan, do you agree with this? It doesn't make sense to have those supplies optional. If you instantiate a csiphy for your board you need a power supply for it. I believe I said I would _try_ to come up with a proposal for that. I should be able to get x1e80100 first pass patches out this week - including such a proposed fix. --- bod
On 08/10/2024 14:50, Vladimir Zapolskiy wrote: > Hi Depeng. > > On 9/30/24 12:26, Depeng Shao wrote: >> Hi Bryan, >> >> On 9/25/2024 11:40 PM, Depeng Shao wrote: >>> Hi Vladimir, Bryan, >>> >>> On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote: >>>> Hi Bryan, >>>> >>>> On 9/18/24 01:40, Bryan O'Donoghue wrote: >>>>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote: >>>>>> 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? >>>>> >>>>> I asked Krzysztof about this offline. >>>>> >>>>> He said something like >>>>> >>>>> Quote: >>>>> This is possible, but I think not between child nodes. >>>>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/ >>>>> devicetree/bindings/example-schema.yaml#L194 >>>>> >>>>> You could require something in children, but not in parent node. For >>>>> children something around: >>>>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/ >>>>> devicetree/bindings/net/qcom,ipa.yaml#L174 >>>>> >>>>> allOf: >>>>> - if: >>>>> required: >>>>> - something-in-parent >>>>> then: >>>>> properties: >>>>> child-node: >>>>> required: >>>>> - something-in-child >>>>> >>>>> I will see if I can turn that into a workable proposal/patch. >>>>> >>>> >>>> thank you for pushing my review request forward. >>>> >>>> Overall I believe making supply properties as optional ones is >>>> sufficient, >>>> technically straightforward and merely good enough, thus please let me >>>> ask you to ponder on this particular variant one more time. >>>> >>> >>> So, we are discussing two things. >>> >>> 1# Use separate supplies for each CSI block, looks like there is no >>> doubt about it anymore. So, I will update it just like based on >>> suggestion. >>> >>> csiphyX-vdda-phy-supply >>> csiphyX-vdda-pll-supply >>> >>> Then I will need below items in the required list if they are required. >>> required: >>> - csiphy0-vdda-phy-supply >>> - csiphy0-vdda-pll-supply >>> - csiphy1-vdda-phy-supply >>> - csiphy1-vdda-pll-supply >>> ... >>> - csiphy7-vdda-phy-supply >>> - csiphy7-vdda-pll-supply >>> >>> 2# Regarding the CSI supplies, if they need to be making as optional? >>> Looks like there is no conclusion now. >>> >>> @Bryan, do you agree with this? >>> >> >> I'm preparing the new version patches, and will send out for reviewing >> in few days. I will follow Vladimir's comments if you have no response, >> it means making supply properties as optional one, so they won't be >> added to the required list. >> > > Recently I published the change, which moves regulator supplies from CSID > to CSIPHY, I believe it makes sense to base the SM8550 change and > regulators > under discussion on top of the series: > > https://lore.kernel.org/all/20240926211957.4108692-1- > vladimir.zapolskiy@linaro.org/ > > Note, that SM8250 regulators are not changed, however their names are > wrong, > the correction shall be a separate change later on... > > Next, I developed my opinion regarding the supply regulator property names: > > 1) voltage supply regulator property names match the pattern "*v*-supply", > and the most common name is "vdd*-supply", the match to the pattern > shall > be preserved, > 2) also it would be much better and it will exclude any confusion, if > SoC pin > names are put into the name, like it is done in a multitude of similar > cases. > > So, in my opinion for SM8550 CAMSS a proposed set of voltage supply > regulator > names should be this one: > > - vdda-csi01-0p9-supply > - vdda-csi01-1p2-supply > - vdda-csi23-0p9-supply > - vdda-csi23-1p2-supply > - vdda-csi46-0p9-supply > - vdda-csi46-1p2-supply > - vdda-csi57-0p9-supply > - vdda-csi57-1p2-supply So I communicated to Depeng to take the patch for the regulators but, I still don't think the above is the right way to do this. I will take a pass at constructing something in the schema to capture the case where a regulator is required if and only if it is instantiated. May not be possible with our current syntax/tools but is 100% how the hardware works so IMO is the right thing to try to do. --- bod
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>; + }; + }; + }; + }; + };