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

[Hans de Goede]

Hi Again :)

On 19-Oct-26 4:33 PM, johannes.goede@xxxxxxxxxxxxxxxx wrote:
> 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@xxxxxxxxxxxxxxxx>
>> ---
>>  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 = {};
> 
> ?

Hmm, I guess that clk_hw_forward_rate_request() now fully fills in the passed
in parent_req, so this change is ok.

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