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@xxxxxxxxxx> ---.../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.yamlnew file mode 100644index 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@xxxxxxxxxx> + +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_timerWhat 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.
+ +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>,As usual, and at no surprise, there is an immediate problem with the incorrespondent unit address.
Actually you're right git show c830aff08d51f8391e59fc6744757c58e320b41b this shoiuld be "vfe0" first and then sorted alphabetically by IP. :(
+ <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>;It's unclear why #address-cells/#size-cells are needed here.
Because the checker wants it. I'll check again to be sure.
+ + csiphy_ep0: endpoint { + clock-lanes = <7>;As it's known, there is no lane 7.
Yes true this should be five. It works because this value is ignored by the driver.
--- bod
