All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-06 19:53 ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-06 19:53 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, Adam Ford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

Currently, certain clocks are derrived as a divider from their
parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
is set, the parent clock is not properly set which can lead
to some relatively inaccurate clock values.

Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
cannot rely on calling a standard determine_rate function,
because the 8m composite clocks have a pre-divider and
post-divider. Because of this, a custom determine_rate
function is necessary to determine the maximum clock
division which is equivalent to pre-divider * the
post-divider.

With this added, the system can attempt to adjust the parent rate
when the proper flags are set which can lead to a more precise clock
value.

On the imx8mplus, no clock changes are present.
On the Mini and Nano, this can help achieve more accurate
lcdif clocks. When trying to get a pixel clock of 31.500MHz
on an imx8m Nano, the clocks divided the 594MHz down, but
left the parent rate untouched which caused a calulation error.

Before:
video_pll              594000000
  video_pll_bypass     594000000
    video_pll_out      594000000
      disp_pixel       31263158
        disp_pixel_clk 31263158

Variance = -236842 Hz

After this patch:
video_pll               31500000
  video_pll_bypass      31500000
    video_pll_out       31500000
      disp_pixel        31500000
        disp_pixel_clk  31500000

Variance = 0 Hz

All other clocks rates and parent were the same.
Similar results on imx8mm were found.

Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
Signed-off-by: Adam Ford <aford173@gmail.com>
---
V2:  Fix build warning found by build bot and fix prediv_value
     and div_value because the values stored are the divisor - 1,
     so we need to add 1 to the values to be correct.

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index cbf0d7955a00..7a6e3ce97133 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
 	return ret;
 }
 
+static int imx8m_divider_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	int prediv_value;
+	int div_value;
+
+	/* if read only, just return current value */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
+		u32 val;
+
+		val = readl(divider->reg);
+		prediv_value = val >> divider->shift;
+		prediv_value &= clk_div_mask(divider->width);
+		prediv_value++;
+
+		div_value = val >> PCG_DIV_SHIFT;
+		div_value &= clk_div_mask(PCG_DIV_WIDTH);
+		div_value++;
+
+		return divider_ro_determine_rate(hw, req, divider->table,
+						 PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+						 divider->flags, prediv_value * div_value);
+	}
+
+	return divider_determine_rate(hw, req, divider->table,
+				      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+				      divider->flags);
+}
+
 static const struct clk_ops imx8m_clk_composite_divider_ops = {
 	.recalc_rate = imx8m_clk_composite_divider_recalc_rate,
 	.round_rate = imx8m_clk_composite_divider_round_rate,
 	.set_rate = imx8m_clk_composite_divider_set_rate,
+	.determine_rate = imx8m_divider_determine_rate,
 };
 
 static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-06 19:53 ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-06 19:53 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, Adam Ford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

Currently, certain clocks are derrived as a divider from their
parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
is set, the parent clock is not properly set which can lead
to some relatively inaccurate clock values.

Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
cannot rely on calling a standard determine_rate function,
because the 8m composite clocks have a pre-divider and
post-divider. Because of this, a custom determine_rate
function is necessary to determine the maximum clock
division which is equivalent to pre-divider * the
post-divider.

With this added, the system can attempt to adjust the parent rate
when the proper flags are set which can lead to a more precise clock
value.

On the imx8mplus, no clock changes are present.
On the Mini and Nano, this can help achieve more accurate
lcdif clocks. When trying to get a pixel clock of 31.500MHz
on an imx8m Nano, the clocks divided the 594MHz down, but
left the parent rate untouched which caused a calulation error.

Before:
video_pll              594000000
  video_pll_bypass     594000000
    video_pll_out      594000000
      disp_pixel       31263158
        disp_pixel_clk 31263158

Variance = -236842 Hz

After this patch:
video_pll               31500000
  video_pll_bypass      31500000
    video_pll_out       31500000
      disp_pixel        31500000
        disp_pixel_clk  31500000

Variance = 0 Hz

All other clocks rates and parent were the same.
Similar results on imx8mm were found.

Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
Signed-off-by: Adam Ford <aford173@gmail.com>
---
V2:  Fix build warning found by build bot and fix prediv_value
     and div_value because the values stored are the divisor - 1,
     so we need to add 1 to the values to be correct.

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index cbf0d7955a00..7a6e3ce97133 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
 	return ret;
 }
 
+static int imx8m_divider_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	int prediv_value;
+	int div_value;
+
+	/* if read only, just return current value */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
+		u32 val;
+
+		val = readl(divider->reg);
+		prediv_value = val >> divider->shift;
+		prediv_value &= clk_div_mask(divider->width);
+		prediv_value++;
+
+		div_value = val >> PCG_DIV_SHIFT;
+		div_value &= clk_div_mask(PCG_DIV_WIDTH);
+		div_value++;
+
+		return divider_ro_determine_rate(hw, req, divider->table,
+						 PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+						 divider->flags, prediv_value * div_value);
+	}
+
+	return divider_determine_rate(hw, req, divider->table,
+				      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+				      divider->flags);
+}
+
 static const struct clk_ops imx8m_clk_composite_divider_ops = {
 	.recalc_rate = imx8m_clk_composite_divider_recalc_rate,
 	.round_rate = imx8m_clk_composite_divider_round_rate,
 	.set_rate = imx8m_clk_composite_divider_set_rate,
+	.determine_rate = imx8m_divider_determine_rate,
 };
 
 static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-06 19:53 ` Adam Ford
  (?)
@ 2023-05-12  3:03 ` Adam Ford
  2023-05-19 22:04     ` Adam Ford
  -1 siblings, 1 reply; 28+ messages in thread
From: Adam Ford @ 2023-05-12  3:03 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, Abel Vesa, Peng Fan, Michael Turquette, Stephen Boyd,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Sat, May 6, 2023 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:
>
> Currently, certain clocks are derrived as a divider from their
> parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> is set, the parent clock is not properly set which can lead
> to some relatively inaccurate clock values.
>
> Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> cannot rely on calling a standard determine_rate function,
> because the 8m composite clocks have a pre-divider and
> post-divider. Because of this, a custom determine_rate
> function is necessary to determine the maximum clock
> division which is equivalent to pre-divider * the
> post-divider.
>
> With this added, the system can attempt to adjust the parent rate
> when the proper flags are set which can lead to a more precise clock
> value.
>
> On the imx8mplus, no clock changes are present.
> On the Mini and Nano, this can help achieve more accurate
> lcdif clocks. When trying to get a pixel clock of 31.500MHz
> on an imx8m Nano, the clocks divided the 594MHz down, but
> left the parent rate untouched which caused a calulation error.
>
> Before:
> video_pll              594000000
>   video_pll_bypass     594000000
>     video_pll_out      594000000
>       disp_pixel       31263158
>         disp_pixel_clk 31263158
>
> Variance = -236842 Hz
>
> After this patch:
> video_pll               31500000
>   video_pll_bypass      31500000
>     video_pll_out       31500000
>       disp_pixel        31500000
>         disp_pixel_clk  31500000
>
> Variance = 0 Hz
>
> All other clocks rates and parent were the same.
> Similar results on imx8mm were found.
>

Peng / Abel,

I was curious if either of you might have time to review this attempt
at enabling determine_rate on the 8m's.  I tested this on the 8mm,
8mn, and 8mp, and found no regressions.

adam
> Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> V2:  Fix build warning found by build bot and fix prediv_value
>      and div_value because the values stored are the divisor - 1,
>      so we need to add 1 to the values to be correct.
>
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index cbf0d7955a00..7a6e3ce97133 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
>         return ret;
>  }
>
> +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> +                                     struct clk_rate_request *req)
> +{
> +       struct clk_divider *divider = to_clk_divider(hw);
> +       int prediv_value;
> +       int div_value;
> +
> +       /* if read only, just return current value */
> +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> +               u32 val;
> +
> +               val = readl(divider->reg);
> +               prediv_value = val >> divider->shift;
> +               prediv_value &= clk_div_mask(divider->width);
> +               prediv_value++;
> +
> +               div_value = val >> PCG_DIV_SHIFT;
> +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> +               div_value++;
> +
> +               return divider_ro_determine_rate(hw, req, divider->table,
> +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +                                                divider->flags, prediv_value * div_value);
> +       }
> +
> +       return divider_determine_rate(hw, req, divider->table,
> +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +                                     divider->flags);
> +}
> +
>  static const struct clk_ops imx8m_clk_composite_divider_ops = {
>         .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
>         .round_rate = imx8m_clk_composite_divider_round_rate,
>         .set_rate = imx8m_clk_composite_divider_set_rate,
> +       .determine_rate = imx8m_divider_determine_rate,
>  };
>
>  static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> --
> 2.39.2
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-12  3:03 ` Adam Ford
@ 2023-05-19 22:04     ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-19 22:04 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, Abel Vesa, Peng Fan, Michael Turquette, Stephen Boyd,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Thu, May 11, 2023 at 10:03 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Sat, May 6, 2023 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > Currently, certain clocks are derrived as a divider from their
> > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > is set, the parent clock is not properly set which can lead
> > to some relatively inaccurate clock values.
> >
> > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > cannot rely on calling a standard determine_rate function,
> > because the 8m composite clocks have a pre-divider and
> > post-divider. Because of this, a custom determine_rate
> > function is necessary to determine the maximum clock
> > division which is equivalent to pre-divider * the
> > post-divider.
> >
> > With this added, the system can attempt to adjust the parent rate
> > when the proper flags are set which can lead to a more precise clock
> > value.
> >
> > On the imx8mplus, no clock changes are present.
> > On the Mini and Nano, this can help achieve more accurate
> > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > on an imx8m Nano, the clocks divided the 594MHz down, but
> > left the parent rate untouched which caused a calulation error.
> >
> > Before:
> > video_pll              594000000
> >   video_pll_bypass     594000000
> >     video_pll_out      594000000
> >       disp_pixel       31263158
> >         disp_pixel_clk 31263158
> >
> > Variance = -236842 Hz
> >
> > After this patch:
> > video_pll               31500000
> >   video_pll_bypass      31500000
> >     video_pll_out       31500000
> >       disp_pixel        31500000
> >         disp_pixel_clk  31500000
> >
> > Variance = 0 Hz
> >
> > All other clocks rates and parent were the same.
> > Similar results on imx8mm were found.
> >
>
> Peng / Abel,
>
> I was curious if either of you might have time to review this attempt
> at enabling determine_rate on the 8m's.  I tested this on the 8mm,
> 8mn, and 8mp, and found no regressions.

