linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
@ 2021-11-06 15:54 Adam Ford
  2021-11-06 15:54 ` [PATCH V2 2/5] arm64: dts: imx8mm: Add CSI nodes Adam Ford
                   ` (7 more replies)
  0 siblings, 8 replies; 29+ messages in thread
From: Adam Ford @ 2021-11-06 15:54 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: tharvey, frieder.schrempf, linux-media, laurent.pinchart, aford,
	cstevens, jagan, Adam Ford, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	linux-kernel

Most of the blk-ctrl reset bits are found in one register, however
there are two bits in offset 8 for pulling the MIPI DPHY out of reset
and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
out of reset or the MIPI_CSI hangs.

Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
Signed-off-by: Adam Ford <aford173@gmail.com>
---

V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
     Rename the new register to mipi_phy_rst_mask
     Encapsulate the edits to this register with an if-statement

 drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 519b3651d1d9..581eb4bc7f7d 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -17,6 +17,7 @@
 
 #define BLK_SFT_RSTN	0x0
 #define BLK_CLK_EN	0x4
+#define BLK_MIPI_RESET_DIV	0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
 
 struct imx8m_blk_ctrl_domain;
 
@@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
 	const char *gpc_name;
 	u32 rst_mask;
 	u32 clk_mask;
+
+	/*
+	 * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
+	 * which is used to control the reset for the MIPI Phy.
+	 * Since it's only present in certain circumstances,
+	 * an if-statement should be used before setting and clearing this
+	 * register.
+	 */
+	u32 mipi_phy_rst_mask;
 };
 
 #define DOMAIN_MAX_CLKS 3
@@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
 
 	/* put devices into reset */
 	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
+	if (data->mipi_phy_rst_mask)
+		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
 
 	/* enable upstream and blk-ctrl clocks to allow reset to propagate */
 	ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
@@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
 
 	/* release reset */
 	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
+	if (data->mipi_phy_rst_mask)
+		regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
 
 	/* disable upstream clocks */
 	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
@@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
 	struct imx8m_blk_ctrl *bc = domain->bc;
 
 	/* put devices into reset and disable clocks */
+	if (data->mipi_phy_rst_mask)
+		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
+
 	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
 	regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
 
@@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
 		.gpc_name = "mipi-csi",
 		.rst_mask = BIT(3) | BIT(4),
 		.clk_mask = BIT(10) | BIT(11),
+		.mipi_phy_rst_mask = BIT(16) | BIT(17),
 	},
 };
 
-- 
2.32.0


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

* [PATCH V2 2/5] arm64: dts: imx8mm: Add CSI nodes
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
@ 2021-11-06 15:54 ` Adam Ford
  2021-11-07 12:58   ` Fabio Estevam
  2021-11-06 15:54 ` [PATCH V2 3/5] arm64: defconfig: Enable VIDEO_IMX_MEDIA Adam Ford
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-06 15:54 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: tharvey, frieder.schrempf, linux-media, laurent.pinchart, aford,
	cstevens, jagan, Adam Ford, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	linux-kernel

There is a csi bridge and csis interface that tie together
to allow csi2 capture.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-By: Tim Harvey <tharvey@gateworks.com>
Tested-By: Tim Harvey <tharvey@gateworks.com>
---
V2:  No change

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

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index c2f3f118f82e..1f69c14d953f 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -1068,6 +1068,22 @@ aips4: bus@32c00000 {
 			#size-cells = <1>;
 			ranges = <0x32c00000 0x32c00000 0x400000>;
 
+			csi: csi@32e20000 {
+				compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
+				reg = <0x32e20000 0x1000>;
+				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk IMX8MM_CLK_CSI1_ROOT>;
+				clock-names = "mclk";
+				power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_CSI_BRIDGE>;
+				status = "disabled";
+
+				port {
+					csi_in: endpoint {
+						remote-endpoint = <&imx8mm_mipi_csi_out>;
+					};
+				};
+			};
+
 			disp_blk_ctrl: blk-ctrl@32e28000 {
 				compatible = "fsl,imx8mm-disp-blk-ctrl", "syscon";
 				reg = <0x32e28000 0x100>;
@@ -1095,6 +1111,41 @@ 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 IMX8MM_CLK_CSI1_CORE>,
+						  <&clk IMX8MM_CLK_CSI1_PHY_REF>;
+				assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>,
+							  <&clk IMX8MM_SYS_PLL2_1000M>;
+				clock-frequency = <333000000>;
+				clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>,
+					 <&clk IMX8MM_CLK_CSI1_ROOT>,
+					 <&clk IMX8MM_CLK_CSI1_PHY_REF>,
+					 <&clk IMX8MM_CLK_DISP_AXI_ROOT>;
+				clock-names = "pclk", "wrap", "phy", "axi";
+				power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_MIPI_CSI>;
+				status = "disabled";
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+					};
+
+					port@1 {
+						reg = <1>;
+
+						imx8mm_mipi_csi_out: endpoint {
+							remote-endpoint = <&csi_in>;
+						};
+					};
+				};
+			};
+
 			usbotg1: usb@32e40000 {
 				compatible = "fsl,imx8mm-usb", "fsl,imx7d-usb";
 				reg = <0x32e40000 0x200>;
-- 
2.32.0


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

* [PATCH V2 3/5] arm64: defconfig: Enable VIDEO_IMX_MEDIA
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
  2021-11-06 15:54 ` [PATCH V2 2/5] arm64: dts: imx8mm: Add CSI nodes Adam Ford
@ 2021-11-06 15:54 ` Adam Ford
  2021-11-07 13:01   ` Fabio Estevam
  2021-11-06 15:54 ` [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera Adam Ford
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-06 15:54 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: tharvey, frieder.schrempf, linux-media, laurent.pinchart, aford,
	cstevens, jagan, Adam Ford, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	linux-kernel

To use a camera, the CSIS and CSI drivers need to be enabled with
VIDEO_IMX_MEDIA.

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

 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index f2e2b9bdd702..bc261cf2ef5a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -958,6 +958,7 @@ CONFIG_MFD_CROS_EC_DEV=y
 CONFIG_STAGING=y
 CONFIG_STAGING_MEDIA=y
 CONFIG_VIDEO_HANTRO=m
+CONFIG_VIDEO_IMX_MEDIA=m
 CONFIG_CHROME_PLATFORMS=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_I2C=y
-- 
2.32.0


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

* [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
  2021-11-06 15:54 ` [PATCH V2 2/5] arm64: dts: imx8mm: Add CSI nodes Adam Ford
  2021-11-06 15:54 ` [PATCH V2 3/5] arm64: defconfig: Enable VIDEO_IMX_MEDIA Adam Ford
@ 2021-11-06 15:54 ` Adam Ford
  2021-11-07 13:01   ` Fabio Estevam
  2021-11-21 23:18   ` Laurent Pinchart
  2021-11-06 15:54 ` [PATCH V2 5/5] arm64: defconfig: Enable OV5640 Adam Ford
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 29+ messages in thread
From: Adam Ford @ 2021-11-06 15:54 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: tharvey, frieder.schrempf, linux-media, laurent.pinchart, aford,
	cstevens, jagan, Adam Ford, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Lucas Stach, Peng Fan, devicetree,
	linux-kernel

The baseboard has support for a TDNext 5640 Camera which
uses an OV5640 connected to a 2-lane CSI2 interface.

With the CSI and mipi_csi2 drivers pointing to an OV5640 camera, the media
pipeline can be configured with the following:

    media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-csis.0':0[1]"

The camera and various nodes in the pipeline can be configured for UYVY:
    media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480 field:none]"
    media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480 field:none]"

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

 .../freescale/imx8mm-beacon-baseboard.dtsi    | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
index 6f5e63696ec0..0fb95f4a5e78 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
@@ -43,6 +43,16 @@ reg_audio: regulator-audio {
 		enable-active-high;
 	};
 
+	reg_camera: regulator-camera {
+		compatible = "regulator-fixed";
+		regulator-name = "mipi_pwr";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		gpio = <&pca6416_1 0 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+		startup-delay-us = <100000>;
+	};
+
 	reg_usdhc2_vmmc: regulator-usdhc2 {
 		compatible = "regulator-fixed";
 		regulator-name = "VSD_3V3";
@@ -67,6 +77,10 @@ sound {
 	};
 };
 
+&csi {
+	status = "okay";
+};
+
 &ecspi2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_espi2>;
@@ -90,6 +104,30 @@ &i2c2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_i2c2>;
 	status = "okay";
+
+	camera@3c {
+		compatible = "ovti,ov5640";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ov5640>;
+		reg = <0x3c>;
+		clocks = <&clk IMX8MM_CLK_CLKO1>;
+		clock-names = "xclk";
+		assigned-clocks = <&clk IMX8MM_CLK_CLKO1>;
+		assigned-clock-parents = <&clk IMX8MM_CLK_24M>;
+		assigned-clock-rates = <24000000>;
+		AVDD-supply = <&reg_camera>;  /* 2.8v */
+		powerdown-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+
+		port {
+			/* MIPI CSI-2 bus endpoint */
+			ov5640_to_mipi_csi2: endpoint {
+				remote-endpoint = <&imx8mm_mipi_csi_in>;
+				clock-lanes = <0>;
+				data-lanes = <1 2>;
+			};
+		};
+	};
 };
 
 &i2c4 {
@@ -141,6 +179,18 @@ pca6416_1: gpio@21 {
 	};
 };
 
+&mipi_csi {
+	status = "okay";
+	ports {
+		port@0 {
+			imx8mm_mipi_csi_in: endpoint {
+				remote-endpoint = <&ov5640_to_mipi_csi2>;
+				data-lanes = <1 2>;
+			};
+		};
+	};
+};
+
 &sai3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_sai3>;
@@ -209,6 +259,14 @@ MX8MM_IOMUXC_SAI3_RXFS_GPIO4_IO28	0x41
 		>;
 	};
 
+	pinctrl_ov5640: ov5640grp {
+		fsl,pins = <
+			MX8MM_IOMUXC_GPIO1_IO07_GPIO1_IO7		0x19
+			MX8MM_IOMUXC_GPIO1_IO06_GPIO1_IO6		0x19
+			MX8MM_IOMUXC_GPIO1_IO14_CCMSRCGPCMIX_CLKO1	0x59
+		>;
+	};
+
 	pinctrl_pcal6414: pcal6414-gpiogrp {
 		fsl,pins = <
 			MX8MM_IOMUXC_SAI2_MCLK_GPIO4_IO27		0x19
-- 
2.32.0


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

* [PATCH V2 5/5] arm64: defconfig: Enable OV5640
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
                   ` (2 preceding siblings ...)
  2021-11-06 15:54 ` [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera Adam Ford
@ 2021-11-06 15:54 ` Adam Ford
  2021-11-07 13:03   ` Fabio Estevam
  2021-11-07 12:58 ` [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Fabio Estevam
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-06 15:54 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: tharvey, frieder.schrempf, linux-media, laurent.pinchart, aford,
	cstevens, jagan, Adam Ford, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	linux-kernel

The Beacon EmbeddedWorks imx8mm development kit has a TD Next 5640
Camera.  Enable the OV5640 driver to use the camera.

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

 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index bc261cf2ef5a..4c1eb9aae5e5 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -668,6 +668,7 @@ CONFIG_VIDEO_QCOM_VENUS=m
 CONFIG_SDR_PLATFORM_DRIVERS=y
 CONFIG_VIDEO_RCAR_DRIF=m
 CONFIG_VIDEO_IMX219=m
+CONFIG_VIDEO_OV5640=m
 CONFIG_VIDEO_OV5645=m
 CONFIG_VIDEO_QCOM_CAMSS=m
 CONFIG_DRM=m
-- 
2.32.0


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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
                   ` (3 preceding siblings ...)
  2021-11-06 15:54 ` [PATCH V2 5/5] arm64: defconfig: Enable OV5640 Adam Ford
@ 2021-11-07 12:58 ` Fabio Estevam
  2021-11-08  8:56 ` Lucas Stach
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 29+ messages in thread
From: Fabio Estevam @ 2021-11-07 12:58 UTC (permalink / raw)
  To: Adam Ford
  Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Tim Harvey, Schrempf Frieder, linux-media, Laurent Pinchart,
	Adam Ford-BE, cstevens, Jagan Teki, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel

Hi Adam,

On Sat, Nov 6, 2021 at 12:54 PM Adam Ford <aford173@gmail.com> wrote:
>
> Most of the blk-ctrl reset bits are found in one register, however
> there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> out of reset or the MIPI_CSI hangs.
>
> Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

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

* Re: [PATCH V2 2/5] arm64: dts: imx8mm: Add CSI nodes
  2021-11-06 15:54 ` [PATCH V2 2/5] arm64: dts: imx8mm: Add CSI nodes Adam Ford
@ 2021-11-07 12:58   ` Fabio Estevam
  0 siblings, 0 replies; 29+ messages in thread
From: Fabio Estevam @ 2021-11-07 12:58 UTC (permalink / raw)
  To: Adam Ford
  Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Tim Harvey, Schrempf Frieder, linux-media, Laurent Pinchart,
	Adam Ford-BE, cstevens, Jagan Teki, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel

On Sat, Nov 6, 2021 at 12:54 PM Adam Ford <aford173@gmail.com> wrote:
>
> There is a csi bridge and csis interface that tie together
> to allow csi2 capture.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-By: Tim Harvey <tharvey@gateworks.com>
> Tested-By: Tim Harvey <tharvey@gateworks.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

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

* Re: [PATCH V2 3/5] arm64: defconfig: Enable VIDEO_IMX_MEDIA
  2021-11-06 15:54 ` [PATCH V2 3/5] arm64: defconfig: Enable VIDEO_IMX_MEDIA Adam Ford
@ 2021-11-07 13:01   ` Fabio Estevam
  0 siblings, 0 replies; 29+ messages in thread
From: Fabio Estevam @ 2021-11-07 13:01 UTC (permalink / raw)
  To: Adam Ford
  Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Tim Harvey, Schrempf Frieder, linux-media, Laurent Pinchart,
	Adam Ford-BE, cstevens, Jagan Teki, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel

On Sat, Nov 6, 2021 at 12:54 PM Adam Ford <aford173@gmail.com> wrote:
>
> To use a camera, the CSIS and CSI drivers need to be enabled with
> VIDEO_IMX_MEDIA.

Reviewed-by: Fabio Estevam <festevam@gmail.com>

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

* Re: [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-06 15:54 ` [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera Adam Ford
@ 2021-11-07 13:01   ` Fabio Estevam
  2021-11-21 23:18   ` Laurent Pinchart
  1 sibling, 0 replies; 29+ messages in thread
From: Fabio Estevam @ 2021-11-07 13:01 UTC (permalink / raw)
  To: Adam Ford
  Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Tim Harvey, Schrempf Frieder, linux-media, Laurent Pinchart,
	Adam Ford-BE, cstevens, Jagan Teki, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	Catalin Marinas, Will Deacon, Lucas Stach, Peng Fan,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel

On Sat, Nov 6, 2021 at 12:54 PM Adam Ford <aford173@gmail.com> wrote:
>
> The baseboard has support for a TDNext 5640 Camera which
> uses an OV5640 connected to a 2-lane CSI2 interface.
>
> With the CSI and mipi_csi2 drivers pointing to an OV5640 camera, the media
> pipeline can be configured with the following:
>
>     media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-csis.0':0[1]"
>
> The camera and various nodes in the pipeline can be configured for UYVY:
>     media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480 field:none]"
>     media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480 field:none]"
>
> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

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

* Re: [PATCH V2 5/5] arm64: defconfig: Enable OV5640
  2021-11-06 15:54 ` [PATCH V2 5/5] arm64: defconfig: Enable OV5640 Adam Ford
@ 2021-11-07 13:03   ` Fabio Estevam
  0 siblings, 0 replies; 29+ messages in thread
From: Fabio Estevam @ 2021-11-07 13:03 UTC (permalink / raw)
  To: Adam Ford
  Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Tim Harvey, Schrempf Frieder, linux-media, Laurent Pinchart,
	Adam Ford-BE, cstevens, Jagan Teki, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel

On Sat, Nov 6, 2021 at 12:54 PM Adam Ford <aford173@gmail.com> wrote:
>
> The Beacon EmbeddedWorks imx8mm development kit has a TD Next 5640
> Camera.  Enable the OV5640 driver to use the camera.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>

Maybe this one could be squashed with the other defconfig patch. Either way:

Reviewed-by: Fabio Estevam <festevam@gmail.com>

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
                   ` (4 preceding siblings ...)
  2021-11-07 12:58 ` [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Fabio Estevam
@ 2021-11-08  8:56 ` Lucas Stach
  2021-11-12  6:54 ` Jagan Teki
  2021-11-21 22:25 ` Laurent Pinchart
  7 siblings, 0 replies; 29+ messages in thread
From: Lucas Stach @ 2021-11-08  8:56 UTC (permalink / raw)
  To: Adam Ford, linux-arm-kernel
  Cc: tharvey, frieder.schrempf, linux-media, laurent.pinchart, aford,
	cstevens, jagan, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, devicetree, linux-kernel

Am Samstag, dem 06.11.2021 um 10:54 -0500 schrieb Adam Ford:
> Most of the blk-ctrl reset bits are found in one register, however
> there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> out of reset or the MIPI_CSI hangs.
> 
> Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> 
> V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
>      Rename the new register to mipi_phy_rst_mask
>      Encapsulate the edits to this register with an if-statement
> 
>  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> index 519b3651d1d9..581eb4bc7f7d 100644
> --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> @@ -17,6 +17,7 @@
>  
>  #define BLK_SFT_RSTN	0x0
>  #define BLK_CLK_EN	0x4
> +#define BLK_MIPI_RESET_DIV	0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
>  
>  struct imx8m_blk_ctrl_domain;
>  
> @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
>  	const char *gpc_name;
>  	u32 rst_mask;
>  	u32 clk_mask;
> +
> +	/*
> +	 * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> +	 * which is used to control the reset for the MIPI Phy.
> +	 * Since it's only present in certain circumstances,
> +	 * an if-statement should be used before setting and clearing this
> +	 * register.
> +	 */
> +	u32 mipi_phy_rst_mask;
>  };
>  
>  #define DOMAIN_MAX_CLKS 3
> @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  
>  	/* put devices into reset */
>  	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> +	if (data->mipi_phy_rst_mask)
> +		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
>  
>  	/* enable upstream and blk-ctrl clocks to allow reset to propagate */
>  	ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  
>  	/* release reset */
>  	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> +	if (data->mipi_phy_rst_mask)
> +		regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
>  
>  	/* disable upstream clocks */
>  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
>  	struct imx8m_blk_ctrl *bc = domain->bc;
>  
>  	/* put devices into reset and disable clocks */
> +	if (data->mipi_phy_rst_mask)
> +		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> +
>  	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
>  	regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
>  
> @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
>  		.gpc_name = "mipi-csi",
>  		.rst_mask = BIT(3) | BIT(4),
>  		.clk_mask = BIT(10) | BIT(11),
> +		.mipi_phy_rst_mask = BIT(16) | BIT(17),
>  	},
>  };
>  



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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
                   ` (5 preceding siblings ...)
  2021-11-08  8:56 ` Lucas Stach
