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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux