Re: [PATCH V5 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Frieder Schrempf]

On 04.09.24 3:40 PM, Frieder Schrempf wrote:
> On 04.09.24 4:32 AM, Adam Ford wrote:
>> Currently, if the clock values cannot be set to the exact rate,
>> the round_rate and set_rate functions use the closest value found in
>> the look-up-table.  In preparation of removing values from the LUT
>> that can be calculated evenly with the integer calculator, it's
>> necessary to ensure to check both the look-up-table and the integer
>> divider clock values to get the closest values to the requested
>> value.  It does this by measuring the difference between the
>> requested clock value and the closest value in both integer divider
>> calucator and the fractional clock look-up-table.
>>
>> Which ever has the smallest difference between them is returned as
>> the cloesest rate.
>>
>> Signed-off-by: Adam Ford <aford173@xxxxxxxxx>
>> Signed-off-by: Dominique Martinet <dominique.martinet@xxxxxxxxxxxxxxxxx>
>> ---
>> V5:  No Change
>> V4:  New to series
>> ---
>>  drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 40 +++++++++++++++-----
>>  1 file changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>> index 8f2c0082aa12..56b08e684179 100644
>> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>> @@ -550,7 +550,7 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
>>  static long phy_clk_round_rate(struct clk_hw *hw,
>>  			       unsigned long rate, unsigned long *parent_rate)
>>  {
>> -	u32 int_div_clk;
>> +	u32 int_div_clk, delta_int, delta_frac;
>>  	int i;
>>  	u16 m;
>>  	u8 p, s;
>> @@ -563,6 +563,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
>>  	for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
>>  		if (phy_pll_cfg[i].pixclk <= rate)
>>  			break;
>> +
>>  	/* If the rate is an exact match, return it now */
>>  	if (rate == phy_pll_cfg[i].pixclk)
>>  		return phy_pll_cfg[i].pixclk;
>> @@ -579,15 +580,21 @@ static long phy_clk_round_rate(struct clk_hw *hw,
>>  	if (int_div_clk == rate)
>>  		return int_div_clk;
>>  
>> -	/* Fall back to the closest value in the LUT */
>> -	return phy_pll_cfg[i].pixclk;
>> +	/* Calculate the differences and use the closest one */
>> +	delta_frac = (rate - phy_pll_cfg[i].pixclk);
>> +	delta_int = (rate - int_div_clk);
>> +
>> +	if (delta_int < delta_frac)
>> +		return int_div_clk;
>> +	else
>> +		return phy_pll_cfg[i].pixclk;

I would also prefer to use a helper for calculating the closest rate.
Something like:

static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
						 u32 int_div_clk,
						 u32 frac_div_clk)
{
	if ((rate - int_div_clk) < (rate - frac_div_clk))
		return int_div_clk;
	
	return frac_div_clk;
}

This can be used above:

return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
phy_pll_cfg[i].pixclk);

And also below in phy_clk_set_rate():

if (fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
phy_pll_cfg[i].pixclk) == int_div_clk)
		phy->cur_cfg = &calculated_phy_pll_cfg;
	else
		phy->cur_cfg = &phy_pll_cfg[i];


>>  }
>>  
>>  static int phy_clk_set_rate(struct clk_hw *hw,
>>  			    unsigned long rate, unsigned long parent_rate)
>>  {
>>  	struct fsl_samsung_hdmi_phy *phy = to_fsl_samsung_hdmi_phy(hw);
>> -	u32 int_div_clk;
>> +	u32 int_div_clk, delta_int, delta_frac;
>>  	int i;
>>  	u16 m;
>>  	u8 p, s;
>> @@ -602,19 +609,34 @@ static int phy_clk_set_rate(struct clk_hw *hw,
>>  		calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
>>  		/* pll_div_regs 3-6 are fixed and pre-defined already */
>>  		phy->cur_cfg  = &calculated_phy_pll_cfg;
> 
>                              ^ unneeded whitespace (comment belongs to
> patch 3/5)
> 
>> +		goto done;
>>  	} else {
>>  		/* Otherwise, search the LUT */
>> -		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>> -		for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
>> -			if (phy_pll_cfg[i].pixclk <= rate)
>> +		for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--) {
>> +			if (phy_pll_cfg[i].pixclk == rate) {
>> +				dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>> +				phy->cur_cfg = &phy_pll_cfg[i];
>> +				goto done;
>> +			}
>> +
>> +			if (phy_pll_cfg[i].pixclk < rate)
>>  				break;
>> +		}
>>  
>>  		if (i < 0)
>>  			return -EINVAL;
>> -
>> -		phy->cur_cfg = &phy_pll_cfg[i];
>>  	}
>>  
>> +	/* Calculate the differences for each clock against the requested value */
>> +	delta_frac = (rate - phy_pll_cfg[i].pixclk);
>> +	delta_int = (rate - int_div_clk);
>> +
>> +	/* Use the value closest to the desired */
>> +	if (delta_int < delta_frac)
>> +		phy->cur_cfg  = &calculated_phy_pll_cfg;
> 
>                              ^ unneeded whitespace
> 
> Are you sure that this is correct? The calculated_phy_pll_cfg is only
> set above if there is an exact match for the integer divider. I don't
> think it contains the data you expect here, or am I missing something?
> 
>> +	else
>> +		phy->cur_cfg = &phy_pll_cfg[i];
>> +done:
>>  	return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
>>  }
>>