[PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Dominique Martinet]

From: Makoto Sato <makoto.sato@atmark-techno.com>

If the requested rate is not an exact match of the integer divider
phy_clk_round_rate() would return the look up table value,
but phy_clk_set_rate() can still use the integer divider if it results
in a frequency that is closer than the look up table.

In particular, not returning the actually used value here made the hdmi
bridge driver reject a frequency that has an integer divider rate
within 0.5% of the target:
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.

This commit updates phy_clk_round_rate() to use the same logic as the
set operation.

Signed-off-by: Makoto Sato <makoto.sato@atmark-techno.com>
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
We're finally using this rewrite in our (outdated) tree and noticed the
"best" mode missing on one of our picky displays.
It all looks good with this fix, thanks again!
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index e4c0a82d16d9ef0f64ebf9e505b8620423cdc416..91c4d27a31f48fc49f1e8417d7089f5519b8a0a2 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -557,8 +557,9 @@ static long phy_clk_round_rate(struct clk_hw *hw,
 	if (int_div_clk == rate)
 		return int_div_clk;
 
-	/* If neither rate is an exact match, use the value from the LUT */
-	return fract_div_phy->pixclk;
+	/* If neither rate is an exact match, use the closest value */
+	return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
+						     fract_div_phy->pixclk);
 }
 
 static int phy_use_fract_div(struct fsl_samsung_hdmi_phy *phy, const struct phy_config *fract_div_phy)

---
base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
change-id: 20250310-8ulp_hdmi-f8deac08611e

Best regards,
-- 
Dominique Martinet <dominique.martinet@atmark-techno.com>



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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1635 bytes --]

Hello,

On Mon, Mar 10, 2025 at 10:21:32AM +0900, Dominique Martinet wrote:
> From: Makoto Sato <makoto.sato@atmark-techno.com>
> 
> If the requested rate is not an exact match of the integer divider
> phy_clk_round_rate() would return the look up table value,
> but phy_clk_set_rate() can still use the integer divider if it results
> in a frequency that is closer than the look up table.
> 
> In particular, not returning the actually used value here made the hdmi
> bridge driver reject a frequency that has an integer divider rate
> within 0.5% of the target:
> 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?

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?

Ideally the implementation would look conceptually like:

	static long phy_clk_round_rate(..., unsigned long rate, ...)
	{
		hw = phy_determine_register_settings_for(rate);
		if (hw_is_error(hw))
			return someerror;

		return phy_get_rate_from(hw);
	}

	static int phy_clk_set_rate(..., unsigned long rate, ...)
	{
		hw = phy_determine_register_settings_for(rate);
		if (hw_is_error(hw))
			return someerror;

		return phy_apply(hw);
	}

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Dominique Martinet]

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.


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


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



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



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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 366 bytes --]

Hello Dominique,

On Mon, Mar 10, 2025 at 06:48:50PM +0900, Dominique Martinet wrote:
> [...] and I'm sure there are other improvements that could be made at
> the edges.

One thing that irritated me is the function names. `phy_clk_round_rate`
sounds too generic. `fsl_samsung_hdmi_phy_clk_round_rate` is long, but
I'd say would be a better match.

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Adam Ford]

On Mon, Mar 10, 2025 at 11:12 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello Dominique,
>
> On Mon, Mar 10, 2025 at 06:48:50PM +0900, Dominique Martinet wrote:
> > [...] and I'm sure there are other improvements that could be made at
> > the edges.
>
> One thing that irritated me is the function names. `phy_clk_round_rate`
> sounds too generic. `fsl_samsung_hdmi_phy_clk_round_rate` is long, but
> I'd say would be a better match.

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.

adam
>
> Best regards
> Uwe

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Adam Ford]

On Mon, Mar 10, 2025 at 11:12 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello Dominique,
>
> On Mon, Mar 10, 2025 at 06:48:50PM +0900, Dominique Martinet wrote:
> > [...] and I'm sure there are other improvements that could be made at
> > the edges.
>
> One thing that irritated me is the function names. `phy_clk_round_rate`
> sounds too generic. `fsl_samsung_hdmi_phy_clk_round_rate` is long, but
> I'd say would be a better match.
Uwe,

I just sent a patch to rename round_rate and set rate functions to be
more explicit.  I also tried to refactor the driver as you requested
by simplifying the code, but I didn't have time to integrate my
fractional-divder code yet.

I will be traveling for most of May, so I won't be able to revisit
this again until after May 24.  Hopefully the patches I submitted meet
your satisfaction, and I hope they address some of the concerns from
Dominique.  I also CC'd Frieder, since he's been active with this
driver as well.

adam
>
> Best regards
> Uwe

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]

Hello Adam,