Gentle nudge.

It's been several weeks since the initial post and the DSI driver is
now available for Mini and Nano, so having this in Mini and Nano will
really help it sync various video sources.

thanks,

adam

>
> adam
> > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > V2:  Fix build warning found by build bot and fix prediv_value
> >      and div_value because the values stored are the divisor - 1,
> >      so we need to add 1 to the values to be correct.
> >
> > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> > index cbf0d7955a00..7a6e3ce97133 100644
> > --- a/drivers/clk/imx/clk-composite-8m.c
> > +++ b/drivers/clk/imx/clk-composite-8m.c
> > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> >         return ret;
> >  }
> >
> > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > +                                     struct clk_rate_request *req)
> > +{
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       int prediv_value;
> > +       int div_value;
> > +
> > +       /* if read only, just return current value */
> > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > +               u32 val;
> > +
> > +               val = readl(divider->reg);
> > +               prediv_value = val >> divider->shift;
> > +               prediv_value &= clk_div_mask(divider->width);
> > +               prediv_value++;
> > +
> > +               div_value = val >> PCG_DIV_SHIFT;
> > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > +               div_value++;
> > +
> > +               return divider_ro_determine_rate(hw, req, divider->table,
> > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                                divider->flags, prediv_value * div_value);
> > +       }
> > +
> > +       return divider_determine_rate(hw, req, divider->table,
> > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                     divider->flags);
> > +}
> > +
> >  static const struct clk_ops imx8m_clk_composite_divider_ops = {
> >         .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> >         .round_rate = imx8m_clk_composite_divider_round_rate,
> >         .set_rate = imx8m_clk_composite_divider_set_rate,
> > +       .determine_rate = imx8m_divider_determine_rate,
> >  };
> >
> >  static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > --
> > 2.39.2
> >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-19 22:04     ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-19 22:04 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, Abel Vesa, Peng Fan, Michael Turquette, Stephen Boyd,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Thu, May 11, 2023 at 10:03 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Sat, May 6, 2023 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > Currently, certain clocks are derrived as a divider from their
> > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > is set, the parent clock is not properly set which can lead
> > to some relatively inaccurate clock values.
> >
> > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > cannot rely on calling a standard determine_rate function,
> > because the 8m composite clocks have a pre-divider and
> > post-divider. Because of this, a custom determine_rate
> > function is necessary to determine the maximum clock
> > division which is equivalent to pre-divider * the
> > post-divider.
> >
> > With this added, the system can attempt to adjust the parent rate
> > when the proper flags are set which can lead to a more precise clock
> > value.
> >
> > On the imx8mplus, no clock changes are present.
> > On the Mini and Nano, this can help achieve more accurate
> > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > on an imx8m Nano, the clocks divided the 594MHz down, but
> > left the parent rate untouched which caused a calulation error.
> >
> > Before:
> > video_pll              594000000
> >   video_pll_bypass     594000000
> >     video_pll_out      594000000
> >       disp_pixel       31263158
> >         disp_pixel_clk 31263158
> >
> > Variance = -236842 Hz
> >
> > After this patch:
> > video_pll               31500000
> >   video_pll_bypass      31500000
> >     video_pll_out       31500000
> >       disp_pixel        31500000
> >         disp_pixel_clk  31500000
> >
> > Variance = 0 Hz
> >
> > All other clocks rates and parent were the same.
> > Similar results on imx8mm were found.
> >
>
> Peng / Abel,
>
> I was curious if either of you might have time to review this attempt
> at enabling determine_rate on the 8m's.  I tested this on the 8mm,
> 8mn, and 8mp, and found no regressions.

Gentle nudge.

It's been several weeks since the initial post and the DSI driver is
now available for Mini and Nano, so having this in Mini and Nano will
really help it sync various video sources.

thanks,

adam

>
> adam
> > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > V2:  Fix build warning found by build bot and fix prediv_value
> >      and div_value because the values stored are the divisor - 1,
> >      so we need to add 1 to the values to be correct.
> >
> > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> > index cbf0d7955a00..7a6e3ce97133 100644
> > --- a/drivers/clk/imx/clk-composite-8m.c
> > +++ b/drivers/clk/imx/clk-composite-8m.c
> > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> >         return ret;
> >  }
> >
> > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > +                                     struct clk_rate_request *req)
> > +{
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       int prediv_value;
> > +       int div_value;
> > +
> > +       /* if read only, just return current value */
> > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > +               u32 val;
> > +
> > +               val = readl(divider->reg);
> > +               prediv_value = val >> divider->shift;
> > +               prediv_value &= clk_div_mask(divider->width);
> > +               prediv_value++;
> > +
> > +               div_value = val >> PCG_DIV_SHIFT;
> > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > +               div_value++;
> > +
> > +               return divider_ro_determine_rate(hw, req, divider->table,
> > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                                divider->flags, prediv_value * div_value);
> > +       }
> > +
> > +       return divider_determine_rate(hw, req, divider->table,
> > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                     divider->flags);
> > +}
> > +
> >  static const struct clk_ops imx8m_clk_composite_divider_ops = {
> >         .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> >         .round_rate = imx8m_clk_composite_divider_round_rate,
> >         .set_rate = imx8m_clk_composite_divider_set_rate,
> > +       .determine_rate = imx8m_divider_determine_rate,
> >  };
> >
> >  static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > --
> > 2.39.2
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-19 22:04     ` Adam Ford
@ 2023-05-20 11:36       ` Peng Fan
  -1 siblings, 0 replies; 28+ messages in thread
From: Peng Fan @ 2023-05-20 11:36 UTC (permalink / raw)
  To: Adam Ford, linux-clk
  Cc: aford, Abel Vesa, Michael Turquette, Stephen Boyd, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

> Subject: Re: [PATCH] clk: imx: composite-8m: Add
> imx8m_divider_determine_rate
> 
> On Thu, May 11, 2023 at 10:03 PM Adam Ford <aford173@gmail.com>
> wrote:
> >
> > On Sat, May 6, 2023 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > Currently, certain clocks are derrived as a divider from their
> > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT is
> > > set, the parent clock is not properly set which can lead to some
> > > relatively inaccurate clock values.
> > >
> > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it cannot rely
> > > on calling a standard determine_rate function, because the 8m
> > > composite clocks have a pre-divider and post-divider. Because of
> > > this, a custom determine_rate function is necessary to determine the
> > > maximum clock division which is equivalent to pre-divider * the
> > > post-divider.
> > >
> > > With this added, the system can attempt to adjust the parent rate
> > > when the proper flags are set which can lead to a more precise clock
> > > value.
> > >
> > > On the imx8mplus, no clock changes are present.
> > > On the Mini and Nano, this can help achieve more accurate lcdif
> > > clocks. When trying to get a pixel clock of 31.500MHz on an imx8m
> > > Nano, the clocks divided the 594MHz down, but left the parent rate
> > > untouched which caused a calulation error.
> > >
> > > Before:
> > > video_pll              594000000
> > >   video_pll_bypass     594000000
> > >     video_pll_out      594000000
> > >       disp_pixel       31263158
> > >         disp_pixel_clk 31263158
> > >
> > > Variance = -236842 Hz
> > >
> > > After this patch:
> > > video_pll               31500000
> > >   video_pll_bypass      31500000
> > >     video_pll_out       31500000
> > >       disp_pixel        31500000
> > >         disp_pixel_clk  31500000
> > >
> > > Variance = 0 Hz
> > >
> > > All other clocks rates and parent were the same.
> > > Similar results on imx8mm were found.
> > >
> >
> > Peng / Abel,
> >
> > I was curious if either of you might have time to review this attempt
> > at enabling determine_rate on the 8m's.  I tested this on the 8mm,
> > 8mn, and 8mp, and found no regressions.
> 
> Gentle nudge.
> 
> It's been several weeks since the initial post and the DSI driver is now
> available for Mini and Nano, so having this in Mini and Nano will really help
> it sync various video sources.

Sorry, overlooked this patch. Will take a look.

Regards,
Peng.

