[RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Tim Harvey]

Add nodes for MIPI DSI and LCDIF on IMX8MM

I'm currently working with a set of patches to convert drm/exynos 
to a bridge [1] and add IMX8MM support [2] in order to get IMX8MM DSI
working for display with a Raspberry Pi DSI touchscreen compatible with
a Toshiba TC358762 DSI bridge and Powertip PH800480T013-IDF02
touchscreen.

I had this working on a 5.10 kernel with the old blk-ctl and
power-domain drivers that didn't make it into mainline but my 5.15
series with blk-ctl backported from next hangs right after
"[drm] Initialized mxsfb-drm 1.0.0 20160824 for 32e00000.lcdif on minor 0"
so I assume I have a power-domain not getting enabled.

Please let me know if you see an issue with the way I've configured
power-domain or clocks here.

Best Regards,

Tim
[1] https://patchwork.kernel.org/project/dri-devel/list/?series=347439&archive=both&state=*
[2] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=359775&archive=both&state=*
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 68 +++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 208a0ed840f4..195dcbff7058 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -188,6 +188,12 @@
 		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";
@@ -1068,6 +1074,68 @@
 			#size-cells = <1>;
 			ranges = <0x32c00000 0x32c00000 0x400000>;
 
+			lcdif: lcdif@32e00000 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
+				reg = <0x32e00000 0x10000>;
+				clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
+					 <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
+					 <&clk IMX8MM_CLK_DISP_APB_ROOT>;
+				clock-names = "pix", "disp_axi", "axi";
+				assigned-clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
+						  <&clk IMX8MM_CLK_DISP_AXI>,
+						  <&clk IMX8MM_CLK_DISP_APB>;
+				assigned-clock-parents = <&clk IMX8MM_VIDEO_PLL1_OUT>,
+							 <&clk IMX8MM_SYS_PLL2_1000M>,
+							 <&clk IMX8MM_SYS_PLL1_800M>;
+				assigned-clock-rate = <594000000>, <500000000>, <200000000>;
+				interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_LCDIF>;
+				status = "disabled";
+
+				port@0 {
+					reg = <0>;
+
+					lcdif_to_dsim: endpoint {
+						remote-endpoint = <&dsim_from_lcdif>;
+					};
+				};
+			};
+
+			mipi_dsi: mipi_dsi@32e10000 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				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";
+
+				port@0 {
+					reg = <0>;
+
+					dsim_from_lcdif: endpoint {
+						remote-endpoint = <&lcdif_to_dsim>;
+					};
+				};
+			};
+
 			csi: csi@32e20000 {
 				compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
 				reg = <0x32e20000 0x1000>;
-- 
2.17.1

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Jagan Teki]

On Tue, Nov 9, 2021 at 11:04 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> Add nodes for MIPI DSI and LCDIF on IMX8MM
>
> I'm currently working with a set of patches to convert drm/exynos
> to a bridge [1] and add IMX8MM support [2] in order to get IMX8MM DSI
> working for display with a Raspberry Pi DSI touchscreen compatible with
> a Toshiba TC358762 DSI bridge and Powertip PH800480T013-IDF02
> touchscreen.
>
> I had this working on a 5.10 kernel with the old blk-ctl and
> power-domain drivers that didn't make it into mainline but my 5.15
> series with blk-ctl backported from next hangs right after
> "[drm] Initialized mxsfb-drm 1.0.0 20160824 for 32e00000.lcdif on minor 0"
> so I assume I have a power-domain not getting enabled.
>
> Please let me know if you see an issue with the way I've configured
> power-domain or clocks here.

I'm reworking back on top of drm-misc-next, may be I will let you know
in couple of days.

Jagan.

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Adam Ford]

On Tue, Nov 9, 2021 at 11:38 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Tue, Nov 9, 2021 at 11:04 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > Add nodes for MIPI DSI and LCDIF on IMX8MM
> >
> > I'm currently working with a set of patches to convert drm/exynos
> > to a bridge [1] and add IMX8MM support [2] in order to get IMX8MM DSI
> > working for display with a Raspberry Pi DSI touchscreen compatible with
> > a Toshiba TC358762 DSI bridge and Powertip PH800480T013-IDF02
> > touchscreen.
> >
> > I had this working on a 5.10 kernel with the old blk-ctl and
> > power-domain drivers that didn't make it into mainline but my 5.15
> > series with blk-ctl backported from next hangs right after
> > "[drm] Initialized mxsfb-drm 1.0.0 20160824 for 32e00000.lcdif on minor 0"
> > so I assume I have a power-domain not getting enabled.
> >
> > Please let me know if you see an issue with the way I've configured
> > power-domain or clocks here.
>
> I'm reworking back on top of drm-misc-next, may be I will let you know
> in couple of days.

I'll try to look at it tonight or next week.  I'll be traveling
tomorrow through Sunday, but I'd like to see the DSI video working
too, so I'll help where I can.

adam
>
> Jagan.

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Adam Ford]

On Tue, Nov 9, 2021 at 11:34 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> Add nodes for MIPI DSI and LCDIF on IMX8MM
>
> I'm currently working with a set of patches to convert drm/exynos
> to a bridge [1] and add IMX8MM support [2] in order to get IMX8MM DSI
> working for display with a Raspberry Pi DSI touchscreen compatible with
> a Toshiba TC358762 DSI bridge and Powertip PH800480T013-IDF02
> touchscreen.
>
> I had this working on a 5.10 kernel with the old blk-ctl and
> power-domain drivers that didn't make it into mainline but my 5.15
> series with blk-ctl backported from next hangs right after
> "[drm] Initialized mxsfb-drm 1.0.0 20160824 for 32e00000.lcdif on minor 0"
> so I assume I have a power-domain not getting enabled.
>
> Please let me know if you see an issue with the way I've configured
> power-domain or clocks here.
>
> Best Regards,
>
> Tim
> [1] https://patchwork.kernel.org/project/dri-devel/list/?series=347439&archive=both&state=*
> [2] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=359775&archive=both&state=*
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 68 +++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 208a0ed840f4..195dcbff7058 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -188,6 +188,12 @@
>                 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";
> @@ -1068,6 +1074,68 @@
>                         #size-cells = <1>;
>                         ranges = <0x32c00000 0x32c00000 0x400000>;
>
> +                       lcdif: lcdif@32e00000 {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";

The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
MXSFB_V4, but with overlays and those overlays have more registers
configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
to see if it makes a difference?

> +                               reg = <0x32e00000 0x10000>;
> +                               clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> +                                        <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
> +                                        <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> +                               clock-names = "pix", "disp_axi", "axi";
> +                               assigned-clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> +                                                 <&clk IMX8MM_CLK_DISP_AXI>,
> +                                                 <&clk IMX8MM_CLK_DISP_APB>;
> +                               assigned-clock-parents = <&clk IMX8MM_VIDEO_PLL1_OUT>,
> +                                                        <&clk IMX8MM_SYS_PLL2_1000M>,
> +                                                        <&clk IMX8MM_SYS_PLL1_800M>;
> +                               assigned-clock-rate = <594000000>, <500000000>, <200000000>;

Just through visual inspection, it appears that the
IMX8MM_CLK_DISP_AXI and IMX8MM_CLK_DISP_APB clock-parents and rates
are already set in the pgc_dispmix, so I think it's safe to reduce
those lines to just assigning IMX8MM_CLK_LCDIF_PIXEL to the
IMX8MM_VIDEO_PLL1_OUT with the assigned-clock-rate set to 594000000.

adam



> +                               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> +                               power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_LCDIF>;
> +                               status = "disabled";
> +
> +                               port@0 {
> +                                       reg = <0>;
> +
> +                                       lcdif_to_dsim: endpoint {
> +                                               remote-endpoint = <&dsim_from_lcdif>;
> +                                       };
> +                               };
> +                       };
> +
> +                       mipi_dsi: mipi_dsi@32e10000 {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               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";
> +
> +                               port@0 {
> +                                       reg = <0>;
> +
> +                                       dsim_from_lcdif: endpoint {
> +                                               remote-endpoint = <&lcdif_to_dsim>;
> +                                       };
> +                               };
> +                       };
> +
>                         csi: csi@32e20000 {
>                                 compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
>                                 reg = <0x32e20000 0x1000>;
> --
> 2.17.1
>

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Marek Vasut]

On 11/9/21 8:35 PM, Adam Ford wrote:

[...]

