| Message ID | 20241119-b4-linux-next-24-11-18-dtsi-x1e80100-camss-v1-2-54075d75f654@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add dt-bindings and dtsi changes for CAMSS on x1e80100 silicon | expand |
Hi Bryan, On 11/19/24 17:11, Bryan O'Donoghue wrote: > On 19/11/2024 14:34, Vladimir Zapolskiy wrote: >> Hi Bryan, >> >> please find a few review comments below. >> >> On 11/19/24 15:10, Bryan O'Donoghue wrote: >>> Add bindings for qcom,x1e80100-camss in order to support the camera >>> subsystem for x1e80100 as found in various Co-Pilot laptops. >>> >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> --- >>> .../bindings/media/qcom,x1e80100-camss.yaml | 354 +++++++++++ >>> ++++++++++ >>> 1 file changed, 354 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100- >>> camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100- >>> camss.yaml >>> new file mode 100644 >>> index >>> 0000000000000000000000000000000000000000..ca2499cd52a51e14bad3cf8a8ca94c9d23ed5030 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml >>> @@ -0,0 +1,354 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/qcom,x1e80100-camss.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Qualcomm X1E80100 Camera Subsystem (CAMSS) >>> + >>> +maintainers: >>> + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> + >>> +description: | >>> + The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms. >>> + >>> +properties: >>> + compatible: >>> + const: qcom,x1e80100-camss >>> + >>> + clocks: >>> + maxItems: 29 >>> + >>> + clock-names: >>> + items: >>> + - const: camnoc_rt_axi >>> + - const: camnoc_nrt_axi >>> + - const: core_ahb >>> + - const: cpas_ahb >>> + - const: cpas_fast_ahb >>> + - const: cpas_vfe0 >>> + - const: cpas_vfe1 >>> + - const: cpas_vfe_lite >>> + - const: cphy_rx_clk_src >>> + - const: csid >>> + - const: csid_csiphy_rx >>> + - const: csiphy0 >>> + - const: csiphy0_timer >>> + - const: csiphy1 >>> + - const: csiphy1_timer >>> + - const: csiphy2 >>> + - const: csiphy2_timer >>> + - const: csiphy4 >>> + - const: csiphy4_timer >> >> What does happen to csiphy3? Could it fall through the cracks? >> > > Nope. > > For whatever reason csiphy4 is the name here. I guess different SKUs > have been fused out this way. I'd assume there's some version that does > csiphy0-csiphy4 inclusive. > > Not here though. > >>> + - const: gcc_axi_hf >>> + - const: gcc_axi_sf >>> + - const: vfe0 >>> + - const: vfe0_fast_ahb >>> + - const: vfe1 >>> + - const: vfe1_fast_ahb >>> + - const: vfe_lite >>> + - const: vfe_lite_ahb >>> + - const: vfe_lite_cphy_rx >>> + - const: vfe_lite_csid >>> + >>> + interrupts: >>> + maxItems: 13 >>> + >>> + interrupt-names: >>> + items: >>> + - const: csid0 >>> + - const: csid1 >>> + - const: csid2 >>> + - const: csid_lite0 >>> + - const: csid_lite1 >>> + - const: csiphy0 >>> + - const: csiphy1 >>> + - const: csiphy2 >>> + - const: csiphy4 >>> + - const: vfe0 >>> + - const: vfe1 >>> + - const: vfe_lite0 >>> + - const: vfe_lite1 >>> + >>> + iommus: >>> + maxItems: 13 >>> + >>> + interconnects: >>> + maxItems: 4 >>> + >>> + interconnect-names: >>> + items: >>> + - const: cam_ahb >>> + - const: cam_hf_mnoc >>> + - const: cam_sf_mnoc >>> + - const: cam_sf_icp_mnoc >>> + >>> + power-domains: >>> + items: >>> + - description: IFE0 GDSC - Image Front End, Global Distributed >>> Switch Controller. >>> + - description: IFE1 GDSC - Image Front End, Global Distributed >>> Switch Controller. >>> + - description: Titan Top GDSC - Titan ISP Block, Global >>> Distributed Switch Controller. >>> + >>> + power-domain-names: >>> + items: >>> + - const: ife0 >>> + - const: ife1 >>> + - const: top >>> + >>> + ports: >>> + $ref: /schemas/graph.yaml#/properties/ports >>> + >>> + description: >>> + CSI input ports. >>> + >>> + patternProperties: >>> + "^port@[03]+$": >>> + $ref: /schemas/graph.yaml#/$defs/port-base >>> + unevaluatedProperties: false >>> + >>> + description: >>> + Input port for receiving CSI data from a CSIPHY. >>> + >>> + properties: >>> + endpoint: >>> + $ref: video-interfaces.yaml# >>> + unevaluatedProperties: false >>> + >>> + properties: >>> + clock-lanes: >>> + maxItems: 1 >>> + >>> + data-lanes: >>> + minItems: 1 >>> + maxItems: 4 >>> + >>> + required: >>> + - clock-lanes >>> + - data-lanes >>> + >>> + reg: >>> + maxItems: 12 >>> + >>> + reg-names: >>> + items: >>> + - const: csid0 >>> + - const: csid1 >>> + - const: csid2 >>> + - const: csid_wrapper >>> + - const: csiphy0 >>> + - const: csiphy1 >>> + - const: csiphy2 >>> + - const: csiphy4 >>> + - const: vfe_lite0 >>> + - const: vfe_lite1 >>> + - const: vfe0 >>> + - const: vfe1 >>> + >>> + vdda-phy-supply: >>> + description: >>> + Phandle to a 0.9V regulator supply to PHY core block. >>> + >>> + vdda-pll-supply: >>> + description: >>> + Phandle to 1.2V regulator supply to PHY refclk pll block. >> >> I believe it's very unlikely that the SoC pads are called like this, >> as we discussed it in the recent past. >> >> Please rename the properties to reflect the names inherited from >> the actual hardware. > > I believe we agreed to convert to the PHY infrastructure after 8550, > 7280 and x1e80100. > > So these names should rename as is. my ask is not related to the planned PHY conversion, it's much simpler and easily doable, just reflect the proper pad names in the property names. There is no such hardware objects on the SoC, which names can be associated to "vdda-phy" or "vdda-pll" property names. Okay, split of CSIPHY specific supplies can be done separately, but can you introduce here property names like "vdd-csiphy-0p9-supply" and "vdd-csiphy-1p2-supply"? Also you put a description like "supply to PHY refclk pll block", but if I remember correctly once you've said that the datasheet (of another SoC) does not give any clues about the usage of the supply, thus it invalidates the given description. I'm unhappy that people tend to copy defects, which are trivial to fix or avoid at least. -- Best wishes, Vladimir
On 20/11/2024 23:02, Vladimir Zapolskiy wrote: > like "vdd-csiphy-0p9-supply" and "vdd-csiphy-1p2-supply"? In theory, however I'd like to avoid adding endless strings of new names into the driver code for each different power input. We can add this additional string name though in the interim between now and refactor for the PHY API. > Also you put a description like "supply to PHY refclk pll block", but if I > remember correctly once you've said that the datasheet (of another SoC) > does not give any clues about the usage of the supply, thus it invalidates > the given description. I'm surmising by extrapolation - that's "probably" what those are just at different voltage levels based on previous iterations of this PHY. I'm just as happy not to describe this or to describe it as no mor that the 1.2v supply etc. --- bod
On 11/21/24 01:27, Bryan O'Donoghue wrote: > On 20/11/2024 23:02, Vladimir Zapolskiy wrote: >> like "vdd-csiphy-0p9-supply" and "vdd-csiphy-1p2-supply"? > > In theory, however I'd like to avoid adding endless strings of new names > into the driver code for each different power input. I don't understand this argument, it's the same degree of endlessness as the endlessness of new designed SoCs. Should it be stopped now or what's the point here? My argument is to represent the actual hardware instead of copying errors. > We can add this additional string name though in the interim between now > and refactor for the PHY API. I don't see it as a good reason to copy an easy to correct mistake. >> Also you put a description like "supply to PHY refclk pll block", but if I >> remember correctly once you've said that the datasheet (of another SoC) >> does not give any clues about the usage of the supply, thus it invalidates >> the given description. > > I'm surmising by extrapolation - that's "probably" what those are just > at different voltage levels based on previous iterations of this PHY. But this is proven to be wrong, let me kindly ask you to align with the SoC documentation here. > I'm just as happy not to describe this or to describe it as no mor that > the 1.2v supply etc. > Thank you for understanding. -- Best wishes, Vladimir
diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml new file mode 100644 index 0000000000000000000000000000000000000000..ca2499cd52a51e14bad3cf8a8ca94c9d23ed5030 --- /dev/null +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml @@ -0,0 +1,354 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/qcom,x1e80100-camss.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm X1E80100 Camera Subsystem (CAMSS) + +maintainers: + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> + +description: | + The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms. + +properties: + compatible: + const: qcom,x1e80100-camss + + clocks: + maxItems: 29 + + clock-names: + items: + - const: camnoc_rt_axi + - const: camnoc_nrt_axi + - const: core_ahb + - const: cpas_ahb + - const: cpas_fast_ahb + - const: cpas_vfe0 + - const: cpas_vfe1 + - const: cpas_vfe_lite + - const: cphy_rx_clk_src + - const: csid + - const: csid_csiphy_rx + - const: csiphy0 + - const: csiphy0_timer + - const: csiphy1 + - const: csiphy1_timer + - const: csiphy2 + - const: csiphy2_timer + - const: csiphy4 + - const: csiphy4_timer + - const: gcc_axi_hf + - const: gcc_axi_sf + - const: vfe0 + - const: vfe0_fast_ahb + - const: vfe1 + - const: vfe1_fast_ahb + - const: vfe_lite + - const: vfe_lite_ahb + - const: vfe_lite_cphy_rx + - const: vfe_lite_csid + + interrupts: + maxItems: 13 + + interrupt-names: + items: + - const: csid0 + - const: csid1 + - const: csid2 + - const: csid_lite0 + - const: csid_lite1 + - const: csiphy0 + - const: csiphy1 + - const: csiphy2 + - const: csiphy4 + - const: vfe0 + - const: vfe1 + - const: vfe_lite0 + - const: vfe_lite1 + + iommus: + maxItems: 13 + + interconnects: + maxItems: 4 + + interconnect-names: + items: + - const: cam_ahb + - const: cam_hf_mnoc + - const: cam_sf_mnoc + - const: cam_sf_icp_mnoc + + power-domains: + items: + - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller. + - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller. + - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller. + + power-domain-names: + items: + - const: ife0 + - const: ife1 + - const: top + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + description: + CSI input ports. + + patternProperties: + "^port@[03]+$": + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + + description: + Input port for receiving CSI data from a CSIPHY. + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + clock-lanes: + maxItems: 1 + + data-lanes: + minItems: 1 + maxItems: 4 + + required: + - clock-lanes + - data-lanes + + reg: + maxItems: 12 + + reg-names: + items: + - const: csid0 + - const: csid1 + - const: csid2 + - const: csid_wrapper + - const: csiphy0 + - const: csiphy1 + - const: csiphy2 + - const: csiphy4 + - const: vfe_lite0 + - const: vfe_lite1 + - const: vfe0 + - const: vfe1 + + vdda-phy-supply: + description: + Phandle to a 0.9V regulator supply to PHY core block. + + vdda-pll-supply: + description: + Phandle to 1.2V regulator supply to PHY refclk pll block. + +required: + - clock-names + - clocks + - compatible + - interconnects + - interconnect-names + - interrupts + - interrupt-names + - iommus + - ports + - power-domains + - power-domain-names + - reg + - reg-names + - vdda-phy-supply + - vdda-pll-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/qcom,x1e80100-gcc.h> + #include <dt-bindings/clock/qcom,x1e80100-camcc.h> + #include <dt-bindings/interconnect/qcom,x1e80100-rpmh.h> + #include <dt-bindings/power/qcom-rpmpd.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + camss: camss@ac62000 { + compatible = "qcom,x1e80100-camss"; + + reg = <0 0x0acb7000 0 0x2000>, + <0 0x0acb9000 0 0x2000>, + <0 0x0acbb000 0 0x2000>, + <0 0x0acb6000 0 0x1000>, + <0 0x0ace4000 0 0x1000>, + <0 0x0ace6000 0 0x1000>, + <0 0x0ace8000 0 0x1000>, + <0 0x0acec000 0 0x4000>, + <0 0x0acc7000 0 0x2000>, + <0 0x0accb000 0 0x2000>, + <0 0x0ac62000 0 0x2a00>, + <0 0x0ac71000 0 0x2a00>; + + reg-names = "csid0", + "csid1", + "csid2", + "csid_wrapper", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy4", + "vfe_lite0", + "vfe_lite1", + "vfe0", + "vfe1"; + + vdda-phy-supply = <&csiphy0_vdda_phy_supply>; + vdda-pll-supply = <&csiphy0_vdda_pll_supply>; + + interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 431 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 359 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 122 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-names = "csid0", + "csid1", + "csid2", + "csid_lite0", + "csid_lite1", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy4", + "vfe0", + "vfe1", + "vfe_lite0", + "vfe_lite1"; + + power-domains = <&camcc CAM_CC_IFE_0_GDSC>, + <&camcc CAM_CC_IFE_1_GDSC>, + <&camcc CAM_CC_TITAN_TOP_GDSC>; + + power-domain-names = "ife0", + "ife1", + "top"; + + clocks = <&camcc CAM_CC_CAMNOC_AXI_RT_CLK>, + <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>, + <&camcc CAM_CC_CORE_AHB_CLK>, + <&camcc CAM_CC_CPAS_AHB_CLK>, + <&camcc CAM_CC_CPAS_FAST_AHB_CLK>, + <&camcc CAM_CC_CPAS_IFE_0_CLK>, + <&camcc CAM_CC_CPAS_IFE_1_CLK>, + <&camcc CAM_CC_CPAS_IFE_LITE_CLK>, + <&camcc CAM_CC_CPHY_RX_CLK_SRC>, + <&camcc CAM_CC_CSID_CLK>, + <&camcc CAM_CC_CSID_CSIPHY_RX_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_CSIPHY4_CLK>, + <&camcc CAM_CC_CSI4PHYTIMER_CLK>, + <&gcc GCC_CAMERA_HF_AXI_CLK>, + <&gcc GCC_CAMERA_SF_AXI_CLK>, + <&camcc CAM_CC_IFE_0_CLK>, + <&camcc CAM_CC_IFE_0_FAST_AHB_CLK>, + <&camcc CAM_CC_IFE_1_CLK>, + <&camcc CAM_CC_IFE_1_FAST_AHB_CLK>, + <&camcc CAM_CC_IFE_LITE_CLK>, + <&camcc CAM_CC_IFE_LITE_AHB_CLK>, + <&camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_LITE_CSID_CLK>; + + clock-names = "camnoc_rt_axi", + "camnoc_nrt_axi", + "core_ahb", + "cpas_ahb", + "cpas_fast_ahb", + "cpas_vfe0", + "cpas_vfe1", + "cpas_vfe_lite", + "cphy_rx_clk_src", + "csid", + "csid_csiphy_rx", + "csiphy0", + "csiphy0_timer", + "csiphy1", + "csiphy1_timer", + "csiphy2", + "csiphy2_timer", + "csiphy4", + "csiphy4_timer", + "gcc_axi_hf", + "gcc_axi_sf", + "vfe0", + "vfe0_fast_ahb", + "vfe1", + "vfe1_fast_ahb", + "vfe_lite", + "vfe_lite_ahb", + "vfe_lite_cphy_rx", + "vfe_lite_csid"; + + iommus = <&apps_smmu 0x800 0x60>, + <&apps_smmu 0x820 0x60>, + <&apps_smmu 0x840 0x60>, + <&apps_smmu 0x860 0x60>, + <&apps_smmu 0x1800 0x60>, + <&apps_smmu 0x1820 0x60>, + <&apps_smmu 0x1840 0x60>, + <&apps_smmu 0x1860 0x60>, + <&apps_smmu 0x18a0 0x00>, + <&apps_smmu 0x18e0 0x00>, + <&apps_smmu 0x1980 0x20>, + <&apps_smmu 0x1900 0x00>, + <&apps_smmu 0x19a0 0x20>; + + 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_SF 0 &mc_virt SLAVE_EBI1 0>, + <&mmss_noc MASTER_CAMNOC_ICP 0 &mc_virt SLAVE_EBI1 0>; + interconnect-names = "cam_ahb", + "cam_hf_mnoc", + "cam_sf_mnoc", + "cam_sf_icp_mnoc"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + csiphy_ep0: endpoint { + clock-lanes = <7>; + data-lanes = <0 1>; + remote-endpoint = <&sensor_ep>; + }; + }; + }; + }; + };
Add bindings for qcom,x1e80100-camss in order to support the camera subsystem for x1e80100 as found in various Co-Pilot laptops. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- .../bindings/media/qcom,x1e80100-camss.yaml | 354 +++++++++++++++++++++ 1 file changed, 354 insertions(+)