@ 2021-11-12  6:54 ` Jagan Teki
  2021-11-19 23:51   ` Tim Harvey
  2021-11-21 22:25 ` Laurent Pinchart
  7 siblings, 1 reply; 29+ messages in thread
From: Jagan Teki @ 2021-11-12  6:54 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, tharvey, frieder.schrempf, linux-media,
	laurent.pinchart, aford, cstevens, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, Peng Fan,
	Lucas Stach, devicetree, linux-kernel, Marek Vasut

On Sat, Nov 6, 2021 at 9:24 PM Adam Ford <aford173@gmail.com> wrote:
>
> Most of the blk-ctrl reset bits are found in one register, however
> there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> out of reset or the MIPI_CSI hangs.
>
> Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
>
> V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
>      Rename the new register to mipi_phy_rst_mask
>      Encapsulate the edits to this register with an if-statement

This is DPHY reset mask, not sure we can handle this via blk-ctrl.
Marek has similar patch to support this [1]. we need to phandle the
phy in host node in order to work this.

However this current patch change seems directly handling dphy reset
which indeed fine me as well.

>
>  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> index 519b3651d1d9..581eb4bc7f7d 100644
> --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> @@ -17,6 +17,7 @@
>
>  #define BLK_SFT_RSTN   0x0
>  #define BLK_CLK_EN     0x4
> +#define BLK_MIPI_RESET_DIV     0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
>
>  struct imx8m_blk_ctrl_domain;
>
> @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
>         const char *gpc_name;
>         u32 rst_mask;
>         u32 clk_mask;
> +
> +       /*
> +        * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> +        * which is used to control the reset for the MIPI Phy.
> +        * Since it's only present in certain circumstances,
> +        * an if-statement should be used before setting and clearing this
> +        * register.
> +        */
> +       u32 mipi_phy_rst_mask;

May be dphy_rst_mask (above comment may not be required, as it
understand directly with commit message).

>  };
>
>  #define DOMAIN_MAX_CLKS 3
> @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>
>         /* put devices into reset */
>         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> +       if (data->mipi_phy_rst_mask)
> +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
>
>         /* enable upstream and blk-ctrl clocks to allow reset to propagate */
>         ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>
>         /* release reset */
>         regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> +       if (data->mipi_phy_rst_mask)
> +               regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
>
>         /* disable upstream clocks */
>         clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
>         struct imx8m_blk_ctrl *bc = domain->bc;
>
>         /* put devices into reset and disable clocks */
> +       if (data->mipi_phy_rst_mask)
> +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> +
>         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
>         regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
>
> @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
>                 .gpc_name = "mipi-csi",
>                 .rst_mask = BIT(3) | BIT(4),
>                 .clk_mask = BIT(10) | BIT(11),
> +               .mipi_phy_rst_mask = BIT(16) | BIT(17),

DPHY has BIT(17) for Master reset and BIT(16) for Slave reset. I think
we just need master reset to enable. I've tested only BIT(17) on
mipi-dsi gpc and it is working.

Jagan.

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-12  6:54 ` Jagan Teki
@ 2021-11-19 23:51   ` Tim Harvey
  2021-11-20 18:33     ` Adam Ford
  0 siblings, 1 reply; 29+ messages in thread
From: Tim Harvey @ 2021-11-19 23:51 UTC (permalink / raw)
  To: Jagan Teki
  Cc: Adam Ford, Linux ARM Mailing List, Schrempf Frieder, linux-media,
	Laurent Pinchart, Adam Ford-BE, cstevens, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, Peng Fan,
	Lucas Stach, Device Tree Mailing List, open list, Marek Vasut

On Thu, Nov 11, 2021 at 10:55 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Sat, Nov 6, 2021 at 9:24 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > Most of the blk-ctrl reset bits are found in one register, however
> > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > out of reset or the MIPI_CSI hangs.
> >
> > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >
> > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> >      Rename the new register to mipi_phy_rst_mask
> >      Encapsulate the edits to this register with an if-statement
>
> This is DPHY reset mask, not sure we can handle this via blk-ctrl.
> Marek has similar patch to support this [1]. we need to phandle the
> phy in host node in order to work this.
>
> However this current patch change seems directly handling dphy reset
> which indeed fine me as well.
>
> >
> >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > index 519b3651d1d9..581eb4bc7f7d 100644
> > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > @@ -17,6 +17,7 @@
> >
> >  #define BLK_SFT_RSTN   0x0
> >  #define BLK_CLK_EN     0x4
> > +#define BLK_MIPI_RESET_DIV     0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> >
> >  struct imx8m_blk_ctrl_domain;
> >
> > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> >         const char *gpc_name;
> >         u32 rst_mask;
> >         u32 clk_mask;
> > +
> > +       /*
> > +        * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > +        * which is used to control the reset for the MIPI Phy.
> > +        * Since it's only present in certain circumstances,
> > +        * an if-statement should be used before setting and clearing this
> > +        * register.
> > +        */
> > +       u32 mipi_phy_rst_mask;
>
> May be dphy_rst_mask (above comment may not be required, as it
> understand directly with commit message).
>
> >  };
> >
> >  #define DOMAIN_MAX_CLKS 3
> > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >
> >         /* put devices into reset */
> >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > +       if (data->mipi_phy_rst_mask)
> > +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> >
> >         /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> >         ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >
> >         /* release reset */
> >         regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > +       if (data->mipi_phy_rst_mask)
> > +               regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> >
> >         /* disable upstream clocks */
> >         clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> >         struct imx8m_blk_ctrl *bc = domain->bc;
> >
> >         /* put devices into reset and disable clocks */
> > +       if (data->mipi_phy_rst_mask)
> > +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > +
> >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> >         regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> >
> > @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
> >                 .gpc_name = "mipi-csi",
> >                 .rst_mask = BIT(3) | BIT(4),
> >                 .clk_mask = BIT(10) | BIT(11),
> > +               .mipi_phy_rst_mask = BIT(16) | BIT(17),
>
> DPHY has BIT(17) for Master reset and BIT(16) for Slave reset. I think
> we just need master reset to enable. I've tested only BIT(17) on
> mipi-dsi gpc and it is working.
>

Jagan,

In my testing I had to use BIT(16) | BIT(17) in order to capture via CSI.

Best regards,

