[Freedreno] [PATCH] clk: qcom: clk-rcg2: fix gfx3d frequency calculation

[Maxime Ripard]

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 at linaro.org>
> > ---
> 
> 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@cerno.tech/

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/freedreno/attachments/20220422/863c4453/attachment.sig>