| Message ID | 20230417055627.16482-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | arm64: dts: imx8mp: Enable CSIS and ISI in DT | expand |
Hi Marco, On Mon, Apr 17, 2023 at 08:50:59AM +0200, Marco Felsch wrote: > Hi Laurent, > > your patch LGTM just one nit/idea, please see below. > > On 23-04-17, Laurent Pinchart wrote: > > Add DT nodes for the two CSI-2 receivers of the i.MX8MP. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 60 +++++++++++++++++++++++ > > 1 file changed, 60 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > index 2dd60e3252f3..2a374a4c14a2 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > @@ -1239,6 +1239,66 @@ ldb_lvds_ch1: endpoint { > > }; > > }; > > > > + mipi_csi_0: csi@32e40000 { > > + compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > + reg = <0x32e40000 0x10000>; > > + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; > > + clock-frequency = <500000000>; > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > + clock-names = "pclk", "wrap", "phy", "axi"; > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > + assigned-clock-rates = <500000000>; > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > If we would add: > mipi_csi_0_in: endpoint {}; > > here we could refernce it from overlays/board dts files more easily. Isn't there an unwritten rule (or consensus) that an endpoint should always have a remote-endpoint property ? While ports describe hardware properties of a device and should always be there regardless of connections, endpoints describe connections and I don't think they should be instantiated with a valid remote-endpoint. > > + }; > > + > > + port@1 { > > + reg = <1>; > > + }; > > + }; > > + }; > > + > > + mipi_csi_1: csi@32e50000 { > > + compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > + reg = <0x32e50000 0x10000>; > > + interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>; > > + clock-frequency = <266000000>; > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > + clock-names = "pclk", "wrap", "phy", "axi"; > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM2_PIX>; > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > + assigned-clock-rates = <266000000>; > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + }; > > + }; > > + }; > > + > > pcie_phy: pcie-phy@32f00000 { > > compatible = "fsl,imx8mp-pcie-phy"; > > reg = <0x32f00000 0x10000>;
Hi Marco, On Mon, Apr 17, 2023 at 10:01:17AM +0200, Marco Felsch wrote: > On 23-04-17, Laurent Pinchart wrote: > > On Mon, Apr 17, 2023 at 08:50:59AM +0200, Marco Felsch wrote: > > > Hi Laurent, > > > > > > your patch LGTM just one nit/idea, please see below. > > > > > > On 23-04-17, Laurent Pinchart wrote: > > > > Add DT nodes for the two CSI-2 receivers of the i.MX8MP. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 60 +++++++++++++++++++++++ > > > > 1 file changed, 60 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > index 2dd60e3252f3..2a374a4c14a2 100644 > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > @@ -1239,6 +1239,66 @@ ldb_lvds_ch1: endpoint { > > > > }; > > > > }; > > > > > > > > + mipi_csi_0: csi@32e40000 { > > > > + compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > > > + reg = <0x32e40000 0x10000>; > > > > + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; > > > > + clock-frequency = <500000000>; > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > > > + clock-names = "pclk", "wrap", "phy", "axi"; > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > > > + assigned-clock-rates = <500000000>; > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>; > > > > + status = "disabled"; > > > > + > > > > + ports { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + port@0 { > > > > + reg = <0>; > > > > > > If we would add: > > > mipi_csi_0_in: endpoint {}; > > > > > > here we could refernce it from overlays/board dts files more easily. > > > > Isn't there an unwritten rule (or consensus) that an endpoint should > > always have a remote-endpoint property ? > > I don't know if there is one. > > > While ports describe hardware properties of a device and should always > > be there regardless of connections, endpoints describe connections and > > I don't think they should be instantiated with a valid > > remote-endpoint. > > I know, therefore I mentioned it as idea to make it 'easier' to add > camera nodes. As a middleground, would it be useful to have a label for the port ? Something like mipi_csi_0: csi@32e40000 { ports { mipi_csi_0_port_0: port@0 { }; }; }; An overlay could then reference that and create the endpoint. I'm not entirely sure how useful that would be though, as the overlay would need to enable the CSI node anyway. Compare -------- &mipi_csi_0 { status = "okay"; }; &mipi_csi_0_port_0 { mipi_csi_0_in: endpoint { remote-endpoint = <&imx327_out>; }; }; -------- with -------- &mipi_csi_0 { status = "okay"; ports { port@0 { mipi_csi_0_in: endpoint { remote-endpoint = <&imx327_out>; }; }; }; }; -------- I have a slight preference for the latter as it groups all the CSI0 data in a single overlay target, but if the former is generally preferred, I'm fine with that too. > > > > + }; > > > > + > > > > + port@1 { > > > > + reg = <1>; > > > > + }; > > > > + }; > > > > + }; > > > > + > > > > + mipi_csi_1: csi@32e50000 { > > > > + compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > > > + reg = <0x32e50000 0x10000>; > > > > + interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>; > > > > + clock-frequency = <266000000>; > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > > > + clock-names = "pclk", "wrap", "phy", "axi"; > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM2_PIX>; > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > > > + assigned-clock-rates = <266000000>; > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>; > > > > + status = "disabled"; > > > > + > > > > + ports { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + port@0 { > > > > + reg = <0>; > > > > + }; > > > > + > > > > + port@1 { > > > > + reg = <1>; > > > > + }; > > > > + }; > > > > + }; > > > > + > > > > pcie_phy: pcie-phy@32f00000 { > > > > compatible = "fsl,imx8mp-pcie-phy"; > > > > reg = <0x32f00000 0x10000>;
On 23-04-17, Laurent Pinchart wrote: > Hi Marco, > > On Mon, Apr 17, 2023 at 10:01:17AM +0200, Marco Felsch wrote: > > On 23-04-17, Laurent Pinchart wrote: > > > On Mon, Apr 17, 2023 at 08:50:59AM +0200, Marco Felsch wrote: > > > > Hi Laurent, > > > > > > > > your patch LGTM just one nit/idea, please see below. > > > > > > > > On 23-04-17, Laurent Pinchart wrote: > > > > > Add DT nodes for the two CSI-2 receivers of the i.MX8MP. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 60 +++++++++++++++++++++++ > > > > > 1 file changed, 60 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > index 2dd60e3252f3..2a374a4c14a2 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > @@ -1239,6 +1239,66 @@ ldb_lvds_ch1: endpoint { > > > > > }; > > > > > }; > > > > > > > > > > + mipi_csi_0: csi@32e40000 { > > > > > + compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > > > > + reg = <0x32e40000 0x10000>; > > > > > + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clock-frequency = <500000000>; > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > > > > + clock-names = "pclk", "wrap", "phy", "axi"; > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > > > > + assigned-clock-rates = <500000000>; > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>; > > > > > + status = "disabled"; > > > > > + > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + port@0 { > > > > > + reg = <0>; > > > > > > > > If we would add: > > > > mipi_csi_0_in: endpoint {}; > > > > > > > > here we could refernce it from overlays/board dts files more easily. > > > > > > Isn't there an unwritten rule (or consensus) that an endpoint should > > > always have a remote-endpoint property ? > > > > I don't know if there is one. > > > > > While ports describe hardware properties of a device and should always > > > be there regardless of connections, endpoints describe connections and > > > I don't think they should be instantiated with a valid > > > remote-endpoint. > > > > I know, therefore I mentioned it as idea to make it 'easier' to add > > camera nodes. > > As a middleground, would it be useful to have a label for the port ? > Something like > > mipi_csi_0: csi@32e40000 { > ports { > mipi_csi_0_port_0: port@0 { > }; > }; > }; > > An overlay could then reference that and create the endpoint. I'm not > entirely sure how useful that would be though, as the overlay would need > to enable the CSI node anyway. Compare Argh.. you're right, the node isn't enabled too. So the only useful use-case for my idea would be: a base-dts which enables the mipi-csi node always (since the products do camera support) and the overlay would only need to reference the port or endpoint phandle. I'm not sure if this use-case is very often. Please ignore it. > -------- > &mipi_csi_0 { > status = "okay"; > }; > > &mipi_csi_0_port_0 { > mipi_csi_0_in: endpoint { > remote-endpoint = <&imx327_out>; > }; > }; > -------- > > with > > -------- > &mipi_csi_0 { > status = "okay"; > > ports { > port@0 { > mipi_csi_0_in: endpoint { > remote-endpoint = <&imx327_out>; > }; > }; > }; > }; > -------- > > I have a slight preference for the latter as it groups all the CSI0 data > in a single overlay target, but if the former is generally preferred, > I'm fine with that too. > > > > > > + }; > > > > > + > > > > > + port@1 { > > > > > + reg = <1>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > + mipi_csi_1: csi@32e50000 { > > > > > + compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > > > > + reg = <0x32e50000 0x10000>; > > > > > + interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clock-frequency = <266000000>; > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > > > > + clock-names = "pclk", "wrap", "phy", "axi"; > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM2_PIX>; > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > > > > + assigned-clock-rates = <266000000>; > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>; > > > > > + status = "disabled"; > > > > > + > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + port@0 { > > > > > + reg = <0>; > > > > > + }; > > > > > + > > > > > + port@1 { > > > > > + reg = <1>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > pcie_phy: pcie-phy@32f00000 { > > > > > compatible = "fsl,imx8mp-pcie-phy"; > > > > > reg = <0x32f00000 0x10000>; > > -- > Regards, > > Laurent Pinchart >
Am Montag, 17. April 2023, 10:15:10 CEST schrieb Laurent Pinchart: > Hi Marco, > > On Mon, Apr 17, 2023 at 10:01:17AM +0200, Marco Felsch wrote: > > On 23-04-17, Laurent Pinchart wrote: > > > On Mon, Apr 17, 2023 at 08:50:59AM +0200, Marco Felsch wrote: > > > > Hi Laurent, > > > > > > > > your patch LGTM just one nit/idea, please see below. > > > > > > > > On 23-04-17, Laurent Pinchart wrote: > > > > > Add DT nodes for the two CSI-2 receivers of the i.MX8MP. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > > > > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 60 > > > > > +++++++++++++++++++++++ > > > > > 1 file changed, 60 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index > > > > > 2dd60e3252f3..2a374a4c14a2 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > @@ -1239,6 +1239,66 @@ ldb_lvds_ch1: endpoint { > > > > > > > > > > }; > > > > > > > > > > }; > > > > > > > > > > + mipi_csi_0: csi@32e40000 { > > > > > + compatible = "fsl,imx8mp- mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > > > > + reg = <0x32e40000 0x10000>; > > > > > + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clock-frequency = <500000000>; > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > > > > + clock-names = "pclk", "wrap", "phy", "axi"; > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > > > > + assigned-clock-rates = <500000000>; > > > > > + power-domains = <&media_blk_ctrl > > > > > IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>; > > > > > + status = "disabled"; > > > > > + > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + port@0 { > > > > > + reg = <0>; > > > > > > > > If we would add: > > > > mipi_csi_0_in: endpoint {}; > > > > > > > > here we could refernce it from overlays/board dts files more easily. > > > > > > Isn't there an unwritten rule (or consensus) that an endpoint should > > > always have a remote-endpoint property ? > > > > I don't know if there is one. > > > > > While ports describe hardware properties of a device and should always > > > be there regardless of connections, endpoints describe connections and > > > I don't think they should be instantiated with a valid > > > remote-endpoint. > > > > I know, therefore I mentioned it as idea to make it 'easier' to add > > camera nodes. > > As a middleground, would it be useful to have a label for the port ? > Something like > > mipi_csi_0: csi@32e40000 { > ports { > mipi_csi_0_port_0: port@0 { > }; > }; > }; > > An overlay could then reference that and create the endpoint. I'm not > entirely sure how useful that would be though, as the overlay would need > to enable the CSI node anyway. Compare > > -------- > &mipi_csi_0 { > status = "okay"; > }; > > &mipi_csi_0_port_0 { > mipi_csi_0_in: endpoint { > remote-endpoint = <&imx327_out>; > }; > }; > -------- > > with > > -------- > &mipi_csi_0 { > status = "okay"; > > ports { > port@0 { > mipi_csi_0_in: endpoint { > remote-endpoint = <&imx327_out>; > }; > }; > }; > }; > -------- > > I have a slight preference for the latter as it groups all the CSI0 data > in a single overlay target, but if the former is generally preferred, > I'm fine with that too. The former is more compact, but also raises the following dtc warnings while creating the .dtbo: Warning (graph_endpoint): /fragment@4/__overlay__: graph endpoint node name should be 'endpoint' Warning (graph_endpoint): /fragment@4/__overlay__: graph connection to node '/ fragment@1/__overlay__/ports/port@1/endpoint' is not bidirectional for the following snippet: &mipi_csi_0_out { remote-endpoint = <&isp1_in>; }; I'm not sure if there is a chance to fix at all. Best regards, Alexander > > > > > > + }; > > > > > + > > > > > + port@1 { > > > > > + reg = <1>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > + mipi_csi_1: csi@32e50000 { > > > > > + compatible = "fsl,imx8mp- mipi-csi2", "fsl,imx8mm-mipi-csi2"; > > > > > + reg = <0x32e50000 0x10000>; > > > > > + interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clock-frequency = <266000000>; > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>; > > > > > + clock-names = "pclk", "wrap", "phy", "axi"; > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM2_PIX>; > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > > > > + assigned-clock-rates = <266000000>; > > > > > + power-domains = <&media_blk_ctrl > > > > > IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>; > > > > > + status = "disabled"; > > > > > + > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + port@0 { > > > > > + reg = <0>; > > > > > + }; > > > > > + > > > > > + port@1 { > > > > > + reg = <1>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > > > > > > pcie_phy: pcie-phy@32f00000 { > > > > > > > > > > compatible = "fsl,imx8mp- pcie-phy"; > > > > > reg = <0x32f00000 0x10000>;
On Mon, Apr 17, 2023 at 08:56:25AM +0300, Laurent Pinchart wrote: > Hello, > > This small patch series adds the CSIS and ISI devices in the i.MX8MP DT > to support cameras. The ISI DT bindings have just been merged and will > appear in v6.4, making this series a candidate for v6.5. > > With these two patches, a board overlay can enable camera support by > instantiating the camera sensor, connecting it to a CSIS instance, and > enabling the CSIS and ISI nodes. The camera pipeline is supported by > V4L2 drivers. > > Laurent Pinchart (2): > arm64: dts: imx8mp: Add CSIS DT nodes > arm64: dts: imx8mp: Add ISI DT node For both, Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 98 +++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > > > base-commit: 20af6be6bee4c3af80aac9b44b3d32d89824dde7
On Mon, Apr 17, 2023 at 6:16 AM Marco Felsch <m.felsch@pengutronix.de> wrote: > > On 23-04-17, Adam Ford wrote: > > ... > > > > > > > > If we would add: > > > > > > > mipi_csi_0_in: > > > endpoint {}; > > > > > > > > > > > > > > here we could refernce it from overlays/board dts files more easily. > > > > > > > > > > > > Isn't there an unwritten rule (or consensus) that an endpoint should > > > > > > always have a remote-endpoint property ? > > > > > > > > > > I don't know if there is one. > > > > > > > > > > > While ports describe hardware properties of a device and should always > > > > > > be there regardless of connections, endpoints describe connections and > > > > > > I don't think they should be instantiated with a valid > > > > > > remote-endpoint. > > > > > > > > > > I know, therefore I mentioned it as idea to make it 'easier' to add > > > > > camera nodes. > > > > > > > > As a middleground, would it be useful to have a label for the port ? > > > > Something like > > > > > > > > mipi_csi_0: csi@32e40000 { > > > > ports { > > > > mipi_csi_0_port_0: port@0 { > > > > }; > > > > }; > > > > }; > > > > > > > > An overlay could then reference that and create the endpoint. I'm not > > > > entirely sure how useful that would be though, as the overlay would need > > > > to enable the CSI node anyway. Compare > > > > > > > > -------- > > > > &mipi_csi_0 { > > > > status = "okay"; > > > > }; > > > > > > > > &mipi_csi_0_port_0 { > > > > mipi_csi_0_in: endpoint { > > > > remote-endpoint = <&imx327_out>; > > > > }; > > > > }; > > > > -------- > > > > > > > > with > > > > > > > > -------- > > > > &mipi_csi_0 { > > > > status = "okay"; > > > > > > > > ports { > > > > port@0 { > > > > mipi_csi_0_in: endpoint { > > > > remote-endpoint = <&imx327_out>; > > > > }; > > > > }; > > > > }; > > > > }; > > > > -------- > > > > > > > > I have a slight preference for the latter as it groups all the CSI0 data > > > > in a single overlay target, but if the former is generally preferred, > > > > I'm fine with that too. > > > > > > The former is more compact, but also raises the following dtc warnings while > > > creating the .dtbo: > > > Warning (graph_endpoint): /fragment@4/__overlay__: graph endpoint node name > > > should be 'endpoint' > > > Warning (graph_endpoint): /fragment@4/__overlay__: graph connection to node '/ > > > fragment@1/__overlay__/ports/port@1/endpoint' is not bidirectional > > > > > > for the following snippet: > > > > > > &mipi_csi_0_out { > > > remote-endpoint = <&isp1_in>; > > > }; > > > > > > I'm not sure if there is a chance to fix at all. > > > > Once there is consensus on how this should be generically plumbed, > > please keep me in the loop, so I can add the corresponding imx8m Nano > > trees as well. I've tested Laurent's work for a while on the Nano > > that I have. I was going to push DT updates for Nano, then I saw this > > conversation, so I decided to hold off for now. > > This was just an idea nothing serious. Maybe Krzysztof have a strong > opinion on that. > > Regards, > Marco > Hi Laurent, Is there any consensus on this yet? I have a imx219 camera attached to an imx8mp-venice-gw74xx that I'm trying to figure out how to connect up via an overlay to test it. Best regards, Tim