Tim

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-19 23:51   ` Tim Harvey
@ 2021-11-20 18:33     ` Adam Ford
  2021-11-21 23:13       ` Laurent Pinchart
  0 siblings, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-20 18:33 UTC (permalink / raw)
  To: Tim Harvey
  Cc: Jagan Teki, Linux ARM Mailing List, Schrempf Frieder,
	linux-media, Laurent Pinchart, Adam Ford-BE, cstevens,
	Rob Herring, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, Catalin Marinas, Will Deacon,
	Peng Fan, Lucas Stach, Device Tree Mailing List, open list,
	Marek Vasut

On Fri, Nov 19, 2021 at 5:51 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Thu, Nov 11, 2021 at 10:55 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Sat, Nov 6, 2021 at 9:24 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > Most of the blk-ctrl reset bits are found in one register, however
> > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > out of reset or the MIPI_CSI hangs.
> > >
> > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > >
> > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > >      Rename the new register to mipi_phy_rst_mask
> > >      Encapsulate the edits to this register with an if-statement
> >
> > This is DPHY reset mask, not sure we can handle this via blk-ctrl.
> > Marek has similar patch to support this [1]. we need to phandle the
> > phy in host node in order to work this.
> >
> > However this current patch change seems directly handling dphy reset
> > which indeed fine me as well.
> >
> > >
> > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > index 519b3651d1d9..581eb4bc7f7d 100644
> > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > @@ -17,6 +17,7 @@
> > >
> > >  #define BLK_SFT_RSTN   0x0
> > >  #define BLK_CLK_EN     0x4
> > > +#define BLK_MIPI_RESET_DIV     0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > >
> > >  struct imx8m_blk_ctrl_domain;
> > >
> > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > >         const char *gpc_name;
> > >         u32 rst_mask;
> > >         u32 clk_mask;
> > > +
> > > +       /*
> > > +        * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > +        * which is used to control the reset for the MIPI Phy.
> > > +        * Since it's only present in certain circumstances,
> > > +        * an if-statement should be used before setting and clearing this
> > > +        * register.
> > > +        */
> > > +       u32 mipi_phy_rst_mask;
> >
> > May be dphy_rst_mask (above comment may not be required, as it
> > understand directly with commit message).
> >
> > >  };
> > >
> > >  #define DOMAIN_MAX_CLKS 3
> > > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > >
> > >         /* put devices into reset */
> > >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > +       if (data->mipi_phy_rst_mask)
> > > +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > >
> > >         /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > >         ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > >
> > >         /* release reset */
> > >         regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > +       if (data->mipi_phy_rst_mask)
> > > +               regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > >
> > >         /* disable upstream clocks */
> > >         clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > >         struct imx8m_blk_ctrl *bc = domain->bc;
> > >
> > >         /* put devices into reset and disable clocks */
> > > +       if (data->mipi_phy_rst_mask)
> > > +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > +
> > >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > >         regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> > >
> > > @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
> > >                 .gpc_name = "mipi-csi",
> > >                 .rst_mask = BIT(3) | BIT(4),
> > >                 .clk_mask = BIT(10) | BIT(11),
> > > +               .mipi_phy_rst_mask = BIT(16) | BIT(17),
> >
> > DPHY has BIT(17) for Master reset and BIT(16) for Slave reset. I think
> > we just need master reset to enable. I've tested only BIT(17) on
> > mipi-dsi gpc and it is working.
> >
>
> Jagan,
>
> In my testing I had to use BIT(16) | BIT(17) in order to capture via CSI.

Lucas,

Based on this, should I redo the patch but without clearing bits 16
and 17?  It seems like both DSI and CSI need at least bit 17, and CSI
appears to need both.  If we clear them, we risk stomping on one
peripheral or another.  If we need both, in theory, we could drop the
need for a DPHY driver on the DSI side.

adam
>
> Best regards,
>
> Tim

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
                   ` (6 preceding siblings ...)
  2021-11-12  6:54 ` Jagan Teki
@ 2021-11-21 22:25 ` Laurent Pinchart
  2021-11-21 22:30   ` Laurent Pinchart
  2021-11-23 13:59   ` Adam Ford
  7 siblings, 2 replies; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-21 22:25 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, tharvey, frieder.schrempf, linux-media, aford,
	cstevens, jagan, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	linux-kernel

Hi Adam,

On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> Most of the blk-ctrl reset bits are found in one register, however
> there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> out of reset or the MIPI_CSI hangs.
> 
> Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> 
> V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
>      Rename the new register to mipi_phy_rst_mask
>      Encapsulate the edits to this register with an if-statement
> 
>  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> index 519b3651d1d9..581eb4bc7f7d 100644
> --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> @@ -17,6 +17,7 @@
>  
>  #define BLK_SFT_RSTN	0x0
>  #define BLK_CLK_EN	0x4
> +#define BLK_MIPI_RESET_DIV	0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
>  
>  struct imx8m_blk_ctrl_domain;
>  
> @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
>  	const char *gpc_name;
>  	u32 rst_mask;
>  	u32 clk_mask;
> +
> +	/*
> +	 * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> +	 * which is used to control the reset for the MIPI Phy.
> +	 * Since it's only present in certain circumstances,
> +	 * an if-statement should be used before setting and clearing this
> +	 * register.
> +	 */
> +	u32 mipi_phy_rst_mask;
>  };
>  
>  #define DOMAIN_MAX_CLKS 3
> @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  
>  	/* put devices into reset */
>  	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> +	if (data->mipi_phy_rst_mask)
> +		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
>  
>  	/* enable upstream and blk-ctrl clocks to allow reset to propagate */
>  	ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  
>  	/* release reset */
>  	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> +	if (data->mipi_phy_rst_mask)
> +		regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
>  
>  	/* disable upstream clocks */
>  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
>  	struct imx8m_blk_ctrl *bc = domain->bc;
>  
>  	/* put devices into reset and disable clocks */
> +	if (data->mipi_phy_rst_mask)
> +		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> +

Is it the best option to enable/disable both the master and slave MIPI
DPHY, regardless of whether they're used or not ? Or would it be better
to implement a reset controller to expose the two resets independently,
and acquire them from the corresponding display and camera drivers ?

>  	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
>  	regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
>  
> @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
>  		.gpc_name = "mipi-csi",
>  		.rst_mask = BIT(3) | BIT(4),
>  		.clk_mask = BIT(10) | BIT(11),
> +		.mipi_phy_rst_mask = BIT(16) | BIT(17),
>  	},
>  };
>  

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-21 22:25 ` Laurent Pinchart
@ 2021-11-21 22:30   ` Laurent Pinchart
  2021-11-23 13:59   ` Adam Ford
  1 sibling, 0 replies; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-21 22:30 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, tharvey, frieder.schrempf, linux-media, aford,
	cstevens, jagan, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	linux-kernel

Hi Adam,

On Mon, Nov 22, 2021 at 12:25:31AM +0200, Laurent Pinchart wrote:
> On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > Most of the blk-ctrl reset bits are found in one register, however
> > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > out of reset or the MIPI_CSI hangs.
> > 
> > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > 
> > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> >      Rename the new register to mipi_phy_rst_mask
> >      Encapsulate the edits to this register with an if-statement
> > 
> >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > index 519b3651d1d9..581eb4bc7f7d 100644
> > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > @@ -17,6 +17,7 @@
> >  
> >  #define BLK_SFT_RSTN	0x0
> >  #define BLK_CLK_EN	0x4
> > +#define BLK_MIPI_RESET_DIV	0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> >  
> >  struct imx8m_blk_ctrl_domain;
> >  
> > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> >  	const char *gpc_name;
> >  	u32 rst_mask;
> >  	u32 clk_mask;
> > +
> > +	/*
> > +	 * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > +	 * which is used to control the reset for the MIPI Phy.
> > +	 * Since it's only present in certain circumstances,
> > +	 * an if-statement should be used before setting and clearing this
> > +	 * register.
> > +	 */
> > +	u32 mipi_phy_rst_mask;
> >  };
> >  
> >  #define DOMAIN_MAX_CLKS 3
> > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >  
> >  	/* put devices into reset */
> >  	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > +	if (data->mipi_phy_rst_mask)
> > +		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> >  
> >  	/* enable upstream and blk-ctrl clocks to allow reset to propagate */
> >  	ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >  
> >  	/* release reset */
> >  	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > +	if (data->mipi_phy_rst_mask)
> > +		regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> >  
> >  	/* disable upstream clocks */
> >  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> >  	struct imx8m_blk_ctrl *bc = domain->bc;
> >  
> >  	/* put devices into reset and disable clocks */
> > +	if (data->mipi_phy_rst_mask)
> > +		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > +
> 
> Is it the best option to enable/disable both the master and slave MIPI
> DPHY, regardless of whether they're used or not ? Or would it be better
> to implement a reset controller to expose the two resets independently,
> and acquire them from the corresponding display and camera drivers ?

And I've now seen this has been raised by Jagan. I'll reply to that
e-mail.

> >  	regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> >  	regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> >  
> > @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
> >  		.gpc_name = "mipi-csi",
> >  		.rst_mask = BIT(3) | BIT(4),
> >  		.clk_mask = BIT(10) | BIT(11),
> > +		.mipi_phy_rst_mask = BIT(16) | BIT(17),
> >  	},
> >  };
> >  

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-20 18:33     ` Adam Ford
@ 2021-11-21 23:13       ` Laurent Pinchart
  0 siblings, 0 replies; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-21 23:13 UTC (permalink / raw)
  To: Adam Ford
  Cc: Tim Harvey, Jagan Teki, Linux ARM Mailing List, Schrempf Frieder,
	linux-media, Adam Ford-BE, cstevens, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, Peng Fan,
	Lucas Stach, Device Tree Mailing List, open list, Marek Vasut

Hi Adam,

On Sat, Nov 20, 2021 at 12:33:25PM -0600, Adam Ford wrote:
> On Fri, Nov 19, 2021 at 5:51 PM Tim Harvey wrote:
> > On Thu, Nov 11, 2021 at 10:55 PM Jagan Teki wrote:
> > > On Sat, Nov 6, 2021 at 9:24 PM Adam Ford wrote:
> > > >
> > > > Most of the blk-ctrl reset bits are found in one register, however
> > > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > > out of reset or the MIPI_CSI hangs.
> > > >
> > > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > ---
> > > >
> > > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > > >      Rename the new register to mipi_phy_rst_mask
> > > >      Encapsulate the edits to this register with an if-statement
> > >
> > > This is DPHY reset mask, not sure we can handle this via blk-ctrl.
> > > Marek has similar patch to support this [1]. we need to phandle the
> > > phy in host node in order to work this.
> > >
> > > However this current patch change seems directly handling dphy reset
> > > which indeed fine me as well.
> > >
> > > >
> > > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > index 519b3651d1d9..581eb4bc7f7d 100644
> > > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > @@ -17,6 +17,7 @@
> > > >
> > > >  #define BLK_SFT_RSTN   0x0
> > > >  #define BLK_CLK_EN     0x4
> > > > +#define BLK_MIPI_RESET_DIV     0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > > >
> > > >  struct imx8m_blk_ctrl_domain;
> > > >
> > > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > > >         const char *gpc_name;
> > > >         u32 rst_mask;
> > > >         u32 clk_mask;
> > > > +
> > > > +       /*
> > > > +        * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > > +        * which is used to control the reset for the MIPI Phy.
> > > > +        * Since it's only present in certain circumstances,
> > > > +        * an if-statement should be used before setting and clearing this
> > > > +        * register.
> > > > +        */
> > > > +       u32 mipi_phy_rst_mask;
> > >
> > > May be dphy_rst_mask (above comment may not be required, as it
> > > understand directly with commit message).
> > >
> > > >  };
> > > >
> > > >  #define DOMAIN_MAX_CLKS 3
> > > > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > >
> > > >         /* put devices into reset */
> > > >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > +       if (data->mipi_phy_rst_mask)
> > > > +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > >
> > > >         /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > > >         ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > >
> > > >         /* release reset */
> > > >         regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > +       if (data->mipi_phy_rst_mask)
> > > > +               regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > >
> > > >         /* disable upstream clocks */
> > > >         clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > > >         struct imx8m_blk_ctrl *bc = domain->bc;
> > > >
> > > >         /* put devices into reset and disable clocks */
> > > > +       if (data->mipi_phy_rst_mask)
> > > > +               regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > +
> > > >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > >         regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> > > >
> > > > @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
> > > >                 .gpc_name = "mipi-csi",
> > > >                 .rst_mask = BIT(3) | BIT(4),
> > > >                 .clk_mask = BIT(10) | BIT(11),
> > > > +               .mipi_phy_rst_mask = BIT(16) | BIT(17),
> > >
> > > DPHY has BIT(17) for Master reset and BIT(16) for Slave reset. I think
> > > we just need master reset to enable. I've tested only BIT(17) on
> > > mipi-dsi gpc and it is working.
> > >
> >
> > Jagan,
> >
> > In my testing I had to use BIT(16) | BIT(17) in order to capture via CSI.
> 
> Lucas,
> 
> Based on this, should I redo the patch but without clearing bits 16
> and 17?  It seems like both DSI and CSI need at least bit 17, and CSI
> appears to need both.  If we clear them, we risk stomping on one
> peripheral or another.  If we need both, in theory, we could drop the
> need for a DPHY driver on the DSI side.

I've tested camera on the i.MX8MM with an NXP BSP v5.4-based kernel.
Register BLK_MIPI_RESET_DIV is set to 0x00030000 by default. The camera
still works if I clear bit 17, and doesn't if I clear bit 16.

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-06 15:54 ` [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera Adam Ford
  2021-11-07 13:01   ` Fabio Estevam
@ 2021-11-21 23:18   ` Laurent Pinchart
  2021-11-22  3:07     ` Adam Ford
  1 sibling, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-21 23:18 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-arm-kernel, tharvey, frieder.schrempf, linux-media, aford,
	cstevens, jagan, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Lucas Stach, Peng Fan, devicetree,
	linux-kernel

Hi Adam,

Thank you for the patch.

On Sat, Nov 06, 2021 at 10:54:26AM -0500, Adam Ford wrote:
> The baseboard has support for a TDNext 5640 Camera which
> uses an OV5640 connected to a 2-lane CSI2 interface.
> 
> With the CSI and mipi_csi2 drivers pointing to an OV5640 camera, the media
> pipeline can be configured with the following:
> 
>     media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-csis.0':0[1]"
> 
> The camera and various nodes in the pipeline can be configured for UYVY:
>     media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480 field:none]"
>     media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480 field:none]"
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>

As the ov5640 is on an add-on module, would a DT overlay be better ?

> ---
> V2:  No change
> 
>  .../freescale/imx8mm-beacon-baseboard.dtsi    | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> index 6f5e63696ec0..0fb95f4a5e78 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> @@ -43,6 +43,16 @@ reg_audio: regulator-audio {
>  		enable-active-high;
>  	};
>  
> +	reg_camera: regulator-camera {
> +		compatible = "regulator-fixed";
> +		regulator-name = "mipi_pwr";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		gpio = <&pca6416_1 0 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		startup-delay-us = <100000>;
> +	};
> +
>  	reg_usdhc2_vmmc: regulator-usdhc2 {
>  		compatible = "regulator-fixed";
>  		regulator-name = "VSD_3V3";
> @@ -67,6 +77,10 @@ sound {
>  	};
>  };
>  
> +&csi {
> +	status = "okay";
> +};
> +
>  &ecspi2 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_espi2>;
> @@ -90,6 +104,30 @@ &i2c2 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_i2c2>;
>  	status = "okay";
> +
> +	camera@3c {
> +		compatible = "ovti,ov5640";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ov5640>;
> +		reg = <0x3c>;
> +		clocks = <&clk IMX8MM_CLK_CLKO1>;
> +		clock-names = "xclk";
> +		assigned-clocks = <&clk IMX8MM_CLK_CLKO1>;
> +		assigned-clock-parents = <&clk IMX8MM_CLK_24M>;
> +		assigned-clock-rates = <24000000>;
> +		AVDD-supply = <&reg_camera>;  /* 2.8v */
> +		powerdown-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> +
> +		port {
> +			/* MIPI CSI-2 bus endpoint */
> +			ov5640_to_mipi_csi2: endpoint {
> +				remote-endpoint = <&imx8mm_mipi_csi_in>;
> +				clock-lanes = <0>;
> +				data-lanes = <1 2>;
> +			};
> +		};
> +	};
>  };
>  
>  &i2c4 {
> @@ -141,6 +179,18 @@ pca6416_1: gpio@21 {
>  	};
>  };
>  
> +&mipi_csi {
> +	status = "okay";
> +	ports {
> +		port@0 {
> +			imx8mm_mipi_csi_in: endpoint {
> +				remote-endpoint = <&ov5640_to_mipi_csi2>;
> +				data-lanes = <1 2>;
> +			};
> +		};
> +	};
> +};
> +
>  &sai3 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_sai3>;
> @@ -209,6 +259,14 @@ MX8MM_IOMUXC_SAI3_RXFS_GPIO4_IO28	0x41
>  		>;
>  	};
>  
> +	pinctrl_ov5640: ov5640grp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_GPIO1_IO07_GPIO1_IO7		0x19
> +			MX8MM_IOMUXC_GPIO1_IO06_GPIO1_IO6		0x19
> +			MX8MM_IOMUXC_GPIO1_IO14_CCMSRCGPCMIX_CLKO1	0x59
> +		>;
> +	};
> +
>  	pinctrl_pcal6414: pcal6414-gpiogrp {
>  		fsl,pins = <
>  			MX8MM_IOMUXC_SAI2_MCLK_GPIO4_IO27		0x19

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-21 23:18   ` Laurent Pinchart
@ 2021-11-22  3:07     ` Adam Ford
  2021-11-23  0:15       ` Laurent Pinchart
  0 siblings, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-22  3:07 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: arm-soc, Tim Harvey, Schrempf Frieder, linux-media, Adam Ford-BE,
	cstevens, Jagan Teki, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Lucas Stach, Peng Fan, devicetree,
	Linux Kernel Mailing List

On Sun, Nov 21, 2021 at 5:18 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Adam,
>
> Thank you for the patch.
>
> On Sat, Nov 06, 2021 at 10:54:26AM -0500, Adam Ford wrote:
> > The baseboard has support for a TDNext 5640 Camera which
> > uses an OV5640 connected to a 2-lane CSI2 interface.
> >
> > With the CSI and mipi_csi2 drivers pointing to an OV5640 camera, the media
> > pipeline can be configured with the following:
> >
> >     media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-csis.0':0[1]"
> >
> > The camera and various nodes in the pipeline can be configured for UYVY:
> >     media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480 field:none]"
> >     media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480 field:none]"
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
>
> As the ov5640 is on an add-on module, would a DT overlay be better ?

