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

From: Adam Ford
Date: Tue Jun 06 2023 - 08:58:00 EST

On Tue, Jun 6, 2023 at 2:13 AM Frieder Schrempf
<frieder.schrempf@xxxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx/

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

Attachment: image.png
Description: PNG image