On Mon, Dec 04, 2023 at 05:46:28PM -0300, Gustavo Sousa wrote:
> 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?
Looks like we've at least attempted to document this:
struct intel_cdclk_vals {
u32 cdclk;
u16 refclk;
u16 waveform;
u8 divider; /* CD2X divider * 2 */
u8 ratio;
};
>
> >
> >>
> >> > 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!
I can try to tweak it a bit anyway for extra clarity.
And thanks for the review.
>
> --
> 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
--
Ville Syrjälä
Intel