At least for the Beacon / LogicPD boards, I would prefer to avoid the
overlays.  We have an i.M6Q and an OMAP3 board with cameras enabled in
our development kit device trees.  If the cameras are not connected,
they just display a message that the cameras are not communicating and
move on.  I'm OK with that.

>
> > ---
> > V2:  No change
> >
> >  .../freescale/imx8mm-beacon-baseboard.dtsi    | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> > index 6f5e63696ec0..0fb95f4a5e78 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> > @@ -43,6 +43,16 @@ reg_audio: regulator-audio {
> >               enable-active-high;
> >       };
> >
> > +     reg_camera: regulator-camera {
> > +             compatible = "regulator-fixed";
> > +             regulator-name = "mipi_pwr";
> > +             regulator-min-microvolt = <2800000>;
> > +             regulator-max-microvolt = <2800000>;
> > +             gpio = <&pca6416_1 0 GPIO_ACTIVE_HIGH>;
> > +             enable-active-high;
> > +             startup-delay-us = <100000>;
> > +     };
> > +
> >       reg_usdhc2_vmmc: regulator-usdhc2 {
> >               compatible = "regulator-fixed";
> >               regulator-name = "VSD_3V3";
> > @@ -67,6 +77,10 @@ sound {
> >       };
> >  };
> >
> > +&csi {
> > +     status = "okay";
> > +};
> > +
> >  &ecspi2 {
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&pinctrl_espi2>;
> > @@ -90,6 +104,30 @@ &i2c2 {
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&pinctrl_i2c2>;
> >       status = "okay";
> > +
> > +     camera@3c {
> > +             compatible = "ovti,ov5640";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&pinctrl_ov5640>;
> > +             reg = <0x3c>;
> > +             clocks = <&clk IMX8MM_CLK_CLKO1>;
> > +             clock-names = "xclk";
> > +             assigned-clocks = <&clk IMX8MM_CLK_CLKO1>;
> > +             assigned-clock-parents = <&clk IMX8MM_CLK_24M>;
> > +             assigned-clock-rates = <24000000>;
> > +             AVDD-supply = <&reg_camera>;  /* 2.8v */
> > +             powerdown-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> > +             reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> > +
> > +             port {
> > +                     /* MIPI CSI-2 bus endpoint */
> > +                     ov5640_to_mipi_csi2: endpoint {
> > +                             remote-endpoint = <&imx8mm_mipi_csi_in>;
> > +                             clock-lanes = <0>;
> > +                             data-lanes = <1 2>;
> > +                     };
> > +             };
> > +     };
> >  };
> >
> >  &i2c4 {
> > @@ -141,6 +179,18 @@ pca6416_1: gpio@21 {
> >       };
> >  };
> >
> > +&mipi_csi {
> > +     status = "okay";
> > +     ports {
> > +             port@0 {
> > +                     imx8mm_mipi_csi_in: endpoint {
> > +                             remote-endpoint = <&ov5640_to_mipi_csi2>;
> > +                             data-lanes = <1 2>;
> > +                     };
> > +             };
> > +     };
> > +};
> > +
> >  &sai3 {
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&pinctrl_sai3>;
> > @@ -209,6 +259,14 @@ MX8MM_IOMUXC_SAI3_RXFS_GPIO4_IO28        0x41
> >               >;
> >       };
> >
> > +     pinctrl_ov5640: ov5640grp {
> > +             fsl,pins = <
> > +                     MX8MM_IOMUXC_GPIO1_IO07_GPIO1_IO7               0x19
> > +                     MX8MM_IOMUXC_GPIO1_IO06_GPIO1_IO6               0x19
> > +                     MX8MM_IOMUXC_GPIO1_IO14_CCMSRCGPCMIX_CLKO1      0x59
> > +             >;
> > +     };
> > +
> >       pinctrl_pcal6414: pcal6414-gpiogrp {
> >               fsl,pins = <
> >                       MX8MM_IOMUXC_SAI2_MCLK_GPIO4_IO27               0x19
>
> --
> Regards,
>
> Laurent Pinchart

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

* Re: [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-22  3:07     ` Adam Ford
@ 2021-11-23  0:15       ` Laurent Pinchart
  2021-11-23  7:38         ` (EXT) " Alexander Stein
  0 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-23  0:15 UTC (permalink / raw)
  To: Adam Ford
  Cc: arm-soc, Tim Harvey, Schrempf Frieder, linux-media, Adam Ford-BE,
	cstevens, Jagan Teki, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Lucas Stach, Peng Fan, devicetree,
	Linux Kernel Mailing List

Hi Adam,

On Sun, Nov 21, 2021 at 09:07:26PM -0600, Adam Ford wrote:
> On Sun, Nov 21, 2021 at 5:18 PM Laurent Pinchart wrote:
> > On Sat, Nov 06, 2021 at 10:54:26AM -0500, Adam Ford wrote:
> > > The baseboard has support for a TDNext 5640 Camera which
> > > uses an OV5640 connected to a 2-lane CSI2 interface.
> > >
> > > With the CSI and mipi_csi2 drivers pointing to an OV5640 camera, the media
> > > pipeline can be configured with the following:
> > >
> > >     media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-csis.0':0[1]"
> > >
> > > The camera and various nodes in the pipeline can be configured for UYVY:
> > >     media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480 field:none]"
> > >     media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480 field:none]"
> > >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > As the ov5640 is on an add-on module, would a DT overlay be better ?
> 
> At least for the Beacon / LogicPD boards, I would prefer to avoid the
> overlays.  We have an i.M6Q and an OMAP3 board with cameras enabled in
> our development kit device trees.  If the cameras are not connected,
> they just display a message that the cameras are not communicating and
> move on.  I'm OK with that.

You know the board better than I do, so I won't push against this, but I
still think it may not lead to the best user experience, especially if a
user wanted to connect a different sensor to the development board.

> > > ---
> > > V2:  No change
> > >
> > >  .../freescale/imx8mm-beacon-baseboard.dtsi    | 58 +++++++++++++++++++
> > >  1 file changed, 58 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> > > index 6f5e63696ec0..0fb95f4a5e78 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> > > @@ -43,6 +43,16 @@ reg_audio: regulator-audio {
> > >               enable-active-high;
> > >       };
> > >
> > > +     reg_camera: regulator-camera {
> > > +             compatible = "regulator-fixed";
> > > +             regulator-name = "mipi_pwr";
> > > +             regulator-min-microvolt = <2800000>;
> > > +             regulator-max-microvolt = <2800000>;
> > > +             gpio = <&pca6416_1 0 GPIO_ACTIVE_HIGH>;
> > > +             enable-active-high;
> > > +             startup-delay-us = <100000>;
> > > +     };
> > > +
> > >       reg_usdhc2_vmmc: regulator-usdhc2 {
> > >               compatible = "regulator-fixed";
> > >               regulator-name = "VSD_3V3";
> > > @@ -67,6 +77,10 @@ sound {
> > >       };
> > >  };
> > >
> > > +&csi {
> > > +     status = "okay";
> > > +};
> > > +
> > >  &ecspi2 {
> > >       pinctrl-names = "default";
> > >       pinctrl-0 = <&pinctrl_espi2>;
> > > @@ -90,6 +104,30 @@ &i2c2 {
> > >       pinctrl-names = "default";
> > >       pinctrl-0 = <&pinctrl_i2c2>;
> > >       status = "okay";
> > > +
> > > +     camera@3c {
> > > +             compatible = "ovti,ov5640";
> > > +             pinctrl-names = "default";
> > > +             pinctrl-0 = <&pinctrl_ov5640>;
> > > +             reg = <0x3c>;
> > > +             clocks = <&clk IMX8MM_CLK_CLKO1>;
> > > +             clock-names = "xclk";
> > > +             assigned-clocks = <&clk IMX8MM_CLK_CLKO1>;
> > > +             assigned-clock-parents = <&clk IMX8MM_CLK_24M>;
> > > +             assigned-clock-rates = <24000000>;
> > > +             AVDD-supply = <&reg_camera>;  /* 2.8v */
> > > +             powerdown-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> > > +             reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> > > +
> > > +             port {
> > > +                     /* MIPI CSI-2 bus endpoint */
> > > +                     ov5640_to_mipi_csi2: endpoint {
> > > +                             remote-endpoint = <&imx8mm_mipi_csi_in>;
> > > +                             clock-lanes = <0>;
> > > +                             data-lanes = <1 2>;
> > > +                     };
> > > +             };
> > > +     };
> > >  };
> > >
> > >  &i2c4 {
> > > @@ -141,6 +179,18 @@ pca6416_1: gpio@21 {
> > >       };
> > >  };
> > >
> > > +&mipi_csi {
> > > +     status = "okay";
> > > +     ports {
> > > +             port@0 {
> > > +                     imx8mm_mipi_csi_in: endpoint {
> > > +                             remote-endpoint = <&ov5640_to_mipi_csi2>;
> > > +                             data-lanes = <1 2>;
> > > +                     };
> > > +             };
> > > +     };
> > > +};
> > > +
> > >  &sai3 {
> > >       pinctrl-names = "default";
> > >       pinctrl-0 = <&pinctrl_sai3>;
> > > @@ -209,6 +259,14 @@ MX8MM_IOMUXC_SAI3_RXFS_GPIO4_IO28        0x41
> > >               >;
> > >       };
> > >
> > > +     pinctrl_ov5640: ov5640grp {
> > > +             fsl,pins = <
> > > +                     MX8MM_IOMUXC_GPIO1_IO07_GPIO1_IO7               0x19
> > > +                     MX8MM_IOMUXC_GPIO1_IO06_GPIO1_IO6               0x19
> > > +                     MX8MM_IOMUXC_GPIO1_IO14_CCMSRCGPCMIX_CLKO1      0x59
> > > +             >;
> > > +     };
> > > +
> > >       pinctrl_pcal6414: pcal6414-gpiogrp {
> > >               fsl,pins = <
> > >                       MX8MM_IOMUXC_SAI2_MCLK_GPIO4_IO27               0x19

-- 
Regards,

Laurent Pinchart

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

* Re: (EXT) Re: [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-23  0:15       ` Laurent Pinchart
@ 2021-11-23  7:38         ` Alexander Stein
  2021-11-23  9:47           ` Laurent Pinchart
  0 siblings, 1 reply; 29+ messages in thread
From: Alexander Stein @ 2021-11-23  7:38 UTC (permalink / raw)
  To: Laurent Pinchart, Adam Ford
  Cc: arm-soc, Tim Harvey, Schrempf Frieder, linux-media, Adam Ford-BE,
	cstevens, Jagan Teki, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Lucas Stach, Peng Fan, devicetree,
	Linux Kernel Mailing List

Am Dienstag, dem 23.11.2021 um 02:15 +0200 schrieb Laurent Pinchart:
> Hi Adam,
> 
> On Sun, Nov 21, 2021 at 09:07:26PM -0600, Adam Ford wrote:
> > On Sun, Nov 21, 2021 at 5:18 PM Laurent Pinchart wrote:
> > > On Sat, Nov 06, 2021 at 10:54:26AM -0500, Adam Ford wrote:
> > > > The baseboard has support for a TDNext 5640 Camera which
> > > > uses an OV5640 connected to a 2-lane CSI2 interface.
> > > > 
> > > > With the CSI and mipi_csi2 drivers pointing to an OV5640
> > > > camera, the media
> > > > pipeline can be configured with the following:
> > > > 
> > > >     media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-
> > > > csis.0':0[1]"
> > > > 
> > > > The camera and various nodes in the pipeline can be configured
> > > > for UYVY:
> > > >     media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480
> > > > field:none]"
> > > >     media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480
> > > > field:none]"
> > > > 
> > > > Signed-off-by: Adam Ford <
> > > > aford173@gmail.com
> > > > >
> > > 
> > > As the ov5640 is on an add-on module, would a DT overlay be
> > > better ?
> > 
> > At least for the Beacon / LogicPD boards, I would prefer to avoid
> > the
> > overlays.  We have an i.M6Q and an OMAP3 board with cameras enabled
> > in
> > our development kit device trees.  If the cameras are not
> > connected,
> > they just display a message that the cameras are not communicating
> > and
> > move on.  I'm OK with that.
> 
> You know the board better than I do, so I won't push against this,
> but I
> still think it may not lead to the best user experience, especially
> if a
> user wanted to connect a different sensor to the development board.

I see the advantages of overlays compared to "stacked" .dts files. But
is there any general supported interface how to actually apply an
overlay?
Documentation/devicetree/overlay-notes.rst
states of_overlay_fdt_apply() but there is only exactly one user in-
kernel (rcar-du). Is it expected that the bootloader like u-boot shall
apply the .dtbo files?

Best regards,
Alexander


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

* Re: (EXT) Re: [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-23  7:38         ` (EXT) " Alexander Stein
@ 2021-11-23  9:47           ` Laurent Pinchart
  2021-11-29 18:56             ` Tim Harvey
  0 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-23  9:47 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Adam Ford, arm-soc, Tim Harvey, Schrempf Frieder, linux-media,
	Adam Ford-BE, cstevens, Jagan Teki, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, Lucas Stach,
	Peng Fan, devicetree, Linux Kernel Mailing List

Hi Alexander,

On Tue, Nov 23, 2021 at 08:38:47AM +0100, Alexander Stein wrote:
> Am Dienstag, dem 23.11.2021 um 02:15 +0200 schrieb Laurent Pinchart:
> > On Sun, Nov 21, 2021 at 09:07:26PM -0600, Adam Ford wrote:
> > > On Sun, Nov 21, 2021 at 5:18 PM Laurent Pinchart wrote:
> > > > On Sat, Nov 06, 2021 at 10:54:26AM -0500, Adam Ford wrote:
> > > > > The baseboard has support for a TDNext 5640 Camera which
> > > > > uses an OV5640 connected to a 2-lane CSI2 interface.
> > > > > 
> > > > > With the CSI and mipi_csi2 drivers pointing to an OV5640 camera, the media
> > > > > pipeline can be configured with the following:
> > > > > 
> > > > >     media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-csis.0':0[1]"
> > > > > 
> > > > > The camera and various nodes in the pipeline can be configured for UYVY:
> > > > >     media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480 field:none]"
> > > > >     media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480 field:none]"
> > > > > 
> > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > 
> > > > As the ov5640 is on an add-on module, would a DT overlay be better ?
> > > 
> > > At least for the Beacon / LogicPD boards, I would prefer to avoid the
> > > overlays.  We have an i.M6Q and an OMAP3 board with cameras enabled in
> > > our development kit device trees.  If the cameras are not connected,
> > > they just display a message that the cameras are not communicating and
> > > move on.  I'm OK with that.
> > 
> > You know the board better than I do, so I won't push against this, but I
> > still think it may not lead to the best user experience, especially if a
> > user wanted to connect a different sensor to the development board.
> 
> I see the advantages of overlays compared to "stacked" .dts files. But
> is there any general supported interface how to actually apply an overlay?
> Documentation/devicetree/overlay-notes.rst
> states of_overlay_fdt_apply() but there is only exactly one user in-
> kernel (rcar-du). Is it expected that the bootloader like u-boot shall
> apply the .dtbo files?

I believe the boot loader is expected to apply overlays nowadays, yes.
That's my personal workflow.

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-21 22:25 ` Laurent Pinchart
  2021-11-21 22:30   ` Laurent Pinchart
@ 2021-11-23 13:59   ` Adam Ford
  2021-11-25  5:42     ` Jagan Teki
  1 sibling, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-23 13:59 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: arm-soc, Tim Harvey, Schrempf Frieder, linux-media, Adam Ford-BE,
	cstevens, Jagan Teki, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	Linux Kernel Mailing List

On Sun, Nov 21, 2021 at 4:25 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Adam,
>
> On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > Most of the blk-ctrl reset bits are found in one register, however
> > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > out of reset or the MIPI_CSI hangs.
> >
> > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >
> > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> >      Rename the new register to mipi_phy_rst_mask
> >      Encapsulate the edits to this register with an if-statement
> >
> >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > index 519b3651d1d9..581eb4bc7f7d 100644
> > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > @@ -17,6 +17,7 @@
> >
> >  #define BLK_SFT_RSTN 0x0
> >  #define BLK_CLK_EN   0x4
> > +#define BLK_MIPI_RESET_DIV   0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> >
> >  struct imx8m_blk_ctrl_domain;
> >
> > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> >       const char *gpc_name;
> >       u32 rst_mask;
> >       u32 clk_mask;
> > +
> > +     /*
> > +      * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > +      * which is used to control the reset for the MIPI Phy.
> > +      * Since it's only present in certain circumstances,
> > +      * an if-statement should be used before setting and clearing this
> > +      * register.
> > +      */
> > +     u32 mipi_phy_rst_mask;
> >  };
> >
> >  #define DOMAIN_MAX_CLKS 3
> > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >
> >       /* put devices into reset */
> >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > +     if (data->mipi_phy_rst_mask)
> > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> >
> >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >
> >       /* release reset */
> >       regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > +     if (data->mipi_phy_rst_mask)
> > +             regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> >
> >       /* disable upstream clocks */
> >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> >       struct imx8m_blk_ctrl *bc = domain->bc;
> >
> >       /* put devices into reset and disable clocks */
> > +     if (data->mipi_phy_rst_mask)
> > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > +
>
> Is it the best option to enable/disable both the master and slave MIPI
> DPHY, regardless of whether they're used or not ? Or would it be better
> to implement a reset controller to expose the two resets independently,
> and acquire them from the corresponding display and camera drivers ?

In some early attempts to implement the blk-ctrl driver, there was an
attempt to enable a reset controller, but it caused some hanging and
issues with suspend-resume due to chicken-egg issues where some items
were coming up in the wrong order.  I think the decision was made to
make the resets part of the power domain so it's very clear that the
order of operations.  Lucas might be able to elaborate more on this.

If bits 16 and 17 can act independently and bit 16 only impacts the
CSI  and doesn't require bit 17, it seems reasonable to me to have the
power-domain part of  the CSI, since this would only be enabled when
the CSI is active.  The power domain is idled when the CSI is idled
which would effectively place the phy in and out of reset only
depending on the state of the CSI.  I am guessing this reset bit
should be assigned to DISPBLK_PD_MIPI_CSI and not
DISPBLK_PD_CSI_BRIDGE, but I can run some more tests.

AFAIK, there is no phy driver for the CSI like there is the DSI, so
adding that would require additional work to the CSI driver to work
around this quirk.  We don't have an acceptable DSI driver yet, so I'd
like to push a V3 with just the corresponding bit enabled for MIPI_CSI
after some testing.  FWICT, NXP set both bits 16 and 17 in their ATF
gpc code, and it never gets cleared, so I think having the bit set and
cleared on demand is an improvement.

>
> >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> >       regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> >
> > @@ -488,6 +505,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
> >               .gpc_name = "mipi-csi",
> >               .rst_mask = BIT(3) | BIT(4),
> >               .clk_mask = BIT(10) | BIT(11),
> > +             .mipi_phy_rst_mask = BIT(16) | BIT(17),
> >       },
> >  };
> >

adam

>
> --
> Regards,
>
> Laurent Pinchart

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-23 13:59   ` Adam Ford
@ 2021-11-25  5:42     ` Jagan Teki
  2021-11-25 15:18       ` Adam Ford
  0 siblings, 1 reply; 29+ messages in thread
From: Jagan Teki @ 2021-11-25  5:42 UTC (permalink / raw)
  To: Adam Ford
  Cc: Laurent Pinchart, arm-soc, Tim Harvey, Schrempf Frieder,
	linux-media, Adam Ford-BE, cstevens, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, Peng Fan,
	Lucas Stach, devicetree, Linux Kernel Mailing List

On Tue, Nov 23, 2021 at 7:29 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Sun, Nov 21, 2021 at 4:25 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Adam,
> >
> > On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > > Most of the blk-ctrl reset bits are found in one register, however
> > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > out of reset or the MIPI_CSI hangs.
> > >
> > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > >
> > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > >      Rename the new register to mipi_phy_rst_mask
> > >      Encapsulate the edits to this register with an if-statement
> > >
> > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > index 519b3651d1d9..581eb4bc7f7d 100644
> > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > @@ -17,6 +17,7 @@
> > >
> > >  #define BLK_SFT_RSTN 0x0
> > >  #define BLK_CLK_EN   0x4
> > > +#define BLK_MIPI_RESET_DIV   0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > >
> > >  struct imx8m_blk_ctrl_domain;
> > >
> > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > >       const char *gpc_name;
> > >       u32 rst_mask;
> > >       u32 clk_mask;
> > > +
> > > +     /*
> > > +      * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > +      * which is used to control the reset for the MIPI Phy.
> > > +      * Since it's only present in certain circumstances,
> > > +      * an if-statement should be used before setting and clearing this
> > > +      * register.
> > > +      */
> > > +     u32 mipi_phy_rst_mask;
> > >  };
> > >
> > >  #define DOMAIN_MAX_CLKS 3
> > > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > >
> > >       /* put devices into reset */
> > >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > +     if (data->mipi_phy_rst_mask)
> > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > >
> > >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > >
> > >       /* release reset */
> > >       regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > +     if (data->mipi_phy_rst_mask)
> > > +             regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > >
> > >       /* disable upstream clocks */
> > >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > >       struct imx8m_blk_ctrl *bc = domain->bc;
> > >
> > >       /* put devices into reset and disable clocks */
> > > +     if (data->mipi_phy_rst_mask)
> > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > +
> >
> > Is it the best option to enable/disable both the master and slave MIPI
> > DPHY, regardless of whether they're used or not ? Or would it be better
> > to implement a reset controller to expose the two resets independently,
> > and acquire them from the corresponding display and camera drivers ?
>
> In some early attempts to implement the blk-ctrl driver, there was an
> attempt to enable a reset controller, but it caused some hanging and
> issues with suspend-resume due to chicken-egg issues where some items
> were coming up in the wrong order.  I think the decision was made to
> make the resets part of the power domain so it's very clear that the
> order of operations.  Lucas might be able to elaborate more on this.

I think supporting via phy driver make sense to me since this resent
is DPHY specific and nothing related to blk-ctrl.

>
> If bits 16 and 17 can act independently and bit 16 only impacts the
> CSI  and doesn't require bit 17, it seems reasonable to me to have the
> power-domain part of  the CSI, since this would only be enabled when
> the CSI is active.  The power domain is idled when the CSI is idled
> which would effectively place the phy in and out of reset only
> depending on the state of the CSI.  I am guessing this reset bit
> should be assigned to DISPBLK_PD_MIPI_CSI and not
> DISPBLK_PD_CSI_BRIDGE, but I can run some more tests.
>
> AFAIK, there is no phy driver for the CSI like there is the DSI, so
> adding that would require additional work to the CSI driver to work
> around this quirk.  We don't have an acceptable DSI driver yet, so I'd
> like to push a V3 with just the corresponding bit enabled for MIPI_CSI
> after some testing.  FWICT, NXP set both bits 16 and 17 in their ATF
> gpc code, and it never gets cleared, so I think having the bit set and
> cleared on demand is an improvement.

How about using the previous one that Marek sent. Add it via CSI
pipeline and i think it would directly.

https://www.spinics.net/lists/devicetree/msg381691.html

Jagan.

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-25  5:42     ` Jagan Teki
@ 2021-11-25 15:18       ` Adam Ford
  2021-11-26  0:33         ` Laurent Pinchart
  0 siblings, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-25 15:18 UTC (permalink / raw)
  To: Jagan Teki
  Cc: Laurent Pinchart, arm-soc, Tim Harvey, Schrempf Frieder,
	linux-media, Adam Ford-BE, cstevens, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Catalin Marinas, Will Deacon, Peng Fan,
	Lucas Stach, devicetree, Linux Kernel Mailing List

On Wed, Nov 24, 2021 at 11:42 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Tue, Nov 23, 2021 at 7:29 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Sun, Nov 21, 2021 at 4:25 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Adam,
> > >
> > > On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > > > Most of the blk-ctrl reset bits are found in one register, however
> > > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > > out of reset or the MIPI_CSI hangs.
> > > >
> > > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > ---
> > > >
> > > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > > >      Rename the new register to mipi_phy_rst_mask
> > > >      Encapsulate the edits to this register with an if-statement
> > > >
> > > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > index 519b3651d1d9..581eb4bc7f7d 100644
> > > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > @@ -17,6 +17,7 @@
> > > >
> > > >  #define BLK_SFT_RSTN 0x0
> > > >  #define BLK_CLK_EN   0x4
> > > > +#define BLK_MIPI_RESET_DIV   0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > > >
> > > >  struct imx8m_blk_ctrl_domain;
> > > >
> > > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > > >       const char *gpc_name;
> > > >       u32 rst_mask;
> > > >       u32 clk_mask;
> > > > +
> > > > +     /*
> > > > +      * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > > +      * which is used to control the reset for the MIPI Phy.
> > > > +      * Since it's only present in certain circumstances,
> > > > +      * an if-statement should be used before setting and clearing this
> > > > +      * register.
> > > > +      */
> > > > +     u32 mipi_phy_rst_mask;
> > > >  };
> > > >
> > > >  #define DOMAIN_MAX_CLKS 3
> > > > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > >
> > > >       /* put devices into reset */
> > > >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > +     if (data->mipi_phy_rst_mask)
> > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > >
> > > >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > > >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > >
> > > >       /* release reset */
> > > >       regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > +     if (data->mipi_phy_rst_mask)
> > > > +             regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > >
> > > >       /* disable upstream clocks */
> > > >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > > >       struct imx8m_blk_ctrl *bc = domain->bc;
> > > >
> > > >       /* put devices into reset and disable clocks */
> > > > +     if (data->mipi_phy_rst_mask)
> > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > +
> > >
> > > Is it the best option to enable/disable both the master and slave MIPI
> > > DPHY, regardless of whether they're used or not ? Or would it be better
> > > to implement a reset controller to expose the two resets independently,
> > > and acquire them from the corresponding display and camera drivers ?
> >
> > In some early attempts to implement the blk-ctrl driver, there was an
> > attempt to enable a reset controller, but it caused some hanging and
> > issues with suspend-resume due to chicken-egg issues where some items
> > were coming up in the wrong order.  I think the decision was made to
> > make the resets part of the power domain so it's very clear that the
> > order of operations.  Lucas might be able to elaborate more on this.
>
> I think supporting via phy driver make sense to me since this resent
> is DPHY specific and nothing related to blk-ctrl.

I would disagree that isn't not blk-ctrl.  The blk-ctrl controls the
reset lines for the CSI and enables clocks.  The additional register
does the same thing to the MIPI CSI and DSI.  The imx7-mipi-csis
driver configures the dphy already, but this reset bit is not part of
its IP block.  It seems weird to me that a phy driver would reference
a phy driver.

>
> >
> > If bits 16 and 17 can act independently and bit 16 only impacts the
> > CSI  and doesn't require bit 17, it seems reasonable to me to have the
> > power-domain part of  the CSI, since this would only be enabled when
> > the CSI is active.  The power domain is idled when the CSI is idled
> > which would effectively place the phy in and out of reset only
> > depending on the state of the CSI.  I am guessing this reset bit
> > should be assigned to DISPBLK_PD_MIPI_CSI and not
> > DISPBLK_PD_CSI_BRIDGE, but I can run some more tests.
> >
> > AFAIK, there is no phy driver for the CSI like there is the DSI, so
> > adding that would require additional work to the CSI driver to work
> > around this quirk.  We don't have an acceptable DSI driver yet, so I'd
> > like to push a V3 with just the corresponding bit enabled for MIPI_CSI
> > after some testing.  FWICT, NXP set both bits 16 and 17 in their ATF
> > gpc code, and it never gets cleared, so I think having the bit set and
> > cleared on demand is an improvement.
>
> How about using the previous one that Marek sent. Add it via CSI
> pipeline and i think it would directly.

That driver specifically addresses the DSI phy and bringing it out of
reset is just one small part of what that driver does.  I don't think
adding CSI functionality to it would be appropriate for that driver as
they are separate IP blocks.

If people don't want the blk-ctl to control this bit, I would advocate
we should do a separate reset controller to be referenced byt the
mipi-csis driver, but that was proposed before and declined.  Since
blt-ctrl already is pulling seemingly unrelated IP blocks by
controlling their clocks and resets.  The fact that NXP included it in
their ATF power-domain controller tells me they considered it related
to power domains and/or resets and not an explicit phy driver.

adam

>
> https://www.spinics.net/lists/devicetree/msg381691.html
>
> Jagan.

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-25 15:18       ` Adam Ford
@ 2021-11-26  0:33         ` Laurent Pinchart
  2021-11-27 13:50           ` Adam Ford
  0 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-26  0:33 UTC (permalink / raw)
  To: Adam Ford
  Cc: Jagan Teki, arm-soc, Tim Harvey, Schrempf Frieder, linux-media,
	Adam Ford-BE, cstevens, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	Linux Kernel Mailing List

Hi Adam,

On Thu, Nov 25, 2021 at 09:18:24AM -0600, Adam Ford wrote:
> On Wed, Nov 24, 2021 at 11:42 PM Jagan Teki wrote:
> > On Tue, Nov 23, 2021 at 7:29 PM Adam Ford wrote:
> > > On Sun, Nov 21, 2021 at 4:25 PM Laurent Pinchart wrote:
> > > > On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > > > > Most of the blk-ctrl reset bits are found in one register, however
> > > > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > > > out of reset or the MIPI_CSI hangs.
> > > > >
> > > > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > ---
> > > > >
> > > > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > > > >      Rename the new register to mipi_phy_rst_mask
> > > > >      Encapsulate the edits to this register with an if-statement
> > > > >
> > > > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > > > >  1 file changed, 18 insertions(+)
> > > > >
> > > > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > index 519b3651d1d9..581eb4bc7f7d 100644
> > > > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > @@ -17,6 +17,7 @@
> > > > >
> > > > >  #define BLK_SFT_RSTN 0x0
> > > > >  #define BLK_CLK_EN   0x4
> > > > > +#define BLK_MIPI_RESET_DIV   0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > > > >
> > > > >  struct imx8m_blk_ctrl_domain;
> > > > >
> > > > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > > > >       const char *gpc_name;
> > > > >       u32 rst_mask;
> > > > >       u32 clk_mask;
> > > > > +
> > > > > +     /*
> > > > > +      * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > > > +      * which is used to control the reset for the MIPI Phy.
> > > > > +      * Since it's only present in certain circumstances,
> > > > > +      * an if-statement should be used before setting and clearing this
> > > > > +      * register.
> > > > > +      */
> > > > > +     u32 mipi_phy_rst_mask;
> > > > >  };
> > > > >
> > > > >  #define DOMAIN_MAX_CLKS 3
> > > > > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > > >
> > > > >       /* put devices into reset */
> > > > >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > > +     if (data->mipi_phy_rst_mask)
> > > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > >
> > > > >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > > > >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > > > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > > >
> > > > >       /* release reset */
> > > > >       regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > > +     if (data->mipi_phy_rst_mask)
> > > > > +             regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > >
> > > > >       /* disable upstream clocks */
> > > > >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > > > >       struct imx8m_blk_ctrl *bc = domain->bc;
> > > > >
> > > > >       /* put devices into reset and disable clocks */
> > > > > +     if (data->mipi_phy_rst_mask)
> > > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > > +
> > > >
> > > > Is it the best option to enable/disable both the master and slave MIPI
> > > > DPHY, regardless of whether they're used or not ? Or would it be better
> > > > to implement a reset controller to expose the two resets independently,
> > > > and acquire them from the corresponding display and camera drivers ?
> > >
> > > In some early attempts to implement the blk-ctrl driver, there was an
> > > attempt to enable a reset controller, but it caused some hanging and
> > > issues with suspend-resume due to chicken-egg issues where some items
> > > were coming up in the wrong order.  I think the decision was made to
> > > make the resets part of the power domain so it's very clear that the
> > > order of operations.  Lucas might be able to elaborate more on this.
> >
> > I think supporting via phy driver make sense to me since this resent
> > is DPHY specific and nothing related to blk-ctrl.
> 
> I would disagree that isn't not blk-ctrl.  The blk-ctrl controls the
> reset lines for the CSI and enables clocks.  The additional register
> does the same thing to the MIPI CSI and DSI.  The imx7-mipi-csis
> driver configures the dphy already, but this reset bit is not part of
> its IP block.  It seems weird to me that a phy driver would reference
> a phy driver.
> 
> > > If bits 16 and 17 can act independently and bit 16 only impacts the
> > > CSI  and doesn't require bit 17, it seems reasonable to me to have the
> > > power-domain part of  the CSI, since this would only be enabled when
> > > the CSI is active.  The power domain is idled when the CSI is idled
> > > which would effectively place the phy in and out of reset only
> > > depending on the state of the CSI.  I am guessing this reset bit
> > > should be assigned to DISPBLK_PD_MIPI_CSI and not
> > > DISPBLK_PD_CSI_BRIDGE, but I can run some more tests.
> > >
> > > AFAIK, there is no phy driver for the CSI like there is the DSI, so
> > > adding that would require additional work to the CSI driver to work
> > > around this quirk.  We don't have an acceptable DSI driver yet, so I'd
> > > like to push a V3 with just the corresponding bit enabled for MIPI_CSI
> > > after some testing.  FWICT, NXP set both bits 16 and 17 in their ATF
> > > gpc code, and it never gets cleared, so I think having the bit set and
> > > cleared on demand is an improvement.
> >
> > How about using the previous one that Marek sent. Add it via CSI
> > pipeline and i think it would directly.
> 
> That driver specifically addresses the DSI phy and bringing it out of
> reset is just one small part of what that driver does.  I don't think
> adding CSI functionality to it would be appropriate for that driver as
> they are separate IP blocks.
> 
> If people don't want the blk-ctl to control this bit, I would advocate
> we should do a separate reset controller to be referenced byt the
> mipi-csis driver, but that was proposed before and declined.  Since
> blt-ctrl already is pulling seemingly unrelated IP blocks by
> controlling their clocks and resets.  The fact that NXP included it in
> their ATF power-domain controller tells me they considered it related
> to power domains and/or resets and not an explicit phy driver.

