[PATCH v1 0/2] arm64: dts: imx8mp: Enable CSIS and ISI in DT

[PATCH v1 0/2] arm64: dts: imx8mp: Enable CSIS and ISI in DT

[Laurent Pinchart]

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

 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 98 +++++++++++++++++++++++
 1 file changed, 98 insertions(+)


base-commit: 20af6be6bee4c3af80aac9b44b3d32d89824dde7
-- 
Regards,

Laurent Pinchart

[PATCH v1 0/2] arm64: dts: imx8mp: Enable CSIS and ISI in DT

[Laurent Pinchart]

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

 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 98 +++++++++++++++++++++++
 1 file changed, 98 insertions(+)


base-commit: 20af6be6bee4c3af80aac9b44b3d32d89824dde7
-- 
Regards,

Laurent Pinchart


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Laurent Pinchart]

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

[PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Laurent Pinchart]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/2] arm64: dts: imx8mp: Add ISI DT node

[Laurent Pinchart]

Add a DT node for the i.MX8MP ISI instance, and model to connection to
two CSI-2 receivers (CSIS).

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 38 +++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 2a374a4c14a2..cc7a938b6f73 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1239,6 +1239,38 @@ ldb_lvds_ch1: endpoint {
 				};
 			};
 
+			isi_0: isi@32e00000 {
+				compatible = "fsl,imx8mp-isi";
+				reg = <0x32e00000 0x4000>;
+				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>,
+					 <&clk IMX8MP_CLK_MEDIA_APB_ROOT>;
+				clock-names = "axi", "apb";
+				fsl,blk-ctrl = <&media_blk_ctrl>;
+				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISI>;
+				status = "disabled";
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						isi_in_0: endpoint {
+							remote-endpoint = <&mipi_csi_0_out>;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+						isi_in_1: endpoint {
+							remote-endpoint = <&mipi_csi_1_out>;
+						};
+					};
+				};
+			};
+
 			mipi_csi_0: csi@32e40000 {
 				compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2";
 				reg = <0x32e40000 0x10000>;
@@ -1265,6 +1297,9 @@ port@0 {
 
 					port@1 {
 						reg = <1>;
+						mipi_csi_0_out: endpoint {
+							remote-endpoint = <&isi_in_0>;
+						};
 					};
 				};
 			};
@@ -1295,6 +1330,9 @@ port@0 {
 
 					port@1 {
 						reg = <1>;
+						mipi_csi_1_out: endpoint {
+							remote-endpoint = <&isi_in_1>;
+						};
 					};
 				};
 			};
-- 
Regards,

Laurent Pinchart

[PATCH v1 2/2] arm64: dts: imx8mp: Add ISI DT node

[Laurent Pinchart]

Add a DT node for the i.MX8MP ISI instance, and model to connection to
two CSI-2 receivers (CSIS).

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 38 +++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 2a374a4c14a2..cc7a938b6f73 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1239,6 +1239,38 @@ ldb_lvds_ch1: endpoint {
 				};
 			};
 
+			isi_0: isi@32e00000 {
+				compatible = "fsl,imx8mp-isi";
+				reg = <0x32e00000 0x4000>;
+				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>,
+					 <&clk IMX8MP_CLK_MEDIA_APB_ROOT>;
+				clock-names = "axi", "apb";
+				fsl,blk-ctrl = <&media_blk_ctrl>;
+				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISI>;
+				status = "disabled";
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						isi_in_0: endpoint {
+							remote-endpoint = <&mipi_csi_0_out>;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+						isi_in_1: endpoint {
+							remote-endpoint = <&mipi_csi_1_out>;
+						};
+					};
+				};
+			};
+
 			mipi_csi_0: csi@32e40000 {
 				compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2";
 				reg = <0x32e40000 0x10000>;
@@ -1265,6 +1297,9 @@ port@0 {
 
 					port@1 {
 						reg = <1>;
+						mipi_csi_0_out: endpoint {
+							remote-endpoint = <&isi_in_0>;
+						};
 					};
 				};
 			};
