[Intel-gfx] [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz
Imre Deak
imre.deak at intel.com
Mon Jun 18 09:15:03 UTC 2018
On Mon, Jun 18, 2018 at 11:11:30AM +0300, Kulkarni, Vandita wrote:
>
>
> > -----Original Message-----
> > From: Deak, Imre
> > Sent: Friday, June 15, 2018 8:09 PM
> > To: intel-gfx at lists.freedesktop.org
> > Cc: Kulkarni, Vandita <vandita.kulkarni at intel.com>; Zanoni, Paulo R
> > <paulo.r.zanoni at intel.com>; Ausmus, James <james.ausmus at intel.com>
> > Subject: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is
> > 38.4MHz
> >
> > Atm we're zeroing out fields in MG_PLL_BIAS and
> > MG_PLL_TDC_COLDST_BIAS if refclk is 38.4MHz, whereas the spec tells us
> > to preserve them.
> > Although the calculated values mostly match the register defaults even for
> > the 38.4MHz case, there are some differences wrt. what BIOS programs (I
> > noticed at least differences in the MG_PLL_BIAS/IREFTRIM and
> > MG_PLL_BIAS/BIASCAL_EN fields). In the lack of further info on how to
> > program these fields, just do what the spec says and preserve the BIOS
> > state.
> >
> > v2:
> > - Preserve the BIOS programmed reg fields instead of programming them.
> >
> > Cc: Vandita Kulkarni <vandita.kulkarni at intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Cc: James Ausmus <james.ausmus at intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > Reviewed-by: James Ausmus <james.ausmus at intel.com> (v1)
> > ---
> > drivers/gpu/drm/i915/intel_dpll_mgr.c | 63
> > +++++++++++++++++++++++++----------
> > drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 ++
> > 2 files changed, 47 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 132fe63e042a..d4c7bacbe83e 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -2812,25 +2812,31 @@ static bool icl_calc_mg_pll_state(struct
> > intel_crtc_state *crtc_state,
> > MG_PLL_SSC_FLLEN |
> > MG_PLL_SSC_STEPSIZE(ssc_stepsize);
> >
> > - pll_state->mg_pll_tdc_coldst_bias =
> > MG_PLL_TDC_COLDST_COLDSTART;
> > + pll_state->mg_pll_tdc_coldst_bias =
> > MG_PLL_TDC_COLDST_COLDSTART |
> > +
> > MG_PLL_TDC_COLDST_IREFINT_EN |
> > +
> > MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |
> > + MG_PLL_TDC_TDCOVCCORR_EN |
> > + MG_PLL_TDC_TDCSEL(3);
> >
> > - if (refclk_khz != 38400) {
> > - pll_state->mg_pll_tdc_coldst_bias |=
> > - MG_PLL_TDC_COLDST_IREFINT_EN |
> > -
> > MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |
> > - MG_PLL_TDC_COLDST_COLDSTART |
> > - MG_PLL_TDC_TDCOVCCORR_EN |
> > - MG_PLL_TDC_TDCSEL(3);
> > + pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
> > + MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
> > + MG_PLL_BIAS_BIAS_BONUS(10) |
> > + MG_PLL_BIAS_BIASCAL_EN |
> > + MG_PLL_BIAS_CTRIM(12) |
> > + MG_PLL_BIAS_VREF_RDAC(4) |
> > + MG_PLL_BIAS_IREFTRIM(iref_trim);
> >
> > - pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
> > - MG_PLL_BIAS_INIT_DCOAMP(0x3F)
> > |
> > - MG_PLL_BIAS_BIAS_BONUS(10) |
> > - MG_PLL_BIAS_BIASCAL_EN |
> > - MG_PLL_BIAS_CTRIM(12) |
> > - MG_PLL_BIAS_VREF_RDAC(4) |
> > - MG_PLL_BIAS_IREFTRIM(iref_trim);
> > + if (refclk_khz == 38400) {
> > + pll_state->mg_pll_tdc_coldst_bias_mask =
> > MG_PLL_TDC_COLDST_COLDSTART;
> > + pll_state->mg_pll_bias_mask = 0;
> > + } else {
> > + pll_state->mg_pll_tdc_coldst_bias_mask = -1U;
> > + pll_state->mg_pll_bias_mask = -1U;
> > }
> >
> > + pll_state->mg_pll_tdc_coldst_bias &= pll_state-
> > >mg_pll_tdc_coldst_bias_mask;
> > + pll_state->mg_pll_bias &= pll_state->mg_pll_bias_mask;
> > +
> > return true;
> > }
> >
> > @@ -2948,9 +2954,21 @@ static bool icl_pll_get_hw_state(struct
> > drm_i915_private *dev_priv,
> > hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
> > hw_state->mg_pll_frac_lock =
> > I915_READ(MG_PLL_FRAC_LOCK(port));
> > hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(port));
> > +
> > hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(port));
> > hw_state->mg_pll_tdc_coldst_bias =
> > I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
> > +
> > + if (dev_priv->cdclk.hw.ref == 38400) {
> > + hw_state->mg_pll_tdc_coldst_bias_mask =
> > MG_PLL_TDC_COLDST_COLDSTART;
> > + hw_state->mg_pll_bias_mask = 0;
> > + } else {
> > + hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
> > + hw_state->mg_pll_bias_mask = -1U;
> > + }
> > +
> > + hw_state->mg_pll_tdc_coldst_bias &= hw_state-
> > >mg_pll_tdc_coldst_bias_mask;
> > + hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
> > break;
> > default:
> > MISSING_CASE(id);
> > @@ -2978,6 +2996,7 @@ static void icl_mg_pll_write(struct
> > drm_i915_private *dev_priv, {
> > struct intel_dpll_hw_state *hw_state = &pll->state.hw_state;
> > enum port port = icl_mg_pll_id_to_port(pll->info->id);
> > + u32 val;
> >
> > I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
> > I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
> > @@ -2988,9 +3007,17 @@ static void icl_mg_pll_write(struct
> > drm_i915_private *dev_priv,
> > I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
> > I915_WRITE(MG_PLL_FRAC_LOCK(port), hw_state-
> > >mg_pll_frac_lock);
> > I915_WRITE(MG_PLL_SSC(port), hw_state->mg_pll_ssc);
> > - I915_WRITE(MG_PLL_BIAS(port), hw_state->mg_pll_bias);
> > - I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port),
> > - hw_state->mg_pll_tdc_coldst_bias);
> > +
> > + val = I915_READ(MG_PLL_BIAS(port));
> > + val &= ~hw_state->mg_pll_bias_mask;
> > + val |= hw_state->mg_pll_bias;
> > + I915_WRITE(MG_PLL_BIAS(port), val);
> > +
> Looks like we are writing the exact read value for pll_bias when
> ref_clk is 38400, we can skip this write or is it mandatory to write?
I'm still hoping that we'll get the proper programming steps for the
38.4MHz case too, at which point we can remove the special casing during
the calculation step and write here the calculated values in all cases.
It's also not a performance critical part, so I'd rather avoid adding
more special casing.
> > + val = I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
> > + val &= ~hw_state->mg_pll_tdc_coldst_bias_mask;
> > + val |= hw_state->mg_pll_tdc_coldst_bias;
> > + I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port), val);
> > +
> Same here.
We have to write MG_PLL_TDC_COLDST_COLDSTART in all cases.
> > POSTING_READ(MG_PLL_TDC_COLDST_BIAS(port));
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index ba925c7ee482..7e522cf4f13f 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -180,6 +180,8 @@ struct intel_dpll_hw_state {
> > uint32_t mg_pll_ssc;
> > uint32_t mg_pll_bias;
> > uint32_t mg_pll_tdc_coldst_bias;
> > + uint32_t mg_pll_bias_mask;
> > + uint32_t mg_pll_tdc_coldst_bias_mask;
> > };
> >
> > /**
> > --
> > 2.13.2
>
More information about the Intel-gfx
mailing list