Re: [PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL

[Michael Tretter <m.tretter@pengutronix.de>]

On Mon, 04 Sep 2023 13:38:33 +0900, Inki Dae wrote:
> 2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>님이 작성:
> 
> >
> > The PLL reference clock may change at runtime. Thus, reading the clock
> > rate during probe is not sufficient to correctly configure the PLL for
> > the expected hs clock.
> >
> > Read the actual rate of the reference clock before calculating the PLL
> > configuration parameters.
> >
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 16 +++++++++-------
> >  include/drm/bridge/samsung-dsim.h     |  1 +
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 6778f1751faa..da90c2038042 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -611,7 +611,12 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> >         u16 m;
> >         u32 reg;
> >
> > -       fin = dsi->pll_clk_rate;
> > +       if (dsi->pll_clk)
> > +               fin = clk_get_rate(dsi->pll_clk);
> > +       else
> > +               fin = dsi->pll_clk_rate;
> > +       dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> > +
> 
> Could you share us the actual use case that in runtime the pll
> reference clock can be changed?

On i.MX8M Nano, the VIDEO_PLL_CLK drives the DISPLAY_PIXEL_CLK_ROOT, which is
used as pixel clock by the LCDIF. Changes to the pixel clock propagate to the
VIDEO_PLL_CLK and may reconfigure the VIDEO_PLL_CLK. This is done to reduce
the error on the pixel clock.

As the ADV3575 as MIPI-DSI device reconstructs the pixel clock, it is
necessary to keep the pixel clock and MIDI-DSI reference clock in
sync. This can be done by using the VIDEO_PLL_CLK to drive the PLL reference
clock (MIPI_DSI_CORE_CLK_ROOT). Without this, a connected HDMI Monitor will
occasionally loose sync.

In this setup, a mode change that changes the pixel clock may change the
VIDEO_PLL, which will change the PLL reference clock.

> 
> This patch is trying to change clock binding behavior which is
> described in dt binding[1]
> [1] Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
> 
> It says,
> "DISM oscillator clock frequency. If absent, the clock frequency of
> sclk_mipi will be used instead."
> 
> However, this patch makes the sclk_mipi to be used first.

No, the behavior, as described in the dt binding, is preserved by the hunk
below. dsi->pll_clk is only set, if the samsung,pll-clock-frequency property
is absent. If the dt property exists, dsi->pll_clk will be NULL and
dsi->pll_clk_rate will be used here.

Michael

> 
> Thanks,
> Inki Dae
> 
> >         fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> >         if (!fout) {
> >                 dev_err(dsi->dev,
> > @@ -1821,18 +1826,15 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
> >         u32 lane_polarities[5] = { 0 };
> >         struct device_node *endpoint;
> >         int i, nr_lanes, ret;
> > -       struct clk *pll_clk;
> >
> >         ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
> >                                        &dsi->pll_clk_rate, 1);
> >         /* If it doesn't exist, read it from the clock instead of failing */
> >         if (ret < 0) {
> >                 dev_dbg(dev, "Using sclk_mipi for pll clock frequency\n");
> > -               pll_clk = devm_clk_get(dev, "sclk_mipi");
> > -               if (!IS_ERR(pll_clk))
> > -                       dsi->pll_clk_rate = clk_get_rate(pll_clk);
> > -               else
> > -                       return PTR_ERR(pll_clk);
> > +               dsi->pll_clk = devm_clk_get(dev, "sclk_mipi");
> > +               if (IS_ERR(dsi->pll_clk))
> > +                       return PTR_ERR(dsi->pll_clk);
> >         }
> >
> >         /* If it doesn't exist, use pixel clock instead of failing */
> > diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> > index 05100e91ecb9..31ff88f152fb 100644
> > --- a/include/drm/bridge/samsung-dsim.h
> > +++ b/include/drm/bridge/samsung-dsim.h
> > @@ -87,6 +87,7 @@ struct samsung_dsim {
> >         void __iomem *reg_base;
> >         struct phy *phy;
> >         struct clk **clks;
> > +       struct clk *pll_clk;
> >         struct regulator_bulk_data supplies[2];
> >         int irq;
> >         struct gpio_desc *te_gpio;
> >
> > --
> > 2.39.2
> >
>