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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZtgaOlHi93b_py7T@atmark-techno.com>
Date: Wed, 4 Sep 2024 17:28:42 +0900
From: Dominique Martinet <dominique.martinet@...ark-techno.com>
To: Adam Ford <aford173@...il.com>
Cc: linux-phy@...ts.infradead.org, linux-imx@....com, festevam@...il.com,
	frieder.schrempf@...tron.de, aford@...conembedded.com,
	Sandor.yu@....com, Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Lucas Stach <l.stach@...gutronix.de>,
	Marco Felsch <m.felsch@...gutronix.de>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4 3/5] phy: freescale: fsl-samsung-hdmi: Support dynamic
 integer

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@...il.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@...ark-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