@@ -1295,6 +1330,9 @@ port@0 {
 
 					port@1 {
 						reg = <1>;
+						mipi_csi_1_out: endpoint {
+							remote-endpoint = <&isi_in_1>;
+						};
 					};
 				};
 			};
-- 
Regards,

Laurent Pinchart


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/2] arm64: dts: imx8mp: Add ISI DT node

[Alexander Stein]

Hi Laurent,

thanks for those ISI patches.

Am Montag, 17. April 2023, 07:56:27 CEST schrieb Laurent Pinchart:
> Add a DT node for the i.MX8MP ISI instance, and model to connection to
> two CSI-2 receivers (CSIS).
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 38 +++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index
> 2a374a4c14a2..cc7a938b6f73 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1239,6 +1239,38 @@ ldb_lvds_ch1: endpoint {
>  				};
>  			};
> 
> +			isi_0: isi@32e00000 {
> +				compatible = "fsl,imx8mp-isi";
> +				reg = <0x32e00000 0x4000>;
> +				interrupts = <GIC_SPI 16 
IRQ_TYPE_LEVEL_HIGH>,
> +					     <GIC_SPI 42 
IRQ_TYPE_LEVEL_HIGH>;
> +				clocks = <&clk 
IMX8MP_CLK_MEDIA_AXI_ROOT>,
> +					 <&clk 
IMX8MP_CLK_MEDIA_APB_ROOT>;
> +				clock-names = "axi", "apb";
> +				fsl,blk-ctrl = <&media_blk_ctrl>;
> +				power-domains = <&media_blk_ctrl 
IMX8MP_MEDIABLK_PD_ISI>;
> +				status = "disabled";
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					port@0 {
> +						reg = <0>;
> +						isi_in_0: endpoint 
{

I'm not sure if DT graph is different, but usually there is an empty line 
between properties and subnodes.

Best regards,
Alexander

> +							remote-
endpoint = <&mipi_csi_0_out>;
> +						};
> +					};
> +
> +					port@1 {
> +						reg = <1>;
> +						isi_in_1: endpoint 
{
> +							remote-
endpoint = <&mipi_csi_1_out>;
> +						};
> +					};
> +				};
> +			};
> +
>  			mipi_csi_0: csi@32e40000 {
>  				compatible = "fsl,imx8mp-mipi-csi2", 
"fsl,imx8mm-mipi-csi2";
>  				reg = <0x32e40000 0x10000>;
> @@ -1265,6 +1297,9 @@ port@0 {
> 
>  					port@1 {
>  						reg = <1>;
> +						mipi_csi_0_out: 
endpoint {
> +							remote-
endpoint = <&isi_in_0>;
> +						};
>  					};
>  				};
>  			};
> @@ -1295,6 +1330,9 @@ port@0 {
> 
>  					port@1 {
>  						reg = <1>;
> +						mipi_csi_1_out: 
endpoint {
> +							remote-
endpoint = <&isi_in_1>;
> +						};
>  					};
>  				};
>  			};


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v1 2/2] arm64: dts: imx8mp: Add ISI DT node

[Alexander Stein]

Hi Laurent,

thanks for those ISI patches.

