[V2,1/2] arm64: dts: imx8mn-beacon: Add HDMI video with sound

  The Beacon Embedded imx8mn development kit has a DSI
to HDMI bridge chip.  The bridge supports stereo audio
and hot-plug detection.

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

Hi Adam,

On 06.06.23 00:33, Adam Ford wrote:
> The Beacon Embedded imx8mn development kit has a DSI
> to HDMI bridge chip.  The bridge supports stereo audio
> and hot-plug detection.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> index 1392ce02587b..2108ec8c019c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts

I have to minor comments below, otherwise this looks good to me.

As I'm trying to come up with similar changes for our boards I also have
some questions below. Maybe you could share your knowledge on these.

Thanks!
Frieder

> @@ -16,4 +16,138 @@ / {
>  	chosen {
>  		stdout-path = &uart2;
>  	};
> +
> +	connector {
> +		compatible = "hdmi-connector";
> +		type = "a";
> +
> +		port {
> +			hdmi_connector_in: endpoint {
> +				remote-endpoint = <&adv7535_out>;
> +			};
> +		};
> +	};
> +
> +	reg_hdmi: regulator-hdmi-dvdd {
> +		compatible = "regulator-fixed";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_reg_hdmi>;
> +		regulator-name = "hdmi_pwr_en";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio2 11 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		startup-delay-us = <70000>;
> +		regulator-always-on;
> +	};
> +
> +	sound-hdmi {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,name = "sound-hdmi";
> +		simple-audio-card,format = "i2s";
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&sai5 0>;
> +			system-clock-direction-out;
> +		};
> +
> +		simple-audio-card,codec {
> +			sound-dai = <&adv_bridge>;
> +		};
> +	};
> +};
> +
> +&i2c2 {
> +	adv_bridge: hdmi@3d {
> +		compatible = "adi,adv7535";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_hdmi_bridge>;
> +		reg = <0x3d>, <0x3b>;
> +		reg-names = "main", "cec";
> +		adi,dsi-lanes = <4>;

On our boards we have this working with 4 lanes. But we also have some
boards that only have 2 DSI lanes connected. We don't get any image in
on the display in this case. Did you ever try 2 lanes or do you have an
idea what could be wrong?

> +		adi,fixed-lanes;

I think this property comes from downstream and should be removed. I
don't see it anywhere in the upstream driver or bindings.

> +		dvdd-supply = <&reg_hdmi>;
> +		v3p3-supply = <&reg_hdmi>;
> +		v1p2-supply = <&reg_hdmi>;
> +		a2vdd-supply = <&reg_hdmi>;
> +		avdd-supply = <&reg_hdmi>;
> +		pvdd-supply = <&reg_hdmi>;

Please sort the reg properties above alphabetically.

> +		interrupt-parent = <&gpio1>;
> +		interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> +		#sound-dai-cells = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +
> +				adv7535_in: endpoint {
> +					remote-endpoint = <&dsi_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +
> +				adv7535_out: endpoint {
> +					remote-endpoint = <&hdmi_connector_in>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +&lcdif {
> +	assigned-clocks = <&clk IMX8MN_VIDEO_PLL1>;
> +	assigned-clock-rates = <594000000>;

Just out of interest: Why do you need to set the video PLL clock here
and how did you determine the "correct" value? Why is this missing in
the i.MX8MM dts?

> +	status = "okay";
> +};
> +
> +&mipi_dsi {
> +	samsung,esc-clock-frequency = <20000000>;

Same here, I'm interested in how you determined the "correct" value for
this property. Are there any rules to follow?

> +	status = "okay";
> +
> +	ports {
> +		port@1 {
> +			reg = <1>;
> +
> +			dsi_out: endpoint {
> +				remote-endpoint = <&adv7535_in>;
> +			};
> +		};
> +	};
> +};
> +
> +&sai5 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_sai5>;
> +	assigned-clocks = <&clk IMX8MN_CLK_SAI5>;
> +	assigned-clock-parents = <&clk IMX8MN_AUDIO_PLL1_OUT>;
> +	assigned-clock-rates = <24576000>;
> +	#sound-dai-cells = <0>;
> +	status = "okay";
> +};
> +
> +&iomuxc {
> +	pinctrl_hdmi_bridge: hdmibridgegrp {
> +		fsl,pins = <
> +			MX8MN_IOMUXC_GPIO1_IO09_GPIO1_IO9		0x19
> +		>;
> +	};
> +
> +	pinctrl_reg_hdmi: reghdmigrp {
> +		fsl,pins = <
> +			MX8MN_IOMUXC_SD1_STROBE_GPIO2_IO11              0x16
> +		>;
> +	};
> +
> +	pinctrl_sai5: sai5grp {
> +		fsl,pins = <
> +			MX8MN_IOMUXC_SAI5_RXD3_SAI5_TX_DATA0	0xd6
> +			MX8MN_IOMUXC_SAI5_RXD2_SAI5_TX_BCLK	0xd6
> +			MX8MN_IOMUXC_SAI5_RXD1_SAI5_TX_SYNC	0xd6
> +		>;
> +	};
>  };
  

On Tue, Jun 6, 2023 at 2:13 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Adam,
>
> On 06.06.23 00:33, Adam Ford wrote:
> > The Beacon Embedded imx8mn development kit has a DSI
> > to HDMI bridge chip.  The bridge supports stereo audio
> > and hot-plug detection.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> > index 1392ce02587b..2108ec8c019c 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
>
> I have to minor comments below, otherwise this looks good to me.
>
> As I'm trying to come up with similar changes for our boards I also have
> some questions below. Maybe you could share your knowledge on these.
>
> Thanks!
> Frieder
>
> > @@ -16,4 +16,138 @@ / {
> >       chosen {
> >               stdout-path = &uart2;
> >       };
> > +
> > +     connector {
> > +             compatible = "hdmi-connector";
> > +             type = "a";
> > +
> > +             port {
> > +                     hdmi_connector_in: endpoint {
> > +                             remote-endpoint = <&adv7535_out>;
> > +                     };
> > +             };
> > +     };
> > +
> > +     reg_hdmi: regulator-hdmi-dvdd {
> > +             compatible = "regulator-fixed";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&pinctrl_reg_hdmi>;
> > +             regulator-name = "hdmi_pwr_en";
> > +             regulator-min-microvolt = <3300000>;
> > +             regulator-max-microvolt = <3300000>;
> > +             gpio = <&gpio2 11 GPIO_ACTIVE_HIGH>;
> > +             enable-active-high;
> > +             startup-delay-us = <70000>;
> > +             regulator-always-on;
> > +     };
> > +
> > +     sound-hdmi {
> > +             compatible = "simple-audio-card";
> > +             simple-audio-card,name = "sound-hdmi";
> > +             simple-audio-card,format = "i2s";
> > +
> > +             simple-audio-card,cpu {
> > +                     sound-dai = <&sai5 0>;
> > +                     system-clock-direction-out;
> > +             };
> > +
> > +             simple-audio-card,codec {
> > +                     sound-dai = <&adv_bridge>;
> > +             };
> > +     };
> > +};
> > +
> > +&i2c2 {
> > +     adv_bridge: hdmi@3d {
> > +             compatible = "adi,adv7535";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&pinctrl_hdmi_bridge>;
> > +             reg = <0x3d>, <0x3b>;
> > +             reg-names = "main", "cec";
> > +             adi,dsi-lanes = <4>;
>
> On our boards we have this working with 4 lanes. But we also have some
> boards that only have 2 DSI lanes connected. We don't get any image in
> on the display in this case. Did you ever try 2 lanes or do you have an
> idea what could be wrong?

I didn't try 2-lane, but see below regarding clocks to see if any of
that makes any sense.

>
> > +             adi,fixed-lanes;
>
> I think this property comes from downstream and should be removed. I
> don't see it anywhere in the upstream driver or bindings.

You're right.  This came from some early debugging I was doing for a
patch I was going to propose when someone beat me to it.  I'll submit
a V3 with this removed.
>
> > +             dvdd-supply = <&reg_hdmi>;
> > +             v3p3-supply = <&reg_hdmi>;
> > +             v1p2-supply = <&reg_hdmi>;
> > +             a2vdd-supply = <&reg_hdmi>;
> > +             avdd-supply = <&reg_hdmi>;
> > +             pvdd-supply = <&reg_hdmi>;
>
> Please sort the reg properties above alphabetically.

OK.

>
> > +             interrupt-parent = <&gpio1>;
> > +             interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> > +             #sound-dai-cells = <0>;
> > +
> > +             ports {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +
> > +                     port@0 {
> > +                             reg = <0>;
> > +
> > +                             adv7535_in: endpoint {
> > +                                     remote-endpoint = <&dsi_out>;
> > +                             };
> > +                     };
> > +
> > +                     port@1 {
> > +                             reg = <1>;
> > +
> > +                             adv7535_out: endpoint {
> > +                                     remote-endpoint = <&hdmi_connector_in>;
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +};
> > +
> > +&lcdif {
> > +     assigned-clocks = <&clk IMX8MN_VIDEO_PLL1>;
> > +     assigned-clock-rates = <594000000>;
>
> Just out of interest: Why do you need to set the video PLL clock here
> and how did you determine the "correct" value? Why is this missing in
> the i.MX8MM dts?

I am glad you asked this question.  In an ideal world we would not
need to set this, because the display clock would propagate up to the
video_pll, but that's currently not happening yet, so one needs to
find a video-pll clock rate that nicely divides into the maximum
number of resoltions as possible.

When you run modetest on a display, you'll get a list of resolutions
with their refresh rate and horizontal and vertical timings as well as
the pixel clock.  The mxsfb needs its clock to match the pixel clock
in order for the displays to sync properly.  With 594000000 set for
the video-pll, the disp-clock divdes down to achieve what it can to
hit the required pixel clock.

Here is an example with some of the extra timing info removed.  The
final column is the 594MHz / pix-clk
name           refresh    pix clk       Divide by 594000000
1920x1080 60          148500        4000
1920x1080 59.94    148352         4003.990509
1920x1080 50         148500         4000
1920x1080 30          74250          8000
1920x1080 29.97     74176          8007.981018
1920x1080 24          74250          8000
1920x1080 23.98     74176          8007.981018
1600x900 60            108000        5500
1280x1024 60.02     108000        5500
1280x800 59.91       71000          8366.197183
1152x864 59.97       81579           7281.285625
1280x720 60            74250          8000
1280x720 59.94       74176          8007.981018
1024x768 60            65000          9138.461538
800x600 60.32         40000          14850
720x576 50              27000          22000
720x480 60              27027          21978.02198
720x480 59.94         27000          22000
640x480 60              25200          23571.42857
640x480 59.94         25175          23594.83615


As you can see, there are a number of resolutions which divide evenly,
but there are a bunch that do not.  If you wanted to achieve
resolutions that do not divide evenly by 594MHz, you'd need to change
the video-pll clock to something else in order for this clock to
evenly divide but then it would break other resolutions..  I have
tested this, and I can get some of these additional resolutions to
work with a different video-pll clock rate.  NXP's downstream kernel
masks this by blocking out clock rates that didn't divide evenly.

The Mini has a different clock parent-child relationship, and the
clock appears to default to 594MHz already, but the Nano has a default
of 650MHz which is why I had to add the clock there.  I didn't want to
put it into the imx8mn.dtsi file, because this may not be appropriate
for some people or people with a fixed display running at specific
resltion and/or pixel rate.

I proposed a solution which would fix both Mini and Nano by allowing
the requested display clock to propagate back up to the video-pll.

On the Mini and Nano, a patch I proposed can help achieve more accurate
lcdif clocks.   For example, when trying to get a pixel clock of 31.500MHz
on an imx8m Nano, the clocks divided the 594MHz down, but
left the parent rate untouched which caused a calculation error.

Before:
video_pll              594000000
  video_pll_bypass     594000000
    video_pll_out      594000000
      disp_pixel       31263158
        disp_pixel_clk 31263158

Variance = -236842 Hz

After this patch:
video_pll               31500000
  video_pll_bypass      31500000
    video_pll_out       31500000
      disp_pixel        31500000
        disp_pixel_clk  31500000

Variance = 0 Hz

If you want to check out the patch I proposed, look at [1].  I'd love
to get some tracking and/or feedback for this.  This would allow the
Mini and Nano to sync more resolutions and allow people to not have to
specify the video-pll rate.
imx8mp is little more complicated, because there are two different
pixel clocks generated from the same video-pll (the DSI and the
LDB/LVDS)

>
> > +     status = "okay";
> > +};
> > +
> > +&mipi_dsi {
> > +     samsung,esc-clock-frequency = <20000000>;
>
> Same here, I'm interested in how you determined the "correct" value for
> this property. Are there any rules to follow?

20MHz was the max clock rate supported by the DSI controller.  It also
appears to be the clock speed set in the NXP downstream kernel, so I
used it.  I wish I had a better explanation.

adam

[1] - https://lore.kernel.org/linux-arm-kernel/20230506195325.876871-1-aford173@gmail.com/

>
> > +     status = "okay";
> > +
> > +     ports {
> > +             port@1 {
> > +                     reg = <1>;
> > +
> > +                     dsi_out: endpoint {
> > +                             remote-endpoint = <&adv7535_in>;
> > +                     };
> > +             };
> > +     };
> > +};
> > +
> > +&sai5 {
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&pinctrl_sai5>;
> > +     assigned-clocks = <&clk IMX8MN_CLK_SAI5>;
> > +     assigned-clock-parents = <&clk IMX8MN_AUDIO_PLL1_OUT>;
> > +     assigned-clock-rates = <24576000>;
> > +     #sound-dai-cells = <0>;
> > +     status = "okay";
> > +};
> > +
> > +&iomuxc {
> > +     pinctrl_hdmi_bridge: hdmibridgegrp {
> > +             fsl,pins = <
> > +                     MX8MN_IOMUXC_GPIO1_IO09_GPIO1_IO9               0x19
> > +             >;
> > +     };
> > +
> > +     pinctrl_reg_hdmi: reghdmigrp {
> > +             fsl,pins = <
> > +                     MX8MN_IOMUXC_SD1_STROBE_GPIO2_IO11              0x16
> > +             >;
> > +     };
> > +
> > +     pinctrl_sai5: sai5grp {
> > +             fsl,pins = <
> > +                     MX8MN_IOMUXC_SAI5_RXD3_SAI5_TX_DATA0    0xd6
> > +                     MX8MN_IOMUXC_SAI5_RXD2_SAI5_TX_BCLK     0xd6
> > +                     MX8MN_IOMUXC_SAI5_RXD1_SAI5_TX_SYNC     0xd6
> > +             >;
> > +     };
> >  };
  

On Tue, Jun 6, 2023 at 7:57 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Tue, Jun 6, 2023 at 2:13 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
> >
> > Hi Adam,
> >
> > On 06.06.23 00:33, Adam Ford wrote:
> > > The Beacon Embedded imx8mn development kit has a DSI
> > > to HDMI bridge chip.  The bridge supports stereo audio
> > > and hot-plug detection.
> > >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> > > index 1392ce02587b..2108ec8c019c 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> >
> > I have to minor comments below, otherwise this looks good to me.
> >
> > As I'm trying to come up with similar changes for our boards I also have
> > some questions below. Maybe you could share your knowledge on these.
> >
> > Thanks!
> > Frieder
> >
> > > @@ -16,4 +16,138 @@ / {
> > >       chosen {
> > >               stdout-path = &uart2;
> > >       };
> > > +
> > > +     connector {
> > > +             compatible = "hdmi-connector";
> > > +             type = "a";
> > > +
> > > +             port {
> > > +                     hdmi_connector_in: endpoint {
> > > +                             remote-endpoint = <&adv7535_out>;
> > > +                     };
> > > +             };
> > > +     };
> > > +
> > > +     reg_hdmi: regulator-hdmi-dvdd {
> > > +             compatible = "regulator-fixed";
> > > +             pinctrl-names = "default";
> > > +             pinctrl-0 = <&pinctrl_reg_hdmi>;
> > > +             regulator-name = "hdmi_pwr_en";
> > > +             regulator-min-microvolt = <3300000>;
> > > +             regulator-max-microvolt = <3300000>;
> > > +             gpio = <&gpio2 11 GPIO_ACTIVE_HIGH>;
> > > +             enable-active-high;
> > > +             startup-delay-us = <70000>;
> > > +             regulator-always-on;
> > > +     };
> > > +
> > > +     sound-hdmi {
> > > +             compatible = "simple-audio-card";
> > > +             simple-audio-card,name = "sound-hdmi";
> > > +             simple-audio-card,format = "i2s";
> > > +
> > > +             simple-audio-card,cpu {
> > > +                     sound-dai = <&sai5 0>;
> > > +                     system-clock-direction-out;
> > > +             };
> > > +
> > > +             simple-audio-card,codec {
> > > +                     sound-dai = <&adv_bridge>;
> > > +             };
> > > +     };
> > > +};
> > > +
> > > +&i2c2 {
> > > +     adv_bridge: hdmi@3d {
> > > +             compatible = "adi,adv7535";
> > > +             pinctrl-names = "default";
> > > +             pinctrl-0 = <&pinctrl_hdmi_bridge>;
> > > +             reg = <0x3d>, <0x3b>;
> > > +             reg-names = "main", "cec";
> > > +             adi,dsi-lanes = <4>;
> >
> > On our boards we have this working with 4 lanes. But we also have some
> > boards that only have 2 DSI lanes connected. We don't get any image in
> > on the display in this case. Did you ever try 2 lanes or do you have an
> > idea what could be wrong?
>
> I didn't try 2-lane, but see below regarding clocks to see if any of
> that makes any sense.

I tried configuring my hardware for 2-lane operation, and it appears
to not function, but I haven't been able to determine why.  Have you
made any progress on getting your 2-lane interface operational?

2-lane clocks probably require a pixel clock 2x the speed of the
4-lane modes.  I'm really tight on time right now, but I can try to
put some time in to investigating the 2-lane operation.

adam
>
> >
> > > +             adi,fixed-lanes;
> >
> > I think this property comes from downstream and should be removed. I
> > don't see it anywhere in the upstream driver or bindings.
>
> You're right.  This came from some early debugging I was doing for a
> patch I was going to propose when someone beat me to it.  I'll submit
> a V3 with this removed.
> >
> > > +             dvdd-supply = <&reg_hdmi>;
> > > +             v3p3-supply = <&reg_hdmi>;
> > > +             v1p2-supply = <&reg_hdmi>;
> > > +             a2vdd-supply = <&reg_hdmi>;
> > > +             avdd-supply = <&reg_hdmi>;
> > > +             pvdd-supply = <&reg_hdmi>;
> >
> > Please sort the reg properties above alphabetically.
>
> OK.
>
> >
> > > +             interrupt-parent = <&gpio1>;
> > > +             interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> > > +             #sound-dai-cells = <0>;
> > > +
> > > +             ports {
> > > +                     #address-cells = <1>;
> > > +                     #size-cells = <0>;
> > > +
> > > +                     port@0 {
> > > +                             reg = <0>;
> > > +
> > > +                             adv7535_in: endpoint {
> > > +                                     remote-endpoint = <&dsi_out>;
> > > +                             };
> > > +                     };
> > > +
> > > +                     port@1 {
> > > +                             reg = <1>;
> > > +
> > > +                             adv7535_out: endpoint {
> > > +                                     remote-endpoint = <&hdmi_connector_in>;
> > > +                             };
> > > +                     };
> > > +             };
> > > +     };
> > > +};
> > > +
> > > +&lcdif {
> > > +     assigned-clocks = <&clk IMX8MN_VIDEO_PLL1>;
> > > +     assigned-clock-rates = <594000000>;
> >
> > Just out of interest: Why do you need to set the video PLL clock here
> > and how did you determine the "correct" value? Why is this missing in
> > the i.MX8MM dts?
>
> I am glad you asked this question.  In an ideal world we would not
> need to set this, because the display clock would propagate up to the
> video_pll, but that's currently not happening yet, so one needs to
> find a video-pll clock rate that nicely divides into the maximum
> number of resoltions as possible.
>
> When you run modetest on a display, you'll get a list of resolutions
> with their refresh rate and horizontal and vertical timings as well as
> the pixel clock.  The mxsfb needs its clock to match the pixel clock
> in order for the displays to sync properly.  With 594000000 set for
> the video-pll, the disp-clock divdes down to achieve what it can to
> hit the required pixel clock.
>
> Here is an example with some of the extra timing info removed.  The
> final column is the 594MHz / pix-clk
> name           refresh    pix clk       Divide by 594000000
> 1920x1080 60          148500        4000
> 1920x1080 59.94    148352         4003.990509
> 1920x1080 50         148500         4000
> 1920x1080 30          74250          8000
> 1920x1080 29.97     74176          8007.981018
> 1920x1080 24          74250          8000
> 1920x1080 23.98     74176          8007.981018
> 1600x900 60            108000        5500
> 1280x1024 60.02     108000        5500
> 1280x800 59.91       71000          8366.197183
> 1152x864 59.97       81579           7281.285625
> 1280x720 60            74250          8000
> 1280x720 59.94       74176          8007.981018
> 1024x768 60            65000          9138.461538
> 800x600 60.32         40000          14850
> 720x576 50              27000          22000
> 720x480 60              27027          21978.02198
> 720x480 59.94         27000          22000
> 640x480 60              25200          23571.42857
> 640x480 59.94         25175          23594.83615
>
>
> As you can see, there are a number of resolutions which divide evenly,
> but there are a bunch that do not.  If you wanted to achieve
> resolutions that do not divide evenly by 594MHz, you'd need to change
> the video-pll clock to something else in order for this clock to
> evenly divide but then it would break other resolutions..  I have
> tested this, and I can get some of these additional resolutions to
> work with a different video-pll clock rate.  NXP's downstream kernel
> masks this by blocking out clock rates that didn't divide evenly.
>
> The Mini has a different clock parent-child relationship, and the
> clock appears to default to 594MHz already, but the Nano has a default
> of 650MHz which is why I had to add the clock there.  I didn't want to
> put it into the imx8mn.dtsi file, because this may not be appropriate
> for some people or people with a fixed display running at specific
> resltion and/or pixel rate.
>
> I proposed a solution which would fix both Mini and Nano by allowing
> the requested display clock to propagate back up to the video-pll.
>
> On the Mini and Nano, a patch I proposed can help achieve more accurate
> lcdif clocks.   For example, when trying to get a pixel clock of 31.500MHz
> on an imx8m Nano, the clocks divided the 594MHz down, but
> left the parent rate untouched which caused a calculation error.
>
> Before:
> video_pll              594000000
>   video_pll_bypass     594000000
>     video_pll_out      594000000
>       disp_pixel       31263158
>         disp_pixel_clk 31263158
>
> Variance = -236842 Hz
>
> After this patch:
> video_pll               31500000
>   video_pll_bypass      31500000
>     video_pll_out       31500000
>       disp_pixel        31500000
>         disp_pixel_clk  31500000
>
> Variance = 0 Hz
>
> If you want to check out the patch I proposed, look at [1].  I'd love
> to get some tracking and/or feedback for this.  This would allow the
> Mini and Nano to sync more resolutions and allow people to not have to
> specify the video-pll rate.
> imx8mp is little more complicated, because there are two different
> pixel clocks generated from the same video-pll (the DSI and the
> LDB/LVDS)
>
> >
> > > +     status = "okay";
> > > +};
> > > +
> > > +&mipi_dsi {
> > > +     samsung,esc-clock-frequency = <20000000>;
> >
> > Same here, I'm interested in how you determined the "correct" value for
> > this property. Are there any rules to follow?
>
> 20MHz was the max clock rate supported by the DSI controller.  It also
> appears to be the clock speed set in the NXP downstream kernel, so I
> used it.  I wish I had a better explanation.
>
> adam
>
> [1] - https://lore.kernel.org/linux-arm-kernel/20230506195325.876871-1-aford173@gmail.com/
>
> >
> > > +     status = "okay";
> > > +
> > > +     ports {
> > > +             port@1 {
> > > +                     reg = <1>;
> > > +
> > > +                     dsi_out: endpoint {
> > > +                             remote-endpoint = <&adv7535_in>;
> > > +                     };
> > > +             };
> > > +     };
> > > +};
> > > +
> > > +&sai5 {
> > > +     pinctrl-names = "default";
> > > +     pinctrl-0 = <&pinctrl_sai5>;
> > > +     assigned-clocks = <&clk IMX8MN_CLK_SAI5>;
> > > +     assigned-clock-parents = <&clk IMX8MN_AUDIO_PLL1_OUT>;
> > > +     assigned-clock-rates = <24576000>;
> > > +     #sound-dai-cells = <0>;
> > > +     status = "okay";
> > > +};
> > > +
> > > +&iomuxc {
> > > +     pinctrl_hdmi_bridge: hdmibridgegrp {
> > > +             fsl,pins = <
> > > +                     MX8MN_IOMUXC_GPIO1_IO09_GPIO1_IO9               0x19
> > > +             >;
> > > +     };
> > > +
> > > +     pinctrl_reg_hdmi: reghdmigrp {
> > > +             fsl,pins = <
> > > +                     MX8MN_IOMUXC_SD1_STROBE_GPIO2_IO11              0x16
> > > +             >;
> > > +     };
> > > +
> > > +     pinctrl_sai5: sai5grp {
> > > +             fsl,pins = <
> > > +                     MX8MN_IOMUXC_SAI5_RXD3_SAI5_TX_DATA0    0xd6
> > > +                     MX8MN_IOMUXC_SAI5_RXD2_SAI5_TX_BCLK     0xd6
> > > +                     MX8MN_IOMUXC_SAI5_RXD1_SAI5_TX_SYNC     0xd6
> > > +             >;
> > > +     };
> > >  };
  

On Sat, Jun 10, 2023 at 1:11 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Tue, Jun 6, 2023 at 7:57 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Tue, Jun 6, 2023 at 2:13 AM Frieder Schrempf
> > <frieder.schrempf@kontron.de> wrote:
> > >
> > > Hi Adam,
> > >
> > > On 06.06.23 00:33, Adam Ford wrote:
> > > > The Beacon Embedded imx8mn development kit has a DSI
> > > > to HDMI bridge chip.  The bridge supports stereo audio
> > > > and hot-plug detection.
> > > >
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> > > > index 1392ce02587b..2108ec8c019c 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-beacon-kit.dts
> > >
> > > I have to minor comments below, otherwise this looks good to me.
> > >
> > > As I'm trying to come up with similar changes for our boards I also have
> > > some questions below. Maybe you could share your knowledge on these.
> > >
> > > Thanks!
> > > Frieder
> > >
> > > > @@ -16,4 +16,138 @@ / {
> > > >       chosen {
> > > >               stdout-path = &uart2;
> > > >       };
> > > > +
> > > > +     connector {
> > > > +             compatible = "hdmi-connector";
> > > > +             type = "a";
> > > > +
> > > > +             port {
> > > > +                     hdmi_connector_in: endpoint {
> > > > +                             remote-endpoint = <&adv7535_out>;
> > > > +                     };
> > > > +             };
> > > > +     };
> > > > +
> > > > +     reg_hdmi: regulator-hdmi-dvdd {
> > > > +             compatible = "regulator-fixed";
> > > > +             pinctrl-names = "default";
> > > > +             pinctrl-0 = <&pinctrl_reg_hdmi>;
> > > > +             regulator-name = "hdmi_pwr_en";
> > > > +             regulator-min-microvolt = <3300000>;
> > > > +             regulator-max-microvolt = <3300000>;
> > > > +             gpio = <&gpio2 11 GPIO_ACTIVE_HIGH>;
> > > > +             enable-active-high;
> > > > +             startup-delay-us = <70000>;
> > > > +             regulator-always-on;
> > > > +     };
> > > > +
> > > > +     sound-hdmi {
> > > > +             compatible = "simple-audio-card";
> > > > +             simple-audio-card,name = "sound-hdmi";
> > > > +             simple-audio-card,format = "i2s";
> > > > +
> > > > +             simple-audio-card,cpu {
> > > > +                     sound-dai = <&sai5 0>;
> > > > +                     system-clock-direction-out;
> > > > +             };
> > > > +
> > > > +             simple-audio-card,codec {
> > > > +                     sound-dai = <&adv_bridge>;
> > > > +             };
> > > > +     };
> > > > +};
> > > > +
> > > > +&i2c2 {
> > > > +     adv_bridge: hdmi@3d {
> > > > +             compatible = "adi,adv7535";
> > > > +             pinctrl-names = "default";
> > > > +             pinctrl-0 = <&pinctrl_hdmi_bridge>;
> > > > +             reg = <0x3d>, <0x3b>;
> > > > +             reg-names = "main", "cec";
> > > > +             adi,dsi-lanes = <4>;
> > >
> > > On our boards we have this working with 4 lanes. But we also have some
> > > boards that only have 2 DSI lanes connected. We don't get any image in
> > > on the display in this case. Did you ever try 2 lanes or do you have an
> > > idea what could be wrong?
> >

+ Lucas,

> > I didn't try 2-lane, but see below regarding clocks to see if any of
> > that makes any sense.
>
Frieder,

> I tried configuring my hardware for 2-lane operation, and it appears
> to not function, but I haven't been able to determine why.  Have you
> made any progress on getting your 2-lane interface operational?

I looked at the NXP down-stream code to compare the HBP, HFP, and HSA
values [1] to what we're generating with this mainline code, but the
formula to achieve these values doesn't make sense to me.  The
equations for the 4-lane calculation were pretty straight-forward.

I tried to do a look-up table similar to the method that NXP did, but
it was rejected in favor of a formula to calculate values.  You  might
try to see if using hard-coded values for HFP, HBP and HSA work for
your display which would at least narrow down where the problem might
be.  NXP's downstream code also has a work-around for calculating the
PHY [2] which only appears to affect 720P@60 for some reason.

If someone has an idea of how to calculate the 2-lane timings for HFP,
HBP, and HSA, that would be useful.

720p@60
HFP     HBP     HSA
159      324       54 (calculated with current formula)
159      320       40 (downstream look-up table)

adam

[1] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L369
[2] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L1018
>
> 2-lane clocks probably require a pixel clock 2x the speed of the
> 4-lane modes.  I'm really tight on time right now, but I can try to
> put some time in to investigating the 2-lane operation.
>
> adam
> >
> > >
> > > > +             adi,fixed-lanes;
> > >
> > > I think this property comes from downstream and should be removed. I
> > > don't see it anywhere in the upstream driver or bindings.
> >
> > You're right.  This came from some early debugging I was doing for a
> > patch I was going to propose when someone beat me to it.  I'll submit
> > a V3 with this removed.
> > >
> > > > +             dvdd-supply = <&reg_hdmi>;
> > > > +             v3p3-supply = <&reg_hdmi>;
> > > > +             v1p2-supply = <&reg_hdmi>;
> > > > +             a2vdd-supply = <&reg_hdmi>;
> > > > +             avdd-supply = <&reg_hdmi>;
> > > > +             pvdd-supply = <&reg_hdmi>;
> > >
> > > Please sort the reg properties above alphabetically.
> >
> > OK.
> >
> > >
> > > > +             interrupt-parent = <&gpio1>;
> > > > +             interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> > > > +             #sound-dai-cells = <0>;
> > > > +
> > > > +             ports {
> > > > +                     #address-cells = <1>;
> > > > +                     #size-cells = <0>;
> > > > +
> > > > +                     port@0 {
> > > > +                             reg = <0>;
> > > > +
> > > > +                             adv7535_in: endpoint {
> > > > +                                     remote-endpoint = <&dsi_out>;
> > > > +                             };
> > > > +                     };
> > > > +
> > > > +                     port@1 {
> > > > +                             reg = <1>;
> > > > +
> > > > +                             adv7535_out: endpoint {
> > > > +                                     remote-endpoint = <&hdmi_connector_in>;
> > > > +                             };
> > > > +                     };
> > > > +             };
> > > > +     };
> > > > +};
> > > > +
> > > > +&lcdif {
> > > > +     assigned-clocks = <&clk IMX8MN_VIDEO_PLL1>;
> > > > +     assigned-clock-rates = <594000000>;
> > >
> > > Just out of interest: Why do you need to set the video PLL clock here
> > > and how did you determine the "correct" value? Why is this missing in
> > > the i.MX8MM dts?
> >
> > I am glad you asked this question.  In an ideal world we would not
> > need to set this, because the display clock would propagate up to the
> > video_pll, but that's currently not happening yet, so one needs to
> > find a video-pll clock rate that nicely divides into the maximum
> > number of resoltions as possible.
> >
> > When you run modetest on a display, you'll get a list of resolutions
> > with their refresh rate and horizontal and vertical timings as well as
> > the pixel clock.  The mxsfb needs its clock to match the pixel clock
> > in order for the displays to sync properly.  With 594000000 set for
> > the video-pll, the disp-clock divdes down to achieve what it can to
> > hit the required pixel clock.
> >
> > Here is an example with some of the extra timing info removed.  The
> > final column is the 594MHz / pix-clk
> > name           refresh    pix clk       Divide by 594000000
> > 1920x1080 60          148500        4000
> > 1920x1080 59.94    148352         4003.990509
> > 1920x1080 50         148500         4000
> > 1920x1080 30          74250          8000
> > 1920x1080 29.97     74176          8007.981018
> > 1920x1080 24          74250          8000
> > 1920x1080 23.98     74176          8007.981018
> > 1600x900 60            108000        5500
> > 1280x1024 60.02     108000        5500
> > 1280x800 59.91       71000          8366.197183
> > 1152x864 59.97       81579           7281.285625
> > 1280x720 60            74250          8000
> > 1280x720 59.94       74176          8007.981018
> > 1024x768 60            65000          9138.461538
> > 800x600 60.32         40000          14850
> > 720x576 50              27000          22000
> > 720x480 60              27027          21978.02198
> > 720x480 59.94         27000          22000
> > 640x480 60              25200          23571.42857
> > 640x480 59.94         25175          23594.83615
> >
> >
> > As you can see, there are a number of resolutions which divide evenly,
> > but there are a bunch that do not.  If you wanted to achieve
> > resolutions that do not divide evenly by 594MHz, you'd need to change
> > the video-pll clock to something else in order for this clock to
> > evenly divide but then it would break other resolutions..  I have
> > tested this, and I can get some of these additional resolutions to
> > work with a different video-pll clock rate.  NXP's downstream kernel
> > masks this by blocking out clock rates that didn't divide evenly.
> >
> > The Mini has a different clock parent-child relationship, and the
> > clock appears to default to 594MHz already, but the Nano has a default
> > of 650MHz which is why I had to add the clock there.  I didn't want to
> > put it into the imx8mn.dtsi file, because this may not be appropriate
> > for some people or people with a fixed display running at specific
> > resltion and/or pixel rate.
> >
> > I proposed a solution which would fix both Mini and Nano by allowing
> > the requested display clock to propagate back up to the video-pll.
> >
> > On the Mini and Nano, a patch I proposed can help achieve more accurate
> > lcdif clocks.   For example, when trying to get a pixel clock of 31.500MHz
> > on an imx8m Nano, the clocks divided the 594MHz down, but
> > left the parent rate untouched which caused a calculation error.
> >
> > Before:
> > video_pll              594000000
> >   video_pll_bypass     594000000
> >     video_pll_out      594000000
> >       disp_pixel       31263158
> >         disp_pixel_clk 31263158
> >
> > Variance = -236842 Hz
> >
> > After this patch:
> > video_pll               31500000
> >   video_pll_bypass      31500000
> >     video_pll_out       31500000
> >       disp_pixel        31500000
> >         disp_pixel_clk  31500000
> >
> > Variance = 0 Hz
> >
> > If you want to check out the patch I proposed, look at [1].  I'd love
> > to get some tracking and/or feedback for this.  This would allow the
> > Mini and Nano to sync more resolutions and allow people to not have to
> > specify the video-pll rate.
> > imx8mp is little more complicated, because there are two different
> > pixel clocks generated from the same video-pll (the DSI and the
> > LDB/LVDS)
> >
> > >
> > > > +     status = "okay";
> > > > +};
> > > > +
> > > > +&mipi_dsi {
> > > > +     samsung,esc-clock-frequency = <20000000>;
> > >
> > > Same here, I'm interested in how you determined the "correct" value for
> > > this property. Are there any rules to follow?
> >
> > 20MHz was the max clock rate supported by the DSI controller.  It also
> > appears to be the clock speed set in the NXP downstream kernel, so I
> > used it.  I wish I had a better explanation.
> >
> > adam
> >
> > [1] - https://lore.kernel.org/linux-arm-kernel/20230506195325.876871-1-aford173@gmail.com/
> >
> > >
> > > > +     status = "okay";
> > > > +
> > > > +     ports {
> > > > +             port@1 {
> > > > +                     reg = <1>;
> > > > +
> > > > +                     dsi_out: endpoint {
> > > > +                             remote-endpoint = <&adv7535_in>;
> > > > +                     };
> > > > +             };
> > > > +     };
> > > > +};
> > > > +
> > > > +&sai5 {
> > > > +     pinctrl-names = "default";
> > > > +     pinctrl-0 = <&pinctrl_sai5>;
> > > > +     assigned-clocks = <&clk IMX8MN_CLK_SAI5>;
> > > > +     assigned-clock-parents = <&clk IMX8MN_AUDIO_PLL1_OUT>;
> > > > +     assigned-clock-rates = <24576000>;
> > > > +     #sound-dai-cells = <0>;
> > > > +     status = "okay";
> > > > +};
> > > > +
> > > > +&iomuxc {
> > > > +     pinctrl_hdmi_bridge: hdmibridgegrp {
> > > > +             fsl,pins = <
> > > > +                     MX8MN_IOMUXC_GPIO1_IO09_GPIO1_IO9               0x19
> > > > +             >;
> > > > +     };
> > > > +
> > > > +     pinctrl_reg_hdmi: reghdmigrp {
> > > > +             fsl,pins = <
> > > > +                     MX8MN_IOMUXC_SD1_STROBE_GPIO2_IO11              0x16
> > > > +             >;
> > > > +     };
> > > > +
> > > > +     pinctrl_sai5: sai5grp {
> > > > +             fsl,pins = <
> > > > +                     MX8MN_IOMUXC_SAI5_RXD3_SAI5_TX_DATA0    0xd6
> > > > +                     MX8MN_IOMUXC_SAI5_RXD2_SAI5_TX_BCLK     0xd6
> > > > +                     MX8MN_IOMUXC_SAI5_RXD1_SAI5_TX_SYNC     0xd6
> > > > +             >;
> > > > +     };
> > > >  };