Message-ID: <CAHCN7xKUdveqVnOXXRL-RhXFrN4OUjJC+VgZguy1kYjx230wCw@mail.gmail.com>
Date: Mon, 31 Mar 2025 08:43:38 -0500
From: Adam Ford <aford173@...il.com>
To: Dominique Martinet <dominique.martinet@...ark-techno.com>
Cc: Uwe Kleine-König <u.kleine-koenig@...libre.com>, 
	Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>, 
	Frieder Schrempf <frieder.schrempf@...tron.de>, Marco Felsch <m.felsch@...gutronix.de>, 
	Lucas Stach <l.stach@...gutronix.de>, linux-phy@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Makoto Sato <makoto.sato@...ark-techno.com>
Subject: Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate
 instead LUT

On Mon, Mar 10, 2025 at 4:49 AM Dominique Martinet
<dominique.martinet@...ark-techno.com> wrote:
>
> Uwe Kleine-König wrote on Mon, Mar 10, 2025 at 10:14:23AM +0100:
> > > for 83.5mHz, the integer divider generates 83.2mHz (-0.36%), but the
> > > next LUT value (82.5mHz) is 1.2% off which incorrectly rejects modes
> > > requiring this frequency.
> >
> > Is the unit here MHz or mHz? I suspect the former?
>
> Err, yes MHz; I was still half asleep when I added that example to the
> commit message..
>
> > Without having looked in detail, I think it would be nice to reduce code
> > duplication between phy_clk_round_rate() and phy_clk_set_rate(). The
> > former has
> >
> >       if (rate > 297000000 || rate < 22250000)
> >               return -EINVAL;
> >
> > which seems to be lacking from the latter so I suspect there are more
> > differences between the two functions than fixed here?
>
> For this particular rate check, I believe that if phy_clk_round_rate()
> rejected the frequency then phy_clk_set_rate() cannot be called at all
> with the said value (at least on this particular setup going through the
> imx8mp-hdmi-tx bridge validating frequencies first before allowing
> modes), not that it'd hurt to recheck.

I believe that is true.  I considered adding it, but when I put debug
code in to trace what was happening, it was being rejected, in one
place, so the other didn't need to.  If the general consensus is to
have it in both places, I can add it.

>
>
> In general though I agree these are doing pretty much the same thing,
> with clk_round_rate() throwing away the pms values and there's more
> duplication than ideal...

I've been working on creating some caching to determine the best
values for the PHY and remember them, so the work doesn't have to be
done again if the next call uses the same clock rate.  I'm not quite
ready to submit it, because I need to rebase it on Linux-Next with
some of the other updates requested by Uwe.  My updates also remove
the look-up table completely and use an algorithm to determine the
fractional divider values - thanks to Frieder's python code that I
ported to C.  I experimented quite a bit with which values have more
impact and reorganized his nested for-loops to keep track of how many
iterations are done and also measuring the time it takes to do the
calculations, so the code doesn't really look like his as much as one
would think.

The downside with my updates is that running 'modetest'  on the 4K
monitor that I use has so many entries, the time it takes to calculate
all the values for the monitor takes a second or two longer than
before, because searching the LUT is quick and doing a series of
for-loops to find the nominal values is more time consuming.  We could
potentially keep the LUT and only use this new calculation if the
entry isn't in the LUT.  I am not generally a fan of LUT's if the
values can be calculated, but I can also see the advantage of speed.

> Unfortunately the code that computes registers for the integer divider
> does it in a global variable so just computing everything in
> round_rate() would forget what last setting was actually applied and
> break e.g. resume, but yes that's just refactoring that should be done.
>
> While we're here I also have no idea what recalc_rate() is supposed to
> do but that 'return 74250000;' is definitely odd, and I'm sure there are
> other improvements that could be made at the edges.

I am not sure where these values came from either.

>
>
> That's quite rude of me given I just sent the patch, but we probably
> won't have time to rework this until mid-april after some urgent work
> ends (this has actually been waiting for testing for 3 months
> already...)
> If this doesn't bother anyone else we can wait for a v2 then, but

If you want, I can submit my stuff as an RFC to give it a try and
solicit feedback.

> otherwise it might be worth considering getting as is until refactoring
> happens (and I pinky promise I'll find time before this summer -- I can
> send a v2 with commit message fixed up as Frieder suggested if this is
> acceptable to you)
>

adam
>
>
> Thanks for the quick feedback either way, and sorry for the long delay.
> --
> Dominique
>
>

Powered by blists - more mailing lists