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

[대인기/Tizen Platform Lab(SR)/삼성전자]

> -----Original Message-----
> From: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
> Sent: Friday, September 8, 2023 6:48 PM
> To: 대인기/Tizen Platform Lab(SR)/삼성전자 <inki.dae@xxxxxxxxxxx>
> Cc: 'Inki Dae' <daeinki@xxxxxxxxx>; '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 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.

Yes, it does. The actual clock can vary according to clock configuration - which child clocks are used and divider values of the clock sources - of the pll_clk source. It would mean that the clock configuration depends on the machine, not the SoC, because the display panel may vary depending on the machine.

In my opinion, a role of the device driver would be to configure the clock sources properly, such as the current driver does, and we may need to find a proper divider value of each clock source and set it in the respective machine device tree so that the actual clock frequency can be close to the given pixel clock frequency provided by the machine device tree as "samsung,pll-clock-frequency".

As a result, even if the type of display attached to the machine varies I think the clock configuration method in the device driver code should not change.

Thanks,
Inki Dae

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