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 7:57 AM Adam Ford <aford173@xxxxxxxxx> wrote:
>
> 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.

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




[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