I think it's a bit more complicated than that, unfortunately. The
BLK_CTRL is a mix of miscellaneous configuration bits thrown together.
It contains enable/disable bits for clocks and resets, but also D-PHY
configuration parameters (GPR_MIPI_[MS]_DPDN_SWAP_{CLK,DAT} in
GPR_MIPI_RESET_DIV, and all the fields of the GPR_MIPI_M_PLL* and
GPR_MIPI_[BMS]_DPHYCTL* registers). The latter should be controlled by
PHY drivers, but we may be able to control get away with hardcoded
values (possibly even hardware reset default values).

For the resets and clocks, reset and clock controllers could have been
nice. I'm not sure if controlling them through a power domain could
count as a bit of an abuse, as the power domain doesn't control power
rails, but looking at the imx8m-blk-ctrl driver the on/off sequences
required by the clocks and resets would be difficult to handle if clocks
and resets were exposed separately. I'd say that in the worst case it's
probably an acceptable hack.

For the D-PHY resets, exposing them through a reset controller would
also be (in my opinion) the most pedantic approach, bus as we have power
domains for the CSI and DSI controllers, controlling the corresponding
D-PHY resets from there is in no case worse that what we have already.

The only part that bothers me is the control of the master D-PHY, used
for MIPI DSI, from the MIPI CSI power domain. I've received feedback
from NXP today that those two GPR reset signals are connected directly
to the corresponding D-PHY, without any special combinatorial logic
in-between. I think it would be worth a try to control bit 16 from the
MIPI CSI power domain and bit 17 from the MIPI DSI power domain,
especially given that bit 17 didn't make any difference in my camera
tests on the i.MX8MM (I couldn't test display as my board doesn't use
the DSI output). If we then run into any issue, we can try to figure it
out.

