| Message ID | 20240812144131.369378-8-quic_depengs@quicinc.com (mailing list archive) |
|---|---|
| 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 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
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
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
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 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
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.
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 16/08/2024 09:45, Depeng Shao wrote: > 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> The review tag was given to above code with above changes. If you are going to implement some more changes, including changing of orders of some lists or adding ports, then drop this tag and explicitly mention in patch changelog that tag was not added because of something. Best regards, Krzysztof
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. And if Linux behavior changes then are you going to rewrite all the DTS for new size? No, the sizes reflect hardware register layout, not concept of pages. I don't think that we should be coming with more nitpicky ideas, one month after the patch was sent and reviewed. Best regards, Krzysztof
On 9/30/24 10:26, 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. > > And if Linux behavior changes then are you going to rewrite all the DTS > for new size? If Linux behaves properly with page size alignments today, then the selected page size alignment for AMBA device IO memory regions is correct, hence any future change from the correct IP device description to another one will be invalid or noop. There is nothing to worry about, I believe. > No, the sizes reflect hardware register layout, not concept of pages. > Absolutely they do. It might be a coincidence that both are aligned in this particular case or another one. > I don't think that we should be coming with more nitpicky ideas, one > month after the patch was sent and reviewed. The change is not yet ready to be accepted from the technical perspective. -- Best wishes, Vladimir
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
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 30/09/2024 08:26, Krzysztof Kozlowski wrote: >>> 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. > And if Linux behavior changes then are you going to rewrite all the DTS > for new size? > > No, the sizes reflect hardware register layout, not concept of pages. > > I don't think that we should be coming with more nitpicky ideas, one > month after the patch was sent and reviewed. Agree. 1. My understanding has always been: - Map the entire register bank extent - The main reason for that is today you might only use 1/4 of the registers in a given bank but tomorrow you might add in new functionality - like the HardISP in which case you'd want the full set of registers not just the 1/4 or the 4k aligned version of that bank. 2. Pages can be all sorts of sizes so aligning to a page makes no sense. 4k isn't special. https://en.wikipedia.org/wiki/Page_(computer_memory)#Multiple_page_sizes --- 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
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 Comments, corrections and objections are always welcome. -- Best wishes, Vladimir
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
Hi Vladimir, On 10/8/2024 10:06 PM, Bryan O'Donoghue wrote: > 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. > Yes, I have picked your patch and rebased the SM8550 change based on your patch. I also verified them and it works good. But I don't understand why the names are csi01, csi23, csi46, csi57. Could you please elaborate more? I'm using csiphyX-vdda-phy-supply and csiphyX-vdda-pll-supply now. 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>; + }; + }; + }; + }; + };