Quoting Ville Syrjälä (2023-12-04 17:05:46-03:00) >On Fri, Dec 01, 2023 at 05:13:38PM -0300, Gustavo Sousa wrote: >> Quoting Ville Syrjala (2023-11-28 08:51:33-03:00) >> >From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> > >> >Currently we have a hardcoded assumption that the cd2x divider >> >is always 2 when squahsing is used. >> >> It seems the code was actually making the assumption that the >> divider is always 1. With the current code, when squashing is used, we >> have that bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) is equivalent to >> bxt_cdclk_cd2x_div_sel(dev_priv, vco / 2, vco), meaning that the >> returned value will always be BXT_CDCLK_CD2X_DIV_SEL_1. > >The real cd2x divider is half of our 'divider' because we >specify the full vco->cdclk divider instead of the vco->cd2xclk >divider. Got it. > >Alternatively you can think of it as being the cd2x divider >specified in .1 binary fixed point format. > >But yeah, saying "cd2x divider is 2" probably isn't the best >way to put this. Yeah, maybe because of my little time with the driver code, but I had interpreted it as the one used right after the PLL output (I'm taking the diagram from MTL specs as reference). Did I miss some comment in the code explaning that? Should one be added? > >> >> > While that is true for all >> >current platforms it might not hold in the future. So eliminate >> >> Not sure about the other platforms, but for MTL, I have checked the >> BSpec table and also did some calculations by hand, and it seems like >> the cd2x divider is actually always 1. >> >> Unless I'm missing something in my analysis above, I believe s/2/1/ in >> the commit message is necessary. With that, >> >> Reviewed-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> Considering that the above understanding about the divider is the common sense, the r-b stands without the tweaks in the commit messages. Thanks for the clarification! -- Gustavo Sousa >> >> >the assumption and calculate the correct divider from the other >> >parameters. >> > >> >Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> >--- >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 ++---- >> > 1 file changed, 2 insertions(+), 4 deletions(-) >> > >> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >index 87d5e5b67c4e..d45071675629 100644 >> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >@@ -1899,10 +1899,8 @@ static void _bxt_set_cdclk(struct drm_i915_private *dev_priv, >> > >> > waveform = cdclk_squash_waveform(dev_priv, cdclk); >> > >> >- if (waveform) >> >- clock = vco / 2; >> >- else >> >- clock = cdclk; >> >+ clock = DIV_ROUND_CLOSEST(cdclk * cdclk_squash_len, >> >+ cdclk_squash_divider(waveform)); >> >> Nit: maybe take the opportunity to rename "clock" to "unsquashed" for >> clarity? >> >> > >> > if (HAS_CDCLK_SQUASH(dev_priv)) >> > dg2_cdclk_squash_program(dev_priv, waveform); >> >-- >> >2.41.0 >> > > >-- >Ville Syrjälä >Intel