On Sun, May 04, 2025 at 03:44:26PM -0500, Adam Ford wrote:
> On Mon, Mar 10, 2025 at 11:12 AM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> >
> > Hello Dominique,
> >
> > On Mon, Mar 10, 2025 at 06:48:50PM +0900, Dominique Martinet wrote:
> > > [...] and I'm sure there are other improvements that could be made at
> > > the edges.
> >
> > One thing that irritated me is the function names. `phy_clk_round_rate`
> > sounds too generic. `fsl_samsung_hdmi_phy_clk_round_rate` is long, but
> > I'd say would be a better match.
> Uwe,
> 
> I just sent a patch to rename round_rate and set rate functions to be
> more explicit.  I also tried to refactor the driver as you requested
> by simplifying the code, but I didn't have time to integrate my
> fractional-divder code yet.
> 
> I will be traveling for most of May, so I won't be able to revisit
> this again until after May 24.  Hopefully the patches I submitted meet
> your satisfaction,

I don't feel authoritative for that driver, so please do stuff to please
the phy maintainers and consider my comments as suggestion from the side
line.

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Adam Ford]

On Mon, May 5, 2025 at 2:55 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello Adam,
>
> On Sun, May 04, 2025 at 03:44:26PM -0500, Adam Ford wrote:
> > On Mon, Mar 10, 2025 at 11:12 AM Uwe Kleine-König
> > <u.kleine-koenig@baylibre.com> wrote:
> > >
> > > Hello Dominique,
> > >
> > > On Mon, Mar 10, 2025 at 06:48:50PM +0900, Dominique Martinet wrote:
> > > > [...] and I'm sure there are other improvements that could be made at
> > > > the edges.
> > >
> > > One thing that irritated me is the function names. `phy_clk_round_rate`
> > > sounds too generic. `fsl_samsung_hdmi_phy_clk_round_rate` is long, but
> > > I'd say would be a better match.
> > Uwe,
> >
> > I just sent a patch to rename round_rate and set rate functions to be
> > more explicit.  I also tried to refactor the driver as you requested
> > by simplifying the code, but I didn't have time to integrate my
> > fractional-divder code yet.
> >
> > I will be traveling for most of May, so I won't be able to revisit
> > this again until after May 24.  Hopefully the patches I submitted meet
> > your satisfaction,
>
> I don't feel authoritative for that driver, so please do stuff to please
> the phy maintainers and consider my comments as suggestion from the side
> line.

I thought you had a good idea, and your suggestion made sense.
Getting that kind of feedback makes me a better programmer.

adam

>
> Best regards
> Uwe

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Adam Ford]

On Mon, Mar 10, 2025 at 4:49 AM Dominique Martinet
<dominique.martinet@atmark-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
>
>

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Dominique Martinet]

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



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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Adam Ford]

On Mon, Mar 31, 2025 at 4:59 PM Dominique Martinet
<dominique.martinet@atmark-techno.com> wrote:
>
>
> 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 only notice the delay when I run modetest to get a list of available
resolutions since it's trying every resolution to see if it can
achieve the desired clock rate.

If I run modetest to set the resolution, it's not really noticeable
because it's really only trying

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

As a compromise, I could leave the LUT, and fall back to the manual
calculations to both speed up the modetest look up, but to also
maximize the ability to sync new resolutions.  Even with what I have,
it's not perfect.  I skipped a few variables since my testing showed
they didn't have a huge impact and adding them increased the number of
calculations to determine the resolution.

> 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

No worries. I am just glad this driver is getting some love and enhancements.

adam
> :-)
>
> Cheers,
> --
> Dominique
>
>

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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

[Frieder Schrempf]

On 10.03.25 2:21 AM, Dominique Martinet wrote:
> From: Makoto Sato <makoto.sato@atmark-techno.com>
> 
> If the requested rate is not an exact match of the integer divider
> phy_clk_round_rate() would return the look up table value,
> but phy_clk_set_rate() can still use the integer divider if it results
> in a frequency that is closer than the look up table.
> 
> In particular, not returning the actually used value here made the hdmi
> bridge driver reject a frequency that has an integer divider rate
> within 0.5% of the target:
> 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.
> 
> This commit updates phy_clk_round_rate() to use the same logic as the
> set operation.
> 
> Signed-off-by: Makoto Sato <makoto.sato@atmark-techno.com>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>

This looks good to me.

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

But I think we should also add the following Fixes tag:

Fixes: 058ea4a06704 ("phy: freescale: fsl-samsung-hdmi: Use closest
divider")

> ---
> We're finally using this rewrite in our (outdated) tree and noticed the
> "best" mode missing on one of our picky displays.
> It all looks good with this fix, thanks again!
> ---
>  drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index e4c0a82d16d9ef0f64ebf9e505b8620423cdc416..91c4d27a31f48fc49f1e8417d7089f5519b8a0a2 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -557,8 +557,9 @@ static long phy_clk_round_rate(struct clk_hw *hw,
>  	if (int_div_clk == rate)
>  		return int_div_clk;
>  
> -	/* If neither rate is an exact match, use the value from the LUT */
> -	return fract_div_phy->pixclk;
> +	/* If neither rate is an exact match, use the closest value */
> +	return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
> +						     fract_div_phy->pixclk);
>  }
>  
>  static int phy_use_fract_div(struct fsl_samsung_hdmi_phy *phy, const struct phy_config *fract_div_phy)
> 
> ---
> base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
> change-id: 20250310-8ulp_hdmi-f8deac08611e
> 
> Best regards,


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