Hi,
On Thu, Apr 21, 2022 at 07:49:12PM -0700, Stephen Boyd wrote:
> +Maxime
>
> Quoting Dmitry Baryshkov (2022-04-19 16:54:47)
> > Since the commit 948fb0969eae ("clk: Always clamp the rounded rate"),
> > the clk_core_determine_round_nolock() would clamp the requested rate
> > between min and max rates from the rate request. Normally these fields
> > would be filled by clk_core_get_boundaries() called from
> > clk_round_rate().
> >
> > However clk_gfx3d_determine_rate() uses a manually crafted rate request,
> > which did not have these fields filled. Thus the requested frequency
> > would be clamped to 0, resulting in weird frequencies being requested
> > from the hardware.
> >
> > Fix this by filling min_rate and max_rate to the values valid for the
> > respective PLLs (0 and ULONG_MAX).
> >
> > Fixes: 948fb0969eae ("clk: Always clamp the rounded rate")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
>
> I hope there aren't others like this lurking.
The problem is larger than that (even though I overlooked this
particular issue), and addressed partially by patches 12-19 here:
https://lore.kernel.org/linux-clk/20220408091037.2041955-1-maxime@xxxxxxxxxx/
I wanted to have your feedback before fixing the relevant drivers, but
these are:
* clk_divider:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-divider.c#L389
Only sunxi-ng is using divider_round_rate_parent, and I couldn't
find any clock with CLK_SET_RATE_PARENT, so this one is probably
minor. This one doesn't setup the boundaries and would probably
benefit from using clk_hw_init_rate_req.
* clk_composite:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-composite.c#L88
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-composite.c#L107
Both are doing an ok job at setting up the parent, but won't set up
the boundaries there. They setup the parent properly though, and
will update the "parent" request best_parent_* fields too. Switching
to clk_core_forward_rate_req would probably help maintenance a bit.
It's used in davinci_pll_obsclk_register,
mtk_clk_register_composite, lpc18xx_cgu_register_div,
lpc18xx_register_base_clk, lpc18xx_cgu_register_pll,
lpc32xx_clk_register, clk_pxa_cken_init and of_ti_composite_clk_setup.
It's not really clear to me whether these clocks have a
clk_round_rate / clk_set_rate on them, but it looks pretty bad.
* at91:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-generated.c#L135
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-master.c#L381
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-peripheral.c#L272
The issue is the same addressed in my series. The clk_rate_request
structure is forwarded as is and only the rate is updated. The
best_parent_* and boundaries aren't updated. It should be switched
to use clk_core_forward_rate_req or something equivalent.
* qcom:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/qcom/clk-rcg2.c#L821
This is the one affected by the patch. It's doing a better job at
filling the clk_rate_request, but indeed doesn't update the
boundaries. This patch is probably sane from an clk_hw
point-of-view, but it's broken if any user has set a boundary. It
should probably be switched to clk_core_forward_rate_req or similar
as well. Since the logic seems fairly intricate, I'm not sure if it
would be convenient though.
Maxime
Attachment:
signature.asc
Description: PGP signature
