Message-ID: <Z-sQFRaWxiGe6XEN@atmark-techno.com>
Date: Tue, 1 Apr 2025 06:58:45 +0900
From: Dominique Martinet <dominique.martinet@...ark-techno.com>
To: Adam Ford <aford173@...il.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


Adam Ford wrote on Mon, Mar 31, 2025 at 08:45:26AM -0500:
> If Dominique won't have time, I can try to clean this up a bit.  I was
> not liking the names either, but I was trying to keep them small.
> I'll default to this convention more in the future, based on your
> feedback.

I've been chasing weird suspend bugs on our platform (another soc) for
the best of the month so it'd be greatly appreciated, sorry.

Feel free to replace our patch with anything equivalent

Adam Ford wrote on Mon, Mar 31, 2025 at 08:43:38AM -0500:
> > 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.

Yes I think it will make the intent more clear, if we're going to share
some code or make it more consistent might as well go all the way.

> > 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.

Hmm, tough question... I don't think we want displays to show up a few
seconds later, we regularily get mails from customers asking how to get
a logo to show up faster on display (I think there may be some variant
of uboot that supports imx8mp hdmi? but we don't have that), so while I
see the appeal of not having an hardcoded look up table I'm not sure we
would appreciate that compromise.

I think it'd be great to just have a way of checking the values in the
LUT are correct though (either statically from Frieder's python code as
I started doing or through a CONFIG_WHATEVER_CHECK_LUT config item
that'd check once at boot, although that's probably overkill); I started
checking from the python code but they weren't computed with the same
algorithm so some values end up being different even if theend result is
the same frequency and I stopped halfway...

> > 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.

Happy to test/review anything you send, but I make no promise on delays
:-)

Cheers,
-- 
Dominique

Powered by blists - more mailing lists