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

[Dominique Martinet <dominique.martinet@atmark-techno.com>]

Adam Ford wrote on Mon, Sep 02, 2024 at 08:30:45PM -0500:
> 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
> 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@gmail.com>

I've rechecked this series with abs() added in the later patch and this
looks fine; all modes I tried properly synced up with my monitor.
(except one but I don't see set_rate() being called for it so it's
something else)

(On a semi-unrelated note on my backport I get a "PLL failed to lock"
message for the first sync only, but everything seems to work regardless
even if there is no further set_rate(), so I'll pretend I didn't see
that... the old code just has a 20ms wait without any check so it's not
like it was any better... anyway that's unrelated to this serie)

I'm also confident enough set_rate() won't be called in parallel with
different rates for my device so I'm fine with the new global, letting
others complain if that's a problem for them.


So, feel free to add this to all 5 patches:
Test-by: Dominique Martinet <dominique.martinet@atmark-techno.com>

Just one style nitpick:

> @@ -453,29 +541,70 @@ 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;
>  	int i;
> +	u16 m;
> +	u8 p, s;
> +
> +	/* If the clock is out of range return error instead of searching */
> +	if (rate > 297000000 || rate < 22250000)
> +		return -EINVAL;
>  
> +	/* Check the look-up table */
>  	for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
>  		if (phy_pll_cfg[i].pixclk <= rate)
> -			return phy_pll_cfg[i].pixclk;
> +			break;
> +	/* If the rate is an exact match, return it now */
> +	if (rate == phy_pll_cfg[i].pixclk)
> +		return phy_pll_cfg[i].pixclk;
> +
> +	/*
> +	 * The math on the lookup table shows the PMS math yields a
> +	 * frequency 5 x pixclk.
> +	 * When we check the integer divider against the desired rate,
> +	 * multiply the rate x 5 and then divide the outcome by 5.
> +	 */
> +	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;

I still think it makes more sense to move the * 5, / 5 and comment
inside fsl_samsung_hdmi_phy_find_pms -- the other caller doesn't have
such the comment so it might look odd depending on where one started
looking.

-- 
Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy