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 <[email protected]>
---
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.
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
> 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 <[email protected]>
> ---
>
> 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?
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
On Mon, Mar 13, 2023 at 11:39 PM Peng Fan <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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/[email protected]/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
>
On Tue, Mar 14, 2023 at 6:13 AM Adam Ford <[email protected]> wrote:
>
> On Mon, Mar 13, 2023 at 11:39 PM Peng Fan <[email protected]> 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 <[email protected]>
> > > ---
> > >
> > > 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/[email protected]/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
> >