2023-06-05 23:00:44

by Adam Ford

[permalink] [raw]
Subject: [PATCH 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 <[email protected]>

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
@@ -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>;
+ adi,fixed-lanes;
+ dvdd-supply = <&reg_hdmi>;
+ v3p3-supply = <&reg_hdmi>;
+ v1p2-supply = <&reg_hdmi>;
+ a2vdd-supply = <&reg_hdmi>;
+ avdd-supply = <&reg_hdmi>;
+ pvdd-supply = <&reg_hdmi>;
+ 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>;
+ status = "okay";
+};
+
+&mipi_dsi {
+ samsung,esc-clock-frequency = <20000000>;
+ 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
+ >;
+ };
};
--
2.39.2



2023-06-06 07:26:19

by Frieder Schrempf

[permalink] [raw]
Subject: Re: [PATCH V2 1/2] arm64: dts: imx8mn-beacon: Add HDMI video with sound

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 <[email protected]>
>
> 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
> + >;
> + };
> };

2023-06-06 13:34:32

by Adam Ford

[permalink] [raw]
Subject: Re: [PATCH V2 1/2] arm64: dts: imx8mn-beacon: Add HDMI video with sound

On Tue, Jun 6, 2023 at 2:13 AM Frieder Schrempf
<[email protected]> 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 <[email protected]>
> >
> > 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/[email protected]/

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


Attachments:
image.png (13.35 kB)

2023-06-10 19:00:40

by Adam Ford

[permalink] [raw]
Subject: Re: [PATCH V2 1/2] arm64: dts: imx8mn-beacon: Add HDMI video with sound

On Tue, Jun 6, 2023 at 7:57 AM Adam Ford <[email protected]> wrote:
>
> On Tue, Jun 6, 2023 at 2:13 AM Frieder Schrempf
> <[email protected]> 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 <[email protected]>
> > >
> > > 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/[email protected]/
>
> >
> > > + 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
> > > + >;
> > > + };
> > > };

2023-06-12 17:30:22

by Adam Ford

[permalink] [raw]
Subject: Re: [PATCH V2 1/2] arm64: dts: imx8mn-beacon: Add HDMI video with sound

On Sat, Jun 10, 2023 at 1:11 PM Adam Ford <[email protected]> wrote:
>
> On Tue, Jun 6, 2023 at 7:57 AM Adam Ford <[email protected]> wrote:
> >
> > On Tue, Jun 6, 2023 at 2:13 AM Frieder Schrempf
> > <[email protected]> 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 <[email protected]>
> > > >
> > > > 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/[email protected]/
> >
> > >
> > > > + 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
> > > > + >;
> > > > + };
> > > > };


Attachments:
image.png (998.00 B)