linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
@ 2023-04-23 21:26 Adam Ford
  2023-04-23 21:26 ` [PATCH 2/2] arm64: defconfig: Enable video capture drivers on imx8mm/imx8mn Adam Ford
  2023-04-24  0:47 ` [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Laurent Pinchart
  0 siblings, 2 replies; 10+ messages in thread
From: Adam Ford @ 2023-04-23 21:26 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: laurent.pinchart, Adam Ford, Rob Herring, Krzysztof Kozlowski,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

The CSI in the imx8mn is the same as what is used in the imx8mm,
but it's routed to the ISI on the Nano. Add both the ISI and CSI
nodes, and pointing them to each other. Since the CSI capture is
dependent on an attached camera, mark both ISI and CSI as
disabled by default.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index 8be8f090e8b8..102550b41f22 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
 				};
 			};
 
+			isi: isi@32e20000 {
+				compatible = "fsl,imx8mn-isi";
+				reg = <0x32e20000 0x100>;
+				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
+					 <&clk IMX8MN_CLK_DISP_APB_ROOT>;
+				clock-names = "axi", "apb";
+				fsl,blk-ctrl = <&disp_blk_ctrl>;
+				power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
+				status = "disabled";
+
+				port {
+					isi_in: endpoint {
+						remote-endpoint = <&mipi_csi_out>;
+					};
+				};
+			};
+
 			disp_blk_ctrl: blk-ctrl@32e28000 {
 				compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
 				reg = <0x32e28000 0x100>;
@@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
 				#power-domain-cells = <1>;
 			};
 
