| Message ID | 20250310-8ulp_hdmi-v1-1-a2f231e31987@atmark-techno.com |
|---|---|
| State | New |
| Headers | show |
| Series | phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT | expand |
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
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,
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.
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
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)