> > https://www.spinics.net/lists/devicetree/msg381691.html

-- 
Regards,

Laurent Pinchart

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-26  0:33         ` Laurent Pinchart
@ 2021-11-27 13:50           ` Adam Ford
  2021-11-27 15:02             ` Laurent Pinchart
  0 siblings, 1 reply; 29+ messages in thread
From: Adam Ford @ 2021-11-27 13:50 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jagan Teki, arm-soc, Tim Harvey, Schrempf Frieder, linux-media,
	Adam Ford-BE, cstevens, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	Linux Kernel Mailing List

On Thu, Nov 25, 2021 at 6:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Adam,
>
> On Thu, Nov 25, 2021 at 09:18:24AM -0600, Adam Ford wrote:
> > On Wed, Nov 24, 2021 at 11:42 PM Jagan Teki wrote:
> > > On Tue, Nov 23, 2021 at 7:29 PM Adam Ford wrote:
> > > > On Sun, Nov 21, 2021 at 4:25 PM Laurent Pinchart wrote:
> > > > > On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > > > > > Most of the blk-ctrl reset bits are found in one register, however
> > > > > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > > > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > > > > out of reset or the MIPI_CSI hangs.
> > > > > >
> > > > > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > > > > >      Rename the new register to mipi_phy_rst_mask
> > > > > >      Encapsulate the edits to this register with an if-statement
> > > > > >
> > > > > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > > > > >  1 file changed, 18 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > > index 519b3651d1d9..581eb4bc7f7d 100644
> > > > > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > > @@ -17,6 +17,7 @@
> > > > > >
> > > > > >  #define BLK_SFT_RSTN 0x0
> > > > > >  #define BLK_CLK_EN   0x4
> > > > > > +#define BLK_MIPI_RESET_DIV   0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > > > > >
> > > > > >  struct imx8m_blk_ctrl_domain;
> > > > > >
> > > > > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > > > > >       const char *gpc_name;
> > > > > >       u32 rst_mask;
> > > > > >       u32 clk_mask;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > > > > +      * which is used to control the reset for the MIPI Phy.
> > > > > > +      * Since it's only present in certain circumstances,
> > > > > > +      * an if-statement should be used before setting and clearing this
> > > > > > +      * register.
> > > > > > +      */
> > > > > > +     u32 mipi_phy_rst_mask;
> > > > > >  };
> > > > > >
> > > > > >  #define DOMAIN_MAX_CLKS 3
> > > > > > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > > > >
> > > > > >       /* put devices into reset */
> > > > > >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > > > +     if (data->mipi_phy_rst_mask)
> > > > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > > >
> > > > > >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > > > > >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > > > > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > > > >
> > > > > >       /* release reset */
> > > > > >       regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > > > +     if (data->mipi_phy_rst_mask)
> > > > > > +             regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > > >
> > > > > >       /* disable upstream clocks */
> > > > > >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > > > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > > > > >       struct imx8m_blk_ctrl *bc = domain->bc;
> > > > > >
> > > > > >       /* put devices into reset and disable clocks */
> > > > > > +     if (data->mipi_phy_rst_mask)
> > > > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > > > +
> > > > >
> > > > > Is it the best option to enable/disable both the master and slave MIPI
> > > > > DPHY, regardless of whether they're used or not ? Or would it be better
> > > > > to implement a reset controller to expose the two resets independently,
> > > > > and acquire them from the corresponding display and camera drivers ?
> > > >
> > > > In some early attempts to implement the blk-ctrl driver, there was an
> > > > attempt to enable a reset controller, but it caused some hanging and
> > > > issues with suspend-resume due to chicken-egg issues where some items
> > > > were coming up in the wrong order.  I think the decision was made to
> > > > make the resets part of the power domain so it's very clear that the
> > > > order of operations.  Lucas might be able to elaborate more on this.
> > >
> > > I think supporting via phy driver make sense to me since this resent
> > > is DPHY specific and nothing related to blk-ctrl.
> >
> > I would disagree that isn't not blk-ctrl.  The blk-ctrl controls the
> > reset lines for the CSI and enables clocks.  The additional register
> > does the same thing to the MIPI CSI and DSI.  The imx7-mipi-csis
> > driver configures the dphy already, but this reset bit is not part of
> > its IP block.  It seems weird to me that a phy driver would reference
> > a phy driver.
> >
> > > > If bits 16 and 17 can act independently and bit 16 only impacts the
> > > > CSI  and doesn't require bit 17, it seems reasonable to me to have the
> > > > power-domain part of  the CSI, since this would only be enabled when
> > > > the CSI is active.  The power domain is idled when the CSI is idled
> > > > which would effectively place the phy in and out of reset only
> > > > depending on the state of the CSI.  I am guessing this reset bit
> > > > should be assigned to DISPBLK_PD_MIPI_CSI and not
> > > > DISPBLK_PD_CSI_BRIDGE, but I can run some more tests.
> > > >
> > > > AFAIK, there is no phy driver for the CSI like there is the DSI, so
> > > > adding that would require additional work to the CSI driver to work
> > > > around this quirk.  We don't have an acceptable DSI driver yet, so I'd
> > > > like to push a V3 with just the corresponding bit enabled for MIPI_CSI
> > > > after some testing.  FWICT, NXP set both bits 16 and 17 in their ATF
> > > > gpc code, and it never gets cleared, so I think having the bit set and
> > > > cleared on demand is an improvement.
> > >
> > > How about using the previous one that Marek sent. Add it via CSI
> > > pipeline and i think it would directly.
> >
> > That driver specifically addresses the DSI phy and bringing it out of
> > reset is just one small part of what that driver does.  I don't think
> > adding CSI functionality to it would be appropriate for that driver as
> > they are separate IP blocks.
> >
> > If people don't want the blk-ctl to control this bit, I would advocate
> > we should do a separate reset controller to be referenced byt the
> > mipi-csis driver, but that was proposed before and declined.  Since
> > blt-ctrl already is pulling seemingly unrelated IP blocks by
> > controlling their clocks and resets.  The fact that NXP included it in
> > their ATF power-domain controller tells me they considered it related
> > to power domains and/or resets and not an explicit phy driver.
>
> I think it's a bit more complicated than that, unfortunately. The
> BLK_CTRL is a mix of miscellaneous configuration bits thrown together.
> It contains enable/disable bits for clocks and resets, but also D-PHY
> configuration parameters (GPR_MIPI_[MS]_DPDN_SWAP_{CLK,DAT} in
> GPR_MIPI_RESET_DIV, and all the fields of the GPR_MIPI_M_PLL* and
> GPR_MIPI_[BMS]_DPHYCTL* registers). The latter should be controlled by
> PHY drivers, but we may be able to control get away with hardcoded
> values (possibly even hardware reset default values).

From my testing, the default values  in this register block appeared
sufficient to run
the OV5640 camera I have.
>
> For the resets and clocks, reset and clock controllers could have been
> nice. I'm not sure if controlling them through a power domain could

That was attempted by Lucas and others, but there were a bunch of
issues with hanging due to order of operations and the interactions
between the bus clock from the blk-ctrl and the GPC power domains.

> count as a bit of an abuse, as the power domain doesn't control power
> rails, but looking at the imx8m-blk-ctrl driver the on/off sequences
> required by the clocks and resets would be difficult to handle if clocks
> and resets were exposed separately. I'd say that in the worst case it's
> probably an acceptable hack.

So if I post a revision with only bit-16 and leaving bit 17 for the
DSI Phy driver, do you have any objections? (see my comment below)
>
> For the D-PHY resets, exposing them through a reset controller would
> also be (in my opinion) the most pedantic approach, bus as we have power
> domains for the CSI and DSI controllers, controlling the corresponding
> D-PHY resets from there is in no case worse that what we have already.
>
> The only part that bothers me is the control of the master D-PHY, used
> for MIPI DSI, from the MIPI CSI power domain. I've received feedback
> from NXP today that those two GPR reset signals are connected directly
> to the corresponding D-PHY, without any special combinatorial logic
> in-between. I think it would be worth a try to control bit 16 from the
> MIPI CSI power domain and bit 17 from the MIPI DSI power domain,
> especially given that bit 17 didn't make any difference in my camera
> tests on the i.MX8MM (I couldn't test display as my board doesn't use
> the DSI output). If we then run into any issue, we can try to figure it
> out.

I went back to test this as well.  With only bit 16 being used, it
appeared to work too, so it seems like it's likely safe to leave bit
17 alone for this.

adam

>
> > > https://www.spinics.net/lists/devicetree/msg381691.html
>
> --
> Regards,
>
> Laurent Pinchart

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

* Re: [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset
  2021-11-27 13:50           ` Adam Ford
@ 2021-11-27 15:02             ` Laurent Pinchart
  0 siblings, 0 replies; 29+ messages in thread
From: Laurent Pinchart @ 2021-11-27 15:02 UTC (permalink / raw)
  To: Adam Ford
  Cc: Jagan Teki, arm-soc, Tim Harvey, Schrempf Frieder, linux-media,
	Adam Ford-BE, cstevens, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Peng Fan, Lucas Stach, devicetree,
	Linux Kernel Mailing List

Hi Adam,

On Sat, Nov 27, 2021 at 07:50:48AM -0600, Adam Ford wrote:
> On Thu, Nov 25, 2021 at 6:34 PM Laurent Pinchart wrote:
> > On Thu, Nov 25, 2021 at 09:18:24AM -0600, Adam Ford wrote:
> > > On Wed, Nov 24, 2021 at 11:42 PM Jagan Teki wrote:
> > > > On Tue, Nov 23, 2021 at 7:29 PM Adam Ford wrote:
> > > > > On Sun, Nov 21, 2021 at 4:25 PM Laurent Pinchart wrote:
> > > > > > On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > > > > > > Most of the blk-ctrl reset bits are found in one register, however
> > > > > > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > > > > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > > > > > out of reset or the MIPI_CSI hangs.
> > > > > > >
> > > > > > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > > > ---
> > > > > > >
> > > > > > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > > > > > >      Rename the new register to mipi_phy_rst_mask
> > > > > > >      Encapsulate the edits to this register with an if-statement
> > > > > > >
> > > > > > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > > > > > >  1 file changed, 18 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > > > index 519b3651d1d9..581eb4bc7f7d 100644
> > > > > > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > > > > > @@ -17,6 +17,7 @@
> > > > > > >
> > > > > > >  #define BLK_SFT_RSTN 0x0
> > > > > > >  #define BLK_CLK_EN   0x4
> > > > > > > +#define BLK_MIPI_RESET_DIV   0x8 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > > > > > >
> > > > > > >  struct imx8m_blk_ctrl_domain;
> > > > > > >
> > > > > > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > > > > > >       const char *gpc_name;
> > > > > > >       u32 rst_mask;
> > > > > > >       u32 clk_mask;
> > > > > > > +
> > > > > > > +     /*
> > > > > > > +      * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > > > > > +      * which is used to control the reset for the MIPI Phy.
> > > > > > > +      * Since it's only present in certain circumstances,
> > > > > > > +      * an if-statement should be used before setting and clearing this
> > > > > > > +      * register.
> > > > > > > +      */
> > > > > > > +     u32 mipi_phy_rst_mask;
> > > > > > >  };
> > > > > > >
> > > > > > >  #define DOMAIN_MAX_CLKS 3
> > > > > > > @@ -78,6 +88,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > > > > >
> > > > > > >       /* put devices into reset */
> > > > > > >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > > > > +     if (data->mipi_phy_rst_mask)
> > > > > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > > > >
> > > > > > >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > > > > > >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > > > > > @@ -99,6 +111,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > > > > >
> > > > > > >       /* release reset */
> > > > > > >       regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > > > > > +     if (data->mipi_phy_rst_mask)
> > > > > > > +             regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > > > >
> > > > > > >       /* disable upstream clocks */
> > > > > > >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > > > > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > > > > > >       struct imx8m_blk_ctrl *bc = domain->bc;
> > > > > > >
> > > > > > >       /* put devices into reset and disable clocks */
> > > > > > > +     if (data->mipi_phy_rst_mask)
> > > > > > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > > > > > +
> > > > > >
> > > > > > Is it the best option to enable/disable both the master and slave MIPI
> > > > > > DPHY, regardless of whether they're used or not ? Or would it be better
> > > > > > to implement a reset controller to expose the two resets independently,
> > > > > > and acquire them from the corresponding display and camera drivers ?
> > > > >
> > > > > In some early attempts to implement the blk-ctrl driver, there was an
> > > > > attempt to enable a reset controller, but it caused some hanging and
> > > > > issues with suspend-resume due to chicken-egg issues where some items
> > > > > were coming up in the wrong order.  I think the decision was made to
> > > > > make the resets part of the power domain so it's very clear that the
> > > > > order of operations.  Lucas might be able to elaborate more on this.
> > > >
> > > > I think supporting via phy driver make sense to me since this resent
> > > > is DPHY specific and nothing related to blk-ctrl.
> > >
> > > I would disagree that isn't not blk-ctrl.  The blk-ctrl controls the
> > > reset lines for the CSI and enables clocks.  The additional register
> > > does the same thing to the MIPI CSI and DSI.  The imx7-mipi-csis
> > > driver configures the dphy already, but this reset bit is not part of
> > > its IP block.  It seems weird to me that a phy driver would reference
> > > a phy driver.
> > >
> > > > > If bits 16 and 17 can act independently and bit 16 only impacts the
> > > > > CSI  and doesn't require bit 17, it seems reasonable to me to have the
> > > > > power-domain part of  the CSI, since this would only be enabled when
> > > > > the CSI is active.  The power domain is idled when the CSI is idled
> > > > > which would effectively place the phy in and out of reset only
> > > > > depending on the state of the CSI.  I am guessing this reset bit
> > > > > should be assigned to DISPBLK_PD_MIPI_CSI and not
> > > > > DISPBLK_PD_CSI_BRIDGE, but I can run some more tests.
> > > > >
> > > > > AFAIK, there is no phy driver for the CSI like there is the DSI, so
> > > > > adding that would require additional work to the CSI driver to work
> > > > > around this quirk.  We don't have an acceptable DSI driver yet, so I'd
> > > > > like to push a V3 with just the corresponding bit enabled for MIPI_CSI
> > > > > after some testing.  FWICT, NXP set both bits 16 and 17 in their ATF
> > > > > gpc code, and it never gets cleared, so I think having the bit set and
> > > > > cleared on demand is an improvement.
> > > >
> > > > How about using the previous one that Marek sent. Add it via CSI
> > > > pipeline and i think it would directly.
> > >
> > > That driver specifically addresses the DSI phy and bringing it out of
> > > reset is just one small part of what that driver does.  I don't think
> > > adding CSI functionality to it would be appropriate for that driver as
> > > they are separate IP blocks.
> > >
> > > If people don't want the blk-ctl to control this bit, I would advocate
> > > we should do a separate reset controller to be referenced byt the
> > > mipi-csis driver, but that was proposed before and declined.  Since
> > > blt-ctrl already is pulling seemingly unrelated IP blocks by
> > > controlling their clocks and resets.  The fact that NXP included it in
> > > their ATF power-domain controller tells me they considered it related
> > > to power domains and/or resets and not an explicit phy driver.
> >
> > I think it's a bit more complicated than that, unfortunately. The
> > BLK_CTRL is a mix of miscellaneous configuration bits thrown together.
> > It contains enable/disable bits for clocks and resets, but also D-PHY
> > configuration parameters (GPR_MIPI_[MS]_DPDN_SWAP_{CLK,DAT} in
> > GPR_MIPI_RESET_DIV, and all the fields of the GPR_MIPI_M_PLL* and
> > GPR_MIPI_[BMS]_DPHYCTL* registers). The latter should be controlled by
> > PHY drivers, but we may be able to control get away with hardcoded
> > values (possibly even hardware reset default values).
> 
> From my testing, the default values  in this register block appeared
> sufficient to run the OV5640 camera I have.
>
> > For the resets and clocks, reset and clock controllers could have been
> > nice. I'm not sure if controlling them through a power domain could
> 
> That was attempted by Lucas and others, but there were a bunch of
> issues with hanging due to order of operations and the interactions
> between the bus clock from the blk-ctrl and the GPC power domains.
> 
> > count as a bit of an abuse, as the power domain doesn't control power
> > rails, but looking at the imx8m-blk-ctrl driver the on/off sequences
> > required by the clocks and resets would be difficult to handle if clocks
> > and resets were exposed separately. I'd say that in the worst case it's
> > probably an acceptable hack.
> 
> So if I post a revision with only bit-16 and leaving bit 17 for the
> DSI Phy driver, do you have any objections? (see my comment below)

How about this ?

@@ -480,6 +497,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
 		.gpc_name = "mipi-dsi",
 		.rst_mask = BIT(5),
 		.clk_mask = BIT(8) | BIT(9),
+		.mipi_phy_rst_mask = BIT(17),
 	},
 	[IMX8MM_DISPBLK_PD_MIPI_CSI] = {
 		.name = "dispblk-mipi-csi",
@@ -488,6 +506,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
 		.gpc_name = "mipi-csi",
 		.rst_mask = BIT(3) | BIT(4),
 		.clk_mask = BIT(10) | BIT(11),
+		.mipi_phy_rst_mask = BIT(16),
 	},
 };

> > For the D-PHY resets, exposing them through a reset controller would
> > also be (in my opinion) the most pedantic approach, bus as we have power
> > domains for the CSI and DSI controllers, controlling the corresponding
> > D-PHY resets from there is in no case worse that what we have already.
> >
> > The only part that bothers me is the control of the master D-PHY, used
> > for MIPI DSI, from the MIPI CSI power domain. I've received feedback
> > from NXP today that those two GPR reset signals are connected directly
> > to the corresponding D-PHY, without any special combinatorial logic
> > in-between. I think it would be worth a try to control bit 16 from the
> > MIPI CSI power domain and bit 17 from the MIPI DSI power domain,
> > especially given that bit 17 didn't make any difference in my camera
> > tests on the i.MX8MM (I couldn't test display as my board doesn't use
> > the DSI output). If we then run into any issue, we can try to figure it
> > out.
> 
> I went back to test this as well.  With only bit 16 being used, it
> appeared to work too, so it seems like it's likely safe to leave bit
> 17 alone for this.
> 
> > > > https://www.spinics.net/lists/devicetree/msg381691.html

-- 
Regards,

Laurent Pinchart

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

* Re: (EXT) Re: [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera
  2021-11-23  9:47           ` Laurent Pinchart
