Re: [Intel-gfx] [PATCH 3/5] drm/i915/icl: Cross check the combo PLL WRPLL parameters wrt. hard-coded PLL freqs

[Imre Deak <imre.deak@intel.com>]

On Thu, Oct 01, 2020 at 07:44:29PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 03:29:27AM +0300, Imre Deak wrote:
> > When selecting the WRPLL dividers for a given port clock/PLL freq, the
> > hard-coded PLL freq in a table entry can be calculated using the rest of
> > parameters in the same entry. Cross-check if the hard coded values match
> > what we calculate with the formula.
> 
> We've never done this on any other plaform I think. Why is this special?

clock in icl_combo_pll_params is already defined by WRPLL params in the
same entry along with refclock. The driver needs to calculate already
this same clock when reading out the PLL HW state, so I thought it makes
sense to determine clock from WRPLL params when looking up an entry from
the PLL params table.

It's also used by the last patch in the patchset that needs to calculate
the clock both with the fractional divider WA applied and not applied.

> Also, shouldn't the state checker catch this anyway?

Afaics the PLL state verification only checks if the calculated /
programmed WRPLL parameters match what we read out.  But the point in
this patch was only to make the table lookup and the clock calculation
during HW readout uniform.

> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 26 ++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index e3370c8dccc8..ded2b2dfe319 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3002,6 +3002,30 @@ static const struct skl_wrpll_params tgl_tbt_pll_38_4MHz_values = {
> >  	.pdiv = 0, .kdiv = 0, .qdiv_mode = 0, .qdiv_ratio = 0,
> >  };
> >  
> > +static int icl_wrpll_ref_clock(struct drm_i915_private *i915);
> > +
> > +static bool icl_dp_combo_pll_clock_match(struct drm_i915_private *i915, int clock,
> > +					 const struct icl_combo_pll_params *p)
> > +{
> > +	int ref_clock = icl_wrpll_ref_clock(i915);
> > +	int pll_freq;
> > +	u32 pdiv;
> > +	u32 qdiv;
> > +	u32 kdiv;
> > +
> > +	cnl_wrpll_decode_divs(&p->wrpll, &pdiv, &qdiv, &kdiv);
> > +
> > +	pll_freq = skl_wrpll_calc_freq(ref_clock,
> > +				       p->wrpll.dco_integer, p->wrpll.dco_fraction,
> > +				       pdiv, qdiv, kdiv);
> > +	drm_WARN_ON(&i915->drm, pll_freq != p->clock);
> > +
> > +	if (clock == pll_freq)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
> >  				  struct skl_wrpll_params *pll_params)
> >  {
> > @@ -3014,7 +3038,7 @@ static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
> >  	int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(icl_dp_combo_pll_24MHz_values); i++) {
> > -		if (clock == params[i].clock) {
> > +		if (icl_dp_combo_pll_clock_match(dev_priv, clock, &params[i])) {
> >  			*pll_params = params[i].wrpll;
> >  			return true;
> >  		}
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx