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

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

On Mon, Jul 16, 2018 at 04:05:52PM -0700, Paulo Zanoni wrote:
> 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.

ok, so this is the part that we disagree mostly ;)
I look to spec and there is qdiv = 1 I look to the code and I see qdiv = 0
for my average developer brain this is a mismatch.

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

I know, but I shouldn't have to look to other table than the tbl one to
check tbt code and arrive to conclusions...  Actually you pointed me that
table at first hand because I was looking to the Combo one instead of the TBT :P

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

on Combo section... on TBT section itself they all mention qdiv=1.

> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx