Re: [PATCH 1/8] drm/i915/icl: compute the TBT PLL registers

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jul 13, 2018 at 03:57:45PM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-07-13 às 14:08 -0700, Rodrigo Vivi escreveu:
> > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > Use the hardcoded tables provided by our spec.
> > > 
> > > v2:
> > >   - SSC stays disabled.
> > >   - Use intel_port_is_tc().
> > > 
> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > icl_dp_combo_pll_19_2MHz_values[] = {
> > >  	  .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0},
> > >  };
> > >  
> > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values = {
> > > +	.dco_integer = 0x151, .dco_fraction = 0x4000,
> > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0,
> > > +};
> > > +
> > > +static const struct skl_wrpll_params icl_tbt_pll_19_2MHz_values =
> > > {
> > > +	.dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > .qdiv_ratio = 0,
> > 
> > in other words, in a cleaner view:
> > 
> > s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables
> 
> From the qdiv ratio bit:
> 
> "This field specifies the Q divider ratio. This field is only used when
> Qdiv Mode is set to Enable to get a divider value other than 1.".
> 
> So having 0 or 1 shouldn't matter since qdiv_mode is zero.
> 
> On the other hand, if you look at the Combo PLL table, when qdiv_mode=0
> it explicitly tells us to use qdiv_ratio=0, opposite to what you're
> asking.
> 
> Then the example below and also the DSI example later both keep saying
> to use qdiv_mode=0 + qdiv_ratio=0.
> 
> Now the TBT PLL example later asks for qdiv_mode=0 qdiv_ratio=1, which
> is what you're asking here, but it's the only the only thing that asks
> for that pattern in that way, which makes me believe that either it's
> wrong (unlikely) or it simply doesn't matter (as written in the bit
> definition).

(just adding a note about what we already discussed in pvt last friday)

On the TBL section there are 3 places they add qdiv = 1, so spec is clearly
non-sense because at same time it "indicates" that for qmode=0 qdiv is always 1
it asks driver to set, because qmode for TBL is always 0 anyways.

So in my view or bspec is just completely wrong or it is stating that it is
driver's responsibility to set it to 1.

Anyways since you already filed the bug against the bspec we could just move
to either way for now... and fix later...

However one thing still bugs me... the mismatch of code and spec table. Even
if spec tells it doesn't matter but they don't update the value there.

I can see more devs in the future getting confused because qdiv=1 is part of
spec's table and code's table mismatch setting it to 0.

> 
> > 
> > with that:
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > > +};
> > > +
> > >  static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > *dev_priv, int clock,
> > >  				  struct skl_wrpll_params
> > > *pll_params)
> > >  {
> > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > drm_i915_private *dev_priv, int clock,
> > >  	return true;
> > >  }
> > >  
> > > +static bool icl_calc_tbt_pll(struct drm_i915_private *dev_priv,
> > > int clock,
> > > +			     struct skl_wrpll_params *pll_params)
> > > +{
> > > +	*pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > +			icl_tbt_pll_24MHz_values :
> > > icl_tbt_pll_19_2MHz_values;
> > > +	return true;
> > > +}
> > > +
> > >  static bool icl_calc_dpll_state(struct intel_crtc_state
> > > *crtc_state,
> > >  				struct intel_encoder *encoder, int
> > > clock,
> > >  				struct intel_dpll_hw_state
> > > *pll_state)
> > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > intel_crtc_state *crtc_state,
> > >  	struct skl_wrpll_params pll_params = { 0 };
> > >  	bool ret;
> > >  
> > > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > > +	if (intel_port_is_tc(dev_priv, encoder->port))
> > > +		ret = icl_calc_tbt_pll(dev_priv, clock,
> > > &pll_params);
> > > +	else if (intel_crtc_has_type(crtc_state,
> > > INTEL_OUTPUT_HDMI))
> > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > &pll_params);
> > >  	else
> > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > &pll_params);
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx