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

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

On Fri, Jul 13, 2018 at 11:43:47AM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-07-13 às 11:04 -0700, Rodrigo Vivi escreveu:
> > On Fri, Jul 13, 2018 at 10:20:27AM -0700, Paulo Zanoni wrote:
> > > Em Qui, 2018-07-12 às 17:16 -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,
> > > > 
> > > > I see 0x151 is the most common for 24MHz, but not the only one.
> > > > I also see 0x168 and 0x195, depending on the link rate.
> > > > 
> > > > So, what am I missing?
> > > 
> > > Are you looking at the table inside the Combo PLL section instead
> > > of
> > > the TBT PLL section? On that same page you are, scroll down by a
> > > lot
> > > until the "TBT PLL Divider Values" table inside the TBT section.
> > 
> > Ha! I knew I was missing something.
> > 
> > Could the order be inverted to match the spec order?
> > first line 19.2
> > second 24
> 
> The combo table has 24 first (both in BSpec and in the code), so this
> patch matches the order that's already in our code. Also the code
> checks usually put 24000 first (since "else" fits 19200 and 38400), so
> I kept the "24000 first" pattern. But I can paint it with the other
> color if you insist :).

Nope, I don't insist.

> 
> 
> > 
> > > 
> > > 
> > > > 
> > > > > +	.pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > .qdiv_ratio = 0,
> > 
> > Also, I see pdiv = 5 and and qdiv = 1...
> > 
> > but I'm sure that I'm missing something else again...
> > probably some mathmagic somewhere... where?
> 
> Look at the DPLL_CFGCR1 definition (page 15725).
> 
> Bit 9 value 0 means Q divider = 1.

I'm afraid there is a confusion here between qdiv ratio
and qdiv mode here.

Bit 9 you mention should be qdiv_mode which should be 0 for TBT indeed.

But qdiv ratio 1 should be qdiv = 1

> 
> Bits 8:6 value 100b means 3.
> 
> Bits 5:2 value 0100b means 5, 1000b means 7.

for these ones you are right.

> 
> Amazing, huh?
> 
> See the combo pll table: it includes both the bit numbers and the
> "original" numbers. The TBT one doesn't. That's very confusing, I know.
> Our table is supposed to have the bits to add to the registers, with
> the "real numbers" in comments. This also matches the combo tables
> defined just above.

I agree... no point... specially because I can't see an standard of the
math there, so better to do .pdiv = 0x4 /* 5 */ like everywhere else
is doing already anyways.

> 
> > 
> > anyways, if that is the case could we change to this table
> > here to match spec one?
> 
> I would prefer to keep as-is since otherwise we'd have to add code to
> translate from the table numbers to register bit numbers.
> 
> > 
> > Thanks,
> > Rodrigo.
> > 
> > > > > +};
> > > > > +
> > > > > +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,
> > > > > +};
> > > > > +
> > > > >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx