diff mbox series

[RFC,v2,2/3] arm64: dts: imx8mm: Add MIPI DSI pipeline

Message ID 20211111101456.584061-3-jagan@amarulasolutions.com
State New
Headers
Series arm64: imx8mm: Add MIPI DSI support |

Commit Message

Jagan Teki Nov. 11, 2021, 10:14 a.m. UTC 
Add MIPI DSI pipeline for i.MX8MM.

Video pipeline start from eLCDIF to MIPI DSI and respective
Panel or Bridge on the backend side.

Add support for it.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 55 +++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Marek Vasut Nov. 11, 2021, 10:21 a.m. UTC | #1 
On 11/11/21 11:14 AM, Jagan Teki wrote:

[...]

> +			dsi: dsi@32e10000 {
> +				compatible = "fsl,imx8mm-mipi-dsim";
> +				reg = <0x32e10000 0x400>;
> +				clocks = <&clk IMX8MM_CLK_DSI_CORE>,
> +					 <&clk IMX8MM_CLK_DSI_PHY_REF>;
> +				clock-names = "bus_clk", "sclk_mipi";
> +				assigned-clocks = <&clk IMX8MM_CLK_DSI_CORE>,
> +						  <&clk IMX8MM_VIDEO_PLL1_OUT>,
> +						  <&clk IMX8MM_CLK_DSI_PHY_REF>;
> +				assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_266M>,
> +							 <&clk IMX8MM_VIDEO_PLL1_BYPASS>,
> +							 <&clk IMX8MM_VIDEO_PLL1_OUT>;
> +				assigned-clock-rates = <266000000>, <594000000>, <27000000>;
> +				interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +				phys = <&mipi_phy 0>;
> +				phy-names = "dsim";
> +				power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_MIPI_DSI>;
> +				samsung,burst-clock-frequency = <891000000>;
> +				samsung,esc-clock-frequency = <54000000>;
> +				samsung,pll-clock-frequency = <27000000>;
> +				status = "disabled";


This 27 MHz is really IMX8MM_CLK_DSI_PHY_REF and 
samsung,burst-clock-frequency is really the DSI link clock which is 
panel/bridge specific ... but, why do we need to specify such policy in 
DT rather than have the panel/bridge drivers negotiate the best clock 
settings with DSIM bridge driver ? This should be something which should 
be implemented in the DRM subsystem, not hard-coded in DT. These ad-hoc 
samsung,*-clock-frequency properties shouldn't even be needed then.

Also, are the DSIM bindings stable now ?
Jagan Teki Nov. 11, 2021, 4:31 p.m. UTC | #2 
On Thu, Nov 11, 2021 at 3:51 PM Marek Vasut <marex@denx.de> wrote:
>
> On 11/11/21 11:14 AM, Jagan Teki wrote:
>
> [...]
>
> > +                     dsi: dsi@32e10000 {
> > +                             compatible = "fsl,imx8mm-mipi-dsim";
> > +                             reg = <0x32e10000 0x400>;
> > +                             clocks = <&clk IMX8MM_CLK_DSI_CORE>,
> > +                                      <&clk IMX8MM_CLK_DSI_PHY_REF>;
> > +                             clock-names = "bus_clk", "sclk_mipi";
> > +                             assigned-clocks = <&clk IMX8MM_CLK_DSI_CORE>,
> > +                                               <&clk IMX8MM_VIDEO_PLL1_OUT>,
> > +                                               <&clk IMX8MM_CLK_DSI_PHY_REF>;
> > +                             assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_266M>,
> > +                                                      <&clk IMX8MM_VIDEO_PLL1_BYPASS>,
> > +                                                      <&clk IMX8MM_VIDEO_PLL1_OUT>;
> > +                             assigned-clock-rates = <266000000>, <594000000>, <27000000>;
> > +                             interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> > +                             phys = <&mipi_phy 0>;
> > +                             phy-names = "dsim";
> > +                             power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_MIPI_DSI>;
> > +                             samsung,burst-clock-frequency = <891000000>;
> > +                             samsung,esc-clock-frequency = <54000000>;
> > +                             samsung,pll-clock-frequency = <27000000>;
> > +                             status = "disabled";
>
>
> This 27 MHz is really IMX8MM_CLK_DSI_PHY_REF and
> samsung,burst-clock-frequency is really the DSI link clock which is
> panel/bridge specific ... but, why do we need to specify such policy in
> DT rather than have the panel/bridge drivers negotiate the best clock
> settings with DSIM bridge driver ? This should be something which should
> be implemented in the DRM subsystem, not hard-coded in DT. These ad-hoc
> samsung,*-clock-frequency properties shouldn't even be needed then.

This look confusion for me, all three clock are used it directly from
exynos. and these indeed are computing pll for this bridge and clock
control of dsim registers are updated from this out come values. No
thoughts as of now how to handle these externally and update the
internal register based on those out come values.

>
> Also, are the DSIM bindings stable now ?

Issue still lies on exynos dsi side, the final driver is not binding
properly. I'm trying to send the next version patches only for
existing exynos dsi to convert into bridge. and subsequently adding
i.mx8mm specifics. More problem for me to test it on exynos boards, i
don't have any either.

Jagan.
Tim Harvey Nov. 12, 2021, 12:36 a.m. UTC | #3 
On Thu, Nov 11, 2021 at 2:15 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Add MIPI DSI pipeline for i.MX8MM.
>
> Video pipeline start from eLCDIF to MIPI DSI and respective
> Panel or Bridge on the backend side.
>
> Add support for it.

Jagan,

Thanks for your continued work on IMX8MM DSI support!

It doesn't look like you sent this to the Device Tree mainling list so
I added that to cc.

>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 55 +++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index caeb93313413..eddf3a467fd2 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -188,6 +188,12 @@ clk_ext4: clock-ext4 {
>                 clock-output-names = "clk_ext4";
>         };
>
> +       mipi_phy: mipi-video-phy {
> +               compatible = "fsl,imx8mm-mipi-video-phy";
> +               syscon = <&disp_blk_ctrl>;
> +               #phy-cells = <1>;
> +       };
> +
>         psci {
>                 compatible = "arm,psci-1.0";
>                 method = "smc";
> @@ -1085,6 +1091,55 @@ lcdif: lcdif@32e00000 {
>                                 interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
>                                 power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_LCDIF>;
>                                 status = "disabled";
> +
> +                               port {
> +                                       lcdif_out_dsi: endpoint {
> +                                               remote-endpoint = <&dsi_in_lcdif>;
> +                                       };
> +                               };
> +                       };
> +
> +                       dsi: dsi@32e10000 {

I wonder if this should this be 'mipi_dsi' like the CSI bindings
Adam's submitted here:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20211106155427.753197-2-aford173@gmail.com/

> +                               compatible = "fsl,imx8mm-mipi-dsim";
> +                               reg = <0x32e10000 0x400>;
> +                               clocks = <&clk IMX8MM_CLK_DSI_CORE>,
> +                                        <&clk IMX8MM_CLK_DSI_PHY_REF>;
> +                               clock-names = "bus_clk", "sclk_mipi";
> +                               assigned-clocks = <&clk IMX8MM_CLK_DSI_CORE>,
> +                                                 <&clk IMX8MM_VIDEO_PLL1_OUT>,
> +                                                 <&clk IMX8MM_CLK_DSI_PHY_REF>;
> +                               assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_266M>,
> +                                                        <&clk IMX8MM_VIDEO_PLL1_BYPASS>,
> +                                                        <&clk IMX8MM_VIDEO_PLL1_OUT>;
> +                               assigned-clock-rates = <266000000>, <594000000>, <27000000>;
> +                               interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +                               phys = <&mipi_phy 0>;
> +                               phy-names = "dsim";
> +                               power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_MIPI_DSI>;
> +                               samsung,burst-clock-frequency = <891000000>;
> +                               samsung,esc-clock-frequency = <54000000>;
> +                               samsung,pll-clock-frequency = <27000000>;
> +                               status = "disabled";
> +
> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       port@0 {
> +                                               reg = <0>;
> +                                               #address-cells = <1>;
> +                                               #size-cells = <0>;

I don't think the '#address-cells' and '#size-cells' are needed here
but I defer to the dt experts!

> +
> +                                               dsi_in_lcdif: endpoint@0 {
> +                                                       reg = <0>;

Per Adam's comment to my posting this should be just "port {" and we
can get rid of the @0 and the "reg=0"

Best regards,

Tim

> +                                                       remote-endpoint = <&lcdif_out_dsi>;
> +                                               };
> +                                       };
> +
> +                                       port@1 {
> +                                               reg = <1>;
> +                                       };
> +                               };
>                         };
>
>                         disp_blk_ctrl: blk-ctrl@32e28000 {
> --
> 2.25.1
>
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index caeb93313413..eddf3a467fd2 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -188,6 +188,12 @@  clk_ext4: clock-ext4 {
 		clock-output-names = "clk_ext4";
 	};
 
+	mipi_phy: mipi-video-phy {
+		compatible = "fsl,imx8mm-mipi-video-phy";
+		syscon = <&disp_blk_ctrl>;
+		#phy-cells = <1>;
+	};
+
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
@@ -1085,6 +1091,55 @@  lcdif: lcdif@32e00000 {
 				interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
 				power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_LCDIF>;
 				status = "disabled";
+
+				port {
+					lcdif_out_dsi: endpoint {
+						remote-endpoint = <&dsi_in_lcdif>;
+					};
+				};
+			};
+
+			dsi: dsi@32e10000 {
+				compatible = "fsl,imx8mm-mipi-dsim";
+				reg = <0x32e10000 0x400>;
+				clocks = <&clk IMX8MM_CLK_DSI_CORE>,
+					 <&clk IMX8MM_CLK_DSI_PHY_REF>;
+				clock-names = "bus_clk", "sclk_mipi";
+				assigned-clocks = <&clk IMX8MM_CLK_DSI_CORE>,
+						  <&clk IMX8MM_VIDEO_PLL1_OUT>,
+						  <&clk IMX8MM_CLK_DSI_PHY_REF>;
+				assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_266M>,
+							 <&clk IMX8MM_VIDEO_PLL1_BYPASS>,
+							 <&clk IMX8MM_VIDEO_PLL1_OUT>;
+				assigned-clock-rates = <266000000>, <594000000>, <27000000>;
+				interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
+				phys = <&mipi_phy 0>;
+				phy-names = "dsim";
+				power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_MIPI_DSI>;
+				samsung,burst-clock-frequency = <891000000>;
+				samsung,esc-clock-frequency = <54000000>;
+				samsung,pll-clock-frequency = <27000000>;
+				status = "disabled";
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						dsi_in_lcdif: endpoint@0 {
+							reg = <0>;
+							remote-endpoint = <&lcdif_out_dsi>;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+					};
+				};
 			};
 
 			disp_blk_ctrl: blk-ctrl@32e28000 {