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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Seg, 2018-07-16 às 15:47 -0700, Rodrigo Vivi escreveu:
> 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.

As I explained in person, I don't think there is a mismatch of code and
table. When the spec says "qdiv" it means the high-level qdiv value,
which is set through the combination of qdiv_mode and qdiv_ratio.

So the TBT table says "qdiv=1", which you can achieve by setting
qdiv_mode=0 qdiv_ratio=whatever. 

The Combo table is more explicit that it lists both qdiv_mode and
qdiv_ratio to be set, and it also lists the real "qdiv" as a number in
parens on the qdiv_ratio column.

The only thing that really "mismatches" is that in the examples, when
qdiv is 1, in three places they say we need qdiv_mode=0 qdiv_ratio=0
and in the TBT example they say to use qdiv_mode=0 and qdiv_ratio=1.
But that perceived "mismatch" shouldn't matter since when qdiv_mode=0
qdiv_ratio can be anything. Still inconsistent, but IMHO irrelevant.

> 
> 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.

I think your confusion is that you don't see qdiv as a thing that is
achieved through qdiv_mode and qdiv_ratio.

> 
> > 
> > > 
> > > 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