+			mipi_csi: mipi-csi@32e30000 {
+				compatible = "fsl,imx8mm-mipi-csi2";
+				reg = <0x32e30000 0x1000>;
+				interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
+				assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
+						  <&clk IMX8MN_CLK_CSI1_PHY_REF>;
+				assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
+							  <&clk IMX8MN_SYS_PLL2_1000M>;
+				assigned-clock-rates = <333000000>;
+				clock-frequency = <333000000>;
+				clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
+					 <&clk IMX8MN_CLK_CAMERA_PIXEL>,
+					 <&clk IMX8MN_CLK_CSI1_PHY_REF>,
+					 <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
+				clock-names = "pclk", "wrap", "phy", "axi";
+				power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
+				status = "disabled";
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+					};
+
+					port@1 {
+						reg = <1>;
+
+						mipi_csi_out: endpoint {
+							remote-endpoint = <&isi_in>;
+						};
+					};
+				};
+			};
+
 			usbotg1: usb@32e40000 {
 				compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
 				reg = <0x32e40000 0x200>;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] arm64: defconfig: Enable video capture drivers on imx8mm/imx8mn
  2023-04-23 21:26 [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Adam Ford
@ 2023-04-23 21:26 ` Adam Ford
  2023-04-24  0:48   ` Laurent Pinchart
  2023-04-24  0:47 ` [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Laurent Pinchart
  1 sibling, 1 reply; 10+ messages in thread
From: Adam Ford @ 2023-04-23 21:26 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: laurent.pinchart, Adam Ford, Rob Herring, Krzysztof Kozlowski,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

The imx8m Mini and imx8m Nano both use the same CSIS driver for
interfacing with sensors and cameras.  The Mini routes the CSIS
output to the imx7-CSI driver, and the Nano routes the CSIS
through the imx8-isi driver like the one used on the imx8mp.

Enable the drivers necessary to facilitate capture on both of
these two platforms.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index a24609e14d50..7005640fbb8e 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -743,6 +743,10 @@ CONFIG_SDR_PLATFORM_DRIVERS=y
 CONFIG_V4L_MEM2MEM_DRIVERS=y
 CONFIG_VIDEO_MEDIATEK_JPEG=m
 CONFIG_VIDEO_MEDIATEK_VCODEC=m
+CONFIG_VIDEO_IMX7_CSI=m
+CONFIG_VIDEO_IMX_MIPI_CSIS=m
+CONFIG_VIDEO_IMX8_ISI=m
+CONFIG_VIDEO_IMX8_ISI_M2M=y
 CONFIG_VIDEO_QCOM_CAMSS=m
 CONFIG_VIDEO_QCOM_VENUS=m
 CONFIG_VIDEO_RCAR_ISP=m
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
  2023-04-23 21:26 [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Adam Ford
  2023-04-23 21:26 ` [PATCH 2/2] arm64: defconfig: Enable video capture drivers on imx8mm/imx8mn Adam Ford
@ 2023-04-24  0:47 ` Laurent Pinchart
  2023-04-24  0:49   ` Laurent Pinchart
  2023-04-24  2:22   ` Adam Ford
  1 sibling, 2 replies; 10+ messages in thread
From: Laurent Pinchart @ 2023-04-24  0:47 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

Hi Adam,

Thank you for the patch.

On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> The CSI in the imx8mn is the same as what is used in the imx8mm,
> but it's routed to the ISI on the Nano. Add both the ISI and CSI
> nodes, and pointing them to each other. Since the CSI capture is
> dependent on an attached camera, mark both ISI and CSI as
> disabled by default.

I'd then write the subject line as "Add CSI and ISI nodes".

> Signed-off-by: Adam Ford <aford173@gmail.com>
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> index 8be8f090e8b8..102550b41f22 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
>  				};
>  			};
>  
> +			isi: isi@32e20000 {
> +				compatible = "fsl,imx8mn-isi";
> +				reg = <0x32e20000 0x100>;

The i.MX8MN reference manual documents the ISI registers block size to
be 64kB. Should we use the same here, even if all the registers we need
are within the first 256 bytes ?

> +				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +				clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> +					 <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> +				clock-names = "axi", "apb";
> +				fsl,blk-ctrl = <&disp_blk_ctrl>;
> +				power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
> +				status = "disabled";
> +
> +				port {
> +					isi_in: endpoint {
> +						remote-endpoint = <&mipi_csi_out>;
> +					};
> +				};

This will fail to validate against the ISI DT binding, as they require a
"ports" node. When a single port is present using a "port" node directly
is fine from an OF graph point of view, but to avoid too much complexity
in the ISI binding the consensus was to always require a "ports" node
for the ISI.

> +			};
> +
>  			disp_blk_ctrl: blk-ctrl@32e28000 {
>  				compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
>  				reg = <0x32e28000 0x100>;
> @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
>  				#power-domain-cells = <1>;
>  			};
>  
> +			mipi_csi: mipi-csi@32e30000 {
> +				compatible = "fsl,imx8mm-mipi-csi2";
> +				reg = <0x32e30000 0x1000>;
> +				interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> +				assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> +						  <&clk IMX8MN_CLK_CSI1_PHY_REF>;
> +				assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
> +							  <&clk IMX8MN_SYS_PLL2_1000M>;
> +				assigned-clock-rates = <333000000>;
> +				clock-frequency = <333000000>;
> +				clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> +					 <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> +					 <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> +					 <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> +				clock-names = "pclk", "wrap", "phy", "axi";
> +				power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
> +				status = "disabled";
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					port@0 {
> +						reg = <0>;
> +					};
> +
> +					port@1 {
> +						reg = <1>;
> +
> +						mipi_csi_out: endpoint {
> +							remote-endpoint = <&isi_in>;
> +						};
> +					};
> +				};
> +			};
> +
>  			usbotg1: usb@32e40000 {
>  				compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
>  				reg = <0x32e40000 0x200>;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: defconfig: Enable video capture drivers on imx8mm/imx8mn
  2023-04-23 21:26 ` [PATCH 2/2] arm64: defconfig: Enable video capture drivers on imx8mm/imx8mn Adam Ford
@ 2023-04-24  0:48   ` Laurent Pinchart
  0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2023-04-24  0:48 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

Hi Adam,

Thank you for the patch.

On Sun, Apr 23, 2023 at 04:26:56PM -0500, Adam Ford wrote:
> The imx8m Mini and imx8m Nano both use the same CSIS driver for
> interfacing with sensors and cameras.  The Mini routes the CSIS
> output to the imx7-CSI driver, and the Nano routes the CSIS
> through the imx8-isi driver like the one used on the imx8mp.
> 
> Enable the drivers necessary to facilitate capture on both of
> these two platforms.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index a24609e14d50..7005640fbb8e 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -743,6 +743,10 @@ CONFIG_SDR_PLATFORM_DRIVERS=y
>  CONFIG_V4L_MEM2MEM_DRIVERS=y
>  CONFIG_VIDEO_MEDIATEK_JPEG=m
>  CONFIG_VIDEO_MEDIATEK_VCODEC=m
> +CONFIG_VIDEO_IMX7_CSI=m
> +CONFIG_VIDEO_IMX_MIPI_CSIS=m
> +CONFIG_VIDEO_IMX8_ISI=m
> +CONFIG_VIDEO_IMX8_ISI_M2M=y
>  CONFIG_VIDEO_QCOM_CAMSS=m
>  CONFIG_VIDEO_QCOM_VENUS=m
>  CONFIG_VIDEO_RCAR_ISP=m

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
  2023-04-24  0:47 ` [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Laurent Pinchart
@ 2023-04-24  0:49   ` Laurent Pinchart
  2023-04-24  2:29     ` Adam Ford
  2023-04-24  2:22   ` Adam Ford
  1 sibling, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2023-04-24  0:49 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

On Mon, Apr 24, 2023 at 03:47:13AM +0300, Laurent Pinchart wrote:
> Hi Adam,

Another comment, do you plan to submit a patch with a camera DT overlay
for an i.MX8MN board ?

> Thank you for the patch.
> 
> On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> > The CSI in the imx8mn is the same as what is used in the imx8mm,
> > but it's routed to the ISI on the Nano. Add both the ISI and CSI
> > nodes, and pointing them to each other. Since the CSI capture is
> > dependent on an attached camera, mark both ISI and CSI as
> > disabled by default.
> 
> I'd then write the subject line as "Add CSI and ISI nodes".
> 
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > index 8be8f090e8b8..102550b41f22 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
> >  				};
> >  			};
> >  
> > +			isi: isi@32e20000 {
> > +				compatible = "fsl,imx8mn-isi";
> > +				reg = <0x32e20000 0x100>;
> 
> The i.MX8MN reference manual documents the ISI registers block size to
> be 64kB. Should we use the same here, even if all the registers we need
> are within the first 256 bytes ?
> 
> > +				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > +				clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > +					 <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> > +				clock-names = "axi", "apb";
> > +				fsl,blk-ctrl = <&disp_blk_ctrl>;
> > +				power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
> > +				status = "disabled";
> > +
> > +				port {
> > +					isi_in: endpoint {
> > +						remote-endpoint = <&mipi_csi_out>;
> > +					};
> > +				};
> 
> This will fail to validate against the ISI DT binding, as they require a
> "ports" node. When a single port is present using a "port" node directly
> is fine from an OF graph point of view, but to avoid too much complexity
> in the ISI binding the consensus was to always require a "ports" node
> for the ISI.
> 
> > +			};
> > +
> >  			disp_blk_ctrl: blk-ctrl@32e28000 {
> >  				compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
> >  				reg = <0x32e28000 0x100>;
> > @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
> >  				#power-domain-cells = <1>;
> >  			};
> >  
> > +			mipi_csi: mipi-csi@32e30000 {
> > +				compatible = "fsl,imx8mm-mipi-csi2";
> > +				reg = <0x32e30000 0x1000>;
> > +				interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> > +				assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > +						  <&clk IMX8MN_CLK_CSI1_PHY_REF>;
> > +				assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
> > +							  <&clk IMX8MN_SYS_PLL2_1000M>;
> > +				assigned-clock-rates = <333000000>;
> > +				clock-frequency = <333000000>;
> > +				clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > +					 <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > +					 <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > +					 <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> > +				clock-names = "pclk", "wrap", "phy", "axi";
> > +				power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
> > +				status = "disabled";
> > +
> > +				ports {
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +
> > +					port@0 {
> > +						reg = <0>;
> > +					};
> > +
> > +					port@1 {
> > +						reg = <1>;
> > +
> > +						mipi_csi_out: endpoint {
> > +							remote-endpoint = <&isi_in>;
> > +						};
> > +					};
> > +				};
> > +			};
> > +
> >  			usbotg1: usb@32e40000 {
> >  				compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
> >  				reg = <0x32e40000 0x200>;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
  2023-04-24  0:47 ` [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Laurent Pinchart
  2023-04-24  0:49   ` Laurent Pinchart
@ 2023-04-24  2:22   ` Adam Ford
  2023-04-24  2:59     ` Adam Ford
  1 sibling, 1 reply; 10+ messages in thread
From: Adam Ford @ 2023-04-24  2:22 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

On Sun, Apr 23, 2023 at 7:46 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Adam,
>
> Thank you for the patch.
>
> On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> > The CSI in the imx8mn is the same as what is used in the imx8mm,
> > but it's routed to the ISI on the Nano. Add both the ISI and CSI
> > nodes, and pointing them to each other. Since the CSI capture is
> > dependent on an attached camera, mark both ISI and CSI as
> > disabled by default.
>
> I'd then write the subject line as "Add CSI and ISI nodes".

That makes sense, especially since I disabled them by default.
>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > index 8be8f090e8b8..102550b41f22 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
> >                               };
> >                       };
> >
> > +                     isi: isi@32e20000 {
> > +                             compatible = "fsl,imx8mn-isi";
> > +                             reg = <0x32e20000 0x100>;
>
> The i.MX8MN reference manual documents the ISI registers block size to
> be 64kB. Should we use the same here, even if all the registers we need
> are within the first 256 bytes ?

I can do that.
>
> > +                             interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > +                             clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > +                                      <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> > +                             clock-names = "axi", "apb";
> > +                             fsl,blk-ctrl = <&disp_blk_ctrl>;
> > +                             power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
> > +                             status = "disabled";
> > +
> > +                             port {
> > +                                     isi_in: endpoint {
> > +                                             remote-endpoint = <&mipi_csi_out>;
> > +                                     };
> > +                             };
>
> This will fail to validate against the ISI DT binding, as they require a
> "ports" node. When a single port is present using a "port" node directly
> is fine from an OF graph point of view, but to avoid too much complexity
> in the ISI binding the consensus was to always require a "ports" node
> for the ISI.


Argh!  I pulled from the wrong test repo.  I remember the discussion
from a few months back.  I'll fix it and the others when I submit V2.

>
> > +                     };
> > +
> >                       disp_blk_ctrl: blk-ctrl@32e28000 {
> >                               compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
> >                               reg = <0x32e28000 0x100>;
> > @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
> >                               #power-domain-cells = <1>;
> >                       };
> >
> > +                     mipi_csi: mipi-csi@32e30000 {
> > +                             compatible = "fsl,imx8mm-mipi-csi2";
> > +                             reg = <0x32e30000 0x1000>;
> > +                             interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> > +                             assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > +                                               <&clk IMX8MN_CLK_CSI1_PHY_REF>;
> > +                             assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
> > +                                                       <&clk IMX8MN_SYS_PLL2_1000M>;
> > +                             assigned-clock-rates = <333000000>;
> > +                             clock-frequency = <333000000>;
> > +                             clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > +                                      <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > +                                      <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > +                                      <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> > +                             clock-names = "pclk", "wrap", "phy", "axi";
> > +                             power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
> > +                             status = "disabled";
> > +
> > +                             ports {
> > +                                     #address-cells = <1>;
> > +                                     #size-cells = <0>;
> > +
> > +                                     port@0 {
> > +                                             reg = <0>;
> > +                                     };
> > +
> > +                                     port@1 {
> > +                                             reg = <1>;
> > +
> > +                                             mipi_csi_out: endpoint {
> > +                                                     remote-endpoint = <&isi_in>;
> > +                                             };
> > +                                     };
> > +                             };
> > +                     };
> > +
> >                       usbotg1: usb@32e40000 {
> >                               compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
> >                               reg = <0x32e40000 0x200>;
>
> --
> Regards,
>
> Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
  2023-04-24  0:49   ` Laurent Pinchart
@ 2023-04-24  2:29     ` Adam Ford
  2023-04-24  3:17       ` Laurent Pinchart
  0 siblings, 1 reply; 10+ messages in thread
From: Adam Ford @ 2023-04-24  2:29 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

On Sun, Apr 23, 2023 at 7:48 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Apr 24, 2023 at 03:47:13AM +0300, Laurent Pinchart wrote:
> > Hi Adam,
>
> Another comment, do you plan to submit a patch with a camera DT overlay
> for an i.MX8MN board ?

My test repo has the ISI driver working with my OV5640 camera, but for
some reason the mainline doesn't work.  I'm trying to figure out
what's different between my test repo and the mainline.  I am not
planning on an overlay, as the Beacon baseboard was only designed with
one camera module, a TD Next 5640, based on the OV5640.  Newer boards
have been designed with a more generic camera interface, so I plan to
use overlays in the future, but for the imx8mn-beacon-kit, I was
planning to follow a similar design to what was done for the
imx8mm-beacon-kit.  They share the same baseboard with a different
processor on the system-on-module.

Do you want me to add a patch to the imx8mn-beacon-kit so there is at
least one user of this driver, once I get my error resolved?

adam
>
> > Thank you for the patch.
> >
> > On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> > > The CSI in the imx8mn is the same as what is used in the imx8mm,
> > > but it's routed to the ISI on the Nano. Add both the ISI and CSI
> > > nodes, and pointing them to each other. Since the CSI capture is
> > > dependent on an attached camera, mark both ISI and CSI as
> > > disabled by default.
> >
> > I'd then write the subject line as "Add CSI and ISI nodes".
> >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > index 8be8f090e8b8..102550b41f22 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
> > >                             };
> > >                     };
> > >
> > > +                   isi: isi@32e20000 {
> > > +                           compatible = "fsl,imx8mn-isi";
> > > +                           reg = <0x32e20000 0x100>;
> >
> > The i.MX8MN reference manual documents the ISI registers block size to
> > be 64kB. Should we use the same here, even if all the registers we need
> > are within the first 256 bytes ?
> >
> > > +                           interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > > +                           clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > > +                                    <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> > > +                           clock-names = "axi", "apb";
> > > +                           fsl,blk-ctrl = <&disp_blk_ctrl>;
> > > +                           power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
> > > +                           status = "disabled";
> > > +
> > > +                           port {
> > > +                                   isi_in: endpoint {
> > > +                                           remote-endpoint = <&mipi_csi_out>;
> > > +                                   };
> > > +                           };
> >
> > This will fail to validate against the ISI DT binding, as they require a
> > "ports" node. When a single port is present using a "port" node directly
> > is fine from an OF graph point of view, but to avoid too much complexity
> > in the ISI binding the consensus was to always require a "ports" node
> > for the ISI.
> >
> > > +                   };
> > > +
> > >                     disp_blk_ctrl: blk-ctrl@32e28000 {
> > >                             compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
> > >                             reg = <0x32e28000 0x100>;
> > > @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
> > >                             #power-domain-cells = <1>;
> > >                     };
> > >
> > > +                   mipi_csi: mipi-csi@32e30000 {
> > > +                           compatible = "fsl,imx8mm-mipi-csi2";
> > > +                           reg = <0x32e30000 0x1000>;
> > > +                           interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> > > +                           assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > +                                             <&clk IMX8MN_CLK_CSI1_PHY_REF>;
> > > +                           assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
> > > +                                                     <&clk IMX8MN_SYS_PLL2_1000M>;
> > > +                           assigned-clock-rates = <333000000>;
> > > +                           clock-frequency = <333000000>;
> > > +                           clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > > +                                    <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > +                                    <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > > +                                    <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> > > +                           clock-names = "pclk", "wrap", "phy", "axi";
> > > +                           power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
> > > +                           status = "disabled";
> > > +
> > > +                           ports {
> > > +                                   #address-cells = <1>;
> > > +                                   #size-cells = <0>;
> > > +
> > > +                                   port@0 {
> > > +                                           reg = <0>;
> > > +                                   };
> > > +
> > > +                                   port@1 {
> > > +                                           reg = <1>;
> > > +
> > > +                                           mipi_csi_out: endpoint {
> > > +                                                   remote-endpoint = <&isi_in>;
> > > +                                           };
> > > +                                   };
> > > +                           };
> > > +                   };
> > > +
> > >                     usbotg1: usb@32e40000 {
> > >                             compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
> > >                             reg = <0x32e40000 0x200>;
>
> --
> Regards,
>
> Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
  2023-04-24  2:22   ` Adam Ford
@ 2023-04-24  2:59     ` Adam Ford
  2023-04-24  3:16       ` Laurent Pinchart
  0 siblings, 1 reply; 10+ messages in thread
From: Adam Ford @ 2023-04-24  2:59 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

On Sun, Apr 23, 2023 at 9:22 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Sun, Apr 23, 2023 at 7:46 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Adam,
> >
> > Thank you for the patch.
> >
> > On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> > > The CSI in the imx8mn is the same as what is used in the imx8mm,
> > > but it's routed to the ISI on the Nano. Add both the ISI and CSI
> > > nodes, and pointing them to each other. Since the CSI capture is
> > > dependent on an attached camera, mark both ISI and CSI as
> > > disabled by default.
> >
> > I'd then write the subject line as "Add CSI and ISI nodes".
>
> That makes sense, especially since I disabled them by default.
> >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > index 8be8f090e8b8..102550b41f22 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
> > >                               };
> > >                       };
> > >
> > > +                     isi: isi@32e20000 {
> > > +                             compatible = "fsl,imx8mn-isi";
> > > +                             reg = <0x32e20000 0x100>;
> >
> > The i.MX8MN reference manual documents the ISI registers block size to
> > be 64kB. Should we use the same here, even if all the registers we need
> > are within the first 256 bytes ?
>
> I can do that.

There is a typo in the Nano Ref Manual.  Even though the table in
"Table 2-6. AIPS4 Memory Map" reads 64K, the DISPLAY_BLK_CTRL starts
at 32e2_8000.  The largest size we can do is 0x8000 (32k)

> >
> > > +                             interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > > +                             clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > > +                                      <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> > > +                             clock-names = "axi", "apb";
> > > +                             fsl,blk-ctrl = <&disp_blk_ctrl>;
> > > +                             power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
> > > +                             status = "disabled";
> > > +
> > > +                             port {
> > > +                                     isi_in: endpoint {
> > > +                                             remote-endpoint = <&mipi_csi_out>;
> > > +                                     };
> > > +                             };
> >
> > This will fail to validate against the ISI DT binding, as they require a
> > "ports" node. When a single port is present using a "port" node directly
> > is fine from an OF graph point of view, but to avoid too much complexity
> > in the ISI binding the consensus was to always require a "ports" node
> > for the ISI.
>
>
> Argh!  I pulled from the wrong test repo.  I remember the discussion
> from a few months back.  I'll fix it and the others when I submit V2.

It appears that using ports still throws warnings:

arch/arm64/boot/dts/freescale/imx8mn.dtsi:1118.11-1128.7: Warning
(graph_child_address): /soc@0/bus@32c00000/isi@32e20000/ports: graph
node has single child node 'port@0', #address-cells/#size-cells are
not necessary

I'll leave it like this because we were told to do so.

adam
>
> >
> > > +                     };
> > > +
> > >                       disp_blk_ctrl: blk-ctrl@32e28000 {
> > >                               compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
> > >                               reg = <0x32e28000 0x100>;
> > > @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
> > >                               #power-domain-cells = <1>;
> > >                       };
> > >
> > > +                     mipi_csi: mipi-csi@32e30000 {
> > > +                             compatible = "fsl,imx8mm-mipi-csi2";
> > > +                             reg = <0x32e30000 0x1000>;
> > > +                             interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> > > +                             assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > +                                               <&clk IMX8MN_CLK_CSI1_PHY_REF>;
> > > +                             assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
> > > +                                                       <&clk IMX8MN_SYS_PLL2_1000M>;
> > > +                             assigned-clock-rates = <333000000>;
> > > +                             clock-frequency = <333000000>;
> > > +                             clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > > +                                      <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > +                                      <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > > +                                      <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> > > +                             clock-names = "pclk", "wrap", "phy", "axi";
> > > +                             power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
> > > +                             status = "disabled";
> > > +
> > > +                             ports {
> > > +                                     #address-cells = <1>;
> > > +                                     #size-cells = <0>;
> > > +
> > > +                                     port@0 {
> > > +                                             reg = <0>;
> > > +                                     };
> > > +
> > > +                                     port@1 {
> > > +                                             reg = <1>;
> > > +
> > > +                                             mipi_csi_out: endpoint {
> > > +                                                     remote-endpoint = <&isi_in>;
> > > +                                             };
> > > +                                     };
> > > +                             };
> > > +                     };
> > > +
> > >                       usbotg1: usb@32e40000 {
> > >                               compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
> > >                               reg = <0x32e40000 0x200>;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
  2023-04-24  2:59     ` Adam Ford
@ 2023-04-24  3:16       ` Laurent Pinchart
  0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2023-04-24  3:16 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

Hi Adam,

On Sun, Apr 23, 2023 at 09:59:11PM -0500, Adam Ford wrote:
> On Sun, Apr 23, 2023 at 9:22 PM Adam Ford wrote:
> > On Sun, Apr 23, 2023 at 7:46 PM Laurent Pinchart wrote:
> > > On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> > > > The CSI in the imx8mn is the same as what is used in the imx8mm,
> > > > but it's routed to the ISI on the Nano. Add both the ISI and CSI
> > > > nodes, and pointing them to each other. Since the CSI capture is
> > > > dependent on an attached camera, mark both ISI and CSI as
> > > > disabled by default.
> > >
> > > I'd then write the subject line as "Add CSI and ISI nodes".
> >
> > That makes sense, especially since I disabled them by default.
> > >
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > > index 8be8f090e8b8..102550b41f22 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > > @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
> > > >                               };
> > > >                       };
> > > >
> > > > +                     isi: isi@32e20000 {
> > > > +                             compatible = "fsl,imx8mn-isi";
> > > > +                             reg = <0x32e20000 0x100>;
> > >
> > > The i.MX8MN reference manual documents the ISI registers block size to
> > > be 64kB. Should we use the same here, even if all the registers we need
> > > are within the first 256 bytes ?
> >
> > I can do that.
> 
> There is a typo in the Nano Ref Manual.  Even though the table in
> "Table 2-6. AIPS4 Memory Map" reads 64K, the DISPLAY_BLK_CTRL starts
> at 32e2_8000.  The largest size we can do is 0x8000 (32k)

32k is fine. If you prefer keeping 0x100, that's fine with me too.

> > > > +                             interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                             clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > > > +                                      <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> > > > +                             clock-names = "axi", "apb";
> > > > +                             fsl,blk-ctrl = <&disp_blk_ctrl>;
> > > > +                             power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
> > > > +                             status = "disabled";
> > > > +
> > > > +                             port {
> > > > +                                     isi_in: endpoint {
> > > > +                                             remote-endpoint = <&mipi_csi_out>;
> > > > +                                     };
> > > > +                             };
> > >
> > > This will fail to validate against the ISI DT binding, as they require a
> > > "ports" node. When a single port is present using a "port" node directly
> > > is fine from an OF graph point of view, but to avoid too much complexity
> > > in the ISI binding the consensus was to always require a "ports" node
> > > for the ISI.
> >
> >
> > Argh!  I pulled from the wrong test repo.  I remember the discussion
> > from a few months back.  I'll fix it and the others when I submit V2.
> 
> It appears that using ports still throws warnings:
> 
> arch/arm64/boot/dts/freescale/imx8mn.dtsi:1118.11-1128.7: Warning
> (graph_child_address): /soc@0/bus@32c00000/isi@32e20000/ports: graph
> node has single child node 'port@0', #address-cells/#size-cells are
> not necessary

Aarghhhh :-)

> I'll leave it like this because we were told to do so.

Let's see if Rob or Krzysztof have a recommendation.

> > > > +                     };
> > > > +
> > > >                       disp_blk_ctrl: blk-ctrl@32e28000 {
> > > >                               compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
> > > >                               reg = <0x32e28000 0x100>;
> > > > @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
> > > >                               #power-domain-cells = <1>;
> > > >                       };
> > > >
> > > > +                     mipi_csi: mipi-csi@32e30000 {
> > > > +                             compatible = "fsl,imx8mm-mipi-csi2";
> > > > +                             reg = <0x32e30000 0x1000>;
> > > > +                             interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                             assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > > +                                               <&clk IMX8MN_CLK_CSI1_PHY_REF>;
> > > > +                             assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
> > > > +                                                       <&clk IMX8MN_SYS_PLL2_1000M>;
> > > > +                             assigned-clock-rates = <333000000>;
> > > > +                             clock-frequency = <333000000>;
> > > > +                             clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > > > +                                      <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > > +                                      <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > > > +                                      <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> > > > +                             clock-names = "pclk", "wrap", "phy", "axi";
> > > > +                             power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
> > > > +                             status = "disabled";
> > > > +
> > > > +                             ports {
> > > > +                                     #address-cells = <1>;
> > > > +                                     #size-cells = <0>;
> > > > +
> > > > +                                     port@0 {
> > > > +                                             reg = <0>;
> > > > +                                     };
> > > > +
> > > > +                                     port@1 {
> > > > +                                             reg = <1>;
> > > > +
> > > > +                                             mipi_csi_out: endpoint {
> > > > +                                                     remote-endpoint = <&isi_in>;
> > > > +                                             };
> > > > +                                     };
> > > > +                             };
> > > > +                     };
> > > > +
> > > >                       usbotg1: usb@32e40000 {
> > > >                               compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
> > > >                               reg = <0x32e40000 0x200>;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
  2023-04-24  2:29     ` Adam Ford
@ 2023-04-24  3:17       ` Laurent Pinchart
  0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2023-04-24  3:17 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, devicetree,
	linux-kernel

On Sun, Apr 23, 2023 at 09:29:04PM -0500, Adam Ford wrote:
> On Sun, Apr 23, 2023 at 7:48 PM Laurent Pinchart wrote:
> > On Mon, Apr 24, 2023 at 03:47:13AM +0300, Laurent Pinchart wrote:
> > > Hi Adam,
> >
> > Another comment, do you plan to submit a patch with a camera DT overlay
> > for an i.MX8MN board ?
> 
> My test repo has the ISI driver working with my OV5640 camera, but for
> some reason the mainline doesn't work.  I'm trying to figure out
> what's different between my test repo and the mainline.  I am not
> planning on an overlay, as the Beacon baseboard was only designed with
> one camera module, a TD Next 5640, based on the OV5640.  Newer boards
> have been designed with a more generic camera interface, so I plan to
> use overlays in the future, but for the imx8mn-beacon-kit, I was
> planning to follow a similar design to what was done for the
> imx8mm-beacon-kit.  They share the same baseboard with a different
> processor on the system-on-module.
> 
> Do you want me to add a patch to the imx8mn-beacon-kit so there is at
> least one user of this driver, once I get my error resolved?

That would be nice.

> > > Thank you for the patch.
> > >
> > > On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> > > > The CSI in the imx8mn is the same as what is used in the imx8mm,
> > > > but it's routed to the ISI on the Nano. Add both the ISI and CSI
> > > > nodes, and pointing them to each other. Since the CSI capture is
> > > > dependent on an attached camera, mark both ISI and CSI as
> > > > disabled by default.
> > >
> > > I'd then write the subject line as "Add CSI and ISI nodes".
> > >
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > > index 8be8f090e8b8..102550b41f22 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > > @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
> > > >                             };
> > > >                     };
> > > >
> > > > +                   isi: isi@32e20000 {
> > > > +                           compatible = "fsl,imx8mn-isi";
> > > > +                           reg = <0x32e20000 0x100>;
> > >
> > > The i.MX8MN reference manual documents the ISI registers block size to
> > > be 64kB. Should we use the same here, even if all the registers we need
> > > are within the first 256 bytes ?
> > >
> > > > +                           interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                           clocks = <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > > > +                                    <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> > > > +                           clock-names = "axi", "apb";
> > > > +                           fsl,blk-ctrl = <&disp_blk_ctrl>;
> > > > +                           power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_ISI>;
> > > > +                           status = "disabled";
> > > > +
> > > > +                           port {
> > > > +                                   isi_in: endpoint {
> > > > +                                           remote-endpoint = <&mipi_csi_out>;
> > > > +                                   };
> > > > +                           };
> > >
> > > This will fail to validate against the ISI DT binding, as they require a
> > > "ports" node. When a single port is present using a "port" node directly
> > > is fine from an OF graph point of view, but to avoid too much complexity
> > > in the ISI binding the consensus was to always require a "ports" node
> > > for the ISI.
> > >
> > > > +                   };
> > > > +
> > > >                     disp_blk_ctrl: blk-ctrl@32e28000 {
> > > >                             compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
> > > >                             reg = <0x32e28000 0x100>;
> > > > @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
> > > >                             #power-domain-cells = <1>;
> > > >                     };
> > > >
> > > > +                   mipi_csi: mipi-csi@32e30000 {
> > > > +                           compatible = "fsl,imx8mm-mipi-csi2";
> > > > +                           reg = <0x32e30000 0x1000>;
> > > > +                           interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                           assigned-clocks = <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > > +                                             <&clk IMX8MN_CLK_CSI1_PHY_REF>;
> > > > +                           assigned-clock-parents = <&clk IMX8MN_SYS_PLL2_1000M>,
> > > > +                                                     <&clk IMX8MN_SYS_PLL2_1000M>;
> > > > +                           assigned-clock-rates = <333000000>;
> > > > +                           clock-frequency = <333000000>;
> > > > +                           clocks = <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > > > +                                    <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > > +                                    <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > > > +                                    <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> > > > +                           clock-names = "pclk", "wrap", "phy", "axi";
> > > > +                           power-domains = <&disp_blk_ctrl IMX8MN_DISPBLK_PD_MIPI_CSI>;
> > > > +                           status = "disabled";
> > > > +
> > > > +                           ports {
> > > > +                                   #address-cells = <1>;
> > > > +                                   #size-cells = <0>;
> > > > +
> > > > +                                   port@0 {
> > > > +                                           reg = <0>;
> > > > +                                   };
> > > > +
> > > > +                                   port@1 {
> > > > +                                           reg = <1>;
> > > > +
> > > > +                                           mipi_csi_out: endpoint {
> > > > +                                                   remote-endpoint = <&isi_in>;
> > > > +                                           };
> > > > +                                   };
> > > > +                           };
> > > > +                   };
> > > > +
> > > >                     usbotg1: usb@32e40000 {
> > > >                             compatible = "fsl,imx8mn-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
> > > >                             reg = <0x32e40000 0x200>;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-04-24  3:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-23 21:26 [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Adam Ford
2023-04-23 21:26 ` [PATCH 2/2] arm64: defconfig: Enable video capture drivers on imx8mm/imx8mn Adam Ford
2023-04-24  0:48   ` Laurent Pinchart
2023-04-24  0:47 ` [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes Laurent Pinchart
2023-04-24  0:49   ` Laurent Pinchart
2023-04-24  2:29     ` Adam Ford
2023-04-24  3:17       ` Laurent Pinchart
2023-04-24  2:22   ` Adam Ford
2023-04-24  2:59     ` Adam Ford
2023-04-24  3:16       ` Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).