Am Montag, 17. April 2023, 07:56:27 CEST schrieb Laurent Pinchart:
> Add a DT node for the i.MX8MP ISI instance, and model to connection to
> two CSI-2 receivers (CSIS).
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 38 +++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index
> 2a374a4c14a2..cc7a938b6f73 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1239,6 +1239,38 @@ ldb_lvds_ch1: endpoint {
>  				};
>  			};
> 
> +			isi_0: isi@32e00000 {
> +				compatible = "fsl,imx8mp-isi";
> +				reg = <0x32e00000 0x4000>;
> +				interrupts = <GIC_SPI 16 
IRQ_TYPE_LEVEL_HIGH>,
> +					     <GIC_SPI 42 
IRQ_TYPE_LEVEL_HIGH>;
> +				clocks = <&clk 
IMX8MP_CLK_MEDIA_AXI_ROOT>,
> +					 <&clk 
IMX8MP_CLK_MEDIA_APB_ROOT>;
> +				clock-names = "axi", "apb";
> +				fsl,blk-ctrl = <&media_blk_ctrl>;
> +				power-domains = <&media_blk_ctrl 
IMX8MP_MEDIABLK_PD_ISI>;
> +				status = "disabled";
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					port@0 {
> +						reg = <0>;
> +						isi_in_0: endpoint 
{

I'm not sure if DT graph is different, but usually there is an empty line 
between properties and subnodes.

Best regards,
Alexander

> +							remote-
endpoint = <&mipi_csi_0_out>;
> +						};
> +					};
> +
> +					port@1 {
> +						reg = <1>;
> +						isi_in_1: endpoint 
{
> +							remote-
endpoint = <&mipi_csi_1_out>;
> +						};
> +					};
> +				};
> +			};
> +
>  			mipi_csi_0: csi@32e40000 {
>  				compatible = "fsl,imx8mp-mipi-csi2", 
"fsl,imx8mm-mipi-csi2";
>  				reg = <0x32e40000 0x10000>;
> @@ -1265,6 +1297,9 @@ port@0 {
> 
>  					port@1 {
>  						reg = <1>;
> +						mipi_csi_0_out: 
endpoint {
> +							remote-
endpoint = <&isi_in_0>;
> +						};
>  					};
>  				};
>  			};
> @@ -1295,6 +1330,9 @@ port@0 {
> 
>  					port@1 {
>  						reg = <1>;
> +						mipi_csi_1_out: 
endpoint {
> +							remote-
endpoint = <&isi_in_1>;
> +						};
>  					};
>  				};
>  			};


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

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.

Regards,
  Marco

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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

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.

Regards,
  Marco

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

On 23-04-17, Laurent Pinchart wrote:
> 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 ?

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.

Regards,
  Marco

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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

On 23-04-17, Laurent Pinchart wrote:
> 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 ?

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.

Regards,
  Marco

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[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.

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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

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
> 

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Alexander Stein]

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


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Alexander Stein]

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


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Adam Ford]

On Mon, Apr 17, 2023 at 5:11 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> 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.

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.

adam
>
> 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>;
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Adam Ford]

On Mon, Apr 17, 2023 at 5:11 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> 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.

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.

adam
>
> 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>;
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Marco Felsch]

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

Re: [PATCH v1 0/2] arm64: dts: imx8mp: Enable CSIS and ISI in DT

[Paul Elder]

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

Re: [PATCH v1 0/2] arm64: dts: imx8mp: Enable CSIS and ISI in DT

[Paul Elder]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Tim Harvey]

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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Tim Harvey]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Adam Ford]

On Mon, Apr 17, 2023 at 1:01 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Add DT nodes for the two CSI-2 receivers of the i.MX8MP.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

Laurent,

Sorry to dig up an old thread, but I have a concern about the clock
ratings and nominal mode vs overdrive mode.  I started investigating
the different data sheets amongst the various imx8m[mnp] families to
make the default device trees run at nominal mode while also creating
a separate dtsi file with settings for overdrive so boards who use it
can include them without having to duplicate the clock settings for
everyone who supports overdrive.

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

According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
500MHz is listed as single-camera, overdrive mode.  Single-camera,
nominal mode is 400MHz, but there is more...
If configured for dual cameras, both CSI can only support up to
266MHz, but we have partially configured both albeit without the
actual camera sensors connected.

> +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> +                               status = "disabled";
> +
> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       port@0 {
> +                                               reg = <0>;
> +                                       };
> +
> +                                       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>;

266MHz is correct for dual camera, but in single camera, the second
CSI is capable of 277MHz.

