Message-ID: <CAHCN7xK8DyRa-NbAnQrZdkU279kHZqXAB2WCH96HgsULxzVKRw@mail.gmail.com>
Date:   Tue, 14 Mar 2023 06:13:57 -0500
From:   Adam Ford <aford173@...il.com>
To:     Peng Fan <peng.fan@....com>
Cc:     "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "l.stach@...gutronix.de" <l.stach@...gutronix.de>,
        Abel Vesa <abelvesa@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] clk: imx: Let IMX8MN_CLK_DISP_PIXEL set parent rate

On Mon, Mar 13, 2023 at 11:39 PM Peng Fan <peng.fan@....com> 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
> >     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@...il.com>
> > ---
> >
> > 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@pengutronix.de/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
>

Powered by blists - more mailing lists