diff mbox series

[3/5] drm/i915/icl: Cross check the combo PLL WRPLL parameters wrt. hard-coded PLL freqs

Message ID 20200929002929.783620-4-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock | expand

Commit Message

Imre Deak Sept. 29, 2020, 12:29 a.m. UTC 
When selecting the WRPLL dividers for a given port clock/PLL freq, the
hard-coded PLL freq in a table entry can be calculated using the rest of
parameters in the same entry. Cross-check if the hard coded values match
what we calculate with the formula.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 26 ++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä Oct. 1, 2020, 4:44 p.m. UTC | #1 
On Tue, Sep 29, 2020 at 03:29:27AM +0300, Imre Deak wrote:
> When selecting the WRPLL dividers for a given port clock/PLL freq, the
> hard-coded PLL freq in a table entry can be calculated using the rest of
> parameters in the same entry. Cross-check if the hard coded values match
> what we calculate with the formula.

We've never done this on any other plaform I think. Why is this special?
Also, shouldn't the state checker catch this anyway?

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 26 ++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index e3370c8dccc8..ded2b2dfe319 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -3002,6 +3002,30 @@ static const struct skl_wrpll_params tgl_tbt_pll_38_4MHz_values = {
>  	.pdiv = 0, .kdiv = 0, .qdiv_mode = 0, .qdiv_ratio = 0,
>  };
>  
> +static int icl_wrpll_ref_clock(struct drm_i915_private *i915);
> +
> +static bool icl_dp_combo_pll_clock_match(struct drm_i915_private *i915, int clock,
> +					 const struct icl_combo_pll_params *p)
> +{
> +	int ref_clock = icl_wrpll_ref_clock(i915);
> +	int pll_freq;
> +	u32 pdiv;
> +	u32 qdiv;
> +	u32 kdiv;
> +
> +	cnl_wrpll_decode_divs(&p->wrpll, &pdiv, &qdiv, &kdiv);
> +
> +	pll_freq = skl_wrpll_calc_freq(ref_clock,
> +				       p->wrpll.dco_integer, p->wrpll.dco_fraction,
> +				       pdiv, qdiv, kdiv);
> +	drm_WARN_ON(&i915->drm, pll_freq != p->clock);
> +
> +	if (clock == pll_freq)
> +		return true;
> +
> +	return false;
> +}
> +
>  static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
>  				  struct skl_wrpll_params *pll_params)
>  {
> @@ -3014,7 +3038,7 @@ static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(icl_dp_combo_pll_24MHz_values); i++) {
> -		if (clock == params[i].clock) {
> +		if (icl_dp_combo_pll_clock_match(dev_priv, clock, &params[i])) {
>  			*pll_params = params[i].wrpll;
>  			return true;
>  		}
> -- 
> 2.25.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Oct. 1, 2020, 5 p.m. UTC | #2 
On Thu, Oct 01, 2020 at 07:44:29PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 03:29:27AM +0300, Imre Deak wrote:
> > When selecting the WRPLL dividers for a given port clock/PLL freq, the
> > hard-coded PLL freq in a table entry can be calculated using the rest of
> > parameters in the same entry. Cross-check if the hard coded values match
> > what we calculate with the formula.
> 
> We've never done this on any other plaform I think. Why is this special?

I think if we do this we should do at init time for all such tables.
Ideally we'd do it at compile time but no constexpr in C :(

> Also, shouldn't the state checker catch this anyway?
> 
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 26 ++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index e3370c8dccc8..ded2b2dfe319 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3002,6 +3002,30 @@ static const struct skl_wrpll_params tgl_tbt_pll_38_4MHz_values = {
> >  	.pdiv = 0, .kdiv = 0, .qdiv_mode = 0, .qdiv_ratio = 0,
> >  };
> >  
> > +static int icl_wrpll_ref_clock(struct drm_i915_private *i915);
> > +
> > +static bool icl_dp_combo_pll_clock_match(struct drm_i915_private *i915, int clock,
> > +					 const struct icl_combo_pll_params *p)
> > +{
> > +	int ref_clock = icl_wrpll_ref_clock(i915);
> > +	int pll_freq;
> > +	u32 pdiv;
> > +	u32 qdiv;
> > +	u32 kdiv;
> > +
> > +	cnl_wrpll_decode_divs(&p->wrpll, &pdiv, &qdiv, &kdiv);
> > +
> > +	pll_freq = skl_wrpll_calc_freq(ref_clock,
> > +				       p->wrpll.dco_integer, p->wrpll.dco_fraction,
> > +				       pdiv, qdiv, kdiv);
> > +	drm_WARN_ON(&i915->drm, pll_freq != p->clock);
> > +
> > +	if (clock == pll_freq)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
> >  				  struct skl_wrpll_params *pll_params)
> >  {
> > @@ -3014,7 +3038,7 @@ static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
> >  	int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(icl_dp_combo_pll_24MHz_values); i++) {
> > -		if (clock == params[i].clock) {
> > +		if (icl_dp_combo_pll_clock_match(dev_priv, clock, &params[i])) {
> >  			*pll_params = params[i].wrpll;
> >  			return true;
> >  		}
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
Imre Deak Oct. 1, 2020, 5:31 p.m. UTC | #3 
On Thu, Oct 01, 2020 at 07:44:29PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 03:29:27AM +0300, Imre Deak wrote:
> > When selecting the WRPLL dividers for a given port clock/PLL freq, the
> > hard-coded PLL freq in a table entry can be calculated using the rest of
> > parameters in the same entry. Cross-check if the hard coded values match
> > what we calculate with the formula.
> 
> We've never done this on any other plaform I think. Why is this special?

clock in icl_combo_pll_params is already defined by WRPLL params in the
same entry along with refclock. The driver needs to calculate already
this same clock when reading out the PLL HW state, so I thought it makes
sense to determine clock from WRPLL params when looking up an entry from
the PLL params table.

It's also used by the last patch in the patchset that needs to calculate
the clock both with the fractional divider WA applied and not applied.

> Also, shouldn't the state checker catch this anyway?

Afaics the PLL state verification only checks if the calculated /
programmed WRPLL parameters match what we read out.  But the point in
this patch was only to make the table lookup and the clock calculation
during HW readout uniform.

> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 26 ++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index e3370c8dccc8..ded2b2dfe319 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3002,6 +3002,30 @@ static const struct skl_wrpll_params tgl_tbt_pll_38_4MHz_values = {
> >  	.pdiv = 0, .kdiv = 0, .qdiv_mode = 0, .qdiv_ratio = 0,
> >  };
> >  
> > +static int icl_wrpll_ref_clock(struct drm_i915_private *i915);
> > +
> > +static bool icl_dp_combo_pll_clock_match(struct drm_i915_private *i915, int clock,
> > +					 const struct icl_combo_pll_params *p)
> > +{
> > +	int ref_clock = icl_wrpll_ref_clock(i915);
> > +	int pll_freq;
> > +	u32 pdiv;
> > +	u32 qdiv;
> > +	u32 kdiv;
> > +
> > +	cnl_wrpll_decode_divs(&p->wrpll, &pdiv, &qdiv, &kdiv);
> > +
> > +	pll_freq = skl_wrpll_calc_freq(ref_clock,
> > +				       p->wrpll.dco_integer, p->wrpll.dco_fraction,
> > +				       pdiv, qdiv, kdiv);
> > +	drm_WARN_ON(&i915->drm, pll_freq != p->clock);
> > +
> > +	if (clock == pll_freq)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
> >  				  struct skl_wrpll_params *pll_params)
> >  {
> > @@ -3014,7 +3038,7 @@ static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
> >  	int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(icl_dp_combo_pll_24MHz_values); i++) {
> > -		if (clock == params[i].clock) {
> > +		if (icl_dp_combo_pll_clock_match(dev_priv, clock, &params[i])) {
> >  			*pll_params = params[i].wrpll;
> >  			return true;
> >  		}
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index e3370c8dccc8..ded2b2dfe319 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -3002,6 +3002,30 @@  static const struct skl_wrpll_params tgl_tbt_pll_38_4MHz_values = {
 	.pdiv = 0, .kdiv = 0, .qdiv_mode = 0, .qdiv_ratio = 0,
 };
 
+static int icl_wrpll_ref_clock(struct drm_i915_private *i915);
+
+static bool icl_dp_combo_pll_clock_match(struct drm_i915_private *i915, int clock,
+					 const struct icl_combo_pll_params *p)
+{
+	int ref_clock = icl_wrpll_ref_clock(i915);
+	int pll_freq;
+	u32 pdiv;
+	u32 qdiv;
+	u32 kdiv;
+
+	cnl_wrpll_decode_divs(&p->wrpll, &pdiv, &qdiv, &kdiv);
+
+	pll_freq = skl_wrpll_calc_freq(ref_clock,
+				       p->wrpll.dco_integer, p->wrpll.dco_fraction,
+				       pdiv, qdiv, kdiv);
+	drm_WARN_ON(&i915->drm, pll_freq != p->clock);
+
+	if (clock == pll_freq)
+		return true;
+
+	return false;
+}
+
 static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
 				  struct skl_wrpll_params *pll_params)
 {
@@ -3014,7 +3038,7 @@  static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(icl_dp_combo_pll_24MHz_values); i++) {
-		if (clock == params[i].clock) {
+		if (icl_dp_combo_pll_clock_match(dev_priv, clock, &params[i])) {
 			*pll_params = params[i].wrpll;
 			return true;
 		}