>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
>> index 208a0ed840f4..195dcbff7058 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
>> @@ -188,6 +188,12 @@
>>                  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";
>> @@ -1068,6 +1074,68 @@
>>                          #size-cells = <1>;
>>                          ranges = <0x32c00000 0x32c00000 0x400000>;
>>
>> +                       lcdif: lcdif@32e00000 {
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> 
> The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> MXSFB_V4, but with overlays and those overlays have more registers
> configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> to see if it makes a difference?

Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.

LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with 
slightly reordered register bits, but nothing like LCDIF rev3 (in MX23) 
... just to make sure there is no confusion.

[...]

>> +                       mipi_dsi: mipi_dsi@32e10000 {
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                               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>;

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 ?

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Tim Harvey]

On Tue, Nov 9, 2021 at 12:39 PM Marek Vasut <marex@denx.de> wrote:
>
> On 11/9/21 8:35 PM, Adam Ford wrote:
>
> [...]
>
> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> >> index 208a0ed840f4..195dcbff7058 100644
> >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> >> @@ -188,6 +188,12 @@
> >>                  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";
> >> @@ -1068,6 +1074,68 @@
> >>                          #size-cells = <1>;
> >>                          ranges = <0x32c00000 0x32c00000 0x400000>;
> >>
> >> +                       lcdif: lcdif@32e00000 {
> >> +                               #address-cells = <1>;
> >> +                               #size-cells = <0>;
> >> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> >
> > The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> > MXSFB_V4, but with overlays and those overlays have more registers
> > configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> > to see if it makes a difference?
>
> Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.
>
> LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with
> slightly reordered register bits, but nothing like LCDIF rev3 (in MX23)
> ... just to make sure there is no confusion.
>
> [...]
>
> >> +                       mipi_dsi: mipi_dsi@32e10000 {
> >> +                               #address-cells = <1>;
> >> +                               #size-cells = <0>;
> >> +                               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>;
>
> 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 ?

Thanks Marek.

No, there is no dsim driver yet. I'm not clear if there is still
dissagreement on if the drm/exynos driver can be split up or if a
whole new somewhat duplicate driver needs to be made. I know Jagan
also has a series he is working on that uses drm/exynos which I
believe he will share an update on in a day or so.

I'm still using the work that Michael [1] and you [2] did a long time back.

I had this solution working on a 5.10 kernel but it was based on the
old unapproved IMX8MM blk-ctl and pd drivers. Therefore I believe the
issue I'm having is something not setup correctly with blk-ctl per my
dt settings or perhaps something missing from the blk-ctl driver like
Adam found [3]

I am hanging at:
[    1.064088] imx_pgc_power_up gpumix
[    1.077506] imx_pgc_power_down gpumix
[    1.097685] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
[    1.102682] imx_pgc_power_up dispmix
[    1.106825] imx_pgc_power_up mipi
[    1.110232] imx-dsim-dsi 32e10000.mipi_dsi: supply vddcore not
found, using dummy regulator
[    1.118760] imx-dsim-dsi 32e10000.mipi_dsi: supply vddio not found,
using dummy regulator
[    1.127361] imx-dsim-dsi 32e10000.mipi_dsi: [drm] *ERROR* modalias
failure on /soc@0/bus@32c00000/mipi_dsi@32e10000/port@0
[    1.138676] imx8m_blk_ctrl_power_off dispblk-mipi-dsi
[    1.143788] imx_pgc_power_down mipi
[    1.145278] imx8m_blk_ctrl_power_on dispblk-lcdif
[    1.147316] imx_pgc_power_down dispmix
[    1.155804] imx_pgc_power_up dispmix
[    1.159820] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/soc@0/bus@32c00000/mipi_dsi@32e10000 to encoder None-34: -517
^^^ this is just a defer
[    1.171789] imx8m_blk_ctrl_power_off dispblk-lcdif
[    1.176655] imx_pgc_power_down dispmix
[    1.181790] libphy: fec_enet_mii_bus: probed
[    3.915806] panel-simple panel: Expected bpc in {6,8} but got: 0
^^^ not sure what this is but had it back in my 5.10 solution as well
so did not investigate
[    3.921912] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
[    3.926936] imx_pgc_power_up dispmix
[    3.931109] imx_pgc_power_up mipi
[    3.935409] imx8m_blk_ctrl_power_on dispblk-lcdif
[    3.940547] imx8m_blk_ctrl_power_off dispblk-lcdif
[    3.945626] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
32e00000.lcdif on minor 0
^^^ not clear why dispblk-lcdif got disabled here

Best regards,

Tim
[1] https://patchwork.kernel.org/project/dri-devel/list/?series=347439&archive=both&state=*
[2] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=359775&archive=both&state=*
[3] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20211106155427.753197-1-aford173@gmail.com/

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Jagan Teki]

On Wed, Nov 10, 2021 at 2:24 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Tue, Nov 9, 2021 at 12:39 PM Marek Vasut <marex@denx.de> wrote:
> >
> > On 11/9/21 8:35 PM, Adam Ford wrote:
> >
> > [...]
> >
> > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > >> index 208a0ed840f4..195dcbff7058 100644
> > >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > >> @@ -188,6 +188,12 @@
> > >>                  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";
> > >> @@ -1068,6 +1074,68 @@
> > >>                          #size-cells = <1>;
> > >>                          ranges = <0x32c00000 0x32c00000 0x400000>;
> > >>
> > >> +                       lcdif: lcdif@32e00000 {
> > >> +                               #address-cells = <1>;
> > >> +                               #size-cells = <0>;
> > >> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> > >
> > > The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> > > MXSFB_V4, but with overlays and those overlays have more registers
> > > configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> > > to see if it makes a difference?
> >
> > Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.
> >
> > LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with
> > slightly reordered register bits, but nothing like LCDIF rev3 (in MX23)
> > ... just to make sure there is no confusion.
> >
> > [...]
> >
> > >> +                       mipi_dsi: mipi_dsi@32e10000 {
> > >> +                               #address-cells = <1>;
> > >> +                               #size-cells = <0>;
> > >> +                               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>;
> >
> > 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 ?
>
> Thanks Marek.
>
> No, there is no dsim driver yet. I'm not clear if there is still
> dissagreement on if the drm/exynos driver can be split up or if a
> whole new somewhat duplicate driver needs to be made. I know Jagan
> also has a series he is working on that uses drm/exynos which I
> believe he will share an update on in a day or so.
>
> I'm still using the work that Michael [1] and you [2] did a long time back.
>
> I had this solution working on a 5.10 kernel but it was based on the
> old unapproved IMX8MM blk-ctl and pd drivers. Therefore I believe the
> issue I'm having is something not setup correctly with blk-ctl per my
> dt settings or perhaps something missing from the blk-ctl driver like
> Adam found [3]
>
> I am hanging at:
> [    1.064088] imx_pgc_power_up gpumix
> [    1.077506] imx_pgc_power_down gpumix
> [    1.097685] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> [    1.102682] imx_pgc_power_up dispmix
> [    1.106825] imx_pgc_power_up mipi
> [    1.110232] imx-dsim-dsi 32e10000.mipi_dsi: supply vddcore not
> found, using dummy regulator
> [    1.118760] imx-dsim-dsi 32e10000.mipi_dsi: supply vddio not found,
> using dummy regulator
> [    1.127361] imx-dsim-dsi 32e10000.mipi_dsi: [drm] *ERROR* modalias
> failure on /soc@0/bus@32c00000/mipi_dsi@32e10000/port@0
> [    1.138676] imx8m_blk_ctrl_power_off dispblk-mipi-dsi
> [    1.143788] imx_pgc_power_down mipi
> [    1.145278] imx8m_blk_ctrl_power_on dispblk-lcdif
> [    1.147316] imx_pgc_power_down dispmix
> [    1.155804] imx_pgc_power_up dispmix
> [    1.159820] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> /soc@0/bus@32c00000/mipi_dsi@32e10000 to encoder None-34: -517
> ^^^ this is just a defer
> [    1.171789] imx8m_blk_ctrl_power_off dispblk-lcdif
> [    1.176655] imx_pgc_power_down dispmix
> [    1.181790] libphy: fec_enet_mii_bus: probed
> [    3.915806] panel-simple panel: Expected bpc in {6,8} but got: 0
> ^^^ not sure what this is but had it back in my 5.10 solution as well
> so did not investigate
> [    3.921912] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> [    3.926936] imx_pgc_power_up dispmix
> [    3.931109] imx_pgc_power_up mipi
> [    3.935409] imx8m_blk_ctrl_power_on dispblk-lcdif
> [    3.940547] imx8m_blk_ctrl_power_off dispblk-lcdif
> [    3.945626] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
> 32e00000.lcdif on minor 0
> ^^^ not clear why dispblk-lcdif got disabled here

I've reproduced this. look like lcdif power-domains are not notified
ON. checking on the power-sequence for lcdif side. old patches from
Lucas on v5.13 seems working as expected.

Jagan.

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Jagan Teki]

On Wed, Nov 10, 2021 at 11:58 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Nov 10, 2021 at 2:24 AM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Tue, Nov 9, 2021 at 12:39 PM Marek Vasut <marex@denx.de> wrote:
> > >
> > > On 11/9/21 8:35 PM, Adam Ford wrote:
> > >
> > > [...]
> > >
> > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > >> index 208a0ed840f4..195dcbff7058 100644
> > > >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > >> @@ -188,6 +188,12 @@
> > > >>                  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";
> > > >> @@ -1068,6 +1074,68 @@
> > > >>                          #size-cells = <1>;
> > > >>                          ranges = <0x32c00000 0x32c00000 0x400000>;
> > > >>
> > > >> +                       lcdif: lcdif@32e00000 {
> > > >> +                               #address-cells = <1>;
> > > >> +                               #size-cells = <0>;
> > > >> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> > > >
> > > > The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> > > > MXSFB_V4, but with overlays and those overlays have more registers
> > > > configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> > > > to see if it makes a difference?
> > >
> > > Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.
> > >
> > > LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with
> > > slightly reordered register bits, but nothing like LCDIF rev3 (in MX23)
> > > ... just to make sure there is no confusion.
> > >
> > > [...]
> > >
> > > >> +                       mipi_dsi: mipi_dsi@32e10000 {
> > > >> +                               #address-cells = <1>;
> > > >> +                               #size-cells = <0>;
> > > >> +                               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>;
> > >
> > > 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 ?
> >
> > Thanks Marek.
> >
> > No, there is no dsim driver yet. I'm not clear if there is still
> > dissagreement on if the drm/exynos driver can be split up or if a
> > whole new somewhat duplicate driver needs to be made. I know Jagan
> > also has a series he is working on that uses drm/exynos which I
> > believe he will share an update on in a day or so.
> >
> > I'm still using the work that Michael [1] and you [2] did a long time back.
> >
> > I had this solution working on a 5.10 kernel but it was based on the
> > old unapproved IMX8MM blk-ctl and pd drivers. Therefore I believe the
> > issue I'm having is something not setup correctly with blk-ctl per my
> > dt settings or perhaps something missing from the blk-ctl driver like
> > Adam found [3]
> >
> > I am hanging at:
> > [    1.064088] imx_pgc_power_up gpumix
> > [    1.077506] imx_pgc_power_down gpumix
> > [    1.097685] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > [    1.102682] imx_pgc_power_up dispmix
> > [    1.106825] imx_pgc_power_up mipi
> > [    1.110232] imx-dsim-dsi 32e10000.mipi_dsi: supply vddcore not
> > found, using dummy regulator
> > [    1.118760] imx-dsim-dsi 32e10000.mipi_dsi: supply vddio not found,
> > using dummy regulator
> > [    1.127361] imx-dsim-dsi 32e10000.mipi_dsi: [drm] *ERROR* modalias
> > failure on /soc@0/bus@32c00000/mipi_dsi@32e10000/port@0
> > [    1.138676] imx8m_blk_ctrl_power_off dispblk-mipi-dsi
> > [    1.143788] imx_pgc_power_down mipi
> > [    1.145278] imx8m_blk_ctrl_power_on dispblk-lcdif
> > [    1.147316] imx_pgc_power_down dispmix
> > [    1.155804] imx_pgc_power_up dispmix
> > [    1.159820] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > /soc@0/bus@32c00000/mipi_dsi@32e10000 to encoder None-34: -517
> > ^^^ this is just a defer
> > [    1.171789] imx8m_blk_ctrl_power_off dispblk-lcdif
> > [    1.176655] imx_pgc_power_down dispmix
> > [    1.181790] libphy: fec_enet_mii_bus: probed
> > [    3.915806] panel-simple panel: Expected bpc in {6,8} but got: 0
> > ^^^ not sure what this is but had it back in my 5.10 solution as well
> > so did not investigate
> > [    3.921912] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > [    3.926936] imx_pgc_power_up dispmix
> > [    3.931109] imx_pgc_power_up mipi
> > [    3.935409] imx8m_blk_ctrl_power_on dispblk-lcdif
> > [    3.940547] imx8m_blk_ctrl_power_off dispblk-lcdif
> > [    3.945626] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
> > 32e00000.lcdif on minor 0
> > ^^^ not clear why dispblk-lcdif got disabled here
>
> I've reproduced this. look like lcdif power-domains are not notified
> ON. checking on the power-sequence for lcdif side. old patches from
> Lucas on v5.13 seems working as expected.

I've found the issue on lcdif atomic_enable, where the bus format is
retrieved from NULL bus_state. Here is the diff for it.

--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -359,13 +359,14 @@ static void mxsfb_crtc_atomic_enable(struct
drm_crtc *crtc,
        drm_crtc_vblank_on(crtc);

        /* If there is a bridge attached to the LCDIF, use its bus format */
-       if (mxsfb->bridge) {
+       if (mxsfb->bridge && state) {
                bridge_state =
                        drm_atomic_get_new_bridge_state(state,
                                                        mxsfb->bridge);
-               bus_format = bridge_state->input_bus_cfg.format;
+               if (bridge_state)
+                       bus_format = bridge_state->input_bus_cfg.format;

I believe this can be fixed via DSIM bridge. working on for those
changes, the key challenges is to make the DSIM to work even for
exynos, which indeed blocker for me to validate in hardware (i don't
have DSI based exynos).

Meanwhile, I have posed RFC for DSIM DTS changes, please check it here.
https://patchwork.kernel.org/project/dri-devel/cover/20211111101456.584061-1-jagan@amarulasolutions.com/

Jagan.

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Tim Harvey]

On Thu, Nov 11, 2021 at 2:19 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Nov 10, 2021 at 11:58 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Wed, Nov 10, 2021 at 2:24 AM Tim Harvey <tharvey@gateworks.com> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 12:39 PM Marek Vasut <marex@denx.de> wrote:
> > > >
> > > > On 11/9/21 8:35 PM, Adam Ford wrote:
> > > >
> > > > [...]
> > > >
> > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > >> index 208a0ed840f4..195dcbff7058 100644
> > > > >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > >> @@ -188,6 +188,12 @@
> > > > >>                  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";
> > > > >> @@ -1068,6 +1074,68 @@
> > > > >>                          #size-cells = <1>;
> > > > >>                          ranges = <0x32c00000 0x32c00000 0x400000>;
> > > > >>
> > > > >> +                       lcdif: lcdif@32e00000 {
> > > > >> +                               #address-cells = <1>;
> > > > >> +                               #size-cells = <0>;
> > > > >> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> > > > >
> > > > > The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> > > > > MXSFB_V4, but with overlays and those overlays have more registers
> > > > > configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> > > > > to see if it makes a difference?
> > > >
> > > > Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.
> > > >
> > > > LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with
> > > > slightly reordered register bits, but nothing like LCDIF rev3 (in MX23)
> > > > ... just to make sure there is no confusion.
> > > >
> > > > [...]
> > > >
> > > > >> +                       mipi_dsi: mipi_dsi@32e10000 {
> > > > >> +                               #address-cells = <1>;
> > > > >> +                               #size-cells = <0>;
> > > > >> +                               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>;
> > > >
> > > > 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 ?
> > >
> > > Thanks Marek.
> > >
> > > No, there is no dsim driver yet. I'm not clear if there is still
> > > dissagreement on if the drm/exynos driver can be split up or if a
> > > whole new somewhat duplicate driver needs to be made. I know Jagan
> > > also has a series he is working on that uses drm/exynos which I
> > > believe he will share an update on in a day or so.
> > >
> > > I'm still using the work that Michael [1] and you [2] did a long time back.
> > >
> > > I had this solution working on a 5.10 kernel but it was based on the
> > > old unapproved IMX8MM blk-ctl and pd drivers. Therefore I believe the
> > > issue I'm having is something not setup correctly with blk-ctl per my
> > > dt settings or perhaps something missing from the blk-ctl driver like
> > > Adam found [3]
> > >
> > > I am hanging at:
> > > [    1.064088] imx_pgc_power_up gpumix
> > > [    1.077506] imx_pgc_power_down gpumix
> > > [    1.097685] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > [    1.102682] imx_pgc_power_up dispmix
> > > [    1.106825] imx_pgc_power_up mipi
> > > [    1.110232] imx-dsim-dsi 32e10000.mipi_dsi: supply vddcore not
> > > found, using dummy regulator
> > > [    1.118760] imx-dsim-dsi 32e10000.mipi_dsi: supply vddio not found,
> > > using dummy regulator
> > > [    1.127361] imx-dsim-dsi 32e10000.mipi_dsi: [drm] *ERROR* modalias
> > > failure on /soc@0/bus@32c00000/mipi_dsi@32e10000/port@0
> > > [    1.138676] imx8m_blk_ctrl_power_off dispblk-mipi-dsi
> > > [    1.143788] imx_pgc_power_down mipi
> > > [    1.145278] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > [    1.147316] imx_pgc_power_down dispmix
> > > [    1.155804] imx_pgc_power_up dispmix
> > > [    1.159820] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > > /soc@0/bus@32c00000/mipi_dsi@32e10000 to encoder None-34: -517
> > > ^^^ this is just a defer
> > > [    1.171789] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > [    1.176655] imx_pgc_power_down dispmix
> > > [    1.181790] libphy: fec_enet_mii_bus: probed
> > > [    3.915806] panel-simple panel: Expected bpc in {6,8} but got: 0
> > > ^^^ not sure what this is but had it back in my 5.10 solution as well
> > > so did not investigate
> > > [    3.921912] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > [    3.926936] imx_pgc_power_up dispmix
> > > [    3.931109] imx_pgc_power_up mipi
> > > [    3.935409] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > [    3.940547] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > [    3.945626] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
> > > 32e00000.lcdif on minor 0
> > > ^^^ not clear why dispblk-lcdif got disabled here
> >
> > I've reproduced this. look like lcdif power-domains are not notified
> > ON. checking on the power-sequence for lcdif side. old patches from
> > Lucas on v5.13 seems working as expected.
>
> I've found the issue on lcdif atomic_enable, where the bus format is
> retrieved from NULL bus_state. Here is the diff for it.
>
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -359,13 +359,14 @@ static void mxsfb_crtc_atomic_enable(struct
> drm_crtc *crtc,
>         drm_crtc_vblank_on(crtc);
>
>         /* If there is a bridge attached to the LCDIF, use its bus format */
> -       if (mxsfb->bridge) {
> +       if (mxsfb->bridge && state) {
>                 bridge_state =
>                         drm_atomic_get_new_bridge_state(state,
>                                                         mxsfb->bridge);
> -               bus_format = bridge_state->input_bus_cfg.format;
> +               if (bridge_state)
> +                       bus_format = bridge_state->input_bus_cfg.format;
>'
> I believe this can be fixed via DSIM bridge. working on for those
> changes, the key challenges is to make the DSIM to work even for
> exynos, which indeed blocker for me to validate in hardware (i don't
> have DSI based exynos).
>
> Meanwhile, I have posed RFC for DSIM DTS changes, please check it here.
> https://patchwork.kernel.org/project/dri-devel/cover/20211111101456.584061-1-jagan@amarulasolutions.com/
>

Jagan,

Thank you! This does resolve the hang on my end as well. I will look
at your 'arm64: imx8mm: Add MIPI DSI support' series.

There was some discussion regarding giving up on trying to split up
the exynos driver such that it could work with IMX8MM vs just
duplicating it. I thought the recommendation was to duplicate it as it
wasn't clear if there was a way to split it out without breaking
current exynos DSI but I'll have to see if I can find the thread.

Best regards,

Tim

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Michael Tretter]

On Thu, 11 Nov 2021 13:21:03 -0800, Tim Harvey wrote:
> On Thu, Nov 11, 2021 at 2:19 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > On Wed, Nov 10, 2021 at 11:58 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > On Wed, Nov 10, 2021 at 2:24 AM Tim Harvey <tharvey@gateworks.com> wrote:
> > > > On Tue, Nov 9, 2021 at 12:39 PM Marek Vasut <marex@denx.de> wrote:
> > > > > On 11/9/21 8:35 PM, Adam Ford wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > >> index 208a0ed840f4..195dcbff7058 100644
> > > > > >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > >> @@ -188,6 +188,12 @@
> > > > > >>                  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";
> > > > > >> @@ -1068,6 +1074,68 @@
> > > > > >>                          #size-cells = <1>;
> > > > > >>                          ranges = <0x32c00000 0x32c00000 0x400000>;
> > > > > >>
> > > > > >> +                       lcdif: lcdif@32e00000 {
> > > > > >> +                               #address-cells = <1>;
> > > > > >> +                               #size-cells = <0>;
> > > > > >> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> > > > > >
> > > > > > The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> > > > > > MXSFB_V4, but with overlays and those overlays have more registers
> > > > > > configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> > > > > > to see if it makes a difference?
> > > > >
> > > > > Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.
> > > > >
> > > > > LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with
> > > > > slightly reordered register bits, but nothing like LCDIF rev3 (in MX23)
> > > > > ... just to make sure there is no confusion.
> > > > >
> > > > > [...]
> > > > >
> > > > > >> +                       mipi_dsi: mipi_dsi@32e10000 {
> > > > > >> +                               #address-cells = <1>;
> > > > > >> +                               #size-cells = <0>;
> > > > > >> +                               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>;
> > > > >
> > > > > 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 ?
> > > >
> > > > Thanks Marek.
> > > >
> > > > No, there is no dsim driver yet. I'm not clear if there is still
> > > > dissagreement on if the drm/exynos driver can be split up or if a
> > > > whole new somewhat duplicate driver needs to be made. I know Jagan
> > > > also has a series he is working on that uses drm/exynos which I
> > > > believe he will share an update on in a day or so.
> > > >
> > > > I'm still using the work that Michael [1] and you [2] did a long time back.
> > > >
> > > > I had this solution working on a 5.10 kernel but it was based on the
> > > > old unapproved IMX8MM blk-ctl and pd drivers. Therefore I believe the
> > > > issue I'm having is something not setup correctly with blk-ctl per my
> > > > dt settings or perhaps something missing from the blk-ctl driver like
> > > > Adam found [3]
> > > >
> > > > I am hanging at:
> > > > [    1.064088] imx_pgc_power_up gpumix
> > > > [    1.077506] imx_pgc_power_down gpumix
> > > > [    1.097685] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > > [    1.102682] imx_pgc_power_up dispmix
> > > > [    1.106825] imx_pgc_power_up mipi
> > > > [    1.110232] imx-dsim-dsi 32e10000.mipi_dsi: supply vddcore not
> > > > found, using dummy regulator
> > > > [    1.118760] imx-dsim-dsi 32e10000.mipi_dsi: supply vddio not found,
> > > > using dummy regulator
> > > > [    1.127361] imx-dsim-dsi 32e10000.mipi_dsi: [drm] *ERROR* modalias
> > > > failure on /soc@0/bus@32c00000/mipi_dsi@32e10000/port@0
> > > > [    1.138676] imx8m_blk_ctrl_power_off dispblk-mipi-dsi
> > > > [    1.143788] imx_pgc_power_down mipi
> > > > [    1.145278] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > > [    1.147316] imx_pgc_power_down dispmix
> > > > [    1.155804] imx_pgc_power_up dispmix
> > > > [    1.159820] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > > > /soc@0/bus@32c00000/mipi_dsi@32e10000 to encoder None-34: -517
> > > > ^^^ this is just a defer
> > > > [    1.171789] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > > [    1.176655] imx_pgc_power_down dispmix
> > > > [    1.181790] libphy: fec_enet_mii_bus: probed
> > > > [    3.915806] panel-simple panel: Expected bpc in {6,8} but got: 0
> > > > ^^^ not sure what this is but had it back in my 5.10 solution as well
> > > > so did not investigate
> > > > [    3.921912] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > > [    3.926936] imx_pgc_power_up dispmix
> > > > [    3.931109] imx_pgc_power_up mipi
> > > > [    3.935409] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > > [    3.940547] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > > [    3.945626] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
> > > > 32e00000.lcdif on minor 0
> > > > ^^^ not clear why dispblk-lcdif got disabled here
> > >
> > > I've reproduced this. look like lcdif power-domains are not notified
> > > ON. checking on the power-sequence for lcdif side. old patches from
> > > Lucas on v5.13 seems working as expected.
> >
> > I've found the issue on lcdif atomic_enable, where the bus format is
> > retrieved from NULL bus_state. Here is the diff for it.
> >
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > @@ -359,13 +359,14 @@ static void mxsfb_crtc_atomic_enable(struct
> > drm_crtc *crtc,
> >         drm_crtc_vblank_on(crtc);
> >
> >         /* If there is a bridge attached to the LCDIF, use its bus format */
> > -       if (mxsfb->bridge) {
> > +       if (mxsfb->bridge && state) {
> >                 bridge_state =
> >                         drm_atomic_get_new_bridge_state(state,
> >                                                         mxsfb->bridge);
> > -               bus_format = bridge_state->input_bus_cfg.format;
> > +               if (bridge_state)
> > +                       bus_format = bridge_state->input_bus_cfg.format;
> >'
> > I believe this can be fixed via DSIM bridge. working on for those
> > changes, the key challenges is to make the DSIM to work even for
> > exynos, which indeed blocker for me to validate in hardware (i don't
> > have DSI based exynos).
> >
> > Meanwhile, I have posed RFC for DSIM DTS changes, please check it here.
> > https://patchwork.kernel.org/project/dri-devel/cover/20211111101456.584061-1-jagan@amarulasolutions.com/
> >
> 
> Jagan,
> 
> Thank you! This does resolve the hang on my end as well. I will look
> at your 'arm64: imx8mm: Add MIPI DSI support' series.
> 
> There was some discussion regarding giving up on trying to split up
> the exynos driver such that it could work with IMX8MM vs just
> duplicating it. I thought the recommendation was to duplicate it as it
> wasn't clear if there was a way to split it out without breaking
> current exynos DSI but I'll have to see if I can find the thread.

The thread is here:

https://lore.kernel.org/all/CAKMK7uF0B1TrtVX+9Whasz49quha_is+77z2wX3c06BsWRiPcQ@mail.gmail.com/

Duplicating seems to be the best way forward, because the Exynos driver
exposes some special behavior (discussed earlier in the same thread), which
isn't compatible with how bridges work.

Michael

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Jagan Teki]

On Fri, Nov 12, 2021 at 2:12 PM Michael Tretter
<m.tretter@pengutronix.de> wrote:
>
> On Thu, 11 Nov 2021 13:21:03 -0800, Tim Harvey wrote:
> > On Thu, Nov 11, 2021 at 2:19 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > On Wed, Nov 10, 2021 at 11:58 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > On Wed, Nov 10, 2021 at 2:24 AM Tim Harvey <tharvey@gateworks.com> wrote:
> > > > > On Tue, Nov 9, 2021 at 12:39 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > On 11/9/21 8:35 PM, Adam Ford wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > > >> index 208a0ed840f4..195dcbff7058 100644
> > > > > > >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > > >> @@ -188,6 +188,12 @@
> > > > > > >>                  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";
> > > > > > >> @@ -1068,6 +1074,68 @@
> > > > > > >>                          #size-cells = <1>;
> > > > > > >>                          ranges = <0x32c00000 0x32c00000 0x400000>;
> > > > > > >>
> > > > > > >> +                       lcdif: lcdif@32e00000 {
> > > > > > >> +                               #address-cells = <1>;
> > > > > > >> +                               #size-cells = <0>;
> > > > > > >> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> > > > > > >
> > > > > > > The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> > > > > > > MXSFB_V4, but with overlays and those overlays have more registers
> > > > > > > configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> > > > > > > to see if it makes a difference?
> > > > > >
> > > > > > Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.
> > > > > >
> > > > > > LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with
> > > > > > slightly reordered register bits, but nothing like LCDIF rev3 (in MX23)
> > > > > > ... just to make sure there is no confusion.
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > >> +                       mipi_dsi: mipi_dsi@32e10000 {
> > > > > > >> +                               #address-cells = <1>;
> > > > > > >> +                               #size-cells = <0>;
> > > > > > >> +                               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>;
> > > > > >
> > > > > > 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 ?
> > > > >
> > > > > Thanks Marek.
> > > > >
> > > > > No, there is no dsim driver yet. I'm not clear if there is still
> > > > > dissagreement on if the drm/exynos driver can be split up or if a
> > > > > whole new somewhat duplicate driver needs to be made. I know Jagan
> > > > > also has a series he is working on that uses drm/exynos which I
> > > > > believe he will share an update on in a day or so.
> > > > >
> > > > > I'm still using the work that Michael [1] and you [2] did a long time back.
> > > > >
> > > > > I had this solution working on a 5.10 kernel but it was based on the
> > > > > old unapproved IMX8MM blk-ctl and pd drivers. Therefore I believe the
> > > > > issue I'm having is something not setup correctly with blk-ctl per my
> > > > > dt settings or perhaps something missing from the blk-ctl driver like
> > > > > Adam found [3]
> > > > >
> > > > > I am hanging at:
> > > > > [    1.064088] imx_pgc_power_up gpumix
> > > > > [    1.077506] imx_pgc_power_down gpumix
> > > > > [    1.097685] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > > > [    1.102682] imx_pgc_power_up dispmix
> > > > > [    1.106825] imx_pgc_power_up mipi
> > > > > [    1.110232] imx-dsim-dsi 32e10000.mipi_dsi: supply vddcore not
> > > > > found, using dummy regulator
> > > > > [    1.118760] imx-dsim-dsi 32e10000.mipi_dsi: supply vddio not found,
> > > > > using dummy regulator
> > > > > [    1.127361] imx-dsim-dsi 32e10000.mipi_dsi: [drm] *ERROR* modalias
> > > > > failure on /soc@0/bus@32c00000/mipi_dsi@32e10000/port@0
> > > > > [    1.138676] imx8m_blk_ctrl_power_off dispblk-mipi-dsi
> > > > > [    1.143788] imx_pgc_power_down mipi
> > > > > [    1.145278] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > > > [    1.147316] imx_pgc_power_down dispmix
> > > > > [    1.155804] imx_pgc_power_up dispmix
> > > > > [    1.159820] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > > > > /soc@0/bus@32c00000/mipi_dsi@32e10000 to encoder None-34: -517
> > > > > ^^^ this is just a defer
> > > > > [    1.171789] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > > > [    1.176655] imx_pgc_power_down dispmix
> > > > > [    1.181790] libphy: fec_enet_mii_bus: probed
> > > > > [    3.915806] panel-simple panel: Expected bpc in {6,8} but got: 0
> > > > > ^^^ not sure what this is but had it back in my 5.10 solution as well
> > > > > so did not investigate
> > > > > [    3.921912] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > > > [    3.926936] imx_pgc_power_up dispmix
> > > > > [    3.931109] imx_pgc_power_up mipi
> > > > > [    3.935409] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > > > [    3.940547] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > > > [    3.945626] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
> > > > > 32e00000.lcdif on minor 0
> > > > > ^^^ not clear why dispblk-lcdif got disabled here
> > > >
> > > > I've reproduced this. look like lcdif power-domains are not notified
> > > > ON. checking on the power-sequence for lcdif side. old patches from
> > > > Lucas on v5.13 seems working as expected.
> > >
> > > I've found the issue on lcdif atomic_enable, where the bus format is
> > > retrieved from NULL bus_state. Here is the diff for it.
> > >
> > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > > @@ -359,13 +359,14 @@ static void mxsfb_crtc_atomic_enable(struct
> > > drm_crtc *crtc,
> > >         drm_crtc_vblank_on(crtc);
> > >
> > >         /* If there is a bridge attached to the LCDIF, use its bus format */
> > > -       if (mxsfb->bridge) {
> > > +       if (mxsfb->bridge && state) {
> > >                 bridge_state =
> > >                         drm_atomic_get_new_bridge_state(state,
> > >                                                         mxsfb->bridge);
> > > -               bus_format = bridge_state->input_bus_cfg.format;
> > > +               if (bridge_state)
> > > +                       bus_format = bridge_state->input_bus_cfg.format;
> > >'
> > > I believe this can be fixed via DSIM bridge. working on for those
> > > changes, the key challenges is to make the DSIM to work even for
> > > exynos, which indeed blocker for me to validate in hardware (i don't
> > > have DSI based exynos).
> > >
> > > Meanwhile, I have posed RFC for DSIM DTS changes, please check it here.
> > > https://patchwork.kernel.org/project/dri-devel/cover/20211111101456.584061-1-jagan@amarulasolutions.com/
> > >
> >
> > Jagan,
> >
> > Thank you! This does resolve the hang on my end as well. I will look
> > at your 'arm64: imx8mm: Add MIPI DSI support' series.
> >
> > There was some discussion regarding giving up on trying to split up
> > the exynos driver such that it could work with IMX8MM vs just
> > duplicating it. I thought the recommendation was to duplicate it as it
> > wasn't clear if there was a way to split it out without breaking
> > current exynos DSI but I'll have to see if I can find the thread.
>
> The thread is here:
>
> https://lore.kernel.org/all/CAKMK7uF0B1TrtVX+9Whasz49quha_is+77z2wX3c06BsWRiPcQ@mail.gmail.com/
>
> Duplicating seems to be the best way forward, because the Exynos driver
> exposes some special behavior (discussed earlier in the same thread), which
> isn't compatible with how bridges work.

Not quite sure about it. Laurent and Inki had similar discussion and
looking for common bridge [1].

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210704090230.26489-7-jagan@amarulasolutions.com/

Jagan.

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Laurent Pinchart]

On Fri, Nov 12, 2021 at 02:28:30PM +0530, Jagan Teki wrote:
> On Fri, Nov 12, 2021 at 2:12 PM Michael Tretter wrote:
> > On Thu, 11 Nov 2021 13:21:03 -0800, Tim Harvey wrote:
> > > On Thu, Nov 11, 2021 at 2:19 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > On Wed, Nov 10, 2021 at 11:58 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > On Wed, Nov 10, 2021 at 2:24 AM Tim Harvey <tharvey@gateworks.com> wrote:
> > > > > > On Tue, Nov 9, 2021 at 12:39 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > > On 11/9/21 8:35 PM, Adam Ford wrote:
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > > > >> index 208a0ed840f4..195dcbff7058 100644
> > > > > > > >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > > > >> @@ -188,6 +188,12 @@
> > > > > > > >>                  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";
> > > > > > > >> @@ -1068,6 +1074,68 @@
> > > > > > > >>                          #size-cells = <1>;
> > > > > > > >>                          ranges = <0x32c00000 0x32c00000 0x400000>;
> > > > > > > >>
> > > > > > > >> +                       lcdif: lcdif@32e00000 {
> > > > > > > >> +                               #address-cells = <1>;
> > > > > > > >> +                               #size-cells = <0>;
> > > > > > > >> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> > > > > > > >
> > > > > > > > The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> > > > > > > > MXSFB_V4, but with overlays and those overlays have more registers
> > > > > > > > configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> > > > > > > > to see if it makes a difference?
> > > > > > >
> > > > > > > Indeed, MX6SX has AS overlay plane support, MX{2,}8 does not.
> > > > > > >
> > > > > > > LCDIFv3 (as NXP calls it) in MX8MP is like LCDIFv6 (in MX6SX) with
> > > > > > > slightly reordered register bits, but nothing like LCDIF rev3 (in MX23)
> > > > > > > ... just to make sure there is no confusion.
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > >> +                       mipi_dsi: mipi_dsi@32e10000 {
> > > > > > > >> +                               #address-cells = <1>;
> > > > > > > >> +                               #size-cells = <0>;
> > > > > > > >> +                               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>;
> > > > > > >
> > > > > > > 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 ?
> > > > > >
> > > > > > Thanks Marek.
> > > > > >
> > > > > > No, there is no dsim driver yet. I'm not clear if there is still
> > > > > > dissagreement on if the drm/exynos driver can be split up or if a
> > > > > > whole new somewhat duplicate driver needs to be made. I know Jagan
> > > > > > also has a series he is working on that uses drm/exynos which I
> > > > > > believe he will share an update on in a day or so.
> > > > > >
> > > > > > I'm still using the work that Michael [1] and you [2] did a long time back.
> > > > > >
> > > > > > I had this solution working on a 5.10 kernel but it was based on the
> > > > > > old unapproved IMX8MM blk-ctl and pd drivers. Therefore I believe the
> > > > > > issue I'm having is something not setup correctly with blk-ctl per my
> > > > > > dt settings or perhaps something missing from the blk-ctl driver like
> > > > > > Adam found [3]
> > > > > >
> > > > > > I am hanging at:
> > > > > > [    1.064088] imx_pgc_power_up gpumix
> > > > > > [    1.077506] imx_pgc_power_down gpumix
> > > > > > [    1.097685] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > > > > [    1.102682] imx_pgc_power_up dispmix
> > > > > > [    1.106825] imx_pgc_power_up mipi
> > > > > > [    1.110232] imx-dsim-dsi 32e10000.mipi_dsi: supply vddcore not
> > > > > > found, using dummy regulator
> > > > > > [    1.118760] imx-dsim-dsi 32e10000.mipi_dsi: supply vddio not found,
> > > > > > using dummy regulator
> > > > > > [    1.127361] imx-dsim-dsi 32e10000.mipi_dsi: [drm] *ERROR* modalias
> > > > > > failure on /soc@0/bus@32c00000/mipi_dsi@32e10000/port@0
> > > > > > [    1.138676] imx8m_blk_ctrl_power_off dispblk-mipi-dsi
> > > > > > [    1.143788] imx_pgc_power_down mipi
> > > > > > [    1.145278] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > > > > [    1.147316] imx_pgc_power_down dispmix
> > > > > > [    1.155804] imx_pgc_power_up dispmix
> > > > > > [    1.159820] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > > > > > /soc@0/bus@32c00000/mipi_dsi@32e10000 to encoder None-34: -517
> > > > > > ^^^ this is just a defer
> > > > > > [    1.171789] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > > > > [    1.176655] imx_pgc_power_down dispmix
> > > > > > [    1.181790] libphy: fec_enet_mii_bus: probed
> > > > > > [    3.915806] panel-simple panel: Expected bpc in {6,8} but got: 0
> > > > > > ^^^ not sure what this is but had it back in my 5.10 solution as well
> > > > > > so did not investigate
> > > > > > [    3.921912] imx8m_blk_ctrl_power_on dispblk-mipi-dsi
> > > > > > [    3.926936] imx_pgc_power_up dispmix
> > > > > > [    3.931109] imx_pgc_power_up mipi
> > > > > > [    3.935409] imx8m_blk_ctrl_power_on dispblk-lcdif
> > > > > > [    3.940547] imx8m_blk_ctrl_power_off dispblk-lcdif
> > > > > > [    3.945626] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
> > > > > > 32e00000.lcdif on minor 0
> > > > > > ^^^ not clear why dispblk-lcdif got disabled here
> > > > >
> > > > > I've reproduced this. look like lcdif power-domains are not notified
> > > > > ON. checking on the power-sequence for lcdif side. old patches from
> > > > > Lucas on v5.13 seems working as expected.
> > > >
> > > > I've found the issue on lcdif atomic_enable, where the bus format is
> > > > retrieved from NULL bus_state. Here is the diff for it.
> > > >
> > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > > > @@ -359,13 +359,14 @@ static void mxsfb_crtc_atomic_enable(struct
> > > > drm_crtc *crtc,
> > > >         drm_crtc_vblank_on(crtc);
> > > >
> > > >         /* If there is a bridge attached to the LCDIF, use its bus format */
> > > > -       if (mxsfb->bridge) {
> > > > +       if (mxsfb->bridge && state) {
> > > >                 bridge_state =
> > > >                         drm_atomic_get_new_bridge_state(state,
> > > >                                                         mxsfb->bridge);
> > > > -               bus_format = bridge_state->input_bus_cfg.format;
> > > > +               if (bridge_state)
> > > > +                       bus_format = bridge_state->input_bus_cfg.format;
> > > >'
> > > > I believe this can be fixed via DSIM bridge. working on for those
> > > > changes, the key challenges is to make the DSIM to work even for
> > > > exynos, which indeed blocker for me to validate in hardware (i don't
> > > > have DSI based exynos).
> > > >
> > > > Meanwhile, I have posed RFC for DSIM DTS changes, please check it here.
> > > > https://patchwork.kernel.org/project/dri-devel/cover/20211111101456.584061-1-jagan@amarulasolutions.com/
> > > >
> > >
> > > Jagan,
> > >
> > > Thank you! This does resolve the hang on my end as well. I will look
> > > at your 'arm64: imx8mm: Add MIPI DSI support' series.
> > >
> > > There was some discussion regarding giving up on trying to split up
> > > the exynos driver such that it could work with IMX8MM vs just
> > > duplicating it. I thought the recommendation was to duplicate it as it
> > > wasn't clear if there was a way to split it out without breaking
> > > current exynos DSI but I'll have to see if I can find the thread.
> >
> > The thread is here:
> >
> > https://lore.kernel.org/all/CAKMK7uF0B1TrtVX+9Whasz49quha_is+77z2wX3c06BsWRiPcQ@mail.gmail.com/
> >
> > Duplicating seems to be the best way forward, because the Exynos driver
> > exposes some special behavior (discussed earlier in the same thread), which
> > isn't compatible with how bridges work.

This seems to tbe the crux of the issue. I don't see what would be
specific to Exynos there, those issues should be fixed globally. If the
DRM bridge API can't support some use cases, it should be improved.

> Not quite sure about it. Laurent and Inki had similar discussion and
> looking for common bridge [1].
> 
> [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210704090230.26489-7-jagan@amarulasolutions.com/

-- 
Regards,

Laurent Pinchart

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Tim Harvey]

On Tue, Nov 9, 2021 at 11:36 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 11:34 AM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > Add nodes for MIPI DSI and LCDIF on IMX8MM
> >
> > I'm currently working with a set of patches to convert drm/exynos
> > to a bridge [1] and add IMX8MM support [2] in order to get IMX8MM DSI
> > working for display with a Raspberry Pi DSI touchscreen compatible with
> > a Toshiba TC358762 DSI bridge and Powertip PH800480T013-IDF02
> > touchscreen.
> >
> > I had this working on a 5.10 kernel with the old blk-ctl and
> > power-domain drivers that didn't make it into mainline but my 5.15
> > series with blk-ctl backported from next hangs right after
> > "[drm] Initialized mxsfb-drm 1.0.0 20160824 for 32e00000.lcdif on minor 0"
> > so I assume I have a power-domain not getting enabled.
> >
> > Please let me know if you see an issue with the way I've configured
> > power-domain or clocks here.
> >
> > Best Regards,
> >
> > Tim
> > [1] https://patchwork.kernel.org/project/dri-devel/list/?series=347439&archive=both&state=*
> > [2] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=359775&archive=both&state=*
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 68 +++++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index 208a0ed840f4..195dcbff7058 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -188,6 +188,12 @@
> >                 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";
> > @@ -1068,6 +1074,68 @@
> >                         #size-cells = <1>;
> >                         ranges = <0x32c00000 0x32c00000 0x400000>;
> >
> > +                       lcdif: lcdif@32e00000 {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
>
> The compatible "imx6sx-lcdif" implies MXSFB_V6.  FWICT, it is like
> MXSFB_V4, but with overlays and those overlays have more registers
> configured in the mxsfb_kms driver.  Have you tried using imx28-lcdif
> to see if it makes a difference?

Adam,

Yes, I see now a discussion regarding this [1]. Changing to
'imx28-lcdif' still hangs.

[1] https://patchwork.kernel.org/project/dri-devel/patch/20210620224834.189411-1-marex@denx.de/

>
> > +                               reg = <0x32e00000 0x10000>;
> > +                               clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> > +                                        <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
> > +                                        <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> > +                               clock-names = "pix", "disp_axi", "axi";
> > +                               assigned-clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> > +                                                 <&clk IMX8MM_CLK_DISP_AXI>,
> > +                                                 <&clk IMX8MM_CLK_DISP_APB>;
> > +                               assigned-clock-parents = <&clk IMX8MM_VIDEO_PLL1_OUT>,
> > +                                                        <&clk IMX8MM_SYS_PLL2_1000M>,
> > +                                                        <&clk IMX8MM_SYS_PLL1_800M>;
> > +                               assigned-clock-rate = <594000000>, <500000000>, <200000000>;
>
> Just through visual inspection, it appears that the
> IMX8MM_CLK_DISP_AXI and IMX8MM_CLK_DISP_APB clock-parents and rates
> are already set in the pgc_dispmix, so I think it's safe to reduce
> those lines to just assigning IMX8MM_CLK_LCDIF_PIXEL to the
> IMX8MM_VIDEO_PLL1_OUT with the assigned-clock-rate set to 594000000.
>

Ok, that makes sense.

Incidentally I added some debug prints in imx_pgc_power_{up,down} and
find we hang at:
imx_pgc_power_down mipi

Best Regards,

Tim

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Adam Ford]

On Tue, Nov 9, 2021 at 11:34 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> Add nodes for MIPI DSI and LCDIF on IMX8MM
>
> I'm currently working with a set of patches to convert drm/exynos
> to a bridge [1] and add IMX8MM support [2] in order to get IMX8MM DSI
> working for display with a Raspberry Pi DSI touchscreen compatible with
> a Toshiba TC358762 DSI bridge and Powertip PH800480T013-IDF02
> touchscreen.
>
> I had this working on a 5.10 kernel with the old blk-ctl and
> power-domain drivers that didn't make it into mainline but my 5.15
> series with blk-ctl backported from next hangs right after
> "[drm] Initialized mxsfb-drm 1.0.0 20160824 for 32e00000.lcdif on minor 0"
> so I assume I have a power-domain not getting enabled.
>
> Please let me know if you see an issue with the way I've configured
> power-domain or clocks here.
>
> Best Regards,
>
> Tim
> [1] https://patchwork.kernel.org/project/dri-devel/list/?series=347439&archive=both&state=*
> [2] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=359775&archive=both&state=*
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 68 +++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 208a0ed840f4..195dcbff7058 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -188,6 +188,12 @@
>                 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";
> @@ -1068,6 +1074,68 @@
>                         #size-cells = <1>;
>                         ranges = <0x32c00000 0x32c00000 0x400000>;
>
> +                       lcdif: lcdif@32e00000 {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;

If there is only 1 port I don't think you need address-cells and
size-cells.  See more below....

> +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> +                               reg = <0x32e00000 0x10000>;
> +                               clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> +                                        <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
> +                                        <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> +                               clock-names = "pix", "disp_axi", "axi";
> +                               assigned-clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> +                                                 <&clk IMX8MM_CLK_DISP_AXI>,
> +                                                 <&clk IMX8MM_CLK_DISP_APB>;
> +                               assigned-clock-parents = <&clk IMX8MM_VIDEO_PLL1_OUT>,
> +                                                        <&clk IMX8MM_SYS_PLL2_1000M>,
> +                                                        <&clk IMX8MM_SYS_PLL1_800M>;
> +                               assigned-clock-rate = <594000000>, <500000000>, <200000000>;
> +                               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> +                               power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_LCDIF>;
> +                               status = "disabled";
> +
> +                               port@0 {
> +                                       reg = <0>;

This should be just "port {" and we can get rid of the @0 and the "reg=0"

This eliminates some build warnings.
> +
> +                                       lcdif_to_dsim: endpoint {
> +                                               remote-endpoint = <&dsim_from_lcdif>;
> +                                       };
> +                               };
> +                       };
> +
> +                       mipi_dsi: mipi_dsi@32e10000 {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;

I think this should move into a sub node...see below

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

I think the mipi_phy is the equivalent to the mipi_rst_mask in the
power domain I added to the CSI.  Looking through the NXP 5.10 kernel,
 BIT(17) is the reset for the MIPI_DSI.  I haven't tried it yet, but
it makes me assume that BIT(16) is for the MIPI_CSI.

Both bits are set in the imx-atf code, and the documentation isn't
clear, but I tweaked the blk-ctrl driver to only set BIT(16) for the
mipi_rst_mask on the CSI.  I still get some hanging, so it's not any
better that way.

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

Add a subnode called "ports" and move address-cells and size-cells
into it along wtih "port@0"

> +                               port@0 {
> +                                       reg = <0>;
> +
> +                                       dsim_from_lcdif: endpoint {
> +                                               remote-endpoint = <&lcdif_to_dsim>;
> +                                       };
> +                               };
> +                       };
> +
>                         csi: csi@32e20000 {
>                                 compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
>                                 reg = <0x32e20000 0x1000>;
> --
> 2.17.1
>

Re: [RFC] arm64: dts: imx8mm: Add MIPI and LCDIF nodes

[Adam Ford]

On Tue, Nov 9, 2021 at 6:24 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 11:34 AM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > Add nodes for MIPI DSI and LCDIF on IMX8MM
> >
> > I'm currently working with a set of patches to convert drm/exynos
> > to a bridge [1] and add IMX8MM support [2] in order to get IMX8MM DSI
> > working for display with a Raspberry Pi DSI touchscreen compatible with
> > a Toshiba TC358762 DSI bridge and Powertip PH800480T013-IDF02
> > touchscreen.
> >
> > I had this working on a 5.10 kernel with the old blk-ctl and
> > power-domain drivers that didn't make it into mainline but my 5.15
> > series with blk-ctl backported from next hangs right after
> > "[drm] Initialized mxsfb-drm 1.0.0 20160824 for 32e00000.lcdif on minor 0"
> > so I assume I have a power-domain not getting enabled.
> >
> > Please let me know if you see an issue with the way I've configured
> > power-domain or clocks here.


I don't get hanging, but the samsung-dsi-imx driver doesn't load
automatically for some reason, so I manually modprobe it.  In my
configuration, I have lcdif->dsi->hdmi-bridge
When I modprobe the samsung-dsim-imx driver, I get the following:

[   50.217288] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000010
[   50.217292] Mem abort info:
[   50.217294]   ESR = 0x96000004
[   50.217297]   EC = 0x25: DABT (current EL), IL = 32 bits
[   50.217301]   SET = 0, FnV = 0
[   50.217303]   EA = 0, S1PTW = 0
[   50.217306]   FSC = 0x04: level 0 translation fault
[   50.217309] Data abort info:
[   50.217311]   ISV = 0, ISS = 0x00000004
[   50.217313]   CM = 0, WnR = 0
[   50.217316] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000044dfb000
[   50.217320] [0000000000000010] pgd=0000000000000000, p4d=0000000000000000
[   50.217331] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[   50.217336] Modules linked in: samsung_dsim_imx samsung_dsim 8021q
garp mrp stp llc caam_jr caamhash_desc caamalg_desc snd_soc_hdmi_codec
crypto_engine rng_core authenc libdes brcmfmac brcmutil cfg80211
crct10dif_ce etnaviv snd_soc_fsl_asoc_card mxsfb snd_soc_imx_audmux
gpu_sched fsl_imx8_ddr_perf imx8m_ddrc spi_imx spi_bitbang adv7511 cec
drm_kms_helper at24 caam clk_bd718x7 snd_soc_wm8962 rtc_pcf85363 error
rtc_snvs snvs_pwrkey imx8mm_thermal snd_soc_fsl_sai imx_pcm_dma
snd_soc_simple_card snd_soc_simple_card_utils imx_cpufreq_dt bluetooth
display_connector ecdh_generic ecc rfkill fuse drm ipv6
[   50.217453] CPU: 0 PID: 252 Comm: kworker/u8:4 Not tainted
5.15.0-ga6004e62a74a-dirty #3
[   50.217459] Hardware name: Beacon EmbeddedWorks i.MX8M Mini
Development Kit (DT)
[   50.217464] Workqueue: events_unbound deferred_probe_work_func
[   50.217482] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   50.217488] pc : mxsfb_crtc_atomic_enable+0x60/0x5e0 [mxsfb]
[   50.217500] lr : mxsfb_crtc_atomic_enable+0x60/0x5e0 [mxsfb]
[   50.217508] sp : ffff800012b6b500
[   50.217510] x29: ffff800012b6b500 x28: ffff00000a52fe30 x27: 0000000000000000
[   50.217519] x26: ffff000004fb0800 x25: 0000000000000038 x24: ffff8000091ab770
[   50.217528] x23: 0000000000000038 x22: ffff800009150c80 x21: ffff000009f4b5e8
[   50.217536] x20: ffff00000a538d80 x19: ffff000009f4b080 x18: fffffffffffeae28
[   50.217544] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000048
[   50.217552] x14: 0000000000000001 x13: 78696d7073696420 x12: fffffffffffcae27
[   50.217560] x11: ffff800011d32ed0 x10: 000000000000000a x9 : ffff800008fc3000
[   50.217569] x8 : ffff000003c71b80 x7 : 0000000000000000 x6 : 000000001abc391d
[   50.217577] x5 : 0000000000000130 x4 : 0000000000000000 x3 : 0000000000000000
[   50.217585] x2 : 0000000000000000 x1 : ffff00000874b880 x0 : 0000000000000000
[   50.217593] Call trace:
[   50.217596]  mxsfb_crtc_atomic_enable+0x60/0x5e0 [mxsfb]
[   50.217604]  drm_atomic_helper_commit_modeset_enables+0x208/0x250
[drm_kms_helper]
[   50.217725]  drm_atomic_helper_commit_tail_rpm+0x50/0xa0 [drm_kms_helper]
[   50.217797]  commit_tail+0xa4/0x184 [drm_kms_helper]
[   50.217869]  drm_atomic_helper_commit+0x160/0x370 [drm_kms_helper]
[   50.217940]  drm_atomic_commit+0x50/0x60 [drm]
[   50.218151]  drm_client_modeset_commit_atomic+0x1c8/0x260 [drm]
[   50.218284]  drm_client_modeset_commit_locked+0x60/0x19c [drm]
[   50.218413]  drm_client_modeset_commit+0x34/0x60 [drm]
[   50.218543]  drm_fb_helper_set_par+0xcc/0x124 [drm_kms_helper]
[   50.218649]  fbcon_init+0x394/0x4e0
[   50.218659]  visual_init+0xb4/0x104
[   50.218668]  do_bind_con_driver.isra.0+0x1c4/0x394
[   50.218676]  do_take_over_console+0x144/0x1fc
[   50.218683]  do_fbcon_takeover+0x6c/0xe0
[   50.218688]  fbcon_fb_registered+0x104/0x11c
[   50.218693]  register_framebuffer+0x1f4/0x350
[   50.218701]  __drm_fb_helper_initial_config_and_unlock+0x338/0x534
[drm_kms_helper]
[   50.218775]  drm_fbdev_client_hotplug+0x148/0x230 [drm_kms_helper]
[   50.218846]  drm_fbdev_generic_setup+0xb4/0x190 [drm_kms_helper]
[   50.218919]  mxsfb_probe+0x324/0x42c [mxsfb]
[   50.218929]  platform_probe+0x6c/0xe0
[   50.218938]  really_probe.part.0+0x9c/0x310
[   50.218946]  __driver_probe_device+0x98/0x144
[   50.218953]  driver_probe_device+0xc8/0x15c
[   50.218959]  __device_attach_driver+0xb8/0x120
[   50.218964]  bus_for_each_drv+0x78/0xd0
[   50.218971]  __device_attach+0xd8/0x180
[   50.218977]  device_initial_probe+0x18/0x24
[   50.218982]  bus_probe_device+0xa0/0xac
[   50.218988]  deferred_probe_work_func+0x88/0xc0
[   50.218995]  process_one_work+0x1d0/0x354
[   50.219001]  worker_thread+0x13c/0x470
[   50.219006]  kthread+0x154/0x160
[   50.219012]  ret_from_fork+0x10/0x20
[   50.219024] Code: f944ee61 b40000a1 aa1403e0 97f73bf5 (b9401017)
[   50.219030] ---[ end trace 8e3f6b78d85cbf0f ]---


Looking at the power domain, it appears that the LCDIf is enabled and
the power domain for it is active, but the dispblk-mipi-dsi power
domain is not:

root@beacon-imx8mm-kit:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          children
            performance
    /device                                             runtime status
----------------------------------------------------------------------------------------------
vpublk-h1                       off-0
            0
vpublk-g2                       off-0
            0
vpublk-g1                       off-0
            0
vpumix                          off-0
            0
    /devices/genpd:0:38330000.blk-ctrl                  suspended
            0
gpu                             off-0
            0
    /devices/platform/soc@0/38000000.gpu                suspended
            0
    /devices/platform/soc@0/38008000.gpu                suspended
            0
dispblk-mipi-csi                off-0
            0
dispblk-mipi-dsi                off-0
            0
    /devices/platform/soc@0/32c00000.bus/32e10000.mipi_dsi  suspended
                0
dispblk-lcdif                   on
            0
    /devices/platform/soc@0/32c00000.bus/32e00000.lcdif  active
             0
dispblk-csi-bridge              off-0
            0
mipi                            off-0
            0
    /devices/genpd:3:32e28000.blk-ctrl                  suspended
            0
    /devices/genpd:4:32e28000.blk-ctrl                  suspended
            0
dispmix                         on
            0
    /devices/genpd:0:32e28000.blk-ctrl                  active
            0
    /devices/genpd:1:32e28000.blk-ctrl                  suspended
            0
    /devices/genpd:2:32e28000.blk-ctrl                  active
            0
vpu-h1                          off-0
            0
    /devices/genpd:3:38330000.blk-ctrl                  suspended
            0
vpu-g2                          off-0
            0
    /devices/genpd:2:38330000.blk-ctrl                  suspended
            0
vpu-g1                          off-0
            0
    /devices/genpd:1:38330000.blk-ctrl                  suspended
            0
gpumix                          off-0
            0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.5
 suspended                  0
usb-otg2                        off-0
            0
usb-otg1                        off-0
            0
pcie                            off-0
            0
hsiomix                         off-0
            0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.1
 suspended                  0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.2
 suspended                  0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.3
 suspended                  0

I'll be traveling for the rest of the week, but I'll try to help when
I come back.

adam

> >
> > Best Regards,
> >
> > Tim
> > [1] https://patchwork.kernel.org/project/dri-devel/list/?series=347439&archive=both&state=*
> > [2] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=359775&archive=both&state=*
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 68 +++++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index 208a0ed840f4..195dcbff7058 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -188,6 +188,12 @@
> >                 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";
> > @@ -1068,6 +1074,68 @@
> >                         #size-cells = <1>;
> >                         ranges = <0x32c00000 0x32c00000 0x400000>;
> >
> > +                       lcdif: lcdif@32e00000 {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
>
> If there is only 1 port I don't think you need address-cells and
> size-cells.  See more below....
>
> > +                               compatible = "fsl,imx8mm-lcdif", "fsl,imx6sx-lcdif";
> > +                               reg = <0x32e00000 0x10000>;
> > +                               clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> > +                                        <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
> > +                                        <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> > +                               clock-names = "pix", "disp_axi", "axi";
> > +                               assigned-clocks = <&clk IMX8MM_CLK_LCDIF_PIXEL>,
> > +                                                 <&clk IMX8MM_CLK_DISP_AXI>,
> > +                                                 <&clk IMX8MM_CLK_DISP_APB>;
> > +                               assigned-clock-parents = <&clk IMX8MM_VIDEO_PLL1_OUT>,
> > +                                                        <&clk IMX8MM_SYS_PLL2_1000M>,
> > +                                                        <&clk IMX8MM_SYS_PLL1_800M>;
> > +                               assigned-clock-rate = <594000000>, <500000000>, <200000000>;
> > +                               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> > +                               power-domains = <&disp_blk_ctrl IMX8MM_DISPBLK_PD_LCDIF>;
> > +                               status = "disabled";
> > +
> > +                               port@0 {
> > +                                       reg = <0>;
>
> This should be just "port {" and we can get rid of the @0 and the "reg=0"
>
> This eliminates some build warnings.
> > +
> > +                                       lcdif_to_dsim: endpoint {
> > +                                               remote-endpoint = <&dsim_from_lcdif>;
> > +                                       };
> > +                               };
> > +                       };
> > +
> > +                       mipi_dsi: mipi_dsi@32e10000 {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
>
> I think this should move into a sub node...see below
>
> > +                               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";
>
> I think the mipi_phy is the equivalent to the mipi_rst_mask in the
> power domain I added to the CSI.  Looking through the NXP 5.10 kernel,
>  BIT(17) is the reset for the MIPI_DSI.  I haven't tried it yet, but
> it makes me assume that BIT(16) is for the MIPI_CSI.
>
> Both bits are set in the imx-atf code, and the documentation isn't
> clear, but I tweaked the blk-ctrl driver to only set BIT(16) for the
> mipi_rst_mask on the CSI.  I still get some hanging, so it's not any
> better that way.
>
> > +                               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";
> > +
>
> Add a subnode called "ports" and move address-cells and size-cells
> into it along wtih "port@0"
>
> > +                               port@0 {
> > +                                       reg = <0>;
> > +
> > +                                       dsim_from_lcdif: endpoint {
> > +                                               remote-endpoint = <&lcdif_to_dsim>;
> > +                                       };
> > +                               };
> > +                       };
> > +
> >                         csi: csi@32e20000 {
> >                                 compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
> >                                 reg = <0x32e20000 0x1000>;
> > --
> > 2.17.1
> >