| Message ID | 20230416191051.106513-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: imx: composite-8m: Fix clock divisions greater than 8 | expand |
> Subject: [PATCH] clk: imx: composite-8m: Fix clock divisions greater than 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 clock > 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 rate > 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 = &imx8m_clk_composite_mux_ops; > } else if (composite_flags & IMX_COMPOSITE_BUS) { > div->shift = PCG_PREDIV_SHIFT; > - div->width = PCG_PREDIV_WIDTH; > + div->width = PCG_DIV_WIDTH; This is wrong. Pre divider width is 3, not 6. > divider_ops = &imx8m_clk_composite_divider_ops; > mux_ops = &imx8m_clk_composite_mux_ops; > } else { > div->shift = PCG_PREDIV_SHIFT; > - div->width = PCG_PREDIV_WIDTH; > + div->width = PCG_DIV_WIDTH; Ditto. You need find the other way to resolve the regression. Regards, Peng. > divider_ops = &imx8m_clk_composite_divider_ops; > mux_ops = &clk_mux_ops; > if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) > -- > 2.39.2
On Sun, Apr 16, 2023 at 7:55 PM Peng Fan <peng.fan@nxp.com> wrote: > > > Subject: [PATCH] clk: imx: composite-8m: Fix clock divisions greater than 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 clock > > 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 rate > > 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 = &imx8m_clk_composite_mux_ops; > > } else if (composite_flags & IMX_COMPOSITE_BUS) { > > div->shift = PCG_PREDIV_SHIFT; > > - div->width = PCG_PREDIV_WIDTH; > > + div->width = PCG_DIV_WIDTH; > This is wrong. Pre divider width is 3, not 6. > > > divider_ops = &imx8m_clk_composite_divider_ops; > > mux_ops = &imx8m_clk_composite_mux_ops; > > } else { > > div->shift = PCG_PREDIV_SHIFT; > > - div->width = PCG_PREDIV_WIDTH; > > + div->width = 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 = &imx8m_clk_composite_divider_ops; > > mux_ops = &clk_mux_ops; > > if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) > > -- > > 2.39.2 >
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 = &imx8m_clk_composite_mux_ops; } else if (composite_flags & IMX_COMPOSITE_BUS) { div->shift = PCG_PREDIV_SHIFT; - div->width = PCG_PREDIV_WIDTH; + div->width = PCG_DIV_WIDTH; divider_ops = &imx8m_clk_composite_divider_ops; mux_ops = &imx8m_clk_composite_mux_ops; } else { div->shift = PCG_PREDIV_SHIFT; - div->width = PCG_PREDIV_WIDTH; + div->width = PCG_DIV_WIDTH; divider_ops = &imx8m_clk_composite_divider_ops; mux_ops = &clk_mux_ops; if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
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 clock 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 rate 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>