| Message ID | 20230314024449.50151-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [RFC] clk: imx: Let IMX8MN_CLK_DISP_PIXEL set parent rate | expand |
> 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@gmail.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? 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 <peng.fan@nxp.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@gmail.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 >
On Tue, Mar 14, 2023 at 6:13 AM Adam Ford <aford173@gmail.com> wrote: > > On Mon, Mar 13, 2023 at 11:39 PM Peng Fan <peng.fan@nxp.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 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@gmail.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 > >
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)
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@gmail.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.