Re: [PATCH V7 3/5] xphy: freescale: fsl-samsung-hdmi: Support dynamic integer

[Frieder Schrempf]

On 11.09.24 3:28 AM, Adam Ford wrote:
> There is currently a look-up table for a variety of resolutions.
> Since the phy has the ability to dynamically calculate the values
> necessary to use the intger divider which should allow more

                       ^ integer

> resolutions without having to update the look-up-table.
> 
> If the lookup table cannot find an exact match, fall back to the
> dynamic calculator of the integer divider.
> 
> Previously, the value of P was hard-coded to 1, this required an
> update to the phy_pll_cfg table to add in the extra value into the
> table, so if the value of P is calculated to be something else
> by the PMS calculator, the calculated_phy_pll_cfg structure
> can be used instead without having to keep track of which method
> was used.
> 
> Signed-off-by: Adam Ford <aford173@xxxxxxxxx>
> ---
> V7:  Refactored much of the code to create smaller helper functions
>      to eliminate redundant code and improve code flow and comment
>      readability.   Any t-b and s-o-b tags removed due to the
>      extent of the changes.
> 
> V6:  Fix comment typos and remove an unnecesary extra calculation
>      by using the cached value.
> V5:  No Change
> V4:  No Change
> V3:  Change size of pll_div_regs to include PHY_REG01 (P)
>      Create calculated_phy_pll_cfg to containe the values
>      Eliminate the PMS calculation from fsl_samsung_hdmi_phy_configure
>      Make the LUT primary and fall back to integer calculator in
>      phy_clk_round_rate.
>      Check the range right away to ensure it's reaonsable rather than
>      trying to find a clock only to learn it's outside the range.
>      Overall added notes and comments where stuff may not be intuitive.
> 
> V2:  Update phy_clk_round_rate and phy_clk_set_rate to both support
>      the integer clock PMS calculator.
> ---
>  drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 371 +++++++++++++------
>  1 file changed, 265 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 4f6874226f9a..49317a96f767 100644
[...]
> +
> +	/* The ref manual states the values of 'P' range from 1 to 11 */
> +	for (_p = 1; _p <= 11; ++_p) {
> +		for (_s = 1; _s <= 16; ++_s) {
> +			u64 tmp;
> +			u32 delta;
> +
> +			/* s must be one or even */
> +			if (_s > 1 && (_s & 0x01) == 1)
> +				_s++;
> +
> +			/* _s cannot be 14 per the TRM */
> +			if (_s == 14)
> +				continue;
> +
> +			/*
> +			 * TODO: Ref Manual doesn't state the range of _m
> +			 * so this should be further refined if possible.
> +			 * This range was set based on the original values
> +			 * in the lookup table
> +			 */
> +			tmp = (u64)fout * (_p * _s);
> +			do_div(tmp, 24 * MHZ);
> +			_m = tmp;
> +			if (_m < 0x30 || _m > 0x7b)
> +				continue;
> +
> +			/*
> +			 * Rev 2 of the Ref Manual states the
> +			 * VCO can range between 750MHz and
> +			 * 3GHz.  The VCO is assumed to be

                                ^ whitespace

> +			 * is assumed to be (M * f_ref) / P,

                           ^ duplicate "is assumed to be"

> +			 * where f_ref is 24MHz.
> +			 */
> +			tmp = (u64)_m * 24 * MHZ;
> +			do_div(tmp, _p);
> +			if (tmp < 750 * MHZ ||
> +			    tmp > 3000 * MHZ)
> +				continue;
> +
> +			/* Final frequency after post-divider */
> +			do_div(tmp, _s);
> +
> +			delta = abs(fout - tmp);
> +			if (delta < min_delta) {
> +				best_p = _p;
> +				best_s = _s;
> +				best_m = _m;
> +				min_delta = delta;
> +				best_freq = tmp;
> +			}
> +		}
> +	}
> +
> +	if (best_freq) {
> +		*p = best_p;
> +		*m = best_m;
> +		*s = best_s;
> +	}
> +
> +	return best_freq / 5;
> +}
> +
[...]
>  
>  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);
> -	int i;
> +	const struct phy_config *fract_div_phy;
> +	u32 int_div_clk;
> +	u16 m;
> +	u8 p, s;
>  
> -	for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> -		if (phy_pll_cfg[i].pixclk <= rate)
> -			break;
> +	/* Search the fractional divider lookup table */
> +	fract_div_phy = fsl_samsung_hdmi_phy_lookup_rate(rate);
>  
> -	if (i < 0)
> -		return -EINVAL;
> +	/* If the rate is an exact match, use that value */
> +	if (fract_div_phy->pixclk == rate)
> +		goto use_fract_div;

Actually I don't really like the jump here and the even more extensive
jumping in the next patch.

Why not simply create another function and do "return use_fract_div()"
here and below at the end of phy_clk_set_rate().

>  
> -	phy->cur_cfg = &phy_pll_cfg[i];
> +	/*
> +	 * If the rate from the fractional divder is not exact, check the integer divider,
> +	 * and use it if that value is an exact match.
> +	 */
> +	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate, &p, &m, &s);
> +	if (int_div_clk == rate) {
> +		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: integer divider rate = %u\n",
> +				   int_div_clk);
> +
> +		fsl_samsung_hdmi_calculate_phy(&calculated_phy_pll_cfg, int_div_clk, p, m, s);
> +		phy->cur_cfg  = &calculated_phy_pll_cfg;
> +		return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
> +	}
>  
> +	/*
> +	 * If neither the fractional divder nor the integer divder can find an exact value

                                     ^ divider              ^ divider

> +	 * fall back to using the fractional divider
> +	 */
> +use_fract_div:
> +	phy->cur_cfg = fract_div_phy;
> +	dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider rate = %u\n",
> +			   phy->cur_cfg->pixclk);
>  	return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
>  }
>