@ 2021-11-29 18:56             ` Tim Harvey
  0 siblings, 0 replies; 29+ messages in thread
From: Tim Harvey @ 2021-11-29 18:56 UTC (permalink / raw)
  To: Laurent Pinchart, Alexander Stein
  Cc: Adam Ford, arm-soc, Schrempf Frieder, linux-media, Adam Ford-BE,
	cstevens, Jagan Teki, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Catalin Marinas, Will Deacon, Lucas Stach, Peng Fan, devicetree,
	Linux Kernel Mailing List

On Tue, Nov 23, 2021 at 1:47 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Alexander,
>
> On Tue, Nov 23, 2021 at 08:38:47AM +0100, Alexander Stein wrote:
> > Am Dienstag, dem 23.11.2021 um 02:15 +0200 schrieb Laurent Pinchart:
> > > On Sun, Nov 21, 2021 at 09:07:26PM -0600, Adam Ford wrote:
> > > > On Sun, Nov 21, 2021 at 5:18 PM Laurent Pinchart wrote:
> > > > > On Sat, Nov 06, 2021 at 10:54:26AM -0500, Adam Ford wrote:
> > > > > > The baseboard has support for a TDNext 5640 Camera which
> > > > > > uses an OV5640 connected to a 2-lane CSI2 interface.
> > > > > >
> > > > > > With the CSI and mipi_csi2 drivers pointing to an OV5640 camera, the media
> > > > > > pipeline can be configured with the following:
> > > > > >
> > > > > >     media-ctl --links "'ov5640 1-003c':0->'imx7-mipi-csis.0':0[1]"
> > > > > >
> > > > > > The camera and various nodes in the pipeline can be configured for UYVY:
> > > > > >     media-ctl -v -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/640x480 field:none]"
> > > > > >     media-ctl -v -V "'csi':0 [fmt:UYVY8_1X16/640x480 field:none]"
> > > > > >
> > > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > >
> > > > > As the ov5640 is on an add-on module, would a DT overlay be better ?
> > > >
> > > > At least for the Beacon / LogicPD boards, I would prefer to avoid the
> > > > overlays.  We have an i.M6Q and an OMAP3 board with cameras enabled in
> > > > our development kit device trees.  If the cameras are not connected,
> > > > they just display a message that the cameras are not communicating and
> > > > move on.  I'm OK with that.
> > >
> > > You know the board better than I do, so I won't push against this, but I
> > > still think it may not lead to the best user experience, especially if a
> > > user wanted to connect a different sensor to the development board.
> >
> > I see the advantages of overlays compared to "stacked" .dts files. But
> > is there any general supported interface how to actually apply an overlay?
> > Documentation/devicetree/overlay-notes.rst
> > states of_overlay_fdt_apply() but there is only exactly one user in-
> > kernel (rcar-du). Is it expected that the bootloader like u-boot shall
> > apply the .dtbo files?
>
> I believe the boot loader is expected to apply overlays nowadays, yes.
> That's my personal workflow.
>

That is my understanding as well. I believe the support to apply dt
overlays within Linux (which the rpi kernel still uses) never got
merged due to race conditions so the focus was moved to bootloader.

I also have begun submitting some dt overlay files [1] [2] which I
will likely repost later this week removing the RFC.

My understanding is that these should be '.dtbo' files in the Linux
Makefile which are handled. My boards use the U-Boot bootloader and to
handle the dt overlays there you need to:
- set CONFIG_OF_LIBFDT_OVERLAY=y which gives you the 'fdt apply' command
- use 'fdt addr <addr> && fdt resize && fdt apply <loadaddr>' prior to
booting with booti
- Note that there is some support at the FIT level as well for
overlays if you need them applied to U-Boot's live dt (I don't for my
needs)

In my U-Boot environment I use scripts for loading the fdt and
applying the overlays. For example for booting kernel/dtb from network
I use:
boot_net setenv fsload tftpboot; run loadfdt && run apply_overlays &&
$fsload $kernel_addr_r venice/Image && booti $kernel_addr_r -
$fdt_addr_r
loadfdt if $fsload $fdt_addr_r $dir/$fdt_file1; then echo loaded
$fdt_file1; elif $fsload $fdt_addr_r $dir/$fdt_file2; then echo loaded
$fdt_file2; elif $fsload $fdt_addr_r $dir/$fdt_file3; then echo loaded
$fdt_file3; elif $fsload $fdt_addr_r $dir/$fdt_file4; then echo loaded
$fdt_file4; elif $fsload $fdt_addr_r $dir/$fdt_file5; then echo loaded
$fdt_file5; fi
apply_overlays fdt addr $fdt_addr_r && fdt resize && for i in
"$fdt_overlays"; do $fsload $loadaddr $dir/$i && fdt apply $loadaddr
&& echo applied $i...; done

Best regards,

Tim
[1] https://www.spinics.net/lists/arm-kernel/msg933447.html
[2] https://www.spinics.net/lists/arm-kernel/msg933638.html

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

end of thread, other threads:[~2021-11-29 22:40 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-06 15:54 [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Adam Ford
2021-11-06 15:54 ` [PATCH V2 2/5] arm64: dts: imx8mm: Add CSI nodes Adam Ford
2021-11-07 12:58   ` Fabio Estevam
2021-11-06 15:54 ` [PATCH V2 3/5] arm64: defconfig: Enable VIDEO_IMX_MEDIA Adam Ford
2021-11-07 13:01   ` Fabio Estevam
2021-11-06 15:54 ` [PATCH V2 4/5] arm64: dts: imx8mm-beacon: Enable OV5640 Camera Adam Ford
2021-11-07 13:01   ` Fabio Estevam
2021-11-21 23:18   ` Laurent Pinchart
2021-11-22  3:07     ` Adam Ford
2021-11-23  0:15       ` Laurent Pinchart
2021-11-23  7:38         ` (EXT) " Alexander Stein
2021-11-23  9:47           ` Laurent Pinchart
2021-11-29 18:56             ` Tim Harvey
2021-11-06 15:54 ` [PATCH V2 5/5] arm64: defconfig: Enable OV5640 Adam Ford
2021-11-07 13:03   ` Fabio Estevam
2021-11-07 12:58 ` [PATCH V2 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset Fabio Estevam
2021-11-08  8:56 ` Lucas Stach
2021-11-12  6:54 ` Jagan Teki
2021-11-19 23:51   ` Tim Harvey
2021-11-20 18:33     ` Adam Ford
2021-11-21 23:13       ` Laurent Pinchart
2021-11-21 22:25 ` Laurent Pinchart
2021-11-21 22:30   ` Laurent Pinchart
2021-11-23 13:59   ` Adam Ford
2021-11-25  5:42     ` Jagan Teki
2021-11-25 15:18       ` Adam Ford
2021-11-26  0:33         ` Laurent Pinchart
2021-11-27 13:50           ` Adam Ford
2021-11-27 15:02             ` 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).