> -----Original Message-----
> From: Deak, Imre
> Sent: Tuesday, June 19, 2018 3:43 PM
> To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Zanoni, Paulo R
> <paulo.r.zanoni@xxxxxxxxx>; Ausmus, James <james.ausmus@xxxxxxxxx>
> 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@xxxxxxxxx>
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Zanoni, Paulo R
> > > <paulo.r.zanoni@xxxxxxxxx>; Ausmus, James
> <james.ausmus@xxxxxxxxx>
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > > Cc: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>; Zanoni,
> > > > > Paulo R <paulo.r.zanoni@xxxxxxxxx>; Ausmus, James
> > > <james.ausmus@xxxxxxxxx>
> > > > > 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@xxxxxxxxx>
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > > > Cc: James Ausmus <james.ausmus@xxxxxxxxx>
> > > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > > Reviewed-by: James Ausmus <james.ausmus@xxxxxxxxx> (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@xxxxxxxxx>
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
> > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
