Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230423121232.1345909-1-aford173@gmail.com> <CGME20230423121305eucas1p287a952d41b1884b117fa15a748b9e1a2@eucas1p2.samsung.com>
 <20230423121232.1345909-6-aford173@gmail.com> <b6b53da5-6986-a958-ef84-650b3a57ad9c@samsung.com>
In-Reply-To: <b6b53da5-6986-a958-ef84-650b3a57ad9c@samsung.com>
From:   Adam Ford <aford173@gmail.com>
Date:   Mon, 24 Apr 2023 05:00:09 -0500
Message-ID: <CAHCN7x+vd-bP8NgS-cRrnm8ojq0kwUg6aXokJv6xSU7BrT04Vw@mail.gmail.com>
Subject: Re: [PATCH V2 5/6] drm: bridge: samsung-dsim: Support non-burst mode
To:     Marek Szyprowski <m.szyprowski@samsung.com>
Cc:     dri-devel@lists.freedesktop.org, aford@beaconembedded.com,
        dario.binacchi@amarulasolutions.com, l.stach@pengutronix.de,
        Inki Dae <inki.dae@samsung.com>,
        Jagan Teki <jagan@amarulasolutions.com>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, Marek Vasut <marex@denx.de>,
        Frieder Schrempf <frieder.schrempf@kontron.de>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Apr 24, 2023 at 3:25=E2=80=AFAM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 23.04.2023 14:12, Adam Ford wrote:
> > The high-speed clock is hard-coded to the burst-clock
> > frequency specified in the device tree.  However, when
> > using devices like certain bridge chips without burst mode
> > and varying resolutions and refresh rates, it may be
> > necessary to set the high-speed clock dynamically based
> > on the desired pixel clock for the connected device.
> >
> > This also removes the need to set a clock speed from
> > the device tree for non-burst mode operation, since the
> > pixel clock rate is the rate requested from the attached
> > device like an HDMI bridge chip.  This should have no
> > impact for people using burst-mode and setting the burst
> > clock rate is still required for those users.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
>
> This one breaks Exynos-5433 based TM2e board with a DSI panel.

Marek S,

Thank you for testing!  I knoiw there are several of us who appreciate
your testing this since it's hard to know if something broke without
hardware.  Is there any way you can tell me if the flag is set to
enable MIPI_DSI_MODE_VIDEO_BURST?
I was trying to be diligent about not breaking your boards, but
without your boards, it's difficult.  The theory was that if
MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the
device tree, it would use the burst clock.

As a fall-back I could just simply check for the presence of the
burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and
burst_clock_rate.


>
> > ---
> >   drivers/gpu/drm/bridge/samsung-dsim.c | 17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/br=
idge/samsung-dsim.c
> > index f165483d5044..cea847b8e23c 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -657,11 +657,21 @@ static unsigned long samsung_dsim_set_pll(struct =
samsung_dsim *dsi,
> >
> >   static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
> >   {
> > -     unsigned long hs_clk, byte_clk, esc_clk;
> > +     unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
> >       unsigned long esc_div;
> >       u32 reg;
> > +     struct drm_display_mode *m =3D &dsi->mode;
> > +     int bpp =3D mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +
> > +     /* m->clock is in KHz */
> > +     pix_clk =3D m->clock * 1000;
> > +
> > +     /* Use burst_clk_rate for burst mode, otherwise use the pix_clk *=
/
> > +     if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->burst_c=
lk_rate)

Would you be willing to test this if this line just read:

              if (dsi->burst_clk_rate)

That would tell me if my fallback idea works.

Thank you,

adam

> > +             hs_clk =3D samsung_dsim_set_pll(dsi, dsi->burst_clk_rate)=
;
> > +     else
> > +             hs_clk =3D samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk=
 * bpp, dsi->lanes));
> >
> > -     hs_clk =3D samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
> >       if (!hs_clk) {
> >               dev_err(dsi->dev, "failed to configure DSI PLL\n");
> >               return -EFAULT;
> > @@ -1800,10 +1810,11 @@ static int samsung_dsim_parse_dt(struct samsung=
_dsim *dsi)
> >                       return PTR_ERR(pll_clk);
> >       }
> >
> > +     /* If it doesn't exist, use pixel clock instead of failing */
> >       ret =3D samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequ=
ency",
> >                                      &dsi->burst_clk_rate);
> >       if (ret < 0)
> > -             return ret;
> > +             dsi->burst_clk_rate =3D 0;
> >
> >       ret =3D samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequen=
cy",
> >                                      &dsi->esc_clk_rate);
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>