RE: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock

["대인기/Tizen Platform Lab(SR)/삼성전자" <inki.dae@samsung.com>]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> Michael Tretter
> Sent: Monday, September 4, 2023 8:15 PM
> To: Inki Dae <daeinki@gmail.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss
> <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; dri-
> devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Jernej Skrabec
> <jernej.skrabec@gmail.com>; Laurent Pinchart
> <Laurent.pinchart@ideasonboard.com>; Andrzej Hajda
> <andrzej.hajda@intel.com>; Marek Szyprowski <m.szyprowski@samsung.com>;
> kernel@pengutronix.de; Jagan Teki <jagan@amarulasolutions.com>
> Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference
> clock
> 
> On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> > 2023년 8월 29일 (화) 오전 12:59, Michael Tretter <m.tretter@pengutronix.de>
> 님이 작성:
> > >
> > > The PLL requires a clock between 2 MHz and 30 MHz after the pre-
> divider.
> > > The reference clock for the PLL may change due to changes to it's
> parent
> > > clock. Thus, the frequency may be out of range or unsuited for
> > > generating the high speed clock for MIPI DSI.
> > >
> > > Try to keep the pre-devider small, and set the reference clock close
> to
> > > 30 MHz before recalculating the PLL configuration. Use a divider with
> a
> > > power of two for the reference clock as this seems to work best in
> > > my tests.
> >
> > Clock frequency is specific to SoC architecture so we have to handle
> > it carefully because samsung-dsim.c is a common module for I.MX and
> > Exynos series.
> > You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
> > pre-divider", and the clock means that fin_pll - PFD input frequency -
> > which can be calculated with oscillator clock frequency / P value?
> > According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.
> >
> > For example,
> > In case of Exyhos, we use 24MHz as oscillator clock frequency, so
> > fin_pll frequency, 8MHz = 24MHz / P(3).
> >
> > Can you tell me the source of the constraint that clocks must be
> > between 2MHz and 30MHz?
> 
> The source is the i.MX8M Nano reference manual, Table 13-40. DPHY PLL
> Parameters. It documents that the P divider frequency (fin_pll) has a
> frequency range of 2 MHz to 30 MHz. According to the same table, the input

In case of Exynos, the range is from 6MHz to 12MHz according to Exynos4212 reference manual, Table 1-5.

> frequency (fin) has a range of 6 MHz to 300 MHz.

In case of Exynos, the range is from 6MHz to 200MHz.

> 
> Is the table incorrect?

It's correct for I.MX but incorrect for Exynos. I think it would mean that the valid range depends on SoC architecture. So I'd say that this patch is specific to I.MX. This was one of what I concerted about when trying to move samsung-dsim.c module to bridge directory for common use between I.MX and Exynos Platforms, and this will be what we have to solve together - I.MX and Exynos engineers. How about using platform specific data - samsung_dsim_driver_data structure?

I.e,

static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
    ...
    .min_fin_pll = 2,
    .max_fin_pll = 30,
    ...
};

static const struct samsung_dsim_driver_data exynosxxxx_dsi_driver_data = {
    ...
    .min_fin_pll = 6,
    .max_fin_pll = 12,
    ...
};

And then,

while (fin > driver_data->max_fin_pll * MHZ)
    fin = fin / 2;

> 
> I also tried to always set the reference clock to 24 MHz, but depending on
> the
> display clock this isn't always possible.

According to dt binding, if samsung,pll-clock-frequency exists then we have to use it first. I'm not sure but could we check if the pll_clk_rate is valid or not depending on the given display clock in advance? If so then maybe we could update the pll_clk_rate correctly by reading the pll_clk frequency again.

Thanks,
Inki Dae

> 
> Michael
> 
> >
> > To other I.MX and Exynos engineers, please do not merge this patch
> > until two SoC platforms are tested correctly.
> >
> > Thanks,
> > Inki Dae
> >
> > >
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index da90c2038042..4de6e4f116db 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> > >         u16 m;
> > >         u32 reg;
> > >
> > > -       if (dsi->pll_clk)
> > > +       if (dsi->pll_clk) {
> > > +               /*
> > > +                * Ensure that the reference clock is generated with a
> power of
> > > +                * two divider from its parent, but close to the PLLs
> upper
> > > +                * limit of the valid range of 2 MHz to 30 MHz.
> > > +                */
> > > +               fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > > +               while (fin > 30 * MHZ)
> > > +                       fin = fin / 2;
> > > +               clk_set_rate(dsi->pll_clk, fin);
> > > +
> > >                 fin = clk_get_rate(dsi->pll_clk);
> > > -       else
> > > +       } else {
> > >                 fin = dsi->pll_clk_rate;
> > > +       }
> > >         dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> > >
> > >         fout = samsung_dsim_pll_find_pms(dsi, fin, freq, &p, &m, &s);
> > >
> > > --
> > > 2.39.2
> > >
> >