Re: [RFC] clk: imx: Let IMX8MN_CLK_DISP_PIXEL set parent rate

[Adam Ford]

On Tue, Mar 14, 2023 at 6:13 AM Adam Ford <aford173@xxxxxxxxx> wrote:
>
> On Mon, Mar 13, 2023 at 11:39 PM Peng Fan <peng.fan@xxxxxxx> wrote:
> >
> > > Subject: [RFC] clk: imx: Let IMX8MN_CLK_DISP_PIXEL set parent rate
> > >
> > > By default the display pixel clock needs to be evenly divide down from
> > > 594MHz which rules out a significant number of resolution and refresh rates.
> > > The current clock tree looks something like:
> > >
> > >  video_pll                594000000
> > >   video_pll_bypass        594000000
> > >    video_pll_out          594000000

It looks like video_pll_out ultimately defined as a mux clock and
according to clk-mux.c, "* rate - rate is only affected by parent
switching. No clk_set_rate support"  Since disp_pixel tries to pass
this to its parent, it makes sense that it stops here.

Since the clock rate is set by the parent, does it make sense to look
for CLK_SET_RATE_PARENT flag and call a function to set the parent
rate if the flag ist set?

It seems like this would be the simplest without having to re-define
what the clock type is, but lets us pass a flag and only mux's with
that type could request the parent to set the clock rate.  This should
let us drop the video_pll clock lower, have a better chance of
successfully getting the ideal frequency for the LCDIF to sync, and
allow for a larger list of resolutions and refresh rates due to the
fact they are no longer limited to being evenly diviisable from 594
MHz.

> > >     disp_pixel            148500000
> > >      disp_pixel_clk       148500000
> > >
> > > To enable CLK_SET_RATE_PARENT on disp_pixel, a helper function needs to
> > > be added called imx8m_clk_hw_composite_flags which can pass the
> > > additional flag to the clock controller. Letting disp_pixel set video_pll_out
> > > rate should actually lower the clock rates of video_pll_bypass and video_pll
> > > as well, since those clocks are already configured to enable
> > > CLK_SET_RATE_PARENT.
> > >
> > > Signed-off-by: Adam Ford <aford173@xxxxxxxxx>
> > > ---
> > >
> > > This is an RFC, because even with this patch, the video_pll_out clock does
> > > not drop to 148500000 like I would expect.  The video_pll clock is a
> > > fractional pll, so it should be able to generate a significant number of
> > > optional clock frequencies to facilitate video.
> > [Peng Fan]
> >
> > Have you ever tried to directly set video pll out clk to the freq that you wanna?
>
> In the application I am using, I have the DSI connected to an HDMI
> bridge, so some resolutions and refresh rates work, as long as they
> are evenly divisible from 594000000.  I am testing a series that was
> recently submitted to enable DSI on the i.MX8M Mini and Nano.
>
> If I manually change the video_pll to different frequencies, I can get
> other resolutions and refresh rates to work, but it then breaks the
> ones that I had previously working.  NXP's downstream code [1] as a
> comment added code to the ADV7511 driver which filters out clocks that
> are not divisible from 594 to mask the issue. It's listed as a TODO,
> and I think the author is blaming the ADV7511 driver, but  from my
> experience it's from the LCDIF clock not being able to reach to the
> proper value.  I think fixing this would also fix NXP's TODO list as
> well.
>
> I had modified the MXSFB driver to set the video_pll in addition to
> setting the disp_plixel_clk and I was able to sync many different
> resolutions and refresh rates, but I was told [2] that the solution
> would be to fix the clock driver by setting the parent clock rate,
> which is how I got here.
>
> adam
>
> [1] - https://github.com/nxp-imx/linux-imx/blob/lf-5.15.y/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c#L80
> [2] - https://lore.kernel.org/linux-arm-kernel/20230313112937.GC7446@xxxxxxxxxxxxxx/T/
>
> >
> > Regards,
> > Peng.
> >
> > >
> > > diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> > > index af256ade554f..a116cc40d7d0 100644
> > > --- a/drivers/clk/imx/clk-imx8mn.c
> > > +++ b/drivers/clk/imx/clk-imx8mn.c
> > > @@ -470,7 +470,7 @@ static int imx8mn_clocks_probe(struct
> > > platform_device *pdev)
> > >       hws[IMX8MN_CLK_DRAM_ALT] =
> > > imx8m_clk_hw_fw_managed_composite("dram_alt",
> > > imx8mn_dram_alt_sels, base + 0xa000);
> > >       hws[IMX8MN_CLK_DRAM_APB] =
> > > imx8m_clk_hw_fw_managed_composite_critical("dram_apb",
> > > imx8mn_dram_apb_sels, base + 0xa080);
> > >
> > > -     hws[IMX8MN_CLK_DISP_PIXEL] =
> > > imx8m_clk_hw_composite("disp_pixel", imx8mn_disp_pixel_sels, base +
> > > 0xa500);
> > > +     hws[IMX8MN_CLK_DISP_PIXEL] =
> > > +imx8m_clk_hw_composite_flags("disp_pixel", imx8mn_disp_pixel_sels,
> > > base
> > > ++ 0xa500, CLK_SET_RATE_PARENT);
> > >       hws[IMX8MN_CLK_SAI2] = imx8m_clk_hw_composite("sai2",
> > > imx8mn_sai2_sels, base + 0xa600);
> > >       hws[IMX8MN_CLK_SAI3] = imx8m_clk_hw_composite("sai3",
> > > imx8mn_sai3_sels, base + 0xa680);
> > >       hws[IMX8MN_CLK_SAI5] = imx8m_clk_hw_composite("sai5",
> > > imx8mn_sai5_sels, base + 0xa780); diff --git a/drivers/clk/imx/clk.h
> > > b/drivers/clk/imx/clk.h index 689b3ad927c0..9977b512845b 100644
> > > --- a/drivers/clk/imx/clk.h
> > > +++ b/drivers/clk/imx/clk.h
> > > @@ -414,6 +414,10 @@ struct clk_hw *__imx8m_clk_hw_composite(const
> > > char *name,
> > >       _imx8m_clk_hw_composite(name, parent_names, reg, \
> > >                       0, IMX_COMPOSITE_CLK_FLAGS_DEFAULT)
> > >
> > > +#define imx8m_clk_hw_composite_flags(name, parent_names, reg, flags)
> > > \
> > > +     _imx8m_clk_hw_composite(name, parent_names, reg, \
> > > +                     0, IMX_COMPOSITE_CLK_FLAGS_DEFAULT |  flags)
> > > +
> > >  #define imx8m_clk_hw_composite_critical(name, parent_names, reg) \
> > >       _imx8m_clk_hw_composite(name, parent_names, reg, \
> > >                       0, IMX_COMPOSITE_CLK_FLAGS_CRITICAL)
> > > --
> > > 2.37.2
> >