At a minimum, I'd like to fix the overdrive frequency to nominal, but
since we're plumbing in both cameras, I wonder if it would be better
to run both at 266MHz with a note on CSI0 that states it could run at
400 or 500 if the second CSI is disabled and a note on the second CSI
that it could run at 277 when the first one is disabled? What are your
thoughts?


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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Adam Ford]

On Mon, Apr 17, 2023 at 1:01 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Add DT nodes for the two CSI-2 receivers of the i.MX8MP.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

Laurent,

Sorry to dig up an old thread, but I have a concern about the clock
ratings and nominal mode vs overdrive mode.  I started investigating
the different data sheets amongst the various imx8m[mnp] families to
make the default device trees run at nominal mode while also creating
a separate dtsi file with settings for overdrive so boards who use it
can include them without having to duplicate the clock settings for
everyone who supports overdrive.

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

According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
500MHz is listed as single-camera, overdrive mode.  Single-camera,
nominal mode is 400MHz, but there is more...
If configured for dual cameras, both CSI can only support up to
266MHz, but we have partially configured both albeit without the
actual camera sensors connected.

> +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> +                               status = "disabled";
> +
> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       port@0 {
> +                                               reg = <0>;
> +                                       };
> +
> +                                       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>;

266MHz is correct for dual camera, but in single camera, the second
CSI is capable of 277MHz.

At a minimum, I'd like to fix the overdrive frequency to nominal, but
since we're plumbing in both cameras, I wonder if it would be better
to run both at 266MHz with a note on CSI0 that states it could run at
400 or 500 if the second CSI is disabled and a note on the second CSI
that it could run at 277 when the first one is disabled? What are your
thoughts?


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Laurent Pinchart]

Hi Adam,

