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

Commit Message

Michael Tretter Aug. 28, 2023, 3:59 p.m. UTC

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.

Signed-off-by: Michael Tretter <[e-mail]>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Marco Felsch Aug. 28, 2023, 4:41 p.m. UTC | #1

On 23-08-28, Michael Tretter wrote:
> 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.
> 
> Signed-off-by: Michael Tretter <[e-mail]>
> ---
>  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;

Really just a cosmetic nit: fin /= 2;

Reviewed-by: Marco Felsch <[e-mail]>

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

Adam Ford Aug. 28, 2023, 6:17 p.m. UTC | #2

On Mon, Aug 28, 2023 at 11:42 AM Marco Felsch <[e-mail]> wrote:
>
> On 23-08-28, Michael Tretter wrote:
> > 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.
> >
> > Signed-off-by: Michael Tretter <[e-mail]>
> > ---
> >  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;
>
> Really just a cosmetic nit: fin /= 2;
>
> Reviewed-by: Marco Felsch <[e-mail]>
>
> > +             clk_set_rate(dsi->pll_clk, fin);
> > +
> >               fin = clk_get_rate(dsi->pll_clk);
> > -     else
> > +     } else {
> >               fin = dsi->pll_clk_rate;
> > +     }

I don't have all the code style rules memorized.  Did you run it
through checkpatch?  I wonder if the braces here are appropriate for a
1-line after the else.  If checkpatch is happy, I am fine with it.

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

Michael Tretter Aug. 29, 2023, 7:49 a.m. UTC | #3

On Mon, 28 Aug 2023 13:17:44 -0500, Adam Ford wrote:
> On Mon, Aug 28, 2023 at 11:42 AM Marco Felsch <[e-mail]> wrote:
> >
> > On 23-08-28, Michael Tretter wrote:
> > > 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.
> > >
> > > Signed-off-by: Michael Tretter <[e-mail]>
> > > ---
> > >  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;
> >
> > Really just a cosmetic nit: fin /= 2;
> >
> > Reviewed-by: Marco Felsch <[e-mail]>
> >
> > > +             clk_set_rate(dsi->pll_clk, fin);
> > > +
> > >               fin = clk_get_rate(dsi->pll_clk);
> > > -     else
> > > +     } else {
> > >               fin = dsi->pll_clk_rate;
> > > +     }
> 
> I don't have all the code style rules memorized.  Did you run it
> through checkpatch?  I wonder if the braces here are appropriate for a
> 1-line after the else.  If checkpatch is happy, I am fine with it.

checkpatch is happy: The coding style states that if one of the branches needs
braces because it has multiple statements, all branches should have braces.

Michael

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

Inki Dae Sept. 4, 2023, 5:44 a.m. UTC | #4

2023년 8월 29일 (화) 오전 12:59, Michael Tretter <[e-mail]>님이 작성:
>
> 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?

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 <[e-mail]>
> ---
>  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
>

Michael Tretter Sept. 4, 2023, 11:15 a.m. UTC | #5

On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> 2023년 8월 29일 (화) 오전 12:59, Michael Tretter <[e-mail]>님이 작성:
> >
> > 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
frequency (fin) has a range of 6 MHz to 300 MHz.

Is the table incorrect?

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

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 <[e-mail]>
> > ---
> >  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
> >
>

Inki Dae Sept. 5, 2023, 9:06 a.m. UTC | #6

> -----Original Message-----
> From: dri-devel <[e-mail]> On Behalf Of
> Michael Tretter
> Sent: Monday, September 4, 2023 8:15 PM
> To: Inki Dae <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>; Robert Foss
> <[e-mail]>; Jonas Karlman <[e-mail]>; dri-
> [e-mail]; [e-mail]; Jernej Skrabec
> <[e-mail]>; Laurent Pinchart
> <[e-mail]>; Andrzej Hajda
> <[e-mail]>; Marek Szyprowski <[e-mail]>;
> [e-mail]; Jagan Teki <[e-mail]>
> 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 <[e-mail]>
> 님이 작성:
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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
> > >
> >

Michael Tretter Sept. 8, 2023, 9:47 a.m. UTC | #7

On Tue, 05 Sep 2023 18:06:06 +0900, 대인기/Tizen Platform Lab(SR)/삼성전자 wrote:
> 
> 
> > -----Original Message-----
> > From: dri-devel <[e-mail]> On Behalf Of
> > Michael Tretter
> > Sent: Monday, September 4, 2023 8:15 PM
> > To: Inki Dae <[e-mail]>
> > Cc: Neil Armstrong <[e-mail]>; Robert Foss
> > <[e-mail]>; Jonas Karlman <[e-mail]>; dri-
> > [e-mail]; [e-mail]; Jernej Skrabec
> > <[e-mail]>; Laurent Pinchart
> > <[e-mail]>; Andrzej Hajda
> > <[e-mail]>; Marek Szyprowski <[e-mail]>;
> > [e-mail]; Jagan Teki <[e-mail]>
> > 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 <[e-mail]>
> > 님이 작성:
> > > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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
> > > >
> > >
> 
> 
>

Inki Dae Sept. 18, 2023, 8:42 a.m. UTC | #8

> -----Original Message-----
> From: Michael Tretter <[e-mail]>
> Sent: Friday, September 8, 2023 6:48 PM
> To: 대인기/Tizen Platform Lab(SR)/삼성전자 <[e-mail]>
> Cc: 'Inki Dae' <[e-mail]>; 'Neil Armstrong'
> <[e-mail]>; 'Robert Foss' <[e-mail]>; 'Jonas
> Karlman' <[e-mail]>; [e-mail]; linux-
> [e-mail]; 'Jernej Skrabec' <[e-mail]>;
> 'Laurent Pinchart' <[e-mail]>; 'Andrzej Hajda'
> <[e-mail]>; 'Marek Szyprowski' <[e-mail]>;
> [e-mail]; 'Jagan Teki' <[e-mail]>
> 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 <[e-mail]> On Behalf Of
> > > Michael Tretter
> > > Sent: Monday, September 4, 2023 8:15 PM
> > > To: Inki Dae <[e-mail]>
> > > Cc: Neil Armstrong <[e-mail]>; Robert Foss
> > > <[e-mail]>; Jonas Karlman <[e-mail]>; dri-
> > > [e-mail]; [e-mail]; Jernej
> Skrabec
> > > <[e-mail]>; Laurent Pinchart
> > > <[e-mail]>; Andrzej Hajda
> > > <[e-mail]>; Marek Szyprowski <[e-mail]>;
> > > [e-mail]; Jagan Teki <[e-mail]>
> > > 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
> <[e-mail]>
> > > 님이 작성:
> > > > >
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  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
> > > > >
> > > >
> >
> >
> >

Patch

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