Re: [PATCH 04/10] clk: qcom: clk-rcg2: forward the gfx3d rate request to the parent

[johannes.goede@oss.qualcomm.com]

Hi,

On 19-Oct-26 3:41 PM, Dmitry Baryshkov wrote:
> GPU DVFS on SDM660 never changed the GFX3D rate. clk_gfx3d_determine_rate()
> builds the parent request by hand and leaves best_parent_rate at zero, so
> the PLL is asked to determine a rate for a zero parent rate and the new
> rate is dropped on the floor.
> 
> Use clk_hw_forward_rate_request(), which fills in the parent's limits and
> rate the way the core expects them.
> 
> Fixes: af1e62f2ffe2 ("clk: qcom: clk-rcg2: Take clock boundaries into consideration for gfx3d")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/clk/qcom/clk-rcg2.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index c44c51d860e1..9e3d7e5c81e5 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -1207,10 +1207,10 @@ 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 = { .min_rate = 0, .max_rate = ULONG_MAX };
> +	struct clk_rate_request parent_req;

Just a random drive-by comment: You are now leaving parent_req entirely uninitialized
filling it with whatever is on the stack. Where as before all values except for
max_rate would be filled with 0.

Are you sure this is what you want ? Maybe:

	struct clk_rate_request parent_req = {};

?

Regards,

Hans




>  	struct clk_rcg2_gfx3d *cgfx = to_clk_rcg2_gfx3d(hw);
>  	struct clk_hw *xo, *p0, *p1, *p2;
> -	unsigned long p0_rate;
> +	unsigned long p0_rate, parent_rate;
>  	u8 mux_div = cgfx->div;
>  	int ret;
>  
> @@ -1234,12 +1234,12 @@ static int clk_gfx3d_determine_rate(struct clk_hw *hw,
>  	if (mux_div == 0)
>  		mux_div = 1;
>  
> -	parent_req.rate = req->rate * mux_div;
> +	parent_rate = req->rate * mux_div;
>  
>  	/* This has to be a fixed rate PLL */
>  	p0_rate = clk_hw_get_rate(p0);
>  
> -	if (parent_req.rate == p0_rate) {
> +	if (parent_rate == p0_rate) {
>  		req->rate = req->best_parent_rate = p0_rate;
>  		req->best_parent_hw = p0;
>  		return 0;
> @@ -1247,7 +1247,7 @@ static int clk_gfx3d_determine_rate(struct clk_hw *hw,
>  
>  	if (req->best_parent_hw == p0) {
>  		/* Are we going back to a previously used rate? */
> -		if (clk_hw_get_rate(p2) == parent_req.rate)
> +		if (clk_hw_get_rate(p2) == parent_rate)
>  			req->best_parent_hw = p2;
>  		else
>  			req->best_parent_hw = p1;
> @@ -1257,16 +1257,9 @@ static int clk_gfx3d_determine_rate(struct clk_hw *hw,
>  		req->best_parent_hw = p2;
>  	}
>  
> -	clk_hw_get_rate_range(req->best_parent_hw,
> -			      &parent_req.min_rate, &parent_req.max_rate);
> +	clk_hw_forward_rate_request(hw, req, req->best_parent_hw, &parent_req,
> +				    parent_rate);
>  
> -	if (req->min_rate > parent_req.min_rate)
> -		parent_req.min_rate = req->min_rate;
> -
> -	if (req->max_rate < parent_req.max_rate)
> -		parent_req.max_rate = req->max_rate;
> -
> -	parent_req.best_parent_hw = req->best_parent_hw;
>  	ret = __clk_determine_rate(req->best_parent_hw, &parent_req);
>  	if (ret)
>  		return ret;
>