> 
> thanks,
> 
> adam
> 
> >
> > adam
> > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to
> > > determine_rate"")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > > V2:  Fix build warning found by build bot and fix prediv_value
> > >      and div_value because the values stored are the divisor - 1,
> > >      so we need to add 1 to the values to be correct.
> > >
> > > diff --git a/drivers/clk/imx/clk-composite-8m.c
> > > b/drivers/clk/imx/clk-composite-8m.c
> > > index cbf0d7955a00..7a6e3ce97133 100644
> > > --- a/drivers/clk/imx/clk-composite-8m.c
> > > +++ b/drivers/clk/imx/clk-composite-8m.c
> > > @@ -119,10 +119,41 @@ static int
> imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> > >         return ret;
> > >  }
> > >
> > > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > > +                                     struct clk_rate_request *req)
> > > +{
> > > +       struct clk_divider *divider = to_clk_divider(hw);
> > > +       int prediv_value;
> > > +       int div_value;
> > > +
> > > +       /* if read only, just return current value */
> > > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > > +               u32 val;
> > > +
> > > +               val = readl(divider->reg);
> > > +               prediv_value = val >> divider->shift;
> > > +               prediv_value &= clk_div_mask(divider->width);
> > > +               prediv_value++;
> > > +
> > > +               div_value = val >> PCG_DIV_SHIFT;
> > > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > > +               div_value++;
> > > +
> > > +               return divider_ro_determine_rate(hw, req, divider->table,
> > > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                                divider->flags, prediv_value * div_value);
> > > +       }
> > > +
> > > +       return divider_determine_rate(hw, req, divider->table,
> > > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                     divider->flags); }
> > > +
> > >  static const struct clk_ops imx8m_clk_composite_divider_ops = {
> > >         .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> > >         .round_rate = imx8m_clk_composite_divider_round_rate,
> > >         .set_rate = imx8m_clk_composite_divider_set_rate,
> > > +       .determine_rate = imx8m_divider_determine_rate,
> > >  };
> > >
> > >  static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > > --
> > > 2.39.2
> > >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-20 11:36       ` Peng Fan
  0 siblings, 0 replies; 28+ messages in thread
From: Peng Fan @ 2023-05-20 11:36 UTC (permalink / raw)
  To: Adam Ford, linux-clk
  Cc: aford, Abel Vesa, Michael Turquette, Stephen Boyd, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

> Subject: Re: [PATCH] clk: imx: composite-8m: Add
> imx8m_divider_determine_rate
> 
> On Thu, May 11, 2023 at 10:03 PM Adam Ford <aford173@gmail.com>
> wrote:
> >
> > On Sat, May 6, 2023 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > Currently, certain clocks are derrived as a divider from their
> > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT is
> > > set, the parent clock is not properly set which can lead to some
> > > relatively inaccurate clock values.
> > >
> > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it cannot rely
> > > on calling a standard determine_rate function, because the 8m
> > > composite clocks have a pre-divider and post-divider. Because of
> > > this, a custom determine_rate function is necessary to determine the
> > > maximum clock division which is equivalent to pre-divider * the
> > > post-divider.
> > >
> > > With this added, the system can attempt to adjust the parent rate
> > > when the proper flags are set which can lead to a more precise clock
> > > value.
> > >
> > > On the imx8mplus, no clock changes are present.
> > > On the Mini and Nano, this can help achieve more accurate lcdif
> > > clocks. When trying to get a pixel clock of 31.500MHz on an imx8m
> > > Nano, the clocks divided the 594MHz down, but left the parent rate
> > > untouched which caused a calulation error.
> > >
> > > Before:
> > > video_pll              594000000
> > >   video_pll_bypass     594000000
> > >     video_pll_out      594000000
> > >       disp_pixel       31263158
> > >         disp_pixel_clk 31263158
> > >
> > > Variance = -236842 Hz
> > >
> > > After this patch:
> > > video_pll               31500000
> > >   video_pll_bypass      31500000
> > >     video_pll_out       31500000
> > >       disp_pixel        31500000
> > >         disp_pixel_clk  31500000
> > >
> > > Variance = 0 Hz
> > >
> > > All other clocks rates and parent were the same.
> > > Similar results on imx8mm were found.
> > >
> >
> > Peng / Abel,
> >
> > I was curious if either of you might have time to review this attempt
> > at enabling determine_rate on the 8m's.  I tested this on the 8mm,
> > 8mn, and 8mp, and found no regressions.
> 
> Gentle nudge.
> 
> It's been several weeks since the initial post and the DSI driver is now
> available for Mini and Nano, so having this in Mini and Nano will really help
> it sync various video sources.

Sorry, overlooked this patch. Will take a look.

Regards,
Peng.

> 
> thanks,
> 
> adam
> 
> >
> > adam
> > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to
> > > determine_rate"")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > > V2:  Fix build warning found by build bot and fix prediv_value
> > >      and div_value because the values stored are the divisor - 1,
> > >      so we need to add 1 to the values to be correct.
> > >
> > > diff --git a/drivers/clk/imx/clk-composite-8m.c
> > > b/drivers/clk/imx/clk-composite-8m.c
> > > index cbf0d7955a00..7a6e3ce97133 100644
> > > --- a/drivers/clk/imx/clk-composite-8m.c
> > > +++ b/drivers/clk/imx/clk-composite-8m.c
> > > @@ -119,10 +119,41 @@ static int
> imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> > >         return ret;
> > >  }
> > >
> > > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > > +                                     struct clk_rate_request *req)
> > > +{
> > > +       struct clk_divider *divider = to_clk_divider(hw);
> > > +       int prediv_value;
> > > +       int div_value;
> > > +
> > > +       /* if read only, just return current value */
> > > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > > +               u32 val;
> > > +
> > > +               val = readl(divider->reg);
> > > +               prediv_value = val >> divider->shift;
> > > +               prediv_value &= clk_div_mask(divider->width);
> > > +               prediv_value++;
> > > +
> > > +               div_value = val >> PCG_DIV_SHIFT;
> > > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > > +               div_value++;
> > > +
> > > +               return divider_ro_determine_rate(hw, req, divider->table,
> > > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                                divider->flags, prediv_value * div_value);
> > > +       }
> > > +
> > > +       return divider_determine_rate(hw, req, divider->table,
> > > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                     divider->flags); }
> > > +
> > >  static const struct clk_ops imx8m_clk_composite_divider_ops = {
> > >         .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> > >         .round_rate = imx8m_clk_composite_divider_round_rate,
> > >         .set_rate = imx8m_clk_composite_divider_set_rate,
> > > +       .determine_rate = imx8m_divider_determine_rate,
> > >  };
> > >
> > >  static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > > --
> > > 2.39.2
> > >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-06 19:53 ` Adam Ford
@ 2023-05-23  2:32   ` Peng Fan
  -1 siblings, 0 replies; 28+ messages in thread
From: Peng Fan @ 2023-05-23  2:32 UTC (permalink / raw)
  To: Adam Ford, linux-clk
  Cc: aford, Abel Vesa, Peng Fan, Michael Turquette, Stephen Boyd,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list



On 5/7/2023 3:53 AM, Adam Ford wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Currently, certain clocks are derrived as a divider from their
> parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> is set, the parent clock is not properly set which can lead
> to some relatively inaccurate clock values.
> 
> Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> cannot rely on calling a standard determine_rate function,
> because the 8m composite clocks have a pre-divider and
> post-divider. Because of this, a custom determine_rate
> function is necessary to determine the maximum clock
> division which is equivalent to pre-divider * the
> post-divider.
> 
> With this added, the system can attempt to adjust the parent rate
> when the proper flags are set which can lead to a more precise clock
> value.
> 
> On the imx8mplus, no clock changes are present.
> On the Mini and Nano, this can help achieve more accurate
> lcdif clocks. When trying to get a pixel clock of 31.500MHz
> on an imx8m Nano, the clocks divided the 594MHz down, but
> left the parent rate untouched which caused a calulation error.

Not all clocks has pre/post div both.

If CLK_SET_RATE_PARENT not set, would there be any issues for
other clocks?

Regards,
Peng.

> 
> Before:
> video_pll              594000000
>    video_pll_bypass     594000000
>      video_pll_out      594000000
>        disp_pixel       31263158
>          disp_pixel_clk 31263158
> 
> Variance = -236842 Hz
> 
> After this patch:
> video_pll               31500000
>    video_pll_bypass      31500000
>      video_pll_out       31500000
>        disp_pixel        31500000
>          disp_pixel_clk  31500000
> 
> Variance = 0 Hz
> 
> All other clocks rates and parent were the same.
> Similar results on imx8mm were found.
> 
> Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> V2:  Fix build warning found by build bot and fix prediv_value
>       and div_value because the values stored are the divisor - 1,
>       so we need to add 1 to the values to be correct.
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index cbf0d7955a00..7a6e3ce97133 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
>          return ret;
>   }
> 
> +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> +                                     struct clk_rate_request *req)
> +{
> +       struct clk_divider *divider = to_clk_divider(hw);
> +       int prediv_value;
> +       int div_value;
> +
> +       /* if read only, just return current value */
> +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> +               u32 val;
> +
> +               val = readl(divider->reg);
> +               prediv_value = val >> divider->shift;
> +               prediv_value &= clk_div_mask(divider->width);
> +               prediv_value++;
> +
> +               div_value = val >> PCG_DIV_SHIFT;
> +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> +               div_value++;
> +
> +               return divider_ro_determine_rate(hw, req, divider->table,
> +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +                                                divider->flags, prediv_value * div_value);
> +       }
> +
> +       return divider_determine_rate(hw, req, divider->table,
> +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +                                     divider->flags);
> +}
> +
>   static const struct clk_ops imx8m_clk_composite_divider_ops = {
>          .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
>          .round_rate = imx8m_clk_composite_divider_round_rate,
>          .set_rate = imx8m_clk_composite_divider_set_rate,
> +       .determine_rate = imx8m_divider_determine_rate,
>   };
> 
>   static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> --
> 2.39.2
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-23  2:32   ` Peng Fan
  0 siblings, 0 replies; 28+ messages in thread
From: Peng Fan @ 2023-05-23  2:32 UTC (permalink / raw)
  To: Adam Ford, linux-clk
  Cc: aford, Abel Vesa, Peng Fan, Michael Turquette, Stephen Boyd,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list



On 5/7/2023 3:53 AM, Adam Ford wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Currently, certain clocks are derrived as a divider from their
> parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> is set, the parent clock is not properly set which can lead
> to some relatively inaccurate clock values.
> 
> Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> cannot rely on calling a standard determine_rate function,
> because the 8m composite clocks have a pre-divider and
> post-divider. Because of this, a custom determine_rate
> function is necessary to determine the maximum clock
> division which is equivalent to pre-divider * the
> post-divider.
> 
> With this added, the system can attempt to adjust the parent rate
> when the proper flags are set which can lead to a more precise clock
> value.
> 
> On the imx8mplus, no clock changes are present.
> On the Mini and Nano, this can help achieve more accurate
> lcdif clocks. When trying to get a pixel clock of 31.500MHz
> on an imx8m Nano, the clocks divided the 594MHz down, but
> left the parent rate untouched which caused a calulation error.

Not all clocks has pre/post div both.

If CLK_SET_RATE_PARENT not set, would there be any issues for
other clocks?

Regards,
Peng.

> 
> Before:
> video_pll              594000000
>    video_pll_bypass     594000000
>      video_pll_out      594000000
>        disp_pixel       31263158
>          disp_pixel_clk 31263158
> 
> Variance = -236842 Hz
> 
> After this patch:
> video_pll               31500000
>    video_pll_bypass      31500000
>      video_pll_out       31500000
>        disp_pixel        31500000
>          disp_pixel_clk  31500000
> 
> Variance = 0 Hz
> 
> All other clocks rates and parent were the same.
> Similar results on imx8mm were found.
> 
> Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> V2:  Fix build warning found by build bot and fix prediv_value
>       and div_value because the values stored are the divisor - 1,
>       so we need to add 1 to the values to be correct.
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index cbf0d7955a00..7a6e3ce97133 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
>          return ret;
>   }
> 
> +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> +                                     struct clk_rate_request *req)
> +{
> +       struct clk_divider *divider = to_clk_divider(hw);
> +       int prediv_value;
> +       int div_value;
> +
> +       /* if read only, just return current value */
> +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> +               u32 val;
> +
> +               val = readl(divider->reg);
> +               prediv_value = val >> divider->shift;
> +               prediv_value &= clk_div_mask(divider->width);
> +               prediv_value++;
> +
> +               div_value = val >> PCG_DIV_SHIFT;
> +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> +               div_value++;
> +
> +               return divider_ro_determine_rate(hw, req, divider->table,
> +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +                                                divider->flags, prediv_value * div_value);
> +       }
> +
> +       return divider_determine_rate(hw, req, divider->table,
> +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +                                     divider->flags);
> +}
> +
>   static const struct clk_ops imx8m_clk_composite_divider_ops = {
>          .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
>          .round_rate = imx8m_clk_composite_divider_round_rate,
>          .set_rate = imx8m_clk_composite_divider_set_rate,
> +       .determine_rate = imx8m_divider_determine_rate,
>   };
> 
>   static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> --
> 2.39.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-23  2:32   ` Peng Fan
@ 2023-05-23  3:23     ` Adam Ford
  -1 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-23  3:23 UTC (permalink / raw)
  To: Peng Fan
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Mon, May 22, 2023 at 9:33 PM Peng Fan <peng.fan@oss.nxp.com> wrote:
>
>
>
> On 5/7/2023 3:53 AM, Adam Ford wrote:
> > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> >
> >
> > Currently, certain clocks are derrived as a divider from their
> > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > is set, the parent clock is not properly set which can lead
> > to some relatively inaccurate clock values.
> >
> > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > cannot rely on calling a standard determine_rate function,
> > because the 8m composite clocks have a pre-divider and
> > post-divider. Because of this, a custom determine_rate
> > function is necessary to determine the maximum clock
> > division which is equivalent to pre-divider * the
> > post-divider.
> >
> > With this added, the system can attempt to adjust the parent rate
> > when the proper flags are set which can lead to a more precise clock
> > value.
> >
> > On the imx8mplus, no clock changes are present.
> > On the Mini and Nano, this can help achieve more accurate
> > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > on an imx8m Nano, the clocks divided the 594MHz down, but
> > left the parent rate untouched which caused a calulation error.
>
> Not all clocks has pre/post div both.
>
> If CLK_SET_RATE_PARENT not set, would there be any issues for
> other clocks?

I did a dump of the clk_summary for Mini, Nano and Plus, and I found
no changes to any clock other than the video_pll, and most of the
clocks do not have CLK_SET_RATE_PARENT set, so from what I could tell
it seemed harmless.

>
> Regards,
> Peng.
>
> >
> > Before:
> > video_pll              594000000
> >    video_pll_bypass     594000000
> >      video_pll_out      594000000
> >        disp_pixel       31263158
> >          disp_pixel_clk 31263158
> >
> > Variance = -236842 Hz
> >
> > After this patch:
> > video_pll               31500000
> >    video_pll_bypass      31500000
> >      video_pll_out       31500000
> >        disp_pixel        31500000
> >          disp_pixel_clk  31500000
> >
> > Variance = 0 Hz
> >
> > All other clocks rates and parent were the same.
> > Similar results on imx8mm were found.
> >
> > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > V2:  Fix build warning found by build bot and fix prediv_value
> >       and div_value because the values stored are the divisor - 1,
> >       so we need to add 1 to the values to be correct.
> >
> > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> > index cbf0d7955a00..7a6e3ce97133 100644
> > --- a/drivers/clk/imx/clk-composite-8m.c
> > +++ b/drivers/clk/imx/clk-composite-8m.c
> > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> >          return ret;
> >   }
> >
> > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > +                                     struct clk_rate_request *req)
> > +{
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       int prediv_value;
> > +       int div_value;
> > +
> > +       /* if read only, just return current value */
> > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > +               u32 val;
> > +
> > +               val = readl(divider->reg);
> > +               prediv_value = val >> divider->shift;
> > +               prediv_value &= clk_div_mask(divider->width);
> > +               prediv_value++;
> > +
> > +               div_value = val >> PCG_DIV_SHIFT;
> > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > +               div_value++;
> > +
> > +               return divider_ro_determine_rate(hw, req, divider->table,
> > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                                divider->flags, prediv_value * div_value);
> > +       }
> > +
> > +       return divider_determine_rate(hw, req, divider->table,
> > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                     divider->flags);
> > +}
> > +
> >   static const struct clk_ops imx8m_clk_composite_divider_ops = {
> >          .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> >          .round_rate = imx8m_clk_composite_divider_round_rate,
> >          .set_rate = imx8m_clk_composite_divider_set_rate,
> > +       .determine_rate = imx8m_divider_determine_rate,
> >   };
> >
> >   static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > --
> > 2.39.2
> >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-23  3:23     ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-23  3:23 UTC (permalink / raw)
  To: Peng Fan
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Mon, May 22, 2023 at 9:33 PM Peng Fan <peng.fan@oss.nxp.com> wrote:
>
>
>
> On 5/7/2023 3:53 AM, Adam Ford wrote:
> > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> >
> >
> > Currently, certain clocks are derrived as a divider from their
> > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > is set, the parent clock is not properly set which can lead
> > to some relatively inaccurate clock values.
> >
> > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > cannot rely on calling a standard determine_rate function,
> > because the 8m composite clocks have a pre-divider and
> > post-divider. Because of this, a custom determine_rate
> > function is necessary to determine the maximum clock
> > division which is equivalent to pre-divider * the
> > post-divider.
> >
> > With this added, the system can attempt to adjust the parent rate
> > when the proper flags are set which can lead to a more precise clock
> > value.
> >
> > On the imx8mplus, no clock changes are present.
> > On the Mini and Nano, this can help achieve more accurate
> > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > on an imx8m Nano, the clocks divided the 594MHz down, but
> > left the parent rate untouched which caused a calulation error.
>
> Not all clocks has pre/post div both.
>
> If CLK_SET_RATE_PARENT not set, would there be any issues for
> other clocks?

I did a dump of the clk_summary for Mini, Nano and Plus, and I found
no changes to any clock other than the video_pll, and most of the
clocks do not have CLK_SET_RATE_PARENT set, so from what I could tell
it seemed harmless.

>
> Regards,
> Peng.
>
> >
> > Before:
> > video_pll              594000000
> >    video_pll_bypass     594000000
> >      video_pll_out      594000000
> >        disp_pixel       31263158
> >          disp_pixel_clk 31263158
> >
> > Variance = -236842 Hz
> >
> > After this patch:
> > video_pll               31500000
> >    video_pll_bypass      31500000
> >      video_pll_out       31500000
> >        disp_pixel        31500000
> >          disp_pixel_clk  31500000
> >
> > Variance = 0 Hz
> >
> > All other clocks rates and parent were the same.
> > Similar results on imx8mm were found.
> >
> > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > V2:  Fix build warning found by build bot and fix prediv_value
> >       and div_value because the values stored are the divisor - 1,
> >       so we need to add 1 to the values to be correct.
> >
> > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> > index cbf0d7955a00..7a6e3ce97133 100644
> > --- a/drivers/clk/imx/clk-composite-8m.c
> > +++ b/drivers/clk/imx/clk-composite-8m.c
> > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> >          return ret;
> >   }
> >
> > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > +                                     struct clk_rate_request *req)
> > +{
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       int prediv_value;
> > +       int div_value;
> > +
> > +       /* if read only, just return current value */
> > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > +               u32 val;
> > +
> > +               val = readl(divider->reg);
> > +               prediv_value = val >> divider->shift;
> > +               prediv_value &= clk_div_mask(divider->width);
> > +               prediv_value++;
> > +
> > +               div_value = val >> PCG_DIV_SHIFT;
> > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > +               div_value++;
> > +
> > +               return divider_ro_determine_rate(hw, req, divider->table,
> > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                                divider->flags, prediv_value * div_value);
> > +       }
> > +
> > +       return divider_determine_rate(hw, req, divider->table,
> > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > +                                     divider->flags);
> > +}
> > +
> >   static const struct clk_ops imx8m_clk_composite_divider_ops = {
> >          .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> >          .round_rate = imx8m_clk_composite_divider_round_rate,
> >          .set_rate = imx8m_clk_composite_divider_set_rate,
> > +       .determine_rate = imx8m_divider_determine_rate,
> >   };
> >
> >   static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > --
> > 2.39.2
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-23  3:23     ` Adam Ford
@ 2023-05-28  3:31       ` Adam Ford
  -1 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-28  3:31 UTC (permalink / raw)
  To: Peng Fan
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Mon, May 22, 2023 at 10:23 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, May 22, 2023 at 9:33 PM Peng Fan <peng.fan@oss.nxp.com> wrote:
> >
> >
> >
> > On 5/7/2023 3:53 AM, Adam Ford wrote:
> > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> > >
> > >
> > > Currently, certain clocks are derrived as a divider from their
> > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > is set, the parent clock is not properly set which can lead
> > > to some relatively inaccurate clock values.
> > >
> > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > cannot rely on calling a standard determine_rate function,
> > > because the 8m composite clocks have a pre-divider and
> > > post-divider. Because of this, a custom determine_rate
> > > function is necessary to determine the maximum clock
> > > division which is equivalent to pre-divider * the
> > > post-divider.
> > >
> > > With this added, the system can attempt to adjust the parent rate
> > > when the proper flags are set which can lead to a more precise clock
> > > value.
> > >
> > > On the imx8mplus, no clock changes are present.
> > > On the Mini and Nano, this can help achieve more accurate
> > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > left the parent rate untouched which caused a calulation error.
> >
> > Not all clocks has pre/post div both.

Peng,

Any suggestions on how to identify this?  looking at the code that
sets the clock rates, it seemed to me like the code was mostly common
code and it looked like the pre and post div's were basically the
same.  CAn you point me to an example where they're identified with
different values?
> >
> > If CLK_SET_RATE_PARENT not set, would there be any issues for
> > other clocks?
>
> I did a dump of the clk_summary for Mini, Nano and Plus, and I found
> no changes to any clock other than the video_pll, and most of the
> clocks do not have CLK_SET_RATE_PARENT set, so from what I could tell
> it seemed harmless.

What do you need from me to be able to move this forward?

thanks

adam
>
> >
> > Regards,
> > Peng.
> >
> > >
> > > Before:
> > > video_pll              594000000
> > >    video_pll_bypass     594000000
> > >      video_pll_out      594000000
> > >        disp_pixel       31263158
> > >          disp_pixel_clk 31263158
> > >
> > > Variance = -236842 Hz
> > >
> > > After this patch:
> > > video_pll               31500000
> > >    video_pll_bypass      31500000
> > >      video_pll_out       31500000
> > >        disp_pixel        31500000
> > >          disp_pixel_clk  31500000
> > >
> > > Variance = 0 Hz
> > >
> > > All other clocks rates and parent were the same.
> > > Similar results on imx8mm were found.
> > >
> > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > > V2:  Fix build warning found by build bot and fix prediv_value
> > >       and div_value because the values stored are the divisor - 1,
> > >       so we need to add 1 to the values to be correct.
> > >
> > > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> > > index cbf0d7955a00..7a6e3ce97133 100644
> > > --- a/drivers/clk/imx/clk-composite-8m.c
> > > +++ b/drivers/clk/imx/clk-composite-8m.c
> > > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> > >          return ret;
> > >   }
> > >
> > > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > > +                                     struct clk_rate_request *req)
> > > +{
> > > +       struct clk_divider *divider = to_clk_divider(hw);
> > > +       int prediv_value;
> > > +       int div_value;
> > > +
> > > +       /* if read only, just return current value */
> > > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > > +               u32 val;
> > > +
> > > +               val = readl(divider->reg);
> > > +               prediv_value = val >> divider->shift;
> > > +               prediv_value &= clk_div_mask(divider->width);
> > > +               prediv_value++;
> > > +
> > > +               div_value = val >> PCG_DIV_SHIFT;
> > > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > > +               div_value++;
> > > +
> > > +               return divider_ro_determine_rate(hw, req, divider->table,
> > > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                                divider->flags, prediv_value * div_value);
> > > +       }
> > > +
> > > +       return divider_determine_rate(hw, req, divider->table,
> > > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                     divider->flags);
> > > +}
> > > +
> > >   static const struct clk_ops imx8m_clk_composite_divider_ops = {
> > >          .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> > >          .round_rate = imx8m_clk_composite_divider_round_rate,
> > >          .set_rate = imx8m_clk_composite_divider_set_rate,
> > > +       .determine_rate = imx8m_divider_determine_rate,
> > >   };
> > >
> > >   static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > > --
> > > 2.39.2
> > >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-28  3:31       ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-28  3:31 UTC (permalink / raw)
  To: Peng Fan
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Mon, May 22, 2023 at 10:23 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, May 22, 2023 at 9:33 PM Peng Fan <peng.fan@oss.nxp.com> wrote:
> >
> >
> >
> > On 5/7/2023 3:53 AM, Adam Ford wrote:
> > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> > >
> > >
> > > Currently, certain clocks are derrived as a divider from their
> > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > is set, the parent clock is not properly set which can lead
> > > to some relatively inaccurate clock values.
> > >
> > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > cannot rely on calling a standard determine_rate function,
> > > because the 8m composite clocks have a pre-divider and
> > > post-divider. Because of this, a custom determine_rate
> > > function is necessary to determine the maximum clock
> > > division which is equivalent to pre-divider * the
> > > post-divider.
> > >
> > > With this added, the system can attempt to adjust the parent rate
> > > when the proper flags are set which can lead to a more precise clock
> > > value.
> > >
> > > On the imx8mplus, no clock changes are present.
> > > On the Mini and Nano, this can help achieve more accurate
> > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > left the parent rate untouched which caused a calulation error.
> >
> > Not all clocks has pre/post div both.

Peng,

Any suggestions on how to identify this?  looking at the code that
sets the clock rates, it seemed to me like the code was mostly common
code and it looked like the pre and post div's were basically the
same.  CAn you point me to an example where they're identified with
different values?
> >
> > If CLK_SET_RATE_PARENT not set, would there be any issues for
> > other clocks?
>
> I did a dump of the clk_summary for Mini, Nano and Plus, and I found
> no changes to any clock other than the video_pll, and most of the
> clocks do not have CLK_SET_RATE_PARENT set, so from what I could tell
> it seemed harmless.

What do you need from me to be able to move this forward?

thanks

adam
>
> >
> > Regards,
> > Peng.
> >
> > >
> > > Before:
> > > video_pll              594000000
> > >    video_pll_bypass     594000000
> > >      video_pll_out      594000000
> > >        disp_pixel       31263158
> > >          disp_pixel_clk 31263158
> > >
> > > Variance = -236842 Hz
> > >
> > > After this patch:
> > > video_pll               31500000
> > >    video_pll_bypass      31500000
> > >      video_pll_out       31500000
> > >        disp_pixel        31500000
> > >          disp_pixel_clk  31500000
> > >
> > > Variance = 0 Hz
> > >
> > > All other clocks rates and parent were the same.
> > > Similar results on imx8mm were found.
> > >
> > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > > V2:  Fix build warning found by build bot and fix prediv_value
> > >       and div_value because the values stored are the divisor - 1,
> > >       so we need to add 1 to the values to be correct.
> > >
> > > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> > > index cbf0d7955a00..7a6e3ce97133 100644
> > > --- a/drivers/clk/imx/clk-composite-8m.c
> > > +++ b/drivers/clk/imx/clk-composite-8m.c
> > > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
> > >          return ret;
> > >   }
> > >
> > > +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> > > +                                     struct clk_rate_request *req)
> > > +{
> > > +       struct clk_divider *divider = to_clk_divider(hw);
> > > +       int prediv_value;
> > > +       int div_value;
> > > +
> > > +       /* if read only, just return current value */
> > > +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > > +               u32 val;
> > > +
> > > +               val = readl(divider->reg);
> > > +               prediv_value = val >> divider->shift;
> > > +               prediv_value &= clk_div_mask(divider->width);
> > > +               prediv_value++;
> > > +
> > > +               div_value = val >> PCG_DIV_SHIFT;
> > > +               div_value &= clk_div_mask(PCG_DIV_WIDTH);
> > > +               div_value++;
> > > +
> > > +               return divider_ro_determine_rate(hw, req, divider->table,
> > > +                                                PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                                divider->flags, prediv_value * div_value);
> > > +       }
> > > +
> > > +       return divider_determine_rate(hw, req, divider->table,
> > > +                                     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> > > +                                     divider->flags);
> > > +}
> > > +
> > >   static const struct clk_ops imx8m_clk_composite_divider_ops = {
> > >          .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> > >          .round_rate = imx8m_clk_composite_divider_round_rate,
> > >          .set_rate = imx8m_clk_composite_divider_set_rate,
> > > +       .determine_rate = imx8m_divider_determine_rate,
> > >   };
> > >
> > >   static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> > > --
> > > 2.39.2
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-06 19:53 ` Adam Ford
@ 2023-06-06 18:45   ` Fabio Estevam
  -1 siblings, 0 replies; 28+ messages in thread
From: Fabio Estevam @ 2023-06-06 18:45 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
>
> Currently, certain clocks are derrived as a divider from their
> parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> is set, the parent clock is not properly set which can lead
> to some relatively inaccurate clock values.
>
> Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> cannot rely on calling a standard determine_rate function,
> because the 8m composite clocks have a pre-divider and
> post-divider. Because of this, a custom determine_rate
> function is necessary to determine the maximum clock
> division which is equivalent to pre-divider * the
> post-divider.
>
> With this added, the system can attempt to adjust the parent rate
> when the proper flags are set which can lead to a more precise clock
> value.
>
> On the imx8mplus, no clock changes are present.
> On the Mini and Nano, this can help achieve more accurate
> lcdif clocks. When trying to get a pixel clock of 31.500MHz
> on an imx8m Nano, the clocks divided the 594MHz down, but
> left the parent rate untouched which caused a calulation error.
>
> Before:
> video_pll              594000000
>   video_pll_bypass     594000000
>     video_pll_out      594000000
>       disp_pixel       31263158
>         disp_pixel_clk 31263158
>
> Variance = -236842 Hz
>
> After this patch:
> video_pll               31500000
>   video_pll_bypass      31500000
>     video_pll_out       31500000
>       disp_pixel        31500000
>         disp_pixel_clk  31500000
>
> Variance = 0 Hz
>
> All other clocks rates and parent were the same.
> Similar results on imx8mm were found.
>
> Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> Signed-off-by: Adam Ford <aford173@gmail.com>

This works fine on my imx8mm-evk, so:

Tested-by: Fabio Estevam <festevam@gmail.com>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-06-06 18:45   ` Fabio Estevam
  0 siblings, 0 replies; 28+ messages in thread
From: Fabio Estevam @ 2023-06-06 18:45 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
>
> Currently, certain clocks are derrived as a divider from their
> parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> is set, the parent clock is not properly set which can lead
> to some relatively inaccurate clock values.
>
> Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> cannot rely on calling a standard determine_rate function,
> because the 8m composite clocks have a pre-divider and
> post-divider. Because of this, a custom determine_rate
> function is necessary to determine the maximum clock
> division which is equivalent to pre-divider * the
> post-divider.
>
> With this added, the system can attempt to adjust the parent rate
> when the proper flags are set which can lead to a more precise clock
> value.
>
> On the imx8mplus, no clock changes are present.
> On the Mini and Nano, this can help achieve more accurate
> lcdif clocks. When trying to get a pixel clock of 31.500MHz
> on an imx8m Nano, the clocks divided the 594MHz down, but
> left the parent rate untouched which caused a calulation error.
>
> Before:
> video_pll              594000000
>   video_pll_bypass     594000000
>     video_pll_out      594000000
>       disp_pixel       31263158
>         disp_pixel_clk 31263158
>
> Variance = -236842 Hz
>
> After this patch:
> video_pll               31500000
>   video_pll_bypass      31500000
>     video_pll_out       31500000
>       disp_pixel        31500000
>         disp_pixel_clk  31500000
>
> Variance = 0 Hz
>
> All other clocks rates and parent were the same.
> Similar results on imx8mm were found.
>
> Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> Signed-off-by: Adam Ford <aford173@gmail.com>

This works fine on my imx8mm-evk, so:

Tested-by: Fabio Estevam <festevam@gmail.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-06-06 18:45   ` Fabio Estevam
  (?)
@ 2023-06-11 17:02   ` Adam Ford
  2023-06-12 16:08       ` Maxime Ripard
  -1 siblings, 1 reply; 28+ messages in thread
From: Adam Ford @ 2023-06-11 17:02 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list, Maxime Ripard

On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > Currently, certain clocks are derrived as a divider from their
> > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > is set, the parent clock is not properly set which can lead
> > to some relatively inaccurate clock values.
> >

+ Maxime

> > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > cannot rely on calling a standard determine_rate function,
> > because the 8m composite clocks have a pre-divider and
> > post-divider. Because of this, a custom determine_rate
> > function is necessary to determine the maximum clock
> > division which is equivalent to pre-divider * the
> > post-divider.
> >
> > With this added, the system can attempt to adjust the parent rate
> > when the proper flags are set which can lead to a more precise clock
> > value.
> >
> > On the imx8mplus, no clock changes are present.
> > On the Mini and Nano, this can help achieve more accurate
> > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > on an imx8m Nano, the clocks divided the 594MHz down, but
> > left the parent rate untouched which caused a calulation error.
> >
> > Before:
> > video_pll              594000000
> >   video_pll_bypass     594000000
> >     video_pll_out      594000000
> >       disp_pixel       31263158
> >         disp_pixel_clk 31263158
> >
> > Variance = -236842 Hz
> >
> > After this patch:
> > video_pll               31500000
> >   video_pll_bypass      31500000
> >     video_pll_out       31500000
> >       disp_pixel        31500000
> >         disp_pixel_clk  31500000
> >
> > Variance = 0 Hz
> >
> > All other clocks rates and parent were the same.
> > Similar results on imx8mm were found.
> >
> > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
>

Peng / Abel,

Any suggestions on how we can move this forward?  Looking at the
clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the
max values which is basically what my patch does.  There was some
discussion about making determine_rate mandatory for muxes[1] and this
patch should help with this in addition to making it easier to sync
more video resolutions on the 8m Mini and Nano.

adam
[1] - https://www.spinics.net/lists/alsa-devel/msg157914.html


> This works fine on my imx8mm-evk, so:
>
> Tested-by: Fabio Estevam <festevam@gmail.com>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-06 19:53 ` Adam Ford
                   ` (3 preceding siblings ...)
  (?)
@ 2023-06-12  8:56 ` Abel Vesa
  -1 siblings, 0 replies; 28+ messages in thread
From: Abel Vesa @ 2023-06-12  8:56 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-clk, aford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On 23-05-06 14:53:25, Adam Ford wrote:
> Currently, certain clocks are derrived as a divider from their
> parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> is set, the parent clock is not properly set which can lead
> to some relatively inaccurate clock values.
> 
> Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> cannot rely on calling a standard determine_rate function,
> because the 8m composite clocks have a pre-divider and
> post-divider. Because of this, a custom determine_rate
> function is necessary to determine the maximum clock
> division which is equivalent to pre-divider * the
> post-divider.
> 
> With this added, the system can attempt to adjust the parent rate
> when the proper flags are set which can lead to a more precise clock
> value.
> 
> On the imx8mplus, no clock changes are present.
> On the Mini and Nano, this can help achieve more accurate
> lcdif clocks. When trying to get a pixel clock of 31.500MHz
> on an imx8m Nano, the clocks divided the 594MHz down, but
> left the parent rate untouched which caused a calulation error.
> 
> Before:
> video_pll              594000000
>   video_pll_bypass     594000000
>     video_pll_out      594000000
>       disp_pixel       31263158
>         disp_pixel_clk 31263158
> 
> Variance = -236842 Hz
> 
> After this patch:
> video_pll               31500000
>   video_pll_bypass      31500000
>     video_pll_out       31500000
>       disp_pixel        31500000
>         disp_pixel_clk  31500000
> 
> Variance = 0 Hz
> 
> All other clocks rates and parent were the same.
> Similar results on imx8mm were found.
> 
> Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
> V2:  Fix build warning found by build bot and fix prediv_value
>      and div_value because the values stored are the divisor - 1,
>      so we need to add 1 to the values to be correct.
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index cbf0d7955a00..7a6e3ce97133 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
>  	return ret;
>  }
>  
> +static int imx8m_divider_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
> +	struct clk_divider *divider = to_clk_divider(hw);
> +	int prediv_value;
> +	int div_value;
> +
> +	/* if read only, just return current value */
> +	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> +		u32 val;
> +
> +		val = readl(divider->reg);
> +		prediv_value = val >> divider->shift;
> +		prediv_value &= clk_div_mask(divider->width);
> +		prediv_value++;
> +
> +		div_value = val >> PCG_DIV_SHIFT;
> +		div_value &= clk_div_mask(PCG_DIV_WIDTH);
> +		div_value++;
> +
> +		return divider_ro_determine_rate(hw, req, divider->table,
> +						 PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +						 divider->flags, prediv_value * div_value);
> +	}
> +
> +	return divider_determine_rate(hw, req, divider->table,
> +				      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
> +				      divider->flags);
> +}
> +
>  static const struct clk_ops imx8m_clk_composite_divider_ops = {
>  	.recalc_rate = imx8m_clk_composite_divider_recalc_rate,
>  	.round_rate = imx8m_clk_composite_divider_round_rate,
>  	.set_rate = imx8m_clk_composite_divider_set_rate,
> +	.determine_rate = imx8m_divider_determine_rate,
>  };
>  
>  static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
> -- 
> 2.39.2
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-06 19:53 ` Adam Ford
                   ` (4 preceding siblings ...)
  (?)
@ 2023-06-12  9:32 ` Abel Vesa
  -1 siblings, 0 replies; 28+ messages in thread
From: Abel Vesa @ 2023-06-12  9:32 UTC (permalink / raw)
  To: linux-clk, Adam Ford
  Cc: aford, Abel Vesa, Peng Fan, Michael Turquette, Stephen Boyd,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel, linux-kernel


On Sat, 06 May 2023 14:53:25 -0500, Adam Ford wrote:
> Currently, certain clocks are derrived as a divider from their
> parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> is set, the parent clock is not properly set which can lead
> to some relatively inaccurate clock values.
> 
> Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> cannot rely on calling a standard determine_rate function,
> because the 8m composite clocks have a pre-divider and
> post-divider. Because of this, a custom determine_rate
> function is necessary to determine the maximum clock
> division which is equivalent to pre-divider * the
> post-divider.
> 
> [...]

Applied, thanks!

[1/1] clk: imx: composite-8m: Add imx8m_divider_determine_rate
      commit: 8208181fe536bba3b411508f81c4426fc9c71d9a

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-06-11 17:02   ` Adam Ford
@ 2023-06-12 16:08       ` Maxime Ripard
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2023-06-12 16:08 UTC (permalink / raw)
  To: Adam Ford
  Cc: Fabio Estevam, linux-clk, aford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

[-- Attachment #1: Type: text/plain, Size: 2684 bytes --]

On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote:
> On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > Currently, certain clocks are derrived as a divider from their
> > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > is set, the parent clock is not properly set which can lead
> > > to some relatively inaccurate clock values.
> > >
> 
> + Maxime
> 
> > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > cannot rely on calling a standard determine_rate function,
> > > because the 8m composite clocks have a pre-divider and
> > > post-divider. Because of this, a custom determine_rate
> > > function is necessary to determine the maximum clock
> > > division which is equivalent to pre-divider * the
> > > post-divider.
> > >
> > > With this added, the system can attempt to adjust the parent rate
> > > when the proper flags are set which can lead to a more precise clock
> > > value.
> > >
> > > On the imx8mplus, no clock changes are present.
> > > On the Mini and Nano, this can help achieve more accurate
> > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > left the parent rate untouched which caused a calulation error.
> > >
> > > Before:
> > > video_pll              594000000
> > >   video_pll_bypass     594000000
> > >     video_pll_out      594000000
> > >       disp_pixel       31263158
> > >         disp_pixel_clk 31263158
> > >
> > > Variance = -236842 Hz
> > >
> > > After this patch:
> > > video_pll               31500000
> > >   video_pll_bypass      31500000
> > >     video_pll_out       31500000
> > >       disp_pixel        31500000
> > >         disp_pixel_clk  31500000
> > >
> > > Variance = 0 Hz
> > >
> > > All other clocks rates and parent were the same.
> > > Similar results on imx8mm were found.
> > >
> > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> 
> Peng / Abel,
> 
> Any suggestions on how we can move this forward?  Looking at the
> clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the
> max values which is basically what my patch does.  There was some
> discussion about making determine_rate mandatory for muxes[1] and this
> patch should help with this in addition to making it easier to sync
> more video resolutions on the 8m Mini and Nano.

Those patches have been queued by Stephen for 6.6 :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-06-12 16:08       ` Maxime Ripard
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2023-06-12 16:08 UTC (permalink / raw)
  To: Adam Ford
  Cc: Fabio Estevam, linux-clk, aford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list


[-- Attachment #1.1: Type: text/plain, Size: 2684 bytes --]

On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote:
> On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > Currently, certain clocks are derrived as a divider from their
> > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > is set, the parent clock is not properly set which can lead
> > > to some relatively inaccurate clock values.
> > >
> 
> + Maxime
> 
> > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > cannot rely on calling a standard determine_rate function,
> > > because the 8m composite clocks have a pre-divider and
> > > post-divider. Because of this, a custom determine_rate
> > > function is necessary to determine the maximum clock
> > > division which is equivalent to pre-divider * the
> > > post-divider.
> > >
> > > With this added, the system can attempt to adjust the parent rate
> > > when the proper flags are set which can lead to a more precise clock
> > > value.
> > >
> > > On the imx8mplus, no clock changes are present.
> > > On the Mini and Nano, this can help achieve more accurate
> > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > left the parent rate untouched which caused a calulation error.
> > >
> > > Before:
> > > video_pll              594000000
> > >   video_pll_bypass     594000000
> > >     video_pll_out      594000000
> > >       disp_pixel       31263158
> > >         disp_pixel_clk 31263158
> > >
> > > Variance = -236842 Hz
> > >
> > > After this patch:
> > > video_pll               31500000
> > >   video_pll_bypass      31500000
> > >     video_pll_out       31500000
> > >       disp_pixel        31500000
> > >         disp_pixel_clk  31500000
> > >
> > > Variance = 0 Hz
> > >
> > > All other clocks rates and parent were the same.
> > > Similar results on imx8mm were found.
> > >
> > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> 
> Peng / Abel,
> 
> Any suggestions on how we can move this forward?  Looking at the
> clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the
> max values which is basically what my patch does.  There was some
> discussion about making determine_rate mandatory for muxes[1] and this
> patch should help with this in addition to making it easier to sync
> more video resolutions on the 8m Mini and Nano.

Those patches have been queued by Stephen for 6.6 :)

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-06-12 16:08       ` Maxime Ripard
@ 2023-06-12 16:11         ` Adam Ford
  -1 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-06-12 16:11 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Fabio Estevam, linux-clk, aford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Mon, Jun 12, 2023 at 11:08 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote:
> > On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > Currently, certain clocks are derrived as a divider from their
> > > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > > is set, the parent clock is not properly set which can lead
> > > > to some relatively inaccurate clock values.
> > > >
> >
> > + Maxime
> >
> > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > > cannot rely on calling a standard determine_rate function,
> > > > because the 8m composite clocks have a pre-divider and
> > > > post-divider. Because of this, a custom determine_rate
> > > > function is necessary to determine the maximum clock
> > > > division which is equivalent to pre-divider * the
> > > > post-divider.
> > > >
> > > > With this added, the system can attempt to adjust the parent rate
> > > > when the proper flags are set which can lead to a more precise clock
> > > > value.
> > > >
> > > > On the imx8mplus, no clock changes are present.
> > > > On the Mini and Nano, this can help achieve more accurate
> > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > > left the parent rate untouched which caused a calulation error.
> > > >
> > > > Before:
> > > > video_pll              594000000
> > > >   video_pll_bypass     594000000
> > > >     video_pll_out      594000000
> > > >       disp_pixel       31263158
> > > >         disp_pixel_clk 31263158
> > > >
> > > > Variance = -236842 Hz
> > > >
> > > > After this patch:
> > > > video_pll               31500000
> > > >   video_pll_bypass      31500000
> > > >     video_pll_out       31500000
> > > >       disp_pixel        31500000
> > > >         disp_pixel_clk  31500000
> > > >
> > > > Variance = 0 Hz
> > > >
> > > > All other clocks rates and parent were the same.
> > > > Similar results on imx8mm were found.
> > > >
> > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> >
> > Peng / Abel,
> >
> > Any suggestions on how we can move this forward?  Looking at the
> > clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the
> > max values which is basically what my patch does.  There was some
> > discussion about making determine_rate mandatory for muxes[1] and this
> > patch should help with this in addition to making it easier to sync
> > more video resolutions on the 8m Mini and Nano.
>
> Those patches have been queued by Stephen for 6.6 :)

One of the patches in the older series was reverted, but this was to
address the patch that was reverted.

adam
>
> Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-06-12 16:11         ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-06-12 16:11 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Fabio Estevam, linux-clk, aford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

On Mon, Jun 12, 2023 at 11:08 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote:
> > On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > Currently, certain clocks are derrived as a divider from their
> > > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > > is set, the parent clock is not properly set which can lead
> > > > to some relatively inaccurate clock values.
> > > >
> >
> > + Maxime
> >
> > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > > cannot rely on calling a standard determine_rate function,
> > > > because the 8m composite clocks have a pre-divider and
> > > > post-divider. Because of this, a custom determine_rate
> > > > function is necessary to determine the maximum clock
> > > > division which is equivalent to pre-divider * the
> > > > post-divider.
> > > >
> > > > With this added, the system can attempt to adjust the parent rate
> > > > when the proper flags are set which can lead to a more precise clock
> > > > value.
> > > >
> > > > On the imx8mplus, no clock changes are present.
> > > > On the Mini and Nano, this can help achieve more accurate
> > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > > left the parent rate untouched which caused a calulation error.
> > > >
> > > > Before:
> > > > video_pll              594000000
> > > >   video_pll_bypass     594000000
> > > >     video_pll_out      594000000
> > > >       disp_pixel       31263158
> > > >         disp_pixel_clk 31263158
> > > >
> > > > Variance = -236842 Hz
> > > >
> > > > After this patch:
> > > > video_pll               31500000
> > > >   video_pll_bypass      31500000
> > > >     video_pll_out       31500000
> > > >       disp_pixel        31500000
> > > >         disp_pixel_clk  31500000
> > > >
> > > > Variance = 0 Hz
> > > >
> > > > All other clocks rates and parent were the same.
> > > > Similar results on imx8mm were found.
> > > >
> > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> >
> > Peng / Abel,
> >
> > Any suggestions on how we can move this forward?  Looking at the
> > clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the
> > max values which is basically what my patch does.  There was some
> > discussion about making determine_rate mandatory for muxes[1] and this
> > patch should help with this in addition to making it easier to sync
> > more video resolutions on the 8m Mini and Nano.
>
> Those patches have been queued by Stephen for 6.6 :)

One of the patches in the older series was reverted, but this was to
address the patch that was reverted.

adam
>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-06-12 16:11         ` Adam Ford
@ 2023-06-13  8:23           ` Maxime Ripard
  -1 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2023-06-13  8:23 UTC (permalink / raw)
  To: Adam Ford
  Cc: Fabio Estevam, linux-clk, aford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

[-- Attachment #1: Type: text/plain, Size: 3277 bytes --]

On Mon, Jun 12, 2023 at 11:11:21AM -0500, Adam Ford wrote:
> On Mon, Jun 12, 2023 at 11:08 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote:
> > > On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote:
> > > >
> > > > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > Currently, certain clocks are derrived as a divider from their
> > > > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > > > is set, the parent clock is not properly set which can lead
> > > > > to some relatively inaccurate clock values.
> > > > >
> > >
> > > + Maxime
> > >
> > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > > > cannot rely on calling a standard determine_rate function,
> > > > > because the 8m composite clocks have a pre-divider and
> > > > > post-divider. Because of this, a custom determine_rate
> > > > > function is necessary to determine the maximum clock
> > > > > division which is equivalent to pre-divider * the
> > > > > post-divider.
> > > > >
> > > > > With this added, the system can attempt to adjust the parent rate
> > > > > when the proper flags are set which can lead to a more precise clock
> > > > > value.
> > > > >
> > > > > On the imx8mplus, no clock changes are present.
> > > > > On the Mini and Nano, this can help achieve more accurate
> > > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > > > left the parent rate untouched which caused a calulation error.
> > > > >
> > > > > Before:
> > > > > video_pll              594000000
> > > > >   video_pll_bypass     594000000
> > > > >     video_pll_out      594000000
> > > > >       disp_pixel       31263158
> > > > >         disp_pixel_clk 31263158
> > > > >
> > > > > Variance = -236842 Hz
> > > > >
> > > > > After this patch:
> > > > > video_pll               31500000
> > > > >   video_pll_bypass      31500000
> > > > >     video_pll_out       31500000
> > > > >       disp_pixel        31500000
> > > > >         disp_pixel_clk  31500000
> > > > >
> > > > > Variance = 0 Hz
> > > > >
> > > > > All other clocks rates and parent were the same.
> > > > > Similar results on imx8mm were found.
> > > > >
> > > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > >
> > > Peng / Abel,
> > >
> > > Any suggestions on how we can move this forward?  Looking at the
> > > clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the
> > > max values which is basically what my patch does.  There was some
> > > discussion about making determine_rate mandatory for muxes[1] and this
> > > patch should help with this in addition to making it easier to sync
> > > more video resolutions on the 8m Mini and Nano.
> >
> > Those patches have been queued by Stephen for 6.6 :)
> 
> One of the patches in the older series was reverted, but this was to
> address the patch that was reverted.

Was it? I haven't been cc'd and it doesn't seem to be in -next either?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-06-13  8:23           ` Maxime Ripard
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2023-06-13  8:23 UTC (permalink / raw)
  To: Adam Ford
  Cc: Fabio Estevam, linux-clk, aford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list


[-- Attachment #1.1: Type: text/plain, Size: 3277 bytes --]

On Mon, Jun 12, 2023 at 11:11:21AM -0500, Adam Ford wrote:
> On Mon, Jun 12, 2023 at 11:08 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote:
> > > On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote:
> > > >
> > > > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > Currently, certain clocks are derrived as a divider from their
> > > > > parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
> > > > > is set, the parent clock is not properly set which can lead
> > > > > to some relatively inaccurate clock values.
> > > > >
> > >
> > > + Maxime
> > >
> > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
> > > > > cannot rely on calling a standard determine_rate function,
> > > > > because the 8m composite clocks have a pre-divider and
> > > > > post-divider. Because of this, a custom determine_rate
> > > > > function is necessary to determine the maximum clock
> > > > > division which is equivalent to pre-divider * the
> > > > > post-divider.
> > > > >
> > > > > With this added, the system can attempt to adjust the parent rate
> > > > > when the proper flags are set which can lead to a more precise clock
> > > > > value.
> > > > >
> > > > > On the imx8mplus, no clock changes are present.
> > > > > On the Mini and Nano, this can help achieve more accurate
> > > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz
> > > > > on an imx8m Nano, the clocks divided the 594MHz down, but
> > > > > left the parent rate untouched which caused a calulation error.
> > > > >
> > > > > Before:
> > > > > video_pll              594000000
> > > > >   video_pll_bypass     594000000
> > > > >     video_pll_out      594000000
> > > > >       disp_pixel       31263158
> > > > >         disp_pixel_clk 31263158
> > > > >
> > > > > Variance = -236842 Hz
> > > > >
> > > > > After this patch:
> > > > > video_pll               31500000
> > > > >   video_pll_bypass      31500000
> > > > >     video_pll_out       31500000
> > > > >       disp_pixel        31500000
> > > > >         disp_pixel_clk  31500000
> > > > >
> > > > > Variance = 0 Hz
> > > > >
> > > > > All other clocks rates and parent were the same.
> > > > > Similar results on imx8mm were found.
> > > > >
> > > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
> > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > >
> > > Peng / Abel,
> > >
> > > Any suggestions on how we can move this forward?  Looking at the
> > > clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the
> > > max values which is basically what my patch does.  There was some
> > > discussion about making determine_rate mandatory for muxes[1] and this
> > > patch should help with this in addition to making it easier to sync
> > > more video resolutions on the 8m Mini and Nano.
> >
> > Those patches have been queued by Stephen for 6.6 :)
> 
> One of the patches in the older series was reverted, but this was to
> address the patch that was reverted.

Was it? I haven't been cc'd and it doesn't seem to be in -next either?

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-06 16:24 ` Adam Ford
  (?)
  (?)
@ 2023-05-06 18:12 ` kernel test robot
  -1 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2023-05-06 18:12 UTC (permalink / raw)
  To: Adam Ford, linux-clk
  Cc: llvm, oe-kbuild-all, aford, Adam Ford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-kernel

Hi Adam,

kernel test robot noticed the following build warnings:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v6.3 next-20230505]
[cannot apply to abelvesa/clk/imx linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Adam-Ford/clk-imx-composite-8m-Add-imx8m_divider_determine_rate/20230507-002453
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20230506162403.375212-1-aford173%40gmail.com
patch subject: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
config: powerpc-randconfig-r012-20230507 (https://download.01.org/0day-ci/archive/20230507/202305070235.r9bEq3U8-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/d92df3b38fc20beb735cb3b75f118b45d1ae304b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Adam-Ford/clk-imx-composite-8m-Add-imx8m_divider_determine_rate/20230507-002453
        git checkout d92df3b38fc20beb735cb3b75f118b45d1ae304b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/clk/imx/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305070235.r9bEq3U8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/clk/imx/clk-composite-8m.c:122:5: warning: no previous prototype for function 'imx8m_divider_determine_rate' [-Wmissing-prototypes]
   int imx8m_divider_determine_rate(struct clk_hw *hw,
       ^
   drivers/clk/imx/clk-composite-8m.c:122:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int imx8m_divider_determine_rate(struct clk_hw *hw,
   ^
   static 
   1 warning generated.


vim +/imx8m_divider_determine_rate +122 drivers/clk/imx/clk-composite-8m.c

   121	
 > 122	int imx8m_divider_determine_rate(struct clk_hw *hw,
   123					      struct clk_rate_request *req)
   124	{
   125		struct clk_divider *divider = to_clk_divider(hw);
   126		int prediv_value;
   127		int div_value;
   128	
   129		/* if read only, just return current value */
   130		if (divider->flags & CLK_DIVIDER_READ_ONLY) {
   131			u32 val;
   132	
   133			val = readl(divider->reg);
   134			prediv_value = val >> divider->shift;
   135			prediv_value &= clk_div_mask(divider->width);
   136	
   137			div_value = val >> PCG_DIV_SHIFT;
   138			div_value &= clk_div_mask(PCG_DIV_WIDTH);
   139	
   140			return divider_ro_determine_rate(hw, req, divider->table,
   141							 PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
   142							 divider->flags, prediv_value * div_value);
   143		}
   144	
   145		return divider_determine_rate(hw, req, divider->table,
   146					      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
   147					      divider->flags);
   148	}
   149	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
  2023-05-06 16:24 ` Adam Ford
  (?)
@ 2023-05-06 17:41 ` kernel test robot
  -1 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2023-05-06 17:41 UTC (permalink / raw)
  To: Adam Ford, linux-clk
  Cc: oe-kbuild-all, aford, Adam Ford, Abel Vesa, Peng Fan,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-kernel

Hi Adam,

kernel test robot noticed the following build warnings:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v6.3 next-20230505]
[cannot apply to abelvesa/clk/imx linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Adam-Ford/clk-imx-composite-8m-Add-imx8m_divider_determine_rate/20230507-002453
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20230506162403.375212-1-aford173%40gmail.com
patch subject: [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230507/202305070143.iTqgbS75-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d92df3b38fc20beb735cb3b75f118b45d1ae304b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Adam-Ford/clk-imx-composite-8m-Add-imx8m_divider_determine_rate/20230507-002453
        git checkout d92df3b38fc20beb735cb3b75f118b45d1ae304b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/clk/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305070143.iTqgbS75-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/clk/imx/clk-composite-8m.c:122:5: warning: no previous prototype for 'imx8m_divider_determine_rate' [-Wmissing-prototypes]
     122 | int imx8m_divider_determine_rate(struct clk_hw *hw,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/imx8m_divider_determine_rate +122 drivers/clk/imx/clk-composite-8m.c

   121	
 > 122	int imx8m_divider_determine_rate(struct clk_hw *hw,
   123					      struct clk_rate_request *req)
   124	{
   125		struct clk_divider *divider = to_clk_divider(hw);
   126		int prediv_value;
   127		int div_value;
   128	
   129		/* if read only, just return current value */
   130		if (divider->flags & CLK_DIVIDER_READ_ONLY) {
   131			u32 val;
   132	
   133			val = readl(divider->reg);
   134			prediv_value = val >> divider->shift;
   135			prediv_value &= clk_div_mask(divider->width);
   136	
   137			div_value = val >> PCG_DIV_SHIFT;
   138			div_value &= clk_div_mask(PCG_DIV_WIDTH);
   139	
   140			return divider_ro_determine_rate(hw, req, divider->table,
   141							 PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
   142							 divider->flags, prediv_value * div_value);
   143		}
   144	
   145		return divider_determine_rate(hw, req, divider->table,
   146					      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
   147					      divider->flags);
   148	}
   149	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-06 16:24 ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-06 16:24 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, Adam Ford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

Currently, certain clocks, are derrived as a divider from their
parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
is set, the parent clock is not which can lead to some relatively
inaccurate clock values for child clocks.

Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
cannot rely on calling a standard clock function, because
the 8m composite clocks have a pre-divider and post-divider.
Because of this, a custom determine_rate function is
necessary to determine the maximum clock division which is
equivalent to pre-divider * the post-divider in most cases.

With this added, the system can attempt to adjust the parent rate
when the proper flags are set which can lead to a more precise clock
value.

For example.  When trying to get a pixel clock of 31.500MHz
on an imx8m Nano, the clocks divided the 594MHz down, but
left the parent rate untouched which caused a calulation error.

Before:
video_pll              594000000
  video_pll_bypass     594000000
    video_pll_out      594000000
      disp_pixel       31263158
        disp_pixel_clk 31263158

Variance = -236842 Hz

After this patch:
video_pll               31500000
  video_pll_bypass      31500000
    video_pll_out       31500000
      disp_pixel        31500000
        disp_pixel_clk  31500000

Variance = 0 Hz

All other clocks rates and parent were the same.
Similar results on imx8mm were found.

Unlike the previous attempt, when tested with imx8mp, no clock
rate or parent changes were found.

Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index cbf0d7955a00..fd49385bd2ae 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -119,10 +119,39 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
 	return ret;
 }
 
+int imx8m_divider_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	int prediv_value;
+	int div_value;
+
+	/* if read only, just return current value */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
+		u32 val;
+
+		val = readl(divider->reg);
+		prediv_value = val >> divider->shift;
+		prediv_value &= clk_div_mask(divider->width);
+
+		div_value = val >> PCG_DIV_SHIFT;
+		div_value &= clk_div_mask(PCG_DIV_WIDTH);
+
+		return divider_ro_determine_rate(hw, req, divider->table,
+						 PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+						 divider->flags, prediv_value * div_value);
+	}
+
+	return divider_determine_rate(hw, req, divider->table,
+				      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+				      divider->flags);
+}
+
 static const struct clk_ops imx8m_clk_composite_divider_ops = {
 	.recalc_rate = imx8m_clk_composite_divider_recalc_rate,
 	.round_rate = imx8m_clk_composite_divider_round_rate,
 	.set_rate = imx8m_clk_composite_divider_set_rate,
+	.determine_rate = imx8m_divider_determine_rate,
 };
 
 static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate
@ 2023-05-06 16:24 ` Adam Ford
  0 siblings, 0 replies; 28+ messages in thread
From: Adam Ford @ 2023-05-06 16:24 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, Adam Ford, Abel Vesa, Peng Fan, Michael Turquette,
	Stephen Boyd, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list

Currently, certain clocks, are derrived as a divider from their
parent clock.  For some clocks, even when CLK_SET_RATE_PARENT
is set, the parent clock is not which can lead to some relatively
inaccurate clock values for child clocks.

Unlike imx/clk-composite-93 and imx/clk-divider-gate, it
cannot rely on calling a standard clock function, because
the 8m composite clocks have a pre-divider and post-divider.
Because of this, a custom determine_rate function is
necessary to determine the maximum clock division which is
equivalent to pre-divider * the post-divider in most cases.

With this added, the system can attempt to adjust the parent rate
when the proper flags are set which can lead to a more precise clock
value.

For example.  When trying to get a pixel clock of 31.500MHz
on an imx8m Nano, the clocks divided the 594MHz down, but
left the parent rate untouched which caused a calulation error.

Before:
video_pll              594000000
  video_pll_bypass     594000000
    video_pll_out      594000000
      disp_pixel       31263158
        disp_pixel_clk 31263158

Variance = -236842 Hz

After this patch:
video_pll               31500000
  video_pll_bypass      31500000
    video_pll_out       31500000
      disp_pixel        31500000
        disp_pixel_clk  31500000

Variance = 0 Hz

All other clocks rates and parent were the same.
Similar results on imx8mm were found.

Unlike the previous attempt, when tested with imx8mp, no clock
rate or parent changes were found.

Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"")
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index cbf0d7955a00..fd49385bd2ae 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -119,10 +119,39 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
 	return ret;
 }
 
+int imx8m_divider_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	int prediv_value;
+	int div_value;
+
+	/* if read only, just return current value */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
+		u32 val;
+
+		val = readl(divider->reg);
+		prediv_value = val >> divider->shift;
+		prediv_value &= clk_div_mask(divider->width);
+
+		div_value = val >> PCG_DIV_SHIFT;
+		div_value &= clk_div_mask(PCG_DIV_WIDTH);
+
+		return divider_ro_determine_rate(hw, req, divider->table,
+						 PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+						 divider->flags, prediv_value * div_value);
+	}
+
+	return divider_determine_rate(hw, req, divider->table,
+				      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+				      divider->flags);
+}
+
 static const struct clk_ops imx8m_clk_composite_divider_ops = {
 	.recalc_rate = imx8m_clk_composite_divider_recalc_rate,
 	.round_rate = imx8m_clk_composite_divider_round_rate,
 	.set_rate = imx8m_clk_composite_divider_set_rate,
+	.determine_rate = imx8m_divider_determine_rate,
 };
 
 static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2023-06-13  8:24 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-06 19:53 [PATCH] clk: imx: composite-8m: Add imx8m_divider_determine_rate Adam Ford
2023-05-06 19:53 ` Adam Ford
2023-05-12  3:03 ` Adam Ford
2023-05-19 22:04   ` Adam Ford
2023-05-19 22:04     ` Adam Ford
2023-05-20 11:36     ` Peng Fan
2023-05-20 11:36       ` Peng Fan
2023-05-23  2:32 ` Peng Fan
2023-05-23  2:32   ` Peng Fan
2023-05-23  3:23   ` Adam Ford
2023-05-23  3:23     ` Adam Ford
2023-05-28  3:31     ` Adam Ford
2023-05-28  3:31       ` Adam Ford
2023-06-06 18:45 ` Fabio Estevam
2023-06-06 18:45   ` Fabio Estevam
2023-06-11 17:02   ` Adam Ford
2023-06-12 16:08     ` Maxime Ripard
2023-06-12 16:08       ` Maxime Ripard
2023-06-12 16:11       ` Adam Ford
2023-06-12 16:11         ` Adam Ford
2023-06-13  8:23         ` Maxime Ripard
2023-06-13  8:23           ` Maxime Ripard
2023-06-12  8:56 ` Abel Vesa
2023-06-12  9:32 ` Abel Vesa
  -- strict thread matches above, loose matches on Subject: below --
2023-05-06 16:24 Adam Ford
2023-05-06 16:24 ` Adam Ford
2023-05-06 17:41 ` kernel test robot
2023-05-06 18:12 ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.