| 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
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>; + }; + }; + }; + }; + };