| Message ID | 20180615143911.31082-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Deak, Imre > Sent: Friday, June 15, 2018 8:09 PM > To: intel-gfx@lists.freedesktop.org > Cc: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Zanoni, Paulo R > <paulo.r.zanoni@intel.com>; Ausmus, James <james.ausmus@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@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: James Ausmus <james.ausmus@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > Reviewed-by: James Ausmus <james.ausmus@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? > + 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. > 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
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@lists.freedesktop.org > > Cc: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Zanoni, Paulo R > > <paulo.r.zanoni@intel.com>; Ausmus, James <james.ausmus@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@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: James Ausmus <james.ausmus@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Reviewed-by: James Ausmus <james.ausmus@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 >
> -----Original Message----- > From: Deak, Imre > Sent: Monday, June 18, 2018 2:45 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > <paulo.r.zanoni@intel.com>; Ausmus, James <james.ausmus@intel.com> > Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is > 38.4MHz > > 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@lists.freedesktop.org > > > Cc: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Zanoni, Paulo R > > > <paulo.r.zanoni@intel.com>; Ausmus, James > <james.ausmus@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@intel.com> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Cc: James Ausmus <james.ausmus@intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > Reviewed-by: James Ausmus <james.ausmus@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. Thanks for the clarification. I saw that algo in the bspec said to program only when ref_clk!=38400 and leave as default in 38400 case. In which case we wouldn't need the masks. We can directly check the ref clk. I think, may be we can leave the programming to default for 38400 and handle it when bspec gets updated with new calculation. The rest of the patch looks good to me. > > > > + 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 > >
On Tue, Jun 19, 2018 at 09:07:25AM +0300, Kulkarni, Vandita wrote: > > -----Original Message----- > > From: Deak, Imre > > Sent: Monday, June 18, 2018 2:45 PM > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > > <paulo.r.zanoni@intel.com>; Ausmus, James <james.ausmus@intel.com> > > Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is > > 38.4MHz > > > > 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@lists.freedesktop.org > > > > Cc: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Zanoni, Paulo R > > > > <paulo.r.zanoni@intel.com>; Ausmus, James > > <james.ausmus@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@intel.com> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Cc: James Ausmus <james.ausmus@intel.com> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > Reviewed-by: James Ausmus <james.ausmus@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. > > Thanks for the clarification. I saw that algo in the bspec said to > program only when ref_clk!=38400 and leave as default in 38400 case. > In which case we wouldn't need the masks. Yes, and that's what this code does, we leave this register unchanged in case of ref_clk=38.4MHz. We do need the masks as the generic way to say which fields in which registers to change. This function doesn't need to care about some other state outside of intel_dpll_hw_state. > We can directly check the ref clk. I'd like to have the full state included in intel_dpll_hw_state, as that is used by the HW/SW state checker and the shared PLL framework to see if a PLL can be reused by doing a memcmp on this struct. > I think, may be we can leave the programming to default for 38400 and > handle it when bspec gets updated with new calculation. Yes, the patch leaves this register at its default in the 38.4MHz case. > > The rest of the patch looks good to me. > > > > > > + 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 > > >
> -----Original Message----- > From: Deak, Imre > Sent: Tuesday, June 19, 2018 3:43 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > <paulo.r.zanoni@intel.com>; Ausmus, James <james.ausmus@intel.com> > Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is > 38.4MHz > > On Tue, Jun 19, 2018 at 09:07:25AM +0300, Kulkarni, Vandita wrote: > > > -----Original Message----- > > > From: Deak, Imre > > > Sent: Monday, June 18, 2018 2:45 PM > > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > > > <paulo.r.zanoni@intel.com>; Ausmus, James > <james.ausmus@intel.com> > > > Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk > > > is 38.4MHz > > > > > > 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@lists.freedesktop.org > > > > > Cc: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Zanoni, > > > > > Paulo R <paulo.r.zanoni@intel.com>; Ausmus, James > > > <james.ausmus@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@intel.com> > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > Cc: James Ausmus <james.ausmus@intel.com> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > Reviewed-by: James Ausmus <james.ausmus@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. > > > > Thanks for the clarification. I saw that algo in the bspec said to > > program only when ref_clk!=38400 and leave as default in 38400 case. > > In which case we wouldn't need the masks. > > Yes, and that's what this code does, we leave this register unchanged in > case of ref_clk=38.4MHz. We do need the masks as the generic way to say > which fields in which registers to change. This function doesn't need to care > about some other state outside of intel_dpll_hw_state. > > > We can directly check the ref clk. > > I'd like to have the full state included in intel_dpll_hw_state, Ok. So the mask with 0 represents do not alter any bit of the register. Looks fine to me. Thanks for the clarification. Reviewed-by: Vandita Kulkarni <vandita.kulkarni@intel.com> as that is used > by the HW/SW state checker and the shared PLL framework to see if a PLL > can be reused by doing a memcmp on this struct. > > > I think, may be we can leave the programming to default for 38400 and > > handle it when bspec gets updated with new calculation. > > Yes, the patch leaves this register at its default in the 38.4MHz case. > > > > > The rest of the patch looks good to me. > > > > > > > > + 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 > > > >
On Tue, Jun 19, 2018 at 05:13:26PM +0000, Patchwork wrote: > == Series Details == > > Series: series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz (rev3) > URL : https://patchwork.freedesktop.org/series/44836/ > State : failure Thanks for the reviews pushed both patches to -dinq. See below for the explanation on failure. > > == Summary == > > = CI Bug Log - changes from CI_DRM_4343 -> Patchwork_9360 = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_9360 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_9360, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/44836/revisions/3/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_9360: > > === IGT changes === > > ==== Possible regressions ==== > > igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b: > fi-glk-j4005: PASS -> FAIL These changes are ICL specific, so should not affect GLK. > > > == Known issues == > > Here are the changes found in Patchwork_9360 that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b: > fi-snb-2520m: PASS -> INCOMPLETE (fdo#103713) > > > ==== Possible fixes ==== > > igt@kms_flip@basic-plain-flip: > fi-glk-j4005: DMESG-WARN (fdo#106000) -> PASS > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: > fi-glk-j4005: DMESG-WARN (fdo#106097) -> PASS > > > fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713 > fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000 > fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097 > > > == Participating hosts (42 -> 38) == > > Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-hsw-4200u > > > == Build changes == > > * Linux: CI_DRM_4343 -> Patchwork_9360 > > CI_DRM_4343: 93475d62c73031c5b4625eaa64b5c0f4079b2f3f @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4524: 9ab9268fa7eeda0a7ea6eb2ab02bb6c5b9c91ba0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_9360: 01529685e7fbbd57d3f7de473799a45bcd0783e0 @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > 01529685e7fb drm/i915/icl: Do read-modify-write as needed during MG PLL programming > e89ce575e104 drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9360/issues.html
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); + + 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); + 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; }; /**