(CC'ing Kieran)

On Tue, Nov 28, 2023 at 09:17:51PM -0600, Adam Ford wrote:
> On Mon, Apr 17, 2023 at 1:01 AM 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>
> > ---
> 
> Laurent,
> 
> Sorry to dig up an old thread, but I have a concern about the clock
> ratings and nominal mode vs overdrive mode.  I started investigating
> the different data sheets amongst the various imx8m[mnp] families to
> make the default device trees run at nominal mode while also creating
> a separate dtsi file with settings for overdrive so boards who use it
> can include them without having to duplicate the clock settings for
> everyone who supports overdrive.
> 
> >  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>;
> 
> According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
> 500MHz is listed as single-camera, overdrive mode.  Single-camera,
> nominal mode is 400MHz, but there is more...
> If configured for dual cameras, both CSI can only support up to
> 266MHz, but we have partially configured both albeit without the
> actual camera sensors connected.
> 
> > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > +                               status = "disabled";
> > +
> > +                               ports {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       port@0 {
> > +                                               reg = <0>;
> > +                                       };
> > +
> > +                                       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>;
> 
> 266MHz is correct for dual camera, but in single camera, the second
> CSI is capable of 277MHz.
> 
> At a minimum, I'd like to fix the overdrive frequency to nominal, but
> since we're plumbing in both cameras, I wonder if it would be better
> to run both at 266MHz with a note on CSI0 that states it could run at
> 400 or 500 if the second CSI is disabled and a note on the second CSI
> that it could run at 277 when the first one is disabled? What are your
> thoughts?

My thoughts is that this all should be selected at runtime, based on how
many cameras are used. That won't be trivial to do though :-S Kieran,
you've been working with two cameras, any opinion ?

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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Laurent Pinchart]

Hi Adam,

(CC'ing Kieran)

On Tue, Nov 28, 2023 at 09:17:51PM -0600, Adam Ford wrote:
> On Mon, Apr 17, 2023 at 1:01 AM 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>
> > ---
> 
> Laurent,
> 
> Sorry to dig up an old thread, but I have a concern about the clock
> ratings and nominal mode vs overdrive mode.  I started investigating
> the different data sheets amongst the various imx8m[mnp] families to
> make the default device trees run at nominal mode while also creating
> a separate dtsi file with settings for overdrive so boards who use it
> can include them without having to duplicate the clock settings for
> everyone who supports overdrive.
> 
> >  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>;
> 
> According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
> 500MHz is listed as single-camera, overdrive mode.  Single-camera,
> nominal mode is 400MHz, but there is more...
> If configured for dual cameras, both CSI can only support up to
> 266MHz, but we have partially configured both albeit without the
> actual camera sensors connected.
> 
> > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > +                               status = "disabled";
> > +
> > +                               ports {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       port@0 {
> > +                                               reg = <0>;
> > +                                       };
> > +
> > +                                       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>;
> 
> 266MHz is correct for dual camera, but in single camera, the second
> CSI is capable of 277MHz.
> 
> At a minimum, I'd like to fix the overdrive frequency to nominal, but
> since we're plumbing in both cameras, I wonder if it would be better
> to run both at 266MHz with a note on CSI0 that states it could run at
> 400 or 500 if the second CSI is disabled and a note on the second CSI
> that it could run at 277 when the first one is disabled? What are your
> thoughts?

My thoughts is that this all should be selected at runtime, based on how
many cameras are used. That won't be trivial to do though :-S Kieran,
you've been working with two cameras, any opinion ?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Kieran Bingham]

Quoting Laurent Pinchart (2023-11-29 10:55:36)
> Hi Adam,
> 
> (CC'ing Kieran)
> 
> On Tue, Nov 28, 2023 at 09:17:51PM -0600, Adam Ford wrote:
> > On Mon, Apr 17, 2023 at 1:01 AM 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>
> > > ---
> > 
> > Laurent,
> > 
> > Sorry to dig up an old thread, but I have a concern about the clock
> > ratings and nominal mode vs overdrive mode.  I started investigating
> > the different data sheets amongst the various imx8m[mnp] families to
> > make the default device trees run at nominal mode while also creating
> > a separate dtsi file with settings for overdrive so boards who use it
> > can include them without having to duplicate the clock settings for
> > everyone who supports overdrive.
> > 
> > >  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>;
> > 
> > According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
> > 500MHz is listed as single-camera, overdrive mode.  Single-camera,
> > nominal mode is 400MHz, but there is more...
> > If configured for dual cameras, both CSI can only support up to
> > 266MHz, but we have partially configured both albeit without the
> > actual camera sensors connected.
> > 
> > > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > > +                               status = "disabled";
> > > +
> > > +                               ports {
> > > +                                       #address-cells = <1>;
> > > +                                       #size-cells = <0>;
> > > +
> > > +                                       port@0 {
> > > +                                               reg = <0>;
> > > +                                       };
> > > +
> > > +                                       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>;
> > 
> > 266MHz is correct for dual camera, but in single camera, the second
> > CSI is capable of 277MHz.
> > 
> > At a minimum, I'd like to fix the overdrive frequency to nominal, but
> > since we're plumbing in both cameras, I wonder if it would be better
> > to run both at 266MHz with a note on CSI0 that states it could run at
> > 400 or 500 if the second CSI is disabled and a note on the second CSI
> > that it could run at 277 when the first one is disabled? What are your
> > thoughts?
> 
> My thoughts is that this all should be selected at runtime, based on how
> many cameras are used. That won't be trivial to do though :-S Kieran,
> you've been working with two cameras, any opinion ?

Well I've been seeing issues like dropped packets and various issues on
the CSI2 receiver that could be because of badly configured clocks ...
so there's definitely some things to check and validate here still.

I don't know how the clocking would be adapted at runtime yet either ...
but it might be something interesting to look at. It's likely more
difficult as it's two separate nodes that would have to know about each
other at some level.

--
Kieran


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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Kieran Bingham]

Quoting Laurent Pinchart (2023-11-29 10:55:36)
> Hi Adam,
> 
> (CC'ing Kieran)
> 
> On Tue, Nov 28, 2023 at 09:17:51PM -0600, Adam Ford wrote:
> > On Mon, Apr 17, 2023 at 1:01 AM 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>
> > > ---
> > 
> > Laurent,
> > 
> > Sorry to dig up an old thread, but I have a concern about the clock
> > ratings and nominal mode vs overdrive mode.  I started investigating
> > the different data sheets amongst the various imx8m[mnp] families to
> > make the default device trees run at nominal mode while also creating
> > a separate dtsi file with settings for overdrive so boards who use it
> > can include them without having to duplicate the clock settings for
> > everyone who supports overdrive.
> > 
> > >  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>;
> > 
> > According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
> > 500MHz is listed as single-camera, overdrive mode.  Single-camera,
> > nominal mode is 400MHz, but there is more...
> > If configured for dual cameras, both CSI can only support up to
> > 266MHz, but we have partially configured both albeit without the
> > actual camera sensors connected.
> > 
> > > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > > +                               status = "disabled";
> > > +
> > > +                               ports {
> > > +                                       #address-cells = <1>;
> > > +                                       #size-cells = <0>;
> > > +
> > > +                                       port@0 {
> > > +                                               reg = <0>;
> > > +                                       };
> > > +
> > > +                                       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>;
> > 
> > 266MHz is correct for dual camera, but in single camera, the second
> > CSI is capable of 277MHz.
> > 
> > At a minimum, I'd like to fix the overdrive frequency to nominal, but
> > since we're plumbing in both cameras, I wonder if it would be better
> > to run both at 266MHz with a note on CSI0 that states it could run at
> > 400 or 500 if the second CSI is disabled and a note on the second CSI
> > that it could run at 277 when the first one is disabled? What are your
> > thoughts?
> 
> My thoughts is that this all should be selected at runtime, based on how
> many cameras are used. That won't be trivial to do though :-S Kieran,
> you've been working with two cameras, any opinion ?

Well I've been seeing issues like dropped packets and various issues on
the CSI2 receiver that could be because of badly configured clocks ...
so there's definitely some things to check and validate here still.

I don't know how the clocking would be adapted at runtime yet either ...
but it might be something interesting to look at. It's likely more
difficult as it's two separate nodes that would have to know about each
other at some level.

--
Kieran


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Adam Ford]

On Wed, Nov 29, 2023 at 2:14 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Laurent Pinchart (2023-11-29 10:55:36)
> > Hi Adam,
> >
> > (CC'ing Kieran)
> >
> > On Tue, Nov 28, 2023 at 09:17:51PM -0600, Adam Ford wrote:
> > > On Mon, Apr 17, 2023 at 1:01 AM 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>
> > > > ---
> > >
> > > Laurent,
> > >
> > > Sorry to dig up an old thread, but I have a concern about the clock
> > > ratings and nominal mode vs overdrive mode.  I started investigating
> > > the different data sheets amongst the various imx8m[mnp] families to
> > > make the default device trees run at nominal mode while also creating
> > > a separate dtsi file with settings for overdrive so boards who use it
> > > can include them without having to duplicate the clock settings for
> > > everyone who supports overdrive.
> > >
> > > >  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>;
> > >
> > > According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
> > > 500MHz is listed as single-camera, overdrive mode.  Single-camera,
> > > nominal mode is 400MHz, but there is more...
> > > If configured for dual cameras, both CSI can only support up to
> > > 266MHz, but we have partially configured both albeit without the
> > > actual camera sensors connected.
> > >
> > > > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > > > +                               status = "disabled";
> > > > +
> > > > +                               ports {
> > > > +                                       #address-cells = <1>;
> > > > +                                       #size-cells = <0>;
> > > > +
> > > > +                                       port@0 {
> > > > +                                               reg = <0>;
> > > > +                                       };
> > > > +
> > > > +                                       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>;
> > >
> > > 266MHz is correct for dual camera, but in single camera, the second
> > > CSI is capable of 277MHz.
> > >
> > > At a minimum, I'd like to fix the overdrive frequency to nominal, but
> > > since we're plumbing in both cameras, I wonder if it would be better
> > > to run both at 266MHz with a note on CSI0 that states it could run at
> > > 400 or 500 if the second CSI is disabled and a note on the second CSI
> > > that it could run at 277 when the first one is disabled? What are your
> > > thoughts?
> >
> > My thoughts is that this all should be selected at runtime, based on how
> > many cameras are used. That won't be trivial to do though :-S Kieran,
> > you've been working with two cameras, any opinion ?
>
> Well I've been seeing issues like dropped packets and various issues on
> the CSI2 receiver that could be because of badly configured clocks ...
> so there's definitely some things to check and validate here still.
>
> I don't know how the clocking would be adapted at runtime yet either ...
> but it might be something interesting to look at. It's likely more
> difficult as it's two separate nodes that would have to know about each
> other at some level.

Any objections if I slow them both to 266Mhz and add a note that
indicates what they're each capable of doing based on the datasheet?
This way, regardless of whether or not people have one or two cameras
it won't violate a spec.  Adding the note at least tells people they
have options to increase the clock speeds if they choose.

adam

>
> --
> Kieran
>
>
> >
> > > > +                               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

Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Adam Ford]

On Wed, Nov 29, 2023 at 2:14 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Laurent Pinchart (2023-11-29 10:55:36)
> > Hi Adam,
> >
> > (CC'ing Kieran)
> >
> > On Tue, Nov 28, 2023 at 09:17:51PM -0600, Adam Ford wrote:
> > > On Mon, Apr 17, 2023 at 1:01 AM 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>
> > > > ---
> > >
> > > Laurent,
> > >
> > > Sorry to dig up an old thread, but I have a concern about the clock
> > > ratings and nominal mode vs overdrive mode.  I started investigating
> > > the different data sheets amongst the various imx8m[mnp] families to
> > > make the default device trees run at nominal mode while also creating
> > > a separate dtsi file with settings for overdrive so boards who use it
> > > can include them without having to duplicate the clock settings for
> > > everyone who supports overdrive.
> > >
> > > >  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>;
> > >
> > > According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
> > > 500MHz is listed as single-camera, overdrive mode.  Single-camera,
> > > nominal mode is 400MHz, but there is more...
> > > If configured for dual cameras, both CSI can only support up to
> > > 266MHz, but we have partially configured both albeit without the
> > > actual camera sensors connected.
> > >
> > > > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > > > +                               status = "disabled";
> > > > +
> > > > +                               ports {
> > > > +                                       #address-cells = <1>;
> > > > +                                       #size-cells = <0>;
> > > > +
> > > > +                                       port@0 {
> > > > +                                               reg = <0>;
> > > > +                                       };
> > > > +
> > > > +                                       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>;
> > >
> > > 266MHz is correct for dual camera, but in single camera, the second
> > > CSI is capable of 277MHz.
> > >
> > > At a minimum, I'd like to fix the overdrive frequency to nominal, but
> > > since we're plumbing in both cameras, I wonder if it would be better
> > > to run both at 266MHz with a note on CSI0 that states it could run at
> > > 400 or 500 if the second CSI is disabled and a note on the second CSI
> > > that it could run at 277 when the first one is disabled? What are your
> > > thoughts?
> >
> > My thoughts is that this all should be selected at runtime, based on how
> > many cameras are used. That won't be trivial to do though :-S Kieran,
> > you've been working with two cameras, any opinion ?
>
> Well I've been seeing issues like dropped packets and various issues on
> the CSI2 receiver that could be because of badly configured clocks ...
> so there's definitely some things to check and validate here still.
>
> I don't know how the clocking would be adapted at runtime yet either ...
> but it might be something interesting to look at. It's likely more
> difficult as it's two separate nodes that would have to know about each
> other at some level.

Any objections if I slow them both to 266Mhz and add a note that
indicates what they're each capable of doing based on the datasheet?
This way, regardless of whether or not people have one or two cameras
it won't violate a spec.  Adding the note at least tells people they
have options to increase the clock speeds if they choose.

adam

>
> --
> Kieran
>
>
> >
> > > > +                               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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel