When adding imx8m_clk_divider_determine_rate to let the imx8mm
and imx8mn get finer granulatiry of video clocks, it accidentally
broke the imx8mp ability to divide clocks by more than 8 on clocks
when the CLK_SET_RATE_PARENT flag is not set.
On the imx8mp, the CLK_SET_RATE_PARENT flag cannot be set on either
media_disp1_pix or media_disp2_pix, because they both share the
video_pll as a common clock, and if two displays are used, the
parent clock needs to be something that both child clocks can
divide.
imx8m_clk_divider_determine_rate ends up calling clk_divider_bestdiv
which uses the value of 'width' to setup the maximum divisor. The
clk-composite-8m driver sets the with to 3 which means the maximum
divisor is 8, but these clocks can divide by up to 64.
Currently, if the video_pll is set to 1039500000, the slowest clock
rate achievable is 129937500 which breaks a whole bunch of lower
resolution and refresh options for both media_disp1_pix and
media_disp2_pix.
By changing the 'width' variable to PCG_DIV_WIDTH, the maximum
divisor becomes 64 which allows the clocks to divide down more,
so the lower resolutions and refresh rates are achievable again.
Fixes: 156e96ff2172 ("clk: imx: composite-8m: Add support to determine_rate")
Signed-off-by: Adam Ford <[email protected]>
diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index 6883a8199b6c..805e6aada83d 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -47,6 +47,7 @@ static unsigned long imx8m_clk_composite_divider_recalc_rate(struct clk_hw *hw,
divider->flags, PCG_DIV_WIDTH);
}
+
static int imx8m_clk_composite_compute_dividers(unsigned long rate,
unsigned long parent_rate,
int *prediv, int *postdiv)
@@ -215,12 +216,12 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
mux_ops = &imx8m_clk_composite_mux_ops;
} else if (composite_flags & IMX_COMPOSITE_BUS) {
div->shift = PCG_PREDIV_SHIFT;
- div->width = PCG_PREDIV_WIDTH;
+ div->width = PCG_DIV_WIDTH;
divider_ops = &imx8m_clk_composite_divider_ops;
mux_ops = &imx8m_clk_composite_mux_ops;
} else {
div->shift = PCG_PREDIV_SHIFT;
- div->width = PCG_PREDIV_WIDTH;
+ div->width = PCG_DIV_WIDTH;
divider_ops = &imx8m_clk_composite_divider_ops;
mux_ops = &clk_mux_ops;
if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
--
2.39.2
> Subject: [PATCH] clk: imx: composite-8m: Fix clock divisions greater than 8
>
> When adding imx8m_clk_divider_determine_rate to let the imx8mm and
> imx8mn get finer granulatiry of video clocks, it accidentally broke the
> imx8mp ability to divide clocks by more than 8 on clocks when the
> CLK_SET_RATE_PARENT flag is not set.
>
> On the imx8mp, the CLK_SET_RATE_PARENT flag cannot be set on either
> media_disp1_pix or media_disp2_pix, because they both share the
> video_pll as a common clock, and if two displays are used, the parent clock
> needs to be something that both child clocks can divide.
>
> imx8m_clk_divider_determine_rate ends up calling clk_divider_bestdiv
> which uses the value of 'width' to setup the maximum divisor. The clk-
> composite-8m driver sets the with to 3 which means the maximum divisor is
> 8, but these clocks can divide by up to 64.
>
> Currently, if the video_pll is set to 1039500000, the slowest clock rate
> achievable is 129937500 which breaks a whole bunch of lower resolution
> and refresh options for both media_disp1_pix and media_disp2_pix.
>
> By changing the 'width' variable to PCG_DIV_WIDTH, the maximum divisor
> becomes 64 which allows the clocks to divide down more, so the lower
> resolutions and refresh rates are achievable again.
>
> Fixes: 156e96ff2172 ("clk: imx: composite-8m: Add support to
> determine_rate")
> Signed-off-by: Adam Ford <[email protected]>
>
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-
> composite-8m.c
> index 6883a8199b6c..805e6aada83d 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -47,6 +47,7 @@ static unsigned long
> imx8m_clk_composite_divider_recalc_rate(struct clk_hw *hw,
> divider->flags, PCG_DIV_WIDTH);
> }
>
> +
> static int imx8m_clk_composite_compute_dividers(unsigned long rate,
> unsigned long parent_rate,
> int *prediv, int *postdiv)
> @@ -215,12 +216,12 @@ struct clk_hw *__imx8m_clk_hw_composite(const
> char *name,
> mux_ops = &imx8m_clk_composite_mux_ops;
> } else if (composite_flags & IMX_COMPOSITE_BUS) {
> div->shift = PCG_PREDIV_SHIFT;
> - div->width = PCG_PREDIV_WIDTH;
> + div->width = PCG_DIV_WIDTH;
This is wrong. Pre divider width is 3, not 6.
> divider_ops = &imx8m_clk_composite_divider_ops;
> mux_ops = &imx8m_clk_composite_mux_ops;
> } else {
> div->shift = PCG_PREDIV_SHIFT;
> - div->width = PCG_PREDIV_WIDTH;
> + div->width = PCG_DIV_WIDTH;
Ditto.
You need find the other way to resolve the regression.
Regards,
Peng.
> divider_ops = &imx8m_clk_composite_divider_ops;
> mux_ops = &clk_mux_ops;
> if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> --
> 2.39.2
On Sun, Apr 16, 2023 at 7:55 PM Peng Fan <[email protected]> wrote:
>
> > Subject: [PATCH] clk: imx: composite-8m: Fix clock divisions greater than 8
> >
> > When adding imx8m_clk_divider_determine_rate to let the imx8mm and
> > imx8mn get finer granulatiry of video clocks, it accidentally broke the
> > imx8mp ability to divide clocks by more than 8 on clocks when the
> > CLK_SET_RATE_PARENT flag is not set.
> >
> > On the imx8mp, the CLK_SET_RATE_PARENT flag cannot be set on either
> > media_disp1_pix or media_disp2_pix, because they both share the
> > video_pll as a common clock, and if two displays are used, the parent clock
> > needs to be something that both child clocks can divide.
> >
> > imx8m_clk_divider_determine_rate ends up calling clk_divider_bestdiv
> > which uses the value of 'width' to setup the maximum divisor. The clk-
> > composite-8m driver sets the with to 3 which means the maximum divisor is
> > 8, but these clocks can divide by up to 64.
> >
> > Currently, if the video_pll is set to 1039500000, the slowest clock rate
> > achievable is 129937500 which breaks a whole bunch of lower resolution
> > and refresh options for both media_disp1_pix and media_disp2_pix.
> >
> > By changing the 'width' variable to PCG_DIV_WIDTH, the maximum divisor
> > becomes 64 which allows the clocks to divide down more, so the lower
> > resolutions and refresh rates are achievable again.
> >
> > Fixes: 156e96ff2172 ("clk: imx: composite-8m: Add support to
> > determine_rate")
> > Signed-off-by: Adam Ford <[email protected]>
> >
> > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-
> > composite-8m.c
> > index 6883a8199b6c..805e6aada83d 100644
> > --- a/drivers/clk/imx/clk-composite-8m.c
> > +++ b/drivers/clk/imx/clk-composite-8m.c
> > @@ -47,6 +47,7 @@ static unsigned long
> > imx8m_clk_composite_divider_recalc_rate(struct clk_hw *hw,
> > divider->flags, PCG_DIV_WIDTH);
> > }
> >
> > +
> > static int imx8m_clk_composite_compute_dividers(unsigned long rate,
> > unsigned long parent_rate,
> > int *prediv, int *postdiv)
> > @@ -215,12 +216,12 @@ struct clk_hw *__imx8m_clk_hw_composite(const
> > char *name,
> > mux_ops = &imx8m_clk_composite_mux_ops;
> > } else if (composite_flags & IMX_COMPOSITE_BUS) {
> > div->shift = PCG_PREDIV_SHIFT;
> > - div->width = PCG_PREDIV_WIDTH;
> > + div->width = PCG_DIV_WIDTH;
> This is wrong. Pre divider width is 3, not 6.
>
> > divider_ops = &imx8m_clk_composite_divider_ops;
> > mux_ops = &imx8m_clk_composite_mux_ops;
> > } else {
> > div->shift = PCG_PREDIV_SHIFT;
> > - div->width = PCG_PREDIV_WIDTH;
> > + div->width = PCG_DIV_WIDTH;
> Ditto.
>
> You need find the other way to resolve the regression.
Peng,
Thank you for the review! It took me a bit to better understand the
clock driver, but I think I found a solution that seems to work
without breaking 8mp. I did a clock dump on the 8mp, and I found no
differences in the clock rates generated before and after my patch. I
will re-test the solution on the Mini and Nano. If all goes well, I
will post a V2 patch which both does a revert and posts the fix.
Instead of using a common function to determine_rate, I wrote one
specific for the composite-8m which uses max values from both the
pre-div and post-div to determine the possible clock rates. If the
read-only flag is set, it reads the existing values of the pre and
post dividers to determine whatever the divider is. It doesn't touch
any pre-existing composite-8m functions.
adam
>
> Regards,
> Peng.
>
> > divider_ops = &imx8m_clk_composite_divider_ops;
> > mux_ops = &clk_mux_ops;
> > if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> > --
> > 2.39.2
>