Re: [PATCH] clk: imx: composite-8m: Fix clock divisions greater than 8

[Adam Ford]

On Sun, Apr 16, 2023 at 7:55 PM Peng Fan <peng.fan@xxxxxxx> 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 <aford173@xxxxxxxxx>
> >
> > 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
>