Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230416191051.106513-1-aford173@gmail.com> <DU0PR04MB9417F545EDAF40730E1C872D889C9@DU0PR04MB9417.eurprd04.prod.outlook.com>
In-Reply-To: <DU0PR04MB9417F545EDAF40730E1C872D889C9@DU0PR04MB9417.eurprd04.prod.outlook.com>
From:   Adam Ford <aford173@gmail.com>
Date:   Tue, 2 May 2023 15:55:04 -0500
Message-ID: <CAHCN7xKcbr4yVLpzberGTwJrkGp_y2d9UaPx5Eyq_nveMeQCjg@mail.gmail.com>
Subject: Re: [PATCH] clk: imx: composite-8m: Fix clock divisions greater than 8
To:     Peng Fan <peng.fan@nxp.com>
Cc:     "linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
        "aford@beaconembedded.com" <aford@beaconembedded.com>,
        Abel Vesa <abelvesa@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Apr 16, 2023 at 7:55=E2=80=AFPM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: [PATCH] clk: imx: composite-8m: Fix clock divisions greater th=
an 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 c=
lock
> > 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 rat=
e
> > 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@gmail.com>
> >
> > 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 =3D &imx8m_clk_composite_mux_ops;
> >       } else if (composite_flags & IMX_COMPOSITE_BUS) {
> >               div->shift =3D PCG_PREDIV_SHIFT;
> > -             div->width =3D PCG_PREDIV_WIDTH;
> > +             div->width =3D PCG_DIV_WIDTH;
> This is wrong. Pre divider width is 3, not 6.
>
> >               divider_ops =3D &imx8m_clk_composite_divider_ops;
> >               mux_ops =3D &imx8m_clk_composite_mux_ops;
> >       } else {
> >               div->shift =3D PCG_PREDIV_SHIFT;
> > -             div->width =3D PCG_PREDIV_WIDTH;
> > +             div->width =3D 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 =3D &imx8m_clk_composite_divider_ops;
> >               mux_ops =3D &clk_mux_ops;
> >               if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> > --
> > 2.39.2
>