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

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

[Dmitry Baryshkov]

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@linaro.org>
---
 drivers/clk/qcom/clk-rcg2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index f675fd969c4d..e9c357309fd9 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -818,7 +818,7 @@ EXPORT_SYMBOL_GPL(clk_pixel_ops);
 static int clk_gfx3d_determine_rate(struct clk_hw *hw,
 				    struct clk_rate_request *req)
 {
-	struct clk_rate_request parent_req = { };
+	struct clk_rate_request parent_req = { .min_rate = 0, .max_rate = ULONG_MAX };
 	struct clk_rcg2_gfx3d *cgfx = to_clk_rcg2_gfx3d(hw);
 	struct clk_hw *xo, *p0, *p1, *p2;
 	unsigned long p0_rate;
-- 
2.35.1

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

[Bjorn Andersson]

On Tue 19 Apr 18:54 CDT 2022, Dmitry Baryshkov wrote:

> 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@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Stephen, please pick this for -fixes.

Thanks,
Bjorn

> ---
>  drivers/clk/qcom/clk-rcg2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index f675fd969c4d..e9c357309fd9 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -818,7 +818,7 @@ EXPORT_SYMBOL_GPL(clk_pixel_ops);
>  static int clk_gfx3d_determine_rate(struct clk_hw *hw,
>  				    struct clk_rate_request *req)
>  {
> -	struct clk_rate_request parent_req = { };
> +	struct clk_rate_request parent_req = { .min_rate = 0, .max_rate = ULONG_MAX };
>  	struct clk_rcg2_gfx3d *cgfx = to_clk_rcg2_gfx3d(hw);
>  	struct clk_hw *xo, *p0, *p1, *p2;
>  	unsigned long p0_rate;
> -- 
> 2.35.1
> 

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

[Bjorn Andersson]

On Tue 19 Apr 18:54 CDT 2022, Dmitry Baryshkov wrote:

> 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")

Reported-by: Rob Clark <robdclark@chromium.org>

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-rcg2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index f675fd969c4d..e9c357309fd9 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -818,7 +818,7 @@ EXPORT_SYMBOL_GPL(clk_pixel_ops);
>  static int clk_gfx3d_determine_rate(struct clk_hw *hw,
>  				    struct clk_rate_request *req)
>  {
> -	struct clk_rate_request parent_req = { };
> +	struct clk_rate_request parent_req = { .min_rate = 0, .max_rate = ULONG_MAX };
>  	struct clk_rcg2_gfx3d *cgfx = to_clk_rcg2_gfx3d(hw);
>  	struct clk_hw *xo, *p0, *p1, *p2;
>  	unsigned long p0_rate;
> -- 
> 2.35.1
> 

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

[Stephen Boyd]

+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@linaro.org>
> ---

I hope there aren't others like this lurking.

>  drivers/clk/qcom/clk-rcg2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index f675fd969c4d..e9c357309fd9 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -818,7 +818,7 @@ EXPORT_SYMBOL_GPL(clk_pixel_ops);
>  static int clk_gfx3d_determine_rate(struct clk_hw *hw,
>                                     struct clk_rate_request *req)
>  {
> -       struct clk_rate_request parent_req = { };
> +       struct clk_rate_request parent_req = { .min_rate = 0, .max_rate = ULONG_MAX };
>         struct clk_rcg2_gfx3d *cgfx = to_clk_rcg2_gfx3d(hw);
>         struct clk_hw *xo, *p0, *p1, *p2;
>         unsigned long p0_rate;

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]

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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Stephen Boyd]

Quoting Maxime Ripard (2022-04-22 02:48:17)
> 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@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:

Ok. Let me move the conversation over to that thread. I'm applying this
to clk-fixes.