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

[Michael Tretter]

On Tue, 05 Sep 2023 18:06:06 +0900, 대인기/Tizen Platform Lab(SR)/삼성전자 wrote:
> 
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Michael Tretter
> > Sent: Monday, September 4, 2023 8:15 PM
> > To: Inki Dae <daeinki@xxxxxxxxx>
> > Cc: Neil Armstrong <neil.armstrong@xxxxxxxxxx>; Robert Foss
> > <rfoss@xxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>; dri-
> > devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jernej Skrabec
> > <jernej.skrabec@xxxxxxxxx>; Laurent Pinchart
> > <Laurent.pinchart@xxxxxxxxxxxxxxxx>; Andrzej Hajda
> > <andrzej.hajda@xxxxxxxxx>; Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>;
> > kernel@xxxxxxxxxxxxxx; Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxx>
> > 님이 작성:
> > > >
> > > > 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

Small clarification: These are the corrected limits of TRM rev. 1. In TRM rev.
0 the limits are incorrectly specified as 6 MHz to 12 MHz.

> 
> 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.

I understand.

> 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,
>     ...
> };

Extending the samsung_dsim_driver_data sounds reasonable. There are already
other frequency limits specified. I will implement this in v2.

> 
> 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.

If samsung,pll-clock-frequency is set, the driver will neither check nor
update the pll_clk, but assume that the clocks are configured correctly. Thus,
the author of the device tree is responsible for selecting and configuring
valid clocks.

I observed that if I set pll_clk to any fixed value for different modes, the
clock framework may use a different clock rate depending on what is possible.
This may result in a reference clock that uses a fractional divider to get the
pll_clk_rate or cannot exactly produce the hs_clock. These situations cause
sync issues on the display.

Checking, if the current pll_clk_rate would involve the pix_clk, the hs_clock,
and the parent of the pll_clk. It may be possible, but I don't see a problem
with calculating a suitable pll_clk_rate, updating the pll_clk, and then
configuring the PLL to generate the hs_clock.

Michael

> 
> 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@xxxxxxxxxxxxxx>
> > > > ---
> > > >  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
> > > >
> > >
> 
> 
>